Use path-component semantics for crawl controls by SihanTeng · Pull Request #76 · tamnd/kage

SihanTeng · 2026-07-31T03:14:59Z

Summary

Make --exclude and --scope-prefix match complete paths and descendants rather than arbitrary string prefixes or substrings.
Normalize prefixes without a leading slash and test exact, descendant, near-name, and nested-substring cases.
Move the compatibility notice under Changed with migration wording.
Correct --max-pages help and docs to say that failed render attempts count toward the cap.

Scope

This PR now contains only the accepted crawl-control changes. --traversal deprecation moved to #82, the contributor guide to #83, and robots documentation to #84.

Test plan

go test -count=1 ./urlx/ ./cli/ ./clone/

tamnd

Thanks for this, the --exclude fix is a real bug and the implementation is right. I want to land it, but I would like it split up first, and one line in the CHANGELOG moved.

Summary of what I am asking for:

File the --exclude change under Changed, not Fixed. It is a behaviour change for anyone relying on the old substring matching.
Fix ScopePrefix the same way while you are in InScope, otherwise half the function follows the new rule and half follows the old one.
Keep --traversal as a deprecated flag rather than deleting it outright.
Split CONTRIBUTING.md into its own PR, and move the robots.txt snippet out of the installation page.

The --max-pages wording change is correct and can land as is. I checked the rest of the CONTRIBUTING claims (make build, make test-short, make test, make vet, KAGE_CHROME, CHROME_BIN, --chrome) and they all hold, and the robots agent token claim is right too.

tamnd · 2026-08-06T03:23:57Z

+// Both sides are treated as URL paths: a prefix of "/api" matches "/api",
+// "/api/", and "/api/v1", but not "/apiv1" or "/map/api". A prefix without a
+// leading slash is normalised to one so "--exclude api" behaves like "/api".
+func pathHasPrefix(path, prefix string) bool {


This is the right implementation. Exact match, tolerant of a trailing slash on either side, and requiring a / boundary so /api does not swallow /apiv1. The doc comment enumerating the cases is exactly the level of prose the rest of this package writes.

One gap: the leading-slash normalisation on the next few lines (so --exclude api behaves like /api) has no test. It is the branch a future refactor is most likely to drop silently. Please add a case for it.

tamnd · 2026-08-06T03:23:57Z

 	}
 	for _, ex := range cfg.ExcludePaths {
-		if ex != "" && strings.Contains(u.Path, ex) {
+		if ex != "" && pathHasPrefix(u.Path, ex) {


While you are in this function, ScopePrefix a few lines above still uses a plain strings.HasPrefix(u.Path, cfg.ScopePrefix), so --scope-prefix /doc also matches /documentation/. That is the same class of bug you are fixing here.

Leaving one half of InScope on path-component semantics and the other half on string-prefix semantics is worse than fixing neither, because nobody can reason about the function without reading it. Please run pathHasPrefix on both.

tamnd · 2026-08-06T03:23:57Z

+		// and /private/x, but not /a/private/x or /privatething.
+		{"https://ex.com/private/x", ScopeConfig{ExcludePaths: []string{"/private"}}, false},
+		{"https://ex.com/private", ScopeConfig{ExcludePaths: []string{"/private"}}, false},
+		{"https://ex.com/a/private/x", ScopeConfig{ExcludePaths: []string{"/private"}}, true},


This line is the proof that the change is not just a bug fix. You are flipping an existing assertion from false to true, which means anyone passing --exclude /private today to catch /a/private/x will silently start crawling those pages after upgrading.

That is fine by me, the new behaviour is what the docs always promised, but it needs to be announced as a behaviour change rather than buried in Fixed.

tamnd · 2026-08-06T03:23:57Z


+### Fixed
+
+- `--exclude` matches path prefixes (and descendants), not arbitrary path substrings, matching the docs.


Please move this line to the Changed section. As the test diff shows, existing --exclude arguments that relied on substring matching will start behaving differently, and people skim Fixed.

Suggested wording: --exclude now matches path prefixes and their descendants rather than arbitrary substrings. If you relied on the old behaviour, pass the full path prefix.

tamnd · 2026-08-06T03:23:57Z

+
+### Changed
+
+- Removed the unused `--traversal` flag (it was accepted but never read by the crawl engine).


You are right that the flag is dead. I checked: cli/clone.go sets cfg.Traversal, clone/config.go stores it, and nothing ever reads it. The frontier is a map based dedup set and ordering comes from the render channel, so --traversal dfs has always been a no-op.

But deleting the flag outright turns every existing script that passes it into a hard unknown flag: --traversal failure, and their crawl does not run at all. They were getting BFS before and they get BFS after, so the only thing that changes is that their command breaks.

Please keep the flag and mark it deprecated instead:

fs.StringVar(&f.traversal, "traversal", "bfs", "frontier order (ignored; the crawl is always breadth-first)") _ = fs.MarkDeprecated("traversal", "the crawl is always breadth-first; this flag was never read")

We can delete it for real in the next minor release.

tamnd · 2026-08-06T03:23:57Z

 	fs.IntVar(&f.assetWorkers, "asset-workers", 8, "concurrent asset download workers")
 	fs.IntVar(&f.browserPages, "browser-pages", 4, "Chrome page-pool size")
-	fs.IntVarP(&f.maxPages, "max-pages", "p", 0, "stop after N pages (0 = unlimited)")
+	fs.IntVarP(&f.maxPages, "max-pages", "p", 0, "attempt at most N page renders (0 = unlimited); failures count toward the cap")


This one is a good catch and can land as is. I verified it against enqueuePage in clone/cloner.go: c.enqueued is incremented when a page is queued, not when it is saved, and nothing decrements it on a failed render. So --max-pages 100 against a site with 30 dead links really does give you about 70 pages.

The old wording promised something the code does not do. Documenting reality first is the right order, though I would like to make the code match the old promise eventually, since "stop after N pages saved" is what people actually want.

tamnd · 2026-08-06T03:23:57Z

+
+## What makes a good PR
+
+- **One concern per PR.** Fix install, or sanitize, or packing — not all three


I agree with this rule and I want it in the repo, which is why I am a little sheepish pointing out that this PR bundles a scope semantics fix, a flag removal, a docs rewrite across five files, and a new contributor guide.

Please split. The --exclude fix and the --max-pages wording can go in one PR and land immediately, --traversal in another, and this file in a third.

tamnd · 2026-08-06T03:23:57Z

+
+## Suggested first contributions
+
+Open issues and the review notes are a good map. High-impact areas that have


"the review notes" does not refer to anything that exists in the repo. Either drop the phrase or point it at something real.

tamnd · 2026-08-06T03:23:57Z

+Open issues and the review notes are a good map. High-impact areas that have
+already bitten users:
+
+- Authenticated clones (cookies / session) with **origin-aware** header handling


This is a precise description of the blocking problem in PR #58, which is open right now. If you spotted that, the useful place to say it is a review comment on that PR rather than a bullet here. I would genuinely welcome it.

tamnd · 2026-08-06T03:23:57Z

 If no browser is found, kage's launcher can download a private copy of Chromium
 on first use.

+To block kage from crawling your own site, add a robots.txt group for its agent


The content is correct. I verified it: cloner.go calls robots.Parse(string(data), "kage"), robots.Parse lowercases the agent token and the group values, so both kage and Kage match, and RespectRobots defaults to true.

Wrong page though. A site administrator looking for "how do I stop this thing" is never going to open a page titled Installation. Please give it its own page, something like docs/content/reference/robots.md, or put it in the politeness section of the README.

Two things worth adding while you are there: Crawl-delay is honoured too, and --no-robots exists, so this is advisory rather than enforcement. Being upfront about that is better than having someone discover it. This also closes out issue #8, which has been open since day one with the answer sitting in a comment thread.

SihanTeng · 2026-08-07T02:36:31Z

Thanks. I narrowed this PR to the crawl-control work that can land together:

pathHasPrefix now applies to both --exclude and --scope-prefix.
Tests cover a missing leading slash, exact and descendant matches, /doc versus /documentation, and nested substring non-matches.
The compatibility notice is under Changed with migration wording.
The accepted --max-pages wording remains.

The other concerns are now separate: #82 deprecates but retains --traversal, #83 adds only CONTRIBUTING.md, and #84 adds the dedicated robots reference and closes #8. I removed the undefined review-notes phrase and moved the authentication security diagnosis to PR #58: #58 (comment)

go test -count=1 ./urlx/ ./cli/ ./clone/ passes. Ready for re-review when convenient.

SihanTeng · 2026-08-07T03:32:55Z

Follow-up documentation pass: README now matches the reference docs for failed --max-pages attempts and path-component semantics, and the CLI --scope-prefix help no longer implies raw string-prefix matching. go test ./cli ./urlx passes.

Co-authored-by: SihanTeng <SihanTeng@users.noreply.github.com>

tamnd · 2026-08-10T06:38:18Z

The pathHasPrefix helper is right and I could not break it, the trailing slash normalisation on both sides is the part people usually get wrong. I squashed the branch onto main to clear the CHANGELOG conflict, the diff is unchanged. One thing I am fixing in a follow up rather than holding this: the new --max-pages wording still does not match the code, since the cap is on enqueued URLs and a page that robots disallows or that turns out not to be HTML spends a slot with no render attempted.

The cap is applied in enqueuePage against c.enqueued, so the budget is spent when a URL is queued rather than when it renders. A page that fails, that robots.txt disallows (checked after dequeue), or that turns out not to be HTML has already taken its slot, so a run can save fewer pages than asked for. The guide now also documents that pages discovered past the cap are still persisted to state.json, so raising the cap and rerunning continues instead of starting over. Follow-up to #76.

SihanTeng marked this pull request as ready for review July 31, 2026 05:06

tamnd reviewed Aug 6, 2026

View reviewed changes

This was referenced Aug 7, 2026

Deprecate the ignored traversal flag #82

Merged

Add a focused contributor guide #83

Merged

Document robots.txt controls for site owners #84

Merged

SihanTeng changed the title ~~Align crawl controls and contributor docs~~ Use path-component semantics for crawl controls Aug 7, 2026

Use path-component semantics for crawl controls

da41970

Co-authored-by: SihanTeng <SihanTeng@users.noreply.github.com>

tamnd force-pushed the fix/cli-docs-cleanup branch from 5c82aac to da41970 Compare August 10, 2026 06:38

tamnd merged commit d7e6ab1 into tamnd:main Aug 10, 2026
9 checks passed

tamnd mentioned this pull request Aug 10, 2026

Describe --max-pages as a queue budget, not a render count #85

Merged


		### Fixed

		- `--exclude` matches path prefixes (and descendants), not arbitrary path substrings, matching the docs.


		### Changed

		- Removed the unused `--traversal` flag (it was accepted but never read by the crawl engine).


		## What makes a good PR

		- One concern per PR. Fix install, or sanitize, or packing — not all three


		## Suggested first contributions

		Open issues and the review notes are a good map. High-impact areas that have

Conversation

SihanTeng commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Scope

Test plan

Uh oh!

tamnd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SihanTeng commented Aug 7, 2026

Uh oh!

SihanTeng commented Aug 7, 2026

Uh oh!

tamnd commented Aug 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SihanTeng commented Jul 31, 2026 •

edited

Loading