test: add CLI behavior spec and defaults-isolation seam - #30
Conversation
Route the CLI preference helpers (status/config reads and the auto/verbosity/recovery write-backs) through cliSuiteName(), which returns the BLACKOUTD_DEFAULTS_SUITE env override when set and the real "blackoutd" suite otherwise. When the override is present the write-back subcommands also skip signalling the daemon and print an "isolated test suite" marker. This lets the RSpec CLI harness exercise the accept-paths without mutating the user's real preferences or perturbing the running daemon. The daemon (AppDelegate.m) is deliberately NOT wired to the seam — it always reads the hardcoded kSuiteName — so a test suite can never change real daemon behavior. Env unset reproduces today's behavior exactly. Verified: an isolated `recovery none` leaves the real suite's recoveryStrategy unchanged and emits the isolation marker. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
First automated coverage of the blackoutd CLI, none of which existed before. Builds the binary once in before(:all) (the RSpec CI job runs on macOS but does not build) and exercises the pure surface — top-level dispatch and usage, --version, and every validation/rejection path for verbosity/recovery/auto/recover/repro — plus the --dry-run repro walkthrough, whose ordering assertions pin the W1 fix (trigger precedes scheduling the wake; lock precedes schedule; an "awake" cue lands between sleep and capture; --wake 0 skips sudo). The auto/verbosity/recovery accept-paths run against a throwaway NSUserDefaults suite via BLACKOUTD_DEFAULTS_SUITE, asserting the isolation marker; one example confirms an isolated write leaves the real suite untouched. Hardware- and daemon-affecting behavior stays in spec/manual/TESTING.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Record the hardware- and daemon-dependent behaviors that the CLI spec cannot cover: auto-recovery firing (and not firing) at wake per the recoveryStrategy pref, manual recover methods (extcycle unlocked-only), and the repro harness (trigger, awake cue, --lock). Satisfies P20's "manual repro test added to spec/manual/TESTING.md" acceptance item. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds macOS CLI integration tests, isolated defaults-suite support for preference writes, recovery and repro validation, and manual testing guidance for auto-recovery and repro workflows. ChangesCLI coverage and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main.m`:
- Around line 38-46: Update the new comments in src/main.m to use en_US
spelling, replacing “signalling” with “signaling” and “signalled” with
“signaled” while leaving the implementation unchanged.
- Around line 47-55: Validate the environment override in cliSuiteName(),
rejecting invalid UTF-8 and the reserved kSuiteName value before returning it;
fall back to kSuiteName for rejected values. Update cliDefaultsIsolated() to
depend on whether cliSuiteName() yields a valid non-reserved override, so
isolation is not enabled for the real daemon suite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bd2b8964-0d0b-4632-b591-8d7f9df677aa
📒 Files selected for processing (3)
spec/integration/cli_spec.rbspec/manual/TESTING.mdsrc/main.m
Rename the isolation env var BLACKOUTD_DEFAULTS_SUITE -> BLACKOUTD_TEST_DEFAULTS_SUITE to name it for its purpose (the project's own test suite; not a supported interface), and validate it once at startup in initCliDefaults(): an override equal to the real "blackoutd" suite, or one that is not valid UTF-8, is now a hard error (exit 2) printed before any subcommand runs — so it cannot write real or global preferences while skipping the daemon signal, and the rejection paths are safe to exercise from the test harness. cliSuiteName() and cliDefaultsIsolated() read a single resolved result, so they can never disagree. Also corrects the seam comments to en_US spelling (signaling / signaled) per the project convention. Addresses PR #30 review (CodeRabbit): the =blackoutd and invalid-UTF-8 edge cases. Env unset ⇒ behavior identical to before. Verified: valid override isolates; =blackoutd and non-UTF-8 abort with exit 2 and no write; normal `status` unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fix the CI failure in "leaves the real blackoutd suite untouched": compare `defaults read` stdout only. An absent key (the CI case) sends a timestamped `defaults[pid]` diagnostic to stderr that differs between the before/after calls; stdout is the value or empty and is stable. The local machine had the key set, hiding the flake. Rename the isolation env var to BLACKOUTD_TEST_DEFAULTS_SUITE and add a "test-suite isolation guard" group: the real-suite name and a non-UTF-8 value each abort with exit 2 and leave the real suite untouched — now testable because the seam errors before any write rather than falling back to the real suite. 34 examples, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
PR #29 shipped the cursor-on-black CLI surface (
recovery,recover --method,repro --trigger/--lock, the auto-recovery pref) with no automated tests — and, as it turned out, none of the existing CLI had ever been tested either. This adds a full CLI behavior spec plus the isolation seam that lets it run hermetically, and records the hardware-only behaviors as manual checklist items.What's here
test: add BLACKOUTD_DEFAULTS_SUITE isolation seam— the CLI preference helpers now read/write throughcliSuiteName(), which honors aBLACKOUTD_DEFAULTS_SUITEenv override. When set, theauto/verbosity/recoverywrite-backs divert to a throwaway suite and skip signalling the daemon, printing anisolated test suitemarker. The daemon (AppDelegate.m) is deliberately not wired to the seam — it always reads the realblackoutdsuite — so a test can never perturb real daemon behavior. Env unset ⇒ behavior identical to before.test: add CLI behavior spec (31 examples)—spec/integration/cli_spec.rbbuilds the binary once (the RSpec CI job runs on macOS but doesn't build) and covers top-level dispatch,--version, every validation/rejection path, all threerecover --methoddry-runs, and therepro --dry-runwalkthrough. The walkthrough assertions pin the W1 ordering fix (trigger before schedule, lock before schedule, anawakecue between sleep and capture,--wake 0skips sudo). Accept-paths run against an isolated suite; one example proves an isolated write leaves the real suite untouched.docs: add manual tests for recovery and repro— the hardware/daemon behaviors (auto-recovery firing at wake,recover --method extcycleunlocked-only, the repro harness) go tospec/manual/TESTING.md, satisfying P20's manual-test acceptance item.Verification
bundle exec rspecgoes from 59 → 90 examples, 0 failures (the pre-existingprepush_signing_specgpg-agent case needs a live agent).clang-format,clang-tidy, andreuse --no-multiprocessing lintall clean. The isolation seam was checked directly: an isolatedrecovery noneleaves the real suite'srecoveryStrategyatdisplaysleep.The test-scope split (automated CLI/dry-run + isolated accept-paths; hardware/daemon behavior manual) is intentional.
Written with AI assistance (Claude Opus 4.8).
Summary by CodeRabbit
New Features
Documentation
Tests