Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThis PR introduces a comprehensive self-upgrade system for the dagu CLI, including a new Changes
Sequence DiagramssequenceDiagram
actor User
participant CLI as CLI Command
participant GitHub as GitHub API
participant Download as Download Module
participant Install as Install Module
participant Cache as Cache
participant Result as Result Formatter
User->>CLI: dagu upgrade [flags]
CLI->>CLI: Validate self-upgrade capability
CLI->>GitHub: Fetch latest/target release
GitHub-->>CLI: Release metadata + assets
CLI->>GitHub: Fetch checksums
GitHub-->>CLI: Checksum map
CLI->>Download: Download binary archive
Download->>Download: Verify checksum
Download-->>CLI: Downloaded file
CLI->>Install: Extract & install binary
Install->>Install: Atomic replacement with backup
Install-->>CLI: Install result
CLI->>Cache: Update cache with results
Cache-->>CLI: Cache saved
CLI->>Result: Format upgrade result
Result-->>User: Display success/details
sequenceDiagram
participant Server as Server Init
participant Cache as Update Cache
participant Templates as Template Config
participant Frontend as Frontend JS
participant Banner as UpdateBanner Component
Server->>Cache: GetCachedUpdateInfo()
Cache-->>Server: UpdateAvailable, LatestVersion
Server->>Templates: Inject into funcsConfig
Templates->>Frontend: Render updateAvailable, latestVersion
Frontend-->>Banner: Config with update info
Banner->>Banner: Check localStorage (dismissed state)
Banner->>Banner: Render banner if not dismissed
Banner->>Banner: On dismiss: store to localStorage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@internal/cmd/upgrade.go`:
- Around line 14-21: The -y/yesFlag currently sets Options.Force (used as
skipConfirm), enabling downgrades/reinstalls and bypassing the "already latest"
check; add an explicit "--force" flag wired to Options.Force (e.g., add {name:
"force", usage: "Force reinstall/downgrade", isBool: true} to upgradeFlags) and
change yesFlag to map to a separate "assume yes" behavior (e.g.,
Options.AssumeYes or skipConfirm) so that skipConfirm/assume-yes no longer
bypasses the early-exit "already latest" check; update usages of skipConfirm,
Options.Force, and the early-exit logic in the upgrade flow so only
Options.Force allows reinstall/downgrade and bypasses the latest-version check.
In `@internal/upgrade/cache.go`:
- Around line 108-154: The comment in CheckAndUpdateCache says "Log but don't
fail" but the LoadCache error is discarded; update CheckAndUpdateCache to
actually log the LoadCache error instead of silently setting cache=nil (or
remove the misleading comment). Specifically, inside CheckAndUpdateCache where
it calls LoadCache(), capture the returned err and call the project logger (or
the standard log package) to emit a descriptive message including the error
(e.g., "failed to load upgrade cache: %v") before continuing; reference the
LoadCache, CheckAndUpdateCache, and SaveCache symbols when making the change.
In `@internal/upgrade/download.go`:
- Around line 46-56: Replace the disabled timeout on the Resty HTTP client in
the download logic: instead of SetTimeout(0) on the client created with
resty.New() (the variable named client in internal/upgrade/download.go), set a
reasonable timeout (e.g. 30 minutes) so long-running binary downloads can
complete but the client won’t hang indefinitely; update the call to
SetTimeout(...) accordingly and add the time package import if missing, keeping
the existing SetRetryCount and AddRetryCondition and still using request context
(SetContext(ctx)).
In `@internal/upgrade/install.go`:
- Around line 224-256: The replaceWindowsBinary function fails when the running
binary cannot be renamed on Windows; modify the upgrade flow to perform the
replacement from a separate helper process (or schedule a deferred swap) instead
of attempting an in-process os.Rename. Concretely, change replaceWindowsBinary
(or call sites invoking it) to: write the new binary to a temp target, spawn a
short-lived helper (e.g., "dagu-replacer" or re-invoke the same executable with
a special --replace-mode flag) that waits for the parent PID to exit, then
renames target -> target.old, moves temp -> target, sets perms, and removes
.old; ensure the helper reports errors back (exit code/log) and that the main
process exits after launching the helper when performing an upgrade. Update any
logic that currently assumes immediate in-process replacement to use this
helper/deferred-replace mechanism.
- Around line 78-140: The path-traversal check in extractArchive (using
filepath.Clean + strings.HasPrefix("..") on f.NameInArchive) misses absolute and
Windows drive paths which can escape destDir; update the validation for
f.NameInArchive so you (1) reject absolute paths (use filepath.IsAbs(name)
and/or check filepath.VolumeName(name) != ""), (2) after computing targetPath :=
filepath.Join(destDir, name) verify the final path stays inside destDir by using
filepath.Rel(destDir, targetPath) and ensuring the returned rel path does not
start with ".." or equal ".." (and that no error occurred), and (3) return a
clear error referencing f.NameInArchive on violation; apply these checks before
creating directories or opening files in extractArchive.
In `@internal/upgrade/platform.go`:
- Around line 21-32: The switch on runtime.GOARCH unconditionally sets
p.Arch="armv7" for GOARCH=="arm", causing armv6 devices to get an incompatible
binary; update the branch handling "arm" to inspect runtime.GOARM and set p.Arch
to "armv6" when runtime.GOARM == "6" and "armv7" otherwise (e.g., runtime.GOARM
== "7"), ensuring this logic sits where p.Arch is assigned (in the same switch
that references runtime.GOARCH) and remains consistent with IsSupported() which
lists "armv6" and "armv7".
In `@internal/upgrade/upgrade.go`:
- Around line 103-117: The final "return true, \"\"" after the switch in
CanSelfUpgrade is unreachable; fix by moving the fallback into the switch as a
default case that returns true, "" (or remove the trailing return and add a
default: return true, "" inside the switch), so that
DetectInstallMethod/InstallMethod handling remains exhaustive and there is no
unreachable statement; update the switch in CanSelfUpgrade accordingly
(reference: function CanSelfUpgrade, DetectInstallMethod, InstallMethod* cases).
- Around line 246-252: The UpgradeCheckCache instance saved by SaveCache omits
LastCheck, leaving it as the zero time so IsCacheValid will always fail; set
LastCheck: time.Now() when constructing the UpgradeCheckCache (where
CurrentVersion/LatestVersion/UpdateAvailable are set) and ensure the package
imports time if not already present so the timestamp compiles.
In `@ui/src/layouts/Layout.tsx`:
- Around line 177-180: The page scroll is caused by the outer <main
className="flex-1 overflow-auto"> plus the inner div using h-full; change the
layout to a column flex container and move scrolling into the content pane: make
the <main> a flex column with className including "flex flex-col flex-1 min-h-0"
(so it respects children' min heights), render <UpdateBanner /> first, then
change the inner content div (the one with children) to be the scrollable area
by replacing "p-4 md:p-6 w-full h-full" with "p-4 md:p-6 w-full flex-1 min-h-0
overflow-auto" so the banner remains fixed and only the content pane scrolls.
🧹 Nitpick comments (4)
ui/src/components/UpdateBanner.tsx (1)
33-38: Add compact sizing and explicit focus styles for the dismiss button.This keeps button sizing consistent and ensures a clear keyboard focus indicator.
As per coding guidelines: Use reduced heights for form elements such as select boxes (`h-7` or smaller), buttons (`h-7` or `h-8`), and inputs with compact padding (`py-0.5` or `py-1`); Provide clear focus indicators (but not intrusive ones) and ensure text remains readable at smaller sizes for accessibility.🎯 Suggested styling tweaks
- <button - onClick={handleDismiss} - className="p-0.5 hover:bg-blue-100 dark:hover:bg-blue-900 rounded" + <button + type="button" + onClick={handleDismiss} + className="h-7 w-7 p-0.5 hover:bg-blue-100 dark:hover:bg-blue-900 rounded focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-blue-400 dark:focus-visible:ring-blue-600"internal/upgrade/cache.go (1)
28-46: Consider usingXDG_CACHE_HOMEinstead ofXDG_CONFIG_HOMEfor cache data.Per the XDG Base Directory specification, cache files should use
XDG_CACHE_HOME(defaults to~/.cache), whileXDG_CONFIG_HOME(defaults to~/.config) is intended for configuration files. Since this is ephemeral cache data,XDG_CACHE_HOMEwould be more appropriate.♻️ Suggested fix
func GetCacheDir() (string, error) { - // Use XDG config directory or fallback to ~/.config/dagu - configDir := os.Getenv("XDG_CONFIG_HOME") - if configDir == "" { + // Use XDG cache directory or fallback to ~/.cache/dagu + cacheDir := os.Getenv("XDG_CACHE_HOME") + if cacheDir == "" { homeDir, err := os.UserHomeDir() if err != nil { return "", fmt.Errorf("failed to get home directory: %w", err) } - configDir = filepath.Join(homeDir, ".config") + cacheDir = filepath.Join(homeDir, ".cache") } - cacheDir := filepath.Join(configDir, "dagu") + cacheDir = filepath.Join(cacheDir, "dagu") if err := os.MkdirAll(cacheDir, 0750); err != nil { return "", fmt.Errorf("failed to create cache directory: %w", err) } return cacheDir, nil }internal/upgrade/github.go (1)
114-138: Tag value is interpolated into URL without encoding.While
NormalizeVersionTagsanitizes the prefix, the tag could theoretically contain URL-unsafe characters. Consider usingurl.PathEscapefor robustness.♻️ Suggested fix
+import "net/url" func (c *GitHubClient) GetRelease(ctx context.Context, tag string) (*Release, error) { // Ensure tag has 'v' prefix tag = NormalizeVersionTag(tag) var release Release resp, err := c.client.R(). SetContext(ctx). SetResult(&release). - Get(fmt.Sprintf("%s/tags/%s", githubAPIURL, tag)) + Get(fmt.Sprintf("%s/tags/%s", githubAPIURL, url.PathEscape(tag)))internal/upgrade/upgrade.go (1)
68-101: Docker detection is limited and could produce false positives.The
/.dockerenvcheck (line 86) only detects Docker containers but doesn't account for podman (/run/.containerenv), containerd, or other container runtimes. Additionally, this check is independent of the executable path, so any container installation would be detected as Docker regardless of how dagu was actually installed inside the container.The codebase already demonstrates awareness of these alternatives (e.g., detection constants in
internal/runtime/builtin/docker/client.go), so consider adopting a more comprehensive approach.
|
In airgapped systems, we should have a configuration to switch this off |
|
In airgapped systems, it just does not work silently. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1623 +/- ##
==========================================
- Coverage 69.95% 69.21% -0.75%
==========================================
Files 313 321 +8
Lines 36274 37090 +816
==========================================
+ Hits 25376 25671 +295
- Misses 8883 9230 +347
- Partials 2015 2189 +174
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Will it attempt and then timeout? |
Summary by CodeRabbit
Release Notes