fix(scripts): resolve release downloads from fork remotes#146
fix(scripts): resolve release downloads from fork remotes#146
Conversation
|
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughLauncher scripts now derive the GitHub release repo from LUMEN_RELEASE_REPO or by parsing the git origin (HTTPS/SSH), with tests updated to assert the resolved download URLs. The goreleaser config adds a checksum artifact name_template. ChangesRelease Repository Resolution (launcher + tests)
Release Artifact Checksums (config-only)
Sequence Diagram(s)sequenceDiagram
participant User
participant Launcher
participant Git
participant Curl
participant GitHub
User->>Launcher: run installer (stdio/first-run)
Launcher->>Launcher: resolve_release_repo()
Launcher->>Git: git remote get-url origin
Git-->>Launcher: origin URL (if present)
Launcher->>Launcher: parse owner/repo or use LUMEN_RELEASE_REPO or fallback
Launcher->>Curl: request https://github.com/<owner/repo>/releases/download/vX.Y.Z/asset
Curl->>GitHub: HTTP GET asset
GitHub-->>Curl: binary asset
Curl-->>Launcher: saved binary
Launcher->>User: execute downloaded binary / continue setup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 14 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/test_run.sh (1)
116-125: ⚡ Quick winMissing test assertions for SSH URL variants without
.gitsuffix.
run.sh'srelease_repo_from_remote_urlhandles bothgit@github.com:*/*(no.git) andssh://git@github.com/*/*(no.git), but only the.gitforms are exercised in the assertion block. Adding the bare-URL cases here would close the coverage gap and confirm thecasefall-through order is correct.✅ Suggested additional assertions
assert_eq "parse ssh:// origin with .git" "def324/lumen" \ "$(release_repo_from_remote_url "ssh://git@github.com/def324/lumen.git")" +assert_eq "parse SSH origin without .git" "def324/lumen" \ + "$(release_repo_from_remote_url "git@github.com:def324/lumen")" +assert_eq "parse ssh:// origin without .git" "def324/lumen" \ + "$(release_repo_from_remote_url "ssh://git@github.com/def324/lumen")" assert_eq "ignore non-GitHub origin" "" \ "$(release_repo_from_remote_url "git@example.com:def324/lumen.git")"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test_run.sh` around lines 116 - 125, The test block is missing assertions for SSH URL variants without the .git suffix; add assertions that call release_repo_from_remote_url with "git@github.com:def324/lumen" and "ssh://git@github.com/def324/lumen" and expect "def324/lumen", mirroring the existing ".git" cases (keep the existing non-GitHub ignore case unchanged) so the case fall-through for release_repo_from_remote_url is fully covered.scripts/run.sh (1)
9-20: 💤 Low valueReplace
| sed 's/[.]git$//'with${var%.git}parameter expansion to avoid unnecessary subprocesses.Shellcheck (SC2001) flags these three
sedcalls. The idiomatic shell fix is suffix-stripping with%.git, not a global${var//…/…}replacement. Applied to all three.git-suffixed branches:♻️ Proposed refactor
release_repo_from_remote_url() { local url="$1" case "$url" in - https://github.com/*/*.git) echo "${url#https://github.com/}" | sed 's/[.]git$//' ;; + https://github.com/*/*.git) local r="${url#https://github.com/}"; echo "${r%.git}" ;; https://github.com/*/*) echo "${url#https://github.com/}" ;; - git@github.com:*/*.git) echo "${url#git@github.com:}" | sed 's/[.]git$//' ;; + git@github.com:*/*.git) local r="${url#git@github.com:}"; echo "${r%.git}" ;; git@github.com:*/*) echo "${url#git@github.com:}" ;; - ssh://git@github.com/*/*.git) echo "${url#ssh://git@github.com/}" | sed 's/[.]git$//' ;; + ssh://git@github.com/*/*.git) local r="${url#ssh://git@github.com/}"; echo "${r%.git}" ;; ssh://git@github.com/*/*) echo "${url#ssh://git@github.com/}" ;; *) echo "" ;; esac }The same change applies to the copy of this function in
scripts/test_run.sh(lines 106, 108, 110).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/run.sh` around lines 9 - 20, The release_repo_from_remote_url function uses sed to strip a trailing .git in three branches; replace those subshell pipes with POSIX parameter expansion by assigning the extracted path to a local variable (e.g., repo="${url#https://github.com/}" or similar) and return the stripped form using ${repo%.git}; update the three branches that end with | sed 's/[.]git$//' (https://github.com/*/*.git, git@github.com:*/*.git, ssh://git@github.com/*/*.git) to use this pattern, and make the same edits to the duplicate function in scripts/test_run.sh so no sed subprocesses are spawned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/run.bat`:
- Around line 27-31: Update run.bat to implement the missing "installed checkout
origin" resolution: when LUMEN_RELEASE_REPO is not defined, run git remote
get-url origin (using for /f with delayed expansion) and parse the URL to
extract the owner/repo (strip any .git suffix), then set REPO to that value if
it matches the expected GitHub https://github.com/<owner>/<repo> pattern;
otherwise fall back to setting REPO=ory/lumen. Ensure the logic uses the same
precedence as described (LUMEN_RELEASE_REPO → git origin → ory/lumen) and
integrates with the existing conditional that currently sets REPO in the else
branch.
---
Nitpick comments:
In `@scripts/run.sh`:
- Around line 9-20: The release_repo_from_remote_url function uses sed to strip
a trailing .git in three branches; replace those subshell pipes with POSIX
parameter expansion by assigning the extracted path to a local variable (e.g.,
repo="${url#https://github.com/}" or similar) and return the stripped form using
${repo%.git}; update the three branches that end with | sed 's/[.]git$//'
(https://github.com/*/*.git, git@github.com:*/*.git,
ssh://git@github.com/*/*.git) to use this pattern, and make the same edits to
the duplicate function in scripts/test_run.sh so no sed subprocesses are
spawned.
In `@scripts/test_run.sh`:
- Around line 116-125: The test block is missing assertions for SSH URL variants
without the .git suffix; add assertions that call release_repo_from_remote_url
with "git@github.com:def324/lumen" and "ssh://git@github.com/def324/lumen" and
expect "def324/lumen", mirroring the existing ".git" cases (keep the existing
non-GitHub ignore case unchanged) so the case fall-through for
release_repo_from_remote_url is fully covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fb60da4f-c3f0-4a12-b881-aadf3aced22b
📒 Files selected for processing (5)
.goreleaser.ymlscripts/run.batscripts/run.shscripts/test_run.shscripts/test_run_windows.ps1
💤 Files with no reviewable changes (1)
- .goreleaser.yml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/test_run_windows.ps1 (1)
183-187: ⚡ Quick winAdd an SSH-origin case for the Windows parser.
Line 186 only exercises the HTTPS branch, but
run.batnow has separate handling forgit@github.com:andssh://git@github.com/. A broken offset in those Windows-specific branches would still pass CI, so I'd add at least one SSH remote scenario here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test_run_windows.ps1` around lines 183 - 187, Add a new Invoke-RunBatScenario invocation to cover an SSH-origin case so the Windows parser's SSH branches are exercised: duplicate the existing Invoke-RunBatScenario block (the one using Name 'origin-remote') and change the OriginUrl to an SSH format (e.g. 'git@github.com:def324/lumen.git' or 'ssh://git@github.com/def324/lumen.git'), keep ExpectedRepo 'def324/lumen' and update PassMessage to indicate it's the SSH-origin case (e.g. 'run.bat stdio derives first-install download repo from git origin (ssh)') so the test suite hits the git@/ssh:// handling in run.bat.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/test_run_windows.ps1`:
- Around line 183-187: Add a new Invoke-RunBatScenario invocation to cover an
SSH-origin case so the Windows parser's SSH branches are exercised: duplicate
the existing Invoke-RunBatScenario block (the one using Name 'origin-remote')
and change the OriginUrl to an SSH format (e.g.
'git@github.com:def324/lumen.git' or 'ssh://git@github.com/def324/lumen.git'),
keep ExpectedRepo 'def324/lumen' and update PassMessage to indicate it's the
SSH-origin case (e.g. 'run.bat stdio derives first-install download repo from
git origin (ssh)') so the test suite hits the git@/ssh:// handling in run.bat.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d73981e-bca0-4e9a-8c34-42b632a88b39
📒 Files selected for processing (4)
scripts/run.batscripts/run.shscripts/test_run.shscripts/test_run_windows.ps1
✅ Files skipped from review due to trivial changes (1)
- scripts/run.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/test_run.sh
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/test_run_windows.ps1 (2)
177-194: ⚡ Quick winConsider adding coverage for the
ssh://git@github.com/origin format and theory/lumenfallback.
run.bathandles three URL schemes (context snippet 1, lines 41–45):
https://github.com/— covered byorigin-remotegit@github.com:— covered byorigin-remote-sshssh://git@github.com/— not coveredThe default fallback to
ory/lumenwhen neitherLUMEN_RELEASE_REPOnor a parseable origin is present is also untested.♻️ Example additional scenario calls
+ Invoke-RunBatScenario ` + -Name 'origin-remote-ssh-url' ` + -ExpectedRepo 'def324/lumen' ` + -OriginUrl 'ssh://git@github.com/def324/lumen.git' ` + -PassMessage 'run.bat stdio derives first-install download repo from ssh:// git origin' + + Invoke-RunBatScenario ` + -Name 'fallback' ` + -ExpectedRepo 'ory/lumen' ` + -PassMessage 'run.bat stdio falls back to ory/lumen when no origin or env override is set'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test_run_windows.ps1` around lines 177 - 194, Add two test scenarios to exercise the missing origin format and the fallback: create an Invoke-RunBatScenario call similar to the existing origin-remote-ssh test but with OriginUrl set to 'ssh://git@github.com/def324/lumen.git' (use Name 'origin-remote-ssh-sshproto' or similar and the same ExpectedRepo and PassMessage pattern) to cover the ssh://git@github.com/ form, and add another Invoke-RunBatScenario with no ReleaseRepoOverride and no OriginUrl (Name 'fallback-ory-lumen') that expects 'ory/lumen' as the ExpectedRepo and a PassMessage noting the fallback; keep the call shape identical to the other scenarios so run.bat stdio assertions exercise both cases.
131-136: 💤 Low valueOptional: silence
PSAvoidUsingEmptyCatchBlockwith a discarding assignment.The empty catches at lines 135–136 are intentional (broken-pipe on fast exit is documented in the comment above), but PSScriptAnalyzer flags them. A one-liner suppresses the lint warning without changing behavior:
♻️ Proposed change
- try { $script:proc.StandardInput.WriteLine($initReq) } catch { } - try { $script:proc.StandardInput.Close() } catch { } + try { $script:proc.StandardInput.WriteLine($initReq) } catch { $null = $_ } + try { $script:proc.StandardInput.Close() } catch { $null = $_ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test_run_windows.ps1` around lines 131 - 136, The empty catch blocks after the calls to $script:proc.StandardInput.WriteLine($initReq) and $script:proc.StandardInput.Close() are intentional but trigger PSScriptAnalyzer's PSAvoidUsingEmptyCatchBlock; update each catch to explicitly discard the caught exception (e.g. catch { $null = $_ } or catch { $null = $_; } ) so behavior is unchanged but the linter is satisfied; locate the two try/catch pairs around calls to StandardInput.WriteLine and StandardInput.Close and replace the empty catch bodies with the discarding assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/test_run_windows.ps1`:
- Around line 177-194: Add two test scenarios to exercise the missing origin
format and the fallback: create an Invoke-RunBatScenario call similar to the
existing origin-remote-ssh test but with OriginUrl set to
'ssh://git@github.com/def324/lumen.git' (use Name 'origin-remote-ssh-sshproto'
or similar and the same ExpectedRepo and PassMessage pattern) to cover the
ssh://git@github.com/ form, and add another Invoke-RunBatScenario with no
ReleaseRepoOverride and no OriginUrl (Name 'fallback-ory-lumen') that expects
'ory/lumen' as the ExpectedRepo and a PassMessage noting the fallback; keep the
call shape identical to the other scenarios so run.bat stdio assertions exercise
both cases.
- Around line 131-136: The empty catch blocks after the calls to
$script:proc.StandardInput.WriteLine($initReq) and
$script:proc.StandardInput.Close() are intentional but trigger
PSScriptAnalyzer's PSAvoidUsingEmptyCatchBlock; update each catch to explicitly
discard the caught exception (e.g. catch { $null = $_ } or catch { $null = $_; }
) so behavior is unchanged but the linter is satisfied; locate the two try/catch
pairs around calls to StandardInput.WriteLine and StandardInput.Close and
replace the empty catch bodies with the discarding assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f6280085-4413-4aed-837a-8c560a93108d
📒 Files selected for processing (1)
scripts/test_run_windows.ps1
2 participants
Summary
Verification
Draft PR for CI verification and separate review from the stdio lifecycle fix.
Summary by CodeRabbit
New Features
Tests
Chores