Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an AI-driven contract changelog system: new GitHub Actions workflow, Bun/TypeScript analyzer and orchestrator scripts, documentation and package script, writes changelogs under Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
…ntract-changelogs-using-AI
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| run: | | ||
| echo "## Contract Changelog Generation" >> $GITHUB_STEP_SUMMARY | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
|
|
||
| echo "**Mode**: 🤖 AI-powered semantic analysis (Claude Sonnet)" >> $GITHUB_STEP_SUMMARY | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
|
|
||
| if [ "${{ steps.check.outputs.has_changes }}" == "true" ]; then | ||
| echo "✅ Changelog file created successfully" >> $GITHUB_STEP_SUMMARY | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| echo "### New changelog file(s):" >> $GITHUB_STEP_SUMMARY | ||
| git diff --name-only --diff-filter=A | grep "^changelog/.*\.md$" | sed 's/^/- /' >> $GITHUB_STEP_SUMMARY || echo "- (see commit for details)" >> $GITHUB_STEP_SUMMARY | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| echo "### Contract changes analyzed:" >> $GITHUB_STEP_SUMMARY | ||
| # Show files from the merged PR or push | ||
| if [ -n "${{ github.event.pull_request.number }}" ]; then | ||
| echo "**PR #${{ github.event.pull_request.number }}**: ${{ github.event.pull_request.title }}" >> $GITHUB_STEP_SUMMARY | ||
| fi | ||
| git diff HEAD~1 --name-only --diff-filter=AM src/**/*.sol 2>/dev/null | sed 's/^/- /' >> $GITHUB_STEP_SUMMARY || echo "- (see commit for details)" >> $GITHUB_STEP_SUMMARY | ||
| else | ||
| echo "ℹ️ No contract changes detected or changelog file already exists for this commit" >> $GITHUB_STEP_SUMMARY | ||
| fi |
There was a problem hiding this comment.
Template Injection in GitHub Workflows Action - critical severity
A GitHub Actions workflow step contains a template expression referencing potentially untrusted GitHub context fields. This may allow malicious input to be injected into shell commands, leading to a potential supply chain attack as tokens of the CI/CD pipeline could be exfiltrated.
Show fix
Remediation: Review your GitHub Actions workflow for any template expressions that interpolate GitHub context values, especially those ending with unsafe suffixes such as 'body', 'title', 'email', 'head_ref', etc. Sanitize or validate these inputs before use, or refactor the workflow to avoid directly embedding untrusted data in shell commands.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| token: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Setup Bun | ||
| uses: oven-sh/setup-bun@v1 |
There was a problem hiding this comment.
3rd party Github Actions should be pinned - medium severity
A third-party GitHub Action was imported, and is not pinned via a hash. This leaves your CI/CD at risk for potential supply chain attacks, if the affected GitHub Action is compromised.
| uses: oven-sh/setup-bun@v1 | |
| uses: oven-sh/setup-bun@f4d14e03ff726c06358e5557344e1da148b56cf7 # v1.2.2 |
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
Test Coverage ReportLine Coverage: 86.72% (2985 / 3442 lines) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (2)
.github/workflows/generateContractChangelog.yml (2)
50-51:⚠️ Potential issue | 🔴 Critical
oven-sh/setup-bunmust be pinned to a full 40-character commit SHA. Also note that v2 is the current major release; v1 is outdated.🔒 Proposed fix (upgrade to v2 with SHA pinning)
- uses: oven-sh/setup-bun@v1 + uses: oven-sh/setup-bun@8f24390df009a496891208e5e36b8a1de1f45135 # v2.0.2If staying on v1, use the pinned SHA from the Aikido bot suggestion:
- uses: oven-sh/setup-bun@v1 + uses: oven-sh/setup-bun@f4d14e03ff726c06358e5557344e1da148b56cf7 # v1.2.2As per coding guidelines: "Every
uses:reference in GitHub Actions workflows MUST use a full 40-character commit SHA, never tags."Please verify the exact SHA for the desired setup-bun version:
#!/bin/bash # Fetch the SHA for oven-sh/setup-bun v2 and v1.2.2 tags via GitHub API gh api repos/oven-sh/setup-bun/git/refs/tags/v2 --jq '.object.sha' gh api repos/oven-sh/setup-bun/git/refs/tags/v1.2.2 --jq '.object.sha'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/generateContractChangelog.yml around lines 50 - 51, Replace the unpinned action reference "uses: oven-sh/setup-bun@v1" with a full 40-character commit SHA and preferably update to the v2 release; locate the line containing uses: oven-sh/setup-bun@v1 and change it to uses: oven-sh/setup-bun@<full-commit-sha-for-v2> (or if you must stay on v1, use the full commit SHA for v1.2.2), fetching the exact SHA via the GitHub API/gh commands suggested in the review and ensure the workflow now references the full 40-character SHA instead of a tag.
95-118:⚠️ Potential issue | 🔴 CriticalTemplate injection via
github.event.pull_request.title+ unquoted$GITHUB_STEP_SUMMARY.Template injection (critical): Line 113 interpolates
${{ github.event.pull_request.title }}directly into the shell script. A PR title containing backticks or$()sequences executes arbitrary commands with the workflow'scontents: writepermissions. Pass untrusted context values through environment variables instead.Unquoted
$GITHUB_STEP_SUMMARY(minor): Every>> $GITHUB_STEP_SUMMARYappend is unquoted, which causes word splitting / globbing if the path ever contains whitespace (SC2086 flagged by actionlint throughout this step).🔒 Proposed fix
- name: Summary if: always() + env: + PR_NUMBER: ${{ github.event.pull_request.number }} + PR_TITLE: ${{ github.event.pull_request.title }} + HAS_CHANGES: ${{ steps.check.outputs.has_changes }} run: | - echo "## Contract Changelog Generation" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - - echo "**Mode**: 🤖 AI-powered semantic analysis (Claude Sonnet)" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - - if [ "${{ steps.check.outputs.has_changes }}" == "true" ]; then - echo "✅ Changelog file created successfully" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - echo "### New changelog file(s):" >> $GITHUB_STEP_SUMMARY - git diff --name-only --diff-filter=A | grep "^changelog/.*\.md$" | sed 's/^/- /' >> $GITHUB_STEP_SUMMARY || echo "- (see commit for details)" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - echo "### Contract changes analyzed:" >> $GITHUB_STEP_SUMMARY - # Show files from the merged PR or push - if [ -n "${{ github.event.pull_request.number }}" ]; then - echo "**PR #${{ github.event.pull_request.number }}**: ${{ github.event.pull_request.title }}" >> $GITHUB_STEP_SUMMARY - fi - git diff HEAD~1 --name-only --diff-filter=AM src/**/*.sol 2>/dev/null | sed 's/^/- /' >> $GITHUB_STEP_SUMMARY || echo "- (see commit for details)" >> $GITHUB_STEP_SUMMARY - else - echo "ℹ️ No contract changes detected or changelog file already exists for this commit" >> $GITHUB_STEP_SUMMARY - fi + { + echo "## Contract Changelog Generation" + echo "" + echo "**Mode**: 🤖 AI-powered semantic analysis (Claude Sonnet)" + echo "" + if [ "${HAS_CHANGES}" == "true" ]; then + echo "✅ Changelog file created successfully" + echo "" + echo "### New changelog file(s):" + git ls-files --others --exclude-standard | grep "^changelog/.*\.md$" | sed 's/^/- /' || echo "- (see commit for details)" + echo "" + echo "### Contract changes analyzed:" + if [ -n "${PR_NUMBER}" ]; then + echo "**PR #${PR_NUMBER}**: ${PR_TITLE}" + fi + git diff HEAD~1 --name-only --diff-filter=AM -- 'src/**/*.sol' 2>/dev/null | sed 's/^/- /' || echo "- (see commit for details)" + else + echo "ℹ️ No contract changes detected or changelog file already exists for this commit" + fi + } >> "$GITHUB_STEP_SUMMARY"Note: After
git push(line 93) runs,HEADadvances to the changelog commit, sogit diff HEAD~1in the Summary step compares the changelog commit to the merge commit — it will show onlychangelog/*.mdadditions, not the original.solchanges. Consider capturing the list of changed Solidity files as a step output in the "Generate changelog" step before the commit for accurate summary reporting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/generateContractChangelog.yml around lines 95 - 118, Escape untrusted PR title and quote summary path: stop interpolating `${{ github.event.pull_request.title }}` directly in the shell and instead pass it into the job step as an environment variable (e.g., CH_PR_TITLE) from the workflow expression, then reference `$CH_PR_TITLE` in the script; also quote every use of the summary file path as `"$GITHUB_STEP_SUMMARY"` to avoid word-splitting/globbing. While editing the Summary step, replace the direct expression with the env var reference and ensure all `>> $GITHUB_STEP_SUMMARY` occurrences become `>> "$GITHUB_STEP_SUMMARY"`; additionally, capture the list of changed Solidity files earlier (e.g., in the "Generate changelog" step) and expose it as a step output (like changed_sol_files) so the Summary step uses that output instead of `git diff HEAD~1` which will only show the changelog commit.
🧹 Nitpick comments (3)
src/Facets/WithdrawFacet.sol (1)
27-31:totalAmountis ambiguous for multi-asset batches.Summing amounts across different assets/decimals is misleading for analytics. Consider emitting the per-asset amounts (or rely solely on
LogWithdrawevents) instead of a single total.💡 Example adjustment
- event BatchWithdrawCompleted( - address[] indexed assets, - address indexed to, - uint256 totalAmount - ); + event BatchWithdrawCompleted( + address[] indexed assets, + address indexed to, + uint256[] amounts + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Facets/WithdrawFacet.sol` around lines 27 - 31, The BatchWithdrawCompleted event's totalAmount field is ambiguous across multiple assets; change the event signature BatchWithdrawCompleted to emit per-asset amounts (e.g., uint256[] amounts) instead of a single totalAmount (or remove totalAmount entirely), then update every emit of BatchWithdrawCompleted in the withdraw batch logic to supply the per-asset amounts (computed alongside assets and amounts used in the batch) and adjust any consumers/tests that read BatchWithdrawCompleted; also consider relying on existing LogWithdraw events if you prefer not to add per-asset arrays..github/workflows/generateContractChangelog.yml (2)
32-34: Add inline comments to explain each permission.♻️ Proposed fix
permissions: - contents: write - pull-requests: read + contents: write # Required to commit and push generated changelog files + pull-requests: read # Required to read PR metadata (number, title, base.ref)As per coding guidelines: "minimal required permissions and inline comments explaining each permission."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/generateContractChangelog.yml around lines 32 - 34, The workflow currently grants permissions without inline comments; update the permissions block so each entry (contents and pull-requests) includes a short inline comment explaining why that minimal permission is needed (e.g., contents: write — to push generated changelog commits; pull-requests: read — to read PR metadata for changelog generation). Edit the permissions section around the entries "contents" and "pull-requests" to add these concise comments and ensure no broader permissions are granted.
36-40: Add aconcurrency:group to prevent overlapping git push conflicts.Without it, two PRs merged in quick succession can have concurrent runs both attempting
git pushto main, causing the second push to fail with a non-fast-forward error. Usecancel-in-progress: falseso that each run's unique changelog file is still committed — they just queue up instead of racing.♻️ Proposed addition
jobs: generate-changelog: + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: false # Each run generates a unique changelog; don't cancel + # Only run if: (1) PR was merged (not just closed), OR (2) manually triggered if: github.event.pull_request.merged == true || github.event_name == 'workflow_dispatch'As per coding guidelines: "Use
concurrency:groups in workflows to prevent duplicate runs on the same PR in GitHub Actions workflows."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/generateContractChangelog.yml around lines 36 - 40, Add a concurrency group to the generate-changelog job to serialize runs and avoid overlapping git pushes: in the job block for generate-changelog, add a concurrency stanza (e.g., concurrency: { group: "generate-changelog-${{ github.ref }}", cancel-in-progress: false }) so runs queue instead of racing when both attempt git push.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/generateContractChangelog.yml:
- Around line 69-82: The current "Check for changes" step (id: check) uses git
diff --name-only --diff-filter=A which misses untracked files created but not
staged; update the step to detect both staged/committed additions and untracked
files by using git diff --name-only --diff-filter=A HEAD (or similar against
HEAD) for added paths and git ls-files --others --exclude-standard to list
untracked files, merge those results into a single list, set GITHUB_OUTPUT
has_changes=true when the combined list is non-empty, and print each matched
path (so the later "Commit and push changelog" step will run).
- Around line 43-44: The workflow uses a mutable tag reference "uses:
actions/checkout@v4"; replace that with a pinned full 40-character commit SHA
for the actions/checkout action to satisfy the policy. Locate the "uses:
actions/checkout@v4" line in the workflow and update it to "uses:
actions/checkout@<FULL_40_CHAR_SHA>" (obtain the SHA from the actions/checkout
repository commit you intend to pin), commit the change, and ensure no other
"uses:" entries in the file use tags or short SHAs.
In `@changelog/README.md`:
- Around line 31-36: Update the changelog README to match the implementation:
replace the “CodeRabbit API” mention with “Claude Sonnet (Anthropic)” and change
the workflow filename reference from `generate-contract-changelog.yml` to the
camel-cased `generateContractChangelog.yml`; ensure the Related Files list still
references the workflow `generateContractChangelog.yml` and the script
`script/changelogAnalysis/generateContractChangelog.ts` (or
`generateContractChangelog.ts` if that’s the actual script name) so the doc text
and filenames align with the code.
In `@script/changelogAnalysis/aiChangelogAnalyzer.ts`:
- Around line 32-45: The function analyzeContractChangesWithAI directly reads
process.env and uses console/require and non-prefixed interfaces; replace direct
env access with getEnvVar() from script/demoScripts/utils/demoScriptHelpers.ts
when obtaining the API key inside analyzeContractChangesWithAI and update the
call to pass that validated key to analyzeWithClaudeSonnet; replace any
console.error usages with consola.error; change any require('child_process')
usage to an ES import (import { execSync } from 'child_process') at top of the
module to satisfy import/first; and rename interfaces AIAnalysisResult and
ContractDiff to IAIAnalysisResult and IContractDiff (updating all references,
function signatures, and return types) to comply with the ESLint interface
naming rule.
- Around line 176-181: In getFileDiff replace the dynamic
require('child_process') with an ESM import: import { execSync } from
'child_process' at the top-level and remove the require call inside the
function; ensure getFileDiff continues to call execSync with the same options
(encoding and maxBuffer) and that TypeScript compilation passes with the module
setting already using ESM imports.
In `@script/changelogAnalysis/generateContractChangelog.ts`:
- Around line 3-9: Add a colocated Bun test file named
generateContractChangelog.test.ts that achieves 100% coverage for the helper(s)
implemented in generateContractChangelog.ts (e.g., generateContractChangelog and
any helpers like analyzeContractChanges or formatChangelog) using
describe/it/expect; include tests for normal flow (valid Solidity changes ->
returns formatted changelog), edge cases (no changes, empty input, unusual
Solidity constructs), error paths (missing
CLAUDE_CODE_SC_CONTRACTS_REPO_CHANGELOGS_API_KEY, Anthropic API/network
failures, and invalid API responses), and assertions that errors are thrown or
handled as the module intends; mock the Anthropic/Claude calls and environment
variables to simulate success, failures, and timeouts so the tests
deterministically cover all branches.
- Around line 192-203: The variable combinedEntry in
generateContractChangelog.ts is declared with let but never reassigned; change
its declaration to const so the reference is immutable while allowing mutation
of nested properties (the object fields like changes.breaking/added/etc. remain
writable); locate the combinedEntry object literal initialization and replace
let with const (symbol: combinedEntry) to satisfy the linter/type rule.
- Around line 11-18: The import groupings violate the import/order rule: move a
blank line between the builtin imports (execSync from 'child_process',
existsSync/writeFileSync/mkdirSync from 'fs', join from 'path') and the internal
import (analyzeContractChangesWithAI, buildContractDiff, getFileDiff from
'./aiChangelogAnalyzer') so there is an empty line separating those groups;
ensure the imports for execSync, existsSync, writeFileSync, mkdirSync, and join
remain together and the './aiChangelogAnalyzer' import is on a new line below
them.
- Around line 174-186: Replace direct console logging and raw env access with
the repo-standard CLI/logging helpers: wire the script entry to citty (use its
command/runner to call mainWithAI) instead of relying on a bare shebang and
console, replace console.log/error calls inside mainWithAI with
consola.info/warn/error, and validate the REPOSITORY environment variable using
the shared getEnvVar() helper (or equivalent env validator) rather than reading
process.env.REPOSITORY directly; ensure imports for citty, consola, and
getEnvVar are added and that the CLI entry calls mainWithAI through the
citty-defined command so the script follows project conventions.
In `@script/changelogAnalysis/README.md`:
- Around line 9-14: Update the README.md fenced code block to include a language
tag so markdownlint MD040 is satisfied: replace the opening triple backticks
"```" for the directory tree with "```text" (i.e., change the block that shows
"script/changelogAnalysis/ ..." to start with ```text and end with ```), leaving
the inner content unchanged.
In `@src/Facets/WithdrawFacet.sol`:
- Around line 35-48: batchWithdraw is currently a no-op; implement input
validation and per-asset withdrawal and emit the batch event. Validate that
_assetAddresses.length == _amounts.length, that _to != address(0), and that
arrays are non-empty; then loop i over _assetAddresses and for each call the
same single-asset withdrawal logic used elsewhere (reuse the code/path in
withdraw or the helper that handles ERC20 vs native transfers and balance
checks), decrement/internal accounting as needed, and emit the existing
BatchWithdrawn (or create/emit a descriptive batch event) after successful
transfers; keep the existing access check (LibDiamond.contractOwner /
LibAccess.enforceAccessControl) in place. Ensure use of SafeERC20
transferFrom/transfer or the project’s token helper to avoid reentrancy/unsafe
transfers and validate amounts > 0 before attempting each transfer.
---
Duplicate comments:
In @.github/workflows/generateContractChangelog.yml:
- Around line 50-51: Replace the unpinned action reference "uses:
oven-sh/setup-bun@v1" with a full 40-character commit SHA and preferably update
to the v2 release; locate the line containing uses: oven-sh/setup-bun@v1 and
change it to uses: oven-sh/setup-bun@<full-commit-sha-for-v2> (or if you must
stay on v1, use the full commit SHA for v1.2.2), fetching the exact SHA via the
GitHub API/gh commands suggested in the review and ensure the workflow now
references the full 40-character SHA instead of a tag.
- Around line 95-118: Escape untrusted PR title and quote summary path: stop
interpolating `${{ github.event.pull_request.title }}` directly in the shell and
instead pass it into the job step as an environment variable (e.g., CH_PR_TITLE)
from the workflow expression, then reference `$CH_PR_TITLE` in the script; also
quote every use of the summary file path as `"$GITHUB_STEP_SUMMARY"` to avoid
word-splitting/globbing. While editing the Summary step, replace the direct
expression with the env var reference and ensure all `>> $GITHUB_STEP_SUMMARY`
occurrences become `>> "$GITHUB_STEP_SUMMARY"`; additionally, capture the list
of changed Solidity files earlier (e.g., in the "Generate changelog" step) and
expose it as a step output (like changed_sol_files) so the Summary step uses
that output instead of `git diff HEAD~1` which will only show the changelog
commit.
---
Nitpick comments:
In @.github/workflows/generateContractChangelog.yml:
- Around line 32-34: The workflow currently grants permissions without inline
comments; update the permissions block so each entry (contents and
pull-requests) includes a short inline comment explaining why that minimal
permission is needed (e.g., contents: write — to push generated changelog
commits; pull-requests: read — to read PR metadata for changelog generation).
Edit the permissions section around the entries "contents" and "pull-requests"
to add these concise comments and ensure no broader permissions are granted.
- Around line 36-40: Add a concurrency group to the generate-changelog job to
serialize runs and avoid overlapping git pushes: in the job block for
generate-changelog, add a concurrency stanza (e.g., concurrency: { group:
"generate-changelog-${{ github.ref }}", cancel-in-progress: false }) so runs
queue instead of racing when both attempt git push.
In `@src/Facets/WithdrawFacet.sol`:
- Around line 27-31: The BatchWithdrawCompleted event's totalAmount field is
ambiguous across multiple assets; change the event signature
BatchWithdrawCompleted to emit per-asset amounts (e.g., uint256[] amounts)
instead of a single totalAmount (or remove totalAmount entirely), then update
every emit of BatchWithdrawCompleted in the withdraw batch logic to supply the
per-asset amounts (computed alongside assets and amounts used in the batch) and
adjust any consumers/tests that read BatchWithdrawCompleted; also consider
relying on existing LogWithdraw events if you prefer not to add per-asset
arrays.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
.github/workflows/generateContractChangelog.ymlchangelog/README.mdpackage.jsonscript/changelogAnalysis/README.mdscript/changelogAnalysis/aiChangelogAnalyzer.tsscript/changelogAnalysis/generateContractChangelog.tsscript/helperFunctions.shsrc/Facets/WithdrawFacet.solsrc/LiFiDiamond.soltest/solidity/LiFiDiamond.t.sol
| - name: Checkout repository | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
actions/checkout must be pinned to a full 40-character commit SHA, not a tag.
Using @v4 is a mutable tag reference; if the upstream action repository is compromised, the tag can be silently redirected to malicious code.
🔒 Proposed fix
- uses: actions/checkout@v4
+ uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2As per coding guidelines: "Every uses: reference in GitHub Actions workflows MUST use a full 40-character commit SHA, never tags (e.g., @v4, @main) or short SHAs."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Checkout repository | |
| uses: actions/checkout@v4 | |
| - name: Checkout repository | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/generateContractChangelog.yml around lines 43 - 44, The
workflow uses a mutable tag reference "uses: actions/checkout@v4"; replace that
with a pinned full 40-character commit SHA for the actions/checkout action to
satisfy the policy. Locate the "uses: actions/checkout@v4" line in the workflow
and update it to "uses: actions/checkout@<FULL_40_CHAR_SHA>" (obtain the SHA
from the actions/checkout repository commit you intend to pin), commit the
change, and ensure no other "uses:" entries in the file use tags or short SHAs.
| - name: Check for changes | ||
| id: check | ||
| run: | | ||
| # Check if any new changelog files were created | ||
| if git diff --name-only --diff-filter=A | grep -q "^changelog/.*\.md$"; then | ||
| echo "has_changes=true" >> "$GITHUB_OUTPUT" | ||
| echo "New changelog file(s) created" | ||
| git diff --name-only --diff-filter=A | grep "^changelog/.*\.md$" | while read file; do | ||
| echo " - $file" | ||
| done | ||
| else | ||
| echo "has_changes=false" >> "$GITHUB_OUTPUT" | ||
| echo "No new changelog files detected" | ||
| fi |
There was a problem hiding this comment.
git diff --diff-filter=A does not detect untracked new files — changelog commits will never occur.
git diff --name-only --diff-filter=A (without a commit reference) compares the index to the working tree. Newly created files written by the TypeScript script that haven't been staged via git add are untracked and are invisible to git diff. As a result, has_changes will always resolve to false and the "Commit and push changelog" step will never execute.
Use git ls-files --others --exclude-standard to enumerate untracked files instead:
🐛 Proposed fix
- name: Check for changes
id: check
run: |
# Check if any new changelog files were created
- if git diff --name-only --diff-filter=A | grep -q "^changelog/.*\.md$"; then
+ if git ls-files --others --exclude-standard | grep -q "^changelog/.*\.md$"; then
echo "has_changes=true" >> "$GITHUB_OUTPUT"
echo "New changelog file(s) created"
- git diff --name-only --diff-filter=A | grep "^changelog/.*\.md$" | while read file; do
+ git ls-files --others --exclude-standard | grep "^changelog/.*\.md$" | while IFS= read -r file; do
echo " - $file"
done
else🧰 Tools
🪛 actionlint (1.7.11)
[error] 71-71: shellcheck reported issue in this script: SC2162:info:5:76: read without -r will mangle backslashes
(shellcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/generateContractChangelog.yml around lines 69 - 82, The
current "Check for changes" step (id: check) uses git diff --name-only
--diff-filter=A which misses untracked files created but not staged; update the
step to detect both staged/committed additions and untracked files by using git
diff --name-only --diff-filter=A HEAD (or similar against HEAD) for added paths
and git ls-files --others --exclude-standard to list untracked files, merge
those results into a single list, set GITHUB_OUTPUT has_changes=true when the
combined list is non-empty, and print each matched path (so the later "Commit
and push changelog" step will run).
changelog/README.md
Outdated
| The changelog generation uses AI-powered analysis (CodeRabbit API) to provide semantic descriptions of contract changes. | ||
|
|
||
| ## Related Files | ||
|
|
||
| - Workflow: `.github/workflows/generate-contract-changelog.yml` | ||
| - Script: `script/changelogAnalysis/generateContractChangelog.ts` |
There was a problem hiding this comment.
Docs: align AI provider and workflow filename with implementation.
The docs mention “CodeRabbit API” and reference generate-contract-changelog.yml, but the implementation uses Claude Sonnet (Anthropic) and the workflow file is camel-cased elsewhere. Please align the text and filename.
📝 Suggested fix
-The changelog generation uses AI-powered analysis (CodeRabbit API) to provide semantic descriptions of contract changes.
+The changelog generation uses AI-powered analysis (Anthropic Claude Sonnet API) to provide semantic descriptions of contract changes.
-Workflow: `.github/workflows/generate-contract-changelog.yml`
+Workflow: `.github/workflows/generateContractChangelog.yml`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The changelog generation uses AI-powered analysis (CodeRabbit API) to provide semantic descriptions of contract changes. | |
| ## Related Files | |
| - Workflow: `.github/workflows/generate-contract-changelog.yml` | |
| - Script: `script/changelogAnalysis/generateContractChangelog.ts` | |
| The changelog generation uses AI-powered analysis (Anthropic Claude Sonnet API) to provide semantic descriptions of contract changes. | |
| ## Related Files | |
| - Workflow: `.github/workflows/generateContractChangelog.yml` | |
| - Script: `script/changelogAnalysis/generateContractChangelog.ts` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@changelog/README.md` around lines 31 - 36, Update the changelog README to
match the implementation: replace the “CodeRabbit API” mention with “Claude
Sonnet (Anthropic)” and change the workflow filename reference from
`generate-contract-changelog.yml` to the camel-cased
`generateContractChangelog.yml`; ensure the Related Files list still references
the workflow `generateContractChangelog.yml` and the script
`script/changelogAnalysis/generateContractChangelog.ts` (or
`generateContractChangelog.ts` if that’s the actual script name) so the doc text
and filenames align with the code.
| export async function analyzeContractChangesWithAI( | ||
| diff: ContractDiff, | ||
| apiKey?: string | ||
| ): Promise<AIAnalysisResult> { | ||
| const key = | ||
| apiKey ?? process.env.CLAUDE_CODE_SC_CONTRACTS_REPO_CHANGELOGS_API_KEY | ||
|
|
||
| if (!key) { | ||
| throw new Error( | ||
| 'No API key provided. Set CLAUDE_CODE_SC_CONTRACTS_REPO_CHANGELOGS_API_KEY environment variable' | ||
| ) | ||
| } | ||
|
|
||
| return analyzeWithClaudeSonnet(diff, key) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual file structure and content
fd -t f "aiChangelogAnalyzer" --exec wc -l {} \;Repository: lifinance/contracts
Length of output: 116
🏁 Script executed:
# Read the file to see the actual state
cat -n script/changelogAnalysis/aiChangelogAnalyzer.ts | head -100Repository: lifinance/contracts
Length of output: 3360
🏁 Script executed:
# Check if getEnvVar helper exists and where it's defined
rg "export.*getEnvVar" --type ts -A 3Repository: lifinance/contracts
Length of output: 426
🏁 Script executed:
# Check for consola usage patterns in script files
rg "import.*consola" script/ --type ts | head -5Repository: lifinance/contracts
Length of output: 401
🏁 Script executed:
# Read the rest of the file to see console.error and require() issues
cat -n script/changelogAnalysis/aiChangelogAnalyzer.ts | tail -106Repository: lifinance/contracts
Length of output: 4241
🏁 Script executed:
# Check the imports at the top of the file
head -30 script/changelogAnalysis/aiChangelogAnalyzer.tsRepository: lifinance/contracts
Length of output: 714
🏁 Script executed:
# Check the ESLint configuration to confirm rules
cat .eslintrc.cjs | grep -A 5 -B 5 "interface-name\|I-prefix" || echo "No direct match found; checking for naming conventions"Repository: lifinance/contracts
Length of output: 117
🏁 Script executed:
# Let's look at the .eslintrc.cjs more broadly
head -100 .eslintrc.cjsRepository: lifinance/contracts
Length of output: 3878
🏁 Script executed:
# Continue reading the naming convention rules
sed -n '80,150p' .eslintrc.cjsRepository: lifinance/contracts
Length of output: 1222
Use shared helpers for env validation, logging, and module imports; fix interface naming to match ESLint requirements.
This module has multiple violations of script guidelines:
-
Environment validation (line 37): Use
getEnvVar()helper fromscript/demoScripts/utils/demoScriptHelpers.tsinstead of directprocess.envaccess. -
Error logging (lines 167, 168, 184): Replace
console.error()withconsola.error()for consistent logging across the codebase. -
Module imports (line 177): Use ES module import syntax (
import { execSync } from 'child_process') instead ofrequire(), which violates ESLint'simport/firstrule. -
Interface naming (lines 8, 18): Interfaces
AIAnalysisResultandContractDiffmust have theIprefix per ESLint config (e.g.,IAIAnalysisResult,IContractDiff). This is a hard requirement enforced by.eslintrc.cjs.
Per guidelines: "script/**/*.ts: TS scripts MUST use .eslintrc.cjs rules, bunx tsx, citty, consola, and env validated via helpers."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@script/changelogAnalysis/aiChangelogAnalyzer.ts` around lines 32 - 45, The
function analyzeContractChangesWithAI directly reads process.env and uses
console/require and non-prefixed interfaces; replace direct env access with
getEnvVar() from script/demoScripts/utils/demoScriptHelpers.ts when obtaining
the API key inside analyzeContractChangesWithAI and update the call to pass that
validated key to analyzeWithClaudeSonnet; replace any console.error usages with
consola.error; change any require('child_process') usage to an ES import (import
{ execSync } from 'child_process') at top of the module to satisfy import/first;
and rename interfaces AIAnalysisResult and ContractDiff to IAIAnalysisResult and
IContractDiff (updating all references, function signatures, and return types)
to comply with the ESLint interface naming rule.
| export function getFileDiff(file: string, oldCommit: string, newCommit: string): string { | ||
| const { execSync } = require('child_process') | ||
| try { | ||
| return execSync(`git diff ${oldCommit} ${newCommit} -- ${file}`, { | ||
| encoding: 'utf-8', | ||
| maxBuffer: 10 * 1024 * 1024, // 10MB buffer for large diffs |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file and the specific lines mentioned
cat -n script/changelogAnalysis/aiChangelogAnalyzer.ts | sed -n '170,185p'Repository: lifinance/contracts
Length of output: 646
🏁 Script executed:
# Check the .eslintrc.cjs to see if no-var-requires is enabled
cat .eslintrc.cjs | grep -A 2 "no-var-requires"Repository: lifinance/contracts
Length of output: 45
🏁 Script executed:
# Search for how child_process is imported elsewhere in the codebase
rg "from 'child_process'" --type ts -A 1Repository: lifinance/contracts
Length of output: 847
🏁 Script executed:
# Also check if there are any other require statements for child_process
rg "require\(['\"]*child_process" --type tsRepository: lifinance/contracts
Length of output: 158
🏁 Script executed:
# Let's check the actual .eslintrc.cjs file
cat .eslintrc.cjs | head -100Repository: lifinance/contracts
Length of output: 3878
🏁 Script executed:
# Also check if there are any other require() patterns in script files
rg "require\(" script/ --type ts | head -20Repository: lifinance/contracts
Length of output: 158
🏁 Script executed:
# Let's search for `@typescript-eslint/no-var-requires` in the eslintrc config more thoroughly
cat .eslintrc.cjs | grep -i "no-var-requires"Repository: lifinance/contracts
Length of output: 45
🏁 Script executed:
# Let's also verify the full extends config to understand what rules are inherited
cat .eslintrc.cjs | grep -A 10 "extends:"Repository: lifinance/contracts
Length of output: 413
🏁 Script executed:
# Let's get the full rules section to see if no-var-requires is mentioned
cat .eslintrc.cjs | grep -E "(no-var-requires|overrides)" -A 5 -B 5Repository: lifinance/contracts
Length of output: 45
🏁 Script executed:
# Let's also check the tsconfig.eslint.json to understand the project configuration
cat tsconfig.eslint.jsonRepository: lifinance/contracts
Length of output: 301
🏁 Script executed:
# Let's try running eslint on the specific file to see what violations are reported
bunx eslint script/changelogAnalysis/aiChangelogAnalyzer.ts --format json 2>&1 | head -200Repository: lifinance/contracts
Length of output: 105
🏁 Script executed:
# Let's check what rules are in the `@typescript-eslint/recommended` preset
# We can look at the node_modules to see what the preset enables, or search for documentation
cat .eslintrc.cjs | tail -100Repository: lifinance/contracts
Length of output: 3376
🏁 Script executed:
# Let's search the entire eslintrc file to see all rules
wc -l .eslintrc.cjsRepository: lifinance/contracts
Length of output: 80
🏁 Script executed:
# Get the entire file to see all rules
cat .eslintrc.cjsRepository: lifinance/contracts
Length of output: 4344
🏁 Script executed:
# Let's check if `@typescript-eslint/no-var-requires` is in the recommended preset
# by searching for any documentation or seeing if it would be applied
# First, let's look at the complete imports section of aiChangelogAnalyzer.ts
cat -n script/changelogAnalysis/aiChangelogAnalyzer.ts | head -20Repository: lifinance/contracts
Length of output: 600
🏁 Script executed:
# Let's check if there are other uses of require() in script files matching the pattern
rg "const.*=.*require\(" script/ --type tsRepository: lifinance/contracts
Length of output: 158
Replace require with an ESM import for child_process.
The require() statement violates @typescript-eslint/no-var-requires from the inherited @typescript-eslint/recommended preset. All other files in the codebase use ESM imports for child_process.
🔧 Proposed fix
+import { execSync } from 'child_process'-export function getFileDiff(file: string, oldCommit: string, newCommit: string): string {
- const { execSync } = require('child_process')
+export function getFileDiff(file: string, oldCommit: string, newCommit: string): string {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function getFileDiff(file: string, oldCommit: string, newCommit: string): string { | |
| const { execSync } = require('child_process') | |
| try { | |
| return execSync(`git diff ${oldCommit} ${newCommit} -- ${file}`, { | |
| encoding: 'utf-8', | |
| maxBuffer: 10 * 1024 * 1024, // 10MB buffer for large diffs | |
| import { execSync } from 'child_process' | |
| export function getFileDiff(file: string, oldCommit: string, newCommit: string): string { | |
| try { | |
| return execSync(`git diff ${oldCommit} ${newCommit} -- ${file}`, { | |
| encoding: 'utf-8', | |
| maxBuffer: 10 * 1024 * 1024, // 10MB buffer for large diffs |
🧰 Tools
🪛 ESLint
[error] 177-177: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@script/changelogAnalysis/aiChangelogAnalyzer.ts` around lines 176 - 181, In
getFileDiff replace the dynamic require('child_process') with an ESM import:
import { execSync } from 'child_process' at the top-level and remove the require
call inside the function; ensure getFileDiff continues to call execSync with the
same options (encoding and maxBuffer) and that TypeScript compilation passes
with the module setting already using ESM imports.
| import { execSync } from 'child_process' | ||
| import { existsSync, writeFileSync, mkdirSync } from 'fs' | ||
| import { join } from 'path' | ||
| import { | ||
| analyzeContractChangesWithAI, | ||
| buildContractDiff, | ||
| getFileDiff, | ||
| } from './aiChangelogAnalyzer' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n script/changelogAnalysis/generateContractChangelog.ts | head -25Repository: lifinance/contracts
Length of output: 907
🏁 Script executed:
cat -n .eslintrc.cjs | head -100Repository: lifinance/contracts
Length of output: 4578
Fix import grouping to satisfy import/order ESLint rule.
The builtin imports (child_process, fs, path) must be separated from the internal import (./aiChangelogAnalyzer) by a blank line. The ESLint rule import/order with newlines-between: always requires blank lines between import groups.
🔧 Proposed fix
import { execSync } from 'child_process'
import { existsSync, writeFileSync, mkdirSync } from 'fs'
import { join } from 'path'
+
import {
analyzeContractChangesWithAI,
buildContractDiff,
getFileDiff,
} from './aiChangelogAnalyzer'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { execSync } from 'child_process' | |
| import { existsSync, writeFileSync, mkdirSync } from 'fs' | |
| import { join } from 'path' | |
| import { | |
| analyzeContractChangesWithAI, | |
| buildContractDiff, | |
| getFileDiff, | |
| } from './aiChangelogAnalyzer' | |
| import { execSync } from 'child_process' | |
| import { existsSync, writeFileSync, mkdirSync } from 'fs' | |
| import { join } from 'path' | |
| import { | |
| analyzeContractChangesWithAI, | |
| buildContractDiff, | |
| getFileDiff, | |
| } from './aiChangelogAnalyzer' |
🧰 Tools
🪛 ESLint
[error] 13-13: There should be at least one empty line between import groups
(import/order)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@script/changelogAnalysis/generateContractChangelog.ts` around lines 11 - 18,
The import groupings violate the import/order rule: move a blank line between
the builtin imports (execSync from 'child_process',
existsSync/writeFileSync/mkdirSync from 'fs', join from 'path') and the internal
import (analyzeContractChangesWithAI, buildContractDiff, getFileDiff from
'./aiChangelogAnalyzer') so there is an empty line separating those groups;
ensure the imports for execSync, existsSync, writeFileSync, mkdirSync, and join
remain together and the './aiChangelogAnalyzer' import is on a new line below
them.
| async function mainWithAI() { | ||
| console.log('🤖 AI-powered analysis (Claude Sonnet)\n') | ||
|
|
||
| const changedFiles = getChangedSolidityFiles() | ||
|
|
||
| if (changedFiles.length === 0) { | ||
| console.log('ℹ️ No Solidity files changed') | ||
| return | ||
| } | ||
|
|
||
| console.log(`Found ${changedFiles.length} changed contract(s):\n`) | ||
| changedFiles.forEach(file => console.log(` - ${file}`)) | ||
| console.log() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the full file structure
cat -n script/changelogAnalysis/generateContractChangelog.tsRepository: lifinance/contracts
Length of output: 8961
🏁 Script executed:
# List files in the changelogAnalysis directory to understand context
ls -la script/changelogAnalysis/Repository: lifinance/contracts
Length of output: 388
🏁 Script executed:
# Search for similar scripts that might show patterns in this directory
find script/changelogAnalysis -name "*.ts" -type fRepository: lifinance/contracts
Length of output: 164
Refactor to use citty for CLI and consola for logging.
This script is a standalone CLI entry point (executable shebang) but uses console.log() and console.error() directly instead of following repo conventions. Per coding guidelines, all script/**/*.ts files must use citty for CLI wiring, consola for logging, and environment variable validation via helpers (e.g., getEnvVar()). Additionally, the REPOSITORY environment variable on line 149 should be validated through a proper helper rather than accessed directly from process.env.
🧰 Tools
🪛 Biome (2.4.4)
[error] 185-185: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@script/changelogAnalysis/generateContractChangelog.ts` around lines 174 -
186, Replace direct console logging and raw env access with the repo-standard
CLI/logging helpers: wire the script entry to citty (use its command/runner to
call mainWithAI) instead of relying on a bare shebang and console, replace
console.log/error calls inside mainWithAI with consola.info/warn/error, and
validate the REPOSITORY environment variable using the shared getEnvVar() helper
(or equivalent env validator) rather than reading process.env.REPOSITORY
directly; ensure imports for citty, consola, and getEnvVar are added and that
the CLI entry calls mainWithAI through the citty-defined command so the script
follows project conventions.
| let combinedEntry: ChangelogEntry = { | ||
| date, | ||
| commitSha, | ||
| commitMessage, | ||
| changes: { | ||
| breaking: [], | ||
| added: [], | ||
| changed: [], | ||
| removed: [], | ||
| fixed: [], | ||
| }, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the file exists and get its size
wc -l script/changelogAnalysis/generateContractChangelog.ts
# Read the relevant context around lines 192-203
echo "=== Context around lines 192-203 ==="
sed -n '180,250p' script/changelogAnalysis/generateContractChangelog.ts | cat -nRepository: lifinance/contracts
Length of output: 2872
🏁 Script executed:
#!/bin/bash
# Verify if combinedEntry is reassigned anywhere in the function
# by searching for reassignment patterns
echo "=== Searching for reassignments to combinedEntry ==="
rg 'combinedEntry\s*=' script/changelogAnalysis/generateContractChangelog.ts -A 2Repository: lifinance/contracts
Length of output: 115
Use const for combinedEntry since the variable is never reassigned.
Only the nested properties of the object are mutated, not the variable itself. Properties can be mutated on const objects in TypeScript.
🔧 Proposed fix
- let combinedEntry: ChangelogEntry = {
+ const combinedEntry: ChangelogEntry = {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let combinedEntry: ChangelogEntry = { | |
| date, | |
| commitSha, | |
| commitMessage, | |
| changes: { | |
| breaking: [], | |
| added: [], | |
| changed: [], | |
| removed: [], | |
| fixed: [], | |
| }, | |
| } | |
| const combinedEntry: ChangelogEntry = { | |
| date, | |
| commitSha, | |
| commitMessage, | |
| changes: { | |
| breaking: [], | |
| added: [], | |
| changed: [], | |
| removed: [], | |
| fixed: [], | |
| }, | |
| } |
🧰 Tools
🪛 ESLint
[error] 192-192: 'combinedEntry' is never reassigned. Use 'const' instead.
(prefer-const)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@script/changelogAnalysis/generateContractChangelog.ts` around lines 192 -
203, The variable combinedEntry in generateContractChangelog.ts is declared with
let but never reassigned; change its declaration to const so the reference is
immutable while allowing mutation of nested properties (the object fields like
changes.breaking/added/etc. remain writable); locate the combinedEntry object
literal initialization and replace let with const (symbol: combinedEntry) to
satisfy the linter/type rule.
| ``` | ||
| script/changelogAnalysis/ | ||
| ├── generateContractChangelog.ts # Entrypoint: git diff → AI analysis → write changelog | ||
| ├── aiChangelogAnalyzer.ts # Claude Sonnet API + diff building + response parsing | ||
| └── README.md # This file | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced block.
Markdownlint (MD040) expects a language on fenced blocks.
💡 Suggested fix
-```
+```text
script/changelogAnalysis/
├── generateContractChangelog.ts # Entrypoint: git diff → AI analysis → write changelog
├── aiChangelogAnalyzer.ts # Claude Sonnet API + diff building + response parsing
└── README.md # This file
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| script/changelogAnalysis/ | |
| ├── generateContractChangelog.ts # Entrypoint: git diff → AI analysis → write changelog | |
| ├── aiChangelogAnalyzer.ts # Claude Sonnet API + diff building + response parsing | |
| └── README.md # This file | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@script/changelogAnalysis/README.md` around lines 9 - 14, Update the README.md
fenced code block to include a language tag so markdownlint MD040 is satisfied:
replace the opening triple backticks "```" for the directory tree with "```text"
(i.e., change the block that shows "script/changelogAnalysis/ ..." to start with
```text and end with ```), leaving the inner content unchanged.
| /// @notice Batch withdraw multiple assets | ||
| /// @param _assetAddresses Array of asset addresses to withdraw | ||
| /// @param _to Address to withdraw to | ||
| /// @param _amounts Array of amounts to withdraw | ||
| function batchWithdraw( | ||
| address[] calldata _assetAddresses, | ||
| address _to, | ||
| uint256[] calldata _amounts | ||
| ) external { | ||
| if (msg.sender != LibDiamond.contractOwner()) { | ||
| LibAccess.enforceAccessControl(); | ||
| } | ||
| // Implementation would go here | ||
| } |
There was a problem hiding this comment.
Implement batchWithdraw (currently a no-op).
The function succeeds without withdrawing or emitting the batch event, which is a functional defect. At minimum, validate array lengths and execute withdrawals. As per coding guidelines: {src,script,test}/**/*.{sol,ts}: Validate all external inputs and configuration (including script/env values) explicitly; prefer existing validation helpers (e.g., Validatable, config readers) over ad-hoc checks.
🔧 Proposed implementation (incl. length validation)
contract WithdrawFacet {
/// Errors ///
error WithdrawFailed();
+ error InvalidArrayLengths();
@@
function batchWithdraw(
address[] calldata _assetAddresses,
address _to,
uint256[] calldata _amounts
) external {
if (msg.sender != LibDiamond.contractOwner()) {
LibAccess.enforceAccessControl();
}
- // Implementation would go here
+ if (_assetAddresses.length == 0 || _assetAddresses.length != _amounts.length) {
+ revert InvalidArrayLengths();
+ }
+
+ uint256 totalAmount;
+ for (uint256 i; i < _assetAddresses.length; ++i) {
+ _withdrawAsset(_assetAddresses[i], _to, _amounts[i]);
+ totalAmount += _amounts[i];
+ }
+
+ emit BatchWithdrawCompleted(_assetAddresses, _to, totalAmount);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Facets/WithdrawFacet.sol` around lines 35 - 48, batchWithdraw is
currently a no-op; implement input validation and per-asset withdrawal and emit
the batch event. Validate that _assetAddresses.length == _amounts.length, that
_to != address(0), and that arrays are non-empty; then loop i over
_assetAddresses and for each call the same single-asset withdrawal logic used
elsewhere (reuse the code/path in withdraw or the helper that handles ERC20 vs
native transfers and balance checks), decrement/internal accounting as needed,
and emit the existing BatchWithdrawn (or create/emit a descriptive batch event)
after successful transfers; keep the existing access check
(LibDiamond.contractOwner / LibAccess.enforceAccessControl) in place. Ensure use
of SafeERC20 transferFrom/transfer or the project’s token helper to avoid
reentrancy/unsafe transfers and validate amounts > 0 before attempting each
transfer.
| "typechain:incremental": "forge build src && bun abi:removeDuplicateEvents && typechain --target ethers-v5 'out/!(*.t).sol/*.json' --out-dir typechain", | ||
| "unpause-all-diamonds": "bunx tsx ./script/tasks/unpauseAllDiamonds.ts", | ||
| "balances": "bunx tsx ./script/balances.ts", | ||
| "changelog:contracts": "bun run script/changelogAnalysis/generateContractChangelog.ts", |
There was a problem hiding this comment.
it want be used by us manually so not need to have new script here
There was a problem hiding this comment.
Was more like a backup but no problem to remove it
| @@ -0,0 +1,116 @@ | |||
| # Generate Contract Changelog | |||
There was a problem hiding this comment.
It generates a new file per commit (changelog/{commitHash}.md). it doesn't create a traditional, readable CHANGELOG.md that customers or devs can scroll through easily or seperate changelog md file for each contract version
There was a problem hiding this comment.
New version includes a general CHANGELOG.md and then specific per contract files, testing right now
|
|
||
| on: | ||
| pull_request: | ||
| types: [closed] |
There was a problem hiding this comment.
generating the changelog only on merge prevents changelog review
There was a problem hiding this comment.
Set up like this until final decision on the Slack being made
| @@ -0,0 +1,116 @@ | |||
| # Generate Contract Changelog | |||
| # - Triggers when a PR is merged to main that modified Solidity contracts | |||
| # - Also triggers on push to test branch (SMAR-41-Research-how-we-can-auto-generate-contract-changelogs-using-AI) | |||
There was a problem hiding this comment.
Set up like this in order to test and validate it on this branch
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
script/changelogAnalysis/generateContractChangelog.ts (4)
265-277: Script entry point must usecitty,consola, andgetEnvVar()per project conventions.
console.log/console.errorand bareprocess.env.REPOSITORYaccess (also repeated inupdateContractChangelog, line 228) remain unfixed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/changelogAnalysis/generateContractChangelog.ts` around lines 265 - 277, Replace direct console logging and raw process.env access in the script entry point and related functions: in mainWithAI (and anywhere else like updateContractChangelog) stop using console.log/console.error and instead use the project's logging utilities (citty for styled output and consola for structured logging) and replace direct process.env.REPOSITORY with the getEnvVar('REPOSITORY') helper; update all occurrences of getChangedSolidityFiles logging and the repository reads to call citty/consola and getEnvVar respectively so the script follows project conventions and centralizes env access.
284-284:combinedEntryis never reassigned — useconst(ESLintprefer-const).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/changelogAnalysis/generateContractChangelog.ts` at line 284, The variable combinedEntry is declared with let but never reassigned; change its declaration to const to satisfy ESLint prefer-const and ensure immutability for the ChangelogEntry object (update the declaration of combinedEntry to use const and verify there are no subsequent reassignments or mutations that would require a let).
3-9: Missing colocated Bun test file.
11-18: Missing blank line between builtin and internal import groups (ESLintimport/order).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/changelogAnalysis/generateContractChangelog.ts` around lines 11 - 18, ESLint import/order wants a blank line between built-in/module imports and internal imports; in generateContractChangelog.ts insert a single empty line between the grouped built-in imports (execSync from 'child_process' and existsSync/mkdirSync/readFileSync/writeFileSync from 'fs' and join from 'path') and the internal import statement that pulls analyzeContractChangesWithAI, buildContractDiff, getFileDiff from './aiChangelogAnalyzer' so the builtins are separated from the project/internal import..github/workflows/generateContractChangelog.yml (4)
48-49:oven-sh/setup-bun@v1must be pinned to a full 40-character commit SHA.Additionally,
bun-version: latestshould be pinned to a specific version for reproducible CI runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/generateContractChangelog.yml around lines 48 - 49, Replace the loose action reference and floating version: change the uses line "oven-sh/setup-bun@v1" to the full 40-character commit SHA for that action, and replace "bun-version: latest" with a specific Bun version string (e.g., "bun-version: 1.x.y") so the workflow uses a pinned action commit and a fixed Bun release for reproducible CI runs; update the uses reference and bun-version key where they appear in the job configuration.
105-106: Template injection viagithub.event.pull_request.title— pass through env var instead.
pr.titleis user-controlled and can contain shell metacharacters. Pass it via anenv:block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/generateContractChangelog.yml around lines 105 - 106, The workflow currently interpolates user-controlled github.event.pull_request.title directly into a shell command, risking template injection; instead add an env variable (e.g., PR_TITLE) on the step that writes to GITHUB_STEP_SUMMARY and assign it from ${{ github.event.pull_request.title }}, then reference that env var in the echo command (use quoted "$PR_TITLE") so the PR title is passed as data rather than expanded by the shell; update the step that writes to GITHUB_STEP_SUMMARY to use PR_TITLE and ensure the env: block contains PR_TITLE: ${{ github.event.pull_request.title }}.
41-42:actions/checkout@v4must be pinned to a full 40-character commit SHA.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/generateContractChangelog.yml around lines 41 - 42, The workflow currently references actions/checkout@v4 which must be pinned to a specific 40-character commit SHA; update the "Checkout repository" step to replace uses: actions/checkout@v4 with uses: actions/checkout@<full-40-character-commit-sha> (the exact commit SHA from the actions/checkout repository), ensuring the SHA is the full 40 characters and not a tag or shortened hash, so the workflow is fully pinned and immutable.
67-76:git diff --name-onlystill misses newly-created (untracked) changelog files.Removing
--diff-filter=Adidn't resolve the core problem: brand-new files written by the TypeScript script are untracked and invisible togit diff.has_changeswill befalseon first run, so the commit step never executes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/generateContractChangelog.yml around lines 67 - 76, The "Check for changes" step (id: check) currently uses git diff --name-only which ignores untracked files, so the has_changes output and subsequent commit never run for newly-created changelog files; update the check to detect both tracked diffs and untracked files (e.g., use git status --porcelain or git ls-files --others --exclude-standard) and set has_changes=true when either a tracked change under changelog/ or an untracked file under changelog/ is present, ensuring the existing has_changes variable and the changelog/ path are used in the test.script/changelogAnalysis/README.md (1)
12-17: Fenced code block is missing a language identifier (markdownlint MD040).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/changelogAnalysis/README.md` around lines 12 - 17, The fenced code block in README.md (the directory tree showing generateContractChangelog.ts and aiChangelogAnalyzer.ts) lacks a language identifier; update the triple-backtick fence to include a language (e.g., ```text or ```bash) so markdownlint MD040 is satisfied and the block is properly identified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/generateContractChangelog.yml:
- Around line 34-38: Add a concurrency group to the generate-changelog job to
prevent simultaneous runs from pushing conflicting changelog commits: inside the
job definition for generate-changelog add a concurrency: block (e.g., using
group: github.ref or a combination like github.workflow + github.ref and
cancel-in-progress: true) so GitHub Actions serializes runs for the same
branch/PR and cancels in-progress duplicates.
- Around line 57-65: The workflow sets COMMIT_SHA but the script
generateContractChangelog.ts currently ignores it (it always uses execSync('git
rev-parse HEAD')); either remove the workflow's COMMIT_SHA input/env if you
won't support historical re-analysis, or modify generateContractChangelog.ts to
read process.env.COMMIT_SHA and, when present, use it (e.g., git checkout
${COMMIT_SHA} before analysis or pass it into git diff/show commands) instead of
calling execSync('git rev-parse HEAD') so the workflow_dispatch commit_sha
actually controls which commit is analyzed.
- Around line 97-108: Update the two git-diff invocations so they compare
against the commit before the changelog commit instead of the working tree or
the merge commit: replace the first `git diff --name-only | grep "^changelog/"`
with `git diff --name-only HEAD~1 | grep "^changelog/"` to list files that were
just committed to changelog, and replace the second `git diff HEAD~1 --name-only
--diff-filter=AM src/**/*.sol` with `git diff HEAD~2 --name-only
--diff-filter=AM src/**/*.sol` so Solidity changes from the feature/merge are
shown (keep the existing `sed` and fallback `|| echo "- (see commit for
details)"` behavior).
In `@script/changelogAnalysis/generateContractChangelog.ts`:
- Line 276: The callback passed to changedFiles.forEach returns the undefined
value from console.log which triggers Biome's useIterableCallbackReturn rule;
replace the forEach usage by iterating changedFiles with a for...of loop
(iterate over the iterable changedFiles and call console.log for each file) so
no callback is passed—update the code that currently reads
changedFiles.forEach(file => console.log(` - ${file}`)) to use a for...of over
changedFiles instead.
- Around line 31-42: Rename the interface ChangelogEntry to IChangelogEntry
throughout the file and any files that import or reference it; update the
interface declaration name and then change all type annotations/usages such as
formatChangelogEntry(entry: ChangelogEntry) -> formatChangelogEntry(entry:
IChangelogEntry), variable declarations like let combinedEntry: ChangelogEntry
-> let combinedEntry: IChangelogEntry, function parameters/returns and any
generics or array types (e.g., ChangelogEntry[] -> IChangelogEntry[]); ensure
any exports/imports are adjusted to the new name to satisfy the ESLint
interface-prefix rule.
---
Duplicate comments:
In @.github/workflows/generateContractChangelog.yml:
- Around line 48-49: Replace the loose action reference and floating version:
change the uses line "oven-sh/setup-bun@v1" to the full 40-character commit SHA
for that action, and replace "bun-version: latest" with a specific Bun version
string (e.g., "bun-version: 1.x.y") so the workflow uses a pinned action commit
and a fixed Bun release for reproducible CI runs; update the uses reference and
bun-version key where they appear in the job configuration.
- Around line 105-106: The workflow currently interpolates user-controlled
github.event.pull_request.title directly into a shell command, risking template
injection; instead add an env variable (e.g., PR_TITLE) on the step that writes
to GITHUB_STEP_SUMMARY and assign it from ${{ github.event.pull_request.title
}}, then reference that env var in the echo command (use quoted "$PR_TITLE") so
the PR title is passed as data rather than expanded by the shell; update the
step that writes to GITHUB_STEP_SUMMARY to use PR_TITLE and ensure the env:
block contains PR_TITLE: ${{ github.event.pull_request.title }}.
- Around line 41-42: The workflow currently references actions/checkout@v4 which
must be pinned to a specific 40-character commit SHA; update the "Checkout
repository" step to replace uses: actions/checkout@v4 with uses:
actions/checkout@<full-40-character-commit-sha> (the exact commit SHA from the
actions/checkout repository), ensuring the SHA is the full 40 characters and not
a tag or shortened hash, so the workflow is fully pinned and immutable.
- Around line 67-76: The "Check for changes" step (id: check) currently uses git
diff --name-only which ignores untracked files, so the has_changes output and
subsequent commit never run for newly-created changelog files; update the check
to detect both tracked diffs and untracked files (e.g., use git status
--porcelain or git ls-files --others --exclude-standard) and set
has_changes=true when either a tracked change under changelog/ or an untracked
file under changelog/ is present, ensuring the existing has_changes variable and
the changelog/ path are used in the test.
In `@script/changelogAnalysis/generateContractChangelog.ts`:
- Around line 265-277: Replace direct console logging and raw process.env access
in the script entry point and related functions: in mainWithAI (and anywhere
else like updateContractChangelog) stop using console.log/console.error and
instead use the project's logging utilities (citty for styled output and consola
for structured logging) and replace direct process.env.REPOSITORY with the
getEnvVar('REPOSITORY') helper; update all occurrences of
getChangedSolidityFiles logging and the repository reads to call citty/consola
and getEnvVar respectively so the script follows project conventions and
centralizes env access.
- Line 284: The variable combinedEntry is declared with let but never
reassigned; change its declaration to const to satisfy ESLint prefer-const and
ensure immutability for the ChangelogEntry object (update the declaration of
combinedEntry to use const and verify there are no subsequent reassignments or
mutations that would require a let).
- Around line 11-18: ESLint import/order wants a blank line between
built-in/module imports and internal imports; in generateContractChangelog.ts
insert a single empty line between the grouped built-in imports (execSync from
'child_process' and existsSync/mkdirSync/readFileSync/writeFileSync from 'fs'
and join from 'path') and the internal import statement that pulls
analyzeContractChangesWithAI, buildContractDiff, getFileDiff from
'./aiChangelogAnalyzer' so the builtins are separated from the project/internal
import.
In `@script/changelogAnalysis/README.md`:
- Around line 12-17: The fenced code block in README.md (the directory tree
showing generateContractChangelog.ts and aiChangelogAnalyzer.ts) lacks a
language identifier; update the triple-backtick fence to include a language
(e.g., ```text or ```bash) so markdownlint MD040 is satisfied and the block is
properly identified.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/generateContractChangelog.ymlscript/changelogAnalysis/README.mdscript/changelogAnalysis/generateContractChangelog.tssrc/Facets/UnitFacet.sol
| jobs: | ||
| generate-changelog: | ||
| # Only run if: (1) PR was merged (not just closed), OR (2) manually triggered, OR (3) push to test branch | ||
| if: github.event.pull_request.merged == true || github.event_name == 'workflow_dispatch' || github.event_name == 'push' | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Missing concurrency: group — concurrent runs can produce git push conflicts.
Two PR merges that both touch .sol files in quick succession will trigger two simultaneous runs. Both try to push changelog commits to the same branch, causing one push to fail.
🔧 Proposed fix
jobs:
generate-changelog:
+ concurrency:
+ group: ${{ github.workflow }}-${{ github.ref }}
+ cancel-in-progress: false # Do not cancel; serialize instead to avoid lost changelogs
if: github.event.pull_request.merged == true || ...As per coding guidelines: "Use concurrency: groups in workflows to prevent duplicate runs on the same PR in GitHub Actions workflows."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/generateContractChangelog.yml around lines 34 - 38, Add a
concurrency group to the generate-changelog job to prevent simultaneous runs
from pushing conflicting changelog commits: inside the job definition for
generate-changelog add a concurrency: block (e.g., using group: github.ref or a
combination like github.workflow + github.ref and cancel-in-progress: true) so
GitHub Actions serializes runs for the same branch/PR and cancels in-progress
duplicates.
| id: generate | ||
| run: | | ||
| echo "🤖 Running AI-powered changelog generation (Claude Sonnet)" | ||
| bun run script/changelogAnalysis/generateContractChangelog.ts | ||
| env: | ||
| COMMIT_SHA: ${{ github.event.inputs.commit_sha || github.event.pull_request.merge_commit_sha || github.sha }} | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| REPOSITORY: ${{ github.repository }} | ||
| CLAUDE_CODE_SC_CONTRACTS_REPO_CHANGELOGS_API_KEY: ${{ secrets.CLAUDE_CODE_SC_CONTRACTS_REPO_CHANGELOGS_API_KEY }} |
There was a problem hiding this comment.
COMMIT_SHA env var is never read by the script — workflow_dispatch commit_sha input is silently ignored.
The TypeScript script always resolves the commit with execSync('git rev-parse HEAD') (line 280 of generateContractChangelog.ts) and never reads process.env.COMMIT_SHA. Providing a commit_sha in workflow_dispatch has no effect; the script always analyzes the current HEAD, making the manual re-analysis feature non-functional.
Fix options:
- Remove the
commit_shainput andCOMMIT_SHAenv var if re-analyzing historical commits is not needed. - Have the script read
process.env.COMMIT_SHAand, when set, rungit checkout ${COMMIT_SHA}(or use it directly ingit diff/git show) before the analysis loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/generateContractChangelog.yml around lines 57 - 65, The
workflow sets COMMIT_SHA but the script generateContractChangelog.ts currently
ignores it (it always uses execSync('git rev-parse HEAD')); either remove the
workflow's COMMIT_SHA input/env if you won't support historical re-analysis, or
modify generateContractChangelog.ts to read process.env.COMMIT_SHA and, when
present, use it (e.g., git checkout ${COMMIT_SHA} before analysis or pass it
into git diff/show commands) instead of calling execSync('git rev-parse HEAD')
so the workflow_dispatch commit_sha actually controls which commit is analyzed.
| if [ "${{ steps.check.outputs.has_changes }}" == "true" ]; then | ||
| echo "✅ changelog/ updated" >> $GITHUB_STEP_SUMMARY | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| echo "### Updated:" >> $GITHUB_STEP_SUMMARY | ||
| git diff --name-only | grep "^changelog/" | sed 's/^/- /' >> $GITHUB_STEP_SUMMARY || true | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| echo "### Contract changes analyzed:" >> $GITHUB_STEP_SUMMARY | ||
| # Show files from the merged PR or push | ||
| if [ -n "${{ github.event.pull_request.number }}" ]; then | ||
| echo "**PR #${{ github.event.pull_request.number }}**: ${{ github.event.pull_request.title }}" >> $GITHUB_STEP_SUMMARY | ||
| fi | ||
| git diff HEAD~1 --name-only --diff-filter=AM src/**/*.sol 2>/dev/null | sed 's/^/- /' >> $GITHUB_STEP_SUMMARY || echo "- (see commit for details)" >> $GITHUB_STEP_SUMMARY |
There was a problem hiding this comment.
Summary step shows empty/incorrect data after the changelog has been committed.
- Line 101:
git diff --name-onlyruns after the "Commit and push" step has already committed all changelog files. At that point working tree equals HEAD, so the grep produces nothing — the "Updated:" list is always empty. - Line 108: After the changelog commit,
HEADis thechore: add contract changelog…commit andHEAD~1is the merge commit. The merge commit itself contains no.solchanges (those were in the PR's feature branch), sogit diff HEAD~1 --diff-filter=AM src/**/*.solalways falls through to"- (see commit for details)".
🔧 Proposed fix
- git diff --name-only | grep "^changelog/" | sed 's/^/- /' >> $GITHUB_STEP_SUMMARY || true
+ git show --name-only --format="" HEAD | grep "^changelog/" | sed 's/^/- /' >> $GITHUB_STEP_SUMMARY || trueFor the Solidity files section, compare against the commit prior to the changelog commit:
- git diff HEAD~1 --name-only --diff-filter=AM src/**/*.sol 2>/dev/null | sed 's/^/- /' >> $GITHUB_STEP_SUMMARY || echo "- (see commit for details)" >> $GITHUB_STEP_SUMMARY
+ git diff HEAD~2 --name-only --diff-filter=AM src/**/*.sol 2>/dev/null | sed 's/^/- /' >> $GITHUB_STEP_SUMMARY || echo "- (see commit for details)" >> $GITHUB_STEP_SUMMARY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/generateContractChangelog.yml around lines 97 - 108,
Update the two git-diff invocations so they compare against the commit before
the changelog commit instead of the working tree or the merge commit: replace
the first `git diff --name-only | grep "^changelog/"` with `git diff --name-only
HEAD~1 | grep "^changelog/"` to list files that were just committed to
changelog, and replace the second `git diff HEAD~1 --name-only --diff-filter=AM
src/**/*.sol` with `git diff HEAD~2 --name-only --diff-filter=AM src/**/*.sol`
so Solidity changes from the feature/merge are shown (keep the existing `sed`
and fallback `|| echo "- (see commit for details)"` behavior).
| interface ChangelogEntry { | ||
| date: string | ||
| commitSha: string | ||
| commitMessage: string | ||
| changes: { | ||
| breaking: string[] | ||
| added: string[] | ||
| changed: string[] | ||
| removed: string[] | ||
| fixed: string[] | ||
| } | ||
| } |
There was a problem hiding this comment.
Interface ChangelogEntry violates the required I prefix naming convention.
Per coding guidelines, all interfaces must be prefixed with I (e.g., IChangelogEntry). ESLint enforces this rule.
🔧 Proposed fix
-interface ChangelogEntry {
+interface IChangelogEntry {Also update all usages: formatChangelogEntry(entry: IChangelogEntry), let combinedEntry: IChangelogEntry = ..., and related type annotations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@script/changelogAnalysis/generateContractChangelog.ts` around lines 31 - 42,
Rename the interface ChangelogEntry to IChangelogEntry throughout the file and
any files that import or reference it; update the interface declaration name and
then change all type annotations/usages such as formatChangelogEntry(entry:
ChangelogEntry) -> formatChangelogEntry(entry: IChangelogEntry), variable
declarations like let combinedEntry: ChangelogEntry -> let combinedEntry:
IChangelogEntry, function parameters/returns and any generics or array types
(e.g., ChangelogEntry[] -> IChangelogEntry[]); ensure any exports/imports are
adjusted to the new name to satisfy the ESLint interface-prefix rule.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
script/changelogAnalysis/generateContractChangelog.ts (6)
31-42: Interface name must useIprefix (e.g.,IChangelogEntry).Per coding guidelines, interfaces must start with
I, enforced by ESLint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/changelogAnalysis/generateContractChangelog.ts` around lines 31 - 42, The interface is named ChangelogEntry but our ESLint rule requires interface names to start with an I; rename the interface to IChangelogEntry and update all references/usages to use IChangelogEntry (e.g., the type annotations, function signatures, and any imports/exports that reference ChangelogEntry) so the symbol name is consistent across the file and project.
1-9: Add colocated Bun tests for this script's helpers.Functions like
extractContractName,formatChangelogEntry,formatContractSections,updateChangelog, andupdateContractChangelogcontain testable logic. Per coding guidelines, new TypeScript helpers must be covered by a colocated*.test.tsfile. Based on learnings: "New TypeScript helpers must be covered by a colocated*.test.tsfile using Bun (describe/it/expect) with 100% coverage, including edge cases and error paths."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/changelogAnalysis/generateContractChangelog.ts` around lines 1 - 9, Add a colocated Bun test file (generateContractChangelog.test.ts) next to generateContractChangelog.ts that exercises extractContractName, formatChangelogEntry, formatContractSections, updateChangelog, and updateContractChangelog using Bun's describe/it/expect; write tests to cover normal flows, edge cases (empty inputs, malformed solidity files, missing sections), and error paths (API failures or invalid arguments) so each helper reaches 100% coverage, mocking any external IO or API calls used by updateChangelog/updateContractChangelog to assert behavior without side effects.
299-310: UseconstforcombinedEntry— the variable is never reassigned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/changelogAnalysis/generateContractChangelog.ts` around lines 299 - 310, The variable combinedEntry is never reassigned, so change its declaration from let to const to reflect immutability; update the declaration of combinedEntry: ChangelogEntry = { ... } to use const combinedEntry so intent is clearer and linting warnings are resolved (the object fields can remain mutable if needed).
286-286: ReplaceforEachwithfor...ofto satisfy BiomeuseIterableCallbackReturn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/changelogAnalysis/generateContractChangelog.ts` at line 286, Replace the array .forEach usage with an explicit for...of loop to satisfy the Biome rule (useIterableCallbackReturn): iterate over the changedFiles iterable using "for (const file of changedFiles)" and call console.log inside the loop; locate the occurrence referencing changedFiles and the console.log callback and swap to the for...of form so no callback is passed to forEach.
11-18: Missing blank line between import groups.ESLint
import/orderrequires a blank line between the builtin imports and the internal./aiChangelogAnalyzerimport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/changelogAnalysis/generateContractChangelog.ts` around lines 11 - 18, The import groups in generateContractChangelog.ts are missing a separating blank line between the builtin/node imports (execSync, existsSync, mkdirSync, readFileSync, writeFileSync, join) and the internal module import (analyzeContractChangesWithAI, buildContractDiff, getFileDiff); add a single blank line between those two groups so the builtin imports are visually separated from the relative './aiChangelogAnalyzer' import to satisfy ESLint import/order.
270-371: Refactor to usecittyfor CLI wiring andconsolafor logging.The script uses bare
console.log/console.errorthroughout and readsprocess.env.REPOSITORY/process.env.COMMIT_SHAwithout validation helpers. Per coding guidelines,script/**/*.tsfiles must usecitty,consola, andgetEnvVar().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/changelogAnalysis/generateContractChangelog.ts` around lines 270 - 371, Replace raw console and process.env usage in mainWithAI with the project's CLI/logging helpers: wire mainWithAI as a citty command (using citty's command export) and replace console.log/console.error with consola methods (consola.info/warn/error/debug) inside mainWithAI and the final error handler; read COMMIT_SHA and REPOSITORY using getEnvVar (e.g., getEnvVar('COMMIT_SHA') and getEnvVar('REPOSITORY')) instead of process.env directly and validate them before use; keep the rest of the flow (calls to buildContractDiff, analyzeContractChangesWithAI, formatChangelogEntry, updateChangelog, updateContractChangelog) identical but ensure errors are logged with consola and the citty command returns appropriate exit codes rather than process.exit in the library code.
🧹 Nitpick comments (3)
script/changelogAnalysis/generateContractChangelog.ts (3)
78-87: Regex won't matchabstract contractorlibrarydeclarations.The pattern
/contract\s+(\w+)/requires the literal wordcontractwithout preceding keywords. Files definingabstract contract Fooorlibrary Foowill fall through to the filename-based fallback, which is acceptable but worth noting if accurate naming matters.🔧 Broader match if desired
- const contractMatch = content.match(/contract\s+(\w+)(?:\s+is|\s*{)/) + const contractMatch = content.match(/(?:abstract\s+)?(?:contract|library|interface)\s+(\w+)(?:\s+is|\s*{)/)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/changelogAnalysis/generateContractChangelog.ts` around lines 78 - 87, The current extractContractName function uses a regex that only matches plain "contract" and misses declarations like "abstract contract" or "library"; update the regex in extractContractName to match optional prefixes and other declaration types (e.g., /(abstract\s+)?(?:contract|library|interface)\s+(\w+)(?:\s+is|\s*{)/) and return the appropriate capture group (the name capture) instead of the old group index, keeping the existing filename fallback behavior intact.
185-198: Fragile re-parsing of already-formatted markdown to extract sections.
updateChangelogreceives the fully formatted markdown fromformatChangelogEntry, then re-parses it viasplit('\n')[0]andslice(4)to extract the commit message and section body. If the format offormatChangelogEntrychanges (e.g., an extra blank line or header field), this silently breaks.Consider passing structured data (commit message + sections string) directly to
updateChangeloginstead of formatting first and re-parsing after.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/changelogAnalysis/generateContractChangelog.ts` around lines 185 - 198, updateChangelog currently re-parses the already-formatted markdown from formatChangelogEntry using fragile split/slice logic; avoid this by returning structured data (e.g. { commitMessage, sections }) from formatChangelogEntry (or changing updateChangelog to accept commitMessage and sections directly) and use those fields to build the block instead of using split('\n')[0] and slice(4); update callers so formatChangelogEntry provides the commit message and sections and then construct the block with commitSha, commitUrl, commitDate, commitAuthor and the provided sections string.
92-168:formatChangelogEntryandformatContractSectionsduplicate section-rendering logic.Both functions iterate over the same five categories (
breaking,added,changed,removed,fixed) with identical emoji headings. Consider havingformatChangelogEntrydelegate toformatContractSectionsfor the section part, keeping the header/commit-link responsibility informatChangelogEntryalone.♻️ Sketch
function formatChangelogEntry(entry: ChangelogEntry): string { let markdown = `## [${entry.date}] - ${entry.commitMessage}\n\n` markdown += `**Commit**: [\`${entry.commitSha}\`](../../commit/${entry.commitSha})\n\n` - if (entry.changes.breaking.length > 0) { - markdown += `### ⚠️ Breaking Changes\n\n` - ... - } - // ... repeated for each section + markdown += formatContractSections(entry.changes) return markdown }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/changelogAnalysis/generateContractChangelog.ts` around lines 92 - 168, formatChangelogEntry duplicates the section-rendering logic already implemented in formatContractSections; refactor formatChangelogEntry to keep only the header/commit-link construction and delegate the five-category section rendering to formatContractSections (pass entry.changes to it, adapting the analysis shape if needed), remove the repeated for-loops/emoji headings from formatChangelogEntry, and ensure formatContractSections handles optional context the same way so output is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@script/changelogAnalysis/generateContractChangelog.ts`:
- Around line 180-181: The git queries hardcode "HEAD" causing wrong metadata
when a different commit is provided; in both updateChangelog and
updateContractChangelog replace the execSync calls that compute commitDate and
commitAuthor (currently using 'git log -1 --format=%ci HEAD' and 'git log -1
--format=%an HEAD') to use the function parameter commitSha (or the COMMIT_SHA
env fallback) instead of HEAD, e.g. pass commitSha into the git log commands
(and preserve a fallback to 'HEAD' only if commitSha is falsy) so commitDate and
commitAuthor reflect the intended commit.
- Around line 313-348: The loop over changedFiles calls
analyzeContractChangesWithAI without protection, so a thrown error aborts the
whole run; wrap the per-file work that includes buildContractDiff,
analyzeContractChangesWithAI, and the combinedEntry/perContract updates in a
try/catch around each file iteration (i.e., inside the for (const file of
changedFiles) block), on error log the file and error (using console.error),
push an informative note into combinedEntry.changes.changed or perContract
(e.g., mark the contractName with an analysis failure), and continue to the next
file so prior results are preserved.
---
Duplicate comments:
In `@script/changelogAnalysis/generateContractChangelog.ts`:
- Around line 31-42: The interface is named ChangelogEntry but our ESLint rule
requires interface names to start with an I; rename the interface to
IChangelogEntry and update all references/usages to use IChangelogEntry (e.g.,
the type annotations, function signatures, and any imports/exports that
reference ChangelogEntry) so the symbol name is consistent across the file and
project.
- Around line 1-9: Add a colocated Bun test file
(generateContractChangelog.test.ts) next to generateContractChangelog.ts that
exercises extractContractName, formatChangelogEntry, formatContractSections,
updateChangelog, and updateContractChangelog using Bun's describe/it/expect;
write tests to cover normal flows, edge cases (empty inputs, malformed solidity
files, missing sections), and error paths (API failures or invalid arguments) so
each helper reaches 100% coverage, mocking any external IO or API calls used by
updateChangelog/updateContractChangelog to assert behavior without side effects.
- Around line 299-310: The variable combinedEntry is never reassigned, so change
its declaration from let to const to reflect immutability; update the
declaration of combinedEntry: ChangelogEntry = { ... } to use const
combinedEntry so intent is clearer and linting warnings are resolved (the object
fields can remain mutable if needed).
- Line 286: Replace the array .forEach usage with an explicit for...of loop to
satisfy the Biome rule (useIterableCallbackReturn): iterate over the
changedFiles iterable using "for (const file of changedFiles)" and call
console.log inside the loop; locate the occurrence referencing changedFiles and
the console.log callback and swap to the for...of form so no callback is passed
to forEach.
- Around line 11-18: The import groups in generateContractChangelog.ts are
missing a separating blank line between the builtin/node imports (execSync,
existsSync, mkdirSync, readFileSync, writeFileSync, join) and the internal
module import (analyzeContractChangesWithAI, buildContractDiff, getFileDiff);
add a single blank line between those two groups so the builtin imports are
visually separated from the relative './aiChangelogAnalyzer' import to satisfy
ESLint import/order.
- Around line 270-371: Replace raw console and process.env usage in mainWithAI
with the project's CLI/logging helpers: wire mainWithAI as a citty command
(using citty's command export) and replace console.log/console.error with
consola methods (consola.info/warn/error/debug) inside mainWithAI and the final
error handler; read COMMIT_SHA and REPOSITORY using getEnvVar (e.g.,
getEnvVar('COMMIT_SHA') and getEnvVar('REPOSITORY')) instead of process.env
directly and validate them before use; keep the rest of the flow (calls to
buildContractDiff, analyzeContractChangesWithAI, formatChangelogEntry,
updateChangelog, updateContractChangelog) identical but ensure errors are logged
with consola and the citty command returns appropriate exit codes rather than
process.exit in the library code.
---
Nitpick comments:
In `@script/changelogAnalysis/generateContractChangelog.ts`:
- Around line 78-87: The current extractContractName function uses a regex that
only matches plain "contract" and misses declarations like "abstract contract"
or "library"; update the regex in extractContractName to match optional prefixes
and other declaration types (e.g.,
/(abstract\s+)?(?:contract|library|interface)\s+(\w+)(?:\s+is|\s*{)/) and return
the appropriate capture group (the name capture) instead of the old group index,
keeping the existing filename fallback behavior intact.
- Around line 185-198: updateChangelog currently re-parses the already-formatted
markdown from formatChangelogEntry using fragile split/slice logic; avoid this
by returning structured data (e.g. { commitMessage, sections }) from
formatChangelogEntry (or changing updateChangelog to accept commitMessage and
sections directly) and use those fields to build the block instead of using
split('\n')[0] and slice(4); update callers so formatChangelogEntry provides the
commit message and sections and then construct the block with commitSha,
commitUrl, commitDate, commitAuthor and the provided sections string.
- Around line 92-168: formatChangelogEntry duplicates the section-rendering
logic already implemented in formatContractSections; refactor
formatChangelogEntry to keep only the header/commit-link construction and
delegate the five-category section rendering to formatContractSections (pass
entry.changes to it, adapting the analysis shape if needed), remove the repeated
for-loops/emoji headings from formatChangelogEntry, and ensure
formatContractSections handles optional context the same way so output is
unchanged.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
script/changelogAnalysis/generateContractChangelog.ts
| const commitDate = execSync('git log -1 --format=%ci HEAD', { encoding: 'utf-8' }).trim() | ||
| const commitAuthor = execSync('git log -1 --format=%an HEAD', { encoding: 'utf-8' }).trim() |
There was a problem hiding this comment.
Bug: git log queries hardcode HEAD instead of using commitSha.
updateChangelog receives commitSha as a parameter, but lines 180–181 query HEAD for the commit date and author. When COMMIT_SHA env var points to a commit that isn't HEAD (e.g., the workflow checks out a different ref), the written metadata will be wrong. The same bug exists in updateContractChangelog at line 236.
🐛 Proposed fix
- const commitDate = execSync('git log -1 --format=%ci HEAD', { encoding: 'utf-8' }).trim()
- const commitAuthor = execSync('git log -1 --format=%an HEAD', { encoding: 'utf-8' }).trim()
+ const commitDate = execSync(`git log -1 --format=%ci ${commitSha}`, { encoding: 'utf-8' }).trim()
+ const commitAuthor = execSync(`git log -1 --format=%an ${commitSha}`, { encoding: 'utf-8' }).trim()And in updateContractChangelog (line 236):
- const commitAuthor = execSync('git log -1 --format=%an HEAD', { encoding: 'utf-8' }).trim()
+ const commitAuthor = execSync(`git log -1 --format=%an ${commitSha}`, { encoding: 'utf-8' }).trim()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const commitDate = execSync('git log -1 --format=%ci HEAD', { encoding: 'utf-8' }).trim() | |
| const commitAuthor = execSync('git log -1 --format=%an HEAD', { encoding: 'utf-8' }).trim() | |
| const commitDate = execSync(`git log -1 --format=%ci ${commitSha}`, { encoding: 'utf-8' }).trim() | |
| const commitAuthor = execSync(`git log -1 --format=%an ${commitSha}`, { encoding: 'utf-8' }).trim() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@script/changelogAnalysis/generateContractChangelog.ts` around lines 180 -
181, The git queries hardcode "HEAD" causing wrong metadata when a different
commit is provided; in both updateChangelog and updateContractChangelog replace
the execSync calls that compute commitDate and commitAuthor (currently using
'git log -1 --format=%ci HEAD' and 'git log -1 --format=%an HEAD') to use the
function parameter commitSha (or the COMMIT_SHA env fallback) instead of HEAD,
e.g. pass commitSha into the git log commands (and preserve a fallback to 'HEAD'
only if commitSha is falsy) so commitDate and commitAuthor reflect the intended
commit.
| for (const file of changedFiles) { | ||
| console.log(`\n🔍 Analyzing ${file}...`) | ||
|
|
||
| const oldContent = getFileAtCommit(file, parentCommit) | ||
| const newContent = getFileAtCommit(file, commitSha) | ||
|
|
||
| if (!newContent) { | ||
| console.log(` ⚠️ Skipped (file deleted or not accessible)`) | ||
| continue | ||
| } | ||
|
|
||
| const contractName = extractContractName(newContent, file) | ||
| const diff = getFileDiff(file, parentCommit, commitSha) | ||
|
|
||
| if (!diff || diff.trim().length === 0) { | ||
| console.log(` ℹ️ No diff found`) | ||
| continue | ||
| } | ||
|
|
||
| const contractDiff = buildContractDiff(file, contractName, oldContent, newContent, diff) | ||
| const aiAnalysis = await analyzeContractChangesWithAI(contractDiff) | ||
|
|
||
| console.log(` ✅ Analysis complete`) | ||
| console.log(` Summary: ${aiAnalysis.summary}`) | ||
|
|
||
| combinedEntry.changes.breaking.push(...aiAnalysis.breaking) | ||
| combinedEntry.changes.added.push(...aiAnalysis.added) | ||
| combinedEntry.changes.changed.push(...aiAnalysis.changed) | ||
| combinedEntry.changes.removed.push(...aiAnalysis.removed) | ||
| combinedEntry.changes.fixed.push(...aiAnalysis.fixed) | ||
|
|
||
| if (aiAnalysis.context) { | ||
| combinedEntry.changes.changed.push(`**Note**: ${aiAnalysis.context}`) | ||
| } | ||
| perContract.push({ contractName, aiAnalysis }) | ||
| } |
There was a problem hiding this comment.
A single AI analysis failure aborts the entire run, discarding all prior results.
analyzeContractChangesWithAI can throw (network errors, API failures, malformed responses). Since there's no per-file try/catch, one bad file stops the loop and the top-level .catch exits the process—losing changelog entries for files that were already analyzed successfully.
🛡️ Proposed fix — wrap per-file analysis in try/catch
for (const file of changedFiles) {
console.log(`\n🔍 Analyzing ${file}...`)
-
- const oldContent = getFileAtCommit(file, parentCommit)
- const newContent = getFileAtCommit(file, commitSha)
-
- if (!newContent) {
- console.log(` ⚠️ Skipped (file deleted or not accessible)`)
- continue
- }
-
- const contractName = extractContractName(newContent, file)
- const diff = getFileDiff(file, parentCommit, commitSha)
-
- if (!diff || diff.trim().length === 0) {
- console.log(` ℹ️ No diff found`)
- continue
- }
-
- const contractDiff = buildContractDiff(file, contractName, oldContent, newContent, diff)
- const aiAnalysis = await analyzeContractChangesWithAI(contractDiff)
+ try {
+ const oldContent = getFileAtCommit(file, parentCommit)
+ const newContent = getFileAtCommit(file, commitSha)
+
+ if (!newContent) {
+ console.log(` ⚠️ Skipped (file deleted or not accessible)`)
+ continue
+ }
+
+ const contractName = extractContractName(newContent, file)
+ const diff = getFileDiff(file, parentCommit, commitSha)
+
+ if (!diff || diff.trim().length === 0) {
+ console.log(` ℹ️ No diff found`)
+ continue
+ }
+
+ const contractDiff = buildContractDiff(file, contractName, oldContent, newContent, diff)
+ const aiAnalysis = await analyzeContractChangesWithAI(contractDiff)
- console.log(` ✅ Analysis complete`)
- console.log(` Summary: ${aiAnalysis.summary}`)
+ console.log(` ✅ Analysis complete`)
+ console.log(` Summary: ${aiAnalysis.summary}`)
- combinedEntry.changes.breaking.push(...aiAnalysis.breaking)
- ...
- perContract.push({ contractName, aiAnalysis })
+ combinedEntry.changes.breaking.push(...aiAnalysis.breaking)
+ combinedEntry.changes.added.push(...aiAnalysis.added)
+ combinedEntry.changes.changed.push(...aiAnalysis.changed)
+ combinedEntry.changes.removed.push(...aiAnalysis.removed)
+ combinedEntry.changes.fixed.push(...aiAnalysis.fixed)
+
+ if (aiAnalysis.context) {
+ combinedEntry.changes.changed.push(`**Note**: ${aiAnalysis.context}`)
+ }
+ perContract.push({ contractName, aiAnalysis })
+ } catch (error) {
+ const msg = error instanceof Error ? error.message : String(error)
+ console.error(` ❌ Failed to analyze ${file}: ${msg}`)
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@script/changelogAnalysis/generateContractChangelog.ts` around lines 313 -
348, The loop over changedFiles calls analyzeContractChangesWithAI without
protection, so a thrown error aborts the whole run; wrap the per-file work that
includes buildContractDiff, analyzeContractChangesWithAI, and the
combinedEntry/perContract updates in a try/catch around each file iteration
(i.e., inside the for (const file of changedFiles) block), on error log the file
and error (using console.error), push an informative note into
combinedEntry.changes.changed or perContract (e.g., mark the contractName with
an analysis failure), and continue to the next file so prior results are
preserved.
…wner(); update changelog README; only code changes trigger changelog
3 participants
Which Jira task belongs to this PR?
https://lifi.atlassian.net/browse/SMAR-41
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)