Egg/issue 2548/slice 1

[jwbron]

No description provided.

[egg-reviewer]

No agent-mode design concerns.

This slice is plumbing — schema 1.1 with four additive pr.context_* fields, per-slice BRC history partitioning in the orchestrator, and planner-prompt advertisement of two optional context-PR keys. None of it touches the agent-mode patterns the design guide flags:

• The planner-prompt additions (_PR_CONTEXT_GUIDANCE, _PR_CONTEXT_YAML_EXAMPLE_LINES) inform the planner about optional keys and explicitly mark which fields the orchestrator owns at runtime — that's orientation, not constraint.
• The YAML structured output is appropriate here: the contract is a machine-readable artifact consumed by orchestrator code paths that open PRs, so this is the "machine-readable output for genuine automation" exception, not human-facing output dressed up as JSON.
• No direct Anthropic API calls, no httpx/requests to api.anthropic.com, no pinned claude-*-YYYYMMDD model identifiers, no post-processing pipelines parsing natural-language agent output.
• The BRC history writer is pure orchestrator-side log routing — it partitions messages already in the bus into per-slice transcript files for reviewer ergonomics. No prompt-side coupling.

— Authored by egg

[egg-reviewer]

Post-merge review (slice-1 of #2548)

Reviewed the full diff (3,555 additions / 186 deletions across 23 files) end-to-end. The slice landed cleanly: schema 1.0→1.1 migration is well-scoped and idempotent, the per-slice BRC history writer has thorough test coverage including path-traversal payloads, and the planner-prompt + ParseResult plumbing is forward-compatibly inert until slices 3-4 wire it up. A few observations worth tracking — none are blocking, but a couple deserve follow-up issues.

Notable: actual bug fix shipped under "follow-up"

sandbox/egg_agent_tools/handlers/brc.py previously had "CONSENSUS_WITHDRAWN" (with a trailing N) in _BRC_HISTORY_TYPES, but the real MessageType enum value is CONSENSUS_WITHDRAW (no N — see orchestrator/message_store.py:68). This meant brc_read_peer_artifact(message_type="CONSENSUS_WITHDRAW") would raise Unknown message_type(s), while CONSENSUS_WITHDRAWN was a no-op filter for messages that don't exist. The new whitelist fixes this. The new TestBrcHistoryTypesDriftGuard is a great regression guard, but a direct end-to-end test that actually filters by CONSENSUS_WITHDRAW and asserts a non-empty result would make the fix unmistakable in the test report. Worth adding.

CONSENSUS_BRC_TYPES is asymmetric and unguarded

pipelines.py:8105 introduces a hardcoded subset of BRC_HISTORY_TYPES that the writer treats as "must have slice_id or be dropped." The drift-guard test (TestBrcHistoryTypesDriftGuard) only locks writer↔reader symmetry on BRC_HISTORY_TYPES; nothing prevents a future PR from adding e.g. CONSENSUS_FOO to BRC_HISTORY_TYPES and forgetting CONSENSUS_BRC_TYPES, in which case the new type would silently route to unattributed_other instead of being the slice-attribution-required member it semantically is. Suggest adding a similar drift guard or, better, deriving CONSENSUS_BRC_TYPES programmatically (e.g. frozenset(t for t in BRC_HISTORY_TYPES if t.startswith("CONSENSUS_"))).

Transitional state: stale {id}-implement.md is never cleaned up

The hard switchover claims "no aggregate implement file is produced," but the writer never removes an existing 42-implement.md from a prior babysit_pr / pre-#2548 run on the same identifier. If a pipeline is restarted post-merge, an in-place .egg-state/brc-history/42-implement.md from an earlier run will persist alongside the new per-slice files; _build_brc_history_link_line will dutifully include both (the sort key correctly clusters them at rank["implement"] with idx=-1 then idx=N≥1). Tests cover the clean-on-disk case but not "stale aggregate already exists, then slice writer runs." Probably won't bite in greenfield use, but agents inheriting state across restarts may see surprising link lines. Consider an explicit cleanup pass when entering partition mode.

Slice-id canonicality ≠ slice-id legitimacy

The writer interpolates any metadata['slice_id'] matching ^slice-[0-9]+$ directly into the filename. There's no cross-check against contract.slices — a message with metadata['slice_id'] = "slice-99999" for a 3-slice contract would produce a 42-implement-slice-99999.md next to the legitimate three. The path is contained (good — that's what the regex guarantees), so this isn't a security issue, but since any role can post arbitrary metadata via routes/messages.py, a buggy or hostile sender can pollute brc-history with phantom-slice files. Logging an info/warning when an observed slice_id isn't in the loaded contract's slice set would surface this without adding false-rejects.

Handler re-sort runs unnecessarily on single-file reads

sandbox/egg_agent_tools/handlers/brc.py:1082 — if len(history_files) > 1: filtered.sort(...) always sorts when slice-scoped, even if the unattributed sibling doesn't exist on disk (because the path is added before the existence check at line 1042). Records from a single existing file are already in chronological order, so the sort is redundant. Either move the existence check to filter history_files before the sort decision, or condition on any_existed plus a tracking counter. Minor perf.

Documentation drift

sandbox/egg_agent_tools/handlers/brc.py:915 and :1015 reference "writer-side seam (orchestrator/routes/pipelines.py ~line 8406)" — the actual SLICE_ID_PATTERN.fullmatch(raw_slice_id) is on line 8417. Drifts every time pipelines.py shifts; consider replacing line numbers with a function/symbol reference (_write_brc_history).

Schema migration: confirmed safe under validate_assignment=True

I did want to flag a concern about _migrate_schema_version_to_1_1 mutating self.schemaVersion while EggContractBaseModel has validate_assignment=True (verified by test_pr_number_validator_re_runs_on_assignment). Walked through the flow:

1. Pydantic v2 with validate_assignment=True re-runs mode="after" model_validators on field assignment.
2. Inside the validator, self.schemaVersion = "1.1" triggers re-validation; the recursive call hits if self.schemaVersion == "1.0" → False, returns self, no further mutation.
3. mode="wrap" validators (like _migrate_phases_to_slices) do not re-fire on field assignment, so _legacy_phases isn't re-stamped.

No infinite recursion. Worth a one-line # safe under validate_assignment=True note in the docstring for the next reader.

_populate_contract_from_plan preservation logic is correct

The preserved_branch / preserved_pr_number / preserved_deferred_actions capture-then-rebuild pattern at line 15216 reads cleanly. Important detail: when contract.pr is None initially, preserved_deferred_actions defaults to [] and context_branch/context_pr_number to None, which is the correct first-population behavior. The asymmetry (planner-emitted context_title/context_description refresh; orchestrator-populated fields preserve) is the right choice and is properly exercised by test_populate_contract_from_plan_preserves_deferred_actions.

Test coverage commendations

• test_invalid_slice_id_pattern_treated_as_unattributed — exhaustive injection payloads (path traversal, separators, case mismatch, newline injection). Good defensive testing.
• test_natural_sort_per_slice_iteration_order — pins iteration order against shuffled input; catches lexicographic regressions.
• test_idempotent_per_slice_write — covers per-slice files AND the unattributed sibling under the #1714 idempotency contract.
• The drift guard for writer↔reader BRC type symmetry is a solid pattern; suggest applying it to CONSENSUS_BRC_TYPES too (see above).

Summary

Foundational work is sound; the slice-1 acceptance criteria are well-met. None of the above is merge-blocking, but the CONSENSUS_BRC_TYPES drift risk and the stale-aggregate-cleanup edge case merit follow-up issues against #2548 before slices 3-4 land — both will become more visible once the orchestrator hooks start exercising real per-slice flows.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

1 previous review(s) hidden.