fix(cli): recognize BuildKit sandbox build progress (Fixes #2311)#2404
Open
deepujain wants to merge 1 commit intoNVIDIA:mainfrom
Open
fix(cli): recognize BuildKit sandbox build progress (Fixes #2311)#2404deepujain wants to merge 1 commit intoNVIDIA:mainfrom
deepujain wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds BuildKit progress recognition to the sandbox build stream: centralizes regex patterns for build/upload phases, introduces an exported Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
src/lib/sandbox-create-stream.ts (1)
125-130: Consider centralizing progress regexes to avoid parser drift.The same patterns are duplicated between
flushLineandshouldShowLine. A shared matcher keeps future edits safer.♻️ Suggested refactor
+const BUILD_PROGRESS_PATTERNS: RegExp[] = [ + /^ {2}Building image /, + /^ {2}Step \d+\/\d+ : /, + /^#\d+ \[/, + /^#\d+ (DONE|CACHED)\b/, +]; + +const UPLOAD_PROGRESS_PATTERNS: RegExp[] = [ + /^ {2}Pushing image /, + /^\s*\[progress\]/, + /^ {2}Image .*available in the gateway/, +]; + +const VISIBLE_PROGRESS_PATTERNS: RegExp[] = [ + ...BUILD_PROGRESS_PATTERNS, + /^ {2}Context: /, + /^ {2}Gateway: /, + /^Successfully built /, + /^Successfully tagged /, + /^ {2}Built image /, + ...UPLOAD_PROGRESS_PATTERNS, + /^Created sandbox: /, + /^✓ /, +]; + +function matchesAny(line: string, patterns: readonly RegExp[]) { + return patterns.some((re) => re.test(line)); +} ... - if ( - /^ {2}Building image /.test(line) || - /^ {2}Step \d+\/\d+ : /.test(line) || - /^#\d+ \[/.test(line) || - /^#\d+ (DONE|CACHED)\b/.test(line) - ) { + if (matchesAny(line, BUILD_PROGRESS_PATTERNS)) { setPhase("build"); - } else if ( - /^ {2}Pushing image /.test(line) || - /^\s*\[progress\]/.test(line) || - /^ {2}Image .*available in the gateway/.test(line) - ) { + } else if (matchesAny(line, UPLOAD_PROGRESS_PATTERNS)) { setPhase("upload"); ... - return ( - /^ {2}Building image /.test(line) || - /^ {2}Step \d+\/\d+ : /.test(line) || - /^#\d+ \[/.test(line) || - /^#\d+ (DONE|CACHED)\b/.test(line) || - ... - /^✓ /.test(line) - ); + return matchesAny(line, VISIBLE_PROGRESS_PATTERNS);Also applies to: 147-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sandbox-create-stream.ts` around lines 125 - 130, The duplicated Docker progress regexes used in flushLine and shouldShowLine should be centralized into a single exported constant (e.g., PROGRESS_REGEXES or progressMatchers) and reused by both functions; update flushLine and shouldShowLine to iterate over that shared array of RegExp objects (or test each with .some) instead of repeating the same literal patterns so future changes only need to be made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/sandbox-create-stream.ts`:
- Around line 125-130: The duplicated Docker progress regexes used in flushLine
and shouldShowLine should be centralized into a single exported constant (e.g.,
PROGRESS_REGEXES or progressMatchers) and reused by both functions; update
flushLine and shouldShowLine to iterate over that shared array of RegExp objects
(or test each with .some) instead of repeating the same literal patterns so
future changes only need to be made in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 70d3bb59-a42a-43bd-94f1-73854b7510d9
📒 Files selected for processing (2)
src/lib/sandbox-create-stream.test.tssrc/lib/sandbox-create-stream.ts
Fixes NVIDIA#2311 Signed-off-by: Deepak Jain <deepujain@gmail.com>
deepujain
force-pushed
the
fix/2311-buildkit-progress
branch
from
065412c to
d17b8f1
Compare
Contributor
Author
|
Centralized the BuildKit and legacy progress matchers so phase detection and visible-output filtering share the same patterns. Reran npm run build:cli, npm run typecheck:cli, and npm test -- src/lib/sandbox-create-stream.test.ts. |
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
1 participant
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.
Summary
NemoClaw's sandbox create stream only recognized the legacy Docker builder format, so BuildKit output would not be treated as active build progress once OpenShell emits it.
This adds BuildKit progress markers to the same parser path as the existing legacy builder output. It keeps the current legacy behavior and makes
#1 [internal] ...,#2 CACHED, and#3 DONE ...visible as build progress.Changes
src/lib/sandbox-create-stream.ts: recognize BuildKit step and completion lines while tracking the build phase.src/lib/sandbox-create-stream.test.ts: cover BuildKit progress output and verify it is streamed to the user.Testing
npm run build:clipassednpm run typecheck:clipassednpm test -- src/lib/sandbox-create-stream.test.tspassednpm testwas also attempted. The full suite is not green on current main in this environment; failures are in existing installer/onboard/legacy-guard tests outside this change.Evidence it works
The new focused test feeds BuildKit-style output into
streamSandboxCreateand verifies that the lines are logged, collected in output, and mark sandbox creation as having seen progress.Fixes #2311
Signed-off-by: Deepak Jain deepujain@gmail.com
Summary by CodeRabbit
Bug Fixes
Refactor
Tests