ci: run Windows advisory on pull requests - #1028
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR introduces a reusable CI guard helper for native watcher tests and updates the Windows advisory workflow to run on pull requests with pull-request read permissions. Test suites are migrated to use the centralized guard, and workflow changes are validated in tests. ChangesWorkflow CI gating and native watcher test guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Suggested priority: P3 (only low-risk paths changed (.github/workflows/windows-advisory.yml, packages/opencode/test/github/ci-workflow.test.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
There was a problem hiding this comment.
Code Review
This pull request updates the CI workflow tests in ci-workflow.test.ts to perform more precise assertions on the parsed workflow object, verifying the push, pull request, and workflow dispatch configurations, as well as updating the expected permissions to include read access for pull requests. I have no feedback to provide as the changes are straightforward and correct.
Wrap each matrix shard's `unit` step in `.github/workflows/windows-advisory.yml` in a process-level retry (max_attempts=2) so a single transient Windows-advisory failure does not turn the advisory red, while persistent regressions stay visible. Root cause / goal - `windows-advisory` was ~40% red over the last ten `dev` runs from transient flake unrelated to merged PR diffs: PR #1043 hit Effect-TS sleep-based timing assertions in `packages/opencode/test/session/run-state.test.ts` on Windows's coarser wall clock; PR #1028 hit a Bun 1.3.14 native segfault on `windows-latest` (`watcher.node` stack frames). Linux `ci` remains the load-bearing required gate; this workflow is advisory and non-blocking. Change boundary - `.github/workflows/windows-advisory.yml`: bash `for` loop with `attempts=2`, each attempt runs in a subshell `( ${{ matrix.command }} )` so `cd packages/...` in some matrix.command values does not leak across attempts. First-attempt failure is exported via `first_exit_code` and surfaced in `$GITHUB_STEP_SUMMARY`. Recovered-on-retry and final-failed outcomes also emit run-level `::notice` / `::warning` annotations so flake stays observable in the Actions UI annotations panel, not only inside the collapsed step summary. - `packages/opencode/test/github/ci-workflow.test.ts`: dedicated workflow contract self-test `retries the Windows unit step once on transient failure` pins retry budget, subshell scoping, first-attempt-failure summary, recovery summary, `::notice` / `::warning` annotation strings, and final exit-by-last-attempt semantics. Existing self-test substrings preserved. Verification - `actionlint .github/workflows/windows-advisory.yml`: ok. - `bun test test/github/ci-workflow.test.ts` from `packages/opencode/`: 17 pass / 0 fail / 305 expect() calls. - CI on the PR: all required `ci` jobs green; `windows-advisory` green across all 5 matrix shards (app / opencode-session / opencode-config-project / opencode-server-tools / desktop). One natural rerun during the PR also exercised the retry path on a transient `actions/cache@v5` failure on `opencode-config-project` and recovered on attempt 2. Review follow-ups - P3 (workflow self-test pin): addressed in commit 30dfa2d. - 待验证 (real CI evidence of retry): partially addressed — group marker `Windows unit attempt 1 of 2` observed on runner; full recovered-on-retry path is exercised only by natural flake and remains an in-prod observation. - P2 (recovered-on-retry visibility): addressed in commit f9a2c65 with `::notice` / `::warning` run-level annotations. Residual risk - Retry hides the first-attempt failure at the check-result level. The ::notice annotation and step-summary entry preserve visibility for humans reviewing the run. Required Linux `ci` gate is untouched. - Upload unit artifacts step uploads JUnit XML from the last attempt that wrote it; on retry recovery the artifact reflects attempt 2. Acceptable for an advisory signal. Deferred work - Rewriting `Effect.sleep`-based timing in `run-state.test.ts` to deterministic primitives is deferred until after the 2026-06-15 `windows-latest` -> `windows-2025-vs2026` runner migration, since flake patterns will shift.
Summary
Restore
windows-advisoryas an advisory pull request workflow for PRs targetingdev. No related issue; this follows maintainer request to catch Windows-only regressions before merge.This also hardens the CI-only skip path for native watcher tests so Windows advisory jobs short-circuit before probing the native
@parcel/watcherbinding.Why
windows-advisorywas only running after merges todevor by manual dispatch, so Windows-specific regressions could surface only after code had already landed. Running it on PRs moves that signal earlier while keeping it outside the required merge gate.The first PR run exposed a flaky Bun/native watcher crash in the Windows advisory shard. The affected live watcher tests were already intended to skip in CI; the guard helper now checks CI before probing native watcher support.
Related Issue
None.
Human Review Status
Pending
Review Focus
Please check that the restored PR trigger remains advisory-only, that
pull-requests: readis scoped to thechangesjob for paths-filter PR metadata reads, and that the watcher CI guard skips without calling the native binding probe.Risk Notes
Non-docs PRs now consume Windows runner capacity for this advisory workflow. The docs-only skip remains in place. Permission surface expands from
contents: readtocontents: readpluspull-requests: readonly on thechangesjob sodorny/paths-filtercan read changed files on pull_request events.The watcher hardening changes only test guard selection for suites that already skip in CI. Local non-CI runs still probe native watcher support before deciding whether to run those live watcher suites.
Visible UI checklist is skipped because no UI or copy changed.
How To Verify
Screenshots or Recordings
Not applicable; no visible UI changes.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
Tests
Chores