Skip to content

Use merge base commit for diffs and shallow clone in auto-review #95

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

catatsuy
Copy link
Owner

This pull request updates the GitHub Actions workflow in .github/workflows/auto-review.yml to optimize the handling of shallow clones and improve the accuracy of diffs by using the merge base commit. It also introduces a new language model and localization for the review process.

Workflow optimization and diff accuracy:

  • Changed the fetch-depth parameter in the actions/checkout step to 1 for a shallow clone, reducing the amount of data fetched. Added a step to fetch the merge base commit for accurate diff generation.
  • Updated the diff generation logic to use the merge base commit (MERGE_BASE_SHA) instead of the base branch reference, ensuring more precise comparisons.

Review process improvements:

  • Modified the bento review command to use the gpt-4.1-mini model and generate reviews in Japanese, enhancing the review process with a specific language model and localization.

Copy link

github-actions bot commented Apr 19, 2025

Automatic Review

One issue in error handling and robustness stands out related to the GitHub API and shell commands:

  • In the new step Fetch necessary commits, the curl call to the GitHub Compare API assumes jq is installed and available in the ubuntu-latest runner. By default, jq is not guaranteed to be present (unless the runner is pre-configured). If jq is missing, this will cause the step to fail cryptically.

Suggestion:
Add an explicit installation step for jq or check its availability before usage, e.g.:

    - name: Install dependencies
      run: sudo apt-get update && sudo apt-get install -y jq
  • The usage of git fetch --depth=1 origin ${MERGE_BASE_SHA} can fail if the commit is already present or if the SHA is invalid. There is no error handling around this command.

Suggestion:
Add error handling to fail gracefully or verify that the fetch was successful.

  • The HEAD_SHA variable is used inside shell scripts but not explicitly exported to the environment, and in one usage (git diff ${MERGE_BASE} ${HEAD_SHA}), it is referenced but not defined. The value of HEAD_SHA is initially fetched only in the "Fetch necessary commits" step but never set into $GITHUB_ENV like MERGE_BASE_SHA. Therefore, ${HEAD_SHA} in subsequent steps may be undefined or empty, potentially leading to wrong diffs or errors.

Suggestion:
Add echo "HEAD_SHA=${HEAD_SHA}" >> $GITHUB_ENV after fetching it, and refer to it consistently using env.HEAD_SHA in later steps. For example:

echo "HEAD_SHA=${HEAD_SHA}" >> $GITHUB_ENV

Then later:

MERGE_BASE=${{ env.MERGE_BASE_SHA }}
HEAD=${{ env.HEAD_SHA }}
git diff ${MERGE_BASE} ${HEAD} ...

This ensures the correct commits are used throughout.


In summary, improvements:

    - name: Install dependencies
      run: sudo apt-get update && sudo apt-get install -y jq

    - name: Fetch necessary commits
      run: |
        BASE_SHA="${{ github.event.pull_request.base.sha }}"
        HEAD_SHA="${{ github.event.pull_request.head.sha }}"
        echo "Base SHA: ${BASE_SHA}"
        echo "Head SHA: ${HEAD_SHA}"
        MERGE_BASE_SHA=$(curl -s \
          -H "Authorization: token ${{ secrets.GITHUB_TOKEN }}" \
          -H "Accept: application/vnd.github+json" \
          "https://api.github.com/repos/${{ github.repository }}/compare/${BASE_SHA}...${HEAD_SHA}" | jq -r '.merge_base_commit.sha')
        if [ "$MERGE_BASE_SHA" = "null" ] || [ -z "$MERGE_BASE_SHA" ]; then
          echo "Error: Could not retrieve merge base commit SHA."
          exit 1
        fi
        echo "Merge base commit SHA: ${MERGE_BASE_SHA}"
        echo "MERGE_BASE_SHA=${MERGE_BASE_SHA}" >> $GITHUB_ENV
        echo "HEAD_SHA=${HEAD_SHA}" >> $GITHUB_ENV
        git fetch --depth=1 origin ${MERGE_BASE_SHA} || echo "Warning: Failed to fetch merge base commit"

    - name: Check for diffs and set SKIP_REVIEW
      run: |
        MERGE_BASE=${{ env.MERGE_BASE_SHA }}
        HEAD=${{ env.HEAD_SHA }}
        echo "Using merge base: ${MERGE_BASE} and head: ${HEAD}"
        DIFF_OUTPUT=$(git diff ${MERGE_BASE} ${HEAD} -- ':!go.sum')
        DIFF_LINES=$(echo "$DIFF_OUTPUT" | wc -l)
        if [ -z "$DIFF_OUTPUT" ]; then
          echo "No changes detected, skipping review."
          echo "SKIP_REVIEW=true" >> $GITHUB_ENV
          exit 0
        elif [ "$DIFF_LINES" -gt 500 ]; then
          echo "Diff too large (${DIFF_LINES} lines), skipping review."
          echo "SKIP_REVIEW=true" >> $GITHUB_ENV
          exit 0
        fi

This will avoid silent failures, undefined variables, and missing dependencies leading to clearer, more reliable workflows.

@catatsuy catatsuy force-pushed the feature-shallow-clone-fetch-merge-base-commit-and-diff branch from c18a073 to 1898a5f Compare April 19, 2025 05:18
@catatsuy catatsuy merged commit 323108c into main Apr 19, 2025
5 checks passed
@catatsuy catatsuy deleted the feature-shallow-clone-fetch-merge-base-commit-and-diff branch April 19, 2025 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant