Validate specs when they are built - #23
Conversation
cli, flag, option, argument and command accepted anything. An empty long name crashed opt-long-name on every invocation with a substring error that never mentioned the spec; a long name without "--" stored values under a mangled key so parsed-ref returned #f while help printed the raw name; a non-string command name parsed fine and then crashed generate-help halfway through its output; and two options with the same long name shared one alist entry, so setting one changed the value attributed to the other. Each builder now checks its arguments and raises with a message naming the builder and the offending value, so a malformed spec fails where it is written. cli and command also reject a duplicate option, argument or command name within their own level; a subcommand may still reuse a top-level option name, since that shadowing is what makes its own option win after its token. The reserved-name check for -h/--help runs before the shape checks so it keeps its more specific message. Closes #11 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Baiju Muthukadan <baiju.m.mail@gmail.com>
baijum
left a comment
baijum
left a comment
There was a problem hiding this comment.
Reviewed and verified locally: full suite in a worktree (197 passed, 0 failed, matching the description) and examples/greeter.scm still runs. The design checks out — failing at the point the spec is written, keeping the #22 reserved-name message, and allowing cross-level shadowing all verified, and I probed edge cases (-= short names, the reserved-before-shape ordering, duplicate detection across namespaces) with a scratch script.
The one substantive gap is inline on the not a spec built by ... gate: it only checks the head symbol, so tag-shaped lists with missing fields either crash inside check-specs with a raw interpreter error or pass validation and crash at parse time. Everything else is nits (one irritant, one docs sentence, an ordering test) plus one design question about same-level argument/command name collisions.
| (let ((s (car ss))) | ||
| (unless (and (pair? s) | ||
| (memq (car s) '(flag option argument command))) | ||
| (spec-error who "not a spec built by flag, option, argument or command" s)) |
There was a problem hiding this comment.
The tag check verifies only the head symbol, not that the list has builder arity, so tag-shaped lists with missing fields still reach the accessors below. I probed this on the branch: '(flag "-x") passes the gate and dies at opt-long with a raw type error in 'car' instead of a spec-error, and '(option "-n" "--count" "N") (missing the default slot) passes validation entirely and crashes at parse time in opt-default — exactly the far-away failure this PR exists to eliminate. If hand-written specs are out of contract that's fine, but the message then slightly overpromises; checking length per tag here (flag/command 4, option 5, argument 3) would make the gate airtight.
There was a problem hiding this comment.
Made airtight in c4797cc. check-spec now requires the builder's length per tag (flag/command 4, option 5, argument 3) and re-runs the builder's own field checks on every element, recursing into a command's specs, so a hand-written list is held to exactly the rules a built one is. '(flag "-x") and '(option "-n" "--count" "N") both raise the not-a-spec message at definition; '(flag "-x" "x" "X") raises the long-name rule. Four tests cover these.
| (spec-error who "duplicate long option name" (opt-long s))) | ||
| (loop (cdr ss) (cons (opt-short s) shorts) (cons (opt-long s) longs) | ||
| args cmds)) | ||
| ((eq? (spec-type s) 'argument) |
There was a problem hiding this comment.
Question, not a blocker: an argument whose name equals a command name at the same level is still accepted, and since the parser consults find-command before a bare word falls through to positionals, that token always dispatches the command — the argument can never receive its own name as a value (verified: (cli "t" "T" (argument "init" ...) (command "init" ...)) parsing "init" yields parsed-command = "init"). Cross-level option shadowing is documented as deliberate (#6); is same-level argument/command shadowing intended too? If yes, a line in the README paragraph would help; if not, a cross-namespace check alongside the others here would catch it.
There was a problem hiding this comment.
Not intended, and it can never work, so it is rejected in c4797cc: argument name is also a command name (or the reverse, whichever comes second). Both orders are tested, and the README paragraph mentions it. Cross-level option shadowing stays allowed as before.
| (define (check-name who what v) | ||
| (check-string who what v) | ||
| (when (= (string-length v) 0) | ||
| (spec-error who (string-append what " must not be empty"))) |
There was a problem hiding this comment.
Nit: this branch raises without the offending value while the string and dash-leading branches pass it as an irritant. It is always the empty string here, so nothing is lost, but passing v anyway would keep the three messages uniform.
There was a problem hiding this comment.
Done in c4797cc; the empty-name branch passes v like the other two.
| (command name description spec ...) ; subcommand with its own specs | ||
| ``` | ||
|
|
||
| Every builder validates its arguments when the spec is built and raises |
There was a problem hiding this comment.
The paragraph covers the string, shape, and duplicate rules but leaves out two the code now enforces: command and argument names must be non-empty and must not start with -, and every spec element must come from a builder (a stray non-spec is rejected). One extra sentence would cover both.
There was a problem hiding this comment.
Extended in c4797cc: the paragraph now covers non-empty, no leading -, builder-only elements, and the new argument/command collision.
| ;; --- Spec validation --- | ||
| (display "=== Spec validation ===") (newline) | ||
|
|
||
| ;; the issue's table: each used to crash later or corrupt keys |
There was a problem hiding this comment.
Nit: the description says the reserved-name check runs before the shape checks so -h keeps its more specific message, but no test pins that ordering. A check like (flag "-h" "" "X") expecting the reserved message would lock it in.
There was a problem hiding this comment.
Pinned in c4797cc with (flag "-h" "" "X") expecting the reserved-name message.
The spec gate checked only the head symbol, so a tag-shaped list with missing fields either crashed inside check-specs with a raw accessor error or passed and crashed at parse time, the far-away failure this validation exists to remove. Each element is now checked for the builder's length and its fields pass the builder's own checks, with a command's specs validated recursively. An argument named like a command at the same level is rejected too: the parser matches commands first, so that argument could never receive its own name as a value. The empty-name branch passes the offending value like its siblings, the README lists the non-empty, no-dash and builder-only rules, and a test pins that the reserved-name check runs before the shape checks. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Baiju Muthukadan <baiju.m.mail@gmail.com>
|
Review addressed in c4797cc:
Suite: 204 passed, 0 failed. |
1 participant
Closes #11. The spec builders accepted anything, and every malformed spec failed somewhere else: at the first invocation, halfway through a help page, or silently by corrupting keys.
What is checked
Each builder validates its arguments and raises with a message naming the builder and the offending value, so a bad spec fails where it is written:
flag,option-plus one character; long name is--plus a name without=;-h/--helpreserved in either slot (unchanged from #22, now checked first so it keeps the more specific message); description is a stringargument-; description is a stringcommandargument; description is a string; specs validated as belowclicli,commandspecsA subcommand may still reuse a top-level option name. That shadowing is what makes its own option win after its token (#6), so it is deliberately not a duplicate.
The issue's table, now
""substring: index 0 out of rangeon every invocationflag: long name must be "--" followed by a name without "=" ""at definition"count""unt",parsed-refreturns#f, help printscountoption: long name must be "--" followed by a name without "=" "count"(command 42 ...)generate-helpcrashes mid-outputcommand: command name must be a string 42--count-b 9changes the value attributed to-acli: duplicate long option name "--count"Tests
25 new checks: every row above plus
--alone, a long name containing=, short names that are missing the dash, too long, or long-shaped, non-string short name, description, command name and app name, empty and dash-leading command and argument names, duplicates of all four kinds at the top level and inside acommand, a stray non-spec in both, and four still-allowed shapes (shadowed subcommand option, the same argument name in two commands, a hyphenated long name, a non-string option default). Suite: 197 passed, 0 failed on Kaappi v0.27.1.Docs
README: a paragraph under Spec Builders listing the rules. CHANGELOG: one Changed entry, since a spec that used to be accepted now raises.
🤖 Generated with Claude Code