Skip to content

perf(images): short-circuit reachable tag scan - #7554

Closed
HOYALIM wants to merge 1 commit into
NVIDIA:mainfrom
HOYALIM:codex/issue-7249-reachable-tag-short-circuit
Closed

perf(images): short-circuit reachable tag scan#7554
HOYALIM wants to merge 1 commit into
NVIDIA:mainfrom
HOYALIM:codex/issue-7249-reachable-tag-short-circuit

Conversation

@HOYALIM

@HOYALIM HOYALIM commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Base-image fallback now checks remote release tags in semantic-version order and stops at the first reachable candidate. It preserves the existing selected tag and local fallback while avoiding the exhaustive reachability fan-out on the common path.

Related Issue

Closes #7249.

Changes

  • Parse the complete remote tag response and preserve peeled annotated-tag commits before ordering candidates.
  • Probe candidates newest-first and return immediately when one is reachable.
  • Measure the real Git command boundary with Trace2 regressions covering one-probe success, two-probe fallback, annotated tags, and complete remote exhaustion.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — internal fallback optimization; selected tags, configuration, and user-facing behavior are unchanged.
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: maintainer review pending; the focused tests prove selection equivalence at the Git boundary.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: no-docs-needed
  • Evidence: only internal release-tag reachability ordering changes; no command, configuration, or operator workflow changes.
  • Agent: Codex Desktop

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit: not applicable
  • Station profile/scenario: not applicable
  • Result: not applicable
  • Supporting evidence: scripts/prepare-dgx-station-host.sh is unchanged.

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • npm run check:diff
  • Targeted behavior tests pass for the current change set — npx vitest run --project cli src/lib/sandbox-base-image/source-identity.test.ts (28 passed).
  • Applicable broad gate passed — npm run test:changed (110 files, 1,125 tests) and npm run typecheck.
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Ho Lim subhoya@gmail.com

Summary by CodeRabbit

  • Bug Fixes
    • Improved version tag selection to return the nearest reachable base image tag.
    • Stops searching once the newest reachable tag is found, improving selection accuracy and efficiency.
    • Preserves local tag fallback behavior when no remote tag is reachable.
    • Correctly handles remote tags that are not reachable from the current branch.

Signed-off-by: Ho Lim <subhoya@gmail.com>
Copilot AI review requested due to automatic review settings July 26, 2026 02:17
@copy-pr-bot

copy-pr-bot Bot commented Jul 26, 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.

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: af36e445-1dcb-44ed-b255-e50fb6cc2855

📥 Commits

Reviewing files that changed from the base of the PR and between 4fdbb79 and cc02a6b.

📒 Files selected for processing (2)
  • src/lib/sandbox-base-image/source-identity.test.ts
  • src/lib/sandbox-base-image/source-identity.ts

📝 Walkthrough

Walkthrough

Remote release tags are now checked in descending semantic-version order and the search stops at the first reachable tag. Tests add Git trace instrumentation for probe counts, annotated-tag handling, unreachable newer tags, and local fallback behavior.

Changes

Remote tag reachability

Layer / File(s) Summary
Ordered reachability selection
src/lib/sandbox-base-image/source-identity.ts
Remote candidates are sorted before ancestry checks, returning the first candidate reachable from HEAD and preserving null when none are reachable.
Probe tracing and selection scenarios
src/lib/sandbox-base-image/source-identity.test.ts
Tests trace merge-base --is-ancestor calls and verify probe counts for reachable, unreachable, annotated, and local-fallback tag scenarios.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: area: sandbox

Suggested reviewers: copilot, apurvvkumaria, cv

Sequence Diagram(s)

sequenceDiagram
  participant SourceIdentity
  participant GitStatus
  participant GitRepository
  SourceIdentity->>GitRepository: read and sort remote release tags
  SourceIdentity->>GitStatus: check newest candidate ancestry
  GitStatus->>GitRepository: run merge-base --is-ancestor
  GitRepository-->>GitStatus: return reachability result
  GitStatus-->>SourceIdentity: return probe status
  SourceIdentity-->>SourceIdentity: stop at first reachable tag
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: short-circuiting the reachable tag scan for image fallback.
Linked Issues check ✅ Passed The rewrite and Trace2 tests cover newest-first short-circuiting, peeled annotated tags, and preserved local fallback as required by #7249.
Out of Scope Changes check ✅ Passed The changes stay focused on base-image tag resolution logic and its regression tests, with no obvious unrelated additions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Model comparison: normalized findings match; normalized E2E selections differ; severity counts match.

Nemotron output stays in workflow artifacts and does not change the assessment above.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: None

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@apurvvkumaria

Copy link
Copy Markdown
Collaborator

Replacement PR #7566 rebuilds this change from current main on an internal branch without modifying the contributor fork.

It preserves Ho Lim as Author of the production commit, keeps the comparator-equivalence regression in a separate signed commit, and credits dfernandez365-rgb's earlier verified #7249 candidate as prior art without claiming copied code.

The replacement passed the focused 29-test suite, the complete diff-scoped hook gate, exact-head security review, and documentation-writer review. I am leaving #7554 open and am not approving or merging either PR.

cv pushed a commit that referenced this pull request Jul 26, 2026
<!-- markdownlint-disable MD041 -->
## Summary

This replacement rebuilds #7554 from current `main` while retaining Ho
Lim as Author for the production optimization.
It reduces fallback reachability probes without changing tag selection,
trust, or local fallback behavior.

## Related Issue

Closes #7249.
Supersedes #7554 without closing it.

## Changes

- Parse the complete remote tag response, preserve peeled annotated-tag
commits, and order candidates with the existing semantic-version
comparator.
- Stop after the first reachable candidate while preserving stable,
prerelease, comparator-equivalent, and local-fallback behavior.
- Add deterministic probe coverage for one-probe success, two-probe
fallback, annotated tags, exhaustion, and comparator-equivalent textual
tags.
- Preserve Ho Lim as Author of commit `bf86e27a8`, replayed from the
verified #7554 production commit `cc02a6b64`.
- Credit dfernandez365-rgb's earlier verified candidate `0e1a0e59` as
prior art, as recorded in
[#7249](#7249 (comment)).
The replacement does not claim that #7554 copied the candidate.
- Documentation writer receipt for exact head `b67bf3a5a`: no docs
change required because the documented selection contract is unchanged.

## Type of Change

- [x] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates

- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [ ] Docs updated for user-facing behavior changes
- [x] Docs not applicable — justification: internal reachability
optimization; commands, configuration, output, and selection behavior
are unchanged.
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: exact-head
nine-category maintainer review passed; published-image trust, digest
pinning, override provenance, and fallback semantics are unchanged.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification

- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [ ] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Improved release tag selection to consistently choose the newest
reachable version from the remote repository.
* Correctly checks tag reachability before selecting a release, avoiding
unreachable or stale tags.
* Preserves the nearest local version as a fallback when no remote
release tag is reachable.
* Maintains consistent ordering when multiple tags represent equivalent
versions.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Ho Lim <subhoya@gmail.com>
@cv

cv commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Superseded by #7566, which preserves the same production optimization on current main and adds the missing comparator-equivalent tag regression. Closing this stale duplicate now that #7566 has merged.

@cv cv closed this Jul 26, 2026
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.

Base-image fallback scans every remote release tag before selecting the newest reachable one

4 participants