Repository navigation
fix(cli): stabilize guarded-stop re-verification and report the failing guard fact - #6402
Conversation
…ng guard fact Three proven defects let an unchanged approved runtime read as changed: - isOcxCommandLine/isOcxStartCommandLine rejected the bundled sidecar's `ocx-<target triple>[.exe]` binary name, so every desktop-spawned runtime failed identity verification at approval and re-resolution. - The managing-CLI `--version` probe invoked a Windows command shim as `cmd /c <shim> --version`; cmd strips the outer quotes when more than one part is quoted, truncating the command at the first space whenever the shim path itself contains one. The shim now runs through a single verbatim-quoted /c line. - Single-shot probes (the shim `--version` probe and the Windows WMIC/PowerShell command-line read) treated one transient timeout as 'unobservable', flipping the compatibility fingerprint or PID identity of an unchanged process. Both get one bounded retry; a persistent failure still answers fail-closed. The approval-changed summary also collapsed ~10 distinct re-checks into one opaque message, so a transient re-probe was indistinguishable from genuine drift. The summary now carries `detail` naming the single guard fact that failed (token fingerprint vs live PID/port/hostname/CLI vs manager verification), threaded through runGuardedManagerStep as well. Addititive on ocx-stop/1; the desktop StopSummary tolerates the field. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
…identity widening Review correction: real Tauri builds run the bundled CLI as ocx.exe and native baseline ownership succeeded against it, so accepting ocx-<target-triple> names in process identity was broadened on an unproven launch path — reverted with its tests. The --version probe retry is now Windows-only (the platform the flake family was verified on); POSIX probes keep exactly one attempt. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
An unprovable manager (`kind: "unknown"` with a reason) is not a proven swap, but the pre-action recheck reported both as 'the service manager changed between approval and stop', losing why revalidation could not answer. Unknown now surfaces as 'the service manager could not be re-verified (reason)'; genuine mismatches keep the changed message. Stop and signal still never run against an unknown target. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
⏳ DRAFT
What to do
Review readiness checklist
0/0 boxes ticked. Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked. |
…90862188-guarded-stop-diagnostics
…90862188-guarded-stop-diagnostics
# Conflicts: # structure/runtime.md
|
Maintainer integration record (MAINTAINERS.md, dev-only)
|
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. |
Summary
detailtoocx-stop/1that names the failing runtime, compatibility, ownership, or manager observation. An unknown manager retains its observation failure reason instead of being reported as a proven manager swap..CMD/.BATversion-probe quoting for paths containing spaces. Retry transient Windows CLI-version and process-command-line probes once; persistent failures remain unknown and refuse takeover. POSIX probes remain single-shot..vbslauncher hypothesis were removed after native/source verification.Verification
Refreshed from
devat06cc3815c06a10120d89ef986c46be95be37f28awithout rewriting branch history, including the accepted upstream role-route and owner-registry ACL fixture corrections. Targeted verification: role routes 7 passed; owner-registry ACL 6 passed. Required CI passed at3075274ba1d77f30ee5772166c7f36938f17e813: zero failed and zero pending checks (platform/path-inapplicable checks remain skipped).bun run typecheck,bun run privacy:scan,bun run structure:check: passed.Focused process-state, managing-CLI, and guarded-stop tests: 48 passed. After the manager-reason follow-up,
bun test tests/cli/cli-stop-json.test.ts: 22 passed; typecheck passed.Combined native Windows build
1c14101e5(this PR plus the startup-budget, Scheduler-scope, and bounded-manager-command PRs): three fresh-sidecar takeovers from actual npm 2.67.0 under a spaced prefix passed in 45.948s, 67.357s, and 54.589s. Owned Quit/relaunch preserved installation identity and generation; guest decline/Quit preserved the npm PID without recording ownership.One additional native attempt under exactly two 30-second CPU workers passed in 49.861s. Timings measure approval to healthy replacement with persisted desktop ownership.
Unresolved: An earlier combined native attempt refused manager revalidation before acting, preserving the npm runtime. That run preceded the specific unknown-manager reason. The four follow-up passes and 46 successful read-only manager observations do not identify its cause or prove it fixed; the CIM-timeout explanation remains a hypothesis.
Full
test:changedexceeded the Windows 900-second cap and is inconclusive. CI remains the full-suite gate.Native app testing did not register Scheduler/WinSW services. Separate serial Scheduler CLI E2E passed on combined
d50be3427: install, supported exact approval, guarded stop (runtimeDown: true, 49.4s), repair/start, and uninstall plus explicit stop restored the initial absent-manager baseline. WinSW requires account credentials and remains untested.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Required local validation passed; commands, results, and any full-suite exception are documented.
I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Link to Devin session: https://app.devin.ai/sessions/a8795b27d4114aa6be37a88565e404b2
Open in Devin Desktop: https://app.devin.ai/desktop/session/a8795b27d4114aa6be37a88565e404b2?variant=devin
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Required local validation passed; commands, results, and any full-suite exception are documented.
I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.