Skip to content

Commit edc2ac7

Browse files
committed
fix(workflows): critical and high priority workflow optimizations (#3228)
Implements critical and high priority fixes from issue #3228 workflow optimization report. ## Critical Fixes 1. **Race condition prevention in spec merging** - Added concurrency control to spec-create.yml merge job - Uses group 'spec-merge-main' with cancel-in-progress: false - Queues merge operations instead of running in parallel - Prevents git merge conflicts when multiple specs approved simultaneously 2. **Squash-merge file loss prevention** - Added pre-merge validation in impl-merge.yml - Checks both implementation and metadata files exist on PR branch - Blocks merge if implementation file missing (only metadata present) - Closes PR with descriptive error message if incomplete 3. **Atomic commits for implementation and metadata** - Modified impl-generate.yml to commit files together - Changed from separate metadata commit to single atomic commit - Verifies implementation file is staged before committing - Prevents partial PRs with metadata but no implementation ## High Priority Fixes 4. **Label propagation after manual merges** - Created new sync-labels.yml workflow - Automatically syncs labels when PRs merged manually - Handles both spec and impl label states - Adds spec-ready, impl:{library}:done labels when missing - Removes spec-request, generate:{library} labels when appropriate 5. **Early verdict labeling with retry logic** - Added preliminary verdict labeling in impl-review.yml - Labels added immediately after score extraction - Includes retry logic for failed label operations - Made later verdict step idempotent (checks existing labels) - Prevents PRs from being stuck without clear status ## Expected Impact - Eliminates race condition failures in spec merging - Prevents file loss from incomplete squash merges - Fixes label state after manual PR merges - Ensures verdict labels always set even if workflow times out - Reduces manual interventions from 12 to <3 per batch Related: #3228
1 parent 67266dc commit edc2ac7

5 files changed

Lines changed: 269 additions & 5 deletions

File tree

.github/workflows/impl-generate.yml

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -526,14 +526,27 @@ jobs:
526526
yaml.dump(data, f, default_flow_style=False, sort_keys=False, allow_unicode=True)
527527
"
528528
529-
# Commit and push
529+
# Commit and push (ATOMIC: both implementation AND metadata together)
530530
git config user.name "github-actions[bot]"
531531
git config user.email "github-actions[bot]@users.noreply.github.com"
532-
git add "$METADATA_FILE"
533-
git commit -m "chore(${LIBRARY}): add metadata for ${SPEC_ID}"
532+
533+
IMPL_FILE="plots/${SPEC_ID}/implementations/${LIBRARY}.py"
534+
535+
# Add both files in a single commit to prevent partial merges
536+
git add "$METADATA_FILE" "$IMPL_FILE"
537+
538+
# Verify both files are staged
539+
if ! git diff --cached --name-only | grep -q "${LIBRARY}.py"; then
540+
echo "::error::Implementation file not staged - cannot commit"
541+
echo "::error::This indicates the implementation file is missing or not modified"
542+
exit 1
543+
fi
544+
545+
git commit -m "feat(${LIBRARY}): implement ${SPEC_ID} with metadata"
534546
git push origin "$BRANCH"
535547
536548
echo "::notice::Created metadata file: $METADATA_FILE (py${PYTHON_VERSION}, ${LIBRARY}==${LIBRARY_VERSION})"
549+
echo "::notice::Committed implementation and metadata atomically"
537550
538551
# ========================================================================
539552
# Process images: optimize + thumbnail

.github/workflows/impl-merge.yml

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,40 @@ jobs:
8282
echo "specification_id=$SPEC_ID" >> $GITHUB_OUTPUT
8383
echo "library=$LIBRARY" >> $GITHUB_OUTPUT
8484
85+
- name: Validate PR completeness before merge
86+
if: steps.check.outputs.should_run == 'true'
87+
env:
88+
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
89+
SPEC_ID: ${{ steps.extract.outputs.specification_id }}
90+
LIBRARY: ${{ steps.extract.outputs.library }}
91+
BRANCH: ${{ steps.check.outputs.branch }}
92+
PR_NUM: ${{ steps.check.outputs.pr_number }}
93+
run: |
94+
# Fetch the PR branch to check its contents
95+
git fetch origin "$BRANCH"
96+
97+
IMPL_FILE="plots/${SPEC_ID}/implementations/${LIBRARY}.py"
98+
META_FILE="plots/${SPEC_ID}/metadata/${LIBRARY}.yaml"
99+
100+
# Check if implementation file exists on the PR branch
101+
if ! git show "origin/${BRANCH}:${IMPL_FILE}" &>/dev/null; then
102+
echo "::error::Implementation file missing on branch: ${IMPL_FILE}"
103+
echo "::error::This indicates an incomplete generation - metadata exists but implementation is missing"
104+
echo "::error::Closing PR to prevent partial merge"
105+
106+
gh pr close "$PR_NUM" --comment "**Merge blocked:** Implementation file \`${IMPL_FILE}\` is missing from this PR branch. Only metadata was found. This indicates an incomplete code generation. Please regenerate using \`generate:${LIBRARY}\` label."
107+
108+
exit 1
109+
fi
110+
111+
# Check if metadata file exists
112+
if ! git show "origin/${BRANCH}:${META_FILE}" &>/dev/null; then
113+
echo "::warning::Metadata file missing on branch: ${META_FILE}"
114+
echo "::warning::This is unusual but not blocking - metadata will be created on next review"
115+
fi
116+
117+
echo "::notice::PR completeness validated: both implementation and metadata present"
118+
85119
- name: Get parent issue from PR body
86120
if: steps.check.outputs.should_run == 'true'
87121
id: issue

.github/workflows/impl-review.yml

Lines changed: 61 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,44 @@ jobs:
294294
gh label create "$LABEL" --color "0e8a16" --description "Quality score ${SCORE}/100" 2>/dev/null || true
295295
gh pr edit "$PR_NUM" --add-label "$LABEL"
296296
297+
- name: Add preliminary verdict label (early)
298+
if: steps.review.conclusion == 'success' && steps.score.outputs.score != '0'
299+
env:
300+
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
301+
PR_NUM: ${{ steps.pr.outputs.pr_number }}
302+
SCORE: ${{ steps.score.outputs.score }}
303+
ATTEMPT_COUNT: ${{ steps.attempts.outputs.count }}
304+
run: |
305+
# Add verdict label early to ensure it's set even if later steps fail
306+
# This is idempotent - re-running won't cause issues
307+
308+
if [ "$SCORE" -ge 90 ]; then
309+
echo "::notice::Adding ai-approved label (score >= 90)"
310+
gh pr edit "$PR_NUM" --add-label "ai-approved" 2>/dev/null || {
311+
echo "::warning::Failed to add ai-approved label, retrying..."
312+
sleep 2
313+
gh pr edit "$PR_NUM" --add-label "ai-approved"
314+
}
315+
elif [ "$ATTEMPT_COUNT" -eq 3 ] && [ "$SCORE" -ge 50 ]; then
316+
echo "::notice::Adding ai-approved label (attempt 4, score >= 50)"
317+
gh pr edit "$PR_NUM" --add-label "ai-approved" 2>/dev/null || {
318+
echo "::warning::Failed to add ai-approved label, retrying..."
319+
sleep 2
320+
gh pr edit "$PR_NUM" --add-label "ai-approved"
321+
}
322+
else
323+
echo "::notice::Adding ai-rejected label (score < 90)"
324+
gh pr edit "$PR_NUM" --add-label "ai-rejected" 2>/dev/null || {
325+
echo "::warning::Failed to add ai-rejected label, retrying..."
326+
sleep 2
327+
gh pr edit "$PR_NUM" --add-label "ai-rejected"
328+
}
329+
330+
if [ "$SCORE" -lt 50 ]; then
331+
gh pr edit "$PR_NUM" --add-label "quality-poor" 2>/dev/null || true
332+
fi
333+
fi
334+
297335
- name: Install Python dependencies for metadata update
298336
if: steps.review.conclusion == 'success' && steps.score.outputs.score != '0'
299337
run: pip install pyyaml
@@ -512,9 +550,30 @@ jobs:
512550
# < 90: Rejected (repair loop, up to 3 attempts)
513551
# After 3 attempts: >= 50 = merge, < 50 = close PR
514552
553+
# Check if verdict label was already added by earlier step
554+
CURRENT_LABELS=$(gh pr view "$PR_NUM" --json labels -q '.labels[].name' 2>/dev/null || echo "")
555+
556+
if echo "$CURRENT_LABELS" | grep -q "ai-approved"; then
557+
echo "::notice::ai-approved label already set, triggering merge"
558+
gh workflow run impl-merge.yml -f pr_number="$PR_NUM"
559+
exit 0
560+
fi
561+
562+
if echo "$CURRENT_LABELS" | grep -q "ai-rejected"; then
563+
echo "::notice::ai-rejected label already set, triggering repair"
564+
gh pr edit "$PR_NUM" --add-label "ai-attempt-${ATTEMPT}" 2>/dev/null || true
565+
gh workflow run impl-repair.yml \
566+
-f pr_number="$PR_NUM" \
567+
-f specification_id="$SPEC_ID" \
568+
-f library="$LIBRARY" \
569+
-f attempt="$ATTEMPT"
570+
exit 0
571+
fi
572+
573+
# Fallback: Add verdict labels if not already set (shouldn't happen but ensures robustness)
515574
if [ "$SCORE" -ge 90 ]; then
516575
# Approved - merge immediately
517-
gh pr edit "$PR_NUM" --add-label "ai-approved"
576+
gh pr edit "$PR_NUM" --add-label "ai-approved" 2>/dev/null || true
518577
echo "::notice::Added ai-approved label (score $SCORE >= 90)"
519578
echo "Triggering impl-merge.yml for approved PR"
520579
gh workflow run impl-merge.yml -f pr_number="$PR_NUM"
@@ -523,7 +582,7 @@ jobs:
523582
# This is the 4th review (after 3 repair attempts)
524583
if [ "$SCORE" -ge 50 ]; then
525584
# Score 50-89 after 3 attempts: acceptable, merge to repo
526-
gh pr edit "$PR_NUM" --add-label "ai-approved"
585+
gh pr edit "$PR_NUM" --add-label "ai-approved" 2>/dev/null || true
527586
528587
gh pr comment "$PR_NUM" --body "## AI Review - Final Status
529588

.github/workflows/spec-create.yml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,11 @@ jobs:
363363
pull-requests: write
364364
issues: write
365365

366+
# Prevent race conditions when multiple specs are approved simultaneously
367+
concurrency:
368+
group: spec-merge-main
369+
cancel-in-progress: false # Queue merges instead of canceling
370+
366371
steps:
367372
- name: Checkout repository
368373
uses: actions/checkout@v6

.github/workflows/sync-labels.yml

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
name: "Sync: Labels"
2+
run-name: "Sync labels after push to main"
3+
4+
# Runs after any push to main to ensure labels match repository state
5+
# This fixes label propagation issues when PRs are merged manually
6+
on:
7+
push:
8+
branches:
9+
- main
10+
workflow_dispatch:
11+
12+
jobs:
13+
sync-spec-labels:
14+
runs-on: ubuntu-latest
15+
permissions:
16+
issues: write
17+
contents: read
18+
19+
steps:
20+
- name: Checkout repository
21+
uses: actions/checkout@v6
22+
with:
23+
fetch-depth: 2 # Need to see what changed
24+
25+
- name: Install yq for YAML parsing
26+
run: |
27+
sudo wget -qO /usr/local/bin/yq https://github.com/mikefarah/yq/releases/latest/download/yq_linux_amd64
28+
sudo chmod +x /usr/local/bin/yq
29+
30+
- name: Detect new specifications merged to main
31+
id: detect
32+
run: |
33+
# Get list of specifications added/modified in this push
34+
CHANGED_SPECS=$(git diff --name-only HEAD~1 HEAD | \
35+
grep -oP 'plots/\K[^/]+(?=/specification\.(md|yaml))' | \
36+
sort -u)
37+
38+
if [ -z "$CHANGED_SPECS" ]; then
39+
echo "::notice::No specification changes detected"
40+
echo "specs=" >> $GITHUB_OUTPUT
41+
exit 0
42+
fi
43+
44+
echo "specs<<EOF" >> $GITHUB_OUTPUT
45+
echo "$CHANGED_SPECS" >> $GITHUB_OUTPUT
46+
echo "EOF" >> $GITHUB_OUTPUT
47+
48+
- name: Sync labels for merged specifications
49+
if: steps.detect.outputs.specs != ''
50+
env:
51+
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
52+
run: |
53+
echo "${{ steps.detect.outputs.specs }}" | while read SPEC_ID; do
54+
[ -z "$SPEC_ID" ] && continue
55+
56+
# Extract issue number from specification.yaml
57+
SPEC_YAML="plots/${SPEC_ID}/specification.yaml"
58+
if [ ! -f "$SPEC_YAML" ]; then
59+
echo "::warning::No specification.yaml for ${SPEC_ID}"
60+
continue
61+
fi
62+
63+
ISSUE=$(yq '.issue' "$SPEC_YAML" 2>/dev/null || echo "")
64+
if [ -z "$ISSUE" ] || [ "$ISSUE" == "null" ]; then
65+
echo "::warning::No issue number in ${SPEC_YAML}"
66+
continue
67+
fi
68+
69+
echo "::notice::Syncing labels for spec ${SPEC_ID} (issue #${ISSUE})"
70+
71+
# Get current labels
72+
CURRENT_LABELS=$(gh issue view "$ISSUE" --json labels -q '.labels[].name' 2>/dev/null || echo "")
73+
74+
# If specification is in main, ensure correct labels
75+
if echo "$CURRENT_LABELS" | grep -q "spec-request"; then
76+
echo "::notice::Removing spec-request label from #${ISSUE}"
77+
gh issue edit "$ISSUE" --remove-label "spec-request" 2>/dev/null || true
78+
fi
79+
80+
if ! echo "$CURRENT_LABELS" | grep -q "spec-ready"; then
81+
echo "::notice::Adding spec-ready label to #${ISSUE}"
82+
gh issue edit "$ISSUE" --add-label "spec-ready" 2>/dev/null || true
83+
fi
84+
done
85+
86+
sync-impl-labels:
87+
runs-on: ubuntu-latest
88+
permissions:
89+
issues: write
90+
contents: read
91+
92+
steps:
93+
- name: Checkout repository
94+
uses: actions/checkout@v6
95+
with:
96+
fetch-depth: 2
97+
98+
- name: Install yq for YAML parsing
99+
run: |
100+
sudo wget -qO /usr/local/bin/yq https://github.com/mikefarah/yq/releases/latest/download/yq_linux_amd64
101+
sudo chmod +x /usr/local/bin/yq
102+
103+
- name: Detect new implementations merged to main
104+
id: detect
105+
run: |
106+
# Get list of implementations added/modified in this push
107+
CHANGED_IMPLS=$(git diff --name-only HEAD~1 HEAD | \
108+
grep -P 'plots/[^/]+/implementations/[^/]+\.py$')
109+
110+
if [ -z "$CHANGED_IMPLS" ]; then
111+
echo "::notice::No implementation changes detected"
112+
echo "impls=" >> $GITHUB_OUTPUT
113+
exit 0
114+
fi
115+
116+
echo "impls<<EOF" >> $GITHUB_OUTPUT
117+
echo "$CHANGED_IMPLS" >> $GITHUB_OUTPUT
118+
echo "EOF" >> $GITHUB_OUTPUT
119+
120+
- name: Sync labels for merged implementations
121+
if: steps.detect.outputs.impls != ''
122+
env:
123+
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
124+
run: |
125+
echo "${{ steps.detect.outputs.impls }}" | while read IMPL_PATH; do
126+
[ -z "$IMPL_PATH" ] && continue
127+
128+
# Extract spec-id and library from path: plots/{spec-id}/implementations/{library}.py
129+
SPEC_ID=$(echo "$IMPL_PATH" | cut -d'/' -f2)
130+
LIBRARY=$(echo "$IMPL_PATH" | cut -d'/' -f4 | sed 's/\.py$//')
131+
132+
# Get issue number
133+
SPEC_YAML="plots/${SPEC_ID}/specification.yaml"
134+
ISSUE=$(yq '.issue' "$SPEC_YAML" 2>/dev/null || echo "")
135+
136+
if [ -z "$ISSUE" ] || [ "$ISSUE" == "null" ]; then
137+
echo "::warning::No issue for ${SPEC_ID}/${LIBRARY}"
138+
continue
139+
fi
140+
141+
echo "::notice::Syncing labels for ${SPEC_ID}/${LIBRARY} (issue #${ISSUE})"
142+
143+
# Remove trigger and pending labels
144+
gh issue edit "$ISSUE" --remove-label "generate:${LIBRARY}" 2>/dev/null || true
145+
gh issue edit "$ISSUE" --remove-label "impl:${LIBRARY}:pending" 2>/dev/null || true
146+
147+
# Create and add done label
148+
gh label create "impl:${LIBRARY}:done" --color "0e8a16" \
149+
--description "${LIBRARY} implementation merged" 2>/dev/null || true
150+
gh issue edit "$ISSUE" --add-label "impl:${LIBRARY}:done" 2>/dev/null || true
151+
152+
echo "::notice::✓ Labels synced for ${SPEC_ID}/${LIBRARY}"
153+
done

0 commit comments

Comments
 (0)