Repository navigation
fix(OMN-15617): resolve bash>=5 explicitly instead of trusting PATH order - #2651
Conversation
…rder On stickybeatz-studio (.200, the rule-11a default gate host), non-interactive ssh resolves `bash` to the system 3.2.57 shell even though a modern bash 5.x sits at /opt/homebrew/bin/bash — it just is not first on PATH for that session class. runner-monitor.sh uses `declare -A` (bash>=4), so all 15 tests in test_runner_monitor_wedge_detection.py fail silently there — a bash syntax error inside a subprocess, not a resolvable "wrong interpreter" diagnostic. - scripts/ci/resolve_modern_bash.sh: new bash-3.2-safe resolver, the single source of truth for finding a bash>=5 interpreter independent of PATH order (checks OMNIBASE_INFRA_BASH_BIN override, the two brew prefixes, then every "bash" on PATH). Fails loud with a pointed remediation message when none is resolvable — never a silent fallback, never a quiet skip. - tests/unit/observability/runner_health/_resolve_modern_bash.py: thin pytest wrapper shelling out to the resolver script; used by both test_runner_monitor_wedge_detection.py (the flagged 15) and test_runner_monitor_auto_bounce.py (same bug pattern, same fix). Both now invoke the resolved interpreter explicitly instead of bare "bash". - scripts/hooks/prepush_smart_tests.sh: bash>=5 canary added before the governed selector runs, using the same resolver script (DRY — single source of truth with the pytest harness). Exports OMNIBASE_INFRA_BASH_BIN so pytest does not have to re-discover it. Fails loud with remediation when unresolvable. Local proxy verification (macOS ships stock bash 3.2, reproducing the identical PATH-order bug): under a stock-only PATH (env -i PATH=/usr/bin:/bin), pre-fix code fails 14/15 in test_runner_monitor_wedge_detection.py; post-fix, 15/15 pass. Full related scope (119 tests) passes with only the pre-existing missing-`flock` skip.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 34 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
#6047) * evidence: OCC companion pass 1 for OmniNode-ai/omnibase_infra#2651 * evidence: OCC companion self-bind for #6047 --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai>
|
| 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)
…d-split (#2652) Addresses two adversarial-verify defects on PR #2651: - Reorder resolve_modern_bash() before the jq/flock tool-availability skip in test_runner_monitor_wedge_detection.py and test_runner_monitor_auto_bounce.py. Previously a missing jq (or flock) triggered pytest.skip() before the bash>=5 canary ran, so a host with the wrong interpreter AND a missing secondary tool would report green-by- absence instead of the RED the ticket's AC2 requires. - Fix scripts/ci/resolve_modern_bash.sh to build CANDIDATES as a bash-3.2- safe array instead of a space-joined string. The prior unquoted word-splitting silently dropped any interpreter path or PATH entry containing a space, risking a silent-wrong-answer resolution in the exact script whose purpose is to eliminate that failure mode. Verified: 102 passed / 4 skipped (flock-only, unrelated) under env -i PATH=/usr/bin:/bin; resolver returns exit 1 with pointed stderr and empty stdout when OMNIBASE_INFRA_MIN_BASH_MAJOR is set unreachably high (proves genuine fail-closed RED); resolver correctly resolves a space-containing OMNIBASE_INFRA_BASH_BIN path post-fix (previously would silently drop the fragment). Ticket: OMN-15617
1 participant
OMN-15617 — .200 gate host bash-version resolution
Fixes the failure OMN-15617 documents: on
stickybeatz-studio(.200, therule-11a default gate host), non-interactive ssh sessions resolve
bashtothe system 3.2.57 shell even though a modern bash 5.x sits at
/opt/homebrew/bin/bash— it just is not first on PATH for that sessionclass.
docker/runners/runner-monitor.shusesdeclare -A(bash>=4), soall 15 tests in
test_runner_monitor_wedge_detection.py(the exact countthe ticket names) fail there on every commit — a bash syntax error deep
inside a subprocess, not a resolvable "wrong interpreter" diagnostic.
Fix shape (per ticket AC — explicit resolution + fail-closed canary, no silent fallback/skip)
scripts/ci/resolve_modern_bash.sh— new bash-3.2-safe resolver, thesingle source of truth for finding a bash interpreter
>=5, independentof PATH order (checks
OMNIBASE_INFRA_BASH_BINoverride, the two brewprefixes, then every
bashon$PATH). Fails loud with a pointedremediation message when none is resolvable — never a silent fallback,
never a quiet skip. Must itself run under bash 3.2 (no
declare -A) sinceit cannot presuppose the thing it's resolving.
tests/unit/observability/runner_health/_resolve_modern_bash.py— thinpytest wrapper shelling out to the resolver;
pytest.fails (not skip) ifunresolvable.
tests/unit/observability/runner_health/test_runner_monitor_wedge_detection.py(the flagged 15 tests) and
test_runner_monitor_auto_bounce.py(same bugpattern, same fix, applied for consistency though only the former is in
the ticket's named count) — both now invoke the resolved interpreter
explicitly instead of bare
"bash"for the wrapper script and the innermonitor.shinvocation.scripts/hooks/prepush_smart_tests.sh— bash>=5 canary added before thegoverned selector runs, using the same resolver script (DRY — single
source of truth with the pytest harness, so they cannot drift). Exports
OMNIBASE_INFRA_BASH_BINso pytest does not have to re-discover it. Failsloud with remediation when unresolvable.
Seams
OMNIBASE_INFRA_BASH_BIN— optional override consumed byscripts/ci/resolve_modern_bash.sh; exported byprepush_smart_tests.shafter resolution so pytest inherits it without re-discoveryscripts/ci/resolve_modern_bash.sh— stdout contract: absolute interpreter path on success (exit 0), nothing on stdout + pointed message on stderr on failure (exit 1)tests/unit/observability/runner_health/_resolve_modern_bash.py::resolve_modern_bash()— imported by bothtest_runner_monitor_wedge_detection.pyandtest_runner_monitor_auto_bounce.pyscripts/hooks/prepush_smart_tests.sh— new block right afterREPO_ROOTresolution, beforeBASE_REF; usesdie()(existing helper) on failuredocker/runners/runner-monitor.shitself,.pre-commit-config.yamlhook wiring, CI workflow filesVerification
Local proxy (macOS ships stock bash 3.2 by default — same class of bug):
under a stock-only
PATH(env -i PATH=/usr/bin:/bin), pre-fix code fails14/15 in
test_runner_monitor_wedge_detection.py(1 static/non-bash testpasses); post-fix, 15/15 pass. Full related scope (119 tests,
tests/unit/observability/runner_health/+tests/ci/test_prepush_hook_host_identity_guard.py) passes with only thepre-existing missing-
flockskip (unrelated)..200(stickybeatz-studio) live validation could not be completed thissession —
ssh stickybeatz-studioreturnedPermission denied (publickey,password,keyboard-interactive)for this session's identity(no ssh-agent identities, local
id_ed25519rejected by the remote host).Gates instead ran on this Mac as a documented rule-11a exception; the local
Mac's stock
/bin/bashis also 3.2.57 (identical to.200's), so thebefore/after repro above is a faithful proxy for the exact failure mode,
though it is not the designated host itself. Flagging for a follow-up
verification pass with .200 access, or operator confirmation of PATH-order
behavior there post-merge.
Push itself DID exercise the real pre-push hook end-to-end on this Mac and
confirmed the canary fires and logs correctly:
followed by the full governed-selector impacted-subset run (3198 passed, 9
skipped, unrelated).
Ticket: OMN-15617
Evidence-Ticket: OMN-15617
Evidence-Source: OCC#6047