Skip to content

fix(parser): keep terminator tail positional - #21

Merged
steipete merged 1 commit into
mainfrom
codex/fix-terminator-remaining-option
Aug 13, 2026
Merged

fix(parser): keep terminator tail positional#21
steipete merged 1 commit into
mainfrom
codex/fix-terminator-remaining-option

Conversation

@steipete

Copy link
Copy Markdown
Owner

Summary

  • keep tokens after a bare -- positional
  • require a remaining option to be selected explicitly before it owns the raw tail
  • preserve explicit remaining-option behavior and add a regression test

Why

CommandParser previously routed a terminator tail into the first declared remaining option even when the user never selected that option. That made -- act like an implicit option invocation instead of only ending option parsing.

Proof

  • swiftformat --lint .
  • swiftlint lint --config .swiftlint.yml --strict
  • swift test --parallel — 65 tests passed
  • swift build -c release — warning-free on Swift 6.4
  • P0–P2 autoreview — no actionable findings
  • source-blind external client: bare terminator, explicit remaining option, varied tails, empty input, and unknown-option rejection all passed

@clawsweeper

clawsweeper Bot commented Aug 13, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

@clawsweeper clawsweeper Bot added 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. labels Aug 13, 2026
@clawsweeper

clawsweeper Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 13, 2026, 5:25 AM ET / 09:25 UTC.

ClawSweeper review

What this changes

This PR changes command parsing so tokens after a bare -- remain positional unless the user explicitly selected a remaining option such as --rest.

Merge readiness

⚠️ Ready for maintainer review - 1 item remains

Keep this owner-authored PR open for normal merge review. The patch cleanly makes a bare terminator positional while preserving explicitly selected remaining-option behavior, with focused regression coverage. Likely related people: steipete (high-confidence parser-area contributor).

Priority: P2
Reviewed head: d7103f4b0302e6d56b17b5dd29b701ad9ce01b5d

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) The implementation is focused, matches the established parser contract, and includes direct regression coverage.
Proof confidence 🌊 off-meta tidepool Not applicable: This owner-authored parser PR is not subject to the external-contributor real-behavior-proof gate; the body also reports focused test and build validation.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This owner-authored parser PR is not subject to the external-contributor real-behavior-proof gate; the body also reports focused test and build validation.
Evidence reviewed 5 items Current-main defect: Current main selects the first remaining option before parsing and routes a bare terminator tail into it, even when that option was never selected.
Patch and regression coverage: The PR removes implicit remaining-option ownership at the terminator branch and adds a test asserting -- tail yields a positional tail while rest remains unset; the existing test now covers explicit --rest behavior.
Established parser contract: The README defines remaining as consuming raw tokens after the option, supporting explicit option selection rather than implicit capture by a bare terminator.
Findings None None.
Security None None.

How this fits together

Commander tokenizes CLI arguments, then groups them into positional values, options, and flags for command resolution. The terminator branch determines whether later raw tokens are treated as positional input or option-owned input.

flowchart LR
A[CLI argument tail] --> B[Tokenizer]
B --> C[Command parser]
C --> D{Bare terminator}
D --> E[Positional tail]
C --> F[Explicit remaining option]
F --> G[Raw option tail]
E --> H[Parsed command values]
G --> H
Loading

Before merge

  • Resolve merge risk (P1) - Merging changes unreleased-main behavior for consumers who relied on a bare -- implicitly filling a remaining option; those consumers must select that option explicitly.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Change scope production +2, -15; tests +13, -1; release notes +1 The patch is narrowly confined to one parser branch, direct regression coverage, and an unreleased changelog entry.

Merge-risk options

Maintainer options:

  1. Accept the corrected terminator contract (recommended)
    Land the focused correction with its regression test; consumers relying on unreleased implicit capture can use the explicit remaining option.
  2. Pause for compatibility policy
    Hold the change only if maintaining the implicit bare-terminator behavior is an intentional public contract.

Technical review

Best possible solution:

Keep bare-terminator handling independent of options, retain --rest as the explicit raw-tail path, and communicate the corrected parser contract at release time.

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

Yes. Current main visibly routes parse(arguments: ["--", "tail"]) into the first remaining option when present, while the proposed regression case establishes the corrected positional result.

Is this the best way to solve the issue?

Yes. Removing only the implicit terminator-to-option branch is the narrowest maintainable fix and preserves the documented explicit remaining-option path.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against e23983764748.

Labels

Label changes:

  • add P2: This is a bounded parser-correctness repair with limited blast radius.
  • add merge-risk: 🚨 compatibility: The parser’s behavior changes for consumers that relied on a bare terminator implicitly populating a remaining option.
  • 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: This owner-authored parser PR is not subject to the external-contributor real-behavior-proof gate; the body also reports focused test and build validation.

Label justifications:

  • P2: This is a bounded parser-correctness repair with limited blast radius.
  • merge-risk: 🚨 compatibility: The parser’s behavior changes for consumers that relied on a bare terminator implicitly populating a remaining option.
  • 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: This owner-authored parser PR is not subject to the external-contributor real-behavior-proof gate; the body also reports focused test and build validation.

Evidence

What I checked:

  • Current-main defect: Current main selects the first remaining option before parsing and routes a bare terminator tail into it, even when that option was never selected. (Sources/Commander/Parser/CommandParser.swift:50, e23983764748)
  • Patch and regression coverage: The PR removes implicit remaining-option ownership at the terminator branch and adds a test asserting -- tail yields a positional tail while rest remains unset; the existing test now covers explicit --rest behavior. (Tests/CommanderTests/ParserTests.swift:39, d7103f4b0302)
  • Established parser contract: The README defines remaining as consuming raw tokens after the option, supporting explicit option selection rather than implicit capture by a bare terminator. (README.md:162, e23983764748)
  • Feature history: Parser history shows the remaining-value behavior originated in the recent parser work and has since received several focused parser fixes by the same contributor. (Sources/Commander/Parser/CommandParser.swift:81, 0c010b87fdda)
  • Release context: The current base identifies as v0.2.4-20-ge239837; this correction is on the PR head and is not part of the latest v0.2.4 release. (CHANGELOG.md:20, d7103f4b0302)

Likely related people:

  • steipete: Current parser and tokenizer history attributes 17 relevant commits to Peter Steinberger, including the original remaining-value work and recent parser validation changes. (role: parser feature author and recent area contributor; confidence: high; commits: 0c010b87fdda, 756cfce9a127, e23983764748; files: Sources/Commander/Parser/CommandParser.swift, Sources/Commander/Parser/Tokenizer.swift, Tests/CommanderTests/ParserTests.swift)

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 9140562 into main Aug 13, 2026
7 checks passed
@steipete
steipete deleted the codex/fix-terminator-remaining-option branch August 13, 2026 09:26
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