Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions .github/workflows/maintenance.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 }}
Expand All @@ -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')"

Expand Down
22 changes: 22 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 4 additions & 1 deletion catalog/python-execution.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"}},
Expand Down
101 changes: 100 additions & 1 deletion scripts/check_maintenance_report_contract.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {}
Expand Down Expand Up @@ -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:
Expand Down
Loading