fix(studio): validate entity names against the entity-store pattern - #919
Conversation
📝 WalkthroughWalkthroughAdds shared entity-name validation, sanitization, error messaging, and help text. Frontend forms, backend models, job-step validation, and OpenAPI schemas now use common lowercase-first naming rules with length and hyphen constraints. ChangesEntity-name validation
Sequence Diagram(s)sequenceDiagram
participant User
participant StudioForm
participant EntityNameSchema
participant ProvidersAPI
User->>StudioForm: Enter entity name
StudioForm->>EntityNameSchema: Validate name
EntityNameSchema-->>StudioForm: Return rule-specific result
StudioForm->>ProvidersAPI: Load existing provider names
ProvidersAPI-->>StudioForm: Return provider list
StudioForm-->>User: Show validation or enable submission
Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/packages/common/src/utils/entityName.ts (1)
93-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an explicit return type.
entityNameSchemais an exported API but relies on inferred Zod types. Declare the compatible schema return type for the pinned Zod version. As per coding guidelines, “Use explicit return types for public APIs and complex functions in TypeScript.”🤖 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 `@web/packages/common/src/utils/entityName.ts` around lines 93 - 100, Update the exported entityNameSchema function with an explicit return type compatible with the pinned Zod version, while preserving its current z.string().superRefine validation behavior and label default.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@web/packages/studio/src/routes/InferenceProvidersListRoute/CreateInferenceProviderSidePanel/index.tsx`:
- Around line 126-142: Update the provider-name validation around formSchema and
the useForm submission flow to validate against a complete provider set, not an
empty or first-page-only result. Ensure all provider pages are loaded (or
perform an equivalent server-side name-availability check), and block submission
while that validation data is unavailable so duplicate names are detected before
the API request.
---
Nitpick comments:
In `@web/packages/common/src/utils/entityName.ts`:
- Around line 93-100: Update the exported entityNameSchema function with an
explicit return type compatible with the pinned Zod version, while preserving
its current z.string().superRefine validation behavior and label default.
🪄 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: e608ddaf-6bb9-4efd-8aa0-9a639e72c545
📒 Files selected for processing (13)
web/packages/common/src/utils/entityName.test.tsweb/packages/common/src/utils/entityName.tsweb/packages/common/src/utils/filesetName.test.tsweb/packages/common/src/utils/filesetName.tsweb/packages/studio/src/components/FilesetCreateModal/constants.tsweb/packages/studio/src/routes/FilesetNewRoute/constants.tsweb/packages/studio/src/routes/FilesetNewRoute/index.test.tsxweb/packages/studio/src/routes/FilesetNewRoute/types.tsweb/packages/studio/src/routes/InferenceProvidersListRoute/CreateInferenceProviderSidePanel/index.test.tsxweb/packages/studio/src/routes/InferenceProvidersListRoute/CreateInferenceProviderSidePanel/index.tsxweb/packages/studio/src/routes/SecretsListRoute/CreateSecretModal/constants.tsweb/packages/studio/src/routes/SecretsListRoute/CreateSecretModal/index.tsxweb/packages/studio/src/routes/agents/AgentEvaluationsRoute/components/SubmitEvaluationModal.tsx
💤 Files with no reviewable changes (2)
- web/packages/studio/src/routes/SecretsListRoute/CreateSecretModal/constants.ts
- web/packages/studio/src/routes/FilesetNewRoute/constants.ts
|
|
@coderabbitai Added the explicit return type in 6153573 — |
|
Tip For best results, initiate chat on the files or code changes.
|
6153573 to
772d1e4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/nemo_platform_plugin/src/nemo_platform_plugin/entity_naming.py (1)
10-25: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRegex verified correct; no dedicated tests for this new shared contract.
Traced
NAME_PATTERNmanually: enforces lowercase start, 2-63 total chars (1 +{1,62}), rejects any--, rejects trailing-. MatchesNAME_PATTERN_DESCRIPTIONandNAME_MAX_LENGTH=63. No functional issue found.This module is now the single source of truth consumed by filesets, secrets, job steps, entities, model providers, workspaces, and projects — but ships with no unit tests in this change. A regression here has wide blast radius.
Want me to draft a
test_entity_naming.pycovering boundary cases (min/max length, leading digit, consecutive hyphens, trailing hyphen, allowed special chars)?🤖 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/entity_naming.py` around lines 10 - 25, Add dedicated unit tests for the shared NAME_PATTERN contract in a new test_entity_naming.py, covering minimum and maximum valid lengths, lowercase-start enforcement, rejection of leading digits, consecutive hyphens, and trailing hyphens, plus acceptance of the temporarily supported @, ., +, and _ characters.openapi/ga/individual/platform.openapi.yaml (1)
9411-9416: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName-field descriptions omit allowed special characters. All three new/updated
namefields use pattern^[a-z](?!.*--)[a-z0-9\-@.+_]{1,62}(?<!-)$, which permits_ . @ +, but each description claims only "lowercase letters, digits, and hyphens" are allowed.
openapi/ga/individual/platform.openapi.yaml#L9411-L9416: updateCreateFilesetRequest.namedescription to mention_ . @ +are also allowed.openapi/ga/individual/platform.openapi.yaml#L9682-L9687: same fix forCreateModelProviderRequest.name.openapi/ga/individual/platform.openapi.yaml#L16458-L16463: same fix forPlatformSecretCreateRequest.name.🤖 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/ga/individual/platform.openapi.yaml` around lines 9411 - 9416, Update the descriptions for CreateFilesetRequest.name in openapi/ga/individual/platform.openapi.yaml lines 9411-9416, CreateModelProviderRequest.name at lines 9682-9687, and PlatformSecretCreateRequest.name at lines 16458-16463 to state that lowercase letters, digits, hyphens, underscores, periods, at signs, and plus signs are allowed, while preserving the existing length, starting-character, consecutive-hyphen, and trailing-hyphen constraints.
🤖 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 `@openapi/ga/individual/platform.openapi.yaml`:
- Around line 9411-9416: Update the descriptions for CreateFilesetRequest.name
in openapi/ga/individual/platform.openapi.yaml lines 9411-9416,
CreateModelProviderRequest.name at lines 9682-9687, and
PlatformSecretCreateRequest.name at lines 16458-16463 to state that lowercase
letters, digits, hyphens, underscores, periods, at signs, and plus signs are
allowed, while preserving the existing length, starting-character,
consecutive-hyphen, and trailing-hyphen constraints.
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/entity_naming.py`:
- Around line 10-25: Add dedicated unit tests for the shared NAME_PATTERN
contract in a new test_entity_naming.py, covering minimum and maximum valid
lengths, lowercase-start enforcement, rejection of leading digits, consecutive
hyphens, and trailing hyphens, plus acceptance of the temporarily supported @,
., +, and _ characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 71e96903-1f76-4655-b5cc-8d9870f2ff54
📒 Files selected for processing (8)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlpackages/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
f87f793 to
cc399c9
Compare
772d1e4 to
b517407
Compare
…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>
2df2443 to
1f7db48
Compare
Forms validated names against the SDK-generated regex `^[\w\-.]+$`, which is what the service DTOs advertise in the OpenAPI spec. The entity store actually enforces a stricter RFC-1035-ish pattern downstream, so names like `Sparl` passed client-side validation and then 422'd on submit. Add `entityName` to common: mirrors `NAME_PATTERN` from packages/nmp_common/src/nmp/common/entities/constants.py and reports the specific rule a value breaks, with a repaired-name suggestion where one exists (`Sparl` -> `Name must be lowercase. Try "sparl".`). Adopt it in the inference provider, secret, and fileset forms, which each carried their own copy of the pattern or their own wording of the same rule. The inference provider form also pre-empts the duplicate-name 409 using the provider list it already fetches. The underlying fix belongs in the OpenAPI spec: once the request DTOs declare NAME_PATTERN, the generated zod is correct and this mirror can go away. Signed-off-by: mschwab <mschwab@nvidia.com>
Signed-off-by: mschwab <mschwab@nvidia.com>
The duplicate-name refinement runs against an empty set while the providers query is in flight, so a duplicate could reach the API and come back as a 409. Disable Add Provider until the list resolves. Also give entityNameSchema an explicit return type. Signed-off-by: mschwab <mschwab@nvidia.com>
#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>
b517407 to
bb7ae84
Compare
Both claimed the generated zod uses a looser pattern than the entity store. That stopped being true once the create DTOs started declaring NAME_PATTERN — the generated schema now carries the identical rule. The override survives only because generated zod reports a generic pattern mismatch, while entityNameSchema names the rule the value breaks and suggests a repair. ExperimentCreateModal keeps its version of the comment: CreateExperimentBody.name is still a bare zod.string() with no pattern at all. Signed-off-by: mschwab <mschwab@nvidia.com>
Problem
Creating an inference provider named
Sparlpassed client-side validation and then failed on submit:The forms validated against the SDK-generated regex
^[\w\-.]+$(max 255) — that's what the service DTOs advertise in the OpenAPI spec. The entity store enforces a stricter RFC-1035-ishNAME_PATTERNdownstream (packages/nmp_common/src/nmp/common/entities/constants.pyonmain; moving tonemo_platform_plugin/entity_naming.pyin #978), so the spec is looser than reality and invalid names got through to a confusing 422 toast.Three places had already worked around this independently, each with its own copy of the pattern or its own wording of the same rule.
Change
New
web/packages/common/src/utils/entityName.ts— mirrorsNAME_PATTERNand reports the specific rule a value breaks, with a repaired-name suggestion where one exists:SparlName must be lowercase. Try "sparl".invalid name!Name cannot contain spaces, "!". Use lowercase letters, numbers, and - _ . @ + only. Try "invalid-name".1providerName must start with a lowercase letter. Try "provider".my--providerName cannot contain consecutive hyphens. Try "my-provider".myprovider-Name cannot end with a hyphen. Try "myprovider".Name must be 63 characters or fewer (currently 64). Try "…".Adopted in every form that names an entity:
CreateInferenceProviderSidePanel— replaced the SDK regex; also pre-empts the duplicate-name 409 using the provider list it already fetchesCreateSecretModal— dropped its duplicateSECRET_NAME_REGEXP(closes theTODO #4082there)FilesetNewRoute/FilesetCreateModal— dropped two more hand-written wordingsSubmitEvaluationModal— replaced a manual.test()inside asuperRefinefilesetName.tsnow delegates to the shared helper and keeps onlytoValidFilesetName(domain-specific'fileset'fallback) andFILESET_NAME_MAX_LENGTH.The regex itself is no longer written here at all —
ENTITY_NAME_REGEXPisentitiesCreateEntityBodyNameRegExpfrom the generated zod, taken from the generic entity-store endpoint rather than one of the per-resource copies. That only works because of #978 (see below).ENTITY_NAME_MAX_LENGTHstays local: the entity-store schema declares nomaxLength, so there is nothing generic to import.entityName.test.tspins it against the fileset and secret create schemas that do declare one, and asserts all four generated schemas — entity, fileset, secret, model provider — still share a single pattern. Drift between them fails CI. Before #978 the model-provider assertion would have failed against^[\w\-.]+$.What stays hand-written is the part codegen can't express: the per-rule error messages and the sanitizer's character classes.
Stacked on #978 — merge that first
Important
This PR targets
astd-349-entity-name-pattern-dtos/mschwab, notmain. #978 must merge first; GitHub will retarget this tomainautomatically when it does.@steramae-nvidia asked whether the spec itself was the problem. It was, and #978 (ASTD-349) fixes it:
CreateModelProviderRequest,CreateFilesetRequest, andPlatformSecretCreateRequestnow declareNAME_PATTERNinstead of^[\w\-.]+$, withmax_length255 → 63 to match. The secrets DTO enforced its rule in afield_validator, so it published no pattern at all; that moved topattern=.NAME_PATTERNcollapse into onenemo_platform_plugin/entity_naming.py, re-exported bynmp_common.nmp_commondepends onnemo-platform-plugin, so the plugin is the only side both can import from.Originally this PR shipped its own mirrored regex as a stopgap. Stacking on #978 removes that: the generated zod now carries the strict pattern, so there is no hand-written copy of
NAME_PATTERNleft anywhere inweb/.Testing
packages/studio— 2657/2657 passpackages/common— 1359/1359 passentityName.test.ts(per-rule messages, sanitizer, zod schema, generated-schema agreement) and 7 cases to the provider panel testtsc --noEmit, eslint, prettier clean on both packagesOne existing assertion changed, and it's an upgrade:
FilesetNewRoutetypingtiny-gpt2-Apreviously matched the generic blob (which happened to contain "must start with a lowercase letter"); it now assertsName must be lowercase. Try "tiny-gpt2-a".Summary by CodeRabbit