Repository navigation
Retire elicitation cards when the question is answered - #1116
alexeyzimarev merged 2 commits into
Conversation
The permission hook carries no tool-use id, so the card is matched on question text to the latest ask, and only after that ask has a result.
|
@codex review |
![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: ebbe42e63e
ℹ️ 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".
| AskedQuestion? latest = null; | ||
| foreach (var ask in _questionAsks) | ||
| if (request.OverlapsQuestion(ask.Fingerprints)) latest = ask; | ||
| return latest is { SettledAt: { } settled } && request.RequestedAt <= settled; |
There was a problem hiding this comment.
When a structurally accepted PermissionPendingDto has a nonempty but unparsable requested_at value, PendingPermissionRequest converts it to DateTimeOffset.MinValue (the shared PermissionWire.IsPendingStructurallyValid only requires that the field be nonempty). This comparison then treats the request as older than every settled matching question, so a current card with the same text can be withdrawn—and, for a server card, tombstoned—despite its actual ordering being unknown. Skip the text-based retirement when the request timestamp cannot be parsed rather than treating it as the oldest possible request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
An unparsed requested_at becomes DateTimeOffset.MinValue, and text matching treated that as older than the settled ask. A card with that stamp is now left in place. Covered by A_question_whose_time_did_not_parse_is_not_retired_by_a_settled_ask.
An unparsed requested_at is the minimum timestamp, which would look older than every settled ask of the same question.
PR Summary by QodoRetire answered elicitation cards without tool-use IDs
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1. Clock skew retires live questions
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| internal static HashSet<string>? QuestionTexts(string? toolInputJson) { | ||
| if (ClaudeElicitation.TryParse(toolInputJson) is not { } parsed) return null; |
There was a problem hiding this comment.
2. Claude parsing bypasses its harness 📘 Rule violation ⌂ Architecture
QuestionTexts directly invokes ClaudeElicitation.TryParse, while Apply recognizes the same vendor tool through ClaudeElicitation.ToolName in the desktop app assembly. When Claude's question format or matching semantics change, maintainers must update both Capacitor.App and the designated Claude harness instead of keeping vendor behavior within the established isolation boundary.
Agent Prompt
## Issue description
Claude-specific AskUserQuestion parsing and matching were added directly to the desktop app, outside the repository's designated vendor harness structure.
## Fix Focus Areas
- src/Capacitor.App/Services/PendingPermissionRequest.cs[119-140]
- src/Capacitor.App/ViewModels/ChatTabViewModel.cs[768-770]
- src/Capacitor.App/ViewModels/ChatTabViewModel.cs[893-923]
- src/Capacitor.Cli.Core/Harness/Claude/ClaudeChatRules.cs[8-18]
## Recommended Fix
Move Claude tool recognition, payload parsing, and question fingerprint extraction behind a vendor-neutral interface implemented under `Capacitor.Cli.Core/Harness/Claude`. Keep only generic reconciliation state and withdrawal orchestration in `Capacitor.App`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
QuestionTexts calls ClaudeElicitation.TryParse, which is already how the desktop reads an AskUserQuestion card. A new interface would move that existing parser.
| lock (_lock) { | ||
| if (!_disposed) ConcludeServerKey(target.RequestId); | ||
| } |
There was a problem hiding this comment.
3. Restarts hide new permission requests 🔗 Cross-repo conflict ≡ Correctness
WithdrawAsync calls ConcludeServerKey, permanently tombstoning an opaque server request ID even though kcap-server generates that ID from a process-local counter. When kcap-server restarts and reuses the same session-and-counter ID, UpsertServer also rejects the new actionable request, so the desktop never presents it.
Agent Prompt
## Issue description
Server-lane withdrawal permanently tombstones request IDs, but kcap-server can reuse those IDs after its in-memory tracker restarts, causing later valid requests to be suppressed.
## Fix Focus Areas
- src/Capacitor.App/Services/PermissionService.cs[133-141]
- src/Capacitor.App/Services/PermissionService.cs[335-344]
- /cross_repos/kcap-server/src/Capacitor.Server/PermissionRequestTracker.cs[12-33]
## Recommended Fix
Coordinate with kcap-server to make permission request IDs globally unique across process lifetimes, such as by including a random or startup-epoch component. Until that contract is available, do not retain server-lane tombstones beyond a server lifecycle boundary.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
The id is session plus a counter that starts over when the server process does. ConcludeServerKey already tombstones that id when a server card is answered or the server reports it settled. This withdraw uses the same key so a replay of the declined question stays dropped. Dropping the tombstone would show the card again. An id that survives a restart is a server change, and it already applies to every concluded request.
| foreach (var ask in _questionAsks) | ||
| if (request.OverlapsQuestion(ask.Fingerprints)) latest = ask; | ||
| return latest is { SettledAt: { } settled } && request.RequestedAt <= settled; |
There was a problem hiding this comment.
1. Clock skew retires live questions 🔗 Cross-repo conflict ≡ Correctness
ShouldWithdraw compares a transcript result's SettledAt timestamp with RequestedAt, even though live server requests synthesize RequestedAt from the desktop's receipt time rather than the question's authoritative creation time. Clock skew or notification delay can invert the ordering, so a matching same-text card may be removed before its tool call reaches the transcript feed or remain open after its result has already arrived.
Agent Prompt
## Issue description
Server permission pushes do not include the question's creation timestamp, so server-lane requests use the desktop's local receipt time as `RequestedAt` and compare it with a transcript result timestamp from another producer. Clock skew or delayed delivery can therefore cause incorrect retirement ordering.
## Fix Focus Areas
- src/Capacitor.App/ViewModels/ChatTabViewModel.cs[893-917]
- src/Capacitor.App/Services/ServerPermissionFeed.cs[28-44]
- src/Capacitor.App/Services/ServerConnectionService.cs[262-264]
- src/Capacitor.App/Services/PendingPermissionRequest.cs[55-64]
- /cross_repos/kcap-server/src/Capacitor.Server/Hooks/SessionHookHandlers.cs[2216-2221]
## Recommended Fix
Extend kcap-server's live permission and elicitation events with an authoritative request creation timestamp generated on the transcript timeline, carry it through `ServerPermissionRequest`, and pass it into `PendingPermissionRequest.FromServer` instead of using `TimeProvider.GetUtcNow()`. Use that timestamp for result ordering, and treat pushes from older servers that omit the field as unordered rather than withdrawing them based on client receipt time; only compare ordering when both timestamps originate from the request and transcript event timeline.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
A local card takes requested_at from the hook, on the same machine as the transcript. A stamp that did not parse is already left in place. A server push has no creation time, so its stamp is the desktop receipt time. A receipt after the result fails the comparison and the card stays; the server responded event still concludes that id. A receipt before the result is the card that was on screen when the question was answered.
2 participants
Closes #1114 — AI-2899
What & why
Sending a message instead of answering AskUserQuestion leaves the desktop question card up, including a second copy of the same question. The permission hook carries no tool-use id, so the card never matched the tool result that records the decline. The chat now retires a question card whose text matches the latest settled ask. A server-lane twin has no withdraw frame, so it is dropped locally. A later ask of the same text stays until its own result.
Where to look
A card requested after the result is left alone, and an open second ask is not cleared by the earlier result. A prompt that is not a question, and has no tool-use id, is still left alone.
Verification
dotnet run --project test/Capacitor.App.Tests.Unit/Capacitor.App.Tests.Unit.csproj -- --treenode-filter "/*/*/ChatTabViewModelTests/*"— 73 passed.dotnet run --project test/Capacitor.App.Tests.Unit/Capacitor.App.Tests.Unit.csproj -- --treenode-filter "/*/*/PermissionServiceTests/*"— 43 passed.