diff --git a/docs/architecture/slice-dag.md b/docs/architecture/slice-dag.md index 73eaec0a55..fc084cd487 100644 --- a/docs/architecture/slice-dag.md +++ b/docs/architecture/slice-dag.md @@ -627,14 +627,14 @@ extended to teach the agents the new schema and constraints: declares this authority explicitly and the architect emits a binding `architect-slices.yaml` scaffold alongside its analysis JSON (`{identifier}-architect-slices.yaml` under `.egg-state/agent-outputs/`). - The scaffold encodes slice `id` / `name` / `goal` / `parent_slice_id`; + The scaffold encodes slice `id` / `name` / `goal` / `dependencies`; `tasks:` is intentionally omitted (that is `task_planner`'s job). - **Planner (`task_planner`)** — sections appended to the plan phase prompt: 1. *Slice composition is NOT the planner's call* (#2809): the architect's `architect-slices.yaml` scaffold is binding. The planner copies it verbatim into the `# yaml-tasks` appendix - (same `id` / `name` / `goal` / `parent_slice_id`, same order) and + (same `id` / `name` / `goal` / `dependencies`, same order) and fills in `tasks:` under each slice. The planner does not silently re-shape slices — if a slice needs to be subdivided, the planner raises NACK pressure on the architect (via the @@ -643,7 +643,8 @@ extended to teach the agents the new schema and constraints: 2. *Forest constraint* (HARD): every slice must have ≤1 DAG parent; the populator hard-rejects multi-parent slices with `ForestValidationError`. The architect's scaffold encodes this - via `parent_slice_id`; the planner preserves it. + via a single-parent `dependencies` id (`slice-`); the planner + preserves it. 3. *Auto-serialization* for would-be multi-parent slices: the architect is responsible for serialising the upstream cluster and populating `serialized_chain_order` on the downstream diff --git a/docs/reference/agent-roles.md b/docs/reference/agent-roles.md index 19cc45a165..0fff868829 100644 --- a/docs/reference/agent-roles.md +++ b/docs/reference/agent-roles.md @@ -92,7 +92,7 @@ All agents within a phase run concurrently via BRC consensus. Concurrency is ena **Outputs**: - `.egg-state/agent-outputs/{identifier}-architect-output.json` — Architectural analysis -- `.egg-state/agent-outputs/{identifier}-architect-slices.yaml` — Binding slice scaffold (`id` / `name` / `goal` / `parent_slice_id`; no `tasks:` — that is `task_planner`'s job) +- `.egg-state/agent-outputs/{identifier}-architect-slices.yaml` — Binding slice scaffold (`id` / `name` / `goal` / `dependencies`; no `tasks:` — that is `task_planner`'s job) **Prompt context**: Full issue body, refine analysis. diff --git a/orchestrator/routes/pipelines.py b/orchestrator/routes/pipelines.py index da732d6918..f21a710fde 100644 --- a/orchestrator/routes/pipelines.py +++ b/orchestrator/routes/pipelines.py @@ -14677,20 +14677,24 @@ def _build_agent_prompt( " ", " goal: |-", " ", - " parent_slice_id: null # root", + " # root slice — omit ``dependencies``", " - id: 2", " name: |-", " ", " goal: |-", " ", - " parent_slice_id: 1", + " dependencies: slice-1", "```", "", - "Use ``parent_slice_id: null`` for root slices and the " - "parent slice's integer ``id`` otherwise. Do NOT include " - "``tasks:`` in the scaffold — that is task_planner's job. " - "Keep ``name`` and ``goal`` concise enough that " - "task_planner can copy them without rewording.", + "Omit ``dependencies`` for root slices; for every non-root " + "slice set ``dependencies`` to its single parent's " + "``slice-`` (e.g. ``slice-1``). ``dependencies`` is the " + "canonical ordering key the plan parser reads (per " + "`.egg/schemas/yaml-tasks.schema.json`) — the slice DAG is a " + "forest, so each slice has at most one parent (one id, not a " + "list). Do NOT include ``tasks:`` in the scaffold — that is " + "task_planner's job. Keep ``name`` and ``goal`` concise " + "enough that task_planner can copy them without rewording.", "", "### File Restrictions", "", @@ -14723,7 +14727,7 @@ def _build_agent_prompt( "is to enumerate ``tasks:`` within those slices, **not to re-shape " "them**. Copy the architect's scaffold verbatim into the " "``# yaml-tasks`` appendix (preserving slice ``id``, ``name``, " - "``goal``, and ``parent_slice_id``) and add ``tasks:`` under each " + "``goal``, and ``dependencies``) and add ``tasks:`` under each " "slice with task IDs of the form ``TASK--``.", "", "If a slice has too many tasks for one BRC cycle, or you discover a " @@ -14740,7 +14744,7 @@ def _build_agent_prompt( f"1. Read the architecture analysis AND the slice scaffold at `{architect_slices_path}`", "2. Copy the architect's slice scaffold verbatim into the " "``# yaml-tasks`` appendix (same ``id`` / ``name`` / ``goal`` / " - "``parent_slice_id`` values, in the same order)", + "``dependencies`` values, in the same order)", "3. Enumerate ``tasks:`` under each slice — discrete, " "actionable, with clear acceptance criteria and dependency ordering " "between tasks", @@ -14892,7 +14896,8 @@ def _build_agent_prompt( "every slice must have at most ONE DAG parent — the " "implement-phase pipeline ships every slice as a stacked " "PR with exactly one base branch. The architect's scaffold " - "encodes this via ``parent_slice_id``; preserve it.", + "encodes this via a single-parent ``dependencies`` id " + "(``slice-``); preserve it.", "", "**Auto-serialization for would-be multi-parent slices**: " "the architect is responsible for serialising would-be " diff --git a/orchestrator/tests/test_pipeline_prompts.py b/orchestrator/tests/test_pipeline_prompts.py index cab1c098be..0abd658201 100644 --- a/orchestrator/tests/test_pipeline_prompts.py +++ b/orchestrator/tests/test_pipeline_prompts.py @@ -2678,7 +2678,12 @@ def test_includes_architect_slices_yaml_when_present(self, tmp_path): " Bootstrap\n" " goal: |-\n" " Stand up the new auth route\n" - " parent_slice_id: null\n" + " - id: 2\n" + " name: |-\n" + " Wire handlers\n" + " goal: |-\n" + " Hook the route into the dispatcher\n" + " dependencies: slice-1\n" ) (outputs_dir / "871-architect-slices.yaml").write_text(scaffold_yaml) (outputs_dir / "871-risk_analyst-output.json").write_text( @@ -2698,7 +2703,7 @@ def test_includes_architect_slices_yaml_when_present(self, tmp_path): # Scaffold heading and raw YAML body (YAML falls through json.loads to raw text) assert "## Slice Scaffold" in content assert "Bootstrap" in content - assert "parent_slice_id: null" in content + assert "dependencies: slice-1" in content # Other agent outputs still included assert "Architecture analysis for issue 871" in content assert "Risk assessment for issue 871" in content @@ -5884,8 +5889,11 @@ def test_architect_prompt_emits_slice_scaffold_yaml(self): # Architect must be directed to write the scaffold at the # issue-prefixed path. assert "871-architect-slices.yaml" in prompt - # Scaffold schema must show parent_slice_id for forest expression. - assert "parent_slice_id" in prompt + # Scaffold must express slice ordering via the canonical + # ``dependencies`` key (the parser/schema key per #2779 / #2870), + # NOT ``parent_slice_id`` — which the parser silently drops. + assert "dependencies: slice-1" in prompt + assert "parent_slice_id" not in prompt # Architect must NOT enumerate tasks in the scaffold — that's # task_planner's role. scaffold_start = prompt.index("Write the slice scaffold") diff --git a/shared/egg_contracts/plan_parser.py b/shared/egg_contracts/plan_parser.py index 97528ae284..fb26b71863 100644 --- a/shared/egg_contracts/plan_parser.py +++ b/shared/egg_contracts/plan_parser.py @@ -89,6 +89,30 @@ # downstream Pydantic validator. Compiled once at import. _JIRA_KEY_PATTERN = re.compile(r"^[A-Z][A-Z0-9_]*-[0-9]+$") +# Every per-slice key the parser actually consumes from the ``# yaml-tasks`` +# appendix. Mirrors the ``slice`` definition in +# ``.egg/schemas/yaml-tasks.schema.json`` (which is ``additionalProperties: +# false``) plus the ``depends_on`` alias the parser tolerates (#2743). A key +# outside this set is silently ignored by the parser, so an unrecognised key +# means the planner expressed something the contract never sees — exactly the +# #2870 failure mode, where the architect emitted ``parent_slice_id`` and the +# whole slice dependency chain was dropped. The schema would have rejected it, +# but the schema is only enforced in tests, never at parse/populate time — so +# we surface the unknown key as a parse warning here to make the drift loud +# instead of silent. +_KNOWN_SLICE_KEYS = frozenset( + { + "id", + "name", + "goal", + "dependencies", + "depends_on", + "serialized_chain_order", + "exit_criteria", + "tasks", + } +) + def _extract_jira_task_fields( task_data: dict[str, Any], @@ -710,6 +734,27 @@ def parse_phases_from_yaml( continue seen_phase_ids.add(phase_num) + # #2870 — flag keys the parser doesn't consume. The yaml-tasks + # schema is ``additionalProperties: false`` but is never validated + # at parse time, so a stray key (e.g. ``parent_slice_id``) is + # otherwise dropped silently — taking its data with it. ``id`` is + # always present; report the rest sorted for a stable message. + unknown_keys = sorted(set(phase_data) - _KNOWN_SLICE_KEYS) + if unknown_keys: + warnings.append( + ParseWarning( + line_number=None, + message=( + f"Slice {phase_num} has unrecognized key(s) " + f"{unknown_keys} — ignored by the parser, so any " + "data they carry (e.g. slice ordering) is silently " + "dropped. Slice ordering must use 'dependencies' " + "(canonical) or 'depends_on' (see #2870)." + ), + context="Allowed slice keys: " + ", ".join(sorted(_KNOWN_SLICE_KEYS)), + ) + ) + phase_name = phase_data.get("name", f"Slice {phase_num}") phase_goal = phase_data.get("goal", "") # #2743 — accept ``depends_on`` as an alias for ``dependencies``. diff --git a/tests/shared/egg_contracts/test_plan_parser.py b/tests/shared/egg_contracts/test_plan_parser.py index ecba0216ed..016fe7a3f6 100644 --- a/tests/shared/egg_contracts/test_plan_parser.py +++ b/tests/shared/egg_contracts/test_plan_parser.py @@ -1004,6 +1004,62 @@ def test_parse_phases_from_yaml_warns_on_bool_depends_on(self): "'depends_on' is a bool" in w.message and "Slice 2" in w.message for w in warnings ) + def test_parse_phases_from_yaml_warns_on_parent_slice_id(self): + """#2870 regression: the architect scaffold once emitted ordering + as ``parent_slice_id`` (a key the parser never reads), so the whole + slice dependency chain was silently dropped and every slice became + a DAG root. The parser must now surface the unrecognized key as a + ParseWarning so the drop is loud, not silent. The drop itself is + still asserted (deps stay empty) — the warning is the new net. + """ + yaml_data = { + "slices": [ + { + "id": 1, + "name": "A", + "parent_slice_id": None, + "tasks": [{"id": "TASK-1-1", "description": "a", "acceptance": "ok"}], + }, + { + "id": 2, + "name": "B", + "parent_slice_id": 1, + "tasks": [{"id": "TASK-2-1", "description": "b", "acceptance": "ok"}], + }, + ] + } + phases, warnings = parse_phases_from_yaml(yaml_data) + slices = [p.to_contract_slice() for p in phases] + # The edge is still dropped — ``parent_slice_id`` is not a parser key. + assert slices[1].dependencies == [] + # ...but it's no longer silent. + assert any("parent_slice_id" in w.message and "Slice 2" in w.message for w in warnings) + + def test_parse_phases_from_yaml_no_warn_on_known_keys(self): + """The unknown-key net must not false-positive on the full set of + keys the parser legitimately consumes (incl. the ``depends_on`` + alias and ``serialized_chain_order``).""" + yaml_data = { + "slices": [ + { + "id": 1, + "name": "A", + "goal": "g", + "exit_criteria": "done", + "tasks": [{"id": "TASK-1-1", "description": "a", "acceptance": "ok"}], + }, + { + "id": 2, + "name": "B", + "depends_on": "slice-1", + "serialized_chain_order": ["slice-1"], + "tasks": [{"id": "TASK-2-1", "description": "b", "acceptance": "ok"}], + }, + ] + } + _, warnings = parse_phases_from_yaml(yaml_data) + assert not any("unrecognized key" in w.message for w in warnings) + class TestParsePhasesFromYaml: """Tests for parsing phases from structured YAML."""