fix(workflows): remove literal dollar-brace-brace from run-block comments (actual root cause)#28
Conversation
…actual bug) Root cause for the workflow_dispatch parse failures that defeated PRs #24, #25, #26, and #27: GitHub Actions substitutes '\${{ ... }}' expressions inside run: blocks *before* the shell sees them, including sequences inside shell comments. Three comments in this file contained the literal string '\${{ }}' (empty expression). GHA's queue-time parser tried to evaluate an empty expression and bailed with 'An expression was expected', always pointing at 'run: |' (col 14) because that's the parent scope. Diagnostic that confirmed this: the reported line numbers shifted between my earlier attempts (55/111 -> 53/126 after the jq refactor). The error position is file-structure-dependent; col 14 is always the '|' of 'run: |'. Meanwhile the literal '\${{ }}' was identical across every version of the file since the security-hardening cascade (mcp-clipboard#87), which is why every '|| X' fallback experiment missed — the fallbacks were on the wrong expressions. Fix: rewrite the three comments to describe the concept without the literal '\${{ }}' characters. Zero logic change. Why this only surfaced on workflow_dispatch (not on normal workflow_run firings): the normal workflow_run code path apparently tolerates empty expressions where the queue-time parser for workflow_dispatch does not. mcp-clipboard has the same latent bug — it just never tripped it because nobody dispatched manually. Cascade: the same fix needs to land on cmeans/mcp-clipboard's file to preserve verbatim parity and to unblock manual dispatch there as well. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cmeans
left a comment
There was a problem hiding this comment.
[QA] Review — zero findings, Ready for QA Signoff.
Sharp root-cause work. The line-shift evidence (55/111 before #27 → 53/126 after #27, col always 14 = | of run: |) is clean and the conclusion that GHA substitutes ${{ ... }} sequences inside run-block comments is a genuinely non-obvious GHA quirk. Worth flagging that these exact comments traced back to the shell-injection hardening cascade (mcp-clipboard#87 → PR #19 → #22's re-sync) — so this bug has been latent across the whole template family since then, masked only by nobody having tried to workflow_dispatch those workflows before.
Verified in this session
| Check | Result |
|---|---|
grep '\${{ *}}' .github/workflows/*.yml on the PR branch |
Zero matches — all three empty-expression sites scrubbed. |
| Diff shape | 3 comment rewrites, zero code changes, zero indentation drift. ${{ secrets.GITHUB_TOKEN }} and ${{ github.repository }} (real content) untouched — those have always been fine. |
pytest tests/ |
259/259 pass. |
ruff check src/ tests/ |
Clean. |
| YAML parse | Clean. |
Cascade impact (spot-checked)
Same grep against cmeans/mcp-clipboard/.github/workflows/ hits two sites in its pr-labels-ci.yml (lines 49, 100). Same latent bug, masked only by mcp-clipboard never having dispatched manually. Updated cmeans/mcp-clipboard#89 with the details — the cascade there now needs to cover the workflow_dispatch trigger, the || '' → env fallback, and this comment scrub all at once. Going forward, grepping for \${{ *}} in any repo adopting this workflow family would catch it before it bites.
Verdict
Applying Ready for QA Signoff. Comment-only change, zero logic risk, no happy-path impact. Even if the workflow_run registration doesn't recover post-merge (unlikely — this is by far the most plausible root cause of everything we've been seeing), the file is strictly cleaner than it was, and the revert path — if you do decide to pull the whole seeding chain — is still simple.
Post-merge
Same playbook. Dispatch from the Actions UI. This time the expected outcome is: the queue doesn't error, both jobs skip cleanly, run completes with status "success". After that, the first PR with a real CI completion should route its workflow_run event to this workflow. The real signal to watch is workflow_run runs > 0 in the API history — independent of the name field quirk (which, recall, mcp-clipboard exhibits too despite its listener working).
….yml comments (#92) ## Summary Two shell comments inside `run:` blocks of `.github/workflows/pr-labels-ci.yml` carried the literal sequence for an empty GitHub Actions expression. GHA substitutes such sequences inside `run:` blocks before the shell sees them — **including within shell comments** — and the queue-time parser rejects the empty form: > Failed to queue workflow run: Invalid Argument - failed to parse workflow: An expression was expected The normal `workflow_run` path tolerated it (evaluated to empty string, carried on), so the bug has been latent on mcp-clipboard since #88 landed. It only surfaces on `workflow_dispatch`, or on a fresh-repo cascade where `workflow_run` registration hasn't happened yet. Root cause was traced on `cmeans/yt-dont-recommend#28` after four separate fix attempts (#24, #25, #26, #27) each produced the same parse error at `run: |` col 14 — the enclosing scope, not the actual offending token. Closes #91. ## Change (+2 / −2, comment-only) Both comments rewritten to describe the concept without the literal characters. Zero logic change; the legitimate non-empty expressions elsewhere in the file (in `env:` blocks) are untouched. ## Test plan - [x] `yaml.safe_load` on `pr-labels-ci.yml` — clean parse (verified locally). - [x] `grep '${{ }}'` on the file returns nothing (verified locally). - [x] No behavior change on the `workflow_run` happy path — CI on this PR exercises the pipeline as usual. - [ ] Post-merge sanity: manual `workflow_dispatch` from the Actions UI on `pr-labels-ci.yml` queues cleanly (both jobs skip because their `if:` guards require `github.event.workflow_run.*`, which don't exist on `workflow_dispatch`). Optional; nothing in mcp-clipboard today requires manual dispatch to work. - [x] Unblocks #89 (adding `workflow_dispatch:` trigger) — safe to pick that up only after this lands, otherwise it would trip the exact same parser error. ## Related - Root-cause diagnosis and fix: `cmeans/yt-dont-recommend#28`. - Sibling issue on this repo: #89 (propagate `workflow_dispatch:` trigger from yt-dont-recommend#24). Must land **after** this. - Companion cascades on other repos tracked separately (mcp-awareness, mcp-synology) — those also need the earlier PR #87 / #88 hardening alongside the comment fix. ## Context Refiled cleanly from the original PR #90 (closed) after the upstream claude-dev session confirmed the issue-first handoff was the correct process. Issue #91 carries the spec. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: cmeans-claude-dev[bot] <3223881+cmeans-claude-dev[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…on (#332) (#333) Closes #332. ## Summary `.github/workflows/pr-labels-ci.yml` on this repo was in the pre-`cmeans/mcp-clipboard#88` state for the sibling `cmeans/*` cascade: contributor-controlled `workflow_run` fields (`head_branch`, `id`, `repository`) were inlined directly as `${{ ... }}` expressions inside `run:` bodies. Git refnames allow shell metacharacters (`$`, backtick, `;`, `&`, `|`, etc.), so a malicious fork-PR branch name would render as directly-executed shell once GHA substituted the expression at queue time. This PR cascades the hardening from `cmeans/mcp-clipboard`: 1. **#88 pattern — env routing.** All contributor-controlled values now flow through step-level `env:` blocks and are referenced as shell variables (`"$HEAD_BRANCH"`, `"$REPO"`, `"$RUN_ID"`). Job-level `if:` conditionals continue to use `${{ ... }}` (that expression context is safe — it's evaluated by GHA itself, not handed to a shell). 2. **#92 pattern — comment escape.** Comments inside `run:` blocks avoid any literal `${{ }}` sequence. GHA's queue-time parser substitutes expressions even inside shell `#` comments and rejects empty expressions with `An expression was expected` on `workflow_dispatch` / fresh-repo registration. Without this, the #88 hardening would have landed a latent trap. Cascaded verbatim from `cmeans/mcp-clipboard`'s `.github/workflows/pr-labels-ci.yml` at the `main` HEAD after PR #92 merged (2026-04-20T20:26:12Z), preserving this repo's AGPL-3.0 header at lines 1–16. No functional behavior change — label transitions, PR lookup, force-push tolerance, and Dev Active skip logic are all byte-identical to before, just with shell values now arriving via environment instead of via text substitution. ## Scope - `.github/workflows/pr-labels-ci.yml` — hardened (22 lines changed) - `CHANGELOG.md` — `[Unreleased] → ### Security` entry No source, tests, migrations, or docs. ## References - Root diagnosis of the GHA parser quirk: [cmeans/yt-dont-recommend#28](cmeans/yt-dont-recommend#28) - Original security hardening (merged): [cmeans/mcp-clipboard#88](cmeans/mcp-clipboard#88) - Comment-escape cascade source (merged): [cmeans/mcp-clipboard#92](cmeans/mcp-clipboard#92) - Follow-up for symmetric `pr-labels.yml` hardening (non-blocking, P3): [#334](#334) ## QA ### Prerequisites Pure workflow-file change. No deploy, no env setup, no Python changes. All verification is static (`yq`/`python -c`, `grep`) plus an optional post-merge Actions UI smoke. ### Automated checks The CHANGELOG check, ruff, mypy, pytest, Codecov — none of these touch the workflow file. They should all pass unchanged against `main`. Confirm via the green CI checks on this PR before approving. ### Manual tests All local verification runs from the repo root on the PR branch (`fix/pr-labels-ci-hardening-332`). 1. - [x] **YAML parse is clean.** ``` python3 -c "import yaml; yaml.safe_load(open('.github/workflows/pr-labels-ci.yml')); print('OK')" ``` Expected: prints `OK`, exit 0. Confirms no syntax regression. 2. - [x] **No literal empty `${{ }}` anywhere in the file.** ``` grep -n '\${{ }}' .github/workflows/pr-labels-ci.yml || echo "(none — good)" ``` Expected: prints `(none — good)`. Guarantees the `#92` parser trap can't surface on `workflow_dispatch` / fresh-repo registration. 3. - [x] **No contributor-controlled field appears inside a `run:` body as an expression.** ``` awk '/^ run: \|/,/^ - name:|^ [^ ]|^jobs:/' .github/workflows/pr-labels-ci.yml | grep -nE '\$\{\{ *github\.event\.workflow_run\.(head_branch|id) *\}\}' || echo "(none — good)" ``` Expected: prints `(none — good)`. The only remaining `${{ github.event.workflow_run.head_branch }}` references are inside job-level `if:` conditionals (safe context, evaluated by GHA not a shell) and step-level `env:` assignments (safe by design — the whole point of the cascade). 4. - [x] **`env:` blocks route the three contributor-controlled fields.** ``` grep -nE '^[[:space:]]+(REPO|RUN_ID|HEAD_BRANCH):' .github/workflows/pr-labels-ci.yml ``` Expected: six lines total (three per job), each assigning from the matching `${{ github.event.workflow_run.* }}` / `${{ github.repository }}` expression. 5. - [x] **`workflows: [CI]` still matches the CI workflow name in this repo.** ``` grep -A1 'workflows:' .github/workflows/pr-labels-ci.yml | head -3; grep '^name:' .github/workflows/ci.yml ``` Expected: `workflows: [CI]` appears in this file AND `name: CI` appears in `.github/workflows/ci.yml`. Names match, so `workflow_run` will still fire on CI completion. 6. - [x] **AGPL header preserved.** ``` sed -n '1,16p' .github/workflows/pr-labels-ci.yml ``` Expected: lines 1–16 are the existing `mcp-awareness — ambient system awareness for AI agents` / `Copyright (C) 2026 Chris Means` / AGPLv3 preamble. Unchanged from `main`. 7. - [x] **Diff against `main` is hardening-only (no behavior drift).** ``` git diff origin/main -- .github/workflows/pr-labels-ci.yml ``` Expected: only adds `REPO:`, `RUN_ID:`, `HEAD_BRANCH:` to the two `env:` blocks, deletes the shell-level `REPO=${{...}}` / `HEAD_BRANCH=${{...}}` assignments, rewrites `API_OUT=...${{ ... }}/pull_requests` to `.../$RUN_ID/pull_requests`, and updates two comment blocks to avoid `${{ }}` literals. No changes to `if:` conditionals, `permissions`, `on:`, job names, label manipulation logic, or `exit 0` paths. 8. - [ ] **(Post-merge) Smoke the label automation end-to-end.** This workflow is `workflow_run`-triggered (no `workflow_dispatch`), so pre-merge dispatch isn't available. After merge, take any in-flight `Awaiting CI` PR and confirm the label transitions still happen on the next CI completion: - Watch `.github/workflows/pr-labels-ci.yml` runs at https://github.com/cmeans/mcp-awareness/actions/workflows/pr-labels-ci.yml - Expected on CI pass: `on-ci-pass` succeeds (~5 s), target PR moves `Awaiting CI → Ready for QA`. - Expected on CI fail: `on-ci-fail` succeeds, target PR gains `CI Failed`. ### Out of scope (explicitly) - **`pr-labels.yml`** (the `on: pull_request:` sibling) is not in this PR. Its current trigger makes `secrets.GITHUB_TOKEN` read-only for fork PRs and the values it does inline into `run:` blocks today are all safe types (numeric, repo name, hex SHA), so no injection vector exists at the current configuration. Symmetric env-routing as defense-in-depth tracked in [#334](#334) (P3, non-blocking). - **Adding `workflow_dispatch` to `pr-labels-ci.yml`** for post-merge verifiability. Not cascaded because mcp-clipboard doesn't have it either. Track separately if desired. --------- Co-authored-by: cmeans-claude-dev[bot] <3223881+cmeans-claude-dev[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
## Summary Closes #52. `.github/workflows/pr-labels-ci.yml` was in its pre-hardening state: both the `on-ci-pass` and `on-ci-fail` jobs inlined `${{ github.event.workflow_run.head_branch }}` directly inside `run:` blocks. `head_branch` is contributor-controlled on fork PRs and git refnames allow shell metacharacters (`$`, backtick, `;`, `&`, `|`, etc.), so a malicious fork branch name would have rendered as directly-executed shell once the expression was substituted. The same hardening has already landed on sibling repos (`cmeans/mcp-clipboard#88`, `cmeans/yt-dont-recommend`), but never cascaded here. ## Change - **`.github/workflows/pr-labels-ci.yml`** — `REPO`, `RUN_ID`, and `HEAD_BRANCH` now come through step-level `env:` blocks on both jobs. The shell references them as `$REPO` / `$RUN_ID` / `$HEAD_BRANCH`. No remaining `${{ ... }}` substitutions inside either `run:` body. - **CHANGELOG entry** added under `## Unreleased` → `### Fixed`. ## Latent-parser-trap note Issue #52 flagged that the verbatim cascade from mcp-clipboard's post-#88 source also carried a shell comment containing a literal empty GHA expression — GHA substitutes `${{ ... }}` inside `run:` blocks before the shell sees them *including within shell comments*, and the queue-time parser rejects an empty expression on `workflow_dispatch`. The explanatory comments in this PR therefore describe the concept ("not a direct GHA expression") rather than showing the literal sequence, matching the fix on `cmeans/mcp-clipboard#92`. This is defense-in-depth today — the trap itself cannot fire on this repo until a `workflow_dispatch:` trigger is added (see *Out of scope* below). ## Test plan - [x] `python3 -c "import yaml; yaml.safe_load(open('.github/workflows/pr-labels-ci.yml'))"` → clean parse. - [x] `grep '${{ }}'` on the file → no matches. - [x] Programmatic scan: no `${{ ... }}` substitution survives inside any `run:` body in the file. - [x] `workflow_run.workflows: [CI]` still matches this repo's CI workflow name (`name: CI` in `ci.yml`). - [ ] After merge: the next CI pass on a PR branch still transitions `Awaiting CI` → `Ready for QA` (and CI fail still sets `CI Failed`). `workflow_run` listeners can only be exercised post-merge on the default branch. ## Out of scope - **Adding a `workflow_dispatch:` trigger** to `pr-labels-ci.yml`. mcp-clipboard defers this to a separate PR (#89 there) and that precedent fits this repo too — keeping issue #52's scope to the hardening cascade. A follow-up issue can pick it up if manual dispatch is wanted; the defensive comment-escape in this PR ensures that follow-up won't trip the queue-time parser. ## Related - Original hardening: `cmeans/mcp-clipboard#88` - Empty-expression parser diagnosis: `cmeans/yt-dont-recommend#28` - Comment-escape cascade: `cmeans/mcp-clipboard#92` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: cmeans-claude-dev[bot] <3223881+cmeans-claude-dev[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…CI fix (#36) Pre-release audit against `git log v0.4.2..main` surfaced three gaps in the `[Unreleased]` section: - No entry for PR #19 (QA workflow + PR label state machine) — a major infrastructure addition precedented by v0.4.2's Ruff entry. - No entry for the #24-#28 `workflow_run` registration fix saga. - The coverage push entry did not call out the `# pragma: no cover` sweep, which is a hard-rule policy for the project. Adds an Added entry for #19, a Fixed entry for #24-#28, and tacks a sentence onto the existing coverage entry. No code changes. Co-authored-by: cmeans-claude-dev[bot] <3223881+cmeans-claude-dev[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Found the actual root cause of the workflow_dispatch parse failures that defeated PRs #24 (workflow_dispatch trigger), #25 (null-coalesce with
''), #26 (null-coalesce with'none'), and #27 (jq from$GITHUB_EVENT_PATH).GitHub Actions substitutes
${{ ... }}expressions insiderun:blocks before the shell sees them — including sequences inside shell comments. Three comments in this file contained the literal string${{ }}(empty expression). GHA's queue-time parser tried to evaluate an empty expression and bailed with "An expression was expected", always pointing atrun: |(col 14) because that's the parent scope.Diagnostic that clinched it
After PR #27 merged, the reported error line numbers shifted from 55/111 (before #27) to 53/126 (after #27). That proved the error position is file-structure-dependent — col 14 is always the
|ofrun: |. Meanwhile the literal${{ }}in comments has been identical across every version of this file since the security-hardening cascade from mcp-clipboard#87. Every|| Xfallback experiment was modifying the wrong expressions.Why this only surfaced on workflow_dispatch
The normal
workflow_runcode path apparently tolerates empty${{ }}expressions (evaluates to empty string, carries on). The queue-time parser forworkflow_dispatchis stricter and rejects them. mcp-clipboard has the same latent bug — it just never tripped because nobody has dispatched manually there. That's why mcp-clipboard's workflow_run firings continued to work fine throughout.Change (8 / −5, one file, comment-only)
Three comments rewritten to describe the concept without the literal
${{ }}characters:# Going through jq -r avoids the ${{ }} expression surface entirely# Going through jq -r avoids the GitHub Actions expression surface entirely ...# not direct ${{ }} interpolation — fork PR branch names are# not a direct GHA expression — fork PR branch names are# HEAD_BRANCH came from jq (above), not direct ${{ }} interpolation —# HEAD_BRANCH came from jq (above), not a direct GHA expression —Zero logic change. Legitimate
${{ secrets.GITHUB_TOKEN }}and${{ github.repository }}in env: blocks are untouched — they have real content inside the braces and parse correctly.Post-merge
Same dispatch test as before: https://github.com/cmeans/yt-dont-recommend/actions/workflows/pr-labels-ci.yml → Run workflow → main. If the theory holds, the queue no longer errors, both jobs skip (because the job-level
if:evaluates to false onworkflow_dispatch), the workflow registers, andgh api ... --jq '.name'should flip to"PR Label Automation (CI)".Cascade
mcp-clipboard's
pr-labels-ci.ymlhas the identical${{ }}in comments. The fix should propagate there in a follow-up PR to preserve the verbatim contract and to unblock manual dispatch if it's ever needed.Caveat
Same as PRs #24 / #25 / #26 / #27 — this PR itself will stick at
Awaiting CIbecause the fix only activates post-merge-and-seed. One more Dev Active toggle.🤖 Generated with Claude Code