bump direct go dependencies#2709
Conversation
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The dependency bumps look correct. The filepath.EvalSymlinks fix in pkg/fsx/vcs.go is sound: since go-billy v5.9.0 already returns a symlink-resolved path from worktree.Filesystem.Root(), both m.repoRoot and absPath are consistently resolved, and the HasPrefix check works correctly on macOS. go.mod/go.sum are self-consistent.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
Reviewed the dependency bumps (go-git/v5 v5.19.0, openai-go/v3 v3.35.0) and the symlink-resolution fix in pkg/fsx/vcs.go.
Fix correctness: The filepath.EvalSymlinks(absPath) addition is correct and well-targeted. Since go-billy/v5 v5.9.0 now returns a symlink-resolved path from worktree.Filesystem.Root(), applying the same resolution to absPath before the strings.HasPrefix check is the right approach. The m.repoRoot field is populated directly from Root() at construction time, so both sides of the comparison are consistently resolved.
One minor note (below the comment threshold): The EvalSymlinks error is silently discarded — if it fails for a transient reason on a valid file, the unresolved absPath would be compared against the resolved m.repoRoot, potentially causing ShouldIgnore to return false for a file that should be checked. In practice this is very low risk (the file exists and was just seen by the caller), but propagating or logging the error would make failures more visible.
Dependency changes: go.mod and go.sum look mechanically correct — hashes updated consistently with the bumps.
dgageot
force-pushed
the
board/82860f53d9d3f864
branch
from
36187ff to
b9bac90
Compare
Two follow-up changes were necessary: - Resolve symlinks on the absolute path in VCSMatcher.ShouldIgnore so it matches m.repoRoot, which go-billy v5.9.0 (pulled in by go-git v5.19.0) now returns with symlinks resolved (e.g. /private/var/... instead of /var/... on macOS). - Pin the transitive github.com/cyphar/filepath-securejoin back to v0.4.1 via a replace directive (and add it to gomoddirectives' replace-allow-list). Starting with v0.5.0 it is dual-licensed BSD-3-Clause + MPL-2.0, which the CI license check does not allow. Assisted-By: docker-agent
Assisted-By: docker-agent
dgageot
force-pushed
the
board/82860f53d9d3f864
branch
from
b9bac90 to
7ca7c4e
Compare
2 participants
Notes on the go-git bump
Two follow-up changes were necessary:
go-billy/v5v5.9.0 (pulled in bygo-git/v5v5.19.0) now resolves symlinks inworktree.Filesystem.Root()(e.g. returns/private/var/...instead of/var/...on macOS). This causedVCSMatcher.ShouldIgnoreto fail itsstrings.HasPrefix(absPath, m.repoRoot)check, breakingTestFilePickerShowIgnoredInGitRepo. Fix: also callfilepath.EvalSymlinksonabsPathinpkg/fsx/vcs.go.The transitive dependency
github.com/cyphar/filepath-securejoinis dual-licensed BSD-3-Clause + MPL-2.0 starting with v0.5.0, which the CI license check (go-licenses) does not allow. Fix: pin it to v0.4.1 via areplacedirective ingo.mod(and add it togomoddirectives'replace-allow-list).Assisted-By: docker-agent