Skip to content

Gate patch releases on green release-candidate CI and report main-line backport checks as not applicable - #972

Draft
kriszyp wants to merge 6 commits into
mainfrom
fix/patch-release-verify-gates
Draft

kriszyp wants to merge 6 commits into
mainfrom
fix/patch-release-verify-gates

Conversation

@kriszyp

@kriszyp kriszyp commented Oct 5, 2026 •

Copy link
Copy Markdown
Member

⊙ Problem

scripts/patch-release.js had two verification gaps, seen during the 5.2.0 release:

  • Vacuous backport pass. A main-line release (--branch main, default --source main) checked milestone PRs merged to main for presence in main's own history. Every PR is present by construction, so the script printed Backport verification passed (N PRs) having verified nothing. v5.3.0 and v5.3.1 were cut this way.
  • No CI check before tagging. Measured on 2026-10-05: the core Release v5.1.29 and Release v5.2.15 commits that were tagged have failing unit and integration runs, and the current core v5.3 head (ce9621c5) has no runs at all. The cherry-pick workflow advances release branches with GITHUB_TOKEN, and GitHub does not trigger push workflows for those pushes.

❓ Your call: Is the requirement (refuse to tag unless required CI is green on both release candidates) right as specified? I think yes. The cost is that every release line blocks today: core v5.1 and v5.2 are red, and the v5.3 heads and the newest harper-pro main commit have no runs. Each release now starts with gh workflow run … --ref <branch> (the script prints the command) and a wait. To back out, pass --ci-override <reason> for one release, or revert this PR.

💡 Solution

Same-ref detection. When the release branch is the source branch (backportVerificationApplies), the script skips the PR query and prints Backport verification not applicable: release cut from source branch main; no backport evidence was checked. It never prints the pass line in that case. RESULT.backportVerification reports passed, missing or not-applicable per repository, so a --json caller cannot read a vacuous pass either.

Release-candidate CI gate. The release candidate is the fetched origin/<branch> commit that the release commit is built on. It is pinned once per repository, and the backport check, version read, CI query and release-commit parent all use that SHA. For each required workflow (REQUIRED_WORKFLOWS: core unit-test.yml and integration-tests.yml; harper-pro unit-tests.yaml and integration-tests.yaml, the workflows that pushes to both main and vX.Y trigger), the script reads actions/workflows/<file>/runs?head_sha=<sha> and every attempt's jobs for each run (filter=all). In evaluateWorkflowRuns, each job's result comes from its most recent completion (a re-run keeps its run_number, so recency comes from the job's completed_at; an unfinished job counts as newest). pull_request runs are ignored because they test a merge ref. > ⚠️ Look hardest: evaluateWorkflowRuns is the definition of green.

These all block: a missing or unfinished run, a run that lists no jobs, a failed or cancelled job, and an API error.

  Required CI on 0e1da502 (origin/v5.2):
    ✗ unit-test.yml / Unit Test (Node.js v22): failure  https://github.com/HarperFast/harper/actions/runs/37084477973/job/111091801245
    ✗ integration-tests.yml / Integration Tests 1/6 (Bun): failure  …
  Required CI on ce9621c5 (origin/v5.3):
    ✗ unit-test.yml: missing
      No run on this commit. Start one, wait for it, then re-run the release:
      gh workflow run unit-test.yml --repo HarperFast/harper --ref v5.3
  • --yes aborts before any checkout, version bump, tag or push, including under --dry-run. It exits 1 and emits RESULT even without --json, with ciFailures (repo, branch, sha, workflow, job, state, url or error) next to missingPRs.
  • Interactive mode prints the failing checks. The existing proceed prompt then asks to confirm overriding the gate; a "y" there is recorded as ciOverride: { reason: 'interactive confirmation' }.
  • --ci-override "<reason>" is the emergency path in either mode. The reason is printed and recorded in RESULT.ciOverride. It does not waive missing backports, and a blank reason is rejected.

Tagged tree = verified tree. Both repositories are checked out at their pinned candidates, and assertReleaseBase runs on each before either gets a release commit. It refuses a HEAD other than the candidate, any staged change, and a modified package.json or package-lock.json. Harper-pro is checked out with -c submodule.recurse=false. With submodule.recurse set, that checkout moves core to harper-pro's recorded gitlink. On main it ran after core's release commit, so harper-pro could pin the old core. Under the first version of this PR's ordering, core's release commit landed on a detached HEAD instead. Review caught it.

Prompts on closed stdin. prompt() used one readline interface per prompt. Measured: an interface created after stdin ended never settled, so the process exited 0 without a RESULT, and an earlier interface swallowed piped answers meant for later prompts. One shared reader, paused between prompts, now returns null on closed stdin. The proceed and CM-deploy prompts treat null as "no"; branch restore keeps its default.

⚖️ Alternatives

❓ Your call: I adopted the design-review alternative over my plan. The planning review returned Framing-Verdict: better-alternative-exists against a separate Tag anyway? [y/N] prompt. Its do-less option reuses the existing proceed confirmation as the override acknowledgment: one prompt, one abort path. The trade-off: with stdin piped from yes, a run without --yes overrides the gate, and the override is recorded as interactive confirmation. Tightening this to a typed token would be a one-line prompt change.

❓ Your call: Harper-pro's CI tests harper-pro against its own core pointer, but the release re-pins core to the core candidate. When the two differ, the script warns, and RESULT.candidates.proCoreGitlink differs from candidates.core; it does not block. On v5.3 today they differ (f3fa8e37 vs ce9621c5). Blocking would require a Sync Core commit plus a CI round on the harper-pro release branch for every patch that changes core.

❓ Your call: The gate checks the release commit's parent. The tagged commit adds only the version bump, the core re-pin and synced dependencies, and it is first tested by its own push CI after the tag exists. Testing that commit before tagging would mean pushing a staging branch, waiting for CI, then tagging: a restructure of the release flow.

❓ Your call: Coverage is per job. A candidate whose only evidence is a narrow dispatch (e.g. -f node-version=24) passes for the jobs that ran, and each green line shows run events and job count (push #8168; 41 jobs). A narrow run cannot hide a failure from an earlier full run or attempt, and a run cancelled before any job started blocks until it is re-run. Requiring the full matrix would need an expected job-name list per workflow. The cherry-pick workflow does not leave narrow runs on release heads: it dispatches tests only before merge (is_merged != 'true') and re-picks fresh commits on merge.

  • Required set: unit and integration only. Those are the workflows that pushes to both main and vX.Y trigger. Lint, typecheck and format run on main pushes only, so requiring them would block every release-branch candidate. To widen the set, edit REQUIRED_WORKFLOWS.
  • A run that lists no jobs blocks, even with a success conclusion. No real run of these workflows succeeds without jobs, so this only fires on an API anomaly; --ci-override gets past it.
  • Skipped and neutral job conclusions count as passing. Workflows skip jobs by design (if:). Treating skipped as failing would block on those legitimate skips. The trade-off is that a test job skipped by a future if: would also read as green.
  • Rejected: the commit check-runs or combined-status API. It returns one row per matrix job plus every scheduled run on the SHA (31–43 workflow runs on recent main heads, mostly Companion Check), and it cannot filter out pull_request runs.
  • Rejected: deriving the required set from rulesets. rules/branches/v5.3 has no rules in either repository. main's required checks are PR merge gates (companion-check, validate / validate) that never run on a push commit.
  • Rejected: a server-side tag rule. The tagged commit is created locally and pushed together with its tag, so no CI can run on it before the push.
  • Complementary, not a replacement: have the cherry-pick workflow dispatch CI on the release branch after it fast-forwards it. That makes evidence exist; this gate is what consults it. Filed as a follow-up finding.
  • Rejected: refusing --branch equal to --source. Main-line releases are a real flow (v5.3.0, v5.3.1).

🔧 Changes

✅ Verification

Route: spawned-CLI tests against real git fixture repositories with a scripted gh shim, plus live dry-runs against GitHub. The release runs on a maintainer's machine, not inside the server, so integration tests do not exercise it.

  • npx mocha --require unitTests/unitTestSetup.cjs 'unitTests/scripts/*.test.mjs': 99 passing.

    • unitTests/scripts/patchReleasePresence.test.mjs, unit level: backportVerificationApplies, and evaluateWorkflowRuns over runs/jobs fixtures shaped like the REST rows (green, missing, pull_request-only, failed or cancelled jobs, unfinished runs, a job re-run that started after the runs were listed, flake superseded, newer failure, a re-run of an older run superseding or not superseding a newer one, an earlier attempt's unrepeated failure, run_number ordering, narrow-run masking, job-less runs including beside a green run, startup_failure, malformed run_number).
    • Same file, spawned CLI (the fixture's gh shim gains Actions routes and unsigned tags): --yes (no --json) aborts on a failed core or pro check with local and remote refs, tags and package.json unchanged, and the queried head_sha is the pinned candidate. Also covered: dry-run abort on an API error, the dispatch command for a missing run, --ci-override reason recorded, the override not waiving a missing backport, a blank reason rejected, interactive decline and closed-stdin abort carrying the gate fields, interactive "y" recorded with CM deploy declined on closed stdin, the tag landing on the pinned candidate when origin/v5.1 is moved during the CI query, a staged change refused before any core or pro tag exists, core's release commit built on its candidate under submodule.recurse=true, the pairing warning, and --branch main printing not-applicable with no PR query (decided per repository with --core-branch).
    • unitTests/scripts/patchRelease.test.mjs: the buildSuccessResult gate fields and a recorded override.
  • Fails on base. Against origin/main's script, 38 of the 99 tests fail. Three targeted mutations prove the mechanism tests. Reverting the merge to origin/<branch> makes the tag land on the unverified commit. Asserting harper-pro after core's bump leaves a core v5.1.1 tag behind. A recursive harper-pro checkout leaves core's release commit on a detached HEAD.

  • npm run build && npm run test:unit: 1601 passing, 2 pending.

  • Live dry-run (--dry-run --yes --json, from a fresh clone, against GitHub):

    line backports core CI harper-pro CI
    v5.3 passed missing ×2 missing ×2, pairing warning
    main not-applicable ×2 ✓ (push #8183, #8168) missing ×2 (47984013, merged 23 min earlier)
    v5.2 missing (30 PRs) 3 failed jobs ✓
  • filter=all vs filter=latest, measured on two real attempt-2 runs: latest lists only attempt 2's jobs (27/27 and 32/32 carried forward), while all returns both attempts, including attempt 1's failure.

  • A shared paused reader does not hold an interactive TTY open. On a pty with stdin held open for 20 s, node exited 0.17 s after start.

  • prettier --check and oxlint --deny-warnings are clean on the changed files. The review CLI's no-restart-test self-check does not apply: this script keeps no crash-recovery state, and a run killed partway leaves the same state as on main.

🤖 Generated by Anthropic Claude (Opus 5.5) via Claude Code dev-agent; posted via @kriszyp.

Related PRs: #970 overlaps (semver preflight in the same script, CONTRIBUTING.md and contract tests; independent change, at most adjacent-hunk conflicts)
Complexity: medium

Review-Coverage: authored=claude; ran=cursor-composer,gemini,codex; adjudicated=domain; blocked=claude(fallback)(out-of-budget),cursor-grok(failed); declined=cursor-kimi,cursor-muse; rounds=6; full=2 @ 9d051e9

Review-Attention: read ~6m (decisions: do-less-alternative, recency-by-completion, every-empty-run-blocks, observed-job-set-is-green, gate-parent-not-release-commit, gitlink-mismatch-warns, interactive-override-reasonless) @ 9d051e9

kriszyp and others added 6 commits October 5, 2026 09:14
… vacuous backport pass

patch-release.js now refuses to tag unless the required unit and
integration workflows passed on both release candidates (harper-pro and
core), evaluated per job so a narrower re-run cannot mask an earlier
failure. --yes aborts with a RESULT line; interactive mode prints the
failing checks and makes the proceed prompt an explicit override;
--ci-override <reason> covers an emergency release and is recorded.

A release cut from its source branch (a main-line release) now reports
backport verification as not applicable instead of passing having
checked nothing. Each candidate SHA is pinned once after fetch and the
release commit is built on it, and prompts settle as "no" on closed
stdin instead of exiting silently.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FhCspWz6s3qTVR7EsMGqeG
Dispatch-Task: harper-pro-patch-release-verify-gates
…se zero-job CI gaps

Check out, fast-forward and assert both repositories before core's
version bump, so a harper-pro refusal no longer leaves a local core
release commit and tag that block the re-run. Block when any run of a
required workflow is unfinished or a successful run lists no jobs, and
carry the gate fields in the aborted RESULT. Document that closed stdin
keeps the branch-restore default.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FhCspWz6s3qTVR7EsMGqeG
Dispatch-Task: harper-pro-patch-release-verify-gates
… results by completion

With submodule.recurse set, checking out and fast-forwarding the
harper-pro release branch moved core to harper-pro's recorded gitlink
after core's base was asserted, so core's release commit landed on a
detached HEAD. Run both without recursion and assert core afterwards.
A re-run keeps its run_number, so the newest result per job now comes
from the job's completed_at.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FhCspWz6s3qTVR7EsMGqeG
Dispatch-Task: harper-pro-patch-release-verify-gates
… job

A job re-run that starts between the runs and jobs queries has no
completed_at, so it now outranks every finished result instead of
losing to an older green one. Drop the highest-run_number conclusion
check, which blocked after an older run's re-run had passed, and
instead block any run that ended before a job started until it is
re-run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FhCspWz6s3qTVR7EsMGqeG
Dispatch-Task: harper-pro-patch-release-verify-gates
…workflow

Fetch jobs with filter=all so a failure from an earlier attempt that a
partial re-run did not repeat stays visible to the per-job recency rule,
and block a run that lists no jobs whatever its conclusion.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FhCspWz6s3qTVR7EsMGqeG
Dispatch-Task: harper-pro-patch-release-verify-gates
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FhCspWz6s3qTVR7EsMGqeG
Dispatch-Task: harper-pro-patch-release-verify-gates
@kriszyp kriszyp added this to the v5.3 milestone Oct 5, 2026
@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Release cherry-pick v5.3: cancelled

Cherry-pick branch cherry-pick/v5.3/pr-972 was deleted — this PR no longer targets v5.3 (milestone is now v5.4).

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request introduces a release-candidate CI gate to the patch-release.js script, ensuring that required CI workflows have passed on the pinned candidate commits before a release is published. It also adds a --ci-override flag for emergency releases, handles cases where backport verification is not applicable, and updates the documentation and test suites. The review feedback highlights a critical cross-platform compatibility issue on Windows where splitting command outputs (from gh api and git diff) solely by \n leaves trailing carriage returns (\r), which can cause JSON parsing failures or malformed filenames. It is recommended to use a regular expression like /\r?\n/ to safely split these lines.

Comment thread scripts/patch-release.js
Comment thread scripts/patch-release.js

This branch has not been deployed

No deployments
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