Add upstream main sync support to sync workflow#28
Conversation
Add sync_target input (latest-tag or main) to workflow_dispatch. Scheduled runs stay tag-only. Introduces unified "Resolve sync target" step that normalizes both modes into common outputs (sync_ref, sync_label, branch_name, tracking_tag) so downstream steps work identically for both modes. Main syncs use independent tracking tag (helio-last-synced-main) and date+sha branch naming. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWorkflow adds a manual Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant Workflow as GitHub Action
participant Upstream
participant Repo
participant CI as Build
Scheduler->>Workflow: trigger (schedule/manual + sync_target)
Workflow->>Workflow: determine sync_mode
Workflow->>Upstream: resolve sync_ref / sync_sha / sync_label
Workflow->>Repo: read tracking_tag -> tracking_sha
Workflow->>Workflow: compare tracking_sha vs sync_sha
alt not synced
Workflow->>Repo: create branch (branch_name)
Workflow->>Repo: git merge sync_ref
Repo->>CI: run build/tests
CI-->>Workflow: outcome (success/failure)
alt success
Workflow->>Repo: force-update tracking_tag -> sync_sha
else failure
Workflow-->>Repo: leave tracking_tag unchanged
end
else already synced
Workflow-->>Scheduler: exit (no-op)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR extends the existing Helio upstream sync GitHub Actions workflow to support syncing either to the latest upstream v* tag (current behavior) or to upstream/main for manual workflow dispatches, while keeping scheduled runs tag-only.
Changes:
- Add
sync_targetinput (latest-tagdefault, ormain) forworkflow_dispatch. - Introduce “Determine sync mode” + “Resolve sync target” steps that normalize outputs (
sync_ref,sync_label,branch_name,tracking_tag, etc.). - Add separate tracking tag for main syncs (
helio-last-synced-main) and a new main-sync branch naming scheme.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # First main sync — use current HEAD of release branch as baseline | ||
| echo "No previous main sync tag found, using HEAD as baseline" | ||
| LAST_SYNCED="HEAD" |
There was a problem hiding this comment.
For the first main sync, LAST_SYNCED is set to the symbolic ref HEAD. After the workflow creates the sync branch and performs the merge, HEAD will point at a different commit (merge commit / conflicted state), so later ranges like git log "$LAST".."$LATEST" and git diff "$LAST".."$LATEST" will be incorrect (often empty). Capture the baseline as an immutable commit SHA (e.g., BASELINE_SHA=$(git rev-parse HEAD) before branch creation) and output/use that instead of HEAD.
| # First main sync — use current HEAD of release branch as baseline | |
| echo "No previous main sync tag found, using HEAD as baseline" | |
| LAST_SYNCED="HEAD" | |
| # First main sync — use current HEAD commit of release branch as immutable baseline | |
| LAST_SYNCED=$(git rev-parse HEAD) | |
| echo "No previous main sync tag found, using commit $LAST_SYNCED (current HEAD) as baseline" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/helio-upstream-sync.yml (1)
333-346:⚠️ Potential issue | 🔴 CriticalThe tracking tag is moved before the actual build completes, leaving broken syncs permanently marked as completed.
createWorkflowDispatchis fire-and-forget—it returns immediately when the API accepts the dispatch request, not when the downstreambuild_allworkflow finishes. As a result,steps.build.outcomeonly reflects whether the dispatch request succeeded, not whether the actual build succeeded. The tracking tag is moved at line 444 based on this incomplete check, and the failure path at line 448 never detects realbuild_allfailures sincesteps.build.outcomewill always besuccessfor a successful dispatch (even if the downstream build fails).To fix this, either:
- Capture the
workflow_run_idfrom the dispatch response (usingreturn_run_details: true), then poll the workflow run status until completion before moving the tag, or- Move tracking-tag updates to a separate workflow that monitors the actual
build_allcompletion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helio-upstream-sync.yml around lines 333 - 346, The dispatch call currently uses github.rest.actions.createWorkflowDispatch (in the build step) which only enqueues build_all and returns immediately so steps.build.outcome reflects dispatch success not build success; update the dispatch to request run details (use return_run_details: true) to obtain the workflow_run_id from the response (or use the equivalent output), then poll the run status via github.rest.actions.getWorkflowRun repeatedly until its status is completed and inspect run.conclusion for success before proceeding to any tracking-tag moves that currently rely on steps.build; alternatively, move the tracking-tag update logic into a separate workflow triggered on workflow_run completed for build_all and have that workflow verify run.conclusion == 'success' before moving the tag. Ensure all references to the build step (steps.build) and the build_all workflow are updated to use the real run conclusion rather than the dispatch outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/helio-upstream-sync.yml:
- Around line 129-131: The workflow sets LAST_SYNCED="HEAD" which captures the
literal string so after the subsequent merge the resolved HEAD points at the new
merge commit; change the assignment to capture the actual baseline SHA before
performing the merge by running git rev-parse HEAD and assigning that output to
LAST_SYNCED (so later git diff and git log invocations use the pre-merge commit
SHA). Locate the step that sets LAST_SYNCED and update it to run git rev-parse
HEAD prior to the merge step so the changelog and Helio reports compare against
the correct baseline SHA.
- Around line 80-87: Replace the local tag scan with a direct upstream query and
normalize tag refs to commit SHAs: query upstream tags with git ls-remote --tags
upstream and pick the latest v* ref to set LATEST_TAG/SYNC_REF (instead of git
tag -l), then compute SYNC_SHA by resolving the tag to its commit with
${LATEST_TAG}^{commit} (use the same commit-dereference form you use for
BASELINE_COMMIT) so comparisons later compare commit SHAs consistently; update
references to LATEST_TAG, SYNC_REF, and SYNC_SHA accordingly.
---
Outside diff comments:
In @.github/workflows/helio-upstream-sync.yml:
- Around line 333-346: The dispatch call currently uses
github.rest.actions.createWorkflowDispatch (in the build step) which only
enqueues build_all and returns immediately so steps.build.outcome reflects
dispatch success not build success; update the dispatch to request run details
(use return_run_details: true) to obtain the workflow_run_id from the response
(or use the equivalent output), then poll the run status via
github.rest.actions.getWorkflowRun repeatedly until its status is completed and
inspect run.conclusion for success before proceeding to any tracking-tag moves
that currently rely on steps.build; alternatively, move the tracking-tag update
logic into a separate workflow triggered on workflow_run completed for build_all
and have that workflow verify run.conclusion == 'success' before moving the tag.
Ensure all references to the build step (steps.build) and the build_all workflow
are updated to use the real run conclusion rather than the dispatch outcome.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afce13da-ac38-4893-9294-7094fc96d3bc
📒 Files selected for processing (1)
.github/workflows/helio-upstream-sync.yml
| # Find latest upstream v* tag | ||
| LATEST_TAG=$(git tag -l 'v*' --sort=-version:refname | head -1) | ||
| if [ -z "$LATEST_TAG" ]; then | ||
| echo "No upstream release tags found" | ||
| exit 1 | ||
| fi | ||
| SYNC_REF="$LATEST_TAG" | ||
| SYNC_SHA=$(git rev-parse "$LATEST_TAG") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/helio-upstream-sync.yml | sed -n '70,100p'Repository: Helio-Additive/OrcaSlicer
Length of output: 1624
🏁 Script executed:
cat -n .github/workflows/helio-upstream-sync.yml | sed -n '130,160p'Repository: Helio-Additive/OrcaSlicer
Length of output: 1479
🏁 Script executed:
cat -n .github/workflows/helio-upstream-sync.yml | sed -n '1,50p'Repository: Helio-Additive/OrcaSlicer
Length of output: 1631
🏁 Script executed:
cat -n .github/workflows/helio-upstream-sync.yml | sed -n '44,75p'Repository: Helio-Additive/OrcaSlicer
Length of output: 1397
🏁 Script executed:
cat -n .github/workflows/helio-upstream-sync.yml | sed -n '100,135p'Repository: Helio-Additive/OrcaSlicer
Length of output: 1778
🌐 Web query:
OrcaSlicer SoftFever releases annotated tags v* versions
💡 Result:
As of March 23, 2026, the v* version tags used for OrcaSlicer releases in the (renamed) GitHub repo OrcaSlicer/OrcaSlicer appear to be lightweight tags (not annotated tags):
- The GitHub Tags view lists each v* tag with a commit SHA and commit message, but does not show any “tagger” metadata or tag message that GitHub typically displays for annotated tags. Examples shown include v2.3.2-rc2, v2.3.2-rc, v2.3.1, v2.3.0, etc. [1]
- The corresponding Release pages show the release author/time and the target commit SHA, but likewise don’t show annotated-tag metadata. [2]
So: OrcaSlicer (SoftFever) release tags matching v* are not annotated; they’re lightweight tags pointing directly at commits. [1][2]
Sources:
[1] GitHub tags list (OrcaSlicer/OrcaSlicer) https://github.com/OrcaSlicer/OrcaSlicer/tags
[2] Example release page (v2.3.1) https://github.com/OrcaSlicer/OrcaSlicer/releases/tag/v2.3.1
🏁 Script executed:
# Check the upstream repo to see tag types
git ls-remote --tags https://github.com/SoftFever/OrcaSlicer.git | head -30Repository: Helio-Additive/OrcaSlicer
Length of output: 1917
Query upstream directly for the release tag and ensure commit-level consistency in comparisons.
The current code scans the local combined tag namespace (git tag -l) which includes both fork-side and upstream tags. Using git ls-remote --tags upstream queries upstream directly and avoids relying on locally fetched tag state.
Additionally, the code has an inconsistency: at line 136, BASELINE_COMMIT is explicitly dereferenced with ^{commit}, but SYNC_SHA at line 87 is not. When these are compared at line 142, this mismatch could cause false negatives if LATEST_TAG is ever an annotated tag (rather than a lightweight tag). Using ${LATEST_TAG}^{commit} ensures the comparison is always commit-to-commit.
🔧 Suggested change
- LATEST_TAG=$(git tag -l 'v*' --sort=-version:refname | head -1)
+ LATEST_TAG=$(
+ git ls-remote --tags --refs upstream 'v*' \
+ | cut -f2 \
+ | sed 's#refs/tags/##' \
+ | sort -V -r \
+ | head -1
+ )
if [ -z "$LATEST_TAG" ]; then
echo "No upstream release tags found"
exit 1
fi
SYNC_REF="$LATEST_TAG"
- SYNC_SHA=$(git rev-parse "$LATEST_TAG")
+ SYNC_SHA=$(git rev-parse "${LATEST_TAG}^{commit}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Find latest upstream v* tag | |
| LATEST_TAG=$(git tag -l 'v*' --sort=-version:refname | head -1) | |
| if [ -z "$LATEST_TAG" ]; then | |
| echo "No upstream release tags found" | |
| exit 1 | |
| fi | |
| SYNC_REF="$LATEST_TAG" | |
| SYNC_SHA=$(git rev-parse "$LATEST_TAG") | |
| # Find latest upstream v* tag | |
| LATEST_TAG=$( | |
| git ls-remote --tags --refs upstream 'v*' \ | |
| | cut -f2 \ | |
| | sed 's#refs/tags/##' \ | |
| | sort -V -r \ | |
| | head -1 | |
| ) | |
| if [ -z "$LATEST_TAG" ]; then | |
| echo "No upstream release tags found" | |
| exit 1 | |
| fi | |
| SYNC_REF="$LATEST_TAG" | |
| SYNC_SHA=$(git rev-parse "${LATEST_TAG}^{commit}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/helio-upstream-sync.yml around lines 80 - 87, Replace the
local tag scan with a direct upstream query and normalize tag refs to commit
SHAs: query upstream tags with git ls-remote --tags upstream and pick the latest
v* ref to set LATEST_TAG/SYNC_REF (instead of git tag -l), then compute SYNC_SHA
by resolving the tag to its commit with ${LATEST_TAG}^{commit} (use the same
commit-dereference form you use for BASELINE_COMMIT) so comparisons later
compare commit SHAs consistently; update references to LATEST_TAG, SYNC_REF, and
SYNC_SHA accordingly.
- Capture HEAD as a SHA before merge so changelog/diff ranges work correctly on first main sync (previously used literal "HEAD" which moves after merge) - Remove "Handle build failure" step — createWorkflowDispatch is fire-and-forget so steps.build.outcome only reflects the API call, not the actual build result. Build failures will surface via PR status checks instead. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| '1. Pull the sync branch locally:', | ||
| ' ```bash', | ||
| ` git fetch origin && git checkout ${branch}`, | ||
| ` git merge ${latest}`, | ||
| ` git merge ${syncRef}`, | ||
| ' ```', |
There was a problem hiding this comment.
The conflict-resolution instructions use git merge ${syncRef} where syncRef may be upstream/main. In a fresh local checkout, contributors typically won’t have an upstream remote configured (and tags like v* may also not exist locally), so these steps can fail. Consider updating the instructions to either (a) include adding/fetching the upstream remote, or (b) merge a concrete commit SHA (e.g., pass sync_sha and use that) so the instructions work reliably.
| # Move the tracking tag to the synced commit | ||
| # Note: build_all is fire-and-forget (dispatch only), so we track the | ||
| # merge itself. Build failures will show up on the PR via status checks. |
There was a problem hiding this comment.
The comment says the workflow is tracking “the merge itself”, but the tag is moved to SYNC_SHA (the upstream commit being merged), not the resulting merge commit on the sync branch. Either update the comment to reflect that the tag tracks the upstream target commit that was synced, or change the tag target (and corresponding skip logic) if the intent is to track the merge commit.
| # Move the tracking tag to the synced commit | |
| # Note: build_all is fire-and-forget (dispatch only), so we track the | |
| # merge itself. Build failures will show up on the PR via status checks. | |
| # Move the tracking tag to the synced upstream target commit | |
| # Note: build_all is fire-and-forget (dispatch only), so we track the | |
| # upstream commit that was synced. Build failures will show up on the PR via status checks. |
| - name: Update last-synced tag | ||
| if: steps.check.outputs.skip != 'true' && steps.merge.outputs.status == 'clean' && steps.build.outcome != 'failure' | ||
| - name: Update tracking tag | ||
| if: steps.check.outputs.skip != 'true' && steps.merge.outputs.status == 'clean' |
There was a problem hiding this comment.
Update tracking tag no longer checks whether the Trigger build check dispatch succeeded (previously gated on steps.build.outcome != 'failure'). Because continue-on-error: true is set, a dispatch/API failure will still allow this step to run, and the tracking tag will be advanced anyway—preventing subsequent runs from retrying the build dispatch for the same upstream target. Consider gating the tag update on successful dispatch (or reintroducing the build-failure issue/alert) so the workflow doesn’t mark a sync as complete when build triggering failed.
| if: steps.check.outputs.skip != 'true' && steps.merge.outputs.status == 'clean' | |
| if: steps.check.outputs.skip != 'true' && steps.merge.outputs.status == 'clean' && steps.build.outcome == 'success' |
…te tag update - Use concrete commit SHA instead of sync_ref in conflict resolution instructions so contributors don't need upstream remote configured - Fix comment: tracking tag tracks the upstream target commit, not the merge commit - Gate tracking tag update on steps.build.outcome == 'success' so a failed dispatch triggers a retry on next run Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Move the tracking tag to the upstream target commit that was synced. | ||
| # Note: build_all is fire-and-forget (dispatch only), so we track the | ||
| # upstream commit. Build failures will show up on the PR via status checks. | ||
| git tag -f "$TRACKING_TAG" "$SYNC_SHA" |
There was a problem hiding this comment.
This tags the tracking marker at SYNC_SHA, but in tag-sync mode SYNC_SHA may be the annotated tag object SHA (see earlier resolution). If that happens, later baseline logic that peels to commits can behave inconsistently. Prefer tagging the peeled commit SHA so the tracking tag always points to a commit object.
| # Move the tracking tag to the upstream target commit that was synced. | |
| # Note: build_all is fire-and-forget (dispatch only), so we track the | |
| # upstream commit. Build failures will show up on the PR via status checks. | |
| git tag -f "$TRACKING_TAG" "$SYNC_SHA" | |
| # Ensure we always tag a commit object, even if SYNC_SHA is an annotated tag. | |
| PEELED_SYNC_SHA="$(git rev-parse "${SYNC_SHA}^{commit}" 2>/dev/null || echo "$SYNC_SHA")" | |
| # Move the tracking tag to the upstream target commit that was synced. | |
| # Note: build_all is fire-and-forget (dispatch only), so we track the | |
| # upstream commit. Build failures will show up on the PR via status checks. | |
| git tag -f "$TRACKING_TAG" "$PEELED_SYNC_SHA" |
| `## Merge conflicts detected syncing ${syncLabel}`, | ||
| '', | ||
| `The automatic merge of upstream release \`${latest}\` into \`${target}\` produced conflicts.`, | ||
| `The automatic merge of upstream \`${syncRef}\` into \`${target}\` produced conflicts.`, |
There was a problem hiding this comment.
syncRef is referenced in the issue body but it’s never defined (no SYNC_REF env var and no const syncRef = ...). This will throw a ReferenceError and prevent the workflow from creating the merge-conflict issue. Define SYNC_REF in env: (e.g., from steps.target.outputs.sync_ref) and read it in the script, or change the message to use an existing variable like syncLabel/syncSha.
| `The automatic merge of upstream \`${syncRef}\` into \`${target}\` produced conflicts.`, | |
| `The automatic merge of upstream \`${syncLabel}\` into \`${target}\` produced conflicts.`, |
| SYNC_REF="$LATEST_TAG" | ||
| SYNC_SHA=$(git rev-parse "$LATEST_TAG") | ||
| SHORT_SHA=$(echo "$SYNC_SHA" | cut -c1-7) | ||
| SYNC_LABEL="$LATEST_TAG" | ||
| BRANCH_NAME="helio-release-candidate-${LATEST_TAG}" | ||
| TRACKING_TAG="helio-last-synced" |
There was a problem hiding this comment.
In tag-sync mode, SYNC_SHA=$(git rev-parse "$LATEST_TAG") may resolve to the tag object SHA for annotated tags rather than the underlying commit. That can break comparisons against peeled commits (e.g., baseline ^{commit}) and can cause the workflow to incorrectly think there’s something to sync. Consider resolving the commit explicitly (e.g., git rev-parse "$LATEST_TAG^{commit}") and keep SYNC_SHA consistently a commit SHA across modes.
- Resolve SYNC_SHA with ^{commit} so annotated tags always produce a
commit SHA, not a tag object SHA
- Fix ReferenceError: syncRef was undefined in the merge conflict
issue body — use syncLabel instead
Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/helio-upstream-sync.yml:
- Around line 77-90: The branch naming is reused across retries causing
non‑idempotent runs; modify the BRANCH_NAME (and analogous branch logic at the
other block) to include a unique attempt suffix (e.g.,
"${BRANCH_NAME}-${GITHUB_RUN_ATTEMPT}" or a timestamp/GITHUB_RUN_ID) or detect
an existing branch and update it instead of failing; ensure the logic that sets
BRANCH_NAME, SHORT_SHA and SYNC_LABEL (and the push/PR creation steps that
follow) uses that unique name or reuses the existing branch/PR when present, and
keep TRACKING_TAG handling the same so reruns won’t conflict with previously
created branches/PRs.
- Around line 305-309: The instructions currently ask contributors to fetch only
the branch before merging ${syncSha}, which can fail if the raw ${syncSha}
object isn't present on origin; update the sequence to fetch the upstream
commit/ref first (e.g., run git fetch origin ${syncSha} or git fetch origin
refs/heads/<upstream-ref>), then checkout ${branch} and merge the fetched commit
(or merge FETCH_HEAD), so replace the existing `git fetch origin && git checkout
${branch}` / `git merge ${syncSha}` steps with a fetch of ${syncSha} (or its
ref) followed by checkout of ${branch} and merging the fetched object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6bdc41c0-5634-4eae-9df3-cebf23794481
📒 Files selected for processing (1)
.github/workflows/helio-upstream-sync.yml
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SYNC_SHA=$(git rev-parse upstream/main) | ||
| SHORT_SHA=$(echo "$SYNC_SHA" | cut -c1-7) | ||
| SYNC_LABEL="main@${SHORT_SHA}" | ||
| BRANCH_NAME="helio-upstream-main-$(date '+%Y-%m-%d')-${SHORT_SHA}" |
There was a problem hiding this comment.
helio-upstream-main-<date>-<shortsha> can collide on reruns (same day + same upstream SHA), and the workflow later pushes the clean merge branch without --force-with-lease. If a prior run already pushed that branch but didn’t advance the tracking tag (e.g., dispatch failure), subsequent runs will likely fail with a non-fast-forward push. Consider making the branch name truly unique (include run_id/run_attempt) or switching to an idempotent approach (recreate/update the branch and force-push with lease).
| BRANCH_NAME="helio-upstream-main-$(date '+%Y-%m-%d')-${SHORT_SHA}" | |
| BRANCH_NAME="helio-upstream-main-$(date '+%Y-%m-%d')-${SHORT_SHA}-${{ github.run_id }}-${{ github.run_attempt }}" |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/helio-upstream-sync.yml (1)
305-309:⚠️ Potential issue | 🟡 MinorRaw SHA may not be available after fetching only the sync branch.
The resolution steps instruct contributors to run
git fetch origin && git checkout ${branch}followed bygit merge ${syncSha}. However, aftergit merge --abort, the pushed branch no longer contains the upstream commit object. If the contributor hasn't configured the upstream remote,${syncSha}won't be available fromorigin.Consider updating the instructions to either fetch upstream directly or use a ref that contains the target commit:
🔧 Suggested improvement to resolution steps
'### Resolution steps', - '1. Pull the sync branch locally and merge the upstream commit:', + '1. Pull the sync branch locally and fetch upstream:', ' ```bash', - ` git fetch origin && git checkout ${branch}`, - ` git merge ${syncSha}`, + ` git remote add upstream https://github.com/SoftFever/OrcaSlicer.git || true`, + ` git fetch upstream --tags`, + ` git fetch origin && git checkout ${branch}`, + ` git merge ${syncSha} # or: git merge upstream/main (for main syncs)`, ' ```',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helio-upstream-sync.yml around lines 305 - 309, Update the bash steps so the raw upstream SHA (${syncSha}) will be available when merging: add or ensure an upstream remote (e.g. run git remote add upstream ...) and fetch the upstream refs/tags (git fetch upstream --tags) before fetching the branch from origin and attempting git merge ${syncSha}; alternatively, offer using an upstream branch ref (e.g. git merge upstream/main) as a fallback if ${syncSha} is not present. Ensure the instructions mention adding the upstream remote, fetching upstream, then running git fetch origin && git checkout ${branch} followed by the merge step.
🧹 Nitpick comments (2)
.github/workflows/helio-upstream-sync.yml (2)
77-90: Tag-mode branch name is not unique across retries.The
mainmode includes date and SHA for uniqueness (helio-upstream-main-$(date '+%Y-%m-%d')-${SHORT_SHA}), butlatest-tagmode uses a fixed name based on the tag (helio-release-candidate-${LATEST_TAG}). If a workflow run fails after pushing the branch but before completing, a retry will conflict with the existing branch.Consider adding a unique suffix (e.g.,
${GITHUB_RUN_ID}or${GITHUB_RUN_ATTEMPT}) to the tag-mode branch name for consistency with the main-mode approach, or detect and update the existing branch.♻️ Suggested fix for unique branch naming
- BRANCH_NAME="helio-release-candidate-${LATEST_TAG}" + BRANCH_NAME="helio-release-candidate-${LATEST_TAG}-${GITHUB_RUN_ID}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helio-upstream-sync.yml around lines 77 - 90, The tag-mode branch name assignment (BRANCH_NAME="helio-release-candidate-${LATEST_TAG}") can collide across retries; change the BRANCH_NAME construction in the else branch (where LATEST_TAG, SYNC_REF, SYNC_SHA, SHORT_SHA, SYNC_LABEL are set) to append a unique suffix such as ${GITHUB_RUN_ID} or ${GITHUB_RUN_ATTEMPT} (or include date/SHORT_SHA) so the resulting branch name is unique per run/retry; update only the BRANCH_NAME assignment to incorporate that suffix and ensure any downstream use of BRANCH_NAME remains consistent.
80-87: Local tag scan may include fork-side tags.The
git tag -l 'v*'command scans the combined local tag namespace, which could include fork-specificv*tags alongside upstream tags fetched with--force. While this is unlikely to cause issues in practice (forks typically don't createv*tags), querying upstream directly would be more robust:♻️ Optional: Query upstream directly for release tags
- LATEST_TAG=$(git tag -l 'v*' --sort=-version:refname | head -1) + LATEST_TAG=$( + git ls-remote --tags --refs upstream 'v*' \ + | cut -f2 \ + | sed 's#refs/tags/##' \ + | sort -V -r \ + | head -1 + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helio-upstream-sync.yml around lines 80 - 87, Replace the local tag scan with a direct upstream query so LATEST_TAG is derived from upstream-only tags: use git ls-remote --tags upstream 'v*' (parse the ref names, sort by version, pick the highest) to set LATEST_TAG (instead of relying on git tag -l), and then obtain SYNC_SHA from the same upstream ls-remote result (or by querying the specific upstream ref) so SYNC_REF/SYNC_SHA reference the upstream tag unambiguously; update the code that currently sets LATEST_TAG, SYNC_REF, and SYNC_SHA to use this upstream ls-remote parsing approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/helio-upstream-sync.yml:
- Around line 305-309: Update the bash steps so the raw upstream SHA
(${syncSha}) will be available when merging: add or ensure an upstream remote
(e.g. run git remote add upstream ...) and fetch the upstream refs/tags (git
fetch upstream --tags) before fetching the branch from origin and attempting git
merge ${syncSha}; alternatively, offer using an upstream branch ref (e.g. git
merge upstream/main) as a fallback if ${syncSha} is not present. Ensure the
instructions mention adding the upstream remote, fetching upstream, then running
git fetch origin && git checkout ${branch} followed by the merge step.
---
Nitpick comments:
In @.github/workflows/helio-upstream-sync.yml:
- Around line 77-90: The tag-mode branch name assignment
(BRANCH_NAME="helio-release-candidate-${LATEST_TAG}") can collide across
retries; change the BRANCH_NAME construction in the else branch (where
LATEST_TAG, SYNC_REF, SYNC_SHA, SHORT_SHA, SYNC_LABEL are set) to append a
unique suffix such as ${GITHUB_RUN_ID} or ${GITHUB_RUN_ATTEMPT} (or include
date/SHORT_SHA) so the resulting branch name is unique per run/retry; update
only the BRANCH_NAME assignment to incorporate that suffix and ensure any
downstream use of BRANCH_NAME remains consistent.
- Around line 80-87: Replace the local tag scan with a direct upstream query so
LATEST_TAG is derived from upstream-only tags: use git ls-remote --tags upstream
'v*' (parse the ref names, sort by version, pick the highest) to set LATEST_TAG
(instead of relying on git tag -l), and then obtain SYNC_SHA from the same
upstream ls-remote result (or by querying the specific upstream ref) so
SYNC_REF/SYNC_SHA reference the upstream tag unambiguously; update the code that
currently sets LATEST_TAG, SYNC_REF, and SYNC_SHA to use this upstream ls-remote
parsing approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 637dd802-f1c7-4f67-ae56-5670d390f3ab
📒 Files selected for processing (1)
.github/workflows/helio-upstream-sync.yml
- Use --force-with-lease on clean merge push so retries overwrite the previous branch (same name) instead of failing - Handle "PR already exists" 422 gracefully on retry - Add upstream remote setup to conflict resolution instructions so the SHA is fetchable locally Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/helio-upstream-sync.yml (1)
415-443:⚠️ Potential issue | 🟡 MinorRemove dead code:
isNoDiff422is defined but never used.The variable
isNoDiff422(Lines 415-430) was superseded by the simplifiederrMsgsapproach (Lines 432-442), but the old code wasn't removed.🧹 Proposed cleanup
try { const { data: pr } = await github.rest.pulls.create({ owner: context.repo.owner, repo: context.repo.repo, head: branch, base: target, title: `Upstream sync: ${syncLabel}`, body: body }); core.setOutput('url', pr.html_url); console.log(`Created PR: ${pr.html_url}`); } catch (err) { - const isNoDiff422 = - err && err.status === 422 && - ( - (typeof err.message === 'string' && err.message.includes('No commits between')) || - (err.response && - err.response.data && - ( - (typeof err.response.data.message === 'string' && - err.response.data.message.includes('No commits between')) || - (Array.isArray(err.response.data.errors) && - err.response.data.errors.some(e => - e && typeof e.message === 'string' && - e.message.includes('No commits between') - )) - )) - ); - const errMsg = err.message || ''; const errData = err.response?.data; const errMsgs = [errMsg, errData?.message || '', ...(errData?.errors || []).map(e => e?.message || '')].join(' ');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helio-upstream-sync.yml around lines 415 - 443, Remove the dead variable isNoDiff422 from the error-handling block: locate the declaration of isNoDiff422 and delete that whole const (the logic computing 422/"No commits between") since the code now uses errMsgs and the subsequent if/else against errMsgs; ensure only errMsgs, err, target, and branch remain used for the PR-skip checks and that no other references to isNoDiff422 exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/helio-upstream-sync.yml:
- Around line 415-443: Remove the dead variable isNoDiff422 from the
error-handling block: locate the declaration of isNoDiff422 and delete that
whole const (the logic computing 422/"No commits between") since the code now
uses errMsgs and the subsequent if/else against errMsgs; ensure only errMsgs,
err, target, and branch remain used for the PR-skip checks and that no other
references to isNoDiff422 exist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5be91323-e2bd-449c-ac19-7e5e793114e7
📒 Files selected for processing (1)
.github/workflows/helio-upstream-sync.yml
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -376,64 +429,27 @@ jobs: | |||
| )) | |||
| ); | |||
|
|
|||
There was a problem hiding this comment.
In the PR creation error handler, isNoDiff422 is computed but never used. This is dead code and makes the control flow harder to follow—either remove it or use it to drive the 422 handling logic so there’s only one source of truth for the “No commits between” detection.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Summary
sync_targetinput to workflow_dispatch: choice oflatest-tag(default) ormainsync_ref,sync_label,branch_name,tracking_tag)helio-last-synced-main) so tag and main syncs don't interferehelio-upstream-main-{date}-{short-sha}Test plan
sync_target: main— should attempt merge of upstream/mainsync_target: latest-tag— should behave same as before (skip if already synced)main@abc1234vsv2.3.2-rc2)🤖 Generated with Claude Code
Summary by CodeRabbit