-
Notifications
You must be signed in to change notification settings - Fork 11
🤖 Remove interrupt sentinel - models resume naturally #133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6efa8fb to
f881d96
Compare
|
@codex review |
|
The static checks are failing but I cannot see the error details in the CI logs. The annotation only says 'Process completed with exit code 2'. All other checks (builds, integration tests, E2E tests) are passing. Can someone please help identify what the lint/typecheck/fmt issue is? |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ 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 👍. |
Integration testing proves that models can successfully resume interrupted streams without synthetic [INTERRUPTED] or [CONTINUE] sentinels. The partial assistant message itself provides sufficient context. Benefits: - Simpler codebase (removed addInterruptedSentinel function) - Cleaner message history (no synthetic user messages) - Natural conversation flow - Reduced complexity in message transformation pipeline Test: tests/ipcMain/resumeWithoutSentinel.test.ts demonstrates successful resumption by mocking out the sentinel injection and verifying the model completes tool execution correctly.
3a1faa5 to
ebde0d0
Compare
- Removed resumeWithoutSentinel.test.ts (redundant with existing test) - Enhanced resumeStream.test.ts to test both Anthropic and OpenAI providers - Existing test already validates resumption works without sentinels
This reverts commit e732a87.
Summary
Removes the
addInterruptedSentinel()function and all interrupt sentinel logic. Integration testing proves that models can successfully resume interrupted streams without synthetic[INTERRUPTED]or[CONTINUE]messages.Motivation
The original implementation injected synthetic user messages (
[INTERRUPTED]) after partial assistant messages to help the model understand context when resuming. However, this added complexity without clear benefit.Question: Can models resume naturally from partial messages alone?
Investigation
Created integration test
tests/ipcMain/resumeWithoutSentinel.test.tsthat:addInterruptedSentinel()to be a no-op (identity function)sleep 5 && echo 'NO_SENTINEL_TEST_SUCCESS'Result: ✅ Test passes. Model naturally continues from partial message and completes the task.
Messages sent to API during resume (no sentinel):
[ { "role": "user", "content": "Run this bash command: sleep 5 && echo 'NO_SENTINEL_TEST_SUCCESS'" }, { "role": "assistant", "content": "I'll run that bash command for you." } ]Model saw the partial response and naturally continued by executing the tool call.
Changes
Removed:
addInterruptedSentinel()function frommodelMessageTransform.tsaiService.tsAdded:
Benefits
✅ Simpler codebase - Removed 30+ lines of sentinel injection logic
✅ Cleaner message history - No synthetic user messages
✅ Natural conversation flow - Models handle partial messages natively
✅ Reduced complexity - One less transformation in the message pipeline
✅ No user confusion - Users won't see
[INTERRUPTED]in exported conversationsTesting
resumeStream.test.tscontinues to pass (was already testing without sentinel awareness)Migration
No migration needed. This is purely a removal of unnecessary complexity.
Generated with
cmux