Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions docs/architecture/slice-dag.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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-<N>`); 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
Expand Down
2 changes: 1 addition & 1 deletion docs/reference/agent-roles.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
25 changes: 15 additions & 10 deletions orchestrator/routes/pipelines.py
Original file line number Diff line number Diff line change
Expand Up @@ -14677,20 +14677,24 @@ def _build_agent_prompt(
" <slice name>",
" goal: |-",
" <what this slice achieves>",
" parent_slice_id: null # root",
" # root slice — omit ``dependencies``",
" - id: 2",
" name: |-",
" <slice name>",
" goal: |-",
" <what this slice achieves>",
" 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-<id>`` (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",
"",
Expand Down Expand Up @@ -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-<slice_id>-<n>``.",
"",
"If a slice has too many tasks for one BRC cycle, or you discover a "
Expand All @@ -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",
Expand Down Expand Up @@ -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-<N>``); preserve it.",
"",
"**Auto-serialization for would-be multi-parent slices**: "
"the architect is responsible for serialising would-be "
Expand Down
16 changes: 12 additions & 4 deletions orchestrator/tests/test_pipeline_prompts.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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
Expand Down Expand Up @@ -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")
Expand Down
45 changes: 45 additions & 0 deletions shared/egg_contracts/plan_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand Down Expand Up @@ -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``.
Expand Down
56 changes: 56 additions & 0 deletions tests/shared/egg_contracts/test_plan_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."""
Expand Down
Loading