Repository navigation
Conversation
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
Contributor
|
Codex review: blocked before merge. Reviewed September 5, 2026, 3:05 PM ET / 19:05 UTC. ClawSweeper reviewWhat this changesMigrates development and CI testing to Vitest 5, updates reporting and fixture lifecycles, and repairs large-image data URL decoding. Merge readiness⛔ Blocked before merge - 4 items remain The migration remains necessary: current main still uses Vitest 4.1.11. No concrete introduced defect was found, and the recorded final TUI acceptance resolves the previous missing-scenario concern. Priority: P2 Review scores
Verification
How this fits togetherOpenClaw’s test tooling turns developer selections and QA scenarios into native test executions, reports, and CI results. This migration changes the runner underneath those workflows while preserving test identity and cleanup guarantees. flowchart TD
A[Developer test selection] --> C[Test launcher]
B[QA scenarios] --> C
C --> D[Vitest 5 projects and workers]
D --> E[Native reports and cleanup]
E --> F[Identity and completion checks]
F --> G[Published results or visible failure]
Decision needed
Why: The final TUI requirement is satisfied, but this reviewer could not retrieve the complete pinned cache/project-loader implementations; accepting the existing direct-inspection receipts requires explicit human judgment. Before merge
Agent review detailsSecurityNone. PR surfacePR surface statistics unavailable: the file list is incomplete. Review metrics
Stored data modelPersistent data-model change detected: Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep one native Vitest 5 execution and reporting flow, preserving project identity, cache generations, bounded cleanup, and the documented criteria for retiring temporary patches. Do we have a high-confidence way to reproduce the issue? Not applicable to the migration as a whole; the supplied evidence records native before/after failures for its component repairs, and this review did not execute tests. Is this the best way to solve the issue? Yes, the coordinated native-runner migration is preferable to a version-only bump or parallel reporting implementation; complete upstream cache/project inspection remains the stated review limitation. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 7f09c3127d36. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (16 earlier review cycles; latest 8 shown)
|
steipete
force-pushed
the
steipete/vitest5-migration-d2657a
branch
from
September 3, 2026 09:21
02723d1 to
5ca1a9f
Compare
steipete
marked this pull request as ready for review
September 3, 2026 09:24
Contributor
Dependency GuardThis PR changes dependency-related files. Maintainers should confirm these changes are intentional. Changed files:
Maintainer follow-up:
|
Contributor
Dependency graph changes notedThis PR includes dependency graph changes. The dependency guard is informational because the PR author is a repository admin, a member of
Security review is still recommended before merge when the dependency graph change is intentional. |
Contributor
Security-sensitive changes notedThis PR includes security-sensitive file changes. The guard is informational because the PR author is a repository admin or a member of
Changed files:
Security review is still recommended before merge when the change is intentional. |
steipete
added a commit
that referenced
this pull request
Sep 3, 2026
Select same-name cross-run replacements using validated check start times, not workflow creation IDs. Preserve same-run queued reconciliation and keep ambiguous recency, unique failures, and later cancellations visible. Verified 131 native watcher cases, 47 synthetic/captured classifier assertions, script and root-test types, focused guards, and isolated Codex autoreview. Read-only replay of PR #137202 now reports its real CI failures without the superseded dispatch cancellation.
steipete
added a commit
that referenced
this pull request
Sep 3, 2026
Clear direct-DM mock history per case and assert the current ingress call. Await submenu opening before pausing its hide, and await the dropdown completed-show event before geometry assertions. Popover membership and Lit update completion precede asynchronous positioning. Retain overlap, hit-testing, lifecycle and failure assertions without larger delays or timeouts. The direct-DM file passes all five cases; complete browser consumer and owner files pass 57 cases after the captured geometry failure. Shared prerequisites for the v4/v5 comparison in #137202.
Long temporary paths legitimately consume the inline preview budget. Verify all available provider prose in the recovered spill, enforce the combined text/title/warning cap, and clean both spilling fixtures in finally. Keep the 800-character budget and all test cases unchanged. A 154-character spill path reproduces the old failure (11 passing, 1 failing); the same complete file passes all 12 cases after the repair. Apply this shared fixture repair to both Vitest comparison references.
Large inline data: URLs could throw "RangeError: Maximum call stack size exceeded" from RegExp.exec inside decodeDataUrl when V8 disables regexp optimization: past ~1MiB of cumulative generated regexp code the fallback engine's quantified whole-payload matches need input-proportional backtrack storage capped at 64MiB. Parse the header and payload with delimiter scans and a negative character class instead; accepted grammar, error precedence, and the decoded-size guard are unchanged. Observed as the sole failure in the original agents-tools 110-file serial order: the oversized-inline reference case received a RangeError instead of the size-limit rejection.
The postinstall inventory suite handed npm the live node_modules/typescript path as a directory install source; npm re-modes bin entries of directory sources, so every run chmodded tsc/tsserver through the pnpm store hardlinks and mutated installed-file identity (ctime) for the whole checkout. Copy the package once per suite (dereferenced) into a temp root and install from the copy, leaving the real store untouched.
The isolated-typescript helper landed without its two call sites, so npm still received the live node_modules/typescript path as a pack input and install source and kept re-moding the pnpm store's tsc/tsserver bins on Linux (macOS npm does not, which hid it locally). Route both the packed bundle source and the offline install source through the helper; a paired box run shows the store ctime stable with this change and mutating without it.
Align the root, UI, browser, and coverage packages on 5.0.0 and preserve the existing approved dependency fixes with verified installed target hashes. Remove the RC-only pending-queue salvage while retaining ordered mock resolution and file cleanup. Own the Chromium root in test.root so standalone and aggregate entrypoints discover the same files. Add a native regression that fails with the previous zero-file aggregate discovery. Use native TestRunner for the injected shutdown fault instead of importing unrelated OpenClaw cleanup code, and resolve reporter contracts without temporary proxy configs. Root test and tooling typechecks pass; native browser discovery matches across all four entrypoints. Linux CI-parity and paired performance validation remain required before landing.
Create private fixture homes and state directories at their producer, matching the native service owner instead of letting a permissive umask turn sealed or inspection-failure scenarios into unsafe-permissions failures. Assert Git executable semantics for reused checkout files, retaining exact mode checks for downloaded replacements. Git and the workspace manifest preserve executable status rather than host-dependent read/write permission bits. Content, blob counts, symlinks, and Git identity assertions remain unchanged. Controlled Linux reproduction failed under umask 002 and passed under 022 before repair. The repaired fixture and network suites pass under both masks with the test host's missing IPv6 localhost mapping corrected. No product behavior, schema, or validation deadline changed.
Capture Memory cleanup handlers independently of mock history, assert the native browser project root, and bind live browser disclosures to their run and tool identifiers. Remove the QA reference to ClawHub documentation intentionally deleted on main. Proof: 126 focused owner tests passed; the four browser-route cases passed with and without inspected synthetic captures. Scoped Codex autoreview reported no actionable P0 findings. The separate bundled fake-timers refresh defect remains unresolved; this does not claim green CI or accepted migration performance.
Recognize stateful .test-mocks imports, await browser position readiness, migrate remaining report cache fixtures, and retain native file results in cleanup diagnostics. Co-authored-by: Vincent Koc <vincentkoc@ieee.org>
steipete
force-pushed
the
steipete/vitest5-migration-d2657a
branch
from
September 5, 2026 04:24
c7b84c3 to
c15e68d
Compare
Contributor
Author
|
@clawsweeper re-review The stable migration is rebased onto current main, and the maintainer explicitly approved the bundled fake-timers repair. The two native timer cases fail before the repair and pass after; the original Telegram timeout cases and all 17 native shutdown cases pass. All seven installed patch target hashes were verified against the pnpm-generated patch. Full native harness recheck: 419 passed. Fresh independent full-diff review through P2 found no actionable defects. The aggregation issue #138174 was fixed by #138250 before this base; its grouped, empty-project, and identity regressions pass on v5. The patch whitespace finding concerns unchanged tab/blank context inside the unified dependency patch, which pnpm regenerated intact; source whitespace checks pass. The current PR body distinguishes completed correctness proof from pending exact-head CI and the controlled paired performance run. |
Contributor
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Use native standalone file projects with execution roots when replaying accepted reports. Preserve fresh config factories, hook ordering, names and pools while removing duplicate resolution and injected names. Keep report identity and publication guards intact. Cover native descriptors, existing project forms, hook-sensitive replay and changed-config rejection.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Member
|
Superseded by #138264, landed as 8e5a3f3. It carries the stable Vitest 5 migration with the final dependency-contract repairs, exact-head package proof, hosted CI, and matched performance acceptance. Peter's fake-timer, ordered-suite, browser, and TUI work is preserved with co-author credit in the landed squash commit. We used the successor because this branch was not maintainer-editable and the final landed diff needed additional pnpm-lock and file-backed project-root repairs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related: #137201. Integrates the shared migration work and selected fixture follow-ups from #138264, with Vincent Koc credited in the commits. Three additional transport fixtures in that PR remain separate.
Problem and change
Move the development and CI toolchain from Vitest 4.1.11 to stable 5.0.0, including root/UI packages, project discovery, mock lifecycle, native reports, QA selectors and worker shutdown. This restores useful native timeout diagnostics while preserving strict CLI validation, trailing task updates and joined worker/profiler cleanup. The obsolete standalone runner patch and abandoned mock-queue recovery are removed.
The approved exact-version patch also fixes Vitest's bundled fake-timers heap:
refresh()must remove a timer using its old deadline before changing and reinserting it. The old ordering can fire synchronous timers out of order and skip due asynchronous timers, hanging Telegram's trickling-body test. Both native timer regressions failed before the repair and pass afterward; production deadlines are unchanged.The migration preserves current main's
clearMocks: false, independent project aliases and stronger service/session fixtures. Other repairs recognize stateful.test-mocksimports, await native popup positioning before hit-testing, own pointer/focus before asserting idle board chrome, and move the native Swift WebSocket fixture's readiness/network state onto one serial queue. Product runtime changes are limited to structured QA report-title reading and a large-image data-URL decoder repair that avoids regexp stack exhaustion while retaining grammar, size limits and error precedence.Current state
Head:
baf3fd7e13144f3009ff42748738264c30163bc3. Comparison base:8abac3db2d4f50f4465d30e4a5c60779d05f1b32.Ready to land: code review, native regressions, required local checks, frozen installation, controlled performance acceptance and all ten final TUI scenarios pass. CI for this head completed successfully: 140 jobs passed, five were intentionally skipped, and no reruns were needed.
Report replay previously resolved each config to obtain its name, then loaded it again as an inline project. Name hooks could run twice or observe an injected name, changing both identity and name-dependent pool selection. The approved native standalone file-project descriptor resolves each captured file once with its execution root, preserving native factories and hooks while avoiding child rediscovery. The consumer deletes preliminary resolution and name injection. Native project attachment also records the container name prefix; replay applies that context after the file hooks. Two containers can share a leaf configuration while retaining distinct qualified names and unchanged early-hook inputs. Fresh name, root, config-path and pool identity checks remain intact; changed config still blocks publication and retains the original reports.
Validation
Current main’s cache-generation fix is preserved in Vitest 5: reporter imports, awaited config-hook imports and idle cache recreation each failed before the port and pass after it. All 13 cache tests pass. The separate-root regression explicitly verifies that excluded B’s old cache survives A’s update, then B imports the new generation; 3 namespace siblings and a native two-project coverage-enable rerun pass.
Final corrections passed 59 distinct selected native cache/project/report cases across the run and targeted correction rerun. The initial new assertion incorrectly assumed module execution order; it now compares complete native identities with multiplicity. Qualified-name replay and changed-leaf rejection are exercised without changing existing runtime assertions or deadlines.
The full all-lane local gate passed after main integration. The final prefix/test corrections passed the affected script/root-test checks, and a frozen pnpm12.3.4 install passed. Final independent review through P2 is clean across all seven review sections.
The approved native file-project repair passes all 82 report-owner cases and 28 descriptor/legacy/error/type contract cases. Focused red/green proof covers normal, early and post hooks, name-dependent pools and changed-root rejection. Actual installed public declarations pass positive and negative type checks. Complete-context independent review through P2 is clean.
The latest broad native run recorded 444 passes and 3 failures: a 5-second synchronous SQLite callback, a 120-second watch case, and 20-second shutdown readiness. All affected checks then passed without source/assertion/timeout changes: four cleanup cases in original order, actual watch first run plus dependency-triggered rerun, and the full 15-case shutdown file. The original failed run is retained. This is separate from the earlier 419-case passing selection below.
The approved timer repair has 2 native red/green regressions; the original 3 Telegram timeout cases, 3 task-update cases and all 17 native shutdown cases pass.
The complete native harness selection passed 419 cases. An earlier run passed 417 and failed 2 cleanup fixtures under competing lint; the original failures remain retained. Both pass alone, all 4 main-route success/failure cases pass in order, and the full selection passes without competing lint. Assertions were retained; diagnostics were strengthened.
Full build and all typecheck programs passed before the mechanical main rebase. Exact c15 changed-file checks and focused rebased doctor/timer/Telegram/config/routing checks passed. The latest board repair also passes the complete changed-file gate and 31 browser cases together.
The Swift readiness failure was reproduced with a real listener under MainActor contention. The repair keeps the five-second budget and uses callback-owned readiness. 1,624 OpenClawKit tests and all 3 focused iOS fleet tests (5 parameter cases) pass. Full-context independent review through P2 is clean.
The board warmup failure expected an idle transparent border while the real pointer hovered the card. Native hover reproduced the same active border; moving pointer and focus outside makes the unchanged assertion pass. Full-context independent review through P2 is clean.
The exact 28-file Node shard that previously stalled twice passes in its original order locally and on Linux Node 24.19. Plain, cold CI-cache and warmed read-only cache runs each passed 635 Linux cases with 3 platform skips; processes joined and source/build inventories were unchanged. No speculative timeout or retry increase was introduced.
All seven installed Vitest patch targets match their patch postimage Git hashes. Patch SHA-256:
817561d2e2942cb023c8c3b76268d56a0b72f90a8cc92b897028e0771e8d7110.Performance acceptance
An exclusive Linux Testbox compared baseline
8abac3db2d4f50f4465d30e4a5c60779d05f1b32with this exact candidate using Node 24.19.0, native pnpm 12.3.4, four CPUs and four workers. Source, dependency, cache and state directories were separate. Necessary browser fixture repairs were mirrored into the V4 reference; the remaining browser import spelling and native mock-queue contract differences were declared and inspected.Both preflights executed 1,906 passing cases plus 14 skipped declarations across the same 120 files. All 336 title-format mappings preserve file, status, native declaration ordinal, suite depth and multiplicity. The 37 mapped source files are byte-identical; 244 quote-only changes and 92 truncation changes were verified against the exact native formatters and source literals.
Median total across all seven workloads: 101.749s → 100.045s. The paired geometric mean ratio is 0.982851 (about 1.71% lower time), with a bootstrap 95% interval of 0.980490–0.985017. Performance acceptance is PASS: no aggregate slowdown beyond 5%, and no critical lane regression beyond 10% and at least one second.
One warmup and all five alternating measured pairs completed without harness retries, failed cases, unjoined processes or source/build/patch/toolchain drift. The independent audit accounts for 84 logical workload receipts, 96 outer invocations and 108 native shard reports: 23,040 declarations, comprising 22,872 passes and 168 skips.
All five measured samples, in seconds
Testbox workflow. The original sealed benchmark archive and native reports are retained; SHA-256:
43fb7d32ee4ca5345673630e31870a10a6b9665098fce06911ffc956c71013c4. The lease is stopped after evidence download, so its eventual workflow cleanup state is distinct from the successful command receipts and independent benchmark verdict.The earlier c15 attempt passed inventory preflight but failed its first browser warmup before any measured pair. That failure and its original reports remain retained. It contributes no sample to this final comparison.
Final TUI acceptance
The exact ten canonical scenarios passed on the same candidate with the host runner, mock OpenAI provider and concurrency one. The independent audit verifies 10 passed scenarios, 36 coverage rows, 34 passed native executions and 32 distinct native identities. The suite completed in 264.972 seconds after the private QA build passed. Source and private build bytes stayed unchanged, and the accepted benchmark archive remained hash-identical.
Final QA workflow. Original QA archive SHA-256:
82c8cab639433fcd3b08a2eb3a71732d890e8a4415ee0990652261ec2b262820. The transfer to this fresh lease verified the benchmark seal, archived manifest, PASS summary and independent audit against the exact baseline and candidate refs. Both leases are completed, all run and transport processes have joined, and temporary carriers are removed; original archives and reports are retained.The scenarios cover command surfaces, editor input, entrypoints, Gateway boundaries, local runtime recovery, local shell, the evidence producer contract, session management, streaming tool cards and terminal safety.
Review and compatibility
ClawSweeper's latest review of
baf3fd7e131(September 5, 17:10 UTC) reports no actionable code findings. Its requested native UI ownership check is complete: all 33 tests intest/vitest-ui-e2e-config.test.tspass against installed Vitest 5.0.0. Direct upstream source, installed declarations and installed implementation confirm thatcreateVitest("test", options)is still a supported deprecated overload (upstream implementation). The seven-workload comparison also passes with complete matched inventories and paired samples. All ten final TUI scenarios also pass, completing the remaining rank-up and landing requirement.The native loader/cache/worker contracts were inspected directly in complete upstream source and installed package bytes, and the final independent review through P2 is clean. Maintainer approval for the exact-version patch is explicit.
Compatibility review: these are contributor/CI toolchain and generated test-artifact changes. No OpenClaw operator config, SQLite schema, persisted user state or Gateway protocol changes occur. Older native Vitest blobs cannot cross versions; retain them as evidence and regenerate the test runs to produce current blobs. Adding runtime compatibility readers would violate the native format contract. The aggregation reference #138174 was already repaired by merged #138250, an ancestor of this base; terminal project-owner capture and grouped/empty/conflicting-identity tests are retained.
Vitest 5.0.0 supports
^22.12.0 || ^24.0.0 || >=26.0.0; OpenClaw's documented test hosts are Node 22.22.3+, 24.15+, or 26+. Node 25 runtime support is unchanged, but it cannot host this test toolchain. Native blobs remain bound to the producing Vitest version and must be regenerated. No operator configuration, database schema or Gateway protocol migration is introduced.Named follow-ups: CI watcher recency when execution-start order differs from run creation order; pre-termination stall evidence that preserves synchronous kill deadlines; the remaining transport fixtures in #138264; local Testbox source-capsule creation/cleanup under system-volume disk pressure. That transport overhead occurred outside measured test durations; staging moved to a separate storage volume.