Skip to content

fix: reject surplus command arguments - #95

Merged
steipete merged 1 commit into
mainfrom
fix/strict-command-arguments
Sep 22, 2026
Merged

steipete merged 1 commit into
mainfrom
fix/strict-command-arguments

Conversation

@steipete

Copy link
Copy Markdown
Collaborator

Commander 0.2.4 silently discarded surplus positional arguments: for example, remindctl completion bash unexpected returned success. Update to Commander 0.3.0 so malformed invocations fail before command execution, and use its explicit root-argument API to preserve renamed executables without the old argv rewrite helper.

Declare the existing multi-value arguments on list, complete, and delete explicitly so strict validation retains bulk operations. Update the list-action test helper, document the behavior, and record the fix in Unreleased.

Validation: the new surplus-argument regression failed before the update; all 9 focused routing/list-action tests pass afterward, including bulk targets, renamed binaries, and literal controls after --. Swift format and strict SwiftLint pass. The docs build passed on AWS Crabbox. The release harness requires macOS csreq and is covered by the PR’s macOS CI, along with the full Swift coverage and universal-binary gates. Independent Codex autoreview found no actionable P0–P2 findings.

@clawsweeper

clawsweeper Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. 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 Sep 22, 2026
@clawsweeper

clawsweeper Bot commented Sep 22, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 22, 2026, 8:10 AM ET / 12:10 UTC.

ClawSweeper review

What this changes

Updates Commander to 0.3.0 to reject surplus arguments, preserves bulk commands and renamed executables, and adds regression tests and documentation.

Merge readiness

✅ Ready for maintainer review

This fix remains necessary: current main and v0.3.7 still accept surplus positional arguments. No blocking correctness defect was found, and the collaborator-authored PR is not eligible for automatic cleanup.

Priority: P2
Reviewed head: fd92edf632eb0ce59751dc41356cb001994b09e7

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused parser integration with relevant regression coverage and no identified blocking defect.
Proof confidence 🌊 off-meta tidepool Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate. Its reported routing tests exercise argument rejection and compatibility cases, but are not treated as a live CLI demonstration; no stored-data contract changes.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate. Its reported routing tests exercise argument rejection and compatibility cases, but are not treated as a live CLI demonstration; no stored-data contract changes.
Evidence reviewed 7 items Introduced patch: Reviewed the pinned base-to-head delta across all ten files. The router uses Commander's explicit root-argument API, and list, complete, and delete explicitly retain variadic arguments.
Main and release still need the fix: Current main retains the old resolve(argv:) path and Commander 0.2.4. The v0.3.7 release also pins Commander 0.2.4, so the submitted fix is not already shipped.
Dependency contract establishes the defect: Dependency ownership was verified through GitHub. Commander 0.2.4 rejects positionals only when the signature has no positional definitions; otherwise it returns every positional value. remindctl's completion handler reads only the first, explaining the successful malformed invocation.
Findings None None.
Security None None.

How this fits together

remindctl routes terminal arguments through Commander before executing commands against Apple Reminders. Argument validation determines whether command handlers run or the CLI returns an error.

flowchart TD
  A[Terminal arguments] --> B[Aliases and global controls]
  B --> C[Command routing]
  C --> D[Validate argument counts]
  D --> E[Reject malformed invocation]
  D --> F[Execute reminder command]
  F --> G[Apple Reminders and terminal output]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

None.

Technical review

Best possible solution:

Keep argument validation in Commander, explicitly preserve supported bulk inputs, and reject malformed requests before command execution.

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

Yes, from source: current main's parser accepts both positionals in remindctl completion bash unexpected, and the handler uses only bash. This review did not execute the CLI.

Is this the best way to solve the issue?

Yes. Upgrading the existing parser and declaring the three supported variadic arguments avoids duplicating validation in individual handlers.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 2a1da5fab273.

Labels

Label changes:

  • add P2: This is a bounded CLI correctness fix for malformed invocations.
  • add merge-risk: 🚨 compatibility: Scripts relying on ignored surplus arguments will now fail; this exact behavior change is explicitly intended by the collaborator-authored proposal and documented in the patch.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate. Its reported routing tests exercise argument rejection and compatibility cases, but are not treated as a live CLI demonstration; no stored-data contract changes.

Label justifications:

  • P2: This is a bounded CLI correctness fix for malformed invocations.
  • merge-risk: 🚨 compatibility: Scripts relying on ignored surplus arguments will now fail; this exact behavior change is explicitly intended by the collaborator-authored proposal and documented in the patch.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate. Its reported routing tests exercise argument rejection and compatibility cases, but are not treated as a live CLI demonstration; no stored-data contract changes.

Evidence

What I checked:

  • Introduced patch: Reviewed the pinned base-to-head delta across all ten files. The router uses Commander's explicit root-argument API, and list, complete, and delete explicitly retain variadic arguments. (Sources/remindctl/CommandRouter.swift:64, fd92edf632eb)
  • Main and release still need the fix: Current main retains the old resolve(argv:) path and Commander 0.2.4. The v0.3.7 release also pins Commander 0.2.4, so the submitted fix is not already shipped. (Package.resolved:10, f4326de7fa16)
  • Dependency contract establishes the defect: Dependency ownership was verified through GitHub. Commander 0.2.4 rejects positionals only when the signature has no positional definitions; otherwise it returns every positional value. remindctl's completion handler reads only the first, explaining the successful malformed invocation. (Sources/Commander/Parser/CommandParser.swift, bd219c4ee903)
  • Upgraded parser compatibility: The direct dependency declaration and changed resolve(arguments:) call establish the dependency-review boundary. Inspected the pinned router, parser, signature validator, package manifest, and dependency policy: surplus values are rejected unless the final positional is .remaining, while flags and terminator tails remain supported. All remindctl command registrations were checked against the new validation rules. (Sources/Commander/Parser/CommandParser.swift, 3f5a3cb1f19c)
  • Dependency release identity: The annotated v0.3.0 tag resolves to the exact commit pinned by this PR. The dependency source remains the existing repository; no target workflow, permission, or lifecycle-hook changes are introduced. (3f5a3cb1f19c)
  • Regression coverage and intentional compatibility change: Tests cover surplus completion arguments, three bulk commands, six single-target commands, renamed executables, and literal controls after --. The collaborator's captured PR body explicitly requests rejection of formerly accepted malformed invocations; README documents quoting multi-word values. Scripts relying on ignored trailing words will now fail intentionally. Reported test success is supplemental evidence; no tests were executed during this read-only review. (Tests/remindctlTests/CommandRouterTests.swift:6, fd92edf632eb)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

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.

@steipete
steipete merged commit b6f2c67 into main Sep 22, 2026
10 checks passed
@vincentkoc
vincentkoc deleted the fix/strict-command-arguments branch September 25, 2026 11:25
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: 🐚 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant