feat(enable): add ralph-enable wizard for existing projects (v0.11.0)#124
Conversation
Add interactive wizard and CI version for enabling Ralph in existing projects. New commands: - ralph-enable: Interactive 5-phase wizard for humans - ralph-enable-ci: Non-interactive version with JSON output for CI/automation New library components: - lib/enable_core.sh: Shared logic for idempotency, project detection, templates - lib/wizard_utils.sh: Interactive prompt utilities - lib/task_sources.sh: Task import from beads, GitHub Issues, PRD documents Features: - Auto-detects project type (TypeScript, Python, Rust, Go) - Auto-detects framework (Next.js, FastAPI, Django, Express) - Imports tasks from beads, GitHub Issues, or PRD documents - Generates .ralphrc project configuration file - Idempotent: safe to run multiple times, respects existing files - Exit codes: 0 (success), 1 (error), 2 (already enabled) Updated: - install.sh: Added new commands to global installation - ralph_loop.sh: Loads .ralphrc configuration at startup Tests: 75 new tests (30 enable_core + 23 task_sources + 22 integration) Total: 396 tests passing (100% pass rate) Closes #85, #121, #64, #87, #99
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an idempotent "enable Ralph in existing project" feature: interactive (ralph-enable) and CI (ralph-enable-ci) commands, three new libraries (enable_core.sh, wizard_utils.sh, task_sources.sh), .ralphrc template and runtime loader, installer wiring for new commands, and extensive unit/integration tests and mocks. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "ralph-enable / ralph-enable-ci"
participant Core as "lib/enable_core.sh"
participant Tasks as "lib/task_sources.sh"
participant FS as "FileSystem"
User->>CLI: invoke enable command (opts)
CLI->>Core: detect_project_context()
Core-->>CLI: project metadata
CLI->>Core: detect_git_info()
Core-->>CLI: git/github info
CLI->>Core: detect_task_sources()
Core-->>CLI: available sources
alt Interactive
CLI->>User: prompt for sources/config
User-->>CLI: selections
end
CLI->>Tasks: import_tasks_from_sources(selected)
Tasks->>Tasks: fetch, normalize, prioritize
Tasks-->>CLI: aggregated tasks
CLI->>Core: enable_ralph_in_directory(options + tasks)
Core->>FS: create `.ralph` structure and write files (PROMPT.md, `@AGENT.md`, `@fix_plan.md`, .ralphrc)
Core-->>CLI: success + created files
CLI->>FS: verify required files exist
CLI->>User: print completion or emit JSON result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Add ralph-enable wizard and CI command to enable Ralph in existing projects and update startup to load .ralphrc with environment-precedence (v0.11.0)Introduce interactive 📍Where to StartStart with the Macroscope summarized 0bce761. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Code Review - PR 124: Ralph Enable Wizard (v0.11.0)This is an excellent feature addition that significantly improves the onboarding experience for existing projects. The implementation is well-structured, thoroughly tested, and follows Ralph's established patterns. Strengths1. Comprehensive Testing (75 new tests, 100% pass rate)
2. Solid Architecture
3. Idempotent and Safe
4. Project Detection
Critical Issues1. Shell Injection Risk in safe_create_file() (lib/enable_core.sh:160) Using echo for file writing can cause problems with content containing backslashes, content starting with -e/-n/-E flags, and newline interpretation variations. Fix: Use printf for reliable output: if printf '%s\n' "$content" > "$target" 2>/dev/null; thenImpact: Template generation could corrupt content with special characters. High Priority Issues2. Unchecked Command Substitution in Project Detection Multiple locations (lib/enable_core.sh:277, 320, 347, 357) use command substitution without validation. Malformed JSON/TOML could inject unexpected values. 3. Missing JSON Validation lib/task_sources.sh:168 and similar functions should validate JSON structure before parsing. Medium Priority Issues4. Inconsistent Bash Shebang Library files use #!/usr/bin/env bash while main scripts use #!/bin/bash. Should standardize for portability. 5. Environment Variable Handling Bug (ralph_loop.sh:102-136) The save/restore pattern loses empty string vs unset distinction. Should use [[ -v VAR ]] test. 6. Template Variable Substitution Issue templates/ralphrc.template:14 uses shell parameter expansion that evaluates at runtime instead of generation time. RecommendationsMust fix before merge:
Recommended for follow-up PR:
Overall AssessmentCode Quality: 4/5 APPROVED with requested changes Great work on this PR! It closes 5 issues and delivers exactly what the Ralph community needs for easier adoption. |
Code Review: feat(enable): add ralph-enable wizard for existing projects (v0.11.0)Excellent work on adding the �[1m━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━�[0m Analyzing your project... �[1;33m○�[0m Ralph status: Not configured Available task sources: �[1m━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━�[0m Where would you like to import tasks from? Selected sources: none �[1m━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━�[0m �[1m┌─ Configuration ───────────────────────────────────────┐�[0m �[1m━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━�[0m Creating Ralph configuration... �[0;34m[INFO]�[0m Enabling Ralph for: �[0;36mProject name�[0m [ralph-claude-code]: ralph-claude-code �[1m━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━�[0m Checking created files... �[0;32m✓�[0m .ralph/PROMPT.md �[0;32m✓�[0m Ralph enabled successfully! Next steps: �[0;36m1.�[0m Review and customize .ralph/PROMPT.md �[0;36mShow current status?�[0m [Y/n]: ✅ Strengths
Ralph is already enabled in this project. Use --force to override. script with JSON output is well-designed
|
|
Author identity unknown *** Please tell me who you are. Run git config --global user.email "you@example.com" to set your account's default identity. fatal: empty ident name (for runner@runnervmymu0l.rpzyaon4tsbuzbyx4xvysldu2h.bx.internal.cloudapp.net) not allowed |
Code Review: PR #124 - Ralph Enable WizardOverall this is a well-architected feature with excellent test coverage (75 new tests, 100% pass rate). The modular design follows Ralph's established patterns and provides significant value for onboarding existing projects. However, there are several issues that should be addressed before merge: 🔴 Critical Issues (Must Fix)1. sed Command Injection Vulnerability (ralph_enable.sh:456, 460)The sed commands use user-provided sed -i "s/MAX_CALLS_PER_HOUR=.*/MAX_CALLS_PER_HOUR=/" .ralphrc
sed -i "s/GITHUB_TASK_LABEL=.*/GITHUB_TASK_LABEL=\"\"/" .ralphrcImpact: Special characters break the command. Examples:
Fix: Use different delimiter or escape: # Option 1: Use | as delimiter
sed -i "s|MAX_CALLS_PER_HOUR=.*|MAX_CALLS_PER_HOUR=|" .ralphrc
sed -i "s|GITHUB_TASK_LABEL=.*|GITHUB_TASK_LABEL=\"\"|" .ralphrc
# Option 2: Escape special chars (more robust)
safe_label=""
sed -i "s/GITHUB_TASK_LABEL=.*/GITHUB_TASK_LABEL=\"\"/" .ralphrc2. Missing jq Dependency Check (ralph_enable_ci.sh:174-177)The --json)
OUTPUT_JSON=true
shift
;;At line 230, files_json=[
"\"${CREATED_FILES[@]}\"\\n"
]Fix: Add validation when --json)
if ! command -v jq &>/dev/null; then
output_error "jq is required for --json output. Install with: apt-get install jq (Linux) or brew install jq (macOS)"
exit 1
fi
OUTPUT_JSON=true
shift
;;3. Shell Injection via echo in safe_create_file (lib/enable_core.sh:160)Using if echo "" > "" 2>/dev/null; thenIssues:
Fix: Use if printf '%s\n' "" > "" 2>/dev/null; then🟡 High Priority Issues4. .ralphrc Template Variable Substitution Issue (templates/ralphrc.template:14)PROJECT_NAME="${PROJECT_NAME:-my-project}"This evaluates at runtime when sourced, not at file generation time. If Expected behavior: Template variables should be substituted when the file is created. Fix: In 5. Environment Variable Handling Bug (ralph_loop.sh:102-136)The save/restore pattern loses the distinction between "empty string" and "unset": local saved_MAX_CALLS_PER_HOUR="${MAX_CALLS_PER_HOUR:-}"
# ... source .ralphrc ...
[[ -n "" ]] && MAX_CALLS_PER_HOUR=""If Fix: Use `[[ -v VAR ]]" test: # Save
[[ -v MAX_CALLS_PER_HOUR ]] && saved_MAX_CALLS_PER_HOUR="true" || saved_MAX_CALLS_PER_HOUR="false"
local saved_max_calls_value="$MAX_CALLS_PER_HOUR"
# Restore
if [[ "" == "true" ]]; then
MAX_CALLS_PER_HOUR=""
fi6. Race Condition in File CreationIn # check_existing_ralph checks if file exists
# Later, safe_create_file creates itBetween the check and creation, another process could create the file. Impact: Low risk for interactive use, but could affect CI scenarios. Fix: Use 🟢 Medium Priority Issues7. Global Variable State (ralph_enable_ci.sh:207-208)declare -a CREATED_FILES=()
TASKS_IMPORTED=0
8. Shebang InconsistencyLibrary files use 9. sed Portability (lib/task_sources.sh:378, 386)text=""BSD sed (macOS) doesn't support text=""✅ Strengths
📋 Recommended Action PlanBefore merge (must fix):
Follow-up PR (should fix): Optional (nice to have): 🎯 Overall AssessmentCode Quality: 4/5 Status: The feature is well-designed and thoroughly tested, but the sed injection and jq dependency issues must be fixed before merge. These are not theoretical—they will cause real problems for users. |
|
Review posted focusing on security vulnerabilities (sed injection, missing jq check, echo/printf issue), high-priority bugs (.ralphrc template substitution, environment var handling), and providing a clear action plan for before merge vs follow-up PR. |
![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
🤖 Fix all issues with AI agents
In `@lib/enable_core.sh`:
- Around line 140-166: safe_create_file currently always skips if the target
exists which prevents enable_ralph_in_directory's --force behavior from working;
change safe_create_file to accept a third parameter (e.g., force) and only
return "SKIP" when the file exists and force is not true, otherwise overwrite
the file (perform the same parent-dir creation and write logic) and log
"OVERWRITE" or "SUCCESS" appropriately; update callers like
enable_ralph_in_directory to pass the force flag through when invoking
safe_create_file so --force actually replaces existing
PROMPT/@AGENT/@fix_plan/.ralphrc files.
In `@lib/task_sources.sh`:
- Around line 482-533: The header lines like "# Tasks from beads" are being
treated as tasks because normalize_tasks treats every line as a task; fix by
normalizing each source's raw output separately and appending only the
normalized results (or strip header lines before normalization). Specifically,
for each fetch function (fetch_beads_tasks -> beads_tasks, fetch_github_tasks ->
github_tasks, extract_prd_tasks -> prd_tasks) call normalize_tasks on that
string and then append the normalized output to all_tasks (increment
source_count as before) instead of inserting literal "# Tasks ..." headings;
alternatively remove any leading '#' comment lines from each source before
calling normalize_tasks. Ensure you update the code paths that currently build
all_tasks and still use source_count, and keep the final normalize_tasks
"$all_tasks" "combined" call only if you intend a second pass normalization.
- Around line 43-82: fetch_beads_tasks currently runs bd list --json and only
falls back to plain-text if the bd command fails, so if jq is missing or JSON
parsing yields no tasks they get silently dropped; also the local filter
parameter is never applied. Update fetch_beads_tasks to (1) include the filter
when invoking bd (e.g., pass "$filter" to bd list or build args from filter) or
apply the filter in the jq/select step, (2) after capturing json_output, attempt
to parse it with jq only if jq is present, but if jq is missing or parsing
produces an empty $tasks, run the plain-text fallback (bd list with the same
filter) and parse lines to produce the markdown task lines; mirror this same
pattern and fallback behavior in fetch_github_tasks so missing jq or parse
failures never drop results.
In `@ralph_enable_ci.sh`:
- Around line 381-385: The current post-generation verification in
ralph_enable_ci.sh only checks for ".ralph/PROMPT.md" and ".ralph/@fix_plan.md"
but check_existing_ralph (in enable_core.sh) also treats ".ralph/@AGENT.md" as
required; update the verification block in ralph_enable_ci.sh to include
".ralph/@AGENT.md" so the script fails when the agent file is missing, ensuring
consistency with check_existing_ralph.
In `@ralph_enable.sh`:
- Around line 184-186: The call to check_existing_ralph can return non-zero (1
or 2) and with set -e enabled will abort the script before the subsequent case
on RALPH_STATE; change the invocation so the script does not exit on those
expected return codes (for example, run check_existing_ralph in a way that
ignores its exit status or temporarily disable set -e around the call) so
RALPH_STATE can be evaluated in the case statement; update the call to
check_existing_ralph used before the case and ensure RALPH_STATE is still set
afterwards.
- Around line 453-462: The current in-place edits use "sed -i" which fails on
macOS; replace the two sed -i invocations (the MAX_CALLS_PER_HOUR and
GITHUB_TASK_LABEL updates) with a POSIX-safe edit that writes to a temporary
file and atomically moves it back (e.g., use mktemp -> run sed without -i into
the temp -> mv temp .ralphrc), remove the blind 2>/dev/null || true suppression
so failures are visible, and keep the same substitution patterns referencing
CONFIG_MAX_CALLS, CONFIG_GITHUB_LABEL, and GITHUB_TASK_LABEL to ensure the
intended updates to .ralphrc occur on both Linux and macOS.
In `@ralph_loop.sh`:
- Around line 102-136: The restore logic in load_ralphrc (the saved_* -> restore
block) always wins because defaults make saved_* non-empty, wiping out .ralphrc
changes; change this to restore only when an explicit override was provided by
the CLI/env by introducing per-setting override flags (e.g.,
MAX_CALLS_PER_HOUR_OVERRIDE, CLAUDE_ALLOWED_TOOLS_OVERRIDE,
CLAUDE_USE_CONTINUE_OVERRIDE, CLAUDE_SESSION_EXPIRY_HOURS_OVERRIDE,
VERBOSE_PROGRESS_OVERRIDE) and check those flags in the restore block instead of
testing saved_* (e.g., only restore MAX_CALLS_PER_HOUR if
MAX_CALLS_PER_HOUR_OVERRIDE is true); set the corresponding *_OVERRIDE=true at
the points that explicitly override values (for example in the --calls handler
and wherever env/CLI parsing assigns defaults) so .ralphrc values remain
effective unless explicitly overridden.
🧹 Nitpick comments (5)
lib/enable_core.sh (1)
18-24: Add a function-level comment for_color.This helper currently lacks the required function-level comment. As per coding guidelines, add a short doc line describing its behavior.
tests/unit/test_enable_core.bats (1)
335-346: Force-overwrite test doesn’t validate the overwrite behavior.The test only checks exit status; it doesn’t confirm that old content was replaced. Adding a content assertion would catch the current
--forceoverwrite bug and prevent regressions. Based on learnings, strengthen this test to validate behavior.✅ Example assertion
run enable_ralph_in_directory assert_success + [[ "$(cat .ralph/PROMPT.md)" != "old content" ]]tests/unit/test_task_sources.bats (1)
209-239: Minor inconsistency in test patterns.Tests at lines 213 and 236 use direct function calls with
|| true, while test at line 225 usesrun. Both approaches work, but consistency would improve maintainability. Consider usingrunconsistently for all tests to leverage BATS'$outputand$statusvariables uniformly.ralph_enable_ci.sh (1)
224-247: Consider escaping special characters in JSON string values.The
output_successfunction directly interpolates$project_nameand$project_typeinto the JSON heredoc. If these contain quotes or special characters, the JSON could be malformed. Sincejqis available, consider using it for proper escaping.♻️ Proposed fix for JSON safety
output_success() { local project_name="$1" local project_type="$2" if [[ "$OUTPUT_JSON" == "true" ]]; then local files_json files_json=$(printf '%s\n' "${CREATED_FILES[@]}" | jq -R . | jq -s .) - cat << EOF -{ - "success": true, - "project_name": "$project_name", - "project_type": "$project_type", - "files_created": $files_json, - "tasks_imported": $TASKS_IMPORTED, - "message": "Ralph enabled successfully" -} -EOF + jq -n \ + --arg name "$project_name" \ + --arg type "$project_type" \ + --argjson files "$files_json" \ + --argjson tasks "$TASKS_IMPORTED" \ + '{success: true, project_name: $name, project_type: $type, files_created: $files, tasks_imported: $tasks, message: "Ralph enabled successfully"}' elseinstall.sh (1)
349-349: Consider breaking the long uninstall command for readability.The uninstall
rm -fcommand now spans many arguments. While functional, breaking it across multiple lines would improve maintainability.♻️ Suggested improvement
uninstall) log "INFO" "Uninstalling Ralph for Claude Code..." - rm -f "$INSTALL_DIR/ralph" "$INSTALL_DIR/ralph-monitor" "$INSTALL_DIR/ralph-setup" "$INSTALL_DIR/ralph-import" "$INSTALL_DIR/ralph-migrate" "$INSTALL_DIR/ralph-enable" "$INSTALL_DIR/ralph-enable-ci" + rm -f "$INSTALL_DIR/ralph" \ + "$INSTALL_DIR/ralph-monitor" \ + "$INSTALL_DIR/ralph-setup" \ + "$INSTALL_DIR/ralph-import" \ + "$INSTALL_DIR/ralph-migrate" \ + "$INSTALL_DIR/ralph-enable" \ + "$INSTALL_DIR/ralph-enable-ci" rm -rf "$RALPH_HOME"
| safe_create_file() { | ||
| local target=$1 | ||
| local content=$2 | ||
|
|
||
| if [[ -f "$target" ]]; then | ||
| enable_log "SKIP" "$target already exists" | ||
| return 1 | ||
| fi | ||
|
|
||
| # Create parent directory if needed | ||
| local parent_dir | ||
| parent_dir=$(dirname "$target") | ||
| if [[ ! -d "$parent_dir" ]]; then | ||
| if ! mkdir -p "$parent_dir" 2>/dev/null; then | ||
| enable_log "ERROR" "Failed to create directory: $parent_dir" | ||
| return 2 | ||
| fi | ||
| fi | ||
|
|
||
| # Write content to file | ||
| if echo "$content" > "$target" 2>/dev/null; then | ||
| enable_log "SUCCESS" "Created $target" | ||
| return 0 | ||
| else | ||
| enable_log "ERROR" "Failed to create: $target" | ||
| return 2 | ||
| fi |
There was a problem hiding this comment.
--force doesn’t overwrite existing files.
enable_ralph_in_directory avoids the early “already enabled” exit when force=true, but safe_create_file still skips existing files. That means --force leaves old PROMPT/@AGENT/@fix_plan/.ralphrc content intact, which is not the expected behavior.
🐛 Proposed fix
-safe_create_file() {
- local target=$1
- local content=$2
+safe_create_file() {
+ local target=$1
+ local content=$2
+ local force="${3:-false}"
- if [[ -f "$target" ]]; then
+ if [[ -f "$target" && "$force" != "true" ]]; then
enable_log "SKIP" "$target already exists"
return 1
fi
+ if [[ -f "$target" && "$force" == "true" ]]; then
+ enable_log "WARN" "Overwriting $target"
+ fi
@@
- safe_create_file ".ralph/PROMPT.md" "$prompt_content"
+ safe_create_file ".ralph/PROMPT.md" "$prompt_content" "$force"
@@
- safe_create_file ".ralph/@AGENT.md" "$agent_content"
+ safe_create_file ".ralph/@AGENT.md" "$agent_content" "$force"
@@
- safe_create_file ".ralph/@fix_plan.md" "$fix_plan_content"
+ safe_create_file ".ralph/@fix_plan.md" "$fix_plan_content" "$force"
@@
- safe_create_file ".ralphrc" "$ralphrc_content"
+ safe_create_file ".ralphrc" "$ralphrc_content" "$force"Also applies to: 707-775
🤖 Prompt for AI Agents
In `@lib/enable_core.sh` around lines 140 - 166, safe_create_file currently always
skips if the target exists which prevents enable_ralph_in_directory's --force
behavior from working; change safe_create_file to accept a third parameter
(e.g., force) and only return "SKIP" when the file exists and force is not true,
otherwise overwrite the file (perform the same parent-dir creation and write
logic) and log "OVERWRITE" or "SUCCESS" appropriately; update callers like
enable_ralph_in_directory to pass the force flag through when invoking
safe_create_file so --force actually replaces existing
PROMPT/@AGENT/@fix_plan/.ralphrc files.
| fetch_beads_tasks() { | ||
| local filter="${1:-status:open}" | ||
| local tasks="" | ||
|
|
||
| # Check if beads is available | ||
| if ! check_beads_available; then | ||
| return 1 | ||
| fi | ||
|
|
||
| # Try to get tasks as JSON | ||
| local json_output | ||
| if json_output=$(bd list --json 2>/dev/null); then | ||
| # Parse JSON and format as markdown tasks | ||
| if command -v jq &>/dev/null; then | ||
| tasks=$(echo "$json_output" | jq -r ' | ||
| .[] | | ||
| select(.status != "closed") | | ||
| "- [ ] [\(.id)] \(.title)" | ||
| ' 2>/dev/null) | ||
| fi | ||
| else | ||
| # Fallback: try plain text output | ||
| tasks=$(bd list 2>/dev/null | while IFS= read -r line; do | ||
| # Extract ID and title from bd list output | ||
| local id title | ||
| id=$(echo "$line" | grep -oE '^[a-z]+-[0-9]+' || echo "") | ||
| title=$(echo "$line" | sed 's/^[a-z]+-[0-9]* *//' || echo "$line") | ||
| if [[ -n "$id" ]]; then | ||
| echo "- [ ] [$id] $title" | ||
| fi | ||
| done) | ||
| fi | ||
|
|
||
| if [[ -n "$tasks" ]]; then | ||
| echo "$tasks" | ||
| return 0 | ||
| else | ||
| return 0 # Empty is not an error | ||
| fi | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "task_sources.sh" | head -20Repository: frankbria/ralph-claude-code
Length of output: 92
🏁 Script executed:
if [ -f "./lib/task_sources.sh" ]; then
wc -l ./lib/task_sources.sh
else
echo "File not found at expected path"
fiRepository: frankbria/ralph-claude-code
Length of output: 96
🏁 Script executed:
git ls-files | grep -i task_sourcesRepository: frankbria/ralph-claude-code
Length of output: 124
🏁 Script executed:
sed -n '43,82p' ./lib/task_sources.shRepository: frankbria/ralph-claude-code
Length of output: 1238
🏁 Script executed:
sed -n '150,185p' ./lib/task_sources.shRepository: frankbria/ralph-claude-code
Length of output: 892
🏁 Script executed:
sed -n '1,50p' ./lib/task_sources.shRepository: frankbria/ralph-claude-code
Length of output: 1313
🏁 Script executed:
sed -n '43,82p' ./lib/task_sources.sh | cat -nRepository: frankbria/ralph-claude-code
Length of output: 1518
🏁 Script executed:
sed -n '150,185p' ./lib/task_sources.sh | cat -nRepository: frankbria/ralph-claude-code
Length of output: 1144
🏁 Script executed:
grep -n "normalize_tasks" ./lib/task_sources.shRepository: frankbria/ralph-claude-code
Length of output: 242
🏁 Script executed:
sed -n '346,375p' ./lib/task_sources.shRepository: frankbria/ralph-claude-code
Length of output: 861
🏁 Script executed:
sed -n '355,400p' ./lib/task_sources.shRepository: frankbria/ralph-claude-code
Length of output: 1243
🏁 Script executed:
sed -n '520,550p' ./lib/task_sources.shRepository: frankbria/ralph-claude-code
Length of output: 840
🏁 Script executed:
head -20 ./lib/task_sources.shRepository: frankbria/ralph-claude-code
Length of output: 675
🏁 Script executed:
grep -A5 "import_tasks_from_sources" ./lib/task_sources.sh | head -30Repository: frankbria/ralph-claude-code
Length of output: 520
Task import can silently drop results if jq is unavailable; beads filter is ignored.
Both fetch_beads_tasks and fetch_github_tasks rely on jq for JSON parsing without a fallback if jq is missing. In fetch_beads_tasks, the fallback to plain text parsing only triggers if the bd list --json command itself fails, not if jq is unavailable. If the JSON call succeeds but jq is not installed, tasks silently becomes empty. This violates the coding guideline requiring JSON fallback to text parsing. Additionally, the filter parameter in fetch_beads_tasks is documented but never applied to the bd query, so caller-supplied filters are ignored.
Restructure both functions to check if tasks are empty after attempting JSON parsing (whether due to missing jq or parse failure) and fall back to text output. Apply the filter parameter to the beads query or post-filter step.
🛠️ Suggested fix for fetch_beads_tasks (apply same pattern to fetch_github_tasks)
- if json_output=$(bd list --json 2>/dev/null); then
- # Parse JSON and format as markdown tasks
- if command -v jq &>/dev/null; then
- tasks=$(echo "$json_output" | jq -r '
- .[] |
- select(.status != "closed") |
- "- [ ] [\(.id)] \(.title)"
- ' 2>/dev/null)
- fi
- else
- # Fallback: try plain text output
- tasks=$(bd list 2>/dev/null | while IFS= read -r line; do
+ if json_output=$(bd list --json 2>/dev/null); then
+ # Parse JSON and format as markdown tasks
+ if command -v jq &>/dev/null; then
+ tasks=$(echo "$json_output" | jq -r '
+ .[] |
+ select(.status != "closed") |
+ "- [ ] [\(.id)] \(.title)"
+ ' 2>/dev/null)
+ fi
+ fi
+
+ # Fallback: try plain text output if JSON parse is unavailable/empty
+ if [[ -z "$tasks" ]]; then
+ tasks=$(bd list 2>/dev/null | while IFS= read -r line; do
# Extract ID and title from bd list output
local id title
id=$(echo "$line" | grep -oE '^[a-z]+-[0-9]+' || echo "")
title=$(echo "$line" | sed 's/^[a-z]+-[0-9]* *//' || echo "$line")
if [[ -n "$id" ]]; then
echo "- [ ] [$id] $title"
fi
done)
fi🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 44-44: filter appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
In `@lib/task_sources.sh` around lines 43 - 82, fetch_beads_tasks currently runs
bd list --json and only falls back to plain-text if the bd command fails, so if
jq is missing or JSON parsing yields no tasks they get silently dropped; also
the local filter parameter is never applied. Update fetch_beads_tasks to (1)
include the filter when invoking bd (e.g., pass "$filter" to bd list or build
args from filter) or apply the filter in the jq/select step, (2) after capturing
json_output, attempt to parse it with jq only if jq is present, but if jq is
missing or parsing produces an empty $tasks, run the plain-text fallback (bd
list with the same filter) and parse lines to produce the markdown task lines;
mirror this same pattern and fallback behavior in fetch_github_tasks so missing
jq or parse failures never drop results.
| # Import from beads | ||
| if echo "$sources" | grep -qw "beads"; then | ||
| local beads_tasks | ||
| if beads_tasks=$(fetch_beads_tasks); then | ||
| if [[ -n "$beads_tasks" ]]; then | ||
| all_tasks="${all_tasks} | ||
| # Tasks from beads | ||
| ${beads_tasks} | ||
| " | ||
| ((source_count++)) | ||
| fi | ||
| fi | ||
| fi | ||
|
|
||
| # Import from GitHub | ||
| if echo "$sources" | grep -qw "github"; then | ||
| local github_tasks | ||
| if github_tasks=$(fetch_github_tasks "$github_label"); then | ||
| if [[ -n "$github_tasks" ]]; then | ||
| all_tasks="${all_tasks} | ||
| # Tasks from GitHub | ||
| ${github_tasks} | ||
| " | ||
| ((source_count++)) | ||
| fi | ||
| fi | ||
| fi | ||
|
|
||
| # Import from PRD | ||
| if echo "$sources" | grep -qw "prd"; then | ||
| if [[ -n "$prd_file" && -f "$prd_file" ]]; then | ||
| local prd_tasks | ||
| if prd_tasks=$(extract_prd_tasks "$prd_file"); then | ||
| if [[ -n "$prd_tasks" ]]; then | ||
| all_tasks="${all_tasks} | ||
| # Tasks from PRD | ||
| ${prd_tasks} | ||
| " | ||
| ((source_count++)) | ||
| fi | ||
| fi | ||
| fi | ||
| fi | ||
|
|
||
| if [[ -z "$all_tasks" ]]; then | ||
| return 1 | ||
| fi | ||
|
|
||
| # Normalize and output | ||
| normalize_tasks "$all_tasks" "combined" | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
Source headings are converted into tasks during normalization.
normalize_tasks treats every line as a task, so # Tasks from beads/GitHub/PRD headings become checkbox items. This loses source separation and adds noisy tasks. Consider normalizing each source before concatenation (or skipping normalization for heading lines).
♻️ Suggested adjustment
- if beads_tasks=$(fetch_beads_tasks); then
+ if beads_tasks=$(fetch_beads_tasks); then
+ beads_tasks=$(normalize_tasks "$beads_tasks" "beads")
if [[ -n "$beads_tasks" ]]; then
all_tasks="${all_tasks}
# Tasks from beads
${beads_tasks}
"
((source_count++))
fi
fi
...
- # Normalize and output
- normalize_tasks "$all_tasks" "combined"
+ # Already normalized per-source; keep headings intact
+ echo "$all_tasks"🤖 Prompt for AI Agents
In `@lib/task_sources.sh` around lines 482 - 533, The header lines like "# Tasks
from beads" are being treated as tasks because normalize_tasks treats every line
as a task; fix by normalizing each source's raw output separately and appending
only the normalized results (or strip header lines before normalization).
Specifically, for each fetch function (fetch_beads_tasks -> beads_tasks,
fetch_github_tasks -> github_tasks, extract_prd_tasks -> prd_tasks) call
normalize_tasks on that string and then append the normalized output to
all_tasks (increment source_count as before) instead of inserting literal "#
Tasks ..." headings; alternatively remove any leading '#' comment lines from
each source before calling normalize_tasks. Ensure you update the code paths
that currently build all_tasks and still use source_count, and keep the final
normalize_tasks "$all_tasks" "combined" call only if you intend a second pass
normalization.
| # Verify required files exist | ||
| if [[ ! -f ".ralph/PROMPT.md" ]] || [[ ! -f ".ralph/@fix_plan.md" ]]; then | ||
| output_error "Required files were not created" | ||
| exit $ENABLE_ERROR | ||
| fi |
There was a problem hiding this comment.
Consider including @AGENT.md in required files verification.
The verification only checks for .ralph/PROMPT.md and .ralph/@fix_plan.md, but check_existing_ralph in enable_core.sh considers .ralph/@AGENT.md as a required file. This inconsistency could lead to success being reported when the configuration is actually incomplete.
🐛 Proposed fix
# Verify required files exist
- if [[ ! -f ".ralph/PROMPT.md" ]] || [[ ! -f ".ralph/@fix_plan.md" ]]; then
+ if [[ ! -f ".ralph/PROMPT.md" ]] || [[ ! -f ".ralph/@fix_plan.md" ]] || [[ ! -f ".ralph/@AGENT.md" ]]; then
output_error "Required files were not created"
exit $ENABLE_ERROR
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Verify required files exist | |
| if [[ ! -f ".ralph/PROMPT.md" ]] || [[ ! -f ".ralph/@fix_plan.md" ]]; then | |
| output_error "Required files were not created" | |
| exit $ENABLE_ERROR | |
| fi | |
| # Verify required files exist | |
| if [[ ! -f ".ralph/PROMPT.md" ]] || [[ ! -f ".ralph/@fix_plan.md" ]] || [[ ! -f ".ralph/@AGENT.md" ]]; then | |
| output_error "Required files were not created" | |
| exit $ENABLE_ERROR | |
| fi |
🤖 Prompt for AI Agents
In `@ralph_enable_ci.sh` around lines 381 - 385, The current post-generation
verification in ralph_enable_ci.sh only checks for ".ralph/PROMPT.md" and
".ralph/@fix_plan.md" but check_existing_ralph (in enable_core.sh) also treats
".ralph/@AGENT.md" as required; update the verification block in
ralph_enable_ci.sh to include ".ralph/@AGENT.md" so the script fails when the
agent file is missing, ensuring consistency with check_existing_ralph.
Fixes from PR #124 review: 1. sed -i portability (ralph_enable.sh:456) - Use portable sed + mv pattern instead of GNU-only sed -i 2. sed regex portability (lib/task_sources.sh) - Replace \s with POSIX [[:space:]] character class - Add sed -E flag for extended regex 3. jq availability check (ralph_enable_ci.sh:177) - Add check for jq when --json flag is used 4. Unused filter parameter (lib/task_sources.sh:44) - Pass filter to bd list --filter command 5. Word-splitting in select_multiple (ralph_enable.sh:322) - Return comma-separated indices instead of space-separated text - Update caller to parse indices correctly 6. Missing || true for check_existing_ralph (ralph_enable.sh:185) - Prevent set -e from exiting on non-zero return 7. select_multiple stdout corruption (lib/wizard_utils.sh) - Redirect interactive output to stderr - Only final result goes to stdout 8. Color variables not exported (lib/wizard_utils.sh:12) - Export WIZARD_* color variables for subshells 9. select_option infinite loop (lib/wizard_utils.sh:179) - Add guard for empty options array
Critical Bug: .ralphrc Configuration Not WorkingIssueThe configuration file feature (introduced in this PR) is completely broken. Configuration values from are immediately overwritten by defaults. Root CauseIn , the function attempts to save and restore environment variable overrides: The problem: Defaults are set at line 24 () before is called. This means is never empty, so line 130 always restores the default 100, wiping out whatever set. This affects all configuration variables:ImpactThe entire configuration system is non-functional. Users cannot:
This defeats the purpose of adding support in the first place. FixTrack which variables were explicitly set by user/environment (vs defaults): Then set when CLI args or environment variables explicitly set values (e.g., in the handler and wherever env variables are parsed). Other Issues Already Addressed✅ macOS compatibility (addressed in commit d84c83e) SummaryThis is a must-fix issue that makes a core feature ( configuration) completely non-functional. The test suite (396 tests) doesn't catch this because it likely doesn't test the loading behavior end-to-end with actual configuration files. Otherwise, the PR is well-designed with excellent architecture and comprehensive test coverage. The sed injection vulnerabilities and jq dependency checks are also critical fixes identified by other reviewers. |
| local project_name="$1" | ||
| local project_type="$2" | ||
|
|
||
| if [[ "$OUTPUT_JSON" == "true" ]]; then |
There was a problem hiding this comment.
$project_name and $project_type are interpolated directly into JSON without escaping. If either contains quotes, backslashes, or newlines, the JSON output will be malformed. Consider using jq to safely construct the JSON object (since jq is already required for --json mode).
🚀 Want me to fix this? Reply ex: "fix it for me".
| if [[ -n "$2" && ! "$2" =~ ^-- ]]; then | ||
| TASK_SOURCE="$2" | ||
| shift 2 |
There was a problem hiding this comment.
The --from option accepts any value without validating it's one of beads, github, or prd. Consider adding validation to fail early with a clear error message for invalid sources.
- if [[ -n "$2" && ! "$2" =~ ^-- ]]; then
- TASK_SOURCE="$2"
- shift 2
+ if [[ -n "$2" && ! "$2" =~ ^-- ]]; then
+ if [[ ! "$2" =~ ^(beads|github|prd)$ ]]; then
+ echo "Error: Invalid source '$2'. Must be one of: beads, github, prd"
+ exit 1
+ fi
+ TASK_SOURCE="$2"
+ shift 2🚀 Want me to fix this? Reply ex: "fix it for me".
| exit $ENABLE_ALREADY_ENABLED | ||
| fi |
There was a problem hiding this comment.
After the user confirms continuation at line 196, FORCE_OVERWRITE should be set to true. Otherwise, enable_ralph_in_directory will still detect the complete state and return ENABLE_ALREADY_ENABLED, causing the "Failed to enable Ralph" error despite user confirmation.
| exit $ENABLE_ALREADY_ENABLED | |
| fi | |
| echo "Exiting. Use --force to overwrite." | |
| exit $ENABLE_ALREADY_ENABLED | |
| fi | |
| FORCE_OVERWRITE=true |
🚀 Want me to fix this? Reply ex: "fix it for me".
Code Review - Ralph Enable Feature (v0.11.0)I've conducted a thorough review of this PR. Overall, this is an excellent addition to Ralph with comprehensive test coverage and well-structured code. Strengths
Issues to Address1. CRITICAL: --force Flag Not Working ProperlyThe safe_create_file() function in lib/enable_core.sh:164 always skips existing files, which breaks the --force override behavior. Add a force parameter to safe_create_file() and update callers to pass it through. 2. Task Import Header Lines Treated as TasksIn lib/task_sources.sh:517-518, comment headers like 'Tasks from beads' may get normalized as actual tasks. Filter these out in normalize_tasks(). 3. Missing jq Fallback in fetch_beads_tasksIn lib/task_sources.sh:56-62, if jq is missing, tasks remains empty. Add fallback to plain-text parsing when jq is unavailable. Same issue in fetch_github_tasks(). 4. Incomplete File Verification in ralph_enable_ci.shIn ralph_enable_ci.sh:381-385, only PROMPT.md and @fix_plan.md are checked. Add verification for @AGENT.md to match check_existing_ralph(). Suggestions (Non-blocking)
Security Assessment: GOOD
RecommendationAPPROVE with minor fixes required for the 4 issues above. The core implementation is solid, test coverage is excellent, and this adds significant value to Ralph. Once the critical --force flag issue and verification gaps are fixed, this is ready to merge. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/task_sources.sh`:
- Around line 209-214: The fallback branch ignores the label filter by calling a
hardcoded "gh issue list --state open"; change it to reuse the same gh_args
array so the label/state filters are preserved: run gh with "${gh_args[@]}"
(redirecting stderr to /dev/null) and pipe that output to wc -l (and tr -d ' '
if desired) to compute count; update the fallback that sets the local variable
count to call gh "${gh_args[@]}" instead of the hardcoded command so the label
filter applied earlier is honored (refer to the gh_args array and the count
variable).
- Around line 150-185: The function fetch_github_tasks currently only parses
GitHub JSON via jq, so when jq is missing tasks stays empty; add a fallback
JSON-to-text parser for json_output inside fetch_github_tasks so it behaves like
fetch_beads_tasks. Specifically, after capturing json_output (and checking
command -v jq), if jq is unavailable parse json_output with shell text tools
(grep/sed/awk or perl) to extract .number and .title from the JSON and build
lines like "- [ ] [#<number>] <title>" assigned to tasks; keep existing behavior
of adding label via gh_args and returning non-zero on gh failure (symbols:
fetch_github_tasks, check_github_available, gh_args, json_output, tasks). Ensure
fallback handles escaping/quotes and still prints tasks when non-empty and
returns 0.
♻️ Duplicate comments (3)
lib/task_sources.sh (2)
43-82: jq unavailability still causes silent task loss.When
bd list --jsonsucceeds butjqis unavailable,tasksremains empty and no fallback occurs. Theelsebranch only triggers whenbd list --jsonfails, not when jq parsing is skipped/fails. This was flagged in a prior review.🐛 Proposed fix to ensure fallback when jq is unavailable
fetch_beads_tasks() { local filter="${1:-status:open}" local tasks="" # Check if beads is available if ! check_beads_available; then return 1 fi # Try to get tasks as JSON (pass filter if provided) local json_output if json_output=$(bd list --json --filter "$filter" 2>/dev/null); then # Parse JSON and format as markdown tasks if command -v jq &>/dev/null; then tasks=$(echo "$json_output" | jq -r ' .[] | select(.status != "closed") | "- [ ] [\(.id)] \(.title)" ' 2>/dev/null) fi - else - # Fallback: try plain text output + fi + + # Fallback: try plain text output if JSON parsing unavailable/empty + if [[ -z "$tasks" ]]; then tasks=$(bd list 2>/dev/null | while IFS= read -r line; do # Extract ID and title from bd list output local id title id=$(echo "$line" | grep -oE '^[a-z]+-[0-9]+' || echo "") title=$(echo "$line" | sed 's/^[a-z]+-[0-9]* *//' || echo "$line") if [[ -n "$id" ]]; then echo "- [ ] [$id] $title" fi done) fi if [[ -n "$tasks" ]]; then echo "$tasks" return 0 else return 0 # Empty is not an error fi }
482-533: Source headings converted into tasks during normalization.The
# Tasks from beads/GitHub/PRDheadings added at lines 488, 502, and 517 are passed tonormalize_tasks, which converts every non-empty line into a checkbox. This creates spurious task entries like- [ ] # Tasks from beads. This was flagged in a prior review.🐛 Proposed fix: normalize each source separately
# Import from beads if echo "$sources" | grep -qw "beads"; then local beads_tasks if beads_tasks=$(fetch_beads_tasks); then if [[ -n "$beads_tasks" ]]; then + local normalized_beads + normalized_beads=$(normalize_tasks "$beads_tasks" "beads") all_tasks="${all_tasks} # Tasks from beads -${beads_tasks} +${normalized_beads} " ((source_count++)) fi fi fi # ... similar for github and prd ... - # Normalize and output - normalize_tasks "$all_tasks" "combined" + # Output already-normalized tasks with preserved headings + echo "$all_tasks" | grep -v '^$' return 0 }ralph_enable_ci.sh (1)
385-389: Verification missing check for@AGENT.md.The verification at lines 386-388 only checks for
.ralph/PROMPT.mdand.ralph/@fix_plan.md, butcheck_existing_ralphinenable_core.shconsiders.ralph/@AGENT.mdas a required file. This inconsistency could report success when the configuration is incomplete. This was flagged in a prior review but appears unaddressed.🐛 Proposed fix
# Verify required files exist - if [[ ! -f ".ralph/PROMPT.md" ]] || [[ ! -f ".ralph/@fix_plan.md" ]]; then + if [[ ! -f ".ralph/PROMPT.md" ]] || [[ ! -f ".ralph/@fix_plan.md" ]] || [[ ! -f ".ralph/@AGENT.md" ]]; then output_error "Required files were not created" exit $ENABLE_ERROR fi
🧹 Nitpick comments (3)
lib/task_sources.sh (3)
284-294: Heading extraction loop is dead code.The
section_namevariable is assigned but never used. The loop iterates through headings without producing any output or side effects. Consider removing this block or implementing the intended functionality.♻️ Proposed removal of dead code
- # Look for headings that might be task sections - local headings - headings=$(grep -E '^#{1,3}[[:space:]]+(TODO|Tasks|Requirements|Features|Backlog|Sprint)' "$prd_file" 2>/dev/null) - if [[ -n "$headings" ]]; then - # Extract content after these headings as potential tasks - while IFS= read -r heading; do - local section_name - section_name=$(echo "$heading" | sed -E 's/^#*[[:space:]]*//') - # This is informational - actual task extraction would need more context - done <<< "$headings" - fi
318-340: Unusedoutput_dirparameter and counter-intuitive return value.The
output_dirparameter is declared but never used. Additionally, returning1whenralph-importexists (line 335) is confusing—consider documenting this more clearly in the function header or refactoring the logic.♻️ Suggested improvements
# convert_prd_with_claude - Full PRD conversion using Claude (calls ralph-import logic) # # Parameters: # $1 (prd_file) - Path to the PRD file -# $2 (output_dir) - Directory to output converted files (optional, defaults to .ralph/) +# $2 (output_dir) - Reserved for future use (currently unused) # -# Outputs: -# Sets CONVERTED_PROMPT_FILE, CONVERTED_FIX_PLAN_FILE, CONVERTED_SPECS_FILE -# # Returns: -# 0 - Success -# 1 - Error +# 0 - Basic extraction performed (fallback) +# 1 - Full conversion available but not performed (caller should use ralph-import directly) # convert_prd_with_claude() { local prd_file=$1 - local output_dir="${2:-.ralph}" + # shellcheck disable=SC2034 + local output_dir="${2:-.ralph}" # Reserved for future use
355-358: Unusedsourceparameter in normalize_tasks.The
sourceparameter is declared but never used within the function. If source attribution is planned, consider implementing it; otherwise, remove the parameter to avoid confusion.
| fetch_github_tasks() { | ||
| local label="${1:-}" | ||
| local limit="${2:-50}" | ||
| local tasks="" | ||
|
|
||
| # Check if GitHub is available | ||
| if ! check_github_available; then | ||
| return 1 | ||
| fi | ||
|
|
||
| # Build gh command | ||
| local gh_args=("issue" "list" "--state" "open" "--limit" "$limit" "--json" "number,title,labels") | ||
| if [[ -n "$label" ]]; then | ||
| gh_args+=("--label" "$label") | ||
| fi | ||
|
|
||
| # Fetch issues | ||
| local json_output | ||
| if ! json_output=$(gh "${gh_args[@]}" 2>/dev/null); then | ||
| return 1 | ||
| fi | ||
|
|
||
| # Parse JSON and format as markdown tasks | ||
| if command -v jq &>/dev/null; then | ||
| tasks=$(echo "$json_output" | jq -r ' | ||
| .[] | | ||
| "- [ ] [#\(.number)] \(.title)" | ||
| ' 2>/dev/null) | ||
| fi | ||
|
|
||
| if [[ -n "$tasks" ]]; then | ||
| echo "$tasks" | ||
| fi | ||
|
|
||
| return 0 | ||
| } |
There was a problem hiding this comment.
No fallback when jq is unavailable for GitHub tasks.
Similar to fetch_beads_tasks, if jq is not installed, tasks remains empty and no error/warning is raised. Per coding guidelines, implement JSON fallback to text parsing.
🐛 Proposed fix
# Parse JSON and format as markdown tasks
if command -v jq &>/dev/null; then
tasks=$(echo "$json_output" | jq -r '
.[] |
"- [ ] [#\(.number)] \(.title)"
' 2>/dev/null)
+ else
+ # Fallback: parse JSON output without jq using basic pattern matching
+ # Note: This is a best-effort fallback; jq is recommended
+ tasks=$(echo "$json_output" | grep -oE '"number":[0-9]+|"title":"[^"]*"' | \
+ paste - - | sed -E 's/"number":([0-9]+).*"title":"([^"]*)"/- [ ] [#\1] \2/')
fi🤖 Prompt for AI Agents
In `@lib/task_sources.sh` around lines 150 - 185, The function fetch_github_tasks
currently only parses GitHub JSON via jq, so when jq is missing tasks stays
empty; add a fallback JSON-to-text parser for json_output inside
fetch_github_tasks so it behaves like fetch_beads_tasks. Specifically, after
capturing json_output (and checking command -v jq), if jq is unavailable parse
json_output with shell text tools (grep/sed/awk or perl) to extract .number and
.title from the JSON and build lines like "- [ ] [#<number>] <title>" assigned
to tasks; keep existing behavior of adding label via gh_args and returning
non-zero on gh failure (symbols: fetch_github_tasks, check_github_available,
gh_args, json_output, tasks). Ensure fallback handles escaping/quotes and still
prints tasks when non-empty and returns 0.
| local count | ||
| if command -v jq &>/dev/null; then | ||
| count=$(gh "${gh_args[@]}" 2>/dev/null | jq 'length' 2>/dev/null || echo "0") | ||
| else | ||
| count=$(gh issue list --state open 2>/dev/null | wc -l | tr -d ' ') | ||
| fi |
There was a problem hiding this comment.
Fallback count ignores label filter.
When jq is unavailable, the fallback uses gh issue list --state open without the --label filter, returning a count that may differ from the filtered count.
🐛 Proposed fix
local count
if command -v jq &>/dev/null; then
count=$(gh "${gh_args[@]}" 2>/dev/null | jq 'length' 2>/dev/null || echo "0")
else
- count=$(gh issue list --state open 2>/dev/null | wc -l | tr -d ' ')
+ # Use same args but without --json for text output
+ local text_args=("issue" "list" "--state" "open")
+ [[ -n "$label" ]] && text_args+=("--label" "$label")
+ count=$(gh "${text_args[@]}" 2>/dev/null | wc -l | tr -d ' ')
fi🤖 Prompt for AI Agents
In `@lib/task_sources.sh` around lines 209 - 214, The fallback branch ignores the
label filter by calling a hardcoded "gh issue list --state open"; change it to
reuse the same gh_args array so the label/state filters are preserved: run gh
with "${gh_args[@]}" (redirecting stderr to /dev/null) and pipe that output to
wc -l (and tr -d ' ' if desired) to compute count; update the fallback that sets
the local variable count to call gh "${gh_args[@]}" instead of the hardcoded
command so the label filter applied earlier is honored (refer to the gh_args
array and the count variable).
- Add RESPONSE_ANALYSIS_FILE export to test_session_continuity.bats setup - Add mock ralph_enable.sh and ralph_enable_ci.sh to test_installation.bats - Add mock lib files: enable_core.sh, wizard_utils.sh, task_sources.sh, timeout_utils.sh All 396 tests now pass.
| local default_index=$2 | ||
| shift 2 | ||
| local options=("$@") | ||
| local num_options=${#options[@]} |
There was a problem hiding this comment.
Suggestion: Add input guards in wizard_utils.sh functions to validate parameters before computing, avoiding errors from invalid defaults or zero totals.
local num_options=${#options[@]}
+
+ # Validate default_index
+ if [[ $default_index -lt 1 || $default_index -gt $num_options ]]; then
+ echo "" >&2
+ return 1
+ fi🚀 Want me to fix this? Reply ex: "fix it for me".
Code Review SummaryThis PR adds excellent functionality for onboarding existing projects to Ralph with comprehensive test coverage (75 new tests, 100% pass rate). The modular architecture and user experience are well-designed. However, two critical bugs remain that must be fixed before merge: 🔴 Critical Issues1. --force flag completely broken (lib/enable_core.sh)doesn't accept a force parameter, so bypasses the early check but then silently skips all file creation. Location: Fix: Add force parameter and honor it: 2. .ralphrc configuration completely non-functional (ralph_loop.sh)The entire feature is broken because the save/restore logic always restores default values, ignoring contents. Location: Fix: Track which variables were explicitly overridden by CLI/env: Do the same for all 6 variables: CLAUDE_TIMEOUT_MINUTES, CLAUDE_OUTPUT_FORMAT, CLAUDE_ALLOWED_TOOLS, CLAUDE_USE_CONTINUE, CLAUDE_SESSION_EXPIRY_HOURS, VERBOSE_PROGRESS. ✅ Issues Already Addressed
📋 Overall AssessmentCode Quality: 4/5 Status: These two bugs completely break the new configuration system and the flag. Once fixed, this PR will be ready to merge and provides significant value for Ralph adoption. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/integration/test_installation.bats (2)
468-490: Uninstall test should verify removal of new enable commands.This test verifies removal of the original five commands but doesn't verify that
ralph-enableandralph-enable-ciare also removed during uninstall.Suggested fix
# Verify command files are removed assert_file_not_exists "$TEST_INSTALL_DIR/ralph" assert_file_not_exists "$TEST_INSTALL_DIR/ralph-monitor" assert_file_not_exists "$TEST_INSTALL_DIR/ralph-setup" assert_file_not_exists "$TEST_INSTALL_DIR/ralph-import" assert_file_not_exists "$TEST_INSTALL_DIR/ralph-migrate" + assert_file_not_exists "$TEST_INSTALL_DIR/ralph-enable" + assert_file_not_exists "$TEST_INSTALL_DIR/ralph-enable-ci"
539-589: End-to-end test should verify complete enable functionality installation.This comprehensive test verifies the complete installation workflow but doesn't include assertions for the new enable scripts and library files that are part of v0.11.0.
Suggested additions (after line 580)
assert_file_exists "$TEST_RALPH_HOME/ralph_import.sh" assert_file_exists "$TEST_RALPH_HOME/migrate_to_ralph_folder.sh" + assert_file_exists "$TEST_RALPH_HOME/ralph_enable.sh" + assert_file_exists "$TEST_RALPH_HOME/ralph_enable_ci.sh" + + # Verify enable-related lib files + assert_file_exists "$TEST_RALPH_HOME/lib/enable_core.sh" + assert_file_exists "$TEST_RALPH_HOME/lib/wizard_utils.sh" + assert_file_exists "$TEST_RALPH_HOME/lib/task_sources.sh" + + # Verify enable commands installed + assert_file_exists "$TEST_INSTALL_DIR/ralph-enable" + assert_file_exists "$TEST_INSTALL_DIR/ralph-enable-ci"
🤖 Fix all issues with AI agents
In `@tests/integration/test_installation.bats`:
- Around line 85-119: Add assertions in the specified tests to verify the new
enable commands and libs: in the test "install.sh creates ~/.local/bin commands"
assert that the installed command stubs ralph-enable and ralph-enable-ci exist
in the bin install location (same style as existing checks for other commands);
in "install.sh sets executable permissions" add permission checks for
ralph-enable and ralph-enable-ci in both $TEST_INSTALL_DIR and $TEST_RALPH_HOME
(matching how other scripts are checked); in "install.sh uninstall removes all
files" assert that ralph-enable and ralph-enable-ci are removed after uninstall;
and in "complete installation workflow end-to-end" assert that ralph-enable,
ralph-enable-ci and the new lib files (enable_core.sh, wizard_utils.sh,
task_sources.sh, timeout_utils.sh) are present in the installed lib directory.
Ensure you follow the existing test assertion helpers and naming conventions
used elsewhere in the file.
🧹 Nitpick comments (1)
tests/integration/test_installation.bats (1)
13-124: Consider adding a comment explaining the test strategy for enable mocks.Per coding guidelines, complex test scenarios should include comments explaining the test strategy. A brief comment above the enable-related mocks (around line 85) would help clarify that these mocks are added to support testing the v0.11.0 enable functionality without invoking the actual wizard behavior.
Suggested comment
EOF + # ========================================================================= + # Mocks for v0.11.0 enable functionality (ralph-enable wizard/CI commands) + # These mocks allow installation tests to verify enable scripts are deployed + # without triggering actual interactive or GitHub/PRD integration behavior. + # ========================================================================= cat > "$MOCK_SOURCE_DIR/ralph_enable.sh" << 'EOF'Based on coding guidelines requiring comments for complex test strategies.
| cat > "$MOCK_SOURCE_DIR/ralph_enable.sh" << 'EOF' | ||
| #!/bin/bash | ||
| # Mock ralph_enable.sh | ||
| echo "Ralph enable running" | ||
| EOF | ||
|
|
||
| cat > "$MOCK_SOURCE_DIR/ralph_enable_ci.sh" << 'EOF' | ||
| #!/bin/bash | ||
| # Mock ralph_enable_ci.sh | ||
| echo "Ralph enable CI running" | ||
| EOF | ||
|
|
||
| # Create mock lib files for new enable functionality | ||
| cat > "$MOCK_SOURCE_DIR/lib/enable_core.sh" << 'EOF' | ||
| #!/bin/bash | ||
| # Mock enable_core.sh | ||
| check_existing_ralph() { :; } | ||
| EOF | ||
|
|
||
| cat > "$MOCK_SOURCE_DIR/lib/wizard_utils.sh" << 'EOF' | ||
| #!/bin/bash | ||
| # Mock wizard_utils.sh | ||
| confirm() { :; } | ||
| EOF | ||
|
|
||
| cat > "$MOCK_SOURCE_DIR/lib/task_sources.sh" << 'EOF' | ||
| #!/bin/bash | ||
| # Mock task_sources.sh | ||
| fetch_beads_tasks() { :; } | ||
| EOF | ||
|
|
||
| cat > "$MOCK_SOURCE_DIR/lib/timeout_utils.sh" << 'EOF' | ||
| #!/bin/bash | ||
| # Mock timeout_utils.sh | ||
| portable_timeout() { timeout "$@"; } |
There was a problem hiding this comment.
Mocks for enable functionality look good, but test assertions are missing.
The mock scripts for ralph_enable.sh, ralph_enable_ci.sh, and the new library files (enable_core.sh, wizard_utils.sh, task_sources.sh, timeout_utils.sh) are correctly structured and follow the existing pattern.
However, while the mocks are in place to support testing the enable functionality, the test assertions in this file haven't been updated to verify that the new commands (ralph-enable, ralph-enable-ci) are actually installed. Specifically:
- Test "install.sh creates ~/.local/bin commands" (line 198) should assert
ralph-enableandralph-enable-ciexist. - Test "install.sh sets executable permissions" (line 216) should verify permissions for enable scripts in both
$TEST_INSTALL_DIRand$TEST_RALPH_HOME. - Test "install.sh uninstall removes all files" (line 468) should verify enable commands are removed.
- Test "complete installation workflow end-to-end" (line 539) should verify enable scripts and lib files are installed.
Suggested additions for test "install.sh creates ~/.local/bin commands" (around line 206)
assert_file_exists "$TEST_INSTALL_DIR/ralph-import"
assert_file_exists "$TEST_INSTALL_DIR/ralph-migrate"
+ assert_file_exists "$TEST_INSTALL_DIR/ralph-enable"
+ assert_file_exists "$TEST_INSTALL_DIR/ralph-enable-ci"
# Verify each command contains proper shebang
grep -q "#!/bin/bash" "$TEST_INSTALL_DIR/ralph"
grep -q "#!/bin/bash" "$TEST_INSTALL_DIR/ralph-monitor"
grep -q "#!/bin/bash" "$TEST_INSTALL_DIR/ralph-setup"
grep -q "#!/bin/bash" "$TEST_INSTALL_DIR/ralph-import"
grep -q "#!/bin/bash" "$TEST_INSTALL_DIR/ralph-migrate"
+ grep -q "#!/bin/bash" "$TEST_INSTALL_DIR/ralph-enable"
+ grep -q "#!/bin/bash" "$TEST_INSTALL_DIR/ralph-enable-ci"Suggested additions for test "install.sh sets executable permissions" (around line 231)
[[ -x "$TEST_RALPH_HOME/ralph_import.sh" ]]
[[ -x "$TEST_RALPH_HOME/migrate_to_ralph_folder.sh" ]]
+ [[ -x "$TEST_RALPH_HOME/ralph_enable.sh" ]]
+ [[ -x "$TEST_RALPH_HOME/ralph_enable_ci.sh" ]]
# Verify lib scripts are executable
[[ -x "$TEST_RALPH_HOME/lib/circuit_breaker.sh" ]]
[[ -x "$TEST_RALPH_HOME/lib/response_analyzer.sh" ]]
[[ -x "$TEST_RALPH_HOME/lib/date_utils.sh" ]]
+ [[ -x "$TEST_RALPH_HOME/lib/enable_core.sh" ]]
+ [[ -x "$TEST_RALPH_HOME/lib/wizard_utils.sh" ]]
+ [[ -x "$TEST_RALPH_HOME/lib/task_sources.sh" ]]🤖 Prompt for AI Agents
In `@tests/integration/test_installation.bats` around lines 85 - 119, Add
assertions in the specified tests to verify the new enable commands and libs: in
the test "install.sh creates ~/.local/bin commands" assert that the installed
command stubs ralph-enable and ralph-enable-ci exist in the bin install location
(same style as existing checks for other commands); in "install.sh sets
executable permissions" add permission checks for ralph-enable and
ralph-enable-ci in both $TEST_INSTALL_DIR and $TEST_RALPH_HOME (matching how
other scripts are checked); in "install.sh uninstall removes all files" assert
that ralph-enable and ralph-enable-ci are removed after uninstall; and in
"complete installation workflow end-to-end" assert that ralph-enable,
ralph-enable-ci and the new lib files (enable_core.sh, wizard_utils.sh,
task_sources.sh, timeout_utils.sh) are present in the installed lib directory.
Ensure you follow the existing test assertion helpers and naming conventions
used elsewhere in the file.
|
Posted review comment to PR #124: #124 (comment) |
Code Review - PR 124: Ralph Enable Wizard (v0.11.0)This is a well-architected feature that significantly improves Ralph's usability for existing projects. The implementation demonstrates strong engineering practices with 75 new tests, modular design, and comprehensive documentation. However, there are critical issues that must be addressed before merge. Critical Issues (Must Fix Before Merge)1. .ralphrc Configuration System is Broken (ralph_loop.sh:97-140)Issue: The configuration file loading logic has a fundamental flaw that makes .ralphrc settings ineffective. Root Cause: At line 24, defaults are set (MAX_CALLS_PER_HOUR=100), then at line 97-110, these variables are saved to restore later. Since defaults are set at line 24, saved_MAX_CALLS_PER_HOUR is never empty - it's always 100. Line 130 then always overwrites whatever .ralphrc set, restoring the default. Impact: ALL configuration from .ralphrc is immediately discarded: MAX_CALLS_PER_HOUR, CLAUDE_TIMEOUT_MINUTES, CLAUDE_OUTPUT_FORMAT, CLAUDE_ALLOWED_TOOLS, CLAUDE_USE_CONTINUE, VERBOSE_PROGRESS. This defeats the entire purpose of adding .ralphrc support in this PR. Fix: Track which variables were explicitly set by environment/CLI vs defaulted, or move the save logic to before defaults are set in the main script. 2. sed Command Injection Risk (ralph_enable.sh:454, 458)Issue: Using unescaped user input in sed substitution commands. Special characters break sed: --label "ralph/task" breaks (/ is sed delimiter), --label "test&replace" (& has special meaning in sed replacement). Fix: Use a different delimiter like | or properly escape special characters. 3. Missing jq Validation for --json Flag (ralph_enable_ci.sh:174-180)Issue: The check happens AFTER setting OUTPUT_JSON=true. Better approach is to move jq check to main execution flow after argument parsing to allow proper error handling. High Priority Issues4. Shell Injection Risk in safe_create_file() (lib/enable_core.sh:160)Issue: Using echo for file writing has edge cases. Content starting with -e, -n, -E flags may be interpreted. Use printf for predictable output. 5. Inconsistent Error Codes in ralph_enable_ci.shIssue: Documentation says 0=Success, 1=Error, 2=Already enabled, but code uses exit 1 for various error types. Recommend adding specific error codes for different failure types (3=Invalid arguments, 4=Missing dependencies). Strengths (Great Work!)
RecommendationDo Not Merge until issue No. 1 (.ralphrc configuration broken) is fixed. This is a fundamental flaw that breaks the entire feature. Issues No. 2 and No. 3 should also be addressed before merge as they represent security and reliability concerns. Issues No. 4-5 can be addressed in a follow-up PR if needed, but fixing them now would be better. SummaryThis PR adds significant value to Ralph and demonstrates excellent engineering practices. However, the .ralphrc loading bug is critical and must be fixed. Once that's addressed, along with the sed injection and jq validation issues, this will be a solid addition. Great work on the comprehensive testing and documentation! The modular architecture is particularly well done. Files reviewed: ralph_enable.sh, ralph_enable_ci.sh, lib/enable_core.sh, lib/wizard_utils.sh, lib/task_sources.sh, ralph_loop.sh, test files, CLAUDE.md Related Issues: Addresses issue 85 |
1. .ralphrc Configuration Loading Fix: - Captured env var state BEFORE setting defaults with _env_* variables - load_ralphrc now only restores values explicitly set by environment - .ralphrc settings are now properly applied (not overwritten by defaults) 2. sed Command Injection Fix: - Replaced sed with awk for .ralphrc updates in ralph_enable.sh - awk -v pattern safely handles user input without shell injection risk 3. Shell Injection Fix in safe_create_file(): - Replaced echo with printf '%s\n' for safer content handling - Prevents issues with backslashes, -n, and special characters 4. Specific Error Codes: - Added ENABLE_INVALID_ARGS=3 for argument errors - Added ENABLE_FILE_NOT_FOUND=4 for missing files - Added ENABLE_DEPENDENCY_MISSING=5 for missing deps (e.g., jq) - Added ENABLE_PERMISSION_DENIED=6 for permission errors - Updated ralph_enable.sh and ralph_enable_ci.sh to use specific codes 5. Added tests for .ralphrc loading pattern verification Test count: 398 (up from 396)
| shift | ||
| ;; | ||
| -v|--version) | ||
| if [[ "$OUTPUT_JSON" == "true" ]]; then |
There was a problem hiding this comment.
The --version handler checks OUTPUT_JSON before --json may have been parsed (e.g., --version --json). Consider parsing all arguments first, then handling --version in main() after parse_arguments() completes.
🚀 Want me to fix this? Reply ex: "fix it for me".
| beads) | ||
| if beads_tasks=$(fetch_beads_tasks 2>/dev/null); then | ||
| imported_tasks="$beads_tasks" | ||
| TASKS_IMPORTED=$(echo "$imported_tasks" | grep -c '^\- \[' || echo "0") |
There was a problem hiding this comment.
grep -c outputs "0" before returning exit code 1 when no matches, so || echo "0" appends a second "0", making TASKS_IMPORTED a multi-line string. Consider capturing to a temp variable and using || true instead, or use grep -c ... 2>/dev/null || : then default empty to "0".
| TASKS_IMPORTED=$(echo "$imported_tasks" | grep -c '^\- \[' || echo "0") | |
| TASKS_IMPORTED=$(echo "$imported_tasks" | grep -c '^\- \[' 2>/dev/null) || TASKS_IMPORTED=0 |
🚀 Want me to fix this? Reply ex: "fix it for me".
| local response | ||
|
|
||
| local yn_hint="[y/N]" | ||
| if [[ "${default,,}" == "y" ]]; then |
There was a problem hiding this comment.
The ${default,,} lowercase syntax requires bash 4.0+. macOS ships with bash 3.2 by default and will fail. Consider using a POSIX-compatible alternative like $(echo "$default" | tr '[:upper:]' '[:lower:]') or [[ "$default" == [Yy]* ]].
🚀 Want me to fix this? Reply ex: "fix it for me".
| # Using awk instead of sed to avoid command injection from user input | ||
| if [[ -f ".ralphrc" ]]; then | ||
| # Update max calls (awk safely handles the value without shell interpretation) | ||
| awk -v val="$CONFIG_MAX_CALLS" '/^MAX_CALLS_PER_HOUR=/{$0="MAX_CALLS_PER_HOUR="val}1' .ralphrc > .ralphrc.tmp && mv .ralphrc.tmp .ralphrc |
There was a problem hiding this comment.
The awk pipeline may leave .ralphrc.tmp orphaned if mv fails. Consider using a trap to clean up the temp file on failure, or checking each command's exit status explicitly.
🚀 Want me to fix this? Reply ex: "fix it for me".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/enable_core.sh`:
- Around line 712-718: In enable_ralph_in_directory() the local variable
skip_tasks (from ENABLE_SKIP_TASKS) is declared but never used; either remove
the declaration (delete the local skip_tasks line) or implement the skip
behavior by checking skip_tasks within the function (e.g., test the variable and
bypass task-related logic where task_content or task execution occurs); update
any conditionals that run task creation/processing to respect skip_tasks so
ENABLE_SKIP_TASKS actually prevents those actions.
♻️ Duplicate comments (5)
lib/enable_core.sh (1)
144-172:--forceflag is not propagated tosafe_create_file, leaving existing files intact.The
safe_create_filefunction always skips existing files regardless of theforceflag passed toenable_ralph_in_directory. When--forceis specified, users expect existing configuration files to be overwritten.🐛 Proposed fix
safe_create_file() { local target=$1 local content=$2 + local force="${3:-false}" - if [[ -f "$target" ]]; then + if [[ -f "$target" && "$force" != "true" ]]; then enable_log "SKIP" "$target already exists" return 1 fi + + if [[ -f "$target" && "$force" == "true" ]]; then + enable_log "WARN" "Overwriting $target (--force)" + fi # Create parent directory if neededThen update callers in
enable_ralph_in_directory(lines 756-779):- safe_create_file ".ralph/PROMPT.md" "$prompt_content" + safe_create_file ".ralph/PROMPT.md" "$prompt_content" "$force" ... - safe_create_file ".ralph/@AGENT.md" "$agent_content" + safe_create_file ".ralph/@AGENT.md" "$agent_content" "$force" ... - safe_create_file ".ralph/@fix_plan.md" "$fix_plan_content" + safe_create_file ".ralph/@fix_plan.md" "$fix_plan_content" "$force" ... - safe_create_file ".ralphrc" "$ralphrc_content" + safe_create_file ".ralphrc" "$ralphrc_content" "$force"ralph_enable_ci.sh (2)
391-395: Include@AGENT.mdin required files verification for consistency.
check_existing_ralphinenable_core.shconsiders.ralph/@AGENT.mdas a required file, but this verification only checksPROMPT.mdand@fix_plan.md. This inconsistency could report success when configuration is incomplete.🐛 Proposed fix
# Verify required files exist - if [[ ! -f ".ralph/PROMPT.md" ]] || [[ ! -f ".ralph/@fix_plan.md" ]]; then + if [[ ! -f ".ralph/PROMPT.md" ]] || [[ ! -f ".ralph/@fix_plan.md" ]] || [[ ! -f ".ralph/@AGENT.md" ]]; then output_error "Required files were not created" exit $ENABLE_ERROR fi
234-257: JSON output may be malformed if project_name or project_type contain special characters.Variables are interpolated directly into the JSON template. If either contains quotes, backslashes, or newlines, the JSON output will be invalid. Since
jqis already required for--jsonmode, use it for safe JSON construction.🐛 Proposed fix
output_success() { local project_name="$1" local project_type="$2" if [[ "$OUTPUT_JSON" == "true" ]]; then local files_json files_json=$(printf '%s\n' "${CREATED_FILES[@]}" | jq -R . | jq -s .) - cat << EOF -{ - "success": true, - "project_name": "$project_name", - "project_type": "$project_type", - "files_created": $files_json, - "tasks_imported": $TASKS_IMPORTED, - "message": "Ralph enabled successfully" -} -EOF + jq -n \ + --arg name "$project_name" \ + --arg type "$project_type" \ + --argjson files "$files_json" \ + --argjson tasks "$TASKS_IMPORTED" \ + '{ + success: true, + project_name: $name, + project_type: $type, + files_created: $files, + tasks_imported: $tasks, + message: "Ralph enabled successfully" + }' elseralph_enable.sh (2)
193-201: MissingFORCE_OVERWRITE=trueafter user confirms continuation.When the user confirms they want to continue despite Ralph already being enabled (line 194),
FORCE_OVERWRITEshould be set totrue. Otherwise,enable_ralph_in_directorywill still detect the complete state and returnENABLE_ALREADY_ENABLED.🐛 Proposed fix
if [[ "$NON_INTERACTIVE" != "true" ]]; then if ! confirm "Do you want to continue anyway?" "n"; then echo "Exiting. Use --force to overwrite." exit $ENABLE_ALREADY_ENABLED fi + FORCE_OVERWRITE=true else
119-126: The--fromoption accepts any value without validation.Invalid sources like
--from invalidare accepted silently and will fail later in task import. Add early validation for clearer error messages.🐛 Proposed fix
--from) if [[ -n "$2" && ! "$2" =~ ^-- ]]; then + if [[ ! "$2" =~ ^(beads|github|prd)$ ]]; then + echo "Error: Invalid source '$2'. Must be one of: beads, github, prd" >&2 + exit $ENABLE_INVALID_ARGS + fi TASK_SOURCE="$2" shift 2
🧹 Nitpick comments (2)
lib/enable_core.sh (2)
101-101:RALPH_MISSING_FILESis set but never used outside this file.Static analysis flagged this variable as unused. While it's set in
check_existing_ralph, it's only referenced in the interactive wizard's Phase 1 output (Line 207 inralph_enable.sh). Consider exporting it or documenting its cross-file usage.♻️ Proposed fix
+# Export for use by callers (e.g., ralph_enable.sh phase_environment_detection) +export RALPH_MISSING_FILES + check_existing_ralph() { RALPH_STATE="none" - RALPH_MISSING_FILES=() + RALPH_MISSING_FILES=() # Exported for interactive wizard display
319-347: Python project detection may overwrite JavaScript/TypeScript detection.If a project has both
package.jsonandpyproject.toml(e.g., a monorepo or a JS project with Python tooling), Python detection will overwrite the earlier JavaScript/TypeScript detection results. Consider usingelifor early return to respect detection precedence.♻️ Proposed fix
- # Detect from pyproject.toml or setup.py (Python) - if [[ -f "pyproject.toml" ]] || [[ -f "setup.py" ]]; then + # Detect from pyproject.toml or setup.py (Python) - only if not already detected + if [[ "$DETECTED_PROJECT_TYPE" == "unknown" ]] && { [[ -f "pyproject.toml" ]] || [[ -f "setup.py" ]]; }; then DETECTED_PROJECT_TYPE="python"Apply similar changes to Rust (line 350) and Go (line 359) detection blocks.
| enable_ralph_in_directory() { | ||
| local force="${ENABLE_FORCE:-false}" | ||
| local skip_tasks="${ENABLE_SKIP_TASKS:-false}" | ||
| local project_name="${ENABLE_PROJECT_NAME:-}" | ||
| local project_type="${ENABLE_PROJECT_TYPE:-}" | ||
| local task_content="${ENABLE_TASK_CONTENT:-}" | ||
|
|
There was a problem hiding this comment.
skip_tasks variable is declared but never used.
The ENABLE_SKIP_TASKS environment variable is read into skip_tasks but never referenced in the function logic. Either remove it or implement the skip-tasks behavior.
🐛 Proposed fix (remove unused variable)
enable_ralph_in_directory() {
local force="${ENABLE_FORCE:-false}"
- local skip_tasks="${ENABLE_SKIP_TASKS:-false}"
local project_name="${ENABLE_PROJECT_NAME:-}"
local project_type="${ENABLE_PROJECT_TYPE:-}"
local task_content="${ENABLE_TASK_CONTENT:-}"📝 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.
| enable_ralph_in_directory() { | |
| local force="${ENABLE_FORCE:-false}" | |
| local skip_tasks="${ENABLE_SKIP_TASKS:-false}" | |
| local project_name="${ENABLE_PROJECT_NAME:-}" | |
| local project_type="${ENABLE_PROJECT_TYPE:-}" | |
| local task_content="${ENABLE_TASK_CONTENT:-}" | |
| enable_ralph_in_directory() { | |
| local force="${ENABLE_FORCE:-false}" | |
| local project_name="${ENABLE_PROJECT_NAME:-}" | |
| local project_type="${ENABLE_PROJECT_TYPE:-}" | |
| local task_content="${ENABLE_TASK_CONTENT:-}" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 714-714: skip_tasks appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
In `@lib/enable_core.sh` around lines 712 - 718, In enable_ralph_in_directory()
the local variable skip_tasks (from ENABLE_SKIP_TASKS) is declared but never
used; either remove the declaration (delete the local skip_tasks line) or
implement the skip behavior by checking skip_tasks within the function (e.g.,
test the variable and bypass task-related logic where task_content or task
execution occurs); update any conditionals that run task creation/processing to
respect skip_tasks so ENABLE_SKIP_TASKS actually prevents those actions.
Follow-Up Code Review - PR 124I've reviewed the latest commits. Good progress, but one critical issue remains. ✅ Issues Fixed1. .ralphrc Configuration Loading - FIXED ✓ 2. Shell Injection via echo - FIXED ✓ 🔴 Critical Issue: --force Flag Non-FunctionalProblem: The --force flag is accepted but files are never actually overwritten. Root cause: safe_create_file() at lib/enable_core.sh:144-151 always skips existing files, regardless of ENABLE_FORCE value. Evidence: When force=true, enable_ralph_in_directory bypasses early return (line 722-726) but then calls safe_create_file which returns 1 for all existing files without overwriting them. Impact: Users running ralph-enable --force --project-name new-name will see success but files still contain old content. Fix needed: Make safe_create_file check ENABLE_FORCE and delete/overwrite existing files when true, or delete files before calling safe_create_file in enable_ralph_in_directory. Test gap: tests/unit/test_enable_core.bats:335 only checks assert_success, doesn't verify file contents actually changed. Summary
Excellent work overall with 75 new tests and modular design. Once --force is fixed, ready to merge. |
The --force flag was accepted but safe_create_file() always skipped existing files regardless of ENABLE_FORCE value. Changes: - safe_create_file() now checks ENABLE_FORCE environment variable - When ENABLE_FORCE="true", overwrites existing files instead of skipping - Added proper logging for overwrite operations Added tests: - Verify enable_ralph_in_directory actually changes file contents with --force - Test safe_create_file overwrites when ENABLE_FORCE is true - Test safe_create_file skips when ENABLE_FORCE is false Test count: 400 (up from 398)
Code Review SummaryThis PR adds excellent functionality for enabling Ralph in existing projects with comprehensive test coverage (75 new tests, 100% pass rate). The modular architecture with separate libraries for enable_core, task_sources, and wizard_utils demonstrates good design principles. However, one critical issue remains that must be fixed before merge. 🔴 Critical Issue: --force Flag Completely Non-FunctionalLocation: Problem: The Root Cause: safe_create_file() {
local target=$1
local content=$2
# No force parameter!
if [[ -f "$target" ]]; then # Always skips if file exists
enable_log "SKIP" "$target already exists"
return 1
fiImpact: Users running Fix Required: safe_create_file() {
local target=$1
local content=$2
local force="${3:-false}" # Add force parameter
if [[ -f "$target" && "$force" != "true" ]]; then
enable_log "SKIP" "$target already exists"
return 1
fi
if [[ -f "$target" && "$force" == "true" ]]; then
enable_log "WARN" "Overwriting $target (--force)"
rm -f "$target" # Delete before writing
# ... rest of function
}
# In enable_ralph_in_directory (lines 756-779):
safe_create_file ".ralph/PROMPT.md" "$prompt_content" "$force"
safe_create_file ".ralph/@AGENT.md" "$agent_content" "$force"
safe_create_file ".ralph/@fix_plan.md" "$fix_plan_content" "$force"
safe_create_file ".ralphrc" "$ralphrc_content" "$force"Test Gap: ✅ Strengths
✅ Issues Already Addressed
🟡 Minor Issues (Can Be Follow-up PR)
📋 RecommendationStatus: The The overall implementation is excellent with strong architecture, testing, and documentation. Once the force flag issue is resolved, this PR will be ready to merge and provides significant value for Ralph adoption. |
| # Write content to file using printf to avoid shell injection | ||
| # printf '%s\n' is safer than echo for arbitrary content (handles backslashes, -n, etc.) | ||
| if printf '%s\n' "$content" > "$target" 2>/dev/null; then | ||
| if [[ -f "$target" ]] && [[ "$force" == "true" ]]; then |
There was a problem hiding this comment.
The condition [[ -f "$target" ]] at line 176 will always be true since you just wrote the file. Consider checking if the file existed before writing (use the result from the earlier check at line 152) to correctly distinguish "Overwrote" vs "Created" messages.
🚀 Want me to fix this? Reply ex: "fix it for me".
| # Tasks from beads | ||
| ${beads_tasks} | ||
| " | ||
| ((source_count++)) |
There was a problem hiding this comment.
((source_count++)) returns exit code 1 when source_count is 0 (pre-increment value). Under set -e, this will exit the script. Consider using ((source_count++)) || true or source_count=$((source_count + 1)).
🚀 Want me to fix this? Reply ex: "fix it for me".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/enable_core.sh`:
- Around line 767-793: The snippet ignores return codes from safe_create_file so
write failures (safe_create_file returning 2) can be missed; after each call to
safe_create_file for ".ralph/PROMPT.md", ".ralph/@AGENT.md",
".ralph/@fix_plan.md", and ".ralphrc" check its exit status ($?) and if it
indicates a hard failure (e.g., return 2) log an error via process/logger (or
echo to stderr) including the target filename and exit code and then exit with
non-zero to prevent printing “Ralph enabled successfully”; apply the same check
after generate_* invocations where safe_create_file is called and ensure
detect_task_sources and task_sources logic runs only if file creation succeeded.
♻️ Duplicate comments (1)
lib/enable_core.sh (1)
726-731:ENABLE_SKIP_TASKShas no effect.
skip_tasksis read but never used, so the flag is a no-op. Either remove it or implement the skip behavior (e.g., ignoretask_contentwhenskip_tasks=true).
As per coding guidelines, this should be wired or removed.🔧 Suggested wiring
enable_ralph_in_directory() { local force="${ENABLE_FORCE:-false}" local skip_tasks="${ENABLE_SKIP_TASKS:-false}" local project_name="${ENABLE_PROJECT_NAME:-}" local project_type="${ENABLE_PROJECT_TYPE:-}" local task_content="${ENABLE_TASK_CONTENT:-}" + + if [[ "$skip_tasks" == "true" ]]; then + task_content="" + fi
🧹 Nitpick comments (1)
lib/enable_core.sh (1)
22-28: Add a brief function-level comment for_color.The coding guidelines require function-level comments for all bash functions;
_coloris missing one.
As per coding guidelines, please add a short doc comment.
| # Generate and create files | ||
| local prompt_content | ||
| prompt_content=$(generate_prompt_md "$project_name" "$DETECTED_PROJECT_TYPE" "$DETECTED_FRAMEWORK") | ||
| safe_create_file ".ralph/PROMPT.md" "$prompt_content" | ||
|
|
||
| local agent_content | ||
| agent_content=$(generate_agent_md "$DETECTED_BUILD_CMD" "$DETECTED_TEST_CMD" "$DETECTED_RUN_CMD") | ||
| safe_create_file ".ralph/@AGENT.md" "$agent_content" | ||
|
|
||
| local fix_plan_content | ||
| fix_plan_content=$(generate_fix_plan_md "$task_content") | ||
| safe_create_file ".ralph/@fix_plan.md" "$fix_plan_content" | ||
|
|
||
| # Detect task sources for .ralphrc | ||
| detect_task_sources | ||
| local task_sources="local" | ||
| if [[ "$DETECTED_BEADS_AVAILABLE" == "true" ]]; then | ||
| task_sources="beads,$task_sources" | ||
| fi | ||
| if [[ "$DETECTED_GITHUB_AVAILABLE" == "true" ]]; then | ||
| task_sources="github,$task_sources" | ||
| fi | ||
|
|
||
| # Generate .ralphrc | ||
| local ralphrc_content | ||
| ralphrc_content=$(generate_ralphrc "$project_name" "$DETECTED_PROJECT_TYPE" "$task_sources") | ||
| safe_create_file ".ralphrc" "$ralphrc_content" |
There was a problem hiding this comment.
Handle safe_create_file failures explicitly.
safe_create_file can return 2 on write failure, but the return codes are ignored. This can emit “Ralph enabled successfully” even when files weren’t created.
🛠️ Proposed fix (check for hard failures only)
local prompt_content
prompt_content=$(generate_prompt_md "$project_name" "$DETECTED_PROJECT_TYPE" "$DETECTED_FRAMEWORK")
- safe_create_file ".ralph/PROMPT.md" "$prompt_content"
+ safe_create_file ".ralph/PROMPT.md" "$prompt_content"
+ rc=$?
+ if [[ $rc -eq 2 ]]; then
+ enable_log "ERROR" "Failed to write .ralph/PROMPT.md"
+ return $ENABLE_ERROR
+ fi
local agent_content
agent_content=$(generate_agent_md "$DETECTED_BUILD_CMD" "$DETECTED_TEST_CMD" "$DETECTED_RUN_CMD")
- safe_create_file ".ralph/@AGENT.md" "$agent_content"
+ safe_create_file ".ralph/@AGENT.md" "$agent_content"
+ rc=$?
+ if [[ $rc -eq 2 ]]; then
+ enable_log "ERROR" "Failed to write .ralph/@AGENT.md"
+ return $ENABLE_ERROR
+ fi
local fix_plan_content
fix_plan_content=$(generate_fix_plan_md "$task_content")
- safe_create_file ".ralph/@fix_plan.md" "$fix_plan_content"
+ safe_create_file ".ralph/@fix_plan.md" "$fix_plan_content"
+ rc=$?
+ if [[ $rc -eq 2 ]]; then
+ enable_log "ERROR" "Failed to write .ralph/@fix_plan.md"
+ return $ENABLE_ERROR
+ fi
@@
local ralphrc_content
ralphrc_content=$(generate_ralphrc "$project_name" "$DETECTED_PROJECT_TYPE" "$task_sources")
- safe_create_file ".ralphrc" "$ralphrc_content"
+ safe_create_file ".ralphrc" "$ralphrc_content"
+ rc=$?
+ if [[ $rc -eq 2 ]]; then
+ enable_log "ERROR" "Failed to write .ralphrc"
+ return $ENABLE_ERROR
+ fi🤖 Prompt for AI Agents
In `@lib/enable_core.sh` around lines 767 - 793, The snippet ignores return codes
from safe_create_file so write failures (safe_create_file returning 2) can be
missed; after each call to safe_create_file for ".ralph/PROMPT.md",
".ralph/@AGENT.md", ".ralph/@fix_plan.md", and ".ralphrc" check its exit status
($?) and if it indicates a hard failure (e.g., return 2) log an error via
process/logger (or echo to stderr) including the target filename and exit code
and then exit with non-zero to prevent printing “Ralph enabled successfully”;
apply the same check after generate_* invocations where safe_create_file is
called and ensure detect_task_sources and task_sources logic runs only if file
creation succeeded.
Code Review - PR 124: Ralph Enable Feature (v0.11.0)OverviewThis is an excellent addition to Ralph that addresses a real user need - enabling Ralph in existing projects. The implementation is comprehensive, well-tested, and follows Ralph's existing architectural patterns. Summary: APPROVED with minor recommendations Strengths1. Excellent Architecture & Design
2. Security & Safety
3. Comprehensive Testing
4. User Experience
5. Documentation
Code Quality Issues (Minor)1. Potential Code Execution in .ralphrc Loading (ralph_loop.sh:109-145) Risk: LOW (user controls their own .ralphrc) 2. Error Handling in Task Import Functions Recommendation: Consider distinguishing between no tasks found vs command failed for better debuggability. 3. Missing Validation for Project Type Override (ralph_enable_ci.sh) Risk: User could specify invalid type, leading to incorrect template generation Performance Considerations1. PRD File Search Efficiency (lib/enable_core.sh:446-454) Performance: Acceptable for small projects, could be slow for large monorepos 2. jq Availability Checks Best Practices AdherenceThe code follows Ralphs documented standards from CLAUDE.md:
Integration Quality.ralphrc Configuration System
This ensures .ralphrc can override defaults, but environment variables always win. Testing QualityReviewed test files:
Test quality highlights:
RecommendationsPriority 1 (Security - Minor)
Priority 2 (Robustness - Optional) Priority 3 (Performance - Optional) Priority 4 (Documentation - Nice to have) ConclusionThis is high-quality work that demonstrates:
The identified issues are all minor and non-blocking. The code is production-ready as-is. Recommendation: APPROVE and MERGE The feature successfully addresses issues No.85, No.121, No.64, No.87, No.99 and provides significant value to Ralph users who want to adopt it in existing projects. Related Files Reviewed
Total new code: approximately 4,012 lines |
1 participant
Summary
ralph-enableinteractive wizard for enabling Ralph in existing projectsralph-enable-cinon-interactive version for CI/automation.ralphrcproject configuration file supportNew Commands
Interactive Wizard
CI/Automation Version
New Files
ralph_enable.shralph_enable_ci.shlib/enable_core.shlib/wizard_utils.shlib/task_sources.shtemplates/ralphrc.templateModified Files
install.shralph-enableandralph-enable-cito global commandsralph_loop.sh.ralphrcconfiguration at startupCLAUDE.mdFeatures
.ralphrcfor project-specific settingsTest plan
lib/enable_core.shlib/task_sources.shralph_enable.shandralph_enable_ci.shRelated Issues
Closes #85, #121, #64, #87, #99
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.