Cover the argument parses that produce a different run rather than an error - #681
Merged
Conversation
… error 91 examples over six seams that had none, picked by what they cost when wrong. Every one of them can be wrong and still exit 0: a flag that mis-parses does not raise, it sends different bytes, stores the rule somewhere else, or no-ops a mutation while reporting success. `gori run rewriter` had no spec at all — 710 lines managing Match & Replace and the `extract` sub-CRUD that mints session bindings. The scope letter leads a rule row because the project and global stores number independently, so `#3` alone does not say which rule the next command addresses; the JSON projection is what a script reads, and `default_enabled` beside `enabled` is the only way that script can tell an override from a library default. Both are now pinned, along with the two refusals `add` makes before a rule reaches a store: an unparseable stub (it would answer every matching request with gori's own 502) and `--part=ws` on an op that is not `replace` (normalising it would move the rule to a different PROTOCOL, silently). The payload flag parsers `fuzz` and `discover` share decide what leaves the machine, and each has already been wrong in a way that produced a quietly different set: `--numbers -10--5` split on the first hyphen and lost its lower bound, `--regex-replace /foo//bar/` dropped everything past the second delimiter, `--brute` split its charset on the wrong colon. Checked through what each source actually generates, since none of them expose their fields. `Run.list_leftover_error` — the seam behind the twelve list dispatchers, split out of `refuse_list_leftovers` precisely so the decision and the wording could be spec'd, and then never spec'd. It is what stops `gori run rewriter --project=t1 rm 1` from listing the rules and exiting 0 with the delete discarded, and what keeps a lone `list` from being called an unknown subcommand by a message that lists `list` among the verbs. `probe`'s flag parses, and the rule-id decode that says which STORE an id addresses: `custom_p_12` is a row in this project's DB, `custom_g_12` lives in settings.json. Reading the global one as a row id would send a delete at whatever project row happened to share the number. `Settings.scan_rules` was the only file in the tree whose entire public API went unmentioned anywhere under spec/. Its parse is the boundary a hand-edited settings.json crosses into the match engine, which reads side/region/kind/severity as though they were enums — so the clamp is the contract, and a rule missing id/title/pattern is dropped rather than defaulted, because an invented pattern would scan live traffic for it. The delete of the LAST rule is checked on disk, not through a reload: an emptied section stops being serialized, which is the exact shape that has made `save`'s merge copy a stale block forward before. `Rules.normalize_shape`, the one place a rule's {target, part} pair is settled and every creating surface calls. The `abort` branches remain out of reach — `abort` calls `exit`, which would take the suite down — so these cover success paths only, the limit spec/cli/run/links_spec.cr already works under. Suite: 8592 examples, no new failures.
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
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.
91 examples over six seams that had none, picked by what they cost when wrong. Every one of them can be wrong and still exit 0: a flag that mis-parses does not raise, it sends different bytes, stores the rule somewhere else, or no-ops a mutation while reporting success.
spec/cli/run/rewriter_spec.crgori run rewriter— 710 lines that had no spec at allspec/cli/run/fuzz_args_spec.crfuzzanddiscoversharespec/settings_scan_rules_spec.crspec/cli/run/probe_helpers_spec.crprobe's flag parses + rule-id decodingspec/cli/run/list_leftovers_spec.crspec/rules_spec.crRules.normalize_shapeHow these were picked
Crystal has no working coverage tooling (kcov is Linux-only,
crystal-coverageis dead), so this used an identifier-index heuristic instead: tokenise all ofspec/, then count how many of each source file'sdefnames appear in that set.The useful result was the negative one. gori's public surface is already essentially covered — of 581 source files, exactly one (
settings/scan_rules.cr) had its entire publicdef self.surface unmentioned anywhere underspec/. So the remaining gaps are in two places, and all six seams here come from them:private def self.helpers insidecli/run/*.cr, whose names never appear in a spec because the parsing and formatting are all privateRun.list_leftover_errorsays exactly that and had zero coverageWhat each one costs when wrong
rewriter — the scope letter leads a rule row because the project and global stores number independently, so
#3alone does not say which rule the next command addresses. The JSON projection is what a script reads, anddefault_enabledbesideenabledis the only way that script can tell an override from a library default. Also the two refusalsaddmakes before a rule reaches a store: an unparseable stub (it would answer every matching request with gori's own 502) and--part=wson an op that is notreplace(normalising it would move the rule to a different PROTOCOL, silently).fuzz/discover flags — each has already been wrong in a way that produced a quietly different payload set:
--numbers -10--5split on the first hyphen and lost its lower bound,--regex-replace /foo//bar/dropped everything past the second delimiter,--brutesplit its charset on the wrong colon. Checked through what each source actually generates, since none of them expose their fields.list_leftover_error — what stops
gori run rewriter --project=t1 rm 1from listing the rules and exiting 0 with the delete discarded, and what keeps a lonelistfrom being called an unknown subcommand by a message that listslistamong the verbs.probe rule ids —
custom_p_12is a row in this project's DB,custom_g_12lives in settings.json. Reading the global one as a row id would send a delete at whatever project row happened to share the number.scan_rules — its parse is the boundary a hand-edited
settings.jsoncrosses into the match engine, which reads side/region/kind/severity as though they were enums, so the clamp is the contract. A rule missing id/title/pattern is dropped rather than defaulted, because an invented pattern would scan live traffic for it. The delete of the LAST rule is checked on disk, not through a reload: an emptied section stops being serialized, which is the exact shape that has madesave's merge copy a stale block forward before.Limits
The
abortbranches remain out of reach —abortcallsexit, which would take the suite down — so these cover success paths only, the limitspec/cli/run/links_spec.cralready works under. Reaching the refusal paths would need a harness that runs the built binary as a subprocess; there isn't one today, and adding it was left out of scope.Verification
crystal spec: 8592 examples, 8 failures — the same 8 as before this branch (project_registry×5,capture_status×3), all from a gori already listening on:8070. No new failures.crystal tool format --checkand ameba: clean on every file touched.spec/cli_run_spec.cr.