Skip to content

Step through desktop question cards and retire them on a terminal answer - #770

Merged
alexeyzimarev merged 3 commits into
mainfrom
desktop-question-stepper
Sep 5, 2026
Merged

alexeyzimarev merged 3 commits into
mainfrom
desktop-question-stepper

Conversation

@alexeyzimarev

Copy link
Copy Markdown
Member

Closes #769 — AI-2497

What & why

The NEEDS YOU question card listed every question of a series at once, a pick showed no selected state, the text was small and dim, and a question answered in the terminal tab stayed on screen until the agent exited. The card walks a series one question at a time with step chips and a Review step that submits, option chrome lives in class styles so the selected state paints, and the Chat tab withdraws a pending request through a withdraw decision on the existing resolve frame once the transcript carries the tool's result — the only completion signal there is, since the plugin registers no PostToolUse hook.

Where to look

The daemon answers a withdrawn hook with a deny, never an allow: the tool already ran, so an allow could only apply to a later call. An older daemon rejects withdraw, and the app concludes the entry locally on that ack, so the card still goes.

Verification

  • Capacitor.App.Tests.Unit full suite: 1337 passed, 0 failed.
  • Daemon Permission* and LocalPermissionBridge* classes: 104 passed; Core Permission* classes: 17 passed.
  • dotnet build src/Capacitor.App --no-incremental: 0 warnings. dotnet publish src/Capacitor.Cli -c Release: no IL2026/IL3050.
  • Headless smoke tests click an option and assert its border is the accent resource, and walk a two-question series through to a disabled Submit on the Review step.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 4, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-04T15:16:05.066966Z ff35dad PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Step through desktop questions and withdraw settled requests

✨ Enhancement 🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Walks multi-question cards through individual questions, review, editing, and final submission.
• Makes selected options visibly distinct with larger, clearer card styling.
• Withdraws stale permission requests when matching transcript tool results arrive.
Diagram

graph TD
  R["Pending requests"] --> C["Chat tab"] --> Q["Question stepper"] --> V["Question card UI"]
  T["Transcript results"] --> C --> S["Permission service"] --> I["Resolve IPC"] --> D["Daemon broker"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Register a PostToolUse hook
  • ➕ Provides an explicit tool-completion event at the plugin boundary.
  • ➕ Avoids correlating pending requests against transcript history.
  • ➖ Requires plugin and lifecycle integration beyond the existing permission protocol.
  • ➖ Would not help older plugins or sessions lacking the new hook.
  • ➖ Adds vendor-specific coupling for a signal already available in the tailed transcript.

Recommendation: Keep the PR's transcript-correlation approach. The Chat tab already tails and replays tool results, making it the smallest compatible place to detect terminal answers; idempotent withdrawal tracking and transport-failure retries address ordering and reconnect risks without expanding the plugin contract.

Files changed (17) +721 / -68

Enhancement (6) +311 / -50
App.axamlAdd a softer question-description text brush +1/-0

Add a softer question-description text brush

• Adds a reusable intermediate-contrast text brush for clearer option descriptions.

src/Capacitor.App/App.axaml

IPermissionService.csExpose permission request withdrawal +4/-0

Expose permission request withdrawal

• Adds the service contract for retiring a request whose tool has already completed elsewhere.

src/Capacitor.App/Services/IPermissionService.cs

QuestionCardViewModel.csAdd stepped question navigation and review +142/-10

Add stepped question navigation and review

• Introduces question and review steps, answer summaries, back/next/edit navigation, and review-only submission for series. Preserves inline submission and the single-select fast path for lone questions.

src/Capacitor.App/ViewModels/QuestionCardViewModel.cs

ChatTabView.axamlRender stepped, readable question cards +156/-38

Render stepped, readable question cards

• Reworks question cards around one visible question, step chips, review rows, and navigation controls. Moves option chrome into class styles so selection paints correctly and improves typography, spacing, indicators, and scroll capacity.

src/Capacitor.App/Views/ChatTabView.axaml

PermissionIpc.csDefine the withdraw resolve decision +7/-1

Define the withdraw resolve decision

• Adds shared constants for allow, deny, and withdraw decisions and documents withdrawal payload and settlement semantics.

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

PermissionPromptBroker.csAdd the tool-settled settlement source +1/-1

Add the tool-settled settlement source

• Defines the broker source used when transcript evidence makes a pending permission request obsolete.

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

Bug fix (3) +52 / -7
PermissionService.csSend withdraw decisions through existing resolve IPC +5/-2

Send withdraw decisions through existing resolve IPC

• Adds withdrawal support and centralizes allow, deny, and withdraw decision names. Withdrawals use the existing resolve frame and retain local conclusion behavior for rejected acknowledgements.

src/Capacitor.App/Services/PermissionService.cs

ChatTabViewModel.csWithdraw pending requests after matching tool results +39/-1

Withdraw pending requests after matching tool results

• Tracks all settled tool IDs and correlates them with pending permission requests. Sends each withdrawal once, retries transport failures during reconciliation, and cancels outstanding work during teardown.

src/Capacitor.App/ViewModels/ChatTabViewModel.cs

PermissionIpc.csSettle withdrawal frames as denied withdrawn hooks +8/-4

Settle withdrawal frames as denied withdrawn hooks

• Accepts withdraw resolve frames and settles them as withdrawn with a tool-settled source. The parked hook receives deny because the completed tool must not authorize a later invocation.

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

Tests (6) +338 / -10
ChatTabViewModelTests.csTest transcript-driven withdrawal reconciliation +65/-2

Test transcript-driven withdrawal reconciliation

• Covers result/request arrival ordering, surviving requests, unrelated entries, in-flight deduplication, and retry after transport failure.

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

ChatTabViewSmokeTests.csSmoke-test selected styling and stepped cards +76/-2

Smoke-test selected styling and stepped cards

• Verifies selected options receive accent styling and can be cleared. Exercises a two-question card through step chips to an incomplete, disabled review submission.

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

FakePermissionService.csSupport scripted withdrawals in permission tests +9/-0

Support scripted withdrawals in permission tests

• Records withdrawal calls, returns queued outcomes, and retains requests after transport failures to enable retry coverage.

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

PermissionServiceTests.csTest withdrawal wire and compatibility behavior +30/-0

Test withdrawal wire and compatibility behavior

• Verifies payload shape, successful and rejected acknowledgements, local request conclusion, and retention after transport failure.

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

QuestionCardViewModelTests.csCover question stepping, review, and submission +130/-4

Cover question stepping, review, and submission

• Tests lone-question behavior, automatic advancement, navigation gating, answer summaries, review editing, incomplete reviews, and review-only submission.

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

PermissionIpcTests.csTest daemon withdrawal settlement semantics +28/-2

Test daemon withdrawal settlement semantics

• Updates resolve validation expectations and verifies withdrawals settle as tool-settled, publish a withdrawn result, and answer the parked hook with deny.

test/Capacitor.Cli.Daemon.Tests.Unit/Services/PermissionIpcTests.cs

Documentation (2) +20 / -1
CHANGES.mdDocument stepped questions and terminal-answer withdrawal +19/-0

Document stepped questions and terminal-answer withdrawal

• Explains the new question-series workflow, selected-state styling fix, transcript completion signal, withdrawal semantics, retry behavior, and older-daemon compatibility.

docs/CHANGES.md

PermissionDecisionLog.csDocument tool-settled decision sources +1/-1

Document tool-settled decision sources

• Updates decision-log source documentation to include policy, daemon shutdown, and tool-settled outcomes.

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

@qodo-code-review

qodo-code-review Bot commented Sep 4, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. WithdrawAsync bypasses mutation lane ✗ Dismissed 📘 Rule violation ⌂ Architecture
Description
The new desktop permission withdrawal sends a mutation directly through ILocalControlOps rather
than the shared DaemonMutationLane. This bypasses the required serialization path and can allow
daemon mutations to race.
Code

src/Capacitor.App/Services/PermissionService.cs[R60-61]

+    public async Task<PermissionResolveOutcome> WithdrawAsync(PendingPermissionRequest target, CancellationToken ct) =>
+        await SendResolveAsync(new PermissionResolveDto(target.RequestId, PermissionResolveDecisions.Withdraw, null, null), ct).ConfigureAwait(false);
Relevance

●●● Strong

Recent app precedents accept fixes for shared-operation races and lifecycle serialization; this
directly violates an explicit mutation-lane rule.

PR-#653
PR-#474

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2738493 requires every desktop daemon mutation to use the single DaemonMutationLane. The
added WithdrawAsync calls the shared direct-send method, which invokes
_ops.ResolvePermissionAsync; application composition creates PermissionService with the raw
operations object rather than the mutation lane.

Rule 2738493: Route all desktop daemon mutations through the single DaemonMutationLane abstraction
src/Capacitor.App/Services/PermissionService.cs[60-66]
src/Capacitor.Cli.Core/LocalIpc/LocalControlOps.cs[120-132]
src/Capacitor.App/App.axaml.cs[155-166]
src/Capacitor.App/App.axaml.cs[315-317]

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 newly added permission withdrawal reaches daemon IPC directly instead of using the app-lifetime `DaemonMutationLane`.

## Issue Context
`WithdrawAsync` delegates to `SendResolveAsync`, which calls `ILocalControlOps.ResolvePermissionAsync`. Route this mutation through the existing shared lane while preserving acknowledgement and transport-failure behavior.

## Fix Focus Areas
- src/Capacitor.App/Services/PermissionService.cs[60-76]
- src/Capacitor.App/App.axaml.cs[155-166]
- src/Capacitor.App/App.axaml.cs[315-317]

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


Grey Divider

Context sources
✅ Compliance rules (platform): 56 rules
Review mode: 🧠 Deep: This is a behavior-dense, cross-cutting change spanning UI state/navigation, transcript reconciliation, IPC protocol semantics, daemon settlement, compatibility handling, and many independent code paths where redundant review could catch subtle defects.

Grey Divider

Tip of the day
💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/Capacitor.App/Services/PermissionService.cs

@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: ff35dad584

ℹ️ 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".

async Task WithdrawAsync(PendingPermissionRequest request) {
try {
var outcome = await _permissions.WithdrawAsync(request, _lifetime.Token);
if (outcome.Kind == PermissionResolveKind.TransportFailure) _withdrawing.Remove(request.RequestId);

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 Reschedule reconciliation after a failed withdrawal

When the one-shot resolve socket times out or returns an unexpected reply while the long-lived permission subscription remains healthy, this only removes the request ID from _withdrawing; it does not call or schedule Reconcile. Empty transcript polls also return before reconciliation, so if the result and the session's final output arrived in the same batch, the stale card remains indefinitely unless an unrelated permission or transcript event occurs. Trigger a bounded retry/reconciliation after clearing the guard so transport failures actually reopen the withdrawal.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 21e29fc. A failed send reopens the request and retries on its own after a bounded doubling backoff (2s, 4s, 8s) instead of waiting for an unrelated permission or transcript event; past the cap, the next resubscribe or reconcile tries again. The guard and the failure count are cleared when the request leaves the cache. Covered by two ChatTabViewModel tests: one drives a single failed withdraw through the backoff with no other event, the other walks all retries to the cap and then shows a later reconcile still retrying.

alexeyzimarev and others added 3 commits September 5, 2026 16:16
…#769)

Option chrome lives in class styles only: a local BorderBrush on the button outranks
the selected style and leaves it inert.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lt (#769)

The plugin registers no PostToolUse hook and a terminal answer leaves the
PermissionRequest hook parked, so the transcript's tool_result is the only signal the
daemon can be given.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The resolve rides a one-shot socket that can fail while the subscription stays
healthy, and an empty transcript poll never reconciles.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Desktop question card shows every question at once, hides picks and outlives a terminal answer

1 participant