git:20260524.2ff8100 to git:20260525.ebe6f41

- Apply when processing PR review feedback — addressing reviewer findings on an open pull request.

- Covers the full cycle: read review comments, validate findings against actual code, classify

- by file and severity, delegate fixes, update documentation, amend the commit, force-push,

- update the PR description, and monitor CI to green. Prevents blind fixes, over-scoped changes,

- and CI surprises.

- # PR Review Fix Protocol

-

- Follow every step in order. Do not skip steps. This skill assumes you already have an open PR with review comments.

-

- ## Step 0 — Check for parallel work on the PR branch

-

- **Reference the [parallel-work-check skill](../parallel-work-check/SKILL.md) for the full protocol.**

-

- Before reading any review comments, check whether other agents have pushed work to this PR branch:

-

- 1. Fetch the remote PR branch state.

- 2. Compare local HEAD with remote HEAD (`git log origin/<branch>..HEAD` and `git log HEAD..origin/<branch>`).

- 3. If remote is ahead: read the new commits with `git log origin/<branch> --not HEAD`.

- 4. Evaluate whether the parallel work supersedes your planned fixes:

- - **Supersedes**: The remote already fixes the issues you planned to address. Abort your planned fixes, integrate the remote changes, and re-evaluate what remains.

- - **Complementary**: The remote fixes different issues than your planned fixes. Integrate first, then proceed.

- - **Does not affect**: The remote touches unrelated files. Proceed with your planned fixes.

- 5. Document the decision using the PARALLEL WORK CHECK template from the parallel-work-check skill.

-

- **Gate:** If remote is ahead and parallel work supersedes your planned fixes, do NOT proceed with Step 1 until you have re-evaluated the remaining scope.

-

- ## Step 1 — Read the review and normalize findings

-

- 1. Collect ALL review comments from the PR (inline comments, general comments, review summary).

- - For bot or app reviews, also identify commits pushed by that bot/app and compare the comment claims against branch history and current code. Do not assume the review text and pushed code are equivalent.

- 2. Normalize each finding into a structured record:

-

- ```

- FINDING-ID: C-01 (C=critical, S=security, T=test, W=wording, P=process)

- SEVERITY: HIGH / MEDIUM / LOW

- FILE: path/to/file.ts

- LINE: 42

- DESCRIPTION: What the reviewer flagged

- EVIDENCE: Exact quote from the reviewer

- ```

-

- 3. Assign a sequential ID to each finding (C-01, C-02, ..., S-01, ..., T-01, ..., W-01, ..., P-01, ...).

- 4. Print the full findings table before proceeding.

-

- **Gate:** Every finding must have an ID, severity, file, and exact quote. If any field is missing, go back and fill it.

-

- ## Step 1a — Delegation strategy: plan-based vs Task-only

-

- When fixing findings on a PR branch (no active swarm plan), the plan system's scaffolding

- requirements (spec.md, plan.json, QA gate selection) may block `declare_scope`. Choose

- the delegation strategy based on context:

-

- **Plan-based delegation (use when a plan is active):**

- - Creates `.swarm/scopes/scope-{taskId}.json` for the coder agent

- - Coder has restricted write access (only files in scope)

- - Preferred for multi-file changes where scope discipline matters

- - Requires: `.swarm/plan.json` + `.swarm/spec.md` + QA gate selection

-

- **Task-only delegation (use when fixing PR review findings without a swarm plan):**

- - Delegate to coder via **Task tool** with exact change specification

- - The coder accepts the delegation, applies fixes, runs tests, and validates changes

- without scope restrictions (no scope file means the coder can write any file)

- - After the Task returns, verify: `syntax_check` passes on changed files,

- relevant tests pass, no unintended file modifications

- - The architect MUST NOT edit source files directly — all code changes go through

- the coder even when no scope is active

-

- **Mechanical vs complex:**

- - **Mechanical** (e.g., updating test expectations, renaming a parameter): one-line

- Task delegation specifying the exact old→new text and file path

- - **Complex** (e.g., logic restructure, adding a new function): detailed Task delegation

- with acceptance criteria, test expectations, and skill references

-

- **Enforcement: regardless of delegation strategy, the architect MUST NOT edit source files

- directly.** All code changes must go through the coder agent. The architect orchestrates,

- specifies, and validates — never edits.

-

- **Validation after Task delegation (any strategy):**

- - `syntax_check` passes on all changed files

- - No unintended files were modified by the coder

- - Relevant test suite passes

- - Diff matches the intended fix scope only

-

- **Gate:** Print your delegation strategy before proceeding:

- `DELEGATION STRATEGY: plan-based / Task-only`

-

- ## Step 1b — Check normalization consistency (when applicable)

-

- When a PR adds data normalization, transformation, or coercion logic:

-

- 1. Identify ALL consumer paths that read the transformed data.

- 2. Verify that normalization is applied consistently across EVERY path.

- 3. Common failure mode: normalization applied in one read path (e.g., gate-check) but not in another (e.g., write/update), causing silent data loss or inconsistent behavior.

- Example: if a PR normalizes file paths during read (via `path.resolve`), verify the

- same normalization is applied during write/update operations. Missing normalization

- on the write path creates an asymmetry: the read gate passes but the stored value

- bypasses normalization.

- 4. If inconsistency is found, flag as a HIGH severity finding requiring shared helper extraction.

-

- ## Step 2 — Validate findings against actual code (do NOT skip)

-

- **Critical step.** Reviewers sometimes flag code that is correct, misread control flow, or cite issues that don't exist at the referenced line. Blindly fixing every finding wastes time and can introduce regressions.

-

- For EACH finding:

-

- 1. Open the referenced file at the referenced line.

- 2. Read the surrounding context (at least 20 lines before and after).

- 3. Determine one of:

- - **VALID** — the finding is correct and the code needs a fix

- - **DOWNGRADE** — the finding is real but severity is lower than stated (e.g., HIGH → LOW)

- - **INVALID** — the finding is incorrect; the code is already correct

- - **NEEDS CONTEXT** — you cannot determine validity without asking the reviewer or user

- 4. Record the verdict next to the finding.

-

- **Rules:**

- - A finding is INVALID only if you can prove with code evidence that the reviewer's claim is wrong.

- - "I think it's fine" is NOT a valid INVALID verdict. You need a specific code-level reason.

- - When downgrading, record the original severity and the recommended severity with a one-line justification.

- - Print the full validated findings table before proceeding.

-

- ## Step 3 — Classify findings by file and fix scope

-

- Group findings by file, then determine fix scope for each group:

-

- ```

- FILE: src/hooks/example.ts

- FINDINGS: C-01 (VALID, HIGH), C-02 (DOWNGRADE→LOW)

- FIX SCOPE: Single file, two changes

- DEPENDENCIES: None (changes are independent)

- ```

-

- For each group, answer:

- - Does this fix require changes to other files? (imports, tests, types)

- - Does this fix change any public API or exported interface?

- - Does this fix require documentation updates (README, release notes)?

-

- **Gate:** Every finding must be classified. Do not proceed with unclassified findings.

-

- ## Step 4 — Fix findings (one file group at a time)

-

- For each file group:

-

- 1. Read the full file before making changes. Do not edit blindly at the flagged line.

- 2. Make the minimal fix that addresses the finding without introducing new behavior.

- 3. For DOWNGRADE findings: still fix them if the change is trivial. Skip only if the fix would be riskier than the finding.

- 4. For INVALID findings: do NOT touch the code. Add a comment to the PR thread explaining why.

-

- **Anti-patterns — do NOT do these:**

- - Fixing a finding by adding a comment that says "// this is fine" — fix the code or skip it.

- - Batch-fixing all findings in one pass without reading each file individually.

- - "While I'm here" changes — only fix what was flagged.

- - Changing unrelated code to "improve" the area around the finding.

- - Accepting coder edits to template literal strings (backtick-delimited `.ts` content) without verifying that internal backticks are escaped. Unescaped backticks cause `SyntaxError` at build time even when the text looks correct in Read output.

-

- ## Step 5 — Update tests

-

- For each code fix:

-

- 1. If the fix changes behavior covered by existing tests: run those tests and verify they still pass.

- 2. If the fix changes behavior NOT covered by existing tests: write a new test proving the fix works.

- 3. If the fix is purely cosmetic (renaming, comment wording): no new test needed, but run the existing suite.

-

- Run the relevant test files in isolation before proceeding to the full suite:

-

- ```bash

- bun --smol test tests/unit/path/to/test.test.ts --timeout 30000

- ```

-

- ## Step 6 — Update documentation

-

- Check if any fix requires documentation updates:

-

- - **Release notes** (`docs/releases/pending/<slug>.md`): Add or update a pending fragment if the fix changes user-visible behavior, fixes a documented issue, or alters an API. Do NOT compute a next version or create `docs/releases/vX.Y.Z.md` — release-please owns the version, and `scripts/release-notes-fragments.mjs` aggregates pending fragments at release time. Pick a unique kebab-case slug.

- - **README / guides**: Update if the fix changes installation steps, configuration options, or usage patterns.

- - **Code comments**: Update if the fix invalidates an existing comment or makes a non-obvious behavior change.

-

- ## Step 7 — Commit the fixes

-

- 1. Stage ALL fix files (code + tests + docs). Do not stage unrelated changes.

- - Use explicit path staging to include new files (tests, docs) that `git add -u` would miss:

- ```bash

- git add src/path/to/changed-file.ts tests/unit/path/to/new-test.test.ts docs/releases/pending/your-fix-slug.md

- ```

- 2. If this is an amendment to the PR commit:

-

- ```bash

- git commit --amend --no-edit

- ```

-

- If the PR uses separate commits for fixes:

-

- ```bash

- git commit -m "fix(pr-review): address findings C-01, C-02, T-01, W-01"

- ```

-

- 3. Verify the commit message follows conventional commit format.

-

- ## Step 8 — Pre-push validation

-

- Before pushing, run local validation to catch formatting and style issues that CI would reject:

-

- 1. **Lint/format check**: Run the project's local linting tool on all modified files. This catches formatting-only issues without burning a CI cycle. On this project, check that any coder-written test files pass formatting validation.

- 2. **Build check**: Run the project build command to verify no syntax errors (especially important when coders modify template literal strings — unescaped backticks cause build failures).

- 3. Stage and commit.

-

- ## Step 9 — Push and update PR

-

- ```bash

- # For amended commits:

- git push --force-with-lease origin <branch>

-

- # For new commits:

- git push origin <branch>

- ```

-

- Update the PR description to include a `## PR Review Fixes` section:

-

- ```markdown

- ## PR Review Fixes

-

- Addressed N findings from PR review:

-

- | ID | Severity | Verdict | Fix |

- |----|----------|---------|-----|

- | C-01 | HIGH | VALID | Fixed traversal check at line 121 |

- | C-02 | MEDIUM | DOWNGRADE→LOW | Added warn() logging for empty catch |

- | S-01 | HIGH | INVALID | Code already uses validated path resolution |

- | T-01 | LOW | VALID | Added test for validated path resolution |

- | W-01 | LOW | VALID | Corrected wording in release notes |

-

- Skipped: none

- ```

-

- ## Step 10 — Monitor CI

-

- 1. Wait for CI to start.

- 2. **If a CI run appears stuck (queued >10 minutes)**: check for stale in-progress runs from prior pushes. Cancel them with `gh run cancel <run-id>` — GitHub Actions concurrency groups queue new runs behind in-progress ones.

- 3. If CI fails:

- - Read the failure log carefully.

- - Determine if the failure is caused by your fix or is a pre-existing issue.

- - If caused by your fix: return to Step 3 for that finding.

- - If caused by formatting/style: fix locally, rebuild, amend, and push again.

- - If pre-existing (infrastructure flake, unrelated test):

- a. Rerun the failed job once.

- b. If the rerun passes: the failure was infrastructure. Proceed.

- c. If the rerun fails the same way: this is a real pre-existing issue. Document it separately in the PR thread (do NOT mark CI as green). The PR author or maintainer must decide whether to merge with a known pre-existing failure or fix it first.

- 4. CI outcomes:

- - **Green (all checks pass)**: You're done.

- - **Pre-existing failure confirmed after rerun**: Document in PR, flag to maintainer. Do NOT claim CI is green.

- - **Blocked (job stuck/queued >30 min after cancellation)**: Re-trigger the workflow.

-

- ## Platform-specific notes

-

- ### PowerShell (Windows)

-

- - `gh` CLI may split arguments on colons and parentheses. Use `-F` body-file flag for PR body content:

-

- ```powershell

- $bodyContent = "PR body text here"

- $tmpFile = Join-Path $env:TEMP "pr-body-$(Get-Random).txt"

- try {

- $bodyContent | Out-File -FilePath $tmpFile -Encoding utf8

- gh pr edit $prNumber --body-file $tmpFile

- } finally {

- if (Test-Path $tmpFile) { Remove-Item $tmpFile -Force }

- }

- ```

-

- - Use variable-stored titles to avoid argument splitting:

-

- ```powershell

- $title = "fix(skill-propagation): address PR review findings"

- gh pr edit $prNumber --title $title

- ```

-

- ### CI timeout handling

-

- - GitHub Actions Ubuntu runners may time out on long test runs (~20 minutes). This is typically an infrastructure issue, not a code problem.

- - If a CI check times out: rerun the failed job once before investigating code changes.

- - If the rerun passes in normal time (under 5 minutes), it was infrastructure.

- ## Decision log template

- Track your validation decisions for auditability:

- ```markdown

- ## Finding Validation Log

- | ID | Reviewer Severity | My Verdict | Justification |

- |----|------------------|------------|---------------|

- | C-01 | HIGH | VALID | Line 121 uses raw path.join without validation |

- | C-02 | MEDIUM | DOWNGRADE→LOW | Empty catch is in a non-critical path; warn() is sufficient |

- | S-01 | HIGH | INVALID | Line 246 already calls resolveLogPath() which uses validateSwarmPath() |

- | T-01 | LOW | VALID | No existing test for validated path resolution in tail-read |

- | W-01 | LOW | VALID | "auto-enriches" is inaccurate; should be "advises on skill selection" |

- ```