Skip to content

perf(ci): parallelise the plugin contract suites and defer the hook-utils clock benchmarks - #3693

Merged
kyle-sexton merged 3 commits into
mainfrom
ci-perf/phase-2-scripts
Sep 3, 2026
Merged

perf(ci): parallelise the plugin contract suites and defer the hook-utils clock benchmarks#3693
kyle-sexton merged 3 commits into
mainfrom
ci-perf/phase-2-scripts

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

No related issue: Phase 2 of the CI/CD performance program tracked at melodic-software/github-iac#385 (cross-repository, so no closing keyword applies here)

Summary

Two blocks dominate this repository's pull-request wall time. plugin-gate runs the whole contract corpus serially: 572 seconds of mostly spawn-bound work on a runner with four vCPUs. hook-utils-sync spends about 106 of its seconds on two interleaved-pair clock comparisons that measure the machine, not the code.

Both are addressed test-side. No plugin production code, no workflow topology and no gate is changed; the consolidation of ci.yml itself is a separate pull request.

Measured on this pull request's own ubuntu-24.04 runs, taken from the Run plugin contract tests step on both sides so the comparison is step against step:

Contract corpus Run Step wall time Result
serial (baseline) 33656901629 572 s green
--jobs 4 33787030286 228 s red
--jobs 4 33794273916 240 s red
--jobs 3 33797359110 312 s green

--jobs 3 ships: it is 1.8x faster than serial and it is the only parallel setting that has gone green. Four is quicker still but has never produced a green run, for the reason in the next section.

Fix

Parallel contract runner. scripts/run-plugin-tests.sh gains --jobs N (default 1, or PLUGIN_TEST_JOBS); ci.yml's plugin-gate passes --jobs 3. The default stays 1 because a parallel run on Windows Git Bash measured sublinear, saturating on process creation, so a developer box is not silently slowed.

Why three and not four: at four, three separate suites failed across two runs, and none of them has a clock in it. In every case an external command produced empty output on a path that is deterministic serially and locally, an awk read of an agent markdown file, a jq read of a local-markdown fixture, and a conformance run. That is one symptom with three faces rather than three flaky suites, and serialising each as it appeared would have grown the allowlist by a suite per run while hiding the cause. The job count is the lever. Root cause is tracked in #3694.

Suites that assert wall-clock ceilings or drive concurrency probes of their own fail spuriously under load. Eighteen are named in the new scripts/run-plugin-tests-serial.txt and run one at a time, before the parallel group, never overlapping anything: the guardrails hook suites, the two statusline-tee copies, the performance and rate-limit benchmarks, the five always-on hook suites that assert the latency budget, the listing-budget check, and two more deadline-based cases (a 2,000 ms spawnSync timeout in the context-budget measure suite, a watchdog asserted to finish inside four seconds in the repo-fleet-hygiene audit suite). Two further entries are labelled in the file as not wall-clock cases: they are the unexplained pair from #3694, listed to fail toward running the suite rather than toward a red lane nobody can diagnose. An entry matching no discovered suite is an error, not an ignored line, so the allowlist cannot outlive what it excuses.

Output stays readable: every suite's output is captured and replayed as one block under a mkdir lock, printed the moment that suite finishes, so concurrent blocks never interleave and the per-suite markers CI log tooling reads survive. Captures are keyed on the suite's index rather than its path: a path-derived key collides whenever a segment already contains the separator, and because the capture holds the exit status too, two colliding suites in one batch could report a real failure as a pass.

Clock benchmarks gated. The two interleaved-pair comparisons in lib/hook-utils.test.sh (late-EOF window count, stall overshoot) run only under HOOK_UTILS_TIMING=1. Unset, each reports as deferred and names the load-independent probe beside it that carries the coverage, which is what the suite's own comments already said about #2105. The new .github/workflows/hook-utils-timing.yml runs the suite with the variable set every Monday on ubuntu-24.04 and windows-2025, plus workflow_dispatch, outside the required aggregate.

Two fixtures. The test_observer.py growth case inherited the production 30-second idle confirmation, so it could only ever end at the lifetime deadline; setting the confirmation to 1 second and the deadline to 10 makes it end when the tail actually goes idle. Both statusline-tee cancellation cases parked their mv shim for 10 seconds behind a readiness marker; bash defers the TERM trap until the foreground mv returns, so the shim's sleep was the floor. 2 seconds proves the same contract.

The remaining three fixtures profiled: generate-adapter.test.sh already makes a single shellcheck call, so nothing to do; the Gitea ceiling walk and the powershell-format per-invocation pwsh spawn are deferred with reasons recorded (the Gitea levers are adapter production code, filed as melodic-software/github-iac#390).

Version bumps. The fixture edits touch three shipped plugins, so context-guard, rate-limit-guard and session-flow each take a patch version and a changelog entry describing the test-side change. scripts/check-changelog-parity.sh states in its own header that it deliberately has no exemption list, so a test-only change does not qualify for one.

Verification

CI on this branch: run 33797359110 is green on every lane.

Local, on this branch:

  • bash scripts/run-plugin-tests.test.sh: exit 0, PASS=52 FAIL=0. New suite covers the exit contract, skip accounting, the allowlist and its stale-entry guard, block integrity under concurrency, real overlap through probe suites, PLUGIN_TEST_JOBS, a case asserting every shipped allowlist entry names an existing suite, a colliding path pair asserted under both parallel and serial dispatch, and a path containing a colon.
  • bash lib/hook-utils.test.sh: exit 0, PASS=284 FAIL=0, 73 seconds with the clock pairs deferred. Both deferred cases report by name and cite their standing regression guard. With HOOK_UTILS_TIMING=1 the suite still exits 0 at PASS=284.
  • bash plugins/context-guard/scripts/statusline-tee.test.sh: PASS=49 FAIL=0, 17 seconds. bash plugins/rate-limit-guard/scripts/statusline-tee.test.sh: PASS=125 FAIL=0, 45 seconds.
  • python plugins/session-flow/skills/running-retro/scripts/test_observer.py LedgerAndRetention: 13 tests OK in 7.8 seconds (the growth case alone was about 30 seconds).
  • shellcheck --rcfile=.shellcheckrc on all five changed shell files: exit 0. typos on every changed file: clean.
  • actionlint -config-file .github/actionlint.yaml .github/workflows/hook-utils-timing.yml .github/workflows/ci.yml: clean. zizmor --persona regular .github/workflows/hook-utils-timing.yml: no findings.
  • bash scripts/check-lane-coverage.sh --check: all 55 lanes reachable from ci-status.needs. bash scripts/check-docs-only-gate.sh --check: 50 references across 6 consumer jobs, all sanctioned. bash scripts/check-changelog-parity.sh --check and --check-bump origin/main: both exit 0.
  • pyright on test_observer.py: 18 errors at origin/main and 18 on this branch, same rule distribution (9 reportAttributeAccessIssue, 5 reportOptionalMemberAccess, 3 reportArgumentType, 1 reportOptionalSubscript). The pre-existing module_from_spec-on-an-Optional-spec and ModuleType attribute-assignment diagnostics did not grow.

Related

…tils clock benchmarks

scripts/run-plugin-tests.sh gains --jobs N (default 1, or PLUGIN_TEST_JOBS)
and runs the parallel group through xargs with each suite's output captured
and replayed as one block under a print lock, so the log stays readable and
the per-suite markers survive. Suites that assert wall-clock ceilings or run
concurrency probes of their own are listed in
scripts/run-plugin-tests-serial.txt and run one at a time before the group;
an entry naming no suite fails the run. The plugin-gate lane passes --jobs 4,
which is also the measurement run the serial allowlist is finalised against.
scripts/run-plugin-tests.test.sh covers the exit contract, the skip
accounting, the allowlist, the block integrity and the concurrency.

lib/hook-utils.test.sh gates its two interleaved-pair clock comparisons
behind HOOK_UTILS_TIMING; the load-independent probes beside them keep the
coverage, and the new weekly hook-utils-timing workflow runs the comparisons
on both operating systems.

Three of the slowest fixtures lose their fixed waits: the observer growth
case sets the one-second idle confirmation the harness defaulted to thirty
seconds, and both statusline-tee cancellation cases park the rename shim for
two seconds behind a readiness marker instead of ten.

Refs melodic-software/github-iac#385 (ci-perf Phase 2.2).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EEVWXSUGwsvFA918uaEnyx
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-03T17:56:16.459764Z 68f18b4 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 4m 38s —— View job


Code review complete

Tasks

  • Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md rules)
  • Run /review:code-review procedure
  • Post inline findings
  • Post summary review comment

This lane covers correctness and maintainability; security-only findings are left to the claude-security-review lane, which this repo already runs. I read all 9 changed files against origin/main and traced the parallel-runner logic (scripts/run-plugin-tests.sh) and its new self-test (scripts/run-plugin-tests.test.sh) in detail, since that's where the behavioral risk concentrates.

Finding posted inline:

  • scripts/run-plugin-tests.sh#L74 — the ${suite//\//__} log-file key (reused at L192) can collide for two distinct suite paths when a path segment contains a literal __, letting two parallel workers race on the same result file and silently swap a FAIL for a PASS. Latent today (no plugins/** path currently contains __), not live, but nothing detects the collision.

What I checked and found sound:

  • The serial/parallel split, the mkdir-based print lock keeping concurrent suite output blocks contiguous, and the --worker re-invocation dispatch (scripts/run-plugin-tests.sh:71-184) — traced the control flow carefully, including that cd happens before workers are spawned so relative suite paths resolve correctly for both the sequential serial loop and the xargs -P parallel batch.
  • The stale-allowlist guard (scripts/run-plugin-tests-serial.txt entries must match a discovered suite or the run fails at L152) and the --root/PLUGIN_TEST_SERIAL_LIST test-injection seam used by the new self-test — every invocation in run-plugin-tests.test.sh correctly overrides PLUGIN_TEST_SERIAL_LIST so it never reads the real production allowlist against fixture suites.
  • The HOOK_UTILS_TIMING gate added to lib/hook-utils.test.sh (Tests 18b/18g) — the new if [[ -z ... ]]; then ok "...deferred..." branch is correctly inserted ahead of the existing elif/fail fallback chain, so the EPOCHREALTIME-absent case is still handled when the gate is off.
  • test_observer.py's new idle_confirm_seconds=1.0, max_seconds=10.0 — confirmed against make_observer()'s real argument parser (default idle_confirm_seconds is indeed 30.0), so the stated bug (the growth case could only ever end at the lifetime deadline) checks out and the fix is sound.
  • The two statusline-tee.test.sh marker-file changes replace a fixed sleep 2 guess with a poll on a parked marker file before sending TERM — removes the race the old fixed sleep had against the shim's own startup, without weakening what the case asserts.
  • .github/workflows/hook-utils-timing.yml — read-only contents: read, pinned actions/checkout SHA, workflow_dispatch + weekly cron, not wired into ci-status, consistent with "advisory, outside the required gate" as described.
  • ci.yml's two hunks — --jobs 4 added to the one run-plugin-tests.sh invocation, and the new workflow file added (alphabetically, correctly) to the check-jsonschema file list. scripts/check-lane-coverage.sh and scripts/check-docs-only-gate.sh both scope to ci.yml's own jobs/references, so the new sibling workflow file doesn't need an entry there — no gap.

Not independently verified (author-claimed only): this lane's Bash tool is restricted to read-only inspection here, so I could not execute scripts/run-plugin-tests.test.sh, lib/hook-utils.test.sh, the statusline-tee.test.sh suites, test_observer.py, shellcheck, actionlint, or zizmor myself. The PR body's local run output and the CI wall-clock claims (572s baseline, --jobs 4 result on run 33656901629) are the author's own report, not something I confirmed.

@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 47s —— View job


Security review complete

  • Gather context (PR diff, changed files, /review:security-review skill criteria)
  • Apply skip gate (open PR? security-relevant surface? already reviewed?)
  • Review new/added workflow: .github/workflows/hook-utils-timing.yml
  • Review modified workflow: .github/workflows/ci.yml
  • Review scripts/run-plugin-tests.sh (new --jobs, locking, allowlist logic)
  • Review scripts/run-plugin-tests-serial.txt and scripts/run-plugin-tests.test.sh
  • Review remaining fixture changes (lib/hook-utils.test.sh, statusline-tee.test.sh x2, test_observer.py)
  • Post findings

No security issues found.

This PR is test/CI infrastructure only; no plugin production code or gate logic changes. Notes from the review (informational, not findings):

  • hook-utils-timing.yml is scoped correctly for a new workflow: permissions: contents: read only, actions/checkout pinned to a full commit SHA with persist-credentials: false, and triggers limited to schedule + workflow_dispatch — no pull_request_target/workflow_run over untrusted code, so no PR-controlled data ever reaches a run: block.
  • The ci.yml diff (lines 217–1421) only adds the new workflow to a static schema-validation file list and passes --jobs 4 to the existing plugin-gate step; no permissions:, trigger, or secret handling changed.
  • scripts/run-plugin-tests.sh validates --jobs with a strict ^[1-9][0-9]*$ regex before it reaches xargs -P, so it can't be used to inject flags/arguments. Suite paths driving bash "$suite" come only from a find plugins .claude/hooks -type f -name '*.test.sh' walk of the repo's own tree (same trust boundary the runner already had pre-PR), not from any external/PR-supplied string.
  • The serial-allowlist reader (run-plugin-tests.sh and its self-test) treats scripts/run-plugin-tests-serial.txt as static, comment-stripped data with a stale-entry guard — no code execution path through that file.
  • Remaining changes (lib/hook-utils.test.sh, both statusline-tee.test.sh fixtures, test_observer.py) are timing/assertion tweaks inside existing test suites; no new inputs, secrets, or trust boundaries touched.

Per the skill's high-signal bar, supply-chain/pinning and permission-widening are zizmor's advisory lane, not re-reported here; nothing in this diff falls into that lane either way (no new/loosened action pins, no permission changes).

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Last security-reviewed head: c37c0bd9d75d0ac51f77b9a29d5658f5233e2b35. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 68f18b4838

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/run-plugin-tests-serial.txt
Comment thread scripts/run-plugin-tests.sh Outdated
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

…ouched plugins

Three defects found in review, plus the version bumps the fixture edits
oblige.

The parallel runner keyed each suite's captured output and exit status on
its path with slashes rewritten to a double underscore. That collides
whenever a path segment already contains one, so two suites in a single
batch could overwrite each other's capture and a real failure could be
reported as a pass. The key is now the suite's index, which is unique by
construction and cannot be collided by any path shape. Its self-test gains
a colliding pair asserted under both parallel and serial dispatch, and a
path containing a colon so the indexed argument cannot be mis-split.

Two further deadline-based suites join the serial allowlist for the reason
the list already exists: a case that fails when a bounded operation
overruns is measuring the box once other suites compete for it. The
context-budget measure suite asserts a two-second spawn deadline and the
repo-fleet-hygiene audit suite asserts a watchdog finishes inside four
seconds.

Two more join it for a different reason, recorded as such. Both fail under
four concurrent suites on a hosted runner, pass serially in the same job
on main, and pass locally, and no hypothesis so far explains either. They
are listed to fail toward running the suite rather than toward a red lane
nobody can diagnose, and the entries carry the issue that holds the debt
so the listing cannot become permanent by default.

The fixture edits touch three shipped plugins, so each takes a patch
version and a changelog entry describing the test-side change. The
changelog gate states it has deliberately no exemption list, so a
test-only change is not exempt from the bump.

Refs melodic-software/github-iac#378
Refs melodic-software/github-iac#385
Refs #3694

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Both review threads confirmed and fixed in a39203dc7, along with the changelog-parity failure.

Log-key collision (run-plugin-tests.sh:74). Real, and worse than a lost log: the capture held the exit status too, so two colliding suites in one batch could report a real failure as a pass. The key is now the suite's index rather than its path, which is unique by construction and cannot be collided by any path shape. Tests: a colliding pair (plugins/a__b/c.test.sh and plugins/a/b__c.test.sh, both failing with distinct output) asserted under --jobs 3 and again under --jobs 1, since a serial run would overwrite silently rather than race; plus a path containing a colon, so the indexed argument the worker receives cannot be mis-split. The self-test is now 52 assertions, up from 40.

Two more deadline-based suites. Both line references verified. measure.test.sh carries a 2,000 ms spawnSync timeout on the ReDoS-shaped title case at 471-478, and audit-fleet.test.sh asserts its watchdog finishes inside four seconds at 1928-1936. Both meet the criterion the list already states and are added with that reason.

Two further entries were added for a different reason and are labelled as such in the file: tool-honesty.test.sh and the jira conformance binding both failed under --jobs 4 on run 33787030286, pass serially in the same job on main, and pass locally, with no hypothesis that survives the evidence. The pipefail plus grep -q SIGPIPE explanation was tested and rejected, and the run's log carries no resource-exhaustion signature. They are listed to fail toward running the suite, and #3694 holds the root cause so the entries cannot become permanent by default.

changelog-parity. The three fixture edits touch shipped plugins, so context-guard, rate-limit-guard and session-flow each take a patch version and a changelog entry describing the test-side change. The gate's own header states it has deliberately no exemption list, so a test-only change does not qualify for one. scripts/check-changelog-parity.sh --check and --check-bump origin/main both exit 0 locally.

@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 41s —— View job


Code review complete

Tasks

  • Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md rules)
  • Run /review:code-review procedure
  • Post inline findings (none — see below)
  • Post summary review comment

Two full reviews already ran on this PR at commit 68f18b4 (code-review and security-review lanes, both linked in the thread above). This update re-reviews only the delta since then: the new commit a39203d ("fix(tests): key the parallel captures by index and bump the fixture-touched plugins"), which was written specifically to address the two findings raised against 68f18b4 — my own inline comment about the ${suite//\//__} collision, and Codex's suggestion to serialize the two remaining deadline-based suites.

Both prior findings are resolved correctly:

  • Collision fix (scripts/run-plugin-tests.sh#L67-L91, #L168-L179, #L207-L211) — the capture key is now a zero-padded discovery-order index (suite_keys), not a path transform, so no suite-path shape can collide by construction. The --worker argument is now NNNNNN:suite, split with ${keyed%%:*} / ${keyed#*:}, which correctly handles a suite path that itself contains a colon (only the first colon is consumed). The dispatch loop, the serial loop, and the summary-aggregation loop all index off the same suite_keys array, so all three stay in sync.
  • Test coverage for the fix (scripts/run-plugin-tests.test.sh#L206-L239) — a genuinely colliding pair (plugins/a__b/c.test.sh vs plugins/a/b__c.test.sh) is asserted to report distinct output and distinct FAIL results under both --jobs 3 and --jobs 1, plus a separate case for a suite path containing a literal colon. These exercise exactly the failure mode described, not just the happy path.
  • Serial allowlist additions (scripts/run-plugin-tests-serial.txt#L58-L73) — measure.test.sh (2,000 ms spawn deadline) and audit-fleet.test.sh (four-second watchdog bound) are added with the exact line references Codex cited; I confirmed both line ranges match the deadline-assertion code they describe. Two further suites (tool-honesty.test.sh, jira.test.sh) are added for unexplained flakiness under --jobs 4, with the debt explicitly tracked against tool-honesty and jira conformance suites fail under run-plugin-tests --jobs 4, pass serially #3694 rather than silently absorbed — consistent with the PR's own "provisional until measured" framing for the allowlist.
  • Fixture version bumps (context-guard 0.7.36→0.7.37, rate-limit-guard 0.7.28→0.7.29, session-flow 0.34.21→0.34.22) — each patch bump has a matching changelog entry describing the test-side-only change, consistent with the changelog gate having no test-only exemption.

No new issues found in this delta. Everything outside a39203d (the workflow files, lib/hook-utils.test.sh, both statusline-tee.test.sh fixtures, test_observer.py) is unchanged since the prior review at 68f18b4 and was already covered there.

Not independently verified (author-claimed only): this lane's Bash tool is restricted to read-only inspection, so I could not execute the updated scripts/run-plugin-tests.test.sh myself to confirm the new collision/colon test cases actually pass.

@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 28s —— View job


Security review complete

  • Gather context (PR diff, changed files, custom instructions)
  • Invoke /review:security-review and follow it exactly
  • Post findings

No security issues found.

Per the skill's skip gate, I scoped this run to the commits since the last reviewed head (68f18b483821170d659d0a49f181bcb9baebfb09a39203dc7a3bd5350d689aba607d3a15eaad4b6d, commit a39203d), since the prior head already has a completed security review that still applies to everything before it.

What changed in this delta:

  • scripts/run-plugin-tests.sh — fixes the log-file-key collision flagged by the prior code-review pass (path-derived key ${suite//\//__} → a discovery-order index key, NNNNNN:suite). Reviewed the new split (key="${keyed%%:*}" / suite="${keyed#*:}") and the accompanying scripts/run-plugin-tests.test.sh collision/colon-path cases: the index is fixed-width and generated internally (never from suite content), so it can't collide or be manipulated, and the colon delimiter can't be mis-split by a suite path containing a colon since only the index-prefix is stripped from the left. Suite paths dispatched to bash "$suite" still come only from find plugins .claude/hooks -type f -name '*.test.sh' over the repo's own tree — no new external/untrusted input reaches this code path.
  • scripts/run-plugin-tests-serial.txt — two additional deadline-based suites added to the serial allowlist per the Codex review comment, plus two suites serialized for an unrelated, tracked flake (tool-honesty and jira conformance suites fail under run-plugin-tests --jobs 4, pass serially #3694). Static data, no code-execution path through this file (unchanged from the prior review).
  • Three plugin.json/CHANGELOG.md pairs (context-guard, rate-limit-guard, session-flow) — version bumps and changelog entries only, no behavioral or security-relevant content.

No workflow files, permissions, triggers, or secret handling changed in this delta, so nothing here falls into this lane's remit beyond what was already reviewed at the prior head.

At four concurrent suites, three separate suites failed across two hosted
runs, and none of them has a clock in it. The failing assertion in each
case was an external command producing EMPTY output where the same command
produces a result serially and locally: an awk read of an agent markdown
file, a jq read of a local-markdown fixture, and a conformance run. That is
one symptom with three faces, not three flaky suites, and serialising each
one as it appears only moves it to the next suite.

So the job count is the lever rather than the allowlist. The runner has
four vCPUs and the suites are spawn-bound, which is why four looked like
the shape that pays; three leaves the box a margin. Both wall times are
recorded against the serial baseline in the pull request.

Also fixes a spelling the hygiene lane caught in two comments, and assigns
the capture key by index rather than by append so it follows the position
in the suite list rather than agreeing with it only while the array stays
dense.

Refs melodic-software/github-iac#378
Refs melodic-software/github-iac#385
Refs #3694

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 47s —— View job


Code review complete

Tasks

  • Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md rules)
  • Invoke /review:code-review and follow it exactly
  • Post inline findings (none anchorable — see below)
  • Post summary review comment

This PR already has two full review rounds at 68f18b4 and a39203d (both linked above). Per the skill's skip gate, this pass reviews only the delta since the last reviewed head: commit c37c0bd ("run three contract suites at a time, not four"), which drops plugin-gate's --jobs 4 to --jobs 3 after three suites failed intermittently at four, plus a defensive suite_keys indexing change and two comment-wording tweaks.

Finding (no changed line to anchor to, so reported here per the reporting contract):

This commit leaves two other comments in the tree still asserting four is the right job count, both now contradicted by the code they document:

  • scripts/run-plugin-tests.sh#L19-L23 — the script's own top-of-file usage doc still reads "on a 4-vCPU Linux runner four at a time is the shape that pays." The new ci.yml comment (.github/workflows/ci.yml#L1421) explicitly points here ("see the runner's header") for the rationale, but the header a reader lands on says the opposite of what this PR just concluded (four caused three spurious failures; three "leaves the box a margin").
  • scripts/run-plugin-tests-serial.txt#L17-L22 — the allowlist's own "PROVISIONAL" finalization criterion says a suite is retired from the serial list once it "passes under --jobs 4 across the verification window," but the verification window going forward runs at --jobs 3 (per this same commit). As written, the stated graduation criterion for the whole allowlist no longer matches what CI actually runs.

(By contrast, scripts/run-plugin-tests-serial.txt#L66-L67's "fail under --jobs 4 on a hosted runner (run 33787030286)" is a historical record tied to a specific run ID and doesn't need updating.)

Neither is a functional bug — the runner behaves correctly regardless of what the comments say — but both are exactly the kind of self-contradictory in-repo documentation this PR is otherwise careful to avoid (the stale-allowlist-entry guard, the discriminating-skip accounting, etc. all exist precisely so behavior and its description can't drift apart silently). Worth a one-line update to each before merge.

Fix this →

What I checked and found sound in this delta:

  • The --jobs 4--jobs 3 change itself (.github/workflows/ci.yml#L1421) — a reasonable response to the described failure mode, and the commit is explicit that the new allowlist entries would only relocate the symptom, not fix it.
  • suite_keys[i]="$(printf '%06d' "$i")" replacing suite_keys+=(...) (scripts/run-plugin-tests.sh#L177) — behaviorally identical today since tests (from mapfile) is always dense, but correctly defensive: it ties the key to array position rather than append order, which is the property the dispatch/summary loops actually rely on.
  • The two "mis-split" → "split at the wrong separator" comment rewords in run-plugin-tests.sh and run-plugin-tests.test.sh — wording only, no behavior change, consistent with each other.

Not independently verified (author-claimed only): the two hosted-runner failure reports at --jobs 4 and the recorded wall times are the author's own account; this lane's tooling doesn't let me execute the suite or inspect the referenced CI runs.

@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 51s —— View job


Security review complete

  • Gather context (PR diff, changed files, custom instructions)
  • Invoke /review:security-review and follow it exactly
  • Apply skip gate / determine delta since last reviewed head
  • Review changed files for this delta
  • Post findings

No security issues found.

Per the skill's skip gate, this run scoped to the commits since the last reviewed head (a39203dc7a3bd5350d689aba607d3a15eaad4b6dc37c0bd9d75d0ac51f77b9a29d5658f5233e2b35, commit c37c0bd, "perf(ci): run three contract suites at a time, not four"), since the prior head already has a completed security review that still applies to everything before it.

What changed in this delta (3 files, 19/-11):

  • .github/workflows/ci.yml — only the plugin-gate job's run-plugin-tests.sh invocation changes, from --jobs 4 to --jobs 3, plus an expanded explanatory comment about why (three suites intermittently produced empty output from external-command reads at four concurrent jobs). No trigger, permissions:, secret handling, or step topology changed.
  • scripts/run-plugin-tests.sh#L170-L182suite_keys+=(...) (append) becomes suite_keys[i]=(...) (assign by index). This is a correctness hardening of the index-keyed capture scheme already reviewed at the prior head (a39203d) — it doesn't change the trust boundary: the index is still generated internally from the same find plugins .claude/hooks -type f -name '*.test.sh' walk, never derived from suite content, so it still cannot collide or be attacker-influenced. Comment wording tweaks only elsewhere in the --worker block (no logic change there).
  • scripts/run-plugin-tests.test.sh — two comment-only edits, no assertion or logic changes.

No workflow permissions, triggers, or secret handling changed in this delta, and no new input reaches any run: block or shell invocation. Nothing here falls into this lane's remit.

@kyle-sexton
kyle-sexton merged commit 78349ae into main Sep 3, 2026
70 checks passed
@kyle-sexton
kyle-sexton deleted the ci-perf/phase-2-scripts branch September 3, 2026 19:50
kyle-sexton added a commit that referenced this pull request Sep 4, 2026
…re-running on every push (#3696)

No related issue: Phase 2 of melodic-software/github-iac#385
(cross-repository)

Refs melodic-software/github-iac#378
Refs melodic-software/github-iac#385

## Summary

`ci.yml` ran about fifty-five jobs. Each paid for its own runner, its
own
checkout and its own toolchain install so it could run a few seconds of
work,
and every one of them ran on every pull request whatever the diff
touched.
Meanwhile both AI review lanes re-ran on every push to an open branch,
so an
active branch bought a full LLM code review and a full LLM security
review per
commit, most of them superseded before they finished.

This collapses `ci.yml` into six jobs behind one resolver, narrows what
each
lane runs to what the diff can actually affect, and stops both review
lanes
re-running on push and running on drafts at all. Phase 2 of the CI/CD
performance program.

## Fix

**Six jobs.** `changes`, `lint`, `test-linux`, `test-windows`,
`hook-utils`,
`ci-status`. Every former lane is a step of one of the four working jobs
and
keeps the name it had, so a check is still findable by the name it
always had.
`yq '.jobs | keys | length'` prints `6`.

**One resolver publishing a table of seven outputs.** `run_full` is the
root,
derived from the docs-only detector. The other six narrow it by the pull
request's draft state and by the ci-workflows `change-detection`
action's
filter groups: `shell`, `node`, `python`, `powershell`, `docs` and
`workflows`.
Every narrowing clause compares a group against `'false'` and never
`'true'`, so
an unset group runs its lane, which is the action's own documented rule
and the
same fail-open direction the detector already takes. Every filter group
repeats
the toolchain and configuration paths, so a pull request touching only a
lockfile, only a workflow file or only the ShellCheck configuration runs
every
lane rather than none. `docs` and `powershell` are computed and read by
nothing
today; they exist so the group set matches the program plan and a
consumer can
be added without touching the resolver.

**One job-level gate, outside the required-lane closure.**
`test-windows`
carries `if: needs.changes.outputs.run_windows == 'true'` and a
`# lane-coverage-ok:` annotation, and it is deliberately not in
`ci-status.needs`. A lane the aggregate cannot see costs a skipped
informational run rather than a false green; inside the closure the same
condition would let a lane report success having run nothing, and both
topology
gates reject it there. Every other gate in the file is at step level.

**The repin: sixteen of seventeen.** Sixteen of this file's seventeen
ci-workflows references now resolve to
`85e140fa2fe9ebe96c41156055e7676de208090a`, the `v0.19.0` tag cut after
the
ShellCheck fan-out merged. Before this, `ci.yml` alone carried three
ci-workflows revisions: the composite actions at `c2654182`, the
gitleaks
action at `f2d5e067`, and the zizmor reusable at `31a5b76c`. One SHA per
repository was never the status quo. Between `c2654182` and the wave tag
the
action surface moved by 272 commits, 51 action files, and +3257 and -353
lines;
the `change-detection` action this rewrite needs does not exist at
`c2654182`
at all.

**The seventeenth is held at the old SHA, deliberately.**
`machine-specific-paths` stays at `c2654182`. At the wave tag it detects
five
path classes, and on this repository all five fire on content that is
not a
machine-specific path: 40 hits across 22 files in ten plugins, every one
of
them prose naming the form the surrounding code parses, a published
CHANGELOG
entry, or a fixture whose assertion subject IS the path. Three of those
categories cannot be reworded. Released history is released, one flagged
changelog entry is itself documenting a previous fix to this very
detector and
necessarily quotes the forms it began catching, and changing a fixture's
literal changes what the test proves. The action's only lever is a
file-level
exclude, so silencing 40 phrases would take 22 files and ten plugins out
of the
scan entirely. melodic-software/ci-workflows#549 carries the
detection-precision
report upstream and #3699 tracks repinning this one line once that
lands.

Other workflow files in this repository still carry other ci-workflows
SHAs.
Converging them is Phase 6b's work, where the program plan verifies its
acceptance criterion per wave, and widening the repin here would mix a
dependency bump into a performance change. **Phase 6b's input set now
includes
this held reference too**, alongside the eight in other files.

**The zizmor correction.** The program's decision 8 specified a
hash-pinned
PyPI wheel at `zizmor==1.29.0`, "the version the reusable pins". Both
halves
were wrong. The reusable downloads a native release archive
(`zizmor-x86_64-unknown-linux-gnu.tar.gz`) verified against a SHA-256
input;
there is no PyPI involvement. And this repository passes no `version`
input, so
it takes the reviewed default at its pinned SHA `31a5b76c`, which is
**v1.27.0**.
1.29.0 was a worker's local binary version. The wheel pinned here is
therefore
`zizmor==1.27.0`, run with the reusable's own persona and with
`GH_TOKEN` from
the job's read-only workflow token for the online audits, so the step is
a
like-for-like replacement and no security-lane behaviour changes inside
a
performance change. One consequence, stated because it is real: like the
existing `pyyaml` pin this is a platform-specific wheel, so a host
outside
`manylinux_2_28_x86_64` cannot install from
`.github/requirements-ci.txt` under
`--require-hashes`.

**The draft filter and its event guard.** Both AI review callers now
trigger on
`opened`, `ready_for_review` and `reopened`, and both skip drafts.
`claude-review.yml`'s condition is
`github.event_name == 'workflow_dispatch' ||
github.event.pull_request.draft == false`:
the event clause is load-bearing, because a dispatched run carries no
`github.event.pull_request` and a bare draft comparison would never run
the
review an operator asked for. That caller also gains a
`workflow_dispatch`
trigger with a `pr-number` input and a fifteen-minute timeout, and its
header
now names the three re-review paths the reusable documents and no
others:
reopen, flip to draft and back to ready, or dispatch with the number.
`@claude`
is not wired here and the header says so.

Dropping the push trigger cannot block a merge, and the live ruleset is
the
evidence: the required status-check set on `main` is `ci-status`,
`do-not-merge / do-not-merge`, `pr-issue-linkage / pr-issue-linkage` and
`pr-title / pr-title`, under three active organization rulesets (`base`,
`ci-gate`, `signing`). ADR 0002 lines 202-205 say otherwise and are
stale on
three counts: they name `security-review / security-review` as required
via a
`security-review-gate` ruleset that is not active, they omit
`pr-issue-linkage / pr-issue-linkage` which is required, and the
`OrganizationAdmin` bypass they attribute to that ruleset therefore does
not
apply. The program plan already schedules the ADR 0002 amendment for
Phase 9,
so that sentence is flagged stale here rather than edited inside a
performance
change. If `security-review-gate` is ever re-enabled, the caller regains
its
push trigger in that same change.

**`--jobs 3`, not 4.** The contract corpus runs three suites at a time.
The
runner has four vCPUs and the suites are spawn-bound, so four looked
like the
shape that pays, and it is faster, but at four three separate suites
failed
across two runs by producing empty output from an external command on a
path
with no clock in it. Serialising each one in turn only moves the symptom
to the
next suite, so the job count is the lever rather than the allowlist.
Measured:
572 s serial, 312 s green at three, two red runs at four. See #3694.

| Setting | Run | Step wall time | Result |
|---|---|---|---|
| serial | 33656901629 | 572 s | green |
| `--jobs 4` | 33787030286 | 228 s | red |
| `--jobs 4` | 33794273916 | 240 s | red |
| `--jobs 3` | 33797359110 | 312 s | green |

**Two departures from the written specification, both forced.**

A step cannot carry both a resolver gate and an event gate. The
docs-only gate
reads a step's condition whole, so
`needs.changes.outputs.X == 'true' && github.event_name ==
'pull_request'` is an
unsanctioned consumer form; and a step gated on the event alone is
`skipped` on
a push, which the fail-closed aggregator turns red. Every diff-scoped
gate
therefore runs on each event and returns early from its own script when
there
is no base ref to diff against.

The stale-base overlap detector runs in a detached worktree at the pull
request
head. On a `pull_request` event this job checks out the synthetic merge
commit,
whose merge-base with the target IS the target tip, so the detector
would have
reported every pull request fresh and proved nothing. Its own former job
carried
`ref: head.sha` for exactly that reason.

**Deleted with its lane:** `scripts/check-rename-sweep.sh` and its
suite. The
gate ledger rules them a permanent lane for a finished rename's residue,
and
nothing else in the tree names either file.

**Also:** `dependabot-miro-bundle.yml` gains a path filter on the two
miro
manifests and a per-pull-request concurrency group, and `AGENTS.md`
gains the
instruction that pays for the draft filter: open a pull request as a
draft and
flip it to ready when the work is done.

## Verification

Every command below was run locally on the branch.

| Command | Result |
|---|---|
| `bash scripts/check-docs-only-gate.sh --check` | exit 0: scope
resolved once in `changes`; 91 references across 4 consumer jobs, all
sanctioned |
| `bash scripts/check-docs-only-gate.test.sh` | 57 of 57 `ok`, 0 `not
ok`, including the live-file case that was red before this rewrite |
| `bash scripts/check-lane-coverage.sh --check` | exit 0: all 4 lanes
reachable from `ci-status.needs` |
| `bash scripts/check-lane-coverage.test.sh` | ALL PASS, including its
live-file case |
| `yq '.jobs \| keys \| length' .github/workflows/ci.yml` | `6` |
| `yq '.on.pull_request.types' .github/workflows/ci.yml` | `[opened,
synchronize, reopened, ready_for_review]` |
| `actionlint -config-file .github/actionlint.yaml` on all four changed
workflows | exit 0, clean |
| `zizmor --persona regular` on all four changed workflows | no findings
(7, 3, 3 and 2 suppressed) |
| `typos` on every changed file | clean |
| `markdownlint-cli2 AGENTS.md` | 0 issues |
| `grep -c "synchronize"` on both review callers | `0` and `0` |
| `grep -c "timeout-minutes: 15" .github/workflows/claude-review.yml` |
`1` |
| `grep -c "draft == false"` on both review callers | `2` and `3` |
| `grep -c "affected-tests.sh --run" .github/workflows/ci.yml` | `1` |
| `bash scripts/check-changelog-parity.sh` `--check`, `--check-bump`,
`--check-preserved`, `--check-order` | all exit 0 |
| `bash scripts/check-contract-slice-prune.sh` `--check` and
`--check-diff origin/main` | both exit 0 |
| `bash scripts/check-vendor-version-bump.sh --check-bump origin/main` |
exit 0 |
| `bash scripts/check-orphaned-fixtures.sh --check`,
`check-fixture-git-isolation.sh --check` | both exit 0 |
| `bash scripts/check-hook-wiring-liveness.sh`, `check-silent-skips.sh`
| both exit 0 |
| `bash scripts/check-plugin-manifest-presence.sh`,
`check-plugin-catalog-enablement.sh` | both exit 0 |
| `bash scripts/check-skill-leaf-names.sh --check`,
`check-loop-lane-floor-drift.sh --check` | both exit 0 |
| `bash scripts/affected-tests.sh --base origin/main` | exit 0; both
deleted files reported as covered by nothing surviving |
| Machine cross-check: every `continue-on-error` step in `lint` against
the `CHECK_RESULTS` feed | 54 steps, 54 feed rows, no unfed step, no
unpaired row, no duplicate |
| The zizmor wheel hash, re-derived from the PyPI JSON API |
`zizmor-1.27.0-py3-none-manylinux_2_28_x86_64.whl`
`sha256:afb28123882d2b8248f1e480bf6cc6d1af102e0d3fbe22a40f7f795b1aa9d435`
|

Two probe pull requests exercised the filter groups against this branch
and are
now closed unmerged: #3697 (lockfile-only, opened as a draft and flipped
to
ready) and #3698 (workflow-only). Four real runs; their per-lane
verdicts are in
the comment below. Three properties only a run could establish:

**The filter groups matched for real, not fail-open.** This is the
property the
two probes cannot distinguish on their own, because the action fails
OPEN to
relevant: a rejected pattern, a parse warning or an API hiccup produces
the same
all-`true` result a correct match does. Run `33808374809`'s match step
settles
it, printing `FILES_LIST_FAILED: false` and then six named groups all
`true`,
with no warning, `outcome=success` in 59 ms. So the group names the
resolver
looks up through `fromJSON` are the names the action actually published.

**The detached worktree reads the head, not the merge commit.** The
stale-base
step's log reads `Preparing worktree (detached HEAD b9996c5)`, which is
#3698's own head commit. Both probe heads were genuinely fresh, so the
detector's verdict proves nothing either way; the checked-out SHA is
what proves
it is looking at the right tree.

**Wall time, against the 25-minute timeout and the program's 12-minute
p95
target.**

| Run | `lint` | `test-linux` | `test-windows` |
|---|---|---|---|
| `33808259580` (this PR, draft) | 5.3 min | 0.2 min | skipped |
| `33808363829` (#3697, draft) | 4.5 min | 0.2 min | skipped |
| `33809595757` (#3697, ready) | — | ran the contract suite | ran |
| `33808374809` (#3698, ready) | 5.6 min | 1.6 min | 1.4 min |

`lint` is the critical path on every pull request including a docs-only
one, and
it is serial now. At 4.5 to 5.6 minutes the p95 target is not
unreachable by
construction.

The affected-suite selector was exercised on both sides: `No suites
selected` on
#3698's lone workflow file, and a non-empty run on #3697's ready-flip
ending
`All 6 selected suites passed or were skipped.`

**Both previously-red rows are resolved.** `runner-policy` was red
because
`.github/standards/runner-policy/policy.json` gave the claude-review
reusable at
`62bef7ba` an `allowedInputs` of exactly `["runner"]`, and the
review-trigger
change above adds `pr-number` and `timeout-minutes`. That file is
standards-distributed, so under ADR-0007 the fix was a change in
`melodic-software/standards` (#521) and its sync back here (#3701,
merged as
`557e1ab01`); this branch is rebased onto it and `runner-policy.mjs` now
passes.
`machine-specific-paths` was red because of the repin, and is resolved
by
holding that one action at its old SHA, above.

Security pass on every trigger and permission change: `pull-requests:
read` is
granted on `changes` alone, which is the only job that needs it (the
change-detection action lists the pull request's files through the API);
no `pull_request_target` anywhere; no secret is reachable from a fork
context;
`actionlint` and `zizmor --persona regular` are clean on every changed
workflow; and `claude-review.yml` keeps its repo-wide `queue: max` seat
group
unchanged, with no second concurrency block added.

One finding from that pass, recorded rather than silently accepted: the
security lane's required check certifies execution, and the reusable's
own
canonical caller keeps the push trigger for that lane specifically.
Without it
the required check can certify a head the branch has since moved past.
The
program plan rules the trigger cut deliberate; the recovery path is to
flip the
pull request to draft and back to ready, and that is written into the
caller's
header rather than left for a reader to discover.

## Related

- Phase 2 of melodic-software/github-iac#385, itself a sub-issue of
  melodic-software/github-iac#378.
- Sub-topic plan: melodic-software/github-iac#391, merged as `02ac028`,
with the
  follow-up in melodic-software/github-iac#393.
- PR A, the script half of Phase 2: #3693, merged as `78349aeb3`.
- PR B, the ci-workflows ShellCheck fan-out:
melodic-software/ci-workflows#547,
  merged as `85e140f` and cut as `v0.19.0`.
- #3694 holds the unexplained parallel-suite failures at `--jobs 4`.
- #3699 tracks repinning `machine-specific-paths` to the wave tag once
  ci-workflows#549 fixes its detection precision.
- #3703 tracks runners for the Node suites outside the four sub-projects
and for
the Pester suites, which no step executes on either the pull-request or
the
push path. The contract-suite step names them in a warning and counts
them in
  the run summary rather than reporting them as executed.
- melodic-software/standards#521 adds `pr-number` and `timeout-minutes`
to the
claude-review reviewed contract; its sync into this repository has to
merge
  before the `runner-policy` row goes green.
- A cross-repository closing keyword does nothing on GitHub, so
  melodic-software/github-iac#385 is closed by hand after this merges.

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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