Skip to content

fix: validate CLI mutation intent and literal arguments - #88

Merged
steipete merged 1 commit into
mainfrom
fix/phase5-cli-inputs
Sep 13, 2026
Merged

steipete merged 1 commit into
mainfrom
fix/phase5-cli-inputs

Conversation

@steipete

Copy link
Copy Markdown
Collaborator

list --delete --rename New silently chose deletion, while mutation flags without a target fell through to listing. Parse mutation flags into one validated action before touching EventKit; reject conflicts and missing targets while keeping ordinary reads and single mutations intact.

The router also treated help/version text after -- as a global control, and used the executable filename as the Commander command name. Honor the tokenizer's terminator boundary and resolve the canonical descriptor so literal titles and renamed executables work. Require finite positive geofence radii: inf, +inf, and 1e309 previously passed validation and reached geocoding.

Regression coverage exercises actual router entry points, parsed list actions, empty targets, conflicting mutation combinations, valid actions, and finite-radius boundaries. Documentation and Unreleased notes describe the corrected behavior.

Live proof: the original CLI deleted a disposable list for the conflicting delete/rename request, rejected a renamed executable, and intercepted help/version after --. The rebuilt signed CLI rejects every conflict and missing target, preserves the same list ID and contents, accepts literal --help/--version reminder titles, runs under a different executable filename, rejects nonfinite radii before geocoding, and still adds/renames/reads normally. All synthetic data was removed. Radius regression tests first failed for inf, +inf, and 1e309, then passed with the finite-value check.

Validation: strict Swift/shell/workflow lint, the full Swift suite and coverage-gate tests pass; RemindCore coverage remains 94.0% (958/1019). make build and make docs-site pass. Final isolated Codex autoreview is scoped-clean through P2.

@clawsweeper

clawsweeper Bot commented Sep 13, 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. 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 13, 2026
@clawsweeper

clawsweeper Bot commented Sep 13, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 13, 2026, 3:14 AM ET / 07:14 UTC.

ClawSweeper review

What this changes

The PR validates list mutation requests before Reminders access, preserves literal arguments after --, supports renamed executables, and rejects nonfinite geofence radii, with regression tests and documentation.

Merge readiness

Ready for maintainer review

The fixes remain necessary on current main. Review found no introduced blocking defect, and this collaborator-authored PR is not eligible for cleanup closure.

Priority: P2
Reviewed head: 5e7d6e29bf85ebe8984df9624495d8c771dc4130

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused correctness patch with useful 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 captured body reports signed native CLI runs covering list preservation, literal titles, renamed execution, and radius rejection; this review inspected source and tests but did not independently execute those runs.
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 captured body reports signed native CLI runs covering list preservation, literal titles, renamed execution, and radius rejection; this review inspected source and tests but did not independently execute those runs.
Evidence reviewed 9 items Pinned change and clean checkout: The inspected head is 5e7d6e2 against d272180. The complete introduced diff contains nine files; git diff --check and the final working-tree status were clean.
Current main still needs the fixes: The live main endpoint still resolves to the pinned base. Its list command checks deletion before renaming and reaches listing when mutation flags lack a target; its router scans help/version flags beyond the terminator.
Validation precedes native operations: List action validation now precedes store construction and requestAccess. Existing multi-list reads, unique-list reuse, deletion confirmation, and target resolution remain intact. AddCommand checks finite radii before requesting access; EventKit geocoding and saving occur downstream.
Findings None None.
Security None None.

How this fits together

remindctl translates terminal commands into Apple Reminders reads and writes through EventKit. Its command router and input validation determine which operation reaches the store and which arguments become reminder data.

flowchart TD
  A[Terminal arguments] --> B[Command router]
  B --> C[Help or version output]
  B --> D[Parse command inputs]
  D --> E{Inputs valid?}
  E -->|No| F[Error before store access]
  E -->|Yes| G[EventKit reads and writes]
  G --> H[Reminder or list output]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +48/-21 (net +27); tests +94/-0 The production growth implements explicit mutation validation and is accompanied by focused routing, action, and radius regression coverage.

Technical review

Best possible solution:

Keep command-intent validation at the CLI boundary while preserving valid list operations, literal positional data, and existing EventKit authorization.

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

Yes, from source: current main selects deletion for a named list with both delete and rename flags, intercepts controls after --, and accepts positive infinity as a radius. No native execution was performed in this read-only review.

Is this the best way to solve the issue?

Yes. The patch corrects the existing command boundary using the pinned parser's semantics and retains established valid operations without introducing configuration or storage changes.

AGENTS.md: not found in the target repository.

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

Labels

Label changes:

  • add P2: This is a bounded CLI correctness fix for conflicting mutation flags and malformed or literal inputs.
  • 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 captured body reports signed native CLI runs covering list preservation, literal titles, renamed execution, and radius rejection; this review inspected source and tests but did not independently execute those runs.

Label justifications:

  • P2: This is a bounded CLI correctness fix for conflicting mutation flags and malformed or literal inputs.
  • 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 captured body reports signed native CLI runs covering list preservation, literal titles, renamed execution, and radius rejection; this review inspected source and tests but did not independently execute those runs.

Evidence

What I checked:

  • Pinned change and clean checkout: The inspected head is 5e7d6e2 against d272180. The complete introduced diff contains nine files; git diff --check and the final working-tree status were clean. (5e7d6e29bf85)
  • Current main still needs the fixes: The live main endpoint still resolves to the pinned base. Its list command checks deletion before renaming and reaches listing when mutation flags lack a target; its router scans help/version flags beyond the terminator. (Sources/remindctl/Commands/ListCommand.swift:48, d27218000139)
  • Validation precedes native operations: List action validation now precedes store construction and requestAccess. Existing multi-list reads, unique-list reuse, deletion confirmation, and target resolution remain intact. AddCommand checks finite radii before requesting access; EventKit geocoding and saving occur downstream. (Sources/remindctl/Commands/ListCommand.swift:55, 5e7d6e29bf85)
  • Dependency contract is directly relevant: CommandRouter imports Commander and calls Program.resolve; Package.resolved pins steipete/Commander 0.2.4 at bd219c4ee9032fee3e009856f81fcc6ec09a85f4. The router changes therefore depend directly on Commander's command-name and terminator contracts. (Sources/remindctl/CommandRouter.swift:65, 5e7d6e29bf85)
  • Pinned tokenizer and router contract: The pinned tokenizer converts every argument following -- into positional data, and Program.resolve looks up the first argument by descriptor name. Both support the proposed router correction. The dependency's full AGENTS.md concerns its own Peekaboo submodule synchronization and imposes no change on this consumer. (Sources/Commander/Parser/Tokenizer.swift:18, bd219c4ee903)
  • Latest release does not supply the radius fix: GitHub identifies v0.3.6 as the latest release, published September 7. Its AddCommand still accepts any parsed radius greater than zero without checking finiteness. (Sources/remindctl/Commands/AddCommand.swift, 972357da9e14)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
  • 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 a23d297 into main Sep 13, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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