CommandArgParser: no test for a non-repeated flag given twice, and last-wins reverses the previous first-wins #548

New issue

Closed

opened 2026-09-13 09:07:04 +00:00 by mph · 3 comments

mph commented

2026-09-13 09:07:04 +00:00

Owner

core/model/CommandArgParser.scala:212 does bag.strings.updated(name, value), which overwrites, where the helper it replaced used .collectFirst and pinned first-wins with a test. There is no test for a non-repeated flag supplied twice. This affects every valued flag on all six kanon plugin commands.

Type: bug
Filed from: /home/mph/ops/kanon-DEVDOCS-246 on branch DEVDOCS-246

core/model/CommandArgParser.scala:212 does `bag.strings.updated(name, value)`, which overwrites, where the helper it replaced used .collectFirst and pinned first-wins with a test. There is no test for a non-repeated flag supplied twice. This affects every valued flag on all six kanon plugin commands. --- **Type:** bug **Filed from:** `/home/mph/ops/kanon-DEVDOCS-246` on branch `DEVDOCS-246`

claude referenced this issue

2026-09-14 23:14:29 +00:00

A dangling valued flag with no following value hard-rejects where it used to fall back to its default #550

claude commented

2026-09-15 00:16:39 +00:00

Member

Intake: needs-input

The test gap is real: --badge, a declared repeatable flag, is the only flag appearing twice anywhere in core/test/CommandArgParserTest.scala. But no document settles which rule a test would assert, so the gap cannot be closed without a decision.

Two facts correct the issue text, and they matter for the decision:

The pre-rewrite parser already did strings.updated(name, stored) (0c06acdf^:core/model/CommandArgParser.scala:136), so last-wins reverses nothing inside this parser.
The .collectFirst that rewrite removed found a missing required flag, not a value. The first-wins behaviour lived in the per-command helpers IssueCreateParser.extractFlagValue and FeedbackParser.extractFlagValue (both args.indexOf), deleted on purpose in IW-463 Phase 5 — and neither carried a test pinning first-wins.

Why no source settles it: the IW-463 2026-09-03 ruling covers only values beginning with --, not repetition; the parser doc comments are silent; the repo runs both rules live (CommandArgParser last-wins, ReviewStateCliParser.extractFlag first-wins); and the external convention is split — clap refuses a duplicated non-multiple arg by default, POSIX Guideline 11 prescribes only "interpreted in the order specified", and GNU getopt leaves the policy to the utility.

1. When a valued flag that is NOT declared repeatable is given twice (for example iw issue create --title A --title B), which must hold?

(a) Last value wins — what iw-cli does today and did before the rewrite, and the common GNU-utility convention.
(b) First value wins — what the now-deleted per-command helpers did, and what the still-live review-state flag helper still does.
(c) Refused by name as a repeated flag — what clap does by default, and what matches this parser's habit of naming what went wrong rather than silently picking.

Choosing (a) makes the work a test plus a doc line. (b) and (c) change behaviour on every valued flag of every declared command.

2. review-state write and review-state update do not go through the declared parser; they read flags with a helper that takes the first occurrence. Should they be brought to the same rule as your answer above, or is that a separate issue?

Note on part (i) of the issue: a characterization test pinning today's behaviour is not independent of this decision — it is option (a).

Not a duplicate of #549, #550 or #552: each names a different branch of the same function, and fixing any of them leaves the duplicate-flag case untested.

Intake: needs-input The test gap is real: `--badge`, a declared *repeatable* flag, is the only flag appearing twice anywhere in `core/test/CommandArgParserTest.scala`. But no document settles which rule a test would assert, so the gap cannot be closed without a decision. **Two facts correct the issue text**, and they matter for the decision: 1. The pre-rewrite parser already did `strings.updated(name, stored)` (`0c06acdf^:core/model/CommandArgParser.scala:136`), so last-wins reverses nothing *inside this parser*. 2. The `.collectFirst` that rewrite removed found a missing required flag, not a value. The first-wins behaviour lived in the per-command helpers `IssueCreateParser.extractFlagValue` and `FeedbackParser.extractFlagValue` (both `args.indexOf`), deleted on purpose in IW-463 Phase 5 — and neither carried a test pinning first-wins. Why no source settles it: the IW-463 2026-09-03 ruling covers only values beginning with `--`, not repetition; the parser doc comments are silent; the repo runs both rules live (`CommandArgParser` last-wins, `ReviewStateCliParser.extractFlag` first-wins); and the external convention is split — clap refuses a duplicated non-multiple arg by default, POSIX Guideline 11 prescribes only "interpreted in the order specified", and GNU `getopt` leaves the policy to the utility. **1.** When a valued flag that is NOT declared repeatable is given twice (for example `iw issue create --title A --title B`), which must hold? - **(a)** Last value wins — what iw-cli does today and did before the rewrite, and the common GNU-utility convention. - **(b)** First value wins — what the now-deleted per-command helpers did, and what the still-live `review-state` flag helper still does. - **(c)** Refused by name as a repeated flag — what clap does by default, and what matches this parser's habit of naming what went wrong rather than silently picking. Choosing (a) makes the work a test plus a doc line. (b) and (c) change behaviour on every valued flag of every declared command. **2.** `review-state write` and `review-state update` do not go through the declared parser; they read flags with a helper that takes the first occurrence. Should they be brought to the same rule as your answer above, or is that a separate issue? Note on part (i) of the issue: a characterization test pinning today's behaviour is not independent of this decision — it *is* option (a). Not a duplicate of #549, #550 or #552: each names a different branch of the same function, and fixing any of them leaves the duplicate-flag case untested.

claude added the

needs-input

label

2026-09-15 00:16:39 +00:00

mph commented

2026-10-10 20:12:42 +00:00

Author

Owner

1: a, 2: yes, same rules

claude removed the

needs-input

label