fix: use merge commit timestamp for PR benchmark entries

[ktrz]

Problem

getCommitFromPullRequestPayload was using pr.head.repo.updated_at as the commit timestamp. This field reflects when the repository's metadata was last updated — for forks, that's typically the last time the fork was synced with upstream, which can be months in the past. The result is a wildly incorrect timestamp in the benchmark chart tooltip.

Solution analysis

We considered three approaches:

1. GitHub API (getCommitFromGitHubAPIRequest) — accurate, but requires github-token to be provided. The issue reporter suggested making the token always required for PR contexts, but we wanted to avoid that.

2. git log -1 --format=%aI <pr.head.sha> — accurate (real commit author date), no token needed. However, actions/checkout defaults to fetch-depth: 1 (shallow clone), which fetches the synthetic merge commit (refs/pull/N/merge) but not its parents. pr.head.sha is a parent of the merge commit and is not available as a git object in a shallow clone, so this fails in the common case.

3. git log -1 --format=%aI HEAD (chosen) — reads the timestamp from the merge commit GitHub creates for the PR. This commit is always HEAD after actions/checkout, accessible even with fetch-depth: 1. GitHub sets the merge commit's timestamp at creation time, which is within seconds of the actual push. Verified with a real PR: merge commit was 3 seconds newer than the PR head commit.

Trade-off

The timestamp is the merge commit time, not the developer's commit author time. The difference is consistently < 30 seconds in practice — acceptable for a chart tooltip, and vastly better than the current bug which can be months off for fork PRs.

No token required. No fork detection needed. No fallback complexity.

Fixes #327

Summary by CodeRabbit

• Bug Fixes
  • Improved commit timestamp accuracy by sourcing directly from git history instead of external data sources.

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request fixes incorrect commit timestamp extraction for GitHub pull requests. Instead of using pr.head.repo.updated_at (which reflects repo sync time, not commit time), the code now fetches the actual commit timestamp by executing git log -1 --format=%aI HEAD, making timestamp data accurate.

Changes

Cohort / File(s)	Summary
Commit Timestamp Extraction src/extract.ts	Converted getCommitFromPullRequestPayload to async function that fetches actual commit timestamp via git log command instead of relying on stale pr.head.repo.updated_at field. Updated getCommit caller to await the async operation.
Test Mocking and Expectations test/extract.spec.ts	Added mock for git command returning fixed ISO timestamp and updated test assertions to verify timestamp now comes from git command rather than webhook-provided repo metadata.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 Once timestamps were stale, from repos out of sync,
Now we hop to the commit, fetch the truth in a blink!
Git log tells the tale, accurate and bright,
No more outdated dates—the benchmark's now right! ✨

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: using merge commit timestamp instead of repository metadata for PR benchmark entries.
Linked Issues check	✅ Passed	The PR implements the chosen solution from issue #327: using git log to read the merge commit timestamp (HEAD), avoiding github-token requirement while providing accurate timestamps.
Out of Scope Changes check	✅ Passed	All changes are directly related to the timestamp fix: updating getCommitFromPullRequestPayload to async, using git cmd instead of pr.head.repo.updated_at, and updating related tests.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch ktrz/327-fix-pr-timestamp

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.27%. Comparing base (b0e7057) to head (013ea0e).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #343      +/-   ##
==========================================
+ Coverage   89.24%   89.27%   +0.02%     
==========================================
  Files          16       16              
  Lines         958      960       +2     
  Branches      203      204       +1     
==========================================
+ Hits          855      857       +2     
+ Misses        103       99       -4     
- Partials        0        4       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[ktrz]

@henrikingo I think we can fix the issue reported by you without using the GH API. It's still an approximation as explained in the PR description but should be only seconds off the real commit timestamp. WDYT?

[coderabbitai]

🧹 Nitpick comments (1)

test/extract.spec.ts (1)

215-245: Consider asserting the exact git command invocation.

To harden this regression test, assert that the mocked cmd was called with ([], 'log', '-1', '--format=%aI', 'HEAD').

Suggested test strengthening

+import { cmd } from '../src/git';
...
     it('collects the commit information from pull_request payload as fallback', async function () {
...
         const { commit } = await extractResult(config);
+        expect(cmd).toHaveBeenCalledWith([], 'log', '-1', '--format=%aI', 'HEAD');
...
         A.equal(commit.timestamp, '2024-01-15T10:30:00+00:00');

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/extract.spec.ts` around lines 215 - 245, Add an assertion after the call
to extractResult to verify the mocked cmd was invoked with the exact git args;
specifically assert that the mocked cmd function (mocked `cmd`) was called with
([], 'log', '-1', '--format=%aI', 'HEAD') to ensure the test validates the
precise git command used by extractResult.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@test/extract.spec.ts`:
- Around line 215-245: Add an assertion after the call to extractResult to
verify the mocked cmd was invoked with the exact git args; specifically assert
that the mocked cmd function (mocked `cmd`) was called with ([], 'log', '-1',
'--format=%aI', 'HEAD') to ensure the test validates the precise git command
used by extractResult.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a4948451-6135-457c-9658-385ede1e145a

📥 Commits

Reviewing files that changed from the base of the PR and between b0e7057 and 013ea0e.

📒 Files selected for processing (2)

• src/extract.ts
• test/extract.spec.ts

[henrikingo]

The issue reporter suggested making the token always required for PR contexts, but we wanted to avoid that.

Good call, I was pretty novice back then I guess.

fix the issue reported by you without using the GH API.

Well, you have to! I didn't realize back then that giving the GITHUB_TOKEN to random bypassers is not safe!

WDYT?

I went back to check how I had fixed this in the mean time. It turns out I also use the local git checkout to grab the latest git commit. Since PR commits are kinda ephemeral anyway, and more importantly, in a pull request context they will always be leaf nodes, I at least convinced myself, that using the merge commit from the pr branch is sufficient, because it will necessarily be a later timestamp than the tip of the base branch. And that is sufficient.

I also found in the pr api another timestamp called pr.head.repo.pushed_at.

Both of these are poorly documented and I can't remember exactly why I found it useable, but it seems to be the timestamp of the most recent push to the head repo. This too feels like it will necessarily be large enough that it's always >= the base commit timestamp. However, this timestamp is wrong / arbitrary, because if someone else pushes something into a completely unrelated branch, you will get their timestamp.

So in practice I use that value as a default or fallback timestamp and get the quasi-accurate timestamp from the locally checked out git repo, just like you here.