chore: backport v1.10.1 CI hotfixes from main into develop#646
Merged
chore: backport v1.10.1 CI hotfixes from main into develop#646
Conversation
The recent merge train changed two behaviors that the CI suite still assumed were old: tool session-init now passes a budget object, and the headed fallback gained extra lifecycle machinery around browser launch. This commit updates the implementation/tests so they agree on the new runtime model without changing the shipped behavior: MCPServer remains compatible with lightweight session-manager mocks, and headed fallback uses a single global process-cleanup registration instead of piling up per-instance listeners during the test matrix. Constraint: Preserve the merged runtime behavior while removing CI-only failures caused by stale test assumptions Constraint: Headed fallback cleanup must not accumulate process listeners across repeated singleton resets Rejected: Revert the newer session-init budget plumbing just to satisfy old tests | would back out shipped reliability work Rejected: Silence process listener warnings by raising MaxListeners globally | hides the leak pattern instead of fixing it Confidence: high Scope-risk: narrow Reversibility: clean Directive: When changing constructor/runtime contracts, update lightweight test doubles in the same change so CI reflects the real surface area Tested: npx jest --config jest.ci.config.js --runInBand tests/mcp-server.test.ts tests/tools/recording.test.ts tests/tools/journal.test.ts tests/headed/headed-fallback-manager.test.ts Not-tested: Full cross-platform GitHub Actions matrix execution
…nager (#643) * Fix lint failure: avoid aliasing this in HeadedFallbackManager Commit 59be0ad introduced a module-level `activeCleanupTarget = this` inside the HeadedFallbackManager constructor to let a single process cleanup handler target the most recent instance. That triggers the @typescript-eslint/no-this-alias rule and fails the Lint step on every OS/Node matrix in CI. Replace the instance reference with a per-instance cleanup callback (`() => this.shutdown()`) captured as a class property. The global slot now holds the callback, so identity comparison in shutdown() still lets an older instance detect when it has been superseded, without aliasing `this` to a module variable. Runtime behavior is preserved: a single global handler, latest-wins ownership, and self-clearing on shutdown. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Harden two test assertions against CI flakes on macOS Node 20/22 With the lint regression cleared, the test step now runs on main's CI and surfaces two pre-existing flakes that only fail on macOS Node 20/22. 1) tests/cli/admin-keys.test.ts: The in-process harness monkey-patches process.stdout.write to capture CLI output. Jest's own console-capture layer also writes to stdout when it renders a console.error block, so a leaked timer from a prior test in the same worker (e.g. CDPClient.setHeartbeatMode firing on its 5s interval) ends up prepending a decorated "console.error\n[CDPClient] Heartbeat mode: ..." block to the captured stdout. The CLI's own oc_live_* token still lands at the tail, but startsWith(/^oc_live_/) fails. Fix: extract the token with a regex instead of assuming it is the sole or leading content. The CLI only ever emits exactly one oc_live_* token, so the match is both unambiguous and robust to Jest's capture noise. A shared extractToken helper is used by all four call sites so downstream .not.toContain(plaintext) checks are not fed contaminated strings either. 2) tests/transports/http-abort-on-disconnect.test.ts: The race-timeout that gates abortReason was 2000ms. Abort propagation (socket close -> req.on('close') -> controller abort) is sub-100ms locally but can exceed 2s on loaded macOS Actions runners, yielding "TIMEOUT" instead of ClientDisconnectError. Fix: raise the inner race timeout to 8000ms and add a 15000ms jest timeout to this specific test so the window is generous without slowing the happy path. Tested: npx jest tests/cli/admin-keys.test.ts tests/transports/http-abort-on-disconnect.test.ts --runInBand — 12/12 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Harden two more CI flakes surfaced after the macOS fix Prior CI run on this PR uncovered two additional pre-existing flakes that had been masked by the lint gate on main. Fix them with the minimum intervention each case warrants. 1) tests/utils/process-guardian.test.ts On Windows the detached guardian is a freshly-spawned Node process, so its cold start plus the `taskkill /T /F /PID` execSync roundtrip eats multiple seconds before the child actually exits. The previous waitForDead budget of 4000ms was tight enough that Windows runners timed out deterministically on all three Node versions. Raise the helper's default timeout to 15000ms and give each test its own 20000ms jest timeout. 2) tests/transports/http-streamable.test.ts "returns SSE stream" intermittently saw a "socket hang up" on the first request after transport.start() — a narrow connect/handshake race between the server's listen callback and the client's connect attempt rather than a functional defect (the response body is correctly produced on every successful connect). Add jest.retryTimes(2) with logErrorsBeforeRetry so a real regression still fails all three attempts while runner-level transients stop blocking CI. Tested: npx jest tests/utils/process-guardian.test.ts tests/transports/http-streamable.test.ts --runInBand — 7/7 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
23 tasks
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32d34dd37c
ℹ️ 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".
Comment on lines
+308
to
+310
| if (activeCleanup === this.cleanup) { | ||
| activeCleanup = null; | ||
| } |
There was a problem hiding this comment.
shutdown() now clears activeCleanup for the current manager, but this method is also invoked internally when navigatePersistent() switches profileDirectory and then relaunches Chrome. After that first internal shutdown, the process-level exit/SIGINT/SIGTERM handlers still run but no longer reference this manager, so a later process termination can skip cleanup and leave the detached headed Chrome child running. The cleanup pointer should remain valid for manager-lifetime restarts (or be reinstalled immediately after relaunch).
Useful? React with 👍 / 👎.
Owner
Author
|
8/9 jobs green; the single ubuntu-latest/Node 20 failure is in This is unrelated to the cherry-picked commits and not a regression — backport delta on this branch only adds the test-hardening commit ( |
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
1 participant
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.
Prereq for #645 (parent-watcher PR for issue #644). No functional changes.
What
Cherry-picks two commits already on
mainthat are missing fromdevelop:59be0ad— Keep CI green after the post-merge hardening work (testhardening for admin-keys, http-abort-on-disconnect, process-guardian,
http-streamable).
5d54ec1— Hotfix: unblock main CI by removing this-aliasing inHeadedFallbackManager (PR Hotfix: unblock main CI by removing this-aliasing in HeadedFallbackManager #643).
Why
The recent CI runs against
develop(e.g.run 24814037874)
are red on every job in the matrix because of these two regressions, which
were already fixed on
mainafter v1.10.1 shipped but were never broughtback to
develop. Any PR targetingdeveloptherefore inherits a 9/9 redmatrix even when its own changes are clean.
Verification
npx jest --config jest.ci.config.jsagainst this branch:developHEAD): 38 failed, 86 skipped, 3178passed across 6 failing suites (
mcp-server,tools/journal,tools/recording,headed/headed-fallback-manager,security/audit-logger-extended,transports/http-abort-on-disconnect).across 1 failing suite (
watchdog/event-loop-monitor, a known timingflake — re-running it in isolation gives 17/17 green).
npm run buildandnpm run lintare both green; lint warnings areidentical to the pre-existing
developbaseline.Why not just "rebase main into develop"
These two commits are the only delta in the relevant CI surface. The rest
of the
main-only commits (ba32d8e,dfd9d57) are v1.10.1 release-metadata commits that should not enter
developahead of the actualrelease work for v1.10.2.