diff --git a/docs/guides/concurrent-execution.md b/docs/guides/concurrent-execution.md index 00d250d8e1..aabd07cb60 100644 --- a/docs/guides/concurrent-execution.md +++ b/docs/guides/concurrent-execution.md @@ -284,8 +284,9 @@ egg-orch message send --to coder --type HANDOFF \ --subject "CI needs a step for the new regression suite" \ --body "tests/test_auth_regression.py won't run in CI. The workflow needs a step invoking it. Please stage the end-state under .github-staging/workflows/ -ci.yml (add a 'pytest tests/test_auth_regression.py' step); the PR builder -emits a manual move-into-.github step for the human reviewer." +ci.yml (add a 'pytest tests/test_auth_regression.py' step) and call the +staged file out in your PR body so the human reviewer moves it into +.github/ before merge." # 2. Coder receives the HANDOFF on its next poll cycle: egg-orch message poll --wait 30 diff --git a/docs/reference/agent-roles.md b/docs/reference/agent-roles.md index 877ebcfbbe..a073a80d66 100644 --- a/docs/reference/agent-roles.md +++ b/docs/reference/agent-roles.md @@ -205,8 +205,9 @@ worktree branch. config — branch-protection invariant). To propose `.github/` changes, write the end-state to `.github-staging/` mirroring the `.github/` structure (e.g. stage `.github/workflows/ci.yml` as - `.github-staging/workflows/ci.yml`); the PR builder auto-emits a - manual step asking the human reviewer to move the files before merge + `.github-staging/workflows/ci.yml`); the agent must call the staged + files out in its PR body so the human reviewer moves them into + `.github/` before merge ([#2508](https://github.com/jwbron/egg/issues/2508)). `sandbox/scripts/` is **writable** — the gateway is the sole egress chokepoint, so credential-shim modifications are reviewed by diff --git a/gateway/gateway.py b/gateway/gateway.py index 838a8fa2e2..5ffb99e056 100644 --- a/gateway/gateway.py +++ b/gateway/gateway.py @@ -3746,10 +3746,10 @@ def gh_pr_create() -> tuple[Response, int] | Response: if policy_result.details and policy_result.details.get("force_draft"): draft = True - # Inject machine-parseable pipeline metadata as an HTML comment. - # Note: _build_pr_body (in the orchestrator) also adds a human-readable - # "## Pipeline Context" section. The two formats are intentionally - # complementary — visible for humans, hidden comment for tooling. + # Inject machine-parseable pipeline metadata as an HTML comment so + # downstream tooling (status reporters, audit scrapers) can recover + # the pipeline_id / agent_role / issue from the PR body without + # round-tripping through the orchestrator state store. session = getattr(g, "session", None) session_pipeline_id = getattr(session, "pipeline_id", None) if session else None if session_pipeline_id: diff --git a/gateway/tests/test_agent_restrictions_patterns.py b/gateway/tests/test_agent_restrictions_patterns.py index d779a13a9f..3cbc559def 100644 --- a/gateway/tests/test_agent_restrictions_patterns.py +++ b/gateway/tests/test_agent_restrictions_patterns.py @@ -261,9 +261,9 @@ def test_can_write_github_staging_dir(self, pattern): The `.github/` blocked prefix is matched via ``startswith(".github/")``, which doesn't match `.github-staging/...`, so the catch-all - ``**`` allowlist reaches it. The PR builder turns staged files - into a manual step asking the human reviewer to move them - into `.github/` before merge. + ``**`` allowlist reaches it. The agent calls the staged files + out in its PR body so the human reviewer moves them into + `.github/` before merge. """ assert pattern.can_write(".github-staging/workflows/ci.yml") is True assert pattern.can_write(".github-staging/CODEOWNERS") is True diff --git a/orchestrator/routes/pipelines.py b/orchestrator/routes/pipelines.py index 11ccf4ff9f..5152586a1a 100644 --- a/orchestrator/routes/pipelines.py +++ b/orchestrator/routes/pipelines.py @@ -1835,12 +1835,11 @@ def create_pipeline() -> tuple[Response, int]: # # This is the *primary* eviction site for auto-FAILED prior runs, # not just a defensive backstop: paths like restart_agent spawn - # failure and _handle_pr_creation_failure call - # store.update_pipeline / store.save_pipeline directly (bypassing - # PATCH), so the PATCH-site clear never fires for them. Without - # this POST-site clear, those auto-FAILED pipelines would leak - # consensus + message-store state into the next run that reuses - # the id. + # failure call store.update_pipeline / store.save_pipeline directly + # (bypassing PATCH), so the PATCH-site clear never fires for them. + # Without this POST-site clear, those auto-FAILED pipelines would + # leak consensus + message-store state into the next run that + # reuses the id. _clear_pipeline_runtime_state(pipeline.id, reason="pipeline_create") logger.info( @@ -6187,15 +6186,14 @@ def _build_role_restrictions_section(repo: str | None = None) -> str: "end-state to top-level `.github-staging/`, mirroring the " "`.github/` structure (e.g. a proposed change to " "`.github/workflows/ci.yml` is staged at " - "`.github-staging/workflows/ci.yml`). The orchestrator's PR " - "builder auto-detects `.github-staging/` and emits a manual " - "step asking the human reviewer to move the staged files into " - "place before merge. Assign such tasks to `role: coder` and " - "make the staging path explicit in the task's " - "`files_affected`. `.github-staging/` must remain tracked by " - "git (do not add it to `.gitignore`); otherwise the staged " - "files won't be in the PR commit and the reviewer's `git mv` " - "will fail." + "`.github-staging/workflows/ci.yml`). The producing agent must " + "call the staged files out in the PR body so the human " + "reviewer moves them into `.github/` before merge. Assign such " + "tasks to `role: coder` and make the staging path explicit in " + "the task's `files_affected`. `.github-staging/` must remain " + "tracked by git (do not add it to `.gitignore`); otherwise the " + "staged files won't be in the PR commit and the reviewer's " + "`git mv` will fail." ) lines.append("") @@ -8730,66 +8728,6 @@ def _fetch_pr_state(pr_number: int, repo: str | None = None) -> dict[str, Any]: } -def _handle_pr_creation_failure( - pipeline_id: str, - current_phase: str, - store, - reason: str | None = None, -) -> None: - """Mark a pipeline as FAILED after PR creation returns no URL. - - Extracted from ``_health_monitor_poll`` so this state-transition logic can - be tested independently of the full polling loop. - - The error message attached to the pipeline tells the user exactly what - happened and how to rescue the work. The agents' commits are on - ``origin/`` regardless of the failure mode, so the - rescue is always "open the PR manually against that branch" — we - surface the exact ``gh pr create`` invocation to avoid forcing users - to dig through orchestrator logs (see #1731). - - ``reason`` is a short phrase explaining *why* PR creation failed (e.g. - ``"fetch+rebase reconcile failed"``). When omitted, the generic - ``"no PR URL returned"`` message is used for back-compat. - """ - reason_text = reason or "no PR URL returned" - error_msg = f"Auto PR creation failed: {reason_text}" - logger.error(error_msg, pipeline_id=pipeline_id, reason=reason_text) - with get_pipeline_state_lock(pipeline_id): - pipeline = store.load_pipeline(pipeline_id) - phase_execution = pipeline.get_phase_execution(current_phase) - # Compose a user-facing message that includes the rescue hint, - # using pipeline state we only have access to inside the lock. - rescue_hint = _format_rescue_hint(pipeline) - full_error = f"{error_msg}\n{rescue_hint}" if rescue_hint else error_msg - phase_execution.status = PipelineStatus.FAILED - phase_execution.error = full_error - phase_execution.completed_at = datetime.now(UTC) - pipeline.status = PipelineStatus.FAILED - pipeline.error = full_error - store.save_pipeline(pipeline) - - -def _format_rescue_hint(pipeline) -> str: - """Build a user-facing rescue hint for a pipeline whose PR couldn't be auto-created. - - Returns an empty string when we don't have enough state to compose a - useful hint (no repo or no branch on the pipeline) — in that case the - error log + pipeline ID are the user's only handholds. - """ - repo = getattr(pipeline, "repo", None) - branch = getattr(pipeline, "branch", None) - if not repo or not branch: - return "" - base = getattr(pipeline, "base_branch", None) or "main" - return ( - f"Agent work is on origin/{branch} in {repo}. " - f"To open the PR manually:\n" - f" gh pr create --repo '{repo}' --head '{branch}' --base '{base}' " - f'--title "..." --body "..."' - ) - - BRC_HISTORY_TYPES = frozenset( { "CONSENSUS_PROPOSE", @@ -9586,189 +9524,6 @@ def _sort_key(name: str) -> tuple[int, int, str]: return f"_Per-phase BRC transcripts: {links}._" -def _pr_metadata_from_plan_draft( - worktree_repo_path: Path, - issue_number: int | None, - pipeline_id: str, -) -> tuple[str | None, str, str, str, list[str], str | None]: - """Parse PR metadata from the plan draft on disk. - - Used as a fallback in ``_build_pr_body`` when ``contract.pr`` is not - populated — e.g. when the plan-phase contract write did not reach the - branch tip (see #1829). The plan draft itself is reliably on the - branch even when the contract is not. - - Returns ``(title, description, test_plan, manual_steps, warnings, - draft_rel_path)``. ``title`` is ``None`` when the draft is missing, - unparseable, or has no ``pr:`` block, signalling the caller to fall - through to the next tier. ``warnings`` is a list of - human-readable parse warning strings collected from ``parse_plan`` - (empty when the parse was clean or the draft was absent); it is - surfaced in the PR body when the caller falls through to the stub - tier so reviewers can see what went wrong (see #1975). - ``draft_rel_path`` is the relative path to the draft that was - parsed, or ``None`` if no draft was attempted. - """ - warnings_out: list[str] = [] - draft_rel = _get_draft_path("plan", issue_number=issue_number, pipeline_id=pipeline_id) - if not draft_rel: - return None, "", "", "", warnings_out, None - plan_path = worktree_repo_path / draft_rel - if not plan_path.exists(): - warnings_out.append(f"Plan draft not found at {draft_rel}") - return None, "", "", "", warnings_out, draft_rel - try: - from egg_contracts.plan_parser import parse_plan - - result = parse_plan(plan_path.read_text()) - except Exception as e: - logger.debug( - "Could not parse plan draft for PR metadata fallback", - path=str(plan_path), - error=str(e), - ) - warnings_out.append(f"parse_plan raised: {e}") - return None, "", "", "", warnings_out, draft_rel - for w in result.warnings: - msg = w.message - if w.context: - msg = f"{msg} ({w.context})" - warnings_out.append(msg) - if not result.pr_title: - return None, "", "", "", warnings_out, draft_rel - return ( - result.pr_title, - result.pr_description or "", - result.pr_test_plan or "", - result.pr_manual_steps or "", - warnings_out, - draft_rel, - ) - - -def _build_github_staging_manual_step(worktree_repo_path: Path) -> str: - """Render the auto manual-step for `.github-staging/` files (issue #2508). - - Producer agents (coder, etc.) cannot push to `.github/` because the - gateway blocks the path as a branch-protection invariant. When a - plan calls for CI workflow or CODEOWNERS changes, the agent instead - writes the proposed end-state to top-level `.github-staging/`, - mirroring the `.github/` structure. This helper scans that - directory and returns a markdown step the human reviewer must - complete before merge: review the staged files, move them into - `.github/`, delete the staging dir, and push the resulting commit. - - Returns an empty string when `.github-staging/` is absent or empty. - """ - staging_dir = worktree_repo_path / ".github-staging" - # Drop the whole step when ``.github-staging`` itself is a symlink: - # ``Path.is_dir()`` follows symlinks, so without this guard a - # ``.github-staging -> /etc`` (or any other host path) would let - # ``rglob`` enumerate the link target's files into the manual-step - # file list, polluting the PR body with arbitrary host-filesystem - # paths. Mirrors the per-entry symlink guard below. - if staging_dir.is_symlink(): - return "" - if not staging_dir.is_dir(): - return "" - - staged_paths: list[str] = [] - for path in sorted(staging_dir.rglob("*")): - # Skip symlinks: ``Path.is_file()`` follows them, so without this - # guard a staged ``.github-staging/evil.yml`` → ``/etc/passwd`` - # would be surfaced in the manual-step file list, the reviewer's - # ``git mv`` would preserve it, and ``.github/evil.yml`` would - # land in the repo as a symlink. The reviewer's only mitigation - # would be the diff (where a symlink shows as a small mode - # change that's easy to skim past). Drop staged symlinks here so - # the helper is the choke point. - if path.is_symlink(): - continue - if not path.is_file(): - continue - try: - rel = path.relative_to(worktree_repo_path).as_posix() - except ValueError: - continue - staged_paths.append(rel) - - if not staged_paths: - return "" - - # Compute concrete move commands per staged file, choosing - # ``git mv`` vs ``git rm`` + ``git mv`` based on whether the target - # ``.github/`` already exists. ``git mv`` refuses to - # overwrite an existing destination, so a template that always - # emits the plain form breaks for replacement scenarios (e.g. - # restaging an existing workflow). - staging_prefix = ".github-staging/" - target_prefix = ".github/" - mkdir_dirs: list[str] = [] - move_cmds: list[str] = [] - for rel in staged_paths: - if not rel.startswith(staging_prefix): - continue - rest = rel[len(staging_prefix) :] - target_rel = f"{target_prefix}{rest}" - target_dir = target_rel.rsplit("/", 1)[0] if "/" in rest else target_prefix.rstrip("/") - if target_dir and target_dir not in mkdir_dirs: - mkdir_dirs.append(target_dir) - target_abs = worktree_repo_path / target_rel - # ``Path.exists()`` follows symlinks and returns False for a - # broken link, so an existing-but-broken symlink would slip - # through the existence check and ``git mv`` would still refuse - # to overwrite it. ``Path.is_symlink()`` returns True regardless - # of whether the target resolves, so the disjunction catches - # regular files, valid symlinks, and broken symlinks. - if target_abs.is_symlink() or target_abs.exists(): - move_cmds.append(f"git rm {target_rel} # target exists; remove before mv") - move_cmds.append(f"git mv {rel} {target_rel}") - - lines = [ - "### Move staged `.github/` changes (auto-generated, issue #2508)", - "", - "This PR includes proposed `.github/` changes under `.github-staging/`. " - "Agent roles cannot push to `.github/` directly (CI workflow / CODEOWNERS " - "branch-protection invariant), so the agent staged the proposed " - "end-state for human review.", - "", - "Staged files:", - ] - for rel in staged_paths: - lines.append(f"- `{rel}`") - lines.extend( - [ - "", - "Before merging:", - "", - "1. Review each staged file for correctness — these are proposed " - "CI / repo-config changes that bypass the agent's normal sandbox.", - "2. Run the following to move each staged file into `.github/` " - "(commands below are pre-computed for this PR; replacement targets " - "are handled via `git rm` + `git mv` since `git mv` refuses to " - "overwrite an existing destination):", - " ```", - ] - ) - for d in mkdir_dirs: - lines.append(f" mkdir -p {d}") - for cmd in move_cmds: - lines.append(f" {cmd}") - lines.extend( - [ - " ```", - " After the moves, `.github-staging/` is no longer tracked " - "by git (git doesn't track empty directories). Run " - "`rm -rf .github-staging` locally if you want to clear any " - "leftover empty subdirectories from your worktree.", - "3. Commit the move and push from a context with the GitHub " - "`workflow` scope (a normal user push works; the bot token may " - "not — see issue #2508 layer 2).", - ] - ) - return "\n".join(lines) - - def _derive_producer_roles_with_tasks( pipeline_id: str, slice_id: str | None, @@ -13527,9 +13282,10 @@ def _build_file_boundary_section(role_value: str, repo: str | None = None) -> st "end-state to top-level `.github-staging/` instead, mirroring " "the `.github/` structure (e.g. stage " "`.github/workflows/test-e2e.yml` as " - "`.github-staging/workflows/test-e2e.yml`). The PR builder " - "auto-emits a manual step asking the human reviewer to move " - "the staged files into place before merge — see issue #2508." + "`.github-staging/workflows/test-e2e.yml`). Call out the " + "staged files explicitly in your PR body so the human reviewer " + "knows to move them into `.github/` before merge — see issue " + "#2508." ) lines.append("") return "\n".join(lines) diff --git a/shared/egg_contracts/agent_roles.py b/shared/egg_contracts/agent_roles.py index 539cf43fa5..472e9daec6 100644 --- a/shared/egg_contracts/agent_roles.py +++ b/shared/egg_contracts/agent_roles.py @@ -214,8 +214,8 @@ def depends_on(self, other: AgentRole) -> bool: ".egg-state/agent-outputs/", # For handoff data # Issue #2508: staging dir for proposed `.github/` changes. # `.github/` itself is blocked below; the agent stages the - # proposed end-state here and the PR builder emits a manual - # step asking the human reviewer to move the files into + # proposed end-state here and calls the staged files out + # in its PR body so the human reviewer moves them into # `.github/` before merge. ".github-staging/", ], diff --git a/shared/egg_contracts/plan_parser.py b/shared/egg_contracts/plan_parser.py index 47f7814536..fe2b52fd5e 100644 --- a/shared/egg_contracts/plan_parser.py +++ b/shared/egg_contracts/plan_parser.py @@ -1681,8 +1681,8 @@ def _check_role_files(task: Task, slice_id: str, repo: str | None = None) -> str "No producer role can push every file in this task. Either " "split the task so each subtask falls within a single " "role's scope, or — for `.github/` files — stage them " - "under top-level `.github-staging/` and let the PR " - "builder emit a manual reviewer step (issue #2508)." + "under top-level `.github-staging/` and call them out in " + "the PR body for the human reviewer (issue #2508)." ) return ( f"Task '{task.id}' (slice '{slice_id}') is assigned role " diff --git a/shared/egg_restrictions/hints.py b/shared/egg_restrictions/hints.py index a0bf2d8925..ef39948ede 100644 --- a/shared/egg_restrictions/hints.py +++ b/shared/egg_restrictions/hints.py @@ -44,15 +44,15 @@ ".github/", # Issue #2508: producer roles cannot push to `.github/` directly, # but they CAN stage proposed changes under top-level - # `.github-staging/` mirroring the `.github/` structure. The PR - # builder auto-emits a manual step asking the human reviewer to - # move the staged files into `.github/` before merge. Point + # `.github-staging/` mirroring the `.github/` structure. Point # agents at the staging path so a 403 here becomes a - # self-correcting nudge rather than a dead-end. + # self-correcting nudge rather than a dead-end. The agent must + # call the staged files out in its PR body so the human reviewer + # moves them into `.github/` before merge. "Stage your proposed change at `.github-staging/` " - "instead — the PR builder will surface a manual step asking " - "the human reviewer to move the staged files into `.github/` " - "before merge. See issue #2508.", + "instead, and call the staged files out in your PR body so the " + "human reviewer moves them into `.github/` before merge. See " + "issue #2508.", ), ( "docs/", diff --git a/shared/egg_restrictions/patterns.py b/shared/egg_restrictions/patterns.py index 1afc9c288c..93de2d92eb 100644 --- a/shared/egg_restrictions/patterns.py +++ b/shared/egg_restrictions/patterns.py @@ -230,9 +230,9 @@ def _build_coder_pattern( # `.github/` changes (CI workflow edits, CODEOWNERS rotation) # write the proposed end-state to top-level `.github-staging/` # mirroring the `.github/` structure; the prefix-match below - # leaves `.github-staging/` allowed via the `**` allowlist, and - # `_build_pr_body` auto-emits a manual step for the human - # reviewer to move the files into `.github/` before merge + # leaves `.github-staging/` allowed via the `**` allowlist. + # The agent flags the staged files in its PR body so the + # human reviewer moves them into `.github/` before merge # (issue #2508). ".github/", ],