Beta release workflow #3131
Beta release workflow #3131
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
.github/workflows/release.yml (5)
354-354: Same issue:always()with success conditions at lines 354, 487, 506, 546.Lines 354, 487, 506, and 546 repeat the same contradictory pattern. Apply the same fix across all conditional expressions.
Also applies to: 487-487, 506-506, 546-546
362-362: Use validatedrelease_branchoutput instead of raw input.Line 362 still uses
github.event.inputs.release_branch_namedirectly. For consistency and to use the validated, canonical release branch value, switch to the output fromget_version:- RELEASE_BRANCH: "${{ github.event.inputs.release_branch_name }}" + RELEASE_BRANCH: "${{ needs.get_version.outputs.release_branch }}"This ensures all downstream jobs reference the same validated value (as flagged in prior reviews).
671-681: Fixgit_refto use released tag or branch, not current workflow branch.Line 681 uses
git_ref: ${{ github.ref_name }}, which for aworkflow_dispatchevent on the main branch will resolve tomain, not the released code. Documentation should be generated from the actual released commit/tag.The
releasejob outputs the tag name (line 638:tag_name). Use that instead:generate-documentation: name: Generate and Publish documentation for ${{ needs.get_version.outputs.package_version }} if: ${{ needs.get_version.outputs.is_beta != 'true' }} uses: ./.github/workflows/generate-documentation.yml needs: [get_version, release] permissions: contents: read pages: write id-token: write with: - git_ref: ${{ github.ref_name }} + git_ref: ${{ needs.release.outputs.tag_name }}This ensures documentation is generated from the correct released tag. (Also noted in prior review feedback that was not yet addressed.)
75-96: Beta/stable branching logic is clear, but add validation for beta releases to requiremainbranch.The
-experimentalsuffix appending logic andIS_BETAdetection are correct. However, for consistency and to prevent accidental beta releases from non-main branches, add a check that beta releases (whenIS_BETA == "true") must come from themainbranch, as stated in the PR objectives.if [ "$IS_BETA" != "true" ] && [[ ! "${RELEASE_BRANCH,,}" =~ ^release[-/] ]]; then echo "❌ Release branch '$RELEASE_BRANCH' must start with 'release-' or 'release/' for stable releases." >&2 exit 1 fi + + if [ "$IS_BETA" = "true" ] && [[ "${RELEASE_BRANCH,,}" != "main" ]]; then + echo "❌ Beta releases must be triggered from the 'main' branch. Found: '$RELEASE_BRANCH'" >&2 + exit 1 + fi
186-186: Resolve contradictory conditional logic across release pipeline: removealways()from jobs that gate on success.The conditional at line 186 (and similarly at lines 354, 487, 506, 546) combines
always()withneeds.<job>.result == 'success', which is contradictory.always()allows the job to run regardless of prior failures, but the subsequent checks require success—these are mutually exclusive intents.Affected jobs:
- Line 186:
pre_release(requiresget_versionsuccess)- Line 354:
ask_for_dev_team_review(requirespre_releasesuccess)- Line 487:
first_review(requiresask_for_dev_team_reviewsuccess)- Line 506:
ask_for_sec_team_review(requiresfirst_reviewsuccess)- Line 546:
release(requiresask_for_sec_team_reviewsuccess)Fix: Remove
always()from all five conditionals since the pipeline gates on prior job success at each stage:- if: ${{ always() && needs.get_version.result == 'success' && ... }} + if: ${{ needs.get_version.result == 'success' && ... }}If jobs should run regardless and notify on failure, keep
always()and remove the success checks instead—but then add explicit failure handling.
🧹 Nitpick comments (3)
.github/workflows/release.yml (3)
103-106: Remove redundantNPM_DIST_TAGwrite toGITHUB_ENV.Lines 103–106 write
NPM_DIST_TAGandRELEASE_BRANCHtoGITHUB_ENV, butNPM_DIST_TAGis already set as an environment variable at line 48 (from the input), and it was validated/normalized in this same step (lines 75–96). The write toGITHUB_ENVis redundant forNPM_DIST_TAGbut is necessary forRELEASE_BRANCH(since it's an input, not an env var at step start).Consolidate by writing only
RELEASE_BRANCHtoGITHUB_ENV:- { - echo "NPM_DIST_TAG=$NPM_DIST_TAG" - echo "RELEASE_BRANCH=$RELEASE_BRANCH" - } >> "$GITHUB_ENV" + + echo "RELEASE_BRANCH=$RELEASE_BRANCH" >> "$GITHUB_ENV"
683-735: Optional: pin shell for consistency and to ensurepipefailportability.The
notify_failuresjob usesset -euo pipefailbut does not explicitly pin the shell. While ubuntu-latest defaults to bash (where pipefail works), pinning the shell explicitly improves consistency and portability:notify_failures: runs-on: ubuntu-latest + shell: bash needs: [...]Minor point, but matches the pattern in other jobs like
pre_release(line 291).
264-278: Consider extending artifact retention for security artifacts.Lines 264–278 upload SBOM and vulnerability-report artifacts without specifying
retention-days. For security artifact retention and retroactive analysis, consider extending the default (5 days) to 30 days:- - name: Upload sbom to OWASP + - name: Upload sbom artifact uses: actions/upload-artifact@v4 with: name: sbom path: sbom.json + retention-days: 30 - name: Upload vulnerability report artifact uses: actions/upload-artifact@v4 with: name: vulnerability-report path: vuln-report.txt + retention-days: 30(Also noted in prior feedback.)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(19 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-09T20:16:27.834Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3101
File: .github/workflows/nodejs.yml:220-221
Timestamp: 2025-10-09T20:16:27.834Z
Learning: In `.github/workflows/nodejs.yml`, the `generate-documentation` job intentionally uses `if: startsWith(github.ref, 'refs/tags/xrpl@')` instead of `env.GIT_REF` to ensure documentation is only generated and deployed on actual tag push events, not when the workflow is called via `workflow_call` with a tag reference.
Applied to files:
.github/workflows/release.yml
📚 Learning: 2025-04-16T15:28:21.204Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2801
File: packages/xrpl/test/wallet/batchSigner.test.ts:0-0
Timestamp: 2025-04-16T15:28:21.204Z
Learning: In the XRPL.js library, hardcoded seeds in test files are acceptable as they don't represent protected data or real funds - they're only used for consistent test behavior.
Applied to files:
.github/workflows/release.yml
📚 Learning: 2024-09-26T21:14:56.813Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2788
File: .github/workflows/nodejs.yml:25-25
Timestamp: 2024-09-26T21:14:56.813Z
Learning: In `.github/workflows/nodejs.yml`, the `build-and-lint` job's actions `actions/setup-node` and `actions/cache` are updated to `v4`.
Applied to files:
.github/workflows/release.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: integration (24.x)
- GitHub Check: browser (24.x)
- GitHub Check: unit (24.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: build-and-lint (24.x)
- GitHub Check: unit (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: browser (24.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (24.x)
- GitHub Check: unit (20.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (24.x)
- GitHub Check: unit (22.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (5)
.github/workflows/release.yml (5)
12-40: ✓ Input validation and checkout logic look good.The new
release_branch_nameinput is properly defined and used for the initial checkout, providing the foundation for downstream validation.
79-79: Use[:blank:]instead of:space:to handle tabs.Line 79 uses
tr -d '[:space:]'which is correct (it includes space, tab, newline, etc.). However, if the intent is only to remove spaces and tabs (not newlines), use[:blank:]for clarity. If newlines should also be removed, the current approach is correct but a comment would help.
140-167: ✓ Version extraction and validation logic is sound.The step correctly uses
IS_BETAfrom the priorvalidate_inputsstep and validates SemVer format only for stable releases. Thedist_tagoutput properly captures the normalized distribution tag.
168-182: ✓ Test jobs correctly wirerelease_branchoutput.Both
run_faucet_testandrun_testsjobs correctly usegit_ref: ${{ needs.get_version.outputs.release_branch }}to checkout from the validated release branch.
541-669: ✓ Release job structure and tag/prerelease logic are sound.The job correctly:
- Checks out the release branch
- Publishes with the appropriate distribution tag
- Conditions
prereleaseandmake_latestonis_beta- Builds the release URL with proper URL encoding
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/release.yml (2)
362-362: Use validatedrelease_branchoutput instead of input.Line 362 references
github.event.inputs.release_branch_namedirectly, butneeds.get_version.outputs.release_branchis the validated and canonical value. For consistency with lines 173, 181, and 199 (which all useneeds.get_version.outputs.release_branch), update this reference.Past review comments flagged this, but it appears the fix was not applied.
Apply this diff:
env: PKG_VERSION: "${{ needs.get_version.outputs.package_version }}" PKG_NAME: "${{ github.event.inputs.package_name }}" - RELEASE_BRANCH: "${{ github.event.inputs.release_branch_name }}" + RELEASE_BRANCH: "${{ needs.get_version.outputs.release_branch }}"
684-684:generate-documentationuses incorrectgit_ref.Line 683 uses
git_ref: ${{ github.ref_name }}, which for aworkflow_dispatchevent resolves to the current branch (typicallymain), not the release branch. Documentation must be generated from the released code.The
releasejob does not exporttag_nameas a job output, so the first suggested fix in the original comment will not work. Use the validated release branch instead:generate-documentation: name: Generate and Publish documentation for ${{ needs.get_version.outputs.package_version }} if: ${{ needs.get_version.outputs.is_beta != 'true' }} uses: ./.github/workflows/generate-documentation.yml needs: [get_version, release] permissions: contents: read pages: write id-token: write with: - git_ref: ${{ github.ref_name }} + git_ref: ${{ needs.get_version.outputs.release_branch }}
🧹 Nitpick comments (3)
.github/workflows/release.yml (3)
719-719: Consider pinning shell for consistency with other jobs.While
set -euo pipefailworks on most Ubuntu runners (which default tobash), some jobs in this workflow explicitly pin the shell (e.g.,shell: bash). For consistency and robustness, consider pinning the shell here as well.A past reviewer flagged that
pipefailmay not work on all shells (dash/sh), so explicit pinning reduces ambiguity.Apply this diff:
steps: - name: Notify Slack about workflow failure + shell: bash env: SLACK_TOKEN: ${{ secrets.SLACK_TOKEN }}
186-186: Remove redundantalways()from job conditional.The
always()function is negated by the subsequent conditions. The logic already checksneeds.get_version.result == 'success', which ensures this job only runs ifget_versionsucceeded. Removingalways()clarifies intent.A past reviewer (ckeshava) flagged this same redundancy.
Apply this diff:
- if: ${{ always() && needs.get_version.result == 'success' && (needs.run_faucet_test.result == 'success' || needs.get_version.outputs.is_beta == 'true') && (needs.run_tests.result == 'success' || needs.get_version.outputs.is_beta == 'true') }} + if: ${{ needs.get_version.result == 'success' && (needs.run_faucet_test.result == 'success' || needs.get_version.outputs.is_beta == 'true') && (needs.run_tests.result == 'success' || needs.get_version.outputs.is_beta == 'true') }}Alternatively, apply the same fix to lines 354, 489, 508, and 548 for consistency.
623-638: Tag creation uses-f(force) despite pre-existence check.Lines 631–634 check if the tag already exists and fail if it does. However, lines 637–638 use the
-fflag to force-overwrite the tag and push. The force flag is redundant given the earlier check and could mask issues if the tag somehow exists despite the check.Consider removing the
-fflags for stricter tag management:echo "🔖 Tagging ${TAG}" - git tag -f "${TAG}" - git push origin -f "${TAG}" + git tag "${TAG}" + git push origin "${TAG}"This ensures the tag is created cleanly without force-overwriting, aligning with the pre-existence check.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(18 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-09T20:16:27.834Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3101
File: .github/workflows/nodejs.yml:220-221
Timestamp: 2025-10-09T20:16:27.834Z
Learning: In `.github/workflows/nodejs.yml`, the `generate-documentation` job intentionally uses `if: startsWith(github.ref, 'refs/tags/xrpl@')` instead of `env.GIT_REF` to ensure documentation is only generated and deployed on actual tag push events, not when the workflow is called via `workflow_call` with a tag reference.
Applied to files:
.github/workflows/release.yml
📚 Learning: 2025-04-16T15:28:21.204Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2801
File: packages/xrpl/test/wallet/batchSigner.test.ts:0-0
Timestamp: 2025-04-16T15:28:21.204Z
Learning: In the XRPL.js library, hardcoded seeds in test files are acceptable as they don't represent protected data or real funds - they're only used for consistent test behavior.
Applied to files:
.github/workflows/release.yml
📚 Learning: 2024-09-26T21:14:56.813Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2788
File: .github/workflows/nodejs.yml:25-25
Timestamp: 2024-09-26T21:14:56.813Z
Learning: In `.github/workflows/nodejs.yml`, the `build-and-lint` job's actions `actions/setup-node` and `actions/cache` are updated to `v4`.
Applied to files:
.github/workflows/release.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (9)
.github/workflows/release.yml (9)
12-33: New input and outputs wired correctly.The new
release_branch_nameinput and the expanded outputs (dist_tag,release_branch,release_pr_number,release_pr_url,is_beta) provide a clean contract for downstream jobs to access validated, canonical values.
41-137: Comprehensive input validation and beta/stable branching logic.The validation step correctly implements the PR objectives: validates inputs, derives
is_betafrom dist-tag semantics, appends "-experimental" suffix for beta releases, enforces release-branch naming for stable releases, and requires PR existence only for stable releases.
159-166: Verify redundantNPM_DIST_TAGassignment.Line 159 reassigns
NPM_DIST_TAGfrom the input with a default fallback, butNPM_DIST_TAGwas already normalized and written toGITHUB_ENVin thevalidate_inputsstep (lines 103–106). A past reviewer flagged similar redundancy.Confirm whether line 159 is necessary or if it should be removed/refactored to avoid duplication.
If this assignment is redundant, remove it:
# Validate package_name if ! [[ "${PKG_NAME}" =~ ^[a-z0-9][a-z0-9-]*$ ]]; then echo "❌ Invalid package_name '${PKG_NAME}' (allowed: [a-z0-9-], must start with alnum)." >&2 exit 1 fi - - NPM_DIST_TAG="${NPM_DIST_TAG:-latest}" if [[ "${IS_BETA:-false}" != "true" ]] && ! [[ "$VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; thenAlternatively, if
NPM_DIST_TAGneeds to be re-evaluated in this step, document why.
160-163: Correct SemVer validation for stable vs. beta releases.The validation correctly enforces x.y.z SemVer for stable releases (
latestdist-tag) while allowing other formats for beta releases. The duplicate check at lines 615–618 in the publish step serves as a defensive safety gate.
91-96: Correct "-experimental" suffix logic for beta releases.The logic correctly appends "-experimental" to non-
latestdist-tags, aligning with the PR objective. ModifyingNPM_DIST_TAGin thevalidate_inputsstep ensures consistency across all downstream uses.
649-650: Correct GitHub release flags for beta vs. stable.The
prereleaseandmake_latestflags are correctly driven by theis_betaoutput: beta releases are marked as prerelease (not latest), and stable releases are marked as latest.
592-621: Correct npm publish validation and flags.The publish step correctly validates that
NPM_DIST_TAGis set and enforces SemVer only for stable releases. The--provenanceflag provides good security posture. The logic allows beta releases with non-SemVer versions while rejecting stable releases with invalid SemVer.
458-485: Slack notifications appropriately contextualized for dev and release workflows.The dev team review message includes reviewer lists, run URLs, and conditionally includes the PR update URL when available. The structure is clear and provides all necessary context for approvers.
112-130: Correct PR validation logic for stable releases.The PR existence check for stable releases correctly uses the GitHub API to validate that an open PR exists from the release branch to
main. The query filters (state=open,base=main,head=owner:branch) are correctly formatted. Beta releases appropriately bypass this check. The error message provides clear guidance.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release.yml (1)
140-166: Critical: NPM_DIST_TAG not properly propagated from validate_inputs to get_version stepThe
validate_inputsstep modifiesNPM_DIST_TAGby appending "-experimental" for beta releases (line 95) and writes it toGITHUB_ENV(line 104). However, theget_versionstep re-reads from the original input (line 145) instead of the validated environment variable. This causes thedist_tagoutput (line 166) to be incorrect for beta releases—it will lack the "-experimental" suffix.Impact: The
releasejob usesneeds.get_version.outputs.dist_tag(line 594), which publishes to npm with the wrong dist tag.Fix: Use the environment variable instead:
- name: Get package version from package.json id: get_version env: IS_BETA: ${{ steps.validate_inputs.outputs.is_beta }} PKG_NAME: ${{ github.event.inputs.package_name }} - NPM_DIST_TAG: ${{ github.event.inputs.npmjs_dist_tag }} + NPM_DIST_TAG: ${{ env.NPM_DIST_TAG }} run: |Also fix line 362:
RELEASE_BRANCH: "${{ needs.get_version.outputs.release_branch }}"(use output instead of input).Minor: Typo at line 666: "succesfully" → "successfully".
♻️ Duplicate comments (1)
.github/workflows/release.yml (1)
362-362: Use validated output instead of input for consistency.Line 362 still references
github.event.inputs.release_branch_namedirectly, but therelease_branchoutput fromget_versionis the validated canonical value. This comment was marked as addressed in past reviews but the code still contains the issue.env: PKG_VERSION: "${{ needs.get_version.outputs.package_version }}" PKG_NAME: "${{ github.event.inputs.package_name }}" - RELEASE_BRANCH: "${{ github.event.inputs.release_branch_name }}" + RELEASE_BRANCH: "${{ needs.get_version.outputs.release_branch }}"
🧹 Nitpick comments (4)
.github/workflows/release.yml (4)
683-683: Verify:generate-documentationgit_ref should reference the release tag, not branch.Per the learning from PR 3101, the
generate-documentationjob should checkout the actual released tag (not just the release branch) to ensure documentation is generated from the correct commit. Currently, it usesneeds.get_version.outputs.release_branch.The
releasejob creates a tag (line 637) and outputstag_name(line 640), but this output is not exported by the job itself—only by the internal step. Downstream jobs cannot access it.Suggested actions:
- Add
tag_nameas a job-level output in thereleasejob:outputs: tag_name: ${{ steps.create_tag.outputs.tag_name }}
- Update
generate-documentationto use the tag:- git_ref: ${{ needs.get_version.outputs.release_branch }} + git_ref: ${{ needs.release.outputs.tag_name }}Alternatively, if the branch and released tag always point to the same commit, document this assumption.
98-101: Validate release branch for beta releases too.Per the PR description, "Workflow will be triggered from the main branch for both beta and stable releases." Currently, line 98-101 only validates the branch name for stable releases (requires
release-orrelease/prefix). Beta releases skip this check entirely.Consider adding validation to ensure beta releases originate from the
mainbranch:- if [ "$IS_BETA" != "true" ] && [[ ! "${RELEASE_BRANCH,,}" =~ ^release[-/] ]]; then - echo "❌ Release branch '$RELEASE_BRANCH' must start with 'release-' or 'release/' for stable releases." >&2 - exit 1 + if [ "$IS_BETA" = "true" ]; then + if [[ "${RELEASE_BRANCH}" != "main" ]]; then + echo "❌ Beta releases must be triggered from 'main' branch, not '${RELEASE_BRANCH}'." >&2 + exit 1 + fi + else + if [[ ! "${RELEASE_BRANCH,,}" =~ ^release[-/] ]]; then + echo "❌ Release branch '$RELEASE_BRANCH' must start with 'release-' or 'release/' for stable releases." >&2 + exit 1 + fi fi
719-719: Pin shell tobashin notify_failures step.The step uses
set -euo pipefail(line 719), which is bash-specific and will not work withdashorsh. Explicitly specifyshell: bashto ensure the pipefail option is honored:steps: - name: Notify Slack about workflow failure + shell: bash env: SLACK_TOKEN: ${{ secrets.SLACK_TOKEN }}
666-666: Minor: Fix typo in success message.- text="${PKG_NAME} ${PKG_VERSION} has been succesfully released and published to npm.js. Release URL: ${RELEASE_URL}" + text="${PKG_NAME} ${PKG_VERSION} has been successfully released and published to npm.js. Release URL: ${RELEASE_URL}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(18 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3101
File: .github/workflows/nodejs.yml:220-221
Timestamp: 2025-10-09T20:16:27.834Z
Learning: In `.github/workflows/nodejs.yml`, the `generate-documentation` job intentionally uses `if: startsWith(github.ref, 'refs/tags/xrpl@')` instead of `env.GIT_REF` to ensure documentation is only generated and deployed on actual tag push events, not when the workflow is called via `workflow_call` with a tag reference.
📚 Learning: 2025-10-09T20:16:27.834Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3101
File: .github/workflows/nodejs.yml:220-221
Timestamp: 2025-10-09T20:16:27.834Z
Learning: In `.github/workflows/nodejs.yml`, the `generate-documentation` job intentionally uses `if: startsWith(github.ref, 'refs/tags/xrpl@')` instead of `env.GIT_REF` to ensure documentation is only generated and deployed on actual tag push events, not when the workflow is called via `workflow_call` with a tag reference.
Applied to files:
.github/workflows/release.yml
📚 Learning: 2025-04-16T15:28:21.204Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2801
File: packages/xrpl/test/wallet/batchSigner.test.ts:0-0
Timestamp: 2025-04-16T15:28:21.204Z
Learning: In the XRPL.js library, hardcoded seeds in test files are acceptable as they don't represent protected data or real funds - they're only used for consistent test behavior.
Applied to files:
.github/workflows/release.yml
📚 Learning: 2024-09-26T21:14:56.813Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2788
File: .github/workflows/nodejs.yml:25-25
Timestamp: 2024-09-26T21:14:56.813Z
Learning: In `.github/workflows/nodejs.yml`, the `build-and-lint` job's actions `actions/setup-node` and `actions/cache` are updated to `v4`.
Applied to files:
.github/workflows/release.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: integration (24.x)
- GitHub Check: integration (20.x)
- GitHub Check: browser (24.x)
- GitHub Check: integration (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: unit (24.x)
- GitHub Check: unit (22.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
.github/workflows/release.yml (1)
186-186: Clarify intent ofalways()in conditional gates.Multiple jobs use
if: ${{ always() && condition }}(lines 186, 354, 489, 508, 548). Past review comments suggest removingalways(), but this conflicts with the requirement that "tests are allowed to fail for beta releases."Question: Do you want these jobs to:
- Option A: Proceed even if upstream jobs fail (requires
always()), but gate on conditions like(tests_passed || is_beta)?- Option B: Skip if upstream jobs fail (no
always()), and only run if conditions are met?The current pattern allows beta releases to proceed through gates even when tests fail, which aligns with the PR objectives. However, past reviewers flagged this as potentially incorrect.
Please clarify the intended behavior so the conditionals can be finalized.
Also applies to: 354-354, 489-489, 508-508, 548-548
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
RELEASE.md (3)
62-64: Explicitly referencerelease_branch_nameas the Git ref for tests.Line 62 mentions the workflow "run[s] unit, integration, and faucet tests against the specified Git ref" but doesn't clarify that this ref is the
release_branch_nameinput provided in the previous step. Explicitly naming it would improve clarity.-Triggers the `faucet_test.yml` and `nodejs.yml` workflows to run unit, integration, and faucet tests against the specified Git ref. +Triggers the `faucet_test.yml` and `nodejs.yml` workflows to run unit, integration, and faucet tests against the release branch (`release_branch_name`).
48-48: Make "pause at specific steps" more explicit.Line 48 states the pipeline will "pause at specific steps" but doesn't enumerate them. Based on the PR context, this should explicitly mention the steps where manual review is required (e.g., before final release, after security scan).
-The pipeline will pause at the "Print Test/Security scan result and invite Dev team to review" step and also before the final release step, relevant team should review the release details and scan result. +The pipeline pauses at two points for manual review: (1) after the security scan results are published, and (2) before the final npm publish and GitHub release. Teams must approve before proceeding to each next stage.
1-111: Document the newgenerate-documentationandnotify_failuresjobs.The PR introduces two new top-level jobs (
generate-documentationandnotify_failures) as part of the beta release workflow restructuring, but they are not mentioned in this guide. Users should understand when and how these jobs run.Add a new section (after the "Release Stage" section or within "How the Pipeline Works") briefly describing these jobs:
+### 6. **Post-Release Steps** +- **generate-documentation**: Generates and deploys package documentation (runs on tag push events). +- **notify_failures**: Notifies relevant teams on Slack if any prior stage fails.Confirm the exact triggering conditions for
generate-documentation(e.g., does it run for both stable and beta, or only on certain dist tags?). Based on retrieved learnings, documentation generation should only occur on actual tag push events, not when called viaworkflow_call. Ensure this behavior is documented or clarified if the new job changes it.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
RELEASE.md(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3101
File: .github/workflows/nodejs.yml:220-221
Timestamp: 2025-10-09T20:16:27.834Z
Learning: In `.github/workflows/nodejs.yml`, the `generate-documentation` job intentionally uses `if: startsWith(github.ref, 'refs/tags/xrpl@')` instead of `env.GIT_REF` to ensure documentation is only generated and deployed on actual tag push events, not when the workflow is called via `workflow_call` with a tag reference.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: integration (24.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (20.x)
- GitHub Check: browser (24.x)
- GitHub Check: unit (22.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
RELEASE.md (1)
27-29: Clarify the relationship between triggering location andrelease_branch_nameinput.The documentation states the workflow must be triggered from
main, then asks users to providerelease_branch_name. This could be clearer—specifically:
- Does "triggered from main" mean the workflow_dispatch UI is accessed on the main branch, or does the workflow checkout
release_branch_nameregardless of the current branch?- For stable releases with a merged PR, should
release_branch_nameequal the now-merged release branch name ormainitself?Please clarify the operational flow and update lines 27-29 to explain whether users need to switch branches before triggering, or if the workflow independently checks out the specified
release_branch_name.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
RELEASE.md(3 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
RELEASE.md
13-13: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
26-26: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: integration (24.x)
- GitHub Check: browser (24.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (20.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
RELEASE.md (4)
13-36: Documentation properly reflects beta vs stable release workflows.The documentation clearly separates and documents the distinct requirements for stable and beta releases, including branch naming conventions, versioning, and distribution tag handling. The beta release guidance around the "-experimental" suffix and flexible branch naming aligns with the PR objectives.
40-56: Release trigger section correctly documents newrelease_branch_nameinput.The triggering instructions now document the
release_branch_nameparameter alongside existing inputs, and the example table is updated to reflect this change. The note that the workflow must be triggered frommainaligns with the PR objectives.
61-61: Review ownership documentation properly differentiates stable vs beta paths.Line 61 correctly documents that stable releases are reviewed by the infosec team, while beta releases are reviewed by security champions from the Dev team. This aligns with the PR objectives and stakeholder changes in the release process.
77-77: Test failure guidance properly reflects beta release allowances.The note that tests are allowed to fail for beta releases aligns with the PR objectives and provides necessary guidance for release managers conducting beta releases.
|
|
||
| ### **Before triggering a release** | ||
|
|
||
| **Stable release ** |
There was a problem hiding this comment.
Fix markdown formatting: remove spaces before closing emphasis markers.
Lines 13 and 26 have trailing spaces inside emphasis markers (**Stable release ** and **Beta release **), which violates the MD037 rule.
Apply this diff to fix the formatting:
-**Stable release **
+**Stable release**-**Beta release **
+**Beta release**As per markdownlint, follow this pattern: **text** (no spaces inside markers).
Also applies to: 26-26
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
13-13: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
🤖 Prompt for AI Agents
In RELEASE.md around lines 13 and 26, the bold emphasis markers include trailing
spaces before the closing asterisks ("**Stable release **" and "**Beta release
**"); remove the interior trailing spaces so the markers read "**Stable
release**" and "**Beta release**" to comply with markdownlint MD037. Edit those
lines to eliminate the extra space(s) immediately before the closing ** for each
emphasized phrase.
RELEASE.md
Outdated
| ### **Reviewing the release details and scan result** | ||
|
|
||
| 1. The pipeline will pause at the "Print Test/Security scan result and invite Dev team to review" step and also before the final release step, relevant team should review the release details and scan result. | ||
| 1. The pipeline will pause at the "Print Test/Security scan result and invite Dev team to review" step and also before the final release step, relevant team should review the release details and scan result. Stable release release will be reviewed by infosec team as Sec reviewer. Beta release will be reviewed by security champions from Dev team. |
There was a problem hiding this comment.
| 1. The pipeline will pause at the "Print Test/Security scan result and invite Dev team to review" step and also before the final release step, relevant team should review the release details and scan result. Stable release release will be reviewed by infosec team as Sec reviewer. Beta release will be reviewed by security champions from Dev team. | |
| 1. The pipeline will pause at the "Print Test/Security scan result and invite Dev team to review" step and also before the final release step, relevant team should review the release details and scan result. Stable release will be reviewed by infosec team as Sec reviewer. Beta release will be reviewed by security champions from Dev team. |
| -H "X-Api-Key: ${OWASP_TOKEN}" \ | ||
| -F "autoCreate=true" \ | ||
| -F "projectName=xrpl-js" \ | ||
| -F "projectName=test-alert" \ |
There was a problem hiding this comment.
Forget to revert the change?
| ``` | ||
| 3. Run npm i to update the package-lock with the updated versions and commit the lock file to the release branch | ||
| 4. Set `npm distribution tag` to `latest`. | ||
| 5. Run npm i to update the package-lock with the updated versions and commit the lock file to the release branch |
There was a problem hiding this comment.
Can we make this line and L36 the same? Let's not describe the same thing differently in two processes.
There was a problem hiding this comment.
for L36, I meant to tell the the npm distribution tag can't be "latest", so it will be treated as beta release @kuan121
There was a problem hiding this comment.
This is nit, but I meant these two lines
- Run npm i to update the package-lock with the updated versions and commit the lock file to the release branch
- Run
npm ito refreshpackage-lock.jsonand commit it.
Are they trying to describe the same thing?
There was a problem hiding this comment.
that make sense. fixed it.
ahoffman-ripple
left a comment
ahoffman-ripple
left a comment
There was a problem hiding this comment.
update projectName in release.yml, beyond that looks good to go
|
Re: #3131 (comment) The reviewers are fetched from a GitHub protected environment. However, npmjs supports only a single protected environment for trusted publishing, which must be the designated “official-release” environment. As a result, reviewers configured in other protected environments cannot grant approval; only reviewers defined within the “official-release” environment are permitted to authorize the release. |
|
Overall, LGTM. We can merge once:
|
| ### **Before triggering a release** | ||
|
|
||
| **Stable release ** | ||
| 1. Create a release branch. A qualified branch name should start with "release-" or "release/", case-insensitive. e.g: `release/xrpl@4.3.8`, `release-xrpl-4.3.8`, `Release/xrpl@4.3.8`. |
There was a problem hiding this comment.
As per changes to release.yml, upper case release branch names are not allowed. RELEASE.md should be updated.
There was a problem hiding this comment.
Environment protection rules for offical-review, github-pages and first-review etc should be updated accordingly to reflect these changes.
There was a problem hiding this comment.
thanks for spotting it. I have made change to release.yml to match the environment protection rule.
| PR_NUMBER="" | ||
| PR_URL="" | ||
| # For stable releases, check that a PR exists from the release branch to main |
There was a problem hiding this comment.
I was under the impression that for stable releases, we would first merge the PR into main branch and then release it from main branch itself so that provenance commit details match. There won't be any need for RELEASE_BRANCH for stable releases. So, checking release -> main PR would be unnecessary for stable releases.
For beta releases, we would release from RELEASE_BRANCH since, we can't merge beta branch into main.
Do you see any issue with this approach?
There was a problem hiding this comment.
Another benefit of releasing stable releases from main branch is that the Git tags are tagged on the commit of main branch as opposed to the commit of release branch.
| } | ||
| ``` | ||
| 3. Run npm i to update the package-lock with the updated versions and commit the lock file to the release branch | ||
| 4. Set `npm distribution tag` to `latest`. |
There was a problem hiding this comment.
This is a Github Action step and should be mentioned somewhere else (or at the end of the list), since other steps are the one developer performs on their local machine.
6 participants
High Level Overview of Change
1, Workflow needs to be triggered from main branch for both beta and stable release
2, For stable release, A PR from release branch to main branch is required before triggering the release pipeline
3, Security champion(Chenna and Cheng) are added to Security reviewers. Beta sec review will be done by Security champion.
4, "-experimental" will automatically appended to dist tag for beta release
5, Tests are allowed to fail for beta release
Context of Change
Type of Change
Did you update HISTORY.md?
Test Plan