fix: one registrar for every repeatable flag; reject an empty occurrence - #111
Merged
Conversation
Repeatable string flags were hand-registered as pflag StringSlice in nine files. StringSlice CSV-splits each occurrence, so an empty one is destroyed during parsing: `--user-id "" --user-id REAL` reaches the command as ["REAL"]. On `apps set-owners`, which replaces the full owner list, an unset shell variable silently set one owner and exited 0; the per-value check in that command could only ever catch a lone empty. Add addRepeatableStringFlag (always StringArray) and repeatableStringFlag (rejects an empty or whitespace-only occurrence, including the lone-empty case that reads back as a zero-length slice with Changed set) to cmd/flags.go, and convert all thirteen flags across ten commands to them. The bespoke checks in apps_set_owners.go and tasks_reassign.go are deleted, leaving one implementation and one wording of the rule. cmd/repeatable_flags_test.go mirrors the pagination guards: an AST guard so no file outside flags.go may register such a flag, a tree guard so no live flag is a stringSlice however it was wired up, and a pinned list so a command cannot quietly drop one. BREAKING: `--flag a,b` is now one value, not two. No repeatable flag ever documented comma-splitting; the API now rejects the joined value by name rather than the CLI silently splitting it. Documented in CHANGELOG.
Both were already stale (ten commands, fourteen flags), and a count in a comment is the drift this change exists to stop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…flag Two searches read their repeatable flags after constructing the client, so an empty value exited 3 on a missing credential instead of 2 with the reason. Both reads are pure and now happen first, matching every other converted command. Measured with no credentials: exit 2, naming the flag. The registrar guard pinned how these flags are registered but nothing pinned how they are read, so reading one directly reverted the fix silently for six of ten commands. They now have rows in the exit-code test. A wrong flag type no longer reports as an empty value, which sent the reader to fix their command line instead of the code; an unregistered name is an error rather than a silent empty, which is the shape this all exists to stop. The one-value-per-occurrence rule is in the README and the agent doc, not only the changelog — a comma in --config-field is the edge the server may accept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rong reason The repeatable-flags bullet landed above agents.md's YAML block, so the shipped output of docs agents opened with a list item and name/description/ version stopped being front matter at all. A test now pins that the doc opens with the block and carries the four keys the help text promises harnesses parse; the whole suite was green while it was broken. Two of the seven new exit-code rows never reached the accessor: mcp tools search marks --app-id and --connector-id required, and cobra's own error is exit 2 as well, so they passed while proving nothing. The rows now supply those flags, and every repeatable row pins the accessor's own wording, so a row that exits 2 by another path fails. mcp servers register gains --catalog-id for the same reason: it was passing on ordering luck. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Correcting the previous commit message: it said every repeatable row pins the accessor's wording. Seven did; nine did not. The four lone-empty rows survive deleting the registrar's Changed branch because each command keeps its own length fallback, so they passed while proving nothing about the shared accessor. They are pinned now, as is --config-field, whose read was the one entry in the registrar map with no end-to-end coverage. The front-matter check searched for the next --- anywhere, so a dropped delimiter would scan into the body and report a body-sized block as valid. It now requires the block to close before the first blank line. Also drops a blank line that made the whole gotchas list loose in CommonMark. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Correcting the previous commit message: it justified the check by saying a dropped delimiter would scan into the body and accept a body-sized block. That needs a second "---" line in the body, and this file has none — the pre-existing "never closed" check already catches the dropped delimiter. Meanwhile the new check failed legitimate front matter: a blank line between keys, or a block scalar containing one, both reported "not closed before the body begins". A guard that rejects correct input gets deleted, so it is gone. Also pins the invalid --type row, which passed with its guard deleted: the value falls through the switch and trips a later check naming a flag the caller never passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reverting the blank-line check removed a false positive and left a false negative: drop the closing --- and the delimiter search runs on to any later --- in the body, reporting twenty lines of prose as valid front matter with the whole suite green. The block is parsed instead, using the yaml package this repo already depends on, against the rendered doc a harness consumes. A blank line between keys now passes, which is what the old check got wrong. Three more rows exited 2 by another path when their own guard was removed: bindings history fell through to the empty-path-segment error, invalid --auth to a different row's guard, and a malformed --config-field pair to "nothing to update". Each pins its own message now, proven by deleting the guard it targets. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
--user-id "" --user-id REALsilently dropped the empty value on everyrepeatable string flag, because pflag's
StringSliceCSV-parses eachoccurrence before any command-level check can see it. Measured against pflag
v1.0.10:
apps set-ownersreplaces the full owner list, so an unset shell variableremoved an intended owner and exited 0. Reproduced against a binary built from
main: two occurrences in, one id in the body.
One rule, one place
addRepeatableStringFlagregistersStringArray;repeatableStringFlagreadsit and returns a usage error (exit 2) on any empty or whitespace occurrence,
including a lone
--flag ""— which round-trips to an empty slice, soChangedis the only discriminator. 14 flags across 10 commands go through both, and the
bespoke check in
apps set-ownersis gone, leaving one implementation.A guard fails the build on any hand-registration or any use of
StringSlice,mirroring
TestPaginationFlagsGoThroughSharedRegistrar, including itsnon-empty assertion so it cannot pass by enumerating nothing.
Breaking
--flag a,bnow means one value, not two. No help text ever documented theseas comma-splittable; they all say "repeatable". The break is loud — the server
names the bad value — except on
--config-field, where"region=us1,env=prod"becomes one field, so the rule is now in the README and the agent doc alongside
the contrast with
--fields, which is comma-separated.Verification
--debugshowing zeroHTTP lines was validated against a request that does send.
polling, since owner writes are async.
pins the message, so one exiting 2 by another path fails.
🤖 Generated with Claude Code