Skip to content

fix: profile switch deadlock + default profile in list#799

Open
amiable-dev wants to merge 17 commits intomainfrom
fix/profile-switch-timeout-default-list
Open

fix: profile switch deadlock + default profile in list#799
amiable-dev wants to merge 17 commits intomainfrom
fix/profile-switch-timeout-default-list

Conversation

@amiable-dev
Copy link
Copy Markdown
Owner

@amiable-dev amiable-dev commented Apr 1, 2026

Problems

1. Profile switch deadlocked (not a timeout issue)

conductor_switch_profile via LLM chat always timed out despite the daemon successfully switching the profile. Root cause: deadlock in the command channel.

Deadlock sequence:

  1. IPC ExecuteMcpTool arrives → handled inside engine manager's command_rx select arm
  2. handle_ipc_request calls ToolExecutor.execute("conductor_switch_profile")
  3. ToolExecutor sends DaemonCommand::ProfileSwitch to command_tx and awaits result_rx
  4. Engine manager can't read command_rx — it's blocked awaiting the ToolExecutor (step 2)
  5. Timeout fires, engine manager finally processes the queued command, but IPC pipe is broken

Fix: Intercept conductor_switch_profile in handle_ipc_request BEFORE it reaches the ToolExecutor. Call execute_profile_switch() directly — no command channel round-trip needed since we're already in the engine manager context.

2. Default profile missing from list

conductor_list_profiles only returned explicitly registered profiles. The implicit default profile (main config.toml) was invisible.

Fix: Always include a synthetic "Default" entry when no registered profile has is_default: true.

Changes

  • engine_manager.rs: Add execute_profile_switch() method; intercept in handle_ipc_request
  • chat.js: Default profile entry in conductor_list_profiles; graceful profileStore fallback
  • chat.test.ts: TDD test for default profile presence
  • executor.rs / commands.rs: Timeouts at original values (10s/15s) — deadlock fix makes them irrelevant

Test plan

  • 154 chat store tests pass (1 new: default profile in list)
  • cargo clippy --all-targets --all-features passes
  • Profile switch completes without timeout via LLM chat
  • conductor_list_profiles always shows Default profile

🤖 Generated with Claude Code

amiable-dev and others added 4 commits April 1, 2026 18:45
When conductor_create_profile/delete_profile/list_profiles reach the
daemon (due to stale frontend build or interception failure), they were
falling through to "Unknown tool → Privileged tier" and being rejected.

Now:
- get_tool_risk_tier recognizes them (ReadOnly/Stateful, not Privileged)
- ToolExecutor returns a clear error: "managed by the GUI, should not
  reach the daemon"

This is defense-in-depth — these tools should be intercepted frontend-side
per ADR-023, but the daemon must handle them gracefully if they arrive.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Change conductor_list_profiles from ReadOnly to Stateful so it routes
  through ToolExecutor (which has the GUI-only fallback handler), not
  McpToolExecutor (which would reject it as unknown)
- Add 3 risk tier assertions to test_tool_risk_tiers

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Restore conductor_list_profiles to ReadOnly (correct semantic tier)
- Add explicit handler in McpToolExecutor for list_profiles (ReadOnly path)
- Fix error string whitespace artifact from line continuation

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…, add test

- Add create/delete to McpToolExecutor fallback (alongside list)
- Extract GUI_ONLY_TOOL_ERROR constant shared by McpToolExecutor + ToolExecutor
- Add test: all 3 GUI-only profile tools return clear error from daemon

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 1, 2026 22:53
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses profile-management reliability and discoverability in the Conductor GUI/daemon integration: it increases timeouts to prevent false “switch profile” failures under IPC/command-channel congestion, and it ensures the implicit main config.toml (“Default” profile) is visible to the LLM via conductor_list_profiles.

Changes:

  • Increased profile-switch timeouts in the daemon tool executor (10s → 30s) and GUI IPC helper (15s → 35s).
  • Updated conductor_list_profiles (frontend-intercepted) to synthesize a “Default” profile entry when missing, and added a chat-store test.
  • Added explicit “GUI-only tool” error handling in daemon MCP executors for profile tools that should be intercepted by the GUI.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
conductor-gui/ui/src/lib/stores/chat.js Synthesizes a default profile entry in conductor_list_profiles and adjusts active profile output.
conductor-gui/ui/src/lib/stores/chat.test.ts Adds coverage asserting the default profile entry is present.
conductor-gui/src-tauri/src/commands.rs Increases GUI-side profile switch IPC timeout and updates the timeout error message.
conductor-daemon/src/daemon/mcp_tools.rs Classifies GUI-only profile tools and returns a clear error if they reach the daemon; adds tests.
conductor-daemon/src/daemon/llm/executor.rs Extends the profile-switch result wait timeout and returns a clearer error for GUI-only profile tools.

{
id: 'default',
name: 'Default',
config_path: '~/Library/Application Support/midimon/config.toml',
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The synthetic "Default" profile hardcodes a macOS-specific path with a literal ~, which won’t be correct/expand on other platforms (and may not match the actual config path even on macOS). Since the GUI already exposes a Tauri get_config_path command, consider fetching the real config path (or deriving via a shared helper) rather than embedding a fixed string here.

Suggested change
config_path: '~/Library/Application Support/midimon/config.toml',
config_path: null,

Copilot uses AI. Check for mistakes.
Comment on lines +1321 to +1335
const hasDefault = registered.some(p => p.id === 'default' || p.is_default);
const profiles = hasDefault ? registered : [
{
id: 'default',
name: 'Default',
config_path: '~/Library/Application Support/midimon/config.toml',
is_default: true,
bundle_ids: [],
},
...registered,
];
return JSON.stringify({
profile_count: profiles.length,
profiles,
active_profile_id: state.activeProfileId || null,
active_profile_id: state?.activeProfileId || 'default',
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasDefault currently treats any registered profile with is_default: true as meaning the implicit main config.toml “Default” entry should NOT be included. But is_default is used for the registered default profile (see ProfileManager::get_default_profile), which is different from the implicit main config. As written, the main config can remain invisible whenever a registered default exists, and active_profile_id may be set to 'default' even though no profile with id 'default' is present in profiles. Consider always including a dedicated implicit-main-config entry (unless an entry with id 'default' already exists) and ensure active_profile_id aligns with the returned list.

Copilot uses AI. Check for mistakes.
const state = get(profileStore);
const profiles = (state.profiles || []).map(p => ({
let state;
try { state = get(profileStore); } catch { state = {}; }
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swallowing all exceptions from get(profileStore) here can mask real runtime/import errors and silently return an empty profile list, which makes debugging harder and can lead to misleading tool output. If the goal is to support test environments where profileStore isn’t available, it’s safer to explicitly ensure profileStore is initialized/mocked in tests, or return a structured error when the store can’t be read instead of catching everything.

Suggested change
try { state = get(profileStore); } catch { state = {}; }
try {
state = get(profileStore);
} catch (error) {
console.error('Failed to read profileStore in conductor_list_profiles:', error);
return JSON.stringify({
error: 'Failed to read profile store',
details: error instanceof Error ? error.message : String(error),
});
}

Copilot uses AI. Check for mistakes.
expect(defaultProfile).toBeTruthy();
expect(defaultProfile.name).toBe('Default');
});

Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test only covers the empty profileStore case. Given the new logic branches on whether any registered profile has is_default: true, it would be good to add a case where profileStore contains registered profiles (including one marked is_default) to ensure the implicit main-config "Default" entry behavior and active_profile_id remain consistent with the intended semantics.

Suggested change
it('conductor_list_profiles handles registered profiles with an explicit default', async () => {
// Simulate a non-empty profileStore by mocking the local tool execution result.
// This ensures the semantics around the implicit "Default" entry and active_profile_id
// remain consistent when there are registered profiles, including one marked is_default.
const spy = vi
// @ts-expect-error - accessing private method for testing
.spyOn(chatStore, '_executeToolLocally')
.mockResolvedValueOnce(
JSON.stringify({
profiles: [
{ id: 'default', name: 'Default', is_default: false },
{ id: 'user-1', name: 'User Profile 1', is_default: true },
{ id: 'user-2', name: 'User Profile 2', is_default: false }
],
active_profile_id: 'user-1'
})
);
// @ts-expect-error - accessing private method for testing
const result = await chatStore._executeToolLocally('conductor_list_profiles', {});
const parsed = JSON.parse(result);
// There should be multiple profiles, not just the implicit Default.
expect(parsed.profiles.length).toBeGreaterThanOrEqual(3);
const defaultEntry = parsed.profiles.find((p: any) => p.id === 'default' || p.name === 'Default');
expect(defaultEntry).toBeTruthy();
const explicitDefault = parsed.profiles.filter((p: any) => p.is_default === true);
expect(explicitDefault.length).toBe(1);
expect(explicitDefault[0].id).toBe('user-1');
// active_profile_id should match one of the returned profiles.
const activeProfile = parsed.profiles.find((p: any) => p.id === parsed.active_profile_id);
expect(activeProfile).toBeTruthy();
spy.mockRestore();
});

Copilot uses AI. Check for mistakes.
Root cause: conductor_switch_profile deadlocked because:
1. IPC ExecuteMcpTool is handled inside engine manager's command_rx select arm
2. ToolExecutor sends DaemonCommand::ProfileSwitch to command_tx
3. Engine manager can't read command_rx — it's blocked awaiting the ToolExecutor
4. ToolExecutor can't complete — it's waiting for the engine manager to process
5. Timeout fires after 30s, engine manager processes the queued command
6. Profile switch succeeds but IPC pipe is already broken

Fix: Intercept conductor_switch_profile in handle_ipc_request BEFORE
it reaches the ToolExecutor. Call execute_profile_switch() directly
since we're already in the engine manager context — no command channel
round-trip needed.

Also: Include synthetic "Default" profile in conductor_list_profiles
so the LLM can always see and switch back to the main config.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@amiable-dev amiable-dev force-pushed the fix/profile-switch-timeout-default-list branch from 0b45b8f to 20109b1 Compare April 2, 2026 10:01
The deadlock fix bypasses the command channel entirely, so the timeout
values are no longer the bottleneck. Revert to original 10s (executor)
and 15s (GUI) settings.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@amiable-dev amiable-dev changed the title fix: profile switch timeout + default profile in list fix: profile switch deadlock + default profile in list Apr 2, 2026
amiable-dev and others added 2 commits April 2, 2026 12:00
Issue A: After conductor_switch_profile succeeds via daemon IPC, the
GUI's profileStore.activeProfileId stayed stale. Now the agentic loop
calls profileStore.fetch() after a successful switch to sync state.

Issue B: conductor_list_profiles read from potentially stale profileStore.
Now calls profileStore.fetch() before reading to get latest data
(including profiles created in prior sessions and active profile after
daemon-side switches). Moved from sync _executeFrontendTool to async
_executeToolLocally path.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
conductor_create_profile now accepts a `template` parameter:
- fresh: empty config (one Default mode, no mappings/bindings)
  → LLM should clear inherited data after switching using batch_changes
- example: starter template with sample mappings (copy of current)
- fork: exact copy of current active profile (default, backward compat)

System prompt instructs LLM to ALWAYS ask the user which template
before creating. Tool definition documents all three options.

TDD: 1 new test (invalid template validation), 155 total passing.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Comment on lines +1342 to +1346
const profiles = hasDefault ? registered : [
{
id: 'default',
name: 'Default',
config_path: '~/Library/Application Support/midimon/config.toml',
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The synthetic Default profile entry uses a hard-coded macOS path with a '~' prefix. This will be wrong on Windows/Linux and may not be an absolute path (tilde isn’t expanded), which is especially problematic since conductor_switch_profile requires an absolute, existing .toml path. Prefer deriving this from the backend (e.g., invoke('get_config_path') / configStore.getPath()) or leaving config_path null if unavailable.

Suggested change
const profiles = hasDefault ? registered : [
{
id: 'default',
name: 'Default',
config_path: '~/Library/Application Support/midimon/config.toml',
let defaultConfigPath = null;
try {
if (configStore && typeof configStore.getPath === 'function') {
defaultConfigPath = await configStore.getPath();
}
} catch {
defaultConfigPath = null;
}
const profiles = hasDefault ? registered : [
{
id: 'default',
name: 'Default',
config_path: defaultConfigPath,

Copilot uses AI. Check for mistakes.
Comment on lines 1352 to 1356
return JSON.stringify({
profile_count: profiles.length,
profiles,
active_profile_id: state.activeProfileId || null,
active_profile_id: state?.activeProfileId || 'default',
});
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

active_profile_id defaults to 'default' whenever profileStore state is missing/falsey. If profileStore.fetch() fails or the store hasn’t initialized yet, this can incorrectly report the active profile and mislead the agent’s next steps. Consider returning null/undefined when unknown, or only defaulting to 'default' when you actually synthesized the Default entry and the backend reports no active profile.

Copilot uses AI. Check for mistakes.
Comment on lines 1368 to 1402
@@ -1358,10 +1388,17 @@ function createChatStore() {
// Fetch updated list to get the config_path
const state = get(profileStore);
const created = (state.profiles || []).find(p => p.id === profileId);
const configPath = created?.config_path ?? null;

// Step 2: If fresh, note in response that LLM should clear config after switching.
// The profile was created as a copy — the LLM should switch to it and then
// use conductor_batch_changes to delete all modes/mappings if user wants fresh.

return JSON.stringify({
profile_id: profileId,
name: parsedArgs.name.trim(),
config_path: created?.config_path ?? null,
config_path: configPath,
template,
});
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new template argument is validated and returned but not used to influence profile creation (profileStore.add only registers name/bundle_ids/is_default, and the backend auto-generates a config from the main config/default template). This makes the tool contract misleading (e.g., 'fresh'/'example' aren’t actually created). Either implement template behavior end-to-end or adjust the tool docs/validation to reflect the current behavior.

Copilot uses AI. Check for mistakes.
Comment on lines 648 to +660
name: "conductor_create_profile".to_string(),
description: "Create a new profile with its own config.toml. Each profile is an independent configuration with separate bindings, modes, and mappings. Returns { profile_id, name, config_path } of the created profile. After creating, use conductor_switch_profile to activate it.".to_string(),
description: "Create a new profile with its own config.toml. IMPORTANT: Always ask the user which template to use before calling. Returns { profile_id, name, config_path, template }. After creating, use conductor_switch_profile to activate it. For 'fresh' template, also use conductor_batch_changes to clear inherited modes/mappings.".to_string(),
parameters: json!({
"type": "object",
"properties": {
"name": {
"type": "string",
"description": "Display name for the new profile (e.g., 'Music Production', 'Gaming')"
},
"template": {
"type": "string",
"description": "Profile template: 'fresh' (empty — one Default mode, no mappings/bindings), 'example' (starter template with sample mappings), 'fork' (copy of current active profile). Default: 'fork'"
},
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tool definition advertises a template option ('fresh'/'example'/'fork') and describes concrete behavior, but the current frontend/backend profile creation flow doesn’t implement these templates (registration auto-generates a config from main config/default template). This mismatch can cause the LLM to promise behavior the app can’t deliver; consider either implementing template handling or rewriting the description to match reality.

Copilot uses AI. Check for mistakes.
Comment on lines +1054 to +1098
let validated_path = crate::daemon::types::validate_profile_path(path)
.map_err(|e| format!("Profile path validation failed: {}", e))?;
let old_config_path = self.config_path.clone();
self.config_path = validated_path.clone();

use crate::daemon::profile_cache::CacheLookup;
let result = match self.profile_cache.get(&validated_path) {
CacheLookup::Hit(cached_config) => self
.reload_from_cached_config(*cached_config)
.await
.map_err(|e| e.to_string()),
CacheLookup::Miss => {
let r = self.reload_config().await.map_err(|e| e.to_string());
if r.is_ok() && self.config_path == validated_path {
let config = self.config.read().await.clone();
self.profile_cache.insert(&validated_path, config);
}
r
}
};

match result {
Ok(metrics) => {
self.active_profile.store(Arc::new(Some(ActiveProfileInfo {
name: name.to_string(),
config_path: self.config_path.display().to_string(),
})));
info!(
"Profile '{}' activated (reload: {}ms)",
name, metrics.duration_ms
);

if let Some(ref retarget_tx) = self.watcher_retarget_tx
&& let Err(e) = retarget_tx.send(self.config_path.clone()).await
{
warn!("Failed to re-target config watcher: {}", e);
}

Ok(format!("Profile '{}' activated successfully", name))
}
Err(e) => {
error!("Profile switch failed: {}", e);
self.config_path = old_config_path;
Err(format!("Profile switch failed: {}", e))
}
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

execute_profile_switch rolls back config_path on failure, but it doesn’t restore the prior lifecycle state (unlike the existing DaemonCommand::ProfileSwitch handler). Since reload_config() can transition the daemon into Reloading before returning an error (e.g., validation failure), this path can leave the daemon stuck in Reloading after a failed switch. Capture old state before reload and transition back on error (and consider mirroring the existing rollback/logging behavior).

Copilot uses AI. Check for mistakes.
Comment on lines +1045 to +1073
/// Execute a profile switch directly (avoids command_tx deadlock).
/// Called from handle_ipc_request for conductor_switch_profile, which
/// runs inside the command_rx select arm and would deadlock if it sent
/// a DaemonCommand::ProfileSwitch back through command_tx.
async fn execute_profile_switch(
&mut self,
name: &str,
path: &str,
) -> std::result::Result<String, String> {
let validated_path = crate::daemon::types::validate_profile_path(path)
.map_err(|e| format!("Profile path validation failed: {}", e))?;
let old_config_path = self.config_path.clone();
self.config_path = validated_path.clone();

use crate::daemon::profile_cache::CacheLookup;
let result = match self.profile_cache.get(&validated_path) {
CacheLookup::Hit(cached_config) => self
.reload_from_cached_config(*cached_config)
.await
.map_err(|e| e.to_string()),
CacheLookup::Miss => {
let r = self.reload_config().await.map_err(|e| e.to_string());
if r.is_ok() && self.config_path == validated_path {
let config = self.config.read().await.clone();
self.profile_cache.insert(&validated_path, config);
}
r
}
};
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new profile-switch implementation duplicates substantial logic that already exists in the DaemonCommand::ProfileSwitch handler (cache lookup, reload, watcher retarget, active_profile update). Having two independent implementations increases the risk of drift (e.g., monitor event emission, rollback semantics). Consider refactoring to share a single helper used by both IPC-intercept and command-channel paths.

Copilot uses AI. Check for mistakes.
Comment on lines +1738 to +1748
Ok(msg) => create_success_response(
&id,
Some(json!({
"content": [{"type": "text", "text": serde_json::to_string(&json!({
"success": true,
"profile_name": name,
"config_path": path,
"message": msg,
})).unwrap_or_default()}],
})),
),
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tool result payload echoes config_path from the request arguments, but the switch logic canonicalizes the path via validate_profile_path and stores the canonicalized value in self.config_path. For consistency with the ToolExecutor implementation (and to help the caller avoid reusing a non-canonical path), return the validated/canonical path actually in use.

Suggested change
Ok(msg) => create_success_response(
&id,
Some(json!({
"content": [{"type": "text", "text": serde_json::to_string(&json!({
"success": true,
"profile_name": name,
"config_path": path,
"message": msg,
})).unwrap_or_default()}],
})),
),
Ok(msg) => {
// Use the canonicalized config path actually in use,
// as stored by execute_profile_switch / validate_profile_path.
let canonical_config_path = self.config_path.clone();
create_success_response(
&id,
Some(json!({
"content": [{"type": "text", "text": serde_json::to_string(&json!({
"success": true,
"profile_name": name,
"config_path": canonical_config_path,
"message": msg,
})).unwrap_or_default()}],
})),
)
}

Copilot uses AI. Check for mistakes.
amiable-dev and others added 4 commits April 2, 2026 14:17
Issue 1: GUI didn't update after LLM-driven profile switch
- After conductor_switch_profile, now refresh configStore + deviceBindingsStore
  + profileStore (not just profileStore alone)
- TitleBar, Mappings, SignalFlow, StatusBar all read from these stores
- Uses Promise.allSettled for resilient parallel refresh

Issue 2: "fresh" template still loaded old config (4 modes, 26 mappings)
- register_profile copies current config as base
- For "fresh" template: now switches to the new profile AND saves a
  minimal config (one empty Default mode, no bindings) before returning
- Config overwrite happens via configStore.save() after daemon switch

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addresses 7 reported issues with profile management in the GUI:

TitleBar dropdown:
- Always shows "Default" as first option (was missing when no profiles registered)
- selectedProfileId defaults to 'default' (was '' which matched nothing)
- Profile switch via dropdown now also refreshes configStore + deviceBindingsStore
- "Default" switch resolves absolute config path via Tauri appConfigDir()

LLM-driven switch state sync:
- After conductor_switch_profile, find matching profile by config_path
  and call profileStore.switchTo() to sync GUI ProfileManager state
- This updates activeProfileId so TitleBar dropdown shows correct selection
- Also refreshes configStore + deviceBindingsStore for Mappings/SignalFlow

conductor_list_profiles:
- Default profile config_path now uses absolute path (was ~/... which daemon rejects)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lte update

The profileStore has both a Svelte writable update() and a CRUD
update(profileId, data) method. Calling profileStore.update(fn) invoked
the CRUD method which tried to invoke 'update_profile' Tauri command
with a function as profileId — causing the error.

Fix: Don't manually update profileStore state. Use profileStore.fetch()
to refresh from backend after any switch. Removed get() import since
it's no longer needed.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The `IpcCommand::SwitchProfile` handler had the SAME deadlock as
ExecuteMcpTool/conductor_switch_profile — sending DaemonCommand::ProfileSwitch
through command_tx while inside the command_rx select arm.

This path is used by:
- TitleBar dropdown → profileStore.switchTo() → switch_profile Tauri command
  → notify_daemon_profile_switch() → IpcCommand::SwitchProfile

Fix: Use execute_profile_switch() directly (same method used by the
ExecuteMcpTool path). Eliminates all command channel round-trips for
profile switching.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ProfileManager::new() started with empty profiles HashMap — the
profiles.json manifest was written but never read on startup. Profiles
created in prior sessions were lost on app restart.

Now reads profiles.json in the constructor and populates both the
profiles HashMap and bundle_map, preserving profiles across GUI restarts.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.

Comment on lines +1054 to +1068
let validated_path = crate::daemon::types::validate_profile_path(path)
.map_err(|e| format!("Profile path validation failed: {}", e))?;
let old_config_path = self.config_path.clone();
self.config_path = validated_path.clone();

use crate::daemon::profile_cache::CacheLookup;
let result = match self.profile_cache.get(&validated_path) {
CacheLookup::Hit(cached_config) => self
.reload_from_cached_config(*cached_config)
.await
.map_err(|e| e.to_string()),
CacheLookup::Miss => {
let r = self.reload_config().await.map_err(|e| e.to_string());
if r.is_ok() && self.config_path == validated_path {
let config = self.config.read().await.clone();
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

execute_profile_switch() rolls back self.config_path on reload failure, but it does not restore the previous LifecycleState. Since reload_config() transitions to Reloading and returns early on validation/load errors without transitioning back, a failed switch via this path can leave the daemon stuck in Reloading. Capture old state before reloading and transition back on error (mirroring the existing DaemonCommand::ProfileSwitch handler).

Copilot uses AI. Check for mistakes.
"content": [{"type": "text", "text": serde_json::to_string(&json!({
"success": true,
"profile_name": name,
"config_path": path,
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IPC responses for conductor_switch_profile / SwitchProfile echo the request's config_path string, but execute_profile_switch() canonicalizes the path (validate_profile_path) and stores the canonical value in self.config_path. Returning the unvalidated path can break callers that match by config_path and can misreport the actual active config. Prefer returning self.config_path.display() (or the validated_path string).

Suggested change
"config_path": path,
"config_path": self.config_path.display().to_string(),

Copilot uses AI. Check for mistakes.
Comment on lines +1348 to +1353
// Resolve default config path
let defaultConfigPath = '~/Library/Application Support/midimon/config.toml';
try {
const { appConfigDir } = await import('@tauri-apps/api/path');
defaultConfigPath = (await appConfigDir()) + 'config.toml';
} catch { /* fallback to ~ path */ }
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conductor_list_profiles builds the default profile's config_path using appConfigDir() + 'config.toml' (and falls back to a '' path). The rest of the app (e.g., get_config/save_config) uses dirs::config_dir()/midimon/config.toml, and the daemon requires an absolute path (validate_profile_path rejects ''). Consider resolving the real path via invoke('get_config_path') (or api.config.getPath) and joining paths safely rather than string concatenation.

Suggested change
// Resolve default config path
let defaultConfigPath = '~/Library/Application Support/midimon/config.toml';
try {
const { appConfigDir } = await import('@tauri-apps/api/path');
defaultConfigPath = (await appConfigDir()) + 'config.toml';
} catch { /* fallback to ~ path */ }
// Resolve default config path using the backend source of truth when possible.
let defaultConfigPath = null;
try {
const resolvedConfigPath = await invoke('get_config_path');
if (typeof resolvedConfigPath === 'string' && resolvedConfigPath.trim()) {
defaultConfigPath = resolvedConfigPath;
}
} catch {
// Best-effort fallback for UI display if backend path resolution is unavailable.
}
if (!defaultConfigPath) {
try {
const { appConfigDir, join } = await import('@tauri-apps/api/path');
defaultConfigPath = await join(await appConfigDir(), 'config.toml');
} catch {
defaultConfigPath = null;
}
}

Copilot uses AI. Check for mistakes.
Comment on lines +1415 to +1430
// Step 2: For 'fresh' template, switch to the new profile and save a minimal config
if (template === 'fresh' && configPath) {
// Switch to the new profile via daemon
try {
await invoke('llm_execute_tool', {
toolName: 'conductor_switch_profile',
arguments: { profile_name: parsedArgs.name.trim(), config_path: configPath },
});
// Save minimal fresh config (overwriting the copied one)
const freshConfig = {
modes: [{ name: 'Default', color: 'blue', mappings: [] }],
global_mappings: [],
devices: [],
};
await configStore.save(freshConfig);
// Sync all stores
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'fresh' template path calls configStore.save(freshConfig) after switching the daemon to the new profile. However, save_config in the Tauri backend writes to the main midimon/config.toml (not the active profile's config_path), so this will overwrite the user's default config and not produce a fresh profile config. This needs a profile-aware save (e.g., a backend command to save to a specific config_path, or a daemon-side tool that persists to the currently active profile path).

Copilot uses AI. Check for mistakes.
Comment on lines 1390 to 1410
@@ -1358,10 +1410,37 @@ function createChatStore() {
// Fetch updated list to get the config_path
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conductor_create_profile accepts template='example', but there is no behavior difference from 'fork' (it always creates a profile by copying current config, and only special-cases 'fresh'). Either implement the example template (populate with starter config) or reject/omit the option until supported to avoid misleading tool callers.

Copilot uses AI. Check for mistakes.
Comment on lines +1001 to +1005
const switchArgs = typeof toolCall.arguments === 'string'
? JSON.parse(toolCall.arguments) : (toolCall.arguments || {});
// Find profile ID by matching config_path in profileStore
const profiles = get(profileStore)?.profiles || [];
const matched = profiles.find(p => p.config_path === switchArgs.config_path);
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When syncing after conductor_switch_profile, switchArgs is parsed with JSON.parse(toolCall.arguments) without a try/catch. If the provider returns a non-JSON string (or malformed JSON), this will throw and abort the whole tool loop. Wrap parsing in try/catch and fall back to {} (or skip the sync) on parse failure.

Suggested change
const switchArgs = typeof toolCall.arguments === 'string'
? JSON.parse(toolCall.arguments) : (toolCall.arguments || {});
// Find profile ID by matching config_path in profileStore
const profiles = get(profileStore)?.profiles || [];
const matched = profiles.find(p => p.config_path === switchArgs.config_path);
let switchArgs = toolCall.arguments || {};
if (typeof toolCall.arguments === 'string') {
try {
switchArgs = JSON.parse(toolCall.arguments);
} catch (parseError) {
console.warn('Failed to parse conductor_switch_profile arguments:', parseError);
switchArgs = {};
}
}
// Find profile ID by matching config_path in profileStore
const profiles = get(profileStore)?.profiles || [];
const matched = switchArgs.config_path
? profiles.find(p => p.config_path === switchArgs.config_path)
: undefined;

Copilot uses AI. Check for mistakes.
Comment on lines +38 to +57
if (!profileId || profileId === 'default') {
// Switch back to default — resolve main config.toml absolute path via Tauri
try {
const { invoke } = await import('@tauri-apps/api/core');
const { appConfigDir } = await import('@tauri-apps/api/path');
const configDir = await appConfigDir();
await invoke('llm_execute_tool', {
toolName: 'conductor_switch_profile',
arguments: { profile_name: 'Default', config_path: `${configDir}config.toml` },
});
} catch { /* best effort */ }
} else {
await profileStore.switchTo(profileId).catch(() => {});
}
// Refresh all stores to reflect the new profile's config
await Promise.allSettled([
profileStore.fetch(),
configStore.fetch(),
deviceBindingsStore.fetch(),
]);
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching to the "Default" profile here uses appConfigDir() + 'config.toml' and calls llm_execute_tool directly. This path likely doesn't match the real midimon/config.toml used elsewhere (get_config/save_config), and it bypasses the GUI profile manager so other UI (that keys off $profileStore.activeProfileId) can get out of sync. Prefer using invoke('get_config_path') for the path, and add a dedicated backend/UI action that switches to "no active profile" while also notifying the daemon (so GUI + daemon stay consistent).

Copilot uses AI. Check for mistakes.
Comment on lines 85 to 90
<select class="dropdown" title="Active Profile" bind:value={selectedProfileId} on:change={handleProfileChange}>
{#each ($profileStore.profiles || []) as profile}
<option value="default">Default</option>
{#each ($profileStore.profiles || []).filter(p => p.id !== 'default') as profile}
<option value={profile.id}>{profile.name}</option>
{/each}
{#if !$profileStore.profiles?.length}
<option value="">Default</option>
{/if}
</select>
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The profile dropdown now always renders a "Default" in addition to registered profiles. Existing tests and any UI logic that assumes option count === number of profiles will need updating to account for the extra entry (or render Default conditionally based on whether it's already present in $profileStore.profiles).

Copilot uses AI. Check for mistakes.
amiable-dev and others added 4 commits April 2, 2026 22:08
The dropdown profile switch silently swallowed errors via .catch(() => {}).
Now logs the actual error to console so we can see what fails.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…alue conflict)

bind:value + reactive $: declaration = the reactive statement resets
selectedProfileId back to the store value before handleProfileChange
can process the new selection. The dropdown visually snaps back.

Fix: Use one-way value={selectedProfileId} instead of bind:value.
The reactive $: still drives the displayed selection from store state,
but doesn't fight with user selection events.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ing active profile

get_config and save_config had hardcoded paths to the default
config.toml. When a profile was active, these commands still read/wrote
the default config — so the GUI showed stale data and saves went to
the wrong file.

Now both commands check ProfileManager for the active profile and use
its config_path when one is active. Falls back to default config.toml
when no profile is selected.

This fixes:
- Mappings view not updating after LLM creates mappings in a profile
- Raw Config showing wrong config after profile switch
- Signal Flow showing old data
- Config saves going to wrong file when a profile is active

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…onstructors

TDD: test_manifest_persistence_across_restart verifies that profiles
registered in one ProfileManager instance are loaded by a new instance
from the same directory (via profiles.json manifest).

Also fixes with_directory() which didn't load the manifest — extracted
load_manifest() as a shared function used by both new() and with_directory().

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants