feat(intake): Create Annotation schema and API#31
Merged
Conversation
Contributor
Documentation preview is readyPreview: https://nvidia-nemo.github.io/nemo-platform/pr-preview/pr-31/pr-31/ Built from This preview is deployed from this PR branch, updates when docs changes are pushed, and will be removed when the PR closes. |
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:
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 (3)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR introduces a complete post-hoc annotation system for intake spans. Users can attach feedback, labels, notes, and metadata to spans or sessions; retrieve, filter, list, and delete annotations via REST API; manage permissions by role; and access these operations through both HTTP and CLI. The implementation spans domain models, OpenAPI specs, ClickHouse persistence, FastAPI handlers, authorization rules, CLI commands, and integration tests. ChangesPost-hoc Span Annotations
🚥 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 docstrings
🧪 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: 13
🤖 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 5695-5790: This endpoint
/apis/intake/v2/workspaces/{workspace}/annotations/{annotation_id} is missing
404 responses for the GET, PATCH and DELETE operations; update the OpenAPI doc
to add a '404' response object for each of the three operations (operationId
get_annotation_apis_intake_v2_workspaces__workspace__annotations__annotation_id__get,
update_annotation_apis_intake_v2_workspaces__workspace__annotations__annotation_id__patch,
delete_annotation_apis_intake_v2_workspaces__workspace__annotations__annotation_id__delete)
so clients can generate not-found handling—add the 404 description and
appropriate content/schema (e.g., the existing error schema used elsewhere like
HTTPValidationError or the service's NotFound schema).
- Around line 11225-11396: AnnotationInput and AnnotationUpdate are too
permissive; change them to use a kind-discriminated oneOf with per-kind schemas
so generated clients match server validation: create distinct schemas (e.g.,
AnnotationInput_feedback, AnnotationInput_label, AnnotationInput_note,
AnnotationInput_metadata and corresponding AnnotationUpdate_* variants) that
declare the exact required/optional fields and constraints for each
AnnotationKind, then replace AnnotationInput and AnnotationUpdate with a oneOf
referencing those per-kind schemas and add a discriminator on "kind" mapping
each enum value to its schema; ensure each per-kind schema includes the shared
properties (session_id, span_id if applicable), sets additionalProperties:false,
and preserves titles/descriptions so runtime 422s are reflected in the OpenAPI
contract.
In `@openapi/ga/openapi.yaml`:
- Around line 5714-5790: Add a documented 404 response for the annotation-by-id
endpoints (operationId
get_annotation_apis_intake_v2_workspaces__workspace__annotations__annotation_id__get,
update_annotation_apis_intake_v2_workspaces__workspace__annotations__annotation_id__patch,
and
delete_annotation_apis_intake_v2_workspaces__workspace__annotations__annotation_id__delete).
For each of these operations, add a '404' responses entry with description
(e.g., "Annotation not found") and content application/json referencing the
appropriate not-found schema (e.g., $ref: '`#/components/schemas/HTTPNotFound`' or
your project's equivalent) so generated clients can handle missing-annotation
cases. Ensure the 404 block mirrors the structure used for the '422' responses.
- Around line 11306-11350: The AnnotationInput schema currently only requires
kind and session_id, allowing invalid field combos; update
openapi/ga/openapi.yaml to model per-AnnotationKind shapes (use oneOf with a
discriminator keyed on AnnotationKind or explicit per-kind schemas) so each
variant (e.g., AnnotationInputText, AnnotationInputNumeric, etc.) declares its
required and forbidden properties and maps to the AnnotationKind values,
ensuring only the allowed fields are valid for a given kind in POST
/annotations; likewise modify or add AnnotationUpdate variants (e.g.,
AnnotationUpdateText, AnnotationUpdateNumeric) or clearly document that updates
are validated only server-side—ensure the discriminator references the existing
AnnotationKind enum and that AnnotationInput/AnnotationUpdate only accept the
matching variant fields.
In `@openapi/openapi.yaml`:
- Around line 5715-5726: Add an explicit 404 response object to the GET, PATCH
and DELETE operations for the /annotations/{annotation_id} path so clients have
a not-found contract; specifically, update each operation under the
/annotations/{annotation_id} path (the operations referenced in the blocks
around the diff ranges) to include a '404' response with a descriptive message
(e.g., "Not Found") and an application/json schema reference to your
not-found/error schema (for example '`#/components/schemas/HTTPNotFound`' or the
project’s standard error schema), mirroring the existing '200' and '422'
response structure; repeat the same 404 addition for the other two occurrences
noted in the comment.
- Around line 11408-11416: The response schema for AnnotationsPage.sort is
currently an open string; change it to the declared enum used by requests so
SDKs stay consistent — replace the sort property schema with a reference to the
AnnotationSortField enum (or inline the same enum values) instead of type:
string, ensuring the property name is AnnotationsPage.sort and it uses the
AnnotationSortField enum definition.
In
`@packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/annotations.py`:
- Around line 281-283: The update command currently requires a positional name
which prevents partial PATCH updates; change the signature(s) that declare name
(e.g., the parameters annotated as "name: Annotated[str, typer.Argument()]") to
make name optional by using typer.Option("--name") with a default of None and
typing Optional[str] (or str | None), so the CLI accepts --name or omits it for
partial updates; apply the same change to the other occurrence mentioned (lines
301-304 equivalent), and update any example/usage docs to show using --name
instead of a positional argument.
In `@services/intake/src/nmp/intake/spans/annotations_repository.py`:
- Around line 102-112: soft_delete_annotation currently reuses the annotation's
existing ingested_at so the tombstone row may not outrank the original; update
the tombstone row to use a fresh ingestion timestamp before inserting.
Specifically, when building the tombstone row (the call to _annotation_to_row
with is_deleted=True), set/override the ingested_at field to the current UTC
timestamp in the same format your other rows use (same type as produced
elsewhere), then pass that row through dict_to_row and insert via
self._client.insert into "annotations" with ANNOTATION_COLUMNS so the
ReplacingMergeTree will prefer the tombstone.
In `@services/intake/src/nmp/intake/spans/api/annotations_schemas.py`:
- Around line 53-58: For kind=AnnotationKind.LABEL tighten validation so labels
cannot carry mixed or foreign payloads: require exactly one of value_text XOR
value_numeric (not both and not neither), continue to enforce that value_numeric
requires name, and forbid metadata payloads (use _forbid to block the
appropriate payload keys similar to how text is forbidden now). Update the block
around self.kind == AnnotationKind.LABEL (references: self.kind, value_text,
value_numeric, name, _forbid) to validate exclusivity and to reject metadata
fields.
- Around line 92-106: The AnnotationUpdate model currently accepts an all-None
payload, allowing no-op PATCHes; add a model-level validator on AnnotationUpdate
(using Pydantic v2 model_validator) that fails validation if all updatable
fields (name, value_text, value_numeric, text, metadata) are None, returning a
ValidationError or raising ValueError with a clear message; keep model_config =
ConfigDict(extra="forbid") and ensure the validator runs after field parsing so
it checks the final values.
In `@services/intake/src/nmp/intake/spans/api/annotations.py`:
- Around line 158-159: The handler currently calls
service.update_annotation(merged) and returns Annotation.from_domain(updated)
but if the row is deleted between the earlier read and this update,
service.update_annotation can raise AnnotationNotFoundError which is not handled
and yields a 500; catch AnnotationNotFoundError around the call to
service.update_annotation(merged) and translate it into an HTTP 404 response
(same behavior as the earlier not-found case), returning the appropriate 404
error response instead of letting the exception propagate.
- Around line 192-197: The current call to service.list_annotations using
AnnotationListFilter(workspace=workspace, span_id=span_id) is hard-limiting
results with page=1 and page_size=1000 (in the block around the list_annotations
call), which silently truncates large spans; change this to paginate through all
pages from service.list_annotations (or use the service's streaming/all-items
API if available) by repeatedly calling service.list_annotations with an
incrementing page (or using next-page tokens) and aggregating results until no
more items are returned, ensuring AnnotationSortField.CREATED_AT_ASC.value is
preserved and exposing pagination or full-result behavior to callers instead of
returning only the first 1000 items.
In `@services/intake/src/nmp/intake/spans/service.py`:
- Around line 157-161: The update_annotation method currently overwrites without
checking existence; before calling
self._annotations.save_annotations([annotation]) first verify the target exists
(e.g., call the repository/getter method on self._annotations to fetch by the
annotation's id or unique key) and if not found raise AnnotationNotFoundError,
otherwise proceed to save and return the annotation; update_annotation,
self._annotations.save_annotations, and AnnotationNotFoundError are the symbols
to modify/use.
🪄 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: d6c63c45-6237-464b-8d5f-6651ace595bb
⛔ Files ignored due to path filters (31)
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/cli/commands/api/intake/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/intake/annotations.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/intake/spans/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/intake/spans/annotations.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/evaluation/api.mdis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/annotations.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/api.mdis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/intake.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/spans/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/spans/annotations.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/spans/spans.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/evaluation/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/evaluation/metric.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/evaluation/row_score.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation_create_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation_filter_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation_kind.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation_list_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation_sort_field.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation_update_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotations_page.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/spans/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/spans/annotation_list_response.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/intake/spans/test_annotations.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/intake/test_annotations.pyis excluded by!sdk/**sdk/stainless.yamlis excluded by!sdk/**
📒 Files selected for processing (19)
docs/auth/authorization/permissions-reference.mdopenapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlpackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/__init__.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/annotations.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/spans/__init__.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/spans/annotations.pyservices/core/auth/src/nmp/core/auth/assets/static-authz.yamlservices/intake/src/nmp/intake/service.pyservices/intake/src/nmp/intake/spans/annotations_repository.pyservices/intake/src/nmp/intake/spans/api/annotations.pyservices/intake/src/nmp/intake/spans/api/annotations_schemas.pyservices/intake/src/nmp/intake/spans/api/dependencies.pyservices/intake/src/nmp/intake/spans/clickhouse_migrations.pyservices/intake/src/nmp/intake/spans/domain.pyservices/intake/src/nmp/intake/spans/service.pyservices/intake/tests/integration/spans/test_annotations_crud.pyservices/intake/tests/integration/spans/test_clickhouse_bootstrap.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openapi/openapi.yaml (1)
5721-5722:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a typed 404 error body, not just description.
Line 5721, Line 5760, and Line 5789 define
404with description only. Addcontent.application/json.schemausing the same standard not-found/error schema used elsewhere in this spec so clients get a stable typed error contract.Also applies to: 5760-5761, 5789-5790
🤖 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 5721 - 5722, Three 404 response blocks currently only have a description (e.g., "'404': description: Annotation not found"); update each of those 404 responses to include a typed JSON body under content.application/json.schema that references the project’s standard not-found/error schema (the same $ref used by other 404 responses in this spec, e.g., the NotFound/ErrorResponse schema under components/schemas). Ensure the content block mirrors other responses (content: application/json: schema: $ref: '`#/components/schemas/YourStandardNotFoundSchema`') so clients receive a stable typed error contract.
🤖 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 `@openapi/openapi.yaml`:
- Around line 5721-5722: Three 404 response blocks currently only have a
description (e.g., "'404': description: Annotation not found"); update each of
those 404 responses to include a typed JSON body under
content.application/json.schema that references the project’s standard
not-found/error schema (the same $ref used by other 404 responses in this spec,
e.g., the NotFound/ErrorResponse schema under components/schemas). Ensure the
content block mirrors other responses (content: application/json: schema: $ref:
'`#/components/schemas/YourStandardNotFoundSchema`') so clients receive a stable
typed error contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ec1fa265-ec66-4119-a5ea-824f5a687901
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**
📒 Files selected for processing (7)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/intake/src/nmp/intake/spans/annotations_repository.pyservices/intake/src/nmp/intake/spans/api/annotations.pyservices/intake/src/nmp/intake/spans/api/annotations_schemas.pyservices/intake/src/nmp/intake/spans/service.py
Signed-off-by: shanaiabuggy <59746633+shanaiabuggy@users.noreply.github.com>
Signed-off-by: shanaiabuggy <59746633+shanaiabuggy@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
openapi/ga/openapi.yaml (2)
11312-11402:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftModel
AnnotationInput/AnnotationUpdateperkind.The schema still allows invalid field mixes for a given
kindbecause every variant field is always legal. That pushes a contract error to runtime only. UseoneOfplus akinddiscriminator for create, and either do the same for patch or split patch shapes by kind.🤖 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/openapi.yaml` around lines 11312 - 11402, AnnotationInput and AnnotationUpdate currently permit invalid field combinations because all kind-specific fields are optional; change AnnotationInput to a discriminator-based oneOf using AnnotationKind (keep AnnotationKind enum) and declare per-kind schemas (e.g., AnnotationInput_Feedback, AnnotationInput_Label, AnnotationInput_Note, AnnotationInput_Metadata) that set required fields and restrict allowed properties, then replace AnnotationInput with a oneOf + discriminator keyed on "kind" (ensure additionalProperties: false on each variant). For updates, either mirror the same discriminator+oneOf pattern with per-kind AnnotationUpdate_* partial schemas (so only allowed fields for that kind can be patched) or split PATCH request bodies into separate per-kind update schemas referenced similarly; keep common fields (session_id etc.) where appropriate and preserve existing constraints (maxLength, minLength) on the per-kind variants.
5721-5723:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocument the 404 error body.
These
404responses still only expose a description. Generated clients cannot type the not-found payload unless you add the sameapplication/jsonerror schema used elsewhere for 404s.Also applies to: 5760-5762, 5789-5790
🤖 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/openapi.yaml` around lines 5721 - 5723, Replace the bare '404' response descriptions (e.g., the block with "description: Annotation not found") with a full response object that includes content -> application/json using the same error schema referenced elsewhere (for example the existing components schema used for other 404s, e.g. ErrorResponse or the existing error model) so generated clients can type the not-found payload; update the other two occurrences of the plain '404' blocks as well to use the identical content/application/json + $ref to the shared error schema.openapi/ga/individual/platform.openapi.yaml (1)
11312-11402:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake create/update payloads
kind-discriminated.These schemas still allow invalid field mixes across
feedback,label,note, andmetadata, while the server validates bykind. Model them asoneOfvariants with akinddiscriminator so the spec matches runtime422behavior.🤖 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 11312 - 11402, The schemas AnnotationInput and AnnotationUpdate allow invalid field combinations because fields are not discriminated by AnnotationKind; change both to use oneOf with discriminators keyed on kind (AnnotationKind) and provide separate variant schemas (e.g., AnnotationInputFeedback, AnnotationInputLabel, AnnotationInputNote, AnnotationInputMetadata and corresponding AnnotationUpdate variants) each listing the allowed/required fields for that kind so the OpenAPI spec enforces the same validation the server applies and will surface 422-like validation errors.
🧹 Nitpick comments (2)
services/intake/src/nmp/intake/spans/api/annotations.py (1)
247-263: ⚡ Quick winAvoid importing a private schema mixin for PATCH validation.
_merge_updatedepends on_AnnotationFieldsMixin, which is a private symbol from another module. This couples the endpoint to non-public internals and can break on schema refactors. Validate through a public schema/helper instead.🤖 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/intake/src/nmp/intake/spans/api/annotations.py` around lines 247 - 263, The _merge_update routine currently instantiates the private _AnnotationFieldsMixin from annotations_schemas to re-validate merged fields; replace that private usage with a public validation schema or helper exported by nmp.intake.spans.api.annotations_schemas (do not import _AnnotationFieldsMixin). Update the import and call site in _merge_update to use the module's public validator (for example, an exported AnnotationFields schema class or a validate_annotation_fields helper) to validate kind, name, value_text, value_numeric, text, and metadata, ensuring the merged values are validated the same way while relying only on public symbols.openapi/openapi.yaml (1)
11418-11422: ⚡ Quick winType
AnnotationsPage.filterasAnnotationFilter.Lines 11418-11422 erase the filter contract back to
object, while the request already definesAnnotationFilter. Reuse that schema here so SDKs keep a stable typed shape.🤖 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 11418 - 11422, The AnnotationsPage schema currently defines "filter" as a generic object; replace that with a reference to the existing AnnotationFilter schema so the field has a stable typed shape. Locate the AnnotationsPage definition and change the "filter" property to reuse AnnotationFilter (e.g., use $ref to AnnotationFilter) instead of "type: object" and "additionalProperties: true" so SDKs will generate the correct typed contract for AnnotationsPage.filter.
🤖 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 11263-11282: The Annotation schema currently omits created_by from
its required list; update the Annotation object’s required array to include
"created_by" so the server-populated created_by string (field name created_by in
the Annotation schema) is marked non-optional in the OpenAPI spec.
In `@openapi/openapi.yaml`:
- Around line 11312-11356: The OpenAPI schema for AnnotationInput currently only
requires kind and session_id so clients can omit kind-specific fields; update
AnnotationInput to be a per-kind union (use oneOf with a discriminator on the
kind property or separate request schemas referenced by a discriminator) and
define each variant schema (e.g., AnnotationInputText, AnnotationInputNumeric,
etc.) with the appropriate required properties for that kind and shared
properties (session_id, span_id, metadata) pulled into a base schema; ensure the
discriminator mapping points to the correct variant schemas and remove the loose
interpretation that allows missing kind-specific payloads.
---
Duplicate comments:
In `@openapi/ga/individual/platform.openapi.yaml`:
- Around line 11312-11402: The schemas AnnotationInput and AnnotationUpdate
allow invalid field combinations because fields are not discriminated by
AnnotationKind; change both to use oneOf with discriminators keyed on kind
(AnnotationKind) and provide separate variant schemas (e.g.,
AnnotationInputFeedback, AnnotationInputLabel, AnnotationInputNote,
AnnotationInputMetadata and corresponding AnnotationUpdate variants) each
listing the allowed/required fields for that kind so the OpenAPI spec enforces
the same validation the server applies and will surface 422-like validation
errors.
In `@openapi/ga/openapi.yaml`:
- Around line 11312-11402: AnnotationInput and AnnotationUpdate currently permit
invalid field combinations because all kind-specific fields are optional; change
AnnotationInput to a discriminator-based oneOf using AnnotationKind (keep
AnnotationKind enum) and declare per-kind schemas (e.g.,
AnnotationInput_Feedback, AnnotationInput_Label, AnnotationInput_Note,
AnnotationInput_Metadata) that set required fields and restrict allowed
properties, then replace AnnotationInput with a oneOf + discriminator keyed on
"kind" (ensure additionalProperties: false on each variant). For updates, either
mirror the same discriminator+oneOf pattern with per-kind AnnotationUpdate_*
partial schemas (so only allowed fields for that kind can be patched) or split
PATCH request bodies into separate per-kind update schemas referenced similarly;
keep common fields (session_id etc.) where appropriate and preserve existing
constraints (maxLength, minLength) on the per-kind variants.
- Around line 5721-5723: Replace the bare '404' response descriptions (e.g., the
block with "description: Annotation not found") with a full response object that
includes content -> application/json using the same error schema referenced
elsewhere (for example the existing components schema used for other 404s, e.g.
ErrorResponse or the existing error model) so generated clients can type the
not-found payload; update the other two occurrences of the plain '404' blocks as
well to use the identical content/application/json + $ref to the shared error
schema.
---
Nitpick comments:
In `@openapi/openapi.yaml`:
- Around line 11418-11422: The AnnotationsPage schema currently defines "filter"
as a generic object; replace that with a reference to the existing
AnnotationFilter schema so the field has a stable typed shape. Locate the
AnnotationsPage definition and change the "filter" property to reuse
AnnotationFilter (e.g., use $ref to AnnotationFilter) instead of "type: object"
and "additionalProperties: true" so SDKs will generate the correct typed
contract for AnnotationsPage.filter.
In `@services/intake/src/nmp/intake/spans/api/annotations.py`:
- Around line 247-263: The _merge_update routine currently instantiates the
private _AnnotationFieldsMixin from annotations_schemas to re-validate merged
fields; replace that private usage with a public validation schema or helper
exported by nmp.intake.spans.api.annotations_schemas (do not import
_AnnotationFieldsMixin). Update the import and call site in _merge_update to use
the module's public validator (for example, an exported AnnotationFields schema
class or a validate_annotation_fields helper) to validate kind, name,
value_text, value_numeric, text, and metadata, ensuring the merged values are
validated the same way while relying only on public symbols.
🪄 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: 8d72c28e-a35d-4834-bf84-8ea7a3abd37e
⛔ Files ignored due to path filters (31)
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/cli/commands/api/intake/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/intake/annotations.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/intake/spans/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/intake/spans/annotations.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/evaluation/api.mdis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/annotations.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/api.mdis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/intake.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/spans/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/spans/annotations.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/spans/spans.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/evaluation/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/evaluation/metric.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/evaluation/row_score.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation_create_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation_filter_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation_kind.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation_list_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation_sort_field.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation_update_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotations_page.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/spans/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/spans/annotation_list_response.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/intake/spans/test_annotations.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/intake/test_annotations.pyis excluded by!sdk/**sdk/stainless.yamlis excluded by!sdk/**
📒 Files selected for processing (19)
docs/auth/authorization/permissions-reference.mdopenapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlpackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/__init__.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/annotations.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/spans/__init__.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/spans/annotations.pyservices/core/auth/src/nmp/core/auth/assets/static-authz.yamlservices/intake/src/nmp/intake/service.pyservices/intake/src/nmp/intake/spans/annotations_repository.pyservices/intake/src/nmp/intake/spans/api/annotations.pyservices/intake/src/nmp/intake/spans/api/annotations_schemas.pyservices/intake/src/nmp/intake/spans/api/dependencies.pyservices/intake/src/nmp/intake/spans/clickhouse_migrations.pyservices/intake/src/nmp/intake/spans/domain.pyservices/intake/src/nmp/intake/spans/service.pyservices/intake/tests/integration/spans/test_annotations_crud.pyservices/intake/tests/integration/spans/test_clickhouse_bootstrap.py
✅ Files skipped from review due to trivial changes (2)
- services/intake/tests/integration/spans/test_clickhouse_bootstrap.py
- docs/auth/authorization/permissions-reference.md
BrianNewsom
reviewed
May 26, 2026
BrianNewsom
reviewed
May 26, 2026
Signed-off-by: shanaiabuggy <59746633+shanaiabuggy@users.noreply.github.com>
BrianNewsom
approved these changes
May 26, 2026
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
openapi/ga/individual/platform.openapi.yaml (1)
17771-17803:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake
created_byrequired in all annotation read variants.Line 17771, Line 20522, Line 20806, and Line 23641 define
created_by, but each variant’srequiredlist omits it. This weakens the response contract for generated clients.Suggested OpenAPI fix
FeedbackAnnotation: required: - annotation_id - workspace - session_id + - created_by - created_at - ingested_at - kind - value @@ LabelAnnotation: required: - annotation_id - workspace - session_id + - created_by - created_at - ingested_at - kind - value_type - value @@ MetadataAnnotation: required: - annotation_id - workspace - session_id + - created_by - created_at - ingested_at - kind - metadata @@ NoteAnnotation: required: - annotation_id - workspace - session_id + - created_by - created_at - ingested_at - kind - textAlso applies to: 20522-20567, 20806-20837, 23641-23671
🤖 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 17771 - 17803, Several annotation response schemas declare a created_by property (e.g., FeedbackAnnotation) but omit it from their required arrays; update each annotation read variant that defines created_by (including the schemas at the other occurrences referenced in the review) to include "created_by" in its required list so generated clients treat it as guaranteed; locate each schema that has a created_by property and add "created_by" to the corresponding required: array.
🤖 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 20538-20551: The schema currently allows `value` to be string or
number regardless of `value_type`, causing invalid payloads; update the OpenAPI
schema for the object that defines `value_type` and `value` to use conditional
JSON Schema (if/then) so that when `value_type` is "text" the `value` must be a
string, and when `value_type` is "numeric" the `value` must be a number; apply
the same conditional pattern to the read schema for `LabelAnnotation` and any
other places that expose the same `value_type`/`value` pair so runtime
validation matches the contract.
- Around line 24030-24042: The NumericFilter schema currently allows an empty
object `{}` which creates an ambiguous no-op; update the NumericFilter object
(the schema titled "NumericFilter" that defines properties $gte and $lte and has
additionalProperties: false) to require at least one bound by adding a
constraint such as minProperties: 1 (or alternatively a oneOf requiring at least
one of the properties $gte or $lte) so that an empty object is rejected.
In `@openapi/ga/openapi.yaml`:
- Around line 24029-24042: NumericFilter currently allows an empty object;
require at least one bound by adding a presence constraint: update the
NumericFilter schema (the object titled NumericFilter that defines properties
$gte and $lte) to enforce that either $gte or $lte is present—e.g., add an anyOf
that lists required: [$gte] and required: [$lte] (or alternatively
minProperties: 1 together with additionalProperties: false) so an empty `{}` no
longer validates.
- Around line 11172-11211: The AnnotationFilter schema currently allows unknown
keys which hides typos; update the AnnotationFilter object schema (the component
named AnnotationFilter in openapi.yaml) to forbid extra properties by adding
"additionalProperties: false" at the same level as "properties" and "type",
ensuring unknown filter keys will be rejected by validation.
- Around line 20584-20599: LabelAnnotationInput currently allows mismatched
value_type and value types; update the openapi schema for the
LabelAnnotationInput object to enforce consistency by adding a discriminating
schema (e.g., a oneOf with two alternatives or JSON Schema if/then): one
alternative where value_type: "numeric" and value is type: number, and the other
where value_type: "text" and value is type: string; modify the block containing
the properties value_type and value to use this oneOf/if-then logic so clients
cannot send value_type=numeric with a string value (and vice versa).
In `@openapi/openapi.yaml`:
- Around line 24029-24042: NumericFilter currently allows an empty object
(no-op); update the NumericFilter schema to require at least one bound by adding
a constraint that enforces presence of $gte and/or $lte. Specifically, modify
the NumericFilter object (the schema named NumericFilter) to include either
minProperties: 1 or a oneOf clause that requires one of the required lists
(e.g., oneOf: [{required: ["$gte"]}, {required: ["$lte"]}, {required:
["$gte","$lte"]}]) so an empty {} is rejected while $gte and/or $lte remain
valid.
- Around line 20584-20612: The schema for LabelAnnotationInput currently only
documents conditional rules in prose; update the OpenAPI schema to encode them
using JSON Schema conditional keywords: add an if/then/else block on the
value_type property (if value_type: {const: "text"} then require value:type
string and allow name to be optional; else if value_type: {const: "numeric"}
then require value:type number and add name to required). Keep the top-level
additionalProperties: false and required base fields (session_id, kind,
value_type, value) but move the type constraints for value and the conditional
requirement of name into the if/then/else clause inside the LabelAnnotationInput
schema so generated clients and validators enforce correct combos.
---
Duplicate comments:
In `@openapi/ga/individual/platform.openapi.yaml`:
- Around line 17771-17803: Several annotation response schemas declare a
created_by property (e.g., FeedbackAnnotation) but omit it from their required
arrays; update each annotation read variant that defines created_by (including
the schemas at the other occurrences referenced in the review) to include
"created_by" in its required list so generated clients treat it as guaranteed;
locate each schema that has a created_by property and add "created_by" to the
corresponding required: array.
🪄 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: 7f34cd3d-fee4-4f80-85b8-9d8d73555977
⛔ Files ignored due to path filters (21)
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/cli/commands/api/intake/annotations.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/annotations.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/api.mdis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation_create_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation_filter_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/annotation_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/feedback_annotation.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/feedback_annotation_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/label_annotation.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/label_annotation_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/metadata_annotation.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/metadata_annotation_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/note_annotation.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/note_annotation_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/numeric_filter_param.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/intake/test_annotations.pyis excluded by!sdk/**sdk/stainless.yamlis excluded by!sdk/**
📒 Files selected for processing (14)
docs/auth/authorization/permissions-reference.mdopenapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlpackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/annotations.pyservices/core/auth/src/nmp/core/auth/assets/static-authz.yamlservices/intake/src/nmp/intake/spans/annotations_repository.pyservices/intake/src/nmp/intake/spans/api/annotations.pyservices/intake/src/nmp/intake/spans/api/annotations_schemas.pyservices/intake/src/nmp/intake/spans/clickhouse_migrations.pyservices/intake/src/nmp/intake/spans/domain.pyservices/intake/src/nmp/intake/spans/service.pyservices/intake/tests/integration/spans/test_annotations_crud.pyservices/intake/tests/integration/spans/test_clickhouse_bootstrap.py
💤 Files with no reviewable changes (2)
- services/intake/src/nmp/intake/spans/service.py
- services/core/auth/src/nmp/core/auth/assets/static-authz.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/annotations.py
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/auth/authorization/permissions-reference.md
- services/intake/tests/integration/spans/test_clickhouse_bootstrap.py
- services/intake/src/nmp/intake/spans/domain.py
- services/intake/src/nmp/intake/spans/clickhouse_migrations.py
Signed-off-by: shanaiabuggy <59746633+shanaiabuggy@users.noreply.github.com>
Signed-off-by: shanaiabuggy <59746633+shanaiabuggy@users.noreply.github.com>
aray12
pushed a commit
that referenced
this pull request
May 28, 2026
* Create Annotation schema and API Signed-off-by: shanaiabuggy <59746633+shanaiabuggy@users.noreply.github.com> * lint Signed-off-by: shanaiabuggy <59746633+shanaiabuggy@users.noreply.github.com> * coderabbit Signed-off-by: shanaiabuggy <59746633+shanaiabuggy@users.noreply.github.com> * review comments Signed-off-by: shanaiabuggy <59746633+shanaiabuggy@users.noreply.github.com> * coderabbit Signed-off-by: shanaiabuggy <59746633+shanaiabuggy@users.noreply.github.com> --------- Signed-off-by: shanaiabuggy <59746633+shanaiabuggy@users.noreply.github.com> Signed-off-by: Alex Ray <alray@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.
Created schema and APIs for annotations, e.g. post span ingest human feedback which can be in the form of a human label, note, or metadata.
Summary by CodeRabbit
New Features
Documentation
Tests