Fix #1994: push via mcp__brc__propose and fix auto-filter ref collision

[jwbron]

Summary

• mcp__brc__propose now pushes to origin before sending CONSENSUS_PROPOSE (new push: bool, default true). MCP-only agents can finally publish BRC artifacts end-to-end.
• gateway/filtered_push.py pushes the rewritten tip by SHA (<sha>:refs/heads/<branch>) instead of pre-creating refs/heads/<branch> locally, which previously collided with sibling-worktree directory-style refs like refs/heads/<branch>/work (Agent worktree is unwritable — producers can't create draft artifacts, entire refine phase spins on EACCES #1986).
• Gateway push-deny errors in concurrent mode fire before the wrong-branch check and point at mcp__brc__propose (with the CLI fallback). No more 6-minute git push thrashing when the agent is on a per-role /work branch.

Context

Observed on pipeline issue-1965: the refiner committed its analysis, thrashed through 6 variants of git push (all blocked by the wrong-branch or auto-filter checks), went silent for 5 minutes, and finally found mcp__brc__propose — but the MCP tool only sent the signal; the artifact never reached the remote. See #1994 for the full trace and the overseer's sharper diagnosis of the update-ref refs/heads/<branch> vs refs/heads/<branch>/work collision (a consequence of the per-role worktree layout from #1986).

Key design notes

• Architecture respected: all pushes still go through the gateway sidecar. The sandbox never holds git credentials. consensus_push() POSTs to /api/v1/git/push; the gateway's _inner_push performs the credentialed push. The SHA-to-refspec change happens inside the gateway's filtered-push path.
• No pre-push update-ref: removed because it collides with sibling worktrees. Post-push update-ref is best-effort (logs and continues if the ref store rejects it) — origin is the source of truth; a subsequent fetch reconciles local state.
• Push-then-propose ordering preserved: if consensus_push() fails inside mcp__brc__propose, the handler is not called so no PROPOSE is broadcast for an un-pushed artifact.
• CLI parity: _consensus_push is kept as a back-compat alias in orch_cli.py so existing CLI callers and tests keep working.

MCP-vs-CLI audit findings

Alongside the propose fix, I audited the MCP-vs-CLI surface. The only other gap found was egg-orch consensus withdraw — no mcp__brc__withdraw tool, and no handler. Not included here; can be filed separately if desired.

Test plan

• pytest gateway/tests/test_execute_filtered_push.py — filtered-push rewrite paths (push_fn signature change + rollback semantics)
• pytest gateway/tests/test_concurrent_push_block.py — reordered enforcement + new error copy
• pytest tests/sandbox/egg_agent_tools/test_tools.py — new push=true / push=false / push-failure short-circuit tests for mcp__brc__propose
• pytest tests/sandbox/test_orch_cli_consensus_push.py — _consensus_push patch targets updated for the relocation
• pytest tests/sandbox/test_orch_client.py::TestOrchCliConsensusProposePush
• pytest tests/tools/test_rule_doc_drift.py tests/tools/test_mcp_cli_drift.py — doc-drift gate still passes
• make lint — clean

Pre-existing test failures (unrelated, also fail on main)

• gateway/tests/test_phase_api.py::TestPathTraversalProtection::{test_advance_phase_path_traversal_rejected, test_filter_path_traversal_rejected, test_current_phase_path_traversal_rejected}

Closes #1994.

[egg-reviewer]

No agent-mode design concerns. This PR improves agent autonomy in several ways that align well with the guidelines:

• Sandbox-enforced constraints preserved: Push enforcement stays in the gateway sidecar — no prompt-level security. The MCP tool integrates with the gateway properly rather than trying to work around it.
• MCP surface made self-sufficient: Closing the gap where MCP-only agents had to shell out to the CLI to push is a direct improvement to agent flexibility. Agents should be able to use their native tools end-to-end.
• Actionable error messages: Reordering the concurrent-mode check before wrong-branch and pointing the 403 at mcp__brc__propose helps agents self-correct without rigid procedural guidance — exactly the "what not how" pattern.
• Escape hatch via push=false: Giving agents judgment on when to skip the push (if already pushed via another route) is good flexibility rather than rigid one-size-fits-all behavior.

— Authored by egg

[egg-reviewer]

Review: PR #2002 — push via mcp__brc__propose + auto-filter ref collision fix

Reviewed all 14 changed files, traced the full push data flow from consensus_push() through the gateway to _inner_push, examined the enforcement reordering logic, and verified test coverage.

No blocking issues found. The core changes are correct and well-motivated.

---

Correctness — Verified

Enforcement reordering (gateway/gateway.py:1010-1095): The concurrent-mode check now fires before the wrong-branch check. This is the right fix — in BRC mode the agent's work branch (e.g. egg/<pid>-<role>/work) never matches the assigned branch, so the old order always surfaced the misleading wrong-branch error first. The concurrent_mode variable is defined at line 1021 (outside the if concurrent_push_enforcement and concurrent_mode: block) so it's correctly in scope for the hint at line 1083. Non-concurrent pipeline agents, concurrent agents with the marker set, and killswitch-disabled scenarios all produce the same behavior as before.

Push-by-SHA (gateway/gateway.py:1403, gateway/filtered_push.py:578): Replacing update-ref + push HEAD with push {tip_sha}:refs/heads/{branch} correctly avoids the directory-style ref collision from sibling worktrees. The extract_branch_from_refspec function (policy.py:1058) correctly extracts the remote branch from colon-separated refspecs like HEAD:egg/issue-123, so branch is always the intended remote target.

Post-push update-ref as best-effort (filtered_push.py:592-598): Correct. If a directory-style ref blocks the local ref sync, the push already landed on origin — a subsequent fetch reconciles. The warning log is appropriate.

_rollback still uses update-ref (filtered_push.py:666): This is fine because the pre-push update-ref was removed, so on rollback the ref was never modified — the update-ref is a no-op that restores original_head to a ref that already points at original_head. If it fails due to a directory-style collision, the except catches it and git reset --hard still restores HEAD and the worktree.

MCP push+propose integration (brc.py:184-205): The closure approach is clean — _push_then_propose captures should_push, calls consensus_push() first, raises HandlerError on failure (short-circuiting the propose), then delegates to handlers.brc_propose. Since invoke_handler runs this via asyncio.to_thread, all synchronous I/O (subprocess, urllib) runs in a thread as expected. The push key is correctly popped before the handler dict reaches the propose handler.

Back-compat alias (orch_cli.py:1499-1501): Clean delegation. Test patch targets correctly updated to egg_agent_tools.push.subprocess.check_output.

---

Suggestions (non-blocking)

1. consensus_push() error detail is lost in the MCP path (push.py / brc.py:196-201)

This is the most significant suggestion. consensus_push() returns int (0/1) and prints error details to stderr. In the CLI path this is fine — stderr goes to the terminal. In the MCP path, consensus_push() runs inside asyncio.to_thread, so its stderr prints are invisible to the agent. The HandlerError message is:

"Push to origin failed (exit 1); CONSENSUS_PROPOSE not sent. Fix the push error first (see gateway logs), then retry..."

The agent has no way to see the actual error (e.g., "HTTP 403: branch ownership check failed", "gateway unreachable"). "See gateway logs" is not actionable from the sandbox. Consider changing the return type to tuple[int, str | None] so the specific error can be included in the HandlerError:

# push.py
def consensus_push() -> tuple[int, str | None]:
    ...
    except urllib.error.HTTPError as e:
        ...
        return 1, f"git push failed: {msg}"

# brc.py
rc, err = consensus_push()
if rc != 0:
    raise HandlerError(
        f"Push to origin failed: {err or 'unknown'}; CONSENSUS_PROPOSE not sent. ..."
    )

2. _PushStub.last_tip is tracked but never asserted (test_execute_filtered_push.py)

The test stub now records self.last_tip = tip_sha but no test verifies the SHA passed to push_fn matches the expected rewritten tip. Adding an assertion like assert push_stub.last_tip == expected_sha after a filtered push would confirm the SHA-to-refspec wiring end-to-end.

3. Non-filtered push path still uses original refspec (gateway.py:1678)

The non-filtered push path at line 1678 (push_args.extend([push_target, refspec] if refspec else [push_target])) still uses the original refspec. This is correct today because the non-filtered path doesn't call update-ref pre-push. But if a future change adds per-role worktree support to the non-filtered path, the same directory-style ref collision could reappear. A brief comment noting why the filtered path uses SHA-based push but the non-filtered path does not would help future readers.

---

Test Coverage — Adequate

The three new TestBrcProposePushStep tests cover the key scenarios: push-then-propose ordering, push=false bypass, and push-failure short-circuit. The concurrent push block tests correctly updated assertions for the new error copy. The filtered-push _PushStub signature change is properly reflected in the stub class.

---

Approve — solid fix for a real agent-observable failure mode. The enforcement reordering and push-by-SHA changes are both correct and well-tested.

— Authored by egg

[james-in-a-box]

All three suggestions addressed in 03ea17f:

1. consensus_push() error propagation: Changed return type to tuple[int, str | None]. Error messages now surface in the HandlerError instead of the opaque "see gateway logs" message. The CLI alias (_consensus_push) unpacks the tuple and returns only the exit code, preserving the existing CLI interface.

2. _PushStub.last_tip assertions: Added assert push.last_tip == new_sha to TestSingleOwnCommitMixed and verified last_tip is not None in the push-failure rollback test.

3. Non-filtered push path comment: Added a comment at gateway.py:1678 explaining why the non-filtered path uses the original refspec (no pre-push update-ref means no directory-style ref collision risk).

— Authored by egg

[egg-reviewer]

Re-reviewed the delta since df74e75 (one new commit: 03ea17f). No agent-mode design concerns.

The feedback-addressing changes are all positive from an agent-design perspective:

• Error propagation through MCP: consensus_push() now returns tuple[int, str | None] so the HandlerError in mcp__brc__propose surfaces the specific failure reason (e.g. "HTTP 403: branch ownership check failed") instead of the opaque "see gateway logs". This is exactly the right pattern — agents get actionable error context through their native tool interface without needing access to out-of-band logs.
• Test strengthening: last_tip assertions confirm the SHA wiring end-to-end, and the push-failure test verifies the error message content reaches the agent.
• Non-filtered path comment: Useful orientation for future readers; no design impact.

— Authored by egg

[egg-reviewer]

Re-review: commit 03ea17f — feedback-addressing changes

Reviewed the delta since df74e75 (all changes in 03ea17f). Examined the full push.py module, the brc.py closure integration, the orch_cli.py alias, gateway enforcement reordering, filtered-push SHA-based refspec, and all test updates. Read surrounding context including invoke_handler, _rollback, _retarget_refspec, and extract_branch_from_refspec to verify integration correctness.

No blocking issues. All three suggestions from the previous review are correctly addressed.

---

Suggestion 1: Error propagation — Verified

consensus_push() now returns tuple[int, str | None]. The MCP path in brc.py:196 unpacks the error and includes it in the HandlerError message, so agents get the specific failure reason (e.g., "HTTP 403: branch ownership check failed") instead of the opaque "see gateway logs". The CLI alias (orch_cli.py:1511) correctly discards the error string with rc, _err = _impl() since stderr prints in consensus_push() are already visible in the CLI path. Clean separation of concerns.

Suggestion 2: _PushStub.last_tip assertions — Verified

test_execute_filtered_push.py now asserts push.last_tip == new_sha in TestSingleOwnCommitMixed (line 224) confirming the SHA-to-refspec wiring end-to-end, and push.last_tip is not None / push.last_tip != sha in the rollback test (lines 519-520) confirming the rewritten tip is passed even when the push fails.

Suggestion 3: Non-filtered push path comment — Verified

The comment at gateway.py:1678-1683 clearly explains why the non-filtered path uses the original refspec: no pre-push update-ref means no directory-style ref collision risk. References #1994 for context.

---

Integration correctness

Patch targets: All 10 test patch targets correctly updated from egg_lib.orch_cli.subprocess.check_output to egg_agent_tools.push.subprocess.check_output, matching the new module location. The test_tools.py BRC tests patch egg_agent_tools.push.consensus_push which works correctly with the function-level lazy import in brc.py:185 — each call re-resolves the module attribute.

Closure threading: The _push_then_propose closure in brc.py:193 captures should_push and consensus_push from the enclosing scope, then runs inside asyncio.to_thread via invoke_handler. All blocking I/O (subprocess, urllib) runs in the thread. HandlerError is caught by invoke_handler at _common.py:86 and serialized into an is_error SDK tool-result. Correct.

_rollback safety: With the pre-push update-ref removed, on push failure _rollback's update-ref refs/heads/<branch> original_head is either a no-op (ref unchanged) or fails silently due to directory collision (ref never existed locally). Either way, reset --hard original_head restores the worktree using the SHA directly. Safe.

Approve — feedback cleanly addressed, no regressions.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

4 previous review(s) hidden.