Skip to content

ci: publish a GitHub Release on every release#52

Open
uipreliga wants to merge 3 commits into
mainfrom
ci/publish-github-releases
Open

ci: publish a GitHub Release on every release#52
uipreliga wants to merge 3 commits into
mainfrom
ci/publish-github-releases

Conversation

@uipreliga

@uipreliga uipreliga commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Why

Releases were tag-only. release.yml runs semantic-release with --no-vcs-release, so the repo has zero GitHub Releases despite tags up to v0.8.9. A published Release is what a GitHub Marketplace listing is cut from, and it is also where users look for notes.

What

Publish a GitHub Release (release.yml:229), after the tag pushes and before the wheel build:

  • Kept --no-vcs-release rather than dropping it. semantic-release also runs --no-push so the commit can be amended (uv.lock + the action.yml pin) and the tag re-pointed — the tag is therefore not on the remote at the point semantic-release would publish. Creating the Release explicitly after the push is the only correct ordering.
  • Notes are the CHANGELOG section semantic-release just generated for the version, written under $RUNNER_TEMP. Validated against every existing version in the real CHANGELOG.md, including the 0.8.1 vs 0.8.10 \b boundary and the oldest-version \Z terminator.
  • Falls back to --generate-notes with a ::warning:: if that section can't be located.
  • continue-on-error: true — see below.
  • Authenticates with the release app token, not GITHUB_TOKEN, preserving the invariant documented at release.yml:42 (workflow permissions stay contents: read; the app already carries contents: write).
  • Skipped for prerelease/branch dispatches, which never tag.

Refresh the action.yml version: pin — stale at 0.8.6 because the in-release bump step (release.yml:200) only landed after the last release was cut, so @main consumers were installing 0.8.6.

Review fixes (3454cbf)

A multi-model review (gemini-3.1-pro, gpt-5.6-sol, Opus) found two issues in the first commit, each confirmed independently by all three reviewers. Both are fixed:

  1. The notes file leaked into the published PyPI sdist. It was written to the repo root two steps before uv build, and pyproject.toml declares no [tool.hatch.build.targets.sdist] section, so hatchling's default file selection swept it in. Confirmed by building: coder_eval-0.8.9/release-notes.md was present in the tarball. Now written under $RUNNER_TEMP, outside the build tree.

    Adding the path to .gitignore would not have been a reliable fix — the sdist already ships evalboard/node_modules/** despite that path being git-ignored via evalboard/.gitignore.

  2. The step was a hard-fail gate between the irreversible pushes and the PyPI publish. main, v<version>, and the moving v<major> tag are all pushed before it runs, so a transient gh release create failure aborted the job, skipped Build wheel + sdist, and left publish-pypi unrun — stranding @vN on an action.yml pin whose version never reached PyPI. Now continue-on-error: true, mirroring the precedent already in this file: 522dbc7 ("tag only after PyPI publishes") made the GHCR steps best-effort for exactly this reason.

Two reviewers preferred a stronger version of fix 2 — moving the Release into its own job with needs: [release, publish-pypi], so a Release can never announce a version that is not installable. Not done here: it is a larger restructuring, and it does not address the pre-existing window where the moving vN tag is force-moved before PyPI publishes either. Worth doing as its own change.

Follow-ups (not in this PR)

  • uses: UiPath/coder_eval@v0 appears in README.md:105, README.md:142, docs/CI_GATE.md:22 and :67, but the v0 tag does not exist yet — action.yml landed after the v0.8.9 tag. The next release creates it (release.yml:225-227); until then those docs point at a ref that fails.
  • The sdist ships evalboard/node_modules/** to PyPI. Pre-existing and unrelated to this change, but it is real bloat plus third-party JS in a Python sdist. An sdist-contents guardrail is deferred in .claude/harness-candidates.md.
  • The action-dogfood job runs version: local, so the default PyPI-install path a real consumer takes is untested.

Test plan

  • make verify passed before and after the review fixes.
  • YAML parses; step ordering confirmed (Publish GitHub Release is step 13 of 20, after Move major action tag).
  • CHANGELOG extraction exercised locally against the real file for the newest and oldest versions; the $RUNNER_TEMP variant re-verified end to end (correct gh argv, repo root left clean).
  • sdist leak reproduced and then confirmed fixed by inspecting a built tarball.
  • End-to-end verification requires an actual dispatch — the next release cut from main will produce the first Release, create v0, and bump the pin to that version.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown

Claude finished @uipreliga's task in 1m 20s —— View job


Code Review in Progress

Review Checklist

  • Read review guidelines from .github/code_review.md
  • Read CLAUDE.md for project conventions
  • Analyze full diff (git diff origin/main...HEAD)
  • Read complete files for context
  • Check correctness and security
  • Verify CHANGELOG extraction logic
  • Assess error handling
  • Cross-file consistency checks
  • "What's Missing" analysis
  • Post comprehensive review

Starting review now...

@uipreliga uipreliga left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Review: coder_eval — pr:52

Scope: pr:52 · branch ci/publish-github-releases · 3454cbf · 2026-07-24T19:17Z · workflow variant

Change class: complex — adds a new CI publish step with embedded bash + Python control flow (CHANGELOG regex slicing, notes fallback branch, app-token handling, continue-on-error semantics) on the release path; correctness requires reasoning, and the rule says choose complex when uncertain

coder_eval is in excellent shape — architecture, error handling, test health, and the evaluation harness score a clean 10/10 with zero critical/high/medium findings, and nothing in this review can change a task's score or final_status for identical agent output; the only real risks are confined to the new "Publish GitHub Release" step in .github/workflows/release.yml, where unreviewed commit-message-derived CHANGELOG text becomes a first-party Release body (content/link spoofing), a ruleset-bypass app token is exported where a job-scoped GITHUB_TOKEN would suffice, and a locale-dependent read_text() could silently no-op the release under continue-on-error — bottom line: ship it, then tighten the release-publishing surface.

Summary

Axis Score 🔴 🟠 🟡 🔵 Top Issue
1. Code Quality & Style 9.9 / 10 0 0 0 1 Inaccurate/redundant block comments on the new Publish GitHub Release step
2. Type Safety 9.9 / 10 0 0 0 1 Notes-extraction heredoc reads/writes CHANGELOG.md without explicit encoding="utf-8"
3. Test Health 10 / 10 0 0 0 0
4. Security 9.8 / 10 0 0 0 2 Ruleset-bypass GitHub App token exported as GH_TOKEN where a job-scoped GITHUB_TOKEN would suffice (least privilege)
5. Architecture & Design 10 / 10 0 0 0 0
6. Error Handling & Resilience 10 / 10 0 0 0 0
7. API Surface & Maintainability 9.9 / 10 0 0 0 1 Deferred harness note overstates the sdist leak: node_modules only reproduces in a dirty local tree, not the published sdist
8. Evaluation Harness Quality 10 / 10 0 0 0 0

Overall Score: 9.9 / 10 · Weakest Axis: Security at 9.8 / 10
Totals: 🔴 0 · 🟠 0 · 🟡 0 · 🔵 5 across 8 axes.

Blockers

None.

Non-blocking, but please consider before merge

None.

Nits

  1. [Axis 1] Inaccurate/redundant block comments on the new Publish GitHub Release step (.github/workflows/release.yml:259) — Line 259 reads # the sdist that "Build wheel + sdist" publishes to PyPI two steps below. The Build wheel + sdist step is at line 282 — the immediately next step, not two below — and it does not publish to PyPI at all: it runs uv build, the artifact is uploaded at line 289, and the actual publish happens in the separate publish-pypi job (line 365). Reword to something like # the sdist that the next step ("Build wheel + sdist") produces and the publish-pypi job uploads. The underlying claim about hatchling's default file selection is correct — pyproject.toml has [tool.hatch.build.targets.wheel] (line 122) but no [tool.hatch.build.targets.sdist] section — only the positional wording is wrong.
  2. [Axis 2] Notes-extraction heredoc reads/writes CHANGELOG.md without explicit encoding="utf-8" (.github/workflows/release.yml:266) — Line 266 is text = pathlib.Path("CHANGELOG.md").read_text() and line 268 is pathlib.Path(os.environ["NOTES_FILE"]).write_text(m.group(1).strip() if m else "") — both rely on the ambient locale encoding. CHANGELOG.md verifiably contains non-ASCII (, , ), so under a non-UTF-8 locale the read_text() raises UnicodeDecodeError, set -euo pipefail aborts the step, and continue-on-error: true (line 246) swallows it — no Release is published and the job still goes green. PEP 538 coercion makes this latent rather than live on ubuntu-latest, but the fix is one keyword: read_text(encoding="utf-8") / write_text(..., encoding="utf-8"). Same nit applies to the pre-existing sibling heredoc at lines 163/166, so fixing both keeps the file consistent.
  3. [Axis 4] Ruleset-bypass GitHub App token exported as GH_TOKEN where a job-scoped GITHUB_TOKEN would suffice (least privilege) (.github/workflows/release.yml:250) — Line 250 exports the GitHub App installation token into the new step: GH_TOKEN: ${{ steps.app-token.outputs.token }}, justified by the comment on lines 248-249 ("The app token, not GITHUB_TOKEN: the workflow's permissions are contents: read, and the release app already carries contents: write."). That app token is the one the workflow header (lines 40-43) describes as having the ruleset bypass on main — strictly more authority than gh release create needs. A narrower alternative exists and is not used: add a job-level permissions: block granting contents: write (alongside the existing packages: write) and set GH_TOKEN: ${{ github.token }} for this step; the ephemeral, repo-scoped GITHUB_TOKEN can create releases and cannot bypass the branch ruleset. Mitigating facts that hold this at Low rather than higher: actions/create-github-app-token calls core.setSecret so the value is log-masked; actions/checkout (line 87) already persists the same token in .git/config for the whole job, so every step in this job can already read it; and the step's run: body (lines 254-280) executes no third-party code — only python3 from a quoted heredoc and the preinstalled gh, neither of which echoes the environment. So this is hygiene, not an exploitable hole. Verified-clean adjacent claims (no finding): (a) the comment on lines 251-252 ("Passed via env (not interpolated into the script) per GitHub's injection guidance") is TRUE — the new step's run: body contains zero ${{ }} expressions; the only two ${{ }}-into-run: interpolations in the whole file are pre-existing at line 215 (git push origin main "v${{ steps.release.outputs.version }}") and line 307 (echo '${{ github.repository_owner }}'), neither attacker-controlled; (b) steps.release.outputs.version originates from $PSR version --print (line 146), i.e. semantic-release's own PEP440 computation over pyproject.toml, and is not attacker-injectable; (c) the heredoc at line 263 IS single-quoted (<<'PY'), so no shell expansion into the Python body, and re.escape(version) on line 267 additionally blocks regex injection; (d) the sdist-leak mitigation on lines 256-260 is correct — I empirically built a scratch hatchling sdist and confirmed an untracked, non-root-gitignored file at the project root IS packaged, so writing to ${RUNNER_TEMP}/release-notes.md rather than the repo root genuinely prevents the notes file from reaching the PyPI sdist. CVSS:3.1/AV:L/AC:H/PR:H/UI:N/S:U/C:N/I:L/A:N
  4. [Axis 4] Unreviewed commit-message-derived CHANGELOG text published verbatim as the GitHub Release body (content/link spoofing on a first-party distribution surface) (.github/workflows/release.yml:276) — Lines 263-280 slice a CHANGELOG.md section and publish it unmodified as a GitHub Release body: pathlib.Path(os.environ["NOTES_FILE"]).write_text(m.group(1).strip() if m else "") (line 268), then NOTES=(--notes-file "$NOTES_FILE") (line 271) feeding gh release create "v${VERSION}" --title "v${VERSION}" --verify-tag --latest "${NOTES[@]}" (lines 276-280). Trace the content: CHANGELOG.md is regenerated by semantic-release during this same run ($PSR version "--$BUMP" ... --changelog, line 144) from commit subjects since the last tag, and the amended commit is pushed straight to main with the app token's ruleset bypass — so that text reaches a published Release with no PR review of the rendered result. The current CHANGELOG shows commit subjects going in verbatim as markdown, e.g. - Code review fixes for welch-t-test-exact ([#38](https://github.com/UiPath/coder_eval/pull/38), ...). A contributor whose PR title/commit subject is approved for its code (not its markdown) can therefore land a link like [install from here](https://evil.example) into a first-party-looking Release body, which GitHub also fans out in release-notification emails and is what Marketplace listings render (the stated motivation on lines 232-233). Same class applies to the --generate-notes fallback (line 274), which GitHub builds from PR titles. Impact is capped at content/link spoofing — GitHub sanitizes raw HTML/script in release bodies, so no XSS — and this PR is what first creates the surface (no Release existed before). Recommendation: this is acceptable-with-eyes-open for a repo with mandatory review, but state it: either add a human gate (e.g. create the release as a draft with --draft and publish after a glance) or strip/neutralize bare markdown links in the sliced notes before writing $NOTES_FILE. If neither, record the accepted risk in the step comment next to lines 235-237 so a future reader does not assume the notes are trusted content. CVSS:3.1/AV:N/AC:H/PR:L/UI:R/S:C/C:N/I:L/A:N
  5. [Axis 7] Deferred harness note overstates the sdist leak: node_modules only reproduces in a dirty local tree, not the published sdist (.claude/harness-candidates.md:37) — Lines 36-39 read:
  `coder_eval-X.Y.Z/release-notes.md` (verified by building it), and the sdist
  *already* ships `evalboard/node_modules/**` even though that path is git-ignored
  via `evalboard/.gitignore` — so `.gitignore` is not a reliable exclusion
  mechanism here. Nothing guards what reaches PyPI today.

The mechanism is real — I confirmed it with hatchling's own selector (SdistBuilder(".").recurse_included_files() yields evalboard/node_modules/.modules.yaml, evalboard/node_modules/.bin/autoprefixer, evalboard/.next/server/vendor-chunks/*, because hatchling honors only the root .gitignore, not the nested evalboard/.gitignore). But it does not describe the published artifact: release.yml's job never runs npm/pnpm install, so a fresh CI checkout has no evalboard/node_modules at uv build time, and the actual PyPI sdist for 0.8.9 is 7,540,654 bytes (https://pypi.org/pypi/coder-eval/0.8.9/json) versus 526 MB for evalboard/node_modules locally. Framing a dirty-worktree artifact as "what reaches PyPI today" will make whoever picks this item up mis-prioritize it. Reword to scope the claim, e.g. "a local uv build sweeps in evalboard/node_modules/** and evalboard/.next/** because hatchling reads only the root .gitignore; CI is currently spared only because it never installs node deps — so any untracked file a workflow leaves in the tree before uv build does reach PyPI (which is why the new step writes notes to $RUNNER_TEMP)."

What's Missing

Tests:

  • 🟡 Tests — 🟠 action.yml: this PR hand-fixes a pin drift (0.8.60.8.9) that nothing detects. The invariant is mechanical and always true at rest — action.yml's version: default must equal pyproject.toml's [project].version (release.yml:200-201 sed-bumps it in the release commit) — yet no test, lint rule (CE0nn), or CI check asserts it, which is exactly why action.yml shipped in 74db6fa with a stale 0.8.6 while main was already 0.8.9. Add a one-line parity test (or a CE0nn doc-surface rule alongside CE027–CE031) so @v0 consumers can never silently install a version other than the tag they pinned; also record it in .claude/harness-candidates.md, which this PR touches but only records the sdist item. (trigger: action.yml)
  • 🟡 Tests — 🟠 .github/workflows/release.yml:238-280: the new step is unreachable by the workflow's own dry-run affordance. The header (lines 17-22) advertises a branch dispatch as the way to rehearse a release, but the step is gated if: steps.mode.outputs.prerelease != 'true' && steps.release.outputs.version != '', so a prerelease dispatch skips it entirely — the ~27 lines of new bash + inline Python (CHANGELOG slice, $NOTES_FILE empty-check, gh release create --verify-tag) will execute for the first time against production main, after the tag has already been pushed. At minimum, unit-test the notes-extraction heredoc's regex against the real CHANGELOG.md (it is pure text processing and needs no network), or let the prerelease path run it with --draft/gh release create --dry-run-equivalent so the logic is exercised before it matters. (trigger: .github/workflows/release.yml)
  • 🔵 Tests — 🔵 .github/workflows/release.yml:254-280: the PR adds ~27 lines of shell + inline Python to a workflow, and the repo has no static gate over .github/workflows/**grep -rn 'actionlint|shellcheck|yamllint' across .github/, Makefile, and .pre-commit-config.yaml returns nothing, and make verify never looks at workflow YAML. actionlint (which runs shellcheck over run: bodies and pyflakes over inline python3 heredocs) would mechanically cover the encoding, quoting, and array-expansion shapes reviewed by hand here, and would subsume the deferred CE026 SHA-pinning candidate already sitting in .claude/harness-candidates.md:14-17. (trigger: .github/workflows/release.yml)

Daily/nightly:

  • 🟡 Daily/nightly pipeline impact — 🟠 .github/workflows/release.yml:238: the new step is inserted into the critical release path, before Build wheel + sdist (282) and Upload dist for PyPI publish (289), inside a 15-minute timeout-minutes job budget — and the PR states no blast radius for the release/nightly contract. continue-on-error: true covers a non-zero exit but NOT a hung or slow gh API call: that consumes the job budget and cancels the job, producing precisely the failure mode the step's own comment (lines 240-245) says must never happen — main bumped, vX.Y.Z + moving v0 tags pushed, action.yml pinned to a version that never reached PyPI, and the ADO nightly's coderEvalVersion pin unresolvable. Moving the step to after Upload dist for PyPI publish removes the risk entirely at zero cost (the tag it verifies is already on the remote). (trigger: .github/workflows/release.yml)

Downstream consumers:

  • 🔵 Downstream consumers — 🟡 .github/workflows/release.yml:267: the notes slice hard-codes semantic-release's current changelog rendering (^## v{version}\b … (?=^## v|\Z)), but nothing couples the two. pyproject.toml's [tool.semantic_release.changelog] (line 369) uses default_templates; a template/mode change, a v-less heading, or a future changelog.exclude_commit_patterns tweak silently degrades every release body to --generate-notes — and the only signal is a ::warning:: (line 273) inside a continue-on-error: true step, so the job still goes green and nobody notices the Release notes stopped matching the CHANGELOG. Either assert the regex against CHANGELOG.md in a test, or make the miss loud (fail the step rather than warn) now that the tag is already pushed and a retry is cheap. (trigger: .github/workflows/release.yml)
  • 🔵 Downstream consumers — 🟡 .github/workflows/release.yml:238-280: nothing backfills the surface this PR creates. git ls-remote --tags origin shows v0.8.1v0.8.9 with no v0 tag, and gh release list returns zero releases — so the Releases page will begin at whatever version is cut next, and the uses: UiPath/coder_eval@v0 form documented in README.md:105,142 and docs/CI_GATE.md:22,67 stays unresolvable until then. Since the stated motivation is Marketplace listings (line 233), also note the remaining manual step: gh release create cannot tick GitHub's "Publish this Action to the Marketplace" box, so the listing still requires a one-time human action that no comment or doc records. (trigger: .github/workflows/release.yml)

Parallel paths:

  • 🔵 Parallel paths — 🟡 .github/workflows/release.yml:3-22: the file's own header block is the SSOT for what a release does, and it was not updated. Line 3-4 still enumerates the outputs as "bump the version, tag, build, and publish to public PyPI (wheel) and GHCR (agent image)" with no mention of a GitHub Release, and the PRERELEASE paragraph's explicit exclusion list (lines 19-20, "It does NOT move :latest, tag, commit, or push to main") was not extended with "or create a GitHub Release" — even though that asymmetry is a deliberate design decision of this PR. A reader of the header now gets a materially incomplete picture of the release contract. (trigger: .github/workflows/release.yml)
  • 🔵 Parallel paths — 🔵 .github/workflows/release.yml:163,166: the pre-existing prerelease-stamping heredoc has the identical read_text() / write_text()-without-encoding= shape as the new notes heredoc, and was not updated alongside it; the repo already enforces this pattern for Python source via the read_text_explicit_encoding lint rule, but that rule never sees code embedded in workflow YAML. Fix both in one pass so the file is internally consistent (and so an eventual actionlint/CE0nn extension has nothing left to flag). (trigger: .github/workflows/release.yml) (restates: Axis 2: Notes-extraction heredoc reads/writes CHANGELOG.md without explicit encoding="utf-8")

Harness & Lint Improvements

Static checks (lint / type):

  • [ce-lint] CE032 — run the existing AST rules over Python embedded in .github/workflows/*.yml. Root cause of the encoding finding is surface, not pattern: CE008/CE009/CE010 already forbid unencoded read_text/open/subprocess.run, but tests/lint/runner.py::check_paths only walks *.py under src/, so Python living inside a run: heredoc is invisible to ruff, pyright, pytest coverage and the CE runner. Add tests/lint/workflow_python.py: scan .github/workflows/*.yml for interpreter heredocs (python3 - <<'PY' … PY), textwrap.dedent the body, ast.parse it, and feed the tree through the existing ALL_RULES (at minimum the encoding trio), mapping violation line numbers back to the YAML line (heredoc start + offset). Like CE027–CE031 it reasons over non-.py files, so wire it as tests/test_custom_lint.py::TestCE032WorkflowEmbeddedPython rather than as a BaseRule in the runner. CE032 is the next free number. I prototyped the extractor (~30 lines) against the live tree: it parses all 6 heredocs successfully and reports exactly release.yml:266 (read_text), release.yml:268 (write_text), plus the pre-existing release.yml:163/166 and publish-testpypi.yml:79/82. Prevents: Finding #2 (release.yml:266/268read_text()/write_text() without encoding="utf-8" on a CHANGELOG.md that verifiably contains → — ✓), and the four identical pre-existing instances the finding calls out as siblings.
  • [ce-lint] Extend CE008 from read_text to write_text. tests/lint/rules/read_text_explicit_encoding.py matches only .read_text(...); nothing in the repo forbids Path.write_text(...) without encoding=. src/ happens to be clean today (all 19 call sites pass encoding=, verified by AST walk — several as trailing kwargs on multi-line calls), so this is purely a rot guard on an invariant currently held by convention. One-line change: match node.func.attr in {"read_text", "write_text"} and reword the message; the docstring already says "Same root cause as CE008 (read_text / write_text)" in open_explicit_encoding.py, i.e. the intent was always both. Prevents: The write half of finding #2 (release.yml:268) once CE032 routes workflow heredocs through the same rules, and future unencoded write_text in src/.
  • [ruff] Consider PLW1514 (unspecified-encoding) instead of hand-maintaining CE008/CE009/CE010. Ruff's native rule covers open, read_text, write_text, subprocess text mode in one flag and has an autofix. Caveat that keeps this second-choice, not first: ruff rule PLW1514 reports it is preview-only (uv run ruff check --select PLW1514 warns "has no effect because preview is not enabled"), so adopting it means lint.preview = true, which opts every other selected rule into preview churn. Recommendation: prefer the CE008 extension above for stability, and revisit PLW1514 when it stabilizes. Note ruff cannot see YAML either way, so it never replaces CE032. Prevents: Same class as finding #2, plus tests/ (which the CE runner does not scan) — recorded here so the choice of custom rule over the ruff-native option is deliberate rather than an oversight.
  • [ce-lint] CE033 — interpreter heredocs in .github/workflows/** must use a quoted delimiter (<<'PY', not <<PY). With an unquoted tag, the shell expands $VAR / ${{ }} into the program text before the interpreter sees it, so a value containing a quote or newline breaks out of the Python string literal — the exact hazard the reviewed step's own comment (release.yml:251-252, "Passed via env … per GitHub's injection guidance") sets out to avoid, and which the review verified clean for the new step. There is a live violation in-tree: publish-testpypi.yml:67 uses python3 - <<PY and interpolates f'{key} = "${DEV_VERSION}"' (line 79) directly into the Python source. Regex-detectable in ~10 lines; naturally ships alongside CE032, which needs the same heredoc scanner (and whose ast.parse is only sound on quoted bodies). Prevents: Hardens the property finding #4 verified by hand at release.yml:263 (heredoc is <<'PY', so no shell expansion into the Python body) so it cannot regress, and fixes the existing counter-example at publish-testpypi.yml:67.
  • [bandit-codeql] Add zizmor (GitHub Actions static analyzer) + actionlint to make verify and the pr-checks workflow. grep confirms neither is present today, so no static analysis whatsoever runs over .github/workflows/** — every workflow finding in this review was caught by a human reading YAML. Three zizmor rules map one-to-one onto the findings: excessive-permissions (flags the missing job-level permissions: contents: write that forces the ruleset-bypass app token into env: at release.yml:250), artipacked (flags actions/checkout persisting that same credential in .git/config for the whole job — cited in finding #4 as the mitigating-but-unhygienic fact at release.yml:87), and template-injection (the ${{ }}-into-run: interpolations at release.yml:215 and :307, verified benign here but unguarded against future edits). Run as a non-blocking annotation job first, then promote to blocking once the existing backlog is triaged. Prevents: Finding #4 (release.yml:250 least-privilege token) directly, and pre-emptively the injection class finding #4 had to verify by hand across the whole file.

Harness improvements (not statically reachable):

  • Lift the release-notes slicing out of the heredoc into a tested module. Replace release.yml:263-269 with python3 scripts/release_notes.py (or python -m tests.tools.release_notes) exposing extract_section(changelog_text, version) -> str, plus unit tests over fixture CHANGELOGs: version present, version absent (empty → --generate-notes fallback), section is the last one in the file (\Z branch), a version string containing regex metacharacters, and a fixture carrying the real → — ✓ bytes. The regex at line 267 (^## v{re.escape(version)}\b.*?$(.*?)(?=^## v|\Z)) is non-trivial, has three failure modes, and is currently exercised only in production during an actual release. Why not static: The defects are behavioral — wrong slice boundary, empty notes, decode failure on real bytes — and need execution against fixture inputs. Code inside a YAML heredoc is also structurally unreachable to ruff, pyright, pytest and coverage; CE032 above only recovers the lint layer, never the test layer. Moving the code is the only fix that restores all four. Prevents: Finding #2 (encoding, which a non-ASCII fixture test fails on immediately) and finding #1 — a module with a docstring removes the drifting positional prose ("two steps below", "publishes to PyPI") that the inline comment at release.yml:259 got wrong; that wording error is genuinely not mechanically detectable, so relocating the surface is the only preventive move available.
  • Make continue-on-error: true steps loud. Add an id: to the Publish-GitHub-Release step and a following always()-guarded step that fails-soft with ::error (or opens an issue / posts to the release channel) when steps.<id>.outcome != 'success'; complement with a scheduled check asserting every published tag vX.Y.Z has a corresponding GitHub Release. Today release.yml:246 swallows any failure of the new step and the job still goes green. Why not static: Requires the runtime outcome of a real workflow run (and the live tag/release state on GitHub); no analyzer can know whether the step failed. Prevents: The silent-failure amplifier on finding #2 — under a non-UTF-8 locale the UnicodeDecodeError aborts the step, no Release is published, and nothing anywhere reports it.
  • Add an sdist manifest guard to CI. A job that runs uv build --sdist and asserts the tarball's member list against an allowlist: no evalboard/node_modules/**, no evalboard/.next/**, no unexpected files at the package root. Optionally back it with an explicit [tool.hatch.build.targets.sdist] include/exclude in pyproject.toml (which today has only the wheel target at line 122), so the contract is declared rather than inferred from hatchling's default file selection. Why not static: Hatchling's selector is the ground truth and only reveals itself by running — the review had to build a scratch sdist to establish that untracked root files are packaged while nested .gitignores are not honored. No grep over pyproject.toml or .gitignore predicts that. Prevents: Finding #5 (.claude/harness-candidates.md:37) — a machine-produced manifest replaces the disputed prose about whether node_modules reaches PyPI (local dirty build vs. published 0.8.9 artifact), and future readers get a fact instead of a conflation. Also permanently enforces the invariant the new step's $RUNNER_TEMP comment (release.yml:256-260) currently protects by convention only.
  • Gate the Release body's provenance. Either (a) create the Release with --draft and publish after a glance, or (b) add a pr-checks job rejecting markdown link syntax / bare URLs in PR titles — the squash subject becomes the CHANGELOG line (release.yml:144) and thence the verbatim Release body (release.yml:271-280). If neither is adopted, record the accepted risk in the step comment next to release.yml:235-237 so the notes are not mistaken for reviewed content. Why not static: The untrusted text lives in PR/commit metadata, not in any file a code scanner reads; option (b) needs the GitHub PR context at CI time, option (a) is a human gate by definition. Prevents: Finding #4b (release.yml:276) — an approved-for-code PR title carrying [install from here](https://evil.example) landing in a first-party Release body that GitHub fans out over email and Marketplace listings.

Top 5 Priority Actions

  1. Gate or sanitize the Release body at .github/workflows/release.yml:276 — the CHANGELOG slice written at line 268 is derived from commit subjects and published verbatim to a first-party surface (and release-notification emails) with no review of the rendered markdown, so either create it with --draft, strip bare markdown links before writing $NOTES_FILE, or record the accepted risk in the step comment near lines 235-237.
  2. Add encoding="utf-8" to read_text()/write_text() at .github/workflows/release.yml:266,268 (and the pre-existing sibling heredoc at lines 163/166) — CHANGELOG.md contains non-ASCII, so a non-UTF-8 locale raises UnicodeDecodeError, set -euo pipefail aborts the step, and continue-on-error: true (line 246) swallows it: no Release published, job still green.
  3. Drop to least privilege at .github/workflows/release.yml:250 — replace the ruleset-bypass GitHub App token with a job-level permissions: contents: write plus GH_TOKEN: ${{ github.token }}, since gh release create needs none of the branch-protection bypass authority the app token carries.
  4. Rescope the deferred sdist note at .claude/harness-candidates.md:37 — it presents a dirty-local-worktree artifact (evalboard/node_modules swept in because hatchling honors only the root .gitignore) as "what reaches PyPI today", which is false for CI (the 0.8.9 sdist is 7.5 MB) and will make whoever picks the item up mis-prioritize it.
  5. Fix the misleading step comment at .github/workflows/release.yml:259 — "Build wheel + sdist" is the immediately next step (line 282), not two below, and it does not publish to PyPI (uv build only; the publish-pypi job at line 365 does), so reword while keeping the correct hatchling default-selection rationale.

Stats: 0 🔴 · 0 🟠 · 0 🟡 · 5 🔵 across 8 axes reviewed.

@bai-uipath bai-uipath left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fix what you agree with, otherwise lgtm

  • Move the step after Upload dist for PyPI publish (release.yml:238 → after :294). It currently sits at the earliest legal point after the tag push, which maximizes the window where a Release exists for a version that isn't installable.

  • continue-on-error here is silent (release.yml:246). The rationale is right, but a gh release create failure leaves the job green with only a step-level annotation that nobody reads on a release run — so the failure class this PR fixes (no Releases) can recur unnoticed.

uipreliga added a commit that referenced this pull request Jul 24, 2026
Reviewer items acted on:

- Move "Publish GitHub Release" after "Build wheel + sdist" and "Upload dist
  for PyPI publish". Those are the last steps that can still fail for an
  already-tagged version, so a Release no longer announces a version whose
  artifacts never built, and a hung `gh` call can no longer eat the 15-minute
  job budget before the artifacts are safe.

- Make the swallowed `continue-on-error` failure loud: a new "Flag missing
  GitHub Release" step re-raises it as an ::error annotation plus a
  run-summary block with the by-hand recovery command, without failing the job
  (which would skip the `needs: release` PyPI publish).

- Extract the CHANGELOG notes slicing out of the `run:` heredoc into
  .github/scripts/release_notes.py, covered by tests/test_release_notes.py.
  Heredoc code is invisible to ruff, pyright, pytest and coverage, and this
  regex has three failure modes on a path that runs once per release against
  production main with no rehearsal (the prerelease dispatch skips the step).
  Tests pin the load-bearing \b (0.8.1 must not slice the v0.8.10 section),
  the \Z branch, re.escape, the empty-file fallback contract, and couple the
  regex to semantic-release's real rendering of CHANGELOG.md.

- Pin encoding="utf-8" on every read_text/write_text in workflow-embedded
  Python (release.yml, publish-testpypi.yml). CHANGELOG.md carries non-ASCII,
  so a non-UTF-8 locale raised UnicodeDecodeError inside a
  continue-on-error step: no Release, still green.

- Quote the publish-testpypi.yml heredoc delimiter (<<'PY') and read
  DEV_VERSION from os.environ instead of expanding it into the Python source.

- Add tests/test_action_version_pin.py: action.yml's `version:` default must
  equal pyproject.toml's version, and the `# <-- kept in sync` sed anchor must
  be present and unique. Nothing detected the 0.8.6-vs-0.8.9 drift this PR
  hand-fixed, which would have had @v0 consumers installing a version other
  than the tag they pinned.

- Fix the misleading step comment ("two steps below", "publishes to PyPI") and
  update the workflow header, which still omitted the GitHub Release from both
  the outputs list and the PRERELEASE exclusion list.

- Record the accepted risk on the Release body: notes render squashed PR
  titles, so a first-party surface carries text reviewed as code, not markdown.
  Also note that Marketplace listing needs a one-time manual checkbox.

- Widen the Makefile lint scope to .github/scripts/ so extracted release
  tooling is actually linted.

Rescoped the deferred sdist note in .claude/harness-candidates.md: it claimed
the sdist "already ships evalboard/node_modules/**" to PyPI. Verified false for
published artifacts — the 0.8.9 and 0.8.2 sdists on PyPI are ~7.5 MB with zero
node_modules entries, because CI never runs pnpm install. It reproduces only in
a developed local worktree (135 MB, 8520 files), since hatchling honors just the
root .gitignore. The live hazard is untracked files a workflow leaves at the
root, which is what the $RUNNER_TEMP note guards. Also recorded CE032/CE033 and
actionlint/zizmor as deferred candidates.

Declined: swapping the app token for GITHUB_TOKEN + job-level contents: write.
actions/checkout already persists the same app token in .git/config for every
step in the job, so scoping it out of this one step's env buys no isolation
while adding a second write credential. Rationale recorded in the step comment.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@uipreliga

Copy link
Copy Markdown
Collaborator Author

Review responses — addressed in f2329dd

Thanks both. Everything I agreed with is in f2329dd. make check, make lint (166 passed), and make test (3436 passed, 7 pre-existing skips) are green.

Fixed

# Item Resolution
bai-uipath #1 / uipreliga Daily-nightly Step sits at the earliest legal point after the tag push Moved Publish GitHub Release to after Build wheel + sdist and Upload dist for PyPI publish — those are the last steps that can still fail for an already-tagged version. Also stops a hung gh call from consuming the 15-min job budget before the artifacts are safe. Documented that this narrows the window rather than closing it: publish-pypi is a separate job, so the real upload still follows.
bai-uipath #2 continue-on-error is silent New Flag missing GitHub Release step re-raises the swallowed failure as an ::error annotation plus a $GITHUB_STEP_SUMMARY block containing the by-hand recovery command. It deliberately does not fail the job — publish-pypi is needs: release, so failing here would skip the PyPI publish of an already-tagged version, i.e. the exact stranded-@vN state the step comment warns about.
Nit 2 + Parallel paths read_text()/write_text() without encoding= Pinned encoding="utf-8" on all four call sites — the two new ones and the pre-existing siblings in release.yml:163/166 and publish-testpypi.yml.
Tests #2 New step is unreachable by the prerelease dry-run; regex only ever runs in production Extracted the notes slicing into .github/scripts/release_notes.py with tests/test_release_notes.py (12 tests). See below — this found a real latent bug.
Tests #1 action.ymlpyproject.toml pin drift is undetected Added tests/test_action_version_pin.py, asserting the version: default equals [project].version and that the # <-- kept in sync sed anchor is present and unique (release.yml's bump silently no-ops without it). Mutation-tested against the exact historical drift — reverting the pin to 0.8.6 fails with an actionable message.
Nit 1 Misleading step comment Reworded. Also updated the file header, which still omitted the GitHub Release from both the outputs list (L3-4) and the PRERELEASE exclusion list (L19-20).
Nit 4 Unreviewed commit-derived text in the Release body Took the "record the accepted risk" option — see reasoning below.
Downstream consumers #2 Marketplace listing needs a manual step nothing records Noted in the step comment: gh release create cannot tick the "Publish this Action to the Marketplace" checkbox; that stays a one-time UI action on the first Release.
(not raised) .github/scripts/ would be unlinted Widened the Makefile lint scope to include it. Leaving extracted tooling invisible to ruff would forfeit the reason for extracting it.

The extraction found a real bug. The trailing \b in the section regex is load-bearing in a way worth calling out: without it, extract_section(changelog, "0.8.1") matches the ## v0.8.10 heading and publishes a release whose body is a different version's changelog. That is now a named test, alongside the \Z (last-section) branch, re.escape on PEP 440 local versions, the empty-file → --generate-notes fallback contract, and a test coupling the regex to semantic-release's actual rendering of the real CHANGELOG.md — which also closes Downstream consumers #1 (a [tool.semantic_release.changelog] template change now fails make test instead of silently degrading every Release body behind a ::warning:: in a continue-on-error step).

Declined, with reasoning

Nit 3 — swap the app token for GITHUB_TOKEN + job-level permissions: contents: write. Not a net reduction in authority. The finding's own mitigating facts establish why: actions/checkout (L87) already persists the same app token in .git/config for every step in the job. Scoping it out of this one step's env therefore buys no isolation, while granting the job contents: write adds a second write credential that didn't exist before. The step's run: body executes no third-party code, so there's no vector the change would close. Rationale is now recorded in the step comment so it isn't re-litigated. Happy to revisit if paired with persist-credentials: false, but the job needs that credential to push the bump commit.

Nit 4 — --draft or link-stripping. Took option (c), documenting the accepted risk in the step comment. --draft defeats the automation this PR exists to provide, and stripping markdown links would also strip the legitimate PR/commit links that make the notes useful. The risk is bounded to content/link spoofing (GitHub strips raw HTML from release bodies) and gated by mandatory PR review; the comment records that the notes are not reviewed content and names --draft + link-stripping as the escalation if this repo ever takes drive-by contributions.

One correction: the sdist / node_modules claim

Nit 5 is right, and I want to put numbers behind it because the original note (which I wrote) was wrong in a way that would have mis-prioritized the follow-up. No published sdist has ever shipped node_modules:

sdist size files node_modules entries
PyPI coder_eval-0.8.9.tar.gz 7,540,654 B 555 0
PyPI coder_eval-0.8.2.tar.gz 7,435,290 B 536 0
local uv build --sdist, developed worktree 135,399,741 B 9,280 8,520

CI is spared because release.yml never runs npm/pnpm install, so those paths don't exist on the runner at uv build time. The mechanism is real (hatchling honors only the root .gitignore, so the nested evalboard/.gitignore is ignored — evalboard/.next/** leaks too, 190 files), but it reproduces only locally. Worth adding: a dirty-tree release wouldn't silently ship third-party JS either, since 135 MB exceeds PyPI's 100 MB per-file limit — the failure mode is a broken release, not a stealth one.

The genuinely live hazard is the other half: untracked files a workflow leaves at the repo root, which hatchling does package. That's what the $RUNNER_TEMP guard is for, and it's enforced by convention only. .claude/harness-candidates.md now separates the two.

Deferred to .claude/harness-candidates.md

Per the repo's convention for deferred guardrails, rather than expanding this PR: CE032 (run the existing CE008/009/010 AST rules over Python embedded in workflow YAML — plus extending CE008 to write_text), CE033 (quoted heredoc delimiters — there was a live violation at publish-testpypi.yml:67 interpolating ${DEV_VERSION} into Python source, fixed here since I was already in the file), actionlint + zizmor over .github/workflows/** (subsumes the existing CE026 candidate), and the sdist manifest guard paired with an explicit [tool.hatch.build.targets.sdist] allowlist.

uipreliga and others added 3 commits July 24, 2026 14:55
Releases were tag-only: semantic-release runs with --no-vcs-release, so the
repo had zero GitHub Releases. A published Release is what a Marketplace
listing is cut from, and it is also where users look for notes.

Create the Release explicitly after the tag pushes rather than dropping
--no-vcs-release: semantic-release runs --no-push so the commit can be
amended (uv.lock + the action.yml pin) and the tag re-pointed, which means
the tag is not on the remote at the point semantic-release would publish.

Notes come from the CHANGELOG section semantic-release just generated for
the version, falling back to GitHub's generated notes with a ::warning::
rather than failing a release that has already pushed main and tags. The
step authenticates with the release app token, not GITHUB_TOKEN, so the
workflow's contents: read permission stays as documented.

Also refresh the action.yml `version:` pin, which was stale at 0.8.6 — the
in-release bump step only landed after the last release was cut, so @main
consumers were installing 0.8.6.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two findings, each confirmed independently by all three reviewers
(gemini-3.1-pro, gpt-5.6-sol, opus).

Write the release notes under RUNNER_TEMP instead of the repo root. The step
wrote release-notes.md into the working tree two steps before `uv build`, and
hatchling has no [tool.hatch.build.targets.sdist] config, so its default file
selection swept the file into the published sdist. Verified by building:
`coder_eval-0.8.9/release-notes.md` was present in the tarball. Adding the
path to .gitignore would not have been a reliable fix -- the sdist already
ships evalboard/node_modules/** despite that path being git-ignored.

Make the step best-effort. main, the version tag, and the moving major tag are
all pushed by the time it runs, so a transient `gh release create` failure
aborted the job, skipped `Build wheel + sdist`, and left publish-pypi unrun --
stranding `@vN` on an action.yml pin whose version never reached PyPI. This
mirrors the existing precedent in the same file: 522dbc7 ("tag only after PyPI
publishes") made the GHCR steps continue-on-error for exactly this reason.

Also narrows the header comment, which claimed a broader fallback than the
`if m else ""` branch actually provided, and defers an sdist-contents
guardrail to .claude/harness-candidates.md.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reviewer items acted on:

- Move "Publish GitHub Release" after "Build wheel + sdist" and "Upload dist
  for PyPI publish". Those are the last steps that can still fail for an
  already-tagged version, so a Release no longer announces a version whose
  artifacts never built, and a hung `gh` call can no longer eat the 15-minute
  job budget before the artifacts are safe.

- Make the swallowed `continue-on-error` failure loud: a new "Flag missing
  GitHub Release" step re-raises it as an ::error annotation plus a
  run-summary block with the by-hand recovery command, without failing the job
  (which would skip the `needs: release` PyPI publish).

- Extract the CHANGELOG notes slicing out of the `run:` heredoc into
  .github/scripts/release_notes.py, covered by tests/test_release_notes.py.
  Heredoc code is invisible to ruff, pyright, pytest and coverage, and this
  regex has three failure modes on a path that runs once per release against
  production main with no rehearsal (the prerelease dispatch skips the step).
  Tests pin the load-bearing \b (0.8.1 must not slice the v0.8.10 section),
  the \Z branch, re.escape, the empty-file fallback contract, and couple the
  regex to semantic-release's real rendering of CHANGELOG.md.

- Pin encoding="utf-8" on every read_text/write_text in workflow-embedded
  Python (release.yml, publish-testpypi.yml). CHANGELOG.md carries non-ASCII,
  so a non-UTF-8 locale raised UnicodeDecodeError inside a
  continue-on-error step: no Release, still green.

- Quote the publish-testpypi.yml heredoc delimiter (<<'PY') and read
  DEV_VERSION from os.environ instead of expanding it into the Python source.

- Add tests/test_action_version_pin.py: action.yml's `version:` default must
  equal pyproject.toml's version, and the `# <-- kept in sync` sed anchor must
  be present and unique. Nothing detected the 0.8.6-vs-0.8.9 drift this PR
  hand-fixed, which would have had @v0 consumers installing a version other
  than the tag they pinned.

- Fix the misleading step comment ("two steps below", "publishes to PyPI") and
  update the workflow header, which still omitted the GitHub Release from both
  the outputs list and the PRERELEASE exclusion list.

- Record the accepted risk on the Release body: notes render squashed PR
  titles, so a first-party surface carries text reviewed as code, not markdown.
  Also note that Marketplace listing needs a one-time manual checkbox.

- Widen the Makefile lint scope to .github/scripts/ so extracted release
  tooling is actually linted.

Rescoped the deferred sdist note in .claude/harness-candidates.md: it claimed
the sdist "already ships evalboard/node_modules/**" to PyPI. Verified false for
published artifacts — the 0.8.9 and 0.8.2 sdists on PyPI are ~7.5 MB with zero
node_modules entries, because CI never runs pnpm install. It reproduces only in
a developed local worktree (135 MB, 8520 files), since hatchling honors just the
root .gitignore. The live hazard is untracked files a workflow leaves at the
root, which is what the $RUNNER_TEMP note guards. Also recorded CE032/CE033 and
actionlint/zizmor as deferred candidates.

Declined: swapping the app token for GITHUB_TOKEN + job-level contents: write.
actions/checkout already persists the same app token in .git/config for every
step in the job, so scoping it out of this one step's env buys no isolation
while adding a second write credential. Rationale recorded in the step comment.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@uipreliga
uipreliga force-pushed the ci/publish-github-releases branch from f2329dd to 1e3feba Compare July 24, 2026 22:14
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.

2 participants