docs(process): add contract registry and milestone design-review gate (#115) - #116
Conversation
- Add docs/08_contract_registry.md indexing wire surfaces and stable identifiers - Rows link to normative specs only; the registry never restates contract text - Planned rows name their design authority (#114 pending, #106 ADR, ADR-0014) - Register the new document in the 00_overview document map Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Add workflow section 7: one breadth-first pass when a milestone is assembled - 7.1 shared-mechanism inventory creates unifying design issues up front - 7.2 checks new contract surfaces against the docs/08 registry - 7.3 annotates cross-issue dependencies with their shared substrate and adds a new-consumer intake re-read - 7.4 standardizes the planned-not-normative banner for forward-written specs - Appended as section 7 so existing section references (2.1, 3) keep numbering Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a public contract registry for wire surfaces and stable identifiers, documents its governance rules, adds a milestone design-review gate, and integrates registry checks into ADR and pull-request workflows. ChangesContract governance documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/00_overview.md`:
- Around line 80-85: Update the “Public documents (00–08)” statement in the
overview to match the documents marked Public in the table, including
09_dependency_policy.md, 10_adr_process.md, and development_workflow.md, or
revise the table’s classifications so both references consistently describe the
same public-document scope.
In `@docs/08_contract_registry.md`:
- Around line 13-14: Clarify Rule 1 in docs/08_contract_registry.md by defining
the required pre-implementation workflow for genuinely new wire surfaces or
stable identifiers: specify whether authors must add a Planned registry row
before issue review or reference a proposed row, and ensure the rule
consistently applies before the implementing PR updates that row.
- Line 36: Update the acknowledgment entry in the contract registry table to
explicitly distinguish ownership: reference Issue `#14` for transport
acknowledgment details, Issue `#17` for ParamStore policy, and Proposal `#114/its`
ADR only for the remaining design decisions. Ensure the linked authority
references consistently direct implementers to the correct owner.
- Around line 17-20: Clarify rule 3 in docs/08_contract_registry.md by defining
how a proposal Issue transfers authority to its resulting ADR and identifying
the exact Accepted status event that makes planned contract details normative.
Align this wording with the §7.4 ownership model, while ensuring planned rows
remain tentative before that acceptance and cannot remain indefinitely without a
completed handoff.
In `@docs/development_workflow.md`:
- Around line 169-171: Update the milestone design review gate in
docs/development_workflow.md to require coverage of §7.1–§7.4, including the
planned-contract banner check defined in §7.4, before milestone issue
implementation begins.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e75e762c-10bb-45d4-9715-936b3291b311
📒 Files selected for processing (3)
docs/00_overview.mddocs/08_contract_registry.mddocs/development_workflow.md
Review verdict: fix before mergeThe overall mechanism is valuable, the diff is documentation-only and focused, relative links resolve, 1. New-surface registration happens too late
This defeats the early-detection goal: overlapping Issues can both observe “no row” and continue until an implementation PR. Smallest fix: require a new Planned row during Issue/Proposal review or milestone assembly, before implementation starts. The implementation PR should advance its implementation state rather than perform first registration. Add 2. The registry conflates contract maturity and implementation state
Smallest fix: split the current Status column into:
Then represent buffers as Normative + Planned implementation. 3. ACK identifier maturity is internally inconsistentThe stable-identifier table combines session ID and ACK UUID grammar as There are also two distinct contracts:
Smallest fix: split session ID, accepted ACK-route ID grammar, and generated correlation-ID format into separate rows. Either document the current parser grammar normatively in 4. Planned identifiers are mixed into normative rowsThe normative implemented Smallest fix: keep current implemented identifiers in normative rows and add separate Planned rows, with their Issue/ADR design authority. Names that remain undecided can be described generically without inventing final enum values. 5. The initial inventory is incompleteThe registry says it covers everything observable by another process over Zenoh, but has no row for:
These are implemented, externally observable contracts and are likely overlap points for future query or discovery mechanisms. Smallest fix: add rows at a useful semantic granularity and include an inventory table in the PR verification mapping implemented routes/codecs and planned Issues to registry rows. The Issue #115 checklist should also be copied into the PR body with completed items, as required by the repository workflow. 6. The shared-mechanism trigger is over-broad
Smallest fix: limit the trigger to a new, unresolved, or materially changed cross-component mechanism used by two or more Issues and lacking an existing contract owner. If an existing owner/ADR applies, the review should reference it instead of creating a redundant Issue. 7. Gate completion and re-scope timing need clarificationThe gate currently requires only posting a review artifact; it does not require an accepted outcome or disposition of findings. “Materially re-scoped” can also occur after some milestone implementation has started, making the current “before implementation of the milestone's Issues begins” wording impossible. Smallest fix: require findings, follow-up ownership, and maintainer/owner acceptance. For re-scope, rerun the gate before added/changed scope begins and pause only affected work. Cover §7.1–§7.4. Minor consistency fixes
No project files or GitHub relationships were modified by this review. |
- Split the registry Status column into Contract status, Implementation status, and Design authority so a normative-by-ADR contract with pending code (buffers/<sid>, ADR-0014 Accepted) is no longer mislabeled Planned - Split session-id, accepted meta/ack route grammar (implemented, lenient), and generated UUIDv4 correlation-id (planned #114) into separate rows - Move planned identifiers (OutcomeUnknown, OutcomeUnknownError) out of the normative rows; add planned rows for #107 fence identifiers and #108 state-lost result - Add rows for op-to-key mapping (03 §3), query semantics (03 §4), and Encoding normalization (03 §2.2) - Route the ack surface authority to current owners #14/#17 with #114 as a pending consolidation proposal, not a settled authority - Reframe the never-restate claim: row cells are non-normative summaries; linked specs are authoritative - Fix the overview public-document sentence to match the document map Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Register new surfaces at issue/proposal review or milestone assembly (registry Rule 1) instead of at first implementation PR, so overlapping issues cannot both observe no row and proceed - Scope the 7.1 unifying-issue trigger to new, unresolved, or materially changed cross-component mechanisms lacking an existing contract owner - Require the gate to cover 7.1-7.4 and to complete only with recorded findings, follow-up owners, and owner acceptance; rerun on material re-scope, pausing only affected work - Add the affected contract-registry row(s) to the required Issue fields and the PR template Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review response — updated by OpusThanks — the reviews caught genuine design flaws, and there is a fair irony that a contract-registry Root cause and fixThe core defect (Sol #2/#3/#4) was one thing: the single Status column mashed three orthogonal DispositionSol findings
CodeRabbit findings — B (registration timing) = Sol #1; C (Rule 3 handoff) fixed: a proposal One partial declineThe automated Description check flags missing Requirements IDs / AC logs / RED / ADR checkbox. Still docs-only; |
- Clarify that the branch prefix is always feat/; change type lives on the issue label and commit message, so the branch name does not repeat it - Prevents inferring type-based prefixes from the commit-type list Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-review verdict: fix before mergeReviewed at head A few contract/process inconsistencies remain. 1. Issue #115 still defines the old scopeThe Issue body still says that the implementation PR first adds/updates a row, uses one normative/planned status axis, and creates a unifying design Issue for any mechanism shared by two or more Issues. The PR now intentionally implements different rules: pre-implementation Planned registration, independent contract/implementation axes, and a scoped ownerless-cross-component trigger. This conflicts with Smallest fix: update Issue #115's Scope and Acceptance Criteria to the reviewed rules, then copy that finalized checklist into the PR body. 2. The required field is not present in the actual PR template
Smallest fix: add an 3. The registry violates its own single-authority model
Normative rows also place implementation locations such as Smallest fix: give each Planned row exactly one authority ( 4. Key-space and ACK rows overlap ambiguouslyThe key-space row marks Smallest fix: scope the first row to explicit implemented path grammar, separate parser grammar from ACK route behavior, and rename the de-facto parser row without “Accepted.” 5. The #108 planned-result inventory is incompleteThe registry includes only Smallest fix: register the state-lost and catalog-unavailable results separately (or use one clearly scoped #108 row that explicitly includes both) under one authority. 6. Registry Rule 2 is not connected to the ADR decision flowRegistry Rule 2 requires an ADR for a second overlapping-purpose surface, while the PR checklist still asks only whether Smallest fix: change the workflow and PR-template ADR field to check Once these are corrected and the remaining checks pass, the mechanism should be ready to merge. |
- Give every Planned row exactly one design authority; settled normative rows use '—' and list implementers/consumers in a separate column - Move src/key.cpp, ParamStore, and StorageNode routing out of the authority column into Implementer / consumers - Scope the key-space row to path grammar (meta/** route shapes) and the ack row to route behavior so the two no longer overlap ambiguously - Rename the lenient meta/ack parser-grammar row to drop 'Accepted', which collided with the Contract-status wording - Register the #108 typed catalog-unavailable result alongside the state-lost read result Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ate (#115) - Add the Contract Registry field to .github/pull_request_template.md so the required section documented in workflow §4 exists in the actual template - Add registry Rule 2 (overlapping surface) as an ADR trigger in docs/10 §6, giving ADR requirements one consistent entry point - Reference '§6 or registry Rule 2' in the workflow §4 ADR field and the PR template ADR section Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-review response — updated by OpusAll six re-review findings applied (head now Disposition
Still docs-only; |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/pull_request_template.md:
- Around line 9-12: Update the Contract Registry prompt in the pull request
template to require authors to list all affected contract-registry rows and
explicitly state the status transition: Implementation, Contract, or none. Do
not limit the request to rows that advanced, so changes with no registry status
update still report affected rows and the applicable authority.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de030263-7c25-4492-a6fe-188ab4b4f0df
📒 Files selected for processing (4)
.github/pull_request_template.mddocs/08_contract_registry.mddocs/10_adr_process.mddocs/development_workflow.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/08_contract_registry.md
- docs/development_workflow.md
- The meta/ack route-id-grammar row listed two design authorities (#14/#114); set it to #114 to match the ack wire-surface row and record #14 as the current owner in notes, satisfying the one-authority rule - Remove the redundant '§6 or Rule 2' phrasing now that registry Rule 2 is a trigger inside docs/10 §6; §4 and the PR template point at §6 as the single ADR-trigger source Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review — FIX BEFORE MERGEFindings
Validation
|
- Track affected rows plus their status transition (Contract / Implementation / none) in §2.1, §4, and the PR template, so a contract change with no status transition is no longer reportable as N/A - Set buffers/<sid> Design authority to '—' (settled normative per the intro rule); the ADR-0014 link stays in the Normative spec column - Set both meta/ack rows' Design authority to '#14 → ADR' (the current owner per docs/03 §6), with #114 recorded as a pending consolidation rather than a pre-anticipated authority; #17 is the ParamStore policy consumer Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Re-review response — updated by OpusAll three findings applied (head
On the ACK point specifically: I'm treating your two rounds as consistent, not contradictory — the Still docs-only; |
Re-review — MERGEThe current head ( Resolved
Validation
Note: when PR #117's ADR-0028 is eventually Accepted, update the related registry row's spec, status, and authority under Rule 1 introduced by this PR. |
1 participant
Summary
docs/08_contract_registry.md: an index of every wire surface and stable identifier, trackedalong three axes (Contract status / Implementation status / single Design authority), with rules
making registration happen at issue review and an overlapping-purpose second surface require an ADR
docs/development_workflow.md§7: a milestone design-review gate (shared-mechanism inventorywith a scoped trigger, registry check, dependency-substrate annotation, new-consumer intake,
planned-not-normative banner) with explicit completion/re-scope criteria
docs/10§6) and the PR templatedocs/00_overview.mddocument mapCloses #115
Issue #115 checklist (finalized — see the reviewed-rules addendum on #115)
docs/08_contract_registry.md: wire-surface and stable-identifier tables, each row carryingContract status, Implementation status, Normative spec, a single Design authority, and
implementers/consumers separately
assembly; overlapping-purpose second surface requires an ADR (wired into
docs/10§6 and thePR template); a Planned row has exactly one authority and becomes Normative only when its
owning ADR is Accepted; the registry is an index, not a spec
docs/development_workflow.md§7 (scoped shared-mechanism trigger, registry check with in-gateregistration, dependency substrate annotation, new-consumer intake, planned-not-normative
banner, gate completion + re-scope criteria)
docs/00_overview.md§5: registry added to the document mapNotes for review
architecture decision and changes no contract. Registry Rule 2 is now itself an ADR trigger, but
this PR introduces no overlapping surface.
docs/here; the convention (now clarified in §1) fixes it tofeat/goingforward. Left as-is per owner decision; not renamed.
Inventory verification (registry rows → source of truth)
src/key.cpp,03§1/§3/§4ParamValuecodec,03§2.1/§2.2/§5meta/sessionreply03§7.1buffers/<sid>(Normative, impl Planned)meta/ackparser grammarsrc/key.cppIsValidSessionId/IsValidAckUuidStatus/ Python exceptions (implemented rows only)04§1.1,05§2.1Verification
(
03§1/§2.1/§2.2/§3/§4/§5/§6/§7.1,04§1.1,05§2.1, ADR-0014,10§6).git diff --checkpasses; SonarCloud quality gate passes.🤖 Generated with Claude Code