Skip to content

fix(insights): follow ups from the profile analysis review - #752

Merged
callingmedic911 merged 1 commit into
mainfrom
fix/insights-profile-followups
Jul 17, 2026
Merged

fix(insights): follow ups from the profile analysis review#752
callingmedic911 merged 1 commit into
mainfrom
fix/insights-profile-followups

Conversation

@callingmedic911

@callingmedic911 callingmedic911 commented Jul 17, 2026

Copy link
Copy Markdown
Member

Follow up to #718, from a review of the merged PR.

Analyze fixes: keep other top level keys in the shared insights.yaml, drop the duplicate network probes, only require --agent to bypass a broken profile, stop reporting local file errors as platform errors. Doctor now runs its checks even without a profile. The .env loader only strips real quote pairs. Bad base urls and ~user paths render as normal errors instead of tracebacks.

Testbed fixes: removed the mid run empty recheck that could kill a restore halfway, split the download cache per repo, compare first span timestamps chronologically, restored --clobber on publish and substring matching for missing releases, tests no longer break when TESTBED_STATE_REPO is set.

Tested: plugin suite green (526 passed) plus a local end to end run. Restored state-v6 into a live platform twice (fresh and skip path), ran doctor with and without a profile, and a real analyze that wrote two insights and kept the extra keys in the file.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added credential checks to analysis preflight validation.
    • Doctor checks now report environment readiness even without a profile.
    • Analysis can continue with explicitly provided agent details when profile loading fails.
  • Bug Fixes

    • Preserved unrelated YAML settings when updating insights.
    • Improved profile path, quoting, and base URL handling.
    • Strengthened restore timestamp validation and isolated state bundle caches.
    • Enabled safe overwriting when retrying bundle uploads.

@github-actions github-actions Bot added the fix label Jul 17, 2026
@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor
Suite Lines Covered Line Rate Branch Rate
Unit Tests 25539/32751 78.0% 62.6%
Integration Tests 14733/31400 46.9% 19.2%

@callingmedic911
callingmedic911 marked this pull request as ready for review July 17, 2026 19:17
@callingmedic911
callingmedic911 requested review from a team as code owners July 17, 2026 19:17
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR separates credential and environment checks, updates profile parsing and CLI preflight behavior, preserves unrelated YAML keys, and changes testbed restore ordering, release uploads, and repository-scoped bundle caching.

Changes

Insights runtime flow

Layer / File(s) Summary
Profile and preflight contracts
plugins/nemo-insights/src/nemo_insights_plugin/contracts/profile.py, plugins/nemo-insights/src/nemo_insights_plugin/preflight.py, plugins/nemo-insights/tests/contracts/test_profile_contract.py, plugins/nemo-insights/tests/test_preflight.py
Profile path, quote parsing, and base URL precedence are updated. Credential checks are separated from environment checks, and workspace probing is skipped when agent or workspace data is unavailable.
Local insights persistence
plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py, plugins/nemo-insights/tests/test_periodic_analysis.py
Existing YAML mappings retain unrelated top-level keys while replacing the insights value.
CLI profile and analysis flow
plugins/nemo-insights/src/nemo_insights_plugin/cli.py, plugins/nemo-insights/tests/test_cli_profile.py
Analyze and doctor preflight behavior, profile-load handling, exception routing, and related CLI coverage are updated.

Testbed bundle operations

Layer / File(s) Summary
Restore matching and ingestion ordering
plugins/nemo-insights/testbed/reingest.py, plugins/nemo-insights/tests/testbed/test_reingest.py
Restore matching compares normalized timestamps, while direct restore validates empty collections up front and posts non-empty collections in the revised order.
Publishing and repository-scoped downloads
plugins/nemo-insights/testbed/publish.py, plugins/nemo-insights/testbed/release.py, plugins/nemo-insights/tests/testbed/test_publish.py, plugins/nemo-insights/tests/testbed/test_release.py
Release uploads use --clobber, missing-release detection checks for embedded “not found” text, and downloaded bundles are cached per state repository.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant InsightsCLI
  participant Preflight
  participant Platform
  User->>InsightsCLI: run analyze or doctor
  InsightsCLI->>Preflight: check credentials
  InsightsCLI->>Preflight: check environment
  Preflight->>Platform: probe base URL and optional workspace
  Platform-->>Preflight: readiness results
  Preflight-->>InsightsCLI: check results
Loading

Possibly related PRs

Suggested reviewers: svvarom

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.83% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title is related to the PR, but it is too generic to convey the primary change. Rename it to mention the main fix, e.g. preserving insights.yaml keys and profile-analysis/preflight changes.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
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.
✨ 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/insights-profile-followups

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🤖 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/src/nemo_insights_plugin/cli.py`:
- Around line 101-103: Update the discovered-profile error path in
_load_profile_or_error so it still loads the discovered profile’s adjacent .env
via load_env_file before raising ProfileError and allowing --agent to continue.
Preserve the existing warning behavior, and add coverage for a malformed
optimizer.yaml with a valid .env and --agent, verifying INFERENCE_API_KEY is
available during analysis.

In `@plugins/nemo-insights/testbed/publish.py`:
- Around line 140-143: Update the publish flow around next_ref and
release._release_gh so concurrent publishers cannot overwrite each other’s
assets: add an atomic publication lock or ref reservation before uploading, or
only pass --clobber after explicitly verifying the existing asset is a partial
upload. Preserve retry behavior for the same publisher while preventing
clobbering an unverified candidate.

In `@plugins/nemo-insights/testbed/reingest.py`:
- Around line 583-584: Update the restore flow in reingest.py so require_empty
performs a preflight pass across every target workspace before issuing any POST
requests. Validate each workspace’s emptiness first, abort the operation if any
target is populated, then run the existing write loop only after all targets
pass.
- Around line 494-497: Update _doc_started_at and the related matching/skip path
so documents with missing or malformed started_at values are rejected or cause
the probe to be unavailable, rather than converted to _EPOCH_ISO or raising
ValueError. Ensure only successfully validated timestamps participate in corpus
fingerprint comparisons, while preserving normal timezone normalization for
valid values.
🪄 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: 0ff48da0-4579-468c-ba3a-77afc0b4a88b

📥 Commits

Reviewing files that changed from the base of the PR and between 8f7743d and b31970c.

📒 Files selected for processing (14)
  • plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py
  • plugins/nemo-insights/src/nemo_insights_plugin/cli.py
  • plugins/nemo-insights/src/nemo_insights_plugin/contracts/profile.py
  • plugins/nemo-insights/src/nemo_insights_plugin/preflight.py
  • plugins/nemo-insights/testbed/publish.py
  • plugins/nemo-insights/testbed/reingest.py
  • plugins/nemo-insights/testbed/release.py
  • plugins/nemo-insights/tests/contracts/test_profile_contract.py
  • plugins/nemo-insights/tests/test_cli_profile.py
  • plugins/nemo-insights/tests/test_periodic_analysis.py
  • plugins/nemo-insights/tests/test_preflight.py
  • plugins/nemo-insights/tests/testbed/test_publish.py
  • plugins/nemo-insights/tests/testbed/test_reingest.py
  • plugins/nemo-insights/tests/testbed/test_release.py

Comment thread plugins/nemo-insights/src/nemo_insights_plugin/cli.py
Comment thread plugins/nemo-insights/testbed/publish.py
Comment thread plugins/nemo-insights/testbed/reingest.py
Comment thread plugins/nemo-insights/testbed/reingest.py
Post-merge review follow-up to #718. Analyze keeps other top level
keys when writing the shared insights file, .env values keep quotes
that are part of the secret, --agent alone bypasses a broken profile,
local file errors are no longer reported as platform errors, doctor
runs its checks even without a profile, analyze drops the duplicate
network probes, restore drops the mid-run recheck, the download cache
is split per repo, and the first-span guard compares timestamps
chronologically. Restores --clobber and substring release matching.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
@callingmedic911
callingmedic911 force-pushed the fix/insights-profile-followups branch from b31970c to 0cd00ed Compare July 17, 2026 19:30
@callingmedic911
callingmedic911 added this pull request to the merge queue Jul 17, 2026
Merged via the queue into main with commit f979b63 Jul 17, 2026
61 checks passed
@callingmedic911
callingmedic911 deleted the fix/insights-profile-followups branch July 17, 2026 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants