Add PR-level stale lock file guard for workflow markdown edits#47238
Conversation
- Add .github/workflows/stale-lock-files.yml: fast PR CI gate that runs check-stale-lock-files.sh on PRs touching .github/workflows/*.md without needing to build the binary, with step summary remediation - Add 3 new edge-case tests to check-stale-lock-files_test.sh: shared/ exclusion, skills/ exclusion, and unknown --base-ref graceful fallback - Add test-scripts Makefile target that runs both check-stale-lock-files and check-workflow-drift shell script test suites; wire into test-all Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds PR-level validation to catch workflow markdown changes without updated lock files.
Changes:
- Adds a stale-lock GitHub Actions check.
- Expands shell-script regression coverage.
- Integrates script tests into
test-all.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/stale-lock-files.yml |
Adds the PR validation workflow. |
scripts/check-stale-lock-files_test.sh |
Tests exclusions and base-ref fallback. |
Makefile |
Adds script tests to the standard suite. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
.github/workflows/stale-lock-files.yml:35
- The new PR gate still passes when a workflow source is deleted but its generated lock file is left behind.
check-stale-lock-files.shincludes the deleted.mdin the diff and then skips it because it no longer exists (scripts/check-stale-lock-files.sh:112), so deletingfoo.mdwithout deletingfoo.lock.ymlproduces a green check even thoughmake recompile --purgewould remove the orphan. Handle deleted sources by requiring the corresponding lock deletion, and add a deletion regression case.
bash scripts/check-stale-lock-files.sh --base-ref "origin/${{ github.base_ref }}"
.github/workflows/stale-lock-files.yml:38
- This summary runs after any earlier failure, including checkout or base-branch fetch failures, and incorrectly reports those infrastructure errors as stale lock files. Give the check step an
idand condition the summary on that step's failed outcome, matching the targeted reporting pattern used in.github/workflows/cgo.yml:159and.github/workflows/ci.yml:253.
if: failure()
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Medium
| - '.github/workflows/*.md' | ||
| - 'scripts/check-stale-lock-files.sh' | ||
| - '.github/workflows/stale-lock-files.yml' | ||
| workflow_dispatch: |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47238 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES — two correctness bugs must be fixed before merge.
### Blocking issues
1. workflow_dispatch is always a no-op (stale-lock-files.yml lines 10, 40)
github.base_ref is empty on manual dispatches. The Fetch base branch step is guarded (if: github.event_name == 'pull_request') but the Check stale lock files step runs unconditionally, passing origin/ to the script. The script then falls back to a working-tree diff on a clean checkout and always exits 0. Either gate the check step the same way, or add a workflow_dispatch input for base_ref. (Noted in existing comment at line 10 and new comment at line 40.)
2. Test script not in paths filter (stale-lock-files.yml lines 12–16)
scripts/check-stale-lock-files_test.sh is missing from the paths trigger, so a PR that only modifies the test file (e.g., removes a failing assertion) bypasses this CI gate entirely.
### Non-blocking
test-scriptsMakefile target (Makefileline 72): runscheck-workflow-drift_test.sh ./$(BINARY_NAME)with nobuildprerequisite — fails confusingly in a clean checkout with a missing binary error that obscures the stale-lock-file tests.- Test 13 coupling: tests an undocumented fallback behavior; if the script is ever hardened to reject invalid refs, this test breaks non-obviously.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 39 AIC · ⌖ 4.98 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
.github/workflows/stale-lock-files.yml:14
Paths filter omits the test script: changes to scripts/check-stale-lock-files_test.sh will not trigger this workflow, so a broken test file can be merged without any CI validation.
<details>
<summary>💡 Suggested fix</summary>
Add the test script to the paths filter:
paths:
- ".github/workflows/*.md"
- "scripts/check-stale-lock-files.sh"
- "scripts/check-stale-lock-files_test.sh"
- ".github/workflows/stale-lock-files.yml"Without this, a PR that only modifies `ch…
.github/workflows/stale-lock-files.yml:40
Check stale lock files step runs unconditionally with an empty base-ref on workflow_dispatch: github.base_ref is empty for manual dispatches, so this step passes origin/ to the script. The Fetch base branch step is already guarded with if: github.event_name == 'pull_request', but this step has no guard, causing the script to silently fall back to a working-tree diff on a clean checkout and always exit 0.
<details>
<summary>💡 Suggested fix</summary>
Either add the same guard:
…
Makefile:72
test-scripts hard-depends on the compiled binary without building it first: make test-scripts runs check-workflow-drift_test.sh ./$(BINARY_NAME), but there is no $(BINARY_NAME) prerequisite — if the binary is missing or stale the target silently passes its own echo and then fails inside the test script with an unhelpful error about a missing executable.
<details>
<summary>💡 Suggested fix</summary>
Either add a binary prerequisite:
test-scripts: buildOr docume…
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two issues.
📋 Key Themes & Highlights
Issues
- Silent no-op on
workflow_dispatch:github.base_refis empty for manual runs, so--base-ref "origin/"is passed to the script. The fallback on a clean checkout finds no working-tree changes and exits 0 — the guard does nothing. Fix or removeworkflow_dispatch. - Missing
buildprerequisite intest-scripts:check-workflow-drift_test.shrequires the binary, buttest-scriptshas no dependency onbuild.make test-scriptson a clean checkout will fail.
Positive Highlights
- ✅ Clean CI gate design: paths filter ensures the workflow only runs when relevant files change
- ✅ Three well-structured test cases covering real edge cases (shared/, skills/ exclusions, unknown base-ref fallback)
- ✅ Clear, actionable step summary on failure pointing to
make recompile - ✅ Good use of
fetch-depth: 0+ explicit fetch to make base-ref comparison reliable
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 30.9 AIC · ⌖ 4.78 AIC · ⊞ 6.7K
Comment /matt to run again
| if: github.event_name == 'pull_request' | ||
| run: git fetch origin ${{ github.base_ref }} --no-tags | ||
|
|
||
| - name: Check stale lock files |
There was a problem hiding this comment.
[/diagnosing-bugs] workflow_dispatch does not populate github.base_ref, so the script receives --base-ref "origin/" (empty ref). The fallback on a clean checkout finds no working-tree changes and silently exits 0 — the guard is a no-op for manual runs.
💡 Suggested fix
Guard the base-ref flag on whether the event is a pull request:
- name: Check stale lock files
run: |
if [ -n "${{ github.base_ref }}" ]; then
bash scripts/check-stale-lock-files.sh --base-ref "origin/${{ github.base_ref }}"
else
bash scripts/check-stale-lock-files.sh
fiOr simply remove workflow_dispatch from the triggers if a meaningful manual run is not needed.
@copilot please address this.
| test-scripts: | ||
| @echo "Running Bash script tests..." | ||
| bash scripts/check-stale-lock-files_test.sh | ||
| bash scripts/check-workflow-drift_test.sh ./$(BINARY_NAME) |
There was a problem hiding this comment.
[/tdd] test-scripts runs check-workflow-drift_test.sh which requires the binary ./$(BINARY_NAME) to already exist, but test-scripts declares no dependency on build. Running make test-scripts on a clean checkout will fail with a missing binary error.
💡 Suggested fix
Add build as a prerequisite:
.PHONY: test-scripts
test-scripts: build
\techo "Running Bash script tests..."
\tbash scripts/check-stale-lock-files_test.sh
\tbash scripts/check-workflow-drift_test.sh ./$(BINARY_NAME)
\t@echo "✓ All Bash script tests passed"check-stale-lock-files_test.sh does not need the binary, so the cost is only incurred when the drift test actually runs.
@copilot please address this.
There was a problem hiding this comment.
Review: Add PR-level stale lock file guard
The PR is well-structured and closes a real gap. Two non-blocking issues found:
-
workflow_dispatchemptiesgithub.base_ref(existing comment, line 10): the check step receives--base-ref "origin/", the script cannot resolve it, falls back to a clean working-tree check, and always passes on manual runs. Either removeworkflow_dispatchor addif: github.event_name == "pull_request"on the affected steps. -
test-scriptsMakefile target has nobuildprerequisite (inline comment, Makefile):check-workflow-drift_test.shrequires the compiled binary but Make will not build it first, causing a confusing failure.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 32.7 AIC · ⌖ 4.55 AIC · ⊞ 5K
| # Run Bash script tests (check-stale-lock-files, check-workflow-drift) | ||
| .PHONY: test-scripts | ||
| test-scripts: | ||
| @echo "Running Bash script tests..." |
There was a problem hiding this comment.
The test-scripts target passes ./$(BINARY_NAME) to check-workflow-drift_test.sh but declares no build prerequisite. If the binary doesn't exist, the test fails with a misleading "No such file or directory" error.
Fix by adding the build prerequisite:
test-scripts: build@copilot please address this.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — I ran the PR finisher flow and pushed |
|
🎉 This pull request is included in a new release. Release: |
Workflow
.mdedits were repeatedly merging without regenerated.lock.ymloutputs because the existingcheck-stale-lock-filesguard only ran on pushes tomain— never on PRs.New CI gate:
.github/workflows/stale-lock-files.ymlTriggers on
pull_requestwhen.github/workflows/*.mdchanges. Runsscripts/check-stale-lock-files.shagainst the PR base ref — no binary build required, fails fast with a step summary pointing atmake recompile.Extended shell script tests
Three new cases in
check-stale-lock-files_test.sh(tests 11–13):shared/subdirectory.mdfiles are excluded (no lock expected)skills/subdirectory.mdfiles are excluded--base-reffalls back to working-tree diff and still catches stale filestest-scriptsMakefile targetWires
check-stale-lock-files_test.shandcheck-workflow-drift_test.shintotest-allso script tests are no longer orphaned from the standard test suite.