Skip to content

Add missing Spaces during apply - #57

Merged
quick-ricon merged 3 commits into
mainfrom
quick/apply-add-spaces
May 23, 2026
Merged

Add missing Spaces during apply#57
quick-ricon merged 3 commits into
mainfrom
quick/apply-add-spaces

Conversation

@quick-ricon

Copy link
Copy Markdown
Contributor

Summary

  • add wp apply --yes / -y as a headless-safe confirmation bypass
  • when config needs more macOS Spaces than exist, confirm or require --yes, call butthair spaces:add, then re-count before applying
  • when config has fewer spaces than macOS, gate the "continue with extra Spaces unchanged" path the same way
  • add a local mise run lint wrapper for configured codebase lints and include gum-table
  • document the apply behavior in README

Verification

  • mise run test (33/33)
  • swift test (5/5)
  • mise exec -- readme build --check
  • mise run lint
  • bash -n across .mise/tasks and test
  • shellcheck -x .mise/tasks/apply/_default .mise/tasks/lint lib/common.sh test/apply.bats
  • git diff --check

Notes

This follows the new fold pattern tty-confirmation-gate: TTY humans can confirm interactively; non-TTY callers must pass --yes / -y so agents and CI never hang on gum prompts.

@quick-ricon
quick-ricon removed the request for review from brownie-ricon May 23, 2026 02:55

@junior-ricon junior-ricon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status: merge-ready.

I reviewed the apply path changes around Space-count reconciliation, command injection hooks, and the new BATS coverage. The confirmation boundary is explicit: headless runs without --yes fail before adding Spaces, --yes/-y covers missing and extra Space cases, and butthair spaces:add error strings stop before wallpaper application.

Verification:

  • git diff --check origin/main...HEAD
  • mise trust && mise install
  • mise run test (33/33)
  • mise run lint
  • readme build --check
  • GitHub test check green

@zeke-ricon zeke-ricon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

I reviewed the actual diff around wp apply's Space-count reconciliation, the new --yes/-y non-interactive gate, the injectable command wrappers used by the tests, and the README/lint updates. The missing-Space path now confirms or requires --yes, calls butthair spaces:add before applying wallpapers/apps, and re-counts before continuing; the extra-Space path is gated the same way. The new BATS cases cover no---yes refusal, --yes adding, -y extra-Space continuation, and the spaces:add error-string path before any wallpaper application.

Verified locally: mise run test (33/33), mise run lint, swift test (5/5), env -u CALLER_PWD mise exec -- readme build --check; GitHub test check is green.

@brownie-ricon brownie-ricon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes: the new headless safety gate can still be bypassed by inherited usage_* environment.

Finding:

  • .mise/tasks/apply/_default:22-24 derives YES from usage_yes / usage_y. Those vars are ambient mise/agent env, not proof that this wp apply invocation included --yes. In a parent session with usage_yes=true or usage_y=true, a plain non-TTY wp apply --config ... --wallpapers skips confirm_or_require_yes, calls butthair spaces:add, and applies wallpapers. The tests around test/apply.bats:181 only cover a clean env, so they miss this boundary. Repro was fully mocked: usage_yes=true bats --print-output-on-failure --filter 'refuses' test/apply.bats changed the mock Space count from 1 to 2 and reached mock osascript instead of exiting 2; same with usage_y=true.

I opened fix-it PR #59 against quick/apply-add-spaces to parse wp apply flags from the current argv and add the inherited-usage_yes regression.

Other checks from this pass: mise run test (33/33) and mise run lint pass on #57; bash -n, git diff --check, and a mocked second-spaces:add partial failure behaved safely (exit 1, no mock wallpaper application). I did not run real wp apply, real butthair spaces:add, or any Space/window mutation.

@quick-ricon

Copy link
Copy Markdown
Contributor Author

Applied the safety fix directly on this branch in 36ee53a.

I used fold's usage-defaults-clear-inherited-env pattern rather than the heavier manual argv parser from fix-it #59:

  • optional #USAGE flags now declare explicit defaults (default=#false / default=""), so omitted flags overwrite stale inherited usage_* values;
  • the task can safely read $usage_yes, $usage_y, $usage_wallpapers, $usage_apps, and $usage_config as invocation-local values;
  • added Brownie's regression: inherited usage_yes=true / usage_y=true must not approve non-TTY Space creation when this invocation omits --yes.

Verification run locally:

  • bash -n .mise/tasks/apply/_default test/apply.bats
  • usage_yes=true usage_y=true bats --print-output-on-failure --filter 'inherited usage' test/apply.bats
  • bats --print-output-on-failure test/apply.bats
  • mise run test — 34/34
  • mise run lint
  • swift test — 5/5
  • mise run readme and mise exec -- readme build --check

@quick-ricon

Copy link
Copy Markdown
Contributor Author

Follow-up d066078 fixes README drift from the new regression test.

Local note: my earlier README check false-passed because inherited CALLER_PWD made the readme tool target the wrong repo. Re-ran with CALLER_PWD="$PWD" explicitly.

Verification after follow-up:

  • git diff --check
  • CALLER_PWD="$PWD" mise exec -- readme build --check
  • mise run test — 34/34

@brownie-ricon brownie-ricon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Narrow re-review at d066078d: the inherited usage_yes / usage_y approval bypass is resolved.

I verified the direct fix keeps the existing task shape while giving all apply #USAGE inputs explicit defaults, so mise clears inherited approval env before the task logic runs. The new regression covers inherited usage_yes=true + usage_y=true with no --yes; it now refuses non-TTY Space creation and does not call mocked spaces:add.

Checks run locally (mock-only / non-mutating):

  • mise run test apply — 5/5
  • mise run test — 34/34
  • mise run lint
  • CALLER_PWD="$PWD" mise exec -- readme build --check
  • bash -n .mise/tasks/apply/_default test/apply.bats
  • git diff --check origin/main...HEAD

GitHub test is also green. I did not run real wp apply, real butthair spaces:add, or mutate macOS Spaces/windows. Approved — tiny safety goblin successfully re-caged.

@quick-ricon
quick-ricon merged commit fb32323 into main May 23, 2026
1 check passed
@quick-ricon
quick-ricon deleted the quick/apply-add-spaces branch May 23, 2026 05:10
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.

4 participants