googleapis / googleapis/release-please
Commit walk is chronological, not topological, so commits on the release branch can be silently omitted from a release
- Dominant language
- TypeScript
- Stars
- 7.5k
- Forks
- 588
- Avg merge
- 12h 16m
- Merged PRs (30d)
- 7
Description
### What happened
A `fix:` commit that was on `main` and not reachable from the previous release tag was not included in the release PR, and nothing reported it. Two of four releasable commits were listed. There was no error, no warning at default verbosity, and CI was green.
### Why
`Manifest.buildPullRequests` walks the release branch through a commit generator and stops as soon as it has seen the previous release's SHA — [src/manifest.ts#L671-L673](https://github.com/googleapis/release-please/blob/main/src/manifest.ts#L671-L673):
```js
} else if (!needsBootstrap && releaseCommitsFound >= expectedShas) {
// found enough commits
break;
}
```
The generator gets its commits from GitHub's GraphQL `history` connection with no `orderBy` — [src/github.ts#L254](https://github.com/googleapis/release-please/blob/main/src/github.ts#L254):
```graphql
history(first: $num, after: $cursor) { … }
```
`history` defaults to **committer date, descending**. So the walk is chronological, not topological, and *"I have reached the last release's commit"* is not the same statement as *"everything after this point is already released"*.
A branch cut *before* a release PR merged and merged *after* it has commits whose committer dates predate the release commit. They therefore sort *behind* the stop point and are never visited — even though they are on the release branch and are not reachable from the release tag.
### Reproduction
1. Open PR A. Let its commits be committed at T0.
2. release-please opens a release PR. Merge it at T1 > T0. The release commit's committer date is T1.
3. Merge PR A at T2 > T1. Its branch commits keep their T0 committer dates.
4. release-please runs. The walk sees the release commit at T1 before it reaches PR A's commits at T0, hits the `break`, and never reads them.
Measured on release-please 17.11.1 against a real repository with `release-pr --dry-run`:
| walk position | commit | committed |
|---|---|---|
| 8 | merge of the PR from step 3 | 08:39:00Z |
| **9** | **previous release commit — the walk stops here** | **08:26:51Z** |
| 15 | `fix(...)` from step 1 | 08:12:49Z |
| 16 | `fix(...)` from step 1 | 08:12:49Z |
`Splitting 8 commits by path`. Two of the four releasable commits reached the changelog.
### Why this is not a configuration mistake
- `bootstrap-sha` self-disables: `needsBootstrap` is false once a matching tag exists, so that branch of the condition never fires.
- `last-release-sha` does not widen the walk — the count-based `break` above is a sibling `else if` and fires regardless.
- No config key widens the walk past a matched release SHA.
### Suggested fix
The stop condition wants to be topological — *"this commit is an ancestor of the last release"* rather than *"this commit **is** the last release"*. Two shapes that would work:
- keep walking past the release SHA until the *set* of unreleased commits is exhausted, i.e. treat the release tag as an exclusion (`main --not `) rather than as a stopping point; or
- ask GitHub for the comparison directly (`GET /repos/{owner}/{repo}/compare/{tag}...{branch}`), which is topological by construction.
Either removes the failure entirely.
A cheaper mitigation, if the walk shape has to stay: warn when the walk terminates on the release SHA while commits remain that are not reachable from the release tag.
### Impact
The trigger is routine — it fires whenever a release PR merges while another pull request is open. The failure is silent in both directions: the release notes are simply short, and the only way to find it is to diff the release PR against `git log ..` by hand.
Contributor guide
Research direction
Start in src/manifest.ts around the release-commit stopping condition and src/github.ts around the GraphQL history query. Reproduce the release-pr --dry-run scenario with a release PR merged before another PR, then compare the walk with git log ... Done means unreleased commits are not silently omitted, with the behavior covered by an appropriate regression test.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- git, github, graphql, typescript
- Domain
- api, release
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100