improve: keep isolated tests under one second#100019
Conversation
|
Codex review: needs changes before merge. Reviewed July 5, 2026, 1:54 PM ET / 17:54 UTC. Summary Reproducibility: yes. for the review blockers: source inspection shows the active setTimeout spies surround vi.waitFor while matching the same 1,000 ms delay they intend to capture. I did not rerun the full isolated inventory in this read-only review. Review metrics: none identified. 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 planning/reporting improvements, but first phase-gate or otherwise narrow the three timeout captures so they can only fire production timers, then rerun focused timer tests and exact-head automation proof. Do we have a high-confidence way to reproduce the issue? Yes for the review blockers: source inspection shows the active setTimeout spies surround vi.waitFor while matching the same 1,000 ms delay they intend to capture. I did not rerun the full isolated inventory in this read-only review. Is this the best way to solve the issue? No as submitted: the overall performance direction is maintainable, but the timeout tests must capture only production timers before this is the best fix. A phase-gated capture or non-1,000 ms polling helper is the narrower repair. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8a187447f94a. Label changesLabel justifications:
Evidence reviewedAcceptance 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 (27 earlier review cycles; latest 8 shown)
|
1f28153 to
7c1e520
Compare
Dependency graph guard clearedThis PR no longer has blocked dependency graph changes. A future dependency graph change requires a fresh
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b4c31f848
ℹ️ 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".
| export * from "../providers/openai-completions.js"; | ||
| export * from "../providers/openai-prompt-cache.js"; | ||
| export * from "../providers/openai-reasoning-effort.js"; | ||
| export * from "../providers/openai-responses.js"; |
There was a problem hiding this comment.
Split light helpers out of the OpenAI barrel
Core callers that only need small helpers now import this barrel, e.g. src/agents/simple-completion-runtime.ts imports supportsOpenAIReasoningEffort and tool-schema code imports projection helpers. Because ESM export * evaluates each re-exported module, those callers also load openai-completions.ts and openai-responses.ts, both of which import the OpenAI SDK, before any provider request. This defeats the lazy provider loading this change is trying to preserve and adds startup/isolated-test cost for non-OpenAI paths; expose the light helpers through narrower internal subpaths or direct files instead of this provider barrel.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,7 @@ | |||
| export * from "../providers/anthropic.js"; | |||
There was a problem hiding this comment.
Keep Anthropic model helpers off the provider barrel
src/agents/transport-message-transform.ts now imports resolveModelBoundThinkingReplayMode from this subpath on the common message-transform path, but this barrel first re-exports anthropic.ts, whose top-level import pulls in @anthropic-ai/sdk. That makes every agent run load the Anthropic provider implementation even when the selected model is not Anthropic, regressing the lazy transport boundary and isolated-test/startup cost; move model-contract/replay helpers to a light internal subpath or import their source file directly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2aaa77d97e
ℹ️ 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".
| @@ -27,15 +27,20 @@ | |||
| "openclaw/extension-api": ["./src/extensionAPI.ts"], | |||
| "openclaw/plugin-sdk": ["./src/plugin-sdk/index.ts"], | |||
| "openclaw/plugin-sdk/*": ["./src/plugin-sdk/*.ts"], | |||
| "@openclaw/ai": ["./packages/ai/src/index.ts"], | |||
There was a problem hiding this comment.
Watch the new AI package sources
Adding this @openclaw/ai alias makes root runtime code resolve AI transports/providers from packages/ai/src, and scripts/tsdown-build.mjs now builds it through the separate tsdown.ai.config.ts; however, scripts/run-node-watch-paths.mjs still watches only the older package source roots and root tsdown config. In pnpm gateway:watch, editing an AI package file or its tsdown config will not rebuild/restart the gateway, leaving stale @openclaw/ai output until a manual rebuild or unrelated watched edit; add the AI source root and AI tsdown config to the run-node watch inputs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c80e4bdd9
ℹ️ 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".
| if (delay === 1_000 && !triggerCompletionTimeout) { | ||
| triggerCompletionTimeout = () => callback(...args); | ||
| return nativeSetTimeout(() => undefined, 60_000); |
There was a problem hiding this comment.
Capture only the compaction timer
In this test the global setTimeout spy is installed before the vi.waitFor on line 890, and vi.waitFor has a 1000 ms default timeout. When the compaction request has not armed its own 1000 ms completion timer yet, this branch captures the test runner's wait timeout as triggerCompletionTimeout; calling it later does not request turn/interrupt, so the test can fail or hang instead of deterministically exercising the configured compaction timeout. Scope the capture to the timer created after beginRequest/the fake client request, or avoid vi.waitFor while this spy is active.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cf312b49b
ℹ️ 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".
| await vi.waitFor(() => { | ||
| expect(noOutputTimeouts.length).toBeGreaterThanOrEqual(2); | ||
| }); |
There was a problem hiding this comment.
Do not let waitFor satisfy the timeout capture
In this POSIX no-output timeout test, the global setTimeout spy is still active while vi.waitFor runs; Vitest documents vi.waitFor's default timeout as 1000 ms (https://vitest.dev/api/vi#vi-waitfor), so the waiter's own watchdog can be pushed into noOutputTimeouts alongside the resolver's initial timer before the script emits ready. Under a slow spawn, the assertion can pass too early, readPidFile can race the pid write, or noOutputTimeouts.at(-1) can fire the waiter's timeout instead of the re-armed no-output timer, leaving the child alive and making this test flaky/hanging.
Useful? React with 👍 / 👎.
| await vi.waitFor(() => { | ||
| expect(noOutputTimeouts.length).toBeGreaterThanOrEqual(2); | ||
| }); |
There was a problem hiding this comment.
Do not count waitFor's timer as policy readiness
This test has the same race with the active setTimeout spy: vi.waitFor schedules a 1000 ms timeout by default (https://vitest.dev/api/vi#vi-waitfor), which matches the captured policy noOutputTimeoutMs. That means noOutputTimeouts.length >= 2 can be satisfied by the policy's first timer plus vi.waitFor's own watchdog before the readiness byte re-arms the policy timer, so the subsequent readPidFile/at(-1) steps can race or fire the wrong callback instead of killing the forked policy child.
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
|
Exact-head landing proof is complete for
Known gap: the sole empty |
|
Merged via squash.
|
Closes #100018
What Problem This Solves
Resolves a problem where the isolated full-suite test inventory contained individual tests above one second and could OOM before reporting every leaf plan. The slow cases mixed real waits, repeated one-time initialization, broad plugin discovery, and oversized fixtures, making test feedback slower and performance regressions harder to audit.
Why This Change Was Made
The duration-report path now runs full-suite leaves with isolated files, bounded workers, timeout cleanup, RSS evidence, and partial-report preservation. Measured outliers move expensive initialization outside behavior timing, defer heavy imports, use lightweight public plugin artifacts and core-registry fast paths, preserve size-limit coverage with smaller fixtures, and exercise deadline behavior with deterministic timers.
User Impact
No end-user product behavior changes. Contributors and CI runners get a reliable isolated inventory, lower memory pressure, and individual test cases that remain below the one-second performance budget.
Evidence
b041d4636593036a086e44513c85d702f24bebb8.05c809399a65f4c26e83342023bf7a61adaa14f1: all 250 leaf plans accounted for; 249 non-empty reports, 87,996 tests across 6,565 files, zero individual tests above 1,000 ms, peak RSS 2.81 GB, and no OOM. The sole emptytooling-11report is inherited from current main's live-test/no-tests shard.tbx_01kwsf64w84wx9na66vaabf2k1(Actions run).tbx_01kwsf2necer626hhd9as1sjce(Actions run).tbx_01kwsa849x50y2xgmxw4zkyhd7(Actions run).28749363609and Workflow Sanity run28749363595passed.