feat(models): Add prompt entity#249
Open
steramae-nvidia wants to merge 7 commits into
Open
Conversation
Contributor
Documentation preview is readyPreview: https://nvidia-nemo.github.io/nemo-platform/pr-preview/pr-249/pr-249/ Built from This preview is deployed from this PR branch, updates when docs changes are pushed, and will be removed when the PR closes. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
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:
📝 WalkthroughWalkthroughAdds workspace-scoped Prompts REST endpoints and OpenAPI contracts, Pydantic schemas and persisted Prompt entity, a PromptService implementing CRUD over EntityClient, FastAPI endpoints wired via a dependency, and unit tests for service and API behavior. ChangesPrompts API and Service
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
services/core/models/tests/unit/test_prompt_service_unit.py (1)
175-199: ⚡ Quick win
update_prompttests should cover omitted-tagssemantics.Add a case where
UpdatePromptRequestomitstagsand assert expected full-replacement behavior (tagscleared). This prevents regressions in update contract.🤖 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/models/tests/unit/test_prompt_service_unit.py` around lines 175 - 199, Add a new unit test mirroring test_update_prompt_success that sends an UpdatePromptRequest which omits the tags field and then calls prompt_service.update_prompt; assert mock_entity_client.update was called and inspect the updated_entity (mock_entity_client.update.call_args[0][0]) to verify tags is cleared (None or empty depending on contract) and other mutable fields are replaced accordingly; reference the existing test_update_prompt_success, UpdatePromptRequest, prompt_service.update_prompt, and mock_entity_client.update to locate where to add this case.services/core/models/tests/unit/api/test_prompts_api.py (1)
99-106: ⚡ Quick winMissing regression test for workspace filter override.
Add a test proving
filter[workspace]cannot override the pathworkspace(expect 400/validation failure). This guards tenant-scope boundaries.🤖 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/models/tests/unit/api/test_prompts_api.py` around lines 99 - 106, Add a regression test ensuring query filter[workspace] cannot override the path workspace: create a new test (e.g., test_list_prompts_workspace_filter_override) that calls the same endpoint used in test_list_prompts_with_name_filter but includes ?filter[workspace][]=other (while path workspace remains "default") and assert the response is a 400/validation failure and that mock_prompt_service.list_prompts was not invoked; locate this alongside test_list_prompts_with_name_filter in services/core/models/tests/unit/api/test_prompts_api.py and assert both status code and absence of service call to prove tenant-scope enforcement.
🤖 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 6712-6736: The OpenAPI PUT operation
update_prompt_apis_models_v2_workspaces__workspace__prompts__name__put currently
references UpdatePromptRequest whose properties are all optional, which yields
PATCH-like behaviour; either make the schema represent a true full replacement
by updating the components/schemas/UpdatePromptRequest to mark all mutable
fields as required (so omission means explicit null/zero) or change the
operation to a PATCH semantic (rename to patch/update summary and description
and/or reference a PartialUpdatePromptRequest) so the spec and
operationId/description align; apply the same fix for the duplicated occurrence
noted at lines ~17383-17432.
- Around line 6596-6613: The page and page_size query parameters lack bounds;
update the OpenAPI parameter schemas for "page" and "page_size" (the parameters
named page and page_size) to include numeric constraints matching other list
endpoints: add "minimum: 1" to the page schema, and add "minimum: 1" and a
sensible "maximum" (e.g., 1000) to the page_size schema (keeping default: 1 and
default: 100 respectively) so negative, zero, or unbounded sizes are prevented.
- Around line 6647-6777: The spec endpoints for prompt CRUD are missing standard
failure responses: add a 409 response to the POST operation (operationId
create_prompt_apis_models_v2_workspaces__workspace__prompts_post) and add 404
responses to the GET, PUT, and DELETE operations (operationIds
get_prompt_apis_models_v2_workspaces__workspace__prompts__name__get,
update_prompt_apis_models_v2_workspaces__workspace__prompts__name__put,
delete_prompt_apis_models_v2_workspaces__workspace__prompts__name__delete); each
new response should include a descriptive description and reference the existing
error response schema used elsewhere in the file (e.g.,
components/schemas/HTTPError or reuse components/schemas/HTTPValidationError if
appropriate) so clients can consume 404/409 error payloads consistently.
In `@openapi/ga/openapi.yaml`:
- Around line 6716-6717: The OpenAPI contract is inconsistent: the operationId
update_prompt_apis_models_v2_workspaces__workspace__prompts__name__put documents
a full replacement (PUT) but the UpdatePromptRequest schema
(UpdatePromptRequest) makes every field optional and permits {} (partial
update). Pick one behavior and make the spec consistent: either change
UpdatePromptRequest to require the mutable fields used for a full replacement
(add required properties and non-null types) so PUT truly replaces the resource,
or update the operation and description to indicate partial-update semantics (or
switch to PATCH) and document that fields are optional; apply the same change to
the duplicate block covering 17383-17433 so both operationId and
UpdatePromptRequest match.
- Around line 8403-8418: The ChatCompletionTool schema currently constrains the
type property with const: function but doesn't require it; update the
ChatCompletionTool schema so the required array includes "type" (in addition to
"function") to ensure objects must include the type field; locate the
ChatCompletionTool definition and add "type" to its required list so validation
enforces the const value.
In `@openapi/openapi.yaml`:
- Around line 6716-6717: The operation
update_prompt_apis_models_v2_workspaces__workspace__prompts__name__put claims to
be a full replacement but uses the UpdatePromptRequest schema which has no
required fields; pick one fix: either (A) enforce full-replacement semantics by
adding required properties to the UpdatePromptRequest schema (declare the
mutable fields as required and validate their types) so a PUT must supply a
complete resource, or (B) change the operation to a PATCH/partial-update (rename
operationId or change HTTP verb and description) and create a partial schema
(e.g., UpdatePromptRequestPatch) that allows optional fields; update references
to UpdatePromptRequest (lines ~17383-17433) accordingly and ensure
examples/validators reflect the chosen semantics.
- Around line 6667-6678: Add a '409' response to the prompt-creation responses
(alongside the existing '201' and '422') to document duplicate-name conflicts:
include description like "Conflict - duplicate prompt name" and a content block
application/json whose schema reuses the project's error schema (e.g. $ref
'`#/components/schemas/HTTPError`' or '`#/components/schemas/HTTPValidationError`'),
so the OpenAPI contract reflects the duplicate-prompt handling implemented in
the code.
In `@services/core/models/src/nmp/core/models/api/service/prompt_service.py`:
- Around line 153-154: The update_prompt implementation currently skips updating
entity.tags when request.tags is None which preserves old tags and breaks PUT
full-replacement semantics; change the code in update_prompt to unconditionally
replace tags (assign entity.tags = request.tags) so a missing/None tags value
clears the existing tags, or if the schema requires a list, assign entity.tags =
request.tags or [] to ensure full replacement.
In `@services/core/models/src/nmp/core/models/api/v2/prompts.py`:
- Around line 64-65: Replace all occurrences that raise HTTPException with
detail=str(e) so that internal exception details are not returned to clients;
instead log the exception server-side and return a generic message like
"Internal server error". Concretely, locate the raise HTTPException(...)
statements in this module (the ones using the exception variable e), call the
module logger with logger.exception(e) or logger.error(..., exc_info=True)
before raising, and change the raise to raise
HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
detail="Internal server error"). Apply this change to every 500 handler in this
file (the HTTPException raises shown in the diff and the other similar blocks).
- Around line 53-57: The code currently allows the query filter
parsed_filter.remove("workspace") to override the path-scoped workspace when
calling service.list_prompts, enabling cross-workspace reads; change it to
always use the path-scoped workspace (ignore any workspace value from
parsed_filter) by setting filter_workspace = workspace (and ensure
parsed_filter.remove("workspace") is not used to override), then pass that
filter_workspace into service.list_prompts (referencing parsed_filter.remove,
filter_workspace, and service.list_prompts in your change).
---
Nitpick comments:
In `@services/core/models/tests/unit/api/test_prompts_api.py`:
- Around line 99-106: Add a regression test ensuring query filter[workspace]
cannot override the path workspace: create a new test (e.g.,
test_list_prompts_workspace_filter_override) that calls the same endpoint used
in test_list_prompts_with_name_filter but includes ?filter[workspace][]=other
(while path workspace remains "default") and assert the response is a
400/validation failure and that mock_prompt_service.list_prompts was not
invoked; locate this alongside test_list_prompts_with_name_filter in
services/core/models/tests/unit/api/test_prompts_api.py and assert both status
code and absence of service call to prove tenant-scope enforcement.
In `@services/core/models/tests/unit/test_prompt_service_unit.py`:
- Around line 175-199: Add a new unit test mirroring test_update_prompt_success
that sends an UpdatePromptRequest which omits the tags field and then calls
prompt_service.update_prompt; assert mock_entity_client.update was called and
inspect the updated_entity (mock_entity_client.update.call_args[0][0]) to verify
tags is cleared (None or empty depending on contract) and other mutable fields
are replaced accordingly; reference the existing test_update_prompt_success,
UpdatePromptRequest, prompt_service.update_prompt, and mock_entity_client.update
to locate where to add this case.
🪄 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: 10f80ebd-d6cb-4c44-a37f-af2b990b7081
⛔ Files ignored due to path filters (1)
sdk/stainless.yamlis excluded by!sdk/**
📒 Files selected for processing (11)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/core/models/src/nmp/core/models/api/dependencies.pyservices/core/models/src/nmp/core/models/api/service/prompt_service.pyservices/core/models/src/nmp/core/models/api/v2/prompts.pyservices/core/models/src/nmp/core/models/entities.pyservices/core/models/src/nmp/core/models/schemas.pyservices/core/models/src/nmp/core/models/service.pyservices/core/models/tests/unit/api/test_prompts_api.pyservices/core/models/tests/unit/test_prompt_service_unit.py
Contributor
|
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Sean Teramae <steramae@nvidia.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Sean Teramae <steramae@nvidia.com>
The CodeQL autofix only patched list_prompts and create_prompt, leaving get_prompt, update_prompt, and delete_prompt still interpolating raw user-controlled workspace/name into log messages (alerts 4168-4174). Sanitize the remaining log calls via _sanitize_for_log, and switch list_prompts to use the shared helper instead of an inlined replace for consistency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Sean Teramae <steramae@nvidia.com>
- Prevent workspace filter from overriding the path-scoped workspace in list_prompts (cross-workspace read vector) - Replace detail=str(e) with generic message in all 500 handlers to avoid leaking backend internals to clients - Fix PUT full-replacement: tags was conditionally skipped; now always replaced (entity.tags = request.tags or []) - Add ge=1/le=1000 bounds on page/page_size Query params - Make ChatCompletionTool.type required (no default) so the generated OpenAPI schema marks it as required per OpenAI spec - Fix validation error handler to return "Invalid prompt data" instead of the raw exception string - Add tests: workspace scope isolation, page/page_size bounds, tags cleared on omission Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Sean Teramae <steramae@nvidia.com>
steramae-nvidia
force-pushed
the
steramae/prompts-service
branch
from
4c5f073 to
85c2728
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
services/core/models/src/nmp/core/models/api/v2/prompts.py (1)
91-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSanitize exception text before writing to logs.
Line 91, Line 100, and Line 159 interpolate raw exception text into log lines. If exception messages carry user input, log-forging via control characters is still possible.
Proposed fix
- logger.warning(f"Entity store validation error during prompt creation: {e}") + logger.warning( + "Entity store validation error during prompt creation: %s", + _sanitize_for_log(e), + ) @@ - logger.warning(f"Prompt creation validation error: {e}") + logger.warning("Prompt creation validation error: %s", _sanitize_for_log(e)) @@ - logger.warning(f"Entity store validation error during prompt update: {e}") + logger.warning( + "Entity store validation error during prompt update: %s", + _sanitize_for_log(e), + )Also applies to: 100-100, 159-159
🤖 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/models/src/nmp/core/models/api/v2/prompts.py` at line 91, The logged exception strings are interpolated raw (e.g., logger.warning(f"...{e}")) which can include control characters—replace these interpolations with a sanitized representation before logging: add a small helper (e.g., sanitize_exception_text(exc) or use repr_safe(exc)) that converts the exception to a safe string by escaping or stripping non-printable/control characters (e.g., remove ASCII 0x00–0x1F and 0x7F or escape them), and use that helper in the three logger calls that currently embed the raw exception (the logger.warning / logger.error sites in prompts.py that interpolate {e} around lines 91, 100 and 159) so logs never contain untrusted control characters.
🤖 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.
Duplicate comments:
In `@services/core/models/src/nmp/core/models/api/v2/prompts.py`:
- Line 91: The logged exception strings are interpolated raw (e.g.,
logger.warning(f"...{e}")) which can include control characters—replace these
interpolations with a sanitized representation before logging: add a small
helper (e.g., sanitize_exception_text(exc) or use repr_safe(exc)) that converts
the exception to a safe string by escaping or stripping non-printable/control
characters (e.g., remove ASCII 0x00–0x1F and 0x7F or escape them), and use that
helper in the three logger calls that currently embed the raw exception (the
logger.warning / logger.error sites in prompts.py that interpolate {e} around
lines 91, 100 and 159) so logs never contain untrusted control characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a608d2be-7e02-47b3-9697-a7d04390ad7b
⛔ Files ignored due to path filters (1)
sdk/stainless.yamlis excluded by!sdk/**
📒 Files selected for processing (11)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/core/models/src/nmp/core/models/api/dependencies.pyservices/core/models/src/nmp/core/models/api/service/prompt_service.pyservices/core/models/src/nmp/core/models/api/v2/prompts.pyservices/core/models/src/nmp/core/models/entities.pyservices/core/models/src/nmp/core/models/schemas.pyservices/core/models/src/nmp/core/models/service.pyservices/core/models/tests/unit/api/test_prompts_api.pyservices/core/models/tests/unit/test_prompt_service_unit.py
🚧 Files skipped from review as they are similar to previous changes (8)
- services/core/models/src/nmp/core/models/schemas.py
- services/core/models/src/nmp/core/models/api/dependencies.py
- services/core/models/src/nmp/core/models/entities.py
- services/core/models/tests/unit/api/test_prompts_api.py
- services/core/models/src/nmp/core/models/api/service/prompt_service.py
- openapi/openapi.yaml
- openapi/ga/openapi.yaml
- openapi/ga/individual/platform.openapi.yaml
Signed-off-by: Sean Teramae <steramae@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openapi/openapi.yaml (1)
6703-6714:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocument the 404 contract for prompt item routes.
get_prompt,update_prompt, anddelete_promptall return404when the prompt is missing, and the API tests assert those paths. The spec only advertises200/204and422, so generated clients miss a normal response case.Suggested OpenAPI fix
responses: '200': description: Return prompt details content: application/json: schema: $ref: '`#/components/schemas/Prompt`' + '404': + description: Prompt not found. '422': description: Validation Error content: application/json: schema: $ref: '`#/components/schemas/HTTPValidationError`' @@ responses: '200': description: Update an existing prompt content: application/json: schema: $ref: '`#/components/schemas/Prompt`' + '404': + description: Prompt not found. '422': description: Validation Error content: application/json: schema: $ref: '`#/components/schemas/HTTPValidationError`' @@ responses: '204': description: Delete a prompt + '404': + description: Prompt not found. '422': description: Validation Error content: application/json: schema: $ref: '`#/components/schemas/HTTPValidationError`'Also applies to: 6741-6752, 6773-6780
🤖 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 6703 - 6714, The OpenAPI ops for get_prompt, update_prompt, and delete_prompt are missing the 404 response contract; add a '404' response block to each operation with a description like "Prompt not found" and content application/json that references the appropriate not-found error schema (e.g. $ref: '`#/components/schemas/HTTPNotFoundError`' or your project's 404 error schema), and repeat the same 404 addition for the other corresponding prompt-item operation blocks (the other similar response sections mentioned in the review). Ensure the response key is '404' and matches the shape used by your existing error components so generated clients will handle missing-prompt cases.services/core/auth/src/nmp/core/auth/assets/static-authz.yaml (1)
301-355:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeclare
models.prompts.*in the permission registry.These role grants and endpoint mappings reference
models.prompts.{read,create,delete,update}, butauthz.permissions.models.promptsis never defined above. That breaks the registry contract for this bundle and can fail authz validation/load before these routes are usable.Suggested fix
models: adapters: read: description: "Read model adapters" list: description: "List model adapters" create: description: "Create model adapters" update: description: "Update model adapters" delete: description: "Delete model adapters" + prompts: + create: + description: "Create prompts" + delete: + description: "Delete prompts" + read: + description: "Read prompts" + update: + description: "Update prompts" create: description: "Create models" delete: description: "Delete models"Also applies to: 1345-1376
🤖 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/assets/static-authz.yaml` around lines 301 - 355, The roles reference permissions under models.prompts (models.prompts.read/create/delete/update) but the permission registry key authz.permissions.models.prompts is missing; add a models.prompts entry to the permissions registry in static-authz.yaml defining the four actions (read, create, update, delete) with appropriate descriptions/scopes so the registry contract is satisfied; ensure you also add the same models.prompts permission set wherever the permissions list is declared for the other bundle section referenced in the comment (the later block that mirrors these role grants).
🤖 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.
Outside diff comments:
In `@openapi/openapi.yaml`:
- Around line 6703-6714: The OpenAPI ops for get_prompt, update_prompt, and
delete_prompt are missing the 404 response contract; add a '404' response block
to each operation with a description like "Prompt not found" and content
application/json that references the appropriate not-found error schema (e.g.
$ref: '`#/components/schemas/HTTPNotFoundError`' or your project's 404 error
schema), and repeat the same 404 addition for the other corresponding
prompt-item operation blocks (the other similar response sections mentioned in
the review). Ensure the response key is '404' and matches the shape used by your
existing error components so generated clients will handle missing-prompt cases.
In `@services/core/auth/src/nmp/core/auth/assets/static-authz.yaml`:
- Around line 301-355: The roles reference permissions under models.prompts
(models.prompts.read/create/delete/update) but the permission registry key
authz.permissions.models.prompts is missing; add a models.prompts entry to the
permissions registry in static-authz.yaml defining the four actions (read,
create, update, delete) with appropriate descriptions/scopes so the registry
contract is satisfied; ensure you also add the same models.prompts permission
set wherever the permissions list is declared for the other bundle section
referenced in the comment (the later block that mirrors these role grants).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5673d03c-341b-4b46-bcd2-4875e4d5750a
⛔ Files ignored due to path filters (1)
sdk/stainless.yamlis excluded by!sdk/**
📒 Files selected for processing (4)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/core/auth/src/nmp/core/auth/assets/static-authz.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- openapi/ga/openapi.yaml
- openapi/ga/individual/platform.openapi.yaml
Signed-off-by: Sean Teramae <steramae@nvidia.com>
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.
Summary by CodeRabbit
New Features
Tests
Chores