feat: publish main benchmarks from the current workflow - #6
Conversation
There was a problem hiding this comment.
Review: direct main publish
Solid refactor. The source_mode trust model is well designed: the persistent-publish gate is anchored to API-derived run metadata (kind==main requires RUN_EVENT==push && HEAD_BRANCH==DEFAULT_BRANCH, resolved from gh api .../actions/runs/${RUN_ID}), not caller-supplied inputs, so fork/PR runs cannot cause persistent Pages writes. RUN_ID is pinned to the trusted trigger, untrusted values are passed via env: and quoted/regex-validated, and the benchmark-publish.yml change (contents: write → read, plus the workflow_run.event == 'pull_request' guard) correctly confines the low-privilege path to comment-only. README and workflow YAML are consistent. No high/medium severity issues.
A few hardening and maintainability notes below.
Additional notes (no reliable inline location)
-
publish.ymlreusable-workflow invariant is caller-enforced. The "workflow-runmode = comment-only / no persistent write" property is enforced only by the caller (benchmark-publish.yml'sevent == 'pull_request'guard), not bypublish.ymlitself. A future caller invokingworkflow-runmode from a workflow_run that was triggered by a push-to-main would resolvekind=mainand persist. Consider asserting insidepublish.ymlthatworkflow-runmode never yieldspublish=true(or documenting that such callers must not grantcontents: write), so the two-trust-model invariant is self-contained. -
publish.ymlPR_NUMBERunbound-variable fragility (~line 190). On thepush/branchpathsPR_NUMBERis never assigned locally; the script survivesset -uonly becausePR_NUMBERarrives via the stepenv:block. If someone later dropsPR_NUMBERfromenv:, the push path breaks with an unbound-variable error far from the edit. InitializingPR_NUMBER=""at the top of the script makes the invariant local and obvious. -
publish.ymlDATA_REPOSITORYnot validated before clone (~line 263). The preview fallback cloneshttps://github.com/$DATA_REPOSITORY.gitwithout the^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$check applied toHEAD_REPOSITORY. It comes from a trusted workflow input so risk is minimal, but validating it would be consistent. (Optional.)
|
Addressed the remaining review notes:
Local |
|
Follow-up
The repository itself uses |
setup-benchmark-go-action
Action bundle sizes
Parser and validation operations
Compared only with the latest matching platform in the main series. |
Summary
current-runpublishing immediately after a successful default-branch benchmark jobworkflow-runmode for existing callers, including persistent Main, Branch, and Pull request Pages data, so current workflows need no migrationworkflow_runpublisher handles PR results without duplicating main runsSecurity
PR workflows only upload artifacts and never receive the Pages write token. The privileged publisher runs from the default branch, requires either its exact triggering workflow run or the current default-branch push, and derives repository, branch, SHA, PR number, and series kind from GitHub run metadata. A PR therefore cannot select the Main series.
PR and branch data are persisted only when their configuration exactly matches
config_pathon the default branch, preventing an artifact from redirectingsite-pathor overwriting another suite. A first setup PR can still produce a preview and comment, but it does not write Pages until the configuration is trusted.Compatibility
source_modedefaults toworkflow-run; existing publisher workflows continue to work unchanged.current-runis opt-in. Consumers that add it should restrict their existingworkflow_runpublisher to PR events to avoid publishing a main run twice.Validation
npm run check(35 tests)npm run buildnpm audit --omit=dev