diff --git a/docs/architecture/git-isolation.md b/docs/architecture/git-isolation.md index 946f15d3a5..8f8c112854 100644 --- a/docs/architecture/git-isolation.md +++ b/docs/architecture/git-isolation.md @@ -35,7 +35,7 @@ This document focuses on the specific challenge of **multi-agent git isolation** | Agent accesses another agent's workspace | Filesystem isolation---other workspaces don't exist in container's view | | Agent pushes to unauthorized branches | Gateway enforces branch ownership policy | | Agent pushes malicious code directly to main | Gateway blocks direct pushes to protected branches; PRs require human review | -| Agent bypasses BRC consensus in concurrent mode | Gateway blocks direct `git push` in concurrent mode; requires `consensus_push` marker from `egg-orch consensus propose --push` | +| Agent bypasses BRC consensus in concurrent mode | Gateway blocks direct `git push` in concurrent mode; requires `consensus_push` marker from `mcp__brc__propose` (or fallback `egg-orch consensus propose --push`) | | Agent discovers or exfiltrates credentials | Credentials only exist in gateway; container never sees them | | Agent modifies git config to bypass security | Container has no access to git metadata; config is gateway-controlled | | Agent escapes via git hooks or filters | Hooks universally disabled via `core.hooksPath=/dev/null` in gateway and orchestrator; filters mitigated in containers by metadata isolation (no `.gitattributes` processing); gateway protected by branch ownership policy (agents cannot push to main) and required human review of all commits | @@ -258,7 +258,7 @@ Each agent works on its own isolated worktree with its own staging area. This ap **Pipeline agents:** In concurrent pipeline execution, all agents push to the same shared branch (e.g., `egg/issue-{N}`) but each agent has its own worktree. Since each role has mutually exclusive file write permissions (coder → source code, tester → tests, documenter → docs), push rebases cannot conflict. Reviewer agents sync their worktrees before reviewing by fetching and merging the pipeline branch, ensuring they evaluate up-to-date code from producers. See [Concurrent Execution Guide](../guides/concurrent-execution.md#per-agent-worktree-isolation) for details. -**Concurrent-mode push enforcement:** In BRC mode, the gateway blocks direct `git push` from pipeline agents — all pushes must go through `egg-orch consensus propose --push`, which bundles the push with a BRC proposal. This structurally enforces the "all changes must be reviewed" invariant rather than relying on agent compliance. See [Gateway README — Concurrent-Mode Push Enforcement](../../gateway/README.md#concurrent-mode-push-enforcement-brc-sessions) for details. +**Concurrent-mode push enforcement:** In BRC mode, the gateway blocks direct `git push` from pipeline agents — all pushes must go through `mcp__brc__propose` (which pushes to origin and sends CONSENSUS_PROPOSE in one step; push is on by default). The fallback CLI is `egg-orch consensus propose --push`. This structurally enforces the "all changes must be reviewed" invariant rather than relying on agent compliance. See [Gateway README — Concurrent-Mode Push Enforcement](../../gateway/README.md#concurrent-mode-push-enforcement-brc-sessions) for details. **Worktree-aware APIs:** All gateway APIs that access the filesystem use `map_container_path_to_worktree()` to resolve container repo paths to worktree paths. This includes git operations, contract operations (`egg-contract show`, `add-commit`, `add-decision`, etc.), and checkpoint operations. The mapping is transparent to agents --- they use their normal repo path and the gateway resolves it to the correct worktree. diff --git a/docs/guides/agent-teams.md b/docs/guides/agent-teams.md index c19454003e..07d7c0ea6d 100644 --- a/docs/guides/agent-teams.md +++ b/docs/guides/agent-teams.md @@ -226,7 +226,7 @@ When a producer pushes new commits after proposing, existing reviews become stal This mechanism enforces the principle that **all changes must be reviewed**: post-proposal pushes cannot bypass the review process. The `check_confirm_guard()` provides a server-side blocking mechanism even if a reviewer misses the `CONSENSUS_RE_REVIEW` notification. See [Concurrent Execution — Auto Re-Propose on Push/Commit](concurrent-execution.md#auto-re-propose-on-pushcommit) for the full details. -Additionally, the gateway enforces that **direct `git push` is blocked** in concurrent mode — agents must use `egg-orch consensus propose --push` to bundle the push with a BRC proposal. This makes the review invariant structural rather than relying on auto-repropose detection. See [Concurrent Execution — Gateway-Level Push Enforcement](concurrent-execution.md#gateway-level-push-enforcement-concurrent-mode) for details. +Additionally, the gateway enforces that **direct `git push` is blocked** in concurrent mode — agents must use `mcp__brc__propose` (or the fallback CLI `egg-orch consensus propose --push`) to bundle the push with a BRC proposal. This makes the review invariant structural rather than relying on auto-repropose detection. See [Concurrent Execution — Gateway-Level Push Enforcement](concurrent-execution.md#gateway-level-push-enforcement-concurrent-mode) for details. ### Agent Crash Mid-Protocol diff --git a/docs/guides/concurrent-execution.md b/docs/guides/concurrent-execution.md index 0481dc4a98..28d0b78b3a 100644 --- a/docs/guides/concurrent-execution.md +++ b/docs/guides/concurrent-execution.md @@ -480,15 +480,15 @@ When a producer pushes new commits after proposing, existing reviews become stal ### Gateway-Level Push Enforcement (Concurrent Mode) -While auto re-propose provides a **safety net** for stale reviews, it relies on the orchestrator detecting post-proposal pushes. A stronger guarantee comes from the gateway itself: in concurrent mode, **direct `git push` is blocked** — all pushes must go through `egg-orch consensus propose --push`. +While auto re-propose provides a **safety net** for stale reviews, it relies on the orchestrator detecting post-proposal pushes. A stronger guarantee comes from the gateway itself: in concurrent mode, **direct `git push` is blocked** — all pushes must go through `mcp__brc__propose` (the fallback CLI is `egg-orch consensus propose --push`). **How the marker flows:** -1. Agent runs `egg-orch consensus propose --push` -2. The orch CLI calls the gateway push API directly (bypassing the git wrapper) with `"consensus_push": true` in the JSON payload +1. Agent calls `mcp__brc__propose(...)` (push defaults to true) — or runs `egg-orch consensus propose --push` +2. Both surfaces delegate to `egg_agent_tools.push.consensus_push()`, which calls the gateway push API directly (bypassing the git wrapper) with `"consensus_push": true` in the JSON payload 3. The gateway checks: if `EGG_CONCURRENT_MODE=true` AND the session has a `pipeline_id` AND the push is not infrastructure (checkpoints/pipeline state), then `consensus_push` must be present -4. Pushes without the marker are rejected with HTTP 403 -5. Fallback: when `GATEWAY_URL` is not set (e.g., local development), the orch CLI falls back to plain `git push`. No concurrent-mode enforcement exists in this path — the gateway is not running to enforce it +4. Pushes without the marker are rejected with HTTP 403 and the error points at `mcp__brc__propose` +5. Fallback: when `GATEWAY_URL` is not set (e.g., local development), the helper falls back to plain `git push`. No concurrent-mode enforcement exists in this path — the gateway is not running to enforce it **Relationship to auto re-propose:** Gateway enforcement makes auto re-propose less critical in concurrent mode — every push IS a proposal, so there are no "orphan pushes" to detect. Auto re-propose remains as defense-in-depth for edge cases (e.g., if an agent manages to push through an alternative path). @@ -496,7 +496,9 @@ While auto re-propose provides a **safety net** for stale reviews, it relies on **Error message for agents:** ``` -Direct push blocked in concurrent mode. Use: egg-orch consensus propose --push +Direct git push is blocked in BRC mode. Publish your artifact via the +mcp__brc__propose tool (which pushes to origin and sends CONSENSUS_PROPOSE +in one step). Fallback CLI: `egg-orch consensus propose --push`. ``` See [Gateway README — Concurrent-Mode Push Enforcement](../../gateway/README.md#concurrent-mode-push-enforcement-brc-sessions) for implementation details. See [#1669](https://github.com/jwbron/egg/issues/1669) for the motivating incident and design rationale. diff --git a/gateway/filtered_push.py b/gateway/filtered_push.py index 87a5ec92fb..b67856ea95 100644 --- a/gateway/filtered_push.py +++ b/gateway/filtered_push.py @@ -294,7 +294,7 @@ def execute_filtered_push( attributed_commits: list[str], attributed_files: list[AttributedFile], blocked_own_files: set[str], - push_fn: Callable[[], tuple[bool, str | None]], + push_fn: Callable[[str], tuple[bool, str | None]], registry_register: Callable[..., Any], pipeline_id: str | None = None, repo: str | None = None, @@ -312,10 +312,13 @@ def execute_filtered_push( blocked_own_files: The set of paths the role cannot write; any own-role file matching this set is stripped from each commit it appears in. - push_fn: Callable ``push_fn() -> (ok: bool, error: str | None)`` - that performs ``git push`` with whatever refspec / options - the caller wants. Invoked after HEAD has been advanced to - the rewritten tip. + push_fn: Callable ``push_fn(tip_sha) -> (ok: bool, error: str | None)`` + that performs the actual ``git push``. The helper hands it + the rewritten tip SHA so the callee can build a SHA-to-refspec + push (``:refs/heads/``) without needing a + local ``refs/heads/`` — see #1994 (directory-style + refs like ``refs/heads//work`` from sibling worktrees + otherwise block creating the leaf ref locally). registry_register: Callable ``(sha, role, pipeline_id, repo, branch) -> bool`` that registers a rewritten own-commit with the authorship registry. Best-effort; failures are @@ -564,19 +567,15 @@ def execute_filtered_push( rewritten_commits=rewritten_commits, ) - # update-ref so the local branch matches our rewrite. - ur = _git(exec_path, "update-ref", f"refs/heads/{branch}", final_tip) - if ur.returncode != 0: - _rollback(exec_path, original_head, branch) - return FilteredPushResult( - success=False, - error=f"update-ref refs/heads/{branch} failed: {(ur.stderr or '').strip()}", - ) - - # Now push. If it fails, roll HEAD + the branch ref back and - # restore the worktree so the caller sees the pre-attempt state. + # Push the rewritten tip straight to the remote. We intentionally + # do NOT ``update-ref refs/heads/`` beforehand: sibling + # worktrees may hold a directory-style ref like + # ``refs/heads//work`` (per-role work branches from #1986) + # which blocks creating ``refs/heads/`` as a leaf ref in + # the shared ref store. Pushing by SHA sidesteps that entirely + # (see #1994). try: - ok, push_err = push_fn() + ok, push_err = push_fn(final_tip) except Exception as exc: # pragma: no cover - defensive _rollback(exec_path, original_head, branch) return FilteredPushResult(success=False, error=f"push raised: {exc}") @@ -585,6 +584,19 @@ def execute_filtered_push( _rollback(exec_path, original_head, branch) return FilteredPushResult(success=False, error=push_err or "Push failed") + # Best-effort: sync the local branch ref to the pushed tip so + # ``git log `` matches origin. Allowed to fail silently + # when a directory-style ref collision prevents the write — the + # push already landed on origin, and a subsequent ``git fetch`` + # reconciles local state. + ur = _git(exec_path, "update-ref", f"refs/heads/{branch}", final_tip) + if ur.returncode != 0: + logger.warning( + "filtered_push_local_ref_sync_failed", + branch=branch, + error=(ur.stderr or "").strip(), + ) + # Post-success: fast-forward the worktree + index to the new tip, # then re-stage the excluded files so the next role sees them as # uncommitted changes (decision-6). diff --git a/gateway/gateway.py b/gateway/gateway.py index 424528d325..c0b0f74a1f 100644 --- a/gateway/gateway.py +++ b/gateway/gateway.py @@ -1008,75 +1008,89 @@ def git_push() -> tuple[Response, int] | Response: # SECURITY: Pipeline and concurrent-mode push enforcement. # Infrastructure pushes (checkpoint branches, etc.) are exempt from both checks. if not is_infrastructure_push: - # Push-target enforcement: in pipeline mode, agents must push only to - # their assigned branch. Prevents improvising branch names on push failure. - # Killswitch: set PUSH_TARGET_ENFORCEMENT=false to disable. - push_target_enforcement = os.environ.get("PUSH_TARGET_ENFORCEMENT", "true").lower() not in ( - "false", - "0", - "no", - ) - if push_target_enforcement and hasattr(g, "session") and g.session: - session_pipeline_id = getattr(g.session, "pipeline_id", None) - session_assigned_branch = getattr(g.session, "assigned_branch", None) - if isinstance(session_pipeline_id, str) and isinstance(session_assigned_branch, str): - if branch != session_assigned_branch: + # Concurrent-mode enforcement runs FIRST. In BRC mode the agent's + # work branch (e.g. egg/-/work) never matches the + # assigned branch, so the wrong-branch check below would almost + # always fire first with a misleading message — sending agents + # down a refspec-guessing rabbit hole. The concurrent-mode + # error points at the actionable tool (#1994). + # Killswitch: set CONCURRENT_PUSH_ENFORCEMENT=false to disable. + concurrent_push_enforcement = os.environ.get( + "CONCURRENT_PUSH_ENFORCEMENT", "true" + ).lower() not in ("false", "0", "no") + concurrent_mode = os.environ.get("EGG_CONCURRENT_MODE", "").lower() == "true" + if concurrent_push_enforcement and concurrent_mode: + session_pipeline_id = None + if hasattr(g, "session") and g.session: + session_pipeline_id = getattr(g.session, "pipeline_id", None) + if isinstance(session_pipeline_id, str) and session_pipeline_id: + if not data.get("consensus_push"): audit_log( - "push_denied_wrong_branch", + "push_denied_concurrent_mode", "git_push", success=False, details={ "repo": repo, "branch": branch, - "assigned_branch": session_assigned_branch, "pipeline_id": session_pipeline_id, + "reason": "Direct push blocked in concurrent mode", }, ) return make_error( - f"Pipeline sessions must push to their assigned branch " - f"'{session_assigned_branch}'. Got '{branch}'.", + "Direct git push is blocked in BRC mode. " + "Publish your artifact via the mcp__brc__propose " + "tool (which pushes to origin and sends " + "CONSENSUS_PROPOSE in one step). Fallback CLI: " + "`egg-orch consensus propose --push`.", status_code=403, details={ - "assigned_branch": session_assigned_branch, - "attempted_branch": branch, + "mode": "concurrent", "pipeline_id": session_pipeline_id, + "requirement": "consensus_push", + "recommended_tool": "mcp__brc__propose", }, ) - # Concurrent-mode enforcement: in concurrent/BRC mode, agents must push - # through the consensus protocol (egg-orch consensus propose --push) - # which sets a consensus_push marker. Direct pushes are blocked to ensure - # all changes go through peer review. - # Killswitch: set CONCURRENT_PUSH_ENFORCEMENT=false to disable. - concurrent_push_enforcement = os.environ.get( - "CONCURRENT_PUSH_ENFORCEMENT", "true" - ).lower() not in ("false", "0", "no") - concurrent_mode = os.environ.get("EGG_CONCURRENT_MODE", "").lower() == "true" - if concurrent_push_enforcement and concurrent_mode: - session_pipeline_id = None - if hasattr(g, "session") and g.session: - session_pipeline_id = getattr(g.session, "pipeline_id", None) - if isinstance(session_pipeline_id, str) and session_pipeline_id: - if not data.get("consensus_push"): + # Push-target enforcement: in pipeline mode (non-concurrent or + # concurrent with the consensus_push marker set), agents must + # push only to their assigned branch. Prevents improvising + # branch names on push failure. + # Killswitch: set PUSH_TARGET_ENFORCEMENT=false to disable. + push_target_enforcement = os.environ.get("PUSH_TARGET_ENFORCEMENT", "true").lower() not in ( + "false", + "0", + "no", + ) + if push_target_enforcement and hasattr(g, "session") and g.session: + session_pipeline_id = getattr(g.session, "pipeline_id", None) + session_assigned_branch = getattr(g.session, "assigned_branch", None) + if isinstance(session_pipeline_id, str) and isinstance(session_assigned_branch, str): + if branch != session_assigned_branch: audit_log( - "push_denied_concurrent_mode", + "push_denied_wrong_branch", "git_push", success=False, details={ "repo": repo, "branch": branch, + "assigned_branch": session_assigned_branch, "pipeline_id": session_pipeline_id, - "reason": "Direct push blocked in concurrent mode", }, ) + hint = ( + " In BRC mode, call mcp__brc__propose — it handles " + "branch targeting for you." + if concurrent_mode + else "" + ) return make_error( - "Direct push blocked in concurrent mode. " - "Use: egg-orch consensus propose --push", + f"Pipeline sessions must push to their assigned branch " + f"'{session_assigned_branch}'. Got '{branch}'.{hint}", status_code=403, details={ - "mode": "concurrent", + "assigned_branch": session_assigned_branch, + "attempted_branch": branch, "pipeline_id": session_pipeline_id, - "requirement": "consensus_push", }, ) @@ -1374,7 +1388,7 @@ def git_push() -> tuple[Response, int] | Response: status_code=500, ) - def _inner_push() -> tuple[bool, str | None]: + def _inner_push(tip_sha: str) -> tuple[bool, str | None]: token_str, auth_mode, token_error = get_token_for_repo(repo) if not token_str: return False, token_error @@ -1382,7 +1396,12 @@ def _inner_push() -> tuple[bool, str | None]: push_args = ["push", "--no-verify"] if force: push_args.append("--force") - push_args.extend([push_target, refspec] if refspec else [push_target]) + # Push by SHA so we don't depend on a local + # refs/heads/ — sibling worktrees may hold a + # directory-style ref (e.g. refs/heads//work) + # that blocks creating the leaf ref. See #1994. + push_refspec = f"{tip_sha}:refs/heads/{branch}" + push_args.extend([push_target, push_refspec]) cmd_inner = git_cmd("-c", "http.extraheader=", *push_args) credential_helper_path_inner = None try: @@ -1656,6 +1675,12 @@ def _register(**kwargs: Any) -> bool: push_args = ["push", "--no-verify"] if force: push_args.append("--force") + # NOTE: The non-filtered push path uses the original refspec (not a + # SHA-based refspec) because it never calls ``update-ref`` pre-push, + # so the directory-style ref collision that affects the filtered path + # (sibling worktree refs like ``refs/heads//work``) does not + # apply here. The filtered path in ``filtered_push.py`` pushes by + # SHA to avoid that collision — see #1994 for context. push_args.extend([push_target, refspec] if refspec else [push_target]) # Clear any http.extraheader from .git/config to ensure the gateway's # credential helper (GIT_ASKPASS) is used. actions/checkout@v4 persists diff --git a/gateway/tests/test_concurrent_push_block.py b/gateway/tests/test_concurrent_push_block.py index f07e282640..84008ca89f 100644 --- a/gateway/tests/test_concurrent_push_block.py +++ b/gateway/tests/test_concurrent_push_block.py @@ -1,12 +1,14 @@ -"""Tests for concurrent-mode push blocking (#1669). +"""Tests for concurrent-mode push blocking (#1669, refined in #1994). When EGG_CONCURRENT_MODE=true, direct git pushes from pipeline agents must be -blocked. Agents must use `egg-orch consensus propose --push` instead, which -sets a `consensus_push` marker in the request payload. +blocked. Agents must use `mcp__brc__propose` (or the fallback CLI +`egg-orch consensus propose --push`), which sets a `consensus_push` marker +in the request payload. -The gateway enforces this AFTER the push-target enforcement and BEFORE the -branch ownership check. Infrastructure pushes (checkpoints, pipeline state) -are always exempt. +The gateway enforces this BEFORE the push-target enforcement so BRC agents +on per-role work branches see the actionable "use mcp__brc__propose" error +first rather than a misleading wrong-branch message (#1994). Infrastructure +pushes (checkpoints, pipeline state) are always exempt. Test scenarios: 1. Push blocked in concurrent mode without consensus_push marker @@ -173,8 +175,8 @@ def test_push_blocked_without_consensus_marker(self, client): assert response.status_code == 403 data = json.loads(response.data) assert data["success"] is False - assert "concurrent mode" in data["message"].lower() - assert "consensus" in data["message"].lower() + assert "brc mode" in data["message"].lower() + assert "mcp__brc__propose" in data["message"] def test_push_allowed_with_consensus_marker(self, client): """Push with consensus_push=true should be allowed in concurrent mode.""" @@ -308,7 +310,7 @@ def test_infrastructure_push_exempt(self, client): # concurrent-mode check specifically does not block it. response = _do_push(client, refspec=CHECKPOINT_BRANCH) assert response.status_code != 403 or ( - "concurrent mode" not in json.loads(response.data)["message"].lower() + "brc mode" not in json.loads(response.data)["message"].lower() ), "Infrastructure push should not be blocked by concurrent mode enforcement" @@ -364,7 +366,7 @@ def test_consensus_push_false_still_blocks(self, client): response = _do_push(client, consensus_push=False) assert response.status_code == 403 data = json.loads(response.data) - assert "concurrent mode" in data["message"].lower() + assert "brc mode" in data["message"].lower() def test_error_response_includes_details(self, client): """The 403 error should include mode, pipeline_id, and requirement details.""" @@ -394,7 +396,8 @@ def test_error_response_includes_details(self, client): assert "mode" in resp_data or "pipeline_id" in resp_data def test_error_message_suggests_consensus_propose(self, client): - """The 403 error message should mention 'egg-orch consensus propose --push'.""" + """The 403 error message should point at mcp__brc__propose (primary) + and list the CLI fallback (#1994).""" session = _make_session("coder") patches = _push_context(session) @@ -416,7 +419,9 @@ def test_error_message_suggests_consensus_propose(self, client): response = _do_push(client) assert response.status_code == 403 data = json.loads(response.data) + assert "mcp__brc__propose" in data["message"] assert "egg-orch consensus propose --push" in data["message"] + assert data.get("data", {}).get("recommended_tool") == "mcp__brc__propose" def test_concurrent_mode_not_set_allows_push(self, client): """When EGG_CONCURRENT_MODE is not set at all, push should not be blocked.""" diff --git a/gateway/tests/test_execute_filtered_push.py b/gateway/tests/test_execute_filtered_push.py index 22045edc9a..559d0ed018 100644 --- a/gateway/tests/test_execute_filtered_push.py +++ b/gateway/tests/test_execute_filtered_push.py @@ -132,15 +132,22 @@ def repo(tmp_path: Path) -> Path: class _PushStub: - """Callable passed as ``push_fn`` to execute_filtered_push.""" + """Callable passed as ``push_fn`` to execute_filtered_push. + + ``push_fn`` now receives the rewritten tip SHA as its only argument + (see #1994) so real callers can build a ``:refs/heads/`` + refspec without depending on a local ref. + """ def __init__(self, ok: bool = True, error: str | None = None) -> None: self.ok = ok self.error = error self.calls = 0 + self.last_tip: str | None = None - def __call__(self) -> tuple[bool, str | None]: + def __call__(self, tip_sha: str) -> tuple[bool, str | None]: self.calls += 1 + self.last_tip = tip_sha return self.ok, self.error @@ -194,6 +201,7 @@ def test_rewrite_strips_blocked_paths(self, repo: Path): AttributedFile(path="src/main.py", commit_sha=sha, authored_by="coder"), AttributedFile(path="docs/README.md", commit_sha=sha, authored_by="coder"), ] + push = _PushStub() result = execute_filtered_push( exec_path=str(repo), push_role="coder", @@ -201,7 +209,7 @@ def test_rewrite_strips_blocked_paths(self, repo: Path): attributed_commits=[sha], attributed_files=attributed_files, blocked_own_files={"docs/README.md"}, - push_fn=_PushStub(), + push_fn=push, registry_register=_RegistryStub(), pipeline_id="issue-1882", repo="owner/repo", @@ -214,6 +222,9 @@ def test_rewrite_strips_blocked_paths(self, repo: Path): new_sha = result.rewritten_commits[0]["new_sha"] assert new_sha != sha assert result.pushed_commits == [new_sha] + # push_fn must receive the rewritten tip SHA so callers can build + # a :refs/heads/ refspec (#1994). + assert push.last_tip == new_sha # The marker is emitted as a proper git trailer so trailers # parse cleanly. new_message = _message(repo, new_sha) @@ -506,6 +517,9 @@ def test_rollback_restores_original_head_and_tree(self, repo: Path): assert result.success is False assert result.error and "rejected" in result.error assert push.calls == 1 + # push_fn must receive a valid rewritten SHA even on push failure. + assert push.last_tip is not None + assert push.last_tip != sha # rewritten tip, not the original # Branch ref must be back at the original HEAD (via _rollback) assert _run(repo, "rev-parse", "refs/heads/egg/issue-1882") == sha diff --git a/sandbox/agent-config/rules/mission.md b/sandbox/agent-config/rules/mission.md index 31e572b89d..669ac4e1b0 100644 --- a/sandbox/agent-config/rules/mission.md +++ b/sandbox/agent-config/rules/mission.md @@ -63,7 +63,7 @@ gh pr create --head egg/ --title "Brief description" --body "..." - - **Push before long-running operations** — test suites, sub-agent spawns, or anything that could consume remaining turns. - **For multi-phase plans**: commit and push after completing each phase before starting the next. Never batch all phases into a single final commit. - **Update the contract as you go**: after each commit, mark the task done with `egg-contract complete-task --task --commit `. After completing all tasks in a phase, mark the phase done with `egg-contract complete-phase --phase --commit `. -- **In concurrent/BRC mode**: direct `git push` is **blocked by the gateway**. You **must** use `egg-orch consensus propose --push` to push changes, which bundles the push with a consensus proposal. This ensures all modifications go through the BRC peer review protocol and prevents redundant auto-repropose cycles. +- **In concurrent/BRC mode**: direct `git push` is **blocked by the gateway**. Commit locally, then call `mcp__brc__propose` — it pushes your commits to origin via the gateway and sends `CONSENSUS_PROPOSE` in one step (push is on by default; pass `push=false` only if you have already pushed through another route). Fallback CLI: `egg-orch consensus propose --push`. Do not improvise refspec variants of `git push` when it fails — the gateway's error message will point you at the right tool. **If push/PR fails**: Notify user via Slack with branch name, repo, and summary. diff --git a/sandbox/egg_agent_tools/push.py b/sandbox/egg_agent_tools/push.py new file mode 100644 index 0000000000..9088e21f71 --- /dev/null +++ b/sandbox/egg_agent_tools/push.py @@ -0,0 +1,159 @@ +"""Shared BRC consensus-push helper. + +This module exposes :func:`consensus_push` — the single implementation +of "push code to the gateway with the ``consensus_push`` marker" shared +between the ``egg-orch consensus propose --push`` CLI shim and the +``mcp__brc__propose`` MCP tool wrapper. Both surfaces MUST route through +this helper so the gateway receives the marker and permits the push in +concurrent mode. + +The actual ``git push`` happens inside the gateway process, which holds +the GitHub App credentials. The agent's sandbox does not authenticate +to origin directly; this helper only hits ``POST /api/v1/git/push`` on +the gateway sidecar. + +Extracted from ``sandbox/egg_lib/orch_cli.py:_consensus_push`` for #1994 +so MCP-only agents (which cannot shell out to the CLI) can publish BRC +artifacts. +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +import urllib.error +import urllib.request + + +def consensus_push() -> tuple[int, str | None]: + """Push code via the gateway with the ``consensus_push`` marker set. + + Calls the gateway push API directly (instead of ``git push``) so the + ``consensus_push`` flag is included in the JSON payload. This lets + the gateway distinguish consensus-protocol pushes from direct pushes + in concurrent mode. + + When ``GATEWAY_URL`` is unset (local development, no gateway + running) we fall back to plain ``git push`` — concurrent-mode + enforcement does not apply in that path because the gateway isn't + present to enforce it. + + Returns ``(0, None)`` on success or ``(1, error_message)`` on failure. + The error message includes the specific reason so MCP callers (where + stderr is not visible to the agent) can surface it in HandlerError. + """ + # Late import to avoid any risk of circular import between the + # ``egg_lib`` CLI layer and the ``egg_agent_tools`` MCP layer. + from egg_lib.cli_push import _retarget_refspec + + repo_path = os.environ.get("EGG_REPO_PATH", "") + gateway_url = os.environ.get("GATEWAY_URL", "") + session_token = os.environ.get("EGG_SESSION_TOKEN", "") + container_id = os.environ.get("CONTAINER_ID", "") + + if not gateway_url: + # Fallback: plain git push when the gateway is not reachable + # (e.g. local development). No concurrent-mode enforcement + # exists in this path. + try: + subprocess.check_output( + ["git", "push"], + text=True, + cwd=repo_path or None, + stderr=subprocess.STDOUT, + ) + return 0, None + except subprocess.CalledProcessError as e: + msg = f"git push failed: {e.output.strip()}" + print(f"Error: {msg}", file=sys.stderr) + return 1, msg + except FileNotFoundError: + msg = "git not found" + print(f"Error: {msg}", file=sys.stderr) + return 1, msg + + try: + branch = subprocess.check_output( + ["git", "branch", "--show-current"], + text=True, + cwd=repo_path or None, + stderr=subprocess.DEVNULL, + ).strip() + except (subprocess.CalledProcessError, FileNotFoundError): + branch = "" + + if not branch: + msg = "could not determine current branch for push" + print(f"Error: {msg}", file=sys.stderr) + return 1, msg + + # Retarget to the assigned pipeline branch when on a per-agent work + # branch (shared logic with ``cli_push``). + retarget = _retarget_refspec(branch) + if retarget: + refspec = retarget + else: + try: + tracking = subprocess.check_output( + ["git", "config", f"branch.{branch}.merge"], + text=True, + cwd=repo_path or None, + stderr=subprocess.DEVNULL, + ).strip() + remote_branch = tracking.removeprefix("refs/heads/") + refspec = f"{branch}:{remote_branch}" if remote_branch != branch else branch + except (subprocess.CalledProcessError, FileNotFoundError): + refspec = branch + + payload = json.dumps( + { + "repo_path": repo_path, + "remote": "origin", + "refspec": refspec, + "force": False, + "container_id": container_id, + "consensus_push": True, + } + ).encode() + + req = urllib.request.Request( + f"{gateway_url}/api/v1/git/push", + data=payload, + headers={ + "Content-Type": "application/json", + "Authorization": f"Bearer {session_token}", + }, + method="POST", + ) + + try: + with urllib.request.urlopen(req, timeout=120) as resp: + body = json.loads(resp.read()) + stdout = body.get("data", {}).get("stdout", "") + stderr = body.get("data", {}).get("stderr", "") + if stdout: + print(stdout) + if stderr: + print(stderr, file=sys.stderr) + return 0, None + except urllib.error.HTTPError as e: + try: + body = json.loads(e.read()) + msg = body.get("message", "Unknown error") + details = body.get("data", {}) + except Exception: + msg = f"HTTP {e.code}" + details = {} + detail_str = f" ({json.dumps(details)})" if details else "" + full_msg = f"git push failed: {msg}{detail_str}" + print(f"Error: {full_msg}", file=sys.stderr) + return 1, full_msg + except urllib.error.URLError as e: + msg = f"gateway unreachable: {e.reason}" + print(f"Error: {msg}", file=sys.stderr) + return 1, msg + + +__all__ = ["consensus_push"] diff --git a/sandbox/egg_agent_tools/tools/brc.py b/sandbox/egg_agent_tools/tools/brc.py index 01407826d3..5aad9898b0 100644 --- a/sandbox/egg_agent_tools/tools/brc.py +++ b/sandbox/egg_agent_tools/tools/brc.py @@ -5,6 +5,7 @@ from typing import Any from egg_agent_tools.handlers import brc as handlers +from egg_agent_tools.handlers.errors import HandlerError from egg_agent_tools.tools._common import invoke_handler from egg_agent_tools.tools._registry import ToolRegistration from egg_agent_tools.tools._tool_compat import tool @@ -52,6 +53,17 @@ "items": {"type": "string"}, "description": "Re-proposal delta (changed artifact references)", }, + "push": { + "type": "boolean", + "default": True, + "description": ( + "Push committed changes to origin via the gateway before " + "sending the proposal (default true). Required in BRC " + "mode — reviewers pull from origin, so an un-pushed " + "artifact is invisible to them. Pass false only if you " + "have already pushed through another route." + ), + }, "pipeline_id": {"type": "string"}, "role": {"type": "string"}, }, @@ -163,12 +175,34 @@ @tool( "propose", - "Send a CONSENSUS_PROPOSE signal starting or re-starting the BRC cycle for " - "this producer. Prefer this over 'egg-orch consensus propose'.", + "Send a CONSENSUS_PROPOSE signal starting or re-starting the BRC cycle " + "for this producer. Pushes your committed changes to origin via the " + "gateway first (disable with push=false). Prefer this over " + "'egg-orch consensus propose --push'.", _PROPOSE_SCHEMA, ) async def brc_propose(args: dict[str, Any]) -> dict[str, Any]: - return await invoke_handler(handlers.brc_propose, args) + from egg_agent_tools.push import consensus_push + + # Strip the MCP-only ``push`` flag before handing the dict to the + # handler so its schema stays clean. Default: push (BRC requires + # the artifact on origin for reviewers to pull). + inbound = dict(args or {}) + should_push = bool(inbound.pop("push", True)) + + def _push_then_propose(handler_args: dict[str, Any]) -> dict[str, Any]: + if should_push: + rc, err = consensus_push() + if rc != 0: + raise HandlerError( + f"Push to origin failed: {err or 'unknown error'}; " + "CONSENSUS_PROPOSE not sent. Fix the push error first, " + "then retry mcp__brc__propose. Pass push=false only if " + "you have already pushed through another route." + ) + return handlers.brc_propose(handler_args) + + return await invoke_handler(_push_then_propose, inbound) @tool( diff --git a/sandbox/egg_lib/orch_cli.py b/sandbox/egg_lib/orch_cli.py index b52db190c6..1acf8ef12a 100755 --- a/sandbox/egg_lib/orch_cli.py +++ b/sandbox/egg_lib/orch_cli.py @@ -50,7 +50,6 @@ import json import os import re -import subprocess import sys from typing import Any from urllib.error import HTTPError, URLError @@ -66,8 +65,6 @@ ORCHESTRATOR_PORT = 9849 # noqa: EGG002 GATEWAY_PORT = 9848 # noqa: EGG002 -from egg_lib.cli_push import _retarget_refspec - # Validation pattern for IDs used in URL path segments _SAFE_ID_PATTERN = re.compile(r"^[a-zA-Z0-9_\-\.]+$") @@ -1500,123 +1497,19 @@ def cmd_signal_readiness(args: argparse.Namespace) -> int: def _consensus_push() -> int: - """Push code via the gateway with the consensus_push marker. + """Back-compat alias for :func:`egg_agent_tools.push.consensus_push`. - Calls the gateway push API directly (instead of ``git push``) so that - the ``consensus_push`` flag is included in the JSON payload. This - allows the gateway to distinguish consensus-protocol pushes from - direct pushes in concurrent mode. + The implementation moved to ``egg_agent_tools.push`` in #1994 so the + ``mcp__brc__propose`` tool can share it. Kept here as a thin alias + so existing CLI callers and unit tests keep working. - Returns 0 on success, 1 on failure. + Returns only the exit code (discards error message) — the CLI + surfaces errors via stderr prints inside ``consensus_push()``. """ - import urllib.error - import urllib.request - - repo_path = os.environ.get("EGG_REPO_PATH", "") - gateway_url = os.environ.get("GATEWAY_URL", "") - session_token = os.environ.get("EGG_SESSION_TOKEN", "") - container_id = os.environ.get("CONTAINER_ID", "") - - if not gateway_url: - # Fallback: use plain git push when gateway URL is not set - # (e.g. local development). No concurrent-mode enforcement exists - # in this path — the gateway is not running to enforce it. - try: - subprocess.check_output( - ["git", "push"], - text=True, - cwd=repo_path or None, - stderr=subprocess.STDOUT, - ) - return 0 - except subprocess.CalledProcessError as e: - print(f"Error: git push failed: {e.output.strip()}", file=sys.stderr) - return 1 - except FileNotFoundError: - print("Error: git not found", file=sys.stderr) - return 1 - - # Resolve refspec: current branch tracking info or just the branch name. - try: - branch = subprocess.check_output( - ["git", "branch", "--show-current"], - text=True, - cwd=repo_path or None, - stderr=subprocess.DEVNULL, - ).strip() - except (subprocess.CalledProcessError, FileNotFoundError): - branch = "" - - if not branch: - print("Error: could not determine current branch for push", file=sys.stderr) - return 1 - - # Retarget to the assigned pipeline branch when on a per-agent work - # branch (shared logic with cli_push). - retarget = _retarget_refspec(branch) - if retarget: - refspec = retarget - else: - # Resolve the remote tracking refspec (e.g. local:remote) - try: - tracking = subprocess.check_output( - ["git", "config", f"branch.{branch}.merge"], - text=True, - cwd=repo_path or None, - stderr=subprocess.DEVNULL, - ).strip() - # tracking is like "refs/heads/egg/issue-123" - remote_branch = tracking.removeprefix("refs/heads/") - refspec = f"{branch}:{remote_branch}" if remote_branch != branch else branch - except (subprocess.CalledProcessError, FileNotFoundError): - refspec = branch - - payload = json.dumps( - { - "repo_path": repo_path, - "remote": "origin", - "refspec": refspec, - "force": False, - "container_id": container_id, - "consensus_push": True, - } - ).encode() - - req = urllib.request.Request( - f"{gateway_url}/api/v1/git/push", - data=payload, - headers={ - "Content-Type": "application/json", - "Authorization": f"Bearer {session_token}", - }, - method="POST", - ) + from egg_agent_tools.push import consensus_push as _impl - try: - with urllib.request.urlopen(req, timeout=120) as resp: - body = json.loads(resp.read()) - stdout = body.get("data", {}).get("stdout", "") - stderr = body.get("data", {}).get("stderr", "") - if stdout: - print(stdout) - if stderr: - print(stderr, file=sys.stderr) - return 0 - except urllib.error.HTTPError as e: - try: - body = json.loads(e.read()) - msg = body.get("message", "Unknown error") - details = body.get("data", {}) - except Exception: - msg = f"HTTP {e.code}" - details = {} - print(f"Error: git push failed: {msg}", file=sys.stderr) - if details: - print(f"Details: {json.dumps(details)}", file=sys.stderr) - return 1 - except urllib.error.URLError as e: - print(f"Error: gateway unreachable: {e.reason}", file=sys.stderr) - return 1 + rc, _err = _impl() + return rc def cmd_consensus_propose(args: argparse.Namespace) -> int: diff --git a/tests/sandbox/egg_agent_tools/test_tools.py b/tests/sandbox/egg_agent_tools/test_tools.py index ecd1be9acb..65fb6dfaf6 100644 --- a/tests/sandbox/egg_agent_tools/test_tools.py +++ b/tests/sandbox/egg_agent_tools/test_tools.py @@ -212,6 +212,98 @@ def test_register_open_question_wrapper_error(self): assert "fail" in resp["content"][0]["text"] +class TestBrcProposePushStep: + """The ``mcp__brc__propose`` wrapper pushes to origin before sending + CONSENSUS_PROPOSE (default push=True). The CLI ``egg-orch consensus + propose --push`` shares the helper; wiring the push into the wrapper + closes #1994 so MCP-only agents can publish artifacts. + """ + + _ARGS = { + "pipeline_id": "p1", + "role": "refiner", + "summary": "x" * 60, + } + + def _wrapper(self): + reg = TOOL_REGISTRY["mcp__brc__propose"] + handler = getattr(reg.sdk_tool, "handler", None) + assert handler is not None + return handler + + def test_push_true_invokes_consensus_push_then_handler(self): + order: list[str] = [] + + def _push(): + order.append("push") + return 0, None + + def _handler(req): + order.append("handler") + # The MCP-only ``push`` flag must be stripped before the + # handler sees the dict. + assert "push" not in req + return {"ok": True, "signal": {"data": {}}, "phase": "refine"} + + with ( + patch("egg_agent_tools.push.consensus_push", side_effect=_push), + patch("egg_agent_tools.handlers.brc.brc_propose", side_effect=_handler), + ): + resp = _run(self._wrapper()(dict(self._ARGS))) + + assert order == ["push", "handler"] + assert "is_error" not in resp + body = json.loads(resp["content"][0]["text"]) + assert body["ok"] is True + + def test_push_false_skips_consensus_push(self): + push_calls = {"n": 0} + + def _push(): + push_calls["n"] += 1 + return 0, None + + def _handler(req): + assert "push" not in req + return {"ok": True, "signal": {"data": {}}} + + args = {**self._ARGS, "push": False} + with ( + patch("egg_agent_tools.push.consensus_push", side_effect=_push), + patch("egg_agent_tools.handlers.brc.brc_propose", side_effect=_handler), + ): + resp = _run(self._wrapper()(args)) + + assert push_calls["n"] == 0 + assert "is_error" not in resp + + def test_push_failure_short_circuits_and_returns_error(self): + """If consensus_push fails the handler must NOT fire (we don't + want to broadcast a PROPOSE for an artifact that never landed + on origin).""" + handler_calls = {"n": 0} + + def _push(): + return 1, "HTTP 403: branch ownership check failed" + + def _handler(req): + handler_calls["n"] += 1 + return {"ok": True} + + with ( + patch("egg_agent_tools.push.consensus_push", side_effect=_push), + patch("egg_agent_tools.handlers.brc.brc_propose", side_effect=_handler), + ): + resp = _run(self._wrapper()(dict(self._ARGS))) + + assert handler_calls["n"] == 0 + assert resp["is_error"] is True + error_text = resp["content"][0]["text"] + assert "Push to origin failed" in error_text + # The specific error reason must be surfaced (not just "see gateway logs") + assert "branch ownership check failed" in error_text + + class TestMessagePrimitiveWrappers: """Event-driven wrappers added in #1922 (wait_for_event / wait_loop / send_heartbeat) also return JSON-serialised responses on success and diff --git a/tests/sandbox/test_orch_cli_consensus_push.py b/tests/sandbox/test_orch_cli_consensus_push.py index 9012e9eb33..09e8583a8a 100644 --- a/tests/sandbox/test_orch_cli_consensus_push.py +++ b/tests/sandbox/test_orch_cli_consensus_push.py @@ -123,7 +123,7 @@ def mock_check_output(cmd, **kwargs): return "" with ( - patch("egg_lib.orch_cli.subprocess.check_output", side_effect=mock_check_output), + patch("egg_agent_tools.push.subprocess.check_output", side_effect=mock_check_output), patch("urllib.request.urlopen", side_effect=mock_urlopen), ): result = _consensus_push() @@ -154,7 +154,7 @@ def mock_check_output(cmd, **kwargs): with ( patch.dict(os.environ, {"EGG_BRANCH": ""}), - patch("egg_lib.orch_cli.subprocess.check_output", side_effect=mock_check_output), + patch("egg_agent_tools.push.subprocess.check_output", side_effect=mock_check_output), patch("urllib.request.urlopen", side_effect=mock_urlopen), ): result = _consensus_push() @@ -182,7 +182,7 @@ def mock_check_output(cmd, **kwargs): with ( patch.dict(os.environ, {"EGG_BRANCH": ""}), - patch("egg_lib.orch_cli.subprocess.check_output", side_effect=mock_check_output), + patch("egg_agent_tools.push.subprocess.check_output", side_effect=mock_check_output), patch("urllib.request.urlopen", side_effect=mock_urlopen), ): result = _consensus_push() @@ -209,7 +209,7 @@ def mock_check_output(cmd, **kwargs): with ( patch.dict(os.environ, {"EGG_BRANCH": "egg/issue-1669"}), - patch("egg_lib.orch_cli.subprocess.check_output", side_effect=mock_check_output), + patch("egg_agent_tools.push.subprocess.check_output", side_effect=mock_check_output), patch("urllib.request.urlopen", side_effect=mock_urlopen), ): result = _consensus_push() @@ -237,7 +237,7 @@ def mock_check_output(cmd, **kwargs): with ( patch.dict(os.environ, {"EGG_BRANCH": "egg/issue-1669"}), - patch("egg_lib.orch_cli.subprocess.check_output", side_effect=mock_check_output), + patch("egg_agent_tools.push.subprocess.check_output", side_effect=mock_check_output), patch("urllib.request.urlopen", side_effect=mock_urlopen), ): result = _consensus_push() @@ -264,7 +264,7 @@ def mock_check_output(cmd, **kwargs): return "" with ( - patch("egg_lib.orch_cli.subprocess.check_output", side_effect=mock_check_output), + patch("egg_agent_tools.push.subprocess.check_output", side_effect=mock_check_output), patch("urllib.request.urlopen", side_effect=mock_urlopen), ): result = _consensus_push() @@ -293,7 +293,7 @@ def mock_check_output(cmd, **kwargs): ) with ( - patch("egg_lib.orch_cli.subprocess.check_output", side_effect=mock_check_output), + patch("egg_agent_tools.push.subprocess.check_output", side_effect=mock_check_output), patch("urllib.request.urlopen", side_effect=http_error), ): result = _consensus_push() @@ -310,7 +310,7 @@ def mock_check_output(cmd, **kwargs): return "" with ( - patch("egg_lib.orch_cli.subprocess.check_output", side_effect=mock_check_output), + patch("egg_agent_tools.push.subprocess.check_output", side_effect=mock_check_output), patch( "urllib.request.urlopen", side_effect=urllib.error.URLError("Connection refused"), @@ -327,7 +327,7 @@ def mock_check_output(cmd, **kwargs): raise subprocess.CalledProcessError(1, cmd) return "" - with patch("egg_lib.orch_cli.subprocess.check_output", side_effect=mock_check_output): + with patch("egg_agent_tools.push.subprocess.check_output", side_effect=mock_check_output): result = _consensus_push() assert result == 1 @@ -350,7 +350,7 @@ def mock_check_output(cmd, text=False, cwd=None, stderr=None, env=None): return "Everything up-to-date" return "" - with patch("egg_lib.orch_cli.subprocess.check_output", side_effect=mock_check_output): + with patch("egg_agent_tools.push.subprocess.check_output", side_effect=mock_check_output): result = _consensus_push() assert result == 0 # Fallback should not pass a custom env (no EGG_CONSENSUS_PUSH) @@ -359,7 +359,7 @@ def mock_check_output(cmd, text=False, cwd=None, stderr=None, env=None): def test_fallback_push_failure_returns_1(self, no_gateway_env): """Fallback git push CalledProcessError should return 1.""" with patch( - "egg_lib.orch_cli.subprocess.check_output", + "egg_agent_tools.push.subprocess.check_output", side_effect=subprocess.CalledProcessError(1, "git push", output="remote rejected"), ): result = _consensus_push() @@ -368,7 +368,7 @@ def test_fallback_push_failure_returns_1(self, no_gateway_env): def test_fallback_git_not_found_returns_1(self, no_gateway_env): """Fallback when git binary not found should return 1.""" with patch( - "egg_lib.orch_cli.subprocess.check_output", + "egg_agent_tools.push.subprocess.check_output", side_effect=FileNotFoundError, ): result = _consensus_push() diff --git a/tests/sandbox/test_orch_client.py b/tests/sandbox/test_orch_client.py index 8ce1910c09..4b79c08a23 100644 --- a/tests/sandbox/test_orch_client.py +++ b/tests/sandbox/test_orch_client.py @@ -717,7 +717,7 @@ def test_push_git_not_found_returns_1(self, mock_push): assert rc == 1 @patch("egg_agent_tools.handlers.brc.orchestrator_request") - @patch("egg_lib.orch_cli.subprocess.check_output") + @patch("egg_agent_tools.push.subprocess.check_output") def test_no_push_flag_skips_git_push(self, mock_subprocess, mock_request): """Without --push, git push is not called (only rev-parse for commit).