Repository navigation
Stop remote agents and answer their prompts from the desktop app - #874
Merged
Merged
Conversation
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The socket that could answer those cards is gone; the server twins they were shadowing carry handles that still work. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Access transitions publish under the access service's lock, so both branches hand off to the pool before touching the permission cache's own lock. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…805) A pending ping names no request id, so the set is filled by reconciling the session's stream headlessly; a response naming an id the set never held re-reconciles rather than trusting the count. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Access is the server's own verdict, so an empty pane can only ever mean no cards. The three card templates now live in one application-level dictionary, so the chat pane and this host cannot drift apart. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The server lane is built before the action service because a remote stop goes over the hub, and the permission service after the directory because a server-lane item names a session that only the directory's map turns into an agent. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The origin lookup and the workspace factory are two reads of a cache the directory recomputes on background threads, so both have to tolerate the row being gone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…805) A missing hub method, a serialization mismatch and a persistent auth fault all rendered as the lane being down. The reason is remembered per entry so the retry ladder does not restate it every few seconds. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Immutable snapshots keep IndexOf and Count at the call sites, so the reading tests are unchanged; a plain List both dropped entries and threw mid-enumeration. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The elicitation push bound its options to a record with required members, so one option missing an id dropped the whole invocation; it is parsed leniently now, the way the permission push already was. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
PR Summary by QodoControl remote agents and answer prompts from the desktop
AI Description
Diagram
High-Level Assessment
Files changed (100)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df18f735b1
ℹ️ 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 on lines
+338
to
+342
| internal void DropServerForSession(string sessionId) { | ||
| lock (_lock) { | ||
| if (_disposed) return; | ||
| foreach (var item in _cache.Items.Where(i => i.Lane == PermissionLane.Server && i.SessionId == sessionId).ToList()) _cache.Remove(item.Key); | ||
| foreach (var key in _shadowed.Where(kv => kv.Value.SessionId == sessionId).Select(kv => kv.Key).ToList()) _shadowed.Remove(key); |
There was a problem hiding this comment.
When a SessionAccessChanged recheck denies access while an earlier Established reconciliation is still fetching details, this method removes the cards without advancing _sessionGenerations. The delayed fetch can therefore pass the generation check in ReplaceServerForSession and restore the revoked session's prompts, including their tool input, after the UI has entered the access-lost state. Increment the session generation as part of this drop so every pre-denial fetch is discarded.
Useful? React with 👍 / 👎.
Comment on lines
+328
to
+330
| var keep = items.Select(i => i.Key).ToHashSet(StringComparer.Ordinal); | ||
| foreach (var stale in _cache.Items.Where(i => i.Lane == PermissionLane.Server && i.SessionId == sessionId && !keep.Contains(i.Key)).ToList()) | ||
| _cache.Remove(stale.Key); |
There was a problem hiding this comment.
If a PermissionRequested push arrives after the detail endpoint has taken its snapshot but before ReconcileAsync applies that snapshot, the pushed item is absent from keep and is removed here. The org-wide PermissionPending ping only drives SessionAttentionTracker, so no subsequent event is guaranteed to restore the card, leaving a genuinely pending prompt unavailable in the desktop app. The replacement needs an event/version boundary or must retain live upserts newer than the fetched snapshot.
Useful? React with 👍 / 👎.
Comment on lines
+91
to
+94
| foreach (var change in changes) { | ||
| if (change.Key != row.Key) continue; | ||
| if (change.Reason == ChangeReason.Remove) { SessionEnded = true; Release(); continue; } | ||
| Apply(change.Current); |
There was a problem hiding this comment.
When the local daemon reconnects and AgentDirectory proves that this remote row is its twin, the directory removes remote:{id} and exposes local:{id} even though the logical agent is still running. Treating that removal as an ended session leaves the open pane falsely ended; clicking the new local rail row cannot recover it because MainWindowViewModel.OpenSession returns early for the same agent ID. The workspace must distinguish an origin transition from a logical-agent removal and switch to the winning row.
Useful? React with 👍 / 👎.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+82
to
+85
| obj is { ValueKind: JsonValueKind.Object } o ? (o.Prop(snake) ?? o.Prop(camel)) is { ValueKind: not JsonValueKind.Null } e ? e : null : null; | ||
| static string? Read(JsonElement obj, string snake, string camel) => Elem(obj, snake, camel) is { ValueKind: JsonValueKind.String } e ? e.GetString() : null; | ||
| static bool? Bool(JsonElement obj, string snake, string camel) => Elem(obj, snake, camel) is { } e && e.ValueKind is JsonValueKind.True or JsonValueKind.False ? e.GetBoolean() : null; | ||
| static int? Int(JsonElement obj, string snake, string camel) => Elem(obj, snake, camel) is { ValueKind: JsonValueKind.Number } e && e.TryGetInt32(out var i) ? i : null; |
There was a problem hiding this comment.
13. Interrupt parsing bypasses helpers 📘 Rule violation ⚙ Maintainability
Options, Elem, Read, and Int classify JSON values through ValueKind property patterns instead of the shared shape helpers. Reconciliation of session-detail events reaches these checks for arrays, objects, nulls, strings, and numbers.
Agent Prompt
## Issue description
Interrupt reconciliation reimplements JSON shape classification with direct `ValueKind` patterns.
## Fix Focus Areas
- src/Capacitor.App/Services/InterruptReconciliation.cs[68-85]
## Recommended Fix
Use `IsArray`, `IsObject`, `IsNull`, `IsString`, and `IsNumber` for the corresponding checks; add and use a shared boolean helper if needed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+73
to
+74
| /// Behaviors the permission-response route accepts. | ||
| public static class PermissionBehaviors { |
There was a problem hiding this comment.
14. Permission constants share a source file 📘 Rule violation ⚙ Maintainability
PermissionBehaviors is added as another top-level type in RemoteWire.cs, which already contains WireTokens and other wire types. Later changes to permission behavior constants therefore extend an aggregate file rather than a source file named for their primary type.
Agent Prompt
## Issue description
The new permission behavior type is another primary top-level type in an existing aggregate source file.
## Fix Focus Areas
- src/Capacitor.Remote.Models/RemoteWire.cs[73-77]
## Recommended Fix
Move `PermissionBehaviors` into a new `PermissionBehaviors.cs` file in the same namespace.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+8
to
+10
| public enum PendingInterruptKind { ClaudePermission, AcpPermission, AcpQuestion, TranscriptQuestion } | ||
|
|
||
| public sealed record PendingInterrupt( |
There was a problem hiding this comment.
24. Interrupt types share one file 📘 Rule violation ⚙ Maintainability
PendingInterruptKind and PendingInterrupt are both public top-level types in PendingInterrupt.cs. The enum and record are independently reusable declarations rather than tiny implementations of a private hierarchy.
Agent Prompt
## Issue description
The pending-interrupt source file declares a public enum alongside its public record.
## Fix Focus Areas
- src/Capacitor.App/Services/PendingInterrupt.cs[8-15]
## Recommended Fix
Move `PendingInterruptKind` into `PendingInterruptKind.cs` and retain `PendingInterrupt` in the existing file.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+14
to
+16
| /// Builds the app's real authenticated HTTP lane against a WireMock server. | ||
| static class WireMockLane { | ||
| public static async Task<(ICapacitorHttpClient Http, ProfileContext Profiles, ServiceProvider Provider)> BuildAsync(WireMockServer server, ConfigRoot root) { |
There was a problem hiding this comment.
15. Server test helper shares the test file 📘 Rule violation ⚙ Maintainability
WireMockLane is introduced as a top-level helper beside ServerSessionHttpTests in the same source file. It is not nested, private, part of a small type hierarchy, or a descriptor used only by a registry.
Agent Prompt
## Issue description
The new server-session test file contains both a top-level HTTP fixture helper and the test class.
## Fix Focus Areas
- test/Capacitor.App.Tests.Unit/ServerSessionHttpTests.cs[14-29]
## Recommended Fix
Move `WireMockLane` into `WireMockLane.cs`, or make it a private nested helper inside the test class.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+8
to
+10
| /// ServerRequestId is the server's id for the same request once the daemon's server leg holds | ||
| /// one — null until then, and always null from a daemon that predates it — so a client hearing | ||
| /// both lanes can pair the two copies without guessing. |
There was a problem hiding this comment.
16. Compatibility comment narrates history 📘 Rule violation ⚙ Maintainability
The PermissionPendingDto documentation says ServerRequestId is null for a daemon that “predates” the field. The compatibility requirement is understandable as a nullable wire contract without describing the relative age of daemon versions.
Agent Prompt
## Issue description
The new wire comment uses historical narration about daemons that predate the field.
## Fix Focus Areas
- src/Capacitor.Cli.Core/LocalIpc/PermissionIpc.cs[8-10]
## Recommended Fix
Rewrite the comment in present tense, stating which currently supported daemon payloads can omit the field and why consumers must accept null.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| RemoteModelsJsonContext.Default.SessionDetailDto)!; | ||
| await Assert.That(detail.LastEventNumber).IsEqualTo(3L); | ||
| await Assert.That(detail.Events![0].EventType).IsEqualTo("InterruptIssued"); | ||
| await Assert.That(detail.Events[0].Payload!.Value.GetProperty("kind").GetString()).IsEqualTo("permission"); |
There was a problem hiding this comment.
11. Remote model tests fail to build 🐞 Bug ≡ Correctness
Session_detail_reads_events_with_payload_and_data dereferences the nullable SessionDetailDto.Events property without a null-forgiving operator on its final assertion. Nullable analysis emits CS8602 for this expression, and the repository promotes warnings to errors, so the Remote.Models unit-test project cannot compile.
Agent Prompt
Issue description
`SessionDetailDto.Events` is declared nullable, but the final assertion in the new contract test dereferences it without null suppression. With nullable warnings treated as errors, this produces CS8602 and prevents the test project from building.
Fix Focus Areas
- test/Capacitor.Remote.Models.Tests.Unit/PermissionWireTests[29-30]
Recommended Fix
Use `detail.Events![0]` in the final assertion as well, or assign `detail.Events!` to a local non-null variable before both assertions.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+150
to
+152
| case ServerRespondKind.NotPending: | ||
| lock (_lock) { if (!_disposed) ConcludeServerKey(target.RequestId); } | ||
| return new(PermissionResolveKind.AlreadyDecided, null); |
There was a problem hiding this comment.
12. Shared viewers lose pending prompts 🔗 Cross-repo conflict ≡ Correctness
SendServerAsync treats every response-route 404 as an already-decided request and removes the card, although kcap-server also returns 404 when a Full-access viewer is not the session owner. When a non-owner opens a Full-shared session, the server delivers permission and elicitation pushes to that viewer, but answering one removes the desktop card while leaving the underlying agent request pending.
Agent Prompt
## Issue description
Full-access non-owner viewers can subscribe to session prompts, but kcap-server's permission-response endpoint only accepts the session owner and disguises that refusal as 404. The desktop interprets 404 as an already-settled request and removes the card even though the agent remains blocked.
## Fix Focus Areas
- src/Capacitor.App/Services/PermissionService.cs[144-152]
- src/Capacitor.App/Services/ServerPermissionFeed.cs[30-35]
- /cross_repos/kcap-server/src/Capacitor.Server/Sessions/SessionHookHandlers.cs[2403-2405]
- /cross_repos/kcap-server/src/Capacitor.Server/Sessions/CapacitorHub.cs[5716-5725]
## Recommended Fix
Coordinate the authorization contract between the repositories. Either allow Full-access viewers who receive prompt payloads to use the response endpoint, or restrict prompt subscription and delivery to owners and expose that denial to the desktop; also ensure an authorization refusal is distinguishable from a genuinely settled request so the desktop does not discard a still-pending card.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
A row removed because the local daemon proved the twin is not an ended session: the host reports the origin change instead, and the window opens the local workspace for the same id. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The two lanes allocate agent ids independently, so one id on each is two agents: the in-flight stop set and the tray's command parameter are keyed by lane, and a rail click names the lane of the row it came from. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The fakes' session map and withdraw now follow the production rules they stand in for: a duplicate session id resolves local instead of throwing, and a server-lane withdraw is refused. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A session with no card, no shadow and no generation of its own had nothing for the clear to move, so the epoch retires every fetch in flight at once. Only the reconciliation is gated on it: a live push already belongs to the new subject. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The tray reads the lane from the clicked key rather than from an entry that may already be gone, a host whose row moved to this machine stops offering cards and Stop, and a set that outlived a disconnect stays marked owed so a response cannot strand it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
An ACP-hosted agent's question reaches the app only over the server lane, in its session's chat group, so a local workspace needs that lease as well. A local agent the server never registered is refused the watch; nothing here reads the verdict, which is what keeps the refusal invisible. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Cross-lane dedup fails open, so a same-id local/remote pair is two unrelated agents. Filtering on the agent id alone put one lane's card under the other's header, where answering it would act on a process the user never opened. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A same-user reconnect moves neither the session generation nor the lane epoch, and the live sequence only protects newer additions, so an older fetch still in flight could re-add a card the reconnect's own reconciliation had proved settled. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The chat group is shared per session, so a superseded-but-successful attempt handing it back dropped the membership the reopened lease was relying on: Established, no retry armed, and no payloads arriving. Attempt numbering guards the published verdict, not subscription ownership. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Cross-lane dedup fails open, so a same-id local row can hold an unrelated agent. Treating its mere presence as proof told the user this agent was now hosted locally and pointed them at that other process; the session id is the evidence already carried on both rows. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The live factory parses the elicitation payload and the reconciled arm did not, so a question raised before its workspace joined would restore as a generic Allow/Deny and settle carrying no updated_input at all. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The daemon accepts an empty-string enum value as an offered id and resolves the answer by exact match, so rejecting it degraded a live selection question into a free-text card whose answer cannot satisfy that contract. Only an absent or null id makes an option unusable. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The lane stays Connected across a re-auth, so reading its status as a connected boolean swallowed the subject change whole: the previous account's session sets and published pips survived, and the fetch it had started went on filling them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The origin-change routing test modelled a twin with no session id on either row, which is no longer evidence of one. RemoteHost.New keeps a one-argument overload rather than an optional parameter: the factory takes it as a method group, and that conversion needs an exact signature. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…dering-dusk main's new PR-context smoke test reads members ISessionWorkspace deliberately does not expose, so the call site narrows to WorkspaceViewModel rather than the interface widening. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Testing ownership and calling the hub were two steps: a replacement attempt claimed and joined the group between them, and the superseded attempt then handed back the membership the live lease was receiving payloads on. The gate is held across the hub call and never with the state lock. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Testing the attempt and committing to the cache were two steps: a reconnect established a newer attempt between them, and the older fetch handed back the request that attempt's own reconciliation had just proved gone. The gate spans the test and the commit, never the fetch. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The denial dispatched an unconditional drop. Delayed behind the grant that replaced it, that drop deleted the cards the grant's own reconciliation had proved pending, or moved the cache generation under that fetch and made its valid result be discarded. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A session id is unique only within one server, and Claude's derive from transcript filenames that imports preserve, so matching a server-lane item on the id alone let a local workspace show another server's prompts and answer its process over HTTP. The directory already proves both facts, so the view models consume its evidence rather than re-deriving identity. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…dering-dusk Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The scope flag and the session id reach the filter from the directory's recompute and the daemon pump, so a predicate-driven add or remove ran Transform and SortAndBind on those threads, mutating a bound collection and the chat tab's request map off the UI thread. Scheduling only the cache input leaves that path uncovered. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The bump stays where the transition is observed: that arrival order is what makes an older attempt stale, and moving it into the gate would let queue order redefine which verdict is newer. Gating the capture is what closes the window — a drop landing between a grant's marker and its commit left the cache rejecting a result whose attempt was still current. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A session id is unique only within one server, so while the local daemon reports another one its rows name different sessions: the lookups stamped a server session's cards and its rail pip onto an unrelated local agent. The flag is an input to the map rather than only to its consumers, so a flip republishes it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A takeover verdict needs current local authority — the socket up and the agent still live on it — because a server verdict that ends the session retires the remote row exactly as a takeover does. The pane keeps the ended verdict it was given rather than reporting a move to a process that is no longer running the session. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…dering-dusk Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This was referenced Sep 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #805 — AI-2553
What & why
Agents on the user's other machines are visible in the desktop app but read-only: a remote session's prompts can only be answered in the web UI, and a remote agent cannot be stopped. The app now stops a remote agent through the hub and answers permission prompts and questions over the server's permission-response route, each on the lane that delivered it, while the local socket keeps answering the local daemon's own. The daemon publishes its local↔server request-id mapping on the local permission wire, so a prompt heard on both lanes renders as one card and, without proof of correlation, as two — never zero. Local ACP-hosted agents' questions, which only the server carries, render too. Opening a remote row shows a card host with the session's authorization lifecycle: access watch, chat join, reconnect re-check, revocation.
Where to look
PermissionService's lane-scoped cache: a local settlement retires its claimed server twin, and a lost daemon subscription drops local cards so their answerable twins surface.docs/CHANGES.mdrecords the three invariants. Cold-start pips for a session never opened still need the server's pending-interrupts seed (AI-2537).Verification
App 1692/1692; Cli.Core 3162 passed, 9 skipped; Remote.Models 7/7; Daemon 3055 passed, 38 skipped, 1 failed (
CodexAppServerSchemaConformanceTests: installed codex 0.154.0 vs the vendored pin; the diff touches no daemon file it reads).dotnet publish src/Capacitor.Cli/Capacitor.Cli.csproj -c Release 2>&1 | grep -E 'IL[23][01][0-9]{2}'prints nothing.