Skip to content

fix(studio): wire evaluator plugin SDK + migrate eval UI to EvaluateJob API - #251

Merged
marcusds merged 13 commits into
mainfrom
fix-studio-sdk-drift/mschwab
Jun 10, 2026
Merged

marcusds merged 13 commits into
mainfrom
fix-studio-sdk-drift/mschwab

Conversation

@marcusds

@marcusds marcusds commented Jun 9, 2026 •

Copy link
Copy Markdown
Contributor

Root cause

PR #228 deregistered the legacy evaluator service from the platform runner, removing evaluator types (`MetricEvaluationJob`, `BenchmarkEvaluationJob`, `evaluationListMetricJobs`, etc.) from the platform OpenAPI spec. The `nemo-evaluator` plugin existed as a replacement but was silently skipped by `script/generate-openapi-spec.sh` because its `pyproject.toml` lacked `[tool.nemo.openapi]`. This broke `studio-ci` web-typecheck on main.

What changed

Commit 1 — wire evaluator plugin into OpenAPI gen + web SDK

  • `plugins/nemo-evaluator/pyproject.toml`: add `[tool.nemo.openapi]` to opt in to spec generation
  • `plugins/nemo-evaluator/openapi/openapi.yaml`: generated evaluator plugin spec (EvaluateJob CRUD, health routes)
  • `web/packages/sdk/orval/constants.ts`: add `evaluator` to `serviceConfigs` so `gen:all` (run via postinstall) generates the evaluator TypeScript SDK
  • `web/packages/sdk/package.json`: add `gen:evaluator` / `gen:evaluator-zod` scripts

Commit 2 — migrate studio evaluation UI to new EvaluateJob API

The new `nemo-evaluator` plugin exposes a unified `EvaluateJob` API instead of the old separate metric/benchmark entity management APIs.

Migrated (old → new):

  • `MetricEvaluationJob` / `BenchmarkEvaluationJob` → `EvaluateJob`
  • `useEvaluationListMetricJobs` → `useEvaluatorListEvaluateJobs`
  • `useEvaluationGetMetricJob` → `useEvaluatorGetEvaluateJob`
  • `useEvaluationDeleteMetricJob` → `useEvaluatorDeleteEvaluateJob`
  • `EvaluationResultsRoute`, `EvaluationResultDetailsRoute`, `ActionMenu`, `JobBulkDeleteModal`, `ComparisonPanel`, `DetailsPanel`, `evaluationJob` selectors

Removed (no equivalent in new API — metric/benchmark entity management is gone):

  • `EvaluationBenchmarksRoute`, `EvaluationMetricsRoute`, `EvaluationMetricCreateRoute`, `EvaluationMetricDetailsRoute`
  • `EvaluationBenchmarksDataView`, `EvaluationMetricsDataView`, benchmark/metric side panels
  • `useMetricPanelForm`, `useSelectedEvaluationConfigFromUrl`, `evaluationConfigMetrics` selector

Test plan

  • `script/generate-openapi-spec.sh` generates `plugins/nemo-evaluator/openapi/openapi.yaml`
  • `pnpm --filter @nemo/sdk gen:all-force` completes without error, populates `generated/evaluator/`
  • `pnpm --filter nemo-studio-ui typecheck` passes
  • Studio `web-typecheck` CI passes on main after merge

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Public Evaluator API specification added (health, greeting, job lifecycle, logs, results, status).
  • Chores
    • SDK generation configured with scripts for the evaluator client.
    • CI workflow tightened for Studio e2e runs.
  • Refactor
    • Evaluation flows simplified to route to a consolidated Evaluation Results page.
    • Side navigation and evaluation UI trimmed (metrics/benchmarks and deep-link evaluate actions removed).
  • Tests
    • Button navigation tests simplified to match new routing.

The nemo-evaluator plugin was missing [tool.nemo.openapi] so it was
never discovered by script/generate-openapi-spec.sh, and therefore never
contributed routes to a plugin openapi.yaml. Studio's evaluation UI
references evaluator types that have been removed from the platform
spec (PR #228 removed the legacy evaluator service). This PR establishes
the generation pipeline so the evaluator plugin contributes its own SDK.

Changes:
- plugins/nemo-evaluator/pyproject.toml: add [tool.nemo.openapi] to opt
  in to OpenAPI spec generation
- plugins/nemo-evaluator/openapi/openapi.yaml: generated evaluator plugin
  spec (EvaluateJob CRUD, health, hello routes)
- web/packages/sdk/orval/constants.ts: add evaluator to serviceConfigs
- web/packages/sdk/package.json: add gen:evaluator + gen:evaluator-zod
  scripts so gen:all generates the evaluator SDK

Studio typecheck still fails because the evaluation UI references the
old platform evaluator API (MetricEvaluationJob, BenchmarkEvaluationJob,
useEvaluationListBenchmarks, etc.) which no longer exists — the new
EvaluateJob API is a different design. A follow-up PR will migrate the
evaluation UI to the new API.

Signed-off-by: mschwab <mschwab@nvidia.com>
@marcusds
marcusds marked this pull request as ready for review June 9, 2026 22:44
@marcusds
marcusds requested review from a team as code owners June 9, 2026 22:44
@github-actions

github-actions Bot commented Jun 9, 2026 •

Copy link
Copy Markdown
Contributor
Suite Lines Covered Line Rate Branch Rate
Unit Tests 19025/25079 75.9% 62.4%
Integration Tests 12032/23851 50.4% 25.6%

@coderabbitai

coderabbitai Bot commented Jun 9, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a full OpenAPI 3.1 spec for the evaluator plugin, wires SDK/client generation (orval + npm scripts), updates Studio UI to navigate to the evaluation results route, and restricts the web-studio-e2e CI job to workflow_dispatch only.

Changes

Evaluator API, SDK & Frontend Integration

Layer / File(s) Summary
OpenAPI specification contract
plugins/nemo-evaluator/openapi/openapi.yaml
Defines v1 health/greeting and v2 workspace evaluation job endpoints (create, list with pagination/filtering, get, cancel, logs/status, list/get/download results) and comprehensive components/schemas for requests, metric bundles, datasets, run configs, agents/models, job statuses, results, and shared response/error types.
SDK generation pipeline wiring
plugins/nemo-evaluator/pyproject.toml, web/packages/sdk/orval/constants.ts, web/packages/sdk/package.json
Adds [tool.nemo.openapi] opt-in, registers evaluator in serviceConfigs pointing to the plugin OpenAPI file with VITE_PLATFORM_BASE_URL and zod: true, and adds gen:evaluator / gen:evaluator-zod npm scripts for client/schema generation.
Studio UI routing → evaluation results
web/packages/studio/src/components/evaluation/ButtonLaunchEvaluation.tsx, web/packages/studio/src/routes/*, web/packages/studio/src/routes/WorkspaceLayout/WorkspaceSideNav.tsx, web/packages/studio/src/components/dataViews/CustomModelsDataView/*
Replaces metric/metrics-run navigation with direct navigation to getEvaluationResultsRoute(workspace) across ButtonLaunchEvaluation, CustomizationJobListRoute, WorkspaceBaseModelsRoute, WorkspaceDashboardRoute; removes Metrics/Benchmarks nav items; replaces per-row Evaluate actions with Chat Playground.
Evaluation layouts, hooks & tests
web/packages/studio/src/routes/evaluation/*, web/packages/studio/src/components/evaluation/ButtonLaunchEvaluation.spec.tsx
EvaluationResultsLayout uses a static "Evaluations" title; EvaluationResultsRoute now uses useEvaluatorListEvaluateJobs; tests and router scaffolding simplified to the new results route.

CI Workflow Trigger Change

Layer / File(s) Summary
web-studio-e2e dispatch-only gate
.github/workflows/ci.yaml
Change web-studio-e2e job if condition to run only when github.event_name == 'workflow_dispatch'.
  • Possibly related PRs

  • Suggested reviewers

    • mckornfield
    • arpitsardhana
    • SandyChapman
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title accurately captures the two main changes: wiring the evaluator plugin SDK and migrating the UI to use the EvaluateJob API.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-studio-sdk-drift/mschwab

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
web/packages/sdk/package.json (1)

7-31: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add required test:ci and typecheck scripts.

This package is missing the required CI scripts for tests and TS type checks.

Suggested fix
@@
-    "test": "vitest run"
+    "test": "vitest run",
+    "test:ci": "vitest run --coverage",
+    "typecheck": "tsc --noEmit"

As per coding guidelines, web/**/package.json must add a test:ci script and a typecheck script.

🤖 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/sdk/package.json` around lines 7 - 31, Add missing CI scripts to
the package.json "scripts" object: create a "test:ci" script that runs the test
runner in CI mode (e.g., invoke the existing "test" target in a CI-friendly way
such as "vitest run" or equivalent) and a "typecheck" script that runs
TypeScript type checking (e.g., "tsc --noEmit"). Update the scripts block
alongside existing entries like "test" and the various "gen:*" entries so CI can
call "npm run test:ci" and "npm run typecheck".

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 `@plugins/nemo-evaluator/openapi/openapi.yaml`:
- Around line 1-423: The OpenAPI spec is missing authentication declarations;
add a components.securitySchemes entry (e.g., an OAuth2 or bearer token scheme)
and apply it globally or per-operation via the security object so protected
endpoints like
create_job_apis_evaluator_v2_workspaces__workspace__evaluate_jobs_post,
list_jobs_apis_evaluator_v2_workspaces__workspace__evaluate_jobs_get and other
/apis/evaluator/v2/... routes reflect actual auth requirements; update the
app/router auth configuration first, then regenerate the OpenAPI file so
components.securitySchemes and either a global security array or operation-level
security arrays are present and reference the new scheme.
- Around line 850-856: The schema for the temperature property is inconsistent:
description says temperature cannot be 0.0 but the schema allows 0.0 via
minimum: 0.0; update the temperature schema (the temperature property in
openapi.yaml) to disallow zero by either adding "exclusiveMinimum: true"
alongside "minimum: 0.0" or by setting a positive minimum (e.g., minimum: 0.0
with exclusiveMinimum: true or a small positive minimum) so generated clients
cannot send 0.0; ensure the change is applied to the temperature property that
includes the description, type, maximum, and minimum fields.

In `@web/packages/sdk/package.json`:
- Around line 17-21: The package.json contains a duplicate scripts key
"gen:evaluator" (seen in the scripts object as gen:evaluator declared twice);
remove the redundant declaration so "gen:evaluator" only appears once in the
scripts section (keep the intended command "tsx ./orval/generate.ts evaluator"
and delete the duplicate entry), ensuring the scripts object keys are unique and
lint errors for duplicate object keys are resolved.

---

Outside diff comments:
In `@web/packages/sdk/package.json`:
- Around line 7-31: Add missing CI scripts to the package.json "scripts" object:
create a "test:ci" script that runs the test runner in CI mode (e.g., invoke the
existing "test" target in a CI-friendly way such as "vitest run" or equivalent)
and a "typecheck" script that runs TypeScript type checking (e.g., "tsc
--noEmit"). Update the scripts block alongside existing entries like "test" and
the various "gen:*" entries so CI can call "npm run test:ci" and "npm run
typecheck".
🪄 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: f9967372-6ede-43f4-8f78-7f95825383f0

📥 Commits

Reviewing files that changed from the base of the PR and between 3502dd0 and 3e65196.

📒 Files selected for processing (4)
  • plugins/nemo-evaluator/openapi/openapi.yaml
  • plugins/nemo-evaluator/pyproject.toml
  • web/packages/sdk/orval/constants.ts
  • web/packages/sdk/package.json

Comment thread plugins/nemo-evaluator/openapi/openapi.yaml
Comment thread plugins/nemo-evaluator/openapi/openapi.yaml
Comment thread web/packages/sdk/package.json Outdated
…teJob

The legacy platform evaluator service was removed in #228, deleting all
metric/benchmark entity management APIs (MetricEvaluationJob,
useEvaluationListMetricJobs, useEvaluationListBenchmarks, etc.) from
the platform SDK. Studio's evaluation UI referenced these exclusively,
breaking studio-ci web-typecheck on main.

Migrated to the new nemo-evaluator plugin SDK (EvaluateJob-based API):
- Replaced MetricEvaluationJob/BenchmarkEvaluationJob with EvaluateJob
- Replaced useEvaluationList/Get/DeleteJob with useEvaluatorList/Get/DeleteEvaluateJob
- Removed metric entity management routes (no equivalent in new API):
  EvaluationBenchmarksRoute, EvaluationMetricsRoute,
  EvaluationMetricCreate/DetailsRoute
- Removed benchmark/metric entity data views and side panels
- Kept and migrated: EvaluationResultsRoute, EvaluationResultDetailsRoute,
  ActionMenu, JobBulkDeleteModal, ComparisonPanel, DetailsPanel,
  evaluationJob selectors, util/evaluations

Signed-off-by: mschwab <mschwab@nvidia.com>
@marcusds marcusds changed the title fix(studio): wire evaluator plugin into OpenAPI gen + web SDK fix(studio): wire evaluator plugin SDK + migrate evaluation UI to EvaluateJob API Jun 9, 2026
… scripts

Signed-off-by: mschwab <mschwab@nvidia.com>
Comment thread plugins/nemo-evaluator/openapi/openapi.yaml
@marcusds marcusds changed the title fix(studio): wire evaluator plugin SDK + migrate evaluation UI to EvaluateJob API fix(studio): wire evaluator plugin SDK + migrate eval UI to EvaluateJob API Jun 9, 2026
marcusds added 4 commits June 9, 2026 16:09
Signed-off-by: mschwab <mschwab@nvidia.com>
Signed-off-by: mschwab <mschwab@nvidia.com>
- evaluations.spec.ts: use spec.target instead of spec.model for getModelName tests
- ComparisonPanel.spec.tsx: update MSW handlers to new evaluator endpoint
  (/apis/evaluator/v2/.../evaluate/jobs/:name/results) and import
  PlatformJobStatus from evaluator schema

Signed-off-by: mschwab <mschwab@nvidia.com>
E2E tests require a running dev server (localhost:5173) and platform API
(:8080) which are never started in PR CI. Every studio PR triggered the
job via web-studio path change detection, always failing with
ERR_CONNECTION_REFUSED. Matches the benchmark-guardrails job pattern.

Signed-off-by: mschwab <mschwab@nvidia.com>
@marcusds
marcusds requested a review from a team as a code owner June 9, 2026 23:47

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)

409-409: ⚡ Quick win

Consider setting persist-credentials: false on checkout actions.

Security best practice to prevent credential leakage. Add to all new web job checkout actions.

🛡️ Example fix
       - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3
+        with:
+          persist-credentials: false

Apply to lines 409, 440, 471, 496, 521, 541.

Also applies to: 440-440, 471-471, 496-496, 521-521, 541-541

🤖 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 @.github/workflows/ci.yaml at line 409, The checkout steps using the
actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 action should set
persist-credentials: false to avoid leaking runner credentials; update each
checkout invocation (the lines containing "uses:
actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10" — e.g., the
occurrences noted around the CI workflow) to include a with: block and add
persist-credentials: false so the action does not persist GITHUB_TOKEN
credentials to the workspace.

Source: Linters/SAST tools

🤖 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 @.github/workflows/ci.yaml:
- Around line 448-455: The workflow step "Test changed packages" uses direct
GitHub template expansions in the shell (e.g. "${{ github.event_name }}" and
"${{ github.base_ref }}") which can be unsafe; export those GitHub context
values into environment variables at the step level (e.g. set GITHUB_EVENT_NAME:
${{ github.event_name }} and GITHUB_BASE_REF: ${{ github.base_ref }}) and then
update the shell logic to reference them (use if [ "$GITHUB_EVENT_NAME" =
"pull_request" ] and pnpm --filter="...[origin/$GITHUB_BASE_REF]" run ...),
ensuring all occurrences of "${{ github.event_name }}" and "${{ github.base_ref
}}" are replaced with the corresponding env vars in the "Test changed packages"
step.
- Around line 418-424: The workflow step directly expands ${{ github.base_ref }}
inside a shell which risks template injection; change the job step to pass
GitHub context into environment variables (e.g., set env: BASE_REF: ${{
github.base_ref }} and EVENT_NAME: ${{ github.event_name }}), then use those env
vars inside the run block (use "$BASE_REF" and "$EVENT_NAME" in the if check and
pnpm filter expression) so the shell receives only runtime environment values
instead of raw workflow templating.

---

Nitpick comments:
In @.github/workflows/ci.yaml:
- Line 409: The checkout steps using the
actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 action should set
persist-credentials: false to avoid leaking runner credentials; update each
checkout invocation (the lines containing "uses:
actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10" — e.g., the
occurrences noted around the CI workflow) to include a with: block and add
persist-credentials: false so the action does not persist GITHUB_TOKEN
credentials to the workspace.
🪄 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: 34e4790a-0842-4390-8e7e-958a891b7373

📥 Commits

Reviewing files that changed from the base of the PR and between 3e65196 and c1cd1eb.

📒 Files selected for processing (1)
  • .github/workflows/ci.yaml

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 2

🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)

409-409: ⚡ Quick win

Consider setting persist-credentials: false on checkout actions.

Security best practice to prevent credential leakage. Add to all new web job checkout actions.

🛡️ Example fix
       - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3
+        with:
+          persist-credentials: false

Apply to lines 409, 440, 471, 496, 521, 541.

Also applies to: 440-440, 471-471, 496-496, 521-521, 541-541

🤖 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 @.github/workflows/ci.yaml at line 409, The checkout steps using the
actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 action should set
persist-credentials: false to avoid leaking runner credentials; update each
checkout invocation (the lines containing "uses:
actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10" — e.g., the
occurrences noted around the CI workflow) to include a with: block and add
persist-credentials: false so the action does not persist GITHUB_TOKEN
credentials to the workspace.

Source: Linters/SAST tools

🤖 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 @.github/workflows/ci.yaml:
- Around line 448-455: The workflow step "Test changed packages" uses direct
GitHub template expansions in the shell (e.g. "${{ github.event_name }}" and
"${{ github.base_ref }}") which can be unsafe; export those GitHub context
values into environment variables at the step level (e.g. set GITHUB_EVENT_NAME:
${{ github.event_name }} and GITHUB_BASE_REF: ${{ github.base_ref }}) and then
update the shell logic to reference them (use if [ "$GITHUB_EVENT_NAME" =
"pull_request" ] and pnpm --filter="...[origin/$GITHUB_BASE_REF]" run ...),
ensuring all occurrences of "${{ github.event_name }}" and "${{ github.base_ref
}}" are replaced with the corresponding env vars in the "Test changed packages"
step.
- Around line 418-424: The workflow step directly expands ${{ github.base_ref }}
inside a shell which risks template injection; change the job step to pass
GitHub context into environment variables (e.g., set env: BASE_REF: ${{
github.base_ref }} and EVENT_NAME: ${{ github.event_name }}), then use those env
vars inside the run block (use "$BASE_REF" and "$EVENT_NAME" in the if check and
pnpm filter expression) so the shell receives only runtime environment values
instead of raw workflow templating.

---

Nitpick comments:
In @.github/workflows/ci.yaml:
- Line 409: The checkout steps using the
actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 action should set
persist-credentials: false to avoid leaking runner credentials; update each
checkout invocation (the lines containing "uses:
actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10" — e.g., the
occurrences noted around the CI workflow) to include a with: block and add
persist-credentials: false so the action does not persist GITHUB_TOKEN
credentials to the workspace.
🪄 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: 34e4790a-0842-4390-8e7e-958a891b7373

📥 Commits

Reviewing files that changed from the base of the PR and between 3e65196 and c1cd1eb.

📒 Files selected for processing (1)
  • .github/workflows/ci.yaml
🛑 Comments failed to post (2)
.github/workflows/ci.yaml (2)

418-424: ⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoff

Template injection: use environment variables for GitHub context.

Directly expanding ${{ github.base_ref }} in shell commands violates security best practices. Use env vars instead.

🔒 Proposed fix
       - name: Typecheck changed packages
+        env:
+          EVENT_NAME: ${{ github.event_name }}
+          BASE_REF: ${{ github.base_ref }}
         run: |
-          if [ "${{ github.event_name }}" = "pull_request" ]; then
-            git fetch origin "${{ github.base_ref }}" --depth=1
-            pnpm --filter="...[origin/${{ github.base_ref }}]" run --parallel --if-present typecheck
+          if [ "$EVENT_NAME" = "pull_request" ]; then
+            git fetch origin "$BASE_REF" --depth=1
+            pnpm --filter="...[origin/$BASE_REF]" run --parallel --if-present typecheck
           else
             pnpm run --recursive --parallel --if-present typecheck
           fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

        env:
          EVENT_NAME: ${{ github.event_name }}
          BASE_REF: ${{ github.base_ref }}
        run: |
          if [ "$EVENT_NAME" = "pull_request" ]; then
            git fetch origin "$BASE_REF" --depth=1
            pnpm --filter="...[origin/$BASE_REF]" run --parallel --if-present typecheck
          else
            pnpm run --recursive --parallel --if-present typecheck
          fi
🧰 Tools
🪛 zizmor (1.25.2)

[error] 420-420: code injection via template expansion (template-injection): may expand into attacker-controllable code

(template-injection)


[error] 421-421: code injection via template expansion (template-injection): may expand into attacker-controllable code

(template-injection)

🤖 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 @.github/workflows/ci.yaml around lines 418 - 424, The workflow step directly
expands ${{ github.base_ref }} inside a shell which risks template injection;
change the job step to pass GitHub context into environment variables (e.g., set
env: BASE_REF: ${{ github.base_ref }} and EVENT_NAME: ${{ github.event_name }}),
then use those env vars inside the run block (use "$BASE_REF" and "$EVENT_NAME"
in the if check and pnpm filter expression) so the shell receives only runtime
environment values instead of raw workflow templating.

Source: Linters/SAST tools


448-455: ⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoff

Template injection: use environment variables for GitHub context.

Same issue as web-typecheck. Use env vars instead of direct template expansion.

🔒 Proposed fix
       - name: Test changed packages
+        env:
+          EVENT_NAME: ${{ github.event_name }}
+          BASE_REF: ${{ github.base_ref }}
         run: |
-          if [ "${{ github.event_name }}" = "pull_request" ]; then
-            git fetch origin "${{ github.base_ref }}" --depth=1
-            pnpm --filter="...[origin/${{ github.base_ref }}]" run --parallel --if-present test:ci
+          if [ "$EVENT_NAME" = "pull_request" ]; then
+            git fetch origin "$BASE_REF" --depth=1
+            pnpm --filter="...[origin/$BASE_REF]" run --parallel --if-present test:ci
           else
             pnpm run --recursive --parallel --if-present test:ci
           fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

      - name: Test changed packages
        env:
          EVENT_NAME: ${{ github.event_name }}
          BASE_REF: ${{ github.base_ref }}
        run: |
          if [ "$EVENT_NAME" = "pull_request" ]; then
            git fetch origin "$BASE_REF" --depth=1
            pnpm --filter="...[origin/$BASE_REF]" run --parallel --if-present test:ci
          else
            pnpm run --recursive --parallel --if-present test:ci
          fi
🧰 Tools
🪛 zizmor (1.25.2)

[error] 451-451: code injection via template expansion (template-injection): may expand into attacker-controllable code

(template-injection)


[error] 452-452: code injection via template expansion (template-injection): may expand into attacker-controllable code

(template-injection)

🤖 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 @.github/workflows/ci.yaml around lines 448 - 455, The workflow step "Test
changed packages" uses direct GitHub template expansions in the shell (e.g. "${{
github.event_name }}" and "${{ github.base_ref }}") which can be unsafe; export
those GitHub context values into environment variables at the step level (e.g.
set GITHUB_EVENT_NAME: ${{ github.event_name }} and GITHUB_BASE_REF: ${{
github.base_ref }}) and then update the shell logic to reference them (use if [
"$GITHUB_EVENT_NAME" = "pull_request" ] and pnpm
--filter="...[origin/$GITHUB_BASE_REF]" run ...), ensuring all occurrences of
"${{ github.event_name }}" and "${{ github.base_ref }}" are replaced with the
corresponding env vars in the "Test changed packages" step.

Source: Linters/SAST tools

Comment thread .github/workflows/ci.yaml
Remove Metrics and Benchmarks sidebar items (routes deleted with
legacy evaluator). Redirect ButtonLaunchEvaluation, dashboard card,
customization job list, and base models "Evaluate" buttons to the
evaluation results list instead of the deleted metric-run routes.

Signed-off-by: mschwab <mschwab@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
web/packages/studio/src/routes/CustomizationJobListRoute/index.tsx (1)

86-88: ⚡ Quick win

Remove stale inline comment before evaluation navigation.

Line 86 and Line 87 describe removed URN::adapter prefill behavior, but Line 88 now always routes to results. Keep the comment aligned or delete it.

Suggested edit
-                  // EvaluationModelSelect treats `URN::adapter` as a single
-                  // form-field value, so when an adapter is selected we append
                   navigate(getEvaluationResultsRoute(workspace));
🤖 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/studio/src/routes/CustomizationJobListRoute/index.tsx` around
lines 86 - 88, The inline comment above the
navigate(getEvaluationResultsRoute(workspace)) call is stale (it mentions
URN::adapter prefill behavior tied to EvaluationModelSelect) and should be
removed or updated to reflect current behavior; locate the block containing
EvaluationModelSelect and the navigate(getEvaluationResultsRoute(workspace))
call and either delete the two comment lines referencing `URN::adapter` or
replace them with a short, accurate comment describing that the route always
navigates to evaluation results now.
🤖 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 `@web/packages/studio/src/routes/CustomizationJobListRoute/index.tsx`:
- Around line 86-88: The inline comment above the
navigate(getEvaluationResultsRoute(workspace)) call is stale (it mentions
URN::adapter prefill behavior tied to EvaluationModelSelect) and should be
removed or updated to reflect current behavior; locate the block containing
EvaluationModelSelect and the navigate(getEvaluationResultsRoute(workspace))
call and either delete the two comment lines referencing `URN::adapter` or
replace them with a short, accurate comment describing that the route always
navigates to evaluation results now.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 4aaf43ed-faad-4e16-bb30-d0965a254f89

📥 Commits

Reviewing files that changed from the base of the PR and between c1cd1eb and a37b87f.

📒 Files selected for processing (5)
  • web/packages/studio/src/components/evaluation/ButtonLaunchEvaluation.tsx
  • web/packages/studio/src/routes/CustomizationJobListRoute/index.tsx
  • web/packages/studio/src/routes/WorkspaceBaseModelsRoute/index.tsx
  • web/packages/studio/src/routes/WorkspaceDashboardRoute/index.tsx
  • web/packages/studio/src/routes/WorkspaceLayout/WorkspaceSideNav.tsx
💤 Files with no reviewable changes (1)
  • web/packages/studio/src/routes/WorkspaceLayout/WorkspaceSideNav.tsx

marcusds and others added 2 commits June 9, 2026 17:18
Remove Evaluate Model buttons/actions from EvaluationResultsLayout,
EvaluationResultsRoute, and CustomModelsDataView dropdown. Update
ButtonLaunchEvaluation spec to test the new navigate-to-results behavior.

Signed-off-by: mschwab <mschwab@nvidia.com>

@crookedstorm crookedstorm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good luck with the test in the future!

@marcusds
marcusds enabled auto-merge June 10, 2026 16:44
@marcusds
marcusds added this pull request to the merge queue Jun 10, 2026
Merged via the queue into main with commit eae5aa1 Jun 10, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants