mr-shepherd

name

mr-shepherd

description

This skill should be used when the user asks to "address MR feedback", "fix review comments", "respond to code review", "handle MR suggestions", "shepherd an MR", "resolve discussion threads", or "run the MR feedback loop". Provides a systematic workflow for reviewing, implementing, and closing GitLab MR feedback — used standalone by operators AND as the thread-handling primitive for /agentic-harness:mr-submit.

GitLab MR Feedback Workflow

A systematic workflow for addressing code review feedback on GitLab merge requests. This skill covers the complete cycle: reviewing feedback, evaluating applicability, planning fixes, implementing changes, pushing updates, and closing discussion threads.

When to Use

• After receiving code review comments on a GitLab MR
• When automated review bots (like ralph-review) post findings
• When needing to systematically address multiple feedback items
• When wanting to properly close discussion threads after fixes

Workflow Overview

1. Fetch & Review  →  2. Evaluate  →  3. Plan  →  4. Implement  →  5. Commit, Rebase if Behind, Push  →  6. Wait for CI & Triage  →  7. Respond & Resolve

Step 1: Fetch and Review Feedback

Get MR Overview with Comments

glab mr view <MR_NUMBER> --comments

List Unresolved Discussion Threads

# URL-encode the project path (org/group/repo → org%2Fgroup%2Frepo)
ENCODED_PATH="org%2Fgroup%2Frepo"
MR_NUMBER=34

glab api "projects/${ENCODED_PATH}/merge_requests/${MR_NUMBER}/discussions?per_page=100" \
  | jq '.[] | select(.notes[0].resolvable == true and .notes[0].resolved == false)
        | {disc_id: .id, note_id: .notes[0].id, body: .notes[0].body[0:100]}'

Categorize Feedback by Severity

Group findings into categories for prioritization:

Severity	Action
Critical/High	Must fix before merge
Medium	Should fix, may defer with justification
Low	Nice to have, can document as "won't fix"

Step 2: Evaluate Each Feedback Item

For each piece of feedback, determine:

1. Is it valid? - Does the concern apply to this codebase/context?
2. Is it applicable? - Does the suggested fix make sense here?
3. What's the impact? - Security, reliability, maintainability?
4. What's the effort? - Quick fix vs. significant refactor?

Decision Matrix

Valid	Applicable	Action
Yes	Yes	Implement fix
Yes	No	Explain alternative approach
No	-	Explain why not applicable

Common "Won't Fix" Justifications

• Environment guarantees: "Dependency X is guaranteed in the Docker image"
• Downstream handling: "The subsequent call will fail and report the error"
• Over-engineering: "Adding this check provides no additional safety"
• Out of scope: "This is a pre-existing issue, tracking separately"

Step 3: Plan the Implementation

Create a mental or written plan grouping related fixes:

## Fixes to Implement

### Group 1: Input Validation

- [ ] Add numeric validation for LINE variable
- [ ] Escape FILE variable for JSON

### Group 2: Error Handling

- [ ] Add error handling for mktemp
- [ ] Pre-compute jq values with error checks

### Group 3: Won't Fix (with justification)

- [ ] jq dependency check - guaranteed in Docker image

Step 4: Implement the Changes

Read the File First

Always read the current file state before editing:

# Use Read tool, not cat
Read: /path/to/file.sh

Make Atomic, Focused Edits

• One logical change per edit when possible
• Preserve existing indentation exactly
• Test syntax after changes:

# For shell scripts
bash -n /path/to/script.sh && echo "Syntax OK"

# For Python
python -m py_compile /path/to/script.py

# For YAML
yamllint /path/to/config.yml

Step 5: Commit, Rebase if Behind, and Push

Commit with Descriptive Message

Reference the feedback being addressed:

git add <files>
git commit -m "$(cat <<'EOF'
fix(component): address code review feedback

- Add input validation for X
- Escape Y for JSON safety
- Add error handling for Z

Addresses feedback from MR !<number>
EOF
)"

Rebase if Behind origin/main

Before every push attempt, check whether the source branch is behind origin/main. Shepherd runs commonly overlap with sibling MRs merging, and a stale branch will be refused by GitLab even after a clean local push. Baking this check into the workflow — rather than treating it as an ad-hoc nudge — keeps the push deterministic.

Detect staleness:

git fetch origin
LOCAL_BASE=$(git merge-base HEAD origin/main)
REMOTE_TIP=$(git rev-parse origin/main)

if [ "$LOCAL_BASE" = "$REMOTE_TIP" ]; then
  echo "Source branch is up to date with origin/main"
else
  echo "Source branch is behind origin/main — rebase required"
fi

Clean rebase (no conflicts):

git pull --rebase origin main
# Rebase applies cleanly → proceed to push

Rebase with conflicts:

git pull --rebase origin main
# Rebase stops on a conflict. Resolve in-place — do NOT use --skip
# (drops commits) and do NOT use --abort unless the conflict is a
# genuine unresolvable logic collision (in which case stop and report).
git status                              # See conflicted files
# [Edit files to resolve conflicts]
git add <resolved-files>
git rebase --continue

Abort path (unresolvable logic conflict only):

# Only when the conflict represents an actual semantic collision
# that requires human judgment — never as a shortcut.
git rebase --abort
# STOP. Report the blocker. Do not push.

Push to Remote

Use git push when the rebase was a fast-forward (no local commits were rewritten). Use git push --force-with-lease when the rebase rewrote history. Never use plain git push --force — it will overwrite concurrent pushes from other shepherds or operators.

# Fast-forward case (no rebase, or rebase had nothing to replay)
git push

# History-rewrite case (rebase replayed local commits on top of origin/main)
git push --force-with-lease

If the push is refused with a non-fast-forward error anyway (race with another push landing between your rebase and your push), retry the pull-rebase + push sequence up to 3 times before reporting a blocker:

for attempt in 1 2 3; do
  git pull --rebase origin main && git push --force-with-lease && break
  echo "Push race on attempt ${attempt}; retrying..."
done

Update MR Description (Optional)

Document what was addressed:

glab mr update <MR_NUMBER> --description "$(cat <<'EOF'
## Summary
[Original summary]

## Changes from Review Feedback
- Fixed: [issue 1]
- Fixed: [issue 2]
- Won't fix: [issue 3] - [justification]
EOF
)"

Step 6: Wait for CI and Triage Failures

A branch with 0 unresolved threads and a red pipeline is not done. Before moving on to thread resolution, wait for the MR's pipeline to reach a terminal state. Then classify any failures: infrastructural failures (transient — retry the job) vs code failures (real — fix and push). Baking this into the contract keeps "shepherded" aligned with "mergeable."

Poll Until Terminal

Prefer the wait-pipeline service (via the /agentic-harness:* commands or a direct Prose invocation) — it handles the full polling contract with proper terminal-state detection and a failed_jobs[] return. Default timeout is 30 minutes; override per-run when CI is known to be slower.

If you're running the workflow by hand, the equivalent direct call is:

ENCODED_PATH="org%2Fgroup%2Frepo"
MR_NUMBER=34

# Get the latest pipeline for the MR
PIPELINE_ID=$(glab api "projects/${ENCODED_PATH}/merge_requests/${MR_NUMBER}/pipelines" \
  | jq -r '.[0].id')

# Poll until terminal (success | failed | canceled | skipped) or 30-min timeout
DEADLINE=$(( $(date +%s) + 1800 ))
while [ "$(date +%s)" -lt "$DEADLINE" ]; do
  STATE=$(glab api "projects/${ENCODED_PATH}/pipelines/${PIPELINE_ID}" | jq -r '.status')
  case "$STATE" in
    success|failed|canceled|skipped) echo "Pipeline ${STATE}"; break ;;
    *) sleep 30 ;;
  esac
done

Act on the Terminal State

On success — proceed to Step 7 (thread resolution). Nothing to triage.

On failed — fetch the failed jobs and classify each one:

glab api "projects/${ENCODED_PATH}/pipelines/${PIPELINE_ID}/jobs?scope[]=failed" \
  | jq '.[] | {id, name, stage, web_url, failure_reason}'

On canceled — read intent. Manually canceled by a human? Ask before retrying. Canceled by a pipeline-level rule (e.g. a superseding pipeline)? Fine — the new pipeline is authoritative.

On skipped — rules-driven skip (e.g. changes: guard didn't match). Not a failure; proceed to Step 7.

On timeout — don't block indefinitely. Report the last-observed pipeline state and let the human decide. Do not auto-retry an ambiguous state.

Infrastructural vs Code Failures

The classifier runs on each failed job's trace. Fetch the trace via:

glab api "projects/${ENCODED_PATH}/jobs/${JOB_ID}/trace" > /tmp/job-${JOB_ID}.log

Then grep for transient-infrastructure signatures.

Common infrastructural failures

These are not code defects. Retry the job — do NOT amend commits or rewrite history.

Pattern	Cause
toomanyrequests: You have reached your unauthenticated pull rate limit	Docker Hub rate-limit on anonymous image pulls
ErrImagePull / ImagePullBackOff	Kubernetes runner couldn't pull the job image
Connection refused to registry-1.docker.io / registry.k8s.io	Upstream registry unreachable
Read timed out during pip install / npm install / go mod download	Dependency mirror flake
dial tcp ... i/o timeout against a well-known external service	Network flake to external dependency
Cannot connect to the Docker daemon / runner system failure	Runner pod itself was unhealthy

Classification snippet:

INFRA_PATTERNS='toomanyrequests|ErrImagePull|ImagePullBackOff|Connection refused|Read timed out|i/o timeout|runner system failure|Cannot connect to the Docker daemon'

if grep -E -q "$INFRA_PATTERNS" "/tmp/job-${JOB_ID}.log"; then
  echo "Infrastructural — retry"
else
  echo "Code failure — fix + push"
fi

Retry the failed job (preferred: use the retry-job service; direct equivalent below):

glab api -X POST "projects/${ENCODED_PATH}/jobs/${JOB_ID}/retry"
# GitLab returns a new job object with a fresh id; the original stays in
# its failed state for audit. Loop back to "Poll Until Terminal" for the
# new job's pipeline state.

After retry, re-enter the poll loop. Do NOT proceed to thread resolution until the pipeline is green — a retry that still fails may be code after all.

Code failures

These are real defects the MR introduced — lint regressions, broken tests, validate failures from a genuine config issue. Do NOT retry. Instead:

1. Read the trace. Identify the failing assertion / lint rule / validation error.
2. Fix the root cause with a new commit. Follow the Step 5 rebase-if-behind + push discipline.
3. Loop back to Step 6: wait for the next pipeline, re-classify.

# After pushing the fix, re-enter the wait loop against the new pipeline
PIPELINE_ID=$(glab api "projects/${ENCODED_PATH}/merge_requests/${MR_NUMBER}/pipelines" \
  | jq -r '.[0].id')
# ... repeat the Poll Until Terminal block

Guardrails

• Never auto-retry on canceled without confirming intent. A human may have canceled the pipeline deliberately.
• Never amend a commit to "fix" an infrastructural failure. The commit is fine; the runner isn't.
• Never retry more than twice for the same job without escalating. A job that fails three times despite matching infra patterns is probably a deeper incident (registry outage, runner scaling failure) that needs a human.
• On timeout, report and stop. Don't guess.

Step 7: Respond and Resolve Threads

Reply to a Discussion Thread

ENCODED_PATH="org%2Fgroup%2Frepo"
MR_NUMBER=34
DISCUSSION_ID="abc123..."

glab api "projects/${ENCODED_PATH}/merge_requests/${MR_NUMBER}/discussions/${DISCUSSION_ID}/notes" \
  -X POST \
  -f "body=Fixed in commit abc1234. [Brief description of fix]"

Reply Templates

For implemented fixes:

Fixed in commit <sha>. [Description of what was done]

\`\`\`bash
# Show the fix if helpful
\`\`\`

For won't fix:

Won't fix - [Justification]. [Explanation of why the concern doesn't apply or is handled elsewhere]

For duplicate findings:

Fixed in commit <sha>. [Brief note that this is same as another finding]

Resolve the Discussion Thread

After replying, resolve the thread:

glab api "projects/${ENCODED_PATH}/merge_requests/${MR_NUMBER}/discussions/${DISCUSSION_ID}" \
  -X PUT \
  -f "resolved=true"

Batch Resolve Multiple Threads

DISCUSSIONS=(
  "disc_id_1"
  "disc_id_2"
  "disc_id_3"
)

for disc_id in "${DISCUSSIONS[@]}"; do
  glab api "projects/${ENCODED_PATH}/merge_requests/${MR_NUMBER}/discussions/${disc_id}" \
    -X PUT -f "resolved=true" > /dev/null 2>&1 \
    && echo "Resolved: ${disc_id:0:12}..." \
    || echo "Failed: ${disc_id:0:12}..."
done

Step 8: Verify Completion

Check for Remaining Unresolved Threads

glab api "projects/${ENCODED_PATH}/merge_requests/${MR_NUMBER}/discussions?per_page=100" \
  | jq '[.[] | select(.notes[0].resolvable == true and .notes[0].resolved == false)] | length'

Expected output: 0

Complete Example

# 1. Get project info
ENCODED_PATH="proserve%2Ftools%2Fmy-repo"
MR_NUMBER=34

# 2. List unresolved discussions
glab api "projects/${ENCODED_PATH}/merge_requests/${MR_NUMBER}/discussions?per_page=100" \
  | jq '.[] | select(.notes[0].resolvable == true and .notes[0].resolved == false)
        | {disc_id: .id, body: .notes[0].body[0:80]}'

# 3. [Implement fixes in code]

# 4. Commit, rebase if behind origin/main, then push
git add -A && git commit -m "fix: address review feedback"
git fetch origin
if [ "$(git merge-base HEAD origin/main)" != "$(git rev-parse origin/main)" ]; then
  git pull --rebase origin main
  git push --force-with-lease
else
  git push
fi

# 5. Reply and resolve each thread
DISC_ID="abc123def456..."
glab api "projects/${ENCODED_PATH}/merge_requests/${MR_NUMBER}/discussions/${DISC_ID}/notes" \
  -X POST -f "body=Fixed in commit xyz789."

glab api "projects/${ENCODED_PATH}/merge_requests/${MR_NUMBER}/discussions/${DISC_ID}" \
  -X PUT -f "resolved=true"

# 6. Verify all resolved
glab api "projects/${ENCODED_PATH}/merge_requests/${MR_NUMBER}/discussions?per_page=100" \
  | jq '[.[] | select(.notes[0].resolvable == true and .notes[0].resolved == false)] | length'
# Output: 0

Tips

Efficient Thread Management

• Process threads in severity order (critical first)
• Group similar fixes together in one commit
• Use consistent reply format for easy scanning

Handling Automated Review Bots

Automated reviewers may post duplicate findings. Track which issues are duplicates and use brief replies like "Fixed (same as above)" to avoid repetition.

When to Create Follow-up Issues

If feedback identifies valid concerns that are out of scope for the current MR:

glab issue create \
  --title "Address [concern] identified in MR !${MR_NUMBER}" \
  --description "Feedback from code review..."

Then reply: "Valid concern, but out of scope for this MR. Created issue #X to track."

Optional: Hand Off to mr-watcher

mr-shepherd is single-pass — after Step 7 verifies zero unresolved threads, the workflow is complete. If the operator wants persistent coverage (new reviewer comments, CI re-runs, main-drift rebases) until the MR actually lands, hand off to the mr-watcher skill as the natural tail state:

Shepherd (single-pass) → Watcher (polls until merged/closed)

When to hand off:

• CI is flaky and will likely need retry-on-infra-failure coverage.
• The MR is expected to collect new reviewer threads over time.
• The operator does not want to manually re-invoke shepherd each time a new comment lands.

How to hand off:

Invoke the mr-watcher skill with the same mr_id. It will bootstrap its checkpoint at .claude/mr-watcher/<mr_id>/state.json, dispatch mr-shepherd-loop per cycle, and exit only when GitLab marks the MR merged or closed. See packages/agentic-harness/skills/mr-watcher/SKILL.md for the full workflow and packages/agentic-harness/services/mr-watch/index.md for the ProseScript orchestrator.

The shepherd itself stays single-pass — watching is an explicitly separate concern, dispatched only when persistent coverage is wanted.

Additional Resources

Related Skills

• gitlab-mr-discussion-threads - Detailed API reference for thread operations
• gitlab-mr-inline-comments - Creating inline comments with proper positioning
• mr-watcher - Long-running tail state; polls until the MR is merged or closed

Reference Files

• references/reply-templates.md - Extended reply templates for various scenarios