ci: link benchmark commits to merged PRs - #19
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
Well-crafted PR that adds LLGo commit and merged-PR links to the Pages run index. The Python is clean and defensive: anchored repository/commit validation, path-traversal protection in repository_from_result, atomic temp-file writes, per-invocation caching, and a thoughtful scoped-token retry (all with real test coverage). The JS re-validates every URL against anchored https://github.com/... regexes and escapes all HTML interpolation, so a tampered index.json can't inject script. Security review found no issues.
A few minor findings inline. Nothing blocking.
Also worth noting (no reliable inline anchor):
- Recovery cost (perf, low): Steady-state enrichment is ~1 API call per publish because
pullRequestResolvedis carried forward inpublish.sh. But if that persisted state is ever lost (index regenerated from scratch, schema change, copy-forward failure), the next publish serially hits the GitHub API once per historical commit (30s timeouts, low anonymous rate limit) and can stall the job. A durable on-disk cache keyed by(repository, commit), independent ofpullRequestResolved, would make recovery cheap. Not blocking given current steady-state behavior.
| <p class="eyebrow">LLGo compiler benchmarks</p> | ||
| <h1>Binary size & build-time history</h1> | ||
| <p class="lede">Binary size and build wall time across Go and LLGo modes. Smaller values rank better within each benchmark and commit.</p> | ||
| <p class="lede">Binary size and build wall time across Go and LLGo modes. Linked commit labels open the pull request that landed each revision.</p> |
There was a problem hiding this comment.
User-facing text overstates that links always open a PR. commitHref (app.js:129-142) falls back to the plain GitHub commit URL whenever a run has no resolved pullRequestUrl (e.g. commits pushed directly to main, or lookups that returned nothing). In those cases the label opens a commit page, not a pull request. The code handles this correctly and the tooltip/aria text distinguishes the two, but this static copy (and line 37, "linked SHAs open their merged pull requests") claims unconditionally that links go to PRs. Consider softening to e.g. "open the merged pull request when one is known, otherwise the commit."
| json.dump(index, index_file, indent=2) | ||
| index_file.write("\n") | ||
| os.replace(temporary, args.index) | ||
| print("Resolved {} LLGo commit(s); {} run(s) link to merged PRs".format(queries, linked)) |
There was a problem hiding this comment.
Completion message mislabels queries as "Resolved ... commit(s)". queries counts GitHub API lookups performed this invocation (incremented only on a cache miss for a not-yet-resolved run); runs already marked pullRequestResolved are skipped before the cache block. On a typical incremental publish where most runs are already resolved via the carry-forward in publish.sh:142-152, this prints "Resolved 0 LLGo commit(s)" even though many commits are resolved and linked. Suggest wording like "Queried GitHub for {} LLGo commit(s)".
| if run.get("pullRequestResolved") is True: | ||
| if run.get("pullRequestUrl"): | ||
| linked += 1 | ||
| continue |
There was a problem hiding this comment.
pullRequestResolved is sticky and can't self-correct a stale repository. This continue runs after line 118 rewrites run["llgoRepository"]. If a run was previously resolved against the fallback/default repository and a correct llgoRepository later becomes derivable (e.g. repository_from_result starts returning a value), the PR lookup is never re-run and pullRequestUrl keeps pointing at the old repository. Given the design re-runs enrichment idempotently on every publish, consider keying the resolved flag on (repository, commit) so a changed repository forces re-resolution — or a short comment documenting that resolution is intentionally sticky.
1 participant
Summary
Validation