Skip to content

Add crispctl automation control plane - #73

Draft
Juns-g wants to merge 10 commits into
didriksg:mainfrom
Juns-g:feature/crispctl-foundation
Draft

Add crispctl automation control plane#73
Juns-g wants to merge 10 commits into
didriksg:mainfrom
Juns-g:feature/crispctl-foundation

Conversation

@Juns-g

@Juns-g Juns-g commented Aug 24, 2026

Copy link
Copy Markdown

Draft status — P0 fresh-artifact validation complete; hosted CI and P1 physical-display validation remain open

Current head: f012784 · Status: Draft — do not merge or mark ready.

What this PR adds

  • crispctl: a JSON-first CLI that talks only to the versioned, owner-only local Unix socket hosted by Crisp.app. Crisp.app remains the sole display-hardware owner.
  • P0 display automation: discovery, capabilities, normal brightness, Extra Brightness, HDR/EDR reporting, and multi-display brightness.
  • One bounded P1 slice: disconnected-display inventory plus exact-UUID physical-display disconnect / reconnect.

Safety contract

  • Mutations require a fresh capability/inventory preflight. Display-connection writes accept exact UUIDs only.
  • Unknown, duplicate, stale, virtual, or hardware-unproven displays fail closed. The last viewable physical display cannot be disconnected.
  • Post-dispatch timeout or cancellation is reported as write_outcome_indeterminate with retrySafe=false; callers must not retry automatically.
  • Display identity, recovery markers, protocol versioning, local socket ownership, absolute deadlines, and bounded transport resources are handled by Crisp.app rather than reimplemented by the CLI.

Batch brightness — product gap to close

  • The current set-all mode is correctly strict: all displays must have a readable restore snapshot or the write is refused before any display changes. This is the right default for unsupervised automation.

But crispctl is also an interactive, human-directed CLI. An explicit, deliberate override mode is still needed for a user who acknowledges a write-only / unverifiable display and accepts manual restoration. It must not be silent or implicit.
The override response must report per-display outcomes and warnings: written and verified, written but read-back unavailable, failed, or not attempted. It must never present an unverifiable write as a restorable transaction.

  • This is a product interaction gap, not an argument to remove the strict default.

P1 scope control — do not expand before validation

  • This PR must not add display aliases/remarks, broader connection operations, or additional recovery policy on top of the existing exact-UUID disconnect/reconnect slice.
  • First close the remaining gates: hosted CI and physical disconnect/reconnect validation. The supplied f012784 artifact has already regressed the P0 Extra Brightness-off fix successfully.
  • If human-friendly aliases are later needed, ship them separately as a UUID-and-hardware-identity-validated mapping. An alias must never bypass exact identity checks for a destructive display-connection operation.

Source-level verification at this head

  • Local Swift Package suite: 151 passed
  • Local Distribution suite: 78 passed
  • Local independent-process crispctl ↔ Unix-socket round trip: passed
  • Local production compilation, changed-file SwiftLint, shell syntax, and git diff --check: passed

Evidence and remaining gates

  • GitHub-hosted CI is not green yet*. The replacement run for f012784 is awaiting upstream approval: run 32924948784.
  • No real physical display disconnect/reconnect was executed. Dock/adapter/WindowServer behavior, sleep/wake behavior, and CG DirectDisplayID reassignment need user-run hardware validation.
  • Fresh-artifact regression was completed on the supplied f012784 package: built-in brightness/EDR and brightness get-all worked; Extra Brightness off now returned ok: true / app_state_verified and immediately read back as disabled, so the old asynchronous false-failure report is fixed. DELL DDC writes and the Samsung software-gamma path were also verified on this machine.
  • DELL DDC discovery was startup-sensitive in this artifact: the first session temporarily fell back to software_gamma; after restarting Crisp it recovered DDC. This is a real-device diagnostic/initialization concern, not evidence that the display lacks DDC support.
  • P1 remains unverified on this machine: even when DELL DDC was available, the connection capability could not positively establish its IOKit-backed physical identity, so both GUI and CLI correctly withheld disconnect/reconnect. Do not treat P1 as real-device-ready until this mapping and an actual disconnect/reconnect run are verified.
  • This branch is not in public Crisp 1.5.0. No Developer ID signing, notarization, release publishing, Homebrew publication, or merge has been performed.

Summary

This PR exposes Crisp.app's existing display-control model to scripts and agents without creating a second hardware-control stack. It intentionally keeps safety and state ownership in the app, while crispctl provides deterministic JSON results, stable exit codes, and explicit capability/verification information

@Juns-g

Juns-g commented Aug 25, 2026

Copy link
Copy Markdown
Author

Historical verification note for test artifact 1b7e2b5 (superseded in part by the correction below).

The package-level and distribution checks listed here passed locally, but the claimed independent-process Unix-socket round-trip was later found not reproducible at this commit because the script hard-coded an obsolete SwiftPM output path. See the correction in the next review comment: #73 (comment)

Real-device observations for the supplied 1b7e2b5 artifact, including the Extra Brightness async outcome issue, are recorded separately: #73 (comment)

Current PR-head and hosted-CI status are maintained in the PR description; this comment is retained only as historical evidence.

@Juns-g

Juns-g commented Aug 25, 2026

Copy link
Copy Markdown
Author

Verification correction for the current PR head 1b7e2b5b2aa16b2cd9e0cdb35e3d9b281638ccd3:

I reproduced the documented independent-process Unix-socket round-trip from that exact ref using a fresh scratch directory, and it currently fails before the fixture host starts. scripts/test-crispctl-roundtrip.sh hard-codes:

  • $SCRATCH/out/Products/Debug/crisp-control-test-host
  • $SCRATCH/out/Products/Debug/crispctl

But swift build --scratch-path "$SCRATCH" emits the binaries under the platform-triple directory (on my machine: $SCRATCH/arm64-apple-macosx/debug/). The concrete failure is at script line 27: No such file or directory.

This means the claim that the independent CLI-process ↔ Unix-socket-host round-trip passed is not reproducible from the submitted commit. Please resolve the binary directory dynamically (for example via swift build --show-bin-path), run this script in CI, then update the verification note once green.

The IPC/CLI architecture and safety semantics look strong; this is a verification-chain blocker, not a request to redesign the feature.

@Juns-g
Juns-g marked this pull request as draft August 25, 2026 05:24
@Juns-g

Juns-g commented Aug 25, 2026

Copy link
Copy Markdown
Author

Supplemental real-device validation on PR head 1b7e2b5 (Crisp 1.5.0 test artifact):

Validated successfully:

  • Built-in display: brightness set/readback including >100% logical EDR brightness; 130% applied immediately and remained stable across immediate, 0.5 s, and 2 s reads.
  • DELL U2723QE: DDC brightness write to 80% changed the physical display (manual visual confirmation). Its hardware brightness readback remains unavailable.
  • LS27D80xU: software_gamma fallback changed the visible output at 80% and restored to 100% (manual visual confirmation). This is not a hardware OSD brightness guarantee.

Non-blocking but actionable issue:
extra-brightness set builtin off is asynchronously applied but reports a terminal failure too early. The immediate response was ok: false / write_verification_failed; immediate state was transitional (enabled: true, persistedEnabled: false), and it converged about 0.5 s later to enabled: false, persistedEnabled: false. Re-enabling succeeds immediately. Please either wait for convergence before returning a terminal result, or expose an explicit accepted/pending outcome plus a documented polling contract. This matters for automation, which could otherwise retry an operation already in flight.

Suggested docs/Skill clarification: distinguish (1) DDC control with verified hardware readback, (2) DDC write-only, and (3) software_gamma fallback. For the latter two, "read current value -> temporary change -> automatically restore" is not safe or guaranteed.

I did not test set-all: with one external display write-only and another gamma-only, a batch mutation cannot be safely restored to its pre-test hardware state. HDR has no writable target on this three-display setup.

@Juns-g

Juns-g commented Aug 25, 2026

Copy link
Copy Markdown
Author

Scope correction: the real-device observations in my previous comment were performed against the locally supplied Crisp 1.5.0 test artifact built from 1b7e2b5, not the current remote PR head a47e637. They remain valid reproduction evidence for that artifact, but current-head confirmation requires a freshly built/distributed artifact (or equivalent local build) from a47e637.

@Juns-g

Juns-g commented Aug 25, 2026

Copy link
Copy Markdown
Author

The verified CI workflow update is now pushed in f55f4cb and adds the DistributionTests plus the headless crispctl Unix-socket round trip to CI. The exact new commands pass locally (65 Distribution tests and CRISPCTL_HEADLESS_ROUNDTRIP_OK). GitHub has held the fork workflow before creating any jobs, so could you please use Approve and run for this run? https://github.com/didriksg/Crisp/actions/runs/32871477850

The PR remains Draft; approving the run only lets the CI execute and does not mark it ready or merge it.

@didriksg

Copy link
Copy Markdown
Owner

CI running is approved

@Juns-g

Juns-g commented Aug 26, 2026

Copy link
Copy Markdown
Author

Batch brightness: retain the strict default, add an explicit human override

Real-device validation showed that brightness set-all 60 safely refuses before making any changes when an external display is DDC-writeable but has no readable brightness snapshot. That strict, automatically restorable mode is the right default for unattended automation.

However, crispctl is also an interactive human-directed CLI. An explicit user request should be able to perform a best-effort batch write when the user knowingly accepts that a particular display cannot be read back or automatically restored. The current all-or-nothing refusal makes that valid use case impossible.

Please keep the existing default and add a separate, deliberately named override mode (the exact flag/name is an implementation decision). It should:

  • require an explicit acknowledgement of every write-only/unverifiable display;
  • keep capability/range/identity preflight for every display;
  • return per-display outcomes, distinguishing verified write, accepted-but-unverifiable write, failure, and not attempted;
  • never describe the override result as automatically restorable or transactional;
  • remain unavailable for display disconnect/reconnect, where the current fail-closed identity guard is still appropriate.

This is a product-completeness gap, not a request to weaken the safe default.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants