fix(codeql): batch-5 cyclic-import, multiple-definition, unreachable

[marcusds]

Summary

Batch 5 of the CodeQL cleanup. Targets the error/warning-severity rules deferred from prior batches: py/unsafe-cyclic-import, py/multiple-definition, py/unreachable-statement. 22 alerts fixed in-tree, 14 dismissed (vendored, false-positives, defensive guards).

Branches off main after #91/#98/#100 merged; no overlap with the still-open #102 (batch-4) — files in flight there were intentionally excluded.

Alerts addressed

Severity	Rule	Fixed	Dismissed	How
Error	py/unsafe-cyclic-import	8	4	nemo-auditor: broke the sdk ⇄ sdk_resources import cycle by introducing AuditorResourceParent / AsyncAuditorResourceParent Protocols in sdk_resources/_parent.py and typing the sub-resource parent against them, instead of importing the concrete AuditorPluginResource under TYPE_CHECKING. The 4 vendored switchyard cycles were dismissed (snapshot, overwritten on re-vendor).
Warning	py/multiple-definition	11	—	Dropped dead pre-assignments later reassigned before use (tool_calling score init, pagination page_num, ray_bootstrap exit_code, e2e/evaluator loop bindings). In tests, dropped pointless bindings inside pytest.raises blocks and—where the dead response rebind in test_jobs_api masked a latent missing assertion—added the absent status_code == 201/200 checks.
Warning	py/unreachable-statement	3	10	Fixed: openapi_tools double-raise → raise … from e; parallelism/models always-true Priority-4 guard + dead return None; evaluator/datasets/utils dead trailing return. Dismissed: post-pytest.raises-block assertions CodeQL marks unreachable because it doesn't model __exit__ suppression (6), a defensive del in a finally (1), a type-checker-load-bearing terminal raise (1), a WIP stub (1), illustrative test scaffolding (1).

Notable

• Latent test gaps found: test_jobs_api created jobs 2 and 3 but never asserted the POST succeeded (the response was clobbered by the next GET). Added the missing assert response.status_code == 201.
• No # noqa left behind: the missing-precision test now asserts pydantic.ValidationError rather than a broad Exception (no B017 suppression).

Test plan

• uv run pytest plugins/nemo-auditor/tests/test_sdk_resources.py — 15 pass
• uv run pytest packages/nmp_common/tests/secrets packages/nemo_evaluator_sdk/tests services/core/jobs/tests/test_jobs_api.py services/intake/tests/test_entries.py — green (894 in the broad run)
• ruff check + ty check clean on all touched, non-excluded files
• auditor sdk import-cycle removed (verified import nemo_auditor.sdk + both sub-resource modules load)
• Re-run CodeQL after merge; confirm the 22 fixed alerts close

Dismissals

14 alerts dismissed via the Security tab with rationale (vendored switchyard, pytest.raises-suppression false positives, defensive finally/terminal-raise guards, one WIP stub). See per-alert comments.

Summary by CodeRabbit

• Refactor

  • Simplified internal flows and variable initialization across metrics, pagination, and parallelism handling.
  • Introduced generic parent interfaces for SDK resources to standardize resource architecture.
  • Streamlined bootstrap and exception-exit behavior.

• Behavior

  • Dataset resolution now enforces remote entity lookup and validates availability.

• Tests

  • Strengthened tests with explicit HTTP status assertions and improved validation error checks.

[github-actions]

Suite	Lines Covered	Line Rate	Branch Rate
Unit Tests	18407/24386	75.5%	61.9%
Integration Tests	11768/23163	50.8%	26.0%

[marcusds]

Inline notes on the rationale behind each change (CodeQL batch-5). Self-authored context, not blocking.

[marcusds]

@claude: Inline notes on the rationale behind each change (CodeQL batch-5). Self-authored context, not blocking.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 1c690fc3-dab9-4fa7-bab5-389d6f59ab28

📥 Commits

Reviewing files that changed from the base of the PR and between f2737c8 and 1b13b5a.

📒 Files selected for processing (1)

• packages/nmp_testing/src/nmp/testing/e2e/evaluator.py

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/nmp_testing/src/nmp/testing/e2e/evaluator.py

---

📝 Walkthrough

Walkthrough

PR refactors auditor SDK resources to use parent protocol abstraction, modernizes E2E test patterns with explicit type casting and smoke testing, and applies targeted cleanups across metric computation, pagination, validation assertions, error handling, and dataset retrieval.

Changes

Auditor SDK Parent Protocol Abstraction

Layer / File(s)	Summary
Parent protocol interface definitions plugins/nemo-auditor/src/nemo_auditor/sdk_resources/_parent.py	New module defines AuditorResourceParent and AsyncAuditorResourceParent protocol types with required _http_client attribute and _url(path: str) method.
Config resource parent type updates plugins/nemo-auditor/src/nemo_auditor/sdk_resources/configs.py	Config resource classes now accept parent protocol types instead of concrete plugin resource types; TYPE_CHECKING conditional import removed.
Target resource parent type updates plugins/nemo-auditor/src/nemo_auditor/sdk_resources/targets.py	Target resource classes updated to accept parent protocol types; import handling streamlined.

E2E Testing Refactoring & Type Safety

Layer / File(s)	Summary
Import and casting support packages/nmp_testing/src/nmp/testing/e2e/evaluator.py	Added cast import from typing for safe type casting of job status detail values.
Job outputs download smoke testing packages/nmp_testing/src/nmp/testing/e2e/evaluator.py	Generic results endpoint called as smoke test; dedicated typed accessors remain for aggregate and row scores.
Job completion verification with casting packages/nmp_testing/src/nmp/testing/e2e/evaluator.py	samples_processed explicitly cast to int before assertions in positivity checks and row-count comparisons.
Dataset fileset creation refactoring packages/nmp_testing/src/nmp/testing/e2e/evaluator.py	create_kwargs dictionary removed; replaced with explicit if schema is not None / else branching to conditionally attach metadata.

Scattered Metric, Validation & Utility Cleanups

Layer / File(s)	Summary
Initialization and logic simplifications packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/metrics/tool_calling.py, packages/nemo_platform_ext/src/nemo_platform_ext/cli/core/pagination.py, services/core/models/src/nmp/core/models/parallelism/models.py, services/customizer/src/nmp/customizer/tasks/training/backends/nemo_rl/ray_bootstrap.py	Metric removes early default fn_name_and_args_accuracy_score initialization; pagination introduces tracked counters and removes standalone page_num; Mamba config detection simplified; Ray bootstrap removes pre-try exit_code default.
Test validation modernization packages/nmp_common/tests/secrets/test_secret_key.py, services/core/models/tests/unit/parallelism/test_parallelism_models.py, services/core/jobs/tests/test_jobs_api.py	Secret key test removes intermediate variable assignment; parallelism test uses pytest.raises(ValidationError); jobs test adds HTTP status code assertions.
Error handling chaining and bootstrap cleanup script/openapi_helper/openapi_tools.py, services/customizer/src/nmp/customizer/tasks/training/backends/nemo_rl/ray_bootstrap.py	OpenAPI error handler chains exception cause; Ray bootstrap removes pre-try exit_code default initialization.
Dataset retrieval enhancement services/evaluator/src/nmp/evaluator/app/datasets/nmp_datasets/utils.py	Dataset utility now fetches metadata from configured HTTP endpoint and constructs a Dataset from the response for string/URN identifiers.

Suggested reviewers

• mckornfield
• svvarom

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 54.17% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main objective: batch-5 CodeQL fixes addressing three specific linting rules (cyclic-import, multiple-definition, unreachable-statement).
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-codeql-issues-batch-5/mschwab

---

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

plugins/nemo-auditor/src/nemo_auditor/sdk_resources/_parent.py (1)

18-35: 💤 Low value

Consider adding __all__ for consistency.

Both configs.py and targets.py define __all__. Adding it here clarifies the public interface.

Suggested addition

 class AsyncAuditorResourceParent(Protocol):
     """Async parent surface used by sub-resources."""
 
     _http_client: httpx.AsyncClient
 
     def _url(self, path: str) -> str:
         """Build the absolute request URL for ``path``."""
         raise NotImplementedError
+
+
+__all__ = ["AuditorResourceParent", "AsyncAuditorResourceParent"]

🤖 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 `@plugins/nemo-auditor/src/nemo_auditor/sdk_resources/_parent.py` around lines
18 - 35, Add a module-level __all__ to explicitly declare the public API for
this file; include the protocol names "AuditorResourceParent" and
"AsyncAuditorResourceParent" in the list so imports and tooling are consistent
with configs.py and targets.py. Update the top-level exports by adding __all__ =
["AuditorResourceParent", "AsyncAuditorResourceParent"] near the class
definitions to make the public interface explicit.

🤖 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 `@packages/nmp_testing/src/nmp/testing/e2e/evaluator.py`:
- Around line 166-167: The code uses a string literal type in the cast call -
replace cast("int", ...) with cast(int, ...) for the samples_processed
assignment and the similar occurrence around line 179; update the two calls that
cast outputs.job_status.status_details.get("samples_processed", 0) to use
cast(int, ...) so the concrete int type is used (refer to the samples_processed
variable and the cast(...) calls on outputs.job_status.status_details).

---

Nitpick comments:
In `@plugins/nemo-auditor/src/nemo_auditor/sdk_resources/_parent.py`:
- Around line 18-35: Add a module-level __all__ to explicitly declare the public
API for this file; include the protocol names "AuditorResourceParent" and
"AsyncAuditorResourceParent" in the list so imports and tooling are consistent
with configs.py and targets.py. Update the top-level exports by adding __all__ =
["AuditorResourceParent", "AsyncAuditorResourceParent"] near the class
definitions to make the public interface explicit.

🪄 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: 17af8ec3-6114-4a4e-ab4f-bbb96de49251

📥 Commits

Reviewing files that changed from the base of the PR and between 99af5c1 and bbbd4d6.

⛔ Files ignored due to path filters (2)

• sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/tool_calling.py is excluded by !sdk/**
• sdk/python/nemo-platform/src/nemo_platform/cli/core/pagination.py is excluded by !sdk/**

📒 Files selected for processing (14)

• packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/metrics/tool_calling.py
• packages/nemo_platform_ext/src/nemo_platform_ext/cli/core/pagination.py
• packages/nmp_common/tests/secrets/test_secret_key.py
• packages/nmp_testing/src/nmp/testing/e2e/evaluator.py
• plugins/nemo-auditor/src/nemo_auditor/sdk_resources/_parent.py
• plugins/nemo-auditor/src/nemo_auditor/sdk_resources/configs.py
• plugins/nemo-auditor/src/nemo_auditor/sdk_resources/targets.py
• script/openapi_helper/openapi_tools.py
• services/core/jobs/tests/test_jobs_api.py
• services/core/models/src/nmp/core/models/parallelism/models.py
• services/core/models/tests/unit/parallelism/test_parallelism_models.py
• services/customizer/src/nmp/customizer/tasks/training/backends/nemo_rl/ray_bootstrap.py
• services/evaluator/src/nmp/evaluator/app/datasets/nmp_datasets/utils.py
• services/intake/tests/test_entries.py

💤 Files with no reviewable changes (4)

• services/customizer/src/nmp/customizer/tasks/training/backends/nemo_rl/ray_bootstrap.py
• packages/nemo_platform_ext/src/nemo_platform_ext/cli/core/pagination.py
• packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/metrics/tool_calling.py
• services/evaluator/src/nmp/evaluator/app/datasets/nmp_datasets/utils.py