feat(gitter): affected commits graph walking by Ly-Joey · Pull Request #4974 · google/osv.dev

Ly-Joey · 2026-03-05T22:38:55Z

Add graph related logic and find affected commits.

Definition of affected commits:

A commit is affected when: from at least one introduced that is an ancestor of the commit, there is no path between them that passes through a fix. A fix can either be a fixed commit, or the children of a lastAffected commit.

Key changes

New endpoint:

POST /affected-commits:
- Accepts a list of events (each containing an eventType (introduced, fixed, lastAffected, or limit) and commit hash)
- Returns a list of affected commits (hash in bytes and associated refs) in protobuf format

Cherry-pick Detection (optional):
Expand introduced and fixed commits to include their cherry-picked equivalents (determined with PatchID)

There's a follow-up PR (WIP) to refactor gitter (the repetitive fetches and semaphore logic, etc) and add LRU cache for repos.

Ly-Joey · 2026-03-09T05:54:07Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces significant new functionality for determining affected commits by walking the git graph. However, it also introduces several critical security vulnerabilities, primarily related to command injection and SSRF. The most severe issue is in buildCommitGraph, where user-controlled data is concatenated into a shell command, and new endpoints lack the protocol validation present in existing ones, allowing for command execution via git protocols. Additionally, a potential Denial of Service vulnerability due to unvalidated input length during hash parsing was identified. Beyond these security concerns, a critical bug was found in the graph building logic, and there are opportunities to improve code structure and testing practices to enhance maintainability and robustness.

go/cmd/gitter/gitter.go

go/cmd/gitter/repository.go

go/cmd/gitter/gitter.go

go/cmd/gitter/gitter_test.go

go/cmd/gitter/repository.go

go/cmd/gitter/repository_test.go

Ly-Joey · 2026-03-10T03:21:20Z

/gemini review

gemini-code-assist

Code Review

The pull request introduces a new /affected-commits API endpoint to the gitter service, enabling clients to query for commits affected by specific events (introduced, fixed, last_affected, limit) within a Git repository. This includes implementing graph traversal logic (Affected and Limit methods) with support for cherry-pick detection, defining new Protobuf messages for the API response, and adding URL validation to both the new and existing /cache handlers. The Commit struct was updated to use Refs instead of Tags, and rootCommits were added to the Repository struct. Review comments highlighted several areas for improvement: optimizing regex compilation for URL validation in both cacheHandler and affectedCommitsHandler by compiling it once at the package level; enhancing test robustness and readability in TestAffectedCommitsHandler by using EventType constants instead of hardcoded strings; clarifying the Limit function's assumption about linear history when traversing commits by following only the first parent; and correcting a bug in the decodeSHA1 helper function in repository_test.go where fmt.Sprintf incorrectly pads with spaces instead of zeros, which would cause hex.DecodeString to fail.

go/cmd/gitter/gitter.go

go/cmd/gitter/gitter_test.go

go/cmd/gitter/repository.go

go/cmd/gitter/repository_test.go

tobyhawker · 2026-03-11T00:33:05Z

I'm not sure that the definition of "affected" is quite right:

A commit is affected when: from at least one introduced there is no path from the commit through a fix to the introduced

As phrased, I don't think it's made explicit that only commits where the introduced is an ancestor are affected. How does something like this sound (just a suggestion, please feel free to adjust)?

A commit is affected when: from at least one introduced that is an ancestor of the commit, there is no path between them that passes through a fixed.

another-rex

Nice!

go/cmd/gitter/gitter.go

another-rex · 2026-03-11T00:36:11Z

go/cmd/gitter/gitter.go

+	}
+	logger.InfoContext(ctx, "Received request: /affected-commits", slog.Any("introduced", introduced), slog.Any("fixed", fixed), slog.Any("last_affected", lastAffected), slog.Any("limit", limit), slog.Bool("cherrypickIntro", cherrypickIntro), slog.Bool("cherrypickFixed", cherrypickFixed))
+
+	semaphore <- struct{}{}


hmm originally I think this semaphore was meant to be inside the singleflight.

another-rex · 2026-03-11T00:44:39Z

go/cmd/gitter/gitter.go

+	repoDirName := getRepoDirName(url)
+	repoPath := filepath.Join(gitStorePath, repoDirName)
+
+	repoLock := GetRepoLock(url)
+	repoLock.RLock()
+	defer repoLock.RUnlock()


Ideally I think the repo lock can be attached to the repo itself, so you can't access the underlying repo without locking. Can we do something like this:

Make fetchRepo return a repo struct, which contains:

path to repo

*Repository (nil by default) and private

to the repo mutex in the global map

then Load can be a function on the repo struct, which will do the locking before loading the repository. Just an idea at the moment, you probably can play around with the types a bit to improve it.

go/cmd/gitter/repository.go

another-rex · 2026-03-11T00:48:49Z

go/cmd/gitter/repository.go

+	if cherrypickIntro {
+		introduced = r.expandByCherrypick(introduced)
+	}
+	if cherrypickFixed {


Should be be cherrypicking last-affected +1 commits?

I’m hesitant to do cherrypicking for lastAffected because it doesn't guarantee a single commit that contains the code fix like a fixed commit does.
Especially if the lastAffected commit is derived from a version tag, then all we can be sure is that a fix commit exists in (current version, next version].
We can always add this logic later if we have more stats that support it.

go/cmd/gitter/repository.go

another-rex · 2026-03-11T02:26:04Z

go/cmd/gitter/repository.go

+		}
+	}
+
+	keys := slices.Collect(maps.Keys(unique))


Does this need to do some sorting for tests to pass? Or is it fine.

I used cmpopts.SortSlices() in the test file to make sure tests pass deterministically.

michaelkedar

Looking good - some initial comments

michaelkedar · 2026-03-11T00:25:34Z

go/cmd/gitter/gitter.go

 	semaphore       chan struct{} // Request concurrency control
 )

+var validURLRegex = regexp.MustCompile(`^(https?|git|ssh)://`)


(I know this isn't new, but) do we actually support ssh? I don't think the gitter on GKE has any credentials to use with it

michaelkedar · 2026-03-11T00:25:57Z

go/cmd/gitter/gitter.go

 		// Check if url starts with protocols: http(s)://, git://, ssh://, (s)ftp://
-		if match, _ := regexp.MatchString("^(https?|git|ssh)://", url); !match {
+		if !validURLRegex.MatchString(url) {


nit: the comment says it supports ftp, but the regex does not (+ same on the comments in other endpoints)

michaelkedar · 2026-03-11T00:39:03Z

go/cmd/gitter/gitter.go

+	// I can't change singleflight's interface
+	repoAny, err, _ := gLoad.Do(repoPath, func() (any, error) {


not in this PR, but if we're using singleflight heavily, and the any typing is clumsy, I reckon we could make a small wrapper with generics

type Group[T any] struct { delegate singleflight.Group } func (g *Group[T]) Do(key string, fn func() (T, error)) (T, error, bool) { // or whatever's preferable // call delegate.Do with wrapped functions and do the casting here }

go/cmd/gitter/repository.go

michaelkedar · 2026-03-11T01:00:48Z

go/cmd/gitter/repository.go

+					// Remove from affected list if it was reached via a previous non-fixed path.
+					delete(affectedFromIntro, unaffected)


Do you not have to check if any of the commits here were introduced commits?

e.g. if it's like

I1 -> F1 -> I2 -> F2

If you do I2 first, then I1 would all the commits after F1 become unaffected?

I think that's fine, since affectedFromIntro is per loop right? Otherwise we'll have problems from fixes and introduced being in parallel as well.

michaelkedar · 2026-03-11T01:03:01Z

go/cmd/gitter/repository.go

+}
+
+// Affected returns a list of commits that are affected by the given introduced, fixed and last_affected events
+func (r *Repository) Affected(ctx context.Context, introStrs, fixedStrs, laStrs []string, cherrypickIntro, cherrypickFixed bool) []*Commit {


It might be useful to move some of these structs/functions to a go/internal/gitter, so that other binaries may reuse the logic if they want to bypass the gitter service (e.g. for a datafix-type thing).

michaelkedar · 2026-03-11T01:05:35Z

go/cmd/gitter/repository.go

+}
+
+// Between walks and returns the commits that are strictly between introduced (inclusive) and limit (exclusive)
+func (r *Repository) Limit(ctx context.Context, introStrs, limitStrs []string) []*Commit {


do we want cherrypick detection here?

michaelkedar · 2026-03-11T02:13:33Z

go/cmd/gitter/repository_test.go

+}
+
+// Testing cases with introduced and fixed only.
+func TestAffected_Introduced_Fixed(t *testing.T) {


Can we add some tests for the edge cases when introduced, fixed and/or last_affected are equal to each other.
(and I suspect the logic may need to be adjusted to account for these)

Also tests for introduced = 0

michaelkedar · 2026-03-11T02:28:17Z

go/cmd/gitter/pb/repository/repository.proto

+message AffectedCommit {
+    bytes hash = 1;
+    repeated string refs = 2;
+}
+
+message AffectedCommitsResponse {
+    repeated AffectedCommit commits = 1;
+}


we probably want to move refs to the top level response (as a mapping of ref -> hash)

tobyhawker · 2026-03-11T02:08:08Z

go/cmd/gitter/repository.go

+	return hashes
+}
+
+// Affected returns a list of commits that are affected by the given introduced, fixed and last_affected events


Is it worth restating our definition of "affected" here in the code, where we're more likely to see it (and so help the reader understand why the code does what it does)?

Another point that might make sense to recap is just before the loop over the "introduced" commits.

another-rex

Oh btw for the bfs and the queue, did you know about: https://pkg.go.dev/container/list

Removed isIntroduced arg from parseHashes since it's unnecessary.

gemini-code-assist bot reviewed Mar 9, 2026

View reviewed changes

Ly-Joey added 16 commits March 10, 2026 02:19

Add back logic split from the big gitter PR

92f0399

making intro=0 work

3912e92

Add a sort to the cherrypick test because maps

e8d20bd

New affected commits logic + individual cherrypick

671bd97

lint

32d12ef

new new logic + update tests

4360bcc

🍒⛏️

9e5da52

lint

cb5c021

lint :0)

be723fe

Fix a test

ad67fc1

Protobuf for response. Include branches as well.

e53c6f2

Fix lint

f751012

Address gemini review

1c610ad

Change naming tag->ref. Update test.

228ec5f

Don't do maps.Clone everytime

19951ae

Put duplicated hash parsing into cleaner function

97b9e25

Ly-Joey force-pushed the feat-gitter-affected-commits branch from 4227a68 to 97b9e25 Compare March 10, 2026 02:19

Ly-Joey added 2 commits March 10, 2026 03:01

Use cmp.Options to simply prettify and sorting

354351e

lint

566a815

gemini-code-assist bot reviewed Mar 10, 2026

View reviewed changes

go/cmd/gitter/gitter.go Outdated Show resolved Hide resolved

go/cmd/gitter/gitter_test.go Show resolved Hide resolved

go/cmd/gitter/repository.go Outdated Show resolved Hide resolved

go/cmd/gitter/repository_test.go Outdated Show resolved Hide resolved

Gemini review changes

c411313

Ly-Joey marked this pull request as ready for review March 10, 2026 04:59

Fix how we reset the global vars for tests

2aba9ca

Ly-Joey requested review from another-rex, cuixq, jess-lowe and michaelkedar March 10, 2026 22:23

Ly-Joey requested a review from tobyhawker March 10, 2026 22:23

robot wants better tests, robot gets better tests

dc52dff

another-rex requested changes Mar 11, 2026

View reviewed changes

another-rex reviewed Mar 11, 2026

View reviewed changes

michaelkedar reviewed Mar 11, 2026

View reviewed changes

address comments on comments

38e2353

tobyhawker reviewed Mar 11, 2026

View reviewed changes

another-rex reviewed Mar 11, 2026

View reviewed changes

Ly-Joey added 5 commits March 11, 2026 05:23

Proto-ify events and split it into SeparatedEvents

616046e

clean up some unnecessary ifs

98c69c9

add comment on the cmp.Option used in tests

271f737

Array goes zoom 💨

3631dbb

Don't add intro=0 multiple times, removed arg

7b58e42

Removed isIntroduced arg from parseHashes since it's unnecessary.

		// I can't change singleflight's interface
		repoAny, err, _ := gLoad.Do(repoPath, func() (any, error) {

		// Remove from affected list if it was reached via a previous non-fixed path.
		delete(affectedFromIntro, unaffected)

Conversation

Ly-Joey commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Key changes

Uh oh!

Ly-Joey commented Mar 9, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ly-Joey commented Mar 10, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tobyhawker commented Mar 11, 2026

Uh oh!

another-rex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelkedar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

another-rex left a comment

Ly-Joey commented Mar 5, 2026 •

edited

Loading