diff --git a/docs/reference/agent-tools.md b/docs/reference/agent-tools.md index 6034dfdce6..7beae05a9d 100644 --- a/docs/reference/agent-tools.md +++ b/docs/reference/agent-tools.md @@ -130,7 +130,7 @@ that requires the handler docstring to explain why no CLI exists. | `mcp__brc__send_heartbeat` | Emit a structured `HEARTBEAT` (schema-validated, per-role deduped, rate-limited) to the dedicated `/heartbeat` endpoint. Use `state=WAITING_ON_ROLE` + `waiting_on=` while blocking on BRC. Valid states: `WORKING`, `WAITING_ON_ROLE`, `WAITING_FOR_EVENT`, `PROPOSED`, `IDLE`. | `handlers.message.message_heartbeat` | `egg-orch message heartbeat` | > **Blocking waits use Bash, not MCP** (#2211). Long-poll waits don't fit the MCP transport — both transports cap tool calls below typical quiet-phase intervals (~30 s streamable-HTTP, ~60 s in-process SDK), and every cap-elapsed return is a wasted LLM turn. Use `egg-orch message wait` / `egg-orch message wait-loop` (sandbox) and `egg-orch pipeline wait-status` (host) via Bash. The §1 idiom in `docs/reference/agent-wait-patterns.md` is the canonical shape. -| `mcp__brc__read_peer_artifact` | Read entries from `.egg-state/brc-history/-.json` (and the per-slice partition `-implement-.json` when `EGG_SLICE_ID` is set and `phase == "implement"`; the sibling `-implement-unattributed.json` is merged in by default and disabled via `include_unattributed=False`). Optional filters: `peer_role` / `producer_role` (alias), `message_type` (str or list). `limit` / `cursor` pagination (default `limit=50`, max 500). The identifier is resolved server-side from `EGG_ISSUE_NUMBER` / `EGG_PIPELINE_ID` (agents cannot pass an arbitrary id; path-traversal hardening). Returns `{items: [...], next_cursor: , total_available: , skipped_malformed: }`. | `handlers.brc.brc_read_peer_artifact` | `egg-orch brc read-peer-artifact` *(slice-5 of #2908; thin wrapper, registration still `cli_command=None` — see callout below)* | +| `mcp__brc__read_peer_artifact` | Read entries from `.egg-state/brc-history/-.json` (and the per-slice partition `-implement-.json` when `EGG_SLICE_ID` is set and `phase == "implement"`; the sibling `-implement-unattributed.json` is merged in by default and disabled via `include_unattributed=False`). Optional filters: `peer_role` / `producer_role` (alias), `message_type` (str or list). `limit` / `cursor` pagination (default `limit=50`, max 500). The identifier is resolved server-side from `EGG_ISSUE_NUMBER` / `EGG_PIPELINE_ID` (agents cannot pass an arbitrary id; path-traversal hardening). Returns `{items: [...], next_cursor: , total_available: , skipped_malformed: , hint?: }` — `hint` is present only when no history file exists on disk, the expected state for the phase currently in flight (#3076): brc-history is written at phase *completion* and reaches an agent worktree only at spawn, so a current-phase read is structurally empty and is not evidence that peers have not proposed (live proposals: `pending_reviews[].proposal_commit_sha` + `git show :`). | `handlers.brc.brc_read_peer_artifact` | `egg-orch brc read-peer-artifact` *(slice-5 of #2908; thin wrapper, registration still `cli_command=None` — see callout below)* | | `mcp__brc__resolve_obligation` | Mark a reviewer's conditional-ACK obligation as satisfied in-cycle (#2338). Required: `reviewer_role`, `producer_role`. Optional: `commit_sha`, `note`. The matrix keeps the obligation text for audit, but `get_pre_merge_conditions` filters resolved entries — the PR body and HITL gate stop surfacing the obligation. The orchestrator persists a `CONSENSUS_OBLIGATION_RESOLVED` message so the resolution survives orchestrator restart, and rejects `resolver_role == producer_role` so a producer cannot self-resolve their own obligation. Resolution is per-version: any later ACK / NACK / invalidate on the same edge resets the resolved flag. | `handlers.brc.brc_resolve_obligation` | `egg-orch brc resolve-obligation` *(slice-5 of #2908; thin wrapper, registration still `cli_command=None` — see callout below)* | #### `brc_propose` push behavior diff --git a/integration_tests/regression/test_brc_concurrency.py b/integration_tests/regression/test_brc_concurrency.py index febcab43d7..594567922c 100644 --- a/integration_tests/regression/test_brc_concurrency.py +++ b/integration_tests/regression/test_brc_concurrency.py @@ -94,7 +94,7 @@ def test_six_reviewers_acking_in_parallel_all_recorded( n = 6 graph = self._wide_graph(n) tracker = make_tracker(self.PIPELINE_ID, graph) - tracker.handle_propose("coder", propose_payload(commit_sha="abc")) + tracker.handle_propose("coder", propose_payload(commit_sha="abc1234")) barrier = threading.Barrier(n) @@ -130,7 +130,7 @@ def test_propose_and_ack_interleaved_does_not_corrupt_state(self, two_reviewer_g with the recorded ACK versions). """ tracker = make_tracker(self.PIPELINE_ID + "-interleave", two_reviewer_graph) - tracker.handle_propose("coder", propose_payload(commit_sha="v1")) + tracker.handle_propose("coder", propose_payload(commit_sha="1111111")) # reviewer_code ACKs v1. tracker.handle_ack("reviewer_code", "coder", {"ack_version": 1, **ack_payload()}) @@ -141,7 +141,7 @@ def repropose() -> dict: barrier.wait(timeout=5) return tracker.handle_re_propose( "coder", - propose_payload(commit_sha="v2"), + propose_payload(commit_sha="2222222"), changed_artifacts=["a.py"], ) @@ -195,7 +195,7 @@ def test_two_concurrent_nacks_block_repropose_until_acknowledged( been informed) is accepted. """ tracker = make_tracker(self.PIPELINE_ID, two_reviewer_graph) - tracker.handle_propose("coder", propose_payload(commit_sha="abc")) + tracker.handle_propose("coder", propose_payload(commit_sha="abc1234")) barrier = threading.Barrier(2) @@ -220,7 +220,7 @@ def nack(reviewer: str) -> dict: # surfaced before the producer can advance. first = tracker.handle_re_propose( "coder", - propose_payload(commit_sha="def"), + propose_payload(commit_sha="def5678"), changed_artifacts=["a.py"], ) assert first["status"] == "open_nacks_blocked" @@ -230,7 +230,7 @@ def nack(reviewer: str) -> dict: # Producer has now been informed — retry proceeds. second = tracker.handle_re_propose( "coder", - propose_payload(commit_sha="def"), + propose_payload(commit_sha="def5678"), changed_artifacts=["a.py"], ) assert second["version"] == 2 @@ -252,7 +252,7 @@ def test_withdraw_within_cooldown_is_rejected(self) -> None: graph = ReviewGraph([ReviewEdge("reviewer_code", "coder", ReviewCriticality.CRITICAL)]) tracker = make_tracker(self.PIPELINE_ID, graph, cooldown_seconds=60) - tracker.handle_propose("coder", propose_payload(commit_sha="abc")) + tracker.handle_propose("coder", propose_payload(commit_sha="abc1234")) # Immediate withdraw — well within the 60s cooldown. with pytest.raises(ValueError, match="[Cc]ooldown"): @@ -288,14 +288,14 @@ def test_flip_flop_lockout_after_repeated_withdrawals(self) -> None: tracker.register_agent("reviewer_code") # Cycle 1: propose → withdraw → counter=1, allowed. - tracker.handle_propose("coder", propose_payload(commit_sha="abc")) + tracker.handle_propose("coder", propose_payload(commit_sha="abc1234")) r1 = tracker.handle_withdraw("coder", reason="bug found") assert r1["status"] == "withdrawn" # Cycle 2: propose → withdraw → peek=2, 2 >= max=2, locked out. # The guard's peek is ``current + 1 >= max`` so with max=2 the # second withdraw is the one that gets locked out. - tracker.handle_propose("coder", propose_payload(commit_sha="def")) + tracker.handle_propose("coder", propose_payload(commit_sha="def5678")) r2 = tracker.handle_withdraw("coder", reason="another bug") assert r2["status"] == "locked_out" assert r2["needs_escalation"] is True diff --git a/integration_tests/regression/test_brc_edge_cases.py b/integration_tests/regression/test_brc_edge_cases.py index efce70d6b5..6857efa219 100644 --- a/integration_tests/regression/test_brc_edge_cases.py +++ b/integration_tests/regression/test_brc_edge_cases.py @@ -53,8 +53,10 @@ def test_tester_confirms_only_when_both_state_machines_confirm(self) -> None: tracker = make_tracker(self.PIPELINE_ID, graph) # Both producers propose. - tracker.handle_propose("coder", propose_payload(commit_sha="abc")) - tracker.handle_propose("tester", propose_payload(commit_sha="def", artifacts=["test_x.py"])) + tracker.handle_propose("coder", propose_payload(commit_sha="abc1234")) + tracker.handle_propose( + "tester", propose_payload(commit_sha="def5678", artifacts=["test_x.py"]) + ) # All cross-ACKs. tracker.handle_ack("reviewer_code", "coder", {"ack_version": 1, **ack_payload()}) @@ -119,7 +121,7 @@ def test_two_slices_have_independent_trackers(self) -> None: tracker_2.register_agent(role) # slice-1: PROPOSE → NACK (consensus blocked). - tracker_1.handle_propose("coder", propose_payload(commit_sha="aaa")) + tracker_1.handle_propose("coder", propose_payload(commit_sha="aaa1111")) tracker_1.handle_nack( "reviewer_code", "coder", @@ -127,7 +129,7 @@ def test_two_slices_have_independent_trackers(self) -> None: ) # slice-2: PROPOSE → ACK → CONFIRMED (consensus reached). - tracker_2.handle_propose("coder", propose_payload(commit_sha="bbb")) + tracker_2.handle_propose("coder", propose_payload(commit_sha="bbb2222")) tracker_2.handle_ack("reviewer_code", "coder", {"ack_version": 1, **ack_payload()}) tracker_2.handle_confirmed("coder") tracker_2.handle_confirmed("reviewer_code") @@ -171,7 +173,7 @@ def test_conditional_ack_then_obligation_resolved_clears_merge_block( ] ) tracker = make_tracker(self.PIPELINE_ID, graph) - tracker.handle_propose("coder", propose_payload(commit_sha="abc")) + tracker.handle_propose("coder", propose_payload(commit_sha="abc1234")) # Conditional ACK — reviewer says "I'd merge if you ran git mv X Y first". tracker.handle_ack( @@ -223,7 +225,7 @@ class TestStaleVersionAckRejection: def test_acking_old_version_after_repropose_is_rejected(self, single_reviewer_graph) -> None: """A reviewer can't ACK v1 after the producer has re-proposed to v2.""" tracker = make_tracker(self.PIPELINE_ID, single_reviewer_graph) - tracker.handle_propose("coder", propose_payload(commit_sha="v1")) + tracker.handle_propose("coder", propose_payload(commit_sha="1111111")) # Reviewer NACKs v1. tracker.handle_nack( "reviewer_code", @@ -233,7 +235,7 @@ def test_acking_old_version_after_repropose_is_rejected(self, single_reviewer_gr # Producer re-proposes (v2). tracker.handle_re_propose( "coder", - propose_payload(commit_sha="v2"), + propose_payload(commit_sha="2222222"), changed_artifacts=["a.py"], ) # Reviewer mistakenly tries to ACK the old v1 — guard rejects. diff --git a/integration_tests/regression/test_brc_phase_timeout.py b/integration_tests/regression/test_brc_phase_timeout.py index 241aaf7813..a95f479850 100644 --- a/integration_tests/regression/test_brc_phase_timeout.py +++ b/integration_tests/regression/test_brc_phase_timeout.py @@ -124,7 +124,7 @@ def test_critical_blocker_lands_phase_override_in_alert( pipeline = _make_pipeline(pipeline_id, config, PipelinePhase.PLAN) graph = ReviewGraph([ReviewEdge("reviewer_code", "coder", ReviewCriticality.CRITICAL)]) tracker = make_tracker(pipeline_id, graph) - tracker.handle_propose("coder", propose_payload(commit_sha="abc")) + tracker.handle_propose("coder", propose_payload(commit_sha="abc1234")) tracker.handle_nack( "reviewer_code", "coder", @@ -181,7 +181,7 @@ def test_advisory_only_path_is_silent_at_alert_layer( ) graph = ReviewGraph([ReviewEdge("reviewer_contract", "coder", ReviewCriticality.ADVISORY)]) tracker = make_tracker(pipeline_id, graph) - tracker.handle_propose("coder", propose_payload(commit_sha="abc")) + tracker.handle_propose("coder", propose_payload(commit_sha="abc1234")) tracker.handle_nack( "reviewer_contract", "coder", @@ -229,7 +229,7 @@ def test_idle_critical_reviewer_fires_overseer_alert( graph = ReviewGraph([ReviewEdge("reviewer_code", "coder", ReviewCriticality.CRITICAL)]) tracker = make_tracker(pipeline_id, graph) # Producer proposes; reviewer never ACKs or NACKs. - tracker.handle_propose("coder", propose_payload(commit_sha="abc")) + tracker.handle_propose("coder", propose_payload(commit_sha="abc1234")) capture, alerts = _capture_alerts() with capture: @@ -271,7 +271,7 @@ def test_idle_advisory_reviewer_fires_notification(self, event_capture, filter_e ) graph = ReviewGraph([ReviewEdge("reviewer_contract", "coder", ReviewCriticality.ADVISORY)]) tracker = make_tracker(pipeline_id, graph) - tracker.handle_propose("coder", propose_payload(commit_sha="abc")) + tracker.handle_propose("coder", propose_payload(commit_sha="abc1234")) capture, alerts = _capture_alerts() with capture: @@ -300,7 +300,7 @@ def test_consensus_reached_before_timeout_is_no_op(self, event_capture, filter_e pipeline = _make_pipeline(pipeline_id, PipelineConfig(), PipelinePhase.PLAN) graph = ReviewGraph([ReviewEdge("reviewer_code", "coder", ReviewCriticality.CRITICAL)]) tracker = make_tracker(pipeline_id, graph) - tracker.handle_propose("coder", propose_payload(commit_sha="abc")) + tracker.handle_propose("coder", propose_payload(commit_sha="abc1234")) tracker.handle_ack("reviewer_code", "coder", {"ack_version": 1, **ack_payload()}) tracker.handle_confirmed("coder") tracker.handle_confirmed("reviewer_code") @@ -366,7 +366,7 @@ def test_resolved_minutes_match_alert_metadata( # branch — that's the path that publishes an alert. graph = ReviewGraph([ReviewEdge("reviewer_code", "coder", ReviewCriticality.CRITICAL)]) tracker = make_tracker(pipeline_id, graph) - tracker.handle_propose("coder", propose_payload(commit_sha="abc")) + tracker.handle_propose("coder", propose_payload(commit_sha="abc1234")) tracker.handle_nack( "reviewer_code", "coder", diff --git a/integration_tests/regression/test_brc_single_cycle.py b/integration_tests/regression/test_brc_single_cycle.py index c9eae227e0..6b0947c19a 100644 --- a/integration_tests/regression/test_brc_single_cycle.py +++ b/integration_tests/regression/test_brc_single_cycle.py @@ -177,7 +177,7 @@ def test_pre_proposal_ack_without_version_is_recorded_then_invalidated( assert result["status"] == "acked" assert result["version"] == 0 # Now the producer proposes — invalidation kicks in. - result = tracker.handle_propose("coder", propose_payload(commit_sha="abc")) + result = tracker.handle_propose("coder", propose_payload(commit_sha="abc1234")) assert "reviewer_code" in result["stale_reviewers"] def test_pre_proposal_ack_with_explicit_version_is_rejected( @@ -210,7 +210,7 @@ def test_producer_confirm_before_full_acks_is_pending(self, two_reviewer_graph) """ pipeline_id = self.PIPELINE_ID + "-prod" tracker = make_tracker(pipeline_id, two_reviewer_graph) - tracker.handle_propose("coder", propose_payload(commit_sha="abc")) + tracker.handle_propose("coder", propose_payload(commit_sha="abc1234")) tracker.handle_ack("reviewer_code", "coder", {"ack_version": 1, **ack_payload()}) # Only reviewer_code has ACKed; reviewer_contract has not. result = tracker.handle_confirmed("coder") diff --git a/orchestrator/attestation_schemas.py b/orchestrator/attestation_schemas.py index 4c24964e8f..808dc5aa7e 100644 --- a/orchestrator/attestation_schemas.py +++ b/orchestrator/attestation_schemas.py @@ -5,11 +5,14 @@ actually doing the work — and enable cross-verification by reviewers. """ +import re from enum import StrEnum from typing import Any from pydantic import BaseModel, Field, model_validator +_COMMIT_SHA_PATTERN = re.compile(r"[A-Za-z0-9_]{7,64}") + class AttestationStrictness(StrEnum): """Strictness level for attestation validation.""" @@ -212,6 +215,55 @@ def validate_commit_sha_present(self) -> ProposalPayload: ) return self + @model_validator(mode="after") + def validate_commit_sha_format(self) -> ProposalPayload: + """Reject commit_sha values containing shell metacharacters (#3076). + + The producer-supplied SHA is interpolated into rendered shell + commands in the reviewer's event prompt (the per-producer + ``git log ..`` and ``git show :`` + renders in ``orchestrator/routes/event_prompt.py``). + ``_extract_proposal_sha_for_producer`` gates the read path with + a stricter hex-only regex, but downstream consumers + (``orchestrator/peer_consensus.py``, + ``orchestrator/routes/signals.py``, + ``orchestrator/routes/pipelines.py``) read + ``_proposal_commit_shas`` directly without revalidating — so + enforce a shell-safe baseline at the writer too: only + ``[A-Za-z0-9_]`` permitted, 7-64 chars. This rejects every + dangerous form (whitespace, ``;``, ``$(…)``, ``..``, ranges) + while accepting reconstruction sentinels like + ``RECONSTRUCTED_NO_SHA`` that downstream code keys on. Skipped + for a no-op propose (#3027): ``commit_sha`` is empty by design + there. + + Asymmetric regex with + ``orchestrator/routes/event_prompt.py::_extract_proposal_sha_for_producer`` + is intentional: the strict hex-only check there is the + shell-interpolation boundary (rejects sentinels before they + reach a rendered ``git`` command), while this loose + alphanumeric+underscore check is the writer-side baseline + (admits sentinels so they can round-trip through + ``_proposal_commit_shas`` to non-shell consumers). Do not + unify — tightening this regex breaks the sentinel round-trip; + loosening the reader regex re-opens the shell-injection gap. + + Relies on ``validate_commit_sha_present`` running first + (pydantic ``model_validator(mode="after")`` honours definition + order) to reject empty non-no-op proposals before this check + sees them; the ``not self.commit_sha`` guard below is + defence-in-depth and unreachable in practice. + """ + if self.no_changes_needed or not self.commit_sha: + return self + if not _COMMIT_SHA_PATTERN.fullmatch(self.commit_sha): + raise ValueError( + "Proposal commit_sha must be 7-64 alphanumeric/underscore " + "characters (no shell metacharacters); got " + f"{self.commit_sha!r}." + ) + return self + class ReviewPayload(BaseModel): """Payload for CONSENSUS_ACK and CONSENSUS_NACK messages. diff --git a/orchestrator/routes/consensus.py b/orchestrator/routes/consensus.py index 02e5da0bba..45d8da5100 100644 --- a/orchestrator/routes/consensus.py +++ b/orchestrator/routes/consensus.py @@ -207,6 +207,16 @@ def _has_pending_peer_proposals( "prior_version": entry.version if entry else 0, "prior_verdict": entry.state.value if entry else "pending", "artifact_refs": artifact_refs, + # The producer's proposed commit SHA (#3076). Per-role + # worktrees share the host repo's object store, so the + # composer can scope the re-review delta to + # ``{last_reviewed}..{proposal_commit_sha}`` and render + # ``git show :`` reads that work from the + # reviewer's worktree without any push/merge + # choreography. ``{sha}..HEAD`` against the REVIEWER's + # HEAD never contains the producer's commits — that was + # the "re-review delta is empty" phantom-NACK. + "proposal_commit_sha": str(snapshot.get("commit_sha") or ""), } ) return bool(pending), pending diff --git a/orchestrator/routes/event_prompt.py b/orchestrator/routes/event_prompt.py index ec68be3c86..9f2ec48625 100644 --- a/orchestrator/routes/event_prompt.py +++ b/orchestrator/routes/event_prompt.py @@ -195,11 +195,15 @@ def _render_producer_delta_section( """Render the per-producer ``git log`` re-review-scope block. Each entry is a dict with keys ``producer``, ``last_reviewed_commit_sha``, - and ``delta``. The command form is rendered verbatim so the agent - sees the exact scope (architect plan acceptance: "git-log delta - command is emitted verbatim with the per-producer - ``last_reviewed_commit_sha`` substituted in"); the rendered diff - follows so the agent can audit the full change as a fresh review. + ``proposal_commit_sha``, and ``delta``. The command form is rendered + verbatim so the agent sees the exact scope (architect plan + acceptance: "git-log delta command is emitted verbatim with the + per-producer ``last_reviewed_commit_sha`` substituted in"); the + rendered diff follows so the agent can audit the full change as a + fresh review. ``proposal_commit_sha`` is used as the range end-ref + (#3076) so the delta is scoped to the producer's pushed work + instead of the reviewer's own HEAD; legacy payloads without it fall + back to ``HEAD`` and the rendered caution. Returns ``(section_markdown, total_delta_bytes)`` so the caller can measure the delta separately from the envelope budget. @@ -225,25 +229,50 @@ def _render_producer_delta_section( for entry in git_log_delta: producer = str(entry.get("producer") or "(unknown)").strip() sha = str(entry.get("last_reviewed_commit_sha") or "").strip() + proposal_sha = str(entry.get("proposal_commit_sha") or "").strip() delta = entry.get("delta") or "" if not isinstance(delta, str): delta = str(delta) # Command is emitted verbatim — the per-producer - # ``last_reviewed_commit_sha`` substituted in so the agent can - # cross-check the scope against the orchestrator's stored value. + # ``last_reviewed_commit_sha`` and the proposal endpoint + # substituted in so the agent can cross-check the scope against + # the orchestrator's stored values. ``end_ref`` is the + # producer's proposed commit when the payload carries one + # (#3076); ``HEAD`` only on legacy payloads. cmd_sha = sha if sha else "" - cmd = f"git log {cmd_sha}..HEAD --not origin/{base_branch} -p" + end_ref = proposal_sha or "HEAD" + cmd = f"git log {cmd_sha}..{end_ref} --not origin/{base_branch} -p" + if delta.strip(): + delta_rendered = delta + elif proposal_sha: + delta_rendered = "(no commits in range — re-review is a no-op)" + else: + # Empty delta against the reviewer's own HEAD is NOT + # evidence the producer didn't revise: per-role worktrees + # mean the reviewer's HEAD never contains the producer's + # commits (#3076 — the "re-review delta is empty" phantom + # NACK). Only trust an empty range when it was scoped to + # the producer's proposal SHA. + delta_rendered = ( + "(no commits in range — CAUTION: this range ended at YOUR " + "worktree's HEAD, which does not contain the producer's " + "commits. An empty delta here is NOT evidence the producer " + "didn't revise. Read the producer's branch directly, e.g. " + "`git log --not " + f"origin/{base_branch} -p`, before issuing a verdict.)" + ) lines.extend( [ f"### Producer: ``{producer}``", "", f"- last_reviewed_commit_sha: `{sha or '-'}`", + f"- proposal_commit_sha: `{proposal_sha or '-'}`", "- Re-review scope (executed by the wrapper):", f" `{cmd}`", "", "Delta:", "```diff", - delta if delta.strip() else "(no commits in range — re-review is a no-op)", + delta_rendered, "```", "", ] @@ -529,19 +558,29 @@ def _run_git_log( sha: str, base_branch: str, repo_path: Path, + end_ref: str = "HEAD", ) -> str: - """Render ``git log {sha}..HEAD --not origin/{base_branch} -p``. + """Render ``git log {sha}..{end_ref} --not origin/{base_branch} -p``. Runs the subprocess in ``repo_path``. The gateway allows ``git log`` with ``-p`` / ``--patch`` and ``--not`` flags (see ``gateway`` allow-list; #2905). On non-zero rc or timeout we return a sentinel string so the agent can audit the failure explicitly rather than silently reviewing an empty diff. + + ``end_ref`` defaults to ``HEAD`` for legacy payloads, but callers + should pass the producer's ``proposal_commit_sha`` when the event + payload carries one (#3076): the reviewer's own HEAD does not + contain the producer's commits (per-role worktrees), so a + ``{sha}..HEAD`` range in the reviewer's worktree is empty even + when the producer revised — the "re-review delta is empty" + phantom-NACK. The proposal SHA resolves from any agent worktree + because all per-role worktrees share the host repo's object store. """ cmd = [ "git", "log", - f"{sha}..HEAD", + f"{sha}..{end_ref}", "--not", f"origin/{base_branch}", "-p", @@ -556,7 +595,7 @@ def _run_git_log( check=False, ) except subprocess.TimeoutExpired: - return f"(git log timed out after {_GIT_LOG_TIMEOUT_SECS}s for {sha}..HEAD)" + return f"(git log timed out after {_GIT_LOG_TIMEOUT_SECS}s for {sha}..{end_ref})" except OSError as exc: # pragma: no cover — defensive return f"(git log failed: {exc})" @@ -651,12 +690,26 @@ def _build_delta_entries( out: list[dict[str, Any]] = [] for producer in scoped_producers: sha = per_producer_sha.get(producer, "") + # The producer's proposed commit SHA from pending_reviews + # (#3076). When present it is BOTH the delta endpoint (the + # reviewer's own HEAD never contains the producer's commits — + # per-role worktrees — so ``{sha}..HEAD`` was empty even after + # a revision) and the anchor for ``git show :`` + # artifact reads, which resolve from any agent worktree via + # the shared host object store. + proposal_sha = _extract_proposal_sha_for_producer(event_payload, producer) if sha: - delta = _run_git_log(sha, base_branch, repo_path) + delta = _run_git_log( + sha, + base_branch, + repo_path, + end_ref=proposal_sha or "HEAD", + ) out.append( { "producer": producer, "last_reviewed_commit_sha": sha, + "proposal_commit_sha": proposal_sha, "delta": delta, } ) @@ -666,22 +719,47 @@ def _build_delta_entries( # Prefer per-producer artifact_refs from pending_reviews; fall # back to the legacy top-level changed_artifacts key. artifacts = _extract_artifacts_for_producer(event_payload, producer) - if not artifacts: + if not artifacts and not proposal_sha: continue refs_text = "\n".join(f"- `{a}`" for a in artifacts) - fallback_delta = ( - "(No `last_reviewed_commit_sha` recorded yet for this " - "producer — falling back to the orchestrator's signal-level " - "`changed_artifacts` list as a degraded baseline. This is " - "NOT the adversarial-re-review path; if your role demands a " - "full audit, fetch and read the actual file diffs yourself " - "before issuing a verdict.)\n\n" - f"Artifacts the orchestrator flagged as changed:\n{refs_text}\n" - ) + if proposal_sha: + # First review with a known proposal SHA: render concrete, + # working read commands instead of the directionless "fetch + # and read the diffs yourself" (#3076 — reviewers NACKed + # plans they could not find because nothing said WHERE the + # producer's work lives; their worktree does not contain + # it, but the shared object store resolves the SHA). + show_cmds = "\n".join(f"- `git show {proposal_sha}:{a}`" for a in artifacts) + fallback_delta = ( + "(No `last_reviewed_commit_sha` recorded yet for this " + "producer — this is your FIRST review of this proposal. " + "The producer's work is NOT in your working tree; per-" + "role worktrees are isolated. Read it via the proposed " + f"commit `{proposal_sha}`, which resolves from your " + "worktree through the shared object store:)\n\n" + + (f"Proposed artifacts:\n{show_cmds}\n\n" if artifacts else "") + + "Full proposed change:\n" + f"- `git log {proposal_sha} --not origin/{base_branch} -p`\n\n" + "Do NOT NACK for a missing file before reading it via " + "these commands — a plain `Read` of the path in your own " + "worktree is expected to fail and is not evidence the " + "artifact does not exist.\n" + ) + else: + fallback_delta = ( + "(No `last_reviewed_commit_sha` recorded yet for this " + "producer — falling back to the orchestrator's signal-level " + "`changed_artifacts` list as a degraded baseline. This is " + "NOT the adversarial-re-review path; if your role demands a " + "full audit, fetch and read the actual file diffs yourself " + "before issuing a verdict.)\n\n" + f"Artifacts the orchestrator flagged as changed:\n{refs_text}\n" + ) out.append( { "producer": producer, "last_reviewed_commit_sha": "", + "proposal_commit_sha": proposal_sha, "delta": fallback_delta, } ) @@ -757,6 +835,56 @@ def _extract_current_producers(event_payload: Any) -> list[str]: return seen +def _extract_proposal_sha_for_producer(event_payload: Any, producer: str) -> str: + """Pull the producer's proposed commit SHA from the event payload. + + Reads ``pending_reviews[i].proposal_commit_sha`` for the entry whose + ``producer`` matches (#3076) — the enrichment added by the + next-action route from + ``PeerConsensusTracker.get_current_proposal_snapshot``. Returns + ``""`` when the payload carries no SHA for the named producer + (legacy payloads, synthetic test paths), in which case callers fall + back to the pre-#3076 behaviour (``HEAD`` delta endpoint / the + degraded artifact-list fallback). + + The value is sanitised to a hex-ish token before being embedded in + rendered shell commands: anything that is not a 7-64 char hex + string is discarded rather than interpolated. + + Asymmetric regex with + ``orchestrator/attestation_schemas.py::ProposalPayload.validate_commit_sha_format`` + is intentional: that writer-side validator uses a loose + ``[A-Za-z0-9_]{7,64}`` so reconstruction sentinels (e.g. + ``RECONSTRUCTED_NO_SHA``) round-trip through + ``_proposal_commit_shas`` to non-shell consumers; this reader-side + check is the strict hex-only shell-interpolation boundary that + rejects those sentinels before any rendered ``git`` command sees + them. Do not unify — tightening the writer breaks the sentinel + round-trip; loosening this reader re-opens the shell-injection gap. + """ + if not isinstance(event_payload, dict) or not isinstance(producer, str): + return "" + producer = producer.strip() + if not producer: + return "" + pending = event_payload.get("pending_reviews") + if not isinstance(pending, list): + return "" + for entry in pending: + if not isinstance(entry, dict): + continue + entry_producer = entry.get("producer") or entry.get("producer_role") + if not isinstance(entry_producer, str) or entry_producer.strip() != producer: + continue + raw = entry.get("proposal_commit_sha") + if isinstance(raw, str): + candidate = raw.strip() + if re.fullmatch(r"[0-9a-fA-F]{7,64}", candidate): + return candidate + return "" + return "" + + def _extract_artifacts_for_producer(event_payload: Any, producer: str) -> list[str]: """Pull the artifact list for a specific producer from the payload. diff --git a/orchestrator/tests/test_brc_history.py b/orchestrator/tests/test_brc_history.py index dd0f5c6711..96179e4d3a 100644 --- a/orchestrator/tests/test_brc_history.py +++ b/orchestrator/tests/test_brc_history.py @@ -686,7 +686,7 @@ def test_triple_backtick_body_does_not_corrupt_yaml_block(self, tmp_path): subject="Proposal with code", body=body_with_fences, phase="implement", - metadata={"version": 1, "commit_sha": "abc123"}, + metadata={"version": 1, "commit_sha": "abc1234"}, ), ] @@ -716,7 +716,7 @@ def test_triple_backtick_body_does_not_corrupt_yaml_block(self, tmp_path): parsed = yaml.safe_load("\n".join(yaml_lines)) assert parsed is not None, "YAML block should be parseable" assert "metadata" in parsed - assert parsed["metadata"]["commit_sha"] == "abc123" + assert parsed["metadata"]["commit_sha"] == "abc1234" elif in_yaml: yaml_lines.append(line) assert yaml_blocks_found >= 1, "Should find at least one YAML metadata block" @@ -1077,7 +1077,7 @@ def test_yaml_block_with_nested_metadata(self, tmp_path): "files_reviewed": ["a.py", "b.py"], }, "revision_count": 3, - "commit_sha": "abc123", + "commit_sha": "abc1234", "version": 2, } messages = [ @@ -1113,7 +1113,7 @@ def test_yaml_block_with_nested_metadata(self, tmp_path): assert parsed["metadata"]["payload"]["reason"] == "Missing error handling" assert parsed["metadata"]["payload"]["files_reviewed"] == ["a.py", "b.py"] assert parsed["metadata"]["revision_count"] == 3 - assert parsed["metadata"]["commit_sha"] == "abc123" + assert parsed["metadata"]["commit_sha"] == "abc1234" found_yaml = True elif in_yaml: yaml_lines.append(line) diff --git a/orchestrator/tests/test_brc_nack_iteration.py b/orchestrator/tests/test_brc_nack_iteration.py index 7f7ec25970..4fd5569d71 100644 --- a/orchestrator/tests/test_brc_nack_iteration.py +++ b/orchestrator/tests/test_brc_nack_iteration.py @@ -67,7 +67,7 @@ def test_evaluate_reports_no_nacks_initially(self, tracker): def test_evaluate_reports_nack_after_nack(self, tracker): tracker.handle_propose( - "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc1234"} ) tracker.handle_nack( "reviewer_code", @@ -87,7 +87,7 @@ def test_evaluate_nack_blocks_completion_even_if_all_confirmed(self, tracker): """If all agents somehow end up in _confirmed but NACKs exist, is_complete must still be False.""" tracker.handle_propose( - "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc1234"} ) # reviewer_code NACKs @@ -116,7 +116,7 @@ def test_evaluate_nack_blocks_completion_even_if_all_confirmed(self, tracker): def test_evaluate_clears_nack_after_re_propose_and_ack(self, tracker): """After producer re-proposes and gets ACKed, NACKs should be resolved.""" tracker.handle_propose( - "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc1234"} ) tracker.handle_nack( "reviewer_code", @@ -132,7 +132,7 @@ def test_evaluate_clears_nack_after_re_propose_and_ack(self, tracker): # Re-propose tracker.handle_re_propose( "coder", - {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "abc123"}, + {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "abc1234"}, changed_artifacts=["a.py"], ) diff --git a/orchestrator/tests/test_brc_phase_propagation.py b/orchestrator/tests/test_brc_phase_propagation.py index 6728324f16..78ed0d3e40 100644 --- a/orchestrator/tests/test_brc_phase_propagation.py +++ b/orchestrator/tests/test_brc_phase_propagation.py @@ -268,7 +268,7 @@ def test_propose_message_has_phase( "agent_role": "coder", "payload": { "summary": "Implemented authentication module with JWT validation and session management", - "commit_sha": "abc123", + "commit_sha": "abc1234", }, }, Path("/tmp/repo"), diff --git a/orchestrator/tests/test_compose_event_prompt.py b/orchestrator/tests/test_compose_event_prompt.py index 59c3cff239..4ea2ec1b05 100644 --- a/orchestrator/tests/test_compose_event_prompt.py +++ b/orchestrator/tests/test_compose_event_prompt.py @@ -793,6 +793,232 @@ def test_build_delta_entries_prefers_real_sha_over_fallback() -> None: assert "degraded baseline" not in entry["delta"] +# --------------------------------------------------------------------------- +# #3076: the re-review delta must end at the producer's PROPOSAL SHA, not +# the reviewer's own HEAD (which never contains the producer's commits — +# per-role worktrees), and the first-review fallback must render concrete +# `git show :` reads instead of a directionless instruction. +# --------------------------------------------------------------------------- + +_COD_MEMORY_WITH_SHA = ( + "## Per-producer assessment\n\n" + "### coder\n\n" + "- producer: coder\n" + "- last_reviewed_commit_sha: 0123abc\n" + "- prior_verdict: NACK\n" + "- prior_nack_reasons: missing tests\n" + "- prior_conditional_obligation: -\n" + "- summary_of_assessment: needs revision.\n" +) + + +def test_build_delta_entries_scopes_delta_to_proposal_sha() -> None: + """When pending_reviews carries the producer's proposal_commit_sha, + the re-review delta runs ``{last_reviewed}..{proposal_sha}`` — NOT + ``..HEAD``. ``{sha}..HEAD`` in the REVIEWER's worktree is empty even + after the producer revises (the "re-review delta is empty" phantom + NACK on pipeline-2b3d8b0b, #3076).""" + from pathlib import Path + from unittest.mock import patch + + from orchestrator.routes.event_prompt import _build_delta_entries + + with patch("orchestrator.routes.event_prompt._run_git_log", return_value="(diff)") as mock_log: + entries = _build_delta_entries( + action="nack", + role="reviewer_code", + base_branch="main", + repo_path=Path("/tmp"), + memory_text=_COD_MEMORY_WITH_SHA, + event_payload={ + "pending_reviews": [ + { + "producer": "coder", + "current_version": 2, + "artifact_refs": ["src/x.py"], + "proposal_commit_sha": "def5678", + } + ] + }, + ) + assert len(entries) == 1 + assert entries[0]["proposal_commit_sha"] == "def5678" + mock_log.assert_called_once() + assert mock_log.call_args.kwargs.get("end_ref") == "def5678", ( + f"delta must end at the proposal SHA, not HEAD — call was {mock_log.call_args!r}" + ) + + +def test_build_delta_entries_legacy_payload_keeps_head_endpoint() -> None: + """Payloads without proposal_commit_sha keep the pre-#3076 HEAD + endpoint — backward compatibility for legacy / synthetic callers.""" + from pathlib import Path + from unittest.mock import patch + + from orchestrator.routes.event_prompt import _build_delta_entries + + with patch("orchestrator.routes.event_prompt._run_git_log", return_value="(diff)") as mock_log: + _build_delta_entries( + action="ack", + role="reviewer_code", + base_branch="main", + repo_path=Path("/tmp"), + memory_text=_COD_MEMORY_WITH_SHA, + event_payload={"pending_reviews": [{"producer": "coder", "current_version": 1}]}, + ) + assert mock_log.call_args.kwargs.get("end_ref") == "HEAD" + + +def test_build_delta_entries_first_review_renders_git_show_commands() -> None: + """First review (no stored SHA) with a proposal SHA renders concrete, + working read commands — the producer's work is not in the reviewer's + working tree, and a plain Read of the path is expected to fail.""" + from pathlib import Path + + from orchestrator.routes.event_prompt import _build_delta_entries + + entries = _build_delta_entries( + action="ack", + role="risk_analyst", + base_branch="main", + repo_path=Path("/tmp"), + memory_text="", + event_payload={ + "pending_reviews": [ + { + "producer": "architect", + "current_version": 1, + "artifact_refs": [".egg-state/drafts/p-plan.md"], + "proposal_commit_sha": "b521d7d", + } + ] + }, + ) + assert len(entries) == 1 + entry = entries[0] + assert entry["proposal_commit_sha"] == "b521d7d" + delta = entry["delta"] + assert "git show b521d7d:.egg-state/drafts/p-plan.md" in delta + assert "git log b521d7d --not origin/main -p" in delta + assert "NOT in your working tree" in delta + # The anti-phantom-NACK instruction is the point of the render. + assert "Do NOT NACK" in delta + + # A 40-char SHA must render identically — guards against the + # SHA-validation regex regressing to a fixed ``{7,7}`` width and + # silently dropping full-length commit SHAs (reviewer suggestion #4 + # on #3078). + full_sha = "b521d7d0123456789abcdef0123456789abcdef0" + entries_full = _build_delta_entries( + action="ack", + role="risk_analyst", + base_branch="main", + repo_path=Path("/tmp"), + memory_text="", + event_payload={ + "pending_reviews": [ + { + "producer": "architect", + "current_version": 1, + "artifact_refs": [".egg-state/drafts/p-plan.md"], + "proposal_commit_sha": full_sha, + } + ] + }, + ) + assert len(entries_full) == 1 + delta_full = entries_full[0]["delta"] + assert f"git show {full_sha}:.egg-state/drafts/p-plan.md" in delta_full + assert f"git log {full_sha} --not origin/main -p" in delta_full + + +def test_build_delta_entries_first_review_sha_without_artifacts_still_renders() -> None: + """A proposal SHA with an empty artifact list still yields an entry + (the full-change git log command) — pre-#3076 the empty artifact + list silently dropped the producer from the section.""" + from pathlib import Path + + from orchestrator.routes.event_prompt import _build_delta_entries + + entries = _build_delta_entries( + action="ack", + role="reviewer_code", + base_branch="main", + repo_path=Path("/tmp"), + memory_text="", + event_payload={ + "pending_reviews": [ + { + "producer": "coder", + "current_version": 1, + "artifact_refs": [], + "proposal_commit_sha": "abc1234", + } + ] + }, + ) + assert len(entries) == 1 + assert "git log abc1234 --not origin/main -p" in entries[0]["delta"] + + +def test_extract_proposal_sha_rejects_non_hex_tokens() -> None: + """The SHA is interpolated into rendered shell commands — anything + that is not a 7-64 char hex token must be discarded, falling back to + the legacy degraded-baseline path.""" + from orchestrator.routes.event_prompt import _extract_proposal_sha_for_producer + + def payload(sha): + return {"pending_reviews": [{"producer": "coder", "proposal_commit_sha": sha}]} + + assert _extract_proposal_sha_for_producer(payload("abc1234"), "coder") == "abc1234" + assert _extract_proposal_sha_for_producer(payload("ABC1234DEF"), "coder") == "ABC1234DEF" + for bad in ("$(rm -rf /)", "HEAD", "abc123", "abc1234..def5678", "", None, 42): + assert _extract_proposal_sha_for_producer(payload(bad), "coder") == "", ( + f"non-hex token {bad!r} must be discarded" + ) + + +def test_render_delta_section_shows_proposal_sha_and_cautions_on_head() -> None: + """The rendered section substitutes the proposal SHA into the verbatim + command and only trusts an empty range when it was proposal-scoped; + an empty HEAD-scoped range gets an explicit caution instead of the + "re-review is a no-op" verdict.""" + from orchestrator.routes.event_prompt import _render_producer_delta_section + + # Proposal-scoped: empty range is a trustworthy no-op. + section, _ = _render_producer_delta_section( + [ + { + "producer": "coder", + "last_reviewed_commit_sha": "0123abc", + "proposal_commit_sha": "def5678", + "delta": "", + } + ], + "main", + ) + assert "git log 0123abc..def5678 --not origin/main -p" in section + assert "proposal_commit_sha: `def5678`" in section + assert "re-review is a no-op" in section + assert "CAUTION" not in section + + # HEAD-scoped (legacy): empty range is NOT evidence — caution required. + section, _ = _render_producer_delta_section( + [ + { + "producer": "coder", + "last_reviewed_commit_sha": "0123abc", + "proposal_commit_sha": "", + "delta": "", + } + ], + "main", + ) + assert "git log 0123abc..HEAD --not origin/main -p" in section + assert "CAUTION" in section + assert "NOT evidence" in section + + # --------------------------------------------------------------------------- # CLI tests: the wrapper-bash entry-point (NACK #2 from reviewer_contract — # plan TASK-3-2 acceptance "snapshot test verifies both branches" for @@ -1462,12 +1688,24 @@ def test_producer_delta_empty_string_renders_no_commits_sentinel() -> None: This is the real production state when a producer ACKs a confirmed proposal and re-confirms without any new commits between the - ``last_reviewed_commit_sha`` and HEAD. The agent must see the - sentinel so they know the re-review is a no-op rather than + ``last_reviewed_commit_sha`` and the proposal SHA. The agent must + see the sentinel so they know the re-review is a no-op rather than silently reading a blank diff block. + + #3076: the trustworthy no-op verdict requires the range to have + been scoped to the producer's ``proposal_commit_sha``. An empty + HEAD-scoped range (legacy payloads) gets an explicit caution + instead — the reviewer's own HEAD never contains the producer's + commits, so emptiness there is NOT evidence of no revision (the + "re-review delta is empty" phantom NACK). """ deltas = [ - {"producer": "coder", "last_reviewed_commit_sha": "abc1234", "delta": " \n\n"}, + { + "producer": "coder", + "last_reviewed_commit_sha": "abc1234", + "proposal_commit_sha": "def5678", + "delta": " \n\n", + }, ] prompt = compose_event_prompt( "reviewer_code", @@ -1479,6 +1717,21 @@ def test_producer_delta_empty_string_renders_no_commits_sentinel() -> None: ) assert "(no commits in range — re-review is a no-op)" in prompt + # Legacy HEAD-scoped empty range: caution, not a no-op verdict. + deltas = [ + {"producer": "coder", "last_reviewed_commit_sha": "abc1234", "delta": " \n\n"}, + ] + prompt = compose_event_prompt( + "reviewer_code", + {"action": "ack"}, + "", + [], + deltas, + "main", + ) + assert "re-review is a no-op" not in prompt + assert "CAUTION" in prompt + def test_producer_delta_missing_keys_renders_defensive_defaults() -> None: """Missing ``producer`` / ``sha`` / ``delta`` keys do not crash. diff --git a/orchestrator/tests/test_conditional_ack.py b/orchestrator/tests/test_conditional_ack.py index 4b49e1fa28..6ca0aaafed 100644 --- a/orchestrator/tests/test_conditional_ack.py +++ b/orchestrator/tests/test_conditional_ack.py @@ -241,7 +241,7 @@ def test_handle_ack_persists_condition(self, matrix_graph): { "summary": "impl", "artifacts": ["src/a.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", }, ) result = tracker.handle_ack( @@ -272,7 +272,7 @@ def test_handle_ack_whitespace_condition_excluded_from_event_and_result(self, ma { "summary": "impl", "artifacts": ["src/a.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", }, ) result = tracker.handle_ack( @@ -294,7 +294,7 @@ def test_handle_ack_without_condition_is_unchanged(self, matrix_graph): tracker.register_agent("reviewer_contract") tracker.handle_propose( "coder", - {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc"}, + {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc1234"}, ) result = tracker.handle_ack( "reviewer_code", @@ -320,7 +320,7 @@ def test_evaluate_includes_conditions_when_present(self, matrix_graph): tracker.register_agent("reviewer_contract") tracker.handle_propose( "coder", - {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc"}, + {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc1234"}, ) tracker.handle_ack( "reviewer_code", @@ -345,7 +345,7 @@ def test_evaluate_empty_list_when_no_conditions(self, matrix_graph): tracker.register_agent("reviewer_contract") tracker.handle_propose( "coder", - {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc"}, + {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc1234"}, ) tracker.handle_ack( "reviewer_code", @@ -367,7 +367,7 @@ def test_section_rendered_when_conditions_exist(self, matrix_graph): tracker.register_agent("reviewer_contract") tracker.handle_propose( "coder", - {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc"}, + {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc1234"}, ) tracker.handle_ack( "reviewer_code", @@ -409,7 +409,7 @@ def test_section_empty_when_no_conditions(self, matrix_graph): tracker.register_agent("reviewer_contract") tracker.handle_propose( "coder", - {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc"}, + {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc1234"}, ) tracker.handle_ack( "reviewer_code", @@ -474,7 +474,7 @@ def _register_tracker_with_proposal(self): tracker.register_agent("reviewer_contract") tracker.handle_propose( "coder", - {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc123"}, + {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc1234"}, ) return tracker @@ -624,7 +624,7 @@ def test_condition_survives_message_store_replay(self): "payload": { "summary": "impl", "artifacts": ["src/a.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", } }, timestamp=base, @@ -676,7 +676,7 @@ def test_resolution_survives_message_store_replay(self): "payload": { "summary": "impl", "artifacts": ["src/a.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", } }, timestamp=base, @@ -1084,7 +1084,7 @@ def test_handle_resolve_obligation_filters_conditions(self, matrix_graph): tracker.register_agent("reviewer_contract") tracker.handle_propose( "coder", - {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc"}, + {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc1234"}, ) tracker.handle_ack( "reviewer_contract", @@ -1129,7 +1129,7 @@ def test_handle_resolve_obligation_no_active_obligation(self, matrix_graph): tracker.register_agent("reviewer_contract") tracker.handle_propose( "coder", - {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc"}, + {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc1234"}, ) # Unconditional ACK — no obligation to resolve. tracker.handle_ack( @@ -1170,7 +1170,7 @@ def _set_tracker(self, matrix_graph): tracker.register_agent("reviewer_contract") tracker.handle_propose( "coder", - {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc"}, + {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc1234"}, ) tracker.handle_ack( "reviewer_contract", @@ -1329,7 +1329,7 @@ def test_no_active_obligation_returns_400(self, app, matrix_graph): tracker.register_agent("reviewer_contract") tracker.handle_propose( "coder", - {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc"}, + {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc1234"}, ) tracker.handle_ack( "reviewer_contract", diff --git a/orchestrator/tests/test_conditional_ack_hitl_gate.py b/orchestrator/tests/test_conditional_ack_hitl_gate.py index 9e8f393f8a..fa7d5f9cf9 100644 --- a/orchestrator/tests/test_conditional_ack_hitl_gate.py +++ b/orchestrator/tests/test_conditional_ack_hitl_gate.py @@ -77,7 +77,7 @@ def _make_tracker(graph, condition="git mv legacy/x new/x before merge"): tracker.register_agent("reviewer_contract") tracker.handle_propose( "coder", - {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc"}, + {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc1234"}, ) if condition: tracker.handle_ack( @@ -729,7 +729,7 @@ def test_resolved_via_live_tracker(self, graph): tracker.register_agent("reviewer_contract") tracker.handle_propose( "coder", - {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc"}, + {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc1234"}, ) tracker.handle_ack( "reviewer_code", diff --git a/orchestrator/tests/test_consensus_next_action.py b/orchestrator/tests/test_consensus_next_action.py index 4a2d7d3c7e..f597eb58c0 100644 --- a/orchestrator/tests/test_consensus_next_action.py +++ b/orchestrator/tests/test_consensus_next_action.py @@ -664,3 +664,28 @@ def test_next_action_reviewer_pending_reviews_includes_artifact_refs(client, sim f"artifact_refs must mirror the producer's current proposal " f"artifacts (a.py from _propose) — got {entry['artifact_refs']!r}" ) + + +def test_next_action_reviewer_pending_reviews_includes_proposal_commit_sha(client, simple_tracker): + """Reviewer-side pending_reviews entries must surface the producer's + proposed commit SHA (#3076). + + The composer scopes the re-review delta to + ``{last_reviewed}..{proposal_commit_sha}`` and renders + ``git show :`` reads from it — ``{sha}..HEAD`` against + the REVIEWER's own HEAD never contains the producer's commits + (per-role worktrees), which produced the "re-review delta is empty" + phantom NACK on pipeline-2b3d8b0b. + """ + _propose(simple_tracker, "coder") + resp = _post_next_action(client, "reviewer_code", tracker=simple_tracker) + assert resp.status_code == 200 + data = json.loads(resp.data) + payload = data.get("data", data) + pending = (payload.get("event_payload") or {}).get("pending_reviews") or [] + assert len(pending) >= 1 + entry = pending[0] + assert entry.get("proposal_commit_sha") == "abc1234", ( + f"pending_reviews entry must carry the producer's proposed commit " + f"SHA (abc1234 from _propose) — entry={entry!r}" + ) diff --git a/orchestrator/tests/test_health_monitor.py b/orchestrator/tests/test_health_monitor.py index e881810cd0..2f48de550a 100644 --- a/orchestrator/tests/test_health_monitor.py +++ b/orchestrator/tests/test_health_monitor.py @@ -2607,7 +2607,7 @@ def test_no_alert_while_peer_producer_has_zero_proposal(self): # level. The advisory ACK below makes that explicit. tracker.handle_propose( "documenter", - {"summary": "docs", "artifacts": ["docs/README.md"], "commit_sha": "abc"}, + {"summary": "docs", "artifacts": ["docs/README.md"], "commit_sha": "abc1234"}, ) tracker.handle_ack( "reviewer_code", @@ -2657,7 +2657,7 @@ def test_alert_fires_after_peers_finally_propose(self): tracker = self._build_tracker() tracker.handle_propose( "documenter", - {"summary": "docs", "artifacts": ["docs/README.md"], "commit_sha": "abc"}, + {"summary": "docs", "artifacts": ["docs/README.md"], "commit_sha": "abc1234"}, ) tracker.handle_ack( "reviewer_code", @@ -2684,11 +2684,11 @@ def test_alert_fires_after_peers_finally_propose(self): # Peers finally propose — global guard clears. tracker.handle_propose( "coder", - {"summary": "code", "artifacts": ["src/m.py"], "commit_sha": "def"}, + {"summary": "code", "artifacts": ["src/m.py"], "commit_sha": "def5678"}, ) tracker.handle_propose( "tester", - {"summary": "tests", "artifacts": ["tests/t.py"], "commit_sha": "ghi"}, + {"summary": "tests", "artifacts": ["tests/t.py"], "commit_sha": "9111111"}, ) # Critical reviewers ACK coder and tester (default implement graph). for reviewer in ( @@ -2747,7 +2747,7 @@ def test_single_producer_phase_unaffected(self): tracker.register_agent("reviewer_solo") tracker.handle_propose( "solo_producer", - {"summary": "solo", "artifacts": ["a.py"], "commit_sha": "abc"}, + {"summary": "solo", "artifacts": ["a.py"], "commit_sha": "abc1234"}, ) tracker.handle_ack( "reviewer_solo", diff --git a/orchestrator/tests/test_messages.py b/orchestrator/tests/test_messages.py index 4bb737caeb..f1a30d14bf 100644 --- a/orchestrator/tests/test_messages.py +++ b/orchestrator/tests/test_messages.py @@ -87,7 +87,7 @@ def test_propose_redacted_for_unreviewed_reviewer(self, client, app): { "summary": "Implemented auth", "artifacts": ["src/auth.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", }, ) @@ -106,7 +106,7 @@ def test_propose_redacted_for_unreviewed_reviewer(self, client, app): "summary": "Implemented auth", "artifacts": ["src/auth.py"], "version": 1, - "commit_sha": "abc123", + "commit_sha": "abc1234", } }, ) @@ -132,7 +132,7 @@ def test_propose_redacted_for_unreviewed_reviewer(self, client, app): assert msg["metadata"]["delphi_redacted"] is True # Payload should only contain version and commit_sha assert msg["metadata"]["payload"]["version"] == 1 - assert msg["metadata"]["payload"]["commit_sha"] == "abc123" + assert msg["metadata"]["payload"]["commit_sha"] == "abc1234" assert "summary" not in msg["metadata"]["payload"] assert "artifacts" not in msg["metadata"]["payload"] @@ -155,7 +155,7 @@ def test_propose_visible_after_reviewer_evaluates(self, client, app): { "summary": "Implemented auth", "artifacts": ["src/auth.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", }, ) @@ -182,7 +182,7 @@ def test_propose_visible_after_reviewer_evaluates(self, client, app): "summary": "Implemented auth", "artifacts": ["src/auth.py"], "version": 1, - "commit_sha": "abc123", + "commit_sha": "abc1234", } }, ) @@ -228,7 +228,7 @@ def test_non_reviewer_sees_propose(self, client, app): { "summary": "Implemented auth", "artifacts": ["src/auth.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", }, ) @@ -279,7 +279,7 @@ def test_propose_redacted_preserves_header(self, client, app): { "summary": "Implemented auth", "artifacts": ["src/auth.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", }, ) @@ -297,7 +297,7 @@ def test_propose_redacted_preserves_header(self, client, app): "summary": "Implemented auth module", "artifacts": ["src/auth.py"], "version": 2, - "commit_sha": "def456", + "commit_sha": "def5678", } }, ) @@ -328,7 +328,7 @@ def test_propose_redacted_preserves_header(self, client, app): # Payload: version and commit_sha preserved assert msg["metadata"]["payload"]["version"] == 2 - assert msg["metadata"]["payload"]["commit_sha"] == "def456" + assert msg["metadata"]["payload"]["commit_sha"] == "def5678" # Body and sensitive payload fields stripped assert msg["body"] == "" @@ -355,7 +355,7 @@ def test_propose_redacted_without_payload_in_metadata(self, client, app): { "summary": "Implemented auth", "artifacts": ["src/auth.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", }, ) @@ -415,7 +415,7 @@ def test_redacted_propose_does_not_mutate_original(self, client, app): { "summary": "Implemented auth", "artifacts": ["src/auth.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", }, ) @@ -425,7 +425,7 @@ def test_redacted_propose_does_not_mutate_original(self, client, app): "summary": "Implemented auth", "artifacts": ["src/auth.py"], "version": 1, - "commit_sha": "abc123", + "commit_sha": "abc1234", } } @@ -530,7 +530,7 @@ def test_multiple_proposals_mixed_redaction(self, client, app): { "summary": "Code implementation", "artifacts": ["src/auth.py"], - "commit_sha": "code123", + "commit_sha": "c0de123", }, ) @@ -544,7 +544,7 @@ def test_multiple_proposals_mixed_redaction(self, client, app): subject="Proposal from coder", body="Coder self-assessment", metadata={ - "payload": {"summary": "Code impl", "version": 1, "commit_sha": "code123"} + "payload": {"summary": "Code impl", "version": 1, "commit_sha": "c0de123"} }, ) ) @@ -557,7 +557,7 @@ def test_multiple_proposals_mixed_redaction(self, client, app): subject="Proposal from tester", body="Tester self-assessment", metadata={ - "payload": {"summary": "Test results", "version": 1, "commit_sha": "test123"} + "payload": {"summary": "Test results", "version": 1, "commit_sha": "7e57123"} }, ) ) diff --git a/orchestrator/tests/test_no_op_propose.py b/orchestrator/tests/test_no_op_propose.py index dd1aaabb16..2241f91729 100644 --- a/orchestrator/tests/test_no_op_propose.py +++ b/orchestrator/tests/test_no_op_propose.py @@ -63,11 +63,78 @@ def test_no_op_blank_reason_rejected(self): def test_real_proposal_still_requires_artifacts_and_commit(self): """The exemption is scoped to no-op — a normal proposal is unchanged.""" with pytest.raises(ValueError, match="artifact"): - ProposalPayload(summary="x" * 30, commit_sha="abc123") + ProposalPayload(summary="x" * 30, commit_sha="abc1234") with pytest.raises(ValueError, match="commit_sha"): ProposalPayload(summary="x" * 30, artifacts=["src/a.py"]) +# --- ProposalPayload.commit_sha format ------------------------------------ + + +class TestProposalPayloadCommitShaFormat: + """The ``commit_sha`` field is regex-validated at the writer (#3076). + + Defense-in-depth for downstream consumers that read + ``_proposal_commit_shas`` directly without revalidating — + ``_extract_proposal_sha_for_producer`` already gates the read path + in ``orchestrator/routes/event_prompt.py`` with stricter hex-only + matching, but enforcing a shell-safe baseline at the writer + prevents a future consumer from re-introducing a shell-injection + gap. The writer regex is intentionally more permissive than the + read regex so reconstruction sentinels like + ``RECONSTRUCTED_NO_SHA`` (peer_consensus.py:2134) round-trip. + """ + + def test_hex_commit_sha_accepted(self): + ProposalPayload(summary="x" * 30, artifacts=["src/a.py"], commit_sha="abc1234") + ProposalPayload( + summary="x" * 30, + artifacts=["src/a.py"], + commit_sha="0" * 40, + ) + + def test_reconstruction_sentinel_accepted(self): + """The ``RECONSTRUCTED_NO_SHA`` sentinel (peer_consensus.py:2134) + rounds through the validator unchanged — the writer regex is + permissive enough to admit it, downstream consumers key on the + exact string.""" + payload = ProposalPayload( + summary="x" * 30, + artifacts=["src/a.py"], + commit_sha="RECONSTRUCTED_NO_SHA", + ) + assert payload.commit_sha == "RECONSTRUCTED_NO_SHA" + + @pytest.mark.parametrize( + "bad_sha", + [ + "abc", # too short (< 7 chars) + "abc123", # 6 chars, still too short + "$(rm -rf /)", # shell injection attempt + "abc1234..def5678", # range syntax (dots forbidden) + "abc1234 def5678", # whitespace + "abc-1234", # hyphen + "abc/1234", # slash + "`whoami`", # backtick injection + "abc;rm", # semicolon + "abc|rm", # pipe + ], + ) + def test_shell_unsafe_commit_sha_rejected(self, bad_sha): + with pytest.raises(ValueError, match="commit_sha"): + ProposalPayload(summary="x" * 30, artifacts=["src/a.py"], commit_sha=bad_sha) + + def test_no_op_skips_format_check(self): + """A no-op proposal has empty commit_sha by design — skip the + format validator (the present-check already returns early).""" + payload = ProposalPayload( + summary="x" * 30, + no_changes_needed=True, + no_changes_reason="no work in this slice", + ) + assert payload.commit_sha == "" + + # --- ApprovalMatrix no-op semantics --------------------------------------- @@ -269,7 +336,7 @@ def test_tests_blocked_alone_is_fine(self): payload = ProposalPayload( summary="x" * 30, artifacts=["src/a.py"], - commit_sha="abc123", + commit_sha="abc1234", attestation={ "tests_execution_blocked": True, "tests_execution_blocked_reason": "no network", diff --git a/orchestrator/tests/test_peer_consensus_integration.py b/orchestrator/tests/test_peer_consensus_integration.py index 744e07b2c4..75e425d632 100644 --- a/orchestrator/tests/test_peer_consensus_integration.py +++ b/orchestrator/tests/test_peer_consensus_integration.py @@ -58,9 +58,9 @@ def test_full_lifecycle(self, tracker): { "summary": "Implemented auth", "artifacts": ["src/auth.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", "attestation": { - "commit_shas": ["abc123"], + "commit_shas": ["abc1234"], "files_changed": ["src/auth.py"], "test_summary": "All pass", "risk_considered": "None", @@ -76,7 +76,7 @@ def test_full_lifecycle(self, tracker): { "summary": "Added tests", "artifacts": ["tests/test_auth.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", "attestation": { "tests_written": 5, "tests_run": 5, @@ -158,7 +158,7 @@ def test_nack_re_propose_cycle(self, tracker): { "summary": "v1", "artifacts": ["src/auth.py", "src/utils.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", }, ) @@ -189,7 +189,7 @@ def test_nack_re_propose_cycle(self, tracker): { "summary": "Fixed SQL injection", "artifacts": ["src/auth.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", }, changed_artifacts=["src/auth.py"], ) @@ -200,7 +200,7 @@ def test_nack_re_propose_cycle(self, tracker): def test_nack_reason_required(self, tracker): tracker.handle_propose( - "coder", {"summary": "v1", "artifacts": ["src/a.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["src/a.py"], "commit_sha": "abc1234"} ) with pytest.raises(ValueError, match="reason"): tracker.handle_nack( @@ -223,18 +223,18 @@ def test_flip_flop_lockout(self): t.register_agent("reviewer_code") # First proposal + withdrawal - t.handle_propose("coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc123"}) + t.handle_propose("coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc1234"}) t.handle_withdraw("coder", "Changed approach") # Second proposal + withdrawal -- should trigger lockout - t.handle_propose("coder", {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "abc123"}) + t.handle_propose("coder", {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "abc1234"}) result = t.handle_withdraw("coder", "Changed again") assert result["status"] == "locked_out" assert result["needs_escalation"] is True def test_withdrawal_requires_reason(self, tracker): tracker.handle_propose( - "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc1234"} ) with pytest.raises(ValueError, match="reason"): tracker.handle_withdraw("coder", "") @@ -246,13 +246,13 @@ def test_bounded_revision_rounds(self): t.register_agent("reviewer_code") # First round - t.handle_propose("coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc123"}) + t.handle_propose("coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc1234"}) t.handle_nack( "reviewer_code", "coder", {"artifact_references": ["a.py"], "reason": "bug 1"} ) # Second round - t.handle_propose("coder", {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "abc123"}) + t.handle_propose("coder", {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "abc1234"}) result = t.handle_nack( "reviewer_code", "coder", {"artifact_references": ["a.py"], "reason": "bug 2"} ) @@ -264,7 +264,7 @@ class TestTimeoutHandling: def test_timeout_critical_blocker_escalates(self, tracker): tracker.handle_propose( - "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc1234"} ) # Only reviewer_code ACKs, reviewer_contract doesn't tracker.handle_ack("reviewer_code", "coder", {"artifact_references": ["a.py"]}) @@ -285,7 +285,7 @@ def test_timeout_advisory_only_proceeds(self): t.register_agent("reviewer_code") t.register_agent("reviewer_contract") - t.handle_propose("coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc123"}) + t.handle_propose("coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc1234"}) t.handle_ack("reviewer_code", "coder", {"artifact_references": ["a.py"]}) # reviewer_contract hasn't ACKed but is advisory @@ -308,7 +308,7 @@ def test_returns_none_when_no_activity(self, tracker): def test_returns_proposal_timestamp_when_only_proposals(self, tracker): tracker.handle_propose( - "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc1234"} ) proposal_ts = tracker.get_latest_proposal_timestamp() progress_ts = tracker.get_latest_progress_timestamp() @@ -317,7 +317,7 @@ def test_returns_proposal_timestamp_when_only_proposals(self, tracker): def test_ack_advances_progress_timestamp_past_proposal(self, tracker): tracker.handle_propose( - "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc1234"} ) proposal_ts = tracker.get_latest_proposal_timestamp() # ACK happens after proposal — progress should advance strictly @@ -346,7 +346,7 @@ def test_reviewer_crash_sole_reviewer(self): def test_producer_crash_preserves_proposal(self, tracker): tracker.handle_propose( - "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc1234"} ) result = tracker.handle_agent_crash("coder") assert result["action"] == "continue" @@ -358,7 +358,7 @@ class TestDelphiOrdering: def test_reviewer_without_evaluation_hidden(self, tracker): """Verify the matrix tracks evaluation status correctly.""" tracker.handle_propose( - "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc1234"} ) # Before review, reviewer hasn't evaluated @@ -613,14 +613,14 @@ def test_concurrent_nacks_different_producers(self, four_agent_tracker): # Both producers propose t.handle_propose( - "coder", {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"} ) t.handle_propose( "tester", { "summary": "tests v1", "artifacts": ["tests/test_auth.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", "attestation": {"tests_run": 5, "checks_passed": ["test"]}, }, ) @@ -645,7 +645,7 @@ def test_concurrent_nacks_different_producers(self, four_agent_tracker): # Coder re-proposes — only rev_code needs to re-review result = t.handle_re_propose( "coder", - {"summary": "fixed auth", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "fixed auth", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, changed_artifacts=["src/auth.py"], ) # rev_contract ACKed auth.py, which is the changed artifact, @@ -661,7 +661,7 @@ def test_overlapping_artifact_invalidation(self, four_agent_tracker): { "summary": "v1", "artifacts": ["src/auth.py", "src/utils.py", "src/db.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", }, ) @@ -687,14 +687,14 @@ def test_cascading_re_propose_preserves_unaffected_acks(self, four_agent_tracker # Both producers propose t.handle_propose( - "coder", {"summary": "code v1", "artifacts": ["src/auth.py"], "commit_sha": "abc123"} + "coder", {"summary": "code v1", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"} ) t.handle_propose( "tester", { "summary": "tests v1", "artifacts": ["tests/test_auth.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", "attestation": {"tests_run": 5, "checks_passed": ["test"]}, }, ) @@ -710,7 +710,7 @@ def test_cascading_re_propose_preserves_unaffected_acks(self, four_agent_tracker ) t.handle_re_propose( "coder", - {"summary": "code v2", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "code v2", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, changed_artifacts=["src/auth.py"], ) @@ -725,7 +725,11 @@ def test_concurrent_nack_and_re_propose_race(self, four_agent_tracker): t.handle_propose( "coder", - {"summary": "v1", "artifacts": ["src/auth.py", "src/utils.py"], "commit_sha": "abc123"}, + { + "summary": "v1", + "artifacts": ["src/auth.py", "src/utils.py"], + "commit_sha": "abc1234", + }, ) # rev_contract ACKs (referencing utils.py only) @@ -740,7 +744,7 @@ def test_concurrent_nack_and_re_propose_race(self, four_agent_tracker): # Coder re-proposes, changing only auth.py result = t.handle_re_propose( "coder", - {"summary": "v2", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "v2", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, changed_artifacts=["src/auth.py"], ) @@ -759,7 +763,7 @@ def test_context_change_nack_not_escalated(self): # Round 1: propose, NACK on file A t.handle_propose( - "coder", {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"} ) r1 = t.handle_nack( "rev_code", @@ -773,7 +777,7 @@ def test_context_change_nack_not_escalated(self): # Round 2: fix and re-propose, reviewer ACKs t.handle_re_propose( "coder", - {"summary": "v2 - fixed auth", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "v2 - fixed auth", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, changed_artifacts=["src/auth.py"], ) t.handle_ack("rev_code", "coder", {"artifact_references": ["src/auth.py"]}) @@ -784,7 +788,7 @@ def test_context_change_nack_not_escalated(self): { "summary": "v3 - includes tester changes", "artifacts": ["src/auth.py", "src/db.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", }, changed_artifacts=["src/db.py"], ) @@ -816,16 +820,16 @@ def test_full_implement_graph(self): { "summary": "Implementation", "artifacts": ["src/main.py", "src/utils.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", }, ) t.handle_propose( "tester", - {"summary": "Tests", "artifacts": ["tests/test_main.py"], "commit_sha": "abc123"}, + {"summary": "Tests", "artifacts": ["tests/test_main.py"], "commit_sha": "abc1234"}, ) t.handle_propose( "documenter", - {"summary": "Docs", "artifacts": ["docs/README.md"], "commit_sha": "abc123"}, + {"summary": "Docs", "artifacts": ["docs/README.md"], "commit_sha": "abc1234"}, ) # All reviewers ACK coder @@ -884,7 +888,7 @@ def test_full_implement_graph(self): { "summary": "Fixed utils", "artifacts": ["src/main.py", "src/utils.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", }, changed_artifacts=["src/utils.py"], ) @@ -957,7 +961,7 @@ def test_handle_timeout_idempotent(self): # Propose but don't ACK — creates a blocking edge t.handle_propose( - "coder", {"summary": "v1", "artifacts": ["src/main.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["src/main.py"], "commit_sha": "abc1234"} ) result1 = t.handle_timeout() @@ -976,7 +980,7 @@ def test_handle_timeout_idempotent_advisory(self): t.register_agent("rev_code") t.handle_propose( - "coder", {"summary": "v1", "artifacts": ["src/main.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["src/main.py"], "commit_sha": "abc1234"} ) result1 = t.handle_timeout() @@ -1003,7 +1007,7 @@ def test_alternating_file_nacks_escalate_at_hard_cap(self): # Alternate NACKs on different files until we hit the cap for i in range(6): t.handle_propose( - "coder", {"summary": f"v{i + 1}", "artifacts": files, "commit_sha": "abc123"} + "coder", {"summary": f"v{i + 1}", "artifacts": files, "commit_sha": "abc1234"} ) result = t.handle_nack( "rev_code", @@ -1060,7 +1064,7 @@ def test_withdraw_after_partial_confirm_unconfirms_stale_reviewer(self, refine_t # v1: refiner proposes t.handle_propose( - "refiner", {"summary": "v1", "artifacts": ["design.md"], "commit_sha": "abc123"} + "refiner", {"summary": "v1", "artifacts": ["design.md"], "commit_sha": "abc1234"} ) # reviewer_agent_design ACKs and confirms on v1 @@ -1081,7 +1085,7 @@ def test_withdraw_after_partial_confirm_unconfirms_stale_reviewer(self, refine_t { "summary": "v2 - added error handling", "artifacts": ["design.md"], - "commit_sha": "abc123", + "commit_sha": "abc1234", }, changed_artifacts=["design.md"], ) @@ -1095,7 +1099,8 @@ def test_withdraw_after_partial_confirm_unconfirms_stale_reviewer(self, refine_t # refiner re-proposes v3 (new proposal after withdrawal) result = t.handle_propose( - "refiner", {"summary": "v3 - final", "artifacts": ["design.md"], "commit_sha": "abc123"} + "refiner", + {"summary": "v3 - final", "artifacts": ["design.md"], "commit_sha": "abc1234"}, ) # The fix: reviewer_agent_design was already un-confirmed during @@ -1128,7 +1133,7 @@ def test_re_propose_after_withdraw_notifies_stale_reviewers(self, refine_tracker # Quick setup: propose, both ACK and confirm, then withdraw and re-propose t.handle_propose( - "refiner", {"summary": "v1", "artifacts": ["design.md"], "commit_sha": "abc123"} + "refiner", {"summary": "v1", "artifacts": ["design.md"], "commit_sha": "abc1234"} ) t.handle_ack("reviewer_agent_design", "refiner", {"artifact_references": ["design.md"]}) t.handle_ack("reviewer_refine", "refiner", {"artifact_references": ["design.md"]}) @@ -1138,7 +1143,7 @@ def test_re_propose_after_withdraw_notifies_stale_reviewers(self, refine_tracker # Withdraw and re-propose t.handle_withdraw("refiner", "Revised approach needed") result = t.handle_propose( - "refiner", {"summary": "v3", "artifacts": ["design.md"], "commit_sha": "abc123"} + "refiner", {"summary": "v3", "artifacts": ["design.md"], "commit_sha": "abc1234"} ) # Both reviewers should be in the stale list @@ -1150,7 +1155,7 @@ def test_no_stale_reviewers_on_first_proposal(self, refine_tracker): """First proposal should never have stale confirmed reviewers.""" t = refine_tracker result = t.handle_propose( - "refiner", {"summary": "v1", "artifacts": ["design.md"], "commit_sha": "abc123"} + "refiner", {"summary": "v1", "artifacts": ["design.md"], "commit_sha": "abc1234"} ) assert result["stale_reviewers"] == [] @@ -1160,7 +1165,7 @@ def test_re_propose_via_changed_artifacts_also_unconfirms(self, refine_tracker): t = refine_tracker t.handle_propose( - "refiner", {"summary": "v1", "artifacts": ["design.md"], "commit_sha": "abc123"} + "refiner", {"summary": "v1", "artifacts": ["design.md"], "commit_sha": "abc1234"} ) t.handle_ack("reviewer_agent_design", "refiner", {"artifact_references": ["design.md"]}) t.handle_confirmed("reviewer_agent_design") @@ -1175,7 +1180,7 @@ def test_re_propose_via_changed_artifacts_also_unconfirms(self, refine_tracker): # Re-propose with changed artifacts result = t.handle_re_propose( "refiner", - {"summary": "v2", "artifacts": ["design.md"], "commit_sha": "abc123"}, + {"summary": "v2", "artifacts": ["design.md"], "commit_sha": "abc1234"}, changed_artifacts=["design.md"], ) @@ -1189,7 +1194,7 @@ def test_producer_confirm_fails_without_re_review(self, refine_tracker): t = refine_tracker t.handle_propose( - "refiner", {"summary": "v1", "artifacts": ["design.md"], "commit_sha": "abc123"} + "refiner", {"summary": "v1", "artifacts": ["design.md"], "commit_sha": "abc1234"} ) t.handle_ack("reviewer_agent_design", "refiner", {"artifact_references": ["design.md"]}) t.handle_ack("reviewer_refine", "refiner", {"artifact_references": ["design.md"]}) @@ -1199,7 +1204,7 @@ def test_producer_confirm_fails_without_re_review(self, refine_tracker): # Withdraw and re-propose t.handle_withdraw("refiner", "Revised approach") t.handle_propose( - "refiner", {"summary": "v3", "artifacts": ["design.md"], "commit_sha": "abc123"} + "refiner", {"summary": "v3", "artifacts": ["design.md"], "commit_sha": "abc1234"} ) # Refiner should NOT be able to confirm (not fully ACKed on v3) @@ -1225,7 +1230,7 @@ def test_nacked_then_confirmed_reviewer_blocked_by_guard(self, refine_tracker): # v1: refiner proposes t.handle_propose( - "refiner", {"summary": "v1", "artifacts": ["design.md"], "commit_sha": "abc123"} + "refiner", {"summary": "v1", "artifacts": ["design.md"], "commit_sha": "abc1234"} ) # reviewer_agent_design ACKs v1 and confirms @@ -1247,7 +1252,7 @@ def test_nacked_then_confirmed_reviewer_blocked_by_guard(self, refine_tracker): # refiner withdraws and re-proposes v2 t.handle_withdraw("refiner", "Addressing NACK feedback") result = t.handle_propose( - "refiner", {"summary": "v2", "artifacts": ["design.md"], "commit_sha": "abc123"} + "refiner", {"summary": "v2", "artifacts": ["design.md"], "commit_sha": "abc1234"} ) # reviewer_agent_design was confirmed on v1, now stale on v2 @@ -1279,13 +1284,13 @@ def test_confirm_before_acked_returns_pending(self, tracker): { "summary": "Implemented feature", "artifacts": ["src/feature.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", }, ) # tester must also propose to pass global zero-proposal guard (#1648) tracker.handle_propose( "tester", - {"summary": "Tests", "artifacts": ["tests/test.py"], "commit_sha": "def456"}, + {"summary": "Tests", "artifacts": ["tests/test.py"], "commit_sha": "def5678"}, ) # Only reviewer_code ACKs, but reviewer_contract hasn't ACKed yet @@ -1316,7 +1321,7 @@ def test_confirm_after_re_propose_invalidates_stale_acks(self, tracker): # Full happy path first: propose, ACK, but then re-propose tracker.handle_propose( "coder", - {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, ) tracker.handle_ack( "reviewer_code", @@ -1332,7 +1337,7 @@ def test_confirm_after_re_propose_invalidates_stale_acks(self, tracker): # Coder re-proposes (invalidating stale ACKs) tracker.handle_re_propose( "coder", - {"summary": "v2", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "v2", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, changed_artifacts=["src/auth.py"], ) @@ -1355,7 +1360,7 @@ def test_reviewer_blocked_by_unassigned_producer(self, tracker): # Coder proposes tracker.handle_propose( "coder", - {"summary": "impl", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "impl", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, ) # reviewer_contract ACKs coder tracker.handle_ack( @@ -1374,7 +1379,7 @@ def test_producer_blocked_by_peer_producer(self, tracker): # Coder proposes tracker.handle_propose( "coder", - {"summary": "impl", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "impl", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, ) # All reviewers ACK coder tracker.handle_ack("reviewer_code", "coder", {"artifact_references": ["src/auth.py"]}) @@ -1392,11 +1397,11 @@ def test_guard_clears_after_all_propose(self, tracker): # Both producers propose tracker.handle_propose( "coder", - {"summary": "impl", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "impl", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, ) tracker.handle_propose( "tester", - {"summary": "tests", "artifacts": ["tests/test.py"], "commit_sha": "def456"}, + {"summary": "tests", "artifacts": ["tests/test.py"], "commit_sha": "def5678"}, ) # reviewer_contract ACKs coder tracker.handle_ack( @@ -1417,7 +1422,7 @@ def test_propose_rejected_when_fully_acked(self, tracker): # Coder proposes tracker.handle_propose( "coder", - {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, ) # Both reviewers ACK @@ -1431,14 +1436,14 @@ def test_propose_rejected_when_fully_acked(self, tracker): with pytest.raises(ValueError, match="already fully ACKed"): tracker.handle_propose( "coder", - {"summary": "v2", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "v2", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, ) def test_re_propose_allowed_after_nack(self, tracker): """handle_re_propose is allowed after NACK (producer phase is WORKING).""" tracker.handle_propose( "coder", - {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, ) tracker.handle_ack("reviewer_code", "coder", {"artifact_references": ["src/auth.py"]}) # reviewer_contract NACKs instead of ACKing @@ -1451,7 +1456,7 @@ def test_re_propose_allowed_after_nack(self, tracker): # Re-proposing after NACK should work (producer phase is WORKING) result = tracker.handle_re_propose( "coder", - {"summary": "v2", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "v2", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, changed_artifacts=["src/auth.py"], ) assert result["status"] == "proposed" @@ -1476,7 +1481,7 @@ def test_reviewer_crash_with_pending_ack_escalates(self): # Coder proposes t.handle_propose( "coder", - {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, ) # Only reviewer_code ACKs; reviewer_contract hasn't reviewed yet @@ -1522,7 +1527,7 @@ def test_reviewer_crash_with_completed_ack_continues(self): # Coder proposes t.handle_propose( "coder", - {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, ) # Both reviewers ACK @@ -1550,7 +1555,7 @@ def test_reviewer_crash_clears_confirmed_on_escalation(self): # Coder proposes t.handle_propose( "coder", - {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, ) # Only reviewer_code ACKs; reviewer_contract hasn't reviewed yet @@ -1585,7 +1590,7 @@ def test_excuse_reviewer_unblocks_consensus(self): # Coder proposes t.handle_propose( "coder", - {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, ) # Only reviewer_code ACKs @@ -1654,12 +1659,12 @@ def test_confirm_error_lists_pending_reviewers(self, tracker): """Premature confirm message lists which reviewers haven't ACKed.""" tracker.handle_propose( "coder", - {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, ) # tester must also propose to pass global zero-proposal guard (#1648) tracker.handle_propose( "tester", - {"summary": "Tests", "artifacts": ["tests/test.py"], "commit_sha": "def456"}, + {"summary": "Tests", "artifacts": ["tests/test.py"], "commit_sha": "def5678"}, ) # Only reviewer_code ACKs @@ -1725,14 +1730,14 @@ def test_sole_reviewer_crash_includes_blocking_producers(self): # Both producers propose t.handle_propose( - "coder", {"summary": "v1", "artifacts": ["src/a.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["src/a.py"], "commit_sha": "abc1234"} ) t.handle_propose( "tester", { "summary": "v1", "artifacts": ["test_a.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", "attestation": {"tests_run": 3, "checks_passed": ["test"]}, }, ) @@ -1919,7 +1924,11 @@ def test_reconstructs_partial_state(self, simple_graph): "coder", "all", metadata={ - "payload": {"summary": "wip", "artifacts": ["src/a.py"], "commit_sha": "abc123"} + "payload": { + "summary": "wip", + "artifacts": ["src/a.py"], + "commit_sha": "abc1234", + } }, timestamp=base, ), @@ -1945,7 +1954,7 @@ def test_registers_tracker_globally(self, simple_graph): "CONSENSUS_PROPOSE", "coder", "all", - metadata={"payload": {"summary": "x", "artifacts": [], "commit_sha": "abc123"}}, + metadata={"payload": {"summary": "x", "artifacts": [], "commit_sha": "abc1234"}}, timestamp=datetime.now(UTC), ), ] @@ -2201,7 +2210,7 @@ class TestACKGuardErrorMessage: def test_ack_guard_includes_confirmed_guidance(self, tracker): """Error message should tell the agent to call confirmed.""" tracker.handle_propose( - "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc1234"} ) tracker.handle_ack("reviewer_code", "coder", {"artifact_references": ["a.py"]}) @@ -2210,7 +2219,7 @@ def test_ack_guard_includes_confirmed_guidance(self, tracker): # Re-proposing when fully ACKed should raise with clear guidance with pytest.raises(ValueError, match="egg-orch consensus confirmed"): tracker.handle_propose( - "coder", {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "abc123"} + "coder", {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "abc1234"} ) @@ -2260,7 +2269,7 @@ def test_stall_demotion_non_reviewer_raises(self, dual_tracker): def test_stall_demotion_allows_consensus_without_stalled_ack(self, dual_tracker): """After demotion, consensus should proceed without the stalled agent's ACK.""" dual_tracker.handle_propose( - "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "abc1234"} ) # reviewer_code ACKs coder, but tester (stalled) does not ACK @@ -2325,7 +2334,7 @@ def test_pre_proposal_ack_then_confirm_deadlock_prevented(self, deadlock_tracker # Coder proposes (version 1) — should invalidate the version-0 ACK result = t.handle_propose( "coder", - {"summary": "Implemented auth", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "Implemented auth", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, ) assert result["version"] == 1 assert "reviewer_code" in result["stale_reviewers"] @@ -2363,7 +2372,7 @@ def test_pre_proposal_ack_invalidated_on_propose(self, deadlock_tracker): # Coder proposes — both version-0 ACKs should be invalidated result = t.handle_propose( "coder", - {"summary": "Implementation", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "Implementation", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, ) stale = result["stale_reviewers"] assert "reviewer_code" in stale @@ -2383,7 +2392,7 @@ def test_withdraw_and_re_propose_still_works(self, deadlock_tracker): # Normal flow: coder proposes, both ACK t.handle_propose( - "coder", {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"} ) t.handle_ack("reviewer_code", "coder", {"artifact_references": ["src/auth.py"]}) t.handle_ack("reviewer_contract", "coder", {"artifact_references": ["src/auth.py"]}) @@ -2396,7 +2405,7 @@ def test_withdraw_and_re_propose_still_works(self, deadlock_tracker): # Coder withdraws and re-proposes t.handle_withdraw("coder", "Need to update approach") result = t.handle_propose( - "coder", {"summary": "v2", "artifacts": ["src/auth.py"], "commit_sha": "def456"} + "coder", {"summary": "v2", "artifacts": ["src/auth.py"], "commit_sha": "def5678"} ) # Confirmed reviewers should be un-confirmed and listed as stale @@ -2419,7 +2428,7 @@ def test_reviewer_cannot_confirm_after_repropose_invalidates_ack(self, deadlock_ # Coder proposes v1, reviewer_code ACKs v1 t.handle_propose( - "coder", {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc123"} + "coder", {"summary": "v1", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"} ) t.handle_ack("reviewer_code", "coder", {"artifact_references": ["src/auth.py"]}) @@ -2431,7 +2440,7 @@ def test_reviewer_cannot_confirm_after_repropose_invalidates_ack(self, deadlock_ ) t.handle_re_propose( "coder", - {"summary": "v2", "artifacts": ["src/auth.py"], "commit_sha": "def456"}, + {"summary": "v2", "artifacts": ["src/auth.py"], "commit_sha": "def5678"}, changed_artifacts=["src/auth.py"], ) @@ -2470,7 +2479,7 @@ def test_confirmed_reviewer_with_pre_proposal_ack_unconfirmed(self, deadlock_tra # Coder proposes — the stale version-0 ACK is invalidated result = t.handle_propose( "coder", - {"summary": "Implementation", "artifacts": ["src/auth.py"], "commit_sha": "abc123"}, + {"summary": "Implementation", "artifacts": ["src/auth.py"], "commit_sha": "abc1234"}, ) assert "reviewer_code" in result["stale_reviewers"] assert "reviewer_code" not in t._confirmed @@ -2585,10 +2594,10 @@ def test_proposal_with_commit_sha_accepted(self, tracker): """Proposal with valid commit_sha should be accepted.""" result = tracker.handle_propose( "coder", - {"summary": "With SHA", "artifacts": ["a.py"], "commit_sha": "abc123"}, + {"summary": "With SHA", "artifacts": ["a.py"], "commit_sha": "abc1234"}, ) assert result["status"] == "proposed" - assert result["commit_sha"] == "abc123" + assert result["commit_sha"] == "abc1234" def test_commit_sha_stored_in_tracker(self, tracker): """get_proposal_commit_sha() should return the SHA from the proposal.""" @@ -2603,7 +2612,7 @@ def test_commit_sha_updated_on_repropose(self, tracker): """Re-proposal should update the stored commit_sha.""" tracker.handle_propose( "coder", - {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "sha1"}, + {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "1111111"}, ) tracker.handle_nack( "reviewer_code", @@ -2612,10 +2621,10 @@ def test_commit_sha_updated_on_repropose(self, tracker): ) tracker.handle_re_propose( "coder", - {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "sha2"}, + {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "2222222"}, changed_artifacts=["a.py"], ) - assert tracker.get_proposal_commit_sha("coder") == "sha2" + assert tracker.get_proposal_commit_sha("coder") == "2222222" class TestUnresolvedNackGuard: @@ -2660,14 +2669,14 @@ def test_reviewer_cannot_confirm_with_unresolved_nack(self, nack_guard_tracker): # Both producers propose t.handle_propose( "coder", - {"summary": "Implementation", "artifacts": ["src/main.py"], "commit_sha": "abc123"}, + {"summary": "Implementation", "artifacts": ["src/main.py"], "commit_sha": "abc1234"}, ) t.handle_propose( "tester", { "summary": "Tests v1", "artifacts": ["tests/test_main.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", "attestation": {"tests_run": 5, "checks_passed": ["test"]}, }, ) @@ -2707,14 +2716,14 @@ def test_confirm_succeeds_after_re_propose_and_re_ack(self, nack_guard_tracker): # Setup: both propose t.handle_propose( "coder", - {"summary": "Impl", "artifacts": ["src/main.py"], "commit_sha": "abc123"}, + {"summary": "Impl", "artifacts": ["src/main.py"], "commit_sha": "abc1234"}, ) t.handle_propose( "tester", { "summary": "Tests v1", "artifacts": ["tests/test_main.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", "attestation": {"tests_run": 5, "checks_passed": ["test"]}, }, ) @@ -2737,7 +2746,7 @@ def test_confirm_succeeds_after_re_propose_and_re_ack(self, nack_guard_tracker): { "summary": "Tests v2 - added edge cases", "artifacts": ["tests/test_main.py"], - "commit_sha": "def456", + "commit_sha": "def5678", "attestation": {"tests_run": 10, "checks_passed": ["test"]}, }, changed_artifacts=["tests/test_main.py"], @@ -2761,14 +2770,14 @@ def test_full_1576_scenario_reaches_consensus(self, nack_guard_tracker): # All producers propose t.handle_propose( "coder", - {"summary": "Implementation", "artifacts": ["src/main.py"], "commit_sha": "abc123"}, + {"summary": "Implementation", "artifacts": ["src/main.py"], "commit_sha": "abc1234"}, ) t.handle_propose( "tester", { "summary": "Tests v1", "artifacts": ["tests/test_main.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", "attestation": {"tests_run": 5, "checks_passed": ["test"]}, }, ) @@ -2796,7 +2805,7 @@ def test_full_1576_scenario_reaches_consensus(self, nack_guard_tracker): { "summary": "Tests v2 - addressed reviewer feedback", "artifacts": ["tests/test_main.py"], - "commit_sha": "def456", + "commit_sha": "def5678", "attestation": {"tests_run": 10, "checks_passed": ["test"]}, }, changed_artifacts=["tests/test_main.py"], @@ -2832,7 +2841,7 @@ def test_nack_guard_does_not_block_when_all_nacks_resolved(self): { "summary": "Tests v1", "artifacts": ["tests/test_main.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", "attestation": {"tests_run": 5, "checks_passed": ["test"]}, }, ) @@ -2850,7 +2859,7 @@ def test_nack_guard_does_not_block_when_all_nacks_resolved(self): { "summary": "Tests v2", "artifacts": ["tests/test_main.py"], - "commit_sha": "def456", + "commit_sha": "def5678", "attestation": {"tests_run": 8, "checks_passed": ["test"]}, }, changed_artifacts=["tests/test_main.py"], @@ -2877,14 +2886,14 @@ def test_nack_guard_with_multiple_producers(self): # Both propose t.handle_propose( - "coder", {"summary": "code", "artifacts": ["a.py"], "commit_sha": "abc123"} + "coder", {"summary": "code", "artifacts": ["a.py"], "commit_sha": "abc1234"} ) t.handle_propose( "tester", { "summary": "tests", "artifacts": ["t.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", "attestation": {"tests_run": 5, "checks_passed": ["test"]}, }, ) @@ -2931,7 +2940,7 @@ def test_nack_guard_response_includes_nacked_producers(self): { "summary": "Tests v1", "artifacts": ["tests/test_main.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", "attestation": {"tests_run": 5, "checks_passed": ["test"]}, }, ) @@ -2970,14 +2979,14 @@ def test_dual_role_producer_side_unaffected_by_nack_guard(self): # coder and tester propose t.handle_propose( - "coder", {"summary": "code", "artifacts": ["a.py"], "commit_sha": "abc123"} + "coder", {"summary": "code", "artifacts": ["a.py"], "commit_sha": "abc1234"} ) t.handle_propose( "tester", { "summary": "tests", "artifacts": ["t.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", "attestation": {"tests_run": 5, "checks_passed": ["test"]}, }, ) @@ -3028,9 +3037,9 @@ def test_jira_pipeline_full_lifecycle(self, jira_tracker): { "summary": "Implemented feature", "artifacts": ["src/feature.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", "attestation": { - "commit_shas": ["abc123"], + "commit_shas": ["abc1234"], "files_changed": ["src/feature.py"], "test_summary": "All pass", "risk_considered": "None", @@ -3045,7 +3054,7 @@ def test_jira_pipeline_full_lifecycle(self, jira_tracker): { "summary": "Added tests", "artifacts": ["tests/test_feature.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", "attestation": { "tests_written": 3, "tests_run": 3, @@ -3140,11 +3149,11 @@ def test_pending_acks_returns_blocking_details(self, jira_tracker): # Both producers must propose to pass global zero-proposal guard (#1648) jira_tracker.handle_propose( "coder", - {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc123"}, + {"summary": "impl", "artifacts": ["src/a.py"], "commit_sha": "abc1234"}, ) jira_tracker.handle_propose( "tester", - {"summary": "tests", "artifacts": ["tests/t.py"], "commit_sha": "def456"}, + {"summary": "tests", "artifacts": ["tests/t.py"], "commit_sha": "def5678"}, ) # Only reviewer_code ACKs coder (reviewer_contract hasn't) jira_tracker.handle_ack("reviewer_code", "coder", {"artifact_references": ["src/a.py"]}) @@ -3173,7 +3182,7 @@ def test_returns_epoch_after_proposal(self, tracker): """Returns the proposal timestamp as epoch float after a producer proposes.""" tracker.handle_propose( "coder", - {"summary": "test", "artifacts": ["a.py"], "commit_sha": "abc123"}, + {"summary": "test", "artifacts": ["a.py"], "commit_sha": "abc1234"}, ) result = tracker.get_earliest_proposal_time("reviewer_code") assert result is not None @@ -3190,13 +3199,13 @@ def test_returns_earliest_with_multiple_producers(self, tracker): # Both coder and tester are upstream of reviewer_code tracker.handle_propose( "coder", - {"summary": "coder proposal", "artifacts": ["a.py"], "commit_sha": "abc123"}, + {"summary": "coder proposal", "artifacts": ["a.py"], "commit_sha": "abc1234"}, ) # Small delay to get a different timestamp time.sleep(0.01) tracker.handle_propose( "tester", - {"summary": "tester proposal", "artifacts": ["t.py"], "commit_sha": "def456"}, + {"summary": "tester proposal", "artifacts": ["t.py"], "commit_sha": "def5678"}, ) result = tracker.get_earliest_proposal_time("reviewer_code") @@ -3231,7 +3240,7 @@ def test_only_considers_upstream_producers(self): # Only tester proposes t.handle_propose( "tester", - {"summary": "test", "artifacts": ["t.py"], "commit_sha": "abc"}, + {"summary": "test", "artifacts": ["t.py"], "commit_sha": "abc1234"}, ) # reviewer_a is upstream of coder (not tester) — should return None @@ -3257,7 +3266,7 @@ def test_empty_when_proposed_but_not_acked(self, tracker): """Returns empty dict when a producer has proposed but not been ACKed.""" tracker.handle_propose( "coder", - {"summary": "test", "artifacts": ["a.py"], "commit_sha": "abc123"}, + {"summary": "test", "artifacts": ["a.py"], "commit_sha": "abc1234"}, ) result = tracker.get_fully_acked_producers() assert result == {} @@ -3266,12 +3275,12 @@ def test_returns_fully_acked_producer(self, tracker): """Returns producer with timestamp when fully ACKed by all critical reviewers.""" tracker.handle_propose( "coder", - {"summary": "test", "artifacts": ["a.py"], "commit_sha": "abc123"}, + {"summary": "test", "artifacts": ["a.py"], "commit_sha": "abc1234"}, ) # tester must also propose to pass global zero-proposal guard (#1648) tracker.handle_propose( "tester", - {"summary": "tests", "artifacts": ["tests/t.py"], "commit_sha": "def456"}, + {"summary": "tests", "artifacts": ["tests/t.py"], "commit_sha": "def5678"}, ) # Both critical reviewers ACK tracker.handle_ack("reviewer_code", "coder", {"artifact_references": ["a.py"]}) @@ -3285,12 +3294,12 @@ def test_excludes_confirmed_producers(self, tracker): """Producers that have already confirmed are excluded.""" tracker.handle_propose( "coder", - {"summary": "test", "artifacts": ["a.py"], "commit_sha": "abc123"}, + {"summary": "test", "artifacts": ["a.py"], "commit_sha": "abc1234"}, ) # tester must also propose to pass global zero-proposal guard (#1648) tracker.handle_propose( "tester", - {"summary": "tests", "artifacts": ["tests/t.py"], "commit_sha": "def456"}, + {"summary": "tests", "artifacts": ["tests/t.py"], "commit_sha": "def5678"}, ) tracker.handle_ack("reviewer_code", "coder", {"artifact_references": ["a.py"]}) tracker.handle_ack("reviewer_contract", "coder", {"artifact_references": ["a.py"]}) @@ -3314,11 +3323,11 @@ def test_excludes_partially_acked(self, tracker): """ tracker.handle_propose( "coder", - {"summary": "test", "artifacts": ["a.py"], "commit_sha": "abc123"}, + {"summary": "test", "artifacts": ["a.py"], "commit_sha": "abc1234"}, ) tracker.handle_propose( "tester", - {"summary": "tests", "artifacts": ["tests/t.py"], "commit_sha": "def456"}, + {"summary": "tests", "artifacts": ["tests/t.py"], "commit_sha": "def5678"}, ) # Only one of coder's two critical reviewers ACKs. tracker.handle_ack("reviewer_code", "coder", {"artifact_references": ["a.py"]}) @@ -3342,7 +3351,7 @@ def test_advisory_ack_not_needed(self): t.handle_propose( "coder", - {"summary": "test", "artifacts": ["a.py"], "commit_sha": "abc"}, + {"summary": "test", "artifacts": ["a.py"], "commit_sha": "abc1234"}, ) # Only critical reviewer ACKs t.handle_ack("reviewer_a", "coder", {"artifact_references": ["a.py"]}) @@ -3368,7 +3377,7 @@ def test_excludes_when_global_zero_proposal_blocks(self): # Documenter proposes; coder and tester have not. t.handle_propose( "documenter", - {"summary": "docs", "artifacts": ["docs/README.md"], "commit_sha": "abc"}, + {"summary": "docs", "artifacts": ["docs/README.md"], "commit_sha": "abc1234"}, ) # All critical reviewers ACK documenter (default-implement graph # uses reviewer_code as the lone critical reviewer for documenter). @@ -3381,11 +3390,11 @@ def test_excludes_when_global_zero_proposal_blocks(self): # Once peers propose, documenter becomes ready. t.handle_propose( - "coder", {"summary": "code", "artifacts": ["src/m.py"], "commit_sha": "def"} + "coder", {"summary": "code", "artifacts": ["src/m.py"], "commit_sha": "def5678"} ) t.handle_propose( "tester", - {"summary": "tests", "artifacts": ["tests/t.py"], "commit_sha": "ghi"}, + {"summary": "tests", "artifacts": ["tests/t.py"], "commit_sha": "9999111"}, ) result = t.get_fully_acked_producers() assert "documenter" in result, ( @@ -3413,7 +3422,7 @@ def _register_default_implement(self) -> PeerConsensusTracker: return t def _propose( - self, t: PeerConsensusTracker, role: str, *, artifacts: list[str], commit: str = "abc123" + self, t: PeerConsensusTracker, role: str, *, artifacts: list[str], commit: str = "abc1234" ) -> dict: return t.handle_propose( role, @@ -3598,8 +3607,8 @@ def test_dual_role_reviewer_side_rejection_drops_memo_but_blocks_re_emit(self): # Both producers propose v1; reviewer_x ACKs tester so tester is # fully ACKed as a producer. tester has NOT yet reviewed coder. - self._propose(t, "coder", artifacts=["a.py"], commit="abc123") - self._propose(t, "tester", artifacts=["t.py"], commit="def456") + self._propose(t, "coder", artifacts=["a.py"], commit="abc1234") + self._propose(t, "tester", artifacts=["t.py"], commit="def5678") t.handle_ack("reviewer_x", "tester", {"artifact_references": ["t.py"]}) # Synthetic producer-side memo for tester at v1 (see seed @@ -3648,7 +3657,7 @@ def test_re_propose_re_arms_nudge(self): t.register_agent("reviewer_code") # v1: propose, ACK — nudge fires. - self._propose(t, "coder", artifacts=["a.py"], commit="abc123") + self._propose(t, "coder", artifacts=["a.py"], commit="abc1234") ack_v1 = t.handle_ack("reviewer_code", "coder", {"artifact_references": ["a.py"]}) v1_ready = [e for e in ack_v1["newly_ready"] if e["role"] == "coder"] assert v1_ready and v1_ready[0]["version"] == 1 @@ -3660,7 +3669,7 @@ def test_re_propose_re_arms_nudge(self): t.handle_nack("reviewer_code", "coder", {"artifact_references": ["a.py"], "reason": "bug"}) repropose_result = t.handle_re_propose( "coder", - {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "def456"}, + {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "def5678"}, changed_artifacts=["a.py"], ) assert all(e["role"] != "coder" for e in repropose_result["newly_ready"]) diff --git a/orchestrator/tests/test_pre_proposal_ack_deadlock.py b/orchestrator/tests/test_pre_proposal_ack_deadlock.py index 7770021990..25f1c47a52 100644 --- a/orchestrator/tests/test_pre_proposal_ack_deadlock.py +++ b/orchestrator/tests/test_pre_proposal_ack_deadlock.py @@ -77,7 +77,7 @@ def _minimal_proposal(summary="test", artifacts=None): return { "summary": summary, "artifacts": artifacts or ["file.py"], - "commit_sha": "abc123", + "commit_sha": "abc1234", } diff --git a/orchestrator/tests/test_producer_push_consensus.py b/orchestrator/tests/test_producer_push_consensus.py index 375c8a14c6..ac3e46ae6b 100644 --- a/orchestrator/tests/test_producer_push_consensus.py +++ b/orchestrator/tests/test_producer_push_consensus.py @@ -31,7 +31,7 @@ # --------------------------------------------------------------------------- -def make_proposal(summary="Test", artifacts=None, commit_sha="abc123"): +def make_proposal(summary="Test", artifacts=None, commit_sha="abc1234"): return { "summary": summary, "artifacts": artifacts or ["src/main.py"], @@ -130,7 +130,7 @@ class TestProducerPushAutoReproposeEnabled: def test_push_in_proposed_phase_auto_re_proposes(self, tracker): """Push at sha2 after propose at sha1 -> auto_re_propose=True, version 2.""" - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) assert tracker.matrix.get_proposal_version("coder") == 1 # ACK from all reviewers at v1 @@ -138,7 +138,7 @@ def test_push_in_proposed_phase_auto_re_proposes(self, tracker): ack_producer(tracker, "reviewer_contract", "coder") ack_producer(tracker, "tester", "coder") - push_result = tracker.handle_producer_push("coder", "sha2") + push_result = tracker.handle_producer_push("coder", "2222222") assert push_result["auto_re_propose"] is True assert push_result["auto_trigger"] == "auto_push" @@ -151,12 +151,12 @@ def test_push_in_confirmed_phase_auto_re_proposes(self, simple_tracker): """Producer confirms (fully ACKed), then pushes -> auto re-propose.""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) ack_producer(tracker, "reviewer_code", "coder") tracker.handle_confirmed("coder") assert tracker._producer_phases["coder"] == ConsensusPhase.CONFIRMED - push_result = tracker.handle_producer_push("coder", "sha2") + push_result = tracker.handle_producer_push("coder", "2222222") assert push_result["auto_re_propose"] is True assert push_result["version"] == 2 @@ -166,7 +166,7 @@ def test_scoped_invalidation_matching_changed_files(self, tracker): """ACK with artifact_refs=["src/main.py"], push with same -> ACK invalidated.""" tracker.handle_propose( "coder", - make_proposal(artifacts=["src/main.py", "src/utils.py"], commit_sha="sha1"), + make_proposal(artifacts=["src/main.py", "src/utils.py"], commit_sha="1111111"), ) # reviewer_code ACKs referencing src/main.py @@ -176,7 +176,9 @@ def test_scoped_invalidation_matching_changed_files(self, tracker): # tester ACKs referencing src/main.py tracker.handle_ack("tester", "coder", {"artifact_references": ["src/main.py"]}) - push_result = tracker.handle_producer_push("coder", "sha2", changed_files=["src/main.py"]) + push_result = tracker.handle_producer_push( + "coder", "2222222", changed_files=["src/main.py"] + ) assert push_result["auto_re_propose"] is True invalidated = set(push_result["invalidated_reviewers"]) @@ -189,7 +191,7 @@ def test_scoped_invalidation_non_matching_changed_files(self, tracker): """ACK with artifact_refs=["src/main.py"], push changes ["test/foo.py"] -> no overlap.""" tracker.handle_propose( "coder", - make_proposal(artifacts=["src/main.py"], commit_sha="sha1"), + make_proposal(artifacts=["src/main.py"], commit_sha="1111111"), ) ack_producer(tracker, "reviewer_code", "coder", artifact_references=["src/main.py"]) @@ -205,7 +207,9 @@ def test_scoped_invalidation_non_matching_changed_files(self, tracker): # changed_files is test/foo.py, the overlap set is empty. # Then it checks ACK artifacts: src/main.py vs test/foo.py -> also empty. # So check_auto_repropose returns False -> push is "skipped". - push_result = tracker.handle_producer_push("coder", "sha2", changed_files=["test/foo.py"]) + push_result = tracker.handle_producer_push( + "coder", "2222222", changed_files=["test/foo.py"] + ) assert push_result["status"] == "skipped" assert push_result["auto_re_propose"] is False @@ -214,13 +218,13 @@ def test_conservative_invalidation_no_changed_files(self, tracker): """Push without changed_files -> ALL ACKs invalidated.""" tracker.handle_propose( "coder", - make_proposal(artifacts=["src/auth.py"], commit_sha="sha1"), + make_proposal(artifacts=["src/auth.py"], commit_sha="1111111"), ) ack_producer(tracker, "reviewer_code", "coder", artifact_references=["src/auth.py"]) ack_producer(tracker, "reviewer_contract", "coder", artifact_references=["src/auth.py"]) ack_producer(tracker, "tester", "coder", artifact_references=["src/auth.py"]) - push_result = tracker.handle_producer_push("coder", "sha2") + push_result = tracker.handle_producer_push("coder", "2222222") assert push_result["auto_re_propose"] is True invalidated = set(push_result["invalidated_reviewers"]) @@ -232,14 +236,14 @@ def test_state_transitions_after_push(self, simple_tracker): """After push: producer -> PROPOSED, confirmed cleared, version incremented.""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) ack_producer(tracker, "reviewer_code", "coder") tracker.handle_confirmed("coder") assert "coder" in tracker._confirmed assert tracker._producer_phases["coder"] == ConsensusPhase.CONFIRMED - tracker.handle_producer_push("coder", "sha2") + tracker.handle_producer_push("coder", "2222222") assert tracker._producer_phases["coder"] == ConsensusPhase.PROPOSED assert "coder" not in tracker._confirmed @@ -249,16 +253,16 @@ def test_multiple_sequential_pushes_increment_version(self, simple_tracker): """Each push with unique SHA increments version.""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) assert tracker.matrix.get_proposal_version("coder") == 1 - result1 = tracker.handle_producer_push("coder", "sha2") + result1 = tracker.handle_producer_push("coder", "2222222") assert result1["version"] == 2 - result2 = tracker.handle_producer_push("coder", "sha3") + result2 = tracker.handle_producer_push("coder", "3333333") assert result2["version"] == 3 - result3 = tracker.handle_producer_push("coder", "sha4") + result3 = tracker.handle_producer_push("coder", "4444444") assert result3["version"] == 4 assert tracker.matrix.get_proposal_version("coder") == 4 @@ -274,7 +278,7 @@ class TestProducerPushNoOp: def test_push_in_working_phase_is_noop(self, tracker): """Producer in WORKING phase -> no-op.""" - result = tracker.handle_producer_push("coder", "sha1") + result = tracker.handle_producer_push("coder", "1111111") assert result["status"] == "no_op" assert "WORKING" in result["reason"] @@ -282,12 +286,12 @@ def test_push_in_working_phase_is_noop(self, tracker): def test_non_producer_push_raises(self, tracker): """Non-producer agent pushing raises ValueError.""" with pytest.raises(ValueError, match="not a producer"): - tracker.handle_producer_push("reviewer_code", "sha1") + tracker.handle_producer_push("reviewer_code", "1111111") def test_unregistered_non_producer_raises(self, tracker): """Unknown agent that is not a producer raises ValueError.""" with pytest.raises(ValueError): - tracker.handle_producer_push("random_agent", "sha1") + tracker.handle_producer_push("random_agent", "1111111") # =========================================================================== @@ -302,9 +306,9 @@ def test_same_commit_sha_skipped(self, simple_tracker): """Propose at sha1, push at sha1 -> skipped, reason mentions unchanged.""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) - result = tracker.handle_producer_push("coder", "sha1") + result = tracker.handle_producer_push("coder", "1111111") assert result["status"] == "skipped" assert "unchanged" in result["reason"].lower() @@ -323,10 +327,10 @@ def test_push_covered_by_recent_explicit_proposal(self): tracker.register_agent("reviewer_code") # Explicit proposal sets _proposal_timestamps - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) # Push immediately after -> covered by the recent proposal - result = tracker.handle_producer_push("coder", "sha2") + result = tracker.handle_producer_push("coder", "2222222") assert result["status"] == "skipped" assert "explicit proposal" in result["reason"].lower() @@ -344,7 +348,7 @@ def test_push_not_covered_by_stale_proposal(self): tracker.register_agent("coder") tracker.register_agent("reviewer_code") - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) # Backdate explicit proposal timestamp to simulate time passage tracker._last_explicit_propose_timestamp["coder"] = datetime.now(UTC) - timedelta( @@ -352,7 +356,7 @@ def test_push_not_covered_by_stale_proposal(self): ) # Push now -> proposal is stale, auto re-propose should fire - result = tracker.handle_producer_push("coder", "sha2") + result = tracker.handle_producer_push("coder", "2222222") assert result["auto_re_propose"] is True def test_debounce_window_active(self): @@ -368,7 +372,7 @@ def test_debounce_window_active(self): tracker.register_agent("coder") tracker.register_agent("reviewer_code") - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) # Backdate explicit proposal timestamp so the explicit-proposal # check doesn't suppress the first push (we're testing debounce here) @@ -377,11 +381,11 @@ def test_debounce_window_active(self): ) # First push succeeds (no prior auto-repropose timestamp) - result1 = tracker.handle_producer_push("coder", "sha2") + result1 = tracker.handle_producer_push("coder", "2222222") assert result1["auto_re_propose"] is True # Second push within debounce window -> skipped - result2 = tracker.handle_producer_push("coder", "sha3") + result2 = tracker.handle_producer_push("coder", "3333333") assert result2["status"] == "skipped" assert "Debounce" in result2["reason"] @@ -399,14 +403,14 @@ def test_max_counter_exceeded(self): tracker.register_agent("coder") tracker.register_agent("reviewer_code") - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) # First push succeeds (count goes to 1) - result1 = tracker.handle_producer_push("coder", "sha2") + result1 = tracker.handle_producer_push("coder", "2222222") assert result1["auto_re_propose"] is True # Second push -> max exceeded (count=1 >= max=1) - result2 = tracker.handle_producer_push("coder", "sha3") + result2 = tracker.handle_producer_push("coder", "3333333") assert result2["status"] == "skipped" assert "Max" in result2["reason"] @@ -417,12 +421,12 @@ def test_no_overlap_with_proposed_or_ack_artifacts(self, simple_tracker): tracker.handle_propose( "coder", - make_proposal(artifacts=["src/main.py"], commit_sha="sha1"), + make_proposal(artifacts=["src/main.py"], commit_sha="1111111"), ) # No ACKs recorded, so no ACK artifact overlap either. result = tracker.handle_producer_push( - "coder", "sha2", changed_files=["totally_unrelated.py"] + "coder", "2222222", changed_files=["totally_unrelated.py"] ) assert result["status"] == "skipped" @@ -436,12 +440,12 @@ def test_overlap_with_ack_artifacts_but_not_proposed_artifacts(self, simple_trac tracker.handle_propose( "coder", - make_proposal(artifacts=["src/main.py"], commit_sha="sha1"), + make_proposal(artifacts=["src/main.py"], commit_sha="1111111"), ) # Reviewer ACKs referencing test.py (different from proposed artifacts) tracker.handle_ack("reviewer_code", "coder", {"artifact_references": ["test.py"]}) - result = tracker.handle_producer_push("coder", "sha2", changed_files=["test.py"]) + result = tracker.handle_producer_push("coder", "2222222", changed_files=["test.py"]) # Should trigger because test.py overlaps with ACK artifact_refs assert result["auto_re_propose"] is True @@ -460,10 +464,10 @@ def test_reviewer_confirm_rejected_with_stale_ack(self, simple_tracker): """ACK at v1, push invalidates -> confirm rejected.""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) ack_producer(tracker, "reviewer_code", "coder") - tracker.handle_producer_push("coder", "sha2") + tracker.handle_producer_push("coder", "2222222") # Reviewer tries to confirm without re-ACKing -> rejected with pytest.raises(ValueError, match="hasn't reviewed"): @@ -473,10 +477,10 @@ def test_reviewer_re_acks_at_new_version_then_confirms(self, simple_tracker): """ACK at v1, push -> v2, reviewer re-ACKs at v2 -> confirm succeeds.""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) ack_producer(tracker, "reviewer_code", "coder") - tracker.handle_producer_push("coder", "sha2") + tracker.handle_producer_push("coder", "2222222") ack_result = ack_producer(tracker, "reviewer_code", "coder") assert ack_result["version"] == 2 @@ -488,7 +492,7 @@ def test_unresolved_nack_blocks_confirm(self, simple_tracker): """NACK without re-propose -> confirm rejected.""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) nack_producer(tracker, "reviewer_code", "coder", reason="bugs found") result = tracker.handle_confirmed("reviewer_code") @@ -532,7 +536,7 @@ def test_after_excuse_reviewers_can_confirm(self, tracker): # Tester proposes and gets ACKed tracker.handle_propose( "tester", - make_proposal(summary="Tests", artifacts=["tests/test.py"], commit_sha="tsha1"), + make_proposal(summary="Tests", artifacts=["tests/test.py"], commit_sha="a111111"), ) ack_producer(tracker, "reviewer_code", "tester", artifact_references=["tests/test.py"]) @@ -546,9 +550,9 @@ def test_after_excuse_reviewers_can_confirm(self, tracker): def test_excuse_clears_producer_state(self, tracker): """excuse_producer clears _producer_phases, _confirmed, _proposal_artifacts, etc.""" # Setup: coder proposes and gets confirmed - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) # tester must also propose to pass global zero-proposal guard (#1648) - tracker.handle_propose("tester", make_proposal(commit_sha="sha2")) + tracker.handle_propose("tester", make_proposal(commit_sha="2222222")) ack_producer(tracker, "reviewer_code", "coder") ack_producer(tracker, "reviewer_contract", "coder") ack_producer(tracker, "tester", "coder") @@ -579,7 +583,7 @@ def test_get_nack_entries_for(self, simple_tracker): """NACK a producer -> get_nack_entries_for returns [(reviewer, entry)].""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) nack_producer(tracker, "reviewer_code", "coder", reason="bad code") entries = tracker.matrix.get_nack_entries_for("coder") @@ -593,7 +597,7 @@ def test_has_unresolved_nacks_as_producer_current_version(self, simple_tracker): """NACK at current version -> has_unresolved_nacks_as_producer returns True.""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) nack_producer(tracker, "reviewer_code", "coder", reason="bugs") assert tracker.matrix.has_unresolved_nacks_as_producer("coder") is True @@ -602,13 +606,13 @@ def test_has_unresolved_nacks_as_producer_old_version(self, simple_tracker): """NACK at v1, re-propose to v2 -> has_unresolved_nacks returns False.""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) nack_producer(tracker, "reviewer_code", "coder", reason="bugs") # Re-propose at v2 tracker.handle_re_propose( "coder", - make_proposal(summary="Fixed", commit_sha="sha2"), + make_proposal(summary="Fixed", commit_sha="2222222"), changed_artifacts=["src/main.py"], ) @@ -618,7 +622,7 @@ def test_has_unresolved_nacks_as_producer_old_version(self, simple_tracker): def test_get_latest_review_versions(self, tracker): """ACK at v2, NACK at v1 -> correct versions returned.""" # Coder proposes v1 - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) # reviewer_code NACKs coder at v1 nack_producer(tracker, "reviewer_code", "coder", reason="bad") @@ -626,7 +630,7 @@ def test_get_latest_review_versions(self, tracker): # Tester proposes v1 tracker.handle_propose( "tester", - make_proposal(summary="Tests", artifacts=["tests/t.py"], commit_sha="tsha1"), + make_proposal(summary="Tests", artifacts=["tests/t.py"], commit_sha="a111111"), ) # reviewer_code ACKs tester at v1 @@ -635,7 +639,7 @@ def test_get_latest_review_versions(self, tracker): # Coder re-proposes at v2 tracker.handle_re_propose( "coder", - make_proposal(summary="Fixed", commit_sha="sha2"), + make_proposal(summary="Fixed", commit_sha="2222222"), changed_artifacts=["src/main.py"], ) @@ -650,7 +654,7 @@ def test_ack_commit_sha_stored(self, simple_tracker): """record_ack with commit_sha -> stored in entry.ack_commit_sha.""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) version = tracker.matrix.get_proposal_version("coder") entry = tracker.matrix.record_ack( @@ -680,7 +684,7 @@ def test_invariants_hold_after_push_with_cleanup(self, simple_tracker): """Push -> un-confirm stale -> invariants pass.""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) ack_producer(tracker, "reviewer_code", "coder") tracker.handle_confirmed("coder") tracker.handle_confirmed("reviewer_code") @@ -688,7 +692,7 @@ def test_invariants_hold_after_push_with_cleanup(self, simple_tracker): assert "reviewer_code" in tracker._confirmed assert "coder" in tracker._confirmed - tracker.handle_producer_push("coder", "sha2") + tracker.handle_producer_push("coder", "2222222") violations = tracker.validate_invariants() assert len(violations) == 0, f"Unexpected violations: {violations}" @@ -697,13 +701,13 @@ def test_full_recovery_from_push_invariants_hold(self, simple_tracker): """Push -> re-ACK -> confirm -> invariants hold.""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) ack_producer(tracker, "reviewer_code", "coder") tracker.handle_confirmed("coder") tracker.handle_confirmed("reviewer_code") # Push disrupts consensus - tracker.handle_producer_push("coder", "sha2") + tracker.handle_producer_push("coder", "2222222") # Recovery ack_producer(tracker, "reviewer_code", "coder") @@ -727,11 +731,11 @@ def test_end_to_end_propose_ack_push_re_ack_confirm(self, simple_tracker): tracker = simple_tracker # v1: propose and ACK - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) ack_producer(tracker, "reviewer_code", "coder") # Push disrupts -> v2 - push_result = tracker.handle_producer_push("coder", "sha2") + push_result = tracker.handle_producer_push("coder", "2222222") assert push_result["version"] == 2 assert push_result["auto_trigger"] == "auto_push" @@ -749,13 +753,13 @@ def test_multiple_pushes_then_consensus(self, simple_tracker): """Multiple version bumps, final ACK, confirm -> consensus.""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) ack_producer(tracker, "reviewer_code", "coder") # Three sequential pushes - tracker.handle_producer_push("coder", "sha2") - tracker.handle_producer_push("coder", "sha3") - push_result = tracker.handle_producer_push("coder", "sha4") + tracker.handle_producer_push("coder", "2222222") + tracker.handle_producer_push("coder", "3333333") + push_result = tracker.handle_producer_push("coder", "4444444") assert push_result["version"] == 4 # Reviewer only needs to ACK the latest version @@ -787,22 +791,22 @@ def test_history_accumulates_across_versions(self, simple_tracker): remain resolvable (unlike the single-slot ``_proposal_commit_shas``).""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) - tracker.handle_producer_push("coder", "sha2") - tracker.handle_producer_push("coder", "sha3") + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) + tracker.handle_producer_push("coder", "2222222") + tracker.handle_producer_push("coder", "3333333") - assert tracker.get_commit_sha_for_version("coder", 1) == "sha1" - assert tracker.get_commit_sha_for_version("coder", 2) == "sha2" - assert tracker.get_commit_sha_for_version("coder", 3) == "sha3" + assert tracker.get_commit_sha_for_version("coder", 1) == "1111111" + assert tracker.get_commit_sha_for_version("coder", 2) == "2222222" + assert tracker.get_commit_sha_for_version("coder", 3) == "3333333" # The current-only slot tracks only the latest. - assert tracker.get_proposal_commit_sha("coder") == "sha3" + assert tracker.get_proposal_commit_sha("coder") == "3333333" def test_unknown_version_returns_empty(self, simple_tracker): """A version with no pinned commit (e.g. version 0 / not yet proposed) resolves to "" so callers fall back to REVIEWER-SYNC.""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) assert tracker.get_commit_sha_for_version("coder", 99) == "" assert tracker.get_commit_sha_for_version("unknown_producer", 1) == "" @@ -812,11 +816,11 @@ def test_reviewer_last_verdict_resolves_to_reviewed_commit(self, simple_tracker) lookup the re-review delta range depends on (#2887).""" tracker = simple_tracker - tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_propose("coder", make_proposal(commit_sha="1111111")) nack_producer(tracker, "reviewer_code", "coder", reason="v1 bug") # Producer re-proposes; reviewer's entry still points at v1. - tracker.handle_propose("coder", make_proposal(commit_sha="sha2")) + tracker.handle_propose("coder", make_proposal(commit_sha="2222222")) entry = tracker.matrix.get_entry("reviewer_code", "coder") assert entry.version == 1 - assert tracker.get_commit_sha_for_version("coder", entry.version) == "sha1" + assert tracker.get_commit_sha_for_version("coder", entry.version) == "1111111" diff --git a/orchestrator/tests/test_repropose_confirmed_clear.py b/orchestrator/tests/test_repropose_confirmed_clear.py index 4a5173135f..b3d9690d74 100644 --- a/orchestrator/tests/test_repropose_confirmed_clear.py +++ b/orchestrator/tests/test_repropose_confirmed_clear.py @@ -41,7 +41,7 @@ def tracker(simple_graph): def _propose(tracker, role="producer", version=1): return tracker.handle_propose( - role, {"summary": f"v{version}", "artifacts": ["a.py"], "commit_sha": "abc123"} + role, {"summary": f"v{version}", "artifacts": ["a.py"], "commit_sha": "abc1234"} ) @@ -91,7 +91,7 @@ def test_confirmed_cleared_on_handle_re_propose(self, tracker): _nack(tracker, "reviewer_a") tracker.handle_re_propose( "producer", - {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "abc123"}, + {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "abc1234"}, changed_artifacts=["a.py"], ) assert "producer" not in tracker._confirmed diff --git a/orchestrator/tests/test_signals.py b/orchestrator/tests/test_signals.py index a527c1ae5d..f0faa9247e 100644 --- a/orchestrator/tests/test_signals.py +++ b/orchestrator/tests/test_signals.py @@ -2179,7 +2179,7 @@ def test_range_spans_reviewer_last_verdict_to_head(self, tracker): tracker.handle_propose( "coder", - {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "sha1"}, + {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "1111111"}, ) tracker.handle_nack( "reviewer_code", "coder", {"artifact_references": ["a.py"], "reason": "x"} @@ -2187,21 +2187,21 @@ def test_range_spans_reviewer_last_verdict_to_head(self, tracker): # Producer re-proposes at sha2; reviewer's entry still pins v1/sha1. tracker.handle_propose( "coder", - {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "sha2"}, + {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "2222222"}, ) - rng = _resolve_reviewer_delta_range(tracker, "coder", "reviewer_code", "sha2") - assert rng == "sha1..sha2" + rng = _resolve_reviewer_delta_range(tracker, "coder", "reviewer_code", "2222222") + assert rng == "1111111..2222222" def test_no_prior_verdict_returns_none(self, tracker): from routes.signals import _resolve_reviewer_delta_range tracker.handle_propose( "coder", - {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "sha1"}, + {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "1111111"}, ) # Reviewer never verdicted (entry.version == 0). - rng = _resolve_reviewer_delta_range(tracker, "coder", "reviewer_code", "sha2") + rng = _resolve_reviewer_delta_range(tracker, "coder", "reviewer_code", "2222222") assert rng is None def test_empty_head_returns_none(self, tracker): diff --git a/orchestrator/tests/test_slice_4_restart_hardening.py b/orchestrator/tests/test_slice_4_restart_hardening.py index 8c59a80c52..38e3dfad19 100644 --- a/orchestrator/tests/test_slice_4_restart_hardening.py +++ b/orchestrator/tests/test_slice_4_restart_hardening.py @@ -416,7 +416,11 @@ def test_restart_phase_clears_each_per_slice_tracker(self) -> None: t.register_agent("tester") t.handle_propose( "coder", - {"summary": "s", "artifacts": ["a.py"], "commit_sha": f"sha{sid}"}, + { + "summary": "s", + "artifacts": ["a.py"], + "commit_sha": f"sha_{sid.replace('-', '_')}", + }, ) slices = [ diff --git a/sandbox/egg_agent_tools/handlers/brc.py b/sandbox/egg_agent_tools/handlers/brc.py index fd67dfcae1..a87a4e60dd 100644 --- a/sandbox/egg_agent_tools/handlers/brc.py +++ b/sandbox/egg_agent_tools/handlers/brc.py @@ -1040,7 +1040,14 @@ def brc_read_peer_artifact(req: dict[str, Any]) -> dict[str, Any]: Response: { ok: True, phase: str, items: [...], next_cursor: str|None, - total_available: int, skipped_malformed: int } + total_available: int, skipped_malformed: int, hint?: str } + + ``hint`` is present only when no history file exists on disk — + the expected state for the phase currently in flight (#3076): + brc-history is written at phase COMPLETION and reaches an agent + worktree only via the spawn fork point, so an empty result for + the current phase is structural, not evidence that peers have + not proposed. ``skipped_malformed`` counts brc-history records that were silently skipped because they failed isinstance-dict parsing; the @@ -1160,6 +1167,14 @@ def brc_read_peer_artifact(req: dict[str, Any]) -> dict[str, Any]: records.extend(chunk) if not any_existed: + # No history file on disk. This is the EXPECTED state for the + # phase currently in flight (#3076): brc-history files are + # written by the orchestrator at phase COMPLETION, into the + # pipeline work branch — they reach an agent worktree only via + # the spawn fork point, i.e. only for phases that completed + # before this agent spawned. Say so explicitly: a bare empty + # result here reads as "peers produced nothing" and has driven + # reviewers to NACK proposals they simply could not see. return { "ok": True, "phase": phase, @@ -1167,6 +1182,18 @@ def brc_read_peer_artifact(req: dict[str, Any]) -> dict[str, Any]: "next_cursor": None, "total_available": 0, "skipped_malformed": prior_skipped, + "hint": ( + "No brc-history file exists in this worktree for phase " + f"{phase!r}. brc-history is written at phase COMPLETION; " + "for the phase currently in flight this tool is always " + "empty and that is NOT evidence that peers have not " + "proposed. Live proposals arrive via your event payload " + "(pending_reviews carries each producer's " + "proposal_commit_sha and artifact_refs); read a peer's " + "proposed artifact with `git show " + ":` — the SHA resolves from " + "your worktree via the shared object store." + ), } filtered: list[dict[str, Any]] = [] diff --git a/sandbox/egg_agent_tools/tools/brc.py b/sandbox/egg_agent_tools/tools/brc.py index 7c6ac4ac37..5c74111307 100644 --- a/sandbox/egg_agent_tools/tools/brc.py +++ b/sandbox/egg_agent_tools/tools/brc.py @@ -406,11 +406,16 @@ async def brc_resolve_obligation(args: dict[str, Any]) -> dict[str, Any]: @tool( "read_peer_artifact", - "Read BRC consensus history for a peer from the local " - "`.egg-state/brc-history/-.json` log. Paginated via " - "`limit` + opaque `cursor`. No CLI counterpart — this is a net-new " - "capability so reviewers don't have to hand-grep brc-history files " - "(decision-8).", + "Read the BRC consensus TRANSCRIPT (message records, not artifact " + "content) for a COMPLETED phase from the local " + "`.egg-state/brc-history/-.json` log. The log is " + "written by the orchestrator at phase completion and reaches your " + "worktree only at spawn — so for the phase currently in flight this " + "tool is always empty (#3076); that is NOT evidence peers have not " + "proposed. For live proposals use your event payload " + "(pending_reviews carries `proposal_commit_sha` + `artifact_refs`) " + "and read artifact content with `git show :`. Paginated " + "via `limit` + opaque `cursor`.", _READ_PEER_ARTIFACT_SCHEMA, ) async def brc_read_peer_artifact(args: dict[str, Any]) -> dict[str, Any]: diff --git a/tests/sandbox/egg_agent_tools/test_handlers_brc.py b/tests/sandbox/egg_agent_tools/test_handlers_brc.py index 94d62eedb3..d9b96f346e 100644 --- a/tests/sandbox/egg_agent_tools/test_handlers_brc.py +++ b/tests/sandbox/egg_agent_tools/test_handlers_brc.py @@ -759,6 +759,41 @@ def test_missing_history_file_returns_empty(self, tmp_path, monkeypatch): # Again: no ``path`` echo in the empty-result branch. assert "path" not in resp + def test_missing_history_file_explains_itself(self, tmp_path, monkeypatch): + """The no-file branch must carry a ``hint`` (#3076). + + brc-history is written at phase COMPLETION; for the phase in + flight the file never exists in an agent worktree, so a bare + empty result reads as "peers produced nothing" — that drove + reviewers to NACK proposals they could not see. The hint must + say the emptiness is structural and point at the live channel. + """ + self._set_env(monkeypatch, tmp_path) + resp = brc.brc_read_peer_artifact({"phase": "plan"}) + hint = resp.get("hint") + assert hint, "empty no-file response must carry a hint (#3076)" + assert "phase COMPLETION" in hint + assert "NOT evidence" in hint + assert "git show" in hint + + def test_hint_absent_when_history_file_exists(self, tmp_path, monkeypatch): + """A populated (or even filtered-to-empty) read of an existing + file is a real answer — no structural-emptiness hint.""" + self._set_env(monkeypatch, tmp_path) + _make_history_file( + tmp_path, + "1917", + "plan", + _records(("coder", "CONSENSUS_PROPOSE")), + ) + resp = brc.brc_read_peer_artifact({"phase": "plan"}) + assert "hint" not in resp + # Filtered to zero records: still no hint — the file existed, + # so emptiness reflects the filter, not the channel. + resp = brc.brc_read_peer_artifact({"phase": "plan", "peer_role": "tester"}) + assert resp["items"] == [] + assert "hint" not in resp + def test_missing_phase_rejected(self, tmp_path, monkeypatch): self._set_env(monkeypatch, tmp_path) with pytest.raises(HandlerError):