Conversation
📊 Test Coverage Report
Coverage measured by |
dev01lay2
left a comment
dev01lay2
left a comment
There was a problem hiding this comment.
PR #110 — feat: automate rc draft release flow
CI: rust/frontend/coverage/profile-e2e pass ✅; build matrix pending (pre-existing infra).
The rc-branch-driven automation is the right UX direction — cuts out the manual "pick bump type" step and anchors prerelease/release classification in source-branch ancestry rather than version string heuristics. The semver comparison Node.js script is solid. validate_release_state gate in release.yml correctly prevents overwriting published releases while allowing draft refreshes. Idempotent commit step (exit 0 on no-diff) is correct — subsequent steps in the job still run.
🔴 BS — Double-trigger on rc branch creation
on.create and on.push.branches: rc/v* both fire when a new rc branch is pushed. GitHub emits a create event and a push event for the same ref when a branch is created with commits (which is always the case for an rc cut from main/develop). With cancel-in-progress: true, one of the two concurrent runs — non-deterministically — gets killed. The release may silently fail on the very first trigger, with no explicit error surfaced to the user.
Fix (option A — simplest): Drop the create trigger entirely; the push trigger already fires on branch creation. Add a paths-ignore-exempt guard if needed:
on:
push:
branches:
- 'rc/v*'
paths-ignore:
- package.json
- src-tauri/Cargo.toml
- src-tauri/Cargo.lock
workflow_dispatch:
...Remove the create block and the if: ${{ github.event_name != 'create' || ... }} guard on prepare (the branch filter already ensures only rc/v* triggers it).
Fix (option B — keep both triggers): Add a job-level guard to skip the push that coincides with a branch creation:
prepare:
if: >-
github.event_name == 'workflow_dispatch' ||
github.event_name == 'create' ||
(github.event_name == 'push' && !github.event.created)🟡 NBS ① — cancel-in-progress: true can orphan a release trigger
If a second push lands on the rc branch while commit_push is running (but not yet at the Trigger Release workflow step), the in-flight run gets cancelled after the version commit is pushed but before release.yml is dispatched. This leaves a version bump without a corresponding draft release update, with no notification.
Consider cancel-in-progress: false to let the in-flight run finish, or restructure to dispatch the release trigger in a separate job so the concurrency boundary is tighter.
🟡 NBS ② — Misleading error when main and develop share HEAD
if [ "$MAIN_BASE" = "$DEVELOP_BASE" ]; then
echo "::error::Could not distinguish whether $REF_NAME was created from main or develop"
exit 1
fiThe actual cause here is that main and develop have identical HEADs (haven't diverged), making the merge-base tie-break useless. The current message ("could not distinguish") reads as a logic bug rather than a repo state issue. Suggest:
echo "::error::main and develop share the same HEAD commit; ancestry check is ambiguous. Ensure develop has diverged from main before creating an rc branch."
📦 PR Build Artifacts
|
dev01lay2
left a comment
dev01lay2
left a comment
There was a problem hiding this comment.
Re-review — new commit 0a991c54
CI: all checks pending at review time (new push).
New commit (test: replace token-like literals with synthetic constants in e2e fixture) is a clean, unrelated test hygiene fix — extracting TEST_ANTHROPIC_KEY / TEST_OPENAI_KEY constants and wiring them through build_image(). Good housekeeping, no concerns.
Workflow files unchanged — prior BS still open:
🔴 Double-trigger on rc branch creation (on.create + on.push.branches: rc/v* both fire for the same branch push). Needs one of the two fixes from the previous review before this can merge.
Keith-CY
force-pushed
the
feat/rc-create-release-automation
branch
from
ab46c6e to
311da28
Compare
# Conflicts: # src-tauri/src/commands/doctor.rs
dev01lay2
left a comment
dev01lay2
left a comment
There was a problem hiding this comment.
Re-review — new commit e56fa9c8
CI: rust check failing; frontend/coverage/profile-e2e pass ✅; build matrix pending.
✅ Prior BS resolved
The double-trigger (on.create + on.push.branches) is gone — the new commit drops the create trigger entirely and uses only the push: branches: rc/v* path. The if: github.event_name != 'create' guard on prepare is also removed, which is correct. Fix (Option A) applied cleanly. 👍
The rest of the workflow refactor looks solid: the package.json / Cargo.toml version consistency check is a good addition, the semver comparison Node.js script is correct, ref_name is properly propagated through needs.prepare.outputs.ref_name in every downstream step, and the idempotent commit path (committed=false, exit 0) is correct.
🔴 BS — cargo fmt --check still failing
src-tauri/src/commands/doctor.rs has formatting violations — rustfmt wants to reformat the classify_connection_quality test assertions and the pick_bottleneck_stage call. Run:
cargo fmt --all
git add -u
git commit -m 'style: cargo fmt'
git pushSame issue as before; same fix.
2 participants
Summary
Verification