Files
ClaudeDo/docs/superpowers/plans/2026-04-17-logic-bug-fixes.md

23 KiB
Raw Permalink Blame History

Logic Bug Fixes Implementation Plan

For agentic workers: REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (- [ ]) syntax for tracking.

Goal: Fix confirmed logic bugs across Worker, App/Ui, and Installer found in the 2026-04-17 three-agent review.

Architecture: Each bug is an isolated change to one or two files. Group by priority (Critical → High → Medium → Info). TDD where the bug is observable via xUnit integration test (Worker, Data); for UI/Installer bugs without test harness, do a focused manual repro and guard with a regression comment referencing the commit.

Tech Stack: .NET 8, Avalonia 12, CommunityToolkit.Mvvm, EF Core + SQLite, SignalR, xUnit (Worker tests only).


File Map

Worker:

  • Modify: src/ClaudeDo.Worker/Hub/WorkerHub.cs — remove premature RunCreated broadcast
  • Modify: src/ClaudeDo.Worker/Runner/TaskRunner.cs — emit RunCreated after run row insert
  • Modify: src/ClaudeDo.Worker/Services/QueueService.cs — add slot-collision guard on RunNow/ContinueTask
  • Modify: src/ClaudeDo.Worker/Runner/ClaudeArgsBuilder.cs — extend quoting to cover whitespace/newline
  • Test: tests/ClaudeDo.Worker.Tests/ClaudeArgsBuilderTests.cs — regression for newline in system prompt
  • Test: tests/ClaudeDo.Worker.Tests/QueueServiceSlotGuardTests.cs — regression for RunNow-while-queued

Ui:

  • Modify: src/ClaudeDo.Ui/ViewModels/TaskListViewModel.cs — guard nullability in AddTask, harden OnTaskUpdated
  • Modify: src/ClaudeDo.Ui/ViewModels/TaskDetailViewModel.cs — defer _taskId assignment until after cancel check
  • Modify: src/ClaudeDo.Ui/ViewModels/TaskEditorViewModel.cs — init TCS before dialog shown

Installer:

  • Modify: src/ClaudeDo.Installer/Steps/RegisterServiceStep.cs — remove inline start; reject CurrentUser without password
  • Modify: src/ClaudeDo.Installer/Steps/DownloadAndExtractStep.cs — rename-before-extract rollback
  • Modify: src/ClaudeDo.Installer/Core/UninstallRunner.cs — add removeAppData parameter
  • Modify: src/ClaudeDo.Installer/Steps/WriteConfigStep.cs — expand ~ in UiDbPath
  • Verify: src/ClaudeDo.Installer/App.xaml.cs — confirm Avalonia vs WPF usings

Critical

Task 1: Worker — fix RunCreated broadcast ordering (W1)

Bug: WorkerHub.RunNow fires RunCreated before the run row is inserted by RunOnceAsync. UI can receive an event for a row that does not yet exist.

Files:

  • Modify: src/ClaudeDo.Worker/Hub/WorkerHub.cs:35-50

  • Modify: src/ClaudeDo.Worker/Runner/TaskRunner.cs:234-256 (RunOnceAsync)

  • Step 1: Remove premature broadcast from WorkerHub

In src/ClaudeDo.Worker/Hub/WorkerHub.cs, replace the body of RunNow:

public async Task RunNow(string taskId)
{
    try
    {
        await _queue.RunNow(taskId);
    }
    catch (InvalidOperationException)
    {
        throw new HubException("override slot busy");
    }
    catch (KeyNotFoundException)
    {
        throw new HubException("task not found");
    }
}
  • Step 2: Emit RunCreated inside RunOnceAsync after row insert

In src/ClaudeDo.Worker/Runner/TaskRunner.cs, find RunOnceAsync. After the runRepo.AddAsync(run, ct); block (~line 256), add:

await _broadcaster.RunCreated(taskId, runNumber, isRetry);

Then remove the existing await _broadcaster.RunCreated(task.Id, 2, true); on line 128 (inside the auto-retry block in RunAsync) and the await _broadcaster.RunCreated(taskId, nextRunNumber, false); on line 219 (in ContinueAsync), since RunOnceAsync now broadcasts unconditionally.

  • Step 3: Build and run Worker tests

Run: dotnet build ClaudeDo.slnx && dotnet test tests/ClaudeDo.Worker.Tests Expected: all existing tests pass.

  • Step 4: Commit
git add src/ClaudeDo.Worker/Hub/WorkerHub.cs src/ClaudeDo.Worker/Runner/TaskRunner.cs
git commit -m "fix(worker): emit RunCreated after run row exists"

Task 2: Ui — harden OnTaskUpdated against async void crash (U2)

Bug: TaskListViewModel.OnTaskUpdated is async void with no try/catch. A DB error escapes to TaskScheduler.UnobservedTaskException and can crash the process.

Files:

  • Modify: src/ClaudeDo.Ui/ViewModels/TaskListViewModel.cs:328-332

  • Step 1: Wrap handler body in try/catch

Replace the existing method with:

private async void OnTaskUpdated(string taskId)
{
    if (CurrentListId is null) return;
    try
    {
        await RefreshSingleAsync(taskId);
    }
    catch (Exception ex)
    {
        System.Diagnostics.Debug.WriteLine($"[TaskListViewModel] OnTaskUpdated failed for {taskId}: {ex}");
    }
}
  • Step 2: Build

Run: dotnet build ClaudeDo.slnx Expected: build succeeds.

  • Step 3: Commit
git add src/ClaudeDo.Ui/ViewModels/TaskListViewModel.cs
git commit -m "fix(ui): swallow DB errors in TaskListViewModel.OnTaskUpdated"

Task 3: Installer — reject CurrentUser service registration without password (I1)

Bug: RegisterServiceStep passes obj=.\<user> to sc.exe create with no password=. SCM rejects it with exit 5 / 1069 and the user gets an opaque error.

Files:

  • Modify: src/ClaudeDo.Installer/Steps/RegisterServiceStep.cs

  • Step 1: Read the current file

Read src/ClaudeDo.Installer/Steps/RegisterServiceStep.cs to confirm the exact shape of the obj= branch and the outer StepResult return.

  • Step 2: Replace CurrentUser branch with early failure

Where the step builds obj=".\\<username>" for the CurrentUser account option, replace it with:

if (ctx.ServiceAccount == ServiceAccountType.CurrentUser)
{
    return StepResult.Fail(
        "Service cannot run as Current User without a password. " +
        "Select 'Local System' or extend ServicePage to capture a password.");
}

Keep the LocalSystem branch (which passes obj= LocalSystem with no password requirement) unchanged.

  • Step 3: Build

Run: dotnet build ClaudeDo.slnx Expected: build succeeds.

  • Step 4: Commit
git add src/ClaudeDo.Installer/Steps/RegisterServiceStep.cs
git commit -m "fix(installer): reject CurrentUser service account without password"

High

Task 4: Worker — guard slot collision on RunNow and ContinueTask (W2)

Bug: Queue slot and override slot have no guard against operating on the same taskId. TaskRunner.MarkRunningAsync can overwrite started_at.

Files:

  • Modify: src/ClaudeDo.Worker/Services/QueueService.cs:59-115

  • Test: tests/ClaudeDo.Worker.Tests/QueueServiceSlotGuardTests.cs (new)

  • Step 1: Write failing test

Create tests/ClaudeDo.Worker.Tests/QueueServiceSlotGuardTests.cs:

using ClaudeDo.Data.Models;
using ClaudeDo.Worker.Services;
using Xunit;

namespace ClaudeDo.Worker.Tests;

public class QueueServiceSlotGuardTests : WorkerTestBase
{
    [Fact]
    public async Task RunNow_rejects_task_already_active_in_queue_slot()
    {
        var queue = ServiceProvider.GetRequiredService<QueueService>();
        var task = await SeedAgentTaskAsync(listId: await SeedListAsync(), title: "blocker");

        // Prime queue slot by wake signal.
        queue.WakeQueue();
        await WaitForActiveSlotAsync("queue", task.Id);

        // RunNow on the same id must throw InvalidOperationException.
        await Assert.ThrowsAsync<InvalidOperationException>(() => queue.RunNow(task.Id));
    }
}

(Helpers WorkerTestBase, SeedAgentTaskAsync, SeedListAsync, WaitForActiveSlotAsync exist in the test project — follow the pattern from existing tests.)

  • Step 2: Run test to verify it fails

Run: dotnet test tests/ClaudeDo.Worker.Tests --filter "FullyQualifiedName~QueueServiceSlotGuardTests" Expected: FAIL — currently RunNow succeeds and creates a duplicate slot.

  • Step 3: Add guard in RunNow

In src/ClaudeDo.Worker/Services/QueueService.cs, inside the lock (_lock) block in RunNow (~line 69), add before the existing override check:

if (_queueSlot?.TaskId == taskId)
    throw new InvalidOperationException("task is already running in queue slot");
  • Step 4: Add same guard in ContinueTask

In the lock (_lock) block in ContinueTask (~line 97), add:

if (_queueSlot?.TaskId == taskId)
    throw new InvalidOperationException("task is already running in queue slot");
  • Step 5: Run tests

Run: dotnet test tests/ClaudeDo.Worker.Tests Expected: PASS.

  • Step 6: Commit
git add src/ClaudeDo.Worker/Services/QueueService.cs tests/ClaudeDo.Worker.Tests/QueueServiceSlotGuardTests.cs
git commit -m "fix(worker): guard against same task in queue and override slot"

Task 5: Worker — quote CLI args with tab/newline/carriage-return (W3)

Bug: ClaudeArgsBuilder.Escape only quotes on space/quote. System prompts with newlines pass through unquoted and corrupt the argument list.

Files:

  • Modify: src/ClaudeDo.Worker/Runner/ClaudeArgsBuilder.cs:56-64

  • Test: tests/ClaudeDo.Worker.Tests/ClaudeArgsBuilderTests.cs (new or existing)

  • Step 1: Write failing test

If ClaudeArgsBuilderTests.cs does not exist, create it:

using ClaudeDo.Worker.Runner;
using Xunit;

namespace ClaudeDo.Worker.Tests;

public class ClaudeArgsBuilderTests
{
    [Fact]
    public void Build_quotes_system_prompt_with_newline()
    {
        var builder = new ClaudeArgsBuilder();
        var args = builder.Build(new ClaudeRunConfig(
            Model: null,
            SystemPrompt: "line1\nline2",
            AgentPath: null,
            ResumeSessionId: null));

        Assert.Contains("--append-system-prompt \"line1\\nline2\"", args);
    }

    [Fact]
    public void Build_quotes_system_prompt_with_tab()
    {
        var builder = new ClaudeArgsBuilder();
        var args = builder.Build(new ClaudeRunConfig(
            Model: null,
            SystemPrompt: "col1\tcol2",
            AgentPath: null,
            ResumeSessionId: null));

        Assert.Contains("\"col1", args);
    }
}
  • Step 2: Run test to verify it fails

Run: dotnet test tests/ClaudeDo.Worker.Tests --filter "FullyQualifiedName~ClaudeArgsBuilderTests" Expected: FAIL — newline is passed through unquoted.

  • Step 3: Extend Escape condition and escape newline/tab

In src/ClaudeDo.Worker/Runner/ClaudeArgsBuilder.cs, replace Escape:

private static string Escape(string value)
{
    if (value.Contains(' ') || value.Contains('"') || value.Contains('\'')
        || value.Contains('\t') || value.Contains('\n') || value.Contains('\r'))
    {
        var escaped = value
            .Replace("\\", "\\\\")
            .Replace("\"", "\\\"")
            .Replace("\n", "\\n")
            .Replace("\r", "\\r")
            .Replace("\t", "\\t");
        return $"\"{escaped}\"";
    }
    return value;
}
  • Step 4: Run tests

Run: dotnet test tests/ClaudeDo.Worker.Tests --filter "FullyQualifiedName~ClaudeArgsBuilderTests" Expected: PASS.

  • Step 5: Commit
git add src/ClaudeDo.Worker/Runner/ClaudeArgsBuilder.cs tests/ClaudeDo.Worker.Tests/ClaudeArgsBuilderTests.cs
git commit -m "fix(worker): escape newline/tab in CLI args"

Task 6: Installer — remove inline service start from RegisterServiceStep (I2)

Bug: RegisterServiceStep calls sc.exe start inline. StartServiceStep exists separately. If the update path ever wires both, the service is started twice.

Files:

  • Modify: src/ClaudeDo.Installer/Steps/RegisterServiceStep.cs

  • Modify: src/ClaudeDo.Installer/App.xaml.cs (pipeline) — ensure StartServiceStep is in the fresh-install pipeline

  • Step 1: Read current pipeline wiring in App.xaml.cs

Read src/ClaudeDo.Installer/App.xaml.cs around line 112 to confirm the list of steps passed into InstallerService.

  • Step 2: Remove inline sc.exe start from RegisterServiceStep

Delete the block (~lines 72-77) that runs sc.exe start <service> when ctx.AutoStart == true.

  • Step 3: Add StartServiceStep to the fresh-install pipeline if missing

In App.xaml.cs, append new StartServiceStep(...) after RegisterServiceStep in the step list. Gate its execution internally on ctx.AutoStart (it already handles exit code 1056).

  • Step 4: Build

Run: dotnet build ClaudeDo.slnx Expected: build succeeds.

  • Step 5: Commit
git add src/ClaudeDo.Installer/Steps/RegisterServiceStep.cs src/ClaudeDo.Installer/App.xaml.cs
git commit -m "fix(installer): move service start out of RegisterServiceStep"

Task 7: Installer — rollback-safe extract in DownloadAndExtractStep (I3)

Bug: Old app/ and worker/ are deleted before extraction. If extraction throws, user is left with no binaries and no recovery path.

Files:

  • Modify: src/ClaudeDo.Installer/Steps/DownloadAndExtractStep.cs:70-95

  • Step 1: Read the current delete/extract sequence

Read src/ClaudeDo.Installer/Steps/DownloadAndExtractStep.cs around lines 70-95 to identify the exact Directory.Delete and ZipFile.ExtractToDirectory calls and which ctx paths they reference.

  • Step 2: Replace delete-before-extract with rename-then-commit

Wrap the delete+extract block:

var appDir  = Path.Combine(ctx.InstallRoot, "app");
var workDir = Path.Combine(ctx.InstallRoot, "worker");
var appBak  = appDir + ".bak";
var workBak = workDir + ".bak";

// Stash existing dirs.
if (Directory.Exists(appBak))  Directory.Delete(appBak, recursive: true);
if (Directory.Exists(workBak)) Directory.Delete(workBak, recursive: true);
if (Directory.Exists(appDir))  Directory.Move(appDir, appBak);
if (Directory.Exists(workDir)) Directory.Move(workDir, workBak);

try
{
    ZipFile.ExtractToDirectory(zipPath, ctx.InstallRoot, overwriteFiles: true);
}
catch
{
    // Roll back to previous binaries.
    if (Directory.Exists(appDir))  Directory.Delete(appDir, recursive: true);
    if (Directory.Exists(workDir)) Directory.Delete(workDir, recursive: true);
    if (Directory.Exists(appBak))  Directory.Move(appBak, appDir);
    if (Directory.Exists(workBak)) Directory.Move(workBak, workDir);
    throw;
}

// Success — drop stash.
if (Directory.Exists(appBak))  Directory.Delete(appBak, recursive: true);
if (Directory.Exists(workBak)) Directory.Delete(workBak, recursive: true);
  • Step 3: Build

Run: dotnet build ClaudeDo.slnx Expected: build succeeds.

  • Step 4: Commit
git add src/ClaudeDo.Installer/Steps/DownloadAndExtractStep.cs
git commit -m "fix(installer): rollback-safe extract with .bak stash"

Bug: Uninstaller always deletes user data (db, logs, configs). Reinstalling a different version silently destroys all tasks.

Files:

  • Modify: src/ClaudeDo.Installer/Core/UninstallRunner.cs:60-80

  • Modify: src/ClaudeDo.Installer/Views/UninstallPage.xaml(.cs) (or equivalent) — add a checkbox

  • Step 1: Read UninstallRunner.RunAsync signature

Read src/ClaudeDo.Installer/Core/UninstallRunner.cs around lines 1-90 to get current signature.

  • Step 2: Add removeAppData parameter to RunAsync

Change signature to:

public async Task<UninstallResult> RunAsync(bool removeAppData, CancellationToken ct = default)

Guard the deletion:

if (removeAppData)
{
    var appData = Paths.Expand("~/.todo-app");
    if (Directory.Exists(appData))
        Directory.Delete(appData, recursive: true);
}
  • Step 3: Wire a "Remove user data" checkbox on the uninstall page

In the uninstall view/VM, add [ObservableProperty] private bool _removeAppData; (default false) and pass it into RunAsync(RemoveAppData).

  • Step 4: Build

Run: dotnet build ClaudeDo.slnx Expected: build succeeds.

  • Step 5: Commit
git add src/ClaudeDo.Installer/Core/UninstallRunner.cs src/ClaudeDo.Installer/Views/UninstallPage.xaml src/ClaudeDo.Installer/Views/UninstallPage.xaml.cs
git commit -m "fix(installer): make user-data deletion on uninstall opt-in"

Medium

Task 9: Ui — guard AddTask against null CurrentListId after await (U1)

Bug: AddTask awaits editor.LoadAgentsAsync. Between CanAddTask and listRepo.GetByIdAsync(CurrentListId) on line 164, a concurrent LoadAsync(null) could null the id. Compiler warns CS8604.

Files:

  • Modify: src/ClaudeDo.Ui/ViewModels/TaskListViewModel.cs:157-170

  • Step 1: Capture CurrentListId before the first await

Replace the start of AddTask:

[RelayCommand(CanExecute = nameof(CanAddTask))]
private async Task AddTask()
{
    var listId = CurrentListId;
    if (listId is null) return;

    string defaultCommitType;
    using (var context = _dbFactory.CreateDbContext())
    {
        var listRepo = new ListRepository(context);
        var list = await listRepo.GetByIdAsync(listId);
        defaultCommitType = list?.DefaultCommitType ?? "chore";
    }

    var editor = _editorFactory();
    await editor.LoadAgentsAsync(_worker);
    editor.InitForCreate(listId, defaultCommitType);
    // …rest unchanged, but use `listId` consistently where CurrentListId was read

Audit the rest of the method: replace every subsequent read of CurrentListId with listId.

  • Step 2: Build

Run: dotnet build ClaudeDo.slnx Expected: build succeeds with no CS8604 on this method.

  • Step 3: Commit
git add src/ClaudeDo.Ui/ViewModels/TaskListViewModel.cs
git commit -m "fix(ui): capture CurrentListId before await in AddTask"

Task 10: Ui — defer _taskId assignment in TaskDetailViewModel.LoadAsync (U3)

Bug: _taskId = taskId is set at line 87, before the previous _loadCts is cancelled. If load is cancelled, _taskId has been clobbered but HasWorktree / CanWorktreeAction still reflect the previous task.

Files:

  • Modify: src/ClaudeDo.Ui/ViewModels/TaskDetailViewModel.cs:76-90

  • Step 1: Reset stale worktree state when starting a new load

Replace the start of LoadAsync:

public async Task LoadAsync(string taskId)
{
    var oldCts = _loadCts;
    var cts = new CancellationTokenSource();
    _loadCts = cts;
    oldCts?.Cancel();
    oldCts?.Dispose();
    var ct = cts.Token;

    _taskId = taskId;

    // Clear stale worktree state so buttons don't act on the previous task.
    HasWorktree = false;
    WorktreeState = "";
    BranchName = null;
    DiffStat = null;
    WorktreePath = null;
    OnPropertyChanged(nameof(CanWorktreeAction));

    LiveText = "";
    _formatter = new StreamLineFormatter();
    // …rest unchanged
  • Step 2: Build

Run: dotnet build ClaudeDo.slnx Expected: build succeeds.

  • Step 3: Commit
git add src/ClaudeDo.Ui/ViewModels/TaskDetailViewModel.cs
git commit -m "fix(ui): reset stale worktree state on TaskDetail reload"

Task 11: Ui — initialize TCS before dialog shown in TaskEditorViewModel (U4)

Bug: ShowAndWaitAsync creates a fresh _tcs only when called. If Save fires before ShowAndWaitAsync (possible if ShowDialogAsync is ever awaited), the result is dropped.

Files:

  • Modify: src/ClaudeDo.Ui/ViewModels/TaskEditorViewModel.cs:72-80, 260-264

  • Modify: src/ClaudeDo.Ui/ViewModels/ListEditorViewModel.cs (same pattern — apply identically)

  • Step 1: Reset _tcs at the start of InitForCreate and InitForEditAsync

In TaskEditorViewModel.cs, at the top of InitForCreate:

public void InitForCreate(string listId, string defaultCommitType = "chore")
{
    _tcs = new TaskCompletionSource<TaskEntity?>();
    _editId = null;
    // …rest unchanged

Same first line at the top of InitForEditAsync and InitForEdit.

  • Step 2: Remove re-assignment in ShowAndWaitAsync
public Task<TaskEntity?> ShowAndWaitAsync() => _tcs.Task;
  • Step 3: Apply the same pattern to ListEditorViewModel

Mirror the same three edits in src/ClaudeDo.Ui/ViewModels/ListEditorViewModel.cs (reset TCS in its InitForCreate / InitForEdit, strip the creation in ShowAndWaitAsync).

  • Step 4: Build

Run: dotnet build ClaudeDo.slnx Expected: build succeeds.

  • Step 5: Commit
git add src/ClaudeDo.Ui/ViewModels/TaskEditorViewModel.cs src/ClaudeDo.Ui/ViewModels/ListEditorViewModel.cs
git commit -m "fix(ui): init editor TCS before dialog can complete"

Task 12: Installer — expand ~ in UiDbPath (I5)

Bug: workerCfg.DbPath = Paths.Expand(ctx.DbPath) but uiCfg.DbPath = ctx.UiDbPath is stored as-is. If UI cannot expand ~ at runtime on Windows, DB path is unresolvable.

Files:

  • Modify: src/ClaudeDo.Installer/Steps/WriteConfigStep.cs:31-34

  • Step 1: Expand UiDbPath symmetrically

Change the assignment:

uiCfg.DbPath = Paths.Expand(ctx.UiDbPath);
  • Step 2: Build

Run: dotnet build ClaudeDo.slnx Expected: build succeeds.

  • Step 3: Commit
git add src/ClaudeDo.Installer/Steps/WriteConfigStep.cs
git commit -m "fix(installer): expand ~ in UiDbPath"

Info

Task 13: Installer — verify App.xaml.cs WPF-vs-Avalonia usings (I6)

Suspected bug: src/ClaudeDo.Installer/App.xaml.cs uses System.Windows (WPF). If the project is Avalonia, wrong base class is inherited.

Files:

  • Read: src/ClaudeDo.Installer/App.xaml.cs

  • Read: src/ClaudeDo.Installer/ClaudeDo.Installer.csproj

  • Step 1: Inspect the csproj for the UI framework SDK

Read src/ClaudeDo.Installer/ClaudeDo.Installer.csproj and look for <UseWPF> or <PackageReference Include="Avalonia" ... />.

  • Step 2: Decision fork

  • If WPF (<UseWPF>true</UseWPF>): System.Windows is correct. Stop. No fix needed.

  • If Avalonia: replace using System.Windows; with using Avalonia; and change Application / StartupEventArgs / ExitEventArgs to Avalonia equivalents (Avalonia.Application, lifetime OnFrameworkInitializationCompleted).

  • Step 3: Build

Run: dotnet build ClaudeDo.slnx Expected: build succeeds.

  • Step 4: Commit only if changed
git add src/ClaudeDo.Installer/App.xaml.cs
git commit -m "fix(installer): use Avalonia application base class"

Out of Scope

Deferred / not fixed in this plan:

  • TryScheduleTrampolineDelete PID-less delay (I6 in review, low severity) — ping -n 3 is flaky but rarely hit
  • AvailableAgents being List<T> instead of ObservableCollection<T> (U5/info) — current OnPropertyChanged pattern works; revisit only if a bug manifests

Self-Review Notes

  • Every Worker bug (W1W3) has a regression test or tested path.
  • Every UI fix names the exact file:line and shows the replacement snippet.
  • Installer Task 3 (I1) does not guess a password-capture UI — it deliberately returns StepResult.Fail, leaving the UX change for a later plan.
  • Task 13 (I6) is a conditional task with a decision fork; no speculative rewrite.
  • Types are consistent: RunCreated(taskId, runNumber, isRetry) in Task 1 matches the existing HubBroadcaster.RunCreated signature used at TaskRunner.cs:128,219.