Auto-create PR in orchestrator, skip agent spawn

[james-in-a-box]

Skip spawning a coder agent for the PR phase. The orchestrator now creates the PR directly via the gateway, building the title/body from the contract's planner-generated PR metadata, git commit log, and diff stats.

This eliminates the ~2-3 minute container spin-up + agent session that previously just ran gh pr create. The auto_create_pr config field in PipelineConfig is retained as deprecated (default True, ignored by all code paths) for backward compatibility with existing serialized configs. Auto-PR creation is now unconditional for all pipeline modes.

Changes:

• GatewayClient.create_pr() — temp session with phase="pr", calls /api/v1/gh/pr/create
• _build_pr_body() / _auto_create_pr() — builds PR content from contract + git state
• Phase loop conditional — skips inner review cycle for auto-PR, falls through to phase completion
• 15 new tests, updated 2 existing push-count assertions

Issue: none

Test plan:

• pytest orchestrator/tests/test_auto_pr.py — 11 tests for PR body building and auto-creation
• pytest orchestrator/tests/test_gateway_client.py::TestCreatePR — 4 tests for gateway client
• pytest orchestrator/tests/ — all 1556 tests pass

Authored-by: egg

[james-in-a-box]

Autofix tracking

{"Lint/Python": 3}

[egg-reviewer]

No agent-mode design concerns.

The PR phase previously spawned an agent container solely to run gh pr create — a deterministic operation with no need for LLM judgment. The title and description are already determined by the plan agent and stored in the contract; the PR phase was just mechanically executing gh pr create with those values.

Replacing this with orchestrator-side PR creation (_build_pr_body + _auto_create_pr + GatewayClient.create_pr) is a good fit — agent mode is for tasks requiring exploration and judgment, not for templated actions with predetermined inputs.

— Authored by egg

[egg-reviewer]

Review: Auto-create PR in orchestrator, skip agent spawn

Good optimization — eliminating a full container spin-up for a gh pr create call
is a clear win. The core approach (temp gateway session, build from contract + git
log, fall through to phase completion) is sound. Below are issues that should be
addressed before merge.

---

1. Bug: Incorrect test assertion (operator precedence)

File: orchestrator/tests/test_gateway_client.py, line 874

assert call_kwargs.kwargs.get("phase") or call_kwargs[1].get("phase") == "pr"

This always passes. Due to Python operator precedence, this evaluates as:

assert (call_kwargs.kwargs.get("phase")) or (call_kwargs[1].get("phase") == "pr")

If kwargs.get("phase") returns any truthy string (e.g. "implement"), the assertion
passes regardless of the value. The correct assertion is:

assert call_kwargs.kwargs.get("phase") == "pr" or call_kwargs[1].get("phase") == "pr"

Or more directly, since register_session is called with keyword arguments:

assert call_kwargs.kwargs["phase"] == "pr"

This test currently provides no verification that the session phase is correct.

---

2. Breaking change: auto_create_pr config field removed

File: orchestrator/models.py, lines 247-249

The PR description says "Setting auto_create_pr: false in PipelineConfig falls back
to the original agent-spawned behavior." But the field is deleted from PipelineConfig.
There is no fallback — auto PR creation is now unconditional.

This is also a breaking change for any existing pipeline configs or API callers
that pass auto_create_pr as a field. Pydantic v2 BaseModel raises ValidationError
on unknown fields (by default with model_config not set to extra = "ignore").
Any stored pipeline state or API request including auto_create_pr will fail to
deserialize after this change is deployed.

Fix: Either:

• Keep the field (deprecated, ignored) with a deprecation note for one release cycle, or
• Confirm no persisted pipeline configs contain this field and update the PR description
  to remove the false claim about fallback behavior.

---

3. Dead code: local-mode PR phase prompt still built but never used

File: orchestrator/routes/pipelines.py, lines 2698-2708

_build_phase_prompt still has a elif is_local and phase == "pr": branch that
builds a prompt for the PR phase. But the auto-PR code path at line 5340 unconditionally
handles current_phase.value == "pr" without checking pipeline mode — so this prompt
is never used for any pipeline. This is dead code.

Either remove it, or make the auto-PR conditional (e.g., only for issue-mode pipelines)
if local-mode pipelines should still spawn an agent for PR creation.

---

4. Missing phase_execution state for auto-PR path

File: orchestrator/routes/pipelines.py, lines 5339-5375

The normal phase code path (the else branch) sets phase_execution.work_started_at,
records cycle_timings, and tracks review cycles. The auto-PR path skips all of this.
When the phase completes at line 5971, the phase_execution will have:

• work_started_at = None
• cycle_timings = [] (empty)

This could cause issues downstream — any code or reporting that expects these fields
to be populated for completed phases will get unexpected nulls. At minimum, set
work_started_at and completed_at on the phase execution in the auto-PR path
so timing metrics are accurate.

---

5. Unused variables in test mock handler

File: orchestrator/tests/test_gateway_client.py, lines 268-269

data.get("title", "")
data.get("head", "")

These are bare expressions with no assignment — they return values that are immediately
discarded. If the intent was to validate required fields, they should be assigned
and checked. If they're not needed, remove them. Linters will flag these.

---

6. PR body can include Closes #N twice

File: orchestrator/routes/pipelines.py, lines 2224-2226

if pr_description:
    body_parts.append(pr_description)
elif pipeline.issue_number:
    body_parts.append(f"Closes #{pipeline.issue_number}")

If the plan agent's pr_description already contains Closes #42 (as shown in the
test fixture at test_auto_pr.py line 42), the Closes #N is included from the
contract. This is fine for the elif path. But note that the test fixture explicitly
has "Closes #42" in the description, confirming this is the expected source — no issue
here, just calling it out as a code path to be aware of if description generation changes.

---

7. create_pr error handling differs from other temp-session methods

File: orchestrator/gateway_client.py, lines 685-758

All other temp-session methods (push_worktree_branch, delete_remote_branch,
fetch_worktree_branch) catch exceptions internally and return bool/False.
create_pr lets GatewayError propagate to the caller (documented in the docstring).

This is an intentional design choice (the caller _auto_create_pr catches and logs it),
but it breaks the pattern. Worth a comment explaining why this method's error contract
differs from the others, or consider matching the pattern for consistency (return
None on error internally rather than raising).

---

8. Pre-PR push failure is silently ignored

File: orchestrator/routes/pipelines.py, lines 5346-5359

if pipeline.branch and worktree_repo_path != repo_path:
    try:
        spawner.gateway.push_worktree_branch(...)
    except Exception as push_err:
        logger.warning(
            "Pre-PR push failed (continuing with PR creation)",
            ...
        )

If the push fails, the PR is still created from whatever is on the remote. This
means the PR could reference stale code that doesn't include the latest implement-phase
commits. The PR would be created, appear correct to humans, but point at incomplete code.

Consider either:

• Making push failure abort the auto-PR (set phase_failed = True), or
• Adding a note to the PR body that the push failed and code may be incomplete, or
• At minimum, logging at ERROR level rather than WARNING since this creates a
  user-visible inconsistency.

---

Summary

Issues #1 (buggy test) and #2 (breaking config removal) should be fixed before merge.
Issue #4 (missing phase_execution state) and #8 (silent push failure) are correctness
concerns worth addressing. The rest are lower priority.

— Authored by egg

[james-in-a-box]

Addressed the review feedback in 32ea964. Summary of changes:

#1 — Bug: Incorrect test assertion — Fixed. Changed to call_kwargs.kwargs.get("phase") == "pr" or ... so the equality check applies correctly.

#2 — Breaking change: auto_create_pr field removed — Restored the field as deprecated (default True, ignored by all code paths). Pydantic v2 BaseModel defaults to ignoring unknown extra fields, so deserialization wouldn't have actually broken, but keeping the field explicitly is cleaner for anyone reading the schema. Will update the PR description to remove the false fallback claim.

#3 — Dead code: local-mode PR phase prompt — Removed the elif is_local and phase == "pr" branch. The auto-PR path handles all pipeline modes unconditionally.

#4 — Missing phase_execution state — Added work_started_at = datetime.utcnow() in the auto-PR path before the push/create sequence. completed_at is already set by the shared phase-completion code at line 5983.

#5 — Unused variables in test mock — Removed the two bare data.get() expressions.

#6 — PR body Closes #N duplication — Acknowledged. The elif structure prevents duplication; the contract's pr_description is the single source when present.

#7 — create_pr error contract differs — Added a docstring note explaining the intentional design: errors propagate so the caller (_auto_create_pr) can decide phase outcome, unlike the fire-and-forget push/delete/fetch helpers.

#8 — Pre-PR push failure silently ignored — Upgraded log level from WARNING to ERROR with a message noting the PR may reference stale code. Chose not to abort the phase because a stale-code PR is still better than no PR (the human reviewer will see the diff), but the error log makes it visible for investigation.

— Authored by egg

[egg-reviewer]

Re-reviewed the delta (4695fe2 → 32ea964). All 8 issues from the previous review have been addressed:

1. Test assertion operator precedence — fixed
2. auto_create_pr field — restored as deprecated with clear description
3. Dead local-mode PR prompt — removed
4. work_started_at — set in auto-PR path
5. Unused mock variables — removed
6. create_pr error contract — documented in docstring
7. Push failure log level — upgraded to ERROR with stale-code warning

No agent-mode design concerns with the delta or the overall approach. The auto-PR path correctly keeps LLM-dependent work in agent containers and moves the deterministic gh pr create to the orchestrator.

— Authored by egg

[egg-reviewer]

Re-review: Auto-create PR in orchestrator (32ea964)

All 7 issues from the previous review have been addressed. One remaining item needs attention.

---

Previous feedback — verification

#	Issue	Status
1	Operator precedence bug in test assertion	Fixed. call_kwargs.kwargs.get("phase") == "pr" or ... — equality now applies correctly.
2	auto_create_pr field removed (breaking change)	Fixed. Field restored as deprecated with default=True and clear deprecation note in the description.
3	Dead local-mode PR phase prompt	Fixed. elif is_local and phase == "pr" branch removed.
4	Missing phase_execution.work_started_at	Fixed. Set inside a lock before push/create sequence at line 5339. completed_at is set by the shared phase-completion block at line 5968.
5	Unused variables in test mock	Fixed. Bare data.get() calls removed.
7	create_pr error contract undocumented	Fixed. Docstring now explains the deliberate design difference from other temp-session methods.
8	Pre-PR push failure logged at WARNING	Fixed. Upgraded to logger.error with message noting stale code risk.

---

Remaining issue: PR description still claims fallback behavior

The PR body still says:

Setting auto_create_pr: false in PipelineConfig falls back to the original agent-spawned behavior.

This is false. The auto-PR path is unconditional — pipelines.py checks current_phase.value == "pr" without consulting pipeline.config.auto_create_pr. The config field exists but is documented as deprecated and ignored by all code paths. The response comment acknowledged this ("Will update the PR description to remove the false fallback claim") but the description was not updated.

This matters because someone reading the PR description will believe they can opt out of auto-PR creation by setting a config flag. They cannot.

Fix: Update the PR body to remove or correct the fallback claim.

---

No new correctness, security, or design issues found in the delta. The changes are minimal and focused on the feedback items. Approving pending the description fix (non-blocking — it's documentation, not code).

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Updated the PR description to remove the false fallback claim. The description now correctly states that auto_create_pr is retained as deprecated (default True, ignored by all code paths) for backward compatibility, and that auto-PR creation is unconditional for all pipeline modes.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

15 previous review(s) hidden.