Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
❌ Author of the following commits did not sign a Contributor Agreement: Please, read and sign the above mentioned agreement if you want to contribute to this project |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds history-safety guards and renames PR readiness flow across multiple workflow fragments and safe-inputs scripts. When /tmp/pr-context/pr.json exists the workflows now: (1) verify the PR head SHA is an ancestor of HEAD (history-rewrite check) and (2) detect merge commits (using merge-base/upstream fork-point + git rev-list --min-parents=2). If either check fails the flow emits a JSON-formatted error and exits before diff generation, self-review or push-to-PR steps. Also updates PR-context JSON to include headRefOid and adjusts tooling names from ready-to-make-pr to ready-to-push-to-pr; tests were added for these guards. Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/ai is there something we could do like forbid merge commits on the git cli? Are our instructions clear enough on not doing bad things? |
|
Yes—this is already mostly clear, and the new ancestry guard is a strong safety net. What is this? | From workflow: Mention in PR Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
|
/ai can you look at the run logs and see what actually happened? You should be able to get the prompt too, and figure out what went wrong |
|
The prior run ( Evidence from that run:
So what went wrong is not tool execution failure; it was that the previous run answered the previous What is this? | From workflow: Mention in PR Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
|
/ai please investigate why Activation prompt context had comment-id: 3976398896 and Request: "". That doesn't make sense |
|
@copilot move the workflow into the right location in the repo |
|
@strawgate I've opened a new pull request, #469, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: strawgate <6384545+strawgate@users.noreply.github.com>
|
/ai maybe we just run the command when the agent calls ready to make pr command and return an error with instructions? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Move the ancestry and merge commit checks from prompt-based instructions into the ready_to_make_pr Python script so they are enforced automatically. When the agent calls ready_to_make_pr, the script now: - push-to-pr: checks that the original PR head SHA (from pr.json) is an ancestor of HEAD (detects rebase/reset) and that no merge commits exist since the PR head - create-pr: checks that no merge commits exist since the upstream fork point Both return a status:error JSON with actionable fix instructions. Also adds headRefOid to the pr-context gh pr view fields so the SHA is available in pr.json. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…y-rewrite-guard-b9148a384abc45fa # Conflicts: # .github/workflows/gh-aw-mention-in-pr.lock.yml
- Remove `---` from Python comments in safe-input fragments that broke the test's YAML frontmatter extraction (split on `---`) - Update test_fragments_have_identical_py to test_fragments_share_common_logic since push and create fragments now have different guard sections - Add TestPushGuards: ancestry rewrite detection, merge commit detection, no-pr-json passthrough, normal commit passthrough - Add TestCreateGuards: merge commit detection, normal commit passthrough - Recompile lock files after merging origin/main Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Create fragment: fail closed when upstream fork point cannot be
resolved instead of silently skipping the merge-commit guard
- Push fragment: replace `git checkout {sha}` (detaches HEAD) with
`git reset --hard {sha}` in recovery guidance to keep branch checked out
- Add test for create fragment's no-upstream fail-closed behavior
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix comment describing diff fallback chain: `git diff @{upstream}`
includes working tree changes, not "committed only"
- Replace vague `git merge-tree or git diff` with specific
`git diff HEAD...origin/<base-branch>` for merge conflict resolution
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| def py_code(self): | ||
| return extract_py_block(PUSH_FRAGMENT) | ||
|
|
||
| @pytest.fixture(autouse=True) |
There was a problem hiding this comment.
[MEDIUM] Cleanup scope allows /tmp/pr-context/pr.json to leak into unrelated tests
The autouse cleanup fixture is scoped to TestPushGuards, but many earlier tests in this file also execute the push-fragment code path. Since the new guard reads global /tmp/pr-context/pr.json when present, stale context can flip otherwise-ok tests into error (e.g., with {"headRefOid":"deadbeef"} set).
Please make cleanup module-wide (or clear /tmp/pr-context/pr.json before every push-fragment test) so all tests that call extract_py_block(PUSH_FRAGMENT) are hermetic and not coupled to ambient runner state.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/gh-aw-bug-exterminator.lock.yml (1)
959-976:⚠️ Potential issue | 🟠 MajorUse the resolved
upstream_shavariable in diff/stat generation instead of re-attempting@{upstream}resolution.Line 948 uses
upstream_shasuccessfully for merge-commit validation, but lines 963 and 973 still reference@{upstream}for diff and stat generation. If the tracking branch is not configured (butupstream_shawas resolved viaorigin/HEADororigin/main), the diff/stat will fail on@{upstream}and fall back toHEAD, missing committed changes.Suggested patch
for diff_cmd in [ - ['git', 'diff', '--merge-base', '@{upstream}'], - ['git', 'diff', '@{upstream}'], + ['git', 'diff', upstream_sha], ['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', upstream_sha], ['git', 'diff', '--stat', 'HEAD'], ]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/gh-aw-bug-exterminator.lock.yml around lines 959 - 976, The diff/stat generation still uses the literal '@{upstream}' instead of the resolved upstream_sha; update the command lists used to build diff_cmd and stat_cmd so any occurrence of '@{upstream}' is replaced with the upstream_sha variable (e.g., ['git','diff','--merge-base', upstream_sha], ['git','diff', upstream_sha], and same for the --stat variants), preserving the existing fallback order and using the upstream_sha variable (not a quoted string) when constructing the commands passed to run().
♻️ Duplicate comments (1)
tests/test_safe_input_ready_to_make_pr.py (1)
418-422:⚠️ Potential issue | 🟠 MajorTest cleanup is scoped too narrowly and can make unrelated tests flaky.
Line 418-Line 422 only cleans
/tmp/pr-context/pr.jsonforTestPushGuards, but earlier tests also execute the push fragment and can pick up stale PR context from the environment. Move this cleanup to a module-level autouse fixture so every push-fragment test is hermetic.Suggested fix
+@pytest.fixture(autouse=True) +def cleanup_pr_json(): + yield + _cleanup_pr_json() + class TestPushGuards: """Test the ancestry and merge-commit guards in the push fragment.""" @@ - `@pytest.fixture`(autouse=True) - def cleanup(self): - yield - _cleanup_pr_json()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_safe_input_ready_to_make_pr.py` around lines 418 - 422, The current autouse fixture named cleanup (def cleanup) that yields then calls _cleanup_pr_json() is defined inside TestPushGuards and only runs for that class; move that fixture to module scope so it runs for every test in the file: create a top-level `@pytest.fixture`(autouse=True) def cleanup(): yield; _cleanup_pr_json() at the module level (outside any Test... class) so all push-fragment tests get a fresh /tmp/pr-context/pr.json and avoid cross-test flakiness.
🧹 Nitpick comments (1)
.github/workflows/gh-aw-issue-fixer.lock.yml (1)
989-995: UseDEFAULT_BRANCHin upstream fallback to avoid false fail-closed on non-mainrepos.Line 989 hardcodes
origin/mainas the final fallback. Iforigin/HEADis unavailable and the repo default branch is notmain, this returns an avoidable error.Proposed fix
- upstream_sha = '' - for ref in ['@{upstream}', 'origin/HEAD', 'origin/main']: + upstream_sha = '' + default_branch = os.getenv('DEFAULT_BRANCH', '').strip() + refs = ['@{upstream}', 'origin/HEAD'] + if default_branch: + refs.append(f'origin/{default_branch}') + refs.append('origin/main') + for ref in refs: r = run(['git', 'merge-base', 'HEAD', ref]) if r.returncode == 0 and r.stdout.strip(): upstream_sha = r.stdout.strip() break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/gh-aw-issue-fixer.lock.yml around lines 989 - 995, The fallback list currently hardcodes 'origin/main' causing failures on repos whose default branch isn't main; update the refs iteration to use the repository default (DEFAULT_BRANCH) instead of 'origin/main' by constructing the final fallback as f'origin/{DEFAULT_BRANCH}' (ensure DEFAULT_BRANCH is read from the environment or a config earlier), leaving the existing loop and use of run([...]) and upstream_sha logic unchanged so the code still checks ['@{upstream}', 'origin/HEAD', f'origin/{DEFAULT_BRANCH}'] and prints the same error message if upstream_sha remains unset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/gh-aw-bug-exterminator.lock.yml:
- Around line 959-976: The diff/stat generation still uses the literal
'@{upstream}' instead of the resolved upstream_sha; update the command lists
used to build diff_cmd and stat_cmd so any occurrence of '@{upstream}' is
replaced with the upstream_sha variable (e.g., ['git','diff','--merge-base',
upstream_sha], ['git','diff', upstream_sha], and same for the --stat variants),
preserving the existing fallback order and using the upstream_sha variable (not
a quoted string) when constructing the commands passed to run().
---
Duplicate comments:
In `@tests/test_safe_input_ready_to_make_pr.py`:
- Around line 418-422: The current autouse fixture named cleanup (def cleanup)
that yields then calls _cleanup_pr_json() is defined inside TestPushGuards and
only runs for that class; move that fixture to module scope so it runs for every
test in the file: create a top-level `@pytest.fixture`(autouse=True) def
cleanup(): yield; _cleanup_pr_json() at the module level (outside any Test...
class) so all push-fragment tests get a fresh /tmp/pr-context/pr.json and avoid
cross-test flakiness.
---
Nitpick comments:
In @.github/workflows/gh-aw-issue-fixer.lock.yml:
- Around line 989-995: The fallback list currently hardcodes 'origin/main'
causing failures on repos whose default branch isn't main; update the refs
iteration to use the repository default (DEFAULT_BRANCH) instead of
'origin/main' by constructing the final fallback as f'origin/{DEFAULT_BRANCH}'
(ensure DEFAULT_BRANCH is read from the environment or a config earlier),
leaving the existing loop and use of run([...]) and upstream_sha logic unchanged
so the code still checks ['@{upstream}', 'origin/HEAD',
f'origin/{DEFAULT_BRANCH}'] and prints the same error message if upstream_sha
remains unset.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.github/workflows/downstream-users.lock.yml.github/workflows/gh-aw-bug-exterminator.lock.yml.github/workflows/gh-aw-code-duplication-fixer.lock.yml.github/workflows/gh-aw-code-simplifier.lock.yml.github/workflows/gh-aw-fragments/safe-output-create-pr.md.github/workflows/gh-aw-fragments/safe-output-push-to-pr.md.github/workflows/gh-aw-issue-fixer.lock.yml.github/workflows/gh-aw-mention-in-issue-no-sandbox.lock.yml.github/workflows/gh-aw-mention-in-issue.lock.yml.github/workflows/gh-aw-mention-in-pr-by-id.lock.yml.github/workflows/gh-aw-mention-in-pr-no-sandbox.lock.yml.github/workflows/gh-aw-mention-in-pr.lock.yml.github/workflows/gh-aw-newbie-contributor-fixer.lock.yml.github/workflows/gh-aw-pr-actions-fixer.lock.yml.github/workflows/gh-aw-pr-review-addresser.lock.yml.github/workflows/gh-aw-release-update.lock.yml.github/workflows/gh-aw-scheduled-fix.lock.yml.github/workflows/gh-aw-small-problem-fixer.lock.yml.github/workflows/gh-aw-test-improvement.lock.yml.github/workflows/gh-aw-test-improver.lock.yml.github/workflows/gh-aw-text-beautifier.lock.ymltests/test_safe_input_ready_to_make_pr.py
|
/ai I think maybe we should also block writes to the .github/workflows folder |
|
Good call — this is already blocked at push time.
What is this? | From workflow: Mention in PR Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
|
/ai right but let's flag it in the ready to push tool so we can tell the agent their push will not work |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
strawgate
deleted the
fix/mention-pr-history-rewrite-guard-b9148a384abc45fa
branch
Summary
Adds history-safety and workflow-path guards for patch-based PR updates so workflows fail fast on rewritten history, merge commits, or prohibited workflow-file edits.
What changed
github/workflows/gh-aw-fragments/safe-output-push-to-pr.md(source) and regenerated.github/workflows/gh-aw-fragments/safe-output-push-to-pr.mdthat:.github/workflows/paths (whichpush_to_pull_request_branchrejects)headRefOid) from/tmp/pr-context/pr.jsonHEAD(git merge-base --is-ancestor)git rev-list --min-parents=2)ready_to_make_prtoready_to_push_to_prand updated related guidance/call sites in regenerated lock workflows..github/workflows/gh-aw-fragments/pr-context.mdto fetch and documentheadRefOidinpr.json..github/workflows/gh-aw-fragments/safe-output-create-pr.mdthat:@{upstream},origin/HEAD, thenorigin/maincreate_pull_requestwhen merge commits exist in the commit rangegit diff HEAD...origin/(base-branch)withbaseRefNamefrom/tmp/pr-context/pr.json.tests/test_safe_input_ready_to_make_pr.pyfor history rewrite detection, merge commit detection, and no-upstream fail-closed behavior..lock.ymlworkflows so runtime behavior matches fragment updates.Why
Issue #463 exposed a failure mode where rewritten history and merge commits break the
git format-patchflow used by safe-output PR operations. These guards catch that state early and provide deterministic recovery steps.Validation
make lint(passes)tests/test_safe_input_ready_to_make_pr.pyFixes #463