update the rockout command with additional review ummph (#2093)

* update the rockout command with additional review ummph

* updated with CI and merge conflict checks

Author: brendancol

.claude/commands/rockout.md

@@ -155,44 +155,190 @@ review-pr / follow-up loop runs in parallel. If CI surfaces a failure
 later, address it as a separate follow-up commit on the same branch --
 do not block the review pass on green CI.
 
-## Step 9 -- Run the Domain-Aware PR Review
+## Step 9 -- Run the Domain-Aware PR Review and Post It as a GitHub Review
+
+Every rockout PR MUST receive a review posted to GitHub as a proper review
+(not a plain issue comment), regardless of how clean the change looks. The
+review is the audit trail.
 
 1. Invoke the `/review-pr` command against the PR number from Step 8:
    ```
    /review-pr <PR_NUMBER>
    ```
-2. Do not pass "post" -- keep the review local so the rockout workflow can act
-   on the findings before any of it lands as a public comment.
+2. Do not pass "post" -- keep `/review-pr` from posting on its own. Rockout
+   will post the review explicitly in step 5 below so it lands as a GitHub
+   review event, not a free-form comment.
 3. Capture the structured output. It will list findings grouped as:
    - **Blockers** -- must fix before merge
    - **Suggestions** -- should fix, not blocking
    - **Nits** -- optional improvements
 4. Run this step regardless of CI status. Do not poll `gh pr checks` or
    wait for workflows to finish before invoking `/review-pr`.
+5. Post the captured review body to GitHub as a review event of type
+   `COMMENT` so it shows up under the PR's Reviews tab (not just the
+   Conversation tab). Use a heredoc to preserve formatting:
+   ```bash
+   gh pr review <PR_NUMBER> --comment --body "$(cat <<'EOF'
+   <humanized review body from /review-pr>
+   EOF
+   )"
+   ```
+   - Use `--comment`, never `--approve` or `--request-changes`. Rockout
+     does not have authority to approve its own work or block it.
+   - If the review body is empty (no findings at all), still post a short
+     review of type `--comment` summarizing that no issues were found, so
+     every rockout PR has a visible review entry.
+   - Confirm via `gh pr view <PR_NUMBER> --json reviews` that a review of
+     state `COMMENTED` now exists on the PR before moving on.
 
 ## Step 10 -- Follow Up on Review Findings
 
-Address every Blocker, then work through Suggestions and Nits in that order.
+Treat the review output as expert input, not a verdict. The reviewer is
+another LLM running a checklist -- it catches real issues, but it also
+flags false positives, misreads context, and occasionally invents
+problems. Your job is to weigh each finding against the actual code, not
+to mechanically apply every suggestion.
+
+Address every Blocker first, then work through Suggestions and Nits in
+that order.
 
 1. For each finding:
-   - Read the referenced file at the cited line.
-   - Decide one of: **fix**, **defer with reason**, or **dismiss with reason**.
-   - Blockers must be either fixed or explicitly deferred with a written
-     justification -- do not silently skip them.
-   - Suggestions and nits may be dismissed when the cost outweighs the value,
-     but record the reason.
+   - Read the referenced file at the cited line and understand the
+     surrounding context before deciding anything.
+   - Verify the finding describes a real problem. If the reviewer
+     misread the code, the cited line does not exist, or the
+     "issue" is actually intended behavior, mark it **dismissed**
+     and record the reason -- do not fix phantom bugs.
+   - For Blockers: take them seriously, but they are not automatic.
+     Confirm the issue is real before fixing. Real blockers must be
+     either fixed or explicitly deferred with a written justification --
+     do not silently skip them. A blocker you genuinely believe is
+     wrong can be dismissed, but the dismissal note must explain why
+     the reviewer was mistaken.
+   - For Suggestions: consider each one seriously. Apply when the
+     change clearly improves correctness, clarity, or performance.
+     Dismiss when the suggestion conflicts with project conventions,
+     would regress something else, or the cost outweighs the value.
+   - For Nits: apply when trivially cheap and clearly an improvement.
+     Dismiss when stylistic preference, churn for churn's sake, or
+     not aligned with how the rest of the codebase is written.
+   - In all cases, record the reason for dismiss / defer so the
+     summary captures the reasoning, not just the verdict.
 2. Group related fixes into focused commits referencing the issue number
    (e.g. `Address review nits: fix NaN propagation in dask path (#<NUMBER>)`).
 3. After applying fixes:
    - Re-run the tests touched by the changes.
    - Push the new commits to the PR branch.
-4. Re-run `/review-pr <PR_NUMBER>` once after the follow-up commits to confirm
-   the prior findings are resolved and no new ones surfaced. Stop iterating
-   once only dismissed-with-reason items remain.
+4. Re-run `/review-pr <PR_NUMBER>` once after the follow-up commits, and
+   post the follow-up review the same way as step 9.5 above
+   (`gh pr review <PR_NUMBER> --comment --body ...`). Stop iterating once
+   only dismissed-with-reason items remain.
 5. Summarize the disposition of each original finding (fixed / deferred /
-   dismissed) in the final rockout summary so the trail is visible.
+   dismissed, with the reason for dismissals) in the final rockout summary
+   so the trail is visible.
+
+**Do not skip this step.** Even if Step 9 returned no Blockers,
+Suggestions, or Nits, the review of type `COMMENTED` from step 9.5 must
+still be posted so every rockout PR carries a visible review entry.
 
-**Skip this step** only if Step 9 returned no Blockers, Suggestions, or Nits.
+## Step 11 -- Resolve Merge Conflicts With `main`
+
+After review follow-ups are done, sync the branch with `main` and resolve
+any conflicts before letting CI have the final word. Stay inside the
+worktree from Step 2 -- do NOT switch the main checkout.
+
+1. Confirm you are still in `$ROCKOUT_WT` on branch `issue-<NUMBER>`:
+   ```bash
+   [ "$(pwd)" = "$ROCKOUT_WT" ] || { echo "CWD drift"; exit 1; }
+   [ "$(git branch --show-current)" = "issue-<NUMBER>" ] || { echo "branch drift"; exit 1; }
+   ```
+2. Fetch the latest `main` and check whether the branch is behind:
+   ```bash
+   git fetch origin main
+   git log --oneline HEAD..origin/main | head
+   ```
+   If there are no new commits on `main`, skip to Step 12.
+3. Merge `origin/main` into the feature branch (prefer merge over rebase
+   so the PR history stays stable for reviewers):
+   ```bash
+   git merge --no-edit origin/main
+   ```
+4. If the merge reports conflicts:
+   - Run `git status` and list every conflicted path.
+   - For each conflicted file, read both sides, understand the intent,
+     and edit the file to a resolution that preserves the feature work
+     AND the incoming changes from `main`. Do NOT blindly accept one
+     side with `git checkout --ours/--theirs` unless you have read the
+     file and confirmed the other side is irrelevant.
+   - After editing, `git add <file>` for each resolved path.
+   - When all conflicts are resolved, finalize with `git commit` (no
+     `-m` flag needed -- git will use the prepared merge message).
+5. Re-run the test suite touched by the change to confirm the merge did
+   not break behaviour. If tests fail because of the merge, fix the
+   root cause; do not paper over with skips.
+6. Push the merge commit to the PR branch:
+   ```bash
+   git push origin issue-<NUMBER>
+   ```
+7. Confirm via `gh pr view <PR_NUMBER> --json mergeable,mergeStateStatus`
+   that the PR is no longer in a conflicted state before moving on.
+
+If the merge produces no conflicts and no test fallout, this step is a
+fast no-op. Run it anyway -- the goal is to know the PR is mergeable
+before CI failures get evaluated in Step 12.
+
+## Step 12 -- Fix CI Failures
+
+CI runs asynchronously after the push in Step 8 (and again after the
+follow-up pushes in Steps 10 and 11). This is the final gate: drive every
+required check to green before declaring the rockout done.
+
+1. Poll the PR's check status until every check has completed (success
+   or failure -- not pending):
+   ```bash
+   gh pr checks <PR_NUMBER>
+   ```
+   If checks are still running, wait and re-poll. Do not declare done
+   while any required check is pending.
+2. For each failing check:
+   - Pull the failing job's logs:
+     ```bash
+     gh run view --log-failed --job <JOB_ID>
+     ```
+     or open the run via `gh pr checks <PR_NUMBER> --watch` and drill
+     into the failing job.
+   - Read the actual failure (test name, traceback, lint rule, etc.).
+     Do not guess from the check name.
+   - Classify the failure:
+     - **Real defect in the change** -- fix the code, add or update a
+       test if coverage was missing, commit the fix.
+     - **Pre-existing flake unrelated to the change** -- rerun the
+       failed job once with `gh run rerun <RUN_ID> --failed`. If it
+       passes, note it in the summary and move on. If it fails again
+       in the same way, treat it as a real failure and fix it.
+     - **Environment / infra issue** (cache miss, runner outage, token
+       expiry) -- rerun the failed job. If it keeps failing for the
+       same infra reason after one rerun, surface it to the user
+       rather than hacking around it.
+3. For real defects, follow the same isolation rules as earlier steps:
+   work inside `$ROCKOUT_WT` on `issue-<NUMBER>`, commit with a message
+   referencing the issue (e.g. `Fix dask path NaN handling for CI (#<NUMBER>)`),
+   and push to the PR branch.
+4. After each push, repeat from step 1 until every required check is
+   green. Do not merge or hand off while any required check is red.
+5. If a check is genuinely not relevant to the change and cannot be
+   made green (e.g. an unrelated workflow that is broken on `main`),
+   record the reason in the final summary and flag it to the user --
+   do not silently ignore red checks.
+6. Once all required checks are green, run the Step 11 conflict re-check
+   one more time (`gh pr view <PR_NUMBER> --json mergeable,mergeStateStatus`)
+   to confirm nothing landed on `main` while CI was running that would
+   re-conflict the branch.
+
+The rockout run is only complete when:
+- Every required CI check on the PR is green (or explicitly justified).
+- The PR reports `mergeable` with no conflicts against `main`.
+- The Step 9 / Step 10 review trail is posted.
 
 ---