Repository navigation
Add desktop daemon settings - #886
Conversation
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 QodoAdd desktop daemon settings and safe rename workflow
AI Description
Diagram
High-Level Assessment
Files changed (29)
|
Code Review by Qodo
1. Daemon renaming is always unavailable
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| Message = "Saved. Update the daemon to apply capacity changes without a restart."; | ||
| } else { | ||
| try { | ||
| var ack = await _ops.PutDaemonSettingsAsync(new DaemonSettingsPutDto(capacity), _lifetime.Token); |
There was a problem hiding this comment.
3. Capacity saves can race daemon changes 📘 Rule violation ⌂ Architecture
SaveAsync calls _ops.PutDaemonSettingsAsync directly rather than submitting the live capacity mutation through the shared DaemonMutationLane. When a save overlaps a queued install, replace, start, or retirement action, this write runs outside the lane's ordering and retirement guards against the changing daemon graph.
Agent Prompt
## Issue description
Live capacity updates bypass the shared daemon mutation lane, allowing them to run independently of lifecycle mutations and the lane's retirement guards.
## Fix Focus Areas
- src/Capacitor.App/ViewModels/SettingsViewModel.cs[133-146]
## Recommended Fix
Add a capacity-update operation to the app-lifetime daemon mutation lane and inject that operation into `SettingsViewModel`. Replace the direct `_ops.PutDaemonSettingsAsync` call with the lane-backed operation while preserving the existing acknowledgement and error messages.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| var previous = await _settings.SaveNameAsync(name, _lifetime.Token); | ||
| Message = "Name saved. Restarting the daemon…"; | ||
| var outcome = await _runMutation(request!, _lifetime.Token); |
There was a problem hiding this comment.
1. Renames proceed after agents become active 🐞 Bug ≡ Correctness
RenameAsync checks Idle before awaiting SaveNameAsync, then invokes _runMutation without checking the asynchronously updated status or snapshot again. If an agent starts while the profile write is in progress, the retire transaction has no active-agent gate and can replace the old service despite the rename requirement for zero active agents.
Agent Prompt
## Issue description
The rename flow can pass its idle check, yield while saving the profile, and then retire the old service after an agent has become active. The CLI retirement path does not independently enforce the active-agent requirement.
## Fix Focus Areas
- src/Capacitor.App/ViewModels/SettingsViewModel.cs[181-190]
- src/Capacitor.Cli/Services/ServiceVerify.cs[785-805]
## Recommended Fix
Make the zero-active-agents condition part of the final rename transaction boundary. At minimum, recheck fresh idle evidence after `SaveNameAsync` and conditionally restore the name when the daemon became busy; preferably also make the retire operation refuse atomically when the old daemon has active agents so an agent cannot start between the final UI check and retirement.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| var help = await Run(cliPath, ["daemon", "--help", "--no-update-check"], | ||
| new RunOptions(EnvOverlay: Env(), Timeout: VersionTimeout), ct).ConfigureAwait(false); | ||
| return !help.TimedOut && help.ExitCode == 0 && | ||
| (help.Stdout.Contains("--retire", StringComparison.Ordinal) || help.Stderr.Contains("--retire", StringComparison.Ordinal)); |
There was a problem hiding this comment.
2. Daemon renaming is always unavailable 🐞 Bug ≡ Correctness
KcapCli.SupportsServiceRetireAsync runs kcap daemon --help --no-update-check and requires both exit code zero and --retire in its output. DaemonCommands.HandleAsync interprets --help as an unknown daemon subcommand, while the only usage text containing --retire belongs to the nested daemon service command, so CanRetireAsync returns false and RenameAsync refuses every rename before saving the name or entering the mutation lane.
Agent Prompt
## Issue description
`SupportsServiceRetireAsync` probes the top-level `daemon` command, whose help output does not expose the service-install `--retire` option and exits unsuccessfully. As a result, every supported CLI is classified as unsupported and Settings never permits a daemon rename.
## Fix Focus Areas
- src/Capacitor.App/Services/KcapCli.cs[137-144]
- src/Capacitor.Cli/Commands/DaemonCommands.cs[19-41]
- src/Capacitor.Cli/Commands/DaemonServiceCommands.cs[659-665]
## Recommended Fix
Probe the `daemon service` command's usage/capability surface instead of top-level `daemon`, and evaluate its documented help behavior correctly rather than requiring a zero exit code from a usage path that intentionally returns nonzero. Alternatively, add a dedicated zero-exit CLI capability query for service retirement and make the app call that query; retain the requirement that the returned output explicitly proves `--retire` support.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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: 3a3d9c16f8
ℹ️ 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".
| IsBusy = true; | ||
| Message = null; | ||
| try { | ||
| if (await _targetRunning(name, _lifetime.Token)) { |
There was a problem hiding this comment.
When the target name belongs to a stopped-but-installed daemon, this socket probe returns false and the rename proceeds. The resulting --replace --verify --retire request reaches ServiceVerify.ApplyReplaceMatrixAsync, where the pre.UnitPresent branch clears the target unit without verifying its profile (src/Capacitor.Cli/Services/ServiceVerify.cs:1081-1084). Renaming one daemon to the name of another profile's stopped service therefore silently removes that other background service; check installed-unit ownership or refuse any occupied target name before dispatching the rename.
Useful? React with 👍 / 👎.
| if (!_startupSettled.IsCompletedSuccessfully || !Idle) { | ||
| Message = "Wait until startup has finished and the daemon is idle before renaming."; |
There was a problem hiding this comment.
If a remote launch is admitted after this final Idle check but before the queued CLI mutation retires the old service, the rename proceeds even though an agent is now active. The mutation lane serializes service mutations, not agent launches, and ServiceVerify.RetireAsync clears the service and waits for it to stop without checking the active-agent count (src/Capacitor.Cli/Services/ServiceVerify.cs:1035-1038), so this race can terminate newly started agent work despite the UI promising rename is restricted to an idle daemon. Establish an admission/pause fence that remains held through retirement, or enforce idle atomically in the daemon-side transaction.
Useful? React with 👍 / 👎.
1 participant
Closes #791 — AI-2535
What & why
Settings opens from the application menu (⌘,) and tray. Capacity saves to the selected profile before applying live, with explicit feedback for stopped or older daemons. Rename requires an idle daemon, a free name and confirmation, then replaces the service and relaunches the app.
Where to look
Rename waits for startup, checks CLI retire support before saving, and rejects environment-controlled names. After any transaction outcome, the old app requires restart before managing the daemon. Transaction failures keep the new name and show recovery; a pre-spawn unsupported-CLI refusal restores it. Unbundled builds require an app restart.
Verification
Claude review clean at 3a3d9c1. 1,729 desktop tests passed; full app/test rebuild has zero warnings. macOS app and CLI Release publishes succeeded; no CLI AOT/trimming diagnostics (local Homebrew deployment-target linker warnings remain). Native preview checked layout, command gates and capacity feedback with isolated configuration and fake daemon data.