Fix #2080: brc_confirm returns ok=False on pending_acks rejection

[jwbron]

Summary

• mcp__brc__confirm now sets ok: False when the orchestrator rejects the confirm (status == "pending_acks"); ok: True is reserved for the producer actually transitioning to CONFIRMED.
• Structured response (status, message, consensus_reached, signal) is unchanged, so callers can still branch on the rejection reason without exception handling — pending_acks is a normal protocol state, not a hard failure.
• Agent rule and handler docstring updated so the new semantics are reflected in instructions, not just code.

Why

Per #2080: returning {ok: True, status: "pending_acks"} made a glance-level read of the response look like success even when the producer's state did not change. On pipeline issue-1965 (#2064) the documenter agent followed exactly that mis-read into a 36-minute wait_loop deadlock. ok should mean "did this operation achieve its intent" — the producer didn't transition, so it didn't.

Why not raise GatewayError

pending_acks is a legitimate protocol state with actionable reasons (producer_not_fully_acked, global_zero_proposal, stale_acks, etc.). Forcing exception handling for an expected branch would be noisy and unlike the rest of the BRC tool surface that returns structured data. Sibling handlers (brc_propose, brc_ack, brc_nack) raise on success: False because they have no in-between state — brc_confirm is the only verb with a "deferred" outcome.

Caller impact

• The only Python caller, cmd_consensus_confirmed (sandbox/egg_lib/orch_cli.py:1681-1708), branches on status, not ok. Exit-code parity (0 confirmed / 2 pending / 1 error) is unchanged.
• No agent prompt currently keys on ok from this handler. The agent rule update (sandbox/agent-config/rules/orchestrator.md) preempts future mis-reads.

Test plan

• pytest tests/sandbox/egg_agent_tools/ — 357 pass, 4 skipped
• make lint-python — clean
• Updated test_handlers_brc.py::test_happy_confirmed and test_pending_acks to assert the new ok semantics
• Updated test_cli_parity.py mock for pending_acks to reflect the new contract (ok: False)

Out of scope

• BRC 'ready to confirm' nudge fires before global guards are clear, sending producers into pending_acks #2078 (upstream "ready to confirm" nudge bug) and check_brc_progress does not unstick fully-ACKed-but-not-confirmed producers — escalation never reaches the agent #2079 (check_brc_progress escalation gap) — independent. This fix is worth doing regardless, since it affects every pending_acks path, not just the documenter case.

Refs: #2064, #2078, #2079, PR #2077.

🤖 Generated with Claude Code

[egg-reviewer]

No agent-mode design concerns. This change actually improves agent-mode alignment — it makes the ok flag of an MCP tool response semantically match "did this operation achieve its intent," which is what an agent reads at a glance. The rule update in sandbox/agent-config/rules/orchestrator.md is concise orienting metadata (one sentence pointing at status/message for the rejection reason), not pre-fetched bulk content.

— Authored by egg

[egg-reviewer]

Approve. Clean, well-scoped fix for the pending_acks mis-read that caused the documenter deadlock on issue-1965.

What I verified

• Semantics match the orchestrator contract. All pending_acks exit paths in orchestrator/peer_consensus.py:511-595 return {"status": "pending_acks", ...} under a successful (success: True) envelope, so body.get("status") == "pending_acks" is the correct discriminator. The two-valued universe (confirmed | pending_acks) is real — there is no third success state to worry about.
• No caller breakage.
  • cmd_consensus_confirmed (sandbox/egg_lib/orch_cli.py:1702) branches on status, not ok. Exit codes (0/2/1) preserved — test_cli_parity.py::test_pending_acks_exits_2 still asserts rc == 2 with the new ok: False mock.
  • Repo-wide grep (brc_confirm, mcp__brc__confirm) shows only the agent rule, the docs reference table, the tests, and the CLI shim — none keys on ok.
• Edge case is unchanged. If the orchestrator returns success: True with no status key at all, pending = False, so ok = True, status = "confirmed" — same behavior as before the PR. Not a regression vector.
• Asymmetry with sibling handlers is justified. brc_propose / brc_ack / brc_nack raise GatewayError on success: False because they have no deferred-success state; brc_confirm does, so returning structured ok: False rather than raising is the right call. The PR description argues this explicitly and the reasoning holds.
• Docstring + agent rule + tests all updated coherently. The new ok semantics are documented at every layer where an agent might see them. test_happy_confirmed and test_pending_acks now both pin ok so a future regression in either direction breaks a test.

Non-blocking notes

• sandbox/agent-config/rules/orchestrator.md:44 is now ~370 chars on one line. Markdown renders it fine and the agent doesn't care, but if anyone ever wants to wrap it for human readability, the bullet would still work as a multi-line entry.
• test_pending_acks could optionally also assert resp["message"] is passed through (it currently is — result.get("message") at brc.py:256), but the existing coverage is sufficient for the contract being established here.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

3 previous review(s) hidden.