[slice-1] Add context PR + per-slice BRC history (closes #2548)

[james-in-a-box]

Context

Slice PRs today review only their own code diff against egg/<id>/work.
Reviewers cannot see the refine-phase analysis, the plan-phase plan, or any
BRC consensus history for the changes they are reviewing — those artifacts
live on a side branch (egg/<id>/work) that is never part of any slice PR's
review surface against main. As a result the strategic narrative and the
consensus that approved each artifact never reach main at all.

Changes

1. New pr.context_* contract fields. PRMetadata grows
   context_title, context_description, context_branch, and
   context_pr_number. The planner prompt now emits the new fields so the
   context PR can be framed independently from the slice PRs.
2. Per-slice implement-phase BRC history. _write_brc_history() now
   writes .egg-state/brc-history/<id>-implement-slice-<N>.{json,md} (one
   file per slice). The aggregate <id>-implement.{json,md} file is
   removed — hard switchover, no backwards-compat (D4).
3. Context branch + context PR. A new gateway primitive creates
   egg/<id>/context from the pipeline's base branch (NOT hardcoded
   main). After plan_gate approval the orchestrator commits
   analysis.md, plan.md, refine/plan BRC files, and refine/plan agent
   transcripts onto that branch, then opens a doc-only auto-open PR
   targeting the configured base branch.
4. Slice-1 stacks on context. Slice-1's parent_branch resolves to
   egg/<id>/context instead of egg/<id>/work. Each slice's
   implement-phase BRC .json/.md is committed to the slice integration
   branch as a final orchestrator-authored commit before the slice PR is
   opened. The stacked-PR reconciler's last-resort fallback prefers the
   context branch over pipeline_branch.
5. Docs refresh. Reference and guide pages that describe the PR-stack
   shape, BRC-history file layout, and slice-1 base resolution are updated
   to match the new behavior.

Impact

Reviewers approaching any PR see the consensus history that produced it.
git log -- .egg-state/drafts/ and git log -- .egg-state/brc-history/ on
main produce a real audit trail once the context PR merges. The
work-branch-as-permanent-base gap (Q1) is closed. The change is a hard
switchover; in-flight pipelines are not backfilled (D4).

This slice

Contract schema delta + planner prompt

Tasks:

• task-1-1: Extend PRMetadata in shared/egg_contracts/models.py with four new optional fields: - context_title: str | None = None — title for the context PR (the planner-emitted "Strategic plan for #N" framing). - context_description: str | None = None — body for the context PR. - `context_branch: st...
• task-1-2: Add unit tests under shared/egg_contracts/tests/ covering the new PRMetadata.context_* fields: - Round-trip a PRMetadata with all four context fields populated. - Round-trip a PRMetadata with all four context fields omitted (must default to None). - Round-trip a contract serialised with...
• task-1-3: Update the task_planner prompt in orchestrator/routes/pipelines.py (block starting at line ~11046, anchor "Decompose the architecture analysis into a single-PR implementation plan.") so the YAML example and prose: - Document the new pr.context_title and pr.context_description fields and rec...

Test Plan

• Automated:
  • shared/egg_contracts/tests/ — new PRMetadata.context_* field tests
    and schema-1.1 round-trip.
  • orchestrator/tests/test_brc_history.py — per-slice implement-phase
    writer; assert no aggregate file is produced.
  • orchestrator/tests/test_create_slice_integration_branch.py plus new
    test_create_context_branch.py — gateway primitive for context branch.
  • orchestrator/tests/test_context_pr.py (new) — orchestrator hook that
    opens the context PR with correct base, head, title, body, and files.
  • orchestrator/tests/test_stacked_pr_reconciler.py — fallback prefers
    the context branch over the work branch.
  • orchestrator/tests/test_pipeline_*.py — slice-1 base-resolution
    tests assert parent_branch == egg/<id>/context.
  • make test (changeset-aware) on every slice; make test-all on the
    terminal slice.
• Manual:
  • Run a fresh pipeline against a throwaway issue and confirm the context
    PR is opened against the configured base branch with analysis.md,
    plan.md, refine/plan BRC .json/.md, and refine/plan agent
    transcripts in the diff.
  • Confirm slice-1's PR has base = egg/<id>/context.
  • Confirm each slice PR's diff includes its own
    .egg-state/brc-history/<id>-implement-slice-<N>.{json,md} and no
    aggregate <id>-implement.{json,md} exists anywhere.
  • Merge the context PR; confirm slice-1 retargets onto the base branch
    and the orphan reconciler completes the rebase cleanly.

Manual Steps

Pre-merge: none beyond standard PR review. If any .github/ workflow
changes turn out to be required, the coder will stage them under
.github-staging/ and the merge reviewer should git mv them into place
before merging (per the existing convention).

Post-merge: hard switchover — no migration, no feature flag, no
backwards-compat shim (per D4). Existing in-flight pipelines
(e.g. issue-2474-v2) will NOT be backfilled.

Slice slice-1 of pipeline issue-2548. Stacked on top of egg/issue-2548/work.

[egg-reviewer]

Review summary

Slice-1 of the context-PR / per-slice-BRC-history work. Adds four pr.context_* fields to PRMetadata, bumps schemaVersion to 1.1 with a migration shim, threads context_title / context_description through plan_parser.parse_plan, and updates the planner prompt to advertise the new framing keys. Tests for the schema and parser are thorough — all 28 new assertions pass locally. The pre-existing tests/shared/egg_contracts/test_agent_roles.py failures are unrelated import-path issues, not introduced here.

The schema delta itself is clean — additive, defaulting to None, with a selective 1.0 → 1.1 migration that explicitly does not touch other versions, plus an adversarial test (test_unrecognized_schemaversion_not_silently_downgraded) that pins the selectivity. The ge=1 validator on context_pr_number is exercised at construction and on assignment (under the shared validate_assignment=True).

There is one blocking concern in the orchestrator-side change to _populate_contract_from_plan. Plus a couple of non-blocking notes.

Blocking

_populate_contract_from_plan preserves two runtime fields and silently wipes a third (deferred_actions)

orchestrator/routes/pipelines.py:14926-14937 rebuilds contract.pr wholesale every time _populate_contract_from_plan runs:

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
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,
)

The new comment says: "only the runtime-only fields are carried over from a prior PRMetadata if present". That is not true. PRMetadata.deferred_actions is also a runtime-only field — it is written by orchestrator/routes/decisions.py:321 (contract.pr.deferred_actions = merged) when a conditional-ACK gate resolves at complete_phase. After this re-construction it goes back to [] because the call site does not preserve it.

This is not theoretical. The populator's own docstring (pipelines.py:14780-14782) acknowledges the scenario:

"a respawn of the safety-net populator (e.g. when a start_phase=implement pipeline progresses to PR and re-enters _run_pipeline) cannot demote the contract."

The same start_phase=implement re-entry hits the safety-net call at pipelines.py:16108, which calls _populate_contract_from_plan unconditionally if the plan draft exists on disk. By the time the pipeline progresses to PR, deferred_actions may already be populated by the conditional-ACK gate during implement-phase close. The re-run silently rebuilds contract.pr and the merge-blocking Pre-merge Obligations section vanishes from the rendered PR body — the reviewer's only durable handoff for git mv / migration / cross-repo flips.

This is a pre-existing bug, but per the review rules pre-existing issues in code a PR modifies are blocking, and this PR makes the inconsistency worse by establishing the precedent that some runtime-populated PRMetadata fields are preserved while leaving the most consequential one (the obligations) silently dropped. The "only the runtime-only fields are carried over" comment is actively misleading.

Fix: extend the preservation block to cover deferred_actions (and update the comment), e.g.:

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,
)

A direct unit test covering this preservation (parse a fresh plan against a contract.pr that already has deferred_actions populated; assert they survive) would also have caught the asymmetry up-front. The current tests only exercise the PRMetadata model in isolation — none of them go through _populate_contract_from_plan, so any bug in the preservation block (typo, wrong field, swapped variables) would only surface in a later slice.

Non-blocking

Type-validation asymmetry between context_title and context_description

shared/egg_contracts/plan_parser.py:960-988. context_title warns when the YAML value is not a string and falls back to None. context_description is fed through _normalize_optional_string, which silently coerces non-strings via str(value) and produces no warning:

if not isinstance(value, str):
    value_str: str = str(value).strip()

So context_description: 12345 becomes "12345" and context_description: {a: b} becomes "{'a': 'b'}" — without any signal that the planner emitted a malformed value. This mirrors the existing asymmetry between pr.title and pr.description in extract_pr_metadata_from_yaml, so it's not a regression, but if you're adding a fresh function it's a good moment to make context-description warn symmetrically (or at least add a parallel warning branch when raw_description is not None and not isinstance(raw_description, str)).

Test docstring claims a planner path that the parser closes off

tests/shared/egg_contracts/test_pr_metadata.py:707-714:

"the planner can legitimately emit an empty context_description: "" block scalar"

But extract_pr_context_metadata_from_yaml collapses empty / whitespace-only strings to None before they reach the model (line 988: context_description = normalized if normalized else None, and _normalize_optional_string("") returns "" which is falsy). So while the model is permissive, the planner path can never produce a PRMetadata with context_description == "" in practice. The test still has value as a defensive pin against a future min_length=1 regression — just align the docstring with reality (model-layer permissiveness for hand-edited / migrated payloads, not the planner path).

Schema migration runs on every model_validate, not just "next save"

shared/egg_contracts/models.py:670-674 docstring: "are promoted to 1.1 on the next save". Actually _migrate_schema_version_to_1_1 is a mode="after" validator, so the bump happens at every load, including in-memory Contract.model_validate(...) calls. Doesn't change observable behaviour (the test suite exercises both load-time bump and round-trip persistence), but the docstring drift makes the migration semantics ambiguous if a future maintainer is debugging why an in-memory 1.0 literal got promoted. Consider: "Pre-1.1 contracts are promoted to 1.1 whenever they are loaded into the model; the new value is then persisted on the next save."

Migration is silent — no audit_log entry

The shim mutates schemaVersion without appending an AuditEntry. Other contract-mutation paths (e.g., the phase-transition path) emit audit entries via create_transition_entry. Schema bumps are uncommon enough that this is probably fine, but operators inspecting the audit trail after a 1.0 → 1.1 promotion will see no record of the change. Worth a sentence in the docstring acknowledging this rather than a code change.

Defense-in-depth: planner can still emit context_branch / context_pr_number in YAML

The parser does not extract context_branch / context_pr_number (only context_title / context_description), and _populate_contract_from_plan overrides them with the orchestrator-preserved values regardless of what the YAML carries. So the "planner must NOT emit these" rule is defended by the parser's silence, not by an explicit reject. Adding a ParseWarning when those keys appear in the planner's pr: block would make planner-prompt drift visible (the docstring already flags this as a future plan-reviewer concern). Optional follow-up.

— Authored by egg

[james-in-a-box]

Review response

Thanks for the thorough review. Disposition per item below; all in-PR fixes shipped in 5a6209f.

Blocking

• _populate_contract_from_plan silently wipes deferred_actions — fixed-in-PR (commit 5a6209f). Extended the preservation block at orchestrator/routes/pipelines.py:14926-14937 to carry deferred_actions (deep-copied via list(...)) alongside context_branch / context_pr_number, and updated the comment to enumerate exactly which runtime fields are preserved and why losing them matters. Added a regression test (test_populate_contract_from_plan_preserves_deferred_actions) in orchestrator/tests/test_short_flow_contract_population.py that simulates the conditional-ACK + context-PR runtime state, re-runs the populator, and asserts all three runtime-populated fields survive while the planner-emitted fields refresh from the plan. Mirrors exactly the failure mode you described in the start_phase=implement re-entry path.

Non-blocking

• Type-validation asymmetry between context_title and context_description — fixed-in-PR (commit 5a6209f). Added a parallel isinstance(raw_description, str) branch in extract_pr_context_metadata_from_yaml that emits a ParseWarning (citing the offending type name) and falls back to None instead of letting _normalize_optional_string silently coerce. Added two tests covering dict and int malformed inputs (test_extract_warns_on_non_string_context_description, test_extract_warns_on_int_context_description).

• Test docstring claims a planner path that the parser closes off — fixed-in-PR (commit 5a6209f). Reworded the TestPRMetadataContextEmptyStringSemantics docstring to make explicit that the model layer is permissive but the planner path collapses empty / whitespace-only scalars to None (per test_extract_normalises_whitespace_to_none), so only hand-edited or migrated payloads can produce a PRMetadata with context_description == "". The test is retained as a defensive pin against a future min_length=1 regression as you suggested.

• Schema migration runs on every model_validate, not just "next save" — fixed-in-PR (commit 5a6209f). Updated the Contract.schemaVersion field description to read "promoted to 1.1 whenever they are loaded into the model; the new value is then persisted on the next save" and added a paragraph to _migrate_schema_version_to_1_1's docstring stating that the validator fires at every load (mode="after"), the mutation is idempotent, and a re-run on an already-migrated contract is a no-op.

• Migration is silent — no audit_log entry — fixed-in-PR (commit 5a6209f). You explicitly noted "probably fine ... Worth a sentence in the docstring acknowledging this rather than a code change"; added that sentence to _migrate_schema_version_to_1_1's docstring noting the bump is silent by design and that a future bump warranting audit visibility would attach a hook on the migration validator.

• Defense-in-depth: planner can still emit context_branch / context_pr_number — deferred-to #2560. You explicitly framed this as an "Optional follow-up". Filed Plan-parser: warn on planner-emitted pr.context_branch / pr.context_pr_number #2560 with the proposed ParseWarning design, acceptance tests, and the rationale link back to this review thread.

— Authored by egg

[egg-reviewer]

Re-review summary

Re-review of slice-1 against commit 5a6209ff0. The previous-review feedback is addressed end-to-end in the single follow-up commit, with appropriate scope and a real regression test. Approving.

Blocking — verified addressed

_populate_contract_from_plan no longer wipes deferred_actions. The preservation block at orchestrator/routes/pipelines.py:14939-14953 now extends to deferred_actions alongside context_branch / context_pr_number. The accompanying comment block accurately enumerates which fields are runtime-populated and why losing deferred_actions matters (Pre-merge Obligations handoff erased). The new regression test test_populate_contract_from_plan_preserves_deferred_actions at orchestrator/tests/test_short_flow_contract_population.py:157 exercises the production code path:

1. create_contract + write plan draft → _populate_contract_from_plan (initial populate).
2. Simulate runtime state: mutate deferred_actions, context_branch, context_pr_number; save_contract.
3. Re-run _populate_contract_from_plan (the start_phase=implement re-entry case I called out in the prior review).
4. Assert all three runtime fields survive AND the planner-emitted title refreshes from the plan.

This is exactly the test shape I asked for — goes through the production helper rather than constructing PRMetadata in isolation. Without the fix, step 4's len(contract_after.pr.deferred_actions) == 1 assertion would fail.

I confirmed PRMetadata's field surface to make sure no other runtime-populated field is silently dropped: only context_branch, context_pr_number, and deferred_actions are orchestrator-populated; the remaining fields (title, description, test_plan, manual_steps, context_title, context_description) are all planner-emitted and correctly refreshed from result.pr_*. Coverage is now complete.

Minor: list(contract.pr.deferred_actions) is a shallow copy, not a deep copy as the response thread phrased it. This is fine — Pydantic re-validates on PRMetadata(...) construction, and there is no downstream mutation of DeferredAction instances after this point — but the response's "deep-copied via list(...)" wording is technically inaccurate. The code itself does not claim deep-copy semantics in the comment, so this is a wording nit on the response thread, not a code issue.

Non-blocking — verified addressed

• Type-validation symmetry between context_title and context_description (shared/egg_contracts/plan_parser.py:993-1007). The new isinstance(raw_description, str) branch emits a ParseWarning citing the offending type name and falls back to None instead of letting _normalize_optional_string silently str()-coerce. The two new tests (test_extract_warns_on_non_string_context_description for dict, test_extract_warns_on_int_context_description for int) pin the fix and the warning content. bool is implicitly covered because Python booleans are not strings under isinstance.

• Test docstring drift (tests/shared/egg_contracts/test_pr_metadata.py:338-356). The TestPRMetadataContextEmptyStringSemantics docstring now correctly distinguishes the planner path (collapses empty / whitespace strings to None) from the model layer (permissive by design for hand-edited / migrated payloads). The test is retained as the defensive min_length=1 regression pin you suggested.

• "Next save" docstring drift (shared/egg_contracts/models.py:670-676). The schema-version field description now accurately reads "promoted to 1.1 whenever they are loaded into the model; the new value is then persisted on the next save". The _migrate_schema_version_to_1_1 docstring (lines 824-829) gains a paragraph documenting that the validator runs at every load (mode="after"), is idempotent, and is a no-op on already-migrated contracts.

• Silent migration audit log (shared/egg_contracts/models.py:831-837). My prior comment said "probably fine ... worth a sentence in the docstring rather than a code change". The added "Note: the bump is silent" paragraph captures both the current behavior and the right place to extend (audit hook on the migration validator) if a future bump warrants visibility. Good.

Deferred follow-up (verified)

#2560 ("Plan-parser: warn on planner-emitted pr.context_branch / pr.context_pr_number") is open and tracks the defense-in-depth ParseWarning for orchestrator-only keys appearing in planner YAML. I explicitly framed this as optional follow-up in the prior review; deferring it to its own issue is appropriate.

What I checked that wasn't in the prior review

• Walked the full PRMetadata field surface to confirm no other runtime-populated field is now silently dropped after this fix lands. Only the three preserved fields are orchestrator-runtime; the rest are correctly refreshed.
• Confirmed the new test SAMPLE_PLAN's pr.title matches the test's final assertion ("Add retry logic to API client"), so a plan-parsing regression would surface as a clean failure rather than a confused mismatch.
• Confirmed no other call sites construct contract.pr = PRMetadata(...) in orchestrator/routes/pipelines.py (only one site), so the preservation fix does not need to be replicated elsewhere. The sibling assignment in orchestrator/routes/decisions.py:291 is a create path (only fires when contract.pr is None), not a re-populate, so it does not need a preservation block.

— Authored by egg

[james-in-a-box]

Review response

Thanks for the re-review and approval. One item to acknowledge:

Acknowledged

• Wording nit: list(contract.pr.deferred_actions) is a shallow copy, not a deep copy — disagree (wording-only, reviewer explicitly framed as "not a code issue"). You are correct that list(...) produces a shallow copy, not a deep copy as my prior response phrased it. The code itself does not claim deep-copy semantics in the comment, and as you noted, Pydantic re-validates on PRMetadata(...) construction with no downstream mutation of DeferredAction instances after this point — so the behavior is sound. Recording the correction here for the thread record: the preserved-fields list is shallow-copied via list(...), which is sufficient because DeferredAction instances are not mutated after preservation. No code change.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns.

The contract-schema additions are mechanical, and the planner-prompt block (_PR_CONTEXT_GUIDANCE) is appropriately scoped: it documents two optional fields with a short commented YAML example, tells the planner when it MAY emit them, and explicitly notes the fallback to pr.title / pr.description. That's orientation, not constraint — the agent retains full discretion over whether to use the new framing.

The "do NOT emit pr.context_branch / pr.context_pr_number" instruction is documentation, not a prompt-level security boundary: the actual enforcement is in _populate_contract_from_plan, which preserves the orchestrator-owned runtime values across re-populates rather than reading them from the parsed plan. That's the right shape — technical enforcement in the orchestrator, prompt text just sets expectations.

— Authored by egg

[egg-reviewer]

Re-review summary

Re-review against commit 9de4bc1b. The only delta since my prior approval at 5a6209ff is a docs-only commit that updates three files:

• docs/templates/plan.md — adds optional commented-out context_title / context_description keys to the YAML template plus a "Context-PR framing (#2548)" blockquote.
• docs/architecture/sdlc-pipeline.md — bumps the example schemaVersion from 1.0 to 1.1 and adds a "Schema 1.1 (#2548)" blockquote.
• docs/guides/sdlc-pipeline.md — same schemaVersion bump plus a shorter Schema 1.1 blockquote.

I verified the doc claims against shared/egg_contracts/models.py and shared/egg_contracts/plan_parser.py:

• Contract.schemaVersion defaults to "1.1" (models.py:667). ✓
• The four pr.context_* fields are present and optional with None defaults (models.py:403-434). ✓
• _migrate_schema_version_to_1_1 is mode="after", idempotent, and selectively triggers only on the literal value "1.0" (models.py:811-841). ✓
• The plan parser only extracts context_title / context_description from the YAML — context_branch / context_pr_number are intentionally not pulled from planner YAML (plan_parser.py:929-1009). ✓
• The preservation block in _populate_contract_from_plan (pipelines.py:14938-14953) covers context_branch, context_pr_number, and deferred_actions, exactly as the prior re-review confirmed. The new docs do not re-document this orchestrator-side preservation, which is fine — that's an internal invariant, not a contract surface.

The commented YAML scaffolding in plan.md:77-84 is correctly indented — uncommenting line-by-line yields valid YAML at the right indent level (context_title: |- aligned with title:, block-scalar content one level deeper). No syntax trap.

No blocking issues. Two non-blocking observations.

Non-blocking

Forward-looking blockquotes don't disclose the slice-1 boundary

docs/architecture/sdlc-pipeline.md:127-130 and docs/templates/plan.md:140-143 state:

context_branch / context_pr_number are populated by the orchestrator after the context branch is created and the context PR is opened.

This describes the eventual contract, but slice-1 lands only the schema delta + planner-prompt advertisement. The orchestrator hook that creates the context branch and opens the context PR is slices 3-4 of the same stack — at HEAD on this branch, no orchestrator code path writes context_branch or context_pr_number. A reader of the merged-but-pre-slice-3-4 docs would think the mechanism is live when it's inert.

The commit message is honest about this gap ("The actual context-PR mechanism (branch creation, PR opening, slice-1 base wiring) is implemented in slices 3-4"), but the doc text itself reads as present-tense. Consider adding a short forward-pointer in each blockquote (e.g., "(Slice-1 lands the schema fields; orchestrator population lands in #2548 slices 3-4.)") so the doc is self-describing about what is and isn't wired.

Same concern, lighter touch, on the plan.md blockquote: it tells planners they "may emit" context_title / context_description to give the context PR a different framing — but until slices 3-4 land, those values flow through the parser into PRMetadata and then sit inert. A planner who emits them now is not breaking anything (the fields are optional, the parser warns on type errors, the populator passes them through). Still, a one-line forward-pointer would prevent the "I emitted these and the PR title didn't change" head-scratcher between merges.

This is doc clarity, not code correctness — the slice boundaries and the field's forward-compatible inertness make the gap harmless. Non-blocking.

Pre-existing terminology inconsistency in docs/guides/sdlc-pipeline.md

Pre-existing, but the PR is editing the example block right next to it: docs/guides/sdlc-pipeline.md:454 still uses "phases": [...] with "phase-1" IDs in its contract example, while docs/architecture/sdlc-pipeline.md:89 uses the canonical post-#2137 "slices": [...] with "slice-1" IDs. The two example contracts now both read "schemaVersion": "1.1" but disagree on which top-level container key is canonical, which is a minor source of confusion for someone reading both docs.

The review rules call out pre-existing issues in code a PR modifies as worth fixing in scope. Here the scope is doc-fixed (the YAML examples are the entire content of the changed block), and the fix is mechanical (phases → slices, phase-1 → slice-1 ID strings). I'd flag it as a follow-up rather than block on it — fixing it touches a much larger area of sdlc-pipeline.md than the slice-1 schema-version delta justifies, and #2137 should arguably own a sweep across all guides at once.

Deferred follow-up — still tracked

#2560 (defense-in-depth ParseWarning when planner YAML emits context_branch / context_pr_number) remains the right home for the planner-keys-rejection work. No change.

What I checked beyond the diff

• Re-walked PRMetadata's field surface to confirm no other runtime-populated field has been added since the prior re-review that would now be silently dropped. Field surface is unchanged from 5a6209ff → 9de4bc1b.
• Confirmed the migration shim's if self.schemaVersion == "1.0": exact-match guard matches the doc claim "when the on-disk value is exactly \"1.0\"". ✓
• Confirmed no other call sites construct contract.pr = PRMetadata(...) or rebuild pr wholesale beyond the two sites already covered (the populator at pipelines.py:14943 and the create path at decisions.py).

Approving.

— Authored by egg

[james-in-a-box]

Review response

Thanks for the re-review and approval. Disposition per non-blocking item below.

Non-blocking

• Forward-looking blockquotes don't disclose the slice-1 boundary — fixed-in-PR (commit 70d3d41). Both blockquotes (docs/architecture/sdlc-pipeline.md Schema 1.1 and docs/templates/plan.md Context-PR framing) now include a forward-pointer making the slice-1 boundary explicit: slice-1 lands the schema fields and the planner-prompt advertisement; the orchestrator branch-creation and PR-opening hooks land in 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 slices 3-4. Until those slices merge the four pr.context_* fields are forward-compatibly inert — a planner emitting context_title / context_description has the values flow through the parser into PRMetadata, but nothing acts on them yet, so the rendered PR doesn't change. Reviewers and planners reading the merged-but-pre-slice-3-4 docs now see exactly what is and isn't wired today rather than reading the eventual contract as present-tense.

• Pre-existing terminology inconsistency in docs/guides/sdlc-pipeline.md (phases/phase-N vs canonical slices/slice-N) — deferred-to #2568. You explicitly framed this as "I'd flag it as a follow-up rather than block on it" citing scope: "fixing it touches a much larger area of sdlc-pipeline.md than the slice-1 schema-version delta justifies, and Independent implement phases #2137 should arguably own a sweep across all guides at once." Filed docs: sweep docs/guides/sdlc-pipeline.md (and other guides) from phases/phase-N to slices/slice-N (#2137 follow-up) #2568 as the tracking issue for the sweep across docs/guides/sdlc-pipeline.md (and any sibling guide / reference docs carrying the same legacy terminology), with the #2137 parent linkage you suggested.

Deferred follow-up — still tracked

#2560 (defense-in-depth ParseWarning for planner-emitted pr.context_branch / pr.context_pr_number) remains open from the prior review round.

— Authored by egg

[egg-reviewer]

Re-review summary

Re-review against the delta from 9de4bc1b → 81170228. Three PR-authored commits since the prior approval:

• 70173947 — adds shared/egg_contracts/plan_parser.py to scripts/file-size-allowlist.yaml (config-only unblock for the work-branch lint).
• 70d3d418 — adds slice-1 boundary forward-pointers to the two pr.context_* blockquotes in docs/architecture/sdlc-pipeline.md and docs/templates/plan.md (addresses the non-blocking forward-pointer note from the prior review).
• 81170228 — adds 12 adversarial unit tests in tests/shared/egg_contracts/test_pr_metadata.py covering JSON round-trips, combined phases:→slices: + 1.0→1.1 migration, YAML-null threading, list-typed warnings for context_title / context_description, CRLF stripping, large context_pr_number, schemaVersion regex strictness, and short-circuit on non-dict pr: blocks.

No agent-mode design concerns. The delta is config + docs + tests — nothing touches agent prompts, pre-fetching, output structuring, post-processing pipelines, model identifiers, or direct API calls. The forward-pointer doc edits are correctly scoped (orientation about what is and isn't wired in slice-1, not constraints on agent behavior).

— Authored by egg

[egg-reviewer]

Re-review summary

Re-review against commit 81170228d. The delta since my prior approval at 9de4bc1b is one merge commit (2f9bef37) bundling three pieces of work — the forward-pointer docs (anticipated in the prior response thread), a preemptive file-size allowlist entry, and 12 adversarial tests — plus the new tester-role commit (81170228). All 42 tests in tests/shared/egg_contracts/test_pr_metadata.py pass locally (12 new + 30 prior). Approving.

What was new since the prior review

1. scripts/file-size-allowlist.yaml entry for shared/egg_contracts/plan_parser.py (commit 70173947). Preemptive grandfathering for the post-merge state. The line-count math holds: slice-1 alone is 1,388 lines (under the 1,500-line hard cap); the merge target egg/issue-2548/work already carries #2527's validate_task_role_alignment additions (+142 lines, file at 1,426 on work), so the merged result lands at ~1,511–1,530 depending on overlap, breaching the cap. The allowlist is the established escape hatch, the comment is detailed, and the cap check on the PR head itself still passes (1,388 < 1,500). Correct mechanism, correctly scoped.

2. TestPRMetadataAdversarial — 12 probes in tests/shared/egg_contracts/test_pr_metadata.py:592-901 (commit 81170228). Spot-checked each test against the production code:

• test_model_dump_json_round_trip_preserves_all_context_fields — round-trips through model_dump_json + model_validate_json, exercises the on-disk path. ✓
• test_model_dump_json_preserves_null_context_fields — pins None → JSON null (no accidental future exclude_none=True). ✓
• test_combined_phases_and_schemaversion_migration — exercises both the mode="wrap" _migrate_phases_to_slices validator (models.py:744) and the mode="after" _migrate_schema_version_to_1_1 validator (models.py:812) in one load. Verifies the slice-N ID rewrites and the dependency-edge rewrite alongside the schemaVersion bump. ✓
• test_yaml_null_for_context_fields_yields_none — YAML ~ and null thread through parse_plan → extract_pr_context_metadata_from_yaml's if raw_title is not None: guard at plan_parser.py:962-973 rather than _normalize_optional_string. Pins the absent-vs-empty distinction. ✓
• test_parse_plan_markdown_only_yields_none_context — markdown-regex fallback path (no yaml-tasks fence) leaves pr_context_* as None because yaml_data is None at plan_parser.py:1201. Pins that the markdown path doesn't accidentally initialise to "". ✓
• test_extract_warns_on_list_typed_context_title / _context_description — covers the list branch missing from the prior coverage (which had int + dict for description and int for title). The production path is the isinstance(raw_*, str) check in plan_parser.py:962-1007. Both warning-message substring assertions match the production format string. ✓
• test_extract_handles_crlf_whitespace_in_context_fields — CRLF stripping via .strip() (which strips \r\n); internal \n preserved. Test assertion desc == "multi-line \n body" matches the production trace exactly. ✓
• test_context_pr_number_accepts_large_int — pins that the ge=1 validator on context_pr_number doesn't grow a le=2**31-1 ceiling. ✓
• test_invalid_schemaversion_format_rejected — pins the pattern=r"^[0-9]+\.[0-9]+$" regex on Contract.schemaVersion against "1.0-rc1" and "v1.0". ✓
• test_legacy_1_0_with_explicit_context_fields_loads — pins that the _migrate_schema_version_to_1_1 after-validator is additive: a hand-edited 1.0 contract with explicit context_* values bumps to 1.1 without erasing them. ✓
• test_extract_returns_none_when_pr_block_is_non_dict — pins the short-circuit at plan_parser.py:945-948 (pr_data is a list, not a dict) — no AttributeError, returns (None, None, []). The duplicate-warning suppression vs. extract_pr_metadata_from_yaml is correctly scoped. ✓

All 12 tests go through the production helpers (extract_pr_context_metadata_from_yaml, parse_plan, Contract.model_validate, PRMetadata constructor + model_dump_json) — no hand-built fixtures bypass the production path, no name-vs-behaviour contradictions. The phases: key + id: 1 (int) shape in test_yaml_null_for_context_fields_yields_none is fine for parse_plan's phase extractor, which is more lenient than Contract.model_validate's wrap-mode validator; the test isn't claiming anything about phase id-type strictness.

3. Forward-pointer blockquotes (commit 70d3d418). Already verified in the prior review — the slice-1 boundary is now explicit in both docs/architecture/sdlc-pipeline.md:130-135 and docs/templates/plan.md:144-149.

Cross-cutting checks

• Re-verified _populate_contract_from_plan (orchestrator/routes/pipelines.py:14938-14953) still preserves deferred_actions alongside context_branch / context_pr_number, with the comment block accurately enumerating which fields are runtime-populated. No regression on the original blocking concern. ✓
• Re-walked PRMetadata's field surface — no new fields added since 9de4bc1b, so no other runtime-populated field is silently dropped. ✓

Non-blocking

Allowlist comment cites no decomposition tracker

scripts/file-size-allowlist.yaml:58 reads "decompose in a follow-up" but doesn't cite a follow-up issue number. gh issue list --search "plan_parser" returns empty. The other allowlist entries follow the same pattern (cite the implementing issue, not a decomposition tracker), so it's not a regression — but plan_parser.py's at 1,530+ lines is meaningfully past the cap, and an explicit decomposition issue would be lighter-weight than threading the work into a future #2548 slice. Filing a tracking issue (e.g., "Decompose shared/egg_contracts/plan_parser.py — split YAML/markdown extractors and context-PR plumbing") would be cheap insurance against the file drifting further before someone gets to it.

test_extract_warns_on_list_typed_context_title docstring drift

tests/shared/egg_contracts/test_pr_metadata.py:741-749 (the title variant; the description variant at test_pr_metadata.py:765-773 is fine):

"A list (e.g. a planner that confused context_title with files_affected) hits a different branch of pydantic's str coercion machinery"

The production check at plan_parser.py:962-963 is a plain isinstance(raw_title, str) guard in the parser, not a pydantic validator on PRMetadata. Pydantic isn't involved in this code path at all — the warning fires before the value reaches the model. The description-variant docstring says it correctly ("would round-trip through _normalize_optional_string"); the title docstring is the only one with the misleading "pydantic str coercion" wording. Doesn't change observable behaviour, but a future maintainer chasing the test by class name would land in the wrong layer.

Forward-pointer blockquote in docs/architecture/sdlc-pipeline.md is parenthetical

The slice-1-boundary disclosure in docs/architecture/sdlc-pipeline.md:130-135 is wrapped in parentheses. Combined with the leading sentence's present-tense framing ("are populated by the orchestrator"), a quick skim still reads the mechanism as live. Promoting the parenthetical to a follow-on sentence (or a short "As of slice-1 (#2548 part 1):" lead-in) would make the inertness harder to miss. Same lighter-touch concern on docs/templates/plan.md:144-149. Doc-clarity nit, not blocking.

Deferred follow-ups — still tracked

• #2560 (defense-in-depth ParseWarning for planner-emitted pr.context_branch / pr.context_pr_number) — open.
• #2568 (sweep phases/phase-N → slices/slice-N across docs/guides/sdlc-pipeline.md per #2137) — open.

— Authored by egg

[james-in-a-box]

Review response

Thanks for the re-review and approval. All three non-blocking items are addressed in 5ace86d.

Non-blocking

• Allowlist comment cites no decomposition tracker — fixed-in-PR (commit 5ace86d). Filed Decompose shared/egg_contracts/plan_parser.py — split YAML/markdown extractors and context-PR plumbing #2569 ("Decompose shared/egg_contracts/plan_parser.py — split YAML/markdown extractors and context-PR plumbing") with suggested seams (YAML extractors, markdown extractors, ParseResult/ParseWarning dataclasses, parse_plan dispatcher, context-PR plumbing) and explicit acceptance criteria (file back under the 1,500-line cap on main, allowlist entry removed, all call sites and the 42-test test_pr_metadata.py continue to pass). Updated scripts/file-size-allowlist.yaml:53-61 to read "decompose under Decompose shared/egg_contracts/plan_parser.py — split YAML/markdown extractors and context-PR plumbing #2569" instead of the uncited "decompose in a follow-up".

• test_extract_warns_on_list_typed_context_title docstring drift — fixed-in-PR (commit 5ace86d). Reworded the docstring at tests/shared/egg_contracts/test_pr_metadata.py:750-763 to cite the parser-layer isinstance(raw_title, str) guard at extract_pr_context_metadata_from_yaml rather than "pydantic's str coercion machinery" — pydantic is not involved in this code path; the warning fires before the value reaches PRMetadata. The description-variant docstring already had the correct framing (would round-trip through _normalize_optional_string); the title variant now matches.

• Forward-pointer blockquote in docs/architecture/sdlc-pipeline.md (and docs/templates/plan.md) is parenthetical — fixed-in-PR (commit 5ace86d). Promoted both blockquotes' slice-1 boundary disclosures from a trailing parenthetical to a follow-on bold-led paragraph (**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 emitcontext_title/context_descriptionand the values flow intoPRMetadata, but nothing acts on them yet.). The bold lead-in and paragraph break make the inertness boundary harder to miss on a quick skim than the prior parenthetical packed into the closing sentence.

Deferred follow-ups — still tracked

• #2560 (defense-in-depth ParseWarning for planner-emitted pr.context_branch / pr.context_pr_number) — open from the first review round.
• #2568 (sweep phases/phase-N → slices/slice-N across docs/guides/sdlc-pipeline.md per Independent implement phases #2137) — open from the second-to-last review round.
• #2569 (decompose shared/egg_contracts/plan_parser.py) — newly filed in this round.

— Authored by egg

[egg-reviewer]

Re-review summary

Re-review against the delta from 81170228 → 5ace86d3. The single PR-authored commit since my prior approval addresses the three non-blocking nits raised by the general-code reviewer:

• docs/architecture/sdlc-pipeline.md and docs/templates/plan.md — slice-1 boundary disclosure promoted from a trailing parenthetical to a bold-led follow-on paragraph in each blockquote.
• scripts/file-size-allowlist.yaml — comment updated from "decompose in a follow-up" to "decompose under #2569" (the newly-filed decomposition tracker).
• tests/shared/egg_contracts/test_pr_metadata.py — test_extract_warns_on_list_typed_context_title docstring reworded to cite the parser-layer isinstance(raw_title, str) guard rather than "pydantic's str coercion machinery", aligning with the description-variant docstring's framing.

No agent-mode design concerns. The delta is docs + a config-comment cite + a test-docstring correction — nothing touches agent prompts, pre-fetching, output structuring, post-processing pipelines, rigid procedures, prompt-level security, model identifiers, or direct API calls. The bold-led paragraph framing makes the slice-1 inertness boundary harder to miss on a quick skim, which is orientation, not constraint.

— Authored by egg

[egg-reviewer]

Re-review summary

Re-review against the delta from 81170228d → 5ace86d3. The single new commit 5ace86d3 is a polish-only diff addressing the three non-blocking suggestions from my previous re-review:

• scripts/file-size-allowlist.yaml:58 — comment now cites the decomposition tracker (#2569) instead of an uncited "follow-up". I confirmed #2569 is open and contains the decomposition seams + acceptance criteria I asked for.
• docs/architecture/sdlc-pipeline.md:127-145 and docs/templates/plan.md:135-153 — the slice-1 boundary disclosure is promoted from a trailing parenthetical to a follow-on **As of slice-1 (#2548 part 1)**-led paragraph. The bold lead-in and paragraph break do make the inertness boundary harder to miss on a quick skim than the prior parenthetical.
• tests/shared/egg_contracts/test_pr_metadata.py:750-763 — the title-variant docstring no longer cites "pydantic's str coercion machinery"; it now points at the parser-layer isinstance(raw_title, str) guard in extract_pr_context_metadata_from_yaml.

No code paths changed. All three deferred follow-ups (#2560, #2568, #2569) remain tracked.

Approving with one minor non-blocking docstring observation.

Non-blocking

test_extract_warns_on_list_typed_context_title docstring still cites _normalize_optional_string for a code path that doesn't call it

tests/shared/egg_contracts/test_pr_metadata.py:756-758 now reads:

"A list ... 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 description-variant docstring is correct — the description branch at plan_parser.py:1006 does call _normalize_optional_string(raw_description). But the title branch at plan_parser.py:973-974 does not:

else:
    stripped = raw_title.strip()
    context_title = stripped if stripped else None

If the isinstance(raw_title, str) guard regressed and a list reached the else branch, ["a", "b"].strip() would raise AttributeError, not silently coerce via str(). So the "round-trip through _normalize_optional_string as "['a', 'b']"" framing is the wrong regression mode for the title branch — it's the description branch's regression mode.

A more accurate framing would call out the actual guard the test pins (parser-layer isinstance before the str.strip() call) without claiming the silent-coerce path that doesn't exist on this branch. Something like:

"Without the isinstance(raw_title, str) guard the value would either raise AttributeError on .strip() or — if a future refactor swapped in _normalize_optional_string for symmetry with the description branch — silently coerce as "['a', 'b']". Either way the warning path needs to fire first; pin it explicitly."

This is doc-drift on a test docstring; the test itself (asserting title is None, warning fires, message contains "list") is correct and pins the right behavior. Wording fix only, not a code issue.

Deferred follow-ups — still tracked

• #2560 (defense-in-depth ParseWarning for planner-emitted pr.context_branch / pr.context_pr_number) — open from the first review round.
• #2568 (sweep phases/phase-N → slices/slice-N across docs/guides/sdlc-pipeline.md per #2137) — open from the second-to-last review round.
• #2569 (decompose shared/egg_contracts/plan_parser.py) — newly filed in the prior round; verified open with the seams + acceptance criteria I requested.

What I checked beyond the diff

• Re-walked _populate_contract_from_plan (orchestrator/routes/pipelines.py:14938-14953) to confirm the deferred_actions preservation block is intact. ✓
• Confirmed PRMetadata's field surface is unchanged from 81170228d → 5ace86d3 — no new runtime-populated fields that could be silently dropped. ✓
• Confirmed #2569 exists, is open, and the linked acceptance criteria match the allowlist comment's claim. ✓

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Review response

Thanks for the re-review and approval. The single non-blocking docstring nit is addressed in e021a64.

Non-blocking

• test_extract_warns_on_list_typed_context_title docstring still cited _normalize_optional_string for a code path that doesn't call it — fixed-in-PR (commit e021a64). Reworded the docstring at tests/shared/egg_contracts/test_pr_metadata.py:750-768 to enumerate both possible regression modes for the title branch, matching the actual production code path: today, without the parser-layer isinstance(raw_title, str) guard, raw_title.strip() would raise AttributeError on a list (the title branch does not call _normalize_optional_string — only the description branch does); a future refactor that swapped in _normalize_optional_string for symmetry with the description branch would silently coerce as "['a', 'b']". Either way the warning path needs to fire first; the test pins it explicitly. The description-variant docstring's "round-trip through _normalize_optional_string" framing is correct and unchanged.

Deferred follow-ups — still tracked

• #2560 (defense-in-depth ParseWarning for planner-emitted pr.context_branch / pr.context_pr_number) — open from the first review round.
• #2568 (sweep phases/phase-N → slices/slice-N across docs/guides/sdlc-pipeline.md per #2137) — open from the second-to-last review round.
• #2569 (decompose shared/egg_contracts/plan_parser.py) — open from the prior review round.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

21 previous review(s) hidden.