Repository navigation
fix(OMN-14891): systemic git-environment isolation for the test suite - #1481
Conversation
Git exports GIT_DIR / GIT_WORK_TREE / GIT_INDEX_FILE / GIT_COMMON_DIR into every hook environment, and those OVERRIDE both `cwd=` and `git -C`. When pytest runs as a subprocess of the repo's own pre-push hook, a fixture doing subprocess.run(["git", "init"], cwd=tmp_path) re-initialises the REAL invoking worktree instead of tmp_path. $OMNI_HOME/<repo>/.git/config is shared by every worktree of that repo, so one session's test run corrupts every concurrent session. Ships three things together (Rule #5: fixture + gate or the class recurs): 1. Session-level scrub in tests/conftest.py — called from pytest_configure (before collection, so import-time git calls are covered) and re-asserted by an autouse session fixture. Runs per-worker under xdist. 2. Pre-commit hook + CI gate (validator-no-unguarded-git-subprocess.yml, ADVISORY pending 2 green dev runs) rejecting git shell-outs in tests/ that can inherit those vars, plus `git config --global/--system`. Fails closed. 3. RED-proven regression (tests/unit/test_git_env_isolation.py) built as a discriminator pair: a negative control asserts the fake GIT_DIR genuinely hijacks an unguarded call, so the guarded assertion cannot be vacuous. Also fixes the 15 unguarded call sites the gate found across 3 files — 10 more than the 5 known ones, and the 4 previously hand-patched files were already correct, which is the evidence that per-file patching (OMN-14744) does not hold. Refs: OMN-14891, OMN-14744 (failed per-file precedent), OMN-14892 (related index corruption).
…iver + classify in standalone-validator debt registry Triage wf_ce6262bf-975: two live-airtight registry reds from adding validator-no-unguarded-git-subprocess.yml without registration. 1. pull-request-workflow-budget.yaml: sanctioned waivers-bucket entry (ticket OMN-14891, expires 2026-08-19, justification, retires) per the ratchet's mechanism (b) — budget stays 49, no raw-count bump. 2. standalone-validator-debt.yaml: classified decorative_debt with its unique context no-unguarded-git-subprocess — honest bucket: context is ADVISORY, NOT in dev required_status_checks (verified 2026-07-22 via gh api); moves to natively_required_contexts when the flip tracked in .github/required-checks.yaml lands. Fixes test_live_ratchet_holds + test_live_registry_is_airtight.
…ative-count bump Session-authorized two-test edit (ledger ~19:45Z decision) on top of ea6cdcd: 1. test_live_pull_request_workflows_matches_registry now asserts live == allowlist UNION active waived files, reusing the validator's own _validate_waivers (imported, not replicated) so expired/incomplete waivers drop out of the active set and the test goes RED when the OMN-14891 waiver lapses (fail-closed, one canonical waiver semantic). 2. test_decorative_debt_is_the_reported_count frozen count 18 -> 19, the two-place-lock edit its docstring mandates for the new decorative entry registered in ea6cdcd.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 16 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 (17)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
OMN-14891 — systemic git-environment isolation for the test suite
Evidence-Ticket: OMN-14891
Closes OMN-14891.
What this changes
tests/conftest.py: a session-scoped fixture unsetsGIT_DIR/GIT_INDEX_FILE/GIT_WORK_TREE/GIT_COMMON_DIRfor the whole test session. Those variables overridecwd=andgit -C, so atmp_pathtest that shells out to git while inheriting them from a pre-push hook mutates the real invoking worktree — observed twice (core.bare=true+ a bogus[user]block + ~6707 phantom staged deletions on the shared canonical clone).no-unguarded-git-subprocessguard (src/omnibase_core/validators/no_unguarded_git_subprocess.py+model_git_subprocess_finding.py), wired as a pre-commit hook and registered ADVISORY in.github/required-checks.yaml(plus.github/workflows/validator-no-unguarded-git-subprocess.yml). It blocks reintroduction of an unguarded git subprocess intests/.tests/unit/test_git_env_isolation.pyandtests/unit/validators/test_no_unguarded_git_subprocess.pyfail against the pre-fix state and pass after.Key finding (why session-level scrub, not more per-file patches)
Of the 5 known polluters, 4 were already hand-patched (OMN-14744) and correct — yet two entirely un-listed files were still polluting:
tests/integration/test_validation_integration.py(12 sites) andtests/scripts/test_check_release_identity.py(2 sites). That is direct evidence per-file patching does not hold; the correct fix is the session-level scrub plus a gate that blocks regression.Owed follow-ups (not in this PR)
no-unguarded-git-subprocessgate advisory → required after 2 consecutive green runs.omnibase_infraper the OMN-14744 recurrence.Notes
dev@d55b7050(OMN-14925 / fix(OMN-14925): align pre-push naming hook with CI-canonical baseline-aware validator #1480), which aligns the local pre-push naming hook to the CI-canonical baseline-aware validator (validate_class_naming.py). Before that fix, the branch's pre-push was blocked by the old rawvalidate_naming.py src/full-scan failing on 29 pre-existing baselined violations (none from this branch — its own two src files are clean). Post-rebase the naming hook passes.omninode-pcwith pre-push + pre-commit hooks installed (readback-verified) and the full omnibase_core unit suite run there. The run was consolidated after a duplicate-launch collision (a second concurrent push of the same branch was stopped so a single clean run produced the result).Re-drive provenance (session fable-gwexec-0722)
This branch's first pre-push run ended in an honest RED (30 failed / 43,206 passed, 10h14m serial) and was fully triaged before this landing — no bypass at any point:
pull-request-workflow-budget.yaml+decorative_debtentry instandalone-validator-debt.yaml) plus 2 sanctioned test evolutions (waiver-aware ratchet assertion mirroring the validator's own expiry semantics + the debt-registry count bump its own docstring mandates). An adversarial expired-waiver check confirmed the evolved assertion stays fail-closed.PREPUSH_PYTEST_ARGS="-n 12 --reruns 2"knob (CI-equivalent xdist profile): 43,236 passed / 0 failed in 47m, governed selector hook Passed, PUSH_EXIT=0.proof: box terminal summary 2026-07-22T20:56Z, ground-truth
git ls-remoteSHA fbb4f7a.Evidence-Source: OCC#4608
Evidence-Commit: 0f660d209381b7b933664dd34514742ddf894890