diff --git a/.github/workflows/downstream-users.lock.yml b/.github/workflows/downstream-users.lock.yml index 8a720acb..003bf2cd 100644 --- a/.github/workflows/downstream-users.lock.yml +++ b/.github/workflows/downstream-users.lock.yml @@ -36,7 +36,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"2af4e8b8cdb1ae88a3f4c13292b8b55bca643a5177cbcba3f01564342aa90e75"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"b309db9c1ea539cc8ed7a88a26d66e9f35d4d455b1b16322f422bd4098679c9b"} name: "Internal: Downstream Users" "on": @@ -883,11 +883,32 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect merge commits + # Find the fork point with the upstream branch to scope the check + upstream_sha = '' + for ref in ['@{upstream}', 'origin/HEAD', 'origin/main']: + r = run(['git', 'merge-base', 'HEAD', ref]) + if r.returncode == 0 and r.stdout.strip(): + upstream_sha = r.stdout.strip() + break + if not upstream_sha: + print(json.dumps({'status': 'error', 'error': 'Unable to determine upstream fork point for merge-commit validation. Fix: ensure remotes are fetched and a tracking branch is set (e.g., `git branch --set-upstream-to origin/`), then rerun ready_to_make_pr.'})) + raise SystemExit(0) + log = run(['git', 'rev-list', '--min-parents=2', f'{upstream_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for PR creation.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... create_pull_request uses git format-patch which breaks on merge commits. Fix: re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], diff --git a/.github/workflows/gh-aw-bug-exterminator.lock.yml b/.github/workflows/gh-aw-bug-exterminator.lock.yml index c399b6ce..bcfe4857 100644 --- a/.github/workflows/gh-aw-bug-exterminator.lock.yml +++ b/.github/workflows/gh-aw-bug-exterminator.lock.yml @@ -37,7 +37,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"218198d7daeb5075e026cb833b2fb8d909f16ed8457059e34022058af9743643"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"48a3e323229dc2b434ab6c1d1f6a1f2ec4aa544cd16acfd50b0eedfea4ccbcc2"} name: "Gh Aw Bug Exterminator" "on": @@ -932,11 +932,32 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect merge commits + # Find the fork point with the upstream branch to scope the check + upstream_sha = '' + for ref in ['@{upstream}', 'origin/HEAD', 'origin/main']: + r = run(['git', 'merge-base', 'HEAD', ref]) + if r.returncode == 0 and r.stdout.strip(): + upstream_sha = r.stdout.strip() + break + if not upstream_sha: + print(json.dumps({'status': 'error', 'error': 'Unable to determine upstream fork point for merge-commit validation. Fix: ensure remotes are fetched and a tracking branch is set (e.g., `git branch --set-upstream-to origin/`), then rerun ready_to_make_pr.'})) + raise SystemExit(0) + log = run(['git', 'rev-list', '--min-parents=2', f'{upstream_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for PR creation.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... create_pull_request uses git format-patch which breaks on merge commits. Fix: re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], diff --git a/.github/workflows/gh-aw-code-duplication-fixer.lock.yml b/.github/workflows/gh-aw-code-duplication-fixer.lock.yml index 6e6b11ee..46506829 100644 --- a/.github/workflows/gh-aw-code-duplication-fixer.lock.yml +++ b/.github/workflows/gh-aw-code-duplication-fixer.lock.yml @@ -37,7 +37,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"20c8f1a7ddd18d5b9e5596b052662c695ce9e398012cb5d02b374481e929efc3"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"35511b1f2b8cca89490cb19ff4b59e8318373c5edd9d4e390682d589ef35fd3e"} name: "Code Duplication Fixer" "on": @@ -934,11 +934,32 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect merge commits + # Find the fork point with the upstream branch to scope the check + upstream_sha = '' + for ref in ['@{upstream}', 'origin/HEAD', 'origin/main']: + r = run(['git', 'merge-base', 'HEAD', ref]) + if r.returncode == 0 and r.stdout.strip(): + upstream_sha = r.stdout.strip() + break + if not upstream_sha: + print(json.dumps({'status': 'error', 'error': 'Unable to determine upstream fork point for merge-commit validation. Fix: ensure remotes are fetched and a tracking branch is set (e.g., `git branch --set-upstream-to origin/`), then rerun ready_to_make_pr.'})) + raise SystemExit(0) + log = run(['git', 'rev-list', '--min-parents=2', f'{upstream_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for PR creation.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... create_pull_request uses git format-patch which breaks on merge commits. Fix: re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], diff --git a/.github/workflows/gh-aw-code-simplifier.lock.yml b/.github/workflows/gh-aw-code-simplifier.lock.yml index 93fb4944..14e3d9f3 100644 --- a/.github/workflows/gh-aw-code-simplifier.lock.yml +++ b/.github/workflows/gh-aw-code-simplifier.lock.yml @@ -37,7 +37,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"1dd217c27d801a10753f45e835ebf26461c5cdb7effaf0d738a18a46c10ad048"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"6138d73dad21c897fd905116ef5d1ca73d766e44210d2eec4392d076babbace0"} name: "Code Simplifier" "on": @@ -949,11 +949,32 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect merge commits + # Find the fork point with the upstream branch to scope the check + upstream_sha = '' + for ref in ['@{upstream}', 'origin/HEAD', 'origin/main']: + r = run(['git', 'merge-base', 'HEAD', ref]) + if r.returncode == 0 and r.stdout.strip(): + upstream_sha = r.stdout.strip() + break + if not upstream_sha: + print(json.dumps({'status': 'error', 'error': 'Unable to determine upstream fork point for merge-commit validation. Fix: ensure remotes are fetched and a tracking branch is set (e.g., `git branch --set-upstream-to origin/`), then rerun ready_to_make_pr.'})) + raise SystemExit(0) + log = run(['git', 'rev-list', '--min-parents=2', f'{upstream_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for PR creation.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... create_pull_request uses git format-patch which breaks on merge commits. Fix: re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], diff --git a/.github/workflows/gh-aw-fragments/pr-context.md b/.github/workflows/gh-aw-fragments/pr-context.md index a859c20b..31f01150 100644 --- a/.github/workflows/gh-aw-fragments/pr-context.md +++ b/.github/workflows/gh-aw-fragments/pr-context.md @@ -9,7 +9,7 @@ steps: mkdir -p /tmp/pr-context # PR metadata - gh pr view "$PR_NUMBER" --json title,body,author,baseRefName,headRefName,url \ + gh pr view "$PR_NUMBER" --json title,body,author,baseRefName,headRefName,headRefOid,url \ > /tmp/pr-context/pr.json # Full diff @@ -109,7 +109,7 @@ steps: | File | Description | | --- | --- | - | `pr.json` | PR metadata — title, body, author, base/head branches, URL | + | `pr.json` | PR metadata — title, body, author, base/head branches, head commit SHA (`headRefOid`), URL | | `pr.diff` | Full unified diff of all changes | | `files.json` | Changed files array — each entry has `filename`, `status`, `additions`, `deletions`, `patch` | | `diffs/.diff` | Per-file diffs — one file per changed file, mirroring the repo path under `diffs/` | diff --git a/.github/workflows/gh-aw-fragments/safe-output-create-pr.md b/.github/workflows/gh-aw-fragments/safe-output-create-pr.md index ce43e47d..771e2401 100644 --- a/.github/workflows/gh-aw-fragments/safe-output-create-pr.md +++ b/.github/workflows/gh-aw-fragments/safe-output-create-pr.md @@ -11,11 +11,32 @@ safe-inputs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect merge commits + # Find the fork point with the upstream branch to scope the check + upstream_sha = '' + for ref in ['@{upstream}', 'origin/HEAD', 'origin/main']: + r = run(['git', 'merge-base', 'HEAD', ref]) + if r.returncode == 0 and r.stdout.strip(): + upstream_sha = r.stdout.strip() + break + if not upstream_sha: + print(json.dumps({'status': 'error', 'error': 'Unable to determine upstream fork point for merge-commit validation. Fix: ensure remotes are fetched and a tracking branch is set (e.g., `git branch --set-upstream-to origin/`), then rerun ready_to_make_pr.'})) + raise SystemExit(0) + log = run(['git', 'rev-list', '--min-parents=2', f'{upstream_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for PR creation.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... create_pull_request uses git format-patch which breaks on merge commits. Fix: re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], diff --git a/.github/workflows/gh-aw-fragments/safe-output-push-to-pr.md b/.github/workflows/gh-aw-fragments/safe-output-push-to-pr.md index cb144c51..eb36a774 100644 --- a/.github/workflows/gh-aw-fragments/safe-output-push-to-pr.md +++ b/.github/workflows/gh-aw-fragments/safe-output-push-to-pr.md @@ -1,7 +1,7 @@ --- safe-inputs: - ready-to-make-pr: - description: "Run the PR readiness checklist before creating or updating a PR" + ready-to-push-to-pr: + description: "Run the PR readiness checklist before pushing to a PR" py: | import os, json, subprocess def find(*paths): @@ -11,11 +11,34 @@ safe-inputs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect history rewrites and merge commits + pr_json_path = '/tmp/pr-context/pr.json' + if os.path.isfile(pr_json_path): + with open(pr_json_path) as f: + pr_data = json.load(f) + pr_head_sha = pr_data.get('headRefOid', '') + if pr_head_sha: + # Check 1: PR head must be an ancestor of HEAD (no rebase/reset) + anc = run(['git', 'merge-base', '--is-ancestor', pr_head_sha, 'HEAD']) + if anc.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'History rewrite detected: the original PR head ({pr_head_sha[:12]}) is not an ancestor of HEAD. This means git rebase, reset, or cherry-pick rewrote history. push_to_pull_request_branch will fail. Fix: run `git reset --hard {pr_head_sha}` to restore the PR branch to its original head, then re-apply your changes as direct file edits and commit as regular commits.'})) + raise SystemExit(0) + # Check 2: no merge commits (multiple parents) since PR head + log = run(['git', 'rev-list', '--min-parents=2', f'{pr_head_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for push.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... push_to_pull_request_branch uses git format-patch which breaks on merge commits. Fix: run `git reset --hard {pr_head_sha}` to restore the PR branch, then re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], @@ -47,14 +70,14 @@ safe-inputs: if pr_template: checklist.append(f'Follow the PR template ({pr_template}) for title, description, and validation notes.') checklist.append('Confirm the requested task is fully completed and validated before creating or pushing PR changes.') if diff_line_count > 0: - checklist.append(f'A diff of your unpushed changes ({diff_line_count} lines) has been saved to `/tmp/self-review/diff.patch` (full diff) and `/tmp/self-review/stat.txt` (summary). Spawn a `code-review` sub-agent via `runSubagent` to review the diff against the codebase. Tell it to read `/tmp/self-review/diff.patch` and the relevant source files, and look for bugs, logic errors, missed edge cases, and style issues. If the sub-agent finds legitimate issues, fix them, commit, and call `ready_to_make_pr` again to regenerate the diff before proceeding.') + checklist.append(f'A diff of your unpushed changes ({diff_line_count} lines) has been saved to `/tmp/self-review/diff.patch` (full diff) and `/tmp/self-review/stat.txt` (summary). Spawn a `code-review` sub-agent via `runSubagent` to review the diff against the codebase. Tell it to read `/tmp/self-review/diff.patch` and the relevant source files, and look for bugs, logic errors, missed edge cases, and style issues. If the sub-agent finds legitimate issues, fix them, commit, and call `ready_to_push_to_pr` again to regenerate the diff before proceeding.') print(json.dumps({'status': 'ok', 'checklist': checklist, 'contributing_guide': contributing, 'pr_template': pr_template, 'diff_line_count': diff_line_count})) safe-outputs: push-to-pull-request-branch: github-token-for-extra-empty-commit: ${{ secrets.EXTRA_COMMIT_GITHUB_TOKEN }} --- -Before calling `push_to_pull_request_branch`, call `ready_to_make_pr` and apply its checklist. +Before calling `push_to_pull_request_branch`, call `ready_to_push_to_pr` and apply its checklist. ## push-to-pull-request-branch Limitations @@ -65,7 +88,7 @@ Before calling `push_to_pull_request_branch`, call `ready_to_make_pr` and apply - You may not submit code that modifies files in `.github/workflows/`. Doing so will cause the submission to be rejected. If asked to modify workflow files, propose the change in a copy placed in a `github/` folder (without the leading period) and note in the PR that the file needs to be relocated by someone with workflow write access. Trying to resolve merge conflicts? Do NOT create merge commits (commits with multiple parents) — `push_to_pull_request_branch` uses `git format-patch` which breaks on merge commits. This means: no `git merge`, no `git rebase`, no `git commit-tree` with multiple `-p` flags. Instead: -1. Use `git merge-tree` or `git diff` to compare the conflicting files between this PR branch and the PR base branch (read from `/tmp/pr-context/pr.json`) +1. Use `git diff HEAD...origin/` (base branch from `/tmp/pr-context/pr.json` field `baseRefName`) to see what the base branch changed in the conflicting files 2. Edit the files directly to incorporate the changes from the base branch 3. Commit the changes as regular (single-parent) commits -4. Use push_to_pull_request_branch to push +4. Once you are done with all of your changes on this branch, call `ready_to_push_to_pr` and then `push_to_pull_request_branch` to push diff --git a/.github/workflows/gh-aw-issue-fixer.lock.yml b/.github/workflows/gh-aw-issue-fixer.lock.yml index 02ac88fd..ce193984 100644 --- a/.github/workflows/gh-aw-issue-fixer.lock.yml +++ b/.github/workflows/gh-aw-issue-fixer.lock.yml @@ -38,7 +38,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"38f1a8915f4a710c85bb4bb481fcd8f5a16fc304c1ec8975935d366c2eac8f04"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"19f326850a01fe8800486e3839057f314a1a2ff0afb9b9b3196aff2a3d7b5d92"} name: "Issue Fixer" "on": @@ -982,11 +982,32 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect merge commits + # Find the fork point with the upstream branch to scope the check + upstream_sha = '' + for ref in ['@{upstream}', 'origin/HEAD', 'origin/main']: + r = run(['git', 'merge-base', 'HEAD', ref]) + if r.returncode == 0 and r.stdout.strip(): + upstream_sha = r.stdout.strip() + break + if not upstream_sha: + print(json.dumps({'status': 'error', 'error': 'Unable to determine upstream fork point for merge-commit validation. Fix: ensure remotes are fetched and a tracking branch is set (e.g., `git branch --set-upstream-to origin/`), then rerun ready_to_make_pr.'})) + raise SystemExit(0) + log = run(['git', 'rev-list', '--min-parents=2', f'{upstream_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for PR creation.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... create_pull_request uses git format-patch which breaks on merge commits. Fix: re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], diff --git a/.github/workflows/gh-aw-mention-in-issue-no-sandbox.lock.yml b/.github/workflows/gh-aw-mention-in-issue-no-sandbox.lock.yml index 6cb5a8df..c2e0d5f7 100644 --- a/.github/workflows/gh-aw-mention-in-issue-no-sandbox.lock.yml +++ b/.github/workflows/gh-aw-mention-in-issue-no-sandbox.lock.yml @@ -39,7 +39,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"5bfcb667b2eba53298d9d9cca9697fe53c865a0630c15bd0e9a4369b6d1f1731"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"0b87d308d3e15d36ce014f6c26b37a29bcc8f7f787c92864df432fa97e3f985a"} name: "Mention in Issue (no sandbox)" "on": @@ -1070,11 +1070,32 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect merge commits + # Find the fork point with the upstream branch to scope the check + upstream_sha = '' + for ref in ['@{upstream}', 'origin/HEAD', 'origin/main']: + r = run(['git', 'merge-base', 'HEAD', ref]) + if r.returncode == 0 and r.stdout.strip(): + upstream_sha = r.stdout.strip() + break + if not upstream_sha: + print(json.dumps({'status': 'error', 'error': 'Unable to determine upstream fork point for merge-commit validation. Fix: ensure remotes are fetched and a tracking branch is set (e.g., `git branch --set-upstream-to origin/`), then rerun ready_to_make_pr.'})) + raise SystemExit(0) + log = run(['git', 'rev-list', '--min-parents=2', f'{upstream_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for PR creation.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... create_pull_request uses git format-patch which breaks on merge commits. Fix: re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], diff --git a/.github/workflows/gh-aw-mention-in-issue.lock.yml b/.github/workflows/gh-aw-mention-in-issue.lock.yml index 575e2462..69e3aa89 100644 --- a/.github/workflows/gh-aw-mention-in-issue.lock.yml +++ b/.github/workflows/gh-aw-mention-in-issue.lock.yml @@ -39,7 +39,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"c8178043454e97abbcd7fd7885e3f0144c1ae947263f00ff78296bfc7792e375"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"9e57139f1dd35552a2b68d05aaba3ccc129668cb5ea581f763fd102f15f4eff2"} name: "Mention in Issue" "on": @@ -1074,11 +1074,32 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect merge commits + # Find the fork point with the upstream branch to scope the check + upstream_sha = '' + for ref in ['@{upstream}', 'origin/HEAD', 'origin/main']: + r = run(['git', 'merge-base', 'HEAD', ref]) + if r.returncode == 0 and r.stdout.strip(): + upstream_sha = r.stdout.strip() + break + if not upstream_sha: + print(json.dumps({'status': 'error', 'error': 'Unable to determine upstream fork point for merge-commit validation. Fix: ensure remotes are fetched and a tracking branch is set (e.g., `git branch --set-upstream-to origin/`), then rerun ready_to_make_pr.'})) + raise SystemExit(0) + log = run(['git', 'rev-list', '--min-parents=2', f'{upstream_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for PR creation.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... create_pull_request uses git format-patch which breaks on merge commits. Fix: re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], diff --git a/.github/workflows/gh-aw-mention-in-pr-by-id.lock.yml b/.github/workflows/gh-aw-mention-in-pr-by-id.lock.yml index 16d73ca7..f4fa5236 100644 --- a/.github/workflows/gh-aw-mention-in-pr-by-id.lock.yml +++ b/.github/workflows/gh-aw-mention-in-pr-by-id.lock.yml @@ -43,7 +43,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"f043fc0691132592cc78f7aa21ce5968eddce5733a24562b6dd4ee676e32744c"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"8eaf0ee006b4e1a7ed22d3c143762af9f6f68663ee2d39b0826a601d7d9f06ce"} name: "Mention in PR by ID" "on": @@ -426,7 +426,7 @@ jobs: **Do NOT** describe what the PR does, list the files you reviewed, summarize inline comments, or restate prior review feedback. The PR author already knows what their PR does. Your inline comments already contain all the detail. The review body exists solely to communicate the approve/request-changes decision and important/critical feedback that cannot be covered in inline comments. GH_AW_PROMPT_EOF cat << 'GH_AW_PROMPT_EOF' - Before calling `push_to_pull_request_branch`, call `ready_to_make_pr` and apply its checklist. + Before calling `push_to_pull_request_branch`, call `ready_to_push_to_pr` and apply its checklist. ## push-to-pull-request-branch Limitations @@ -437,10 +437,10 @@ jobs: - You may not submit code that modifies files in `.github/workflows/`. Doing so will cause the submission to be rejected. If asked to modify workflow files, propose the change in a copy placed in a `github/` folder (without the leading period) and note in the PR that the file needs to be relocated by someone with workflow write access. Trying to resolve merge conflicts? Do NOT create merge commits (commits with multiple parents) — `push_to_pull_request_branch` uses `git format-patch` which breaks on merge commits. This means: no `git merge`, no `git rebase`, no `git commit-tree` with multiple `-p` flags. Instead: - 1. Use `git merge-tree` or `git diff` to compare the conflicting files between this PR branch and the PR base branch (read from `/tmp/pr-context/pr.json`) + 1. Use `git diff HEAD...origin/` (base branch from `/tmp/pr-context/pr.json` field `baseRefName`) to see what the base branch changed in the conflicting files 2. Edit the files directly to incorporate the changes from the base branch 3. Commit the changes as regular (single-parent) commits - 4. Use push_to_pull_request_branch to push + 4. Once you are done with all of your changes on this branch, call `ready_to_push_to_pr` and then `push_to_pull_request_branch` to push GH_AW_PROMPT_EOF cat << 'GH_AW_PROMPT_EOF' ## resolve-pull-request-review-thread Limitations @@ -652,7 +652,7 @@ jobs: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ github.event.pull_request.number || inputs.target-pr-number || github.event.issue.number }} name: Fetch PR context to disk - run: "set -euo pipefail\nmkdir -p /tmp/pr-context\n\n# PR metadata\ngh pr view \"$PR_NUMBER\" --json title,body,author,baseRefName,headRefName,url \\\n > /tmp/pr-context/pr.json\n\n# Full diff\nif ! gh pr diff \"$PR_NUMBER\" > /tmp/pr-context/pr.diff; then\n echo \"::warning::Failed to fetch full PR diff; per-file diffs from files.json are still available.\"\n : > /tmp/pr-context/pr.diff\nfi\n\n# Changed files list (--paginate may output concatenated arrays; jq -s 'add' merges them)\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/files\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/files.json\n\n# Per-file diffs\njq -c '.[]' /tmp/pr-context/files.json | while IFS= read -r entry; do\n filename=$(echo \"$entry\" | jq -r '.filename')\n mkdir -p \"/tmp/pr-context/diffs/$(dirname \"$filename\")\"\n echo \"$entry\" | jq -r '.patch // empty' > \"/tmp/pr-context/diffs/${filename}.diff\"\ndone\n\n# File orderings for sub-agent review (3 strategies)\njq -r '[.[] | .filename] | sort | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_az.txt\njq -r '[.[] | .filename] | sort | reverse | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_za.txt\njq -r '[.[] | {filename, size: ((.additions // 0) + (.deletions // 0))}] | sort_by(-.size) | .[].filename' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_largest.txt\n\n# Existing reviews\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/reviews.json\n\n# Review threads with resolution status (GraphQL — REST lacks isResolved/isOutdated)\ngh api graphql --paginate -f query='\n query($owner: String!, $repo: String!, $number: Int!, $endCursor: String) {\n repository(owner: $owner, name: $repo) {\n pullRequest(number: $number) {\n reviewThreads(first: 100, after: $endCursor) {\n pageInfo { hasNextPage endCursor }\n nodes {\n id\n isResolved\n isOutdated\n isCollapsed\n path\n line\n startLine\n comments(first: 100) {\n nodes {\n id\n databaseId\n body\n author { login }\n createdAt\n }\n }\n }\n }\n }\n }\n }\n' -F owner=\"${GITHUB_REPOSITORY%/*}\" -F repo=\"${GITHUB_REPOSITORY#*/}\" -F \"number=$PR_NUMBER\" \\\n --jq '.data.repository.pullRequest.reviewThreads.nodes' \\\n | jq -s 'add // []' > /tmp/pr-context/review_comments.json\n\n# Per-file review threads (mirrors diffs/ structure)\njq -c '.[]' /tmp/pr-context/review_comments.json | while IFS= read -r thread; do\n filepath=$(echo \"$thread\" | jq -r '.path // empty')\n [ -z \"$filepath\" ] && continue\n mkdir -p \"/tmp/pr-context/threads/$(dirname \"$filepath\")\"\n echo \"$thread\" >> \"/tmp/pr-context/threads/${filepath}.jsonl\"\ndone\n# Convert per-file JSONL to proper JSON arrays\nmkdir -p /tmp/pr-context/threads\nfind /tmp/pr-context/threads -name '*.jsonl' 2>/dev/null | while IFS= read -r jsonl; do\n jq -s '.' \"$jsonl\" > \"${jsonl%.jsonl}.json\"\n rm \"$jsonl\"\ndone\n\n# PR discussion comments\ngh api \"repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/comments\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/comments.json\n\n# Linked issues\njq -r '.body // \"\"' /tmp/pr-context/pr.json 2>/dev/null \\\n | grep -oiE '(fixes|closes|resolves)\\s+#[0-9]+' \\\n | grep -oE '[0-9]+$' \\\n | sort -u \\\n | while read -r issue; do\n gh api \"repos/$GITHUB_REPOSITORY/issues/$issue\" > \"/tmp/pr-context/issue-${issue}.json\" || true\n done || true\n\n# Write manifest\ncat > /tmp/pr-context/README.md << 'MANIFEST'\n# PR Context\n\nPre-fetched PR data. All files are in `/tmp/pr-context/`.\n\n| File | Description |\n| --- | --- |\n| `pr.json` | PR metadata — title, body, author, base/head branches, URL |\n| `pr.diff` | Full unified diff of all changes |\n| `files.json` | Changed files array — each entry has `filename`, `status`, `additions`, `deletions`, `patch` |\n| `diffs/.diff` | Per-file diffs — one file per changed file, mirroring the repo path under `diffs/` |\n| `file_order_az.txt` | Changed files sorted alphabetically (A→Z), one filename per line |\n| `file_order_za.txt` | Changed files sorted reverse-alphabetically (Z→A), one filename per line |\n| `file_order_largest.txt` | Changed files sorted by diff size descending (largest first), one filename per line |\n| `reviews.json` | Prior review submissions — author, state (APPROVED/CHANGES_REQUESTED/COMMENTED), body |\n| `review_comments.json` | All review threads (GraphQL) — each thread has `id` (node ID for resolving), `isResolved`, `isOutdated`, `path`, `line`, and nested `comments` with `id`, `databaseId` (numeric REST ID for replies), body/author |\n| `threads/.json` | Per-file review threads — one file per changed file with existing threads, mirroring the repo path under `threads/` |\n| `comments.json` | PR discussion comments (not inline) |\n| `issue-{N}.json` | Linked issue details (one file per linked issue, if any) |\n| `agents.md` | Repository conventions from `generate_agents_md` (if written by agent) |\n| `review-instructions.md` | Review instructions, criteria, and calibration examples (if written by review-process fragment) |\nMANIFEST\n\necho \"PR context written to /tmp/pr-context/\"\nls -la /tmp/pr-context/" + run: "set -euo pipefail\nmkdir -p /tmp/pr-context\n\n# PR metadata\ngh pr view \"$PR_NUMBER\" --json title,body,author,baseRefName,headRefName,headRefOid,url \\\n > /tmp/pr-context/pr.json\n\n# Full diff\nif ! gh pr diff \"$PR_NUMBER\" > /tmp/pr-context/pr.diff; then\n echo \"::warning::Failed to fetch full PR diff; per-file diffs from files.json are still available.\"\n : > /tmp/pr-context/pr.diff\nfi\n\n# Changed files list (--paginate may output concatenated arrays; jq -s 'add' merges them)\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/files\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/files.json\n\n# Per-file diffs\njq -c '.[]' /tmp/pr-context/files.json | while IFS= read -r entry; do\n filename=$(echo \"$entry\" | jq -r '.filename')\n mkdir -p \"/tmp/pr-context/diffs/$(dirname \"$filename\")\"\n echo \"$entry\" | jq -r '.patch // empty' > \"/tmp/pr-context/diffs/${filename}.diff\"\ndone\n\n# File orderings for sub-agent review (3 strategies)\njq -r '[.[] | .filename] | sort | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_az.txt\njq -r '[.[] | .filename] | sort | reverse | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_za.txt\njq -r '[.[] | {filename, size: ((.additions // 0) + (.deletions // 0))}] | sort_by(-.size) | .[].filename' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_largest.txt\n\n# Existing reviews\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/reviews.json\n\n# Review threads with resolution status (GraphQL — REST lacks isResolved/isOutdated)\ngh api graphql --paginate -f query='\n query($owner: String!, $repo: String!, $number: Int!, $endCursor: String) {\n repository(owner: $owner, name: $repo) {\n pullRequest(number: $number) {\n reviewThreads(first: 100, after: $endCursor) {\n pageInfo { hasNextPage endCursor }\n nodes {\n id\n isResolved\n isOutdated\n isCollapsed\n path\n line\n startLine\n comments(first: 100) {\n nodes {\n id\n databaseId\n body\n author { login }\n createdAt\n }\n }\n }\n }\n }\n }\n }\n' -F owner=\"${GITHUB_REPOSITORY%/*}\" -F repo=\"${GITHUB_REPOSITORY#*/}\" -F \"number=$PR_NUMBER\" \\\n --jq '.data.repository.pullRequest.reviewThreads.nodes' \\\n | jq -s 'add // []' > /tmp/pr-context/review_comments.json\n\n# Per-file review threads (mirrors diffs/ structure)\njq -c '.[]' /tmp/pr-context/review_comments.json | while IFS= read -r thread; do\n filepath=$(echo \"$thread\" | jq -r '.path // empty')\n [ -z \"$filepath\" ] && continue\n mkdir -p \"/tmp/pr-context/threads/$(dirname \"$filepath\")\"\n echo \"$thread\" >> \"/tmp/pr-context/threads/${filepath}.jsonl\"\ndone\n# Convert per-file JSONL to proper JSON arrays\nmkdir -p /tmp/pr-context/threads\nfind /tmp/pr-context/threads -name '*.jsonl' 2>/dev/null | while IFS= read -r jsonl; do\n jq -s '.' \"$jsonl\" > \"${jsonl%.jsonl}.json\"\n rm \"$jsonl\"\ndone\n\n# PR discussion comments\ngh api \"repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/comments\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/comments.json\n\n# Linked issues\njq -r '.body // \"\"' /tmp/pr-context/pr.json 2>/dev/null \\\n | grep -oiE '(fixes|closes|resolves)\\s+#[0-9]+' \\\n | grep -oE '[0-9]+$' \\\n | sort -u \\\n | while read -r issue; do\n gh api \"repos/$GITHUB_REPOSITORY/issues/$issue\" > \"/tmp/pr-context/issue-${issue}.json\" || true\n done || true\n\n# Write manifest\ncat > /tmp/pr-context/README.md << 'MANIFEST'\n# PR Context\n\nPre-fetched PR data. All files are in `/tmp/pr-context/`.\n\n| File | Description |\n| --- | --- |\n| `pr.json` | PR metadata — title, body, author, base/head branches, head commit SHA (`headRefOid`), URL |\n| `pr.diff` | Full unified diff of all changes |\n| `files.json` | Changed files array — each entry has `filename`, `status`, `additions`, `deletions`, `patch` |\n| `diffs/.diff` | Per-file diffs — one file per changed file, mirroring the repo path under `diffs/` |\n| `file_order_az.txt` | Changed files sorted alphabetically (A→Z), one filename per line |\n| `file_order_za.txt` | Changed files sorted reverse-alphabetically (Z→A), one filename per line |\n| `file_order_largest.txt` | Changed files sorted by diff size descending (largest first), one filename per line |\n| `reviews.json` | Prior review submissions — author, state (APPROVED/CHANGES_REQUESTED/COMMENTED), body |\n| `review_comments.json` | All review threads (GraphQL) — each thread has `id` (node ID for resolving), `isResolved`, `isOutdated`, `path`, `line`, and nested `comments` with `id`, `databaseId` (numeric REST ID for replies), body/author |\n| `threads/.json` | Per-file review threads — one file per changed file with existing threads, mirroring the repo path under `threads/` |\n| `comments.json` | PR discussion comments (not inline) |\n| `issue-{N}.json` | Linked issue details (one file per linked issue, if any) |\n| `agents.md` | Repository conventions from `generate_agents_md` (if written by agent) |\n| `review-instructions.md` | Review instructions, criteria, and calibration examples (if written by review-process fragment) |\nMANIFEST\n\necho \"PR context written to /tmp/pr-context/\"\nls -la /tmp/pr-context/" - name: Write review instructions to disk run: "mkdir -p /tmp/pr-context\ncat > /tmp/pr-context/review-instructions.md << 'REVIEW_EOF'\n# Review Instructions for Sub-agents\n\nYou are a code review sub-agent. Read these instructions, then review the PR files in the order provided in your prompt.\n\n## Context\n\nBefore reviewing files, read these to understand the PR:\n\n1. `/tmp/pr-context/pr.json` — PR title, description, author, and branches. Understand what the PR is trying to accomplish.\n2. `/tmp/pr-context/agents.md` — Repository coding conventions and guidelines (if it exists).\n3. `/tmp/pr-context/review_comments.json` — Existing review threads. Note which files already have threads so you don't duplicate.\n4. `/tmp/pr-context/issue-*.json` — Linked issue details (if any). Understand the motivation and acceptance criteria.\n\n## Process\n\nReview the PR diff file by file in your assigned order. For each changed file:\n\n1. **Read the diff** for this file from `/tmp/pr-context/diffs/.diff` to understand what changed. If the diff is empty or truncated (e.g., binary files or very large changes), fall back to reading the full file from the workspace and comparing against context.\n2. **Read the full file from the workspace.** The PR branch is checked out locally — open the file directly to get complete contents with line numbers.\n3. **Check existing threads** for this file from `/tmp/pr-context/threads/.json` (if it exists). Skip issues that are already under discussion — each thread has `isResolved` and `isOutdated` fields.\n4. **Identify potential issues** matching the review criteria below.\n5. **Quick-check each issue** before including it:\n - What specific code pattern or change triggers this concern?\n - Is there an obvious guard, handler, or mitigation visible in the immediate context?\n - Can you describe a concrete failure scenario (the `evidence` field)? If you cannot articulate what specific input or state triggers the problem, drop the finding.\n - If the issue is clearly handled, skip it. If you're unsure, include it — the parent will verify.\n6. **Add to your findings list.** Do NOT leave inline comments — you don't have that tool. Return findings in this format:\n\n```\n- file: path/to/file\n line: 42\n severity: HIGH\n title: Brief title\n description: What the issue is and why it matters\n evidence: The specific code pattern and failure scenario\n suggestion: corrected code here (optional — only if you can provide a concrete fix)\n```\n\n**Review every file in your assigned order.** Files reviewed earlier get more attention, which is why different sub-agents use different orderings.\n\n**Check existing threads** — per-file threads are at `/tmp/pr-context/threads/.json` (step 3 above). The full list is at `/tmp/pr-context/review_comments.json`. Do not flag issues that are already under discussion (resolved or unresolved). For outdated threads, only re-flag if the issue still applies to the current diff.\n\n**Return your full findings list** when done, or an empty list if no issues were found.\n\n## Review Criteria\n\nFocus on these categories in priority order:\n\n1. Security vulnerabilities (injection, XSS, auth bypass, secrets exposure)\n2. Logic bugs that could cause runtime failures or incorrect behavior\n3. Data integrity issues (race conditions, missing transactions, corruption risk)\n4. Performance bottlenecks (N+1 queries, memory leaks, blocking operations)\n5. Error handling gaps (unhandled exceptions, missing validation)\n6. Breaking changes to public APIs without migration path\n7. Missing or incorrect test coverage for critical paths\n\n## What NOT to Flag\n\nOnly review the diff — do not flag issues in unchanged code, pre-existing problems not introduced by this PR, or style preferences handled by linters or formatters.\n\n**Common false positives** — these patterns look like issues but usually aren't. Before flagging anything in these categories, confirm the problem is real by reading the surrounding code:\n\n- **Security — input already sanitized:** Don't flag injection or XSS risks when inputs are sanitized upstream, parameterized queries are used, or the framework auto-escapes output.\n- **Null/undefined — guarded elsewhere:** Don't flag potential null dereferences if the value is guaranteed by a type guard, assertion, schema validation, or upstream null check.\n- **Error handling — handled at a different layer:** Don't flag missing try/catch if the caller, middleware, or framework catches and handles the error (e.g., Express error middleware, React error boundaries).\n- **Performance — theoretical, not practical:** Don't flag algorithmic complexity (e.g., O(n^2)) unless N is demonstrably large enough to matter in the actual usage context. \"This could be slow\" without evidence is not actionable.\n- **Validation — exists at another layer:** Don't flag missing input validation if it's handled by an API gateway, middleware, schema validator, or type system.\n- **Test coverage — trivial or generated code:** Don't flag missing tests for trivial getters/setters, auto-generated code, or simple delegation methods.\n- **Style or naming — not in coding guidelines:** Don't flag naming conventions or code style unless they violate the repository's documented coding guidelines (from `generate_agents_md` or CONTRIBUTING docs).\n\n**Existing review threads** — check BEFORE flagging any issue:\n\n- **Resolved with reviewer reply** (e.g. \"This is intentional\") — reviewer's decision is final. Do NOT re-flag.\n- **Resolved without reply** — author likely fixed it. Do NOT re-raise unless the fix introduced a new problem.\n- **Unresolved** — already flagged. Do NOT duplicate.\n- **Outdated** — only re-flag if the issue still applies to the current diff.\n\nWhen in doubt, do not duplicate. Redundant comments erode trust.\n\nFinding no issues is a valid and valuable outcome. An empty findings list is better than findings that waste the author's time or erode trust. Do not manufacture findings to justify your review — if the code is sound, return an empty list.\n\n## Severity Classification\n\nDetermine severity AFTER investigating the issue, not before. First identify the problem and trace through the code, then assign a severity based on the evidence you found.\n\n- 🔴 CRITICAL — Must fix before merge (security vulnerabilities, data corruption, production-breaking bugs)\n- 🟠 HIGH — Should fix before merge (logic errors, missing validation, significant performance issues)\n- 🟡 MEDIUM — Address soon, non-blocking (error handling gaps, suboptimal patterns, missing edge cases)\n- ⚪ LOW — Author discretion (minor improvements, documentation gaps)\n- 💬 NITPICK — Truly optional (stylistic preferences, alternative approaches)\n\n## Review Intensity\n\nThe review intensity is `${{ inputs.intensity || 'balanced' }}`.\n\n- **conservative**: High evidence bar. Only flag when you can demonstrate a concrete failure scenario. If you can construct a reasonable counterargument, do not flag. Approval with zero findings is the expected outcome for most PRs.\n- **balanced**: Standard evidence bar. Flag when you can point to specific code that would fail. If the issue is ambiguous, lean toward not flagging.\n- **aggressive**: Lower evidence bar. Flag when evidence exists even if the failure scenario is not fully confirmed. Improvement suggestions welcome but must cite specific code.\n\n## Calibration Examples\n\nUse these examples to calibrate your judgment. Each pair shows a real issue and a similar-looking pattern that is NOT an issue.\n\n### Example 1: Null/Undefined Access\n\n**True positive — flag this:**\n\n```js\n// PR adds this handler\napp.get('/user/:id', async (req, res) => {\n const user = await db.findUser(req.params.id);\n res.json({ name: user.name, email: user.email });\n});\n```\n\nWhy flag: `db.findUser()` can return `null` when no user matches the ID. Accessing `user.name` will throw a TypeError at runtime. No upstream guard exists — the route handler is the entry point.\n\n**False positive — do NOT flag this:**\n\n```ts\n// PR adds this line inside an existing function\nconst settings = user.getSettings();\n```\n\nWhy skip: Reading the full file reveals `user` is typed as `User` (not `User | null`), and the calling function only runs after `authenticateUser()` middleware which guarantees a valid user object. The null case is handled at a different layer.\n\n### Example 2: SQL Injection\n\n**True positive — flag this:**\n\n```python\n# PR adds this query\ncursor.execute(f\"SELECT * FROM orders WHERE customer_id = '{customer_id}'\")\n```\n\nWhy flag: String interpolation in a SQL query with user-controlled input (`customer_id` comes from the request). No parameterization or sanitization anywhere in the call chain.\n\n**False positive — do NOT flag this:**\n\n```python\n# PR adds this query\ncursor.execute(f\"SELECT * FROM orders WHERE status = '{OrderStatus.PENDING.value}'\")\n```\n\nWhy skip: The interpolated value is a hardcoded enum constant (`OrderStatus.PENDING`), not user input. There is no injection vector.\n\n### Example 3: Borderline — Do NOT Flag\n\n```go\n// PR adds this function\nfunc processItems(items []Item) []Result {\n results := make([]Result, 0)\n for _, item := range items {\n for _, tag := range item.Tags {\n results = append(results, process(item, tag))\n }\n }\n return results\n}\n```\n\nThis looks like an O(n*m) performance concern. But without evidence that `items` or `Tags` are large in practice, this is speculative. The function processes a bounded dataset (items from a single user request). Do not flag theoretical performance issues without evidence of real-world impact.\nREVIEW_EOF" - env: @@ -1175,13 +1175,13 @@ jobs: "logDir": "/opt/gh-aw/safe-inputs/logs", "tools": [ { - "name": "ready-to-make-pr", - "description": "Run the PR readiness checklist before creating or updating a PR", + "name": "ready-to-push-to-pr", + "description": "Run the PR readiness checklist before pushing to a PR", "inputSchema": { "properties": {}, "type": "object" }, - "handler": "ready-to-make-pr.py", + "handler": "ready-to-push-to-pr.py", "timeout": 60 } ] @@ -1206,10 +1206,10 @@ jobs: - name: Setup Safe Inputs Tool Files run: | - cat > /opt/gh-aw/safe-inputs/ready-to-make-pr.py << 'GH_AW_SAFE_INPUTS_PY_READY-TO-MAKE-PR_EOF' + cat > /opt/gh-aw/safe-inputs/ready-to-push-to-pr.py << 'GH_AW_SAFE_INPUTS_PY_READY-TO-PUSH-TO-PR_EOF' #!/usr/bin/env python3 - # Auto-generated safe-input tool: ready-to-make-pr - # Run the PR readiness checklist before creating or updating a PR + # Auto-generated safe-input tool: ready-to-push-to-pr + # Run the PR readiness checklist before pushing to a PR import json import os @@ -1230,11 +1230,34 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect history rewrites and merge commits + pr_json_path = '/tmp/pr-context/pr.json' + if os.path.isfile(pr_json_path): + with open(pr_json_path) as f: + pr_data = json.load(f) + pr_head_sha = pr_data.get('headRefOid', '') + if pr_head_sha: + # Check 1: PR head must be an ancestor of HEAD (no rebase/reset) + anc = run(['git', 'merge-base', '--is-ancestor', pr_head_sha, 'HEAD']) + if anc.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'History rewrite detected: the original PR head ({pr_head_sha[:12]}) is not an ancestor of HEAD. This means git rebase, reset, or cherry-pick rewrote history. push_to_pull_request_branch will fail. Fix: run `git reset --hard {pr_head_sha}` to restore the PR branch to its original head, then re-apply your changes as direct file edits and commit as regular commits.'})) + raise SystemExit(0) + # Check 2: no merge commits (multiple parents) since PR head + log = run(['git', 'rev-list', '--min-parents=2', f'{pr_head_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for push.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... push_to_pull_request_branch uses git format-patch which breaks on merge commits. Fix: run `git reset --hard {pr_head_sha}` to restore the PR branch, then re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], @@ -1266,12 +1289,12 @@ jobs: if pr_template: checklist.append(f'Follow the PR template ({pr_template}) for title, description, and validation notes.') checklist.append('Confirm the requested task is fully completed and validated before creating or pushing PR changes.') if diff_line_count > 0: - checklist.append(f'A diff of your unpushed changes ({diff_line_count} lines) has been saved to `/tmp/self-review/diff.patch` (full diff) and `/tmp/self-review/stat.txt` (summary). Spawn a `code-review` sub-agent via `runSubagent` to review the diff against the codebase. Tell it to read `/tmp/self-review/diff.patch` and the relevant source files, and look for bugs, logic errors, missed edge cases, and style issues. If the sub-agent finds legitimate issues, fix them, commit, and call `ready_to_make_pr` again to regenerate the diff before proceeding.') + checklist.append(f'A diff of your unpushed changes ({diff_line_count} lines) has been saved to `/tmp/self-review/diff.patch` (full diff) and `/tmp/self-review/stat.txt` (summary). Spawn a `code-review` sub-agent via `runSubagent` to review the diff against the codebase. Tell it to read `/tmp/self-review/diff.patch` and the relevant source files, and look for bugs, logic errors, missed edge cases, and style issues. If the sub-agent finds legitimate issues, fix them, commit, and call `ready_to_push_to_pr` again to regenerate the diff before proceeding.') print(json.dumps({'status': 'ok', 'checklist': checklist, 'contributing_guide': contributing, 'pr_template': pr_template, 'diff_line_count': diff_line_count})) - GH_AW_SAFE_INPUTS_PY_READY-TO-MAKE-PR_EOF - chmod +x /opt/gh-aw/safe-inputs/ready-to-make-pr.py + GH_AW_SAFE_INPUTS_PY_READY-TO-PUSH-TO-PR_EOF + chmod +x /opt/gh-aw/safe-inputs/ready-to-push-to-pr.py - name: Generate Safe Inputs MCP Server Config id: safe-inputs-config diff --git a/.github/workflows/gh-aw-mention-in-pr-no-sandbox.lock.yml b/.github/workflows/gh-aw-mention-in-pr-no-sandbox.lock.yml index 4a37f85e..0e971bf5 100644 --- a/.github/workflows/gh-aw-mention-in-pr-no-sandbox.lock.yml +++ b/.github/workflows/gh-aw-mention-in-pr-no-sandbox.lock.yml @@ -44,7 +44,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"764aa68cced612fafebc99df7d93376341b5031834e5c0e6363a66671047ebef"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"37eed9452f2bf3443bda29742805f3e384dcd059772d5238c9aa7c0737a31c4a"} name: "Mention in PR (no sandbox)" "on": @@ -442,7 +442,7 @@ jobs: **Do NOT** describe what the PR does, list the files you reviewed, summarize inline comments, or restate prior review feedback. The PR author already knows what their PR does. Your inline comments already contain all the detail. The review body exists solely to communicate the approve/request-changes decision and important/critical feedback that cannot be covered in inline comments. GH_AW_PROMPT_EOF cat << 'GH_AW_PROMPT_EOF' - Before calling `push_to_pull_request_branch`, call `ready_to_make_pr` and apply its checklist. + Before calling `push_to_pull_request_branch`, call `ready_to_push_to_pr` and apply its checklist. ## push-to-pull-request-branch Limitations @@ -453,10 +453,10 @@ jobs: - You may not submit code that modifies files in `.github/workflows/`. Doing so will cause the submission to be rejected. If asked to modify workflow files, propose the change in a copy placed in a `github/` folder (without the leading period) and note in the PR that the file needs to be relocated by someone with workflow write access. Trying to resolve merge conflicts? Do NOT create merge commits (commits with multiple parents) — `push_to_pull_request_branch` uses `git format-patch` which breaks on merge commits. This means: no `git merge`, no `git rebase`, no `git commit-tree` with multiple `-p` flags. Instead: - 1. Use `git merge-tree` or `git diff` to compare the conflicting files between this PR branch and the PR base branch (read from `/tmp/pr-context/pr.json`) + 1. Use `git diff HEAD...origin/` (base branch from `/tmp/pr-context/pr.json` field `baseRefName`) to see what the base branch changed in the conflicting files 2. Edit the files directly to incorporate the changes from the base branch 3. Commit the changes as regular (single-parent) commits - 4. Use push_to_pull_request_branch to push + 4. Once you are done with all of your changes on this branch, call `ready_to_push_to_pr` and then `push_to_pull_request_branch` to push GH_AW_PROMPT_EOF cat << 'GH_AW_PROMPT_EOF' ## resolve-pull-request-review-thread Limitations @@ -726,7 +726,7 @@ jobs: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ github.event.pull_request.number || inputs.target-pr-number || github.event.issue.number }} name: Fetch PR context to disk - run: "set -euo pipefail\nmkdir -p /tmp/pr-context\n\n# PR metadata\ngh pr view \"$PR_NUMBER\" --json title,body,author,baseRefName,headRefName,url \\\n > /tmp/pr-context/pr.json\n\n# Full diff\nif ! gh pr diff \"$PR_NUMBER\" > /tmp/pr-context/pr.diff; then\n echo \"::warning::Failed to fetch full PR diff; per-file diffs from files.json are still available.\"\n : > /tmp/pr-context/pr.diff\nfi\n\n# Changed files list (--paginate may output concatenated arrays; jq -s 'add' merges them)\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/files\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/files.json\n\n# Per-file diffs\njq -c '.[]' /tmp/pr-context/files.json | while IFS= read -r entry; do\n filename=$(echo \"$entry\" | jq -r '.filename')\n mkdir -p \"/tmp/pr-context/diffs/$(dirname \"$filename\")\"\n echo \"$entry\" | jq -r '.patch // empty' > \"/tmp/pr-context/diffs/${filename}.diff\"\ndone\n\n# File orderings for sub-agent review (3 strategies)\njq -r '[.[] | .filename] | sort | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_az.txt\njq -r '[.[] | .filename] | sort | reverse | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_za.txt\njq -r '[.[] | {filename, size: ((.additions // 0) + (.deletions // 0))}] | sort_by(-.size) | .[].filename' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_largest.txt\n\n# Existing reviews\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/reviews.json\n\n# Review threads with resolution status (GraphQL — REST lacks isResolved/isOutdated)\ngh api graphql --paginate -f query='\n query($owner: String!, $repo: String!, $number: Int!, $endCursor: String) {\n repository(owner: $owner, name: $repo) {\n pullRequest(number: $number) {\n reviewThreads(first: 100, after: $endCursor) {\n pageInfo { hasNextPage endCursor }\n nodes {\n id\n isResolved\n isOutdated\n isCollapsed\n path\n line\n startLine\n comments(first: 100) {\n nodes {\n id\n databaseId\n body\n author { login }\n createdAt\n }\n }\n }\n }\n }\n }\n }\n' -F owner=\"${GITHUB_REPOSITORY%/*}\" -F repo=\"${GITHUB_REPOSITORY#*/}\" -F \"number=$PR_NUMBER\" \\\n --jq '.data.repository.pullRequest.reviewThreads.nodes' \\\n | jq -s 'add // []' > /tmp/pr-context/review_comments.json\n\n# Per-file review threads (mirrors diffs/ structure)\njq -c '.[]' /tmp/pr-context/review_comments.json | while IFS= read -r thread; do\n filepath=$(echo \"$thread\" | jq -r '.path // empty')\n [ -z \"$filepath\" ] && continue\n mkdir -p \"/tmp/pr-context/threads/$(dirname \"$filepath\")\"\n echo \"$thread\" >> \"/tmp/pr-context/threads/${filepath}.jsonl\"\ndone\n# Convert per-file JSONL to proper JSON arrays\nmkdir -p /tmp/pr-context/threads\nfind /tmp/pr-context/threads -name '*.jsonl' 2>/dev/null | while IFS= read -r jsonl; do\n jq -s '.' \"$jsonl\" > \"${jsonl%.jsonl}.json\"\n rm \"$jsonl\"\ndone\n\n# PR discussion comments\ngh api \"repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/comments\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/comments.json\n\n# Linked issues\njq -r '.body // \"\"' /tmp/pr-context/pr.json 2>/dev/null \\\n | grep -oiE '(fixes|closes|resolves)\\s+#[0-9]+' \\\n | grep -oE '[0-9]+$' \\\n | sort -u \\\n | while read -r issue; do\n gh api \"repos/$GITHUB_REPOSITORY/issues/$issue\" > \"/tmp/pr-context/issue-${issue}.json\" || true\n done || true\n\n# Write manifest\ncat > /tmp/pr-context/README.md << 'MANIFEST'\n# PR Context\n\nPre-fetched PR data. All files are in `/tmp/pr-context/`.\n\n| File | Description |\n| --- | --- |\n| `pr.json` | PR metadata — title, body, author, base/head branches, URL |\n| `pr.diff` | Full unified diff of all changes |\n| `files.json` | Changed files array — each entry has `filename`, `status`, `additions`, `deletions`, `patch` |\n| `diffs/.diff` | Per-file diffs — one file per changed file, mirroring the repo path under `diffs/` |\n| `file_order_az.txt` | Changed files sorted alphabetically (A→Z), one filename per line |\n| `file_order_za.txt` | Changed files sorted reverse-alphabetically (Z→A), one filename per line |\n| `file_order_largest.txt` | Changed files sorted by diff size descending (largest first), one filename per line |\n| `reviews.json` | Prior review submissions — author, state (APPROVED/CHANGES_REQUESTED/COMMENTED), body |\n| `review_comments.json` | All review threads (GraphQL) — each thread has `id` (node ID for resolving), `isResolved`, `isOutdated`, `path`, `line`, and nested `comments` with `id`, `databaseId` (numeric REST ID for replies), body/author |\n| `threads/.json` | Per-file review threads — one file per changed file with existing threads, mirroring the repo path under `threads/` |\n| `comments.json` | PR discussion comments (not inline) |\n| `issue-{N}.json` | Linked issue details (one file per linked issue, if any) |\n| `agents.md` | Repository conventions from `generate_agents_md` (if written by agent) |\n| `review-instructions.md` | Review instructions, criteria, and calibration examples (if written by review-process fragment) |\nMANIFEST\n\necho \"PR context written to /tmp/pr-context/\"\nls -la /tmp/pr-context/" + run: "set -euo pipefail\nmkdir -p /tmp/pr-context\n\n# PR metadata\ngh pr view \"$PR_NUMBER\" --json title,body,author,baseRefName,headRefName,headRefOid,url \\\n > /tmp/pr-context/pr.json\n\n# Full diff\nif ! gh pr diff \"$PR_NUMBER\" > /tmp/pr-context/pr.diff; then\n echo \"::warning::Failed to fetch full PR diff; per-file diffs from files.json are still available.\"\n : > /tmp/pr-context/pr.diff\nfi\n\n# Changed files list (--paginate may output concatenated arrays; jq -s 'add' merges them)\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/files\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/files.json\n\n# Per-file diffs\njq -c '.[]' /tmp/pr-context/files.json | while IFS= read -r entry; do\n filename=$(echo \"$entry\" | jq -r '.filename')\n mkdir -p \"/tmp/pr-context/diffs/$(dirname \"$filename\")\"\n echo \"$entry\" | jq -r '.patch // empty' > \"/tmp/pr-context/diffs/${filename}.diff\"\ndone\n\n# File orderings for sub-agent review (3 strategies)\njq -r '[.[] | .filename] | sort | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_az.txt\njq -r '[.[] | .filename] | sort | reverse | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_za.txt\njq -r '[.[] | {filename, size: ((.additions // 0) + (.deletions // 0))}] | sort_by(-.size) | .[].filename' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_largest.txt\n\n# Existing reviews\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/reviews.json\n\n# Review threads with resolution status (GraphQL — REST lacks isResolved/isOutdated)\ngh api graphql --paginate -f query='\n query($owner: String!, $repo: String!, $number: Int!, $endCursor: String) {\n repository(owner: $owner, name: $repo) {\n pullRequest(number: $number) {\n reviewThreads(first: 100, after: $endCursor) {\n pageInfo { hasNextPage endCursor }\n nodes {\n id\n isResolved\n isOutdated\n isCollapsed\n path\n line\n startLine\n comments(first: 100) {\n nodes {\n id\n databaseId\n body\n author { login }\n createdAt\n }\n }\n }\n }\n }\n }\n }\n' -F owner=\"${GITHUB_REPOSITORY%/*}\" -F repo=\"${GITHUB_REPOSITORY#*/}\" -F \"number=$PR_NUMBER\" \\\n --jq '.data.repository.pullRequest.reviewThreads.nodes' \\\n | jq -s 'add // []' > /tmp/pr-context/review_comments.json\n\n# Per-file review threads (mirrors diffs/ structure)\njq -c '.[]' /tmp/pr-context/review_comments.json | while IFS= read -r thread; do\n filepath=$(echo \"$thread\" | jq -r '.path // empty')\n [ -z \"$filepath\" ] && continue\n mkdir -p \"/tmp/pr-context/threads/$(dirname \"$filepath\")\"\n echo \"$thread\" >> \"/tmp/pr-context/threads/${filepath}.jsonl\"\ndone\n# Convert per-file JSONL to proper JSON arrays\nmkdir -p /tmp/pr-context/threads\nfind /tmp/pr-context/threads -name '*.jsonl' 2>/dev/null | while IFS= read -r jsonl; do\n jq -s '.' \"$jsonl\" > \"${jsonl%.jsonl}.json\"\n rm \"$jsonl\"\ndone\n\n# PR discussion comments\ngh api \"repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/comments\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/comments.json\n\n# Linked issues\njq -r '.body // \"\"' /tmp/pr-context/pr.json 2>/dev/null \\\n | grep -oiE '(fixes|closes|resolves)\\s+#[0-9]+' \\\n | grep -oE '[0-9]+$' \\\n | sort -u \\\n | while read -r issue; do\n gh api \"repos/$GITHUB_REPOSITORY/issues/$issue\" > \"/tmp/pr-context/issue-${issue}.json\" || true\n done || true\n\n# Write manifest\ncat > /tmp/pr-context/README.md << 'MANIFEST'\n# PR Context\n\nPre-fetched PR data. All files are in `/tmp/pr-context/`.\n\n| File | Description |\n| --- | --- |\n| `pr.json` | PR metadata — title, body, author, base/head branches, head commit SHA (`headRefOid`), URL |\n| `pr.diff` | Full unified diff of all changes |\n| `files.json` | Changed files array — each entry has `filename`, `status`, `additions`, `deletions`, `patch` |\n| `diffs/.diff` | Per-file diffs — one file per changed file, mirroring the repo path under `diffs/` |\n| `file_order_az.txt` | Changed files sorted alphabetically (A→Z), one filename per line |\n| `file_order_za.txt` | Changed files sorted reverse-alphabetically (Z→A), one filename per line |\n| `file_order_largest.txt` | Changed files sorted by diff size descending (largest first), one filename per line |\n| `reviews.json` | Prior review submissions — author, state (APPROVED/CHANGES_REQUESTED/COMMENTED), body |\n| `review_comments.json` | All review threads (GraphQL) — each thread has `id` (node ID for resolving), `isResolved`, `isOutdated`, `path`, `line`, and nested `comments` with `id`, `databaseId` (numeric REST ID for replies), body/author |\n| `threads/.json` | Per-file review threads — one file per changed file with existing threads, mirroring the repo path under `threads/` |\n| `comments.json` | PR discussion comments (not inline) |\n| `issue-{N}.json` | Linked issue details (one file per linked issue, if any) |\n| `agents.md` | Repository conventions from `generate_agents_md` (if written by agent) |\n| `review-instructions.md` | Review instructions, criteria, and calibration examples (if written by review-process fragment) |\nMANIFEST\n\necho \"PR context written to /tmp/pr-context/\"\nls -la /tmp/pr-context/" - name: Write review instructions to disk run: "mkdir -p /tmp/pr-context\ncat > /tmp/pr-context/review-instructions.md << 'REVIEW_EOF'\n# Review Instructions for Sub-agents\n\nYou are a code review sub-agent. Read these instructions, then review the PR files in the order provided in your prompt.\n\n## Context\n\nBefore reviewing files, read these to understand the PR:\n\n1. `/tmp/pr-context/pr.json` — PR title, description, author, and branches. Understand what the PR is trying to accomplish.\n2. `/tmp/pr-context/agents.md` — Repository coding conventions and guidelines (if it exists).\n3. `/tmp/pr-context/review_comments.json` — Existing review threads. Note which files already have threads so you don't duplicate.\n4. `/tmp/pr-context/issue-*.json` — Linked issue details (if any). Understand the motivation and acceptance criteria.\n\n## Process\n\nReview the PR diff file by file in your assigned order. For each changed file:\n\n1. **Read the diff** for this file from `/tmp/pr-context/diffs/.diff` to understand what changed. If the diff is empty or truncated (e.g., binary files or very large changes), fall back to reading the full file from the workspace and comparing against context.\n2. **Read the full file from the workspace.** The PR branch is checked out locally — open the file directly to get complete contents with line numbers.\n3. **Check existing threads** for this file from `/tmp/pr-context/threads/.json` (if it exists). Skip issues that are already under discussion — each thread has `isResolved` and `isOutdated` fields.\n4. **Identify potential issues** matching the review criteria below.\n5. **Quick-check each issue** before including it:\n - What specific code pattern or change triggers this concern?\n - Is there an obvious guard, handler, or mitigation visible in the immediate context?\n - Can you describe a concrete failure scenario (the `evidence` field)? If you cannot articulate what specific input or state triggers the problem, drop the finding.\n - If the issue is clearly handled, skip it. If you're unsure, include it — the parent will verify.\n6. **Add to your findings list.** Do NOT leave inline comments — you don't have that tool. Return findings in this format:\n\n```\n- file: path/to/file\n line: 42\n severity: HIGH\n title: Brief title\n description: What the issue is and why it matters\n evidence: The specific code pattern and failure scenario\n suggestion: corrected code here (optional — only if you can provide a concrete fix)\n```\n\n**Review every file in your assigned order.** Files reviewed earlier get more attention, which is why different sub-agents use different orderings.\n\n**Check existing threads** — per-file threads are at `/tmp/pr-context/threads/.json` (step 3 above). The full list is at `/tmp/pr-context/review_comments.json`. Do not flag issues that are already under discussion (resolved or unresolved). For outdated threads, only re-flag if the issue still applies to the current diff.\n\n**Return your full findings list** when done, or an empty list if no issues were found.\n\n## Review Criteria\n\nFocus on these categories in priority order:\n\n1. Security vulnerabilities (injection, XSS, auth bypass, secrets exposure)\n2. Logic bugs that could cause runtime failures or incorrect behavior\n3. Data integrity issues (race conditions, missing transactions, corruption risk)\n4. Performance bottlenecks (N+1 queries, memory leaks, blocking operations)\n5. Error handling gaps (unhandled exceptions, missing validation)\n6. Breaking changes to public APIs without migration path\n7. Missing or incorrect test coverage for critical paths\n\n## What NOT to Flag\n\nOnly review the diff — do not flag issues in unchanged code, pre-existing problems not introduced by this PR, or style preferences handled by linters or formatters.\n\n**Common false positives** — these patterns look like issues but usually aren't. Before flagging anything in these categories, confirm the problem is real by reading the surrounding code:\n\n- **Security — input already sanitized:** Don't flag injection or XSS risks when inputs are sanitized upstream, parameterized queries are used, or the framework auto-escapes output.\n- **Null/undefined — guarded elsewhere:** Don't flag potential null dereferences if the value is guaranteed by a type guard, assertion, schema validation, or upstream null check.\n- **Error handling — handled at a different layer:** Don't flag missing try/catch if the caller, middleware, or framework catches and handles the error (e.g., Express error middleware, React error boundaries).\n- **Performance — theoretical, not practical:** Don't flag algorithmic complexity (e.g., O(n^2)) unless N is demonstrably large enough to matter in the actual usage context. \"This could be slow\" without evidence is not actionable.\n- **Validation — exists at another layer:** Don't flag missing input validation if it's handled by an API gateway, middleware, schema validator, or type system.\n- **Test coverage — trivial or generated code:** Don't flag missing tests for trivial getters/setters, auto-generated code, or simple delegation methods.\n- **Style or naming — not in coding guidelines:** Don't flag naming conventions or code style unless they violate the repository's documented coding guidelines (from `generate_agents_md` or CONTRIBUTING docs).\n\n**Existing review threads** — check BEFORE flagging any issue:\n\n- **Resolved with reviewer reply** (e.g. \"This is intentional\") — reviewer's decision is final. Do NOT re-flag.\n- **Resolved without reply** — author likely fixed it. Do NOT re-raise unless the fix introduced a new problem.\n- **Unresolved** — already flagged. Do NOT duplicate.\n- **Outdated** — only re-flag if the issue still applies to the current diff.\n\nWhen in doubt, do not duplicate. Redundant comments erode trust.\n\nFinding no issues is a valid and valuable outcome. An empty findings list is better than findings that waste the author's time or erode trust. Do not manufacture findings to justify your review — if the code is sound, return an empty list.\n\n## Severity Classification\n\nDetermine severity AFTER investigating the issue, not before. First identify the problem and trace through the code, then assign a severity based on the evidence you found.\n\n- 🔴 CRITICAL — Must fix before merge (security vulnerabilities, data corruption, production-breaking bugs)\n- 🟠 HIGH — Should fix before merge (logic errors, missing validation, significant performance issues)\n- 🟡 MEDIUM — Address soon, non-blocking (error handling gaps, suboptimal patterns, missing edge cases)\n- ⚪ LOW — Author discretion (minor improvements, documentation gaps)\n- 💬 NITPICK — Truly optional (stylistic preferences, alternative approaches)\n\n## Review Intensity\n\nThe review intensity is `${{ inputs.intensity || 'balanced' }}`.\n\n- **conservative**: High evidence bar. Only flag when you can demonstrate a concrete failure scenario. If you can construct a reasonable counterargument, do not flag. Approval with zero findings is the expected outcome for most PRs.\n- **balanced**: Standard evidence bar. Flag when you can point to specific code that would fail. If the issue is ambiguous, lean toward not flagging.\n- **aggressive**: Lower evidence bar. Flag when evidence exists even if the failure scenario is not fully confirmed. Improvement suggestions welcome but must cite specific code.\n\n## Calibration Examples\n\nUse these examples to calibrate your judgment. Each pair shows a real issue and a similar-looking pattern that is NOT an issue.\n\n### Example 1: Null/Undefined Access\n\n**True positive — flag this:**\n\n```js\n// PR adds this handler\napp.get('/user/:id', async (req, res) => {\n const user = await db.findUser(req.params.id);\n res.json({ name: user.name, email: user.email });\n});\n```\n\nWhy flag: `db.findUser()` can return `null` when no user matches the ID. Accessing `user.name` will throw a TypeError at runtime. No upstream guard exists — the route handler is the entry point.\n\n**False positive — do NOT flag this:**\n\n```ts\n// PR adds this line inside an existing function\nconst settings = user.getSettings();\n```\n\nWhy skip: Reading the full file reveals `user` is typed as `User` (not `User | null`), and the calling function only runs after `authenticateUser()` middleware which guarantees a valid user object. The null case is handled at a different layer.\n\n### Example 2: SQL Injection\n\n**True positive — flag this:**\n\n```python\n# PR adds this query\ncursor.execute(f\"SELECT * FROM orders WHERE customer_id = '{customer_id}'\")\n```\n\nWhy flag: String interpolation in a SQL query with user-controlled input (`customer_id` comes from the request). No parameterization or sanitization anywhere in the call chain.\n\n**False positive — do NOT flag this:**\n\n```python\n# PR adds this query\ncursor.execute(f\"SELECT * FROM orders WHERE status = '{OrderStatus.PENDING.value}'\")\n```\n\nWhy skip: The interpolated value is a hardcoded enum constant (`OrderStatus.PENDING`), not user input. There is no injection vector.\n\n### Example 3: Borderline — Do NOT Flag\n\n```go\n// PR adds this function\nfunc processItems(items []Item) []Result {\n results := make([]Result, 0)\n for _, item := range items {\n for _, tag := range item.Tags {\n results = append(results, process(item, tag))\n }\n }\n return results\n}\n```\n\nThis looks like an O(n*m) performance concern. But without evidence that `items` or `Tags` are large in practice, this is speculative. The function processes a bounded dataset (items from a single user request). Do not flag theoretical performance issues without evidence of real-world impact.\nREVIEW_EOF" - env: @@ -1279,13 +1279,13 @@ jobs: "logDir": "/opt/gh-aw/safe-inputs/logs", "tools": [ { - "name": "ready-to-make-pr", - "description": "Run the PR readiness checklist before creating or updating a PR", + "name": "ready-to-push-to-pr", + "description": "Run the PR readiness checklist before pushing to a PR", "inputSchema": { "properties": {}, "type": "object" }, - "handler": "ready-to-make-pr.py", + "handler": "ready-to-push-to-pr.py", "timeout": 60 } ] @@ -1310,10 +1310,10 @@ jobs: - name: Setup Safe Inputs Tool Files run: | - cat > /opt/gh-aw/safe-inputs/ready-to-make-pr.py << 'GH_AW_SAFE_INPUTS_PY_READY-TO-MAKE-PR_EOF' + cat > /opt/gh-aw/safe-inputs/ready-to-push-to-pr.py << 'GH_AW_SAFE_INPUTS_PY_READY-TO-PUSH-TO-PR_EOF' #!/usr/bin/env python3 - # Auto-generated safe-input tool: ready-to-make-pr - # Run the PR readiness checklist before creating or updating a PR + # Auto-generated safe-input tool: ready-to-push-to-pr + # Run the PR readiness checklist before pushing to a PR import json import os @@ -1334,11 +1334,34 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect history rewrites and merge commits + pr_json_path = '/tmp/pr-context/pr.json' + if os.path.isfile(pr_json_path): + with open(pr_json_path) as f: + pr_data = json.load(f) + pr_head_sha = pr_data.get('headRefOid', '') + if pr_head_sha: + # Check 1: PR head must be an ancestor of HEAD (no rebase/reset) + anc = run(['git', 'merge-base', '--is-ancestor', pr_head_sha, 'HEAD']) + if anc.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'History rewrite detected: the original PR head ({pr_head_sha[:12]}) is not an ancestor of HEAD. This means git rebase, reset, or cherry-pick rewrote history. push_to_pull_request_branch will fail. Fix: run `git reset --hard {pr_head_sha}` to restore the PR branch to its original head, then re-apply your changes as direct file edits and commit as regular commits.'})) + raise SystemExit(0) + # Check 2: no merge commits (multiple parents) since PR head + log = run(['git', 'rev-list', '--min-parents=2', f'{pr_head_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for push.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... push_to_pull_request_branch uses git format-patch which breaks on merge commits. Fix: run `git reset --hard {pr_head_sha}` to restore the PR branch, then re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], @@ -1370,12 +1393,12 @@ jobs: if pr_template: checklist.append(f'Follow the PR template ({pr_template}) for title, description, and validation notes.') checklist.append('Confirm the requested task is fully completed and validated before creating or pushing PR changes.') if diff_line_count > 0: - checklist.append(f'A diff of your unpushed changes ({diff_line_count} lines) has been saved to `/tmp/self-review/diff.patch` (full diff) and `/tmp/self-review/stat.txt` (summary). Spawn a `code-review` sub-agent via `runSubagent` to review the diff against the codebase. Tell it to read `/tmp/self-review/diff.patch` and the relevant source files, and look for bugs, logic errors, missed edge cases, and style issues. If the sub-agent finds legitimate issues, fix them, commit, and call `ready_to_make_pr` again to regenerate the diff before proceeding.') + checklist.append(f'A diff of your unpushed changes ({diff_line_count} lines) has been saved to `/tmp/self-review/diff.patch` (full diff) and `/tmp/self-review/stat.txt` (summary). Spawn a `code-review` sub-agent via `runSubagent` to review the diff against the codebase. Tell it to read `/tmp/self-review/diff.patch` and the relevant source files, and look for bugs, logic errors, missed edge cases, and style issues. If the sub-agent finds legitimate issues, fix them, commit, and call `ready_to_push_to_pr` again to regenerate the diff before proceeding.') print(json.dumps({'status': 'ok', 'checklist': checklist, 'contributing_guide': contributing, 'pr_template': pr_template, 'diff_line_count': diff_line_count})) - GH_AW_SAFE_INPUTS_PY_READY-TO-MAKE-PR_EOF - chmod +x /opt/gh-aw/safe-inputs/ready-to-make-pr.py + GH_AW_SAFE_INPUTS_PY_READY-TO-PUSH-TO-PR_EOF + chmod +x /opt/gh-aw/safe-inputs/ready-to-push-to-pr.py - name: Generate Safe Inputs MCP Server Config id: safe-inputs-config diff --git a/.github/workflows/gh-aw-mention-in-pr.lock.yml b/.github/workflows/gh-aw-mention-in-pr.lock.yml index 667ca3cf..cb3c7f5a 100644 --- a/.github/workflows/gh-aw-mention-in-pr.lock.yml +++ b/.github/workflows/gh-aw-mention-in-pr.lock.yml @@ -44,7 +44,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"6c993989c5542e01f4c2ab3a0f4de5322c78a0a29f2a93f87ca68fb54610077f"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"3e2208ff26fa33bb18399c3abb9067fe36fae6c3b0bd59650e6973ee1ba67b99"} name: "Mention in PR" "on": @@ -454,7 +454,7 @@ jobs: **Do NOT** describe what the PR does, list the files you reviewed, summarize inline comments, or restate prior review feedback. The PR author already knows what their PR does. Your inline comments already contain all the detail. The review body exists solely to communicate the approve/request-changes decision and important/critical feedback that cannot be covered in inline comments. GH_AW_PROMPT_EOF cat << 'GH_AW_PROMPT_EOF' - Before calling `push_to_pull_request_branch`, call `ready_to_make_pr` and apply its checklist. + Before calling `push_to_pull_request_branch`, call `ready_to_push_to_pr` and apply its checklist. ## push-to-pull-request-branch Limitations @@ -465,10 +465,10 @@ jobs: - You may not submit code that modifies files in `.github/workflows/`. Doing so will cause the submission to be rejected. If asked to modify workflow files, propose the change in a copy placed in a `github/` folder (without the leading period) and note in the PR that the file needs to be relocated by someone with workflow write access. Trying to resolve merge conflicts? Do NOT create merge commits (commits with multiple parents) — `push_to_pull_request_branch` uses `git format-patch` which breaks on merge commits. This means: no `git merge`, no `git rebase`, no `git commit-tree` with multiple `-p` flags. Instead: - 1. Use `git merge-tree` or `git diff` to compare the conflicting files between this PR branch and the PR base branch (read from `/tmp/pr-context/pr.json`) + 1. Use `git diff HEAD...origin/` (base branch from `/tmp/pr-context/pr.json` field `baseRefName`) to see what the base branch changed in the conflicting files 2. Edit the files directly to incorporate the changes from the base branch 3. Commit the changes as regular (single-parent) commits - 4. Use push_to_pull_request_branch to push + 4. Once you are done with all of your changes on this branch, call `ready_to_push_to_pr` and then `push_to_pull_request_branch` to push GH_AW_PROMPT_EOF cat << 'GH_AW_PROMPT_EOF' ## resolve-pull-request-review-thread Limitations @@ -758,7 +758,7 @@ jobs: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ github.event.pull_request.number || inputs.target-pr-number || github.event.issue.number }} name: Fetch PR context to disk - run: "set -euo pipefail\nmkdir -p /tmp/pr-context\n\n# PR metadata\ngh pr view \"$PR_NUMBER\" --json title,body,author,baseRefName,headRefName,url \\\n > /tmp/pr-context/pr.json\n\n# Full diff\nif ! gh pr diff \"$PR_NUMBER\" > /tmp/pr-context/pr.diff; then\n echo \"::warning::Failed to fetch full PR diff; per-file diffs from files.json are still available.\"\n : > /tmp/pr-context/pr.diff\nfi\n\n# Changed files list (--paginate may output concatenated arrays; jq -s 'add' merges them)\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/files\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/files.json\n\n# Per-file diffs\njq -c '.[]' /tmp/pr-context/files.json | while IFS= read -r entry; do\n filename=$(echo \"$entry\" | jq -r '.filename')\n mkdir -p \"/tmp/pr-context/diffs/$(dirname \"$filename\")\"\n echo \"$entry\" | jq -r '.patch // empty' > \"/tmp/pr-context/diffs/${filename}.diff\"\ndone\n\n# File orderings for sub-agent review (3 strategies)\njq -r '[.[] | .filename] | sort | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_az.txt\njq -r '[.[] | .filename] | sort | reverse | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_za.txt\njq -r '[.[] | {filename, size: ((.additions // 0) + (.deletions // 0))}] | sort_by(-.size) | .[].filename' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_largest.txt\n\n# Existing reviews\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/reviews.json\n\n# Review threads with resolution status (GraphQL — REST lacks isResolved/isOutdated)\ngh api graphql --paginate -f query='\n query($owner: String!, $repo: String!, $number: Int!, $endCursor: String) {\n repository(owner: $owner, name: $repo) {\n pullRequest(number: $number) {\n reviewThreads(first: 100, after: $endCursor) {\n pageInfo { hasNextPage endCursor }\n nodes {\n id\n isResolved\n isOutdated\n isCollapsed\n path\n line\n startLine\n comments(first: 100) {\n nodes {\n id\n databaseId\n body\n author { login }\n createdAt\n }\n }\n }\n }\n }\n }\n }\n' -F owner=\"${GITHUB_REPOSITORY%/*}\" -F repo=\"${GITHUB_REPOSITORY#*/}\" -F \"number=$PR_NUMBER\" \\\n --jq '.data.repository.pullRequest.reviewThreads.nodes' \\\n | jq -s 'add // []' > /tmp/pr-context/review_comments.json\n\n# Per-file review threads (mirrors diffs/ structure)\njq -c '.[]' /tmp/pr-context/review_comments.json | while IFS= read -r thread; do\n filepath=$(echo \"$thread\" | jq -r '.path // empty')\n [ -z \"$filepath\" ] && continue\n mkdir -p \"/tmp/pr-context/threads/$(dirname \"$filepath\")\"\n echo \"$thread\" >> \"/tmp/pr-context/threads/${filepath}.jsonl\"\ndone\n# Convert per-file JSONL to proper JSON arrays\nmkdir -p /tmp/pr-context/threads\nfind /tmp/pr-context/threads -name '*.jsonl' 2>/dev/null | while IFS= read -r jsonl; do\n jq -s '.' \"$jsonl\" > \"${jsonl%.jsonl}.json\"\n rm \"$jsonl\"\ndone\n\n# PR discussion comments\ngh api \"repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/comments\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/comments.json\n\n# Linked issues\njq -r '.body // \"\"' /tmp/pr-context/pr.json 2>/dev/null \\\n | grep -oiE '(fixes|closes|resolves)\\s+#[0-9]+' \\\n | grep -oE '[0-9]+$' \\\n | sort -u \\\n | while read -r issue; do\n gh api \"repos/$GITHUB_REPOSITORY/issues/$issue\" > \"/tmp/pr-context/issue-${issue}.json\" || true\n done || true\n\n# Write manifest\ncat > /tmp/pr-context/README.md << 'MANIFEST'\n# PR Context\n\nPre-fetched PR data. All files are in `/tmp/pr-context/`.\n\n| File | Description |\n| --- | --- |\n| `pr.json` | PR metadata — title, body, author, base/head branches, URL |\n| `pr.diff` | Full unified diff of all changes |\n| `files.json` | Changed files array — each entry has `filename`, `status`, `additions`, `deletions`, `patch` |\n| `diffs/.diff` | Per-file diffs — one file per changed file, mirroring the repo path under `diffs/` |\n| `file_order_az.txt` | Changed files sorted alphabetically (A→Z), one filename per line |\n| `file_order_za.txt` | Changed files sorted reverse-alphabetically (Z→A), one filename per line |\n| `file_order_largest.txt` | Changed files sorted by diff size descending (largest first), one filename per line |\n| `reviews.json` | Prior review submissions — author, state (APPROVED/CHANGES_REQUESTED/COMMENTED), body |\n| `review_comments.json` | All review threads (GraphQL) — each thread has `id` (node ID for resolving), `isResolved`, `isOutdated`, `path`, `line`, and nested `comments` with `id`, `databaseId` (numeric REST ID for replies), body/author |\n| `threads/.json` | Per-file review threads — one file per changed file with existing threads, mirroring the repo path under `threads/` |\n| `comments.json` | PR discussion comments (not inline) |\n| `issue-{N}.json` | Linked issue details (one file per linked issue, if any) |\n| `agents.md` | Repository conventions from `generate_agents_md` (if written by agent) |\n| `review-instructions.md` | Review instructions, criteria, and calibration examples (if written by review-process fragment) |\nMANIFEST\n\necho \"PR context written to /tmp/pr-context/\"\nls -la /tmp/pr-context/" + run: "set -euo pipefail\nmkdir -p /tmp/pr-context\n\n# PR metadata\ngh pr view \"$PR_NUMBER\" --json title,body,author,baseRefName,headRefName,headRefOid,url \\\n > /tmp/pr-context/pr.json\n\n# Full diff\nif ! gh pr diff \"$PR_NUMBER\" > /tmp/pr-context/pr.diff; then\n echo \"::warning::Failed to fetch full PR diff; per-file diffs from files.json are still available.\"\n : > /tmp/pr-context/pr.diff\nfi\n\n# Changed files list (--paginate may output concatenated arrays; jq -s 'add' merges them)\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/files\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/files.json\n\n# Per-file diffs\njq -c '.[]' /tmp/pr-context/files.json | while IFS= read -r entry; do\n filename=$(echo \"$entry\" | jq -r '.filename')\n mkdir -p \"/tmp/pr-context/diffs/$(dirname \"$filename\")\"\n echo \"$entry\" | jq -r '.patch // empty' > \"/tmp/pr-context/diffs/${filename}.diff\"\ndone\n\n# File orderings for sub-agent review (3 strategies)\njq -r '[.[] | .filename] | sort | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_az.txt\njq -r '[.[] | .filename] | sort | reverse | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_za.txt\njq -r '[.[] | {filename, size: ((.additions // 0) + (.deletions // 0))}] | sort_by(-.size) | .[].filename' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_largest.txt\n\n# Existing reviews\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/reviews.json\n\n# Review threads with resolution status (GraphQL — REST lacks isResolved/isOutdated)\ngh api graphql --paginate -f query='\n query($owner: String!, $repo: String!, $number: Int!, $endCursor: String) {\n repository(owner: $owner, name: $repo) {\n pullRequest(number: $number) {\n reviewThreads(first: 100, after: $endCursor) {\n pageInfo { hasNextPage endCursor }\n nodes {\n id\n isResolved\n isOutdated\n isCollapsed\n path\n line\n startLine\n comments(first: 100) {\n nodes {\n id\n databaseId\n body\n author { login }\n createdAt\n }\n }\n }\n }\n }\n }\n }\n' -F owner=\"${GITHUB_REPOSITORY%/*}\" -F repo=\"${GITHUB_REPOSITORY#*/}\" -F \"number=$PR_NUMBER\" \\\n --jq '.data.repository.pullRequest.reviewThreads.nodes' \\\n | jq -s 'add // []' > /tmp/pr-context/review_comments.json\n\n# Per-file review threads (mirrors diffs/ structure)\njq -c '.[]' /tmp/pr-context/review_comments.json | while IFS= read -r thread; do\n filepath=$(echo \"$thread\" | jq -r '.path // empty')\n [ -z \"$filepath\" ] && continue\n mkdir -p \"/tmp/pr-context/threads/$(dirname \"$filepath\")\"\n echo \"$thread\" >> \"/tmp/pr-context/threads/${filepath}.jsonl\"\ndone\n# Convert per-file JSONL to proper JSON arrays\nmkdir -p /tmp/pr-context/threads\nfind /tmp/pr-context/threads -name '*.jsonl' 2>/dev/null | while IFS= read -r jsonl; do\n jq -s '.' \"$jsonl\" > \"${jsonl%.jsonl}.json\"\n rm \"$jsonl\"\ndone\n\n# PR discussion comments\ngh api \"repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/comments\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/comments.json\n\n# Linked issues\njq -r '.body // \"\"' /tmp/pr-context/pr.json 2>/dev/null \\\n | grep -oiE '(fixes|closes|resolves)\\s+#[0-9]+' \\\n | grep -oE '[0-9]+$' \\\n | sort -u \\\n | while read -r issue; do\n gh api \"repos/$GITHUB_REPOSITORY/issues/$issue\" > \"/tmp/pr-context/issue-${issue}.json\" || true\n done || true\n\n# Write manifest\ncat > /tmp/pr-context/README.md << 'MANIFEST'\n# PR Context\n\nPre-fetched PR data. All files are in `/tmp/pr-context/`.\n\n| File | Description |\n| --- | --- |\n| `pr.json` | PR metadata — title, body, author, base/head branches, head commit SHA (`headRefOid`), URL |\n| `pr.diff` | Full unified diff of all changes |\n| `files.json` | Changed files array — each entry has `filename`, `status`, `additions`, `deletions`, `patch` |\n| `diffs/.diff` | Per-file diffs — one file per changed file, mirroring the repo path under `diffs/` |\n| `file_order_az.txt` | Changed files sorted alphabetically (A→Z), one filename per line |\n| `file_order_za.txt` | Changed files sorted reverse-alphabetically (Z→A), one filename per line |\n| `file_order_largest.txt` | Changed files sorted by diff size descending (largest first), one filename per line |\n| `reviews.json` | Prior review submissions — author, state (APPROVED/CHANGES_REQUESTED/COMMENTED), body |\n| `review_comments.json` | All review threads (GraphQL) — each thread has `id` (node ID for resolving), `isResolved`, `isOutdated`, `path`, `line`, and nested `comments` with `id`, `databaseId` (numeric REST ID for replies), body/author |\n| `threads/.json` | Per-file review threads — one file per changed file with existing threads, mirroring the repo path under `threads/` |\n| `comments.json` | PR discussion comments (not inline) |\n| `issue-{N}.json` | Linked issue details (one file per linked issue, if any) |\n| `agents.md` | Repository conventions from `generate_agents_md` (if written by agent) |\n| `review-instructions.md` | Review instructions, criteria, and calibration examples (if written by review-process fragment) |\nMANIFEST\n\necho \"PR context written to /tmp/pr-context/\"\nls -la /tmp/pr-context/" - name: Write review instructions to disk run: "mkdir -p /tmp/pr-context\ncat > /tmp/pr-context/review-instructions.md << 'REVIEW_EOF'\n# Review Instructions for Sub-agents\n\nYou are a code review sub-agent. Read these instructions, then review the PR files in the order provided in your prompt.\n\n## Context\n\nBefore reviewing files, read these to understand the PR:\n\n1. `/tmp/pr-context/pr.json` — PR title, description, author, and branches. Understand what the PR is trying to accomplish.\n2. `/tmp/pr-context/agents.md` — Repository coding conventions and guidelines (if it exists).\n3. `/tmp/pr-context/review_comments.json` — Existing review threads. Note which files already have threads so you don't duplicate.\n4. `/tmp/pr-context/issue-*.json` — Linked issue details (if any). Understand the motivation and acceptance criteria.\n\n## Process\n\nReview the PR diff file by file in your assigned order. For each changed file:\n\n1. **Read the diff** for this file from `/tmp/pr-context/diffs/.diff` to understand what changed. If the diff is empty or truncated (e.g., binary files or very large changes), fall back to reading the full file from the workspace and comparing against context.\n2. **Read the full file from the workspace.** The PR branch is checked out locally — open the file directly to get complete contents with line numbers.\n3. **Check existing threads** for this file from `/tmp/pr-context/threads/.json` (if it exists). Skip issues that are already under discussion — each thread has `isResolved` and `isOutdated` fields.\n4. **Identify potential issues** matching the review criteria below.\n5. **Quick-check each issue** before including it:\n - What specific code pattern or change triggers this concern?\n - Is there an obvious guard, handler, or mitigation visible in the immediate context?\n - Can you describe a concrete failure scenario (the `evidence` field)? If you cannot articulate what specific input or state triggers the problem, drop the finding.\n - If the issue is clearly handled, skip it. If you're unsure, include it — the parent will verify.\n6. **Add to your findings list.** Do NOT leave inline comments — you don't have that tool. Return findings in this format:\n\n```\n- file: path/to/file\n line: 42\n severity: HIGH\n title: Brief title\n description: What the issue is and why it matters\n evidence: The specific code pattern and failure scenario\n suggestion: corrected code here (optional — only if you can provide a concrete fix)\n```\n\n**Review every file in your assigned order.** Files reviewed earlier get more attention, which is why different sub-agents use different orderings.\n\n**Check existing threads** — per-file threads are at `/tmp/pr-context/threads/.json` (step 3 above). The full list is at `/tmp/pr-context/review_comments.json`. Do not flag issues that are already under discussion (resolved or unresolved). For outdated threads, only re-flag if the issue still applies to the current diff.\n\n**Return your full findings list** when done, or an empty list if no issues were found.\n\n## Review Criteria\n\nFocus on these categories in priority order:\n\n1. Security vulnerabilities (injection, XSS, auth bypass, secrets exposure)\n2. Logic bugs that could cause runtime failures or incorrect behavior\n3. Data integrity issues (race conditions, missing transactions, corruption risk)\n4. Performance bottlenecks (N+1 queries, memory leaks, blocking operations)\n5. Error handling gaps (unhandled exceptions, missing validation)\n6. Breaking changes to public APIs without migration path\n7. Missing or incorrect test coverage for critical paths\n\n## What NOT to Flag\n\nOnly review the diff — do not flag issues in unchanged code, pre-existing problems not introduced by this PR, or style preferences handled by linters or formatters.\n\n**Common false positives** — these patterns look like issues but usually aren't. Before flagging anything in these categories, confirm the problem is real by reading the surrounding code:\n\n- **Security — input already sanitized:** Don't flag injection or XSS risks when inputs are sanitized upstream, parameterized queries are used, or the framework auto-escapes output.\n- **Null/undefined — guarded elsewhere:** Don't flag potential null dereferences if the value is guaranteed by a type guard, assertion, schema validation, or upstream null check.\n- **Error handling — handled at a different layer:** Don't flag missing try/catch if the caller, middleware, or framework catches and handles the error (e.g., Express error middleware, React error boundaries).\n- **Performance — theoretical, not practical:** Don't flag algorithmic complexity (e.g., O(n^2)) unless N is demonstrably large enough to matter in the actual usage context. \"This could be slow\" without evidence is not actionable.\n- **Validation — exists at another layer:** Don't flag missing input validation if it's handled by an API gateway, middleware, schema validator, or type system.\n- **Test coverage — trivial or generated code:** Don't flag missing tests for trivial getters/setters, auto-generated code, or simple delegation methods.\n- **Style or naming — not in coding guidelines:** Don't flag naming conventions or code style unless they violate the repository's documented coding guidelines (from `generate_agents_md` or CONTRIBUTING docs).\n\n**Existing review threads** — check BEFORE flagging any issue:\n\n- **Resolved with reviewer reply** (e.g. \"This is intentional\") — reviewer's decision is final. Do NOT re-flag.\n- **Resolved without reply** — author likely fixed it. Do NOT re-raise unless the fix introduced a new problem.\n- **Unresolved** — already flagged. Do NOT duplicate.\n- **Outdated** — only re-flag if the issue still applies to the current diff.\n\nWhen in doubt, do not duplicate. Redundant comments erode trust.\n\nFinding no issues is a valid and valuable outcome. An empty findings list is better than findings that waste the author's time or erode trust. Do not manufacture findings to justify your review — if the code is sound, return an empty list.\n\n## Severity Classification\n\nDetermine severity AFTER investigating the issue, not before. First identify the problem and trace through the code, then assign a severity based on the evidence you found.\n\n- 🔴 CRITICAL — Must fix before merge (security vulnerabilities, data corruption, production-breaking bugs)\n- 🟠 HIGH — Should fix before merge (logic errors, missing validation, significant performance issues)\n- 🟡 MEDIUM — Address soon, non-blocking (error handling gaps, suboptimal patterns, missing edge cases)\n- ⚪ LOW — Author discretion (minor improvements, documentation gaps)\n- 💬 NITPICK — Truly optional (stylistic preferences, alternative approaches)\n\n## Review Intensity\n\nThe review intensity is `${{ inputs.intensity || 'balanced' }}`.\n\n- **conservative**: High evidence bar. Only flag when you can demonstrate a concrete failure scenario. If you can construct a reasonable counterargument, do not flag. Approval with zero findings is the expected outcome for most PRs.\n- **balanced**: Standard evidence bar. Flag when you can point to specific code that would fail. If the issue is ambiguous, lean toward not flagging.\n- **aggressive**: Lower evidence bar. Flag when evidence exists even if the failure scenario is not fully confirmed. Improvement suggestions welcome but must cite specific code.\n\n## Calibration Examples\n\nUse these examples to calibrate your judgment. Each pair shows a real issue and a similar-looking pattern that is NOT an issue.\n\n### Example 1: Null/Undefined Access\n\n**True positive — flag this:**\n\n```js\n// PR adds this handler\napp.get('/user/:id', async (req, res) => {\n const user = await db.findUser(req.params.id);\n res.json({ name: user.name, email: user.email });\n});\n```\n\nWhy flag: `db.findUser()` can return `null` when no user matches the ID. Accessing `user.name` will throw a TypeError at runtime. No upstream guard exists — the route handler is the entry point.\n\n**False positive — do NOT flag this:**\n\n```ts\n// PR adds this line inside an existing function\nconst settings = user.getSettings();\n```\n\nWhy skip: Reading the full file reveals `user` is typed as `User` (not `User | null`), and the calling function only runs after `authenticateUser()` middleware which guarantees a valid user object. The null case is handled at a different layer.\n\n### Example 2: SQL Injection\n\n**True positive — flag this:**\n\n```python\n# PR adds this query\ncursor.execute(f\"SELECT * FROM orders WHERE customer_id = '{customer_id}'\")\n```\n\nWhy flag: String interpolation in a SQL query with user-controlled input (`customer_id` comes from the request). No parameterization or sanitization anywhere in the call chain.\n\n**False positive — do NOT flag this:**\n\n```python\n# PR adds this query\ncursor.execute(f\"SELECT * FROM orders WHERE status = '{OrderStatus.PENDING.value}'\")\n```\n\nWhy skip: The interpolated value is a hardcoded enum constant (`OrderStatus.PENDING`), not user input. There is no injection vector.\n\n### Example 3: Borderline — Do NOT Flag\n\n```go\n// PR adds this function\nfunc processItems(items []Item) []Result {\n results := make([]Result, 0)\n for _, item := range items {\n for _, tag := range item.Tags {\n results = append(results, process(item, tag))\n }\n }\n return results\n}\n```\n\nThis looks like an O(n*m) performance concern. But without evidence that `items` or `Tags` are large in practice, this is speculative. The function processes a bounded dataset (items from a single user request). Do not flag theoretical performance issues without evidence of real-world impact.\nREVIEW_EOF" - env: @@ -1313,13 +1313,13 @@ jobs: "logDir": "/opt/gh-aw/safe-inputs/logs", "tools": [ { - "name": "ready-to-make-pr", - "description": "Run the PR readiness checklist before creating or updating a PR", + "name": "ready-to-push-to-pr", + "description": "Run the PR readiness checklist before pushing to a PR", "inputSchema": { "properties": {}, "type": "object" }, - "handler": "ready-to-make-pr.py", + "handler": "ready-to-push-to-pr.py", "timeout": 60 } ] @@ -1344,10 +1344,10 @@ jobs: - name: Setup Safe Inputs Tool Files run: | - cat > /opt/gh-aw/safe-inputs/ready-to-make-pr.py << 'GH_AW_SAFE_INPUTS_PY_READY-TO-MAKE-PR_EOF' + cat > /opt/gh-aw/safe-inputs/ready-to-push-to-pr.py << 'GH_AW_SAFE_INPUTS_PY_READY-TO-PUSH-TO-PR_EOF' #!/usr/bin/env python3 - # Auto-generated safe-input tool: ready-to-make-pr - # Run the PR readiness checklist before creating or updating a PR + # Auto-generated safe-input tool: ready-to-push-to-pr + # Run the PR readiness checklist before pushing to a PR import json import os @@ -1368,11 +1368,34 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect history rewrites and merge commits + pr_json_path = '/tmp/pr-context/pr.json' + if os.path.isfile(pr_json_path): + with open(pr_json_path) as f: + pr_data = json.load(f) + pr_head_sha = pr_data.get('headRefOid', '') + if pr_head_sha: + # Check 1: PR head must be an ancestor of HEAD (no rebase/reset) + anc = run(['git', 'merge-base', '--is-ancestor', pr_head_sha, 'HEAD']) + if anc.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'History rewrite detected: the original PR head ({pr_head_sha[:12]}) is not an ancestor of HEAD. This means git rebase, reset, or cherry-pick rewrote history. push_to_pull_request_branch will fail. Fix: run `git reset --hard {pr_head_sha}` to restore the PR branch to its original head, then re-apply your changes as direct file edits and commit as regular commits.'})) + raise SystemExit(0) + # Check 2: no merge commits (multiple parents) since PR head + log = run(['git', 'rev-list', '--min-parents=2', f'{pr_head_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for push.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... push_to_pull_request_branch uses git format-patch which breaks on merge commits. Fix: run `git reset --hard {pr_head_sha}` to restore the PR branch, then re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], @@ -1404,12 +1427,12 @@ jobs: if pr_template: checklist.append(f'Follow the PR template ({pr_template}) for title, description, and validation notes.') checklist.append('Confirm the requested task is fully completed and validated before creating or pushing PR changes.') if diff_line_count > 0: - checklist.append(f'A diff of your unpushed changes ({diff_line_count} lines) has been saved to `/tmp/self-review/diff.patch` (full diff) and `/tmp/self-review/stat.txt` (summary). Spawn a `code-review` sub-agent via `runSubagent` to review the diff against the codebase. Tell it to read `/tmp/self-review/diff.patch` and the relevant source files, and look for bugs, logic errors, missed edge cases, and style issues. If the sub-agent finds legitimate issues, fix them, commit, and call `ready_to_make_pr` again to regenerate the diff before proceeding.') + checklist.append(f'A diff of your unpushed changes ({diff_line_count} lines) has been saved to `/tmp/self-review/diff.patch` (full diff) and `/tmp/self-review/stat.txt` (summary). Spawn a `code-review` sub-agent via `runSubagent` to review the diff against the codebase. Tell it to read `/tmp/self-review/diff.patch` and the relevant source files, and look for bugs, logic errors, missed edge cases, and style issues. If the sub-agent finds legitimate issues, fix them, commit, and call `ready_to_push_to_pr` again to regenerate the diff before proceeding.') print(json.dumps({'status': 'ok', 'checklist': checklist, 'contributing_guide': contributing, 'pr_template': pr_template, 'diff_line_count': diff_line_count})) - GH_AW_SAFE_INPUTS_PY_READY-TO-MAKE-PR_EOF - chmod +x /opt/gh-aw/safe-inputs/ready-to-make-pr.py + GH_AW_SAFE_INPUTS_PY_READY-TO-PUSH-TO-PR_EOF + chmod +x /opt/gh-aw/safe-inputs/ready-to-push-to-pr.py - name: Generate Safe Inputs MCP Server Config id: safe-inputs-config diff --git a/.github/workflows/gh-aw-newbie-contributor-fixer.lock.yml b/.github/workflows/gh-aw-newbie-contributor-fixer.lock.yml index 4e43221c..798abe18 100644 --- a/.github/workflows/gh-aw-newbie-contributor-fixer.lock.yml +++ b/.github/workflows/gh-aw-newbie-contributor-fixer.lock.yml @@ -37,7 +37,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"024dac322c2a829d3934945b47e8d4fee6d109bc488ace55b42d8fbbc62c3c58"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"c04e88382cf7c1d21fff1cd9355de38fe083e4eda41206a0d4d5b5997ccee6e9"} name: "Newbie Contributor Fixer" "on": @@ -935,11 +935,32 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect merge commits + # Find the fork point with the upstream branch to scope the check + upstream_sha = '' + for ref in ['@{upstream}', 'origin/HEAD', 'origin/main']: + r = run(['git', 'merge-base', 'HEAD', ref]) + if r.returncode == 0 and r.stdout.strip(): + upstream_sha = r.stdout.strip() + break + if not upstream_sha: + print(json.dumps({'status': 'error', 'error': 'Unable to determine upstream fork point for merge-commit validation. Fix: ensure remotes are fetched and a tracking branch is set (e.g., `git branch --set-upstream-to origin/`), then rerun ready_to_make_pr.'})) + raise SystemExit(0) + log = run(['git', 'rev-list', '--min-parents=2', f'{upstream_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for PR creation.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... create_pull_request uses git format-patch which breaks on merge commits. Fix: re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], diff --git a/.github/workflows/gh-aw-pr-actions-fixer.lock.yml b/.github/workflows/gh-aw-pr-actions-fixer.lock.yml index 08986bb1..c38f4bab 100644 --- a/.github/workflows/gh-aw-pr-actions-fixer.lock.yml +++ b/.github/workflows/gh-aw-pr-actions-fixer.lock.yml @@ -37,7 +37,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"e490edb6d9a4e54d16cbbef444ea346179e20b8ad15ccb211e1edb6efc039de2"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"067ef5b1714251b55aa9fc1316a2fda34c0f3af4158afe58b21c4637ae7f99c0"} name: "PR Actions Fixer" "on": @@ -287,7 +287,7 @@ jobs: If you exceed 10 mentions or 50 links, the comment will be rejected. GH_AW_PROMPT_EOF cat << 'GH_AW_PROMPT_EOF' - Before calling `push_to_pull_request_branch`, call `ready_to_make_pr` and apply its checklist. + Before calling `push_to_pull_request_branch`, call `ready_to_push_to_pr` and apply its checklist. ## push-to-pull-request-branch Limitations @@ -298,10 +298,10 @@ jobs: - You may not submit code that modifies files in `.github/workflows/`. Doing so will cause the submission to be rejected. If asked to modify workflow files, propose the change in a copy placed in a `github/` folder (without the leading period) and note in the PR that the file needs to be relocated by someone with workflow write access. Trying to resolve merge conflicts? Do NOT create merge commits (commits with multiple parents) — `push_to_pull_request_branch` uses `git format-patch` which breaks on merge commits. This means: no `git merge`, no `git rebase`, no `git commit-tree` with multiple `-p` flags. Instead: - 1. Use `git merge-tree` or `git diff` to compare the conflicting files between this PR branch and the PR base branch (read from `/tmp/pr-context/pr.json`) + 1. Use `git diff HEAD...origin/` (base branch from `/tmp/pr-context/pr.json` field `baseRefName`) to see what the base branch changed in the conflicting files 2. Edit the files directly to incorporate the changes from the base branch 3. Commit the changes as regular (single-parent) commits - 4. Use push_to_pull_request_branch to push + 4. Once you are done with all of your changes on this branch, call `ready_to_push_to_pr` and then `push_to_pull_request_branch` to push GH_AW_PROMPT_EOF cat << 'GH_AW_PROMPT_EOF' @@ -893,13 +893,13 @@ jobs: "logDir": "/opt/gh-aw/safe-inputs/logs", "tools": [ { - "name": "ready-to-make-pr", - "description": "Run the PR readiness checklist before creating or updating a PR", + "name": "ready-to-push-to-pr", + "description": "Run the PR readiness checklist before pushing to a PR", "inputSchema": { "properties": {}, "type": "object" }, - "handler": "ready-to-make-pr.py", + "handler": "ready-to-push-to-pr.py", "timeout": 60 } ] @@ -924,10 +924,10 @@ jobs: - name: Setup Safe Inputs Tool Files run: | - cat > /opt/gh-aw/safe-inputs/ready-to-make-pr.py << 'GH_AW_SAFE_INPUTS_PY_READY-TO-MAKE-PR_EOF' + cat > /opt/gh-aw/safe-inputs/ready-to-push-to-pr.py << 'GH_AW_SAFE_INPUTS_PY_READY-TO-PUSH-TO-PR_EOF' #!/usr/bin/env python3 - # Auto-generated safe-input tool: ready-to-make-pr - # Run the PR readiness checklist before creating or updating a PR + # Auto-generated safe-input tool: ready-to-push-to-pr + # Run the PR readiness checklist before pushing to a PR import json import os @@ -948,11 +948,34 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect history rewrites and merge commits + pr_json_path = '/tmp/pr-context/pr.json' + if os.path.isfile(pr_json_path): + with open(pr_json_path) as f: + pr_data = json.load(f) + pr_head_sha = pr_data.get('headRefOid', '') + if pr_head_sha: + # Check 1: PR head must be an ancestor of HEAD (no rebase/reset) + anc = run(['git', 'merge-base', '--is-ancestor', pr_head_sha, 'HEAD']) + if anc.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'History rewrite detected: the original PR head ({pr_head_sha[:12]}) is not an ancestor of HEAD. This means git rebase, reset, or cherry-pick rewrote history. push_to_pull_request_branch will fail. Fix: run `git reset --hard {pr_head_sha}` to restore the PR branch to its original head, then re-apply your changes as direct file edits and commit as regular commits.'})) + raise SystemExit(0) + # Check 2: no merge commits (multiple parents) since PR head + log = run(['git', 'rev-list', '--min-parents=2', f'{pr_head_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for push.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... push_to_pull_request_branch uses git format-patch which breaks on merge commits. Fix: run `git reset --hard {pr_head_sha}` to restore the PR branch, then re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], @@ -984,12 +1007,12 @@ jobs: if pr_template: checklist.append(f'Follow the PR template ({pr_template}) for title, description, and validation notes.') checklist.append('Confirm the requested task is fully completed and validated before creating or pushing PR changes.') if diff_line_count > 0: - checklist.append(f'A diff of your unpushed changes ({diff_line_count} lines) has been saved to `/tmp/self-review/diff.patch` (full diff) and `/tmp/self-review/stat.txt` (summary). Spawn a `code-review` sub-agent via `runSubagent` to review the diff against the codebase. Tell it to read `/tmp/self-review/diff.patch` and the relevant source files, and look for bugs, logic errors, missed edge cases, and style issues. If the sub-agent finds legitimate issues, fix them, commit, and call `ready_to_make_pr` again to regenerate the diff before proceeding.') + checklist.append(f'A diff of your unpushed changes ({diff_line_count} lines) has been saved to `/tmp/self-review/diff.patch` (full diff) and `/tmp/self-review/stat.txt` (summary). Spawn a `code-review` sub-agent via `runSubagent` to review the diff against the codebase. Tell it to read `/tmp/self-review/diff.patch` and the relevant source files, and look for bugs, logic errors, missed edge cases, and style issues. If the sub-agent finds legitimate issues, fix them, commit, and call `ready_to_push_to_pr` again to regenerate the diff before proceeding.') print(json.dumps({'status': 'ok', 'checklist': checklist, 'contributing_guide': contributing, 'pr_template': pr_template, 'diff_line_count': diff_line_count})) - GH_AW_SAFE_INPUTS_PY_READY-TO-MAKE-PR_EOF - chmod +x /opt/gh-aw/safe-inputs/ready-to-make-pr.py + GH_AW_SAFE_INPUTS_PY_READY-TO-PUSH-TO-PR_EOF + chmod +x /opt/gh-aw/safe-inputs/ready-to-push-to-pr.py - name: Generate Safe Inputs MCP Server Config id: safe-inputs-config diff --git a/.github/workflows/gh-aw-pr-review-addresser.lock.yml b/.github/workflows/gh-aw-pr-review-addresser.lock.yml index 8725bee3..b7454986 100644 --- a/.github/workflows/gh-aw-pr-review-addresser.lock.yml +++ b/.github/workflows/gh-aw-pr-review-addresser.lock.yml @@ -40,7 +40,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"4f1ee1257f4864d55ec2da7ea89e4f721fd045e23e88ff58a32ed227cf00ba16"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"a75125e9e8e3ea172d6e4e27fb0b9fd66bd2403dfcb81a93da21c00f7d9aeeae"} name: "PR Review Addresser" "on": @@ -298,7 +298,7 @@ jobs: If you exceed 10 mentions or 50 links, the comment will be rejected. GH_AW_PROMPT_EOF cat << 'GH_AW_PROMPT_EOF' - Before calling `push_to_pull_request_branch`, call `ready_to_make_pr` and apply its checklist. + Before calling `push_to_pull_request_branch`, call `ready_to_push_to_pr` and apply its checklist. ## push-to-pull-request-branch Limitations @@ -309,10 +309,10 @@ jobs: - You may not submit code that modifies files in `.github/workflows/`. Doing so will cause the submission to be rejected. If asked to modify workflow files, propose the change in a copy placed in a `github/` folder (without the leading period) and note in the PR that the file needs to be relocated by someone with workflow write access. Trying to resolve merge conflicts? Do NOT create merge commits (commits with multiple parents) — `push_to_pull_request_branch` uses `git format-patch` which breaks on merge commits. This means: no `git merge`, no `git rebase`, no `git commit-tree` with multiple `-p` flags. Instead: - 1. Use `git merge-tree` or `git diff` to compare the conflicting files between this PR branch and the PR base branch (read from `/tmp/pr-context/pr.json`) + 1. Use `git diff HEAD...origin/` (base branch from `/tmp/pr-context/pr.json` field `baseRefName`) to see what the base branch changed in the conflicting files 2. Edit the files directly to incorporate the changes from the base branch 3. Commit the changes as regular (single-parent) commits - 4. Use push_to_pull_request_branch to push + 4. Once you are done with all of your changes on this branch, call `ready_to_push_to_pr` and then `push_to_pull_request_branch` to push GH_AW_PROMPT_EOF cat << 'GH_AW_PROMPT_EOF' ## resolve-pull-request-review-thread Limitations @@ -569,7 +569,7 @@ jobs: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ github.event.pull_request.number || inputs.target-pr-number || github.event.issue.number }} name: Fetch PR context to disk - run: "set -euo pipefail\nmkdir -p /tmp/pr-context\n\n# PR metadata\ngh pr view \"$PR_NUMBER\" --json title,body,author,baseRefName,headRefName,url \\\n > /tmp/pr-context/pr.json\n\n# Full diff\nif ! gh pr diff \"$PR_NUMBER\" > /tmp/pr-context/pr.diff; then\n echo \"::warning::Failed to fetch full PR diff; per-file diffs from files.json are still available.\"\n : > /tmp/pr-context/pr.diff\nfi\n\n# Changed files list (--paginate may output concatenated arrays; jq -s 'add' merges them)\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/files\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/files.json\n\n# Per-file diffs\njq -c '.[]' /tmp/pr-context/files.json | while IFS= read -r entry; do\n filename=$(echo \"$entry\" | jq -r '.filename')\n mkdir -p \"/tmp/pr-context/diffs/$(dirname \"$filename\")\"\n echo \"$entry\" | jq -r '.patch // empty' > \"/tmp/pr-context/diffs/${filename}.diff\"\ndone\n\n# File orderings for sub-agent review (3 strategies)\njq -r '[.[] | .filename] | sort | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_az.txt\njq -r '[.[] | .filename] | sort | reverse | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_za.txt\njq -r '[.[] | {filename, size: ((.additions // 0) + (.deletions // 0))}] | sort_by(-.size) | .[].filename' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_largest.txt\n\n# Existing reviews\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/reviews.json\n\n# Review threads with resolution status (GraphQL — REST lacks isResolved/isOutdated)\ngh api graphql --paginate -f query='\n query($owner: String!, $repo: String!, $number: Int!, $endCursor: String) {\n repository(owner: $owner, name: $repo) {\n pullRequest(number: $number) {\n reviewThreads(first: 100, after: $endCursor) {\n pageInfo { hasNextPage endCursor }\n nodes {\n id\n isResolved\n isOutdated\n isCollapsed\n path\n line\n startLine\n comments(first: 100) {\n nodes {\n id\n databaseId\n body\n author { login }\n createdAt\n }\n }\n }\n }\n }\n }\n }\n' -F owner=\"${GITHUB_REPOSITORY%/*}\" -F repo=\"${GITHUB_REPOSITORY#*/}\" -F \"number=$PR_NUMBER\" \\\n --jq '.data.repository.pullRequest.reviewThreads.nodes' \\\n | jq -s 'add // []' > /tmp/pr-context/review_comments.json\n\n# Per-file review threads (mirrors diffs/ structure)\njq -c '.[]' /tmp/pr-context/review_comments.json | while IFS= read -r thread; do\n filepath=$(echo \"$thread\" | jq -r '.path // empty')\n [ -z \"$filepath\" ] && continue\n mkdir -p \"/tmp/pr-context/threads/$(dirname \"$filepath\")\"\n echo \"$thread\" >> \"/tmp/pr-context/threads/${filepath}.jsonl\"\ndone\n# Convert per-file JSONL to proper JSON arrays\nmkdir -p /tmp/pr-context/threads\nfind /tmp/pr-context/threads -name '*.jsonl' 2>/dev/null | while IFS= read -r jsonl; do\n jq -s '.' \"$jsonl\" > \"${jsonl%.jsonl}.json\"\n rm \"$jsonl\"\ndone\n\n# PR discussion comments\ngh api \"repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/comments\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/comments.json\n\n# Linked issues\njq -r '.body // \"\"' /tmp/pr-context/pr.json 2>/dev/null \\\n | grep -oiE '(fixes|closes|resolves)\\s+#[0-9]+' \\\n | grep -oE '[0-9]+$' \\\n | sort -u \\\n | while read -r issue; do\n gh api \"repos/$GITHUB_REPOSITORY/issues/$issue\" > \"/tmp/pr-context/issue-${issue}.json\" || true\n done || true\n\n# Write manifest\ncat > /tmp/pr-context/README.md << 'MANIFEST'\n# PR Context\n\nPre-fetched PR data. All files are in `/tmp/pr-context/`.\n\n| File | Description |\n| --- | --- |\n| `pr.json` | PR metadata — title, body, author, base/head branches, URL |\n| `pr.diff` | Full unified diff of all changes |\n| `files.json` | Changed files array — each entry has `filename`, `status`, `additions`, `deletions`, `patch` |\n| `diffs/.diff` | Per-file diffs — one file per changed file, mirroring the repo path under `diffs/` |\n| `file_order_az.txt` | Changed files sorted alphabetically (A→Z), one filename per line |\n| `file_order_za.txt` | Changed files sorted reverse-alphabetically (Z→A), one filename per line |\n| `file_order_largest.txt` | Changed files sorted by diff size descending (largest first), one filename per line |\n| `reviews.json` | Prior review submissions — author, state (APPROVED/CHANGES_REQUESTED/COMMENTED), body |\n| `review_comments.json` | All review threads (GraphQL) — each thread has `id` (node ID for resolving), `isResolved`, `isOutdated`, `path`, `line`, and nested `comments` with `id`, `databaseId` (numeric REST ID for replies), body/author |\n| `threads/.json` | Per-file review threads — one file per changed file with existing threads, mirroring the repo path under `threads/` |\n| `comments.json` | PR discussion comments (not inline) |\n| `issue-{N}.json` | Linked issue details (one file per linked issue, if any) |\n| `agents.md` | Repository conventions from `generate_agents_md` (if written by agent) |\n| `review-instructions.md` | Review instructions, criteria, and calibration examples (if written by review-process fragment) |\nMANIFEST\n\necho \"PR context written to /tmp/pr-context/\"\nls -la /tmp/pr-context/" + run: "set -euo pipefail\nmkdir -p /tmp/pr-context\n\n# PR metadata\ngh pr view \"$PR_NUMBER\" --json title,body,author,baseRefName,headRefName,headRefOid,url \\\n > /tmp/pr-context/pr.json\n\n# Full diff\nif ! gh pr diff \"$PR_NUMBER\" > /tmp/pr-context/pr.diff; then\n echo \"::warning::Failed to fetch full PR diff; per-file diffs from files.json are still available.\"\n : > /tmp/pr-context/pr.diff\nfi\n\n# Changed files list (--paginate may output concatenated arrays; jq -s 'add' merges them)\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/files\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/files.json\n\n# Per-file diffs\njq -c '.[]' /tmp/pr-context/files.json | while IFS= read -r entry; do\n filename=$(echo \"$entry\" | jq -r '.filename')\n mkdir -p \"/tmp/pr-context/diffs/$(dirname \"$filename\")\"\n echo \"$entry\" | jq -r '.patch // empty' > \"/tmp/pr-context/diffs/${filename}.diff\"\ndone\n\n# File orderings for sub-agent review (3 strategies)\njq -r '[.[] | .filename] | sort | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_az.txt\njq -r '[.[] | .filename] | sort | reverse | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_za.txt\njq -r '[.[] | {filename, size: ((.additions // 0) + (.deletions // 0))}] | sort_by(-.size) | .[].filename' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_largest.txt\n\n# Existing reviews\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/reviews.json\n\n# Review threads with resolution status (GraphQL — REST lacks isResolved/isOutdated)\ngh api graphql --paginate -f query='\n query($owner: String!, $repo: String!, $number: Int!, $endCursor: String) {\n repository(owner: $owner, name: $repo) {\n pullRequest(number: $number) {\n reviewThreads(first: 100, after: $endCursor) {\n pageInfo { hasNextPage endCursor }\n nodes {\n id\n isResolved\n isOutdated\n isCollapsed\n path\n line\n startLine\n comments(first: 100) {\n nodes {\n id\n databaseId\n body\n author { login }\n createdAt\n }\n }\n }\n }\n }\n }\n }\n' -F owner=\"${GITHUB_REPOSITORY%/*}\" -F repo=\"${GITHUB_REPOSITORY#*/}\" -F \"number=$PR_NUMBER\" \\\n --jq '.data.repository.pullRequest.reviewThreads.nodes' \\\n | jq -s 'add // []' > /tmp/pr-context/review_comments.json\n\n# Per-file review threads (mirrors diffs/ structure)\njq -c '.[]' /tmp/pr-context/review_comments.json | while IFS= read -r thread; do\n filepath=$(echo \"$thread\" | jq -r '.path // empty')\n [ -z \"$filepath\" ] && continue\n mkdir -p \"/tmp/pr-context/threads/$(dirname \"$filepath\")\"\n echo \"$thread\" >> \"/tmp/pr-context/threads/${filepath}.jsonl\"\ndone\n# Convert per-file JSONL to proper JSON arrays\nmkdir -p /tmp/pr-context/threads\nfind /tmp/pr-context/threads -name '*.jsonl' 2>/dev/null | while IFS= read -r jsonl; do\n jq -s '.' \"$jsonl\" > \"${jsonl%.jsonl}.json\"\n rm \"$jsonl\"\ndone\n\n# PR discussion comments\ngh api \"repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/comments\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/comments.json\n\n# Linked issues\njq -r '.body // \"\"' /tmp/pr-context/pr.json 2>/dev/null \\\n | grep -oiE '(fixes|closes|resolves)\\s+#[0-9]+' \\\n | grep -oE '[0-9]+$' \\\n | sort -u \\\n | while read -r issue; do\n gh api \"repos/$GITHUB_REPOSITORY/issues/$issue\" > \"/tmp/pr-context/issue-${issue}.json\" || true\n done || true\n\n# Write manifest\ncat > /tmp/pr-context/README.md << 'MANIFEST'\n# PR Context\n\nPre-fetched PR data. All files are in `/tmp/pr-context/`.\n\n| File | Description |\n| --- | --- |\n| `pr.json` | PR metadata — title, body, author, base/head branches, head commit SHA (`headRefOid`), URL |\n| `pr.diff` | Full unified diff of all changes |\n| `files.json` | Changed files array — each entry has `filename`, `status`, `additions`, `deletions`, `patch` |\n| `diffs/.diff` | Per-file diffs — one file per changed file, mirroring the repo path under `diffs/` |\n| `file_order_az.txt` | Changed files sorted alphabetically (A→Z), one filename per line |\n| `file_order_za.txt` | Changed files sorted reverse-alphabetically (Z→A), one filename per line |\n| `file_order_largest.txt` | Changed files sorted by diff size descending (largest first), one filename per line |\n| `reviews.json` | Prior review submissions — author, state (APPROVED/CHANGES_REQUESTED/COMMENTED), body |\n| `review_comments.json` | All review threads (GraphQL) — each thread has `id` (node ID for resolving), `isResolved`, `isOutdated`, `path`, `line`, and nested `comments` with `id`, `databaseId` (numeric REST ID for replies), body/author |\n| `threads/.json` | Per-file review threads — one file per changed file with existing threads, mirroring the repo path under `threads/` |\n| `comments.json` | PR discussion comments (not inline) |\n| `issue-{N}.json` | Linked issue details (one file per linked issue, if any) |\n| `agents.md` | Repository conventions from `generate_agents_md` (if written by agent) |\n| `review-instructions.md` | Review instructions, criteria, and calibration examples (if written by review-process fragment) |\nMANIFEST\n\necho \"PR context written to /tmp/pr-context/\"\nls -la /tmp/pr-context/" - env: GITHUB_TOKEN: ${{ github.token }} REPO_NAME: ${{ github.repository }} @@ -1002,13 +1002,13 @@ jobs: "logDir": "/opt/gh-aw/safe-inputs/logs", "tools": [ { - "name": "ready-to-make-pr", - "description": "Run the PR readiness checklist before creating or updating a PR", + "name": "ready-to-push-to-pr", + "description": "Run the PR readiness checklist before pushing to a PR", "inputSchema": { "properties": {}, "type": "object" }, - "handler": "ready-to-make-pr.py", + "handler": "ready-to-push-to-pr.py", "timeout": 60 } ] @@ -1033,10 +1033,10 @@ jobs: - name: Setup Safe Inputs Tool Files run: | - cat > /opt/gh-aw/safe-inputs/ready-to-make-pr.py << 'GH_AW_SAFE_INPUTS_PY_READY-TO-MAKE-PR_EOF' + cat > /opt/gh-aw/safe-inputs/ready-to-push-to-pr.py << 'GH_AW_SAFE_INPUTS_PY_READY-TO-PUSH-TO-PR_EOF' #!/usr/bin/env python3 - # Auto-generated safe-input tool: ready-to-make-pr - # Run the PR readiness checklist before creating or updating a PR + # Auto-generated safe-input tool: ready-to-push-to-pr + # Run the PR readiness checklist before pushing to a PR import json import os @@ -1057,11 +1057,34 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect history rewrites and merge commits + pr_json_path = '/tmp/pr-context/pr.json' + if os.path.isfile(pr_json_path): + with open(pr_json_path) as f: + pr_data = json.load(f) + pr_head_sha = pr_data.get('headRefOid', '') + if pr_head_sha: + # Check 1: PR head must be an ancestor of HEAD (no rebase/reset) + anc = run(['git', 'merge-base', '--is-ancestor', pr_head_sha, 'HEAD']) + if anc.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'History rewrite detected: the original PR head ({pr_head_sha[:12]}) is not an ancestor of HEAD. This means git rebase, reset, or cherry-pick rewrote history. push_to_pull_request_branch will fail. Fix: run `git reset --hard {pr_head_sha}` to restore the PR branch to its original head, then re-apply your changes as direct file edits and commit as regular commits.'})) + raise SystemExit(0) + # Check 2: no merge commits (multiple parents) since PR head + log = run(['git', 'rev-list', '--min-parents=2', f'{pr_head_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for push.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... push_to_pull_request_branch uses git format-patch which breaks on merge commits. Fix: run `git reset --hard {pr_head_sha}` to restore the PR branch, then re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], @@ -1093,12 +1116,12 @@ jobs: if pr_template: checklist.append(f'Follow the PR template ({pr_template}) for title, description, and validation notes.') checklist.append('Confirm the requested task is fully completed and validated before creating or pushing PR changes.') if diff_line_count > 0: - checklist.append(f'A diff of your unpushed changes ({diff_line_count} lines) has been saved to `/tmp/self-review/diff.patch` (full diff) and `/tmp/self-review/stat.txt` (summary). Spawn a `code-review` sub-agent via `runSubagent` to review the diff against the codebase. Tell it to read `/tmp/self-review/diff.patch` and the relevant source files, and look for bugs, logic errors, missed edge cases, and style issues. If the sub-agent finds legitimate issues, fix them, commit, and call `ready_to_make_pr` again to regenerate the diff before proceeding.') + checklist.append(f'A diff of your unpushed changes ({diff_line_count} lines) has been saved to `/tmp/self-review/diff.patch` (full diff) and `/tmp/self-review/stat.txt` (summary). Spawn a `code-review` sub-agent via `runSubagent` to review the diff against the codebase. Tell it to read `/tmp/self-review/diff.patch` and the relevant source files, and look for bugs, logic errors, missed edge cases, and style issues. If the sub-agent finds legitimate issues, fix them, commit, and call `ready_to_push_to_pr` again to regenerate the diff before proceeding.') print(json.dumps({'status': 'ok', 'checklist': checklist, 'contributing_guide': contributing, 'pr_template': pr_template, 'diff_line_count': diff_line_count})) - GH_AW_SAFE_INPUTS_PY_READY-TO-MAKE-PR_EOF - chmod +x /opt/gh-aw/safe-inputs/ready-to-make-pr.py + GH_AW_SAFE_INPUTS_PY_READY-TO-PUSH-TO-PR_EOF + chmod +x /opt/gh-aw/safe-inputs/ready-to-push-to-pr.py - name: Generate Safe Inputs MCP Server Config id: safe-inputs-config diff --git a/.github/workflows/gh-aw-pr-review.lock.yml b/.github/workflows/gh-aw-pr-review.lock.yml index ebdffd12..096dcb9b 100644 --- a/.github/workflows/gh-aw-pr-review.lock.yml +++ b/.github/workflows/gh-aw-pr-review.lock.yml @@ -40,7 +40,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"1b2161458056b26894b5284f34bbf728738acc188f845d353b31224304013428"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"68a777412c77ebdcfffccbeeb3b012b97cf87e39d9dc272ed9536f3be1993cf4"} name: "PR Review" "on": @@ -685,7 +685,7 @@ jobs: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ github.event.pull_request.number || inputs.target-pr-number || github.event.issue.number }} name: Fetch PR context to disk - run: "set -euo pipefail\nmkdir -p /tmp/pr-context\n\n# PR metadata\ngh pr view \"$PR_NUMBER\" --json title,body,author,baseRefName,headRefName,url \\\n > /tmp/pr-context/pr.json\n\n# Full diff\nif ! gh pr diff \"$PR_NUMBER\" > /tmp/pr-context/pr.diff; then\n echo \"::warning::Failed to fetch full PR diff; per-file diffs from files.json are still available.\"\n : > /tmp/pr-context/pr.diff\nfi\n\n# Changed files list (--paginate may output concatenated arrays; jq -s 'add' merges them)\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/files\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/files.json\n\n# Per-file diffs\njq -c '.[]' /tmp/pr-context/files.json | while IFS= read -r entry; do\n filename=$(echo \"$entry\" | jq -r '.filename')\n mkdir -p \"/tmp/pr-context/diffs/$(dirname \"$filename\")\"\n echo \"$entry\" | jq -r '.patch // empty' > \"/tmp/pr-context/diffs/${filename}.diff\"\ndone\n\n# File orderings for sub-agent review (3 strategies)\njq -r '[.[] | .filename] | sort | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_az.txt\njq -r '[.[] | .filename] | sort | reverse | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_za.txt\njq -r '[.[] | {filename, size: ((.additions // 0) + (.deletions // 0))}] | sort_by(-.size) | .[].filename' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_largest.txt\n\n# Existing reviews\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/reviews.json\n\n# Review threads with resolution status (GraphQL — REST lacks isResolved/isOutdated)\ngh api graphql --paginate -f query='\n query($owner: String!, $repo: String!, $number: Int!, $endCursor: String) {\n repository(owner: $owner, name: $repo) {\n pullRequest(number: $number) {\n reviewThreads(first: 100, after: $endCursor) {\n pageInfo { hasNextPage endCursor }\n nodes {\n id\n isResolved\n isOutdated\n isCollapsed\n path\n line\n startLine\n comments(first: 100) {\n nodes {\n id\n databaseId\n body\n author { login }\n createdAt\n }\n }\n }\n }\n }\n }\n }\n' -F owner=\"${GITHUB_REPOSITORY%/*}\" -F repo=\"${GITHUB_REPOSITORY#*/}\" -F \"number=$PR_NUMBER\" \\\n --jq '.data.repository.pullRequest.reviewThreads.nodes' \\\n | jq -s 'add // []' > /tmp/pr-context/review_comments.json\n\n# Per-file review threads (mirrors diffs/ structure)\njq -c '.[]' /tmp/pr-context/review_comments.json | while IFS= read -r thread; do\n filepath=$(echo \"$thread\" | jq -r '.path // empty')\n [ -z \"$filepath\" ] && continue\n mkdir -p \"/tmp/pr-context/threads/$(dirname \"$filepath\")\"\n echo \"$thread\" >> \"/tmp/pr-context/threads/${filepath}.jsonl\"\ndone\n# Convert per-file JSONL to proper JSON arrays\nmkdir -p /tmp/pr-context/threads\nfind /tmp/pr-context/threads -name '*.jsonl' 2>/dev/null | while IFS= read -r jsonl; do\n jq -s '.' \"$jsonl\" > \"${jsonl%.jsonl}.json\"\n rm \"$jsonl\"\ndone\n\n# PR discussion comments\ngh api \"repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/comments\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/comments.json\n\n# Linked issues\njq -r '.body // \"\"' /tmp/pr-context/pr.json 2>/dev/null \\\n | grep -oiE '(fixes|closes|resolves)\\s+#[0-9]+' \\\n | grep -oE '[0-9]+$' \\\n | sort -u \\\n | while read -r issue; do\n gh api \"repos/$GITHUB_REPOSITORY/issues/$issue\" > \"/tmp/pr-context/issue-${issue}.json\" || true\n done || true\n\n# Write manifest\ncat > /tmp/pr-context/README.md << 'MANIFEST'\n# PR Context\n\nPre-fetched PR data. All files are in `/tmp/pr-context/`.\n\n| File | Description |\n| --- | --- |\n| `pr.json` | PR metadata — title, body, author, base/head branches, URL |\n| `pr.diff` | Full unified diff of all changes |\n| `files.json` | Changed files array — each entry has `filename`, `status`, `additions`, `deletions`, `patch` |\n| `diffs/.diff` | Per-file diffs — one file per changed file, mirroring the repo path under `diffs/` |\n| `file_order_az.txt` | Changed files sorted alphabetically (A→Z), one filename per line |\n| `file_order_za.txt` | Changed files sorted reverse-alphabetically (Z→A), one filename per line |\n| `file_order_largest.txt` | Changed files sorted by diff size descending (largest first), one filename per line |\n| `reviews.json` | Prior review submissions — author, state (APPROVED/CHANGES_REQUESTED/COMMENTED), body |\n| `review_comments.json` | All review threads (GraphQL) — each thread has `id` (node ID for resolving), `isResolved`, `isOutdated`, `path`, `line`, and nested `comments` with `id`, `databaseId` (numeric REST ID for replies), body/author |\n| `threads/.json` | Per-file review threads — one file per changed file with existing threads, mirroring the repo path under `threads/` |\n| `comments.json` | PR discussion comments (not inline) |\n| `issue-{N}.json` | Linked issue details (one file per linked issue, if any) |\n| `agents.md` | Repository conventions from `generate_agents_md` (if written by agent) |\n| `review-instructions.md` | Review instructions, criteria, and calibration examples (if written by review-process fragment) |\nMANIFEST\n\necho \"PR context written to /tmp/pr-context/\"\nls -la /tmp/pr-context/" + run: "set -euo pipefail\nmkdir -p /tmp/pr-context\n\n# PR metadata\ngh pr view \"$PR_NUMBER\" --json title,body,author,baseRefName,headRefName,headRefOid,url \\\n > /tmp/pr-context/pr.json\n\n# Full diff\nif ! gh pr diff \"$PR_NUMBER\" > /tmp/pr-context/pr.diff; then\n echo \"::warning::Failed to fetch full PR diff; per-file diffs from files.json are still available.\"\n : > /tmp/pr-context/pr.diff\nfi\n\n# Changed files list (--paginate may output concatenated arrays; jq -s 'add' merges them)\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/files\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/files.json\n\n# Per-file diffs\njq -c '.[]' /tmp/pr-context/files.json | while IFS= read -r entry; do\n filename=$(echo \"$entry\" | jq -r '.filename')\n mkdir -p \"/tmp/pr-context/diffs/$(dirname \"$filename\")\"\n echo \"$entry\" | jq -r '.patch // empty' > \"/tmp/pr-context/diffs/${filename}.diff\"\ndone\n\n# File orderings for sub-agent review (3 strategies)\njq -r '[.[] | .filename] | sort | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_az.txt\njq -r '[.[] | .filename] | sort | reverse | .[]' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_za.txt\njq -r '[.[] | {filename, size: ((.additions // 0) + (.deletions // 0))}] | sort_by(-.size) | .[].filename' /tmp/pr-context/files.json \\\n > /tmp/pr-context/file_order_largest.txt\n\n# Existing reviews\ngh api \"repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/reviews.json\n\n# Review threads with resolution status (GraphQL — REST lacks isResolved/isOutdated)\ngh api graphql --paginate -f query='\n query($owner: String!, $repo: String!, $number: Int!, $endCursor: String) {\n repository(owner: $owner, name: $repo) {\n pullRequest(number: $number) {\n reviewThreads(first: 100, after: $endCursor) {\n pageInfo { hasNextPage endCursor }\n nodes {\n id\n isResolved\n isOutdated\n isCollapsed\n path\n line\n startLine\n comments(first: 100) {\n nodes {\n id\n databaseId\n body\n author { login }\n createdAt\n }\n }\n }\n }\n }\n }\n }\n' -F owner=\"${GITHUB_REPOSITORY%/*}\" -F repo=\"${GITHUB_REPOSITORY#*/}\" -F \"number=$PR_NUMBER\" \\\n --jq '.data.repository.pullRequest.reviewThreads.nodes' \\\n | jq -s 'add // []' > /tmp/pr-context/review_comments.json\n\n# Per-file review threads (mirrors diffs/ structure)\njq -c '.[]' /tmp/pr-context/review_comments.json | while IFS= read -r thread; do\n filepath=$(echo \"$thread\" | jq -r '.path // empty')\n [ -z \"$filepath\" ] && continue\n mkdir -p \"/tmp/pr-context/threads/$(dirname \"$filepath\")\"\n echo \"$thread\" >> \"/tmp/pr-context/threads/${filepath}.jsonl\"\ndone\n# Convert per-file JSONL to proper JSON arrays\nmkdir -p /tmp/pr-context/threads\nfind /tmp/pr-context/threads -name '*.jsonl' 2>/dev/null | while IFS= read -r jsonl; do\n jq -s '.' \"$jsonl\" > \"${jsonl%.jsonl}.json\"\n rm \"$jsonl\"\ndone\n\n# PR discussion comments\ngh api \"repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/comments\" --paginate \\\n | jq -s 'add // []' > /tmp/pr-context/comments.json\n\n# Linked issues\njq -r '.body // \"\"' /tmp/pr-context/pr.json 2>/dev/null \\\n | grep -oiE '(fixes|closes|resolves)\\s+#[0-9]+' \\\n | grep -oE '[0-9]+$' \\\n | sort -u \\\n | while read -r issue; do\n gh api \"repos/$GITHUB_REPOSITORY/issues/$issue\" > \"/tmp/pr-context/issue-${issue}.json\" || true\n done || true\n\n# Write manifest\ncat > /tmp/pr-context/README.md << 'MANIFEST'\n# PR Context\n\nPre-fetched PR data. All files are in `/tmp/pr-context/`.\n\n| File | Description |\n| --- | --- |\n| `pr.json` | PR metadata — title, body, author, base/head branches, head commit SHA (`headRefOid`), URL |\n| `pr.diff` | Full unified diff of all changes |\n| `files.json` | Changed files array — each entry has `filename`, `status`, `additions`, `deletions`, `patch` |\n| `diffs/.diff` | Per-file diffs — one file per changed file, mirroring the repo path under `diffs/` |\n| `file_order_az.txt` | Changed files sorted alphabetically (A→Z), one filename per line |\n| `file_order_za.txt` | Changed files sorted reverse-alphabetically (Z→A), one filename per line |\n| `file_order_largest.txt` | Changed files sorted by diff size descending (largest first), one filename per line |\n| `reviews.json` | Prior review submissions — author, state (APPROVED/CHANGES_REQUESTED/COMMENTED), body |\n| `review_comments.json` | All review threads (GraphQL) — each thread has `id` (node ID for resolving), `isResolved`, `isOutdated`, `path`, `line`, and nested `comments` with `id`, `databaseId` (numeric REST ID for replies), body/author |\n| `threads/.json` | Per-file review threads — one file per changed file with existing threads, mirroring the repo path under `threads/` |\n| `comments.json` | PR discussion comments (not inline) |\n| `issue-{N}.json` | Linked issue details (one file per linked issue, if any) |\n| `agents.md` | Repository conventions from `generate_agents_md` (if written by agent) |\n| `review-instructions.md` | Review instructions, criteria, and calibration examples (if written by review-process fragment) |\nMANIFEST\n\necho \"PR context written to /tmp/pr-context/\"\nls -la /tmp/pr-context/" - name: Write review instructions to disk run: "mkdir -p /tmp/pr-context\ncat > /tmp/pr-context/review-instructions.md << 'REVIEW_EOF'\n# Review Instructions for Sub-agents\n\nYou are a code review sub-agent. Read these instructions, then review the PR files in the order provided in your prompt.\n\n## Context\n\nBefore reviewing files, read these to understand the PR:\n\n1. `/tmp/pr-context/pr.json` — PR title, description, author, and branches. Understand what the PR is trying to accomplish.\n2. `/tmp/pr-context/agents.md` — Repository coding conventions and guidelines (if it exists).\n3. `/tmp/pr-context/review_comments.json` — Existing review threads. Note which files already have threads so you don't duplicate.\n4. `/tmp/pr-context/issue-*.json` — Linked issue details (if any). Understand the motivation and acceptance criteria.\n\n## Process\n\nReview the PR diff file by file in your assigned order. For each changed file:\n\n1. **Read the diff** for this file from `/tmp/pr-context/diffs/.diff` to understand what changed. If the diff is empty or truncated (e.g., binary files or very large changes), fall back to reading the full file from the workspace and comparing against context.\n2. **Read the full file from the workspace.** The PR branch is checked out locally — open the file directly to get complete contents with line numbers.\n3. **Check existing threads** for this file from `/tmp/pr-context/threads/.json` (if it exists). Skip issues that are already under discussion — each thread has `isResolved` and `isOutdated` fields.\n4. **Identify potential issues** matching the review criteria below.\n5. **Quick-check each issue** before including it:\n - What specific code pattern or change triggers this concern?\n - Is there an obvious guard, handler, or mitigation visible in the immediate context?\n - Can you describe a concrete failure scenario (the `evidence` field)? If you cannot articulate what specific input or state triggers the problem, drop the finding.\n - If the issue is clearly handled, skip it. If you're unsure, include it — the parent will verify.\n6. **Add to your findings list.** Do NOT leave inline comments — you don't have that tool. Return findings in this format:\n\n```\n- file: path/to/file\n line: 42\n severity: HIGH\n title: Brief title\n description: What the issue is and why it matters\n evidence: The specific code pattern and failure scenario\n suggestion: corrected code here (optional — only if you can provide a concrete fix)\n```\n\n**Review every file in your assigned order.** Files reviewed earlier get more attention, which is why different sub-agents use different orderings.\n\n**Check existing threads** — per-file threads are at `/tmp/pr-context/threads/.json` (step 3 above). The full list is at `/tmp/pr-context/review_comments.json`. Do not flag issues that are already under discussion (resolved or unresolved). For outdated threads, only re-flag if the issue still applies to the current diff.\n\n**Return your full findings list** when done, or an empty list if no issues were found.\n\n## Review Criteria\n\nFocus on these categories in priority order:\n\n1. Security vulnerabilities (injection, XSS, auth bypass, secrets exposure)\n2. Logic bugs that could cause runtime failures or incorrect behavior\n3. Data integrity issues (race conditions, missing transactions, corruption risk)\n4. Performance bottlenecks (N+1 queries, memory leaks, blocking operations)\n5. Error handling gaps (unhandled exceptions, missing validation)\n6. Breaking changes to public APIs without migration path\n7. Missing or incorrect test coverage for critical paths\n\n## What NOT to Flag\n\nOnly review the diff — do not flag issues in unchanged code, pre-existing problems not introduced by this PR, or style preferences handled by linters or formatters.\n\n**Common false positives** — these patterns look like issues but usually aren't. Before flagging anything in these categories, confirm the problem is real by reading the surrounding code:\n\n- **Security — input already sanitized:** Don't flag injection or XSS risks when inputs are sanitized upstream, parameterized queries are used, or the framework auto-escapes output.\n- **Null/undefined — guarded elsewhere:** Don't flag potential null dereferences if the value is guaranteed by a type guard, assertion, schema validation, or upstream null check.\n- **Error handling — handled at a different layer:** Don't flag missing try/catch if the caller, middleware, or framework catches and handles the error (e.g., Express error middleware, React error boundaries).\n- **Performance — theoretical, not practical:** Don't flag algorithmic complexity (e.g., O(n^2)) unless N is demonstrably large enough to matter in the actual usage context. \"This could be slow\" without evidence is not actionable.\n- **Validation — exists at another layer:** Don't flag missing input validation if it's handled by an API gateway, middleware, schema validator, or type system.\n- **Test coverage — trivial or generated code:** Don't flag missing tests for trivial getters/setters, auto-generated code, or simple delegation methods.\n- **Style or naming — not in coding guidelines:** Don't flag naming conventions or code style unless they violate the repository's documented coding guidelines (from `generate_agents_md` or CONTRIBUTING docs).\n\n**Existing review threads** — check BEFORE flagging any issue:\n\n- **Resolved with reviewer reply** (e.g. \"This is intentional\") — reviewer's decision is final. Do NOT re-flag.\n- **Resolved without reply** — author likely fixed it. Do NOT re-raise unless the fix introduced a new problem.\n- **Unresolved** — already flagged. Do NOT duplicate.\n- **Outdated** — only re-flag if the issue still applies to the current diff.\n\nWhen in doubt, do not duplicate. Redundant comments erode trust.\n\nFinding no issues is a valid and valuable outcome. An empty findings list is better than findings that waste the author's time or erode trust. Do not manufacture findings to justify your review — if the code is sound, return an empty list.\n\n## Severity Classification\n\nDetermine severity AFTER investigating the issue, not before. First identify the problem and trace through the code, then assign a severity based on the evidence you found.\n\n- 🔴 CRITICAL — Must fix before merge (security vulnerabilities, data corruption, production-breaking bugs)\n- 🟠 HIGH — Should fix before merge (logic errors, missing validation, significant performance issues)\n- 🟡 MEDIUM — Address soon, non-blocking (error handling gaps, suboptimal patterns, missing edge cases)\n- ⚪ LOW — Author discretion (minor improvements, documentation gaps)\n- 💬 NITPICK — Truly optional (stylistic preferences, alternative approaches)\n\n## Review Intensity\n\nThe review intensity is `${{ inputs.intensity || 'balanced' }}`.\n\n- **conservative**: High evidence bar. Only flag when you can demonstrate a concrete failure scenario. If you can construct a reasonable counterargument, do not flag. Approval with zero findings is the expected outcome for most PRs.\n- **balanced**: Standard evidence bar. Flag when you can point to specific code that would fail. If the issue is ambiguous, lean toward not flagging.\n- **aggressive**: Lower evidence bar. Flag when evidence exists even if the failure scenario is not fully confirmed. Improvement suggestions welcome but must cite specific code.\n\n## Calibration Examples\n\nUse these examples to calibrate your judgment. Each pair shows a real issue and a similar-looking pattern that is NOT an issue.\n\n### Example 1: Null/Undefined Access\n\n**True positive — flag this:**\n\n```js\n// PR adds this handler\napp.get('/user/:id', async (req, res) => {\n const user = await db.findUser(req.params.id);\n res.json({ name: user.name, email: user.email });\n});\n```\n\nWhy flag: `db.findUser()` can return `null` when no user matches the ID. Accessing `user.name` will throw a TypeError at runtime. No upstream guard exists — the route handler is the entry point.\n\n**False positive — do NOT flag this:**\n\n```ts\n// PR adds this line inside an existing function\nconst settings = user.getSettings();\n```\n\nWhy skip: Reading the full file reveals `user` is typed as `User` (not `User | null`), and the calling function only runs after `authenticateUser()` middleware which guarantees a valid user object. The null case is handled at a different layer.\n\n### Example 2: SQL Injection\n\n**True positive — flag this:**\n\n```python\n# PR adds this query\ncursor.execute(f\"SELECT * FROM orders WHERE customer_id = '{customer_id}'\")\n```\n\nWhy flag: String interpolation in a SQL query with user-controlled input (`customer_id` comes from the request). No parameterization or sanitization anywhere in the call chain.\n\n**False positive — do NOT flag this:**\n\n```python\n# PR adds this query\ncursor.execute(f\"SELECT * FROM orders WHERE status = '{OrderStatus.PENDING.value}'\")\n```\n\nWhy skip: The interpolated value is a hardcoded enum constant (`OrderStatus.PENDING`), not user input. There is no injection vector.\n\n### Example 3: Borderline — Do NOT Flag\n\n```go\n// PR adds this function\nfunc processItems(items []Item) []Result {\n results := make([]Result, 0)\n for _, item := range items {\n for _, tag := range item.Tags {\n results = append(results, process(item, tag))\n }\n }\n return results\n}\n```\n\nThis looks like an O(n*m) performance concern. But without evidence that `items` or `Tags` are large in practice, this is speculative. The function processes a bounded dataset (items from a single user request). Do not flag theoretical performance issues without evidence of real-world impact.\nREVIEW_EOF" - env: diff --git a/.github/workflows/gh-aw-release-update.lock.yml b/.github/workflows/gh-aw-release-update.lock.yml index e69b541c..45eb4ec5 100644 --- a/.github/workflows/gh-aw-release-update.lock.yml +++ b/.github/workflows/gh-aw-release-update.lock.yml @@ -36,7 +36,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"540fed2332d999fe5901bc59a882c439fe966cffe03b7910e56f993df13555e6"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"85a99e6d4dafa3f0dc4211e76df3cfe1eb8e942f35d264440861fd9e7f08a70e"} name: "Release Update Check" "on": @@ -903,11 +903,32 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect merge commits + # Find the fork point with the upstream branch to scope the check + upstream_sha = '' + for ref in ['@{upstream}', 'origin/HEAD', 'origin/main']: + r = run(['git', 'merge-base', 'HEAD', ref]) + if r.returncode == 0 and r.stdout.strip(): + upstream_sha = r.stdout.strip() + break + if not upstream_sha: + print(json.dumps({'status': 'error', 'error': 'Unable to determine upstream fork point for merge-commit validation. Fix: ensure remotes are fetched and a tracking branch is set (e.g., `git branch --set-upstream-to origin/`), then rerun ready_to_make_pr.'})) + raise SystemExit(0) + log = run(['git', 'rev-list', '--min-parents=2', f'{upstream_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for PR creation.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... create_pull_request uses git format-patch which breaks on merge commits. Fix: re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], diff --git a/.github/workflows/gh-aw-scheduled-fix.lock.yml b/.github/workflows/gh-aw-scheduled-fix.lock.yml index 8becb0c8..7d16b783 100644 --- a/.github/workflows/gh-aw-scheduled-fix.lock.yml +++ b/.github/workflows/gh-aw-scheduled-fix.lock.yml @@ -37,7 +37,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"18eac1fd19c9aa09b0f306acbbc1b3a08d4f214f087b25be62fb8b5b8a947e7d"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"c9cffce0af80d574d9cb7ee55dd471ab58dd3fdc5529b07ddcb809be0792db61"} name: "Scheduled Fix" "on": @@ -958,11 +958,32 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect merge commits + # Find the fork point with the upstream branch to scope the check + upstream_sha = '' + for ref in ['@{upstream}', 'origin/HEAD', 'origin/main']: + r = run(['git', 'merge-base', 'HEAD', ref]) + if r.returncode == 0 and r.stdout.strip(): + upstream_sha = r.stdout.strip() + break + if not upstream_sha: + print(json.dumps({'status': 'error', 'error': 'Unable to determine upstream fork point for merge-commit validation. Fix: ensure remotes are fetched and a tracking branch is set (e.g., `git branch --set-upstream-to origin/`), then rerun ready_to_make_pr.'})) + raise SystemExit(0) + log = run(['git', 'rev-list', '--min-parents=2', f'{upstream_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for PR creation.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... create_pull_request uses git format-patch which breaks on merge commits. Fix: re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], diff --git a/.github/workflows/gh-aw-small-problem-fixer.lock.yml b/.github/workflows/gh-aw-small-problem-fixer.lock.yml index 08806e21..5350e29e 100644 --- a/.github/workflows/gh-aw-small-problem-fixer.lock.yml +++ b/.github/workflows/gh-aw-small-problem-fixer.lock.yml @@ -37,7 +37,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"bba6645ad30ad38083fb374055d6952f84711b2304ebe53704bb5f00a7043069"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"684652f5d6bff5a991b9a97623abddaaa11931c82bc93e51c417a1d53a487e57"} name: "Small Problem Fixer" "on": @@ -986,11 +986,32 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect merge commits + # Find the fork point with the upstream branch to scope the check + upstream_sha = '' + for ref in ['@{upstream}', 'origin/HEAD', 'origin/main']: + r = run(['git', 'merge-base', 'HEAD', ref]) + if r.returncode == 0 and r.stdout.strip(): + upstream_sha = r.stdout.strip() + break + if not upstream_sha: + print(json.dumps({'status': 'error', 'error': 'Unable to determine upstream fork point for merge-commit validation. Fix: ensure remotes are fetched and a tracking branch is set (e.g., `git branch --set-upstream-to origin/`), then rerun ready_to_make_pr.'})) + raise SystemExit(0) + log = run(['git', 'rev-list', '--min-parents=2', f'{upstream_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for PR creation.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... create_pull_request uses git format-patch which breaks on merge commits. Fix: re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], diff --git a/.github/workflows/gh-aw-test-improvement.lock.yml b/.github/workflows/gh-aw-test-improvement.lock.yml index f8296b42..ef4b9b15 100644 --- a/.github/workflows/gh-aw-test-improvement.lock.yml +++ b/.github/workflows/gh-aw-test-improvement.lock.yml @@ -41,7 +41,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"08d168e02162376fdd5be78c4b6cfa1f910732fad86deb9e101fab2c9271416d"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"341673b6d22586c36cdb7edda52391d0620d0de567ee8751cfa21ff206917051"} name: "Test Improver" "on": @@ -946,11 +946,32 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect merge commits + # Find the fork point with the upstream branch to scope the check + upstream_sha = '' + for ref in ['@{upstream}', 'origin/HEAD', 'origin/main']: + r = run(['git', 'merge-base', 'HEAD', ref]) + if r.returncode == 0 and r.stdout.strip(): + upstream_sha = r.stdout.strip() + break + if not upstream_sha: + print(json.dumps({'status': 'error', 'error': 'Unable to determine upstream fork point for merge-commit validation. Fix: ensure remotes are fetched and a tracking branch is set (e.g., `git branch --set-upstream-to origin/`), then rerun ready_to_make_pr.'})) + raise SystemExit(0) + log = run(['git', 'rev-list', '--min-parents=2', f'{upstream_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for PR creation.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... create_pull_request uses git format-patch which breaks on merge commits. Fix: re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], diff --git a/.github/workflows/gh-aw-test-improver.lock.yml b/.github/workflows/gh-aw-test-improver.lock.yml index 0106c706..21a4233d 100644 --- a/.github/workflows/gh-aw-test-improver.lock.yml +++ b/.github/workflows/gh-aw-test-improver.lock.yml @@ -36,7 +36,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"08d168e02162376fdd5be78c4b6cfa1f910732fad86deb9e101fab2c9271416d"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"341673b6d22586c36cdb7edda52391d0620d0de567ee8751cfa21ff206917051"} name: "Test Improver" "on": @@ -941,11 +941,32 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect merge commits + # Find the fork point with the upstream branch to scope the check + upstream_sha = '' + for ref in ['@{upstream}', 'origin/HEAD', 'origin/main']: + r = run(['git', 'merge-base', 'HEAD', ref]) + if r.returncode == 0 and r.stdout.strip(): + upstream_sha = r.stdout.strip() + break + if not upstream_sha: + print(json.dumps({'status': 'error', 'error': 'Unable to determine upstream fork point for merge-commit validation. Fix: ensure remotes are fetched and a tracking branch is set (e.g., `git branch --set-upstream-to origin/`), then rerun ready_to_make_pr.'})) + raise SystemExit(0) + log = run(['git', 'rev-list', '--min-parents=2', f'{upstream_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for PR creation.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... create_pull_request uses git format-patch which breaks on merge commits. Fix: re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], diff --git a/.github/workflows/gh-aw-text-beautifier.lock.yml b/.github/workflows/gh-aw-text-beautifier.lock.yml index 445d7740..641158d2 100644 --- a/.github/workflows/gh-aw-text-beautifier.lock.yml +++ b/.github/workflows/gh-aw-text-beautifier.lock.yml @@ -38,7 +38,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"8276eb97a80c0abd997c5d568c174ee440b4fa4624945e95e0e21aa0a1bb3f37"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"9316e7680e177a5213dbd790b69a0d8cea2738c8f8df76699ce73478b1aaa446"} name: "Text Beautifier" "on": @@ -943,11 +943,32 @@ jobs: return subprocess.run(cmd, capture_output=True, text=True, timeout=60) except subprocess.TimeoutExpired: return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: detect merge commits + # Find the fork point with the upstream branch to scope the check + upstream_sha = '' + for ref in ['@{upstream}', 'origin/HEAD', 'origin/main']: + r = run(['git', 'merge-base', 'HEAD', ref]) + if r.returncode == 0 and r.stdout.strip(): + upstream_sha = r.stdout.strip() + break + if not upstream_sha: + print(json.dumps({'status': 'error', 'error': 'Unable to determine upstream fork point for merge-commit validation. Fix: ensure remotes are fetched and a tracking branch is set (e.g., `git branch --set-upstream-to origin/`), then rerun ready_to_make_pr.'})) + raise SystemExit(0) + log = run(['git', 'rev-list', '--min-parents=2', f'{upstream_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for PR creation.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... create_pull_request uses git format-patch which breaks on merge commits. Fix: re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') # Generate diff of all local changes vs upstream for self-review - # Try --merge-base (committed+staged+unstaged vs upstream), fall back to - # @{upstream} 2-dot (committed only), then HEAD (uncommitted only) + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) diff_text = '' for diff_cmd in [ ['git', 'diff', '--merge-base', '@{upstream}'], diff --git a/github/workflows/gh-aw-fragments/safe-output-push-to-pr.md b/github/workflows/gh-aw-fragments/safe-output-push-to-pr.md new file mode 100644 index 00000000..b02286f7 --- /dev/null +++ b/github/workflows/gh-aw-fragments/safe-output-push-to-pr.md @@ -0,0 +1,109 @@ +--- +safe-inputs: + ready-to-push-to-pr: + description: "Run the PR readiness checklist before pushing to a PR" + py: | + import os, json, subprocess + def find(*paths): + return next((p for p in paths if os.path.isfile(p)), None) + def run(cmd): + try: + return subprocess.run(cmd, capture_output=True, text=True, timeout=60) + except subprocess.TimeoutExpired: + return subprocess.CompletedProcess(cmd, 1, stdout='', stderr='diff timed out') + + # Guard: fail early on prohibited workflow file changes + changed_files = [] + for names_cmd in [ + ['git', 'diff', '--name-only', '--merge-base', '@{upstream}'], + ['git', 'diff', '--name-only', '@{upstream}'], + ['git', 'diff', '--name-only', 'HEAD'], + ]: + names = run(names_cmd) + if names.stdout.strip(): + changed_files = [line.strip() for line in names.stdout.splitlines() if line.strip()] + break + if any(path.startswith('.github/workflows/') for path in changed_files): + print(json.dumps({'status': 'error', 'error': 'Changes under .github/workflows/ detected in your local branch. push_to_pull_request_branch will reject these changes. Fix: move proposed workflow edits to matching paths under `github/workflows/` (without the leading dot), then ask a maintainer to relocate them to `.github/workflows/`.'})) + raise SystemExit(0) + + # Guard: detect history rewrites and merge commits + pr_json_path = '/tmp/pr-context/pr.json' + if os.path.isfile(pr_json_path): + with open(pr_json_path) as f: + pr_data = json.load(f) + pr_head_sha = pr_data.get('headRefOid', '') + if pr_head_sha: + # Check 1: PR head must be an ancestor of HEAD (no rebase/reset) + anc = run(['git', 'merge-base', '--is-ancestor', pr_head_sha, 'HEAD']) + if anc.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'History rewrite detected: the original PR head ({pr_head_sha[:12]}) is not an ancestor of HEAD. This means git rebase, reset, or cherry-pick rewrote history. push_to_pull_request_branch will fail. Fix: run `git reset --hard {pr_head_sha}` to restore the PR branch to its original head, then re-apply your changes as direct file edits and commit as regular commits.'})) + raise SystemExit(0) + # Check 2: no merge commits (multiple parents) since PR head + log = run(['git', 'rev-list', '--min-parents=2', f'{pr_head_sha}..HEAD']) + if log.returncode != 0: + print(json.dumps({'status': 'error', 'error': f'Failed to check for merge commits (git rev-list exited {log.returncode}): {log.stderr.strip()}. Cannot verify commit history is safe for push.'})) + raise SystemExit(0) + merge_shas = log.stdout.strip() + if merge_shas: + print(json.dumps({'status': 'error', 'error': f'Merge commit(s) detected: {merge_shas.splitlines()[0][:12]}... push_to_pull_request_branch uses git format-patch which breaks on merge commits. Fix: run `git reset --hard {pr_head_sha}` to restore the PR branch, then re-apply your changes as direct file edits (no git merge/rebase/commit-tree with multiple -p flags) and commit as regular single-parent commits.'})) + raise SystemExit(0) + + contributing = find('CONTRIBUTING.md', 'CONTRIBUTING.rst', 'docs/CONTRIBUTING.md', 'docs/contributing.md') + pr_template = find('.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md', '.github/PULL_REQUEST_TEMPLATE/pull_request_template.md') + # Generate diff of all local changes vs upstream for self-review + # Try --merge-base (vs common ancestor), fall back to + # @{upstream} 2-dot (vs upstream tip), then HEAD (uncommitted only) + diff_text = '' + for diff_cmd in [ + ['git', 'diff', '--merge-base', '@{upstream}'], + ['git', 'diff', '@{upstream}'], + ['git', 'diff', 'HEAD'], + ]: + result = run(diff_cmd) + if result.stdout.strip(): + diff_text = result.stdout.strip() + break + stat_text = '' + for stat_cmd in [ + ['git', 'diff', '--stat', '--merge-base', '@{upstream}'], + ['git', 'diff', '--stat', '@{upstream}'], + ['git', 'diff', '--stat', 'HEAD'], + ]: + result = run(stat_cmd) + if result.stdout.strip(): + stat_text = result.stdout.strip() + break + os.makedirs('/tmp/self-review', exist_ok=True) + with open('/tmp/self-review/diff.patch', 'w') as f: + f.write(diff_text) + with open('/tmp/self-review/stat.txt', 'w') as f: + f.write(stat_text) + diff_line_count = len(diff_text.splitlines()) + checklist = [] + if contributing: checklist.append(f'Review the contributing guide ({contributing}) before opening or updating a PR.') + if pr_template: checklist.append(f'Follow the PR template ({pr_template}) for title, description, and validation notes.') + checklist.append('Confirm the requested task is fully completed and validated before creating or pushing PR changes.') + if diff_line_count > 0: + checklist.append(f'A diff of your unpushed changes ({diff_line_count} lines) has been saved to `/tmp/self-review/diff.patch` (full diff) and `/tmp/self-review/stat.txt` (summary). Spawn a `code-review` sub-agent via `runSubagent` to review the diff against the codebase. Tell it to read `/tmp/self-review/diff.patch` and the relevant source files, and look for bugs, logic errors, missed edge cases, and style issues. If the sub-agent finds legitimate issues, fix them, commit, and call `ready_to_push_to_pr` again to regenerate the diff before proceeding.') + print(json.dumps({'status': 'ok', 'checklist': checklist, 'contributing_guide': contributing, 'pr_template': pr_template, 'diff_line_count': diff_line_count})) +safe-outputs: + push-to-pull-request-branch: + github-token-for-extra-empty-commit: ${{ secrets.EXTRA_COMMIT_GITHUB_TOKEN }} +--- + +Before calling `push_to_pull_request_branch`, call `ready_to_push_to_pr` and apply its checklist. + +## push-to-pull-request-branch Limitations + +- **Patch size**: Max ~10 MB (10,240 KB). Keep changes focused — very large refactors may exceed this. +- **Fork PRs**: Cannot push to fork PR branches. Check via `pull_request_read` with method `get` whether the PR head repo differs from the base repo. If it's a fork, explain that you cannot push and suggest the author apply changes themselves. +- **Committed changes required**: You must have locally committed changes before calling push. Uncommitted or staged-only changes will fail. +- **Branch**: Pushes to the PR's head branch. The workspace must have the PR branch checked out. +- You may not submit code that modifies files in `.github/workflows/`. Doing so will cause the submission to be rejected. If asked to modify workflow files, propose the change in a copy placed in a `github/` folder (without the leading period) and note in the PR that the file needs to be relocated by someone with workflow write access. + +Trying to resolve merge conflicts? Do NOT create merge commits (commits with multiple parents) — `push_to_pull_request_branch` uses `git format-patch` which breaks on merge commits. This means: no `git merge`, no `git rebase`, no `git commit-tree` with multiple `-p` flags. Instead: +1. Use `git diff HEAD...origin/` (base branch from `/tmp/pr-context/pr.json` field `baseRefName`) to see what the base branch changed in the conflicting files +2. Edit the files directly to incorporate the changes from the base branch +3. Commit the changes as regular (single-parent) commits +4. Once you are done with all of your changes on this branch, call `ready_to_push_to_pr` and then `push_to_pull_request_branch` to push diff --git a/tests/test_safe_input_ready_to_make_pr.py b/tests/test_safe_input_ready_to_make_pr.py index ee275ef5..243f8fe3 100644 --- a/tests/test_safe_input_ready_to_make_pr.py +++ b/tests/test_safe_input_ready_to_make_pr.py @@ -32,7 +32,10 @@ def extract_py_block(fragment_path: Path) -> str: parts = text.split("---", 2) assert len(parts) >= 3, f"Expected YAML frontmatter in {fragment_path}" frontmatter = yaml.safe_load(parts[1]) - py_code = frontmatter["safe-inputs"]["ready-to-make-pr"]["py"] + safe_inputs = frontmatter["safe-inputs"] + # The safe-input key varies: ready-to-push-to-pr (push) vs ready-to-make-pr (create) + first_key = next(iter(safe_inputs)) + py_code = safe_inputs[first_key]["py"] assert py_code, f"No py: block found in {fragment_path}" return py_code @@ -117,11 +120,14 @@ def test_create_extract(self): assert "import" in code assert "json.dumps" in code - def test_fragments_have_identical_py(self): - """The two fragments should have identical Python logic.""" + def test_fragments_share_common_structure(self): + """Both fragments should share the same diff/checklist structure.""" push_code = extract_py_block(PUSH_FRAGMENT) create_code = extract_py_block(CREATE_FRAGMENT) - assert push_code == create_code + # Both should contain the core logic markers + for marker in ["contributing = find(", "diff_line_count", "json.dumps", "self-review"]: + assert marker in push_code, f"Push fragment missing '{marker}'" + assert marker in create_code, f"Create fragment missing '{marker}'" # --------------------------------------------------------------------------- @@ -368,3 +374,164 @@ def test_diff_line_count_in_checklist(self, py_code, tmp_path): assert count > 0 # The line count should appear in the checklist text assert f"({count} lines)" in " ".join(output["checklist"]) + + +# --------------------------------------------------------------------------- +# Push fragment: history rewrite and merge commit guards +# --------------------------------------------------------------------------- + + +def _get_head_sha(repo: Path) -> str: + """Get the current HEAD commit SHA.""" + result = subprocess.run( + ["git", "rev-parse", "HEAD"], + cwd=str(repo), + capture_output=True, + text=True, + check=True, + ) + return result.stdout.strip() + + +def _write_pr_json(head_sha: str) -> None: + """Write a minimal pr.json with the given headRefOid.""" + os.makedirs("/tmp/pr-context", exist_ok=True) + with open("/tmp/pr-context/pr.json", "w") as f: + json.dump({"headRefOid": head_sha}, f) + + +def _cleanup_pr_json() -> None: + """Remove pr.json so it doesn't leak between tests.""" + try: + os.remove("/tmp/pr-context/pr.json") + except FileNotFoundError: + pass + + +class TestPushGuards: + """Test the ancestry and merge-commit guards in the push fragment.""" + + @pytest.fixture + def py_code(self): + return extract_py_block(PUSH_FRAGMENT) + + @pytest.fixture(autouse=True) + def cleanup(self): + yield + _cleanup_pr_json() + + def test_no_pr_json_passes(self, py_code, tmp_path): + """Without pr.json the guard is skipped — should succeed.""" + _cleanup_pr_json() + repo = make_git_repo(tmp_path, with_upstream=True) + output = run_py_in_repo(py_code, str(repo)) + assert output["status"] == "ok" + + def test_normal_commit_passes(self, py_code, tmp_path): + """A regular commit on top of the PR head should pass.""" + repo = make_git_repo(tmp_path, with_upstream=True) + head_sha = _get_head_sha(repo) + _write_pr_json(head_sha) + + # Add a normal commit + (repo / "new.txt").write_text("new\n") + subprocess.run(["git", "add", "new.txt"], cwd=str(repo), check=True, capture_output=True) + subprocess.run(["git", "commit", "-m", "add new"], cwd=str(repo), check=True, capture_output=True) + + output = run_py_in_repo(py_code, str(repo)) + assert output["status"] == "ok" + + def test_history_rewrite_detected(self, py_code, tmp_path): + """If HEAD diverges from PR head (rebase), guard should error.""" + repo = make_git_repo(tmp_path, with_upstream=True) + + # Record the initial head + initial_sha = _get_head_sha(repo) + + # Make a second commit, then record that as PR head + (repo / "a.txt").write_text("a\n") + subprocess.run(["git", "add", "a.txt"], cwd=str(repo), check=True, capture_output=True) + subprocess.run(["git", "commit", "-m", "second"], cwd=str(repo), check=True, capture_output=True) + pr_head = _get_head_sha(repo) + + # Now reset back to initial — simulates a rebase that dropped the PR head + subprocess.run(["git", "reset", "--hard", initial_sha], cwd=str(repo), check=True, capture_output=True) + (repo / "b.txt").write_text("b\n") + subprocess.run(["git", "add", "b.txt"], cwd=str(repo), check=True, capture_output=True) + subprocess.run(["git", "commit", "-m", "diverged"], cwd=str(repo), check=True, capture_output=True) + + _write_pr_json(pr_head) + output = run_py_in_repo(py_code, str(repo)) + assert output["status"] == "error" + assert "History rewrite" in output["error"] + + def test_merge_commit_detected(self, py_code, tmp_path): + """A merge commit after the PR head should be detected.""" + repo = make_git_repo(tmp_path, with_upstream=True) + pr_head = _get_head_sha(repo) + _write_pr_json(pr_head) + + # Create a side branch and merge it — produces a merge commit + subprocess.run(["git", "checkout", "-b", "side"], cwd=str(repo), check=True, capture_output=True) + (repo / "side.txt").write_text("side\n") + subprocess.run(["git", "add", "side.txt"], cwd=str(repo), check=True, capture_output=True) + subprocess.run(["git", "commit", "-m", "side"], cwd=str(repo), check=True, capture_output=True) + subprocess.run(["git", "checkout", "main"], cwd=str(repo), check=True, capture_output=True) + (repo / "main2.txt").write_text("main2\n") + subprocess.run(["git", "add", "main2.txt"], cwd=str(repo), check=True, capture_output=True) + subprocess.run(["git", "commit", "-m", "main2"], cwd=str(repo), check=True, capture_output=True) + subprocess.run(["git", "merge", "side", "--no-edit"], cwd=str(repo), check=True, capture_output=True) + + output = run_py_in_repo(py_code, str(repo)) + assert output["status"] == "error" + assert "Merge commit" in output["error"] + + +# --------------------------------------------------------------------------- +# Create fragment: merge commit guard +# --------------------------------------------------------------------------- + + +class TestCreateGuards: + """Test the merge-commit guard in the create fragment.""" + + @pytest.fixture + def py_code(self): + return extract_py_block(CREATE_FRAGMENT) + + def test_normal_commit_passes(self, py_code, tmp_path): + """Regular commits should pass the create guard.""" + repo = make_git_repo(tmp_path, with_upstream=True) + (repo / "new.txt").write_text("new\n") + subprocess.run(["git", "add", "new.txt"], cwd=str(repo), check=True, capture_output=True) + subprocess.run(["git", "commit", "-m", "add new"], cwd=str(repo), check=True, capture_output=True) + + output = run_py_in_repo(py_code, str(repo)) + assert output["status"] == "ok" + + def test_merge_commit_detected(self, py_code, tmp_path): + """A merge commit should be detected by the create guard.""" + repo = make_git_repo(tmp_path, with_upstream=True) + + # Create a side branch and merge it + subprocess.run(["git", "checkout", "-b", "side"], cwd=str(repo), check=True, capture_output=True) + (repo / "side.txt").write_text("side\n") + subprocess.run(["git", "add", "side.txt"], cwd=str(repo), check=True, capture_output=True) + subprocess.run(["git", "commit", "-m", "side"], cwd=str(repo), check=True, capture_output=True) + subprocess.run(["git", "checkout", "main"], cwd=str(repo), check=True, capture_output=True) + (repo / "main2.txt").write_text("main2\n") + subprocess.run(["git", "add", "main2.txt"], cwd=str(repo), check=True, capture_output=True) + subprocess.run(["git", "commit", "-m", "main2"], cwd=str(repo), check=True, capture_output=True) + subprocess.run(["git", "merge", "side", "--no-edit"], cwd=str(repo), check=True, capture_output=True) + + output = run_py_in_repo(py_code, str(repo)) + assert output["status"] == "error" + assert "Merge commit" in output["error"] + + def test_no_upstream_fails_closed(self, py_code, tmp_path): + """Without an upstream ref, the create guard should fail closed.""" + repo = make_git_repo(tmp_path, with_upstream=False) + + output = run_py_in_repo(py_code, str(repo)) + assert output["status"] == "error" + assert "upstream" in output["error"].lower()