fix(drift): the rejection lookup no longer saturates, and the Ollama artifact is pinned - #359
Merged
Conversation
…edup window `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 `<key> 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.
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-<arch>.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 <dest>` 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.
commit: |
jpr5
marked this pull request as ready for review
August 6, 2026 03:07
jpr5
added a commit
that referenced
this pull request
Aug 6, 2026
## What this closes `test-drift.yml`'s Ollama provisioning step fetched `https://ollama.com/install.sh` and ran it with **no digest check of any kind** — not even the weaker script-only pin `fix-drift.yml` carries today. The step holds no provider key of its own, and that protects nothing. It runs before **`Run drift tests`**, which is handed `OPENAI_API_KEY`, `ANTHROPIC_API_KEY`, `GOOGLE_API_KEY`, `OPENROUTER_API_KEY`, `FAL_KEY`, `COHERE_API_KEY` and `ELEVENLABS_API_KEY`. A payload that cannot read those keys can still plant a `node`/`npx`/`git` earlier on PATH, and the later key-holding step executes it. (The workflow's other 15 `secrets.` references live in the `notify` and `drift-live-pr` jobs — separate runners, not reachable from this step. Saying so rather than quoting "22 secrets".) ## Watched red-green-rebreak **RED**, the committed step's own `run:` body executed verbatim with `curl` serving substituted bytes: ``` STEP_EXIT=0 SH_RAN argv=…/ollama-install.sh what sh was handed: ATTACKER-CONTROLLED PAYLOAD ``` Arbitrary third-party code executed and the step reported success. **GREEN**, same harness after the fix: ``` STEP_EXIT=1 ::error::the Ollama v0.32.6 tarball does not match its pinned sha256 — REFUSING to unpack it as root. ``` No `SH_RAN`, no `TAR_RAN`. The new guard asserts the *absence of a payload at an executor*, not an exit code — and its positive control asserts `tar` was handed exactly the verified bytes, so "always refuse" cannot satisfy it. **RE-BREAK**, three mutations, each restored (workflow md5 identical before and after all three): neutering the digest comparison, pointing the fetch back at `install.sh`, and emptying the pin each red exactly their intended guard. ## The remedy `install.sh` is not used. Pinning it would not have been sufficient — it streams an unversioned, undigested `ollama-linux-<arch>.tar.zst` into `sudo tar -x` and cannot be fixed in place because it never holds the file. So the release artifact is fetched from an immutable release tag and sha256-verified before anything unpacks it. Same shape as #359 does for `fix-drift.yml`; that file is untouched here. Digest `dec2fa50…aadcfc` (v0.32.6, 1,420,686,963 bytes) agreed by **three independent sources verified in this PR**: the release's own `sha256sum.txt`, the GitHub release API's asset `digest` field, and sha256 of the downloaded bytes. Archive layout confirmed by listing it — 54 entries, `bin/ollama` + `lib/ollama/*`. ## This is NOT the last unpinned executable in the repo Swept all 15 workflows. Still unpinned, **not fixed here**, ranked by blast radius: 1. `publish-release.yml:189` — `pip install hatch`, unpinned PyPI package, in the job with `environment: pypi` + `id-token: write` that publishes `aimock-pytest` to PyPI via OIDC. 2. `fix-drift.yml:90` — the same Ollama defect in its weaker form; #359's. 3. `publish-commit.yml:20` — `npx pkg-pr-new publish`; `pkg-pr-new` is in neither `package.json` nor `pnpm-lock.yaml`, so npx fetches `latest` and runs it. No repo secrets in that job. 4. `test-pytest.yml:48` — `pip install ./packages/aimock-pytest[test]` resolves unpinned transitive deps. No secrets in that job. 5. Toolchain: `actions/setup-node`'s Node download and `pnpm/action-setup`'s pnpm (`packageManager: pnpm@10.28.2` — a version, no integrity hash). Applies to every job including this one. Not a finding: the `git clone` of ag-ui is read by static regex parsing, never executed. Every `npx tsx`/`npx vitest` resolves from the lockfile-installed tree. Every `uses:` is SHA-pinned. ## Gates `pnpm build` 0 · `pnpm test:drift` 0 · `tsc --noEmit` 0 · `pnpm lint` 0 · `pnpm test:exports` 0 · prettier clean · zizmor (medium) 0 findings · `bash -n` on 9 `run:` bodies, 0 failures · commitlint 0. `pnpm test`: 5061 passed / 1 failed — `ws-framing`, a known pre-existing flake; passes in isolation (24/24, exit 0). Suite 5057 → 5062. `actionlint` exits 1 on one **pre-existing** SC2086 *info* in the `notify` job, identical on `origin/main` (base L261 = head L329, exactly this diff's offset).
pull Bot
pushed a commit
to TheTechOddBug/aimock
that referenced
this pull request
Aug 6, 2026
test-drift.yml's provisioning step fetched https://ollama.com/install.sh and ran it with no digest check of any kind. Executing that step's own run: body verbatim with curl serving substituted content, `sh` was handed "ATTACKER-CONTROLLED PAYLOAD" and the step exited 0. The step holds no provider key itself, and that protects nothing. It runs before `Run drift tests`, which is handed OPENAI_API_KEY, ANTHROPIC_API_KEY, GOOGLE_API_KEY, OPENROUTER_API_KEY, FAL_KEY, COHERE_API_KEY and ELEVENLABS_API_KEY. A payload that cannot read those keys can still plant a node/npx/git earlier on PATH, and the later key-holding steps execute it. Pinning install.sh would not have been enough — proven in fix-drift.yml's own writeup: the script streams an unversioned, undigested ollama-linux-<arch>.tar.zst through zstd -d into sudo tar -x, and it cannot be fixed in place because it never holds the file. So install.sh is not used. The release artifact is fetched from an immutable release tag and its sha256 checked before anything unpacks it, mirroring the shape fix-drift.yml is getting in CopilotKit#359. Digest dec2fa50…aadcfc for ollama-linux-amd64.tar.zst v0.32.6 (1420686963 bytes), agreed by three independent sources verified here: the release's own sha256sum.txt, the GitHub release API's asset digest field, and sha256 of the downloaded bytes. Archive layout confirmed by listing it — 54 entries, bin/ollama plus lib/ollama/*, so -C /usr/local puts the binary on the default PATH. This is NOT the last unpinned executable in the repo, and the new guard does not claim otherwise. Still unpinned and not fixed here: `npx pkg-pr-new publish` (publish-commit.yml) fetches an unlockfiled npm package and runs it; `pip install hatch` (publish-release.yml) runs an unpinned PyPI package in the job that holds PyPI OIDC publish rights; `pip install ./packages/aimock-pytest[test]` (test-pytest.yml) resolves unpinned transitive deps; and in this job the setup-node and pnpm/action-setup toolchain downloads carry no committed digest. fix-drift.yml:90 has the same Ollama defect in its weaker form and is CopilotKit#359's. New guard executes the step rather than reading it: tampered bytes must not reach `sh` or `tar`, and a positive control asserts tar was handed exactly the verified bytes. Three mutations confirmed red — neutering the digest comparison, pointing the fetch back at install.sh, and emptying the pin.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two follow-ups on
mainfrom the adversarial review of #357.1. The rejection/dedup query is already saturated — silently
gh pr list --limit Nis a window, not a ceiling: it returns the newest N rows and drops the rest with no flag, no warning and no count. The marker self-heal — the thing that makes a human's rejection survive a body edit — was a plain--state all --limit 200whose state filter is applied client-side, so every PR in the repo competed for those 200 slots and the merged ones won.Measured live on this repo, 2026-08-05:
Two things the review did not have:
--state closedis not a fix. gh maps it to CLOSED-or-MERGED — 186 merged against 14 closed, just as full.#1, 30, 50, 56, 59, 61, 63, 94, 95, 96, 119, 120, each confirmed CLOSED bygh pr view. None is a drift PR, so no rejection is lost today — but the mechanism is running now, and MERGED is the population that grows daily.Fix. Both self-heal listings additionally query
--search "is:unmerged"and union the two views. That excludes merged PRs server-side: 200/200 slots consumed becomes 28/200, and 28 is the complete unmerged population back to #1.is:unmergedis a STATE predicate, so unlike<key> in:bodyit cannot be defeated by the very body edit the self-heal repairs, and a just-closed PR matches under either indexed state. The plain listing is unioned rather than replaced, so the index-free view stays and plain entries win on collision.Headroom is not a proof, so saturation is also made loud:
assert_listing_completerefuses any listing that comes back full at its--limitrather than deciding on it.--limit 200also stopped being a magic number — each step bindsPR_LIST_LIMITonce and the audit reads the same variable.2. "the one unpinned executable in the job" was false
89e5a07pinnedollama.com/install.sh. That script then streamsollama.com/download/ollama-linux-<arch>.tar.zst— mutable, unversioned, no digest — throughzstd -dintosudo tar -xunder/usr/local.Run verbatim out of the pinned script's own bytes, with curl serving attacker content:
A compromised tarball plants a root-owned
git/gh/nodeon PATH exactly as well as a compromised script would, and every later step holds acontents:write+pull-requests:writeapp token those binaries can reach.It cannot be fixed in place — install.sh pipes the download straight into
tar, so it never holds the file and has nothing to verify;OLLAMA_VERSIONonly appends?version=, a version pin rather than a byte pin.Fix. install.sh is not used. The release artifact is fetched from an immutable release tag and sha256-checked before anything unpacks it. Digest agreed by three independent sources: the v0.32.6 release's
sha256sum.txt, the GitHub release API's assetdigestfield, and sha256 of the 1,420,686,963 downloaded bytes. Archive layout confirmed by listing it (bin/ollama+lib/ollama/*, so-C /usr/localputs the binary on PATH). Dropping install.sh also drops its NVIDIA CUDA repo adds and$PACKAGE_MANAGER -y install; the step already ranollama serveitself rather than using its systemd unit.The false claim is corrected in the workflow comment, the suite's header comment, its describe name and its failure messages. Two further stale claims about the self-heal listing
--state open(false since #357) are corrected too. The commit message that first asserted it is in history and cannot be rewritten.Red-green
The
ghstub could not previously express item 1's bug — it ignored--limitand returned the whole population, so a saturated window looked exactly like a healthy one. It now models what real gh does.RED (watched). The existing rejection scenario plus 200 newer MERGED PRs — both PR-open steps re-proposed the rejected changeset:
GREEN. 154 passed, exit 0.
RE-BREAK (watched), each mutation restored and md5-verified:
assert_listing_completecallsif falseollama.com/install.shItem 2's guard now watches
tar, notsh, and the positive control assertstarwas handed exactly the verified bytes rather than merely that it ran.Gates
pnpm build0 ·pnpm test0 (5048 passed / 171 files) ·pnpm test:drift0 ·tsc --noEmit0 ·pnpm lint0 ·pnpm test:exports0 · prettier clean ·actionlint0 ·zizmor0 findings ·bash -non all 16run:bodies 0 · commitlint 0.Not fixed here
.github/workflows/test-drift.yml:177does the samesh install.shwith no pin at all, in a job holding five provider API keys, with provisioning ahead of the steps that use them. Same defect, different file — flagged, not touched.