fix(workflows): critical and high priority workflow optimizations (#3228)#3339
Merged
MarkusNeusinger merged 3 commits intomainfrom Jan 8, 2026
Merged
fix(workflows): critical and high priority workflow optimizations (#3228)#3339MarkusNeusinger merged 3 commits intomainfrom
MarkusNeusinger merged 3 commits intomainfrom
Conversation
) 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
## Duplicate Detection (Medium Priority Fix) Added automatic duplicate detection in spec-create.yml: - Validates that specification directory was actually created - Checks Claude's comment for duplicate indicators - Auto-closes issue with 'duplicate' label if duplicate detected - Removes 'spec-request' label to prevent confusion - Provides clear error message if creation failed for other reasons This prevents duplicate issues from staying open with stale labels. ## Documentation Updates (CLAUDE.md) Updated workflow documentation to reflect recent improvements: 1. **Supporting Workflows** - Added sync-labels.yml to the list - Documents automatic label synchronization after manual merges 2. **Workflow Data Flow** - Enhanced Flow A (spec creation) - Added duplicate detection validation step - Shows when issues are auto-closed for duplicates 3. **Workflow Data Flow** - Enhanced Flow B (implementation) - Documents atomic commits (implementation + metadata together) - Shows PR completeness validation in impl-merge.yml - Prevents partial merges with missing implementation files 4. **Decoupled Architecture** - Added benefits - Labels auto-synced after manual PR merges - Spec merge operations serialized to prevent race conditions All workflow improvements from #3228 are now fully documented. Related: #3228
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements critical workflow optimizations to improve reliability and reduce manual interventions for the pyplots.ai automated plot generation system. The changes target race conditions, file loss prevention, label synchronization, and early error detection.
Key Changes:
- Adds concurrency control to specification merges to prevent race conditions
- Implements PR completeness validation before merge to catch partial generations
- Introduces automatic label synchronization after manual PR merges via new
sync-labels.ymlworkflow - Adds early verdict labeling with retry logic to prevent stuck PRs
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/sync-labels.yml |
New workflow to auto-sync labels (spec-ready, impl:*:done) after manual PR merges by detecting file changes |
.github/workflows/spec-create.yml |
Adds concurrency control (spec-merge-main group) to serialize merges and duplicate detection logic to auto-close duplicate specs |
.github/workflows/impl-review.yml |
Implements early verdict labeling (ai-approved/ai-rejected) with retry logic, checks for existing labels before re-adding |
.github/workflows/impl-merge.yml |
Adds pre-merge validation to ensure both implementation and metadata files exist on PR branch before merge |
.github/workflows/impl-generate.yml |
Attempts to implement atomic commits by staging both implementation and metadata together (has critical bug) |
CLAUDE.md |
Documents all workflow improvements including duplicate detection, atomic commits, completeness validation, and label sync |
| id: detect | ||
| run: | | ||
| # Get list of specifications added/modified in this push | ||
| CHANGED_SPECS=$(git diff --name-only HEAD~1 HEAD | \ |
There was a problem hiding this comment.
The use of git diff --name-only HEAD~1 HEAD may fail in edge cases such as the first commit to main or after force pushes/rebases. Consider adding error handling for when HEAD~1 doesn't exist. A safer approach would be to use git diff --name-only ${{ github.event.before }} ${{ github.event.after }} which GitHub provides in the push event context.
Suggested change
| CHANGED_SPECS=$(git diff --name-only HEAD~1 HEAD | \ | |
| CHANGED_SPECS=$(git diff --name-only "${{ github.event.before }}" "${{ github.event.after }}" | \ |
Comment on lines
+107
to
+109
| CHANGED_IMPLS=$(git diff --name-only HEAD~1 HEAD | \ | ||
| grep -P 'plots/[^/]+/implementations/[^/]+\.py$') | ||
|
|
There was a problem hiding this comment.
The use of git diff --name-only HEAD~1 HEAD may fail in edge cases such as the first commit to main or after force pushes/rebases. Consider adding error handling for when HEAD~1 doesn't exist. A safer approach would be to use git diff --name-only ${{ github.event.before }} ${{ github.event.after }} which GitHub provides in the push event context.
Suggested change
| CHANGED_IMPLS=$(git diff --name-only HEAD~1 HEAD | \ | |
| grep -P 'plots/[^/]+/implementations/[^/]+\.py$') | |
| BEFORE="${{ github.event.before }}" | |
| AFTER="${{ github.event.after }}" | |
| if [ "$BEFORE" = "0000000000000000000000000000000000000000" ]; then | |
| # First push: diff-tree on the after commit only | |
| CHANGED_IMPLS=$(git diff-tree --no-commit-id --name-only -r "$AFTER" | \ | |
| grep -P 'plots/[^/]+/implementations/[^/]+\.py$' || true) | |
| else | |
| # Normal push: diff between before and after SHAs | |
| CHANGED_IMPLS=$(git diff --name-only "$BEFORE" "$AFTER" | \ | |
| grep -P 'plots/[^/]+/implementations/[^/]+\.py$' || true) | |
| fi |
Comment on lines
+538
to
+544
| # Verify both files are staged | ||
| if ! git diff --cached --name-only | grep -q "${LIBRARY}.py"; then | ||
| echo "::error::Implementation file not staged - cannot commit" | ||
| echo "::error::This indicates the implementation file is missing or not modified" | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
The verification logic at line 539 checks if the implementation file is staged, but this will always fail because Claude already committed and pushed that file earlier in the workflow. When you run git add on a file that's already committed and unchanged, Git doesn't stage it. This means the error "Implementation file not staged - cannot commit" will always be triggered, blocking all implementation workflows.
The current approach doesn't achieve atomic commits since there are two separate commits. If the goal is to have both files in the same commit, Claude should NOT be instructed to commit/push the implementation file (remove lines 397-404 from the prompt), and let this workflow step create the first and only commit with both files.
Suggested change
| # Verify both files are staged | |
| if ! git diff --cached --name-only | grep -q "${LIBRARY}.py"; then | |
| echo "::error::Implementation file not staged - cannot commit" | |
| echo "::error::This indicates the implementation file is missing or not modified" | |
| exit 1 | |
| fi | |
| # Verify implementation file exists in the repository | |
| if ! git ls-files --error-unmatch "$IMPL_FILE" >/dev/null 2>&1; then | |
| echo "::error::Implementation file not found in repository - cannot commit" | |
| echo "::error::Expected implementation at: $IMPL_FILE" | |
| exit 1 | |
| fi | |
| # Verify metadata file is staged (implementation may already be committed) | |
| if ! git diff --cached --name-only | grep -q "$METADATA_FILE"; then | |
| echo "::error::Metadata file not staged - cannot commit" | |
| echo "::error::This indicates the metadata file was not added correctly" | |
| exit 1 | |
| fi |
Fixes 3 issues identified by Copilot code review:
1. **sync-labels.yml (spec detection)**: Use GitHub event context
- Changed from `git diff HEAD~1 HEAD` to `${{ github.event.before/after }}`
- Handles edge cases: first commit, force pushes, rebases
- Added fallback for initial push (SHA = 0000...)
2. **sync-labels.yml (impl detection)**: Same fix applied
- Uses GitHub event context for reliability
- Prevents failures on edge cases
3. **impl-generate.yml (critical bug)**: Fixed file verification logic
- Claude already commits implementation file earlier in workflow
- Changed verification from "is file staged?" to "does file exist?"
- Uses `git ls-files --error-unmatch` to verify implementation
- Only stages and verifies metadata file (which we create)
- Prevents false "not staged" errors that would block all workflows
Updated CLAUDE.md to reflect the corrected workflow behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements all critical, high, and medium priority fixes from issue #3228 workflow optimization report.
sync-labels.ymlworkflow for automatic label fixing after manual mergesTest plan
approvedlabel to all - verify merges are queued (no race conditions)sync-labels.ymladdsspec-readylabelsync-labels.ymladdsimpl:{library}:donelabelduplicatelabelChanges
spec-create.ymlimpl-merge.ymlimpl-generate.ymlimpl-review.ymlsync-labels.ymlCLAUDE.mdExpected Impact
Closes #3228
🤖 Generated with Claude Code