ci: verify the shared hook lib on Windows - #1975
Conversation
`lib/hook-utils.sh` carries OSTYPE-gated branches that a Linux runner never executes: case-insensitive path folding for the Windows filesystem, and `cygpath` short-name (8.3) resolution. Every CI job in this repository runs on ubuntu-24.04, so those branches have only ever been exercised by whatever a maintainer happened to run on a dev box. This adds the job that makes them a gate. Deliberately scoped to one suite. The rest of the corpus is platform-agnostic bash string logic that Linux exercises identically — `lib/powershell/ ps-command.sh` classifies PowerShell commands yet carries zero OSTYPE / cygpath / uname branches, so Linux tests it faithfully. Adding a platform-agnostic suite here would buy no coverage while paying Windows' process-creation cost, measured at ~140ms per spawn against ~3ms on Linux. Free: GitHub Actions is free for public repositories on standard GitHub-hosted runners, Windows included; only larger runners are charged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 1m 36s —— View job Review:
|
|
Claude finished @kyle-sexton's task in 50s —— View job Security Review
Scope: this PR's only change is adding one new job, No security issues found. Checked specifically for the categories in scope for this lane (logic/architecture/trust-boundary reasoning, not what zizmor already covers statically):
Supply-chain/pin verification ( |
|
Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f309474be7
ℹ️ 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".
The runner policy forbids the floating `*-latest` labels outright (policy.json forbiddenHostedRunnerLabels), for the same reason the Linux lanes pin ubuntu-24.04: an image roll should be a reviewed commit, not a silent Tuesday. Verified locally against both gates — actionlint clean, `runner-policy.mjs --root .` passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 11s —— View job Security Review
Scope: this PR's only change is the new No security issues found. The second commit is the only delta since the last security pass on this PR, and it's a supply-chain hardening, not a new risk: it swaps the floating Re-checked the categories in scope for this lane against the full current diff:
No CRITICAL, IMPORTANT, or SUGGESTION findings. |
|
Claude finished @kyle-sexton's task in 1m 2s —— View job Code ReviewVerdict: the previously-flagged gap is still open — as of HEAD ( 🔴
|
hook-utils-windows was absent from the ci-status needs graph. ci-status is the single required check and derives its verdict from join(needs.*.result), so the Windows lane was informational: a Windows-only regression in lib/hook-utils.sh would go red while the required aggregate reported success — the exact hole the job comment claimed to close. Adding it to needs is sufficient, since the aggregate reads the needs graph rather than a hand-maintained list. Verified no job in that graph carries a job-level `if:`, so none can report `skipped` into the `case` arm that treats a non-success result as a failure. The job comment now says where the gating actually comes from, so the next lane added here does not repeat this. runner-policy passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 55s —— View job Code Review
Verdict: the gap flagged in every prior review pass on this PR (mine and Codex's) is fixed as of Previously-flagged gap: resolved
Everything else holds from prior passes
No outstanding findings. Branch · |
|
Claude finished @kyle-sexton's task in 55s —— View job Security Review
Scope: this PR's only change remains No security issues found.
Re-confirmed against the categories in scope for this lane, across the full cumulative diff (
No CRITICAL, IMPORTANT, or SUGGESTION findings. Branch · |
) Closes #1978 ## Summary Every hook in this marketplace sources `lib/hook-utils.sh`, and buffering the hook payload spawned three external processes to do work bash can do in-process. On Windows Git Bash, where process creation is `fork()` emulation, each spawn costs roughly 140 ms — paid on every tool call, in all 16 plugins that carry the library. ## Fix - **`hook::resolve_read_slice`**: the `awk` float division becomes fixed-point shell arithmetic, printing the same three-decimal form `read -t` is given. `printf -v`, not `$( )`, because a command substitution forks the shell even for a builtin — the fork is the cost being removed. - **`hook::buffer_stdin`**: `printf | tr -d '\r'` becomes `${input//$'\r'/}`, and the post-loop `jq -e .` validity probe is skipped when `hook::json_complete` already parsed the identical CR-stripped buffer with jq inside the read loop. `json_complete` returns non-zero both for an incomplete buffer and for absent/broken jq, so the flag is set only on its success path and the jq-absent fail-open is untouched. - **New `hook::jq_fields`**: extracts several fields from one payload in a single jq process, for hooks that read two or three fields from the same envelope and currently pay a fork plus an exec for each. It uses `// ""` rather than `// empty` so an absent field keeps its slot instead of silently shifting every later index onto the wrong filter, reads NUL-separated values through a process substitution (command substitution strips NUL), and strips CR **after** the read — the Windows jq build writes stdout in text mode and expands every LF it emits to CRLF, so a value cleaned inside jq arrives dirty anyway. No hook call sites change in this PR. The plugins that read a second field already gate it behind an early exit or a telemetry probe, so converting them would add work on the common path; the batch helper's win is in the guardrails git guards, which read `.tool_input.command` and `.tool_name` unconditionally — and those files are in flight in #1974. The helper ships now because the lib sync gate makes every library change cost a version bump in all 16 carrying plugins; adding it later would pay that a second time. ## Verification **Measured, quiet box, 15 alternating pairs** of the same `block-dangerous-git` invocation against each library version (alternating so machine-load drift hits both arms equally): | lib | mean | min | max | | --- | --- | --- | --- | | `main` | 1672 ms | 1316 ms | 2443 ms | | this branch | 1401 ms | 1120 ms | 1760 ms | ~270 ms per invocation, and the slow tail shrinks with the mean. That is less than the 3 × 140 ms the spawn-count model predicts; the measured number is the one to trust. **Gates run locally:** - `lib/hook-utils.test.sh` — new coverage for the slice format (including the fallbacks a non-numeric bound and a `0.000` quotient must take) and for `hook::jq_fields` (multi-line and CR-carrying values, absent-field slot retention, unparsable payload, no-filter call, non-string values). The two `buffer_stdin` timing assertions that fail intermittently here fail the same way on `main` (1–3 failures per run on both sides) — they are wall-clock-ceiling tests on a loaded Windows box, the same class as the ceilings tracked for `block-noncanonical-commit.test.sh`. - `plugins/guardrails/hooks/block-dangerous-git.test.sh` — the black-box hook contract suite, run serially (never concurrently: its wall-clock assertions fail spuriously under parallelism). - `scripts/sync-hook-utils.sh --check` — all 16 plugin copies match. - `scripts/sync-hook-utils.sh --check-bump origin/main` — every carrying plugin bumped. - `scripts/check-changelog-parity.sh --check-bump origin/main` and `--check-order`. - `scripts/check-shell-portability.sh --paths`, `shellcheck -x`, `shfmt -d -i 2`, `markdownlint-cli2`, `scripts/check-manifest-duplicate-keys.py`. **Fresh-docs mandate**: no WebFetch was required for this change and none was performed. The edit is internal implementation of a shell library — it touches no hook contract surface, no manifest field beyond the mechanical `version` bumps the sync gate itself demands, and no documented harness behavior. The 16 touched manifests are version lines only. ## Related - Refs #1974 — carries the guardrails PowerShell false-positive fix and touches `plugins/guardrails/CHANGELOG.md` and `plugin.json`. Both PRs bump guardrails to `0.19.1`, so whichever merges second needs a one-line rebase onto `0.19.2`. - Refs #1975 — adds the Windows CI job for `lib/hook-utils.test.sh`; it is the coverage that would have caught a Windows-only regression in this file. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
No linked issue
Summary
Every CI job in this repository runs on
ubuntu-24.04.lib/hook-utils.shcarriesOSTYPE-gated branches that a Linux runner never executes:cygpathshort-name (8.3) resolutionThose branches have only ever been exercised by whatever a maintainer happened to run on a dev box. This adds the job that makes them a gate.
Why only one suite
This is deliberately narrow, and the narrowness is the point.
I checked which code is genuinely platform-dependent rather than assuming.
lib/hook-utils.shhas 18 platform-conditional sites with explicitOSTYPEbranches.lib/powershell/ps-command.sh— which classifies PowerShell commands, and is therefore the intuitive candidate for a Windows job — has zero. It is pure bash string manipulation and behaves identically on both platforms, so Linux already tests it faithfully.Adding platform-agnostic suites to a Windows runner would buy no coverage while paying Windows' process-creation cost. Measured on a Windows dev box: ~140ms per process spawn, against ~3ms on Linux. A hook invocation spawns 7 externals, so it costs ~2.0s on Windows; the largest suite has 319 cases and takes ~11 minutes there. That cost is exactly why the full suite is unusable locally on Windows, and why it should not be replicated in CI.
The comment block on the job states this rule so the job does not accrete suites over time.
Cost
Free. GitHub Actions is free for public repositories on standard GitHub-hosted runners, Windows included; only larger runners are charged — per GitHub's billing docs, fetched for this change:
Verification
actionlintclean. The job's real verification is its first run on this PR — iflib/hook-utils.test.shhas latent Windows assumptions, this is where we find out, which is the point of adding it.Related
lib/hook-utils.sh's per-invocation subprocess spawns from 7 to ~1 (which also addresses two open hook-timeout reports from a consuming project), and an affected-suite selector so a local run does not mean running all 60 suites.block-noncanonical-commit.test.shuses hardtimeout 30wall-clock ceilings, which fail spuriously when the box is loaded. Surfaced while running suites in parallel.