Repository navigation
fix(native-chat): main's draft-queue tests follow #23524's /clear (fixes main) - #23940
Conversation
…hed clear holds nothing #23726 and #23524 merged minutes apart. #23726's queue gate takes its "blocked" answer from the shared send check, which #23524 changed: a /clear that never committed changed nothing the chat reads, so it refuses no send. Three #23726 tests still seeded a "prepared" /clear record and expected it to hold the queue, and the /clear carry passed a possibly-undefined replacement id where #23726's helper takes a string, so main failed its unit tests and tc:node. The gate code already follows the new rule; the tests now say it: a /clear an older build left prepared holds no draft and Send-now sends it, and a send made while a /clear is starting its replacement is still told to wait. The carry reads the replacement id from the ids it already checked.
|
Agreed, from the queue side. I checked it against my own fix for the same breakage, #23939, which I'm closing in favour of this one. The type fix and the three test rewrites match mine. One thing to keep in view: on main the /clear hand-over of queued messages still runs before the clear is saved. Under #23524 a clear that never saved "never happened", so if that save fails the messages have already left the chat the user is still in. That's harmless today, because the queue isn't advertised yet. #23935 moves the hand-over after A narrow gap remains even then: a crash between the clear's save and the hand-over leaves the messages hidden on the cleared chat, and nothing moves them later. I'll follow up with a re-carry: a cleared chat that still holds queued messages hands them over again. The hand-over is already idempotent. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| describe('the one queue gate', () => { | ||
| it('Send-now refuses while a clear is in doubt, with the refusal any send gets', async () => { | ||
| const working = await workingSend() | ||
| it('a clear an older build left prepared holds nothing: Send-now sends the draft', async () => { |
There was a problem hiding this comment.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Fixed the three stale draft-queue tests —
structured-agent-session-queued-gate.test.tsnow seeds an older build'sprepared/clearand expects Send-now to proceed (ok: true) with the draft handed off;structured-agent-session-queued-messages.test.tsdrains a draft held behind a prepared clear once the turn settles; both reflect #23524's rule that an unfinished clear changed nothing the chat reads. - Updated the in-flight clear test —
structured-agent-session-queued-command.test.tsnow waits on the replacementattachcall (a live signal) instead of the prepared record #23524 no longer writes, while still assertingWAIT_REFUSALfor a send during an in-flight clear. The unusedHOST_TEST_SESSION as SESSIONimport was dropped. - Type fix in
structured-conversation-command.ts— the carry call readsids.sessionId(narrowed tostringinsideif (ids)) instead of the optionalreplacementSessionId; the two are the same value there, so behavior is unchanged. - Stale comments corrected in
structured-agent-session-queued-messages.tsandstructured-agent-session-queued-mutations.ts.
I verified the changed tests pin real behavior rather than passing vacuously: with the send check made to block an unfinished clear again, the Send-now sends the draft and drains when the turn settles tests would fail. Ran the 5 named files (76 pass), pnpm tc:node, oxlint, and oxfmt --check — all clean. A full agent-session-wire/ run passed 1108 tests with one unrelated failure (structured-agent-session-wedged-profile-migration.test.ts:498 times out on base 29c49aec as well, so it is pre-existing and out of scope).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughTests now expect Send-now to accept a queued draft and hand it off when the working turn settles while a prepared, unknown clear remains. The in-flight clear test waits for the mocked Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The runtime behavior is not shown to change, but two updated tests could miss regressions in queued-draft hand-off timing and Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/native-chat/agent-session-wire/structured-agent-session-queued-gate.test.ts (1)
44-51: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert
handedOverAtafter settling the active send.
workingSend()leaves the active submission unsettled.sendNow()records the queued submission immediately, butrig.handoff()checks only the queued-message link. The current assertion can therefore pass before delivery hands the draft over.Suggested fix
- await workingSend() + const working = await workingSend() ... expect(await sendNow(draftId)).toMatchObject({ ok: true }) - await eventually(async () => expect(await rig.handoff(draftId)).toBeDefined()) + expect((await rig.handoff(draftId))?.handedOverAt).toBeUndefined() + await settleAccepted(working, 'a') + await eventually(async () => + expect((await rig.handoff(draftId))?.handedOverAt).toBeDefined() + )src/main/native-chat/agent-session-wire/structured-agent-session-queued-command.test.ts (1)
112-113: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the committed clear record after releasing
attach.The
attachspy only synchronizes with the controller's in-memory pending gate. The send assertion can pass before the clear record is committed. The final assertions can also pass ifsetConversationCommandis skipped afterattach, because they check only the returned replacement ID and draft lists. Assert the committed record to retain coverage of the clear lifecycle.Suggested fix
import { + HOST_TEST_SESSION as SESSION, HOST_TEST_THREAD as THREAD, hostTestMessage, hostTestOperationId } from './structured-agent-session-host-test-data' @@ } expect(await rig.drafts()).toHaveLength(0) expect(await rig.drafts(replacementId)).toHaveLength(0) + expect(rig.store.getRecord(SESSION)?.conversationCommand).toMatchObject({ + command: 'clear', + phase: 'committed', + state: 'completed', + replacementSessionId + }) } finally {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 736ed5fa-0ea4-484b-a3c5-54158e9d2217
📒 Files selected for processing (6)
src/main/native-chat/agent-session-wire/structured-agent-session-queued-command.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-gate.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-messages.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-messages.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-mutations.tssrc/main/native-chat/agent-session-wire/structured-conversation-command.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
Brings in #23940, which fixed on main the same three #23726 queue tests this branch had adapted. Resolution: - The two tests where an older build's unfinished /clear holds nothing take main's version. - The in-flight /clear test keeps this branch's version: it holds the one store write, because a /clear no longer starts the new conversation's agent, so holding that start would never engage. - The draft carry keeps this branch's shape: it runs after the commit and opens the new conversation, under its own lock, only when there are drafts to carry.
…3789, #23812, #23852) into brennanb2025/codex-home-single-writer #23744 moves every agent's folder pre-trust into the PTY spawn hook (and the structured Codex chat create), removing Codex's launch-prep and resume-prep trust writes. markCodexProjectTrusted now takes the config files to write. Conflicts: - agent-trust-presets.ts: #23744's configFiles signature and nested lane acquisition, with this branch's lane rationale (hook installs read and rewrite across awaits); main's "capture->restore window" wording described the grant snapshot this branch removed. - agent-trust-write-deadline.ts: the Codex writer waits only on managed-home installs' inline grant; the real-home background approval holds no lane. #23744's SSH/SFTP writer is gone, so it is no longer named; its 1.5 s short deadline is kept. - codex-trust-config-concurrent-launch.test.ts: this branch's tests (a launch's write lands while a grant session runs; the renamed sibling-failure case) with #23744's new call signature. Outside the conflicts: codex-launch-during-real-home-grant.test.ts passed launchAgent/workspacePath into launch prep, which #23744 removed; its launch helper now runs launch prep and then applyAgentWorkspaceTrust, as the spawn builder does, so it still proves a launch and its project trust land while the real-home approval hangs.
1 participant
ELI5
Main's unit tests and typecheck broke because two changes merged minutes apart disagreed. #23524 made an unfinished
/clearcount as never having happened, so it no longer blocks sending. #23726, which added the queue for messages typed while the agent is working, had three tests that still expected an unfinished/clearto hold that queue, plus one line that no longer typechecked. This brings #23726's tests and that line in line with #23524.What Changed
main(29c49ae), three feat(native-chat): hold mid-turn messages in a host-owned queue #23726 tests failed andtc:nodefailed.structured-agent-session-queued-gate.test.ts: "Send-now refuses while a clear is in doubt".structured-agent-session-queued-command.test.ts: "in flight, refuses a capable send".structured-agent-session-queued-messages.test.ts: "a draft held by a clear left prepared…".structured-conversation-command.ts: a possibly-undefined replacement id passed where a string is required (TS2322 and TS2345).structuredAgentSessionSendBlock. fix(native-chat): an unfinished /clear or refused Codex rewind no longer locks the chat #23524 changed that check so a/clearthat never committed refuses nothing, because it changed nothing the chat reads. The gate code already follows the new rule. The three tests seeded a "prepared"/clearrecord (which fix(native-chat): an unfinished /clear or refused Codex rewind no longer locks the chat #23524 no longer writes) and expected it to hold the queue./clearthat an older build left "prepared" holds nothing: Send-now sends the queued message, and the queue drains when the turn settles./clearis still starting its replacement is still told to wait, and nothing lands on the old chat. That test now waits for the replacement start rather than a record that no longer exists.ids.sessionId).main.Why
/clearagain would bring back the latch fix(native-chat): an unfinished /clear or refused Codex rewind no longer locks the chat #23524 removed. With that old rule put back, the two rewritten tests fail./cleara single write that starts no agent, so there is never a/clearin doubt at all.Linked Issue
Fixes
mainafter #23524 and #23726.Visual Proof
N/A: this changes tests, one type-level line and comments. Behaviour is unchanged from
main's already-merged #23524.Testing
The three test files, plus
structured-conversation-command.test.tsandstructured-agent-session-send-block.test.ts: 5 files, 76 tests pass.tc:nodepasses, where it fails onmain.oxlintpasses on the changed files.Ablation: with the send check blocking an unfinished
/clearagain, the two "holds nothing" tests fail.I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Host-side tests and one type-level line; no platform, SSH, mobile or wire effect.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)