chore: declare shell: bash on every skill with dynamic-context injections - #860
Conversation
…ions Fleet sweep closing the gap the pinned precompute convention (plugins/playbooks/skills/skill-authoring/reference/precompute-context.md) names: `!` injection commands here use bash-only constructs (`||`, `2>/dev/null`, pipes to head), and `shell:` left undeclared falls through to the PowerShell tool on a Windows host without Git Bash, silently breaking every probe. 64 skills across 26 plugins gain an explicit `shell: bash` frontmatter declaration (official key, verified against the skills doc); each affected plugin takes a patch bump with a CHANGELOG entry. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 23s —— View job PR Review
Overall: ✅ LGTM — fleet sweep is correct and complete. The PR does exactly what it says: adds Completeness checkCross-referenced every SKILL.md file in the repo against what the PR touched:
The PR description's stated count of "64 skills across 26 plugins" matches exactly. Placement and syntax
Version bumpsAll 26 plugins receive a patch bump ( CHANGELOG entriesEvery bumped plugin carries a new entry at the top of its CHANGELOG with a consistent explanation tied to the convention. The entries correctly reference the precompute convention's "Windows / Incidental changes in plugin.jsonSeveral Convention alignmentThe change directly implements the pinned policy in
No gaps between the convention and the implementation found. |
|
Claude finished @kyle-sexton's task in 2m 33s —— View job Security Review
ScopeThis PR makes exactly two kinds of changes across 100 files: adding FindingsNo security vulnerabilities introduced by this PR. The review covered every changed file category: SKILL.md changes (64 files — each adds The injection commands (
plugin.json changes (26 files — version bumps and keywords array reformatting only) No new capabilities, MCP server entries, subagent definitions, GitHub Actions / CI No workflow files were modified. Net assessmentThe |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af6944e0cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Resolves conflicts on source-control and work-items plugin.json/CHANGELOG.md: main advanced source-control to 0.15.8 (#839, merged) and work-items to 0.18.2 (#826, merged) since this branch was last rebased. Also re-checked live open PRs at merge time and found work-items now carries a new open claim, #857 at 0.19.0 (Jira adapter) — re-bumped this branch's work-items claim from 0.18.3 to 0.19.1 to stay ahead of it. source-control's 0.15.10 and claude-ops's 0.17.3 remain valid (still one past #840/#860's 0.15.9 and #844/#860's 0.17.2 open claims, respectively). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K1V3gkrfSf75isB8MiDy3o
repo-hygiene had no open-PR collision at this PR's original open, but #860 (chore/skill-shell-declaration) opened since and claims 0.4.5 exactly, matching this branch's prior claim. Bump to 0.4.6 to stay ahead of it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K1V3gkrfSf75isB8MiDy3o
…line cap The shell: bash frontmatter line pushed babysit-prs SKILL.md from 499 to the 500-line hard cap; joining one wrapped sentence restores headroom. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…nline-only census Codex review finding: the sweep's census grepped only the inline backtick-injection form and missed stateless's fenced block, whose command is bash-only. Census re-run over both forms confirms this was the single miss. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
docs-hygiene and session-flow both took their next patch number on main while this branch was open; the sweep entries restack as 0.8.3 and 0.12.2.
|
Claude finished @kyle-sexton's task in 1m 14s —— View job PR Review (synchronize)
Incremental review — commits since af6944eThree commits landed after the prior LGTM review. All three are correct. deac426 —
80bda95 — The 4fd2c72 — merge from origin/main Standard merge commit; no content changes beyond what the two fixes introduced. Overall: ✅ LGTM — the two fixes are correct and the PR is ready to merge. The Codex-reported miss (stateless) is resolved, the 500-line cap compliance is restored, and a re-run of the census over both injection forms (inline backtick and fenced block) confirms no remaining gaps. |
|
Claude finished @kyle-sexton's task in 3m 57s —— View job Security Review
ScopeThis PR makes changes across four categories:
No GitHub Actions workflow files were modified. FindingsSKILL.md changes — no new surfacesAll 65 diffs are single-line additions of plugin.json changes — no capability wideningEvery plugin.json diff is a patch version bump and/or a parse-concern-value.sh — one latent concern worth notingSUGGESTION · confidence: PLAUSIBLE raw_line=$(sed -n "s/^${key}:[[:space:]]*//p" "$concern_file" | head -1)
Why this is not exploitable today: every call site in the current codebase passes hardcoded literal keys ( Recommendation: guard the key parameter with a format check at the top of the function to make the documented invariant machine-enforced: if [[ ! "$key" =~ ^[a-z_]+$ ]]; then
echo "parse-concern-value: key must be [a-z_]+ — got: $key" >&2
exit 2
finoise-shapes.sh escaping — correct for current usepattern+="|$(printf '%s' "$val" | sed "s/[.[\\*^\$()+?{|]/\\\\&/g")"This escapes the consumer-controlled Net assessmentThe primary PR changes (65 × No CRITICAL or IMPORTANT security findings. One SUGGESTION (input validation on |
Re-syncs onto main after #860 (fleet-wide `shell: bash` declaration sweep) merged, which bumped source-control to 0.15.9, claude-ops to 0.17.2, and repo-hygiene to 0.4.5. Resolved conflicts on those three plugins' plugin.json and CHANGELOG.md; re-derived each held version against the new main plus a fresh live-open-PR check rather than assuming the prior numbers still held: - source-control 0.15.10: still one past main's 0.15.9 and #840's live 0.15.9 claim. - claude-ops 0.17.3: still one past main's 0.17.2 and #844's live 0.17.2 claim. - repo-hygiene 0.4.6: still one past main's new 0.4.5 (#860, now merged); no open-PR collision remains for this plugin. - work-items 0.19.1 and guardrails 0.9.6 were untouched by this merge (no conflict) and remain correct against #857/#861 (both still 0.19.0, open) and no open collision, respectively. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K1V3gkrfSf75isB8MiDy3o
Integrate #860 (fleet-wide `shell: bash` skill-frontmatter chore, which merged source-control to 0.15.9). Re-derived this PR's bump to 0.15.10 (one past current main) and re-labeled the 0.15.9 CHANGELOG entry to 0.15.10, stacked above the merged-in 0.15.9 (shell: bash) entry. SKILL.md auto-merged (my body edits + #860's frontmatter, distinct regions); babysit-prs skill-quality gate green at 499/500 lines.
Resolves conflicts in plugins/toolchain/.claude-plugin/plugin.json and CHANGELOG.md against #860's fleet-wide 0.5.2 shell-declaration sweep — kept the 0.7.0 lychee-offline entry stacked on top (one past sibling PR #859's 0.6.0 claim on the same plugin). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: 0f69ae76-07e0-4aae-9c4a-11688fc0e8ea
…uction (#864) (#877) ## What Evaluate the lanes skill's deterministic, read-only git probes against the pinned precompute convention (`plugins/playbooks/skills/skill-authoring/reference/precompute-context.md`) and either convert each to `!` dynamic-context injection or record why it fails the four conditions. ## Per-probe verdict | Git probe | Location | Verdict | |---|---|---| | `git rev-parse --show-toplevel` (repo root) | `SKILL.md` front matter | **Already converted** — `!` injection with `\|\| echo` fallback; `shell: bash` declared. | | `git rev-parse --show-toplevel` (lane-config resolution) | `SKILL.md` front matter | **Already converted** — `!` injection with `\|\| echo` fallback. | | staleness probe: `git fetch origin` + `git symbolic-ref` + `git log <lane-launch-commit>..<default> -- <plugin-path>` | `context/refresh.md` | **Recorded reason it fails all four conditions** — stays a body instruction. | The two front-matter probes were `!`-injected at the skill's creation (#616) and gained `shell: bash` in 0.17.2 — already compliant, nothing to convert. ## Why the staleness probe is not an injection candidate It fails **every** convention condition: - **Not deterministic/read-only + not cheap/bounded** — `git fetch` is a network round-trip (latency-variable, can hang) that also updates remote-tracking refs. - **Not needed up front every invocation** — consulted only when weighing a restart, not on a plain `status`/`stop`/`start`. - **Not judgment-independent** — needs a computed `<lane-launch-commit>` argument (the running lane's launch point) plus the pathspec of whichever plugin the lane runs; a single-pass injection cannot supply either. Recorded inline in `context/refresh.md` so a future reader does not re-litigate it. ## Automated gate skill-quality Check 18 (precompute opportunity) scans only `SKILL.md` fenced shell blocks and is silent when the skill already injects with `!` — lanes does — so the gate was already satisfied; this PR adds no new `!` blocks. ## Changes - `context/refresh.md` — durable "Not an `!` injection candidate" note next to the staleness probe. - `claude-ops` version `0.17.2` → `0.17.3` + matching CHANGELOG entry (changelog-parity gate). ## Verification - `check-skill.sh lanes` — PASS (0 errors, 0 warnings; markdownlint clean, 163/500 lines) - `check-changelog-parity.sh --check` and `--check-bump origin/main` — PASS - `validate-plugins.sh` + `claude plugin validate .` — PASS - markdownlint on changed files — 0 errors Closes #864 ## Related - #791 — origin of the side-finding - #616 — original `!` conversion of the front-matter probes - #860 — shell-declaration fleet sweep that added `shell: bash` Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
… skills (#883) Closes #865 ## What Two new checks in `plugins/skill-quality/scripts/check-skill.sh`, guarding the regression the 2026-07-21 fleet census surfaced (64 skills across 26 plugins using `!` injections with bash-only constructs and no `shell:` declaration — silently broken on a Windows host without Git Bash, where injections fall through to the PowerShell tool). PR #860 swept the fleet; nothing prevented regression until now. - **Check 19 — injection shell-declaration.** When a skill carries `!` dynamic-context injections (inline `` !`cmd` `` or ` ```! ` blocks) and declares no `shell:` frontmatter, it FAILs on detectable bash-only syntax and WARNs on portable-looking commands. - **Check 20 — defensive fallback.** WARNs on any injected command with no `|| <fallback>` continuation, per the pinned precompute convention. Eight self-tests cover both tiers, fenced-block extraction, the `||`-not-`|| echo` match, and two over-reach guards. ## FAIL/WARN tiering rationale Portability is not statically decidable, so the tiers split on evidence strength: - **FAIL** only on *detectable* bash-only syntax with no `shell:` — the exact census failure. The bash-only token set is deliberately narrow: `/dev/null` (PowerShell is `$null`), `command -v` (a bash builtin; PowerShell is `Get-Command`), and a pipe into a Unix text tool with no same-named PowerShell cmdlet (`head`, `sed`, `awk`, …; `sort`/`tee` are excluded because PowerShell aliases them). Tight avoids a false FAIL that blocks a PR; anything the set misses degrades to the WARN path — never a false negative that FAILs a portable skill. - **WARN** when injections exist with no `shell:` but the commands only *look* portable — static analysis can't prove it, so nudge rather than block. - **Silent** when `shell:` is declared: the author has taken explicit responsibility for the shell (see scope boundary below). Check 20 is WARN-only (not FAIL) — the fallback convention is a defensive nicety, not a correctness invariant; injection failure/stderr semantics are undocumented, so this is a nudge. ## Why in the plugin's check-skill.sh, not the repo-level check-skill-portability.sh The repo already has `scripts/check-skill-portability.sh`, but that is a flat *token-present-in-file → violation* grep (its concern is ecosystem/forge/branch agnosticism, e.g. a bare `origin/main`). #865 needs a three-way conditional that a flat scanner cannot express: **an injection exists ∧ no `shell:` is declared ∧ a bash-only token sits inside the injected command text** (not anywhere in the file — a bash-only token in a plain ` ```bash ` example must not flag). That per-skill, structure-aware logic belongs with the other skill-contract checks in `check-skill.sh`, which already parses frontmatter and code fences. ## Scope boundary A `shell:` declaration is trusted wholesale — the check does not validate that the injected commands actually match the declared shell, so `shell: pwsh` with bash-only commands is intentionally out of scope (per-shell syntax validation is a separate, much larger concern). Both checks scan the injected command text only; inline injections are recognized only at line start or after whitespace (per the injection docs), so a mid-token `` !` `` such as an inline `` `#!` `` code span in prose is not captured. ## Version `skill-quality` 0.7.2 → **0.8.0** (minor — matches check-18's minor-bump precedent for a new check), with the parity-gated CHANGELOG entry and regenerated README catalog in the same diff. ## Gates run locally (all green) - `check-skill.test.sh` — 47 assertions pass (39 existing + 8 new) - `shellcheck --rcfile=.shellcheckrc` on both scripts — clean - `scripts/validate-plugins.sh` — passes (catalog regenerated) - `scripts/check-changelog-parity.sh --check-bump origin/main` — passes - `markdownlint-cli2` on changed markdown — clean - Self-check of the modified check skill — PASS, all 7 triggers preserved, SKILL.md 107/500 ## Related Related but NOT closed by this PR: - #860 — the fleet sweep that added `shell: bash` to the 64 affected skills; this PR is the regression guard that keeps the census failure from returning. - #531 — the repo-level `check-skill-portability.sh` agnosticism gate (the sibling "portability" gate this PR deliberately does not extend; see the rationale section above). - `plugins/playbooks/skills/skill-authoring/reference/precompute-context.md` — the pinned precompute / defensive-fallback convention that checks 19 and 20 enforce.
…gs for babysit snapshot (#882) ## Summary Fixes #511. The babysit PR-queue snapshot derived `self_logins` — the posting identities whose comments self-classification suppresses — from the `--author` discovery filter, conflating two distinct concerns (*which authors' PRs to discover* vs *whose comments to suppress*). That broke in both directions: - **Under-inclusion:** configured extra self identities (`babysit_self_logins`) rode on `--author`, so when autopilot widening drops `--author` entirely, a bot poster's own comments re-fired `new_human_blocking_feedback` every cycle. - **Over-inclusion:** a `--queue --author alice` run authenticated as a different login made `alice` a self-login, suppressing Alice's genuine feedback from the worker-dispatch arm. ## The fix `pr_queue_snapshot.py` now resolves self-identity from dedicated **`--self`** (full override — exactly the given logins, `@me` not added) / **`--extra-self`** (added on top of the authenticated `@me`) flags, mirroring the existing `babysit-readiness-gate.sh` semantics, resolved **independently of `--author`** and before the `--pr`/`--queue` scope split (so single-PR scope is covered too). This supersedes the `@me`-only union added in #494, and the author-derived self fallback in `build_config` is removed so **no discovery author can leak into the self set** in any path. The skill's step-4 invocation and the `babysit_self_logins` userConfig table row now route the configured extras through `--extra-self` instead of overloading `--author @me,<self-logins>`; because self-identity no longer rides on `--author`, it survives autopilot widening. ## Tests (red-green, with transparency on the rewritten spec) - Added `SelfIdentityDecouplingTests` asserting both directions at the `build_snapshot` level (extra-self survives a dropped `--author`; a discovery author is not treated as self). **Verified these fail against the pre-fix author-union code and pass after the fix.** - The `resolve_self_logins` unit tests pinned the *old* author-union contract; they are rewritten to the new flag-based contract, and `test_discovery_authors_are_unioned_with_the_self_login` (which asserted the over-inclusion behavior now fixed) is deleted. `test_raw_author_fallback_drops_me_when_unresolved` becomes `test_missing_resolved_self_logins_yields_empty_not_author`. These are a ratified spec change (the issue's converged decision), not test weakening. ## Verification (local gates) - `python -m unittest discover -s tests -p 'test_*.py'` (the CI entry via `engine.test.sh`) — **341 tests OK**. - `check-changelog-parity.sh --check-bump origin/main` — PASS (0.16.0 entry present). - `check-skill-portability.sh origin/main` — PASS. - `check-changed-skills.sh` — SKILL.md 499/500 lines (under the hard cap); the one reported "engine.test.sh failed" is the pre-existing repo-wide ruff E402 quirk that does **not** reproduce in CI (main is green; merged babysit PRs #860/#839 pass skill-quality-gate), and this change adds zero new ruff errors (32 before == 32 after). - `markdownlint-cli2` on the changed docs — clean; `plugin.json` valid. - Independent fresh-context code review (producer ≠ reviewer). ## Version Per-plugin bump **0.15.9 → 0.16.0** (minor — new `--self`/`--extra-self` CLI surface alongside the behavior fix). `marketplace.json` pins no version for `source-control`, so only `plugin.json` changes. Closes #511 ## Related - #497 — single-`--pr` scope leaves `self_logins` empty (sibling facet of the same seam, triaged blocked-by #511). This change resolves self-identity for `--pr` scope too, but its acceptance is left for the #497 lane to verify — not closed here. - #494 — added the `@me`-only `--queue` union this supersedes. - #473 — engine-level self-filter (related self-dispatch symptom). - #881 — follow-up (filed): the snapshot raises on an unresolvable `@me` where the readiness gate degrades to the `--extra-self` set; tracks whether fail-loud is the intended contract. Surfaced in this PR's review. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
What
Fleet sweep: every SKILL.md using
!`command`dynamic-context injection now declaresshell: bashin frontmatter — 65 skills across 26 plugins, each plugin patch-bumped with a CHANGELOG entry.Why
The pinned precompute convention (
plugins/playbooks/skills/skill-authoring/reference/precompute-context.md, "Windows /shell:awareness") requires portable injection commands or an explicitshell:declaration. The fleet's injections are bash-only (||fallbacks,2>/dev/null, pipes), and withshell:undeclared they fall through to the PowerShell tool on a Windows host without Git Bash, silently inlining broken output into every skill load. Originally surfaced againstknowledge:youtube-digest; a census showed the same gap in all 64 injection-bearing skills, so the sweep fixes the class, not the instance.shellis the official frontmatter key (bashdefault |powershell), verified against the skills documentation this session.Gates
claude plugin validate .— PASSmarkdownlint-cli2— 0 errors on all changed CHANGELOGsscripts/check-changelog-parity.sh --check— PASS (all 26 bumps carry CHANGELOG entries)skill-quality checksample (youtube-digest) — PASSFollow-up
Enforcement (skill-quality check: injections present ⇒
shell:declared or commands portable) tracked separately.Related
🤖 Generated with Claude Code