refactor(conversations): Launch, part b: fresh start_conversation, the reservation and one child spec builder - #2243
Conversation
BinaryBourbon
left a comment
There was a problem hiding this comment.
Verdict: APPROVED
Reviewed the complete diff at 34d45e3176cd9331f1a1686d594dd1529d6d450c against 15dd3550ea765615b85cc8a8eb0c9f1ec2b940f4. No actionable defects found.
Coverage: apps/fountain/lib/fountain/conversations.ex, apps/fountain/lib/fountain/conversations/launch.ex, apps/fountain/lib/fountain/conversations/wake.ex, and apps/fountain/lib/fountain/conversations/rehydrator.ex.
I challenged the extraction across tenant-scoped admission, inference selection, persistent-home fallback, transaction/lock ordering, failed-start compensation, audit/activation timing, public delegates, and prompt replay. The moved reservation, failure cleanup, home selection, and inference-resolution functions have identical parsed Elixir syntax trees after accounting for module qualification and visibility. Existing Launch functions are unchanged. Fresh launch retains its validation and side-effect sequence; all three child-spec call sites construct the same options as before, including the rehydrator's explicit initial_prompt: nil.
Validation: parsed all four files at both revisions with pinned Elixir 1.19.2; normalized function comparisons and git diff --check passed. Inspected the existing prompt-replay, launch commit-boundary, admission-lock isolation, rehydrator, and conversation-creation regression assertions. GitHub's required CI check and secret scan succeeded for this head. I did not run the application test suite locally (rl-env is unavailable); this is an independent source review, not a Review Loop verification attestation.
…e reservation and one child spec builder Stage 7b of #2175 (one owner per conversation lifecycle verb). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Jake Gaylor <jhgaylor@gmail.com>
34d45e3 to
4e6f2cf
Compare
Stage 7b of #2175 (one owner per conversation lifecycle verb). Depends on 7a
(#2225, merged) and stage 2 (#2238, merged), which already gave
Wakethesandbox probe and
start_conversation_server/4. Re-anchored againstorigin/mainat15dd3550, well past4224258d: stages 1, 2, 3b, 6 and 7aof #2175 had all landed by then, so several helpers this stage needs were
already opened as
@doc falsedoors (with_sandbox_lock,resolve_vault_id,resolve_environment_id,resolve_inference_credential_id,resolve_permission_policy,resolve_sandbox_provider,broadcast_sidebar_update,check_execution_limits,resolve_saved_inference). Stage 3 (#2211) has not landed yet, sorequire_provider_commit_boundary/0andmint_machine_name/3were stillprivate; opened them here with the same
@doc falseshape stage 3 will use,so that merge is a no-op collision at worst.
What moved (
conversations.ex5,277 → 4,972;launch.ex352 → 783)Into
Fountain.Conversations.Launch, cut with their comments, docstrings andlock-order notes:
start_conversation/2(all three clauses, with its doc) — thesandbox_idattach clause, the fresh clause, and the
{:home, sandbox}/unique-index-race recovery in its
else.fail_initial_start/2and its sole helperpending_initial_binding?/4—the delayed-start compensation, machine → parent → sandbox lock order kept
exactly.
reserve_initial_conversation/4— the tenantFOR SHAREwaits, thechannel-rotation unbind, and the
with_sandbox_reservation/3-nested insertof sandbox + conversation + allowance.
InferenceBinding.reserve/2(viareserve_inference/1, now a door) still runs inside this same transaction.home_or_new/5(both clauses).resolve_admission_inference/5— madedef(not just moved) becauseattach_conversation, which stays inConversationsuntil 7c, calls ittoo; it's the one function stage 7c will need a door for in reverse once
attach_conversationmoves.Calls that now cross the module boundary gained a
Conversations.prefix(e.g.
Conversations.require_provider_commit_boundary()); calls thatstopped crossing it (
Launch.unbind_rotated_channel/2,Launch.restore_rotated_channel/2, which the moved code used to reach fromconversations.ex) lost theirs, sincereserve_initial_conversationand thefresh clause now live beside them.
One child spec builder
Launch.child_spec/4(three required args, one optionalextrakeyword —still callable as
child_spec/3) replaces the Horde child spec literal thatwas written three times:
start_conversation's fresh clause (moved here) now calls it directly.Wake.start_conversation_server/4(conversations/wake.ex) callsLaunch.child_spec/3.Conversations.Rehydrator's boot sweep (conversations/rehydrator.ex)calls
Launch.child_spec(conv.id, conv.sandbox_id, runtime_module, initial_prompt: nil)— the sameinitial_prompt: nilit always sent,now appended onto the shared three keys instead of duplicating them.
All three produce the identical term as before:
{conversation_id:, sandbox_id:, runtime_module:}for the first two,{conversation_id:, sandbox_id:, runtime_module:, initial_prompt: nil}forthe rehydrator.
test/fountain/conversations/prompt_replay_test.exspins thefirst two shapes exactly (
Enum.sort(Keyword.keys(spec_args())) == [:conversation_id, :runtime_module, :sandbox_id]), unedited and stillgreen.
What stayed in
conversations.ex, and whyattach_conversation/3stays until stage 7c. It went fromdefpto a@doc false def(one comment naming #2217) because the movedstart_conversation'ssandbox_idclause and its{:home, ...}/unique-index-race branches call it — three call sites, all now
Conversations.attach_conversation/3.Every other private helper the moved code depends on stayed put and got the
same
@doc falsedoor treatment already established by stages 7a/6/2(comment naming #2217,
defp→def, no logic touched):resolve_sandbox_mode/2,resolve_sandbox_api_access/2,check_sandbox_api_name/2,resolve_parent_id/2(all their clauses),resolve_admission_limits/2,insert_conversation_row/1,after_conversation_created/1,record_execution_allowance_created/3,get_root_conversation_id/1,broadcast_graph_update/1,record_sandbox_usage/2(all five clauses),stamp_terminated_at/1,reserve_inference/1,require_provider_commit_boundary/0andmint_machine_name/3(all five clauses, see the stage-3 note above).check_sandbox_api_attach/2,create_attached_conversation/3anddeliver_attach_prompt/3are untouched — onlyattach_conversationreachesthem, and it didn't move.
Ownership comments
Four
_unsafe_*calls became remote once their callers moved intoLaunch;each carries a
# ownership: ...comment within credo's ten-line lookback:Agents._unsafe_current_version_id/1(comment moved with the code, alreadypresent),
Conversations._unsafe_get_conversation!/1(conv is the rowreserve_initial_conversationjust created, same launch),Conversations._unsafe_find_home/4(twice — inhome_or_newand in theunique-index-race recovery — both note the scoped
get_agentthat ranfirst), and
Conversations._unsafe_running_turns_elsewhere/2infail_initial_start(theFOR UPDATEre-read two lines above it).mix credois clean.Invariants carried
conversation.created) still fires after the insert transactioncommits, never inside it;
start_conversationstays inaudit_guardrail_test.exs's@must_auditand the guardrail (which callsConversations.start_conversation, now a delegate) still passes.Billing.check_spend/1moved once, with the fresh clause; none added.@transient_errorsvocabulary: untouched, nothing in this moverenamed an error atom.
Task.async. No server change —conversation_server.exuntouched.conversation_server_size_test.exs) is unaffected; thisfile isn't the server.
Delegate left
Conversations.start_conversation(attrs, opts \\ []), do: Launch.start_conversation(attrs, opts)forteam.ex:389/:764,team/schedules.ex:303, and the ~60 test call sites acrossconversations_start_test.exsand friends — grepped the whole repo, noother caller needed a second door.
Tests run unchanged, by explicit path from
apps/fountain(357 total, 0 failures)test/fountain/conversations_start_test.exs,conversations/launch_commit_boundary_test.exs,conversations/launch_credential_set_test.exs,conversations/sandbox_mode_test.exs,conversations/initial_start_failure_test.exs,conversations/admission_lock_isolation_test.exs,conversations/prompt_replay_test.exs,conversations/rehydrator_test.exs,audit_guardrail_test.exs,team_test.exs,fountain_web/controllers/conversation_controller_test.exs— run together,357 tests, 0 failures.
../../ee/test/fountain/credits_enforcement_test.exsrun separately from
apps/fountain: 11 tests, 0 failures.mix format --check-formattedandmix credoclean on all four touchedfiles.
mise exec -- mix precommit --fullfrom the worktree root,DATABASE_URLset: PASSED (10 stages, 332s).
No behaviour change, no changelog fragment.
Closes #2217
🤖 Generated with Claude Code