test(mcp-probe): order the settle-fence test on the probe's connect instead of a 20 ms sleep - #449
Conversation
…nstead of a 20 ms sleep
|
@codex review |
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Two changes:
packages/agent-bundle/tests/mcp-probe-service.test.ts) — see below.@codex reviewre-request / head-SHA-confirmation / quota-fallback rules with the maintainer's new process: a local reviewer subagent self-review before merge, findings fixed or dismissed in a "Self-review" PR section, organic review comments still addressed, external reviews never solicited.Test fix
pnpm check:local-cionorigin/main@10a98a0fb(final sweep, load average 50–75) failed exactly one test, on the Node 22.19 verify leg:Root cause: the test slept 20 ms "to let the probe reach its (blocked) connect" and then read the plugin-data file that
createPluginDatawrites beforeconnect. Under load the probe had not reachedcreatePluginDatainside 20 ms, sopluginDatawas still the previous value and the read hitENOENT. A second 50 ms sleep guarded the "settle must not resolve early" assertion the same way.Fix at source (test only, no product change): the stubbed
connectresolves aconnectStarteddeferred the moment it is entered, and the test awaits that instead of the sleep — plugin data provably exists at that point and the teardown is provably not registered yet. The early-resolution check drains one macrotask (setImmediate) instead of sleeping 50 ms: a fence that only covered registered teardowns resolves within the current macrotask, so the regression is still observed, with no wall-clock budget left in the test.Evidence
.worktrees/local-ci/logs/verify-node22/07-test-unit.logfrom the sweep run (3015/3021, one failed test; Node 24 and 26 verify legs and the gates leg were green on the same commit)./tmp/final-sweep/probe-loop-*.log).mainunder the same load is recorded in the sweep report (10-run loop).Test plan
pnpm exec rstest --config rstest.unit.config.ts packages/agent-bundle/tests/mcp-probe-service.test.ts×20 under loadSelf-review
change-risk-reviewersubagent, modelgpt-5.6-sol-high, on headf3dfd3d4bvsorigin/main, asked for concrete merge risks only.mcp-probe-service.ts:428,590—createPluginDataand theproof.txtwrite complete beforeconnect()is called on the same probe, so the awaited read cannotENOENT. Disposition: confirms the fix; no change.mcp-probe-service.test.ts:745-750— a teardown-onlysettle()resolves through microtasks beforesetImmediatefires, so the regression is still caught. Disposition: confirms the guard; no change.mcp-probe-service.ts:558-590— one probe callsconnect()exactly once; never reaching it fails/times out rather than false-greening. Disposition: no change.AGENTS.md:66-80— repo-wide search found no remaining instruction to post@codex reviewor record unreviewed SHAs. Disposition: no change..changeset/README.md:24-27,.changeset/config.json:6—tests/**and root guidance are exempt; no changeset required (the "Changeset present" check passed without a label). Disposition: no change.