diff --git a/docs/architecture/sdlc-pipeline.md b/docs/architecture/sdlc-pipeline.md index b342004a88..ad22a55d66 100644 --- a/docs/architecture/sdlc-pipeline.md +++ b/docs/architecture/sdlc-pipeline.md @@ -83,7 +83,7 @@ The contract is a JSON document tracking the complete state of an issue through ```json { - "schemaVersion": "1.0", + "schemaVersion": "1.1", "issue": { "number": 133, "title": "...", "url": "..." }, "current_phase": "implement", "slices": [{ @@ -116,6 +116,27 @@ The contract is a JSON document tracking the complete state of an issue through > existing imports. See [Slice-DAG Implement Phase](slice-dag.md) for > the full design. +> **Schema 1.1 (#2548)**: `schemaVersion` was bumped from `1.0` to `1.1` +> to track the addition of four optional `pr.context_*` fields on +> `PRMetadata` (`context_title`, `context_description`, `context_branch`, +> `context_pr_number`) used by the dedicated context-PR mechanism. The +> bump is purely additive — pre-1.1 contracts load transparently via a +> Pydantic `model_validator(mode="after")` migration that stamps +> `schemaVersion = "1.1"` on every load when the on-disk value is exactly +> `"1.0"`; the migration is silent (no audit-log entry) and idempotent. +> `context_title` / `context_description` are planner-emitted optional +> framing for the strategic-plan PR; `context_branch` / +> `context_pr_number` are populated by the orchestrator after the context +> branch is created and the context PR is opened. +> +> **As of slice-1 (#2548 part 1)**, only the schema fields and the +> planner-prompt advertisement are wired. The orchestrator +> branch-creation and PR-opening hooks land in #2548 slices 3-4 — until +> those slices merge, the four `pr.context_*` fields are +> forward-compatibly inert: planners may emit `context_title` / +> `context_description` and the values flow into `PRMetadata`, but +> nothing acts on them yet. + ## HITL (Human-in-the-Loop) Mechanism For detailed HITL workflow documentation, see [HITL Decisions](../hitl-decisions.md). diff --git a/docs/guides/concurrent-execution.md b/docs/guides/concurrent-execution.md index 8ea8ce67bd..f9fd11d324 100644 --- a/docs/guides/concurrent-execution.md +++ b/docs/guides/concurrent-execution.md @@ -705,7 +705,11 @@ When the orchestrator auto-creates the PR (during the PR phase), it includes a o > _Per-phase BRC transcripts: [`refine`](./.egg-state/brc-history/42-refine.md), [`plan`](./.egg-state/brc-history/42-plan.md), [`implement`](./.egg-state/brc-history/42-implement.md)._ -Phases are ordered by canonical execution order (`refine` → `plan` → `implement` → `pr`); any non-canonical names sort alphabetically after. The line is omitted entirely when no transcript files exist on disk or the identifier is `None`. See [#1828](https://github.com/jwbron/egg/issues/1828) for why the old inline BRC Consensus Summary was removed. +In slice-aware mode (issue mode with `contract.slices`, #2548 hard switchover), the implement phase is partitioned per slice — the writer produces `{identifier}-implement-slice-.md` (one file per slice) plus `{identifier}-implement-unattributed.md` for cross-cutting messages without canonical slice scope (HEARTBEAT, OVERSEER_ALERT, AGENT_FAILED, …). The aggregate `{identifier}-implement.md` file is **not** produced in slice mode. The link line clusters the per-slice files at the canonical `implement` rank in natural-sort order, with the unattributed sibling rendered last: + +> _Per-phase BRC transcripts: [`refine`](./.egg-state/brc-history/42-refine.md), [`plan`](./.egg-state/brc-history/42-plan.md), [`implement-slice-1`](./.egg-state/brc-history/42-implement-slice-1.md), [`implement-slice-2`](./.egg-state/brc-history/42-implement-slice-2.md), [`implement-unattributed`](./.egg-state/brc-history/42-implement-unattributed.md)._ + +Babysit_pr and other non-slice implement runs continue to emit the aggregate `{identifier}-implement.md` file. Phases are ordered by canonical execution order (`refine` → `plan` → `implement` → `pr`); any non-canonical names sort alphabetically after. The line is omitted entirely when no transcript files exist on disk or the identifier is `None`. See [#1828](https://github.com/jwbron/egg/issues/1828) for why the old inline BRC Consensus Summary was removed and [#2548](https://github.com/jwbron/egg/issues/2548) for the per-slice partition. ### Consensus Check diff --git a/docs/guides/sdlc-pipeline.md b/docs/guides/sdlc-pipeline.md index b6c8252f85..3a0da38f3e 100644 --- a/docs/guides/sdlc-pipeline.md +++ b/docs/guides/sdlc-pipeline.md @@ -444,7 +444,7 @@ The local orchestrator handles concurrent contract updates through `orchestrator ```json { - "schemaVersion": "1.0", + "schemaVersion": "1.1", "issue": { "number": 123, "title": "Add feature X", @@ -483,6 +483,13 @@ The local orchestrator handles concurrent contract updates through `orchestrator } ``` +> **Schema 1.1 (#2548)**: The default `schemaVersion` is now `"1.1"`, which +> additively introduces four optional `pr.context_*` fields +> (`context_title`, `context_description`, `context_branch`, +> `context_pr_number`). Pre-1.1 contract JSON loads cleanly — a Pydantic +> `model_validator` silently promotes `"1.0"` to `"1.1"` on load and the +> bumped value is persisted on the next save. + ### Role-Based Field Ownership The `shared/egg_contracts/roles.py` module defines field ownership: diff --git a/docs/templates/plan.md b/docs/templates/plan.md index c7f31298ea..a1435f3b35 100644 --- a/docs/templates/plan.md +++ b/docs/templates/plan.md @@ -74,6 +74,14 @@ pr: manual_steps: | Pre-merge: [any required steps before merging, e.g. migrations, config changes] Post-merge: [any required steps after merging, e.g. deployments] + # Optional context-PR framing (#2548); omit to reuse pr.title / pr.description. + # context_title: |- + # Strategic plan for # — refine/plan analysis + BRC history + # context_description: |- + # Carries the refine analysis, the plan, the BRC consensus + # history that approved each, and the agent transcripts — + # so reviewers approaching the slice stack can see the strategic + # narrative on a PR that targets the configured base branch. phases: - id: 1 name: |- @@ -125,6 +133,23 @@ phases: > the task's files — see [Agent Roles Reference](../reference/agent-roles.md#role-aware-task-assignment) > for the file-to-role mapping. Tasks without a `role` default to the coder. +> **Context-PR framing (#2548)**: `pr.context_title` and `pr.context_description` +> are *optional* keys planners may emit to give the dedicated context PR a +> different framing from the slice PRs (e.g. "Strategic plan for #N" vs the +> slice's "Implement …"). When omitted the orchestrator falls back to +> `pr.title` / `pr.description`. Two sibling fields — `pr.context_branch` and +> `pr.context_pr_number` — exist on the contract but are populated by the +> orchestrator after the context branch is created and the context PR is +> opened; planners must NOT emit them. +> +> **As of slice-1 (#2548 part 1)**, only the schema fields and this +> planner-prompt guidance are wired. The orchestrator branch-creation +> and PR-opening hooks land in #2548 slices 3-4 — until those slices +> merge, any `context_title` / `context_description` a planner emits +> flows through the parser into `PRMetadata` but nothing acts on it +> yet, so emitting them now is forward-compatibly safe but does not +> change the rendered PR. + > **Slices vs. phases (#2137)**: The plan parser accepts either `slices:` > (canonical, post-#2137) or `phases:` (legacy alias) at the top of the > `# yaml-tasks` block. New plans should emit `slices:` so they ingest as diff --git a/orchestrator/routes/messages.py b/orchestrator/routes/messages.py index 56e9099a76..b1abf28c98 100644 --- a/orchestrator/routes/messages.py +++ b/orchestrator/routes/messages.py @@ -666,11 +666,17 @@ def post_heartbeat(pipeline_id: str) -> tuple[Response, int]: # Emit as a normal HEARTBEAT message on the bus so downstream # consumers (HealthMonitor, overseer, UI) see it. - metadata = {"state": state} + metadata: dict[str, Any] = {"state": state} if waiting_on: metadata["waiting_on"] = waiting_on if body.get("since"): metadata["since"] = body["since"] + # Tag with slice_id so the implement-phase BRC writer can partition + # this HEARTBEAT into the correct per-slice transcript (#2548). + # Pipeline-level (non-slice) heartbeats leave the metadata off + # entirely. + if slice_id: + metadata["slice_id"] = slice_id msg = Message( pipeline_id=pipeline_id, diff --git a/orchestrator/routes/pipelines.py b/orchestrator/routes/pipelines.py index d5be4613a4..054ff4f26d 100644 --- a/orchestrator/routes/pipelines.py +++ b/orchestrator/routes/pipelines.py @@ -8094,6 +8094,26 @@ def _finalize_pr_phase_failed( } ) +# Subset of BRC_HISTORY_TYPES that the orchestrator's CONSENSUS_* signal +# handlers tag with ``metadata['slice_id']`` for slice-aware implement +# pipelines (#2548). The implement-phase BRC writer treats a missing +# ``slice_id`` on these as a contract violation (drop with WARNING), +# while the remaining BRC_HISTORY_TYPES (HEARTBEAT, STATUS, HANDOFF, +# AGENT_FAILED, NUDGE, OVERSEER_ALERT) come from emitters that do not +# uniformly carry slice scope — those are routed to the unattributed +# sibling file rather than dropped, so the audit trail stays complete. +CONSENSUS_BRC_TYPES = frozenset( + { + "CONSENSUS_PROPOSE", + "CONSENSUS_ACK", + "CONSENSUS_NACK", + "CONSENSUS_WITHDRAW", + "CONSENSUS_CONFIRMED", + "CONSENSUS_RE_REVIEW", + "CONSENSUS_OBLIGATION_RESOLVED", + } +) + def _get_message_store(): """Import and return the message store factory function, or None if unavailable.""" @@ -8107,91 +8127,53 @@ def _get_message_store(): return get_message_store -def _write_brc_history( - worktree_path: Path, +def _render_brc_history_markdown( + brc_messages: list[Any], pipeline_id: str, phase: str, - identifier: int | str, -) -> None: - """Write BRC consensus message history for a phase to .egg-state. - - Retrieves BRC-related messages for the given phase from the message store - and writes them as a chronological markdown log to - ``.egg-state/brc-history/{identifier}-{phase}.md``. - No-ops gracefully when the message store is unavailable or contains no - BRC messages for the pipeline and phase. - - Args: - worktree_path: Path to the worktree repo directory - pipeline_id: The pipeline ID to retrieve messages for - phase: The pipeline phase name (e.g. "implement", "plan") - identifier: The pipeline identifier for file naming + *, + slice_id: str | None = None, +) -> str: + """Render *brc_messages* as a chronological markdown log. + + The output shape mirrors the legacy aggregate file: a heading line, + a generated-timestamp footer, and one ``### [ts] role (TYPE): subject`` + section per message with a fenced YAML metadata block. + + ``Generated:`` is derived from the *latest* message timestamp (not + wall-clock time) so regenerating the file from the same message set + produces byte-identical output. This keeps the PR-phase safety-net + rewrite (:func:`_rewrite_brc_history_for_pr`) idempotent: when no new + BRC messages arrived between phase completion and PR creation, the + rewritten file matches the previous commit and the follow-up commit is + skipped by :func:`_commit_statefiles_to_worktree`. See #1714. """ - logger.info( - "_write_brc_history: entering", - pipeline_id=pipeline_id, - phase=phase, - identifier=str(identifier), - ) - - store_fn = _get_message_store() - if store_fn is None: - logger.info( - "_write_brc_history: early return — message store unavailable", - pipeline_id=pipeline_id, - phase=phase, - ) - return - - try: - store = store_fn() - messages = store.get_messages(pipeline_id, limit=10000) - except Exception as e: - logger.warning( - "_write_brc_history: early return — failed to retrieve messages", - pipeline_id=pipeline_id, - phase=phase, - error=str(e), - ) - return - - if not messages: - logger.info( - "_write_brc_history: early return — no messages in store", - pipeline_id=pipeline_id, - phase=phase, - ) - return - - brc_messages = [m for m in messages if m.message_type in BRC_HISTORY_TYPES and m.phase == phase] - if not brc_messages: - logger.info( - "_write_brc_history: early return — no BRC messages for phase", - pipeline_id=pipeline_id, - phase=phase, - total_messages=len(messages), - ) - return - - # Format as markdown. `Generated:` is derived from the latest - # message timestamp (not wall-clock time) so regenerating the - # file from the same message set produces byte-identical output. - # This keeps the PR-phase safety-net rewrite - # (_rewrite_brc_history_for_pr) idempotent: when no new BRC - # messages arrived between phase completion and PR creation, - # the rewritten file matches the previous commit and the - # follow-up commit is skipped by _commit_statefiles_to_worktree. - # See #1714. message_timestamps = [m.timestamp for m in brc_messages if m.timestamp is not None] if message_timestamps: generated_str = max(message_timestamps).strftime("%Y-%m-%dT%H:%M:%SZ") else: generated_str = "unknown" + # The "unattributed" bucket is not a slice — it holds cross-cutting + # non-CONSENSUS messages that lack canonical slice scope (HEARTBEAT, + # OVERSEER_ALERT, AGENT_FAILED, …) routed to a sibling file so the + # audit trail stays complete. Rendering it as "Slice: unattributed" + # would mislead a reviewer who lands on the file via a link line — + # special-case the heading and metadata block instead. + is_unattributed = slice_id == "unattributed" lines: list[str] = [] - lines.append(f"# BRC Consensus History — {phase} phase") + if is_unattributed: + lines.append(f"# BRC Consensus History — {phase} phase, cross-cutting (unattributed)") + elif slice_id: + lines.append(f"# BRC Consensus History — {phase} phase, {slice_id}") + else: + lines.append(f"# BRC Consensus History — {phase} phase") lines.append("") lines.append(f"Generated: {generated_str}") lines.append(f"Pipeline: {pipeline_id}") + if is_unattributed: + lines.append("Section: cross-cutting (unattributed)") + elif slice_id: + lines.append(f"Slice: {slice_id}") lines.append("") for msg in brc_messages: @@ -8224,24 +8206,56 @@ def _write_brc_history( ) lines.append("````") lines.append("") + return "\n".join(lines) + + +def _write_brc_history_file( + worktree_path: Path, + pipeline_id: str, + phase: str, + identifier: int | str, + brc_messages: list[Any], + *, + slice_id: str | None = None, +) -> None: + """Render and persist the markdown + JSON companion files for one bucket. + + ``slice_id``, when provided, switches the on-disk filename from the + aggregate ``{identifier}-{phase}.{md,json}`` shape used by + refine/plan/pr to the per-slice ``{identifier}-{phase}-{slice_id}.{md,json}`` + shape used by implement (#2548 — hard switchover, no aggregate + implement file is produced). + """ + if slice_id: + stem = f"{identifier}-{phase}-{slice_id}" + else: + stem = f"{identifier}-{phase}" history_dir = worktree_path / ".egg-state" / "brc-history" history_dir.mkdir(parents=True, exist_ok=True) - history_file = history_dir / f"{identifier}-{phase}.md" + history_file = history_dir / f"{stem}.md" # Write the markdown history file try: - history_file.write_text("\n".join(lines)) + history_file.write_text( + _render_brc_history_markdown( + brc_messages, + pipeline_id, + phase, + slice_id=slice_id, + ) + ) except Exception as md_err: logger.warning( "Failed to write BRC history markdown file", pipeline_id=pipeline_id, phase=phase, + slice_id=slice_id, error=str(md_err), ) # Write a JSON companion artifact containing the full message dicts - json_file = history_dir / f"{identifier}-{phase}.json" + json_file = history_dir / f"{stem}.json" try: json_data = [msg.to_dict() for msg in brc_messages] json_file.write_text(json.dumps(json_data, indent=2, default=str)) @@ -8250,6 +8264,7 @@ def _write_brc_history( "Failed to write BRC history JSON companion file", pipeline_id=pipeline_id, phase=phase, + slice_id=slice_id, error=str(json_err), ) @@ -8257,11 +8272,247 @@ def _write_brc_history( "Wrote BRC history file", pipeline_id=pipeline_id, phase=phase, + slice_id=slice_id, path=str(history_file), message_count=len(brc_messages), ) +def _write_brc_history( + worktree_path: Path, + pipeline_id: str, + phase: str, + identifier: int | str, +) -> None: + """Write BRC consensus message history for a phase to .egg-state. + + Retrieves BRC-related messages for the given phase from the message store + and writes them as a chronological markdown log to + ``.egg-state/brc-history/{identifier}-{phase}.md``. + + For the ``implement`` phase the writer auto-detects slice-aware vs + aggregate mode (#2548): + + * If at least one BRC message carries a canonical + ``metadata['slice_id']`` (validated against + ``SLICE_ID_PATTERN``), the writer partitions messages per-slice + and writes one file per slice as + ``{identifier}-implement-{slice_id}.{md,json}``. + Per-message attribution rules: + + - ``CONSENSUS_*`` messages without a canonical slice_id are + dropped with a single aggregate WARNING — the orchestrator's + CONSENSUS_* signal handlers tag every implement-phase write + under D4, so a missing slice_id is a contract violation. + - Other ``BRC_HISTORY_TYPES`` (HEARTBEAT, STATUS, HANDOFF, + AGENT_FAILED, NUDGE, OVERSEER_ALERT) come from emitters that + do not uniformly carry slice scope. When they lack a + canonical slice_id they are routed to a sibling + ``{identifier}-implement-unattributed.{md,json}`` file rather + than dropped, so the audit trail stays complete and reviewers + of any per-slice transcript can cross-reference. + + * If **no** BRC message carries a slice_id (babysit_pr and other + non-slice pipelines), the writer falls back to the aggregate + ``{identifier}-implement.{md,json}`` filename. This preserves + the documented babysit_pr artifact named in + ``skills/babysit-pr/SKILL.md``. + + No-ops gracefully when the message store is unavailable or contains no + BRC messages for the pipeline and phase. + + Args: + worktree_path: Path to the worktree repo directory + pipeline_id: The pipeline ID to retrieve messages for + phase: The pipeline phase name (e.g. "implement", "plan") + identifier: The pipeline identifier for file naming + """ + logger.info( + "_write_brc_history: entering", + pipeline_id=pipeline_id, + phase=phase, + identifier=str(identifier), + ) + + store_fn = _get_message_store() + if store_fn is None: + logger.info( + "_write_brc_history: early return — message store unavailable", + pipeline_id=pipeline_id, + phase=phase, + ) + return + + try: + store = store_fn() + messages = store.get_messages(pipeline_id, limit=10000) + except Exception as e: + logger.warning( + "_write_brc_history: early return — failed to retrieve messages", + pipeline_id=pipeline_id, + phase=phase, + error=str(e), + ) + return + + if not messages: + logger.info( + "_write_brc_history: early return — no messages in store", + pipeline_id=pipeline_id, + phase=phase, + ) + return + + brc_messages = [m for m in messages if m.message_type in BRC_HISTORY_TYPES and m.phase == phase] + if not brc_messages: + logger.info( + "_write_brc_history: early return — no BRC messages for phase", + pipeline_id=pipeline_id, + phase=phase, + total_messages=len(messages), + ) + return + + if phase == "implement": + # Implement-phase BRC messages are partitioned per-slice (#2548) + # for slice-aware pipelines (issue mode with `contract.slices`): + # the orchestrator's CONSENSUS_* signal handlers tag every + # implement-phase consensus message with `metadata['slice_id']`, + # and this writer buckets them into one transcript file per + # slice. Babysit_pr and other non-slice pipelines have no slice + # scope on any message, so they fall back to the aggregate + # `{identifier}-implement.{md,json}` filename (preserving the + # documented babysit_pr artifact named in + # `skills/babysit-pr/SKILL.md`). + # + # ``metadata['slice_id']`` is interpolated into the on-disk + # filename below, so this is a gateway-facing seam in the same + # sense as ``signals.py`` / the restart route / + # ``concurrent_executor`` branch builders: every value MUST be + # validated against the canonical ``SLICE_ID_PATTERN`` before + # use, otherwise an attacker-controlled metadata blob (any role + # can post arbitrary metadata via ``messages.py``) could smuggle + # path separators into the filename and write outside + # ``.egg-state/brc-history/``. See ``slice_id_validation.py`` + # for the invariant. ``SLICE_ID_PATTERN`` is already imported at + # module top (the same try/except sandbox-vs-orchestrator dual + # import that imports ``extract_slice_id``); no local re-import + # is needed. + + buckets: dict[str, list[Any]] = {} + # ``unattributed_consensus`` holds CONSENSUS_* messages that lack + # a canonical slice_id — those are a D4 contract violation and + # are dropped with a single aggregate WARNING. ``unattributed_other`` + # holds non-CONSENSUS BRC types (HEARTBEAT, STATUS, HANDOFF, + # AGENT_FAILED, NUDGE, OVERSEER_ALERT) whose emitters do not + # uniformly carry slice scope; those are written to the + # ``unattributed`` sibling file so the audit trail stays complete. + unattributed_consensus: list[Any] = [] + unattributed_other: list[Any] = [] + for msg in brc_messages: + # ``Message.metadata`` is a Pydantic dict[str, Any] field with a + # default_factory=dict (message_store.Message), so it is always a + # dict at this point — no need to guard with getattr/isinstance. + raw_slice_id = msg.metadata.get("slice_id") + if isinstance(raw_slice_id, str) and SLICE_ID_PATTERN.fullmatch(raw_slice_id): + buckets.setdefault(raw_slice_id, []).append(msg) + continue + if str(getattr(msg, "message_type", "")) in CONSENSUS_BRC_TYPES: + unattributed_consensus.append(msg) + else: + unattributed_other.append(msg) + + if not buckets: + # No slice-attributed messages anywhere — this is a non-slice + # pipeline (babysit_pr or any other implement-phase run that + # never spawned slice scopes). Fall back to the aggregate + # `{identifier}-implement.{md,json}` filename so we never + # silently drop the entire BRC stream when no per-slice + # bucketing is possible. See #2548 reviewer_code_holistic + # finding #3. + logger.info( + "_write_brc_history: no slice-attributed implement-phase " + "messages — writing aggregate file (non-slice pipeline)", + pipeline_id=pipeline_id, + phase=phase, + total_brc_messages=len(brc_messages), + ) + _write_brc_history_file( + worktree_path, + pipeline_id, + phase, + identifier, + brc_messages, + ) + return + + # Slice-aware pipeline: at least one canonical slice_id was + # observed. CONSENSUS_* messages that lack a canonical slice_id + # are a D4 hard-switchover contract violation — drop them with + # a loud aggregate WARNING (count + sample types) so an operator + # notices the asymmetry rather than silently shipping a thinned- + # out transcript. + if unattributed_consensus: + sample_types = sorted( + {str(getattr(m, "message_type", "")) for m in unattributed_consensus[:8]} + ) + logger.warning( + "_write_brc_history: dropped implement-phase CONSENSUS_* messages " + "without canonical metadata.slice_id (hard switchover, #2548)", + pipeline_id=pipeline_id, + phase=phase, + dropped_count=len(unattributed_consensus), + sample_message_types=sample_types, + attributed_count=sum(len(v) for v in buckets.values()), + ) + + # Non-CONSENSUS BRC types without a canonical slice_id come from + # emitters that do not uniformly attach slice scope (HealthMonitor + # nudges, overseer respawn alerts, AGENT_FAILED broadcasts, + # CLI-routed HANDOFF/NUDGE messages, etc.). Route them to a + # sibling ``{identifier}-implement-unattributed.{md,json}`` file + # so the audit trail stays complete — reviewers reading any + # per-slice transcript can cross-reference. See #2548 + # reviewer_code blocking finding. + if unattributed_other: + _write_brc_history_file( + worktree_path, + pipeline_id, + phase, + identifier, + unattributed_other, + slice_id="unattributed", + ) + + # Natural sort by the integer suffix so a 12-slice pipeline iterates + # `slice-1, slice-2, ..., slice-12` rather than the lexicographic + # `slice-1, slice-10, slice-11, slice-12, slice-2`. Every key is + # already SLICE_ID_PATTERN-validated (`^slice-[0-9]+$`) above, so the + # int() parse is total. + for slice_id, slice_msgs in sorted( + buckets.items(), key=lambda kv: int(kv[0].rsplit("-", 1)[1]) + ): + _write_brc_history_file( + worktree_path, + pipeline_id, + phase, + identifier, + slice_msgs, + slice_id=slice_id, + ) + return + + # Refine, plan, and pr phases continue to write the aggregate + # `{identifier}-{phase}.{md,json}` file — only implement is per-slice. + _write_brc_history_file( + worktree_path, + pipeline_id, + phase, + identifier, + brc_messages, + ) + + def _rewrite_brc_history_for_pr( worktree_path: Path, pipeline_id: str, @@ -8540,7 +8791,34 @@ def _build_brc_history_link_line( canonical = [p.value for p in PipelinePhase] rank = {name: i for i, name in enumerate(canonical)} - phases.sort(key=lambda name: (rank.get(name, len(canonical)), name)) + + # Per-slice implement files (#2548) carry the stem + # ``implement-slice-{N}``; cluster them at the canonical ``implement`` + # rank so the rendered link order is + # ``refine → plan → implement[-slice-N] → implement-unattributed → + # pr`` instead of pushing the per-slice files past pr to the end of + # the list. Within the implement cluster, sort by the integer slice + # index so a 12-slice pipeline renders ``slice-1, slice-2, …, + # slice-12`` rather than the lexicographic ``slice-1, slice-10, + # slice-11, slice-12, slice-2``. The ``implement-unattributed`` + # sibling (cross-cutting non-CONSENSUS BRC types without slice scope, + # see ``_write_brc_history``) sorts after every per-slice file so a + # reviewer reads each slice transcript first, then the cross-cutting + # context. + def _sort_key(name: str) -> tuple[int, int, str]: + if name == "implement": + return (rank["implement"], -1, "") + if name == "implement-unattributed": + return (rank["implement"], 1 << 30, name) + if name.startswith("implement-slice-"): + try: + idx = int(name.rsplit("-", 1)[1]) + except ValueError: + idx = 1 << 30 # malformed → sort last within cluster + return (rank["implement"], idx, name) + return (rank.get(name, len(canonical)), 0, name) + + phases.sort(key=_sort_key) links = ", ".join( f"[`{phase}`](./.egg-state/brc-history/{identifier}-{phase}.md)" for phase in phases @@ -8990,6 +9268,42 @@ def _auto_create_pr( " components as a result.", ] +# Shared context-PR framing guidance injected into planner prompts (#2548). +# The planner may optionally emit ``pr.context_title`` / ``pr.context_description`` +# to give the dedicated context PR a different framing from the slice PRs; +# falls back to ``pr.title`` / ``pr.description`` when omitted. The +# orchestrator-populated fields ``pr.context_branch`` and +# ``pr.context_pr_number`` are intentionally excluded — those are runtime +# values written by the orchestrator after the context branch is created +# and the context PR is opened, and the planner must NOT emit them. +_PR_CONTEXT_GUIDANCE = [ + "**Optional context-PR framing (#2548)**: the orchestrator opens a " + "dedicated *context PR* at the root of the slice stack carrying the " + "refine/plan analysis docs and BRC consensus history. You MAY emit " + "`pr.context_title` and `pr.context_description` to frame this " + 'context PR differently from the slice PRs (e.g. "Strategic plan ' + 'for #N" vs the slice\'s "Implement …"). Both keys are optional — ' + "omit them and the orchestrator falls back to `pr.title` / " + "`pr.description`. Do NOT emit `pr.context_branch` or " + "`pr.context_pr_number`: those are populated by the orchestrator " + "after the context branch is created and the PR is opened.", +] + +# Example YAML lines documenting the optional context-PR keys. Indented to +# match the surrounding ``pr:`` block (`` context_title:`` lines up with +# `` description:``). Both lines are commented-out hints because they are +# optional — emitting them is encouraged when the framing should differ. +_PR_CONTEXT_YAML_EXAMPLE_LINES = [ + " # Optional context-PR framing (#2548); omit to reuse pr.title / pr.description.", + " # context_title: |-", + " # Strategic plan for # — refine/plan analysis + BRC history", + " # context_description: |-", + " # Carries the refine analysis, the plan, the BRC consensus", + " # history that approved each, and the agent transcripts —", + " # so reviewers approaching the slice stack can see the strategic", + " # narrative on a PR that targets the configured base branch.", +] + # YAML safety guidance for planner prompts. Plain (unquoted) scalars break # when they contain ``: `` sequences — e.g. "Add `sequence: int = 0` field" # parses as a nested mapping and raises ScannerError. Block scalars (``|-``) @@ -9266,6 +9580,8 @@ def _build_phase_prompt( "", *_PR_DESCRIPTION_GUIDANCE, "", + *_PR_CONTEXT_GUIDANCE, + "", "End your document with a fenced YAML block like this:", "", "````", @@ -9281,6 +9597,7 @@ def _build_phase_prompt( " manual_steps: |", " Pre-merge: any required steps before merging", " Post-merge: any required steps after merging", + *_PR_CONTEXT_YAML_EXAMPLE_LINES, "phases:", " - id: 1", " name: |-", @@ -11071,6 +11388,8 @@ def _build_agent_prompt( "", *_PR_DESCRIPTION_GUIDANCE, "", + *_PR_CONTEXT_GUIDANCE, + "", "End your document with a fenced YAML block like this:", "", "````", @@ -11086,6 +11405,7 @@ def _build_agent_prompt( " manual_steps: |", " Pre-merge: any required steps before merging", " Post-merge: any required steps after merging", + *_PR_CONTEXT_YAML_EXAMPLE_LINES, "phases:", " - id: 1", " name: |-", @@ -14876,11 +15196,38 @@ def _populate_contract_from_plan( if result.pr_title: from egg_contracts.models import PRMetadata + # #2548 — preserve orchestrator-populated runtime fields on + # ``PRMetadata`` across re-populates. The planner-emitted + # ``context_title`` / ``context_description`` still flow in + # fresh from the parsed plan; the fields below are populated + # by orchestrator code paths (gateway primitives, the + # conditional-ACK gate at ``complete_phase``) and would + # otherwise be silently dropped when this safety-net + # populator re-runs (e.g. on a ``start_phase=implement`` + # re-entry where ``deferred_actions`` was already populated + # during implement-phase close). + # + # ``deferred_actions`` is the merge-blocking *Pre-merge + # Obligations* handoff written by ``decisions.py`` after a + # conditional-ACK gate resolves; losing it here erases the + # reviewer's only durable handoff for git-mv / migration / + # cross-repo flips. See test + # ``test_populate_contract_from_plan_preserves_deferred_actions``. + preserved_branch = contract.pr.context_branch if contract.pr is not None else None + preserved_pr_number = contract.pr.context_pr_number if contract.pr is not None else None + preserved_deferred_actions = ( + list(contract.pr.deferred_actions) if contract.pr is not None else [] + ) contract.pr = PRMetadata( title=result.pr_title, description=result.pr_description or "", test_plan=result.pr_test_plan or "", manual_steps=result.pr_manual_steps or "", + context_title=result.pr_context_title, + context_description=result.pr_context_description, + context_branch=preserved_branch, + context_pr_number=preserved_pr_number, + deferred_actions=preserved_deferred_actions, ) changed = True diff --git a/orchestrator/routes/signals.py b/orchestrator/routes/signals.py index 13ef92213f..56c1e3a861 100644 --- a/orchestrator/routes/signals.py +++ b/orchestrator/routes/signals.py @@ -938,6 +938,7 @@ def _emit_ready_to_confirm_nudges( phase: str, newly_ready: list[dict[str, Any]], tracker: Any = None, + slice_id: str | None = None, ) -> None: """Emit a STATUS to each producer that newly became ready to confirm. @@ -951,12 +952,18 @@ def _emit_ready_to_confirm_nudges( supplied, the per-version memo entry is rolled back so the producer can be re-nudged on the next state change. Other producers in the batch are still attempted. + + ``slice_id`` is forwarded into the STATUS metadata so the + implement-phase BRC writer (#2548) routes the nudge into the + producer's per-slice transcript. Pipeline-level (non-slice) callers + leave it as ``None``. """ if not newly_ready: return from message_store import Message, MessageType, get_message_store store = get_message_store() + _slice_meta: dict[str, Any] = {"slice_id": slice_id} if slice_id is not None else {} for entry in newly_ready: producer = entry["role"] version = entry["version"] @@ -975,7 +982,7 @@ def _emit_ready_to_confirm_nudges( f"`egg-orch consensus confirmed` to confirm." ), phase=phase, - metadata={"ready_to_confirm": True, "version": version}, + metadata={"ready_to_confirm": True, "version": version, **_slice_meta}, ) ) except Exception as exc: @@ -1128,7 +1135,15 @@ def handle_consensus_propose_signal( details=result, ) - # Write consensus message to message bus + # Write consensus message to message bus. + # Tag every CONSENSUS_* message with slice_id metadata when the + # producer is slice-scoped so the implement-phase BRC writer + # (#2548) can partition messages into per-slice transcript + # files. Pipeline-level (non-slice) callers leave the metadata + # off entirely — matches the legacy non-slice shape and signals + # the writer to fall back to its aggregate filename. + _slice_meta: dict[str, Any] = {"slice_id": slice_id} if slice_id is not None else {} + from message_store import Message, MessageType, get_message_store store = get_message_store() @@ -1146,6 +1161,7 @@ def handle_consensus_propose_signal( "payload": payload, "version": result.get("version"), "commit_sha": commit_sha, + **_slice_meta, }, ) ) @@ -1171,13 +1187,16 @@ def handle_consensus_propose_signal( metadata={ "producer_role": agent_role, "version": result.get("version"), + **_slice_meta, }, ) ) # A new proposal can unblock the global zero-proposal guard for # producers that were previously fully ACKed but unable to confirm. - _emit_ready_to_confirm_nudges(pipeline_id, phase, result.get("newly_ready", []), tracker) + _emit_ready_to_confirm_nudges( + pipeline_id, phase, result.get("newly_ready", []), tracker, slice_id=slice_id + ) return make_success_response( f"Proposal recorded for {agent_role}", @@ -1270,6 +1289,10 @@ def handle_consensus_ack_signal( store = get_message_store() phase = _resolve_pipeline_phase(pipeline_id, repo_path) + # Tag with slice_id metadata for the implement-phase BRC writer's + # per-slice partitioning (#2548). Pipeline-level callers leave it + # off, matching the legacy non-slice shape. + _slice_meta: dict[str, Any] = {"slice_id": slice_id} if slice_id is not None else {} store.add_message( Message( pipeline_id=pipeline_id, @@ -1279,7 +1302,11 @@ def handle_consensus_ack_signal( subject=f"ACK from {reviewer_role} for {producer_role}", body=payload.get("reason", ""), phase=phase, - metadata={"payload": payload, "version": result.get("version")}, + metadata={ + "payload": payload, + "version": result.get("version"), + **_slice_meta, + }, ) ) @@ -1288,7 +1315,9 @@ def handle_consensus_ack_signal( # critical-reviewer ACK predicate. Replaces the prior ``fully_acked`` # gate which fired before global guards (e.g. zero-proposal) cleared # and could mislead an advisory-only producer like documenter (#2078). - _emit_ready_to_confirm_nudges(pipeline_id, phase, result.get("newly_ready", []), tracker) + _emit_ready_to_confirm_nudges( + pipeline_id, phase, result.get("newly_ready", []), tracker, slice_id=slice_id + ) return make_success_response( f"ACK recorded: {reviewer_role} -> {producer_role}", @@ -1359,6 +1388,9 @@ def handle_consensus_nack_signal( from message_store import Message, MessageType, get_message_store store = get_message_store() + # Tag with slice_id metadata for the implement-phase BRC writer's + # per-slice partitioning (#2548). + _slice_meta: dict[str, Any] = {"slice_id": slice_id} if slice_id is not None else {} store.add_message( Message( pipeline_id=pipeline_id, @@ -1372,6 +1404,7 @@ def handle_consensus_nack_signal( "payload": payload, "reason": result.get("reason"), "revision_count": result.get("revision_count"), + **_slice_meta, }, ) ) @@ -1423,6 +1456,9 @@ def handle_consensus_withdraw_signal( from message_store import Message, MessageType, get_message_store store = get_message_store() + # Tag with slice_id metadata for the implement-phase BRC writer's + # per-slice partitioning (#2548). + _slice_meta: dict[str, Any] = {"slice_id": slice_id} if slice_id is not None else {} store.add_message( Message( pipeline_id=pipeline_id, @@ -1432,6 +1468,7 @@ def handle_consensus_withdraw_signal( subject=f"Withdrawal by {agent_role}", body=reason, phase=_resolve_pipeline_phase(pipeline_id, repo_path), + metadata=_slice_meta, ) ) @@ -1859,6 +1896,11 @@ def handle_consensus_excuse_producer_signal( store = get_message_store() phase = _resolve_pipeline_phase(pipeline_id, repo_path) + # Tag with slice_id so the implement-phase BRC writer can + # partition this STATUS into the correct per-slice transcript + # (#2548). Pipeline-level (non-slice) callers leave the metadata + # off entirely. + _slice_meta: dict[str, Any] = {"slice_id": slice_id} if slice_id is not None else {} # Notify all agents that the producer has been excused store.add_message( @@ -1879,6 +1921,7 @@ def handle_consensus_excuse_producer_signal( "producer_role": producer_role, "reason": reason, "affected_reviewers": result.get("affected_reviewers", []), + **_slice_meta, }, ) ) @@ -1977,6 +2020,12 @@ def handle_consensus_resolve_obligation_signal( store = get_message_store() phase = _resolve_pipeline_phase(pipeline_id, repo_path) + # Tag with slice_id metadata for the implement-phase BRC writer's + # per-slice partitioning (#2548). CONSENSUS_OBLIGATION_RESOLVED is + # in BRC_HISTORY_TYPES and can fire during the implement phase + # with slice scope (typical case: tester satisfies a coder's + # conditional ACK on a per-slice review). + _slice_meta: dict[str, Any] = {"slice_id": slice_id} if slice_id is not None else {} store.add_message( Message( pipeline_id=pipeline_id, @@ -1996,6 +2045,7 @@ def handle_consensus_resolve_obligation_signal( "note": note, "version": result.get("version"), "condition": result.get("condition", ""), + **_slice_meta, }, ) ) @@ -2069,6 +2119,10 @@ def handle_consensus_producer_push_signal( store = get_message_store() phase = _resolve_pipeline_phase(pipeline_id, repo_path) + # Tag with slice_id metadata for the implement-phase BRC + # writer's per-slice partitioning (#2548). Same shape as the + # manual re-propose path in handle_consensus_propose_signal. + _slice_meta: dict[str, Any] = {"slice_id": slice_id} if slice_id is not None else {} store.add_message( Message( pipeline_id=pipeline_id, @@ -2087,6 +2141,7 @@ def handle_consensus_producer_push_signal( "commit_sha": commit_sha, "version": result.get("version"), "changed_files": changed_files, + **_slice_meta, }, ) ) @@ -2117,6 +2172,7 @@ def handle_consensus_producer_push_signal( "producer_role": agent_role, "version": result.get("version"), "commit_sha": commit_sha, + **_slice_meta, }, ) ) @@ -2129,7 +2185,7 @@ def handle_consensus_producer_push_signal( # ACKs were just invalidated), but skipping the call would # silently regress if a future guard depends on peer versions. _emit_ready_to_confirm_nudges( - pipeline_id, phase, result.get("newly_ready", []), tracker + pipeline_id, phase, result.get("newly_ready", []), tracker, slice_id=slice_id ) return make_success_response( diff --git a/orchestrator/tests/test_brc_history.py b/orchestrator/tests/test_brc_history.py index f43d1a5c38..b63e8b8256 100644 --- a/orchestrator/tests/test_brc_history.py +++ b/orchestrator/tests/test_brc_history.py @@ -63,6 +63,15 @@ def _make_contract_json( return contract +# Default slice_id stamped onto implement-phase BRC messages by the test +# helpers below. Issue #2548 hard-switchover: ``_write_brc_history`` drops +# implement-phase BRC messages without a ``metadata['slice_id']`` with a +# warning, so every fixture in this module must seed one. Tests that want +# to exercise the missing-slice_id WARNING path explicitly pass +# ``slice_id=None`` (or omit ``slice_id`` from the override metadata). +_DEFAULT_IMPLEMENT_SLICE_ID = "slice-1" + + def _make_brc_message( pipeline_id="issue-42", from_role="coder", @@ -72,8 +81,25 @@ def _make_brc_message( phase="implement", timestamp=None, metadata=None, + slice_id="__default__", ): - """Create a BRC Message for testing.""" + """Create a BRC Message for testing. + + ``slice_id`` is merged into ``metadata`` for implement-phase messages so + tests can rely on the post-#2548 hard-switchover writer producing per-slice + files. Pass ``slice_id=None`` to omit it (used by the missing-slice_id + WARNING regression test). When ``metadata`` already contains a + ``slice_id`` key it wins (caller intent). + """ + md = dict(metadata or {}) + if slice_id == "__default__": + # Default policy: implement phase auto-stamps slice-1 unless metadata + # already supplies one; non-implement phases never auto-stamp. + if phase == "implement" and "slice_id" not in md: + md["slice_id"] = _DEFAULT_IMPLEMENT_SLICE_ID + elif slice_id is not None: + md.setdefault("slice_id", slice_id) + # slice_id=None and metadata lacks "slice_id" -> intentionally unattributed return Message( pipeline_id=pipeline_id, from_role=from_role, @@ -83,11 +109,11 @@ def _make_brc_message( body=body, phase=phase, timestamp=timestamp or datetime(2026, 4, 8, 12, 0, 0, tzinfo=UTC), - metadata=metadata or {}, + metadata=md, ) -def _make_brc_messages(pipeline_id="issue-42", phase="implement"): +def _make_brc_messages(pipeline_id="issue-42", phase="implement", slice_id="__default__"): """Create a typical set of BRC messages for a phase.""" return [ _make_brc_message( @@ -98,6 +124,7 @@ def _make_brc_messages(pipeline_id="issue-42", phase="implement"): body="Implemented auth fix", phase=phase, timestamp=datetime(2026, 4, 8, 12, 0, 0, tzinfo=UTC), + slice_id=slice_id, ), _make_brc_message( pipeline_id=pipeline_id, @@ -107,6 +134,7 @@ def _make_brc_messages(pipeline_id="issue-42", phase="implement"): body="Code looks good", phase=phase, timestamp=datetime(2026, 4, 8, 12, 5, 0, tzinfo=UTC), + slice_id=slice_id, ), _make_brc_message( pipeline_id=pipeline_id, @@ -116,6 +144,7 @@ def _make_brc_messages(pipeline_id="issue-42", phase="implement"): body="Tests pass", phase=phase, timestamp=datetime(2026, 4, 8, 12, 10, 0, tzinfo=UTC), + slice_id=slice_id, ), _make_brc_message( pipeline_id=pipeline_id, @@ -125,11 +154,12 @@ def _make_brc_messages(pipeline_id="issue-42", phase="implement"): body="", phase=phase, timestamp=datetime(2026, 4, 8, 12, 15, 0, tzinfo=UTC), + slice_id=slice_id, ), ] -def _make_mixed_messages(pipeline_id="issue-42", phase="implement"): +def _make_mixed_messages(pipeline_id="issue-42", phase="implement", slice_id="__default__"): """Create messages with both BRC and non-BRC types.""" return [ _make_brc_message( @@ -139,6 +169,7 @@ def _make_mixed_messages(pipeline_id="issue-42", phase="implement"): subject="Working on task", body="Starting implementation", phase=phase, + slice_id=slice_id, ), _make_brc_message( pipeline_id=pipeline_id, @@ -147,6 +178,7 @@ def _make_mixed_messages(pipeline_id="issue-42", phase="implement"): subject="Proposal from coder", body="Done with implementation", phase=phase, + slice_id=slice_id, ), _make_brc_message( pipeline_id=pipeline_id, @@ -155,10 +187,16 @@ def _make_mixed_messages(pipeline_id="issue-42", phase="implement"): subject="Test status", body="Running tests", phase=phase, + slice_id=slice_id, ), ] +def _implement_path(tmp_path, identifier="42", suffix=".md", slice_id=_DEFAULT_IMPLEMENT_SLICE_ID): + """Resolve the canonical per-slice implement-phase BRC history path (#2548).""" + return tmp_path / ".egg-state" / "brc-history" / f"{identifier}-implement-{slice_id}{suffix}" + + def _setup_contract(tmp_path, issue_number=42): """Set up a contract JSON file in the temp directory.""" contract_dir = tmp_path / ".egg-state" / "contracts" @@ -181,11 +219,14 @@ def test_creates_file_with_brc_messages(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - expected_path = tmp_path / ".egg-state" / "brc-history" / "42-implement.md" + expected_path = _implement_path(tmp_path) assert expected_path.exists(), f"Expected BRC history file at {expected_path}" content = expected_path.read_text() assert len(content) > 0 + # #2548 hard switchover: aggregate file MUST NOT be produced. + aggregate = tmp_path / ".egg-state" / "brc-history" / "42-implement.md" + assert not aggregate.exists(), "Aggregate implement file leaked through hard switchover" def test_file_contains_chronological_messages(self, tmp_path): """BRC history file contains messages in chronological order.""" @@ -198,7 +239,7 @@ def test_file_contains_chronological_messages(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() # Verify all BRC message types appear assert "CONSENSUS_PROPOSE" in content @@ -266,7 +307,7 @@ def test_filters_only_brc_history_messages(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - expected_path = tmp_path / ".egg-state" / "brc-history" / "42-implement.md" + expected_path = _implement_path(tmp_path) assert expected_path.exists() content = expected_path.read_text() @@ -318,7 +359,7 @@ def test_file_contains_phase_header(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() # File should contain a header with phase info assert "implement" in content.lower() @@ -352,7 +393,7 @@ def test_includes_nack_messages(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() assert "CONSENSUS_NACK" in content assert "reviewer_code" in content @@ -395,7 +436,7 @@ def test_includes_re_review_and_withdraw(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() assert "CONSENSUS_RE_REVIEW" in content assert "CONSENSUS_WITHDRAW" in content @@ -438,7 +479,7 @@ def test_filters_messages_by_phase(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() # Should contain implement-phase messages assert "Implement proposal" in content assert "ACK for implement" in content @@ -499,7 +540,11 @@ def test_multiple_phases_create_separate_files(self, tmp_path): history_dir = tmp_path / ".egg-state" / "brc-history" assert (history_dir / "42-refine.md").exists() assert (history_dir / "42-plan.md").exists() - assert (history_dir / "42-implement.md").exists() + # #2548: implement phase emits a per-slice file, not the aggregate. + assert (history_dir / f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md").exists() + assert not (history_dir / "42-implement.md").exists(), ( + "Aggregate implement.md leaked through hard switchover" + ) def test_messages_with_empty_body(self, tmp_path): """Messages with empty body are included but don't break formatting.""" @@ -521,7 +566,7 @@ def test_messages_with_empty_body(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - expected_path = tmp_path / ".egg-state" / "brc-history" / "42-implement.md" + expected_path = _implement_path(tmp_path) assert expected_path.exists() content = expected_path.read_text() assert "CONSENSUS_CONFIRMED" in content @@ -554,7 +599,7 @@ def test_ack_metadata_round_trips_into_yaml_block(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() assert "````yaml" in content assert "artifact_references" in content assert "orchestrator/routes/pipelines.py" in content @@ -584,7 +629,7 @@ def test_nack_metadata_reason_and_revision_count_round_trip(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() assert "````yaml" in content assert "revision_count" in content assert "payload" in content @@ -611,7 +656,7 @@ def test_propose_commit_sha_in_metadata(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() assert "commit_sha" in content assert "abc123def456" in content @@ -638,7 +683,7 @@ def test_to_role_shown_for_directed_messages(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() assert "→ reviewer_code" in content def test_to_role_omitted_for_broadcast(self, tmp_path): @@ -662,7 +707,7 @@ def test_to_role_omitted_for_broadcast(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() assert "→" not in content def test_triple_backtick_body_does_not_corrupt_yaml_block(self, tmp_path): @@ -693,7 +738,7 @@ def test_triple_backtick_body_does_not_corrupt_yaml_block(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() # The body should appear verbatim assert "```python" in content assert "print('hi')" in content @@ -739,7 +784,7 @@ def test_handoff_included_in_history(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() assert "HANDOFF" in content assert "Code ready for testing" in content @@ -764,7 +809,7 @@ def test_overseer_alert_included_in_history(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() assert "OVERSEER_ALERT" in content @@ -782,7 +827,7 @@ def test_json_file_written_alongside_md(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - json_path = tmp_path / ".egg-state" / "brc-history" / "42-implement.json" + json_path = _implement_path(tmp_path, suffix=".json") assert json_path.exists(), "JSON companion file should exist" def test_json_round_trips_to_message_dicts(self, tmp_path): @@ -796,7 +841,7 @@ def test_json_round_trips_to_message_dicts(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - json_path = tmp_path / ".egg-state" / "brc-history" / "42-implement.json" + json_path = _implement_path(tmp_path, suffix=".json") data = json.loads(json_path.read_text()) assert isinstance(data, list) assert len(data) == len(messages) @@ -878,7 +923,7 @@ def test_json_includes_non_consensus_types(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - json_path = tmp_path / ".egg-state" / "brc-history" / "42-implement.json" + json_path = _implement_path(tmp_path, suffix=".json") data = json.loads(json_path.read_text()) types_in_json = {entry["message_type"] for entry in data} @@ -914,7 +959,7 @@ def test_json_includes_metadata_fields(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - json_path = tmp_path / ".egg-state" / "brc-history" / "42-implement.json" + json_path = _implement_path(tmp_path, suffix=".json") data = json.loads(json_path.read_text()) assert len(data) == 1 metadata = data[0]["metadata"] @@ -946,7 +991,7 @@ def test_json_write_failure_does_not_block_md(self, tmp_path): ): _write_brc_history(tmp_path, "issue-42", "implement", 42) - md_path = tmp_path / ".egg-state" / "brc-history" / "42-implement.md" + md_path = _implement_path(tmp_path) assert md_path.exists(), "Markdown file should still be written despite JSON failure" @@ -1040,7 +1085,7 @@ def test_yaml_block_is_parseable(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() # Extract YAML blocks from fenced code blocks in_yaml = False @@ -1094,7 +1139,7 @@ def test_yaml_block_with_nested_metadata(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() in_yaml = False yaml_lines: list[str] = [] @@ -1142,14 +1187,14 @@ def test_md_write_failure_does_not_block_json(self, tmp_path): history_dir.mkdir(parents=True, exist_ok=True) # Make the .md file a directory so write_text fails - md_path = history_dir / "42-implement.md" + md_path = history_dir / f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md" md_path.mkdir() with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) # JSON should still be written despite MD failure - json_path = history_dir / "42-implement.json" + json_path = history_dir / f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.json" assert json_path.exists(), "JSON file should be written despite markdown write failure" data = json.loads(json_path.read_text()) assert len(data) == 1 @@ -1169,7 +1214,7 @@ def test_no_json_file_when_no_brc_messages(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - json_path = tmp_path / ".egg-state" / "brc-history" / "42-implement.json" + json_path = _implement_path(tmp_path, suffix=".json") assert not json_path.exists(), "No JSON file should be created for empty message store" def test_multiple_phases_create_separate_json_files(self, tmp_path): @@ -1190,11 +1235,19 @@ def test_multiple_phases_create_separate_json_files(self, tmp_path): history_dir = tmp_path / ".egg-state" / "brc-history" assert (history_dir / "42-refine.json").exists() assert (history_dir / "42-plan.json").exists() - assert (history_dir / "42-implement.json").exists() + # #2548: implement is per-slice; aggregate JSON must NOT exist. + assert (history_dir / f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.json").exists() + assert not (history_dir / "42-implement.json").exists(), ( + "Aggregate implement.json leaked through hard switchover" + ) # Each JSON file should only contain messages for that phase - for phase in ["refine", "plan", "implement"]: - data = json.loads((history_dir / f"42-{phase}.json").read_text()) + for phase, fname in [ + ("refine", "42-refine.json"), + ("plan", "42-plan.json"), + ("implement", f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.json"), + ]: + data = json.loads((history_dir / fname).read_text()) for entry in data: assert entry["phase"] == phase, ( f"JSON for {phase} contains message from {entry['phase']}" @@ -1222,7 +1275,7 @@ def test_json_preserves_to_role_for_directed_messages(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - json_path = tmp_path / ".egg-state" / "brc-history" / "42-implement.json" + json_path = _implement_path(tmp_path, suffix=".json") data = json.loads(json_path.read_text()) assert data[0]["to_role"] == "reviewer_code" @@ -1250,7 +1303,7 @@ def test_status_included_in_history(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() assert "STATUS" in content assert "Ready to confirm" in content @@ -1274,7 +1327,7 @@ def test_nudge_included_in_history(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() assert "NUDGE" in content def test_status_replaces_removed_question_type(self, tmp_path): @@ -1305,7 +1358,7 @@ def test_status_replaces_removed_question_type(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() assert "STATUS" in content assert "Should I test the SSO path?" in content @@ -1329,7 +1382,7 @@ def test_agent_failed_included_in_history(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() assert "AGENT_FAILED" in content assert "Container exited with code 1" in content @@ -1353,7 +1406,7 @@ def test_handoff_included_in_history(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + content = _implement_path(tmp_path).read_text() assert "HANDOFF" in content @@ -1368,7 +1421,7 @@ def _touch(self, tmp_path, filename: str) -> None: def test_returns_empty_when_identifier_is_none(self, tmp_path): from routes.pipelines import _build_brc_history_link_line - self._touch(tmp_path, "42-implement.md") + self._touch(tmp_path, f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md") assert _build_brc_history_link_line(tmp_path, None) == "" def test_returns_empty_when_history_dir_missing(self, tmp_path): @@ -1379,52 +1432,67 @@ def test_returns_empty_when_history_dir_missing(self, tmp_path): def test_returns_empty_when_no_matching_files(self, tmp_path): from routes.pipelines import _build_brc_history_link_line - # File for a different pipeline/identifier - self._touch(tmp_path, "99-implement.md") + # File for a different pipeline/identifier (#2548: per-slice form). + self._touch(tmp_path, f"99-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md") assert _build_brc_history_link_line(tmp_path, 42) == "" def test_links_files_in_canonical_phase_order(self, tmp_path): - """Phases link in refine → plan → implement → pr order even if files were created otherwise.""" + """Phases link in refine → plan → implement order even if files were created otherwise. + + After #2548, implement-phase BRC files are per-slice (e.g. + ``42-implement-slice-1.md``); the link-line builder treats the suffix + after the identifier as the phase label, so the slice file appears as + ``implement-slice-1`` rather than ``implement``. The canonical phases + ``refine``/``plan``/``pr`` still sort before any non-canonical name — + which now includes the slice suffix. + """ from routes.pipelines import _build_brc_history_link_line - # Create deliberately out of order - self._touch(tmp_path, "42-implement.md") + # Create deliberately out of order. Implement phase uses the per-slice + # form (#2548 hard switchover) — there is no aggregate ``42-implement.md``. + impl_file = f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md" + self._touch(tmp_path, impl_file) self._touch(tmp_path, "42-plan.md") self._touch(tmp_path, "42-refine.md") result = _build_brc_history_link_line(tmp_path, 42) assert result.startswith("_Per-phase BRC transcripts:") assert result.endswith("._") - # Canonical order: refine before plan before implement + # Canonical order: refine before plan before implement(-slice-N). assert result.index("refine") < result.index("plan") < result.index("implement") - # Link format + # Link format — refine/plan unchanged, implement now includes slice suffix. assert "[`plan`](./.egg-state/brc-history/42-plan.md)" in result assert "[`refine`](./.egg-state/brc-history/42-refine.md)" in result - assert "[`implement`](./.egg-state/brc-history/42-implement.md)" in result + assert ( + f"[`implement-{_DEFAULT_IMPLEMENT_SLICE_ID}`](./.egg-state/brc-history/{impl_file})" + ) in result def test_ignores_json_companions(self, tmp_path): from routes.pipelines import _build_brc_history_link_line - self._touch(tmp_path, "42-implement.md") - self._touch(tmp_path, "42-implement.json") + self._touch(tmp_path, f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md") + self._touch(tmp_path, f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.json") result = _build_brc_history_link_line(tmp_path, 42) # .json not surfaced as its own phase assert ".json" not in result - assert "[`implement`]" in result + assert f"[`implement-{_DEFAULT_IMPLEMENT_SLICE_ID}`]" in result def test_string_identifier_works(self, tmp_path): """Babysit-pr identifiers like 'pr-123-abc1234' glob the corresponding files.""" from routes.pipelines import _build_brc_history_link_line - self._touch(tmp_path, "pr-123-abc1234-implement.md") + impl_file = f"pr-123-abc1234-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md" + self._touch(tmp_path, impl_file) self._touch(tmp_path, "pr-123-abc1234-plan.md") # Unrelated file for a different identifier must not leak in self._touch(tmp_path, "42-refine.md") result = _build_brc_history_link_line(tmp_path, "pr-123-abc1234") assert "[`plan`](./.egg-state/brc-history/pr-123-abc1234-plan.md)" in result - assert "[`implement`](./.egg-state/brc-history/pr-123-abc1234-implement.md)" in result + assert ( + f"[`implement-{_DEFAULT_IMPLEMENT_SLICE_ID}`](./.egg-state/brc-history/{impl_file})" + ) in result assert "42-refine" not in result def test_unknown_phase_names_sorted_after_canonical(self, tmp_path): @@ -1449,13 +1517,17 @@ def test_body_includes_link_line_when_history_files_exist(self, tmp_path): history_dir = tmp_path / ".egg-state" / "brc-history" history_dir.mkdir(parents=True) (history_dir / "42-plan.md").write_text("stub") - (history_dir / "42-implement.md").write_text("stub") + # #2548: implement is per-slice — the aggregate file is gone. + impl_file = f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md" + (history_dir / impl_file).write_text("stub") title, body, _ = _build_pr_body(pipeline, tmp_path) assert "_Per-phase BRC transcripts:" in body assert "[`plan`](./.egg-state/brc-history/42-plan.md)" in body - assert "[`implement`](./.egg-state/brc-history/42-implement.md)" in body + assert ( + f"[`implement-{_DEFAULT_IMPLEMENT_SLICE_ID}`](./.egg-state/brc-history/{impl_file})" + ) in body # The dropped inline summary must not reappear assert "## BRC Consensus Summary" not in body # Existing sections still present @@ -1480,8 +1552,952 @@ def test_link_line_appears_before_authored_by(self, tmp_path): _setup_contract(tmp_path) history_dir = tmp_path / ".egg-state" / "brc-history" history_dir.mkdir(parents=True) - (history_dir / "42-implement.md").write_text("stub") + # #2548: per-slice implement file replaces the aggregate. + (history_dir / f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md").write_text("stub") title, body, _ = _build_pr_body(pipeline, tmp_path) assert body.index("Per-phase BRC transcripts") < body.index("Authored-by: egg") + + +# --------------------------------------------------------------------------- +# Per-slice implement-phase BRC history (#2548 slice-2) +# --------------------------------------------------------------------------- + + +class TestPerSliceImplementBrcHistory: + """Implement-phase ``_write_brc_history`` partitions BRC messages by + ``metadata['slice_id']`` and writes one file per slice (#2548 slice-2, + hard switchover under D4 — no aggregate ``-implement.{md,json}`` is + produced). + + These tests pin the per-slice partitioning contract end-to-end: + multi-slice fan-out, file-naming shape, message routing into the right + bucket, and the no-aggregate-file invariant that the planner explicitly + called out as the slice's most observable acceptance criterion. + """ + + def _make_implement_msgs(self, slice_id, *, body_prefix="work"): + """Build a 4-message PROPOSE/ACK/ACK/CONFIRMED BRC quartet for *slice_id*.""" + return _make_brc_messages(pipeline_id="issue-42", phase="implement", slice_id=slice_id) + + def test_writes_one_file_per_slice_no_aggregate(self, tmp_path): + """Two slices' worth of implement BRC messages produce two per-slice + files and no aggregate ``42-implement.{md,json}``.""" + from routes.pipelines import _write_brc_history + + messages = [] + # Distinct timestamps so the two buckets render in stable order. + for i, sid in enumerate(["slice-1", "slice-2"]): + for j, m in enumerate(self._make_implement_msgs(sid)): + # Disambiguate timestamps so renderer ordering is stable. + m.timestamp = datetime(2026, 4, 8, 12, i * 30 + j, 0, tzinfo=UTC) + messages.append(m) + + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = messages + + with patch("message_store.get_message_store", return_value=mock_store): + _write_brc_history(tmp_path, "issue-42", "implement", 42) + + history_dir = tmp_path / ".egg-state" / "brc-history" + slice1_md = history_dir / "42-implement-slice-1.md" + slice2_md = history_dir / "42-implement-slice-2.md" + slice1_json = history_dir / "42-implement-slice-1.json" + slice2_json = history_dir / "42-implement-slice-2.json" + + # Per-slice files exist, both .md and .json. + assert slice1_md.exists(), "slice-1 markdown file missing" + assert slice2_md.exists(), "slice-2 markdown file missing" + assert slice1_json.exists(), "slice-1 JSON companion missing" + assert slice2_json.exists(), "slice-2 JSON companion missing" + + # Aggregate file MUST NOT exist (hard switchover, D4). + assert not (history_dir / "42-implement.md").exists(), ( + "Aggregate 42-implement.md leaked through hard switchover" + ) + assert not (history_dir / "42-implement.json").exists(), ( + "Aggregate 42-implement.json leaked through hard switchover" + ) + + # And there should be exactly the expected per-slice files plus the + # JSON companions — no other implement-phase artifacts. + produced = sorted(p.name for p in history_dir.glob("42-implement*")) + assert produced == [ + "42-implement-slice-1.json", + "42-implement-slice-1.md", + "42-implement-slice-2.json", + "42-implement-slice-2.md", + ], f"Unexpected files: {produced}" + + def test_each_slice_file_contains_only_its_own_messages(self, tmp_path): + """The slice-1 file must NOT contain any slice-2 messages and vice + versa — partitioning must isolate the buckets.""" + from routes.pipelines import _write_brc_history + + messages = [] + for sid, marker in [("slice-1", "alpha-marker"), ("slice-2", "beta-marker")]: + for m in self._make_implement_msgs(sid): + if m.message_type == MessageType.CONSENSUS_PROPOSE: + m.body = f"{marker} body" + messages.append(m) + + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = messages + + with patch("message_store.get_message_store", return_value=mock_store): + _write_brc_history(tmp_path, "issue-42", "implement", 42) + + history_dir = tmp_path / ".egg-state" / "brc-history" + slice1_md = (history_dir / "42-implement-slice-1.md").read_text() + slice2_md = (history_dir / "42-implement-slice-2.md").read_text() + + # Each marker lands ONLY in its own slice's file. + assert "alpha-marker" in slice1_md + assert "alpha-marker" not in slice2_md, "alpha-marker leaked into slice-2" + assert "beta-marker" in slice2_md + assert "beta-marker" not in slice1_md, "beta-marker leaked into slice-1" + + # And the JSON companions match the same partitioning. + slice1_json = json.loads((history_dir / "42-implement-slice-1.json").read_text()) + slice2_json = json.loads((history_dir / "42-implement-slice-2.json").read_text()) + assert all(entry["metadata"].get("slice_id") == "slice-1" for entry in slice1_json) + assert all(entry["metadata"].get("slice_id") == "slice-2" for entry in slice2_json) + + def test_single_slice_still_uses_per_slice_filename(self, tmp_path): + """Even a single-slice pipeline writes ``-implement-slice-1.{md,json}`` + — there is no fallback to the aggregate filename when only one slice + exists. (Hard switchover — no special-case for N=1.)""" + from routes.pipelines import _write_brc_history + + messages = self._make_implement_msgs("slice-1") + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = messages + + with patch("message_store.get_message_store", return_value=mock_store): + _write_brc_history(tmp_path, "issue-42", "implement", 42) + + history_dir = tmp_path / ".egg-state" / "brc-history" + assert (history_dir / "42-implement-slice-1.md").exists() + assert (history_dir / "42-implement-slice-1.json").exists() + assert not (history_dir / "42-implement.md").exists() + assert not (history_dir / "42-implement.json").exists() + + def test_per_slice_file_carries_slice_label_in_header(self, tmp_path): + """Each per-slice file's ``# BRC Consensus History`` header includes + the slice_id so a reviewer scanning the markdown knows which slice + the consensus belongs to.""" + from routes.pipelines import _write_brc_history + + messages = self._make_implement_msgs("slice-7") + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = messages + + with patch("message_store.get_message_store", return_value=mock_store): + _write_brc_history(tmp_path, "issue-42", "implement", 42) + + content = (tmp_path / ".egg-state" / "brc-history" / "42-implement-slice-7.md").read_text() + assert "# BRC Consensus History" in content + assert "slice-7" in content, "Slice label missing from per-slice file header" + assert "implement" in content.lower() + + def test_messages_without_slice_id_dropped_with_warning(self, tmp_path): + """Implement-phase BRC messages that lack ``metadata['slice_id']`` + are silently dropped from the on-disk history (hard switchover) and + a single aggregate WARNING is emitted naming the dropped count.""" + from routes.pipelines import _write_brc_history + + # Mix attributed and unattributed messages. + attributed = self._make_implement_msgs("slice-1") + unattributed = [ + _make_brc_message( + pipeline_id="issue-42", + from_role="coder", + message_type=MessageType.CONSENSUS_PROPOSE, + subject="Stray PROPOSE", + body="missing slice_id", + phase="implement", + slice_id=None, # <-- intentionally omit metadata.slice_id + ), + _make_brc_message( + pipeline_id="issue-42", + from_role="reviewer_code", + message_type=MessageType.CONSENSUS_NACK, + subject="Stray NACK", + body="missing slice_id", + phase="implement", + slice_id=None, + ), + ] + for m in unattributed: + assert "slice_id" not in m.metadata, "fixture leak — slice_id was stamped" + + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = attributed + unattributed + + with ( + patch("message_store.get_message_store", return_value=mock_store), + patch("routes.pipelines.logger") as mock_logger, + ): + _write_brc_history(tmp_path, "issue-42", "implement", 42) + + # The slice-1 file exists and contains ONLY the attributed PROPOSE + # body; the unattributed Stray PROPOSE/NACK must NOT be present. + slice1_md = tmp_path / ".egg-state" / "brc-history" / "42-implement-slice-1.md" + assert slice1_md.exists() + content = slice1_md.read_text() + assert "missing slice_id" not in content, "Unattributed message leaked into slice-1 file" + assert "Stray PROPOSE" not in content + assert "Stray NACK" not in content + + # An aggregate file must STILL not exist, even though there were + # unattributed messages — the writer never falls back. + assert not (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").exists() + assert not (tmp_path / ".egg-state" / "brc-history" / "42-implement.json").exists() + + # A single warning was emitted with the dropped count. + warning_calls = [ + c + for c in mock_logger.warning.call_args_list + if "slice_id" in str(c) or "without metadata" in str(c) + ] + assert len(warning_calls) >= 1, ( + f"Expected a warning about dropped messages, got: {mock_logger.warning.call_args_list}" + ) + # The warning surfaces the dropped count so an operator knows scale. + kwargs = warning_calls[0][1] + assert kwargs.get("dropped_count") == 2 + + def test_all_messages_unattributed_writes_aggregate_babysit_fallback(self, tmp_path): + """When EVERY implement-phase BRC message lacks ``slice_id``, the + writer falls back to the aggregate ``{identifier}-implement.{md,json}`` + filename so non-slice pipelines (babysit_pr) keep producing the + artifact documented in ``skills/babysit-pr/SKILL.md``. + + Surfaced as v2-NACK reviewer_code_holistic finding #2 (#2548): v2 + dropped the entire BRC stream for babysit_pr; v3 falls back to + aggregate when no message carries a canonical slice_id. + """ + from routes.pipelines import _write_brc_history + + unattributed = [ + _make_brc_message( + pipeline_id="issue-42", + from_role="coder", + message_type=MessageType.CONSENSUS_PROPOSE, + subject=f"Stray {i}", + body="no slice_id", + phase="implement", + slice_id=None, + ) + for i in range(3) + ] + for m in unattributed: + assert "slice_id" not in m.metadata + + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = unattributed + + with patch("message_store.get_message_store", return_value=mock_store): + _write_brc_history(tmp_path, "issue-42", "implement", 42) + + history_dir = tmp_path / ".egg-state" / "brc-history" + # Babysit fallback: aggregate IS produced when no slice_id anywhere. + assert (history_dir / "42-implement.md").exists(), ( + "Babysit fallback: aggregate file MUST be written when no message " + "carries slice_id (preserves documented babysit_pr artifact)" + ) + assert (history_dir / "42-implement.json").exists() + # And NO per-slice files were produced. + per_slice = list(history_dir.glob("42-implement-*.md")) + assert per_slice == [], f"Babysit fallback must NOT write per-slice files, got: {per_slice}" + + def test_babysit_aggregate_fallback_contains_all_messages(self, tmp_path): + """The babysit aggregate fallback contains every (BRC-eligible) + implement-phase message — no message is silently dropped just + because no message in the bucket happened to carry slice_id.""" + from routes.pipelines import _write_brc_history + + unattributed = [ + _make_brc_message( + pipeline_id="issue-42", + from_role="coder", + message_type=MessageType.CONSENSUS_PROPOSE, + subject="Babysit-PROPOSE", + body="alpha", + phase="implement", + slice_id=None, + ), + _make_brc_message( + pipeline_id="issue-42", + from_role="reviewer_code", + message_type=MessageType.CONSENSUS_ACK, + subject="Babysit-ACK", + body="beta", + phase="implement", + slice_id=None, + ), + ] + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = unattributed + + with patch("message_store.get_message_store", return_value=mock_store): + _write_brc_history(tmp_path, "issue-42", "implement", 42) + + content = (tmp_path / ".egg-state" / "brc-history" / "42-implement.md").read_text() + assert "alpha" in content, "Babysit aggregate dropped the PROPOSE body" + assert "beta" in content, "Babysit aggregate dropped the ACK body" + assert "Babysit-PROPOSE" in content + assert "Babysit-ACK" in content + + def test_refine_phase_keeps_aggregate_filename(self, tmp_path): + """Regression: refine phase still writes the aggregate + ``42-refine.{md,json}`` and does NOT write a per-slice file even when + messages happen to carry ``metadata['slice_id']``.""" + from routes.pipelines import _write_brc_history + + # Refine messages with a slice_id leftover (defensive — should be + # ignored for non-implement phases). + messages = _make_brc_messages(pipeline_id="issue-42", phase="refine", slice_id="slice-1") + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = messages + + with patch("message_store.get_message_store", return_value=mock_store): + _write_brc_history(tmp_path, "issue-42", "refine", 42) + + history_dir = tmp_path / ".egg-state" / "brc-history" + assert (history_dir / "42-refine.md").exists(), "Refine aggregate .md missing" + assert (history_dir / "42-refine.json").exists(), "Refine aggregate .json missing" + # No per-slice refine file should exist. + assert not (history_dir / "42-refine-slice-1.md").exists(), ( + "Refine phase must not partition by slice" + ) + assert not (history_dir / "42-refine-slice-1.json").exists(), ( + "Refine phase must not partition by slice" + ) + + def test_plan_phase_keeps_aggregate_filename(self, tmp_path): + """Regression: plan phase still writes the aggregate + ``42-plan.{md,json}`` (only implement is per-slice — D4).""" + from routes.pipelines import _write_brc_history + + messages = _make_brc_messages(pipeline_id="issue-42", phase="plan", slice_id="slice-1") + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = messages + + with patch("message_store.get_message_store", return_value=mock_store): + _write_brc_history(tmp_path, "issue-42", "plan", 42) + + history_dir = tmp_path / ".egg-state" / "brc-history" + assert (history_dir / "42-plan.md").exists() + assert (history_dir / "42-plan.json").exists() + assert not (history_dir / "42-plan-slice-1.md").exists() + assert not (history_dir / "42-plan-slice-1.json").exists() + + def test_pr_phase_keeps_aggregate_filename(self, tmp_path): + """Regression: pr phase still writes the aggregate + ``42-pr.{md,json}``. The contract carved out implement only.""" + from routes.pipelines import _write_brc_history + + messages = _make_brc_messages(pipeline_id="issue-42", phase="pr", slice_id="slice-1") + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = messages + + with patch("message_store.get_message_store", return_value=mock_store): + _write_brc_history(tmp_path, "issue-42", "pr", 42) + + history_dir = tmp_path / ".egg-state" / "brc-history" + assert (history_dir / "42-pr.md").exists() + assert (history_dir / "42-pr.json").exists() + assert not (history_dir / "42-pr-slice-1.md").exists() + assert not (history_dir / "42-pr-slice-1.json").exists() + + def test_partial_attribution_only_attributed_messages_get_files(self, tmp_path): + """Mix of slice-1, slice-2, and unattributed messages: per-slice + files exist for slice-1 and slice-2, no aggregate, unattributed are + dropped with a single warning.""" + from routes.pipelines import _write_brc_history + + slice1_msgs = self._make_implement_msgs("slice-1") + slice2_msgs = self._make_implement_msgs("slice-2") + # One unattributed message. + stray = _make_brc_message( + pipeline_id="issue-42", + phase="implement", + from_role="coder", + message_type=MessageType.CONSENSUS_PROPOSE, + subject="Stray", + body="no slice_id", + slice_id=None, + ) + assert "slice_id" not in stray.metadata + all_messages = slice1_msgs + slice2_msgs + [stray] + + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = all_messages + + with ( + patch("message_store.get_message_store", return_value=mock_store), + patch("routes.pipelines.logger") as mock_logger, + ): + _write_brc_history(tmp_path, "issue-42", "implement", 42) + + history_dir = tmp_path / ".egg-state" / "brc-history" + produced = sorted(p.name for p in history_dir.glob("42-implement*")) + assert produced == [ + "42-implement-slice-1.json", + "42-implement-slice-1.md", + "42-implement-slice-2.json", + "42-implement-slice-2.md", + ], f"Unexpected files: {produced}" + + # The dropped count must equal exactly 1 — the writer must not + # double-count or miscount. + warning_calls = [ + c + for c in mock_logger.warning.call_args_list + if "slice_id" in str(c) or "without metadata" in str(c) + ] + assert any(c[1].get("dropped_count") == 1 for c in warning_calls), ( + f"Expected dropped_count=1, got warnings: {warning_calls}" + ) + + def test_non_consensus_unattributed_routed_to_unattributed_sibling_file(self, tmp_path): + """Non-CONSENSUS BRC types (HEARTBEAT, OVERSEER_ALERT, AGENT_FAILED, + STATUS, NUDGE, HANDOFF) without ``metadata['slice_id']`` are routed + to ``{identifier}-implement-unattributed.{md,json}`` rather than + dropped. Their emitters do not uniformly carry slice scope (overseer + respawn, HealthMonitor escalation, CLI message-send), and dropping + them would silently strip cross-cutting context from per-slice + transcripts. See #2548 reviewer_code blocking finding.""" + from routes.pipelines import _write_brc_history + + # One canonical CONSENSUS_PROPOSE so partition mode engages. + attributed = self._make_implement_msgs("slice-1") + # An OVERSEER_ALERT and a HEARTBEAT, both without slice_id — these + # should land in the unattributed sibling, not be dropped. + unattributed_other = [ + _make_brc_message( + pipeline_id="issue-42", + from_role="orchestrator", + message_type=MessageType.OVERSEER_ALERT, + subject="brc_confirmation_timeout — call mcp__brc__confirm", + body="orchestrator nudge with no explicit slice scope", + phase="implement", + slice_id=None, + ), + _make_brc_message( + pipeline_id="issue-42", + from_role="coder", + message_type=MessageType.HEARTBEAT, + subject="heartbeat: WORKING", + body="", + phase="implement", + slice_id=None, + ), + ] + for m in unattributed_other: + assert "slice_id" not in m.metadata, "fixture leak — slice_id stamped" + + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = attributed + unattributed_other + + with ( + patch("message_store.get_message_store", return_value=mock_store), + patch("routes.pipelines.logger") as mock_logger, + ): + _write_brc_history(tmp_path, "issue-42", "implement", 42) + + history_dir = tmp_path / ".egg-state" / "brc-history" + # Per-slice file exists for the canonical message. + assert (history_dir / "42-implement-slice-1.md").exists() + assert (history_dir / "42-implement-slice-1.json").exists() + # Unattributed sibling file exists and contains the OVERSEER_ALERT + # and HEARTBEAT. + unattributed_md = history_dir / "42-implement-unattributed.md" + unattributed_json = history_dir / "42-implement-unattributed.json" + assert unattributed_md.exists(), ( + "Non-CONSENSUS BRC messages without slice_id must land in the " + "unattributed sibling, not be dropped" + ) + assert unattributed_json.exists() + content = unattributed_md.read_text() + assert "brc_confirmation_timeout" in content + assert "OVERSEER_ALERT" in content + assert "HEARTBEAT" in content + # No aggregate file (partition mode is engaged). + assert not (history_dir / "42-implement.md").exists() + # No CONSENSUS_* drop warning (none were dropped). + warning_calls = [ + c + for c in mock_logger.warning.call_args_list + if "without canonical metadata.slice_id" in str(c) + ] + assert warning_calls == [], ( + f"Did not expect drop warnings for non-CONSENSUS unattributed; got: {warning_calls}" + ) + + def test_consensus_dropped_non_consensus_routed_when_mixed(self, tmp_path): + """When unattributed messages include BOTH CONSENSUS_* and + non-CONSENSUS_* types, the writer must split the bucket: CONSENSUS_* + are dropped with a warning (D4 contract violation), non-CONSENSUS_* + are routed to the unattributed sibling so the audit trail stays + complete.""" + from routes.pipelines import _write_brc_history + + attributed = self._make_implement_msgs("slice-1") + stray_consensus = _make_brc_message( + pipeline_id="issue-42", + from_role="coder", + message_type=MessageType.CONSENSUS_PROPOSE, + subject="Stray PROPOSE", + body="contract violation — must be dropped", + phase="implement", + slice_id=None, + ) + stray_alert = _make_brc_message( + pipeline_id="issue-42", + from_role="orchestrator", + message_type=MessageType.OVERSEER_ALERT, + subject="overseer_restart", + body="cross-cutting alert — must be routed to unattributed", + phase="implement", + slice_id=None, + ) + + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = attributed + [stray_consensus, stray_alert] + + with ( + patch("message_store.get_message_store", return_value=mock_store), + patch("routes.pipelines.logger") as mock_logger, + ): + _write_brc_history(tmp_path, "issue-42", "implement", 42) + + history_dir = tmp_path / ".egg-state" / "brc-history" + # CONSENSUS_* drop produced a warning with count=1 (only the stray + # PROPOSE, not the OVERSEER_ALERT). + warning_calls = [ + c + for c in mock_logger.warning.call_args_list + if "CONSENSUS_*" in str(c) or "without canonical metadata.slice_id" in str(c) + ] + assert any(c[1].get("dropped_count") == 1 for c in warning_calls), ( + f"Expected CONSENSUS_* drop warning with count=1; got {warning_calls}" + ) + # The OVERSEER_ALERT landed in the unattributed sibling. + unattributed_md = (history_dir / "42-implement-unattributed.md").read_text() + assert "overseer_restart" in unattributed_md + assert "OVERSEER_ALERT" in unattributed_md + # The stray CONSENSUS_PROPOSE did NOT land anywhere. + for produced in history_dir.glob("42-implement*.md"): + content = produced.read_text() + assert "Stray PROPOSE" not in content, ( + f"CONSENSUS_* drop must not leak into {produced.name}" + ) + + def test_implement_messages_with_empty_slice_id_treated_as_unattributed(self, tmp_path): + """Empty-string slice_id fails ``SLICE_ID_PATTERN`` validation and is + treated as unattributed. + + Critically, the writer must NEVER produce a file named + ``42-implement-.md`` (i.e. interpolating the empty string into the + per-slice stem) — that would be both ugly on disk and a path-shape + injection vector. + + When mixed with at least one canonical-attributed message, the + empty-slice_id messages are dropped with a warning. When all + messages have empty slice_id, the babysit aggregate fallback + engages (separate test). + """ + from routes.pipelines import _write_brc_history + + # Mix an empty-slice_id message with a canonical one so partition + # mode engages (otherwise we'd get the aggregate fallback path). + canonical = _make_brc_message( + pipeline_id="issue-42", + phase="implement", + from_role="coder", + message_type=MessageType.CONSENSUS_PROPOSE, + subject="Canonical", + body="ok", + slice_id="slice-1", + ) + empty = _make_brc_message( + pipeline_id="issue-42", + phase="implement", + from_role="reviewer_code", + message_type=MessageType.CONSENSUS_NACK, + subject="Empty slice_id", + body="should be dropped", + metadata={"slice_id": ""}, + slice_id=None, + ) + # Sanity: the fixture really has empty string slice_id. + assert empty.metadata.get("slice_id") == "" + + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = [canonical, empty] + + with ( + patch("message_store.get_message_store", return_value=mock_store), + patch("routes.pipelines.logger") as mock_logger, + ): + _write_brc_history(tmp_path, "issue-42", "implement", 42) + + history_dir = tmp_path / ".egg-state" / "brc-history" + # The dangerous filename MUST NOT be produced. + assert not (history_dir / "42-implement-.md").exists(), ( + "Empty slice_id must NOT be interpolated into a per-slice stem" + ) + # The canonical slice-1 file IS produced. + assert (history_dir / "42-implement-slice-1.md").exists() + # Aggregate is NOT produced (because partition mode engaged). + assert not (history_dir / "42-implement.md").exists() + # Drop warning was emitted with count=1. + warning_calls = [ + c + for c in mock_logger.warning.call_args_list + if "slice_id" in str(c) or "without canonical" in str(c) + ] + assert len(warning_calls) >= 1, "Expected drop warning for empty slice_id" + assert any(c[1].get("dropped_count") == 1 for c in warning_calls) + + def test_three_slices_all_get_distinct_files(self, tmp_path): + """N=3 slices produces 3 distinct per-slice .md/.json pairs in + deterministic order — exercises the bucket sort path.""" + from routes.pipelines import _write_brc_history + + messages = [] + for sid in ["slice-3", "slice-1", "slice-2"]: # deliberately unordered + messages.extend(self._make_implement_msgs(sid)) + + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = messages + + with patch("message_store.get_message_store", return_value=mock_store): + _write_brc_history(tmp_path, "issue-42", "implement", 42) + + history_dir = tmp_path / ".egg-state" / "brc-history" + for sid in ["slice-1", "slice-2", "slice-3"]: + assert (history_dir / f"42-implement-{sid}.md").exists(), ( + f"Per-slice file for {sid} missing" + ) + assert (history_dir / f"42-implement-{sid}.json").exists() + # Aggregate must not exist. + assert not (history_dir / "42-implement.md").exists() + assert not (history_dir / "42-implement.json").exists() + + def test_idempotent_per_slice_write(self, tmp_path): + """Running the writer twice with the same input produces + byte-identical per-slice files AND a byte-identical + ``unattributed`` sibling file (idempotency invariant from #1714 + carried over into per-slice mode + the cross-cutting sibling + added in the per-slice partition fix).""" + from routes.pipelines import _write_brc_history + + messages = [] + for sid in ["slice-1", "slice-2"]: + messages.extend(self._make_implement_msgs(sid)) + # Mix in non-CONSENSUS BRC types without slice_id so the writer + # produces the unattributed sibling alongside the per-slice + # files. The sibling is committed to the branch and read by + # reviewers, so it is on the same idempotency contract. + messages.append( + _make_brc_message( + pipeline_id="issue-42", + from_role="overseer", + message_type=MessageType.OVERSEER_ALERT, + subject="brc_confirmation_timeout", + body="elapsed", + phase="implement", + timestamp=datetime(2026, 4, 8, 12, 30, 0, tzinfo=UTC), + slice_id=None, + ) + ) + messages.append( + _make_brc_message( + pipeline_id="issue-42", + from_role="coder", + message_type=MessageType.HEARTBEAT, + subject="alive", + body="hb", + phase="implement", + timestamp=datetime(2026, 4, 8, 12, 31, 0, tzinfo=UTC), + slice_id=None, + ) + ) + + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = messages + + history_dir = tmp_path / ".egg-state" / "brc-history" + + with patch("message_store.get_message_store", return_value=mock_store): + _write_brc_history(tmp_path, "issue-42", "implement", 42) + first_s1_md = (history_dir / "42-implement-slice-1.md").read_text() + first_s2_md = (history_dir / "42-implement-slice-2.md").read_text() + first_s1_json = (history_dir / "42-implement-slice-1.json").read_text() + first_s2_json = (history_dir / "42-implement-slice-2.json").read_text() + first_unattr_md = (history_dir / "42-implement-unattributed.md").read_text() + first_unattr_json = (history_dir / "42-implement-unattributed.json").read_text() + + # Second call (e.g. PR-phase safety-net rewrite). + _write_brc_history(tmp_path, "issue-42", "implement", 42) + second_s1_md = (history_dir / "42-implement-slice-1.md").read_text() + second_s2_md = (history_dir / "42-implement-slice-2.md").read_text() + second_s1_json = (history_dir / "42-implement-slice-1.json").read_text() + second_s2_json = (history_dir / "42-implement-slice-2.json").read_text() + second_unattr_md = (history_dir / "42-implement-unattributed.md").read_text() + second_unattr_json = (history_dir / "42-implement-unattributed.json").read_text() + + assert first_s1_md == second_s1_md, "slice-1 markdown not idempotent" + assert first_s2_md == second_s2_md, "slice-2 markdown not idempotent" + assert first_s1_json == second_s1_json, "slice-1 JSON not idempotent" + assert first_s2_json == second_s2_json, "slice-2 JSON not idempotent" + assert first_unattr_md == second_unattr_md, "unattributed sibling markdown not idempotent" + assert first_unattr_json == second_unattr_json, "unattributed sibling JSON not idempotent" + + def test_message_metadata_is_always_a_dict(self): + """Pydantic invariant: ``Message.metadata`` is a dict[str, Any] field + with ``default_factory=dict`` — the writer relies on this to skip + defensive None/non-dict guards (#2548 reviewer_code non-blocking). + Pin the invariant so a future Pydantic-config change (e.g. allowing + None) shows up here rather than as a runtime crash in the writer. + """ + # Default construction yields an empty dict, never None. + m = Message( + pipeline_id="issue-42", + from_role="coder", + to_role="all", + message_type=MessageType.CONSENSUS_PROPOSE, + subject="x", + body="y", + phase="implement", + ) + assert isinstance(m.metadata, dict), ( + f"Pydantic default for Message.metadata must be a dict, got {type(m.metadata)}" + ) + assert m.metadata == {} + # Explicit dict is preserved. + m2 = Message( + pipeline_id="issue-42", + from_role="coder", + to_role="all", + message_type=MessageType.CONSENSUS_PROPOSE, + subject="x", + body="y", + phase="implement", + metadata={"slice_id": "slice-1"}, + ) + assert isinstance(m2.metadata, dict) + assert m2.metadata.get("slice_id") == "slice-1" + + def test_invalid_slice_id_pattern_treated_as_unattributed(self, tmp_path): + """slice_id values that don't match SLICE_ID_PATTERN (``^slice-[0-9]+$``) + are treated as unattributed — preventing path-traversal / + filename-injection through metadata. + + This is a defense-in-depth test: SLICE_ID_PATTERN is enforced at + every gateway-facing seam upstream, but the writer also validates + locally so a future leak cannot smuggle ``../etc/passwd`` (or any + non-canonical value) into ``42-implement-.md``. + """ + from routes.pipelines import _write_brc_history + + # A canonical slice-1 message so the writer engages partition mode + # (otherwise it would fall back to the babysit aggregate). + canonical = _make_brc_message( + pipeline_id="issue-42", + phase="implement", + from_role="coder", + message_type=MessageType.CONSENSUS_PROPOSE, + subject="canonical", + body="ok", + slice_id="slice-1", + ) + # Various malformed slice_id values that MUST NOT produce a file. + injection_payloads = [ + "../etc/passwd", # path traversal + "slice-1/extra", # directory separator + "slice-1.bad", # extra suffix + "slice-", # missing digits + "phase-1", # legacy non-canonical + "SLICE-1", # case mismatch + "slice- 1", # whitespace + "slice-01a", # non-digits + "slice-1\nx", # newline injection + ] + bad_msgs = [ + _make_brc_message( + pipeline_id="issue-42", + phase="implement", + from_role="coder", + message_type=MessageType.CONSENSUS_NACK, + subject=f"bad-{i}", + body=f"injection {payload!r}", + metadata={"slice_id": payload}, + slice_id=None, # let metadata stand + ) + for i, payload in enumerate(injection_payloads) + ] + + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = [canonical, *bad_msgs] + + with ( + patch("message_store.get_message_store", return_value=mock_store), + patch("routes.pipelines.logger") as mock_logger, + ): + _write_brc_history(tmp_path, "issue-42", "implement", 42) + + history_dir = tmp_path / ".egg-state" / "brc-history" + # Only the canonical slice-1 file exists. + produced = sorted(p.name for p in history_dir.glob("42-implement*")) + assert produced == [ + "42-implement-slice-1.json", + "42-implement-slice-1.md", + ], f"Malformed slice_id payloads leaked into output files: {produced}" + + # The aggregate file MUST NOT exist (we have at least one canonical). + assert not (history_dir / "42-implement.md").exists() + # Path-traversal: must not have written anywhere outside brc-history. + # Sanity: the brc-history dir is the ONLY dir under .egg-state for + # this test (write would have escaped if traversal succeeded). + sibling_dirs = sorted(p.name for p in (tmp_path / ".egg-state").iterdir() if p.is_dir()) + assert sibling_dirs == ["brc-history"], ( + f"Unexpected directories created: {sibling_dirs} — possible traversal" + ) + + # Drop warning was emitted with the right shape. + warning_calls = [ + c + for c in mock_logger.warning.call_args_list + if "slice_id" in str(c) or "without canonical" in str(c) + ] + assert len(warning_calls) >= 1, ( + f"Expected drop warning for malformed slice_ids, got: {mock_logger.warning.call_args_list}" + ) + kwargs = warning_calls[0][1] + assert kwargs.get("dropped_count") == len(injection_payloads), ( + f"Expected dropped_count={len(injection_payloads)}, got: {kwargs}" + ) + + def test_natural_sort_per_slice_iteration_order(self, tmp_path): + """Per-slice buckets iterate in natural-sort (integer-suffix) order + so a 12-slice pipeline writes ``slice-1, slice-2, … slice-12`` and + not the lexicographic ``slice-1, slice-10, slice-11, slice-12, + slice-2, …`` (#2548 reviewer_code non-blocking). + + The current writer doesn't expose iteration order externally beyond + the order of ``logger.info`` "Wrote BRC history file" calls, so we + intercept those to assert the expected sequence. + """ + from routes.pipelines import _write_brc_history + + # 12 slices in deliberately shuffled input order. + sids = ["slice-7", "slice-1", "slice-12", "slice-2", "slice-11"] + messages = [] + for sid in sids: + messages.extend(self._make_implement_msgs(sid)) + + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = messages + + wrote_calls: list[dict] = [] + + def capture(*args, **kwargs): + if args and args[0] == "Wrote BRC history file": + wrote_calls.append(kwargs) + + with ( + patch("message_store.get_message_store", return_value=mock_store), + patch("routes.pipelines.logger") as mock_logger, + ): + mock_logger.info.side_effect = capture + _write_brc_history(tmp_path, "issue-42", "implement", 42) + + # Order of slice_ids in the "Wrote BRC history file" log entries + # should be sorted by integer suffix. + slice_ids_in_order = [c["slice_id"] for c in wrote_calls] + assert slice_ids_in_order == [ + "slice-1", + "slice-2", + "slice-7", + "slice-11", + "slice-12", + ], f"Expected natural-sort iteration order, got {slice_ids_in_order}" + + +class TestPerSliceImplementBrcHistoryRewriteForPr: + """The PR-phase safety-net rewrite (``_rewrite_brc_history_for_pr``) + inherits the per-slice partitioning from ``_write_brc_history`` (#2548). + These tests verify the rewrite path treats per-slice files correctly + and never produces an aggregate. + """ + + def test_rewrite_for_pr_emits_per_slice_implement_files(self, tmp_path): + """When the PR phase rewrites BRC history, the implement-phase + rewrite produces per-slice files and no aggregate file.""" + from routes.pipelines import _rewrite_brc_history_for_pr + + messages = _make_brc_messages(pipeline_id="issue-42", phase="implement", slice_id="slice-1") + messages.extend( + _make_brc_messages(pipeline_id="issue-42", phase="implement", slice_id="slice-2") + ) + + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = messages + + phases = { + "implement": MagicMock(status=PipelineStatus.COMPLETE), + } + + with ( + patch("message_store.get_message_store", return_value=mock_store), + patch("routes.pipelines._commit_statefiles_to_worktree"), + ): + _rewrite_brc_history_for_pr(tmp_path, "issue-42", phases, 42) + + history_dir = tmp_path / ".egg-state" / "brc-history" + assert (history_dir / "42-implement-slice-1.md").exists() + assert (history_dir / "42-implement-slice-2.md").exists() + assert not (history_dir / "42-implement.md").exists() + assert not (history_dir / "42-implement.json").exists() + + def test_rewrite_for_pr_mixes_aggregate_refine_and_per_slice_implement(self, tmp_path): + """Mixed multi-phase rewrite: refine emits aggregate, implement + emits per-slice — both shapes coexist in the same brc-history dir.""" + from routes.pipelines import _rewrite_brc_history_for_pr + + all_messages = [] + all_messages.extend(_make_brc_messages(pipeline_id="issue-42", phase="refine")) + all_messages.extend( + _make_brc_messages(pipeline_id="issue-42", phase="implement", slice_id="slice-1") + ) + + mock_store = MagicMock(spec=MessageStore) + mock_store.get_messages.return_value = all_messages + + phases = { + "refine": MagicMock(status=PipelineStatus.COMPLETE), + "implement": MagicMock(status=PipelineStatus.COMPLETE), + } + + with ( + patch("message_store.get_message_store", return_value=mock_store), + patch("routes.pipelines._commit_statefiles_to_worktree"), + ): + _rewrite_brc_history_for_pr(tmp_path, "issue-42", phases, 42) + + history_dir = tmp_path / ".egg-state" / "brc-history" + # Refine: aggregate. Implement: per-slice. + assert (history_dir / "42-refine.md").exists() + assert (history_dir / "42-refine.json").exists() + assert (history_dir / "42-implement-slice-1.md").exists() + assert (history_dir / "42-implement-slice-1.json").exists() + # No aggregate implement file. + assert not (history_dir / "42-implement.md").exists() + assert not (history_dir / "42-implement.json").exists() + # Refine never partitions by slice. + assert not (history_dir / "42-refine-slice-1.md").exists() diff --git a/orchestrator/tests/test_brc_phase_propagation.py b/orchestrator/tests/test_brc_phase_propagation.py index b922caae19..f2d3fd0005 100644 --- a/orchestrator/tests/test_brc_phase_propagation.py +++ b/orchestrator/tests/test_brc_phase_propagation.py @@ -48,6 +48,13 @@ def _make_pipeline( ) +# Default slice_id seeded onto implement-phase BRC messages so the +# post-#2548 hard-switchover writer accepts them. Tests that need an +# unattributed (missing-slice_id) message must set ``slice_id=None`` +# AND avoid passing ``metadata={"slice_id": ...}``. +_DEFAULT_IMPLEMENT_SLICE_ID = "slice-1" + + def _make_brc_message( pipeline_id="issue-42", from_role="coder", @@ -57,8 +64,20 @@ def _make_brc_message( phase=None, timestamp=None, metadata=None, + slice_id="__default__", ): - """Create a BRC Message for testing. Defaults to phase=None to mimic pre-fix.""" + """Create a BRC Message for testing. Defaults to phase=None to mimic pre-fix. + + For implement-phase messages, ``metadata['slice_id']`` is auto-stamped + to ``slice-1`` (#2548 hard switchover) unless the caller passes an + explicit ``slice_id`` or sets the key in ``metadata`` directly. + """ + md = dict(metadata or {}) + if slice_id == "__default__": + if phase == "implement" and "slice_id" not in md: + md["slice_id"] = _DEFAULT_IMPLEMENT_SLICE_ID + elif slice_id is not None: + md.setdefault("slice_id", slice_id) return Message( pipeline_id=pipeline_id, from_role=from_role, @@ -68,7 +87,7 @@ def _make_brc_message( body=body, phase=phase, timestamp=timestamp or datetime(2026, 4, 8, 12, 0, 0, tzinfo=UTC), - metadata=metadata or {}, + metadata=md, ) @@ -715,7 +734,13 @@ def test_includes_messages_with_correct_phase(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - expected_path = tmp_path / ".egg-state" / "brc-history" / "42-implement.md" + # #2548: implement is per-slice — the aggregate file is gone. + expected_path = ( + tmp_path + / ".egg-state" + / "brc-history" + / f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md" + ) assert expected_path.exists() content = expected_path.read_text() assert "New message" in content diff --git a/orchestrator/tests/test_conditional_ack.py b/orchestrator/tests/test_conditional_ack.py index 061e1cfcc9..7b8c109159 100644 --- a/orchestrator/tests/test_conditional_ack.py +++ b/orchestrator/tests/test_conditional_ack.py @@ -776,6 +776,9 @@ def _conditional_ack_message(self): "pre_merge_condition": "git mv legacy/x new/x before merge", }, "version": 1, + # #2548 hard switchover: implement-phase BRC messages must + # carry a slice_id, otherwise the writer drops them. + "slice_id": "slice-1", }, ) @@ -788,7 +791,7 @@ def test_condition_appears_in_markdown_transcript(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - md_path = tmp_path / ".egg-state" / "brc-history" / "42-implement.md" + md_path = tmp_path / ".egg-state" / "brc-history" / "42-implement-slice-1.md" assert md_path.exists() content = md_path.read_text() assert "pre_merge_condition" in content @@ -803,7 +806,7 @@ def test_condition_appears_in_json_companion(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - json_path = tmp_path / ".egg-state" / "brc-history" / "42-implement.json" + json_path = tmp_path / ".egg-state" / "brc-history" / "42-implement-slice-1.json" assert json_path.exists() data = json.loads(json_path.read_text()) assert len(data) == 1 diff --git a/orchestrator/tests/test_diagnostic_logging_1633.py b/orchestrator/tests/test_diagnostic_logging_1633.py index 15584f827e..d6d2f3aec6 100644 --- a/orchestrator/tests/test_diagnostic_logging_1633.py +++ b/orchestrator/tests/test_diagnostic_logging_1633.py @@ -22,6 +22,10 @@ from message_store import Message, MessageStore, MessageType from models import PipelineStatus +# Default slice_id seeded onto implement-phase BRC messages so the +# post-#2548 hard-switchover writer accepts them. +_DEFAULT_IMPLEMENT_SLICE_ID = "slice-1" + def _make_brc_message( pipeline_id="issue-42", @@ -31,8 +35,20 @@ def _make_brc_message( body="test body", phase="implement", timestamp=None, + slice_id="__default__", ): - """Create a BRC message for testing.""" + """Create a BRC message for testing. + + For implement-phase messages, ``metadata['slice_id']`` is auto-stamped + to ``slice-1`` (#2548 hard switchover) unless ``slice_id`` is set + explicitly (pass ``None`` to test the missing-slice_id drop path). + """ + md: dict = {} + if slice_id == "__default__": + if phase == "implement": + md["slice_id"] = _DEFAULT_IMPLEMENT_SLICE_ID + elif slice_id is not None: + md["slice_id"] = slice_id return Message( pipeline_id=pipeline_id, from_role=from_role, @@ -42,7 +58,7 @@ def _make_brc_message( body=body, phase=phase, timestamp=timestamp or datetime(2026, 4, 8, 12, 0, 0, tzinfo=UTC), - metadata={}, + metadata=md, ) @@ -511,10 +527,15 @@ def fake_run(cmd, **kwargs): with patch("message_store.get_message_store", return_value=mock_store): mod._rewrite_brc_history_for_pr(worktree, pipeline_id, phases, identifier) - # Assertion (a): BRC history files written for both COMPLETE phases + # Assertion (a): BRC history files written for both COMPLETE phases. + # #2548: implement phase is per-slice; aggregate file is gone. brc_dir = worktree / ".egg-state" / "brc-history" assert (brc_dir / "42-refine.md").exists(), "BRC history for refine should exist" - assert (brc_dir / "42-implement.md").exists(), "BRC history for implement should exist" + impl_file = brc_dir / f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md" + assert impl_file.exists(), "Per-slice BRC history for implement should exist" + assert not (brc_dir / "42-implement.md").exists(), ( + "Aggregate implement.md leaked through hard switchover" + ) # Assertion (b): BRC files have correct content refine_content = (brc_dir / "42-refine.md").read_text() @@ -524,7 +545,7 @@ def fake_run(cmd, **kwargs): assert "CONSENSUS_PROPOSE" in refine_content assert "CONSENSUS_ACK" in refine_content - implement_content = (brc_dir / "42-implement.md").read_text() + implement_content = impl_file.read_text() assert "implement phase" in implement_content assert "Tests pass" in implement_content @@ -578,7 +599,13 @@ def fake_run(cmd, **kwargs): with patch("message_store.get_message_store", return_value=mock_store): mod._rewrite_brc_history_for_pr(worktree, "issue-42", phases, 42) - history_file = worktree / ".egg-state" / "brc-history" / "42-implement.md" + # #2548: implement is per-slice — the aggregate file is gone. + history_file = ( + worktree + / ".egg-state" + / "brc-history" + / f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md" + ) assert history_file.exists(), "BRC history file should exist on disk" content = history_file.read_text() @@ -647,7 +674,12 @@ def fake_run(cmd, **kwargs): brc_dir = worktree / ".egg-state" / "brc-history" assert (brc_dir / "42-refine.md").exists(), "COMPLETE phase should have BRC file" assert not (brc_dir / "42-plan.md").exists(), "FAILED phase should NOT have BRC file" + # #2548: implement is per-slice now; no aggregate, and the per-slice + # file should also be absent because the implement phase is RUNNING. assert not (brc_dir / "42-implement.md").exists(), "RUNNING phase should NOT have BRC file" + assert not (brc_dir / f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md").exists(), ( + "RUNNING phase should NOT have per-slice BRC file" + ) # --------------------------------------------------------------------------- @@ -682,7 +714,10 @@ def test_write_brc_history_messages_exist_but_wrong_phase(self, tmp_path): _write_brc_history(tmp_path, "issue-42", "implement", 42) # Request "implement" history_dir = tmp_path / ".egg-state" / "brc-history" + # #2548: neither aggregate nor per-slice file should exist when no + # implement-phase messages are present. assert not (history_dir / "42-implement.md").exists() + assert not (history_dir / f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md").exists() def test_write_brc_history_string_identifier(self, tmp_path): """_write_brc_history works with string pipeline identifiers.""" @@ -695,8 +730,15 @@ def test_write_brc_history_string_identifier(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", "my-pipeline") - history_file = tmp_path / ".egg-state" / "brc-history" / "my-pipeline-implement.md" + # #2548: implement is per-slice — the aggregate file is gone. + history_file = ( + tmp_path + / ".egg-state" + / "brc-history" + / f"my-pipeline-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md" + ) assert history_file.exists() + assert not (tmp_path / ".egg-state" / "brc-history" / "my-pipeline-implement.md").exists() def test_rewrite_brc_history_mixed_statuses_logging(self, tmp_path): """Entry log correctly reports completed vs non-completed phase counts.""" diff --git a/orchestrator/tests/test_messages.py b/orchestrator/tests/test_messages.py index 721cd29657..6299f94973 100644 --- a/orchestrator/tests/test_messages.py +++ b/orchestrator/tests/test_messages.py @@ -2668,6 +2668,90 @@ def test_heartbeat_rejects_invalid_slice_id(self, client, app, bad_slice_id): ) mock_gw_client.heartbeat_session_by_container.assert_not_called() + def test_heartbeat_message_metadata_carries_slice_id(self, client, app): + """Slice-scoped HEARTBEATs land on the bus with ``slice_id`` in + ``Message.metadata`` so the implement-phase BRC writer (#2548) can + partition them into the correct per-slice transcript file. + + Without this, every slice-scoped HEARTBEAT would be routed to the + ``unattributed`` sibling file even when the producer route already + knew the slice scope. + """ + from message_store import get_message_store + + store = get_message_store() + + with app.test_request_context(): + mock_gw_client = MagicMock() + with ( + patch( + "routes.messages.get_state_store_for_pipeline" + ) as mock_get_store_for_pipeline, + patch( + "gateway_client.get_gateway_client", + return_value=mock_gw_client, + ), + ): + mock_get_store_for_pipeline.return_value = ( + MagicMock(), + _make_pipeline_mock(), + ) + resp = client.post( + "/api/v1/pipelines/heartbeat-slice-meta-pipeline/heartbeat", + json={ + "from_role": "tester", + "state": "WORKING", + "slice_id": "slice-7", + }, + ) + assert resp.status_code == 200 + + # Inspect the stored message's metadata. + messages = store.get_messages("heartbeat-slice-meta-pipeline", limit=10) + heartbeats = [m for m in messages if m.message_type == "HEARTBEAT"] + assert heartbeats, "Expected a HEARTBEAT to be persisted on the bus" + assert heartbeats[-1].metadata.get("slice_id") == "slice-7", ( + f"slice_id missing from HEARTBEAT metadata: {heartbeats[-1].metadata}" + ) + + def test_heartbeat_metadata_omits_slice_id_when_pipeline_level(self, client, app): + """Non-slice (pipeline-level) HEARTBEATs MUST NOT carry a + ``slice_id`` key in metadata. The BRC writer treats absence as + "no slice scope" and falls back to the aggregate filename for + non-slice pipelines (babysit_pr et al.); a stray empty/None value + would smuggle these messages into the slice-aware path.""" + from message_store import get_message_store + + store = get_message_store() + + with app.test_request_context(): + mock_gw_client = MagicMock() + with ( + patch( + "routes.messages.get_state_store_for_pipeline" + ) as mock_get_store_for_pipeline, + patch( + "gateway_client.get_gateway_client", + return_value=mock_gw_client, + ), + ): + mock_get_store_for_pipeline.return_value = ( + MagicMock(), + _make_pipeline_mock(), + ) + resp = client.post( + "/api/v1/pipelines/heartbeat-noslice-pipeline/heartbeat", + json={"from_role": "coder", "state": "WORKING"}, + ) + assert resp.status_code == 200 + + messages = store.get_messages("heartbeat-noslice-pipeline", limit=10) + heartbeats = [m for m in messages if m.message_type == "HEARTBEAT"] + assert heartbeats, "Expected a HEARTBEAT to be persisted on the bus" + assert "slice_id" not in heartbeats[-1].metadata, ( + f"Pipeline-level HEARTBEAT must omit slice_id key, got: {heartbeats[-1].metadata}" + ) + def test_heartbeat_sibling_slices_do_not_share_throttle(self, client, app): """Sibling slices with the same role each fan out independently (#2451). diff --git a/orchestrator/tests/test_pr_phase_brc_rewrite.py b/orchestrator/tests/test_pr_phase_brc_rewrite.py index 0ddf87ea9b..0907f4d738 100644 --- a/orchestrator/tests/test_pr_phase_brc_rewrite.py +++ b/orchestrator/tests/test_pr_phase_brc_rewrite.py @@ -18,6 +18,10 @@ from message_store import Message, MessageStore, MessageType from models import PipelineStatus +# Default slice_id stamped on implement-phase messages so the post-#2548 +# hard-switchover writer accepts them. +_DEFAULT_IMPLEMENT_SLICE_ID = "slice-1" + def _make_brc_message( pipeline_id="issue-42", @@ -27,8 +31,21 @@ def _make_brc_message( body="test body", phase="implement", timestamp=None, + slice_id="__default__", ): - """Create a BRC message for testing.""" + """Create a BRC message for testing. + + For implement-phase messages, ``metadata['slice_id']`` is auto-stamped + to ``slice-1`` (#2548 hard switchover) so the writer keeps producing a + file. Pass ``slice_id=None`` explicitly to test the missing-slice_id + drop path. + """ + md: dict = {} + if slice_id == "__default__": + if phase == "implement": + md["slice_id"] = _DEFAULT_IMPLEMENT_SLICE_ID + elif slice_id is not None: + md["slice_id"] = slice_id return Message( pipeline_id=pipeline_id, from_role=from_role, @@ -38,7 +55,7 @@ def _make_brc_message( body=body, phase=phase, timestamp=timestamp or datetime(2026, 4, 8, 12, 0, 0, tzinfo=UTC), - metadata={}, + metadata=md, ) @@ -84,7 +101,11 @@ def test_writes_history_for_completed_phases(self, tmp_path): history_dir = tmp_path / ".egg-state" / "brc-history" assert (history_dir / "42-refine.md").exists() assert (history_dir / "42-plan.md").exists() - assert (history_dir / "42-implement.md").exists() + # #2548: implement is per-slice — aggregate is gone. + assert (history_dir / f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md").exists() + assert not (history_dir / "42-implement.md").exists(), ( + "Aggregate implement.md leaked through hard switchover" + ) def test_skips_non_complete_phases(self, tmp_path): """Phases with FAILED or RUNNING status are not written.""" @@ -114,8 +135,9 @@ def test_skips_non_complete_phases(self, tmp_path): history_dir = tmp_path / ".egg-state" / "brc-history" assert (history_dir / "42-refine.md").exists() assert (history_dir / "42-plan.md").exists() - # implement was FAILED, so not written + # implement was FAILED, so neither aggregate nor per-slice file exists. assert not (history_dir / "42-implement.md").exists() + assert not (history_dir / f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md").exists() def test_idempotent_rewrite(self, tmp_path): """Re-writing BRC history overwrites existing files safely.""" @@ -135,7 +157,12 @@ def test_idempotent_rewrite(self, tmp_path): with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) - history_file = tmp_path / ".egg-state" / "brc-history" / "42-implement.md" + history_file = ( + tmp_path + / ".egg-state" + / "brc-history" + / f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md" + ) first_content = history_file.read_text() # Add more messages and re-write @@ -201,7 +228,12 @@ def test_regeneration_with_same_messages_is_byte_identical(self, tmp_path): mock_store = MagicMock(spec=MessageStore) mock_store.get_messages.return_value = messages - history_file = tmp_path / ".egg-state" / "brc-history" / "42-implement.md" + history_file = ( + tmp_path + / ".egg-state" + / "brc-history" + / f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md" + ) with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) @@ -230,7 +262,12 @@ def test_generated_timestamp_tracks_latest_message(self, tmp_path): mock_store = MagicMock(spec=MessageStore) mock_store.get_messages.return_value = messages - history_file = tmp_path / ".egg-state" / "brc-history" / "42-implement.md" + history_file = ( + tmp_path + / ".egg-state" + / "brc-history" + / f"42-implement-{_DEFAULT_IMPLEMENT_SLICE_ID}.md" + ) with patch("message_store.get_message_store", return_value=mock_store): _write_brc_history(tmp_path, "issue-42", "implement", 42) diff --git a/orchestrator/tests/test_short_flow_contract_population.py b/orchestrator/tests/test_short_flow_contract_population.py index d4a12a01ef..7491438032 100644 --- a/orchestrator/tests/test_short_flow_contract_population.py +++ b/orchestrator/tests/test_short_flow_contract_population.py @@ -154,6 +154,75 @@ def test_no_plan_draft_is_noop(self, tmp_path: Path): contract = load_contract(pipeline_id, tmp_path) assert len(contract.phases) == 0 # Still empty + def test_populate_contract_from_plan_preserves_deferred_actions(self, tmp_path: Path): + """A re-populate must preserve runtime-only ``PRMetadata`` fields. + + Regression for the slice-1 review in PR #2555: the populator + rebuilds ``contract.pr`` wholesale from the plan, and a prior + version preserved ``context_branch`` / ``context_pr_number`` + but silently wiped ``deferred_actions`` — the merge-blocking + Pre-merge Obligations handoff written by the conditional-ACK + gate at ``decisions.py:complete_phase``. The + ``start_phase=implement`` re-entry path can hit this populator + after ``deferred_actions`` is already populated; losing it + erases the only durable handoff for git-mv / migration / + cross-repo flips. + + Setup: create a contract, populate ``contract.pr`` once from + the plan, then mutate ``contract.pr.deferred_actions`` and + ``contract.pr.context_branch`` / ``context_pr_number`` to + simulate runtime-populated state, save, and re-run the + populator. Assert the runtime fields survive while the + planner-emitted fields are refreshed from the plan. + """ + from egg_contracts.loader import create_contract, load_contract, save_contract + from egg_contracts.models import DeferredAction + from routes.pipelines import _populate_contract_from_plan + + pipeline_id = "pipeline-deferred-preserve" + + create_contract(pipeline_id=pipeline_id, title="Test", repo_root=tmp_path) + + drafts_dir = tmp_path / ".egg-state" / "drafts" + drafts_dir.mkdir(parents=True, exist_ok=True) + (drafts_dir / f"{pipeline_id}-plan.md").write_text(SAMPLE_PLAN) + + # First populate — establishes ``contract.pr`` from the plan. + _populate_contract_from_plan(tmp_path, pipeline_id, "local") + + # Simulate runtime-populated state: a conditional-ACK gate + # resolved at ``complete_phase`` and stamped a deferred action, + # plus the orchestrator opened the context PR and stamped the + # branch / PR-number. + contract = load_contract(pipeline_id, tmp_path) + assert contract.pr is not None + contract.pr.deferred_actions = [ + DeferredAction( + reviewer="reviewer_code", + condition="must rename foo → bar before merge", + resolved_in_diff="", + ) + ] + contract.pr.context_branch = "egg/pipeline-deferred-preserve/context" + contract.pr.context_pr_number = 7777 + save_contract(contract, tmp_path) + + # Re-run the populator (e.g. start_phase=implement re-entry). + _populate_contract_from_plan(tmp_path, pipeline_id, "local") + + # All three runtime-populated fields must survive the re-build. + contract_after = load_contract(pipeline_id, tmp_path) + assert contract_after.pr is not None + assert len(contract_after.pr.deferred_actions) == 1 + assert ( + contract_after.pr.deferred_actions[0].condition == "must rename foo → bar before merge" + ) + assert contract_after.pr.deferred_actions[0].reviewer == "reviewer_code" + assert contract_after.pr.context_branch == "egg/pipeline-deferred-preserve/context" + assert contract_after.pr.context_pr_number == 7777 + # And the planner-emitted fields are still refreshed from the plan. + assert contract_after.pr.title == "Add retry logic to API client" + class TestEnsureStatefilesRestoresPRMetadata: """_ensure_statefiles_on_branch re-populates PR metadata from plan draft. diff --git a/orchestrator/tests/test_signals.py b/orchestrator/tests/test_signals.py index e995b24783..371ae4bd75 100644 --- a/orchestrator/tests/test_signals.py +++ b/orchestrator/tests/test_signals.py @@ -1463,6 +1463,163 @@ def test_valid_decision_proceeds(self, app): assert data["success"] is True mock_tracker.excuse_producer.assert_called_once_with("coder", "Not delivering") + def test_excuse_producer_status_carries_slice_id_metadata(self, app): + """Slice-scoped excuse-producer STATUS lands on the bus with + ``slice_id`` in ``Message.metadata`` so the implement-phase BRC + writer (#2548) routes it into the producer's per-slice transcript.""" + with app.app_context(): + from models import DecisionStatus + from routes.signals import handle_consensus_excuse_producer_signal + + mock_decision = MagicMock() + mock_decision.status = DecisionStatus.RESOLVED + mock_decision.context = "failed_role:coder" + + mock_queue = MagicMock() + mock_queue.get_decision.return_value = mock_decision + + mock_tracker = MagicMock() + mock_tracker.excuse_producer.return_value = { + "status": "excused", + "affected_reviewers": ["reviewer_code"], + } + + mock_store_inst = MagicMock() + + with ( + patch("decision_queue.get_decision_queue", return_value=mock_queue), + patch( + "peer_consensus.get_peer_consensus_tracker", + return_value=mock_tracker, + ), + patch("message_store.get_message_store", return_value=mock_store_inst), + patch("routes.signals._resolve_pipeline_phase", return_value="implement"), + ): + response, status_code = handle_consensus_excuse_producer_signal( + "issue-42", + { + "producer_role": "coder", + "reason": "Not delivering", + "decision_id": "dec-123", + "slice_id": "slice-3", + }, + Path("/tmp/repo"), + ) + + assert status_code == 200 + # Inspect the Message that was added to the store. + mock_store_inst.add_message.assert_called_once() + stored_message = mock_store_inst.add_message.call_args[0][0] + assert stored_message.message_type == "STATUS" + assert stored_message.metadata.get("slice_id") == "slice-3", ( + f"slice_id missing from excuse-producer STATUS metadata: {stored_message.metadata}" + ) + + def test_ready_to_confirm_status_carries_slice_id_metadata(self, app): + """Slice-scoped ``_emit_ready_to_confirm_nudges`` stamps + ``slice_id`` on the ready-to-confirm STATUS so the implement-phase + BRC writer routes the nudge into the producer's per-slice + transcript (#2548 follow-up; pins the metadata stamp on the + ready-to-confirm STATUS path that the three call sites — propose, + ACK, producer-push — feed).""" + with app.app_context(): + from routes.signals import _emit_ready_to_confirm_nudges + + mock_store_inst = MagicMock() + mock_tracker = MagicMock() + + with patch("message_store.get_message_store", return_value=mock_store_inst): + _emit_ready_to_confirm_nudges( + "issue-42", + "implement", + [{"role": "coder", "version": 3}], + tracker=mock_tracker, + slice_id="slice-2", + ) + + mock_store_inst.add_message.assert_called_once() + stored = mock_store_inst.add_message.call_args[0][0] + assert stored.message_type == "STATUS" + assert stored.metadata.get("ready_to_confirm") is True + assert stored.metadata.get("version") == 3 + assert stored.metadata.get("slice_id") == "slice-2", ( + f"slice_id missing from ready-to-confirm STATUS metadata: {stored.metadata}" + ) + + def test_ready_to_confirm_status_omits_slice_id_when_pipeline_level(self, app): + """Pipeline-level (non-slice) ready-to-confirm STATUS MUST NOT + carry a ``slice_id`` key. ``_emit_ready_to_confirm_nudges`` + defaults the parameter to ``None``; the writer treats absence as + "no slice scope" so babysit_pr et al. continue to land in the + aggregate file.""" + with app.app_context(): + from routes.signals import _emit_ready_to_confirm_nudges + + mock_store_inst = MagicMock() + + with patch("message_store.get_message_store", return_value=mock_store_inst): + _emit_ready_to_confirm_nudges( + "issue-42", + "implement", + [{"role": "coder", "version": 1}], + ) + + mock_store_inst.add_message.assert_called_once() + stored = mock_store_inst.add_message.call_args[0][0] + assert "slice_id" not in stored.metadata, ( + f"Pipeline-level ready-to-confirm STATUS must omit slice_id, got: {stored.metadata}" + ) + + def test_excuse_producer_status_omits_slice_id_when_pipeline_level(self, app): + """Non-slice (pipeline-level) excuse-producer STATUS MUST NOT + carry a ``slice_id`` key — the BRC writer treats absence as + "no slice scope" and falls back to the aggregate filename + (babysit_pr et al.).""" + with app.app_context(): + from models import DecisionStatus + from routes.signals import handle_consensus_excuse_producer_signal + + mock_decision = MagicMock() + mock_decision.status = DecisionStatus.RESOLVED + mock_decision.context = "failed_role:coder" + + mock_queue = MagicMock() + mock_queue.get_decision.return_value = mock_decision + + mock_tracker = MagicMock() + mock_tracker.excuse_producer.return_value = { + "status": "excused", + "affected_reviewers": ["reviewer_code"], + } + + mock_store_inst = MagicMock() + + with ( + patch("decision_queue.get_decision_queue", return_value=mock_queue), + patch( + "peer_consensus.get_peer_consensus_tracker", + return_value=mock_tracker, + ), + patch("message_store.get_message_store", return_value=mock_store_inst), + patch("routes.signals._resolve_pipeline_phase", return_value="implement"), + ): + response, status_code = handle_consensus_excuse_producer_signal( + "issue-42", + { + "producer_role": "coder", + "reason": "Not delivering", + "decision_id": "dec-123", + }, + Path("/tmp/repo"), + ) + + assert status_code == 200 + mock_store_inst.add_message.assert_called_once() + stored_message = mock_store_inst.add_message.call_args[0][0] + assert "slice_id" not in stored_message.metadata, ( + f"Pipeline-level STATUS must omit slice_id key, got: {stored_message.metadata}" + ) + # --------------------------------------------------------------------------- # ACK version forwarding tests (#1637) diff --git a/sandbox/egg_agent_tools/handlers/brc.py b/sandbox/egg_agent_tools/handlers/brc.py index ec336e2c33..2473f1d943 100644 --- a/sandbox/egg_agent_tools/handlers/brc.py +++ b/sandbox/egg_agent_tools/handlers/brc.py @@ -14,6 +14,7 @@ get_agent_role, get_pipeline_id, orchestrator_request, + resolve_slice_id, ) from egg_agent_tools.handlers._gateway import maybe_attach_slice_id as _maybe_attach_slice_id from egg_agent_tools.handlers.errors import GatewayError, HandlerError @@ -804,9 +805,16 @@ def brc_resolve_obligation(req: dict[str, Any]) -> dict[str, Any]: "CONSENSUS_PROPOSE", "CONSENSUS_ACK", "CONSENSUS_NACK", + "CONSENSUS_WITHDRAW", "CONSENSUS_CONFIRMED", "CONSENSUS_RE_REVIEW", - "CONSENSUS_WITHDRAWN", + "CONSENSUS_OBLIGATION_RESOLVED", + "STATUS", + "HANDOFF", + "AGENT_FAILED", + "NUDGE", + "OVERSEER_ALERT", + "HEARTBEAT", } ) @@ -878,15 +886,34 @@ def brc_read_peer_artifact(req: dict[str, Any]) -> dict[str, Any]: """Read consensus history for a peer from the local brc-history log. No CLI counterpart (decision-8): reads from the local - ``.egg-state/brc-history/-.json`` file - written by ``orchestrator.routes.pipelines._write_brc_history`` - so reviewers never have to hand-grep JSON off disk. + ``.egg-state/brc-history/`` files written by + ``orchestrator.routes.pipelines._write_brc_history`` so reviewers + never have to hand-grep JSON off disk. + + File resolution mirrors the writer's per-slice partition (#2548): + + * ``phase ∈ {refine, plan, pr}`` — reads the aggregate + ``{identifier}-{phase}.json`` file. + * ``phase == "implement"`` and ``EGG_SLICE_ID`` is set — reads the + per-slice ``{identifier}-implement-{slice_id}.json`` file. If a + sibling ``{identifier}-implement-unattributed.json`` exists + (cross-cutting messages without slice scope: HEARTBEAT, + OVERSEER_ALERT, AGENT_FAILED, etc.), its records are merged into + the response and re-sorted by timestamp so reviewers see the + slice transcript and the cross-cutting context interleaved. + Pass ``include_unattributed=False`` to skip the merge. + * ``phase == "implement"`` and ``EGG_SLICE_ID`` is unset — reads + the aggregate ``{identifier}-implement.json`` file (babysit_pr + and other non-slice pipelines). Security: caller-supplied ``pipeline_id``/``issue``/``repo_path`` are ignored; the identifier and repo root are resolved server-side from ``EGG_PIPELINE_ID`` / ``EGG_ISSUE_NUMBER`` / ``EGG_REPO_PATH`` - (risk_analyst R2 + reviewer_code NACK #1). The resolved file path - is canonicalised and asserted to sit under + (risk_analyst R2 + reviewer_code NACK #1). ``EGG_SLICE_ID`` is + validated against the canonical ``slice-`` regex before being + interpolated into the filename — same defense-in-depth as the + writer-side seam (`orchestrator/routes/pipelines.py` ~line 8406). + The resolved file path is canonicalised and asserted to sit under ``/.egg-state/brc-history/``; anything else raises ``HandlerError``. ``peer_role`` must match ``[a-z0-9_-]``. @@ -900,6 +927,11 @@ def brc_read_peer_artifact(req: dict[str, Any]) -> dict[str, Any]: ``message_type``; accepts a single value or a list. limit (int): optional page size (default 50, max 500). cursor (str): opaque pagination token. + include_unattributed (bool): optional, default ``True``. When + reading a slice-scoped implement transcript, also merge + records from the sibling + ``{identifier}-implement-unattributed.json`` file. Set + ``False`` to read only the per-slice file. Response: { ok: True, phase: str, items: [...], next_cursor: str|None, @@ -954,6 +986,14 @@ def brc_read_peer_artifact(req: dict[str, Any]) -> dict[str, Any]: if limit > 500: raise HandlerError("'limit' must be <= 500") + raw_include = req.get("include_unattributed") + if raw_include is None: + include_unattributed = True + elif isinstance(raw_include, bool): + include_unattributed = raw_include + else: + raise HandlerError("'include_unattributed' must be a boolean if provided") + cursor_state = _decode_cursor(req.get("cursor")) offset = cursor_state["offset"] prior_skipped = cursor_state["skipped_malformed"] @@ -961,13 +1001,60 @@ def brc_read_peer_artifact(req: dict[str, Any]) -> dict[str, Any]: identifier = _resolve_env_identifier_for_brc_history() repo_root = Path(os.environ.get("EGG_REPO_PATH") or os.getcwd()).resolve() history_dir = (repo_root / ".egg-state" / "brc-history").resolve() - history_file = (history_dir / f"{identifier}-{phase}.json").resolve() - # Containment check: catches symlinks / .. in identifier/phase that - # escape the allowed directory even after the env-only resolution. - if not history_file.is_relative_to(history_dir): - raise HandlerError("Resolved brc-history path escapes .egg-state/brc-history/") - if not history_file.exists(): + # Mirror the writer's per-slice partition for the implement phase + # (#2548). When EGG_SLICE_ID is set and phase=="implement" we read + # the slice's transcript file and (by default) merge in the + # cross-cutting `unattributed` sibling. Other phases and non-slice + # implement runs read the aggregate file. + history_files: list[Path] = [] + if phase == "implement": + # Defense-in-depth via the public _gateway helper: resolves + # EGG_SLICE_ID, validates against the canonical `^slice-$` + # regex (same seam the orchestrator writer enforces at + # `pipelines.py` ~8406), and raises HandlerError on malformed + # values before we interpolate into the filename. Pass `{}` so + # caller-supplied `slice_id` is ignored — slice scope is an + # env-only signal here for the same cross-pipeline-read + # hardening as `_resolve_env_identifier_for_brc_history`. + slice_id_env = resolve_slice_id({}) + if slice_id_env is not None: + slice_file = (history_dir / f"{identifier}-implement-{slice_id_env}.json").resolve() + history_files.append(slice_file) + if include_unattributed: + unattr_file = (history_dir / f"{identifier}-implement-unattributed.json").resolve() + history_files.append(unattr_file) + else: + history_files.append((history_dir / f"{identifier}-{phase}.json").resolve()) + else: + history_files.append((history_dir / f"{identifier}-{phase}.json").resolve()) + + # Containment check on every resolved path: catches symlinks / .. in + # identifier/phase/slice_id that escape the allowed directory even + # after the env-only resolution and SLICE_ID_PATTERN validation. + for hf in history_files: + if not hf.is_relative_to(history_dir): + raise HandlerError("Resolved brc-history path escapes .egg-state/brc-history/") + + records: list[Any] = [] + any_existed = False + for hf in history_files: + if not hf.exists(): + continue + any_existed = True + try: + chunk = json.loads(hf.read_text()) + except (OSError, json.JSONDecodeError) as exc: + raise HandlerError( + f"Failed to read brc-history file for phase {phase!r}: {exc}" + ) from exc + if not isinstance(chunk, list): + raise HandlerError( + f"Malformed brc-history file for phase {phase!r}: expected a JSON array" + ) + records.extend(chunk) + + if not any_existed: return { "ok": True, "phase": phase, @@ -977,13 +1064,6 @@ def brc_read_peer_artifact(req: dict[str, Any]) -> dict[str, Any]: "skipped_malformed": prior_skipped, } - try: - records = json.loads(history_file.read_text()) - except (OSError, json.JSONDecodeError) as exc: - raise HandlerError(f"Failed to read brc-history file for phase {phase!r}: {exc}") from exc - if not isinstance(records, list): - raise HandlerError(f"Malformed brc-history file for phase {phase!r}: expected a JSON array") - filtered: list[dict[str, Any]] = [] skipped_malformed = 0 for rec in records: @@ -996,6 +1076,12 @@ def brc_read_peer_artifact(req: dict[str, Any]) -> dict[str, Any]: continue filtered.append(rec) + # Re-sort merged records by timestamp so the per-slice transcript + # and the unattributed sibling interleave chronologically. Records + # without a timestamp sort last, in original order (stable sort). + if len(history_files) > 1: + filtered.sort(key=lambda r: (r.get("timestamp") is None, r.get("timestamp") or "")) + total = len(filtered) total_skipped = prior_skipped + skipped_malformed if offset >= total: diff --git a/sandbox/egg_agent_tools/tools/brc.py b/sandbox/egg_agent_tools/tools/brc.py index f765ffe558..b8d72183be 100644 --- a/sandbox/egg_agent_tools/tools/brc.py +++ b/sandbox/egg_agent_tools/tools/brc.py @@ -260,7 +260,10 @@ "description": ( "Optional message_type filter; accepts a single type or a " "list (CONSENSUS_PROPOSE, CONSENSUS_ACK, CONSENSUS_NACK, " - "CONSENSUS_CONFIRMED, CONSENSUS_RE_REVIEW, CONSENSUS_WITHDRAWN)" + "CONSENSUS_WITHDRAW, CONSENSUS_CONFIRMED, " + "CONSENSUS_RE_REVIEW, CONSENSUS_OBLIGATION_RESOLVED, " + "STATUS, HANDOFF, AGENT_FAILED, NUDGE, OVERSEER_ALERT, " + "HEARTBEAT)" ), }, "limit": { @@ -272,6 +275,18 @@ "type": "string", "description": "Opaque pagination token returned by a prior call", }, + "include_unattributed": { + "type": "boolean", + "default": True, + "description": ( + "When reading a slice-scoped implement transcript " + "(EGG_SLICE_ID set + phase='implement'), also merge " + "records from the sibling " + "-implement-unattributed.json file " + "(cross-cutting messages without slice scope). " + "Default true; set false to read only the per-slice file." + ), + }, }, "required": ["phase"], "additionalProperties": False, diff --git a/scripts/file-size-allowlist.yaml b/scripts/file-size-allowlist.yaml index ab0029a198..5efff26f1e 100644 --- a/scripts/file-size-allowlist.yaml +++ b/scripts/file-size-allowlist.yaml @@ -50,3 +50,12 @@ files: issue: "2248" orchestrator/kubernetes_spawner.py: issue: "2248" + # On the egg/issue-2548/work merge target, slice-1's + # extract_pr_context_metadata_from_yaml + ParseResult.pr_context_* + # plumbing (#2548) stacks on top of #2527's validate_task_role_alignment + # additions, pushing the file to ~1,530 lines. The slice-1 branch alone + # is 1,388 lines (under the 1,500-line hard cap), but the merged + # work-branch state breaches the cap. Allowlisting under #2548 so the + # BRC implement-phase lint passes; decompose under #2569. + shared/egg_contracts/plan_parser.py: + issue: "2548" diff --git a/shared/egg_contracts/models.py b/shared/egg_contracts/models.py index ee9683e498..59b479631d 100644 --- a/shared/egg_contracts/models.py +++ b/shared/egg_contracts/models.py @@ -369,7 +369,23 @@ class DeferredAction(EggContractBaseModel): class PRMetadata(EggContractBaseModel): - """Planner-generated PR metadata: title, description, test plan, and manual steps.""" + """Planner-generated PR metadata: title, description, test plan, and manual steps. + + Schema 1.1 (#2548) adds four optional ``context_*`` fields used by the + new dedicated context-PR mechanism. The context PR sits at the root of + the slice stack and carries the refine/plan analysis docs and BRC + consensus history, so that strategic narrative reaches ``main`` even + when slice PRs cascade-merge through the work branch. + + * ``context_title`` / ``context_description`` are populated by the + planner when it wants the context PR framed differently from the + slice PRs (e.g. "Strategic plan for #N" vs the slice's "Implement + …"). When omitted the orchestrator falls back to ``title`` / + ``description``. + * ``context_branch`` / ``context_pr_number`` are populated by the + orchestrator after the context branch is created and the context + PR is opened — planners must NOT emit these fields. + """ title: str = Field(..., min_length=1, description="PR title (recommended max 70 chars)") description: str = Field(default="", description="PR description/body") @@ -381,6 +397,41 @@ class PRMetadata(EggContractBaseModel): default="", description="Manual pre/post-merge steps (migrations, config changes, etc.)", ) + # ------------------------------------------------------------------ + # #2548 — context-PR fields (schema 1.1). + # ------------------------------------------------------------------ + context_title: str | None = Field( + default=None, + description=( + "Optional title for the dedicated context PR (#2548). Lets the " + "context PR be framed differently from slice PRs (e.g. " + "'Strategic plan for #N'). Falls back to ``title`` when None." + ), + ) + context_description: str | None = Field( + default=None, + description=( + "Optional body for the dedicated context PR (#2548). Falls " + "back to ``description`` when None." + ), + ) + context_branch: str | None = Field( + default=None, + description=( + "Branch name ``egg//context`` once the orchestrator " + "has created it (#2548). Populated by the orchestrator hook that " + "runs after plan_gate; planners must NOT emit this field." + ), + ) + context_pr_number: int | None = Field( + default=None, + ge=1, + description=( + "GitHub PR number once the context PR has been opened (#2548). " + "Populated by the orchestrator; planners must NOT emit this field. " + "Constrained to >=1 because GitHub PR numbers are positive." + ), + ) deferred_actions: list[DeferredAction] = Field( default_factory=list, description=( @@ -613,7 +664,16 @@ class Contract(EggContractBaseModel): """The complete SDLC contract.""" schemaVersion: str = Field( # noqa: N815 - default="1.0", pattern=r"^[0-9]+\.[0-9]+$", description="Schema version" + default="1.1", + pattern=r"^[0-9]+\.[0-9]+$", + description=( + "Schema version. Bumped to ``1.1`` in #2548 to track the addition " + "of the optional ``pr.context_*`` fields. Pre-1.1 contracts load " + "transparently — the new fields default to None — and are " + "promoted to ``1.1`` whenever they are loaded into the model; " + "the new value is then persisted on the next save. See " + "``_migrate_schema_version_to_1_1``." + ), ) issue: IssueInfo | None = Field(default=None, description="Issue metadata") pipeline_id: str | None = Field( @@ -748,6 +808,38 @@ def _migrate_phases_to_slices(cls, data: Any, handler: Any) -> Contract: instance._legacy_phases = legacy_phases return instance + @model_validator(mode="after") + def _migrate_schema_version_to_1_1(self) -> Contract: + """Promote pre-1.1 contracts to schema ``1.1`` (#2548). + + The ``1.0`` → ``1.1`` bump is purely additive — it documents the + arrival of the ``pr.context_*`` fields, which are all optional + and default to ``None``. Pre-1.1 JSON loads cleanly without the + fields; we just stamp the new version so downstream tooling + (audit, status renderers) sees a consistent value. + + We deliberately do NOT touch versions outside ``{1.0}`` so that + an unrelated future bump (e.g. a hypothetical ``2.0``) does not + get silently downgraded back to ``1.1``. + + This validator runs in ``mode="after"``, so the bump happens at + every load — including in-memory ``Contract.model_validate(...)`` + calls — not lazily on the next save. The mutation is idempotent + (the conditional only fires when the value is exactly ``"1.0"``) + so re-running the validator on an already-migrated contract is + a no-op. + + Note: the bump is silent — no ``AuditEntry`` is appended. + Operators inspecting the audit trail after a 1.0 → 1.1 + promotion will not see a record of the change. Schema bumps + are uncommon enough that this is intentional; if a future + bump warrants audit visibility, a dedicated audit hook on + the migration validator is the right place to add it. + """ + if self.schemaVersion == "1.0": + self.schemaVersion = "1.1" + return self + @model_validator(mode="after") def _require_issue_or_pipeline_id(self) -> Contract: """At least one of issue or pipeline_id must be set.""" diff --git a/shared/egg_contracts/plan_parser.py b/shared/egg_contracts/plan_parser.py index 8d8eefd653..e2a298102e 100644 --- a/shared/egg_contracts/plan_parser.py +++ b/shared/egg_contracts/plan_parser.py @@ -205,6 +205,11 @@ class ParseResult: pr_description: str | None = None pr_test_plan: str | None = None pr_manual_steps: str | None = None + # #2548 — context-PR fields. Optional; default to None when the + # planner omits them (the orchestrator falls back to ``pr_title`` / + # ``pr_description`` for the context-PR framing in that case). + pr_context_title: str | None = None + pr_context_description: str | None = None def to_contract_phases(self) -> list[Slice]: """Backward-compat alias for ``to_contract_slices`` (#2137). @@ -915,6 +920,95 @@ def extract_pr_metadata_from_yaml( return pr_title, pr_description, pr_test_plan, pr_manual_steps, warnings +def extract_pr_context_metadata_from_yaml( + yaml_data: dict[str, Any] | None, +) -> tuple[str | None, str | None, list[ParseWarning]]: + """Extract optional context-PR framing fields from the ``pr:`` block. + + Added in #2548 alongside the dedicated context-PR mechanism. The + planner can emit ``pr.context_title`` and ``pr.context_description`` + to frame the strategic-plan PR differently from the slice PRs (e.g. + "Strategic plan for #N" vs "Implement …"). Both keys are optional — + when omitted the orchestrator falls back to ``pr.title`` / + ``pr.description`` for the context PR's framing. + + The orchestrator-populated fields ``pr.context_branch`` and + ``pr.context_pr_number`` are intentionally NOT extracted here: + planners must not emit them, and a future plan-reviewer may emit a + warning if they do appear in a planner-authored YAML. We currently + accept-and-ignore unknown keys to stay forward-compatible with + minor planner-prompt drift. + + Args: + yaml_data: Parsed YAML data from a yaml-tasks code fence. + + Returns: + Tuple of (context_title, context_description, warnings). Each + of the two value slots is ``None`` when absent or malformed. + """ + warnings: list[ParseWarning] = [] + + if yaml_data is None: + return None, None, warnings + + pr_data = yaml_data.get("pr") + if not isinstance(pr_data, dict): + # ``extract_pr_metadata_from_yaml`` already produces a structural + # warning for the non-dict case; do not duplicate it here. + return None, None, warnings + + raw_title = pr_data.get("context_title") + raw_description = pr_data.get("context_description") + + context_title: str | None = None + if raw_title is not None: + if not isinstance(raw_title, str): + warnings.append( + ParseWarning( + line_number=None, + message=( + f"'pr.context_title' must be a string, got {type(raw_title).__name__}" + ), + context="context-PR title will fall back to pr.title", + ) + ) + else: + stripped = raw_title.strip() + context_title = stripped if stripped else None + + # Normalize description to a non-empty string, then collapse the + # absent/empty case to ``None`` so the orchestrator can reliably + # detect "fall back to pr.description" semantics. The existing + # ``pr.description`` field defaults to "" because PRMetadata + # requires a string body, but ``context_description`` is Optional + # at the model layer. + # + # Symmetric with the ``context_title`` branch above: warn loudly + # when the planner emitted a non-string scalar (e.g. an int or a + # nested mapping). Without this check ``_normalize_optional_string`` + # would silently coerce via ``str(value)`` and a planner-prompt + # regression that started emitting structured values would land + # quietly on the contract. + context_description: str | None = None + if raw_description is not None: + if not isinstance(raw_description, str): + warnings.append( + ParseWarning( + line_number=None, + message=( + f"'pr.context_description' must be a string, got " + f"{type(raw_description).__name__}" + ), + context="context-PR description will fall back to pr.description", + ) + ) + else: + normalized = _normalize_optional_string(raw_description) + context_description = normalized if normalized else None + + return context_title, context_description, warnings + + def parse_phases_from_markdown(content: str) -> list[ParsedPhase]: """ Parse phase sections from markdown content. @@ -1101,6 +1195,14 @@ def parse_plan(content: str) -> ParseResult: ) warnings.extend(pr_warnings) + # Extract optional context-PR framing fields (#2548). These are + # captured separately to keep ``extract_pr_metadata_from_yaml``'s + # 5-tuple signature stable for existing callers. + pr_context_title, pr_context_description, pr_context_warnings = ( + extract_pr_context_metadata_from_yaml(yaml_data) + ) + warnings.extend(pr_context_warnings) + return ParseResult( success=True, phases=phases, @@ -1110,6 +1212,8 @@ def parse_plan(content: str) -> ParseResult: pr_description=pr_description, pr_test_plan=pr_test_plan, pr_manual_steps=pr_manual_steps, + pr_context_title=pr_context_title, + pr_context_description=pr_context_description, ) diff --git a/tests/sandbox/egg_agent_tools/test_handlers_brc.py b/tests/sandbox/egg_agent_tools/test_handlers_brc.py index 95ad7d1ca6..af11c2ebc1 100644 --- a/tests/sandbox/egg_agent_tools/test_handlers_brc.py +++ b/tests/sandbox/egg_agent_tools/test_handlers_brc.py @@ -1070,6 +1070,209 @@ def test_docstring_mentions_no_cli_rationale(self): lower = doc.lower() assert "no cli" in lower or "no-cli" in lower + # ---- Slice-aware implement-phase reads (#2548 follow-up) ----------- + + def _make_slice_history_file(self, root, identifier: str, slice_id: str, records: list[dict]): + """Write a per-slice implement-phase brc-history file.""" + dir_ = root / ".egg-state" / "brc-history" + dir_.mkdir(parents=True, exist_ok=True) + path = dir_ / f"{identifier}-implement-{slice_id}.json" + path.write_text(json.dumps(records)) + return path + + def _make_unattributed_history_file(self, root, identifier: str, records: list[dict]): + """Write the cross-cutting `unattributed` sibling file.""" + dir_ = root / ".egg-state" / "brc-history" + dir_.mkdir(parents=True, exist_ok=True) + path = dir_ / f"{identifier}-implement-unattributed.json" + path.write_text(json.dumps(records)) + return path + + def test_slice_scoped_implement_reads_per_slice_file(self, tmp_path, monkeypatch): + """When EGG_SLICE_ID is set and phase=='implement', the handler + reads {identifier}-implement-{slice_id}.json — not the legacy + aggregate file (which the writer no longer produces in slice mode).""" + self._set_env(monkeypatch, tmp_path) + monkeypatch.setenv("EGG_SLICE_ID", "slice-1") + # Slice-1 has its own transcript; no aggregate file exists. + self._make_slice_history_file( + tmp_path, + "1917", + "slice-1", + _records(("coder", "CONSENSUS_PROPOSE"), ("reviewer_code", "CONSENSUS_ACK")), + ) + resp = brc.brc_read_peer_artifact({"phase": "implement"}) + assert resp["ok"] is True + assert len(resp["items"]) == 2 + assert resp["total_available"] == 2 + + def test_slice_scoped_implement_no_aggregate_fallback(self, tmp_path, monkeypatch): + """A slice-scoped agent must NOT silently dead-end into the + aggregate file even if a stale {identifier}-implement.json + happens to be on disk — the slice file is the canonical path.""" + self._set_env(monkeypatch, tmp_path) + monkeypatch.setenv("EGG_SLICE_ID", "slice-2") + # Aggregate file from a previous run (or another tool) — must + # be ignored when slice-scoped. + _make_history_file( + tmp_path, + "1917", + "implement", + _records(("coder", "CONSENSUS_PROPOSE")), + ) + # Slice-2's per-slice file does not exist. + resp = brc.brc_read_peer_artifact({"phase": "implement"}) + assert resp["items"] == [] + assert resp["total_available"] == 0 + + def test_slice_scoped_implement_merges_unattributed_sibling(self, tmp_path, monkeypatch): + """By default the slice transcript is merged with the cross- + cutting `unattributed` sibling so reviewers see OVERSEER_ALERT, + AGENT_FAILED, etc. interleaved with their slice's CONSENSUS_*.""" + self._set_env(monkeypatch, tmp_path) + monkeypatch.setenv("EGG_SLICE_ID", "slice-1") + # Slice-1 records and unattributed records have distinct + # timestamps so we can assert chronological interleave. + slice_recs = [ + { + "id": "s1", + "from_role": "coder", + "message_type": "CONSENSUS_PROPOSE", + "body": "b1", + "timestamp": "2026-04-24T00:00:01Z", + }, + { + "id": "s2", + "from_role": "reviewer_code", + "message_type": "CONSENSUS_ACK", + "body": "b2", + "timestamp": "2026-04-24T00:00:03Z", + }, + ] + unattr_recs = [ + { + "id": "u1", + "from_role": "overseer", + "message_type": "OVERSEER_ALERT", + "body": "u1", + "timestamp": "2026-04-24T00:00:02Z", + }, + ] + self._make_slice_history_file(tmp_path, "1917", "slice-1", slice_recs) + self._make_unattributed_history_file(tmp_path, "1917", unattr_recs) + resp = brc.brc_read_peer_artifact({"phase": "implement"}) + assert len(resp["items"]) == 3 + # Re-sorted by timestamp: s1 → u1 → s2. + assert [r["id"] for r in resp["items"]] == ["s1", "u1", "s2"] + + def test_slice_scoped_implement_skip_unattributed_on_request(self, tmp_path, monkeypatch): + """``include_unattributed=False`` reads only the per-slice file.""" + self._set_env(monkeypatch, tmp_path) + monkeypatch.setenv("EGG_SLICE_ID", "slice-1") + self._make_slice_history_file( + tmp_path, + "1917", + "slice-1", + _records(("coder", "CONSENSUS_PROPOSE")), + ) + self._make_unattributed_history_file( + tmp_path, + "1917", + _records(("overseer", "OVERSEER_ALERT")), + ) + resp = brc.brc_read_peer_artifact({"phase": "implement", "include_unattributed": False}) + assert len(resp["items"]) == 1 + assert resp["items"][0]["from_role"] == "coder" + + def test_slice_scoped_implement_unattributed_only_present(self, tmp_path, monkeypatch): + """If a slice never produced any CONSENSUS_* but unattributed + traffic exists, the handler still returns the unattributed + records rather than an empty response.""" + self._set_env(monkeypatch, tmp_path) + monkeypatch.setenv("EGG_SLICE_ID", "slice-1") + self._make_unattributed_history_file( + tmp_path, + "1917", + _records(("overseer", "OVERSEER_ALERT")), + ) + resp = brc.brc_read_peer_artifact({"phase": "implement"}) + assert len(resp["items"]) == 1 + assert resp["items"][0]["from_role"] == "overseer" + + def test_pipeline_level_implement_reads_aggregate(self, tmp_path, monkeypatch): + """Without EGG_SLICE_ID the handler reads the aggregate + ``{identifier}-implement.json`` file (babysit_pr / non-slice + runs are unaffected by the slice-aware switch).""" + self._set_env(monkeypatch, tmp_path) + monkeypatch.delenv("EGG_SLICE_ID", raising=False) + _make_history_file( + tmp_path, + "1917", + "implement", + _records(("coder", "CONSENSUS_PROPOSE")), + ) + resp = brc.brc_read_peer_artifact({"phase": "implement"}) + assert len(resp["items"]) == 1 + + def test_invalid_slice_id_env_rejected(self, tmp_path, monkeypatch): + """Defense-in-depth: a malformed EGG_SLICE_ID must not be + interpolated into the filename. Same regex the writer enforces + at the orchestrator seam.""" + self._set_env(monkeypatch, tmp_path) + monkeypatch.setenv("EGG_SLICE_ID", "../etc/passwd") + with pytest.raises(HandlerError) as exc: + brc.brc_read_peer_artifact({"phase": "implement"}) + assert "slice" in str(exc.value).lower() + + def test_invalid_include_unattributed_rejected(self, tmp_path, monkeypatch): + """``include_unattributed`` must be a bool when supplied.""" + self._set_env(monkeypatch, tmp_path) + with pytest.raises(HandlerError): + brc.brc_read_peer_artifact({"phase": "plan", "include_unattributed": "yes"}) + + def test_slice_scoped_non_implement_reads_aggregate(self, tmp_path, monkeypatch): + """EGG_SLICE_ID only switches the implement phase. Refine/plan/pr + always read the aggregate file — slice-aware writers never + partition those phases.""" + self._set_env(monkeypatch, tmp_path) + monkeypatch.setenv("EGG_SLICE_ID", "slice-1") + _make_history_file( + tmp_path, + "1917", + "plan", + _records(("coder", "CONSENSUS_PROPOSE")), + ) + resp = brc.brc_read_peer_artifact({"phase": "plan"}) + assert len(resp["items"]) == 1 + + def test_filter_by_message_type_overseer_alert_in_unattributed(self, tmp_path, monkeypatch): + """Reviewers can scan cross-cutting alerts in their slice's + transcript by filtering on a non-CONSENSUS_* type. Regression + guard: the handler's ``_BRC_HISTORY_TYPES`` whitelist must include + the same non-CONSENSUS_* types the writer emits to the + ``unattributed`` sibling, otherwise this raises ``Unknown + message_type(s)`` even though matching records exist on disk.""" + self._set_env(monkeypatch, tmp_path) + monkeypatch.setenv("EGG_SLICE_ID", "slice-1") + self._make_slice_history_file( + tmp_path, + "1917", + "slice-1", + _records(("coder", "CONSENSUS_PROPOSE")), + ) + self._make_unattributed_history_file( + tmp_path, + "1917", + _records( + ("overseer", "OVERSEER_ALERT"), + ("system", "HEARTBEAT"), + ), + ) + resp = brc.brc_read_peer_artifact({"phase": "implement", "message_type": "OVERSEER_ALERT"}) + assert len(resp["items"]) == 1 + assert resp["items"][0]["message_type"] == "OVERSEER_ALERT" + assert resp["items"][0]["from_role"] == "overseer" + class TestBrcPipelineIdValidation: """Pipeline IDs are interpolated into URL paths — format validation @@ -1218,3 +1421,54 @@ def test_orchestrator_failure_surfaces(self): "producer_role": "coder", } ) + + +class TestBrcHistoryTypesDriftGuard: + """Regression guard locking writer/reader symmetry for the *full* BRC + history type set. + + The writer (``orchestrator.routes.pipelines.BRC_HISTORY_TYPES``) and + the reader-side filter whitelist + (``egg_agent_tools.handlers.brc._BRC_HISTORY_TYPES``) must list the + same types: any type the writer emits must be filterable by the + reader, and any type the reader accepts must be one the writer + actually produces. The single-type regression test added in #2548 + locks ``OVERSEER_ALERT`` only — this test parses the writer-side + literal out of the orchestrator source and asserts membership + equality, so future drift on either side surfaces as a test failure. + + The handler module deliberately does *not* import the orchestrator + package (which pulls fastapi); the regex-extraction approach + preserves that boundary while still locking the contract. + """ + + def test_handler_whitelist_matches_writer_set(self): + import re + + pipelines_path = ROOT / "orchestrator" / "routes" / "pipelines.py" + source = pipelines_path.read_text() + match = re.search( + r"^BRC_HISTORY_TYPES\s*=\s*frozenset\s*\(\s*\{(?P.*?)\}\s*\)", + source, + re.MULTILINE | re.DOTALL, + ) + assert match is not None, ( + "Could not locate ``BRC_HISTORY_TYPES = frozenset({...})`` " + f"literal in {pipelines_path}; the drift-guard regex needs " + "updating to track the new shape." + ) + writer_types = frozenset(re.findall(r'"([A-Z_]+)"', match.group("body"))) + assert writer_types, ( + "Parsed ``BRC_HISTORY_TYPES`` literal is empty — the regex " + "did not capture any type names; check the literal shape." + ) + handler_types = brc._BRC_HISTORY_TYPES + assert handler_types == writer_types, ( + "Sandbox handler whitelist drifted from orchestrator writer " + "set. " + f"Handler-only (reader accepts but writer never emits): " + f"{sorted(handler_types - writer_types)}; " + f"Writer-only (writer emits but reader rejects): " + f"{sorted(writer_types - handler_types)}. " + "Update one or both to keep the partition symmetric." + ) diff --git a/tests/shared/egg_contracts/test_models.py b/tests/shared/egg_contracts/test_models.py index 6191993d1d..e5d71dc985 100644 --- a/tests/shared/egg_contracts/test_models.py +++ b/tests/shared/egg_contracts/test_models.py @@ -296,7 +296,11 @@ def test_minimal_contract(self): url="https://github.com/owner/repo/issues/133", ), ) - assert contract.schemaVersion == "1.0" + # schemaVersion default bumped from "1.0" to "1.1" in #2548 to + # track the addition of the optional ``pr.context_*`` fields. + # See ``test_pr_metadata.py::test_default_schemaversion_is_1_1`` + # for the canonical pin. + assert contract.schemaVersion == "1.1" assert contract.issue.number == 133 assert contract.current_phase == PipelinePhase.REFINE assert contract.phases == [] diff --git a/tests/shared/egg_contracts/test_pr_metadata.py b/tests/shared/egg_contracts/test_pr_metadata.py new file mode 100644 index 0000000000..e83957f6b6 --- /dev/null +++ b/tests/shared/egg_contracts/test_pr_metadata.py @@ -0,0 +1,906 @@ +"""Tests for PRMetadata.context_* fields + schemaVersion 1.0→1.1 migration. + +Added in #2548 (slice-1, task-1-2). Covers the four new optional +``PRMetadata.context_*`` fields the planner emits for the doc-only +context PR (issue #2548) and the load-time migration shim that +back-fills the new fields as ``None`` when an on-disk contract +written with ``schemaVersion="1.0"`` is loaded into the post-rename +``schemaVersion="1.1"`` model. + +The acceptance criteria from the plan: + +* Round-trip a ``PRMetadata`` with all four context fields populated. +* Round-trip a ``PRMetadata`` with all four context fields omitted + (defaults must be ``None``). +* Round-trip a contract serialised with ``schemaVersion="1.0"`` and + no context fields, and confirm migration populates the defaults. +* Confirm ``context_pr_number`` validation: ``0`` and negative values + are rejected (``ge=1``); positive ``int`` values round-trip. + +The tests live at ``tests/shared/egg_contracts/`` because that is the +pytest collection root in the project's ``[tool.pytest.ini_options] +testpaths`` (the in-package path ``shared/egg_contracts/tests/`` is NOT +in ``testpaths`` / ``scripts/select_tests/_constants.TEST_ROOT_DIRS`` +and would not be discovered by ``make test`` or ``make test-all``). +The plan task-1-2 ``files_affected`` referenced the in-package path +but the canonical location of every other ``PRMetadata`` test +(``tests/shared/egg_contracts/test_models.py``) is here. +""" + +from __future__ import annotations + +from typing import Any + +import pytest +from egg_contracts.models import ( + Contract, + IssueInfo, + PRMetadata, +) +from pydantic import ValidationError + + +def _minimal_contract_payload(*, schema_version: str = "1.0") -> dict[str, Any]: + """Return a minimal contract payload at the requested schema version. + + The contract has an ``IssueInfo`` and a single ``PRMetadata`` with + the legacy required field (``title``) populated and no ``context_*`` + keys set. Used to drive the migration round-trip in + :func:`test_contract_schemaversion_1_0_loads_with_context_defaults_none`. + + The return type is ``dict[str, Any]`` rather than the more precise + ``dict[str, dict[str, str | list[Any]]]`` because callers extend + ``payload["pr"]`` with arbitrary new keys (``context_title``, + ``context_pr_number``, ``deferred_actions`` entries) — pinning a + narrower inner type only forces casts at every mutation site. + """ + return { + "schemaVersion": schema_version, + "issue": { + "number": 2548, + "title": "context PR + per-slice BRC history", + "url": "https://example.com/i/2548", + }, + "current_phase": "refine", + "slices": [], + "decisions": [], + "audit_log": [], + "pr": { + "title": "Add context PR + per-slice BRC history", + "description": "", + "test_plan": "", + "manual_steps": "", + "deferred_actions": [], + }, + } + + +class TestPRMetadataContextFields: + """The four new optional ``context_*`` fields on ``PRMetadata`` (#2548).""" + + def test_context_fields_default_to_none(self): + """Constructing without the new keys must leave them ``None``. + + Backwards-compat: a planner emitting only the legacy fields + (``title`` / ``description`` / ``test_plan`` / ``manual_steps``) + must produce a ``PRMetadata`` whose ``context_*`` fields are all + ``None`` — that is what allows ``contract.pr.context_branch or + pipeline_branch`` to fall back cleanly in slice-4. + """ + pr = PRMetadata(title="Add context PR + per-slice BRC history") + assert pr.context_title is None + assert pr.context_description is None + assert pr.context_branch is None + assert pr.context_pr_number is None + + def test_context_fields_populated_round_trip(self): + """All four ``context_*`` fields populated must round-trip via JSON. + + Asserts construction → ``model_dump()`` → ``model_validate()`` + is value-preserving for every field the orchestrator persists + (``context_branch`` and ``context_pr_number``) and every field + the planner emits (``context_title`` and ``context_description``). + """ + pr = PRMetadata( + title="Add context PR + per-slice BRC history", + description="Per-slice BRC history + context PR work.", + context_title="Strategic plan for #2548", + context_description="Refine + plan artifacts for issue 2548.", + context_branch="egg/issue-2548/context", + context_pr_number=4242, + ) + dumped = pr.model_dump() + assert dumped["context_title"] == "Strategic plan for #2548" + assert dumped["context_description"] == "Refine + plan artifacts for issue 2548." + assert dumped["context_branch"] == "egg/issue-2548/context" + assert dumped["context_pr_number"] == 4242 + + round_trip = PRMetadata.model_validate(dumped) + assert round_trip.context_title == "Strategic plan for #2548" + assert round_trip.context_description == "Refine + plan artifacts for issue 2548." + assert round_trip.context_branch == "egg/issue-2548/context" + assert round_trip.context_pr_number == 4242 + + def test_context_fields_omitted_round_trip(self): + """Omitting the new keys at construction must round-trip as ``None``. + + Mirror of ``test_context_fields_default_to_none`` but at the + JSON-round-trip boundary — confirms ``model_dump()`` does not + synthesise spurious values and ``model_validate()`` accepts the + dump as-is. + """ + pr = PRMetadata(title="Plain PR — no context fields") + dumped = pr.model_dump() + assert dumped["context_title"] is None + assert dumped["context_description"] is None + assert dumped["context_branch"] is None + assert dumped["context_pr_number"] is None + + round_trip = PRMetadata.model_validate(dumped) + assert round_trip.context_title is None + assert round_trip.context_description is None + assert round_trip.context_branch is None + assert round_trip.context_pr_number is None + + +class TestPRMetadataContextPRNumberValidator: + """``context_pr_number`` must only accept positive integers (``ge=1``). + + Mirrors the validation already on ``IssueInfo.number`` — a GitHub PR + number is always a positive integer; ``0`` and negatives indicate a + bug somewhere upstream and should be surfaced loudly. + """ + + def test_positive_pr_number_accepted(self): + pr = PRMetadata(title="t", context_pr_number=1) + assert pr.context_pr_number == 1 + pr = PRMetadata(title="t", context_pr_number=999_999) + assert pr.context_pr_number == 999_999 + + def test_zero_pr_number_rejected(self): + with pytest.raises(ValidationError): + PRMetadata(title="t", context_pr_number=0) + + def test_negative_pr_number_rejected(self): + with pytest.raises(ValidationError): + PRMetadata(title="t", context_pr_number=-1) + + def test_none_pr_number_accepted(self): + """``None`` is the sentinel for 'not yet opened' and must remain valid.""" + pr = PRMetadata(title="t", context_pr_number=None) + assert pr.context_pr_number is None + + def test_pr_number_validator_re_runs_on_assignment(self): + """``validate_assignment=True`` makes ``setattr`` re-run validation. + + Regression for the shared ``EggContractBaseModel`` config (#2490). + Setting ``context_pr_number`` to ``0`` after construction must + raise — without this guard a buggy orchestrator path could + smuggle a 0 onto a previously-valid PRMetadata. + """ + pr = PRMetadata(title="t", context_pr_number=10) + with pytest.raises(ValidationError): + pr.context_pr_number = 0 + # Original value unchanged after the failed assignment. + assert pr.context_pr_number == 10 + + +class TestPRMetadataSchemaVersionMigration: + """A ``schemaVersion=1.0`` contract must load cleanly into the 1.1 model. + + The migration shim is on ``Contract`` (model-level), not on + ``PRMetadata`` directly, but the observable behavior we lock down + here is at the ``Contract.pr.context_*`` level: a pre-#2548 contract + on disk has no ``context_*`` keys; loading it into the post-#2548 + model must: + + * succeed (no ``ValidationError``), + * leave ``context_title`` / ``context_description`` / ``context_branch`` + / ``context_pr_number`` defaulted to ``None``, + * produce a contract whose ``schemaVersion`` is the post-migration + string (``"1.1"`` per the plan). + """ + + def test_legacy_1_0_payload_loads_without_context_keys(self): + """A 1.0 payload missing the four keys parses and defaults to ``None``.""" + payload = _minimal_contract_payload(schema_version="1.0") + contract = Contract.model_validate(payload) + + assert contract.pr is not None + assert contract.pr.context_title is None + assert contract.pr.context_description is None + assert contract.pr.context_branch is None + assert contract.pr.context_pr_number is None + + def test_legacy_1_0_payload_round_trip_preserves_defaults(self): + """Load → dump → reload must not synthesise spurious context values.""" + payload = _minimal_contract_payload(schema_version="1.0") + first = Contract.model_validate(payload) + dumped = first.model_dump() + second = Contract.model_validate(dumped) + + assert second.pr is not None + assert second.pr.context_title is None + assert second.pr.context_description is None + assert second.pr.context_branch is None + assert second.pr.context_pr_number is None + + def test_default_schemaversion_is_1_1(self): + """Brand-new ``Contract`` defaults the schemaVersion to ``1.1``. + + The plan bumps the default from ``"1.0"`` to ``"1.1"``. This + test pins that default so a future revert is caught loudly. + """ + contract = Contract( + issue=IssueInfo( + number=1, + title="t", + url="https://github.com/o/r/issues/1", + ) + ) + assert contract.schemaVersion == "1.1" + + def test_explicit_1_1_payload_loads_with_context_fields(self): + """A 1.1 payload with all context fields populated round-trips.""" + payload = _minimal_contract_payload(schema_version="1.1") + payload["pr"]["context_title"] = "Strategic plan for #2548" + payload["pr"]["context_description"] = "Refine + plan artifacts." + payload["pr"]["context_branch"] = "egg/issue-2548/context" + payload["pr"]["context_pr_number"] = 4242 + contract = Contract.model_validate(payload) + + assert contract.pr is not None + assert contract.pr.context_title == "Strategic plan for #2548" + assert contract.pr.context_description == "Refine + plan artifacts." + assert contract.pr.context_branch == "egg/issue-2548/context" + assert contract.pr.context_pr_number == 4242 + + def test_legacy_1_0_payload_does_not_lose_legacy_pr_fields(self): + """Migration must not drop any legacy ``PRMetadata`` field on the way in. + + Adversarial regression: a too-eager migration that rebuilt + ``PRMetadata`` from scratch could lose ``deferred_actions`` or + ``manual_steps``. Pin the legacy fields explicitly. + """ + payload = _minimal_contract_payload(schema_version="1.0") + payload["pr"]["description"] = "legacy description" + payload["pr"]["test_plan"] = "legacy test plan" + payload["pr"]["manual_steps"] = "legacy manual steps" + payload["pr"]["deferred_actions"] = [ + { + "reviewer": "reviewer_code", + "condition": "must rename foo → bar before merge", + "resolved_in_diff": "", + } + ] + contract = Contract.model_validate(payload) + + assert contract.pr is not None + assert contract.pr.description == "legacy description" + assert contract.pr.test_plan == "legacy test plan" + assert contract.pr.manual_steps == "legacy manual steps" + assert len(contract.pr.deferred_actions) == 1 + assert contract.pr.deferred_actions[0].condition == "must rename foo → bar before merge" + + def test_legacy_1_0_promotes_schemaversion_to_1_1(self): + """Loading a 1.0 payload must promote the version to 1.1 on the loaded model. + + The plan calls for "promotion" semantics — pre-#2548 contracts + on disk are bumped to 1.1 when loaded into the new model so + downstream tooling sees a consistent value. This pins the + bump direction. + """ + payload = _minimal_contract_payload(schema_version="1.0") + contract = Contract.model_validate(payload) + assert contract.schemaVersion == "1.1" + + def test_legacy_1_0_round_trip_persists_at_1_1(self): + """After the 1.0→1.1 promotion, dump→reload must keep the version at 1.1. + + Adversarial: a faulty migration that lived on the *input* path + (e.g. wrap-mode mutation of incoming dict) could re-trigger on + the second load and silently re-bump or downgrade. The + canonical post-migration version must be stable across an + arbitrary number of round-trips. + """ + payload = _minimal_contract_payload(schema_version="1.0") + first = Contract.model_validate(payload) + assert first.schemaVersion == "1.1" + + dumped = first.model_dump() + assert dumped["schemaVersion"] == "1.1" + + second = Contract.model_validate(dumped) + assert second.schemaVersion == "1.1" + + # Third round-trip — really pin idempotency. + third = Contract.model_validate(second.model_dump()) + assert third.schemaVersion == "1.1" + + def test_unrecognized_schemaversion_not_silently_downgraded(self): + """A schemaVersion outside the migration set must NOT be rewritten. + + Adversarial: the migration shim must be selective. A future + ``2.0`` (or even an in-between ``1.2``) loading on an old + binary should keep its declared version, not get silently + downgraded to ``1.1``. The plan explicitly calls this out: + "We deliberately do NOT touch versions outside ``{1.0}``". + """ + payload = _minimal_contract_payload(schema_version="1.2") + contract = Contract.model_validate(payload) + assert contract.schemaVersion == "1.2" + + payload_v2 = _minimal_contract_payload(schema_version="2.0") + contract_v2 = Contract.model_validate(payload_v2) + assert contract_v2.schemaVersion == "2.0" + + +class TestPRMetadataContextEmptyStringSemantics: + """Empty / whitespace strings are accepted at the model layer. + + The orchestrator hook computes ``contract.pr.context_title or + contract.pr.title`` to pick the framing for the context PR — both + ``None`` and ``""`` fall back via Python truthiness, so the model + deliberately does NOT enforce a min_length on the context-string + fields. These tests pin model-layer permissiveness so a future + ``min_length=1`` regression is caught by the test suite. + + Note: the planner path (``extract_pr_context_metadata_from_yaml``) + collapses whitespace-only / empty scalars to ``None`` before they + reach the model — see + ``test_extract_normalises_whitespace_to_none``. So in practice + only hand-edited or migrated payloads can produce a ``PRMetadata`` + with ``context_description == ""``; the model layer keeps that + door open by design. + """ + + def test_empty_context_title_accepted(self): + pr = PRMetadata(title="t", context_title="") + assert pr.context_title == "" + + def test_empty_context_description_accepted(self): + pr = PRMetadata(title="t", context_description="") + assert pr.context_description == "" + + def test_empty_context_branch_accepted(self): + # ``context_branch`` carries a git ref name; an empty string is + # not a valid ref, but the model layer is permissive — the + # orchestrator gateway primitive validates the ref shape when + # it actually creates the branch (slice-3). + pr = PRMetadata(title="t", context_branch="") + assert pr.context_branch == "" + + def test_or_fallback_works_with_none_and_empty(self): + """Mirror of the orchestrator hook's runtime fallback expression. + + ``context_title or title`` must yield ``title`` for both ``None`` + and ``""``. If a future commit tightens the model to reject + ``""`` this test fails loudly because the orchestrator's + fallback semantics depend on this dual treatment. + """ + pr_none = PRMetadata(title="fallback-title") + assert (pr_none.context_title or pr_none.title) == "fallback-title" + + pr_empty = PRMetadata(title="fallback-title", context_title="") + assert (pr_empty.context_title or pr_empty.title) == "fallback-title" + + pr_set = PRMetadata(title="fallback-title", context_title="explicit-context") + assert (pr_set.context_title or pr_set.title) == "explicit-context" + + +class TestPlanParserContextFieldExtraction: + """End-to-end tests for the planner-emitted ``pr.context_*`` keys. + + Task-1-3's acceptance criteria require that planner-emitted + YAML containing ``context_title:`` and ``context_description:`` is + parsed without error and the values land on ``contract.pr.context_*``; + omitting the keys leaves them as ``None``. Live in this file + because they exercise the same surface (``PRMetadata.context_*``) + that task-1-2 owns; without these tests a regression in + ``extract_pr_context_metadata_from_yaml`` could silently drop + planner-emitted keys without breaking the model-level tests above. + """ + + @staticmethod + def _make_yaml( + *, + with_context_title: bool = False, + with_context_description: bool = False, + title_value: str = "Strategic plan for #2548", + description_value: str = "Refine + plan artifacts.", + ) -> dict[str, Any]: + """Build a yaml-tasks dict, optionally with the new context keys.""" + pr_block: dict[str, str] = { + "title": "Implement #2548", + "description": "Slice-1 stub.", + } + if with_context_title: + pr_block["context_title"] = title_value + if with_context_description: + pr_block["context_description"] = description_value + return {"pr": pr_block, "phases": []} + + def test_extract_returns_none_pair_when_pr_block_missing(self): + from egg_contracts.plan_parser import extract_pr_context_metadata_from_yaml + + title, desc, warnings = extract_pr_context_metadata_from_yaml({"phases": []}) + assert title is None + assert desc is None + assert warnings == [] + + def test_extract_returns_none_pair_when_yaml_data_is_none(self): + from egg_contracts.plan_parser import extract_pr_context_metadata_from_yaml + + title, desc, warnings = extract_pr_context_metadata_from_yaml(None) + assert title is None + assert desc is None + assert warnings == [] + + def test_extract_returns_none_pair_when_keys_absent(self): + from egg_contracts.plan_parser import extract_pr_context_metadata_from_yaml + + yaml_data = self._make_yaml() # neither key present + title, desc, warnings = extract_pr_context_metadata_from_yaml(yaml_data) + assert title is None + assert desc is None + assert warnings == [] + + def test_extract_returns_populated_when_keys_present(self): + from egg_contracts.plan_parser import extract_pr_context_metadata_from_yaml + + yaml_data = self._make_yaml( + with_context_title=True, + with_context_description=True, + ) + title, desc, warnings = extract_pr_context_metadata_from_yaml(yaml_data) + assert title == "Strategic plan for #2548" + assert desc == "Refine + plan artifacts." + assert warnings == [] + + def test_extract_normalises_whitespace_to_none(self): + """A planner emitting whitespace-only block scalars must collapse to None. + + The orchestrator hook's ``contract.pr.context_title or + contract.pr.title`` fallback works with both ``None`` and + ``""``; collapsing whitespace to ``None`` here keeps the + contract diff clean (no spurious whitespace strings) and + matches the existing ``_normalize_optional_string`` behavior + for legacy fields. + """ + from egg_contracts.plan_parser import extract_pr_context_metadata_from_yaml + + yaml_data = self._make_yaml( + with_context_title=True, + with_context_description=True, + title_value=" ", + description_value=" ", + ) + title, desc, warnings = extract_pr_context_metadata_from_yaml(yaml_data) + assert title is None + assert desc is None + + def test_extract_warns_on_non_string_context_title(self): + """A non-string ``context_title`` must produce a ParseWarning. + + Mirror of the existing behavior on ``pr.title`` — surfacing the + type mismatch makes planner-prompt regressions easy to spot. + """ + from egg_contracts.plan_parser import extract_pr_context_metadata_from_yaml + + yaml_data = { + "pr": { + "title": "Implement #2548", + "context_title": 12345, # int — not a string + }, + "phases": [], + } + title, _desc, warnings = extract_pr_context_metadata_from_yaml(yaml_data) + assert title is None # malformed → fall back + assert len(warnings) == 1 + assert "context_title" in warnings[0].message + assert "int" in warnings[0].message + + def test_extract_warns_on_non_string_context_description(self): + """A non-string ``context_description`` must also warn. + + Symmetric with the ``context_title`` branch above. Without an + explicit type check, ``_normalize_optional_string`` would + silently coerce non-strings via ``str(value)`` (e.g. an int + ``12345`` becomes ``"12345"``, a dict ``{a: b}`` becomes + ``"{'a': 'b'}"``) and a planner-prompt regression that started + emitting structured values would land quietly on the contract. + """ + from egg_contracts.plan_parser import extract_pr_context_metadata_from_yaml + + yaml_data = { + "pr": { + "title": "Implement #2548", + "context_description": {"unexpected": "mapping"}, + }, + "phases": [], + } + _title, desc, warnings = extract_pr_context_metadata_from_yaml(yaml_data) + assert desc is None # malformed → fall back + assert len(warnings) == 1 + assert "context_description" in warnings[0].message + assert "dict" in warnings[0].message + + def test_extract_warns_on_int_context_description(self): + """Integer scalars on ``context_description`` warn rather than coerce.""" + from egg_contracts.plan_parser import extract_pr_context_metadata_from_yaml + + yaml_data = { + "pr": { + "title": "Implement #2548", + "context_description": 12345, + }, + "phases": [], + } + _title, desc, warnings = extract_pr_context_metadata_from_yaml(yaml_data) + assert desc is None + assert len(warnings) == 1 + assert "context_description" in warnings[0].message + assert "int" in warnings[0].message + + def test_parse_plan_threads_context_into_parse_result(self): + """End-to-end: ``parse_plan`` must populate ``ParseResult.pr_context_*``.""" + from egg_contracts.plan_parser import parse_plan + + plan_md = ( + "# Plan\n\n" + "```yaml\n" + "# yaml-tasks\n" + "pr:\n" + ' title: "Implement #2548"\n' + ' description: "Slice-1 stub."\n' + ' context_title: "Strategic plan for #2548"\n' + " context_description: |\n" + " Refine + plan artifacts for issue 2548.\n" + "phases:\n" + " - id: 1\n" + " name: slice-1\n" + " tasks: []\n" + "```\n" + ) + result = parse_plan(plan_md) + assert result.success is True + assert result.pr_context_title == "Strategic plan for #2548" + assert result.pr_context_description == "Refine + plan artifacts for issue 2548." + + def test_parse_plan_defaults_context_to_none_when_keys_omitted(self): + from egg_contracts.plan_parser import parse_plan + + plan_md = ( + "# Plan\n\n" + "```yaml\n" + "# yaml-tasks\n" + "pr:\n" + ' title: "Implement #2548"\n' + ' description: "Slice-1 stub."\n' + "phases:\n" + " - id: 1\n" + " name: slice-1\n" + " tasks: []\n" + "```\n" + ) + result = parse_plan(plan_md) + assert result.success is True + assert result.pr_context_title is None + assert result.pr_context_description is None + + +class TestPRMetadataAdversarial: + """Adversarial probes added by the tester role (#2548 task-1-2). + + The classes above pin the happy paths and the symmetric warning + branches. The tests below try to break the implementation in ways + a planner-prompt regression, a hand-edited contract, or a future + refactor of ``_migrate_schema_version_to_1_1`` could plausibly + expose. + """ + + def test_model_dump_json_round_trip_preserves_all_context_fields(self): + """``model_dump_json()`` is the on-disk path; round-trip must be + value-preserving for every ``context_*`` field. + + Adversarial: ``model_dump()`` returns Python objects, but the + contract is persisted via JSON. A future custom serializer that + treated ``None`` as "omit from output" would silently drop the + absent-context distinction; this test fails loudly if that + happens. + """ + import json + + pr = PRMetadata( + title="Implement #2548", + context_title="Strategic plan for #2548", + context_description="Refine + plan artifacts.", + context_branch="egg/issue-2548/context", + context_pr_number=4242, + ) + as_json = pr.model_dump_json() + # Survives a JSON round-trip — no lossy custom encoder. + decoded = json.loads(as_json) + assert decoded["context_title"] == "Strategic plan for #2548" + assert decoded["context_description"] == "Refine + plan artifacts." + assert decoded["context_branch"] == "egg/issue-2548/context" + assert decoded["context_pr_number"] == 4242 + + round_trip = PRMetadata.model_validate_json(as_json) + assert round_trip.context_title == "Strategic plan for #2548" + assert round_trip.context_description == "Refine + plan artifacts." + assert round_trip.context_branch == "egg/issue-2548/context" + assert round_trip.context_pr_number == 4242 + + def test_model_dump_json_preserves_null_context_fields(self): + """A ``None`` context value must serialise as JSON ``null``, not omitted. + + Adversarial: ``model_dump_json(exclude_none=True)`` is a one-line + change away in the future. Pin the explicit-null behavior so an + accidental ``exclude_none`` regression breaks the test rather + than silently changing the on-disk shape (round-trips would + still work but external readers would see schema drift). + """ + import json + + pr = PRMetadata(title="Plain PR — no context fields") + as_json = pr.model_dump_json() + decoded = json.loads(as_json) + assert decoded["context_title"] is None + assert decoded["context_description"] is None + assert decoded["context_branch"] is None + assert decoded["context_pr_number"] is None + + def test_combined_phases_and_schemaversion_migration(self): + """A legacy contract with both ``phases:`` (pre-#2137) AND ``schemaVersion=1.0`` + (pre-#2548) must be migrated correctly by both validators. + + Adversarial: both migrations live on the same model. + ``_migrate_phases_to_slices`` runs in ``mode="wrap"`` and + rewrites the input dict; ``_migrate_schema_version_to_1_1`` + runs in ``mode="after"`` on the constructed instance. A bug in + the wrap-mode validator could swallow the schemaVersion field; + a bug in the after-mode validator could fire before the wrap + completes. Pin the combined invariant: legacy keys re-map AND + the schemaVersion bumps to 1.1 in the same load. + """ + payload = _minimal_contract_payload(schema_version="1.0") + # Reshape the payload to exercise the legacy phases-key path. + del payload["slices"] + payload["phases"] = [ + {"id": "phase-1", "name": "first", "tasks": []}, + { + "id": "phase-2", + "name": "second", + "tasks": [], + "dependencies": ["phase-1"], + }, + ] + contract = Contract.model_validate(payload) + # schemaVersion was bumped to the post-#2548 version. + assert contract.schemaVersion == "1.1" + # phases-key was migrated to slices, and the slice-N IDs were + # canonicalised (the dependency edge too). + assert len(contract.slices) == 2 + assert contract.slices[0].id == "slice-1" + assert contract.slices[1].id == "slice-2" + assert contract.slices[1].dependencies == ["slice-1"] + # Context fields default to None on the bumped contract. + assert contract.pr is not None + assert contract.pr.context_title is None + assert contract.pr.context_pr_number is None + + def test_yaml_null_for_context_fields_yields_none(self): + """A planner emitting ``context_title: ~`` (YAML null) must thread + through as ``None``, not the string ``"~"`` or ``"None"``. + + Adversarial: a fragile parser that did ``str(value)`` on raw + YAML scalars would silently coerce a YAML null to ``"None"``, + which the orchestrator's ``or pr.title`` fallback would happily + accept as a non-empty string and use as the context-PR title. + Lock this down at the parse-plan boundary. + """ + from egg_contracts.plan_parser import parse_plan + + plan_md = ( + "# Plan\n\n" + "```yaml\n" + "# yaml-tasks\n" + "pr:\n" + ' title: "Implement #2548"\n' + ' description: "Slice-1 stub."\n' + " context_title: ~\n" + " context_description: null\n" + "phases:\n" + " - id: 1\n" + " name: slice-1\n" + " tasks: []\n" + "```\n" + ) + result = parse_plan(plan_md) + assert result.success is True + assert result.pr_context_title is None + assert result.pr_context_description is None + + def test_parse_plan_markdown_only_yields_none_context(self): + """A plan document with no yaml-tasks fence (markdown-regex + fallback path) must still produce ``pr_context_*`` as ``None``. + + Adversarial: parse_plan's third-priority fallback bypasses + ``extract_pr_context_metadata_from_yaml`` because there is no + YAML to extract from. The ``ParseResult`` defaults must keep + the context fields ``None`` — without this guard a regression + that initialised them to ``""`` would leak into the contract + and the orchestrator's truthiness fallback would still work, + masking the bug. + """ + from egg_contracts.plan_parser import parse_plan + + plan_md = ( + "# Plan\n\n" + "## Phase 1: Setup\n" + "**Goal**: foo\n\n" + "- [TASK-1-1] Do thing — Acceptance: it works\n" + ) + result = parse_plan(plan_md) + assert result.success is True + assert result.pr_context_title is None + assert result.pr_context_description is None + + def test_extract_warns_on_list_typed_context_title(self): + """A list value for ``context_title`` must warn — not coerce. + + Adversarial: the existing tests cover ``int`` and ``dict`` for + the description branch and ``int`` for the title branch. A + ``list`` (e.g. a planner that confused ``context_title`` with + ``files_affected``) would round-trip through + ``_normalize_optional_string`` as ``"['a', 'b']"`` if the + ``isinstance(raw_title, str)`` guard in + ``extract_pr_context_metadata_from_yaml`` regressed. The check + fires in the parser before the value reaches ``PRMetadata``, so + pydantic is not involved in this code path; pin the + parser-layer warning path explicitly. + """ + from egg_contracts.plan_parser import extract_pr_context_metadata_from_yaml + + yaml_data = { + "pr": { + "title": "Implement #2548", + "context_title": ["a", "b"], + }, + "phases": [], + } + title, _desc, warnings = extract_pr_context_metadata_from_yaml(yaml_data) + assert title is None + assert len(warnings) == 1 + assert "context_title" in warnings[0].message + assert "list" in warnings[0].message + + def test_extract_warns_on_list_typed_context_description(self): + """Symmetric with ``test_extract_warns_on_list_typed_context_title``. + + Adversarial: the description branch's existing coverage is + ``dict`` and ``int``. A ``list`` would round-trip through + ``_normalize_optional_string`` as ``"[a, b]"`` if the type + guard regressed; lock the warning path down so a planner + emitting an accidental list of strings is caught. + """ + from egg_contracts.plan_parser import extract_pr_context_metadata_from_yaml + + yaml_data = { + "pr": { + "title": "Implement #2548", + "context_description": ["line one", "line two"], + }, + "phases": [], + } + _title, desc, warnings = extract_pr_context_metadata_from_yaml(yaml_data) + assert desc is None + assert len(warnings) == 1 + assert "context_description" in warnings[0].message + assert "list" in warnings[0].message + + def test_extract_handles_crlf_whitespace_in_context_fields(self): + """A planner emitting CRLF / mixed whitespace must still strip cleanly. + + Adversarial: agents writing on Windows-line-ending hosts (or a + planner whose prompt template has CRLF) could emit + ``" Strategic plan \\r\\n"`` for ``context_title``. The + existing ``_normalize_optional_string`` uses ``.strip()`` which + handles CRLF; pin the behavior so a future hand-rolled + replacement that only stripped ``\\n`` would catch the gap. + """ + from egg_contracts.plan_parser import extract_pr_context_metadata_from_yaml + + yaml_data = { + "pr": { + "title": "Implement #2548", + "context_title": " Strategic plan for #2548 \r\n", + "context_description": "\r\n multi-line \n body \r\n", + }, + "phases": [], + } + title, desc, warnings = extract_pr_context_metadata_from_yaml(yaml_data) + assert title == "Strategic plan for #2548" + # Internal newlines preserved; only leading/trailing stripped. + assert desc == "multi-line \n body" + assert warnings == [] + + def test_context_pr_number_accepts_large_int(self): + """A high GitHub PR number (six- or seven-digit) must round-trip. + + Adversarial: a future ``int32``-style validator (``le=2**31-1``) + added without thought would clip realistic PR numbers on a + long-lived monorepo. Pin a generous ceiling so the validator + stays scoped to the ``ge=1`` lower bound documented in the model. + """ + # GitHub doesn't publish a hard cap; common monorepos already + # exceed 100k PRs. 10_000_000 is comfortably above any + # plausible repo for the foreseeable future. + pr = PRMetadata(title="t", context_pr_number=10_000_000) + assert pr.context_pr_number == 10_000_000 + # And it round-trips through model_validate without loss. + round_trip = PRMetadata.model_validate(pr.model_dump()) + assert round_trip.context_pr_number == 10_000_000 + + def test_invalid_schemaversion_format_rejected(self): + """``schemaVersion`` must match the ``M.N`` regex — a freeform + string like ``"1.0-rc1"`` or ``"v1.0"`` must raise. + + Adversarial: a future migration that emitted ``"1.1-#2548"`` or + ``"v1.1"`` would silently land on disk if the regex were + relaxed; pin the strict format so any drift fails fast. + """ + payload = _minimal_contract_payload(schema_version="1.0-rc1") + with pytest.raises(ValidationError): + Contract.model_validate(payload) + + payload = _minimal_contract_payload(schema_version="v1.0") + with pytest.raises(ValidationError): + Contract.model_validate(payload) + + def test_legacy_1_0_with_explicit_context_fields_loads(self): + """A 1.0 payload that ALREADY carries the new ``context_*`` keys + (e.g., a hand-edited contract or a partial mid-flight migration) + must load cleanly: the schemaVersion bumps, the explicit context + values are preserved. + + Adversarial: the migration shim only runs when schemaVersion is + exactly ``"1.0"``. Pin that the bump does NOT erase explicit + context values — the validator must be additive, not corrective. + """ + payload = _minimal_contract_payload(schema_version="1.0") + payload["pr"]["context_title"] = "Strategic plan for #2548" + payload["pr"]["context_description"] = "Refine + plan artifacts." + payload["pr"]["context_branch"] = "egg/issue-2548/context" + payload["pr"]["context_pr_number"] = 4242 + + contract = Contract.model_validate(payload) + assert contract.schemaVersion == "1.1" + assert contract.pr is not None + assert contract.pr.context_title == "Strategic plan for #2548" + assert contract.pr.context_description == "Refine + plan artifacts." + assert contract.pr.context_branch == "egg/issue-2548/context" + assert contract.pr.context_pr_number == 4242 + + def test_extract_returns_none_when_pr_block_is_non_dict(self): + """A malformed ``pr:`` block (e.g. a list) must not crash the + extractor. Returns ``(None, None, [])`` — the warning is + already produced by ``extract_pr_metadata_from_yaml`` so we do + not duplicate it here, but the extractor must short-circuit + rather than ``AttributeError`` on ``.get``. + + Adversarial: a planner that confused YAML mapping syntax could + emit ``pr: [title, body]``. The legacy ``extract_pr_metadata_from_yaml`` + produces a structural warning for that case; the new context + extractor must align with that contract (silent short-circuit + when its sibling already warned) rather than raising. + """ + from egg_contracts.plan_parser import extract_pr_context_metadata_from_yaml + + title, desc, warnings = extract_pr_context_metadata_from_yaml( + {"pr": ["title-as-list-item", "body-as-list-item"]} + ) + assert title is None + assert desc is None + assert warnings == []