diff --git a/.github/workflows/workfile.yml b/.github/workflows/workfile.yml index 39e18bb..49e815d 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,111 @@ 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 + # 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 + 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..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: backlog +status: done type: bug priority: low area: core @@ -9,7 +9,17 @@ 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 +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 @@ -52,8 +62,19 @@ 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 -- [ ] `pnpm run check` green, doctor 0/0 +- [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 +- [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 df5f949..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: backlog +status: review type: feature priority: medium area: infra @@ -9,9 +9,10 @@ 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] +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 @@ -33,7 +34,31 @@ 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] 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. -- [ ] 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 +- 2026-08-07 21:43Z illodev@local#42eb42f5 · doing → review + +## 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. +- 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/.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/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/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..dfc2208 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,106 @@ 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 + # 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 + 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 +252,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 +291,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/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 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: {