refactor(studio): consolidate four delete modals into generic BulkDeleteModal (ASTD-232)#439
Open
aray12 wants to merge 15 commits into
Open
refactor(studio): consolidate four delete modals into generic BulkDeleteModal (ASTD-232)#439aray12 wants to merge 15 commits into
aray12 wants to merge 15 commits into
Conversation
…generic BulkDeleteModal Replaces DataDesignerJobsDataView/DeleteJobModal, SafeSynthesizerJobsDataView/DeleteJobModal, FilesetListRoute/DatasetBulkDeleteModal, and FilesetFileExplorer/BulkDeleteModal with a single generic BulkDeleteModal<T> that accepts items, open, onDelete, title, and onClose. Each caller now owns its own mutation hook and passes onDelete as a callback, keeping delete logic co-located with its query-cache invalidation while the modal handles pending state, error display, and confirmation UI via DeleteConfirmationModal. Signed-off-by: Alex Ray <alray@nvidia.com>
…re (#398) Add a new E2E test suite (`test_jobs_auth.py`) that validates workspace isolation and principal propagation under an auth-enabled platform config. Introduce a reusable `diagnostics.py` module in the jobs controller layer to collect and log structured job/step/task state on errors, and wire it into the reconciler and scheduler for automatic debug-level diagnostics when steps transition to ERROR or encounter unexpected exceptions. Refactor `e2e/conftest.py` to support multiple running-services instances keyed by config hash, enabling per-test-module platform configs (e.g., `local-subprocess.yaml` with auth enabled) to coexist in a single session. Add a `local-subprocess.yaml` E2E config and extend `nmp_testing` utilities with `grant_workspace_role`, `unique_email`, and `TEST_ADMIN_EMAIL` helpers needed by the auth test scenarios. Signed-off-by: Ryan S <267728323+ironcommit@users.noreply.github.com>
* feat: surface experiment metadata as dynamic columns Union of metadata keys across loaded rows, sorted alphabetically, inserted after the Models column. Each key becomes a hideable column via the existing EditColumnsMenu. Cell rendering: null/undefined → '-', object/array → JSON.stringify, primitives → String(). Values over 50 chars truncate with a tooltip showing the full string. Signed-off-by: Nathan Walston <nwalston@nvidia.com> * fix: normalize metadata keys to lowercase to collapse case variants 'status' and 'Status' now produce a single column instead of two identically-labelled headers. The accessor finds the first key that lowercases to a match, so the value is still retrieved correctly regardless of how the producer cased the key. Signed-off-by: Nathan Walston <nwalston@nvidia.com> * style: fix prettier formatting in ExperimentGroupDataView Signed-off-by: Nathan Walston <nwalston@nvidia.com> --------- Signed-off-by: Nathan Walston <nwalston@nvidia.com>
Signed-off-by: Swarom Muley <smuley@nvidia.com>
…usages (#385) * fix(studio): agent rule for correct usage of KUI select API, global CSS patch Signed-off-by: Octavian Drulea <odrulea@nvidia.com> * fix(studio): add agent rule to prevent future bugs Signed-off-by: Octavian Drulea <odrulea@nvidia.com> * fix(studio): apply SelectListbox in 2 call sites that were modified Signed-off-by: Octavian Drulea <odrulea@nvidia.com> * fix(studio): fix breaking test Signed-off-by: Octavian Drulea <odrulea@nvidia.com> --------- Signed-off-by: Octavian Drulea <odrulea@nvidia.com>
…356) Signed-off-by: Henrique Tolentino <htolentino@nvidia.com>
* Add a pop out window for the Studio Code Agent Signed-off-by: Henrique Tolentino <htolentino@nvidia.com> * Small fixes Signed-off-by: Henrique Tolentino <htolentino@nvidia.com> * Fix tests and format Signed-off-by: Henrique Tolentino <htolentino@nvidia.com> * Fix unrelated tests Signed-off-by: Henrique Tolentino <htolentino@nvidia.com> --------- Signed-off-by: Henrique Tolentino <htolentino@nvidia.com>
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
* feat(studio): row pinning for ExperimentGroupDataView Signed-off-by: Nicholas Kolean <nakolean@gmail.com> * change from local storage to two query approach with pinned status saved in be Signed-off-by: Nicholas Kolean <nakolean@gmail.com> * rebase fix Signed-off-by: Nicholas Kolean <nakolean@gmail.com> --------- Signed-off-by: Nicholas Kolean <nakolean@gmail.com>
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
nemoguardrails 0.21.0 depends on LangChain but does not declare it as a direct dependency, leaving consumers exposed to vulnerable transitive versions. Explicitly add langchain, langchain-community, langchain-core, and langchain-openai with minimum version bounds that include the CVE fixes, and update uv.lock accordingly. Also document the vendored Switchyard snapshot in NOTICE and include NOTICE in the root pyproject.toml license-files list. Signed-off-by: Ryan S <267728323+ironcommit@users.noreply.github.com>
) * chore: upgrade the python base container used in some other builds Signed-off-by: Brooke Storm <brookes@nvidia.com> * chore: update another image Signed-off-by: Brooke Storm <brookes@nvidia.com> * chore: correct typo Signed-off-by: Brooke Storm <brookes@nvidia.com> * chore: target the current vuln surface Signed-off-by: Brooke Storm <brookes@nvidia.com> --------- Signed-off-by: Brooke Storm <brookes@nvidia.com>
…227) (#366) * refactor(studio): replace deprecated ReadOnlyField with KVPair (ASTD-227) Migrate all usages of the deprecated ReadOnlyField component to KVPair from @nemo/common and delete the deprecated file. Signed-off-by: Alex Ray <alray@nvidia.com> * fix(studio): use nullish checks for metric params to preserve falsy values (ASTD-227) Signed-off-by: Alex Ray <alray@nvidia.com> --------- Signed-off-by: Alex Ray <alray@nvidia.com>
…dentical-delete-modals-into-one-generic Signed-off-by: Alex Ray <alray@nvidia.com>
Contributor
Contributor
|
📝 WalkthroughWalkthroughAdds job controller diagnostics (scheduler/reconciler), fixes workspace-scoped task listing, and introduces a subprocess backend grace window. Refactors the E2E harness to a config-hash pooled services model with auth support. Refactors the Studio Claude Code chat page to a shared ChangesJobs Service: Diagnostics, Scheduler Conflicts, Subprocess Grace Window
E2E Harness: Config-Hash Pooling and Auth Job Tests
Studio: Shared ClaudeCodeChatProvider, Top-Bar Pop-Out, and Job Artifacts
Studio UI: BulkDeleteModal, SelectListbox, KVPair migration, ExperimentGroup pinning
Packaging, Docker, Dependency Updates, Specs, and Docs
Possibly related PRs
🚥 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: 3
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/artifacts.ts (1)
429-435: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
studio_link→jobdetection diverges from the backend.The live (TS) path only checks
input.destination, butbuildStudioLinkHrefFromInputabove and the Python history parser (coding_agent_artifacts.py) also fall back topage/resource_type. Astudio_linkcarryingpage: "job"records a job artifact in history but not in the live stream, so the artifacts pane differs before vs. after reload.Align with the destination resolution used elsewhere
if ((toolName === 'studio_link' || toolName.endsWith('__studio_link')) && isRecord(input)) { - const destination = getString(input.destination); + const destination = + getString(input.destination) ?? getString(input.page) ?? getString(input.resource_type); const label = getString(input.label) ?? destination; const href = buildStudioLinkHrefFromInput(input, artifacts.workspace); if (label) appendLink(artifacts, { label, destination, href }); if (destination === 'job') recordJobArtifact(artifacts, input); }🤖 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/agents/ClaudeCodeChatRoute/artifacts.ts` around lines 429 - 435, The live artifact handling in artifacts.ts is only checking input.destination for `studio_link` job detection, which diverges from the shared destination resolution used by buildStudioLinkHrefFromInput and the Python history parser. Update the `studio_link` branch to resolve the target with the same fallback order as the existing href logic (including `page` and `resource_type`) before deciding whether to call `recordJobArtifact`, so live stream and history produce the same job artifact behavior.
🟡 Minor comments (20)
web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/context/ClaudeCodeChatProvider.tsx-83-86 (1)
83-86: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
startNewChatleavesloadStatusstuck on'loading'.If a session load is in flight (
loadStatus === 'loading'),startNewChatnullsrequestedSessionIdRefso the pending fetch hits the stale guard (Line 64/73) and returns beforesetLoadStatus('idle'). The status never clears, so any consumer spinner stays up.Proposed fix
const startNewChat = useCallback(() => { requestedSessionIdRef.current = null; + setLoadStatus('idle'); handleReset(); }, [handleReset]);🤖 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/agents/ClaudeCodeChatRoute/context/ClaudeCodeChatProvider.tsx` around lines 83 - 86, `startNewChat` in `ClaudeCodeChatProvider` can leave `loadStatus` stuck on `'loading'` when it clears `requestedSessionIdRef` before an in-flight load finishes. Update `startNewChat` and the related load/reset flow so any pending request still drives `setLoadStatus('idle')` on cancellation or stale-guard exit, and make sure the status is cleared even when `handleReset` is called while a session fetch is active.services/core/jobs/tests/controllers/test_subprocess_backend.py-314-333 (1)
314-333: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLeaked subprocess.
_schedule_without_otel_exportstarts a realsleep 10, thenpop(key)drops metadata without terminating it. Process lingers post-test. Terminate before pop, or use a short-lived command.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/core/jobs/tests/controllers/test_subprocess_backend.py` around lines 314 - 333, The test in test_sync_uses_persisted_task_when_local_metadata_is_missing leaks a real subprocess because _schedule_without_otel_export starts the sleep command and _process_registry.pop removes tracking without stopping it. Update the test to terminate the subprocess before removing the registry entry, or replace the long-running command with a short-lived one; use _subprocess_backend, _schedule_without_otel_export, and SubprocessProcessKey to locate the affected setup.services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.py-374-374 (1)
374-374: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMixed tz-aware/naive comparison can raise
TypeError.
datetime.datetime.minis naive. If any task lacks both timestamps while others carry tz-aware ones,maxcompares aware vs naive and throws. Use a tz-aware sentinel.Proposed fix
- latest_task = max(tasks.data, key=lambda task: task.updated_at or task.created_at or datetime.datetime.min) + latest_task = max( + tasks.data, + key=lambda task: task.updated_at + or task.created_at + or datetime.datetime.min.replace(tzinfo=datetime.timezone.utc), + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.py` at line 374, The latest_task selection in subprocess.py can raise a TypeError when max() compares tz-aware timestamps against the naive datetime.datetime.min fallback. Update the lambda used in the latest_task assignment to use a timezone-aware sentinel that matches the timestamp types being compared, so task.updated_at and task.created_at always fall back to a comparable aware datetime.web/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/index.tsx-72-85 (1)
72-85: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInvalid jobs are silently dropped and partial failures resolve as success.
validdiscards jobs missingworkspace/name. If every selected job is invalid,Promise.all([])resolves, the modal reports success and clears selection without deleting anything. Also,Promise.allrejects on the first failure while other in-flight deletes keep running, so only one error surfaces and successfully-deleted rows are silently removed.Consider
Promise.allSettled(as the dataset flow already does viauseMutateMany) and aggregating errors.🤖 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/components/dataViews/SafeSynthesizerJobsDataView/index.tsx` around lines 72 - 85, The bulk delete flow in handleDeleteJobs is treating missing workspace/name entries as a silent success and letting Promise.all short-circuit on the first delete failure. Update handleDeleteJobs in SafeSynthesizerJobsDataView to explicitly handle invalid jobs instead of dropping them, and switch the mutation fan-out to Promise.allSettled so every delete attempt completes and failures can be aggregated. Then surface a combined error from deleteJobMutation.mutateAsync results rather than resolving success when nothing was deleted.e2e/services_pool.py-25-25 (1)
25-25: 📐 Maintainability & Code Quality | 🟡 MinorAvoid
_pytest.nodes.NodeUsepytest.Modulehere; the only caller passes a module, so the internal pytest type is unnecessary.🤖 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 `@e2e/services_pool.py` at line 25, The type annotation in the services pool helper is using the internal pytest symbol `_pytest.nodes.Node` unnecessarily. Update the relevant import and any related type hints in `services_pool` to use `pytest.Module` instead, since the only caller passes a module and this keeps the API on public pytest types.plans/2026-06-03-jobs-persistent-storage-plan.md-16-21 (1)
16-21: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse repo-relative links instead of local filesystem paths.
These
/Users/...references won’t work outside your machine and make the plan harder to share. Replace them with repo-relative links.🤖 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 `@plans/2026-06-03-jobs-persistent-storage-plan.md` around lines 16 - 21, The plan currently uses local filesystem links in the referenced files, which makes it hard to share and navigate. Update the linked items in this section to use repo-relative paths instead of /Users/... locations, keeping the same target files and tests such as test_jobs.py, docker.py, endpoints.py, schemas.py, test_docker_backend.py, and test_jobs_persistent_storage.py.spec/jobs-local-remote-unification-spec.md-35-37 (1)
35-37: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse repo-relative links instead of local filesystem paths.
These
/Users/...source links are machine-specific and won’t resolve for other contributors. Replace them with repo-relative paths.🤖 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 `@spec/jobs-local-remote-unification-spec.md` around lines 35 - 37, The document currently uses machine-specific source links, so update the related entries to use repo-relative paths instead of local filesystem paths. Adjust the affected markdown in the spec so the references for local/remote target selection, subprocess inheritance, and task/job storage point to paths within the repository, using the existing link targets in this spec as the guide.spec/subprocess-first-class-execution-resolution-spec.md-33-37 (1)
33-37: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse repo-relative links instead of local filesystem paths.
These
/Users/...source links are machine-specific and won’t resolve for other contributors. Replace them with repo-relative paths.🤖 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 `@spec/subprocess-first-class-execution-resolution-spec.md` around lines 33 - 37, The affected links are using machine-specific absolute paths, so update the references in the spec to repo-relative paths instead. Replace the /Users/... style link with the corresponding repository-relative location for the jobs endpoint code, and keep the reference tied to the relevant symbols around the Jobs API ingress rewrite and the run_local/submit_remote execution paths so the link works for all contributors.plans/2026-06-03-helm-chart-kube-e2e-plan.md-3-9 (1)
3-9: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse repo-relative links instead of local filesystem paths.
These
/Users/...references are machine-specific and won’t resolve for other contributors. Switch them to repo-relative paths.🤖 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 `@plans/2026-06-03-helm-chart-kube-e2e-plan.md` around lines 3 - 9, The plan text uses machine-specific /Users/... links that will not work for other contributors. Replace every local filesystem reference with repo-relative links, especially the mentions of Makefile, e2e/conftest.py, docker-bake.hcl, and the archived Platform-Deploy chart path, so the document is portable and points to locations within this repository or clearly relative external references.plans/2026-06-03-jobs-pause-resume-plan.md-16-21 (1)
16-21: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse repo-relative links instead of local filesystem paths.
These
/Users/...references won’t work outside your machine and make the plan harder to share. Replace them with repo-relative links.🤖 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 `@plans/2026-06-03-jobs-pause-resume-plan.md` around lines 16 - 21, The plan currently uses local /Users/... links, which are not portable or shareable. Update the referenced items to use repo-relative links instead, preserving the same targets in the jobs pause/resume plan and its test/backend/dispatcher references so they work for everyone across the repo.container.md-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTurn this into a structured doc page.
This reads like internal brainstorming, not publishable documentation: no heading, no sectioning, and lots of first-person narration. Split it into a proper explanation/reference page or keep it in a plan.
As per coding guidelines,
**/*.{md,rst}content should fit one Diataxis quadrant; this doesn't yet map cleanly.🤖 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 `@container.md` at line 1, The current content is unstructured brainstorming and should be converted into a proper documentation page with a clear Diataxis fit instead of remaining as a freeform note. Refactor the text into a single-doc structure with a title, short introduction, and distinct sections that explain the concepts and relationships among jobs service, backend, executor, profile, provider, and platform config, using neutral third-person language instead of first-person narration. If it cannot be made into a publishable explanation/reference page, move it into a plan or internal note instead of keeping it in the main docs.Source: Coding guidelines
spec/jobs-backed-local-run-ux-and-progressive-start-spec.md-14-14 (1)
14-14: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRemove the machine-local path from the doc.
That link only works on your machine and leaks local environment details. Replace it with a repo-relative reference or plain code text.
🤖 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 `@spec/jobs-backed-local-run-ux-and-progressive-start-spec.md` at line 14, The doc currently includes a machine-specific link to run_local(...) in scheduler.py, which leaks local environment details and won’t work for others. Update the reference in the spec to use either a repo-relative path or plain code text, and keep the mention anchored to the run_local(...) path in the scheduler module without any absolute local filesystem details.spec/caller-execution-hints-and-profile-plumbing-spec.md-34-42 (1)
34-42: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the absolute source paths.
These
/Users/...links will be broken outside your workstation. Use repo-relative paths or plain code references 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 `@spec/caller-execution-hints-and-profile-plumbing-spec.md` around lines 34 - 42, The spec currently embeds absolute /Users/... source links that will break outside your workstation; replace them with repo-relative paths or plain code references. Update the references in the caller execution hints section to point to the relevant symbols like scheduler.py, add_job_routes(...), and the Jobs schema for platform_spec without using machine-specific paths.spec/jobs-runtime-availability-and-capabilities-spec.md-30-34 (1)
30-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the absolute source paths.
These
/Users/...links will be broken outside your workstation. Use repo-relative paths or plain code references 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 `@spec/jobs-runtime-availability-and-capabilities-spec.md` around lines 30 - 34, Replace the absolute source links in the jobs runtime spec with repo-relative paths or plain symbol references so they work outside your workstation; update the listed references in the spec text to point at the relevant symbols like validate_docker_available, the platform config downgrade logic in config.py, Jobs config/profile construction, default backend config selection, and the customization contributor instead of /Users/... paths.spec/nvapi-authentication-gateway-spec.md-450-450 (1)
450-450: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename the second
Recommendationheading.
This duplicates the earlier section title and will collide in generated anchors / lint checks.🤖 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 `@spec/nvapi-authentication-gateway-spec.md` at line 450, The second Recommendation heading duplicates the earlier section title and should be renamed to a unique section name. Update the repeated markdown heading in the spec so it has a distinct title, preserving the surrounding section content and keeping anchor generation and lint checks unambiguous.Source: Linters/SAST tools
public-images.md-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStart the page with an H1.
This page begins with##, so markdownlint will flag it and the doc has no top-level title.🤖 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 `@public-images.md` at line 1, The document currently starts with a second-level heading instead of a top-level title, which will trigger markdownlint and leaves the page without a proper title. Update the opening heading in public-images.md to an H1 and keep the rest of the section hierarchy unchanged, using the existing “Problem Statement” heading as the page’s top-level entry point.Source: Linters/SAST tools
public-images.md-13-26 (1)
13-26: 📐 Maintainability & Code Quality | 🟡 Minorpublic-images.md:13-26 — Soften the GHCR limits claim. GHCR public packages are free and support anonymous pulls, but the docs don’t promise “unlimited” rate limits, and GitHub doesn’t publish a specific anonymous-pull quota. Keep the 10 GB per-layer limit only with an official citation.
🤖 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 `@public-images.md` around lines 13 - 26, The GHCR section in public-images.md overstates the public pull and pricing limits, so soften the language in the “Optimize GitHub Container Registry (GHCR)” guidance. Keep the documented 10 GB per-layer constraint, but replace “free and unlimited” and any implied anonymous-pull quota claims with a more cautious statement that public packages support anonymous pulls and GitHub does not publish a specific public pull limit. Ensure the wording in the GHCR bullets stays aligned with the rest of the section and cites only the confirmed limit details.spec/permissionless-authenticated-routes-spec.md-50-88 (1)
50-88: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the same caller taxonomy as the main authz spec.
USER/WORKLOADis not defined anywhere else in this stack, whileplugin-service-authz-spec.mdusesANON/PRINCIPAL/SERVICE_PRINCIPAL. Rewriting this option in the same terms will make the tradeoff easier to compare and implement.🤖 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 `@spec/permissionless-authenticated-routes-spec.md` around lines 50 - 88, Rewrite the options in this spec to use the same caller taxonomy as the main authz spec: replace the `USER`/`WORKLOAD` terms in the option descriptions with `ANON`/`PRINCIPAL`/`SERVICE_PRINCIPAL` so the authenticated-route behavior is expressed consistently with `plugin-service-authz-spec.md` and easier to compare across specs.spec/service-role-visibility-and-bindability-spec.md-9-15 (1)
9-15: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDon't attribute role metadata to the v1 authz spec.
The main authz spec currently stops at permissions and path rules; roles are still a separate follow-up track. Calling roles part of that spec blurs the dependency order and makes the rollout harder to read.
🤖 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 `@spec/service-role-visibility-and-bindability-spec.md` around lines 9 - 15, The service authz spec should stay scoped to permissions, service-scoped roles, and path rules, without implying role visibility or bindability metadata is part of v1. Update the wording in the authz/spec documentation to keep roles clearly as a separate follow-up track and avoid attributing role metadata to the main spec, using the spec section that mentions permissions, service-scoped roles, and path rules as the place to adjust.spec/plugin-service-authz-spec.md-132-140 (1)
132-140: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify the empty-scope representation.
scopesis nullable in the model, but the spec never says whetherNoneand[]mean the same thing. That ambiguity will leak into validation and serialization, so pick one representation or define the difference explicitly.Also applies to: 239-247
🤖 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 `@spec/plugin-service-authz-spec.md` around lines 132 - 140, Clarify the meaning of PathRule.scopes by explicitly defining whether None and an empty list are equivalent or distinct, and update the spec consistently so validation and serialization use the same representation. Adjust the PathRule model description and any related authz sections that reference scopes so the behavior is unambiguous across callers and permissions handling.
🧹 Nitpick comments (5)
web/packages/studio/src/routes/FilesetListRoute/DatasetBulkDeleteModal/index.tsx (1)
49-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
anyin thecloneElementcast.Guidelines forbid
any. Type the element to its injected prop instead; this also drops the eslint-disable. NotecloneElementoverrides any existingonClickonslotTrigger.As per coding guidelines: "Never use `any` type in TypeScript."♻️ Proposed typing
- const trigger = isValidElement(slotTrigger) ? ( - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - cloneElement(slotTrigger as React.ReactElement<any>, { onClick: openTrigger }) + const trigger = isValidElement(slotTrigger) ? ( + cloneElement(slotTrigger as React.ReactElement<{ onClick?: React.MouseEventHandler }>, { + onClick: openTrigger, + })🤖 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/FilesetListRoute/DatasetBulkDeleteModal/index.tsx` around lines 49 - 57, The `DatasetBulkDeleteModal` trigger uses an `any` cast in the `cloneElement` call, which violates the typing guidelines and forces an eslint disable. Update the `trigger` logic to type `slotTrigger` as a React element with the injected `onClick` prop instead of `React.ReactElement<any>`, and remove the eslint suppression. Keep in mind `cloneElement` will replace any existing `onClick` on `slotTrigger`, so ensure the intended handler is the one passed through `openTrigger`.Source: Coding guidelines
web/packages/studio/src/components/evaluation/Configurations/ConfigurationDetailsPanel.tsx (1)
44-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent missing-value placeholder.
configName/configIddefault to'-', but sibling panels (Metric/LLMJudge) passundefinedand rely onKVPair's'—'default. Drop the|| '-'so all eval panels render the same placeholder.♻️ Align with KVPair default
- const configName = config.name || '-'; - const configId = config.id || '-'; + const configName = config.name; + const configId = config.id;🤖 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/components/evaluation/Configurations/ConfigurationDetailsPanel.tsx` around lines 44 - 52, The ConfigurationDetailsPanel component is overriding missing values with a different placeholder than the other eval panels. Update the configName and configId handling in ConfigurationDetailsPanel so it passes through undefined instead of forcing '-', letting KVPair apply its standard '—' default for consistency with the Metric and LLMJudge panels.spec/jobs-runtime-availability-and-capabilities-spec.md (1)
194-203: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDefine snapshot freshness before clients cache availability.
“Query Jobs once” is too loose for a dynamic availability model. Specify whether the snapshot is request-scoped, process-scoped, or revalidated before dispatch; otherwise clients will make stale selection decisions.
🤖 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 `@spec/jobs-runtime-availability-and-capabilities-spec.md` around lines 194 - 203, Clarify the freshness semantics for the availability snapshot in the Jobs flow described in the spec: explicitly state whether the set returned by Jobs is request-scoped, process-scoped, or must be revalidated before dispatch. Update the section around the shared deterministic resolver and the Jobs validation/dispatch steps to define when clients may cache the returned availability and when they must refresh it, so the resolver behavior is unambiguous.process.md (1)
1-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a top-level heading and split the memo.
This reads like an internal note, not a docs page. Give it a title and sectioned structure so it matches the repo's markdown conventions. As per coding guidelines, markdown content should fit one doc type and stay scannable.
🤖 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 `@process.md` around lines 1 - 25, This memo needs a top-level heading and clearer sectioning so it reads like a proper docs page instead of an internal note. Update the markdown in process.md by adding a title and splitting the current freeform narrative into concise sections that group the core ideas (for example, delivery intake, timeline/milestones, cycle planning, ownership, process support, and quality gates) while keeping the tone consistent with the existing proposal. Use the existing topics in the body as the anchors for the new structure.Sources: Coding guidelines, Linters/SAST tools
spec/brian-auth-call.md (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove this transcript out of
spec/or rewrite it as a summary.This is one long transcription dump, so it is hard to scan and doesn't fit the repo's markdown conventions. A titled summary with sections would be far more usable.
🤖 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 `@spec/brian-auth-call.md` at line 1, This file is a raw transcript dump under spec and should be replaced with a concise, titled summary or moved out of spec entirely. Rewrite it as a structured markdown note with sections for the auth goals, current state, gaps, next steps, and follow-ups, using the same `spec/brian-auth-call` document as the place to locate and update it. Keep only the actionable takeaways from the conversation and remove the verbatim dialogue so the document matches the repo’s markdown conventions and is easier to scan.Sources: Coding guidelines, 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 `@e2e/configs/local-subprocess.yaml`:
- Line 9: The base_url in the local-subprocess config is set to a
non-client-reachable host, which can break connections. Update the configuration
entry for base_url in local-subprocess.yaml to use a reachable address such as
http://localhost:8080 instead of 0.0.0.0, keeping the change scoped to the
config value.
In `@script/submit-hello-world-job.sh`:
- Around line 33-61: The JSON payload in submit-hello-world-job.sh is being
built by directly interpolating shell variables, which can break when values
contain quotes or newlines. Replace the heredoc-based construction around the
payload_file write with a proper JSON serializer so WORKSPACE, JOB_NAME,
MESSAGE, EXECUTION_PROFILE, and IMAGE are safely escaped. Keep the existing
payload structure and update the script’s payload generation logic accordingly.
In `@spec/options-for-multi-idp.md`:
- Around line 31-39: The spec currently links to absolute local checkout paths
under /Users/rsadler, which makes the references break outside that machine and
exposes a local filesystem path. Update the references in this section to use
repo-relative links or plain code symbols for OIDCConfig, AuthConfig, and the
related fields in base.py so the links work in any clone.
---
Outside diff comments:
In `@web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/artifacts.ts`:
- Around line 429-435: The live artifact handling in artifacts.ts is only
checking input.destination for `studio_link` job detection, which diverges from
the shared destination resolution used by buildStudioLinkHrefFromInput and the
Python history parser. Update the `studio_link` branch to resolve the target
with the same fallback order as the existing href logic (including `page` and
`resource_type`) before deciding whether to call `recordJobArtifact`, so live
stream and history produce the same job artifact behavior.
---
Minor comments:
In `@container.md`:
- Line 1: The current content is unstructured brainstorming and should be
converted into a proper documentation page with a clear Diataxis fit instead of
remaining as a freeform note. Refactor the text into a single-doc structure with
a title, short introduction, and distinct sections that explain the concepts and
relationships among jobs service, backend, executor, profile, provider, and
platform config, using neutral third-person language instead of first-person
narration. If it cannot be made into a publishable explanation/reference page,
move it into a plan or internal note instead of keeping it in the main docs.
In `@e2e/services_pool.py`:
- Line 25: The type annotation in the services pool helper is using the internal
pytest symbol `_pytest.nodes.Node` unnecessarily. Update the relevant import and
any related type hints in `services_pool` to use `pytest.Module` instead, since
the only caller passes a module and this keeps the API on public pytest types.
In `@plans/2026-06-03-helm-chart-kube-e2e-plan.md`:
- Around line 3-9: The plan text uses machine-specific /Users/... links that
will not work for other contributors. Replace every local filesystem reference
with repo-relative links, especially the mentions of Makefile, e2e/conftest.py,
docker-bake.hcl, and the archived Platform-Deploy chart path, so the document is
portable and points to locations within this repository or clearly relative
external references.
In `@plans/2026-06-03-jobs-pause-resume-plan.md`:
- Around line 16-21: The plan currently uses local /Users/... links, which are
not portable or shareable. Update the referenced items to use repo-relative
links instead, preserving the same targets in the jobs pause/resume plan and its
test/backend/dispatcher references so they work for everyone across the repo.
In `@plans/2026-06-03-jobs-persistent-storage-plan.md`:
- Around line 16-21: The plan currently uses local filesystem links in the
referenced files, which makes it hard to share and navigate. Update the linked
items in this section to use repo-relative paths instead of /Users/...
locations, keeping the same target files and tests such as test_jobs.py,
docker.py, endpoints.py, schemas.py, test_docker_backend.py, and
test_jobs_persistent_storage.py.
In `@public-images.md`:
- Line 1: The document currently starts with a second-level heading instead of a
top-level title, which will trigger markdownlint and leaves the page without a
proper title. Update the opening heading in public-images.md to an H1 and keep
the rest of the section hierarchy unchanged, using the existing “Problem
Statement” heading as the page’s top-level entry point.
- Around line 13-26: The GHCR section in public-images.md overstates the public
pull and pricing limits, so soften the language in the “Optimize GitHub
Container Registry (GHCR)” guidance. Keep the documented 10 GB per-layer
constraint, but replace “free and unlimited” and any implied anonymous-pull
quota claims with a more cautious statement that public packages support
anonymous pulls and GitHub does not publish a specific public pull limit. Ensure
the wording in the GHCR bullets stays aligned with the rest of the section and
cites only the confirmed limit details.
In `@services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.py`:
- Line 374: The latest_task selection in subprocess.py can raise a TypeError
when max() compares tz-aware timestamps against the naive datetime.datetime.min
fallback. Update the lambda used in the latest_task assignment to use a
timezone-aware sentinel that matches the timestamp types being compared, so
task.updated_at and task.created_at always fall back to a comparable aware
datetime.
In `@services/core/jobs/tests/controllers/test_subprocess_backend.py`:
- Around line 314-333: The test in
test_sync_uses_persisted_task_when_local_metadata_is_missing leaks a real
subprocess because _schedule_without_otel_export starts the sleep command and
_process_registry.pop removes tracking without stopping it. Update the test to
terminate the subprocess before removing the registry entry, or replace the
long-running command with a short-lived one; use _subprocess_backend,
_schedule_without_otel_export, and SubprocessProcessKey to locate the affected
setup.
In `@spec/caller-execution-hints-and-profile-plumbing-spec.md`:
- Around line 34-42: The spec currently embeds absolute /Users/... source links
that will break outside your workstation; replace them with repo-relative paths
or plain code references. Update the references in the caller execution hints
section to point to the relevant symbols like scheduler.py, add_job_routes(...),
and the Jobs schema for platform_spec without using machine-specific paths.
In `@spec/jobs-backed-local-run-ux-and-progressive-start-spec.md`:
- Line 14: The doc currently includes a machine-specific link to run_local(...)
in scheduler.py, which leaks local environment details and won’t work for
others. Update the reference in the spec to use either a repo-relative path or
plain code text, and keep the mention anchored to the run_local(...) path in the
scheduler module without any absolute local filesystem details.
In `@spec/jobs-local-remote-unification-spec.md`:
- Around line 35-37: The document currently uses machine-specific source links,
so update the related entries to use repo-relative paths instead of local
filesystem paths. Adjust the affected markdown in the spec so the references for
local/remote target selection, subprocess inheritance, and task/job storage
point to paths within the repository, using the existing link targets in this
spec as the guide.
In `@spec/jobs-runtime-availability-and-capabilities-spec.md`:
- Around line 30-34: Replace the absolute source links in the jobs runtime spec
with repo-relative paths or plain symbol references so they work outside your
workstation; update the listed references in the spec text to point at the
relevant symbols like validate_docker_available, the platform config downgrade
logic in config.py, Jobs config/profile construction, default backend config
selection, and the customization contributor instead of /Users/... paths.
In `@spec/nvapi-authentication-gateway-spec.md`:
- Line 450: The second Recommendation heading duplicates the earlier section
title and should be renamed to a unique section name. Update the repeated
markdown heading in the spec so it has a distinct title, preserving the
surrounding section content and keeping anchor generation and lint checks
unambiguous.
In `@spec/permissionless-authenticated-routes-spec.md`:
- Around line 50-88: Rewrite the options in this spec to use the same caller
taxonomy as the main authz spec: replace the `USER`/`WORKLOAD` terms in the
option descriptions with `ANON`/`PRINCIPAL`/`SERVICE_PRINCIPAL` so the
authenticated-route behavior is expressed consistently with
`plugin-service-authz-spec.md` and easier to compare across specs.
In `@spec/plugin-service-authz-spec.md`:
- Around line 132-140: Clarify the meaning of PathRule.scopes by explicitly
defining whether None and an empty list are equivalent or distinct, and update
the spec consistently so validation and serialization use the same
representation. Adjust the PathRule model description and any related authz
sections that reference scopes so the behavior is unambiguous across callers and
permissions handling.
In `@spec/service-role-visibility-and-bindability-spec.md`:
- Around line 9-15: The service authz spec should stay scoped to permissions,
service-scoped roles, and path rules, without implying role visibility or
bindability metadata is part of v1. Update the wording in the authz/spec
documentation to keep roles clearly as a separate follow-up track and avoid
attributing role metadata to the main spec, using the spec section that mentions
permissions, service-scoped roles, and path rules as the place to adjust.
In `@spec/subprocess-first-class-execution-resolution-spec.md`:
- Around line 33-37: The affected links are using machine-specific absolute
paths, so update the references in the spec to repo-relative paths instead.
Replace the /Users/... style link with the corresponding repository-relative
location for the jobs endpoint code, and keep the reference tied to the relevant
symbols around the Jobs API ingress rewrite and the run_local/submit_remote
execution paths so the link works for all contributors.
In
`@web/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/index.tsx`:
- Around line 72-85: The bulk delete flow in handleDeleteJobs is treating
missing workspace/name entries as a silent success and letting Promise.all
short-circuit on the first delete failure. Update handleDeleteJobs in
SafeSynthesizerJobsDataView to explicitly handle invalid jobs instead of
dropping them, and switch the mutation fan-out to Promise.allSettled so every
delete attempt completes and failures can be aggregated. Then surface a combined
error from deleteJobMutation.mutateAsync results rather than resolving success
when nothing was deleted.
In
`@web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/context/ClaudeCodeChatProvider.tsx`:
- Around line 83-86: `startNewChat` in `ClaudeCodeChatProvider` can leave
`loadStatus` stuck on `'loading'` when it clears `requestedSessionIdRef` before
an in-flight load finishes. Update `startNewChat` and the related load/reset
flow so any pending request still drives `setLoadStatus('idle')` on cancellation
or stale-guard exit, and make sure the status is cleared even when `handleReset`
is called while a session fetch is active.
---
Nitpick comments:
In `@process.md`:
- Around line 1-25: This memo needs a top-level heading and clearer sectioning
so it reads like a proper docs page instead of an internal note. Update the
markdown in process.md by adding a title and splitting the current freeform
narrative into concise sections that group the core ideas (for example, delivery
intake, timeline/milestones, cycle planning, ownership, process support, and
quality gates) while keeping the tone consistent with the existing proposal. Use
the existing topics in the body as the anchors for the new structure.
In `@spec/brian-auth-call.md`:
- Line 1: This file is a raw transcript dump under spec and should be replaced
with a concise, titled summary or moved out of spec entirely. Rewrite it as a
structured markdown note with sections for the auth goals, current state, gaps,
next steps, and follow-ups, using the same `spec/brian-auth-call` document as
the place to locate and update it. Keep only the actionable takeaways from the
conversation and remove the verbatim dialogue so the document matches the repo’s
markdown conventions and is easier to scan.
In `@spec/jobs-runtime-availability-and-capabilities-spec.md`:
- Around line 194-203: Clarify the freshness semantics for the availability
snapshot in the Jobs flow described in the spec: explicitly state whether the
set returned by Jobs is request-scoped, process-scoped, or must be revalidated
before dispatch. Update the section around the shared deterministic resolver and
the Jobs validation/dispatch steps to define when clients may cache the returned
availability and when they must refresh it, so the resolver behavior is
unambiguous.
In
`@web/packages/studio/src/components/evaluation/Configurations/ConfigurationDetailsPanel.tsx`:
- Around line 44-52: The ConfigurationDetailsPanel component is overriding
missing values with a different placeholder than the other eval panels. Update
the configName and configId handling in ConfigurationDetailsPanel so it passes
through undefined instead of forcing '-', letting KVPair apply its standard '—'
default for consistency with the Metric and LLMJudge panels.
In
`@web/packages/studio/src/routes/FilesetListRoute/DatasetBulkDeleteModal/index.tsx`:
- Around line 49-57: The `DatasetBulkDeleteModal` trigger uses an `any` cast in
the `cloneElement` call, which violates the typing guidelines and forces an
eslint disable. Update the `trigger` logic to type `slotTrigger` as a React
element with the injected `onClick` prop instead of `React.ReactElement<any>`,
and remove the eslint suppression. Keep in mind `cloneElement` will replace any
existing `onClick` on `slotTrigger`, so ensure the intended handler is the one
passed through `openTrigger`.
🪄 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: ba64c6b9-5435-41aa-bd8f-b81c3a63f842
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (133)
.github/workflows/docker-build-dispatch.yml.github/workflows/docker-build-liveness.yml.github/workflows/docker-build-trigger.ymlNOTICEcontainer.mddocker-bake.hcldocker/Dockerfile.bakedocker/base/Dockerfile.nmp-python-basedocs/about/release-notes/current-release.mdxdocs/set-up/config-reference.mdxe2e/configs/local-subprocess.yamle2e/conftest.pye2e/services_pool.pye2e/test_data_designer.pye2e/test_jobs.pye2e/test_jobs_auth.pyghcr-mirroring-issue.mdpackages/nemo_platform/pyproject.tomlpackages/nmp_testing/src/nmp/testing/__init__.pypackages/nmp_testing/src/nmp/testing/utils.pypackages/nmp_testing/tests/unit/test_e2e_harness.pyplans/2026-06-03-helm-chart-kube-e2e-plan.mdplans/2026-06-03-jobs-pause-resume-plan.mdplans/2026-06-03-jobs-persistent-storage-plan.mdplugins/nemo-guardrails/pyproject.tomlplugins/nemo-switchyard/pyproject.tomlplugins/nemo-switchyard/vendor/switchyard/pyproject.tomlprocess.mdpublic-images.mdpyproject.tomlpytest.iniscript/Untitledscript/run-e2e-linux.shscript/run-hello-world-jobs.shscript/submit-hello-world-job.shservices/core/jobs/src/nmp/core/jobs/api/v2/jobs/endpoints.pyservices/core/jobs/src/nmp/core/jobs/app/dispatcher.pyservices/core/jobs/src/nmp/core/jobs/config.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.pyservices/core/jobs/src/nmp/core/jobs/controllers/diagnostics.pyservices/core/jobs/src/nmp/core/jobs/controllers/reconciler.pyservices/core/jobs/src/nmp/core/jobs/controllers/scheduler.pyservices/core/jobs/tests/controllers/test_diagnostics.pyservices/core/jobs/tests/controllers/test_reconciler.pyservices/core/jobs/tests/controllers/test_scheduler.pyservices/core/jobs/tests/controllers/test_subprocess_backend.pyservices/core/jobs/tests/integration/test_task_auth_runtime.pyservices/core/jobs/tests/test_dispatcher_cross_workspace.pyservices/core/jobs/tests/test_timestamp_contracts.pyservices/studio/src/nmp/studio/coding_agent_artifacts.pyservices/studio/src/nmp/studio/coding_agent_mcp_tools.pyservices/studio/src/nmp/studio/coding_agents.pyservices/studio/tests/unit/test_coding_agents.pyspec/brian-auth-call-summary.mdspec/brian-auth-call.mdspec/caller-execution-hints-and-profile-plumbing-spec.mdspec/capability-vs-provider-execution-model-spec.mdspec/core-role-default-grants-spec.mdspec/daemon-group-local-jobs-follow-on-spec.mdspec/first-class-subprocess-provider-slack-draft.mdspec/jobs-backed-local-run-ux-and-progressive-start-spec.mdspec/jobs-local-remote-unification-spec.mdspec/jobs-runtime-availability-and-capabilities-spec.mdspec/machine-auth-authentication-and-authorization-spec.mdspec/nvapi-authentication-gateway-spec.mdspec/oidc-scope-and-claim-mapping-spec.mdspec/options-for-multi-idp.mdspec/permissionless-authenticated-routes-spec.mdspec/plugin-defined-scopes-spec.mdspec/plugin-service-authz-spec.mdspec/plugin-service-authz-ticket-description.mdspec/provider-backend-extensibility-spec.mdspec/service-role-visibility-and-bindability-spec.mdspec/subprocess-first-class-execution-resolution-spec.mdspec/trusted-probes-and-endpoints-spec.mdthird_party/licenses.jsonlthird_party/osv-licenses.jsonthird_party/requirements-main.txtweb/packages/common/src/components/form/ControlledSearchableSelect/index.tsxweb/packages/studio/AGENTS.mdweb/packages/studio/src/components/BulkDeleteModal/index.tsxweb/packages/studio/src/components/DatasetsTable/index.test.tsxweb/packages/studio/src/components/Layouts/GlobalNav/index.test.tsxweb/packages/studio/src/components/Layouts/GlobalNav/index.tsxweb/packages/studio/src/components/common/ReadOnlyField.tsxweb/packages/studio/src/components/dataViews/DataDesignerJobsDataView/DeleteJobModal.tsxweb/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsxweb/packages/studio/src/components/dataViews/ExperimentGroupDataView/index.tsxweb/packages/studio/src/components/dataViews/ExperimentGroupDataView/useExperimentGroupExperiments.test.tsweb/packages/studio/src/components/dataViews/ExperimentGroupDataView/useExperimentGroupExperiments.tsweb/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/DeleteJobModal.test.tsxweb/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/DeleteJobModal.tsxweb/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/index.tsxweb/packages/studio/src/components/evaluation/Configurations/ConfigurationDetailsPanel.tsxweb/packages/studio/src/components/evaluation/Configurations/LLMJudgeDisplay.tsxweb/packages/studio/src/components/evaluation/Configurations/MetricDisplay.tsxweb/packages/studio/src/components/evaluation/Configurations/TaskDisplay.tsxweb/packages/studio/src/components/evaluation/EvaluationModelSelect.tsxweb/packages/studio/src/components/filesets/FilesetFileExplorer/BulkDeleteModal/index.test.tsxweb/packages/studio/src/components/filesets/FilesetFileExplorer/BulkDeleteModal/index.tsxweb/packages/studio/src/components/filesets/FilesetFileExplorer/extractFilePathsFromDirectory.tsweb/packages/studio/src/index.cssweb/packages/studio/src/routes/DashboardLandingRoute/index.test.tsxweb/packages/studio/src/routes/DashboardLandingRoute/index.tsxweb/packages/studio/src/routes/FilesetListRoute/DatasetBulkDeleteModal/index.test.tsxweb/packages/studio/src/routes/FilesetListRoute/DatasetBulkDeleteModal/index.tsxweb/packages/studio/src/routes/FilesetListRoute/index.test.tsxweb/packages/studio/src/routes/PageLayout/index.tsxweb/packages/studio/src/routes/SafeSynthesizerJobReportRoute/components/ScorePanels/DataPrivacyPanel.test.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/ClaudeCodeChatThread.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/ClaudeCodeHistoryPanel.test.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/ClaudeCodeLayout.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/ClaudeCodeTopBarChat.test.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/ClaudeCodeTopBarChat.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/activeSessionStorage.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/api.test.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/api.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/artifacts.test.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/artifacts.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/context/ClaudeCodeChatProvider.test.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/context/ClaudeCodeChatProvider.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/context/useClaudeCodeChatContext.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/ArtifactSections.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/ClaudeCodeArtifactsPane.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/helpers.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/index.test.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/index.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/types.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/useClaudeCodeChatRuntime.test.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/useClaudeCodeChatRuntime.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/useCustomAssistantChatRuntime.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/util.test.tsweb/packages/studio/src/util/localStorage.ts
💤 Files with no reviewable changes (8)
- .github/workflows/docker-build-dispatch.yml
- web/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/DeleteJobModal.test.tsx
- web/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/DeleteJobModal.tsx
- web/packages/studio/src/components/common/ReadOnlyField.tsx
- .github/workflows/docker-build-liveness.yml
- .github/workflows/docker-build-trigger.yml
- web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/DeleteJobModal.tsx
- third_party/licenses.jsonl
|
|
||
| platform: | ||
| runtime: "none" | ||
| base_url: "http://0.0.0.0:8080" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use a client-reachable base URL.
Line 9 should use http://localhost:8080 (or a concrete host), not 0.0.0.0, to avoid client connection failures.
Proposed fix
- base_url: "http://0.0.0.0:8080"
+ base_url: "http://localhost:8080"📝 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.
Suggested change
| base_url: "http://0.0.0.0:8080" | |
| base_url: "http://localhost:8080" |
🤖 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 `@e2e/configs/local-subprocess.yaml` at line 9, The base_url in the
local-subprocess config is set to a non-client-reachable host, which can break
connections. Update the configuration entry for base_url in
local-subprocess.yaml to use a reachable address such as http://localhost:8080
instead of 0.0.0.0, keeping the change scoped to the config value.
Comment on lines
+33
to
+61
| cat > "${payload_file}" <<EOF | ||
| { | ||
| "workspace": "${WORKSPACE}", | ||
| "name": "${JOB_NAME}", | ||
| "source": "hello-world", | ||
| "spec": { | ||
| "message": "${MESSAGE}" | ||
| }, | ||
| "platform_spec": { | ||
| "steps": [ | ||
| { | ||
| "name": "hello-world", | ||
| "executor": { | ||
| "provider": "cpu", | ||
| "profile": "${EXECUTION_PROFILE}", | ||
| "container": { | ||
| "image": "${IMAGE}", | ||
| "entrypoint": ["nemo-platform"], | ||
| "command": ["run", "task", "--task", "nmp.hello_world.tasks.hello_world"] | ||
| } | ||
| }, | ||
| "config": { | ||
| "message": "${MESSAGE}" | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| EOF |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stop interpolating shell vars directly into JSON.
Lines 33-61 can emit invalid JSON when values contain quotes/newlines. Build JSON with a serializer.
Proposed fix
-payload_file="$(mktemp)"
-trap 'rm -f "${payload_file}"' EXIT
-
-cat > "${payload_file}" <<EOF
-{
- "workspace": "${WORKSPACE}",
- "name": "${JOB_NAME}",
- "source": "hello-world",
- "spec": {
- "message": "${MESSAGE}"
- },
- "platform_spec": {
- "steps": [
- {
- "name": "hello-world",
- "executor": {
- "provider": "cpu",
- "profile": "${EXECUTION_PROFILE}",
- "container": {
- "image": "${IMAGE}",
- "entrypoint": ["nemo-platform"],
- "command": ["run", "task", "--task", "nmp.hello_world.tasks.hello_world"]
- }
- },
- "config": {
- "message": "${MESSAGE}"
- }
- }
- ]
- }
-}
-EOF
+payload_file="$(mktemp)"
+trap 'rm -f "${payload_file}"' EXIT
+
+python3 - <<'PY' "${payload_file}" "${WORKSPACE}" "${JOB_NAME}" "${MESSAGE}" "${EXECUTION_PROFILE}" "${IMAGE}" "${PROJECT}"
+import json
+import sys
+
+path, workspace, job_name, message, profile, image, project = sys.argv[1:]
+payload = {
+ "workspace": workspace,
+ "name": job_name,
+ "source": "hello-world",
+ "spec": {"message": message},
+ "platform_spec": {
+ "steps": [
+ {
+ "name": "hello-world",
+ "executor": {
+ "provider": "cpu",
+ "profile": profile,
+ "container": {
+ "image": image,
+ "entrypoint": ["nemo-platform"],
+ "command": ["run", "task", "--task", "nmp.hello_world.tasks.hello_world"],
+ },
+ },
+ "config": {"message": message},
+ }
+ ]
+ },
+}
+if project:
+ payload["project"] = project
+with open(path, "w", encoding="utf-8") as f:
+ json.dump(payload, f)
+PY📝 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.
Suggested change
| cat > "${payload_file}" <<EOF | |
| { | |
| "workspace": "${WORKSPACE}", | |
| "name": "${JOB_NAME}", | |
| "source": "hello-world", | |
| "spec": { | |
| "message": "${MESSAGE}" | |
| }, | |
| "platform_spec": { | |
| "steps": [ | |
| { | |
| "name": "hello-world", | |
| "executor": { | |
| "provider": "cpu", | |
| "profile": "${EXECUTION_PROFILE}", | |
| "container": { | |
| "image": "${IMAGE}", | |
| "entrypoint": ["nemo-platform"], | |
| "command": ["run", "task", "--task", "nmp.hello_world.tasks.hello_world"] | |
| } | |
| }, | |
| "config": { | |
| "message": "${MESSAGE}" | |
| } | |
| } | |
| ] | |
| } | |
| } | |
| EOF | |
| payload_file="$(mktemp)" | |
| trap 'rm -f "${payload_file}"' EXIT | |
| python3 - <<'PY' "${payload_file}" "${WORKSPACE}" "${JOB_NAME}" "${MESSAGE}" "${EXECUTION_PROFILE}" "${IMAGE}" "${PROJECT}" | |
| import json | |
| import sys | |
| path, workspace, job_name, message, profile, image, project = sys.argv[1:] | |
| payload = { | |
| "workspace": workspace, | |
| "name": job_name, | |
| "source": "hello-world", | |
| "spec": {"message": message}, | |
| "platform_spec": { | |
| "steps": [ | |
| { | |
| "name": "hello-world", | |
| "executor": { | |
| "provider": "cpu", | |
| "profile": profile, | |
| "container": { | |
| "image": image, | |
| "entrypoint": ["nemo-platform"], | |
| "command": ["run", "task", "--task", "nmp.hello_world.tasks.hello_world"], | |
| }, | |
| }, | |
| "config": {"message": message}, | |
| } | |
| ] | |
| }, | |
| } | |
| if project: | |
| payload["project"] = project | |
| with open(path, "w", encoding="utf-8") as f: | |
| json.dump(payload, f) | |
| 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 `@script/submit-hello-world-job.sh` around lines 33 - 61, The JSON payload in
submit-hello-world-job.sh is being built by directly interpolating shell
variables, which can break when values contain quotes or newlines. Replace the
heredoc-based construction around the payload_file write with a proper JSON
serializer so WORKSPACE, JOB_NAME, MESSAGE, EXECUTION_PROFILE, and IMAGE are
safely escaped. Keep the existing payload structure and update the script’s
payload generation logic accordingly.
Comment on lines
+31
to
+39
| `OIDCConfig` models one provider: one `issuer`, one `client_id`, optional endpoint overrides, and one claim-mapping profile. See [packages/nmp_common/src/nmp/common/config/base.py](/Users/rsadler/src/nemo-platform/packages/nmp_common/src/nmp/common/config/base.py:89). | ||
|
|
||
| Relevant indicators: | ||
|
|
||
| - single `issuer`: [base.py](/Users/rsadler/src/nemo-platform/packages/nmp_common/src/nmp/common/config/base.py:97) | ||
| - single `client_id`: [base.py](/Users/rsadler/src/nemo-platform/packages/nmp_common/src/nmp/common/config/base.py:110) | ||
| - single `email_claim`, `groups_claim`, `subject_claim`: [base.py](/Users/rsadler/src/nemo-platform/packages/nmp_common/src/nmp/common/config/base.py:143) | ||
| - one nested `auth.oidc` object under `AuthConfig`: [base.py](/Users/rsadler/src/nemo-platform/packages/nmp_common/src/nmp/common/config/base.py:234) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace the absolute checkout paths.
These links point at /Users/rsadler/..., so they break in every other clone and leak a local filesystem path. Use repo-relative links or plain code references instead.
Also applies to: 703-718
🤖 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 `@spec/options-for-multi-idp.md` around lines 31 - 39, The spec currently links
to absolute local checkout paths under /Users/rsadler, which makes the
references break outside that machine and exposes a local filesystem path.
Update the references in this section to use repo-relative links or plain code
symbols for OIDCConfig, AuthConfig, and the related fields in base.py so the
links work in any clone.
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
9 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
DataDesignerJobsDataView/DeleteJobModal,SafeSynthesizerJobsDataView/DeleteJobModal,FilesetListRoute/DatasetBulkDeleteModal,FilesetFileExplorer/BulkDeleteModal) with a single genericBulkDeleteModal<T>atcomponents/BulkDeleteModal/.items,open,onDelete: (items: T[]) => Promise<void>,title: string | ((count: number) => string), andonClose.onDeletecallback — modal handles pending state, error display, and the confirmation UI viaDeleteConfirmationModal.BulkDeleteModal/utils.ts(extractFilePathsFromDirectory) relocated toFilesetFileExplorer/extractFilePathsFromDirectory.ts.Test plan
pnpm --filter nemo-studio-ui typecheck— passespnpm --filter nemo-studio-ui lint— passespnpm --filter nemo-studio-ui test src/components/DatasetsTable src/routes/FilesetListRoute src/components/filesets/FilesetFileExplorer src/components/dataViews/DataDesignerJobsDataView src/components/dataViews/SafeSynthesizerJobsDataView— 136 tests passSummary by CodeRabbit
New Features
Bug Fixes