-
Notifications
You must be signed in to change notification settings - Fork 3
YPE-2486: harden and document the release procedure #268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4f4d343
98a3a86
6ab6118
5f56a0d
9e3c408
2c76143
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| name: Commitlint | ||
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, edited, synchronize, reopened] | ||
|
|
||
| jobs: | ||
| commitlint: | ||
| name: Lint Commit Messages | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v5 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Setup pnpm | ||
| uses: pnpm/action-setup@v4 | ||
| with: | ||
| version: 9.0.0 | ||
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: 20 | ||
| cache: 'pnpm' | ||
|
|
||
| - name: Install dependencies | ||
| run: pnpm install --frozen-lockfile | ||
|
|
||
| # Lint only the commits this branch adds, using the merge-base with the | ||
| # live origin/main as the lower bound (not the PR's recorded base SHA, | ||
| # which goes stale as main moves forward). The merge-base is robust even | ||
| # when the branch is behind main: it always resolves to the point where | ||
| # this branch diverged, so the range is exactly the new commits. | ||
| # | ||
| # main MUST be fetched with full history (no --depth). A shallow fetch | ||
| # hides the merge-base, so `git log <main>..HEAD` can no longer exclude | ||
| # shared history and the range balloons to include unrelated historical | ||
| # commits. See docs/release-hardening-plan.md (AC6). | ||
| - name: Resolve lint range | ||
| id: range | ||
| env: | ||
| HEAD_SHA: ${{ github.event.pull_request.head.sha }} | ||
| run: | | ||
| set -euo pipefail | ||
| git fetch --no-tags origin main:refs/remotes/origin/main | ||
| base=$(git merge-base origin/main "$HEAD_SHA") | ||
| echo "base=${base}" >> "$GITHUB_OUTPUT" | ||
| echo "Linting ${base}..${HEAD_SHA}" | ||
|
|
||
| - name: Lint commits | ||
| run: pnpm exec commitlint --from ${{ steps.range.outputs.base }} --to ${{ github.event.pull_request.head.sha }} --verbose | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| pnpm exec commitlint --edit "$1" | ||
|
greptile-apps[bot] marked this conversation as resolved.
|
||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| // Commit message linting for the React SDK. | ||
| // | ||
| // Ported from platform-sdk-kotlin's working setup, with two React-specific | ||
| // adaptations: | ||
| // | ||
| // 1. The release commit. Kotlin uses semantic-release, whose bot emits | ||
| // "chore(release): ...". This repo uses Changesets, whose release commit | ||
| // subject is "chore: version packages" (see .github/workflows/release.yml). | ||
| // We ignore that subject so the bot's own commit never fails the lint | ||
| // range on a workflow re-run. | ||
| // | ||
| // 2. The ticket prefix. About half of this repo's history leads with a | ||
| // YouVersion ticket id, e.g. "YPE-1234: feat(ui): ..." or | ||
| // "YPE-1234 - feat(ui): ...". We allow an optional leading ticket | ||
| // reference before the conventional type/scope/subject so the team's | ||
| // existing convention keeps passing. | ||
| // | ||
| // This repo is ESM ("type": "module"), so the config uses `export default` | ||
| // rather than Kotlin's `module.exports`. | ||
| export default { | ||
| extends: ['@commitlint/config-conventional'], | ||
| ignores: [(message) => /^chore: version packages(?:\r?\n|$)/i.test(message)], | ||
| parserPreset: { | ||
| parserOpts: { | ||
| // Optional "YPE-1234: " or "YPE-1234 - " prefix, then the standard | ||
| // conventional header. Capture groups after the optional prefix must stay | ||
| // aligned with headerCorrespondence below. | ||
| headerPattern: /^(?:YPE-\d+(?:: | - ))?(\w+)(?:\(([^)]*)\))?(!)?: (.+)$/, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new parser only preserves the ticket-prefix convention when the text after Prompt To Fix With AIThis is a comment left during a code review.
Path: commitlint.config.js
Line: 28
Comment:
**Rejects common subjects**
The new parser only preserves the ticket-prefix convention when the text after `YPE-####` is already a conventional, lower-case subject. Recent merge subjects in this repo include forms like `YPE-1146 - Prep React SDK for major bump`, `YPE-2923: feat(ui): Move ...`, and plain titles such as `Add Language detection ...`; those now fail with `type-empty`, `subject-empty`, or `subject-case`. If this check is required on PRs, branches that follow those still-common repo title styles can be blocked even though the docs say the team's existing convention keeps passing.
How can I resolve this? If you propose a fix, please make it concise.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Public contributions will not start with a ticket number, but the PR title may get updated with one after we discuss and accept the change. Let's add this to the plan feedback and "turn the crank again". |
||
| headerCorrespondence: ['type', 'scope', 'breaking', 'subject'], | ||
| }, | ||
| }, | ||
| rules: { | ||
| 'body-max-line-length': [0, 'always'], | ||
| 'footer-max-line-length': [0, 'always'], | ||
| }, | ||
| }; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jhampton @davidfedor , three things need a decision before this lands; recommendations are in the doc. #1 (commit-lint) and #2 (package manager) need David's input. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| # Release Hardening — Open Decisions (YPE-2486) | ||
|
|
||
| **Status:** 🟡 Blocked. [PR #268](https://github.com/youversion/platform-sdk-react/pull/268) does not land until the decisions below are made. Some need @davidfedor. | ||
|
|
||
| Review feedback (jhampton, `CHANGES_REQUESTED`, 2026-06-29) raised three items that are **decisions, not bugs**. Decide each deliberately, then pin it two ways: | ||
|
|
||
| - **Deterministic** — code/config (`commitlint.config.js`, workflows, `package.json`, hooks). | ||
| - **Semi-deterministic** — [`.greptile/rules.md`](https://www.greptile.com/docs/code-review/custom-standards), `AGENTS.md`. | ||
|
|
||
| Update each decision's status as it lands. | ||
|
|
||
| --- | ||
|
|
||
| ## Decision 1 — Commit-lint subject model 🔴 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the Swift repo, we enforce conventional commits to ensure the Changelog generation is complete and clean, and that our versioning math reads for the most-breaking change (i.e. patch -> minor -> major). We work around this issue entirely because Jira actually parses branch names, not just commits.
https://lifechurch.atlassian.net/browse/YPE-3340?search_id=8355af1d-6356-4d26-8e72-8190a66d6052
The correct answer is to enforce conventional commits. Ticket references can be front-and-center in our PRs, but when we squash-merge, the commit title matters as the changelog is calculated IIUC. Please check my math if that doesn't align to the implementation. |
||
|
|
||
| **Problem.** [`commitlint.config.js`](../commitlint.config.js) allows a leading `YPE-####` prefix, then enforces strict conventional. Wrong assumption: external contributors don't open PRs with a ticket number (it's added to the PR title later), and much of the repo's own history fails strict conventional anyway (e.g. `YPE-1146 - Prep React SDK...`, `Add Language detection...`). | ||
|
|
||
| **Options.** | ||
| 1. **Lint the PR title, not commits** (squash-merge makes the title the commit). Ticket can be added to the title post-acceptance; contributors aren't blocked mid-branch. | ||
| 2. **Keep per-commit linting, relaxed** — drop the prefix parser, loosen `subject-case`. | ||
| 3. **Strict per-commit as-is** — most consistent, most friction, contradicts "ticket added later." | ||
|
|
||
| **Recommend:** Option 1. **Decides:** @davidfedor. | ||
| **Pin:** PR-title lint job in `commitlint.yml`; drop/trim the parser in `commitlint.config.js`; note the convention in `.greptile/rules.md` + `AGENTS.md`. | ||
|
|
||
| --- | ||
|
|
||
| ## Decision 2 — Package-manager lane 🔴 | ||
|
|
||
| **Problem.** The [`commit-msg`](../.husky/commit-msg) hook runs bare `pnpm exec` — uses whatever pnpm is on PATH, not the pinned `pnpm@9.0.0`. A newer global pnpm fails commits. The repo also never states/enforces that it's a pnpm workspace. | ||
|
|
||
| **Options.** | ||
| 1. **pnpm + Corepack** — `corepack pnpm exec` in hooks (activates the pinned version); README rationale; optional CI guard. | ||
| 2. **pnpm, no Corepack** — document it, rely on `engines`/`packageManager`; mismatch can still bite locally. | ||
| 3. **Pure node** — not viable; the repo uses `workspace:*` (pnpm-only). | ||
|
|
||
| **Recommend:** Option 1. **Decides:** @davidfedor (approve README rationale). | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Option 1 - great explanation, no notes. That said, I saw that |
||
| **Pin:** `corepack pnpm exec` in hooks + optional CI check; README "Package Manager" section; `.greptile/rules.md` flag for bare `pnpm exec` in hooks. | ||
|
|
||
| --- | ||
|
|
||
| ## Decision 3 — Node-version policy 🟠 | ||
|
|
||
| **Problem.** Versions are inconsistent: CI/commitlint on Node 20, release on Node 24, `engines.node >=20`. The commitlint break is already fixed (pinned to v19, Node ≥18, commit `98a3a86`) but there's no stated policy. | ||
|
|
||
| **Options.** | ||
| 1. **Standardize on Node 20** across CI/hooks; new dev-deps must support it. | ||
| 2. **Raise floor to Node 22** — bigger blast radius (consumers, contributors). | ||
| 3. **Per-workflow, documented** — allow release to differ, write down why. | ||
|
|
||
| **Recommend:** Option 1. **Decides:** team. | ||
| **Pin:** align `node-version` across `ci.yml`/`commitlint.yml`/`release.yml`; `AGENTS.md` note "new dev-deps must support `engines.node`." | ||
|
|
||
| --- | ||
|
|
||
| ## Already fixed (no decision) | ||
|
|
||
| | Item | Fix | | ||
| | --- | --- | | ||
| | commitlint `@21` needs Node ≥22.12 | Pinned to `19.8.1` — `98a3a86` | | ||
| | Runbook used bare `$VERSION` tag; repo tags per-package | §1/§9 loop over `<pkg>@$VERSION` — `5f56a0d` | | ||
| | Release-ignore regex too broad | Anchored to `(?:\r?\n\|$)` — `5f56a0d` | | ||
| | Commit-lint range too wide (shallow fetch) | `git merge-base origin/main HEAD` + full fetch — `6ab6118` | | ||
|
|
||
| ## Next | ||
|
|
||
| 1. Decide 1–3 (consult @davidfedor). | ||
| 2. Implement deterministic guardrails. | ||
| 3. Add `.greptile/rules.md` + `AGENTS.md` rules. | ||
| 4. Update [`release-hardening-plan.md`](./release-hardening-plan.md), then unblock PR #268. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| # Release Hardening Plan (YPE-2486) | ||
|
|
||
| ## Why this doc exists | ||
|
|
||
| YPE-2486 was opened to port [`platform-sdk-swift`'s YPE-2684 release hardening](https://github.com/youversion/platform-sdk-swift) to the React SDK. The Swift hardening was built around a custom bash orchestrator (`scripts/release.sh`) with explicit version inputs, tag-state resume mode, and bounded retry on `pod trunk push`. | ||
|
|
||
| The React SDK does not use that shape — it uses [Changesets](https://github.com/changesets/changesets) with `changesets/action@v1`, triggered on push to `main`. Most of YPE-2486's "Constant across SDKs" scope assumes the bash-orchestrator model and is structurally inapplicable here; the one registry-agnostic item that *does* apply — commit-lint (AC6) — was ported from the Kotlin SDK. This document records what the team decided to do instead. | ||
|
|
||
| The sibling RN-Expo SDK ([`platform-sdk-reactnative-expo`](https://github.com/youversion/platform-sdk-reactnative-expo)) made the same call — see its [`docs/release-hardening-plan.md`](https://github.com/youversion/platform-sdk-reactnative-expo/blob/main/docs/release-hardening-plan.md) for the parallel pivot. | ||
|
|
||
| ## Decision: keep Changesets, harden around the edges | ||
|
|
||
| Three options were on the table: | ||
|
|
||
| 1. **Adopt the Swift model.** Replace Changesets with a `workflow_dispatch`-triggered bash orchestrator that takes a `version` input and implements resume mode, bounded retry, and per-package idempotency. Faithful to the Swift hardening, but rips out a working release model and introduces ~1000 LoC of bash to maintain. | ||
| 2. **Keep Changesets, layer Swift-style hardening on top.** Add `npm view` pre-publish checks, `EPUBLISHCONFLICT` handling, and retry logic inside the workflow alongside `changesets/action`. Higher complexity than option 3 with marginal upside — Changesets already handles most of these cases. | ||
| 3. **Keep Changesets, document the failure surface, and accept what's already idempotent.** Land the failure-mode runbook and engineering plan called for by YPE-2486 ACs 4 / 5 / 7, port the registry-agnostic items that *do* apply (commit-lint, AC6), document the breaking-change gate, drop the AC items that conflict with Changesets, and update the ticket scope to match. | ||
|
|
||
| **Option 3 was chosen.** Changesets-action already: | ||
|
|
||
| - Re-reads the registry per package before publishing, so a retry skips packages that landed. | ||
| - Treats publishing as atomic per package (one tarball uploads or none does — no partial-package state). | ||
| - Re-runs cleanly on workflow retry: if the "Version Packages" PR didn't merge yet, the next run regenerates it; if the publish failed mid-way, the next run picks up the missing packages. | ||
|
|
||
| The hardening Swift needed (tag-resume, bounded retry, per-package idempotency) is mostly built in for free here. What's actually missing is **documentation** — operators need to know how to map an npm error message to a recovery, what failure classes are transient vs hard, and what an Automation token vs Publish token means for CI. | ||
|
|
||
| ## What landed under YPE-2486 | ||
|
|
||
| | AC | Deliverable | Status | | ||
| | --- | --- | --- | | ||
| | 4 | `RELEASE-RUNBOOK.md` with 10 failure modes: EPUBLISHCONFLICT-after-success, transient 5xx, provenance attestation failure, expired NPM_TOKEN, OTP/2FA, workspace:* rewrite, peer-dep skew, dist-tag drift, rogue tag, wrong version input | ✅ Landed (all AC4-named modes covered: registry transient, EPUBLISHCONFLICT-after-success, provenance, NPM_TOKEN, OTP/2FA, rogue tag, wrong VERSION input) | | ||
| | 5 | Operator guide: `PUBLISHING.md` retained, cross-links runbook | ✅ Landed (cross-link added) | | ||
| | 6 | Commit-lint workflow anchored at `origin/main` tip, ignores the release commit | ✅ Landed (ported from the Kotlin SDK) | | ||
| | 7 | This engineering plan | ✅ This document | | ||
| | BREAKING CHANGE requires manual approval | The "Version Packages" PR merge is the approval gate | ✅ Documented below | | ||
|
|
||
| ## What was dropped from scope (and why) | ||
|
|
||
| | Original AC item | Why dropped | | ||
| | --- | --- | | ||
| | 1. Manual `workflow_dispatch` with explicit version input | Conflicts with Changesets' auto-on-merge model. Versions come from `.changeset/*.md` files, not workflow inputs. Operator override is done by hand-editing the "Version Packages" PR before merging. | | ||
| | 2. Resume-on-re-dispatch in a custom script | `changesets/action` is idempotent by design — re-running it from the Actions tab is the recovery path. No custom resume logic needed. | | ||
| | 3. Custom `npm view` idempotency check + EPUBLISHCONFLICT handling | Changesets-action does per-package registry checks before publishing. EPUBLISHCONFLICT-after-success is documented in the runbook so operators recognize the symptom; no in-band classifier needed. | | ||
| | Bounded retry on registry-publish | Not supported by `changesets/action` upstream. Workflow re-trigger from Actions UI fills the gap. | | ||
| | Dist-tag preservation in resume mode | Changesets handles dist-tag via `.changeset/pre.json`; #8 in the runbook covers the manual recovery if it drifts. | | ||
|
|
||
| These items aren't impossible to add; they just don't pull their weight against the Changesets baseline. If the release model ever pivots (e.g. to manual-dispatch for slow-rolling enterprise channels), they should be re-evaluated. | ||
|
|
||
| ## Commit linting (AC6) | ||
|
|
||
| Ported from the Kotlin SDK's working setup ([`commitlint.yml`](https://github.com/youversion/platform-sdk-kotlin/blob/main/.github/workflows/commitlint.yml) + [`commitlint.config.js`](https://github.com/youversion/platform-sdk-kotlin/blob/main/commitlint.config.js)), which is the reference this AC was modeled on: | ||
|
|
||
| - **CI** — [`.github/workflows/commitlint.yml`](../.github/workflows/commitlint.yml) runs on PRs and anchors the lint range at the **merge-base with the live `origin/main`** (`git fetch origin main` full history → `base=$(git merge-base origin/main <head.sha>)` → `commitlint --from <base> --to <head.sha>`), not the PR's recorded base SHA. The recorded base goes stale as `main` advances and would re-lint already-merged commits; the merge-base lints only commits new to the branch, and stays correct even when the branch is behind main. Note: `main` must be fetched with full history — a shallow (`--depth=1`) fetch hides the merge-base and balloons the range to include unrelated historical commits. | ||
| - **Local** — a husky `commit-msg` hook ([`.husky/commit-msg`](../.husky/commit-msg)) runs `commitlint --edit` so authors catch a bad message at commit time, before pushing. | ||
| - **Config** — [`commitlint.config.js`](../commitlint.config.js) extends `@commitlint/config-conventional`. | ||
|
|
||
| Two React-specific deltas from the Kotlin original: | ||
|
|
||
| - **Release-commit ignore.** Kotlin (semantic-release) ignores `chore(release):`. This repo (Changesets) emits `chore: version packages` — the config ignores that subject instead, so the release bot's own commit never fails the lint range on a workflow re-run. | ||
| - **Ticket prefix allowed.** ~Half this repo's history leads with a ticket id (`YPE-1234: feat(ui): …` / `YPE-1234 - feat(ui): …`). A custom `headerPattern` permits an optional leading ticket reference before the conventional `type(scope): subject`, so the team's existing convention keeps passing. Note: `config-conventional`'s `subject-case` rule still applies, so capitalized subjects (`feat(ui): Move …`) are flagged — a minor habit change, consistent with the Kotlin reference. | ||
|
|
||
| ## BREAKING CHANGE approval | ||
|
|
||
| The ticket requires that a breaking change can't ship without manual approval. In the Changesets model this gate already exists and is structural — there is no separate workflow to add: | ||
|
|
||
| - A breaking change is declared as a `major` bump in its `.changeset/*.md` entry. | ||
| - That bump flows into the **"Version Packages" PR**, where the new `major` version is visible in the `package.json` diffs. | ||
| - Nothing publishes until a human **merges that PR**. The merge is the manual approval — a reviewer can block a `major` bump there, or send it back to correct the bump level before anything reaches npm. | ||
|
|
||
| So the approval point for breaking changes is the same review-and-merge step every release passes through; no auto-publish path bypasses it (the workflow only publishes *after* the version PR merges to `main`). Recovery for a wrong bump that slips through is in [`RELEASE-RUNBOOK.md` §10](../RELEASE-RUNBOOK.md#10-wrong-version-input-bad-bump-in-the-version-packages-pr). | ||
|
|
||
| ## What's *not* in this PR but worth knowing | ||
|
|
||
| - **Trusted Publishing** is configured for all three packages (`@youversion/platform-core`, `…-react-hooks`, `…-react-ui`) — see [`PUBLISHING.md`](../PUBLISHING.md). `NPM_TOKEN` is kept as a fallback only. | ||
| - **Provenance** is on by default (`publishConfig.provenance: true` in each package + `NPM_CONFIG_PROVENANCE: true` in [`release.yml`](../.github/workflows/release.yml)). | ||
| - **Three-package fixed group** in [`.changeset/config.json`](../.changeset/config.json) ensures every release ships all three packages at the same version. | ||
| - **The RN SDK matches this same shape** so contributors and maintainers don't have to relearn the release flow per repo. | ||
|
|
||
| ## Related work / cross-references | ||
|
|
||
| - [`platform-sdk-reactnative-expo`'s YPE-2790](https://github.com/youversion/platform-sdk-reactnative-expo) — parallel pivot on the RN repo, same Changesets model, slim runbook with RN-specific failure modes. | ||
| - [`platform-sdk-swift`'s YPE-2684](https://github.com/youversion/platform-sdk-swift) — original Swift hardening; the React SDK does not adopt its orchestrator shape but borrows its failure-class taxonomy. | ||
| - [`changesets/action`](https://github.com/changesets/action) — upstream action behavior. | ||
| - [npm Trusted Publishing](https://docs.npmjs.com/trusted-publishers) — the OIDC auth path used here. |


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This workflow installs
@commitlint/cli@21.1.0, whose locked package metadata requires Node>=22.12.0, but the job runs on Node 20. When a pull request runs this new check, dependency install orpnpm exec commitlintcan fail before any commit message is linted, blocking every PR. The release workflow already uses Node 24, so this check should use a compatible Node version too.Prompt To Fix With AI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same treatments here around node versions.