From ceb425e0d7ac33b91f35ddbef7fe1064d539f874 Mon Sep 17 00:00:00 2001
From: Jammy2211 <JNightingale2211@gmail.com>
Date: Wed, 26 Aug 2026 13:23:13 -0400
Subject: [PATCH] fix(sweep): stop repo_settings.yml aborting silently under
 bash -e
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

GitHub runs a `run:` block as `bash -e {0}`. The block set `-uo pipefail` and
never disabled `-e`, so the per-repo read

    before=$(gh api "repos/$slug" --jq '.delete_branch_on_merge' 2>/dev/null)

aborted the entire script the moment one repo was unreadable — and silently,
because that stderr is discarded. The `case "$before" in ... *)` unreadable
branch, and with it the in-org/out-of-org failure-policy split #292 added, could
never execute. The first real dispatch failed in 20s with no output at all.

A second instance was latent: `[ "$hard" -eq 1 ] && failed=1` ends `visit()` in
two branches, and with hard=0 that compound returns 1, so the function returned
1 and `-e` aborted on the first out-of-org repo. Both are now `if` blocks.

`-e` is disabled explicitly rather than with `|| true` on the read, because the
hazard applies to every guarded command in the block.

The tests are the real fix. #292 was control-tested across seven scenarios and
still shipped this, because the harness ran `bash <script>` while GitHub runs
`bash -e <script>` — it validated a shell mode that never executes. The new
tests run the extracted block exactly as the runner does, and were verified to
FAIL against the pre-fix workflow and pass against this one.

Sibling sweep: branch_archive.yml and branch_sweep_all.yml explicitly `set -e`
and discard no stderr, so abort-on-error is intended there; no other run: block
in this repo combines a swallowed failure with an inherited -e.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EqUT9Wc6mVLiJmbcstT7Z3
---
 .github/workflows/repo_settings.yml  |  12 +-
 tests/test_repo_settings_workflow.py | 176 +++++++++++++++++++++++++++
 2 files changed, 186 insertions(+), 2 deletions(-)
 create mode 100644 tests/test_repo_settings_workflow.py

diff --git a/.github/workflows/repo_settings.yml b/.github/workflows/repo_settings.yml
index 3908d776..aa3c88fd 100644
--- a/.github/workflows/repo_settings.yml
+++ b/.github/workflows/repo_settings.yml
@@ -84,6 +84,14 @@ jobs:
           MODE: ${{ inputs.mode }}
         run: |
           set -uo pipefail
+          # GitHub runs this block as `bash -e`. That is wrong for this script:
+          # its whole design is to survive a per-repo failure and REPORT it, and
+          # the read below sends stderr to /dev/null, so under -e the first
+          # unreadable repo aborted the run with no output at all — the
+          # unreadable branch and the in-org/out-of-org split never executed.
+          # Disabled explicitly rather than with `|| true` on one line, because
+          # the hazard applies to every guarded command here, not just that one.
+          set +e
           mode="${MODE:-apply}"
 
           if [ -z "${GH_TOKEN:-}" ]; then
@@ -135,7 +143,7 @@ jobs:
               *)
                 echo "::warning::$slug — could not read the setting (repo missing, or the PAT cannot see it)"
                 echo "| \`$slug\` | $scope | ? | **unreadable** |" >> "$GITHUB_STEP_SUMMARY"
-                [ "$hard" -eq 1 ] && failed=1
+                if [ "$hard" -eq 1 ]; then failed=1; fi
                 return ;;
             esac
 
@@ -156,7 +164,7 @@ jobs:
               # imply. Widening a PAT is a human decision, not a retry.
               echo "::warning::$slug — PATCH refused; the PAT needs admin on this repo"
               echo "| \`$slug\` | $scope | off | **refused (needs admin)** |" >> "$GITHUB_STEP_SUMMARY"
-              [ "$hard" -eq 1 ] && failed=1
+              if [ "$hard" -eq 1 ]; then failed=1; fi
             fi
           }
 
diff --git a/tests/test_repo_settings_workflow.py b/tests/test_repo_settings_workflow.py
new file mode 100644
index 00000000..d580b76f
--- /dev/null
+++ b/tests/test_repo_settings_workflow.py
@@ -0,0 +1,176 @@
+"""tests/test_repo_settings_workflow.py — the settings sweep's shell block.
+
+**Why this file exists.** The sweep was control-tested across seven scenarios
+before it shipped, and still failed on its first real dispatch. The harness ran
+`bash <script>`; GitHub runs `bash -e {0}`. Under `-e` the per-repo read
+
+    before=$(gh api "repos/$slug" ... 2>/dev/null)
+
+aborts the whole script the moment one repo is unreadable — silently, since its
+stderr is discarded — so the unreadable branch and the in-org/out-of-org failure
+split were dead code in production while passing every local test. A control
+test that does not reproduce the caller's invocation proves nothing about the
+caller, so every test here runs the block **exactly as GitHub does**.
+
+Fixtures use invented repo names: this is a `.py` file under a framework organ,
+so the tenant firewall scans it.
+"""
+
+from __future__ import annotations
+
+import os
+import shutil
+import subprocess
+import textwrap
+from pathlib import Path
+
+import pytest
+import yaml
+
+BRAIN_HOME = Path(__file__).resolve().parents[1]
+WORKFLOW = BRAIN_HOME / ".github" / "workflows" / "repo_settings.yml"
+
+# GitHub's default for a `run:` block with no `shell:` key. Hardcoded rather
+# than read from the workflow because it is a property of the *runner*, and the
+# point of these tests is to pin the runner's behaviour, not to echo the file.
+GITHUB_SHELL = ["bash", "-e"]
+
+
+def _step() -> dict:
+    doc = yaml.safe_load(WORKFLOW.read_text())
+    for st in doc["jobs"]["settings"]["steps"]:
+        if "Read or set" in str(st.get("name", "")):
+            return st
+    raise AssertionError("the per-repo step is gone — this test needs repointing")
+
+
+def test_the_step_does_not_override_the_shell():
+    """If it ever sets `shell:`, GITHUB_SHELL above stops being the truth."""
+    assert "shell" not in _step(), "step now sets shell: — update GITHUB_SHELL"
+
+
+def _harness(tmp_path: Path, gh_body: str) -> Path:
+    """A cwd the block can run in: stub `gh`, a body map, the real derivation."""
+    binv = tmp_path / "stubbin"
+    binv.mkdir()
+    gh = binv / "gh"
+    gh.write_text("#!/usr/bin/env bash\n" + textwrap.dedent(gh_body))
+    gh.chmod(0o755)
+
+    (tmp_path / "bin").mkdir()
+    shutil.copy(BRAIN_HOME / "bin" / "branch_sweep_targets.py", tmp_path / "bin")
+
+    (tmp_path / ".mind").mkdir()
+    (tmp_path / ".mind" / "repos.yaml").write_text(
+        yaml.safe_dump(
+            {
+                "repos": {
+                    "Lib": {"github": "Org/Lib", "category": "library"},
+                    "Mine": {"github": "Person/Mine", "category": "admin"},
+                }
+            },
+            sort_keys=False,
+        )
+    )
+    (tmp_path / "script.sh").write_text(_step()["run"])
+    return binv
+
+
+def _run(tmp_path: Path, gh_body: str, mode: str = "audit"):
+    binv = _harness(tmp_path, gh_body)
+    env = {
+        **os.environ,
+        "PATH": f"{binv}{os.pathsep}{os.environ['PATH']}",
+        "GH_TOKEN": "stub",
+        "ORG": "Org",
+        "MODE": mode,
+        "GITHUB_STEP_SUMMARY": str(tmp_path / "summary.md"),
+    }
+    (tmp_path / "summary.md").write_text("")
+    proc = subprocess.run(
+        [*GITHUB_SHELL, "script.sh"],
+        cwd=tmp_path,
+        env=env,
+        capture_output=True,
+        text=True,
+    )
+    return proc, (tmp_path / "summary.md").read_text()
+
+
+# The org listing every scenario shares; `$1 $2 …` is the gh argv.
+_LIST = '''
+    case "$*" in
+      *"orgs/Org/repos"*) printf 'Org/Alpha\\nOrg/Beta\\n'; exit 0 ;;
+'''
+
+
+def test_an_unreadable_in_org_repo_is_reported_not_silently_fatal(tmp_path):
+    """The regression: under -e this aborted with no output at all."""
+    proc, summary = _run(
+        tmp_path,
+        _LIST
+        + '''
+      *"repos/Org/Beta"*) exit 1 ;;
+      *) echo true; exit 0 ;;
+    esac
+    ''',
+    )
+    assert "::warning::" in proc.stdout, f"silent abort regressed:\n{proc.stdout!r}"
+    assert "Org/Beta" in proc.stdout
+    assert "unreadable" in summary
+    assert "Org/Alpha" in summary, "aborted before finishing the sweep"
+    assert proc.returncode == 1, "an in-org failure must still fail the job"
+
+
+def test_an_unreadable_out_of_org_repo_warns_but_keeps_the_job_green(tmp_path):
+    """A personal-account repo the org PAT cannot admin must not redden the schedule."""
+    proc, summary = _run(
+        tmp_path,
+        _LIST
+        + '''
+      *"repos/Person/Mine"*) exit 1 ;;
+      *) echo true; exit 0 ;;
+    esac
+    ''',
+    )
+    assert "::warning::" in proc.stdout
+    assert "Person/Mine" in proc.stdout
+    assert proc.returncode == 0, "an out-of-org failure must not fail the job"
+
+
+def test_a_refused_patch_out_of_org_keeps_the_job_green(tmp_path):
+    """`hard=0` on the PATCH branch — the other trailing-compound instance."""
+    proc, summary = _run(
+        tmp_path,
+        _LIST
+        + '''
+      *"-X PATCH"*) case "$*" in *Person/Mine*) exit 1 ;; esac; exit 0 ;;
+      *) echo false; exit 0 ;;
+    esac
+    ''',
+        mode="apply",
+    )
+    assert "refused (needs admin)" in summary
+    assert proc.returncode == 0
+
+
+def test_all_already_on_is_green_and_sweeps_every_repo(tmp_path):
+    proc, summary = _run(tmp_path, _LIST + '''
+      *) echo true; exit 0 ;;
+    esac
+    ''')
+    assert proc.returncode == 0, proc.stdout + proc.stderr
+    for slug in ("Org/Alpha", "Org/Beta", "Person/Mine"):
+        assert slug in summary, f"{slug} missing — the sweep stopped early"
+
+
+def test_an_empty_org_listing_refuses_rather_than_reporting_a_clean_sweep(tmp_path):
+    """Losing org read must not look like 'nothing to do'."""
+    proc, _ = _run(tmp_path, '''
+    case "$*" in
+      *"orgs/Org/repos"*) exit 0 ;;
+      *) echo true; exit 0 ;;
+    esac
+    ''')
+    assert proc.returncode == 1
+    assert "listed no repos" in proc.stdout