Stabilize cross-platform CI after preview E2E rollout - #4386
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: faf91bd6e2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| async function waitForCleanGit(cwd: string) { | ||
| async function waitForCleanGit(harness: HybridChatHarness) { | ||
| await harness.bridge.settleInFlight(); |
There was a problem hiding this comment.
On loaded Windows runners where the chat end event precedes Git finalization by 5–15 seconds, this new call throws after settleInFlight()'s default 5,000 ms, so the existing 15-second Git-status wait below is never reached. This reduces the helper's prior waiting budget and can fail precisely the late-finalizer cases being addressed; pass an explicit timeout of at least 15 seconds or coordinate both checks under one shared deadline. rules/hybrid-testing.mdL162-L165
Useful? React with 👍 / 👎.
![dyad-assistant[bot]](https://avatars.githubusercontent.com/in/3027874?s=60&v=4){kind=small}
| } | ||
|
|
||
| async function waitForCleanGit(cwd: string) { | ||
| async function waitForCleanGit(harness: HybridChatHarness) { |
There was a problem hiding this comment.
🟡 MEDIUM
settleInFlight default 5s timeout throws instead of retrying
waitForCleanGit now calls harness.bridge.settleInFlight() with no timeout argument. The bridge default is 5000ms and it THROWS on expiry ("settleInFlight timed out ... A hung handler at teardown is a real bug") rather than returning. This call sits outside the waitFor below, so it is not retried: on a loaded Windows runner where a real git commit/finalizer invoke takes longer than 5s, the helper fails hard with a misleading teardown-flavoured error instead of waiting out the finalizer. That is a new non-retryable failure vector in the same helper this PR is trying to make more tolerant of slow runners, and it is used at seven call sites in the slowest tests in the file.
💡 Suggestion: Pass an explicit budget aligned with the surrounding waitFor, e.g. await harness.bridge.settleInFlight(15_000), or move the settle inside the waitFor callback so it is retried.
| ), | ||
| ).toBeTruthy(); | ||
| }, | ||
| { timeout: 30_000 }, |
There was a problem hiding this comment.
🟡 MEDIUM
Assertion timeout raised to 30s without raising the 90s test timeout
The push-success assertion timeout goes from 20_000 to 30_000, but the enclosing it(...) still has a 90_000 budget (line 399). The waits already in this test before this point are 30s (conflict resolution) + 15s (Conflicts resolved) + 15s (Continue to Sync), on top of real setupLinkedApp clone/push work. With the new 30s the worst-case in-test waiting alone exceeds the 90s test timeout, so on exactly the loaded runner this change targets the vitest timeout fires first and the extra 10s is never usable - and the failure surfaces as a generic test timeout rather than the specific assertion.
💡 Suggestion: Raise the it(...) timeout (e.g. to 120_000) alongside the assertion timeout so the new budget can actually be consumed.
| const CALLS_DIR = resolve(__dirname, "recorded_calls"); | ||
| const FIXTURES_DIR = resolve(__dirname, "fixtures"); | ||
|
|
||
| function normalizeNewlines(content: string): string { |
There was a problem hiding this comment.
🟡 MEDIUM
Read-time newline normalization masks CRLF handling instead of pinning fixtures
The Windows failure comes from git checkout converting the recorded fixtures and call files to CRLF (the repo-wide '* text=auto' in .gitattributes). Normalizing at read time makes the replay green, but it also means the recordings are no longer byte-identical to what was captured, and the search/replace parser and processor are never exercised with CRLF input from this suite - so a genuine \r handling gap in parseSearchReplaceBlocks/applySearchReplace would stay invisible. The repo already solves this exact problem the other way for e2e snapshots ('e2e-tests/snapshots/** text eol=lf'), so this also diverges from the established convention.
💡 Suggestion: Prefer a .gitattributes entry pinning src/tests/evals/fixtures/** and src/tests/evals/recorded_calls/** to 'text eol=lf' so the recordings stay verbatim; if read-time normalization is kept, add a case that replays CRLF input end-to-end.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Test- and docs-only PR (no production code paths, no IPC surface, schema, or UI primitives touched), so the Electron boundary, The issues below are about timeout budgeting and about one fix masking rather than pinning the underlying condition. None of them are merge blockers. Issues Summary
settleInFlight budget. Git collaboration timeout. Raising only the inner assertion to 30s leaves the Eval fixture newlines. The root cause is 🟢 Low Priority Notes (5 items)
Generated by Dyadbot persona-based code review |
🎭 Playwright Test Results✅ All tests passed!
Total: 294 tests passed (12 skipped) |
github-actions
Bot
added
the
needs-human:review-issue
1 participant
Summary
Fix the cross-platform unit and E2E failures exposed by the main CI run after isolated preview testing landed. The changes make the affected tests observe the repository/runtime coordination boundaries that production already enforces and remove Windows-only text/path assumptions.
pnpm installholding the app operation slot for 65–70 seconds, so repository creation stayed queued until the original 60-second assertion timed out..git/index.lock.#skip-bugbot