Repository navigation
Conversation
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):No automated checks match this pull request. ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
…anged files' sources
alexiscolin
left a comment
alexiscolin
left a comment
There was a problem hiding this comment.
Right shape for fork PRs (untrusted render, privileged publish), crawling gnodev is the right approach, and moving previews to their own branch-published repo settles the storage question. Since GNOWEB_PREVIEW_BASE_URL is already set here, everything below is live at merge, publish included.
Three things first, all inline: the publish job trusts the PR number written by the untrusted job (a fork can replace another PR's preview and overwrite its bot comment); the artifact is copied verbatim, and a .git/ inside it freezes publishing for every preview until it is removed; and the privileged job executes bin/reindex.sh from gno-previews@main while holding a token that writes PR comments here.
Smaller, not inline: the comment body is still posted as the untrusted job wrote it, which only bites combined with the first point; the sticky lookup does not check the comment's author; a render finishing after close recreates pr-<N>/ for up to a day until evict.yml runs; in wantFile the budget is charged at link discovery, not capture; ScreenshotPairs says "New in this PR" when the base render simply failed. Previews still share the gnolang.github.io origin with the other gnolang sites, bounded but worth one line in the header. _preview* is not in .gitignore yet.
The description lags the design in places: the gh variable set snippet still says /gno, "as soon as the snapshot is on gh-pages", the measured table's deploy rows, notes item 2, and the closing line about a draft and deploy-pages.yml; pr-preview.yml:6 too. No ADR; if this was AI-assisted, AGENTS.md asks for one, and either way the previews-repo and deploy-key decisions currently live only in workflow comments and the other repo's README.
AI-assisted review
| git reset -q --hard origin/main | ||
| rm -rf "pr-$PR" | ||
| mkdir -p "pr-$PR" | ||
| cp -r ../_preview/. "pr-$PR/" |
There was a problem hiding this comment.
Copied verbatim, then git add -A. The stock upload drops hidden files, but the PR controls its own pr-preview.yml, so include-hidden-files: true ships a .git/ and pr-<N> becomes a gitlink without .gitmodules (reproduced). GitHub documents that as a Pages build error, and a failed build freezes publishing for every PR until the gitlink is removed. No size bound either; the site has a 1 GB cap and evict.yml runs once a day. Before the copy: find _preview -name .git -prune -exec rm -rf {} + and a du -sm check.
There was a problem hiding this comment.
Reproduced exactly: with a commit inside the artifact's .git, git add -A records pr-9 as mode 160000, and git even warns about it. Without a commit it fails outright. Either way publication breaks, and in the gitlink case for every preview rather than just the one.
Both guards added before the copy:
find ../_preview -name .git -prune -exec rm -rf {} +
size=$(du -sm ../_preview | cut -f1)
[ "$size" -le "$MAX_PREVIEW_MIB" ] || { echo "::error::preview is ${size} MiB, over the ${MAX_PREVIEW_MIB} MiB cap"; exit 1; }Cap is 40 MiB against a measured worst case of 8.8 MiB — headroom, not a target.
There was a problem hiding this comment.
Confirmed for .git. The size cap is per PR only though, see the new comment on publish.yml:138.
| return path.Join(append(parts, "index.html")...) | ||
| } | ||
|
|
||
| func slug(s string) string { |
There was a problem hiding this comment.
Dots survive the slug: /r/x/y:.. folds onto r/x/y/index.html and overwrites the render page; :p/a-b, :p/a/b and :p/a&b collide silently; a long argument fails the render on path length. Reject ./.., hash-suffix when the slug differs from its input, cap the length.
There was a problem hiding this comment.
Confirmed, and worse than a collision: /r/x/y:.. produced r/x/y/index.html, overwriting the realm's own render page. /r/x/y$.. too. And :p/a-b, :p/a/b, :p/a&b all landed on p-a-b.
slug now trims . as well as -, caps at 64 chars, and appends a 4-byte digest of the input whenever the result is not already a safe segment — so anything non-trivial is collision-free by construction rather than by luck. TestURLToFileIsSafe asserts no output escapes its realm directory, that those three argument forms get distinct files, and that a 400-character argument stays bounded.
There was a problem hiding this comment.
Confirmed. The README and crawl.go examples still show the old slugs without the digest.
| ChangedFiles: plan.ChangedFiles, | ||
| FileBudget: fileBudget(plan), | ||
| } | ||
| if err := waitReady(c.Base, urlOf(plan.Realms[0]), cfg.timeout); err != nil { |
There was a problem hiding this comment.
Nothing watches gnodev: a boot failure or a realm that does not load (as far as I can see gnodev does not type-check before genesis, so a broken realm is a skipped tx and a 404) means the full timeout and a misleading "not ready", and the base pass can now add a second one. Probe gnoweb's /ready and select on cmd.Wait(); on failure the log is never surfaced (the upload step only runs on success), so print it with an if: failure() step.
There was a problem hiding this comment.
Right, and I hit this myself earlier in the branch: a port collision between the head and base passes cost the full five-minute timeout and reported "not ready", which said nothing about the actual cause.
startGnodev now returns a <-chan error fed by cmd.Wait(), and waitReady selects on it — a node that dies fails in seconds with its exit status. The log gap was real too: the render step deletes the logs on success and nothing else runs on failure, so there is now an if: failure() step that tails both.
There was a problem hiding this comment.
Dead node is covered, thanks. A realm that fails to load doesn't kill gnodev though, and if it's Realms[0], the page waitReady probes, it's still 5 min then "not ready"; probing /ready would avoid that.
There was a problem hiding this comment.
Fixed in 0420507: waitReady probes /ready, and a realm that fails to load is reported as missed. I also made it require gnoweb's {"status":"ready"} body. A local run with another server already on the port got that server's catch-all 200 and crawled it as the preview. The ports are now checked free before gnodev starts.
…safe slugs, JS-bearing screenshots
github-actions
Bot
added
the
📦 ⛰️ gno.land
|
@alexiscolin thank you — every finding reproduced, all are fixed in The PR number was fork-controlled. Resolved from the The artifact is untrusted data. Nothing in this repository executes code from The rest: slugs no longer traverse or collide (digest-suffixed and length-capped, with a test asserting no output escapes its realm directory); the trailing slash is preserved and On the screenshots — you were right and it mattered. 0 controllers over Also addressed: the description no longer talks about Taking you up on the |
|
I've launched a review on that PR. There's around 200 comments / 20 warnings. It's from a new workflow I'm trying, so I will not post them there's noises, but I recommend you to pass an AI on it. |
…n grow without limit
|
Pushed Every other axis of the crawl is bounded by the realm itself. It has the files it has, and gnoweb defines the tabs. Render arguments are not bounded by anything: a realm may link one page per value it knows about, and each one is a full page of gnoweb chrome. Today that is theoretical here, which is why this is a bound and not a bug fix. Measured on
So the default of 10 changes nothing about what is rendered today. What it buys is that a realm which starts enumerating cannot exhaust That is not hypothetical elsewhere. moul/gno-contracts#184 runs an adaptation of this renderer over a repository of 193 packages: one realm there ( Three details worth a reviewer's eye:
|
alexiscolin
left a comment
alexiscolin
left a comment
There was a problem hiding this comment.
Thanks Manfred, I went through your fixes one by one: 13 of the 16 points hold, including the PR-number fix (I tried injecting through the branch name and it comes back empty). Three are still open: readiness (a dead gnodev is caught now, but if the first realm, the page waitReady probes, fails to load, gnodev keeps running and it still waits 5 min; probing gnoweb's /ready would avoid that), the /gno URL in the description, and the comment body, see below.
Two things keep me from approving:
-
Repo variables aren't passed to
pull_requestruns from forks (GitHub's own settings page says so, quoted in community discussion #44322). Soif: vars.GNOWEB_PREVIEW_BASE_URL != ''skips the render for every fork PR, which is most of ours. The lab PRs were all same-repo, so this path was never exercised. -
The other way round is the real problem: a fork that drops that
if:from its copy of the workflow still gets published, since publish runs from master with the variable set and never checks who opened the PR. Whatever it uploads goes live on gnolang.github.io, which also hosts the godoc mirror, the guide, benchmarks and workshops. Worst case I found: its script can set gnoweb'sactionAddressInputin localStorage, and every other preview then pastes that value unquoted into the gnokey commands of crossing functions, so a reviewer copying a command from an honest preview could runx; curl …|sh. The variable and deploy key are already set, so this goes live on merge.
My suggestion: gate publish behind an Environment with required reviewers, pick trusted vs untrusted by checking the author's permission in a trusted step (a lot of us, you and me included, open PRs from forks), and move the deploy key into it. Cleanup uses the same key, so it probably wants its own environment without reviewers. I left the rest inline.
About #6158: it's a fork PR, so it wouldn't get a preview either way, and gnodev pins the local network kind, so the preview couldn't show mainnet/testnet anyway. Would you be OK lifting the block there?
| publish: | ||
| # A skipped preview uploads no artifact, and the download below turns the whole job | ||
| # into a no-op — no comment, no commit, nothing. | ||
| if: > |
There was a problem hiding this comment.
Nothing here looks at who opened the PR, so any successful run of a workflow named pr / preview gets published, including one a fork wrote itself. An Environment with required reviewers, chosen by the author's permission, would close it.
There was a problem hiding this comment.
Fixed in f09f12f. A resolve job with no secret checks that both the PR author and the triggering actor have write access (collaborators/{user}/permission). If they do, publishing goes through the gno-previews environment; otherwise it goes through gno-previews-review, which has required reviewers. The deploy key exists only in those two environments. Freshness is re-checked after approval.
|
|
||
| on: | ||
| workflow_run: # zizmor: ignore[dangerous-triggers] | ||
| workflows: [pr / preview] |
There was a problem hiding this comment.
workflow_run matches by name, so a fork can also add a new workflow with name: pr / preview. And there's no separate approval: approving a fork's CI approves its publish too.
There was a problem hiding this comment.
Fixed in f09f12f: resolve also requires workflow_run.path == '.github/workflows/pr-preview.yml', so a same-named look-alike is ignored. Approving a fork's CI run now only lets it render. Publishing it is a separate approval on gno-previews-review.
| # for a gnodev build they did not ask for. | ||
| # | ||
| # gh variable set GNOWEB_PREVIEW_BASE_URL -R <repo> -b https://<owner>.github.io/<repo> | ||
| if: vars.GNOWEB_PREVIEW_BASE_URL != '' |
There was a problem hiding this comment.
This is always empty on fork pull_request runs, so the render skips for most PRs (see main comment).
There was a problem hiding this comment.
Fixed in f09f12f: the gate is github.repository == 'gnolang/gno' || vars.GNOWEB_PREVIEW_BASE_URL != '', so fork PRs here render whether or not the run sees variables, and a fork's own PRs still skip unless it opts in.
| REPO: ${{ github.repository }} | ||
| PR: ${{ steps.meta.outputs.pr }} | ||
| run: | | ||
| [ -s _preview/comment.md ] || { echo "no comment body — nothing to post"; exit 0; } |
There was a problem hiding this comment.
The comment is written by the render job, i.e. the fork, and posted as github-actions[bot]. publish could rebuild it from preview.json (I checked, the output is identical), validating the paths and using the trusted base URL.
There was a problem hiding this comment.
Done in 0420507 + f09f12f, as you suggested. Publish checks out misc/gnopreview from the default branch and runs gnopreview comment over preview.json. TrustedPlan refuses any package path that is not a plain path, and derives every link, image path and caption again from the realm name and its own tables. Screenshots missing from the snapshot are dropped. For an honest snapshot the output is byte-identical: checked on a realm render with a before/after pair and on a gnoweb render with four shots. The render job no longer gets the base URL at all.
| # error — and a failed build stops publishing for every preview, not | ||
| # just this one. The size cap bounds the shared 1 GB site the same way. | ||
| find ../_preview -name .git -prune -exec rm -rf {} + | ||
| size=$(du -sm ../_preview | cut -f1) |
There was a problem hiding this comment.
The cap is per PR only: ~25 PRs at 40 MiB go past the 1 GB Pages limit before the daily evict. du also charges empty files nothing, so file count is barely bounded. Branch builds are also soft-limited to ~10/hour.
There was a problem hiding this comment.
Fixed in f09f12f. Per preview: 20 MiB and 1,000 files, apparent size, so empty files count. Site-wide: publish refuses to take the whole site past 900 MiB between two evictions. On the ~10 builds/hour soft limit: each push carries the full tree, so any build that does run publishes everything pushed before it. A burst delays previews but does not lose them.
| # previews are off — which is what every fork gets by default, so nobody pays | ||
| # for a gnodev build they did not ask for. | ||
| # | ||
| # gh variable set GNOWEB_PREVIEW_BASE_URL -R <repo> -b https://<owner>.github.io/<repo> |
There was a problem hiding this comment.
This gives …/gno for gnolang/gno; the PR description has the same stale line, while the real value is …/gno-previews.
There was a problem hiding this comment.
Gone. The render job no longer builds a URL, since the comment is built at publish time from the variable. The PR description's snippet now says /gno-previews.
| r/gnoland/home/index.html # the render view | ||
| r/gnoland/home/_t/source/index.html # $source | ||
| r/gnoland/home/_t/file-home.gno-source/… # $source&file=home.gno | ||
| r/gnoland/blog/_a/p-hello/index.html # :p/hello render arguments |
There was a problem hiding this comment.
Slug examples are missing the digest (crawl_test.go:59 expects p-hello-13cc55eb for this one), same on line 88 and in crawl.go:24 and :519.
There was a problem hiding this comment.
Fixed in 0420507, here and in crawl.go. The two documented examples are now pinned in TestURLToFile, so the docs cannot drift from urlToFile again.
| Write access is a deploy key rather than a fine-grained token: it cannot reach | ||
| another repository, does not expire, and carries no personal identity. | ||
|
|
||
| ## Bounds |
There was a problem hiding this comment.
Bounds leaves out the 400-page, 10-arg and 40 MiB caps, and the ADR never names GNOWEB_PREVIEW_BASE_URL or PREVIEWS_DEPLOY_KEY.
There was a problem hiding this comment.
Fixed in f09f12f: the ADR now lists the 400-page, 10-arg and per-preview/site caps, the timeouts, and names GNOWEB_PREVIEW_BASE_URL, PREVIEWS_DEPLOY_KEY and both environments.
| Dir string // examples/gno.land/r/gnoland/home (repo-relative, slash-separated) | ||
| Imports []string // gno.land/* imports of its non-test files | ||
| Realm bool // lives under gno.land/r/ | ||
| Draft bool // gnomod.toml: draft = true |
There was a problem hiding this comment.
Draft is set but never read.
There was a problem hiding this comment.
Removed in 0420507. modFlags became modIgnored.
| @echo "serving $(OUT) on http://localhost:$(PORT)/" | ||
| @cd $(OUT) && python3 -m http.server $(PORT) | ||
|
|
||
| $(GNODEV): |
There was a problem hiding this comment.
/tmp/gnodev is built once and never rebuilt, which is the mismatch the README warns about, and run duplicates the README usage. Maybe just drop it?
There was a problem hiding this comment.
Dropped in 0420507. The README usage is the one copy.
|
About 5f4450e: I measured this one: the inScope check doesn't keep the queue from filling when the links are all on one page. On a realm with 2,000 |
…ng, waiting or linking past what the snapshot holds
…ironment for everyone else
|
Round 2 is addressed in One thing is left before merge, and it is settings, not code: the deploy key has to move into the |
alexiscolin
left a comment
alexiscolin
left a comment
There was a problem hiding this comment.
Went through round 2 again, every fix holds in the code: write-access gate on author + actor, the workflow_run.path check, freshness after approval, the comment rebuilt by TrustedPlan, site-wide caps, timeouts and the process-group kill, /ready. Nice work, approving.
One thing worth a quick follow-up (inline): a PR that only touches tests, a README or misc/gnopreview renders nothing and uploads no artifact, but publish still runs and fails on the download. It's off the PR checks so nothing breaks, it's just noise (and an approval request once the review env exists).
And before merge, settings side: create gno-previews and gno-previews-review (master-only deployment branches, required reviewers + prevent self-review on the second), move PREVIEWS_DEPLOY_KEY into both and delete the repo secret. Until then forks fail closed, which is fine, but the key isn't environment-only yet like the ADR says.
| # makes the maintainer the actor, and an outsider pushing to a maintainer's | ||
| # fork makes the outsider the actor. Any doubt, an API error included, | ||
| # lands on the side that asks a human. | ||
| - name: Decide whether publishing needs a maintainer's approval |
There was a problem hiding this comment.
When the plan is empty (only _test.gno, a README, or misc/gnopreview changed), the render job ends green with no artifact, and publish then dies on the download. Listing actions/runs/{id}/artifacts?name=pr-preview here and standing down when it's missing would skip it before any environment gets involved. actions: read is already there.
| echo "$1: $p" >&2 | ||
| [ "$p" = admin ] || [ "$p" = write ] | ||
| } | ||
| if can_write "$AUTHOR" && can_write "$ACTOR"; then |
There was a problem hiding this comment.
Small gap: a maintainer opening a PR from an outsider's fork branch is both author and actor, so that code skips review. Could also require HEAD_OWNER to be gnolang or the author, and send anything else to gno-previews-review.
| return func() { | ||
| // gnodev spawns a node; kill the whole process group. | ||
| _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGTERM) | ||
| <-waited |
There was a problem hiding this comment.
nit: no SIGKILL fallback, so a gnodev that ignores SIGTERM holds the job until the 20 min timeout.
| pkg := &Pkg{ | ||
| Path: strings.TrimPrefix(rel, examplesRel+"/"), | ||
| Dir: rel, | ||
| Realm: strings.Contains(rel, "/r/"), |
There was a problem hiding this comment.
nit: Contains(rel, "/r/") would also match a /p/ package with an r segment in its path. A prefix check on gno.land/r/ is stricter.
This branch has not been deployed
5 participants
What
Every pull request that changes a realm, a package a realm imports, or gnoweb itself gets a
static gnoweb snapshot published to a per-PR URL, and a sticky comment linking to exactly
what changed.
transitively import a changed package, each with
sourceandhelpshortcutsfour views that carry most of gnoweb's chrome
👀 See it running
This is deployed and working in a scratch repo — moul/gno-preview-lab,
a single-commit snapshot of this branch with every workflow but these four removed — with one
pull request per code path:
r/gnoland/homep/nt/ownable/v0The godoc site is unharmed at the root of the same Pages site:
https://moul.github.io/gno-preview-lab/
Lab PR #1 changes gnoweb's CSS to something deliberately loud, so the screenshot proves the
preview rendered that branch rather than the published site:
Home — rendered markdown
Help / actions forms
How
misc/gnopreviewbootsgnodevon just the selected packages (dependencies resolve lazily outof
examples/), crawls the resulting gnoweb, and writes a self-contained static tree.Three workflows, split on the trust boundary so previews work for pull requests opened from forks:
pr-preview.ymlpull_requestpr-preview-publish.ymlworkflow_runpr-preview-cleanup.ymlpull_request_target: closedpr-<N>/Publishing is gated on who is asking. A job holding no secret checks that both the PR author
and the run's triggering actor have write access here; if so the snapshot publishes through the
gno-previewsenvironment, otherwise (forks from outside contributors) throughgno-previews-review, which has required reviewers. The deploy key lives only in those twoenvironments. Rendering always runs on
gnolang/gno; elsewhere it is opt-in through theGNOWEB_PREVIEW_BASE_URLrepository variable, which the lab sets:gh variable set GNOWEB_PREVIEW_BASE_URL -R gnolang/gno -b https://gnolang.github.io/gno-previewsWhy previews live in their own repository
Previews publish to gnolang/gno-previews, a
separate Pages site in branch mode.
deploy-pages.ymlis not touched by this PR — the godocsite keeps deploying exactly as it does today.
The two workloads want opposite things. The docs are regenerated from scratch on every deploy
(measured: 2 m 57 s) and nobody needs them within five minutes of a merge. A preview's whole value
is being there when a reviewer clicks the link. A Pages site built by an Actions workflow has no
incremental deploy — the entire site ships every time — so sharing one would mean every preview
push rebuilds and re-uploads the docs, putting the link 3–6 minutes behind the comment. Publishing
from a branch, as the previews repo does, pushes only the bytes that changed.
Keeping them apart also bounds the blast radius: a broken preview job cannot delay or break the
documentation, and the two do not share a 1 GB site budget or a git history.
The previews repo runs its own scheduled
evict.yml— drop previews whose PR closed, drop previewswhose PR has been quiet for 21 days, then enforce a 700 MiB budget oldest-first. Without it the site
would reach the 1 GB cap at roughly 78 concurrent previews.
Setup
Changed in round 2, not yet applied: the deploy key moves from a repository secret into the
environments
gno-previews(no reviewers) andgno-previews-review(required reviewers). Untilthose exist the review path refuses to publish rather than creating an unprotected environment.
PREVIEWS_DEPLOY_KEY: the private half of a write-enabled deploy key ongnolang/gno-previews, as a secret of those two environments only. A deploy key ratherthan a fine-grained PAT because it cannot reach any other repository, it does not expire, and it carries no personal identity — preview commits are
not attributed to whoever minted the credential, and nothing breaks when that person rotates a
token or leaves.
GNOWEB_PREVIEW_BASE_URL=https://gnolang.github.io/gno-previews(already set), which isalso the publishing switch: unset means nothing is published or cleaned up. On a fork's own
repository it is also what turns rendering on.
Measured on the lab
p/nt/avl/v0)gno-previews→ servingThe comment does not wait on the deploy: the publishing job posts it as soon as the snapshot is
pushed, and the link is serving about half a minute later.
Scope and bounds
Deliberately not previewed:
*_test.gno,*_filetest.gno,filetests/,examples/quarantined/**, andignore = truepackages. Realms undergno.land/r/tests/arepreviewed when changed directly, never pulled in as dependents.
The crawl skips URL shapes that enumerate rather than describe —
$state&oid=/&tid=(walksthe object graph, and trips gnoweb's own rate limiter),
$state(~1.3 MB per realm of a chainthat only ran
init()),$help&func=X($helpalready lists them all),$download. At most 25realms and 400 pages, and the comment says how many realms were dropped.
misc/gnopreview/README.mddocuments the output layout, the URL-to-path slugging (no$,:or&ever reaches a file path), and the known limits.Notes for review
pending run per group, so with three PRs publishing at once the middle one was cancelled: its
snapshot rendered, its comment never appeared. The group is now keyed on the source head
repository and branch, so different PRs publish in parallel — the shared branch is already
serialized by the replay loop.
cancel-in-progress: trueon thepagesgroup starves the deploy. With a dispatch perpreview publish, consecutive
deploy / pagesruns cancelled one another and the site stoppedupdating for the length of the burst. Left at
false, its original value: every run publishesthe complete site, so GitHub's one-running-plus-one-pending behaviour already coalesces a
burst, and the pending run picks up everything when it starts. Moot now that previews no
longer share the docs pipeline, but the reasoning is why
deploy-pages.ymlends up unchanged.merge-base checkout, using the head's gnodev binary and the head's assets in both passes, so
the pair isolates the realm change rather than whatever else moved on master. A realm the PR
adds says so instead of faking a before; a run with no base checkout makes no claim at all.
gnodev defaults both its RPC listener and its keybase to fixed locations, so the two passes are
given distinct ones — otherwise they fight over
127.0.0.1:26657.noindex, and were not. gnoweb's layout emits<meta name="robots" content="index, follow">on every page(
components/layouts/head.html:38), so a published snapshot — a near-duplicate of a realgno.land page — was actively inviting the crawler. The renderer now replaces that tag with
noindex, nofollowrather than adding a second one; two conflicting robots directives leavethe outcome to each crawler's precedence rules. Not a
robots.txtDisallow: a disallowedpath can still be indexed from an external link, and being disallowed is exactly what stops a
crawler from reading the
noindex.misc/gnopreviewis registered inci-dir-misc.yml's matrix — that file's own comment warns anunlisted program is never built.
public/js/index.jsbuilds its dynamicimport()specifiers from a hardcoded/public/js/controller-prefix. The snapshot rewrites it relative to the asset's own URL;without that, every interactive control 404s from any mount point but the site root. The renderer
warns loudly if that string ever disappears.
branch's
gnopreview commentfrom the artifact'spreview.json, refusing any package paththat is not a plain path and deriving every link, image path and caption again. It is
byte-identical to what the render job writes for an honest snapshot (checked on a realm render
with a before/after pair and on a gnoweb render with four screenshots).
actionlint,zizmor --persona=regular,golangci-lintwith.github/golangci.yml,go fix,go vetandgo testall clean.ADR:
gno.land/adr/pr6194_gnoweb_pr_previews.md.Review round 1
Every one of @alexiscolin's findings reproduced; all are fixed in
fix(ci): address review …. The three security ones:a cross-PR primitive: name another number and you replace that preview and PATCH its comment.
Now resolved from the
workflow_runpayload, with a head-SHA check that stands down when anewer run has superseded this one, and
state=openso a render finishing after close publishesnothing.
.gitin the artifact makesgit add -Arecordpr-<N>as mode160000(reproduced),which GitHub Pages reports as a build error — freezing publication for every preview. Stripped
before the copy, plus a size cap on the snapshot.
bin/reindex.shfromgno-previews@mainwhile holdingpull-requests: writehere. Nothing in this repository runs code from that one any more; theindex is regenerated by a workflow over there with its own token.
And the rest: slugs no longer traverse (
:..folded onto the realm's own render page) or collide(
:p/a-b,:p/a/b,:p/a&bshared one file) — non-trivial slugs now carry a digest and are lengthcapped; the trailing slash is preserved, since
/r/x/y/is a different page from/r/x/y(48,642vs 62,174 bytes) and
/ris no longer seeded because gnoweb answers it with 400; gnodev is watched,so a node that dies fails in seconds with the reason instead of timing out on "not ready", and the
log is printed on failure; the sticky comment matches the bot's authorship, not just the marker;
the per-file budget is charged at capture rather than at link discovery; "New in this PR" is
asserted from the merge-base tree rather than inferred from a missing capture.
Screenshots were showing the no-JS state. Chrome will not load ES modules over
file://—0 controllers initialise versus 3 over HTTP, matching the numbers in the review. The renderer now
serves the snapshot on a loopback port while photographing it. A one-line gnoweb fix using
import.meta.urlinindex.tswould also remove the/public/js/controller-rewrite this dependson; happy to take that PR.
Review round 2
All of @alexiscolin's second-round points are addressed in
f09f12fand0420507; eachfix that has a test was seen red before green.
or a maintainer's approval through
gno-previews-review(both, because a maintainer's"Update branch" makes the maintainer the actor). The deploy key is environment-only. The run
must come from
.github/workflows/pr-preview.yml(workflow_run.path), so a same-namedlook-alike is ignored. Freshness is re-checked after approval, since an approval can come
days after the render.
github.repository == 'gnolang/gno' || vars.GNOWEB_PREVIEW_BASE_URL != '', which holds whether or not a fork run sees variables.take the whole site past 900 MiB between two evictions.
process group (killing only the parent leaves its helpers holding the pipe open).
/readyand requires its{"status":"ready"}body. A bare 200was not enough: a local run crawled another server already on the port. The ports are
checked free before gnodev starts.
2,000-link case goes from 2,004 fetches to 7, and a separate test covers the before-pass.
--no-renames; slug examplescarry their digest (and are pinned by a test);
Draftand the Makefile are gone; the ADRnames the variable, the secret, the environments and every cap.