fix: kill OpenCode child processes when spacebot exits on Windows#495
Closed
TheDarkSkyXD wants to merge 1 commit intospacedriveapp:mainfrom
Closed
fix: kill OpenCode child processes when spacebot exits on Windows#495TheDarkSkyXD wants to merge 1 commit intospacedriveapp:mainfrom
TheDarkSkyXD wants to merge 1 commit intospacedriveapp:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdded a Windows-only Job Object setup on startup and extended the shutdown sequence to explicitly deduplicate and shut down OpenCode server-pools before per-agent disconnect and database close operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
TheDarkSkyXD
force-pushed
the
fix/cleanup-child-processes-on-exit
branch
from
ff2aaed to
cb6a6fe
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
interface/src/components/TopologyGraph.tsx (1)
1718-1731:⚠️ Potential issue | 🟠 MajorAgent avatars regress to the fallback here.
ProfileNodestill readsdata.avatarUrlon Line 163, but neither agent-node payload populates that field anymore. That means grouped and ungrouped agents both lose their custom avatar and fall back to the seed-based placeholder.♻️ Suggested fix
data: { nodeId: agentId, nodeKind: "agent", configDisplayName: topoAgent?.display_name ?? null, configRole: topoAgent?.role ?? null, chosenName: profile?.display_name ?? null, avatarSeed: profile?.avatar_seed ?? agentId, + avatarUrl: api.agentAvatarUrl(agentId), bio: profile?.bio ?? null, gradientStart: agentInfoMap.get(agentId)?.gradient_start ?? null, @@ data: { nodeId: agent.id, nodeKind: "agent", configDisplayName: agent.display_name ?? null, configRole: agent.role ?? null, chosenName: profile?.display_name ?? null, avatarSeed: profile?.avatar_seed ?? agent.id, + avatarUrl: api.agentAvatarUrl(agent.id), bio: profile?.bio ?? null, gradientStart: agentInfoMap.get(agent.id)?.gradient_start ?? null,Also applies to: 1764-1778
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/TopologyGraph.tsx` around lines 1718 - 1731, The agent node payload no longer sets avatarUrl so ProfileNode (which reads data.avatarUrl) falls back to the seed-based placeholder; update the node construction (the object where nodeId/nodeKind/... are set) to include an avatarUrl property sourced from the agent profile (e.g., profile?.avatar_url or profile?.avatarUrl) and default to null if missing, and apply the same change in the other similar block around the 1764-1778 section so grouped and ungrouped agents both preserve custom avatars.interface/src/components/WebChatPanel.tsx (1)
92-183:⚠️ Potential issue | 🟠 MajorKey the uncontrolled input by chat identity.
Because the textarea is now uncontrolled and
FloatingChatInputis rendered without akey, switching to anotheragentIdreuses the previous DOM value andhasTextstate. That lets an old draft bleed into the next agent's conversation and be sent there by mistake.Minimal fix
- <FloatingChatInput + <FloatingChatInput + key={agentId} onSubmit={handleSubmit} disabled={isSending || isTyping} agentId={agentId} />Also applies to: 286-290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/WebChatPanel.tsx` around lines 92 - 183, FloatingChatInput currently uses an uncontrolled textarea (textareaRef) and keeps internal state (hasText) but isn’t keyed by agent, so switching agentId reuses the same DOM/input value; fix by adding a React key tied to agentId so the textarea and component remount when agentId changes (e.g., add key={agentId} to the outermost wrapper or the <textarea> inside FloatingChatInput), and optionally reset hasText/textarea value in an effect that runs when agentId changes to be extra-safe; apply the same key fix to the other occurrence mentioned (lines ~286-290).
🧹 Nitpick comments (4)
interface/src/routes/AgentDetail.tsx (1)
369-369: Minimal debounce value may not provide meaningful benefit.Adding
debounce={1}toResponsiveContainerintroduces only a 1ms delay before resize recalculations. While this is a valid prop, such a small value provides negligible debouncing effect. If the intent is to reduce layout thrashing during window resizes, a value in the range of 50-200ms would be more effective. If the intent is to fix a specific initialization timing issue,debounce={1}is fine.Could you clarify the motivation for this change? If it resolves a specific bug (e.g., initial render flicker, ResizeObserver loop errors), this value makes sense. Otherwise, consider either removing it or increasing to a more impactful value.
Also applies to: 430-430, 556-556
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentDetail.tsx` at line 369, The debounce={1} on ResponsiveContainer offers negligible delay; update the three ResponsiveContainer instances in AgentDetail.tsx (the components using the ResponsiveContainer prop) to either remove the debounce prop or set it to a more effective value (e.g., debounce={50}–{200}) depending on the intent, and add a short inline comment explaining the chosen value or if it was removed so reviewers know whether this was to reduce resize thrash or to address an initialization timing quirk.src/api/system.rs (1)
260-262: Include Windows error code for debugging.When
GetDiskFreeSpaceExWfails, the generic error message provides no diagnostic value. Capturing the last OS error helps with troubleshooting.🔧 Proposed fix to include error code
if result == 0 { - return Err(anyhow::anyhow!("GetDiskFreeSpaceExW call failed")); + return Err(anyhow::anyhow!( + "GetDiskFreeSpaceExW call failed: {}", + std::io::Error::last_os_error() + )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/system.rs` around lines 260 - 262, The error branch after the GetDiskFreeSpaceExW call (the if result == 0 block in src/api/system.rs) should include the Windows OS error details; capture std::io::Error::last_os_error() and include its code/message in the returned anyhow error so the log contains the OS error (e.g., include error.raw_os_error() or the error Display) rather than the generic "GetDiskFreeSpaceExW call failed" message.src/agent/compactor.rs (1)
226-247: Same ToolResult guard logic—consider extracting to a shared helper.This forward-scan logic is duplicated across
emergency_truncate,run_compaction, andWorker::compact_historyin worker.rs. Extracting it to a reusable helper would reduce duplication and ensure consistent behavior if the logic needs future adjustment.♻️ Example helper extraction
/// Advance `remove_count` past consecutive User messages containing ToolResult blocks. /// Ensures ToolResults aren't orphaned when their corresponding ToolCalls are removed. /// Caps at `total - 2` to always retain at least 2 messages. fn advance_past_tool_results( history: &[Message], mut remove_count: usize, total: usize, ) -> usize { while remove_count < total.saturating_sub(2) { let has_tool_result = matches!( history.get(remove_count), Some(Message::User { content }) if content.iter().any(|item| matches!(item, UserContent::ToolResult(_))) ); if has_tool_result { remove_count += 1; } else { break; } } remove_count }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/compactor.rs` around lines 226 - 247, Duplicate forward-scan logic to skip User messages with ToolResult blocks appears in emergency_truncate, run_compaction, and Worker::compact_history; extract that logic into a single helper function (e.g., advance_past_tool_results) that accepts a history slice (&[Message]), current remove_count, and total and returns the advanced remove_count, then replace the duplicated while-loop in emergency_truncate, run_compaction, and Worker::compact_history with calls to this helper to ensure consistent behavior and reduce duplication.interface/src/components/ModelSelect.tsx (1)
334-336: Minor: O(n) indexOf lookup on each item render.
flatList.indexOf(model)is called for every model in the dropdown. For large model lists, this becomes O(n²). Consider building a Map from model to index once whenflatListchanges.♻️ Proposed optimization
const flatList = useMemo(() => { const items: ModelInfo[] = []; for (const p of sortedProviders) { for (const m of grouped[p]) { items.push(m); } } return items; }, [sortedProviders, grouped]); +const flatIndexMap = useMemo(() => { + const map = new Map<string, number>(); + flatList.forEach((m, i) => map.set(m.id, i)); + return map; +}, [flatList]); // Then in render: -const flatIdx = flatList.indexOf(model); +const flatIdx = flatIndexMap.get(model.id) ?? -1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/ModelSelect.tsx` around lines 334 - 336, The per-item O(n) lookup comes from calling flatList.indexOf(model) inside the render loop (grouped[prov].map) to compute flatIdx; fix by precomputing a Map (e.g., modelIndexMap) that maps each model to its index whenever flatList changes (useMemo or equivalent) and replace flatList.indexOf(model) with modelIndexMap.get(model) when computing isHighlighted against highlightIndex in ModelSelect.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/components/ConnectionScreen.tsx`:
- Around line 89-96: The effect auto-starting the sidecar should not run when
the user has configured a non-local server; modify the useEffect that currently
checks hasSidecar, autoStarted, sidecarState, state and calls handleStartLocal()
to also verify the current server URL is empty/default-local (e.g. serverUrl is
falsy or matches localhost/127.0.0.1 and default port) or require an explicit
opt-in flag; locate the variables/refs used in this block (autoStarted,
hasSidecar, sidecarState, state, handleStartLocal) and add the extra guard (or
helper isLocalServer(serverUrl)) so handleStartLocal() is only invoked for
local/default URLs.
In `@interface/src/components/ModelSelect.tsx`:
- Around line 192-202: The current handleBlur only commits the custom model ID
on blur which delays parent updates; instead, ensure onChange is called
immediately on each keystroke (where the input updates `filter` — e.g., the
input's change handler often named something like handleFilterChange or the
onChange passed to the input) so parent state (used by Settings.tsx:
`modelInput` and `setTestedSignature`) updates in real time, and retain the
existing handleBlur (with `suppressBlurCommitRef`, `filter.trim()`, `value`
comparison) to perform a final trimmed commit on blur; in short: invoke
onChange(filter) during typing and keep the trimmed onChange(trimmed) in
handleBlur to commit the final value.
In `@interface/src/components/TopologyGraph.tsx`:
- Around line 998-1012: graphKey currently fingerprints humans only by id and
display_name, so edits made via updateHuman (which triggers refetch and affects
buildGraph rendering because buildGraph uses human role, bio, and description)
won't change graphKey and nodes stay stale; update the graphKey useMemo to
include each human's role, bio, and description (e.g., extend the data.humans
mapping used in graphKey to return
`${hu.id}|${hu.display_name}|${hu.role}|${hu.bio}|${hu.description}`) so changes
to those fields will produce a new key and force rebuild of nodes in buildGraph.
In `@interface/src/routes/AgentProjects.tsx`:
- Around line 133-146: loadDir can race and apply stale api.listDir responses;
add a request-tracking token (e.g. a useRef counter or AbortController) and
capture a local id before calling api.listDir, then only call setCurrentPath,
setParentPath, setEntries, setError and setLoading if the local id matches the
latest token (or the request wasn't aborted). Update the loadDir closure to
increment the request id (or create/attach an AbortSignal) before the await, and
check that token after the await and in the catch/finally blocks so only the
latest navigation updates state.
In `@interface/src/routes/Overview.tsx`:
- Around line 53-56: The empty/loading state is keyed to the lightweight list
(agents) while the canvas renders overviewAgents, causing the graph to be blank
when overviewAgents is stale; change the logic so the UI uses the same source as
the graph—either switch all loading/empty checks to overviewAgents (e.g., use
overviewAgents.length instead of agents.length) or cache/retain the last
non-empty overviewAgents payload until the overview refresh completes (apply the
same change wherever agents vs overviewAgents are compared, e.g., the checks
around the canvas rendering and the blocks referenced in the 149-205 range).
In `@interface/src/routes/Settings.tsx`:
- Around line 593-595: The dialog is being dead-ended when
handleStartAnthropicOAuth sets an error (via setAnthropicOAuthMessage) before
the dialog state exists, which hides the model picker/button and footer
controls; fix by preserving or initializing the dialog state on errors instead
of returning early — in handleStartAnthropicOAuth, after validating
anthropicOAuthModel or catching the start request error, set the message but
also ensure the dialog state (e.g., anthropicOAuthState or a boolean that
controls the dialog render) is initialized (or reset to 'idle') so the footer
still renders Restart/Done; similarly adjust the render guards that check state
(the conditional blocks around the model picker, button and footer) so they
render the footer even when an error message exists (see uses of
anthropicOAuthModel, setAnthropicOAuthMessage, and the dialog state checks
referenced at the other ranges 3059-3092 and 3144-3153).
In `@src/api/fs.rs`:
- Around line 71-79: The loop silently drops per-entry errors; change the read
loop so errors from read_dir.next_entry().await and from entry.file_type().await
are propagated (not masked) — e.g., match the Result from
read_dir.next_entry().await and return or ? on Err, and handle file_type().await
errors instead of using unwrap_or(false) so failures produce an error response
rather than a partial DirEntry list; update the code that builds Vec<DirEntry>
(entries) to propagate those errors from read_dir and entry.file_type calls.
In `@src/api/providers.rs`:
- Around line 1314-1327: The code currently accepts any non-empty request.model
and persists it into AnthropicOAuthSession; before creating or storing the
session (where AnthropicOAuthSession is constructed/used) you must validate that
request.model starts with the "anthropic/" prefix and reject otherwise. Update
the code path around the model handling (the request.model trim/to_string and
the place where AnthropicOAuthSession is created) to check
request.model.starts_with("anthropic/") and return an
Err/Ok(Json(AnthropicOAuthStartResponse { success: false, message: "Model must
be an anthropic/* model", ... })) when it does not, so only models with the
"anthropic/" prefix are ever persisted or routed. Ensure the same validation is
applied before any persistence or routing that uses AnthropicOAuthSession or
later routing logic.
- Around line 422-423: The API-key vs OAuth flags are conflated: keep them
separate by making the "configured" boolean for API-key providers check the
API-key presence (not crate::auth::credentials_path) while leaving
anthropic_oauth_configured and openai_oauth_configured to use
crate::auth::credentials_path(&instance_dir). Specifically, change the variables
that represent "anthropic configured" and "openai configured" so they test the
provider's API-key existence (use the existing API-key check/function for
Anthropic/OpenAI instead of credentials_path), keep anthropic_oauth_configured
and openai_oauth_configured as-is, and apply the same correction at the other
occurrences mentioned (the blocks around the other two pairs). Ensure you
reference and update the variables named anthropic_oauth_configured,
openai_oauth_configured and the corresponding anthropic/openai "configured"
variables so the Remove action targets the API-key state only.
- Around line 1418-1431: The code currently uses unwrap_or_default() to turn
read failures into an empty config and then parses/writes it, risking clobbering
the user's config; change the flow so reading, parsing into
toml_edit::DocumentMut, and writing are all part of the success path: explicitly
handle the Result from tokio::fs::read_to_string(&config_path) (do not use
unwrap_or_default()), return or propagate an error (or log and return an error
response) if reading fails, attempt parse only on the real content and handle
parse errors (do not silently ignore), call apply_model_routing(&mut doc,
&session.model) only after successful parse, and only then write back with
tokio::fs::write(&config_path, doc.to_string()).await while logging and
returning failures instead of proceeding on error.
In `@src/daemon.rs`:
- Around line 257-320: start_ipc_server currently exposes unauthenticated TCP
control on loopback; fix by adding an instance-specific secret handshake or
switching to a Windows IPC transport with OS ACLs before dispatching commands.
Generate a cryptographically-random token in start_ipc_server, persist it with
restrictive perms (e.g., port_file.with_extension("token")) and remove it on
shutdown, then require the token to be presented and validated by
handle_ipc_connection before honoring sensitive commands (shutdown/status);
alternatively replace the TcpListener usage in start_ipc_server with a
Windows-named-pipe server that enforces OS-level access control and update the
corresponding connection handling code to validate the pipe security context.
Ensure the token is not logged and is cleaned up with the port file.
- Around line 249-252: Replace the silent discards of Result/Errors in the
cleanup tasks with explicit handling and logging: capture the Result from
cleanup_rx.wait_for(|shutdown| *shutdown).await and from
std::fs::remove_file(&cleanup_socket), log any Err values (including context
like "watcher wait failed" and "failed to remove cleanup socket"), but treat
Err(kind == io::ErrorKind::NotFound) as non-fatal and suppress only that case;
apply the same pattern to the other cleanup task and to the cleanup_stale_files
helper so that failures are logged rather than hidden (refer to the async block
spawned with tokio::spawn, cleanup_rx.wait_for, std::fs::remove_file, and the
cleanup_stale_files function).
- Around line 225-245: The IPC accept loop spawned in the tokio task never
observes shutdown changes, leaving the JoinHandle pending and the TCP listener
active; fix by cloning a watch::Receiver (shutdown_rx) into the async accept
loop (the task that calls listener.accept()) and use a select! (or
tokio::select!) to await either listener.accept() or shutdown_rx.changed() so
the loop breaks when the watch signals shutdown; ensure the per-connection spawn
still receives shutdown_tx for handle_ipc_connection but the outer accept loop
exits cleanly when shutdown_rx changes (also apply the same pattern to the
similar loop around handle_ipc_connection at symbols referenced near lines
291-311).
---
Outside diff comments:
In `@interface/src/components/TopologyGraph.tsx`:
- Around line 1718-1731: The agent node payload no longer sets avatarUrl so
ProfileNode (which reads data.avatarUrl) falls back to the seed-based
placeholder; update the node construction (the object where nodeId/nodeKind/...
are set) to include an avatarUrl property sourced from the agent profile (e.g.,
profile?.avatar_url or profile?.avatarUrl) and default to null if missing, and
apply the same change in the other similar block around the 1764-1778 section so
grouped and ungrouped agents both preserve custom avatars.
In `@interface/src/components/WebChatPanel.tsx`:
- Around line 92-183: FloatingChatInput currently uses an uncontrolled textarea
(textareaRef) and keeps internal state (hasText) but isn’t keyed by agent, so
switching agentId reuses the same DOM/input value; fix by adding a React key
tied to agentId so the textarea and component remount when agentId changes
(e.g., add key={agentId} to the outermost wrapper or the <textarea> inside
FloatingChatInput), and optionally reset hasText/textarea value in an effect
that runs when agentId changes to be extra-safe; apply the same key fix to the
other occurrence mentioned (lines ~286-290).
---
Nitpick comments:
In `@interface/src/components/ModelSelect.tsx`:
- Around line 334-336: The per-item O(n) lookup comes from calling
flatList.indexOf(model) inside the render loop (grouped[prov].map) to compute
flatIdx; fix by precomputing a Map (e.g., modelIndexMap) that maps each model to
its index whenever flatList changes (useMemo or equivalent) and replace
flatList.indexOf(model) with modelIndexMap.get(model) when computing
isHighlighted against highlightIndex in ModelSelect.tsx.
In `@interface/src/routes/AgentDetail.tsx`:
- Line 369: The debounce={1} on ResponsiveContainer offers negligible delay;
update the three ResponsiveContainer instances in AgentDetail.tsx (the
components using the ResponsiveContainer prop) to either remove the debounce
prop or set it to a more effective value (e.g., debounce={50}–{200}) depending
on the intent, and add a short inline comment explaining the chosen value or if
it was removed so reviewers know whether this was to reduce resize thrash or to
address an initialization timing quirk.
In `@src/agent/compactor.rs`:
- Around line 226-247: Duplicate forward-scan logic to skip User messages with
ToolResult blocks appears in emergency_truncate, run_compaction, and
Worker::compact_history; extract that logic into a single helper function (e.g.,
advance_past_tool_results) that accepts a history slice (&[Message]), current
remove_count, and total and returns the advanced remove_count, then replace the
duplicated while-loop in emergency_truncate, run_compaction, and
Worker::compact_history with calls to this helper to ensure consistent behavior
and reduce duplication.
In `@src/api/system.rs`:
- Around line 260-262: The error branch after the GetDiskFreeSpaceExW call (the
if result == 0 block in src/api/system.rs) should include the Windows OS error
details; capture std::io::Error::last_os_error() and include its code/message in
the returned anyhow error so the log contains the OS error (e.g., include
error.raw_os_error() or the error Display) rather than the generic
"GetDiskFreeSpaceExW call failed" message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00b3f984-4400-41ad-8969-f92b8cbee6b2
⛔ Files ignored due to path filters (59)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.tomldesktop/src-tauri/Cargo.lockis excluded by!**/*.lock,!**/*.lockdesktop/src-tauri/Cargo.tomlis excluded by!**/*.tomldesktop/src-tauri/capabilities/default.jsonis excluded by!**/*.jsondesktop/src-tauri/gen/schemas/acl-manifests.jsonis excluded by!**/gen/**,!**/*.json,!**/gen/**desktop/src-tauri/gen/schemas/capabilities.jsonis excluded by!**/gen/**,!**/*.json,!**/gen/**desktop/src-tauri/gen/schemas/desktop-schema.jsonis excluded by!**/gen/**,!**/*.json,!**/gen/**desktop/src-tauri/gen/schemas/windows-schema.jsonis excluded by!**/gen/**,!**/*.json,!**/gen/**desktop/src-tauri/icons/64x64.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square107x107Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square142x142Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square150x150Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square284x284Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square30x30Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square310x310Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square44x44Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square71x71Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square89x89Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/StoreLogo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-anydpi-v26/ic_launcher.xmlis excluded by!**/*.xmldesktop/src-tauri/icons/android/mipmap-hdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-hdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-hdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-mdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-mdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-mdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xhdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xhdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xhdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxhdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxhdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxhdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxxhdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxxhdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxxhdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/values/ic_launcher_background.xmlis excluded by!**/*.xmldesktop/src-tauri/icons/icon.icois excluded by!**/*.ico,!**/*.icodesktop/src-tauri/icons/icon.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-20x20@1x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-20x20@2x-1.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-20x20@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-20x20@3x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-29x29@1x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-29x29@2x-1.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-29x29@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-29x29@3x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-40x40@1x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-40x40@2x-1.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-40x40@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-40x40@3x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-512@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-60x60@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-60x60@3x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-76x76@1x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-76x76@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-83.5x83.5@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/tauri.windows.conf.jsonis excluded by!**/*.jsoninterface/package.jsonis excluded by!**/*.json
📒 Files selected for processing (29)
desktop/src-tauri/src/main.rsinterface/src/api/client.tsinterface/src/components/ConnectionScreen.tsxinterface/src/components/CortexChatPanel.tsxinterface/src/components/ModelSelect.tsxinterface/src/components/TopBar.tsxinterface/src/components/TopologyGraph.tsxinterface/src/components/WebChatPanel.tsxinterface/src/routes/AgentDetail.tsxinterface/src/routes/AgentProjects.tsxinterface/src/routes/Overview.tsxinterface/src/routes/Settings.tsxscripts/bundle-sidecar.ps1src/agent/compactor.rssrc/agent/worker.rssrc/api.rssrc/api/fs.rssrc/api/models.rssrc/api/providers.rssrc/api/server.rssrc/api/ssh.rssrc/api/system.rssrc/config/types.rssrc/daemon.rssrc/llm/anthropic/params.rssrc/llm/manager.rssrc/llm/routing.rssrc/main.rssrc/tools/browser.rs
Comment on lines
+89
to
+96
| // Auto-start the sidecar when running in Tauri with a bundled binary | ||
| const autoStarted = useRef(false); | ||
| useEffect(() => { | ||
| if (hasSidecar && !autoStarted.current && sidecarState === "idle" && state === "disconnected") { | ||
| autoStarted.current = true; | ||
| handleStartLocal(); | ||
| } | ||
| }, [hasSidecar, sidecarState, state, handleStartLocal]); |
Contributor
There was a problem hiding this comment.
Don't auto-start over a non-local server URL.
This runs for any disconnected bundled build, so a user who intentionally pointed serverUrl at a remote Spacebot will get switched to localhost on mount as soon as the sidecar becomes ready. Gate the auto-start behind an empty/default-local URL (or an explicit opt-in) before calling handleStartLocal().
Suggested guard
const autoStarted = useRef(false);
useEffect(() => {
- if (hasSidecar && !autoStarted.current && sidecarState === "idle" && state === "disconnected") {
+ const shouldAutoStart =
+ serverUrl.trim() === "" || serverUrl === "http://localhost:19898";
+ if (
+ hasSidecar &&
+ shouldAutoStart &&
+ !autoStarted.current &&
+ sidecarState === "idle" &&
+ state === "disconnected"
+ ) {
autoStarted.current = true;
handleStartLocal();
}
- }, [hasSidecar, sidecarState, state, handleStartLocal]);
+ }, [hasSidecar, sidecarState, state, serverUrl, handleStartLocal]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/components/ConnectionScreen.tsx` around lines 89 - 96, The
effect auto-starting the sidecar should not run when the user has configured a
non-local server; modify the useEffect that currently checks hasSidecar,
autoStarted, sidecarState, state and calls handleStartLocal() to also verify the
current server URL is empty/default-local (e.g. serverUrl is falsy or matches
localhost/127.0.0.1 and default port) or require an explicit opt-in flag; locate
the variables/refs used in this block (autoStarted, hasSidecar, sidecarState,
state, handleStartLocal) and add the extra guard (or helper
isLocalServer(serverUrl)) so handleStartLocal() is only invoked for
local/default URLs.
Comment on lines
192
to
202
| const handleBlur = () => { | ||
| if (suppressBlurCommitRef.current) { | ||
| suppressBlurCommitRef.current = false; | ||
| return; | ||
| } | ||
| // Commit typed custom model ID when input loses focus (e.g. clicking Save/Test) | ||
| const trimmed = filter.trim(); | ||
| if (trimmed && trimmed !== value) { | ||
| onChange(trimmed); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Blur-only commit may delay parent component state updates.
The new handleBlur commits the typed custom model ID only when the input loses focus. Per the relevant code snippets in Settings.tsx, call sites use onChange to update state for button disable logic (e.g., line 889: disabled={!modelInput.trim()}) and to invalidate tested signatures (line 882: setTestedSignature(null)).
With this change, while typing a custom model ID:
- The "Open authorization page" button remains disabled until blur
- The "Verified" badge doesn't invalidate until blur
- Real-time validation feedback is lost
If this is intentional for performance, consider documenting the behavior. Otherwise, the intermediate typing should still call onChange for responsiveness.
🔧 Alternative: call onChange on each keystroke while still committing on blur
const handleInputChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const val = e.target.value;
setFilter(val);
setHighlightIndex(-1);
if (!open) setOpen(true);
+ // Notify parent of intermediate value for real-time UI updates
+ onChange(val);
};
const handleBlur = () => {
if (suppressBlurCommitRef.current) {
suppressBlurCommitRef.current = false;
return;
}
- // Commit typed custom model ID when input loses focus (e.g. clicking Save/Test)
- const trimmed = filter.trim();
- if (trimmed && trimmed !== value) {
- onChange(trimmed);
- }
+ // Parent already has the value from handleInputChange; no additional commit needed
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/components/ModelSelect.tsx` around lines 192 - 202, The current
handleBlur only commits the custom model ID on blur which delays parent updates;
instead, ensure onChange is called immediately on each keystroke (where the
input updates `filter` — e.g., the input's change handler often named something
like handleFilterChange or the onChange passed to the input) so parent state
(used by Settings.tsx: `modelInput` and `setTestedSignature`) updates in real
time, and retain the existing handleBlur (with `suppressBlurCommitRef`,
`filter.trim()`, `value` comparison) to perform a final trimmed commit on blur;
in short: invoke onChange(filter) during typing and keep the trimmed
onChange(trimmed) in handleBlur to commit the final value.
Comment on lines
+998
to
+1012
| const graphKey = useMemo(() => { | ||
| if (!data) return ""; | ||
| const now = Date.now(); | ||
| const threshold = now - 5 * 60 * 1000; | ||
| const a = data.agents.map((ag) => { | ||
| const s = agentProfiles.get(ag.id); | ||
| const inf = agentInfoMap.get(ag.id); | ||
| const online = s?.last_activity_at != null && new Date(s.last_activity_at).getTime() > threshold; | ||
| return `${ag.id}|${ag.display_name}|${ag.role}|${s?.profile?.display_name}|${s?.profile?.bio}|${s?.profile?.avatar_seed}|${inf?.gradient_start}|${inf?.gradient_end}|${online}|${s?.channel_count}|${s?.memory_total}`; | ||
| }).join("\n"); | ||
| const l = data.links.map((lk) => `${lk.from}-${lk.to}-${lk.direction}-${lk.kind}`).join(","); | ||
| const g = data.groups.map((gr) => `${gr.name}:${gr.agent_ids.join("+")}:${gr.color ?? ""}`).join(","); | ||
| const h = data.humans.map((hu) => `${hu.id}|${hu.display_name}`).join(","); | ||
| return `${a}||${l}||${g}||${h}`; | ||
| }, [data, agentProfiles, agentInfoMap]); |
Contributor
There was a problem hiding this comment.
graphKey is missing rendered human fields.
buildGraph() uses human role, bio, and description, but Line 1010 fingerprints humans with only id and display_name. After updateHuman refetches topology, edits to those fields will not change graphKey, so the existing nodes state keeps stale card content until some unrelated graph change forces a rebuild.
♻️ Suggested fix
- const h = data.humans.map((hu) => `${hu.id}|${hu.display_name}`).join(",");
+ const h = data.humans
+ .map(
+ (hu) =>
+ `${hu.id}|${hu.display_name ?? ""}|${hu.role ?? ""}|${hu.bio ?? ""}|${hu.description ?? ""}`,
+ )
+ .join(",");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const graphKey = useMemo(() => { | |
| if (!data) return ""; | |
| const now = Date.now(); | |
| const threshold = now - 5 * 60 * 1000; | |
| const a = data.agents.map((ag) => { | |
| const s = agentProfiles.get(ag.id); | |
| const inf = agentInfoMap.get(ag.id); | |
| const online = s?.last_activity_at != null && new Date(s.last_activity_at).getTime() > threshold; | |
| return `${ag.id}|${ag.display_name}|${ag.role}|${s?.profile?.display_name}|${s?.profile?.bio}|${s?.profile?.avatar_seed}|${inf?.gradient_start}|${inf?.gradient_end}|${online}|${s?.channel_count}|${s?.memory_total}`; | |
| }).join("\n"); | |
| const l = data.links.map((lk) => `${lk.from}-${lk.to}-${lk.direction}-${lk.kind}`).join(","); | |
| const g = data.groups.map((gr) => `${gr.name}:${gr.agent_ids.join("+")}:${gr.color ?? ""}`).join(","); | |
| const h = data.humans.map((hu) => `${hu.id}|${hu.display_name}`).join(","); | |
| return `${a}||${l}||${g}||${h}`; | |
| }, [data, agentProfiles, agentInfoMap]); | |
| const graphKey = useMemo(() => { | |
| if (!data) return ""; | |
| const now = Date.now(); | |
| const threshold = now - 5 * 60 * 1000; | |
| const a = data.agents.map((ag) => { | |
| const s = agentProfiles.get(ag.id); | |
| const inf = agentInfoMap.get(ag.id); | |
| const online = s?.last_activity_at != null && new Date(s.last_activity_at).getTime() > threshold; | |
| return `${ag.id}|${ag.display_name}|${ag.role}|${s?.profile?.display_name}|${s?.profile?.bio}|${s?.profile?.avatar_seed}|${inf?.gradient_start}|${inf?.gradient_end}|${online}|${s?.channel_count}|${s?.memory_total}`; | |
| }).join("\n"); | |
| const l = data.links.map((lk) => `${lk.from}-${lk.to}-${lk.direction}-${lk.kind}`).join(","); | |
| const g = data.groups.map((gr) => `${gr.name}:${gr.agent_ids.join("+")}:${gr.color ?? ""}`).join(","); | |
| const h = data.humans | |
| .map( | |
| (hu) => | |
| `${hu.id}|${hu.display_name ?? ""}|${hu.role ?? ""}|${hu.bio ?? ""}|${hu.description ?? ""}`, | |
| ) | |
| .join(","); | |
| return `${a}||${l}||${g}||${h}`; | |
| }, [data, agentProfiles, agentInfoMap]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/components/TopologyGraph.tsx` around lines 998 - 1012, graphKey
currently fingerprints humans only by id and display_name, so edits made via
updateHuman (which triggers refetch and affects buildGraph rendering because
buildGraph uses human role, bio, and description) won't change graphKey and
nodes stay stale; update the graphKey useMemo to include each human's role, bio,
and description (e.g., extend the data.humans mapping used in graphKey to return
`${hu.id}|${hu.display_name}|${hu.role}|${hu.bio}|${hu.description}`) so changes
to those fields will produce a new key and force rebuild of nodes in buildGraph.
Comment on lines
+133
to
+146
| const loadDir = useCallback(async (path?: string) => { | ||
| setLoading(true); | ||
| setError(null); | ||
| try { | ||
| const result = await api.listDir(path); | ||
| setCurrentPath(result.path); | ||
| setParentPath(result.parent); | ||
| setEntries(result.entries.filter((e) => e.is_dir)); | ||
| } catch (e) { | ||
| setError(e instanceof Error ? e.message : "Failed to load directory"); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, []); |
Contributor
There was a problem hiding this comment.
Ignore stale listDir responses.
loadDir can race with itself; if a slower request resolves after the user has already navigated elsewhere, it will overwrite the newer currentPath/entries state and clear the spinner early. Track a request id or abort signal so only the latest navigation updates state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/routes/AgentProjects.tsx` around lines 133 - 146, loadDir can
race and apply stale api.listDir responses; add a request-tracking token (e.g. a
useRef counter or AbortController) and capture a local id before calling
api.listDir, then only call setCurrentPath, setParentPath, setEntries, setError
and setLoading if the local id matches the latest token (or the request wasn't
aborted). Update the loadDir closure to increment the request id (or
create/attach an AbortSignal) before the await, and check that token after the
await and in the catch/finally blocks so only the latest navigation updates
state.
interface/src/routes/Overview.tsx
Outdated
Comment on lines
+53
to
+56
| // Use the lightweight agents list for the header count (loads instantly), | ||
| // and the heavy overview agents only for the topology graph. | ||
| const agents = agentsData?.agents ?? []; | ||
| const overviewAgents = overviewData?.agents ?? []; |
Contributor
There was a problem hiding this comment.
Keep the empty/loading state keyed to the same data source as the graph.
agents.length now decides when to leave the empty/loading UI, but the canvas still renders overviewAgents. After a create/delete or slow refresh, the fast query can say "there are agents" while overview is still empty or stale, leaving the graph blank until the next 60s overview poll. Use the same source for both decisions, or keep the last overview payload until the refresh lands.
Also applies to: 149-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/routes/Overview.tsx` around lines 53 - 56, The empty/loading
state is keyed to the lightweight list (agents) while the canvas renders
overviewAgents, causing the graph to be blank when overviewAgents is stale;
change the logic so the UI uses the same source as the graph—either switch all
loading/empty checks to overviewAgents (e.g., use overviewAgents.length instead
of agents.length) or cache/retain the last non-empty overviewAgents payload
until the overview refresh completes (apply the same change wherever agents vs
overviewAgents are compared, e.g., the checks around the canvas rendering and
the blocks referenced in the 149-205 range).
src/api/providers.rs
Outdated
Comment on lines
+1314
to
+1327
| let model = request.model.trim().to_string(); | ||
| if model.is_empty() { | ||
| return Ok(Json(AnthropicOAuthStartResponse { | ||
| success: false, | ||
| message: "Model cannot be empty".to_string(), | ||
| authorize_url: None, | ||
| state: None, | ||
| })); | ||
| } | ||
|
|
||
| let mode = match request.mode.as_deref() { | ||
| Some("console") => crate::auth::AuthMode::Console, | ||
| _ => crate::auth::AuthMode::Max, | ||
| }; |
Contributor
There was a problem hiding this comment.
Reject non-Anthropic models before storing the OAuth session.
request.model is persisted into AnthropicOAuthSession and later written into routing, but this path never enforces the anthropic/... prefix that the API-key flows require. A buggy or crafted client can complete Anthropic OAuth and still rewrite defaults to a non-Anthropic model.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/providers.rs` around lines 1314 - 1327, The code currently accepts
any non-empty request.model and persists it into AnthropicOAuthSession; before
creating or storing the session (where AnthropicOAuthSession is
constructed/used) you must validate that request.model starts with the
"anthropic/" prefix and reject otherwise. Update the code path around the model
handling (the request.model trim/to_string and the place where
AnthropicOAuthSession is created) to check
request.model.starts_with("anthropic/") and return an
Err/Ok(Json(AnthropicOAuthStartResponse { success: false, message: "Model must
be an anthropic/* model", ... })) when it does not, so only models with the
"anthropic/" prefix are ever persisted or routed. Ensure the same validation is
applied before any persistence or routing that uses AnthropicOAuthSession or
later routing logic.
src/api/providers.rs
Outdated
Comment on lines
+1418
to
+1431
| let content = if config_path.exists() { | ||
| tokio::fs::read_to_string(&config_path) | ||
| .await | ||
| .unwrap_or_default() | ||
| } else { | ||
| String::new() | ||
| }; | ||
|
|
||
| if let Ok(mut doc) = content.parse::<toml_edit::DocumentMut>() { | ||
| apply_model_routing(&mut doc, &session.model); | ||
| if let Err(error) = tokio::fs::write(&config_path, doc.to_string()).await { | ||
| tracing::warn!(%error, "failed to write config.toml after Anthropic OAuth"); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Don't rewrite config.toml from an empty fallback.
If the existing file can't be read, unwrap_or_default() turns that into an empty document, and the subsequent write can replace the user's whole config with only the routing block. Parse/write failures are also logged and ignored, yet the handler still returns success and claims routing was updated. Make config read/parse/write part of the success path instead of continuing with defaults.
As per coding guidelines: "Don't silently discard errors. No let _ = on Results. Handle them, log them, or propagate them."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/providers.rs` around lines 1418 - 1431, The code currently uses
unwrap_or_default() to turn read failures into an empty config and then
parses/writes it, risking clobbering the user's config; change the flow so
reading, parsing into toml_edit::DocumentMut, and writing are all part of the
success path: explicitly handle the Result from
tokio::fs::read_to_string(&config_path) (do not use unwrap_or_default()), return
or propagate an error (or log and return an error response) if reading fails,
attempt parse only on the real content and handle parse errors (do not silently
ignore), call apply_model_routing(&mut doc, &session.model) only after
successful parse, and only then write back with tokio::fs::write(&config_path,
doc.to_string()).await while logging and returning failures instead of
proceeding on error.
src/daemon.rs
Outdated
Comment on lines
+225
to
+245
| let handle = tokio::spawn(async move { | ||
| loop { | ||
| match listener.accept().await { | ||
| Ok((stream, _address)) => { | ||
| let shutdown_tx = shutdown_tx.clone(); | ||
| let uptime = start_time.elapsed(); | ||
| tokio::spawn(async move { | ||
| let (reader, writer) = stream.into_split(); | ||
| if let Err(error) = | ||
| handle_ipc_connection(reader, writer, &shutdown_tx, uptime).await | ||
| { | ||
| tracing::warn!(%error, "IPC connection handler failed"); | ||
| } | ||
| }); | ||
| } | ||
| Err(error) => { | ||
| tracing::warn!(%error, "failed to accept IPC connection"); | ||
| } | ||
| } | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Stop the IPC listener when shutdown is signaled.
Shutdown only updates the watch channel; these loops never read it. That leaves the returned JoinHandle pending, and the Windows TCP listener can keep accepting commands until process exit. Use a cloned shutdown_rx inside the accept loop and break once it changes.
🧩 Proposed fix
+ let mut listener_shutdown_rx = shutdown_rx.clone();
let handle = tokio::spawn(async move {
loop {
- match listener.accept().await {
- Ok((stream, _address)) => {
- let shutdown_tx = shutdown_tx.clone();
- let uptime = start_time.elapsed();
- tokio::spawn(async move {
- let (reader, writer) = stream.into_split();
- if let Err(error) =
- handle_ipc_connection(reader, writer, &shutdown_tx, uptime).await
- {
- tracing::warn!(%error, "IPC connection handler failed");
- }
- });
- }
- Err(error) => {
- tracing::warn!(%error, "failed to accept IPC connection");
- }
+ tokio::select! {
+ changed = listener_shutdown_rx.changed() => {
+ if changed.is_err() || *listener_shutdown_rx.borrow() {
+ break;
+ }
+ }
+ accept_result = listener.accept() => match accept_result {
+ Ok((stream, _address)) => {
+ let shutdown_tx = shutdown_tx.clone();
+ let uptime = start_time.elapsed();
+ tokio::spawn(async move {
+ let (reader, writer) = stream.into_split();
+ if let Err(error) =
+ handle_ipc_connection(reader, writer, &shutdown_tx, uptime).await
+ {
+ tracing::warn!(%error, "IPC connection handler failed");
+ }
+ });
+ }
+ Err(error) => {
+ tracing::warn!(%error, "failed to accept IPC connection");
+ }
+ }
}
}
});Also applies to: 291-311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/daemon.rs` around lines 225 - 245, The IPC accept loop spawned in the
tokio task never observes shutdown changes, leaving the JoinHandle pending and
the TCP listener active; fix by cloning a watch::Receiver (shutdown_rx) into the
async accept loop (the task that calls listener.accept()) and use a select! (or
tokio::select!) to await either listener.accept() or shutdown_rx.changed() so
the loop breaks when the watch signals shutdown; ensure the per-connection spawn
still receives shutdown_tx for handle_ipc_connection but the outer accept loop
exits cleanly when shutdown_rx changes (also apply the same pattern to the
similar loop around handle_ipc_connection at symbols referenced near lines
291-311).
src/daemon.rs
Outdated
Comment on lines
+249
to
+252
| tokio::spawn(async move { | ||
| let _ = cleanup_rx.wait_for(|shutdown| *shutdown).await; | ||
| let _ = std::fs::remove_file(&cleanup_socket); | ||
| }); |
Contributor
There was a problem hiding this comment.
Handle cleanup failures instead of discarding them.
These let _ = sites hide watcher failures and file-removal errors, which makes stale PID/socket/port state much harder to diagnose on the next launch. Log them and only suppress NotFound explicitly; apply the same pattern in both cleanup tasks and cleanup_stale_files.
🧹 Proposed fix
tokio::spawn(async move {
- let _ = cleanup_rx.wait_for(|shutdown| *shutdown).await;
- let _ = std::fs::remove_file(&port_file_cleanup);
+ if let Err(error) = cleanup_rx.wait_for(|shutdown| *shutdown).await {
+ tracing::warn!(%error, "IPC cleanup watcher closed before shutdown");
+ return;
+ }
+ if let Err(error) = std::fs::remove_file(&port_file_cleanup)
+ && error.kind() != std::io::ErrorKind::NotFound
+ {
+ tracing::warn!(%error, path = %port_file_cleanup.display(), "failed to remove port file");
+ }
});Also applies to: 313-317, 634-638
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/daemon.rs` around lines 249 - 252, Replace the silent discards of
Result/Errors in the cleanup tasks with explicit handling and logging: capture
the Result from cleanup_rx.wait_for(|shutdown| *shutdown).await and from
std::fs::remove_file(&cleanup_socket), log any Err values (including context
like "watcher wait failed" and "failed to remove cleanup socket"), but treat
Err(kind == io::ErrorKind::NotFound) as non-fatal and suppress only that case;
apply the same pattern to the other cleanup task and to the cleanup_stale_files
helper so that failures are logged rather than hidden (refer to the async block
spawned with tokio::spawn, cleanup_rx.wait_for, std::fs::remove_file, and the
cleanup_stale_files function).
src/daemon.rs
Outdated
Comment on lines
+257
to
+320
| #[cfg(windows)] | ||
| pub async fn start_ipc_server( | ||
| paths: &DaemonPaths, | ||
| ) -> anyhow::Result<(watch::Receiver<bool>, tokio::task::JoinHandle<()>)> { | ||
| use tokio::net::TcpListener; | ||
|
|
||
| if let Some(parent) = paths.socket.parent() { | ||
| std::fs::create_dir_all(parent).with_context(|| { | ||
| format!("failed to create instance directory: {}", parent.display()) | ||
| })?; | ||
| } | ||
|
|
||
| // Bind to an ephemeral port on localhost | ||
| let listener = TcpListener::bind("127.0.0.1:0") | ||
| .await | ||
| .context("failed to bind IPC TCP listener")?; | ||
| let port = listener | ||
| .local_addr() | ||
| .context("failed to get local address")? | ||
| .port(); | ||
|
|
||
| // Write the port so clients can find us | ||
| let port_file = paths.socket.with_extension("port"); | ||
| std::fs::write(&port_file, port.to_string()) | ||
| .with_context(|| format!("failed to write port file: {}", port_file.display()))?; | ||
|
|
||
| // Also write a PID file on Windows since daemonize doesn't do it for us | ||
| std::fs::write(&paths.pid_file, std::process::id().to_string()) | ||
| .with_context(|| format!("failed to write PID file: {}", paths.pid_file.display()))?; | ||
|
|
||
| let (shutdown_tx, shutdown_rx) = watch::channel(false); | ||
| let start_time = Instant::now(); | ||
| let port_file_cleanup = port_file.clone(); | ||
|
|
||
| let handle = tokio::spawn(async move { | ||
| loop { | ||
| match listener.accept().await { | ||
| Ok((stream, _address)) => { | ||
| let shutdown_tx = shutdown_tx.clone(); | ||
| let uptime = start_time.elapsed(); | ||
| tokio::spawn(async move { | ||
| let (reader, writer) = stream.into_split(); | ||
| if let Err(error) = | ||
| handle_ipc_connection(reader, writer, &shutdown_tx, uptime).await | ||
| { | ||
| tracing::warn!(%error, "IPC connection handler failed"); | ||
| } | ||
| }); | ||
| } | ||
| Err(error) => { | ||
| tracing::warn!(%error, "failed to accept IPC connection"); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| let mut cleanup_rx = shutdown_rx.clone(); | ||
| tokio::spawn(async move { | ||
| let _ = cleanup_rx.wait_for(|shutdown| *shutdown).await; | ||
| let _ = std::fs::remove_file(&port_file_cleanup); | ||
| }); | ||
|
|
||
| Ok((shutdown_rx, handle)) | ||
| } |
Contributor
There was a problem hiding this comment.
Don't expose unauthenticated daemon control over loopback TCP.
On Windows, any local process that finds the ephemeral port can call shutdown or status. The Unix socket path had filesystem permissions as a boundary; this TCP version does not. Require a per-instance secret/handshake or move to a Windows IPC transport with OS-level access control before dispatching commands.
Also applies to: 336-348, 354-389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/daemon.rs` around lines 257 - 320, start_ipc_server currently exposes
unauthenticated TCP control on loopback; fix by adding an instance-specific
secret handshake or switching to a Windows IPC transport with OS ACLs before
dispatching commands. Generate a cryptographically-random token in
start_ipc_server, persist it with restrictive perms (e.g.,
port_file.with_extension("token")) and remove it on shutdown, then require the
token to be presented and validated by handle_ipc_connection before honoring
sensitive commands (shutdown/status); alternatively replace the TcpListener
usage in start_ipc_server with a Windows-named-pipe server that enforces
OS-level access control and update the corresponding connection handling code to
validate the pipe security context. Ensure the token is not logged and is
cleaned up with the port file.
OpenCode server processes spawned for coding agents were left running after spacebot exited because: (1) shutdown_all() was never called on the OpenCode server pools during graceful shutdown, (2) std::process::exit(0) bypasses destructors so kill_on_drop never fires, and (3) hard kills (e.g. Tauri closing the sidecar) orphan children entirely. This adds explicit pool shutdown during graceful teardown and creates a Windows Job Object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE at startup so the OS kills all children when the process exits by any means. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TheDarkSkyXD
force-pushed
the
fix/cleanup-child-processes-on-exit
branch
from
cb6a6fe to
0d0cf9f
Compare
Contributor
Author
|
Closing in favor of a clean PR. |
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
1 participant
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
shutdown_all()on all OpenCode server pools during graceful shutdown, before agents are torn down. Deduplicates pools by pointer to avoid double-shutdown.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSEat process startup so the OS automatically terminates all child processes when spacebot exits — regardless of how it exits (graceful,std::process::exit, hard kill from Tauri, Task Manager, etc.).jobapi2andwinntfeatures to thewinapidependency for the Job Object APIs.Why
OpenCode server processes (Claude Code CLI) spawned for coding agents were left running after spacebot exited because:
shutdown_all()was never called on the OpenCode server pools during the graceful shutdown sequencestd::process::exit(0)at the end of shutdown bypasses destructors, sokill_on_drop(true)on theChildhandles never firesTest plan
opencodeprocesses remain in Task Managerspacebot stop) also cleans up OpenCode processes#[cfg(windows)]only, pool shutdown is cross-platform)🤖 Generated with Claude Code