[SP-3605] feat: enhanced installation scripts for all platforms, add update notification component #114
Conversation
matiasdaloia
added
documentation
WalkthroughAdds a cross-platform auto-update subsystem (interface, implementation, Wails bindings, frontend UI), new multi‑platform installer scripts and shared libs, unified CI/release workflow with SHA256 generation, many autogenerated test mocks, removes fsnotify watcher and MD5 use, renames project artifacts/Makefile targets, and adds installation docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant FE as Frontend
participant UI as UpdateNotification
participant Svc as UpdateService
participant GH as GitHub
participant FS as FileSystem
User->>FE: launch app
FE->>UI: mount → CheckForUpdate()
UI->>Svc: CheckForUpdate()
Svc->>GH: GET /releases/latest
GH-->>Svc: release metadata + assets + SHA256SUMS
Svc->>Svc: semver compare
alt newer release found
Svc-->>UI: UpdateInfo (includes expected_sha256)
UI->>User: show toast / dialog
User->>UI: "Download Update"
UI->>Svc: DownloadUpdate(UpdateInfo)
Svc->>GH: stream asset download
GH-->>Svc: binary stream
Svc->>FS: write temp file + compute SHA256
Svc->>Svc: verify checksum
Svc-->>UI: downloaded path
User->>UI: "Restart & Update"
UI->>Svc: ApplyUpdate(path)
Svc->>FS: run installer/replace and trigger restart
else no update
Svc-->>UI: Available=false
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
INSTALLATION.md (1)
63-68: Consider adding a safety note for manual uninstallation.The manual uninstallation instructions use
sudo rm -rfwhich is potentially dangerous if paths are mistyped. Consider adding a warning or suggesting users verify the paths before executing.Apply this diff to add a safety note:
**Uninstallation:** The PKG installer does not include an automatic uninstaller. To remove: ```bash +# Verify the paths exist before removing +ls -la "/Applications/SCANOSS Code Compare.app" +ls -la /usr/local/bin/scanoss-cc + +# Then remove (use with caution) sudo rm -rf "/Applications/SCANOSS Code Compare.app" sudo rm /usr/local/bin/scanoss-cc</blockquote></details> <details> <summary>Makefile (1)</summary><blockquote> `57-61`: **Verify assets directory structure for cp command.** The `cp $(ASSETS_DIR)/* $(BUILD_DIR)/assets` command will fail if the assets directory contains subdirectories. Consider using `cp -R` for robustness. Apply this diff to handle potential subdirectories: ```diff cp_assets: ## Copy the necessary assets to the build folder @echo "Copying assets to build directory..." @mkdir -p $(BUILD_DIR)/assets - @cp $(ASSETS_DIR)/* $(BUILD_DIR)/assets + @cp -R $(ASSETS_DIR)/* $(BUILD_DIR)/assets/frontend/src/components/UpdateNotification.tsx (1)
61-65: Surface update-check failures to the user.Right now a failed check is only logged to the console. If the updater endpoint is unreachable, users just assume they’re current. Mirror the other error paths and toast this failure so they get immediate feedback and can retry.
Apply this diff inside the
catch:} catch (error) { console.error('Failed to check for updates:', error); + toast({ + variant: 'destructive', + title: 'Update check failed', + description: 'Could not reach the update service. Please try again later.', + }); } finally { setChecking(false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.github/workflows/release.yml(8 hunks).pre-commit-config.yaml(1 hunks)INSTALLATION.md(1 hunks)Makefile(3 hunks)README.md(1 hunks)backend/entities/file.go(0 hunks)backend/entities/version.go(1 hunks)backend/service/update_service.go(1 hunks)backend/service/update_service_impl.go(1 hunks)frontend/src/components/SettingsDialog.tsx(1 hunks)frontend/src/components/StatusBar.tsx(2 hunks)frontend/src/components/UpdateNotification.tsx(1 hunks)frontend/wailsjs/go/models.ts(1 hunks)frontend/wailsjs/go/service/UpdateServiceImpl.d.ts(1 hunks)frontend/wailsjs/go/service/UpdateServiceImpl.js(1 hunks)main.go(3 hunks)scripts/macos/conclusion.html(1 hunks)scripts/macos/distribution.xml(1 hunks)scripts/macos/license.html(1 hunks)scripts/macos/scripts/postinstall(1 hunks)scripts/macos/scripts/preinstall(1 hunks)scripts/macos/welcome.html(1 hunks)scripts/windows/installer.nsi(1 hunks)
💤 Files with no reviewable changes (1)
- backend/entities/file.go
🧰 Additional context used
🧬 Code graph analysis (7)
frontend/src/components/UpdateNotification.tsx (4)
frontend/src/components/ui/use-toast.ts (2)
useToast(192-192)toast(192-192)backend/entities/version.go (1)
UpdateInfo(32-39)frontend/wailsjs/go/models.ts (1)
UpdateInfo(830-870)frontend/wailsjs/go/service/UpdateServiceImpl.js (3)
CheckForUpdate(9-11)DownloadUpdate(13-15)ApplyUpdate(5-7)
frontend/src/components/StatusBar.tsx (1)
frontend/src/components/UpdateNotification.tsx (1)
UpdateNotification(34-177)
backend/service/update_service.go (3)
frontend/wailsjs/go/service/UpdateServiceImpl.js (5)
CheckForUpdate(9-11)DownloadUpdate(13-15)ApplyUpdate(5-7)GetCurrentVersion(17-19)SetContext(21-23)backend/entities/version.go (1)
UpdateInfo(32-39)frontend/wailsjs/go/models.ts (1)
UpdateInfo(830-870)
frontend/wailsjs/go/models.ts (1)
backend/entities/version.go (1)
UpdateInfo(32-39)
main.go (2)
backend/service/update_service_impl.go (1)
NewUpdateService(57-64)frontend/wailsjs/go/service/UpdateServiceImpl.js (1)
SetContext(21-23)
backend/service/update_service_impl.go (4)
backend/service/update_service.go (1)
UpdateService(33-48)backend/entities/version.go (2)
UpdateInfo(32-39)AppVersion(29-29)frontend/wailsjs/go/models.ts (1)
UpdateInfo(830-870)frontend/wailsjs/runtime/runtime.js (1)
Quit(184-186)
frontend/wailsjs/go/service/UpdateServiceImpl.d.ts (3)
frontend/wailsjs/go/service/UpdateServiceImpl.js (5)
ApplyUpdate(5-7)CheckForUpdate(9-11)DownloadUpdate(13-15)GetCurrentVersion(17-19)SetContext(21-23)backend/entities/version.go (1)
UpdateInfo(32-39)frontend/wailsjs/go/models.ts (1)
UpdateInfo(830-870)
🪛 markdownlint-cli2 (0.18.1)
README.md
83-83: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
88-88: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (21)
.pre-commit-config.yaml (1)
3-3: Version v0.3.1 is valid but lacks release notes—verify this change was intentional and tested.Both v0.3.1 and v0.2.0 exist as legitimate releases in the scanoss/pre-commit-hooks repository. However, v0.3.1 has no documented release notes or changelog. Additionally, the version bump skips v0.3.0 entirely (which also lacks documentation), jumping directly from v0.2.0 to v0.3.1.
Confirm:
- This version jump was intentional (not a typo or version mismatch)
- Changes in v0.3.1 have been tested in CI/CD
- No breaking changes affect your pipeline or workflow
INSTALLATION.md (1)
1-278: LGTM! Comprehensive and well-structured installation guide.The installation guide is thorough, covering all major platforms with clear instructions, troubleshooting steps, and verification commands. The documentation is consistent with README.md and provides good user experience with platform-specific guidance.
README.md (1)
27-90: LGTM! Well-documented installation instructions.The expanded installation section provides clear, platform-specific guidance with concrete commands and verification steps. The auto-update documentation is helpful for users.
Note: The markdownlint warnings about emphasis vs. headings on lines 83 and 88 appear to be false positives, as the bold text is used for feature labels within a list, which is appropriate formatting.
scripts/windows/installer.nsi (2)
242-243: App Paths registration is a good practice.Registering the CLI executable in App Paths (lines 242-243) allows Windows to find
scanoss-cc.exeeven without modifying PATH. This is excellent UX as users can run the command from any location.
247-247: The review comment is incorrect.APP_VERSIONis properly defined as a preprocessor parameter passed tomakensisvia the-DAPP_VERSION=$APP_VERSIONflag in the build workflow (.github/workflows/release.yml:404). NSIS scripts receive such parameters as compile-time constants, making${APP_VERSION}available throughout the installer script.Likely an incorrect or invalid review comment.
.github/workflows/release.yml (2)
322-330: AppRun script implementation looks solid.The AppRun script correctly sets up the environment (PATH and LD_LIBRARY_PATH) and executes the binary with proper path resolution using
readlink -f.
449-490: Good conditional logic for installer handling.The dynamic discovery and conditional signing of the Windows installer (lines 449-464) provides good flexibility. The workflow handles cases where the installer may or may not be present, which aids in debugging and incremental development.
frontend/src/components/StatusBar.tsx (1)
32-32: LGTM! Clean integration of update notification.The UpdateNotification component is properly imported and integrated into the status bar alongside the existing AppSettings component. The placement in the right-side cluster is appropriate for a notification feature.
Also applies to: 58-58
scripts/macos/license.html (1)
1-22: LGTM! Simple and appropriate license page.The HTML license page is clean and appropriate for the macOS installer context. It provides proper attribution and user guidance while maintaining simple, accessible styling.
frontend/src/components/SettingsDialog.tsx (1)
53-55: Remove this review comment — the code correctly delegates scrolling to react-arborist's virtualized rendering.The removal of ScrollArea is intentional and not problematic. The SkipSettings component uses the proper flexbox pattern (
minBlockSize: 0) to constrain height, and ResultsTree passes width/height props to react-arborist, which handles scrolling internally through virtualization. This is the correct implementation for virtualized components—it's more efficient than manual scroll wrappers and avoids double-scrolling issues. The parentoverflow-hiddenworks as expected with content that respects height constraints.Likely an incorrect or invalid review comment.
frontend/wailsjs/go/models.ts (1)
830-870: LGTM! Auto-generated model aligns with backend.The UpdateInfo class correctly mirrors the backend struct with appropriate TypeScript types. The
published_atfield is typed asanyto accommodate Go'stime.TimeJSON serialization.backend/entities/version.go (1)
26-39: LGTM! Well-structured entity definition.The UpdateInfo struct is properly documented and uses appropriate types. The
time.Timefor PublishedAt andint64for Size are correct choices for their respective data.main.go (3)
104-104: LGTM! Update service properly instantiated.The update service is created using the standard constructor pattern.
118-118: LGTM! Context properly wired during startup.The update service context is set during the app's OnStartup lifecycle, consistent with other services.
123-135: LGTM! Service properly bound for frontend access.The update service is correctly included in the Bind list. The change from
[]interface{}to[]anyis a modern Go idiom (they are equivalent).frontend/wailsjs/go/service/UpdateServiceImpl.d.ts (1)
1-14: LGTM! Auto-generated TypeScript declarations.The function signatures correctly expose the backend UpdateService API with appropriate Promise-based types.
frontend/wailsjs/go/service/UpdateServiceImpl.js (1)
1-23: LGTM! Auto-generated JavaScript wrapper.The wrapper correctly delegates all method calls to the Go backend through the Wails runtime bridge.
backend/service/update_service_impl.go (4)
45-64: LGTM! Reasonable defaults for update service.The 30-second timeout for API calls and 10-minute timeout for downloads are appropriate. Using
os.TempDir()for downloads is standard practice.
84-162: LGTM! Robust update checking logic.The method properly handles errors, validates responses, and cleans up resources. The version comparison and asset selection logic is appropriate for the use case.
328-330: LGTM! Simple version accessor.Returns the build-time version. If empty, CheckForUpdate appropriately handles it with an error.
333-344: LGTM! Platform-specific asset naming.The asset name patterns are reasonable and should work with the GitHub releases mentioned in the PR. Ensure release assets follow these naming conventions.
Verify that GitHub releases include assets matching these patterns:
- macOS:
*-Installer.pkg- Windows:
*-Setup.exe- Linux:
*.AppImage
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Makefile (1)
57-64: Add safety checks for source directories.Both
cp_assetsandcp_scriptsassume the source directories exist. If$(ASSETS_DIR)or$(SCRIPTS_DIR)are missing or empty, thecpcommands will fail. Consider adding existence checks or using more defensive patterns:cp_assets: ## Copy the necessary assets to the build folder @echo "Copying assets to build directory..." @mkdir -p $(BUILD_DIR)/assets + @test -d $(ASSETS_DIR) || (echo "Error: $(ASSETS_DIR) not found" && exit 1) @cp $(ASSETS_DIR)/* $(BUILD_DIR)/assets cp_scripts: ## Copy the necessary scripts to the build folder @echo "Copying scripts to build directory..." + @test -d $(SCRIPTS_DIR) || (echo "Error: $(SCRIPTS_DIR) not found" && exit 1) @cp -R $(SCRIPTS_DIR)/* $(BUILD_DIR)/.github/workflows/release.yml (1)
294-338: Consider verifying appimagetool download integrity.The AppImage creation flow correctly follows AppImage conventions. However, the appimagetool is downloaded from a continuous release without verification (line 333). While this is common practice, consider adding checksum verification for enhanced security:
# Download appimagetool wget -q https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage + # Optionally verify checksum (would need to fetch expected hash) chmod +x appimagetool-x86_64.AppImageAlternatively, pin to a specific appimagetool release version instead of
continuousfor reproducible builds.backend/service/update_service_impl.go (1)
138-160: Asset selection could match unintended files.Line 145 uses
strings.Contains(asset.Name, assetName)to find platform-specific assets. This could cause false positives if asset names contain the pattern as a substring rather than as the expected suffix.Use suffix matching for more precise asset selection:
for _, asset := range release.Assets { - if strings.Contains(asset.Name, assetName) { + if strings.HasSuffix(asset.Name, assetName) { downloadURL = asset.BrowserDownloadURL size = asset.SizeThis ensures
-Installer.pkgonly matches files ending with that suffix, not files likePre-Installer.pkg.txt.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
.github/workflows/release.yml(8 hunks)Makefile(3 hunks)backend/entities/version.go(1 hunks)backend/service/update_service_impl.go(1 hunks)go.mod(1 hunks)scripts/macos/distribution.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/macos/distribution.xml
🧰 Additional context used
🧬 Code graph analysis (1)
backend/service/update_service_impl.go (4)
backend/service/update_service.go (1)
UpdateService(33-48)frontend/wailsjs/go/service/UpdateServiceImpl.js (5)
SetContext(21-23)CheckForUpdate(9-11)GetCurrentVersion(17-19)DownloadUpdate(13-15)ApplyUpdate(5-7)backend/entities/version.go (2)
UpdateInfo(32-40)AppVersion(29-29)frontend/wailsjs/runtime/runtime.js (1)
Quit(184-186)
⏰ 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
🔇 Additional comments (18)
Makefile (2)
10-10: GOTOOLCHAIN setting is correct per previous review resolution.The export matches go.mod's
go 1.23.0directive as requested in the previous review. Note that go.mod also specifiestoolchain go1.23.2on line 5, but the previous review explicitly resolved to use 1.23.0 for reproducible builds aligned with the minimum version requirement.
66-68: Build dependencies are correctly scoped.The
buildtarget depends oncp_assetsbut notcp_scripts, which is appropriate since scripts are only needed for platform-specific packaging (macOS .pkg, Windows NSIS). The workflows correctly callcp_scriptsexplicitly when required (e.g., release.yml lines 143, 392).go.mod (1)
22-22: Dependency addition is appropriate.The
github.com/Masterminds/semver/v3library is correctly added to support semantic version comparison in the update service. This replaces the previous custom version parsing logic and provides robust handling of semantic versioning edge cases.backend/entities/version.go (1)
31-40: UpdateInfo entity is well-structured.The struct design is clean and appropriate for update metadata exchange:
- JSON tags are correctly specified for all fields
PublishedAtastime.Timewill serialize to RFC3339 formatExpectedSHA256correctly usesomitemptysince it's optional- All fields are properly exported for Wails frontend bindings
.github/workflows/release.yml (4)
181-201: macOS .pkg notarization flow is correctly implemented.The notarization sequence follows Apple's best practices:
- Uses modern
notarytool(not legacyaltool)--waitflag ensures synchronous completion- Staples the notarization ticket to the installer
- Validates the stapled ticket before proceeding
391-406: Windows NSIS build correctly addresses previous review.The addition of
make cp_scriptson line 392 ensures the NSIS installer script is copied tobuild/windows/beforemakensisexecutes. This resolves the critical issue from the previous review.
450-491: Windows installer signing and packaging flow is well-designed.The conditional installer discovery and signing logic (lines 450-478) gracefully handles scenarios where no installer is present, while the zip creation (lines 479-491) correctly includes the signed installer when available. This provides flexibility for different build configurations.
135-180: Verification complete: distribution.xml is properly copied.The
make cp_scriptstarget copiesscripts/macos/distribution.xmltobuild/macos/distribution.xmlviacp -R scripts/* build/, preserving the directory structure. The workflow correctly references the copied file in theproductbuildcommand.backend/service/update_service_impl.go (10)
58-66: Constructor is appropriately configured.The
NewUpdateServiceconstructor sets sensible defaults:
- 30-second HTTP timeout for API calls (separate from the 10-minute download timeout used in
DownloadUpdate)- Uses system temp directory for downloads
87-136: Update check logic is correctly implemented.The version checking flow properly handles:
- Missing current version validation
- Version normalization (trimming "v" prefix)
- Semantic version comparison via helper function
- Graceful response when no update is available (
Available: false)
180-246: Download and verification implementation is excellent.This function correctly addresses all previously flagged security concerns:
- Mandatory SHA256 checksum verification (lines 230-241)
- Proper error handling with temp file cleanup on all failure paths
- Efficient streaming with simultaneous hashing via
io.MultiWriter- Case-insensitive checksum comparison to avoid subtle matching errors
- Explicit rejection if no expected checksum is provided (line 230-233)
249-262: Platform dispatcher is cleanly implemented.The platform-specific update dispatch correctly delegates to dedicated handlers for macOS, Windows, and Linux, with appropriate error handling for unsupported platforms.
264-277: macOS update application is correctly implemented.Uses the standard
opencommand to launch the installer and properly checks the error fromcmd.Start()before quitting the application.
294-306: Linux update dispatcher correctly handles multiple formats.The extension-based dispatch for AppImage and .deb packages is appropriate for Linux's diverse packaging ecosystem.
308-346: AppImage update correctly addresses previous review concerns.The implementation now properly checks the restart error (lines 339-341), resolving the critical issue from the previous review. The backup/restore logic provides good safety against failed updates.
348-360: .deb update application is appropriately implemented.Uses
xdg-opento launch the system's default package manager, allowing the user to review and approve the .deb installation.
382-392: Version comparison now uses industry-standard library.The replacement of custom version parsing with
github.com/Masterminds/semver/v3correctly addresses the critical issue from the previous review. This library properly handles semantic versioning, pre-release versions, and build metadata.
368-379: Asset name patterns align with release workflow.The platform-specific asset suffixes match the naming conventions used in the release workflow:
- macOS:
-Installer.pkg(release.yml line 166)- Windows:
-Setup.exe(release.yml line 405)- Linux:
.AppImage(release.yml line 337)
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/windows/installer.nsi (1)
1-22: Document the APP_VERSION requirement.The script references
${APP_VERSION}at lines 248 and 255 but doesn't define it in the header. This must be passed via-DAPP_VERSIONwhen invoking makensis, which the workflow does at line 338. Consider adding a comment in the header section documenting this requirement.; Application Settings !define APP_NAME "SCANOSS Code Compare" !define APP_EXE "scanoss-cc-windows.exe" !define APP_CLI_NAME "scanoss-cc" !define COMPANY_NAME "SCANOSS" !define INSTALL_DIR_NAME "SCANOSS" !define REG_UNINSTALL_KEY "Software\Microsoft\Windows\CurrentVersion\Uninstall\${APP_NAME}" +; Note: APP_VERSION must be passed via command line: makensis -DAPP_VERSION=x.y.z.github/workflows/release.yml (1)
226-273: AppImage creation looks good, but consider pinning appimagetool version.The AppImage creation follows best practices with proper AppDir structure, .desktop file, and AppRun script. However, downloading appimagetool from the
continuousrelease (line 268) may introduce instability if the latest version has breaking changes.Consider pinning to a specific appimagetool release version:
- wget -q https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage + wget -q https://github.com/AppImage/AppImageKit/releases/download/13/appimagetool-x86_64.AppImage
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/release.yml(7 hunks).pre-commit-config.yaml(1 hunks)backend/service/update_service_impl.go(1 hunks)scripts/windows/installer.nsi(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .pre-commit-config.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
backend/service/update_service_impl.go (3)
backend/service/update_service.go (1)
UpdateService(33-48)backend/entities/version.go (2)
UpdateInfo(32-40)AppVersion(29-29)frontend/wailsjs/runtime/runtime.js (1)
Quit(184-186)
🔇 Additional comments (10)
scripts/windows/installer.nsi (4)
54-82: LGTM!The PATH manipulation logic is correct. It properly checks for existing entries before appending and broadcasts the environment change to notify the system.
84-132: LGTM!The PATH removal logic correctly handles edge cases like trailing semicolons and properly reconstructs the PATH variable after removal.
243-244: LGTM!The App Paths registration creates a convenient alias allowing users to invoke
scanoss-cc.exefrom the command line while the actual binary is namedscanoss-cc-windows.exe.
264-290: LGTM!The uninstall section properly reverses all installation actions, including PATH removal, registry cleanup, file deletion, and shortcut removal.
.github/workflows/release.yml (4)
326-339: LGTM! The cp_scripts concern from the previous review is now addressed.The Windows job now correctly calls both
make cp_assets(line 326) andmake cp_scripts(line 327) before building the NSIS installer. This ensuresscripts/windows/installer.nsiis copied tobuild/windows/before makensis executes at line 338.
383-424: LGTM!The installer signing flow is well-designed with conditional execution based on whether the installer is found. This gracefully handles cases where the NSIS build might fail while still allowing the main executable signing to proceed.
452-475: LGTM!The release creation properly includes AppImage artifacts alongside ZIP files. The use of null-delimited find output and read is a good practice for handling filenames safely.
99-99: INSTALL_MACOS.md exists and is correctly referenced.Verification confirms the file exists in the repository root and contains valid documentation for macOS installation. The workflow reference at line 99 is valid.
backend/service/update_service_impl.go (2)
193-245: Guard against nil parent context before WithTimeout.
context.WithTimeoutpanics when the parent context isnil. IfDownloadUpdateruns beforeSetContexthas been called (or if the caller accidentally passesnil), the service will crash here instead of returning an error. Please seed the service with a non‑nil default (e.g., initializectxtocontext.Background()in the constructor or fall back right beforeWithTimeout) so the update flow can’t bring down the process. (willem.dev)A minimal fix inside this method would look like:
- ctx, cancel := context.WithTimeout(u.ctx, downloadTimeout) + parent := u.ctx + if parent == nil { + parent = context.Background() + } + ctx, cancel := context.WithTimeout(parent, downloadTimeout)
317-333: Handle cross-device rename when replacing the AppImage.The code renames the freshly downloaded AppImage (stored under
os.TempDir()) over the running executable. On many distros/tmpis a different mount (tmpfs), soos.RenamereturnsEXDEV, leaving users stuck on the old binary. Detect that error and fall back to copying into place (or download the update into the executable’s directory to begin with) so Linux updates succeed regardless of mount layout. (unix.stackexchange.com)
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
INSTALLATION.md (1)
108-140: Clarify Linux installation process and binary naming.The Linux section has multiple clarity issues that could confuse users:
- Inconsistent section titles: Sections reference "(.deb)" and "(.rpm)" but the instructions describe downloading and extracting a ZIP file, not installing .deb or .rpm packages directly.
- Missing details: No mention of setting executable permissions (
chmod +x), which is critical on Linux.- Unclear binary name: The extracted binary name is not specified. Users need to know the exact executable name to reference in PATH.
- Missing verification: Unlike macOS and Windows, there's no verification step showing users how to confirm the installation.
- Vague AUR guidance: The Arch section is too brief and non-actionable.
Revise the Linux section to clarify the actual packaging strategy and provide consistent, detailed instructions. Consider whether these are truly distribution-specific packages or a single cross-distribution ZIP with universal binaries. Align the presentation with the macOS and Windows sections.
backend/service/update_service_impl.go (1)
277-277: Consider validating external command availability.The code relies on external commands (
unzip,open,xdg-open) without checking if they exist, which could lead to unclear error messages if a command is missing.Add a helper function to validate command availability before use:
// validateCommand checks if a command is available in PATH func validateCommand(name string) error { _, err := exec.LookPath(name) if err != nil { return fmt.Errorf("required command '%s' not found in PATH", name) } return nil }Then call it before using each command:
+ // Validate unzip is available + if err := validateCommand("unzip"); err != nil { + return err + } + // Use unzip command to extract cmd := exec.Command("unzip", "-o", updatePath, "-d", extractDir)This provides clearer error messages if a required tool is missing.
Also applies to: 359-359
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
.github/workflows/release.yml(6 hunks)INSTALLATION.md(1 hunks)README.md(1 hunks)backend/service/update_service_impl.go(1 hunks)go.mod(1 hunks)scripts/windows/installer.nsi(1 hunks)wails.json(0 hunks)
💤 Files with no reviewable changes (1)
- wails.json
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- scripts/windows/installer.nsi
🧰 Additional context used
🧬 Code graph analysis (1)
backend/service/update_service_impl.go (4)
backend/service/update_service.go (1)
UpdateService(33-48)frontend/wailsjs/go/service/UpdateServiceImpl.js (5)
SetContext(21-23)CheckForUpdate(9-11)GetCurrentVersion(17-19)DownloadUpdate(13-15)ApplyUpdate(5-7)backend/entities/version.go (2)
UpdateInfo(32-40)AppVersion(29-29)frontend/wailsjs/runtime/runtime.js (1)
Quit(184-186)
🪛 markdownlint-cli2 (0.18.1)
INSTALLATION.md
55-55: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
60-60: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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). (4)
- GitHub Check: Build for Windows
- GitHub Check: Build for MacOS
- GitHub Check: Build for Linux
- GitHub Check: build
🔇 Additional comments (6)
INSTALLATION.md (1)
67-67: Verify external file reference.Line 67 references
INSTALL_MACOS.md, which is not included in this PR. Confirm that:
- This file exists in the repository
- If it exists, determine whether it needs updates to align with the new installation flow
- If it doesn't exist, either create it or remove the reference and consolidate content into INSTALLATION.md
README.md (1)
40-40: Verify reference to INSTALL_MACOS.md.Line 40 references
INSTALL_MACOS.md, which is not included in this PR. This is the same issue flagged in INSTALLATION.md. Verify that this file exists in the repository or consolidate its content into the primary documentation.backend/service/update_service_impl.go (1)
436-447: Asset selection only finds ZIP files for Linux, not DEB packages.The
getAssetNameForPlatformfunction returns"-linux.zip"for Linux (line 443), butapplyUpdateLinux(lines 328-340) also supports.debpackages. This means.debupdates will never be found byCheckForUpdate, even though the installation logic exists.Clarify the intended Linux distribution strategy. If
.debpackages should be supported for updates, modify the asset selection logic:func getAssetNameForPlatform() string { switch runtime.GOOS { case "darwin": return "-mac.zip" // macOS DMG in a ZIP file case "windows": return "-Setup.exe" // Windows installer case "linux": - return "-linux.zip" // Linux binary in a ZIP file + // Try to detect distribution type and prefer .deb if on Debian-based system + if _, err := exec.LookPath("dpkg"); err == nil { + return ".deb" + } + return "-linux.zip" default: return "" } }Alternatively, if ZIP is the only supported auto-update format for Linux, remove the
.debcase fromapplyUpdateLinuxand document that.debpackages require manual installation..github/workflows/release.yml (3)
271-272: ✅ Past review comment addressed:make cp_scriptscorrectly positioned.The previous review flagged that
scripts/windows/installer.nsiwas not being copied tobuild/windows/before makensis runs. Line 272 now correctly addsmake cp_scriptsaftermake cp_assetsand within the bash build context, ensuring the installer script is present before Scoop/NSIS installation and the subsequent build step.
336-351: Conditional installer signing relies on output path assumption.The "Find and Sign Windows Installer" step (lines 336–351) searches for
*-Setup.exefiles usingfind. If the NSIS build at line 291 does not produce a file at the expected location (e.g., due to misconfiguration, missingdist/directory, or differentOutFilepath in the.nsiscript), the search returns nothing andINSTALLER_FOUNDis set tofalse. The subsequent signing step skips silently, leaving an unsigned installer in the final artifact if one was created.Confirm that the NSIS output path is predictable and add logging or validation to catch mismatches.
288-292: The primary concern in the review comment is incorrect; the .nsi script explicitly defines OutFile.The review claims the .nsi script does not specify an
OutFilepath, but verification shows it explicitly definesOutFile "..\..\dist\${APP_NAME}-Setup.exe"at line 18 ofscripts/windows/installer.nsi. Themake cp_scriptsstep (line 272) copies this script tobuild/windows/, and the relative path resolves correctly to the repository root'sdist/directory.However, one valid concern remains: the makensis command at line 291 has no error handling. If the build fails, the workflow proceeds to the artifact upload step at line 299, which silently fails if the expected file doesn't exist. Adding error handling would improve reliability:
$APP_VERSION = (git tag --sort=-version:refname | Select-Object -First 1) -replace '^v', '' makensis -DAPP_VERSION=$APP_VERSION build/windows/installer.nsi if ($LASTEXITCODE -ne 0) { exit 1 }Likely an incorrect or invalid review comment.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
INSTALLATION.md (1)
55-65: Fix Markdown heading violations for DMG CLI setup options.Lines 55 and 60 violate MD036 linting rules by using bold emphasis (
**...**) instead of proper Markdown headings. These should be semantic subheadings (###) for consistency with document structure.Apply this diff to fix the issues:
-**Option 1: Symlink (Recommended)** +### Option 1: Symlink (Recommended) + ```bash sudo ln -s "/Applications/SCANOSS Code Compare.app/Contents/MacOS/SCANOSS Code Compare" /usr/local/bin/scanoss-cc-Option 2: Add to PATH
+### Option 2: Add to PATH
+echo 'export PATH="/Applications/SCANOSS Code Compare.app/Contents/MacOS:$PATH"' >> ~/.zshrc echo 'alias scanoss-cc="/Applications/SCANOSS Code Compare.app/Contents/MacOS/SCANOSS Code Compare"' >> ~/.zshrc source ~/.zshrc</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between efecbe44931abdae9c4e652c6989fbcb6481f1e9 and c92cce4f1f6b5d6f28b2e148c8b8cf62646ba77b. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `.github/workflows/release.yml` (6 hunks) * `INSTALLATION.md` (1 hunks) * `Makefile` (3 hunks) * `README.md` (1 hunks) * `backend/service/update_service_impl.go` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * backend/service/update_service_impl.go </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>INSTALLATION.md</summary> 11-11: Link fragments should be valid (MD051, link-fragments) --- 55-55: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 60-60: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 83-83: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 88-88: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 195-195: Bare URL used (MD034, no-bare-urls) </details> </details> </details> <details> <summary>⏰ 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). (5)</summary> * GitHub Check: unit_tests * GitHub Check: integration-tests * GitHub Check: Build for Windows * GitHub Check: Build for MacOS * GitHub Check: Build for Linux </details> <details> <summary>🔇 Additional comments (8)</summary><blockquote> <details> <summary>README.md (1)</summary><blockquote> `65-79`: **Auto-Updates and Verification sections look good.** The verification steps are consistent across platforms, and the Auto-Updates description clearly explains the feature to end users. </blockquote></details> <details> <summary>Makefile (3)</summary><blockquote> `7-10`: **GOTOOLCHAIN alignment with go.mod looks correct.** Line 10 specifies `go1.23.0`, which should match the go.mod directive. The new `ASSETS_DIR` variable (line 7) provides a clean way to manage asset paths. --- `43-47`: **Lint target simplification improves UX.** Renaming `go_lint_local` to `lint` and `go_lint_local_fix` to `lint-fix` aligns with common make conventions and is more intuitive for users. --- `57-60`: **Asset copying refactoring is clean and consistent.** The cp_assets target properly creates the build directory structure and copies assets in a centralized location. This supports the workflow integration and ensures assets are available before build steps. </blockquote></details> <details> <summary>.github/workflows/release.yml (4)</summary><blockquote> `50-54`: **Centralized asset preparation through make target improves maintainability.** Using `make cp_assets` instead of inline shell commands ensures consistency across all build jobs and makes the build process more maintainable by referencing the Makefile source of truth. --- `216-220`: **Consistent asset preparation across all platform builds.** Both Linux (line 218) and Windows (line 271) builds use the same `make cp_assets` pattern, ensuring uniform asset handling across platforms. Please verify that the `build/assets` directory structure created by `make cp_assets` is correctly referenced by the wails build process across all platforms. If wails expects assets in a different location or with a different structure for any platform, the asset copy logic may need platform-specific adjustments. Also applies to: 269-273 --- `302-327`: **Windows code signing workflow is clear and properly structured.** The workflow correctly downloads the unsigned executable, signs it with CodeSignTool, and creates a ZIP of the signed artifact. The step naming is descriptive. --- `349-364`: **Release creation logic properly aggregates all platform artifacts.** The release creation step correctly discovers all `*.zip` files from the three platform build jobs (macOS, Linux, Windows) and includes them in the release. The use of an array and proper shell expansion ensures all artifacts are included. Please confirm that the `find . -name "*.zip" -print0` command correctly locates all three platform artifacts downloaded in the previous steps. If artifacts are stored in different directories or have different naming conventions, this glob pattern may need adjustment. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
backend/service/update_service_impl.go (3)
363-367: Handle restore operation error.If the executable replacement fails at line 363, the backup restore at line 365 ignores any error. If both operations fail, the user could be left with a broken installation.
Apply this diff:
if err := os.Rename(newBinaryPath, currentExe); err != nil { // Restore backup on failure - os.Rename(backupPath, currentExe) + if restoreErr := os.Rename(backupPath, currentExe); restoreErr != nil { + return fmt.Errorf("failed to replace executable: %w (restore also failed: %v)", err, restoreErr) + } return fmt.Errorf("failed to replace executable: %w", err) }
421-431: Binary detection logic remains fragile.The check at line 426 relies on executable bit preservation and substring matching. The executable bit may not survive ZIP extraction, and the substring match is brittle.
Consider a more robust approach that checks for expected binary names first, then falls back to file size or other heuristics:
// Find the binary in the extracted directory var newBinaryPath string err = filepath.Walk(extractDir, func(path string, info os.FileInfo, err error) error { if err != nil { return err } - // Look for executable files that are not directories - if !info.IsDir() && (info.Mode()&0o111 != 0 || strings.Contains(info.Name(), "scanoss-cc")) { + // Look for the expected binary by name + baseName := filepath.Base(info.Name()) + if !info.IsDir() && (baseName == "scanoss-cc" || baseName == "scanoss-cc-linux") && info.Size() > 1024*1024 { newBinaryPath = path return filepath.SkipAll } return nil })
451-455: Handle restore operation error in Linux update path.Similar to the Windows update path, the backup restore at line 453 ignores errors. This should be handled consistently.
Apply this diff:
if err := os.Rename(newBinaryPath, currentExe); err != nil { // Restore backup on failure - os.Rename(backupPath, currentExe) + if restoreErr := os.Rename(backupPath, currentExe); restoreErr != nil { + return fmt.Errorf("failed to replace executable: %w (restore also failed: %v)", err, restoreErr) + } return fmt.Errorf("failed to replace executable: %w", err) }INSTALLATION.md (3)
117-126: Fix Markdown heading violations.Lines 117 and 122 use bold emphasis instead of proper Markdown headings, violating MD036.
Apply this diff:
**Setting Up CLI Access:** After installing via DMG, you need to manually set up CLI access. Choose one of these methods: -**Option 1: Symlink (Recommended)** +### Option 1: Symlink (Recommended) + ```bash sudo ln -s "/Applications/scanoss-cc.app/Contents/MacOS/scanoss-cc" /usr/local/bin/scanoss-cc-Option 2: Add to PATH
+### Option 2: Add to PATH
+echo 'export PATH="/Applications/scanoss-cc.app/Contents/MacOS:$PATH"' >> ~/.zshrc source ~/.zshrc--- `163-173`: **Fix Markdown heading violations for Windows options.** Lines 163 and 168 use bold emphasis instead of proper Markdown headings, violating MD036. Apply this diff: ```diff **Installation Options:** -**Option 1: Portable Use** +### Option 1: Portable Use + - Run directly from Downloads or any location - No installation required - Ideal for USB drives or temporary usage -**Option 2: Install to Program Files (Recommended)** +### Option 2: Install to Program Files (Recommended) + ```powershell # In PowerShell (Run as Administrator) New-Item -Path "C:\Program Files\SCANOSS" -ItemType Directory -Force Move-Item -Path ".\scanoss-cc.exe" -Destination "C:\Program Files\SCANOSS\" -Force--- `314-316`: **Wrap bare URL in Markdown link format.** Line 314 contains a bare URL violating MD034. Apply this diff: ```diff **Solution:** SCANOSS Code Compare requires WebView2 runtime. It's typically pre-installed on Windows 11, but Windows 10 users may need to install it: -- Download from: https://developer.microsoft.com/en-us/microsoft-edge/webview2/ +- Download from: [Microsoft WebView2 Runtime](https://developer.microsoft.com/en-us/microsoft-edge/webview2/) - Or the app will prompt to download it automatically on first launch
🧹 Nitpick comments (4)
scripts/install.sh (1)
12-16: Unused color variable.The
BLUEvariable is defined but never used in the script.If you plan to use this color in future enhancements, consider adding a comment. Otherwise, remove it:
readonly GREEN='\033[0;32m' readonly YELLOW='\033[1;33m' -readonly BLUE='\033[0;34m' readonly NC='\033[0m'scripts/install-macos.sh (1)
94-96: Unused variable.The
dmg_urlvariable at line 95 is assigned but never used. The URL is only needed for the download function call.Remove the unused variable:
- # Download the DMG - local dmg_url=$(get_asset_url "$version" "macos") local dmg_path="$temp_dir/scanoss-cc.dmg"scripts/install-linux.sh (1)
168-198: Surface architecture handling for non-amd64 hostsRight now we always fetch the default
amd64bundle, so an ARM machine ends up with an unusable binary and a confusing “Exec format error”. Sincecommon.shalready exposesdetect_architectureanddownload_and_verify_assetaccepts anarchargument, please detect the host arch up front and either request the matching asset (once the asset naming supports it) or abort with an explicit message if we only shipamd64builds today. Even a simple guard like:- download_and_verify_asset "$version" "linux" "$zip_path" + local arch=$(detect_architecture) + if [ "$arch" != "amd64" ]; then + abort "Linux installer currently supports amd64 only. Please download the appropriate package manually." + fi + download_and_verify_asset "$version" "linux" "$zip_path" "$arch"will save users from digging through a failed install. We can relax the abort once the asset helper handles multi-arch names.
scripts/lib/common.sh (1)
5-5: Let callers decide onset -eBecause this file is sourced, the
set -ehere flips errexit for every script that imports the helpers (including ad-hoc shells that just want logging). That surprise is the root cause of the hard exits noted elsewhere. Please drop the globalset -efrom the library and enable it explicitly in the top-level installers instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/release.yml(7 hunks)INSTALLATION.md(1 hunks)INSTALL_MACOS.md(2 hunks)Makefile(3 hunks)README.md(1 hunks)backend/entities/watcher.go(0 hunks)backend/repository/result_repository_json_impl.go(1 hunks)backend/service/update_service_impl.go(1 hunks)frontend/wailsjs/go/models.ts(1 hunks)go.mod(2 hunks)main.go(4 hunks)scripts/install-linux.sh(1 hunks)scripts/install-macos.sh(1 hunks)scripts/install-windows.ps1(1 hunks)scripts/install.sh(1 hunks)scripts/lib/common.sh(1 hunks)scripts/lib/github-api.sh(1 hunks)wails.json(1 hunks)
💤 Files with no reviewable changes (1)
- backend/entities/watcher.go
🚧 Files skipped from review as they are similar to previous changes (3)
- go.mod
- .github/workflows/release.yml
- frontend/wailsjs/go/models.ts
🧰 Additional context used
🧬 Code graph analysis (8)
scripts/install.sh (2)
scripts/install-linux.sh (1)
main(214-247)scripts/install-macos.sh (1)
main(187-269)
scripts/install-macos.sh (2)
scripts/lib/common.sh (9)
command_exists(78-80)log_info(15-17)log_warn(19-21)confirm(170-191)abort(34-37)log_error(23-25)setup_temp_dir(150-157)get_latest_version(207-227)show_completion(230-245)scripts/lib/github-api.sh (2)
get_asset_url(9-56)download_and_verify_asset(147-169)
backend/repository/result_repository_json_impl.go (1)
internal/utils/file_utils.go (1)
FileReader(26-28)
scripts/lib/github-api.sh (1)
scripts/lib/common.sh (7)
log_debug(27-31)abort(34-37)command_exists(78-80)log_warn(19-21)log_info(15-17)download_file(83-114)verify_checksum(117-147)
backend/service/update_service_impl.go (4)
backend/service/update_service.go (1)
UpdateService(33-48)frontend/wailsjs/go/models.ts (1)
UpdateInfo(830-872)backend/entities/version.go (2)
UpdateInfo(32-40)AppVersion(29-29)frontend/wailsjs/runtime/runtime.js (1)
Quit(184-186)
scripts/install-linux.sh (2)
scripts/lib/common.sh (8)
log_info(15-17)log_warn(19-21)confirm(170-191)abort(34-37)command_exists(78-80)setup_temp_dir(150-157)get_latest_version(207-227)show_completion(230-245)scripts/lib/github-api.sh (1)
download_and_verify_asset(147-169)
scripts/lib/common.sh (1)
scripts/install.sh (4)
log_error(27-29)log_warn(23-25)abort(31-34)log_info(19-21)
main.go (3)
backend/repository/result_repository_json_impl.go (1)
NewResultRepositoryJsonImpl(46-60)backend/service/update_service_impl.go (1)
NewUpdateService(59-66)frontend/wailsjs/go/service/UpdateServiceImpl.js (1)
SetContext(21-23)
🪛 markdownlint-cli2 (0.18.1)
INSTALLATION.md
117-117: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
122-122: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
163-163: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
168-168: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
314-314: Bare URL used
(MD034, no-bare-urls)
🪛 Shellcheck (0.11.0)
scripts/install.sh
[warning] 15-15: BLUE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 38-38: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 145-145: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/install-macos.sh
[warning] 27-27: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 71-161: install_direct references arguments, but none are ever passed.
(SC2120)
[warning] 89-89: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 92-92: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 95-95: dmg_url appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 95-95: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 117-117: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 194-194: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/lib/github-api.sh
[warning] 154-154: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 155-155: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 161-161: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/install-linux.sh
[warning] 27-27: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 48-48: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 162-162: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 165-165: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/lib/common.sh
[warning] 41-41: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 59-59: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 151-151: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 154-154: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
⏰ 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). (4)
- GitHub Check: Build for Linux
- GitHub Check: Build for Windows
- GitHub Check: Build for MacOS
- GitHub Check: build
🔇 Additional comments (19)
wails.json (1)
3-4: Consistent project naming across configuration.The renaming from "SCANOSS Code Compare" to "scanoss-cc" aligns well with the broader project-wide naming updates and is appropriate for the Wails configuration.
Makefile (3)
2-2: Good: GOTOOLCHAIN now aligns with go.mod requirement.The GOTOOLCHAIN export set to
go1.23.0addresses the previous review concern about version mismatch. Go releases use version syntax '1.N.P', with the initial release as 1.N.0, sogo1.23.0is the correct format for Go 1.23's initial release. This ensures reproducible builds using the minimum required Go version.The added ASSETS_DIR variable is straightforward and properly referenced in the cp_assets target.
Also applies to: 7-7, 10-10
57-61: Asset copying strategy is clear and explicit.The cp_assets target now explicitly handles asset placement: all assets go into
BUILD_DIR/assetswhileappicon.pngis additionally placed atBUILD_DIRroot. This dual placement suggestsappicon.pngis needed in both locations for build/packaging purposes. The updated messaging accurately reflects this behavior.
43-43: Lint target renaming verified—no downstream impact detected.Verification confirms that the old target names (
go_lint_local,go_lint_local_fix) are not referenced in CI/CD workflows, documentation, or any other files. The renaming tolintandlint-fixis clean and safe, with no required updates elsewhere in the codebase.backend/repository/result_repository_json_impl.go (1)
46-60: LGTM! Clean simplification of repository initialization.The removal of filesystem watcher complexity in favor of explicit cache loading is a good architectural decision. The constructor now has a single responsibility: initialize the repository and perform an initial cache load. Config change handling is preserved through the listener mechanism.
backend/service/update_service_impl.go (3)
229-243: Excellent! Checksum verification properly implemented.The mandatory checksum verification with clear error messages addresses the previous security concern. The implementation correctly validates the downloaded file against the expected SHA256 before proceeding.
373-376: Good error handling for restart command.The error from
cmd.Start()is now properly checked and returned. This addresses the previous concern about failing to restart.
505-515: Excellent use of semantic versioning library.Using the
semverlibrary correctly handles version comparison with proper error checking. This resolves the previous concern about silent parsing failures.main.go (3)
78-82: Clean integration of simplified repository initialization.The removal of watcher-based initialization aligns with the architectural simplification in the repository layer.
103-117: Update service properly wired into application lifecycle.The new update service is correctly instantiated and its context is set during startup, following the established pattern used by other services.
122-133: Good use of modern Go type alias.Using
[]anyinstead of[]interface{}is the idiomatic modern Go style.INSTALL_MACOS.md (1)
5-18: Excellent Quick Install section.The automated installation script recommendation provides a streamlined user experience and aligns with the new multi-platform installer approach.
scripts/install.sh (1)
74-127: Well-structured platform detection and delegation.The universal installer cleanly detects the platform and delegates to platform-specific scripts. The error handling and user guidance for Windows (PowerShell instructions) is appropriate.
README.md (3)
27-49: Excellent Quick Install documentation.The streamlined installation instructions with platform-specific one-liners and security-conscious option provide a great user experience.
82-91: Great! Linux installation now includes executable permissions.The
chmod +xstep at line 86 addresses the previous critical issue. The instructions are now complete and will work correctly.
100-107: Clear auto-update documentation.The auto-update feature documentation helps users understand the update flow and sets proper expectations.
scripts/install-macos.sh (3)
40-68: Excellent Homebrew installation flow.The Homebrew installation path with optional Homebrew installation and proper error handling provides a smooth user experience.
71-161: Robust direct installation implementation.The DMG installation flow properly handles permissions, cleanup, symlink creation, and quarantine attribute removal. The implementation is comprehensive and user-friendly.
164-184: Clear installation menu with good UX.The interactive menu clearly presents the trade-offs between installation methods, helping users make informed decisions.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/install-macos.sh (1)
95-95: Remove unused variable.
dmg_urlis assigned but never used. Line 100 usesdownload_and_verify_assetwhich internally resolves the URL.Apply this diff:
- # Download the DMG - local dmg_url=$(get_asset_url "$version" "macos") - local dmg_path="$temp_dir/scanoss-cc.dmg" + # Download the DMG + local dmg_path="$temp_dir/scanoss-cc.dmg"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
INSTALLATION.md(1 hunks)scripts/install-macos.sh(1 hunks)scripts/install-windows.ps1(1 hunks)scripts/lib/github-api.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
scripts/lib/github-api.sh (1)
scripts/lib/common.sh (7)
log_debug(27-31)abort(34-37)command_exists(78-80)log_warn(19-21)log_info(15-17)download_file(83-114)verify_checksum(117-147)
scripts/install-macos.sh (2)
scripts/lib/common.sh (9)
command_exists(78-80)log_info(15-17)log_warn(19-21)confirm(170-191)abort(34-37)log_error(23-25)setup_temp_dir(150-157)get_latest_version(207-227)show_completion(230-245)scripts/lib/github-api.sh (2)
get_asset_url(9-56)download_and_verify_asset(163-185)
🪛 markdownlint-cli2 (0.18.1)
INSTALLATION.md
117-117: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
122-122: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Shellcheck (0.11.0)
scripts/lib/github-api.sh
[warning] 170-170: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 171-171: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/install-macos.sh
[warning] 71-161: install_direct references arguments, but none are ever passed.
(SC2120)
[warning] 89-89: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 92-92: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 95-95: dmg_url appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 95-95: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 117-117: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 194-194: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ 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
🔇 Additional comments (2)
scripts/lib/github-api.sh (1)
178-182: LGTM - Checksum fallback correctly implemented.The checksum verification correctly handles missing SHA256SUMS files by capturing the return value in a conditional guard. When
get_asset_checksumreturns non-zero, verification is skipped gracefully without aborting the installer.scripts/install-windows.ps1 (1)
327-346: LGTM - Checksum verification correctly implemented.The installer properly verifies SHA256 checksums by fetching the SHA256SUMS file and validating the downloaded asset. The implementation correctly:
- Fetches the checksum from the release
- Validates the file if a checksum is available
- Cleans up and aborts on verification failure
- Warns when checksums are unavailable
- Respects the -NoVerify flag for edge cases
This addresses the concern from past reviews and provides security parity with Linux/macOS installers.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
scripts/install.sh (1)
11-11: Remove unused color constant or add to logging output.The
BLUEconstant (line 11) is declared alongside other color constants but never used in the script. Either remove it or use it in a log function (e.g.,log_debugif debug logging is added).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/release.yml(7 hunks)CHANGELOG.md(2 hunks)scripts/install-linux.sh(1 hunks)scripts/install-macos.sh(1 hunks)scripts/install.sh(1 hunks)scripts/lib/common.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
scripts/install-linux.sh (2)
scripts/lib/common.sh (8)
log_info(15-17)log_warn(19-21)confirm(169-190)abort(34-37)command_exists(78-80)setup_temp_dir(152-156)get_latest_version(206-226)show_completion(229-244)scripts/lib/github-api.sh (1)
download_and_verify_asset(163-185)
scripts/install.sh (3)
scripts/lib/common.sh (5)
log_error(23-25)abort(34-37)log_info(15-17)log_warn(19-21)detect_os(58-75)scripts/install-macos.sh (1)
main(226-308)scripts/install-linux.sh (1)
main(215-248)
scripts/install-macos.sh (2)
scripts/lib/common.sh (9)
command_exists(78-80)log_info(15-17)log_warn(19-21)confirm(169-190)abort(34-37)log_error(23-25)setup_temp_dir(152-156)get_latest_version(206-226)show_completion(229-244)scripts/lib/github-api.sh (2)
get_asset_url(9-56)download_and_verify_asset(163-185)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
162-162: Link and image reference definitions should be needed
Unused link or image reference definition: "0.9.0"
(MD053, link-image-reference-definitions)
🪛 Shellcheck (0.11.0)
scripts/lib/common.sh
[warning] 41-41: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 59-59: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/install-linux.sh
[warning] 27-27: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 48-48: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 162-162: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 163-163: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 166-166: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/install.sh
[warning] 15-15: BLUE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 38-38: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 145-145: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/install-macos.sh
[warning] 71-200: install_direct references arguments, but none are ever passed.
(SC2120)
[warning] 100-100: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 101-101: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 104-104: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 107-107: zip_url appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 107-107: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 123-123: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 134-134: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 135-135: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 147-147: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 233-233: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ 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
🔇 Additional comments (9)
.github/workflows/release.yml (2)
343-349: SHA256 checksum generation improves release integrity.The addition of SHA256 checksum generation (lines 343-349) and inclusion in the release (line 362) is a security best practice. The implementation correctly:
- Generates checksums for all
.zipfiles- Stores them in a
SHA256SUMSfile- Includes the file in the release artifacts
This ensures users can verify download integrity. Verify that documentation instructs users on how to validate checksums.
264-267: Original review comment is incorrect and should be disregarded.The Windows build at lines 264-267 is correct and complete. The concern about missing
make cp_scriptsfor NSIS is outdated:
cp_scriptstarget no longer exists in the Makefile- NSIS is not part of the current build process — no references to makensis or .nsi files exist in any workflow
- Current Windows build is functional — it uses
make cp_assets(which exists) and builds successfully with WailsThe workflow shows the Windows build generates a standard .exe artifact that is then signed and packaged in a subsequent job. There is no NSIS installer step in the release pipeline.
Likely an incorrect or invalid review comment.
scripts/install.sh (1)
37-54: LGTM! OS detection logic is sound.The
detect_osfunction correctly identifies macOS, Linux, and Windows (including Git Bash variants). The error handling for unsupported OS is appropriate.scripts/install-macos.sh (1)
155-200: LGTM! macOS-specific installation logic is comprehensive.The direct installation flow handles:
- Permission checks and sudo elevation
- DMG mounting and unmounting with proper error handling
- Quarantine attribute removal (important for macOS Gatekeeper)
- Symlink creation with fallback approaches
- Post-install verification and optional app launch
This is well-structured and user-friendly.
scripts/install-linux.sh (2)
47-99: LGTM! Distro detection and dependency installation are well-implemented.The script correctly:
- Detects multiple Linux distributions via
/etc/os-releasewith fallbacks- Uses appropriate package managers per distro (apt, dnf, pacman, zypper)
- Includes graceful degradation with warnings if packages aren't available
- Allows users to skip dependency installation with confirmation
This approach ensures compatibility across major Linux distributions.
102-130: LGTM! Desktop entry creation follows Linux standards.The
.desktopfile is properly formatted with appropriate categories, keywords, and exec path. The script updates the desktop database whenupdate-desktop-databaseis available, enabling GUI launchers to recognize the application.scripts/lib/common.sh (3)
1-50: LGTM! Library foundation is solid and well-structured.The common library provides essential utilities for cross-platform installation:
- Comprehensive logging with color support
- Robust OS and architecture detection
- Retry logic in downloads with progress feedback
- Checksum verification with case-insensitive comparison
- Temp directory management with cleanup
- User interaction helpers
Function exports (lines 247-252) ensure these utilities are available to sourced scripts. This is a good foundation for the installer ecosystem.
83-114: LGTM! Download function includes appropriate resilience patterns.The
download_file()function includes:
- Retry logic with configurable attempts (default 3)
- Support for both curl and wget
- Progress bar feedback
- 2-second delays between retries
This pattern improves reliability over unreliable networks and is appropriate for distribution downloads.
117-147: LGTM! Checksum verification is security-conscious.The
verify_checksum()function:
- Supports both
shasumandsha256sumtools- Performs case-insensitive comparison (lines 135-136)
- Provides clear error messages on mismatch
- Gracefully skips verification if no checksum tool is available
This is appropriate for verifying release integrity.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
scripts/install-windows.ps1 (4)
94-111: Consider renaming function to match its behavior.The function name
Get-FileWithProgresssuggests progress indication, but the implementation usesWebClient.DownloadFile()synchronously without displaying progress (and$ProgressPreferenceis set toSilentlyContinueglobally). Consider renaming toGet-FileorDownload-Filefor clarity, or implement actual progress display usingInvoke-WebRequestwith progress callbacks.
177-178: Regex pattern for checksum matching could be more strict.The pattern
'^\s*([a-fA-F0-9]+)\s+.*' + [regex]::Escape($AssetName)is permissive with.*between the hash and filename. Standard SHA256SUMS format is more strict. Consider:- $pattern = '^\s*([a-fA-F0-9]+)\s+.*' + [regex]::Escape($AssetName) + $pattern = '^\s*([a-fA-F0-9]{64})\s+\*?' + [regex]::Escape($AssetName) + '$'This ensures exactly 64 hex characters (SHA256), optionally matches the binary mode indicator (
*), and anchors to the end of the line.
145-145: Asset name matching could be more specific.Using
-like "*$assetPattern*"may match unintended assets if the release contains files likescanoss-cc-win.zip.sigortest-scanoss-cc-win.zip. Consider exact matching or more specific patterns:- $asset = $release.assets | Where-Object { $_.name -like "*$assetPattern*" } | Select-Object -First 1 + $asset = $release.assets | Where-Object { $_.name -eq $assetPattern } | Select-Object -First 1
368-372: Consider adding guidance about restarting terminals for PATH changes.While the script updates
$env:Pathfor the current session (line 259), the Machine-level PATH change won't affect other open terminals until they're restarted. Consider adding a note in the completion message (lines 386-402) to inform users they may need to restart their terminal or PowerShell session for the PATH to take effect system-wide.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/install-windows.ps1(1 hunks)
⏰ 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
🔇 Additional comments (3)
scripts/install-windows.ps1 (3)
328-347: Checksum verification properly implemented!The previous review concern about missing checksum verification has been fully addressed. The implementation correctly:
- Fetches the expected checksum from the SHA256SUMS release asset
- Verifies the downloaded ZIP before extraction
- Aborts installation and cleans up on checksum mismatch
- Only skips verification when explicitly requested via
-NoVerify- Provides appropriate warnings when checksums are unavailable
216-220: Interactive prompt may block in non-interactive scenarios.The
Read-Hostprompt will block or fail if the script runs in a non-interactive context (scheduled tasks, remote sessions, automation). Consider adding a parameter like-AutoInstallWebView2to handle non-interactive scenarios gracefully, or detect interactivity and default to a safe behavior.Example enhancement:
[CmdletBinding()] param( # ... existing parameters ... [switch]$AutoInstallWebView2 ) # In Install-WebView2: if ($AutoInstallWebView2) { $response = 'Y' } else { $response = Read-Host "Download and install WebView2 Runtime? (Y/n)" }
355-359: Verify the ZIP archive structure is well-defined.The script recursively searches for the executable and takes the first match. If the ZIP contains multiple
.exefiles or has an unexpected structure, this could select the wrong binary. Ensure your release process produces a consistent ZIP structure with a singlescanoss-cc.exefile.Optionally, add validation to ensure exactly one executable is found:
$exeFiles = @(Get-ChildItem -Path $tempDir -Filter $Script:ExeName -Recurse) if ($exeFiles.Count -eq 0) { throw "Executable not found in archive: $Script:ExeName" } elseif ($exeFiles.Count -gt 1) { throw "Multiple executables found in archive. Expected exactly one." } $exePath = $exeFiles[0]
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/install-windows.ps1 (1)
250-251: PATH duplication check may produce false positives.The substring match
$currentPath -like "*$Path*"could incorrectly match if$Pathis a substring of another PATH entry (e.g., if installing toC:\Program Files\SCANOSSand PATH containsC:\Program Files\SCANOSS-OLD). This would skip the PATH update when it should actually be performed.Apply the fix suggested in the previous review:
- if ($currentPath -like "*$Path*") { + $pathEntries = $currentPath -split ';' | ForEach-Object { $_.TrimEnd('\') } + $targetPath = $Path.TrimEnd('\') + if ($pathEntries -contains $targetPath) { Write-Info "Already in PATH" return }
🧹 Nitpick comments (2)
scripts/install-windows.ps1 (2)
94-111: Consider renaming function to match its behavior.The function is named
Get-FileWithProgressbut does not display download progress. TheSystem.Net.WebClient.DownloadFilemethod used here is synchronous and doesn't provide progress updates. Consider renaming toGet-Fileor implementing actual progress tracking.If progress display is desired, you could use the
DownloadFileAsyncmethod with progress events, or useInvoke-WebRequestwith a custom progress handler. However, given the script sets$ProgressPreference = "SilentlyContinue"globally, the current implementation may be intentional.
78-111: Consider adding timeout protection for network operations.Network calls (GitHub API, file downloads) lack explicit timeout values. If the remote server is slow or unresponsive, the installer could hang indefinitely. While not critical for an interactive script (users can manually cancel), adding timeouts improves resilience.
For
System.Net.WebClientoperations, you can set a timeout:$webClient = New-Object System.Net.WebClient $webClient.Timeout = 300000 # 5 minutes in milliseconds $webClient.DownloadFile($Url, $OutputPath)For PowerShell 5.1's
Invoke-RestMethod/Invoke-WebRequest, timeouts require setting$PSDefaultParameterValuesor wrapping calls withStart-JobandWait-Job -Timeout.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/install-windows.ps1(1 hunks)
⏰ 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). (2)
- GitHub Check: unit_tests
- GitHub Check: integration-tests
🔇 Additional comments (1)
scripts/install-windows.ps1 (1)
328-347: Checksum verification has been properly implemented.The previous review concern about missing checksum verification has been addressed. The implementation correctly:
- Fetches the SHA256SUMS file for the release
- Validates the downloaded ZIP against the expected checksum
- Removes the file and throws an error if verification fails
- Warns but proceeds if no checksum is available
- Can be skipped explicitly with the
-NoVerifyflagThis maintains security parity with the Linux and macOS installers.
| function Test-WebView2 { | ||
| $webView2Key = "HKLM:\SOFTWARE\WOW6432Node\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}" | ||
|
|
||
| if (Test-Path $webView2Key) { | ||
| $version = (Get-ItemProperty -Path $webView2Key -Name "pv" -ErrorAction SilentlyContinue).pv | ||
| if ($version) { | ||
| Write-Info "WebView2 is installed (version: $version)" | ||
| return $true | ||
| } | ||
| } | ||
|
|
||
| return $false | ||
| } |
There was a problem hiding this comment.
WebView2 detection may miss some installation locations.
The registry check only examines WOW6432Node (32-bit applications on 64-bit Windows). WebView2 Runtime can be installed in multiple locations:
- Native 64-bit:
HKLM:\SOFTWARE\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5} - Per-user:
HKCU:\SOFTWARE\Microsoft\EdgeUpdate\Clients\{...}
This could result in false negatives, triggering an unnecessary reinstallation prompt.
Consider checking multiple registry locations:
function Test-WebView2 {
- $webView2Key = "HKLM:\SOFTWARE\WOW6432Node\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}"
-
- if (Test-Path $webView2Key) {
- $version = (Get-ItemProperty -Path $webView2Key -Name "pv" -ErrorAction SilentlyContinue).pv
- if ($version) {
- Write-Info "WebView2 is installed (version: $version)"
- return $true
+ $webView2Paths = @(
+ "HKLM:\SOFTWARE\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}",
+ "HKLM:\SOFTWARE\WOW6432Node\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}",
+ "HKCU:\SOFTWARE\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}"
+ )
+
+ foreach ($path in $webView2Paths) {
+ if (Test-Path $path) {
+ $version = (Get-ItemProperty -Path $path -Name "pv" -ErrorAction SilentlyContinue).pv
+ if ($version) {
+ Write-Info "WebView2 is installed (version: $version)"
+ return $true
+ }
}
}
return $false
}🤖 Prompt for AI Agents
In scripts/install-windows.ps1 around lines 196-208, the Test-WebView2 function
only checks the WOW6432Node registry path and can miss 64-bit and per-user
installs; update the function to check multiple known locations
(HKLM:\SOFTWARE\WOW6432Node\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5},
HKLM:\SOFTWARE\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5},
and
HKCU:\SOFTWARE\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5});
for each path attempt to read the "pv" property with Get-ItemProperty
-ErrorAction SilentlyContinue and return true as soon as a non-empty version is
found, otherwise return false. Ensure error-suppression remains and log the
detected path/version when found.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
matiasdaloia
force-pushed
the
chore/mdaloia/SP-3605-Update-scanoss.cc-Installation-documentation
branch
from
8613d97 to
9166d4a
Compare
* [SP-3447] fix: remove test and tests from default skipped dirs * [SP-3447] chore: update changelog [SP-3605] feat: enhanced installation scripts for all platforms, add update notification component [SP-3605] chore: update scanoss.json [SP-3605] feat: add gh release checksum verification, add semver validation [SP-3605] fix: windows installer config [SP-3605] fix: release workflow [SP-3605] fix: release workflow [SP-3605] fix: release workflow [SP-3605] fix: windows release workflow [SP-3605] fix: simplify windows distribution [SP-3605] feat: standardize binary names and add installation scripts [SP-3605] fix: update installation scripts [SP-3605] fix: unit tests [SP-3605] fix: macos install permission errors [SP-3605] chore: update changelog [SP-3605] fix: windows install script [SP-3605] fix: windows install script [SP-3605] fix: windows install script fix: remove all Unicode characters from PowerShell script for compatibility [SP-3605] chore: update changelog
matiasdaloia
force-pushed
the
chore/mdaloia/SP-3605-Update-scanoss.cc-Installation-documentation
branch
from
9166d4a to
186db0f
Compare
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
matiasdaloia
deleted the
chore/mdaloia/SP-3605-Update-scanoss.cc-Installation-documentation
branch
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
INSTALLATION.md (1)
117-126: Convert option labels to semantic Markdown headings.Lines 117 and 122 still use bold emphasis instead of proper Markdown headings, violating MD036. These should be converted to
###subheadings for proper document structure and accessibility.Apply this diff:
**Setting Up CLI Access:** After installing via DMG, you need to manually set up CLI access. Choose one of these methods: -**Option 1: Symlink (Recommended)** +### Option 1: Symlink (Recommended) + ```bash sudo ln -s "/Applications/scanoss-cc.app/Contents/MacOS/scanoss-cc" /usr/local/bin/scanoss-cc-Option 2: Add to PATH
+### Option 2: Add to PATH
+echo 'export PATH="/Applications/scanoss-cc.app/Contents/MacOS:$PATH"' >> ~/.zshrc source ~/.zshrc</blockquote></details> <details> <summary>scripts/install-windows.ps1 (2)</summary><blockquote> `195-207`: **Expand WebView2 detection beyond WOW6432Node.** We only probe the 32‑bit HKLM\WOW6432Node hive. Native 64‑bit installs live under `HKLM:\SOFTWARE\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}`, and per-user installs under `HKCU`. Ignoring those yields false negatives and needlessly forces users through the runtime installer. The earlier review feedback already called this out—please enumerate all three standard keys and exit as soon as a version is found. ([jdocs.mdbgo.io](https://jdocs.mdbgo.io/PowerShell/Microsoft.PowerShell.Utility/Invoke-RestMethod/?utm_source=openai)) ```diff function Test-WebView2 { - $webView2Key = "HKLM:\SOFTWARE\WOW6432Node\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}" - - if (Test-Path $webView2Key) { - $version = (Get-ItemProperty -Path $webView2Key -Name "pv" -ErrorAction SilentlyContinue).pv - if ($version) { - Write-Info "WebView2 is installed (version: $version)" - return $true - } - } - - return $false + $webView2Paths = @( + "HKLM:\SOFTWARE\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}", + "HKLM:\SOFTWARE\WOW6432Node\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}", + "HKCU:\SOFTWARE\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}" + ) + + foreach ($path in $webView2Paths) { + if (Test-Path $path) { + $version = (Get-ItemProperty -Path $path -Name "pv" -ErrorAction SilentlyContinue).pv + if ($version) { + Write-Info "WebView2 is installed (version: $version)" + return $true + } + } + } + + return $false }
248-262: Fix PATH duplication detection.
$currentPath -like "*$Path*"treats substring matches as duplicates (e.g."C:\Program Files\SCANOSS-OLD"blocks adding"C:\Program Files\SCANOSS"). That leaves binaries unreachable and regresses the installer. Split the PATH, normalise separators, and perform an exact, case-insensitive comparison per the prior review guidance. (techtarget.com)- if ($currentPath -like "*$Path*") { - Write-Info "Already in PATH" - return - } - - $newPath = "$currentPath;$Path" + $pathEntries = $currentPath -split ';' | ForEach-Object { $_.TrimEnd('\') } + $targetPath = $Path.TrimEnd('\') + if ($pathEntries -contains $targetPath) { + Write-Info "Already in PATH" + return + } + + $newPath = "$currentPath;$targetPath" @@ - $env:Path = "$env:Path;$Path" + if (-not (($env:Path -split ';') -contains $targetPath)) { + $env:Path = "$env:Path;$targetPath" + }backend/service/update_service_impl.go (1)
363-366: Restore failure must be reported.If replacing the executable fails, we attempt to undo it but ignore the restore error, potentially leaving users with neither old nor new binary—the same regression highlighted in the earlier review. Capture and surface the restore failure so operators aren’t stranded. (techtarget.com)
if err := os.Rename(newBinaryPath, currentExe); err != nil { // Restore backup on failure - os.Rename(backupPath, currentExe) - return fmt.Errorf("failed to replace executable: %w", err) + if restoreErr := os.Rename(backupPath, currentExe); restoreErr != nil { + return fmt.Errorf("failed to replace executable: %w (restore also failed: %v)", err, restoreErr) + } + return fmt.Errorf("failed to replace executable: %w", err) }
🧹 Nitpick comments (3)
scripts/install.sh (2)
15-15: Remove unused BLUE color variable.The
BLUEvariable is defined but never referenced in the script. Consider removing it to eliminate dead code.Apply this diff:
readonly RED='\033[0;31m' readonly GREEN='\033[0;32m' readonly YELLOW='\033[1;33m' -readonly BLUE='\033[0;34m' readonly NC='\033[0m'
37-54: Separate variable declaration and assignment to preserve error handling.Line 38 combines
localdeclaration with command substitution, which masks the command's exit status. Withset -e, ifunamefails, the error won't propagate correctly. The same pattern occurs on line 145.Apply this diff:
detect_os() { - local os="$(uname -s)" + local os + os="$(uname -s)" case "$os" inAnd for line 145:
# Detect platform - local platform=$(detect_os) + local platform + platform=$(detect_os)scripts/install-linux.sh (1)
48-48: Consider separating variable declarations for better error visibility.Lines 48, 162, and 166 combine local declarations with command substitution, which can mask command failures. While the called functions have their own error handling, separating declaration and assignment is a best practice in strict error-handling scripts.
For example, on line 48:
- local distro=$(detect_distro) + local distro + distro=$(detect_distro)Apply similar changes to lines 162 and 166 for consistency.
Also applies to: 162-162, 166-166
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
.gitattributes(1 hunks).github/workflows/release.yml(7 hunks).pre-commit-config.yaml(1 hunks)CHANGELOG.md(2 hunks)INSTALLATION.md(1 hunks)INSTALL_MACOS.md(2 hunks)Makefile(3 hunks)README.md(1 hunks)backend/entities/file.go(0 hunks)backend/entities/version.go(1 hunks)backend/entities/watcher.go(0 hunks)backend/repository/result_repository_json_impl.go(1 hunks)backend/repository/result_repository_json_impl_test.go(5 hunks)backend/service/mocks/mock_ComponentService.go(1 hunks)backend/service/mocks/mock_HTTPClient.go(1 hunks)backend/service/mocks/mock_LicenseService.go(1 hunks)backend/service/mocks/mock_ScanossApiService.go(1 hunks)backend/service/mocks/mock_UpdateService.go(1 hunks)backend/service/update_service.go(1 hunks)backend/service/update_service_impl.go(1 hunks)frontend/src/components/SettingsDialog.tsx(1 hunks)frontend/src/components/StatusBar.tsx(2 hunks)frontend/src/components/UpdateNotification.tsx(1 hunks)frontend/wailsjs/go/models.ts(1 hunks)frontend/wailsjs/go/service/UpdateServiceImpl.d.ts(1 hunks)frontend/wailsjs/go/service/UpdateServiceImpl.js(1 hunks)go.mod(2 hunks)main.go(4 hunks)scanoss.json(2 hunks)scripts/install-linux.sh(1 hunks)scripts/install-macos.sh(1 hunks)scripts/install-windows.ps1(1 hunks)scripts/install.sh(1 hunks)scripts/lib/common.sh(1 hunks)scripts/lib/github-api.sh(1 hunks)wails.json(1 hunks)
💤 Files with no reviewable changes (2)
- backend/entities/file.go
- backend/entities/watcher.go
🚧 Files skipped from review as they are similar to previous changes (13)
- .gitattributes
- go.mod
- scanoss.json
- backend/entities/version.go
- frontend/src/components/SettingsDialog.tsx
- INSTALL_MACOS.md
- backend/service/mocks/mock_ComponentService.go
- frontend/wailsjs/go/service/UpdateServiceImpl.d.ts
- main.go
- backend/service/update_service.go
- wails.json
- frontend/src/components/StatusBar.tsx
- frontend/src/components/UpdateNotification.tsx
🧰 Additional context used
🧬 Code graph analysis (11)
scripts/install.sh (3)
scripts/lib/common.sh (5)
log_error(23-25)abort(34-37)log_info(15-17)log_warn(19-21)detect_os(58-75)scripts/install-linux.sh (1)
main(215-248)scripts/install-macos.sh (1)
main(224-306)
scripts/install-macos.sh (2)
scripts/lib/common.sh (9)
command_exists(78-80)log_info(15-17)log_warn(19-21)confirm(169-190)abort(34-37)log_error(23-25)setup_temp_dir(152-156)get_latest_version(206-226)show_completion(229-244)scripts/lib/github-api.sh (1)
download_and_verify_asset(163-185)
backend/service/mocks/mock_UpdateService.go (3)
frontend/wailsjs/go/service/UpdateServiceImpl.js (5)
ApplyUpdate(5-7)CheckForUpdate(9-11)DownloadUpdate(13-15)GetCurrentVersion(17-19)SetContext(21-23)backend/entities/version.go (1)
UpdateInfo(32-40)frontend/wailsjs/go/models.ts (1)
UpdateInfo(830-872)
backend/repository/result_repository_json_impl_test.go (1)
backend/repository/result_repository_json_impl.go (1)
NewResultRepositoryJsonImpl(46-60)
backend/service/mocks/mock_ScanossApiService.go (4)
frontend/wailsjs/go/service/LicenseServiceImpl.js (1)
GetLicensesByPurl(9-11)frontend/wailsjs/go/models.ts (4)
ComponentRequest(234-247)GetLicensesByPurlResponse(378-409)ComponentSearchRequest(248-269)ComponentSearchResponse(300-331)frontend/wailsjs/go/service/ComponentServiceImpl.js (1)
SearchComponents(37-39)frontend/wailsjs/go/service/UpdateServiceImpl.js (1)
SetContext(21-23)
backend/service/update_service_impl.go (5)
backend/service/update_service.go (1)
UpdateService(33-48)frontend/wailsjs/go/service/UpdateServiceImpl.js (5)
SetContext(21-23)CheckForUpdate(9-11)GetCurrentVersion(17-19)DownloadUpdate(13-15)ApplyUpdate(5-7)backend/entities/version.go (2)
UpdateInfo(32-40)AppVersion(29-29)frontend/wailsjs/go/models.ts (1)
UpdateInfo(830-872)frontend/wailsjs/runtime/runtime.js (1)
Quit(184-186)
frontend/wailsjs/go/models.ts (1)
backend/entities/version.go (1)
UpdateInfo(32-40)
backend/service/mocks/mock_LicenseService.go (2)
frontend/wailsjs/go/service/LicenseServiceImpl.js (1)
GetLicensesByPurl(9-11)frontend/wailsjs/go/models.ts (2)
ComponentRequest(234-247)GetLicensesByPurlResponse(378-409)
backend/repository/result_repository_json_impl.go (1)
internal/utils/file_utils.go (1)
FileReader(26-28)
scripts/install-linux.sh (2)
scripts/lib/common.sh (8)
log_info(15-17)log_warn(19-21)confirm(169-190)abort(34-37)command_exists(78-80)setup_temp_dir(152-156)get_latest_version(206-226)show_completion(229-244)scripts/lib/github-api.sh (1)
download_and_verify_asset(163-185)
scripts/lib/github-api.sh (1)
scripts/lib/common.sh (7)
log_debug(27-31)abort(34-37)command_exists(78-80)log_warn(19-21)log_info(15-17)download_file(83-114)verify_checksum(117-147)
🪛 markdownlint-cli2 (0.18.1)
INSTALLATION.md
117-117: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
122-122: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Shellcheck (0.11.0)
scripts/install.sh
[warning] 15-15: BLUE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 38-38: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 145-145: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/install-macos.sh
[warning] 71-198: install_direct references arguments, but none are ever passed.
(SC2120)
[warning] 100-100: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 101-101: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 104-104: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 121-121: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 132-132: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 133-133: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 145-145: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 231-231: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/install-linux.sh
[warning] 48-48: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 162-162: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 166-166: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/lib/common.sh
[warning] 41-41: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 59-59: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/lib/github-api.sh
[warning] 170-170: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 171-171: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ 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: integration-tests
🔇 Additional comments (31)
Makefile (4)
2-2: Makefile structure and variable definitions look correct.APP_NAME, ASSETS_DIR, and GOTOOLCHAIN (go1.23.0) are properly defined and aligned with the go.mod requirement. The GOTOOLCHAIN version matches the prior review fix (commit 9166d4a).
Also applies to: 7-7, 10-10
43-43: Lint target renaming is clear; note this is a breaking change.The rename from
go_lint_local→lintandgo_lint_local_fix→lint-fixfollows more standard Makefile conventions and improves clarity. Both targets correctly invokegolangci-lintwith and without--fix. Be aware this breaks any existing scripts or CI jobs referencing the old target names.Also applies to: 46-46
57-61: cp_assets target correctly implements BUILD_DIR–based asset structure.The refactored logic creates BUILD_DIR/assets, copies all assets there, then copies appicon.png to the BUILD_DIR root. This aligns with the unified asset handling described in the PR and supports the new installer scripts. The approach is sound: if assets are missing, the build fails immediately (desired fail-fast behavior).
63-63: Target dependencies are correct.The
build,binary, andbuild_macostargets correctly depend oncp_assetsto ensure assets are staged before building, maintaining the expected build order.Also applies to: 67-67, 71-71
.pre-commit-config.yaml (1)
3-3: LGTM!The pre-commit hook version update is appropriate and straightforward.
CHANGELOG.md (1)
10-36: LGTM!The changelog entry is comprehensive and well-structured, following the Keep a Changelog format. The version link at line 163 properly references this new release, resolving the previously noted orphaned link issue.
backend/service/mocks/mock_LicenseService.go (1)
80-135: LGTM!The auto-generated mock implementation for
GetLicensesByPurlfollows standard testify/mock patterns and is consistent with other mocks in the repository.scripts/lib/common.sh (4)
5-5: LGTM!Using
set -efor strict error handling is appropriate for installation scripts, ensuring failures are caught early.
39-80: LGTM!The OS and architecture detection logic is comprehensive and handles the main platforms appropriately. The ShellCheck SC2155 warnings about combined declaration and assignment are false positives here—
unameis a core POSIX utility that won't fail in this context, and the pattern improves readability.
82-114: LGTM!The download function with retry logic (3 attempts, 2-second delays) and fallback between curl/wget is robust and handles transient network failures appropriately.
116-147: LGTM!The checksum verification is well-implemented with:
- Support for both
shasumandsha256sum- Case-insensitive comparison (lines 135-136)
- Graceful degradation when no checksum tool is available
scripts/lib/github-api.sh (4)
9-56: LGTM!The asset URL fetching logic is robust, with appropriate platform detection and error handling via
abortfor missing assets.
58-89: LGTM!The checksum fetching correctly uses
set +e/set -eto handle missingSHA256SUMSfiles gracefully, returning 1 to allow callers to skip verification without aborting the installer.
91-117: LGTM!The
version_existsfunction correctly disables errexit and checks for HTTP 200, preventing the installer from aborting on 404 responses. This addresses the concern from the previous review.
162-185: LGTM!The download and verification flow is correct:
- Line 178 properly uses conditional assignment to handle
get_asset_checksumreturning 1- This allows verification to be skipped gracefully when SHA256SUMS is absent
- The ShellCheck SC2155 warnings at lines 170-171 are acceptable here since
get_asset_urlcallsaborton failure (won't return) andbasenamewon't fail on a valid URL stringscripts/install-macos.sh (6)
14-28: LGTM!The library sourcing logic elegantly handles both local and remote (curl-based) execution, with proper trap cleanup using single quotes for delayed variable expansion.
39-68: LGTM!The Homebrew installation path is well-structured with user confirmation before installing Homebrew itself, and appropriate error handling throughout.
112-164: LGTM!The DMG extraction and mounting logic is robust, with proper mount point detection and cleanup via
hdiutil detach.
165-198: LGTM!The CLI symlink creation gracefully handles permissions with sudo fallback and provides helpful warnings rather than aborting if symlink creation fails. The quarantine clearing at line 193 is essential for macOS security.
224-306: LGTM!The main function provides a well-structured user experience with:
- Platform validation
- Non-interactive flag support (
--homebrew,--direct)- Interactive menu with fallback logic
- Post-installation verification
The ShellCheck warnings (SC2120, SC2155) are acceptable in this context.
100-101: Code is already correct; review comment is based on a misunderstanding of trap semantics.The current implementation at line 101 correctly uses double quotes:
trap "cleanup_temp_dir '$temp_dir'" EXIT INT TERM. This captures the$temp_dirvalue when the trap is registered, which is the correct behavior for a cleanup handler—ensuring the exact directory created is passed to the cleanup function. Using single quotes (as suggested in the review) would defer evaluation until trap execution, potentially causing issues if the variable is unset by then. The single quotes inside the double quotes are literal characters and do not affect expansion.Likely an incorrect or invalid review comment.
.github/workflows/release.yml (5)
13-13: LGTM!Introducing
ARTIFACT_NAME_PREFIXunifies artifact naming across platforms, simplifying release packaging and eliminating platform-specific naming inconsistencies.
49-58: LGTM!The Mac build correctly calls
make cp_assetsbefore the Wails build, ensuring frontend assets are present. The unified artifact naming viaARTIFACT_NAME_PREFIXis consistently applied.
210-230: LGTM!The Linux build follows the same pattern as macOS, with
make cp_assetsbefore building and consistent artifact naming.
263-322: LGTM!The Windows build workflow is well-structured:
make cp_assetsensures frontend assets are present- Unsigned artifact is created first
- Separate signing job with CodeSignTool
- Final signed artifact packaged as ZIP
The unified naming is consistently applied throughout.
343-362: LGTM!The SHA256 checksum generation and inclusion in the release is a significant security improvement. The array-based approach for collecting release files is more maintainable than per-file lists.
backend/repository/result_repository_json_impl_test.go (1)
48-48: LGTM!All test cases have been correctly updated to use the new single-argument constructor
NewResultRepositoryJsonImpl(mu), aligning with the removal of the watcher parameter from the implementation.Also applies to: 65-65, 79-79, 90-90, 103-103
frontend/wailsjs/go/service/UpdateServiceImpl.js (1)
1-23: Auto-generated Wails bindings look correct.This file is auto-generated by Wails and correctly exposes the UpdateService methods to the frontend. No manual changes needed.
frontend/wailsjs/go/models.ts (1)
830-872: Auto-generated UpdateInfo model is correct.The UpdateInfo class is correctly generated from the Go struct and follows the established pattern used by other entities in this file. The field mappings and conversion logic are appropriate.
backend/repository/result_repository_json_impl.go (1)
46-60: Watcher removal is clean and well-structured.The removal of the file watcher dependency simplifies the repository's initialization and lifecycle management. The initial cache load on line 52 and the config change listener on line 57 ensure the cache remains synchronized without the watcher. This is a cleaner approach that reduces complexity.
backend/service/mocks/mock_UpdateService.go (1)
1-274: Auto-generated mock follows correct patterns.This mock is correctly generated by mockery and provides proper type-safe expectations for all UpdateService methods. The expecter pattern and cleanup registration are implemented correctly.
| digest := *asset.Digest | ||
| expectedSHA256, _ = strings.CutPrefix(digest, "sha256:") | ||
| log.Info().Msgf("Found SHA256 digest for asset: %s", expectedSHA256) | ||
| } else { | ||
| log.Warn().Msg("No digest found for release asset - update download will fail verification") | ||
| } | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if downloadURL == "" { | ||
| return nil, fmt.Errorf("no suitable download found for this platform") | ||
| } | ||
|
|
||
| log.Info().Msgf("Update available: %s", latestVersion) | ||
|
|
||
| return &entities.UpdateInfo{ | ||
| Version: latestVersion, | ||
| DownloadURL: downloadURL, | ||
| ReleaseNotes: release.Body, | ||
| PublishedAt: release.PublishedAt, | ||
| Size: size, | ||
| Available: true, | ||
| ExpectedSHA256: expectedSHA256, | ||
| }, nil | ||
| } | ||
|
|
||
| // DownloadUpdate downloads the new version to a temporary location | ||
| func (u *UpdateServiceImpl) DownloadUpdate(updateInfo *entities.UpdateInfo) (string, error) { | ||
| log.Info().Msgf("Downloading update from: %s", updateInfo.DownloadURL) | ||
|
|
||
| // Create a temporary file | ||
| tmpFile, err := os.CreateTemp(u.downloadDir, "scanoss-update-*") | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to create temp file: %w", err) | ||
| } | ||
| defer tmpFile.Close() | ||
|
|
||
| tmpPath := tmpFile.Name() | ||
|
|
||
| // Create request with timeout | ||
| ctx, cancel := context.WithTimeout(u.ctx, downloadTimeout) | ||
| defer cancel() | ||
|
|
||
| req, err := http.NewRequestWithContext(ctx, "GET", updateInfo.DownloadURL, nil) | ||
| if err != nil { | ||
| os.Remove(tmpPath) | ||
| return "", fmt.Errorf("failed to create request: %w", err) | ||
| } | ||
|
|
||
| // Download the file | ||
| resp, err := u.httpClient.Do(req) | ||
| if err != nil { | ||
| os.Remove(tmpPath) | ||
| return "", fmt.Errorf("failed to download update: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| os.Remove(tmpPath) | ||
| return "", fmt.Errorf("download failed with status %d", resp.StatusCode) | ||
| } | ||
|
|
||
| // Copy with progress tracking | ||
| hash := sha256.New() | ||
| multiWriter := io.MultiWriter(tmpFile, hash) | ||
|
|
||
| written, err := io.Copy(multiWriter, resp.Body) | ||
| if err != nil { | ||
| os.Remove(tmpPath) | ||
| return "", fmt.Errorf("failed to save update: %w", err) | ||
| } | ||
|
|
||
| // Compute actual checksum as hex string | ||
| actualChecksum := hex.EncodeToString(hash.Sum(nil)) | ||
| log.Info().Msgf("Downloaded %d bytes (SHA256: %s)", written, actualChecksum) | ||
|
|
||
| // Verify checksum - this is mandatory for security | ||
| if updateInfo.ExpectedSHA256 == "" { | ||
| os.Remove(tmpPath) | ||
| return "", fmt.Errorf("no expected checksum available for verification - cannot safely install update") | ||
| } | ||
|
|
||
| expectedChecksum := strings.ToLower(strings.TrimSpace(updateInfo.ExpectedSHA256)) | ||
| actualChecksumLower := strings.ToLower(actualChecksum) | ||
|
|
||
| if expectedChecksum != actualChecksumLower { | ||
| os.Remove(tmpPath) | ||
| return "", fmt.Errorf("checksum verification failed: expected %s, got %s", expectedChecksum, actualChecksumLower) | ||
| } |
There was a problem hiding this comment.
Allow secure updates when GitHub omits asset.Digest.
ExpectedSHA256 is required in DownloadUpdate, yet CheckForUpdate leaves it empty whenever the REST API doesn’t surface asset.Digest. GitHub only added digests mid‑2025; older releases—and any third‑party mirrors—still lack that field. In those cases we now guarantee update failure ("no expected checksum available for verification" -> hard error). Please fall back to existing checksum assets (e.g. SHA256SUMS) or gracefully skip verification instead of blocking the update altogether. (github.blog)
- if updateInfo.ExpectedSHA256 == "" {
- os.Remove(tmpPath)
- return "", fmt.Errorf("no expected checksum available for verification - cannot safely install update")
- }
+ if updateInfo.ExpectedSHA256 == "" {
+ // Try the classic SHA256SUMS asset before giving up
+ checksumURL := strings.TrimSuffix(updateInfo.DownloadURL, filepath.Base(updateInfo.DownloadURL)) + "SHA256SUMS"
+ expected, err := fetchChecksum(ctx, u.httpClient, checksumURL, filepath.Base(updateInfo.DownloadURL))
+ if err != nil {
+ os.Remove(tmpPath)
+ return "", fmt.Errorf("unable to obtain checksum for %s: %w", updateInfo.DownloadURL, err)
+ }
+ updateInfo.ExpectedSHA256 = expected
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/service/update_service_impl.go around lines 152-241, CheckForUpdate
currently leaves ExpectedSHA256 empty when asset.Digest is missing which causes
DownloadUpdate to hard-fail; fix by (1) in CheckForUpdate add a fallback that
searches release assets for checksum files (names like "SHA256SUMS", "*.sha256",
"*.sha256sum", "<asset>.sha256") and if found download and parse the checksum
file to extract the SHA256 for the chosen asset and populate ExpectedSHA256; (2)
if no checksum asset is present, change behavior to not block updates: in
DownloadUpdate remove the hard error when updateInfo.ExpectedSHA256 == "" and
instead log a warning that verification is skipped and proceed to return the
downloaded file (or optionally mark the UpdateInfo as Unverified), preserving
existing verification logic when a checksum is present; ensure all network/file
errors are handled and logged and that tests are updated to cover both fallback
checksum parsing and the verification-skip path.
Summary by CodeRabbit
New Features
Documentation
Chores / Build
Removed