Commits on synchronize event

[kapcod]

Hello, as far as I can understand this code

ChatGPT-CodeReview/src/bot.ts

Line 102 in 763282a

base: commits[commits.length - 2].sha,

in case of synchronize PR event this action only looks at the last commit. However not always a push adds only 1 commit.
Why not use github.event.before instead? It can cause issues in cases of merges from master/main into the current branch and in cases of rebasing the current branch. Therefore, before can be used only when the branch state is base -> before -> head, and in all other cases, it can fall back to the current solution with the last commit.

[anc95]

yes. it makes sense to use before.

It can cause issues in cases of merges from master/main into the current branch and in cases of rebasing the current branch.

i didn't get this. if merge the main to current branch, there would be a new commit, it should be expected to review, right? it may contain some conflict resolve code. can you classify on this. Thank you

[kapcod]

If branch was merged/rebased with master, "diff before..head" will also include "diff before..master" - potentially hundreds of unrelated commits. Theoretically it can be detected and even fixed by merging "before" with "base". Then it can be compared with "refs/pull/NUMBER/merge" (Google says it should be available at PR time). Or just ignore it and fallback to full review. 🤷‍♂️

…

On Sun, Jan 4, 2026, 05:25 小安 ***@***.***> wrote: *anc95* left a comment (anc95/ChatGPT-CodeReview#202) <#202 (comment)> yes. it makes sense to use before. It can cause issues in cases of merges from master/main into the current branch and in cases of rebasing the current branch. i didn't get this. if merge the main to current branch, there would be a new commit, it should be expected to review, right? it may contain some conflict resolve code. can you classify on this. Thank you — Reply to this email directly, view it on GitHub <#202 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHKEBU75NJTGTRENNHSCJ34FCBZNAVCNFSM6AAAAACQMNUV22VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTOMBXGU4DCNJYGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[kapcod]

Here's the full code suggestion (in bash) to calculate partial diff whenever possible. I tried generating the ts code using Gemini, but I'm not sure if the resulting code is working.

# --- 1. Setup Standard Variables ---
          EVENT="${{ github.event.action }}"
          # Use origin/ ref to ensure we have the remote state of the base branch
          BASE="origin/${{ github.event.pull_request.base.ref }}"
          HEAD="${{ github.event.pull_request.head.sha }}"
          
          # Initialize BEFORE only for synchronize events
          BEFORE=""
          if [[ "$EVENT" == "synchronize" ]]; then
            BEFORE="${{ github.event.before }}"
          fi

          echo "Event: $EVENT"
          echo "Base: $BASE"
          echo "Head: $HEAD"

          # --- 2. The Logic Switch ---
          
          # Check: Do we have a valid BEFORE commit to compare against?
          if [[ -n "$BEFORE" ]]; then
            echo "Update detected. Checking integration topology..."
            
            # Check if the integration point (merge-base) has moved.
            # MB_OLD: Where the PREVIOUS commit branched off from Base
            # MB_NEW: Where the CURRENT commit branches off from Base
            MB_OLD=$(git merge-base "$BEFORE" "$BASE")
            MB_NEW=$(git merge-base "$HEAD" "$BASE")

            if [ "$MB_OLD" == "$MB_NEW" ]; then
               # ---------------------------------------------------------
               # Path A: Linear Update (Fastest)
               # ---------------------------------------------------------
               # The base branch hasn't been merged in. We can just check 
               # the diff between the previous push and this one.
               echo "Linear update. Scanning $BEFORE..$HEAD"
               git diff "$BEFORE..$HEAD"
            else
               # ---------------------------------------------------------
               # Path B: Complex Update (Virtual Merge)
               # ---------------------------------------------------------
               # The integration point changed (Merge/Rebase from Master).
               # We use Virtual Merges to cancel out the noise from Master.
               echo "Rebase/Merge detected. Using Virtual Merge Strategy."
               
               TREE_OLD=$(git merge-tree --write-tree "$BASE" "$BEFORE")
               TREE_NEW=$(git merge-tree --write-tree "$BASE" "$HEAD")
               
               git diff "$TREE_OLD" "$TREE_NEW"
            fi

          else
            # ---------------------------------------------------------
            # Path C: Full Scan (Fallback / First Run)
            # ---------------------------------------------------------
            # This handles 'opened', 'reopened', or any error case where
            # 'before' is missing.
            echo "New/Reopened PR. Scanning full PR changes."
            
            # Use 3-dot diff to compare Head against the common ancestor
            git diff "$BASE...$HEAD"
          fi

[kapcod]

This is the ts code suggestion by Gemini - I don't know octokit api to know if it's working. Also this doesn't do merge-tree as we can't run git commands in this context.

// 1. Identify the scan range (Default to Full PR Scan: Base...Head)
      const baseSha = context.payload.pull_request.base.sha;
      const headSha = context.payload.pull_request.head.sha;
      const beforeSha = context.payload.before; // Only exists on 'synchronize'

      let compareBase = baseSha;
      let compareHead = headSha;
      let scanType = 'full';

      // 2. Smart Logic: Check if we can do an Incremental Scan
      if (context.payload.action === 'synchronize' && beforeSha) {
        log.info(`Event is synchronize. Analyzing topology for incremental scan...`);

        try {
          // Check if the integration point (merge-base) has changed.
          // This detects if 'master' was merged into the PR branch.
          const [baseBefore, baseHead] = await Promise.all([
            context.octokit.repos.compareCommits({
              owner: repo.owner,
              repo: repo.repo,
              base: baseSha,
              head: beforeSha,
            }),
            context.octokit.repos.compareCommits({
              owner: repo.owner,
              repo: repo.repo,
              base: baseSha,
              head: headSha,
            }),
          ]);

          const mergeBaseOld = baseBefore.data.merge_base_commit.sha;
          const mergeBaseNew = baseHead.data.merge_base_commit.sha;

          if (mergeBaseOld === mergeBaseNew) {
            // Integration point is stable (Linear update).
            // It is safe to scan only the new commits.
            compareBase = beforeSha;
            compareHead = headSha;
            scanType = 'incremental';
            log.info(`Topology stable. Using incremental scan: ${beforeSha}...${headSha}`);
          } else {
            // Integration point changed (likely merged master).
            // Fallback to full scan to avoid showing all of master's changes as "new".
            log.info(`Merge/Rebase detected. Falling back to full PR scan: ${baseSha}...${headSha}`);
          }
        } catch (error) {
          log.warn("Failed to determine topology. Defaulting to full scan.", error);
        }
      }

      // 3. Perform the actual diff fetch based on the calculated strategy
      const data = await context.octokit.repos.compareCommits({
        owner: repo.owner,
        repo: repo.repo,
        base: compareBase,
        head: compareHead,
      });

      let { files: changedFiles, commits } = data.data;
      
      log.info(`Scan complete (${scanType}). Files changed: ${changedFiles?.length || 0}`);