diff --git a/.github/workflows/fix-drift.yml b/.github/workflows/fix-drift.yml index 15dfe4c1..92e01b5f 100644 --- a/.github/workflows/fix-drift.yml +++ b/.github/workflows/fix-drift.yml @@ -54,8 +54,8 @@ jobs: # daemon the Ollama leg would spuriously quarantine/error on a missing host # instead of behaving as it does under test-drift.yml's own daily run. # - # THIS RUNS FIRST, AND THE SCRIPT IS CHECKSUM-PINNED. Both halves matter and - # neither alone is sufficient. + # THIS RUNS FIRST, AND EVERY BYTE IT EXECUTES IS CHECKSUM-PINNED. Both + # halves matter and neither alone is sufficient. # # `sh <(curl …)` of a MUTABLE URL is arbitrary third-party code executing as # the runner user, and it used to sit six steps after the app token was @@ -66,34 +66,66 @@ jobs: # setup step instead of before it, which is strictly better for a job with # `timeout-minutes: 30`. # - # But reordering alone does not close it. A compromised script can plant a + # But reordering alone does not close it. A compromised payload can plant a # `git`, `gh` or `node` earlier on PATH, or write a shell rc, and every later - # step — the ones that DO hold the token — would execute it. So the bytes are - # verified against a sha256 reviewed at the URL, and a mismatch is a hard - # `exit 1` before `sh` ever sees the file. Everything else in this job's - # third-party surface is already pinned: every `uses:` by commit SHA, and - # `pnpm install --frozen-lockfile` by the lockfile's own integrity hashes. - # This was the one unpinned executable in the job. + # step — the ones that DO hold the token — would execute it. # - # WHEN THIS FAILS: Ollama published a new install.sh. Read the diff at - # https://ollama.com/install.sh, then re-pin OLLAMA_INSTALL_SHA256 below to - # the digest the error message prints. The failure is loud and alerted (the - # end-of-job catch-all covers the infra/setup window), never silent — an - # unverified script must not run just because verifying it was inconvenient. + # ollama.com/install.sh IS NOT USED, and pinning it was not enough. Its own + # bytes can be pinned, but the script then streams + # `https://ollama.com/download/ollama-linux-.tar.zst` — a mutable, + # unversioned URL carrying no digest — straight into `sudo tar -x` under + # /usr/local, so a SECOND unpinned payload plants root-owned binaries on + # PATH just as effectively as the script could. It cannot be fixed in place + # either: the script pipes that download through `zstd -d` into `tar`, so it + # never holds the file and has nothing to verify. (On a GPU host it also adds + # NVIDIA CUDA apt/yum repos and runs `$PACKAGE_MANAGER -y install` — more + # unpinned root execution this job has no use for.) + # + # So the release artifact is fetched DIRECTLY and verified before anything + # unpacks it: one URL, pinned to an immutable release tag, whose bytes must + # match a reviewed sha256 or the step hard-`exit 1`s before `tar` runs. The + # tarball is the whole product — `bin/ollama` plus `lib/ollama/*` — and this + # step already ran `ollama serve` itself rather than using the systemd unit + # install.sh sets up, so nothing else in that script was load-bearing here. + # + # Every other byte this job executes is already pinned: each `uses:` by + # commit SHA, and `pnpm install --frozen-lockfile` by the lockfile's own + # integrity hashes. With this, nothing in the job runs code it has not + # verified. + # + # WHEN THIS FAILS: Ollama cut a new release, or an artifact was rebuilt. + # Pick the version at https://github.com/ollama/ollama/releases, take + # `ollama-linux-amd64.tar.zst`'s digest from that release's own + # `sha256sum.txt`, and update BOTH values below together. The failure is loud + # and alerted (the end-of-job catch-all covers the infra/setup window), never + # silent — unverified bytes must not be unpacked as root just because + # verifying them was inconvenient. - name: Provision Ollama daemon (drift-sync-check re-collect gate) env: - # sha256 of https://ollama.com/install.sh, 15902 bytes, reviewed 2026-08-05. - OLLAMA_INSTALL_SHA256: 25f64b810b947145095956533e1bdf56eacea2673c55a7e586be4515fc882c9f + # ollama-linux-amd64.tar.zst from the v0.32.6 release, 1420686963 bytes. + # Digest agreed on 2026-08-05 by three independent sources: the release's + # sha256sum.txt, the GitHub release API's own asset `digest` field, and + # sha256 of the downloaded bytes. + OLLAMA_VERSION: v0.32.6 + OLLAMA_TARBALL_SHA256: dec2fa50d24e6868ca3c4c977d69d059399372105f951a9acc320a5a79aadcfc run: | set -euo pipefail - SCRIPT="${RUNNER_TEMP}/ollama-install.sh" - curl -fsSL https://ollama.com/install.sh -o "$SCRIPT" - ACTUAL="$(sha256sum "$SCRIPT" | cut -d' ' -f1)" - if [ "$ACTUAL" != "${OLLAMA_INSTALL_SHA256}" ]; then - echo "::error::ollama.com/install.sh does not match its pinned sha256 — REFUSING to execute it. Expected ${OLLAMA_INSTALL_SHA256}, got ${ACTUAL}. Review the script at that URL and re-pin OLLAMA_INSTALL_SHA256 in .github/workflows/fix-drift.yml if the change is legitimate." + # Fail on a MISSING decompressor rather than discovering it mid-pipe, + # where `tar` would be handed a truncated stream as root. + if ! command -v zstd >/dev/null 2>&1; then + echo "::error::zstd is not installed on this runner, so the pinned Ollama tarball cannot be unpacked. Install zstd before this step." exit 1 fi - sh "$SCRIPT" + TARBALL="${RUNNER_TEMP}/ollama-linux-amd64.tar.zst" + curl -fsSL "https://github.com/ollama/ollama/releases/download/${OLLAMA_VERSION}/ollama-linux-amd64.tar.zst" -o "$TARBALL" + ACTUAL="$(sha256sum "$TARBALL" | cut -d' ' -f1)" + if [ "$ACTUAL" != "${OLLAMA_TARBALL_SHA256}" ]; then + echo "::error::the Ollama ${OLLAMA_VERSION} tarball does not match its pinned sha256 — REFUSING to unpack it as root. Expected ${OLLAMA_TARBALL_SHA256}, got ${ACTUAL}. Check the release's sha256sum.txt and re-pin OLLAMA_VERSION/OLLAMA_TARBALL_SHA256 in .github/workflows/fix-drift.yml if the change is legitimate." + exit 1 + fi + # Only now, on bytes that matched. The archive is `bin/ollama` + + # `lib/ollama/*`, so /usr/local puts the binary on the default PATH. + zstd -d -c "$TARBALL" | sudo tar -xf - -C /usr/local ollama serve > /tmp/ollama-serve.log 2>&1 & for _ in $(seq 1 30); do if curl -sf http://127.0.0.1:11434/api/version >/dev/null 2>&1; then @@ -491,9 +523,11 @@ jobs: # Identity must be body-INDEPENDENT, since a body-keyed lookup cannot # find a PR whose body lost the key: the head branch ends in # "-", and renaming a branch closes any open PR whose head - # it is, so for an open PR that suffix is fixed. Listed with a plain - # state-filtered query, never `--search`, whose index is body-keyed AND - # lags an edit by minutes. + # it is, so for an open PR that suffix is fixed. Never listed with a + # ` in:body` search: that index is BODY-keyed and lags an edit by + # minutes, so it is blind to precisely the deletion this repair exists + # for. (`is:unmerged` below is a STATE predicate and carries neither + # property — see the window note.) # # `--state all`, NOT `--state open`, and the repair then scoped to OPEN or # CLOSED. A CLOSED PR carrying this marker is how a human REJECTS a @@ -514,7 +548,53 @@ jobs: # decision, never a pending proposal and never a rejection, so no guard # here consults it and appending machine markers to it would be a write # with no reader. - if ! MANAGED="$(gh pr list --state all --limit 200 \ + # + # ---- THE WINDOW IS PART OF THE ANSWER -------------------------------- + # `--limit N` is not "up to N", it is a WINDOW: gh returns the newest N + # rows matching the query and drops the rest with no flag, no warning + # and no count. The `--state` filter above is applied CLIENT-side by the + # jq below, so a plain `--state all` listing spends its whole window on + # every PR this repo has — renovate, human, bot — and the merged ones, + # which no guard in this step consults, crowd out the ones that decide. + # + # Measured on this repo on 2026-08-05: `--state all --limit 200` came + # back FULL, 184 of the 200 MERGED, reaching back only as far as #125. + # The repo had 26 unmerged-closed PRs and this listing could see 14 — + # twelve were ALREADY outside the window. A closed drift PR that ages + # out the same way does not make this step cautious, it makes it wrong: + # "no closed PR carries this key" is what it reads, so it re-proposes a + # changeset a human REJECTED, every morning, exactly the bug the widened + # state filter exists to fix, restored silently by a window nobody + # watched. + # + # So the merged population is excluded SERVER-side by a second listing. + # `--search "is:unmerged"` is a STATE predicate, not a body one: unlike + # ` in:body` it cannot be defeated by the very body edit this + # self-heal is here to repair, and a PR closed moments ago matches it + # under either indexed state (open is unmerged too). Same measurement: + # it returned the complete unmerged population, 28 rows, back to #1. + # `--state closed` is NOT a substitute — gh maps it to CLOSED-or-MERGED + # and that query returned 186 merged against 14 closed, just as full. + # + # The plain listing is UNIONED rather than replaced. It is the one view + # that needs no search index at all, so it stays as the index-free + # floor; the unmerged listing supplies the aged-out closed PRs it can no + # longer reach. Neither is trusted alone, and plain entries win on + # collision so nothing this step already saw today can change under it. + PR_LIST_LIMIT=200 + # SATURATION IS A FAULT, NOT A RESULT. A truncated listing is + # indistinguishable from a complete one — same shape, same exit code — + # and every guard downstream reads "absent" where the honest answer is + # "did not look". A listing that comes back FULL cannot be proven + # complete, so it is REFUSED rather than believed: fail-closed and loud + # beats a duplicate PR nobody can explain. + assert_listing_complete() { + if [ "$(printf '%s' "$2" | jq 'length')" -ge "${PR_LIST_LIMIT}" ]; then + echo "::error::the ${1} came back FULL at its --limit of ${PR_LIST_LIMIT}, so it is TRUNCATED and every PR older than the newest ${PR_LIST_LIMIT} is invisible to it. Answering a dedup or rejection lookup from a truncated listing reports 'not proposed' / 'not rejected' for a PR that exists, which opens a duplicate PR or re-proposes a changeset a human rejected. Refusing to decide on it — narrow the query or raise --limit in .github/workflows/fix-drift.yml." + exit 1 + fi + } + if ! MANAGED="$(gh pr list --state all --limit "${PR_LIST_LIMIT}" \ --json number,url,state,body,headRefName 2>&1)"; then echo "::error::gh pr list failed, so this run cannot prove the dedup marker it owns is still intact on the PRs it manages — refusing to risk a duplicate: ${MANAGED}" exit 1 @@ -523,6 +603,23 @@ jobs: echo "::error::gh pr list returned a non-array payload — refusing to self-heal markers against it: ${MANAGED}" exit 1 fi + if ! UNMERGED="$(gh pr list --state all --limit "${PR_LIST_LIMIT}" \ + --search "is:unmerged" --json number,url,state,body,headRefName 2>&1)"; then + echo "::error::gh pr list (is:unmerged) failed, so this run cannot see the closed PRs that carry human rejections and would re-propose a rejected changeset — refusing: ${UNMERGED}" + exit 1 + fi + if ! printf '%s' "$UNMERGED" | jq -e 'type == "array"' >/dev/null 2>&1; then + echo "::error::gh pr list (is:unmerged) returned a non-array payload — refusing to self-heal markers against it: ${UNMERGED}" + exit 1 + fi + # Only the unmerged listing is audited for saturation: the plain one is + # EXPECTED to be full (merged PRs alone exceed the limit) and is not + # relied on for completeness — that is exactly why it is unioned with a + # query whose population is bounded by the repo's unmerged PRs. + assert_listing_complete "unmerged-PR listing this step's marker self-heal reads" "$UNMERGED" + MANAGED="$(jq -cn --argjson a "$MANAGED" --argjson b "$UNMERGED" \ + '($a | map(.number)) as $seen + | $a + [$b[] | . as $p | select(($seen | index($p.number)) == null)]')" MINE="$(printf '%s' "$MANAGED" | jq -c --arg k "$CHANGESET_KEY" ' map(select((.state == "OPEN" or .state == "CLOSED") and (.headRefName | test("^fix/drift-")) @@ -577,9 +674,11 @@ jobs: # which repairs only live PRs): a CLOSED proposal is a REJECTION this # guard has to see, or closing a PR to reject it earns an identical one # the next morning, daily, forever. `--search` narrows server-side on the - # changeset key, so widening the state filter cannot let closed PRs crowd - # the older matching PR out of the `--limit` window. - if ! ALL_PRS="$(gh pr list --state all --limit 200 \ + # changeset key, so — unlike the self-heal listing above — a merged PR + # cannot spend a slot here and widening the state filter cannot crowd the + # older matching PR out of the `--limit` window. The audit below holds + # that reasoning to account rather than assuming it. + if ! ALL_PRS="$(gh pr list --state all --limit "${PR_LIST_LIMIT}" \ --search "${CHANGESET_KEY} in:body" --json number,url,body,state 2>&1)"; then echo "::error::gh pr list failed, so this run cannot prove changeset ${CHANGESET_KEY} is not already proposed — refusing to open a possibly-duplicate PR: ${ALL_PRS}" exit 1 @@ -588,14 +687,15 @@ jobs: echo "::error::gh pr list returned a non-array payload — refusing to dedup against it: ${ALL_PRS}" exit 1 fi - # Fold the self-heal's own (search-index-free) view in. `--search` above - # is index-backed and lags a body edit by minutes, so a PR whose marker - # this step just restored is very likely STILL absent from ALL_PRS — - # dropping it here would re-open exactly the duplicate the repair just - # prevented. Union rather than replace: ALL_PRS is the only source of - # CLOSED (human-rejected) PRs, which the `--state open` self-heal list - # cannot see. Self-heal entries win on collision — they carry the - # post-repair body. + assert_listing_complete "changeset-keyed dedup listing for ${CHANGESET_KEY}" "$ALL_PRS" + # Fold the self-heal's own view in. `--search " in:body"` is a + # BODY-keyed index and lags an edit by minutes, so a PR whose marker this + # step just restored is very likely STILL absent from ALL_PRS — dropping + # it here would re-open exactly the duplicate the repair just prevented. + # Union rather than replace, in both directions: ALL_PRS reaches PRs the + # self-heal's branch-key anchor never selects, and the self-heal reaches + # the closed PRs whose body no longer contains the key at all. Self-heal + # entries win on collision — they carry the post-repair body. ALL_PRS="$(jq -cn --argjson a "$ALL_PRS" --argjson b "$REASSERTED" \ '($b | map(.number)) as $healed | [$a[] | . as $p | select(($healed | index($p.number)) == null)] + $b')" @@ -858,9 +958,10 @@ jobs: # hashes to a DIFFERENT changeset key: (a) cannot match it, the # changeset guard legitimately does not fire, and the per-note # marker is the ONLY thing standing between it and a duplicate PR. - # Listed with a plain state-filtered query, never `--search`: that index - # is body-keyed AND lags an edit by minutes, so it cannot see a body this - # step is about to repair. + # Never listed with a ` in:body` search: that index is BODY-keyed + # AND lags an edit by minutes, so it cannot see a body this step is about + # to repair. (`is:unmerged` below is a STATE predicate and carries + # neither property — see the window note.) # # `--state all`, not `--state open`, for the reason spelled out on the # ok-applied step above: a CLOSED PR's body is where a human's REJECTION of @@ -878,7 +979,39 @@ jobs: # (see bot_managed below). Dropping it from this field list silently # narrows candidacy back to keyed branches, so the presence check below # fails closed on it rather than letting coverage quietly shrink. - if ! MANAGED="$(gh pr list --state all --limit 200 \ + # + # ---- THE WINDOW IS PART OF THE ANSWER -------------------------------- + # Same window defect, same remedy, as the ok-applied step above; the two + # steps each carry their own listing, so a fix applied to one of them + # leaves the other saturating exactly as before. `--limit N` is a WINDOW: + # gh returns the newest N rows and drops the rest with no flag, no + # warning and no count, and this listing's `--state` filter is applied + # CLIENT-side by the jq below, so merged PRs no guard here consults spend + # the window that decides. Measured on this repo on 2026-08-05: + # `--state all --limit 200` came back FULL, 184 of 200 MERGED, reaching + # back only to #125, with twelve of the repo's 26 unmerged-closed PRs + # ALREADY outside it. A rejection that ages out that way reads as "never + # rejected" and gets re-proposed every morning, silently. + # + # `--search "is:unmerged"` is a STATE predicate: no merged PR can spend a + # slot, it is not defeated by the body edit this self-heal repairs, and a + # just-closed PR matches under either indexed state. It returned the + # complete unmerged population, 28 rows, back to #1. `--state closed` is + # NOT a substitute — gh maps it to CLOSED-or-MERGED (186 merged against + # 14 closed on the same measurement). The plain listing is UNIONED, not + # replaced: it is the index-free floor, and plain entries win on + # collision. + PR_LIST_LIMIT=200 + # SATURATION IS A FAULT, NOT A RESULT — a truncated listing is + # indistinguishable from a complete one, and the guards below then read + # "absent" where the honest answer is "did not look". Refuse it. + assert_listing_complete() { + if [ "$(printf '%s' "$2" | jq 'length')" -ge "${PR_LIST_LIMIT}" ]; then + echo "::error::the ${1} came back FULL at its --limit of ${PR_LIST_LIMIT}, so it is TRUNCATED and every PR older than the newest ${PR_LIST_LIMIT} is invisible to it. Answering a dedup or rejection lookup from a truncated listing reports 'not proposed' / 'not rejected' for a PR that exists, which opens a duplicate PR or re-proposes a changeset a human rejected. Refusing to decide on it — narrow the query or raise --limit in .github/workflows/fix-drift.yml." + exit 1 + fi + } + if ! MANAGED="$(gh pr list --state all --limit "${PR_LIST_LIMIT}" \ --json number,url,state,body,headRefName,files,author 2>&1)"; then echo "::error::gh pr list failed, so this run cannot prove the dedup markers it owns are still intact on the PRs it manages — refusing to risk a duplicate: ${MANAGED}" exit 1 @@ -887,6 +1020,23 @@ jobs: echo "::error::gh pr list returned a non-array payload — refusing to self-heal markers against it: ${MANAGED}" exit 1 fi + if ! UNMERGED="$(gh pr list --state all --limit "${PR_LIST_LIMIT}" \ + --search "is:unmerged" --json number,url,state,body,headRefName,files,author 2>&1)"; then + echo "::error::gh pr list (is:unmerged) failed, so this run cannot see the closed PRs that carry human rejections and would re-propose a rejected changeset — refusing: ${UNMERGED}" + exit 1 + fi + if ! printf '%s' "$UNMERGED" | jq -e 'type == "array"' >/dev/null 2>&1; then + echo "::error::gh pr list (is:unmerged) returned a non-array payload — refusing to self-heal markers against it: ${UNMERGED}" + exit 1 + fi + # Only the unmerged listing is audited: the plain one is EXPECTED to be + # full (merged PRs alone exceed the limit) and is not relied on for + # completeness — which is exactly why it is unioned with a query whose + # population is bounded by the repo's unmerged PRs. + assert_listing_complete "unmerged-PR listing this step's marker self-heal reads" "$UNMERGED" + MANAGED="$(jq -cn --argjson a "$MANAGED" --argjson b "$UNMERGED" \ + '($a | map(.number)) as $seen + | $a + [$b[] | . as $p | select(($seen | index($p.number)) == null)]')" # `has("author")`, not "the login is non-empty": gh always emits every key # it was asked for, so an ABSENT key means the `--json` list above (or # gh's payload shape) changed and half of bot_managed's coverage went with @@ -1107,9 +1257,11 @@ jobs: # to "set `Decision: include` … or close to reject", so a dedup view that # cannot see a CLOSED PR turns following that instruction into a brand-new # identical PR the next morning, and every morning after. `--search` - # narrows server-side on the changeset key, so widening the state filter - # cannot push the older matching PR out of the window. - if ! ALL_PRS="$(gh pr list --state all --limit 200 \ + # narrows server-side on the changeset key, so — unlike the self-heal + # listing above — a merged PR cannot spend a slot here and widening the + # state filter cannot push the older matching PR out of the window. The + # audit below holds that reasoning to account rather than assuming it. + if ! ALL_PRS="$(gh pr list --state all --limit "${PR_LIST_LIMIT}" \ --search "${CHANGESET_KEY} in:body" --json number,url,body,state 2>&1)"; then echo "::error::gh pr list failed, so this run cannot prove this decision is not already proposed — refusing to open a possibly-duplicate PR: ${ALL_PRS}" exit 1 @@ -1118,10 +1270,12 @@ jobs: echo "::error::gh pr list returned a non-array payload — refusing to dedup against it: ${ALL_PRS}" exit 1 fi - # Fold the self-heal's own (search-index-free) view in, exactly as the - # ok-applied path above does and for the same reason: the `--search` index - # lags a body edit, so a PR whose marker this step just restored is likely - # still absent here, while ALL_PRS is the only source of CLOSED PRs. + assert_listing_complete "changeset-keyed dedup listing for ${CHANGESET_KEY}" "$ALL_PRS" + # Fold the self-heal's own view in, exactly as the ok-applied path above + # does and for the same reason: the ` in:body` index lags a body + # edit, so a PR whose marker this step just restored is likely still + # absent here, while the self-heal reaches the closed PRs whose body no + # longer contains the key at all. ALL_PRS="$(jq -cn --argjson a "$ALL_PRS" --argjson b "$REASSERTED" \ '($b | map(.number)) as $healed | [$a[] | . as $p | select(($healed | index($p.number)) == null)] + $b')" @@ -1364,9 +1518,12 @@ jobs: # CLOSED, and both suppress it for good: the closure is permanent and the # needs-human alert is additionally gated on `rejected == ''`. Enacted with # `exit 0` alone, such a run is byte-identical to a quiet day — the encoding - # collision the rest of this workflow exists to break — and it has no recovery - # path either, because the marker self-heal lists `--state open` only and so - # never touches the closed PR. + # collision the rest of this workflow exists to break. And the suppression is + # now SELF-REPAIRING: the marker self-heal above reaches closed PRs, so + # deleting the marker from the closed PR's body does not un-suppress the + # changeset, it just gets the marker written back on the next run. That makes + # naming the real way out mandatory rather than merely helpful — the one + # route a human would reach for first is the one route that no longer works. # # So the re-PROPOSAL is suppressed and the TELLING is not: this names the # closing PR and the two ways back out. It does NOT fail the job — a respected diff --git a/src/__tests__/fix-drift-workflow.test.ts b/src/__tests__/fix-drift-workflow.test.ts index f15adec2..fddd387e 100644 --- a/src/__tests__/fix-drift-workflow.test.ts +++ b/src/__tests__/fix-drift-workflow.test.ts @@ -863,9 +863,11 @@ const MAPFILE_SHIM = [ * `diff --name-only` from `headSha`/`diffFiles`; `gh pr list` answers the * self-heal candidate listing and the body-keyed dedup listing separately, * because pointing one selector at the other listing is itself a defect this - * file guards. The two are told apart by `--search`, NOT by `--state`: both are - * `--state all` now that the marker repair has to reach the CLOSED PR that - * records a rejection, and only the dedup listing is body-keyed. + * file guards. They are told apart by the SEARCH QUERY, not by `--state`: all of + * them are `--state all` now that the marker repair has to reach the CLOSED PR + * that records a rejection. Only ` in:body` is body-keyed; `is:unmerged` is + * the self-heal's own view, narrowed server-side so merged PRs cannot spend its + * `--limit` window, and is answered from the same source as the plain listing. */ function observePersistStep( step: Step, @@ -910,6 +912,12 @@ function observePersistStep( [ `printf '%s\\n' "$*" >> ${JSON.stringify(logFile)}`, 'case "$*" in', + // `is:unmerged` is the SELF-HEAL's listing, not the dedup one, so it is + // answered from the same source as the plain state listing. Ordered ahead + // of the generic `--search` arm, which would otherwise hand the self-heal + // the body-keyed dedup population and quietly change what these scenarios + // are testing. + ' *"is:unmerged"*) cat "$GH_OPEN_JSON" ;;', ' *"--search"*) cat "$GH_ALL_JSON" ;;', ' "pr list --state open"*|"pr list --state all"*) cat "$GH_OPEN_JSON" ;;', "esac", @@ -1115,6 +1123,13 @@ function observePrStep( join(fix, "search.json"), JSON.stringify(prs.filter((p) => p.body.includes(sc.changesetKey))), ); + // `is:unmerged` is a STATE predicate, not a body one: it matches the whole + // unmerged population — every OPEN and every CLOSED-never-merged PR — and a + // MERGED PR can never crowd one out of the window. + writeFileSync( + join(fix, "unmerged.json"), + JSON.stringify(prs.filter((p) => p.state !== "MERGED")), + ); writeFileSync(join(fix, "match.out"), sc.matchOut ?? ""); const calls = join(dir, "gh-calls.log"); writeFileSync(calls, ""); @@ -1142,11 +1157,22 @@ function observePrStep( "#!/bin/sh", `printf '%s\\n' "$*" >> ${JSON.stringify(calls)}`, 'ARGS="$*"', + // `--limit` TRUNCATES, and the truncation is the whole point. Real gh + // returns the NEWEST n PRs matching the query and drops the rest + // silently — no flag, no warning, no count. A stub that hands back the + // entire population regardless of `--limit` cannot tell a saturated + // window from a healthy one, so every guard reading a saturated listing + // "passes" here while returning a false negative on the real repo. + // Newest-first by PR number is gh's own default listing order. + "LIMIT=30; prev=''", + 'for a in "$@"; do [ "$prev" = "--limit" ] && LIMIT="$a"; prev="$a"; done', + 'serve() { jq -c --argjson n "$LIMIT" \'sort_by(-.number)[:$n]\' "$1"; }', 'case "$ARGS" in', ` "pr list"*headRefOid*) cat ${JSON.stringify(join(fix, "match.out"))}; exit 0;;`, - ` "pr list"*--search*) cat ${JSON.stringify(join(fix, "search.json"))}; exit 0;;`, - ` "pr list"*"--state all"*) cat ${JSON.stringify(join(fix, "all.json"))}; exit 0;;`, - ` "pr list"*"--state open"*) cat ${JSON.stringify(join(fix, "open.json"))}; exit 0;;`, + ` "pr list"*"is:unmerged"*) serve ${JSON.stringify(join(fix, "unmerged.json"))}; exit 0;;`, + ` "pr list"*--search*) serve ${JSON.stringify(join(fix, "search.json"))}; exit 0;;`, + ` "pr list"*"--state all"*) serve ${JSON.stringify(join(fix, "all.json"))}; exit 0;;`, + ` "pr list"*"--state open"*) serve ${JSON.stringify(join(fix, "open.json"))}; exit 0;;`, ' "pr edit"*)', ' N="$3"; shift 3; BF=""', ' while [ $# -gt 0 ]; do case "$1" in --body-file) BF="$2"; shift;; esac; shift; done', @@ -1892,39 +1918,49 @@ describe("fix-drift.yml — the LLM freewriter + anti-cheat predicate are GONE", // above), so both the permissions and the stale comment are dead and must be // removed. // --------------------------------------------------------------------------- -// The ONE unpinned executable in the job: `sh <(curl https://ollama.com/install.sh)`. +// Ollama provisioning: the job's third-party BYTES, and where they get executed. // // Every `uses:` here is pinned by commit SHA and `pnpm install --frozen-lockfile` -// is pinned by the lockfile's integrity hashes, but that URL is MUTABLE, and the -// step used to run six steps after an app token with `contents: write` + +// is pinned by the lockfile's integrity hashes, but this step fetches a release +// artifact over the network and unpacks it into /usr/local AS ROOT, and it used +// to run six steps after an app token with `contents: write` + // `pull-requests: write` was minted into the job. Two independent properties are -// asserted, because neither alone closes it: the code runs BEFORE the token -// exists, and the bytes are verified before `sh` sees them (a compromised script -// that cannot read the token can still plant a `git`/`gh`/`node` on PATH for the -// later steps that hold it). +// asserted, because neither alone closes it: the fetch happens BEFORE the token +// exists, and the bytes are verified before anything unpacks them (a payload that +// cannot read the token can still plant a root-owned `git`/`gh`/`node` on PATH +// for the later steps that hold it). +// +// Pinning ollama.com/install.sh was NOT sufficient and this suite used to assert +// the weaker property. That script streams an unversioned, undigested +// `ollama-linux-.tar.zst` through `zstd -d` into `sudo tar -x` — RUN +// VERBATIM out of the pinned script's own bytes on 2026-08-05, attacker-supplied +// content reached `sudo tar -xf - -C ` and the script exited 0. So the +// artifact is fetched directly and digest-checked instead, and the question this +// harness asks is about `tar`, not about `sh`. // --------------------------------------------------------------------------- -describe("fix-drift.yml — the unpinned install script cannot reach the app token", () => { +describe("fix-drift.yml — nothing unpacks bytes it has not pinned, and none of it can reach the app token", () => { const OLLAMA_STEP = "Provision Ollama daemon (drift-sync-check re-collect gate)"; /** * EXECUTE the provisioning step's `run:` body with `curl` serving `served` bytes. * - * Only `curl` and `sh` are stubbed — the verification itself (sha256sum, the - * comparison, the exit) is the workflow's own code, run as written. `sh` records - * that it was reached, which is the question that matters: a step that "fails" - * AFTER handing the file to a shell has not refused anything. + * Only `curl`, `sudo` and `tar` are stubbed — the verification itself + * (sha256sum, the comparison, the exit) is the workflow's own code, run as + * written. `tar` records that it was reached, which is the question that + * matters: a step that "fails" AFTER handing the payload to a root-privileged + * extractor has not refused anything. */ const observeProvision = ( served: string, expectedSha256?: string, - ): { stepExit: number; shRan: boolean; stdio: string } => { + ): { stepExit: number; tarRan: boolean; tarSaw: string; stdio: string } => { const dir = mkdtempSync(join(tmpdir(), "fix-drift-ollama-")); try { const bin = join(dir, "bin"); mkdirSync(bin); const servedFile = join(dir, "served"); writeFileSync(servedFile, served); - const shMarker = join(dir, "sh-ran"); + const tarSawFile = join(dir, "tar-saw"); // -o is the only curl form this step uses for the download; the // readiness poll (`curl -sf http://127.0.0.1:11434/...`) has no -o and must // simply fail so the wait loop falls through. @@ -1939,9 +1975,17 @@ describe("fix-drift.yml — the unpinned install script cannot reach the app tok ].join("\n"), { mode: 0o755 }, ); - writeFileSync(join(bin, "sh"), `#!/bin/sh\ntouch ${JSON.stringify(shMarker)}\n`, { + // `tar` is the root-privileged extractor and the thing that must never see + // unverified bytes: it records WHAT it was handed, so "refused" is the + // absence of a payload rather than an inference from an exit code. + writeFileSync(join(bin, "tar"), `#!/bin/sh\ncat > ${JSON.stringify(tarSawFile)}\n`, { mode: 0o755, }); + // `sudo` must pass through, or a refusal would be indistinguishable from + // "sudo is not installed on the machine running this suite". + writeFileSync(join(bin, "sudo"), '#!/bin/sh\nexec "$@"\n', { mode: 0o755 }); + // `zstd` decompresses; the served bytes stand in for the archive verbatim. + writeFileSync(join(bin, "zstd"), '#!/bin/sh\ncat "${3:--}"\n', { mode: 0o755 }); for (const noop of ["ollama", "sleep"]) writeFileSync(join(bin, noop), "#!/bin/sh\nexit 0\n", { mode: 0o755 }); const step = stepByName(OLLAMA_STEP); @@ -1949,7 +1993,7 @@ describe("fix-drift.yml — the unpinned install script cannot reach the app tok writeFileSync(script, runOf(step)); const env: Record = {}; for (const [k, v] of Object.entries(step.env)) env[k] = v; - if (expectedSha256 !== undefined) env.OLLAMA_INSTALL_SHA256 = expectedSha256; + if (expectedSha256 !== undefined) env.OLLAMA_TARBALL_SHA256 = expectedSha256; const res = spawnSync("/bin/bash", [script], { cwd: dir, encoding: "utf-8", @@ -1957,7 +2001,8 @@ describe("fix-drift.yml — the unpinned install script cannot reach the app tok }); return { stepExit: res.status ?? -1, - shRan: existsSync(shMarker), + tarRan: existsSync(tarSawFile), + tarSaw: existsSync(tarSawFile) ? readFileSync(tarSawFile, "utf-8") : "", stdio: `${res.stdout ?? ""}${res.stderr ?? ""}`, }; } finally { @@ -1965,7 +2010,7 @@ describe("fix-drift.yml — the unpinned install script cannot reach the app tok } }; - it("the install script runs BEFORE the app token is minted, not after", () => { + it("provisioning runs BEFORE the app token is minted, not after", () => { const names = steps().map((s) => s.id ?? s.name ?? ""); const ollamaIdx = names.indexOf(OLLAMA_STEP); const tokenIdx = names.indexOf("app-token"); @@ -1973,7 +2018,7 @@ describe("fix-drift.yml — the unpinned install script cannot reach the app tok expect(tokenIdx, "no app-token step found").toBeGreaterThan(-1); expect( ollamaIdx, - "the unpinned install script executes while an app token with contents:write and " + + "third-party bytes are unpacked as root while an app token with contents:write and " + "pull-requests:write is already live in the job", ).toBeLessThan(tokenIdx); }); @@ -1981,38 +2026,67 @@ describe("fix-drift.yml — the unpinned install script cannot reach the app tok it("the pin is a real sha256 the step actually compares the download against", () => { const step = stepByName(OLLAMA_STEP); expect( - step.env.OLLAMA_INSTALL_SHA256 ?? "", + step.env.OLLAMA_TARBALL_SHA256 ?? "", "the step declares no pinned digest, so there is nothing to verify against", ).toMatch(/^[0-9a-f]{64}$/); // …and the comparison must be against the DOWNLOAD, not against a constant // recomputed from itself. - expect(codeOf(step)).toMatch(/sha256sum "\$SCRIPT"/); + expect(codeOf(step)).toMatch(/sha256sum "\$TARBALL"/); }); - it("TAMPERED bytes are REFUSED before `sh` ever sees the file (EXECUTED)", () => { - const obs = observeProvision("#!/bin/sh\n# attacker-substituted payload\nexit 0\n"); + it("NOTHING in the step is fetched from a mutable, unversioned URL", () => { + // The defect this replaced: a pinned install.sh whose OWN download — + // `ollama.com/download/ollama-linux-.tar.zst`, no version, no digest — + // went into `sudo tar -x`. Pinning the wrapper while it fetches an unpinned + // payload is the failure mode, so every URL the step names must carry the + // version, and the script that does not must not come back. + const code = codeOf(stepByName(OLLAMA_STEP)); + const urls = code.match(/https?:\/\/[^\s"')]+/g) ?? []; + expect(urls.length, "the step fetches nothing at all").toBeGreaterThan(0); + for (const url of urls) { + if (url.startsWith("http://127.0.0.1")) continue; // the local readiness poll + expect( + url, + `\`${url}\` is not pinned to a release version, so its bytes can change under the ` + + "digest that is supposed to describe them", + ).toContain("${OLLAMA_VERSION}"); + } expect( - obs.shRan, - "the workflow handed a script whose bytes do NOT match the pin to `sh` — this is " + - "arbitrary third-party code executing as the runner user, and every later step in " + - "the job holds a write-scoped app token it can reach through PATH", + code, + "ollama.com/install.sh is back — it streams an unversioned, undigested tarball into " + + "`sudo tar -x`, so pinning the script's own bytes leaves a second unpinned payload " + + "planting root-owned binaries on PATH", + ).not.toContain("ollama.com/install.sh"); + }); + + it("TAMPERED bytes are REFUSED before root `tar` ever sees them (EXECUTED)", () => { + const obs = observeProvision("ATTACKER-SUBSTITUTED ARCHIVE\n"); + expect( + obs.tarRan, + "the workflow handed an archive whose bytes do NOT match the pin to a root-privileged " + + "`tar` unpacking into /usr/local — that plants arbitrary root-owned binaries on PATH, " + + "and every later step in the job holds a write-scoped app token they can reach. " + + `tar was handed: ${JSON.stringify(obs.tarSaw)}`, ).toBe(false); expect(obs.stepExit, "the step concluded successfully on a tampered download").not.toBe(0); expect(obs.stdio).toContain("does not match its pinned sha256"); }); - it("POSITIVE CONTROL: bytes that DO match the pin are executed (the gate is not just 'always refuse')", () => { + it("POSITIVE CONTROL: bytes that DO match the pin are unpacked (the gate is not just 'always refuse')", () => { // The digest is computed from the served bytes here rather than shipping a - // 15KB copy of the real script: the property under test is that a MATCH + // 1.4GB copy of the real archive: the property under test is that a MATCH // proceeds, and without this the refusal above is satisfied by a step that - // never runs anything at all. - const good = "#!/bin/sh\n# the reviewed upstream script\nexit 0\n"; + // never unpacks anything at all. + const good = "the reviewed upstream archive\n"; const sha = createHash("sha256").update(good).digest("hex"); const obs = observeProvision(good, sha); expect( - obs.shRan, + obs.tarRan, "a download matching its pin was refused, so provisioning can never run", ).toBe(true); + expect(obs.tarSaw, "tar was reached but handed something other than the verified bytes").toBe( + good, + ); expect(obs.stepExit, obs.stdio).toBe(0); }); }); @@ -2879,7 +2953,19 @@ describe("fix-drift.yml — the needs-human alert always names a PR", () => { `${step}: \`${call}\` has no --limit. gh pr list defaults to 30, so once 30 ` + "newer PRs exist an already-proposed PR falls out of the window, the dedup " + "misses it, and the workflow opens a duplicate", - ).toMatch(/--limit \d+/); + ).toMatch(/--limit (?:\d+|"\$\{PR_LIST_LIMIT\}")/); + } + // A limit spelled as a VARIABLE has to resolve to a number in the step that + // uses it. `set -u` would catch an unset one loudly, but a limit bound to + // something non-numeric would not: `--limit ''` is how gh's default of 30 + // comes back through a spelling that still reads as explicit here. + for (const st of steps()) { + const code = codeOf(st); + if (!code.includes('--limit "${PR_LIST_LIMIT}"')) continue; + expect( + code, + `${st.name}: uses --limit "\${PR_LIST_LIMIT}" but never binds PR_LIST_LIMIT to a number`, + ).toMatch(/^\s*PR_LIST_LIMIT=\d+\s*$/m); } }); }); @@ -3411,8 +3497,17 @@ describe("fix-drift.yml — dedup survives a human body edit and a CLOSED PR", ( '--search "${CHANGESET_KEY} in:body"', ); expect(listing, `${st.name}: the dedup listing has no explicit --limit`).toContain( - "--limit 200", + '--limit "${PR_LIST_LIMIT}"', ); + // …and the window it gets is AUDITED. A listing that comes back full is + // truncated, and a truncated dedup listing answers "not proposed" / "not + // rejected" for a PR that exists. Without this the audit can be deleted + // from one of the two steps and nothing anywhere reds. + expect( + code, + `${st.name}: the dedup listing is never checked for saturation, so a full ` + + "window is read as a complete answer", + ).toContain('assert_listing_complete "changeset-keyed dedup listing'); // `state` must be REQUESTED, or `.state` is null on every entry and both the // CLOSED and the OPEN selectors are dead for every PR at once. expect( @@ -5053,6 +5148,140 @@ describe("fix-drift.yml — the PR-open steps decide correctly when RUN, not whe }); } + // ------------------------------------------------------------------------- + // The rejection lookup must not be crowded out of its own `--limit` window. + // + // The self-heal listing above is what makes a rejection survive a body edit, + // and it was a PLAIN `--state all --limit 200` — a client-side state filter + // over a server-side window that EVERY PR in the repo competes for. Measured + // on CopilotKit/aimock on 2026-08-05: `--state all --limit 200` came back with + // exactly 200 PRs, 184 of them MERGED and discarded by the jq, and the window + // reached only as far back as #125. The repo has 26 unmerged-closed PRs; that + // listing could see 14. Twelve closed PRs were ALREADY invisible to it, with + // no error, no warning and no truncation flag — and MERGED is the population + // that grows every day. The rejection lookup does not degrade when the window + // fills, it INVERTS: "no closed PR carries this key" is what the step reads, + // and re-proposing the rejected changeset every morning is what it then does. + // + // `--state closed` is NOT the remedy and this was measured too: gh maps it to + // CLOSED-or-MERGED, and the same query returned 186 merged against 14 closed. + // ------------------------------------------------------------------------- + it("a rejection is still found once MERGED PRs have filled the listing window", () => { + // The exact scenario above — a human closed #77 to reject this changeset, + // then rewrote its body and the marker went with it — except the repo has + // since merged 200 newer PRs. Nothing about the rejection changed; only the + // number of merged PRs sitting in front of it did. + const closed: PrFixture = { + number: 77, + url: "https://x/pr/77", + state: "CLOSED", + headRefName: `fix/drift-2026-07-01-8888-${KEY}`, + body: "Rejecting this — we are keeping the family.\n", + files: [{ path: NOTE }], + author: BOT, + }; + const merged: PrFixture[] = Array.from({ length: 200 }, (_, i) => ({ + number: 1000 + i, + url: `https://x/pr/${1000 + i}`, + state: "MERGED" as const, + headRefName: `renovate/dep-${i}`, + body: "unrelated\n", + files: [], + author: { login: "renovate[bot]" }, + })); + const r = observePrStep("pr", { + changesetKey: KEY, + committed: { [REGISTRY]: "// edited\n" }, + prs: [closed, ...merged], + matchOut: '{"url":"https://x/pr/56","number":56}', + }); + expect( + r.created, + "200 merged PRs pushed the CLOSED rejection out of the dedup listing's window, so the " + + "step could not see that a human had already rejected this exact changeset and " + + "re-proposed it — which it then does every morning, silently, for ever. The window " + + `is spent on MERGED PRs no guard here even consults. stdio: ${r.stdio}`, + ).toBe(false); + expect(r.outputs.rejected, "the run published no rejection at all").toBe("77"); + expect(r.edits["77"] ?? "", "the rejection marker was never repaired").toContain( + ``, + ); + expect(r.stepExit, r.stdio).toBe(0); + }); + + it("needs_human_pr: a rejection survives a full window there too", () => { + // The needs-human step carries its own copy of the listing, so a fix applied + // to only one of the two leaves the other saturating exactly as before. + const merged: PrFixture[] = Array.from({ length: 200 }, (_, i) => ({ + number: 1000 + i, + url: `https://x/pr/${1000 + i}`, + state: "MERGED" as const, + headRefName: `renovate/dep-${i}`, + body: "unrelated\n", + files: [], + author: { login: "renovate[bot]" }, + })); + const r = observePrStep("needs_human_pr", { + changesetKey: KEY, + committed: { [NOTE]: "note\n" }, + prs: [ + { + number: 77, + url: "https://x/pr/77", + state: "CLOSED", + headRefName: `drift-needs-human/2026-07-01-8888-${KEY}`, + body: "Rejecting this — we are keeping the family.\n", + files: [{ path: NOTE }], + author: BOT, + }, + ...merged, + ], + matchOut: '{"url":"https://x/pr/57","number":57}', + }); + expect( + r.created, + "the needs-human step re-proposed a changeset a human had rejected, because merged " + + `PRs had filled its dedup listing's window. stdio: ${r.stdio}`, + ).toBe(false); + expect(r.outputs.rejected, "the run published no rejection at all").toBe("77"); + expect(r.stepExit, r.stdio).toBe(0); + }); + + it("a listing that comes back FULL is REFUSED, not decided on", () => { + // The narrowing above buys headroom; it does not make the window infinite. + // So the remaining case — the unmerged population itself reaching the limit — + // must be LOUD. A truncated listing has the same shape and the same exit code + // as a complete one, so "found nothing" and "could not look" would otherwise + // share an encoding, which is the defect this whole guard exists to remove. + // 200 unmerged PRs, none of them this changeset's: the honest answer is "I + // cannot tell", and the step must not open a PR on it. + const unmerged: PrFixture[] = Array.from({ length: 200 }, (_, i) => ({ + number: 2000 + i, + url: `https://x/pr/${2000 + i}`, + state: (i % 2 === 0 ? "OPEN" : "CLOSED") as "OPEN" | "CLOSED", + headRefName: `feature/x-${i}`, + body: "unrelated\n", + files: [], + author: { login: "someone" }, + })); + for (const [id, committed] of [ + ["pr", { [REGISTRY]: "// edited\n" }], + ["needs_human_pr", { [NOTE]: "note\n" }], + ] as const) { + const r = observePrStep(id, { changesetKey: KEY, committed, prs: unmerged }); + expect( + r.stepExit, + `${id}: the listing came back full — and therefore truncated — and the step decided ` + + `on it anyway instead of failing. stdio: ${r.stdio}`, + ).not.toBe(0); + expect(r.stdio).toContain("came back FULL at its --limit"); + expect( + r.created, + `${id}: a PR was opened off a listing that could not be proven complete`, + ).toBe(false); + } + }); + it("needs_human_pr: the widened self-heal does NOT touch a MERGED PR either", () => { // The needs-human step admits CLOSED through its own `select_state`, and that // predicate is the only thing keeping MERGED out. Without this, relaxing it to