✨ Add progress bar and performance optimizations to analysis modes

[tsanders-rh]

Summary

Enhances kantra analysis with visual progress feedback and startup performance optimizations for both containerless and hybrid modes.

Depends on: #592 (Hybrid analysis mode)

Changes

Progress Bar Implementation

• Containerless mode progress bar: Real-time visual feedback showing percentage, completion bar, and current rule being processed
• Hybrid mode progress bar: Matches containerless UX with clean status messages and visual progress
• Binary analysis support: Progress tracking for JAR/WAR/EAR file analysis
• --no-progress flag: Disable progress bar for scripting/CI environments

Performance Optimizations

• Parallel rule loading: Load multiple rulesets concurrently instead of sequentially
• Parallel startup tasks: Run config validation, volume creation, and ruleset extraction concurrently
• Significantly reduces startup time when using multiple rulesets

User Experience Improvements

• Clean initialization messages: "Running source/binary analysis...", "✓ Initialized providers", "✓ Loaded N rules"
• Cursor hiding/showing for cleaner progress rendering
• Suppressed container command output (volume/network creation)
• Fixed INFO message bleed that interfered with progress bar
• "Analysis complete!" message with results section

Bug Fixes

• Fixed timeout and race condition in binary analysis
• Improved health check result handling

Example Output

Before:

INFO[0000] Running source code analysis
... verbose log output ...

After (with progress bar):

Running source code analysis...
✓ Initialized providers: java, builtin
✓ Loaded 45 rules

Progress: [████████████████████████████████] 100% (45/45) | Processing: quarkus-springboot
Analysis complete!

Results: 127 incidents across 8 files

Testing

• ✅ Tested with Java source code analysis
• ✅ Tested with binary (.war) analysis
• ✅ Tested with multiple rulesets (parallel loading)
• ✅ Tested --no-progress flag for scripting
• ✅ Verified both containerless and hybrid modes

Dependencies

• Requires analyzer-lsp v0.8.1-alpha for progress event support
• Depends on PR ✨ feat: Add hybrid analysis mode (analyzer in-process, providers in containers) #592 for hybrid mode implementation

Summary by CodeRabbit

• New Features

  • Live progress UI for provider init, decompilation, rule loading/execution and report generation; opt-out via --no-progress.
  • Hybrid in-process analysis with improved host/container path handling and parallel startup.
  • Binary input auto-detection with tailored progress/messages.
  • Option to publish container ports and capture command output for richer diagnostics.

• Bug Fixes

  • Stronger provider health/startup checks, better error propagation and cleanup.
  • Wait-for-target/decompilation waits now support timeouts and clearer status reporting.

• Docs

  • Updated hybrid health-check/readiness guidance.

• Chores

  • Dependency bumps and logging improvements.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

cmd/analyze-hybrid.go (2)

249-251: Fix Maven settings path for containerized providers.

The container cannot resolve the host filesystem path in a.mavenSettingsFile. Inside the container, the Maven settings file lives at the mounted path. This will cause provider initialization to fail when --maven-settings is used.

Apply this diff to use the container path:

 		providerSpecificConfig["depOpenSourceLabelsFile"] = "/usr/local/etc/maven.default.index"
 		if a.mavenSettingsFile != "" {
-			providerSpecificConfig["mavenSettingsFile"] = a.mavenSettingsFile
+			providerSpecificConfig["mavenSettingsFile"] = path.Join(util.ConfigMountPath, "settings.xml")
 		}

---

771-813: Check rule loading errors to prevent silent failures.

The result.err field is logged (line 790) but never checked during result collection (lines 807-812). This allows analysis to proceed even if all rulesets fail to load, potentially running with zero rules. Compare this to the startup task error handling at lines 574-577 where errors are properly validated.

Add error checking in the result collection loop:

+	var loadErrors []error
 	// Collect and merge results
 	for result := range resultChan {
+		if result.err != nil {
+			loadErrors = append(loadErrors, fmt.Errorf("%s: %w", result.rulePath, result.err))
+			continue
+		}
 		ruleSets = append(ruleSets, result.ruleSets...)
 		for k, v := range result.providers {
 			needProviders[k] = v
 		}
 	}
+
+	// Fail if no rulesets loaded successfully
+	if len(ruleSets) == 0 {
+		if len(loadErrors) > 0 {
+			return fmt.Errorf("failed to load any rulesets: %v", loadErrors)
+		}
+		return fmt.Errorf("no rulesets loaded - check rule paths and formats")
+	}
+	if len(loadErrors) > 0 {
+		operationalLog.Info("some rulesets failed to load but continuing with successful ones", "failures", len(loadErrors))
+	}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fcf0f2d and 1d3e9e8.

📒 Files selected for processing (2)

• cmd/analyze-hybrid.go (22 hunks)
• docs/hybrid.md (3 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-08-08T15:06:52.056Z

Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.

Applied to files:

• cmd/analyze-hybrid.go

🧬 Code graph analysis (1)

cmd/analyze-hybrid.go (3)

pkg/provider/java.go (1)

• WalkJavaPathForTarget (67-108)

pkg/util/util.go (5)

• JavaArchive (82-82)
• WebArchive (83-83)
• EnterpriseArchive (84-84)
• ClassFile (85-85)
• SourceMountPath (31-31)

cmd/analyze-bin.go (1)

• ConsoleHook (43-46)

🪛 LanguageTool

docs/hybrid.md

[style] ~306-~306: The double modal “required Recommended” is nonstandard (only accepted in certain dialects). Consider “to be Recommended”.
Context: ... No local LSP installation required - Recommended for: macOS and production use --- *...

(NEEDS_FIXED)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Build & test from commit

🔇 Additional comments (9)

docs/hybrid.md (1)

219-219: LGTM! Documentation accurately reflects implementation.

The documentation updates correctly describe the new TCP port polling with exponential backoff, mark completed TODOs, and fix the grammar issue. These changes align well with the implementation in cmd/analyze-hybrid.go.

Also applies to: 247-248, 306-306

cmd/analyze-hybrid.go (8)

35-88: LGTM! Excellent defensive validation.

Early validation of Maven settings and input paths with helpful error messages prevents confusing failures later. This improves the user experience significantly.

---

175-222: LGTM! Robust health check implementation.

The TCP polling with exponential backoff (50ms to 500ms) and context cancellation handling is well-designed. The timeout error includes helpful troubleshooting steps.

---

347-406: LGTM! Correct handling of binary vs source analysis.

The conditional logic properly skips target directory walking for binary inputs since decompilation occurs in the container. The dynamic Java exclude paths are correctly applied to the builtin provider.

---

620-643: LGTM! Efficient parallel health checks.

Running health checks concurrently for all providers reduces startup time. The goroutine variable capture is correct, and error handling properly fails fast if any provider fails to become ready.

---

670-685: LGTM! Proper cleanup on provider setup failure.

This correctly addresses the previous review comment by cleaning up all started providers and containers when setup fails. This prevents resource leaks and port conflicts on subsequent runs.

---

691-716: LGTM! Container path translation properly handles nested paths.

This correctly addresses the previous review comment by using strings.HasPrefix to handle nested container paths (e.g., /opt/input/source/java-project-1234), not just exact matches. The file input case is also properly handled using the parent directory.

---

837-920: LGTM! Comprehensive progress reporting implementation.

The progress bar implementation is well-designed with:

• Conditional logger to avoid interference (lines 425-430)
• Channel-based event consumption (lines 850-905)
• Cumulative progress tracking across rulesets (lines 854-898)
• Clean stage transitions and completion messages

The integration with RunRulesWithOptions is clean and the goroutine cleanup with progressCancel() and <-progressDone is correct.

---

941-986: LGTM! Clean output generation and results summary.

The output writing and static report generation are well-structured. The conditional results summary (lines 976-982) only in progress mode maintains clean console output. The timing instrumentation throughout the function provides good observability.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

pkg/provider/java_test.go (1)

390-396: Consider asserting that an error is returned.

The test only logs the error without asserting its presence. Based on the WaitForTargetDir implementation, calling watcher.Add() on a nonexistent path should return an error. Consider adding an assertion to make the test more robust.

Apply this diff to improve the test:

 func TestWaitForTargetDir_WithNonexistentPath(t *testing.T) {
 	logger := getTestLogger()
 	err := WaitForTargetDir(logger, "/nonexistent/path", 100*time.Millisecond)
-	// The initial Stat will fail, but it depends on the implementation
-	// This test is mostly to ensure we don't panic
-	t.Logf("Error for nonexistent path: %v", err)
+	// Should return an error when trying to watch a nonexistent path
+	require.Error(t, err)
 }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6ecabf5 and b292dcc.

📒 Files selected for processing (1)

• pkg/provider/java_test.go (4 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

pkg/provider/java_test.go (1)

pkg/provider/java.go (1)

• WaitForTargetDir (129-174)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Build & test from commit

🔇 Additional comments (1)

pkg/provider/java_test.go (1)

348-360: LGTM! Timeout parameters are appropriate.

The timeout values are well-chosen for each test scenario:

• 5 seconds for tests expecting successful completion (existing target, delayed creation)
• 100 milliseconds for tests expecting failures or timeouts

The test logic correctly validates the updated WaitForTargetDir API with explicit timeout handling.

Also applies to: 362-371, 373-388, 475-486

[codecov]

Codecov Report

❌ Patch coverage is 7.15884% with 415 lines in your changes missing coverage. Please review.
✅ Project coverage is 31.07%. Comparing base (e07ec59) to head (318060f).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/analyze-hybrid.go	5.43%	174 Missing ⚠️
cmd/analyze-bin.go	0.00%	109 Missing ⚠️
cmd/analyze.go	1.72%	57 Missing ⚠️
pkg/provider/java.go	41.17%	28 Missing and 2 partials ⚠️
cmd/progress_mode.go	0.00%	24 Missing ⚠️
cmd/command-context.go	0.00%	19 Missing ⚠️
cmd/static-report.go	0.00%	1 Missing ⚠️
pkg/container/container.go	0.00%	1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (7.15%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #593      +/-   ##
==========================================
- Coverage   33.21%   31.07%   -2.14%     
==========================================
  Files          33       34       +1     
  Lines        3947     4257     +310     
==========================================
+ Hits         1311     1323      +12     
- Misses       2490     2787     +297     
- Partials      146      147       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

cmd/analyze-hybrid.go (1)

227-228: Consider using build args for LSP paths like Maven settings does.

The Maven settings file correctly uses util.ConfigMountPath from build args (Line 232), but the LSP server path and bundle path are hardcoded. These paths may differ in downstream builds.

Consider defining these paths in cmd/settings.go alongside JavaProviderImage:

// In cmd/settings.go Config struct:
JavaLspServerPath string `env:"JAVA_LSP_SERVER_PATH" default:"/jdtls/bin/jdtls"`
JavaBundlePath    string `env:"JAVA_BUNDLE_PATH" default:"/jdtls/java-analyzer-bundle/java-analyzer-bundle.core/target/java-analyzer-bundle.core-1.0.0-SNAPSHOT.jar"`

Then reference them here:

-providerSpecificConfig["lspServerPath"] = "/jdtls/bin/jdtls"
-providerSpecificConfig["bundles"] = "/jdtls/java-analyzer-bundle/java-analyzer-bundle.core/target/java-analyzer-bundle.core-1.0.0-SNAPSHOT.jar"
+providerSpecificConfig["lspServerPath"] = Settings.JavaLspServerPath
+providerSpecificConfig["bundles"] = Settings.JavaBundlePath

Based on learnings

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b292dcc and 5eb4d21.

📒 Files selected for processing (1)

• cmd/analyze-hybrid.go (21 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-08-08T15:06:52.056Z

Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.

Applied to files:

• cmd/analyze-hybrid.go

🧬 Code graph analysis (1)

cmd/analyze-hybrid.go (4)

pkg/util/util.go (6)

• ConfigMountPath (33-33)
• JavaArchive (82-82)
• WebArchive (83-83)
• EnterpriseArchive (84-84)
• ClassFile (85-85)
• SourceMountPath (31-31)

pkg/provider/java.go (1)

• WalkJavaPathForTarget (67-108)

cmd/settings.go (2)

• Config (26-35)
• Settings (24-24)

cmd/analyze-bin.go (1)

• ConsoleHook (43-46)

🔇 Additional comments (7)

cmd/analyze-hybrid.go (7)

156-203: LGTM! Health check implementation is robust.

The exponential backoff strategy (50ms to 500ms) with proper context cancellation handling and detailed error messages is well-designed for local container health checks.

---

332-344: LGTM! Binary input handling is correct.

The logic correctly skips target directory walking for binary files since decompilation happens in the container-based Java provider. The builtin provider only needs to exclude target directories for source code analysis.

---

508-572: LGTM! Parallel startup tasks with proper cleanup.

The parallel execution of config validation, volume creation, and ruleset extraction is well-implemented. The cleanup logic (lines 559-564) correctly removes the volume on any startup failure. Since providers haven't started yet at this point, volume cleanup is the only resource that needs cleanup.

---

606-636: LGTM! Parallel health checks improve startup performance.

The parallel health check implementation correctly captures loop variables and uses a fail-fast approach. The 30-second timeout per provider is appropriate for containerized providers on localhost.

---

659-674: LGTM! Provider cleanup properly prevents resource leaks.

The cleanup logic correctly stops all previously-started providers (line 667) and removes their containers (line 670) when setupNetworkProvider fails. This addresses the past review comment about preventing leftover containers from blocking subsequent runs.

---

680-705: LGTM! Container-to-host path translation is correct.

The path translation logic correctly handles both directory and file input cases by using prefix matching instead of exact matching. This ensures nested container paths (e.g., /opt/input/source/java-project-1234) are properly translated to host paths so the builtin provider can find the files. This addresses the critical past review comment.

---

756-824: LGTM! Rule loading error handling prevents silent failures.

The parallel rule loading implementation correctly validates that at least one ruleset loaded successfully (lines 808-814) and logs warnings for partial failures (lines 816-821). This addresses the critical past review comment about preventing analysis from proceeding with zero rules.

The error collection and validation ensures that if all rulesets fail to load, the analysis fails with a clear error message.

[tsanders-rh]

@eemcmullan - Thanks for review. All updated per comments. Waiting on CI, but ready for you to recheck. Thanks!

[shawn-hurley]

Can we consider adding a some type that will encapsulate all of the noProgress checks?

[shawn-hurley]

I think that this is something that we should consider adding the analyzer-lsp library, this could be helpful for all the integrators (addon/hub, kai, kantra). I don't think we should stop it from going here, but just wanted to call that out.

[tsanders-rh]

konveyor/analyzer-lsp#967

[tsanders-rh]

Thanks for the feedback, @shawn-hurley! I've addressed both of your comments:

1. Encapsulating noProgress checks

I've created a ProgressMode type in cmd/progress_mode.go that encapsulates all the noProgress logic. This provides better abstraction and makes the code more maintainable.

Key methods:

• OperationalLogger(log) - Returns the logger or logr.Discard() based on progress mode
• ShouldAddConsoleHook() - Determines if console hooks should be added
• HideCursor() / ShowCursor() - Terminal cursor control
• Printf() / Println() - Conditional output to stderr
• IsEnabled() / IsDisabled() - Query progress state

Before:

var operationalLog logr.Logger
if a.noProgress {
    operationalLog = a.log
} else {
    operationalLog = logr.Discard()
}

if !a.noProgress {
    fmt.Fprintf(os.Stderr, "\033[?25l")
    defer fmt.Fprintf(os.Stderr, "\033[?25h")
}

After:

progressMode := NewProgressMode(a.noProgress)
operationalLog := progressMode.OperationalLogger(a.log)

progressMode.HideCursor()
defer progressMode.ShowCursor()

This refactoring affected:

• cmd/analyze-bin.go: 11 instances replaced
• cmd/analyze-hybrid.go: 11 instances replaced

2. Logging discarded output with verbosity control

I've added helper methods in cmd/command-context.go to conditionally capture and log container command output based on verbosity level:

• setupCommandOutput(cmd) - Configures stdout/stderr (discard or capture)
• logCommandOutput(cmdName, stdout, stderr) - Logs captured output

Implementation:

func (c *AnalyzeCommandContext) setupCommandOutput(cmd *exec.Cmd) (stdout, stderr *bytes.Buffer) {
    if c.log.V(2).Enabled() {
        // Capture output for verbose logging
        stdout = &bytes.Buffer{}
        stderr = &bytes.Buffer{}
        cmd.Stdout = stdout
        cmd.Stderr = stderr
        return stdout, stderr
    }
    // Discard output to keep console clean
    cmd.Stdout = io.Discard
    cmd.Stderr = io.Discard
    return nil, nil
}

Usage:

cmd := exec.Command(Settings.ContainerBinary, args...)
stdout, stderr := c.setupCommandOutput(cmd)
err := cmd.Run()
c.logCommandOutput("network create", stdout, stderr)

Behavior:

• Normal mode: Output discarded (clean console)
• Verbose mode (-v 2): Output captured and logged for debugging

Applied to:

• createContainerNetwork()
• createContainerVolume()

All tests pass and the changes maintain backward compatibility.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

cmd/analyze-bin.go (1)

893-944: Replace log.Fatalln in static report path to avoid leaving cursor hidden and hard exits

buildStaticReportFile still uses log.Fatalln on error (Lines 930, 935, 940). Because RunAnalysisContainerless now hides the cursor and relies on a deferred ShowCursor(), any log.Fatalln in this call stack will:

• Call os.Exit(1) and bypass the defer, leaving the user’s terminal cursor hidden.
• Bypass the normal error return path and be inconsistent with the rest of the function, which now returns errors instead of exiting.

This is a user‑visible regression and should be fixed.

You can preserve behavior by converting these to normal error returns and logging through a.log (or operationalLog at the caller) instead of using the global log package. For example:

 func (a *analyzeCommand) buildStaticReportFile(ctx context.Context, staticReportPath string, depsErr bool) error {
@@
-	apps, err := validateFlags(outputAnalyses, applicationNames, outputDeps, a.log)
-	if err != nil {
-		log.Fatalln("failed to validate flags", err)
-	}
+	apps, err := validateFlags(outputAnalyses, applicationNames, outputDeps, a.log)
+	if err != nil {
+		a.log.Error(err, "failed to validate flags for static report")
+		return fmt.Errorf("failed to validate flags for static report: %w", err)
+	}
@@
-	err = loadApplications(apps)
-	if err != nil {
-		log.Fatalln("failed to load report data from analysis output", err)
-	}
+	if err := loadApplications(apps); err != nil {
+		a.log.Error(err, "failed to load report data from analysis output")
+		return fmt.Errorf("failed to load report data from analysis output: %w", err)
+	}
@@
-	err = generateJSBundle(apps, outputJSPath, a.log)
-	if err != nil {
-		log.Fatalln("failed to generate output.js file from template", err)
-	}
-
-	return nil
+	if err := generateJSBundle(apps, outputJSPath, a.log); err != nil {
+		a.log.Error(err, "failed to generate output.js file from template")
+		return fmt.Errorf("failed to generate output.js file from template: %w", err)
+	}
+
+	return nil
 }

With these changes, GenerateStaticReportContainerless can propagate errors normally, ensuring the deferred ShowCursor() always runs and the CLI remains consistent with the rest of the codebase’s error‑handling approach.

(Optionally, you may also want to either write something meaningful to static-report.log in GenerateStaticReportContainerless or drop the unused log file entirely.)

Also applies to: 958-997

♻️ Duplicate comments (1)

cmd/analyze.go (1)

209-227: Avoid rerunning JSON/static-report generation after RunAnalysisHybridInProcess

In the hybrid branch, RunAnalysisHybridInProcess already:

• writes output.yaml
• calls CreateJSONOutput() when --json-output is set
• calls GenerateStaticReport(ctx, operationalLog)

The subsequent block in RunE:

err = analyzeCmd.CreateJSONOutput()
...
err = analyzeCmd.GenerateStaticReport(ctx, operationalLog)

reruns both JSON and static-report generation, doubling the work with no user benefit and extending total runtime, especially for larger reports. This is the same concern raised earlier and still present.

You can safely rely on RunAnalysisHybridInProcess to handle these steps and delete the redundant block:

-			err = analyzeCmd.CreateJSONOutput()
-			if err != nil {
-				log.Error(err, "failed to create json output file")
-				return err
-			}
-
-			// Create conditional logger for static report generation
-			var operationalLog logr.Logger
-			if analyzeCmd.noProgress {
-				operationalLog = log
-			} else {
-				operationalLog = logr.Discard()
-			}
-			err = analyzeCmd.GenerateStaticReport(ctx, operationalLog)
-			if err != nil {
-				log.Error(err, "failed to generate static report")
-				return err
-			}
-
-			return nil
+			return nil

This preserves behavior for both progress and --no-progress modes while ensuring the generator runs only once per analysis.

Also applies to: 282-293

🧹 Nitpick comments (8)

cmd/progress_mode.go (1)

10-75: ProgressMode encapsulation looks solid; consider minor ergonomics improvements

The enabled/disabled gating, logger selection, and cursor/print behavior all align with the progress UX you’re building. No correctness issues spotted.

If you ever want easier testing or to avoid escape sequences when stderr isn’t a TTY, consider letting ProgressMode accept an io.Writer (or injecting a small isTTY check) instead of hard‑coding os.Stderr. That’s purely a nice‑to‑have.

cmd/analyze-bin.go (2)

63-81: Progress bar rendering logic is correct; message truncation is slightly byte-oriented

The bar computation, clamping, and in‑place update logic (carriage return + clear to EOL) all look good and should behave well in typical terminals.

One minor nit: len(message) truncation is byte‑based, so very long non‑ASCII rule names could get chopped mid‑rune. If you expect non‑ASCII rule IDs, using []rune(message) for truncation would avoid that. Otherwise this is fine.

---

676-807: Provider setup helpers are consistent with the new operational logger pattern

The refactors to:

• setBuiltinProvider / setJavaProvider
• setupJavaProvider
• setupBuiltinProvider
• setInternalProviders
• startProvidersContainerless

all follow a consistent pattern:

• Operational messages go through operationalLog, while errors are still logged on a.log.
• ContextLines and CLI‑selected AnalysisMode are applied before client creation.
• Errors are returned with context (wrapped via fmt.Errorf) instead of causing hard exits.
• setInternalProviders now surfaces provider setup failures rather than silently swallowing them.

This improves observability and makes failures easier to handle upstream without introducing new correctness issues.

Also applies to: 809-841

cmd/analyze.go (1)

84-87: --no-progress flag wiring is correct and follows prior flag-ownership guidance

The new noProgress field on analyzeCommand and the corresponding --no-progress flag are correctly scoped to the command struct (not package-level globals), which avoids shared mutable state across commands/tests and aligns with earlier guidance for this repo. The flag description is clear and matches its usage in containerless and hybrid paths.

Based on learnings

Also applies to: 327-327

cmd/analyze-hybrid.go (4)

36-68: Hybrid-specific validation and provider overrides look reasonable; minor UX/override caveats

• validateProviderConfig correctly validates --override-provider-settings and performs a best‑effort port availability check before starting providers.
• The override loading/apply path (loadOverrideProviderSettings, applyProviderOverrides, mergeProviderSpecificConfig) cleanly overlays JSON overrides on top of generated configs for matching providers.

Two minor points:

1. The troubleshooting text in the port‑in‑use error is hardcoded to podman commands; since Settings.ContainerBinary is configurable, users running with docker or another tool will see misleading instructions. Consider either making the message generic (“your container tool”) or building the commands from Settings.ContainerBinary.

2. applyProviderOverrides only merges the first InitConfig entry. That matches today’s single‑config usage, but if you ever support multiple InitConfig entries per provider, you’ll probably want to extend the merge logic to handle them all.

Otherwise, the behavior is clear and scoped to hybrid mode as intended.

Also applies to: 70-151

---

326-388: Parallel startup tasks and volume cleanup behavior look correct

runParallelStartupTasks concurrently:

• Validates provider config,
• Creates the container volume, and
• Extracts default rulesets (if enabled),

then aggregates results via a buffered channel.

On any failure it:

• Removes the created volume (if any) via container volume rm, and
• Returns a wrapped error indicating which task failed.

The concurrency is straightforward (WaitGroup + buffered channel) and avoids races on volName/rulesetsDir. If you later add more startup tasks or a dedicated volume cleanup helper, this function should still compose nicely.

---

390-451: Hybrid builtin provider setup is correct; excludedDirs always set (even when empty)

setupBuiltinProviderHybrid:

• Skips WalkJavaPathForTarget for binary inputs (where decompilation happens in the container) and logs accordingly.
• For source inputs, computes javaTargetPaths and passes them as excludedDirs so builtin doesn’t double-count Java target output.
• Uses lib.GetProviderClient and initializes the builtin provider with optional additional configs.

One behavioral change vs containerless is that excludedDirs is always present in ProviderSpecificConfig, even when javaTargetPaths is empty. Assuming the builtin provider treats a nil/empty excludedDirs slice as “no exclusions,” this should be fine; if it doesn’t, you may want to only set the key when len(javaTargetPaths) > 0 to exactly mirror the containerless behavior.

---

466-918: Hybrid in-process analysis pipeline and progress integration look good end-to-end

RunAnalysisHybridInProcess now:

• Uses ProgressMode to gate logs, hide/show the cursor, and print progress messages and a final results summary.
• Detects binary inputs to adjust user-facing messages and correctly tweaks util.SourceMountPath only during volume creation, restoring it before client config.
• Runs providers in containers via RunProvidersHostNetwork, then performs parallel health checks using waitForProvider with proper context handling.
• Sets up network-based provider clients for all configured providers, transforms container paths in additional builtin configs back to host paths (with special handling for binary inputs), and then initializes the host-side builtin provider.
• Parallelizes rule loading across rulesets, aggregates results, and fails fast if no rules successfully load while tolerating partial failures with clear logging.
• Runs dependency analysis for full-mode Java runs, wires in the shared progress reporter from setupProgressReporter, and cleanly cancels/waits for the progress goroutine.
• Sorts and writes results, conditionally emits JSON, and triggers containerized static-report generation with timing logs and a concise results summary.

The lifecycle of providers and resources (including provider.Stop, spans, and cleanup delegated back to CleanAnalysisResources) is coherent, and progress behavior mirrors the containerless path. I don’t see correctness issues in this flow; the main remaining duplication concern is the extra static-report/JSON call in RunE, which is addressed separately in cmd/analyze.go.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7969ab2 and 318060f.

📒 Files selected for processing (5)

• cmd/analyze-bin.go (20 hunks)
• cmd/analyze-hybrid.go (20 hunks)
• cmd/analyze.go (10 hunks)
• cmd/command-context.go (4 hunks)
• cmd/progress_mode.go (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• cmd/command-context.go

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-08-08T15:06:52.056Z

Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.

Applied to files:

• cmd/analyze-hybrid.go

🧬 Code graph analysis (2)

cmd/analyze-bin.go (3)

cmd/progress_mode.go (1)

• NewProgressMode (18-20)

pkg/util/util.go (5)

• JavaArchive (82-82)
• WebArchive (83-83)
• EnterpriseArchive (84-84)
• ClassFile (85-85)
• JavaProvider (72-72)

pkg/provider/java.go (2)

• JavaProvider (18-20)
• WalkJavaPathForTarget (67-108)

cmd/analyze-hybrid.go (5)

pkg/util/util.go (6)

• ConfigMountPath (33-33)
• JavaArchive (82-82)
• WebArchive (83-83)
• EnterpriseArchive (84-84)
• ClassFile (85-85)
• SourceMountPath (31-31)

cmd/settings.go (2)

• Settings (24-24)
• Config (26-35)

pkg/provider/java.go (1)

• WalkJavaPathForTarget (67-108)

cmd/progress_mode.go (1)

• NewProgressMode (18-20)

cmd/analyze-bin.go (1)

• ConsoleHook (42-45)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Build & test from commit

🔇 Additional comments (5)

cmd/analyze-bin.go (1)

83-379: Containerless progress integration and logging flow look consistent

The containerless pipeline now:

• Wraps progress behavior via ProgressMode (cursor hide/show, conditional prints).
• Uses OperationalLogger for timing and operational messages so they don’t interfere with the progress bar.
• Detects binary vs source analysis for user‑facing messages.
• Avoids the console hook when progress is enabled.
• Wires the analyzer’s progress reporter via setupProgressReporter and correctly cancels and waits for the goroutine only when progress is enabled.

The nil handling for progressCancel (via ProgressMode.IsEnabled() checks) means you won’t dereference a nil cancel func when --no-progress is set, and the wait on progressDone only occurs when the channel is non‑nil.

Overall, the control flow and error propagation here look good.

cmd/analyze.go (2)

1118-1221: Static report containerization via GenerateStaticReport looks correct

GenerateStaticReport now:

• Uses operationalLog for status messages so you can suppress them when progress is enabled.
• Handles missing dependencies.yaml gracefully and logs the “source-only” case.
• Builds bulk and single-app argument lists correctly, re-mapping host paths into container mount paths for bulk YAML outputs.
• Runs js-bundle-generator in the runner image and copies the static-report assets into the output directory.
• Logs the final file:// URL through the operational logger.

The volume mappings and argument construction look consistent with the rest of the containerized flows, and error paths sensibly return errors instead of exiting the process.

---

1494-1576: Progress reporter context cancellation is properly implemented—approval stands

Verification confirms the implementation is sound:

When NewChannelReporter receives context cancellation, it spawns a goroutine that monitors ctx.Done() and calls Close() to close the events channel. The Close() method safely closes the channel after setting the closed flag, ensuring the for-range loop in setupProgressReporter will terminate properly without deadlock or panic.

The shutdown semantics are correct as-is. The optional nil-check suggestion for progressCancel() call sites remains defensible but unnecessary given the existing alignment between noProgress and actual nil/non-nil values.

cmd/analyze-hybrid.go (2)

153-202: Provider health check with exponential backoff is sound

waitForProvider:

• Polls localhost:port with a bounded exponential backoff (50ms–500ms).
• Honors context cancellation on both the main polling loop and the sleep.
• Returns a detailed troubleshooting error if the provider never becomes ready.

This is a solid improvement over fixed sleeps and should behave well even under transient startup delays.

---

204-325: Manual verification needed: Proxy field handling in external provider package

The external provider package (github.com/konveyor/analyzer-lsp/provider/lib) behavior cannot be verified from this repository. However, investigation reveals a potential concern:

The local codebase shows inconsistent Proxy assignment patterns:

• cmd/analyze-bin.go sets Config.Proxy (top-level) at lines 608, 720, 756
• cmd/analyze-hybrid.go setupNetworkProvider sets only InitConfig[0].Proxy (line 278), not Config.Proxy

The setupNetworkProvider function has a comment stating "Keep as pointer - InitConfig.Proxy is *Proxy!" suggesting awareness of the per-InitConfig field, but this differs from the top-level pattern used elsewhere.

To confirm this code is correct, verify with the analyzer-lsp provider package maintainers that:

1. Both Config.Proxy and InitConfig.Proxy fields are honored when creating network clients
2. If both are set, which takes precedence and is that intentional
3. Whether setupNetworkProvider should also set the top-level Config.Proxy field

[shawn-hurley]

@eemcmullan Will leave for you to merge when you are good to go!