Smar 104 automate louper support when adding new network viem update pr#1632
Smar 104 automate louper support when adding new network viem update pr#1632
Conversation
WalkthroughAdds a GitHub Actions workflow Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
…-new-network-viem-update-PR
![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
🤖 Fix all issues with AI agents
In @.github/workflows/updateLouperViem.yml:
- Around line 35-76: The Detect Newly Added Networks step always compares
against main when run via workflow_dispatch, so OLD_NETWORKS equals NEW_NETWORKS
and ADDED_NETWORKS is always empty; update the workflow to accept a
workflow_dispatch input (e.g., base_sha or compare_ref) and use that input to
set BASE_SHA (falling back to pull_request.base.sha when present) before
fetching and git show, and add set -e at the top of the run block to enable
strict failure handling; update uses of BASE_SHA and the origin/main fallback
logic accordingly so manual runs can specify the commit/branch to diff against.
- Around line 144-145: The pipeline currently swallows merge conflicts by
appending '|| echo "Merge conflicts or already up to date"' to the git merge
command; remove that fallback and instead fail the job if conflicts occur: run
git merge "origin/${DEFAULT_BRANCH}" --no-edit without the '|| echo' and
immediately after the merge check for conflicts (e.g. using git diff --name-only
--diff-filter=U or git ls-files -u) and if any conflicted files are found output
a clear error and exit non‑zero so later steps (jq edits, npm install, git
commit) do not run on a conflicted working tree.
- Around line 220-224: The git commit command is currently interpolating `${{
env.added_networks }}` (and similarly `${{ env.network_details }}`) before the
shell runs, creating a shell-injection risk; fix by moving those GH Actions
values into step-level environment variables (e.g. ADDED_NETWORKS and
NETWORK_DETAILS set to `${{ env.added_networks }}` and `${{ env.network_details
}}`) and then reference the safe shell variables inside the commit message
command (use quoted expansion like "$ADDED_NETWORKS" piped to jq/sed) so the
shell sees only the env var contents at runtime and proper quoting prevents
injection; update both the git commit invocation that uses env.added_networks
and the similar invocation that uses env.network_details.
- Around line 278-286: The script currently treats EXISTING_PR as "updating" but
only performs a GET to fetch the PR URL; change the GET to a PATCH that updates
the PR title/body with the new network details before setting PR_URL and
PR_NUMBER. Locate the block referencing EXISTING_PR and PR_URL and replace the
curl call that hits
"https://api.github.com/repos/mark3labs/louper/pulls/$EXISTING_PR" with a PATCH
request using the same GITHUB_TOKEN and Accept header, sending a JSON payload
containing the updated title and body (the new network details variable) and
then parse the returned .html_url into PR_URL as done now. Ensure the
Authorization header remains "token ${GITHUB_TOKEN}" and that the request
content-type/Accept are set to application/vnd.github.v3+json so the PR is
actually updated.
- Line 37: Add shell error handling to each multi-line run: block by inserting
set -e at the very top of the script body so any failing command aborts the
step; update all run: blocks in the workflow (the four multi-line script blocks
present) to begin with set -e (or equivalent explicit checks) so commands like
jq or git fetch cannot fail silently.
- Around line 119-121: CURRENT_VIEM is read raw from package.json and may
include a ^ or ~ prefix, so the if comparison against LATEST_VIEM will never
match; fix by stripping leading ^ or ~ from CURRENT_VIEM before the comparison
(e.g., create CURRENT_VIEM_NOPREFIX by removing a leading '^' then removing a
leading '~' using bash parameter expansion on CURRENT_VIEM), then compare
CURRENT_VIEM_NOPREFIX to LATEST_VIEM in the existing if block that checks
CURRENT_VIEM == LATEST_VIEM so the workflow correctly detects when viem is
already at the latest version.
🧹 Nitpick comments (5)
.github/workflows/updateLouperViem.yml (5)
17-19:pull-requests: writepermission is misleading and unnecessary.This permission applies to the current repository's
GITHUB_TOKEN, but PRs are created in the externalmark3labs/louperrepo usingGIT_ACTIONS_BOT_PAT_CLASSIC. The default token'spull-requests: writescope has no effect on external repositories. Remove it to follow least-privilege.Suggested fix
permissions: contents: read # required to read repository contents - pull-requests: write # required to create PR in target repository
22-27: Add aconcurrencygroup to prevent duplicate workflow runs.If two PRs affecting
networks.jsonmerge in quick succession, parallel runs could race on the same target branch inmark3labs/louper. As per coding guidelines, workflows should useconcurrency:groups to prevent duplicate runs.Suggested addition after line 23
update-louper-viem: runs-on: ubuntu-latest + concurrency: + group: update-louper-viem + cancel-in-progress: false # Only run when PR is merged to main branch
250-256: Duplicate GitHub API call to detect the default branch.The default branch is already fetched in the previous step (line 102-105) and could be stored in
$GITHUB_ENVfor reuse here, avoiding a redundant API call.Suggested fix — add to lines 105-106 in the previous step
DEFAULT_BRANCH=$(echo "$REPO_INFO" | jq -r '.default_branch // "dev"') echo "Default branch: ${DEFAULT_BRANCH}" + echo "DEFAULT_BRANCH=${DEFAULT_BRANCH}" >> $GITHUB_ENVThen replace lines 250-256 with:
- # Detect default branch of target repository - REPO_INFO=$(curl -s -H "Authorization: token ${GITHUB_TOKEN}" \ - -H "Accept: application/vnd.github.v3+json" \ - "https://api.github.com/repos/mark3labs/louper") - - DEFAULT_BRANCH=$(echo "$REPO_INFO" | jq -r '.default_branch // "dev"') - echo "Detected default branch: ${DEFAULT_BRANCH}" + DEFAULT_BRANCH="${{ env.DEFAULT_BRANCH }}" + echo "Using default branch: ${DEFAULT_BRANCH}"
315-327: Use$GITHUB_STEP_SUMMARYfor better visibility in the Actions UI.Plain
echooutput is buried in step logs. Writing to$GITHUB_STEP_SUMMARYrenders Markdown on the workflow run's summary page, making outcomes easier to find.Suggested fix
- name: Summary if: always() run: | - echo "## Summary" - echo "" + { + echo "## Summary" + echo "" if [ "${{ env.SKIP_UPDATE }}" == "true" ]; then - echo "✅ No new networks detected. Skipped update." + echo "✅ No new networks detected. Skipped update." elif [ "${{ env.UPDATE_NEEDED }}" == "true" ]; then - echo "✅ Updated viem to ${{ env.LATEST_VIEM }} and created/updated PR in target repository" - echo "PR: ${{ env.PR_URL }}" + echo "✅ Updated viem to ${{ env.LATEST_VIEM }} and created/updated PR in target repository" + echo "PR: ${{ env.PR_URL }}" else - echo "✅ viem is already at latest version. No update needed." + echo "✅ viem is already at latest version. No update needed." fi + } | tee -a "$GITHUB_STEP_SUMMARY"
84-91: Externalize the hardcoded target repository as a workflow-levelenvvariable.
mark3labs/louperappears in 5+ places (lines 88, 104, 253, 261, 293). A typo in any one of them would silently break the workflow. Define it once at the workflow or job level:Suggested addition at the top level (after line 9)
+env: + TARGET_REPO: mark3labs/louper + on:Then replace all occurrences, e.g.:
- repository: mark3labs/louper + repository: ${{ env.TARGET_REPO }}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/updateLouperViem.yml:
- Around line 67-73: The shell is word-splitting unquoted JSON variables: wrap
variable expansions in double quotes and add a brief comment/initialization;
specifically change echo "added_networks=$(echo $ADDED_NETWORKS | jq -c .)" to
use echo "added_networks=$(echo "$ADDED_NETWORKS" | jq -c .)" and change echo
"network_details=$(echo $NETWORK_DETAILS | jq -c .)" to echo
"network_details=$(echo "$NETWORK_DETAILS" | jq -c .)"; ensure ADDED_NETWORKS is
initialized/empty-defaulted before use and keep the jq invocation (jq -n
--argjson networks "$ADDED_NETWORKS" --argjson all "$(cat config/networks.json)"
...) as-is but add a one-line comment above the block explaining it extracts
name/chainId.
- Around line 159-177: The script currently replaces versions like "^2.20.0"
with "2.25.0" losing any semver prefix; modify the update logic to preserve the
existing prefix when present by extracting a prefix from VIEM_EXISTS (e.g.
PREFIX=$(echo "$VIEM_EXISTS" | sed -n 's/^\([~^]\).*/\1/p') || PREFIX="^") and
then use "${PREFIX}${LATEST_VIEM}" in the jq writes (the jq assignments that set
.dependencies.viem and .devDependencies.viem and the add-to-devDependencies
fallback). Ensure the same prefix-preserving behavior is applied in the branches
that check/jq-update .dependencies.viem, .devDependencies.viem and when adding
viem if not found.
🧹 Nitpick comments (4)
.github/workflows/updateLouperViem.yml (4)
16-18:pull-requests: writepermission comment is misleading.The
pull-requests: writescope on the workflow'sGITHUB_TOKENapplies to this repository (lifinance/contracts), not the target (mark3labs/louper). The target repo PR creation/update uses the PAT (GIT_ACTIONS_BOT_PAT_CLASSIC). If this permission isn't used for anything else in the workflow, it can be removed to follow least-privilege. If it's kept, update the comment to accurately describe its purpose.As per coding guidelines: "Always document why each permission is needed with inline comments" and "Use minimal permissions (principle of least privilege)."
20-24: Add aconcurrencygroup to prevent duplicate runs.If two PRs modifying
config/networks.jsonmerge in quick succession, two instances of this workflow will race on the same target repo branch and may conflict. A concurrency group serializes or cancels duplicate runs.Proposed fix
jobs: update-louper-viem: runs-on: ubuntu-latest + concurrency: + group: update-louper-viem + cancel-in-progress: false # Only run when PR is merged to main branch if: github.event.pull_request.merged == true && github.event.pull_request.base.ref == 'main'As per coding guidelines: "Use
concurrency:groups to prevent duplicate runs on the same PR."
262-267: Default branch is fetched via API twice — consider reusing from the previous step.The same GitHub API call to detect the default branch is made both in the "Update viem" step (line 102) and here (line 262). You could set
DEFAULT_BRANCHas an$GITHUB_ENVvariable in the first step and reuse it, avoiding a redundant API call.
296-298: PR title uses raw${{ env.LATEST_VIEM }}interpolation inside JSON, unlike the body which is properly escaped.The body is safely escaped via
jq -Rs ., but the title is directly interpolated into the JSON payload string. WhileLATEST_VIEMfromnpm viewis practically a safe semver string, this is inconsistent. For robustness, build the entire payload withjq:Proposed fix (for the PATCH case; apply similarly to POST)
- PR_RESPONSE=$(curl -s -X PATCH \ - -H "Authorization: token ${GITHUB_TOKEN}" \ - -H "Accept: application/vnd.github.v3+json" \ - "https://api.github.com/repos/mark3labs/louper/pulls/$EXISTING_PR" \ - -d "{ - \"title\": \"chore: Update viem to ${{ env.LATEST_VIEM }}\", - \"body\": $(echo "$PR_BODY" | jq -Rs .) - }") + PATCH_PAYLOAD=$(jq -n \ + --arg title "chore: Update viem to ${LATEST_VIEM}" \ + --arg body "$PR_BODY" \ + '{title: $title, body: $body}') + PR_RESPONSE=$(curl -s -X PATCH \ + -H "Authorization: token ${GITHUB_TOKEN}" \ + -H "Accept: application/vnd.github.v3+json" \ + "https://api.github.com/repos/mark3labs/louper/pulls/$EXISTING_PR" \ + -d "$PATCH_PAYLOAD")Also applies to: 311-313
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/updateLouperViem.yml:
- Around line 209-220: UPDATED_VIEM is read from package.json with its semver
prefix (e.g., ^2.25.0) while LATEST_VIEM is a bare version, so the equality
check in the verification block (using UPDATED_VIEM and LATEST_VIEM) always
fails; fix by normalizing UPDATED_VIEM before comparison—strip any leading
range/prefix characters (like ^, ~, >=, <=) the same way used earlier for
LATEST_VIEM, then compare the stripped value to LATEST_VIEM and only emit the
warning if they differ.
- Around line 298-312: The PATCH branch doesn't validate the GitHub API response
(PR_RESPONSE) so failures leave PR_URL null and silently proceed; after creating
PR_RESPONSE in the block that updates an existing PR (using EXISTING_PR) check
the HTTP status and the parsed html_url like the POST branch: parse a status
code or check jq -r '.html_url' for "null" and if invalid print the PR_RESPONSE
(or a clear error) and exit non‑zero, otherwise export PR_URL and PR_NUMBER to
GITHUB_ENV as currently done; ensure the logic mirrors the POST path's
validation and error handling.
🧹 Nitpick comments (6)
.github/workflows/updateLouperViem.yml (6)
16-18:pull-requests: writepermission is unused and over-broad.This workflow never modifies PRs in this repository — all cross-repo PR operations use the PAT (
GIT_ACTIONS_BOT_PAT_CLASSIC). TheGITHUB_TOKEN'spull-requests: writescope has no effect on the target repository (mark3labs/louper). You can drop it to follow least-privilege.Suggested fix
permissions: contents: read # required to read repository contents - pull-requests: write # required to create PR in target repositoryAs per coding guidelines: "Use minimal permissions (principle of least privilege)" and "Always document why each permission is needed with inline comments."
20-24: Missingconcurrencygroup to prevent duplicate workflow runs.If two PRs modifying
config/networks.jsonare merged in quick succession, two workflow runs could race against each other pushing to the same branch in the target repo. Add a concurrency group.Suggested fix
jobs: update-louper-viem: runs-on: ubuntu-latest + concurrency: + group: update-louper-viem + cancel-in-progress: false # Only run when PR is merged to main branch if: github.event.pull_request.merged == true && github.event.pull_request.base.ref == 'main'As per coding guidelines: "Use
concurrency:groups to prevent duplicate runs on the same PR."
284-296: Remaining${{ env.* }}expressions still interpolated before the shell.Lines 288-291 use
${{ env.LATEST_VIEM }},${{ env.CURRENT_VIEM }}, etc., which are expanded by the Actions runner before the shell executes. While these are low-risk (semver strings and GitHub context values), the same hardening applied toadded_networks/network_details(step-levelenv:+ shell variable references) would be consistent.Suggested fix — add to step env
- name: Create or Update Pull Request in target repository if: env.SKIP_UPDATE != 'true' && env.UPDATE_NEEDED == 'true' env: GITHUB_TOKEN: ${{ secrets.GIT_ACTIONS_BOT_PAT_CLASSIC }} NETWORK_DETAILS_JSON: ${{ env.network_details }} + LATEST_VIEM: ${{ env.LATEST_VIEM }} + CURRENT_VIEM: ${{ env.CURRENT_VIEM }} + BRANCH_NAME: ${{ env.BRANCH_NAME }} + SOURCE_REPO: ${{ github.repository }} + SOURCE_SHA: ${{ github.sha }}Then replace
${{ env.LATEST_VIEM }}→${LATEST_VIEM},${{ env.CURRENT_VIEM }}→${CURRENT_VIEM}, etc. in therun:block.
341-354: Consider writing to$GITHUB_STEP_SUMMARYfor visibility.The summary output only goes to the log. Using
$GITHUB_STEP_SUMMARYwould surface results directly on the Actions run page.Example
echo "## Summary" + echo "## Summary" >> "$GITHUB_STEP_SUMMARY" echo "" if [ "${{ env.SKIP_UPDATE }}" == "true" ]; then echo "✅ No new networks detected. Skipped update." + echo "✅ No new networks detected. Skipped update." >> "$GITHUB_STEP_SUMMARY" elif [ "${{ env.UPDATE_NEEDED }}" == "true" ]; then echo "✅ Updated viem to ${{ env.LATEST_VIEM }} and created/updated PR in target repository" + echo "✅ Updated viem to ${{ env.LATEST_VIEM }} and created/updated PR" >> "$GITHUB_STEP_SUMMARY" echo "PR: ${{ env.PR_URL }}" + echo "PR: ${{ env.PR_URL }}" >> "$GITHUB_STEP_SUMMARY" else echo "✅ viem is already at latest version. No update needed." + echo "✅ viem is already at latest version. No update needed." >> "$GITHUB_STEP_SUMMARY" fi
3-8: Header comment is slightly inaccurate.Line 7 says "runs on push to main branch (after PR merge) or when networks.json changes" — the "or" implies two separate triggers, but the actual trigger (line 10-14) is solely
pull_request: types: [closed]with a paths filter. It doesn't trigger on push events. Consider rephrasing for accuracy.Suggested fix
-- runs on push to main branch (after PR merge) or when networks.json changes +- runs when a PR that modifies config/networks.json is merged into mainAs per coding guidelines: the header comment block should accurately describe "Triggers: When it runs (events, branches, paths)."
91-96: Duplicate GitHub API call for default branch detection.The default branch is fetched from the GitHub API at lines 102-106 (inside "Update viem") and again at lines 271-276 (inside "Create or Update PR"). Consider storing
DEFAULT_BRANCHin$GITHUB_ENVafter the first call and reading it in the second step to avoid the redundant API request.
2 participants
Which Jira task belongs to this PR?
https://lifi.atlassian.net/browse/SMAR-104
Why did I implement it this way?
Because a new GitHub action it's required in order to update Louper viem version to the latest one
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)