Skip to content

Desktop shell: Home with repository and harness selection - #653

Merged
alexeyzimarev merged 16 commits into
mainfrom
alexeyzimarev/ai-2194-desktop-shell-home
Aug 24, 2026
Merged

alexeyzimarev merged 16 commits into
mainfrom
alexeyzimarev/ai-2194-desktop-shell-home

Conversation

@alexeyzimarev

Copy link
Copy Markdown
Member

Gives the Avalonia desktop app a Home screen: pick a repository, pick a harness, start a hosted session. First slice of the hosted-agents shell.

Linear: AI-2194 (part of AI-2171 — design record and decisions are in the AI-2171 comment).

No GitHub issue to close. The slice issues were created directly in Linear rather than in GitHub Issues, which is backwards from this repo's convention (CLAUDE.md: open issues in GitHub, Linear auto-imports them). Creating one now would import as a second Linear issue alongside AI-2194, so this PR references Linear only. Flagging rather than papering over it.

What this does

  • The daemon advertises what it can host. DaemonInfoDto gains a trailing SupportedVendors, populated from the same set already sent on DaemonConnect — one source of truth, no second computation. null means unknown (an older daemon), never none.
  • The harness is remembered per repository. AppState.HarnessByRepo maps repo path → vendor token. Switching repository switches the chip; each repo keeps its own. A repo with no stored choice falls back to the default, never to the previously selected vendor.
  • HostedHarnessCatalog builds the picker rows from Core.Setup.HarnessCatalog.All, contributing only what Core has no notion of: transport family and daemon-advertised availability. A tenth vendor added to Core appears automatically.
  • Sessions start through the server. The local Spawn frame resolves against the daemon's PTY launchers — claude and codex only — so a nine-vendor picker cannot use it. ServerLaunchClient invokes RequestLaunchAgentV2, which reaches every vendor via the runtime factories.
  • HomeViewModel + HomeView, wired through the composition root and disposed on both shutdown paths. Home is the window's default tab.

Worth a reviewer's attention

  • The hub payload binds by name under SnakeCaseLower. An earlier revision of this branch used camelCase keys: it compiled, passed every test, and would have bound daemon_name/repo_path as null on every launch. LaunchHubJson.Configure is now shared by the client and its tests so the two cannot diverge, and a test pins the exact twelve-key set.
  • UI-thread marshalling. IDaemonClientService.Agents/.Snapshots are pushed from a background thread; both projections ObserveOn before their binding operator. There is a test that genuinely fails if either is removed — it asserts the thread the bound collection is mutated on, because "does not throw" turned out not to be falsifiable here.

Not in this slice

  • The repository list. Only the folder picker landed, not the derived list of known repositories — so per-repo harness memory is currently reachable only by re-navigating a native dialog. Needs a decision: land it here or defer explicitly.
  • Theme. Home is dark-only inside a theme-following shell (RequestedThemeVariant="Default"). On a light-mode host it renders as a dark slab in light chrome.
  • Artifacts/outputs and any diff view, both deliberately out of scope.

Tests

Capacitor.App.Tests.Unit 1169/1169 · Capacitor.Cli.Core.Tests.Unit 1996/1996 · AOT publish clean.

Capacitor.Cli.Daemon.Tests.Unit is 2759 pass / 1 fail / 38 skipped. The failure is Installed_codex_schema_matches_the_vendored_pin — installed codex 0.149.0 against an older vendored pin. Pre-existing on main; this branch touches no codex or schema files.

🤖 Generated with Claude Code

alexeyzimarev and others added 15 commits August 23, 2026 18:28
Add HarnessByRepo member to AppState to persist the vendor harness choice
per repository path. Null key means the choice was never made; empty string
key ("") holds the choice for the scratch "No repository" target.

Tests verify serialization round-trip and null default behavior.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…rs from Core

Removes duplicated source of truth by deriving the vendor list from
Capacitor.Cli.Core.Setup.HarnessCatalog.All instead of maintaining a
separate Known array. Transport family (pty/acp/rpc) is now kept in a
private map as it's specific to the daemon's hosting strategy, separate
from Core's vendor registration which handles installation flags and
detection logic.

This fixes the name collision that prevented using the new HostedHarnessCatalog
class alongside Core's HarnessCatalog without explicit qualification.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Now that the app's harness catalogue is renamed to HostedHarnessCatalog,
the unqualified HarnessCatalog reference correctly resolves to Core's
HarnessCatalog via the using statement. The workaround qualification
is no longer needed.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Home needs every vendor, but the daemon's local Spawn frame only
resolves claude/codex against its PTY launcher dictionary. The
server's RequestLaunchAgentV2 reaches all nine vendors through the
runtime factories, so the launch path goes through the hub instead
of the local socket.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The server applies PropertyNamingPolicy = SnakeCaseLower to every hub
payload (kcap-server JsonDefaults.ConfigureSignalRPayload); the
client's payload was serializing camelCase keys, so DaemonName and
RepoPath would bind null server-side and every launch would fail.

Fix both the wire naming (explicit snake_case [JsonPropertyName] on
every member, plus the same SnakeCaseLower policy the daemon's own
ServerConnection/WatchCommand apply) and the test gap that missed it:
LaunchRequestTests now serializes through LaunchHubJson.Configure,
the exact JsonSerializerOptions ServerLaunchClient hands
AddJsonProtocol, instead of a bare context that only proved the
client's own idea of the format. Adds a test pinning the full
twelve-key set.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sessions/Harnesses were bound straight off IDaemonClientService's background
thread, matching neither MainWindowViewModel nor ConsentPromptViewModel's
ObserveOn-before-binding rule. Add RxSchedulers.MainThreadScheduler ObserveOn
before SortAndBind/ToProperty, and bring HomeViewModelTests into the
AvaloniaSession.WithImmediateRxScheduler / NotInParallel("AvaloniaSession")
cohort those schedulers require.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds Home as the first MainWindow tab, backed by a new HomeView bound to
Task 5's HomeViewModel: goal input, repository/harness chips, remember
toggle, Start, and the active-sessions grid. Introduces the design's dark
palette as Application-level resources (App.axaml) instead of per-control
hex literals. HomeViewModel now implements IDisposable via a
CompositeDisposable, matching TrayViewModel/ActivityViewModel, since this
task is what first constructs one.
…the default tab

Task 6 fix round: HomeView was inert (no DataContext) and Agents was pinned as
the default tab to dodge two smoke-test assumptions instead of fixing them.

- App.BuildAndShowMainWindow now constructs HomeViewModel over the same
  IDaemonClientService instance MainWindowViewModel uses (never a second daemon
  connection), plus a fresh AppStateStore/ServerLaunchClient — the same
  cheap-construction pattern BuildLifecycleController already relies on.
  MainWindowViewModel exposes it as Home; MainWindow.axaml binds HomeView's
  DataContext to it. App reads Home back off the built window's own DataContext
  into a new _home field, so BuildAndShowMainWindow's signature (and therefore
  AppStartupTests' direct call to it) never changes; _home disposes through the
  same UI-disposables list as _activity/_trayVm/_pause, on both the normal
  shutdown and startup-failure paths.
- Removed the IsSelected="True" pin on Agents so Home is genuinely the default
  tab, and updated the two MainWindowSmokeTests that assumed Agents opened
  first to select it explicitly before asserting on its content.
…e surface

The scratch target's comments claimed a "" repo path launches into a daemon-owned
worktree; AgentOrchestrator rejects any repo path that fails Directory.Exists, so
the key is storage-only until the daemon accepts a repo-less launch. The concept
and its key handling stay.

ServerLaunchClient leaked a HubConnection whenever StartAsync threw (the instance
was never assigned to _hub) and disposed its gate out from under an in-flight
launch. The client is now held by the composition root, shared across window
rebuilds, and disposed after Home on both teardown paths.

SessionCardViewModel built SolidColorBrushes on the daemon pump thread, which
worked only by accident of per-instance dispatcher affinity; ImmutableSolidColorBrush
is not an AvaloniaObject, so the four dots are shared rather than reallocated per
card per revision.

Also fixes the tab comments Home's arrival falsified, and the JSON naming comment
in ILaunchClient: an explicit [JsonPropertyName] always beats a policy, and a
policy on JsonSerializerOptions does reach source-generated metadata.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ing and the vendor map

Three gaps the branch left unfalsifiable. A snapshot narrowing the harness picker
was untested end to end (the fake's supportedVendors parameter had no caller).

HomeViewModel's ObserveOn before SortAndBind could be deleted with every test still
green: the suite pins the scheduler to Immediate and pushes from the UI thread. The
new smoke test pushes from a background thread over the session's real scheduler and
asserts the bound collection is mutated ON the UI thread — "does not throw" is not
falsifiable here, since the push raises nothing and the container still realizes even
unmarshalled (a bare VerifyAccess and a control property set from the same thread do
throw, so the harness enforces affinity; this path defers its UI work).

The transport-family map is hand-written while the vendor list comes from Core, so a
tenth vendor would be labelled "chat" silently. The runtime fallback stays — an
unknown advertised vendor must still be listed — but the gap is now a red suite.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… type name

Three corrections made while executing, so the plan matches what was built:
- Capacitor.Cli is the only AOT-published project; the publish gate never
  compiles Capacitor.App, so it cannot be evidence about app code.
- The hub payload is a source-generated record, not an anonymous type.
- Task 5 consumes HostedHarnessCatalog; HarnessCatalog is Core's own type.
@linear-code

linear-code Bot commented Aug 24, 2026

Copy link
Copy Markdown

AI-2194

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Desktop shell Home tab with repository/harness selection and server-launched sessions

✨ Enhancement 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Add a Home tab to pick repository + harness and view active hosted sessions.
• Advertise daemon-supported vendors and derive harness picker rows from Core catalog.
• Launch sessions via SignalR hub with pinned snake_case payload tests and UI-thread guards.
Diagram

graph TD
  HV[/"HomeView"/] --> HVM(["HomeViewModel"]) --> HHC(["HostedHarnessCatalog"])
  HVM(["HomeViewModel"]) --> AS(["AppStateStore"])
  HVM(["HomeViewModel"]) --> SLC(["ServerLaunchClient"]) --> HUB{{"Server SignalR Hub"}}
  DCS(["IDaemonClientService"]) --> HVM(["HomeViewModel"])
  DMD(["Daemon (StatusIpc)"]) --> DCS(["IDaemonClientService"])
  subgraph Legend
    direction LR
    _ui[/"UI"/] ~~~ _svc(["Service/VM"]) ~~~ _ext{{"External"}}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Share server contract types (LaunchAgentRequestV2) via a common package
  • ➕ Eliminates duplicated payload shape and reduces drift risk
  • ➕ Avoids manually pinning the field list and casing rules in the app
  • ➖ Requires new shared dependency boundary and versioning discipline
  • ➖ Can be difficult if Server.Core types pull in server-only dependencies
2. Use local Spawn frame for launches (PTY-only) and defer hub integration
  • ➕ Simpler transport (local socket) and fewer auth/config concerns
  • ➕ Lower initial integration surface for the first UI slice
  • ➖ Cannot launch non-PTY vendors; conflicts with a multi-vendor picker
  • ➖ Would require rework when adding full vendor support later

Recommendation: Current approach is the right tradeoff for this slice: launching through the server hub matches the multi-vendor picker and uses the daemon’s runtime-factory availability as the source of truth. Keeping a small, explicit payload type plus LaunchHubJson-shared options and a pinned key-set test is a pragmatic substitute for shared contract types until a clean cross-assembly contract package exists.

Files changed (27) +2180 / -15

Enhancement (15) +714 / -12
App.axaml.csWire HomeViewModel and ServerLaunchClient into app lifecycle +54/-6

Wire HomeViewModel and ServerLaunchClient into app lifecycle

• Constructs HomeViewModel during main-window build, holds a single ServerLaunchClient instance across window rebuilds, and disposes Home + launch client on both shutdown paths.

src/Capacitor.App/App.axaml.cs

AppStateStore.csPersist harness choice per repository in AppState +6/-1

Persist harness choice per repository in AppState

• Extends AppState with HarnessByRepo (repo path → vendor token) including reserved scratch key "" and null-as-unknown semantics for “never chosen”.

src/Capacitor.App/Services/AppStateStore.cs

HostedHarnessCatalog.csBuild harness picker options from Core catalog + daemon availability +61/-0

Build harness picker options from Core catalog + daemon availability

• Derives vendor list from Capacitor.Cli.Core.Setup.HarnessCatalog.All, overlays transport family mapping, and marks availability based on daemon-advertised SupportedVendors (null means unknown ⇒ all available).

src/Capacitor.App/Services/HostedHarnessCatalog.cs

ILaunchClient.csIntroduce launch seam and source-generated hub payload type +53/-0

Introduce launch seam and source-generated hub payload type

• Defines LaunchRequest/LaunchOutcome and ILaunchClient, plus a concrete LaunchAgentRequestV2Payload with explicit snake_case JsonPropertyName attributes and a generated JsonSerializerContext.

src/Capacitor.App/Services/ILaunchClient.cs

LaunchHubJson.csCentralize SignalR JSON payload configuration +19/-0

Centralize SignalR JSON payload configuration

• Provides a single Configure method that inserts the generated TypeInfoResolver and applies SnakeCaseLower naming, shared by ServerLaunchClient and tests to prevent drift.

src/Capacitor.App/Services/LaunchHubJson.cs

ServerLaunchClient.csImplement SignalR-based session launch via RequestLaunchAgentV2 +93/-0

Implement SignalR-based session launch via RequestLaunchAgentV2

• Builds a lazy, cached HubConnection to /hubs/sessions using TokenStore access tokens, invokes RequestLaunchAgentV2, and returns server error text verbatim; includes safe disposal and concurrency gating.

src/Capacitor.App/Services/ServerLaunchClient.cs

HomeViewModel.csAdd Home view-model with per-repo harness selection and Start command +155/-0

Add Home view-model with per-repo harness selection and Start command

• Maintains SelectedRepoPath/SelectedVendor with per-repo persistence rules, projects daemon snapshots into Harnesses, projects agent cache into Sessions with UI-thread marshalling, and launches sessions through ILaunchClient.

src/Capacitor.App/ViewModels/HomeViewModel.cs

MainWindowViewModel.csExpose HomeViewModel for new Home tab +8/-1

Expose HomeViewModel for new Home tab

• Adds optional HomeViewModel property and constructor parameter so the composition root can inject the Home VM over the shared daemon client.

src/Capacitor.App/ViewModels/MainWindowViewModel.cs

SessionCardViewModel.csAdd session card view-model for Active sessions grid +58/-0

Add session card view-model for Active sessions grid

• Introduces a DTO-derived card VM with title/status/age fields and immutable status-dot brushes safe for background-thread creation.

src/Capacitor.App/ViewModels/SessionCardViewModel.cs

HomeView.axamlCreate Home tab UI (launcher + active sessions) +102/-0

Create Home tab UI (launcher + active sessions)

• Adds a scrollable Home surface with goal input, repository/harness chips, remember toggle, start button/error text, and an Active sessions ItemsControl using the new palette resources.

src/Capacitor.App/Views/HomeView.axaml

HomeView.axaml.csAdd repository picker and harness flyout behavior + converters +93/-0

Add repository picker and harness flyout behavior + converters

• Implements folder picker for repository selection, builds a MenuFlyout for harness selection with availability gating, and adds converters for repo label, harness label, and empty-session visibility.

src/Capacitor.App/Views/HomeView.axaml.cs

MainWindow.axamlAdd Home as first/default tab +5/-1

Add Home as first/default tab

• Introduces a Home TabItem before Agents/Activity and binds it to MainWindowViewModel.Home while leaving existing named controls unchanged for smoke tests.

src/Capacitor.App/Views/MainWindow.axaml

MainWindow.axaml.csUpdate Activity tab selection assumption after Home becomes default +1/-1

Update Activity tab selection assumption after Home becomes default

• Adjusts comment/documentation in code-behind to reflect Home as the initially selected tab.

src/Capacitor.App/Views/MainWindow.axaml.cs

StatusIpc.csExtend DaemonInfoDto with SupportedVendors +5/-1

Extend DaemonInfoDto with SupportedVendors

• Adds trailing SupportedVendors (string[]?) to DaemonInfoDto with null meaning “unknown/older daemon”, preserving additive DTO compatibility.

src/Capacitor.Cli.Core/LocalIpc/StatusIpc.cs

DaemonStatusIpc.csPopulate SupportedVendors in daemon status snapshots +1/-1

Populate SupportedVendors in daemon status snapshots

• Threads DaemonConfig.SupportedVendors into the DaemonInfoDto used for local status snapshot serialization.

src/Capacitor.Cli.Daemon/Services/DaemonStatusIpc.cs

Tests (9) +611 / -3
AppStateStoreTests.csTest AppState HarnessByRepo serialization and defaults +25/-0

Test AppState HarnessByRepo serialization and defaults

• Adds tests verifying per-repo harness map round-trips and that missing harness map deserializes as null (not empty).

test/Capacitor.App.Tests.Unit/AppStateStoreTests.cs

FakeDaemonClientService.csAllow tests to script SupportedVendors in snapshots +4/-2

Allow tests to script SupportedVendors in snapshots

• Extends Snap helper to accept supportedVendors and pass it into DaemonInfoDto for Home/Harness tests.

test/Capacitor.App.Tests.Unit/FakeDaemonClientService.cs

HomeViewModelTests.csAdd HomeViewModel behavioral tests +191/-0

Add HomeViewModel behavioral tests

• Covers per-repo harness persistence rules, start request contents and error handling, scratch-key isolation, and snapshot-driven harness availability updates under UI scheduler constraints.

test/Capacitor.App.Tests.Unit/HomeViewModelTests.cs

HomeViewSmokeTests.csAdd headless UI smoke tests for Home view +216/-0

Add headless UI smoke tests for Home view

• Validates named controls exist, Start enablement behavior, StartError visibility binding, session-card item count tracking, and asserts UI-thread marshalling for bound collection mutations.

test/Capacitor.App.Tests.Unit/HomeViewSmokeTests.cs

HostedHarnessCatalogTests.csTest HostedHarnessCatalog option building and transport mapping +63/-0

Test HostedHarnessCatalog option building and transport mapping

• Pins availability behavior, transport-family mapping, unknown vendor handling, and guards that every Core vendor has an explicit transport mapping.

test/Capacitor.App.Tests.Unit/HostedHarnessCatalogTests.cs

LaunchRequestTests.csPin SignalR launch payload casing and key set +67/-0

Pin SignalR launch payload casing and key set

• Serializes with the exact LaunchHubJson options, asserts sentinel model/vendor behavior, and pins the full 12-field snake_case key set to prevent silent binding regressions.

test/Capacitor.App.Tests.Unit/LaunchRequestTests.cs

MainWindowSmokeTests.csAdjust smoke tests for Home being default tab +18/-0

Adjust smoke tests for Home being default tab

• Adds a helper to select the Agents tab explicitly before asserting Agents-tab content, avoiding reliance on initial tab selection order.

test/Capacitor.App.Tests.Unit/MainWindowSmokeTests.cs

DaemonStatusDtoTests.csAdd SupportedVendors DTO round-trip/compat tests +26/-0

Add SupportedVendors DTO round-trip/compat tests

• Adds serialization round-trip coverage for SupportedVendors and asserts older snapshots without the field deserialize as null.

test/Capacitor.Cli.Core.Tests.Unit/LocalIpc/DaemonStatusDtoTests.cs

StatusIpcJsonTests.csUpdate pinned daemon status JSON to include supported_vendors:null +1/-1

Update pinned daemon status JSON to include supported_vendors:null

• Updates exact JSON snapshot expectation to account for the newly added SupportedVendors field serialized as null when absent.

test/Capacitor.Cli.Core.Tests.Unit/LocalIpc/StatusIpcJsonTests.cs

Documentation (1) +834 / -0
2026-08-23-ai2194-desktop-shell-home.mdAdd detailed implementation plan for Desktop Shell Home slice +834/-0

Add detailed implementation plan for Desktop Shell Home slice

• Introduces a step-by-step plan covering daemon vendor advertisement, per-repo harness memory, server launch transport, Home VM/view wiring, and targeted tests and AOT constraints.

docs/superpowers/plans/2026-08-23-ai2194-desktop-shell-home.md

Other (2) +21 / -0
App.axamlAdd Home design palette resources +20/-0

Add Home design palette resources

• Defines Kcap-prefixed brushes for the Home surface palette without shadowing FluentTheme defaults used by existing tabs.

src/Capacitor.App/App.axaml

Capacitor.App.csprojAdd SignalR client dependency for hub launches +1/-0

Add SignalR client dependency for hub launches

• Adds Microsoft.AspNetCore.SignalR.Client package reference (version via central package management).

src/Capacitor.App/Capacitor.App.csproj

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4cdfb8eb01

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/Capacitor.App/App.axaml.cs Outdated
// `new AppStateStore(PathHelpers.ConfigPath("app-state.json"))` already relies on. The
// composition root passes its held client so teardown can dispose it; a caller that passes
// none (a test) gets an unheld one, which owns nothing until a launch is actually made.
var home = new HomeViewModel(service, new AppStateStore(PathHelpers.ConfigPath("app-state.json")), launch ?? new ServerLaunchClient());

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Share the state store across runtime writers

This creates a second AppStateStore for the same file instead of sharing the instance created for the lifecycle graph. Because AppStateStore serializes read-modify-write operations only with its per-instance semaphore, a harness choice can overlap a lifecycle/shim/consent update, with both instances reading the old record and the last rename silently discarding the other update (for example, losing ConsentQuarantineAcked or the new HarnessByRepo entry). Pass the runtime's shared store into Home or make serialization common across instances.

Useful? React with 👍 / 👎.

<Button x:Name="StartButton" Content="Start" Command="{Binding StartCommand}"
Background="{StaticResource KcapAccentBrush}" Foreground="#07120E" FontWeight="SemiBold"
CornerRadius="7" Padding="16,7"
IsEnabled="{Binding SelectedRepoPath, Converter={x:Static StringConverters.IsNotNullOrEmpty}}" />

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Prevent launching a daemon-unavailable harness

When the daemon advertises that the selected vendor is unavailable, the flyout disables that vendor but this button remains enabled because it checks only the repository path. For example, a daemon supporting only codex plus a newly selected repository leaves the default claude selected, so clicking Start sends a launch request for a capability the daemon explicitly lacks and predictably surfaces a server rejection. Include the selected option's Available state in the command/button predicate or switch to an available selection.

Useful? React with 👍 / 👎.

@qodo-code-review

qodo-code-review Bot commented Aug 24, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Launch ignores cancellation ✓ Resolved 🐞 Bug ☼ Reliability
Description
HomeViewModel.StartAsync always calls ILaunchClient.StartAsync with CancellationToken.None, so
launches can’t be cancelled during shutdown and can race with teardown, producing spurious failures
or leaving long-running launch tasks alive after the VM is disposed.
Code

src/Capacitor.App/ViewModels/HomeViewModel.cs[R139-142]

+    async Task StartAsync() {
+        var request = new LaunchRequest(_daemon.DaemonName, SelectedRepoPath, SelectedVendor, Goal);
+        var outcome = await _launch.StartAsync(request, CancellationToken.None);
+        if (outcome.Started) {
Evidence
The PR introduces an uncancellable launch path: StartAsync uses CancellationToken.None, while app
shutdown later disposes the launch client/HubConnection, creating a race between an in-flight hub
invoke and teardown.

src/Capacitor.App/ViewModels/HomeViewModel.cs[139-147]
src/Capacitor.App/App.axaml.cs[1021-1041]
src/Capacitor.App/Services/ServerLaunchClient.cs[15-31]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`HomeViewModel.StartAsync` hardcodes `CancellationToken.None` when calling `_launch.StartAsync(...)`, so launch attempts are not cancelable and can race with app shutdown/disposal.

### Issue Context
App shutdown disposes `_home` and then disposes the shared `_launch` (and its HubConnection). If a Start is in-flight, disposing the transport mid-invoke can fault the ongoing call. The code comment explicitly says the launch transport must not be disposed while a launch is in flight, but the current implementation has no cancellation/coordination mechanism.

### Fix Focus Areas
- src/Capacitor.App/ViewModels/HomeViewModel.cs[112-148]
- src/Capacitor.App/App.axaml.cs[1021-1041]

### What to change
- Make `StartCommand` use ReactiveCommand's cancellable overload (accepting a `CancellationToken`) and pass that token through to `_launch.StartAsync(request, ct)`.
- Thread an application shutdown token into `HomeViewModel` (e.g., constructor parameter) and link it with the command token (linked CTS), so shutdown reliably cancels in-flight launches.
- Optionally disable Start while shutdown is in progress (or when `_daemon.DaemonName` is empty / disconnected) to avoid starting operations that will be immediately torn down.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Linear IDs in comments ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
Source/XAML comments added in this PR include Linear issue identifiers (e.g., AI-2194, AI-2171).
This violates the requirement to avoid Linear coupling in code comments and should be replaced with
non-Linear references (or removed).
Code

src/Capacitor.App/App.axaml.cs[R68-71]

+    // Constructed INSIDE BuildAndShowMainWindow (Task 6, AI-2194), over the same `service`
+    // instance MainWindowViewModel itself uses — retrieved back off the built window's own
+    // DataContext right below, so this field (and therefore disposal) never needs a second
+    // construction path or a signature change to BuildAndShowMainWindow (AppStartupTests calls
Evidence
PR Compliance ID 24 prohibits Linear issue identifiers in source/config comments. The added comments
in multiple files explicitly contain AI-2171/AI-2194, demonstrating the violation.

CLAUDE.md: Do Not Use Linear Issue Numbers in Source Comments
src/Capacitor.App/App.axaml[6-9]
src/Capacitor.App/App.axaml.cs[68-72]
src/Capacitor.App/ViewModels/HomeViewModel.cs[13-20]
test/Capacitor.App.Tests.Unit/MainWindowSmokeTests.cs[29-31]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New comments in source/XAML/test code include Linear issue identifiers (e.g., `AI-2194`, `AI-2171`), which is disallowed.

## Issue Context
Compliance requires avoiding Linear identifiers in source/config comments to prevent long-lived coupling to Linear. If an issue/design reference is needed, use a GitHub issue number; otherwise, reword to a timeless description or point to a non-Linear doc path without embedding the Linear ID.

## Fix Focus Areas
- src/Capacitor.App/App.axaml[6-9]
- src/Capacitor.App/App.axaml.cs[68-72]
- src/Capacitor.App/ViewModels/HomeViewModel.cs[13-20]
- test/Capacitor.App.Tests.Unit/MainWindowSmokeTests.cs[29-31]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Repo key casing mismatch ✓ Resolved 🐞 Bug ≡ Correctness
Description
HarnessByRepo is keyed by absolute repo path, but HomeViewModel persists and restores using
case-sensitive dictionary semantics; on case-insensitive filesystems (Windows/macOS default), the
same path with different casing won’t match and the stored harness won’t be restored.
Code

src/Capacitor.App/ViewModels/HomeViewModel.cs[R150-153]

+    static IReadOnlyDictionary<string, string> WithEntry(IReadOnlyDictionary<string, string>? existing, string key, string value) {
+        var next = existing is null ? new Dictionary<string, string>() : new Dictionary<string, string>(existing);
+        next[key] = value;
+        return next;
Evidence
The PR introduces HarnessByRepo persistence and lookup, but the helper that clones/builds the
dictionary uses default (case-sensitive) semantics and lookup uses TryGetValue with the provided
repoPath, so key mismatches occur when casing differs.

src/Capacitor.App/Services/AppStateStore.cs[9-18]
src/Capacitor.App/ViewModels/HomeViewModel.cs[131-137]
src/Capacitor.App/ViewModels/HomeViewModel.cs[150-154]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`HarnessByRepo` is keyed by absolute repo path, but the code writes/reads those keys with default case-sensitive `Dictionary<string,string>` behavior. This can cause a stored selection to be missed when the same repository path is later returned with different casing.

### Issue Context
- `WithEntry(...)` constructs `new Dictionary<string,string>(existing)` and later `TryGetValue(repoPath, ...)` is used.
- On Windows (and often macOS), filesystem paths are effectively case-insensitive, so casing differences are common.

### Fix Focus Areas
- src/Capacitor.App/ViewModels/HomeViewModel.cs[131-137]
- src/Capacitor.App/ViewModels/HomeViewModel.cs[150-154]
- src/Capacitor.App/Services/AppStateStore.cs[9-18]

### What to change
- Normalize repo path keys before storing/lookup (e.g., `Path.GetFullPath(repoPath)` and on Windows consider a canonical casing strategy).
- Use a consistent comparer for the in-memory dictionary used for `HarnessByRepo` (e.g., `StringComparer.OrdinalIgnoreCase` on Windows only), and ensure `WithEntry` preserves that comparer when cloning.
- Add/adjust a unit test to cover differing casing of the same path resulting in correct restore.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Hub recreated while in use ✓ Resolved 🐞 Bug ☼ Reliability
Description
ServerLaunchClient can dispose and recreate the shared HubConnection in GetConnectionAsync whenever
it’s not Connected, but StartAsync does not hold the gate during InvokeAsync; concurrent launches
can end up disposing a HubConnection that another StartAsync is still using.
Code

src/Capacitor.App/Services/ServerLaunchClient.cs[R36-41]

+            if (_hub is { State: HubConnectionState.Connected }) return _hub;
+
+            if (_hub is not null) {
+                await _hub.DisposeAsync();
+                _hub = null;
+            }
Evidence
The PR adds a shared HubConnection with logic to dispose/recreate it under the gate, but StartAsync
uses the connection outside the gate, so another StartAsync/refresh path can dispose that same
instance mid-invoke.

src/Capacitor.App/Services/ServerLaunchClient.cs[15-30]
src/Capacitor.App/Services/ServerLaunchClient.cs[33-41]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ServerLaunchClient.GetConnectionAsync` disposes `_hub` and rebuilds it when `_hub.State != Connected`, but `StartAsync` releases the `_gate` before calling `hub.InvokeAsync(...)`. Under concurrency, one launch can be invoking while another decides the hub is non-connected and disposes it.

### Issue Context
Even if the UI typically issues one Start at a time, this is a real race if multiple starts are triggered (double-click, automation, future batching), or if connection state transitions mid-invoke.

### Fix Focus Areas
- src/Capacitor.App/Services/ServerLaunchClient.cs[15-30]
- src/Capacitor.App/Services/ServerLaunchClient.cs[33-76]

### What to change
- Option A (simple/robust): serialize the entire launch call by holding `_gate` across both `GetConnectionAsync` and the `InvokeAsync` call.
- Option B: keep a separate in-flight counter/refcount so `_hub` is never disposed while there are active invocations.
- Option C: enable SignalR automatic reconnect and avoid disposing `_hub` on transient disconnects; instead, await reconnection or let invoke fail and retry with a new hub.
- Add a unit test (or targeted concurrency test) that runs two concurrent StartAsync calls and asserts no ObjectDisposedException/invalid state faults are produced by internal disposal.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View medium (1)
5. JsonValueKind checked directly ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
LaunchRequestTests asserts JSON null-ness by checking JsonElement.ValueKind directly instead of
using the standard JsonElementExtensions-style accessors. This violates the JSON inspection
standardization rule and can lead to inconsistent JSON handling patterns.
Code

test/Capacitor.App.Tests.Unit/LaunchRequestTests.cs[R50-52]

+        var json = Payload(new LaunchRequest("kcap-dev", "/repo", "claude", "   "));
+        await Assert.That(json.GetProperty("prompt").ValueKind).IsEqualTo(JsonValueKind.Null);
+    }
Evidence
PR Compliance ID 22 forbids direct JsonElement.ValueKind checks where the standardized helpers
should be used. The added test performs a direct ValueKind comparison on prompt, matching the
rule’s failure criteria.

CLAUDE.md: Use JsonElementExtensions Instead of Checking JSON ValueKind Directly
test/Capacitor.App.Tests.Unit/LaunchRequestTests.cs[49-52]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new test checks `json.GetProperty("prompt").ValueKind == JsonValueKind.Null` directly.

## Issue Context
Compliance requires using the repository’s standardized JSON inspection helpers (`JsonElementExtensions`) rather than direct `ValueKind` comparisons.

## Fix Focus Areas
- test/Capacitor.App.Tests.Unit/LaunchRequestTests.cs[49-52]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/Capacitor.App/App.axaml.cs Outdated
Comment thread test/Capacitor.App.Tests.Unit/LaunchRequestTests.cs
Comment thread src/Capacitor.App/ViewModels/HomeViewModel.cs
Comment thread src/Capacitor.App/ViewModels/HomeViewModel.cs
Comment thread src/Capacitor.App/Services/ServerLaunchClient.cs Outdated
@alexeyzimarev

Copy link
Copy Markdown
Member Author

Note for reviewers: the Active session cards on Home are display-only on purpose — no click handler, no command. Opening a session needs the session workspace, which is AI-2195 (declared blocked_by this issue), so in this slice there is nowhere for a click to go.

Wiring the two actions AgentActionService already exposes (OpenInWeb, RequestStop) onto each card was considered as an interim affordance and declined: it commits the card's click gesture to a meaning AI-2195 wants to take over. Rationale and the follow-ups are recorded on AI-2195.

So an inert card is expected behaviour here, not a defect.

- Thread the shutdown token into HomeViewModel.StartAsync; a launch held
  CancellationToken.None and could not be cancelled against teardown.
- Hold the connection gate across the hub invoke. GetConnectionAsync
  disposes and rebuilds a connection that is not Connected, so releasing
  before the invoke let a second launch dispose one still in use.
- Compare repo keys the way the filesystem does: case-insensitive on
  Windows and macOS, case-sensitive on Linux. Applied on read, since
  System.Text.Json rebuilds the dictionary with an ordinal comparer.
- Assert JSON null through JsonElementExtensions.IsNull rather than
  reading ValueKind directly.
- Remove Linear issue IDs from source comments (CI gate).
@alexeyzimarev

Copy link
Copy Markdown
Member Author

All five review findings verified against the code and fixed in 11930d79.

Finding Disposition
Launch ignores cancellation Fixed. HomeViewModel now takes the app's shutdown token and passes it to ILaunchClient.StartAsync. This was also the upstream cause of a shutdown race already noted during review.
Linear IDs in comments Fixed — 12 occurrences removed; scripts/check-linear-ids.sh passes locally. Also removed references to internal plan artifacts ("Task 5", "task-6-brief") that a future reader cannot resolve.
JsonValueKind checked directly Fixed via JsonElementExtensions.IsNull. I checked first whether a defensive accessor would weaken a wire-format assertion — IsNull is a direct kind check, so it does not, and GetProperty still distinguishes absent from explicit null.
Repo key casing mismatch Fixed, with one correction to the suggestion: a blanket case-insensitive comparer would be wrong on Linux, where two paths differing only in case are genuinely different repositories. Now OrdinalIgnoreCase on Windows/macOS, Ordinal on Linux, applied on read because System.Text.Json rebuilds the dictionary with a default comparer on load.
Hub recreated while in use Fixed. StartAsync now holds the connection gate across the invoke, so a second launch cannot dispose a connection the first is mid-invoke on. Launches are a per-click UI action, so serializing them costs nothing.

The casing fix carries a test that asserts the platform's own answer rather than one hardcoded expectation, so it is meaningful on every CI leg. Verified it fails without the fix: reverting the comparer to Ordinal turns it red, restoring it turns it green.

App suite 1170/1170.

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.

1 participant