Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ on:
- 'Makefile'
- 'actions/setup/**'
- '.github/workflows/ci.yml'
- '.github/workflows/*.md'
- '.github/aw/releases.json'
- '.github/aw/releases.schema.json'
- '.github/aw/compat.json'
Expand Down Expand Up @@ -408,6 +409,35 @@ jobs:
- name: Compile pkg/cli/workflows
run: make compile-cli-workflows

check-workflow-drift:
name: Check workflow markdown/lock drift
if: ${{ needs.changes.outputs.has_changes == 'true' }}
needs:
- changes
- update
runs-on: ubuntu-latest
timeout-minutes: 10
permissions:
contents: read
concurrency:
group: ci-${{ github.ref }}-check-workflow-drift
cancel-in-progress: true
steps:
- name: Checkout code
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2

- name: Download gh-aw binary artifact
uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4
with:
name: gh-aw-linux-amd64
path: .

- name: Prepare gh-aw binary
run: chmod +x ./gh-aw

- name: Check workflow drift
run: make check-workflow-drift

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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

\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
\n\n@copilot please address this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-aw

Option 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.


js-integration-live-api:
if: ${{ needs.changes.outputs.has_changes == 'true' && (github.ref == 'refs/heads/main') }}
needs:
Expand Down
16 changes: 12 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,13 @@ lint-lock: build
@echo "Linting committed lock files with gh aw lint..."
./$(BINARY_NAME) lint

# Check for drift between workflow markdown sources and generated lock files.
# 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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

\n💡 Consider making the binary prerequisite conditional\n\nOther targets like compile-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
\n\n@copilot please address this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bash scripts/check-workflow-drift.sh ./$(BINARY_NAME)
Comment thread
Copilot marked this conversation as resolved.

# Format code
.PHONY: fmt
fmt: fmt-go fmt-cjs fmt-json
Expand Down Expand Up @@ -1083,10 +1090,10 @@ agent-finish: deps-dev fmt lint build build-wasm test-all validate-otel-contract

# Lightweight pre-PR gate — run before every report_progress / create_pull_request call.
# Includes formatting + lint validation to prevent lint-fix PR churn:
# build + fmt + lint + test-unit.
# build + fmt + lint + test-unit + workflow drift check.
.PHONY: agent-report-progress
agent-report-progress: build fmt lint test-unit
@echo "Pre-PR validation passed (zero lint errors). Safe to call report_progress."
agent-report-progress: build fmt lint test-unit check-workflow-drift
@echo "Pre-PR validation passed (zero lint errors, lock files in sync). Safe to call report_progress."

# Extended pre-PR gate with lock-file-only linting.
.PHONY: agent-report-progress-lint
Expand Down Expand Up @@ -1157,6 +1164,7 @@ help:
@echo " actionlint - Validate workflows with actionlint (depends on build)"
@echo " lint-lock - Run lock-file-only lint with gh aw lint (depends on build)"
@echo " validate-workflows - Validate compiled workflow lock files (depends on build)"
@echo " check-workflow-drift - Check for drift between .md sources and .lock.yml files (depends on build)"
@echo " install - Install binary locally"
@echo " sync-action-pins - Sync actions-lock.json from .github/aw to pkg/actionpins/data and pkg/workflow/data (runs automatically during build)"
@echo " sync-action-scripts - Sync install-gh-aw.sh to actions/setup-cli/install.sh (runs automatically during build)"
Expand All @@ -1176,7 +1184,7 @@ help:
@echo " clean-docs - Clean documentation artifacts (dist, node_modules, .astro)"

@echo " agent-finish - Complete validation sequence (build, test, fix, recompile, fmt, lint, security-scan)"
@echo " agent-report-progress - Lightweight pre-PR gate: build + fmt + lint + test-unit"
@echo " agent-report-progress - Lightweight pre-PR gate: build + fmt + lint + test-unit + check-workflow-drift"
@echo " agent-report-progress-lint - Pre-PR gate + gh aw lint lock-file check"
@echo " sbom - Generate SBOM in SPDX and CycloneDX formats (requires syft)"
@echo " help - Show this help message"
102 changes: 102 additions & 0 deletions scripts/check-workflow-drift.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
#!/bin/bash
set +o histexpand

# check-workflow-drift.sh - Detect drift between workflow markdown sources and generated lock files
#
# Compiles all .github/workflows/*.md files and then checks whether the resulting
# .lock.yml files match what is already on disk. Exits non-zero and prints a clear
# remediation message if any drift is detected.
#
# Usage:
# check-workflow-drift.sh [<path-to-binary>]
#
# Arguments:
# <path-to-binary> Path to the gh-aw binary. Defaults to ./gh-aw.
#
# Exit codes:
# 0 - Lock files are up to date with their markdown sources
# 1 - Drift detected (lock files need to be regenerated)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.


set -euo pipefail

# Disable colors when not connected to a TTY, when NO_COLOR is set, or when
# TERM=dumb — this keeps output readable when captured into CI step summaries.
if [ -t 1 ] && [ -z "${NO_COLOR:-}" ] && [ "${TERM:-}" != "dumb" ]; then
RED='\033[0;31m'
GREEN='\033[0;32m'
YELLOW='\033[1;33m'
NC='\033[0m'
else
RED=''
GREEN=''
YELLOW=''
NC=''
fi

BINARY="${1:-./gh-aw}"

echo "Checking for workflow markdown/lock file drift..."
echo ""

# Compile all workflow markdown files, regenerating lock files in place.
# --validate: enforce schema validation so compilation errors surface clearly
# --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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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

\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
\n\n@copilot please address this.

# so running this on a clean checkout leaves the working tree unmodified.
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 ""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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

\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 with git stash or run in a git worktree add.\n\n
\n\n@copilot please address this.

echo " make recompile"
echo ""
exit 1
fi

# Collect lock files that changed (modified or newly created by compilation).
# git diff --name-only: tracked files whose content changed
# git ls-files --others --exclude-standard: new untracked files (new lock files)
# git ls-files --deleted: tracked files removed by --purge
#
# The pathspec is intentionally single-quoted to prevent the shell from
# expanding the glob before passing it to git. Git receives the literal
# pattern '.github/workflows/*.lock.yml' and applies its own glob matching,
# which works correctly across all git versions we target.
drift_modified=$(git diff --name-only -- '.github/workflows/*.lock.yml' 2>/dev/null || true)
drift_untracked=$(git ls-files --others --exclude-standard -- '.github/workflows/*.lock.yml' 2>/dev/null || true)
drift_deleted=$(git ls-files --deleted -- '.github/workflows/*.lock.yml' 2>/dev/null || true)

# Combine and de-duplicate, skipping empty variables.
# Each capture variable may contain multiple newline-separated filenames.
# printf '%s\n' "$v" preserves embedded newlines, so all filenames are passed
# to sort -u as individual lines. The || true on each iteration prevents
# set -e from killing the subshell when [ -n "$v" ] is false.
all_drift=$(
for v in "$drift_modified" "$drift_untracked" "$drift_deleted"; do
[ -n "$v" ] && printf '%s\n' "$v" || true
done | sort -u
)

if [ -z "$all_drift" ]; then
echo -e "${GREEN}✓ All workflow lock files are in sync with their markdown sources.${NC}"
exit 0
fi

echo ""
echo -e "${RED}ERROR: Workflow lock files are out of sync with their markdown sources.${NC}"
echo ""
echo "The following lock files differ from what would be generated by compilation:"
echo ""
while IFS= read -r file; do
echo " $file"
done <<< "$all_drift"
echo ""
echo -e "${YELLOW}Fix:${NC} Run the following commands to regenerate the lock files and commit them:"
echo ""
echo " make recompile"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

echo " git add .github/workflows/*.lock.yml"
echo " git commit -m 'chore: regenerate workflow lock files'"
echo ""
echo "Lock files must always be committed together with their .md sources."
exit 1
Loading