Skip to content

fix(OMN-12636): dirty-canonical sweep — base rescue worktree on canonical HEAD + expand untracked dirs (also OMN-12635) - #1023

Merged
jonahgabriel merged 1 commit into
devfrom
jonah/omn-12636-dirty-canonical-handler-fixes
Jun 3, 2026
Merged

jonahgabriel merged 1 commit into
devfrom
jonah/omn-12636-dirty-canonical-handler-fixes

Conversation

@jonahgabriel

@jonahgabriel jonahgabriel commented Jun 3, 2026 •

Copy link
Copy Markdown
Contributor

Evidence-Source: OCC#2119
Evidence-Ticket: OMN-12636

OCC contracts + DoD receipts: OmniNode-ai/onex_change_control#2119

Summary

Fixes two bugs in HandlerDirtyCanonicalSweep
(src/omnimarket/nodes/node_dirty_canonical_sweep/handlers/handler_dirty_canonical_sweep.py),
the backing node for /onex:ship_dirty_canonical (epic OMN-7466).

Tickets: OMN-12636 (primary) and OMN-12635 — both High, both in Active Sprint.

Bug 1 — stale-base diff (OMN-12636)

_ship_repo created the rescue worktree from origin/<base_branch> (dev) and
then copied the canonical clone's dirty file contents onto that base.
git status --porcelain reports dirtiness relative to the clone's actual
HEAD
, not origin/<base>. When the canonical clone is behind dev these two
reference points diverge, so the copy overwrites already-merged dev work and
produces a regressive partial PR.

Evidence (2026-06-03): omnibase_infra canonical clone was 20 commits
behind dev. The node produced PR #1845 with only 11 of 31 dirty files
(+50/-329) — reverting already-merged dev work — and had to be closed and
re-shipped as #1846. The -329 minus-heavy diff is the regression signature.

Fix: resolve the canonical clone's HEAD via git rev-parse HEAD
(new _canonical_head helper) and branch the rescue worktree from that SHA,
so the copied dirty contents land on the same tree they were diffed against.
The PR base on GitHub stays <base_branch> (dev) via --base — only the
worktree's starting commit changed.

Bug 2 — untracked-directory crash (OMN-12635)

The Step 2 copy loop called read_bytes() on each dirty path. git status --porcelain reports an untracked directory as a single entry
(e.g. evidence/OMN-12584/); read_bytes() on a directory raises
[Errno 21] Is a directory and the ship aborts with status="failed".

Evidence (2026-06-03): observed on onex_change_control (had an untracked
evidence/OMN-12584/ dir) — the sweep crashed and the repo had to be shipped
manually.

Fix: when a dirty path is a directory, recurse (rglob) and copy every
contained file with preserved relative paths. No directory ever has
read_bytes() called on it.

dod_evidence

TDD red→green, both reproductions added first and confirmed failing pre-fix:

  • test_worktree_based_on_canonical_head_not_origin_base (OMN-12636) —
    asserts the worktree branches from the canonical HEAD SHA and not
    origin/dev. Failed pre-fix (origin/dev present), passes post-fix.
  • test_dirty_untracked_directory_is_expanded_to_files (OMN-12635) —
    dirty set ?? evidence/OMN-12584/; failed pre-fix with the exact
    [Errno 21] Is a directory error and repos_failed=1, now ships with
    repos_shipped=1.
  • test_ship_calls_git_in_correct_sequence updated to assert rev-parse is
    invoked and the worktree uses the canonical HEAD SHA, not origin/dev.

Verification (worktree omni_worktrees/OMN-12636/omnimarket):

  • uv run pytest tests/unit/nodes/node_dirty_canonical_sweep/... — 6 passed.
  • uv run pytest tests/ -m "not kafka" (CI's invocation) — 6011 passed,
    10 skipped. The only 4 failures are pre-existing test_datasource_postgres.py
    integration tests that connect to .201:5436; they skip cleanly when
    OMNIDASH_ANALYTICS_DB_URL is unset (as in CI) and fail locally only due to a
    stale local Postgres password — unrelated to this change.
  • uv run mypy src/omnimarket/nodes/node_dirty_canonical_sweep/ --strict — clean.
  • uv run ruff format / ruff check --fix — clean.
  • pre-commit run --files <changed> — all hooks pass.

…D + expand untracked dirs

Two bugs in HandlerDirtyCanonicalSweep (node_dirty_canonical_sweep), the
backing node for /onex:ship_dirty_canonical (epic OMN-7466):

OMN-12636 (stale-base diff): _ship_repo created the rescue worktree from
origin/<base_branch> (dev) and then copied the canonical clone's dirty file
contents onto that base. git status --porcelain reports dirtiness relative
to the clone's actual HEAD, not origin/<base>. When the clone is behind dev,
those two reference points diverge and the copy reverts already-merged dev
commits, producing a regressive partial PR. Observed 2026-06-03: omnibase_infra
was 20 commits behind dev -> PR #1845 had only 11 of 31 dirty files (+50/-329),
closed and re-shipped as #1846. Fix: resolve the canonical clone HEAD via
git rev-parse HEAD and branch the rescue worktree from that SHA, so copied
dirty contents land on the same tree they were diffed against. The PR base
branch on GitHub remains <base_branch> (dev) via --base.

OMN-12635 (untracked-directory crash): the Step 2 copy loop called
read_bytes() on each dirty path. git status --porcelain reports an untracked
directory as a single entry (e.g. evidence/OMN-12584/); read_bytes() on a
directory raises "[Errno 21] Is a directory" and the ship aborts with
status=failed. Observed 2026-06-03 on onex_change_control (shipped manually).
Fix: when a dirty path is a directory, recurse and copy every contained file
with preserved relative paths.

TDD: added test_worktree_based_on_canonical_head_not_origin_base and
test_dirty_untracked_directory_is_expanded_to_files (both fail pre-fix),
updated test_ship_calls_git_in_correct_sequence to assert the worktree
branches from the canonical HEAD SHA (not origin/dev) and that rev-parse is
invoked.
@coderabbitai

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@jonahgabriel, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 12 minutes and 48 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e052752f-49fb-4aca-81d6-792b2958bf0e

📥 Commits

Reviewing files that changed from the base of the PR and between 5f11dfd and 234ffac.

📒 Files selected for processing (2)
  • src/omnimarket/nodes/node_dirty_canonical_sweep/handlers/handler_dirty_canonical_sweep.py
  • tests/unit/nodes/node_dirty_canonical_sweep/test_handler_dirty_canonical_sweep.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jonah/omn-12636-dirty-canonical-handler-fixes

Comment @coderabbitai help to get the list of available commands and usage tips.

@jonahgabriel
jonahgabriel enabled auto-merge June 3, 2026 15:26
@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

✅ Architectural Review — PASSED

Errors: 0
Warnings: 0

What this checks

Rule Description
ARCH-TOPIC-001 No hardcoded Kafka topic strings in handler code
ARCH-DI-001 No event_bus=None bypass
ARCH-DI-002 No direct Handler instantiation outside workflow_runner/adapters
ARCH-DI-003 No reinvented DI containers
ARCH-TOPIC-002 contract.yaml topics follow `onex.{cmd

No architectural violations found.

Powered by node_pr_review_bot

@github-actions

github-actions Bot commented Jun 3, 2026 •

Copy link
Copy Markdown

✅ PR Review Bot — CLEAN

Findings: 0
Run ID: ``

Note: Reviewer models are running as stubs (OMN-7969–OMN-7972 pending). Architectural findings from Phase 1 (arch-lint) are the authoritative signal until model handlers land.


Verdict semantics

Verdict Meaning
clean No blocking findings
risks_noted Findings noted; threads passed verification
blocking_issue MAJOR/CRITICAL findings not resolved — do not merge

Powered by node_pr_review_bot — FSM pipeline with multi-model review fan-out

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

⚠️ Hostile Reviewer — DEGRADED (informational)

Blocking findings (critical/error): 0
Total findings: 0
Models succeeded: none

Note: All reviewer models failed or were unavailable. Degraded results are informational during the pilot phase (OMN-8524) and do not block merge. Error: cli_review exited non-zero (all models failed or binary error)


Gate semantics (pilot phase)

Verdict Meaning Blocks merge?
passed No critical/error findings No
blocked CRITICAL or ERROR findings found Yes
degraded All models unavailable (infra) No (pilot)

Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524)

@jonahgabriel
jonahgabriel added this pull request to the merge queue Jun 3, 2026
Merged via the queue into dev with commit 03f08fd Jun 3, 2026
51 of 53 checks passed
@jonahgabriel
jonahgabriel deleted the jonah/omn-12636-dirty-canonical-handler-fixes branch June 3, 2026 15:46
andywu42 pushed a commit to andywu42/omnimarket that referenced this pull request Jun 10, 2026
* ci(OMN-10475): wire zone-filter caller — skip heavy matrix on docs / declarative-evidence diffs

Adds a `zone-filter` job at the head of ci.yml that calls the reusable
workflow shipped in omnibase_core (OmniNode-ai/omnibase_core/.github/
workflows/zone-filter.yml@main). The four heaviest jobs gain
`needs: zone-filter` plus
`if: needs.zone-filter.outputs.docs_only != 'true'`:

  - typecheck (mypy --strict)
  - test (pytest)
  - e2e-workflow-runner (golden_chain_sweep E2E)
  - golden-chains (1000+ golden chain tests)

`lint` (ruff + topic guards), `topic-naming-lint`, `runtime-sweep`, and
`contract-compliance` continue to run on every PR. The `ci-summary`
aggregator already accepts `skipped` (line ~340 in pre-existing logic),
so docs-only PRs aggregate green even when the heavy jobs short-circuit.

DOCS zone now covers `docs/`, `standards/`, plus the declarative-evidence
trees `contracts/`, `drift/dod_receipts/`, `allowlists/`, and `.evidence/`
(per OMN-10475 — see omnibase_core PR OmniNode-ai#1023).

* ci(OMN-10475): pin zone-filter workflow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant