Add CLI hardening and crawler output features - #3
Conversation
![vorflux[bot]](https://avatars.githubusercontent.com/in/2483280?s=60&v=4){kind=small}
| pool.terminate() | ||
| process.stderr.write("\n Interrupted. Stopping workers...\n") | ||
| process.exit(130) | ||
| } |
There was a problem hiding this comment.
Bug: SIGINT handler exits before manifest/output bookkeeping can run.
process.exit(130) fires immediately after pool.terminate(), which bypasses Promise.all(writes), writeManifest(...), and the bundle/jsonl/failures writes further down. An interrupted run loses all manifest progress.
This breaks the new resume flows: after stopping a long crawl and rerunning with --changed-only, pages that were already written during the first run will be re-fetched because they were never recorded as entry.ok in the manifest. The interrupted flag on line 44 is effectively dead code since the process has already exited before it can be checked.
Suggested fix: instead of process.exit(130), set interrupted = true, call pool.terminate(), and let the normal flow fall through to write the manifest with whatever entries succeeded before the interrupt. Then exit with code 130 at the end.
| const outputPaths = new OutputPathAllocator() | ||
| const planned = urls.map((url) => ({ url, outputPath: outputPaths.allocate(url) })) | ||
| const manifestPath = config.manifestFile ?? defaultManifestPath(config.out) | ||
| const manifest = readManifest(manifestPath) | ||
| if (config.diff) { | ||
| const diff = manifestDiff( | ||
| planned.map((page) => page.url), | ||
| manifest, | ||
| ) | ||
| console.log(JSON.stringify(diff, null, 2)) | ||
| return | ||
| } | ||
| const skipExisting = config.resume || config.changedOnly || !config.overwrite | ||
| const skipped = skipExisting | ||
| ? planned | ||
| .filter((page) => { | ||
| const entry = manifest.entries[page.url] | ||
| const fileExists = existsSync(join(config.out, page.outputPath)) | ||
| if (config.changedOnly) return entry?.ok && fileExists | ||
| return fileExists |
There was a problem hiding this comment.
Bug: --resume/--changed-only is unstable for colliding URLs.
OutputPathAllocator assigns _2, _3, ... suffixes based on the current discovery order, but the skip logic here checks existence using the newly allocated page.outputPath instead of the path stored in manifest.entries[page.url].outputPath.
For URLs that collide (e.g. /foo, /foo.html, /foo?tab=js), a different discovery order on the next run can swap which URL maps to foo.md vs foo_2.md. This means --changed-only/--resume can skip the wrong file or leave stale content behind.
Suggested fix: either seed the allocator from prior manifest paths, or use manifest.entries[page.url]?.outputPath for the existence check instead of recomputing a fresh path.
|
|
||
| export const urlToOutputPath = (url: string): string => { | ||
| const parsed = new URL(url) | ||
| let pathname = decodeURIComponent(parsed.pathname) |
There was a problem hiding this comment.
Bug: decodeURIComponent can crash the entire crawl on malformed percent-encoded paths.
decodeURIComponent(parsed.pathname) throws URIError for inputs like /%E0%A4%A (incomplete sequences). new URL(...) happily accepts those URLs, so if discovery finds one broken link from a real site, the urls.map((url) => outputPaths.allocate(url)) call in src/index.ts will abort the whole run before any fetching starts.
Since this tool targets arbitrary public docs sites, malformed hrefs are realistic.
Suggested fix:
let pathname: string
try {
pathname = decodeURIComponent(parsed.pathname)
} catch {
pathname = parsed.pathname
}
Testing ResultsShipped tests: 23/23 passAdditional tests written and run (8 files, 109 tests):
Grand total: 132/132 pass, 0 fail (610ms, 240 expect() calls) E2E Smoke Tests (13/13 pass)
Bug Found
|
|
Addressed the review findings in 5f61663:
Verification:
|
|
Reviewed the fixes in 5f61663 -- all four issues are addressed correctly:
No further issues found. |
![accesslint[bot]](https://avatars.githubusercontent.com/in/1502?s=60&v=4){kind=small}
| describe("fallbackMarkdown", () => { | ||
| test("preserves common block and inline structure", () => { | ||
| const result = fallbackMarkdown(` | ||
| <html> |
There was a problem hiding this comment.
WCAG 3.1.1: <html> element missing lang attribute.
The <html> element must have a lang attribute.
Details
Page text sample: "import { describe, expect, test } from "bun:test" import { extractHtmlMetadata, fallbackMarkdown, frontmatter } from "./convert" describe("frontmatter", () => { test("escapes YAML-sensitive strings", "
Screen readers use the lang attribute to determine which language rules and pronunciation to use. Without it, content may be mispronounced. Set lang to the primary language of the page (e.g., lang='en' for English, lang='es' for Spanish).
| describe("extractLinks", () => { | ||
| test("extracts same-host anchor hrefs and ignores script text", () => { | ||
| const html = ` | ||
| <html> |
There was a problem hiding this comment.
WCAG 2.4.2: Document is missing a <title> element.
Documents must have a <title> element to provide users with an overview of content.
Details
Page text: "import { describe, expect, test } from "bun:test" import { extractLinks, isAllowedByRobots, parseLocs, parseRobots } from "./discover" describe("parse"
Screen reader users rely on page titles to identify and navigate between tabs/windows. Add a descriptive <title> element in <head> that summarizes the page purpose. Keep titles unique across the site, placing specific content before the site name (e.g., 'Contact Us - Acme Corp').
Best Practice: Page has no mechanism to bypass repeated content. Add a <main> landmark or skip link.
Page must have a mechanism to bypass repeated blocks of content.
Details
Missing: no landmarks (<main>, <nav>, <header>, <footer>), no skip link, no headings
Keyboard users must be able to skip repetitive content like navigation. Provide a skip link at the top of the page that links to the main content (e.g., <a href="#main">Skip to main content</a>), or use a <main> landmark. Screen readers can jump directly to landmarks, so a properly marked-up <main> element satisfies this requirement.
Best Practice: Page does not contain a level-one heading.
Page should contain a level-one heading.
Details
A level-one heading (<h1> or role='heading' with aria-level='1') helps users understand the page topic and provides a landmark for screen reader navigation. Each page should have exactly one h1 that describes the main content, typically matching or similar to the page title.
Best Practice: Page has no main landmark.
Page should have exactly one main landmark.
Details
The main landmark contains the primary content of the page. Screen readers allow users to jump directly to main content. Use a single <main> element (or role='main') to wrap the central content, excluding headers, footers, and navigation.
WCAG 3.1.1: <html> element missing lang attribute.
The <html> element must have a lang attribute.
Details
Page text sample: "import { describe, expect, test } from "bun:test" import { extractLinks, isAllowedByRobots, parseLocs, parseRobots } from "./discover" describe("parseLocs", () => { test("parses XML loc elements and d"
Screen readers use the lang attribute to determine which language rules and pronunciation to use. Without it, content may be mispronounced. Set lang to the primary language of the page (e.g., lang='en' for English, lang='es' for Spanish).
|
Updated this PR against the latest upstream main in f63fcf1. What changed:
Verification:
|
There was a problem hiding this comment.
Reviewed the upstream merge in f63fcf1 -- found 7 issues in the new SPA rendering, route extraction, and browser integration code. 4 issues are posted as inline comments below; 3 additional issues are in a separate comment (lines not in the diff).
| convertTimeoutMs: config.convertTimeoutMs, | ||
| delayMs: config.delayMs, | ||
| headers: config.headers, | ||
| jobTimeoutMs: config.timeoutMs, |
There was a problem hiding this comment.
Bug: SPA detection uses a second, unconfigured fetch that can disagree with discovery.
discover() already fetched the base URL using configured headers, user-agent, delay, timeout, and redirect handling. This second fetch(config.url, { redirect: "follow" }) ignores all of those options, so it can get a different response (e.g. a bot-block page, login wall, or error) from what discovery saw.
If this fetch fails or returns a non-SPA page, useBrowser stays false and workers will convert SPA shells without rendering, producing empty output.
Suggested fix: have discover() return the SPA detection result as metadata, or reuse the HTML already fetched during discovery.
| const rendered = await renderPage(finalUrl, { timeout: 20000 }) | ||
| if (rendered) text = rendered.html | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
Bug: Default job timeout (10s) is shorter than the SPA render timeout (20s).
Workers call renderPage(..., { timeout: 20000 }), but the CLI default --timeout is 10000ms and WorkerPool uses that as the whole-job timeout. A normal SPA render will be killed by the pool before Playwright finishes.
Suggested fix: separate the request timeout from the worker/job timeout, or increase the job timeout when useBrowser is true to account for fetch + render + conversion time.
| let browser: BrowserLike | null = null | ||
| let context: BrowserContextLike | null = null | ||
|
|
||
| let browser: Browser | null = null |
There was a problem hiding this comment.
Issue: Per-worker browser instances are not shared or gracefully closed.
The module-level browser/context singletons are per JS isolate, not shared across the worker pool. With browser mode enabled, up to workerCount independent Chromium instances can be launched (one per Bun worker). closeBrowser() in index.ts only closes the main-thread instance; it does not reach browser instances inside workers.
pool.terminate() stops the Bun worker but does not call Playwright's browser.close(), risking leaked Chromium child processes.
Suggested fix: either render in the main process with a shared browser/context, or add an explicit worker shutdown message that calls closeBrowser() before termination.
| // Prevent path traversal attacks from malicious hash fragments | ||
| const full = join(outDir, outputPath) | ||
| const rel = relative(resolve(outDir), resolve(full)) | ||
| if (rel.startsWith("..") || isAbsolute(rel)) { |
There was a problem hiding this comment.
Minor: Path traversal check is slightly too broad.
rel.startsWith("..") also rejects safe in-directory names like ..well-known/foo.md. While unlikely in practice, the precise check would be:
if (rel === ".." || rel.startsWith("../") || isAbsolute(rel))
Additional review findings for f63fcf1These 3 issues could not be placed as inline comments (lines are outside the diff hunks or in files not modified in this commit). 1.
|
| <html lang="en"> | ||
| <head><title>Links fixture</title></head> | ||
| <body> | ||
| <main> |
There was a problem hiding this comment.
Best Practice: Page has multiple main landmarks.
Page should have exactly one main landmark.
Details
The main landmark contains the primary content of the page. Screen readers allow users to jump directly to main content. Use a single <main> element (or role='main') to wrap the central content, excluding headers, footers, and navigation.
Best Practice: Page has multiple main landmarks.
Page should not have more than one main landmark.
Details
Only one main landmark should exist per page. The main landmark identifies the primary content area. If you have multiple content sections, use <section> with appropriate headings instead of multiple main elements.
1 participant
Summary
Verification