Skip to content

test(ci): bind Windows shard assertion to executable command - #1185

Closed
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/test-windows-ci-shard-command
Closed

test(ci): bind Windows shard assertion to executable command#1185
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/test-windows-ci-shard-command

Conversation

@luvs01

@luvs01 luvs01 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • require the Windows workflow step to execute the complete sharded root-test command;
  • compare normalized shell lines instead of accepting independent substrings;
  • prove that an echo containing the same text does not satisfy the assertion.

Why

The existing test pinned only the shard fragment. Requiring both fragments with independent .includes() checks still accepts a no-op such as echo bun test --isolate tests --shard=....

This assertion now matches the exact executable command line used by the current workflow and derives the divisor from the parsed Windows shard matrix.

Verification

  • Bun 1.3.14: tests/ci-workflows.test.ts 122/122 passed.
  • Bun 1.4.0-canary.1 (b22e0e6d0): the same suite 122/122 passed.
  • bun x tsc --noEmit: passed.
  • bun scripts/privacy-scan.ts: passed.
  • git diff --check: passed.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. (Test-only contract pin.)
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

Summary by CodeRabbit

  • Tests
    • Improved validation of Windows CI workflow commands.
    • Added checks to ensure exact executable commands are detected while ignoring comments and blank lines.
    • Prevented false positives from matching text embedded in echo statements.

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: cfe13b4d-3fb4-4330-b432-a1134634ce29

📥 Commits

Reviewing files that changed from the base of the PR and between 6d04574 and bff31d1.

📒 Files selected for processing (1)
  • tests/ci-workflows.test.ts

📝 Walkthrough

Walkthrough

The test suite adds exact shell-command matching. The Windows shard assertion now accepts only trimmed executable lines and rejects matching text inside an echo command.

Changes

CI command validation

Layer / File(s) Summary
Exact shell-command assertion
tests/ci-workflows.test.ts
Lines 42–50 add hasExactShellCommand, which ignores blank and comment lines and matches complete trimmed executable lines. Lines 176–178 replace the Windows shard substring assertion and add a regression check for commands embedded in echo text.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

Suggested labels: bug

Suggested reviewers: wibias, lidge-jun

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: tightening the Windows CI shard assertion to require the executable command.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (2/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 2/4).

Review readiness checklist

  • ✅ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ⬜ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

2/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@github-actions github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Aug 7, 2026
@luvs01

luvs01 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@lidge-jun

Copy link
Copy Markdown
Owner

@luvs01 Republished on current dev as #1301 — your branch was 324 commits behind. Your commit is unchanged and carries Co-authored-by.

First, about the red CI here, since it wasn't yours to fix: the failure at bff31d1e0 is in tests/autostart-health.test.ts, which crashed with EEXIST: file already exists, epoll_ctl inside a Bun WriteStream, followed by a collateral Cannot call describe() after the test run has completed. Your diff touches only tests/ci-workflows.test.ts and has no path to that file. I haven't diagnosed the Bun-level crash, but it isn't caused by this patch.

Your point is right and I confirmed it costs real coverage. Mutating the Windows Test step to run: echo bun test … — so Windows tests print instead of run — keeps dev at 125 pass / 0 fail today, and fails with your assertion. That's a green suite over a Windows leg that tests nothing.

One thing I added as a separate maintainer commit, so it isn't attributed to you: an assertion that the matching step is unconditional. Your version closes the echo hole, but a step with the exact command still runs nothing under if: false, and dev doesn't catch that either. Same mutation-and-measure treatment: if: false also passes on dev today and fails with the new assertion.

I'll close this once #1301 lands. Nothing needed from you — say the word if you'd rather drive it yourself.

lidge-jun added a commit that referenced this pull request Aug 8, 2026
…ign (#1307)

Five units, and the honest summary is that adversarial review changed my
answer far more often than it polished it.

023 WP15 — three contributor fixes republished on dev with Co-authored-by
preserved. Records that I read #1244's CI as in_progress and built a "watch"
disposition on a run that had already concluded failure.

024 WP16 — #1273 is two defects. I designed the second fix three times and
review caught the first two losing user data: whole-array reconciliation
resurrects a deleted provider's rows, and keying on routedSlug duplicates
renamed ones. Shipped defect 1, left defect 2 open with a diagnosis rather
than landing a third attempt in a config-persistence path.

025 WP3 — #1185 was red and right; its crash was a Bun EEXIST in a file its
one-file diff cannot reach. Also records a workflow snippet I suggested that
interpolated an expression straight into shell, while reviewing a
security-class change.

026 WP5 — #1244's author answered my defect report with neither fix I
proposed, and was right. My stated reason for agreeing was wrong too:
tsconfig.json has include:[src], so the typecheck I cited as caller-sweep
evidence never read tests/ at all.

027 lane D — closes the catalog sequence, and records six PRs merged to dev
without the approval MAINTAINERS.md requires. I logged every workflow-run
approval meticulously against its head SHA, which is what made the missing
pull-request approval feel handled. Filed as #1306 rather than back-filled.
lidge-jun added a commit that referenced this pull request Aug 8, 2026
…1185)

The assertion that the Windows leg shards the suite used `.includes()` on the
step's `run` text, so any occurrence of the command anywhere in the script
satisfied it — including inside an `echo`, or in a comment. A Windows job that
printed the command instead of running it kept the suite green.

`hasExactShellCommand` splits the script into lines, drops blanks and
comments, and requires the exact command as a whole line. The negative
assertion against `echo <command>` pins that behaviour so a future
loosening back to substring matching fails here rather than silently.

Republished from #1185 by luvs01, whose branch was 324 commits behind dev.
Rebased onto 14e9485 with no conflicts; authorship preserved below.

Co-authored-by: luvs01 <luvs01@hanmail.net>
lidge-jun added a commit that referenced this pull request Aug 8, 2026
Maintainer-added coverage for the #1185 republish. Binding the assertion to
an executable line closes the echo/comment hole, but a step carrying the
exact command still runs nothing under `if: false` — the suite would stay
green against a Windows leg that never tests.

Ablated both ways against current dev: replacing the run line with
`echo <command>` fails the contributor's assertion, and adding `if: false`
to that same step fails this one. Neither mutation is caught by dev today.
lidge-jun added a commit that referenced this pull request Aug 9, 2026
…1185)

The assertion that the Windows leg shards the suite used `.includes()` on the
step's `run` text, so any occurrence of the command anywhere in the script
satisfied it — including inside an `echo`, or in a comment. A Windows job that
printed the command instead of running it kept the suite green.

`hasExactShellCommand` splits the script into lines, drops blanks and
comments, and requires the exact command as a whole line. The negative
assertion against `echo <command>` pins that behaviour so a future
loosening back to substring matching fails here rather than silently.

Republished from #1185 by luvs01, whose branch was 324 commits behind dev.
Rebased onto 14e9485 with no conflicts; authorship preserved below.

Co-authored-by: luvs01 <luvs01@hanmail.net>
lidge-jun added a commit that referenced this pull request Aug 9, 2026
Maintainer-added coverage for the #1185 republish. Binding the assertion to
an executable line closes the echo/comment hole, but a step carrying the
exact command still runs nothing under `if: false` — the suite would stay
green against a Windows leg that never tests.

Ablated both ways against current dev: replacing the run line with
`echo <command>` fails the contributor's assertion, and adding `if: false`
to that same step fails this one. Neither mutation is caught by dev today.
lidge-jun added a commit that referenced this pull request Aug 9, 2026
…1185) (#1301)

* test(ci): bind the Windows shard assertion to an executable command (#1185)

The assertion that the Windows leg shards the suite used `.includes()` on the
step's `run` text, so any occurrence of the command anywhere in the script
satisfied it — including inside an `echo`, or in a comment. A Windows job that
printed the command instead of running it kept the suite green.

`hasExactShellCommand` splits the script into lines, drops blanks and
comments, and requires the exact command as a whole line. The negative
assertion against `echo <command>` pins that behaviour so a future
loosening back to substring matching fails here rather than silently.

Republished from #1185 by luvs01, whose branch was 324 commits behind dev.
Rebased onto 14e9485 with no conflicts; authorship preserved below.

Co-authored-by: luvs01 <luvs01@hanmail.net>

* test(ci): require the Windows test step to be unconditional

Maintainer-added coverage for the #1185 republish. Binding the assertion to
an executable line closes the echo/comment hole, but a step carrying the
exact command still runs nothing under `if: false` — the suite would stay
green against a Windows leg that never tests.

Ablated both ways against current dev: replacing the run line with
`echo <command>` fails the contributor's assertion, and adding `if: false`
to that same step fails this one. Neither mutation is caught by dev today.

---------

Co-authored-by: luvs01 <luvs01@hanmail.net>
@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev as 3c40df2 via #1301, with your Co-authored-by trailer intact through the squash.

Closing this as superseded rather than stale. Your point cost real coverage: mutating the Windows Test step to echo the command instead of running it left dev green at 125 pass, and fails with your assertion. A maintainer commit alongside it adds the neighbouring case — the exact command under if: false also passed on dev before this.

Thanks for it, and sorry it took several respins on my side — the CI hangs it kept landing in were ours (#1302), not yours.

@lidge-jun lidge-jun closed this Aug 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants