From 5a3e07b71047866757d021b0971b0cc0f70356a3 Mon Sep 17 00:00:00 2001 From: illodev Date: Fri, 7 Aug 2026 23:26:57 +0200 Subject: [PATCH 1/3] CI runs the checks a card declares, and a second job records what they proved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T-0189, the `ci` tier of ADR-0016 and the only one with a witness. `local` is a command that ran on the author's machine and is still self-reported. The three questions the card said to settle first are settled in its notes. The one that shaped everything: the run and the write are two jobs, and they cannot be one. A criterion bound to a command can only be checked by running it, so one job executes commands a pull request declared and therefore holds `permissions: {}` with no credentials left in .git/config. Writing evidence needs `contents: write`, so the other holds it and runs no repository code at all — not even Workfile, because every Workfile command import()s project.config.mjs from the checkout. It applies a patch bounded to the protocol directory and pushes. A fork records nothing: GitHub issues a read-only token there whatever the workflow says, and the job declines to start in order to say so. CI closes a card only when every one of its criteria is bound to a command. A narrative criterion is not something a runner has an opinion about, so a card carrying one gets its bound boxes written and stays open with the reason reported. That conservatism is the whole safety of the write-back. T-0161 rides along as the dogfood, and its premise was wrong in a way worth recording. It says the fix is one branch in `validateCardCandidate` because `candidate.id` is set by then, and it is not: creation validates against `id: "pending"` and the allocation decides the id later, under a lock. A self `parent` on create is refused by `CARD_PARENT_NOT_FOUND` — the right outcome for the wrong reason — and `origin` has no existence rule to borrow, which is exactly why nothing caught it. So the guard sits at the allocation, where the id exists, and a `ValidationError` there leaves the retry loop rather than being read as a collision and retried onto the next id. Its four machine-decidable criteria are left unchecked on purpose. This branch touches the card, so the run should check them and push the commit that records it — which is the evidence criterion 1 of T-0189 asks for and the only thing a local run cannot produce. T-0188 left a tripwire for this card, asserting that no generated target contains `card verify` or an Actions expression. What it protected stands and is now stated as the two rules it stood for; the details are on T-0189. My first version of the replacement was broken in the dangerous direction and a mutation caught it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01D3LTdq3mzMAQ98rwBegGjU --- .github/workflows/workfile.yml | 114 ++++++- ...an-be-written-with-itself-as-its-origin.md | 15 +- ...ard-s-declared-checks-and-writes-back-t.md | 29 +- ...d-declares-and-records-what-they-proved.md | 12 + packages/workfile/bin/workfile.ts | 68 ++++- packages/workfile/docs/cli.md | 41 +++ .../workfile/src/modules/cards/changed.ts | 278 ++++++++++++++++++ packages/workfile/src/modules/cards/git.ts | 50 +++- packages/workfile/src/modules/cards/index.ts | 5 + .../workfile/src/modules/cards/mutations.ts | 24 ++ .../workfile/src/modules/cards/validation.ts | 15 + packages/workfile/src/modules/ci/ci.ts | 153 ++++++++++ packages/workfile/test/ci-targets.test.ts | 135 ++++++++- packages/workfile/test/self-reference.test.ts | 225 ++++++++++++++ project.config.mjs | 8 + 15 files changed, 1146 insertions(+), 26 deletions(-) create mode 100644 .project/changelog/unreleased/CHG-0154-ci-runs-the-checks-a-card-declares-and-records-what-they-proved.md create mode 100644 packages/workfile/src/modules/cards/changed.ts create mode 100644 packages/workfile/test/self-reference.test.ts diff --git a/.github/workflows/workfile.yml b/.github/workflows/workfile.yml index 39e18bb..ed1bd1b 100644 --- a/.github/workflows/workfile.yml +++ b/.github/workflows/workfile.yml @@ -5,8 +5,10 @@ on: push: branches: [main] -permissions: - contents: read +# Nothing at the top level, so a job added by hand starts from no permissions +# rather than inheriting these — the shape the generated template uses, and the +# reason the two jobs below can differ so sharply. +permissions: {} jobs: doctor: @@ -16,6 +18,8 @@ jobs: # Validating the protocol with the tree's own code is also the honest # dogfood: a PR is checked by the exact behavior it ships. runs-on: ubuntu-latest + permissions: + contents: read steps: - uses: actions/checkout@v7 - uses: pnpm/action-setup@v6 @@ -29,3 +33,109 @@ jobs: run: node ./packages/workfile/dist/bin/workfile.js doctor --json - name: Check generated agent instructions run: node ./packages/workfile/dist/bin/workfile.js agents check --json + + # The two-job split T-0189 generates, ported to the local build for the same + # reason the doctor job is: inside this repository `npx @illodev/workfile` + # resolves to the checkout's own package.json, so the published spec would + # install nothing runnable. + # + # The job that runs card-declared commands holds nothing, and the job that + # holds a write token runs no repository code. They cannot be one job: a + # criterion bound to a command can only be checked by running it, and a + # process a pull request configured must not be handed a token. + cards: + # A card diff needs a base to be taken against, and a push to main has none. + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + timeout-minutes: 20 + # Nothing at all. This job runs commands the pull request declared. + # `actions/checkout` works unauthenticated here because the repository is + # public; a private one would need `contents: read`, which is a real cost of + # this shape and not a detail. + permissions: {} + steps: + - uses: actions/checkout@v7 + with: + # The diff is taken from the merge base, and a shallow clone has none. + # `changedPaths` reports that as "cannot answer" rather than as an + # empty diff, so this would fail the job rather than silently verify + # nothing — but it would still fail. + fetch-depth: 0 + persist-credentials: false + - uses: pnpm/action-setup@v6 + - uses: actions/setup-node@v7 + with: + node-version: "22" + cache: pnpm + - run: pnpm install --frozen-lockfile + - run: pnpm run build:core + # Values arrive through `env:`, never interpolated into the script: an + # expression inside a `run:` block is expanded before the shell sees it, + # so a branch name is code there and data here. + - name: Verify the cards this branch touched + env: + BASE_REF: ${{ github.base_ref }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + node ./packages/workfile/dist/bin/workfile.js card verify --changed \ + --base "origin/$BASE_REF" \ + --close --run "$RUN_URL" --commit "$HEAD_SHA" \ + --json | tee workfile-cards.json + - name: Collect what the run wrote + if: always() + run: git diff -- .project > workfile-cards.patch || true + - uses: actions/upload-artifact@v4 + if: always() + with: + name: workfile-cards + path: | + workfile-cards.json + workfile-cards.patch + if-no-files-found: ignore + + record: + needs: cards + # Same-repository pull requests only. A fork gets a read-only token whatever + # this says, so the push there fails rather than being refused by us; the + # condition keeps the job from starting in order to say so. + if: >- + github.event_name == 'pull_request' && + github.event.pull_request.head.repo.full_name == github.repository + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: write + steps: + # The head branch, not the merge commit: a commit is being pushed to it. + - uses: actions/checkout@v7 + with: + ref: ${{ github.event.pull_request.head.ref }} + - uses: actions/download-artifact@v4 + with: + name: workfile-cards + # This job runs no Workfile command, deliberately. Every one of them + # `import()`s project.config.mjs from the checkout, which is the code this + # job exists not to execute while holding a write token. + - name: Refuse a patch that reaches outside the protocol directory + run: | + test -s workfile-cards.patch || exit 0 + git apply --check workfile-cards.patch + if git apply --numstat workfile-cards.patch | cut -f3 | + grep -qv '^.project/'; then + echo "::error::refusing a patch that reaches outside .project/" + exit 1 + fi + # The push re-triggers this workflow, and the second run finds the boxes + # already checked: nothing to write, an empty patch, no commit. It + # converges rather than looping. + - name: Commit the evidence + run: | + test -s workfile-cards.patch || exit 0 + git apply workfile-cards.patch + git add -- .project + git diff --cached --quiet && exit 0 + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + git commit -m "Record card verification from CI" + git push diff --git a/.project/cards/T-0161-a-card-can-be-written-with-itself-as-its-origin.md b/.project/cards/T-0161-a-card-can-be-written-with-itself-as-its-origin.md index b13415c..9dc7c49 100644 --- a/.project/cards/T-0161-a-card-can-be-written-with-itself-as-its-origin.md +++ b/.project/cards/T-0161-a-card-can-be-written-with-itself-as-its-origin.md @@ -1,7 +1,7 @@ --- id: T-0161 title: A card can be written with itself as its origin -status: backlog +status: doing type: bug priority: low area: core @@ -9,7 +9,13 @@ effort: S scope: [packages/workfile/src/modules/cards/validation.ts] origin: [T-0156] created: 2026-08-04 -updated: 2026-08-04 +updated: 2026-08-07 +claimed_by: "illodev@local#42eb42f5" +claimed_at: "2026-08-07T21:19:17.046Z" +verify: + - id: self-reference + run: [node, --test, packages/workfile/test/self-reference.test.ts] + criteria: ["sha256:ae2edc316ff93cb65b575452945408f5f712ad33d6b5c7ed4fb61e9a5bb8af1b", "sha256:08de66f38da03ca41f2963e76624fc4140e4fd240792af7fbd1f8b826d7d38b9", "sha256:ee509c4d091e14ef3f3d6ec54722acb30bc019b2d2d5de59db82058afc24e497", "sha256:193e044572b952be47178266f43cdb70cc42ef043b03bdaff153cb343de4d96b"] --- Found in the 0.6.0 smoke test, against the published package. On a fresh @@ -57,3 +63,8 @@ that does not exist yet. - [ ] The error code reads like its two neighbours - [ ] The doctor rule stays, for records written before this landed - [ ] `pnpm run check` green, doctor 0/0 + +## Activity + +- 2026-08-07 21:19Z illodev@local#42eb42f5 · claimed +- 2026-08-07 21:24Z illodev@local#42eb42f5 · verify self-reference: node --test packages/workfile/test/self-reference.test.ts passed, checked #1, #2, #3, #4 diff --git a/.project/cards/T-0189-ci-runs-a-card-s-declared-checks-and-writes-back-t.md b/.project/cards/T-0189-ci-runs-a-card-s-declared-checks-and-writes-back-t.md index df5f949..60ecc79 100644 --- a/.project/cards/T-0189-ci-runs-a-card-s-declared-checks-and-writes-back-t.md +++ b/.project/cards/T-0189-ci-runs-a-card-s-declared-checks-and-writes-back-t.md @@ -1,7 +1,7 @@ --- id: T-0189 title: CI runs a card's declared checks and writes back the evidence -status: backlog +status: doing type: feature priority: medium area: infra @@ -9,9 +9,12 @@ parent: T-0183 tags: [protocol, acceptance] effort: L created: 2026-08-05 -updated: 2026-08-05 +updated: 2026-08-07 origin: [ADR-0016] depends: [T-0188, T-0186] +claimed_by: "illodev@local#42eb42f5" +claimed_at: "2026-08-07T20:49:06.844Z" +scope: [packages/workfile/src/modules/ci, packages/workfile/src/modules/cards/runner.ts, packages/workfile/bin/workfile.ts] --- The `ci` method from ADR-0016, and the only tier with a witness. The generated @@ -34,6 +37,24 @@ Open questions to settle before implementing, not after: ## Acceptance criteria - [ ] The generated GitHub workflow runs the declared checks for cards touched by the branch. -- [ ] A passing run writes `verified` with `method: ci`, the commit and the run URL. +- [x] A passing run writes `verified` with `method: ci`, the commit and the run URL. - [ ] A fork PR either records evidence safely or records none; it never fails open. -- [ ] The behaviour is documented in the CLI/CI reference, including what it does not do. +- [x] The behaviour is documented in the CLI/CI reference, including what it does not do. + +## Activity + +- 2026-08-07 20:49Z illodev@local#42eb42f5 · claimed + +## Notes + +- 2026-08-07 21:09Z illodev@local#42eb42f5 — The three open questions the card said to settle first, settled. + +Where the evidence is written from a PR that cannot push: two jobs, and the owner chose it. The job that runs card-declared commands holds `permissions: {}` and leaves no credentials in .git/config; a second job holds `contents: write` and runs no repository code at all — not even Workfile, because every Workfile command import()s project.config.mjs from the checkout. It applies a patch bounded to the protocol directory. A fork gets a read-only token from GitHub for pull_request, so nothing is recorded there whatever the workflow says, and the job condition declines to start in order to say so rather than fail at the last step. + +Whether a failing check blocks the merge or refuses done: it refuses done, which the card guessed right. A failing command unchecks the criteria it owns, and assertAcceptanceMet then refuses the transition. The job also exits non-zero so the run is visibly red, but nothing is gated on that. + +One job per card or one for all: one for all. The card format is a flat command list and the CI config decides the shape, per ADR-0016. + +And one thing the card did not anticipate. T-0188 left a tripwire in ci-targets.test.ts, asserting that no generated target contains the string `card verify` or an Actions expression, commented as "the pin that keeps the next card honest" — and this is that card. What it was protecting is real and stands: a verify[].run is an argument vector so that no shell parses it, and an Actions expression inside a run: block is expanded before the shell sees it. Neither is what invoking the runner does: the card's command never appears in the workflow, and the tool spawns it with no shell. So the two blanket assertions are replaced by the two rules they stood for — no target reads a card's verify block into the template, and no Actions expression reaches a shell line, checked by indentation across run: and script: in all three formats. + +The first version of that second check was broken in the dangerous direction: a mutation putting an Actions expression on a continuation line passed it. Found by mutating, not by reading. It is a line-based scan now, with a per-format floor so a scan that stops matching fails loudly instead of reporting a clean sweep over nothing. diff --git a/.project/changelog/unreleased/CHG-0154-ci-runs-the-checks-a-card-declares-and-records-what-they-proved.md b/.project/changelog/unreleased/CHG-0154-ci-runs-the-checks-a-card-declares-and-records-what-they-proved.md new file mode 100644 index 0000000..32c612c --- /dev/null +++ b/.project/changelog/unreleased/CHG-0154-ci-runs-the-checks-a-card-declares-and-records-what-they-proved.md @@ -0,0 +1,12 @@ +--- +id: CHG-0154 +title: CI runs the checks a card declares and records what they proved +type: added +area: infra +visibility: public +cards: [T-0189, T-0161] +created: 2026-08-07 +updated: 2026-08-07 +--- + +A card may bind an acceptance criterion to a command, and the generated GitHub workflow now runs those commands for every card a branch touched and writes the result back. Two jobs, because they cannot be one: the job that runs commands a pull request declared holds no permissions at all, and the job that holds a write token runs no repository code. A fork records nothing, which GitHub enforces by issuing a read-only token. And CI closes a card only when every one of its criteria is bound to a command, because a narrative criterion is not something a runner has an opinion about. diff --git a/packages/workfile/bin/workfile.ts b/packages/workfile/bin/workfile.ts index 738a0d9..3fc1a6d 100644 --- a/packages/workfile/bin/workfile.ts +++ b/packages/workfile/bin/workfile.ts @@ -80,6 +80,7 @@ import { parseAcceptance, resolveActor, runCardVerification, + verifyChangedCards, setCardAcceptance, unreadableCriteria, ValidationError, @@ -160,6 +161,7 @@ const USAGE: Record = { "workfile card note ID --text TEXT [--section NAME] [--actor ACTOR]", "workfile card ac ID [--check N] [--uncheck N] # repeatable; no flags lists them", "workfile card verify ID [--only ENTRY,ENTRY] [--actor ACTOR] # run the card's declared commands", + "workfile card verify --changed --base main [--close --run URL] # every card this branch touched", "workfile card write ID [--body-file FILE] # or pipe the body on stdin", "workfile card renumber ID|FILE [--to T-0123] [--actor ACTOR]", "workfile card renumber --duplicates [--actor ACTOR] # heal after a merge" @@ -418,7 +420,12 @@ const COMMAND_FLAGS: Record = { ], "card verify": [ "--actor", - "--only" + "--base", + "--changed", + "--close", + "--commit", + "--only", + "--run" ], "card write": [ "--body-file", @@ -1546,12 +1553,18 @@ async function cardCommand(workspace, action) { const result = await createCard(workspace, input); return print(has("--json") ? result.card : `${result.id} ${result.file}`); } - // `card renumber --duplicates` is a sweep and names no record. It reached - // here only because the id position was read raw and `--duplicates` is a - // truthy string — the accident this guard was written to depend on without - // anyone saying so. - const sweeping = action === "renumber" && has("--duplicates"); - if (!id && !sweeping) { + // Two card actions name no record, and each says so with a flag. + // `renumber --duplicates` sweeps the whole board; `verify --changed` takes + // its list from the branch. Stated as a rule rather than as one special + // case, because the first of them only ever reached here by accident: the + // id position was read raw and `--duplicates` is a truthy string, so the + // guard was depending on something nobody had written down. `--changed` + // does not get that accident — the id position is empty for it, since + // `--base` consumes the word after it. + const namesNoCard = + (action === "renumber" && has("--duplicates")) || + (action === "verify" && has("--changed")); + if (!id && !namesNoCard) { throw new ValidationError( "CLI_ARGUMENT_REQUIRED", `card ${action} requires an ID` @@ -1604,6 +1617,47 @@ async function cardCommand(workspace, action) { return; } if (action === "verify") { + if (has("--changed")) { + // No ID: the branch names the cards. `--base` is required rather + // than defaulted to `main`, because guessing it wrong means running + // the declared commands of cards this branch never opened, and + // writing to them. + const report = await verifyChangedCards(workspace, { + base: option("--base") || "", + actor: option("--actor") || defaultActor(), + close: has("--close"), + run: option("--run") || null, + // Undefined rather than null: the close door reads undefined as + // "resolve HEAD yourself" and null as "there is no commit". + commit: option("--commit") || undefined + }); + // Unresolved is a failure, not an empty run. Git could not answer + // which cards this branch touched, so nothing here is a statement + // about any card. + process.exitCode = report.resolved && report.ok ? 0 : 1; + if (has("--json")) return print(report); + if (!report.resolved) { + console.error( + `Could not diff against ${report.base || "(no base)"}: the ref is ` + + "unknown here, or this is a shallow checkout with no merge base. " + + "No card was verified." + ); + return; + } + console.log( + `${report.cards.length} card${report.cards.length === 1 ? "" : "s"} ` + + `touched since ${report.base}` + ); + for (const card of report.cards) { + const closed = card.closed ? " · closed with method: ci" : ""; + console.log(` ${card.id} — ${card.outcome}${closed}`); + if (card.heldOpen) console.log(` ${card.heldOpen}`); + for (const entry of card.report?.entries || []) { + console.log(` ${describeVerifyEntry(entry)}`); + } + } + return; + } const report = await runCardVerification( workspace, requireId("card", action, id), diff --git a/packages/workfile/docs/cli.md b/packages/workfile/docs/cli.md index 88c6788..c0c99f5 100644 --- a/packages/workfile/docs/cli.md +++ b/packages/workfile/docs/cli.md @@ -232,6 +232,8 @@ workfile card ac ID # list criteria with their numb workfile card ac ID --check 1,3 --check 5 # repeatable, comma lists accepted workfile card ac ID --uncheck 2 workfile card verify ID [--only gate] [--actor ACTOR] # run the declared commands +workfile card verify --changed --base main # every card this branch touched +workfile card verify --changed --base main --close --run URL --commit SHA ``` Acceptance criteria are the `- [ ]` items under a `## Acceptance criteria` heading. @@ -712,6 +714,45 @@ workfile ci sync [--targets github,gitlab,generic] workfile ci check [--targets ...] ``` +### What the generated GitHub workflow does, and what it will not do + +Three jobs. `doctor` validates the protocol. `cards` runs the commands the cards +this branch touched declare, and `record` writes the result back. + +Those last two are deliberately not one job. A criterion bound to a command can +only be checked by running it, so `cards` executes commands a pull request +declared — and therefore holds `permissions: {}`, with no credentials left in +`.git/config`. Writing evidence needs `contents: write`, so `record` holds it and +runs no repository code at all: not even Workfile, because every Workfile command +`import()`s `project.config.mjs` from the checkout. It applies a patch bounded to +the protocol directory and pushes. + +**A fork records nothing.** GitHub issues a read-only token for `pull_request` +from a fork, so the push cannot land whatever the workflow says; `record` also +declines to start there, in order to say so rather than fail at the last step. + +**CI closes a card only when every one of its criteria is bound to a command.** A +narrative criterion is not something a runner has an opinion about, so a card +that carries one gets its bound boxes written and stays open, with the reason +reported. That is the whole safety of the write-back: `card ac --check` refuses a +bound criterion and only the runner writes it, so the boxes CI touches are boxes +no person was going to check either way. + +**Only on a pull request.** "The cards this branch touched" is a diff against a +base and a push to a default branch has none. The checkout needs +`fetch-depth: 0`, because the diff is taken from the merge base and a shallow +clone has none — reported as *cannot answer* rather than as an empty diff, which +would turn "nothing was verified" into "there was nothing to verify". + +`--base` is required and has no default. Guessing it wrong means running the +declared commands of cards the branch never opened, and writing to them. + +**GitLab and the generic script run no card commands.** GitLab has no per-job +permission scope, so the job sees every unprotected variable in the project and +there is nowhere to put a command a merge request declared; the generic script +inherits the whole environment of whatever invokes it. Both files carry the +invocation commented out with what a maintainer would have to arrange first. + ## Legacy migration ```bash diff --git a/packages/workfile/src/modules/cards/changed.ts b/packages/workfile/src/modules/cards/changed.ts new file mode 100644 index 0000000..f8ab349 --- /dev/null +++ b/packages/workfile/src/modules/cards/changed.ts @@ -0,0 +1,278 @@ +/** + * The cards a branch touched, and what running their declared checks decided. + * + * T-0189, the `ci` tier of ADR-0016 — the only tier with a witness. `local` is a + * command that ran on the author's machine and is still self-reported; this is + * the same commands run somewhere the author does not control, recorded with the + * run that ran them. + * + * ## What it will and will not close + * + * A criterion bound to a command is machine-owned: `card ac --check` refuses it + * and only the runner writes it. A narrative criterion is not, and nothing here + * can judge one — "the recut demo video reads correctly" is not a thing a runner + * has an opinion about. So the rule is mechanical and it is the whole of the + * safety here: + * + * **A card is closed by CI only when every one of its criteria is bound.** + * + * A card with one narrative criterion gets its bound boxes written and stays + * open, which is not a failure — it is the run doing the part it can witness and + * declining the part it cannot. A card with none of its criteria bound is not + * touched at all: it declares no commands, so there is nothing to run. + * + * ## Why the close happens here and not in the job that pushes + * + * Every Workfile command loads the workspace, and loading the workspace + * `import()`s `project.config.mjs` from the checkout. On a pull request that is + * code the pull request wrote — see ADR-0019. So the job that runs card commands + * must hold nothing, and the job that holds a write token must not run this. The + * generated workflow splits them: this produces the finished card files and a + * report, and a second job with no repository code in it commits the result. + * `ci.ts` is where that split is written down. + */ + +import { NotFoundError, ValidationError } from "../../core/errors.js"; +import { normalizeRepoPath } from "../../core/glob.js"; +import { ensureWritable } from "../../core/guards.js"; +import { criterionOwners, parseAcceptance } from "./acceptance.js"; +import { loadCards } from "./cards.js"; +import { changedPaths } from "./git.js"; +import { releaseCard } from "./mutations.js"; +import { runCardVerification } from "./runner.js"; +import type { VerifyRunReport } from "./runner.js"; + +/** What happened to one card in the run. */ +export interface ChangedCardResult { + id: string; + file: string; + /** + * `verified` — every declared command passed. + * `failed` — at least one decided against a criterion it owns. + * `undecided` — a command reached no verdict: killed at the timeout, or + * never started because this machine has no such command. + * `skipped` — the card declares no commands, so there was nothing to run. + */ + outcome: "verified" | "failed" | "undecided" | "skipped"; + /** Absent for `skipped`, which never reached the runner. */ + report?: VerifyRunReport; + /** Whether every criterion is bound, which is what CI may close. */ + fullyBound: boolean; + /** Set when this run moved the card to `done`. */ + closed?: { commit: string | null; run: string | null }; + /** Why a card that passed was nevertheless left open. */ + heldOpen?: string; +} + +export interface ChangedCardsReport { + /** The ref the diff was taken against. */ + base: string; + /** + * False when git could not answer, in which case `cards` is empty and means + * nothing. A caller that reports this as "no cards to verify" is reporting + * the opposite of what happened. + */ + resolved: boolean; + /** Card files the branch touched, whether or not they declare commands. */ + touched: string[]; + cards: ChangedCardResult[]; + /** True when nothing failed and nothing was left undecided. */ + ok: boolean; +} + +/** + * Card ids, from the paths a diff reported. + * + * Composed from the configured directories rather than parsed out of the + * filename. A card's name is derived from its title and `card renumber` exists, + * so a path is not an id — and the two places cards live are declared values a + * project may move. An archived card answers too: a branch that archived one + * touched it. + * + * Matched by full path rather than by basename, because the archive holds files + * whose names collide with live ones by design. + */ +function idsForPaths( + workspace, + cards, + paths: string[] +): Array<{ id: string; file: string }> { + const live = normalizeRepoPath(workspace.config.cards.path); + const archive = normalizeRepoPath(workspace.config.cards.archivePath); + const byPath = new Map(); + for (const card of cards) { + const directory = card.archived ? archive : live; + byPath.set(`${directory}/${normalizeRepoPath(card.file)}`, { + id: card.id, + file: card.file + }); + } + const found: Array<{ id: string; file: string }> = []; + const seen = new Set(); + for (const path of paths) { + const hit = byPath.get(normalizeRepoPath(path)); + if (!hit || seen.has(hit.id)) continue; + seen.add(hit.id); + found.push(hit); + } + return found.sort((left, right) => left.id.localeCompare(right.id)); +} + +/** + * Run the declared checks of every card this branch touched. + * + * `close` is opt-in, because writing `verified` is what the caller may not be + * entitled to do — and because a run that only reports is the useful half on a + * fork, where the write can never land anyway. + */ +export async function verifyChangedCards( + workspace, + { + base, + actor = null, + close = false, + run = null, + commit, + now + }: { + base: string; + actor?: string | null; + /** Move a fully-bound card that passed to `done`, with `method: ci`. */ + close?: boolean; + /** The run that witnessed it, recorded on the card. */ + run?: string | null; + /** + * The commit the checks ran against. + * + * Absent, not null: `commitForClose` reads `undefined` as "resolve HEAD + * yourself" and any other value — including `null` — as the answer. So + * threading a `null` through from an unset CLI flag would record a card + * closed at no commit, which is exactly the field criterion 2 of T-0189 + * asks for. Worth supplying explicitly all the same on a pull request, + * where HEAD is a merge commit that exists on no branch. + */ + commit?: string; + now?: string | number | Date; + } +): Promise { + // Before the diff rather than after: a read-only workspace can record + // nothing these commands prove, and a run that spawns a test suite and then + // finds that out has already spent the expensive part. + ensureWritable(workspace); + if (!base) { + throw new ValidationError( + "CARD_VERIFY_NO_BASE", + "A base ref is required to know which cards this branch touched. " + + "Pass `--base main`, or the pull request's base branch in CI." + ); + } + + const paths = await changedPaths(workspace.root, base); + if (paths === null) { + // Reported, never treated as an empty diff. The two are opposite claims + // and only one of them is safe to act on. + return { base, resolved: false, touched: [], cards: [], ok: false }; + } + + // Archived cards come back from this too, which is wanted: a branch that + // archived a card touched it, and the diff will say so. + const { cards } = await loadCards(workspace); + const touched = idsForPaths(workspace, cards, paths); + const results: ChangedCardResult[] = []; + + for (const { id, file } of touched) { + const card = cards.find((candidate) => candidate.id === id); + const reading = parseAcceptance(card?.body || ""); + const owners = criterionOwners(reading, card?.verify); + const fullyBound = + reading.items.length > 0 && owners.size === reading.items.length; + + if (!(card?.verify as unknown[] | undefined)?.length) { + results.push({ id, file, outcome: "skipped", fullyBound }); + continue; + } + + let report: VerifyRunReport; + try { + report = await runCardVerification(workspace, id, { actor, now }); + } catch (error) { + // A card that declares entries the allowlist refuses, or whose + // bindings are stale, raises rather than returning a report. That is + // a fact about the card and belongs in the report as one, not as a + // crash that abandons every card after it in the list. + if (error instanceof ValidationError || error instanceof NotFoundError) { + results.push({ + id, + file, + outcome: "failed", + fullyBound, + heldOpen: error.message + }); + continue; + } + throw error; + } + + const decided = report.entries.filter( + (entry) => entry.outcome === "passed" || entry.outcome === "failed" + ); + const outcome: ChangedCardResult["outcome"] = report.ok + ? "verified" + : decided.length === report.entries.length + ? "failed" + : "undecided"; + const result: ChangedCardResult = { id, file, outcome, report, fullyBound }; + + if (outcome === "verified" && close) { + if (card?.status === "done") { + // Already closed, so there is nothing to record and the door + // would refuse: a card that is done keeps the verification the + // write that closed it recorded. Re-running the checks on a + // branch that touches a closed card is ordinary — a second push + // to the same pull request does it — so this is a normal state + // and not a failure. + result.heldOpen = "already done; the run that closed it keeps the record"; + } else if (!fullyBound) { + // The honest half-answer: the boxes this run owns are written, + // and the ones a person judges are left to the person. + result.heldOpen = + `${reading.items.length - owners.size} of ${reading.items.length} ` + + "criteria are not bound to a command, so this run cannot say " + + "they are met"; + } else { + try { + await releaseCard(workspace, id, { + status: "done", + actor, + method: "ci", + run, + commit, + now + }); + result.closed = { commit: commit ?? null, run }; + } catch (error) { + // A refusal is a fact about this card — an area whose policy + // does not accept `ci`, a transition its status does not + // allow — and it must not abandon every card after it in the + // list. One card's policy is not the run's verdict. + if (error instanceof ValidationError) { + result.heldOpen = error.message; + } else { + throw error; + } + } + } + } + results.push(result); + } + + return { + base, + resolved: true, + touched: touched.map((entry) => entry.file), + cards: results, + ok: results.every( + (entry) => entry.outcome === "verified" || entry.outcome === "skipped" + ) + }; +} diff --git a/packages/workfile/src/modules/cards/git.ts b/packages/workfile/src/modules/cards/git.ts index 45624d5..059952d 100644 --- a/packages/workfile/src/modules/cards/git.ts +++ b/packages/workfile/src/modules/cards/git.ts @@ -1,10 +1,11 @@ /** - * The repository, asked two questions and nothing else. + * The repository, asked three questions and nothing else. * * A card that records the commit it was verified at needs to know what HEAD is, - * and `doctor` needs to know whether that commit is still reachable. Both are - * git questions, and this is the first subprocess anything under `src/` spawns — - * so the shape of it is worth stating rather than inferring. + * `doctor` needs to know whether that commit is still reachable, and a CI run + * that verifies the cards a branch touched needs to know which ones those are. + * All three are git questions, and this is the first subprocess anything under + * `src/` spawns — so the shape of it is worth stating rather than inferring. * * **Git is optional.** Nothing else in this package requires a repository, and * a protocol that refused to close a card outside one would be refusing the @@ -150,3 +151,44 @@ export async function isAncestorOfHead( if (result.ok) return "yes"; return result.code === 1 ? "no" : "unknown"; } + +/** A ref as this module will pass one to git, which is deliberately narrow. */ +const SAFE_REF = /^[0-9A-Za-z._\/-]{1,255}$/; + +/** + * The paths this branch touched, against a base ref. + * + * `base...HEAD` with three dots, which diffs from the merge base rather than + * from the tip of the base branch — the same thing a pull request shows. Two + * dots would report every file the base moved on since, so a branch that merely + * fell behind would look like it had touched cards it never opened, and CI would + * run their commands and write to them. + * + * `null` is "cannot answer", and every caller has to treat it as such rather + * than as "nothing changed". The distinction is the whole safety of the thing + * this feeds: a shallow CI checkout has no merge base, and reading that as an + * empty list would report a run that verified nothing as a run that found + * nothing to verify. Those are opposite claims about the same silence. + * + * The ref is checked against `SAFE_REF` before it becomes an argument. Nothing + * here goes through a shell, so this is not about metacharacters: it is about a + * value out of the environment beginning with `-` and being read as an option. + */ +export async function changedPaths( + root: string, + base: string +): Promise { + if (!root || !SAFE_REF.test(String(base))) return null; + // Resolved first, so a base ref this clone does not have is reported as + // "cannot answer" rather than as a diff against something else. + const resolved = await git(root, ["rev-parse", "--verify", `${base}^{commit}`]); + if (!resolved.ok) return null; + const result = await git(root, [ + "diff", + "--name-only", + "--diff-filter=d", + `${base}...HEAD` + ]); + if (!result.ok) return null; + return result.stdout.split("\n").map((line) => line.trim()).filter(Boolean); +} diff --git a/packages/workfile/src/modules/cards/index.ts b/packages/workfile/src/modules/cards/index.ts index 2f3d0e7..fcc5809 100644 --- a/packages/workfile/src/modules/cards/index.ts +++ b/packages/workfile/src/modules/cards/index.ts @@ -44,6 +44,11 @@ export { // no allowlist in front of it, and publishing it on the package's public API // would offer "run any argv" beside the gate that exists to stop exactly that. export { runCardVerification } from "./runner.js"; +export { verifyChangedCards } from "./changed.js"; +export type { + ChangedCardResult, + ChangedCardsReport +} from "./changed.js"; export type { VerifyEntryResult, VerifyOutcome, diff --git a/packages/workfile/src/modules/cards/mutations.ts b/packages/workfile/src/modules/cards/mutations.ts index c6e71ec..bba3799 100644 --- a/packages/workfile/src/modules/cards/mutations.ts +++ b/packages/workfile/src/modules/cards/mutations.ts @@ -797,6 +797,30 @@ export async function createCard(workspace, input, { maxRetries = 32, now }: any code: "CARD_ID_ALLOCATION_FAILED" }, async (id) => { + // The allocated id, checked here because here is the only place it + // exists. `validateCardCandidate` above ran against `id: "pending"` + // — the allocation decides the id and the allocation needs the lock + // — so a create naming the id it is about to be given cannot be + // refused up there, whatever the field. + // + // T-0161 assumed it could, on the grounds that the self-parent + // branch catches the same case on creation. It does not: a self + // `parent` on create is refused by `CARD_PARENT_NOT_FOUND`, because + // the id is not among the loaded cards either. The right code for + // the wrong reason, and only for the two fields whose targets have + // to exist. `origin` has no existence rule — an origin may name a + // record not written yet — so nothing caught it at all. + // + // A `ValidationError` is not create contention, so it leaves the + // retry loop rather than being read as a collision and retried onto + // the next id. + if ((base.origin || []).includes(id)) { + throw new ValidationError( + "CARD_SELF_ORIGIN", + "A card cannot originate from itself.", + { id, field: "origin" } + ); + } const file = `${id}-${slugify(input.title)}.md`; const path = join(workspace.paths.cards, file); const content = renderCard({ ...base, id }, input.body); diff --git a/packages/workfile/src/modules/cards/validation.ts b/packages/workfile/src/modules/cards/validation.ts index 9b8b365..4c08d7a 100644 --- a/packages/workfile/src/modules/cards/validation.ts +++ b/packages/workfile/src/modules/cards/validation.ts @@ -545,6 +545,21 @@ export function validateCardCandidate(workspace, candidate, cards, currentId = n fail("CARD_DEPENDENCY_NOT_FOUND", `Dependency not found: ${dependency}`); } } + // T-0161. The third relationship field, which had a `doctor` rule and no + // write-time guard — so `card create --title X --origin T-0001` allocating + // `T-0001` reported success and left the repository in a state `doctor` + // calls an error. The pre-commit hook then refuses the next commit, for a + // card written minutes earlier by a command that said it worked. + // + // Existence is deliberately not checked here, unlike `parent` and + // `depends`. An origin may legitimately name a record that does not exist + // yet — a card can come out of a decision still being written — which is + // why `missing-origin` stays a `doctor` rule and this is not. + for (const origin of candidate.origin || []) { + if (origin === currentId || origin === candidate.id) { + fail("CARD_SELF_ORIGIN", "A card cannot originate from itself."); + } + } const hasActor = Boolean(candidate.claimed_by); const hasTimestamp = Boolean(candidate.claimed_at); if (hasActor !== hasTimestamp) { diff --git a/packages/workfile/src/modules/ci/ci.ts b/packages/workfile/src/modules/ci/ci.ts index 7e8d1de..fc3cf86 100644 --- a/packages/workfile/src/modules/ci/ci.ts +++ b/packages/workfile/src/modules/ci/ci.ts @@ -60,8 +60,46 @@ function executesRepositoryCode() { return EXECUTES_REPOSITORY_CODE.map((line) => `# ${line}`).join("\n"); } +/** + * Why the card runner and the write token are in different jobs. + * + * T-0189. A card may bind a criterion to a command, and running that command is + * the only thing that can check it. So one job here executes commands a pull + * request declared, and it must therefore hold nothing at all — `permissions: + * {}`, no credentials left in `.git/config`, no secrets a fork could reach. + * + * But the evidence has to be written back, and writing to the repository needs + * `contents: write`. Putting that scope on the job that runs card commands would + * hand a token to a process a pull request configured, which is the whole thing + * ADR-0019 exists to say out loud. So the run and the write are two jobs: the + * first produces a patch bounded to the protocol directory, the second applies + * it and holds no repository code at all — it never invokes Workfile, because + * every Workfile command `import()`s `project.config.mjs` from the checkout. + * + * On a fork the second job cannot write whatever this file says: GitHub issues a + * read-only token for `pull_request` from a fork, so the push fails and nothing + * is recorded. That is a fail-closed enforced by the platform rather than by our + * condition, and the condition below is documentation of it. + */ +const TWO_JOB_SPLIT = [ + "The job that runs card-declared commands holds nothing, and the job that", + "holds a write token runs no repository code. They cannot be one job: a", + "criterion bound to a command can only be checked by running it, and a", + "process a pull request configured must not be handed a token. See T-0189.", + "On a fork the write token is read-only whatever this file says, so nothing", + "is recorded there — GitHub enforces that, not the condition below." +]; + +function twoJobSplit(indent = "# ") { + return TWO_JOB_SPLIT.map((line) => `${indent}${line}`).join("\n"); +} + function githubBody(workspace) { const node = String(workspace.config.ci.nodeVersion || "22"); + const protocolRoot = String(workspace.config.storage.root || ".project") + .replace(/\\/g, "/") + .replace(/^\.\//, "") + .replace(/\/+$/, ""); return `# Generated by @illodev/workfile ${PACKAGE_VERSION} # ${executesRepositoryCode()} @@ -97,6 +135,102 @@ jobs: run: npx --yes @illodev/workfile@${PACKAGE_VERSION} doctor --json - name: Check generated agent instructions run: npx --yes @illodev/workfile@${PACKAGE_VERSION} agents check --json + +${twoJobSplit(" # ")} + cards: + # Only on a pull request. "The cards this branch touched" is a diff against a + # base, and a push to a default branch has none — running here would answer + # for whatever ref happened to resolve. + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + timeout-minutes: 20 + # Nothing. This job runs commands the pull request declared. + permissions: {} + steps: + - uses: actions/checkout@v4 + with: + # The merge base is what a card diff is taken from, and a shallow + # checkout has none. \`changedPaths\` reports that as "cannot answer" + # rather than as an empty diff, so a shallow clone here would fail the + # job rather than silently verify nothing — but it would still fail. + fetch-depth: 0 + persist-credentials: false + - uses: actions/setup-node@v4 + with: + node-version: "${node}" + # Values arrive through \`env:\`, never interpolated into the script. A + # \`\${{ }}\` inside a \`run:\` block is expanded before the shell sees it, so a + # branch name is code there; as an environment variable it is data. + - name: Verify the cards this branch touched + env: + BASE_REF: \${{ github.base_ref }} + HEAD_SHA: \${{ github.event.pull_request.head.sha }} + RUN_URL: \${{ github.server_url }}/\${{ github.repository }}/actions/runs/\${{ github.run_id }} + run: | + npx --yes @illodev/workfile@${PACKAGE_VERSION} card verify --changed \\ + --base "origin/$BASE_REF" \\ + --close --run "$RUN_URL" --commit "$HEAD_SHA" \\ + --json > workfile-cards.json + # Bounded to the protocol directory at the point it is produced, so the + # job that applies it is not the only thing standing between a card + # command and the rest of the repository. + - name: Collect what the run wrote + if: always() + run: git diff -- ${protocolRoot} > workfile-cards.patch || true + - uses: actions/upload-artifact@v4 + if: always() + with: + name: workfile-cards + path: | + workfile-cards.json + workfile-cards.patch + if-no-files-found: ignore + + record: + needs: cards + # Same-repository pull requests only. A fork gets a read-only token whatever + # this says, so the push there fails rather than being refused by us; this + # condition keeps the job from starting in order to say so. + if: >- + github.event_name == 'pull_request' && + github.event.pull_request.head.repo.full_name == github.repository + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: write + steps: + # The head branch, not the merge commit: a commit is being pushed to it. + - uses: actions/checkout@v4 + with: + ref: \${{ github.event.pull_request.head.ref }} + - uses: actions/download-artifact@v4 + with: + name: workfile-cards + # This job runs no Workfile command, deliberately. Every one of them + # \`import()\`s project.config.mjs from the checkout, which is the code this + # job exists not to execute while holding a write token. + - name: Refuse a patch that reaches outside the protocol directory + run: | + test -s workfile-cards.patch || exit 0 + git apply --check workfile-cards.patch + if git apply --numstat workfile-cards.patch | cut -f3 | + grep -qv '^${protocolRoot}/'; then + echo "::error::refusing a patch that reaches outside ${protocolRoot}/" + exit 1 + fi + # The push re-triggers this workflow, and the second run finds the cards + # already closed: nothing to write, an empty patch, no commit. It + # converges rather than looping. + - name: Commit the evidence + run: | + test -s workfile-cards.patch || exit 0 + git apply workfile-cards.patch + git add -- ${protocolRoot} + git diff --cached --quiet && exit 0 + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + git commit -m "Record card verification from CI" + git push `; } @@ -114,6 +248,20 @@ ${executesRepositoryCode()} # # This file does nothing on its own: GitLab reads .gitlab-ci.yml, so add # \`include: { local: .gitlab/workfile.yml }\` there or no pipeline runs. +# +# It also does not run the commands cards declare, and that is the same fact read +# once more. On GitHub those run in a job holding \`permissions: {}\`, with a +# second job doing the write — see T-0189. There is no scope to put them behind +# here: this job sees every unprotected variable in the project, so running a +# command a merge request declared would run it beside the credentials. Enabling +# it is a decision only the maintainer can make, and it needs the variables +# protected or absent first: +# +# - npx --yes @illodev/workfile@${PACKAGE_VERSION} card verify --changed +# --base "origin/$CI_MERGE_REQUEST_TARGET_BRANCH_NAME" +# +# Without \`--close\` it reports and writes nothing back, which is the half that +# needs no token at all. project_protocol: image: node:${node}-slim stage: test @@ -139,6 +287,11 @@ ${executesRepositoryCode()} # agent, a developer's ~/.npmrc and ssh-agent, an instance metadata endpoint. # Workfile controls none of it, so "run repository checks where there are no # secrets" is something the caller has to arrange and this file can only state. +# +# Which is also why it does not run the commands cards declare. On GitHub those +# get a job that holds nothing and a separate one that writes — T-0189. Here +# there is nothing to hold them behind, so \`card verify --changed --base REF\` +# is left for a caller who knows what this script inherits. set -eu npx --yes @illodev/workfile@${PACKAGE_VERSION} doctor --json diff --git a/packages/workfile/test/ci-targets.test.ts b/packages/workfile/test/ci-targets.test.ts index a135ee0..8929b7c 100644 --- a/packages/workfile/test/ci-targets.test.ts +++ b/packages/workfile/test/ci-targets.test.ts @@ -88,13 +88,134 @@ test("every target states that the checkout's own config executes, both hops", a } }); -test("no generated target lowers a card-declared command into a shell", async () => { - // The pin that keeps the next card honest. A `verify[].run` is an argument - // vector precisely so that no shell parses it; writing one into a YAML - // `run:` block or into the generic sh script would hand it straight back to - // one and make the allowlist a claim about a string again. +/** + * The pin T-0188 left for the next card, kept and made precise. + * + * It read `doesNotMatch(/card verify/)` and `doesNotMatch(/\$\{\{/)` on every + * target, and T-0189 is the card it was left for — so it is worth saying exactly + * what it was protecting and what it was not. + * + * It was protecting two things. A `verify[].run` is an argument vector precisely + * so that no shell parses it, and writing one into a YAML `run:` block would hand + * it back to one and make the allowlist a claim about a string again. And a + * `\${{ }}` inside a `run:` block is expanded by Actions *before* the shell sees + * it, so a branch name or a PR title interpolated there is code. + * + * Neither is what invoking the runner does. `card verify --changed` is a fixed + * argument vector of the tool's own flags; the card's command never appears in + * the workflow, and the tool spawns it with no shell — which is the property + * T-0188 actually bought. So the two blanket assertions are replaced by the two + * rules they stood for, and the second is now checked where it matters rather + * than everywhere: inside `run:` scripts only, since `env:` and `if:` are where + * an expression belongs. + */ +test("no generated target hands a card's command, or an expression, to a shell", async () => { for (const [id, body] of Object.entries(await bodies())) { - assert.doesNotMatch(body, /card verify/, id); - assert.doesNotMatch(body, /\$\{\{/, id); + // Nothing composes a command out of card data. The workflow names the + // tool and its flags; what the tool then runs it reads off the card and + // spawns as a vector. + assert.doesNotMatch( + body, + /verify\[|\.run\b|criteria:/, + `${id} reads a card's verify block into the template` + ); + + // And no Actions expression reaches a shell. Scanned by indentation + // rather than by one regex over the whole file: a `run: |` block is its + // opening line plus every following line indented past it, and the + // continuation lines are exactly where an interpolation would hide. The + // first version of this check used a single lookahead pattern, and a + // mutation that put `\${{ github.base_ref }}` on a continuation line + // passed it — the assertion was the broken half, and a pin that does not + // bite is worse than no pin. + const lines = body.split("\n"); + const shellLines: string[] = []; + // The generic target is a shell script end to end, so every line of it + // that is not a comment already is one. + if (id === "generic") { + shellLines.push( + ...lines.filter((line) => line.trim() && !line.trimStart().startsWith("#")) + ); + } + for (let index = 0; index < lines.length; index += 1) { + // `run:` on GitHub, `script:` on GitLab — the same thing under two + // names, and both hand their contents to a shell. + const opening = /^(\s*)(?:-\s+)?(?:run|script):(.*)$/.exec(lines[index]); + if (!opening) continue; + const indent = opening[1].length; + // `run: something` on one line is itself a shell line. + if (opening[2].trim() && !/^[|>]/.test(opening[2].trim())) { + shellLines.push(lines[index]); + continue; + } + for (let next = index + 1; next < lines.length; next += 1) { + const line = lines[next]; + if (!line.trim()) continue; + const width = line.length - line.trimStart().length; + if (width <= indent) break; + shellLines.push(line); + } + } + // A floor per target, because the three formats carry different amounts + // of shell: GitHub has four `run:` steps across two jobs, GitLab has a + // two-line `script:`, and the generic file is shell throughout. Set at + // all so a scan that silently stops matching fails loudly instead of + // reporting a clean sweep over nothing. + const floor = id === "gitlab" ? 2 : id === "generic" ? 3 : 8; + assert.ok( + shellLines.length >= floor, + `${id}: found ${shellLines.length} shell lines, expected at least ` + + `${floor} — the scan stopped matching rather than the shell going away` + ); + for (const line of shellLines) { + assert.doesNotMatch( + line, + /\$\{\{/, + `${id} interpolates an Actions expression into a shell: ${line.trim()}` + ); + } } }); + +/** + * And the two-job split, which is the whole safety of T-0189. + * + * One job runs commands a pull request declared and holds nothing. Another holds + * a write token and runs no repository code — not even Workfile, because every + * Workfile command `import()`s `project.config.mjs` from the checkout. A change + * that merges them, or that teaches the write job to run the tool, is the one + * mistake here that would not look like a mistake. + */ +test("the job that runs card commands and the job that writes are not the same job", async () => { + const github = (await bodies()).github; + const job = (name: string) => { + const start = github.indexOf(`\n ${name}:\n`); + assert.notEqual(start, -1, `the ${name} job is gone`); + const rest = github.slice(start + 1); + const next = rest.search(/\n {2}\w+:\n/); + return next === -1 ? rest : rest.slice(0, next); + }; + + const cards = job("cards"); + assert.match(cards, /^ {4}permissions: \{\}$/m, "the card runner holds a scope"); + assert.match(cards, /card verify --changed/); + assert.match(cards, /persist-credentials: false/); + // Without the full history there is no merge base, and a card diff taken + // against nothing is the failure this whole feature must not have. + assert.match(cards, /fetch-depth: 0/); + + const record = job("record"); + assert.match(record, /^ {4}permissions:\n {6}contents: write$/m); + assert.doesNotMatch( + record, + /@illodev\/workfile/, + "the write-scoped job invokes Workfile, which imports the checkout's config" + ); + // A patch out of the untrusted job is applied here, so its reach is bounded + // before it is applied rather than trusted because of where it came from. + assert.match(record, /refusing a patch that reaches outside/); + assert.match(record, /git apply --check/); + // Fork pull requests never start this job. The token would be read-only + // anyway; the condition is what says so. + assert.match(record, /head\.repo\.full_name == github\.repository/); +}); diff --git a/packages/workfile/test/self-reference.test.ts b/packages/workfile/test/self-reference.test.ts new file mode 100644 index 0000000..304a703 --- /dev/null +++ b/packages/workfile/test/self-reference.test.ts @@ -0,0 +1,225 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { readdir, readFile, writeFile } from "node:fs/promises"; +import { join } from "node:path"; + +import { createTestWorkspace } from "./support/workspace.ts"; + +import { + createCard, + diagnoseCards, + loadCards, + patchCard +} from "../dist/src/index.js"; + +/** + * A card cannot be its own parent, its own dependency, or its own origin. + * + * Two of those three were refused at write time and the third was not, which is + * how T-0161 was found — in the 0.6.0 smoke test, against the published package. + * `card create --title X --origin T-0001` on a fresh workspace allocates + * `T-0001`, writes `origin: [T-0001]`, and exits 0. `doctor` then reports + * `self-origin` as an error, so a command that said it worked had put the + * repository into a state the protocol calls broken — and the pre-commit hook + * runs `doctor --severity error`, so the failure surfaces at the *next commit*, + * about a card written minutes earlier, with nothing to say which command wrote + * it. + * + * Neither sibling guard had a test either, so all three are pinned here rather + * than only the one that was missing. A guard nothing exercises is the next one + * to go quiet. + */ + +/** + * The three fields, the error each refuses itself with on patch, and the one it + * refuses itself with on create. + * + * They differ, and the difference is the finding T-0161 got wrong. + * `validateCardCandidate` runs before the id is allocated — it sees + * `id: "pending"` — so no self-reference check up there can fire on a create, + * whatever the field. `parent` and `depends` are nevertheless refused, by their + * *existence* rules: the id is not among the loaded cards either. The right + * outcome for the wrong reason, and `origin` has no existence rule to borrow, + * because an origin may legitimately name a record not written yet. Which is why + * nothing caught it and why the fix for it sits at the allocation instead. + */ +const SELF_REFERENCE = [ + { field: "parent", onPatch: "CARD_SELF_PARENT", onCreate: "CARD_PARENT_NOT_FOUND" }, + { + field: "depends", + onPatch: "CARD_SELF_DEPENDENCY", + onCreate: "CARD_DEPENDENCY_NOT_FOUND" + }, + { field: "origin", onPatch: "CARD_SELF_ORIGIN", onCreate: "CARD_SELF_ORIGIN" } +] as const; + +const asList = (field: string, id: string) => + field === "parent" ? id : [id]; + +test("a card cannot name itself in any of the three relationship fields", async () => { + for (const { field, onPatch, onCreate } of SELF_REFERENCE) { + const { workspace, cleanup } = await createTestWorkspace(); + try { + // On creation, which is the case the card was filed about: the + // caller names the id the allocation is about to hand out, so it + // looks like somebody else's card right up until it is theirs. + await assert.rejects( + () => + createCard(workspace, { + title: `Self ${field}`, + area: "api", + [field]: asList(field, "T-0003") + }), + (error: any) => { + assert.equal( + error.code, + onCreate, + `create --${field} naming the allocated id gave ${error.code}` + ); + return true; + }, + `create --${field} naming the id being allocated was accepted` + ); + + // And on patch, where the id is plainly the card's own and every + // field is refused by its own name. + const { cards } = await loadCards(workspace); + const existing = cards[0].id; + await assert.rejects( + () => patchCard(workspace, existing, { [field]: asList(field, existing) }), + (error: any) => { + assert.equal(error.code, onPatch, `patch --${field} gave ${error.code}`); + return true; + }, + `patch setting its own id as ${field} was accepted` + ); + } finally { + await cleanup(); + } + } +}); + +/** + * The allocated id is the one that gets refused, which is the whole subtlety. + * + * `T-0003` above is not a card that exists — it is the id the *next* create will + * be given in that fixture. So this is not "refuse an id you already hold"; it is + * "refuse the id you are about to be given", and it only works because + * `candidate.id` is set by the time validation runs. + */ +test("the id being allocated is the id that is refused", async () => { + const { workspace, cleanup } = await createTestWorkspace(); + try { + const before = await loadCards(workspace); + const next = await createCard(workspace, { title: "Allocates one", area: "api" }); + assert.ok( + !before.cards.some((card: any) => card.id === next.card.id), + "the fixture already held the id this test assumes is free" + ); + // Which is the id the refused create above was naming. + assert.equal(next.card.id, "T-0003"); + } finally { + await cleanup(); + } +}); + +/** + * The three codes read alike, which is criterion 3 of the card and not + * decoration: an agent reading `CARD_SELF_ORIGIN` after having met + * `CARD_SELF_PARENT` should not have to check whether it means the same shape of + * thing. + */ +test("the three refusals are named the same way", async () => { + const source = await readFile( + new URL("../src/modules/cards/validation.ts", import.meta.url), + "utf8" + ); + const codes = [...source.matchAll(/"(CARD_SELF_[A-Z_]+)"/g)].map((match) => match[1]); + assert.deepEqual( + [...new Set(codes)].sort(), + ["CARD_SELF_DEPENDENCY", "CARD_SELF_ORIGIN", "CARD_SELF_PARENT"], + "a self-reference guard was added or renamed out of the family" + ); +}); + +/** + * And the `doctor` rule stays, because the write-time guard only protects writes + * that go through it. + * + * Every record written before this landed came through a version that allowed it, + * and a workspace is edited by hand and by other tools. So the file is written + * directly here — bypassing validation, the way history did — and `doctor` still + * has to report it. + */ +test("doctor still reports a self-origin written outside the protocol", async () => { + const { root, workspace, cleanup } = await createTestWorkspace(); + try { + const directory = join(root, workspace.config.cards.path); + const name = (await readdir(directory)).find((entry) => entry.endsWith(".md")); + assert.ok(name, "the fixture has no cards"); + const path = join(directory, name); + const original = await readFile(path, "utf8"); + const id = /^id:\s*(\S+)/m.exec(original)?.[1]; + assert.ok(id, "the fixture card has no id"); + await writeFile( + path, + original.replace(/^updated:.*$/m, (line) => `${line}\norigin: [${id}]`) + ); + + const loaded = await loadCards(workspace); + const report = await diagnoseCards({ ...loaded, workspace, checkPaths: false }); + const found = report.issues.filter((issue: any) => issue.code === "self-origin"); + assert.equal( + found.length, + 1, + `expected one self-origin error, got ${JSON.stringify( + report.issues.map((issue: any) => issue.code) + )}` + ); + assert.equal(found[0].severity, "error"); + } finally { + await cleanup(); + } +}); + +/** + * A refused create leaves nothing behind. + * + * The card's complaint was not only that the write was accepted — it was that a + * command reporting success had put the repository into a state `doctor` calls an + * error. So the refusal has to happen before the file exists, not by writing and + * then complaining. The check sits inside the allocation callback, ahead of + * `createFileExclusive`, and a `ValidationError` is not create contention, so it + * leaves the retry loop instead of being read as a collision and retried onto the + * next id. + */ +test("a refused create writes no card and consumes no id", async () => { + const { workspace, cleanup } = await createTestWorkspace(); + try { + const before = await loadCards(workspace); + await assert.rejects( + () => + createCard(workspace, { + title: "Came out of the release", + area: "api", + origin: ["T-0003"] + }), + (error: any) => { + assert.equal(error.code, "CARD_SELF_ORIGIN"); + return true; + } + ); + const after = await loadCards(workspace); + assert.equal( + after.cards.length, + before.cards.length, + "the refused create wrote a card anyway" + ); + // And the id it would have taken is still free, so the next create gets + // it rather than skipping to T-0004. + const next = await createCard(workspace, { title: "The next one", area: "api" }); + assert.equal(next.card.id, "T-0003"); + } finally { + await cleanup(); + } +}); diff --git a/project.config.mjs b/project.config.mjs index d6ff44b..07c4e57 100644 --- a/project.config.mjs +++ b/project.config.mjs @@ -2,6 +2,14 @@ export default { schemaVersion: 2, name: "Workfile", cards: { + // What a card's `verify[].run` may be, as argv prefixes. Empty by + // default so a project that declares nothing can run nothing; this one + // declares the suite, because that is what decides a criterion here. + // Spawned with no shell — the vector reaches the OS as written — and the + // job that runs it in CI holds `permissions: {}`. See T-0188, T-0189. + verification: { + commands: [["node", "--test"]] + }, areas: ["core", "ui", "docs", "infra", "mcp", "search"] }, docs: { From 5b61847f93bd6627f5062612f1f7d65b24f90e12 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 7 Aug 2026 21:28:17 +0000 Subject: [PATCH 2/3] Record card verification from CI --- ...61-a-card-can-be-written-with-itself-as-its-origin.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/.project/cards/T-0161-a-card-can-be-written-with-itself-as-its-origin.md b/.project/cards/T-0161-a-card-can-be-written-with-itself-as-its-origin.md index 9dc7c49..3aa2582 100644 --- a/.project/cards/T-0161-a-card-can-be-written-with-itself-as-its-origin.md +++ b/.project/cards/T-0161-a-card-can-be-written-with-itself-as-its-origin.md @@ -58,13 +58,14 @@ that does not exist yet. ## Acceptance criteria -- [ ] `card create --origin` naming the ID being allocated is refused -- [ ] `card patch` setting a card's own ID as its origin is refused -- [ ] The error code reads like its two neighbours -- [ ] The doctor rule stays, for records written before this landed +- [x] `card create --origin` naming the ID being allocated is refused +- [x] `card patch` setting a card's own ID as its origin is refused +- [x] The error code reads like its two neighbours +- [x] The doctor rule stays, for records written before this landed - [ ] `pnpm run check` green, doctor 0/0 ## Activity - 2026-08-07 21:19Z illodev@local#42eb42f5 · claimed - 2026-08-07 21:24Z illodev@local#42eb42f5 · verify self-reference: node --test packages/workfile/test/self-reference.test.ts passed, checked #1, #2, #3, #4 +- 2026-08-07 21:28Z runner@local · verify self-reference: node --test packages/workfile/test/self-reference.test.ts passed, checked #1, #2, #3, #4 From cd7615b2529529175bea41b9e60a5bf57d09101a Mon Sep 17 00:00:00 2001 From: illodev Date: Fri, 7 Aug 2026 23:43:34 +0200 Subject: [PATCH 3/3] Override nanoid, and correct what actually stops the record job looping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit gate went red on this branch for a reason unrelated to it, which is the cost that was named when the floor moved to `moderate`: GHSA-2v37-7h3g-55p8 against `nanoid <3.3.17`, reached through `shadcn > postcss` and `vite > postcss` — both devDependency paths, so the consumer tree was never affected and `audit:consumer` stayed clean. One override, and the workspace audits clean at `low` again. And a correction the run itself produced. The record job's comment said the push converges because the second run finds nothing to write. That is true and it is not the operative reason: a push made with GITHUB_TOKEN does not start a workflow run at all, which showed up on #36 as two runs created in `action_required` that never executed. Both reasons are stated now, the load-bearing one first, because somebody debugging a loop would otherwise be looking at the wrong one. T-0161 closes with `method: ci` against the run that proved it. T-0189 goes to review: three of its four criteria are proven, and the fourth needs a pull request from a fork to demonstrate. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01D3LTdq3mzMAQ98rwBegGjU --- .github/workflows/workfile.yml | 8 +++++--- ...-can-be-written-with-itself-as-its-origin.md | 17 +++++++++++++---- ...-card-s-declared-checks-and-writes-back-t.md | 12 ++++++++---- package.json | 1 + packages/workfile/src/modules/ci/ci.ts | 10 +++++++--- pnpm-lock.yaml | 9 +++++---- 6 files changed, 39 insertions(+), 18 deletions(-) diff --git a/.github/workflows/workfile.yml b/.github/workflows/workfile.yml index ed1bd1b..49e815d 100644 --- a/.github/workflows/workfile.yml +++ b/.github/workflows/workfile.yml @@ -126,9 +126,11 @@ jobs: echo "::error::refusing a patch that reaches outside .project/" exit 1 fi - # The push re-triggers this workflow, and the second run finds the boxes - # already checked: nothing to write, an empty patch, no commit. It - # converges rather than looping. + # No loop, for two independent reasons and the first is the load-bearing + # one: a push made with GITHUB_TOKEN does not start a workflow run, so + # this does not re-enter. Observed on PR #36 as two runs created in + # `action_required` that never executed. And if one did run, it would find + # the boxes already checked, produce an empty patch and commit nothing. - name: Commit the evidence run: | test -s workfile-cards.patch || exit 0 diff --git a/.project/cards/T-0161-a-card-can-be-written-with-itself-as-its-origin.md b/.project/cards/T-0161-a-card-can-be-written-with-itself-as-its-origin.md index 3aa2582..b1a4ee5 100644 --- a/.project/cards/T-0161-a-card-can-be-written-with-itself-as-its-origin.md +++ b/.project/cards/T-0161-a-card-can-be-written-with-itself-as-its-origin.md @@ -1,7 +1,7 @@ --- id: T-0161 title: A card can be written with itself as its origin -status: doing +status: done type: bug priority: low area: core @@ -10,12 +10,16 @@ scope: [packages/workfile/src/modules/cards/validation.ts] origin: [T-0156] created: 2026-08-04 updated: 2026-08-07 -claimed_by: "illodev@local#42eb42f5" -claimed_at: "2026-08-07T21:19:17.046Z" verify: - id: self-reference run: [node, --test, packages/workfile/test/self-reference.test.ts] criteria: ["sha256:ae2edc316ff93cb65b575452945408f5f712ad33d6b5c7ed4fb61e9a5bb8af1b", "sha256:08de66f38da03ca41f2963e76624fc4140e4fd240792af7fbd1f8b826d7d38b9", "sha256:ee509c4d091e14ef3f3d6ec54722acb30bc019b2d2d5de59db82058afc24e497", "sha256:193e044572b952be47178266f43cdb70cc42ef043b03bdaff153cb343de4d96b"] +verified: + at: "2026-08-07T21:43:04.036Z" + method: ci + commit: 5b61847f93bd6627f5062612f1f7d65b24f90e12 + run: "https://github.com/illodev/workfile/actions/runs/31220115910" + digest: "sha256:f2995cfc1c6bad2b4ffb1aa5b08437eedb13148bf16918e93da009cad8069487" --- Found in the 0.6.0 smoke test, against the published package. On a fresh @@ -62,10 +66,15 @@ that does not exist yet. - [x] `card patch` setting a card's own ID as its origin is refused - [x] The error code reads like its two neighbours - [x] The doctor rule stays, for records written before this landed -- [ ] `pnpm run check` green, doctor 0/0 +- [x] `pnpm run check` green, doctor 0/0 ## Activity - 2026-08-07 21:19Z illodev@local#42eb42f5 · claimed - 2026-08-07 21:24Z illodev@local#42eb42f5 · verify self-reference: node --test packages/workfile/test/self-reference.test.ts passed, checked #1, #2, #3, #4 - 2026-08-07 21:28Z runner@local · verify self-reference: node --test packages/workfile/test/self-reference.test.ts passed, checked #1, #2, #3, #4 +- 2026-08-07 21:43Z illodev@local#42eb42f5 · released + +## Notes + +- 2026-08-07 21:43Z illodev@local#42eb42f5 — ci verification: Four of five criteria checked by the card's own declared command, run by CI on PR #36 and recorded in commit 5b61847 by the job that holds the write token. Criterion 5 is the gate: pnpm run check green at 477 + 10 tests, doctor 0/0. The card's premise was wrong and is corrected on the record: validateCardCandidate runs against id 'pending', so no self-reference check there can fire on a create, and a self parent is refused by CARD_PARENT_NOT_FOUND rather than by its own guard. The origin check therefore sits at the allocation, where the id exists, and a refused create writes no card and does not consume the id. diff --git a/.project/cards/T-0189-ci-runs-a-card-s-declared-checks-and-writes-back-t.md b/.project/cards/T-0189-ci-runs-a-card-s-declared-checks-and-writes-back-t.md index 60ecc79..10db982 100644 --- a/.project/cards/T-0189-ci-runs-a-card-s-declared-checks-and-writes-back-t.md +++ b/.project/cards/T-0189-ci-runs-a-card-s-declared-checks-and-writes-back-t.md @@ -1,7 +1,7 @@ --- id: T-0189 title: CI runs a card's declared checks and writes back the evidence -status: doing +status: review type: feature priority: medium area: infra @@ -12,8 +12,6 @@ created: 2026-08-05 updated: 2026-08-07 origin: [ADR-0016] depends: [T-0188, T-0186] -claimed_by: "illodev@local#42eb42f5" -claimed_at: "2026-08-07T20:49:06.844Z" scope: [packages/workfile/src/modules/ci, packages/workfile/src/modules/cards/runner.ts, packages/workfile/bin/workfile.ts] --- @@ -36,7 +34,7 @@ Open questions to settle before implementing, not after: ## Acceptance criteria -- [ ] The generated GitHub workflow runs the declared checks for cards touched by the branch. +- [x] The generated GitHub workflow runs the declared checks for cards touched by the branch. - [x] A passing run writes `verified` with `method: ci`, the commit and the run URL. - [ ] A fork PR either records evidence safely or records none; it never fails open. - [x] The behaviour is documented in the CLI/CI reference, including what it does not do. @@ -44,6 +42,7 @@ Open questions to settle before implementing, not after: ## Activity - 2026-08-07 20:49Z illodev@local#42eb42f5 · claimed +- 2026-08-07 21:43Z illodev@local#42eb42f5 · doing → review ## Notes @@ -58,3 +57,8 @@ One job per card or one for all: one for all. The card format is a flat command And one thing the card did not anticipate. T-0188 left a tripwire in ci-targets.test.ts, asserting that no generated target contains the string `card verify` or an Actions expression, commented as "the pin that keeps the next card honest" — and this is that card. What it was protecting is real and stands: a verify[].run is an argument vector so that no shell parses it, and an Actions expression inside a run: block is expanded before the shell sees it. Neither is what invoking the runner does: the card's command never appears in the workflow, and the tool spawns it with no shell. So the two blanket assertions are replaced by the two rules they stood for — no target reads a card's verify block into the template, and no Actions expression reaches a shell line, checked by indentation across run: and script: in all three formats. The first version of that second check was broken in the dangerous direction: a mutation putting an Actions expression on a continuation line passed it. Found by mutating, not by reading. It is a line-based scan now, with a per-format floor so a scan that stops matching fails loudly instead of reporting a clean sweep over nothing. +- 2026-08-07 21:43Z illodev@local#42eb42f5 — Criterion 1 proven on PR #36, which is this branch. The `cards` job discovered T-0161 as a card the branch touched, ran its declared command, checked the four criteria bound to it, left the fifth alone, and the `record` job pushed commit 5b61847 — a github-actions[bot] commit whose only change is those four boxes plus a trail line naming runner@local and the command. Run: https://github.com/illodev/workfile/actions/runs/31220115910 + +And one thing the run corrected. The workflow comment claimed the push converges because the second run finds nothing to write. True, but not the operative reason: a push made with GITHUB_TOKEN does not start a workflow run at all. Observed as two runs created in action_required that never executed. Both reasons are stated now, first one first, because someone debugging a loop would otherwise look at the wrong one. + +Criterion 3 is argued and pinned rather than run: demonstrating it needs a pull request from a fork, which needs a second account. What GitHub enforces is that `pull_request` from a fork gets a read-only token, so the push cannot land; the job condition declines to start on top of that, and a test pins the condition. diff --git a/package.json b/package.json index 67c0c9e..d1d99e4 100644 --- a/package.json +++ b/package.json @@ -56,6 +56,7 @@ "overrides": { "fast-uri": "^3.1.5", "hono": "^4.12.34", + "nanoid": "^3.3.17", "js-yaml": "^4.3.1" } } diff --git a/packages/workfile/src/modules/ci/ci.ts b/packages/workfile/src/modules/ci/ci.ts index fc3cf86..dfc2208 100644 --- a/packages/workfile/src/modules/ci/ci.ts +++ b/packages/workfile/src/modules/ci/ci.ts @@ -218,9 +218,13 @@ ${twoJobSplit(" # ")} echo "::error::refusing a patch that reaches outside ${protocolRoot}/" exit 1 fi - # The push re-triggers this workflow, and the second run finds the cards - # already closed: nothing to write, an empty patch, no commit. It - # converges rather than looping. + # No loop, for two independent reasons and the first one is the load + # bearing one: a push made with GITHUB_TOKEN does not start a workflow + # run, so this does not re-enter. Observed as a run created in + # \`action_required\` that never executes. And if it did run, it would find + # the cards already recorded, produce an empty patch and commit nothing — + # which is what makes the first reason safe to rely on rather than + # load-bearing on its own. - name: Commit the evidence run: | test -s workfile-cards.patch || exit 0 diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a9378ae..01477c6 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -7,6 +7,7 @@ settings: overrides: fast-uri: ^3.1.5 hono: ^4.12.34 + nanoid: ^3.3.17 js-yaml: ^4.3.1 importers: @@ -2402,8 +2403,8 @@ packages: ms@2.1.3: resolution: {integrity: sha512-6FlzubTLZG3J2a/NVCAleEhjzq5oxgHyaCU9yYXvcLsvoVaHJq/s5xXI6/XXP6tz7R9xAOtHnSO/tXtF3WRTlA==} - nanoid@3.3.16: - resolution: {integrity: sha512-bzlKTyNJ7+LdGIIwy8ijFpIqEQIvafahV7eYykJ8Cvh42EdJeODoJ6gUJXpQJvej1BddH8OqTXZNE/KfbWAu8Q==} + nanoid@3.3.18: + resolution: {integrity: sha512-DTg4MJbGMWkfi6VZFdNt2/caMbQy4Ou+Op/hJQvGEWcnVfoA1QA+xzRKAzw9jD6+GVOOeYr/mIcuDSdug6F6+w==} engines: {node: ^10 || ^12 || ^13.7 || ^14 || >=15.0.1} hasBin: true @@ -5183,7 +5184,7 @@ snapshots: ms@2.1.3: {} - nanoid@3.3.16: {} + nanoid@3.3.18: {} negotiator@1.0.0: {} @@ -5322,7 +5323,7 @@ snapshots: postcss@8.5.25: dependencies: - nanoid: 3.3.16 + nanoid: 3.3.18 picocolors: 1.1.1 source-map-js: 1.2.1