[AI-2039] kcap daemon service ensure: the flow's daemon-install ladder - #656
Conversation
Adds 'kcap daemon service ensure': from a fresh status read, install when there is no unit or start when the unit is stopped, baking the born-prompt consent directive on install and gating the start exactly as an app-managed start is. A gate refusal exits with the verify transaction's coded exit plus one start_gate_reason= line, mapped machine-readably to recovery_surface=takeover|reinstall|attention via the pinned ReasonRouting table — never guessed at from prose. On non-launchd the ladder degrades to plain install/start; --json reports verified:false so the flow's copy can say so. Ambiguous states (unknown probe, active transaction, orphan label, stale marker) fail closed to attention with a coded reason. Moves ReasonRouting/RecoverySurface from the retiring Capacitor.App into Capacitor.Cli.Core so the CLI and the app share one pinned mapping (the same rescue shape as AI-2167). Adds --json output (ServiceEnsureJson), pure classifier + failure-map, and unit tests.
Code Review by Qodo
1.
|
- Default the profile to the resolved active one, so a bare 'ensure' on launchd still carries KCAP_PROFILE for the start gate's identity half (matches how Install resolves the pin). - Drift now carries verify_start_gate_drift as its reason alongside the attention surface — the JSON and the human line no longer read empty. - Mark the console-writing dispatch tests [NotInParallel].
realtonyyoung
left a comment
There was a problem hiding this comment.
Peer review — automated code-review flow (reviewer vendor: codex, model gpt-5.6-sol)
Ran a 3-round structured code-review flow against this PR. The reviewer signed off after round 3 with seven actionable findings (1 High×3, Medium×3, Low×1). One round-1 finding was withdrawn after context, and findings fixed or superseded mid-flow are not reposted — what follows is only what was still open at sign-off.
Each finding is an inline thread below, anchored to an added line, so it can be resolved individually.
| # | Severity | Summary |
|---|---|---|
| 1 | High | EnsureUnitEnv keeps ambient KCAP_URL, so --profile P does not actually pin P |
| 2 | High | Classifier success arm precedes the orphan_label / stale_marker fail-closed checks |
| 3 | High | "Already enabled" never proves the service job owns the validated daemon pid |
| 4 | Medium | ServiceStateToken ignores UnitPresent — a successful start reports state:"not_installed" |
| 5 | Low | LastGateReason is never cleared on entry, violating its own documented contract |
| 6 | Medium | EnsureFailure omits Verified, so refused launchd transactions always serialize verified:false |
| 8 | Medium | Coded viability_reason / refusal_reason evidence is discarded outside exits 28/29 |
Withdrawn (no action needed): finding 7, "the new verb is absent from the Getting Started surface." The reviewer withdrew it once shown README.md:153 in full — that section points a human at kcap daemon service install and links to the daemon section, while ensure is explicitly flow-driven/machine-facing and is documented at README.md:744 and in help-daemon.txt. Reviewer's words: "Adding it to onboarding would blur the supported human workflow rather than repair missing documentation."
Findings 6 and 8 were deliberately kept separate at the reviewer's call: "Finding 6 drops the verified-path boolean from every refusal DTO; finding 8 drops specific cause/recovery evidence that the engine already determined. They have different fixes and different consumer impact."
Reviewer caveats, carried through verbatim: the PR head f922298 and the branch-only test/design files were not present in the reviewer's launch worktree, so it treated the submitted diff and test descriptions as authoritative and verified the referenced base implementations locally. Tests were not run. I independently confirmed every base-code claim against main before relaying — ServiceEnvironment.cs:15 really does list KCAP_URL among captured keys; LaunchdServiceManager.QueryCore really does yield Probe=Absent, State=NotInstalled, UnitPresent=true for a stopped-but-installed service; ServiceVerify.cs:407 and :710 really are stderr-only Say(...) emissions with no in-process equivalent.
Posted as a COMMENT review, not an approval or a change request.
|
NO FINDINGS |
realtonyyoung
left a comment
There was a problem hiding this comment.
Re-reviewed the current head after the prior findings were addressed. No actionable findings. Static review only; no build or tests run.
AI-2039's ensure ladder (#656) merged to main after this branch was cut; both PRs added 'using Capacitor.Cli.Core;' to the same app test files, so the merge ref carried the directive twice (position-independent additions — git sees no textual conflict, the compiler does). Drop the duplicates.
* feat: rescue LoginShellProbe and PathShimInstaller into Capacitor.Cli.Core Move the process seam (IProcessRunner + records + the production ProcessRunner implementation) and the two Avalonia-only setup classes into Core so the flow can drive them after AI-2053 deletes the app. App consumers pick them up via using; the installer's destination-override seam becomes public (Core has no InternalsVisibleTo for the app). Tests move with the classes. * feat: kcap daemon shim ensure — the flow's PATH-fix capability The flow's Agents screen PATH warning offers 'fix it for me' and 'show me the line'. The lane carries values, not paths or commands (retirement spec 6.1), so the fix is a named capability the CLI composes itself: it resolves its own binary path (never a server-supplied one), probes the interactive login shell, and on a positive absence links /usr/local/bin/kcap to itself via the osascript admin prompt, then re-probes so success is never reported on the symlink alone. Unknown probe, filesystem conflict, and non-macOS all fail closed with a coded reason. --json emits the outcome the flow keys off. * fix: address review findings on the shim ensure verb and the Core rescue - Fail closed on a null post-install re-probe (was asserted as a definitive not-on-path diagnosis — the one guess in an otherwise fail-closed ladder). - Add an independent preflight seam so the conflict row is stubbable; the conflict refusal is now covered by a test instead of being untestable. - Reject unknown flags (--help, typos) before any probe or prompt. - Make the isMacOs seam nullable so the off-macOS arm can be forced on a macOS host (a bool default could not distinguish unspecified from false). - Reuse the probe instead of constructing a second one for the installer; make the classifier types internal. - Sanitize control bytes from human console output (the JSON arm is already escaped by System.Text.Json). - Fix stale doc references (ServiceProcess comment, ShimOfferCoordinator wording, installer class doc) and move the README shim section out of the middle of the service prose; design doc no longer cites the unmerged service-ensure sibling branch. * fix: address qodo findings on the shim ensure PR - Share FakeLoginShellProbe via Capacitor.Tests.Helpers instead of duplicating it in the Core and App test suites (the repo rule: cross-suite helpers live in Helpers with a public surface). - Off-macOS refusal (unsupported_platform) now beats an unknown probe in the classifier — the flow expects a stable platform row, not a probe-dependent one — and the daemon usage line lists the reviewer subcommand it dispatches. * fix: address review — preserve the npm launcher and the coded conflict row Two flow-contract corrections from review: - Link the shim to the npm launcher (kcap.js) when this CLI is part of an npm-global install, not to the native binary kcap.js spawned. The launcher is what intercepts 'kcap update' and runs npm; linking the native image would have made /usr/local/bin/kcap update a no-op. The launcher is a sibling package, so its path is derived from the running binary's own location with no environment lookup; a standalone binary links to itself. - Re-preflight after a failed install: the outer preflight and the installer's checks are not atomic, so an entry that appears mid-flight (or a non-forcing ln -s that loses the race) now still surfaces the coded refused/conflict row instead of a generic failed. * Merge main into ai-2167/let-the-flow-fix-a-broken-kcap-path AI-2039's ensure ladder (#656) merged to main after this branch was cut; both PRs added 'using Capacitor.Cli.Core;' to the same app test files, so the merge ref carried the directive twice (position-independent additions — git sees no textual conflict, the compiler does). Drop the duplicates. * fix: platform-neutral launcher-resolution test GetFullPath both sides of the npm-launcher assertion — on Windows a hardcoded POSIX path normalizes against the current drive's root, so the comparison failed on the Windows CI leg.
The first-run flow's Done detour ("reach this machine from anywhere") needs one action that makes the daemon service-installed and running. The ladder existed only in the Avalonia wizard (
Capacitor.App, being retired by AI-2053):DaemonStepViewModelclassifies a freshservice status --json,DaemonMutationLanedispatches and classifies the mutation, andReasonRoutingmapsstart_gate_reason=tokens to a recovery surface. The CLI had all the primitives but nothing composing them into the ladder the flow drives. This adds that composition.kcap daemon service ensure— from a fresh status read, install when there is no unit or start when the unit is stopped, and report "already enabled" when the daemon is running. Ambiguous states (unknown probe, active transaction, orphan label, stale marker) fail closed to attention with a coded reason — never guessed, never mutated into.prompt— the install bakesKCAP_CONSENT_SEED_DEFAULT=prompt(plus the expected-server pin) exactly as the app'sMutationEnvdoes, so an app-installed daemon is bornprompt: nothing runs unattended on someone else's say-so.promptis refused withverify_start_gate(28) and onestart_gate_reason=line, mapped machine-readably torecovery_surface=takeover|reinstall|attentionvia the pinnedReasonRoutingtable — never derived from prose.ensure --jsonreports"verified":falseso the flow's copy can say so.ReasonRouting/RecoverySurfacemove toCapacitor.Cli.Core— the CLI cannot reference the app, both reference Core; the pinned token→surface table now has one home (same rescue shape as AI-2167), with the app's references updated.--jsonemits a snake_caseServiceEnsureJson(service id, fresh state, action, outcome, recovery, reason, verified) through the sharedServiceJsonContext, including on the pre-flight refusals.Windows answer to AI-2039's open question, established while wiring the install: the daemon itself is fully cross-platform (ConPTY, Scheduled Task service, win-x64 npm, Windows CI), hosted agents and the server→daemon SignalR path work there; what is Windows-gated is the local
kcap agentdrive and the launchd-only verified transaction. The detour therefore shows everywhere; the copy reflects plain install off-macOS.Tests: 28 new/moved unit tests (classifier rows incl. the launchd stopped-but-installed shape, born-prompt bake, JSON render, failure→recovery mapping, dispatch rows);
ReasonRoutingtests moved with the type. Full CLI suite 3481, Core 2009, App 1157, all green; AOT publish clean.