ci: rebuild Pages order from full LLGo history - #43
Conversation
There was a problem hiding this comment.
Review: reindex Pages from full LLGo main history
The core idea is sound and well-executed: rebuild each run's first-parent main position from an authoritative full history rather than trusting the index embedded at build time. The --filter=blob:none --no-checkout --single-branch clone in the Pages workflow is an efficient, correct choice for a commit-graph-only consumer, and --unshallow in the benchmark workflow is a necessary correctness fix (a bounded, blob-free cost). The added inline comments accurately describe the behavior they motivate, load_main_history validates commit format and fails loudly, and --main-history stays optional so ordering gracefully falls back to embedded hints.
One inline finding on the test. A few minor notes below.
Minor / non-blocking
order_runsnever clears a stalellgoMainIndexfor commits absent from the rebuilt history (ci/llgo-size/enrich_pull_requests.py). The reindex only sets the index for commits found inmain_history; a run whose commit is no longer on first-parent main (rebase/force-push/dropped commit) keeps its embedded index fromreport.sh/publish.shand is still treated as a main run byis_main_run, sorting into the topological section with a bogus position. Since the PR's premise is that the embedded index is "only a hint," consider clearing it when a real history is supplied:elif main_history: run.pop("llgoMainIndex", None)— guarded so the no---main-historypath still preserves hints.- Security context (pre-existing, out of this diff's scope): the Pages workflow
sourcesci/llgo-size/llgo-version.envand interpolates${LLGO_REPOSITORY}into a clone URL. Here it's low risk (the Pages job runs only frommain/workflow_dispatchwith trusted content). Note thatllgo-binary-size.ymluses the samesourcepattern on thepull_requestbuild path, where PR-controlled content is executed on the runner andLLGO_REPOSITORYis unvalidated before the clone — no secret/write-token exposure (it'spull_request, notpull_request_target, and publish steps aremain-gated), but arbitrary runner code execution and cloning from an attacker-chosen repo are possible. Worth hardening separately by parsing/validating instead ofsourceing. - Optional wording nit: the comment "otherwise every run appears to be main index 1" (
llgo-binary-size.yml) describes the shallow symptom; "runs can collapse to main index 1" is slightly more precise.
| { | ||
| "key": first, | ||
| "llgoCommit": first, | ||
| "llgoMainIndex": 1, |
There was a problem hiding this comment.
[P2] Test presets are dead input; doesn't verify stale-index correction
Both runs now carry llgoMainIndex: 1, but both commits (first, second) are in the main_history passed to order_runs, which unconditionally overwrites the index for any in-history commit. The preset 1 values are therefore always overwritten to 1/2, and the test passes identically whether they are 1,1 or absent — so it does not actually exercise replacement of a stale index, despite the test name.
If the intent is to prove a stale index gets corrected, make the presets wrong (e.g. both 99) and assert the corrected [2, 1]. Otherwise these two added lines add no coverage and could be reverted.
Summary
llgoMainIndexvalues in the ordering testValidation
bash -n ci/llgo-size/publish-site.sh ci/llgo-size/publish.shpython3 -m unittest ci/llgo-size/test_enrich_pull_requests.py8b630ab5(1540),fd3b2873(1539), then older commits