From 0a8de4b846ed14ee144fa029c50ae0bce018bb49 Mon Sep 17 00:00:00 2001 From: Danil Silantyev Date: Sat, 15 Aug 2026 04:39:13 +0500 Subject: [PATCH] fix(maintenance): the sweep ran under `bash -e` and could not report a finding The first real run of maintenance.yml failed, and only the run could show why. GitHub runs `run:` steps as `bash -e {0}`. `-e` arrives on the shell's own command line, where no `set` inside the script can reach it -- so the step's `set -uo pipefail`, and its comment stating that `-e` was deliberately absent, were both wrong. On the first finding bash aborted at the sweep line: `$?` was never read, sweep.txt was never printed, and $GITHUB_OUTPUT never received a status. The report step then had nothing to act on. The mechanism was unreachable in exactly the case it exists for. `shell: bash {0}` fixes it. The report step becomes `if: always()` so a sweep that dies for another reason is still reported, and an empty status is treated as a finding rather than as silence. The contract now resolves the shell the way GitHub does -- step, job defaults, workflow defaults, then `bash -e {0}` -- and executes the sweep step under it with a stub interpreter, asserting the exit code is recorded for a clean sweep and for a finding. Reverting to the default is caught, and so is declaring plain `bash`, which expands to `--noprofile --norc -eo pipefail`. --- .github/workflows/maintenance.yml | 25 ++++- CHANGELOG.md | 22 ++++ catalog/python-execution.yml | 5 +- scripts/check_maintenance_report_contract.py | 101 ++++++++++++++++++- 4 files changed, 149 insertions(+), 4 deletions(-) diff --git a/.github/workflows/maintenance.yml b/.github/workflows/maintenance.yml index b5caf50..b1a51f4 100644 --- a/.github/workflows/maintenance.yml +++ b/.github/workflows/maintenance.yml @@ -100,11 +100,21 @@ jobs: # definition through the API. Unauthenticated that is 60 requests an # hour, which 44 actions exhaust, so the sweep passes its own token. GH_TOKEN: ${{ github.token }} + # `bash {0}` and not the default. GitHub runs `run:` steps as + # `bash -e {0}`, and `-e` arrives on the shell's own command line where + # no `set` inside the script can reach it -- `set -uo pipefail` does not + # remove it. So the step below claimed to tolerate a non-zero sweep and + # did not: on the first finding, bash aborted at that very line, `$?` + # was never read, `sweep.txt` was never printed, and `$GITHUB_OUTPUT` + # never received a status. The reporting mechanism was unreachable in + # exactly the case it exists for, which the first real run demonstrated. + shell: bash {0} run: | set -uo pipefail .venv/bin/python -I -B scripts/check_python_syntax.py - # Deliberately not `set -e`: a finding is the expected outcome and - # must be reported, not abort the job before it can be filed. + # A finding is the expected outcome here and must be reported, not + # abort the job before it can be filed. See the `shell:` note above + # for why saying so in the script alone was not enough. .venv/bin/python -I -B scripts/check_python_execution_contract.py --launch validate_all.py -- --tier scheduled > sweep.txt 2>&1 status=$? cat sweep.txt @@ -117,7 +127,11 @@ jobs: echo '```' } >> "$GITHUB_STEP_SUMMARY" + # A sweep step that dies for any reason must still be reported. Without + # this the job simply ends, and an advisory lane whose findings vanish is + # indistinguishable from one with nothing to say. - name: File or update the tracking issue + if: always() env: GH_TOKEN: ${{ github.token }} SWEEP_STATUS: ${{ steps.sweep.outputs.status }} @@ -126,6 +140,13 @@ jobs: run: | set -euo pipefail title="Maintenance: advisory sweep findings" + # An empty status means the sweep step never reached the line that + # records one. Treat that as a finding rather than as silence: the + # failure mode this whole job guards against is debt nobody meets. + if [ -z "${SWEEP_STATUS}" ]; then + SWEEP_STATUS=1 + printf 'the advisory sweep did not complete; see %s\n' "$RUN_URL" > sweep.txt + fi existing="$(gh issue list --state open --search "$title in:title" \ --json number --jq '.[0].number // empty')" diff --git a/CHANGELOG.md b/CHANGELOG.md index 56ff326..5092f3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,28 @@ ## [Unreleased] +- Let the advisory sweep survive finding something. The first real run of + `maintenance.yml` failed, and the reason was only visible from the run: GitHub + runs `run:` steps as `bash -e {0}`, and `-e` arrives on the shell's own command + line where no `set` inside the script can reach it. The sweep step said + `set -uo pipefail` and carried a comment stating `-e` was deliberately absent. + It was not. On the first finding bash aborted at that very line, so `$?` was + never read, `sweep.txt` was never printed, and `$GITHUB_OUTPUT` never received a + status — leaving the report step nothing to act on. The reporting mechanism was + unreachable in exactly the case it exists for. + + The step now declares `shell: bash {0}`. The report step is `if: always()`, so a + sweep that dies for any other reason is still reported, and a missing status is + treated as a finding rather than as silence — an advisory lane whose findings + vanish looks identical to one with nothing to say. + + `check_maintenance_report_contract.py` now resolves the shell the way GitHub + does — step, then job defaults, then workflow defaults, then `bash -e {0}` — and + executes the sweep step under it with a stub interpreter, asserting the exit + code is recorded for both a clean and a finding sweep. Reverting to the default + shell is caught; so is declaring plain `bash`, which expands to + `--noprofile --norc -eo pipefail` and is equally fatal here. + - Make the advisory sweep able to produce the one output it exists for. The whole justification for moving calendar and network debt off the pull-request path is that findings become a single tracking issue a human meets rather than diff --git a/catalog/python-execution.yml b/catalog/python-execution.yml index 8be7873..cd3527f 100644 --- a/catalog/python-execution.yml +++ b/catalog/python-execution.yml @@ -242,7 +242,10 @@ "_resolve": {"count": 1, "profile": "isolated-python-fixture"} }, "check_gate_contract.py": {"_run": {"count": 1, "profile": "isolated-python-fixture"}}, - "check_maintenance_report_contract.py": {"_run_with_fake_gh": {"count": 1, "profile": "shell-fixture"}}, + "check_maintenance_report_contract.py": { + "_run_sweep": {"count": 1, "profile": "shell-fixture"}, + "_run_with_fake_gh": {"count": 1, "profile": "shell-fixture"} + }, "check_monorepo_routing.py": {"_run": {"count": 1, "profile": "external-tool-fixture"}}, "check_pr_hygiene_contract.py": {"_exercise_validator": {"count": 1, "profile": "shell-fixture"}}, "check_privileged_ref_guard.py": {"_run_guard": {"count": 1, "profile": "shell-fixture"}}, diff --git a/scripts/check_maintenance_report_contract.py b/scripts/check_maintenance_report_contract.py index fcbd457..cb2324e 100644 --- a/scripts/check_maintenance_report_contract.py +++ b/scripts/check_maintenance_report_contract.py @@ -32,8 +32,19 @@ WORKFLOW = REPO_ROOT / ".github/workflows/maintenance.yml" JOB = "sweep" CHECKOUT_STEP = "Checkout" +SWEEP_STEP = "Run the advisory sweep" REPORT_STEP = "File or update the tracking issue" +# How GitHub turns a `shell:` value into a command line. The default matters +# most: an unspecified shell is `bash -e {0}`, and `-e` arrives on the shell's +# own argv where no `set` inside the script can reach it. +DEFAULT_SHELL = "bash -e {0}" +NAMED_SHELLS = { + "bash": "bash --noprofile --norc -eo pipefail {0}", + "sh": "sh -e {0}", + "python": "python {0}", +} + FAKE_GH = """#!/usr/bin/env bash set -u printf '%s\\n' "$*" >> "$FAKE_GH_LOG" @@ -107,6 +118,93 @@ def _run_with_fake_gh(script: str, mode: str, env: dict[str, str]): return done, log.read_text(encoding="utf-8") +def _shell_argv(step: dict, job: dict, doc: dict) -> list[str]: + """The argv GitHub would actually run this step's script under.""" + declared = ( + step.get("shell") + or ((job.get("defaults") or {}).get("run") or {}).get("shell") + or ((doc.get("defaults") or {}).get("run") or {}).get("shell") + ) + template = NAMED_SHELLS.get(str(declared), str(declared)) if declared else DEFAULT_SHELL + return str(template).split() + + +def _run_sweep(argv: list[str], script: str, sweep_exit: int) -> tuple[int, str, str]: + """Execute the sweep step with a stub interpreter, under the real shell.""" + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + venv = root / ".venv" / "bin" + venv.mkdir(parents=True) + stub = venv / "python" + # Exits 0 for the cold syntax gate and `sweep_exit` for the sweep, so the + # case under test is "the sweep reported a finding", not "python broke". + stub.write_text( + "#!/usr/bin/env bash\n" + 'case "$*" in *check_python_syntax.py*) exit 0 ;; esac\n' + f"exit {sweep_exit}\n", + encoding="utf-8") + stub.chmod(0o755) + (root / "scripts").mkdir() + output = root / "github_output" + output.write_text("", encoding="utf-8") + summary = root / "github_step_summary" + summary.write_text("", encoding="utf-8") + program = root / "step.sh" + program.write_text(script, encoding="utf-8") + done = subprocess.run( + [*(str(program) if part == "{0}" else part for part in argv)], cwd=root, + env=clean_environment({ + "PATH": "/usr/bin:/bin", + "GITHUB_OUTPUT": str(output), + "GITHUB_STEP_SUMMARY": str(summary), + "GH_TOKEN": "fake", + }), + capture_output=True, text=True, timeout=60) + return done.returncode, output.read_text(encoding="utf-8"), done.stdout + + +def _sweep_problems() -> list[str]: + """The sweep must record its own exit code, including when it finds something. + + This is the case the first real run of the workflow failed on, and no amount + of reading the script could have shown it: the script said `set -uo pipefail` + and a comment said `-e` was deliberately absent, while GitHub was running the + whole thing under `bash -e`. On the first finding bash aborted at the sweep + line, so `$?` was never read and the report step never had a status to act on. + """ + doc = load_yaml(WORKFLOW) + job = (doc.get("jobs") or {}).get(JOB) or {} + try: + step = _step(SWEEP_STEP) + except ValueError as exc: + return [str(exc)] + script = str(step.get("run") or "") + argv = _shell_argv(step, job, doc) + problems: list[str] = [] + for sweep_exit in (0, 1): + code, output, stdout = _run_sweep(argv, script, sweep_exit) + if f"status={sweep_exit}" not in output: + problems.append( + f"{WORKFLOW.name}: {SWEEP_STEP!r} did not record `status={sweep_exit}` " + f"when the sweep exited {sweep_exit} (shell: {' '.join(argv)}); " + f"step exit {code}, GITHUB_OUTPUT {output.strip()!r}") + return problems + + +def _always_problems() -> list[str]: + try: + step = _step(REPORT_STEP) + except ValueError as exc: + return [str(exc)] + if str(step.get("if") or "").strip() not in {"always()", "${{ always() }}"}: + return [ + f"{WORKFLOW.name}: {REPORT_STEP!r} must be `if: always()`; a sweep step " + "that dies for any reason would otherwise skip reporting entirely, and " + "an advisory lane whose findings vanish looks exactly like one with " + "nothing to say"] + return [] + + def _checkout_problems() -> list[str]: try: options = _step(CHECKOUT_STEP).get("with") or {} @@ -173,7 +271,8 @@ def _report_problems() -> list[str]: def check() -> list[str]: - return _checkout_problems() + _report_problems() + return (_checkout_problems() + _sweep_problems() + + _always_problems() + _report_problems()) def main() -> int: