ci: publish built binaries with run history#5
Conversation
There was a problem hiding this comment.
Review summary
Net change (after the intra-PR revert of the binary-artifact packaging): history is now keyed by the full LLGo commit instead of the Actions run number, the dashboard labels/columns are updated, and the deploy-pages gate is tightened. Reviewed the net state, not the reverted commits.
One clear correctness bug (malformed HTML), one defense-in-depth path-traversal gap in publish.sh, plus a column mislabel and some now-stale docs. Details inline and below.
Findings not tied to a changed diff line
docs/llgo-binary-size-handoff.md:35— stale history path. Step 5 still sayspublish.sharchives topages/data/runs/<run>-<attempt>/. This PR changed the key to the LLGo commit (data/runs/$run_key,run_key = llgoCommit || sourceCommit || id), and the same doc now states two lines later that history is keyed by the full LLGo commit. Update line 35 topages/data/runs/<llgo-commit>/so it stops contradicting the new behavior.docs/llgo-binary-size-handoff.md:25— wrong concurrency group name (pre-existing). Doc saysconcurrency.group: llgo-binary-size-pages, but the workflow setsgroup: llgo-binary-size-build(.github/workflows/llgo-binary-size.yml:40), whose comment explicitly keeps builds out of the Pages queue. Not touched by this PR, but worth fixing while nearby text is being updated.- Minor / optional —
ci/llgo-size/site/app.jshistory charts.renderHistoryChartsfetches every run'sresults.jsonconcurrently withcache: "no-store", so each page load re-downloads the full history and it grows linearly with the number of published commits. Consider droppingno-storefor the immutable per-commit files and/or capping the charted history. Non-blocking.
| except (OSError, ValueError): | ||
| continue | ||
| key = run.get("llgoCommit") or run.get("sourceCommit") or source | ||
| if not re.fullmatch(r"[A-Za-z0-9._-]+", str(key)) or str(key) == source: |
There was a problem hiding this comment.
Path-traversal / destructive-op gap: the key comes from results.json content (llgoCommit/sourceCommit) and the only guard is re.fullmatch(r"[A-Za-z0-9._-]+", ...), which matches . and ... A results.json in the persisted pages history whose llgoCommit is ".." yields target_dir = runs_dir/.. = .../data, so shutil.rmtree(target_dir) (below) wipes the entire published history and os.rename then clobbers data. "." similarly targets the runs dir itself.
Under the happy path values are 40-char SHAs (LLGO_COMMIT is validated upstream), so this is defense-in-depth rather than an active exploit — but the sole protection deliberately permits the traversal tokens. Reject bare dots explicitly, e.g. add or str(key) in (".", "..") to this skip condition, and apply the same fix to the run_key validator at line 80 (ideally a shared SHA-shaped pattern like ^[0-9a-f]{7,40}$).
| row.innerHTML = | ||
| "<td>" + link + "<small>" + escapeHtml(dateLabel(run.createdAt)) + "</small></td>" + | ||
| "<td><code>" + escapeHtml(shortSha(run.llgoCommit)) + "</code></td>" + | ||
| "<td><code>" + (run.workflowUrl ? '<a href="' + escapeHtml(run.workflowUrl) + '">' : "") + escapeHtml(commitLabel(run)) + (run.workflowUrl ? "</a>" : "") + "</td>" + |
There was a problem hiding this comment.
Malformed HTML: unclosed <code>. This cell opens <td><code> but closes only with </td> — the <code> element is never closed (</code> is missing before </td>). Every history row emits unbalanced markup; the browser will auto-recover but the monospace styling can bleed into following cells depending on how the parser reconciles the open element. The pre-PR version closed it (... + "</code></td>"). Fix: append </code> before </td>.
| <div class="table-wrap"> | ||
| <table> | ||
| <thead><tr><th>Run</th><th>LLGo commit</th><th>Go</th><th>LLVM</th><th>Workflow</th></tr></thead> | ||
| <thead><tr><th>LLGo commit</th><th>Created</th><th>Go</th><th>LLVM</th><th>Workflow</th></tr></thead> |
There was a problem hiding this comment.
Header/content mismatch: the last column header is Workflow, but renderHistoryTable (app.js:191) renders run.ref (e.g. refs/heads/main) in that 5th cell — the workflow link was moved into the first (LLGo commit) cell. Rename this header to Ref (or render the workflow link in the last cell) so the column label matches what is displayed.
Summary
Validation
bash -nfor publication scriptsgit diff --check