From 1942f151c875185be167195928a6b02527a526b6 Mon Sep 17 00:00:00 2001 From: Jonathan Liebig Date: Mon, 17 Aug 2026 13:02:10 +0200 Subject: [PATCH] feat(mcp): accept review findings for rework Review-Convergence: CONTINUE --- .../github/pull_request_progress.ex | 5 +- .../mcp/architect_delivery_tools.ex | 185 +++++++++ .../mcp/review_readiness.ex | 166 +++++++- .../symphony_plus_plus/mcp/server.ex | 6 +- .../symphony_plus_plus/mcp/tool_catalog.ex | 3 + .../mcp/tool_catalog/input_schemas.ex | 21 + .../mcp/tool_catalog/surface_specs.ex | 4 + .../priv/symphony_plus_plus/mcp_contract.json | 3 +- .../github_pull_request_test.exs | 28 +- .../mcp/accepted_review_rework_test.exs | 368 ++++++++++++++++++ .../mcp/worker_tools_ready_gate_test.exs | 5 + .../skills/symphony-architect/SKILL.md | 8 + .../skills/symphony-work-package/SKILL.md | 6 +- .../references/worker_prompt.md | 4 + .../skills/symphony-worker/SKILL.md | 4 + 15 files changed, 793 insertions(+), 23 deletions(-) create mode 100644 elixir/test/symphony_elixir/symphony_plus_plus/mcp/accepted_review_rework_test.exs diff --git a/elixir/lib/symphony_elixir/symphony_plus_plus/github/pull_request_progress.ex b/elixir/lib/symphony_elixir/symphony_plus_plus/github/pull_request_progress.ex index a406cde36d..b0ea6488db 100644 --- a/elixir/lib/symphony_elixir/symphony_plus_plus/github/pull_request_progress.ex +++ b/elixir/lib/symphony_elixir/symphony_plus_plus/github/pull_request_progress.ex @@ -7,7 +7,7 @@ defmodule SymphonyElixir.SymphonyPlusPlus.GitHub.PullRequestProgress do @spec chronological_events([ProgressEvent.t()]) :: [ProgressEvent.t()] def chronological_events(events) when is_list(events) do Enum.sort_by(events, fn %ProgressEvent{sequence: sequence, created_at: created_at, id: id} -> - {created_at || DateTime.from_unix!(0), sequence || 0, id || ""} + {timestamp_sort_value(created_at), sequence || 0, id || ""} end) end @@ -122,4 +122,7 @@ defmodule SymphonyElixir.SymphonyPlusPlus.GitHub.PullRequestProgress do end defp clean_string(_value), do: nil + + defp timestamp_sort_value(%DateTime{} = value), do: DateTime.to_unix(value, :microsecond) + defp timestamp_sort_value(_value), do: 0 end diff --git a/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/architect_delivery_tools.ex b/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/architect_delivery_tools.ex index 5082fb880b..82cbfad553 100644 --- a/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/architect_delivery_tools.ex +++ b/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/architect_delivery_tools.ex @@ -28,7 +28,9 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ArchitectDeliveryTools do alias SymphonyElixir.SymphonyPlusPlus.Authorization.MCPError alias SymphonyElixir.SymphonyPlusPlus.Dashboard alias SymphonyElixir.SymphonyPlusPlus.Dashboard.BlockerProjection + alias SymphonyElixir.SymphonyPlusPlus.Dashboard.MetadataProjection alias SymphonyElixir.SymphonyPlusPlus.DashboardPubSub + alias SymphonyElixir.SymphonyPlusPlus.GitHub.PullRequestProgress alias SymphonyElixir.SymphonyPlusPlus.MCP.{ Auth, @@ -181,6 +183,47 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ArchitectDeliveryTools do end end + def call("accept_review_rework", %Config{} = config, session, arguments) do + with {:ok, live_session} <- Auth.require_session(session, config.repo), + :ok <- require_delivery_write_capability(live_session), + {:ok, work_request_id} <- CurrentWorkRequest.id_argument(arguments, live_session), + {:ok, work_package_id} <- required_argument(arguments, "work_package_id"), + {:ok, idempotency_key} <- required_argument(arguments, "idempotency_key"), + {:ok, evidence} <- accepted_review_rework_evidence(arguments), + {:ok, work_request, work_package, filters, scope} <- + WorkRequestScope.authorized_work_package_scope( + config.repo, + live_session, + work_request_id, + work_package_id, + :work_package_repair_state, + "accept_review_rework" + ), + {:ok, result} <- + run_architect_transaction(config.repo, fn -> + accept_review_rework_in_transaction( + config.repo, + live_session, + work_request, + work_package, + filters, + evidence, + "accept_review_rework:#{work_package_id}:#{String.trim(idempotency_key)}" + ) + end) do + {:ok, + ToolResult.tool_result(%{ + "work_package" => work_package_payload(result.work_package), + "accepted_review_rework" => ProgressEvents.payload(result.event), + "scope" => scope + })} + else + {:tool_error, reason} -> invalid_params_error("accept_review_rework", reason) + {:error, :not_found} -> not_found_error("accept_review_rework") + {:error, reason} -> architect_error(reason, "accept_review_rework") + end + end + def call("cleanup_work_request_work_package_runtime", %Config{} = config, session, arguments) do with {:ok, live_session} <- Auth.require_session(session, config.repo), :ok <- require_delivery_write_capability(live_session), @@ -328,6 +371,25 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ArchitectDeliveryTools do end end + defp accepted_review_rework_evidence(arguments) do + with {:ok, evidence} <- required_object(arguments, "evidence"), + {:ok, provider} <- required_argument(evidence, "provider"), + {:ok, reference} <- required_argument(evidence, "reference"), + {:ok, head_sha} <- required_argument(evidence, "head_sha"), + {:ok, finding} <- required_argument(evidence, "finding") do + normalized = %{ + "provider" => provider |> String.trim() |> Redactor.redact_text(), + "reference" => reference |> String.trim() |> Redactor.redact_text(), + "head_sha" => head_sha |> String.trim() |> String.downcase(), + "finding" => finding |> String.trim() |> Redactor.redact_text() + } + + if Enum.all?(Map.values(normalized), &filled_string?/1), + do: {:ok, normalized}, + else: {:tool_error, "empty_accepted_review_rework_evidence"} + end + end + defp required_runtime_cleanup_delivery_outcome(arguments) do allowed_outcomes = ["superseded", "abandoned"] @@ -795,6 +857,129 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ArchitectDeliveryTools do WorkRequestService.get_work_package(repo, work_request_id, work_package_id) end + defp accept_review_rework_in_transaction( + repo, + %Session{} = session, + %WorkRequest{} = work_request, + %WorkPackage{} = scoped_work_package, + filters, + evidence, + idempotency_key + ) do + primary_scope? = WorkRequestScope.primary_work_request_scope?(repo, work_request, filters) + + with :ok <- lock_access_grant(repo, session.assignment.grant_id), + {:ok, _architect_grant} <- WorkRequestScope.require_live_architect_grant(repo, session), + :ok <- lock_work_package(repo, Session.work_package_id(session)), + :ok <- lock_work_package(repo, scoped_work_package.id), + {:ok, work_package} <- WorkPackageRepository.get(repo, scoped_work_package.id), + :ok <- + WorkRequestScope.require_scoped_delivery_work_package_visibility( + work_package, + work_request, + work_package, + primary_scope?, + filters + ), + {:ok, progress_events} <- PlanningRepository.list_progress_events(repo, work_package.id) do + case Enum.find(progress_events, &(&1.idempotency_key == idempotency_key)) do + %ProgressEvent{} = event -> + replay_accepted_review_rework(work_package, event, work_request.id, evidence) + + nil -> + append_accepted_review_rework( + repo, + session, + work_request, + work_package, + progress_events, + evidence, + idempotency_key + ) + end + end + end + + defp append_accepted_review_rework( + repo, + session, + work_request, + work_package, + progress_events, + evidence, + idempotency_key + ) do + with :ok <- require_accepted_review_rework_status(work_package), + {:ok, head_sha, pr} <- current_accepted_review_rework_pr(work_package, progress_events, evidence), + payload = + evidence + |> Map.put("type", "accepted_review_rework") + |> Map.put("source_tool", "accept_review_rework") + |> Map.put("work_request_id", work_request.id) + |> Map.put("work_package_id", work_package.id) + |> Map.put("head_sha", head_sha) + |> Map.put("pr", pr), + {:ok, reopened} <- WorkPackageRepository.update_status(repo, work_package.id, "ready_for_merge", "active"), + {:ok, event} <- + PlanningRepository.append_audit_progress_event_for_work_package(repo, session.assignment, work_package.id, %{ + "summary" => "Accepted verified review finding for rework", + "status" => "accepted_review_rework", + "idempotency_key" => idempotency_key, + "payload" => payload + }) do + {:ok, %{work_package: reopened, event: event}} + end + end + + defp replay_accepted_review_rework(work_package, %ProgressEvent{payload: payload} = event, work_request_id, evidence) do + expected = + evidence + |> Map.take(["provider", "reference", "head_sha", "finding"]) + |> Map.put("type", "accepted_review_rework") + |> Map.put("source_tool", "accept_review_rework") + |> Map.put("work_request_id", work_request_id) + |> Map.put("work_package_id", work_package.id) + + if is_map(payload) and Map.take(payload, Map.keys(expected)) == expected, + do: {:ok, %{work_package: work_package, event: event}}, + else: {:tool_error, "idempotency_conflict"} + end + + defp require_accepted_review_rework_status(%WorkPackage{kind: "phase_child"}), + do: {:tool_error, "phase_child_rework_not_allowed"} + + defp require_accepted_review_rework_status(%WorkPackage{status: "ready_for_merge"}), do: :ok + defp require_accepted_review_rework_status(%WorkPackage{}), do: {:tool_error, "work_package_not_ready_for_rework"} + + defp current_accepted_review_rework_pr(work_package, progress_events, evidence) do + current_head_sha = MetadataProjection.latest_current_head_sha(progress_events) + pr = MetadataProjection.metadata(progress_events, [], work_package.id, work_package.review_requirement).pr + + cond do + not filled_string?(current_head_sha) -> + {:tool_error, "missing_current_head_sha"} + + not exact_head_sha?(evidence["head_sha"], current_head_sha) -> + {:tool_error, "stale_rework_head"} + + not is_map(pr) or not exact_head_sha?(pr["head_sha"], current_head_sha) -> + {:tool_error, "missing_current_attached_pr"} + + PullRequestProgress.merged?(%{"merge_state" => pr["merge_state"]}) -> + {:tool_error, "current_attached_pr_already_merged"} + + true -> + identity = Map.take(pr, ["url", "repository", "number"]) |> Map.put("head_sha", current_head_sha) + {:ok, String.downcase(current_head_sha), identity} + end + end + + defp exact_head_sha?(left, right) when is_binary(left) and is_binary(right) do + String.downcase(String.trim(left)) == String.downcase(String.trim(right)) + end + + defp exact_head_sha?(_left, _right), do: false + defp cleanup_worktree_runtime_in_transaction(repo, %Session{} = session, work_package_id) do with :ok <- lock_access_grant(repo, session.assignment.grant_id), {:ok, _architect_grant} <- WorkRequestScope.require_live_architect_grant(repo, session), diff --git a/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/review_readiness.ex b/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/review_readiness.ex index e34b9a24e6..82e150772e 100644 --- a/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/review_readiness.ex +++ b/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/review_readiness.ex @@ -106,18 +106,20 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ReviewReadiness do {:ok, requested_head_sha} <- optional_head_sha(arguments) do replay_head_sha = requested_head_sha || latest_current_head_sha(state.progress_events) arguments = maybe_put_headless_review_idempotency_key(arguments, requested_head_sha, payload) + arguments = scope_review_rework_idempotency(arguments, state.progress_events) replay_arguments = maybe_put_review_head_sha(arguments, replay_head_sha) replay_payload = maybe_put_review_head_sha(payload, replay_head_sha) - case replay_existing_metadata_event( - repo, - session, - replay_arguments, - "submit_review_package", - "review_package_submitted", - replay_payload, - state.progress_events - ) do + replay_existing_metadata_event( + repo, + session, + replay_arguments, + "submit_review_package", + "review_package_submitted", + replay_payload, + events_after_latest_rework(state.progress_events) + ) + |> case do {:ok, result} -> put_remaining_readiness_gates_or_rollback(repo, session, result) @@ -161,12 +163,15 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ReviewReadiness do payload = maybe_put_review_head_sha(payload, review_head_sha) with :ok <- maybe_append_live_branch_refresh(repo, session, refresh), + {:ok, current_state} <- PlanningRepository.get_state(repo, state.work_package.id), + :ok <- require_rework_head_advanced(current_state.progress_events, review_head_sha), result <- submit_new_review_package(repo, session, arguments, artifacts, payload, review_head_sha), :ok <- confirm_live_branch_refresh(state.work_package, config, refresh) do put_remaining_readiness_gates_or_rollback(repo, session, result) else {:tool_error, reason} -> rollback_review_head_error(repo, reason) {:error, code, message, data} -> repo.rollback({:mcp_error, code, message, data}) + {:error, reason} -> repo.rollback(reason) end {:tool_error, reason} -> @@ -305,6 +310,24 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ReviewReadiness do defp maybe_put_headless_review_idempotency_key(arguments, _requested_head_sha, _payload), do: arguments + defp scope_review_rework_idempotency(arguments, progress_events) do + case latest_accepted_review_rework(progress_events) do + %ProgressEvent{id: id} -> + fallback = + :crypto.hash(:sha256, Jason.encode!([id, arguments])) + |> Base.url_encode64(padding: false) + + case Map.get(arguments, "idempotency_key") do + value when is_binary(value) -> Map.put(arguments, "idempotency_key", value <> ":rework:" <> id) + nil -> Map.put(arguments, "idempotency_key", "rework:#{id}:#{fallback}") + _invalid -> arguments + end + + nil -> + arguments + end + end + defp persist_review_artifacts_or_rollback(repo, %Session{} = session, artifacts, head_sha, result) do case append_review_artifacts(repo, session, artifacts, head_sha) do :ok -> result @@ -394,8 +417,11 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ReviewReadiness do {:ok, state} <- PlanningRepository.get_state(repo, work_package_id), {:ok, requirement} <- required_review_requirement(state.work_package), {:ok, head_sha} <- required_current_review_head(state.progress_events), - payload <- review_completion_payload(work_package_id, requirement, head_sha, reference, note), - arguments <- review_completion_arguments(requirement, head_sha, note), + :ok <- require_rework_head_advanced(state.progress_events, head_sha), + :ok <- require_new_review_package_after_rework(state.progress_events, head_sha), + rework_id = accepted_review_rework_id(state.progress_events), + payload <- review_completion_payload(work_package_id, requirement, head_sha, reference, note, rework_id), + arguments <- review_completion_arguments(requirement, head_sha, note, rework_id), {:ok, result} <- ProgressEvents.append_metadata(repo, session, arguments, "complete_review", "review_complete", payload), {:ok, result} <- put_remaining_readiness_gates(repo, session, result) do result @@ -416,7 +442,7 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ReviewReadiness do end end - defp review_completion_payload(work_package_id, requirement, head_sha, reference, note) do + defp review_completion_payload(work_package_id, requirement, head_sha, reference, note, rework_id) do %{ "type" => "review_completion", "source_tool" => "complete_review", @@ -427,19 +453,21 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ReviewReadiness do } |> put_if_present("reference", reference) |> put_if_present("note", note) + |> put_if_present("accepted_review_rework_id", rework_id) end - defp review_completion_arguments(requirement, head_sha, note) do + defp review_completion_arguments(requirement, head_sha, note, rework_id) do %{ "summary" => "Required review completed", "body" => note, "status" => "review_complete", - "idempotency_key" => review_completion_idempotency_key(requirement, head_sha) + "idempotency_key" => review_completion_idempotency_key(requirement, head_sha, rework_id) } end - defp review_completion_idempotency_key(requirement, head_sha) do - digest = :crypto.hash(:sha256, Jason.encode!([head_sha, requirement])) |> Base.url_encode64(padding: false) + defp review_completion_idempotency_key(requirement, head_sha, rework_id) do + inputs = if is_binary(rework_id), do: [head_sha, requirement, rework_id], else: [head_sha, requirement] + digest = :crypto.hash(:sha256, Jason.encode!(inputs)) |> Base.url_encode64(padding: false) "current:" <> digest end @@ -472,6 +500,8 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ReviewReadiness do {merge_metadata_missing?(state, "branch"), "branch_attached"}, {merge_metadata_missing?(state, "pr"), "pr_attached"}, {current_pr_state_missing?(state), "current_pr_state"}, + {rework_head_not_advanced?(state), "rework_head_advanced"}, + {rework_current_pr_state_missing?(state), "rework_current_pr_state"}, {ScopeGuard.missing?(state.work_package, state.progress_events), @scope_guard_gate}, {review_artifacts_missing?(state), "review_artifacts_attached"}, {review_current_head_missing?(state), "review_current_head"}, @@ -563,6 +593,8 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ReviewReadiness do defp readiness_failure_message("branch_attached"), do: "Current branch metadata is missing." defp readiness_failure_message("pr_attached"), do: "Current PR metadata is missing." defp readiness_failure_message("current_pr_state"), do: "Current synced PR state is missing." + defp readiness_failure_message("rework_head_advanced"), do: "Accepted review rework requires a different exact head." + defp readiness_failure_message("rework_current_pr_state"), do: "Accepted review rework requires fresh synced PR state for the new head." defp readiness_failure_message("review_artifacts_attached"), do: "Current-head validation artifacts are missing." defp readiness_failure_message("review_current_head"), do: "Required review cannot be completed until the current exact head is attached." defp readiness_failure_message("review_complete"), do: "Required review is not completed for the current exact head and requirement." @@ -593,6 +625,42 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ReviewReadiness do not current_pr_state_present?(state.progress_events, current_head_sha) end + defp rework_head_not_advanced?(state) do + case latest_accepted_review_rework(state.progress_events) do + %ProgressEvent{payload: payload} -> + current_head_sha = latest_current_head_sha(state.progress_events) + not is_binary(current_head_sha) or exact_head_sha?(Map.get(payload, "head_sha"), current_head_sha) + + nil -> + false + end + end + + defp rework_current_pr_state_missing?(state) do + not is_nil(latest_accepted_review_rework(state.progress_events)) and + not fresh_rework_pr_state_present?(state.progress_events) + end + + defp fresh_rework_pr_state_present?(progress_events) do + current_head_sha = latest_current_head_sha(progress_events) + + case latest_attached_pr_ref(progress_events) do + {:ok, attached_ref} -> + Enum.any?(events_after_latest_rework(progress_events), &fresh_rework_pr_state_event?(&1, attached_ref, current_head_sha)) + + {:tool_error, _reason} -> + false + end + end + + defp fresh_rework_pr_state_event?(%ProgressEvent{payload: payload} = event, attached_ref, current_head_sha) + when is_map(payload) do + payload_type?(event, "pr", "sync_pr") and exact_head_sha?(payload["head_sha"], current_head_sha) and + pr_payload_ref(payload) == attached_ref and current_pr_state_payload?(payload) + end + + defp fresh_rework_pr_state_event?(%ProgressEvent{}, _attached_ref, _current_head_sha), do: false + defp review_current_head_missing?(%{work_package: %WorkPackage{review_requirement: nil}}), do: false defp review_current_head_missing?(state), do: is_nil(latest_current_head_sha(state.progress_events)) @@ -604,7 +672,7 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ReviewReadiness do is_nil(head_sha) or not MetadataProjection.review_completion_present?( - state.progress_events, + events_after_latest_rework(state.progress_events), state.work_package.id, head_sha, requirement @@ -643,10 +711,68 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ReviewReadiness do defp latest_review_package_event(progress_events, current_head_sha) do progress_events + |> events_after_latest_rework() |> current_head_review_package_events(current_head_sha) |> List.last() end + defp require_rework_head_advanced(progress_events, head_sha) do + case latest_accepted_review_rework(progress_events) do + %ProgressEvent{payload: payload} -> + current_head_sha = latest_current_head_sha(progress_events) + + cond do + not exact_head_sha?(head_sha, current_head_sha) -> {:tool_error, "rework_review_head_not_current"} + exact_head_sha?(Map.get(payload, "head_sha"), head_sha) -> {:tool_error, "rework_head_not_advanced"} + true -> :ok + end + + nil -> + :ok + end + end + + defp require_new_review_package_after_rework(progress_events, head_sha) do + case latest_accepted_review_rework(progress_events) do + %ProgressEvent{} -> + if Enum.any?(events_after_latest_rework(progress_events), &exact_review_package_event?(&1, head_sha)), + do: :ok, + else: {:tool_error, "new_review_package_required"} + + nil -> + :ok + end + end + + defp exact_review_package_event?(%ProgressEvent{payload: payload} = event, head_sha) do + payload_type?(event, "review_package", "submit_review_package") and exact_head_sha?(payload["head_sha"], head_sha) + end + + defp accepted_review_rework_id(progress_events) do + case latest_accepted_review_rework(progress_events) do + %ProgressEvent{id: id} -> id + nil -> nil + end + end + + defp latest_accepted_review_rework(progress_events) do + progress_events + |> Enum.filter(&payload_type?(&1, "accepted_review_rework", "accept_review_rework")) + |> List.last() + end + + defp events_after_latest_rework(progress_events) do + case latest_accepted_review_rework(progress_events) do + %ProgressEvent{id: id} -> + progress_events + |> Enum.drop_while(&(&1.id != id)) + |> Enum.drop(1) + + nil -> + progress_events + end + end + defp current_head_review_package_events(progress_events, current_head_sha) do Enum.filter(progress_events, fn event -> payload_type?(event, "review_package", "submit_review_package") and current_head_review_package?(event, current_head_sha) @@ -1083,6 +1209,12 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ReviewReadiness do defp head_sha_matches?(_left, _right), do: false + defp exact_head_sha?(left, right) when is_binary(left) and is_binary(right) do + String.downcase(String.trim(left)) == String.downcase(String.trim(right)) + end + + defp exact_head_sha?(_left, _right), do: false + defp payload_type?(%ProgressEvent{payload: payload}, type, source_tools) when is_map(payload) and is_list(source_tools) do Map.get(payload, "type") == type and Map.get(payload, "source_tool") in source_tools end diff --git a/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/server.ex b/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/server.ex index 5592cba0a5..6fa67aebcf 100644 --- a/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/server.ex +++ b/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/server.ex @@ -1263,6 +1263,7 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.Server do defp architect_tool(name, arguments, %__MODULE__{config: config, session: session}) when name in [ "reconcile_work_request", + "accept_review_rework", "record_work_package_delivery", "cleanup_work_request_work_package_runtime", "revoke_work_package_worker_key" @@ -1752,8 +1753,9 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.Server do defp architect_tool_capability("read_delivery_board"), do: "read:work_request" defp architect_tool_capability("reconcile_work_request"), do: "read:work_request" - defp architect_tool_capability(tool) when tool in ["cleanup_work_request_work_package_runtime", "record_work_package_delivery", "revoke_work_package_worker_key"], - do: "write:work_request" + defp architect_tool_capability(tool) + when tool in ["accept_review_rework", "cleanup_work_request_work_package_runtime", "record_work_package_delivery", "revoke_work_package_worker_key"], + do: "write:work_request" defp architect_tool_capability("list_guidance_requests"), do: "read:guidance_request" defp architect_tool_capability("read_guidance_request"), do: "read:guidance_request" diff --git a/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/tool_catalog.ex b/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/tool_catalog.ex index 3cec796c2d..ed76cba155 100644 --- a/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/tool_catalog.ex +++ b/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/tool_catalog.ex @@ -78,6 +78,7 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ToolCatalog do "resolve_blocker", "read_delivery_board", "reconcile_work_request", + "accept_review_rework", "cleanup_work_request_work_package_runtime", "record_work_package_delivery", "revoke_work_package_worker_key", @@ -148,6 +149,7 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ToolCatalog do [ "read_delivery_board", "reconcile_work_request", + "accept_review_rework", "cleanup_work_request_work_package_runtime", "record_work_package_delivery", "revoke_work_package_worker_key", @@ -155,6 +157,7 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ToolCatalog do ] @delivery_policy_tools [ "reconcile_work_request", + "accept_review_rework", "cleanup_work_request_work_package_runtime", "record_work_package_delivery", "revoke_work_package_worker_key" diff --git a/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/tool_catalog/input_schemas.ex b/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/tool_catalog/input_schemas.ex index 9b8cfaf73e..c5c2fd9e76 100644 --- a/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/tool_catalog/input_schemas.ex +++ b/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/tool_catalog/input_schemas.ex @@ -364,6 +364,27 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ToolCatalog.InputSchemas do ) end + def architect_tool_input_schema("accept_review_rework") do + schema( + %{ + "work_request_id" => current_work_request_id_schema(), + "work_package_id" => described_string_schema("Ordinary ready-for-merge WorkPackage with the accepted finding."), + "idempotency_key" => described_string_schema("Opaque stable key for this accepted finding."), + "evidence" => + schema( + %{ + "provider" => described_string_schema("Provider or review system that produced the typed finding."), + "reference" => described_string_schema("Opaque provider reference for the immutable evidence."), + "head_sha" => described_string_schema("Exact current head of the attached PR."), + "finding" => markdown_string_schema("Verified nonempty changes-required finding in Markdown.") + }, + ["provider", "reference", "head_sha", "finding"] + ) + }, + ["work_package_id", "idempotency_key", "evidence"] + ) + end + def architect_tool_input_schema("add_comment") do schema( scoped_properties(%{ diff --git a/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/tool_catalog/surface_specs.ex b/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/tool_catalog/surface_specs.ex index 3fde65bfc1..9b3797477c 100644 --- a/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/tool_catalog/surface_specs.ex +++ b/elixir/lib/symphony_elixir/symphony_plus_plus/mcp/tool_catalog/surface_specs.ex @@ -224,6 +224,10 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.ToolCatalog.SurfaceSpecs do defp architect_tool_description("read_delivery_board"), do: "Read the scoped WorkRequest delivery-board projection for visible work-package closeout without broad package visibility." defp architect_tool_description("reconcile_work_request"), do: "Dry-run or apply deterministic WorkRequest delivery closeout repairs from structured PR/GitHub evidence." + defp architect_tool_description("accept_review_rework") do + "Accept one verified typed changes-required finding for an ordinary ready-for-merge WorkPackage's current attached PR and exact head, preserve immutable evidence, and atomically return it to active rework." + end + defp architect_tool_description("record_work_package_delivery") do "Record an idempotent work-package delivery closeout. Required evidence depends on outcome: pr_merged needs PR evidence, completed_no_pr needs direct evidence, superseded needs successor and reason, and abandoned needs rationale. Use abandoned for cleaned no-code failed dispatches that never reached implementation. Terminal delivery clears any active blocker residue." end diff --git a/elixir/priv/symphony_plus_plus/mcp_contract.json b/elixir/priv/symphony_plus_plus/mcp_contract.json index a41e66364c..16a2c48b9a 100644 --- a/elixir/priv/symphony_plus_plus/mcp_contract.json +++ b/elixir/priv/symphony_plus_plus/mcp_contract.json @@ -1,6 +1,6 @@ { "version": 7, - "mcp_contract_fingerprint": "3a7556b6d6bac4c1684f76b694e9d2bd5a69b57133ecef7f680d341fb0c9f083", + "mcp_contract_fingerprint": "6300d025dfa54155c02fc4c297d310824b85741b33113a4696f0659c9f15e4f4", "tool_sets": { "unbound_tools": [ "sympp.health", @@ -64,6 +64,7 @@ "resolve_blocker", "read_delivery_board", "reconcile_work_request", + "accept_review_rework", "cleanup_work_request_work_package_runtime", "record_work_package_delivery", "revoke_work_package_worker_key", diff --git a/elixir/test/symphony_elixir/symphony_plus_plus/github_pull_request_test.exs b/elixir/test/symphony_elixir/symphony_plus_plus/github_pull_request_test.exs index 6339fa884a..ab94fcd043 100644 --- a/elixir/test/symphony_elixir/symphony_plus_plus/github_pull_request_test.exs +++ b/elixir/test/symphony_elixir/symphony_plus_plus/github_pull_request_test.exs @@ -1,7 +1,33 @@ defmodule SymphonyElixir.SymphonyPlusPlus.GitHubPullRequestTest do use ExUnit.Case, async: true - alias SymphonyElixir.SymphonyPlusPlus.GitHub.{Client, DryClient, PullRequest} + alias SymphonyElixir.SymphonyPlusPlus.GitHub.{Client, DryClient, PullRequest, PullRequestProgress} + alias SymphonyElixir.SymphonyPlusPlus.Planning.ProgressEvent + + test "orders provider head evidence by the DateTime instant across second boundaries" do + earlier = %ProgressEvent{ + id: "earlier", + sequence: 1, + created_at: ~U[2026-08-17 11:44:17.975000Z], + payload: %{"type" => "branch", "source_tool" => "attach_branch", "head_sha" => "head-a"} + } + + later = %ProgressEvent{ + id: "later", + sequence: 2, + created_at: ~U[2026-08-17 11:44:18.427000Z], + payload: %{"type" => "branch", "source_tool" => "attach_branch", "head_sha" => "head-b"} + } + + assert PullRequestProgress.chronological_events([later, earlier]) == [earlier, later] + assert PullRequestProgress.expected_head_sha([later, earlier], %{}) == "head-b" + + subsecond_earlier = %{earlier | id: "subsecond-earlier", sequence: 4, created_at: ~U[2026-08-17 11:44:17.427000Z]} + subsecond_later = %{later | id: "subsecond-later", sequence: 3, created_at: ~U[2026-08-17 11:44:17.975000Z]} + + assert PullRequestProgress.chronological_events([subsecond_later, subsecond_earlier]) == + [subsecond_earlier, subsecond_later] + end test "parses GitHub PR URLs" do assert {:ok, ref} = PullRequest.parse(%{"url" => "https://github.com/nextide/symphony-plus-plus/pull/42"}, nil) diff --git a/elixir/test/symphony_elixir/symphony_plus_plus/mcp/accepted_review_rework_test.exs b/elixir/test/symphony_elixir/symphony_plus_plus/mcp/accepted_review_rework_test.exs new file mode 100644 index 0000000000..0ee2c96329 --- /dev/null +++ b/elixir/test/symphony_elixir/symphony_plus_plus/mcp/accepted_review_rework_test.exs @@ -0,0 +1,368 @@ +Code.require_file("../../../support/symphony_plus_plus/mcp_case.exs", __DIR__) + +defmodule SymphonyElixir.SymphonyPlusPlus.MCP.AcceptedReviewReworkTest do + use SymphonyElixir.SymphonyPlusPlus.MCPCase + + alias SymphonyElixir.SymphonyPlusPlus.GitHub.PullRequestProgress + + @head_a "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + test "an accepted current-head finding atomically reopens once and requires fresh head-B evidence", %{repo: repo} do + fixture = TestSupport.git_repo_fixture!("main", prefix: "sympp-accepted-review-rework") + branch = "agent/accepted-review-rework" + worktree = Path.join(fixture.root, "worktree") + TestSupport.git_output!(fixture.repo_root, ["worktree", "add", "-b", branch, worktree, "HEAD"]) + TestSupport.git_output!(fixture.repo_root, ["remote", "set-url", "origin", "https://github.com/nextide/symphony-plus-plus.git"]) + head_a = fixture.repo_root |> TestSupport.git_output!(["rev-parse", "HEAD"]) |> String.trim() + + work_request = + create_work_request!(repo, + id: "WR-MCP-ACCEPTED-REWORK", + repo: "nextide/symphony-plus-plus", + base_branch: "main", + status: "ready_for_slicing" + ) + + review_requirement = %{"type" => "automated", "args" => %{"reviewer" => "review-suite", "mode" => "fast"}} + + assert {:ok, package} = + CanonicalWorkPackageFixtures.add_work_package( + repo, + work_request.id, + work_request_work_package_attrs( + id: "WP-MCP-ACCEPTED-REWORK", + kind: "mcp", + status: "active", + base_branch: "main", + branch_pattern: branch, + worktree_path: worktree, + worktree_target_repo_root: fixture.repo_root, + review_requirement: review_requirement, + dispatched_at: DateTime.utc_now(:microsecond) + ) + ) + + append_done_plan(repo, package.id) + assert {:ok, minted} = AccessGrantService.mint_worker_grant(repo, package.id) + assert {:ok, worker_assignment} = AccessGrantService.claim(repo, minted.work_key.secret, claimed_by: "worker-1") + worker_session = MCPHarness.session(worker_assignment, proof_hash: minted.grant.secret_hash) + + assert {:ok, lease} = + ClaimLeaseService.claim( + repo, + package.id, + %{"actor_kind" => "agent", "actor_id" => "worker-1", "actor_display_name" => "worker-1"}, + access_grant_id: minted.grant.id, + stale_after_ms: :timer.hours(1) + ) + + pr_url = "https://github.com/nextide/symphony-plus-plus/pull/9369" + attach_tool(repo, worker_session, "attach_branch", %{"branch" => package.branch_pattern, "head_sha" => head_a}) + attach_tool(repo, worker_session, "attach_pr", %{"url" => pr_url, "head_sha" => head_a}) + sync_pr_state(repo, worker_session, pr_url, head_a) + review_a = submit_review(repo, worker_session, head_a, "review-a") + completion_a = attach_tool(repo, worker_session, "complete_review", %{"reference" => "review-a-complete"}) + + assert get_in(mcp_tool(repo, worker_session, "mark_ready", %{}), ["result", "structuredContent", "work_package", "status"]) == + "ready_for_merge" + + {_anchor, architect_session, architect_grant} = + create_work_request_handoff_architect_session(repo, work_request, ["read:work_request", "write:work_request"]) + + comment = + mcp_tool(repo, architect_session, "add_comment", %{ + "target_kind" => "work_package", + "target_id" => package.id, + "body" => "changes requested" + }) + + assert get_in(comment, ["result", "structuredContent", "comment", "id"]) + assert repo.get!(WorkPackage, package.id).status == "ready_for_merge" + + rejected_prose = + mcp_tool(repo, worker_session, "append_progress", %{ + "summary" => "changes requested", + "idempotency_key" => "raw-comment-cannot-reopen" + }) + + assert get_in(rejected_prose, ["error", "data", "reason"]) == "already_ready" + + package_before = package_contract(repo.get!(WorkPackage, package.id)) + grant_before = immutable_record(repo.get!(AccessGrant, minted.grant.id)) + architect_grant_before = immutable_record(architect_grant) + lease_before = immutable_record(lease) + arguments = accepted_rework_arguments(work_request.id, package.id, head_a) + + assert {:ok, _merged_snapshot} = + PlanningRepository.append_audit_progress_event_for_work_package( + repo, + worker_assignment, + package.id, + %{ + "summary" => "Current provider snapshot reports merged", + "status" => "pr_synced", + "idempotency_key" => "merged-provider-snapshot", + "payload" => + provider_snapshot(pr_url, head_a, %{ + "status" => "merged", + "merged" => true + }) + } + ) + + assert {:ok, events_before_merged_rejection} = PlanningRepository.list_progress_events(repo, package.id) + + assert get_in(mcp_tool(repo, architect_session, "accept_review_rework", arguments), ["error", "data", "reason"]) == + "current_attached_pr_already_merged" + + assert repo.get!(WorkPackage, package.id).status == "ready_for_merge" + assert {:ok, events_after_merged_rejection} = PlanningRepository.list_progress_events(repo, package.id) + assert Enum.map(events_after_merged_rejection, & &1.id) == Enum.map(events_before_merged_rejection, & &1.id) + + assert {:ok, _clean_snapshot} = + PlanningRepository.append_audit_progress_event_for_work_package( + repo, + worker_assignment, + package.id, + %{ + "summary" => "Current provider snapshot reports open", + "status" => "pr_synced", + "idempotency_key" => "open-provider-snapshot", + "payload" => provider_snapshot(pr_url, head_a, %{"status" => "clean", "merged" => false}) + } + ) + + abbreviated = put_in(arguments, ["evidence", "head_sha"], String.slice(head_a, 0, 12)) + + assert get_in(mcp_tool(repo, architect_session, "accept_review_rework", abbreviated), ["error", "data", "reason"]) == + "stale_rework_head" + + responses = + 1..2 + |> Task.async_stream(fn _ -> mcp_tool(repo, architect_session, "accept_review_rework", arguments) end, + max_concurrency: 2, + ordered: false, + timeout: 10_000 + ) + |> Enum.map(fn {:ok, response} -> response end) + + assert Enum.all?(responses, &(get_in(&1, ["result", "structuredContent", "work_package", "status"]) == "active")) + assert [accepted_event_id] = responses |> Enum.map(&get_in(&1, ["result", "structuredContent", "accepted_review_rework", "id"])) |> Enum.uniq() + + assert {:ok, progress_events} = PlanningRepository.list_progress_events(repo, package.id) + assert Enum.count(progress_events, &accepted_rework_event?/1) == 1 + accepted_event = Enum.find(progress_events, &(&1.id == accepted_event_id)) + assert accepted_event.payload["reference"] == "mcpdiag_9369db53009417dd" + assert accepted_event.payload["provider"] == "review-suite" + assert accepted_event.payload["finding"] == "The exact reviewed head has a reachable changes-required finding." + assert accepted_event.payload["head_sha"] == head_a + + assert accepted_event.payload["pr"] == %{ + "url" => pr_url, + "repository" => "nextide/symphony-plus-plus", + "number" => 9369, + "head_sha" => head_a + } + + conflict = put_in(arguments, ["evidence", "finding"], "A conflicting finding.") + + assert get_in(mcp_tool(repo, architect_session, "accept_review_rework", conflict), ["error", "data", "reason"]) == + "idempotency_conflict" + + assert get_in( + mcp_tool(repo, architect_session, "accept_review_rework", Map.put(arguments, "idempotency_key", "second-acceptance")), + ["error", "data", "reason"] + ) == "work_package_not_ready_for_rework" + + assert package_contract(repo.get!(WorkPackage, package.id)) == package_before + assert immutable_record(repo.get!(AccessGrant, minted.grant.id)) == grant_before + assert immutable_record(repo.get!(AccessGrant, architect_grant.id)) == architect_grant_before + assert {:ok, current_lease} = ClaimLeaseService.current_for_work_package(repo, package.id) + assert immutable_record(current_lease) == lease_before + + assert get_in(submit_review(repo, worker_session, head_a, "review-a-retry"), ["error", "data", "reason"]) == + "rework_head_not_advanced" + + assert get_in(mcp_tool(repo, worker_session, "complete_review", %{}), ["error", "data", "reason"]) == + "rework_head_not_advanced" + + stale_ready = mcp_tool(repo, worker_session, "mark_ready", %{}) + assert "rework_head_advanced" in get_in(stale_ready, ["error", "data", "missing"]) + + File.write!(Path.join(worktree, "unreviewed-head.txt"), "unreviewed\n") + TestSupport.git_output!(worktree, ["add", "unreviewed-head.txt"]) + TestSupport.git_output!(worktree, ["commit", "-m", "Unreviewed intermediate head"]) + intermediate_head = worktree |> TestSupport.git_output!(["rev-parse", "HEAD"]) |> String.trim() + attach_tool(repo, worker_session, "attach_branch", %{"branch" => package.branch_pattern, "head_sha" => intermediate_head}) + + assert get_in(mcp_tool(repo, worker_session, "complete_review", %{}), ["error", "data", "reason"]) == + "new_review_package_required" + + File.write!(Path.join(worktree, "head-b.txt"), "head B\n") + TestSupport.git_output!(worktree, ["add", "head-b.txt"]) + TestSupport.git_output!(worktree, ["commit", "-m", "Head B"]) + head_b = worktree |> TestSupport.git_output!(["rev-parse", "HEAD"]) |> String.trim() + config = Config.default(repo: repo, repo_root: fixture.repo_root) + review_b = submit_live_review(repo, worker_session, config, head_b, "review-b") + assert {:ok, refreshed_events} = PlanningRepository.list_progress_events(repo, package.id) + + assert refreshed_events + |> Enum.filter(&(&1.payload["type"] == "branch")) + |> List.last() + |> get_in([Access.key(:payload), "head_sha"]) == head_b + + attach_tool(repo, worker_session, "attach_pr", %{"url" => pr_url, "head_sha" => head_b}) + + assert {:ok, provider_events} = PlanningRepository.list_progress_events(repo, package.id) + + assert PullRequestProgress.expected_head_sha(provider_events, %{repository: "nextide/symphony-plus-plus", number: 9369}) == head_b, + inspect(Enum.map(provider_events, &{&1.sequence, &1.created_at, &1.payload["source_tool"], &1.payload["head_sha"]})) + + assert get_in(submit_review(repo, worker_session, String.slice(head_b, 0, 12), "review-b-short"), [ + "error", + "data", + "reason" + ]) == "rework_review_head_not_current" + + before_sync = mcp_tool(repo, worker_session, "mark_ready", %{}) + assert "rework_current_pr_state" in get_in(before_sync, ["error", "data", "missing"]) + + sync_pr_state(repo, worker_session, pr_url, head_b) + + before_completion = mcp_tool(repo, worker_session, "mark_ready", %{}) + assert "review_complete" in get_in(before_completion, ["error", "data", "missing"]) + completion_b = attach_tool(repo, worker_session, "complete_review", %{"reference" => "review-b-complete"}) + ready_b = mcp_tool(repo, worker_session, "mark_ready", %{}) + assert get_in(ready_b, ["result", "structuredContent", "work_package", "status"]) == "ready_for_merge" + + assert get_in(review_a, ["result", "structuredContent", "progress_event", "id"]) != + get_in(review_b, ["result", "structuredContent", "progress_event", "id"]) + + assert get_in(completion_a, ["result", "structuredContent", "progress_event", "id"]) != + get_in(completion_b, ["result", "structuredContent", "progress_event", "id"]) + + assert {:ok, final_events} = PlanningRepository.list_progress_events(repo, package.id) + assert Enum.any?(final_events, &(&1.id == accepted_event_id and &1.payload["head_sha"] == head_a)) + assert Enum.any?(final_events, &(&1.payload["type"] == "review_package" and &1.payload["head_sha"] == head_a)) + assert Enum.any?(final_events, &(&1.payload["type"] == "review_package" and &1.payload["head_sha"] == head_b)) + end + + test "phase children and terminal packages cannot use accepted review rework", %{repo: repo} do + work_request = create_work_request!(repo, id: "WR-MCP-REWORK-REJECTED", status: "ready_for_slicing") + assert {:ok, _phase} = PhaseRepository.create(repo, %{id: "phase-rework-rejected", title: "Rejected rework phase"}) + + assert {:ok, parent} = + CanonicalWorkPackageFixtures.add_work_package( + repo, + work_request.id, + work_request_work_package_attrs(id: "WP-MCP-REWORK-PARENT", status: "active") + ) + + assert {:ok, child} = + CanonicalWorkPackageFixtures.add_work_package( + repo, + work_request.id, + work_request_work_package_attrs( + id: "WP-MCP-REWORK-CHILD", + kind: "phase_child", + status: "ready_for_architect_merge", + parent_id: parent.id, + phase_id: "phase-rework-rejected" + ) + ) + + assert {:ok, terminal} = + CanonicalWorkPackageFixtures.add_work_package( + repo, + work_request.id, + work_request_work_package_attrs(id: "WP-MCP-REWORK-TERMINAL", status: "merged") + ) + + {_anchor, session, _grant} = + create_work_request_handoff_architect_session(repo, work_request, ["read:work_request", "write:work_request"]) + + assert get_in(mcp_tool(repo, session, "accept_review_rework", accepted_rework_arguments(work_request.id, child.id)), [ + "error", + "data", + "reason" + ]) == "phase_child_rework_not_allowed" + + assert get_in(mcp_tool(repo, session, "accept_review_rework", accepted_rework_arguments(work_request.id, terminal.id)), [ + "error", + "data", + "reason" + ]) == "work_package_not_ready_for_rework" + end + + defp submit_review(repo, session, head_sha, idempotency_key) do + mcp_tool(repo, session, "submit_review_package", review_arguments(head_sha, idempotency_key)) + end + + defp submit_live_review(repo, session, config, head_sha, idempotency_key) do + response = + MCPHarness.request( + %{ + "jsonrpc" => "2.0", + "id" => idempotency_key, + "method" => "tools/call", + "params" => %{"name" => "submit_review_package", "arguments" => review_arguments(head_sha, idempotency_key)} + }, + repo: repo, + session: session, + config: config + ) + + assert get_in(response, ["result", "structuredContent", "progress_event", "id"]) + response + end + + defp review_arguments(head_sha, idempotency_key) do + %{ + "summary" => "Validated #{head_sha}", + "tests" => ["mix test accepted_review_rework_test.exs"], + "artifacts" => ["review-suite-#{head_sha}.json"], + "head_sha" => head_sha, + "acceptance_criteria_met" => true, + "idempotency_key" => idempotency_key + } + end + + defp accepted_rework_arguments(work_request_id, work_package_id, head_sha \\ @head_a) do + %{ + "work_request_id" => work_request_id, + "work_package_id" => work_package_id, + "idempotency_key" => "accepted-finding-9369", + "evidence" => %{ + "provider" => " review-suite ", + "reference" => " mcpdiag_9369db53009417dd ", + "head_sha" => head_sha, + "finding" => " The exact reviewed head has a reachable changes-required finding. " + } + } + end + + defp accepted_rework_event?(%ProgressEvent{payload: payload}) do + payload["type"] == "accepted_review_rework" and payload["source_tool"] == "accept_review_rework" + end + + defp provider_snapshot(url, head_sha, merge_state) do + %{ + "type" => "pr", + "source_tool" => "sync_pr", + "url" => url, + "repository" => "nextide/symphony-plus-plus", + "number" => 9369, + "head_sha" => head_sha, + "check_summary" => %{"status" => "passing"}, + "review_state" => %{"status" => "approved"}, + "merge_state" => merge_state + } + end + + defp package_contract(package) do + package |> immutable_record() |> Map.drop([:status, :dispatched_at]) + end + + defp immutable_record(struct) do + struct |> Map.from_struct() |> Map.drop([:__meta__, :inserted_at, :updated_at]) + end +end diff --git a/elixir/test/symphony_elixir/symphony_plus_plus/mcp/worker_tools_ready_gate_test.exs b/elixir/test/symphony_elixir/symphony_plus_plus/mcp/worker_tools_ready_gate_test.exs index d8865a432b..a404e1335a 100644 --- a/elixir/test/symphony_elixir/symphony_plus_plus/mcp/worker_tools_ready_gate_test.exs +++ b/elixir/test/symphony_elixir/symphony_plus_plus/mcp/worker_tools_ready_gate_test.exs @@ -264,6 +264,11 @@ defmodule SymphonyElixir.SymphonyPlusPlus.MCP.WorkerToolsReadyGateTest do assert String.ends_with?(get_in(completion_payload, ["review", "args", "context"]), "[truncated]") assert get_in(completion, ["result", "structuredContent", "remaining_readiness_gates"]) == [] + legacy_digest = :crypto.hash(:sha256, Jason.encode!(["review-head-a", review])) |> Base.url_encode64(padding: false) + + assert get_in(completion, ["result", "structuredContent", "progress_event", "idempotency_key"]) == + "complete_review:#{package.id}:current:#{legacy_digest}" + replay = attach_tool(repo, session, "complete_review", %{ "reference" => "human-review-42", diff --git a/plugins/symphony-plus-plus-mcp/skills/symphony-architect/SKILL.md b/plugins/symphony-plus-plus-mcp/skills/symphony-architect/SKILL.md index 65978a4c3b..ec1c162d3e 100644 --- a/plugins/symphony-plus-plus-mcp/skills/symphony-architect/SKILL.md +++ b/plugins/symphony-plus-plus-mcp/skills/symphony-architect/SKILL.md @@ -170,6 +170,14 @@ Workers own implementation, tests, any declared review, GitHub review when required, CI/static gates when present, and PR readiness. Do not take over their review loop; send important findings back to the worker. +For an ordinary `ready_for_merge` package, call `accept_review_rework` only +after you accept a verified typed changes-required finding for its current +attached PR and exact head. Supply a nonempty finding, provider identity, +opaque reference, and stable idempotency key. Comments, provider wording, +severity labels, and worker prose do not authorize rework. The operation +preserves the package contract and runtime authority while returning it to +`active` once for that accepted evidence. + ## Guidance - Answer package guidance when recorded intent already decides it. diff --git a/plugins/symphony-plus-plus-mcp/skills/symphony-work-package/SKILL.md b/plugins/symphony-plus-plus-mcp/skills/symphony-work-package/SKILL.md index 1830163292..35288c192d 100644 --- a/plugins/symphony-plus-plus-mcp/skills/symphony-work-package/SKILL.md +++ b/plugins/symphony-plus-plus-mcp/skills/symphony-work-package/SKILL.md @@ -141,7 +141,11 @@ Before `mark_ready()`: blockers still require the architect or trusted local operator. After `mark_ready()` succeeds, evidence is frozen except idempotent replay of -already-recorded writes. +already-recorded writes. If an architect accepts a verified review finding and +returns the package to `active`, advance to a different exact head, run +`sync_pr` for fresh provider state, submit a new review package, and complete +the required review. Evidence for the old head remains immutable but cannot +satisfy readiness again. Return ready or terminal packages to the architect named by `next_owner`; the worker does not need or receive architect tools for that handoff. diff --git a/plugins/symphony-plus-plus-mcp/skills/symphony-work-package/references/worker_prompt.md b/plugins/symphony-plus-plus-mcp/skills/symphony-work-package/references/worker_prompt.md index e444114d66..30a6a97249 100644 --- a/plugins/symphony-plus-plus-mcp/skills/symphony-work-package/references/worker_prompt.md +++ b/plugins/symphony-plus-plus-mcp/skills/symphony-work-package/references/worker_prompt.md @@ -100,6 +100,10 @@ Before ready: lifecycle calls only to restate existing evidence. Resolve worker-owned blockers with `resolve_blocker`; architect-owned human blockers still require the architect or trusted local operator. +8. If the architect accepts a review finding after readiness, advance to a + different exact head, refresh the attached PR with `sync_pr`, submit a new + review package, and complete the required review. Old-head evidence remains + in the ledger but is not readiness evidence for the rework cycle. Final output: - PR URL and final head SHA. diff --git a/plugins/symphony-plus-plus-mcp/skills/symphony-worker/SKILL.md b/plugins/symphony-plus-plus-mcp/skills/symphony-worker/SKILL.md index 4701ad8801..9c388fa1fe 100644 --- a/plugins/symphony-plus-plus-mcp/skills/symphony-worker/SKILL.md +++ b/plugins/symphony-plus-plus-mcp/skills/symphony-worker/SKILL.md @@ -65,6 +65,10 @@ seeking architect-only tools. - If CI/checks exist, make sure they are green or report the exact blocker. If no CI exists, say so. - After material changes, rerun any declared review for the new exact head. +- If an architect accepts review rework after readiness, keep the old evidence + as history and advance to a different exact head. Refresh the attached PR + with `sync_pr`, submit a new review package, and complete the required review + before calling `mark_ready` again. - Record validation and review evidence in the active Symphony++ state. For WorkPackages, that state is the ledger-backed claim opened by the WorkPackage skill.