fix(ci): resolve checks-node-test regressions on main#63995
fix(ci): resolve checks-node-test regressions on main#63995corbin-breton wants to merge 2 commits intoopenclaw:mainfrom
Conversation
Two test failures have been breaking CI on main: 1. temp-path-guard: extensions/qa-lab/src/multipass.runtime.ts used Math.random() + Date.now() on the same line, triggering the weak randomness guardrail. Replace with crypto.randomBytes(). 2. delivery-context: the Telegram test expected the generic "channel:" prefix for delivery targets, but the bundled Telegram plugin provides resolveDeliveryTarget which returns the raw chatId. Update the test expectation to match the actual bundled plugin behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR fixes two CI regressions on Confidence Score: 5/5Safe to merge — both changes are minimal, correct, and directly verified against the actual runtime code paths. The No files require special attention. Reviews (1): Last reviewed commit: "fix(ci): resolve checks-node-test regres..." | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebf1b52284
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function createVmSuffix() { | ||
| return `${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 8)}`; | ||
| return `${Date.now().toString(36)}-${randomBytes(4).toString("hex")}`; |
There was a problem hiding this comment.
Keep VM suffix deterministic under test hooks
Using randomBytes(4) here makes createVmSuffix() nondeterministic even when tests stub Date.now() and Math.random(). In extensions/qa-lab/src/multipass.runtime.test.ts, the missing-multipass test computes expectedVmName via a separate createQaMultipassPlan() call and then filters temp directories by that prefix; after this change those two plan calls can produce different VM names, so leaked transfer dirs may no longer be detected and the test can pass falsely. Please add a controllable seam (or mock randomBytes) so test-time VM name generation stays stable.
Useful? React with 👍 / 👎.
The test harness mock for pi-embedded-helpers.js listed exports explicitly but missed the recently added sanitizeUserFacingText. Spread importActual so new exports are always included. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Closing - all fixes have been merged into main via #63917 and subsequent commits. No remaining diff. |
Summary
Three test failures have been breaking
checks-node-teston every CI run on main:extensions/qa-lab/src/multipass.runtime.tscreateVmSuffix()usedMath.random()+Date.now()on the same line, triggering the weak-randomness guardrail. Replaced withcrypto.randomBytes().src/utils/delivery-context.test.tsexpected the genericchannel:prefix for delivery targets, but the bundled Telegram plugin providesresolveDeliveryTargetwhich returns the raw chatId. Updated the test expectation to match the actual bundled plugin behavior.run.overflow-compaction.harness.tslisted mock exports explicitly but missed the recently addedsanitizeUserFacingText. SpreadimportActualso new exports are always included.Test plan
checks-node-testpasses (temp-path-guard, delivery-context, usage-reporting, incomplete-turn)🤖 Generated with Claude Code