Skip to content

Conversation

@Alex-Alaniz
Copy link

Summary

Addresses #19 - Rewords the Setup section to clarify that the two options are alternative approaches, not sequential steps.

Changes

  • Renamed "Option 1/Option 2" → "Approach A/Approach B"
  • Added clear labels: "recommended for teams" vs "for personal use"
  • Added skills copy step to Approach A for completeness

Before

"Option 1: Copy to your project"
"Option 2: Install skills globally"

Users were confused whether both steps were required.

After

"Choose one of the following approaches:"
"Approach A: Project-local setup (recommended for teams)"
"Approach B: Global setup (for personal use across projects)"

Now it's clear these are mutually exclusive paths.

Reword 'Option 1/Option 2' to 'Approach A/Approach B' to clarify these
are alternative setup strategies, not sequential steps.

- Approach A: Project-local (recommended for teams)
- Approach B: Global (for personal use)

Fixes snarktank#19
@greptile-apps
Copy link

greptile-apps bot commented Jan 23, 2026

Greptile Summary

This PR successfully clarifies that the two setup approaches are mutually exclusive alternatives, addressing the confusion from issue #19. The changes rename "Option 1/2" to "Approach A/B" and add clear guidance about when to use each approach (teams vs personal use).

However, the PR introduces a critical bug in Approach A: the command cp -r /path/to/ralph/skills .claude/skills/ creates a nested directory structure at .claude/skills/skills/prd/ instead of the correct .claude/skills/prd/. This will cause Ralph to fail since the skills won't be found at the expected paths. Approach B correctly copies individual skill directories (skills/prd and skills/ralph), which is the pattern Approach A should follow.

The documentation improvements are valuable, but the path bug must be fixed before merge to prevent a broken user experience.

Confidence Score: 2/5

  • This PR should not be merged due to a critical path bug that will break Approach A setup
  • While the documentation clarity improvements are excellent and address the original issue, the PR introduces a critical bug that creates incorrect directory structures. The nested skills/skills/ path will cause Ralph to fail for users following Approach A. This is a functional regression that outweighs the documentation improvements.
  • README.md requires immediate attention to fix the skills copy command in Approach A (lines 40-42)

Important Files Changed

Filename Overview
README.md Improved clarity of setup instructions but introduced critical path bug in Approach A that creates nested directory structure

Sequence Diagram

sequenceDiagram
    participant User
    participant README
    participant FileSystem
    
    User->>README: Read Setup section
    README->>User: "Choose one of the following approaches"
    
    alt Approach A: Project-local setup
        User->>README: Select Approach A (teams)
        README->>User: Copy ralph.sh to scripts/ralph/
        User->>FileSystem: cp ralph.sh scripts/ralph/
        README->>User: Copy prompt templates
        User->>FileSystem: cp prompt.md/CLAUDE.md to scripts/ralph/
        README->>User: Copy skills to project
        User->>FileSystem: cp -r skills .claude/skills/ OR .amp/skills/
        Note over User,FileSystem: BUG: Creates nested skills/skills/ structure
        FileSystem-->>User: Skills at wrong path (.claude/skills/skills/prd)
    else Approach B: Global setup
        User->>README: Select Approach B (personal)
        README->>User: Copy skills to global config
        User->>FileSystem: cp -r skills/prd ~/.config/amp/skills/
        User->>FileSystem: cp -r skills/ralph ~/.config/amp/skills/
        FileSystem-->>User: Skills correctly at ~/.config/amp/skills/prd
        README->>User: Then copy ralph.sh to each project
        User->>FileSystem: cp ralph.sh to project
    end
    
    User->>User: Attempt to use Ralph
    alt Approach A was chosen
        User->>FileSystem: Ralph looks for skills
        FileSystem-->>User: Skills not found (wrong path)
    else Approach B was chosen
        User->>FileSystem: Ralph looks for skills
        FileSystem-->>User: Skills found correctly
    end
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +40 to +42
cp -r /path/to/ralph/skills .claude/skills/ # For Claude Code
# OR
cp -r /path/to/ralph/skills .amp/skills/ # For Amp
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: incorrect path - creates nested skills/skills/ directory structure

copying the entire skills directory will create .claude/skills/skills/prd/ instead of .claude/skills/prd/

Suggested change
cp -r /path/to/ralph/skills .claude/skills/ # For Claude Code
# OR
cp -r /path/to/ralph/skills .amp/skills/ # For Amp
# Also copy the skills to your project
cp -r /path/to/ralph/skills/prd .claude/skills/ # For Claude Code
cp -r /path/to/ralph/skills/ralph .claude/skills/
# OR
cp -r /path/to/ralph/skills/prd .amp/skills/ # For Amp
cp -r /path/to/ralph/skills/ralph .amp/skills/
Prompt To Fix With AI
This is a comment left during a code review.
Path: README.md
Line: 40:42

Comment:
**logic:** incorrect path - creates nested `skills/skills/` directory structure

copying the entire `skills` directory will create `.claude/skills/skills/prd/` instead of `.claude/skills/prd/`

```suggestion
# Also copy the skills to your project
cp -r /path/to/ralph/skills/prd .claude/skills/    # For Claude Code
cp -r /path/to/ralph/skills/ralph .claude/skills/
# OR
cp -r /path/to/ralph/skills/prd .amp/skills/       # For Amp
cp -r /path/to/ralph/skills/ralph .amp/skills/
```

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant