Skip to content

Commit 092cc99

Browse files
morlutomorluto
authored
Merge pull request #273 from morluto/fix/critical-bugs-audit
fix: critical bug audit - 5 root-cause fixes with regression tests
2 parents ab985ac + 7279de2 commit 092cc99

9 files changed

Lines changed: 315 additions & 35 deletions

File tree

‎internal/acquire/acquire.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -387,20 +387,28 @@ func (m *Manager) resolveDefaultBranch(ctx context.Context, mirrorPath string) (
387387
return branch, nil
388388
}
389389
}
390+
} else if ctx.Err() != nil {
391+
// Propagate cancellation instead of masking it as ErrNoCommit.
392+
return "", ctx.Err()
390393
}
391394
out, err = m.git(ctx, mirrorPath, "symbolic-ref", "--quiet", "--short", "HEAD")
392395
if err == nil {
393396
branch := strings.TrimSpace(out)
394397
if branch != "" && branch != "HEAD" {
395398
return branch, nil
396399
}
400+
} else if ctx.Err() != nil {
401+
return "", ctx.Err()
397402
}
398403
return "", ErrNoCommit
399404
}
400405

401406
func (m *Manager) resolveCommit(ctx context.Context, mirrorPath, branch string) (string, error) {
402407
out, err := m.git(ctx, mirrorPath, "rev-parse", "--verify", "refs/heads/"+branch+"^{commit}")
403408
if err != nil {
409+
if ctx.Err() != nil {
410+
return "", ctx.Err()
411+
}
404412
return "", fmt.Errorf("resolve commit for %q: %w", branch, ErrNoCommit)
405413
}
406414
return strings.TrimSpace(out), nil

‎internal/discovery/search.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,16 @@ func (p *SearchPartitioner) partition(ctx context.Context, baseQuery string, sta
137137
}
138138

139139
mid := splitMid(start, end)
140+
// Guard against zero-progress splits: for a 1-second window,
141+
// splitMid returns start, which would recurse into two duplicate
142+
// zero-width unsplittable windows. Emit as unsplittable instead.
143+
if !mid.After(start) {
144+
*out = append(*out, Window{
145+
Query: query, Qualifier: qual, Start: start, End: end,
146+
Total: resp.Total, Incomplete: resp.Incomplete, Unsplittable: true,
147+
})
148+
return nil
149+
}
140150
if err := p.partition(ctx, baseQuery, start, mid, qual, out); err != nil {
141151
return err
142152
}

‎internal/discovery/search_test.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,3 +219,36 @@ func TestRefreshOverlapCheckpoint(t *testing.T) {
219219
t.Fatalf("expected checkpoint %v, got %v", end, cp)
220220
}
221221
}
222+
223+
// TestPartitionZeroProgressSplit proves that a 1-second overflowing window
224+
// emits exactly one unsplittable window, not two duplicate zero-width ones.
225+
func TestPartitionZeroProgressSplit(t *testing.T) {
226+
calls := 0
227+
p := &SearchPartitioner{
228+
Searcher: &fakeSearcher{
229+
fn: func(ctx context.Context, query string) (SearchResponse, error) {
230+
calls++
231+
return SearchResponse{Total: 1001}, nil
232+
},
233+
},
234+
}
235+
// A 1-second window where the result exceeds the limit.
236+
// Before the fix: splitMid returns mid==start, producing two
237+
// duplicate unsplittable windows [start,start] and [end,end].
238+
// After the fix: exactly one unsplittable window [start,end].
239+
start := time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC)
240+
end := start.Add(time.Second)
241+
windows, err := p.Partition(context.Background(), "language:go", start, end, Created)
242+
if err != nil {
243+
t.Fatalf("Partition: %v", err)
244+
}
245+
if len(windows) != 1 {
246+
t.Fatalf("expected 1 window for 1-second overflowing range, got %d: %+v", len(windows), windows)
247+
}
248+
if !windows[0].Unsplittable {
249+
t.Fatal("expected window to be unsplittable")
250+
}
251+
if windows[0].Total != 1001 {
252+
t.Fatalf("expected total 1001, got %d", windows[0].Total)
253+
}
254+
}

‎internal/github/pull_request_workflows.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ func (c *Client) feedbackIssueComments(ctx context.Context, owner, repo string,
181181
}
182182
page = resp.NextPage
183183
}
184-
return items, FeedbackCoverage{Fetched: len(items), Total: len(items) + 1, Reason: "item_limit_reached"}, nil
184+
return items, FeedbackCoverage{Fetched: len(items), Total: 0, Reason: "item_limit_reached"}, nil
185185
}
186186

187187
func (c *Client) feedbackReviews(ctx context.Context, owner, repo string, number, limit int, budget *RequestBudget) ([]FeedbackReview, FeedbackCoverage, error) {
@@ -203,7 +203,7 @@ func (c *Client) feedbackReviews(ctx context.Context, owner, repo string, number
203203
}
204204
page = resp.NextPage
205205
}
206-
return items, FeedbackCoverage{Fetched: len(items), Total: len(items) + 1, Reason: "item_limit_reached"}, nil
206+
return items, FeedbackCoverage{Fetched: len(items), Total: 0, Reason: "item_limit_reached"}, nil
207207
}
208208

209209
func (c *Client) feedbackInlineComments(ctx context.Context, owner, repo string, number, limit int, budget *RequestBudget) ([]FeedbackComment, FeedbackCoverage, error) {
@@ -230,7 +230,7 @@ func (c *Client) feedbackInlineComments(ctx context.Context, owner, repo string,
230230
}
231231
page = resp.NextPage
232232
}
233-
return items, FeedbackCoverage{Fetched: len(items), Total: len(items) + 1, Reason: "item_limit_reached"}, nil
233+
return items, FeedbackCoverage{Fetched: len(items), Total: 0, Reason: "item_limit_reached"}, nil
234234
}
235235

236236
const pullRequestFeedbackThreadsQuery = `query PullRequestFeedback($owner: String!, $repo: String!, $number: Int!, $first: Int!, $after: String) {

‎internal/workspace/adopt.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,11 @@ func (m *Manager) gitPath(ctx context.Context, worktree, arg string) (string, er
163163
func (m *Manager) resolvePathRef(ctx context.Context, path, ref string) (string, error) {
164164
out, err := m.git(ctx, path, "rev-parse", "--verify", "--end-of-options", strings.TrimSpace(ref)+"^{commit}")
165165
if err != nil {
166+
// Preserve context cancellation so callers can distinguish a
167+
// genuine "not found" from a cancelled or timed-out operation.
168+
if ctx.Err() != nil {
169+
return "", ctx.Err()
170+
}
166171
return "", ErrNotFound
167172
}
168173
return strings.TrimSpace(out), nil

‎internal/workspace/merge.go‎

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,14 +38,67 @@ func (m *Manager) checkMerge(ctx context.Context, path, baseOID, headOID string)
3838
return MergeCheck{}, err
3939
}
4040
mergeBase = strings.TrimSpace(mergeBase)
41+
// The three-tree form reads only existing objects. Its output includes
42+
// arbitrary file content, so recognize only a conflict heading followed by
43+
// Git's object metadata rather than matching a marker or message anywhere
44+
// in the diff.
4145
out, err := m.git(ctx, path, "merge-tree", mergeBase, baseOID, headOID)
4246
if err != nil {
4347
return MergeCheck{}, err
4448
}
45-
conflicted := strings.Contains(out, "changed in both") || strings.Contains(out, "<<<<<<<") || strings.Contains(out, "CONFLICT")
49+
conflicted := legacyMergeTreeConflicted(out)
4650
summary := "revisions merge cleanly"
4751
if conflicted {
4852
summary = "revisions have merge conflicts"
4953
}
5054
return MergeCheck{MergeBase: mergeBase, Conflicted: conflicted, Summary: summary}, nil
5155
}
56+
57+
func legacyMergeTreeConflicted(out string) bool {
58+
lines := strings.Split(out, "\n")
59+
for i, line := range lines {
60+
switch line {
61+
case "changed in both", "added in both", "removed in local", "removed in remote":
62+
if hasMergeTreeObjectMetadata(lines[i+1:]) {
63+
return true
64+
}
65+
}
66+
}
67+
return false
68+
}
69+
70+
func hasMergeTreeObjectMetadata(lines []string) bool {
71+
metadataLines := 0
72+
for _, line := range lines {
73+
fields := strings.Fields(line)
74+
if len(fields) != 4 || (fields[0] != "base" && fields[0] != "our" && fields[0] != "their") || !isFileMode(fields[1]) || !isObjectID(fields[2]) {
75+
break
76+
}
77+
metadataLines++
78+
}
79+
return metadataLines >= 2
80+
}
81+
82+
func isFileMode(value string) bool {
83+
if len(value) != 6 {
84+
return false
85+
}
86+
for _, char := range value {
87+
if char < '0' || char > '7' {
88+
return false
89+
}
90+
}
91+
return true
92+
}
93+
94+
func isObjectID(value string) bool {
95+
if len(value) != 40 && len(value) != 64 {
96+
return false
97+
}
98+
for _, char := range value {
99+
if (char < '0' || char > '9') && (char < 'a' || char > 'f') {
100+
return false
101+
}
102+
}
103+
return true
104+
}

‎internal/workspace/merge_test.go‎

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
package workspace
2+
3+
import (
4+
"context"
5+
"path/filepath"
6+
"strings"
7+
"testing"
8+
)
9+
10+
func TestManagerCheckMergeUsesAlreadyFetchedRevisionsWithoutMutation(t *testing.T) {
11+
t.Parallel()
12+
ctx := context.Background()
13+
remote, baseSHA, candidateSHA := setupRemote(t)
14+
mgr := newManager(t)
15+
if err := mgr.Clone(ctx, remote, "origin"); err != nil {
16+
t.Fatal(err)
17+
}
18+
path := mgr.mirrors["origin"].path
19+
before := strings.TrimSpace(runGit(t, path, "show-ref"))
20+
beforeObjects := strings.TrimSpace(runGit(t, path, "count-objects", "-v"))
21+
result, err := mgr.CheckMerge(ctx, path, baseSHA, candidateSHA)
22+
if err != nil {
23+
t.Fatal(err)
24+
}
25+
if result.Conflicted || result.MergeBase != baseSHA {
26+
t.Fatalf("unexpected merge result: %+v", result)
27+
}
28+
after := strings.TrimSpace(runGit(t, path, "show-ref"))
29+
if before != after {
30+
t.Fatalf("merge check changed refs\nbefore: %s\nafter: %s", before, after)
31+
}
32+
afterObjects := strings.TrimSpace(runGit(t, path, "count-objects", "-v"))
33+
if beforeObjects != afterObjects {
34+
t.Fatalf("merge check changed object store\nbefore: %s\nafter: %s", beforeObjects, afterObjects)
35+
}
36+
}
37+
38+
// TestManagerCheckMergeNoFalsePositiveOnConflictMarkerInContent proves that
39+
// a file containing conflict markers in its content does not trigger a
40+
// false-positive conflict detection when the merge is actually clean.
41+
func TestManagerCheckMergeNoFalsePositiveOnConflictMarkerInContent(t *testing.T) {
42+
t.Parallel()
43+
ctx := context.Background()
44+
45+
dir := t.TempDir()
46+
remote := filepath.Join(dir, "remote.git")
47+
runGit(t, "", "init", "--bare", remote)
48+
49+
src := filepath.Join(dir, "src")
50+
runGit(t, "", "clone", remote, src)
51+
52+
writeFile(t, filepath.Join(src, "file.txt"), "hello")
53+
runGit(t, src, "add", ".")
54+
runGit(t, src, "-c", "user.email=test@example.com", "-c", "user.name=Test", "commit", "-m", "base")
55+
runGit(t, src, "push", "origin", "master")
56+
57+
marker := strings.Repeat("<", 7)
58+
runGit(t, src, "checkout", "-b", "feature")
59+
writeFile(t, filepath.Join(src, "file.txt"), marker+" HEAD\nworld\n=======\nuniverse\n"+strings.Repeat(">", 7)+" branch\n")
60+
runGit(t, src, "add", ".")
61+
runGit(t, src, "-c", "user.email=test@example.com", "-c", "user.name=Test", "commit", "-m", "feature adds conflict markers in content")
62+
runGit(t, src, "push", "origin", "feature")
63+
64+
runGit(t, src, "checkout", "master")
65+
writeFile(t, filepath.Join(src, "other.txt"), "different change")
66+
runGit(t, src, "add", ".")
67+
runGit(t, src, "-c", "user.email=test@example.com", "-c", "user.name=Test", "commit", "-m", "master adds other file")
68+
runGit(t, src, "push", "origin", "master")
69+
70+
baseSHA := strings.TrimSpace(runGit(t, src, "rev-parse", "master"))
71+
candidateSHA := strings.TrimSpace(runGit(t, src, "rev-parse", "feature"))
72+
mgr := newManager(t)
73+
if err := mgr.Clone(ctx, remote, "origin"); err != nil {
74+
t.Fatal(err)
75+
}
76+
77+
result, err := mgr.CheckMerge(ctx, mgr.mirrors["origin"].path, baseSHA, candidateSHA)
78+
if err != nil {
79+
t.Fatal(err)
80+
}
81+
if result.Conflicted {
82+
t.Fatalf("false positive: merge reported conflict but branches merge cleanly. Result: %+v", result)
83+
}
84+
}
85+
86+
func TestManagerCheckMergeDetectsAddAddConflict(t *testing.T) {
87+
t.Parallel()
88+
ctx := context.Background()
89+
90+
dir := t.TempDir()
91+
remote := filepath.Join(dir, "remote.git")
92+
runGit(t, "", "init", "--bare", remote)
93+
94+
src := filepath.Join(dir, "src")
95+
runGit(t, "", "clone", remote, src)
96+
writeFile(t, filepath.Join(src, "base.txt"), "base")
97+
runGit(t, src, "add", ".")
98+
runGit(t, src, "-c", "user.email=test@example.com", "-c", "user.name=Test", "commit", "-m", "base")
99+
runGit(t, src, "push", "origin", "master")
100+
101+
runGit(t, src, "checkout", "-b", "feature")
102+
writeFile(t, filepath.Join(src, "same.txt"), "feature")
103+
runGit(t, src, "add", ".")
104+
runGit(t, src, "-c", "user.email=test@example.com", "-c", "user.name=Test", "commit", "-m", "feature adds same path")
105+
runGit(t, src, "push", "origin", "feature")
106+
107+
runGit(t, src, "checkout", "master")
108+
writeFile(t, filepath.Join(src, "same.txt"), "master")
109+
runGit(t, src, "add", ".")
110+
runGit(t, src, "-c", "user.email=test@example.com", "-c", "user.name=Test", "commit", "-m", "master adds same path")
111+
runGit(t, src, "push", "origin", "master")
112+
113+
baseSHA := strings.TrimSpace(runGit(t, src, "rev-parse", "master"))
114+
candidateSHA := strings.TrimSpace(runGit(t, src, "rev-parse", "feature"))
115+
mgr := newManager(t)
116+
if err := mgr.Clone(ctx, remote, "origin"); err != nil {
117+
t.Fatal(err)
118+
}
119+
result, err := mgr.CheckMerge(ctx, mgr.mirrors["origin"].path, baseSHA, candidateSHA)
120+
if err != nil {
121+
t.Fatal(err)
122+
}
123+
if !result.Conflicted {
124+
t.Fatalf("add/add conflict reported clean: %+v", result)
125+
}
126+
}

‎internal/workspace/workspace.go‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -353,13 +353,20 @@ func (m *Manager) Create(ctx context.Context, mirrorName, baseRef, candidateRef,
353353
return nil, fmt.Errorf("create workspaces dir: %w", err)
354354
}
355355
path := filepath.Join(workDir, name)
356-
if _, err := os.Stat(path); err == nil {
357-
return nil, ErrExists
358-
} else if !os.IsNotExist(err) {
359-
return nil, fmt.Errorf("stat workspace path: %w", err)
356+
// Atomically reserve the final path before asking Git to populate it. This
357+
// both serializes concurrent creators and proves that any later cleanup is
358+
// limited to a directory created by this invocation.
359+
if err := os.Mkdir(path, 0755); err != nil {
360+
if errors.Is(err, os.ErrExist) {
361+
return nil, ErrExists
362+
}
363+
return nil, fmt.Errorf("reserve workspace path: %w", err)
360364
}
361365

362366
if _, err := m.git(ctx, mi.path, "worktree", "add", "--detach", path, candidateSHA); err != nil {
367+
// git worktree add may create a partial directory before
368+
// failing. Clean it up so it does not leak on disk.
369+
_ = os.RemoveAll(path)
363370
return nil, fmt.Errorf("create worktree: %w", err)
364371
}
365372

@@ -385,10 +392,6 @@ func (m *Manager) Create(ctx context.Context, mirrorName, baseRef, candidateRef,
385392
}
386393

387394
m.mu.Lock()
388-
if _, ok := m.workspaces[name]; ok {
389-
m.mu.Unlock()
390-
return nil, ErrExists
391-
}
392395
m.workspaces[name] = ws
393396
m.mu.Unlock()
394397
return ws, nil

0 commit comments

Comments
 (0)