revert: rollback to pre-plugin-consolidation state - #501
Conversation
Rollback commits after 86f75f1 to restore: - plugins/me/ - plugins/suggest-compacting/ - plugins/ralph-loop/ - scripts/ralph/ - skills/suggest-compacting/ - src/suggest-compacting/ - commands/ralph-*.md - agents/ralph/config.sh.example This preserves history while reverting the plugin consolidation changes.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR restructures Claude Code's plugin architecture from root-based discovery to a plugins/* directory structure, updates versions across plugins, introduces five new plugins (ralph-loop, handoff, git-guard, jira, suggest-compacting), adds extensive documentation and skills for AI-driven iteration workflows, refactors create-pr with conflict detection and PR status verification, enhances git workflow protection via commit-guard.sh, and removes eval-driven development features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Claude as Claude Instance N
participant PID as PID Manager
participant PRD as PRD/Progress
participant Git as Git Repo
participant Shell as Bash Loop
User->>Shell: /ralph-init (create PRD)
User->>Shell: /ralph-loop (start iterations)
Shell->>PRD: read prd.json stories
loop Each Iteration
Shell->>Claude: spawn fresh instance with prompt
Claude->>PRD: read current story
Claude->>Git: implement story changes
Claude->>Git: run tests & verify
Claude->>Shell: emit result or COMPLETE
Shell->>PRD: update progress.txt
Shell->>Git: commit on success
alt Detects COMPLETE
Shell->>PID: clean up PID file
Shell->>User: report completion
else Max Iterations Reached
Shell->>User: report max iterations
end
end
sequenceDiagram
participant Session1 as Session 1
participant SessionStart as SessionStart Hook
participant Handoff as Handoff Storage
participant User
participant Session2 as Session 2
participant Pickup as Pickup Command
Session1->>User: /handoff (save context)
User->>Handoff: /handoff summarizes & saves
Handoff->>Handoff: writes JSON to ~/.claude/handoffs/
Session1->>Handoff: includes plan_path, tasks_session_id
Note over Session2: New session starts
Session2->>SessionStart: SessionStart hook fires
SessionStart->>Handoff: check for recent unloaded handoffs
Handoff->>User: notify about available handoffs
User->>Pickup: /pickup {uuid}
Pickup->>Handoff: load handoff JSON
Pickup->>User: restore plan, tasks, context
Pickup->>Handoff: mark as loaded_at
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 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 |
![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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/run-all-tests.sh (1)
26-42:⚠️ Potential issue | 🟠 Major
set -ecauses early exit on first test failure, making the summary unreachable.
run_testsreturns1on failure (line 41). Withset -euo pipefail(line 10), bare calls likerun_tests ...on lines 55 and 66 will terminate the script immediately on the first failure. TheFAILED_TESTSaccumulator and the summary block (lines 77-86) are effectively dead code.Suppress
set -efor these calls so failures are collected:Proposed fix
for dir in "${test_dirs[@]}"; do if [ -d "${SCRIPT_DIR}/${dir}" ]; then - run_tests "${SCRIPT_DIR}/${dir}" "${dir} tests" + run_tests "${SCRIPT_DIR}/${dir}" "${dir} tests" || true fi doneAnd similarly for line 66:
- run_tests "${SCRIPT_DIR}" "Root tests" + run_tests "${SCRIPT_DIR}" "Root tests" || trueAlso applies to: 55-56, 66-66
🤖 Fix all issues with AI agents
In `@docs/TESTING.md`:
- Around line 48-54: Update the test count table entries for the listed test
categories in TESTING.md: replace "Unit Tests: 148" with "Unit Tests: 290",
change "Integration Tests: 20 tests (10 plugin loading + 10 cross-plugin
interactions)" to "Integration Tests: 18 tests (8 plugin_loading + 10
cross_plugin_interactions)", keep "Performance Tests: 10", "Error Handling
Tests: 23", "Edge Case Tests: 17", and "Negative Tests: 17" as-is, and update
the bold total from "Total: 178 tests" to "Total: 397 tests" so the documented
counts for the entries named "Unit Tests", "Integration Tests", and the bold
"Total" reflect the actual suite.
In `@hooks/commit-guard.sh`:
- Around line 113-119: The grep -E patterns in validate_git_command use \s and
\S which are not portable to BSD grep; update every matches_pattern call in
validate_git_command to replace \s with [[:space:]] and \S with [^[:space:]]
(e.g., change "^\s*(\S*=\S*\s+)*git\s+" to
"^[[:space:]]*([^[:space:]]*=[^[:space:]]*[[:space:]]+)*git[[:space:]]+" and
similarly change "git\s+commit.*--no-verify" ->
"git[[:space:]]+commit.*--no-verify", "git\s+commit" -> "git[[:space:]]+commit",
"git\s+.*skip.*hooks" -> "git[[:space:]]+.*skip.*hooks", "git\s+.*--no-.*hook"
-> "git[[:space:]]+.*--no-.*hook", "git\s+update-ref" ->
"git[[:space:]]+update-ref", "git\s+filter-branch" ->
"git[[:space:]]+filter-branch", and "git\s+config.*core\.hooksPath" ->
"git[[:space:]]+config.*core\.hooksPath"); modify the patterns where
matches_pattern is invoked so the security checks work on macOS.
In `@plugins/me/scripts/check-conflicts.sh`:
- Around line 50-84: The three-argument git merge-tree call (git merge-tree
"$MERGE_BASE" HEAD "origin/$BASE") always exits 0 so MERGE_EXIT is meaningless
and the conflict-handling block never runs; also set -euo pipefail can
prematurely abort if you switch to the newer --write-tree form. Fix by either
(A) keeping the legacy form and detect conflicts by parsing MERGE_OUTPUT for
"CONFLICT" (use the existing MERGE_OUTPUT variable and conditional like if echo
"$MERGE_OUTPUT" | grep -q "CONFLICT"; then ... fi) or (B) switch to the modern
git merge-tree --write-tree form and temporarily disable exit-on-error around
that call (save/restore errexit with set +e / set -e or run the command in a
subshell) so you can capture its non-zero exit in MERGE_EXIT; update the
subsequent checks to use the chosen detection (MERGE_OUTPUT contains markers or
MERGE_EXIT) and remove the dead-code assumptions.
In `@plugins/me/scripts/verify-pr-status.sh`:
- Around line 73-89: The script uses set -euo pipefail so the git merge command
will abort the script before the existing if [[ $? -ne 0 ]] block runs; change
the merge invocation to handle failures explicitly — either wrap the merge with
a controlled failure handler (e.g. run git merge origin/"$BASE" --no-edit || {
<conflict-handling-echoes>; exit 1; }) or temporarily disable errexit around the
merge (set +e; git merge origin/"$BASE" --no-edit; ret=$?; set -e; if [[ $ret
-ne 0 ]]; then <conflict-handling-echoes>; exit 1; fi), and remove or replace
the unreachable if [[ $? -ne 0 ]] branch accordingly so the conflict messages
are emitted when the merge fails.
In `@scripts/ralph/ralph.sh`:
- Around line 313-325: The current pattern loses Claude's real exit code because
exit_code=$? is evaluated inside the then-block of the negated if; change the
flow to run the claude command and capture its exit status immediately into
exit_code before branching: execute the command capturing stdout into OUTPUT and
appending to "$ITERATION_LOG" while preserving the exit status (e.g.
OUTPUT=$(echo "$PROMPT" | claude --print --dangerously-skip-permissions 2>&1 |
tee -a "$ITERATION_LOG") || exit_code=$?; exit_code=${exit_code:-0}), then check
if exit_code is non-zero to log to "$ERRORS_LOG" and inspect [[ $exit_code -gt 1
]] to write the fatal ACTIVITY_LOG entry and exit; if set -e is enabled, use the
"|| exit_code=$?" suffix (or wrap the command in a subshell) so errexit doesn't
abort the script before you capture exit_code.
In `@skills/generate-status-report/scripts/jql_builder.py`:
- Around line 106-109: The ORDER BY branch uses sanitize_jql_value(order_by)
which rejects commas so default "priority DESC, updated DESC" raises ValueError;
fix by either extending sanitize_jql_value to permit commas and commas+space
(e.g., add ',' to the allowed character set) or implement a dedicated sanitizer
(e.g., sanitize_jql_order_by) that allows comma-separated field directions and
validates each token only contains allowed field names and "ASC"/"DESC"; update
the ORDER BY branch to call the chosen sanitizer before appending f' ORDER BY
{order_by}' so multi-field order clauses pass validation.
In `@src/suggest-compacting/tsconfig.json`:
- Around line 6-7: The tsconfig in this package points rootDir at "./src" and
includes "src/**/*" which doesn't match actual source files (auto-compact.ts,
session-start.ts, lib/state.ts) in the package root; update the tsconfig.json to
set "rootDir" to "./" and change "include" to ["**/*"] (or alternatively move
the TypeScript files into a nested src/ directory) so the compiler actually
finds and emits these files.
🟠 Major comments (17)
skills/opensearch/SKILL.md-39-44 (1)
39-44:⚠️ Potential issue | 🟠 MajorRemove or create the referenced resource directories and files.
Lines 41–43 reference
references/,examples/, andscripts/health-check.sh, but these do not exist in theskills/opensearch/directory. Either create these resources as part of the PR or remove these references from the documentation to avoid broken links.plugins/lsp-python/.claude-plugin/plugin.json-19-21 (1)
19-21:⚠️ Potential issue | 🟠 MajorRevert command to
pyright-langserver—pyright --stdiowill fail.The
pyrightnpm package provides two separate executables:pyright(CLI type-checker) andpyright-langserver(LSP server). Usingpyright --stdiois incorrect and will error with "Unexpected option --stdio". The LSP server must be invoked withpyright-langserver --stdio.hooks/session-start.sh-44-48 (1)
44-48:⚠️ Potential issue | 🟠 MajorShell-variable interpolation into Python code is an injection risk.
$created_atand$FIVE_MIN_AGOare spliced directly into a Python-cstring. A malformed or adversarialcreated_atvalue in a handoff JSON file (e.g., containing') would break the Python expression or execute arbitrary code. Although the files live under$HOME, this is still a code-injection footgun.Pass the values as arguments instead:
Proposed fix
- if python3 -c "from datetime import datetime; d1=datetime.fromisoformat('$created_at'.replace('Z', '+00:00')); d2=datetime.fromisoformat('$FIVE_MIN_AGO'.replace('Z', '+00:00')); exit(0 if d1 >= d2 else 1)" 2>/dev/null; then + if python3 -c " +import sys +from datetime import datetime, timezone +d1 = datetime.fromisoformat(sys.argv[1].replace('Z', '+00:00')) +d2 = datetime.fromisoformat(sys.argv[2].replace('Z', '+00:00')) +sys.exit(0 if d1 >= d2 else 1) +" "$created_at" "$FIVE_MIN_AGO" 2>/dev/null; thenThis also avoids the deprecated
datetime.utcnow()used on line 17 (deprecated since Python 3.12).plugins/handoff/scripts/handoff.sh-7-8 (1)
7-8:⚠️ Potential issue | 🟠 MajorMissing argument validation will cause unbound variable error.
With
set -u(line 2), accessing$1without a default when no argument is passed will abort the script with an unhelpful "unbound variable" error. Add a guard or default.Proposed fix
-# Arguments: $1 = summary -SUMMARY="$1" +# Arguments: $1 = summary +if [[ $# -lt 1 || -z "$1" ]]; then + echo "Usage: handoff.sh <summary>" >&2 + exit 1 +fi +SUMMARY="$1"plugins/handoff/scripts/handoff.sh-28-40 (1)
28-40:⚠️ Potential issue | 🟠 Major
stat -fis macOS-only — this silently fails on Linux.
stat -f "%m %N"is a macOS/BSD syntax. On GNU/Linux, the equivalent isstat -c "%Y %n". Since errors are swallowed by2>/dev/null, on Linux this will silently produce an emptyPLAN_PATHandTASKS_SESSION_IDwith no indication of failure.Proposed cross-platform fix
+# Cross-platform stat for modification time +stat_mtime() { + if stat -f "%m %N" "$1" 2>/dev/null; then + return + fi + stat -c "%Y %n" "$1" 2>/dev/null +} + # Detect session references # Check for active plan (most recent .md file in ~/.claude/plans/) PLAN_PATH="" if [ -d "$HOME/.claude/plans" ]; then - PLAN_PATH=$(find "$HOME/.claude/plans" -name "*.md" -type f -exec stat -f "%m %N" {} \; 2>/dev/null | sort -rn | head -1 | cut -d' ' -f2-) + PLAN_PATH=$(find "$HOME/.claude/plans" -name "*.md" -type f -print0 2>/dev/null | while IFS= read -r -d '' f; do stat_mtime "$f"; done | sort -rn | head -1 | cut -d' ' -f2-)As per coding guidelines, "Shell scripts should pass ShellCheck validation."
plugins/handoff/scripts/pickup.sh-143-144 (1)
143-144:⚠️ Potential issue | 🟠 Major
PROJECT_PATHinterpolated unsafely into jq filter string.
PROJECT_PATHis spliced directly into the jq expression via shell string concatenation. If the path contains"or\, the jq filter will break or behave unexpectedly. Use--arginstead for safe variable passing.🔧 Proposed fix
- HANDOFF_FILE=$(find "$HANDOFF_DIR" -name "*.json" -type f -exec jq -r 'select(.project_path == "'"$PROJECT_PATH"'") | select(.loaded_at == null) | .id + " " + .created_at' {} \; 2>/dev/null | sort -k2 -r | head -1 | cut -d' ' -f1) + HANDOFF_FILE=$(find "$HANDOFF_DIR" -name "*.json" -type f -exec jq -r --arg pp "$PROJECT_PATH" 'select(.project_path == $pp) | select(.loaded_at == null) | .id + " " + .created_at' {} \; 2>/dev/null | sort -k2 -r | head -1 | cut -d' ' -f1)plugins/handoff/scripts/pickup.sh-114-123 (1)
114-123:⚠️ Potential issue | 🟠 MajorUnsanitized UUID allows path traversal.
The
$1argument is used directly in the file path without validation. A value like../../etc/passwdwould escapeHANDOFF_DIR. Validate that the UUID matches the expected format before constructing the path.🛡️ Proposed fix
# Load specific handoff by UUID UUID="$1" + + # Validate UUID format (alphanumeric + hyphens only) + if [[ ! "$UUID" =~ ^[a-zA-Z0-9_-]+$ ]]; then + echo "Error: Invalid handoff ID format: $UUID" >&2 + exit 1 + fi + HANDOFF_FILE="$HANDOFF_DIR/${UUID}.json"commands/handoff.md-5-5 (1)
5-5:⚠️ Potential issue | 🟠 Major
Bash(*)is overly permissive — scope to the handoff script.
handoff.mdis the only command granting unrestricted Bash access. Other commands likehandoff-list.mdandpickup.mdcorrectly restrict to specific scripts, andralph-loop.mddemonstrates how to support script arguments with a pattern likeBash(${CLAUDE_PLUGIN_ROOT}/scripts/ralph.sh*). Scope this to match:-allowed-tools: Bash(*) +allowed-tools: ["Bash(${CLAUDE_PLUGIN_ROOT}/scripts/handoff.sh*)"]tests/run-all-tests.sh-51-51 (1)
51-51:⚠️ Potential issue | 🟠 MajorAdd
suggest-compactingto test_dirs array.The
tests/suggest-compacting/directory exists with tests that should be run as part of the consolidated test suite. Add it to thetest_dirsarray on line 51 for consistency with other plugin test directories.Note:
ralph-looptests exist as root-level bats files (tests/ralph_loop_command_tests.bats,tests/ralph_loop_script_tests.bats) and will be executed as part of the root tests (line 66), so they do not need to be added totest_dirs.src/suggest-compacting/session-start.ts-23-29 (1)
23-29:⚠️ Potential issue | 🟠 Major
readStdinnever rejects on error — process can hang indefinitely.If
process.stdinemits an'error'event, the promise is never settled and the process hangs.Proposed fix
function readStdin(): Promise<string> { return new Promise((resolve, reject) => { let data = ''; process.stdin.on('data', (chunk) => data += chunk); process.stdin.on('end', () => resolve(data)); + process.stdin.on('error', (err) => reject(err)); }); }plugins/ralph-loop/.claude-plugin/marketplace.json-3-3 (1)
3-3:⚠️ Potential issue | 🟠 MajorCorrect marketplace.json version to match plugin.json: change
5.7.1to5.23.3.The marketplace.json declares version
5.7.1while plugin.json declares5.23.3. This version mismatch can cause confusion in marketplace tooling and validation.src/suggest-compacting/auto-compact.ts-35-41 (1)
35-41:⚠️ Potential issue | 🟠 Major
readStdincan hang if stdin emits an error.If
process.stdinemits an'error'event, the promise never settles, potentially blocking the hook pipeline indefinitely.Proposed fix
function readStdin(): Promise<string> { - return new Promise((resolve) => { + return new Promise((resolve, reject) => { let data = ''; process.stdin.on('data', (chunk) => data += chunk); process.stdin.on('end', () => resolve(data)); + process.stdin.on('error', reject); }); }hooks/commit-guard.sh-41-111 (1)
41-111:⚠️ Potential issue | 🟠 Major
extract_commit_messageuses two parsing passes, and the first corrupts the input for the second.The first pass (lines 47-57) uses a
while =~loop that removes matched substrings fromcommand. Then the second pass (lines 60-108) does character-by-character parsing on the already-modifiedcommandstring. This means quoted messages partially consumed by the regex pass will be invisible to (or garbled for) the character parser.Additionally, line 47's
\shas the same POSIX portability issue flagged above, so on macOS the entire first pass silently does nothing — which coincidentally may make the second pass work correctly on the original string.Consider using only one parsing strategy. The character-by-character parser (lines 60-108) is more robust and could handle all cases on its own.
♻️ Simplify to a single parsing pass
Remove lines 45-57 (the regex pass) and operate the character-by-character parser on the original
commandvalue. This also eliminates the\sportability problem in the regex.src/suggest-compacting/lib/state.ts-18-27 (1)
18-27:⚠️ Potential issue | 🟠 Major
readState,writeState, andincrementStatedon't validatesessionId, creating a path-traversal risk.
isValidSessionIdexists (line 13) but is never enforced inside these functions. SincesessionIdis interpolated directly into a file path viaSTATE_FILE(), a caller that forgets to validate could be exploited with a value like../../etc/passwd. The relevant snippet fromauto-compact.jsshows thesessionIdcomes from external JSON input (input.session_id).🛡️ Proposed fix: validate at the boundary
export async function readState(sessionId: string): Promise<ToolCountState | null> { + if (!isValidSessionId(sessionId)) { + throw new Error(`Invalid session ID: ${sessionId}`); + } const filepath = STATE_FILE(sessionId);Apply similarly to
writeState(validatestate.sessionId) andincrementState.Also applies to: 30-34, 37-44
src/suggest-compacting/lib/state.ts-20-23 (1)
20-23:⚠️ Potential issue | 🟠 Major
parseIntcan returnNaN, silently corrupting the count.If the state file contains non-numeric data (corruption, empty file, etc.),
parseIntreturnsNaN. This propagates throughincrementStateasNaN + 1 → NaN, silently breaking the counter without any error signal.🐛 Proposed fix: validate the parsed integer
const content = await fs.readFile(filepath, 'utf-8'); const count = parseInt(content.trim(), 10); - return { count, sessionId }; + if (Number.isNaN(count)) { + return null; + } + return { count, sessionId };tests/suggest-compacting/suggest-compacting.bats-3-11 (1)
3-11:⚠️ Potential issue | 🟠 MajorTest state is written to the real
$HOMEdirectory, breaking test isolation.
TEST_STATE_DIR(line 6) andTEST_STATE_FILE(line 7) point to$HOME/.claude/suggest-compacting, which is the production path. Running these tests on a developer machine will create/modify files under their real~/.claude/directory. The TypeScript code instate.tsreadsprocess.env.HOMEto derive the state directory, so overridingHOMEin the test environment would fix this.🧪 Proposed fix: isolate test state using a temp HOME
setup() { # Setup test environment + export ORIGINAL_HOME="$HOME" + export HOME="$(mktemp -d -t suggest-compacting-home.XXXXXX)" export TEST_SESSION_ID="test-session-123" - export TEST_STATE_DIR="$HOME/.claude/suggest-compacting" + export TEST_STATE_DIR="$HOME/.claude/suggest-compacting" # now points to temp HOME export TEST_STATE_FILE="$TEST_STATE_DIR/tool-count-$TEST_SESSION_ID.txt" export CLAUDE_PLUGIN_ROOT="$PWD/plugins/suggest-compacting" # Clean up any existing test state rm -f "$TEST_STATE_FILE" 2>/dev/null || true } teardown() { # Clean up test state - rm -f "$TEST_STATE_FILE" 2>/dev/null || true + rm -rf "$HOME/.claude" 2>/dev/null || true + rm -rf "$HOME" + export HOME="$ORIGINAL_HOME" }skills/create-pr/scripts/check-conflicts.sh-41-45 (1)
41-45:⚠️ Potential issue | 🟠 MajorThe 3-argument
git merge-treeform does not use exit codes to signal conflicts on Git < 2.38.On Git versions prior to 2.38,
git merge-tree <base> <branch1> <branch2>unconditionally exits with status 0 regardless of conflicts—it only prints results to stdout. The exit-code-based conflict detection only works with Git 2.38+ and the--write-treeflag. This means the script will always report "No conflicts detected" on older Git versions, never reaching the conflict analysis code on line 55.Use one of these fixes:
- Switch to
git merge-tree --write-tree(requires Git ≥ 2.38), or- Always check the output for
CONFLICTmarkers instead of relying on the exit code.Option 2: Check output regardless of exit code
-if MERGE_OUTPUT=$(git merge-tree "$MERGE_BASE" HEAD "origin/$BASE" 2>&1); then - echo "OK: No conflicts detected" - echo " - Current branch merges cleanly with origin/$BASE" - exit 0 -fi +MERGE_OUTPUT=$(git merge-tree "$MERGE_BASE" HEAD "origin/$BASE" 2>&1) || true + +if ! echo "$MERGE_OUTPUT" | grep -q "CONFLICT"; then + echo "OK: No conflicts detected" + echo " - Current branch merges cleanly with origin/$BASE" + exit 0 +fi
🟡 Minor comments (33)
skills/using-git-worktrees/SKILL.md-32-34 (1)
32-34:⚠️ Potential issue | 🟡 MinorFix typo in grep pattern ("director" → "directory").
Current pattern won’t match expected text in CLAUDE.md.✅ Proposed fix
-grep -i "worktree.*director" CLAUDE.md 2>/dev/null +grep -i "worktree.*directory" CLAUDE.md 2>/dev/nullCODE_CLEANUP_TEST.md-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorThis looks like a test/debug artifact — should it be committed?
CODE_CLEANUP_TEST.mdcontaining only a heading with a timestamp-like number (1770684849) doesn't appear in the PR's listed paths to restore. If this was used to verify cleanup behavior, it should be removed before merge.Also, the file is missing a trailing newline (markdownlint MD047). As per coding guidelines, markdown files should pass markdownlint validation.
skills/triage-issue/references/bug-report-templates.md-20-63 (1)
20-63:⚠️ Potential issue | 🟡 MinorNested triple-backtick fences break Markdown rendering.
Each template embeds inner
```fences (e.g., lines 25, 29, 54) inside an outer```markdownfence (line 20). Standard Markdown parsers will interpret the first inner```as closing the outer fence, causing the rest of the template to render as raw text. This same pattern recurs in all six templates.Fix by using four-backtick (``````) fences for the outer blocks, or indent inner fences by 4 spaces.
As per coding guidelines, "Markdown files should pass markdownlint validation."
tests/skills/test_skill_content.bats-6-16 (1)
6-16:⚠️ Potential issue | 🟡 MinorThe
setup()skip makes the "SKILL.md exists" test unable to fail.
setup()skips all tests when the file is absent, so the@test "SKILL.md exists"on line 14 can only ever pass or be skipped — it can never report a failure. If the intent is to assert the file's presence, move the skip guard out ofsetup()and into only the tests that read the file's content.Proposed fix
setup() { export SKILL_MD="${BATS_TEST_DIRNAME}/../../skills/create-pr/SKILL.md" - - if [[ ! -f "$SKILL_MD" ]]; then - skip "SKILL.md not found" - fi } `@test` "SKILL.md exists" { [ -f "$SKILL_MD" ] } `@test` "SKILL.md has required sections" { + [[ -f "$SKILL_MD" ]] || skip "SKILL.md not found" # Required sections that all skills should have grep -q "^## Overview" "$SKILL_MD" grep -q "^## When to Use" "$SKILL_MD" # Note: "Red Flags" is optional and not required for all skills }plugins/me/scripts/verify-pr-status.sh-100-116 (1)
100-116:⚠️ Potential issue | 🟡 Minor
git diff --diff-filter=Uwon't list conflicts when GitHub reports DIRTY.When the PR state is
DIRTY, GitHub is reporting remote conflicts — there's no active merge in the local working tree.git diff --name-only --diff-filter=U(line 105) only lists files with unresolved merge markers from a local merge operation, so it will produce no output here. Consider removing this command or replacing it with a more informative message (e.g., instruct the user to attempt the merge locally first).docs/TESTING.md-215-220 (1)
215-220:⚠️ Potential issue | 🟡 MinorDuplicated performance benchmarks content.
Lines 215–220 repeat the exact same bullet list that already appears under the "Performance Tests" heading at lines 188–193. This orphaned block (no heading, sits right after the Edge Cases section) looks like an accidental copy-paste artifact.
Proposed fix — remove the duplicate block
-Performance benchmarks for critical operations: - -- Plugin list caching effectiveness -- JSON parsing speed -- File operation efficiency -- Test execution timing -skills/triage-issue/SKILL.md-322-361 (1)
322-361:⚠️ Potential issue | 🟡 MinorNested fenced code blocks will break markdown rendering.
Lines 322–361 contain a markdown template with inner fenced code blocks (
```on lines 327 and 329) nested inside an outer fenced code block (```markdownon line 322). Since both use the same triple-backtick delimiter, the inner block will prematurely close the outer one, corrupting the rendered output.Use four backticks for the outer fence or indent the inner blocks.
Proposed fix (use 4-backtick outer fence)
-```markdown +````markdown ## Issue Description ... -``` +````skills/create-pr/scripts/verify-pr-status.sh-23-26 (1)
23-26:⚠️ Potential issue | 🟡 MinorNo guard for missing PR — script aborts with an opaque error.
If the current branch has no associated PR,
gh pr viewon lines 23–24 exits non-zero, andset -eterminates the script without a user-friendly message. Consider wrapping these in a guard.Proposed fix
-PR_URL=$(gh pr view --json url -q .url) -PR_STATUS=$(gh pr view --json mergeable,mergeStateStatus) +PR_URL=$(gh pr view --json url -q .url 2>/dev/null) || { + echo "ERROR: No PR found for the current branch" >&2 + echo " - Ensure you are on a branch with an open PR" >&2 + exit 1 +} +PR_STATUS=$(gh pr view --json mergeable,mergeStateStatus)skills/create-pr/scripts/verify-pr-status.sh-30-33 (1)
30-33:⚠️ Potential issue | 🟡 Minor
$CHECKSmay benullor empty, causing jq to fail.If
statusCheckRollupisnull(no checks configured) or an empty array, thejqexpressions on lines 32–33 will error onnullinput or produce unexpected results. Add a null guard.Proposed fix
CHECKS=$(gh pr view --json statusCheckRollup -q '.statusCheckRollup') + if [[ -z "$CHECKS" || "$CHECKS" == "null" ]]; then + echo "" + echo "✓ PR is merge-ready" + echo " - Status: CLEAN" + echo " - Required checks: None configured" + echo " - URL: $PR_URL" + exit 0 + fi + PENDING_REQUIRED=$(echo "$CHECKS" | jq '[.[] | select(.isRequired==true and (.state=="PENDING" or .state=="IN_PROGRESS"))] | length')skills/ci-troubleshooting/SKILL.md-72-83 (1)
72-83:⚠️ Potential issue | 🟡 MinorFenced code block not surrounded by blank lines (MD031).
Markdownlint requires a blank line before and after fenced code blocks. The opening fence on line 73 is immediately preceded by the list-item text on line 72 with no intervening blank line. As per coding guidelines, Markdown files should pass markdownlint validation.
🔧 Proposed fix
2. **Branch CI:** Push to feature branch (NOT main), verify green before merging + ```bash # Push to feature branch git push origin <branch-name>skills/ci-troubleshooting/SKILL.md-24-30 (1)
24-30:⚠️ Potential issue | 🟡 MinorFenced code block missing language specifier (MD040).
Static analysis flags this block. Add a language identifier (e.g.,
text) to satisfy markdownlint.🔧 Proposed fix
-``` +```text 1. OBSERVE: Get actual error from GitHub Actions (30 sec)scripts/ralph/ralph.sh-16-20 (1)
16-20:⚠️ Potential issue | 🟡 Minor
^[0-9]+$accepts0, contradicting the "positive integer" error message.
seq 1 0produces no output, so zero iterations would just fall through to the "max iterations reached" exit. If zero iterations should be rejected, tighten the regex:🔧 Proposed fix
- if ! [[ "$1" =~ ^[0-9]+$ ]]; then + if ! [[ "$1" =~ ^[1-9][0-9]*$ ]]; thenskills/generate-status-report/scripts/jql_builder.py-22-36 (1)
22-36:⚠️ Potential issue | 🟡 MinorDead code: double-quote escaping on line 36 is unreachable.
The regex on line 27 rejects any input containing
"(double quotes aren't in the allowed set), sosanitize_jql_valuewill always raise before reaching line 36. The.replace('"', '""')is dead code.Either add
"to the allowed set (so the escaping is meaningful) or remove the dead.replace().plugins/ralph-loop/.claude-plugin/plugin.json-5-8 (1)
5-8:⚠️ Potential issue | 🟡 MinorInconsistent author attribution.
This plugin lists
"Anthropic"/"support@anthropic.com"as author, while the other new plugins (jira, suggest-compacting, etc.) use"baleen37"/"git@baleen.me". Verify this is intentional.skills/generate-status-report/scripts/jql_builder.py-92-95 (1)
92-95:⚠️ Potential issue | 🟡 Minor
days_back=0is silently ignored due to truthiness check.
if days_back:evaluates toFalsewhendays_back=0, so passing0skips the filter silently rather than producingupdated >= -0d. If0is intentionally unsupported, the validation on line 93 should also cover it. If it should be supported, use an explicitNonecheck.Proposed fix
- if days_back: - if not isinstance(days_back, int) or days_back < 0: + if days_back is not None: + if not isinstance(days_back, int) or days_back < 0:commands/cancel-ralph.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 Minor
Bash(kill*)glob is overly permissive.The wildcard
kill*matches any executable starting with "kill" (e.g.,killall,killall5), not justkill <PID>. Consider narrowing toBash(kill *)to restrict to thekillcommand only, since the procedure only needs to kill a single PID.tests/handoff/structure.bats-64-81 (1)
64-81:⚠️ Potential issue | 🟡 Minor"session-start hook" tests don't invoke any hook — they only verify fixture creation.
Both tests (
"session-start hook detects recent handoffs"and"session-start hook ignores already loaded handoffs") create a fixture and assert on its fields, but never invoke the actual session-start hook script. The test names suggest behavioral validation that isn't happening. Either rename to reflect what's actually tested (fixture structure) or wire up the actual hook invocation.plugins/me/package.json-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorVersion mismatch:
package.jsonsays5.0.0butplugin.jsonreportedly declares5.23.3.The AI summary indicates
plugins/me/.claude-plugin/plugin.jsonhas version"5.23.3". If both files describe the same plugin, the versions should be consistent to avoid confusion in marketplace discovery or dependency resolution.#!/bin/bash # Verify the version in plugin.json cat plugins/me/.claude-plugin/plugin.json 2>/dev/null | jq '.version'tests/handoff/structure.bats-26-28 (1)
26-28:⚠️ Potential issue | 🟡 MinorTest is trivially true —
setup()already creates$HANDOFF_TEST_DIR.This test asserts that the directory exists, but
setup()on line 10 explicitlymkdir -p "$HANDOFF_TEST_DIR". It doesn't exercise any production directory-creation logic. Consider removing it or testing the actual plugin's directory-creation behavior.commands/ralph-loop.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorOverly broad allowed-tools glob:
ralph.sh*matches any file starting withralph.sh.The trailing
*onralph.sh*will matchralph.sh,ralph.sh.bak,ralph.sh.old, etc. If the intent is to allowralph.shwith arguments, the glob may need a different syntax (e.g., space before*). If onlyralph.shshould be allowed, drop the*.#!/bin/bash # Check what ralph.sh files exist fd "ralph.sh" --type f # Check how other command files define allowed-tools rg "allowed-tools" --glob "commands/*.md" -nplugins/git-guard/README.md-53-56 (1)
53-56:⚠️ Potential issue | 🟡 MinorFenced code block missing language specifier.
Add a language identifier (e.g.,
text) to satisfy MD040.Proposed fix
-``` +```text [ERROR] --no-verify is not allowed in this repository [INFO] Please use 'git commit' without --no-verify. All commits must pass quality checks.</details> </blockquote></details> <details> <summary>tests/handoff/filtering.bats-47-55 (1)</summary><blockquote> `47-55`: _⚠️ Potential issue_ | _🟡 Minor_ **Test only verifies `created_at` is non-empty — doesn't test recency detection.** The test is named "session-start hook detects recent handoffs" but the assertion (`[ -n "$created_at" ]`) would pass for *any* valid timestamp, including one from years ago. Consider asserting that the handoff falls within the expected recency window (e.g., within the last 5 minutes) or exercising the actual hook logic that performs that check. </blockquote></details> <details> <summary>plugins/jira/README.md-36-45 (1)</summary><blockquote> `36-45`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix markdownlint violations.** Several markdownlint issues were flagged by static analysis. As per coding guidelines, Markdown files should pass markdownlint validation. 1. **Lines 37–45**: Fenced code block needs a blank line before it and a language identifier (e.g., ` ```text `). 2. **Line 170**: Fenced code block needs a language identifier (e.g., ` ```text `). 3. **Lines 180, 184, 191**: Headings under "Required Permissions" and "Confluence Permissions" need a blank line before them. <details> <summary>Proposed fixes</summary> ```diff **Example:** + -``` +```text User: Triage this error - "NullPointerException in PaymentProcessor.processRefund() line 245"After authentication, verify the connection works: -``` +```text User: "What Jira projects can I access?"### Read Permissions + - **Browse Projects**: View projects and issues - **View Issues**: Read issue details, comments, and history ### Write Permissions + - **Create Issues**: Create new bugs, tasks, stories, and epics### Confluence Permissions (Optional) + - **View Pages**: Read Confluence pages for spec-to-backlog and meeting notesAlso applies to: 170-174, 179-194
tests/handoff/references.bats-26-45 (1)
26-45:⚠️ Potential issue | 🟡 MinorTilde expansion test doesn't actually exercise tilde expansion.
TEST_PLAN_FILEis built from$TEST_TEMP_DIR(an absolute/tmp/…path), so it never starts with~. The substitution${PLAN_PATH/#\~/$TEST_TEMP_DIR}is a no-op and the assertion on Line 44 is trivially true—identical to Line 40.To actually test tilde expansion, the handoff JSON should store a path beginning with
~/…and the test should verify that~gets expanded to the real home/temp directory.Proposed sketch
- # Create handoff with plan_path reference - create_handoff_json "$HANDOFF_FILE" "plan-handoff" "$NOW" "$TEST_PROJECT_PATH" "Handoff with plan reference" "main" "test-project" "$TEST_PLAN_FILE" + # Store the plan_path with a tilde prefix to simulate ~ in the handoff + TILDE_PLAN_PATH="~/.claude/plans/test-plan.md" + create_handoff_json "$HANDOFF_FILE" "plan-handoff" "$NOW" "$TEST_PROJECT_PATH" "Handoff with plan reference" "main" "test-project" "$TILDE_PLAN_PATH" # Extract plan_path from handoff PLAN_PATH=$(jq -r '.references.plan_path // empty' "$HANDOFF_FILE") - [ "$PLAN_PATH" = "$TEST_PLAN_FILE" ] + [ "$PLAN_PATH" = "$TILDE_PLAN_PATH" ] # Verify the expansion works - TEST_EXPANDED="${PLAN_PATH/#\~/$TEST_TEMP_DIR}" - [ "$TEST_EXPANDED" = "$TEST_PLAN_FILE" ] + TEST_EXPANDED="${PLAN_PATH/#\~/$HOME}" + [ -f "$TEST_EXPANDED" ] || [ "$TEST_EXPANDED" = "$HOME/.claude/plans/test-plan.md" ]skills/triage-issue/references/search-patterns.md-202-204 (1)
202-204:⚠️ Potential issue | 🟡 MinorAdd a language specifier to the fenced code block.
The fields list code block at line 202 is missing a language identifier (MD040). Use
textsince it's not executable code.Proposed fix
-``` +```text fields: ["summary", "description", "status", "resolution", "priority", "created", "updated", "resolved", "assignee", "reporter", "components"]</details> As per coding guidelines, "Markdown files should pass markdownlint validation". </blockquote></details> <details> <summary>plugins/suggest-compacting/README.md-40-43 (1)</summary><blockquote> `40-43`: _⚠️ Potential issue_ | _🟡 Minor_ **Add a language specifier to the fenced code block.** The code block showing example messages is missing a language identifier, which triggers MD040. Use `text` as the language specifier. <details> <summary>Proposed fix</summary> ```diff -``` +```text [SuggestCompacting] 50 tool calls reached - consider /compact if transitioning phases [SuggestCompacting] 75 tool calls - good checkpoint for /compact if context is staleAs per coding guidelines, "Markdown files should pass markdownlint validation".
skills/suggest-compacting/strategic-compact/SKILL.md-35-45 (1)
35-45:⚠️ Potential issue | 🟡 MinorFix markdownlint violations: missing blank lines and language specifiers around code blocks.
Lines 38 and 43 trigger MD031 (blanks-around-fences) and MD040 (fenced-code-language). Add blank lines around and a language tag to each fenced block.
Proposed fix
At threshold (default: 50): -``` + +```text [StrategicCompact] 50 tool calls reached - consider /compact if transitioning phasesEvery 25 calls after threshold:
-+ +text
[StrategicCompact] 75 tool calls - good checkpoint for /compact if context is staleAs per coding guidelines, "Markdown files should pass markdownlint validation".
skills/suggest-compacting/strategic-compact/SKILL.md-99-109 (1)
99-109:⚠️ Potential issue | 🟡 MinorSame markdownlint issues in the State Directory section.
Lines 102 and 107 trigger MD031 and MD040. Add blank lines and
textlanguage specifiers.Proposed fix
Session counters stored in: -``` + +```text ~/.claude/strategic-compact/tool-count-{session_id}.txtSession ID extracted from SessionStart hook and stored in:
-+ +text
~/.claude/strategic-compact/session-env.shAs per coding guidelines, "Markdown files should pass markdownlint validation".
skills/suggest-compacting/strategic-compact/SKILL.md-127-150 (1)
127-150:⚠️ Potential issue | 🟡 MinorAdd blank lines after headings in the Use Cases section.
Lines 127, 133, 140, and 147 trigger MD022 (blanks-around-headings). Each
###heading needs a blank line before the numbered list that follows.Proposed fix (showing one example, apply to all four)
### Long Debugging Sessions + 1. Explore symptoms and reproduce bugAs per coding guidelines, "Markdown files should pass markdownlint validation".
README.md-19-20 (1)
19-20:⚠️ Potential issue | 🟡 MinorPlugin name mismatch: README says "strategic-compact" but the actual plugin directory is "suggest-compacting".
The README references "strategic-compact" here and in the project structure (line 106-107), but every other file in this PR uses "suggest-compacting" (e.g.,
tests/suggest-compacting/,src/suggest-compacting/,plugins/suggest-compacting/). This will confuse users looking for the plugin.📝 Fix the naming to match the actual plugin
-- **strategic-compact**: Strategic content compaction and organization tools -- (automatic PreToolUse hook) +- **suggest-compacting**: Strategic content compaction and organization tools + (automatic PreToolUse hook)tests/ralph_loop_script_tests.bats-508-511 (1)
508-511:⚠️ Potential issue | 🟡 Minor
git branch -D ralph/*doesn't expand globs to branch names.Shell globbing doesn't apply to git branch names. This command will try to delete a literal branch named
ralph/*, which almost certainly doesn't exist. In the cleanup section of askipped test the impact is negligible, but if this test is ever unskipped it won't clean up properly.🐛 Proposed fix
- git branch -D ralph/* 2>/dev/null || true + git branch --list 'ralph/*' | xargs -r git branch -D 2>/dev/null || trueREADME.md-106-107 (1)
106-107:⚠️ Potential issue | 🟡 MinorSame naming mismatch in the project structure tree.
📝 Fix the naming
-│ └── strategic-compact/ # Content compaction -│ └── hooks/ # PreToolUse hook (suggests compaction) +│ └── suggest-compacting/ # Content compaction +│ └── hooks/ # PreToolUse hook (suggests compaction)tests/ralph_loop_script_tests.bats-566-570 (1)
566-570:⚠️ Potential issue | 🟡 MinorSame
git branch -D ralph/*glob issue here.🐛 Proposed fix
- git branch -D ralph/* 2>/dev/null || true + git branch --list 'ralph/*' | xargs -r git branch -D 2>/dev/null || true
| - Unit Tests: 148 tests | ||
| - Integration Tests: 20 tests (10 plugin loading + 10 cross-plugin interactions) | ||
| - Performance Tests: 10 tests | ||
| - Error Handling Tests: 23 tests | ||
| - Edge Case Tests: 17 tests | ||
| - Negative Tests: 17 tests | ||
| - **Total: 178 tests** |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Count actual BATS test cases across the repository
echo "=== Unit Tests (tests/ directory) ==="
find tests/ -name "*.bats" -type f 2>/dev/null | while read f; do
count=$(grep -c '@test' "$f" 2>/dev/null || echo 0)
echo "$f: $count tests"
done | tee /tmp/unit_tests.txt
echo ""
echo "=== Integration/Plugin Tests (plugins/ directory) ==="
find plugins/ -name "*.bats" -type f 2>/dev/null | while read f; do
count=$(grep -c '@test' "$f" 2>/dev/null || echo 0)
echo "$f: $count tests"
done | tee /tmp/plugin_tests.txt
echo ""
echo "=== Total Tests ==="
total_unit=$(awk -F': ' '{sum+=$2} END {print sum}' /tmp/unit_tests.txt)
total_plugin=$(awk -F': ' '{sum+=$2} END {print sum}' /tmp/plugin_tests.txt)
echo "Total in tests/: ${total_unit:-0}"
echo "Total in plugins/: ${total_plugin:-0}"
echo "Combined Total: $((${total_unit:-0} + ${total_plugin:-0}))"Repository: baleen37/everything-agent
Length of output: 524
🏁 Script executed:
# List all BATS test files to understand structure
echo "=== All BATS test files in repository ==="
find . -name "*.bats" -type f 2>/dev/null | sortRepository: baleen37/everything-agent
Length of output: 1102
🏁 Script executed:
# Check if docs/TESTING.md exists and view the relevant section
echo "=== Content of docs/TESTING.md (lines 40-60) ==="
if [ -f docs/TESTING.md ]; then
sed -n '40,60p' docs/TESTING.md
else
echo "docs/TESTING.md not found"
fiRepository: baleen37/everything-agent
Length of output: 580
🏁 Script executed:
# Install and run markdownlint on the markdown file
npm list -g markdownlint-cli 2>/dev/null || npm install -g markdownlint-cli > /dev/null 2>&1
if [ -f docs/TESTING.md ]; then
echo "=== Markdownlint results for docs/TESTING.md ==="
markdownlint docs/TESTING.md 2>&1 || echo "(markdownlint check completed)"
else
echo "docs/TESTING.md not found for linting"
fiRepository: baleen37/everything-agent
Length of output: 227
🏁 Script executed:
#!/bin/bash
# Count `@test` declarations in each BATS file
echo "=== Test count by file ==="
for f in $(find tests/ -name "*.bats" -type f | sort); do
count=$(grep -c '@test' "$f" 2>/dev/null || echo 0)
echo "$count tests in $f"
done
echo ""
echo "=== Summary by category ==="
echo "Unit Tests (main tests/ files):"
find tests/ -maxdepth 1 -name "*.bats" -type f | while read f; do
grep -c '@test' "$f" 2>/dev/null || echo 0
done | awk '{sum+=$1} END {print sum " total"}'
echo "Error Handling Tests (error_handling.bats):"
grep -c '@test' tests/error_handling.bats 2>/dev/null || echo 0
echo "Edge Case Tests (edge_cases.bats):"
grep -c '@test' tests/edge_cases.bats 2>/dev/null || echo 0
echo "Negative Tests (negative_tests.bats):"
grep -c '@test' tests/negative_tests.bats 2>/dev/null || echo 0
echo "Integration Tests:"
find tests/integration/ -name "*.bats" -type f 2>/dev/null | while read f; do
grep -c '@test' "$f" 2>/dev/null || echo 0
done | awk '{sum+=$1} END {print sum " total"}'
echo "Performance Tests (benchmarks.bats):"
grep -c '@test' tests/performance/benchmarks.bats 2>/dev/null || echo 0
echo ""
echo "=== Grand Total ==="
find tests/ -name "*.bats" -type f | while read f; do
grep -c '@test' "$f" 2>/dev/null || echo 0
done | awk '{sum+=$1} END {print sum " tests"}'Repository: baleen37/everything-agent
Length of output: 1658
🏁 Script executed:
#!/bin/bash
# Categorize all tests properly
echo "=== Direct unit tests in tests/ root ==="
find tests/ -maxdepth 1 -name "*.bats" -type f | while read f; do
count=$(grep -c '@test' "$f")
echo "$count - $(basename $f)"
done | awk '{sum+=$1} END {print "Subtotal: " sum}'
echo ""
echo "=== Plugin-specific tests ==="
find tests/ -mindepth 2 -name "*.bats" -type f | while read f; do
count=$(grep -c '@test' "$f")
echo "$count - $f"
done | awk '{sum+=$1} END {print "Subtotal: " sum}'
echo ""
echo "=== Breakdown by category ==="
echo "Category counts:"
echo " Error Handling: $(grep -c '@test' tests/error_handling.bats)"
echo " Edge Cases: $(grep -c '@test' tests/edge_cases.bats)"
echo " Negative Tests: $(grep -c '@test' tests/negative_tests.bats)"
echo " Performance: $(grep -c '@test' tests/performance/benchmarks.bats)"
echo " Integration plugin_loading: $(grep -c '@test' tests/integration/plugin_loading.bats)"
echo " Integration cross_plugin: $(grep -c '@test' tests/integration/cross_plugin_interactions.bats)"
echo ""
echo "Remaining unit tests (not categorized above):"
find tests/ -maxdepth 1 -name "*.bats" -type f ! -name "error_handling.bats" ! -name "edge_cases.bats" ! -name "negative_tests.bats" | while read f; do
count=$(grep -c '@test' "$f")
echo " $count - $(basename $f)"
done | awk '{sum+=$1} END {print " Subtotal: " sum}'Repository: baleen37/everything-agent
Length of output: 419
Update test counts to reflect actual test suite: The documented test counts are significantly inaccurate. Actual test counts are:
- Unit Tests: 290 (not 148)
- Integration Tests: 18 (not 20) — plugin_loading has 8 tests, cross_plugin_interactions has 10
- Performance Tests: 10 ✓
- Error Handling Tests: 23 ✓
- Edge Case Tests: 17 ✓
- Negative Tests: 17 ✓
- Total: 397 tests (not 178)
The documentation needs to be updated with accurate counts before this PR merges.
🤖 Prompt for AI Agents
In `@docs/TESTING.md` around lines 48 - 54, Update the test count table entries
for the listed test categories in TESTING.md: replace "Unit Tests: 148" with
"Unit Tests: 290", change "Integration Tests: 20 tests (10 plugin loading + 10
cross-plugin interactions)" to "Integration Tests: 18 tests (8 plugin_loading +
10 cross_plugin_interactions)", keep "Performance Tests: 10", "Error Handling
Tests: 23", "Edge Case Tests: 17", and "Negative Tests: 17" as-is, and update
the bold total from "Total: 178 tests" to "Total: 397 tests" so the documented
counts for the entries named "Unit Tests", "Integration Tests", and the bold
"Total" reflect the actual suite.
| validate_git_command() { | ||
| local command="$1" | ||
|
|
||
| # Quick exit: if not a git command, allow immediately | ||
| if ! matches_pattern "$command" "^\s*(\S*=\S*\s+)*git\s+"; then | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Test regex portability issue
echo "Testing \s and \S portability with grep -E:"
echo "git commit --no-verify" | grep -qE "git\s+commit" && echo "✓ \s works" || echo "✗ \s does NOT work"
echo "git commit --no-verify" | grep -qE "git[[:space:]]+commit" && echo "✓ [[:space:]] works" || echo "✗ [[:space:]] does NOT work"Repository: baleen37/everything-agent
Length of output: 143
🏁 Script executed:
# Read the hooks/commit-guard.sh file to verify the patterns mentioned
head -200 hooks/commit-guard.sh | tail -100Repository: baleen37/everything-agent
Length of output: 3215
🏁 Script executed:
# Find and display the matches_pattern function
grep -n "matches_pattern" hooks/commit-guard.sh | head -20Repository: baleen37/everything-agent
Length of output: 753
🏁 Script executed:
# Show the matches_pattern function definition
sed -n '1,50p' hooks/commit-guard.shRepository: baleen37/everything-agent
Length of output: 1481
🏁 Script executed:
# Check all grep calls in the file
grep -n "grep" hooks/commit-guard.shRepository: baleen37/everything-agent
Length of output: 162
\s and \S in grep -E patterns are not portable to BSD grep (macOS default).
On macOS systems where BSD grep is used, the patterns \s and \S are not recognized in Extended Regular Expressions. This causes all calls to matches_pattern to silently fail, bypassing the security checks. This affects lines 117, 122, 140, 145, 151, 156, 162, 168, and 175 in validate_git_command.
Replace \s with [[:space:]] and \S with [^[:space:]] in all patterns:
- Line 117:
^\s*(\S*=\S*\s+)*git\s+→^[[:space:]]*([^[:space:]]*=[^[:space:]]*[[:space:]]+)*git[[:space:]]+ - Line 122:
git\s+commit.*--no-verify→git[[:space:]]+commit.*--no-verify - Line 129:
git\s+commit→git[[:space:]]+commit - Line 140:
git\s+.*skip.*hooks→git[[:space:]]+.*skip.*hooks - Line 145:
git\s+.*--no-.*hook→git[[:space:]]+.*--no-.*hook - Line 162:
git\s+update-ref→git[[:space:]]+update-ref - Line 168:
git\s+filter-branch→git[[:space:]]+filter-branch - Line 175:
git\s+config.*core\.hooksPath→git[[:space:]]+config.*core\.hooksPath
🤖 Prompt for AI Agents
In `@hooks/commit-guard.sh` around lines 113 - 119, The grep -E patterns in
validate_git_command use \s and \S which are not portable to BSD grep; update
every matches_pattern call in validate_git_command to replace \s with
[[:space:]] and \S with [^[:space:]] (e.g., change "^\s*(\S*=\S*\s+)*git\s+" to
"^[[:space:]]*([^[:space:]]*=[^[:space:]]*[[:space:]]+)*git[[:space:]]+" and
similarly change "git\s+commit.*--no-verify" ->
"git[[:space:]]+commit.*--no-verify", "git\s+commit" -> "git[[:space:]]+commit",
"git\s+.*skip.*hooks" -> "git[[:space:]]+.*skip.*hooks", "git\s+.*--no-.*hook"
-> "git[[:space:]]+.*--no-.*hook", "git\s+update-ref" ->
"git[[:space:]]+update-ref", "git\s+filter-branch" ->
"git[[:space:]]+filter-branch", and "git\s+config.*core\.hooksPath" ->
"git[[:space:]]+config.*core\.hooksPath"); modify the patterns where
matches_pattern is invoked so the security checks work on macOS.
| # Check for merge conflicts using merge-tree | ||
| echo "Checking for conflicts with origin/$BASE..." | ||
| MERGE_OUTPUT=$(git merge-tree "$MERGE_BASE" HEAD "origin/$BASE" 2>&1) | ||
| MERGE_EXIT=$? | ||
|
|
||
| if [[ $MERGE_EXIT -ne 0 ]]; then | ||
| # Conflicts detected | ||
| echo -e "${YELLOW}WARNING: Conflicts detected with origin/$BASE${NC}" >&2 | ||
| echo "" >&2 | ||
|
|
||
| # Try to detect whitespace-only conflicts | ||
| DIFF_OUTPUT=$(git diff "origin/$BASE"...HEAD 2>/dev/null || echo "") | ||
|
|
||
| if echo "$DIFF_OUTPUT" | grep -q "^[-+]\s*$"; then | ||
| echo -e "${YELLOW}Note: Some conflicts may be whitespace-only${NC}" >&2 | ||
| echo " - Consider: git merge origin/$BASE --strategy-option=ignore-space-change" >&2 | ||
| fi | ||
|
|
||
| # Show conflict summary | ||
| echo -e "${RED}Conflicts require manual resolution:${NC}" >&2 | ||
| echo " 1. git fetch origin $BASE" >&2 | ||
| echo " 2. git merge origin/$BASE" >&2 | ||
| echo " 3. Resolve conflicts in affected files" >&2 | ||
| echo " 4. git add <resolved-files>" >&2 | ||
| echo " 5. git commit" >&2 | ||
| echo "" >&2 | ||
|
|
||
| # Try to extract conflicting files from merge-tree output | ||
| if echo "$MERGE_OUTPUT" | grep -q "CONFLICT"; then | ||
| echo -e "${RED}Files with conflicts:${NC}" >&2 | ||
| echo "$MERGE_OUTPUT" | grep "CONFLICT" | sed 's/^/ - /' >&2 | ||
| fi | ||
|
|
||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
git merge-tree (old three-argument form) always exits 0 — conflict detection is broken.
The script invokes the legacy three-argument form of git merge-tree (<base-tree> <branch1> <branch2>), which always returns exit code 0 regardless of conflicts. Conflicts are indicated only in the stdout output, not via the exit code. Therefore MERGE_EXIT on line 53 will always be 0, and the entire conflict-handling block (lines 55–83) is dead code.
Additionally, even if the newer git merge-tree --write-tree form were used (which does return non-zero on conflicts), set -euo pipefail on line 2 would cause the script to terminate at line 52 before MERGE_EXIT is captured.
Two options to fix:
- Use the modern form with
set -eguard:
Proposed fix using new-style merge-tree
# Check for merge conflicts using merge-tree
echo "Checking for conflicts with origin/$BASE..."
-MERGE_OUTPUT=$(git merge-tree "$MERGE_BASE" HEAD "origin/$BASE" 2>&1)
-MERGE_EXIT=$?
+MERGE_EXIT=0
+MERGE_OUTPUT=$(git merge-tree --write-tree HEAD "origin/$BASE" 2>&1) || MERGE_EXIT=$?- Or parse the old-form output for conflict markers (more portable across git versions):
Alternative — parse output for CONFLICT markers
echo "Checking for conflicts with origin/$BASE..."
-MERGE_OUTPUT=$(git merge-tree "$MERGE_BASE" HEAD "origin/$BASE" 2>&1)
-MERGE_EXIT=$?
-
-if [[ $MERGE_EXIT -ne 0 ]]; then
+MERGE_OUTPUT=$(git merge-tree "$MERGE_BASE" HEAD "origin/$BASE" 2>&1)
+
+if echo "$MERGE_OUTPUT" | grep -q "CONFLICT"; thenDoes git merge-tree three-argument form return non-zero exit code on conflicts?
🤖 Prompt for AI Agents
In `@plugins/me/scripts/check-conflicts.sh` around lines 50 - 84, The
three-argument git merge-tree call (git merge-tree "$MERGE_BASE" HEAD
"origin/$BASE") always exits 0 so MERGE_EXIT is meaningless and the
conflict-handling block never runs; also set -euo pipefail can prematurely abort
if you switch to the newer --write-tree form. Fix by either (A) keeping the
legacy form and detect conflicts by parsing MERGE_OUTPUT for "CONFLICT" (use the
existing MERGE_OUTPUT variable and conditional like if echo "$MERGE_OUTPUT" |
grep -q "CONFLICT"; then ... fi) or (B) switch to the modern git merge-tree
--write-tree form and temporarily disable exit-on-error around that call
(save/restore errexit with set +e / set -e or run the command in a subshell) so
you can capture its non-zero exit in MERGE_EXIT; update the subsequent checks to
use the chosen detection (MERGE_OUTPUT contains markers or MERGE_EXIT) and
remove the dead-code assumptions.
| git merge origin/"$BASE" --no-edit | ||
|
|
||
| if [[ $? -ne 0 ]]; then | ||
| echo "" | ||
| echo "✗ Merge failed - conflicts detected" | ||
| echo "" | ||
| echo "Files with conflicts:" | ||
| git diff --name-only --diff-filter=U | sed 's/^/ - /' | ||
| echo "" | ||
| echo "Resolution steps:" | ||
| echo " 1. Resolve conflicts in listed files" | ||
| echo " 2. git add <files>" | ||
| echo " 3. git commit" | ||
| echo " 4. git push" | ||
| echo " 5. Re-run this workflow" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
set -e makes the merge-failure handling dead code.
With set -euo pipefail active, when git merge origin/"$BASE" --no-edit (line 73) fails, the script terminates immediately — it never reaches the if [[ $? -ne 0 ]] check on line 75. The entire conflict-handling block (lines 76–89) is unreachable.
Proposed fix
# Update branch
- git merge origin/"$BASE" --no-edit
-
- if [[ $? -ne 0 ]]; then
+ if ! git merge origin/"$BASE" --no-edit; then
echo ""
echo "✗ Merge failed - conflicts detected"📝 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.
| git merge origin/"$BASE" --no-edit | |
| if [[ $? -ne 0 ]]; then | |
| echo "" | |
| echo "✗ Merge failed - conflicts detected" | |
| echo "" | |
| echo "Files with conflicts:" | |
| git diff --name-only --diff-filter=U | sed 's/^/ - /' | |
| echo "" | |
| echo "Resolution steps:" | |
| echo " 1. Resolve conflicts in listed files" | |
| echo " 2. git add <files>" | |
| echo " 3. git commit" | |
| echo " 4. git push" | |
| echo " 5. Re-run this workflow" | |
| exit 1 | |
| fi | |
| if ! git merge origin/"$BASE" --no-edit; then | |
| echo "" | |
| echo "✗ Merge failed - conflicts detected" | |
| echo "" | |
| echo "Files with conflicts:" | |
| git diff --name-only --diff-filter=U | sed 's/^/ - /' | |
| echo "" | |
| echo "Resolution steps:" | |
| echo " 1. Resolve conflicts in listed files" | |
| echo " 2. git add <files>" | |
| echo " 3. git commit" | |
| echo " 4. git push" | |
| echo " 5. Re-run this workflow" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In `@plugins/me/scripts/verify-pr-status.sh` around lines 73 - 89, The script uses
set -euo pipefail so the git merge command will abort the script before the
existing if [[ $? -ne 0 ]] block runs; change the merge invocation to handle
failures explicitly — either wrap the merge with a controlled failure handler
(e.g. run git merge origin/"$BASE" --no-edit || { <conflict-handling-echoes>;
exit 1; }) or temporarily disable errexit around the merge (set +e; git merge
origin/"$BASE" --no-edit; ret=$?; set -e; if [[ $ret -ne 0 ]]; then
<conflict-handling-echoes>; exit 1; fi), and remove or replace the unreachable
if [[ $? -ne 0 ]] branch accordingly so the conflict messages are emitted when
the merge fails.
| if ! OUTPUT=$(echo "$PROMPT" | claude --print --dangerously-skip-permissions 2>&1 | tee -a "$ITERATION_LOG"); then | ||
| exit_code=$? | ||
| iteration_end=$(date -u +"%Y-%m-%dT%H:%M:%SZ") | ||
| echo "Warning: Claude command failed with exit code $exit_code" >&2 | ||
| echo "Check log: $ITERATION_LOG" >&2 | ||
| # Log error to errors.log | ||
| echo "${iteration_end} - Iteration $i failed with exit code ${exit_code}" >> "$ERRORS_LOG" | ||
| # Continue on transient errors (exit code 1), abort on fatal errors (> 1) | ||
| if [[ $exit_code -gt 1 ]]; then | ||
| # Log activity before exiting | ||
| echo "${iteration_end} - Iteration $i: START=${iteration_start}, END=${iteration_end}, STATUS=FATAL_ERROR" >> "$ACTIVITY_LOG" | ||
| exit $exit_code | ||
| fi |
There was a problem hiding this comment.
exit_code=$? is always 0 inside the then block — fatal errors are never detected.
After if ! OUTPUT=$(...); then, $? reflects the exit status of the negated test expression (which is 0 because the condition was true). The actual non-zero exit code from claude is lost. Consequently, the [[ $exit_code -gt 1 ]] guard on line 321 never triggers, and fatal errors silently continue the loop instead of aborting.
Capture the exit code before the if:
🐛 Proposed fix
- if ! OUTPUT=$(echo "$PROMPT" | claude --print --dangerously-skip-permissions 2>&1 | tee -a "$ITERATION_LOG"); then
- exit_code=$?
+ OUTPUT=$(echo "$PROMPT" | claude --print --dangerously-skip-permissions 2>&1 | tee -a "$ITERATION_LOG") || true
+ exit_code=$?
+ if [[ $exit_code -ne 0 ]]; then
iteration_end=$(date -u +"%Y-%m-%dT%H:%M:%SZ")
echo "Warning: Claude command failed with exit code $exit_code" >&2However, with set -e active, using || true disables the errexit trap for that line. An alternative is to temporarily capture the code:
🐛 Alternative fix using explicit capture
+ local exit_code=0
+ OUTPUT=$(echo "$PROMPT" | claude --print --dangerously-skip-permissions 2>&1 | tee -a "$ITERATION_LOG") || exit_code=$?
- if ! OUTPUT=$(echo "$PROMPT" | claude --print --dangerously-skip-permissions 2>&1 | tee -a "$ITERATION_LOG"); then
- exit_code=$?
+ if [[ $exit_code -ne 0 ]]; then
iteration_end=$(date -u +"%Y-%m-%dT%H:%M:%SZ")📝 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.
| if ! OUTPUT=$(echo "$PROMPT" | claude --print --dangerously-skip-permissions 2>&1 | tee -a "$ITERATION_LOG"); then | |
| exit_code=$? | |
| iteration_end=$(date -u +"%Y-%m-%dT%H:%M:%SZ") | |
| echo "Warning: Claude command failed with exit code $exit_code" >&2 | |
| echo "Check log: $ITERATION_LOG" >&2 | |
| # Log error to errors.log | |
| echo "${iteration_end} - Iteration $i failed with exit code ${exit_code}" >> "$ERRORS_LOG" | |
| # Continue on transient errors (exit code 1), abort on fatal errors (> 1) | |
| if [[ $exit_code -gt 1 ]]; then | |
| # Log activity before exiting | |
| echo "${iteration_end} - Iteration $i: START=${iteration_start}, END=${iteration_end}, STATUS=FATAL_ERROR" >> "$ACTIVITY_LOG" | |
| exit $exit_code | |
| fi | |
| OUTPUT=$(echo "$PROMPT" | claude --print --dangerously-skip-permissions 2>&1 | tee -a "$ITERATION_LOG") || true | |
| exit_code=$? | |
| if [[ $exit_code -ne 0 ]]; then | |
| iteration_end=$(date -u +"%Y-%m-%dT%H:%M:%SZ") | |
| echo "Warning: Claude command failed with exit code $exit_code" >&2 | |
| echo "Check log: $ITERATION_LOG" >&2 | |
| # Log error to errors.log | |
| echo "${iteration_end} - Iteration $i failed with exit code ${exit_code}" >> "$ERRORS_LOG" | |
| # Continue on transient errors (exit code 1), abort on fatal errors (> 1) | |
| if [[ $exit_code -gt 1 ]]; then | |
| # Log activity before exiting | |
| echo "${iteration_end} - Iteration $i: START=${iteration_start}, END=${iteration_end}, STATUS=FATAL_ERROR" >> "$ACTIVITY_LOG" | |
| exit $exit_code | |
| fi | |
| fi |
🤖 Prompt for AI Agents
In `@scripts/ralph/ralph.sh` around lines 313 - 325, The current pattern loses
Claude's real exit code because exit_code=$? is evaluated inside the then-block
of the negated if; change the flow to run the claude command and capture its
exit status immediately into exit_code before branching: execute the command
capturing stdout into OUTPUT and appending to "$ITERATION_LOG" while preserving
the exit status (e.g. OUTPUT=$(echo "$PROMPT" | claude --print
--dangerously-skip-permissions 2>&1 | tee -a "$ITERATION_LOG") || exit_code=$?;
exit_code=${exit_code:-0}), then check if exit_code is non-zero to log to
"$ERRORS_LOG" and inspect [[ $exit_code -gt 1 ]] to write the fatal ACTIVITY_LOG
entry and exit; if set -e is enabled, use the "|| exit_code=$?" suffix (or wrap
the command in a subshell) so errexit doesn't abort the script before you
capture exit_code.
| if order_by: | ||
| # Validate order_by contains only safe keywords | ||
| order_by = sanitize_jql_value(order_by) | ||
| query += f' ORDER BY {order_by}' |
There was a problem hiding this comment.
Critical: sanitize_jql_value will reject the default order_by value.
The default order_by is "priority DESC, updated DESC", which contains a comma. sanitize_jql_value only allows [a-zA-Z0-9\s\-_.@], so the comma will trigger a ValueError on every call that uses the default (or any multi-field) ORDER BY.
Proposed fix
Either add comma to the allowed characters in sanitize_jql_value, or use a dedicated sanitizer for the order_by clause:
- # Validate order_by contains only safe keywords
- order_by = sanitize_jql_value(order_by)
+ # Validate order_by contains only safe keywords
+ safe_order_pattern = re.compile(r'^[a-zA-Z0-9\s,]+$')
+ if not safe_order_pattern.match(order_by):
+ raise ValueError(f"Invalid characters in order_by: '{order_by}'")📝 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.
| if order_by: | |
| # Validate order_by contains only safe keywords | |
| order_by = sanitize_jql_value(order_by) | |
| query += f' ORDER BY {order_by}' | |
| if order_by: | |
| # Validate order_by contains only safe keywords | |
| safe_order_pattern = re.compile(r'^[a-zA-Z0-9\s,]+$') | |
| if not safe_order_pattern.match(order_by): | |
| raise ValueError(f"Invalid characters in order_by: '{order_by}'") | |
| query += f' ORDER BY {order_by}' |
🤖 Prompt for AI Agents
In `@skills/generate-status-report/scripts/jql_builder.py` around lines 106 - 109,
The ORDER BY branch uses sanitize_jql_value(order_by) which rejects commas so
default "priority DESC, updated DESC" raises ValueError; fix by either extending
sanitize_jql_value to permit commas and commas+space (e.g., add ',' to the
allowed character set) or implement a dedicated sanitizer (e.g.,
sanitize_jql_order_by) that allows comma-separated field directions and
validates each token only contains allowed field names and "ASC"/"DESC"; update
the ORDER BY branch to call the chosen sanitizer before appending f' ORDER BY
{order_by}' so multi-field order clauses pass validation.
| "outDir": "./dist", | ||
| "rootDir": "./src", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check directory structure of src/suggest-compacting/
echo "=== Directory listing of src/suggest-compacting/ ==="
ls -la src/suggest-compacting/ 2>/dev/null | head -20
echo ""
echo "=== All TypeScript files in src/suggest-compacting/ and subdirs ==="
find src/suggest-compacting -name '*.ts' -type f 2>/dev/null
echo ""
echo "=== Check if nested src/ directory exists ==="
ls -la src/suggest-compacting/src/ 2>/dev/null || echo "No src/ subdirectory found"
echo ""
echo "=== Display tsconfig.json content ==="
cat -n src/suggest-compacting/tsconfig.json 2>/dev/null
echo ""
echo "=== Check if there's a top-level tsconfig.json ==="
cat -n tsconfig.json 2>/dev/null | head -30Repository: baleen37/everything-agent
Length of output: 1366
rootDir and include patterns do not match actual source file locations.
The tsconfig at src/suggest-compacting/tsconfig.json specifies rootDir: "./src" and include: ["src/**/*"], which would expect TypeScript source files in src/suggest-compacting/src/**/*. However, the actual source files are in src/suggest-compacting/ directly:
auto-compact.tssession-start.tslib/state.ts
No nested src/ subdirectory exists. With this configuration, no files would be compiled. Change rootDir to "./" and include to ["**/*"], or move source files to a nested src/ subdirectory.
🤖 Prompt for AI Agents
In `@src/suggest-compacting/tsconfig.json` around lines 6 - 7, The tsconfig in
this package points rootDir at "./src" and includes "src/**/*" which doesn't
match actual source files (auto-compact.ts, session-start.ts, lib/state.ts) in
the package root; update the tsconfig.json to set "rootDir" to "./" and change
"include" to ["**/*"] (or alternatively move the TypeScript files into a nested
src/ directory) so the compiler actually finds and emits these files.
Restore the consolidated plugin structure by undoing PR #501. This brings back the state before the me/ralph-loop/suggest-compacting plugins were restored.
1 participant
Summary
Rollback commits after
86f75f1to restore:plugins/me/plugins/suggest-compacting/plugins/ralph-loop/scripts/ralph/skills/suggest-compacting/src/suggest-compacting/commands/ralph-*.mdagents/ralph/config.sh.exampleThis reverts the plugin consolidation changes while preserving history.
Test plan
plugins/me/exists and is functionalplugins/ralph-loop/existsplugins/suggest-compacting/existsSummary by CodeRabbit
New Features
Bug Fixes & Improvements