Skip to content

fix(ci): make pre-push portable on Windows - #10574

Open
tianmind-studio wants to merge 9 commits into
BasedHardware:mainfrom
tianmind-studio:codex/windows-preflight-portability
Open

fix(ci): make pre-push portable on Windows#10574
tianmind-studio wants to merge 9 commits into
BasedHardware:mainfrom
tianmind-studio:codex/windows-preflight-portability

Conversation

@tianmind-studio

@tianmind-studio tianmind-studio commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Fixes #9724.
Fixes #10293.

Summary

  • make the single-flight runner register only host-supported signals and replace destructive Windows os.kill(pid, 0) probing
  • launch the extensionless pre-push script through Git Bash and carry one explicit Python executable across every hook boundary, including the PRE_PUSH_SKIP_PR_PREFLIGHT=1 failure-class hatch
  • contain the Windows command tree in a kill-on-close Job Object, with a bootstrap barrier that assigns the job before the real command can spawn
  • bind stale-lock identity to Windows process creation ticks so PID reuse cannot impersonate the prior owner
  • decode Git metadata as UTF-8 so native Windows locales can resolve non-ASCII worktree paths
  • make the check manifest Windows-aware and add a path-scoped native Windows workflow that executes the manifest-owned portability checks
  • preserve the mainline forwardable_signals and direct-child fallback contracts while extending Windows ownership to the full process tree
  • restore the linked-worktree repo-root fallback across all five downstream shell/Python pre-push entrypoints, so a show-toplevel exit 128 does not force contributors toward --no-verify
  • keep native process tests event-driven with pipe handshakes and WaitForSingleObject, without wall-clock polling or leaked handles

Root cause

The pre-push control plane assumed POSIX signals, process groups, executable script launching, virtualenv layout, and python3 naming at native Windows boundaries. It also let Python decode Git's UTF-8 output with the active Windows code page, so a worktree path containing non-ASCII characters could fail before any check ran.

A second gap remained after #10401: the installed hook dispatcher could recover when git rev-parse --show-toplevel failed in a linked worktree, but scripts/pre-push, scripts/pre-push-singleflight, scripts/pr-preflight, .github/scripts/preflight_runner.py, and .github/scripts/pr_preflight.py repeated the unguarded lookup and aborted later in the same chain.

Fixing only the first Windows exception or only the Makefile/dispatcher boundary would expose the next launch, liveness, decoding, root-resolution, or hatch failure. This PR closes that reachable portability chain.

Attribution

The linked-worktree root-resolution slice preserves Nik Shevchenko's original #10491 implementation through the rebased commit lineage, with Co-authored-by: Nik Shevchenko <kodjima33@gmail.com>. Its regression was adapted to the explicit Windows Python launcher introduced by this PR and registered in both manifest lanes plus native Windows CI.

Verification

  • rebased onto current origin/main (ffc13daf4c) and reran the native Windows checks
  • native Windows manifest run: pre-push-worktree-fallback, preflight-runner-portability, and check-manifest-contract -> 3 checks passed
  • native Windows: test_preflight_runner.py -> 23 tests passed with ResourceWarning promoted to an error, including a real Job Object parent/descendant termination proof, explicit hatch interpreter coverage, and runner/direct PR-preflight execution from a Chinese worktree path with PYTHONUTF8=0
  • native Windows: a temporary Git repository ran the real pre-push-singleflight -> preflight_runner -> pre-push chain with no python3 on PATH
  • native Windows: test_run_checks.py -> 33 tests passed, 1 POSIX-only fixture skipped; the real manifest subprocess path also resolved python3 to the active interpreter with an empty PATH
  • native Windows: full test_pr_preflight.py -> 26 tests passed, 3 explicit host-capability skips
  • native Windows: test-pre-push-worktree-fallback.sh passed against all five downstream root resolvers under the reproduced exit-128 condition
  • test_pre_push_ci_prediction.py -> 14 tests passed
  • scripts/test-pre-push-diff-base.sh passed under Git Bash
  • bash -n passed for all four touched shell scripts
  • Black 26.5.1 passed for the touched Python files with the repository's 120-column/no-string-normalization settings
  • actionlint 1.7.12 passed for the Windows workflow
  • python -m py_compile passed for all touched Python files
  • scripts/failure-class validate --base origin/main --head HEAD ... passed with Failure-Class: none
  • git diff --check origin/main...HEAD passed

GNU Make is not installed on the local Windows host, so scripts/test-make-setup.sh could not be rerun there. The new regression exercises every downstream root resolver directly on native Windows; Linux CI remains authoritative for the existing Make fixture and the full manifest.

The committed diff changes the manifest, so the legacy local selector intentionally attempts every manifest check. On native Windows that broad run currently stops while collecting the existing dev-harness-unit-tests because its qualification module imports POSIX-only fcntl; the path-scoped Windows workflow therefore executes the portability contracts by manifest check ID, while Linux CI remains the full-manifest authority.

Product invariants affected

none

Failure class (fixes)

Failure-Class: none

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Thanks for tightening the Windows pre-push path here. I reviewed this as workflow-sensitive CI/hook code rather than a normal app change.

What looks good:

  • The patch addresses the actual Windows portability chain instead of only papering over the missing POSIX signal constant.
  • The new runner tests cover host-specific signal selection, avoiding destructive Windows os.kill(pid, 0), PID-reuse protection via process creation ticks, explicit Bash/Python launcher boundaries, invalid UTF-8 output, and Windows-only job-object behavior where the host can exercise it.
  • The new Windows workflow is path-scoped and uses contents: read, so I do not see an obvious privilege expansion or supply-chain red flag.

I also locally ran the relevant portable checks on the PR head (45fbe17b):

  • python3 .github/scripts/test_preflight_runner.py
  • python3 .github/scripts/test_run_checks.py
  • python3 .github/scripts/run_checks.py --lane ci --base HEAD --head HEAD --platform windows --check-id preflight-runner-portability --check-id check-manifest-contract
  • bash -n scripts/pre-push scripts/pre-push-singleflight scripts/pr-preflight
  • python3 -m py_compile .github/scripts/preflight_runner.py .github/scripts/run_checks.py .github/scripts/test_preflight_runner.py .github/scripts/test_run_checks.py

All passed in my Linux review environment, and GitHub's native Windows portability check is also green. Because this adds/changes CI and pre-push control-plane behavior, I am not formally approving; a human maintainer should still review the workflow/hook surface before merge. My maintainer signal is positive pending that human workflow review.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

@tianmind-studio

Copy link
Copy Markdown
Contributor Author

#10594 is the small manifest-platform slice of this PR; if it merges first, I will rebase this branch and drop the duplicate platform lines. #10595 is complementary but independent: it fixes UTF-8 decoding in other preflight guards and is not subsumed by this runner/launcher change.

Copy link
Copy Markdown
Contributor Author

CI follow-up on refreshed head a0a4622002: every effective check for this PR passed, including the native Windows contracts, metadata preflight, backend unit suite, generated files, Dart, and Android compile. The only red lane is the unrelated macOS PTTInputDeviceProbeTests.testMainRunLoopKeepsPumpingWhileTheHALIsWedged throughput flake (ticks == 7, reads == 7) in run 30283186121.

The same test already failed on current main and the prior #10574 head with different positive counts, so this is not introduced by the Windows pre-push diff. I tracked it in #10724 and opened the one-file deterministic test fix in #10725.

Git-on-my-level added a commit that referenced this pull request Jul 27, 2026
Fixes #10724.

## What changed and why

- wait until the injected CoreAudio probe has actually entered its
wedged semaphore before checking responsiveness
- replace the fixed `> 10` timer/read throughput requirements in a 300
ms window with three explicit main-actor heartbeat completions and three
explicit `currentSnapshot` completions
- keep snapshot reads off-main so a blocking regression times out
without preventing the test from releasing and joining the parked HAL
probe
- exempt the real Swift package test path,
`desktop/macos/Desktop/Tests/`, from the user-facing changelog gate and
cover that path classification with regression cases

`testMainRunLoopKeepsPumpingWhileTheHALIsWedged` was testing a binary
liveness contract with host-throughput thresholds. Under macOS runner
contention, the main actor and snapshot path still made progress, but
the isolated suite completed too few iterations. The production contract
is that the main actor and turn-start snapshot reads both keep
completing while the HAL probe remains blocked off-main, so the new test
observes those two completions independently instead of measuring
scheduler throughput.

The changelog gate already exempts internal test paths because its
post-merge push run cannot see a PR-only `no-changelog-needed` label. It
covered the legacy lowercase `desktop/macos/tests/` directory but not
the Swift package tests under `Desktop/Tests/`, so this test-only patch
exposed the same open failure class on the actual Swift test path.

## Failure evidence

- current `main`: [run
30276084659](https://github.com/BasedHardware/omi/actions/runs/30276084659),
`reads == 10`
- unrelated #10574 head: [run
30280746072](https://github.com/BasedHardware/omi/actions/runs/30280746072),
`ticker.count == 5`, `reads == 4`
- unrelated #10574 rerun: [run
30283186121](https://github.com/BasedHardware/omi/actions/runs/30283186121),
`ticker.count == 7`, `reads == 7`

All sibling `PTTInputDeviceProbeTests` cases passed in those failures.

## Product invariants affected

none

## How it was verified

- `python3 .github/scripts/test_desktop_changelog.py` passed: 4 tests
- `python3 .github/scripts/check-desktop-changelog.py --base origin/main
--head HEAD` passed: no desktop changes require a changelog entry
- `python3 desktop/macos/scripts/check_desktop_test_quality.py` passed
- `git diff --check origin/main...HEAD` passed
- reviewed the semaphore ordering: `started` is fulfilled immediately
before `release.wait()`, main-actor heartbeat and snapshot-read
expectations settle before `release.signal()`, and `finished` is awaited
afterward

This contribution was prepared on Windows, so the local commit hook
could not run the Xcode-only `swift-format` command (`xcrun` is
unavailable). Desktop Swift CI is the authoritative compile, formatter,
and XCTest verification for the Swift test change.

## Tests

- `ChangelogRequirementTests` now covers both a top-level and nested
Swift package test path.
- The rewritten PTT test is the regression proof for the three linked
flaky runs; it fails in bounded time if either the main actor stalls or
snapshot reads block while the injected HAL probe is parked.

## Failure class (fixes)

Failure-Class: FC-push-gate-internal-path-scope

## Scoped cleanups

The changelog path exemption is a separate commit because the initial
test-only head demonstrated that relying on `no-changelog-needed` would
pass only the PR lane and could still redden the post-merge push lane.


<!-- This is an auto-generated description by cubic. -->
<a
href="https://cubic.dev/pr/BasedHardware/omi/pull/10742?utm_source=github"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img
alt="Review in cubic"
src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a>
<!-- End of auto-generated description by cubic. -->
@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Reviewed the refreshed head a0a4622 as workflow-sensitive CI/hook code.

Positive maintainer signal from my side:

  • The new linked-worktree fallback regression is focused and covers the remaining downstream shell/Python entrypoints that could still abort after fix(setup): resolve repo root without a work tree in linked worktrees #10401.
  • The Windows runner changes keep the workflow path-scoped with contents: read; I do not see a privilege expansion, supply-chain concern, or agent-instruction behavior change.
  • Focused local verification passed under a stripped environment: test_preflight_runner.py, test_run_checks.py, test_pr_preflight.py, shell syntax checks, Python compilation, and the manifest-selected Windows portability checks including pre-push-worktree-fallback.

I am still not formally approving because this touches CI/pre-push control-plane behavior and the PR already carries workflow-review / needs-maintainer-review. Also, GitHub currently shows red Desktop Swift checks; they look outside this hook portability surface, but a human maintainer should decide whether to rerun/waive those before merge.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

Exercise the real manifest subprocess path on native Windows and verify python3 resolves to the active interpreter even when PATH is empty.

Tests: python .github/scripts/test_run_checks.py (33 passed, 1 skipped).
tianmind-studio and others added 3 commits July 29, 2026 00:49
Merge the mainline forwardable-signals and send-signal behavior with Windows Job Object process-tree ownership after rebasing the portability branch. Keep PR preflight fixtures executable on native Windows.

Tests: manifest pr-preflight-contract-tests, preflight-runner-portability, and check-manifest-contract on native Windows; actionlint 1.7.12.

Failure-Class: none
Carry the complete BasedHardware#10293 root-resolution fix from BasedHardware#10491 into the portable pre-push chain, adapting its regression to the explicit Windows Python launcher contract and running it in the native Windows lane.

Tests: native Windows manifest checks pre-push-worktree-fallback, preflight-runner-portability, and check-manifest-contract; Python preflight and manifest unit suites; actionlint.

Failure-Class: none

Co-authored-by: Nik Shevchenko <kodjima33@gmail.com>
Use the selected interpreter for the PR-preflight hatch and replace wall-clock process polling with pipe and process-handle synchronization.

Failure-Class: none
@tianmind-studio
tianmind-studio force-pushed the codex/windows-preflight-portability branch from a0a4622 to 4ca8e0a Compare July 28, 2026 16:52
@tianmind-studio
tianmind-studio force-pushed the codex/windows-preflight-portability branch from 4ca8e0a to e4609d8 Compare July 28, 2026 17:01

Copy link
Copy Markdown
Contributor Author

CI is now fully green on refreshed head e4609d8aa4: the native Windows process/launcher contracts, Backend Hermetic Merge Gate, backend unit suite, Desktop Swift checks, Hygiene, Dart, Android, and generated-file checks all pass. The prior red Hermetic run was an actions/checkout fetch cancelled at the workflow's two-minute limit; I rewrote only the top commit metadata to retrigger it because fork contributors cannot rerun upstream Actions. The code diff is unchanged from 4ca8e0aeb3.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Reviewed refreshed head e4609d8aa4 as workflow-sensitive CI/hook code.

Updated maintainer signal:

  • The code diff remains focused on the Windows/native pre-push and manifest-runner portability surface: explicit Python/Bash launcher boundaries, Windows job-object process ownership, PID-reuse-safe lock checks, UTF-8 Git output handling, and linked-worktree repo-root fallback.
  • I do not see a privilege expansion, dependency/supply-chain concern, or agent-instruction behavior change. The new workflow stays path-scoped and uses contents: read.
  • Focused local verification passed under a stripped environment on this head: test_preflight_runner.py, test_run_checks.py, test_pr_preflight.py, shell syntax checks, Python compilation, and the manifest-selected Windows portability checks including pre-push-worktree-fallback.
  • GitHub now shows the relevant effective checks green, including the native Windows process/launcher contracts.

I am still not formally approving because this changes CI/pre-push control-plane behavior and the PR carries workflow-review / needs-maintainer-review; a human maintainer should make the final workflow sign-off before merge. My current signal is positive pending that human review.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-maintainer-review Needs a human maintainer to sign off before merge workflow-review Needs maintainer review for workflow, automation, hooks, or CI behavior

Projects

None yet

2 participants