Consent prompt window, Activity feed, tray attention (desktop supervisor) - #504
Open
alexeyzimarev wants to merge 26 commits into
Open
Consent prompt window, Activity feed, tray attention (desktop supervisor)#504alexeyzimarev wants to merge 26 commits into
alexeyzimarev wants to merge 26 commits into
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…aved/requester_display DTO fields, hoisted decision record
…uester_display threading
…re, consent/2 capability
…fail-closed validation
…ache, lifetime tombstones, prune hygiene Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…in-flight TOCTOU Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…oordinator-owned raise The window renders ONE pinned request: the sorted queue head, released only on an advance, so an arrival, a prune or a replay can never swap the display out from under a click. Three honesty rules carry the design: * hint expiry is not a verdict — the countdown says so and the buttons stay active, so a click after zero either applies (backward clock step) or runs the "Already decided" path on the daemon's Ok=false; * an in-flight resolve outranks the clock — "Expiring…", the ack governs; * a transport failure discloses nothing about the rule, whose outcome on that path describes a save that was never sent. The advance also skips the identity it just concluded: the service's eviction and the ack's continuation are two independently posted jobs, so the queue view may not have caught up yet. The coordinator owns the single window (open-or-activate, close = defer, reopen re-creates) and is what filters the service's unconditional entry-added signal by visibility and marshals it to the UI thread.
…rtup and shutdown ConsentService is created beside the daemon client over the shared ops/ticker/notifier, and the prompt coordinator builds a fresh window (and ViewModel) per raise. Teardown stays reverse-creation order and now runs coordinator before service before daemon client (spec §5), on both the shutdown and startup-failure paths, so no click can reach a disposed service; a resolve already in flight was cancelled by the shutdown token and settles on the ViewModel's silent-abort path.
…der it On the last pending request — the common single-prompt case — the warning toast was notified and then discarded: the advance emptied the queue and the window closed on the same beat, before the posted overlay could render, leaving the disclosure on stderr only. Exactly what "never a silent success" exists to prevent. Settle now asks what the advance would land on. With something queued it advances as before (multi-entry behaviour unchanged, and the toast lands over the next request); with nothing queued it takes the existing 2-tick terminal hold, showing the warning in the window as well as over it, and closes after. Applied with no warning still advances and closes immediately. The hold now serves two conclusions, so the phase is named Concluded rather than AlreadyDecided and both entry points share one Hold helper. Also drops an issue number from a comment (scripts/check-linear-ids.sh).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ing the prompt window EntryAdded keyed on the cache KEY, so it was wrong in both directions. A successor B under A's RequestId — a relaunch, the likeliest second prompt there is — replaced A's slot in silence and never raised a window for the retry. And a resubscribe's clear+replay made every replayed entry look new, so a window the user had explicitly closed (a defer) came straight back on any reconnect blip, while an OPEN window saw the intermediate empty changeset, closed itself, and was rebuilt a moment later with a fresh ViewModel, a reset pin and stolen focus. The signal now fires on the FIRST SURFACING of a PromptId: a service-lifetime `_surfaced` set with the same never-reused-GUID argument the tombstones rest on (tombstones are a subset of it, kept separate because a tombstone DROPS a frame while `_surfaced` only keeps it quiet). A successor carries a fresh identity and raises; a replayed one does not. The window half is a one-beat close deferral: only a DECISION — an ack, or the end of its terminal hold — closes on the spot. A queue the cache emptied waits one ticker beat, and a replay landing inside it disarms the close. The pin still releases immediately, so nothing that left the cache stays on screen. Also moves OnStatus's down-level `_cache.Clear()` inside `_lock`, closing the window where an Upsert that had already passed its tombstone test lands its insert after the clear and resurrects a previous incarnation's entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sistency items ActivityViewModel's ticker subscription was constructor-scoped and dropped on the floor. The shared ticker is Publish().RefCount(), so an undisposed subscriber keeps its Interval — and the ViewModel — alive past teardown, including the startup-failure path where the app lingers on an error window. It becomes IDisposable, holds the subscription, and joins App's two disposal lists in reverse creation order (after the consent service, before the pause controller). Also, from the same review pass: * the DateTimeOffset parse styles now agree — RoundtripKind in both the feed and ConsentService, over the same daemon-written ISO stamps; * LaunchConsentBroker's class doc names all four instance-scoped removal sites, not two (the timeout claim and TryResolve's own claim were missing); * UiTicker's pipeline note says "this ticker", which is what it constructs; * the §5/§6 refinements this wave discovered are recorded in the spec the way the §6 disclosure amendment was. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR Summary by QodoDesktop supervisor consent prompt + activity feed + tray attention (consent/2)
AI Description
Diagram
High-Level Assessment
Files changed (57)
|
![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: 94da3db84e
ℹ️ 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".
| internal static string ActivityStatKey(string daemonName) { | ||
| try { | ||
| var path = ConsentDecisionLogReader.PathFor(daemonName); | ||
| return $"{StatOf(path + ".1")}|{StatOf(path)}"; |
There was a problem hiding this comment.
On a normal installation, consent-decisions.jsonl.1 does not exist until the live log first exceeds the rotation threshold. StatOf(path + ".1") therefore throws and collapses the entire pair to the constant key "absent", so creating or appending to the current log never looks like a stat change. This can leave the Activity tab empty or stale indefinitely—especially when the ack-triggered refresh runs before the daemon appends its record—until the user hides and reopens the tab or the first 1 MB rotation occurs. Represent each missing file independently so changes to the live file still alter the polling key.
Useful? React with 👍 / 👎.
Code Review by Qodo
1. Activity feed never refreshes
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+14
to
+17
| /// Four guards carry the reviewed reasoning: | ||
| /// | ||
| /// * <b>EntryAdded is the FIRST SURFACING of a PromptId, never a new cache key</b> — the signal | ||
| /// is the raise trigger (spec §6), so it has to mean "a request the user has not been offered |
There was a problem hiding this comment.
1. consentservice comment block too long 📘 Rule violation ⚙ Maintainability
New multi-paragraph doc/comments were added that restate design rationale in-line, which reduces readability and increases maintenance overhead. The compliance checklist requires keeping comments minimal and favoring self-explanatory code.
Agent Prompt
## Issue description
Verbose multi-paragraph comments were introduced (especially in `ConsentService` and `ActivityViewModel`) where clearer naming/structure (and shorter, targeted comments) would better convey intent.
## Issue Context
PR Compliance requires comments to be brief and necessary, avoiding long narrative blocks.
## Fix Focus Areas
- src/Capacitor.App/Services/ConsentService.cs[10-40]
- src/Capacitor.App/ViewModels/ActivityViewModel.cs[17-32]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+254
to
+257
| internal static string ActivityStatKey(string daemonName) { | ||
| try { | ||
| var path = ConsentDecisionLogReader.PathFor(daemonName); | ||
| return $"{StatOf(path + ".1")}|{StatOf(path)}"; |
There was a problem hiding this comment.
2. Activity feed never refreshes 🐞 Bug ≡ Correctness
App.ActivityStatKey returns "absent" when the rotated log file (.1) is missing, so ActivityViewModel’s stat-key poll never detects changes while only consent-decisions.jsonl exists. This leaves the Activity tab stale after new decisions are appended unless the tab is reselected or an explicit refresh is triggered.
Agent Prompt
### Issue description
`ActivityStatKey()` computes a combined key for two files (`consent-decisions.jsonl.1` and `consent-decisions.jsonl`). If `.1` is missing, `new FileInfo(path).Length` throws, the broad `catch` returns the constant string `"absent"`, and the key never changes even when the main log grows. `ActivityViewModel` uses this key to suppress refreshes, so updates are not detected.
### Issue Context
- `.1` is only created after rotation; it is commonly absent.
- The stat key should change whenever either file changes, and should not collapse to a constant value when only one file exists.
### Fix Focus Areas
- src/Capacitor.App/App.axaml.cs[248-264]
### Suggested fix
- Replace `StatOf()` with a missing-file tolerant implementation:
- Option A: `FileInfo fi = new(path); if (!fi.Exists) return "missing"; return $"{fi.LastWriteTimeUtc.Ticks}:{fi.Length}";`
- Option B: catch `FileNotFoundException/DirectoryNotFoundException` inside `StatOf` and return a stable sentinel like `"0:0"`.
- Avoid wrapping the entire combined computation in a single catch that masks changes in the existing file.
- Add/adjust a unit test to cover the common case: only the main log exists and changes -> stat key changes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+93
to
+97
| void SafeRefresh() { | ||
| ConsentLogReadResult result; | ||
| try { result = _read(); } catch { return; } // swallowed — last-good rows stay on display | ||
| Apply(result); | ||
| } |
There was a problem hiding this comment.
3. Ui-thread log reads 🐞 Bug ➹ Performance
ActivityViewModel calls the injected log reader synchronously from UiTicker ticks, which run on the UI thread. Reading and parsing the JSONL files on the UI thread can cause visible UI stalls when the log is large or the disk is slow.
Agent Prompt
### Issue description
`ActivityViewModel.SafeRefresh()` performs `_read()` (file open/read + JSON parse) inline on the UI thread because it is invoked from `UiTicker.Ticks` which is scheduled on `RxSchedulers.MainThreadScheduler`. This can block rendering/input during refresh.
### Issue Context
- `ConsentDecisionLogReader.ReadTail()` reads all lines from both log files into memory before taking the tail.
- Even if the file is capped, synchronous disk IO + JSON parsing on the UI thread is avoidable.
### Fix Focus Areas
- src/Capacitor.App/ViewModels/ActivityViewModel.cs[51-97]
- src/Capacitor.Cli.Core/LocalIpc/ConsentDecisionLog.cs[29-57]
- src/Capacitor.App/Services/UiTicker.cs[6-38]
### Suggested fix
- Run `_read()` on a background scheduler/task and marshal only `Apply(result)` to the UI thread.
- Example approach: `Observable.Start(_read, RxSchedulers.TaskpoolScheduler)` (or `Task.Run`) then `.ObserveOn(RxSchedulers.MainThreadScheduler)`.
- Keep the existing “swallow errors / keep last-good rows” semantics.
- Consider debouncing/coalescing refresh triggers so multiple triggers in quick succession don’t queue multiple reads.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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 #503. AI-1652.
The consent UX for the desktop supervisor app — the piece that answers the original "daemon launches without asking" complaint on machines running the app.
What's in here
App (
src/Capacitor.App)ConsentService: status-driven subscription gated on the newconsent/2capability, identity-guarded pending cache, service-lifetime tombstones (ghost-replay defense), prune hygiene, one-resolve-at-a-time lane. The shared 1 Hz ticker is hoisted into an app-lifetimeUiTicker.Wire/daemon hardening (all additive)
prompt_idrequest identity on pendings, echoed on resolves, atomically claimed by the broker — a stale resolve can never decide a different launch that reused the agent id (the daemon makes no id-non-reuse guarantee).ConsentSubscribeV2 = 17,ConsentResolveV2 = 18): a pre-upgrade daemon's codec rejects them before routing, so mixed-version consent fails closed on the wire — no capability-check TOCTOU across daemon restarts.consent/2is advertised for discovery only.rule_savedon consent acks: the handler deliberately persists an "Allow & remember" rule before resolving (durable trust), so an already-decided outcome now discloses the installed rule instead of hiding it. The button says "Allow & remember" — not "Always allow" — because earlier deny rules (including Pause) shadow the appended allow.requester_displaythreaded through the consent pipeline (gate input → prompt request → pending DTO → decision record) so prompts and the feed show names, notgithub:…ids.Process
Spec survived a 7-round hosted-Codex review (
docs/superpowers/specs/2026-08-08-ai1652-consent-prompt-activity-feed-design.md); implemented as 11 SDD tasks with per-task reviews, two task-level fix rounds, and a whole-branch final review whose one must-fix (identity-aware raise signal) landed in the final fix wave. App suite 307/307; consent-focused CLI suites green; AOT publish clean.🤖 Generated with Claude Code