Restore floating Node 26 integration coverage - #2342
Conversation
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Bound unresolved fetches without changing the affected fetch call, treat the known stall as handled evidence, and reject every unrelated nonzero test result. Exercise both versions on relevant pull requests as well as nightly. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Keep collecting samples after a slow request, only emit the known-defect discriminator after two observations, and fail a single observation as inconclusive. Bound the operations request and trim duplicate workflow narration. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Emit collected stall evidence before teardown and distinguish the single-sample inconclusive outcome from unrelated test failures in workflow diagnostics. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a new canary test file, .github/canaries/static-cache-headers-fetch.test.ts, to verify static cache-header options using global fetch. The review feedback points out that when a fetch request times out, the underlying request is not aborted, which can leave network sockets open and cause the CI process to hang. It is recommended to use an AbortController to abort the fetch request upon reaching the hard timeout.
|
On the AbortSignal suggestion: I tested that shape directly, and adding The live floating run provides the end-to-end check for this concern: one GET exceeded the 20-second bound, a second took 2,118 ms, teardown completed, the test process exited cleanly, and the workflow classified the reproduction in 1m17s: https://github.com/HarperFast/harper/actions/runs/32973938547 |
|
1 blocker: floating the Node 26 matrix entry together with the new |
|
CI caveat: Windows integration shard 2 failed twice because |
|
I don't think we need this Node 26 canary workflow. See #2025 (comment). |
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
|
I reran the faithful six-case fetch path against the official Node 26.8.0 artifact with bundled Undici 8.10.0; all six cases passed without the stall discriminator, so the dedicated canary and its reproducer are removed in this update. One external-state caveat remains: |
| # checking whether harper#2025 is still reproducing on the newest Node 26 patch. | ||
| echo "node-versions=[${{ github.event.inputs.node-version }}]" >> $GITHUB_OUTPUT | ||
| fi | ||
|
|
There was a problem hiding this comment.
What: Floating the Node 26 matrix entry (26 instead of the prior "26.5.0" pin) combined with the new NODE26_CI_MINIMUM: 26.8.0 freshness assertion (line 5, enforced in the "Verify Node.js version" steps at lines 82-85 and 126-129) currently guarantees a CI failure. actions/setup-node's version manifest resolves floating 26 to 26.7.0 today, not 26.8.0 — confirmed by the author's own workflow run (https://github.com/HarperFast/harper/actions/runs/33016752654), which failed with check-latest resolved stale Node 26.7.0; expected >=26.8.0 and never ran the Node 26 integration suite.
Why it matters: Push-to-main and the nightly schedule both use the all matrix ([22, 24, 26]), so every one of those runs will hit this failure until actions/setup-node's manifest publishes 26.8.0. Regular pull_request checks won't reveal this — that event pins the matrix to [24] only (line 53) — so this regresses main/nightly CI health in a way PR authors won't see in their own PR's checks. The PR body itself says "this PR must not merge until that external lag clears or the pin is retained," so this is a self-disclosed, verified-red state, not a hypothetical.
Suggested fix: Per the two options the author already laid out in the PR body — either hold this PR until actions/setup-node publishes a manifest entry for >=26.8.0, or retain a version pin (e.g. keep "26.5.0" or pin to "26.8.0" once available) instead of floating 26 for now.
|
Heads-up: the external blocker named in this PR's body has cleared — One thing to change before this lands, though: the I have the same change with that floor and the local Node-version differential behind it in #2378, opened before spotting the overlap here. The two overlap almost entirely — full verdict and evidence on the issue. Whichever you prefer, the other should be closed rather than both landing. 🤖 Claude Opus 5 (dispatch agent), via Claude Code. |
Remove the dedicated Node 26 fetch canary and its reproducer after an exact Node 26.8.0 / bundled Undici 8.10.0 run cleared the faithful six-case fetch path, and restore floating Node 26 in the broad integration matrix. A CI-only 26.8.0 freshness floor prevents
check-latestfrom silently reporting success on the stale runtime, but the currentsetup-nodemanifest still resolves 26.7.0, so this PR must not merge until that external lag clears or the pin is retained.For the human reviewer
26matrix entry now, accepting a red main/nightly workflow untilactions/node-versionspublishes 26.8.0, or retain the 26.5.0 pin while still removing the obsolete dedicated canary. Re-pinning is a one-line workflow change; saying no to the current form delays floating coverage but keeps CI green.package.json's supported engine range but are intentionally below the integration-CI freshness floor because they carry the investigated Undici behavior. Narrowing published support would be user-visible; leaving it broad keeps install compatibility at the cost of an untested band.Verification
26.8.0-alpha.0.0.0, so a verification-only preload normalized the version string past Harper/npm's startup guard; it did not change Undici or fetch behavior.check-latest resolved stale Node 26.7.0; expected >=26.8.0; the full Node 26 integration suite therefore did not run.npm run build,npm run lint:required, Prettier, andgit diff --checkpassed.npm run test:integration:allreached 1,834 passing tests and zero assertion failures; the command exited nonzero on the existing Ollama JSON import-attribute error, with six cancelled children and 20 skipped tests.npm run test:unit:resourcesreached 1,739 passing tests and 22 pending, with eight existing audit/transaction failures unrelated to the workflow-only diff.npm run test:unit:maincould not complete because the local test bootstrap lacked a storage path, and a retry with an explicit path stalled in an unrelated component test.Refs #2025
Complexity: medium
Review-Coverage: authored=codex; ran=gemini,claude; declined=cursor-grok,cursor-composer,domain; rounds=11 @ 87d497d
Human-Review-Need: 4 @ 87d497d