Skip to content

feat(pages): standardize web crawler policy - #939

Open
wu21-web wants to merge 17 commits into
alibaba:mainfrom
wu21-web:robots
Open

feat(pages): standardize web crawler policy#939
wu21-web wants to merge 17 commits into
alibaba:mainfrom
wu21-web:robots

Conversation

@wu21-web

Copy link
Copy Markdown
Contributor

Description

Standardlize web crawler policy with sitemap.xml and robots.xml, deploy to domain root and move scripts.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • make test passes locally
  • Manual testing (describe below)

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA

Related Issues

none

@wu21-web wu21-web changed the title feat(pages): standardized web crawler policy feat(pages): standardize web crawler policy Aug 16, 2026
@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

🔍 OpenCodeReview found 9 issue(s) in this PR.

  • ✅ Successfully posted inline: 7 comment(s)
  • ❌ Failed to post inline: 2 comment(s)

bug · low

📄 install.sh (L47-L47)

⚠️ GitHub could not post this as an inline comment: Line 47 could not be resolved (outside PR diff hunks)

The cleanup trap omits HUP (SIGHUP), which means if the terminal is closed or the session is lost during download, the temporary directory would not be cleaned up (until the shell exits, at which point EXIT fires). Adding HUP makes the cleanup more robust for long-running downloads:

💡 Suggested Change

Before:

  trap 'rm -rf "$tmp"' INT TERM EXIT

After:

  trap 'rm -rf "$tmp"' HUP INT TERM EXIT

bug · low

📄 install.sh (L53-L53)

⚠️ GitHub could not post this as an inline comment: Line 53 could not be resolved (outside PR diff hunks)

Potential issue: sha256sum (GNU coreutils) can produce checksums in binary mode with a * prefix before the filename (e.g., abc123 *opencodereview-linux-amd64). In that case, awk's $2 would be *opencodereview-linux-amd64, which would not match the plain asset name, causing the checksum lookup to fail with "no checksum entry" even though the entry exists.

Consider stripping the * prefix or making the match more flexible:

💡 Suggested Change

Before:

  want="$(awk -v a="$asset" '$2 == a {print tolower($1)}' "$tmp/sha256sum.txt")"

After:

  want="$(awk -v a="$asset" '{sub(/^\*/, "", $2); if ($2 == a) print tolower($1)}' "$tmp/sha256sum.txt")"

Comment thread pages/install.ps1 Outdated
Comment thread pages/install.ps1 Outdated
Comment thread pages/install.sh Outdated
Comment thread pages/install.sh Outdated
Comment thread pages/install.sh Outdated
Comment thread pages/public/sitemap.xml
Comment thread pages/public/sitemap.xml
Comment thread pages/public/robots.txt
@wu21-web
wu21-web marked this pull request as ready for review August 17, 2026 23:39
@lizhengfeng101

Copy link
Copy Markdown
Contributor

Thanks for tackling this — the site genuinely needed robots.txt/sitemap.xml. Two things I'd want sorted before merge, plus one question.

Why move the install scripts?

I'll be honest, I'm not seeing what this buys us. The old setup (real files at the repo root, cp install.sh _site/ at deploy time) already served both open-codereview.ai/install.sh and the raw GitHub URL. Moving them under pages/ only seems to save the two paths: entries in the workflow filter, and it costs us the symlink.

And that cost is real: raw.githubusercontent.com serves a symlink's target path, not its contents. I checked against your branch:

$ curl -fsSL https://raw.githubusercontent.com/wu21-web/open-code-review/robots/install.sh
pages/install.sh          # 16 bytes

That URL is exactly what we hand users in all four installation docs (docs/{en,zh,ja,ru}/installation.md), so after merge curl -fsSL .../install.sh | sh pipes the string pages/install.sh into sh and dies with "No such file or directory". Same for irm ... | iex. That makes this a breaking change rather than the non-breaking one the description claims. (Minor, but also: Windows clones without symlink support check the root files out as 16-byte text files.)

Unless there's a motivation I'm missing, I'd suggest dropping this part entirely — keep the scripts at the root and restore 'install.sh' / 'install.ps1' in the workflow paths: filter. That leaves this PR purely about the crawler files, which is a nice tight scope.

The sitemap lists 23 URLs, 22 of which currently 404

The site is a client-side SPA and GitHub Pages serves 404.html with an actual 404 status:

/                            200
/docs                        404
/docs/quickstart             404
/blog/introducing-ocr-blog   404

Search engines will reject all of those as "Submitted URL not found," so as it stands the sitemap fills Search Console with errors instead of getting us indexed. Either prerender a real index.html per route (we could generate the HtmlWebpackPlugin entries from the DocSlug union in content/docs/index.ts — that fixes site-wide indexability, not just the sitemap), or ship with just / for now and do prerendering separately. FWIW every slug in the sitemap checks out against the docs and blog registries, including cicdintegrations/ci.md, so that part's solid.

Smaller stuff in the workflow

  • test -f _site/robots.txt && cmp ... silently passes when the file is missing — bash -e doesn't exit on the left side of &&. The existing Verify install scripts step gets this right with one assertion per line; worth copying that shape.
  • CopyPlugin already emits public/* into dist/, so the two new cp pages/public/... _site/ lines are dead, and they make the cmp verify the cp rather than the build. If CopyPlugin ever breaks we'd still go green. Assert on pages/dist/robots.txt instead.
  • Folding apt-get into the step named "Install dependencies" (with a stray working-directory: pages) is a bit surprising, and it adds an apt-get update round trip to every deploy. The container already has Node — a few lines of Node, or npx fast-xml-parser, drops the apt dependency entirely.
  • The sitemap is hand-maintained with no test, and pages/ already has vitest wired up. A test that diffs the sitemap against the docs/blog slug sets would keep it from drifting — or generate it from that registry at build time.

Nits

  • The new public/ tree in pages/README.md omits images/.
  • I'd skip the robots.txt link in the 5 READMEs and 4 FAQs — it's an ops artifact, not a doc page, and in the FAQs it breaks the surrounding relative-link convention. Crawlers find the sitemap via the Sitemap: directive anyway.
  • No <lastmod>, which would help recrawl on docs and blog. Free if the sitemap becomes generated.
  • The ja FAQ line uses where its neighbors use ——.

Nothing here is a security concern — the script blobs moved byte-for-byte (8f92a5fa, d042d64a), checksum verification and the sudo path are untouched.

@wu21-web

Copy link
Copy Markdown
Contributor Author

@lizhengfeng101 Answers here:

  1. I didn't use symlinks because I thought they can redirect. I moved the install scripts because I consider them to be associated with pages (serve for pages as well), so they are a part of the pages. I can revert this change if you react with 👀 . Any raw url, e.g raw.githubusercontent.com/xxx/xx/install.sh should be deprecated and removed from the docs, should be replaced with the site url.
  2. I will check that soon. Surprised that OCR didn't catch that.
  3. All these are true. Tip on the last point, I didn't know how to generate the sitemap.xml on deploy dynamically, no idea. I assume manually maintaining that if no one can help me.
  4. Sure. All nits are accurate.

I'm a newcomer and wrote this by myself, so there might be plenty of nits and bugs. So feel free to criticize my code.

@wu21-web
wu21-web marked this pull request as draft August 18, 2026 13:02
@wu21-web
wu21-web marked this pull request as draft August 18, 2026 13:02
@wu21-web

Copy link
Copy Markdown
Contributor Author

Updates done.

@wu21-web
wu21-web marked this pull request as ready for review August 19, 2026 11:56
Regenerate public/sitemap.xml from the DocSlug/BlogSlug unions and the
static routes so a new page can no longer be silently omitted from the
sitemap. Centralize the site origin and route list in scripts/site-config.cjs,
add validate-sitemap coverage that the sitemap matches the routes exactly, and
have the deploy workflow assert on pages/dist/ instead of the public/ source.
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.

2 participants