Fix #2870: emit canonical 'dependencies' in slice scaffold; warn on dropped slice keys by jwbron · Pull Request #2872 · jwbron/egg

jwbron · 2026-05-29T05:57:37Z

Summary

Fixes #2870. The plan→implement contract populator silently dropped the slice dependency chain when the architect scaffold expressed ordering via parent_slice_id. plan_parser reads slice edges only from dependencies (or the depends_on alias, #2743) — never parent_slice_id — so a multi-slice linear chain parsed to all-roots and ran concurrently, guaranteeing integration-branch conflicts for overlapping slices.

Root cause

A prompt-side vocabulary drift between two commits:

Fix #2756: canonicalize yaml-tasks dependencies to a slice-N string #2779 (2026-05-22) deliberately settled dependencies as the canonical single-parent key and forbade parent_slice_id in .egg/schemas/yaml-tasks.schema.json (additionalProperties: false). Parser and schema agree on dependencies.
Fix #2809: architect owns slice composition; risk_analyst becomes dual-role plan reviewer #2821 (2026-05-27) introduced the architect slice scaffold and told the architect + task_planner prompts to emit/preserve parent_slice_id — a key neither the parser, the schema, nor even the task_planner's own Slice-DAG worked example use.

The architect faithfully emitted parent_slice_id; the parser ignored it; the edge vanished. Schema validation would have caught it, but the schema is only enforced in tests, never at parse/populate time → silent drop.

Fix (two layers)

1. Align the prompts to the canonical vocabulary (the actual fix). The architect scaffold and task_planner copy instructions now emit dependencies: slice-<N> (omit for roots), matching the schema, the parser, and the task_planner Slice-DAG worked example. No schema or parser vocabulary change — parent_slice_id stays out, as #2779 intended. A single-parent forest is fully expressible via dependencies.

2. Make the next drift loud, not silent (defense-in-depth). plan_parser now emits a ParseWarning when a slice carries a key outside the set it consumes (id, name, goal, dependencies, depends_on, serialized_chain_order, exit_criteria, tasks). This "fires the mechanism that already exists" — the schema encodes exactly this rule but was never wired into parse time — and is vocabulary-agnostic: a future stray key surfaces instead of taking its data with it.

I deliberately did not add an "all-roots → NACK" preflight (issue option c): a multi-root forest is legitimate (independent parallel slices), so it would false-positive. The unknown-key warning is the false-positive-free net that catches this bug class precisely.

Tests

plan_parser warns on parent_slice_id while still documenting the drop (deps stay empty); no false-positive on the full known-key set.
Prompt tests assert the architect scaffold emits dependencies: slice-1 and no longer contains parent_slice_id; synthesizer pass-through fixture updated to the canonical key.
Affected suites green locally: tests/shared/egg_contracts/test_plan_parser.py (126), orchestrator/tests/test_pipeline_prompts.py + dependency + schema tests (488). Lint clean.

Notes

Per the paused issue-2777-replan, this is one of three orchestrator bugs gating that resume — landing it unblocks the dependency-drop side.

…nknown slice keys The plan→implement contract populator silently dropped the slice dependency chain whenever the architect scaffold expressed ordering via ``parent_slice_id``. ``plan_parser`` reads slice edges only from ``dependencies`` (or the ``depends_on`` alias, #2743) and never ``parent_slice_id``, so multi-slice linear chains parsed to all-roots and ran concurrently — guaranteeing integration-branch conflicts for overlapping slices. Root cause: #2779 (2026-05-22) settled ``dependencies`` as the canonical single-parent key and forbade ``parent_slice_id`` in ``yaml-tasks.schema.json`` (``additionalProperties: false``). #2821 (2026-05-27) then introduced the architect slice scaffold and told the architect + task_planner prompts to emit/preserve ``parent_slice_id`` — a key the parser, schema, and even the task_planner's own worked example do not use. This is that prompt-side drift. Fix (two layers): - Align the prompts back to the canonical vocabulary: the architect scaffold and task_planner copy instructions now emit ``dependencies: slice-<N>`` (omit for roots), matching the schema, the parser, and the task_planner Slice-DAG worked example. No schema or parser vocabulary change — ``parent_slice_id`` stays out, as #2779 intended. - Make the next drift loud, not silent: ``plan_parser`` now emits a ParseWarning when a slice carries a key outside the set it consumes. The schema already encodes this rule but is only enforced in tests, never at parse/populate time — so an unrecognized key (e.g. a future stray ``parent_slice_id``) would otherwise vanish with its data. Tests: parser warns on ``parent_slice_id`` while still documenting the drop; no false-positive on the full known-key set; prompt tests assert the scaffold emits ``dependencies: slice-1`` and no ``parent_slice_id``.

egg-reviewer

Approve — fix is correct and works end-to-end

I traced the full plan→implement chain and the fix closes #2870 on the happy path:

architect prompt now emits dependencies: slice-1 → task_planner copies it verbatim → parse_phases_from_yaml reads dependencies → to_contract_slice normalises "slice-1" → ["slice-1"] → runtime parent_slice_id = slice_obj.dependencies[0] (orchestrator/routes/pipelines.py:2524) → SliceScheduler builds DAG edges from slice.dependencies. The prompt vocabulary now agrees with the parser, the schema (.egg/schemas/yaml-tasks.schema.json), and the runtime — no more silent edge drop.

Verified:

_KNOWN_SLICE_KEYS exactly matches the schema's slice properties (id, name, goal, dependencies, serialized_chain_order, exit_criteria, tasks) plus the depends_on alias (#2743). No missing/extra key → no false positives.
The new ParseWarning is actually surfaced, not collected-and-dropped: _populate_contract_from_plan logs every result.warnings entry unconditionally at orchestrator/routes/pipelines.py:20109-20115 — i.e. in the exact populator where #2870 occurred. The "loud, not silent" claim holds.
Warning placement is safe — it runs after the dict / id / duplicate-id guards, so phase_data is always a dict and id is always present.
Tests exercise the production code path (parse_phases_from_yaml), assert both the drop (deps == []) and the warning, include a false-positive guard over the full known-key set, and the prompt tests assert the real prompt builder's output. No self-seeding goldens, no hand-built fixtures bypassing the parser, no name-vs-behaviour contradiction.
Remaining parent_slice_id references in code/tests are the runtime DAG field (correctly derived from dependencies[0]), not the scaffold vocabulary — those are fine to keep.

Non-blocking suggestions

1. Stale docs still describe the old parent_slice_id scaffold vocabulary. The PR's whole thesis is aligning on the canonical dependencies key, but these references now contradict the fix and could lead a future contributor to re-introduce parent_slice_id into the prompt — recreating #2870. Since the PR is already the authority on this vocabulary, it's the natural place to update them:

docs/architecture/slice-dag.md:630 — "The scaffold encodes slice id / name / goal / parent_slice_id"
docs/architecture/slice-dag.md:637 — "(same id / name / goal / parent_slice_id, same order)"
docs/architecture/slice-dag.md:646 — "The architect's scaffold encodes this via parent_slice_id; the planner preserves it."
docs/reference/agent-roles.md:95 — "Binding slice scaffold (id / name / goal / parent_slice_id...)"

(docs/architecture/slice-dag.md:191's (slice_id, parent_slice_id) is the runtime iter_ready() tuple — leave it.)

2. The defense-in-depth layer only logs; it neither recovers nor blocks. If the architect drifts again and emits an unknown ordering key, the dependency chain is still dropped, the slices still run concurrently (→ integration-branch conflicts), and the only signal is a logger.warning line — the PR-body parse-warning banner only renders when the stub fallback fires (pipelines.py:10079). An operator not tailing orchestrator logs sees nothing until the conflict surfaces. Rejecting parser-mapping (option a) and the all-roots preflight (option c) is defensible and well-argued in the PR, so this is not a blocker — but note the residual failure mode is unchanged; only its observability improved, and only for a log-watcher. Worth considering whether the unknown-key case on a multi-slice plan deserves a louder signal than a log line.

3. Minor: the warning message hardcodes parent_slice_id as the example ("a key like 'parent_slice_id' is silently dropped") even when the actual stray key is something else. Illustrative, but slightly misleading when a different unknown key triggers it.

— Authored by egg

Replace the remaining `parent_slice_id` scaffold-vocabulary references in docs (slice-dag.md, agent-roles.md) with the canonical `dependencies` key this PR settles on, so they no longer contradict the fix or risk re-introducing #2870. The runtime `iter_ready()` tuple reference is left as-is (it is the DAG field, not the scaffold key). Generalize the unknown-slice-key ParseWarning message so it points at the actual stray key(s) instead of hardcoding `parent_slice_id` as the example, which was misleading when a different unknown key triggered it.

james-in-a-box · 2026-05-29T06:24:45Z

Thanks for the thorough trace — appreciate the end-to-end verification. Disposition per non-blocking suggestion:

1. Stale parent_slice_id doc references — fixed-in-PR (commit 025e544).
Updated all four scaffold-vocabulary references to the canonical dependencies key:

docs/architecture/slice-dag.md:630 (scaffold encodes … / dependencies)
docs/architecture/slice-dag.md:637 (planner copies … / dependencies)
docs/architecture/slice-dag.md:646 (now "via a single-parent dependencies id (slice-<N>)")
docs/reference/agent-roles.md:95 (binding scaffold … / dependencies)

Left slice-dag.md:191 as-is per your note — that's the runtime iter_ready() tuple (slice_id, parent_slice_id), the DAG field, not the scaffold key.

3. Warning message hardcoded parent_slice_id — fixed-in-PR (commit 025e544).
The message now points at the actual stray key(s) instead of citing parent_slice_id as a fixed example:

Slice N has unrecognized key(s) {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).

{unknown_keys} already enumerates the real offenders, so the message is now accurate regardless of which key drifts.

2. Defense-in-depth only logs, no louder signal — disagree (warning-only is the correct layer for this PR's scope; a louder channel is out of scope and net-negative here).
You flagged this as defensible and not a blocker, and I think warning-only is in fact the right call rather than a gap to close:

The warning is emitted as a structured log event (event="Plan parse warning" with warning_message + warning_context) at pipelines.py:20109-20115 — the exact populator where Plan→implement: slice dependency chain dropped when yaml-tasks slices use parent_slice_id (parser reads dependencies/depends_on only) #2870 occurred. That's the operator alerting surface (you alert on level=warning event="Plan parse warning" in log aggregation), not "tail and hope."
The two "louder" channels both have real downsides that fall outside fixing the vocabulary drift:
- PR-body banner (pipelines.py:10079) is, by design (PR body silently falls back to 'Issue #N' stub when plan parse emits warnings #1975), the tier-3 stub-fallback path. It lives in a separate builder that never sees populate-time warnings; threading them through is a distinct observability feature, not a Plan→implement: slice dependency chain dropped when yaml-tasks slices use parent_slice_id (parser reads dependencies/depends_on only) #2870 fix.
- Plan-review NACK via plan_review_feedback (the forest-violation channel) would block the plan on any unrecognized key — re-introducing precisely the false-positive risk this PR deliberately avoided, and which you agreed was right when rejecting options (a) and (c). A benign extra annotation key would NACK an otherwise valid plan.

So the warning stays false-positive-free and precise, on the standard observability surface; escalating the channel is a separate decision that isn't required to close #2870. Happy to file a follow-up if you'd prefer the louder-signal question tracked separately — just say the word.

— Authored by egg

james-in-a-box · 2026-05-29T06:25:06Z

Thanks for the thorough trace — appreciate the end-to-end verification. Disposition per non-blocking suggestion:

1. Stale parent_slice_id doc references — fixed-in-PR (commit 025e544).
Updated all four scaffold-vocabulary references to the canonical dependencies key:

docs/architecture/slice-dag.md:630 (scaffold encodes … / dependencies)
docs/architecture/slice-dag.md:637 (planner copies … / dependencies)
docs/architecture/slice-dag.md:646 (now "via a single-parent dependencies id (slice-<N>)")
docs/reference/agent-roles.md:95 (binding scaffold … / dependencies)

Left slice-dag.md:191 as-is per your note — that's the runtime iter_ready() tuple (slice_id, parent_slice_id), the DAG field, not the scaffold key.

3. Warning message hardcoded parent_slice_id — fixed-in-PR (commit 025e544).
The message now points at the actual stray key(s) instead of citing parent_slice_id as a fixed example:

Slice N has unrecognized key(s) {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).

{unknown_keys} already enumerates the real offenders, so the message is now accurate regardless of which key drifts.

2. Defense-in-depth only logs, no louder signal — disagree (warning-only is the correct layer for this PR's scope; a louder channel is net-negative here).
You flagged this as defensible and not a blocker, and I think warning-only is in fact the right call rather than a gap to close:

The warning is emitted as a structured log event (event="Plan parse warning" with warning_message + warning_context) at pipelines.py:20109-20115 — the exact populator where Plan→implement: slice dependency chain dropped when yaml-tasks slices use parent_slice_id (parser reads dependencies/depends_on only) #2870 occurred. That's the operator alerting surface (you alert on level=warning event="Plan parse warning" in log aggregation), not "tail and hope."
The two "louder" channels both have real downsides that fall outside fixing the vocabulary drift:
- PR-body banner (pipelines.py:10079) is, by design (PR body silently falls back to 'Issue #N' stub when plan parse emits warnings #1975), the tier-3 stub-fallback path. It lives in a separate builder that never sees populate-time warnings; threading them through is a distinct observability feature, not a Plan→implement: slice dependency chain dropped when yaml-tasks slices use parent_slice_id (parser reads dependencies/depends_on only) #2870 fix.
- Plan-review NACK via plan_review_feedback (the forest-violation channel) would block the plan on any unrecognized key — re-introducing precisely the false-positive risk this PR deliberately avoided, and which you agreed was right when rejecting options (a) and (c). A benign extra annotation key would NACK an otherwise valid plan.

So the warning stays false-positive-free and precise, on the standard observability surface; escalating the channel is a separate decision that isn't required to close #2870.

— Authored by egg

james-in-a-box · 2026-05-29T06:25:56Z

Feedback contract violation detected.

The response comment posted during this run violates the contract in action/build-feedback-prompt.sh. Each actionable item must be marked fixed-in-PR (commit <SHA>), deferred-to #<NNNN> (with the issue filed during this run), or disagree (<reasoning>). Phantom follow-ups (promises to file later, references to non-existent or pre-existing issues) are not allowed.

Violations:

forbidden phrase(s): file a follow-up

View run logs

— Authored by egg

The prior Integration Tests run failed during k3s image import with 'no space left on device' (3/3 import attempts) — a runner infra flake, not a code failure. The same branch passed the Test workflow on the prior commit, and this PR touches no integration-test infrastructure.

james-in-a-box · 2026-05-29T06:53:33Z

Autofix tracking

{"Test/Integration Tests / Integration Tests": 1}

egg-reviewer

No agent-mode design concerns.

This PR aligns the architect/task_planner prompt vocabulary (parent_slice_id → dependencies) with the canonical key the plan_parser/contract populator actually reads, and adds a ParseWarning for unrecognized slice keys. From an agent-mode design perspective this is well-aligned:

The # yaml-tasks appendix is structured output with a genuine downstream machine consumer (the parser → contract that schedules slices as stacked PRs), so prescribing the exact key name is appropriate — not the "structured output for humans" anti-pattern.
The fix removes a silent drop where the agent's output was effectively discarded; making the prompt vocabulary match the consumer helps the agent operate effectively.
The unknown-key warning is sound defense-in-depth — it surfaces future drift loudly rather than swallowing it, without false-positive-prone hard rejection.

No pre-fetching, post-processing middlemen, rigid-procedure, prompt-level-security, direct-API, or pinned-model-identifier issues.

— Authored by egg

egg-reviewer

Re-review — all delta verified, suggestions resolved correctly

Since my prior review at 925d6c86, the delta is exactly two commits: 025e544 (docs + warning-message generalization) and 7701c4b (empty CI re-trigger, no file changes). I traced both end-to-end against the three non-blocking suggestions I raised. All are properly resolved — not superficially.

Suggestion 1 (stale parent_slice_id scaffold docs) — fixed, complete.
All four scaffold-vocabulary references now read dependencies:

docs/architecture/slice-dag.md:630 (scaffold encodes … / dependencies)
docs/architecture/slice-dag.md:637 (planner copies … / dependencies)
docs/architecture/slice-dag.md:646 ("via a single-parent dependencies id (slice-<N>)")
docs/reference/agent-roles.md:95 (binding scaffold … / dependencies)

Verified the cleanup is exhaustive: a repo-wide grep for parent_slice_id leaves only legitimate references — the runtime iter_ready() tuple at slice-dag.md:191 (which I'd explicitly flagged to leave), the runtime DAG field across slice_scheduler.py / pipelines.py (correctly derived from dependencies[0] at pipelines.py:2524), and accurate #2870 failure-mode comments at plan_parser.py:98,739. Docs and code no longer contradict; no path remains that would lead a contributor to re-introduce the scaffold key.

Suggestion 3 (warning hardcoded parent_slice_id as a fixed example) — fixed.
plan_parser.py:748-752 now reads "any data they carry (e.g. slice ordering) is silently dropped" and relies on the {unknown_keys} enumeration to name the actual offender(s). Accurate regardless of which key drifts.

I confirmed the message change is behaviorally inert against the tests: test_parse_phases_from_yaml_warns_on_parent_slice_id (line 1036) asserts "parent_slice_id" in w.message — that substring now comes from the {unknown_keys} list, not the removed hardcoded example, so the assertion still holds. The false-positive guard test_parse_phases_from_yaml_no_warn_on_known_keys still triggers on the unchanged "unrecognized key" substring. No golden or fixture pins the removed phrase, so nothing breaks.

Suggestion 2 (warning-only, no louder signal) — disagreed, accepted.
I flagged this as defensible and explicitly not a blocker; the author's reasoning is sound. The warning is a structured log event (event="Plan parse warning") emitted at the exact populator where #2870 occurred — the standard operator alerting surface, not "tail and hope." Both "louder" channels carry real downsides outside the scope of fixing the vocabulary drift: the PR-body banner is the tier-3 stub-fallback path (never sees populate-time warnings), and a plan-review NACK on any unknown key would re-introduce the false-positive risk this PR deliberately rejected (options a/c). Warning-only is the correct layer here.

Other checks:

7701c4b is an empty CI re-trigger — nothing to review.
All CI checks green, including the Integration Tests run that previously failed and was autofixed.
No new logic introduced in this delta; the fix's core (architect scaffold → dependencies → parser → runtime DAG) is unchanged from my prior end-to-end-verified approval.

No blocking issues. Approving.

— Authored by egg

james-in-a-box · 2026-05-29T06:58:34Z

egg review completed. View run logs

6 previous review(s) hidden.

…ropped slice keys (#2872) * Fix #2870: emit canonical 'dependencies' in slice scaffold; warn on unknown slice keys The plan→implement contract populator silently dropped the slice dependency chain whenever the architect scaffold expressed ordering via ``parent_slice_id``. ``plan_parser`` reads slice edges only from ``dependencies`` (or the ``depends_on`` alias, #2743) and never ``parent_slice_id``, so multi-slice linear chains parsed to all-roots and ran concurrently — guaranteeing integration-branch conflicts for overlapping slices. Root cause: #2779 (2026-05-22) settled ``dependencies`` as the canonical single-parent key and forbade ``parent_slice_id`` in ``yaml-tasks.schema.json`` (``additionalProperties: false``). #2821 (2026-05-27) then introduced the architect slice scaffold and told the architect + task_planner prompts to emit/preserve ``parent_slice_id`` — a key the parser, schema, and even the task_planner's own worked example do not use. This is that prompt-side drift. Fix (two layers): - Align the prompts back to the canonical vocabulary: the architect scaffold and task_planner copy instructions now emit ``dependencies: slice-<N>`` (omit for roots), matching the schema, the parser, and the task_planner Slice-DAG worked example. No schema or parser vocabulary change — ``parent_slice_id`` stays out, as #2779 intended. - Make the next drift loud, not silent: ``plan_parser`` now emits a ParseWarning when a slice carries a key outside the set it consumes. The schema already encodes this rule but is only enforced in tests, never at parse/populate time — so an unrecognized key (e.g. a future stray ``parent_slice_id``) would otherwise vanish with its data. Tests: parser warns on ``parent_slice_id`` while still documenting the drop; no false-positive on the full known-key set; prompt tests assert the scaffold emits ``dependencies: slice-1`` and no ``parent_slice_id``. * Address review: update stale parent_slice_id docs; generalize warn msg Replace the remaining `parent_slice_id` scaffold-vocabulary references in docs (slice-dag.md, agent-roles.md) with the canonical `dependencies` key this PR settles on, so they no longer contradict the fix or risk re-introducing #2870. The runtime `iter_ready()` tuple reference is left as-is (it is the DAG field, not the scaffold key). Generalize the unknown-slice-key ParseWarning message so it points at the actual stray key(s) instead of hardcoding `parent_slice_id` as the example, which was misleading when a different unknown key triggered it. * ci: re-trigger Test workflow after transient runner disk exhaustion The prior Integration Tests run failed during k3s image import with 'no space left on device' (3/3 import attempts) — a runner infra flake, not a code failure. The same branch passed the Test workflow on the prior commit, and this PR touches no integration-test infrastructure. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com>

egg-reviewer Bot approved these changes May 29, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

egg-reviewer Bot approved these changes May 29, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

egg-reviewer Bot approved these changes May 29, 2026

View reviewed changes

jwbron merged commit f73cd9e into main May 29, 2026
31 checks passed

Navigation Menu

Fix #2870: emit canonical 'dependencies' in slice scaffold; warn on dropped slice keys - #2872

Fix #2870: emit canonical 'dependencies' in slice scaffold; warn on dropped slice keys#2872
jwbron merged 3 commits into
mainfrom
egg/2870-yaml-tasks-dependency-vocab

jwbron commented May 29, 2026

Uh oh!

egg-reviewer Bot left a comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

james-in-a-box Bot commented May 29, 2026

Uh oh!

james-in-a-box Bot commented May 29, 2026

Uh oh!

james-in-a-box Bot commented May 29, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

james-in-a-box Bot commented May 29, 2026

Uh oh!

This comment has been minimized.

egg-reviewer Bot left a comment

Uh oh!

This comment has been minimized.

egg-reviewer Bot left a comment

Uh oh!

james-in-a-box Bot commented May 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

jwbron commented May 29, 2026

Summary

Root cause

Fix (two layers)

Tests

Notes

Uh oh!

egg-reviewer Bot left a comment

Choose a reason for hiding this comment

Approve — fix is correct and works end-to-end

Non-blocking suggestions

Uh oh!

This comment has been minimized.

This comment has been minimized.

james-in-a-box Bot commented May 29, 2026

Uh oh!

james-in-a-box Bot commented May 29, 2026

Uh oh!

james-in-a-box Bot commented May 29, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

james-in-a-box Bot commented May 29, 2026

Uh oh!

This comment has been minimized.

egg-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

egg-reviewer Bot left a comment

Choose a reason for hiding this comment

Re-review — all delta verified, suggestions resolved correctly

Uh oh!

james-in-a-box Bot commented May 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant