refactor(retro): declare the shipped scripts in allowed-tools, and fix the path they used - #87
Merged
Merged
Conversation
…x the path they used Applies the rule from netresearch/skill-repo-skill#272. gh, glab, git, find, grep and jq stay: the skill runs those itself. Bash(python3:*) is replaced by rules naming the shipped scripts. The conversion surfaced a path that resolves nowhere. skill-discovery.md documented python3 ${CLAUDE_PLUGIN_ROOT}/scripts/find-org-skills.py bash ${CLAUDE_PLUGIN_ROOT}/scripts/find-installed-skills.sh but neither the plugin cache nor this repository has a scripts/ directory at that level -- the scripts live under skills/retro/scripts/. Both invocations would have failed with "No such file or directory" for anyone who copied them. Three forms existed side by side for the same files: the broken ${CLAUDE_PLUGIN_ROOT}/scripts/..., the correct but long ${CLAUDE_PLUGIN_ROOT}/skills/retro/scripts/..., and bare scripts/... in backticks. All are now ${CLAUDE_SKILL_DIR}/scripts/..., which Claude Code substitutes in the body and in the frontmatter Bash rules alike, and which does not depend on the skill's position inside a plugin. The reference to agent-harness-skill/scripts/verify-harness.sh is untouched -- it names another skill's script as prose, not as a call. Not verified end to end: a session with permission prompts bypassed cannot show whether a pattern matches. Checked instead: 0 validator errors, warnings unchanged at 19 against main. Assisted-by: claude-code:claude-opus-5 Agent-Session: https://claude.ai/code/session_01JYQciiXoiApXBfcJrFMnA9 Agent-Host: 32116e Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
|
Warning Review limit reachedNext included review available in 11 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Comment |
|
Member
Author
|
Self-review: 9b1b8bb The review this pull request demands is unsatisfiable (Copilot quota wall or repeated bot failures on this head). Per the documented fallback, the diff on this head was reviewed by the PR author; this comment is the on-the-record attestation the merge gate reads back. It stops matching on the next push. |
CybotTM
added a commit
to netresearch/skill-repo-skill
that referenced
this pull request
Aug 29, 2026
…#275) Closes #271. The rule in `repository-quality-rules.md` (§ allowed-tools) has been documentation only, and a rule nothing checks drifts. ## What it flags Both conditions must hold: the skill directory contains `scripts/`, **and** `allowed-tools` grants an interpreter (`bash`, `sh`, `python`, `python3`, `uv`, `node`, `perl`, `ruby`) or a bare `Bash`. A skill without `scripts/` keeps its interpreter rule without comment. Two forms are matched that a naive pattern misses, and both showed up in the fleet sweep this check follows: - `Bash(git:*,make:*,bash:*)` — comma syntax, with the interpreter buried mid-list (`agent-harness`); - `Bash` with no parentheses at all, the widest form the field takes (`peer-qa-review`). My own first sweep used the naive `Bash(bash:*)` pattern and reported the fleet clean while those two repositories were not. That is the reason the pattern here is wider than it looks like it needs to be. Warning rather than error, on purpose: the field is optional, and whether a tool belongs in the narrow form is a judgement call — `git`, `jq`, `gh` that the agent runs itself stay listed separately. ## The timing was the condition on the issue All nine repositories shipping skills with `scripts/` are converted first: netresearch/agent-rules-skill#109, #273, netresearch/automated-assessment-skill#82, netresearch/enterprise-readiness-skill#102, netresearch/jira-skill#214, netresearch/matrix-skill#127, netresearch/retro-skill#87, netresearch/agent-harness-skill#66, netresearch/peer-qa-review-skill#51. Verified against fresh clones of all nine current `main` branches: **zero** warnings. A warning that fires everywhere on the day it lands trains people to ignore it. ## Verification ``` 10/10 repository tests pass, 43 assertions 8 new cases, both directions (5 interpreter forms warn, 3 quiet cases stay quiet) without the check, 5 of the 8 fail — confirmed by reverting the validator and re-running shellcheck clean; pre-commit run --files <changed>: all hooks pass ``` _Assisted by claude-code:claude-opus-5 — [Session](https://claude.ai/code/session_01JYQciiXoiApXBfcJrFMnA9)_
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Applies the rule from netresearch/skill-repo-skill#272: a skill that ships scripts declares the scripts, not the interpreter.
gh,glab,git,find,grepandjqstay — the skill runs those itself throughout the retro workflow.The documented path resolved nowhere
skill-discovery.mdtold the reader to runNeither the plugin cache nor this repository has a
scripts/directory at that level — the scripts live underskills/retro/scripts/. Both invocations fail with "No such file or directory" for anyone who copies them, and the skill's own instruction to run skill discovery pointed at them.Three forms existed side by side for the same files: the broken
${CLAUDE_PLUGIN_ROOT}/scripts/…, the correct but long${CLAUDE_PLUGIN_ROOT}/skills/retro/scripts/…, and barescripts/…in backticks. All are now${CLAUDE_SKILL_DIR}/scripts/…, which Claude Code substitutes in the body and in the frontmatter Bash rules alike and which does not depend on the skill's position inside a plugin.The reference to
agent-harness-skill/scripts/verify-harness.shis untouched: it names another skill's script as prose, not as a call.Verification
Not verified end to end: a session with permission prompts bypassed cannot show whether a pattern matches. #272 documents what the check can and cannot conclude.
Assisted by claude-code:claude-opus-5 — Session