From 4544c5d888e94ea1921e1cf9f43a28e7d70e33f2 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Wed, 26 Aug 2026 12:22:29 -0700 Subject: [PATCH 1/6] Add shared simplify skill --- ai/AGENTS.md | 2 +- ai/agents/code-reviewer.md | 6 +++--- ai/skills/go/SKILL.md | 8 ++++---- ai/skills/simplify/SKILL.md | 35 ++++++++++++++++++++++++++++++++++ ai/tests/test-ai-installers.sh | 4 ++++ 5 files changed, 47 insertions(+), 8 deletions(-) create mode 100644 ai/skills/simplify/SKILL.md diff --git a/ai/AGENTS.md b/ai/AGENTS.md index c2f55f0..af1dcbd 100644 --- a/ai/AGENTS.md +++ b/ai/AGENTS.md @@ -25,7 +25,7 @@ If arriving from an approved Plan Mode plan, invoke the `go` skill with `--plan- 2. `unit-test-writer` writes tests first (red) 3. Implement minimal code to pass (green) 4. Refactor with tests passing -5. Run `/simplify` to review changed code, then `comment-cleanup` over the result +5. Run the `simplify` skill to review changed code, then `comment-cleanup` over the result 6. `code-reviewer` before committing After 2 failed attempts, stop and use `bug-root-cause-analyzer`. Don't keep pushing a broken approach. diff --git a/ai/agents/code-reviewer.md b/ai/agents/code-reviewer.md index 52e5391..fdab825 100644 --- a/ai/agents/code-reviewer.md +++ b/ai/agents/code-reviewer.md @@ -1,11 +1,11 @@ --- name: code-reviewer -description: "Reviews code for bugs, logic errors, security vulnerabilities, and project guideline violations. Does not cover readability or refactoring (use /simplify for that). Examples: before committing changes, after implementing a new feature, or when you want a correctness check." +description: "Reviews code for bugs, logic errors, security vulnerabilities, and project guideline violations. Does not cover readability or refactoring (use the simplify skill for that). Examples: before committing changes, after implementing a new feature, or when you want a correctness check." model: opus color: red --- -You are a senior code reviewer focused on correctness and safety. Catch bugs, security issues, and project guideline violations — not refactoring or style improvements (`/simplify` handles those). +You are a senior code reviewer focused on correctness and safety. Catch bugs, security issues, and project guideline violations — not refactoring or style improvements (the `simplify` skill handles those). ## Before You Review @@ -28,7 +28,7 @@ You are a senior code reviewer focused on correctness and safety. Catch bugs, se ## Out of Scope -Do not flag readability, naming aesthetics, redundant code, structural refactoring, comment quality, or stylistic preferences — use `/simplify` for those. +Do not flag readability, naming aesthetics, redundant code, structural refactoring, comment quality, or stylistic preferences — use the `simplify` skill for those. ## Confidence Scoring diff --git a/ai/skills/go/SKILL.md b/ai/skills/go/SKILL.md index 1c47715..dff7b0f 100644 --- a/ai/skills/go/SKILL.md +++ b/ai/skills/go/SKILL.md @@ -77,7 +77,7 @@ When the branch has no upstream, `@{u}` yields nothing — count branch commits - If `implement` was recorded done and the tree is clean with branch commits → also `simplify-commit: `. - Open PR on the branch → `pr: `. - Review steps are never inferred — leave them pending. Re-reviewing already-reviewed work is cheap; skipping an un-run review isn't. -- If the adopted diff (dirty files plus commits since the merge-base with the default branch) touches testable code but no test files, dispatch `unit-test-writer` in the background now, prompted with the diff: write tests for the changed behavior, match existing test conventions, report which fail. Note the gap in the position report. Fold the results in at the next commit — resuming at Step 5, collect after `/simplify` so the tests ride the same commit; resuming later, collect before Step 7 starts, reconcile guessed names against the real code, run the suite, and commit via `Skill("commit", args: "--force Add tests for $SLUG")`. Skip the dispatch for diffs with no testable behavior (docs, config). +- If the adopted diff (dirty files plus commits since the merge-base with the default branch) touches testable code but no test files, dispatch `unit-test-writer` in the background now, prompted with the diff: write tests for the changed behavior, match existing test conventions, report which fail. Note the gap in the position report. Fold the results in at the next commit — resuming at Step 5, collect after the `simplify` skill so the tests ride the same commit; resuming later, collect before Step 7 starts, reconcile guessed names against the real code, run the suite, and commit via `Skill("commit", args: "--force Add tests for $SLUG")`. Skip the dispatch for diffs with no testable behavior (docs, config). - Nothing to resume (clean tree, no branch commits, no PR, no `TASK`) → stop and ask the user what to build. **Work branch guard.** If HEAD is detached or the current branch is the repo's default branch, create and switch to `haacked/$SLUG` before anything commits — uncommitted work carries over with the checkout. If the default branch also had local commits its upstream lacks, they're on the new branch now; point the default branch back at its upstream (`git branch -f origin/`) so the work lives only on the feature branch, and say so in the position report. A branch created here has no PR yet — leave `pr` pending regardless of what the earlier lookup returned. @@ -191,7 +191,7 @@ Append `- implement: done` to the state file. ### Step 5: Quality passes and commit -Invoke `/simplify` (bundled Claude slash command — not a skill). It applies its own fixes. Note anything it flags but declines to change — those items feed the explain-open wrap-up in Step 11. If a Step 2 test-gap dispatch is outstanding, collect it now so the tests ride this commit. +Invoke the `simplify` skill. It applies its own fixes. Note anything it flags but declines to change — those items feed the explain-open wrap-up in Step 11. If a Step 2 test-gap dispatch is outstanding, collect it now so the tests ride this commit. Then clean the comments over the same changes: @@ -212,7 +212,7 @@ Then commit. Use a message that matches the situation: Skill("commit", args: "--force ") ``` -Append `- simplify-commit: ` to the state file — also when `/simplify` and `comment-cleanup` made no changes and there was nothing to commit, so the step doesn't rerun. +Append `- simplify-commit: ` to the state file — also when the `simplify` skill and `comment-cleanup` made no changes and there was nothing to commit, so the step doesn't rerun. ### Step 6: Open a draft PR (if needed) @@ -320,7 +320,7 @@ It watches the PR's checks, reruns flaky failures, and fixes legit ones — comm ### Step 11: Explain open items and report -Gather every loose end the run accumulated: prompt-optimizer suggestions Step 4 declined (under `## Declined prompt suggestions` in the state file), items `/simplify` flagged but didn't change, `review-code` Fix Summary items needing judgment or declined, entries in `.notes/review-skipped.md` (written only by older `review-fix-cycle` runs — usually absent), the `## Held comments` section of the state file, and comments `address-pr-reviews` held for the user rather than acting on. Then have them explained, passing the PR URL so it reads the saved review artifacts rather than relying on this conversation — a long run may have compacted the review out of context: +Gather every loose end the run accumulated: prompt-optimizer suggestions Step 4 declined (under `## Declined prompt suggestions` in the state file), items the `simplify` skill flagged but didn't change, `review-code` Fix Summary items needing judgment or declined, entries in `.notes/review-skipped.md` (written only by older `review-fix-cycle` runs — usually absent), the `## Held comments` section of the state file, and comments `address-pr-reviews` held for the user rather than acting on. Then have them explained, passing the PR URL so it reads the saved review artifacts rather than relying on this conversation — a long run may have compacted the review out of context: ```text Skill("explain-open", args: "") diff --git a/ai/skills/simplify/SKILL.md b/ai/skills/simplify/SKILL.md new file mode 100644 index 0000000..92812ff --- /dev/null +++ b/ai/skills/simplify/SKILL.md @@ -0,0 +1,35 @@ +--- +name: simplify +description: Simplify recently changed code for clarity, consistency, and maintainability while preserving behavior. Use after implementation or when asked to simplify or refactor code without changing what it does. +model: opus +metadata: + execution-tier: deep +--- + +# Simplify + +Improve recently changed code without changing its observable behavior. + +## Scope + +Use the files or code the user names. Otherwise, inspect the working tree and branch diff and focus on code changed for the current task. Preserve unrelated user changes and avoid broad cleanup outside that scope. + +If there is no changed code and the user did not name a target, ask what code to simplify. + +## Review and edit + +Read the applicable repository instructions and nearby code before editing. Apply changes that materially improve clarity, consistency, or maintainability, including: + +- Reduce unnecessary nesting, indirection, duplication, and special cases. +- Remove abstractions or helpers that do not earn their complexity. +- Make data flow, names, and control flow easier to follow. +- Remove comments that only restate the code, while preserving comments that explain non-obvious constraints. +- Align changed code with established project patterns. + +Prefer explicit, readable code over compressed expressions. Do not optimize for fewer lines, introduce speculative abstractions, expand the task, or change public behavior. + +Apply safe improvements directly. Leave ambiguous or behavior-changing opportunities untouched and report them to the user. + +## Verify + +Run the most relevant formatter, linter, and focused tests for the changed code. Finish with a concise summary of material simplifications, verification performed, and anything left unchanged because it requires user judgment. diff --git a/ai/tests/test-ai-installers.sh b/ai/tests/test-ai-installers.sh index d6fc846..515afdf 100755 --- a/ai/tests/test-ai-installers.sh +++ b/ai/tests/test-ai-installers.sh @@ -365,6 +365,10 @@ if run_dispatcher "$both_home" --skills-only; then test -L "$both_home/.claude/skills/$enabled_skill" check "Dispatcher with no routing flag installs Codex skills" \ test -L "$both_home/.agents/skills/$enabled_skill" + check "Dispatcher installs the shared simplify skill for Claude" \ + test -L "$both_home/.claude/skills/simplify" + check "Dispatcher installs the shared simplify skill for Codex" \ + test -L "$both_home/.agents/skills/simplify" else fail "Dispatcher runs both installers by default" fi From 07b2ff07f3d3a4adfff78245af2e0d1344e83515 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Thu, 27 Aug 2026 09:39:14 -0700 Subject: [PATCH 2/6] Keep Claude bundled simplify skill --- ai/README.md | 2 +- ai/claude/excluded-skills.txt | 3 +++ ai/install-claude.sh | 15 +++++++++++++++ ai/tests/test-ai-installers.sh | 19 +++++++++++++++++-- 4 files changed, 36 insertions(+), 3 deletions(-) create mode 100644 ai/claude/excluded-skills.txt diff --git a/ai/README.md b/ai/README.md index 1eec837..12b3a23 100644 --- a/ai/README.md +++ b/ai/README.md @@ -19,7 +19,7 @@ The installers preserve regular files and unmanaged symlinks in the destination ## Shared sources - `AGENTS.md` contains global instructions and is linked as `~/.claude/CLAUDE.md` and `~/.codex/AGENTS.md`. -- `skills/` is linked into both `~/.claude/skills/` and `~/.agents/skills/`. A skill refers to its own scripts relative to its directory so it resolves under either agent; an absolute `~/.dotfiles/…` path means it reaches outside itself, to a repo binary or another skill. Codex-only exclusions live in `codex/excluded-skills.txt`; they cover configuration workflows, orchestrators that still depend on Claude-only slash commands or external Claude skills, and skills whose `allowed-tools` fence matters because they read untrusted input, since Codex has no per-skill tool scoping. +- `skills/` is linked into `~/.claude/skills/` and `~/.agents/skills/`, subject to each platform's exclusions. A skill refers to its own scripts relative to its directory so it resolves under either agent; an absolute `~/.dotfiles/…` path means it reaches outside itself, to a repo binary or another skill. Codex exclusions live in `codex/excluded-skills.txt`; they cover configuration workflows, orchestrators that still depend on Claude-only slash commands or external Claude skills, and skills whose `allowed-tools` fence matters because they read untrusted input, since Codex has no per-skill tool scoping. Claude exclusions live in `claude/excluded-skills.txt`; they prevent personal skills from overriding bundled Claude workflows with the same name. - `agents/` contains the canonical Markdown agent definitions. Claude consumes them directly and `bin/render-codex-agents.py` converts them to Codex TOML. - `mcp-servers.sh` defines the MCP inventory once while each installer uses its platform's registration command. diff --git a/ai/claude/excluded-skills.txt b/ai/claude/excluded-skills.txt new file mode 100644 index 0000000..a7db930 --- /dev/null +++ b/ai/claude/excluded-skills.txt @@ -0,0 +1,3 @@ +# Claude Code bundles these skills. Installing a personal skill with the same name +# overrides the bundled workflow. +simplify diff --git a/ai/install-claude.sh b/ai/install-claude.sh index 7272df4..55943b7 100755 --- a/ai/install-claude.sh +++ b/ai/install-claude.sh @@ -7,6 +7,10 @@ export ZSH=$HOME/.dotfiles . $ZSH/ai/helpers/json-settings.sh . $ZSH/ai/helpers/managed-links.sh +is_excluded_skill() { + grep -Ev '^[[:space:]]*(#|$)' "$ZSH/ai/claude/excluded-skills.txt" | grep -Fxq "$1" +} + # Uninstall function uninstall_claude_config() { info "Uninstalling Claude configuration…" @@ -273,6 +277,17 @@ if [ "$INSTALL_SKILLS" = "true" ]; then [ -d "$skill_dir" ] || continue skill_name=$(basename "$skill_dir") destination=~/.claude/skills/"$skill_name" + if is_excluded_skill "$skill_name"; then + if [ -L "$destination" ]; then + case "$(readlink "$destination")" in + "$ZSH"/ai/skills/*) rm -f "$destination" ;; + *) warning "$destination is unmanaged and still overrides Claude's bundled $skill_name skill; remove it by hand" ;; + esac + elif [ -e "$destination" ]; then + warning "$destination is unmanaged and still overrides Claude's bundled $skill_name skill; remove it by hand" + fi + continue + fi install_managed_link "$skill_dir" "$destination" "$ZSH/ai/skills/" done success "Symlinked skills" diff --git a/ai/tests/test-ai-installers.sh b/ai/tests/test-ai-installers.sh index 515afdf..a6ec155 100755 --- a/ai/tests/test-ai-installers.sh +++ b/ai/tests/test-ai-installers.sh @@ -360,19 +360,34 @@ fi # With no routing flag the dispatcher runs both installers. Every case above routes # to exactly one, so a regression in the fall-through would ship Codex uninstalled. both_home=$(fresh_home dispatch-both) +# An excluded skill may already have a managed link from an earlier install. Remove +# that link so Claude loads its bundled /simplify workflow. +mkdir -p "$both_home/.claude/skills" +ln -s "$both_home/.dotfiles/ai/skills/simplify/" "$both_home/.claude/skills/simplify" if run_dispatcher "$both_home" --skills-only; then check "Dispatcher with no routing flag installs Claude skills" \ test -L "$both_home/.claude/skills/$enabled_skill" check "Dispatcher with no routing flag installs Codex skills" \ test -L "$both_home/.agents/skills/$enabled_skill" - check "Dispatcher installs the shared simplify skill for Claude" \ - test -L "$both_home/.claude/skills/simplify" + check "Dispatcher leaves Claude's bundled simplify skill in place" \ + test ! -e "$both_home/.claude/skills/simplify" check "Dispatcher installs the shared simplify skill for Codex" \ test -L "$both_home/.agents/skills/simplify" else fail "Dispatcher runs both installers by default" fi +excluded_home=$(fresh_home excluded-claude) +excluded_target="${TEST_ROOT}/personal-simplify" +mkdir -p "$excluded_home/.claude/skills" "$excluded_target" +ln -s "$excluded_target" "$excluded_home/.claude/skills/simplify" +if HOME="$excluded_home" "$CLAUDE_INSTALLER" --skills-only >/dev/null 2>&1; then + check_eq "Claude preserves an unmanaged excluded skill" \ + "$(symlink_target "$excluded_home/.claude/skills/simplify")" "$excluded_target" +else + fail "Claude tolerates an unmanaged excluded skill" +fi + # The guard that refuses to replace a real file or directory is what replaced the old # rm -rf. Every unmanaged fixture above is a symlink, so that branch never runs there. claude_dir_home=$(fresh_home guard-claude) From 4b08cd924b0ab0e16b081568cfaf8bd1267fc2ff Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Thu, 27 Aug 2026 14:02:22 -0700 Subject: [PATCH 3/6] Share the excluded-skills parser across installers and tests ai/helpers/excluded-skills.sh holds the one parser both installers and the three skill tests read, so a looser matcher can no longer disagree with an installer about which skills a platform excludes. The Claude installer tests loop over the exclusion lists rather than naming a skill, so a new entry is covered without new assertions. Claude-Session: https://claude.ai/code/session_01KJkXqGq8Jm5Kj9EH8yuxR6 --- README.md | 4 +- ai/helpers/excluded-skills.sh | 18 +++++++++ ai/install-claude.sh | 17 ++++---- ai/install-codex.sh | 10 ++--- ai/skills/simplify/SKILL.md | 4 +- ai/tests/test-ai-installers.sh | 67 ++++++++++++++++++++----------- ai/tests/test-canonical-skills.sh | 11 ++--- ai/tests/test-skill-spec.sh | 10 ++--- 8 files changed, 83 insertions(+), 58 deletions(-) create mode 100644 ai/helpers/excluded-skills.sh diff --git a/README.md b/README.md index e09407a..9c1d030 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,7 @@ This repo ships a fair amount of tooling: shared AI skills and subagents, shell ### AI skills -Skills live in [`ai/skills/`](ai/skills). The installer symlinks the same directories into `~/.claude/skills/` and `~/.agents/skills/`, so both platforms use one canonical source. Each skill is a self-contained directory with a `SKILL.md` and any supporting scripts. +Skills live in [`ai/skills/`](ai/skills). The installer symlinks the same directories into `~/.claude/skills/` and `~/.agents/skills/`, so both platforms use one canonical source, minus each platform's exclusions in `ai/claude/excluded-skills.txt` and `ai/codex/excluded-skills.txt`. Each skill is a self-contained directory with a `SKILL.md` and any supporting scripts. | Skill | What it does | | ------- | ------------ | @@ -82,7 +82,7 @@ Subagents live in [`ai/agents/`](ai/agents). Claude uses the Markdown definition | Agent | When to use it | | ------- | -------------- | | [`bug-root-cause-analyzer`](ai/agents/bug-root-cause-analyzer.md) | Failing tests, intermittent bugs, or environment-specific defects that need a systematic investigation. | -| [`code-reviewer`](ai/agents/code-reviewer.md) | Pre-commit correctness, security, and guideline review (use `/simplify` for readability). | +| [`code-reviewer`](ai/agents/code-reviewer.md) | Pre-commit correctness, security, and guideline review (use the `simplify` skill for readability). | | [`implementation-planner`](ai/agents/implementation-planner.md) | Break down complex features into staged technical plans before writing code. | | [`investigator`](ai/agents/investigator.md) | Investigate a single operational hypothesis using Grafana, Prometheus, Loki, and PostHog data. Spawn in parallel for multi-hypothesis incident reviews. | | [`note-taker`](ai/agents/note-taker.md) | Preserve non-obvious technical discoveries after a long exploration session. | diff --git a/ai/helpers/excluded-skills.sh b/ai/helpers/excluded-skills.sh new file mode 100644 index 0000000..1430c3d --- /dev/null +++ b/ai/helpers/excluded-skills.sh @@ -0,0 +1,18 @@ +#!/bin/sh + +# Shared parser for the per-platform excluded-skills lists. +# +# Both installers and the skill tests read the lists through this one function. A +# looser matcher in any one of them would call a skill excluded that the installer +# still ships, and the mismatch surfaces as an unrelated assertion about a missing +# symlink. + +# is_excluded_skill SKILL_NAME EXCLUSIONS_FILE +# +# Succeeds when SKILL_NAME occupies a whole line of EXCLUSIONS_FILE. Blank lines and +# lines whose first non-space character is `#` are skipped. A file that does not +# exist excludes nothing. +is_excluded_skill() { + [ -f "$2" ] || return 1 + grep -Ev '^[[:space:]]*(#|$)' "$2" | grep -Fxq "$1" +} diff --git a/ai/install-claude.sh b/ai/install-claude.sh index 55943b7..43347a4 100755 --- a/ai/install-claude.sh +++ b/ai/install-claude.sh @@ -6,10 +6,9 @@ export ZSH=$HOME/.dotfiles . $ZSH/ai/helpers/output.sh . $ZSH/ai/helpers/json-settings.sh . $ZSH/ai/helpers/managed-links.sh +. $ZSH/ai/helpers/excluded-skills.sh -is_excluded_skill() { - grep -Ev '^[[:space:]]*(#|$)' "$ZSH/ai/claude/excluded-skills.txt" | grep -Fxq "$1" -} +CLAUDE_EXCLUSIONS="$ZSH/ai/claude/excluded-skills.txt" # Uninstall function uninstall_claude_config() { @@ -277,13 +276,11 @@ if [ "$INSTALL_SKILLS" = "true" ]; then [ -d "$skill_dir" ] || continue skill_name=$(basename "$skill_dir") destination=~/.claude/skills/"$skill_name" - if is_excluded_skill "$skill_name"; then - if [ -L "$destination" ]; then - case "$(readlink "$destination")" in - "$ZSH"/ai/skills/*) rm -f "$destination" ;; - *) warning "$destination is unmanaged and still overrides Claude's bundled $skill_name skill; remove it by hand" ;; - esac - elif [ -e "$destination" ]; then + if is_excluded_skill "$skill_name" "$CLAUDE_EXCLUSIONS"; then + case "$(readlink "$destination")" in + "$ZSH"/ai/skills/*) rm -f "$destination" ;; + esac + if [ -e "$destination" ] || [ -L "$destination" ]; then warning "$destination is unmanaged and still overrides Claude's bundled $skill_name skill; remove it by hand" fi continue diff --git a/ai/install-codex.sh b/ai/install-codex.sh index 7e433e4..d1f08ac 100755 --- a/ai/install-codex.sh +++ b/ai/install-codex.sh @@ -3,12 +3,15 @@ set -eu DOTFILES_ROOT="$HOME/.dotfiles" +CODEX_EXCLUSIONS="$DOTFILES_ROOT/ai/codex/excluded-skills.txt" # shellcheck source=/dev/null . "$DOTFILES_ROOT/ai/helpers/output.sh" # shellcheck source=/dev/null . "$DOTFILES_ROOT/ai/helpers/managed-links.sh" # shellcheck source=/dev/null +. "$DOTFILES_ROOT/ai/helpers/excluded-skills.sh" +# shellcheck source=/dev/null . "$DOTFILES_ROOT/ai/mcp-servers.sh" UNINSTALL=false @@ -78,11 +81,6 @@ while [ $# -gt 0 ]; do shift done -is_excluded_skill() { - skill_name="$1" - grep -Ev '^[[:space:]]*(#|$)' "$DOTFILES_ROOT/ai/codex/excluded-skills.txt" | grep -Fxq "$skill_name" -} - remove_managed_skill_links() { [ -d "$HOME/.agents/skills" ] || return 0 for link in "$HOME"/.agents/skills/*; do @@ -148,7 +146,7 @@ if [ "$INSTALL_SKILLS" = "true" ]; then [ -d "$skill_dir" ] || continue skill_name=$(basename "$skill_dir") destination="$HOME/.agents/skills/$skill_name" - if is_excluded_skill "$skill_name"; then + if is_excluded_skill "$skill_name" "$CODEX_EXCLUSIONS"; then # remove_managed_skill_links only removes symlinks, so an excluded skill that # predates the exclusion survives as a real directory and Codex keeps loading it. if [ -e "$destination" ] || [ -L "$destination" ]; then diff --git a/ai/skills/simplify/SKILL.md b/ai/skills/simplify/SKILL.md index 92812ff..f84425f 100644 --- a/ai/skills/simplify/SKILL.md +++ b/ai/skills/simplify/SKILL.md @@ -22,9 +22,7 @@ Read the applicable repository instructions and nearby code before editing. Appl - Reduce unnecessary nesting, indirection, duplication, and special cases. - Remove abstractions or helpers that do not earn their complexity. -- Make data flow, names, and control flow easier to follow. -- Remove comments that only restate the code, while preserving comments that explain non-obvious constraints. -- Align changed code with established project patterns. +- Replace names and control flow that obscure what the code does. Prefer explicit, readable code over compressed expressions. Do not optimize for fewer lines, introduce speculative abstractions, expand the task, or change public behavior. diff --git a/ai/tests/test-ai-installers.sh b/ai/tests/test-ai-installers.sh index a6ec155..411b61a 100755 --- a/ai/tests/test-ai-installers.sh +++ b/ai/tests/test-ai-installers.sh @@ -86,22 +86,25 @@ file_lacks() { # path fixed-string [[ -f "$1" ]] && ! grep -Fq "$2" "$1" } -EXCLUSIONS="${REPO_ROOT}/ai/codex/excluded-skills.txt" - -# Same parse as is_excluded_skill in ai/install-codex.sh. Keep the three in step: -# a looser matcher here would call a skill excluded that the installer still ships, -# and the mismatch surfaces as an unrelated assertion about a missing symlink. -is_excluded_skill() { # skill_name - [[ -f "$EXCLUSIONS" ]] || return 1 - grep -Ev '^[[:space:]]*(#|$)' "$EXCLUSIONS" | grep -Fxq "$1" -} +CODEX_EXCLUSIONS="${REPO_ROOT}/ai/codex/excluded-skills.txt" +CLAUDE_EXCLUSIONS="${REPO_ROOT}/ai/claude/excluded-skills.txt" + +# shellcheck source=/dev/null +. "${REPO_ROOT}/ai/helpers/excluded-skills.sh" +# The Claude assertions below loop over this file, so a missing one would leave the +# exclusion branch untested while every loop reported success. +check "Claude exclusions are declared" test -f "$CLAUDE_EXCLUSIONS" + +# The subject of every "installs a skill" assertion below, so it has to be a skill +# both platforms ship. first_enabled_skill() { local skill name for skill in "${REPO_ROOT}"/ai/skills/*; do [[ -d "$skill" ]] || continue name=$(basename "$skill") - if ! is_excluded_skill "$name"; then + if ! is_excluded_skill "$name" "$CODEX_EXCLUSIONS" && + ! is_excluded_skill "$name" "$CLAUDE_EXCLUSIONS"; then printf '%s\n' "$name" return fi @@ -155,15 +158,15 @@ else fail "Codex skill installation succeeds" fi -if [[ -f "$EXCLUSIONS" ]]; then +if [[ -f "$CODEX_EXCLUSIONS" ]]; then while IFS= read -r excluded; do - is_excluded_skill "$excluded" || continue + is_excluded_skill "$excluded" "$CODEX_EXCLUSIONS" || continue if [[ ! -e "$FAKE_HOME/.agents/skills/$excluded" && ! -L "$FAKE_HOME/.agents/skills/$excluded" ]]; then pass else fail "Codex excludes skill $excluded" fi - done <"$EXCLUSIONS" + done <"$CODEX_EXCLUSIONS" else fail "Codex exclusions are declared" fi @@ -360,30 +363,46 @@ fi # With no routing flag the dispatcher runs both installers. Every case above routes # to exactly one, so a regression in the fall-through would ship Codex uninstalled. both_home=$(fresh_home dispatch-both) -# An excluded skill may already have a managed link from an earlier install. Remove -# that link so Claude loads its bundled /simplify workflow. +# Seeded with a managed link for every Claude-excluded skill, which is what an install +# that predates the exclusion leaves behind. Running both installers over one home is +# also the only place the two platforms can be seen disagreeing about the same skill: +# Claude has to drop the link so its bundled workflow loads, Codex still ships ours. mkdir -p "$both_home/.claude/skills" -ln -s "$both_home/.dotfiles/ai/skills/simplify/" "$both_home/.claude/skills/simplify" +while IFS= read -r excluded; do + is_excluded_skill "$excluded" "$CLAUDE_EXCLUSIONS" || continue + ln -s "$both_home/.dotfiles/ai/skills/$excluded/" "$both_home/.claude/skills/$excluded" +done <"$CLAUDE_EXCLUSIONS" if run_dispatcher "$both_home" --skills-only; then check "Dispatcher with no routing flag installs Claude skills" \ test -L "$both_home/.claude/skills/$enabled_skill" check "Dispatcher with no routing flag installs Codex skills" \ test -L "$both_home/.agents/skills/$enabled_skill" - check "Dispatcher leaves Claude's bundled simplify skill in place" \ - test ! -e "$both_home/.claude/skills/simplify" - check "Dispatcher installs the shared simplify skill for Codex" \ - test -L "$both_home/.agents/skills/simplify" + while IFS= read -r excluded; do + is_excluded_skill "$excluded" "$CLAUDE_EXCLUSIONS" || continue + check "Claude drops its managed link for excluded skill $excluded" \ + test ! -e "$both_home/.claude/skills/$excluded" + check "Codex still installs shared skill $excluded" \ + test -L "$both_home/.agents/skills/$excluded" + done <"$CLAUDE_EXCLUSIONS" else fail "Dispatcher runs both installers by default" fi +# A link the user made by hand is not ours to delete, so an excluded name pointing +# outside the repo survives and the installer only warns about it. excluded_home=$(fresh_home excluded-claude) -excluded_target="${TEST_ROOT}/personal-simplify" +excluded_target="${TEST_ROOT}/personal-skill" mkdir -p "$excluded_home/.claude/skills" "$excluded_target" -ln -s "$excluded_target" "$excluded_home/.claude/skills/simplify" +while IFS= read -r excluded; do + is_excluded_skill "$excluded" "$CLAUDE_EXCLUSIONS" || continue + ln -s "$excluded_target" "$excluded_home/.claude/skills/$excluded" +done <"$CLAUDE_EXCLUSIONS" if HOME="$excluded_home" "$CLAUDE_INSTALLER" --skills-only >/dev/null 2>&1; then - check_eq "Claude preserves an unmanaged excluded skill" \ - "$(symlink_target "$excluded_home/.claude/skills/simplify")" "$excluded_target" + while IFS= read -r excluded; do + is_excluded_skill "$excluded" "$CLAUDE_EXCLUSIONS" || continue + check_eq "Claude preserves an unmanaged link at excluded skill $excluded" \ + "$(symlink_target "$excluded_home/.claude/skills/$excluded")" "$excluded_target" + done <"$CLAUDE_EXCLUSIONS" else fail "Claude tolerates an unmanaged excluded skill" fi diff --git a/ai/tests/test-canonical-skills.sh b/ai/tests/test-canonical-skills.sh index e31b455..a0babad 100755 --- a/ai/tests/test-canonical-skills.sh +++ b/ai/tests/test-canonical-skills.sh @@ -18,17 +18,14 @@ failures=0 skills_checked=0 tiers_checked=0 -# Same parse as is_excluded_skill in ai/install-codex.sh. A looser matcher here would -# exempt a skill the installer still ships to Codex. -is_excluded_skill() { # skill_name - grep -Ev '^[[:space:]]*(#|$)' "$EXCLUSIONS" | grep -Fxq "$1" -} +# shellcheck source=/dev/null +. "${AI_DIR}/helpers/excluded-skills.sh" matches="" for skill_dir in "$SKILLS_DIR"/*; do [[ -d "$skill_dir" ]] || continue skill_name=$(basename "$skill_dir") - if is_excluded_skill "$skill_name"; then + if is_excluded_skill "$skill_name" "$EXCLUSIONS"; then continue fi skills_checked=$((skills_checked + 1)) @@ -155,7 +152,7 @@ for skill_dir in "$SKILLS_DIR"/*; do [[ -d "$skill_dir" ]] || continue skill_name=$(basename "$skill_dir") # Excluded skills may point at Claude-only helpers that live outside this repo. - is_excluded_skill "$skill_name" && continue + is_excluded_skill "$skill_name" "$EXCLUSIONS" && continue skill_file="$skill_dir/SKILL.md" [[ -f "$skill_file" ]] || continue # Drop absolute references first: their tails look exactly like relative ones, and diff --git a/ai/tests/test-skill-spec.sh b/ai/tests/test-skill-spec.sh index 0d87d04..407e4e7 100755 --- a/ai/tests/test-skill-spec.sh +++ b/ai/tests/test-skill-spec.sh @@ -15,10 +15,8 @@ README="$(cd "${SCRIPT_DIR}/../.." && pwd)/README.md" passes=0 failures=0 -# Same parse as is_excluded_skill in ai/install-codex.sh. -is_excluded_skill() { - grep -Ev '^[[:space:]]*(#|$)' "$EXCLUSIONS" | grep -Fxq "$1" -} +# shellcheck source=/dev/null +. "${SCRIPT_DIR}/../helpers/excluded-skills.sh" allowed_keys="name description license compatibility metadata allowed-tools argument-hint model color disable-model-invocation" @@ -60,10 +58,10 @@ for skill_dir in "$SKILLS_DIR"/*; do # `compatibility` marks a skill as Claude-only, which must track the # codex/excluded-skills.txt membership that install-codex.sh reads; drift # in either direction is silent, so enforce both. - if printf '%s\n' "$fm_keys" | grep -Fxq compatibility && ! is_excluded_skill "$skill_name"; then + if printf '%s\n' "$fm_keys" | grep -Fxq compatibility && ! is_excluded_skill "$skill_name" "$EXCLUSIONS"; then problems+=" declares compatibility but is not in codex/excluded-skills.txt"$'\n' fi - if ! printf '%s\n' "$fm_keys" | grep -Fxq compatibility && is_excluded_skill "$skill_name"; then + if ! printf '%s\n' "$fm_keys" | grep -Fxq compatibility && is_excluded_skill "$skill_name" "$EXCLUSIONS"; then problems+=" excluded from Codex but missing compatibility frontmatter"$'\n' fi while IFS= read -r key; do From 0dc3fb90eaa91e943ed31be97bfcec2ccdde3838 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Thu, 27 Aug 2026 14:09:51 -0700 Subject: [PATCH 4/6] Ship Codex-only skills from ai/codex/skills/ Claude bundles a simplify workflow under that name, so ours goes to Codex alone. Its location decides that: the Codex installer walks both skill roots into one destination, and the Claude installer's loop carries no per-platform branch. The skill tests validate both roots, so a Codex-only skill still meets the spec and still pairs its model with a matching execution tier. remove_managed_link in ai/helpers/managed-links.sh decides what this repo owns for every uninstall sweep, so it and install_managed_link share one definition. Claude-Session: https://claude.ai/code/session_01KJkXqGq8Jm5Kj9EH8yuxR6 --- README.md | 3 +- ai/README.md | 3 +- ai/claude/excluded-skills.txt | 3 -- ai/{ => codex}/skills/simplify/SKILL.md | 0 ai/helpers/managed-links.sh | 30 +++++++++++ ai/install-claude.sh | 38 +++----------- ai/install-codex.sh | 38 +++++++------- ai/tests/test-ai-installers.sh | 68 +++++++++---------------- ai/tests/test-canonical-skills.sh | 4 +- ai/tests/test-skill-spec.sh | 24 ++++++--- 10 files changed, 104 insertions(+), 107 deletions(-) delete mode 100644 ai/claude/excluded-skills.txt rename ai/{ => codex}/skills/simplify/SKILL.md (100%) diff --git a/README.md b/README.md index 9c1d030..63a76df 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,7 @@ This repo ships a fair amount of tooling: shared AI skills and subagents, shell ### AI skills -Skills live in [`ai/skills/`](ai/skills). The installer symlinks the same directories into `~/.claude/skills/` and `~/.agents/skills/`, so both platforms use one canonical source, minus each platform's exclusions in `ai/claude/excluded-skills.txt` and `ai/codex/excluded-skills.txt`. Each skill is a self-contained directory with a `SKILL.md` and any supporting scripts. +Skills live in [`ai/skills/`](ai/skills). The installer symlinks the same directories into `~/.claude/skills/` and `~/.agents/skills/`, so both platforms use one canonical source, minus the Codex exclusions in `ai/codex/excluded-skills.txt`. [`ai/codex/skills/`](ai/codex/skills) holds the few skills only Codex gets, because Claude already bundles its own under the same name. Each skill is a self-contained directory with a `SKILL.md` and any supporting scripts. | Skill | What it does | | ------- | ------------ | @@ -64,6 +64,7 @@ Skills live in [`ai/skills/`](ai/skills). The installer symlinks the same direct | [`quarterly-planning`](ai/skills/quarterly-planning) | Draft quarterly goals for a PostHog team, walking the HOGS framework from issues and strategy docs. | | [`resolve-conflicts`](ai/skills/resolve-conflicts) | Resolve git conflicts with mergiraf structural merging, lock file handling, stacked PR dedup. | | [`review-fix-cycle`](ai/skills/review-fix-cycle) | One review, fix, simplify, clean comments, commit iteration. | +| [`simplify`](ai/codex/skills/simplify) | Simplify recently changed code for clarity and maintainability without changing behavior. Codex only; Claude bundles its own. | | [`sprint-planning`](ai/skills/sprint-planning) | Bi-weekly sprint planning updates for the Feature Flags Platform team. | | [`squash`](ai/skills/squash) | Squash each contributor's run of contiguous commits on the branch into one, preserving authorship. | | [`standup`](ai/skills/standup) | Generate standup notes from your recent GitHub PR activity. | diff --git a/ai/README.md b/ai/README.md index 12b3a23..8cb0493 100644 --- a/ai/README.md +++ b/ai/README.md @@ -19,7 +19,8 @@ The installers preserve regular files and unmanaged symlinks in the destination ## Shared sources - `AGENTS.md` contains global instructions and is linked as `~/.claude/CLAUDE.md` and `~/.codex/AGENTS.md`. -- `skills/` is linked into `~/.claude/skills/` and `~/.agents/skills/`, subject to each platform's exclusions. A skill refers to its own scripts relative to its directory so it resolves under either agent; an absolute `~/.dotfiles/…` path means it reaches outside itself, to a repo binary or another skill. Codex exclusions live in `codex/excluded-skills.txt`; they cover configuration workflows, orchestrators that still depend on Claude-only slash commands or external Claude skills, and skills whose `allowed-tools` fence matters because they read untrusted input, since Codex has no per-skill tool scoping. Claude exclusions live in `claude/excluded-skills.txt`; they prevent personal skills from overriding bundled Claude workflows with the same name. +- `skills/` is linked into both `~/.claude/skills/` and `~/.agents/skills/`. A skill refers to its own scripts relative to its directory so it resolves under either agent; an absolute `~/.dotfiles/…` path means it reaches outside itself, to a repo binary or another skill. Codex exclusions live in `codex/excluded-skills.txt`; they cover configuration workflows, orchestrators that still depend on Claude-only slash commands or external Claude skills, and skills whose `allowed-tools` fence matters because they read untrusted input, since Codex has no per-skill tool scoping. +- `codex/skills/` is linked into `~/.agents/skills/` only. It holds skills that exist because Claude bundles its own workflow under that name, so shipping ours to Claude would override the bundled one. Both roots land in the same destination directory, so a name may appear in only one of them. - `agents/` contains the canonical Markdown agent definitions. Claude consumes them directly and `bin/render-codex-agents.py` converts them to Codex TOML. - `mcp-servers.sh` defines the MCP inventory once while each installer uses its platform's registration command. diff --git a/ai/claude/excluded-skills.txt b/ai/claude/excluded-skills.txt deleted file mode 100644 index a7db930..0000000 --- a/ai/claude/excluded-skills.txt +++ /dev/null @@ -1,3 +0,0 @@ -# Claude Code bundles these skills. Installing a personal skill with the same name -# overrides the bundled workflow. -simplify diff --git a/ai/skills/simplify/SKILL.md b/ai/codex/skills/simplify/SKILL.md similarity index 100% rename from ai/skills/simplify/SKILL.md rename to ai/codex/skills/simplify/SKILL.md diff --git a/ai/helpers/managed-links.sh b/ai/helpers/managed-links.sh index aaa8590..3245d33 100644 --- a/ai/helpers/managed-links.sh +++ b/ai/helpers/managed-links.sh @@ -35,3 +35,33 @@ install_managed_link() { ln -s "$source_path" "$destination" fi } + +# remove_managed_link DESTINATION MANAGED_PREFIX [MANAGED_PREFIX...] +# +# Delete DESTINATION when it is a symlink this repo owns, meaning its target starts +# with one of the MANAGED_PREFIX values. Prefixes are matched the same way +# install_managed_link matches them, so the two agree on what "ours" means. +# +# Returns 0 when DESTINATION is gone, whether it was removed or was never there, and +# 1 when something this repo does not own still occupies it, so callers can warn +# about what they left behind. Callers under `set -e` must invoke this in an `if` or +# `||` context; a bare call aborts on the first destination it cannot own. +remove_managed_link() { + local destination="$1" + shift + local link_target prefix + if [ -L "$destination" ]; then + link_target=$(readlink "$destination") + for prefix in "$@"; do + case "$link_target" in + "$prefix"*) + rm -f "$destination" + return 0 + ;; + esac + done + fi + # A dangling symlink fails -e, so both tests are needed to spot a survivor. + [ -e "$destination" ] || [ -L "$destination" ] || return 0 + return 1 +} diff --git a/ai/install-claude.sh b/ai/install-claude.sh index 43347a4..35c6832 100755 --- a/ai/install-claude.sh +++ b/ai/install-claude.sh @@ -6,9 +6,6 @@ export ZSH=$HOME/.dotfiles . $ZSH/ai/helpers/output.sh . $ZSH/ai/helpers/json-settings.sh . $ZSH/ai/helpers/managed-links.sh -. $ZSH/ai/helpers/excluded-skills.sh - -CLAUDE_EXCLUSIONS="$ZSH/ai/claude/excluded-skills.txt" # Uninstall function uninstall_claude_config() { @@ -17,12 +14,9 @@ uninstall_claude_config() { # Remove CLAUDE.md symlink if [ "$INSTALL_CLAUDE_MD" = "true" ]; then if [ -L ~/.claude/CLAUDE.md ]; then - case "$(readlink ~/.claude/CLAUDE.md)" in - "$ZSH"/ai/AGENTS.md|"$ZSH"/ai/CLAUDE.md) - rm -f ~/.claude/CLAUDE.md - success "Removed CLAUDE.md symlink" - ;; - esac + if remove_managed_link ~/.claude/CLAUDE.md "$ZSH/ai/AGENTS.md" "$ZSH/ai/CLAUDE.md"; then + success "Removed CLAUDE.md symlink" + fi elif [ -f ~/.claude/CLAUDE.md ]; then warning "~/.claude/CLAUDE.md is a regular file, not a symlink - skipping" fi @@ -32,11 +26,7 @@ uninstall_claude_config() { if [ "$INSTALL_AGENTS" = "true" ]; then if [ -d ~/.claude/agents ]; then for agent in ~/.claude/agents/*.*; do - if [ -L "$agent" ]; then - case "$(readlink "$agent")" in - "$ZSH"/ai/agents/*) rm -f "$agent" ;; - esac - fi + remove_managed_link "$agent" "$ZSH/ai/agents/" done success "Removed agent symlinks" fi @@ -45,15 +35,10 @@ uninstall_claude_config() { # Remove skill symlinks and contexts if [ "$INSTALL_SKILLS" = "true" ]; then if [ -d ~/.claude/skills ]; then + # The glob must not be restricted to */, or a broken symlink left behind + # by a renamed or deleted skill fails -d and survives the sweep. for skill in ~/.claude/skills/*; do - # Remove any skill symlink, including broken ones left behind by - # a renamed or deleted skill. A dangling symlink fails -d/-e, so - # the glob must not be restricted to */ and the test must be -L. - if [ -L "$skill" ]; then - case "$(readlink "$skill")" in - "$ZSH"/ai/skills/*) rm -f "$skill" ;; - esac - fi + remove_managed_link "$skill" "$ZSH/ai/skills/" done success "Removed skill symlinks" fi @@ -276,15 +261,6 @@ if [ "$INSTALL_SKILLS" = "true" ]; then [ -d "$skill_dir" ] || continue skill_name=$(basename "$skill_dir") destination=~/.claude/skills/"$skill_name" - if is_excluded_skill "$skill_name" "$CLAUDE_EXCLUSIONS"; then - case "$(readlink "$destination")" in - "$ZSH"/ai/skills/*) rm -f "$destination" ;; - esac - if [ -e "$destination" ] || [ -L "$destination" ]; then - warning "$destination is unmanaged and still overrides Claude's bundled $skill_name skill; remove it by hand" - fi - continue - fi install_managed_link "$skill_dir" "$destination" "$ZSH/ai/skills/" done success "Symlinked skills" diff --git a/ai/install-codex.sh b/ai/install-codex.sh index d1f08ac..899c762 100755 --- a/ai/install-codex.sh +++ b/ai/install-codex.sh @@ -84,10 +84,7 @@ done remove_managed_skill_links() { [ -d "$HOME/.agents/skills" ] || return 0 for link in "$HOME"/.agents/skills/*; do - [ -L "$link" ] || continue - case "$(readlink "$link")" in - "$DOTFILES_ROOT"/ai/skills/*) rm -f "$link" ;; - esac + remove_managed_link "$link" "$DOTFILES_ROOT/ai/skills/" "$DOTFILES_ROOT/ai/codex/skills/" || true done } @@ -101,9 +98,7 @@ remove_managed_agents() { for agent in "$HOME"/.codex/agents/*.toml; do [ -e "$agent" ] || [ -L "$agent" ] || continue if [ -L "$agent" ]; then - case "$(readlink "$agent")" in - "$HOME"/.codex/.dotfiles-agents/* | "$HOME"/.codex/agents/.dotfiles/*) rm -f "$agent" ;; - esac + remove_managed_link "$agent" "$HOME/.codex/.dotfiles-agents/" "$HOME/.codex/agents/.dotfiles/" || true elif [ "$(head -n 1 "$agent")" = "$MANAGED_AGENT_HEADER" ]; then rm -f "$agent" fi @@ -142,19 +137,24 @@ if [ "$INSTALL_SKILLS" = "true" ]; then # excluded-skills.txt, stops being linked on an existing install. remove_managed_skill_links shadowed_skills="" - for skill_dir in "$DOTFILES_ROOT"/ai/skills/*/; do - [ -d "$skill_dir" ] || continue - skill_name=$(basename "$skill_dir") - destination="$HOME/.agents/skills/$skill_name" - if is_excluded_skill "$skill_name" "$CODEX_EXCLUSIONS"; then - # remove_managed_skill_links only removes symlinks, so an excluded skill that - # predates the exclusion survives as a real directory and Codex keeps loading it. - if [ -e "$destination" ] || [ -L "$destination" ]; then - warning "$skill_name is excluded from Codex but $destination still exists; remove it by hand" + # ai/codex/skills/ holds skills only Codex gets, because Claude bundles its own + # under the same name. Both roots land in one destination directory, so a name may + # appear in only one of them. + for skills_root in "$DOTFILES_ROOT/ai/skills" "$DOTFILES_ROOT/ai/codex/skills"; do + for skill_dir in "$skills_root"/*/; do + [ -d "$skill_dir" ] || continue + skill_name=$(basename "$skill_dir") + destination="$HOME/.agents/skills/$skill_name" + if is_excluded_skill "$skill_name" "$CODEX_EXCLUSIONS"; then + # remove_managed_skill_links only removes symlinks, so an excluded skill that + # predates the exclusion survives as a real directory and Codex keeps loading it. + if [ -e "$destination" ] || [ -L "$destination" ]; then + warning "$skill_name is excluded from Codex but $destination still exists; remove it by hand" + fi + elif ! install_managed_link "$skill_dir" "$destination" "$skills_root/"; then + shadowed_skills="$shadowed_skills $skill_name" fi - elif ! install_managed_link "$skill_dir" "$destination" "$DOTFILES_ROOT/ai/skills/"; then - shadowed_skills="$shadowed_skills $skill_name" - fi + done done if [ -n "$shadowed_skills" ]; then error "Not linked, a directory already occupies the destination:$shadowed_skills" diff --git a/ai/tests/test-ai-installers.sh b/ai/tests/test-ai-installers.sh index 411b61a..02a00ac 100755 --- a/ai/tests/test-ai-installers.sh +++ b/ai/tests/test-ai-installers.sh @@ -87,30 +87,34 @@ file_lacks() { # path fixed-string } CODEX_EXCLUSIONS="${REPO_ROOT}/ai/codex/excluded-skills.txt" -CLAUDE_EXCLUSIONS="${REPO_ROOT}/ai/claude/excluded-skills.txt" # shellcheck source=/dev/null . "${REPO_ROOT}/ai/helpers/excluded-skills.sh" -# The Claude assertions below loop over this file, so a missing one would leave the -# exclusion branch untested while every loop reported success. -check "Claude exclusions are declared" test -f "$CLAUDE_EXCLUSIONS" - -# The subject of every "installs a skill" assertion below, so it has to be a skill -# both platforms ship. +# The subject of every "installs a skill" assertion below, so it has to be a shared +# skill both platforms ship. first_enabled_skill() { local skill name for skill in "${REPO_ROOT}"/ai/skills/*; do [[ -d "$skill" ]] || continue name=$(basename "$skill") - if ! is_excluded_skill "$name" "$CODEX_EXCLUSIONS" && - ! is_excluded_skill "$name" "$CLAUDE_EXCLUSIONS"; then + if ! is_excluded_skill "$name" "$CODEX_EXCLUSIONS"; then printf '%s\n' "$name" return fi done } +# Skills only Codex gets, because Claude bundles its own under the same name. +first_codex_only_skill() { + local skill + for skill in "${REPO_ROOT}"/ai/codex/skills/*; do + [[ -d "$skill" ]] || continue + basename "$skill" + return + done +} + if run_installer "$CLAUDE_INSTALLER" --claude-md-only; then check "Claude instructions are a symlink" test -L "$FAKE_HOME/.claude/CLAUDE.md" check_eq "Claude uses the canonical global instructions" \ @@ -363,50 +367,26 @@ fi # With no routing flag the dispatcher runs both installers. Every case above routes # to exactly one, so a regression in the fall-through would ship Codex uninstalled. both_home=$(fresh_home dispatch-both) -# Seeded with a managed link for every Claude-excluded skill, which is what an install -# that predates the exclusion leaves behind. Running both installers over one home is -# also the only place the two platforms can be seen disagreeing about the same skill: -# Claude has to drop the link so its bundled workflow loads, Codex still ships ours. -mkdir -p "$both_home/.claude/skills" -while IFS= read -r excluded; do - is_excluded_skill "$excluded" "$CLAUDE_EXCLUSIONS" || continue - ln -s "$both_home/.dotfiles/ai/skills/$excluded/" "$both_home/.claude/skills/$excluded" -done <"$CLAUDE_EXCLUSIONS" if run_dispatcher "$both_home" --skills-only; then check "Dispatcher with no routing flag installs Claude skills" \ test -L "$both_home/.claude/skills/$enabled_skill" check "Dispatcher with no routing flag installs Codex skills" \ test -L "$both_home/.agents/skills/$enabled_skill" - while IFS= read -r excluded; do - is_excluded_skill "$excluded" "$CLAUDE_EXCLUSIONS" || continue - check "Claude drops its managed link for excluded skill $excluded" \ - test ! -e "$both_home/.claude/skills/$excluded" - check "Codex still installs shared skill $excluded" \ - test -L "$both_home/.agents/skills/$excluded" - done <"$CLAUDE_EXCLUSIONS" + # Running both installers over one home is the only place the two platforms can be + # seen disagreeing about a name: Codex gets ours, Claude keeps its bundled one. + codex_only_skill=$(first_codex_only_skill) + if [[ -n "$codex_only_skill" ]]; then + check "Codex installs Codex-only skill $codex_only_skill" \ + test -L "$both_home/.agents/skills/$codex_only_skill" + check "Claude leaves its bundled $codex_only_skill skill alone" \ + test ! -e "$both_home/.claude/skills/$codex_only_skill" + else + fail "A Codex-only skill exists to test" + fi else fail "Dispatcher runs both installers by default" fi -# A link the user made by hand is not ours to delete, so an excluded name pointing -# outside the repo survives and the installer only warns about it. -excluded_home=$(fresh_home excluded-claude) -excluded_target="${TEST_ROOT}/personal-skill" -mkdir -p "$excluded_home/.claude/skills" "$excluded_target" -while IFS= read -r excluded; do - is_excluded_skill "$excluded" "$CLAUDE_EXCLUSIONS" || continue - ln -s "$excluded_target" "$excluded_home/.claude/skills/$excluded" -done <"$CLAUDE_EXCLUSIONS" -if HOME="$excluded_home" "$CLAUDE_INSTALLER" --skills-only >/dev/null 2>&1; then - while IFS= read -r excluded; do - is_excluded_skill "$excluded" "$CLAUDE_EXCLUSIONS" || continue - check_eq "Claude preserves an unmanaged link at excluded skill $excluded" \ - "$(symlink_target "$excluded_home/.claude/skills/$excluded")" "$excluded_target" - done <"$CLAUDE_EXCLUSIONS" -else - fail "Claude tolerates an unmanaged excluded skill" -fi - # The guard that refuses to replace a real file or directory is what replaced the old # rm -rf. Every unmanaged fixture above is a symlink, so that branch never runs there. claude_dir_home=$(fresh_home guard-claude) diff --git a/ai/tests/test-canonical-skills.sh b/ai/tests/test-canonical-skills.sh index a0babad..aff895e 100755 --- a/ai/tests/test-canonical-skills.sh +++ b/ai/tests/test-canonical-skills.sh @@ -63,7 +63,9 @@ while IFS='|' read -r tier claude_model _; do done <"${AI_DIR}/codex/model-tiers.conf" tier_failures=0 -for skill_file in "$SKILLS_DIR"/*/SKILL.md; do +# Codex-only skills carry the same model and tier pair, so check both roots. +for skill_file in "$SKILLS_DIR"/*/SKILL.md "${AI_DIR}"/codex/skills/*/SKILL.md; do + [[ -f "$skill_file" ]] || continue model=$(sed -n 's/^model: //p' "$skill_file") [[ -n "$model" ]] || continue tiers_checked=$((tiers_checked + 1)) diff --git a/ai/tests/test-skill-spec.sh b/ai/tests/test-skill-spec.sh index 407e4e7..2d37695 100755 --- a/ai/tests/test-skill-spec.sh +++ b/ai/tests/test-skill-spec.sh @@ -1,5 +1,6 @@ #!/usr/bin/env bash -# Validate every skill in ai/skills/ against the agentskills.io specification: +# Validate every skill in ai/skills/ and ai/codex/skills/ against the agentskills.io +# specification: # directory name must equal the frontmatter `name`, description must be 1-1024 # characters, and frontmatter may only use spec keys (name, description, license, # compatibility, metadata, allowed-tools) plus this repo's own Claude extensions @@ -8,7 +9,9 @@ set -uo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -SKILLS_DIR="$(cd "${SCRIPT_DIR}/../skills" && pwd)" +AI_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)" +# Shared skills, then the ones only Codex gets. Both must meet the spec. +SKILL_ROOTS=("${AI_DIR}/skills" "${AI_DIR}/codex/skills") EXCLUSIONS="${SCRIPT_DIR}/../codex/excluded-skills.txt" README="$(cd "${SCRIPT_DIR}/../.." && pwd)/README.md" @@ -20,14 +23,21 @@ failures=0 allowed_keys="name description license compatibility metadata allowed-tools argument-hint model color disable-model-invocation" -for skill_dir in "$SKILLS_DIR"/*; do - [[ -d "$skill_dir" ]] || continue +skill_dirs=() +for skill_root in "${SKILL_ROOTS[@]}"; do + for skill_dir in "$skill_root"/*; do + [[ -d "$skill_dir" ]] && skill_dirs+=("$skill_dir") + done +done + +for skill_dir in "${skill_dirs[@]}"; do skill_name=$(basename "$skill_dir") skill_file="$skill_dir/SKILL.md" problems="" - # The table is hand-maintained, so a new skill drops off it silently. - readme_link=$(printf '[`%s`](ai/skills/%s)' "$skill_name" "$skill_name") + # The table is hand-maintained, so a new skill drops off it silently. The link has + # to name the skill's own root, so a moved skill fails until the row moves with it. + readme_link=$(printf '[`%s`](%s)' "$skill_name" "${skill_dir#"${AI_DIR%/ai}"/}") if ! grep -Fq "$readme_link" "$README"; then problems+=" no row in the README skill table"$'\n' fi @@ -95,7 +105,7 @@ done # An empty scan would otherwise read as a clean run. if (( passes == 0 && failures == 0 )); then - echo "FAIL no skills found under $SKILLS_DIR" + echo "FAIL no skills found under ${SKILL_ROOTS[*]}" exit 1 fi From a1293d7b3d1939bc3a4091f4348802e98497d12c Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Thu, 27 Aug 2026 15:32:28 -0700 Subject: [PATCH 5/6] Check both skill roots in every canonical and spec test The Codex-only root reached one of the five checks in test-canonical-skills.sh, so a hardcoded ~/.claude path, an absolute self-reference, a dangling scripts/ path, or a scalar metadata: block shipped from it with nothing reporting. All five checks now run off one skill_dirs list built from both roots, the way test-skill-spec.sh already does, and the self-reference pattern derives from each skill's own root so a Codex skill can match it at all. Both files now require every declared root to contribute a skill. The Codex root holds exactly one, so deleting it would otherwise drop the whole root out of every check while the suite stayed green. test-canonical-skills.sh runs in CI again. It was excluded over a false positive in sprint-planning that no longer exists, and it passes on main. The Codex uninstall sweep gets a test for the second prefix it passes, asserted after a check that the link was installed, and remove_managed_link's dangling-symlink branch gets a fixture whose target does not exist. Dropping either behavior fails only its own check. excluded-skills.sh credited both installers though only the Codex one sources it, and go/SKILL.md named the same step two ways. Claude-Session: https://claude.ai/code/session_01KJkXqGq8Jm5Kj9EH8yuxR6 --- .github/workflows/test.yml | 3 +- ai/helpers/excluded-skills.sh | 2 +- ai/skills/go/SKILL.md | 2 +- ai/tests/test-ai-installers.sh | 26 +++++++++++++++- ai/tests/test-canonical-skills.sh | 51 +++++++++++++++++++------------ ai/tests/test-skill-spec.sh | 15 +++++++-- 6 files changed, 71 insertions(+), 28 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 9dae008..bc945f4 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -11,8 +11,7 @@ jobs: steps: - uses: actions/checkout@v4 - name: Run AI tooling tests - # test-canonical-skills.sh is excluded: it fails on a false positive in - # sprint-planning/scripts/test_board_scripts.py (a comment, not a path). run: | ai/tests/test-skill-spec.sh + ai/tests/test-canonical-skills.sh ai/tests/test-ai-installers.sh diff --git a/ai/helpers/excluded-skills.sh b/ai/helpers/excluded-skills.sh index 1430c3d..3f76e12 100644 --- a/ai/helpers/excluded-skills.sh +++ b/ai/helpers/excluded-skills.sh @@ -2,7 +2,7 @@ # Shared parser for the per-platform excluded-skills lists. # -# Both installers and the skill tests read the lists through this one function. A +# The Codex installer and the skill tests read the lists through this one function. A # looser matcher in any one of them would call a skill excluded that the installer # still ships, and the mismatch surfaces as an unrelated assertion about a missing # symlink. diff --git a/ai/skills/go/SKILL.md b/ai/skills/go/SKILL.md index dff7b0f..76fd226 100644 --- a/ai/skills/go/SKILL.md +++ b/ai/skills/go/SKILL.md @@ -201,7 +201,7 @@ Skill("comment-cleanup") It defaults to the uncommitted diff, which is exactly the work this step is about to commit. Append the items it hands back for the author's call, one line each with file and line, under a `## Held comments` section at the end of the state file, so Step 11 still has them after a compaction or a resume. -Step 8 runs `comment-cleanup` over its own fixes, and `address-pr-reviews` runs it over the fixes it makes in Step 9. Step 10 does not, deliberately: `ci-monitor`'s `allowed-tools` fence excludes `Skill` because it reads untrusted CI logs, and widening that fence to tidy comments on a CI hotfix is the wrong trade. `/simplify` still runs only here. +Step 8 runs `comment-cleanup` over its own fixes, and `address-pr-reviews` runs it over the fixes it makes in Step 9. Step 10 does not, deliberately: `ci-monitor`'s `allowed-tools` fence excludes `Skill` because it reads untrusted CI logs, and widening that fence to tidy comments on a CI hotfix is the wrong trade. The `simplify` skill still runs only here. Then commit. Use a message that matches the situation: diff --git a/ai/tests/test-ai-installers.sh b/ai/tests/test-ai-installers.sh index 02a00ac..ac10951 100755 --- a/ai/tests/test-ai-installers.sh +++ b/ai/tests/test-ai-installers.sh @@ -86,6 +86,11 @@ file_lacks() { # path fixed-string [[ -f "$1" ]] && ! grep -Fq "$2" "$1" } +path_absent() { # path + # A dangling symlink fails -e, so both tests are needed to call a path absent. + [[ ! -e "$1" && ! -L "$1" ]] +} + CODEX_EXCLUSIONS="${REPO_ROOT}/ai/codex/excluded-skills.txt" # shellcheck source=/dev/null @@ -195,9 +200,28 @@ unmanaged_skill_target="${TEST_ROOT}/personal-skill" mkdir -p "$unmanaged_skill_target" ln -s "$unmanaged_skill_target" "$FAKE_HOME/.agents/skills/personal-skill" echo 'name = "personal"' >"$FAKE_HOME/.codex/agents/personal.toml" +# A skill deleted or renamed in the repo leaves a link whose target no longer resolves, +# so the sweep has to recognize it by prefix rather than by -e. +ln -s "$FAKE_HOME/.dotfiles/ai/skills/removed-skill" "$FAKE_HOME/.agents/skills/removed-skill" +uninstalled_codex_only_skill=$(first_codex_only_skill) +if [[ -n "$uninstalled_codex_only_skill" ]]; then + # Assert the link is here first, or the removal check below passes on a link + # the installer never created. + check "Codex installed its Codex-only skill before uninstall" \ + test -L "$FAKE_HOME/.agents/skills/$uninstalled_codex_only_skill" +fi if run_installer "$CODEX_INSTALLER" --uninstall; then check "Codex uninstall removes its instruction symlink" test ! -L "$FAKE_HOME/.codex/AGENTS.md" check "Codex uninstall removes its managed skill symlink" test ! -L "$FAKE_HOME/.agents/skills/$enabled_skill" + if [[ -n "$uninstalled_codex_only_skill" ]]; then + # The second prefix the sweep passes is only reached by a skill from this root. + check "Codex uninstall removes its managed Codex-only skill symlink" \ + test ! -L "$FAKE_HOME/.agents/skills/$uninstalled_codex_only_skill" + else + fail "A Codex-only skill exists to uninstall" + fi + check "Codex uninstall removes a managed symlink whose target is gone" \ + path_absent "$FAKE_HOME/.agents/skills/removed-skill" check "Codex uninstall removes its generated agent file" test ! -e "$FAKE_HOME/.codex/agents/code-reviewer.toml" check "Codex uninstall preserves an unmanaged skill symlink" test -L "$FAKE_HOME/.agents/skills/personal-skill" check "Codex uninstall preserves an unmanaged agent file" test -f "$FAKE_HOME/.codex/agents/personal.toml" @@ -379,7 +403,7 @@ if run_dispatcher "$both_home" --skills-only; then check "Codex installs Codex-only skill $codex_only_skill" \ test -L "$both_home/.agents/skills/$codex_only_skill" check "Claude leaves its bundled $codex_only_skill skill alone" \ - test ! -e "$both_home/.claude/skills/$codex_only_skill" + path_absent "$both_home/.claude/skills/$codex_only_skill" else fail "A Codex-only skill exists to test" fi diff --git a/ai/tests/test-canonical-skills.sh b/ai/tests/test-canonical-skills.sh index aff895e..fa7dea7 100755 --- a/ai/tests/test-canonical-skills.sh +++ b/ai/tests/test-canonical-skills.sh @@ -1,16 +1,13 @@ #!/usr/bin/env bash -# Skills shared with Codex must not depend on a provider-specific install path. -# Claude-only skills listed in codex/excluded-skills.txt are exempt. -# -# TODO: the matcher below also flags prose comments that mention ~/.claude -# (e.g. sprint-planning/scripts/test_board_scripts.py:36), so it currently -# fails on main and is excluded from .github/workflows/test.yml until the -# pattern learns to skip comment lines. +# Skills Codex installs must not depend on a provider-specific install path. Both +# skill roots are checked; Claude-only skills in codex/excluded-skills.txt are exempt. set -uo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" AI_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)" -SKILLS_DIR="${AI_DIR}/skills" +# Shared skills, then the ones only Codex gets. Both land in ~/.agents/skills/. +SKILL_ROOTS=("${AI_DIR}/skills" "${AI_DIR}/codex/skills") +REPO_ROOT="$(cd "${AI_DIR}/.." && pwd)" EXCLUSIONS="${AI_DIR}/codex/excluded-skills.txt" passes=0 @@ -21,9 +18,23 @@ tiers_checked=0 # shellcheck source=/dev/null . "${AI_DIR}/helpers/excluded-skills.sh" +skill_dirs=() +for skill_root in "${SKILL_ROOTS[@]}"; do + root_count=0 + for skill_dir in "$skill_root"/*; do + [[ -d "$skill_dir" ]] || continue + skill_dirs+=("$skill_dir") + root_count=$((root_count + 1)) + done + # A root that goes empty would otherwise drop out of every check below unnoticed. + if [[ "$root_count" -eq 0 ]]; then + echo "FAIL: no skills found under $skill_root" + exit 1 + fi +done + matches="" -for skill_dir in "$SKILLS_DIR"/*; do - [[ -d "$skill_dir" ]] || continue +for skill_dir in "${skill_dirs[@]}"; do skill_name=$(basename "$skill_dir") if is_excluded_skill "$skill_name" "$EXCLUSIONS"; then continue @@ -41,7 +52,7 @@ done # A glob that matches nothing would otherwise report a clean run, so treat an empty # scan as a failure rather than a pass. if [[ "$skills_checked" -eq 0 ]]; then - echo "FAIL: no skills were inspected under $SKILLS_DIR" + echo "FAIL: no skills were inspected under ${SKILL_ROOTS[*]}" exit 1 fi @@ -63,8 +74,8 @@ while IFS='|' read -r tier claude_model _; do done <"${AI_DIR}/codex/model-tiers.conf" tier_failures=0 -# Codex-only skills carry the same model and tier pair, so check both roots. -for skill_file in "$SKILLS_DIR"/*/SKILL.md "${AI_DIR}"/codex/skills/*/SKILL.md; do +for skill_dir in "${skill_dirs[@]}"; do + skill_file="$skill_dir/SKILL.md" [[ -f "$skill_file" ]] || continue model=$(sed -n 's/^model: //p' "$skill_file") [[ -n "$model" ]] || continue @@ -92,12 +103,12 @@ fi # the whole repo. ci-monitor is exempt: its allowed-tools frontmatter must name the # absolute paths Claude matches permissions against. self_ref_failures=0 -for skill_dir in "$SKILLS_DIR"/*; do - [[ -d "$skill_dir" ]] || continue +for skill_dir in "${skill_dirs[@]}"; do skill_name=$(basename "$skill_dir") [[ "$skill_name" == "ci-monitor" ]] && continue + # The pattern names the skill's own root, or a Codex-only skill never matches. # shellcheck disable=SC2088 # A literal tilde is what we search for, not a path. - self_refs=$(grep -RFn "~/.dotfiles/ai/skills/${skill_name}/" "$skill_dir" 2>/dev/null || true) + self_refs=$(grep -RFn "~/.dotfiles/${skill_dir#"${REPO_ROOT}"/}/" "$skill_dir" 2>/dev/null || true) if [[ -n "$self_refs" ]]; then echo "FAIL: $skill_name references its own directory by absolute path" echo "$self_refs" @@ -115,7 +126,8 @@ fi # is not a YAML mapping, so a typo here costs the skill with no error anywhere. metadata_failures=0 metadata_checked=0 -for skill_file in "$SKILLS_DIR"/*/SKILL.md; do +for skill_dir in "${skill_dirs[@]}"; do + skill_file="$skill_dir/SKILL.md" [[ -f "$skill_file" ]] || continue metadata_checked=$((metadata_checked + 1)) problem=$(awk ' @@ -150,8 +162,7 @@ fi # these references without ever opening one, so a typo in a rewritten path ships # silently and fails at the shell when a user reaches that step. path_failures=0 -for skill_dir in "$SKILLS_DIR"/*; do - [[ -d "$skill_dir" ]] || continue +for skill_dir in "${skill_dirs[@]}"; do skill_name=$(basename "$skill_dir") # Excluded skills may point at Claude-only helpers that live outside this repo. is_excluded_skill "$skill_name" "$EXCLUSIONS" && continue @@ -180,6 +191,6 @@ else fi echo "" -echo "Inspected ${skills_checked} shared skills and ${tiers_checked} tiered SKILL.md files" +echo "Inspected ${skills_checked} skills and ${tiers_checked} tiered SKILL.md files" echo "Results: ${passes} passed, ${failures} failed" [[ "${failures}" -eq 0 ]] diff --git a/ai/tests/test-skill-spec.sh b/ai/tests/test-skill-spec.sh index 2d37695..f3a096c 100755 --- a/ai/tests/test-skill-spec.sh +++ b/ai/tests/test-skill-spec.sh @@ -13,7 +13,8 @@ AI_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)" # Shared skills, then the ones only Codex gets. Both must meet the spec. SKILL_ROOTS=("${AI_DIR}/skills" "${AI_DIR}/codex/skills") EXCLUSIONS="${SCRIPT_DIR}/../codex/excluded-skills.txt" -README="$(cd "${SCRIPT_DIR}/../.." && pwd)/README.md" +REPO_ROOT="$(cd "${SCRIPT_DIR}/../.." && pwd)" +README="${REPO_ROOT}/README.md" passes=0 failures=0 @@ -25,9 +26,17 @@ allowed_keys="name description license compatibility metadata allowed-tools argu skill_dirs=() for skill_root in "${SKILL_ROOTS[@]}"; do + root_count=0 for skill_dir in "$skill_root"/*; do - [[ -d "$skill_dir" ]] && skill_dirs+=("$skill_dir") + [[ -d "$skill_dir" ]] || continue + skill_dirs+=("$skill_dir") + root_count=$((root_count + 1)) done + # A root that goes empty would otherwise drop out of every check below unnoticed. + if [[ "$root_count" -eq 0 ]]; then + echo "FAIL: no skills found under $skill_root" + exit 1 + fi done for skill_dir in "${skill_dirs[@]}"; do @@ -37,7 +46,7 @@ for skill_dir in "${skill_dirs[@]}"; do # The table is hand-maintained, so a new skill drops off it silently. The link has # to name the skill's own root, so a moved skill fails until the row moves with it. - readme_link=$(printf '[`%s`](%s)' "$skill_name" "${skill_dir#"${AI_DIR%/ai}"/}") + readme_link=$(printf '[`%s`](%s)' "$skill_name" "${skill_dir#"${REPO_ROOT}"/}") if ! grep -Fq "$readme_link" "$README"; then problems+=" no row in the README skill table"$'\n' fi From c9ccd158b0c0ec3ca7e34ba0ac30beee157fc916 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Thu, 27 Aug 2026 15:36:23 -0700 Subject: [PATCH 6/6] Run the plain-writing contract test in CI It is the last script under ai/tests/ that no workflow ran, so a skill could drop its plain-writing delegation and nothing would report it. It needs no HOME and no ~/.dotfiles, and it checks callers under ai/skills/ only, which is right: no Codex-only skill delegates prose editing yet. Claude-Session: https://claude.ai/code/session_01KJkXqGq8Jm5Kj9EH8yuxR6 --- .github/workflows/test.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index bc945f4..47eeca8 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -14,4 +14,5 @@ jobs: run: | ai/tests/test-skill-spec.sh ai/tests/test-canonical-skills.sh + ai/tests/test-plain-writing-contract.sh ai/tests/test-ai-installers.sh