ci: preserve every LLGo binary-size revision - #39
Conversation
There was a problem hiding this comment.
Review summary
This PR cleanly restructures the LLGo binary-size CI so history is ordered by each result's position on LLGo's first-parent main history rather than by build-completion time, and decouples the coalescible pin update from the per-commit benchmark build. The design is sound and well-commented.
Verified strengths:
- The oldest-first ordering direction change is applied consistently across
app.js(latestRun(),chartRuns()slice(-limit), initial page opening on the newest page). Remaining[0]accesses areselectedKeys[0](comparison slot A) orbenchmarkNames[0], none of which depend on run ordering. - 1-based indexing is consistent between the workflow's
awk ... print NRandenumerate(..., start=1). - Payload fields are routed through env vars (avoiding shell-injection sinks), commit SHAs validated with anchored
^[0-9a-f]{40}$, and repository fields checked by exact equality. - The
binary-sizeskip guard (push+github-actions[bot]+ci: pin LLGoprefix) correctly matches the pin commit message, preventing a self-trigger loop.
The findings below are low-severity robustness/maintainability notes; none are blocking. See inline comments.
| for run in index.get("runs", []): | ||
| commit = str(run.get("llgoCommit", "")).lower() | ||
| if commit in positions: | ||
| run["llgoMainIndex"] = positions[commit] |
There was a problem hiding this comment.
[P2] order_runs never clears a stale llgoMainIndex
order_runs only assigns llgoMainIndex when a run's commit is found in the freshly loaded --main-history; it never clears an existing value. A run persists its own build-time llgoMainIndex (baked into results.json by report.sh and copied into the index by publish.sh), so if that commit later drops off the current first-parent main history (history rewrite) or --main-history is omitted, the run keeps a potentially outdated position and the sort trusts it. In practice first-parent history is stable so this is low-likelihood, but recomputing/clearing the field for every run would make the freshly loaded history the single source of truth.
| def order_key(run): | ||
| position = run.get("llgoMainIndex") | ||
| if isinstance(position, int) and not isinstance(position, bool): | ||
| return (0, position, "", str(run.get("key", ""))) | ||
| committed_at = str(run.get("llgoCommittedAt") or run.get("createdAt") or "") | ||
| return (1, 0, committed_at, str(run.get("key", ""))) | ||
|
|
||
| index.setdefault("runs", []).sort(key=order_key) |
There was a problem hiding this comment.
[P3] Runs absent from main history always sort last, ignoring commit date
In order_key, any run with an integer llgoMainIndex is placed in group 0; every run without one goes to group 1. A run whose LLGo commit is not on first-parent main (e.g. a manual workflow_dispatch of an arbitrary/branch commit) is therefore forced to the very end of the timeline regardless of its llgoCommittedAt. Given the oldest-first display this makes such a run appear as the newest entry, so latestRun() picks it as the default environment/page. This may be intended (main history is authoritative), but the consequence is non-obvious — a short comment noting the intent, or reconsidering the fallback, would help.
| permissions: | ||
| contents: write | ||
| actions: write | ||
| runs-on: ubuntu-24.04 | ||
| concurrency: | ||
| # Pin updates may be coalesced because the file only records the latest | ||
| # LLGo main revision. The benchmark job below is not part of this queue: |
There was a problem hiding this comment.
[P3] Untrusted payload validated after write-scoped checkout
The update-pin job runs with contents: write and performs actions/checkout@v4 (persisting a write-scoped token) before the source_repository/llgo_repository/llgo_commit allow-listing and SHA validation run in the later step. Validation does gate the URL/git usage, so this is defense-in-depth rather than exploitable — and anyone able to send a repository_dispatch already holds a write token. Still, validating in a minimal-permission gating job that the privileged jobs needs: would ensure untrusted payloads never reach a contents: write context. The same pattern applies to the binary-size job's dispatch handling.
Summary
llgo-main-updatedcommit directly instead of putting all builds in one lossy concurrency groupmaintopology, independent of build completion timeValidation
go test ./...python3 -m unittest ci/llgo-size/test_enrich_pull_requests.pybash -n ci/llgo-size/report.sh ci/llgo-size/publish.shshellcheck ci/llgo-size/publish.shgit diff --check