Skip to content

fix(api-model): avoid panic parsing build versions with a "-dirty" suffix#3775

Open
kurotych wants to merge 1 commit into
NVIDIA:mainfrom
kurotych:fix-dirty-version-panic
Open

fix(api-model): avoid panic parsing build versions with a "-dirty" suffix#3775
kurotych wants to merge 1 commit into
NVIDIA:mainfrom
kurotych:fix-dirty-version-panic

Conversation

@kurotych

@kurotych kurotych commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Bug description

The NICo API panicked at runtime

thread 'tokio-rt-worker' (7) panicked at crates/api-model/src/machine/upgrade_policy.rs:239:43:
called `Result::unwrap()` on an `Err` value: ParseIntError { kind: InvalidDigit }
stack backtrace:

The root cause is a combination of:

  • default VERSION constructor which uses the following command that includes the --dirty key: VERSION ?= $(shell git describe --tags --always --dirty 2>/dev/null || echo dev)
  • BuildVersion parser which can't parse the default-generated version string like: v2.0.0-rc.2-9-g0401aad9f-dirty

Proposed solution

Strip the -dirty suffix before parsing BuildVersion if present.

Related issues

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

Signed-off-by: Anatolii Kurotych <akurotych@gmail.com>
@kurotych
kurotych requested a review from a team as a code owner July 21, 2026 15:20
@copy-pr-bot

copy-pr-bot Bot commented Jul 21, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

BuildVersion now recognizes an optional -dirty suffix, stores its presence, reproduces it in display output, and continues ignoring it during ordering. Version parsing tests cover both clean and dirty inputs.

Changes

Build Version Dirty-State Support

Layer / File(s) Summary
Parse and display dirty status
crates/api-model/src/machine/upgrade_policy.rs
Adds is_dirty, strips and records a trailing -dirty suffix during parsing, and conditionally emits the suffix through Display.
Preserve ordering semantics and update tests
crates/api-model/src/machine/upgrade_policy.rs
Documents that is_dirty is excluded from ordering and updates parsing expectations for clean and dirty versions.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Title check ✅ Passed The title accurately summarizes the bug fix for parsing build versions with a "-dirty" suffix.
Description check ✅ Passed The description is clearly aligned with the change and explains the panic, root cause, and fix.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

🧹 Nitpick comments (1)
crates/api-model/src/machine/upgrade_policy.rs (1)

320-459: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Expand dirty-version regression coverage.

The new test covers only the five-part parser branch and checks is_dirty; it does not verify Display round-tripping or dirty inputs handled by the other parser branches. Add table-driven cases for representative shapes and a build_version_display_round_trips case ending in -dirty.

🤖 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/api-model/src/machine/upgrade_policy.rs` around lines 320 - 459,
Expand the version parser test table with dirty-input cases covering the other
supported shapes, not only the five-part branch, and assert the expected
is_dirty and parsed fields. Add a build_version_display_round_trips test using a
BuildVersion whose formatted output ends with “-dirty”, verifying parsing the
displayed value reproduces the original version.
🤖 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.

Nitpick comments:
In `@crates/api-model/src/machine/upgrade_policy.rs`:
- Around line 320-459: Expand the version parser test table with dirty-input
cases covering the other supported shapes, not only the five-part branch, and
assert the expected is_dirty and parsed fields. Add a
build_version_display_round_trips test using a BuildVersion whose formatted
output ends with “-dirty”, verifying parsing the displayed value reproduces the
original version.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ef3b2e3f-a421-4272-aa12-210b0f17d78e

📥 Commits

Reviewing files that changed from the base of the PR and between 24cd3f0 and 98c94c0.

📒 Files selected for processing (1)
  • crates/api-model/src/machine/upgrade_policy.rs

@poroh

poroh commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

/ok to test 98c94c0

@poroh

poroh commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

@kfelternv, could you please take a look at this follow-up?

PR #2560 introduced the root images-core target with:

VERSION ?= $(shell git describe --tags --always --dirty ...)

That makes locally built images report versions ending in -dirty, which exposed this parser failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants