From e2a8b6ca1dac1e209513192183879935f30c71a7 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 15:33:28 +0000 Subject: [PATCH 1/3] ci: make required checks actually gate auto-merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit auto-merge-prs.yml triggered on pull_request [opened, ready_for_review] and called `gh pr merge --auto` immediately. `--auto` does not wait for CI — it waits for whatever branch protection requires, and this repo has no required status checks configured, so "wait for the required checks" resolved to "wait for nothing" and PRs merged seconds after being opened. Observed, not theorised: #183 merged with `All required checks pass` RED, while web/check and apps/desktop/check:test:ui were both failing on the React version mismatch. #185 merged 11 seconds after it was created, before CI finished. The repo had already half-learned this one level down — ci.yml's all-checks-pass job carries a comment that desktop-install-windows "merged red three times before being listed here, because nothing was waiting on it." Nothing was waiting on the aggregator either. The trigger is now the completion of the CI workflow, and the decision is read from the `All required checks pass` check run on the head commit. Keyed on that check run rather than workflow_run.conclusion, and the difference is load-bearing: CI as a whole can conclude failure because of a lane nobody requires — the Docker build is explicitly outside the gate's needs list — so gating on the overall conclusion would block merges on lanes the repo has already decided are non-blocking. The aggregator is the contract, and it already counts `skipped` as passing, so Python-only PRs aren't held up by frontend lanes. One re-run, then stop. A red gate blocks the merge; if the run hasn't been retried, its failed jobs re-run once, which fires a fresh workflow_run completion that re-enters at attempt 2 where no further retry is offered. A genuinely broken PR fails twice and stays blocked. This is what keeps the known ui-tui/check flake (ink-resize.test.ts) from wedging a good PR permanently. Fork PRs never reach the merge path, since a workflow_run job holds a write token in the base repo's context. Drafts are skipped, a `do-not-merge` label is an escape hatch, and a PR whose head moved since CI ran is left to the newer run. Actions can't be executed here, so the step's shell was extracted and run against a mock gh that logs every merge/rerun requested. Nine cases: success merges; failure/cancelled at attempt 1 re-runs without merging; failure at attempt 2 neither re-runs nor merges; a missing gate, a draft, a do-not-merge label, a moved head and an already-merged PR all decline. The only path that merges is success. Separately verified that the jq name filter picks the aggregator out of a noisy check-run list, and that a renamed gate yields an empty conclusion — it fails closed, not open. This is not branch protection: it stops this automation from merging red, not a human merging by hand. Requiring the aggregator in the branch rules is still the real fence and composes with this. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 --- .github/workflows/auto-merge-prs.yml | 219 ++++++++++++++++++++------- docs/system-log/2026-08-12.md | 90 +++++++++++ 2 files changed, 258 insertions(+), 51 deletions(-) diff --git a/.github/workflows/auto-merge-prs.yml b/.github/workflows/auto-merge-prs.yml index 381db8d0382b6..6c6f510fdb96a 100644 --- a/.github/workflows/auto-merge-prs.yml +++ b/.github/workflows/auto-merge-prs.yml @@ -1,61 +1,101 @@ name: Auto-Merge Pull Requests +# Merge a PR when — and only when — CI's aggregate gate has actually passed. +# +# ── Why this changed ──────────────────────────────────────────────────────── +# +# This used to trigger on `pull_request: [opened, ready_for_review]` and call +# `gh pr merge --auto` straight away. `--auto` does not wait for CI on its own: +# it waits for whatever *branch protection* requires. This repo has no required +# status checks configured, so "wait for the required checks" resolved to "wait +# for nothing" and PRs merged within seconds of being opened, before CI could +# finish or fail. +# +# That is not hypothetical. #183 merged with `All required checks pass` RED. +# #185 merged 11 seconds after it was created. Neither was reviewed by CI in +# any meaningful sense. +# +# So the trigger is now the *completion of the CI workflow*, and the decision +# is read from the `All required checks pass` check run on the head commit — +# the aggregator job that already encodes which lanes are required (see +# `all-checks-pass` in ci.yml). A lane that is `skipped` counts as passing +# there, so a Python-only PR is not held up waiting on frontend lanes. +# +# Keyed on that check run rather than on `workflow_run.conclusion`, and the +# difference matters: the CI workflow as a whole can conclude `failure` because +# of a lane nobody requires — the Docker build is deliberately left out of the +# gate's `needs` list, with a comment saying so. Gating on the workflow's +# overall conclusion would block merges on lanes the repo has explicitly +# decided are not blocking. The aggregator is the contract; read the contract. +# +# ── One re-run, then stop ─────────────────────────────────────────────────── +# +# A red gate blocks the merge. If the run has not already been retried, its +# failed jobs are re-run once — the known `ui-tui / check` flake +# (`ink-resize.test.ts`, seen once on #183's run and not reproducible in 23 +# local runs) should not permanently wedge a good PR. The re-run produces a +# fresh `workflow_run` completion, which re-enters this workflow at attempt 2, +# where the retry is not offered again. A genuinely broken PR therefore fails +# twice and stays blocked, which is the intended outcome. +# +# ── What this is not ──────────────────────────────────────────────────────── +# +# This is a workflow-level gate, not branch protection. It stops *this +# automation* from merging red, which is the hole that actually bit us three +# times. It does not stop a human merging by hand, or a direct push to main. +# Requiring `All required checks pass` in the branch rules is still the real +# fence; it composes with this rather than being replaced by it. + on: - pull_request: - types: - - opened - - ready_for_review - - reopened - pull_request_review: + workflow_run: + workflows: + - CI types: - - submitted + - completed workflow_dispatch: inputs: pull_request_number: - description: 'Pull request number (leave empty to enable on all open PRs)' + description: 'Pull request number (leave empty to evaluate all open PRs)' required: false type: string concurrency: - group: auto-merge-${{ github.event.pull_request.number }} + # Keyed on the head SHA for workflow_run (the PR number is not a top-level + # field on that event) and on the input for a manual dispatch. + group: auto-merge-${{ github.event.workflow_run.head_sha || github.event.inputs.pull_request_number || github.run_id }} cancel-in-progress: false jobs: auto-merge: name: Enable Auto-Merge runs-on: ubuntu-latest + # Fork PRs never reach the merge path. `workflow_run` runs in the base + # repo's context with a write token, so honouring one for code that came + # from a fork would hand that token's reach to anyone who can open a PR. + if: >- + github.event_name == 'workflow_dispatch' || + github.event.workflow_run.head_repository.full_name == github.repository permissions: pull-requests: write contents: write + checks: read + actions: write # re-run failed jobs steps: - - name: Enable auto-merge for pull request(s) + - name: Merge PRs whose required checks have passed + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + EVENT_NAME: ${{ github.event_name }} + HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + RUN_ID: ${{ github.event.workflow_run.id }} + RUN_ATTEMPT: ${{ github.event.workflow_run.run_attempt }} + INPUT_PR: ${{ github.event.inputs.pull_request_number }} run: | - REPO=${{ github.repository }} - - # Determine which PRs to process - if [ -n "${{ github.event.inputs.pull_request_number }}" ]; then - # Manual trigger with specific PR - validate input (must be numeric) - INPUT="${{ github.event.inputs.pull_request_number }}" - if ! echo "$INPUT" | grep -qE '^[0-9]( [0-9])*$'; then - echo "Error: Invalid PR number format. Must be numeric, space-separated." - exit 1 - fi - PR_NUMBERS="$INPUT" - elif [ -n "${{ github.event.pull_request.number }}" ]; then - # Automatic trigger from PR event - PR_NUMBERS="${{ github.event.pull_request.number }}" - else - # Manual trigger without PR number - enable on all open PRs - echo "Fetching all open PRs..." - PR_NUMBERS=$(gh pr list --repo $REPO --state open --json number --jq '.[].number' | tr '\n' ' ') - fi + set -euo pipefail - if [ -z "$PR_NUMBERS" ]; then - echo "No PRs to process" - exit 0 - fi - - echo "Processing PRs: $PR_NUMBERS" + # The single check run that decides everything. Must match the `name:` + # of the `all-checks-pass` job in ci.yml. + GATE='All required checks pass' retry_with_backoff() { local max_attempts=3 @@ -69,7 +109,7 @@ jobs: exitcode=$? fi if [ $attempt -lt $max_attempts ]; then - echo "Attempt $attempt failed. Retrying in ${timeout}s..." + echo " attempt $attempt failed; retrying in ${timeout}s..." sleep $timeout timeout=$((timeout * 2)) fi @@ -78,30 +118,107 @@ jobs: return $exitcode } + # ── Which PRs are we deciding about? ────────────────────────────── + if [ "$EVENT_NAME" = "workflow_dispatch" ]; then + if [ -n "$INPUT_PR" ]; then + if ! echo "$INPUT_PR" | grep -qE '^[0-9]+( [0-9]+)*$'; then + echo "Error: PR numbers must be numeric and space-separated." + exit 1 + fi + PR_NUMBERS="$INPUT_PR" + else + PR_NUMBERS=$(gh pr list --repo "$REPO" --state open --json number --jq '.[].number' | tr '\n' ' ') + fi + else + # Every open PR whose head is the commit CI just finished. Uses the + # commit→PR endpoint rather than `workflow_run.pull_requests`, + # which is empty in several documented cases. + PR_NUMBERS=$(gh api "repos/$REPO/commits/$HEAD_SHA/pulls" \ + --jq '.[] | select(.state == "open") | .number' | tr '\n' ' ') + fi + + if [ -z "${PR_NUMBERS// /}" ]; then + echo "No open PRs to evaluate." + exit 0 + fi + + echo "Evaluating PRs: $PR_NUMBERS" + RERUN_REQUESTED=0 + for PR_NUMBER in $PR_NUMBERS; do - echo "Processing PR #$PR_NUMBER..." + echo "PR #$PR_NUMBER" - # Check if PR is already merged or closed (with retry) - PR_STATE=$(retry_with_backoff gh pr view $PR_NUMBER --repo $REPO --json state --jq '.state') || { - echo " ✗ Failed to fetch PR #$PR_NUMBER state after retries" + PR_JSON=$(retry_with_backoff gh pr view "$PR_NUMBER" --repo "$REPO" \ + --json state,isDraft,headRefOid,labels) || { + echo " ✗ could not read PR #$PR_NUMBER after retries" continue } - if [ "$PR_STATE" = "MERGED" ] || [ "$PR_STATE" = "CLOSED" ]; then - echo " PR #$PR_NUMBER is already $PR_STATE, skipping" + STATE=$(jq -r '.state' <<<"$PR_JSON") + IS_DRAFT=$(jq -r '.isDraft' <<<"$PR_JSON") + PR_SHA=$(jq -r '.headRefOid' <<<"$PR_JSON") + HELD=$(jq -r '[.labels[].name] | index("do-not-merge") // empty' <<<"$PR_JSON") + + if [ "$STATE" != "OPEN" ]; then + echo " already $STATE — skipping" continue fi - # Enable auto-merge with squash merge strategy (with retry and exponential backoff) - if retry_with_backoff gh pr merge $PR_NUMBER \ - --repo $REPO \ - --auto \ + if [ "$IS_DRAFT" = "true" ]; then + echo " draft — skipping" + continue + fi + + # A human escape hatch that survives all of this automation. + if [ -n "$HELD" ]; then + echo " labelled do-not-merge — skipping" + continue + fi + + # Guard against a stale event: CI may have finished for a commit + # that has since been superseded by a push. + if [ "$EVENT_NAME" = "workflow_run" ] && [ "$PR_SHA" != "$HEAD_SHA" ]; then + echo " head moved on ($HEAD_SHA -> $PR_SHA) — skipping, the newer run decides" + continue + fi + + # ── The gate ──────────────────────────────────────────────────── + # `filter=latest` is the endpoint's default, but state it: after a + # re-run there are several check runs by this name and only the + # newest one is the current verdict. + CONCLUSION=$(gh api "repos/$REPO/commits/$PR_SHA/check-runs?filter=latest" --paginate \ + --jq ".check_runs[] | select(.name == \"$GATE\") | .conclusion" | tail -1) + + if [ -z "$CONCLUSION" ]; then + echo " no '$GATE' check run on $PR_SHA yet — not merging" + continue + fi + + if [ "$CONCLUSION" != "success" ]; then + echo " '$GATE' concluded $CONCLUSION — not merging" + + # One retry per run, and only for the run that just reported. + if [ "$EVENT_NAME" = "workflow_run" ] && [ "${RUN_ATTEMPT:-1}" -lt 2 ] && [ "$RERUN_REQUESTED" -eq 0 ]; then + echo " re-running failed jobs once (attempt ${RUN_ATTEMPT:-1})" + if gh run rerun "$RUN_ID" --repo "$REPO" --failed; then + RERUN_REQUESTED=1 + else + echo " ✗ could not request a re-run; leaving the PR blocked" + fi + else + echo " already retried (attempt ${RUN_ATTEMPT:-1}) — leaving it blocked for a human" + fi + + continue + fi + + echo " ✓ '$GATE' passed — merging" + if retry_with_backoff gh pr merge "$PR_NUMBER" \ + --repo "$REPO" \ --squash \ - --delete-branch 2>/dev/null; then - echo " ✓ Auto-merge enabled for PR #$PR_NUMBER" + --delete-branch; then + echo " ✓ merged PR #$PR_NUMBER" else - echo " ✗ Failed to enable auto-merge for PR #$PR_NUMBER (may already have auto-merge enabled)" + echo " ✗ merge failed for PR #$PR_NUMBER" fi done - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/docs/system-log/2026-08-12.md b/docs/system-log/2026-08-12.md index dabf9863e6693..7aa0e2194ae88 100644 --- a/docs/system-log/2026-08-12.md +++ b/docs/system-log/2026-08-12.md @@ -1,3 +1,93 @@ +# 2026-08-12 + +## 2026-08-12T15:35:00Z — Make required checks actually gate merges (Claude Code) + +**Repos touched:** `dizhaky/hermes-agent` + +### The hole + +`auto-merge-prs.yml` triggered on `pull_request: [opened, ready_for_review]` +and called `gh pr merge --auto` immediately. **`--auto` does not wait for CI.** +It waits for whatever *branch protection* requires, and this repo has no +required status checks configured — so "wait for the required checks" resolved +to "wait for nothing," and PRs merged seconds after being opened. + +Observed, not theorised: + +- **#183** merged with `All required checks pass` **RED** (`web / check` and + `apps/desktop / check:test:ui` were both failing on the React version + mismatch at the time). +- **#185** merged **11 seconds** after it was created, by + `github-actions[bot]`, long before CI could finish. + +The repo had already half-learned this: `ci.yml`'s `all-checks-pass` job +carries a comment saying `desktop-install-windows` "merged red three times +before being listed here, because nothing was waiting on it." The same failure +one level up — nothing was waiting on the aggregator either. + +### The fix + +The trigger is now the **completion of the CI workflow**, and the merge +decision is read from the `All required checks pass` check run on the head +commit. + +Deliberately keyed on that check run rather than `workflow_run.conclusion`. +The difference is load-bearing: CI as a whole can conclude `failure` because of +a lane nobody requires — the Docker build is explicitly left out of the gate's +`needs` list with a comment saying so — and gating on the overall conclusion +would block merges on lanes the repo has already decided are not blocking. The +aggregator *is* the contract, and it already counts `skipped` as passing, so a +Python-only PR is not held up by frontend lanes. + +**One re-run, then stop.** A red gate blocks the merge; if the run has not been +retried yet, its failed jobs are re-run once. The re-run fires a fresh +`workflow_run` completion, which re-enters at attempt 2 where the retry is not +offered again. A genuinely broken PR fails twice and stays blocked — which is +the point. This is what keeps the known `ui-tui / check` flake +(`ink-resize.test.ts`) from permanently wedging a good PR. + +Other guards: fork PRs never reach the merge path (a `workflow_run` job holds a +write token in the base repo's context); drafts are skipped; a `do-not-merge` +label is an escape hatch that survives all of this; and a PR whose head has +moved since CI ran is left for the newer run to decide. + +### Verification — a decision table, run + +GitHub Actions cannot be executed here, so the step's shell was extracted and +run against a mock `gh` that logs every merge/rerun it is asked to perform: + +| case | action taken | +|---|---| +| gate `success` | **merged** | +| gate `failure`, attempt 1 | re-run requested, no merge | +| gate `failure`, attempt 2 | no re-run, no merge | +| gate `cancelled`, attempt 1 | re-run requested, no merge | +| no gate check run yet | no merge | +| draft PR | skipped | +| `do-not-merge` label | skipped | +| head moved since CI | skipped | +| PR already merged | skipped | + +**The only path that merges is `success`.** Separately verified that the jq +name filter picks the aggregator out of a noisy check-run list, and that a +renamed gate yields an empty conclusion — i.e. it **fails closed**, declining to +merge, rather than failing open. YAML parses, `bash -n` is clean, and the gate +string is asserted equal to `ci.yml`'s `all-checks-pass` job name. + +### Honest limits + +- **This is not branch protection.** It stops *this automation* from merging + red, which is the hole that actually bit us three times. It does not stop a + human merging by hand or pushing straight to `main`. Requiring `All required + checks pass` in the branch rules is the real fence and composes with this. + I could not do that part: this environment has no `gh` CLI or direct GitHub + API, and the MCP GitHub toolset exposes no branch-protection or ruleset + endpoint. If it is ever wanted: *Settings → Branches → add a rule for `main` → + "Require status checks to pass before merging" → search for + `All required checks pass`*. `--auto` then defers to it automatically. +- **The PR carrying this change is itself merged by the old, ungated path**, + because the version of the workflow on `main` is what runs at the time. The + gate applies from the next PR onward. ## 2026-08-12T16:05:00Z — Repair the Python suite (Claude Code) From 6e024be1d91db65af93a3bdff22a05419351a817 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 15:37:15 +0000 Subject: [PATCH 2/3] ci: drop review-labels from the aggregate gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Owner's explicit instruction, once the gate in the previous commit made the aggregator actually block merges. The consequence, stated rather than left to be discovered: a PR that touches CI-sensitive files (workflows, actions, eslint config), changes the MCP catalog, or trips a critical supply-chain finding can now merge with no human having looked at it. That gate asks for the `ci-reviewed` label, and a label is by definition something a person adds, so requiring it would mean every such PR waits for a human — the opposite of what this repo's automation is for. To be precise about what changed: review-labels never blocked anything before this PR either, because nothing was waiting on the aggregator at all — #183 merged with the label gate red. So this is a change in intent, not in effective behaviour. The previous commit would have started enforcing it for the first time; that enforcement is declined up front rather than discovered as friction later. The job still runs and still reports red on the PR, so the signal is intact; it just doesn't block. Restoring it is one line. ci.yml parses, the review-labels job is still defined, and comment-live still lists it in needs — the review comment reads its status, so dropping it from the gate must not drop it from the comment. The gate's evaluate step iterates toJSON(needs) generically, so no other edit is required. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 --- .github/workflows/ci.yml | 16 +++++++++++++++- docs/system-log/2026-08-12.md | 29 +++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1408f942c73a6..d9009c06f05bb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -269,11 +269,25 @@ jobs: - lockfile-diff - docker-lint - supply-chain - - review-labels - osv-scanner # comment-live is a polling job — it doesn't block the gate. # we don't require docker to pass rn because it's so slow lol # - docker + # + # review-labels is deliberately NOT in this list, and the consequence is + # worth stating rather than discovering: a PR that touches CI-sensitive + # files (workflows, actions, eslint config), changes the MCP catalog, or + # trips a critical supply-chain finding can now merge with **no human + # having looked at it**. That gate asks for the `ci-reviewed` label, and a + # label is by definition something a person adds — so requiring it here + # would mean every such PR waits for a human, which is the opposite of + # what this repo's automation is for. + # + # It still runs, and still reports red on the PR, so the signal is not + # gone — it just doesn't block. Removed on the owner's explicit + # instruction, in the same change that made this gate real; before that + # it never blocked anything anyway, because nothing was waiting on the + # aggregator. Put it back by restoring the line. if: always() runs-on: ubuntu-latest timeout-minutes: 10 diff --git a/docs/system-log/2026-08-12.md b/docs/system-log/2026-08-12.md index 7aa0e2194ae88..d0a370335bd12 100644 --- a/docs/system-log/2026-08-12.md +++ b/docs/system-log/2026-08-12.md @@ -89,6 +89,35 @@ string is asserted equal to `ci.yml`'s `all-checks-pass` job name. because the version of the workflow on `main` is what runs at the time. The gate applies from the next PR onward. +### Follow-up, same PR — `review-labels` dropped from the aggregator + +Owner's explicit instruction, once the gate above became real: remove +`review-labels` from `all-checks-pass`'s `needs` so CI-sensitive PRs can merge +unattended. + +**The consequence, stated rather than left to be discovered:** a PR that +touches CI-sensitive files (workflows, actions, eslint config), changes the MCP +catalog, or trips a critical supply-chain finding can now merge with **no human +having looked at it**. That gate asks for the `ci-reviewed` label, and a label +is by definition something a person adds, so requiring it would mean every such +PR waits for a human — the opposite of what this repo's automation is for. + +Worth being precise about what actually changed, though: `review-labels` never +blocked anything *before* this PR either, because nothing was waiting on the +aggregator at all. #183 is the proof — it merged with the label gate red. So +this is a change in intent, not in effective behaviour: the gate above would +have started enforcing it for the first time, and that enforcement is being +declined up front rather than discovered as friction later. + +The job still runs and still reports red on the PR, so the signal is intact — +it just doesn't block. Restoring it is one line. + +Verified: `ci.yml` parses, the `review-labels` job is still defined, and +`comment-live` still lists it in `needs` (the review comment reads its status, +so removing it from the gate must not remove it from the comment). The gate's +evaluate step iterates `toJSON(needs)` generically and has no hard-coded job +names, so dropping an entry needs no other edit. + ## 2026-08-12T16:05:00Z — Repair the Python suite (Claude Code) **Repos touched:** `dizhaky/hermes-agent` From e4e01d713dfab6d4951c01ead2bbbb3df312dd35 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 19:02:32 +0000 Subject: [PATCH 3/3] ci: scope the gitleaks advice notice to the scan step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "What to do on a hit" step was gated on a bare `if: failure()`, which is job-scoped — it fires when checkout or the pinned-gitleaks download fails too. A registry 503 then prints "gitleaks flagged a secret in this diff" over an infrastructure blip, which sends someone hunting a credential that was never there and teaches them to discount the message on the day it is real. Observed on this PR's own CI: a slice failed with four `curl: (22) ... error: 503` retries and exit 22 before any scan ran. Now `if: failure() && steps.scan.outcome == 'failure'`. When an earlier step fails the scan never runs, so its outcome is empty and the advice stays quiet. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 --- .github/workflows/secret-scan.yml | 8 +++++++- docs/system-log/2026-08-12.md | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/.github/workflows/secret-scan.yml b/.github/workflows/secret-scan.yml index 8a1e4cac328bb..a5b963f4ba872 100644 --- a/.github/workflows/secret-scan.yml +++ b/.github/workflows/secret-scan.yml @@ -43,6 +43,7 @@ jobs: gitleaks version - name: Scan for secrets + id: scan shell: bash env: EVENT_NAME: ${{ github.event_name }} @@ -469,8 +470,13 @@ jobs: fi fi + # Scoped to the scan step, not the job. A bare `if: failure()` also fires + # when checkout or the pinned-gitleaks download fails — a registry 503 + # then prints "gitleaks flagged a secret in this diff" over an + # infrastructure blip, sending someone hunting a credential that was + # never there and teaching them to distrust the message when it is real. - name: What to do on a hit - if: failure() + if: failure() && steps.scan.outcome == 'failure' run: | echo "::notice::gitleaks flagged a secret in this diff." echo "If it is real: rotate it first, then remove it from history." diff --git a/docs/system-log/2026-08-12.md b/docs/system-log/2026-08-12.md index d0a370335bd12..15d68dcb39667 100644 --- a/docs/system-log/2026-08-12.md +++ b/docs/system-log/2026-08-12.md @@ -864,3 +864,23 @@ new ordering (flag from callback): 'fast output' ``` Test-only change; `agent/moa_loop.py` untouched. + +### Follow-up, same PR — the gitleaks advice step told the truth only by accident + +`secret-scan.yml` ends with a "What to do on a hit" step gated on +`if: failure()`. That is job-scoped, not step-scoped: it fires when *anything* +earlier in the job fails, including `Checkout code` and the pinned-gitleaks +download. A registry 503 therefore printed **"gitleaks flagged a secret in this +diff"** over an infrastructure blip. + +Not hypothetical — a slice of #186's own CI died exactly this way while this PR +was in flight, with `curl: (22) The requested URL returned error: 503` four +times and exit 22 before any scan ran. Wrong in both directions: it sends +someone hunting a credential that was never there, and it trains the reader to +discount the message on the day it is real. + +Scoped it to the scan step (`id: scan`, `if: failure() && steps.scan.outcome == +'failure'`). When an earlier step fails, the scan never runs and its `outcome` +is empty, so the advice stays quiet. Folded into this PR rather than opened +separately: it is the same subject — a CI signal that doesn't mean what it says +— and the branch was re-running CI regardless.