fix(drift): the install script cannot reach the app token, and a per-note overlap no longer discards the run - #357
Merged
Merged
Conversation
…4 builtin `mapfile` was the only bash-4-or-newer construct anywhere in fix-drift.yml. The runner has bash 5 so it worked in production — but it also made this one step's body the one body a test could not EXECUTE on a stock macOS dev box (bash 3.2), and a guard that cannot be run gets hand-mirrored in TypeScript instead. That mirror is what produced the bulk of this PR's review findings. Replaced with a portable `read` loop that folds the drift-proposals/* filter in, so the intermediate array and its loop are gone rather than rewritten. Proven equivalent under bash 5.2 across six input shapes (empty, one note, mixed registry-edit-plus-notes, a final line with NO trailing newline, no notes at all, and a path containing a space); `|| [ -n "$f" ]` is what covers the unterminated final line where a naive read loop drops it and mapfile does not. Two tests keyed on the literal `mapfile -t COMMITTED` went red on that behaviour-preserving swap — they were measuring an implementation choice, not the fact they exist for. Re-anchored on the `git diff --name-only` invocation that actually produces the note list.
…h the app token live `sh <(curl https://ollama.com/install.sh)` is arbitrary third-party code from a MUTABLE URL, executing as the runner user, and it sat six steps after an app token with contents:write + pull-requests:write was minted into the job. Everything else in the job's third-party surface is pinned — every `uses:` by commit SHA, and `pnpm install --frozen-lockfile` by the lockfile's integrity hashes. This was the exception. Two changes, because neither alone closes it: - the step now runs BEFORE the mint, so the token does not exist while that code runs and cannot be read out of the job's state. It also starts the token's one-hour life after the slowest setup step rather than before it. - the downloaded bytes are verified against a sha256 reviewed at the URL, and a mismatch is a hard exit BEFORE `sh` sees the file. Reordering alone leaves the real attack: a compromised script can plant a `git`, `gh` or `node` earlier on PATH, and every later step — the ones that hold the token — would run it. When Ollama publishes a new install.sh this step fails, loudly and alerted (the end-of-job catch-all covers the infra/setup window). The error prints the digest it got and names the file to re-pin. An unverified script must not run just because verifying it was inconvenient. Red-green: pre-fix the step is at index 7 against the token's index 1, declares no digest, and handed an attacker-substituted payload straight to `sh` (`shRan=true`, step exit 0). Post-fix the same payload is refused with `::error::…does not match its pinned sha256`, `sh` is never reached, and the step exits 1 — while a download whose bytes DO match still executes, so the gate is not just "always refuse". Four mutations bound: inverting the comparison reds both the refusal and the positive control; dropping the `exit 1` reds the refusal; comparing the pin to itself instead of to the download reds two; moving the step back after the mint reds the ordering guard.
…e rest of the run
The needs-human step's per-note guard `exit 0`'d the whole run on the FIRST note
some open PR already proposed. On a mixed run that lost work: drift-sync applies a
mechanical registry edit AND defers a different family to a human, and if the
deferred note is already proposed by a PR from an earlier run — whose outcome set
was different, so it hashes to a different changeset key and the primary guard
legitimately does not fire — the registry edit was committed locally, never pushed,
proposed to nobody, and re-detected and re-abandoned identically every morning. A
note set that merely GREW since the last run ({B} then {B,C}) lost C the same way.
The stand-down can never be right, which is why it is removed rather than narrowed:
control only reaches that loop after the primary changeset guard has declined, so by
construction no open PR carries this run's changeset and the run's content is
un-proposed. The overlap is still REPORTED — two open PRs carrying one note looks
exactly like the duplicate-PR spam this workflow exists to prevent, and the human
reading the second one deserves to know why it exists.
Also adds the harness the red-green needed: `observePrStep` EXECUTES either
PR-open step's own several-hundred-line body against a real git repository with a
real base commit and diff, the real jq, and a scripted `gh`. Only `gh` and
`git push` are stubbed. Hand-mirroring these guards in TypeScript is what produced
most of this PR's review findings, so "the step stood down" is now the observable
ABSENCE of a `pr create` call. A CANARY pins that the harness can make both bodies
push and open a PR, so the stand-down assertions cannot be satisfied by a harness
that runs nothing.
Red-green, EXECUTED: pre-fix the mixed run printed "note … already proposed in open
PR #42 — not opening a duplicate" and exited 0 with created=false, pushed=false;
post-fix it warns about the overlap, pushes, and opens exactly one PR. Two mutations
bound: restoring the run-wide `exit 0` reds three tests, and silencing the warning
reds one. The negative control — a re-fire of the SAME changeset — still opens no
second PR, so the dedup that matters is intact.
…ecords it
Closing a drift PR rejects its changeset, and the record of that decision is the
`<!-- drift-changeset: <key> -->` marker in the CLOSED PR's body. Two things then
conspired. The dedup listing finds closed PRs with `--search "<key> in:body"`,
which cannot match a body the key was deleted from. And the marker self-heal that
exists precisely to repair deleted markers listed `--state open`, so it never saw
the closed PR either. So a human editing a closed PR's body — a normal thing to do
to a PR you have just declined, e.g. writing down why — destroyed the only record
of the rejection PERMANENTLY, and the bot re-proposed the identical changeset the
next morning and every morning after. The one marker whose loss cannot be repaired
by any other path was the one marker the repair could not reach.
The head branch ends in "-<changeset key>" and a PR's head branch cannot be
renamed, so the closed PR is still identifiable body-independently. Both self-heals
now list `--state all`; candidacy is UNCHANGED and every candidate still has to
carry THIS run's key in its branch, which is what keeps a human's
`fix/drift-<slug>` branch out. Two narrowings keep the wider population safe, and
each is guarded by a test that reds without it:
- MERGED reaches neither self-heal. A merged PR is an ACCEPTED decision, never a
pending proposal and never a rejection, so no guard consults it and writing
machine markers into it would be a write with no reader.
- on the needs-human step, CLOSED is admitted through the branch-key anchor ONLY
(`select_state`). The note-path anchor is the one that runs the fail-closed
changed-file audit, and letting the accumulated closed backlog into that audit
would turn a single 100-file closed PR into a red cron every morning.
Red-green, EXECUTED on both steps: pre-fix each pushed a branch and opened a PR for
a changeset a human had rejected, publishing no `rejected` output at all; post-fix
each repairs the marker on the closed PR — appended to the human's own prose, which
is asserted, not assumed — publishes `rejected=77`, and opens nothing. Eight
mutations bound: reverting either listing to `--state open` reds the executed
rejection guard plus the listing assertion; dropping the ok-applied MERGED
exclusion, relaxing `select_state` to `true`, admitting CLOSED through the
note-path anchor, dropping OPEN from it, and removing it from either the candidate
selection or the truncation audit each red at least one test. The first attempt at
this had NO guard on the needs-human narrowing — two mutations passed — so the
MERGED and closed-backlog cases were added until every branch could fail.
…heal now undoes Two claims went stale with the two fixes on this branch. DRIFT.md offered deleting the `<!-- drift-changeset: … -->` marker from a closed PR's body as a way to un-suppress a rejected changeset. The marker self-heal now covers closed PRs and puts it back, so that route is gone — deliberately: a missing marker is far more often a human rewriting the body to record why they declined than an un-suppression, and reading it as the latter resurrected the rejected changeset every morning, permanently. Reopening the PR is the deliberate act and remains the documented one. Deleting the separate `drift-suppression-reported` ack marker still re-posts the notice; that marker is not in the repaired set. And the per-note `drift-proposal-note:` marker is no longer described as a secondary dedup guard, because it no longer decides anything — it reports an overlap and the run proceeds.
…hem, not to a stub Delivery is already proven POSITIVELY on this branch — each alert publishes `posted=true` only after its curl returns 0, and the end-of-job catch-all stands down on that output rather than on an exit code. What was never exercised is the POST itself: every guard stubs curl with `exit N`, so the flags, the method, the header and the JSON body have only ever been read, never run. "Slack would accept this" was an inference from the value of `$MSG`. So `SLACK_WEBHOOK` now points at a local HTTP server and the REAL curl runs. For each of the six Slack-posting steps the request that arrives is asserted to be one POST, `application/json`, a body that parses to an object with a non-empty `text`, and — measured on the wire rather than on `$MSG` — a real newline with no literal backslash-n, which is the form Slack renders as two characters in the channel. The `posted=true` receipt is then checked against a POST that genuinely happened. The first version used `spawnSync`, which blocks the event loop serving the sink: curl connected, nothing ever answered, and all six steps died on `--max-time 15` reporting "sent no request". That is also the vacuity check — the guard fails when nothing arrives. Four mutations bound: `-X PUT` for `-X POST`, dropping the Content-Type header, `-d "$MSG"` for `-d "$PAYLOAD"`, and turning one step's `NL=$'\n'` into the literal two characters each red. STILL UNPROVEN, and only a real webhook can close it: that Slack RENDERS these messages as intended. Nothing in this repo has ever reached Slack. What is closed is everything between the `MSG=` assignment and the socket.
commit: |
jpr5
marked this pull request as ready for review
August 6, 2026 02:39
jpr5
added a commit
that referenced
this pull request
Aug 6, 2026
…artifact is pinned (#359) Two follow-ups on `main` from the adversarial review of #357. ## 1. The rejection/dedup query is already saturated — silently `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 marker self-heal — the thing that makes a human's 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 won. Measured live on this repo, 2026-08-05: ``` gh pr list --state all --limit 200 -> 200 [MERGED=184 CLOSED=14 OPEN=2] #125..#358 gh pr list --state closed --limit 200 -> 200 [MERGED=186 CLOSED=14] gh pr list --state all --limit 200 --search "is:unmerged" -> 28 [CLOSED=26 OPEN=2] #1..#358 ``` Two things the review did not have: - **`--state closed` is not a fix.** gh maps it to CLOSED-**or**-MERGED — 186 merged against 14 closed, just as full. - **Twelve closed PRs are already invisible.** The repo has 26 unmerged-closed PRs; the current listing sees 14. The lost set is `#1, 30, 50, 56, 59, 61, 63, 94, 95, 96, 119, 120`, each confirmed CLOSED by `gh 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:unmerged` is a STATE predicate, so unlike `<key> in:body` it 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_complete` refuses any listing that comes back full at its `--limit` rather than deciding on it. `--limit 200` also stopped being a magic number — each step binds `PR_LIST_LIMIT` once and the audit reads the same variable. ## 2. "the one unpinned executable in the job" was false `89e5a07` pinned `ollama.com/install.sh`. That 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`. Run verbatim out of the pinned script's own bytes, with curl serving attacker content: ``` STEP_EXIT=0 SUDO_RAN argv=tar -xf - -C ./dest TAR_RAN argv=-xf - -C ./dest what tar was handed: ATTACKER-CONTROLLED TARBALL BYTES ``` A compromised tarball plants a root-owned `git`/`gh`/`node` on PATH exactly as well as a compromised script would, and every later step holds a `contents:write` + `pull-requests:write` app 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_VERSION` only 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 asset `digest` field, and sha256 of the 1,420,686,963 downloaded bytes. Archive layout confirmed by listing it (`bin/ollama` + `lib/ollama/*`, so `-C /usr/local` puts the binary on PATH). Dropping install.sh also drops its NVIDIA CUDA repo adds and `$PACKAGE_MANAGER -y install`; the step already ran `ollama serve` itself 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 `gh` stub could not previously express item 1's bug — it ignored `--limit` and 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: ``` FAIL a rejection is still found once MERGED PRs have filled the listing window stdio: Pushed branch main-1111222233334444 … Matched PR #56 FAIL needs_human_pr: a rejection survives a full window there too stdio: Pushed needs-human branch … Opened needs-human PR #57 ``` **GREEN.** 154 passed, exit 0. **RE-BREAK (watched), each mutation restored and md5-verified:** | mutation | tests red | |---|---| | neuter the union so unmerged rows are dropped | 2 | | delete all four `assert_listing_complete` calls | 2 | | raise the audit threshold out of reach | 1 | | replace the tarball digest comparison with `if false` | 1 | | point the fetch back at `ollama.com/install.sh` | 1 | Item 2's guard now watches `tar`, not `sh`, and the positive control asserts `tar` was handed exactly the verified bytes rather than merely that it ran. ## Gates `pnpm build` 0 · `pnpm test` 0 (**5048 passed / 171 files**) · `pnpm test:drift` 0 · `tsc --noEmit` 0 · `pnpm lint` 0 · `pnpm test:exports` 0 · prettier clean · `actionlint` 0 · `zizmor` 0 findings · `bash -n` on all 16 `run:` bodies 0 · commitlint 0. ## Not fixed here `.github/workflows/test-drift.yml:177` does the same `sh install.sh` with **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.
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
1 participant
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.
Six proven fixes to
Fix Drift, harvested onto the merged #348 main. One is a security fix.What changes
The one unpinned executable in the job no longer runs while the app token is live.
sh <(curl https://ollama.com/install.sh)is arbitrary third-party code from a mutable URL, and itsat six steps after an app token with
contents: write+pull-requests: writewas minted intothe job. Everything else in that job is pinned — every
uses:by commit SHA,pnpm installby thelockfile. This was the exception. Two changes, because neither alone closes it: the step now runs
before the mint, and the downloaded bytes are checked against a sha256 with a hard
exit 1before
shever sees the file. Reordering alone leaves the real attack — a compromised scriptplants a
git/gh/nodeon PATH and the later token-holding steps run it.A note some open PR already proposed no longer discards the rest of the run. The needs-human
step's per-note guard
exit 0'd the whole step on the first overlapping note. On a mixed run — aregistry edit applied AND a different family deferred — the registry edit was committed locally,
never pushed, proposed to nobody, and re-abandoned identically every morning. The stand-down can
never be right (control only reaches that loop once the changeset guard has declined, so nothing
this run carries is proposed anywhere), so it is removed rather than narrowed. The overlap is still
reported, as a warning.
A rejection survives a human editing the closed PR that records it. Closing a drift PR rejects
its changeset, and the record is a marker in that closed PR's body. The marker self-heal that
exists to repair deleted markers listed
--state open, so it never saw the closed PR — a humanwriting down why they declined destroyed the rejection permanently and the bot re-proposed every
morning. Both self-heals now list
--state all; candidacy is unchanged (every candidate stillcarries this run's key as its branch suffix). MERGED reaches neither self-heal, and on the
needs-human step CLOSED is admitted through the branch-key anchor only, so the closed backlog
cannot drag a truncated file list into the fail-closed audit.
The persist step's note scan no longer needs a bash-4 builtin.
mapfilewas the only bash-4construct anywhere in the workflow, and it made that step the one body a test could not execute on
a stock macOS box — which is exactly how a hand-written mirror gets written instead of a run.
Two test/doc commits. The alert payloads are now POSTed to a socket that records them, so the
method, header and body are measured on the wire instead of against a
curlstub. And DRIFT.md nolonger offers deleting a closed PR's marker as a way to un-suppress a rejection, because the
self-heal now puts it back.
Red-green, re-run on this base
These commits were written against an older base. Every proof was re-run here and watched.
ollama. Pre-fix restored: the step sits at index 7 against the token's index 1, declares no
digest, and hands attacker-substituted bytes straight to
sh(shRan=true). Post-fix the samepayload is refused with
::error::…does not match its pinned sha256,shis never reached, andthe step exits 1 — while bytes that DO match still execute, so the gate is not "always refuse". The
pin was re-verified against the live URL rather than taken on trust: 15902 bytes,
25f64b81…82c9f, byte-for-byte.per-note. Pre-fix restored:
created=false,pushed=false, andneeds-human: note drift-proposals/openai-mythical-1.new-family.md already proposed in open PR #42 — not opening a duplicate. Post-fix it warns, pushes and opens exactly one PR; the negativecontrol (a re-fire of the SAME changeset) still opens no second PR.
rejection. Eight mutations — reverting either listing to
--state open, dropping the MERGEDexclusion, relaxing or removing the state narrowing from either jq program — all red, none
survived. The pre-fix red is the workflow pushing a branch for a changeset a human had rejected.
alerts. Four mutations on the wire (
-X PUT, dropped Content-Type, a literal\n, the wrong-dvariable) all red.Not included
fd867dc("a provider whose live listing was too short to trust is not a quiet day") isdeliberately absent — it needs the threshold fix that ships with it, and alone it would red the
daily cron every morning.
scripts/drift-sync.tsis untouched here.Verification
pnpm build,pnpm test(5044 passed),pnpm test:drift(119 passed / 66 skipped),pnpm test:exports,tsc --noEmit,eslint,prettier --check,actionlint,bash -non all13
run:bodies, andzizmor --min-severity mediumall exit 0.fix-drift-workflow.test.tsgoes130 → 150 tests.
Three files change. No
package.json,CHANGELOG.md,pnpm-lock.yaml,charts/or.claude-plugin/— this PR consumes no version.Still unproven
The workflow has never executed on Actions; every proof here is a local run of a
run:body withgh,curlandgit pushstubbed. That Slack renders these messages as intended is alsounproven. And the ollama pin will red the cron on the next upstream publish — fail-closed, chosen
deliberately, with the digest printed in the error.