Skip to content

feat: publish main benchmarks from the current workflow - #6

Open
cpunion wants to merge 3 commits into
xgo-dev:mainfrom
dastai:codex/direct-main-publish
Open

feat: publish main benchmarks from the current workflow#6
cpunion wants to merge 3 commits into
xgo-dev:mainfrom
dastai:codex/direct-main-publish

Conversation

@cpunion

@cpunion cpunion commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • add current-run publishing for the current default-branch push while preserving trusted workflow-run publishing
  • publish long-term Pages data only for main; pull requests update the validated comment and preview artifact without writing Pages
  • run the self benchmark publisher directly after the main benchmark job and keep fork PR comments behind the trusted workflow_run boundary
  • resolve queued fork runs through the measured commit's PR association, while still skipping a stale comment if the PR head has moved

current-run rejects non-push events, non-current run IDs, and non-default branches. The example workflow derives the default branch instead of hardcoding main. Fork pull requests still require the artifact configuration to match the trusted default-branch file.

Validation

  • npm run check
  • npm run build
  • npm audit --omit=dev

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: writeread, 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.yml reusable-workflow invariant is caller-enforced. The "workflow-run mode = comment-only / no persistent write" property is enforced only by the caller (benchmark-publish.yml's event == 'pull_request' guard), not by publish.yml itself. A future caller invoking workflow-run mode from a workflow_run that was triggered by a push-to-main would resolve kind=main and persist. Consider asserting inside publish.yml that workflow-run mode never yields publish=true (or documenting that such callers must not grant contents: write), so the two-trust-model invariant is self-contained.

  • publish.yml PR_NUMBER unbound-variable fragility (~line 190). On the push/branch paths PR_NUMBER is never assigned locally; the script survives set -u only because PR_NUMBER arrives via the step env: block. If someone later drops PR_NUMBER from env:, the push path breaks with an unbound-variable error far from the edit. Initializing PR_NUMBER="" at the top of the script makes the invariant local and obvious.

  • publish.yml DATA_REPOSITORY not validated before clone (~line 263). The preview fallback clones https://github.com/$DATA_REPOSITORY.git without the ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ check applied to HEAD_REPOSITORY. It comes from a trusted workflow input so risk is minimal, but validating it would be consistent. (Optional.)

Comment thread .github/workflows/benchmark.yml Outdated
Comment thread test/workflow.test.js
@cpunion

cpunion commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the remaining review notes:

  • b086acb makes the reusable workflow-run mode reject every non-pull_request source, so the comment-only trust boundary no longer depends solely on the caller.
  • d113051 already initializes the PR number locally and validates the data repository before use.
  • The two inline maintainability comments were also addressed in d113051; their threads are now resolved.

Local npm test (34 tests) and npm run build pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant