Repository navigation
ci: verify the port band above the first binder, not the last - #8
Conversation
build-test.yml ran ci/tools/max-port.sh between 'Run ci/t/' and the runtime suite. The step reads as a guard for the job, but a verifier can only speak for the steps below it, so prove bound TEST_BASE_PORT unverified and the one failure max-port.sh exists to name -- a leftover server on the band, or a band reaching the kernel ephemeral range -- still arrived inside the suite as an unattributed bind error or timeout. Move the step above 'Run ci/t/', and add the ordering rule to lint-ci-ports.sh so the position cannot regress: a job carrying the verifier must run it before the first step that binds, where a binder is the runtime driver, prove, or coverage.sh. Every presence check stayed green in the broken shape -- distinct band, passed through, verifier present -- which is why nothing caught it. Negative controls: new fixtures/policy/verify-after-bind goes red (selftest, all controls held), and reverting the reorder in a scratch copy of the live tree names the real job and both step numbers. max-port.sh itself was re-verified in both directions: exit 1 on a squatted band, exit 1 on a band reaching the ephemeral floor, exit 0 clean. Docs that described the old order as a divergence to work around (README drift list, PROMPT-sync-module 2a) now state the rule instead.
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Self-review of the check added in the previous commit. A bare "prove " substring also matches `improve `, `approve ` and `prover`, so a run block that merely mentions improving something read as a binder sitting above the verifier. Binder matching is now word-bounded. And a single multi-line run block that verifies and then binds has one step index for both, which the index comparison could only read as 'the binder is not below the verifier' -- a finding on correct code. Same-index now falls back to position within the step's text, and only reports when the bind genuinely comes first. Both shapes go into the clean fixture rather than a new one: they are cases the check must stay SILENT on, so they belong in the positive control. Verified by mutation -- restoring the substring match and dropping the same-step branch turns the clean fixture red with exactly these two jobs named.
1 participant
TL;DR
build-test.ymlchecks that its test port band is free — but it did the check after the first test suite had already used the band. So the check could only ever speak for the suite that ran second. Moving it up, and teaching the port linter that a check placed below a binder is not a check.Precisely:
Verify this job's port band is freemoves aboveRun ci/t/, andlint-ci-ports.shgains an ordering rule.The mechanism
test-nginxdeclaresTEST_BASE_PORT: '19200'and runs three things against it:prove -v ci/t/, thenmax-port.sh, thentest_runtime.py.max-port.shis the tool that names two specific failures — a leftover server still listening inside the band, or a band that reaches into the kernel ephemeral range where the allocator will race a listener. Neither is visible from the failure it produces downstream.Sitting between the two suites, it guarded the runtime suite and nothing else.
provebound the band unverified, so both of those failures reached Test::Nginx as a bind error or a timeout with no cause attached — which is exactly the shape the tool exists to remove. The step's own comment claimed it "fails BEFORE the fixture starts", true of the runtime fixture and not ofprove.Every existing check reads the broken shape as correct: the band is declared, it is distinct from
ci-deep.yml's 19400, and it is passed through to the driver. Presence was checked; position was not.The linter rule
A verifier is a property of a position, not of a job, so
check_portsnow compares indices: a job that runsci/tools/max-port.shmust run it before the first step that binds, where a binder is the runtime driver,prove, orcoverage.sh. Non-runsteps keep their slot so the comparison stays an ordering comparison.Scoped to jobs that already carry the verifier. Whether a binding job must carry one at all is a wider question and a separate row — see Out of scope.
Testing
ci/linter/selftest.sh: all controls held, including the newverify-after-bindfixture going red (exit 1) and thecleanfixture staying green. The fixture encodes the exact shipped shape — distinct band, passed through, verifier present, verifier belowprove.git show HEAD~1:.github/workflows/build-test.ymlinto a scratchWORKFLOW_POLICY_ROOTreproduces the finding by name —build-test.yml:test-nginx runs ci/tools/max-port.sh at step 7, after step 6 has already bound the band. Post-fix tree is clean.max-port.shitself re-verified in both directions, since the reorder is only worth anything if the tool goes red: exit 1 with a squatted listener at 19205, exit 1 on a band at 60000 reaching the ephemeral floor (32768 here), exit 0 and19263on the clean band.ci/linter/run-all.shclean on the full tree.The second commit fixes two false positives the check shipped with, both found by self-review before CI: a bare
provesubstring also matchesimprove/approve, and a single multi-linerun:block that verifies then binds has one step index for both, which index comparison could only read as a violation. Both shapes are now cases in thecleanfixture — the check must stay silent on them — and restoring the naive matching turns that fixture red naming exactly those two jobs.Docs
The README drift list and
ci/PROMPT-sync-module.md§ 2a both told a derived module to work around this repo's step order. That instruction had a shelf life of this PR; both now state the rule — put the verifier above the first binder — and cite the old shape as the worked example of getting it wrong.Out of scope
Two adjacent findings, both real, both filed rather than fixed here:
build-test.yml:asanandci-deep.yml:build-flavorsrunprovewith noTEST_NGINX_PORT, so they take Test::Nginx's hardcoded 1984 — three matrix cells in the latter.lint-ci-ports.shdoes not catch it because "runtime-bearing" meanstest_runtime.py, andproveis not it.ci-deep.yml:coveragedeclares 19400 and never verifies it.Both want the linter's declaration check widened to cover
prove, which turns the tree red until those jobs get bands — a different change with a different blast radius.