fix(api): declare the entity-store name pattern on the remaining models DTOs [ASTD-420] - #921
Merged
Merged
Conversation
Contributor
|
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 ignored due to path filters (23)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe pull request standardizes entity-name validation across OpenAPI specifications, Pydantic request models, and CLI guidance. Names now use a 63-character lowercase pattern. Tests cover accepted names, rejected names, and generated schema metadata. ChangesName validation contract
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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
🧹 Nitpick comments (1)
packages/nemo_platform_plugin/tests/files/test_types.py (1)
13-13: 🚀 Performance & Scalability | 🔵 TrivialNo test guards parity across the 3 inlined
NAME_PATTERNcopies.
files/types.py,secrets/types.py, andnmp_common.entities.constantseach define their own copy of the same pattern/description/max-length. A quick test asserting all three stay identical would catch future drift cheaply.🤖 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 `@packages/nemo_platform_plugin/tests/files/test_types.py` at line 13, In the tests near the existing imports in test_types.py, add a focused parity test comparing NAME_PATTERN, its associated description, and NAME_MAX_LENGTH from files/types.py, secrets/types.py, and nmp_common.entities.constants. Import the corresponding symbols from each module and assert all three definitions remain identical.
🤖 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 `@packages/nemo_platform_plugin/src/nemo_platform_plugin/files/types.py`:
- Around line 77-97: Update NAME_PATTERN_DESCRIPTION in
packages/nemo_platform_plugin/src/nemo_platform_plugin/files/types.py:77-97 to
document that names may also contain @, ., +, and _; make the same description
change to _NAME_DESCRIPTION in
packages/nemo_platform_plugin/src/nemo_platform_plugin/secrets/types.py:18-39 so
both descriptions accurately match the shared regex character class.
---
Nitpick comments:
In `@packages/nemo_platform_plugin/tests/files/test_types.py`:
- Line 13: In the tests near the existing imports in test_types.py, add a
focused parity test comparing NAME_PATTERN, its associated description, and
NAME_MAX_LENGTH from files/types.py, secrets/types.py, and
nmp_common.entities.constants. Import the corresponding symbols from each module
and assert all three definitions remain identical.
🪄 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: d1849205-8af8-487b-8892-1c7179f410c5
📒 Files selected for processing (11)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlpackages/nemo_platform_plugin/src/nemo_platform_plugin/files/types.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/secrets/types.pypackages/nemo_platform_plugin/tests/files/test_types.pypackages/nmp_common/src/nmp/common/entities/constants.pyservices/core/models/src/nmp/core/models/schemas.pyservices/core/models/tests/unit/test_schemas_entity_names.pyservices/core/secrets/tests/integration/test_secrets_with_auth.pyservices/core/secrets/tests/test_secrets.py
Contributor
|
marcusds
force-pushed
the
entity-name-pattern-spec/mschwab
branch
from
July 28, 2026 20:15
ccf0286 to
5055ddb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
openapi/openapi.yaml (1)
9344-9350: 🗄️ Data Integrity & Integration | 🔵 TrivialBreaking change: maxLength reduced 255→63 on 8 create endpoints.
oasdiff flags these as breaking (
request-property-max-length-decreased). Intentional per PR, but SDK/web-SDK regeneration is deferred, so generated clients won't enforce the new limit client-side until regenerated — surface this in release notes / API changelog.Also applies to: 9401-9406, 9456-9462, 9500-9505, 9551-9556, 9586-9591, 9676-9681, 9808-9813
🤖 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 `@openapi/openapi.yaml` around lines 9344 - 9350, Document the intentional maxLength reduction from 255 to 63 for the adapter name schema across all eight create endpoints in the release notes or API changelog. Clearly identify this as a breaking API change and note that generated SDK and web-SDK clients will not enforce the new limit until regeneration.Source: Linters/SAST tools
🤖 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 `@openapi/ga/openapi.yaml`:
- Around line 13975-13981: Update the duplicated Name schema definitions at all
referenced locations so the pattern matches the description: allow only
lowercase letters, digits, and hyphens, while preserving the lowercase-start,
2–63 character, no-consecutive-hyphens, and no-trailing-hyphen constraints.
Remove the unsupported @, ., +, and _ characters from each pattern.
In `@openapi/openapi.yaml`:
- Around line 9344-9350: Update the descriptions for all affected name schema
entries to state that names may contain lowercase letters, digits, hyphens,
periods, plus signs, underscores, and @ symbols, while retaining the existing
length, starting-letter, consecutive-hyphen, and trailing-hyphen constraints.
Apply the wording consistently to each listed occurrence without changing the
regex patterns.
- Around line 13975-13981: Relax the inherited name validation for the read DTOs
ModelDeployment, ModelDeploymentConfig, ModelEntity, ModelProvider, and Prompt
so GET/list responses can deserialize legacy uppercase or longer names. Keep the
strict ModelEntityBaseModel.name constraints for write/request validation,
separating read and write schemas where necessary without changing write
behavior.
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/secrets/types.py`:
- Around line 32-36: Update _NAME_DESCRIPTION in
packages/nemo_platform_plugin/src/nemo_platform_plugin/secrets/types.py:32-36 to
state that ., +, _, and @ are permitted, matching _NAME_REGEX. Regenerate the
PlatformSecretCreateRequest.name description in openapi/openapi.yaml:16464-16469
from the updated constant.
---
Nitpick comments:
In `@openapi/openapi.yaml`:
- Around line 9344-9350: Document the intentional maxLength reduction from 255
to 63 for the adapter name schema across all eight create endpoints in the
release notes or API changelog. Clearly identify this as a breaking API change
and note that generated SDK and web-SDK clients will not enforce the new limit
until regeneration.
🪄 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: 71150676-3084-497a-8522-8dfc1d35814f
📒 Files selected for processing (11)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlpackages/nemo_platform_plugin/src/nemo_platform_plugin/files/types.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/secrets/types.pypackages/nemo_platform_plugin/tests/files/test_types.pypackages/nmp_common/src/nmp/common/entities/constants.pyservices/core/models/src/nmp/core/models/schemas.pyservices/core/models/tests/unit/test_schemas_entity_names.pyservices/core/secrets/tests/integration/test_secrets_with_auth.pyservices/core/secrets/tests/test_secrets.py
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/nmp_common/src/nmp/common/entities/constants.py
- services/core/secrets/tests/test_secrets.py
- packages/nemo_platform_plugin/tests/files/test_types.py
- services/core/models/tests/unit/test_schemas_entity_names.py
- services/core/models/src/nmp/core/models/schemas.py
- services/core/secrets/tests/integration/test_secrets_with_auth.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/files/types.py
This was referenced Jul 29, 2026
…ls DTOs #978 tightened three DTOs — `CreateModelProviderRequest`, `CreateFilesetRequest`, `PlatformSecretCreateRequest` — and collapsed NAME_PATTERN into a single definition in `nemo_platform_plugin/entity_naming.py`. The rest of the models service was left on the loose rule. `CreatePromptRequest`, `CreateModelEntityRequest`, `CreateModelAdapterRequest`, `CreateModelDeploymentConfigRequest`, `CreateModelDeploymentRequest`, and the shared `ModelEntityBaseModel` still advertise `^[\w\-.]+$` with max_length 255, while the entity store enforces `^[a-z](?!.*--)[a-z0-9\-@.+_]{1,62}(?<!-)$` with max 63. So `Sparl` passes request validation, passes any generated client's validation, and then 422s from the entity store with `loc: ['body', 'name']` pointing at a body the caller never sent. Declare NAME_PATTERN and NAME_MAX_LENGTH on those six. Read and lookup DTOs keep the looser pattern so entities created before the rule stay reachable. NAME_PATTERN uses lookaround, which pydantic's default rust-regex engine cannot compile, so the affected models need `regex_engine="python-re"`. #978 set that inline on the one DTO it touched; this lifts it to a module-level ENTITY_NAME_CONFIG rather than repeating the ConfigDict seven times. Not a compatibility break: any name that passes the loose pattern but fails the strict one already 422s at the entity store today. This moves the rejection to the edge, where the error can name the actual field. Regenerates openapi/openapi.yaml and the two GA variants — the diff is exclusively pattern, maxLength, and description. Plugin specs regenerate with no diff. Signed-off-by: mschwab <mschwab@nvidia.com>
marcusds
force-pushed
the
entity-name-pattern-spec/mschwab
branch
from
August 12, 2026 16:31
5055ddb to
c1182f7
Compare
Contributor
|
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. |
marcusds
changed the title
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 `@openapi/ga/individual/platform.openapi.yaml`:
- Around line 14248-14254: The response schemas for ModelProvider, Prompt,
ModelEntity, ModelDeploymentConfig, and ModelDeployment should stop inheriting
the strict ModelEntityBaseModel name validation; introduce or reuse a permissive
response base model for read conversions from entity.name or entity.base_name,
while retaining the existing strict constraints on create request schemas.
🪄 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: 92ad2d24-c137-4869-b3f4-8a8d34fea184
📒 Files selected for processing (6)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlpackages/nemo_platform_plugin/tests/files/test_types.pyservices/core/models/src/nmp/core/models/schemas.pyservices/core/models/tests/unit/test_schemas_entity_names.py
🚧 Files skipped from review as they are similar to previous changes (3)
- services/core/models/tests/unit/test_schemas_entity_names.py
- packages/nemo_platform_plugin/tests/files/test_types.py
- services/core/models/src/nmp/core/models/schemas.py
The name pattern and 63-character limit declared on the models DTOs had not been propagated to the generated Python SDK, so lint-python-sdk failed with the OpenAPI spec out of sync. Signed-off-by: mschwab <mschwab@nvidia.com>
marcusds
changed the title
Signed-off-by: mschwab <mschwab@nvidia.com>
Contributor
maxdubrinsky
approved these changes
Aug 12, 2026
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
2 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.
Problem
The entity store enforces
^[a-z](?!.*--)[a-z0-9\-@.+_]{1,62}(?<!-)$, max 63. #978 declared that on three DTOs —CreateModelProviderRequest,CreateFilesetRequest,PlatformSecretCreateRequest— and collapsed the constant intonemo_platform_plugin/entity_naming.py. The rest of the models service is still on the loose rule:mainCreatePromptRequest^[\w\-.]+$, max 255CreateModelEntityRequest^[\w\-.]+$, max 255CreateModelAdapterRequest^[\w\-.]+$, max 255CreateModelDeploymentConfigRequest^[\w\-.]+$, max 255CreateModelDeploymentRequest^[\w\-.]+$, max 255ModelEntityBaseModel^[\w\-.]+$, max 255So
SparlorMy_Promptvalidates against the published spec, passes any generated client's validation, and then 422s from the entity store withloc: ['body', 'name']— pointing at a body the caller never sent.Change
Declare
NAME_PATTERN+NAME_MAX_LENGTHon the six above. Read/lookup DTOs (Get*,List*,Delete*) keep the loose pattern so entities created before the rule stay reachable.NAME_PATTERNuses lookaround, which pydantic's default rust-regex engine cannot compile, so affected models needregex_engine="python-re". #978 set that inline on the one DTO it touched; this lifts it to a module-levelENTITY_NAME_CONFIGrather than repeating theConfigDictseven times, and switches the provider DTO over to it.Not a compatibility break: any name that passes the loose pattern but fails the strict one already 422s at the entity store today. This moves the rejection to the edge, where the error can name the actual field.
Worth a reviewer's eye
ModelEntityBaseModelis a response base (ModelEntity,ModelDeploymentsubclass it), not a create request. Tightening it means an entity whose stored name predates entity-store enforcement would now fail response validation rather than being returned. The entity store has enforced this rule on the write path for a while, so there should be no such rows — but that assumption is worth confirming before merge. Happy to drop it from this PR and file it separately if you'd rather not carry the risk.Testing
services/core/models/tests/unit— 977 passed, 1 skipped (torch)test_schemas_entity_names.py: 6 DTOs × invalid/valid names, plus asserts the emitted JSON schema carries the pattern and maxLength — 85 passed together withpackages/nemo_platform_plugin/tests/files/test_types.pyruff check,ruff format --checkclean; thetypre-commit hook passesNote: this repo's shared venv installs the packages editable from the main checkout, so tests run from a worktree need
PYTHONPATHpointed at the worktree'ssrcdirs or they silently exercisemain's code.Spec regeneration
openapi/openapi.yamland the two GA variants are regenerated. The diff is exclusivelypattern/maxLength/description— no path or operation changes. Plugin specs regenerate with no diff.Follow-up
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/models.pystill carries a hand-rewritten lookaround-free variant that has drifted (it accepts names up to 125 characters). Out of scope here —nemo_evaluator_sdkhas no dependency onnemo_platform_plugin, so fixing it needs either a new dependency or an equivalence test.Summary by CodeRabbit
Bug Fixes
Documentation