From b2e189dbb2d8aaa2481cff18effd0638f87a0ad6 Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Wed, 5 Aug 2026 19:54:55 -0700 Subject: [PATCH 1/2] fix(drift): merged PRs no longer crowd a human rejection out of the dedup window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gh pr list --limit N` is a window, not a ceiling: it returns the newest N rows and drops the rest with no flag, no warning and no count. The self-heal listing that makes a rejection survive a body edit was a plain `--state all --limit 200` whose state filter is applied client-side, so every PR in the repo competed for those 200 slots — and the merged ones, which no guard here consults, won. Measured on this repo on 2026-08-05: that listing came back FULL, 184 of the 200 MERGED, reaching back only to #125. The repo had 26 unmerged-closed PRs and the listing could see 14; twelve were ALREADY outside the window. A closed drift PR aging out the same way does not make the 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, silently. Both self-heal listings now additionally query `--search "is:unmerged"`, a state predicate that excludes merged PRs server-side (28 rows, back to #1, on the same measurement) and — unlike ` in:body` — cannot be defeated by the body edit the self-heal exists to repair. The plain listing is unioned, not replaced, so the index-free view stays. `--state closed` is not a substitute: gh maps it to CLOSED-or-MERGED and returned 186 merged against 14 closed. Headroom is not a proof, so every listing whose completeness a guard relies on is now audited: one that comes back full is refused with an `::error::` rather than decided on, because a truncated listing and a complete one are otherwise indistinguishable. Red-green: the `gh` stub now models `--limit` truncation the way real gh does (newest-first, silent). Against the unfixed workflow both PR-open steps opened a brand-new PR for a changeset a human had rejected; they now find it. Mutations confirmed: neutering the union reds 2, deleting the audit calls reds 2, raising the audit threshold out of reach reds 1. --- .github/workflows/fix-drift.yml | 183 +++++++++++++++++---- src/__tests__/fix-drift-workflow.test.ts | 197 ++++++++++++++++++++++- 2 files changed, 343 insertions(+), 37 deletions(-) diff --git a/.github/workflows/fix-drift.yml b/.github/workflows/fix-drift.yml index 15dfe4c1..4bb113f3 100644 --- a/.github/workflows/fix-drift.yml +++ b/.github/workflows/fix-drift.yml @@ -491,9 +491,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 +516,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 +571,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 +642,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 +655,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 +926,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 +947,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 +988,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 +1225,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 +1238,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 +1486,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..57333805 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', @@ -2879,7 +2905,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 +3449,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 +5100,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 From 6e5b629aaf81e87ae89e3b39531b3ff82f908b3d Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Wed, 5 Aug 2026 20:01:03 -0700 Subject: [PATCH 2/2] fix(drift): pin the Ollama artifact that actually gets unpacked as root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit's claim that install.sh was "the one unpinned executable in the job" was false, and this makes the code and the comments true. Pinning install.sh's own bytes stopped nothing important: the script then streams `ollama.com/download/ollama-linux-.tar.zst` — mutable, unversioned, no digest — through `zstd -d` into `sudo tar -x` under /usr/local. Running that function verbatim out of the pinned script's own bytes, with curl serving attacker-supplied content, the payload reached `sudo tar -xf - -C ` and the script exited 0. A compromised tarball plants a root-owned `git`/`gh`/`node` on PATH exactly as well as a compromised script would, and every later step in this job holds a contents:write + pull-requests:write app token those binaries can reach. It cannot be fixed in place, either: install.sh pipes the download straight into tar, so it never holds the file and has nothing to verify. So install.sh is not used. The release artifact is fetched directly from an immutable release tag and its sha256 is checked before anything unpacks it. The tarball is the whole product (bin/ollama + lib/ollama/*) and this step already ran `ollama serve` itself rather than using install.sh's systemd unit, so nothing else in that script was load-bearing — and dropping it also drops its NVIDIA CUDA repo adds and `$PACKAGE_MANAGER -y install`. The digest was agreed by three independent sources: the v0.32.6 release's sha256sum.txt, the GitHub release API's asset `digest` field, and sha256 of the 1420686963 downloaded bytes. Archive layout confirmed by listing it. Red-green: the guard now watches `tar`, not `sh` — tampered bytes must not reach a root-privileged extractor, and the positive control asserts tar was handed the verified bytes. Mutations confirmed: neutering the digest comparison reds the refusal, and reintroducing the install.sh URL reds the unversioned-URL guard. The false claim is corrected in the workflow comment, the suite's header comment, its describe name and its failure messages. The commit message that first made it is in history and cannot be rewritten. --- .github/workflows/fix-drift.yml | 76 ++++++++++----- src/__tests__/fix-drift-workflow.test.ts | 112 ++++++++++++++++------- 2 files changed, 134 insertions(+), 54 deletions(-) diff --git a/.github/workflows/fix-drift.yml b/.github/workflows/fix-drift.yml index 4bb113f3..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 + 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 - sh "$SCRIPT" + # 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 diff --git a/src/__tests__/fix-drift-workflow.test.ts b/src/__tests__/fix-drift-workflow.test.ts index 57333805..fddd387e 100644 --- a/src/__tests__/fix-drift-workflow.test.ts +++ b/src/__tests__/fix-drift-workflow.test.ts @@ -1918,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. @@ -1965,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); @@ -1975,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", @@ -1983,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 { @@ -1991,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"); @@ -1999,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); }); @@ -2007,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("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( + 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 `sh` ever sees the file (EXECUTED)", () => { - const obs = observeProvision("#!/bin/sh\n# attacker-substituted payload\nexit 0\n"); + it("TAMPERED bytes are REFUSED before root `tar` ever sees them (EXECUTED)", () => { + const obs = observeProvision("ATTACKER-SUBSTITUTED ARCHIVE\n"); 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", + 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); }); });