fix(openapi): schema name collision hardening - #786
Merged
albcui merged 9 commits intoJul 20, 2026
Conversation
Contributor
|
albcui
force-pushed
the
aalgo-352-openapi-schema-collision-hardening/albcui
branch
from
July 20, 2026 19:49
6cf17d6 to
e3246bd
Compare
albcui
marked this pull request as ready for review
July 20, 2026 19:49
Contributor
|
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:
📝 WalkthroughWalkthroughThe change aligns shared entity delete and sort contracts across services, OpenAPI specifications, and CLI documentation. It also makes OpenAPI schema-collision handling fail by default and updates related generation logic and tests. ChangesEntity API contract alignment
Strict OpenAPI collision handling
Sequence Diagram(s)sequenceDiagram
participant process_plugin_specs
participant apply_schema_fixes
participant tweak_spec
participant _normalize_refs_and_schema_keys
process_plugin_specs->>apply_schema_fixes: process all plugin specs
apply_schema_fixes->>tweak_spec: tweak specification
tweak_spec->_normalize_refs_and_schema_keys: normalize schema names
_normalize_refs_and_schema_keys-->>tweak_spec: raise ValueError on differing collision
Possibly related PRs
Suggested reviewers: 🚥 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}
Contributor
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 `@docs/cli/reference.mdx`:
- Line 1163: Regenerate the CLI reference documentation using the make target
generate-cli-reference-docs, rather than editing the sort option entries
manually, and commit all resulting generated changes across the affected
occurrences.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9ddbc0c6-692e-454d-b6fc-2b6af207007f
⛔ Files ignored due to path filters (3)
sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/files/filesets.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/guardrail/configs.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/inference/prompts.pyis excluded by!sdk/**
📒 Files selected for processing (4)
docs/cli/reference.mdxpackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/files/filesets.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/guardrail/configs.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/inference/prompts.py
JashG
approved these changes
Jul 20, 2026
mckornfield
approved these changes
Jul 20, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
services/core/entities/src/nmp/core/entities/api/v2/workspaces/endpoints.py (1)
458-461: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
DeleteResponse.idsemantics differ from the IAM role-binding delete.Here
idis the resource's natural key (name/principal_id), matching the URL path parameter. Iniam/endpoints.py,revoke_role_bindinginstead returnsobj.id(the entity's surrogate UUID), not the pathname. Same sharedDeleteResponse.idfield, two different semantics across services — confusing for SDK consumers expecting one meaning.Also applies to: 921-924
🤖 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 `@services/core/entities/src/nmp/core/entities/api/v2/workspaces/endpoints.py` around lines 458 - 461, Update the workspace deletion handler’s DeleteResponse construction to return the deleted workspace entity’s surrogate UUID in id, matching revoke_role_binding, instead of the path-based name. Preserve the existing deletion message and response structure.services/core/auth/src/nmp/core/auth/api/v2/iam/endpoints.py (1)
248-248: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winInconsistent
idsemantics vs. otherDeleteResponseproducers — see consolidated comment.🤖 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 `@services/core/auth/src/nmp/core/auth/api/v2/iam/endpoints.py` at line 248, Update the DeleteResponse construction in this endpoint to match the established id semantics used by the other DeleteResponse producers, rather than unconditionally returning obj.id. Preserve the existing deletion flow and align the response field with the consolidated convention.
🤖 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 `@services/core/auth/src/nmp/core/auth/api/v2/iam/endpoints.py`:
- Line 248: Update the DeleteResponse construction in this endpoint to match the
established id semantics used by the other DeleteResponse producers, rather than
unconditionally returning obj.id. Preserve the existing deletion flow and align
the response field with the consolidated convention.
In `@services/core/entities/src/nmp/core/entities/api/v2/workspaces/endpoints.py`:
- Around line 458-461: Update the workspace deletion handler’s DeleteResponse
construction to return the deleted workspace entity’s surrogate UUID in id,
matching revoke_role_binding, instead of the path-based name. Preserve the
existing deletion message and response structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a89ef811-639d-4666-87f6-c7c52c913885
⛔ Files ignored due to path filters (7)
sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**sdk/python/nemo-platform/.nmpcontext/stainless.yamlis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/projects/api.mdis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/projects/projects.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/shared/delete_response.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/test_projects.pyis excluded by!sdk/**sdk/stainless.yamlis excluded by!sdk/**
📒 Files selected for processing (13)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlpackages/nmp_common/src/nmp/common/api/common.pyservices/core/auth/src/nmp/core/auth/api/v2/iam/endpoints.pyservices/core/auth/tests/integration/test_iam_role_bindings.pyservices/core/entities/src/nmp/core/entities/api/v2/entities/endpoints.pyservices/core/entities/src/nmp/core/entities/api/v2/projects/endpoints.pyservices/core/entities/src/nmp/core/entities/api/v2/schemas.pyservices/core/entities/src/nmp/core/entities/api/v2/workspaces/endpoints.pyservices/core/entities/tests/integration/test_generic_entities.pyservices/core/entities/tests/integration/test_projects.pyservices/core/entities/tests/integration/test_workspace_members.py
💤 Files with no reviewable changes (3)
- services/core/auth/tests/integration/test_iam_role_bindings.py
- services/core/entities/src/nmp/core/entities/api/v2/schemas.py
- services/core/entities/tests/integration/test_workspace_members.py
#737 fixed the Automodel/Unsloth backends, but two collisions remained across the merged platform services and were silently collapsed by the schema-name normalizer: - DeleteResponse: core-entities' model (required id + deleted_count) collided with the shared nmp.common.api.common.DeleteResponse (id, deleted_at); guardrails had redefined the shared shape locally. - GenericSortField: core-entities, guardrails, and common each defined a different enum under the same name. The merge kept only the first-seen schema and repointed every $ref to it, so delete/sort endpoints across services referenced the wrong contract in the generated SDK. Give each its true name (schema-name change only; JSON wire format unchanged): - guardrails: drop the local DeleteResponse, import the byte-identical shared nmp.common.api.common.DeleteResponse. - core-entities: DeleteResponse -> EntityDeleteResponse (keeps deleted_count), updating all four delete endpoints. - core-entities GenericSortField -> WorkspaceSortField; guardrails GenericSortField -> GuardrailConfigSortField. Regenerates openapi/openapi.yaml and the ga/ merged specs. Refs AALGO-352. Signed-off-by: Albert Cui <albcui@nvidia.com>
The differing-content collision gate added in #737 was opt-in: only the nemo-customizer plugin spec enforced it, while platform/service specs stayed on warn-and-collapse. That is exactly how the AALGO-352 collisions shipped silently. With those fixed (zero collisions across the platform, all services, and all plugins), flip the default so any future collision fails spec generation loudly instead of shipping a wrong SDK contract: - tweak_spec, apply_schema_fixes, and PluginConfig.strict_schema_collisions default to True. The plugin "lenient" branch now passes strict_collisions=False explicitly (it previously relied on the old default). - A spec can still opt out via strict_collisions=False / [tool.nemo.openapi].strict_schema_collisions = false. - Generalize the collision error message (rename/dedupe or namespace) and invert the unit tests to pin the policy: default raises, opt-out warns. Enforcement is at spec-generation time (make refresh-openapi / the manual openapi-generator pre-commit hook). CI does not regenerate the spec today, so this does not yet gate PRs on its own; adding a CI regen/drift check is a follow-up. Refs AALGO-352. Signed-off-by: Albert Cui <albcui@nvidia.com>
Regenerate the vendored Python SDK (`make update-sdk`) to pick up the schema renames from the AALGO-352 fix. Adds EntityDeleteResponse, WorkspaceSortField, and GuardrailConfigSortField: the entities/projects/workspaces delete endpoints now return EntityDeleteResponse, and the workspaces / guardrail-config list sort params reference the new enums. The shared DeleteResponse / GenericSortField are unchanged (still used by iam role-bindings, files, etc.). The Stainless mapping report was warning-free; the three new models auto-mapped correctly (entity_delete_response under $shared since it is returned by three resources; the two sort fields under their own resources). Also hand-update packages/nemo_platform_plugin/entities.py: its delete() / delete_by_id() wrapper methods forward AsyncEntitiesResource.delete_entity_by_name, whose return type is now EntityDeleteResponse, so retype those annotations. Regen does not touch this hand-written file. (Pre-existing `parent: str | Omit` ty diagnostics in that file are unrelated and left as-is.) Refs AALGO-352. Signed-off-by: Albert Cui <albcui@nvidia.com>
Guardrails' config list defined its own GuardrailConfigSortField (created_at only) in the legacy nmp.guardrails.app.common.common. GuardrailConfig is entity-store-backed, so it can sort by any entity base field; point it at the shared nmp.common.api.common.GenericSortField and retire the local enum. Widen GenericSortField with updated_at — both guardrail configs and filesets are entity-backed and the store already supports the column — and convert it from (str, Enum) to StrEnum. A bare (str, Enum) member stringifies to "GenericSortField.CREATED_AT_ASC" rather than "created_at", so passing the member straight to the entity client produced a malformed sort and broke the list request; StrEnum makes members stringify to their value. Guardrails now passes sort.value, matching the filesets endpoint. Regenerate the OpenAPI spec and Python SDK for the removed GuardrailConfigSortField and widened GenericSortField. Signed-off-by: Albert Cui <albcui@nvidia.com>
Workspaces and projects each defined their own sort enum (WorkspaceSortField, ProjectSortField) duplicating the shared nmp.common.api.common.GenericSortField. Both are entity-store-backed and sort via getattr over real base columns, so point them at the shared enum and delete the bespoke ones. ProjectSortField was already identical (created_at, updated_at, name); workspaces additively gains name-sorting (DBWorkspace.name is a real, unique column). This also retires ProjectSortField's stale (str, Enum) base in favor of the shared StrEnum. Regenerate the OpenAPI spec, Python SDK, and CLI for the removed schemas and the widened workspace sort options. Signed-off-by: Albert Cui <albcui@nvidia.com>
Entity/project/workspace delete endpoints returned EntityDeleteResponse
(message, id, deleted_count) while guardrails and IAM used the shared
nmp.common.api.common.DeleteResponse (message, id, deleted_at) — the name
collision the hardening pass resolved by keeping two models. Both extra
fields turned out to be write-only (read only by integration tests; no web
or SDK consumer), and deleted_count is ~always 1 (delete_entity_by_name
returns a hardcoded 1; the handler 404s on 0). So collapse to a single
DeleteResponse{message, id} with id required, and point all six delete
endpoints (entities, projects, workspaces, members, guardrails, IAM) plus
the plugin EntityClient at it. Drop EntityDeleteResponse, deleted_count,
and deleted_at.
Regenerate the OpenAPI spec and Python SDK; update the integration asserts
that checked the removed fields.
Signed-off-by: Albert Cui <albcui@nvidia.com>
The per-plugin strict_schema_collisions opt-out (warn-and-collapse) had no users — every collision was fixed and no spec opts out — and it can never be correct: collapsing two differing-content models into one name silently repoints the other's $refs, shipping a wrong contract in the generated SDK. It was migration scaffolding for the lenient->strict flip, now complete. Remove it entirely: drop PluginConfig.strict_schema_collisions and the strict_collisions parameter threaded through tweak_spec / _normalize_refs_and_schema_keys / apply_schema_fixes, collapse the plugin strict/lenient split to a single pass, and delete the customizer's now- redundant pin plus the warn-and-collapse test. Collisions now always raise. No schema or SDK change: the generated spec is byte-identical. Signed-off-by: Albert Cui <albcui@nvidia.com>
albcui
force-pushed
the
aalgo-352-openapi-schema-collision-hardening/albcui
branch
from
July 20, 2026 22:23
0fbeb06 to
a1a2fe8
Compare
albcui
enabled auto-merge
July 20, 2026 22:23
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
$refnamespace AND the actual schema differs, we accept the first one and throw away the rest. The result is an incorrect OpenAPI spec post merge. This PR hardens the OpenAPI spec processing to strictly error when collisions are detected. This revealed the following issues, which are fixed as part of this PR.DeleteResponseandGenericSortFieldhad collisions between Guardrails and Core Entities services.DeleteResponseandGenericSortField, and re-use the ones innmp-commonGenericSortFieldfromnmp-common. Re-namedDeleteResponsetoEntityDeleteResponsefor now because it has an extradeleted_countfield. However, ideally, we can leveragenmp-common'sDeleteResponseas well.Summary by CodeRabbit
New Features
updated_at/-updated_atsorting to multiple list commands (including workspaces, filesets, guardrail configs, and related resources).id(removingdeleted_at).Documentation
--sortoptions and the updated delete response schema.Bug Fixes