fix(chat): queue parallel ask_user_question tool calls - #119
Closed
amostalong wants to merge 612 commits into
Closed
amostalong wants to merge 612 commits into
amostalong wants to merge 612 commits into
Conversation
Extract the dotnet runtime download / system-dotnet probe out of csharp_lsp::assets into a reusable dotnet_runtime module, keeping the legacy csharp-lsp cache directory so existing installs do not re-download. The upcoming C# compile-server sidecar hosts on the same runtime.
Move the C# compile step of unity_execute / unity_run_states out of the Unity Editor process into a Locus-managed compile server (.NET 10 + Roslyn 4.14, stdio JSON-RPC), gated by a new default-off unity_sidecar_compiler setting: - locus_compile_server/: framework-dependent server with verbatim ports of the snippet/run_states wrappers and diagnostic formatting, a fingerprint-keyed metadata reference cache (PrefetchMetadata, no file locks), a per-domain-generation session image registry (reference_session_images lets later snippets use earlier snippet types), and a compile/raw entry reserved for hot-reload; xunit parity tests pin the generated source to the Unity implementation. - src-tauri csharp_compile: process manager on the shared dotnet runtime, compile params cache synced via the new get_compile_params pipe roundtrip (invalidated with the type index), settings commands, and warm-up on Unity connect. - unity_bridge: execute/run_states route through the sidecar when enabled and ship DLL bytes via the new execute_loaded / run_states_loaded messages; compile diagnostics return to the agent in the legacy wording without occupying Unity; any sidecar failure (including an older Unity plugin) falls back to the in-Unity path. - locus_unity: reference collection split into path collection + materialization, get_compile_params provider with mtime/size fingerprint and domain generation, and the *_loaded handlers reusing the existing execution/session pipelines. - settings UI toggle under C# code analysis; compile-server published to gen/compile-server and bundled as a resource.
Phase 6 of the compile-sidecar plan, keeping the in-Unity fallback: - unity_sidecar_compiler now defaults to on; session counters (sidecar compiles / compile errors / fallbacks) are tracked and shown in the settings card for rollout observability. - compile_named / invoke_named pre-compile View Scripts in the sidecar via the new compile/viewScript method (path-qualified View Script diagnostic format ported verbatim). The result rides in optional assembly_b64 / assembly_id request fields: a current plugin loads the bytes on a cache miss (falling back to its local compiler on any load failure), an older plugin ignores the fields and compiles from source unchanged — the sourceHash + domainFingerprint cache key semantics stay Unity-side and untouched. invoke_named_cached fast path is unaffected. Final removal of Locus.Roslyn.dll and the in-Unity compile paths stays out of scope per the plan.
Asynchronous sidecar failures (warm-up errors, runtime download problems, runtime fallbacks to the in-Unity compiler) were only written to the backend log; the settings card showed them only on the next visit. Emit a csharp-compile-status event on every state change (spawn result, warm-up outcome, fallback, counters) and subscribe in the settings card, mirroring the csharp-lsp-status pattern.
A disconnect/reconnect cycle could enter a self-sustaining cancel loop: Mono delivers the old connection's read-loop finally late, after the client has already reconnected and submitted a new execute, and the unconditional CancelActiveExecuteCode there killed that fresh request — which re-triggered the client's 15s no-progress reconnect, minting the next stale teardown. Guard the cancel with the same is-current- connection check the writer cleanup already uses. Also take the get_compile_params roundtrip off the critical path it was aggravating: the per-request fingerprint hashing now runs on the pipe worker instead of the editor main thread (only the once-per-domain path collection still posts there), the Rust side caps the roundtrip at 10s before falling back to the in-Unity compiler, and the params are prefetched in the background when Unity connects.
The send-stage progress always said "Sending execute_code to Unity" even when the sidecar path ships execute_loaded, which misdirected the stall diagnosis. Derive the label from the message actually sent.
…ead pipes Two defects surfaced by live testing: - Sidecar snippet assemblies were named __LocusSnippet_*, escaping the type index's __LocusRuntimeAsync_ skip prefix: every executed snippet changed the index fingerprint, forcing a full main-thread re-export (and a multi-MB pipe write) before the next unity_execute. Name them __LocusRuntimeAsync_* again — compatible with the skip list of every plugin version — and also add __LocusSnippet_ to the skip list for assemblies already loaded in running sessions. - When a pipe write fails (client gone), Mono can sit on the dead connection's pending ReadLineAsync for seconds; the single pipe instance stays occupied and reconnects fail with ERROR_PIPE_BUSY (observed as a 20s 'all pipe instances busy' stall). Dispose the dead server stream on write failure so the listen loop accepts again immediately. Also add timing logs along the execute path (type index refresh, compile params roundtrip, sidecar compile, send attempts, first Unity progress, no-progress gate) for diagnosing future stalls.
extract_refs_with_resolver used to run its own line scanner (phase1_parse) on top of parse_yaml_docs, so every scene/prefab was parsed twice per full scan — three times in the watcher, which also ran collect_guids_from_lines. The parser main loop now optionally captures guid-bearing flow maps as RawYamlRef while it walks the file, and references.rs resolves them against the YamlDoc list it already produced. parse_yaml_docs also streams text.lines() instead of collecting a Vec<&str>. extract_refs / extract_refs_with_resolver keep their signatures as thin wrappers, so existing callers and tests are unchanged.
parse_importer_subassets ran a full serde_yaml deserialization on every .meta file even though only internalIDToNameTable / fileIDToRecycleName / spriteSheet tables can produce entries. A byte-level substring precheck now returns early for the vast majority of metas (NativeFormatImporter, DefaultImporter, MonoImporter), which dominate the meta-parse phase of a full scan and every watcher reprocess.
Full scan recorded the .meta file's size/mtime on every binary asset (textures, audio, models, shaders), so the post-scan reconcile saw a size mismatch on all of them and re-processed the whole set serially after each scan, and the asset-size stat undercounted exactly the largest files. The parallel meta phase now probes the content file and the materialize loop uses those stats, replacing its serial exists() call; the yaml backfill keeps max(meta, content) mtime, matching what the watcher writes. The fixed ignore list (Library/Temp/Build/...) applied at every depth and silently dropped legal asset folders like Assets/Build. The scan roots are only Assets/ and Packages/, so the list is replaced by Unity's own rules: hidden names, ~-suffixed names (Samples~), cvs, plus node_modules. Scanner, watcher event filter and discovery walk share the same predicate. Directory creates/renames only report the directory itself on Windows, so renamed-in folders stayed invisible until the 10-minute discovery sweep; structural events now walk the subtree and queue its metas immediately. Duplicate-GUID metas produced INSERT OR REPLACE pairs whose replaced asset_objects rowid stranded the first copy's FTS row forever; shadow nodes and colliding object_keys are now deduped before insert. The watcher classified unknown extensions as OtherYaml where full scan says MetaOnly, and used is_file() for exists_on_disk where full scan counts directories (folder assets); both sides now share one classification.
…tor probe MonoMod.RuntimeDetour 21.12.13.1 + MonoMod.Utils + Mono.Cecil merged into locus_unity/Editor/Detour/Locus.Detour.dll (bun run unity:bundle-detour); unity_hot_reload config flag (default off) with settings toggle and hot-patch counters riding the csharp-compile status payload; hot_reload_probe pipe message reports codeOptimization and a detour/restore self-test.
…dex export off the main thread HotDiff is a pure member-level syntax diff (parsed with the project's real defines): body/accessor/ctor edits, new private members and new types are hot; field layout, consts, static initializers, signatures, generics, partials and new Unity messages are cold with explicit reasons. Protocol version 2 on both sides. export_type_index handlers now run on the pipe worker (no Unity API involved).
…ding rewrites compile/hotPatch diffs, rewrites and compiles edited files into a __LocusHotPatch_ assembly: file-local types are renamed and every reference to them requalified so patched bodies bind the ORIGINAL assembly's types and statics (object identity and static state never split); static initializers/cctors are inert in the patch; accessibility is bypassed via IgnoreAccessibility + IgnoresAccessChecksTo; an instance-field metadata guard rejects stale baselines. End-to-end and golden tests pin the rewrite.
hot_patch_loaded loads the patch assembly on the main thread (between frames) and redirects each original method, all-or-nothing with rollback; re-patching a method releases the previous detour first so redirects never stack. Domain-generation guard rejects stale patches, the Debug code-optimization gate is enforced editor-side, hot_patch_dispose releases by patch id or all, and __LocusHotPatch_ assemblies stay out of the type index.
The coordinator captures a per-file baseline at the first write/edit after the last recompile, probes the editor gates (plugin version, Debug optimization, detour self-test), compiles via compile/hotPatch, ships hot_patch_loaded, and layers new public types into the cached type index (TI-C). Cold classifications queue for unity_recompile, which clears all hot-reload state on convergence; deterministic compile errors surface directly. unity_hot_reload joins unity_recompile as a sequential tool-round barrier.
… the sidecar index/types walks the prefetched reference metadata with Unity's exact skip/dedup/ordering rules, so refresh_unity_type_index no longer reflects over the whole AppDomain or ships a multi-MB export — only the cheap Unity fingerprint roundtrip remains, keeping one fingerprint scheme for currency checks and the skill-package delta channel (deliberate deviation from the plan's params-fingerprint idea). The Unity export stays as the degradation path; hot-patch new types keep layering via TI-C.
…m-fixes fix(macos): apply cross-platform build fixes from r1n7aro#62
… a bundled models.dev catalog
… auto and command channels
… and codex tool search
…preview folding optional (r1n7aro#114)
…age output, and keep output tails when truncating
… the inactive-skill warning
When the LLM emits multiple `ask_user_question` tool calls in the same response (e.g. collecting 3 different inputs at once — name + license + publish flag), the previous single-value `pendingQuestion` field only ever showed the last question in the UI and the other N-1 oneshot receivers in the backend were never resolved, so the run hung after the first answer. The LLM contract requires all `tool_result` blocks to be returned together (one per `tool_use_id`), so the backend `join_all` in `agent::instance::execute_tool_round` is the right gate. The fix is to give the frontend a queue and dequeue it FIFO, while the backend already collects answers naturally. ## Frontend (TS) - `StreamState.pendingQuestion: PendingQuestion | null` -> `pendingQuestions: PendingQuestion[]` - `setQuestion` mutation -> `enqueueQuestion` (with de-dup by id) - `clearPendingInput` / `clearPendingInputs` filter / empty the array - `answerQuestion` in `stores/chat.ts` and `useEmbeddedChatSession.ts` shifts the head and sends the answer - `SessionRuntimeSnapshot` gains `pendingQuestions?: PendingQuestion[]` (the legacy `pendingQuestion?: PendingQuestion | null` is kept and used as the queue head for backward compat) - Keep `pendingQuestion` as a computed alias returning the head, so every existing template binding keeps working - Expose `pendingQuestionCount` for templates that want to render a "Q 1 / 3" indicator (out of scope for this PR, just plumbing) ## Backend (Rust) - `SessionRuntimeSnapshot` gains `pending_questions: Vec<PendingQuestion>` - `runtime::apply_event_to_snapshot` retains-then-pushes on `StreamEvent::AskUser` and retains matching ids on `StreamEvent::InputAnswered`, mirroring the existing `pending_tool_confirms` pattern - The legacy `pending_question: Option<PendingQuestion>` is kept in sync as the queue head so older frontend builds that still read it see a sensible value - `RunStart` and `fallback_runtime_snapshot` clear / init the new vec ## Tests - Existing `askUser` and `inputAnswered` tests in `useStreamReducer.test.ts` updated for the new state shape and `enqueueQuestion` mutation - New regression test: queues 3 askUser events, asserts FIFO order, asserts `inputAnswered` only removes the matching entry - New de-dup test: repeated `enqueueQuestion` for the same id does not grow the queue No backend agent loop change needed: `mod.rs::execute_single_tool` is already invoked in parallel via `futures::future::join_all`, which naturally waits for all `execute_ask` futures to resolve before the tool_results go back to the LLM in a single batch. Design-by: amostalong@126.com Co-authored-by: deepseek-v4-flash
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When the LLM emits multiple
ask_user_questiontool calls in the same response (e.g. collecting 3 different inputs at once — name + license + publish flag), the currentpendingQuestion: T | nullfield only ever shows the last question in the UI and the other N-1 oneshot receivers in the backend are never resolved, so the run hangs after the first answer.The LLM contract requires all
tool_resultblocks to be returned together (one pertool_use_id), so the backendjoin_allinagent::instance::execute_tool_roundis the right gate. The fix is to give the frontend a queue and dequeue it FIFO, while the backend already collects answers naturally.Reproduction
Ask Claude to do something that requires 3 inputs at once:
When Claude emits 3
ask_user_questiontool calls in one response, the current code:futures::future::join_allFix
Frontend (TS)
StreamState.pendingQuestion: T | null→pendingQuestions: T[]setQuestionmutation →enqueueQuestion(with de-dup by id)clearPendingInput/clearPendingInputsfilter / empty the arrayanswerQuestioninstores/chat.tsanduseEmbeddedChatSession.tsshifts the head and sends the answerSessionRuntimeSnapshotgainspendingQuestions?: PendingQuestion[](legacypendingQuestion?kept as the queue head for backward compat)pendingQuestionexposed as a computed alias returning the head, so every existing template binding keeps workingpendingQuestionCountexposed for templates that want to render a "Q 1 / 3" indicator (plumbing only — UI follows up separately)Backend (Rust)
SessionRuntimeSnapshotgainspending_questions: Vec<PendingQuestion>runtime::apply_event_to_snapshotretains-then-pushes onStreamEvent::AskUserand retains matching ids onStreamEvent::InputAnswered, mirroring the existingpending_tool_confirmspatternpending_question: Option<PendingQuestion>kept in sync as the queue head so older frontend builds still see a sensible valueRunStartandfallback_runtime_snapshotclear / init the new vecNo backend agent loop change needed:
mod.rs::execute_single_toolis already invoked in parallel viafutures::future::join_all, which naturally waits for allexecute_askfutures to resolve before the tool_results go back to the LLM in a single batch.Tests
askUserandinputAnsweredtests inuseStreamReducer.test.tsupdated for the new state shape andenqueueQuestionmutationinputAnsweredonly removes the matching entryenqueueQuestionfor the same id does not grow the queuebun test src/__tests__/useStreamReducer.test.ts→ 63 pass / 0 fail (was 60 before this PR; +3 new tests).Diff Stats
The patch is intentionally minimal — it does not include any UI/UX changes (the "Q 1 / 3" indicator is a separate concern that downstream forks can layer on top).