Skip to content

fix(guard-push): make the gh availability probe injectable so its unit test is hermetic - #2222

Closed
BigSimmo wants to merge 1 commit into
mainfrom
claude/guard-push-test-isolation
Closed

fix(guard-push): make the gh availability probe injectable so its unit test is hermetic#2222
BigSimmo wants to merge 1 commit into
mainfrom
claude/guard-push-test-isolation

Conversation

@BigSimmo

Copy link
Copy Markdown
Owner

Summary

tests/guard-push.test.ts > inFlightCiGuard formats actionable blocked message with PR and run details fails locally with Test timed out in 30000ms.

The test injects prViewer and runFetcher, but inFlightCiGuard calls the real ghIsAvailable() before reaching them, which spawns the gh binary. That is a local probe (gh --version), not a network call — but on a loaded machine it is not fast. Measured on this workstation while several worktrees were running suites:

$ time gh --version
gh version 2.97.0 (2026-07-31)
real 1m37.478s
$ time node -e "execFileSync('gh',['--version'],{stdio:'ignore'})"
real 0m17.040s

So a unit test that stubs every dependency it knows about was still hostage to an external process it never asked for, and went red for reasons unrelated to the code under test.

ghAvailable now joins prViewer and runFetcher as an injectable dependency, defaulting to ghIsAvailable so production behaviour is unchanged. The two existing call sites pass ghAvailable: () => true.

Two tests pin the behaviour so it cannot regress quietly:

  • fails open when gh is unavailable, without consulting the PR — asserts ok: true, the gh not available note, and that prViewer was never called.
  • never spawns a process when every dependency is injected — asserts the injected probe is the only one consulted.

autoMergeGuard has the same un-injected ghIsAvailable() call, but it takes no options object and has no direct unit test, so it is deliberately left alone rather than widened into an unrelated refactor.

RAG impact: no retrieval behaviour change — this touches a git pre-push guard script and its unit test only; no retrieval, ranking, selection, ordering, or clinical surface is involved.

Verification

  • npx vitest run tests/guard-push.test.ts -t "inFlightCiGuard" — the target test now passes in 1ms, down from a 30s timeout. Both new tests pass in 0ms.
  • npx prettier --write scripts/guard-push.mjs tests/guard-push.test.ts — both already conformant ("unchanged").
  • npx eslint scripts/guard-push.mjs tests/guard-push.test.ts --max-warnings 0 — exit 0.
  • Full-file run: 41 passed, 1 unrelated failure (below).

Not fixed here, reported rather than masked.push-range parsing > keeps a Windows new-branch static command scoped to the PR side of an advanced main also times out locally, taking 61s when run alone — so it is not sibling contention. It is genuinely I/O-heavy: it writes 360 files with 96-character names into a real git repo, commits them, and diffs. That is slow on a loaded Windows ReFS Dev Drive, not a stubbing defect, and raising its timeout would hide machine cost rather than fix anything. Left for a separate decision; worth a ledger row.

Clinical Governance Preflight

  • Source-backed claims still require linked source verification before clinical use
  • No patient-identifiable document workflow was introduced or expanded without explicit governance approval
  • Supabase target remains Clinical KB Database (sjrfecxgysukkwxsowpy)
  • Service-role keys and private document access remain server-only
  • Demo/synthetic content remains clearly separated from real clinical sources
  • Source metadata, review status, and outdated/unknown-source behavior remain conservative
  • Deployment classification/TGA SaMD impact was checked when clinical decision-support behavior changed

Notes

  • Default behaviour is unchanged: with no injection, the guard still probes for gh exactly as before, and still fails open when it is absent.
  • The 97s gh --version is itself worth noticing — it made every push in this session slow, since the pre-push guard invokes gh several times. That is an environment issue, not something this PR addresses.

…t test is hermetic
tests/guard-push.test.ts "inFlightCiGuard formats actionable blocked message"
injects prViewer and runFetcher, but inFlightCiGuard still called the real
ghIsAvailable() first, which spawns the gh binary. On a loaded machine that call
was measured at 97s for `gh --version` (17s through a bare node spawn), so the
test blew vitest's 30s limit and failed - a unit test held hostage by an
external process it never asked for, and one that goes red for reasons that have
nothing to do with the code under test.
ghAvailable joins prViewer and runFetcher as an injectable dependency, defaulting
to ghIsAvailable so production behaviour is unchanged. The two existing call
sites inject `ghAvailable: () => true`.
Two tests pin the behaviour so this cannot regress silently: one asserts the
fail-open path when gh is unavailable AND that the PR is not consulted in that
case, and one asserts the guard spawns nothing when every dependency is injected.
The target test now runs in 1ms, down from a 30s timeout.
Not fixed here, and reported rather than masked: "push-range parsing > keeps a
Windows new-branch static command scoped to the PR side of an advanced main"
also times out locally, taking 61s alone. That one is genuinely I/O-heavy - it
writes 360 files with 96-character names into a real git repo and commits them -
so it is slow on a loaded Windows Dev Drive rather than defective. Raising its
timeout would be masking machine cost, not fixing a stubbing defect, so it is
left for a separate decision.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your current included review allowance is based on your included PR review attempts over the past 7 days.

Next review available in:24 minutes

Limit details: You’ve used the included review currently available. Your 87 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7757c2e0-3eff-4c45-9bd8-e94b02baf4cc

📥 Commits

Reviewing files that changed from the base of the PR and between e2de814 and f5504a0.

📒 Files selected for processing (2)
  • scripts/guard-push.mjs
  • tests/guard-push.test.ts

Comment @coderabbitai help to get the list of available commands.

@supabase

supabaseBot commented Aug 21, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@BigSimmo

Copy link
Copy Markdown
OwnerAuthor

@copilot resolve the merge conflicts on this branch.

@BigSimmo

Copy link
Copy Markdown
OwnerAuthor

Closing as superseded, not abandoned: the same fix landed on main first in #2209 (af2075a5c), which makes ghAvailable injectable on inFlightCiGuard with the same parameter name and the same ghIsAvailable default.

I checked whether anything here was still additive before closing. It is not. #2209's fail-open test is stronger than the two I added — it throws if prViewer or runFetcher is consulted at all, where mine merely counted the calls, and it already catches the exact regression mine were written to guard (reverting to the un-injected probe would make the injected ghAvailable: () => false case fall through to prViewer and fail). Resolving the conflict would have reproduced what main already has.

Root cause recorded for whoever hits this next: gh --version was measured at 1m37s on the Windows workstation (17s through a bare node spawn), so the un-injected probe blew vitest's 30s limit. That slowness also made every local git push take 5-10 minutes, since guard-push.mjs invokes gh several times per push. Worth diagnosing as an environment issue in its own right.

🤖 Closed by Claude Code

@BigSimmo
BigSimmo deleted the claude/guard-push-test-isolation branch August 21, 2026 13:05
Sign up for freeto 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

@BigSimmo