test: make the local pnpm test gate green on macOS hosts#100069
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c0d640d01
ℹ️ 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".
| delete process.env.OPENCLAW_GATEWAY_TOKEN; | ||
| delete process.env.OPENCLAW_GATEWAY_PASSWORD; |
There was a problem hiding this comment.
When this file runs in shared non-isolated Vitest workers with OPENCLAW_GATEWAY_TOKEN or OPENCLAW_GATEWAY_PASSWORD set by the invoking environment, these deletions are never restored because serviceEnvSnapshot only captures the service marker/PID keys. That permanently changes the worker environment for every later test file, so tests that intentionally read the initial gateway auth env will see a different value. Please include these keys in the suite snapshot or clear them through a scoped env helper.
Useful? React with 👍 / 👎.
|
|
||
| expect(result.ok).toBe(true); | ||
| expect(result.command).toMatch(/^& .+python3(?:\.\d+)? a\.py$/); | ||
| expect(result.command).toBe(`& ${python3} a.py`); |
There was a problem hiding this comment.
On Windows hosts whose temp directory contains spaces, such as a profile under C:\Users\Jane Doe, buildEnforcedShellCommand correctly quotes the resolved executable via windowsEscapeArg, producing & "...\python3.exe" a.py. This exact unescaped template then fails even though the renderer is behaving correctly, leaving the Windows/macOS-local test gate red in that environment. Compare against the escaped executable path instead of interpolating python3 raw.
Useful? React with 👍 / 👎.
|
Codex review: needs changes before merge. Reviewed July 4, 2026, 4:59 PM ET / 20:59 UTC. Summary PR surface: Source +19, Tests +113. Total +132 across 17 files. Reproducibility: yes. source-reproducible: the linked issue gives the macOS failing command, and the remaining PR defects follow from the current escaping and env-restore contracts without running write-producing tests in this read-only review. Review metrics: 1 noteworthy metric.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Keep the fixture-level macOS test-gate approach, but restore or scope every newly cleared inherited env key and compare the Windows assertion against the escaped fixture path before merge. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: the linked issue gives the macOS failing command, and the remaining PR defects follow from the current escaping and env-restore contracts without running write-producing tests in this read-only review. Is this the best way to solve the issue? No, not yet: the fixture-layer direction is the right boundary, but this PR still needs three narrow test-isolation repairs before it is the best mergeable fix. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ed098c655069. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +19, Tests +113. Total +132 across 17 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (2 earlier review cycles)
|
Canonicalize shared fixture roots (fs.realpath after mkdtemp) so tests that intercept or compare fs paths by equality survive the macOS /var -> /private/var tmpdir symlink; resolve exec-approvals python3 from a fixture PATH instead of the host; pin UTC for the logs-cli rendered-timestamp assertion; canonicalize qqbot scoped-media fixtures. Fixes the deterministic macOS failures in #100025 plus the store.pruning.integration and qqbot classes.
Forking no-output scripts emit a readiness byte after the pid write so the rolling no-output window starts once the pid exists; boundary-artifacts waitForFile now requires non-empty content (empty read parsed to a NaN pid) and sibling failures are sentinel-gated instead of timer-raced; crabbox DELETE_CWD_ONCE restore deadline becomes a real fail-safe; codex turn-watch delay asserted within a bounded window and the promptness test no longer races startup against a 200ms attempt cap.
server-reload-handlers clears leaked OPENCLAW_SKIP_CHANNELS/PROVIDERS before each test (module-load side effect of sibling gateway helpers); gateway run --token exports credentials into process.env, so option-collisions snapshots and clears them and run-main.exit clears them per test; the imessage watch-subscribe-retry test clears the leaked runtime singleton so a sibling file's recovery cursor cannot add since_rowid.
waitFor existsSync + immediate parseInt raced writeFileSync and produced NaN pids under parallel-suite load; both suites now share a waitForPidFile helper that polls for non-empty content, with headroom for tsx-booting probes.
8c0d640 to
a5fbfd8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5fbfd85af
ℹ️ 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".
| delete process.env.OPENCLAW_SKIP_CHANNELS; | ||
| delete process.env.OPENCLAW_SKIP_PROVIDERS; |
There was a problem hiding this comment.
When the gateway-server shard runs with OPENCLAW_SKIP_CHANNELS or OPENCLAW_SKIP_PROVIDERS set by the invoking environment, this new beforeEach deletes those values and never restores them. That shard runs with isolate: false in test/vitest/vitest.gateway-server.config.ts, so later files in the same worker see a different env; it also defeats the per-test snapshots below because they now snapshot undefined. Please snapshot these two keys before clearing and restore them after the suite.
Useful? React with 👍 / 👎.
…0069) Fixes 21 macOS-only failing test cases across three classes: canonicalized fixture roots (macOS /var -> /private/var tmpdir symlink vs production realpathing), load-tolerant process-spawn tests (content-gated pid files, readiness-sequenced kill windows, bounded-window timer assertions), and cross-file worker leak guards (skip-channel env, gateway token env, imessage runtime singleton). Test-only; no production changes. Fixes openclaw#100025.
What Problem This Solves
pnpm testcould not go green on a clean macOS checkout ofmain(Linux CI green on the same SHA), which breaks thescripts/pr prepare-runlocal gate for every PR landed from a maintainer Mac (#100025). Beyond the two deterministic infra failures in the issue, full-suite runs on macOS failed 13 of 89 shards with a different subset each run.Fixes #100025. Supersedes #100026 with fixture-layer fixes (details below).
Why This Change Was Made
Triage of full-suite runs on a Darwin 25.5 / Node 24 host reduced every failure to one of three classes, all test-environment bugs (no production behavior was wrong):
Host dependence (deterministic on macOS)
os.tmpdir()sits behind the/var -> /private/varsymlink while production realpaths state/session/media paths. Any test that intercepts or compares fs paths by equality breaks. Fixed at the source: the shared fixture helpers (src/test-helpers/temp-dir.ts,src/test-utils/openclaw-test-state.ts,src/test-utils/fixture-suite.ts) now canonicalize their roots withfs.realpathaftermkdtemp(identity on Linux). This fixesbackup-createlate-SQLite guard tests and all threestore.pruning.integrationrepair tests without per-test patches. (The same class in the qqbot scoped-media tests was fixed independently onmainin 7a92c47 while this branch was in flight; this PR rebases onto it.)exec-approvals-analysisresolved the hostpython3(Homebrew'spython@3.xpath triggers quoting). The test now resolves from a fixturePATHvia the existingmakeTempDir/makeExecutable/makePathEnvhelpers and asserts exact equality — also fixing Windows hosts, where the old regex broke onpython3.exe.logs-cliasserted a rendered timestamp that follows the host time zone (host here runsEurope/London, hence+01:00). The test now pinswithTimeZone("UTC", ...)like its siblings.Process-spawn races under parallel-suite load
src/test-utils/process-tree.ts) now emit one readiness byte on stderr after writing the pid file; both consumers (install-policy,secrets/resolve) use a rolling no-output timer that re-arms on output, so the killing silence window now starts strictly after the pid exists. Budgets raised from 150ms (lost to shell spawn latency under load) to 1s/10s.prepare-extension-package-boundary-artifacts,dev-tooling-safety, andsecret-provider-integrations: pid-file waits now require non-empty content (a reader betweenopen(O_CREAT)andwritesaw an empty pid file →parseInt("") = NaN), sibling "delayed-fail" steps are sentinel-gated on the descendant's pid/ready file instead of a 150ms timer, and the group-kill test's step timeout leaves room for two Node boots.crabbox-wrapperDELETE_CWD_ONCE fake: the restore-wait deadline is a fail-safe (the wrapper normally kills the child mid-loop); raised 1s → 10s so wrapper timer starvation can't make the child exit 66 before the wrapper reacts.codex run-attempt.turn-watches: the terminal-idle delay is real-clock derived (timeout - elapsed), so exact equality loses a same-millisecond race under load (observed1799999vs1800000); asserted within a bounded window instead. The "times out promptly" case had a 200ms overall attempt cap that startup could eat under load — promptness is proven by the 5ms completion-idle promptError, so the cap is now generous.Cross-file leaks in shared vitest workers (
isolate: false)server-reload-handlers: gateway test helpers setOPENCLAW_SKIP_CHANNELS/OPENCLAW_SKIP_PROVIDERSat module load; a leaked value routes reloads into the skip branch. Added the same documentedbeforeEachguardserver-aux-handlers.test.tsalready uses.gateway run --tokenexports credentials intoprocess.envas a production side effect;run.option-collisionsnow snapshots/clears them (polluter side) andrun-main.exitclears them per test (victim side) — a leakedtok_runchanged gateway preflight auth.imessage monitor.watch-subscribe-retry: sibling suites install the imessage runtime singleton without clearing it; a leaked runtime resurrects another file's recovery cursor andwatch.subscribegains an unexpectedsince_rowid. The test now clears the singleton inbeforeEach.Supersedes #100026: that PR loosens the Windows-render regex (still host-dependent), adds a second compare path to the backup spy, patches two pruning expectations, and adds a
paths.tsrealpath-parent fallback for missing files. With fixture roots canonicalized, all of those tests pass without per-test patches and without the production change, so the fixture-layer fix is preferred (mainstays lean; no prod diff in this PR).Known remaining (out of scope):
src/gateway/server-startup-log.test.ts("blocked channel plugin" warnings) can still flip under worker sharing — the mock onplugin-registryis bypassed when the dynamically imported blocker chain was cached by another file, and the real registry then sees the repo's bundled slack manifest. Deterministic repro: addvi.resetModules()to the file. This is the plugin-manifest/module-leak class tracked separately (nested-worktree plugin-manifest leak follow-up).User Impact
Maintainers can run
pnpm test/scripts/pr prepare-runon macOS and get a green gate. No production code changes; test-only (+ test helpers).Evidence
Host: Darwin 25.5, Node 24, system TZ Europe/London.
node scripts/run-vitest.mjs run --config test/vitest/vitest.infra.config.ts src/infra/exec-approvals-analysis.test.ts src/infra/backup-create.test.ts→ 3 failed. After: 59/59 pass.pnpm test-equivalent (node scripts/test-projects.mjs) on this Mac, five runs: baseline failed 13/89 shards; the failing-test inventory across runs surfaced 21 distinct cases (each run samples a different subset); intermediate runs converged to 1 failed shard as fixes landed (the qqbot pair was proven pre-existing by reverse-applying the helper edits against pristine helpers, and the tooling pid-file pair surfaced only in run 4); final run on the committed tree: 89/89 shards passed, exit 0.node scripts/run-tsgo.mjs -b tsconfig.projects.jsonexit 0.gpt-5.5): clean, no accepted/actionable findings.