docs: pull our type-boundary guidance into STYLE_GUIDE.md - #4648
Conversation
|
🐇 ✅ Action performedFull review finished. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
Walkthrough
ChangesStyle guide guidance
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (2)
STYLE_GUIDE.md (2)
162-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpecify the presence representation and absence semantics.
The guidance is correct but too broad for an API design contract. State which protobuf representation applies in each case, such as
optional, a singular message, a wrapper, oroneof. Also state whether omission differs from an explicit zero or default across Rust models and database records. This will prevent inconsistent implementations of the same API contract.As per coding guidelines, document requiredness, defaults, and interactions completely.
🤖 Prompt for 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. In `@STYLE_GUIDE.md` around lines 162 - 166, Expand the presence guidance in the protobuf/Rust/database/API contract section to map each cardinality case to a specific representation, including optional scalars, singular messages, wrappers, and oneof fields. Explicitly define omission versus explicit zero/default semantics and requiredness across protobuf messages, Rust models, database records, and update APIs, including defaults and mode-dependent field interactions.Source: Coding guidelines
170-172: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the compatibility rollout order explicit.
“Update or backfill existing data and writers” does not define how old readers behave or whether new readers accept omitted fields. Document an ordered rollout: deploy readers that accept old and new forms, update writers, backfill existing data, verify mixed-version and rollback behavior, then enforce the stronger invariant. Without this detail, teams can apply the guidance in an order that breaks older clients or rejects unmigrated records.
As per coding guidelines, document fallback behavior, interactions, and unsupported paths completely.
🤖 Prompt for 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. In `@STYLE_GUIDE.md` around lines 170 - 172, Update the compatibility guidance in the staged persisted/wire-field rollout section to specify this order: deploy readers that accept both omitted and present fields, update writers, backfill existing data, verify mixed-version and rollback behavior, and only then enforce the stronger invariant. Explicitly document fallback behavior, reader/writer interactions, and unsupported paths.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@STYLE_GUIDE.md`:
- Around line 162-166: Expand the presence guidance in the
protobuf/Rust/database/API contract section to map each cardinality case to a
specific representation, including optional scalars, singular messages,
wrappers, and oneof fields. Explicitly define omission versus explicit
zero/default semantics and requiredness across protobuf messages, Rust models,
database records, and update APIs, including defaults and mode-dependent field
interactions.
- Around line 170-172: Update the compatibility guidance in the staged
persisted/wire-field rollout section to specify this order: deploy readers that
accept both omitted and present fields, update writers, backfill existing data,
verify mixed-version and rollback behavior, and only then enforce the stronger
invariant. Explicitly document fallback behavior, reader/writer interactions,
and unsupported paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 90ab202d-a68b-432a-8074-b35c7a7629ce
📒 Files selected for processing (1)
STYLE_GUIDE.md
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 `@STYLE_GUIDE.md`:
- Around line 170-172: Expand the staged compatibility guidance in
STYLE_GUIDE.md to explicitly separate migration steps: first make readers accept
omitted fields through a documented fallback, then update all writers, backfill
existing records, and validate mixed-version clients before enforcing
requiredness. Define the field’s requiredness, omission fallback, and error
behavior for each compatibility state.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 440e8ec0-8570-41a1-aef9-b96ed765e815
📒 Files selected for processing (1)
STYLE_GUIDE.md
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
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 `@STYLE_GUIDE.md`:
- Around line 171-172: Update the enum-selected mode guidance in STYLE_GUIDE.md
to require explicit validation for fields incompatible with the selected enum
value, including documented errors and accepted-value interactions. State that
oneof or a separate request type should be used when the invariant must be
enforced structurally.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 25e34ea8-dc86-4395-9e65-7be319b45ceb
📒 Files selected for processing (1)
STYLE_GUIDE.md
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@STYLE_GUIDE.md`:
- Around line 931-933: Expand the newtype guidance in STYLE_GUIDE.md to require
documenting rejected boundary forms and their expected validation errors,
alongside accepted forms, for each wire, serde, database, or other
representation boundary. Explicitly state when a newtype accepts every
underlying value, and align the testing guidance with these documented
contracts.
- Around line 168-170: Update the guidance around three-state create/update
fields to require an explicit representation for preserve, clear (NULL), and
set, such as an update enum, nested Option, or field mask. Document each wire
state, its storage mapping, and invalid-input behavior, while retaining
Option<T> guidance for semantic absence where appropriate.
- Around line 165-166: Update the protobuf guidance near the existing oneof
recommendation to document the zero-member state: when exactly one alternative
is required, specify the behavior for an unset oneof and the validation error
returned. Keep the existing guidance for mutually exclusive alternatives
unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3f2a2d72-28bf-4f26-9442-28b80e0a960b
📒 Files selected for processing (1)
STYLE_GUIDE.md
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 `@STYLE_GUIDE.md`:
- Around line 168-170: Revise the field-omission guidance in STYLE_GUIDE.md
around “Carry semantic absence as Option<T>” to separate create and update
semantics. For create fields, require omission to be defined as infer, default,
or reject; for update fields, define omission behavior against the existing
value, including preserve, clear, reset, or other applicable outcomes. Also
require explicit zero/default behavior and error handling for both operations.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6e8bbae0-79d6-427c-8afb-b33e1cb73f18
📒 Files selected for processing (1)
STYLE_GUIDE.md
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
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 `@STYLE_GUIDE.md`:
- Around line 174-177: Revise the update-state guidance to require a complete
wire representation for preserve, clear, and set: either pair a field mask or
enum with a value field and explicitly define the clear convention, or use a
dedicated update oneof with explicit set and clear variants. Document how each
wire state maps through Rust to storage, including defaults and
invalid-combination errors, and ensure the Rust representation matches the
selected wire contract.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0dcc47e2-4c21-4646-b8a3-23ddb357cc40
📒 Files selected for processing (1)
STYLE_GUIDE.md
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
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 `@STYLE_GUIDE.md`:
- Around line 935-938: Update the structured-value guidance in STYLE_GUIDE.md to
replace the narrow “wire string” wording with general boundary representation
language. State that conversion should use whatever representation the interface
requires, including strings, bytes, numbers, or structured messages, while
preserving the recommendation to retain domain types internally.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d0fd345f-3510-47fb-8fa1-cd03ae052685
📒 Files selected for processing (1)
STYLE_GUIDE.md
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 `@STYLE_GUIDE.md`:
- Around line 169-179: Update the guidance section in STYLE_GUIDE.md to
explicitly distinguish complete-value updates from patch updates before
describing preserve, clear, and set. State that complete-value updates require
callers to resubmit unchanged fields and selector variants, while patch updates
interpret omission as preservation; define omission, preservation, fallback
behavior, and documented errors for both shapes, including invalid combinations.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0dc0b11f-1287-40aa-adb4-4e5acd87fbfe
📒 Files selected for processing (1)
STYLE_GUIDE.md
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4648.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@STYLE_GUIDE.md`:
- Around line 182-192: Update the patch-operation guidance near “Preserve, set,
and clear” to either define wire encoding, precedence, Rust representation,
storage outcome, and invalid-combination errors for reset, infer, and default,
or remove those operation names if unsupported. Ensure the documented
operations, mappings, defaults, fallback behavior, and errors are consistent.
- Around line 194-197: Update the boolean-mode guidance in the “Make modes
explicit” section to document that an omitted implicit proto3 bool reads as
false and cannot be distinguished from explicit false. State that
presence-sensitive modes must use optional bool or a semantic enum, and document
the validation error for omitted values when presence is required.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 19a95f55-1ccb-4391-ae38-c25354e0e076
📒 Files selected for processing (1)
STYLE_GUIDE.md
|
@coderabbitai full_review, thanks! |
|
🐇🔍
|
|
@coderabbitai full_review, thanks! |
|
🐇🔍
|
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
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 `@STYLE_GUIDE.md`:
- Around line 182-190: Expand the patch guidance in STYLE_GUIDE.md to define
field-mask selection independently from payload omission. Document behavior for
selected-but-omitted values, including whether they preserve, default, clear, or
fail validation, and specify precedence and invalid combinations for overlapping
parent and child paths such as schedule.spec and schedule.spec.timezone. Also
state whether selection is determined by update_mask, payload presence, or both,
including fallback, default, and error behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ae6aa1ca-f29e-43b9-8136-b453d0502320
📒 Files selected for processing (1)
STYLE_GUIDE.md
This is an attempt to capture the general NICo maintainer design principles and guidance around API presence and strong domain types for the codebase. This change is derived from the pre-OSS review corpus as a whole -- years of MRs and tens of thousands of comments and discussions. As such, it leans into the guiding design decisions and principles used to define and grow the project into the product we have today. The idea is to ensure we capture our core principles in `STYLE_GUIDE.md`. If any of those principles have changed, we should capture that too, ensuring we don't lose sight of why decisions were made as the codebase evolves with new contributors, human and agentic alike. For this change specifically, I focused on making API and internal types describe the real contract. The search surfaced related optionality, zero-value presence, mode-specific field, boolean-operation, identifier, address, and parse-once conversations throughout the corpus and across multiple participants. This pulls out the recurring parts: - Make optionality describe semantic absence across every layer. - Use explicit presence when zero is valid and unset means something different. - Use semantic operations and types when a boolean or group of optional fields hides distinct modes. - Treat new mandatory wire or persisted fields as compatibility migrations. - Parse and validate at the untyped boundary, then keep the domain type internally. It also keeps the important exception that not every primitive needs a wrapper. A newtype should enforce an invariant or prevent a real mix-up, and each boundary representation it crosses should remain deliberate and tested. Again, we can always adjust this now or later. The hope is that we don't lose the reasoning behind why we made certain decisions to get us where we are now, and can continue using that reasoning to help drive future decisions. This supports NVIDIA#4625 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
5 participants
This is an attempt to capture the general NICo maintainer design principles and guidance around API presence and strong domain types for the codebase. This PR is derived from the pre-OSS review corpus as a whole -- years of MRs and tens of thousands of comments and discussions. As such, it leans into the guiding design decisions and principles used to define and grow the project into the product we have today.
The idea is to ensure we capture our core principles in
STYLE_GUIDE.md. If any of those principles have changed, we should capture that too, ensuring we don't lose sight of why decisions were made as the codebase evolves with new contributors, human and agentic alike.For this PR specifically, I focused on making API and internal types describe the real contract. The search surfaced related optionality, zero-value presence, mode-specific field, boolean-operation, identifier, address, and parse-once conversations throughout the corpus and across multiple participants.
This pulls out the recurring parts:
It also keeps the important exception that not every primitive needs a wrapper. A newtype should enforce an invariant or prevent a real mix-up, and each boundary representation it crosses should remain deliberate and tested.
Again, we can always adjust this now or later. The hope is that we don't lose the reasoning behind why we made certain decisions to get us where we are now, and can continue using that reasoning to help drive future decisions.
Related issues
This supports #4625
Type of Change
Breaking Changes
Testing
Validation passed with
cargo make format-nightly,cargo make clippy,cargo make carbide-lints,rumdl check --config docs/.rumdl.toml STYLE_GUIDE.md,git diff --check, and Pandoc rendering. No tests were run because this is documentation-only.Additional Notes
This deliberately does not recommend wrapping every primitive. A newtype should add safety, and representation tests are expected only for the boundaries that type actually crosses.
Closes #4625