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 |
Summary
current-runpublishing for the current default-branch push while preserving trustedworkflow-runpublishingmain; pull requests update the validated comment and preview artifact without writing Pagesworkflow_runboundarycurrent-runrejects non-push events, non-current run IDs, and non-default branches. The example workflow derives the default branch instead of hardcodingmain. Fork pull requests still require the artifact configuration to match the trusted default-branch file.Validation
npm run checknpm run buildnpm audit --omit=dev