Repository navigation
ci: skip the test jobs on documentation-only pull requests - #15
Conversation
|
Probe run proving the skip path, as promised in How tested. A throwaway documentation-only PR against a base carrying this change (#16) produced run 43017949338:
The positive path is proven by this PR's own run: it changes The probe PR and its |
|
Gate 1 note — the two red checks are not this PR's.
It is fixed by #13, which is now 12/12 green. Once #13 is stacked and this branch is rebased, Gate 1 can be satisfied here. |
d91b4b2 to
cef3a07
Compare
f811a53 to
9c6dacf
Compare
cef3a07 to
17e264a
Compare
9c6dacf to
b25d092
Compare
dafe1bd to
74a648b
Compare
b25d092 to
7a62d7b
Compare
A README or docstring change currently runs the full matrix -- unit, C, notebooks, a 30-minute Docker smoke train and the perf run -- none of which can observe a Markdown edit. On a fork whose process documentation changes often this is the single largest source of runner time, and it is pure waste. A changes job diffs the PR against the merge base and votes code=false only when every changed path is recognised documentation (*.md, docs/, .github/ISSUE_TEMPLATE/). Anything unrecognised, and every push build, votes rue, so the failure direction is always 'run the tests'. The five heavy jobs gain eeds: changes and an if:. Job-level if: rather than workflow-level paths-ignore is deliberate: a workflow skipped by path filtering leaves its required checks pending forever and blocks the merge, whereas a skipped job is reported to branch protection as passing. No third-party action is used; the detection is five lines of git.
7a62d7b to
290ac54
Compare
|
Both blocking findings from the Track B review are fixed in 1. Rename detection could hide a deleted code path
Reproduced the hole and the fix by running the detector's own filter logic against synthetic histories:
2. A failing detector skipped every required job
Now, on all five jobs: if: always() && !(needs.changes.result == 'success' && needs.changes.outputs.code == 'false')Tests are skipped only when the detector ran to success and positively reported This also settles the skip-state evidence question. The new condition's skip set is a strict subset of the old one: both skip exactly when The "tests run" state is demonstrated by this PR's own checks: it touches
|
jdsika
left a comment
There was a problem hiding this comment.
Track B re-review — posted on behalf of the same fresh-context reviewer, re-verifying 290ac54a8 after the two blocking findings were fixed.
Verdict: approve
(a) Syntax and coverage
Valid Actions expression syntax — the ! is inside the expression, not a YAML tag, and always() overrides the implicit success condition. Present on all five heavy jobs (lines 62, 105, 144, 184, 200). None missed.
(b) Case analysis
| Detector outcome | New condition |
|---|---|
success, code=false |
skip |
success, code=true |
run |
| failure | run |
| cancelled | run |
| skipped | run |
| success, empty/unset output | run |
The only conditional skip is a successful detector positively reporting false.
(c) The strict-subset argument
Sound, once the old implicit success() is accounted for: always() removes failure-based skipping but does not override the remaining boolean predicate. The reviewer's sharpening is worth recording — subset membership alone does not prove preservation; what proves it is substituting the historical probe's own (success, false) result into the new expression, which still skips. Its documentation edit also remains code=false under --no-renames. So preservation is established logically; the old run is not empirical execution of the amended YAML, and that distinction is accepted deliberately here.
(d) Detector behaviour
--no-renames exposes both the deletion and the addition path, closing the bypass without introducing new under-detection. Copies keep their added destination; gitlink and mode-only changes on non-documentation paths are testable; a genuinely empty tree diff has no change needing tests. Git command failures now hit the fail-safe gate.
Non-blocking residual (line 48): || true suppresses any grep failure, not only the expected exit code 1 of "everything filtered out". An exit code above 1 — a real grep error — would be read as "documentation only". Accepted as-is for now: the pattern is a fixed literal, so exit codes above 1 are unreachable short of an I/O error. Recorded here rather than fixed so it is not rediscovered.
(e) Scope
No growth. Diffing the old and new workflow blobs shows only --no-renames, the five revised conditions, and the comments explaining them. Still one commit, one file.
forkctl track → B.
What
Add a fast
changesjob toci.ymlthat diffs a pull request against its merge base, and gate thefive heavy jobs — Unit Tests, C Drive Tests, Notebook Execution, Smoke Train Test, Smoke Performance
Test — on it, so a documentation-only pull request skips them.
Why
Every pull request runs roughly 12 minutes of compilation and simulation, including a ~15 minute
smoke train, regardless of what it changed. A large share of the pull requests this fork actually
opens touch nothing but Markdown: process documentation, contributing guidelines, the PR template.
Rebuilding the C extension and running a training smoke test to validate a typo fix costs runner
minutes and wall-clock review latency for zero information.
Why a
changesjob rather thanpaths-ignoreWorkflow-level
paths-ignoreis the obvious approach and it is a trap here. A required check thatis filtered out at workflow level never reports at all, so branch protection leaves it pending
forever and the pull request can never be merged. A job-level
if:that evaluates false reportsskipped, which branch protection treats as passing. Hence the
needs:/if:shape.No third-party action is used — the filter is
git diff --name-onlyagainst the merge base.Why it is conservative
Only
*.md,docs/and.github/ISSUE_TEMPLATE/vote "no code". Anything unrecognised means thefull suite runs. Push builds have no base ref to diff against and always run everything. The
failure mode is therefore "ran tests we did not need", never "skipped tests we did need".
The
|| trueon thegrep -Evis load-bearing:grep -vexits 1 when it filters everything out,which is precisely the documentation-only case, and would otherwise trip
set -e.How tested
pre-commit run --all-filesmake rebuild(required if.c/.hchanged — zero new warnings)make test-unit/make test-evalmake test-c(C sim logic)make test-notebooks(notebooks touched)make test-docker-smoke(dynamics, rewards, observations, or training loop touched)make -C tests/drive perf(hot paths touched)make testbefore marking ready (unit + eval + c + notebooks — does not includedocker-smoke or perf; those are the two boxes above)
This commit changes only
.github/workflows/ci.yml; no Python, C, notebook or config behaviour istouched, so the suites above have nothing to say about it. §4.8's rule for a changed workflow
applies instead: the workflow has to have run, in each state it is supposed to distinguish.
The YAML was parsed to confirm the
pull_requestbranches:filter — which carries the fork-onlyintegration/3.0entry — is untouched by the edit.code=truepath: this pull request itself. It changes.github/workflows/ci.yml, which is nota documentation path, so the
changesjob must print the file and reportcode=true, and all fiveheavy jobs must run normally. See the checks on this PR.
code=falsepath: cannot be exercised by this PR, because this PR is a code change byconstruction. A throwaway documentation-only pull request was opened against this branch to drive
the other branch of the condition, and linked in a comment below. Expected:
changesprintsDocumentation only: skipping the test jobs.and the five jobs report skipped, not pending.New behaviour is covered by a new test / the fix has a test that fails without it:
No unit test — the behaviour is a GitHub Actions condition and the evidence is the runs above.
Stack hygiene
forkctl check-commitpasses (--choreon a fork-only branch)integration/3.0Merge gate
explicitly accepted in the thread
forkctl mirror <slug> --sha HEADpasses, i.e. thecommit cherry-picks onto pure
upstream/3.0and its diff is upstream-cleanGate 3 result:
The cherry-pick keeping the fork-only
integration/3.0branch filter out of the mirror is thespecific thing Gate 3 is checking here, and it passes.
Upstream disposition
Emerge-Lab/PufferDriverequested now — needs a human to verify the finaltitle, body and diff first, and the body must carry the authorship disclosure from
.github/fork/PROCESS.md→ Agent authorshipupstream/3.0andupstream pays the same runner cost, but CI policy is a maintainer preference rather than a
defect, so it should be offered deliberately and with human approval (§5).
chore(fork):change, Gate 3 exemptRisk / notes
Branch protection interaction. The five gated jobs are required checks. Skipped required checks
satisfy branch protection; pending ones do not. If someone later converts this to workflow-level
paths-ignore, documentation PRs become unmergeable. Worth a comment in the file if it ever getsrewritten.
fetch-depth: 0is needed on thechangesjob sogit merge-basehas history to work with.Deliberately out of scope:
install.yml's six-job matrix is not filtered. It is cheaper thanthe test matrix and filtering it is a separate judgement about how much install coverage a docs PR
deserves. Flagged as a follow-up, not done here.
Not filtered either:
pre-commit, which must keep running on documentation — it is what checkstrailing whitespace and end-of-file in Markdown.