Skip to content

Commit 71a09b1

Browse files
committed
fix(agent): deterministic PR base — retarget agent-chosen base to the orchestrator's
The PR is opened by the agent's own `gh pr create` (prompt workflow), so its `--base` is a model judgment call, not the orchestrator's. Live-caught on the aws-samples#247 chain stress test (2026-07-18): a stacked child that branched off its predecessor's branch opened its PR against `main` anyway ("this was based off the chain-Y branch, let me open the PR against main"), and a root whose detect_default_branch correctly returned `linear-vercel` was ALSO pointed at `main`. Wrong base ⇒ the PR shows the whole default-branch divergence (100s of files) instead of the child's real delta, and a stacked child merges onto the wrong branch. ensure_pr now reconciles an existing PR's base against setup.default_branch (the orchestrator base_branch for a stacked child / the detected repo default for a root) via `gh pr edit --base` — removing the agent's discretion without forbidding it from opening the PR (keeps the agent-authored title/body). Best-effort: a retarget failure WARNs, never fatal. Prompt hardened to tell the agent not to substitute main/master even when the branch was cut elsewhere. Tests: 4 new (retarget-on-mismatch, no-op-on-match, retarget-failure-warns, create-path-uses-right-base); existing already-open test asserts no spurious retarget. Deferred: the diamond base + the reconciler's hardcoded 'main' defaultBranch need a RepoConfig.default_branch field (server doesn't know the repo default); tracked separately.
1 parent c1ad5b0 commit 71a09b1

3 files changed

Lines changed: 204 additions & 6 deletions

File tree

agent/src/post_hooks.py

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -398,6 +398,79 @@ def _note_unpushed_commits(repo_dir: str, branch: str, config: TaskConfig) -> No
398398
log("WARN", f"Failed to post un-pushed-commits note: {type(e).__name__}: {e}")
399399

400400

401+
def _reconcile_pr_base(repo_dir: str, branch: str, config: TaskConfig, expected_base: str) -> None:
402+
"""Deterministically retarget an existing PR onto ``expected_base``.
403+
404+
The PR is created by the AGENT (its own ``gh pr create`` in the prompt
405+
workflow), so the ``--base`` it chose is a model judgment call, not the
406+
orchestrator's. Live-caught on the #247 chain stress test (2026-07-18):
407+
a stacked child that branched off its predecessor's branch STILL opened
408+
its PR against ``main`` — the agent reasoned "this was based off the
409+
chain-Y branch, let me open the PR against main" and even a root whose
410+
``detect_default_branch`` correctly returned ``linear-vercel`` was pointed
411+
at ``main``. Wrong base ⇒ the PR shows the whole default-branch divergence
412+
(100s of files) instead of the child's real delta, and a stacked child's
413+
PR merges onto the wrong branch.
414+
415+
``expected_base`` is ``setup.default_branch`` — which is the orchestrator's
416+
``base_branch`` for a stacked child (the predecessor's branch, or ``main``
417+
for a diamond) and ``detect_default_branch`` for a root. This post-hook
418+
reads the PR's current base and, if it disagrees, retargets it via
419+
``gh pr edit --base`` — removing the agent's discretion without forbidding
420+
it from opening the PR (which keeps the agent-authored title/body).
421+
422+
Best-effort: any failure is logged, never fatal — a mis-based PR is a
423+
presentation/merge-target defect, not a reason to fail the whole task.
424+
"""
425+
try:
426+
view = subprocess.run(
427+
[
428+
"gh",
429+
"pr",
430+
"view",
431+
branch,
432+
"--repo",
433+
config.repo_url,
434+
"--json",
435+
"baseRefName",
436+
"-q",
437+
".baseRefName",
438+
],
439+
cwd=repo_dir,
440+
capture_output=True,
441+
text=True,
442+
timeout=60,
443+
)
444+
except (OSError, subprocess.SubprocessError) as exc:
445+
log("WARN", f"Could not read PR base to reconcile ({type(exc).__name__}) — leaving as-is")
446+
return
447+
if view.returncode != 0 or not view.stdout.strip():
448+
# No PR / unreadable base — nothing to reconcile (creation path handles
449+
# the no-PR case; a transient read error just leaves the base as-is).
450+
return
451+
current_base = view.stdout.strip()
452+
if current_base == expected_base:
453+
return
454+
log(
455+
"POST",
456+
f"Retargeting PR base '{current_base}' → '{expected_base}' "
457+
f"(deterministic; agent chose the wrong base)",
458+
)
459+
result = run_cmd(
460+
["gh", "pr", "edit", branch, "--repo", config.repo_url, "--base", expected_base],
461+
label="reconcile-pr-base",
462+
cwd=repo_dir,
463+
check=False,
464+
)
465+
if result.returncode != 0:
466+
stderr_msg = result.stderr.strip()[:200] if result.stderr else "(no stderr)"
467+
log(
468+
"WARN",
469+
f"Failed to retarget PR base to '{expected_base}' (gh exit {result.returncode}): "
470+
f"{stderr_msg} — PR remains based on '{current_base}'",
471+
)
472+
473+
401474
def ensure_pr(
402475
config: TaskConfig,
403476
setup: RepoSetup,
@@ -491,6 +564,10 @@ def ensure_pr(
491564
if result.returncode == 0 and result.stdout.strip():
492565
pr_url = result.stdout.strip()
493566
log("POST", f"PR already exists: {pr_url}")
567+
# The agent opened this PR and picked its own --base; correct it to the
568+
# orchestrator-supplied / detected base if it disagrees (stacked-child
569+
# + root wrong-base fix, 2026-07-18).
570+
_reconcile_pr_base(repo_dir, branch, config, default_branch)
494571
return pr_url
495572

496573
# Check if there are any commits on this branch beyond the default branch

agent/src/prompts/new_task.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,12 @@
8585
- Any patterns or conventions you discovered about this repo
8686
- Suggestions for future tasks on this repo
8787
88-
Run:
88+
Run this EXACTLY — the `--base` value is chosen for you (for a stacked \
89+
sub-issue it is the predecessor's branch so the PR shows only your delta; for \
90+
a normal task it is the repo default). Do NOT substitute a different base such \
91+
as `main`/`master` even if this branch was cut from another branch — targeting \
92+
the wrong base makes the PR show the whole branch divergence instead of your \
93+
change:
8994
```
9095
gh pr create --repo {repo_url} --head {branch_name} --base {default_branch} --title "<type>(<module>): <description>" --body "<body>"
9196
```

agent/tests/test_post_hooks.py

Lines changed: 121 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,18 @@ def __call__(self, cmd, **kwargs):
4545
return _cp()
4646

4747

48-
def _pr_view(url: str) -> _SubprocessRunRecorder:
49-
"""Recorder whose ``gh pr view`` returns *url* (other calls rc=0, empty)."""
48+
def _pr_view(url: str, base: str = "main") -> _SubprocessRunRecorder:
49+
"""Recorder for the two ``gh pr view`` shapes ensure_pr issues.
50+
51+
The URL query (``--json url``) returns *url*; the base query
52+
(``--json baseRefName``, used by ``_reconcile_pr_base``) returns *base*.
53+
Defaulting *base* to ``main`` matches ``_setup``'s default_branch so the
54+
reconcile is a no-op unless a test opts into a mismatch.
55+
"""
5056

5157
def responder(cmd):
58+
if "view" in cmd and "baseRefName" in cmd:
59+
return _cp(returncode=0, stdout=base + "\n")
5260
if "view" in cmd:
5361
return _cp(returncode=0, stdout=url + "\n")
5462
return _cp()
@@ -182,17 +190,24 @@ def _ensure_pushed(d, b):
182190

183191
class TestEnsurePrCreate:
184192
def test_returns_existing_pr_when_already_open(self, monkeypatch):
185-
# First `gh pr view` returns a URL -> short-circuit, no creation.
186-
sub = _pr_view("https://github.com/o/r/pull/1")
193+
# First `gh pr view` returns a URL -> short-circuit, no creation. The
194+
# existing PR's base ("main") already matches default_branch, so the
195+
# base reconcile is a no-op (no `gh pr edit`).
196+
sub = _pr_view("https://github.com/o/r/pull/1", base="main")
187197
run_cmd = _RunCmdRecorder()
188198
monkeypatch.setattr(post_hooks.subprocess, "run", sub)
189199
monkeypatch.setattr(post_hooks, "run_cmd", run_cmd)
190200

191201
url = post_hooks.ensure_pr(
192-
_config(), _setup(), build_passed=True, lint_passed=True, strategy="create"
202+
_config(),
203+
_setup(default_branch="main"),
204+
build_passed=True,
205+
lint_passed=True,
206+
strategy="create",
193207
)
194208
assert url == "https://github.com/o/r/pull/1"
195209
assert "create-pr" not in run_cmd.labels()
210+
assert "reconcile-pr-base" not in run_cmd.labels()
196211

197212
def test_no_commits_means_no_pr(self, monkeypatch):
198213
# pr view -> empty (no existing PR); git log diff -> empty (no commits).
@@ -249,3 +264,104 @@ def responder(cmd):
249264
assert "Resolves #55" in body
250265
assert "**PASS**" in body # build passed
251266
assert "**FAIL**" in body # lint failed
267+
268+
269+
class TestReconcilePrBase:
270+
"""The agent picks its own PR --base; ensure_pr corrects it deterministically
271+
to setup.default_branch (the orchestrator's base for a stacked child / the
272+
detected repo default for a root). Live-caught on the #247 chain: a stacked
273+
child + a root both opened against a wrong 'main'."""
274+
275+
def test_retargets_when_base_mismatches(self, monkeypatch):
276+
# Existing PR is based on 'main' but the stacked child's real base is
277+
# the predecessor branch -> ensure_pr issues `gh pr edit --base <pred>`.
278+
pred = "bgagent/task-x/abca-1-predecessor"
279+
sub = _pr_view("https://github.com/o/r/pull/7", base="main")
280+
run_cmd = _RunCmdRecorder()
281+
monkeypatch.setattr(post_hooks.subprocess, "run", sub)
282+
monkeypatch.setattr(post_hooks, "run_cmd", run_cmd)
283+
284+
url = post_hooks.ensure_pr(
285+
_config(),
286+
_setup(default_branch=pred),
287+
build_passed=True,
288+
lint_passed=True,
289+
strategy="create",
290+
)
291+
assert url == "https://github.com/o/r/pull/7"
292+
assert "reconcile-pr-base" in run_cmd.labels()
293+
edit_cmd = run_cmd.cmd_for("reconcile-pr-base")
294+
assert "edit" in edit_cmd
295+
assert edit_cmd[edit_cmd.index("--base") + 1] == pred
296+
297+
def test_noop_when_base_matches(self, monkeypatch):
298+
# PR base already == default_branch -> no `gh pr edit`.
299+
sub = _pr_view("https://github.com/o/r/pull/7", base="develop")
300+
run_cmd = _RunCmdRecorder()
301+
monkeypatch.setattr(post_hooks.subprocess, "run", sub)
302+
monkeypatch.setattr(post_hooks, "run_cmd", run_cmd)
303+
304+
url = post_hooks.ensure_pr(
305+
_config(),
306+
_setup(default_branch="develop"),
307+
build_passed=True,
308+
lint_passed=True,
309+
strategy="create",
310+
)
311+
assert url == "https://github.com/o/r/pull/7"
312+
assert "reconcile-pr-base" not in run_cmd.labels()
313+
314+
def test_retarget_failure_warns_and_is_not_fatal(self, monkeypatch):
315+
# `gh pr edit` fails -> WARN naming the consequence, URL still returned.
316+
pred = "bgagent/task-x/abca-1-predecessor"
317+
sub = _pr_view("https://github.com/o/r/pull/7", base="main")
318+
run_cmd = _RunCmdRecorder(returncodes={"reconcile-pr-base": 1})
319+
monkeypatch.setattr(post_hooks.subprocess, "run", sub)
320+
monkeypatch.setattr(post_hooks, "run_cmd", run_cmd)
321+
warns: list[str] = []
322+
monkeypatch.setattr(
323+
post_hooks, "log", lambda lvl, msg: warns.append(msg) if lvl == "WARN" else None
324+
)
325+
326+
url = post_hooks.ensure_pr(
327+
_config(),
328+
_setup(default_branch=pred),
329+
build_passed=True,
330+
lint_passed=True,
331+
strategy="create",
332+
)
333+
assert url == "https://github.com/o/r/pull/7"
334+
assert any("PR remains based on 'main'" in w for w in warns)
335+
336+
def test_reconcile_skipped_for_freshly_created_pr_path(self, monkeypatch):
337+
# When the agent did NOT pre-create the PR, ensure_pr creates it with the
338+
# correct --base directly; no separate reconcile needed (create path
339+
# already uses default_branch). Guards against double-work.
340+
def responder(cmd):
341+
if "view" in cmd:
342+
return _cp(returncode=1, stderr="no pr")
343+
if "log" in cmd and "--reverse" in cmd:
344+
return _cp(returncode=0, stdout="feat: x\n")
345+
if "log" in cmd:
346+
return _cp(returncode=0, stdout="feat: x\n\n---")
347+
return _cp()
348+
349+
sub = _SubprocessRunRecorder(responder=responder)
350+
run_cmd = _RunCmdRecorder(stdouts={"create-pr": "https://github.com/o/r/pull/8\n"})
351+
monkeypatch.setattr(post_hooks, "ensure_pushed", lambda d, b: True)
352+
monkeypatch.setattr(post_hooks.subprocess, "run", sub)
353+
monkeypatch.setattr(post_hooks, "run_cmd", run_cmd)
354+
355+
pred = "bgagent/task-x/abca-1-predecessor"
356+
url = post_hooks.ensure_pr(
357+
_config(),
358+
_setup(default_branch=pred),
359+
build_passed=True,
360+
lint_passed=True,
361+
strategy="create",
362+
)
363+
assert url == "https://github.com/o/r/pull/8"
364+
# create path used the right base; no post-creation reconcile fired.
365+
create_cmd = run_cmd.cmd_for("create-pr")
366+
assert create_cmd[create_cmd.index("--base") + 1] == pred
367+
assert "reconcile-pr-base" not in run_cmd.labels()

0 commit comments

Comments
 (0)