OSAC-2766: Design - Type-Safe Resource References - #121
Conversation
|
Warning Review limit reached
Next review available in: 19 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: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (2)
WalkthroughAdds a PRD and design for replacing opaque resource-reference strings with typed protobuf references, centralized request validation, nested consumer wire formats, database/CEL updates, and an incremental rollout and testing strategy. ChangesType-safe resource references
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ReferenceValidator
participant DAO
participant ServerHandler
Client->>ReferenceValidator: Send typed-reference request
ReferenceValidator->>DAO: Resolve referenced resources
DAO-->>ReferenceValidator: Return lookup results
ReferenceValidator->>ServerHandler: Pass validated and populated request
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
AI Design Review: EP-121Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured enhancement proposal with concrete implementation details, sound architectural decisions (interceptor + defense-in-depth triggers), a realistic incremental delivery plan, and comprehensive test strategy across all three levels. Feedback: The proposal is missing the User Stories section required by the template — the Workflow Description partially compensates but explicit 'As a , I want...' stories would strengthen the motivation and help reviewers validate that all personas are covered. The database backfill strategy ('UPDATE table SET data = data') should include concrete batching detail (batch size, expected runtime, locking implications) rather than a one-line acknowledgment, especially for tables that could grow large. Consider adding a proto lint rule or custom protobuf option to enforce the Reference/LocalReference naming convention programmatically, rather than relying solely on convention — this would prevent accidental false-positive detection by the interceptor if a non-reference message is ever named with a 'Reference' suffix. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-121Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-architected enhancement proposal with precise scope, concrete implementation details, sound architectural patterns (interceptor + DB triggers for defense-in-depth), and a comprehensive test strategy across three tiers. Feedback: The Alternatives section is essentially empty, deferring to an external PRD discussion -- reviewers need to see the trade-off reasoning inline, particularly the URI/ARN vs. per-type message decision and why a generic Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-121Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-architected design document that covers all major dimensions (proto schema, interceptor, consumers, delivery, testing, operations) with concrete examples and sound technical decisions; the only gaps are the missing User Stories section and a thin Alternatives section. Feedback: Add a User Stories section under Motivation as required by the template — include stories for at least the Tenant User, Tenant Admin, and Developer personas to ground the motivation in concrete user needs. Expand the Alternatives section to briefly summarize the rejected approaches (URI/ARN format, generic reference message, do nothing) inline rather than deferring entirely to the PRD discussion — reviewers should be able to evaluate trade-offs without leaving the document. Consider providing a recommended default for Open Question 1 (status-level typed references) so the chunk implementers have a clear starting point rather than waiting for controller maintainer feedback. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-121Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured design that covers all layers (proto, interceptor, DB, CLI, UI) with sound architectural decisions, a practical incremental delivery plan, and comprehensive test strategy — only minor template compliance gaps (missing User Stories) and a thin Alternatives section hold it back from perfection. Feedback: Add a User Stories section to the Motivation — the template requires it, and it would ground the three problem classes in concrete personas (e.g., 'As a tenant admin, I want to reference a platform-scoped NetworkClass by name so that the system validates the reference exists before I deploy infrastructure'). The Alternatives section should document the evaluated options (URI/ARN format, generic reference message, do-nothing) inline rather than pointing to PR #113 — reviewers shouldn't need to leave the document to understand the design trade-offs. Consider whether naming-convention-based reference detection (*Reference/*LocalReference suffix) should be hardened with a custom proto option or annotation to prevent false positives if a non-reference message name happens to match the pattern. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
bb94768 to
fad3fbd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
AI EP Review: EP-121Score: 10/10 | Verdict: PASS
Verdict: A thorough, well-structured design document with clear user-facing outcomes, concrete business justification, an extremely detailed implementation plan, and appropriate scope — one of the stronger enhancement proposals in the OSAC project. Feedback: The Alternatives section is too thin — it defers all reasoning to an external PR discussion rather than documenting the tradeoffs inline. The review-patterns guide flags this as an anti-pattern. Summarize the URI/ARN format, generic reference message, and do-nothing alternatives with 1-2 sentences each on why they were rejected, so reviewers don't need to hunt through PR comments. The two open questions (status-level reference typing, CEL filter communication strategy) should be resolved before merging to avoid scope ambiguity during implementation. Critical (0)None. Important (1)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-121Score: 7/8 | Verdict: PASS
Verdict: A strong, deeply technical design that demonstrates thorough understanding of the OSAC architecture and provides implementation-ready detail, held back from a perfect score by missing user stories and a thin Alternatives section. Feedback: Add explicit user stories in the 'As a [role], I want [action] so that [goal]' format for all affected personas — at minimum Tenant User, Tenant Admin, Cloud Provider Admin (who manages global catalogs/templates referenced cross-tenant), and Cloud Infrastructure Admin (who manages platform-scoped NetworkClass, IP pools). The Alternatives section needs at least one real alternative evaluated inline (e.g., the URI/ARN format or generic Reference approach mentioned in the PRD discussion) with trade-offs and rejection rationale — pointing to an external PR discussion doesn't satisfy the template requirement. Consider explicitly addressing or deferring the Documentation and Installation cross-cutting dimensions from osac-dimensions.md. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
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 `@enhancements/type-safe-resource-references/design.md`:
- Around line 534-538: Update the reference validation design so every detected
reference message type must have a registered lookup function: reject missing
registrations during startup configuration, and return a server error when an
unregistered reference is encountered at runtime instead of passing it through
unvalidated. Apply this consistently to both reference-detection and
runtime-validation flows.
- Around line 309-324: Update the rollout guidance in the attachment migration
section to explicitly require flattening attachment payloads from target: {
case, value } into the selected top-level oneof field, such as public_ip or
external_ip, containing a { name: value } reference. Clarify that
useCreatePublicIPAttachment and useCreateExternalIPAttachment call sites need
this structural change in addition to wrapping string references, and preserve
the documented proto wire format.
- Around line 580-587: Update the forward-reference removal design around
reverse delete checks so database-side race protection remains: retain an
appropriate constraint or locking mechanism, or explicitly serialize validation,
insertion, and deletion. Ensure concurrent child insertion and parent deletion
cannot both commit while leaving a dangling reference, while preserving the
updated JSON paths and reverse-reference trigger behavior.
- Around line 264-278: Resolve the catalog-item reference scope mismatch by
choosing either tenant-local or shared/full semantics for compute instance
catalog items, then consistently update ComputeInstanceCatalogItemReference or
ComputeInstanceCatalogItemLocalReference across the proto definitions, reference
registry, CLI, UI, and wire examples. Ensure the API inventory and reference
matrix use the same symbol and scope model.
- Around line 301-305: Update the subnet creation mapping for
CreateSubnetInput.virtualNetworkId to use the parent virtual network name, not
subnetName, inside spec.virtual_network.name. Rename the input/local variable
from virtualNetworkId to a name-based identifier as needed, while preserving the
existing security-group and virtual-network mappings.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 590f995c-aae6-4f09-b139-2f99204664b0
📒 Files selected for processing (1)
enhancements/type-safe-resource-references/design.md
AI Design Review: EP-121Score: 7/8 | Verdict: PASS
Verdict: A thorough, well-architected design with deep implementation detail and concrete test scenarios; the main weakness is scope presentation — missing formal user stories, a thin Alternatives section, and incomplete persona/dimension coverage. Feedback: Add a User Stories subsection under Motivation with formal 'As a [role]' stories for all four OSAC personas (especially Cloud Provider Admin, who manages global catalogs and cross-tenant template references affected by this change). Bring the Alternatives section inline — the rubric requires at least one real alternative with trade-off analysis in the document itself, not a pointer to PR discussion. Address or explicitly defer the Installation and Documentation cross-cutting dimensions (e.g., does the interceptor require any Helm chart configuration? Which API reference docs need updating per chunk?). Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-121Score: 7/8 | Verdict: PASS
Verdict: A thorough, well-architected design with deep implementation specificity that is ready for implementation; the main weakness is incomplete persona coverage and a thin Alternatives section that delegates analysis to an external PR discussion. Feedback: Add a formal 'User Stories' subsection under Motivation covering all four OSAC personas -- Cloud Provider Admin (managing global catalogs/templates with typed references) and Cloud Infrastructure Admin (managing platform-scoped resources like NetworkClass and IP pools) are missing from the current workflows. Bring the alternatives analysis into the document itself rather than pointing to PR #113 -- reviewers should not need to leave the document to understand why URI/ARN format and generic reference messages were rejected. Address the Tenant Onboarding, Documentation, and Installation dimensions from osac-dimensions.md explicitly, even if only to state they are not affected. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
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 `@enhancements/type-safe-resource-references/design.md`:
- Line 264: Update the proto API inventory so InstanceTypeLocalReference is
listed only under instance_type_type.proto, removing it from
compute_instance_type.proto. Keep compute_instance_type.proto’s usage by
importing the owning proto definition, preserving the single-definition rule and
avoiding duplicate generated symbols.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3c25582f-7e6c-4425-a851-6567a6fb8eed
📒 Files selected for processing (1)
enhancements/type-safe-resource-references/design.md
AI Design Review: EP-121Score: 7/8 | Verdict: PASS
Verdict: A thorough, well-architected design with strong technical depth and testability, held back from a perfect score by missing formal user stories, an effectively empty Alternatives section, and some unaddressed cross-cutting dimensions. Feedback: Add formal user stories in 'As a [role], I want to [action] so that [goal]' format for all affected personas, especially Cloud Provider Admin (IAM references in Chunk 5) and Cloud Infrastructure Admin (platform-scoped resources like NetworkClass). Document the URI/ARN and generic-reference-message alternatives inline with rejection rationale rather than pointing to the external PR #113 discussion — the design should be self-contained. Explicitly address or defer the documentation, installation, and provisioning dimensions from osac-dimensions.md to close the silence gaps. Critical (0)None. Important (4)
Suggestions (4)
Review costModel: claude-opus-4-6 |
rccrdpccl
left a comment
rccrdpccl
left a comment
There was a problem hiding this comment.
overall looks good, just a few clarifying questions
AI Design Review: EP-121Score: 7/8 | Verdict: PASS
Verdict: A thorough and well-architected design scoring 7/8 — strong in architecture, feasibility, and testability, held back from a perfect score by missing user stories, a hollow alternatives section, and incomplete cross-cutting dimension coverage. Feedback: Add a User Stories subsection under Motivation with formal 'As a [role], I want to...' stories covering at least Tenant User, Tenant Admin, and Cloud Provider Admin (who manages global catalogs and templates that are cross-tenant referenced). Flesh out the Alternatives section inline — the three alternatives (URI/ARN format, generic reference message, do nothing) are named but their trade-offs and rejection rationale should be explained in the design itself rather than deferring to an external PR discussion. Address or explicitly defer the Installation and Documentation cross-cutting dimensions. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
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 `@enhancements/type-safe-resource-references/design.md`:
- Around line 595-598: Update the trigger migration and JSON-path matching
design to require tenant and project predicates alongside resource names,
matching the interceptor’s scoping rules. Apply these constraints consistently
to forward triggers, reverse triggers, and their related indexes, and add an
integration test covering same-name resources across tenants to verify they
remain distinct.
- Around line 571-577: Update the interceptor ordering in the documented chain
so Auth runs before Transaction, ensuring authentication completes before
opening a database transaction. Revise the surrounding explanation to state that
the interceptor runs after Auth for tenant context while DAO lookups still share
the request transaction.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b27b1267-14d7-47ff-8cf0-c7d2d7e70567
📒 Files selected for processing (1)
enhancements/type-safe-resource-references/design.md
…hared - Add blank line before fenced JSON block (MD031) - Add concurrent create/delete integration test scenario - Fix mutation example to show public (shared) vs private (tenant) forms - Move id-only/both-match tests to full reference field (CatalogItem) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
Match the required OSAC-<jira-key>-<slug> naming convention. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
Local references now include an id field alongside name, matching the same resolution modes as full references (name-only, id-only, both with consistency check). This allows clients currently using resource IDs to continue working during the transition to name-based references. Updated: proto examples, CLI section (--field-id flags), interceptor description, before/after example, and test plan. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
- Add project field to private API mutation example - Add blank lines before CLI code fences (MD031) - Add scope context (caller vs shared tenant) to CatalogItem test cases Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
Address CrystalChun's review feedback: - Add `string project` field to public full references so tenant users can scope lookups within a project (empty = tenant-global) - Add `bool shared` field to private full references per API Guidelines (private must be a superset of public) - Add project-level access disclaimer in Security section - Update CLI flags, mutation examples, test plan, and resolution logic Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
7ab5da9 to
b8be671
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
enhancements/OSAC-1330-type-safe-resource-references/design.md (3)
662-684: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftBound and batch interceptor validation work.
The interceptor recursively validates every repeated/nested reference and aggregates every failure, but the design specifies no maximum reference count, deduplication, batching, or error cap. Large repeated fields can create N+1 DAO queries and oversized error responses. Add per-request bounds, deduplicate lookups, batch where possible, and honor request deadlines.
🤖 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 `@enhancements/OSAC-1330-type-safe-resource-references/design.md` around lines 662 - 684, Update the interceptor’s recursive message-walking and error-aggregation design to enforce a per-request maximum reference count and error cap, deduplicate identical lookups, batch DAO queries where supported, and propagate request deadlines through validation. Preserve complete FieldViolation paths for reported failures while preventing unbounded queries and oversized responses.
95-103: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlign the overview with the canonical reference schemas.
This section says full references contain
id, tenant, project, nameand local references contain onlyname, but the schemas below define public full references asid, name, project, shared, private full references as those fields plustenant, and local references asid, name. This contradiction can produce incompatible proto implementations.🤖 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 `@enhancements/OSAC-1330-type-safe-resource-references/design.md` around lines 95 - 103, Update the overview in the “Per-type reference messages in proto” section to match the canonical schemas below: describe public full references as id, name, project, and shared; private full references as those fields plus tenant; and local references as id and name. Ensure the field-replacement guidance uses these corrected definitions consistently.
717-721: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep resource names immutable when triggers reference them.
The name-based reverse triggers mean deleting a referenced resource by name can either leak stale references or become blocked after the name is reused. Either treat
metadata.nameas immutable for referenced resources, or add atomic rename propagation plus a delete/reuse policy that preserves referential integrity.🤖 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 `@enhancements/OSAC-1330-type-safe-resource-references/design.md` around lines 717 - 721, Define and enforce an immutability rule for resource metadata.name whenever the resource is referenced by triggers, preventing renames that would invalidate name-based reverse references. Ensure trigger validation and update paths preserve the existing name for referenced resources, while leaving unreferenced resource behavior unchanged.
♻️ Duplicate comments (1)
enhancements/OSAC-1330-type-safe-resource-references/design.md (1)
723-741: 🗄️ Data Integrity & Integration | 🟠 MajorDefine the complete uniqueness scope for trigger lookups.
The design now treats
projectas a lookup dimension and includes project-scoped tests, while same-tenant trigger predicates and indexes include onlytenant. If names can repeat across projects, triggers can match the wrong resource. Either includeprojectin predicates/indexes or explicitly guarantee tenant-wide name uniqueness, with a cross-project same-name test.Based on learnings, trigger indexes must use the complete name-uniqueness scope.
🤖 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 `@enhancements/OSAC-1330-type-safe-resource-references/design.md` around lines 723 - 741, Clarify the name-uniqueness scope for same-tenant trigger lookups in the design. If names may repeat across projects, update the predicates and associated indexes to include project alongside tenant for forward and reverse triggers; otherwise explicitly guarantee tenant-wide uniqueness and add a cross-project same-name test. Ensure the documented trigger index scope matches the complete uniqueness rule.Source: Learnings
🤖 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 `@enhancements/OSAC-1330-type-safe-resource-references/design.md`:
- Around line 615-623: Update the interceptor’s reference-resolution flow to
derive local-reference tenant/project from the parent resource’s scope rather
than caller auth context. Preserve explicitly resolved tenant/project values for
full references, and ensure the lookup filter always enforces both scope fields
instead of selecting by id or metadata.name alone.
---
Outside diff comments:
In `@enhancements/OSAC-1330-type-safe-resource-references/design.md`:
- Around line 662-684: Update the interceptor’s recursive message-walking and
error-aggregation design to enforce a per-request maximum reference count and
error cap, deduplicate identical lookups, batch DAO queries where supported, and
propagate request deadlines through validation. Preserve complete FieldViolation
paths for reported failures while preventing unbounded queries and oversized
responses.
- Around line 95-103: Update the overview in the “Per-type reference messages in
proto” section to match the canonical schemas below: describe public full
references as id, name, project, and shared; private full references as those
fields plus tenant; and local references as id and name. Ensure the
field-replacement guidance uses these corrected definitions consistently.
- Around line 717-721: Define and enforce an immutability rule for resource
metadata.name whenever the resource is referenced by triggers, preventing
renames that would invalidate name-based reverse references. Ensure trigger
validation and update paths preserve the existing name for referenced resources,
while leaving unreferenced resource behavior unchanged.
---
Duplicate comments:
In `@enhancements/OSAC-1330-type-safe-resource-references/design.md`:
- Around line 723-741: Clarify the name-uniqueness scope for same-tenant trigger
lookups in the design. If names may repeat across projects, update the
predicates and associated indexes to include project alongside tenant for
forward and reverse triggers; otherwise explicitly guarantee tenant-wide
uniqueness and add a cross-project same-name test. Ensure the documented trigger
index scope matches the complete uniqueness rule.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 4f7db594-da6b-4386-90b8-1d6f78ed6b50
📒 Files selected for processing (1)
enhancements/OSAC-1330-type-safe-resource-references/design.md
AI EP Review: EP-121Score: 10/10 | Verdict: PASS
Verdict: A thorough, well-structured design document that clearly articulates the problem (type-unsafe string references), provides a detailed and measurable solution (per-type proto messages + centralized interceptor), and demonstrates strong engineering rigor across security, observability, delivery planning, and testing. Feedback: The Alternatives section is thin — it defers to a PR discussion rather than summarizing the trade-offs inline, which makes the document less self-contained. The three open questions (status-level references, forward trigger retention, CEL filter communication) should be resolved before implementation begins to avoid mid-stream design pivots. Consider explicitly listing which OSAC services (BMaaS, CaaS, VMaaS) are affected in the Summary for quicker reader orientation. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-121Score: 7/8 | Verdict: PASS
Verdict: A thorough, well-architected design with deep implementation detail and concrete test plans, held back slightly by a missing inline alternatives analysis and thin documentation dimension coverage. Feedback: The Alternatives section needs at least one real alternative evaluated inline with trade-offs — deferring to 'PRD PR #113 discussion' forces reviewers to leave the document to understand why alternatives were rejected. Present the URI/ARN approach and generic reference message briefly with rejection rationale. Additionally, address the Documentation dimension from osac-dimensions.md: state whether API reference docs in fulfillment-service and osac-docs architecture guides will be updated, or explicitly defer documentation to a later milestone. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
| string id = 1; | ||
| string tenant = 2; | ||
| string project = 3; | ||
| string name = 4; | ||
| bool shared = 5; |
There was a problem hiding this comment.
nit: I think the field order matters and maybe should match the public order with any additions being after?
| string id = 1; | |
| string tenant = 2; | |
| string project = 3; | |
| string name = 4; | |
| bool shared = 5; | |
| string id = 1; | |
| string name = 2; | |
| string project = 3; | |
| bool shared = 4; | |
| string tenant = 5; |
There was a problem hiding this comment.
Updated — private field order now matches public with tenant appended at the end:
string id = 1;
string name = 2;
string project = 3;
bool shared = 4;
string tenant = 5;
- Private full reference field order now matches public with tenant appended: id=1, name=2, project=3, shared=4, tenant=5 - Remove cross-tenant information disclosure paragraph (no longer relevant since public API has no tenant field) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
AI Design Review: EP-121Score: 7/8 | Verdict: PASS
Verdict: A thorough and well-architected design that scores 7/8, held back only by a thin Alternatives section that defers rationale to external discussion rather than presenting trade-offs inline. Feedback: The Alternatives section needs real content: pick at least the URI/ARN approach and the generic-reference-message approach, describe each in 3-5 sentences, and explain why per-type messages were chosen over them — don't send the reader to a PR discussion. Consider explicitly declaring which services (BMaaS, CaaS, VMaaS) are in scope and mapping the delivery chunks to the cross-cutting dimensions from osac-dimensions.md. The three Open Questions (status-level references, forward trigger retention, CEL filter migration) should each have a stated resolution timeline or decision owner to avoid blocking implementation. Critical (0)None. Important (2)
Suggestions (4)
Review costModel: claude-opus-4-6 |
…ontext Local references now resolve tenant/project from the owning resource's metadata rather than the caller's auth context. This ensures correct scoping when a Cloud Provider Admin creates resources in a different tenant/project via the private API. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
AI Design Review: EP-121Score: 7/8 | Verdict: PASS
Verdict: A thorough, well-structured design with deep technical detail across architecture, feasibility, and testability, held back from a perfect score only by an empty Alternatives section that defers trade-off analysis to an external PR discussion. Feedback: The Alternatives section must document at least one real alternative with rationale within the design itself — naming URI/ARN, generic reference, and do-nothing options but deferring all analysis to 'PRD PR #113 discussion' makes the design not self-contained. Even a brief paragraph per alternative explaining why it was rejected would satisfy the requirement. Additionally, Open Question 2 (whether to keep forward triggers for FOR SHARE locking or move locking into the interceptor) has direct data consistency implications — the design should state the author's recommended option with reasoning, even if the final decision is deferred to implementation. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, htayrie-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
PRE_COMMIT_PR_BASE_SHA is a snapshot from the pull_request webhook payload, captured at the PR's last open/synchronize event. It doesn't advance as main gains new commits, and re-running an old CI job replays that same stale payload rather than refreshing it. This caused a false-positive class of failure: a long-lived PR that hasn't been pushed to since some other, unrelated PR merged a still-non-compliant enhancements/ directory into main would fail check-ep-naming on that unrelated directory, even though the PR never touches it and it's already correctly grandfathered on main itself. Observed concretely on PR osac-project#121, which failed on enhancements/storage-control-plane-osac-2872 (merged by PR osac-project#134) despite never touching that path. Fix: grandfathering now also checks the live tip of the base branch (PRE_COMMIT_LIVE_BASE_REF, e.g. origin/main, fetched fresh at the start of every CI run) in addition to the stale base SHA — a path is grandfathered if it exists at either reference. This keeps enforcement scoped to genuinely new paths, so contributors actively fixing their own directory's naming are never blocked by an unrelated pre-existing violation elsewhere in the repo. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
Full-repo sweep across all 41 retired directory names from the entire OSAC-2870 effort (not just this PR's 5) turned up two more stale references that earlier passes missed: - OSAC-1330-type-safe-resource-references/design.md still linked to '/enhancements/networking' (renamed to OSAC-356-networking in osac-project#144). This directory was self-renamed by osac-project#121's own branch, so it never went through our cross-reference sweep. - OSAC-985-metering-and-usage-tracking/design.md still linked to '/enhancements/vm-instance-types' (renamed to OSAC-46-vm-instance-types in osac-project#144). metering-and-usage-tracking was one of the directories deferred at that time due to an open PR, so it was excluded from that pass's cross-reference sweep and the reference went stale once the deferred rename landed in osac-project#149. No open PRs conflict with either file (re-verified). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
…ure keys Renames caas-cluster-storage, cluster-version-api, disk-image-OSAC-2540, and secret-management to the OSAC-2836 naming convention (enhancements/OSAC-NNNN-slug/), using each EP's confirmed Jira Feature key. Also lowercases cluster-version-api's DESIGN.md to design.md, and corrects caas-cluster-storage/design.md's stale tracking-link (was pointing at the parent Epic OSAC-1123 instead of the Feature OSAC-1332 that prd.md already cited). cluster-and-vm-provisioning-wizard, metering-and-usage-tracking, and type-safe-resource-references are deferred to a fast-follow PR pending enhancement-proposals#108, osac-project#131, and osac-project#121. Signed-off-by: Tommy Hughes <tohughes@redhat.com>
PRE_COMMIT_PR_BASE_SHA is a snapshot from the pull_request webhook payload, captured at the PR's last open/synchronize event. It doesn't advance as main gains new commits, and re-running an old CI job replays that same stale payload rather than refreshing it. This caused a false-positive class of failure: a long-lived PR that hasn't been pushed to since some other, unrelated PR merged a still-non-compliant enhancements/ directory into main would fail check-ep-naming on that unrelated directory, even though the PR never touches it and it's already correctly grandfathered on main itself. Observed concretely on PR osac-project#121, which failed on enhancements/storage-control-plane-osac-2872 (merged by PR osac-project#134) despite never touching that path. Fix: grandfathering now also checks the live tip of the base branch (PRE_COMMIT_LIVE_BASE_REF, e.g. origin/main, fetched fresh at the start of every CI run) in addition to the stale base SHA — a path is grandfathered if it exists at either reference. This keeps enforcement scoped to genuinely new paths, so contributors actively fixing their own directory's naming are never blocked by an unrelated pre-existing violation elsewhere in the repo. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
Full-repo sweep across all 41 retired directory names from the entire OSAC-2870 effort (not just this PR's 5) turned up two more stale references that earlier passes missed: - OSAC-1330-type-safe-resource-references/design.md still linked to '/enhancements/networking' (renamed to OSAC-356-networking in osac-project#144). This directory was self-renamed by osac-project#121's own branch, so it never went through our cross-reference sweep. - OSAC-985-metering-and-usage-tracking/design.md still linked to '/enhancements/vm-instance-types' (renamed to OSAC-46-vm-instance-types in osac-project#144). metering-and-usage-tracking was one of the directories deferred at that time due to an open PR, so it was excluded from that pass's cross-reference sweep and the reference went stale once the deferred rename landed in osac-project#149. No open PRs conflict with either file (re-verified). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
|
/retitle OSAC-2766: Design - Type-Safe Resource References |
openshift-ci
Bot
changed the title
7 participants
Design: Type-Safe Resource References
Jira: https://redhat.atlassian.net/browse/OSAC-1330
PRD: enhancements/type-safe-resource-references/prd.md (PR #113, merged)
Summary
This design replaces all 34 opaque
stringreference fields across 15 public API resources with per-type structured protobuf messages (<Type>Referencefor cross-tenant and<Type>LocalReferencefor same-tenant references). A centralized gRPC interceptor using protoreflect validates references before server handlers run, replacing scattered inline validation and standardizing error reporting onInvalidArgumentwith structured field paths. The interceptor fails closed — unregistered reference types cause a startup failure and runtimeInternalerror, not a silent pass-through. Delivery is incremental across 5 resource-group chunks (Networking → Compute → IP Management → Clusters+BareMetal → IAM), each updating all layers (proto, server, DB triggers, CLI, UI) atomically.Requesting Review On
FOR SHARElocking moved into the interceptor's DAO lookups? The current triggers useSELECT ... FOR SHAREto serialize concurrent child inserts and parent deletes underREAD COMMITTED. To be resolved during Epic 1 implementation.How to Review
Summary by CodeRabbit