Add .factory directory with installation docs and scripts#519
Add .factory directory with installation docs and scripts#519
Conversation
📝 WalkthroughWalkthroughThis PR introduces plugin configuration files, installation documentation, a SessionStart hook guard clause, and a comprehensive test suite for the Superpowers Factory Droid plugin. It establishes plugin metadata via manifest files, adds an early-exit condition when DROID_PLUGIN_ROOT is set, and provides extensive test coverage for plugin installation, skill discovery, and legacy cleanup validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
it works well on my local, try to get more feedback. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
.factory/hooks/session-start.sh (1)
36-44: Considerjqfor JSON construction to eliminate the fragility of manual string interpolation.The heredoc approach relies on
escape_for_jsonto produce a safe string. Ifjqis available in the target environment, using it is more robust and self-documenting:♻️ Proposed refactor using jq
-# Output context injection as JSON. -cat <<EOF -{ - "additional_context": "${session_context}", - "hookSpecificOutput": { - "hookEventName": "SessionStart", - "additionalContext": "${session_context}" - } -} -EOF +# Output context injection as JSON. +jq -n \ + --arg ctx "$session_context" \ + '{additional_context: $ctx, hookSpecificOutput: {hookEventName: "SessionStart", additionalContext: $ctx}}'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.factory/hooks/session-start.sh around lines 36 - 44, The heredoc in session-start.sh builds JSON by interpolating "${session_context}" and relies on escape_for_json, which is fragile; instead, use jq (if available) to construct the JSON to avoid manual escaping. Modify the code in session-start.sh to pipe or pass session_context into jq to produce the object with keys "additional_context" and "hookSpecificOutput" (containing "hookEventName": "SessionStart" and "additionalContext"), replacing the current heredoc; keep references to session_context and escape_for_json so you can remove the manual escaping once jq is used..factory/scripts/init-superpowers.sh (3)
54-62: Skills absent from the repo are silently skipped.If a skill directory doesn't exist under
$SUPERPOWERS_DIR/skills/, the loop skips it with no user-visible output. Users may not realise certain skills weren't linked.♻️ Proposed fix
for skill in "${CORE_SKILLS[@]}"; do if [ -d "$SUPERPOWERS_DIR/skills/$skill" ]; then # Remove old link if exists rm -f "$SKILLS_DIR/superpowers-$skill" # Create new link ln -sf "$SUPERPOWERS_DIR/skills/$skill" "$SKILLS_DIR/superpowers-$skill" echo " ✅ superpowers-$skill" + else + echo " ⚠️ superpowers-$skill (not found in repo, skipped)" fi done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.factory/scripts/init-superpowers.sh around lines 54 - 62, The loop over CORE_SKILLS currently skips missing skill dirs silently; update the block that iterates CORE_SKILLS to detect when "$SUPERPOWERS_DIR/skills/$skill" is not a directory and emit a clear warning (e.g., echo to stderr or prefixed "⚠️") mentioning the missing skill and path so users know it wasn't linked; keep the existing behavior for the true branch (rm -f, ln -sf, success echo) in the same loop and reference the CORE_SKILLS variable, the checked path "$SUPERPOWERS_DIR/skills/$skill", and the created link name "$SKILLS_DIR/superpowers-$skill" when composing the warning.
68-68:ls | grepanti-pattern (SC2010) — use a glob instead.ShellCheck SC2010:
lsoutput piped togrepbreaks for filenames with spaces or special characters.♻️ Proposed fix
-ls -1 "$SKILLS_DIR" | grep -E "^superpowers" || echo " (none found)" +shopt -s nullglob +skills=("$SKILLS_DIR"/superpowers*) +if [ "${`#skills`[@]}" -eq 0 ]; then + echo " (none found)" +else + for s in "${skills[@]}"; do echo " $(basename "$s")"; done +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.factory/scripts/init-superpowers.sh at line 68, Replace the unsafe `ls -1 "$SKILLS_DIR" | grep -E "^superpowers"` pipeline with shell globbing against "$SKILLS_DIR"/superpowers* and print each match safely (e.g., iterate matches and output basename), falling back to " (none found)" if no matches; target the line that uses SKILLS_DIR and the "superpowers" pattern so filenames with spaces/special chars are handled correctly.
5-5:set -eonly; missing-uo pipefailfor consistency.
session-start.shusesset -euo pipefail.-ucatches unbound variable typos and-o pipefailensures a failed command in a pipe (like line 68) propagates correctly instead of silently succeeding.♻️ Proposed fix
-set -e +set -euo pipefail🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.factory/scripts/init-superpowers.sh at line 5, Replace the lone "set -e" invocation with "set -euo pipefail" (i.e., add -u and -o pipefail) to match session-start.sh; update any commands in the script that rely on unset variables or pipes that should propagate failures so they explicitly handle defaults or check return codes where needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.factory/hooks/session-start.sh:
- Line 14: The warning_message currently begins with the string literal "\n\n"
which becomes escaped into "\\n\\n" by escape_for_json and results in literal
"\n" text in JSON; remove the leading "\n\n" from warning_message so it contains
no quoted backslash-n sequences (leave any needed newlines to session_context
which already provides them) and ensure you still pass the trimmed
warning_message through escape_for_json to produce warning_escaped used in
session_context.
- Line 18: The command substitution that sets using_superpowers_content
currently redirects stderr into stdout (using 2>&1) which concatenates cat's
error with the fallback; change the redirection to suppress cat's stderr (use
2>/dev/null) so the || fallback "Error reading using-superpowers skill" is used
cleanly; update the line that defines using_superpowers_content to use
2>/dev/null and keep the existing || echo fallback, referencing the variable
using_superpowers_content and PLUGIN_ROOT to locate the change.
In @.factory/INSTALL.md:
- Line 67: The fenced code blocks under the "Example Workflow" and "Project
Structure" sections are missing language specifiers and trigger MD040; update
each opening fence from ``` to ```text so the blocks become explicit text code
fences (e.g., change the "Example Workflow" block and the "Project Structure"
block to start with ```text).
- Around line 181-182: The uninstall step uses the command rm
~/.factory/skills/superpowers* which will error in shells like zsh when the glob
has no matches; update the uninstall command used in the INSTALL.md section (the
line containing "rm ~/.factory/skills/superpowers*") to make it idempotent by
using rm with the force option (add -f and optionally --) so the command does
not fail if no files match the glob.
- Around line 41-46: The skills loop in INSTALL.md omits two skills that
init-superpowers.sh links; update the space-separated list used by the for skill
in ...; do loop to include using-superpowers and writing-skills so the loop
checks $HOME/.factory/superpowers/skills/$skill and creates the symlink to
$HOME/.factory/skills/superpowers-$skill for those two missing entries (ensure
the exact token names "using-superpowers" and "writing-skills" are added to the
list alongside the existing skill names).
In @.factory/skills/superpowers/SKILL.md:
- Line 32: The markdown has two fenced code blocks without language specifiers
causing MD040: add language tags to the diagram block and the TodoWrite
snippet—specifically update the workflow diagram fenced block (the ASCII
flowchart) to start with ```text and update the TodoWrite example fenced block
to start with ```json (or ```text if you prefer plain text) so the blocks are
properly labeled; locate the unlabeled fences in SKILL.md around the "workflow
diagram" block and the "TodoWrite" example and add the appropriate language
identifiers.
- Line 138: Replace the compound modifier "higher quality" with the hyphenated
form "higher-quality" in the sentence beginning "By following these workflows
systematically, you produce higher quality code..." within SKILL.md so the
compound adjective before "code" is correctly hyphenated.
---
Nitpick comments:
In @.factory/hooks/session-start.sh:
- Around line 36-44: The heredoc in session-start.sh builds JSON by
interpolating "${session_context}" and relies on escape_for_json, which is
fragile; instead, use jq (if available) to construct the JSON to avoid manual
escaping. Modify the code in session-start.sh to pipe or pass session_context
into jq to produce the object with keys "additional_context" and
"hookSpecificOutput" (containing "hookEventName": "SessionStart" and
"additionalContext"), replacing the current heredoc; keep references to
session_context and escape_for_json so you can remove the manual escaping once
jq is used.
In @.factory/scripts/init-superpowers.sh:
- Around line 54-62: The loop over CORE_SKILLS currently skips missing skill
dirs silently; update the block that iterates CORE_SKILLS to detect when
"$SUPERPOWERS_DIR/skills/$skill" is not a directory and emit a clear warning
(e.g., echo to stderr or prefixed "⚠️") mentioning the missing skill and path so
users know it wasn't linked; keep the existing behavior for the true branch (rm
-f, ln -sf, success echo) in the same loop and reference the CORE_SKILLS
variable, the checked path "$SUPERPOWERS_DIR/skills/$skill", and the created
link name "$SKILLS_DIR/superpowers-$skill" when composing the warning.
- Line 68: Replace the unsafe `ls -1 "$SKILLS_DIR" | grep -E "^superpowers"`
pipeline with shell globbing against "$SKILLS_DIR"/superpowers* and print each
match safely (e.g., iterate matches and output basename), falling back to "
(none found)" if no matches; target the line that uses SKILLS_DIR and the
"superpowers" pattern so filenames with spaces/special chars are handled
correctly.
- Line 5: Replace the lone "set -e" invocation with "set -euo pipefail" (i.e.,
add -u and -o pipefail) to match session-start.sh; update any commands in the
script that rely on unset variables or pipes that should propagate failures so
they explicitly handle defaults or check return codes where needed.
| warning_message="" | ||
| legacy_skills_dir="${HOME}/.config/superpowers/skills" | ||
| if [ -d "$legacy_skills_dir" ]; then | ||
| warning_message="\n\n<important-reminder>IN YOUR FIRST REPLY AFTER SEEING THIS MESSAGE YOU MUST TELL THE USER:⚠️ **WARNING:** Superpowers now uses Droid CLI's skills system. Custom skills in ~/.config/superpowers/skills will not be read. Move custom skills to ~/.factory/skills instead. To make this message go away, remove ~/.config/superpowers/skills</important-reminder>" |
There was a problem hiding this comment.
"\n\n" in double quotes produces literal \n text, not newlines, after JSON escaping.
In bash, \n inside double quotes is a literal two-character sequence (backslash + n), not a newline. When escape_for_json runs, the step s="${s//\\/\\\\}" doubles the backslash, producing \\n in the JSON output — which decodes to the literal string \n, not a line break.
Since session_context already provides \n\n (valid JSON newlines) immediately before ${warning_escaped}, the simplest fix is to drop the leading newlines from warning_message entirely:
🐛 Proposed fix
- warning_message="\n\n<important-reminder>IN YOUR FIRST REPLY AFTER SEEING THIS MESSAGE YOU MUST TELL THE USER:⚠️ **WARNING:** Superpowers now uses Droid CLI's skills system. Custom skills in ~/.config/superpowers/skills will not be read. Move custom skills to ~/.factory/skills instead. To make this message go away, remove ~/.config/superpowers/skills</important-reminder>"
+ warning_message="<important-reminder>IN YOUR FIRST REPLY AFTER SEEING THIS MESSAGE YOU MUST TELL THE USER:⚠️ **WARNING:** Superpowers now uses Droid CLI's skills system. Custom skills in ~/.config/superpowers/skills will not be read. Move custom skills to ~/.factory/skills instead. To make this message go away, remove ~/.config/superpowers/skills</important-reminder>"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| warning_message="\n\n<important-reminder>IN YOUR FIRST REPLY AFTER SEEING THIS MESSAGE YOU MUST TELL THE USER:⚠️ **WARNING:** Superpowers now uses Droid CLI's skills system. Custom skills in ~/.config/superpowers/skills will not be read. Move custom skills to ~/.factory/skills instead. To make this message go away, remove ~/.config/superpowers/skills</important-reminder>" | |
| warning_message="<important-reminder>IN YOUR FIRST REPLY AFTER SEEING THIS MESSAGE YOU MUST TELL THE USER:⚠️ **WARNING:** Superpowers now uses Droid CLI's skills system. Custom skills in ~/.config/superpowers/skills will not be read. Move custom skills to ~/.factory/skills instead. To make this message go away, remove ~/.config/superpowers/skills</important-reminder>" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.factory/hooks/session-start.sh at line 14, The warning_message currently
begins with the string literal "\n\n" which becomes escaped into "\\n\\n" by
escape_for_json and results in literal "\n" text in JSON; remove the leading
"\n\n" from warning_message so it contains no quoted backslash-n sequences
(leave any needed newlines to session_context which already provides them) and
ensure you still pass the trimmed warning_message through escape_for_json to
produce warning_escaped used in session_context.
| fi | ||
|
|
||
| # Read using-superpowers content | ||
| using_superpowers_content=$(cat "${PLUGIN_ROOT}/../skills/using-superpowers/SKILL.md" 2>&1 || echo "Error reading using-superpowers skill") |
There was a problem hiding this comment.
2>&1 concatenates cat's stderr error message with the || fallback, producing a mangled error string.
On a missing file, this line captures cat: <path>: No such file or directory\nError reading using-superpowers skill into using_superpowers_content, because 2>&1 redirects stderr into the command substitution's stdout before || echo appends the fallback. Use 2>/dev/null to suppress cat's error output and rely solely on the fallback.
🐛 Proposed fix
-using_superpowers_content=$(cat "${PLUGIN_ROOT}/../skills/using-superpowers/SKILL.md" 2>&1 || echo "Error reading using-superpowers skill")
+using_superpowers_content=$(cat "${PLUGIN_ROOT}/../skills/using-superpowers/SKILL.md" 2>/dev/null || echo "Error reading using-superpowers skill")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| using_superpowers_content=$(cat "${PLUGIN_ROOT}/../skills/using-superpowers/SKILL.md" 2>&1 || echo "Error reading using-superpowers skill") | |
| using_superpowers_content=$(cat "${PLUGIN_ROOT}/../skills/using-superpowers/SKILL.md" 2>/dev/null || echo "Error reading using-superpowers skill") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.factory/hooks/session-start.sh at line 18, The command substitution that
sets using_superpowers_content currently redirects stderr into stdout (using
2>&1) which concatenates cat's error with the fallback; change the redirection
to suppress cat's stderr (use 2>/dev/null) so the || fallback "Error reading
using-superpowers skill" is used cleanly; update the line that defines
using_superpowers_content to use 2>/dev/null and keep the existing || echo
fallback, referencing the variable using_superpowers_content and PLUGIN_ROOT to
locate the change.
| # Core workflow skills | ||
| for skill in brainstorming writing-plans executing-plans test-driven-development systematic-debugging using-git-worktrees finishing-a-development-branch requesting-code-review receiving-code-review verification-before-completion subagent-driven-development dispatching-parallel-agents; do | ||
| if [ -d "$HOME/.factory/superpowers/skills/$skill" ]; then | ||
| ln -sf "$HOME/.factory/superpowers/skills/$skill" "$HOME/.factory/skills/superpowers-$skill" | ||
| fi | ||
| done |
There was a problem hiding this comment.
Skills list in Step 3 is missing using-superpowers and writing-skills.
init-superpowers.sh links 14 skills (including using-superpowers and writing-skills) but the manual loop shown here only covers 12. Users following the manual steps will be missing two skill symlinks.
🐛 Proposed fix
-for skill in brainstorming writing-plans executing-plans test-driven-development systematic-debugging using-git-worktrees finishing-a-development-branch requesting-code-review receiving-code-review verification-before-completion subagent-driven-development dispatching-parallel-agents; do
+for skill in brainstorming writing-plans executing-plans test-driven-development systematic-debugging using-git-worktrees finishing-a-development-branch requesting-code-review receiving-code-review verification-before-completion subagent-driven-development dispatching-parallel-agents using-superpowers writing-skills; do📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Core workflow skills | |
| for skill in brainstorming writing-plans executing-plans test-driven-development systematic-debugging using-git-worktrees finishing-a-development-branch requesting-code-review receiving-code-review verification-before-completion subagent-driven-development dispatching-parallel-agents; do | |
| if [ -d "$HOME/.factory/superpowers/skills/$skill" ]; then | |
| ln -sf "$HOME/.factory/superpowers/skills/$skill" "$HOME/.factory/skills/superpowers-$skill" | |
| fi | |
| done | |
| # Core workflow skills | |
| for skill in brainstorming writing-plans executing-plans test-driven-development systematic-debugging using-git-worktrees finishing-a-development-branch requesting-code-review receiving-code-review verification-before-completion subagent-driven-development dispatching-parallel-agents using-superpowers writing-skills; do | |
| if [ -d "$HOME/.factory/superpowers/skills/$skill" ]; then | |
| ln -sf "$HOME/.factory/superpowers/skills/$skill" "$HOME/.factory/skills/superpowers-$skill" | |
| fi | |
| done |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.factory/INSTALL.md around lines 41 - 46, The skills loop in INSTALL.md
omits two skills that init-superpowers.sh links; update the space-separated list
used by the for skill in ...; do loop to include using-superpowers and
writing-skills so the loop checks $HOME/.factory/superpowers/skills/$skill and
creates the symlink to $HOME/.factory/skills/superpowers-$skill for those two
missing entries (ensure the exact token names "using-superpowers" and
"writing-skills" are added to the list alongside the existing skill names).
|
|
||
| ### Example Workflow | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Fenced code blocks missing language specifiers (MD040).
Both the "Example Workflow" block (line 67) and the "Project Structure" block (line 117) trigger MD040. Specifying text satisfies the linter and is semantically appropriate for both.
🐛 Proposed fix
-```
+```text
You: I want to add a user authentication feature to my app-```
+```text
~/.factory/
├── superpowers/Also applies to: 117-117
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 67-67: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.factory/INSTALL.md at line 67, The fenced code blocks under the "Example
Workflow" and "Project Structure" sections are missing language specifiers and
trigger MD040; update each opening fence from ``` to ```text so the blocks
become explicit text code fences (e.g., change the "Example Workflow" block and
the "Project Structure" block to start with ```text).
|
|
||
| ## Workflow Overview | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Two fenced code blocks missing language specifiers (MD040).
The workflow diagram (line 32) and the TodoWrite example (line 73) trigger MD040. Use text for the diagram and json (or text) for the TodoWrite snippet.
🐛 Proposed fix
-```
+```text
┌─────────────────┐
│ Brainstorming │ ← Explore requirements, create design-```
+```json
TodoWrite: {"todos": "1. [pending] Explore project context\n..."}Also applies to: 73-73
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.factory/skills/superpowers/SKILL.md at line 32, The markdown has two fenced
code blocks without language specifiers causing MD040: add language tags to the
diagram block and the TodoWrite snippet—specifically update the workflow diagram
fenced block (the ASCII flowchart) to start with ```text and update the
TodoWrite example fenced block to start with ```json (or ```text if you prefer
plain text) so the blocks are properly labeled; locate the unlabeled fences in
SKILL.md around the "workflow diagram" block and the "TodoWrite" example and add
the appropriate language identifiers.
| 3. **Implement** - Use TDD (RED-GREEN-REFACTOR) | ||
| 4. **Review** - Ensure quality and specification compliance | ||
|
|
||
| By following these workflows systematically, you produce higher quality code with fewer bugs and better alignment with requirements. |
There was a problem hiding this comment.
Missing hyphen in compound modifier "higher-quality code".
As flagged by LanguageTool: "higher-quality" used as a compound adjective before "code" should be hyphenated.
🐛 Proposed fix
-By following these workflows systematically, you produce higher quality code with fewer bugs and better alignment with requirements.
+By following these workflows systematically, you produce higher-quality code with fewer bugs and better alignment with requirements.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| By following these workflows systematically, you produce higher quality code with fewer bugs and better alignment with requirements. | |
| By following these workflows systematically, you produce higher-quality code with fewer bugs and better alignment with requirements. |
🧰 Tools
🪛 LanguageTool
[grammar] ~138-~138: Use a hyphen to join words.
Context: ...flows systematically, you produce higher quality code with fewer bugs and better ...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.factory/skills/superpowers/SKILL.md at line 138, Replace the compound
modifier "higher quality" with the hyphenated form "higher-quality" in the
sentence beginning "By following these workflows systematically, you produce
higher quality code..." within SKILL.md so the compound adjective before "code"
is correctly hyphenated.
|
Hi! Thanks so much for contributing this. Is there some way to get rid of the custom skills/superpowers skill? It's duplicative of standard content, and maintenance is going to be a nightmare. |
obra
added
new-harness
…plified install - Add .factory-plugin/plugin.json and marketplace.json for native plugin system - Simplify INSTALL.md for droid plugin workflow - Remove legacy hooks, scripts, and .factory/skills (now uses shared skills/) - Update session-start hook with DROID_PLUGIN_ROOT detection - Add comprehensive droid test suite (manifests, hook, skill discovery, etc.)
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
tests/droid/test-helpers.sh (2)
13-13:grep -q "$pattern"uses BRE —\|alternation won't work on macOS BSD grep.
assert_containsandassert_not_containsdefault to BRE mode. Callers intest-skill-invocation.shpass patterns like"brainstorming\|Brainstorming"which rely on the GNU-specific BRE\|extension. In basic regular expressions the meta-characters|(and others) lose their special meaning, and the backslashed form\|is a GNU extension — a shell script that works on a Linux machine may fail on a Mac due to differences between GNU grep and BSD grep.Switch to ERE (
-E) in the helper and drop the backslash in caller patterns:♻️ Proposed fix
- if echo "$output" | grep -q "$pattern"; then + if echo "$output" | grep -qE "$pattern"; thenAnd in callers (e.g.
test-skill-invocation.sh):-assert_contains "$output" "brainstorming\|Brainstorming" "Suggests brainstorming skill" +assert_contains "$output" "brainstorming|Brainstorming" "Suggests brainstorming skill"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/droid/test-helpers.sh` at line 13, The helpers assert_contains and assert_not_contains use plain grep which defaults to BRE and fails on macOS for GNU-only extensions like '\|'; update the grep invocation in those functions (the line using grep -q "$pattern") to use ERE mode (grep -E -q) and update any caller patterns (e.g., in test-skill-invocation.sh) to remove the backslash alternation (use "brainstorming|Brainstorming" instead of "brainstorming\|Brainstorming") so alternation works cross-platform.
98-98: Path argument is interpolated directly into the Python inline string — single quotes in the path will break the command.Both
assert_valid_json(line 98) andassert_json_field(lines 119–120) embed$pathand$fielddirectly inside a Python single-quoted string literal. A path or field containing'will produce a Python syntax error (silently swallowed by2>/dev/null), and in the worst case injects arbitrary Python.♻️ Proposed fix — pass values via environment instead
assert_valid_json() { local path="$1" local test_name="${2:-valid JSON}" - if python3 -c "import json; json.load(open('$path'))" 2>/dev/null; then + if ASSERT_PATH="$path" python3 -c "import json, os; json.load(open(os.environ['ASSERT_PATH']))" 2>/dev/null; thenlocal actual - actual=$(python3 -c " + actual=$(ASSERT_PATH="$path" ASSERT_FIELD="$field" python3 -c " import json, sys -data = json.load(open('$path')) -keys = '$field'.split('.') +import os +data = json.load(open(os.environ['ASSERT_PATH'])) +keys = os.environ['ASSERT_FIELD'].split('.') val = data for k in keys: if isinstance(val, list): val = val[int(k)] else: val = val[k] print(val) " 2>/dev/null)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/droid/test-helpers.sh` at line 98, assert_valid_json and assert_json_field are vulnerable because they interpolate $path and $field directly into a single-quoted Python command; instead, stop embedding those variables into the Python source and pass them via environment variables (e.g., set PATH_VAR and FIELD_VAR) and read them inside Python (os.environ['PATH_VAR'] / os.environ['FIELD_VAR']) when calling python3 -c, or use a heredoc with a quoted delimiter to avoid interpolation; update the calls in assert_valid_json and assert_json_field to export/pass the variables and change the inline Python to load the path and field from os.environ before json.load / field access to prevent quoting/injection issues.tests/droid/test-skill-invocation.sh (1)
27-52: LLM-driven assertions are inherently non-deterministic and may produce intermittent failures.Each test sends a free-form prompt to the LLM and matches a keyword in its response. Even with broad alternation patterns (e.g.,
"verif\|check\|confirm\|complete"), model output variation across runs or model versions can cause spurious failures unrelated to code changes.Consider adding a
--integrationgate or marking these scenarios asxfail/best-effort so CI is not blocked by model-output flakiness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/droid/test-skill-invocation.sh` around lines 27 - 52, These LLM-driven tests in tests/droid/test-skill-invocation.sh (the blocks invoking droid exec and using assert_contains with FAILED) are flaky; update the script to gate them behind an integration flag or mark them as expected-fail/soft-fail so CI won't fail on intermittent model output changes — e.g., check an env var like RUN_INTEGRATION or a --integration CLI arg at the top of the script and skip (or echo xfail) the droid exec/assert_contains blocks unless that flag is set, or convert the failing assertions to non-blocking best-effort checks that only increment a warning counter instead of failing CI.tests/droid/run-droid-tests.sh (2)
123-172: Significant code duplication between verbose and non-verbose branches.The
run_cmdconstruction (lines 124–128 and 148–152) is identical in both branches. The only behavioral difference is whether output is captured. Extractrun_cmdbefore the verbose check to eliminate ~10 duplicated lines:♻️ Proposed refactor
+ if [ -n "$TIMEOUT_CMD" ]; then + run_cmd="$TIMEOUT_CMD $TIMEOUT bash $test_path" + else + run_cmd="bash $test_path" + fi + if [ "$VERBOSE" = true ]; then - if [ -n "$TIMEOUT_CMD" ]; then - run_cmd="$TIMEOUT_CMD $TIMEOUT bash $test_path" - else - run_cmd="bash $test_path" - fi if eval "$run_cmd"; thenAnd remove the duplicate
run_cmdconstruction from the non-verbose branch (lines 148–152).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/droid/run-droid-tests.sh` around lines 123 - 172, Extract the identical run_cmd construction out of the VERBOSE conditional: build run_cmd once using TIMEOUT_CMD, TIMEOUT and test_path (the same logic currently duplicated in both branches) before the if [ "$VERBOSE" = true ] check, then remove the duplicated construction inside both the verbose and non-verbose branches so each branch only differs by whether output is captured (eval "$run_cmd" vs output=$(eval "$run_cmd" 2>&1)) and logging behavior; keep existing references to run_cmd, TIMEOUT_CMD, TIMEOUT, test_path and VERBOSE so behavior is unchanged.
119-119:chmod +xis redundant and breaks in read-only CI environments.Tests are already invoked via
bash $test_path, so execute permission is never consulted. On environments where the repository is checked out read-only (common in CI),chmod +xexits non-zero andset -eaborts the entire test runner before any test runs.🛠️ Proposed fix
- chmod +x "$test_path" - start_time=$(date +%s)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/droid/run-droid-tests.sh` at line 119, Remove the redundant chmod +x "$test_path" which can fail in read-only CI; since tests are executed via bash $test_path, delete the chmod invocation (or replace it with a non-failing no-op like a conditional check or append "|| true") so the script no longer exits on chmod failure; update the script around the chmod +x "$test_path" line and ensure the existing invocation bash $test_path remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.factory/INSTALL.md:
- Around line 22-26: The fenced code block containing the interactive UI command
"/plugins" is missing a language specifier; change the opening fence from ``` to
```text (or ```bash for consistency with other blocks) so the block reads
```text followed by /plugins and closes with ``` to satisfy MD040 and match the
other fenced blocks.
In `@tests/droid/test-legacy-cleanup.sh`:
- Around line 37-41: The test aborts under set -e when INSTALL.md is missing
because install_content=$(cat "$REPO_ROOT/.factory/INSTALL.md") will exit
non-zero; change the logic in the test to handle a missing file explicitly:
first check for the file (e.g., test -f "$REPO_ROOT/.factory/INSTALL.md") and if
missing increment FAILED and set install_content to empty, otherwise read the
file into install_content and run assert_contains/assert_not_contains as before
(references: variable install_content and functions
assert_contains/assert_not_contains and counter FAILED).
In `@tests/droid/test-plugin-installation.sh`:
- Around line 52-68: The test shell assigns outputs from live LLM calls (the
three occurrences of output=$(droid exec ...)) which can fail and trigger set
-e, and the assertions on open-ended LLM text
(assert_contains/assert_not_contains) are flaky; fix by wrapping each droid exec
call to capture its exit status without aborting (e.g., run the subshell and if
it returns non-zero increment FAILED and include the output in diagnostics) for
the three places where output=$(droid exec ...) appears, and replace the brittle
content assertions for Tests 3–5 with deterministic structural checks (for
example verify the plugin/skill registry files or call a droid
list-skills/list-plugins command) instead of grepping freeform model replies;
apply the same guarded-capture pattern and failure-increment
(FAILED=$((FAILED+1))) to the blocks using assert_contains/assert_not_contains
so CI records failures and prints diagnostics rather than silently aborting.
In `@tests/droid/test-plugin-manifests.sh`:
- Around line 65-73: The current comparison treats empty strings as a match
because both plugin_version and marketplace_version are set via python3 calls
silenced with 2>/dev/null; update the logic around the variables plugin_version
and marketplace_version to detect python failures: do not discard stderr (remove
2>/dev/null) or capture the python exit status ($?) after each assignment and
fail if non-zero, and also check that both variables are non-empty before
comparing; if either extraction fails, print a clear [FAIL] with context and
increment FAILED instead of proceeding to the equality check.
- Around line 27-28: The test incorrectly asserts a hardcoded version ("4.3.1")
while naming the check "version present", which is brittle; update the assertion
that references assert_json_field for the "version" field to verify
presence/non-empty value instead of a literal string (e.g., assert_json_field
... "version" with a non-empty matcher like ".+" or use an existing
presence-check helper), keeping the test name "version present"; alternatively,
if you want exact-version enforcement, keep the literal but rename the test
description to "version = 4.3.1" and document the need to update this test on
each release.
In `@tests/droid/test-session-start-hook.sh`:
- Around line 44-51: The test currently invokes the hook with
"DROID_PLUGIN_ROOT=\"$REPO_ROOT\" bash \"$HOOK_SCRIPT\"" under set -euo pipefail
so a non-zero hook will abort the script before exit_code and FAILED are
updated; modify the invocation to prevent immediate exit by adding a safe
fallback (append "|| true") so the command never triggers set -e to exit, then
capture exit_code and update FAILED accordingly; targets: the invocation that
uses DROID_PLUGIN_ROOT and HOOK_SCRIPT, the exit_code variable capture, and the
FAILED counter update.
In `@tests/droid/test-skill-discovery.sh`:
- Around line 50-61: The test currently runs skill_names=$(ls -1 "$SKILLS_DIR")
which will cause the script to abort under set -e if SKILLS_DIR is missing;
update the Test 4 block to first guard the directory (check [ -d "$SKILLS_DIR" ]
or use a safe fallback for ls like redirecting errors and allowing empty
output), and if the directory is missing increment FAILED and print a clear
failure message; reference the SKILLS_DIR variable, the skill_names assignment,
and the FAILED counter so you add the directory existence check before computing
skill_names and ensure the failure path increments FAILED and does not let the
script exit early.
- Around line 22-47: The Test 3 loop can iterate the literal pattern when no
subdirs exist because the shell nullglob option isn't set; enable nullglob
before the loop (or at the script top) so "$SKILLS_DIR"/*/ expands to zero
entries instead of the pattern, ensuring the for loop over skill_dir and checks
around skill_file/SKILL.md (variables skill_dir, skill_file, SKILLS_DIR and the
Test 3 loop) do not produce spurious failures.
---
Nitpick comments:
In `@tests/droid/run-droid-tests.sh`:
- Around line 123-172: Extract the identical run_cmd construction out of the
VERBOSE conditional: build run_cmd once using TIMEOUT_CMD, TIMEOUT and test_path
(the same logic currently duplicated in both branches) before the if [
"$VERBOSE" = true ] check, then remove the duplicated construction inside both
the verbose and non-verbose branches so each branch only differs by whether
output is captured (eval "$run_cmd" vs output=$(eval "$run_cmd" 2>&1)) and
logging behavior; keep existing references to run_cmd, TIMEOUT_CMD, TIMEOUT,
test_path and VERBOSE so behavior is unchanged.
- Line 119: Remove the redundant chmod +x "$test_path" which can fail in
read-only CI; since tests are executed via bash $test_path, delete the chmod
invocation (or replace it with a non-failing no-op like a conditional check or
append "|| true") so the script no longer exits on chmod failure; update the
script around the chmod +x "$test_path" line and ensure the existing invocation
bash $test_path remains unchanged.
In `@tests/droid/test-helpers.sh`:
- Line 13: The helpers assert_contains and assert_not_contains use plain grep
which defaults to BRE and fails on macOS for GNU-only extensions like '\|';
update the grep invocation in those functions (the line using grep -q
"$pattern") to use ERE mode (grep -E -q) and update any caller patterns (e.g.,
in test-skill-invocation.sh) to remove the backslash alternation (use
"brainstorming|Brainstorming" instead of "brainstorming\|Brainstorming") so
alternation works cross-platform.
- Line 98: assert_valid_json and assert_json_field are vulnerable because they
interpolate $path and $field directly into a single-quoted Python command;
instead, stop embedding those variables into the Python source and pass them via
environment variables (e.g., set PATH_VAR and FIELD_VAR) and read them inside
Python (os.environ['PATH_VAR'] / os.environ['FIELD_VAR']) when calling python3
-c, or use a heredoc with a quoted delimiter to avoid interpolation; update the
calls in assert_valid_json and assert_json_field to export/pass the variables
and change the inline Python to load the path and field from os.environ before
json.load / field access to prevent quoting/injection issues.
In `@tests/droid/test-skill-invocation.sh`:
- Around line 27-52: These LLM-driven tests in
tests/droid/test-skill-invocation.sh (the blocks invoking droid exec and using
assert_contains with FAILED) are flaky; update the script to gate them behind an
integration flag or mark them as expected-fail/soft-fail so CI won't fail on
intermittent model output changes — e.g., check an env var like RUN_INTEGRATION
or a --integration CLI arg at the top of the script and skip (or echo xfail) the
droid exec/assert_contains blocks unless that flag is set, or convert the
failing assertions to non-blocking best-effort checks that only increment a
warning counter instead of failing CI.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.factory-plugin/marketplace.json.factory-plugin/plugin.json.factory/INSTALL.mdhooks/session-starttests/droid/README.mdtests/droid/run-droid-tests.shtests/droid/test-helpers.shtests/droid/test-legacy-cleanup.shtests/droid/test-plugin-installation.shtests/droid/test-plugin-manifests.shtests/droid/test-session-start-hook.shtests/droid/test-skill-discovery.shtests/droid/test-skill-invocation.sh
✅ Files skipped from review due to trivial changes (1)
- .factory-plugin/marketplace.json
| Or use the interactive UI: | ||
| ``` | ||
| /plugins | ||
| ``` | ||
| Navigate to Browse tab, find superpowers, and install. |
There was a problem hiding this comment.
MD040: fenced code block at line 23 is missing a language specifier.
The interactive UI command block should declare a language to satisfy the linter (and for consistency with the other bash-fenced blocks).
🐛 Proposed fix
- ```
+ ```text
/plugins
- ```
+ ```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Or use the interactive UI: | |
| ``` | |
| /plugins | |
| ``` | |
| Navigate to Browse tab, find superpowers, and install. | |
| Or use the interactive UI: |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.factory/INSTALL.md around lines 22 - 26, The fenced code block containing
the interactive UI command "/plugins" is missing a language specifier; change
the opening fence from ``` to ```text (or ```bash for consistency with other
blocks) so the block reads ```text followed by /plugins and closes with ``` to
satisfy MD040 and match the other fenced blocks.
| echo "Test 5: INSTALL.md uses plugin system..." | ||
| install_content=$(cat "$REPO_ROOT/.factory/INSTALL.md") | ||
| assert_contains "$install_content" "plugin" "INSTALL.md mentions plugin system" || FAILED=$((FAILED + 1)) | ||
| assert_not_contains "$install_content" "symlink" "INSTALL.md does not mention symlinks" || FAILED=$((FAILED + 1)) | ||
| echo "" |
There was a problem hiding this comment.
set -e will abort the script at line 38 if INSTALL.md is absent, bypassing the failure summary.
If Test 4 already registered a failure (file missing), cat on the same missing path exits non-zero, and set -euo pipefail kills the script before the FAILED counter or summary block is printed. The test run appears to crash rather than reporting a clean FAILED: N test(s) total.
🐛 Proposed fix
# Test 5: INSTALL.md references plugin system (not symlinks)
echo "Test 5: INSTALL.md uses plugin system..."
-install_content=$(cat "$REPO_ROOT/.factory/INSTALL.md")
-assert_contains "$install_content" "plugin" "INSTALL.md mentions plugin system" || FAILED=$((FAILED + 1))
-assert_not_contains "$install_content" "symlink" "INSTALL.md does not mention symlinks" || FAILED=$((FAILED + 1))
+if install_content=$(cat "$REPO_ROOT/.factory/INSTALL.md" 2>/dev/null); then
+ assert_contains "$install_content" "plugin" "INSTALL.md mentions plugin system" || FAILED=$((FAILED + 1))
+ assert_not_contains "$install_content" "symlink" "INSTALL.md does not mention symlinks" || FAILED=$((FAILED + 1))
+else
+ echo " [FAIL] Cannot read INSTALL.md — skipping content checks"
+ FAILED=$((FAILED + 2))
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "Test 5: INSTALL.md uses plugin system..." | |
| install_content=$(cat "$REPO_ROOT/.factory/INSTALL.md") | |
| assert_contains "$install_content" "plugin" "INSTALL.md mentions plugin system" || FAILED=$((FAILED + 1)) | |
| assert_not_contains "$install_content" "symlink" "INSTALL.md does not mention symlinks" || FAILED=$((FAILED + 1)) | |
| echo "" | |
| echo "Test 5: INSTALL.md uses plugin system..." | |
| if install_content=$(cat "$REPO_ROOT/.factory/INSTALL.md" 2>/dev/null); then | |
| assert_contains "$install_content" "plugin" "INSTALL.md mentions plugin system" || FAILED=$((FAILED + 1)) | |
| assert_not_contains "$install_content" "symlink" "INSTALL.md does not mention symlinks" || FAILED=$((FAILED + 1)) | |
| else | |
| echo " [FAIL] Cannot read INSTALL.md — skipping content checks" | |
| FAILED=$((FAILED + 2)) | |
| fi | |
| echo "" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/droid/test-legacy-cleanup.sh` around lines 37 - 41, The test aborts
under set -e when INSTALL.md is missing because install_content=$(cat
"$REPO_ROOT/.factory/INSTALL.md") will exit non-zero; change the logic in the
test to handle a missing file explicitly: first check for the file (e.g., test
-f "$REPO_ROOT/.factory/INSTALL.md") and if missing increment FAILED and set
install_content to empty, otherwise read the file into install_content and run
assert_contains/assert_not_contains as before (references: variable
install_content and functions assert_contains/assert_not_contains and counter
FAILED).
| output=$(droid exec "List the superpowers skills you have available. Just list their names, nothing else." 2>&1) | ||
| assert_contains "$output" "test-driven-development\|systematic-debugging\|brainstorming" "Droid recognizes superpowers skills" || FAILED=$((FAILED + 1)) | ||
| echo "" | ||
|
|
||
| # Test 4: Droid can describe a specific skill | ||
| echo "Test 4: Skill content loading..." | ||
| output=$(droid exec "What is the test-driven-development skill about? Answer in one sentence." 2>&1) | ||
| assert_contains "$output" "test\|TDD\|Test" "Droid can describe TDD skill" || FAILED=$((FAILED + 1)) | ||
| echo "" | ||
|
|
||
| # Test 5: SessionStart hook does NOT inject content (Factory skips it) | ||
| echo "Test 5: No SessionStart injection..." | ||
| # In Factory Droid, the hook should exit early and not inject using-superpowers content. | ||
| # We verify by checking that droid doesn't mention "EXTREMELY_IMPORTANT" or the hook injection markers | ||
| # in a basic prompt that doesn't invoke any skill. | ||
| output=$(droid exec "Say hello. Nothing else." 2>&1) | ||
| assert_not_contains "$output" "EXTREMELY_IMPORTANT" "No hook injection markers in output" || FAILED=$((FAILED + 1)) |
There was a problem hiding this comment.
Tests 3–5 are non-deterministic and can abort the script on droid errors.
Two compounding issues:
-
Non-determinism: Tests 3, 4, and 5 assert on live LLM output (
droid execwith open-ended prompts). Model phrasing variability, rate limiting, and network errors make these inherently flaky in CI. Tests 3 and 4 in particular check that the model lists specific skill names or mentions "TDD" — valid responses may not match the grep patterns. -
Silent abort via
set -e: The standalone assignmentsoutput=$(droid exec ...)on lines 52, 58, and 67 triggerset -eifdroid execexits non-zero (e.g., API error). The script aborts without recording a FAILED entry or printing a diagnostic. Guard each call:
🛠️ Proposed fix (Test 3 shown; apply same pattern to Tests 4 and 5)
-output=$(droid exec "List the superpowers skills you have available. Just list their names, nothing else." 2>&1)
-assert_contains "$output" "test-driven-development\|systematic-debugging\|brainstorming" "Droid recognizes superpowers skills" || FAILED=$((FAILED + 1))
+if output=$(droid exec "List the superpowers skills you have available. Just list their names, nothing else." 2>&1); then
+ assert_contains "$output" "test-driven-development\|systematic-debugging\|brainstorming" "Droid recognizes superpowers skills" || FAILED=$((FAILED + 1))
+else
+ echo " [FAIL] droid exec failed (exit $?)"
+ FAILED=$((FAILED + 1))
+fiFor the non-determinism concern, consider replacing LLM-output assertions with structural checks (e.g., verifying the skill files are loaded into the plugin context) rather than asserting on model-generated text.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/droid/test-plugin-installation.sh` around lines 52 - 68, The test shell
assigns outputs from live LLM calls (the three occurrences of output=$(droid
exec ...)) which can fail and trigger set -e, and the assertions on open-ended
LLM text (assert_contains/assert_not_contains) are flaky; fix by wrapping each
droid exec call to capture its exit status without aborting (e.g., run the
subshell and if it returns non-zero increment FAILED and include the output in
diagnostics) for the three places where output=$(droid exec ...) appears, and
replace the brittle content assertions for Tests 3–5 with deterministic
structural checks (for example verify the plugin/skill registry files or call a
droid list-skills/list-plugins command) instead of grepping freeform model
replies; apply the same guarded-capture pattern and failure-increment
(FAILED=$((FAILED+1))) to the blocks using assert_contains/assert_not_contains
so CI records failures and prints diagnostics rather than silently aborting.
| assert_json_field "$REPO_ROOT/.factory-plugin/plugin.json" "name" "superpowers" "name = superpowers" || FAILED=$((FAILED + 1)) | ||
| assert_json_field "$REPO_ROOT/.factory-plugin/plugin.json" "version" "4.3.1" "version present" || FAILED=$((FAILED + 1)) |
There was a problem hiding this comment.
Misleading test name and brittle hardcoded version.
Line 28 asserts version = "4.3.1", but the test name is "version present" — these describe different things. More importantly, this assertion will fail on every version bump unless this test file is updated in lockstep with the manifest.
If the intent is only to verify the field exists, check for a non-empty value instead:
🛠️ Proposed fix (presence check)
-assert_json_field "$REPO_ROOT/.factory-plugin/plugin.json" "version" "4.3.1" "version present" || FAILED=$((FAILED + 1))
+plugin_ver=$(python3 -c "import json; print(json.load(open('$REPO_ROOT/.factory-plugin/plugin.json')).get('version',''))" 2>/dev/null)
+if [ -n "$plugin_ver" ]; then
+ echo " [PASS] version is present ($plugin_ver)"
+else
+ echo " [FAIL] version field is missing or empty"
+ FAILED=$((FAILED + 1))
+fiIf exact-version verification is the goal, keep the assertion but rename the test name to "version = 4.3.1" and document that this file must be updated on every release.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/droid/test-plugin-manifests.sh` around lines 27 - 28, The test
incorrectly asserts a hardcoded version ("4.3.1") while naming the check
"version present", which is brittle; update the assertion that references
assert_json_field for the "version" field to verify presence/non-empty value
instead of a literal string (e.g., assert_json_field ... "version" with a
non-empty matcher like ".+" or use an existing presence-check helper), keeping
the test name "version present"; alternatively, if you want exact-version
enforcement, keep the literal but rename the test description to "version =
4.3.1" and document the need to update this test on each release.
| plugin_version=$(python3 -c "import json; print(json.load(open('$REPO_ROOT/.factory-plugin/plugin.json'))['version'])" 2>/dev/null) | ||
| marketplace_version=$(python3 -c "import json; print(json.load(open('$REPO_ROOT/.factory-plugin/marketplace.json'))['plugins'][0]['version'])" 2>/dev/null) | ||
|
|
||
| if [ "$plugin_version" = "$marketplace_version" ]; then | ||
| echo " [PASS] Versions match: $plugin_version" | ||
| else | ||
| echo " [FAIL] Version mismatch: plugin.json=$plugin_version, marketplace.json=$marketplace_version" | ||
| FAILED=$((FAILED + 1)) | ||
| fi |
There was a problem hiding this comment.
False-positive PASS in Test 6 when python3 fails silently.
Both plugin_version and marketplace_version are captured with 2>/dev/null. If either JSON file is missing or malformed (which could happen if earlier tests failed but execution continued), python3 exits non-zero, both variables become empty strings, and [ "" = "" ] evaluates to true — printing [PASS] Versions match: with no version shown.
🛠️ Proposed fix
plugin_version=$(python3 -c "import json; print(json.load(open('$REPO_ROOT/.factory-plugin/plugin.json'))['version'])" 2>/dev/null)
marketplace_version=$(python3 -c "import json; print(json.load(open('$REPO_ROOT/.factory-plugin/marketplace.json'))['plugins'][0]['version'])" 2>/dev/null)
+if [ -z "$plugin_version" ] || [ -z "$marketplace_version" ]; then
+ echo " [FAIL] Could not read version from one or both JSON files"
+ FAILED=$((FAILED + 1))
+elif [ "$plugin_version" = "$marketplace_version" ]; then
-if [ "$plugin_version" = "$marketplace_version" ]; then
echo " [PASS] Versions match: $plugin_version"
else
echo " [FAIL] Version mismatch: plugin.json=$plugin_version, marketplace.json=$marketplace_version"
FAILED=$((FAILED + 1))
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| plugin_version=$(python3 -c "import json; print(json.load(open('$REPO_ROOT/.factory-plugin/plugin.json'))['version'])" 2>/dev/null) | |
| marketplace_version=$(python3 -c "import json; print(json.load(open('$REPO_ROOT/.factory-plugin/marketplace.json'))['plugins'][0]['version'])" 2>/dev/null) | |
| if [ "$plugin_version" = "$marketplace_version" ]; then | |
| echo " [PASS] Versions match: $plugin_version" | |
| else | |
| echo " [FAIL] Version mismatch: plugin.json=$plugin_version, marketplace.json=$marketplace_version" | |
| FAILED=$((FAILED + 1)) | |
| fi | |
| plugin_version=$(python3 -c "import json; print(json.load(open('$REPO_ROOT/.factory-plugin/plugin.json'))['version'])" 2>/dev/null) | |
| marketplace_version=$(python3 -c "import json; print(json.load(open('$REPO_ROOT/.factory-plugin/marketplace.json'))['plugins'][0]['version'])" 2>/dev/null) | |
| if [ -z "$plugin_version" ] || [ -z "$marketplace_version" ]; then | |
| echo " [FAIL] Could not read version from one or both JSON files" | |
| FAILED=$((FAILED + 1)) | |
| elif [ "$plugin_version" = "$marketplace_version" ]; then | |
| echo " [PASS] Versions match: $plugin_version" | |
| else | |
| echo " [FAIL] Version mismatch: plugin.json=$plugin_version, marketplace.json=$marketplace_version" | |
| FAILED=$((FAILED + 1)) | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/droid/test-plugin-manifests.sh` around lines 65 - 73, The current
comparison treats empty strings as a match because both plugin_version and
marketplace_version are set via python3 calls silenced with 2>/dev/null; update
the logic around the variables plugin_version and marketplace_version to detect
python failures: do not discard stderr (remove 2>/dev/null) or capture the
python exit status ($?) after each assignment and fail if non-zero, and also
check that both variables are non-empty before comparing; if either extraction
fails, print a clear [FAIL] with context and increment FAILED instead of
proceeding to the equality check.
| DROID_PLUGIN_ROOT="$REPO_ROOT" bash "$HOOK_SCRIPT" > /dev/null 2>&1 | ||
| exit_code=$? | ||
| if [ $exit_code -eq 0 ]; then | ||
| echo " [PASS] Exit code is 0" | ||
| else | ||
| echo " [FAIL] Exit code is $exit_code (expected 0)" | ||
| FAILED=$((FAILED + 1)) | ||
| fi |
There was a problem hiding this comment.
set -e will kill the test script at line 44 if the hook exits non-zero — exit_code is never captured.
Line 32 and line 56 both correctly append || true to prevent set -euo pipefail from aborting on hook failure. Line 44 is missing the same guard; if the hook exits non-zero for any reason (e.g., an error before the DROID_PLUGIN_ROOT guard fires), the test script exits immediately and neither the FAILED counter nor the summary block is reached.
🐛 Proposed fix
-DROID_PLUGIN_ROOT="$REPO_ROOT" bash "$HOOK_SCRIPT" > /dev/null 2>&1
-exit_code=$?
+exit_code=0
+DROID_PLUGIN_ROOT="$REPO_ROOT" bash "$HOOK_SCRIPT" > /dev/null 2>&1 || exit_code=$?📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DROID_PLUGIN_ROOT="$REPO_ROOT" bash "$HOOK_SCRIPT" > /dev/null 2>&1 | |
| exit_code=$? | |
| if [ $exit_code -eq 0 ]; then | |
| echo " [PASS] Exit code is 0" | |
| else | |
| echo " [FAIL] Exit code is $exit_code (expected 0)" | |
| FAILED=$((FAILED + 1)) | |
| fi | |
| exit_code=0 | |
| DROID_PLUGIN_ROOT="$REPO_ROOT" bash "$HOOK_SCRIPT" > /dev/null 2>&1 || exit_code=$? | |
| if [ $exit_code -eq 0 ]; then | |
| echo " [PASS] Exit code is 0" | |
| else | |
| echo " [FAIL] Exit code is $exit_code (expected 0)" | |
| FAILED=$((FAILED + 1)) | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/droid/test-session-start-hook.sh` around lines 44 - 51, The test
currently invokes the hook with "DROID_PLUGIN_ROOT=\"$REPO_ROOT\" bash
\"$HOOK_SCRIPT\"" under set -euo pipefail so a non-zero hook will abort the
script before exit_code and FAILED are updated; modify the invocation to prevent
immediate exit by adding a safe fallback (append "|| true") so the command never
triggers set -e to exit, then capture exit_code and update FAILED accordingly;
targets: the invocation that uses DROID_PLUGIN_ROOT and HOOK_SCRIPT, the
exit_code variable capture, and the FAILED counter update.
| for skill_dir in "$SKILLS_DIR"/*/; do | ||
| skill_name=$(basename "$skill_dir") | ||
| if [ -f "$skill_dir/SKILL.md" ]; then | ||
| echo " [PASS] $skill_name/SKILL.md exists" | ||
| else | ||
| echo " [FAIL] $skill_name/SKILL.md missing" | ||
| FAILED=$((FAILED + 1)) | ||
| fi | ||
| done | ||
| echo "" | ||
|
|
||
| # Test 3: Each SKILL.md has a description (first non-empty line or frontmatter) | ||
| echo "Test 3: SKILL.md has content..." | ||
| for skill_dir in "$SKILLS_DIR"/*/; do | ||
| skill_name=$(basename "$skill_dir") | ||
| skill_file="$skill_dir/SKILL.md" | ||
| [ -f "$skill_file" ] || continue | ||
|
|
||
| line_count=$(wc -l < "$skill_file" | tr -d ' ') | ||
| if [ "$line_count" -gt 3 ]; then | ||
| echo " [PASS] $skill_name/SKILL.md has content ($line_count lines)" | ||
| else | ||
| echo " [FAIL] $skill_name/SKILL.md is too short ($line_count lines)" | ||
| FAILED=$((FAILED + 1)) | ||
| fi | ||
| done |
There was a problem hiding this comment.
Glob without nullglob produces a spurious failure on an empty skills directory.
If $SKILLS_DIR contains no subdirectories, "$SKILLS_DIR"/*/ is unexpanded and the loop iterates once with the literal pattern as $skill_dir, causing both Test 2 and Test 3 to report a false failure.
🛠️ Proposed fix
+shopt -s nullglob
# Test 2: Every skill directory has a SKILL.md
echo "Test 2: SKILL.md presence in each skill..."
for skill_dir in "$SKILLS_DIR"/*/; doApply the same fix before the Test 3 loop, or set nullglob once at the top of the script.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/droid/test-skill-discovery.sh` around lines 22 - 47, The Test 3 loop
can iterate the literal pattern when no subdirs exist because the shell nullglob
option isn't set; enable nullglob before the loop (or at the script top) so
"$SKILLS_DIR"/*/ expands to zero entries instead of the pattern, ensuring the
for loop over skill_dir and checks around skill_file/SKILL.md (variables
skill_dir, skill_file, SKILLS_DIR and the Test 3 loop) do not produce spurious
failures.
| # Test 4: No duplicate skill names | ||
| echo "Test 4: No duplicate skill names..." | ||
| skill_names=$(ls -1 "$SKILLS_DIR") | ||
| unique_count=$(echo "$skill_names" | sort -u | wc -l | tr -d ' ') | ||
| total_count=$(echo "$skill_names" | wc -l | tr -d ' ') | ||
| if [ "$unique_count" -eq "$total_count" ]; then | ||
| echo " [PASS] All $total_count skill names are unique" | ||
| else | ||
| echo " [FAIL] Duplicate skill names found" | ||
| echo "$skill_names" | sort | uniq -d | sed 's/^/ /' | ||
| FAILED=$((FAILED + 1)) | ||
| fi |
There was a problem hiding this comment.
Script aborts silently via set -e if $SKILLS_DIR doesn't exist.
When Test 1 fails and execution continues, the standalone skill_names=$(ls -1 "$SKILLS_DIR") assignment on line 52 exits non-zero (directory missing), set -e terminates the script immediately without reaching the summary or recording the failure in FAILED.
Guard the test or use a safe fallback:
🛠️ Proposed fix
# Test 4: No duplicate skill names
echo "Test 4: No duplicate skill names..."
-skill_names=$(ls -1 "$SKILLS_DIR")
-unique_count=$(echo "$skill_names" | sort -u | wc -l | tr -d ' ')
-total_count=$(echo "$skill_names" | wc -l | tr -d ' ')
-if [ "$unique_count" -eq "$total_count" ]; then
- echo " [PASS] All $total_count skill names are unique"
-else
- echo " [FAIL] Duplicate skill names found"
- echo "$skill_names" | sort | uniq -d | sed 's/^/ /'
- FAILED=$((FAILED + 1))
-fi
+if [ -d "$SKILLS_DIR" ]; then
+ skill_names=$(ls -1 "$SKILLS_DIR")
+ unique_count=$(echo "$skill_names" | sort -u | wc -l | tr -d ' ')
+ total_count=$(echo "$skill_names" | wc -l | tr -d ' ')
+ if [ "$unique_count" -eq "$total_count" ]; then
+ echo " [PASS] All $total_count skill names are unique"
+ else
+ echo " [FAIL] Duplicate skill names found"
+ echo "$skill_names" | sort | uniq -d | sed 's/^/ /'
+ FAILED=$((FAILED + 1))
+ fi
+else
+ echo " [SKIP] $SKILLS_DIR not found (already failed Test 1)"
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Test 4: No duplicate skill names | |
| echo "Test 4: No duplicate skill names..." | |
| skill_names=$(ls -1 "$SKILLS_DIR") | |
| unique_count=$(echo "$skill_names" | sort -u | wc -l | tr -d ' ') | |
| total_count=$(echo "$skill_names" | wc -l | tr -d ' ') | |
| if [ "$unique_count" -eq "$total_count" ]; then | |
| echo " [PASS] All $total_count skill names are unique" | |
| else | |
| echo " [FAIL] Duplicate skill names found" | |
| echo "$skill_names" | sort | uniq -d | sed 's/^/ /' | |
| FAILED=$((FAILED + 1)) | |
| fi | |
| # Test 4: No duplicate skill names | |
| echo "Test 4: No duplicate skill names..." | |
| if [ -d "$SKILLS_DIR" ]; then | |
| skill_names=$(ls -1 "$SKILLS_DIR") | |
| unique_count=$(echo "$skill_names" | sort -u | wc -l | tr -d ' ') | |
| total_count=$(echo "$skill_names" | wc -l | tr -d ' ') | |
| if [ "$unique_count" -eq "$total_count" ]; then | |
| echo " [PASS] All $total_count skill names are unique" | |
| else | |
| echo " [FAIL] Duplicate skill names found" | |
| echo "$skill_names" | sort | uniq -d | sed 's/^/ /' | |
| FAILED=$((FAILED + 1)) | |
| fi | |
| else | |
| echo " [SKIP] $SKILLS_DIR not found (already failed Test 1)" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/droid/test-skill-discovery.sh` around lines 50 - 61, The test currently
runs skill_names=$(ls -1 "$SKILLS_DIR") which will cause the script to abort
under set -e if SKILLS_DIR is missing; update the Test 4 block to first guard
the directory (check [ -d "$SKILLS_DIR" ] or use a safe fallback for ls like
redirecting errors and allowing empty output), and if the directory is missing
increment FAILED and print a clear failure message; reference the SKILLS_DIR
variable, the skill_names assignment, and the FAILED counter so you add the
directory existence check before computing skill_names and ensure the failure
path increments FAILED and does not let the script exit early.
@obra Thanks for your time and reply! I didn't realize the architectural design, which led to such an inelegant situation. I've re-optimized the implementation. I've organized the issues I encountered below. Some design-related questions still need discussion and feedback to avoid any incompatibility issues. If you have any feedback or suggestions, please let me know. SessionStart Hook BehaviorThe upstream Factory Droid has a native Skill tool — once a plugin is installed, skills are automatically registered in the available list. However, testing revealed:
Current approach: Add Factory environment detection in Why Modify hooks/session-startThe upstream Factory Droid is different:
Therefore, environment detection was added at the beginning of the script: if [ -n "${DROID_PLUGIN_ROOT:-}" ]; then
exit 0
fi
Approaches tried but ineffective:
The tests result 
|
obra
added
duplicate
|
Closing this for the same reason as #139. The author engaged constructively in 2026-02-24 to address the maintainer's concern about the custom In #1137 the maintainer subsequently signed up for Factory and reported: "I was 100% able to install the native superpowers claude code plugin in Factory with zero hiccups. It appears to have..just worked?" — meaning a Factory-specific integration PR is likely no longer needed. @vurihuang — really do appreciate the careful engagement. If a Factory-specific gap surfaces that the native install doesn't cover, please open a fresh PR with the specific need. — Claude Opus 4.7, Claude Code 2.1.112 |
2 participants
Motivation and Context