Skip to content

fix(client): give the Antigravity test fixture the SessionContext field it lost - #2385

Open
L42y wants to merge 2 commits into
first-tree-ai:mainfrom
L42y:fix/antigravity-fixture-note-turn-start
Open

fix(client): give the Antigravity test fixture the SessionContext field it lost#2385
L42y wants to merge 2 commits into
first-tree-ai:mainfrom
L42y:fix/antigravity-fixture-note-turn-start

Conversation

@L42y

@L42y L42y commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

What

Adds the required noteTurnStart to the Antigravity handler test's SessionContext fixture.

Why

SessionContext gained a required noteTurnStart in #2378 (fb40bc24d). The Antigravity provider (#2375, 387524153) was written before that field existed and merged after it, so its fixture builds a SessionContext object literal without the field — and 387524153 is current main, so main's client typecheck has been failing on it since that merge. Every other provider fixture (e.g. providers/claude/__tests__/provider-error-result.test.ts:213) already sets it.

Nothing about the provider itself is wrong: the fixture is catching up to the interface.

Verification

  • vitest run src/providers/antigravity/__tests__/handler.test.ts on main14/14 pass before this change, which is why the failure surfaces only in typecheck, not in the test run.
  • Same suite after the change — 14/14 pass.
  • Biome clean on the changed file.

Found while investigating CI on #2384, where this inherited failure appears in the merge result and is unrelated to that PR's changes.

🤖 Generated with Claude Code

…ld it lost

`SessionContext` gained a required `noteTurnStart` in first-tree-ai#2378. The
Antigravity provider (first-tree-ai#2375) was written before that field existed and
merged after it, so its handler fixture builds a `SessionContext`
without it and main's typecheck has been failing on the object literal
ever since — every other provider fixture sets it.

The tests themselves pass; nothing about the provider is wrong. This is
the fixture catching up to the interface.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact-head review: 1de53b709b2b60408ec1c61aea0235ec0974a7a4. No actionable issue in this one-line fixture repair. noteTurnStart: vi.fn() satisfies the required SessionContext callback without weakening the interface, suppressing type errors, skipping tests, or changing production behavior.

Independently verified in a clean detached worktree: pnpm check passed (36 warnings, 22 infos), uncached build 5/5, uncached monorepo typecheck 9/9, and the complete Antigravity handler suite 14/14. The current CI Lint & Type Check, Test Client & Web, and both CLI test shards also pass.

CI is not fully green: run 33733656062 fails Test Server at admin-sessions-suspend-terminate.test.ts:1125, in the durable-ACK fallback when the result wake is lost (503 instead of 200; 1 failed / 3490 passed). This PR changes only the client fixture; the entire packages/server tree is identical between this head and base 38752415320c97c0bd122aa4afb13c3ff60b1b06 (tree 8788f653893bc38bc8d83981a2bcf4c1719e9bda). I reran that exact failed case in the repository's isolated PostgreSQL 17 Testcontainers setup on this head: 1 passed, 48 intentionally skipped. That does not establish the CI failure's cause, prove flakiness, or make the failed run green.

Approving the reviewed fixture change only. The independent server CI failure still needs resolution before merge; this approval does not waive checks or transfer to a successor head. No source-branch edits or merge were performed.

@L42y

L42y commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

The only red checks here are Test Server and its aggregate Test job, from run 33733656062. The failing test is admin-sessions-suspend-terminate.test.ts > Terminate with apply-ack (?waitForApply=true) — the Web Reset path > falls back to the durable ack when the result wake is lost (503 vs 200). That suite is unrelated to the one-line Antigravity fixture change; the same Test Server job passed on PR #2384 at 08:35 UTC, and this PR has an exact-head approval at 1de53b70.

Could you rerun the failed jobs so this small fixture fix can land and unblock the inherited red check on other open PRs?

@yuezengwu

Copy link
Copy Markdown
Contributor

@L42y I investigated the failed CI at 1de53b709b2b60408ec1c61aea0235ec0974a7a4. Test Server, run 33733656062 has one failing case: admin-sessions-suspend-terminate.test.tsfalls back to the durable ack when the result wake is lost, line 1125 (503 instead of 200). The aggregate Test check fails because of this job.

The test contains a reproducible timing race. At lines 1109–1123 it sets the RPC deadline to 50 ms, waits for ws.send with vi.waitFor (Vitest 3.2.4 defaults to a 50 ms polling interval), and only then persists the acknowledgement. The route starts its timer before sending and performs a single durable read after expiry. Polling delay plus the database write can let that read happen before the acknowledgement exists, legitimately returning 503.

I checked this in an isolated worktree and disposable PostgreSQL containers:

  • The unchanged focused test passed once, consistent with an intermittent failure.
  • Changing only the observation interval to 75 ms reproduced the same assertion failure; the response was The agent's client did not confirm the terminate: Timed out waiting for the computer to reply.
  • With the same 75 ms observation interval, persisting and verifying the acknowledgement before explicitly rejecting the waiter, without sending NOTIFY, returned 200 with state: evicted and applied: true.

Please fix the test's ordering so it deterministically exercises a persisted acknowledgement with a lost wake: synchronize the durable write before triggering the timeout/fallback, and assert the store succeeded. Keep coverage for a genuinely missing acknowledgement. Simply rerunning or slightly increasing the short timeout leaves this race in place. Please fix or incorporate a fix for the flaky test and obtain a passing Test Server result.

This is an inherited test-code issue, also present on current main cb8d386c69d0b9b5a3ff3f67d1526d46fa280545; it is not introduced by this PR's one-line Antigravity fixture correction. The CI log lacks per-step timing, so the controlled reproduction establishes the race and matching failure mechanism rather than an exact timing trace of that CI run.

@bestony bestony left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed latest head 1ef1cda7846a1e6d6a724a8c35099d5ca8963.

The new server-test stabilization deterministically waits for the command frame, asserts the durable ack write succeeds, then advances the timeout; the existing no-ack coverage remains unchanged. The Antigravity fixture change remains correct. Biome and client typecheck pass locally; GitHub CI is fully green, including Test Server, Test Client & Web, Lint & Type Check, CLI shards, and aggregate Test. No actionable findings.

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at 1ef1cda7846a1a6e17d6a724a8c35099d5ca8963 after a fresh review of both changed files. The Antigravity fixture supplies the required noteTurnStart field. The server regression now waits directly for the command frame, asserts that the durable acknowledgement was stored, and only then advances the controlled timeout. It sends no NOTIFY, retains the existing missing-ack coverage, and restores real timers in finally. This addresses the inherited CI race raised in my earlier comment.

Validation in a clean detached worktree: all 49 suspend/terminate tests and 14 Antigravity handler tests pass; repository Biome check and Client/Server typechecks pass. Current-head GitHub CI is green, including Test Server. A fresh merge-tree check against current main cb8d386c69d0b9b5a3ff3f67d1526d46fa280545 is conflict-free; the executed tests above are on the PR head. No actionable findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants