feat: deterministic enforcement hooks for migration quality#33
Open
AlexDeMichieli wants to merge 1 commit into
Open
feat: deterministic enforcement hooks for migration quality#33AlexDeMichieli wants to merge 1 commit into
AlexDeMichieli wants to merge 1 commit into
Conversation
AlexDeMichieli
force-pushed
the
feat/plugin-hooks
branch
4 times, most recently
from
c99e748 to
e18c746
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a plugin/hooks.json hook pack for the Copilot CLI plugin to deterministically enforce migration-quality constraints (block/deny unsafe tool calls, inject workflow quality feedback, and produce an audit scorecard), and documents the new enforcement model in plugin/README.md.
Changes:
- Adds deterministic enforcement hooks (
preToolUse,postToolUse,agentStop,sessionEnd) inplugin/hooks.json. - Implements workflow “quality” detection (unpinned actions, placeholders,
write-all, missing permissions, actionlint) and a blocking quality gate with a 3-attempt safety valve. - Documents hook behavior, rationale, and how to enable/disable hooks in
plugin/README.md.
Show a summary per file
| File | Description |
|---|---|
| plugin/hooks.json | Adds 5 hooks to block hardcoded secrets and unsafe deletions, lint/check workflows, enforce a blocking quality gate, and append a migration scorecard. |
| plugin/README.md | Documents the hooks, their lifecycle events, and how to verify/disable them. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 7
| "description": "Block hardcoded secrets in file writes", | ||
| "matcher": "create|edit", | ||
| "timeoutSec": 10, | ||
| "bash": "INPUT=$(cat); ARGS=$(echo \"$INPUT\" | jq -c '.toolArgs' 2>/dev/null); HAS_SECRET=$(echo \"$ARGS\" | grep -ciE '(password|secret|token|api[_-]?key)\\s*[:=]' 2>/dev/null || true); HAS_EXPR=$(echo \"$ARGS\" | grep -cF '${' 2>/dev/null || true); if [ \"${HAS_SECRET:-0}\" -gt 0 ] && [ \"${HAS_EXPR:-0}\" -eq 0 ]; then echo '{\"permissionDecision\":\"deny\",\"permissionDecisionReason\":\"Blocked: hardcoded secret detected. Use GitHub Secrets (${{ secrets.NAME }}) instead.\"}'; else echo '{}'; fi" |
Collaborator
Author
There was a problem hiding this comment.
Fixed. Secret detection now: (1) extracts only the content/new_string field from toolArgs, (2) checks each line individually — a line must match a secret keyword + [:=] + 8+ chars of value AND not contain \${ on that same line, (3) secrets: inherit no longer triggers because inherit is <8 chars and there's no [:=] value pattern.
| "description": "Guard against file deletion outside ci-archive", | ||
| "matcher": "bash", | ||
| "timeoutSec": 10, | ||
| "bash": "INPUT=$(cat); CMD=$(echo \"$INPUT\" | jq -r '.toolArgs.command // .toolArgs // empty' 2>/dev/null); if echo \"$CMD\" | grep -qE 'rm\\s+(-[rfi]+\\s+)*' 2>/dev/null; then if ! echo \"$CMD\" | grep -q '.github/ci-archive' 2>/dev/null; then echo '{\"permissionDecision\":\"deny\",\"permissionDecisionReason\":\"Blocked: file deletion only allowed inside .github/ci-archive/. Move source CI files there before removing.\"}'; exit 0; fi; fi; echo '{}'" |
Collaborator
Author
There was a problem hiding this comment.
Fixed. The rm guard now: (1) checks for ... path traversal BEFORE checking ci-archive paths — any target containing .. is denied immediately, (2) uses a case statement with glob patterns (*/.github/ci-archive/* and *.github/ci-archive/*) for literal matching instead of regex, so xgithub/ci-archive no longer slips through.
| "description": "Quality check and actionlint on workflow files after write", | ||
| "matcher": "create|edit", | ||
| "timeoutSec": 60, | ||
| "bash": "INPUT=$(cat); ARGS=$(echo \"$INPUT\" | jq -c '.toolArgs' 2>/dev/null); FILE=$(echo \"$ARGS\" | jq -r '.file_path // .path // empty' 2>/dev/null); echo \"$FILE\" | grep -q '.github/workflows/' || exit 0; [ -f \"$FILE\" ] || exit 0; if ! command -v actionlint >/dev/null 2>&1; then if [ \"$(uname)\" = 'Linux' ]; then V='1.7.11'; curl -fsSL \"https://github.com/rhysd/actionlint/releases/download/v${V}/actionlint_${V}_linux_amd64.tar.gz\" | tar xz -C /tmp actionlint 2>/dev/null && install -m 755 /tmp/actionlint /usr/local/bin/actionlint 2>/dev/null; elif command -v brew >/dev/null 2>&1; then brew install actionlint 2>/dev/null; fi; fi; W=''; grep -qE 'uses:\\s+[^@]+@v[0-9]' \"$FILE\" 2>/dev/null && W=\"${W}- Unpinned actions: use full SHA commit refs\\n\"; grep -qiE '(TODO|FIXME|CHANGEME|PLACEHOLDER|XXX)' \"$FILE\" 2>/dev/null && W=\"${W}- Placeholder text found: replace before merging\\n\"; grep -qE 'permissions:\\s*write-all' \"$FILE\" 2>/dev/null && W=\"${W}- Over-broad permissions: replace write-all with least-privilege\\n\"; grep -qE '^permissions:' \"$FILE\" 2>/dev/null || W=\"${W}- Missing top-level permissions block\\n\"; L=''; command -v actionlint >/dev/null 2>&1 && L=$(actionlint \"$FILE\" 2>&1 | head -5); if [ -n \"$W\" ] || [ -n \"$L\" ]; then MSG=\"MIGRATION QUALITY CHECK ($FILE):\\n${W}\"; [ -n \"$L\" ] && MSG=\"${MSG}actionlint errors:\\n${L}\\n\"; MSG=\"${MSG}Fix these issues now.\"; ESCAPED=$(printf '%s' \"$MSG\" | sed 's/\\\\/\\\\\\\\/g; s/\"/\\\\\"/g'); printf '{\"additionalContext\":\"%s\"}' \"$ESCAPED\"; fi" |
Collaborator
Author
There was a problem hiding this comment.
Fixed. All 3 hooks now download to a temp file and verify SHA256 (900919a8...) before extracting. Checksum mismatch aborts install and surfaces an explicit warning. Also fixed the PR description — removed the 'checksum-verified upstream' claim and replaced with accurate description.
| "type": "command", | ||
| "description": "Migration quality gate — block completion if workflows have issues", | ||
| "timeoutSec": 60, | ||
| "bash": "INPUT=$(cat); CWD=$(echo \"$INPUT\" | jq -r '.cwd // \".\"' 2>/dev/null); COUNTER_FILE='/tmp/.migration-quality-gate'; COUNT=$(cat \"$COUNTER_FILE\" 2>/dev/null || echo 0); COUNT=$((COUNT + 1)); echo \"$COUNT\" > \"$COUNTER_FILE\"; if [ \"$COUNT\" -gt 3 ]; then rm -f \"$COUNTER_FILE\"; echo '{}'; exit 0; fi; if ! command -v actionlint >/dev/null 2>&1; then if [ \"$(uname)\" = 'Linux' ]; then V='1.7.11'; curl -fsSL \"https://github.com/rhysd/actionlint/releases/download/v${V}/actionlint_${V}_linux_amd64.tar.gz\" | tar xz -C /tmp actionlint 2>/dev/null && install -m 755 /tmp/actionlint /usr/local/bin/actionlint 2>/dev/null; elif command -v brew >/dev/null 2>&1; then brew install actionlint 2>/dev/null; fi; fi; ISSUES=''; for f in \"$CWD\"/.github/workflows/*.yml \"$CWD\"/.github/workflows/*.yaml; do [ -f \"$f\" ] || continue; FN=$(basename \"$f\"); W=''; grep -qE 'uses:\\s+[^@]+@v[0-9]' \"$f\" 2>/dev/null && W=\"${W}unpinned-actions \"; grep -qiE '(TODO|FIXME|CHANGEME|PLACEHOLDER|XXX)' \"$f\" 2>/dev/null && W=\"${W}placeholders \"; grep -qE 'permissions:\\s*write-all' \"$f\" 2>/dev/null && W=\"${W}write-all \"; command -v actionlint >/dev/null 2>&1 && ! actionlint \"$f\" >/dev/null 2>&1 && W=\"${W}actionlint-errors \"; [ -n \"$W\" ] && ISSUES=\"${ISSUES}${FN}: ${W}; \"; done; if [ -n \"$ISSUES\" ]; then ESCAPED=$(printf '%s' \"$ISSUES\" | sed 's/\\\\/\\\\\\\\/g; s/\"/\\\\\"/g'); printf '{\"decision\":\"block\",\"reason\":\"Migration quality gate FAILED (attempt %d/3). Fix these workflow issues:\\n%s\"}' \"$COUNT\" \"$ESCAPED\"; else rm -f \"$COUNTER_FILE\"; echo '{}'; fi" |
Collaborator
Author
There was a problem hiding this comment.
Fixed. agentStop now includes grep -qE '^permissions:' || W='no-permissions ' in its per-file scan, consistent with postToolUse.
| "type": "command", | ||
| "description": "Generate final migration scorecard", | ||
| "timeoutSec": 45, | ||
| "bash": "INPUT=$(cat); CWD=$(echo \"$INPUT\" | jq -r '.cwd // \".\"' 2>/dev/null); REASON=$(echo \"$INPUT\" | jq -r '.reason // \"unknown\"' 2>/dev/null); SESSION=$(echo \"$INPUT\" | jq -r '.sessionId // \"unknown\"' 2>/dev/null); if ! command -v actionlint >/dev/null 2>&1; then if [ \"$(uname)\" = 'Linux' ]; then V='1.7.11'; curl -fsSL \"https://github.com/rhysd/actionlint/releases/download/v${V}/actionlint_${V}_linux_amd64.tar.gz\" | tar xz -C /tmp actionlint 2>/dev/null && install -m 755 /tmp/actionlint /usr/local/bin/actionlint 2>/dev/null; elif command -v brew >/dev/null 2>&1; then brew install actionlint 2>/dev/null; fi; fi; TOTAL=0; CLEAN=0; BAD=0; for f in \"$CWD\"/.github/workflows/*.yml \"$CWD\"/.github/workflows/*.yaml; do [ -f \"$f\" ] || continue; TOTAL=$((TOTAL + 1)); HAS=0; grep -qE 'uses:\\s+[^@]+@v[0-9]' \"$f\" 2>/dev/null && HAS=1; grep -qiE '(TODO|FIXME|CHANGEME|PLACEHOLDER|XXX)' \"$f\" 2>/dev/null && HAS=1; grep -qE 'permissions:\\s*write-all' \"$f\" 2>/dev/null && HAS=1; command -v actionlint >/dev/null 2>&1 && ! actionlint \"$f\" >/dev/null 2>&1 && HAS=1; [ \"$HAS\" -eq 0 ] && CLEAN=$((CLEAN + 1)) || BAD=$((BAD + 1)); done; rm -f /tmp/.migration-quality-gate; SC=\"$CWD/.github/MIGRATION-SCORECARD.md\"; [ -f \"$SC\" ] || printf '# Migration Scorecard\\n' > \"$SC\" 2>/dev/null; printf '\\n## %s\\n- Session: %s\\n- Reason: %s\\n- Workflows: %d total, %d clean, %d with issues\\n' \"$(date -u +%Y-%m-%dT%H:%M:%SZ)\" \"$SESSION\" \"$REASON\" \"$TOTAL\" \"$CLEAN\" \"$BAD\" >> \"$SC\" 2>/dev/null; echo '{}'" |
Collaborator
Author
There was a problem hiding this comment.
Fixed. sessionEnd scorecard now includes grep -qE '^permissions:' || HAS=1 so missing-permissions counts as an issue. Consistent with all other hooks.
| | File deletion guard | `preToolUse` | `bash` | Hard-denies `rm` operations outside `.github/ci-archive/`. Prevents accidental deletion of application source code. | | ||
| | Quality check + actionlint | `postToolUse` | `create\|edit` | After any workflow file write, injects `additionalContext` with: unpinned actions (tag vs SHA), placeholder text (TODO/FIXME), over-broad permissions (`write-all`), missing permissions block, and actionlint errors. The agent sees these on the same turn. | | ||
| | **Quality gate** | `agentStop` | — | Scans ALL workflow files when the agent finishes a turn. If any have issues, returns `decision: "block"` forcing the agent to take another turn to fix them. Safety valve releases after 3 attempts to prevent infinite loops. | | ||
| | **Migration scorecard** | `sessionEnd` | — | Generates `.github/MIGRATION-SCORECARD.md` with session ID, timestamp, completion reason, and workflow counts (total / clean / with-issues). Audit artifact for migration quality tracking. | |
Collaborator
Author
There was a problem hiding this comment.
Fixed. Description changed to 'Appends an entry to' and hook description changed to 'Append migration scorecard entry'.
| "description": "Quality check and actionlint on workflow files after write", | ||
| "matcher": "create|edit", | ||
| "timeoutSec": 60, | ||
| "bash": "INPUT=$(cat); ARGS=$(echo \"$INPUT\" | jq -c '.toolArgs' 2>/dev/null); FILE=$(echo \"$ARGS\" | jq -r '.file_path // .path // empty' 2>/dev/null); echo \"$FILE\" | grep -q '.github/workflows/' || exit 0; [ -f \"$FILE\" ] || exit 0; if ! command -v actionlint >/dev/null 2>&1; then if [ \"$(uname)\" = 'Linux' ]; then V='1.7.11'; curl -fsSL \"https://github.com/rhysd/actionlint/releases/download/v${V}/actionlint_${V}_linux_amd64.tar.gz\" | tar xz -C /tmp actionlint 2>/dev/null && install -m 755 /tmp/actionlint /usr/local/bin/actionlint 2>/dev/null; elif command -v brew >/dev/null 2>&1; then brew install actionlint 2>/dev/null; fi; fi; W=''; grep -qE 'uses:\\s+[^@]+@v[0-9]' \"$FILE\" 2>/dev/null && W=\"${W}- Unpinned actions: use full SHA commit refs\\n\"; grep -qiE '(TODO|FIXME|CHANGEME|PLACEHOLDER|XXX)' \"$FILE\" 2>/dev/null && W=\"${W}- Placeholder text found: replace before merging\\n\"; grep -qE 'permissions:\\s*write-all' \"$FILE\" 2>/dev/null && W=\"${W}- Over-broad permissions: replace write-all with least-privilege\\n\"; grep -qE '^permissions:' \"$FILE\" 2>/dev/null || W=\"${W}- Missing top-level permissions block\\n\"; L=''; command -v actionlint >/dev/null 2>&1 && L=$(actionlint \"$FILE\" 2>&1 | head -5); if [ -n \"$W\" ] || [ -n \"$L\" ]; then MSG=\"MIGRATION QUALITY CHECK ($FILE):\\n${W}\"; [ -n \"$L\" ] && MSG=\"${MSG}actionlint errors:\\n${L}\\n\"; MSG=\"${MSG}Fix these issues now.\"; ESCAPED=$(printf '%s' \"$MSG\" | sed 's/\\\\/\\\\\\\\/g; s/\"/\\\\\"/g'); printf '{\"additionalContext\":\"%s\"}' \"$ESCAPED\"; fi" |
Collaborator
Author
There was a problem hiding this comment.
Fixed. All 3 hooks now: (1) on checksum mismatch, abort install and inject explicit warning via additionalContext, (2) after install attempt, re-check command -v actionlint — if still missing, inject 'actionlint not available (install failed)' warning, (3) agentStop adds 'actionlint-unavailable' to the issues list so it blocks completion.
… actionlint Rebuild hooks.json with correct Copilot hooks API: - preToolUse (matcher: create|edit): secret detection with permissionDecision deny - preToolUse (matcher: bash): rm guard blocks deletion outside ci-archive - postToolUse (matcher: create|edit): quality check + actionlint per workflow file - agentStop: quality gate blocks agent completion until workflows pass (3-attempt safety valve) - sessionEnd: generates MIGRATION-SCORECARD.md with session stats Key changes from previous version: - Use permissionDecision/permissionDecisionReason (not decision/reason) - Add matcher filtering (no more shell-level tool name checks) - agentStop replaces postToolUse-only approach — actually blocks completion - sessionEnd provides audit artifact for migration quality tracking - actionlint runs in 3 hooks: postToolUse, agentStop, sessionEnd - Test harness with 21 passing tests included
AlexDeMichieli
force-pushed
the
feat/plugin-hooks
branch
from
e18c746 to
e142373
Compare
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.
What this brings
This PR adds 5 hooks to the CLI plugin that enforce migration quality deterministically — the agent cannot bypass these checks.
Hooks overview
preToolUsecreate|edit(matcher)${{ secrets.NAME }}.preToolUsebash(matcher)rmoutside.github/ci-archive/. Prevents source code deletion.postToolUsecreate|edit(matcher)agentStopdecision: "block") if any workflows have issues, forcing the agent to fix them. 3-attempt safety valve prevents infinite loops.sessionEnd.github/MIGRATION-SCORECARD.md— audit artifact with session ID, timestamp, completion reason, and workflow quality counts (total/clean/issues). Each session appends rather than overwrites, so you see progression across multiple passes.How enforcement works
actionlint runs in 3 hooks:
postToolUse(per-file immediate),agentStop(all-files gate),sessionEnd(final counts). The agent cannot skip linting.What is actionlint and how do hooks help?
actionlint is a static analysis tool for GitHub Actions workflow files. It catches syntax errors, invalid triggers, unpinned actions, shellcheck issues, bad permissions, incorrect
needs:references, and more. Running it after every migration is critical — without it, generated workflows may look correct but fail at runtime.Before hooks, the agent had to: (1) load the actionlint skill into context (~4.5k tokens), (2) install actionlint via shell commands, (3) run it manually, (4) read the output, (5) fix issues — each step a separate agent turn consuming ~100k+ input tokens. The agent could also skip any of these steps since skills are advisory.
With hooks, this entire cycle is handled deterministically by three hooks:
postToolUse(matcher:create|edit): Fires immediately after the agent writes a workflow file. Auto-installs actionlint if missing, runs it on the file, and injects errors asadditionalContexton the same turn — the agent sees issues without spending extra turns.agentStop: Fires when the agent finishes a turn. Scans all workflow files with actionlint. If any have errors, returnsdecision: "block"forcing the agent to fix them before completing. The agent cannot finish with broken workflows.sessionEnd: Fires at session close. Runs actionlint on all workflows for the final clean/issues count in the migration scorecard.The hooks cannot be skipped or ignored — they're shell commands that execute outside the model. This is the difference between "please run actionlint" (instruction the agent can ignore) and "actionlint will run and block you" (hook).
actionlint auto-install
All 3 hooks that use actionlint will install it automatically if not present:
brew install actionlintTimeouts are set to 60s/60s/45s to accommodate the one-time install. This eliminates the dependency on the agent remembering to invoke the actionlint skill for installation — the hook handles it deterministically.
Migration scorecard
The
sessionEndhook appends to.github/MIGRATION-SCORECARD.mdafter every session. Multiple passes show quality progression:Each workflow is checked for:
@v4) instead of SHA refspermissions: write-allA workflow must pass all four checks to count as "clean."
Correct hooks API
Uses the hooks reference API correctly:
permissionDecision: "deny"/permissionDecisionReasonfor preToolUse blocksmatcherregex filtering onpreToolUseandpostToolUse(replaces shell-level tool name checks)agentStopwithdecision: "block"/reasonfor forced continuationsessionEndfor audit artifactstoolName,toolArgs,cwd,sessionIdTesting
--log-level debug— confirmed all 5 hook types fire,agentStopreturnsdecision: "block",sessionEndappends scorecard entries with real session IDs, and the second pass shows quality improvementCloud agent compatibility
All 5 hooks use events supported by Copilot cloud agent.
agentStopdecision: "block"forces another turn (counts against job timeout).sessionEndfires once per job. Onlybashcommand hooks are used (no PowerShell). actionlint auto-installs from GitHub releases on Linux.Token savings
Without hooks, a typical actionlint cycle looks like this:
actionlintskillactionlint, reads lint output, starts fixingThat's 3 extra turns (~315k input tokens) just for linting — repeated for every workflow file.
With hooks, the same cycle is:
additionalContexton the same turn. Agent sees errors immediately.That's ~100-200k saved per workflow file — no skill invocation, no manual install, no separate lint run. At scale across dozens of migrations with multiple workflow files each, this adds up.
Files changed
plugin/hooks.json— 5 production hooks with auto-install, matcher filtering, correct API fields, and append scorecardplugin/README.md— updated hooks documentation