Repository navigation
fix(decompose): a bare --limit or --max-scrapes is refused, not read as 1 - #277
Merged
Merged
Conversation
…as 1
What changed: bin/decompose.mjs routes --limit and --max-scrapes through the checkFlagValues call at the top of the CLI, with { int: true, min: 0 } specs beside the four value flags, and drops the hand-written Number(raw) loop (and the `numbers` object it iterated) that ran later. The refusal for a flag with no value is "--<name> needs a value"; a non-number or a negative is "--<name> must be a whole number of at least 0, not "<value>"" (it was "must be a whole number, not"). Three rows in cli.test.mjs's bad-value table cover a bare --max-scrapes, a bare --limit and --limit abc; the test count is unchanged (rows in one registration, README stays 1786).
Why: parseFlags reads a flag with no value as `true`, and Number(true) is 1, so the loop accepted it: `decompose --dry-run --max-scrapes` reported "scrape up to 1 of the pages found" and exited 0, and without --dry-run it scraped - and paid for - one page nobody asked for; a bare --limit kept one search result instead of eight (#269, found by the owner 2026-10-11). checkFlagValues exists for exactly this and decompose already called it for its other value flags. This supersedes PR #273's fix of the same defect by LetMeSleep8h, which had the right remedy but left `numbers` defined and unused, carried no regression test, and shared its README hunk with PR #275; the remedy is theirs and is credited below. Alternative set aside: keeping the loop and adding a `=== true` check to it - two validators for one flag, one of them home-made, which is how the gap arose. No ADR: a bug fix with one obvious remedy.
What it touched: research-kit/bin/decompose.mjs (the checkFlagValues specs; the loop removed); research-kit/test/cli.test.mjs (three rows); docs/ARCHITECTURE.md (the decompose.mjs row gains the clause); CHANGELOG.md (Unreleased). No new flag, format, key or check; the documented flags and their defaults are unchanged.
What you verified:
- verified: red first - with only the test rows in place, cli.test.mjs failed on "decompose.mjs --dry-run --max-scrapes exited 0" (114 passed, 1 failed, 270.8 s; red-before-fix.log / red-before-fix-result.json in the evidence dir).
- verified: after the fix, cli.test.mjs + decompose.test.mjs (the cli needle also selects artifact-cli): 152 passed, 0 failed, 0 unsupported, 295.3 s, Node 24.20.0 win32 (focus-after-fix-result.json sha256 1d482d9f5ab6ca4e385148080a85d54574dd00a27c1a87795822c03d38330fcb).
- verified: by hand from an empty folder - `--dry-run --max-scrapes` -> "--max-scrapes needs a value", exit 2; `--dry-run --limit` -> "--limit needs a value", exit 2; `--dry-run --limit abc` -> "--limit must be a whole number of at least 0, not "abc"", exit 2; `--dry-run --max-scrapes -1` -> the same shape, exit 2; `--dry-run --max-scrapes 0 --topic probe` -> the dry run's three "would search" lines, exit 0.
- verified: git diff --check clean; the four files LF-only.
- untested when written: the installed commit gate's full-suite run for this commit, and its all-seven CI.
What you got wrong and fixed: nothing to report.
Co-authored-by: LetMeSleep8h <LetMeSleep8h@users.noreply.github.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.
Closes #269. Supersedes #273 (the remedy is @LetMeSleep8h's, credited with
Co-authored-byin the commit).d2c0d0d — fix(decompose):
--limitand--max-scrapesgo through thecheckFlagValuescall at the top of the CLI with{ int: true, min: 0 }specs, and the later hand-writtenNumber(raw)loop (plus thenumbersobject it iterated) is removed.parseFlagsreads a bare flag astrue,Number(true)is1, sodecompose --dry-run --max-scrapesreported "scrape up to 1" and exited 0, and a real run paid for one page nobody asked for. Now:--max-scrapes needs a value/--limit needs a value, exit 2;--limit abc→--limit must be a whole number of at least 0, not "abc". Three rows added to the bad-value table incli.test.mjs(test count unchanged, 1786).docs/ARCHITECTURE.mddecompose row andCHANGELOG.mdupdated in the same commit; five-part report in the commit message.Why not merge #273 as is: right fix, but it left
numbersdefined and unused, carried no regression test (#269 asked for the twocli.testrows), had no CHANGELOG/map/report, and its README hunk duplicates and conflicts with #275 (which fixes #268 with the test the issue suggested).Verified:
cli.test.mjsfailed ondecompose.mjs --dry-run --max-scrapes exited 0(114 passed / 1 failed).selftest.mjs cli.test.mjs decompose.test.mjs→ 152 passed / 0 failed / 0 unsupported, 295.3 s, Node 24.20.0 win32.--max-scrapes, bare--limit,--limit abc,--max-scrapes -1all refused with exit 2;--max-scrapes 0 --topic probe --dry-runruns (exit 0).Untested when opened: this PR's own CI.
Merge is the operator's.
🤖 Generated with Claude Code