Skip to content

fix(cli): honor verify unknown exit status - #649

Merged
steipete merged 2 commits into
mainfrom
codex/verify-state-exit-contract
Aug 28, 2026
Merged

fix(cli): honor verify unknown exit status#649
steipete merged 2 commits into
mainfrom
codex/verify-state-exit-contract

Conversation

@steipete

@steipete steipete commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

Restore verify's documented exit status 2 when its underlying state tool fails before it can evaluate predicates. The shared MCP CLI renderer keeps exit 1 by default; only verify opts into exit 2. Evaluated satisfied/unsatisfied/unknown results retain their existing 0/1/2 handling.

This PR is now independent of the unmerged browser/application-inventory stack and targets main. Only the original verify fix, focused test hardening, documentation, and the two user-facing changelog entries are included.

Root cause

VerifyCommand rendered an errored verify_state response through MCPToolCommandOutput.output, which threw exit 1 before the command could apply its unknown=2 contract. This path dates to the original verify feature. A defaulted renderer parameter fixes the command boundary without changing other callers.

The regression fixture deliberately avoids the real service container: it uses mocks, in-memory snapshots, an explicit temporary mutation store, and trapped configuration/provider access. It does not rely on HOME overrides to protect operator credentials.

Verification

  • pnpm run build:cli passed on both current main and the candidate.
  • swift test --package-path Apps/CLI --no-parallel -Xswiftc -DPEEKABOO_SKIP_AUTOMATION --filter 'VerifyCommandTests|MCPToolCommandOutputTests' passed: 11 tests / 15 parameter cases. Optional automation tests were disabled, and live/automation flags were unset.
  • Both new verify cases fail against the pre-fix verify call path and pass with the fix. Text and JSON tests cover verify error=2, the shared renderer's default error=1, explicit override=2, and successful output remaining non-throwing.
  • Changed-file SwiftLint and inherited-config SwiftFormat checks, pnpm run lint:docs, and git diff --check passed.
  • Independent Codex autoreview of the complete candidate reported no actionable P0–P2 findings.

Live CLI proof

Built baseline and candidate binaries were copied to isolated proof paths, signed with the installed app's matching Developer ID, and signature-verified before execution. With PROOF_BIN selecting each signed build, these real subprocess commands were run:

"$PROOF_BIN" verify --app Fixture --window-exists --stable-samples 0 --no-remote
"$PROOF_BIN" verify --app Fixture --window-exists --stable-samples 0 --no-remote --json

Both formats return the same state-tool validation failure. Main exits 1; the candidate exits 2. JSON remains a standard error envelope with success: false, data.isError: true, and VALIDATION_ERROR.

A signed, nonactivating native fixture was also observed by exact PID/window ID. Both builds conservatively reported unknown/exit 2 because unrelated running-process metadata was incomplete. Native satisfied/unsatisfied outcomes were not proved on this host. The existing installed GUI Bridge did not meet current runtime requirements; no user app was upgraded/restarted and no unrelated process was changed to bypass these limitations. The fixture preserved foreground application and cursor position at creation.

The full local CLI/provider/automation suites were not run because of the documented shared-state initialization hazard; fresh hosted macOS CI is required before merge. No browser feature changes, release, or publication are part of this PR.

@clawsweeper

clawsweeper Bot commented Aug 27, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@cursor

cursor Bot commented Aug 27, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Narrow CLI exit-code and documentation change with tests; default MCP tool error behavior unchanged for other commands.

Overview
Restores peekaboo verify’s documented exit 2 when verify_state fails before predicates can be evaluated (e.g. capture preflight refusal). Previously the shared MCP tool error path always exited 1.

MCPToolCommandOutput.output now accepts an optional errorExitCode (default 1). Only VerifyCommand passes ExitCode(2) and rethrows after rendering the error envelope; satisfied/unsatisfied/unknown outcomes still use 0/1/2 as before. Other MCP-backed commands (e.g. browser) keep exit 1 on tool errors.

Adds unit tests for the renderer’s default vs override exit codes and an integration-style VerifyCommand test with mocked services and forced preflight refusal. Changelog and docs/commands/verify.md note that tool failures exit 2 and use the standard JSON error envelope without predicate fields.

Reviewed by Cursor Bugbot for commit 9e4d1c5. Bugbot is set up for automated code reviews on this repo. Configure here.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 27, 2026
@clawsweeper

clawsweeper Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 27, 2026, 11:26 PM ET / August 28, 2026, 03:26 UTC.

ClawSweeper review

What this changes

The PR makes peekaboo verify exit 2 when its state tool fails before predicate evaluation, while keeping the shared MCP renderer’s default tool-error exit status at 1.

Merge readiness

Blocked until stronger real behavior proof is added - 3 items remain

Keep open for normal maintainer review: the patch restores the documented verify error contract with focused regression coverage, and no actionable correctness defect was found.

Priority: P2
Reviewed head: 9e4d1c5f439665f9d6d48eb6b1839607e3c9505f
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) PR readiness rating was derived from proof quality, review findings, security review, and reviewer confidence.
Proof confidence 🦪 silver shellfish (2/6) Needs stronger real behavior proof before merge: ClawSweeper live verification failed on the reviewed terminal surface. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦀 challenger crab (6/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: ClawSweeper live verification failed on the reviewed terminal surface. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 6 items Current-main behavior: Current main routes an errored state-tool response through the shared renderer, whose error branch throws exit status 1; this establishes the documented-contract mismatch remains on main.
Narrow command-boundary fix: The introduced hunk passes exit status 2 only from verify; the shared renderer retains its default status 1 for all other callers.
Regression coverage: Tests cover text and JSON tool failures, the renderer default and override statuses, successful rendering, and ensure the preflight failure does not create the requested screenshot.
Findings None None.
Security None None.

Live Verification

Command: pnpm run build:cli

Result: FAIL (failed) — step 1 run bin_path=$(swift build --package-path Apps/CLI --show-bin-path) || exit $?; "$bin_path/peekaboo" verify --app Fixture --window-exists --stable-samples 0 --no-remote; exit_status=$?; printf 'verify_exit=%s\n' "$exit_status"; test "$exit_status" -eq 2: terminal run was blocked by the previous command: terminal command failed with exit status 1: "pnpm run build:cli"

[failure]
terminal run was blocked by the previous command: terminal command failed with exit status 1: "pnpm run build:cli"

[command 1 combined output]
[WARN] Unsupported platform: wanted: {"cpu":["arm64","x64"],"os":["darwin"],"libc":["any"]} (current: {"os":"linux","cpu":"x64","libc":"glibc"})
$ swift build --package-path Apps/CLI
error: the package manifest at '/tmp/clawsweeper-live-proof-649-pRIojQ/target/Commander/Package.swift' cannot be accessed (/tmp/clawsweeper-live-proof-649-pRIoj
Q/target/Commander/Package.swift doesn't exist in file system)
[ELIFECYCLE] Command failed with exit code 1.

Assertions:

  • FAIL expect_output: verify_exit=2

How this fits together

The CLI verify command queries fresh native window and accessibility state through an MCP tool, renders text or JSON results, and returns an exit status for scripts and automation. This change alters only the pre-evaluation tool-failure branch.

flowchart LR
  A[Verify CLI request] --> B[State verification tool]
  B --> C{Tool error?}
  C -->|Yes| D[Standard error output]
  D --> E[Exit status 2]
  C -->|No| F[Evaluate predicates]
  F --> G[Exit 0, 1, or 2]
Loading

Decision needed

Question Recommendation
Should the documented exit-status-2 contract for pre-evaluation verify tool failures be accepted for the next CLI release? Accept the documented contract restoration: Merge after required checks pass and communicate the corrected pre-evaluation failure status through the included changelog entries.

Why: This intentionally changes the observable status received by existing automation, even though it restores the documented ternary contract.

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: ClawSweeper live verification failed on the reviewed terminal surface. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P1) - Scripts that had treated pre-evaluation tool failures as generic exit status 1 will now receive the documented unknown status 2.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch size 124 additions, 5 deletions across 7 files The functional change is small; most added lines are targeted regression fixtures and tests.
Production versus tests production net +2, tests +113 The CLI behavior change is covered by substantially more focused test code than production code.

Merge-risk options

Maintainer options:

  1. Accept the documented exit-code correction (recommended)
    Land the scoped change after required checks pass, accepting that pre-evaluation failures are now distinguishable as unknown with status 2.

Technical review

Best possible solution:

Merge the narrow contract restoration after the required Swift checks pass, preserving the documented distinction between unevaluated state and an evaluated unsatisfied predicate.

Do we have a high-confidence way to reproduce the issue?

Yes. Current-main source establishes the error-1 path, and the supplied signed baseline-versus-candidate CLI runs report the same failure changing from exit 1 to exit 2 in text and JSON modes.

Is this the best way to solve the issue?

Yes. A defaulted renderer parameter confines the exit-status override to verify and leaves all other MCP command callers on their existing error-1 behavior.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 715caa24bbe1.

Labels

Label changes:

  • add merge-risk: 🚨 compatibility: Automation that depended on the prior generic status 1 for tool failures will observe status 2 instead.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦀 challenger crab.
  • add status: needs maintainer proof decision: A ClawSweeper-authored PR needs a maintainer proof capture or override decision. Needs stronger real behavior proof before merge: ClawSweeper live verification failed on the reviewed terminal surface. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: needs maintainer proof decision.

Label justifications:

  • P2: This corrects a documented CLI automation exit-status contract with limited blast radius.
  • merge-risk: 🚨 compatibility: Automation that depended on the prior generic status 1 for tool failures will observe status 2 instead.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦀 challenger crab.
  • status: needs maintainer proof decision: A ClawSweeper-authored PR needs a maintainer proof capture or override decision. Needs stronger real behavior proof before merge: ClawSweeper live verification failed on the reviewed terminal surface. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

Likely related people:

  • Peter Steinberger: Current history attributes the initial verify command and the recent repair to the same person. (role: original feature introducer and recent CLI-area contributor; confidence: high; commits: e9a93b874697, 342eac4b7bc2; files: Apps/CLI/Sources/PeekabooCLI/Commands/AI/VerifyCommand.swift, Apps/CLI/Sources/PeekabooCLI/Commands/MCP/MCPToolCommandOutput.swift)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add after-fix proof from a real setup, such as a short recording, terminal output, linked artifact, or redacted logs.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-08-27T09:01:15.124Z sha 97419aa :: needs changes before merge. :: [P3] Remove the release-owned root changelog entry

@steipete
steipete changed the base branch from codex/native-window-binding-after-parity-1-38 to main August 28, 2026 03:20
@steipete
steipete force-pushed the codex/verify-state-exit-contract branch from 97419aa to 9e4d1c5 Compare August 28, 2026 03:22
@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: needs maintainer proof decision A ClawSweeper-authored PR needs a maintainer proof capture or override decision. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 28, 2026
@steipete
steipete merged commit f46c055 into main Aug 28, 2026
15 checks passed
@steipete
steipete deleted the codex/verify-state-exit-contract branch August 28, 2026 04:53
@steipete

Copy link
Copy Markdown
Collaborator Author

Merged in f46c055901c4 after verification of exact PR head 9e4d1c5f439665f9d6d48eb6b1839607e3c9505f.

The shared renderer still defaults to exit 1. verify now explicitly requests exit 2 for failed state-tool evaluation. The PR was separated from its unmerged browser feature stack before landing; no feature-stack commits were included.

Verification performed:

  • pnpm run build:cli passed for the baseline and candidate.
  • swift test --package-path Apps/CLI --no-parallel -Xswiftc -DPEEKABOO_SKIP_AUTOMATION --filter 'VerifyCommandTests|MCPToolCommandOutputTests' passed (11 tests / 15 parameter cases), with optional automation disabled and live/automation flags unset. The isolated fixture avoids shared credential initialization. Both new verify cases failed against the pre-fix call path and passed with the fix.
  • Changed-file SwiftLint and inherited-config SwiftFormat, pnpm run lint:docs, and git diff --check passed. Independent Codex autoreview found no actionable P0–P2 findings in the complete unchanged candidate.
  • Real signed baseline/candidate CLI subprocesses ran verify --app Fixture --window-exists --stable-samples 0 --no-remote, both with and without --json. The same state-tool validation failure exited 1 before and 2 after; JSON error-envelope fields were checked.
  • A signed nonactivating native window fixture was observed by exact PID/window ID. Ambient process metadata was incomplete, so both builds correctly returned unknown/exit 2. Native satisfied/unsatisfied outcomes were not proved on this host. The installed GUI Bridge was incompatible with the current runtime; no user app or unrelated process was changed to work around that. The task-owned window was closed afterward.
  • Exact-head macOS CI passed core, CLI, Tachikoma, Peekaboo/Inspector app builds, and SwiftLint. Exact-head CodeQL passed Actions, JavaScript/TypeScript, and Swift. No required run failed or was rerun; Swift CodeQL took about 84 minutes.

The full local CLI/provider/automation suites were intentionally not run against operator state; hosted macOS CI supplied the broader checks. The earlier bot's Linux/missing-submodule attempt did not exercise native behavior and is not counted as proof. No release or artifact publication was performed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: needs maintainer proof decision A ClawSweeper-authored PR needs a maintainer proof capture or override decision.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant