From 20f197a44363b2dc41474eeee574535b600f018f Mon Sep 17 00:00:00 2001 From: Will Griffin Date: Fri, 22 May 2026 12:14:57 -0600 Subject: [PATCH 1/2] docs(checklist): add hazards from real Copilot catches on have-config + pr-review Two waves of Copilot review on this week's PRs surfaced patterns the checklist didn't cover. Folding them in immediately rather than waiting for batched pr-review-tune. Theme 7 (renamed: "Config hazards: dead, surprising, or over-active") gains three new bullets: - Shared config with invisible side effects on consumers. Caught on have-config PR #2: `incremental: true` in a shared tsconfig.base.json silently emits .tsbuildinfo in every consuming tree. Generalizes to any extended/published config (tsconfig, eslint, prettier, vite, biome). - Shared config too narrow for documented use cases. Same PR: tsconfig-base README claimed SvelteKit support but lib was ES2022 only (no DOM). Either narrow the docs, add a variant, or require explicit override. - README install instructions that contradict package.json. Same PR: README told consumers to install three packages that were already optionalDependencies. Causes duplicate installs / version skew. Theme 8 (Infra/deploy hazards) gains three new bullets from pr-review PR #2: - GitHub Actions workflow-command injection from user-controlled content. `::error::` without escaping `%` `\r` `\n` lets a crafted commit message inject workflow commands or spoof logs. Real security issue, not theoretical. - `echo "$user_input" | grep` parses dashes as flags. Subjects starting with `-n`/`-e` get treated as echo options. Use `printf '%s\n'`. - GitHub Actions permissions broader than the workflow needs. pull-requests: read on a workflow that only does git log + event payload reads is dead scope. Least privilege. All five patterns are concrete with file/line citations from the source PRs. Should reduce time spent on the same class of issue in future shared-config and workflow PRs. --- prompts/checklist.md | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/prompts/checklist.md b/prompts/checklist.md index 337996a..baf4d39 100644 --- a/prompts/checklist.md +++ b/prompts/checklist.md @@ -132,7 +132,11 @@ For any PR that touches code referenced by docs *or* updates docs: - **Doc step is grammatically broken or ambiguous** after a recent edit. - **Examples in docs that point at paths or commands the PR moved**. -### 7. Dead config & unwired parameters +### 7. Config hazards: dead, surprising, or over-active + +Config that's missing a consumer is wasteful; config that has *invisible +side effects on consumers* is the inverse problem and is just as +common in shared-config / monorepo / base-config setups. - **Optional function parameter that no call site sets**: the behavior gated by the parameter is unreachable. Either make it required and @@ -141,6 +145,25 @@ For any PR that touches code referenced by docs *or* updates docs: Dockerfile / app**: dead config, easy to mistake for a real secret channel later. - **Feature flags / settings that have no read site** after a refactor. +- **Shared config with invisible side effects on consumers**: a config + consumed by other repos or packages (tsconfig, eslint, prettier, + vite, biome, package.json `scripts`) that turns on a behavior with + consumer-visible side effects without documenting it. Example: + `incremental: true` in a shared `tsconfig.base.json` silently emits + `.tsbuildinfo` files in every consumer's tree. If the behavior would + surprise the consumer, it belongs in the consumer's own config (let + them opt in), not the shared base. +- **Shared config too narrow for the documented use cases**: shipping + a tsconfig that documents itself as appropriate for "SvelteKit apps" + but sets `lib: ["ES2022"]` only (no DOM types) — consumers hit + confusing type errors. Either narrow the documented use case, add a + per-environment variant (browser/node), or require an explicit + override and call it out. +- **README install instructions that contradict package.json**: + README tells consumers to `pnpm add -D foo bar baz` but `foo` and + `bar` are already `optionalDependencies` (installed transitively). + Causes duplicate installation and version skew. Only document + installs the consumer must do themselves. ### 8. Infrastructure & deploy hazards @@ -166,6 +189,22 @@ For changes under `manifests/`, `.github/workflows/`, `infra/`, `iac/`, clear usage error. - **Interpolated shell variables into `psql -c` / `sed` / `perl` substitutions** without escaping — fine today, time-bomb tomorrow. +- **GitHub Actions workflow-command injection from user-controlled + content**: printing a commit subject, PR title, branch name, or any + other event-payload string inside a `::error::` / `::warning::` / + `::notice::` / `::set-output::` line without escaping lets an + attacker inject arbitrary workflow commands via `%` / `\r` / `\n` in + the source string. Escape with `s//\%/%25/`, `s/\r/%0D/`, `s/\n/%0A/` + before printing. (Even non-attack cases — a commit subject containing + `%` will get URL-decoded in the log and confuse you.) +- **`echo "$user_input" | grep` parses dashes as flags**: subjects + starting with `-n`, `-e`, `-E` get treated as echo options by some + shells. Use `printf '%s\n' "$user_input"` instead — safe for any + input. +- **GitHub Actions permissions broader than the workflow needs**: each + `permissions:` entry should map to a real API call the workflow + makes. `pull-requests: read` on a workflow that only reads git log + and event payload is dead scope. Drop it (least privilege). ### 9. Type-system tightening (where it matters) From ba6f1618cc9bc1f61875fda8b40706fbe0ac00f0 Mon Sep 17 00:00:00 2001 From: Will Griffin Date: Fri, 22 May 2026 12:31:14 -0600 Subject: [PATCH 2/2] docs(checklist): fix workflow-command escape syntax + clarify echo bullet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Copilot catches on PR #3: 1. The escape examples were written as pseudo-sed (`s//\%/%25/` etc.) which is malformed and unhelpful. Replaced with the actual bash parameter-expansion helper from the commitlint.yml workflow that triggered this checklist update — concrete, copy-pasteable, and correct. Also linked GitHub's workflow-commands docs as the authoritative source for the encoding requirements. 2. Bullet title attributed the dash-parsing problem to the whole `echo "$user_input" | grep` pipeline; the issue is actually `echo` alone (grep just sees empty stdin). Renamed the bullet to "`echo "$user_input"` treats leading `-n`/`-e`/`-E` as flags" and expanded the body to explain the downstream silent-empty-input failure mode, which is the more dangerous symptom. --- prompts/checklist.md | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/prompts/checklist.md b/prompts/checklist.md index baf4d39..9f3bbe9 100644 --- a/prompts/checklist.md +++ b/prompts/checklist.md @@ -194,13 +194,26 @@ For changes under `manifests/`, `.github/workflows/`, `infra/`, `iac/`, other event-payload string inside a `::error::` / `::warning::` / `::notice::` / `::set-output::` line without escaping lets an attacker inject arbitrary workflow commands via `%` / `\r` / `\n` in - the source string. Escape with `s//\%/%25/`, `s/\r/%0D/`, `s/\n/%0A/` - before printing. (Even non-attack cases — a commit subject containing - `%` will get URL-decoded in the log and confuse you.) -- **`echo "$user_input" | grep` parses dashes as flags**: subjects - starting with `-n`, `-e`, `-E` get treated as echo options by some - shells. Use `printf '%s\n' "$user_input"` instead — safe for any - input. + the source string. Per [GitHub's workflow-commands docs](https://docs.github.com/en/actions/learn-github-actions/workflow-commands-for-github-actions), + apply these replacements before printing user input: `%` → `%25`, + CR → `%0D`, LF → `%0A`. A safe bash helper: + ```bash + escape_workflow_msg() { + local s="$1" + s="${s//\%/%25}" + s="${s//$'\r'/%0D}" + s="${s//$'\n'/%0A}" + printf '%s' "$s" + } + ``` + (Even in non-attack cases, a commit subject containing `%` will get + URL-decoded in the log and confuse you.) +- **`echo "$user_input"` treats leading `-n`/`-e`/`-E` as flags**: when + a commit subject (or any user-controlled string) starts with one of + those, bash builtins and some `/bin/echo` implementations swallow the + argument as an option instead of printing it. Downstream `grep` / + pipelines silently get empty input and a wrong result. Use + `printf '%s\n' "$user_input"` — always safe regardless of content. - **GitHub Actions permissions broader than the workflow needs**: each `permissions:` entry should map to a real API call the workflow makes. `pull-requests: read` on a workflow that only reads git log