Fix queued prompt cancellation state synchronization - #1709
Conversation
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (20)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughPrompt queue items now include a ChangesPrompt Queue Cancellability
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🔵 Low · up to The PR makes prompt cancellation server-authoritative and backfills some legacy web prompts as user-cancellable. It is mergeable with owner awareness, but the migration should be confirmed not to classify integration-owned prompts as removable, and session-wide cancellation should be confirmed as the intended permission boundary. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Summary
PR #1709: Fix queued prompt cancellation state synchronization by @ColeMurray updates 13 files (+105/-25). The change consistently projects server-authoritative cancellability, safely defaults older-server queue items to non-cancellable, and orders authoritative queue broadcasts before correlated cancellation results.
Critical Issues
None.
Suggestions
None.
Nitpicks
None.
Positive Feedback
- The UI hint and server-side cancellation guard use the same eligibility criteria, while the server remains authoritative.
- Defaulting the new schema field to
falsegives a safe rolling-deployment path. - Tests cover successful and rejected ordering, integration-owned prompts, schema compatibility, and UI visibility. Focused verification passed: 258 unit tests, 41 integration tests, and both affected downstream typechecks.
Questions
None.
Verdict
Approve: Ready to merge; no blocking issues found.
There was a problem hiding this comment.
The queue-before-ack ordering and fail-closed client compatibility are sound, but the new contract does not yet establish an authoritative cancellation boundary. cancellable duplicates the repository mutation predicate and derives ownership from caller-supplied provenance, so the projection can drift from deletion and malformed/spoofed enqueue metadata can create either unremovable web work or removable integration work. Please make cancellation ownership canonical at the authenticated enqueue boundary, reuse one policy for projection and mutation, and cover each ownership signal independently in integration tests.
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
Summary
Testing
npm run build -w @open-inspect/sharednpm run typecheck -w @open-inspect/control-planenpm run typecheck -w @open-inspect/webnpm test -w @open-inspect/shared -- --run src/types/server-messages.test.tsnpm test -w @open-inspect/control-plane -- --run src/session/message-repository.test.ts src/session/message-queue.test.tsnpm test -w @open-inspect/web -- --run src/components/queued-prompt-stack.test.tsx src/components/session-timeline.test.tsx src/components/session-timeline-scroll.test.tsx src/lib/session-socket/reducer.test.tsnpm run test:integration -w @open-inspect/control-plane -- --run test/integration/websocket-client.test.tsCreated with Open-Inspect
Summary by CodeRabbit
New Features
Bug Fixes