[issue-3200][slice-3/10] Derive

[james-in-a-box]

Derive the four #3189 anchor fields (last-reviewed SHA/producer, latest verdicts, open NACK reasons, conditional-ACK obligations) from the BRC message record; extend shared/egg_anchor additively. Closes the substrate gap the NACK flagged (BRCState carries none of these today; #3189 OPEN). Logical dep: none; serialized after slice 2 (#3046). Hard prereq of the protected root (slice 4).

Base PR: #3234

What's in this PR

Commits (4):

• Persist BRC history for slice-3 (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)
• implement(BRC context discipline: protected directive root + queryable environment, bounded by a deterministic threshold reseed #3200): align slice-3 tests to merged derivation (task-3-2)
• implement(BRC context discipline: protected directive root + queryable environment, bounded by a deterministic threshold reseed #3200): test Deterministic BRC memory: derive the factual layer (review anchors, verdicts, NACK reasons, obligations) from the message record instead of agent transcription #3189 BRC anchor derivation (slice-3, task-3-2)
• implement(BRC context discipline: protected directive root + queryable environment, bounded by a deterministic threshold reseed #3200): derive Deterministic BRC memory: derive the factual layer (review anchors, verdicts, NACK reasons, obligations) from the message record instead of agent transcription #3189 BRC anchors from the message record (slice-3, task-3-1)

.egg-state/brc-history/3200-implement-slice-3.json    | 749 +++++++++++++++++++++++++++++++++++
 .egg-state/brc-history/3200-implement-slice-3.md      | 815 ++++++++++++++++++++++++++++++++++++++
 orchestrator/tests/test_brc_anchor_derivation.py      | 247 ++++++++++++
 shared/egg_anchor/__init__.py                         |  12 +
 shared/egg_anchor/brc_derive.py                       | 271 +++++++++++++
 shared/egg_anchor/models.py                           |  99 +++++
 shared/egg_anchor/tests/test_brc_anchor_derivation.py | 323 +++++++++++++++
 7 files changed, 2516 insertions(+)

This slice

Derive

Files affected:

• shared/egg_anchor/
• orchestrator/
• shared/egg_anchor/tests/
• orchestrator/tests/

Tasks (2) + acceptance criteria

• task-3-1: Implement a deterministic derivation that, from the BRC message record (CONSENSUS_PROPOSE/ACK/NACK/conditional-ACK messages), computes: (i) last-reviewed SHA per producer (latest reviewed proposal_commit_sha per edge); (ii) latest verdict per reviewer->producer edge; (iii) open NACK reasons (current-version NACKs not yet resolved); (iv) conditional-ACK obligations (pre_merge_condition, with resolved/unresolved status). Derivation is mechanical (never agent-transcribed). Extend shared/egg_anchor model(s) to carry these four fields additively (do not break BRCState.acks/nacks/last_message_id at models.py:96-103).
  • Acceptance criteria: The four fields derive purely from the message record; the egg_anchor model carries them additively without breaking existing fields; against a fixture message record the derived anchors match exactly; no agent-authored content enters this layer.
• task-3-2: Tests: a fixture message stream (multiple producers; an ACK then re-propose + NACK; a conditional-ACK with an unresolved obligation) yields the correct last-reviewed SHA per producer, latest verdict per edge, open NACK reason, and unresolved obligation; a resolved obligation is marked resolved; legacy acks/nacks/last_message_id untouched.
  • Acceptance criteria: Derivation correctness for all four fields asserted against a realistic fixture; obligation resolved/unresolved distinguished; legacy-field non-regression asserted; tests pass.

Stack

• Position: slice 3 of 10 in pipeline issue-3200
• Base PR: BRC context discipline (#3200) — pipeline state branch (implementation in slice PRs #3236–#3243) #3234
• Stacked on top of egg/issue-3200/slice-2

[james-in-a-box]

Autofix tracking

{"Lint/Python": 2}

[james-in-a-box]

Contract verification — PR #3238 (issue-3200, slice-3 "Derive")

Verdict: Approve. Both slice-3 tasks are fully implemented and every per-task acceptance criterion is objectively met. I re-ran the two derivation test files locally (shared/egg_anchor/tests/test_brc_anchor_derivation.py + orchestrator/tests/test_brc_anchor_derivation.py) — 9 passed.

task-3-1 — derive the four #3189 anchor fields ✅

shared/egg_anchor/brc_derive.py::derive_brc_anchors computes all four fields mechanically from the BRC message record:

Field	Evidence
(i) last-reviewed SHA per producer	_assemble picks the SHA of the highest version any reviewer verdicted on (brc_derive.py:210-221); superseded v1 is correctly excluded
(ii) latest verdict per reviewer→producer edge	edge map overwrites on each ACK/NACK in replay order (:160-187), emitted as ReviewEdgeVerdict
(iii) open NACKs (current version only)	gated on state["version"] == current (:241-254) — superseded NACKs dropped
(iv) conditional-ACK obligations w/ resolved status	pre_merge_condition → ConditionalAckObligation, CONSENSUS_OBLIGATION_RESOLVED flips resolved (:189-197, :255-264)

• Purely from the message record / no agent prose — verified: only structured fields are read (message_type, from_role, to_role, metadata); _nack_reason deliberately takes the structured payload.reason/meta.reason, never the free-form body (:104-109). Sound, and the design avoids any orchestrator-package dependency.
• Additive, no break to legacy fields — verified: BRCState.acks/nacks/last_message_id keep their original agent-id-list meaning; the new BRCState.derived defaults to None (models.py:184-202). New types (ReviewVerdict, ReviewEdgeVerdict, OpenNack, ConditionalAckObligation, BRCDerivedAnchors) are purely additive and exported in __init__.py.
• Matches a fixture record exactly — test_derivation_agrees_with_matrix asserts exact values against the ApprovalMatrix ground-truth projection.

Replay correctness spot-checks I verified by hand against the fixture: unversioned ACK/NACK fall back to the producer's current version at processing time (:168-169), bool is correctly rejected as a version (int subclass, :73-74), and outputs are deterministically sorted by (producer, reviewer) — test_derivation_is_deterministic confirms.

task-3-2 — fixture tests ✅

• Four-field correctness against a realistic multi-producer fixture (ACK → re-propose → NACK + conditional-ACK) — asserted in both the shared and orchestrator suites.
• Resolved vs unresolved obligation distinguished — test_resolved_obligation_marked_resolved; test_derivation_agrees_with_matrix pins coder_ob.resolved is False / tester_ob.resolved is True.
• Legacy non-regression — test_legacy_brcstate_fields_untouched (unguarded, runs today).
• The orchestrator-side test cross-checks the derivation against the live ApprovalMatrix substrate, which is a strong correctness anchor.

Commit linkage ✅

Commits map cleanly to tasks (34278c6 derivation → task-3-1; 226412c/3c0c9ad tests → task-3-2; dcfe077 BRC-history persistence (#2548); 9bb6452 coder/tester merge).

Non-blocking notes (not gating)

1. Scope leak (cosmetic): 951f2cf ("apply automated formatting fixes") also reformats one line in orchestrator/agent_model_resolution.py:457-461 (multi-line → single-line, line-length only). Behavior-neutral and outside the slice's logical scope, but worth a glance from the human reviewer — it's unrelated to the anchor work and was swept in by the auto-formatter.
2. Criterion marking: this sliced contract has no top-level ac-N acceptance criteria (they live as per-task free text), so egg-contract verify-criterion --criterion ac-N is not applicable here. The orchestrator was also unreachable for the duration of this review (egg-contract show returned "Orchestrator unreachable"); I read the contract directly from .egg-state/contracts/issue-3200.json.

No contract violations found. All acceptance criteria for both tasks are satisfied.

— Authored by egg

[egg-reviewer]

Review: slice-3 BRC anchor derivation (#3189 / #3200)

Verdict: Approve. No blocking issues. The feature works end-to-end against real data, is purely additive, and is well-tested. Details below, plus non-blocking suggestions.

I went beyond the unit fixtures and validated the derivation against the real captured message record shipped in this PR (.egg-state/brc-history/3200-implement-slice-3.json, 37 real messages), and traced field-name consistency against the orchestrator's own message reader/writer. This is the check the hand-built fixtures can't provide on their own.

What I verified

1. The derivation is functional against real data (not a silent no-op). Running derive_brc_anchors on the captured record produces correct output:

• last_reviewed_sha → {"coder": "34278c681", "tester": "3c0c9ad5a"} (no-op documenter correctly omitted — empty commit_sha, nothing to anchor)
• 11 latest_verdicts with correct per-edge reviewed_sha resolved via the version→SHA join
• empty open_nacks / obligations (matches reality — that pipeline had no NACK/conditional-ACK)

2. Field-name consistency across modules — the main cross-module risk for this kind of "consume a serialized record" feature. The fixtures hand-build message dicts; I confirmed every field the derivation reads matches what the orchestrator actually emits and what its own replay reader expects:

• PROPOSE: metadata.version + metadata.commit_sha — present in real record (brc_derive.py:82-84, _version reads meta.get("version")). ✓
• ACK/NACK: producer in to_role, version in metadata.version (= producer proposal version in the real record), reason/condition under metadata.payload.* — _payload handles the nesting, _commit_sha/_pre_merge_condition/_nack_reason check both meta and payload. ✓
• CONSENSUS_OBLIGATION_RESOLVED: the derivation reads metadata.reviewer_role / metadata.producer_role (brc_derive.py:191-192). This is an exact match to the message-bus message written in routes/signals.py:3117-3126 and to the orchestrator's own replay reader at peer_consensus.py:2268-2271. (Note the SSE emit_event in handle_resolve_obligation uses different keys reviewer/producer — that's a separate event bus the derivation correctly does not consume.) ✓

3. Logic correctness. Replay semantics (latest verdict per edge wins; re-propose advances producer_version and supersedes prior NACKs/obligations via the state["version"] != current filter in _assemble); bool-rejection in _version (brc_derive.py:73); to_role == "all" guard; structured-fields-only (never free-form body) — all correct. Additive model extension preserves BRCState.acks/nacks/last_message_id (asserted by the un-guarded test_legacy_brcstate_fields_untouched).

4. Tests pass and exercise the real path. All 9 tests green locally. The orchestrator-side test_derivation_agrees_with_matrix builds the real ApprovalMatrix via its public API and asserts the message-fed derivation agrees with the matrix projection — a legitimate ground-truth cross-check, not a self-seeded golden.

5. No security surface. Read-only, no eval/exec/pickle/subprocess/deserialization, O(n), structured fields only.

The feature is not yet wired into any consumer — expected: this is the substrate slice; slice-4 (protected root) consumes it. BRCState.derived defaults to None (additive/optional).

Non-blocking suggestions

1. Add a real-record regression test (highest value). The unit fixtures are hand-built dicts; I had to manually confirm they match the real message shape. A small test that feeds an actual .egg-state/brc-history/*.json (or a trimmed copy) through derive_brc_anchors would lock the real-shape contract so a future change to the orchestrator's message metadata keys breaks a test rather than silently turning the derivation into a no-op. Today, a key rename (e.g. metadata.version) would pass every existing test while producing wrong anchors on live data.

2. Document the caller's scoping responsibility. derive_brc_anchors does no slice/phase filtering — unlike the orchestrator's reconstruct_tracker_from_messages, which filters by slice_id + phase (peer_consensus.py:2128-2135) precisely to avoid cross-slice state conflation (#2761). The brc-history files are already per-slice, so the contract is "pass a pre-scoped record," but that expectation is implicit. A one-line note in the docstring would save slice-4 from accidentally passing an unscoped, multi-slice stream.

3. last_reviewed_sha naming. It's a per-producer max-across-reviewers aggregate, while the per-edge reviewed SHA lives in latest_verdicts[].reviewed_sha. The field doc covers it, but the name reads as if it were per-edge; a half-sentence clarifying "highest version any reviewer verdicted on" inline at the map would reduce misuse risk.

4. Fixture timestamps. The unit fixtures omit timestamp, so _ordered's (timestamp, id) sort path (brc_derive.py:122-123) — the path real records actually take — is never exercised by a unit test. Consider adding timestamps to one fixture variant.

5. Scope creep (trivial). The orchestrator/agent_model_resolution.py change is an unrelated one-line reformat (collapsing a multiline ternary); semantically identical, harmless, but unrelated to this slice.

— Authored by egg

[james-in-a-box]

Review feedback addressed

Both reviews approved with non-blocking suggestions. Per-item disposition:

egg-reviewer suggestions

1. Add a real-record regression test (highest value) — fixed-in-PR (commit 03ee22c). Added test_real_record_locks_live_message_shape, which derives anchors from an actual captured BRC record (the 37-message 3200-implement-slice-3 record, copied verbatim to shared/egg_anchor/tests/data/brc_record_real.json so the test is self-contained and not coupled to the transient .egg-state/ dir). It pins the exact output (last_reviewed_sha == {"coder": "34278c681", "tester": "3c0c9ad5a"}, 11 verdict edges, empty NACKs/obligations), so a future rename of a metadata key the derivation reads now fails a test instead of silently producing empty anchors on live data.

2. Document the caller's scoping responsibility — fixed-in-PR (commit 03ee22c). Added a "Caller scoping responsibility" paragraph to the brc_derive module docstring spelling out that the function does no slice/phase filtering (unlike reconstruct_tracker_from_messages) and expects a pre-scoped per-slice/per-phase record — exactly the shape the brc-history/<id>-<phase>.json files already have.

3. last_reviewed_sha naming — fixed-in-PR (commit 03ee22c). Added an inline note at the aggregation in _assemble clarifying it is a per-producer max-across-reviewers aggregate, distinct from the per-edge latest_verdicts[].reviewed_sha.

4. Fixture timestamps — fixed-in-PR (commit 03ee22c). Added test_timestamp_sort_path_recovers_chronology, which hands a timestamped record to the derivation newest-first; the assertions only hold if the (timestamp, id) sort in _ordered recovers chronology (under the input-order fallback the edge would settle on the earlier v1 ACK). This exercises the sort path real records take, which the timestamp-less hand-built fixtures never reached.

5. Scope creep — agent_model_resolution.py one-line reformat — disagree (formatter-mandated, not a real problem). The change is a behavior-neutral collapse of a multi-line ternary applied by ruff format to clear the Lint/Python CI failure. The collapsed line is 96 chars (under the 100-char line-length), so reverting it would simply be re-collapsed by the formatter and reintroduce the lint failure. Both reviewers marked it cosmetic/non-blocking; there is nothing to fix here.

james-in-a-box (contract-verification) notes

• Note 1 — scope leak in agent_model_resolution.py — disagree (formatter-mandated). Same item as egg-reviewer Phase 3: Container extraction #5 above; see that reasoning.
• Note 2 — criterion marking N/A — no action needed (the reviewer noted this sliced contract has no top-level ac-N criteria, so verify-criterion is not applicable).

All 9 egg_anchor derivation tests pass locally (shared/egg_anchor/tests/test_brc_anchor_derivation.py), including the two new regression tests; ruff check and ruff format --check are clean on the changed files.

— Authored by egg

[egg-reviewer]

Re-review: slice-3 BRC anchor derivation (#3189 / #3200)

Verdict: Approve. All non-blocking suggestions from the prior review are properly addressed (not superficially), and the new delta introduces no blocking issues.

Scope of this re-review

The PR branch was rebased since my last review at 951f2cf. The true PR-authored delta since then is two commits — 03ee22cd (address review) and 68193439 (mypy fix) — touching exactly three files:

• shared/egg_anchor/brc_derive.py — docstring + one inline comment only; no logic change from the approved version.
• shared/egg_anchor/tests/data/brc_record_real.json — new real-record fixture (749 lines).
• shared/egg_anchor/tests/test_brc_anchor_derivation.py — two new tests.

Verification of the addressed suggestions

#1 — real-record regression test. Confirmed it does the job it was asked to do, and confirmed it is not a self-seeded golden. I ran the file (9 passed, 0 skipped — the locator resolves derive_brc_anchors via the package export, so the assertions actually execute rather than skip), then independently checked every pinned value against the fixture content:

• 3 CONSENSUS_PROPOSE: coder v1 34278c681, tester v1 3c0c9ad5a, documenter v1 '' → last_reviewed_sha == {"coder": "34278c681", "tester": "3c0c9ad5a"} is correct, and the empty-SHA documenter is rightly excluded.
• 11 CONSENSUS_ACK edges → len(latest_verdicts) == 11 is correct.
• 0 NACKs / 0 obligations → empty open_nacks/obligations is correct.

These are facts about the captured consensus, independently verifiable from the record — not outputs the implementation defined for itself. A future rename of metadata.version / commit_sha / to_role now breaks this test instead of silently emptying live anchors. This is exactly the cross-module shape-lock the hand-built fixtures couldn't provide.

#2 — caller-scoping docstring. Added and accurate: derive_brc_anchors does no slice/phase filtering (verified — it iterates the message list directly), and the docstring correctly states the per-slice/per-phase pre-scoping contract that brc-history/<id>-<phase>.json already satisfies.

#3 — last_reviewed_sha naming. Inline note at _assemble correctly clarifies it is a per-producer max-across-reviewers aggregate, distinct from per-edge latest_verdicts[].reviewed_sha.

#4 — timestamp sort path. test_timestamp_sort_path_recovers_chronology hands the record newest-first and asserts the v2 NACK (not the v1 ACK) is the surviving verdict — assertions that only hold if _ordered's (timestamp, id) sort runs. This genuinely exercises the path real records take and the timestamp-less fixtures never reached.

#5 / contract-note #1 — agent_model_resolution.py scope creep. Now moot, and the disagreement is correct: that change is not in the PR delta at all. It belongs to slice-2, which has advanced two commits (8bdbb9e2, 5b7fa58d "Decouple unknown-model window…") past this PR's branch point (216f1de2). It only appeared as a "reversal" in a two-dot slice-2..HEAD diff. Nothing for this PR to do.

The mypy fix (cast(list[dict[str, Any]], json.loads(...))) is trivial and correct.

Non-blocking note

• The PR branch is 2 commits behind its base egg/issue-3200/slice-2. Normal stacked-PR drift — the slice-2 refinements (_conservative_unknown_window, Claude-alias keying) will be picked up on the orchestrator-side rebase before merge. Worth a glance from the merger that the base is current, but not a code issue here.

The core derivation logic is unchanged from the prior approval; the delta is purely additive test coverage + documentation. No security surface (read-only, structured-fields-only, O(n)).

— Authored by egg

[james-in-a-box]

Contract re-verification — slice-3 "Derive" (issue #3200)

Verdict: Approve (no blocking issues). Posted as a comment because this PR is self-authored by the egg bot account.

Delta since last review

The last-reviewed commit 951f2cf7 is no longer in history — the PR branch has been collapsed to a single orphan commit (68193439, "Fix mypy no-any-return in test_brc_anchor_derivation"), so a literal 951f2cf..HEAD delta is not recoverable. I re-verified against a direct tree diff slice-2 → slice-3 and re-ran the slice's tests. The named change (a no-any-return mypy fix in the orchestrator test) introduces no regression; all derivation tests are green.

task-3-1 — deterministic derivation + additive model ✅

• shared/egg_anchor/brc_derive.py::derive_brc_anchors computes all four #3189 fields mechanically from structured message fields (message_type / from_role / to_role / metadata). It never reads agent prose — _nack_reason deliberately takes the structured reason, never the free-form body. Non-consensus messages are ignored; replay semantics (re-propose advances version, superseded verdicts/obligations become historical) mirror peer_consensus without depending on the orchestrator package.
• Model extension is additive (shared/egg_anchor/models.py): new BRCDerivedAnchors, ReviewEdgeVerdict, OpenNack, ConditionalAckObligation, and the ReviewVerdict enum; BRCState.derived is Optional, default None. BRCState.acks / nacks / last_message_id (the models.py:96-103 contract) are untouched and keep their agent-id-list meaning — confirmed by the no-skip test_legacy_brcstate_fields_untouched.
• __init__.py exports the new symbols.

The task AC ("four fields derive purely from the record; additive without breaking existing fields; match the fixture exactly; no agent-authored content") is met.

task-3-2 — fixture tests ✅

• shared/egg_anchor/tests/test_brc_anchor_derivation.py — 9 passed. Covers the full AC scenario: multiple producers, ACK→re-propose→NACK (current-version NACK reason surfaced, superseded v1 SHA correctly excluded), unresolved conditional-ACK obligation, resolved-vs-unresolved distinction, determinism, plus two regression tests added from prior review feedback (a captured real 37-message record locking live field shapes, and an out-of-order timestamped record exercising the (timestamp, id) sort path).
• orchestrator/tests/test_brc_anchor_derivation.py — 2 passed. Anchors ground truth to the real ApprovalMatrix substrate.

I traced every assertion against the derivation logic and they are consistent. Per review conventions I did not run the full suite; CI gates on the green check suite.

Note (non-blocking) — base drift, not slice-3 work

The slice-2 → slice-3 tree diff shows orchestrator/agent_model_resolution.py and orchestrator/tests/test_reseed_threshold.py differing: current slice-2 tightened _conservative_unknown_window (decoupled→min-coupled) and added two tests after slice-3 branched from it. These are slice-2/AC-3 territory, not slice-3's "Derive" scope, and reconcile on the orchestrator's controlled rebase onto the base branch. Flagging for awareness so a naive merge doesn't regress slice-2's newer state — it does not affect slice-3's contract compliance.

Contract-marking

Could not run egg-contract verify-criterion: the orchestrator is unreachable from this session, and the on-disk contract is a refine-phase snapshot whose top-level acceptance_criteria list is empty (no ac-N ids exist to mark). Slice-3's task-level acceptance criteria (prose) are both fully satisfied as detailed above.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Review feedback addressed

The two most recent reviews — egg-reviewer[bot] (APPROVED) and james-in-a-box[bot] contract re-verification (Approve) — are both at the current HEAD 68193439 and confirm the prior round of suggestions was properly addressed. CI is fully green at HEAD. Per-item disposition of every item raised:

Prior egg-reviewer suggestions (#1–#5) and james-in-a-box notes — already fixed-in-PR in commits 03ee22c / 68193439 and confirmed addressed in the re-reviews:

• Phases 1-2: Repository setup, docs, and gateway extraction (partial) #1 real-record regression test → test_real_record_locks_live_message_shape (commit 03ee22c).
• Phase 1: Repository setup and CI infrastructure #2 caller-scoping docstring → added to brc_derive module docstring (commit 03ee22c).
• Phase 1.5: Documentation extraction #3 last_reviewed_sha naming clarification → inline note at _assemble (commit 03ee22c).
• Phase 2: Gateway module extraction #4 timestamp sort-path test → test_timestamp_sort_path_recovers_chronology (commit 03ee22c).
• Phase 3: Container extraction #5 / contract-note 1 agent_model_resolution.py scope creep → disagree. The re-review confirmed this is not in the PR delta at all — it is slice-2 base drift that only surfaced in a two-dot diff. Nothing for this PR to change.

New note (both latest reviews) — base drift: PR branch 2 commits behind base egg/issue-3200/slice-2 — disagree (not a code issue; orchestrator-managed). Both reviewers explicitly flag this as non-blocking and "not a code issue here": the slice-2 refinements (_conservative_unknown_window, Claude-alias keying) are picked up via the orchestrator's controlled rebase onto the base branch before merge. In pipeline mode git rebase onto the base is gateway-blocked by design and reconciled orchestrator-side, so there is no in-PR action available or appropriate. Flagged for the merger's awareness only.

Contract criterion-marking note — no action needed. This sliced contract carries no top-level ac-N acceptance criteria (they live as per-task prose), so verify-criterion is N/A, as both reviewers observed.

No code changes were required this round — the latest reviews are approvals and CI is green.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

12 previous review(s) hidden.