refactor(ralph-loop): sync with official ralph-wiggum plugin - #446
refactor(ralph-loop): sync with official ralph-wiggum plugin#446baleen37 wants to merge 2 commits into
Conversation
Revert to simpler single-session architecture matching official: - Remove SessionStart hook and session-start-hook.sh - Remove project UUID system and lib/state.sh - Use simple .claude/ralph-loop.local.md state file - Update hooks.json to match official structure - Consolidate cancel-ralph logic into command file Keep local differences: - Plugin name: ralph-loop (branding preference) - Version: 5.7.2 (continuing local versioning) - Safety: empty array handling in setup script - Fix markdownlint issues (line length, code blocks) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe Ralph Loop plugin is comprehensively refactored, consolidating state management from a multi-session, per-session directory architecture to a single local Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (CLI)
participant Setup as setup-ralph-loop.sh
participant FS as FileSystem (.claude/ralph-loop.local.md)
participant Stop as stop-hook.sh
User->>Setup: run /ralph-loop (with flags)
Setup->>FS: create/write `.claude/ralph-loop.local.md` (frontmatter: active, iteration, max_iterations, completion_promise, started_at)
Note right of FS: Single fixed per-repo state file
User->>Stop: trigger Stop hook (e.g., stop event)
Stop->>FS: read `.claude/ralph-loop.local.md`
Stop->>FS: update iteration / optionally delete file
Stop-->>User: emit JSON decision block / status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 3
🤖 Fix all issues with AI agents
In `@plugins/ralph-loop/hooks/stop-hook.sh`:
- Around line 56-67: The jq call that sets TRANSCRIPT_PATH can fail if
HOOK_INPUT is empty/malformed, which would abort the script under set -e and
skip the cleanup; wrap or guard the jq invocation so it cannot cause a non-zero
exit (e.g., capture jq stderr and use a fallback/quiet failure or temporarily
disable set -e around the command) and then perform a robust check (if [[ -z
"$TRANSCRIPT_PATH" || ! -f "$TRANSCRIPT_PATH" ]]) to run the same cleanup (rm
"$RALPH_STATE_FILE" and exit 0) when jq fails or returns an empty path; update
the code around the TRANSCRIPT_PATH assignment and the subsequent file-existence
branch to ensure cleanup always runs even if jq fails.
- Around line 90-105: The jq failure check is unreachable under set -e because
command substitution will abort the script; change the flow to run jq in a
conditional so its non-zero exit is handled (e.g. use if ! LAST_OUTPUT=$(echo
"$LAST_LINE" | jq -r '...'' 2>&1); then ... fi). Specifically, replace the
current assignment + subsequent "$?" check with an if-not wrapper that captures
LAST_OUTPUT and, on failure, writes the same stderr messages, removes
RALPH_STATE_FILE and exits 0; ensure you refer to LAST_LINE, LAST_OUTPUT, jq,
and RALPH_STATE_FILE when making the change.
In `@plugins/ralph-loop/scripts/setup-ralph-loop.sh`:
- Around line 137-142: The current YAML quoting of COMPLETION_PROMISE can
produce malformed YAML when the value contains quotes or backslashes; update the
block that sets COMPLETION_PROMISE_YAML to validate and reject problematic
characters instead of blindly quoting: in the script around the
COMPLETION_PROMISE / COMPLETION_PROMISE_YAML logic, add a check on
COMPLETION_PROMISE for characters like double-quote (") and backslash (\) (and
optionally newlines) and if present print a clear error and exit non-zero,
otherwise safely wrap the value in double quotes as you do now; reference
COMPLETION_PROMISE and COMPLETION_PROMISE_YAML in your change so the guard runs
before assigning COMPLETION_PROMISE_YAML.
🧹 Nitpick comments (1)
plugins/ralph-loop/scripts/setup-ralph-loop.sh (1)
144-154: Theactive: truefrontmatter field is written but never read.The stop hook (lines 15–17) checks file existence to determine if a loop is active — it never reads the
activefield. This is dead metadata. Not harmful, but worth noting for future cleanup or to start using it (e.g., for pausing without deleting the file).
|
|
||
| # Get transcript path from hook input | ||
| TRANSCRIPT_PATH=$(echo "$HOOK_INPUT" | jq -r '.transcript_path') | ||
|
|
||
| if [[ ! -f "$TRANSCRIPT_PATH" ]]; then | ||
| echo "⚠️ Ralph loop: Transcript file not found" >&2 | ||
| echo " Expected: $TRANSCRIPT_PATH" >&2 | ||
| echo " This is unusual and may indicate a Claude Code internal issue." >&2 | ||
| echo " Ralph loop is stopping." >&2 | ||
| rm "$STATE_FILE" | ||
| exit 0 | ||
| echo "⚠️ Ralph loop: Transcript file not found" >&2 | ||
| echo " Expected: $TRANSCRIPT_PATH" >&2 | ||
| echo " This is unusual and may indicate a Claude Code internal issue." >&2 | ||
| echo " Ralph loop is stopping." >&2 | ||
| rm "$RALPH_STATE_FILE" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Same set -e risk applies to the jq call on line 58.
If HOOK_INPUT is empty or malformed, jq -r '.transcript_path' will fail, and the script will abort without running the cleanup on lines 61–66. Though the risk is lower (Claude provides the hook input), defensive handling is still warranted for the same reason as above.
Proposed fix
-TRANSCRIPT_PATH=$(echo "$HOOK_INPUT" | jq -r '.transcript_path')
+TRANSCRIPT_PATH=$(echo "$HOOK_INPUT" | jq -r '.transcript_path' 2>/dev/null) || TRANSCRIPT_PATH=""📝 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.
| # Get transcript path from hook input | |
| TRANSCRIPT_PATH=$(echo "$HOOK_INPUT" | jq -r '.transcript_path') | |
| if [[ ! -f "$TRANSCRIPT_PATH" ]]; then | |
| echo "⚠️ Ralph loop: Transcript file not found" >&2 | |
| echo " Expected: $TRANSCRIPT_PATH" >&2 | |
| echo " This is unusual and may indicate a Claude Code internal issue." >&2 | |
| echo " Ralph loop is stopping." >&2 | |
| rm "$STATE_FILE" | |
| exit 0 | |
| echo "⚠️ Ralph loop: Transcript file not found" >&2 | |
| echo " Expected: $TRANSCRIPT_PATH" >&2 | |
| echo " This is unusual and may indicate a Claude Code internal issue." >&2 | |
| echo " Ralph loop is stopping." >&2 | |
| rm "$RALPH_STATE_FILE" | |
| exit 0 | |
| fi | |
| # Get transcript path from hook input | |
| TRANSCRIPT_PATH=$(echo "$HOOK_INPUT" | jq -r '.transcript_path' 2>/dev/null) || TRANSCRIPT_PATH="" | |
| if [[ ! -f "$TRANSCRIPT_PATH" ]]; then | |
| echo "⚠️ Ralph loop: Transcript file not found" >&2 | |
| echo " Expected: $TRANSCRIPT_PATH" >&2 | |
| echo " This is unusual and may indicate a Claude Code internal issue." >&2 | |
| echo " Ralph loop is stopping." >&2 | |
| rm "$RALPH_STATE_FILE" | |
| exit 0 | |
| fi |
🤖 Prompt for AI Agents
In `@plugins/ralph-loop/hooks/stop-hook.sh` around lines 56 - 67, The jq call that
sets TRANSCRIPT_PATH can fail if HOOK_INPUT is empty/malformed, which would
abort the script under set -e and skip the cleanup; wrap or guard the jq
invocation so it cannot cause a non-zero exit (e.g., capture jq stderr and use a
fallback/quiet failure or temporarily disable set -e around the command) and
then perform a robust check (if [[ -z "$TRANSCRIPT_PATH" || ! -f
"$TRANSCRIPT_PATH" ]]) to run the same cleanup (rm "$RALPH_STATE_FILE" and exit
0) when jq fails or returns an empty path; update the code around the
TRANSCRIPT_PATH assignment and the subsequent file-existence branch to ensure
cleanup always runs even if jq fails.
| LAST_OUTPUT=$(echo "$LAST_LINE" | jq -r ' | ||
| .message.content | | ||
| map(select(.type == "text")) | | ||
| map(.text) | | ||
| join("\n") | ||
| ' 2>/dev/null) | ||
| .message.content | | ||
| map(select(.type == "text")) | | ||
| map(.text) | | ||
| join("\n") | ||
| ' 2>&1) | ||
|
|
||
| # Check if jq succeeded | ||
| if ! echo "$LAST_LINE" | jq -r '.message.content' >/dev/null 2>&1; then | ||
| echo "⚠️ Ralph loop: Failed to parse assistant message JSON" >&2 | ||
| echo " This may indicate a transcript format issue" >&2 | ||
| echo " Ralph loop is stopping." >&2 | ||
| rm "$STATE_FILE" | ||
| exit 0 | ||
| if [[ $? -ne 0 ]]; then | ||
| echo "⚠️ Ralph loop: Failed to parse assistant message JSON" >&2 | ||
| echo " Error: $LAST_OUTPUT" >&2 | ||
| echo " This may indicate a transcript format issue" >&2 | ||
| echo " Ralph loop is stopping." >&2 | ||
| rm "$RALPH_STATE_FILE" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
$? check is unreachable under set -e — jq failure will abort the script.
With set -euo pipefail active, if jq returns a non-zero exit code, the assignment LAST_OUTPUT=$(...) on line 90 causes the script to exit immediately. The graceful error handling on lines 98–105 is never reached.
This means a malformed transcript entry will crash the hook ungracefully instead of cleaning up the state file and allowing exit.
Proposed fix: capture exit code properly
-LAST_OUTPUT=$(echo "$LAST_LINE" | jq -r '
+LAST_OUTPUT=$(echo "$LAST_LINE" | jq -r '
.message.content |
map(select(.type == "text")) |
map(.text) |
join("\n")
-' 2>&1)
-
-# Check if jq succeeded
-if [[ $? -ne 0 ]]; then
+' 2>/dev/null) || {
echo "⚠️ Ralph loop: Failed to parse assistant message JSON" >&2
- echo " Error: $LAST_OUTPUT" >&2
echo " This may indicate a transcript format issue" >&2
echo " Ralph loop is stopping." >&2
rm "$RALPH_STATE_FILE"
exit 0
-fi
+}As per coding guidelines, hook scripts must use stderr for error messages — the error output is correct, but the control flow around $? defeats the purpose of the error handling.
📝 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.
| LAST_OUTPUT=$(echo "$LAST_LINE" | jq -r ' | |
| .message.content | | |
| map(select(.type == "text")) | | |
| map(.text) | | |
| join("\n") | |
| ' 2>/dev/null) | |
| .message.content | | |
| map(select(.type == "text")) | | |
| map(.text) | | |
| join("\n") | |
| ' 2>&1) | |
| # Check if jq succeeded | |
| if ! echo "$LAST_LINE" | jq -r '.message.content' >/dev/null 2>&1; then | |
| echo "⚠️ Ralph loop: Failed to parse assistant message JSON" >&2 | |
| echo " This may indicate a transcript format issue" >&2 | |
| echo " Ralph loop is stopping." >&2 | |
| rm "$STATE_FILE" | |
| exit 0 | |
| if [[ $? -ne 0 ]]; then | |
| echo "⚠️ Ralph loop: Failed to parse assistant message JSON" >&2 | |
| echo " Error: $LAST_OUTPUT" >&2 | |
| echo " This may indicate a transcript format issue" >&2 | |
| echo " Ralph loop is stopping." >&2 | |
| rm "$RALPH_STATE_FILE" | |
| exit 0 | |
| fi | |
| LAST_OUTPUT=$(echo "$LAST_LINE" | jq -r ' | |
| .message.content | | |
| map(select(.type == "text")) | | |
| map(.text) | | |
| join("\n") | |
| ' 2>/dev/null) || { | |
| echo "⚠️ Ralph loop: Failed to parse assistant message JSON" >&2 | |
| echo " This may indicate a transcript format issue" >&2 | |
| echo " Ralph loop is stopping." >&2 | |
| rm "$RALPH_STATE_FILE" | |
| exit 0 | |
| } |
🤖 Prompt for AI Agents
In `@plugins/ralph-loop/hooks/stop-hook.sh` around lines 90 - 105, The jq failure
check is unreachable under set -e because command substitution will abort the
script; change the flow to run jq in a conditional so its non-zero exit is
handled (e.g. use if ! LAST_OUTPUT=$(echo "$LAST_LINE" | jq -r '...'' 2>&1);
then ... fi). Specifically, replace the current assignment + subsequent "$?"
check with an if-not wrapper that captures LAST_OUTPUT and, on failure, writes
the same stderr messages, removes RALPH_STATE_FILE and exits 0; ensure you refer
to LAST_LINE, LAST_OUTPUT, jq, and RALPH_STATE_FILE when making the change.
| # Quote completion promise for YAML if it contains special chars or is not null | ||
| if [[ -n "$COMPLETION_PROMISE" ]] && [[ "$COMPLETION_PROMISE" != "null" ]]; then | ||
| COMPLETION_PROMISE_YAML="\"$COMPLETION_PROMISE\"" | ||
| COMPLETION_PROMISE_YAML="\"$COMPLETION_PROMISE\"" | ||
| else | ||
| COMPLETION_PROMISE_YAML="null" | ||
| COMPLETION_PROMISE_YAML="null" | ||
| fi |
There was a problem hiding this comment.
YAML quoting is fragile for special characters in COMPLETION_PROMISE.
If the user supplies a promise containing double quotes, backslashes, or other YAML-special characters (e.g., --completion-promise 'say "done"'), the resulting YAML will be malformed — completion_promise: "say "done"" — and the stop hook's sed/grep parsing will extract the wrong value or fail silently.
Consider escaping or single-quoting the value, or rejecting promises with problematic characters.
Proposed minimal fix: reject quotes in the promise
--completion-promise)
if [[ -z "${2:-}" ]]; then
...
fi
+ if [[ "$2" == *'"'* ]] || [[ "$2" == *'\'* ]]; then
+ echo "❌ Error: --completion-promise must not contain double quotes or backslashes" >&2
+ exit 1
+ fi
COMPLETION_PROMISE="$2"🤖 Prompt for AI Agents
In `@plugins/ralph-loop/scripts/setup-ralph-loop.sh` around lines 137 - 142, The
current YAML quoting of COMPLETION_PROMISE can produce malformed YAML when the
value contains quotes or backslashes; update the block that sets
COMPLETION_PROMISE_YAML to validate and reject problematic characters instead of
blindly quoting: in the script around the COMPLETION_PROMISE /
COMPLETION_PROMISE_YAML logic, add a check on COMPLETION_PROMISE for characters
like double-quote (") and backslash (\) (and optionally newlines) and if present
print a clear error and exit non-zero, otherwise safely wrap the value in double
quotes as you do now; reference COMPLETION_PROMISE and COMPLETION_PROMISE_YAML
in your change so the guard runs before assigning COMPLETION_PROMISE_YAML.
- Create test utilities layer (tests/helpers/test_utils.bash) - Add plugin discovery functions: find_all_plugins, get_plugin_manifest - Add validation wrappers: assert_valid_plugin, assert_valid_skill - Add file iteration helpers: for_each_skill_file, for_each_command_file - Add comprehensive validation: validate_plugin_manifest_comprehensive - Standardize fixture creation (tests/helpers/fixture_factory.bash) - Add create_marketplace_json, create_hooks_json, create_skill_md - Add create_plugin_with_custom_fields for custom JSON fields - Refactor negative_tests, edge_cases, validate_plugin_manifest - Split parser.test.ts into 5 focused test files - parser-basic.test.ts (basic parsing, 11 tests) - parser-exclusion.test.ts (exclusion markers, 8 tests) - parser-metadata.test.ts (metadata extraction, 5 tests) - parser-tool-calls.test.ts (tool call handling, 8 tests) - parser-edge-cases.test.ts (edge cases, 9 tests) - Create common plugin validation module - Add validate_plugin_manifest_comprehensive for complete validation - Add check_all_plugin_manifests, count_valid_plugins, get_invalid_plugins - Create tests/plugin_validation_common.bats with reusable patterns - Standardize assertion patterns (tests/helpers/bats_helper.bash) - Add assert_json_field, assert_json_field_type - Add assert_output_contains, assert_output_matches - Add assert_exit_code for consistent exit code checking - Fix missing matcher field in ralph-loop/hooks/hooks.json Results: All 162 tests passing, ~30-40% reduction in duplication Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/TESTING.md (1)
302-310:⚠️ Potential issue | 🟡 MinorExample uses old
loadpath.Line 304 says
Source the helper: load bats_helper.bashbut the actual convention used throughout the codebase (and shown correctly in line 310) isload helpers/bats_helper. The instruction text on line 304 should match.
🤖 Fix all issues with AI agents
In @.claude/ralph-loop.local.md:
- Around line 1-7: Add .claude/ralph-loop.local.md and
.claude/settings.local.json (or the patterns "*.local.md" and
"settings.local.json" under the .claude directory) to .gitignore, then remove
the tracked copies from Git index (git rm --cached) and commit the change so
those ephemeral runtime files are no longer committed; update .gitignore to
include the new patterns and ensure you stage and commit the removal of
.claude/ralph-loop.local.md and .claude/settings.local.json from the repository.
In `@docs/TESTING.md`:
- Around line 46-54: The "### Test Count" block lists category counts that sum
to 235 but shows a total of 178; update that block under the heading "### Test
Count" so the total is correct (change **Total: 178 tests** to **Total: 235
tests**) or explicitly indicate which categories are subsets of Unit Tests
(e.g., annotate "Error Handling Tests", "Edge Case Tests", "Negative Tests" as
subsets) and then adjust the displayed totals accordingly to remove
double-counting.
- Around line 265-272: Update the Fixture Factory section to reflect the new
signatures: change the `create_minimal_plugin` and `create_full_plugin` entries
to accept two optional author parameters (`[author_name] [author_email]`)
instead of a single `[author]`; keep the other function signatures as-is and
ensure the descriptions note that author name and email are separate optional
arguments so callers pass both when available.
- Around line 215-220: Remove the duplicated "Performance benchmarks for
critical operations" bullet list that was copy-pasted under the
"edge_cases.bats" section; specifically locate the block that starts with
"Performance benchmarks for critical operations:" and the bullets ("Plugin list
caching effectiveness", "JSON parsing speed", "File operation efficiency", "Test
execution timing") in the edge_cases.bats section and delete that redundant
block so only the original list (the one in Lines 188–193) remains.
In `@plugins/conversation-memory/src/core/parser-basic.test.ts`:
- Around line 160-172: The test case name and assertions are misleading because
parseConversationFile(tempFilePath) uses an absolute tempFilePath which will
always have ≥2 path segments and thus never hit the "unknown" fallback; update
the test by either renaming it to reflect that it only asserts project is a
defined string, or modify the input to actually exercise the fallback by passing
a path with fewer than 2 segments (e.g., just 'test-conversation.jsonl' or a
bare filename) to parseConversationFile and then assert result.project ===
'unknown'; reference the existing test function and
tempFilePath/parseConversationFile identifiers when making the change.
In `@plugins/conversation-memory/src/core/parser-tool-calls.test.ts`:
- Around line 201-203: Update the stale test comment in
plugins/conversation-memory/src/core/parser-tool-calls.test.ts to reflect that
this is a regression test for a previously-fixed parser bug: rephrase the line
referencing "This is the bug: parser requires assistantMessages.length > 0" to
state that the parser bug was fixed (see parser.ts check for toolCalls.length >
0) and that the test ensures conversations with only tool_use blocks (no
assistantMessages) still produce an exchange; reference symbols
assistantMessages and toolCalls and the parser.ts fix in the comment for
clarity.
In `@tests/edge_cases.bats`:
- Around line 139-146: The test passes coincidentally because create_skill_md in
fixture_factory.bash injects a literal "\n" instead of an actual newline,
producing malformed frontmatter with name and description on the same line; fix
create_skill_md to emit a real newline between frontmatter fields (use
printf/heredoc or proper newline expansion rather than a backslash-n literal) so
files generated by create_skill_md produce valid YAML frontmatter, and then
update tests (e.g., this case in tests/edge_cases.bats) to also assert the
description field via has_frontmatter_field to prevent regressions.
In `@tests/helpers/fixture_factory.bash`:
- Around line 330-346: The frontmatter string uses literal "\n" inside double
quotes so newlines are not being produced; update the construction of the
frontmatter variable (the local variable frontmatter and the conditional
appending of additional_frontmatter) to use an actual newline (for example via
$'\n' quoting or printf) so that name and description are on separate lines in
the generated SKILL.md; ensure the code that writes SKILL.md still uses the
frontmatter variable unchanged and that additional_frontmatter is appended with
the same newline semantics.
In `@tests/integration/plugin_loading.bats`:
- Around line 133-137: The inline Python call uses
yaml.safe_load(open('$workflow')) which breaks and allows shell-injection when
$workflow contains quotes; change the invocation to pass the path as an argument
(use python3 -c with sys.argv[1]) so the script reads the file from argv instead
of interpolating $workflow into the command string, keeping the surrounding
logic (the elif branch that checks python3 and the invalid_count increment)
unchanged.
In `@tests/negative_tests.bats`:
- Around line 170-179: The test "rejects plugin.json with wrong field type"
currently only creates a fixture with create_plugin_with_custom_fields and
asserts the manifest's keywords is a string via assert_json_field_type; change
it to actually run the type-check validator and assert it fails: after building
the manifest (using create_plugin_with_custom_fields and manifest path), invoke
the validator command/function used elsewhere in tests (e.g., run validate_json
or the CLI validator) against "$manifest" and assert a non-zero exit (e.g., use
run .. and assert_failure or equivalent) and optionally assert the validator
output mentions "keywords" or "type"; keep the existing helpers
validate_json/assert_json_field_type for verifying fixture setup but add the
run+failure assertion to ensure the validator rejects the wrong-type "keywords".
- Around line 128-136: The test "rejects marketplace.json with duplicate plugin
names" currently calls validate_json without running or asserting failure;
change the test to invoke validate_json via the test harness (use run
validate_json "$marketplace_file") and then assert a non-zero exit (e.g., [
"$status" -ne 0 ] or run's failure assertion) and/or assert the stderr contains
a duplicate-name error; update the test block referencing validate_json and the
test name to ensure it actually fails when duplicate names ("dup-plugin") are
present.
- Around line 51-59: The test named "rejects hooks.json with invalid structure"
is asserting success against validate_json, which only checks JSON validity;
either update the test to actually assert rejection by calling the structural
validator (replace or follow validate_json with the schema/structure check
function—e.g., call validate_hooks or validate_json_schema on "$hooks_file") and
assert a non-zero exit status, or rename the test string to reflect current
behavior (e.g., "accepts valid JSON with invalid hooks structure") and adjust
the inline comment; locate the test by the `@test` "rejects hooks.json with
invalid structure" line and modify the assertion around validate_json
accordingly.
In `@tests/new_assertions.bats`:
- Around line 19-22: Add a failing test for assert_dir_not_exists by creating a
directory under TEST_TEMP_DIR and asserting the helper returns a non-zero
status; specifically add a test named like "assert_dir_not_exists fails when
directory exists" that creates the directory (mkdir -p), runs
assert_dir_not_exists on that path, and checks that "$status" is non-zero to
verify the assertion fails. Reference the existing test pattern in
tests/new_assertions.bats and reuse TEST_TEMP_DIR and a local variable (e.g.,
existing) to locate where to add the new test.
In `@tests/performance/benchmarks.bats`:
- Around line 42-62: The test divides total_time by plugin_count (and later by
file_count) which will cause a division-by-zero if no plugins/files matched;
update the benchmark test to guard the division by checking plugin_count (and
file_count) > 0 before computing avg_time or asserting — e.g., after the loop
that uses parse_plugin_json, if plugin_count is zero, either skip/fail the test
with a clear message or set avg_time to 0 and avoid the division; apply the same
pattern for the file_count block that uses benchmark_start/benchmark_end; use
the existing symbols parse_plugin_json, benchmark_start, benchmark_end,
total_time, plugin_count, file_count, and avg_time to locate and modify the
code.
- Around line 12-21: The current benchmark_start/benchmark_end uses date +%s%N
which is not portable (macOS emits a literal %N); replace that with a portable
helper get_time_ns() that returns an integer nanosecond timestamp: try date
+%s%N and detect if the output contains a literal "%N" (or if date supports %N),
and if not fall back to a reliable interpreter (e.g., python3 -c "import time;
print(int(time.time()*1e9))") or another portable method; then change
benchmark_start to set _BENCHMARK_START_TIME=$(get_time_ns) and benchmark_end to
read end_time=$(get_time_ns) and compute milliseconds via $(( (end_time -
_BENCHMARK_START_TIME) / 1000000 )), keeping the function names benchmark_start,
benchmark_end and the variable _BENCHMARK_START_TIME unchanged.
In `@tests/plugin_validation_common.bats`:
- Around line 202-211: The final assertion in the test "common:
validate_plugin_manifest_comprehensive works on valid plugin" is masked by a
trailing "|| true", which makes the check always pass; update the assertion that
inspects the output variable lines (the expression currently using [
"${`#lines`[@]}" -eq 0 ] || [[ "${lines[*]}" == *"all"*"valid"* ]]) by removing
the "|| true" and leaving the two-part check so the test fails when there are
lines that do not contain the expected "all" and "valid" text; target the test
block name and the variables plugin_path and lines to locate and fix the
assertion.
🧹 Nitpick comments (16)
tests/helpers/fixture_factory.bash (1)
246-291: Heredoc-based JSON generation is fragile with special characters in inputs.
create_marketplace_jsonuses raw string interpolation into JSON heredocs. Ifmarketplace_name,marketplace_description, orowner_namecontain characters like"or\, the output will be malformed JSON. Since this is a test fixture factory with controlled inputs, this is low-risk, but worth noting.Consider using
jq(already available via$JQ_BIN) for generating JSON safely if you ever need to handle arbitrary inputs.tests/helpers/test_utils.bash (3)
128-181: Inconsistent required fields betweenassert_valid_pluginandvalidate_plugin_manifest_comprehensive.
assert_valid_plugin(Line 148-173) requiresnameandversion, whilevalidate_plugin_manifest_comprehensive(Line 707) requiresname,description, andauthor— but notversion. Test authors using one vs. the other will get different validation behavior. Consider aligning the required fields or clearly documenting the intended difference.Also applies to: 682-745
296-307:for_each_skill_fileexcludes.worktreesbut notnode_modules.Other iteration helpers don't need this since they search under
$PROJECT_ROOT/plugins, butfor_each_skill_filesearches from$PROJECT_ROOT(the entire project tree). You haveis_under_node_modulesdefined below — consider addingnode_modulesexclusion to thegrep -vchain to avoid picking up vendored SKILL.md files.♻️ Suggested fix
- skill_files=$(find "$PROJECT_ROOT" -name "SKILL.md" -type f 2>/dev/null | grep -v ".worktrees" | sort) + skill_files=$(find "$PROJECT_ROOT" -name "SKILL.md" -type f 2>/dev/null | grep -v ".worktrees" | grep -v "node_modules" | sort)
474-488:count_matchesrelies on word-splitting of$find_args— intentional but fragile.Unquoted
$find_argson Line 487 enables multi-argument passing but will break if any argument value contains spaces. This is fine for the documented usage pattern ("-name plugin.json -type f"), but worth a brief inline comment noting the intentional word-splitting.tests/performance/benchmarks.bats (1)
110-132: Glob**requiresshopt -s globstarto recurse directories.Line 114 uses
"${PROJECT_ROOT}"/**/SKILL.mdwhich only recurses ifglobstaris enabled. Without it,**matches a single directory level. The skip onfile_count=0prevents hard failure, but the test may silently skip when it shouldn't.♻️ Use `find` for portability
- for skill_file in "${PROJECT_ROOT}"/**/SKILL.md; do + while IFS= read -r skill_file; do - if [ -f "$skill_file" ]; then benchmark_start has_frontmatter_delimiter "$skill_file" local elapsed @@ -120,7 +120,7 @@ total_time=$((total_time + elapsed)) file_count=$((file_count + 1)) - fi - done + done < <(find "${PROJECT_ROOT}" -name "SKILL.md" -type f 2>/dev/null)plugins/conversation-memory/src/core/parser.ts (1)
158-171: Non-deterministic tool call IDs contrast with deterministic exchange IDs.Exchange IDs are stable MD5 digests (line 94-97), but tool call IDs use
crypto.randomUUID()(line 161), meaning re-parsing the same file produces different tool call IDs each time. If these IDs are persisted or used for deduplication/diffing, this creates inconsistency.Consider deriving tool call IDs deterministically, e.g., hashing
archivePath + lineNumber + blockIndex.plugins/conversation-memory/src/core/parser-edge-cases.test.ts (1)
24-40: Weak assertion on malformed JSON handling.The test constructs 2 valid user-assistant exchanges interleaved with invalid lines, but Line 39 only asserts
toBeGreaterThanOrEqual(1). This makes the test pass even if one valid exchange is silently dropped. Consider assertingexpect(exchanges).toHaveLength(2)to ensure all valid exchanges survive.Suggested fix
- expect(exchanges.length).toBeGreaterThanOrEqual(1); + expect(exchanges).toHaveLength(2);tests/plugin_validation_common.bats (2)
187-200: Test name is misleading — it doesn't assert all plugins are valid.The test is named
"common: get_invalid_plugins returns empty list when all are valid"but it unconditionally passes even when invalid plugins are found (Lines 193-199 only validate the format of the invalid plugin names). The test name should reflect that it validates the function's output format, not that all plugins are valid.
75-161: Significant test overlap withtests/plugin_json.bats.The common test cases here (valid JSON, required fields, non-empty values, naming convention, allowed fields, comprehensive validation) largely duplicate what's already tested in
tests/plugin_json.bats. Since this file is documented as a "reusable module" that can be sourced by others, consider whether the@testblocks should be removed in favor of exporting only the helper functions, leaving the actual test assertions to consuming files.plugins/conversation-memory/src/core/parser-exclusion.test.ts (1)
120-134: Prefer static import forparseConversationFile.
parseConversationWithResultis already statically imported from./parser.jsat Line 4. The dynamicawait import('./parser.js')on Line 128 is unnecessary and inconsistent. AddparseConversationFileto the static import.Suggested fix
At line 4:
-import { parseConversationWithResult } from './parser.js'; +import { parseConversationWithResult, parseConversationFile } from './parser.js';At line 128:
- const { parseConversationFile } = await import('./parser.js'); - const result = await parseConversationFile(tempFilePath); + const result = await parseConversationFile(tempFilePath);tests/validate_paths.bats (1)
52-58: The.gitexclusion test is a no-op.This test only checks if
.gitexists and then unconditionally passes withtrue. It doesn't verify that any path-checking logic actually excludes.git. Consider either implementing a real assertion or marking this as a TODO/skip with a reason.tests/integration/cross_plugin_interactions.bats (2)
55-72: Considerskipinstead of hard failure when no skills exist.Line 71 asserts
found_skills > 0, which will fail if the repo has no skills. If skills are optional, this shouldskiprather than fail, similar to how other tests handle optional features.
140-161: Same concern: hard failure when no agents found.Line 160 asserts
found_agents > 0. If agents are optional, consider usingskipwhen none are found.tests/plugin_json.bats (1)
77-96:json_getreturns"null"string for null-valued JSON fields — not caught by empty check.On Line 87,
[ -z "$value" ]won't catch a JSON field with an explicitnullvalue, sincejq -routputs the literal string"null". Consider adding a null check for robustness:Suggested fix
- if [ -z "$value" ]; then + if [ -z "$value" ] || [ "$value" = "null" ]; thentests/negative_tests.bats (1)
138-144: Consider adding individual assertion messages for each invalid format.Each
run !silently overwrites$status/$output, so if an intermediate case incorrectly passes (returns 0), BATS will fail the test but won't indicate which format was wrongly accepted. Splitting into separate@testblocks or adding per-case diagnostics would improve debuggability..claude/ralph-loop.local.md (1)
3-4:max_iterations: 0for "unlimited" is ambiguous.A value of
0could reasonably mean "zero iterations allowed" or "unlimited." If this is the convention, it should be documented. A sentinel like-1or a separateunlimited: truefield would be clearer.
| --- | ||
| active: true | ||
| iteration: 2 | ||
| max_iterations: 0 | ||
| completion_promise: "베스트프렉티스로 적용된 test 로 전부 되었을때" | ||
| started_at: "2026-02-07T05:31:19Z" | ||
| --- |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if .claude/ralph-loop.local.md is gitignored
echo "=== Checking .gitignore for .local.md patterns ==="
if [ -f .gitignore ]; then
cat .gitignore | grep -E "(\.local|ralph-loop)" || echo "No matching .gitignore entries found"
else
echo "No .gitignore found"
fi
echo ""
echo "=== Checking if file is tracked ==="
git ls-files --error-unmatch .claude/ralph-loop.local.md 2>&1
echo ""
echo "=== Any .local.md files in git index? ==="
git ls-files '*.local.md' 2>/dev/null || echo "No .local.md files found"
echo ""
echo "=== Files in .claude/ directory ==="
git ls-files '.claude/*' 2>/dev/null | head -20
echo ""
echo "=== Check if .claude directory is in git ==="
git ls-tree -r HEAD .claude/ 2>/dev/null || echo "Not in current HEAD"Repository: baleen37/claude-plugins
Length of output: 591
Add .local.md files to .gitignore.
The file .claude/ralph-loop.local.md is tracked in git and contains ephemeral runtime state (active: true, iteration: 2, started_at timestamp). Files matching the .local.md convention should not be committed. Additionally, .claude/settings.local.json has the same issue. Add both to .gitignore and remove them from git tracking.
🤖 Prompt for AI Agents
In @.claude/ralph-loop.local.md around lines 1 - 7, Add
.claude/ralph-loop.local.md and .claude/settings.local.json (or the patterns
"*.local.md" and "settings.local.json" under the .claude directory) to
.gitignore, then remove the tracked copies from Git index (git rm --cached) and
commit the change so those ephemeral runtime files are no longer committed;
update .gitignore to include the new patterns and ensure you stage and commit
the removal of .claude/ralph-loop.local.md and .claude/settings.local.json from
the repository.
| ### Test Count | ||
|
|
||
| - 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.
Test count total doesn't add up.
The individual counts sum to 235 (148 + 20 + 10 + 23 + 17 + 17), but the total is listed as 178. Either the total should be updated or some categories (Error Handling, Edge Cases, Negative) are subsets of Unit Tests — which should be clarified.
🤖 Prompt for AI Agents
In `@docs/TESTING.md` around lines 46 - 54, The "### Test Count" block lists
category counts that sum to 235 but shows a total of 178; update that block
under the heading "### Test Count" so the total is correct (change **Total: 178
tests** to **Total: 235 tests**) or explicitly indicate which categories are
subsets of Unit Tests (e.g., annotate "Error Handling Tests", "Edge Case Tests",
"Negative Tests" as subsets) and then adjust the displayed totals accordingly to
remove double-counting.
| Performance benchmarks for critical operations: | ||
|
|
||
| - Plugin list caching effectiveness | ||
| - JSON parsing speed | ||
| - File operation efficiency | ||
| - Test execution timing |
There was a problem hiding this comment.
Duplicate content: performance benchmarks list repeated.
Lines 188–193 already describe performance benchmarks. Lines 215–220 repeat the exact same list under the "edge_cases.bats" section. This appears to be a copy-paste error.
🐛 Proposed fix — remove duplicate block
- Minimal/maximum valid configurations
-Performance benchmarks for critical operations:
-
-- Plugin list caching effectiveness
-- JSON parsing speed
-- File operation efficiency
-- Test execution timing
-
## Test Helpers📝 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.
| Performance benchmarks for critical operations: | |
| - Plugin list caching effectiveness | |
| - JSON parsing speed | |
| - File operation efficiency | |
| - Test execution timing | |
| - Minimal/maximum valid configurations | |
| ## Test Helpers |
🤖 Prompt for AI Agents
In `@docs/TESTING.md` around lines 215 - 220, Remove the duplicated "Performance
benchmarks for critical operations" bullet list that was copy-pasted under the
"edge_cases.bats" section; specifically locate the block that starts with
"Performance benchmarks for critical operations:" and the bullets ("Plugin list
caching effectiveness", "JSON parsing speed", "File operation efficiency", "Test
execution timing") in the edge_cases.bats section and delete that redundant
block so only the original list (the one in Lines 188–193) remains.
| ### Fixture Factory (`tests/helpers/fixture_factory.bash`) | ||
|
|
||
| - `create_minimal_plugin <base_dir> <name> [version] [author]`: Create minimal plugin | ||
| - `create_full_plugin <base_dir> <name> [version] [author]`: Create complete plugin | ||
| - `create_command_file <commands_dir> <name> <description>`: Create command file | ||
| - `create_agent_file <agents_dir> <name> <description> [model]`: Create agent file | ||
| - `create_skill_file <skills_dir> <name> <description> [content]`: Create skill directory | ||
| - `cleanup_fixtures <fixture_root>`: Clean up test fixtures |
There was a problem hiding this comment.
Fixture factory signatures in docs are stale.
The documentation shows [author] as a single optional parameter, but create_minimal_plugin and create_full_plugin now accept [author_name] [author_email] as separate parameters.
📝 Proposed fix
-- `create_minimal_plugin <base_dir> <name> [version] [author]`: Create minimal plugin
-- `create_full_plugin <base_dir> <name> [version] [author]`: Create complete plugin
+- `create_minimal_plugin <base_dir> <name> [version] [author_name] [author_email]`: Create minimal plugin
+- `create_full_plugin <base_dir> <name> [version] [author_name] [author_email]`: Create complete plugin🤖 Prompt for AI Agents
In `@docs/TESTING.md` around lines 265 - 272, Update the Fixture Factory section
to reflect the new signatures: change the `create_minimal_plugin` and
`create_full_plugin` entries to accept two optional author parameters
(`[author_name] [author_email]`) instead of a single `[author]`; keep the other
function signatures as-is and ensure the descriptions note that author name and
email are separate optional arguments so callers pass both when available.
| test('returns "unknown" for project when path has insufficient parts', async () => { | ||
| const jsonlContent = [ | ||
| { type: 'user', message: { role: 'user', content: 'Hello' }, timestamp: '2024-01-01T00:00:00.000Z' }, | ||
| { type: 'assistant', message: { role: 'assistant', content: 'Hi!' }, timestamp: '2024-01-01T00:00:01.000Z' } | ||
| ].map((obj) => JSON.stringify(obj)); | ||
|
|
||
| fs.writeFileSync(tempFilePath, jsonlContent.join('\n')); | ||
| const result = await parseConversationFile(tempFilePath); | ||
|
|
||
| // tempFilePath ends with 'test-conversation.jsonl', so parent is the tempDir name | ||
| expect(result.project).toBeDefined(); | ||
| expect(typeof result.project).toBe('string'); | ||
| }); |
There was a problem hiding this comment.
Test name is misleading — it doesn't actually exercise the "unknown" project fallback.
tempFilePath is an absolute path (e.g., /tmp/parser-test-xxx/test-conversation.jsonl) which always has ≥ 2 parts, so project will be the temp directory name, never "unknown". The assertions (lines 170-171) only check that project is a defined string, which every path satisfies.
To actually test the "unknown" fallback, you'd need a path with fewer than 2 segments (e.g., just a filename with no directory). Consider renaming the test to reflect what it actually validates, or adjusting the test to cover the edge case.
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/parser-basic.test.ts` around lines 160 -
172, The test case name and assertions are misleading because
parseConversationFile(tempFilePath) uses an absolute tempFilePath which will
always have ≥2 path segments and thus never hit the "unknown" fallback; update
the test by either renaming it to reflect that it only asserts project is a
defined string, or modify the input to actually exercise the fallback by passing
a path with fewer than 2 segments (e.g., just 'test-conversation.jsonl' or a
bare filename) to parseConversationFile and then assert result.project ===
'unknown'; reference the existing test function and
tempFilePath/parseConversationFile identifiers when making the change.
| @test "rejects plugin.json with wrong field type" { | ||
| local FIXTURE_ROOT="${TEST_TEMP_DIR}/negative_tests" | ||
| local plugin_path | ||
| plugin_path=$(create_plugin_with_custom_fields "$FIXTURE_ROOT" "wrong-type-test" '"keywords": "should-be-array"') | ||
|
|
||
| local manifest="$plugin_path/.claude-plugin/plugin.json" | ||
| validate_json "$manifest" | ||
| # JSON is valid but type is wrong - verify it's a string not array | ||
| assert_json_field_type "$manifest" "keywords" "string" "keywords field should be string (wrong type for this test)" | ||
| } |
There was a problem hiding this comment.
Test verifies the fixture, not the rejection of wrong types.
"rejects plugin.json with wrong field type" confirms that keywords is indeed a string (the intentionally wrong type), but never asserts that a validator rejects it. If the goal is to test that a type-checking validator flags this, a run + failure assertion on a type-validation function is needed.
🤖 Prompt for AI Agents
In `@tests/negative_tests.bats` around lines 170 - 179, The test "rejects
plugin.json with wrong field type" currently only creates a fixture with
create_plugin_with_custom_fields and asserts the manifest's keywords is a string
via assert_json_field_type; change it to actually run the type-check validator
and assert it fails: after building the manifest (using
create_plugin_with_custom_fields and manifest path), invoke the validator
command/function used elsewhere in tests (e.g., run validate_json or the CLI
validator) against "$manifest" and assert a non-zero exit (e.g., use run .. and
assert_failure or equivalent) and optionally assert the validator output
mentions "keywords" or "type"; keep the existing helpers
validate_json/assert_json_field_type for verifying fixture setup but add the
run+failure assertion to ensure the validator rejects the wrong-type "keywords".
| @test "assert_dir_not_exists passes when directory does not exist" { | ||
| local non_existent="${TEST_TEMP_DIR}/does-not-exist" | ||
| assert_dir_not_exists "$non_existent" "Directory should not exist" | ||
| } |
There was a problem hiding this comment.
Missing failure-case test for assert_dir_not_exists.
Other assertion tests include both passing and failing cases (e.g., assert_file_not_exists has both at lines 6-17), but assert_dir_not_exists only tests the passing case. Consider adding a test that creates a directory and verifies the assertion fails.
Suggested addition after line 22
`@test` "assert_dir_not_exists fails when directory exists" {
local existing="${TEST_TEMP_DIR}/exists-dir"
mkdir -p "$existing"
run assert_dir_not_exists "$existing"
[ "$status" -ne 0 ]
}🤖 Prompt for AI Agents
In `@tests/new_assertions.bats` around lines 19 - 22, Add a failing test for
assert_dir_not_exists by creating a directory under TEST_TEMP_DIR and asserting
the helper returns a non-zero status; specifically add a test named like
"assert_dir_not_exists fails when directory exists" that creates the directory
(mkdir -p), runs assert_dir_not_exists on that path, and checks that "$status"
is non-zero to verify the assertion fails. Reference the existing test pattern
in tests/new_assertions.bats and reuse TEST_TEMP_DIR and a local variable (e.g.,
existing) to locate where to add the new test.
| benchmark_start() { | ||
| _BENCHMARK_START_TIME=$(date +%s%N 2>/dev/null || date +%s000000000) | ||
| } | ||
|
|
||
| # Helper: End benchmark and return elapsed milliseconds | ||
| benchmark_end() { | ||
| local end_time | ||
| end_time=$(date +%s%N 2>/dev/null || date +%s000000000) | ||
| echo $(( (end_time - _BENCHMARK_START_TIME) / 1000000 )) | ||
| } |
There was a problem hiding this comment.
date +%s%N is not portable to macOS — benchmark timing will be broken.
On macOS, date +%s%N succeeds (exit 0) but outputs literal %N in the result (e.g., 1707300000%N), so the || fallback never triggers. Subsequent arithmetic in benchmark_end will fail or produce nonsensical values.
🐛 Proposed fix — use a portable nanosecond timer with proper detection
benchmark_start() {
- _BENCHMARK_START_TIME=$(date +%s%N 2>/dev/null || date +%s000000000)
+ local ns
+ ns=$(date +%s%N 2>/dev/null)
+ if [[ "$ns" == *N* ]] || [[ -z "$ns" ]]; then
+ # macOS date doesn't support %N — fall back to seconds with zero-padded ns
+ ns=$(date +%s000000000)
+ fi
+ _BENCHMARK_START_TIME="$ns"
}
benchmark_end() {
local end_time
- end_time=$(date +%s%N 2>/dev/null || date +%s000000000)
+ end_time=$(date +%s%N 2>/dev/null)
+ if [[ "$end_time" == *N* ]] || [[ -z "$end_time" ]]; then
+ end_time=$(date +%s000000000)
+ fi
echo $(( (end_time - _BENCHMARK_START_TIME) / 1000000 ))
}📝 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.
| benchmark_start() { | |
| _BENCHMARK_START_TIME=$(date +%s%N 2>/dev/null || date +%s000000000) | |
| } | |
| # Helper: End benchmark and return elapsed milliseconds | |
| benchmark_end() { | |
| local end_time | |
| end_time=$(date +%s%N 2>/dev/null || date +%s000000000) | |
| echo $(( (end_time - _BENCHMARK_START_TIME) / 1000000 )) | |
| } | |
| benchmark_start() { | |
| local ns | |
| ns=$(date +%s%N 2>/dev/null) | |
| if [[ "$ns" == *N* ]] || [[ -z "$ns" ]]; then | |
| # macOS date doesn't support %N — fall back to seconds with zero-padded ns | |
| ns=$(date +%s000000000) | |
| fi | |
| _BENCHMARK_START_TIME="$ns" | |
| } | |
| # Helper: End benchmark and return elapsed milliseconds | |
| benchmark_end() { | |
| local end_time | |
| end_time=$(date +%s%N 2>/dev/null) | |
| if [[ "$end_time" == *N* ]] || [[ -z "$end_time" ]]; then | |
| end_time=$(date +%s000000000) | |
| fi | |
| echo $(( (end_time - _BENCHMARK_START_TIME) / 1000000 )) | |
| } |
🤖 Prompt for AI Agents
In `@tests/performance/benchmarks.bats` around lines 12 - 21, The current
benchmark_start/benchmark_end uses date +%s%N which is not portable (macOS emits
a literal %N); replace that with a portable helper get_time_ns() that returns an
integer nanosecond timestamp: try date +%s%N and detect if the output contains a
literal "%N" (or if date supports %N), and if not fall back to a reliable
interpreter (e.g., python3 -c "import time; print(int(time.time()*1e9))") or
another portable method; then change benchmark_start to set
_BENCHMARK_START_TIME=$(get_time_ns) and benchmark_end to read
end_time=$(get_time_ns) and compute milliseconds via $(( (end_time -
_BENCHMARK_START_TIME) / 1000000 )), keeping the function names benchmark_start,
benchmark_end and the variable _BENCHMARK_START_TIME unchanged.
| @test "plugin JSON parsing is reasonably fast" { | ||
| local total_time=0 | ||
| local plugin_count=0 | ||
|
|
||
| for plugin_dir in "${PROJECT_ROOT}"/plugins/*/; do | ||
| if [ -d "$plugin_dir" ]; then | ||
| benchmark_start | ||
| parse_plugin_json "$plugin_dir" > /dev/null | ||
| local elapsed | ||
| elapsed=$(benchmark_end) | ||
| total_time=$((total_time + elapsed)) | ||
| plugin_count=$((plugin_count + 1)) | ||
| fi | ||
| done | ||
|
|
||
| local avg_time=$((total_time / plugin_count)) | ||
| echo "Parsed $plugin_count plugins in ${total_time}ms (avg: ${avg_time}ms each)" | ||
|
|
||
| # Each plugin should parse in less than 100ms | ||
| assert_lt "$avg_time" "100" "Average parse time should be under 100ms" | ||
| } |
There was a problem hiding this comment.
Division by zero if no plugins or JSON files are found.
Lines 57 and 91 compute $((total_time / plugin_count)) and $((total_time / file_count)) respectively. If the glob matches nothing, the counter stays at 0, causing a divide-by-zero arithmetic error.
🐛 Proposed fix (example for lines 56-58)
+ if [ "$plugin_count" -eq 0 ]; then
+ skip "No plugins found"
+ fi
local avg_time=$((total_time / plugin_count))Apply the same pattern before Line 91 for file_count.
Also applies to: 76-96
🤖 Prompt for AI Agents
In `@tests/performance/benchmarks.bats` around lines 42 - 62, The test divides
total_time by plugin_count (and later by file_count) which will cause a
division-by-zero if no plugins/files matched; update the benchmark test to guard
the division by checking plugin_count (and file_count) > 0 before computing
avg_time or asserting — e.g., after the loop that uses parse_plugin_json, if
plugin_count is zero, either skip/fail the test with a clear message or set
avg_time to 0 and avoid the division; apply the same pattern for the file_count
block that uses benchmark_start/benchmark_end; use the existing symbols
parse_plugin_json, benchmark_start, benchmark_end, total_time, plugin_count,
file_count, and avg_time to locate and modify the code.
| @test "common: validate_plugin_manifest_comprehensive works on valid plugin" { | ||
| local FIXTURE_ROOT="${TEST_TEMP_DIR}/comprehensive_test" | ||
| local plugin_path | ||
| plugin_path=$(create_minimal_plugin "$FIXTURE_ROOT" "test-valid-plugin") | ||
|
|
||
| run validate_plugin_manifest_comprehensive "$plugin_path" | ||
| # Should pass with exit code 0 | ||
| [ "$status" -eq 0 ] | ||
| [ "${#lines[@]}" -eq 0 ] || [[ "${lines[*]}" == *"all"*"valid"* ]] || true | ||
| } |
There was a problem hiding this comment.
Assertion on Line 210 always passes due to || true.
The expression [ "${#lines[@]}" -eq 0 ] || [[ "${lines[*]}" == *"all"*"valid"* ]] || true short-circuits to success regardless of output content. If the intent is to verify output when present, drop the || true:
Suggested fix
- [ "${`#lines`[@]}" -eq 0 ] || [[ "${lines[*]}" == *"all"*"valid"* ]] || true
+ [ "${`#lines`[@]}" -eq 0 ] || [[ "${lines[*]}" == *"all"*"valid"* ]]📝 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 "common: validate_plugin_manifest_comprehensive works on valid plugin" { | |
| local FIXTURE_ROOT="${TEST_TEMP_DIR}/comprehensive_test" | |
| local plugin_path | |
| plugin_path=$(create_minimal_plugin "$FIXTURE_ROOT" "test-valid-plugin") | |
| run validate_plugin_manifest_comprehensive "$plugin_path" | |
| # Should pass with exit code 0 | |
| [ "$status" -eq 0 ] | |
| [ "${#lines[@]}" -eq 0 ] || [[ "${lines[*]}" == *"all"*"valid"* ]] || true | |
| } | |
| `@test` "common: validate_plugin_manifest_comprehensive works on valid plugin" { | |
| local FIXTURE_ROOT="${TEST_TEMP_DIR}/comprehensive_test" | |
| local plugin_path | |
| plugin_path=$(create_minimal_plugin "$FIXTURE_ROOT" "test-valid-plugin") | |
| run validate_plugin_manifest_comprehensive "$plugin_path" | |
| # Should pass with exit code 0 | |
| [ "$status" -eq 0 ] | |
| [ "${`#lines`[@]}" -eq 0 ] || [[ "${lines[*]}" == *"all"*"valid"* ]] | |
| } |
🤖 Prompt for AI Agents
In `@tests/plugin_validation_common.bats` around lines 202 - 211, The final
assertion in the test "common: validate_plugin_manifest_comprehensive works on
valid plugin" is masked by a trailing "|| true", which makes the check always
pass; update the assertion that inspects the output variable lines (the
expression currently using [ "${`#lines`[@]}" -eq 0 ] || [[ "${lines[*]}" ==
*"all"*"valid"* ]]) by removing the "|| true" and leaving the two-part check so
the test fails when there are lines that do not contain the expected "all" and
"valid" text; target the test block name and the variables plugin_path and lines
to locate and fix the assertion.
Pull request was closed
1 participant
Summary
Syncs ralph-loop plugin architecture with the official ralph-wiggum plugin, simplifying from a multi-session enhanced version back to the official single-session approach.
Changes
Removed Features
scripts/lib/state.shdependencyUpdated Files
hooks/hooks.json: Removed SessionStart hook, matches officialhooks/stop-hook.sh: Reverted to official implementation (177 lines)commands/cancel-ralph.md: Uses inline operations vs scriptcommands/ralph-loop.md: Minor description updatecommands/help.md: Fixed markdownlint issuesREADME.md: Fixed markdownlint issuesDeleted Files
hooks/session-start-hook.shscripts/cancel-ralph.shscripts/lib/state.shpackage.json,bun.lock(not needed for plugins)tests/ralph-loop-specific.batsPreserved Local Differences
ralph-loop(branding preference)5.7.2(local versioning scheme)Motivation
The official plugin's simpler architecture is sufficient for current use cases. The multi-session enhancements added complexity without clear benefit, and maintaining them in sync with official updates was becoming difficult.
Testing
Auto Merge
This PR is ready for auto-merge when CI passes.
Summary by CodeRabbit
Documentation
Refactor
Tests
Chores