Repository navigation
fix(ci): resolve contract_compliance_check detached-HEAD in PR merge context [OMN-7996] [OMN-7998] - #257
Merged
jonahgabriel merged 8 commits intoApr 13, 2026
Conversation
…context [OMN-7996, OMN-7998]
- _check_command now substitutes {pr} and {repo} placeholders before execution,
so contract YAML DoD checks don't rely on git branch context
- pre-commit commands demoted to WARN (not BLOCK) when pre-commit is not installed in runner
- contracts/OMN-7996.yaml and OMN-7998.yaml updated to use parameterized gh calls
(gh pr view {pr} --repo {repo} ...) and gh pr checks without --watch
- Two new tests: placeholder substitution and pre-commit-absent warn path
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 45 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdated contract YAML evidence to use Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Compliance Runner
participant Validator as Repo Validator
participant PreCommit as Pre-commit Checker
participant Shell as Subprocess (sh / gh / which)
Runner->>Validator: receive `repo` value
Validator-->>Runner: valid / invalid
alt invalid
Runner-->>Runner: return BLOCK (Invalid repo)
else valid
Runner->>Runner: substitute `{pr}` and `{repo}` into command
Runner->>PreCommit: is command a pre-commit invocation?
alt pre-commit command
PreCommit->>Shell: which pre-commit
Shell-->>PreCommit: found / not found
alt not found and (CI or local)
Runner-->>Runner: return WARN (skipped pre-commit)
else found
Runner->>Shell: run substituted command in workspace (cwd)
Shell-->>Runner: output / exit status
end
else other command
Runner->>Shell: run substituted command in workspace (cwd)
Shell-->>Runner: output / exit status
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/OMN-7996.yaml`:
- Around line 33-34: The current check item uses check_type: "command" with the
GH CLI invocation, which bypasses the repository's standardized handling in
_check_test_passes; replace this entry to use check_type: "test_passes" (keeping
the same gh pr checks payload or equivalent identifier) so the framework invokes
_check_test_passes and converts non-green states into BLOCK/WARN as intended;
locate the YAML entry with check_type: "command" and check_value: "gh pr checks
{pr} --repo {repo} --json name,state" and change the type to test_passes so the
standardized logic is used.
- Around line 22-28: The current command checks under id "dod-002" only request
mergedAt/state and can succeed even if the PR is not targeting main; update the
check_value to query baseRefName along with state/mergedAt (use gh pr view
--repo {repo} {pr} --json baseRefName,state,mergedAt) and make the -q expression
assert baseRefName == "main" and the expected state/mergedAt (e.g. for merged:
.baseRefName=="main" and .mergedAt!=null, or for open: .baseRefName=="main" and
.state=="OPEN"); modify the check_value strings in both places to use a combined
JSON query so the command returns non‑zero when the branch or state/timestamp is
not as expected and ensure this change is applied to the check where check_type
is "command" (id "dod-002") and any related _check_command usage.
In `@contracts/OMN-7998.yaml`:
- Around line 33-34: The current contract entry uses check_type: "command" with
check_value "gh pr checks {pr} --repo {repo} --json name,state"; replace this
with check_type: "test_passes" (or the equivalent token your schema expects) so
that the existing _check_test_passes logic is invoked instead of running the raw
command; specifically, change the check_type field for the entry containing
check_value "gh pr checks {pr} --repo {repo} --json name,state" to "test_passes"
so _check_test_passes will parse check names and convert non-green states into
BLOCK/WARN as intended.
- Around line 22-28: The gh command checks (the check_value entries that call
"gh pr view {pr} --repo {repo} --json state -q .state" and "gh pr view {pr}
--repo {repo} --json mergedAt -q .mergedAt" under id "dod-002") only fetch
state/mergedAt and can succeed for PRs not targeting main; update those checks
to assert baseRefName=="main" as well and fail with a non-zero exit when either
condition is not met — i.e., call gh pr view to fetch both baseRefName and
state/mergedAt and combine checks so the shell returns non-zero unless
baseRefName equals "main" and state==MERGED (or mergedAt is non-empty) to ensure
the check truly verifies the PR targets main and is merged.
In `@tests/test_contract_compliance_check.py`:
- Around line 152-161: The test test_check_command_placeholder_substitution
currently only verifies exit status and would pass without actual substitution;
update it to mock the internal runner function (e.g., _run) and assert that _run
was invoked with the command string after placeholders are substituted (e.g.,
"echo pr=586 repo=OmniNode-ai/omnidash") when calling _check_command with
pr_number=586 and repo="OmniNode-ai/omnidash"; keep the existing assertion for
result but add the mock assertion to ensure placeholders are actually replaced
before execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98faea2e-dc30-4440-84d6-d5e57a2b7129
📒 Files selected for processing (4)
contracts/OMN-7996.yamlcontracts/OMN-7998.yamlscripts/ci/run_contract_compliance_check.pytests/test_contract_compliance_check.py
…ful tests + state assertion
- Validate {repo} against ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ before substitution;
adversarial values return BLOCK instead of reaching sh -c
- test_check_command_placeholder_substitution now captures the executed command
and asserts 586/OmniNode-ai/omnidash appear and no literal {pr}/{repo} remain
- Add test_check_command_invalid_repo_blocks for the new validation path
- dod-001 in OMN-7996 and OMN-7998: pipe state through grep -E '^(OPEN|MERGED)$'
so CLOSED PRs fail the check
- pre-commit demotion now also triggers on CI=true env var (finding #4)
- Add test_check_command_precommit_skipped_in_ci for CI gate path
- Fix truncated summary in contracts/OMN-7996.yaml (finding #5)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_contract_compliance_check.py (2)
202-209: Make the CI pre-commit test hermetic.This test depends on whatever
which pre-commitreturns on the host, so it does not actually force the “installed but still skipped becauseCI=true” path. Mock_runto reportpre-commitpresent and keep the assertion focused on the CI-specific behavior.Suggested tightening
def test_check_command_precommit_skipped_in_ci( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: """In CI=true, pre-commit is always skipped regardless of installation.""" monkeypatch.setenv("CI", "true") - result, detail = _check_command("pre-commit run --all-files", tmp_path) + with patch( + "run_contract_compliance_check._run", + return_value=(0, "/usr/bin/pre-commit", ""), + ): + result, detail = _check_command("pre-commit run --all-files", tmp_path) assert result == _RESULT_WARN assert "CI" in detail or "skipped" in detail🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_contract_compliance_check.py` around lines 202 - 209, The test test_check_command_precommit_skipped_in_ci is non-hermetic because it relies on the real PATH; modify it to monkeypatch/mock the internal helper _run (the one used by _check_command) to return a result that indicates "pre-commit" is present (e.g., a successful which-like response) while still setting CI=true, so _check_command exercises the "installed but skipped due to CI" branch; ensure the mock only affects the external check and keep the existing assertions that result == _RESULT_WARN and that the detail contains "CI" or "skipped".
177-186: Assert that invalidrepoinput never reaches_run.This currently proves the return value, but not the “reject before execution” guarantee. If
_check_command()regressed and still spawned a subprocess before returningBLOCK, this test would miss it.Suggested tightening
def test_check_command_invalid_repo_blocks(tmp_path: Path) -> None: """Adversarial repo value must be rejected before shell substitution.""" - result, detail = _check_command( - "gh pr view {pr} --repo {repo}", - tmp_path, - pr_number=1, - repo="evil; rm -rf /", - ) + with patch("run_contract_compliance_check._run") as run: + result, detail = _check_command( + "gh pr view {pr} --repo {repo}", + tmp_path, + pr_number=1, + repo="evil; rm -rf /", + ) assert result == _RESULT_BLOCK assert "Invalid" in detail + run.assert_not_called()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_contract_compliance_check.py` around lines 177 - 186, Modify the test to assert that _run is never invoked by monkeypatching the module-level _run used by _check_command to a stub that raises if called (e.g., def _run_should_not_be_called(*a, **k): raise AssertionError("_run was called"); monkeypatch.setattr(<module_where__check_command_is_defined> , "_run", _run_should_not_be_called)), then call _check_command("gh pr view {pr} --repo {repo}", tmp_path, pr_number=1, repo="evil; rm -rf /") and assert the returned result is _RESULT_BLOCK and the detail contains "Invalid"; this ensures the function returns BLOCK before any subprocess is spawned. Ensure you reference the same _check_command and _run symbols from the module under test when patching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ci/run_contract_compliance_check.py`:
- Around line 216-256: The command-check currently runs shell commands in the
process CWD so --workspace is ignored; modify the _run(...) helper to accept a
cwd: Path|None and pass cwd=str(cwd) (or None) into subprocess.run(...,
cwd=...), then update _check_command to call _run(["which","pre-commit"],
timeout=5, cwd=_workspace) and _run(["sh","-c", cmd_str], timeout=60,
cwd=_workspace) (ensure _workspace is passed as a Path and converted in _run).
This ensures both the pre-commit existence check and the command invocation
honor the workspace override.
- Around line 244-255: The current condition treats CI as a reason to skip
pre-commit (in_ci or rc_which != 0), which disables enforcement in CI; change
the logic so we only skip when pre-commit is not installed. In the block that
checks cmd_str.lstrip().startswith("pre-commit"), keep the _run(["which",
"pre-commit"], ...) call and then only branch on rc_which != 0 (remove the in_ci
OR), printing the same warn message and returning _RESULT_WARN and the message
if pre-commit is missing; otherwise proceed to run the pre-commit check even
when CI=true (use the existing variables cmd_str, in_ci, rc_which, _run, and the
return values _RESULT_WARN).
---
Nitpick comments:
In `@tests/test_contract_compliance_check.py`:
- Around line 202-209: The test test_check_command_precommit_skipped_in_ci is
non-hermetic because it relies on the real PATH; modify it to monkeypatch/mock
the internal helper _run (the one used by _check_command) to return a result
that indicates "pre-commit" is present (e.g., a successful which-like response)
while still setting CI=true, so _check_command exercises the "installed but
skipped due to CI" branch; ensure the mock only affects the external check and
keep the existing assertions that result == _RESULT_WARN and that the detail
contains "CI" or "skipped".
- Around line 177-186: Modify the test to assert that _run is never invoked by
monkeypatching the module-level _run used by _check_command to a stub that
raises if called (e.g., def _run_should_not_be_called(*a, **k): raise
AssertionError("_run was called");
monkeypatch.setattr(<module_where__check_command_is_defined> , "_run",
_run_should_not_be_called)), then call _check_command("gh pr view {pr} --repo
{repo}", tmp_path, pr_number=1, repo="evil; rm -rf /") and assert the returned
result is _RESULT_BLOCK and the detail contains "Invalid"; this ensures the
function returns BLOCK before any subprocess is spawned. Ensure you reference
the same _check_command and _run symbols from the module under test when
patching.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4dac8430-f801-41dc-a309-4b1c297d3296
📒 Files selected for processing (4)
contracts/OMN-7996.yamlcontracts/OMN-7998.yamlscripts/ci/run_contract_compliance_check.pytests/test_contract_compliance_check.py
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/OMN-7996.yaml
- contracts/OMN-7998.yaml
jonahgabriel
changed the title
jonahgabriel
changed the title
jonahgabriel
changed the title
…tighter precommit gate [OMN-7996, OMN-7998] - dod-001: extend gh pr view to query baseRefName; awk gate fails if base != main - dod-003: switch from raw command to test_passes check type in both contracts - _check_command: pass workspace as cwd to subprocess so --workspace is honored - CI demotion: narrow from CI=true alone to (binary absent AND in CI); installed pre-commit enforces even in CI - test: assert exact rendered shell string in substitution test; add workspace cwd test; add present-in-CI enforces test
…e-commit gate
Replace {pr}/{repo} template literals with actual values (PR 588,
OmniNode-ai/omnidash). Replace pre-commit runtime command with
file_exists check for .pre-commit-config.yaml — pre-commit is not
installed in CI runners.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_contract_compliance_check.py`:
- Around line 217-259: Update the tests to deterministically exercise the non-CI
and CI paths and to validate that the actual subprocess calls occur: in
test_check_command_precommit_missing_not_ci_warns add a monkeypatch fixture and
call monkeypatch.delenv("CI", raising=False) so CI is unset, then assert the
detail equals the exact expected message returned by _check_command for a
missing pre-commit (use the literal string produced by _check_command) instead
of a loose "skipped" containment; in
test_check_command_precommit_present_in_ci_enforces capture the patched _run
(assign the context manager to a mock like mock_run) and after calling
_check_command assert mock_run.call_count == 2 and that the second call's
args/kwargs correspond to running "pre-commit run --all-files" so you prove the
pre-commit run actually executed while CI is set, and keep the result assertion
against _RESULT_PASS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3736e663-6a15-4c68-8908-2921fcbc0e56
📒 Files selected for processing (4)
contracts/OMN-7996.yamlcontracts/OMN-7998.yamlscripts/ci/run_contract_compliance_check.pytests/test_contract_compliance_check.py
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/OMN-7998.yaml
- contracts/OMN-7996.yaml
…heck dod-002 previously only checked mergedAt existed; CodeRabbit flagged it can pass on PRs not targeting main. Now asserts both non-empty mergedAt and baseRefName==main via awk, consistent with dod-001.
- delenv CI in missing-not-ci test to pin non-CI path - assert exact detail string instead of loose "skipped" containment - capture _run calls in present-in-ci test to prove subprocess ran
…placeholder literals [OMN-7996, OMN-7998]
OMN-7998: dod-002 used mergedAt check which always BLOCKs on open PRs since
mergedAt is null. Changed to same OPEN|MERGED base-branch assertion as dod-001.
OMN-7996: dod-001 and dod-002 had {pr}/{repo} placeholder literals never
replaced with actual values. Substituted PR 586 / OmniNode-ai/omnidash.
Changed dod-004 from pre-commit run (fails in CI) to file_exists check.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_contract_compliance_check.py (1)
233-246: Consider asserting the exact detail message for consistency.This test asserts
"skipped" in detailwhile the sibling test on line 230 asserts the exact message. For consistency and to prevent false positives, consider asserting the exact detail string.Suggested tightening
assert result == _RESULT_WARN - assert "skipped" in detail + assert detail == "pre-commit check skipped (binary absent in CI)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_contract_compliance_check.py` around lines 233 - 246, Replace the loose substring check in test_check_command_precommit_absent_and_ci_warns with an exact equality assertion for the detail message: update the assertion that currently does assert "skipped" in detail to assert detail == "<use the exact detail string used by the sibling test>" so it matches the exact message produced by _check_command when pre-commit is absent (and still returns _RESULT_WARN); locate the sibling test’s exact expected string and copy it here to ensure consistency with _check_command behavior and the other test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_contract_compliance_check.py`:
- Around line 233-246: Replace the loose substring check in
test_check_command_precommit_absent_and_ci_warns with an exact equality
assertion for the detail message: update the assertion that currently does
assert "skipped" in detail to assert detail == "<use the exact detail string
used by the sibling test>" so it matches the exact message produced by
_check_command when pre-commit is absent (and still returns _RESULT_WARN);
locate the sibling test’s exact expected string and copy it here to ensure
consistency with _check_command behavior and the other test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5473df4-dd8f-4c65-b77e-986771552a52
📒 Files selected for processing (2)
contracts/OMN-7998.yamltests/test_contract_compliance_check.py
Merged
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_check_commandnow substitutes{pr}and{repo}placeholders before running any shell command, so DoD checks in contract YAML files work in GitHub Actions merge-ref (detached HEAD) context without relying on implicit git branch resolutionpre-commitcommands are demoted toWARN(notBLOCK) whenpre-commitis not installed in the runner; install it locally to run the checkcontracts/OMN-7996.yamlandcontracts/OMN-7998.yamlupdated: baregh pr view/gh pr checks --watchreplaced withgh pr view {pr} --repo {repo}/gh pr checks {pr} --repo {repo}(removes--watchwhich blocks indefinitely in CI)Affected PRs
Fixes the failing
Contract Compliance Checkgate on:Test plan
uv run pytest tests/test_contract_compliance_check.py -v— all 23 tests passpre-commit run --all-files— cleanuv run python scripts/ci/run_contract_compliance_check.py --pr 586 --repo OmniNode-ai/omnidash --contracts-dir contracts— dod-001/002/003 PASS, dod-004 WARN (pre-commit installed locally, no BLOCK)Contract Compliance CheckSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores