ci: bound the five jobs #812 left unbounded, including the fifth required context (#844) - #845
ci: bound the five jobs #812 left unbounded, including the fifth required context (#844)#845hkngln wants to merge 1 commit into
Conversation
…ired context (#844) tan-cli#812 bounded parity.yml, which produces four of the five required contexts. The fifth -- `zizmor · workflow security`, the `name:` of ci.yml's `workflow-security` job -- still inherited GitHub's 360-minute job default, as did ci.yml's `python`, `shim` and `wheel-floor` and version-identity.yml's `not-a-released-version`. Measured `grep -c timeout-minutes` on both files at the branch point: 0. ci.yml: python 60, shim 10, wheel-floor 10, workflow-security 10. version-identity.yml: not-a-released-version 10. The `python` cap is set by the RELEASE path, not the PR path, and that is the whole subtlety. Same job, 6-8 min on pull_request/merge_group across six consecutive runs (32049093308, 32047576495, 32046563750, 32044765485, 32039025818, 32030135439) but 23 min on a release tag (run 31834718564, v0.6.0-rc1) -- release.yml:235 calls this workflow with `sdk_parity: true`, which switches on the planner byte-parity and planner-binding suites that skip without an alp-sdk checkout. A cap picked from the PR measurement passes every PR and then fails the tag. 30 would leave that run 30% headroom; 60 keeps it bounded without being a tripwire. The other four measure under a minute on both paths, so 10 is a hang catcher rather than a budget. After this, zero jobs across all seven pull_request-triggered workflows are unbounded -- verified by parsing every file in .github/workflows/ and printing each job's value, not by eye. (parity.yml's own six arrive with #841.) tests/gates: 470 passed, 9 skipped, exit 0. zizmor 1.29.0 --min-severity medium --no-online-audits: exit 0, no findings.
alpCaner
left a comment
There was a problem hiding this comment.
Reviewed against live data — the PR is sound. Every load-bearing claim checks out, and on the merge result there are genuinely zero unbounded jobs in any pull_request-triggered workflow. Notes below are minor; only the first is worth acting on before merge.
Worth doing before merge
This PR closes #844, which is the last open tracker for the deferred follow-up.
The body defers the enforcement gate ("its own change"), but nothing enforces the five new values today: #841's merged gate lists only parity.yml's six jobs by ID in _PARITY_JOBS. gh issue list --repo alplabai/tan-cli --state open --search "timeout-minutes in:title,body" returns only #844. Once this merges, the follow-up has no home.
Suggest filing it first and adding Refs #<new> to the body.
Minor
ci.yml:136 — timeout-minutes: 60 on python is 6.7x the PR-path p100.
Measured across the last 20 ci.yml runs: 6m, 6m, 6m, 7m, 8m x11, 9m x3. So on the >99% of runs that are PRs, a wedged job holds for an hour against a 6-9m norm.
The entire 3x spread is driven by one input this workflow already declares (sdk_parity, ci.yml:65-75, type: boolean, default: false), and timeout-minutes accepts expressions:
timeout-minutes: ${{ inputs.sdk_parity && 60 || 20 }}20 on pull_request/push/merge_group (where inputs is null, so falsy), 60 on the workflow_call from release.yml. The body weighs only the flat 30-vs-60 choice; the conditional gets both.
The "What was unbounded" table's | the other five | — | row.
False at the stated branch point, where parity.yml carried six unbounded jobs (seam1-plan-shape, seam2, first-blink, python-tests-shard, python-tests, notify-planner-drift). The body contradicts itself two sections later ("once #841 brings parity.yml's six"); the commit message gets it right. Moot in effect now that #841 has merged, but the body is what the reader sees.
Eleven jobs remain unbounded outside PR-triggered workflows — release.yml's verify-version, build, release, publish_npm, release_gate, plus python-binaries.yml's four, planner-resync.yml:propose, release-combination.yml:resolve-refs. A wedged release.yml build/release holds a tag for GitHub's 360-minute default, and this repo has already spent a tag that way (v0.5.0-rc3, #319, cited in ci.yml's own comments). The changelog headline is correctly scoped to "PR-triggered workflow", so this is scope-completeness rather than a false claim — but neither the "Deliberately not in this PR" section nor the fragment mentions them.
Nits
ci.yml:127-128(and the body table, andchangelog.d/844.fixed.md:15): "6-8 minutes on apull_request/merge_grouprun" understates the range — runs 32117050171, 32012525215 and 31989462133 each measured 9m. Does not change the chosen bound.ci.yml:134/changelog.d/844.fixed.md:20: "30 would leave that 23-minute run 30% headroom" — 1422s is 23.7m, so 26.5%.- No label, no milestone. Repo practice is mixed here (#842/#837/#833/#832 carry both; #841/#839, both CI-only, carry neither).
Verified
- "including the fifth required context" is exact.
gh api repos/alplabai/tan-cli/branches/{dev,main}/protection->seam1 -- plan-shape parity,python -- pytest across python/ ({ubuntu,windows,macos}-latest)(four fromparity.yml) andzizmor · workflow security, which is thename:ofci.yml:481'sworkflow-securityjob. Four from parity, one from ci.yml. - Count is right against current
dev. Parsing all 11 workflows on thegit merge-tree origin/dev HEADresult (rc=0, no conflict): 0 unbounded PR-triggered jobs, 11 unbounded elsewhere. - The 23-minute release measurement is real — run 31834718564
gates / python= 1422s. Historic release-pathgates / python: 633s, 766s, 794s, 844s, 900s, 1069s, 1422s. A rising trend, so 60 over 30 is the defensible call. - The four 10-minute bounds are hang catchers, not budgets — 40-120x headroom, measured across 10 runs:
shim5-9s,wheel-floor9-16s,zizmor5-9s,not-a-released-version8-11s. - Nothing a hard cancel would skip — no
if: always(),if: !cancelled(),upload-artifact,continue-on-errororactions/cachein either changed file. - No conflicting timeout —
grep -rn -i timeouton both files returns only the five lines this PR adds. release.yml:235really issdk_parity: true.
Gates
| gate | result |
|---|---|
| YAML validity, both changed workflows | parse clean, job lists as expected |
zizmor --min-severity medium --no-online-audits .github/workflows/ @ 1.29.0 (the required context itself) |
rc=0 — No findings to report. Good job! (13 ignored, 54 suppressed) |
pytest tests/gates -q |
rc=0 — 470 passed, 9 skipped |
| #841's merged gate re-rooted at the merge result | rc=0 — 15 passed |
assemble_changelog.py --check |
rc=0 — fixed 844.fixed.md |
| actionlint | not installed on this machine, not run |
Full pytest tests not run — the diff is two workflows plus a changelog fragment, no Python.
One environment note, not a finding against this PR: a first pytest tests/gates run reported 2 failures in test_tan_under_test_guard.py with ModuleNotFoundError: No module named 'pytest' inside the guard's subprocess. Cause is local only — python/tests/conftest.py:625 does monkeypatch.setenv("HOME", ...), which hides this box's user-site pytest from the child interpreter. Re-run with PYTHONPATH set: 470 passed, rc=0. CI installs pytest outside user-site, so it cannot hit this.
Closes #844.
Five jobs across two workflows get a
timeout-minutes. One of them is the fifth required context, which #812 left behind while closing the other four.What was unbounded
grep -c "timeout-minutes"on both files at the branch point (b304e9c) answered 0. Parsing every workflow that runson: pull_request:timeout-minutesci.ymlpython,shim,wheel-floor,workflow-securityversion-identity.ymlnot-a-released-versionworkflow-security'sname:iszizmor · workflow security, one of the five contexts branch protection requires on bothmainanddev. #812 bounded the four that come fromparity.ymland this one was the remainder.The
pythoncap is set by the release path, not the PR pathThis is the part worth reviewing rather than skimming. The same job:
versus, on run 31834718564 — the
v0.6.0-rc1tag:~3x, because
release.yml:235calls this workflow withsdk_parity: true, switching on the planner byte-parity and planner-binding suites that SKIP without an alp-sdk checkout (282 of 1163 tests, perci.yml's own input comment). A cap chosen from the PR measurement passes every PR and then fails the tag. 30 would leave that 23-minute run 30% headroom; 60 keeps it bounded without being a tripwire, on the one path where a false timeout is most expensive.The other four are under a minute on both paths, so 10 is a hang catcher, not a budget.
Values
ci.ymlpythonci.ymlshimci.ymlwheel-floorci.ymlworkflow-securityversion-identity.ymlnot-a-released-versionPlacement follows the house convention — directly after
runs-on:, with a one-line justification (clean-host.yml:213,getting-started.yml:94,release-combination.yml:176).Two corrections to #844's own evidence
Both surfaced while implementing, and both are in the issue's favour:
pin-move-verify.yml. It landed ondevas2cd3e39(feat(pin-move-verify): build the SENDER -- dispatch a proposed tuple to alp-e2e when a PR touches a pin site #823) between my measurement and this branch. It already carriestimeout-minutes: 100on itsverifyjob, so the conclusion is unchanged — but the table said six PR-triggered workflows and there are seven.concurrency:block, keyedpin-move-verify-${{ github.event.pull_request.head.sha || github.event.merge_group.head_sha || github.run_id }}. I checked this specifically because ci(parity): supersede superseded PR runs and bound every job (#812) #841's new gate asserts aconcurrencyblock on everypull_requestworkflow, and a new file without one would have turned ci(parity): supersede superseded PR runs and bound every job (#812) #841 red on its next merge withdev. It does not.After this branch, parsing all seven: zero unbounded jobs, once #841 brings
parity.yml's six.Deliberately not in this PR
#844's follow-up section suggests tightening
test_parity_workflow_concurrency_and_timeouts.pyfrom "parity.yml's jobs by name" to "every job in every PR-triggered workflow". That gate file only exists on #841's branch, which has not merged. Stacking this PR on it would add a merge-order dependency behind a merge queue for no benefit. Once both land, the tightening is a few lines and I will open it as its own change.Test plan
tests/gates— 470 passed, 9 skipped, exit 0 (470 and not 484 because ci(parity): supersede superseded PR runs and bound every job (#812) #841's new gate file is on that branch, not this one)zizmor 1.29.0 --min-severity medium --no-online-audits .github/workflows/— exit 0,No findings to report, CI-pinned versionpull_requestworkflow parsed and each job'stimeout-minutesprinted — verified as data, not by reading the diffpythonat 60 on a release tag; the 23-minute measurement above is why 60 and not 30.