Skip to content

feat: add docs discovery helpers and strategies (IN-1317) - #4670

Open
gaspergrom wants to merge 15 commits into
mainfrom
feat/IN-1317-docs-discovery-strategies
Open

gaspergrom wants to merge 15 commits into
mainfrom
feat/IN-1317-docs-discovery-strategies

Conversation

@gaspergrom

Copy link
Copy Markdown
Contributor

Summary

  • Adds 8 discovery strategies for locating a project's documentation site: llmsTxtProbe, docsSubdomain, docsPath, packageManifest, readmeScrape, githubHomepage, projectWebsite, serpStrategy.
  • Adds github.ts/http.ts helpers for fetch/probe with SSRF-guard validation (destination/host matching, bracketed-IPv6 normalization).

Part of the docs-readiness epic (IN-1305). Stacked on #4648/#4658.

https://linuxfoundation.atlassian.net/browse/IN-1317

Adds projectDocReadinessRuns, projectDocDiscoveries and projectDocOverrides
for the AI documentation readiness pipeline (epic IN-1305).

Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Adds projectDocReadiness (one row per project per run date, replicated to
Tinybird later) and projectDocReadinessChecks (latest per-check afdocs
result per project) for epic IN-1305.

Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Start (idempotent on workflowId), finish (once only) and lookup helpers
for docs readiness pipeline runs, with DB tests.

Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
…312)

Upsert/find helpers for the latest docs URL discovery per project and
create/deactivate/find-active helpers for manual docs URL overrides,
with DB tests.

Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
…IN-1313)

Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Copilot AI balanced review requested due to automatic review settings September 18, 2026 12:00

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not something to fix in this PR: this diff only adds an internal @crowd/common workspace dependency to package.json. The flagged package (afdocs) predates this PR — it was added in #4648 (already merged to main). Leaving this unresolved for visibility, but no code change belongs here.

@cursor

cursor Bot commented Sep 18, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
New outbound fetching (user-supplied URLs, GitHub API, SerpAPI) with SSRF mitigations that explicitly do not cover DNS rebinding; misconfiguration or guard gaps could still enable internal network access.

Overview
Adds a docs discovery layer under docs_readiness_worker that turns project metadata (website, repos, optional GitHub/SerpAPI credentials) into ranked IDocCandidate results for the docs-readiness pipeline.

HTTP/GitHub plumbing: http.ts normalizes URLs, probes whether a URL is live HTML docs, and fetches text behind SSRF guards (private/loopback hosts, non-http(s) schemes, redirect hops, response size cap). github.ts picks a primary GitHub repo, parses owner/repo via @crowd/common’s canonicalizeRepoUrl, and calls the GitHub API for homepage, README, and root package.json.

Discovery strategies: Eight pluggable strategies in strategies.tsllmsTxtProbe, docsSubdomain, docsPath, packageManifest, readmeScrape, githubHomepage, projectWebsite, plus optional serpStrategy—each tagging method, confidence, and liveness. A default STRATEGIES list omits Serp until a key is present.

Deps & tests: Wires @crowd/common into the worker package. Adds broad Vitest coverage for URL/repo edge cases, SSRF behavior, GitHub client error paths, and each strategy’s happy/empty/error cases.

Reviewed by Cursor Bugbot for commit 215acdc. Bugbot is set up for automated code reviews on this repo. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 3 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit d16f7bd. Configure here.

Comment thread services/apps/docs_readiness_worker/src/discovery/github.ts
Comment thread services/apps/docs_readiness_worker/src/discovery/strategies.ts
Comment thread services/apps/docs_readiness_worker/src/discovery/http.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The SSRF guard has literal IPv4-mapped IPv6 and DNS-resolution bypasses, while response bodies remain unbounded.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds documentation-site discovery capabilities to the docs-readiness worker.

Changes:

  • Implements eight discovery strategies using project metadata, GitHub, and SERP.
  • Adds HTTP/GitHub helpers with URL validation and probing.
  • Adds comprehensive Vitest coverage and workspace dependency wiring.
File summaries
File Description
src/discovery/strategies.ts Implements discovery strategies and confidence mapping.
src/discovery/strategies.test.ts Tests strategy behavior and failures.
src/discovery/http.ts Adds URL normalization, probing, and SSRF checks.
src/discovery/http.test.ts Tests HTTP utilities and SSRF filtering.
src/discovery/github.ts Adds GitHub repository metadata helpers.
src/discovery/github.test.ts Tests GitHub parsing and requests.
package.json Adds the common workspace dependency.
pnpm-lock.yaml Records the workspace dependency.
Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file
  • Files reviewed: 7/8 changed files
  • Comments generated: 5
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread services/apps/docs_readiness_worker/src/discovery/http.ts Outdated
Comment thread services/apps/docs_readiness_worker/src/discovery/http.ts
Comment thread services/apps/docs_readiness_worker/src/discovery/http.ts Outdated
Comment thread services/apps/docs_readiness_worker/src/discovery/strategies.ts Outdated
Comment thread services/apps/docs_readiness_worker/src/discovery/strategies.ts Outdated

@themarolt themarolt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good overall - one question on STRATEGIES leaving out serpStrategy. the copilot points on the ssrf guard (ipv4-mapped ipv6 form, unbounded response.text()) look real to me too, worth doing before this gets wired into activities

…L matching

Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
…nse size

Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Copilot AI review requested due to automatic review settings September 18, 2026 13:25
@gaspergrom

Copy link
Copy Markdown
Contributor Author

Re: @themarolt's review — on serpStrategy: it's left out of STRATEGIES on purpose. It hits a paid SerpAPI key (ctx.serpApiKey) rather than being free like the other 7, so it's exported separately for the not-yet-written activity/workflow layer to call explicitly when CROWD_DOCS_READINESS_SERP_API_KEY is configured, instead of running automatically for every project. On the SSRF points: both are fixed now (1f855b4) — the IPv4-mapped-IPv6 canonicalization bypass and the unbounded response.text() buffering, with regression tests for each.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The SSRF guard remains bypassable through DNS resolution, and several fetch paths retain unread response bodies.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

services/apps/docs_readiness_worker/src/discovery/strategies.ts:70

  • The root-domain fallback runs only when the docs-subdomain fetch returns null. If docs.<domain>/llms.txt returns a 200 HTML catch-all or a short placeholder, it fails validation below and the potentially valid <domain>/llms.txt is never probed. Apply the same validity predicate before deciding whether to fall back; also guard a null normalized domain so this does not request docs.null.

services/apps/docs_readiness_worker/src/discovery/http.ts:87

  • This does not protect requests whose public-looking hostname resolves to loopback, RFC1918, or cloud-metadata addresses, so a project-controlled website/link can still trigger SSRF. Resolve and reject every A/AAAA result on every redirect hop, and pin the validated address for the connection to avoid rebinding; the repository's analogous preflight check is in services/apps/packages_worker/src/security-contacts/protocol/fetchContent.ts:91-120.
// Only rejects literal private/loopback hosts; a public hostname whose DNS record
// points at an internal address (DNS rebinding) is a known follow-up, not covered here.
async function guardedFetch(url: string, timeoutMs: number): Promise<Response | null> {

services/apps/docs_readiness_worker/src/discovery/http.ts:158

  • probe only inspects headers from a GET response and then drops the unread body. Repeated liveness probes can therefore pin Undici connections and response buffers until abort/GC; capture the metadata and explicitly cancel the body before returning, as the repository does for unused fetch bodies in services/apps/packages_worker/src/packagist/fetchPackage.ts:36-43.
    return {
      ok: response.ok,
      status: response.status,
      finalUrl: response.url,
      contentType: response.headers.get('content-type') ?? '',

services/apps/docs_readiness_worker/src/discovery/http.ts:176

  • The common non-OK path (for example, probing a missing /llms.txt) returns without cancelling the response body. This can retain the connection and buffer an attacker-controlled error body until timeout/GC; dispose of the body before returning null.
    const response = await guardedFetch(url, timeoutMs)
    if (!response || !response.ok || !response.body) {
      return null
  • Files reviewed: 7/8 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment on lines +99 to +106
if (REDIRECT_STATUSES.has(response.status)) {
const location = response.headers.get('location')
if (!location) {
return null
}
current = new URL(location, current).toString()
continue
}
return null
}

return githubRepos.find((repo) => repo.split('/').length >= 5) ?? githubRepos[0]

@themarolt themarolt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the ssrf and body-cap fixes look good, tests cover each case. one thing still open on primaryRepo - left a comment

return null
}

return githubRepos.find((repo) => repo.split('/').length >= 5) ?? githubRepos[0]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the >= 5 slash count doesn't line up with what parseGithubRepo accepts any more, so a mixed list picks an entry that then fails to parse. primaryRepo(['https://github.com/torvalds', 'git@github.com:torvalds/linux.git']) returns the bare org url - 4 parts vs 2, neither hits 5, so it falls through to githubRepos[0] - and parseGithubRepo on that is null, so packageManifest, readmeScrape and githubHomepage all return [] even though a valid repo was in the list. the scp form can never reach 5 parts, and scheme-less github.com/owner/repo is 3.

picking by what actually parses covers every form isGithubUrl now lets through:

Suggested change
return githubRepos.find((repo) => repo.split('/').length >= 5) ?? githubRepos[0]
return githubRepos.find((repo) => parseGithubRepo(repo)) ?? githubRepos[0]

the falls back to the first github.com URL when none has both segments test locks in the current pick - worth turning that into a mixed-form case.

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.

3 participants