feat(jobs): add auth-aware E2E tests and job diagnostics infrastructure - #398
Merged
Conversation
ironcommit
force-pushed
the
auth-tests-2/rsadler
branch
2 times, most recently
from
June 23, 2026 00:56
7fc1e79 to
224fc1e
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds workspace-scoped task listing to ChangesJob Auth E2E and Diagnostics
Sequence DiagramsequenceDiagram
participant Test as E2E Test Module
participant Services as Pooled Services
participant Auth as Auth Subsystem
participant Dispatcher as JobDispatcher
participant Diagnostics as Diagnostics Logger
Test->>Services: request base_url (config pooled by hash)
alt auth_enabled in config
Services-->>Test: base_url + requires auth headers
Test->>Auth: generate unsigned JWT
Test->>Test: inject Authorization header
else auth disabled
Services-->>Test: base_url
end
Test->>Dispatcher: list_tasks(step_id, workspace=workspace)
Dispatcher-->>Test: scoped tasks
Test->>Services: submit job with workspace context
alt job fails
Services->>Diagnostics: log_job_diagnostics_if_debug(context)
Diagnostics-->>Services: snapshot (status, steps, tasks, logs)
end
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
services/core/jobs/tests/integration/test_task_auth_runtime.py (1)
33-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a concrete type for
http_clientin the protocol and implementation.
http_clientis currently untyped, which weakens static checks for this helper.Proposed change
+import httpx from typing import Protocol class _SecretAccessTask(Protocol): - def run(self, *, http_client) -> str: ... + def run(self, *, http_client: httpx.Client) -> str: ... @@ class _Task: `@staticmethod` - def run(*, http_client) -> str: + def run(*, http_client: httpx.Client) -> str:As per coding guidelines,
**/*.py: "Always prefer concrete type hints over string-based ones in Python code; do not import types under TYPE_CHECKING, instead import types as regular imports when possible".🤖 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/integration/test_task_auth_runtime.py` around lines 33 - 40, The `http_client` parameter in both the `_SecretAccessTask` protocol's `run` method and the `_Task` class's `run` method in `_secret_access_task_module` function lacks a concrete type hint. Add a concrete type annotation to the `http_client` parameter in both locations using the appropriate HTTP client type. Import the HTTP client type as a regular import at the top of the file rather than under TYPE_CHECKING to comply with the coding guidelines and enable proper static type checking.Source: Coding guidelines
services/core/jobs/src/nmp/core/jobs/controllers/diagnostics.py (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove postponed annotations from this module.
from __future__ import annotationsis unnecessary here and makes runtime annotations string-based.As per coding guidelines,
**/*.py: Always prefer concrete type hints over string-based ones in Python code.Suggested change
-from __future__ import annotations🤖 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/diagnostics.py` at line 4, Remove the `from __future__ import annotations` import statement from the top of the diagnostics module as it converts type hints to strings at runtime, which violates the coding guideline that mandates concrete type hints over string-based annotations. Simply delete this import line from the imports section.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 `@e2e/conftest.py`:
- Around line 363-373: The teardown logic in the process termination loop does
not guard against processes that have already exited before teardown reaches
them, which can cause terminate() to raise an exception. Before calling
services.proc.terminate(), check if the process has already exited using
services.proc.poll() which returns None if the process is still running or a
return code if it has exited. Skip the termination and wait logic for processes
that have already exited to avoid raising exceptions during fixture teardown.
In `@e2e/test_data_designer.py`:
- Around line 15-16: The `run_nemo_local` function call around lines 189-199
does not check the subprocess result for failures. When the CLI exits with a
non-zero status code, the test continues executing instead of failing
immediately, causing later assertions to fail with misleading context. Capture
the return value from `run_nemo_local` and add a check to verify it succeeded
(exit code 0). If it failed, raise an exception with a clear error message
immediately so the test fails fast with proper context about the actual CLI
failure.
In `@services/core/jobs/src/nmp/core/jobs/controllers/diagnostics.py`:
- Around line 123-125: The code directly includes raw user-controlled job log
messages (entry.message) in the diagnostics dictionary, which can leak secrets
and PII when the diagnostics are logged via logger.debug. To fix this, sanitize
or filter the log entries before adding them to diagnostics["job_logs"] instead
of directly copying entry.message. Consider either redacting sensitive patterns
from the messages, limiting the log content to non-sensitive fields, or
replacing the raw messages with generic indicators. Apply the same sanitization
approach to all occurrences where job logs are added to diagnostics, including
the similar code referenced at lines 152-160.
---
Nitpick comments:
In `@services/core/jobs/src/nmp/core/jobs/controllers/diagnostics.py`:
- Line 4: Remove the `from __future__ import annotations` import statement from
the top of the diagnostics module as it converts type hints to strings at
runtime, which violates the coding guideline that mandates concrete type hints
over string-based annotations. Simply delete this import line from the imports
section.
In `@services/core/jobs/tests/integration/test_task_auth_runtime.py`:
- Around line 33-40: The `http_client` parameter in both the `_SecretAccessTask`
protocol's `run` method and the `_Task` class's `run` method in
`_secret_access_task_module` function lacks a concrete type hint. Add a concrete
type annotation to the `http_client` parameter in both locations using the
appropriate HTTP client type. Import the HTTP client type as a regular import at
the top of the file rather than under TYPE_CHECKING to comply with the coding
guidelines and enable proper static type checking.
🪄 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: a83518d1-5a7e-4e59-9ae0-cf7ac1d59374
📒 Files selected for processing (17)
e2e/configs/local-subprocess.yamle2e/conftest.pye2e/test_data_designer.pye2e/test_jobs.pye2e/test_jobs_auth.pypackages/nmp_testing/src/nmp/testing/__init__.pypackages/nmp_testing/src/nmp/testing/utils.pypytest.iniservices/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/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_reconciler.pyservices/core/jobs/tests/controllers/test_scheduler.pyservices/core/jobs/tests/integration/test_task_auth_runtime.pyservices/core/jobs/tests/test_dispatcher_cross_workspace.py
Contributor
|
ironcommit
force-pushed
the
auth-tests-2/rsadler
branch
from
June 23, 2026 16:25
224fc1e to
bc2a2e5
Compare
Contributor
|
🌿 Preview your docs: https://nvidia-preview-auth-tests-2-rsadler.docs.buildwithfern.com/nemo-platform |
ironcommit
force-pushed
the
auth-tests-2/rsadler
branch
from
June 23, 2026 17:30
bc2a2e5 to
4610ba5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e/conftest.py (2)
82-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse concrete Python type hints.
These string-based annotations violate the repo rule; move the stash key below
_E2EServicesPooland annotatepytest.Nodedirectly.Proposed fix
-_services_pool_manager_key = pytest.StashKey["_E2EServicesPool"]() - ... -def _resolve_e2e_config_layers_from_node(node: "pytest.Node") -> list[str | dict[str, Any]]: +def _resolve_e2e_config_layers_from_node(node: pytest.Node) -> list[str | dict[str, Any]]: ... -def _load_effective_e2e_config_from_node(node: "pytest.Node") -> tuple[list[Path], dict[str, Any]]: +def _load_effective_e2e_config_from_node(node: pytest.Node) -> tuple[list[Path], dict[str, Any]]:class _E2EServicesPool: ... services.proc.wait(timeout=5) +_services_pool_manager_key = pytest.StashKey[_E2EServicesPool]() + + def _services_log_dir(tmp_path_factory: pytest.TempPathFactory) -> Path:As per coding guidelines, "
**/*.py: Always prefer concrete type hints over string-based ones in Python code; do not import types under TYPE_CHECKING, instead import types as regular imports when possible."Also applies to: 297-297, 362-362
🤖 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/conftest.py` at line 82, The variable _services_pool_manager_key at line 82 uses a string-based type annotation "_E2EServicesPool" which violates the concrete type hints requirement. Remove the quotes around _E2EServicesPool and ensure the _E2EServicesPool class or type is defined and imported before the pytest.StashKey declaration so you can reference it directly without string quotes. Additionally, import pytest.Node directly and use it as a concrete type hint instead of as a string in the same line and in the other occurrences at lines 297 and 362. This ensures all type annotations use concrete types rather than string-based forward references.Source: Coding guidelines
341-351: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDo not pass the canonicalized config to services.
Line 383 returns normalized config, and
_materialize_config_pathwrites that same data, sojobs.executorscan be reordered before the platform starts. Keep normalization for hashing only.Proposed fix
- return resolved_paths, _normalize_config(effective_config) + return resolved_paths, effective_configAlso applies to: 383-383
🤖 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/conftest.py` around lines 341 - 351, The normalization logic in the code that sorts items for the jobs.executors path should only be used for hashing and comparison purposes, not for modifying the actual configuration passed to services. Currently, the normalized (sorted) version is being returned and used by _materialize_config_path, which causes the executors to be reordered before the platform starts. Keep the normalization logic intact for sorting/hashing, but return the original unsorted value instead of the normalized version so that the configuration maintains its original order when passed to services.
🤖 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 `@services/core/jobs/src/nmp/core/jobs/controllers/scheduler.py`:
- Around line 155-157: The conflict handling logic in the scheduler is too
permissive for RESUMING steps. When a PENDING update conflicts and retrieve()
still returns RESUMING, the code silently ignores the failure instead of
detecting that the step is stuck. Fix this by modifying the conflict handling to
check if the persisted status has actually advanced from the originally fetched
status before ignoring the conflict. After catching a conflict exception, fetch
the current status again and compare it with the status that was initially
retrieved. Only ignore the conflict when the persisted status has progressed
beyond the RESUMING state that was found, not when it remains RESUMING. Apply
this logic to both the filter_params section around line 155 and the conflict
handling block mentioned in lines 193-203.
---
Nitpick comments:
In `@e2e/conftest.py`:
- Line 82: The variable _services_pool_manager_key at line 82 uses a
string-based type annotation "_E2EServicesPool" which violates the concrete type
hints requirement. Remove the quotes around _E2EServicesPool and ensure the
_E2EServicesPool class or type is defined and imported before the
pytest.StashKey declaration so you can reference it directly without string
quotes. Additionally, import pytest.Node directly and use it as a concrete type
hint instead of as a string in the same line and in the other occurrences at
lines 297 and 362. This ensures all type annotations use concrete types rather
than string-based forward references.
- Around line 341-351: The normalization logic in the code that sorts items for
the jobs.executors path should only be used for hashing and comparison purposes,
not for modifying the actual configuration passed to services. Currently, the
normalized (sorted) version is being returned and used by
_materialize_config_path, which causes the executors to be reordered before the
platform starts. Keep the normalization logic intact for sorting/hashing, but
return the original unsorted value instead of the normalized version so that the
configuration maintains its original order when passed to services.
🪄 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: 7e53a7d9-e9e1-446c-94b1-e270f2333901
📒 Files selected for processing (20)
docs/set-up/config-reference.mdxe2e/configs/local-subprocess.yamle2e/conftest.pye2e/test_data_designer.pye2e/test_jobs.pye2e/test_jobs_auth.pypackages/nmp_testing/src/nmp/testing/__init__.pypackages/nmp_testing/src/nmp/testing/utils.pypytest.iniservices/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/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/integration/test_task_auth_runtime.pyservices/core/jobs/tests/test_dispatcher_cross_workspace.py
✅ Files skipped from review due to trivial changes (4)
- e2e/test_jobs.py
- pytest.ini
- docs/set-up/config-reference.mdx
- packages/nmp_testing/src/nmp/testing/init.py
🚧 Files skipped from review as they are similar to previous changes (13)
- services/core/jobs/src/nmp/core/jobs/api/v2/jobs/endpoints.py
- services/core/jobs/tests/controllers/test_diagnostics.py
- services/core/jobs/src/nmp/core/jobs/app/dispatcher.py
- services/core/jobs/tests/controllers/test_reconciler.py
- e2e/test_data_designer.py
- services/core/jobs/tests/test_dispatcher_cross_workspace.py
- e2e/configs/local-subprocess.yaml
- services/core/jobs/src/nmp/core/jobs/config.py
- services/core/jobs/tests/integration/test_task_auth_runtime.py
- e2e/test_jobs_auth.py
- packages/nmp_testing/src/nmp/testing/utils.py
- services/core/jobs/src/nmp/core/jobs/controllers/reconciler.py
- services/core/jobs/src/nmp/core/jobs/controllers/diagnostics.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
services/core/jobs/tests/controllers/test_subprocess_backend.py (2)
341-351: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for stale pending metadata path.
This suite tests only the non-stale pending branch. Add a case where
updated_at/created_atis older than the grace window and assertsync()returnsERROR.🤖 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 341 - 351, Add a new test function to cover the stale pending metadata path where the step has been pending for longer than the grace window. Create a test that sets up a pending step with updated_at and created_at timestamps older than the configured grace period, calls backend.sync() on that step, and asserts that the returned update status is ERROR rather than PENDING. This complements the existing test_sync_keeps_recent_pending_step_pending_when_local_metadata_is_missing function which only covers the non-stale pending case.
319-322: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid spawning unmanaged
sleep 10in this unit test.Line 319 starts a real process, and Line 321 drops its registry handle, so backend cleanup can’t terminate it. Build the missing-metadata scenario without launching a live subprocess.
Suggested simplification
- _schedule_without_otel_export(backend, step) - key = SubprocessProcessKey(step.workspace, step.job, str(step.attempt_id), step.name) - backend._process_registry.pop(key) + # Keep local registry empty to simulate missing metadata.🤖 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 319 - 322, The test spawns a real subprocess with `_schedule_without_otel_export(backend, step)` and then removes it from the process registry via `backend._process_registry.pop(key)`, leaving an unmanaged subprocess that cannot be cleaned up by the backend. Instead of launching a live subprocess to test the missing-metadata scenario, mock the necessary components and process state to simulate the condition without actually spawning a subprocess, ensuring proper resource cleanup during test execution.
🤖 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 `@services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.py`:
- Around line 384-390: The `_pending_step_missing_metadata_is_stale` function
assumes that `anchor` (derived from `step.updated_at` or `step.created_at`) is a
datetime object, but it can be a string timestamp. Before performing datetime
arithmetic operations with `datetime.timedelta` on the `anchor` variable, check
if it is a string and parse it into a datetime object if necessary. This will
prevent type errors when the code path receives string timestamps and metadata
is missing.
- Around line 361-376: The code accesses tasks.data as an attribute on lines 373
and 376, but NemoResponse.data is a method that requires parentheses to be
invoked, not a property. Change tasks.data to tasks.data() in both locations:
the conditional check if not tasks.data() and in the max() function call with
tasks.data(). Also apply the same fix to the other locations mentioned:
diagnostics.py:119, base.py:306, and kubernetes_job.py:378 where this pattern
appears.
---
Nitpick comments:
In `@services/core/jobs/tests/controllers/test_subprocess_backend.py`:
- Around line 341-351: Add a new test function to cover the stale pending
metadata path where the step has been pending for longer than the grace window.
Create a test that sets up a pending step with updated_at and created_at
timestamps older than the configured grace period, calls backend.sync() on that
step, and asserts that the returned update status is ERROR rather than PENDING.
This complements the existing
test_sync_keeps_recent_pending_step_pending_when_local_metadata_is_missing
function which only covers the non-stale pending case.
- Around line 319-322: The test spawns a real subprocess with
`_schedule_without_otel_export(backend, step)` and then removes it from the
process registry via `backend._process_registry.pop(key)`, leaving an unmanaged
subprocess that cannot be cleaned up by the backend. Instead of launching a live
subprocess to test the missing-metadata scenario, mock the necessary components
and process state to simulate the condition without actually spawning a
subprocess, ensuring proper resource cleanup during test execution.
🪄 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: 32244c68-414d-4e86-b260-ea79bdc877b9
📒 Files selected for processing (3)
services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.pyservices/core/jobs/tests/controllers/test_scheduler.pyservices/core/jobs/tests/controllers/test_subprocess_backend.py
🚧 Files skipped from review as they are similar to previous changes (1)
- services/core/jobs/tests/controllers/test_scheduler.py
mckornfield
approved these changes
Jun 23, 2026
ironcommit
force-pushed
the
auth-tests-2/rsadler
branch
2 times, most recently
from
June 23, 2026 19:57
fca1e87 to
2023793
Compare
ironcommit
enabled auto-merge
June 23, 2026 21:23
ironcommit
disabled auto-merge
June 23, 2026 21:26
ironcommit
force-pushed
the
auth-tests-2/rsadler
branch
2 times, most recently
from
June 23, 2026 21:34
74d2dd0 to
b62e8ab
Compare
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>
ironcommit
force-pushed
the
auth-tests-2/rsadler
branch
from
June 23, 2026 21:47
b62e8ab to
db3a5e7
Compare
ironcommit
enabled auto-merge
June 23, 2026 21:48
mikeknep
pushed a commit
that referenced
this pull request
Jun 24, 2026
…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>
matthewgrossman
pushed a commit
that referenced
this pull request
Jun 24, 2026
…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> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
This was referenced Jun 24, 2026
This was referenced Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add auth-aware E2E tests that validate workspace isolation and principal propagation under an auth-enabled platform config. Introduce a reusable
diagnosticsmodule for structured job/step/task state collection on errors, and fix a workspace-scoping bug inlist_tasks.Changes
Auth-aware E2E test suite (
e2e/test_jobs_auth.py)Per-module platform config support (
e2e/conftest.py)pytest.mark.e2e_config(...)marker for declaring ordered config file layers and inline dict overlays per test module_servicesfixture with a module-scoped_services_instancebacked by a session-scoped poolnemo_runfixture; replace withrun_nemo_localfromnmp.testingJob diagnostics module (
services/core/jobs/.../controllers/diagnostics.py)collect_job_diagnostics(): fetches job, step, task, status, and log data via the SDK and returns a structured dictlog_job_diagnostics_if_debug(): conditionally emits diagnostics at DEBUG levelBug fix: workspace-scoped task listing
workspacethroughdispatcher.list_tasks()tostore.list()so task queries for non-default workspaces return correct resultstest_list_tasks_uses_step_workspace_for_non_default_jobs)Type safety improvements in scheduler/reconciler
PlatformJobStepsListFilterParamandSDKPlatformJobStatusinstead of raw dicts/strings.valuefor enum status fields to match SDK expectationsassert step.step_spec is not Noneguard before accessing executorOther
e2e/configs/local-subprocess.yamlE2E config (runtime: none, subprocess executor)run_nemo_localnow acceptsbase_urlandworkspacekeyword argumentse2e_configas a custom pytest marker inpytest.initest_task_auth_runtime.pyto use a Protocol-based task stub returning the secret value directly instead of capturing stdoutSummary by CodeRabbit
include_job_logs_in_diagnosticsto optionally include raw job logs in controller diagnostics.