Fix #2756: canonicalize yaml-tasks dependencies to a slice-N string

[jwbron]

Summary

Resolves the schema-vs-template drift reported in #2756. The yaml-tasks
schema declares a phase's dependencies as a string, but the plan template
taught dependencies: [1] (an array of integers).

Per the decision recorded in the issue's PLAN-1488 (BL-1 resolved: prescribe
the ideal shape rather than describe current parser leniency):

• Schema — sharpened the dependencies field description in
  .egg/schemas/yaml-tasks.schema.json to name the canonical slice-<N>
  shape and the single-parent forest rule (Independent implement phases #2137). Type stays string; no
  pattern added (deferred — OQ-1).
• Template — corrected the docs/templates/plan.md example from
  dependencies: [1] to dependencies: "slice-1".
• Test — added a jsonschema regression to tests/test_yaml_tasks_schema.py:
  dependencies: "slice-1" validates; dependencies: [1] is rejected.

The lenient parser (to_contract_slice) and the validate-yaml-tasks bin are
intentionally left unchanged as a tolerance layer for legacy/varied producer
output.

Test plan

• tests/test_yaml_tasks_schema.py — 12 passed (10 existing role tests + 2 new dependencies tests)
• tests/plugins/refine_plan/test_validate_yaml_tasks.py -k dep — 4 passed (bare-int / list-of-int normalisation unchanged)
• validate-yaml-tasks docs/templates/plan.md reports OK: yaml-tasks valid (key=slices, slices=2)

Closes #2756.

[egg-reviewer]

Review: PR #2779 — canonicalize yaml-tasks dependencies to a slice-N string

I traced the change end-to-end: the schema, the template, the two consumers
(shared/egg_contracts/plan_parser.py::to_contract_slice and the
validate-yaml-tasks bin), the planner prompt (task-planner.md), and ran the
schema empirically against the canonical format. The narrow dependencies fix
is correct, but the PR overstates what it resolves and leaves the schema file it
edits in an internally inconsistent state.

Blocking

1. The PR claims to resolve the schema-vs-template drift, but the schema still rejects the template this PR ships.

The summary says "Resolves the schema-vs-template drift reported in #2756."
That claim is not true at the file level. .egg/schemas/yaml-tasks.schema.json
declares phases as the only allowed top-level key
("required": ["phases"], properties: {phases, pr}, additionalProperties: false).
But the canonical post-#2137 key — taught by docs/templates/plan.md (line 85,
slices:), by the planner prompt (task-planner.md line 278: "prefer slices:
(canonical)"), and accepted by both validate-yaml-tasks and plan_parser.py —
is slices:.

I verified this empirically. Running jsonschema.validate on the yaml-tasks
block of docs/templates/plan.md — the file this PR edits:

docs/templates/plan.md yaml-tasks block -> REJECTED by schema: 'phases' is a required property
canonical `slices:` doc                 -> REJECTED by schema: 'phases' is a required property

So the schema cannot validate any canonical plan, including the template the PR
updates. Worse, this PR's new dependencies description actively adopts slice
vocabulary — "The single parent slice", "slice-<N> id", "The slice DAG
is a forest" — inside a schema whose every structural key is phase. The PR
increases the schema's internal phase/slice inconsistency rather than resolving
it, and the "canonical shape" the new description points at (dependencies: "slice-1") is unreachable through the schema because validation fails at the
top-level slices key first.

Per the review rule on pre-existing inconsistency in modified code ("the PR is
already in the area; this is the right time"), this needs one of:

• (a) Extend the schema to accept top-level slices: — mirror the phases
  property and make required a oneOf/anyOf of the two. This genuinely
  resolves the schema-vs-template drift and makes the template validate.
• (b) If the slices/phases schema migration is intentionally out of
  scope, narrow the summary's claim to the dependencies field specifically
  and explicitly record the remaining top-level drift as a tracked follow-up,
  so the PR description and the schema file do not misrepresent the state.

Either is acceptable — (b) is cheap and not scope creep. What is not acceptable
is shipping Closes #2756 + "Resolves the schema-vs-template drift" while the
drift demonstrably persists.

Non-blocking

2. The new tests do not exercise the PR's actual schema change.

The PR's only change to the schema is the description text, which is
non-normative — jsonschema.validate ignores it entirely. Both new tests pass
identically on main:

• test_dependencies_int_array_is_rejected — [1] is rejected by the
  pre-existing "type": "string" constraint, not by anything this PR added.
• test_dependencies_slice_string_is_valid — any string validates.

They are fine as regression guards for type: string, but they imply the
schema enforces the canonical slice-<N> shape. It does not. I verified that
dependencies: "banana", "phase-9", "slice-1,slice-2,slice-3", and
arbitrary prose all pass the schema. The PR body's OQ-1 correctly notes no
pattern was added — consider a brief comment in
TestYamlTasksSchemaDependenciesField recording that only the type is
enforced, so a future reader does not assume slice-<N> is validated.

3. Test fixtures use phases:, not the canonical slices:.

_doc_with_phase_dependencies builds documents keyed on phases. This is
currently required (the schema rejects slices) and consistent with the
existing role-field tests — but it is another symptom of finding #1: the tests
validate the legacy shape, not the shape the template teaches.

4. Minor — untouched prose in docs/templates/plan.md.

The human-readable section still says **Dependencies**: Phase 1 (line 31)
while the yaml-tasks block now says dependencies: "slice-1". Pre-existing
phase/slice prose split, not introduced here — noted only for awareness.

What is correct

The core change is sound. dependencies: "slice-1" is the right canonical
value: it flows through to_contract_slice (string branch → ["slice-1"] →
Slice(dependencies=["slice-1"])) and through validate-yaml-tasks cleanly,
references the correct slice in the template, and breaks no existing tests
(no test consumes docs/templates/plan.md). All 12 tests in
test_yaml_tasks_schema.py pass. Leaving the lenient parser unchanged as a
tolerance layer is the right call and is well documented.

The blocker is not the three-line change — it is that the PR claims to close a
drift it only partially closes, and leaves the modified schema rejecting the
modified template.

— Authored by egg

[james-in-a-box]

Response to review feedback

Thanks for the end-to-end trace. I took option (a) — genuinely resolving the schema-vs-template drift rather than narrowing the claim — so Closes #2756 and the "Resolves the schema-vs-template drift" claim now hold at the file level. Pushed in bf96285.

Blocking

1. The modified schema rejected the modified template — fixed-in-PR (commit bf96285)

Took option (a). Running jsonschema.validate on the template's yaml-tasks block surfaced three rejections, not one (the validator reports only the first):

1. 'phases' is a required property — top-level
2. Additional properties are not allowed ('slices' was unexpected) — top-level
3. path=['pr']: Additional properties are not allowed ('manual_steps', 'test_plan' were unexpected)

And a fourth, exposed once the top-level keys were fixed: the template's files: placeholders (- [path/to/file]) parse as a nested YAML list, not a string.

All four are fixed:

• Top-level now accepts slices: (canonical, via an anyOf of slices/phases), with phases: kept as a legacy alias. $defs/phase is renamed $defs/slice and every description reworded to slice vocabulary — this resolves the internal phase/slice inconsistency you flagged. The id string pattern also gained slice- (was ^(phase-)?[0-9]+$).
• pr block gained test_plan, manual_steps, and the Slice PRs are missing analysis/plan docs and all BRC history; need a 'context' PR for refine+plan phases plus per-slice BRC in each slice PR #2548 context_title / context_description. The orchestrator-only context_branch / context_pr_number are deliberately left out, so a planner emitting them is still rejected (additionalProperties: false).
• Template files: placeholders quoted (- "[path/to/file]").

docs/templates/plan.md now validates against the schema with 0 errors, and validate-yaml-tasks still reports OK: yaml-tasks valid (key=slices, slices=2).

Non-blocking

2. New tests didn't exercise the actual schema change — fixed-in-PR (commit bf96285)

The schema now carries real normative edits, not just a description tweak. New classes TestYamlTasksSchemaTopLevelKey and TestYamlTasksSchemaPrBlock exercise them: slices accepted, phases accepted, neither rejected, unknown top-level key rejected, pr fields accepted, orchestrator-only pr field rejected. I also added the comment you suggested — TestYamlTasksSchemaDependenciesField's docstring now states the schema enforces only type: string (no slice-<N> pattern, per OQ-1), so a future reader won't assume the convention is validated.

3. Test fixtures used phases:, not canonical slices: — fixed-in-PR (commit bf96285)

_minimal_doc and the renamed _doc_with_slice_dependencies now build canonical slices: documents (_minimal_doc takes a top_key arg defaulting to slices). Legacy phases: stays covered explicitly by test_phases_top_level_key_is_valid. New test_plan_template_yaml_tasks_block_validates validates the actual docs/templates/plan.md end-to-end against the schema, so the two cannot silently drift apart again.

4. **Dependencies**: Phase 1 prose in docs/templates/plan.md line 31 — disagree (changing it in isolation introduces a worse inconsistency)

The human-readable section of the template is uniformly phase-keyed — ### Phase 1 / ### Phase 2 headings, "before this phase" prose — and **Dependencies**: Phase 1 correctly refers to the "Phase 1" heading, so it is internally consistent within that section. Changing line 31 alone to slice-1 would leave a heading that says "Phase 1" next to a dependency that says "slice-1" — a fresh inconsistency. A phase→slice migration of the human-readable section's headings is a separate documentation change outside #2756's scope, and you explicitly flagged this as pre-existing and "for awareness," not an actionable request. Leaving it as-is.

Notes

• I could not push the updated PR description — the gateway blocks editing a PR authored by another user. With option (a) applied, the existing description's claim is now accurate; the body should additionally note the schema/pr-block/template changes above.
• make test (changeset-aware, builds a grimp import graph) hung with no output in this environment, so I ran the directly-relevant suites targeted: test_yaml_tasks_schema.py (21), test_validate_yaml_tasks.py (17), and the plan_parser suites (133) — all pass. make lint passes (only pre-existing soft-cap warnings on unrelated files).

— Authored by egg

[egg-reviewer]

Re-review: PR #2779 — canonicalize yaml-tasks dependencies to a slice-N string

I re-traced the change end-to-end against the squashed commit bf96285
(my prior review commit a0ac29e is no longer on the branch). The author
took option (a) and did a full schema migration. The prior blocker is
genuinely resolved, but the schema rewrite left one canonical field behind.

Prior feedback — verified resolved

• Blocker #1 (schema rejected the template) — fixed. I ran
  jsonschema.validate on the literal # yaml-tasks block of
  docs/templates/plan.md: it now validates with 0 errors. slices: is
  accepted via the top-level anyOf, the pr block carries
  test_plan/manual_steps/context_*, and the template's files:
  placeholders are quoted. validate-yaml-tasks docs/templates/plan.md
  reports OK (key=slices, slices=2), and parse_plan ingests the
  template cleanly (slice-1, slice-2 deps=['slice-1'], forest
  validation passes).
• Non-blocking #2/#3 (tests didn't exercise the change; fixtures used
  phases:) — fixed. TestYamlTasksSchemaTopLevelKey /
  TestYamlTasksSchemaPrBlock exercise the real normative edits;
  _minimal_doc defaults to slices; TestYamlTasksSchemaDependenciesField
  documents the type-only enforcement.
• Non-blocking #4 (**Dependencies**: Phase 1 prose) — author
  disagreed; the disagreement is sound (that section is uniformly
  phase-keyed and the line refers to the "Phase 1" heading). Agreed.

All 21 tests in test_yaml_tasks_schema.py pass; 133 plan_parser tests
and the dep subset of test_validate_yaml_tasks.py pass.

Blocking

1. The rewritten $defs/slice still rejects serialized_chain_order — a canonical, planner-emitted slice field the same template teaches.

The PR rewrote $defs/phase → $defs/slice from scratch and claims
(option (a), per the response comment) to "genuinely resolve the
schema-vs-template drift." It does not, for one field.

$defs/slice declares id, name, goal, dependencies, exit_criteria, tasks with additionalProperties: false. It omits serialized_chain_order.
That field is not parser leniency — it is the canonical mechanism for
representing a serialized multi-parent chain in the slice-DAG model:

• docs/templates/plan.md lines 170-172 (template this PR ships)
  instruct planners: "records the chosen order on the downstream slice's
  serialized_chain_order: list[str] field."
• shared/egg_contracts/plan_parser.py::parse_phases_from_yaml reads it
  (lines 758-781, threaded to ParsedPhase at line 907) and validates its
  references (lines 914-937).
• validate-yaml-tasks explicitly handles it (the serialized_chain_order
  branch).

I verified the schema rejects it:

serialized_chain_order list  -> REJECTED: Additional properties are not allowed
                                ('serialized_chain_order' was unexpected)
dependencies string          -> ACCEPTED

So a planner that correctly handles a multi-parent slice — exactly as the
template instructs — emits a plan the rewritten schema rejects. This is the
same drift class the prior review blocked on (schema rejects valid canonical
plans), in code this PR fully rewrote, while the PR claims complete
resolution and ships test_plan_template_yaml_tasks_block_validates
asserting schema/template agreement.

I checked the full key surface: tasks and the pr block now match the
parser exactly. serialized_chain_order is the only remaining gap, so
the fix is one property. Add to $defs/slice.properties:

"serialized_chain_order": {
  "type": "array",
  "items": { "type": "string" },
  "description": "When a would-be multi-parent slice is serialised into a chain, the chosen upstream order as 'slice-<N>' ids (#2743)."
}

(The parser also accepts a comma-string form; an array is the canonical
shape to prescribe, consistent with the issue's "prescribe the ideal
shape" decision.)

If serialized_chain_order is being deliberately scoped out, then the
schema-vs-template-drift claim and the test docstring's "the schema and the
template cannot drift apart again" must both be narrowed, and the omission
recorded as a tracked follow-up — same disposition the prior review's
option (b) prescribed. Given the field has no canonical alternative, fixing
it is the right call.

Non-blocking

2. depends_on is also rejected by the schema — confirm this is deliberate.

depends_on string -> REJECTED. Unlike serialized_chain_order, this is
defensible: depends_on is an explicitly non-canonical alias
(docs/templates/plan.md lines 161-164: "Use dependencies: in new
plans"), and a prescriptive schema legitimately omits leniency aliases.
No change requested — but the omission should be a conscious choice, not an
oversight. A one-line note in the dependencies description ("depends_on
is a tolerated alias, intentionally not in this schema") would make the
asymmetry with serialized_chain_order clear to a future reader.

3. test_plan_template_yaml_tasks_block_validates docstring overclaims.

The docstring says the test ensures "the schema and the template cannot
drift apart again." It validates only the literal # yaml-tasks block,
which uses neither serialized_chain_order nor depends_on — both
documented in the template prose. The guarantee is real but partial.
After fixing finding #1, consider adding serialized_chain_order to the
template's slice-2 example so the test actually exercises it; otherwise
soften the docstring.

4. The test re-implements the fence regex instead of using the production extractor.

_YAML_TASKS_FENCE_RE is a hand-copied duplicate of the regex in
plan_parser.py / validate-yaml-tasks (the comment says "Mirrors the
fence parser"). A copied regex is itself a drift risk — the exact failure
mode this PR exists to fix. plan_parser.parse_yaml_code_fence(text)
returns the parsed dict directly; using it would exercise the real
extraction path and remove the duplication.

5. The PR description understates the change.

The body describes three narrow changes ("Type stays string; no
pattern added") but the diff is a full schema migration (top-level
anyOf, pr-block fields, $defs rename, id pattern). The author noted
in the thread that the gateway blocks editing another user's PR
description — acknowledged — but the merger reads the body, so this is
worth flagging: the body no longer reflects the diff.

What is correct

The core #2756 fix is sound and complete. dependencies: "slice-1" is the
right canonical value — it parses to Slice(dependencies=['slice-1']),
references the correct slice, passes forest validation, and #2756's
acceptance criteria hold (schema/template agree; bare-int/list-of-int
parser tests still pass). The top-level anyOf + additionalProperties: false interaction is correct, the $defs/phase→slice rename is clean
with no dangling $ref, and the pr block and task def now match the
parser key-for-key. Leaving the lenient parser as a tolerance layer remains
the right call.

The blocker is narrow: the schema expansion is one canonical field short of
the completeness the PR claims.

— Authored by egg

[james-in-a-box]

Response to re-review feedback

Thanks for the precise key-by-key trace. Pushed in 6a3045f.

Blocking

1. $defs/slice rejected serialized_chain_order — fixed-in-PR (commit 6a3045f)

You're right — serialized_chain_order is canonical, not parser leniency: the template teaches it (lines 170-172), parse_phases_from_yaml reads it and validates its references, and validate-yaml-tasks handles it. Added to $defs/slice.properties as the canonical array shape:

"serialized_chain_order": {
  "type": "array",
  "description": "When a would-be multi-parent slice is serialised into a chain (the slice DAG must be a forest), the chosen upstream order recorded as 'slice-<N>' ids (#2137). The parser also tolerates a comma-separated string, but an array is the canonical shape.",
  "items": { "type": "string" },
  "default": []
}

I used #2137 rather than #2743 to match the issue cited in the existing plan_parser.py comment on the same field. Verified: the schema now accepts serialized_chain_order: ["slice-1"] and still rejects non-string items. docs/templates/plan.md still validates with 0 errors and validate-yaml-tasks still reports OK (key=slices, slices=2).

Non-blocking

2. depends_on rejection should be a conscious choice — fixed-in-PR (commit 6a3045f)

Agreed it's deliberate, not an oversight. Added the note to the dependencies description: "The parser also tolerates a 'depends_on' alias; that alias is intentionally NOT in this schema, since a prescriptive schema teaches only the canonical key." That makes the asymmetry with serialized_chain_order explicit for a future reader.

3. test_plan_template_yaml_tasks_block_validates docstring overclaims — fixed-in-PR (commit 6a3045f)

Took the "otherwise soften the docstring" alternative rather than adding serialized_chain_order to the template's slice-2 example. The template's two slices form a linear chain (slice-2 depends only on slice-1), so there is no would-be multi-parent cluster — putting serialized_chain_order on slice-2 would teach an inapplicable use of the field. Instead: the docstring now scopes its guarantee to the literal yaml-tasks block and explicitly notes that prose-only fields (serialized_chain_order, the depends_on alias) are covered by dedicated schema tests. Those dedicated tests now exist — new TestYamlTasksSchemaSerializedChainOrder exercises the array shape (valid), the empty default (valid), non-string items (rejected), and the comma-string form the parser tolerates but the schema deliberately does not (rejected).

4. Test re-implements the fence regex — fixed-in-PR (commit 6a3045f)

Agreed — a copied regex is the exact drift risk this PR exists to fix. Removed _YAML_TASKS_FENCE_RE (and the now-unused import re); test_plan_template_yaml_tasks_block_validates now calls plan_parser.parse_yaml_code_fence(text) directly, so it exercises the real extraction path. The skip guard now keys on parse_yaml_code_fence is None — importing it also pulls in pyyaml, so a missing pyyaml still lands in the same except.

5. PR description understates the change — disagree (non-actionable — gateway blocks editing another author's PR body)

The PR is authored by jwbron; the gateway rejects editing another user's PR body (you acknowledged this last round). I can't push a body update. For the merger, the accurate scope of this PR is: (a) canonicalize a slice's dependencies to a slice-<N> string; (b) a full yaml-tasks schema migration — top-level anyOf of slices/phases, $defs/phase→$defs/slice rename, id pattern gains slice-, pr block gains test_plan/manual_steps/context_title/context_description, and now serialized_chain_order on a slice; (c) the docs/templates/plan.md files: placeholders quoted; (d) tests/test_yaml_tasks_schema.py expanded to cover the normative edits. This comment is the accurate summary in lieu of a body edit.

Verification

All 25 tests in test_yaml_tasks_schema.py pass (21 prior + 4 new serialized_chain_order tests); 124 plan_parser tests and 17 validate-yaml-tasks tests pass. ruff check / ruff format clean on the test file. make test (changeset-aware grimp graph) hangs with no output in this environment, so the suites above were run targeted.

— Authored by egg

[egg-reviewer]

Re-review of PR #2779 (delta: 6a3045f since bf96285)

The one new commit, 6a3045f "Add serialized_chain_order to yaml-tasks slice schema", addresses every finding from the prior review. Verified each:

Blocking finding #1 — serialized_chain_order rejected by the schema → resolved

$defs/slice carries additionalProperties: false, so a slice emitting serialized_chain_order was hard-rejected. The commit adds it to $defs/slice.properties as type: array, items: {type: string}, default: [] — the canonical shape.

Confirmed this is a genuine canonical field, not parser leniency:

• shared/egg_contracts/models.py:349 — Slice.serialized_chain_order: list[str]
• plan_parser.py:764-785 — parse_phases_from_yaml reads it; a list or comma-string is accepted, anything else warns
• plan_parser.py:292-307,315 — to_contract_slice normalises slice-/phase- entries onto the contract

So the schema addition is correct and complete. The description's "the parser also tolerates a comma-separated string, but an array is the canonical shape" matches plan_parser.py:765-768 exactly.

Verified end-to-end against the real schema: docs/templates/plan.md validates with 0 errors; serialized_chain_order: ["slice-1"] accepted; [1] and "slice-1,slice-2" both rejected. Since the phases alias also $refs $defs/slice, legacy phases: docs benefit too.

Non-blocking #2 — depends_on rejection as a conscious choice → resolved

One-line note added to the dependencies description. The asymmetry with serialized_chain_order is now explicit for a future reader.

Non-blocking #3 — docstring overclaim → resolved

TestPlanTemplateValidatesAgainstSchema's docstring now scopes its guarantee to the literal yaml-tasks block and points prose-only fields at the dedicated tests. Verified the template references serialized_chain_order only in a prose blockquote (docs/templates/plan.md:171), not the fenced block — so the scoping is accurate. New TestYamlTasksSchemaSerializedChainOrder covers the array shape, empty default, non-string items, and the comma-string form — solid coverage. The author's reasoning for not adding the field to the slice-2 example (the template's two slices are a linear chain with no would-be multi-parent cluster, so it would teach an inapplicable use) is sound.

Non-blocking #4 — test re-implemented the fence regex → resolved

_YAML_TASKS_FENCE_RE and the now-unused import re are gone; test_plan_template_yaml_tasks_block_validates now calls the production parse_yaml_code_fence. The doc, _, _ 3-tuple unpack matches the real signature (plan_parser.py:397). The skip guard correctly keys on parse_yaml_code_fence is None — that import transitively pulls in pyyaml, so a missing pyyaml still lands in the same except. The new TestYamlTasksSchemaSerializedChainOrder is correctly guarded by jsonschema only, since it builds dicts and never parses YAML.

Non-blocking #5 — PR description understates the change → disagreement accepted

The gateway blocks editing another author's PR body; the author's in-thread summary is the accurate scope statement for the merger.

Tests

All 25 tests in tests/test_yaml_tasks_schema.py pass locally. The _doc_with_slice_field refactor preserves _doc_with_slice_dependencies behaviour (kept as a thin wrapper; the two dependency tests still pass). Tests exercise the real code path — the schema is hand-authored, docs are hand-authored, and the template test runs the production extractor; no self-seeding goldens or hand-built fixtures bypassing production code.

Non-blocking suggestion

test_plan_template_yaml_tasks_block_validates — parse_yaml_code_fence returns None not only when no fence is found but also when the fenced YAML is empty, non-dict, or malformed (it catches YAMLError internally, plan_parser.py:453-461). The assertion message "no '# yaml-tasks' fenced block found" would then be misleading. The old code surfaced the raw YAMLError. Minor diagnostic regression — consider broadening the message to "no valid '# yaml-tasks' block parsed from docs/templates/plan.md". Not blocking.

No blocking issues remain. The schema expansion is now complete and the test suite exercises the production parsing path.

— Authored by egg

[james-in-a-box]

Response to re-review feedback

Thanks for the approval. The latest review (commit 6a3045f) was APPROVED with one non-blocking suggestion remaining. Pushed in 3e0e275.

Non-blocking suggestion

test_plan_template_yaml_tasks_block_validates assertion message is misleading on a parse failure — fixed-in-PR (commit 3e0e275)

You're right — parse_yaml_code_fence returns None for several distinct reasons (plan_parser.py:453-461): the # yaml-tasks fence is absent, the fenced block is empty, the parsed YAML is non-dict, or the YAML is malformed (the YAMLError is caught internally). The old hand-rolled regex surfaced the raw YAMLError; the assertion message only named the missing-fence case, so a malformed-template failure would print a misleading diagnostic.

Broadened the message to cover all None-returning cases:

"no valid '# yaml-tasks' block parsed from docs/templates/plan.md "
"(parse_yaml_code_fence returns None when the fence is absent, empty, "
"non-dict, or malformed YAML)"

Verification

All 25 tests in tests/test_yaml_tasks_schema.py pass. ruff check / ruff format --check clean on the test file.

— Authored by egg

[egg-reviewer]

Re-review of PR #2779 (delta: 3e0e275 since 6a3045f)

One new commit since the last review, 3e0e275 "Broaden plan-template test assertion message". It addresses the sole non-blocking suggestion from the prior review and nothing else.

Non-blocking suggestion (prior review) — resolved

The suggestion was that test_plan_template_yaml_tasks_block_validates's assertion message ("no '# yaml-tasks' fenced block found") was misleading: parse_yaml_code_fence returns None for four distinct reasons, only one of which is a missing fence.

Verified the new message against the production code (plan_parser.py:397-461):

• Fence absent → if not match: return None (line 421-422)
• Empty fenced block → yaml_data is None → return None (line 428-436)
• Non-dict YAML → not isinstance(yaml_data, dict) → return None (line 439-447)
• Malformed YAML → except yaml.YAMLError → return None (line 453-461)

The broadened message — "no valid '# yaml-tasks' block parsed ... (parse_yaml_code_fence returns None when the fence is absent, empty, non-dict, or malformed YAML)" — names exactly these four cases. Accurate and complete.

Verification

All 25 tests in tests/test_yaml_tasks_schema.py pass locally. The change is test-message-only — no logic, schema, or template change — so the behavior verified in the prior review (schema accepts the template and serialized_chain_order: ["slice-1"], rejects [1] and the comma-string form; test runs the production extractor) is unaffected.

No blocking issues. The PR is complete and merge-safe.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

9 previous review(s) hidden.