Skip to content

fix(OMN-16333): unblock every omnibase_infra push — scope the pinned-interpreter gate to local invocations - #2825

Closed
jonahgabriel wants to merge 1 commit into
devfrom
jonah/omn-16333-prepush-ssh-probe-unblock
Closed

jonahgabriel wants to merge 1 commit into
devfrom
jonah/omn-16333-prepush-ssh-probe-unblock

Conversation

@jonahgabriel

@jonahgabriel jonahgabriel commented Aug 21, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

git push to omnibase_infra has been failing for every developer, on every branch, from every worktree. The pre-push hook runs a test that rejects the hook script itself.

FAILED tests/ci/test_prepush_hook_pinned_interpreter.py::test_prepush_smart_tests_has_no_floating_python_invocation
FAILED tests/ci/test_prepush_hook_pinned_interpreter.py::test_no_hook_script_has_a_floating_python_invocation
2 failed, 1501 passed

Found while trying to land OMN-16323; reproduced independently against origin/dev.

Root cause — two correct changes that are mutually incompatible

The pinned-interpreter gate (OMN-14953) asserts no scripts/hooks/*.sh invokes python3 unless immediately preceded by uv run. Its stated rationale is local interpreter skew.

OMN-16295 then added host_load_ratio() to prepush_smart_tests.sh, which invokes python3 three times:

  • line 150 — a local read. Genuinely in scope; the gate was right about this one.
  • lines 159 / 162 — ssh ... "$target" "python3 -c ...", executing on .200 / .201.

The scanner is a line-level regex with one exemption, so it cannot tell local from remote and flags all three.

Why the obvious fixes are wrong

Prefixing the remote calls with uv run breaks the feature — ssh jonah@192.168.86.201 "command -v uv" returns command not found. uv is a property of this local checkout, not of the remote hosts, so the gate's invariant is unsatisfiable by construction for a remote command: requiring uv run there forbids the call rather than pinning it.

Dropping python from the remote probe is also wrong. _PREPUSH_LOAD_PROBE_PY uses os.getloadavg() + os.cpu_count() precisely because both are POSIX-portable, letting one snippet serve the Linux .201 and macOS .200 targets. A shell equivalent needs /proc/loadavg+nproc on Linux and sysctl on macOS — exactly the branching the author avoided.

Fix

Narrowing an over-broad matcher rather than bypassing it, per omni_home CLAUDE.md rule 10.

  1. scripts/hooks/prepush_smart_tests.sh — route the local probe through uv run python3. Verified working: uv run python3 -c "$PROBE" returns 13.87 24.
  2. tests/ci/test_prepush_hook_pinned_interpreter.py — exempt python tokens following an ssh token in the same logical line.

The scan now joins backslash continuations. That detail is load-bearing: the real probe puts ssh and the remote python3 on different physical lines, so a physical-line carve-out silently misses and the repo stays deadlocked — which is exactly what happened on my first attempt.

Coverage for the narrowing

Four tests so the carve-out cannot silently widen:

  • test_local_bare_python_is_still_flagged — a bare local python3 is still a violation even when the same file also contains ssh calls.
  • test_ssh_embedded_python_is_exempt — single-line ssh form.
  • test_ssh_exemption_spans_a_line_continuation — the real backslash-continued form.
  • test_continuation_does_not_leak_exemption_to_later_commands — once the ssh command ends, a later local python3 is flagged again.

uv run pytest tests/ci/test_prepush_hook_pinned_interpreter.py → 7 passed.

Proof the deadlock is broken

This PR's own branch pushed successfully with the pre-push hook enabled and no --no-verify, which was impossible on dev immediately before it.

Ticket

OMN-16333. Blocks OMN-16323 (release.yml shell-injection fix) in this repo, which in turn gates OMN-16289 step 2.

Evidence-Ticket: OMN-16333
Evidence-Source: OCC#6815

…d probe, scope the gate to local invocations

The pinned-interpreter gate (OMN-14953) and the ssh load probe (OMN-16295)
were mutually incompatible, so `git push` failed for every developer on
every branch: the pre-push hook ran a test that rejected the hook script.

Two changes:

1. prepush_smart_tests.sh — route the LOCAL probe through `uv run python3`.
   This one was a real violation of the invariant; the gate was right.

2. test_prepush_hook_pinned_interpreter.py — exempt python tokens that
   follow an `ssh` token in the same LOGICAL line. Those execute on a
   remote host, which has no uv venv for this repo (`ssh .201 'command -v
   uv'` -> not found), so the pinned-venv invariant is unsatisfiable there
   by construction; requiring `uv run` would forbid the call, not pin it.
   The scan now joins backslash continuations, because the real probe puts
   `ssh` and the remote `python3` on different physical lines — scanning
   physical lines is what made the carve-out miss and the repo deadlock.

Narrowing an over-broad matcher rather than bypassing, per omni_home
CLAUDE.md rule 10. Four tests cover the narrowing so it cannot silently
widen: a local bare python3 is still flagged (with or without ssh elsewhere
in the file), the exemption spans a continuation, and it does not leak past
a completed ssh command.
@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

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: 53 minutes

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

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 24e0d212-aa90-4401-a3a4-15536188d9ed

📥 Commits

Reviewing files that changed from the base of the PR and between 2444b99 and 7d30a88.

📒 Files selected for processing (2)
  • scripts/hooks/prepush_smart_tests.sh
  • tests/ci/test_prepush_hook_pinned_interpreter.py

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

@github-actions

Copy link
Copy Markdown
Contributor

❓ Hostile Reviewer — UNKNOWN

Blocking findings (critical): 0
Total findings: 0
Models succeeded: none


Gate semantics (pilot phase)

Verdict Meaning Blocks merge?
passed No critical findings No
blocked CRITICAL findings found Yes
degraded All models unavailable (infra) No (pilot)

Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8468/OMN-8524)

@jonahgabriel

Copy link
Copy Markdown
Collaborator Author

Superseded by #2824 (OMN-16326), which landed on dev while this was in CI.

Both PRs fix the same deadlock — the pre-push hook running a test that rejected the hook script, blocking every push for every developer. #2824's approach is better and I'm withdrawing this one in its favour.

Mine narrowed the OMN-14953 matcher to exempt python invocations inside ssh remote commands, on the reasoning that this repo's uv venv cannot exist on .201. That works, but it weakens the gate: it carves a permanent hole that future code could sit in.

#2824 instead makes the probe interpreter-free — getconf _NPROCESSORS_ONLN plus /proc/loadavg with a sysctl fallback — so no python is invoked in the hook at all and the original invariant stays strict, with no carve-out to maintain. That is the stronger fix.

Verified on dev at 063cf6ecff69b21d3f2896e404b7d5f2644e2ed8: uv run pytest tests/ci/test_prepush_hook_pinned_interpreter.py passes, and I pushed jonah/omn-16323-release-tag-injection through the live hook with no bypass flag — the deadlock is genuinely gone.

The four regression tests I wrote here (local bare python3 still flagged, ssh single-line, ssh across a backslash continuation, exemption not leaking past a completed ssh command) are not carried over, since #2824 removes the need for the carve-out they guard. OMN-16333 is closed as superseded by OMN-16326.

Follow-on release.yml security fix for this repo continues in #2827.

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