fix(drift): one Fix Drift run per day, and an alert that names the PR - #348
Draft
jpr5 wants to merge 40 commits into
Draft
fix(drift): one Fix Drift run per day, and an alert that names the PR#348jpr5 wants to merge 40 commits into
jpr5 wants to merge 40 commits into
Conversation
commit: |
jpr5
added a commit
that referenced
this pull request
Aug 4, 2026
…unreadable file list is not "note absent" Two fail-silent defects in the CANDIDATE SELECTION around the PR-body marker repair (the repair itself is unchanged). H-F3. `^fix/drift-` is not a bot marker. Humans use `fix/drift-<slug>` for drift work — #348 `fix/drift-alert-signal`, #351, #352, which is exactly the hand-off the needs-human PR body asks a human to perform. So the note-path anchor selected HUMAN PRs: the step appended machine markers to prose it does not own, and folded that human PR into the bot's dedup set, where it suppressed a genuine needs-human PR indefinitely. Candidacy now requires the bot branch shape both branch builders emit — prefix plus date, run id and a trailing 16-hex changeset key — which is the signal the ok-applied dedup already relied on, generalised from "this run's key" to "a changeset key" so a bot PR carrying a DIFFERENT key (precisely what the note-path anchor exists for) still qualifies. A guard pins the pattern against BOTH branch-construction sites, so changing either shape reds instead of silently selecting nothing. H-F1. The note-in-files question was answered by TWO independently written jq expressions that had to agree, the second ending `jq -e … >/dev/null 2>&1` — so a jq fault, a malformed payload, or gh's `first: 100` file-list truncation ALL read as "note not present", which is the answer that opens a duplicate PR. One jq definition now answers it once and carries its verdict forward with the candidate (`keyAnchored`, `wantMarkers`), so nothing downstream re-derives it; an unreadable or truncated file list on a bot candidate is reported and FAILS CLOSED instead of being read as absence. The note-path anchor previously had no guard anywhere: it could be killed three ways with the whole suite green. It now has executable ones — the step's own jq programs and its whole NOTES_JSON…REASSERTED section are run against fixture payloads and what they select is measured, so forcing the index check false, rewiring the notes argjson to `[]`, dropping the anchor, or reverting the bot pattern each red.
fix-drift.yml declared BOTH a `schedule` cron (6:10 UTC) and a `workflow_run` leg on "Drift Tests", and admitted the latter whenever the upstream run concluded `failure`. While Drift Tests was green the workflow_run leg resolved `skipped`, so the duplication was invisible. From 2026-07-29, when Drift Tests went red, every day produced TWO identical Fix Drift runs — one `workflow_run` and one `schedule` — each re-running the full sync and each firing its own Slack alert. `concurrency: drift-fix` serialises the two runs but does not dedupe them. The workflow_run leg bought nothing: test-drift.yml's only main-branch trigger is its own 6:00 cron and this workflow's is 6:10, so the leg's entire contribution was ~9 minutes of latency on an unattended daily job. Delete it, and the now-unreachable `github.event.workflow_run.conclusion` leg of the job's `if:` gate with it. Also drops the now-dead `dangerous-triggers: fix-drift.yml` suppression from .github/zizmor.yml — it only existed to excuse the workflow_run trigger, and zizmor is clean at min-severity medium without it. The double-fire guard added to fix-drift-workflow.test.ts parses the workflow's `on:` block and asserts at most ONE declared trigger can fire when "Drift Tests" concludes failure, plus that the job's `if:` carries no leg gated on an undeclared event. Both halves were mutation-tested: each fails when its half of the change is reverted. (cherry picked from commit 191e86c)
The "Alert on needs-human decision" step renders its PR link from
`steps.needs_human_pr.outputs.url`, which was written ONLY at the very end
of the persist step, after `gh pr create`. That step has three early
`exit 0` returns — no new commit, changeset-key dedup hit, per-note dedup
hit — and none of them wrote `url=`, so every one of them produced the
contentless alert variant. The changeset-key path even HAD the number in
`$DUP` ("already proposed in open PR #343") and discarded it.
That is why six consecutive days of needs-human alerts named no PR: the
run that opened #343 carried the link, and every run after it took a dedup
path.
Hoist the open-PR lookup above the no-new-commit guard, resolve both the
number and the url, and emit them via a small `emit_pr` helper on all four
exit paths (three dedup returns plus the freshly-opened PR). The alert now
renders "Review the note in PR #N ... merge: <url>"; its no-PR fallback no
longer claims the note is "already proposed in an open PR", because with
the lookup hoisted that case always yields a url — reaching the fallback
now means the opposite.
Also adds `--limit 200` to every `gh pr list` here and in the ok-applied
Push+PR step. This is in the fix's blast radius, not drive-by: the hoisted
lookup IS the dedup lookup, and `gh pr list` defaults to 30, so once 30
newer PRs exist the already-proposed PR falls out of the window, the guard
misses it and a duplicate PR is opened (verified locally: with 35 open PRs
and the duplicate at position 33, the unpatched guard pushes a branch and
calls `gh pr create`; patched, it finds #343 and skips).
Regression guards assert every early return emits before exiting, that the
lookup precedes the no-new-commit guard, that the alert renders #N, that
the stale fallback wording is gone, and that no dedup/matching
`gh pr list` runs without an explicit --limit.
(cherry picked from commit a5d5b5c)
…h no Slack The sync step ran under `set +e`, recorded an `exit_code` output that nothing read, and always exited 0. drift-sync.ts's fatal handler exits 1 WITHOUT printing a `reason=` line (runDriftSyncCli awaits fetchProviderChurnInput for every provider before its first console.log), so a crash left REASON="", every alert `if:` keyed on a reason value false, and the end-of-job catch-all — which needs `failure()` — skipped as well. Observed: crash -> green job, zero Slack. Not hypothetical. An invalid GOOGLE_API_KEY makes Gemini answer 400, and isInfraSkip() only absorbs 401/402/403/429/5xx, so a 400 throws out through that handler. Reproduced by running the real drift-sync.ts. - classify an empty or unrecognised `reason=` as `sync-crashed` and fail the step, so the job goes red; the three classified reasons still exit 0 because their handling steps are gated on success() - route `sync-crashed` to the gate-failure alert (a crash is a tooling fault, not a product decision) and name drift-sync's real exit code in the message, which also makes the previously-unread `exit_code` output load-bearing - make the catch-all an actual catch-all: it was keyed on `reason == ''`, so on an ok-no-churn day (the common case) a failure in any later step — e.g. one of the two always() artifact uploads — fired NO alert at all, contradicting the comment above it. It now keys on "the job failed and neither specific alert ran", and its message names the window instead of always blaming setup. - turn on pipefail and make the two tolerated greps say `|| true` explicitly (cherry picked from commit a5151fb5a94f443f49732bb896be2b024c3a9f75)
Both dedup guards ran `gh pr list … 2>/dev/null || echo '[]'`, which encodes a 401 / secondary rate limit / 5xx as the positive assertion "there are no open PRs". That does not degrade the guard, it DISABLES it, and the next thing each step does is open the duplicate PR the guard exists to prevent. Reproduced: with the list call failing and a matching open PR already present, the step opened a second PR and then died red blaming a missing PR for what was a 401. - treat a failed or non-array `gh pr list` as fatal on both PR paths, with a message that names the API error, instead of as "found nothing" - fail closed on an empty CHANGESET_KEY instead of silently switching the PRIMARY guard off and falling through to open a PR (computeChangesetKey only returns "" for an all-no-op run, which neither of these paths can be) - stop the head-SHA polling loops reporting a hard API failure as "not indexed yet"; keep the gh error and report it when the loop gives up (cherry picked from commit 185027ba917545a742b7f7fbab0a89705c914a25)
All four alerts built their message with "\n" inside a double-quoted bash string. That is already the two characters backslash and n, so `jq --arg` correctly escapes it to \\n and Slack displays a visible \n — which is what the owner's own screenshots show. test-drift.yml already documents this exact gotcha and works around it with NL=$'\n'; adopt the same idiom here. Observed by rendering each of the four payloads and inspecting the bytes: before, every message was a single Slack line carrying a literal \n; after, each renders 2-4 real lines with zero literal \n. The JSON escape goes from \\n to \n. (cherry picked from commit 99a66d45f2f694caa35cd4365be2b761bd12705f)
`.body | contains($m)` THROWS on a null body, and GitHub returns `body: null` for a PR with an empty description. A single such PR anywhere in the repo's open set made BOTH dedup guards die (observed: jq error, step exit 5), taking the whole drift pipeline down with it — the ok-applied rejected/duplicate lookups, the needs-human rejected/duplicate lookups and the per-note loop all read the same field. Use `(.body // "")` in every one. (cherry picked from commit 83d0b74eee20870e7d737d8ada56d3c760b5a467)
The missing-SLACK_WEBHOOK branch of the sync-success notify exited 0 while all three sibling alerts exit 1, so a dropped notification left the job green and nobody was told a PR is waiting — the same fail-silent shape the rest of this workflow is being fixed for. Made consistent with its siblings. (cherry picked from commit 54a12ae825a33cc6b2d6dd020e95136b4b043c67)
…delling them The two guards this branch needs — a drift-sync crash must be audible, and every Slack alert must carry a real newline — are both answered by running the step's own `run:` body and reading the result, so neither is a re-implementation of bash or jq in TypeScript. A step slicer reads `name`/`id`/`if`/`env` keys and the dedented `run:` block straight out of the workflow. It is held to the artifact by a guard that re-indents every extracted body and requires it to be a literal substring of the file, so a slicer that paraphrases fails rather than passing quietly. `observeSyncStepUnderCrash` runs the sync step against a drift-sync that dies in its fatal handler and reports the step's real exit status and real `$GITHUB_OUTPUT`. `assembleSlackMessage` runs each alert body up to its `MSG=` line and prints what `$MSG` holds, with env overrides so each arm of a multi-branch message is observed on its own. The early-infra catch-all's two guards move onto the same footing: its gate is read from the parsed `if:` rather than a 400-byte window, and both arms of its WHERE= message are assembled and compared. Content adapted from 42673a4 and 6cf9fd0 on the superseded branch. Their `simulateJob`/`evaluateIf` dependency is not carried over: whether a published reason reaches a human is answered here by matching it against the alert steps' own `if:` text, which needs no model of the runner's expression evaluator.
…w does DRIFT.md said Fix Drift also runs on a failed `Drift Tests` run. It does not: that trigger was the double-fire, and a maintainer following the doc would re-add it. The zizmor ignore counts the `pull_request` fields it enumerates. The suite header no longer calls itself text-only, since two of its guards now run the workflow's own shell.
… declare Also drops the guard that asserted the string "no PR spam" appears somewhere in the file. Every line of dedup logic could be deleted and it stayed green; the five guards on the changeset-key and note-path markers are what actually hold that behaviour.
Nine of these could not be made to fail. Mutation-testing found each one:
- re-adding the workflow_run trigger in BLOCK-sequence YAML kept the
double-fire guard green, because the trigger reader only understood the
flow-sequence spelling of `workflows:`. It now reads both, and an absent
list counts as covering Drift Tests rather than as covering nothing.
- replacing the persist step's `gh pr create` with `true` kept six guards
green: they grepped the whole step, and its rationale comment says
`gh pr create`. Questions about what a step DOES are now asked of its code
surface, with comment lines dropped.
- renaming the needs-human branch into the ok-applied `fix/drift-*` namespace
kept the distinct-branch guard green, because the temp PR-body filename also
contains `drift-needs-human`. It now reads the BRANCH= assignment.
- gutting the per-note dedup loop, and blanking the ok-applied step's
duplicate lookup, both stayed green: the marker strings and the dead skip
branch survived. Both now require the query and the derivation.
- deleting the needs-human alert's terminal `exit 1` stayed green, because
both alert bodies carry a conditional `exit 1` in their missing-webhook
branch. The guard now requires the LAST statement to be it.
- ungating the defense-in-depth assert step stayed green, since the ok-applied
PR step's own `if:` contains the gate as a substring. Read per step now.
- dropping the persist step's outcome from the gate alert's `if:` stayed
green, since the step also imports that outcome as env.
- the --limit guard matched `gh pr list …` against the flattened file, so an
invocation whose --json came after a pipe was never examined. It now walks
each invocation, and skips the `::error::` prose that is not a call.
…must loop Two more guards that could not be made to fail. The gate-failure alert's `FAILING_STEP` check passed on a body where an arm had been renamed out of use; it now assembles the message under three failure windows and requires three distinct results. The per-note dedup check matched the PR-body writer's loop over the same variable, so gutting the dedup loop stayed green; it is now scoped to the region between the git-diff scan and the branch push.
A boundary regex that only breaks on `- name:` merges the bare `- uses:` steps into the step above, which would hand a neighbouring body to the two executors.
…one they name The sync step's reason= output was matched against the whole flattened file, where the assert step's own `echo "reason=…" >> $GITHUB_OUTPUT` satisfied it — the sync step could stop publishing a reason and the guard stayed green. The ok-applied PR-open step's gate was read out of a byte window rather than the step's `if:`. Also pins that the slicer sees the unnamed `uses:` steps: a boundary that only breaks on `- name:` merges them into the step above.
fetchProviderChurnInput turns an unusable credential into a SKIP, so a revoked key made every provider skip, the core report ok-no-churn, and the job end GREEN with no alert — byte-identical to a quiet day. Classify the AUTH-class skips as provider-unchecked, fail the step, and key the gate-failure alert on it.
Both dedup guards match an HTML comment parked in prose a human owns. A wholesale rewrite of PR #343's body deleted its markers and the next scheduled run opened duplicate PR #350. And dedup only listed OPEN PRs, so closing a proposal to reject it guaranteed an identical one every morning after. Each PR-open step now re-asserts its own markers off an anchor a body edit cannot touch, and lists --state all so a rejection is respected.
…observed not grepped
…ead of dying silent
…ds as a decision to review
…et in silence Both PR-open paths tested "a human closed a PR carrying this marker" BEFORE "an open PR already carries it". `--search "<key> in:body"` is state-blind, so one changeset routinely matches several PRs: on 2026-08-04 the live query for eaa8db65f5647493 returned [#350 CLOSED, #343 MERGED], and the closure of the spam DUPLICATE read as a rejection of the changeset itself. The result was permanent suppression with no PR, no Slack (the needs-human alert is gated on `rejected == ''`) and no recovery path — the self-heal lists `--state open` only, so it never touches the closed PR. The open-PR test now runs first, so a closure only counts once no open PR carries the changeset. A merged PR still cannot read as a rejection. And the suppression is no longer silent: `rejected`/`rejected_url` (the ok-applied `rejected` output was previously written and read by nothing at all) feed a new notification naming the closing PR and the two ways back out. Also in the same alerting surface: - A FAILED Slack POST was read as proof of delivery. The catch-all stood down on `alert_*.outcome != 'failure'`, but a `curl -fsS` dying against a rotated webhook exits the step non-zero too — so the likeliest way for alerting to break also disabled the backstop covering it. Delivery is now proven positively with a `posted=true` output written only after the curl returns 0, and every POST is bounded (`--max-time`, a small `--retry`). - The cancellation POST is hoisted ahead of both `always()` artifact uploads, which sat in front of it inside the single 5-minute cancellation budget. - The stale-key preflight keys on a new `unchecked-providers=` MACHINE line instead of grepping drift-sync's human-facing log prose, and an unreadable log (grep exit 2) or an absent line is now a fault rather than proof that every provider was checked.
…the suite
An adversarial pass applied 42 mutations to this workflow: 28 red, 14 GREEN, and
applying all 13 compatible vacuous ones AT ONCE still gave actionlint 0,
`bash -n` clean and 4991/4991 passing. One third of the guards protecting this
PR's fixes could not fail. Each is replaced (not supplemented) by one that can:
- The empty-changeset-key refusal was sliced with
`code.indexOf("\n fi", at)` — a `fi` at 20 spaces of file indent,
which exists nowhere here — so `indexOf` returned -1, `slice(at, -1)` spanned
the whole rest of the step, and 7 (ok-applied) / 14 (needs-human) unrelated
`exit 1`s satisfied the assertion. The refusal could be replaced with `echo
"continuing dedup-blind"` and stay green. Blocks are now sliced on their own
indentation and THROW when the closer is absent.
- The marker self-healer had three independent kill switches: emptying its loop
input, requiring two missing markers rather than one (the #343 -> #350 incident
shape exactly), and writing bare markers its own detector never matches. The
third passed because the assertion ran against the whole step, where the
unrelated PR-BODY writer satisfied it — the wrong artifact.
- The `--search` prohibition was `slice(0, indexOf("gh pr edit"))` plus a
`(?![\s\S]*gh pr edit)` lookahead: a slice that by construction contains no
`gh pr edit`, so the lookahead could never match.
- The closed-PR dedup asserted its literals existed but never that they were
WIRED UP, so pointing the CLOSED selector at the OPEN-narrowed listing, or
dropping `state` from the `--json` list, both passed with the rejection path
dead. It now checks the wiring and EXECUTES the real jq selectors against the
live #350/#343 payload.
- The dropped-notification guard only inspected the missing-webhook branch, so
dropping `-f` (a rotated webhook's 404 then exits 0) or appending `|| true` to
all four POSTs passed.
- `continue-on-error` appeared zero times in the workflow AND the suite, and one
such line on `sync`, `pr` or `alert_gate` defeated three claims. The slicer now
reads it, `simulateJob` models it, and a MODEL guard proves the ban is
load-bearing.
`simulateJob` also no longer equates "selected" with "delivered": an alert whose
`curl` failed delivered nothing, which is what makes the `posted=true` backstop
key testable at all.
And `bot_managed` — the predicate deciding which PRs the self-heal may touch —
had no test anywhere, while the workflow comment asserted "A test pins this
pattern against BOTH branch-construction sites". It now does, by running both
`BRANCH=` builders under bash and feeding the result to the workflow's own jq.
F1 was not one bad guard, it was one bad HABIT: deriving a slice bound (or an ordering fact) from an `indexOf` whose -1 was never checked. All 39 locator call sites in this file were audited. Seven fed a bound with no -1 check and now route through a `locate()` that throws; the rest are explicit presence tests, or a `lastIndexOf(…) + 1` whose -1 is the correct start, and are left alone with a note saying which. The audit turned up one more guard in the same class, and it was live: "every `git push` runs behind the app-token `insteadOf` credential" called `steps()` TWICE and then `order.indexOf(inject)`. `steps()` re-parses, so the two arrays hold different objects, `indexOf` returned -1 for the injector, and every real push index is greater than -1 — the ordering claim was trivially true. Observed both ways: moving the credential-injection step to AFTER the push left the old guard green and reds the new one. Also folded the two near-identical `checks: read` / `statuses: read` permission tests into one (same slice, same scope, two assertions) rather than leaving a second copy of a guard to keep in step.
…T.md now says how to undo it Two behaviours a maintainer could previously only learn by reading the workflow: - Closing a drift-sync PR suppresses that changeset permanently. DRIFT.md now states it, states that a still-OPEN PR outranks a closed one (so closing a duplicate rejects nothing), states that a MERGED PR is never a rejection, and — the part with no other discoverable source — names the way back out: reopen the PR, or delete the changeset marker from its body. - An unusable provider credential makes the run `provider-unchecked` rather than `ok-no-churn`, keyed on the `unchecked-providers=` machine line, with 429/5xx still tolerated. Guarded, because the recovery instruction is the one line whose absence leaves a drifted registry with no way for a human to find the exit.
… return, not after it The closed-rejection test moved past TWO gates last round, not one. With the `HEAD_SHA == BASE_SHA` return in front of it, a re-fire whose note is already in the repo (a previous needs-human PR was merged) exited 0 before the rejection was ever tested: no `rejected` output, so the suppression notice could not fire, and the needs-human alert took its contentless "No open PR carries this decision" branch and failed the job — red every morning, on the exact state this work exists to quiet. That gate says nothing about OPEN-vs-CLOSED precedence, so it now runs LAST of the three changeset-keyed decisions. The guard could not see it: it compared the two selectors' text positions, which stayed in the demanded order while the rejection became unreachable. It now constrains the rejection test against EVERY early return in the step — only the open-duplicate dedup may reach `exit 0` first — so a third gate cannot be interposed later either. Also corrects both copies of the reorder comment. Run against the real jq selectors, `[#350 CLOSED, #343 MERGED]` yields REJECTED -> #350 under BOTH orders, because #343 is MERGED and never a pending proposal; only the `[CLOSED, OPEN]` shape changes behaviour, and what makes the merged case recoverable is the suppression notice, not the ordering.
…nds the day green and silent `exit_code` was captured, published, and read by nothing that could act on it: the sync step's only exit test keyed on `reason` alone, so `reason=ok-no-churn` plus a non-zero drift-sync process exit left the step exiting 0, the job green, every reason-keyed alert condition false and the catch-all (which needs `failure()`) silent — zero Slack, byte-identical to a genuinely quiet day. Reachable from any Node-level failure after the log lines print. A non-zero exit under an `ok-*` reason is now reclassified `sync-crashed`, the bucket the gate-failure alert already keys on. `needs-human` and `gate-failed` exit non-zero by design and already alert, so they are left alone. The gate alert's wording no longer asserts that a crash printed no reason line. Observed by executing the step's own body against a stub that prints a clean quiet-day log and exits 1: step exit was 0 before, non-zero after, with a known-negative pinning that the same log with a zero exit is still a quiet day.
…ing a note that does not exist On an ok-applied run the needs-human persist step is gated `reason == 'needs-human' && success()`, so the runner always evaluates and skips it and its outcome is ALWAYS 'skipped'. That arm sat first in the FAILING_STEP ladder, so every ok-applied failure — a refused assert, a rejected push, a `gh pr create` error, an artifact upload — reported "the needs-human note was NOT persisted". False on a run that has no note, it sent a human hunting for a file that does not exist, and it left the assert/pr/else arms dead: they need the persist step to be neither failure nor skipped, i.e. success, and this alert never fires then. Both needs-human arms are now gated on the run shape as well as the outcome, `Configure git for push` has an id so its window can be named, and the fallback reports the outcomes it observed rather than "an unidentified step". The fixture hid it: `NEEDS_HUMAN_PR_OUTCOME: ""` is a state the runner cannot produce, which is what made the dead arms look reachable. Both gate-alert message guards now derive every outcome from the job simulation, and the windows they cover are five real scenarios, each asserted to NAME its failing step.
…e artifact uploads
…rely formatted right
jpr5
force-pushed
the
fix/drift-alert-signal
branch
from
August 5, 2026 17:44
9549e23 to
692334d
Compare
…ery instead of preceding it `rejected_ack` is the state that silences the human-rejection notice for ever, and it was written inside the persist step — several steps BEFORE the notice POSTs. So on the first run after a closure the marker landed, the POST then failed, and every run after it read `rejected_ack=true`, stood the notice down and exited GREEN and silent: the suppression permanently un-narrated. Reached with certainty rather than by chance when the webhook has been rotated, which also kills the catch-all's POST that day, leaving nothing behind but a red cron run. The write now lives in its own step placed after the notice and gated on `steps.alert_rejected.outputs.posted == 'true'` — the same positive proof of delivery the catch-all stands down for — so the receipt cannot exist without the telling. Every failure path in it warns and exits 0, so an unrecordable ack costs one repeated ping and never a red run on a day the telling landed. The persist steps now only READ the marker. That step also scopes the write the way the marker self-healer is scoped (bot author AND a drift branch): rejection detection is body-keyed and state-blind, so a human PR that quotes or inherits the changeset marker was a full-body-rewrite target. It re-reads the PR live for the branch and author, which also stops a rewrite from the stale `--search` copy. Guards: the writer is located from the artifact, its position compared with the notice's, and its unreachability without a delivered POST settled by simulating the job with that POST failing. Mutation-tested: reverting the move, dropping the gate, defeating the gate with an extra || clause, dropping always(), rewriting instead of appending, flipping the closure precedence, deleting the idempotency check, deleting either half of the ownership test, and reddening the write-failure path all fail the suite.
…ranch on, within budget
…hatch are all covered
jpr5
force-pushed
the
fix/drift-alert-signal
branch
from
August 5, 2026 19:36
0367ff3 to
1b06649
Compare
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.
Fix Driftfailed every day and its Slack alerts arrived without saying what to look at. This fixes both.What changes
One automatic run per day. The workflow had two automatic legs — a
schedulecron and aworkflow_runleg that fired on a failedDrift Testsrun. Both fired, so a redDrift Testsproduced two Fix Drift runs. Theworkflow_runleg is removed;scheduleplus manualworkflow_dispatchremain.Alerts name the PR and render as text. Three early-return paths reached the alert step without ever setting the PR URL, so the message arrived contentless. The PR is now emitted before the no-new-commit guard, so every exit path carries it. Slack alerts also emitted a literal
\ninstead of a line break; they now render real newlines.A crashed sync can no longer report a green job with no Slack. A fatal
drift-synccrash printed noreason=line and the step swallowed its exit code, so every alert including the catch-all was skipped and the daily job went green in silence. A crash now alerts.A tooling fault is no longer reported as a product decision. A skipped persist step — which any earlier failure can cause — satisfied
!= 'failure'but not== 'failure', routing an engineering fault to the needs-human alert instead of the triage one.A null PR body no longer kills the dedup queries.
.body | contains(...)throws on a null body; both dedup paths now null-guard it. A dropped success notification also no longer passes as a green job.An unanswerable dedup query is no longer read as "no duplicate." It now fails closed.
With the
workflow_runtrigger gone,fix-drift.ymlno longer needs itsdangerous-triggersexemption, so that entry comes out of.github/zizmor.yml— the workflow is back under the rule rather than excused from it.DRIFT.mdand the zizmor comment now describe what the workflow actually declares.Verification
Red-green is against the real
run:bodies, executed under bash 5.2 in a container with onlyghandcurlstubbed — not against a re-implementation of the workflow's semantics.The double-fire RED is production itself: seven consecutive days of two
failureruns per day, oneworkflow_runand oneschedule. GREEN is one automatic leg.The alert RED is the actual daily scenario — note persisted on an earlier run, PR still open — where the payload names no PR and carries
\non the wire. GREEN names the PR and breaks the line.Every guard in the test file was mutation-tested: 80 mutations, 79 red. Two guards that could not be made to fail were deleted rather than kept, because a test that cannot fail advertises coverage that does not exist.
Deliberately not in this PR
A follow-up carries the remaining alert-plumbing work: keying the catch-all on delivery rather than on the alert step's exit status (a rotated webhook or a Slack 5xx currently makes the alert "fail" without posting, and the catch-all then stands down), the preflight's blindness to
drift-sync's "listing too short to trust" skip, the per-note guard's run-wideexit 0, and rejection semantics when a human edits a closed PR's body.