Repository navigation
fix(OMN-13940): unquarantine executable merge sweep cron - #1861
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCI workflows now pin uv to the workflow Python version, adjust checkout and scan behavior, and change golden-chain-live to run with local containers. The golden-chain live runner uses ChangesCI workflow hardening
Script and test updates
Merge sweep contract and entrypoint docs
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Gate
participant Postgres
participant KafkaConsumer
participant Sweep
CLI->>Gate: run_gate(analytics_url)
Gate->>Postgres: initialize_database(analytics_url)
Gate->>KafkaConsumer: start consume loop with analytics_url
KafkaConsumer->>Postgres: project_envelope / _upsert
Gate->>Sweep: launch sweep subprocess with OMNIDASH_ANALYTICS_DB_URL
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jonahgabriel
enabled auto-merge
July 4, 2026 21:28
Contributor
❓ Hostile Reviewer — UNKNOWNBlocking findings (critical/error): 0 Gate semantics (pilot phase)
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524) |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/validate-pr-contracts.yml (1)
40-44: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSet
persist-credentials: falseon checkout (flagged by zizmor).Static analysis (zizmor
artipacked) flags this checkout for not disabling credential persistence — the checkout token remains in.git/configfor the rest of the job. This job doesn't need to push back to the repo, so persisting credentials isn't needed.🛡️ Proposed fix
- name: Checkout if: github.event_name != 'merge_group' uses: actions/checkout@v7 with: fetch-depth: 1 + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/validate-pr-contracts.yml around lines 40 - 44, The Checkout step in the workflow leaves the GitHub token persisted in the local git config, which zizmor flags as unnecessary for this job. Update the actions/checkout usage in the Checkout step to disable credential persistence by setting persist-credentials to false, since this workflow only needs read access and does not push changes. Keep the fix localized to the Checkout action configuration in the workflow.Source: Linters/SAST tools
🧹 Nitpick comments (3)
scripts/tests/test-cron-merge-sweep-executable.sh (1)
55-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a trap for temp-dir cleanup.
rm -rf "${tmp_root}"only runs if execution reaches line 83; an unexpected early exit inside the function (e.g., from a future edit adding a command outsideset +e) would leak the temp directory. Atrap 'rm -rf "${tmp_root}"' RETURN(or EXIT) is more robust.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/test-cron-merge-sweep-executable.sh` around lines 55 - 84, The temp directory cleanup in the test function is fragile because rm -rf on tmp_root only runs at the end of the happy path. Add a trap in the test scope (preferably RETURN or EXIT) so the cleanup always runs even if the function exits early, and place it around the test setup in test-cron-merge-sweep-executable.sh near the tmp_root/mock_bin logic..github/workflows/validate-pr-contracts.yml (1)
46-52: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSame credential-persistence hygiene applies to this checkout.
This step also passes an explicit token for cross-repo checkout; consider disabling credential persistence here too, consistent with the fix above.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/validate-pr-contracts.yml around lines 46 - 52, The cross-repo checkout in the workflow still uses an explicit token, so the same credential-persistence cleanup should be applied here. Update the Checkout omnibase_infra step in validate-pr-contracts.yml to disable persisted credentials on the actions/checkout call, consistent with the other checkout fix, while keeping the repository and path setup unchanged..github/workflows/ci.yml (1)
237-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
uv syncin this job isn't pinned toPYTHON_VERSIONlike the others.Lines 79 and 1062 in this same file were updated to
uv sync --frozen --group dev --python "${PYTHON_VERSION}", but this pyright job's install step still uses the un-pinned form. Givenpyrightconfig.jsonpinspythonVersionto 3.12, leaving this job unpinned breaks the consistency this PR is establishing and risksuv syncresolving a different interpreter than the one pyright expects.♻️ Proposed fix
- name: Install dependencies working-directory: ${{ github.workspace }} - run: uv sync --frozen --group dev + run: uv sync --frozen --group dev --python "${PYTHON_VERSION}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 237 - 239, The pyright job’s Install dependencies step is still using an unpinned uv sync invocation, unlike the other jobs in this workflow. Update that step to pass the same PYTHON_VERSION value used elsewhere so the job resolves the interpreter consistently with pyrightconfig.json; use the existing Install dependencies block in this workflow as the place to apply the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 176-229: The no-root fallback in the pyright native runtime
libraries step can fail because it calls apt-get download libatomic1 without
refreshing package metadata first. In the branch that sets PYRIGHT_LIBATOMIC_DIR
and LD_LIBRARY_PATH, add an apt-get update before downloading the package, and
keep the existing apt-get/dpkg-deb flow in the same Ensure pyright native
runtime libraries step so stale or empty indexes don’t block locating
libatomic1.
- Around line 1391-1422: The Start Redpanda step currently writes 1048576 to
aio-max-nr unconditionally, which can reduce an already higher host-wide limit
on the shared runner. Update the shell in the Start Redpanda run block to first
read /proc/sys/fs/aio-max-nr and only write the new value when the current value
is lower, keeping the existing sudo/write-path fallback behavior intact.
In @.github/workflows/security-scan.yml:
- Around line 31-33: The dependency scan job is still being excluded for
pull_request events, which prevents the runner selection on the security-scan
workflow from ever taking effect. Update the job-level condition for
dependency-scan so PRs are allowed to run, and ensure the runner selection
expression in the job configuration still routes pull_request to
OMNI_PUBLIC_PR_RUNS_ON_JSON while non-PR runs use OMNI_TRUSTED_CI_RUNS_ON_JSON;
this will also make the Run Dependency Review step reachable on PRs.
In `@scripts/ci/run_golden_chain_live.py`:
- Around line 416-418: The startup path in run_golden_chain_live.py is using the
wrong argparse field: _parser() registers --db-dsn, so parse_args() provides
args.db_dsn, not args.analytics_url. Update the analytics URL lookup in
run_gate() to use the existing parsed attribute from _parser(), keeping the
_require() validation and the OMNIDASH_ANALYTICS_DB_URL or DATABASE_URL fallback
message intact.
- Around line 145-160: The initialize_database retry loop still allows a single
psycopg2.connect call to hang past the overall deadline, so add a per-connection
timeout at the connection site in initialize_database. Update the
psycopg2.connect(analytics_url) call to include a connect_timeout so each
attempt is bounded, while keeping the existing retry and last_error handling
intact.
---
Outside diff comments:
In @.github/workflows/validate-pr-contracts.yml:
- Around line 40-44: The Checkout step in the workflow leaves the GitHub token
persisted in the local git config, which zizmor flags as unnecessary for this
job. Update the actions/checkout usage in the Checkout step to disable
credential persistence by setting persist-credentials to false, since this
workflow only needs read access and does not push changes. Keep the fix
localized to the Checkout action configuration in the workflow.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 237-239: The pyright job’s Install dependencies step is still
using an unpinned uv sync invocation, unlike the other jobs in this workflow.
Update that step to pass the same PYTHON_VERSION value used elsewhere so the job
resolves the interpreter consistently with pyrightconfig.json; use the existing
Install dependencies block in this workflow as the place to apply the change.
In @.github/workflows/validate-pr-contracts.yml:
- Around line 46-52: The cross-repo checkout in the workflow still uses an
explicit token, so the same credential-persistence cleanup should be applied
here. Update the Checkout omnibase_infra step in validate-pr-contracts.yml to
disable persisted credentials on the actions/checkout call, consistent with the
other checkout fix, while keeping the repository and path setup unchanged.
In `@scripts/tests/test-cron-merge-sweep-executable.sh`:
- Around line 55-84: The temp directory cleanup in the test function is fragile
because rm -rf on tmp_root only runs at the end of the happy path. Add a trap in
the test scope (preferably RETURN or EXIT) so the cleanup always runs even if
the function exits early, and place it around the test setup in
test-cron-merge-sweep-executable.sh near the tmp_root/mock_bin logic.
🪄 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: c5d1a989-2a88-42df-bc44-3bcf472a11b8
📒 Files selected for processing (9)
.github/workflows/ci.yml.github/workflows/hostile-reviewer.yml.github/workflows/no-faked-boundary.yml.github/workflows/onex-schema-compat.yml.github/workflows/security-scan.yml.github/workflows/validate-pr-contracts.ymlscripts/ci/run_golden_chain_live.pyscripts/cron-merge-sweep.shscripts/tests/test-cron-merge-sweep-executable.sh
💤 Files with no reviewable changes (1)
- scripts/cron-merge-sweep.sh
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
196-220: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLocal extraction fallback exits 0 without verifying
libatomic.so.1was actually found.Unlike the package-manager path (Line 232,
has_libatomiccheck), the no-root/no-sudoapt-get download+dpkg-deb -xfallback unconditionally writesPYRIGHT_LIBATOMIC_DIR/LD_LIBRARY_PATHand exits 0 (Line 216), even iffinddidn't locatelibatomic.so.1*(e.g.found_dirends up empty). That silently reports success while leaving a brokenLD_LIBRARY_PATHfor later steps, deferring the failure to a confusing pyright crash instead of a clear step failure here. Also noteGITHUB_ENVwrites only apply to subsequent steps, so any added verification here needs a locally-scoped check, not a re-read ofPYRIGHT_LIBATOMIC_DIR.🐛 Proposed fix to verify extraction before exiting 0
found_dir="$(find "${lib_dir}" -name 'libatomic.so.1*' -print -quit | xargs dirname)" + if [ -z "${found_dir}" ] || [ ! -d "${found_dir}" ]; then + echo "::error::libatomic.so.1 could not be located after local apt-get download/extraction" + exit 1 + fi { echo "PYRIGHT_LIBATOMIC_DIR=${lib_dir}" echo "LD_LIBRARY_PATH=${found_dir}:${LD_LIBRARY_PATH:-}" } >> "${GITHUB_ENV}" exit 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 196 - 220, The local extraction fallback in the CI workflow currently exits successfully even if `libatomic.so.1` was not found after `apt-get download` and `dpkg-deb -x`. Update the fallback block in the `libatomic` setup logic to verify `found_dir` is non-empty and that the extracted library actually exists before writing `PYRIGHT_LIBATOMIC_DIR` and `LD_LIBRARY_PATH` to `GITHUB_ENV`. If the check fails, emit the same clear error path and exit nonzero instead of returning success.
🧹 Nitpick comments (2)
tests/ci/test_golden_chain_live_runner.py (1)
66-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the retry/failure paths of
initialize_database.This test only exercises the immediate-success path. The rewritten
initialize_databaseretries onpsycopg2.OperationalErrorup to a 30s monotonic deadline before raisingRuntimeError(scripts/ci/run_golden_chain_live.py:146-161) — that retry/backoff behavior is the core change in this cohort and is currently untested. Afake_connectwith a side-effect list (raiseOperationalErroronce, then succeed) plus monkeypatchingtime.sleep(to avoid a real 2s wait) would cover the retry branch cheaply; a variant that always raises would cover the deadline-exhaustion →RuntimeErrorbranch.🧪 Example additional test for the retry path
def test_initialize_database_retries_on_operational_error( monkeypatch: pytest.MonkeyPatch, ) -> None: attempts: list[int] = [] def fake_connect(*args: object, **kwargs: object) -> FakeConnection: attempts.append(1) if len(attempts) == 1: raise psycopg2.OperationalError("not ready") return FakeConnection() monkeypatch.setattr(psycopg2, "connect", fake_connect) monkeypatch.setattr(runner.time, "sleep", lambda _seconds: None) runner.initialize_database("postgresql://example") assert len(attempts) == 2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ci/test_golden_chain_live_runner.py` around lines 66 - 106, Add coverage for the new retry and failure behavior in initialize_database, since the current test only checks the immediate-success path. Add a test around runner.initialize_database that monkeypatches psycopg2.connect to raise psycopg2.OperationalError once and then succeed, and monkeypatch runner.time.sleep so the retry path runs without delay. Add a second test where psycopg2.connect keeps failing until the monotonic deadline is exceeded, and assert initialize_database raises RuntimeError; this should reference the same initialize_database and psycopg2.connect symbols so the retry loop and deadline handling are exercised..github/workflows/ci.yml (1)
180-191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant duplicate check.
Lines 189-191 duplicate the exact ldconfig check already covered by
has_libatomic()at Line 181. Callinghas_libatomicdirectly would avoid the near-duplicate logic.♻️ Proposed simplification
- if command -v ldconfig >/dev/null 2>&1 && ldconfig -p | grep -q 'libatomic.so.1'; then - exit 0 - fi + if has_libatomic; then + exit 0 + fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 180 - 191, The CI workflow contains a redundant libatomic availability check after defining has_libatomic(); replace the repeated ldconfig test with a direct call to has_libatomic so the logic is centralized. Update the shell block in the workflow to use the existing has_libatomic function instead of duplicating its check, keeping the rest of the fallback logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 196-220: The local extraction fallback in the CI workflow
currently exits successfully even if `libatomic.so.1` was not found after
`apt-get download` and `dpkg-deb -x`. Update the fallback block in the
`libatomic` setup logic to verify `found_dir` is non-empty and that the
extracted library actually exists before writing `PYRIGHT_LIBATOMIC_DIR` and
`LD_LIBRARY_PATH` to `GITHUB_ENV`. If the check fails, emit the same clear error
path and exit nonzero instead of returning success.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 180-191: The CI workflow contains a redundant libatomic
availability check after defining has_libatomic(); replace the repeated ldconfig
test with a direct call to has_libatomic so the logic is centralized. Update the
shell block in the workflow to use the existing has_libatomic function instead
of duplicating its check, keeping the rest of the fallback logic unchanged.
In `@tests/ci/test_golden_chain_live_runner.py`:
- Around line 66-106: Add coverage for the new retry and failure behavior in
initialize_database, since the current test only checks the immediate-success
path. Add a test around runner.initialize_database that monkeypatches
psycopg2.connect to raise psycopg2.OperationalError once and then succeed, and
monkeypatch runner.time.sleep so the retry path runs without delay. Add a second
test where psycopg2.connect keeps failing until the monotonic deadline is
exceeded, and assert initialize_database raises RuntimeError; this should
reference the same initialize_database and psycopg2.connect symbols so the retry
loop and deadline handling are exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1813e349-51a6-4768-bbec-c86a54bea1fa
📒 Files selected for processing (8)
.github/workflows/ci.yml.github/workflows/hostile-reviewer.yml.github/workflows/security-scan.yml.github/workflows/validate-pr-contracts.ymlscripts/ci/run_golden_chain_live.pyscripts/tests/test-cron-merge-sweep-executable.shtests/ci/test_golden_chain_live_runner.pytests/hooks/test_hostile_reviewer_workflow.py
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/hostile-reviewer.yml
- scripts/tests/test-cron-merge-sweep-executable.sh
- .github/workflows/security-scan.yml
- scripts/ci/run_golden_chain_live.py
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/hooks/test_ci_postgres_workflow.py (2)
241-299: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPython-pin coverage omits
validate-skill-aspiration.yml.
test_contract_compliance_pins_uv_python,test_no_faked_boundary_pins_uv_python, andtest_plugin_compat_pins_uv_pythonenforce the pin per-workflow, andtest_ci_uv_sync_steps_pin_pythonsweeps every job inci.yml, but no test targetsvalidate-skill-aspiration.yml, which is the one workflow in this cohort that doesn't pin--python(see comment on that file). Adding an equivalent assertion here would have caught it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/hooks/test_ci_postgres_workflow.py` around lines 241 - 299, The current workflow coverage checks several YAML files for pinned uv python usage, but it never validates validate-skill-aspiration.yml, so the missing --python pin slips through. Add a new test in test_ci_postgres_workflow.py alongside test_contract_compliance_pins_uv_python and test_plugin_compat_pins_uv_python that loads validate-skill-aspiration.yml, locates the relevant install step with _step, and asserts the uv sync/install command includes --python "${PYTHON_VERSION}".
178-219: 🎯 Functional Correctness | 🔵 TrivialAssertions accurately mirror the new
ci.ymlgolden-chain-live job.Concurrency, service, Redpanda options, and Postgres lifecycle assertions match the workflow content correctly. Note that if the container-name race condition flagged in
ci.yml(Lines 1383-1419) is fixed by parameterizing the container name, the exact-string assertions here (Lines 203, 206, 218) will need to be updated accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/hooks/test_ci_postgres_workflow.py` around lines 178 - 219, Update the `test_ci_postgres_workflow` assertions around the `Start golden-chain Postgres` and `Stop golden-chain Postgres` steps so they no longer depend on the hard-coded container name if `ci.yml` is changed to parameterize it. Keep the existing checks for the step structure and PostgreSQL lifecycle behavior, but reference the container-name symbol or derived value used by the workflow instead of asserting the literal `omniclaude-golden-chain-postgres` string in `start_step["run"]` and `cleanup_step["run"]`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 1383-1419: The golden-chain Postgres container name is host-global
and can collide across parallel jobs on the same self-hosted runner. Update the
workflow’s Start/Stop/logging steps to use a run-scoped container name derived
from `${{ github.run_id }}` instead of `omniclaude-golden-chain-postgres`, and
keep the `docker rm -f`, `docker port`, and `docker logs` calls consistent with
that name. Also update the corresponding expectations in
`tests/hooks/test_ci_postgres_workflow.py` to match the new unique container
name.
In @.github/workflows/validate-skill-aspiration.yml:
- Around line 71-82: The retry loop in the workflow still calls uv sync without
the Python version pin used elsewhere, so update the sync step in
validate-skill-aspiration.yml to pass the same --python "${PYTHON_VERSION}"
value already set by the Set up Python step. Keep the retry behavior in place,
but make the uv sync invocation consistent with the pinned pattern used in
plugin-compat-gate.yml and the other cohort workflows so this step uses the same
deterministic interpreter.
In `@contracts/OMN-13940.yaml`:
- Around line 34-45: The command in the checks block is using a folded YAML
scalar, which collapses the Python heredoc into invalid single-line text. Change
the `check_value` in the `checks` entry to a literal block scalar so the
embedded script keeps its line breaks intact; this applies to the heredoc
starting with `python - <<'PY'` and the rest of the script in the same block.
---
Nitpick comments:
In `@tests/hooks/test_ci_postgres_workflow.py`:
- Around line 241-299: The current workflow coverage checks several YAML files
for pinned uv python usage, but it never validates
validate-skill-aspiration.yml, so the missing --python pin slips through. Add a
new test in test_ci_postgres_workflow.py alongside
test_contract_compliance_pins_uv_python and test_plugin_compat_pins_uv_python
that loads validate-skill-aspiration.yml, locates the relevant install step with
_step, and asserts the uv sync/install command includes --python
"${PYTHON_VERSION}".
- Around line 178-219: Update the `test_ci_postgres_workflow` assertions around
the `Start golden-chain Postgres` and `Stop golden-chain Postgres` steps so they
no longer depend on the hard-coded container name if `ci.yml` is changed to
parameterize it. Keep the existing checks for the step structure and PostgreSQL
lifecycle behavior, but reference the container-name symbol or derived value
used by the workflow instead of asserting the literal
`omniclaude-golden-chain-postgres` string in `start_step["run"]` and
`cleanup_step["run"]`.
🪄 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: afed806a-c32f-45a6-842a-adb50ea9666e
📒 Files selected for processing (12)
.github/workflows/ci.yml.github/workflows/no-faked-boundary.yml.github/workflows/plugin-compat-gate.yml.github/workflows/validate-skill-aspiration.ymlcontracts/OMN-13940.yamlplugins/onex/hooks/scripts/error-guard.shplugins/onex/skills/merge_sweep/SKILL.mdplugins/onex/skills/merge_sweep/prompt.mdscripts/ci/run_golden_chain_live.pytests/ci/test_golden_chain_live_runner.pytests/hooks/test_ci_postgres_workflow.pytests/integration/skills/merge_sweep/test_merge_sweep_integration.py
✅ Files skipped from review due to trivial changes (1)
- plugins/onex/hooks/scripts/error-guard.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/no-faked-boundary.yml
- tests/ci/test_golden_chain_live_runner.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 1377-1389: The Redpanda startup in the CI workflow is racing with
other processes because the port chosen by the Python socket probe is released
before docker run binds it. Update the Redpanda setup block in the workflow to
retry the docker run/port allocation sequence when the bind fails, using the
existing kafka_port assignment and GOLDEN_CHAIN_REDPANDA_CONTAINER setup as the
retry scope so shared-runner port collisions are handled gracefully.
🪄 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: f4db7dbb-a2e0-4ef2-9247-c7fda9185113
📒 Files selected for processing (5)
.github/workflows/ci.yml.github/workflows/onex-schema-compat.yml.github/workflows/validate-skill-aspiration.ymlcontracts/OMN-13940.yamltests/hooks/test_ci_postgres_workflow.py
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/OMN-13940.yaml
- .github/workflows/validate-skill-aspiration.yml
- tests/hooks/test_ci_postgres_workflow.py
jonahgabriel
force-pushed
the
jonah/omn-13940-unquarantine-merge-sweep
branch
from
July 5, 2026 19:47
c5bd508 to
4bbe4b6
Compare
jonahgabriel
disabled auto-merge
July 8, 2026 17:01
jonahgabriel
added a commit
that referenced
this pull request
Jul 9, 2026
…rity Gate) on GitHub-hosted runners so they never wedge under self-hosted saturation (#1870) ## Summary Fixes the permanent-wedge class in `OMN-14127`: the required branch-protection contexts **`CI Summary`** and **`Security Gate`** never post any check-run under self-hosted fleet saturation, leaving PRs `BLOCKED` forever with 0 failing / 0 pending checks and no auto-recovery. Closes OMN-14127 (workflow-structure half; runner-fleet capacity half is OMN-13932 / OMN-14080). ## Verified root cause (evidence, not assumption) A `needs`-gated job gets **no GitHub check-run at all** until its `needs` reach a terminal state. Confirmed live: - **Required contexts are absent, not failing.** On queued CI runs (`gh pr checks` on #1867/#1863/#1861) `CI Summary` / `Security Gate` / `Tests Gate` / `Quality Gate` produce **zero rows** — never `pending`, never `skipped`. Standalone single-job contexts (`contract-validation`, `deploy-gate`) show `pending` because they instantiate immediately (no `needs`). - **The aggregators either stall-then-cancel or never instantiate.** Per the ticket trace (run `28839961159`): the `if: always()` gates (`quality-gate`/`security-gate`) became eligible when their upstreams cancelled, then **sat queued on the self-hosted fleet until the whole run was cancelled ~2h47m later — posting no terminal status**; and `ci-summary`/`tests-gate` were **never instantiated** because `quality-gate` never terminalized before run-cancellation. - Result: a required context that never reports → GitHub keeps `mergeStateStatus: BLOCKED` indefinitely with no timeout/retry. Note: the ticket title says `CI Summary`/`Tests Gate`, but the **live** required contexts on `dev` branch protection are `CI Summary` and **`Security Gate`** (`Tests Gate` is not required). Verified via `gh api repos/OmniNode-ai/omniclaude/branches/dev/protection/required_status_checks`. ## Fix — relocate the result-reading aggregation layer to GitHub-hosted `ubuntu-latest` These jobs do only static / result-reading work (`echo` + `needs.*.result`) with no LAN/service dependency, so they never need a scarce self-hosted slot. Moving them off the fleet means they get a runner immediately and reach a terminal state as soon as their `needs` resolve — instead of stalling in the queue and being cancelled. | Job | `name:` (context) | Why moved | |-----|-------------------|-----------| | `ci-summary` | **CI Summary** (required) | Emits the required context; hosted + `if: always()` → always terminal once its gate `needs` resolve | | `security-gate` | **Security Gate** (required) | Emits the required context; whole subtree now hosted | | `security-python` | Python Security Scan | Security Gate leaf — pure `bandit` static scan, hosted-safe → makes the Security Gate subtree fully starvation-immune | | `detect-secrets` | Secret Detection | Security Gate leaf — pure `detect-secrets` static scan, hosted-safe | | `quality-gate` | Quality Gate | Feeds CI Summary | | `tests-gate` | Tests Gate | Feeds CI Summary | - **No context/job name changed** (byte-for-byte). A rename would itself name-mismatch branch protection and wedge every PR. - **`if: always()` preserved** on all gate aggregators; verdict logic untouched. - **Heavy jobs are NOT moved** — `test` (matrix), `hooks-tests`, `golden-chain-live`, the 35 quality leaves, etc. stay on the self-hosted fleet. Only the lightweight aggregation layer + the two static security leaves move. - **No branch-protection / `required_status_checks` change** in this PR (operator-gated step 2 — see below). ## Residual (honest scope boundary) `CI Summary` still `needs` the self-hosted quality/tests **leaf** subtree. If those leaves sit `queued` indefinitely (never even cancel), `quality-gate`/`tests-gate` — and therefore `CI Summary` — still cannot instantiate. That is the runner-fleet **capacity** problem (OMN-13932 / OMN-14080), not a workflow-structure problem. This PR fully removes the *aggregation-layer* stall (the observed failure in the ticket trace: gates sitting queued and getting cancelled) and makes `Security Gate` fully starvation-immune (its entire subtree is now hosted). Full `CI Summary` starvation-immunity would require either moving its whole leaf subtree hosted or converting it to a no-`needs` poller — flagged as a possible fast-follow, not done here. ## Local verification - `python3 -c "import yaml; yaml.safe_load(...)"` → parses, 60 jobs. - `actionlint .github/workflows/ci.yml` → 5 findings, identical to `origin/dev` baseline (all pre-existing shellcheck style/info in untouched `run:` blocks) — **zero new findings**. - `pre-commit run --files .github/workflows/ci.yml` → exit 0, 0 failures (incl. `ban-hardcoded-runner-ip`, concurrency-primitive-guard, self-gating-workflows). - pre-push hooks (mypy / pyright / ONEX validation / exports) → passed. ## Evidence <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated multiple CI jobs to run on GitHub-hosted runners to improve reliability when self-hosted capacity is constrained. * Adjusted CI gate result aggregation to consistently use a hosted environment. * Reworked CI summary evaluation to use fail-closed polling with bounded time, producing more accurate success/pending/failure verdicts. * **Tests** * Added unit and CLI tests covering the new CI summary verdict logic and run-attempt filtering. * **Chores** * Increased HTTP timeout for the no-faked-boundary workflow to improve stability. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Evidence-Ticket: OMN-14127 Evidence-Source: OCC#3722
jonahgabriel
force-pushed
the
jonah/omn-13940-unquarantine-merge-sweep
branch
from
July 9, 2026 13:26
62bba4d to
457f30e
Compare
This was referenced Jul 10, 2026
jonahgabriel
added a commit
that referenced
this pull request
Jul 10, 2026
…x + routine) (#1879) ## omniclaude dev→main promotion Promotes `dev` (5fa434d) to `main` (currently e614e57). 9 commits, all routine previously-merged/reviewed ci/build/docs/fix work. The load-bearing change is the OMN-14244 deploy-gate matcher fix, which callers (omnibase_core/omnibase_infra/omnimarket) resolve from omniclaude `@main` — this promotion is required to unblock omnibase_infra#2245 (OMN-14190 Piece-4). Promotion branch tree equals `origin/dev` (`-s ours --allow-unrelated-histories` merge of `origin/main` for mergeability only — `origin/main` and `origin/dev` have no common git ancestor in this repo's history; verified `git diff origin/dev` on the promotion branch is empty). dev-head (5fa434d) is an ancestor of this branch so the cited OCC receipt binds. Tracked under a dedicated promotion ticket (OMN-14249, following the OMN-13828 promotion-ticket precedent) so the OCC receipt binds cleanly to this PR's number without superseding the already-merged OMN-14244/OMN-14190 contracts. Branch renamed from an earlier `promotion/omn-14244-...` attempt (#1878, closed) because the repo's DoD Evidence Check hard gate derives its required ticket from the branch name. ### Commits (origin/main..origin/dev) - `5fa434d06` fix(OMN-14244): narrow deploy-gate runtime-path matcher (stop false-positiving non-deploy CLIs, catch real deploy CLIs) (#1876) - `1b74ec1f0` feat(OMN-14190): wire OCC stamp/validate pre-commit hook (#1875) - `62308da2e` docs(OMN-14105): genericize onex skills — strip internal operating detail for public exposure (#1869) - `60cfec6fd` ci(OMN-14176): keep hostile reviewer on dev torch-free path (#1873) - `df599a05c` docs(OMN-13961): handoff skill writes canonical docs/handoff/ (singular) not docs/handoffs/ (#1864) - `8e19db1b2` build(OMN-14178): bump omniintelligence pin to torch-free base (post-#768 rl split) (#1874) - `a6272b37d` ci(OMN-13502): flip no-faked-boundary gate report-only -> enforcing (#1860) - `b94d846db` ci(OMN-13902): sibling-lock-refresh workflow (fan-out) (#1857) - `12fe25f4d` fix(OMN-13940): unquarantine executable merge sweep cron (#1861) ### Note on `a6272b37d` (no-faked-boundary: report-only -> enforcing) This is a hardening flip, not a behavior regression — omniclaude's self-scan was burned to zero findings before the flip (OMN-13500/#1849) and the job is not yet a required branch-protection context (documented as a deliberate follow-up in #1860's own body), so it cannot wedge this or any other PR. ### Why this is not a prod/runtime action This is a `dev -> main` release-branch promotion for the omniclaude repo (a Claude Code plugin repo). It does not touch `.201`, any prod compose lane, or any live runtime. Operator-authorized (OMN-14190 Piece-4 chain). promotion-receipt: OCC-3791 Evidence-Source: OCC#3791 Evidence-Ticket: OMN-14249 Evidence-Commit: 40b994cd457bdd5cc71b123c8787114ea891c0ce Refs OMN-14249, OMN-14244, OMN-14190, OMN-13502. Supersedes #1877/#1878 (closed). --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: jonahgabriel <jonahgabriel@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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
Unblocks the periodic merge-sweep cron entrypoint by removing the stale normal-execution quarantine that exited
64before the runtime-backed sweep could dispatch.Changes
scripts/cron-merge-sweep.shwhen executed directly by launchd/cron.Test plan
bash scripts/tests/test-cron-merge-sweep-executable.shbash scripts/tests/test-merge-sweep-ledger.shbash scripts/tests/test-queue-heal.shuv run pytest tests/ -m unit(not run; shell cron fix has focused shell tests)uv run pre-commit run --all-filesuv run mypy src/ --strict(pre-push mypy passed)Type safety checklist
metadata["key"]ormetadata.get("key")string literal access on Pydantic model fieldsmetadata: dict[str, Any]fields without TypedDict or# ONEX_EXCLUDE:commentexcept Exception— must use narrowed type, or minimal-scope boundary withlogger.exception(...)+ degrade comment, or typed wrap/re-raiseRelated issues
Closes OMN-13940
Evidence-Source: OCC#3611
Evidence-Ticket: OMN-13940
Summary by CodeRabbit
Bug Fixes
Documentation
Tests