Repository navigation
fix(OMN-14988): bash-3.2-safe slash normalization in check_no_infra_inmemory_import.sh - #2498
Conversation
…nmemory_import.sh
The no-infra-inmemory-import gate (OMN-7077/OMN-13419) false-positived on a
clean tree on macOS: 8 violations reported, all 8 in the script's own
ALLOWLIST, zero true positives.
Root cause is two platform facts compounding:
* BSD grep (macOS) reports the search root "src/" as "src//" in every hit,
so paths arrive as src//omnibase_infra/... GNU grep on Linux CI does not.
* The normalization ${file//\/\//\/} is bash-version-dependent. Under bash
>= 4.3 it collapses correctly; under bash 3.2 -- which IS /usr/bin/env bash
on stock macOS, and therefore the interpreter this hook runs under locally
-- the backslash in the replacement word is retained literally, producing
src\/omnibase_infra/... which matches no ALLOWLIST entry.
Fix: hold the pattern and replacement in variables (_normalize_path), removing
the escaping ambiguity entirely; loop so runs longer than two slashes collapse
fully rather than partially.
Adds regression coverage in tests/unit/scripts/validation/:
* normalization asserted per bash interpreter present on the machine, so
bash 3.2 is covered where it exists;
* end-to-end gate runs with a grep stub pinning BSD-style src// hits, both
for the allowlisted set (must exit 0) and for a non-allowlisted violation
(must still exit 1 -- the fix must not neuter the gate);
* a clean-tree run of the real script under every bash;
* a static ratchet rejecting reintroduction of the fragile substitution in
executable lines, which is RED on every platform including Linux CI where
the bash 3.2 behavior cannot be reproduced.
RED/GREEN: 13 failed against the pre-fix script, 19 passed after.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 6 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 (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…ibase_infra#2498 (#5133) * evidence(OMN-14988): author OCC companion for OmniNode-ai/omnibase_infra#2498 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head c9ca7eb18d55027720f8da42cb153559c788cbb7. * evidence(OMN-14988): self-bind OCC#5133 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@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)
1 participant
OMN-14988 —
check_no_infra_inmemory_import.shfalse-positives on a clean treeTicket: OMN-14988
OCC companion: OmniNode-ai/onex_change_control#5133 (autobind) — lands first
Evidence-Ticket: OMN-14988
Evidence-Source: OCC#5133
The defect
The
no-infra-inmemory-importgate (OMN-7077 / OMN-13419) — a pre-commit hook and arequired CI Lint step — reported 8 violations on an unmodified
devtree, and all 8were entries in the script's own
ALLOWLIST. Zero true positives; signal-to-noise 0/8.Reproduced on
.200at cleandev, by direct invocation (not viapre-commit run, sothe failure is in the script itself, independent of the framework or
core.hooksPath):Root cause — two platform facts that only fail together
BSD grep (macOS) doubles the root separator.
grep -rn ... src/reportssrc//omnibase_infra/.... GNU grep on Linux CI reportssrc/omnibase_infra/....The normalization was bash-version-dependent.
file="${file//\/\//\/}"collapsescorrectly under bash >= 4.3, but under bash 3.2 — which is
/usr/bin/env bashonstock macOS, and therefore the interpreter this hook actually runs under locally — the
backslash in the replacement word is retained literally:
The gate only fails when BSD grep and bash 3.2 are both in play, which is why Linux CI
never saw it and macOS contributors always did.
Fix
Hold the pattern and replacement in variables (
_normalize_path), which removes thebackslash-escaping ambiguity entirely and behaves identically on 3.2 and 5.x, and loop so
runs longer than two slashes collapse fully instead of partially (
src///a.py->src/a.py,not
src//a.py). The call site keeps aTRAPcomment naming the one-liner and why it mustnot come back.
A
--print-normalized-pathentry point is added so the normalization can be asserted perinterpreter without depending on the local grep flavor.
Seams
Sole boundary is grep stdout line ->
ALLOWLISTstring comparison. Contract: the pathsegment left of the first
:normalizes to a single-slash,src/-rooted, repo-relativepath, compared by exact string equality against
ALLOWLISTentries in the same form. Thatseam is now driven directly by the regression tests via a grep stub, not inferred.
ALLOWLISTcontents, the matchPATTERN, exit codes, and output text are unchanged.Test evidence — RED before, GREEN after
Same test file both runs:
The RED is "exists but wrong", not "missing":
test_gate_exits_zero_on_bsd_grep_doubled_slash_hitsand
test_gate_still_flags_non_allowlisted_doubled_slash_hitfail on/bin/bash(3.2)and pass on bash 5.3 against the pre-fix script — that split is the defect signature.
Coverage added:
covered where it exists), including the exact reported
src//omnibase_infra/backends/auto_configure.pycase and an explicit "no backslash in output" assertion.
grepstub pinning BSD-stylesrc//hits, so thedoubled-slash input is deterministic on every platform rather than dependent on the local
grep.
src//prefixmust still be reported and still exit 1. A naive "fix" that allowlisted everything would
fail here.
lines. This one is RED on every platform including Linux CI, where the bash 3.2
behavior cannot be reproduced behaviorally.
Gate results
Heavy gates ran on
.200(stickybeatz-studio) per rule 11a, via patch-transfer withsha256verified identical on both hosts (f8f98289…script,d6f483a8…test):scripts/ci/detect_test_paths.py) resolved this change totests/unit/scripts/— no hand-typed-k.uv run pytest tests/unit/scripts/ -q -n autoon.200: 840 passed, 0 failed.devbaseline for the same selection on.200was 8 failed — 7 shell-hygiene(see deviations) and 1
test_gate_allows_allowlisted_adapter_import, which was thisdefect already failing the pre-existing gate test on
.200. This PR takes thatselection to zero failures and introduces none.
shellcheck -xon the modified script: clean.ruff check/ruff format --check/mypy --stricton the test file: clean.pre-commit run --fileson both changed files: all hooks pass, includingBlock infra-path EventBusInmemory imports (OMN-7077)-> Passed.The blocker this retires
OMN-14988 was escalated because an empty
git commit --allow-empty(zero content change)could not clear this hook, forcing a documented
--no-verifybypass against rule 10 — agate that cannot be satisfied by a zero-diff commit trains bypass behavior directly.
The commit in this PR was created with the full pre-commit suite running and the OMN-7077
hook passing. No
--no-verify, no skip token, no allowlist addition.Deviations from standing process (stated, not hidden)
.200. All heavy gates (test suite, shellcheck,ruff, mypy, pre-commit) ran on
.200. The push itself could not: the local PreToolUseworktree guard blocks creating a worktree for a remote path over ssh (it resolves
the remote path against the local canonical root), and
.200's canonical-clone guardblocks pushing from the canonical clone. Both guards were left intact —
ONEX_WORKTREES_ROOT,onex hooks disable, andALLOW_CANONICAL_CLONE_COMMIT=1wereall deliberately not used. A push is a network op, not the contended-CPU path rule
11a exists to move off this Mac.
.200gate runs used the canonical clone's working tree (patch applied, gates run,git checkout --restored, porcelain verified 0) — CLAUDE.md permits running there;nothing was branched, staged, or committed in it. The authoritative worktree is
$OMNI_HOME/omni_worktrees/OMN-14988/omnibase_infra.Incidental findings on
.200(not fixed here, not mine to land)test_shell_hygiene_gate.pyfailures were not a missing shellcheck —shellcheck 0.11.0 is installed at
/opt/homebrew/bin, which a non-interactive sshPATH does not include. With the PATH corrected the gate is green. Any automation that
ssh's into
.200and runs these gates without/opt/homebrew/binon PATH gets afail-closed red that looks like a real failure.
~/omni_home/omnibase_infraon.200contains a stale emptyvalidation/directory(dated Jun 12) that trips the
ONEX Root Directory Cleanlinesshook. Being empty anduntracked it is invisible to
git status --porcelain. Absent from the worktree; harmlessto this PR, but it makes that hook permanently red in
.200's canonical clone.Merge
Codex lands. No
--auto, not a draft. Companion #5133 first.A hand-authored companion (#5132) was opened companion-first three minutes before OCC
autobind produced #5133 for this PR; per standing precedent autobind wins the companion
slot and #5132 was closed as the duplicate. #5133 is the thin autobind shape — its three
dod_evidenceitems prove only that this PR exists and has files, none of them falsifiableagainst the fix. The falsifiable probes (clean-tree exit-0 under bash 3.2, the RED/GREEN
counts, the anti-neutering check, governed-selection 840/0, shellcheck, and the OMN-7077
hook passing without
--no-verify) are recorded in this body and as adod_evidencecomment on OMN-14988, and should be re-landed onto
contracts/OMN-14988.yamlafter #5133merges — the same repair pattern as onex_change_control#5129 and #5117.