feat(experiments): Add BE for new Experiment pinned_at field#349
Merged
Conversation
Signed-off-by: shanaiabuggy <[email protected]>
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds experiment pinning to the intake service: a new ChangesExperiment Pinning
Possibly related PRs
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 10501-10506: The `pinned_at` field description documents that it
can be null when unpinned, but the type definition only allows string values.
Update the type definition for `pinned_at` to allow both string and null values
using OpenAPI 3.1 syntax by changing `type: string` to `type: [string, null]` to
match the documented behavior and prevent client/validator errors when receiving
null responses.
In `@openapi/ga/openapi.yaml`:
- Around line 10501-10506: The pinned_at field schema in openapi/openapi.yaml,
openapi/ga/openapi.yaml, and openapi/ga/individual/platform.openapi.yaml
currently only allows type: string with format: date-time, but the description
states the field can be null when unpinned. Update the pinned_at field
definition in all three files to allow null values by adding nullable: true to
the schema, or by changing type from string to [string, null] to enable proper
deserialization when experiments are unpinned.
In `@openapi/openapi.yaml`:
- Around line 10501-10506: The pinned_at field schema currently only accepts
string values in date-time format, but the description states it should be null
when unpinned. Update the pinned_at field schema to allow null values alongside
the date-time string type by adding `nullable: true` to the schema definition,
ensuring generated clients will accept both null responses for unpinned
experiments and date-time strings for pinned experiments.
- Around line 3863-3866: The documentation in the OpenAPI specification
describes the pin operation as "Idempotent" but this is incorrect because the
operation updates the `pinned_at` timestamp to the current time on every call,
which changes state and ordering. Remove the claim that this operation is
idempotent, or replace it with accurate terminology such as "repeatable" that
correctly describes the behavior of re-pinning an already-pinned experiment
while acknowledging that the timestamp and ordering will change with each call.
In `@services/core/auth/src/nmp/core/auth/assets/static-authz.yaml`:
- Around line 949-958: The pin and unpin operations for experiments are
currently mapped to incorrect permissions because they mutate existing
experiment state (the `pinned_at` field) rather than creating or deleting
resources. Find the `pin` and `unpin` operation definitions in the
static-authz.yaml file and update them to use the `intake.experiments.update`
permission instead of their current permission mappings, while keeping
appropriate write scopes like `intake:write` and `platform:write`. This ensures
that only principals with update rights can perform these operations.
🪄 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: 1a6a506e-54b6-4082-9409-927664941a13
⛔ Files ignored due to path filters (19)
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/resources/experiments/api.mdis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiments/experiments.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/files/api.mdis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/files/filesets.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_filter_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_list_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_response.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/files/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/files/fileset.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/files/fileset_create_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/files/fileset_metadata_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/shared/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/shared/fileset_metadata.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/shared_params/fileset_metadata_param.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/test_experiments.pyis excluded by!sdk/**sdk/stainless.yamlis excluded by!sdk/**
📒 Files selected for processing (8)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/core/auth/src/nmp/core/auth/assets/static-authz.yamlservices/intake/src/nmp/intake/api/v2/experiments/endpoints.pyservices/intake/src/nmp/intake/api/v2/experiments/schemas.pyservices/intake/src/nmp/intake/entities/experiments.pyservices/intake/tests/integration/test_experiments_crud.py
Contributor
|
Signed-off-by: shanaiabuggy <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openapi/ga/individual/platform.openapi.yaml (1)
10501-10507:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse OpenAPI 3.1 null syntax for
pinned_at.
nullable: trueis not the 3.1 schema form; clients/validators can still treat this as string-only. Model null explicitly.Fix
pinned_at: title: Pinned At description: Timestamp at which the experiment was pinned, or null if unpinned. Managed via POST/DELETE /experiments/{name}/pin. - nullable: true - type: string - format: date-time + anyOf: + - type: string + format: date-time + - type: 'null'Verify with:
#!/bin/bash set -euo pipefail python - <<'PY' from pathlib import Path path = Path("openapi/ga/individual/platform.openapi.yaml") text = path.read_text() assert "openapi: 3.1.0" in "\n".join(text.splitlines()[:5]) start = text.index(" pinned_at:") end = text.index(" evaluator_names:", start) block = text[start:end] print(block) if "nullable: true" in block: raise SystemExit("pinned_at still uses nullable; use anyOf/type union with null for OpenAPI 3.1.") if "type: 'null'" not in block and 'type: "null"' not in block: raise SystemExit("pinned_at does not explicitly allow null.") PY🤖 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 10501 - 10507, The pinned_at field in the platform.openapi.yaml file uses the OpenAPI 3.0 syntax nullable: true, which is not valid for OpenAPI 3.1. Replace the nullable: true line with an anyOf structure that explicitly defines the allowed types: string with format date-time and null. This ensures the field properly declares that it can be either a date-time string or null, compliant with OpenAPI 3.1 schema requirements.
🤖 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 3863-3866: The documentation currently states that
most-recently-pinned experiments sort first when re-pinning occurs, but the GET
/experiments list endpoint does not reflect this behavior in its sorting
contract. Update the GET /experiments endpoint definition to either document the
default pin-aware ordering behavior (explaining that results are sorted by
pinned_at when experiments are pinned) or add pinned_at as an available sort
field in the sort parameter options alongside created_at, updated_at, and name.
Ensure the list endpoint contract aligns with the behavior described in the
re-pinning documentation.
---
Duplicate comments:
In `@openapi/ga/individual/platform.openapi.yaml`:
- Around line 10501-10507: The pinned_at field in the platform.openapi.yaml file
uses the OpenAPI 3.0 syntax nullable: true, which is not valid for OpenAPI 3.1.
Replace the nullable: true line with an anyOf structure that explicitly defines
the allowed types: string with format date-time and null. This ensures the field
properly declares that it can be either a date-time string or null, compliant
with OpenAPI 3.1 schema requirements.
🪄 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: 2e031e3d-8995-4e85-aff4-100297bbcae5
⛔ Files ignored due to path filters (2)
sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiments/experiments.pyis excluded by!sdk/**
📒 Files selected for processing (8)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/core/auth/src/nmp/core/auth/assets/static-authz.yamlservices/core/models/tests/integration/test_model_entity_service_integration.pyservices/core/models/tests/unit/test_model_entity_service_unit.pyservices/intake/src/nmp/intake/api/v2/experiments/endpoints.pyservices/intake/src/nmp/intake/api/v2/experiments/schemas.py
✅ Files skipped from review due to trivial changes (1)
- services/core/models/tests/integration/test_model_entity_service_integration.py
🚧 Files skipped from review as they are similar to previous changes (3)
- services/intake/src/nmp/intake/api/v2/experiments/schemas.py
- services/intake/src/nmp/intake/api/v2/experiments/endpoints.py
- openapi/openapi.yaml
Signed-off-by: shanaiabuggy <[email protected]>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/intake/tests/integration/test_experiments_crud.py (1)
417-422: ⚡ Quick winAlso test unpin (DELETE) for consistency.
Line 421 only tests POST /pin. Test DELETE /pin as well to match the pattern in
test_pin_unknown_experiment_returns_404(lines 413-414).Suggested addition
assert client.delete(f"{EXPERIMENTS}/exp-soft").status_code == 204 assert client.post(f"{EXPERIMENTS}/exp-soft/pin").status_code == 404 + assert client.delete(f"{EXPERIMENTS}/exp-soft/pin").status_code == 404🤖 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/tests/integration/test_experiments_crud.py` around lines 417 - 422, The test_pin_rejects_deleted_experiment function currently only tests the POST endpoint for pinning a deleted experiment but should also test the DELETE endpoint for unpinning to be consistent with the pattern used in test_pin_unknown_experiment_returns_404. Add an assertion that tests the DELETE request to the pin endpoint on the deleted experiment, verifying that it also returns a 404 status code, matching the structure of the existing POST assertion.
🤖 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 `@services/intake/tests/integration/test_experiments_crud.py`:
- Around line 417-422: The test_pin_rejects_deleted_experiment function
currently only tests the POST endpoint for pinning a deleted experiment but
should also test the DELETE endpoint for unpinning to be consistent with the
pattern used in test_pin_unknown_experiment_returns_404. Add an assertion that
tests the DELETE request to the pin endpoint on the deleted experiment,
verifying that it also returns a 404 status code, matching the structure of the
existing POST assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 386a0c0e-b282-440e-868d-18c574db7f0a
⛔ Files ignored due to path filters (3)
sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiments/experiments.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_list_params.pyis excluded by!sdk/**
📒 Files selected for processing (5)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/intake/src/nmp/intake/api/v2/experiments/endpoints.pyservices/intake/tests/integration/test_experiments_crud.py
🚧 Files skipped from review as they are similar to previous changes (4)
- openapi/ga/openapi.yaml
- openapi/ga/individual/platform.openapi.yaml
- openapi/openapi.yaml
- services/intake/src/nmp/intake/api/v2/experiments/endpoints.py
Signed-off-by: shanaiabuggy <[email protected]>
Signed-off-by: shanaiabuggy <[email protected]>
shanaiabuggy
commented
Jun 22, 2026
asutermo
approved these changes
Jun 22, 2026
Signed-off-by: shanaiabuggy <[email protected]>
shanaiabuggy
enabled auto-merge
June 22, 2026 17:21
shanaiabuggy
disabled auto-merge
June 22, 2026 17:36
This was referenced Jun 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
pinned_attimestamp to distinguish pinned vs unpinned experiments.filter[is_pinned]=true/false) and sorting bypinned_at(including-pinned_at).filter[is_pinned], andpinned_atsort options.pinned_atsorting.