Commit 16c46fa51c01

@@ -247,18 +247,232 @@ cat > review_checklist.md <<'EOF'
 EOF
 ```
 
-### Inline Comments
+### Inline Comments with GitHub API
 
-Add specific line comments:
+For thorough code reviews, you can add inline comments on specific file paths and line numbers using the GitHub API. This provides context-aware feedback directly on the code.
+
+#### Basic Syntax
 
 ```bash
-# Comment on specific lines (requires GitHub API)
-gh api repos/:owner/:repo/pulls/<number>/reviews \
-  -f event=COMMENT \
-  -f body="Review comments" \
-  -F comments[][path]="file.ts" \
+# Single inline comment
+gh api repos/OWNER/REPO/pulls/PR_NUMBER/reviews \
+  -f event="COMMENT" \
+  -f body="Overall review summary" \
+  -F comments[][path]="path/to/file.go" \
   -F comments[][line]=42 \
-  -F comments[][body]="Consider extracting this to a function"
+  -F comments[][side]="RIGHT" \
+  -F comments[][body]="Your feedback here (supports Markdown)"
+```
+
+#### Required Parameters
+
+- **`event`**: Review event type
+  - `"COMMENT"` - Comment without approval/rejection
+  - `"APPROVE"` - Approve the PR
+  - `"REQUEST_CHANGES"` - Request changes (blocks merge)
+
+- **`body`**: Overall review summary (appears at top of review)
+
+- **For each inline comment**:
+  - `comments[][path]` - File path relative to repo root
+  - `comments[][line]` - Line number in the diff
+  - `comments[][side]` - `"RIGHT"` (new code) or `"LEFT"` (old code)
+  - `comments[][body]` - Comment text (Markdown supported)
+
+#### Complete Example: Request Changes with Multiple Inline Comments
+
+```bash
+# Get repository info first
+REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner)
+PR_NUMBER=81
+
+# Post comprehensive review with inline comments
+gh api repos/${REPO}/pulls/${PR_NUMBER}/reviews \
+  -f event="REQUEST_CHANGES" \
+  -f body="$(cat <<'EOF'
+Thanks for the PR! This adds valuable functionality, but I found several issues that need addressing before merge.
+
+## Summary
+- ❌ **Critical**: No tests included (blocker)
+- ⚠️ **High**: Inconsistent error handling
+- 💡 **Medium**: Some code duplication
+
+Please address the critical and high-priority issues. The medium items are optional improvements.
+EOF
+)" \
+  -F comments[][path]="internal/artifacthub/client.go" \
+  -F comments[][line]=1 \
+  -F comments[][side]="RIGHT" \
+  -F comments[][body]="$(cat <<'EOF'
+**Missing Package Documentation**
+
+Add package-level documentation:
+\`\`\`go
+// Package artifacthub provides a client for interacting with the Artifact Hub API
+// to discover, retrieve, and install Tekton tasks and pipelines.
+package artifacthub
+\`\`\`
+EOF
+)" \
+  -F comments[][path]="internal/artifacthub/client.go" \
+  -F comments[][line]=221 \
+  -F comments[][side]="RIGHT" \
+  -F comments[][body]="$(cat <<'EOF'
+**Missing Error Wrapping**
+
+Use \`%w\` to wrap errors for better error inspection:
+\`\`\`go
+return nil, fmt.Errorf("API request failed with status %d: %w", resp.StatusCode, err)
+\`\`\`
+
+This enables \`errors.Is()\` and \`errors.As()\` usage.
+EOF
+)" \
+  -F comments[][path]="internal/tools/artifacthub.go" \
+  -F comments[][line]=47 \
+  -F comments[][side]="RIGHT" \
+  -F comments[][body]="$(cat <<'EOF'
+**Inconsistent Return Signature**
+
+This function returns an error that's always \`nil\`. Make it consistent with similar functions:
+
+\`\`\`go
+// Remove error return since it's never used
+func installArtifactHubTask() *mcp.ServerTool {
+    return mcp.NewServerTool(...)
+}
+\`\`\`
+EOF
+)" \
+  -F comments[][path]="internal/tools/artifacthub.go" \
+  -F comments[][line]=312 \
+  -F comments[][side]="RIGHT" \
+  -F comments[][body]="$(cat <<'EOF'
+**Code Duplication (DRY Violation)**
+
+This parameter conversion code is duplicated at lines 312-329 and 375-392. Extract to a helper function:
+
+\`\`\`go
+func convertParamValue(value interface{}) string {
+    switch v := value.(type) {
+    case string:
+        return v
+    case int:
+        return strconv.Itoa(v)
+    case float64:
+        return strconv.FormatFloat(v, 'f', -1, 64)
+    case bool:
+        return strconv.FormatBool(v)
+    default:
+        return fmt.Sprintf("%v", v)
+    }
+}
+\`\`\`
+EOF
+)"
+```
+
+#### Example: Approve with Inline Suggestions
+
+```bash
+# Approve but add non-blocking suggestions
+gh api repos/${REPO}/pulls/${PR_NUMBER}/reviews \
+  -f event="APPROVE" \
+  -f body="$(cat <<'EOF'
+LGTM! Great work on this feature.
+
+I've left a few inline suggestions for future improvements, but none are blocking.
+
+✅ Functionality is solid
+✅ Code follows Go best practices
+✅ Error handling is good
+
+Approved to merge.
+EOF
+)" \
+  -F comments[][path]="internal/artifacthub/client.go" \
+  -F comments[][line]=128 \
+  -F comments[][side]="RIGHT" \
+  -F comments[][body]="💡 **Optional**: Consider making the HTTP timeout configurable via a \`ClientOption\` function for better testability." \
+  -F comments[][path]="internal/tools/artifacthub.go" \
+  -F comments[][line]=100 \
+  -F comments[][side]="RIGHT" \
+  -F comments[][body]="💡 **Future enhancement**: Could add caching here to avoid repeated Artifact Hub lookups for the same package."
+```
+
+#### Example: Comment on Old Code (LEFT side)
+
+```bash
+# Comment on code being removed/changed (LEFT side of diff)
+gh api repos/${REPO}/pulls/${PR_NUMBER}/reviews \
+  -f event="COMMENT" \
+  -f body="Question about the changes" \
+  -F comments[][path]="pkg/handler.go" \
+  -F comments[][line]=15 \
+  -F comments[][side]="LEFT" \
+  -F comments[][body]="Why was this error handling removed? Was it redundant?"
+```
+
+#### Finding Line Numbers
+
+To determine the correct line numbers for comments:
+
+```bash
+# View the diff with line numbers
+gh pr diff ${PR_NUMBER} > /tmp/pr.diff
+
+# Or view specific file
+gh pr diff ${PR_NUMBER} | grep -A5 -B5 "filename"
+
+# The line number refers to the line in the DIFF, not the final file
+```
+
+**Important**: Line numbers refer to the position in the **diff**, not the absolute line number in the file.
+
+#### Best Practices for Inline Comments
+
+**⚠️ CRITICAL: ALWAYS ASK USER BEFORE POSTING REVIEWS**
+
+When reviewing PRs, **NEVER** post the review directly without explicit user approval:
+1. **Draft the review** with all inline comments
+2. **Show the user** what you plan to post (full command with all comments)
+3. **Ask for confirmation**: "Should I post this review?"
+4. **Wait for approval** before executing the command
+
+This prevents accidental reviews and allows the user to review/modify the feedback.
+
+**Other Best Practices:**
+
+1. **Use for specific issues**: Inline comments are best for pointing out specific problems
+2. **General feedback in body**: Put high-level summary in the review body
+3. **Include suggestions**: Don't just point out problems, suggest fixes
+4. **Use code blocks**: Format code suggestions with triple backticks
+5. **Be constructive**: Frame feedback positively
+6. **Prioritize issues**: Use tags like ❌ (critical), ⚠️ (important), 💡 (suggestion)
+7. **Explain reasoning**: Help the author understand WHY something should change
+8. **Limit quantity**: Too many inline comments can be overwhelming (aim for <15)
+
+#### Troubleshooting
+
+**Line number errors**:
+```bash
+# Error: "line N does not exist in diff"
+# Solution: The line number must be a changed line (added/removed/modified)
+# Use only lines visible in: gh pr diff <number>
+```
+
+**Side confusion**:
+- `"RIGHT"` = New code (green in GitHub UI, lines with `+`)
+- `"LEFT"` = Old code (red in GitHub UI, lines with `-`)
+
+**Testing your review**:
+```bash
+# Before posting real review, test with event="COMMENT"
+# This won't block the PR or send notifications
+gh api repos/${REPO}/pulls/${PR_NUMBER}/reviews \
+  -f event="COMMENT" \
+  -f body="Test review" \
+  # ... test your inline comments
 ```
 
 ### Batch Review
@@ -326,6 +540,58 @@ gh pr review 123 --comment --body "Adding review comments. Will approve after @o
 gh pr review 123 --approve
 ```
 
+### Scenario 4: Thorough Review with Inline Comments
+
+```bash
+# 1. Fetch PR information
+REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner)
+PR_NUMBER=123
+
+gh pr view ${PR_NUMBER} --json title,body,author,files
+gh pr checks ${PR_NUMBER}
+
+# 2. Analyze the diff
+gh pr diff ${PR_NUMBER} > /tmp/pr_${PR_NUMBER}.diff
+
+# 3. Check for issues
+## No tests?
+gh pr diff ${PR_NUMBER} --name-only | grep -q "_test" || echo "⚠️ No tests found"
+
+## Security concerns?
+gh pr diff ${PR_NUMBER} | grep -iE "(password|secret|api_key)" && echo "⚠️ Potential secrets"
+
+# 4. Post comprehensive review with inline comments
+gh api repos/${REPO}/pulls/${PR_NUMBER}/reviews \
+  -f event="REQUEST_CHANGES" \
+  -f body="$(cat <<'EOF'
+Thanks for the PR! I've reviewed the changes and found several issues.
+
+## Summary
+- ❌ **Critical**: Missing test coverage (blocker)
+- ⚠️ **High**: Error handling needs improvement
+- 💡 **Medium**: Consider refactoring duplicated code
+
+I've left inline comments on specific lines. Please address the critical issues before merge.
+EOF
+)" \
+  -F comments[][path]="internal/client.go" \
+  -F comments[][line]=1 \
+  -F comments[][side]="RIGHT" \
+  -F comments[][body]="**Missing package documentation** - Add a package comment explaining the purpose" \
+  -F comments[][path]="internal/client.go" \
+  -F comments[][line]=150 \
+  -F comments[][side]="RIGHT" \
+  -F comments[][body]="**Error wrapping**: Use \`%w\` here to preserve the error chain" \
+  -F comments[][path]="internal/handler.go" \
+  -F comments[][line]=45 \
+  -F comments[][side]="RIGHT" \
+  -F comments[][body]="**Missing tests**: Add tests for this new handler in \`handler_test.go\`"
+
+# 5. Track the review
+echo "✅ Review posted. Track updates:"
+echo "gh pr view ${PR_NUMBER}"
+```
+
 ## Custom Tool: Enhanced Review
 
 For complex review workflows:
@@ -407,6 +673,15 @@ EOF
 
 ## Best Practices
 
+**⚠️ #1 RULE: ALWAYS ASK USER BEFORE POSTING REVIEWS**
+
+Never post PR reviews, approvals, or request changes without explicit user confirmation. Always:
+1. Draft the review and show it to the user
+2. Ask: "Should I post this review?"
+3. Wait for approval before executing
+
+**Other Best Practices:**
+
 1. **Review promptly**: Don't let PRs sit unreviewed
 2. **Be constructive**: Frame feedback positively
 3. **Explain reasoning**: Don't just say "change this"