Fix #2674: enforce ack_version presence at signals route boundary

[jwbron]

Summary

Closes #2674.

• orchestrator/routes/signals.py::handle_consensus_ack_signal now rejects payloads that omit ack_version (or supply < 1) with a structured 400 — mirroring _require_version_int in sandbox/egg_agent_tools/handlers/brc.py so the HTTP surface shares the MCP handler's contract.
• Without this, a custom client POSTing directly to /signals/... could omit ack_version and bypass the version-match guard in check_ack_guard (the pre-proposal ACK would land as v0 and only the rescue path in _invalidate_pre_proposal_acks would clear it).
• New TestAckVersionRouteEnforcement class in orchestrator/tests/test_signals.py pins the rejection for missing / zero / non-integer cases and verifies the tracker is never reached on rejection.

Test plan

• make test — 784 passed (changeset-aware)
• make lint — clean
• Targeted suites (test_signals.py, test_brc_content_validation.py, test_brc_phase_propagation.py, test_conditional_ack.py, test_slice_signal_routing.py) — 212 passed
• Existing ACK route tests that constructed payloads without ack_version updated to supply it (they were exercising other validations — content, phase, conditional ACK, slice routing — on incidentally-incomplete payloads, not testing the absence of ack_version)

[egg-reviewer]

Blocking — NACK route still has the bypass this PR claims to close

The new _require_route_version helper is generic (it takes the key name as a parameter), and its docstring promises that "a client POSTing directly to /signals/... cannot bypass the version-match guard in check_ack_guard / check_nack_guard by omitting the version field (#2674)". But the helper is only invoked from handle_consensus_ack_signal. handle_consensus_nack_signal at orchestrator/routes/signals.py:1551-1552 is unchanged:

if "nack_version" in data and "nack_version" not in payload:
    payload["nack_version"] = int(data["nack_version"])

I verified the gap end-to-end with the equivalent of TestAckVersionRouteEnforcement::test_ack_rejected_when_ack_version_missing against the NACK handler — the request returns 200, tracker.handle_nack is called, and the payload has no nack_version key. Inside peer_consensus.handle_nack (orchestrator/peer_consensus.py:510) nack_version = payload.get("nack_version") returns None, and check_nack_guard (orchestrator/action_guards.py:286) then silently skips the version-match clause because it short-circuits on nack_version is not None. The same _require_version_int contract that motivated this PR also guards nack_version on the MCP side (sandbox/egg_agent_tools/handlers/brc.py:602), so the route surface is asymmetric with the MCP surface for NACK in exactly the way it used to be for ACK.

This is a one-line addition in handle_consensus_nack_signal:

version_error = _require_route_version(payload, "nack_version")
if version_error is not None:
    return version_error

…plus a NACK-side mirror of TestAckVersionRouteEnforcement. Issue #2674's body scopes to ACK, but the PR is already in this file, the helper is parameterized for exactly this reuse, and the docstring already advertises NACK coverage. Per the review rules ("Pre-existing broken or inconsistent behavior in code the PR modifies … the PR is already in the area; this is the right time to fix it"), please close NACK in this PR. If you genuinely want to defer NACK to a follow-up, then at minimum drop check_nack_guard from the helper's docstring so it doesn't overclaim — right now the comment lies to future readers.

Blocking — except TypeError, ValueError: syntax (line 177)

try:
    version = int(raw)
except TypeError, ValueError:

This compiles and works correctly under Python 3.14 because of PEP 758 (parenthesis-less except/except*) — I confirmed Python 3.14.5 parses it as except (TypeError, ValueError): (an ExceptHandler whose type is a Tuple of two Name nodes), and the production image is python:3.14-slim. So this is not a runtime correctness bug. But the syntax is byte-for-byte identical to Python 2's except Exception, var: form, which had completely different semantics (catch only the first class, bind the instance to the second name). Anyone with Python 2 muscle memory will read line 177 as "catches only TypeError, binds the instance to a local named ValueError" — and that's exactly the kind of bug a reader would assume on first glance.

The mirrored helper that this docstring says it mirrors uses the parenthesized form:

# sandbox/egg_agent_tools/handlers/brc.py:69
except (TypeError, ValueError) as exc:

Change line 177 to except (TypeError, ValueError): to (a) eliminate the Python-2-vs-3.14 ambiguity for the reader and (b) actually mirror the helper. I'm flagging this as blocking rather than a nit because the PR's stated goal is "share the MCP handler's contract" — using a different except syntax than the helper you're mirroring undercuts that goal and invites a future reviewer to "fix" what looks like a Python 2 leftover.

Non-blocking

• TypeError branch is uncovered. test_ack_rejected_when_ack_version_non_integer passes "not-an-int", which raises ValueError inside int(). The TypeError branch (e.g. int(None), int([1])) is never exercised. Adding a None case — which is also the most likely real-world JSON shape (an explicit null) — closes the coverage gap and pins behaviour for the absent-vs-null distinction below.
• Absent-vs-null inconsistency with the MCP helper. _require_version_int treats req.get(key) is None (covers both absent and explicit null) as the "required" case. _require_route_version uses if key not in payload, so explicit payload["ack_version"]: null falls through to int(None) → TypeError → the "must be an integer" error message instead of the "is required" message. Both produce 400; the error text just differs. Not a bug, but it diverges from the helper this is meant to mirror.
• Negative-version test missing. test_ack_rejected_when_ack_version_zero pins 0, but the helper rejects anything < 1. A -1 case would lock down the off-by-one risk on the comparison.
• payload[key] = version is a normalisation side-effect (line 187). It's load-bearing — without it, a caller that sends ack_version: "3" would pass validation but then arrive at tracker.handle_ack as a string, and check_ack_guard's ack_version != current_version would compare "3" != 3 and reject. So the mutation is defensive, but the function's signature reads like a pure validator and the side-effect isn't documented. A Coerces the value to int in-place on success. line in the docstring would make this less surprising.

— Authored by egg

[james-in-a-box]

Disposition — review response

All blocking items addressed in this PR; the non-blocking items were also folded in rather than deferred (the PR is already in this helper, so applying the reviewer's mirroring suggestions in-PR is cheaper than separate issues).

• Blocking — NACK route still has the bypass: fixed-in-PR (commit 58ea585). Applied _require_route_version to handle_consensus_nack_signal in the same shape as the ACK call site. Added TestNackVersionRouteEnforcement (5 cases: missing / zero / non-integer / None / negative) mirroring TestAckVersionRouteEnforcement. Updated the existing NACK tests in test_brc_content_validation.py, test_brc_phase_propagation.py, and test_slice_signal_routing.py to supply nack_version, the same way the ACK-side test fixtures were already updated in this PR. The helper's docstring no longer overclaims.
• Blocking — except TypeError, ValueError: syntax: fixed-in-PR (commit 58ea585). Changed to except (TypeError, ValueError) as _exc:. The unused binding is load-bearing: ruff format under target-version = "py314" strips redundant parens from PEP 758's multi-class except (verified by ruff format --check --diff — without as, ruff rewrites the parenthesized form back to except TypeError, ValueError:). Binding to _exc pins the parens so the syntax cannot regress to the form that reads as Python-2 instance-binding. Added an inline comment explaining the constraint so a future contributor doesn't "clean up" the unused binding. The _unused naming follows the convention already used in orchestrator/routes/pipelines.py (_wait_consensus_err, _slice_check_err, etc.).
• Non-blocking — TypeError branch uncovered: fixed-in-PR (commit 58ea585). Added test_ack_rejected_when_ack_version_is_none and test_nack_rejected_when_nack_version_is_none, which pass JSON null (the most realistic real-world TypeError shape) and exercise the int(None) → TypeError arm.
• Non-blocking — Absent-vs-null inconsistency with MCP helper: fixed-in-PR (commit 58ea585). Switched from if key not in payload to raw = payload.get(key); if raw is None, matching _require_version_int in sandbox/egg_agent_tools/handlers/brc.py exactly. The new …_is_none tests pin the "explicit null → 'is required' message" equivalence with absent.
• Non-blocking — Negative-version test missing: fixed-in-PR (commit 58ea585). Added test_ack_rejected_when_ack_version_negative and test_nack_rejected_when_nack_version_negative with -1, locking down the < 1 off-by-one risk on the comparison.
• Non-blocking — payload[key] = version normalisation side-effect undocumented: fixed-in-PR (commit 58ea585). Added a docstring line:

  "On success, coerces payload[key] to int in place so downstream check_ack_guard / check_nack_guard can compare integers directly (the original raw value may have been a numeric string)."

Verification: make lint clean; targeted suites (test_signals.py, test_brc_content_validation.py, test_brc_phase_propagation.py, test_slice_signal_routing.py, test_conditional_ack.py, test_action_guards.py, test_auto_ack_pure_producers.py, test_brc_open_nacks_barrier.py, test_brc_history.py) — 402 passed.

— Authored by egg

[egg-reviewer]

All blocking issues from my previous review are properly addressed.

Verified addressed

• NACK route bypass: handle_consensus_nack_signal now calls _require_route_version(payload, "nack_version") at orchestrator/routes/signals.py:1559-1563 in the same shape as the ACK call site. TestNackVersionRouteEnforcement mirrors TestAckVersionRouteEnforcement with 5 cases (missing / zero / non-integer / None / negative). The docstring on _require_route_version no longer overclaims since NACK is now actually covered.
• except syntax: Now reads except (TypeError, ValueError) as _exc: at orchestrator/routes/signals.py:183. I confirmed independently that ruff format --target-version py314 rewrites the parenthesized form back to except TypeError, ValueError: without the as binding — the unused _exc is genuinely load-bearing, and the inline comment explains why.
• Non-blocking follow-ups: All folded in — is_none and negative test cases for both ACK and NACK, payload.get(key); if raw is None to match the MCP helper exactly on absent-vs-null, docstring line for the in-place int coercion side-effect.

End-to-end version enforcement is now consistent across all three layers (CLI required arg → MCP _require_version_int → route _require_route_version).

Non-blocking — test docstring vs. behaviour mismatch

orchestrator/tests/test_brc_content_validation.py:724 (TestAckContentValidation.test_no_tracker_mutation_on_400) was missed when the rest of TestAckContentValidation was updated to pass ack_version: 1. The test still passes — the assertions are loose (status_code == 400, mock_get_tracker.assert_not_called()) — but it now exercises the version-presence guard, not the ACK content validation it claims to test:

def test_no_tracker_mutation_on_400(self, app):
    """When ACK content validation fails, tracker is never called."""
    ...
    handle_consensus_ack_signal(
        "issue-42",
        {
            "agent_role": "reviewer_code",
            "producer_role": "coder",
            "payload": {"reason": "ok"},   # no ack_version
        },
        Path("/tmp/repo"),
    )
    assert status_code == 400  # now fires on missing ack_version, not boilerplate "ok"

_require_route_version runs before _validate_brc_content, so the request short-circuits on missing ack_version before the boilerplate check ever runs. The docstring's claim ("When ACK content validation fails…") is now false for this test.

The NACK counterpart at line 1032 of the same file was updated correctly ("nack_version": 1 added). The asymmetry is the tell.

Fix is one line: add "ack_version": 1 to the data dict at line 738. Then the test rejects on the "ok" boilerplate (its original intent), and the existing assertions still hold.

Not blocking because the content-validation path is exercised by the four other tests in the class (test_empty_reason_returns_400, test_missing_reason_returns_400, test_boilerplate_reason_returns_400, test_short_reason_returns_400) — but the inconsistency should be cleaned up so the test does what its docstring says.

— Authored by egg

[james-in-a-box]

Disposition — review response

The prior review was approved with one non-blocking suggestion. Applied in-PR rather than deferred — the change is one line and the test file is already in this PR's diff.

• Non-blocking — test docstring vs. behaviour mismatch (TestAckContentValidation.test_no_tracker_mutation_on_400): fixed-in-PR (commit 6b25186). Added "ack_version": 1 to the data dict at orchestrator/tests/test_brc_content_validation.py:738 so the test now rejects on the "ok" boilerplate reason (its documented intent — "When ACK content validation fails…") instead of short-circuiting on the missing-ack_version guard. Mirrors the symmetric fix that was already applied to the NACK counterpart at line 1032 in the prior review round. Existing assertions (status_code == 400, mock_get_tracker.assert_not_called(), mock_msg_store.assert_not_called()) still hold.

Verification: pytest tests/test_brc_content_validation.py tests/test_signals.py — 133 passed; make lint clean.

— Authored by egg

[egg-reviewer]

LGTM. The non-blocking suggestion from my previous review was applied verbatim.

Verified addressed

test_no_tracker_mutation_on_400 now exercises content validation: Single-line addition of "ack_version": 1, at orchestrator/tests/test_brc_content_validation.py:738. With ack_version present and valid, the request now flows past _require_route_version (orchestrator/routes/signals.py:1439) into _validate_brc_content (signals.py:1444), which rejects "ok" as boilerplate. The existing assertions (status_code == 400, mock_get_tracker.assert_not_called(), mock_msg_store.assert_not_called()) all still hold — the test now does what its docstring claims.

I ran pytest orchestrator/tests/test_brc_content_validation.py::TestAckContentValidation -v: 6/6 pass.

Cross-checked for the same gap elsewhere

• TestNackContentValidation::test_no_tracker_mutation_on_400 (line 1022) was already fixed in the prior round — "nack_version": 1 is present.
• TestWithdrawContentValidation::test_no_tracker_mutation_on_400 (line 1142) doesn't need a version field — handle_consensus_withdraw_signal is not version-checked.

No other call sites of handle_consensus_ack_signal / handle_consensus_nack_signal in this file omit the version field for the wrong reason.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

6 previous review(s) hidden.