Skip to content

Commit 0e9cd7d

Browse files
committed
feat/batch-changes: polish error message and tests in codingAgent port
- Replace internal-type-leaking error message with user-facing wording in run_steps.go (codingAgent ModelProviderURL check). - Align temp-file error wording in writeRunScriptFile with the rest of the file ('writing to temporary file'). - Trim redundant assertions in TestRedactSensitiveEnv and TestRedactSensitiveArgs. - Tighten TestParseBatchSpec_v3_codingAgentRequiresImage to assert only the actual error path. - Extract v3SpecWith helper to deduplicate YAML scaffold across batch_spec_test.go cases.
1 parent c108b01 commit 0e9cd7d

3 files changed

Lines changed: 27 additions & 66 deletions

File tree

internal/batches/executor/run_steps.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,7 @@ func executeSingleStep(
281281
)
282282
if step.CodingAgent != nil {
283283
if opts.Task.ModelProviderURL == "" {
284-
err = errors.New("codingAgent step requires WorkspacesExecutionInput.ModelProviderURL to be set")
284+
err = errors.New("codingAgent step requires a model-provider URL")
285285
opts.UI.StepPreparingFailed(stepIdx+1, err)
286286
return bytes.Buffer{}, bytes.Buffer{}, err
287287
}
@@ -670,7 +670,7 @@ func writeRunScriptFile(tempDir, script string) (string, func(), error) {
670670

671671
if _, err := runScriptFile.WriteString(script); err != nil {
672672
cleanup()
673-
return "", nil, errors.Wrap(err, "writing temporary file")
673+
return "", nil, errors.Wrap(err, "writing to temporary file")
674674
}
675675
if err := runScriptFile.Close(); err != nil {
676676
cleanup()

internal/batches/executor/run_steps_test.go

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package executor
22

33
import (
44
"slices"
5-
"strings"
65
"testing"
76

87
codingagenttypes "github.com/sourcegraph/sourcegraph/lib/batches/codingagent/types"
@@ -11,16 +10,12 @@ import (
1110
func TestRedactSensitiveEnv(t *testing.T) {
1211
in := map[string]string{
1312
codingagenttypes.ModelProviderTokenEnvVar: "tok-abc",
14-
codingagenttypes.JobIDEnvVar: "job-123",
1513
"PATH": "/bin",
1614
}
1715
out := redactSensitiveEnv(in)
1816
if got := out[codingagenttypes.ModelProviderTokenEnvVar]; got != redactedPlaceholder {
1917
t.Errorf("token: got %q want %q", got, redactedPlaceholder)
2018
}
21-
if got := out[codingagenttypes.JobIDEnvVar]; got != "job-123" {
22-
t.Errorf("job id should not be redacted: got %q", got)
23-
}
2419
if got := out["PATH"]; got != "/bin" {
2520
t.Errorf("PATH should not be redacted: got %q", got)
2621
}
@@ -47,9 +42,6 @@ func TestRedactSensitiveArgs(t *testing.T) {
4742
if !slices.Contains(out, codingagenttypes.JobIDEnvVar+"=job-123") {
4843
t.Errorf("job id should pass through: %v", out)
4944
}
50-
if strings.Contains(strings.Join(out, " "), "tok-abc") {
51-
t.Errorf("token leaked anywhere in joined args: %v", out)
52-
}
5345
}
5446

5547
func TestForwardCodingAgentEnv(t *testing.T) {

lib/batches/batch_spec_test.go

Lines changed: 25 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,31 @@ package batches
22

33
import (
44
"encoding/json"
5+
"fmt"
56
"strings"
67
"testing"
78
)
89

10+
// v3SpecWith wraps stepsYAML in the minimum scaffold needed for ParseBatchSpec
11+
// to accept a v3 spec.
12+
func v3SpecWith(stepsYAML string) []byte {
13+
return []byte(fmt.Sprintf(`
14+
version: 3
15+
name: test
16+
description: test
17+
on:
18+
- repository: github.com/sourcegraph/sourcegraph
19+
steps:
20+
%s
21+
changesetTemplate:
22+
title: test
23+
body: test
24+
branch: test
25+
commit:
26+
message: test
27+
`, stepsYAML))
28+
}
29+
930
func TestStep_MarshalJSON_canonicalizesImageToContainer(t *testing.T) {
1031
v3FromImage, err := json.Marshal(Step{Image: "alpine:3", Run: "echo hi"})
1132
if err != nil {
@@ -31,23 +52,7 @@ func TestStep_MarshalJSON_canonicalizesImageToContainer(t *testing.T) {
3152
}
3253

3354
func TestParseBatchSpec_v3_imageMirroredToContainer(t *testing.T) {
34-
spec := []byte(`
35-
version: 3
36-
name: test
37-
description: test
38-
on:
39-
- repository: github.com/sourcegraph/sourcegraph
40-
steps:
41-
- run: echo hi
42-
image: alpine:3
43-
changesetTemplate:
44-
title: test
45-
body: test
46-
branch: test
47-
commit:
48-
message: test
49-
`)
50-
got, err := ParseBatchSpec(spec)
55+
got, err := ParseBatchSpec(v3SpecWith(" - run: echo hi\n image: alpine:3"))
5156
if err != nil {
5257
t.Fatalf("ParseBatchSpec failed: %v", err)
5358
}
@@ -63,53 +68,17 @@ changesetTemplate:
6368
}
6469

6570
func TestParseBatchSpec_v3_codingAgentRequiresImage(t *testing.T) {
66-
spec := []byte(`
67-
version: 3
68-
name: test
69-
description: test
70-
on:
71-
- repository: github.com/sourcegraph/sourcegraph
72-
steps:
73-
- codingAgent:
74-
type: codex
75-
prompt: do the thing
76-
changesetTemplate:
77-
title: test
78-
body: test
79-
branch: test
80-
commit:
81-
message: test
82-
`)
83-
_, err := ParseBatchSpec(spec)
71+
_, err := ParseBatchSpec(v3SpecWith(" - codingAgent:\n type: codex\n prompt: do the thing"))
8472
if err == nil {
8573
t.Fatal("expected validation error, got nil")
8674
}
87-
if !strings.Contains(err.Error(), "requires an image") &&
88-
!strings.Contains(err.Error(), "Must validate") {
75+
if !strings.Contains(err.Error(), "requires an image") {
8976
t.Errorf("error should mention missing image, got: %v", err)
9077
}
9178
}
9279

9380
func TestParseBatchSpec_v3_codingAgentStep(t *testing.T) {
94-
spec := []byte(`
95-
version: 3
96-
name: test
97-
description: test
98-
on:
99-
- repository: github.com/sourcegraph/sourcegraph
100-
steps:
101-
- codingAgent:
102-
type: codex
103-
prompt: do the thing
104-
image: alpine:3
105-
changesetTemplate:
106-
title: test
107-
body: test
108-
branch: test
109-
commit:
110-
message: test
111-
`)
112-
got, err := ParseBatchSpec(spec)
81+
got, err := ParseBatchSpec(v3SpecWith(" - codingAgent:\n type: codex\n prompt: do the thing\n image: alpine:3"))
11382
if err != nil {
11483
t.Fatalf("ParseBatchSpec failed: %v", err)
11584
}

0 commit comments

Comments
 (0)