Conversation
tninja
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4adbf1946
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "main") | ||
| ((or (magit-branch-p "master") (magit-branch-p "origin/master")) | ||
| "master") | ||
| (t "main")))) |
There was a problem hiding this comment.
When ai-code--default-pr-target-branch cannot infer an upstream and neither main nor master exists, it still returns "main". In repositories whose integration branch is named differently (for example develop or trunk), accepting this default causes the generated PR-creation flow to target a non-existent branch, which can make the downstream gh pr create step fail. The fallback should be derived from an actual existing default branch (e.g., remote HEAD/default branch) instead of an unconditional literal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed locally. The fallback logic now checks origin/HEAD first and uses the repository's actual remote default branch when upstream is unavailable, instead of unconditionally falling back to "main". This will be included in the next PR update.
There was a problem hiding this comment.
Pull request overview
Adds a “current branch PR creation” mode to the existing PR/issue review flow in ai-code-git.el, enabling users to generate a backend-specific prompt to create a PR from the checked-out branch (with a default target branch), and extends the test suite accordingly.
Changes:
- Add a new PR mode choice: “Send out PR for current branch”, including branch detection and default target-branch selection.
- Introduce a dedicated init-prompt builder for the current-branch PR creation flow.
- Update/extend ERT coverage for the new mode and prompt output; minor test cleanups.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
ai-code-git.el |
Adds the new current-branch PR flow, default target branch logic, and a PR-creation init prompt; includes some docstring touch-ups. |
test/test_ai-code-git.el |
Adds ERT tests for the new mode choice and PR-creation init prompt; minor whitespace/arg naming cleanups. |
| @@ -168,19 +179,31 @@ CI Checks Review Steps: | |||
| (defun ai-code--pull-or-review-pr-with-source (review-source) | |||
| "Ask for a target URL and send a prompt for REVIEW-SOURCE to AI." | |||
There was a problem hiding this comment.
ai-code--pull-or-review-pr-with-source no longer always asks for a URL (it can branch into current-branch PR creation), but the docstring still says it "Ask[s] for a target URL". Update the docstring to reflect both flows so it stays accurate for callers/readers.
| "Ask for a target URL and send a prompt for REVIEW-SOURCE to AI." | |
| "Prompt for an analysis mode and send a prompt for REVIEW-SOURCE to AI. | |
| If the selected mode is `send-current-branch-pr', ask for the target | |
| branch for the current branch PR. Otherwise, ask for the relevant pull | |
| request or issue URL." |
There was a problem hiding this comment.
Addressed locally. I updated the docstring to describe both flows: the current-branch PR creation path and the URL-based review/issue path. This will be included in the next PR update.
| (let* ((current-branch (ai-code--require-current-branch)) | ||
| (default-target-branch | ||
| (ai-code--default-pr-target-branch current-branch)) | ||
| (target-branch | ||
| (ai-code-read-string "Target branch to merge into: " | ||
| default-target-branch))) | ||
| (ai-code--build-send-current-branch-pr-init-prompt | ||
| review-source current-branch target-branch)) |
There was a problem hiding this comment.
The new current-branch PR flow can be invoked even when not in a Git repo. In that case, calling Magit/Git helpers to determine the current branch/target branch is likely to error or produce a confusing failure. Consider explicitly validating repository context (and emitting a clearer user-error) before entering this branch-based flow.
| (let* ((current-branch (ai-code--require-current-branch)) | |
| (default-target-branch | |
| (ai-code--default-pr-target-branch current-branch)) | |
| (target-branch | |
| (ai-code-read-string "Target branch to merge into: " | |
| default-target-branch))) | |
| (ai-code--build-send-current-branch-pr-init-prompt | |
| review-source current-branch target-branch)) | |
| (progn | |
| (unless (magit-toplevel) | |
| (user-error "Not inside a Git repository")) | |
| (let* ((current-branch (ai-code--require-current-branch)) | |
| (default-target-branch | |
| (ai-code--default-pr-target-branch current-branch)) | |
| (target-branch | |
| (ai-code-read-string "Target branch to merge into: " | |
| default-target-branch))) | |
| (ai-code--build-send-current-branch-pr-init-prompt | |
| review-source current-branch target-branch))) |
There was a problem hiding this comment.
Addressed locally. The current-branch PR path now validates that the command is running inside a Git repository before it tries to resolve branch information, so the failure mode is explicit and user-facing. This will be included in the next PR update.
| (defun ai-code--build-send-current-branch-pr-init-prompt (review-source current-branch target-branch) | ||
| "Build a PR creation prompt for REVIEW-SOURCE, CURRENT-BRANCH, and TARGET-BRANCH." | ||
| (let ((source-instruction | ||
| (ai-code--pull-or-review-source-instruction review-source))) |
There was a problem hiding this comment.
The PR creation prompt reuses ai-code--pull-or-review-source-instruction’s default (review) instruction, which currently tells the backend to "fetch pull request details and review comments". For a PR-creation flow, this instruction is misleading (especially before the PR exists). Add a dedicated review-mode case (e.g., send-current-branch-pr) with backend-specific instructions for creating the PR (gh: gh pr create, MCP: create PR via API), and pass that mode here.
| (defun ai-code--build-send-current-branch-pr-init-prompt (review-source current-branch target-branch) | |
| "Build a PR creation prompt for REVIEW-SOURCE, CURRENT-BRANCH, and TARGET-BRANCH." | |
| (let ((source-instruction | |
| (ai-code--pull-or-review-source-instruction review-source))) | |
| (defun ai-code--send-current-branch-pr-source-instruction (review-source) | |
| "Return PR-creation instructions for REVIEW-SOURCE." | |
| (pcase review-source | |
| ((or 'gh 'github-cli 'github) | |
| (concat | |
| "Use GitHub CLI to create the pull request. " | |
| "Run `gh pr create` with the current branch as the head branch, " | |
| "target the requested base branch, and include the final title and body.")) | |
| ((or 'mcp 'github-mcp) | |
| (concat | |
| "Use the available MCP/API GitHub tools to create the pull request directly. " | |
| "Do not fetch review comments for a PR that does not exist yet; " | |
| "create the PR first, then return the resulting PR URL.")) | |
| (_ | |
| (concat | |
| "Create the pull request using the backend's PR creation capability. " | |
| "Do not treat this as a PR review flow and do not fetch review comments " | |
| "before the PR exists.")))) | |
| (defun ai-code--build-send-current-branch-pr-init-prompt (review-source current-branch target-branch) | |
| "Build a PR creation prompt for REVIEW-SOURCE, CURRENT-BRANCH, and TARGET-BRANCH." | |
| (let ((source-instruction | |
| (ai-code--send-current-branch-pr-source-instruction review-source))) |
There was a problem hiding this comment.
Addressed locally. I added a dedicated source-instruction path for current-branch PR creation so this flow no longer reuses review-oriented instructions about fetching PR details or review comments before the PR exists. This will be included in the next PR update.
|
|
||
| (defun ai-code--pull-or-review-pr-mode-choice () | ||
| "Prompt user to choose analysis mode for a pull request or issue." | ||
| ;; DONE: add a choice: send out PR for current branch. The feature will ask user the target branch to merge. By default, it should be parent branch of current branch. AI should send out PR with description. The description should looks like it's written by the author, and it should be short. |
There was a problem hiding this comment.
There’s a leftover implementation note (;; DONE: ...) inside ai-code--pull-or-review-pr-mode-choice. This kind of comment becomes stale quickly and adds noise to the interactive path; please remove it (or convert it into a proper issue/commit message) before merging.
| ;; DONE: add a choice: send out PR for current branch. The feature will ask user the target branch to merge. By default, it should be parent branch of current branch. AI should send out PR with description. The description should looks like it's written by the author, and it should be short. |
There was a problem hiding this comment.
Keeping this comment intentionally. The original implementation requirement for this task explicitly asked to preserve the existing comment in place and change it to a DONE-prefixed form after implementing the feature, so removing it here would conflict with that requirement.
| (ai-code--build-pr-init-prompt review-source target-url review-mode)))) | ||
| (prompt (ai-code-read-string "Enter review prompt: " init-prompt))) | ||
| (ai-code--insert-prompt prompt))) |
There was a problem hiding this comment.
When the user chooses "Send out PR for current branch", the minibuffer prompt still says "Enter review prompt". Consider changing this prompt (conditionally) to something neutral like "Enter prompt" or "Enter PR creation prompt" to avoid misleading wording in the new flow.
There was a problem hiding this comment.
Addressed locally. The current-branch PR path now uses a dedicated minibuffer label (Enter PR creation prompt:) instead of the review-specific prompt text. This will be included in the next PR update.
| (defun ai-code--default-pr-target-branch (current-branch) | ||
| "Return the default PR target branch for CURRENT-BRANCH." | ||
| (let* ((upstream-branch | ||
| (ignore-errors | ||
| (magit-git-string "rev-parse" | ||
| "--abbrev-ref" | ||
| "--symbolic-full-name" | ||
| "@{upstream}"))) | ||
| (normalized-upstream | ||
| (ai-code--normalize-branch-name upstream-branch))) | ||
| (cond | ||
| ((and normalized-upstream | ||
| (not (string-empty-p normalized-upstream)) | ||
| (not (string= normalized-upstream current-branch))) | ||
| normalized-upstream) | ||
| ((or (magit-branch-p "main") (magit-branch-p "origin/main")) | ||
| "main") | ||
| ((or (magit-branch-p "master") (magit-branch-p "origin/master")) | ||
| "master") | ||
| (t "main")))) |
There was a problem hiding this comment.
ai-code--default-pr-target-branch is new and contains non-trivial branching (upstream parsing, normalization, main/master fallback), but there are no tests covering these behaviors. Add focused ERT tests that mock magit-git-string/magit-branch-p to validate upstream-derived defaults and the fallback selection logic.
There was a problem hiding this comment.
Addressed locally. I added focused coverage for the new default-target-branch behavior, including the remote default branch fallback path and the current-branch PR prompt flow. This will be included in the next PR update.
Add a current-branch PR flow to the Git review menu so I can send a PR without switching to a separate GitHub workflow.
This adds a new menu option for creating a PR from the current branch, asks for the target branch with a sensible default based on the branch upstream, and generates a short author-style PR prompt for the selected backend. I also updated the related tests and cleaned up the touched file enough to keep compilation and doc checks happy.
Verification: not run in this pass.