Repository navigation
Reject unknown actions to background - #106
ShryukGrandhi wants to merge 1 commit into
Conversation
"removemacai background of" listed the background items and exited 0, so a typo for "off" went unnoticed. Fail with "unknown action" unless the action is off, on or missing. Fixes omlahore#82 Signed-off-by: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Validate background action Sources/removemacai/TweakCommands.swift |
The command rejects an unrecognized first argument. No argument still lists background items, and off and on continue to the action flow. |
Priority:
Estimated code review effort: 1 (Trivial) | ~5 minutes
Change: Bug fix · Severity of issue fixed: Low
Possibly related PRs
- omlahore/RemoveMacAI#97: This PR changes the same command to reject unknown actions and adds a self-test for that case.
Suggested reviewers: omlahore, bubleg
Merge Risk: 
e2a9c
-
The requested background action validation is implemented without an identified regression.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "Reject unknown actions to background" matches the main change. TweakCommands.background now rejects any action other than off or on, which the linked issue #82requests. The title is short …Linked Issues check [ #82] requires thatbackgroundaccept onlyoff,on, or no action. Any other action must fail with an error such asunknown action "of", use off or on, and the check must sit at the top of `Twe…Out of Scope Changes check The whole-PR diff against the merge base changes one file, Sources/removemacai/TweakCommands.swift, with about +4/-1 lines. Every changed line belongs toTweakCommands.background. The new validati…Full details: Title check
-
Explanation
The title "Reject unknown actions to background" matches the main change. TweakCommands.background now rejects any action other than off or on, which the linked issue
#82requests. The title is short and specific, and it names the behavior change. Minor wording issue: "to background" is slightly awkward, since the change targets the background command, but a teammate scanning history would still understand the primary change.
Full details: Linked Issues check
-
Explanation
[
#82] requires thatbackgroundaccept onlyoff,on, or no action. Any other action must fail with an error such asunknown action "of", use off or on, and the check must sit at the top ofTweakCommands.background. The reviewed head adds this check as the first statement inbackgroundinSources/removemacai/TweakCommands.swift. It runsTerm.fail("unknown action \"\(action)\", use off or on")whenargs.firstis neitheroffnoron. This runs beforeBackgroundItems.scan()anddisabledLabels(). The listing guard changed fromguard let action = args.first, action == "off" || action == "on" elsetoguard let action = args.first else. So the listing branch runs only when no action is given. Theoff/onhandling is unchanged. The diff shows no coding requirement from#82left unmet. The PR description reports manual testing on macOS. The PR adds no automated test for this path. The PR states thatTerm.failexits the process, so a selftest is not practical. The issue does not list an automated test as a requirement, so this does not fail the assessment.
Full details: Out of Scope Changes check
-
Explanation
The whole-PR diff against the merge base changes one file,
Sources/removemacai/TweakCommands.swift, with about +4/-1 lines. Every changed line belongs toTweakCommands.background. The new validation implements#82. The guard rewrite is required so the listing branch runs only when no action is given. No unrelated code, refactoring, or documentation changes appear in the diff. The PR description and objectives state the same scope.
-
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Summary
removemacai background of(a typo foroff) printed the list of background items and exited 0, so the typo went unnoticed. Onlyoff,onor no action is valid now. Anything else fails withunknown action "of", use off or onand exits 1.Changes
TweakCommands.backgroundchecks the action first, before it scans background items, and fails on anything other thanofforon.Testing
Tested on macOS 26.5.1 (25F80), Apple silicon, with a debug build:
removemacai background oflisted the items and exited 0.removemacai background ofprintserror: unknown action "of", use off or onand exits 1.removemacai backgroundstill lists the items and exits 0.removemacai background offwith no labels still fails withname the items, as listed by: removemacai background.removemacai selftestpasses.There's no selftest check for this because
Term.failexits the process. The earlier argument checks (#72, #74) don't have one either.Fixes #82
Summary by CodeRabbit
onoroff, instead of displaying the background item list.