feat(insights): add reproducible all-subject testbed workflow - #749
Merged
Conversation
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:
📝 WalkthroughWalkthroughThe PR moves Analyst client construction to callers, adds authenticated Intake support, introduces transactional ChangesInsights workflows
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🧹 Nitpick comments (3)
plugins/nemo-insights/tests/testbed/test_checked_in_insights.py (1)
20-21: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFilter
namesto analyzable subject types to matchanalyze allbehavior.
sorted(subjects)includes all registry subjects, but_analyze_allincli.pyonly runs subjects whosetypeisbenchmarkorintake. If a non-analyzable subject type is added later, this test would expect a YAML file thatanalyze allnever generates.♻️ Proposed fix
subjects = load_registry(cli.REGISTRY_PATH) - names = sorted(subjects) + names = sorted(name for name, subject in subjects.items() if subject.type in ("benchmark", "intake")) manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8"))🤖 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-insights/tests/testbed/test_checked_in_insights.py` around lines 20 - 21, Update the subject selection in the test setup around load_registry and names to retain only registry entries whose type is “benchmark” or “intake”, matching cli._analyze_all; then sort the filtered subject names so expectations exclude non-analyzable subjects.plugins/nemo-insights/testbed/reingest.py (1)
339-382: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUnnecessary
build_trace_request(candidate)call when span-count limit is the trigger.Line 372 builds the full candidate protobuf request unconditionally, but when
len(candidate) > max_spansis the sole flush trigger, theByteSize()check is never needed. Moving the protobuf build inside anelsebranch avoids redundant serialization on the span-count path.♻️ Proposed refactor
candidate = batch + [otlp] - candidate_request = build_trace_request(candidate) - if len(candidate) > max_spans or candidate_request.ByteSize() > max_bytes: + if len(candidate) > max_spans: requests.append(build_trace_request(batch)) _reject_oversized(otlp, doc) batch = [otlp] + elif build_trace_request(candidate).ByteSize() > max_bytes: + requests.append(build_trace_request(batch)) + _reject_oversized(otlp, doc) + batch = [otlp] else: batch = candidate🤖 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-insights/testbed/reingest.py` around lines 339 - 382, Update the batching logic in build_trace_requests so it checks len(candidate) > max_spans before constructing candidate_request; only build and measure candidate_request when the span-count limit is not exceeded, while preserving the existing flush and oversized-span handling.docs/superpowers/plans/2026-07-16-insights-testbed-pr66-port.md (1)
6-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftReorganize the changed documentation around the repository’s documentation contract.
docs/superpowers/plans/2026-07-16-insights-testbed-pr66-port.md#L6-L14,493-L495: classify as HOW-TO, put prerequisites first, addNext Steps, and replace product names with substitutions.plugins/nemo-insights/testbed/README.md#L12-L34,60-L67,104-L106,212-L218,378-L379: move HOW-TO, REFERENCE, and EXPLANATION additions to separate pages with cross-links; add top-level prerequisites andNext Steps.As per coding guidelines: “Each documentation page should fit ONE Diataxis quadrant,” “Always list prerequisites at the top,” “Include 'Next Steps' section,” and “Never hardcode product names.”
🤖 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 `@docs/superpowers/plans/2026-07-16-insights-testbed-pr66-port.md` around lines 6 - 14, Reorganize docs/superpowers/plans/2026-07-16-insights-testbed-pr66-port.md (lines 6-14 and 493-495) as a HOW-TO: put prerequisites first, replace hardcoded product names with substitutions, and add a Next Steps section. Rework plugins/nemo-insights/testbed/README.md (lines 12-34, 60-67, 104-106, 212-218, and 378-379) so each Diataxis quadrant’s content lives on its own linked page, with prerequisites at the top and a Next Steps section; avoid hardcoded product names throughout.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 `@docs/superpowers/plans/2026-07-16-insights-testbed-pr66-port.md`:
- Around line 74-75: Extend the test assertions after the status check to
validate the request’s Basic authorization header, using the existing captured
request data in seen and the expected authentication value configured by the
test. Keep the URL rewrite assertion unchanged and ensure the assertion verifies
the header rather than only the endpoint.
In `@plugins/nemo-insights/testbed/artifact.py`:
- Around line 132-151: The _basic_auth_intake_client_for function currently uses
the first basic-auth subject for all workspaces without validating the remaining
selected subjects. Before building the shared client, require every selected
subject to have the same auth mode, intake_path_prefix, auth_user_env, and
auth_password_env; reject mixed configurations, or separate exports by
configuration instead of reusing one client.
In `@plugins/nemo-insights/testbed/cli.py`:
- Around line 1114-1117: The no_check_in flow around _check_in_insights and
_write_insights_manifest must update the subject YAML and provenance manifest
transactionally. Stage both replacements first, then atomically promote them
together only after parsing, dependency hashing, and manifest writing succeed;
on any failure, roll back both files so baseline bytes and provenance remain
consistent.
- Around line 140-150: Update _check_in_insights to read and parse the Analyst
output as YAML before calling _atomic_write_text. Reject empty, malformed, or
non-mapping documents and require an insights list, exiting with a clear
validation error while leaving the existing destination unchanged; only write
the header and source content after validation succeeds.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-07-16-insights-testbed-pr66-port.md`:
- Around line 6-14: Reorganize
docs/superpowers/plans/2026-07-16-insights-testbed-pr66-port.md (lines 6-14 and
493-495) as a HOW-TO: put prerequisites first, replace hardcoded product names
with substitutions, and add a Next Steps section. Rework
plugins/nemo-insights/testbed/README.md (lines 12-34, 60-67, 104-106, 212-218,
and 378-379) so each Diataxis quadrant’s content lives on its own linked page,
with prerequisites at the top and a Next Steps section; avoid hardcoded product
names throughout.
In `@plugins/nemo-insights/testbed/reingest.py`:
- Around line 339-382: Update the batching logic in build_trace_requests so it
checks len(candidate) > max_spans before constructing candidate_request; only
build and measure candidate_request when the span-count limit is not exceeded,
while preserving the existing flush and oversized-span handling.
In `@plugins/nemo-insights/tests/testbed/test_checked_in_insights.py`:
- Around line 20-21: Update the subject selection in the test setup around
load_registry and names to retain only registry entries whose type is
“benchmark” or “intake”, matching cli._analyze_all; then sort the filtered
subject names so expectations exclude non-analyzable subjects.
🪄 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: 8b6ab93d-bcac-47c6-b86b-b515e6f2857e
⛔ Files ignored due to path filters (1)
plugins/nemo-insights/testbed/state.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
docs/superpowers/plans/2026-07-16-insights-testbed-pr66-port.mddocs/superpowers/specs/2026-07-16-insights-testbed-pr66-port-design.mdplugins/nemo-insights/src/nemo_insights_plugin/analyst/run.pyplugins/nemo-insights/src/nemo_insights_plugin/cli.pyplugins/nemo-insights/src/nemo_insights_plugin/jobs/analyze.pyplugins/nemo-insights/testbed/README.mdplugins/nemo-insights/testbed/adapters.pyplugins/nemo-insights/testbed/artifact.pyplugins/nemo-insights/testbed/cli.pyplugins/nemo-insights/testbed/export.pyplugins/nemo-insights/testbed/intake_client.pyplugins/nemo-insights/testbed/reingest.pyplugins/nemo-insights/testbed/tau2run.pyplugins/nemo-insights/testbed/testbeds.tomlplugins/nemo-insights/tests/test_analyst_run.pyplugins/nemo-insights/tests/test_cli_profile.pyplugins/nemo-insights/tests/test_periodic_analysis.pyplugins/nemo-insights/tests/testbed/test_adapters.pyplugins/nemo-insights/tests/testbed/test_artifact_snapshot_auth.pyplugins/nemo-insights/tests/testbed/test_checked_in_insights.pyplugins/nemo-insights/tests/testbed/test_cli.pyplugins/nemo-insights/tests/testbed/test_export.pyplugins/nemo-insights/tests/testbed/test_intake_client.pyplugins/nemo-insights/tests/testbed/test_registry.pyplugins/nemo-insights/tests/testbed/test_reingest.pyplugins/nemo-insights/tests/testbed/test_tau2run.py
Contributor
|
callingmedic911
force-pushed
the
port-insights-testbed/aditypandey
branch
2 times, most recently
from
July 17, 2026 20:05
a5e498d to
0ec6a83
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/nemo-insights/testbed/publish.py (1)
136-143: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPrevent concurrent publishers from clobbering immutable refs. Ref selection is not atomic, so unconditional overwrite can silently replace another publisher’s bundle.
plugins/nemo-insights/testbed/publish.py#L136-L143: reserve the ref or verify retry ownership before overwriting.plugins/nemo-insights/tests/testbed/test_publish.py#L163-L174: cover concurrent/stale-list collisions rather than requiring unconditional--clobber.🤖 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-insights/testbed/publish.py` around lines 136 - 143, Prevent concurrent publishers from overwriting immutable release refs: update the publish flow around release.next_ref, _ensure_release, and release._release_gh so a ref is reserved atomically or --clobber is allowed only when retry ownership is verified. Update plugins/nemo-insights/testbed/publish.py lines 136-143 accordingly, and extend plugins/nemo-insights/tests/testbed/test_publish.py lines 163-174 to cover stale-list/concurrent ref collisions without requiring unconditional --clobber.
🧹 Nitpick comments (2)
plugins/nemo-insights/testbed/README.md (2)
103-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse configured substitutions for product names.
Replace the hardcoded Platform and NeMo Optimizer names with the repository’s Sphinx substitutions.
As per coding guidelines, “Never hardcode product names; use substitutions in Sphinx configuration to maintain consistency.”
🤖 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-insights/testbed/README.md` around lines 103 - 105, Update the README text around the subject-registry ownership statement to replace the hardcoded Platform and NeMo Optimizer product names with the repository’s configured Sphinx substitution tokens, preserving the existing meaning and sentence structure.Source: Coding guidelines
12-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit this README by Diataxis type and move prerequisites first.
The page mixes quick-start, how-to, reference, architecture, and CI operations; prerequisites appear much later. Split these into focused pages with prerequisites at each page’s top.
As per coding guidelines, “Each documentation page should fit ONE Diataxis quadrant” and “Always list prerequisites at the top of documentation pages before other content.”
🤖 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-insights/testbed/README.md` around lines 12 - 36, Reorganize the testbed documentation into focused Diataxis pages, separating quick-start, how-to procedures, command reference, architecture, and CI/operational workflows. Move the prerequisites to the top of every resulting page before commands or other content, and update the README navigation or links so each topic remains discoverable without mixing quadrants.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-insights/testbed/release.py`:
- Around line 88-89: Update _release_missing to recognize only release-specific
“not found” errors, rather than any stderr containing “404” or “not found”;
preserve authentication and generic 404 failures so they propagate. Add a
regression test covering generic 404/authentication stderr and retain coverage
for a genuinely missing-release error.
- Around line 153-169: Update the download flow in the release helper to
download into a temporary directory, then atomically move the completed tarball
into the per-repository cache only after success; preserve cached-file reuse and
cleanup temporary artifacts on failure so interrupted downloads cannot be
trusted. Add coverage in plugins/nemo-insights/tests/testbed/test_release.py
lines 208-218 verifying a failed download leaves no reusable cache entry.
In `@plugins/nemo-insights/tests/testbed/test_cli.py`:
- Around line 1051-1109: Update the fake_analyze helper in
test_analyze_glamr_live_base_preserves_remote_auth to create the required
Insights output at out_path before returning. Preserve the existing “REPORT-OK”
return value so cli.main() reaches the authentication assertions.
In `@plugins/nemo-insights/tests/testbed/test_reingest.py`:
- Around line 260-275: Extend
test_ingest_bundle_oversized_span_raises_before_post to include a valid first
workspace and an oversized span in a later workspace, asserting no export
requests occur. Update ingest_bundle to prebuild and validate requests for every
workspace before posting or otherwise mutating state, preserving the
all-or-nothing behavior when any workspace exceeds the limit.
---
Outside diff comments:
In `@plugins/nemo-insights/testbed/publish.py`:
- Around line 136-143: Prevent concurrent publishers from overwriting immutable
release refs: update the publish flow around release.next_ref, _ensure_release,
and release._release_gh so a ref is reserved atomically or --clobber is allowed
only when retry ownership is verified. Update
plugins/nemo-insights/testbed/publish.py lines 136-143 accordingly, and extend
plugins/nemo-insights/tests/testbed/test_publish.py lines 163-174 to cover
stale-list/concurrent ref collisions without requiring unconditional --clobber.
---
Nitpick comments:
In `@plugins/nemo-insights/testbed/README.md`:
- Around line 103-105: Update the README text around the subject-registry
ownership statement to replace the hardcoded Platform and NeMo Optimizer product
names with the repository’s configured Sphinx substitution tokens, preserving
the existing meaning and sentence structure.
- Around line 12-36: Reorganize the testbed documentation into focused Diataxis
pages, separating quick-start, how-to procedures, command reference,
architecture, and CI/operational workflows. Move the prerequisites to the top of
every resulting page before commands or other content, and update the README
navigation or links so each topic remains discoverable without mixing quadrants.
🪄 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: ea0fb985-b86a-45fe-9392-993a369b9fec
⛔ Files ignored due to path filters (1)
plugins/nemo-insights/testbed/state.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.pyplugins/nemo-insights/src/nemo_insights_plugin/analyst/run.pyplugins/nemo-insights/src/nemo_insights_plugin/cli.pyplugins/nemo-insights/src/nemo_insights_plugin/contracts/profile.pyplugins/nemo-insights/src/nemo_insights_plugin/jobs/analyze.pyplugins/nemo-insights/src/nemo_insights_plugin/preflight.pyplugins/nemo-insights/testbed/README.mdplugins/nemo-insights/testbed/adapters.pyplugins/nemo-insights/testbed/artifact.pyplugins/nemo-insights/testbed/cli.pyplugins/nemo-insights/testbed/export.pyplugins/nemo-insights/testbed/intake_client.pyplugins/nemo-insights/testbed/publish.pyplugins/nemo-insights/testbed/reingest.pyplugins/nemo-insights/testbed/release.pyplugins/nemo-insights/testbed/tau2run.pyplugins/nemo-insights/testbed/testbeds.tomlplugins/nemo-insights/tests/contracts/test_profile_contract.pyplugins/nemo-insights/tests/test_analyst_run.pyplugins/nemo-insights/tests/test_cli_profile.pyplugins/nemo-insights/tests/test_periodic_analysis.pyplugins/nemo-insights/tests/test_preflight.pyplugins/nemo-insights/tests/testbed/test_adapters.pyplugins/nemo-insights/tests/testbed/test_artifact_snapshot_auth.pyplugins/nemo-insights/tests/testbed/test_checked_in_insights.pyplugins/nemo-insights/tests/testbed/test_cli.pyplugins/nemo-insights/tests/testbed/test_export.pyplugins/nemo-insights/tests/testbed/test_intake_client.pyplugins/nemo-insights/tests/testbed/test_publish.pyplugins/nemo-insights/tests/testbed/test_registry.pyplugins/nemo-insights/tests/testbed/test_reingest.pyplugins/nemo-insights/tests/testbed/test_release.pyplugins/nemo-insights/tests/testbed/test_tau2run.py
🚧 Files skipped from review as they are similar to previous changes (16)
- plugins/nemo-insights/src/nemo_insights_plugin/jobs/analyze.py
- plugins/nemo-insights/tests/testbed/test_checked_in_insights.py
- plugins/nemo-insights/testbed/intake_client.py
- plugins/nemo-insights/testbed/tau2run.py
- plugins/nemo-insights/testbed/testbeds.toml
- plugins/nemo-insights/testbed/export.py
- plugins/nemo-insights/tests/testbed/test_export.py
- plugins/nemo-insights/tests/testbed/test_artifact_snapshot_auth.py
- plugins/nemo-insights/src/nemo_insights_plugin/analyst/run.py
- plugins/nemo-insights/tests/testbed/test_tau2run.py
- plugins/nemo-insights/tests/test_analyst_run.py
- plugins/nemo-insights/tests/testbed/test_adapters.py
- plugins/nemo-insights/tests/testbed/test_registry.py
- plugins/nemo-insights/testbed/reingest.py
- plugins/nemo-insights/tests/testbed/test_intake_client.py
- plugins/nemo-insights/testbed/adapters.py
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/nemo-insights/testbed/publish.py (1)
136-143: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPrevent concurrent publishers from clobbering immutable refs. Ref selection is not atomic, so unconditional overwrite can silently replace another publisher’s bundle.
plugins/nemo-insights/testbed/publish.py#L136-L143: reserve the ref or verify retry ownership before overwriting.plugins/nemo-insights/tests/testbed/test_publish.py#L163-L174: cover concurrent/stale-list collisions rather than requiring unconditional--clobber.🤖 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-insights/testbed/publish.py` around lines 136 - 143, Prevent concurrent publishers from overwriting immutable release refs: update the publish flow around release.next_ref, _ensure_release, and release._release_gh so a ref is reserved atomically or --clobber is allowed only when retry ownership is verified. Update plugins/nemo-insights/testbed/publish.py lines 136-143 accordingly, and extend plugins/nemo-insights/tests/testbed/test_publish.py lines 163-174 to cover stale-list/concurrent ref collisions without requiring unconditional --clobber.
🧹 Nitpick comments (2)
plugins/nemo-insights/testbed/README.md (2)
103-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse configured substitutions for product names.
Replace the hardcoded Platform and NeMo Optimizer names with the repository’s Sphinx substitutions.
As per coding guidelines, “Never hardcode product names; use substitutions in Sphinx configuration to maintain consistency.”
🤖 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-insights/testbed/README.md` around lines 103 - 105, Update the README text around the subject-registry ownership statement to replace the hardcoded Platform and NeMo Optimizer product names with the repository’s configured Sphinx substitution tokens, preserving the existing meaning and sentence structure.Source: Coding guidelines
12-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit this README by Diataxis type and move prerequisites first.
The page mixes quick-start, how-to, reference, architecture, and CI operations; prerequisites appear much later. Split these into focused pages with prerequisites at each page’s top.
As per coding guidelines, “Each documentation page should fit ONE Diataxis quadrant” and “Always list prerequisites at the top of documentation pages before other content.”
🤖 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-insights/testbed/README.md` around lines 12 - 36, Reorganize the testbed documentation into focused Diataxis pages, separating quick-start, how-to procedures, command reference, architecture, and CI/operational workflows. Move the prerequisites to the top of every resulting page before commands or other content, and update the README navigation or links so each topic remains discoverable without mixing quadrants.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-insights/testbed/release.py`:
- Around line 88-89: Update _release_missing to recognize only release-specific
“not found” errors, rather than any stderr containing “404” or “not found”;
preserve authentication and generic 404 failures so they propagate. Add a
regression test covering generic 404/authentication stderr and retain coverage
for a genuinely missing-release error.
- Around line 153-169: Update the download flow in the release helper to
download into a temporary directory, then atomically move the completed tarball
into the per-repository cache only after success; preserve cached-file reuse and
cleanup temporary artifacts on failure so interrupted downloads cannot be
trusted. Add coverage in plugins/nemo-insights/tests/testbed/test_release.py
lines 208-218 verifying a failed download leaves no reusable cache entry.
In `@plugins/nemo-insights/tests/testbed/test_cli.py`:
- Around line 1051-1109: Update the fake_analyze helper in
test_analyze_glamr_live_base_preserves_remote_auth to create the required
Insights output at out_path before returning. Preserve the existing “REPORT-OK”
return value so cli.main() reaches the authentication assertions.
In `@plugins/nemo-insights/tests/testbed/test_reingest.py`:
- Around line 260-275: Extend
test_ingest_bundle_oversized_span_raises_before_post to include a valid first
workspace and an oversized span in a later workspace, asserting no export
requests occur. Update ingest_bundle to prebuild and validate requests for every
workspace before posting or otherwise mutating state, preserving the
all-or-nothing behavior when any workspace exceeds the limit.
---
Outside diff comments:
In `@plugins/nemo-insights/testbed/publish.py`:
- Around line 136-143: Prevent concurrent publishers from overwriting immutable
release refs: update the publish flow around release.next_ref, _ensure_release,
and release._release_gh so a ref is reserved atomically or --clobber is allowed
only when retry ownership is verified. Update
plugins/nemo-insights/testbed/publish.py lines 136-143 accordingly, and extend
plugins/nemo-insights/tests/testbed/test_publish.py lines 163-174 to cover
stale-list/concurrent ref collisions without requiring unconditional --clobber.
---
Nitpick comments:
In `@plugins/nemo-insights/testbed/README.md`:
- Around line 103-105: Update the README text around the subject-registry
ownership statement to replace the hardcoded Platform and NeMo Optimizer product
names with the repository’s configured Sphinx substitution tokens, preserving
the existing meaning and sentence structure.
- Around line 12-36: Reorganize the testbed documentation into focused Diataxis
pages, separating quick-start, how-to procedures, command reference,
architecture, and CI/operational workflows. Move the prerequisites to the top of
every resulting page before commands or other content, and update the README
navigation or links so each topic remains discoverable without mixing quadrants.
🪄 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: ea0fb985-b86a-45fe-9392-993a369b9fec
⛔ Files ignored due to path filters (1)
plugins/nemo-insights/testbed/state.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.pyplugins/nemo-insights/src/nemo_insights_plugin/analyst/run.pyplugins/nemo-insights/src/nemo_insights_plugin/cli.pyplugins/nemo-insights/src/nemo_insights_plugin/contracts/profile.pyplugins/nemo-insights/src/nemo_insights_plugin/jobs/analyze.pyplugins/nemo-insights/src/nemo_insights_plugin/preflight.pyplugins/nemo-insights/testbed/README.mdplugins/nemo-insights/testbed/adapters.pyplugins/nemo-insights/testbed/artifact.pyplugins/nemo-insights/testbed/cli.pyplugins/nemo-insights/testbed/export.pyplugins/nemo-insights/testbed/intake_client.pyplugins/nemo-insights/testbed/publish.pyplugins/nemo-insights/testbed/reingest.pyplugins/nemo-insights/testbed/release.pyplugins/nemo-insights/testbed/tau2run.pyplugins/nemo-insights/testbed/testbeds.tomlplugins/nemo-insights/tests/contracts/test_profile_contract.pyplugins/nemo-insights/tests/test_analyst_run.pyplugins/nemo-insights/tests/test_cli_profile.pyplugins/nemo-insights/tests/test_periodic_analysis.pyplugins/nemo-insights/tests/test_preflight.pyplugins/nemo-insights/tests/testbed/test_adapters.pyplugins/nemo-insights/tests/testbed/test_artifact_snapshot_auth.pyplugins/nemo-insights/tests/testbed/test_checked_in_insights.pyplugins/nemo-insights/tests/testbed/test_cli.pyplugins/nemo-insights/tests/testbed/test_export.pyplugins/nemo-insights/tests/testbed/test_intake_client.pyplugins/nemo-insights/tests/testbed/test_publish.pyplugins/nemo-insights/tests/testbed/test_registry.pyplugins/nemo-insights/tests/testbed/test_reingest.pyplugins/nemo-insights/tests/testbed/test_release.pyplugins/nemo-insights/tests/testbed/test_tau2run.py
🚧 Files skipped from review as they are similar to previous changes (16)
- plugins/nemo-insights/src/nemo_insights_plugin/jobs/analyze.py
- plugins/nemo-insights/tests/testbed/test_checked_in_insights.py
- plugins/nemo-insights/testbed/intake_client.py
- plugins/nemo-insights/testbed/tau2run.py
- plugins/nemo-insights/testbed/testbeds.toml
- plugins/nemo-insights/testbed/export.py
- plugins/nemo-insights/tests/testbed/test_export.py
- plugins/nemo-insights/tests/testbed/test_artifact_snapshot_auth.py
- plugins/nemo-insights/src/nemo_insights_plugin/analyst/run.py
- plugins/nemo-insights/tests/testbed/test_tau2run.py
- plugins/nemo-insights/tests/test_analyst_run.py
- plugins/nemo-insights/tests/testbed/test_adapters.py
- plugins/nemo-insights/tests/testbed/test_registry.py
- plugins/nemo-insights/testbed/reingest.py
- plugins/nemo-insights/tests/testbed/test_intake_client.py
- plugins/nemo-insights/testbed/adapters.py
🛑 Comments failed to post (4)
plugins/nemo-insights/testbed/release.py (2)
88-89: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not classify every 404 as a missing release.
Private-repository authentication failures can report
HTTP 404: Not Found; this now converts them into an empty release instead of surfacing the credential failure. Match release-specific errors and retain a regression test for generic 404/auth stderr.🤖 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-insights/testbed/release.py` around lines 88 - 89, Update _release_missing to recognize only release-specific “not found” errors, rather than any stderr containing “404” or “not found”; preserve authentication and generic 404 failures so they propagate. Add a regression test covering generic 404/authentication stderr and retain coverage for a genuinely missing-release error.
153-169: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Publish cached downloads atomically. An interrupted download can leave a file that subsequent calls incorrectly trust.
plugins/nemo-insights/testbed/release.py#L153-L169: download into a temporary directory and atomically move the completed asset into the cache.plugins/nemo-insights/tests/testbed/test_release.py#L208-L218: test that a failed download leaves no reusable cache entry.📍 Affects 2 files
plugins/nemo-insights/testbed/release.py#L153-L169(this comment)plugins/nemo-insights/tests/testbed/test_release.py#L208-L218🤖 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-insights/testbed/release.py` around lines 153 - 169, Update the download flow in the release helper to download into a temporary directory, then atomically move the completed tarball into the per-repository cache only after success; preserve cached-file reuse and cleanup temporary artifacts on failure so interrupted downloads cannot be trusted. Add coverage in plugins/nemo-insights/tests/testbed/test_release.py lines 208-218 verifying a failed download leaves no reusable cache entry.plugins/nemo-insights/tests/testbed/test_cli.py (1)
1051-1109: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Write the required Insights output in the fake analyst.
Line 1089 returns without creating
out_path, socli.main()exits with “Analyst did not write” before these auth assertions run.Proposed fix
) -> str: + out_path.write_text("insights: []\n", encoding="utf-8") return "REPORT-OK"📝 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.def test_analyze_glamr_live_base_preserves_remote_auth(monkeypatch, tmp_path): from testbed.adapters import IntakeAdapter, TestbedAdapter from testbed.registry import Subject glamr = Subject( "glamr", "intake", { "agent": "glamr", "workspace": "default", "base_url": "https://original.example", "auth": "basic", "intake_path_prefix": "/glamr/intake", "auth_user_env": "GLAMR_INTAKE_USER", "auth_password_env": "GLAMR_INTAKE_PASSWORD", }, ) monkeypatch.setattr(cli, "load_registry", lambda _path: {"glamr": glamr}) monkeypatch.setattr(cli, "_load_dotenv", lambda *args, **kwargs: None) monkeypatch.setattr(cli, "TMP", tmp_path) monkeypatch.setenv("INFERENCE_API_KEY", "sk-test") monkeypatch.setenv("GLAMR_INTAKE_USER", "intake-user") monkeypatch.setenv("GLAMR_INTAKE_PASSWORD", "secret") built: dict[str, object] = {} real_build_adapter = cli.build_adapter def capture_build_adapter(subject: Subject) -> TestbedAdapter: built.update(subject.config) return real_build_adapter(subject) async def fake_analyze( self: IntakeAdapter, *, record: dict[str, object] | None, since: datetime | None, verbose: bool, out_path: Path, ) -> str: out_path.write_text("insights: []\n", encoding="utf-8") return "REPORT-OK" monkeypatch.setattr(cli, "build_adapter", capture_build_adapter) monkeypatch.setattr(IntakeAdapter, "analyze", fake_analyze) monkeypatch.setattr( sys, "argv", ["testbed", "analyze", "glamr", "--live", "--base", "https://override.example"], ) cli.main() assert built["base_url"] == "https://override.example" assert {key: built[key] for key in cli._REMOTE_AUTH_KEYS} == { "auth": "basic", "intake_path_prefix": "/glamr/intake", "auth_user_env": "GLAMR_INTAKE_USER", "auth_password_env": "GLAMR_INTAKE_PASSWORD", }🤖 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-insights/tests/testbed/test_cli.py` around lines 1051 - 1109, Update the fake_analyze helper in test_analyze_glamr_live_base_preserves_remote_auth to create the required Insights output at out_path before returning. Preserve the existing “REPORT-OK” return value so cli.main() reaches the authentication assertions.plugins/nemo-insights/tests/testbed/test_reingest.py (1)
260-275: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Test oversized spans in a later workspace.
ingest_bundlebuilds requests inside the workspace loop. A valid first workspace can therefore be posted before an oversized span in the second workspace raises. Add a two-workspace test asserting zero exports, then prebuild all workspaces’ requests before any mutation.🧰 Tools
🪛 ast-grep (0.44.1)
[info] 261-261: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"payload": "x" * (5 * 1024 * 1024)})
Note: [CWE-116] Improper Encoding or Escaping of Output.(use-jsonify)
[warning] 268-268: Do not make http calls without encryption
Context: "http://x"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.(requests-http)
🤖 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-insights/tests/testbed/test_reingest.py` around lines 260 - 275, Extend test_ingest_bundle_oversized_span_raises_before_post to include a valid first workspace and an oversized span in a later workspace, asserting no export requests occur. Update ingest_bundle to prebuild and validate requests for every workspace before posting or otherwise mutating state, preserving the all-or-nothing behavior when any workspace exceeds the limit.
Capture the Platform-native boundaries and verification requirements before moving durable testbed behavior out of Optimizer. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Split the migration into test-first GLAMR, restore, baseline, and verification stages while preserving PR 718 boundaries. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Delay authenticated client ownership transfer until export, prevent stale analyze-all outputs from being promoted, and lock in restore and Tau2 boundary behavior. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Keep implementation-only artifacts in the Platform pull request. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Scope snapshot credentials per subject, promote single baseline updates transactionally, and fingerprint complete plugin behavior and resolved dependencies. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Keep CI-generated Insights as uploaded runtime artifacts and defer baseline promotion to the dedicated baseline workflow. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
callingmedic911
force-pushed
the
port-insights-testbed/aditypandey
branch
from
July 20, 2026 16:51
5812316 to
58f55fb
Compare
callingmedic911
commented
Jul 20, 2026
callingmedic911
commented
Jul 20, 2026
callingmedic911
commented
Jul 20, 2026
nicot
approved these changes
Jul 20, 2026
Rename the opt-out flag around its actual baseline semantics and restore the pinned NVQ intake subject for CI coverage. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
svvarom
approved these changes
Jul 20, 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
3 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.
testbed can now:
analyze allto analyze every pinned benchmark and Intake subject. Checked-in results update only when every run succeeds;--no-baseline-updatekeeps results as local/CI artifacts.Summary by CodeRabbit
analyze allfor transactional analysis and atomic promotion of complete Insights snapshots.--no-baseline-updateto generate outputs without updating checked-in artifacts.