fix(api): declare the entity-store name pattern on create DTOs [ASTD-349] - #978
Merged
Merged
Conversation
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:
📝 WalkthroughWalkthroughName validation is tightened across fileset, model provider, and platform secret requests. Shared constants, Pydantic models, OpenAPI schemas, CLI messages, documentation, and secret API tests now reflect lowercase-starting names, restricted punctuation, and 63-character limits where enforced. ChangesEntity name validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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.
🧹 Nitpick comments (1)
packages/nemo_platform_plugin/src/nemo_platform_plugin/files/types.py (1)
77-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated pattern risks drifting from nmp_common's source of truth.
Comment acknowledges the duplication, but nothing enforces it stays in sync with
nmp.common.entities.constants.NAME_PATTERN.Add a unit test in either package asserting the two pattern strings are identical, so future changes to one side fail loudly.
🤖 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/src/nemo_platform_plugin/files/types.py` around lines 77 - 87, Add a unit test that imports the local NAME_PATTERN and nmp.common.entities.constants.NAME_PATTERN, then asserts their pattern strings are identical. Place the test in either package’s existing unit-test suite and keep the assertion focused on detecting future drift between these two symbols.
🤖 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 `@packages/nemo_platform_plugin/src/nemo_platform_plugin/files/types.py`:
- Around line 77-87: Add a unit test that imports the local NAME_PATTERN and
nmp.common.entities.constants.NAME_PATTERN, then asserts their pattern strings
are identical. Place the test in either package’s existing unit-test suite and
keep the assertion focused on detecting future drift between these two symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 38bac438-c76f-4eab-92a8-dee90ecffd9e
📒 Files selected for processing (9)
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/nmp_common/src/nmp/common/entities/constants.pyservices/core/models/src/nmp/core/models/schemas.pyservices/core/secrets/tests/integration/test_secrets_with_auth.pyservices/core/secrets/tests/test_secrets.py
Contributor
|
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 `@packages/nemo_platform_plugin/src/nemo_platform_plugin/entity_naming.py`:
- Around line 29-33: Update NAME_PATTERN_DESCRIPTION to accurately document the
characters accepted by NAME_PATTERN, including @, ., +, and _, while retaining
the existing length, starting-character, consecutive-hyphen, and trailing-hyphen
rules.
🪄 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: 1d12a3f9-a12d-4f90-b463-eaec65b4a1ee
📒 Files selected for processing (5)
packages/nemo_platform_plugin/src/nemo_platform_plugin/entity_naming.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/files/types.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/jobs/spec.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/secrets/types.pypackages/nmp_common/src/nmp/common/entities/constants.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nemo_platform_plugin/src/nemo_platform_plugin/secrets/types.py
marcusds
added a commit
that referenced
this pull request
Jul 29, 2026
#978 makes the create DTOs declare NAME_PATTERN, so the generated zod now carries the strict pattern and entityName.ts no longer has to hand-mirror it. Take ENTITY_NAME_REGEXP from entitiesCreateEntityBodyNameRegExp — the generic entity-store endpoint, not one of the per-resource copies. The max length stays local because the entity-store schema declares no maxLength; a test pins it against the create DTOs that do. That test also asserts the entity, fileset, secret, and model-provider schemas still agree on the pattern, so a future drift between them fails CI. The per-rule error messages and the sanitizer's character classes have no generated equivalent and remain hand-written. Signed-off-by: mschwab <mschwab@nvidia.com>
marcusds
added a commit
that referenced
this pull request
Jul 29, 2026
…349] NAME_PATTERN_DESCRIPTION claimed names may contain "only lowercase letters, digits, and hyphens", but the pattern has allowed @, ., + and _ since it was written — see the TODO(#3530) directly above it. The spec therefore told SDK consumers a name like llama-3.2-3b@v1.0.0 was invalid when the service accepts it, and contradicted Studio's own help text, which lists the special characters correctly. All nine name fields in the spec now carry wording that matches the nine patterns beside them. Reported by CodeRabbit on #978. Signed-off-by: mschwab <mschwab@nvidia.com>
mckornfield
reviewed
Jul 29, 2026
mckornfield
left a comment
mckornfield
left a comment
Contributor
There was a problem hiding this comment.
feel like we should leave off those special characters if versioning is going to happen, even eventually
Contributor
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 `@docs/cli/reference.mdx`:
- Line 1103: Update the fileset name descriptions at the referenced repeated
locations to document the accepted underscore character as “_”, not “\__”.
Preserve the existing allowed-character list and formatting, escaping the
underscore only as needed for MDX rendering.
- Line 1103: Update the generator source that produces the fileset name
documentation instead of editing the generated reference page directly. Apply
the change at the source corresponding to the entries near the fileset name
references, then run the designated reference-page regeneration command so all
affected occurrences, including the additional referenced entries, are
regenerated consistently.
🪄 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: 27331ff6-2de2-4d20-bd9d-65b59f9fcfb1
⛔ Files ignored due to path filters (21)
sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**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/inference/providers.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/projects.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/workspaces/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/entities/entities.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/files/filesets.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/inference/providers.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/inference/virtual_models.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/projects/projects.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/secrets/secrets.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/workspaces/workspaces.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/entities/entity_create_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/entities/entity_update_entity_by_name_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/files/fileset_create_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/inference/provider_create_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/jobs/platform_job_step_spec.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/jobs/platform_job_step_spec_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/projects/project_create_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/secrets/secret_create_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/workspaces/workspace_create_params.pyis excluded by!sdk/**
📒 Files selected for processing (5)
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/inference/providers.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/projects.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/workspaces/__init__.py
marcusds
added a commit
that referenced
this pull request
Jul 30, 2026
…349] NAME_PATTERN_DESCRIPTION claimed names may contain "only lowercase letters, digits, and hyphens", but the pattern has allowed @, ., + and _ since it was written — see the TODO(#3530) directly above it. The spec therefore told SDK consumers a name like llama-3.2-3b@v1.0.0 was invalid when the service accepts it, and contradicted Studio's own help text, which lists the special characters correctly. All nine name fields in the spec now carry wording that matches the nine patterns beside them. Reported by CodeRabbit on #978. Signed-off-by: mschwab <mschwab@nvidia.com>
marcusds
force-pushed
the
astd-349-entity-name-pattern-dtos/mschwab
branch
from
July 30, 2026 08:00
f87f793 to
cc399c9
Compare
Contributor
marcusds
added a commit
that referenced
this pull request
Jul 30, 2026
#978 makes the create DTOs declare NAME_PATTERN, so the generated zod now carries the strict pattern and entityName.ts no longer has to hand-mirror it. Take ENTITY_NAME_REGEXP from entitiesCreateEntityBodyNameRegExp — the generic entity-store endpoint, not one of the per-resource copies. The max length stays local because the entity-store schema declares no maxLength; a test pins it against the create DTOs that do. That test also asserts the entity, fileset, secret, and model-provider schemas still agree on the pattern, so a future drift between them fails CI. The per-rule error messages and the sanitizer's character classes have no generated equivalent and remain hand-written. Signed-off-by: mschwab <mschwab@nvidia.com>
…349] The spec advertised `^[\w\-.]+$` (max 255) for fileset, model provider, and secret names while the entity store enforces the stricter RFC-1035-ish NAME_PATTERN downstream. Names like `Sparl` or `My_Provider` passed every generated client's validation and then failed with a 422, so the published contract was wrong for all SDK consumers, not just Studio. Declare NAME_PATTERN on the three create-request DTOs. Each needs regex_engine="python-re" — the pattern uses lookaround, which Pydantic's default Rust engine rejects — matching what the entity-store schemas already do. max_length drops 255 -> 63 to agree with the regex, which caps at 63 by construction. The secrets DTO enforced its rule in a field_validator, so no pattern reached the spec at all; moving it to `pattern=` publishes it. Its uppercase-name regression tests now bypass the request model with model_construct so they still exercise the server-side 422 path. `@` is legal under NAME_PATTERN, so secrets now accept `a@b`. The entity store always did. Signed-off-by: mschwab <mschwab@nvidia.com>
NAME_PATTERN was copy-pasted verbatim in four Python modules — nmp_common, plus files/types.py, secrets/types.py, and jobs/spec.py inside nemo_platform_plugin. Each carried a comment explaining it was inlined to avoid an nmp_common dependency, and jobs/spec.py cited files/types.py as the precedent, so the duplication was self-propagating. The dependency only runs one way: nmp_common depends on nemo-platform-plugin, so the plugin cannot import nmp_common without a cycle. That makes the plugin the correct home. New leaf module entity_naming.py holds the single definition and imports nothing, so modules that need to stay leaf nodes still can. nmp_common re-exports it, leaving every existing constants.NAME_PATTERN call site untouched. Pure refactor: regenerating the OpenAPI spec produces no diff. Signed-off-by: mschwab <mschwab@nvidia.com>
Signed-off-by: mschwab <mschwab@nvidia.com>
…349] NAME_PATTERN_DESCRIPTION claimed names may contain "only lowercase letters, digits, and hyphens", but the pattern has allowed @, ., + and _ since it was written — see the TODO(#3530) directly above it. The spec therefore told SDK consumers a name like llama-3.2-3b@v1.0.0 was invalid when the service accepts it, and contradicted Studio's own help text, which lists the special characters correctly. All nine name fields in the spec now carry wording that matches the nine patterns beside them. Reported by CodeRabbit on #978. Signed-off-by: mschwab <mschwab@nvidia.com>
Picks up the NAME_PATTERN_DESCRIPTION change across the vendored Python SDK, the generated CLI commands, and the CLI reference docs. plugins/nemo-customizer/openapi/openapi.yaml is deliberately excluded: regenerating it locally drops the whole rl/jobs surface, which comes from an optional dependency that isn't installed here. Signed-off-by: mschwab <mschwab@nvidia.com>
Two problems with the previous wording, "contain only lowercase letters, digits, hyphens, and @ . + _". The trailing underscore is markdown-escaped downstream, so the CLI reference rendered "@ . + \__". Dropping the literal characters removes the artifact. Second, @, + and _ are slated for removal (TODO(#3530)), and advertising them invites adoption ahead of that migration — mckornfield's point on this PR. Dots are not in the same category: llama-3.1-8b is the example in CreateModelProviderRequest and dotted versions are how model names are written, so those stay documented. "use" rather than "contain only" keeps this from being a false claim about what the service rejects; the machine-readable pattern remains the contract. Signed-off-by: mschwab <mschwab@nvidia.com>
…-349] Propagates the NAME_PATTERN_DESCRIPTION rewording through the vendored Python SDK, the generated CLI commands, and the CLI reference docs. Also clears the "@ . + \__" escaping artifact the previous wording left in docs/cli/reference.mdx. Signed-off-by: mschwab <mschwab@nvidia.com>
marcusds
force-pushed
the
astd-349-entity-name-pattern-dtos/mschwab
branch
from
July 30, 2026 16:37
2df2443 to
1f7db48
Compare
marcusds
added a commit
that referenced
this pull request
Jul 30, 2026
#978 makes the create DTOs declare NAME_PATTERN, so the generated zod now carries the strict pattern and entityName.ts no longer has to hand-mirror it. Take ENTITY_NAME_REGEXP from entitiesCreateEntityBodyNameRegExp — the generic entity-store endpoint, not one of the per-resource copies. The max length stays local because the entity-store schema declares no maxLength; a test pins it against the create DTOs that do. That test also asserts the entity, fileset, secret, and model-provider schemas still agree on the pattern, so a future drift between them fails CI. The per-rule error messages and the sanitizer's character classes have no generated equivalent and remain hand-written. Signed-off-by: mschwab <mschwab@nvidia.com>
maxdubrinsky
approved these changes
Jul 30, 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
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.
Problem
The OpenAPI spec advertises a name pattern the platform does not accept.
Create-request DTOs declare
^[\w\-.]+$(max 255), but the entity store enforces the stricter RFC-1035-ishNAME_PATTERNdownstream:So
SparlorMy_Providervalidates against the published spec, passes any generated client's validation, and then fails with a 422. That hits every SDK consumer, not just Studio.Raised by @steramae-nvidia on #919:
#919 works around this on the frontend with a hand-mirrored copy of
NAME_PATTERN. That mirror exists only because the spec is wrong.Change
Two commits.
1. Declare the real pattern on the create DTOs
CreateModelProviderRequest.name^[\w\-.]+$, max 255NAME_PATTERN, max 63CreateFilesetRequest.name^[\w\-.]+$, max 255NAME_PATTERN, max 63PlatformSecretCreateRequest.namefield_validatoronly — no pattern in the spec at allpattern=, max 63Three things worth calling out:
regex_engine="python-re"is required.NAME_PATTERNuses lookahead and lookbehind; Pydantic's default Rust engine rejects them outright. The entity-store schemas already set this — without it the models don't build.max_length255 → 63. The regex caps at 63 by construction (^[a-z]+{1,62}), so the old pair contradicted itself.@is legal underNAME_PATTERN. Secrets previously rejecteda@bvia its own validator; it now accepts it, matching what the entity store always allowed. A loosening in one direction alongside the tightening.2. Collapse the mirrored constants
The first commit made an existing problem worse, so the second one fixes it.
NAME_PATTERNwas copy-pasted verbatim in four Python modules:nmp_common, plusfiles/types.py,secrets/types.py, andjobs/spec.pyinsidenemo_platform_plugin. Each carried a comment explaining it was inlined to avoid annmp_commondependency, andjobs/spec.pycitedfiles/types.pyas the precedent — the duplication was self-propagating.The dependency only runs one way:
nmp_common/pyproject.tomldepends onnemo-platform-plugin, andnmp_common/entities/client.pyimportsEntityBasefrom it. So the plugin cannot importnmp_commonwithout a cycle, which makes the plugin the correct home — as theno-nmp-common-in-pluginspre-commit hook's own comment prescribes.New leaf module
nemo_platform_plugin/entity_naming.pyholds the single definition and imports nothing, so modules that need to stay leaf nodes still can.nmp_commonre-exports it, leaving every existingconstants.NAME_PATTERNcall site untouched.Regenerating the OpenAPI spec after this commit produces no diff — it's a pure refactor.
Testing
Sparl,my--provider,myprovider-,1provider, 64-char,My_Providerrejected;ok-name,a@bacceptednemo_platform_extdaemonize/socket tests plus 2 evaluator errors, all confirmed identical on a clean treenmp.common.entities.constants.NAME_PATTERN is nemo_platform_plugin.entity_naming.NAME_PATTERNruff,ruff format,tyclean on changed filesfilesCreateFilesetBodyNameMax = 63and the new regex. That output is gitignored, so nothing to commitTwo secrets tests changed.
test_create_secret_with_invalid_name_returns_friendly_errorasserted a message that no longer exists and listeda@bas invalid — replaced with a reject-list plus an accept-list. The two uppercase regression tests intest_secrets_with_auth.pywere failing because the shared request model now rejects locally before the HTTP call; they usemodel_constructto bypass it so the original 500-vs-422 server-side coverage stays intact.Known remaining drift (not fixed here)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/models.py:31carries a hand-rewritten lookaround-free variant commented "Keep this aligned withnmp.common.entities.constants.NAME_PATTERN". It has drifted:a-b-b-…(81 chars)a-b-b-…(125 chars)Each hyphen branch consumes two characters per repetition, so
{1,62}admits up to 125 characters instead of 63.nemo_evaluator_sdkhas no dependency onnemo_platform_plugin, so fixing it needs either a new dependency or a test asserting equivalence — out of scope here, happy to file it.On the frontend,
maintoday has two copies (filesetName.tsandCreateSecretModal/constants.ts). #919 collapses both into one —entityName.ts— so once that merges there is a single frontend copy left. It can drop its hand-written regex in favour of the generatedfilesCreateFilesetBodyNameRegExponce this PR lands; the per-rule error messages and the sanitizer's character classes have no generated equivalent and stay.Follow-up
web/packages/common/src/utils/entityName.ts(added in #919) can be reduced to the generated schema.Summary by CodeRabbit
namevalidation across filesets, model providers, platform secrets, and related entity inputs.@ . + _, plus existing constraints: no consecutive hyphens and no trailing hyphen.