Repository navigation
Remove orchestration forwarding after in-place clear - #26610
brennanb2025 wants to merge 5 commits into
Conversation
5c6a2cd to
bda2c6d
Compare
28f66bb to
5227dcb
Compare
5227dcb to
42fcbed
Compare
|
Review summary (final head Problem: after #26579, What changes for users: nothing visible in the chat. Orchestrated workers and coordinators that run Reviews: rework round 1 and the final review found no blocking issues; their small comment and fixture findings are fixed. Each rebase since then (onto the reworked #26579, then onto its semantic main merge, then onto main after #26579 merged) left all five commits identical by range comparison, and nothing main added since the last tested base touches this PR's files. Validation on this head: 674 tests in 61 explicit files pass under the Node runner (one whole-repo scan test hit its 30 s timeout under machine load in the batch and passed alone, 9/9); fresh CI: all checks finished on Not verified live: Run creation, mail, worker reads/stops and custody in a running app, SSH, phone, Windows and Linux. The screenshots in the body are the reused clear-flow check; they do not prove live orchestration addressing. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (49)
💤 Files with no reviewable changes (5)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change replaces Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to This change makes cleared workers keep their own address, inbox, and transcript instead of following replacement links. No concrete merge-blocking risk was identified; live orchestration paths were not exercised, and old read cursors spanning replacement conversations may need restarting. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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 |
This branch has not been deployed
1 participant
ELI5
/clearnow keeps one conversation, but orchestration still follows the older replacement-conversation graph. That gives worker ownership, mail and output two competing ways to identify the same chat. This follow-up removes the graph so clearing context keeps the worker address, inbox, task and transcript.Follows #26579 (merged), which made
/clearkeep the same conversation.What Changed
No database schema, RPC parameter, stream operation, provider launch or UI change. Older-host replacement decoders and client readers stay (from #26579) for independently updated hosts and clients. Ordinary read cursors retain their fingerprints. A cursor previously made from several conversations can return the existing
source_changedrefusal and must be restarted.Why
Keeping forwarding beside stable identity preserves the bug class. The actual record and existing worker identity already answer ownership; one journal already contains both sides of clear. Removing the graph follows the common pattern of separating durable conversation identity from the provider's lifecycle.
Differences from the common pattern
Linked Issue
N/A — maintainer follow-up from a native chat audit.
Visual Proof
Reused the current parent rework's isolated hidden-app check, whose visible clear flow is unchanged by this orchestration deletion. A stand-in provider keeps the same chat tab, session and earlier messages above one divider, then answers in a fresh provider context. All four permitted configuration hashes match before launch, during activation and after teardown; all recorded app and driver processes exited and the isolated profiles were removed.
Before clear:
After clear and the next reply:
This check did not exercise Run creation, address resolution, mail, worker reads or stops, custody or queues. Tests cover those behaviors; the screenshots do not establish live orchestration addressability.
Testing
macOS, on final head
42fcbed0795(the same five commits rebased onto mainbb44f74d009, which includes merged #26579): 674 tests passed in all 61 explicit files under the Node runner (one whole-repo scan test timed out at 30 s under machine load in the batch and passed alone, 9/9). Freshtc:nodeandtc:clipassed;orca-ci-checkspassed 22/22. Both lockfiles restored; worktree clean. No live orchestration, real-provider, SSH, paired-client, phone, Windows or Linux check is claimed.Review
Rework round 1 found no P0–P2 issues and independently passed 624 tests in 58 files. Its P3 stale clear fixtures are corrected: both now release the old provider chain and proof pointer, with all existing assertions retained. The short final review of
c669aee6037found no P0–P2 and independently passed 625 tests in 58 files; its P3 stale session-ID descriptions are now corrected in the shared address contract and migration comment, with no runtime or assertion change. All five commits remain equivalent after the latest clean rebase onto5f2c39efb62d, verified with the actual bounded range comparison. The coordinator’s first-hand short review of28f66bb90cbwas clean and confirmed the equivalent commits and comment-only correction. The final rebase onto main changed no commit (range comparison: all five identical), and nothing main added since the last tested parent touches this PR's files or conversation addressing.Agent skill upstream boundary
Notes
A cleared supervised worker retains its task and custody but its fresh provider context loses the old dispatch preamble, as in #26579; this cleanup does not add automatic reinjection. Terminal agents retain their CLI behavior. Folder workspaces use the same record authority without requiring Git.
Checklist