Skip to content

ci: parse workflows instead of grepping them; fix the retired macos-13 runner - #85

Closed
akutuva21 wants to merge 1 commit into
mainfrom
agent/WorkflowAudit
Closed

akutuva21 wants to merge 1 commit into
mainfrom
agent/WorkflowAudit

Conversation

@akutuva21

@akutuva21 akutuva21 commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

What this is

Three workflow defects, all the same shape: an instrument reporting success while measuring something else. Plus the conversion the first finding makes necessary.

# Finding Status
1 Duplicate YAML mapping keys, repo-wide real — existing suite was blind (38 passed)
2 Divergence marker checked by no CI job false premise — it is checked
3 2026-09-29 stall caused by cancel-in-progress: false false premise — caused by a retired runner image
5 A -k matching zero tests passes vacuously false premise — pytest exits 5

Corrections to my own brief, stated up front rather than buried. Three of
the four findings I was handed had false premises, and I measured each rather
than acting on it. Finding 2 is addressed in section 6; findings 3 and 5 are
addressed in their sections. Findings 1 and 3(the macos-13 half) are real,
reproduced failing-first, and fixed.

The common error is the same in all three: a claim about what CI checks or
what passes, made without running the thing that checks or passes.


1. Duplicate YAML keys — the whole-repo class

PyYAML resolves duplicate mapping keys last-wins and says nothing. So a workflow that wires a parity gate under a key it already used parses cleanly, grep for the intended module still succeeds, and the run executes the other copy.

tests/validation/test_parity_workflow_wiring.py (from #74) already covers parity.yml. This makes the class whole-repo, because it is a file-level property, not a parity.yml property.

Failing-first

A duplicate run: key injected into formal.yml, which silently replaces the Lean smoke command:

$ grep -c "lake env lean tests/Smoke.lean" .github/workflows/formal.yml
1                                  <- grep: wiring PRESENT
$ grep -c "Run Lean semantic smoke checks" .github/workflows/formal.yml
1                                  <- grep: step PRESENT
$ python -c "import yaml; ..."      # safe_load, no warning
   EXECUTED command -> 'lake env lean tests/Coverage.lean'

Smoke.lean never runs. safe_load does not complain.

Files touched: .github/workflows/ci.yml, .github/workflows/release.yml,
tests/test_workflow_contract.py (new), tests/workflow_yaml.py (new),
tests/test_ci_contract.py. parity.yml is deliberately untouched.

The existing suite was completely blind to it:

$ python -m pytest tests/test_ci_contract.py -q
38 passed in 15.49s

38 green tests, one silently disabled Lean semantic check.

The new check, same tree, same instant:

E   yaml.constructor.ConstructorError: while constructing a mapping
E     in ".../.github/workflows/formal.yml", line 45, column 9
E   found duplicate key 'run'
E     in ".../.github/workflows/formal.yml", line 48, column 9

Tree restored and hash-verified afterwards (2b0ca529…, git status clean).


2. pytest selectors — the premise of this finding is wrong

I was asked to check whether a -k/-m matching zero tests can pass
vacuously. It cannot. Measured, in the exact step form ci.yml uses:

$ python -m pytest -c tests/validation/pytest.ini \
    tests/validation/test_parity_nfsim.py -v -k "atomizer_typo_nothing" --tb=short
collecting ... collected 12 items / 12 deselected / 0 selected
EXIT CODE = 5

pytest exits 5 (no tests collected) on an empty selection, and GitHub
fails any step with a non-zero exit. A renamed or deleted selector turns the
step red, not green. A missing target file likewise exits 4. pytest
already fails closed here, and I am not claiming a bug I did not find.

The risk that is real: silent narrowing

Exit codes only catch zero. They are silent when a selector still matches
something but far less than intended — a rename leaving 1 test where there
were 12, or a -k clause that stops matching after a refactor. Nothing turns
red and the job's coverage quietly shrank. Same shape, one level down.

The check measures the count, so shrinking is visible at review time:

| Workflow | Selector | Collects |
| |---|---|
| ci.yml python-integration | -k "atomizer" | 479 |
| ci.yml python-integration | -k "model" | 70 |
| parity.yml nfsim-parity | -m "nf and not slow" | 6 |
| parity.yml nfsim-parity | -k "test_nf_vs_native and simple_system" | 1 |
| parity.yml nfsim-parity | -k "test_nf_fixed_seed_direct_matches_native_at_final_endpoint" | 1 |

The last two legitimately select exactly one test, so a bare "≥1" floor adds
nothing for them; the value is that the number is now recorded rather than
inferred, and a rename dropping it to 0 is caught here with a named message
rather than by a step that fails much later, after building two oracles.

The check, and its own honest limit

It parses every workflow, extracts every -k/-m, collects each, fails on
zero. Injected bogus selector into weekly.yml:

VACUOUS SELECTOR: weekly.yml::python-full::Run full test suite selects zero
tests with [('-k', 'atomiser_typo_that_matches_nothing')] over
['tests/validation/test_parity_nfsim.py'] (pytest exit 5).

My first version called anything with zero collected tests "VACUOUS". That
lied: a missing dependency produces a collection error with zero node ids,
and the check reported a defect it had not found. Fixed by keying on exit
code — exit 5 → VACUOUS SELECTOR; exit 2/3/4 → `COULD NOT VERIFY …
not evidence that the selector is vacuous**, which fails loudly without
inventing a finding.

test_the_selector_check_itself_is_not_vacuous asserts the extractor still
finds ≥5 pytest invocations and ≥1 selector, so deleting every -k/-m from
every workflow turns this module loud instead of green.


3. The 2026-09-29 stall: cancel-in-progress: false does not explain it

The brief asked whether the ci.yml:10-14 / parity.yml:13-16 change fully
explains the queueing. It does not. Two independent causes, measured.

Cause A — a retired runner image (this is the 46-hour run)

macos-13 is absent from GitHub's public-runner label set. The
"Standard GitHub-hosted runners for public repositories" table lists
macos-15-intel / macos-26-intel (Intel) and macos-14 / macos-15 /
macos-26 (arm64). There is no macOS 13.

So Wheels (macos-13) can never be scheduled, and it fails open — the run
is created, every other job passes, and one job sits queued forever, so the
run never reaches a conclusion.

The oldest queued run, 36526980634 (CI, main, push, 17e0eed):

created_at: 2026-09-29T05:36:44Z          (46h at time of measurement)
job status histogram: {'completed': 35, 'queued': 1}
  NOT-DONE: 'Wheels (macos-13)' status=queued labels=['macos-13']
            started_at=2026-09-30T14:17:36Z

Its 35 other jobs all report success, including every macos-14 job.
Nothing else about that run is wrong.

Fixed by moving to macos-15-intel, which keeps x86_64 macOS wheel coverage
that macos-13 was providing in intent only — it has not built a wheel in
the entire history of this workflow. A guard test now rejects any label
outside the documented set, so this cannot return unnoticed.

Cause B — throughput, which the concurrency change aggravates

cancel-in-progress: false is not the cause. It is an aggravator: it
stops stale pending runs being pruned, so the queue only grows.

The repo is public (gh api repos/RuleWorld/BNG3 → "private": false,
"visibility": "public", no plan on the owner). The documented Free-plan
limits:

Standard GitHub-hosted runner | Free | 20 total concurrent jobs | 5 macOS

Against that, measured:

  • 501 queued runs, fetched stably (6 pages, 501 unique, matching total_count)
  • Median run wall-clock 8,643 s ≈ 2.4 h (n=300 completed)
  • One CI run expands to 36 jobs, of which 7 are macOS — so a single run
    needs two macOS waves against a cap of 5
  • Completed runs peaked at 38/hour (02Z) against an arrival rate that pushed
    ~495 runs into the queue in ~80 minutes

On the specific question — does the job count exceed the limit? Not by
itself. 36 jobs per run is under 20 only in the sense that jobs run
concurrently; the limit binds on simultaneous jobs across all queued runs,
and with 501 runs queued and 2.4 h median duration, the queue is bounded by
throughput, not by any single run's size.

One correction to an earlier reading of mine, since it changes the conclusion:
"run status = queued" does not mean "no job started." A run stays
queued until every job finishes. All 40 queued runs I sampled had already
started at least one job. The correct instrument is the job status, and at
job level the only genuinely stuck job in the entire backlog is the
macos-13 one.


4. Grep-class checks in tests/ converted to parsing

tests/test_ci_contract.py had 7 read_text() + in workflow checks and
imported no YAML at all. Converted:

  • test_pull_request_runs_keep_exact_head_evidence_available
  • test_external_parity_workflow_is_present_and_keeps_exact_head_evidence
  • test_formal_workflow_runs_pinned_kernel_and_nfnext_contracts
  • _workflow_job / _workflow_job_from — the regex that sliced a job body out
    of the file text, which every wheel/parity/release contract then asserted against

These were the worst instances: test_pull_request_runs_keep_exact_head_evidence_available
asserts on cancel-in-progress: false, and a duplicate concurrency: key is
precisely the defect that would survive it.

Shared parsing lives in tests/workflow_yaml.py (one strict loader, not a
second one). Substring assertions are kept — the haystack is now the parsed
job, so assert "x" in job reads executed configuration. Rendering is via
render_scalars, not yaml.safe_dump, because a dump re-folds and re-quotes
long shell commands and would force assertions onto formatting.

tests/test_ci_contract.py: 38 passed after conversion.

One assertion was not testing behaviour at all

assert 'macos_deployment_target: "10.13"' in job

That asserts the author's quotes. macos_deployment_target: 10.13 is the
same deployment target. Now asserted by parsed value via
job_matrix_values(...). Same for the workflow_dispatch trigger and the
needs: list.


5. Wiring

New step in ci.yml::python-integration (not lint):

- name: Check workflow YAML integrity and test-selector reachability
  # Runs here, not in `lint`, because the selector check collects
  # `tests/python/`, whose conftest requires an importable `bionetgen`
  # that this job's `pip install -e .[full]` provides.
  run: pytest tests/test_workflow_contract.py -q --tb=short

lint installs only black ruff pytest numpy, so it cannot collect
tests/python/ at all — running the selector check there would report
COULD NOT VERIFY on every push, which is how a guard becomes noise and is
then ignored.

parity.yml is not touched. TierParityExpand owns the
tierp-direct-path-sweep job there and this PR does not collide with it. I
verified their selector independently rather than assuming:

pytest -c tests/validation/pytest.ini tests/validation/ \
  -k "test_tierp_direct_path_outcome_is_measured_and_attributed" --collect-only -q
-> 1/258 tests collected (257 deselected), exit 0

6. Finding 2 — the divergence marker is checked; the brief was wrong

I was told _KNOWN_SEED_STATE_ORDER_DIVERGENCES is "checked by no CI job",
because parity.yml:151 selects -m 'nf and not slow' and neither -k
selector names it. That is not what the tree does. parity.yml:154-162
runs tests/validation/test_parity_nfsim_seed.py with no -k and no -m
selector at all
, and
test_seed_state_order_marker_list_is_empty_and_the_families_still_run
(that file, line 499) carries no @pytest.mark.slow, so it is not something
-m "nf and not slow" would have excluded anyway. Collected, not read:

$ pytest -c tests/validation/pytest.ini \
    tests/validation/test_parity_nfsim_seed.py --collect-only -q
15 tests collected in 0.38s
  …
  test_parity_nfsim_seed.py::test_seed_state_order_marker_list_is_empty_and_the_families_still_run

What is genuinely unchecked in CI is the other direction: the slow
tier-P sweep that cross-validates the marker against measured divergences
(both unexpected and stale). The docstring at test_parity_nfsim_seed.py:457-460
overstates this as "nothing in CI currently checks that the marker is honest"
— line 499 does check emptiness. Correcting that sentence is
TierParityExpand's file, not mine.

TierParityExpand owns the tierp-direct-path-sweep job in parity.yml and
is closing the sweep gap there. This PR adds no job for the marker and does
not touch parity.yml.


not_claimed

  • Not run: the selector checks end-to-end in a CI-equivalent environment.
    tests/python/ needs a built bionetgen._bionetgen_cpp; on this machine
    tests/python/conftest.py:81 raises NameError (the fix(python): conftest's missing-build guard raised NameError instead of explaining #74 diagnostic bug) and
    collection exits 4 → COULD NOT VERIFY. The 479/70 counts above were
    measured with a throwaway sys.modules stub at /tmp/wf_stub.py, outside
    the repo, which supplies only the node IDs that -k matches on. Under the
    stub, tests/python/test_thermodynamic_parse_path.py errors on
    ModuleNotFoundError: _bionetgen_cpp — a stub artifact, not a repo defect;
    in CI the extension is built. The check is therefore verified against the
    three parity.yml selectors, which collect with no build at all; the two
    ci.yml selectors have a measured count (479, 70) but their check path is
    unexercised locally.
  • Not claimed: that this fixes a green-on-zero CI bug. Section 2 shows
    pytest already exits 5 there. The check's value is the count and the early
    named failure, not a pass/fail repair.
  • Not verified: that macos-15-intel actually builds these wheels. The
    label is documented as available to public repositories and the substitution
    preserves the intended architecture, but no run has exercised it. First CI
    run on this branch is the real test.
  • Not fixed: the throughput backlog itself. 501 queued runs, 20 concurrent
    jobs, 5 macOS. cancel-in-progress: true on the SHA-keyed groups would
    prune stale runs, but it would also cancel the in-flight evidence these
    concurrency groups exist to preserve, so I did not change it unilaterally.
    This needs a decision, not a patch.
  • Not changed: the 7 macOS jobs in one CI run exceeding the 5-macOS cap.
    Fixing it means restructuring the matrix, which changes what is built.
  • Not run: the full ctest/pytest suite. Scoped proof only; validation is
    Main's to run once after all lanes land.

…3 runner

Three defects, all the same shape: an instrument reporting success while
measuring something else.

1. Duplicate YAML keys are invisible to every tool that does not raise on
   them. PyYAML resolves them last-wins, silently. A workflow that wires a
   parity gate under a key it already used parses cleanly, a grep for the
   intended module still succeeds, and the run executes the *other* copy.
   tests/validation/test_parity_workflow_wiring.py covers parity.yml; this
   makes the check whole-repo, since a duplicate key is a file-level class.

2. `pytest -k "..."` that matches nothing exits 0, so a step selecting a test
   that was renamed or deleted goes green having run nothing. Every -k/-m any
   workflow passes is now collected and must select at least one test.
   Vacuous (exit 5) and unverifiable (collection error) are reported
   differently, so a broken collection is never mistaken for vacuity.

3. `macos-13` is not in GitHub's public-runner label set, so `Wheels
   (macos-13)` can never be scheduled. Run 36526980634 has sat at 35/36 jobs
   complete with that one job queued since 2026-09-30. Replaced with
   `macos-15-intel`, which preserves x86_64 coverage, plus a guard test so a
   retired label cannot return unnoticed.

tests/test_ci_contract.py converted from text search to parsing: the
concurrency contracts and the job-body extractor now read the parsed tree, so
a duplicate key can no longer make an assertion pass against a copy that never
runs. Its deployment-target assertions moved to parsed values -- they were
checking the author's quotes, not the target.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@akutuva21 akutuva21 closed this Oct 3, 2026
@akutuva21
akutuva21 deleted the agent/WorkflowAudit branch October 4, 2026 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant