docs: update egg_anchor README for BRC anchor derivation [doc-updater] - #3264
Conversation
Document the new brc_derive.py module and associated models added in #3238 (slice-3 of #3200/#3189): - Add brc_derive.py to the Files table - Add ReviewVerdict enum to the Enums table - Add BRCDerivedAnchors, ReviewEdgeVerdict, OpenNack, and ConditionalAckObligation to the Key Sub-Models section - Add a Derivation section under Functions documenting derive_brc_anchors() Triggered by: #3238 Authored-by: egg
There was a problem hiding this comment.
Approve. Thorough review of this docs-only change against the implementation it documents (shared/egg_anchor/brc_derive.py, models.py, __init__.py). Every claim checks out.
Verification
I traced each README addition to the source:
ReviewVerdictenum (README.md:135) —ack,nack,conditional_ackexactly matchesmodels.py:68-70. ✓BRCDerivedAnchors(README.md:146) — the four fieldslast_reviewed_sha,latest_verdicts,open_nacks,conditional_ack_obligationsmatchmodels.py:166-181. ✓ReviewEdgeVerdict(README.md:147) —reviewer,producer,verdict,version,reviewed_shamatchmodels.py:118-122. ✓OpenNack(README.md:148) —reviewer,producer,version,reasonmatchmodels.py:133-136. ✓ConditionalAckObligation(README.md:149) —reviewer,producer,version,condition,resolvedmatchmodels.py:148-152; the#1998reference matches the model docstring. ✓derive_brc_anchors(messages)(README.md:174) — signature and "purely mechanical / structured fields only / caller passes a pre-scoped record" all faithfully summarize the module docstring andderive_brc_anchorsbody. ✓__init__.pyexports "derivation" (README.md:195) — confirmed:derive_brc_anchorsis imported and in__all__. ✓tests/covers BRC anchor derivation (README.md:201) — confirmed:tests/test_brc_anchor_derivation.pyexists. ✓brc_derive.pyFiles-table row (README.md:197) — file exists; description accurate. ✓
Completeness: the PR body claims five new exported types plus derive_brc_anchors. All six are documented, and brc_derive.__all__ exports only derive_brc_anchors (helpers are underscore-private), so nothing public is left undocumented.
Non-blocking note
README.md:146summarizeslast_reviewed_shaas "(producer → SHA)". That's the correct shape, but the code comment (brc_derive.py_assemble) is careful to note it's a per-producer max-across-reviewers aggregate, not a per-edge value — the per-edge reviewed SHA lives inlatest_verdicts[].reviewed_sha. The currentFielddescription inmodels.py:168("latest proposal any reviewer has verdicted on") captures this nuance; the README line drops it. Acceptable abbreviation for a sub-model summary, but consider mirroring the model's phrasing if you want the distinction visible at the README level.
No correctness, security, or consistency issues. The documentation does not drift from the code.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Thanks for the thorough review. Addressing the one non-blocking note:
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review — Approve. The single new commit (49e5dc2) since my last review at 68801d6 is a one-line README clarification that directly addresses the only non-blocking note from my prior review. It checks out against the implementation.
Delta reviewed
shared/egg_anchor/README.md:146 — the last_reviewed_sha summary was expanded from (producer → SHA) to:
producer → SHA of the latest proposal any reviewer has verdicted on — a per-producer max-across-reviewers aggregate, not a per-edge value; the per-edge reviewed SHA lives in
latest_verdicts[].reviewed_sha
Verification against source
I traced the new text to the production code, not just the prior comment:
brc_derive.py:218-233(_assemble) computeslast_reviewed_sha[producer]as the SHA of the highest version any reviewer has verdicted on for that producer — amaxoveredgeskeyed by producer. This is exactly a "per-producer max-across-reviewers aggregate." ✓- The inline comment at
brc_derive.py:219-223states verbatim that this is "NOT a per-edge value" and that "the per-edge reviewed SHA ... lives inlatest_verdicts[].reviewed_sha". The README now mirrors this. ✓ brc_derive.py:242confirms the per-edge value is populated intoReviewEdgeVerdict.reviewed_sha. ✓models.py:166-168Field description ("latest proposal any reviewer has verdicted on") is consistent with the expanded README phrasing. ✓
Markdown is well-formed (backticks balanced). No drift between docs and code. No correctness, security, or consistency concerns.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Reviewed the latest feedback. Both reviews from the automated reviewer are approvals, and the single non-blocking note was already resolved in the most recent commit. Per-item disposition:
No other actionable items: the first review's verification section traced every README claim to the source and found no drift; the second review confirmed the delta. No code changes were required in this round, so nothing new to push. — Authored by egg |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…-230715-3146/work # Conflicts: # shared/egg_anchor/README.md
Conflict Resolution SummaryResolved merge conflicts with
DetailBoth conflicts in
Verified the resolved Files table against the actual package contents ( Please review: Nothing security- or logic-sensitive — README-only, no code changed. The combined table rows accurately reflect the merged package surface. — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review — Approve. The only change since my last review at 49e5dc2 is the merge commit fbdb59e, which resolved a conflict with main (#3263 concurrently added the protected_root.py docs to the same README tables). The resolution is purely additive and correct — no PR content was lost and no drift was introduced.
Delta reviewed
The merge touched only shared/egg_anchor/README.md. I verified the resolved tables against the actual package surface at the PR head (fbdb59e):
- Files table combines both sides correctly: this PR's
brc_derive.pyrow + main'sprotected_root.pyrow both present; the__init__.pyrow reads…validator, derivation, protected root renderer; thetests/row merges to…BRC anchor derivation, and protected root renderer. ✓ git ls-treeconfirms bothbrc_derive.pyandprotected_root.pyexist. ✓__init__.pyexportsderive_brc_anchors, plusRootCaps,render_protected_root— exactly what the merged table claims. ✓
Model docs re-verified against source
I re-checked every documented sub-model against models.py to confirm the merge didn't perturb this PR's additions:
ReviewVerdict→ack,nack,conditional_ack(models.py:60). ✓ReviewEdgeVerdict→reviewer,producer,verdict,version,reviewed_sha(models.py:109). ✓OpenNack→reviewer,producer,version,reason(models.py:125). ✓ConditionalAckObligation→reviewer,producer,version,condition,resolved(models.py:139). ✓BRCDerivedAnchors→last_reviewed_sha,latest_verdicts,open_nacks,conditional_ack_obligations(models.py:155). ✓
The last_reviewed_sha per-producer-aggregate clarification from the prior round survived the merge intact and still matches models.py:166-168 / brc_derive.py.
Markdown is well-formed (tables aligned, backticks balanced). Docs-only, no code changed. No correctness, security, or consistency concerns.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
|
Reviewed the latest feedback. All three reviews from the authorized reviewer ( Per-item disposition:
The second re-review (commit — Authored by egg |
|
egg feedback addressed. View run logs 9 previous review(s) hidden. |
Update
shared/egg_anchor/README.mdto document the new BRC anchor derivation layer added in #3238 (slice-3 of #3200/#3189).The merged PR added
brc_derive.py— a new module inegg_anchorthat mechanically derives four structured anchor fields (BRCDerivedAnchors) from the BRC message record. It also added five new exported types:ReviewVerdict,ReviewEdgeVerdict,OpenNack,ConditionalAckObligation, andBRCDerivedAnchors. None of these were reflected in the library README.Changes:
brc_derive.pyto the Files table with a description of its purposeReviewVerdictenum to the Enums tablederive_brc_anchors()No other docs needed updating:
brc-memory.mdcovers the ephemeral markdown memory artifact (a separate concern), andanchor-recovery.md/coordination-state.mddon't discuss the derivation layer.Triggered by: #3238
Test Plan
shared/egg_anchor/brc_derive.pyandshared/egg_anchor/models.pyAuthored-by: egg