ci: Automate develop sync - #40885
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughAdds a new sync-develop action path and workflow that merge master into develop after final release completion, resolve selected conflicts, and open or update pull requests with optional auto-merge. ChangesDevelop-master sync feature
Suggested labels: Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant release-action as release-action
participant syncDevelopWithMaster as syncDevelopWithMaster
participant git operations as git operations
participant GitHub API as GitHub API
release-action->>syncDevelopWithMaster: githubToken
syncDevelopWithMaster->>git operations: fetch master and develop refs
syncDevelopWithMaster->>git operations: merge master without commit
git operations-->>syncDevelopWithMaster: conflicted files
syncDevelopWithMaster->>syncDevelopWithMaster: classify conflicts
alt unresolvable conflicts
syncDevelopWithMaster->>git operations: abort merge and reset branch
syncDevelopWithMaster->>GitHub API: create manual-resolution PR
else version and changelog conflicts only
syncDevelopWithMaster->>git operations: resolve version files and changelog
syncDevelopWithMaster->>git operations: add files, commit, push
syncDevelopWithMaster->>GitHub API: create or update PR
syncDevelopWithMaster->>GitHub API: enable auto-merge
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #40885 +/- ##
===========================================
- Coverage 69.26% 69.21% -0.05%
===========================================
Files 3536 3536
Lines 138592 138592
Branches 24681 24725 +44
===========================================
- Hits 95991 95930 -61
- Misses 38592 38646 +54
- Partials 4009 4016 +7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
coderabbitai
Bot
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (16)
packages/release-action/src/syncDevelop.ts (10)
93-93: ⚡ Quick winRemove implementation comment per coding guidelines.
The variable name
extraVersionFilesand filter operation are self-documenting.♻️ Suggested diff
- // the non-package.json version files (e.g. apps/meteor/app/utils/rocketchat.info) const extraVersionFiles = (await getUpdateFilesList(cwd)).filter((file) => path.basename(file) !== 'package.json');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/release-action/src/syncDevelop.ts` at line 93, Delete the implementation comment "// the non-package.json version files (e.g. apps/meteor/app/utils/rocketchat.info)" above the extraVersionFiles filter; the variable name extraVersionFiles and its filter operation are self-explanatory, so remove that stray comment to comply with coding guidelines.Source: Coding guidelines
26-28: ⚡ Quick winRemove implementation comment per coding guidelines.
The constant name
TITLE_PREFIXand its value'chore: sync develop with master'are self-documenting.♻️ Suggested diff
-// Every sync PR title starts with this conventional prefix so it satisfies the -// repo's PR title rules (see .github/PULL_REQUEST_TEMPLATE.md). A branch sync is -// housekeeping with no end-user changelog impact, hence `chore:`. const TITLE_PREFIX = 'chore: sync develop with master';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/release-action/src/syncDevelop.ts` around lines 26 - 28, Remove the surrounding explanatory comment above the TITLE_PREFIX constant: the name TITLE_PREFIX and its value 'chore: sync develop with master' are self-describing, so delete the multi-line implementation comment and leave only the constant declaration (refer to TITLE_PREFIX in syncDevelop.ts) to comply with coding guidelines.Source: Coding guidelines
60-62: ⚡ Quick winRemove implementation comment per coding guidelines.
The function name
unionMergeChangelogand git command arguments document the behavior.♻️ Suggested diff
-// Resolve a conflicted CHANGELOG.md with a lossless union of both sides so no -// release notes are dropped. Ordering still needs a human eye, which is why the -// changelog tier always opens a review PR rather than auto-merging. async function unionMergeChangelog(file: string) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/release-action/src/syncDevelop.ts` around lines 60 - 62, Remove the implementation comment block that documents behavior above the unionMergeChangelog function; the function name unionMergeChangelog and the git command arguments already express the behavior, so delete the three-line explanatory comment (about resolving conflicted CHANGELOG.md with a lossless union and opening a review PR) to comply with coding guidelines and keep the source concise.Source: Coding guidelines
207-207: ⚡ Quick winRemove implementation comment per coding guidelines.
The warning message in the catch block already explains the non-critical nature of the failure.
♻️ Suggested diff
} catch (err) { - // auto-merge may be disabled on the repo or the PR may already be mergeable; don't fail the sync core.warning(`Could not enable auto-merge: ${(err as Error).message}`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/release-action/src/syncDevelop.ts` at line 207, Remove the implementation comment "// auto-merge may be disabled on the repo or the PR may already be mergeable; don't fail the sync" from the catch block in syncDevelop.ts; leave the existing warning log message intact (do not change the catch logic or logging), so only delete that explanatory inline comment per coding guidelines.Source: Coding guidelines
96-96: ⚡ Quick winRemove implementation comment per coding guidelines.
The git operations that follow clearly show the branching and merging strategy.
♻️ Suggested diff
- // branch off develop, then merge master into it await resetBranchTo(syncBranch, `origin/${TARGET}`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/release-action/src/syncDevelop.ts` at line 96, Remove the redundant implementation comment "// branch off develop, then merge master into it" from the code (it's purely descriptive of the following git operations). Delete that single-line comment in syncDevelop.ts so the code follows the guideline of avoiding implementation comments; no code changes besides removing that comment are needed.Source: Coding guidelines
88-88: ⚡ Quick winRemove implementation comment per coding guidelines.
The variable name
versionand JSON parsing already convey the purpose.♻️ Suggested diff
- // the released version, read from master's root package.json — used for titles const { version } = JSON.parse(await showFile(`origin/${SOURCE}:package.json`));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/release-action/src/syncDevelop.ts` at line 88, Remove the implementation comment "// the released version, read from master's root package.json — used for titles" above the version variable; locate the declaration named version (where JSON.parse is used to read package.json) and delete that comment so the code follows the guideline that the variable name and JSON parsing are self-descriptive.Source: Coding guidelines
146-146: ⚡ Quick winRemove implementation comment per coding guidelines.
The final
ensurePullRequestcall withautoMerge: truedocuments the tier-1 behavior.♻️ Suggested diff
- // Tier 1: only version lines conflicted (or a clean merge) → safe to auto-merge. await ensurePullRequest({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/release-action/src/syncDevelop.ts` at line 146, Remove the redundant implementation comment "// Tier 1: only version lines conflicted (or a clean merge) → safe to auto-merge." in syncDevelop.ts; the behavior is already documented by the subsequent ensurePullRequest call that passes autoMerge: true, so just delete that comment and leave the ensurePullRequest(...) invocation and its autoMerge flag unchanged.Source: Coding guidelines
103-104: ⚡ Quick winRemove implementation comment per coding guidelines.
The condition, warning message, and subsequent operations document the tier-3 handling.
♻️ Suggested diff
- // Tier 3: real code conflicts — we can't safely resolve. Open a manual PR with - // master as-is so a human resolves everything in the PR. if (other.length > 0) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/release-action/src/syncDevelop.ts` around lines 103 - 104, Remove the implementation-only comment "Tier 3: real code conflicts — we can't safely resolve. Open a manual PR with master as-is so a human resolves everything in the PR." from syncDevelop (packages/release-action/src/syncDevelop.ts) — delete those lines so the code contains only the condition, warning message, and operations themselves, aligning with the coding guideline to avoid implementation comments.Source: Coding guidelines
122-122: ⚡ Quick winRemove implementation comment per coding guidelines.
The operations clearly show version and changelog conflict resolution.
♻️ Suggested diff
- // resolve the predictable conflicts: version lines keep develop's side for (const file of versionFiles) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/release-action/src/syncDevelop.ts` at line 122, Remove the implementation-only comment "// resolve the predictable conflicts: version lines keep develop's side" from packages/release-action/src/syncDevelop.ts (it is an implementation detail comment in the sync logic); simply delete that line so the code contains no explanatory implementation comment while leaving the surrounding logic and any conflict-resolution behavior in the syncDevelop function intact.Source: Coding guidelines
134-134: ⚡ Quick winRemove implementation comment per coding guidelines.
The condition and function call make the tier-2 handling clear.
♻️ Suggested diff
- // Tier 2: changelog conflicts were auto-resolved but ordering needs review → no auto-merge. if (changelogs.length > 0) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/release-action/src/syncDevelop.ts` at line 134, Remove the inline implementation comment above the tier-2 branch (the comment starting with "Tier 2: changelog conflicts were auto-resolved but ordering needs review → no auto-merge.") — delete that comment and leave the existing condition and function call handling the tier-2 case untouched, since the condition and invocation already make the behavior clear.Source: Coding guidelines
.github/workflows/develop-sync.yml (1)
23-26: ⚡ Quick winConsider adding
persist-credentials: falsefor defense in depth.While the CI_PAT token is necessary for subsequent git operations, explicitly setting
persist-credentials: falseprevents the token from being accessible to scripts run in later steps, reducing the attack surface if a compromised dependency or build script attempts to extract credentials.🛡️ Suggested addition
- name: Checkout Repo uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: fetch-depth: 0 token: ${{ secrets.CI_PAT }} + persist-credentials: falseNote: You'll need to ensure that git operations in the "Sync develop with master" step (line 38-43) explicitly use the token from the environment variable rather than relying on persisted credentials.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/develop-sync.yml around lines 23 - 26, Add persist-credentials: false to the actions/checkout step to avoid exposing the CI_PAT to later steps; update the checkout usage in the workflow (the actions/checkout invocation shown) and ensure subsequent git operations in the "Sync develop with master" step explicitly use the token from the environment (e.g., the CI_PAT secret) rather than relying on persisted credentials.packages/release-action/src/gitUtils.ts (5)
49-50: ⚡ Quick winRemove implementation comment per coding guidelines.
The function signature and return type already document the behavior.
♻️ Suggested diff
-// start a merge but leave it uncommitted so conflicts can be inspected/resolved. -// returns true when the merge applied cleanly, false when there are conflicts. export async function mergeNoCommit(ref: string): Promise<boolean> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/release-action/src/gitUtils.ts` around lines 49 - 50, Remove the redundant implementation comment above the merge helper in gitUtils.ts (the two-line comment starting "// start a merge..." and "// returns true...") because the function signature/return type already document its behavior; locate the comment near the merge helper function (e.g., the function that returns a boolean indicating whether the merge applied cleanly) and delete those lines to comply with the coding guidelines.Source: Coding guidelines
44-45: ⚡ Quick winRemove implementation comment per coding guidelines.
The coding guidelines specify "Avoid code comments in the implementation" for TypeScript files. The function name and signature already convey the intent.
♻️ Suggested diff
-// reset/create a local branch pointing at the given ref (e.g. origin/develop) export async function resetBranchTo(branch: string, ref: string) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/release-action/src/gitUtils.ts` around lines 44 - 45, Remove the inline implementation comment above the function declaration for resetBranchTo; the function name and signature already convey intent, so delete the comment line "// reset/create a local branch pointing at the given ref (e.g. origin/develop)" directly above the export async function resetBranchTo(branch: string, ref: string) declaration to comply with the TypeScript coding guideline to avoid implementation comments.Source: Coding guidelines
95-95: ⚡ Quick winRemove implementation comment per coding guidelines.
The function name is self-documenting.
♻️ Suggested diff
-// contents of a file at a given ref (e.g. origin/master:package.json) or merge stage (e.g. :2:CHANGELOG.md) export async function showFile(ref: string): Promise<string> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/release-action/src/gitUtils.ts` at line 95, Remove the redundant implementation comment "// contents of a file (e.g. origin/master:package.json) or merge stage (e.g. :2:CHANGELOG.md)" which simply restates the function behavior; leave the surrounding code and function signature unchanged so the function (the one that returns file contents at a ref/merge stage) remains self-documenting without the comment.Source: Coding guidelines
69-69: ⚡ Quick winRemove implementation comment per coding guidelines.
The function name clearly describes the resolution strategy.
♻️ Suggested diff
-// resolve a conflicted file by keeping our side (the branch we merge into) export async function checkoutOurs(file: string) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/release-action/src/gitUtils.ts` at line 69, Remove the implementation comment "// resolve a conflicted file by keeping our side (the branch we merge into)" that sits above the conflict-resolution function; per guidelines, delete this descriptive implementation comment and leave the function (resolveConflictKeepOurs) signature and body unchanged so the code relies on the self-describing function name instead of the inline comment.Source: Coding guidelines
82-82: ⚡ Quick winRemove implementation comment per coding guidelines.
The function name and boolean return type convey the meaning.
♻️ Suggested diff
-// true when `ancestor` is reachable from `descendant` (nothing to sync) export async function isAncestor(ancestor: string, descendant: string): Promise<boolean> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/release-action/src/gitUtils.ts` at line 82, Remove the implementation comment "// true when `ancestor` is reachable from `descendant` (nothing to sync)" that sits above the boolean-returning reachability function (the function that checks if an ancestor is reachable from a descendant); the function name and its boolean return type already convey the meaning so simply delete that comment to comply with the coding guidelines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/develop-sync.yml:
- Around line 23-26: Add persist-credentials: false to the actions/checkout step
to avoid exposing the CI_PAT to later steps; update the checkout usage in the
workflow (the actions/checkout invocation shown) and ensure subsequent git
operations in the "Sync develop with master" step explicitly use the token from
the environment (e.g., the CI_PAT secret) rather than relying on persisted
credentials.
In `@packages/release-action/src/gitUtils.ts`:
- Around line 49-50: Remove the redundant implementation comment above the merge
helper in gitUtils.ts (the two-line comment starting "// start a merge..." and
"// returns true...") because the function signature/return type already
document its behavior; locate the comment near the merge helper function (e.g.,
the function that returns a boolean indicating whether the merge applied
cleanly) and delete those lines to comply with the coding guidelines.
- Around line 44-45: Remove the inline implementation comment above the function
declaration for resetBranchTo; the function name and signature already convey
intent, so delete the comment line "// reset/create a local branch pointing at
the given ref (e.g. origin/develop)" directly above the export async function
resetBranchTo(branch: string, ref: string) declaration to comply with the
TypeScript coding guideline to avoid implementation comments.
- Line 95: Remove the redundant implementation comment "// contents of a file
(e.g. origin/master:package.json) or merge stage (e.g. :2:CHANGELOG.md)" which
simply restates the function behavior; leave the surrounding code and function
signature unchanged so the function (the one that returns file contents at a
ref/merge stage) remains self-documenting without the comment.
- Line 69: Remove the implementation comment "// resolve a conflicted file by
keeping our side (the branch we merge into)" that sits above the
conflict-resolution function; per guidelines, delete this descriptive
implementation comment and leave the function (resolveConflictKeepOurs)
signature and body unchanged so the code relies on the self-describing function
name instead of the inline comment.
- Line 82: Remove the implementation comment "// true when `ancestor` is
reachable from `descendant` (nothing to sync)" that sits above the
boolean-returning reachability function (the function that checks if an ancestor
is reachable from a descendant); the function name and its boolean return type
already convey the meaning so simply delete that comment to comply with the
coding guidelines.
In `@packages/release-action/src/syncDevelop.ts`:
- Line 93: Delete the implementation comment "// the non-package.json version
files (e.g. apps/meteor/app/utils/rocketchat.info)" above the extraVersionFiles
filter; the variable name extraVersionFiles and its filter operation are
self-explanatory, so remove that stray comment to comply with coding guidelines.
- Around line 26-28: Remove the surrounding explanatory comment above the
TITLE_PREFIX constant: the name TITLE_PREFIX and its value 'chore: sync develop
with master' are self-describing, so delete the multi-line implementation
comment and leave only the constant declaration (refer to TITLE_PREFIX in
syncDevelop.ts) to comply with coding guidelines.
- Around line 60-62: Remove the implementation comment block that documents
behavior above the unionMergeChangelog function; the function name
unionMergeChangelog and the git command arguments already express the behavior,
so delete the three-line explanatory comment (about resolving conflicted
CHANGELOG.md with a lossless union and opening a review PR) to comply with
coding guidelines and keep the source concise.
- Line 207: Remove the implementation comment "// auto-merge may be disabled on
the repo or the PR may already be mergeable; don't fail the sync" from the catch
block in syncDevelop.ts; leave the existing warning log message intact (do not
change the catch logic or logging), so only delete that explanatory inline
comment per coding guidelines.
- Line 96: Remove the redundant implementation comment "// branch off develop,
then merge master into it" from the code (it's purely descriptive of the
following git operations). Delete that single-line comment in syncDevelop.ts so
the code follows the guideline of avoiding implementation comments; no code
changes besides removing that comment are needed.
- Line 88: Remove the implementation comment "// the released version, read from
master's root package.json — used for titles" above the version variable; locate
the declaration named version (where JSON.parse is used to read package.json)
and delete that comment so the code follows the guideline that the variable name
and JSON parsing are self-descriptive.
- Line 146: Remove the redundant implementation comment "// Tier 1: only version
lines conflicted (or a clean merge) → safe to auto-merge." in syncDevelop.ts;
the behavior is already documented by the subsequent ensurePullRequest call that
passes autoMerge: true, so just delete that comment and leave the
ensurePullRequest(...) invocation and its autoMerge flag unchanged.
- Around line 103-104: Remove the implementation-only comment "Tier 3: real code
conflicts — we can't safely resolve. Open a manual PR with master as-is so a
human resolves everything in the PR." from syncDevelop
(packages/release-action/src/syncDevelop.ts) — delete those lines so the code
contains only the condition, warning message, and operations themselves,
aligning with the coding guideline to avoid implementation comments.
- Line 122: Remove the implementation-only comment "// resolve the predictable
conflicts: version lines keep develop's side" from
packages/release-action/src/syncDevelop.ts (it is an implementation detail
comment in the sync logic); simply delete that line so the code contains no
explanatory implementation comment while leaving the surrounding logic and any
conflict-resolution behavior in the syncDevelop function intact.
- Line 134: Remove the inline implementation comment above the tier-2 branch
(the comment starting with "Tier 2: changelog conflicts were auto-resolved but
ordering needs review → no auto-merge.") — delete that comment and leave the
existing condition and function call handling the tier-2 case untouched, since
the condition and invocation already make the behavior clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4007af42-8517-4350-8d45-007c12acf71b
📒 Files selected for processing (6)
.github/workflows/develop-sync.ymlpackages/release-action/action.ymlpackages/release-action/src/gitUtils.tspackages/release-action/src/index.tspackages/release-action/src/syncDevelop.tspackages/release-action/src/utils.ts
📜 Review details
⏰ 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). (2)
- GitHub Check: Hacktron Security Check
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/release-action/src/utils.tspackages/release-action/src/index.tspackages/release-action/src/gitUtils.tspackages/release-action/src/syncDevelop.ts
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/release-action/src/utils.tspackages/release-action/src/index.tspackages/release-action/src/gitUtils.tspackages/release-action/src/syncDevelop.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/release-action/src/utils.tspackages/release-action/src/index.tspackages/release-action/src/gitUtils.tspackages/release-action/src/syncDevelop.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/release-action/src/utils.tspackages/release-action/src/index.tspackages/release-action/src/gitUtils.tspackages/release-action/src/syncDevelop.ts
🪛 zizmor (1.25.2)
.github/workflows/develop-sync.yml
[warning] 22-26: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 6-9: use of fundamentally insecure workflow trigger (dangerous-triggers): workflow_run is almost always used insecurely
(dangerous-triggers)
🔇 Additional comments (8)
packages/release-action/src/syncDevelop.ts (3)
40-58: LGTM!
156-194: LGTM!
212-240: LGTM!packages/release-action/src/utils.ts (1)
69-69: LGTM!packages/release-action/src/index.ts (1)
10-10: LGTM!Also applies to: 52-53
packages/release-action/action.yml (1)
6-6: LGTM!.github/workflows/develop-sync.yml (2)
35-43: LGTM!
6-19: Limit workflow_run attacker scope: ensure no workflow_run artifacts/payload are consumed by the called action
Publish Final Releaseis defined in.github/workflows/publish-release.ymlwithon: push: branches: [master], sodevelop-sync.yml’sworkflow_runcan only be triggered from runs of that workflow onmaster, anddevelop-sync.ymlfurther gates execution ongithub.event.workflow_run.conclusion == 'success'.Still, the security claim that this trigger path doesn’t consume any workflow_run artifacts/payload can’t be concluded from the
develop-sync.ymlsnippet alone—inspect./packages/release-action(used bydevelop-sync.ymlwithaction: sync-develop) for anyactions/download-artifactusage or reads ofgithub.event.workflow_run.*.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
| types: [completed] | ||
|
|
||
| concurrency: ${{ github.workflow }} | ||
|
|
There was a problem hiding this comment.
It would be nice to apply the principle of least privilege to the GitHub Actions permissions here. Setting permissions: {} by default and explicitly defining only what each job needs would avoid granting GITHUB_TOKEN more access than it needs.
For example, apply permissions: {} at this level and then add the required permissions per job.
This is part of a security initiative. More details can be found here: https://rocketchat.atlassian.net/browse/SB-975.
There is also a PR applying this approach across all workflows in the repository: #40687
49fa221 to
c93787b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/develop-sync.yml (1)
26-30: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
artipacked(persist-credentials) is a false positive here.The sync action pushes the
sync/master-to-develop-<sha>branch using the credentials persisted byactions/checkout, sopersist-credentials: falsewould break the push. No change needed. Note: theCI_PATremains in the git config during the subsequentsetup-node/installstep, so keep lifecycle-script execution from dependencies in mind as a supply-chain consideration (HUSKY: 0only covers husky).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/develop-sync.yml around lines 26 - 30, The Checkout Repo step in the develop-sync workflow should remain configured to persist credentials, because the sync job depends on those credentials to push the sync/master-to-develop-<sha> branch after actions/checkout. Do not add persist-credentials: false to this checkout; instead keep the existing CI_PAT-based setup intact and be mindful of the later setup-node/install steps where the token remains in git config.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/develop-sync.yml:
- Around line 26-30: The Checkout Repo step in the develop-sync workflow should
remain configured to persist credentials, because the sync job depends on those
credentials to push the sync/master-to-develop-<sha> branch after
actions/checkout. Do not add persist-credentials: false to this checkout;
instead keep the existing CI_PAT-based setup intact and be mindful of the later
setup-node/install steps where the token remains in git config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b96a7e9-ab49-46e5-8fe6-0468ce14347b
📒 Files selected for processing (1)
.github/workflows/develop-sync.yml
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
⚠️ CI failures not shown inline (5)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 Additional context used
🪛 zizmor (1.26.1)
.github/workflows/develop-sync.yml
[warning] 26-30: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 6-9: use of fundamentally insecure workflow trigger (dangerous-triggers): workflow_run is almost always used insecurely
(dangerous-triggers)
🔇 Additional comments (3)
.github/workflows/develop-sync.yml (3)
6-9: 🔒 Security & Privacy
workflow_runtrigger runs in a privileged context — confirm no untrusted code/input is consumed.The static analysis flags
workflow_runas a dangerous trigger because the job runs with repo secrets (hereCI_PAT) and write-capable context. The risk is mitigated here: there's aconclusion == 'success'gate, and theactions/checkoutstep uses the default ref (your default branch) rather than checking out an attacker-controlled PR head. Please confirm thesync-developaction does not read any attacker-influenced data from the triggering run (e.g.,workflow_runhead branch/PR title) when constructing branches, commits, or PR bodies.Source: Linters/SAST tools
11-11: LGTM!
16-24: LGTM!Also applies to: 42-47
Adds syncDevelopWithMaster() which merges master into develop after a release, resolving version-bump conflicts in develop's favour and routing changelog/code conflicts to review/manual sync PRs. Adds the git primitives it needs to gitUtils and exports getUpdateFilesList. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Runs after 'Publish Final Release' succeeds and invokes the release-action sync-develop mode to open a develop<-master sync PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
c93787b to
c7267d3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/develop-sync.yml (1)
19-22: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding
timeout-minutesto the job.No timeout is set; a stuck checkout, build, or sync step could block the job (and the shared concurrency group) indefinitely.
Proposed fix
sync: name: Sync develop if: ${{ github.event.workflow_run.conclusion == 'success' }} runs-on: ubuntu-24.04 + timeout-minutes: 15 permissions: contents: read🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/develop-sync.yml around lines 19 - 22, The sync job currently has no timeout, so a hung checkout, build, or sync step in the workflow can block it indefinitely. Add a timeout to the job definition for the sync job in the develop-sync workflow so the job fails after a reasonable duration; use the existing sync job stanza (with the sync and runs-on settings) to place the timeout alongside the other job-level keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/develop-sync.yml:
- Around line 19-22: The sync job currently has no timeout, so a hung checkout,
build, or sync step in the workflow can block it indefinitely. Add a timeout to
the job definition for the sync job in the develop-sync workflow so the job
fails after a reasonable duration; use the existing sync job stanza (with the
sync and runs-on settings) to place the timeout alongside the other job-level
keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ef30984-9253-4a3b-a6a4-22780dcc9370
📒 Files selected for processing (1)
.github/workflows/develop-sync.yml
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
⚠️ CI failures not shown inline (5)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 Additional context used
🪛 zizmor (1.26.1)
.github/workflows/develop-sync.yml
[warning] 26-30: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 6-9: use of fundamentally insecure workflow trigger (dangerous-triggers): workflow_run is almost always used insecurely
(dangerous-triggers)
🔇 Additional comments (3)
.github/workflows/develop-sync.yml (3)
11-11: Least-privilege permissions concern already addressed.Prior review requested
permissions: {}at top-level with per-job scoping; this is now implemented (Lines 16, 23-24).
6-9: 🔒 Security & PrivacyConfirm
workflow_runtrigger doesn't check out untrusted code.Static analysis flags
workflow_runas a dangerous trigger since it runs with repo secrets in context. Since noref:is set on the checkout step, it should default to this workflow file's own ref rather than the triggering run's head — please confirm this is the case and that nothing in the action ever checks out or executes code fromgithub.event.workflow_run.head_sha/head_branch.Source: Linters/SAST tools
26-30: 🔒 Security & PrivacyKeep
persist-credentialsenabled here. The sync action usesgit push/git push --forceinpackages/release-action/src/gitUtils.ts, so the checkout token must remain available in git config for this job.
2 participants
Proposed changes
Automates the develop sync that's required after every release merged to
master. Today this is a recurring manual chore: each release bumps the version line in ~76package.jsonfiles (+apps/meteor/app/utils/rocketchat.info), leavingdevelopbehindmasterand blocking the next release branch from merging cleanly.How it works
A new
sync-developmode inpackages/release-actionruns after the Publish Final Release workflow succeeds (viaworkflow_run). It mergesmasterinto async/master-to-develop-<sha>branch and classifies conflicts into three tiers:package.json,rocketchat.info)--ours(develop wins)CHANGELOG.md--ours; changelogs union-merged (lossless)If
developalready containsmaster, it exits without opening a PR. PRs are idempotent (an existing open sync PR is updated, not duplicated). All sync-PR titles use thechore:prefix to satisfy the repo's PR title rules. Auto-merge uses theMERGEmethod so master's history is preserved on develop, and goes through branch protection / CI like any other PR.Why
workflow_run(not a step in the publish job)Decouples the sync from the release job — a sync failure never blocks or obscures the publish — and it only fires when the release actually succeeded. It also naturally skips the old-version patch flow, which never pushes to
master.Files
packages/release-action/src/syncDevelop.ts— new sync logicpackages/release-action/src/gitUtils.ts— git primitives (fetch, merge--no-commit, conflict listing,checkout --ours, ancestor check, union helpers)packages/release-action/src/index.ts/action.yml— wire thesync-developmodepackages/release-action/src/utils.ts— exportgetUpdateFilesList.github/workflows/develop-sync.yml— trigger workflowNotes / follow-ups
dist/index.jsis gitignored and built in CI (the workflow builds the action before use), so it isn't committed.release-X.Y.Zminor branch, and a version-aware (vs. union) changelog merge.🤖 Generated with Claude Code
Summary by CodeRabbit
masterintodevelopafter a final release finishes successfully.sync-develop) that creates or updates a pull request targetingdevelop.CHANGELOG.mdwhen possible, otherwise opens a manual-review pull request.