agent: Skip HBN configuration when config versions are unchanged#2848
Merged
Conversation
Contributor
Summary by CodeRabbit
WalkthroughThe agent now tracks applied network configuration versions to skip redundant updates, adds a helper for treating empty strings as absent, and preserves ChangesNetwork config version gating
NVUE apply logging
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (2)
crates/agent/src/nvue.rs (1)
1045-1047: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse structured tracing fields for diff/apply diagnostics.
Line 1047 logs interpolated text; please emit
apply_stdoutandconfig_diff_stdoutas tracing fields so logs remain queryable in logfmt.Proposed change
- tracing::info!("nv config apply: {stdout}"); + tracing::info!(apply_stdout = %stdout, "nv config apply"); // We're logging this just to see what was in there, in case it can help // explain the "config apply executed with no config diff" message. - tracing::info!("nv config diff: {config_diff_stdout}"); + tracing::info!(config_diff_stdout = %config_diff_stdout, "nv config diff");As per coding guidelines, "When writing log messages, prefer placing common fields as attributes passed to tracing functions instead of using string interpolation." As per path instructions, "Review Rust code against STYLE_GUIDE.md: ... structured tracing fields ..."
🤖 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 `@crates/agent/src/nvue.rs` around lines 1045 - 1047, The nv config diagnostics are using interpolated text instead of structured tracing fields, which makes the logs harder to query. Update the tracing in nvue.rs around the config diff/apply logging to pass apply_stdout and config_diff_stdout as fields on the tracing call rather than embedding them into the message string, keeping the existing diagnostic context while making the output structured and logfmt-friendly.Sources: Coding guidelines, Path instructions
crates/agent/src/main_loop.rs (1)
661-664: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the skip log structured.
The
{:?}payload buries version values inside the message, making logfmt search harder. Prefer explicit tracing fields for the two versions. As per coding guidelines, “All services should emit logs in 'logfmt' syntax” and “prefer placing common fields as attributes passed to tracing functions”; as per path instructions, use “structured tracing fields”.Proposed fix
tracing::debug!( - "No configuration change, skipping HBN updates: {:?}", - &self.current_network_version + managed_host_config_version = self + .current_network_version + .managed_host_config_version + .as_deref() + .unwrap_or_default(), + instance_network_config_version = self + .current_network_version + .instance_network_config_version + .as_deref() + .unwrap_or_default(), + "No configuration change, skipping HBN updates" );🤖 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 `@crates/agent/src/main_loop.rs` around lines 661 - 664, The skip message in the HBN update path is unstructured because `tracing::debug!` formats `self.current_network_version` with `{:?}` inside the message, which makes version values harder to search in logfmt. Update the logging in `main_loop.rs` around the “No configuration change, skipping HBN updates” branch to use structured tracing fields on `tracing::debug!` instead of embedding the version in the message, and include both version values as explicit attributes so the log stays logfmt-friendly and searchable.Sources: Coding guidelines, Path instructions
🤖 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 `@crates/agent/src/main_loop.rs`:
- Around line 427-445: CurrentNetworkVersion::matches_versions_from currently
treats the default tracker state as already matching empty gRPC versions, which
can wrongly skip the first HBN apply; add explicit state in
CurrentNetworkVersion to record whether an apply has happened and only permit
the version match shortcut after a successful apply. Update the logic in
CurrentNetworkVersion and its callers in main_loop.rs so the initial Default
state (None, None) never suppresses the first update when
managed_host_config_version and instance_network_config_version are absent.
---
Nitpick comments:
In `@crates/agent/src/main_loop.rs`:
- Around line 661-664: The skip message in the HBN update path is unstructured
because `tracing::debug!` formats `self.current_network_version` with `{:?}`
inside the message, which makes version values harder to search in logfmt.
Update the logging in `main_loop.rs` around the “No configuration change,
skipping HBN updates” branch to use structured tracing fields on
`tracing::debug!` instead of embedding the version in the message, and include
both version values as explicit attributes so the log stays logfmt-friendly and
searchable.
In `@crates/agent/src/nvue.rs`:
- Around line 1045-1047: The nv config diagnostics are using interpolated text
instead of structured tracing fields, which makes the logs harder to query.
Update the tracing in nvue.rs around the config diff/apply logging to pass
apply_stdout and config_diff_stdout as fields on the tracing call rather than
embedding them into the message string, keeping the existing diagnostic context
while making the output structured and logfmt-friendly.
🪄 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: ad1e5f64-6273-4708-91c5-cf62dc6c7cd4
📒 Files selected for processing (3)
crates/agent/src/lib.rscrates/agent/src/main_loop.rscrates/agent/src/nvue.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
rwthompsonii
approved these changes
Jun 25, 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.
We've seen evidence that the
nv config diffmethod that one of the HBN update functions relies on may sometimes result in no-op config changes. NVUE will complain about this with a "config apply executed with no config diff" message, but more annoyingly this also causes the code that checks for actual changes to get a false positive, which turns into a needlessPostConfigCheckWaithealth alert.In order to avoid this, we now track the
managed_host_config_versionandinstance_network_config_versionfields from theManagedHostNetworkConfigResponsemessage, and avoid calling any reconfiguration code if these versions haven't changed since the last time this agent process saw them.Related issues
Type of Change
Breaking Changes
Testing
Additional Notes