Repository navigation
Agent Mode: model who drives a conversation as a ConversationDriver enum - #16355
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR replaces the overlapping AIConversation ownership booleans with a ConversationDriver enum, keeps compatibility wrappers for existing call sites, and adds focused unit coverage for driver predicates, constructor mapping, restore mapping, and wrapper semantics.
Concerns
- No blocking concerns found. I did not find material spec drift; the provided spec context contains no approved repository spec. The security pass did not identify new input, auth, secrets, logging, command execution, or data-handling risks.
- Comment audit: the added/changed doc comments on ConversationDriver and the legacy wrapper methods document current API semantics and migration guidance without narrating implementation internals; the new test comment explains the compatibility invariant being asserted.
- Test audit: the new tests cover the per-variant predicate table and the legacy constructor/wrapper behavior affected by the refactor.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /warp-agent-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
28fb8e0 to
1589cda
Compare
c9abdfe to
955b01c
Compare
1589cda to
95fac3c
Compare
955b01c to
0051d79
Compare
95fac3c to
f1f397f
Compare
af940d8 to
d2de106
Compare
2e84c08 to
fe5c450
Compare
d2de106 to
a91b229
Compare
Replace the three overlapping ownership booleans on AIConversation (is_viewing_shared_session, is_cli_agent_transcript, is_remote_child) with a single ConversationDriver field plus derived behaviour predicates. The legacy accessors stay as thin wrappers so call sites are unchanged; only the in-file reads in conversation.rs move to the predicates they actually mean.
a91b229 to
8be8e96
Compare
|
@kevinyang372 Warp CI failed on this PR. May the windows runner test factory diagnose it, commit fixes, and push to this same branch? Explicit consent also opts this PR into automatic handling of future CI failures; we will not open a replacement PR or merge. Please reply to this request with an explicit yes or no, mentioning @warp and “windows runner test”. Responding as windows runner test: Open session · View in factory |

Description
Prep PR for the ACP harness stack (inserted between 2/4 and 3/4), from the design discussion on #16331. No behavior change.
AIConversationcarried three overlapping booleans that each answered part of "who owns this turn":is_viewing_shared_session(~25 call sites conflating "not owned here" with "inputs arrive via the stream"),is_cli_agent_transcript, andis_remote_child. The next PR needs a fourth mode (an external harness that authors the stream but whose lifecycle this client does not own), and bolting on another boolean would mean auditing every site again.What:
ai/agent/conversation_driver.rs:enum ConversationDriver { Native, SharedSessionViewer, CliAgentTranscript, RemoteChild }with exhaustive-match predicates that call sites can ask instead of "is this a viewer?":owns_turn_lifecycle,executes_tool_calls_locally,reconstructs_inputs_from_messages,reports_task_status,is_read_only_ui,is_persisted_locally.AIConversationstores a singledriver(plusdriver()/set_driver()); the three booleans and their setters remain as thin wrappers whose docs point at the predicate to migrate to, so no call site outsideconversation.rschanges. Insideconversation.rsthe three input-reconstruction sites and the persistence guard move to the predicates they actually mean.parent_conversation_id) is an orthogonal axis and is left alone.Audit: no production or test path ever sets two of the old booleans on one conversation (
is_cli_agent_transcriptonly viastart_new_conversationfrom mutually exclusiveAgentViewEntryOrigins;mark_as_remote_childonly on a fresh child;set_is_viewing_shared_session(true)only on cloud-restored/forked conversations withis_remote_childhard-coded false), so a flat enum loses nothing.set_is_viewing_shared_session(false)only demotes aSharedSessionViewerback toNative, matching the old independent-bool semantics under that invariant.Deliberately not migrated (candidates for a follow-up, each a one-line change once the enum exists):
local_agent_task_sync_model.rs(reports_task_status),agent_input_footer(is_read_only_ui), and the viewer-only timing / exchange-move / temp-dir-cleanup sites inconversation.rs.Linked Issue
ready-to-specorready-to-implement.Testing
New
conversation_driver_tests.rs(per-variant predicate table) andconversation_tests.rscases for constructor → driver mapping and wrapper/setter semantics. Ran the conversation, blocklist controller, history model, shared session, pane group, orchestration, task sync, and agent-view suites (~1,250 tests) — all pass.cargo clippy -D warningsand./script/formatclean../script/runAgent Mode
Co-Authored-By: Warp agent@warp.dev