Skip to content

Reject a -sites list with no site keys instead of panicking - #23

Merged
Sriram-PR merged 4 commits into
Sriram-PR:mainfrom
Detoy:fix/empty-sites-list
Oct 7, 2026
Merged

Sriram-PR merged 4 commits into
Sriram-PR:mainfrom
Detoy:fix/empty-sites-list

Conversation

@Detoy

@Detoy Detoy commented Oct 7, 2026

Copy link
Copy Markdown

Fixes #13.

resolveSiteKeys accepted any non-empty -sites string, even one with no keys in it. For crawl -sites "," that meant an empty list on the single-site path, so siteKeys[0] panicked with exit 2. For watch -sites "," the scheduler started with zero sites and idled forever.

Now -sites only counts as a selector when it yields at least one key. Otherwise it's treated as not given, so both commands print Error: one of -site, -sites, or --all-sites is required with exit 1, the same as passing no selector. One side effect worth calling out: -site a -sites "," now crawls a with no warning, where before it warned and then dropped -site. If you'd rather that combination be an error, I can change it.

The run task-spec path already rejects blank sites entries in TaskSpec.Validate, so it isn't affected.

Testing:

  • Extended TestResolveSiteKeys with ",", " , ,", " " and -site a -sites ",". These failed before the change.
  • Built the binary and checked crawl -sites "," and watch -sites " , ". Both now exit 1 with the error above.
  • go test -race ./cmd/..., go test ./..., go vet ./..., golangci-lint run ./... and golangci-lint fmt --diff ./... are all clean.

AI disclosure: I used an AI coding assistant to help write this change and the test. I reviewed the diff and ran the checks above locally.

`crawl -sites ","` resolved to an empty key list but was accepted, so
the single-site path indexed siteKeys[0] and panicked. `watch -sites ","`
started and idled forever watching zero sites.

Treat a -sites value with no keys as if it was not given. Both commands
now print "one of -site, -sites, or --all-sites is required" and exit 1,
and an empty -sites next to a -site no longer discards that -site.

Fixes Sriram-PR#13
@Sriram-PR

Copy link
Copy Markdown
Owner

Thanks for the fix, and for flagging the -site a -sites "," case.

Let's keep that combination working but make it visible: when -sites is set but contains no keys and -site is given, use -site and return a warning, -sites has no site keys; using -site, so it prints the same way as the existing "both -site and -sites given" warning. That's the final if siteKey != "" branch of resolveSiteKeys, setting the warning when sites != "".

Please update the resolveSiteKeys("a", ",", false) case to expect that warning. Once CI is green I'll squash-merge.

@Detoy

Detoy commented Oct 7, 2026

Copy link
Copy Markdown
Author

Added the warning you asked for: -site a -sites "," now prints "-sites has no site keys; using -site", and I updated the TestResolveSiteKeys case to match. I reviewed the change before pushing. Thanks for the quick look!

@Sriram-PR

Copy link
Copy Markdown
Owner

Thanks, this looks great. I checked the new warning with both crawl and watch, confirmed that -sites "," on its own now exits 1 with the usual selector error, and reran the other combinations. All behave as expected. Merging now. Thanks for the clear write-up and the AI disclosure!

@Sriram-PR
Sriram-PR merged commit 6f2672c into Sriram-PR:main Oct 7, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

crawl -sites "," panics with index out of range

2 participants