feat: add CI drift check for workflow markdown vs generated lock files#43747
Conversation
- Add scripts/check-workflow-drift.sh: detects when .md sources and .lock.yml files are out of sync by recompiling and checking git diff. Exits 1 with a clear "run make recompile" message when drift is found. - Add check-workflow-drift Makefile target (depends on build) - Wire check-workflow-drift into agent-report-progress so agents fail before committing if lock files are stale - Add check-workflow-drift CI job to ci.yml (needs: changes + update) - Extend ci.yml push paths to include .github/workflows/*.md changes - Update Makefile help text Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Avoids spurious exit-1 when set -e + pipefail kills the subshell because `[ -n "$v" ]` returns 1 for empty drift variables. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…vars Both behaviours are intentional and tested; comments prevent future confusion for reviewers unfamiliar with git's own glob pathspec syntax. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43747 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ 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. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds a workflow drift guardrail to detect when .github/workflows/*.md sources and their generated .lock.yml outputs are out of sync, so contributors/agents are prompted to regenerate lock files before committing.
Changes:
- Added
scripts/check-workflow-drift.shto recompile workflows and fail if any.github/workflows/*.lock.ymlwould be modified/added/deleted. - Added
make check-workflow-driftand wired it intoagent-report-progressas a pre-PR gate. - Added a new CI job
check-workflow-driftand ensured the CI workflow triggers when.github/workflows/*.mdchanges.
Show a summary per file
| File | Description |
|---|---|
| scripts/check-workflow-drift.sh | New drift-check script that recompiles and detects changed/untracked/deleted lock files. |
| Makefile | Adds check-workflow-drift target and includes it in agent-report-progress + help output. |
| .github/workflows/ci.yml | Adds a CI job to run the drift check and triggers CI on workflow markdown changes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Low
| .PHONY: check-workflow-drift | ||
| check-workflow-drift: build | ||
| @bash scripts/check-workflow-drift.sh ./$(BINARY_NAME) |
| BINARY="${1:-./gh-aw}" | ||
|
|
||
| echo "Checking for workflow markdown/lock file drift..." | ||
| echo "" | ||
|
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on a blocking CI correctness issue and several design/test gaps.
📋 Key Themes & Highlights
Blocking issue
- Missing Go toolchain in CI job (
ci.yml:439):make check-workflow-driftinvokesbuild: go build ..., but the new job has nosetup-gostep. The job will fail every time unless the binary invocation is decoupled from the Makefile'sbuildprerequisite.
Design issues
- Working-tree mutation:
gh aw compilewrites lock files in place; any CI step that runs after this (or a concurrent job sharing the same checkout) will observe unexpected state. - Binary default silently duplicates
BINARY_NAME: the./gh-awfallback can diverge from the Makefile variable. - Remediation message recommends a raw
git committhat bypasses the repo's pre-PR gate (make agent-report-progress). check-workflow-drift: buildalways rebuilds even for standalone local runs; other targets use a conditional guard instead.
Test coverage
- No smoke tests for either the happy path or the error paths. Compile-flag changes would silently break the script.
Positive highlights
- ✅ Excellent error messaging and exit-code discipline throughout the script
- ✅ Color/TTY detection (
NO_COLOR,TERM=dumb) is a great CI-hygiene touch - ✅ Clean separation: new Makefile target + standalone script + CI job are all independent invocation surfaces
- ✅ Adding
.github/workflows/*.mdtoon.push.pathsensures the check triggers exactly when needed
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 107.8 AIC · ⌖ 7.95 AIC · ⊞ 6.7K
Comment /matt to run again
| run: chmod +x ./gh-aw | ||
|
|
||
| - name: Check workflow drift | ||
| run: make check-workflow-drift |
There was a problem hiding this comment.
[/codebase-design] make check-workflow-drift triggers the build PHONY target (go build), but this CI job has no setup-go step — it will fail on ubuntu-latest without Go in $PATH.\n\n
💡 Suggested fix: invoke the script directly
\n\nSince the job already downloads and prepares./gh-aw, bypass make and call the script directly:\n\nyaml\n- name: Check workflow drift\n run: bash scripts/check-workflow-drift.sh ./gh-aw\n\n\nThis avoids an implicit go build in a job that has no Go toolchain setup.\n\n| if ! "$BINARY" compile --validate --no-check-update --purge 2>&1; then | ||
| echo "" | ||
| echo -e "${RED}ERROR${NC}: Workflow compilation failed — fix the errors above, then run:" | ||
| echo "" |
There was a problem hiding this comment.
[/codebase-design] The script calls gh aw compile which modifies .lock.yml files in-place on the checked-out working tree, leaving a dirty state after the check runs. Any subsequent CI steps that inspect or commit files will observe unexpected changes.\n\n
💡 Suggested fix: run compilation against a temp directory or stash/restore
\n\nThe safest CI approach is to copy the workflow markdowns to a temp dir, compile there, and diff against the originals — so the real checkout is never mutated. Alternatively, save/restore the working tree withgit stash or run in a git worktree add.\n\n| # --no-check-update: skip version-update check (CI-safe, avoids network calls) | ||
| # --purge: remove orphaned .lock.yml files whose .md source was deleted | ||
| # | ||
| # The compiler skips writing a lock file when its content is already up to date, |
There was a problem hiding this comment.
[/tdd] The compilation failure path (exit 1 on compile error) is distinct from a drift failure, but there are no tests covering either path. If the compile flag names change (--validate, --no-check-update, --purge) or the binary interface evolves, this silently breaks.\n\n
💡 Suggested improvement: add a bats/shunit2 smoke test
\n\nA minimal integration test would:\n1. Copy a known.md source, run the script → assert exit 0\n2. Tamper a .lock.yml file, run the script → assert exit 1 with the expected filename in output\n3. Run the script with a missing binary → assert a clear error\n\nEven a single shell-based test in scripts/test-check-workflow-drift.sh would catch regressions.\n\n| # | ||
| # Exit codes: | ||
| # 0 - Lock files are up to date with their markdown sources | ||
| # 1 - Drift detected (lock files need to be regenerated) |
There was a problem hiding this comment.
[/codebase-design] The script accepts a positional argument for the binary path but the CI job always passes ./gh-aw explicitly. Meanwhile make check-workflow-drift calls the script with ./$(BINARY_NAME). The default (./gh-aw) duplicates the Makefile variable — if BINARY_NAME ever changes, the default will silently diverge.
💡 Suggestion: remove the default and always require the argument
BINARY="${1:?Usage: check-workflow-drift.sh <path-to-binary>}"This makes the contract explicit and surfaces misconfigurations immediately.
@copilot please address this.
| # Compiles all .github/workflows/*.md files and fails if any .lock.yml would | ||
| # change, reminding contributors to run 'make recompile' before committing. | ||
| .PHONY: check-workflow-drift | ||
| check-workflow-drift: build |
There was a problem hiding this comment.
[/codebase-design] check-workflow-drift: build rebuilds the binary every time the drift check runs locally, even when the binary is already up-to-date. When called from agent-report-progress (which already depends on build), make deduplicates it — but running make check-workflow-drift standalone will always trigger a rebuild, which is slow and unexpected.\n\n
💡 Consider making the binary prerequisite conditional
\n\nOther targets likecompile-cli-workflows already check for the binary and skip building when present. Consider either removing the build dep (and documenting that the user must build first) or adding a guard similar to compile-cli-workflows.\n\n| echo "" | ||
| echo -e "${YELLOW}Fix:${NC} Run the following commands to regenerate the lock files and commit them:" | ||
| echo "" | ||
| echo " make recompile" |
There was a problem hiding this comment.
[/codebase-design] The remediation message tells contributors to run git add .github/workflows/*.lock.yml and commit, but the AGENTS.md rules require report_progress after any file change. Recommending a raw git commit bypasses the pre-PR gate and could lead contributors to push without running make agent-report-progress.
💡 Suggested messaging change
Replace the raw git commit suggestion with the sanctioned workflow:
Fix: regenerate and stage the lock files, then use report_progress:
make recompile
git add .github/workflows/*.lock.yml
# then call report_progress to commit and push via the pre-PR gate
@copilot please address this.
There was a problem hiding this comment.
Blocking: CI job always fails due to prerequisite
The Makefile target unconditionally invokes — including in the new CI job that has no step. The job downloads a pre-built artifact, then Syncing actions-lock.json from .github/aw to pkg/actionpins/data/action_pins.json and pkg/workflow/data/action_pins.json...
✓ Action pins synced successfully
Syncing install-gh-aw.sh to actions/setup-cli/install.sh...
✓ Action scripts synced successfully
go build -ldflags "-s -w -X main.version=26e9201-dirty" -o gh-aw ./cmd/gh-aw immediately discards it and attempts a Go compilation that fails. The drift check never runs. Two inline comments detail the specific fix locations.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 212.1 AIC · ⌖ 6.57 AIC · ⊞ 5.5K
Comment /review to run again
| run: chmod +x ./gh-aw | ||
|
|
||
| - name: Check workflow drift | ||
| run: make check-workflow-drift |
There was a problem hiding this comment.
CI job will always fail here: make check-workflow-drift triggers the build Makefile prerequisite, which unconditionally runs go build — but this job has no setup-go step and no Go toolchain configured. Every run fails before the drift check ever executes.
💡 Suggested fix
Either:
Option A — call the script directly (cheapest; matches the intent of downloading the artifact):
- name: Check workflow drift
run: bash scripts/check-workflow-drift.sh ./gh-awOption B — fix the Makefile target to guard the build like compile-cli-workflows does:
check-workflow-drift:
`@if` [ ! -x "./$(BINARY_NAME)" ]; then \
echo "./$(BINARY_NAME) not found; building it first..."; \
$(MAKE) build; \
fi
`@bash` scripts/check-workflow-drift.sh ./$(BINARY_NAME)Option A is simplest since the script already accepts a binary path and ./gh-aw is already present and executable in this job.
| # Compiles all .github/workflows/*.md files and fails if any .lock.yml would | ||
| # change, reminding contributors to run 'make recompile' before committing. | ||
| .PHONY: check-workflow-drift | ||
| check-workflow-drift: build |
There was a problem hiding this comment.
build is an unconditional prerequisite: check-workflow-drift: build always runs go build, even when the binary already exists. The sibling target compile-cli-workflows avoids this exact problem with an explicit existence guard. Without the same guard here, any environment that has the binary but lacks Go (e.g., the CI job that downloads the artifact) will fail at the go build step — never reaching the actual drift check.
💡 Suggested fix
Match the pattern already used by compile-cli-workflows:
.PHONY: check-workflow-drift
check-workflow-drift:
`@if` [ ! -x "./$(BINARY_NAME)" ]; then \
echo "./$(BINARY_NAME) not found; building it first..."; \
$(MAKE) build; \
fi
`@bash` scripts/check-workflow-drift.sh ./$(BINARY_NAME)This lets the CI job (which downloads a pre-built artifact) use it without triggering a Go compilation, while still building locally if the binary is absent.
Stale
.lock.ymlfiles were a recurring CI friction point — agents and contributors were committing.github/workflows/*.mdedits without regenerating the corresponding lock files.What changed
scripts/check-workflow-drift.sh(new)gh aw compile --validate --no-check-update --purge, then checksgit diff/git ls-filesfor any.lock.ymlthat changed, appeared, or was deletedmake recompileremediation message|| trueper loop iteration to preventset -e+pipefailfrom killing the subshell when all drift captures are emptyMakefilecheck-workflow-drifttarget (requires built binary)agent-report-progress— drift in committed lock files now fails the pre-PR gate beforereport_progressis called.github/workflows/ci.ymlcheck-workflow-driftjob (needschanges+update) runs the check against the uploaded binary artifact.github/workflows/*.mdtoon.push.pathsso the job triggers when workflow sources changeFailure output example