feat: add CODE_AUTO_MERGE env var to post-code script - #491
Conversation
|
🤖 Review · |
4c88885 to
2e267a9
Compare
|
🤖 Finished Review · ✅ Success · Started 10:42 AM UTC · Completed 10:58 AM UTC |
Qodo is busy workingCheck back in a few minutes. Qodo's code review agents are on it. |
PR Summary by QodoAdd CODE_AUTO_MERGE env var to enable GitHub auto-merge in post-code
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
ReviewFindingsHigh
Medium
Low
Next steps:
Previous runReviewFindingsHigh
Medium
Low
Next steps:
Previous run (2)ReviewFindingsHigh
Low
Next steps:
Previous run (3)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (4)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (5)ReviewFindingsHigh
Medium
Low
Labels: PR modifies code agent harness and post-code scripts, adding a new feature Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 11:01 AM UTC · Completed 11:18 AM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| 1. **Pre-script** validates inputs on the runner before sandbox creation. It also checks for open PRs linked to the issue. | ||
| 2. **Sandbox** — the agent reads the issue, explores the codebase, writes code, runs tests and linters, and commits locally. It has no network access (enforced by OpenShell). | ||
| 3. **Post-script** runs on the runner: it performs protected path checks, secret scanning, pre-commit checks, pushes the branch, creates the PR, and best-effort assigns the PR to a human owner (latest `/fs-code` invoker, else issue assignee, else issue author). | ||
| 3. **Post-script** runs on the runner: it performs protected path checks, secret scanning, pre-commit checks, pushes the branch, creates the PR, optionally enables auto-merge, and best-effort assigns the PR to a human owner (latest `/fs-code` invoker, else issue assignee, else issue author). |
There was a problem hiding this comment.
I don't think this is required, otherwise this will grow into a big list of things that it does, probably not even comprehensive right now. If you want to add something, add "and other operations" at the end.
There was a problem hiding this comment.
Addressed in f3ba9b9 — removed the auto-merge mention from the post-script description.
There was a problem hiding this comment.
It looks like this snuck back in in a subsequent commit.
0dbf6ce to
bb597d4
Compare
|
🤖 Review · |
|
🤖 Finished Review · ✅ Success · Started 10:07 AM UTC · Completed 10:25 AM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
f3ba9b9 to
4f60e98
Compare
|
🤖 Finished Review · ✅ Success · Started 10:48 AM UTC · Completed 11:04 AM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass focused on the CODE_AUTO_MERGE safety semantics (branch-protection dependency, merge-method defaults, and the merge-queue claim). Four findings are attached as inline comments. One additional finding has no single line to anchor to and is included below.
[MEDIUM] PR description Summary contradicts the shipped implementation
The PR description's Summary section still reads: "enables GitHub auto-merge ... using the repo's configured merge strategy (no merge method flag — respects branch protection settings)." This is no longer accurate: both auto-merge call sites (post-code.src.sh:535-539 and 715-719) always pass an explicit --squash/--rebase/--merge flag via CODE_AUTO_MERGE_METHOD, added in a later commit specifically because the "no flag" approach didn't work. The Summary was never updated to match, so a reviewer relying on the description alone would be misled about how merge-strategy selection works, including the safety property ("respects branch protection settings") disputed in the inline comments on this review.
Suggested fix: Update the PR Summary to describe the CODE_AUTO_MERGE_METHOD-based behavior, its default ("merge"), and the merge-queue caveat, so the description matches the diff being merged.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Follow-up review pass on the auto-merge feature — three new findings attached as inline comments (env-validation break, silent method fallback, unsanitized log output), plus one below that has no diff line to anchor to. Separately, the existing comment at post-code.src.sh:713 has been escalated to CRITICAL with verification details from the gh CLI source.
[MEDIUM] Docs regression: "How the agent works" lost the auto-merge mention
Commit 2d86dbf added "...pushes the branch, creates the PR, optionally enables auto-merge, and best-effort assigns the PR..." to the post-script bullet in docs/code.md (line 53). Commit 4f60e98 reverted that phrase — apparently an accidental hunk resolution while updating the doc for CODE_AUTO_MERGE_METHOD — so the current head's prose no longer mentions auto-merge at all, while the Variables table two paragraphs above documents both variables and the script unconditionally performs the step when enabled.
Suggested fix: Restore "...creates the PR, optionally enables auto-merge, and best-effort assigns the PR..." in the post-script bullet of docs/code.md. (Posted in the review body because line 53 is not part of this PR's diff.)
|
Addressed in 2f93e79 — updated the PR description to reflect the current implementation: auto-detection of merge methods, merge-state guard, merge queue support, and safety guards. |
|
Addressed in 2f93e79 — restored "optionally enables auto-merge" in the post-script description at docs/code.md line 54. |
|
All findings from this review addressed in 2f93e79:
|
|
🤖 Review · |
Signed-off-by: Marta Anon <manon@redhat.com>
When CODE_AUTO_MERGE=true, enable GitHub auto-merge on newly created PRs using the repo's configured merge strategy. Best-effort — failure to enable auto-merge is a warning, not a blocking error. The env var is passed through forge.github.runner_env in the code agent harness. The fullsend CLI will populate it from config.Defaults.AutoMerge in a follow-up change. Signed-off-by: Marta Anon <manon@redhat.com>
Signed-off-by: Marta Anon <manon@redhat.com>
Signed-off-by: Marta Anon <manon@fullsend.sh> Signed-off-by: Marta Anon <manon@redhat.com>
gh pr merge --auto defaults to merge commit, which silently fails for repos that only allow squash or rebase. Accept CODE_AUTO_MERGE_METHOD (squash|rebase|merge, default: merge) and pass the corresponding flag. Also fixes inaccurate docs claiming "uses the repo's configured merge strategy" and adds merge queue incompatibility note. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Marta Anon <manon@redhat.com>
- Add merge-state guard: only arm auto-merge when PR is BLOCKED (requires branch protection). Prevents immediate merge on unprotected repos where gh silently downgrades --auto to a direct merge. - Auto-detect merge method from repo's allowed methods (prefer squash) when CODE_AUTO_MERGE_METHOD is unset. - Detect merge queues and omit method flag so gh can negotiate with the queue. - Skip re-arming on existing PRs where auto-merge is already set. - Warn on unknown CODE_AUTO_MERGE_METHOD values instead of silent coercion. - Capture and sanitize gh pr merge output via print_sanitized_gha_log. - Remove CODE_AUTO_MERGE/CODE_AUTO_MERGE_METHOD from forge.github.runner_env to avoid harness env validation failures on deployments that don't define these optional vars. - Restore auto-merge mention in docs post-script description. - Extract enable_auto_merge helper to deduplicate existing-PR and new-PR paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Marta Anon <manon@redhat.com>
2f93e79 to
9e9a752
Compare
|
🤖 Review · |
|
🤖 Finished Review · ✅ Success · Started 10:43 AM UTC · Completed 10:57 AM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The bundled script contains PR_NUMBER from the failure-report library.
Adding a local pr_number in enable_auto_merge triggered shellcheck
SC2153 ("Possible misspelling"). Follow the existing convention of
using target_pr for function parameters.
Signed-off-by: Marta Anon <manon@fullsend.sh>
Signed-off-by: Marta Anon <manon@redhat.com>
94ddb9b to
ae8c86b
Compare
|
🤖 Finished Review · ✅ Success · Started 11:17 AM UTC · Completed 11:33 AM UTC |
| fi | ||
|
|
||
| local merge_state | ||
| merge_state="$(echo "${pr_json}" | jq -r '.mergeStateStatus // "UNKNOWN"')" |
There was a problem hiding this comment.
[HIGH] enable_auto_merge's UNKNOWN-state guard races GitHub's async mergeability computation with no retry, likely no-opping the primary use case
The BLOCKED-only guard added to fix the earlier CRITICAL bypass bug (commit 2f93e79) queries gh pr view --json mergeStateStatus exactly once, immediately after gh pr create (new-PR path) or immediately after a push (existing-PR path), with zero delay/retry/backoff. GitHub computes mergeStateStatus/mergeable asynchronously in the background after a PR is created or its head ref changes, and commonly returns UNKNOWN for a window immediately afterward. The UNKNOWN branch here treats this identically to a hard skip: it logs a warning and returns 0 permanently for that invocation — there is no re-check. This means the single highest-value scenario this PR advertises (arming auto-merge on a freshly created PR against a branch-protected repo) is likely to silently no-op on the only invocation that occurs. This is a new, previously-unraised consequence of the BLOCKED-guard fix itself (distinct from the original CRITICAL bug, which was about incorrectly merging on CLEAN/HAS_HOOKS/UNSTABLE states and has been fixed). It also remains unverified: the PR's test plan still shows both manual end-to-end verification checkboxes unchecked ("confirm gh pr merge --auto is called" and "confirm auto-merge is skipped ... without branch protection"), and the unit tests in scripts/post-code-test.sh only exercise hand-reimplemented copies of the decision logic, never the real gh-pr-view-driven state machine, so this UNKNOWN race path has zero test coverage.
Suggestion: add a short bounded retry/poll (e.g. 2-3 attempts with a brief sleep) around the mergeStateStatus check before treating UNKNOWN as a skip, and complete the two unchecked manual-verification checkboxes against a real branch-protected repo before merging this feature as final.
4 participants
Summary
CODE_AUTO_MERGEandCODE_AUTO_MERGE_METHODenvironment variables to the code agent post-code scriptCODE_AUTO_MERGE=true, enables GitHub auto-merge on newly created and existing PRsCODE_AUTO_MERGE_METHOD(squash/rebase/merge), or auto-detected from the repo's allowed merge methods when unset (prefers squash)ghcan negotiate with the queueCODE_AUTO_MERGE_METHODvalues instead of silent coercionrunner_env) to avoid harness validation failures on deployments that don't define these optional varsThe fullsend CLI will populate this from
config.Defaults.AutoMergein a follow-up change. Until then, orgs can set the env vars manually in their GHA workflow files.Test plan
decide_auto_mergetest cases (true, empty, false, TRUE, 1)resolve_merge_method_flagtest cases (squash, merge, rebase, default, unknown with warning)CODE_AUTO_MERGEmake test— 93 post-code tests + all others)CODE_AUTO_MERGE=trueon a repo with branch protection and confirmgh pr merge --autois called🤖 Generated with Claude Code