diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 9dae008..47eeca8 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -11,8 +11,8 @@ 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-plain-writing-contract.sh ai/tests/test-ai-installers.sh diff --git a/README.md b/README.md index e09407a..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. 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. | @@ -82,7 +83,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/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/README.md b/ai/README.md index 1eec837..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 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 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/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/codex/skills/simplify/SKILL.md b/ai/codex/skills/simplify/SKILL.md new file mode 100644 index 0000000..f84425f --- /dev/null +++ b/ai/codex/skills/simplify/SKILL.md @@ -0,0 +1,33 @@ +--- +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. +- 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. + +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/helpers/excluded-skills.sh b/ai/helpers/excluded-skills.sh new file mode 100644 index 0000000..3f76e12 --- /dev/null +++ b/ai/helpers/excluded-skills.sh @@ -0,0 +1,18 @@ +#!/bin/sh + +# Shared parser for the per-platform excluded-skills lists. +# +# 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. + +# 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/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 7272df4..35c6832 100755 --- a/ai/install-claude.sh +++ b/ai/install-claude.sh @@ -14,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 @@ -29,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 @@ -42,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 diff --git a/ai/install-codex.sh b/ai/install-codex.sh index 7e433e4..899c762 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,18 +81,10 @@ 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 - [ -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 } @@ -103,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 @@ -144,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"; 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/skills/go/SKILL.md b/ai/skills/go/SKILL.md index 1c47715..76fd226 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: @@ -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: @@ -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/tests/test-ai-installers.sh b/ai/tests/test-ai-installers.sh index d6fc846..ac10951 100755 --- a/ai/tests/test-ai-installers.sh +++ b/ai/tests/test-ai-installers.sh @@ -86,28 +86,40 @@ 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" +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 +. "${REPO_ROOT}/ai/helpers/excluded-skills.sh" + +# 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"; 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" \ @@ -155,15 +167,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 @@ -188,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" @@ -365,6 +396,17 @@ 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" + # 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" \ + path_absent "$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 diff --git a/ai/tests/test-canonical-skills.sh b/ai/tests/test-canonical-skills.sh index e31b455..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 @@ -18,17 +15,28 @@ 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" + +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"; then + if is_excluded_skill "$skill_name" "$EXCLUSIONS"; then continue fi skills_checked=$((skills_checked + 1)) @@ -44,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 @@ -66,7 +74,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 +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 tiers_checked=$((tiers_checked + 1)) @@ -93,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" @@ -116,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 ' @@ -151,11 +162,10 @@ 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" && 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 @@ -181,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 0d87d04..f3a096c 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,28 +9,44 @@ 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" +REPO_ROOT="$(cd "${SCRIPT_DIR}/../.." && pwd)" +README="${REPO_ROOT}/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" -for skill_dir in "$SKILLS_DIR"/*; do - [[ -d "$skill_dir" ]] || continue +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 + +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#"${REPO_ROOT}"/}") if ! grep -Fq "$readme_link" "$README"; then problems+=" no row in the README skill table"$'\n' fi @@ -60,10 +77,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 @@ -97,7 +114,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