From f3bf8238528be40088abe7a36f1ddf393085a39e Mon Sep 17 00:00:00 2001 From: Sasa Junuzovic Date: Sun, 5 Apr 2026 17:41:10 -0700 Subject: [PATCH 1/2] fix: add fix-forward to responder and pre-push self-review to implementer (#520) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Responder: new step 5 — after addressing review comments, identify the class of each issue and scan all changed files for other instances of the same problem. Prevents the same class being flagged in the next round. Implementer: new pre-push self-review — before committing, audit for stale docstrings, weak assertions, dead references, parallel code paths, and naming mismatches. Catches the reviewer's most common findings. Analysis of 32 pipeline PRs showed fix-forward would have saved ~12 review rounds across 9 PRs (28%). In 4 additional PRs, implementer-side review would have prevented round 1 comments entirely. Closes #520 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/issue-implementer.md | 8 ++++++++ .github/workflows/review-responder.md | 14 +++++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/.github/workflows/issue-implementer.md b/.github/workflows/issue-implementer.md index b3305499..4203c1c5 100644 --- a/.github/workflows/issue-implementer.md +++ b/.github/workflows/issue-implementer.md @@ -57,4 +57,12 @@ make fix && make check Fix any remaining errors and iterate until `make check` passes cleanly. +**Pre-push self-review**: Before committing, audit your own changes for common reviewer findings: +- **Docstrings**: Does every changed/new function have an accurate docstring? Do existing docstrings still match the new behavior? +- **Test assertions**: Does each test actually verify what its name/docstring claims? Are assertions specific (exact match, not substring)? +- **Dead references**: If you renamed or removed something, grep for stale references in docs, comments, imports, and tests +- **Parallel code paths**: If you fixed a bug or added a guard, check sibling functions/paths for the same gap +- **Naming consistency**: Do test names, variable names, and comments all match the current behavior? +Fix any issues you find before pushing. + Open a pull request with the fix. The PR title should reference the issue number. The PR body MUST include `Closes #${{ github.event.inputs.issue_number }}` so GitHub auto-closes the issue when the PR merges. Include tests as specified in the issue. The PR must NOT be a draft — open it as a regular PR ready for review. diff --git a/.github/workflows/review-responder.md b/.github/workflows/review-responder.md index 3c2c4ad1..be3a2601 100644 --- a/.github/workflows/review-responder.md +++ b/.github/workflows/review-responder.md @@ -70,10 +70,18 @@ This workflow addresses unresolved review comments on a pull request. c. Make the requested fix in the code d. Reply to the comment thread using `reply_to_pull_request_review_comment` with the comment's `databaseId` as the `comment_id` -5. After addressing all comments, run `make fix` to auto-fix lint and format issues, then run `make check` to verify all checks pass: `make fix && make check` +5. **Fix-forward scan**: After addressing all comments, review what you just fixed and identify the *class* of each issue (e.g., "stale docstring", "missing assertion", "TOCTOU race", "dead code"). For each class, scan ALL files changed in this PR for other instances of the same problem. Common patterns to check: + - If you fixed a stale/inaccurate docstring → audit every docstring in the changed functions + - If you fixed a weak test assertion → check sibling tests for similar assertion gaps + - If you fixed a bug in one code path → check parallel code paths for the same bug + - If you removed dead code → grep for similar dead references elsewhere + - If you fixed a naming mismatch → check all related names/comments for consistency + Fix any additional instances you find. This prevents the reviewer from flagging the same class of issue in the next round. -6. If CI checks fail, fix the issues and re-run until they pass. Do not push broken code. +6. Run `make fix` to auto-fix lint and format issues, then run `make check` to verify all checks pass: `make fix && make check` -7. Push all changes in a single commit with message "fix: address review comments". +7. If CI checks fail, fix the issues and re-run until they pass. Do not push broken code. + +8. Push all changes in a single commit with message "fix: address review comments". If a review comment requests a change that would be architecturally significant or you're unsure about, reply to the thread explaining your concern rather than making the change blindly. From 4f7d7bdb5e73a267b59452bfa25f611f9dd90a05 Mon Sep 17 00:00:00 2001 From: Sasa Junuzovic Date: Sun, 5 Apr 2026 18:05:13 -0700 Subject: [PATCH 2/2] fix: reorder implementer self-review before make check --- .github/workflows/issue-implementer.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/.github/workflows/issue-implementer.md b/.github/workflows/issue-implementer.md index 4203c1c5..3f744591 100644 --- a/.github/workflows/issue-implementer.md +++ b/.github/workflows/issue-implementer.md @@ -49,7 +49,15 @@ Read `.github/copilot-instructions.md` and follow all referenced guidelines for Read all files in the repository. Read issue #${{ github.event.inputs.issue_number }} to understand what needs to be fixed. Implement the fix following the spec in the issue, including any testing requirements. -Before committing, run `make fix` to auto-fix lint and format issues, then run `make check` to verify all checks pass (lint, type check, security, tests): +**Self-review before pushing**: After implementing, audit your own changes for common reviewer findings: +- **Docstrings**: Does every changed/new function have an accurate docstring? Do existing docstrings still match the new behavior? +- **Test assertions**: Does each test actually verify what its name/docstring claims? Are assertions specific (exact match, not substring)? +- **Dead references**: If you renamed or removed something, grep for stale references in docs, comments, imports, and tests +- **Parallel code paths**: If you fixed a bug or added a guard, check sibling functions/paths for the same gap +- **Naming consistency**: Do test names, variable names, and comments all match the current behavior? +Fix any issues you find. + +Then run `make fix` to auto-fix lint and format issues, then run `make check` to verify all checks pass (lint, type check, security, tests): ``` make fix && make check @@ -57,12 +65,4 @@ make fix && make check Fix any remaining errors and iterate until `make check` passes cleanly. -**Pre-push self-review**: Before committing, audit your own changes for common reviewer findings: -- **Docstrings**: Does every changed/new function have an accurate docstring? Do existing docstrings still match the new behavior? -- **Test assertions**: Does each test actually verify what its name/docstring claims? Are assertions specific (exact match, not substring)? -- **Dead references**: If you renamed or removed something, grep for stale references in docs, comments, imports, and tests -- **Parallel code paths**: If you fixed a bug or added a guard, check sibling functions/paths for the same gap -- **Naming consistency**: Do test names, variable names, and comments all match the current behavior? -Fix any issues you find before pushing. - Open a pull request with the fix. The PR title should reference the issue number. The PR body MUST include `Closes #${{ github.event.inputs.issue_number }}` so GitHub auto-closes the issue when the PR merges. Include tests as specified in the issue. The PR must NOT be a draft — open it as a regular PR ready for review.