Skip to content

Commit 7612fb3

Browse files
taras-yemetstaras-yemets
committed
feat/batch-changes: address sourcegraph#12503 review for v1 codingAgent port
Apply review feedback from sourcegraph/sourcegraph#12503 (where the upstream codingAgent step lives) to the src-cli v1 port. The same lib/batches/codingagent surface is shared, so the same review applies here. Changes: - lib/batches/codex: switch to upstream-recommended curl-based install (codex/claude code both recommend it), pin codex CLI version, drop dependency on the agent CLI being pre-baked in the run image. ImageRequirements is now [curl, tar] so misconfigured images fail fast at step start. - lib/batches/codingagent/types: add Agent.InstallScript and InstallDir so each agent owns its own pinned install at a Sourcegraph-controlled path, ignoring whatever the user image ships. - lib/batches: add Step.MarshalJSON to canonicalize v3 image: into container: on the wire. Without this the prep-side cache key (which marshals Step to JSON) includes image while src-cli's executor side does not, producing silent cache misses on every v3 spec. - lib/batches: reject codingAgent + run in the same step at parse time instead of silently picking one. - internal/batches/executor: extract forwardCodingAgentEnv with a docstring spelling out what's forwarded and why. - Tests for forwardCodingAgentEnv and the v3 image/codingAgent parse paths. Test Plan: - go test ./internal/batches/executor/ (root module) - cd lib && go test ./batches/...
1 parent 1ef7e95 commit 7612fb3

8 files changed

Lines changed: 572 additions & 174 deletions

File tree

‎internal/batches/executor/run_steps.go‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -330,14 +330,7 @@ func executeSingleStep(
330330
// the server) into the user container so the agent CLI can talk to the
331331
// /model-provider/batches proxy.
332332
if step.CodingAgent != nil {
333-
for _, key := range []string{codingagenttypes.ModelProviderTokenEnvVar, codingagenttypes.JobIDEnvVar} {
334-
for _, e := range opts.GlobalEnv {
335-
if v, ok := strings.CutPrefix(e, key+"="); ok {
336-
env[key] = v
337-
break
338-
}
339-
}
340-
}
333+
forwardCodingAgentEnv(opts.GlobalEnv, env)
341334
}
342335

343336
opts.UI.StepPreparingSuccess(stepIdx + 1)
@@ -610,6 +603,22 @@ func createRunScriptFile(ctx context.Context, tempDir string, stepRun string, st
610603
return runScriptFile.Name(), runScript.String(), cleanup, nil
611604
}
612605

606+
// forwardCodingAgentEnv copies the model-provider auth env vars
607+
// (SRC_BATCHES_MODEL_PROVIDER_TOKEN, SRC_BATCHES_JOB_ID) from globalEnv into
608+
// stepEnv. These are placed on the v1 CliStep that runs `src batch exec` by
609+
// the Sourcegraph server; the agent CLI in the user container needs them to
610+
// reach the /.executors/model-provider/batches proxy.
611+
func forwardCodingAgentEnv(globalEnv []string, stepEnv map[string]string) {
612+
for _, key := range []string{codingagenttypes.ModelProviderTokenEnvVar, codingagenttypes.JobIDEnvVar} {
613+
for _, e := range globalEnv {
614+
if v, ok := strings.CutPrefix(e, key+"="); ok {
615+
stepEnv[key] = v
616+
break
617+
}
618+
}
619+
}
620+
}
621+
613622
// writeRunScriptFile writes a pre-rendered run script (e.g. a codingAgent
614623
// step desugared by the codingagent registry) verbatim to a temp file, with
615624
// the same permission semantics as createRunScriptFile. Unlike that helper,

‎internal/batches/executor/run_steps_test.go‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
package executor
2+
3+
import (
4+
"testing"
5+
6+
codingagenttypes "github.com/sourcegraph/sourcegraph/lib/batches/codingagent/types"
7+
)
8+
9+
// TestForwardCodingAgentEnv verifies that the model-provider auth env vars
10+
// placed on the v1 CliStep by the Sourcegraph server are forwarded into the
11+
// user container env for codingAgent steps.
12+
func TestForwardCodingAgentEnv(t *testing.T) {
13+
cases := []struct {
14+
name string
15+
globalEnv []string
16+
stepEnv map[string]string
17+
want map[string]string
18+
}{
19+
{
20+
name: "forwards both vars",
21+
globalEnv: []string{
22+
"PATH=/bin",
23+
codingagenttypes.ModelProviderTokenEnvVar + "=tok-abc",
24+
codingagenttypes.JobIDEnvVar + "=job-123",
25+
},
26+
stepEnv: map[string]string{},
27+
want: map[string]string{
28+
codingagenttypes.ModelProviderTokenEnvVar: "tok-abc",
29+
codingagenttypes.JobIDEnvVar: "job-123",
30+
},
31+
},
32+
{
33+
name: "forwards only what is set",
34+
globalEnv: []string{
35+
codingagenttypes.JobIDEnvVar + "=job-456",
36+
},
37+
stepEnv: map[string]string{},
38+
want: map[string]string{
39+
codingagenttypes.JobIDEnvVar: "job-456",
40+
},
41+
},
42+
{
43+
name: "preserves preexisting step env and overwrites on match",
44+
globalEnv: []string{
45+
codingagenttypes.ModelProviderTokenEnvVar + "=from-global",
46+
},
47+
stepEnv: map[string]string{
48+
"OTHER": "x",
49+
codingagenttypes.ModelProviderTokenEnvVar: "from-step",
50+
},
51+
want: map[string]string{
52+
"OTHER": "x",
53+
codingagenttypes.ModelProviderTokenEnvVar: "from-global",
54+
},
55+
},
56+
{
57+
name: "no-op when env not present",
58+
globalEnv: []string{"PATH=/bin"},
59+
stepEnv: map[string]string{},
60+
want: map[string]string{},
61+
},
62+
}
63+
64+
for _, tc := range cases {
65+
t.Run(tc.name, func(t *testing.T) {
66+
forwardCodingAgentEnv(tc.globalEnv, tc.stepEnv)
67+
if len(tc.stepEnv) != len(tc.want) {
68+
t.Fatalf("len mismatch: got %d want %d (got=%v want=%v)", len(tc.stepEnv), len(tc.want), tc.stepEnv, tc.want)
69+
}
70+
for k, v := range tc.want {
71+
if got := tc.stepEnv[k]; got != v {
72+
t.Errorf("env[%q]: got %q want %q", k, got, v)
73+
}
74+
}
75+
})
76+
}
77+
}

‎lib/batches/batch_spec.go‎

Lines changed: 28 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package batches
22

33
import (
4+
"encoding/json"
45
"fmt"
56
"strings"
67

@@ -93,28 +94,43 @@ type Step struct {
9394
Run string `json:"run,omitempty" yaml:"run"`
9495
CodingAgent *CodingAgentStep `json:"codingAgent,omitempty" yaml:"codingAgent,omitempty"`
9596
Container string `json:"container,omitempty" yaml:"container"`
96-
Image string `json:"image,omitempty" yaml:"image,omitempty"`
97+
Image string `json:"image,omitempty" yaml:"image"`
9798
Env env.Environment `json:"env" yaml:"env"`
9899
Files map[string]string `json:"files,omitempty" yaml:"files,omitempty"`
99100
Outputs Outputs `json:"outputs,omitempty" yaml:"outputs,omitempty"`
100101
Mount []Mount `json:"mount,omitempty" yaml:"mount,omitempty"`
101102
If any `json:"if,omitempty" yaml:"if,omitempty"`
102103
}
103104

104-
// CodingAgentType identifies a registered coding-agent implementation.
105105
type CodingAgentType string
106106

107107
const (
108108
CodingAgentTypeCodex CodingAgentType = "codex"
109109
)
110110

111-
// CodingAgentStep is a v3-spec step that delegates the step's work to a
112-
// coding agent CLI invoked via the server-side model-provider proxy.
113111
type CodingAgentStep struct {
114112
Type CodingAgentType `json:"type,omitempty" yaml:"type"`
115113
Prompt string `json:"prompt,omitempty" yaml:"prompt"`
116114
}
117115

116+
// MarshalJSON canonicalizes the v3 `image:` field into `container:` on the
117+
// wire. Both fields exist on Step for ergonomic reasons (v3 specs use
118+
// `image:`, v1/v2 specs use `container:`), but src-cli's executor reads
119+
// `Container`. Without canonicalization, the prep-side cache key — computed
120+
// by JSON-marshaling Step — would include `image` while the executor side
121+
// (which round-trips through src-cli) would not, producing divergent keys
122+
// and silent cache misses for any v3 spec.
123+
func (s Step) MarshalJSON() ([]byte, error) {
124+
// Use an alias type to avoid infinite recursion through MarshalJSON.
125+
type stepAlias Step
126+
canon := stepAlias(s)
127+
if canon.Container == "" {
128+
canon.Container = canon.Image
129+
}
130+
canon.Image = ""
131+
return json.Marshal(canon)
132+
}
133+
118134
func (s *Step) IfCondition() string {
119135
switch v := s.If.(type) {
120136
case bool:
@@ -178,12 +194,12 @@ func parseBatchSpec(schema string, data []byte) (*BatchSpec, error) {
178194
}
179195

180196
if spec.Version == 3 {
181-
// Mirror v3 `image:` into `container:` so executor consumers that
182-
// read step.Container keep working.
197+
// Mirror v3 `image:` into `container:` so in-memory consumers that
198+
// read step.Container (e.g. the executor transform) keep working.
199+
// JSON serialization is canonicalized separately in Step.MarshalJSON
200+
// so prep-side cache hashing matches src-cli/executor serialization.
183201
for i := range spec.Steps {
184-
if spec.Steps[i].Image != "" {
185-
spec.Steps[i].Container = spec.Steps[i].Image
186-
}
202+
spec.Steps[i].Container = spec.Steps[i].Image
187203
}
188204
}
189205

@@ -207,6 +223,9 @@ func parseBatchSpec(schema string, data []byte) (*BatchSpec, error) {
207223
errs = errors.Append(errs, NewValidationError(errors.Newf("step %d files target path contains invalid characters", i+1)))
208224
}
209225
}
226+
if step.CodingAgent != nil && step.Run != "" {
227+
errs = errors.Append(errs, NewValidationError(errors.Newf("step %d: codingAgent and run cannot be combined in the same step", i+1)))
228+
}
210229
}
211230

212231
return &spec, errs

‎lib/batches/batch_spec_test.go‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
package batches
2+
3+
import (
4+
"testing"
5+
)
6+
7+
// TestParseBatchSpec_v3_imageMirroredToContainer ensures that in v3 specs the
8+
// step-level `image:` field is mirrored into Step.Container so executor
9+
// consumers that read step.Container keep working.
10+
func TestParseBatchSpec_v3_imageMirroredToContainer(t *testing.T) {
11+
spec := []byte(`
12+
version: 3
13+
name: test
14+
description: test
15+
on:
16+
- repository: github.com/sourcegraph/sourcegraph
17+
steps:
18+
- run: echo hi
19+
image: alpine:3
20+
changesetTemplate:
21+
title: test
22+
body: test
23+
branch: test
24+
commit:
25+
message: test
26+
`)
27+
got, err := ParseBatchSpec(spec)
28+
if err != nil {
29+
t.Fatalf("ParseBatchSpec failed: %v", err)
30+
}
31+
if len(got.Steps) != 1 {
32+
t.Fatalf("expected 1 step, got %d", len(got.Steps))
33+
}
34+
if got.Steps[0].Image != "alpine:3" {
35+
t.Errorf("Step.Image: got %q want %q", got.Steps[0].Image, "alpine:3")
36+
}
37+
if got.Steps[0].Container != "alpine:3" {
38+
t.Errorf("Step.Container (mirrored from image): got %q want %q", got.Steps[0].Container, "alpine:3")
39+
}
40+
}
41+
42+
// TestParseBatchSpec_v3_codingAgentStep ensures that a v3 spec with a
43+
// codingAgent step parses with the expected typed step.
44+
func TestParseBatchSpec_v3_codingAgentStep(t *testing.T) {
45+
spec := []byte(`
46+
version: 3
47+
name: test
48+
description: test
49+
on:
50+
- repository: github.com/sourcegraph/sourcegraph
51+
steps:
52+
- codingAgent:
53+
type: codex
54+
prompt: do the thing
55+
image: alpine:3
56+
changesetTemplate:
57+
title: test
58+
body: test
59+
branch: test
60+
commit:
61+
message: test
62+
`)
63+
got, err := ParseBatchSpec(spec)
64+
if err != nil {
65+
t.Fatalf("ParseBatchSpec failed: %v", err)
66+
}
67+
if len(got.Steps) != 1 {
68+
t.Fatalf("expected 1 step, got %d", len(got.Steps))
69+
}
70+
step := got.Steps[0]
71+
if step.CodingAgent == nil {
72+
t.Fatal("expected step.CodingAgent to be set")
73+
}
74+
if step.CodingAgent.Type != CodingAgentTypeCodex {
75+
t.Errorf("CodingAgent.Type: got %q want %q", step.CodingAgent.Type, CodingAgentTypeCodex)
76+
}
77+
if step.CodingAgent.Prompt != "do the thing" {
78+
t.Errorf("CodingAgent.Prompt: got %q want %q", step.CodingAgent.Prompt, "do the thing")
79+
}
80+
if step.Container != "alpine:3" {
81+
t.Errorf("Step.Container (mirrored from image): got %q want %q", step.Container, "alpine:3")
82+
}
83+
}

‎lib/batches/codex/codex.go‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,10 @@ import (
1212

1313
const model = "gpt-5.4"
1414

15+
// pinnedVersion is the codex CLI release we test against. Bump in lockstep
16+
// with the model_providers.* config keys below; codex CLI is pre-v1.
17+
const pinnedVersion = "0.134.0"
18+
1519
var routes = []types.ModelProviderRoute{
1620
{WirePath: "/responses", UpstreamPath: "/v1/completions/openai-responses"},
1721
}
@@ -20,11 +24,29 @@ type Agent struct{}
2024

2125
func (Agent) Type() batcheslib.CodingAgentType { return batcheslib.CodingAgentTypeCodex }
2226
func (Agent) ModelProviderRoutes() []types.ModelProviderRoute { return routes }
23-
func (Agent) ImageRequirements() []string { return []string{"codex"} }
27+
func (Agent) ImageRequirements() []string { return []string{"curl", "tar"} }
28+
29+
// InstallScript installs codex from GitHub Releases into types.InstallDir.
30+
func (Agent) InstallScript() string {
31+
return fmt.Sprintf(`_install_dir=%s
32+
_version=%s
33+
_codex_arch=$(uname -m)
34+
case "$_codex_arch" in
35+
x86_64) _codex_triple=x86_64-unknown-linux-musl ;;
36+
aarch64) _codex_triple=aarch64-unknown-linux-musl ;;
37+
*) echo "codingAgent codex: unsupported architecture: $_codex_arch" >&2; exit 1 ;;
38+
esac
39+
mkdir -p "$_install_dir"
40+
curl -fsSL "https://github.com/openai/codex/releases/download/rust-v${_version}/codex-${_codex_triple}.tar.gz" | tar -xz -C "$_install_dir"
41+
mv "$_install_dir/codex-${_codex_triple}" "$_install_dir/codex"
42+
chmod +x "$_install_dir/codex"
43+
"$_install_dir/codex" --version >/dev/null || { echo "codingAgent codex: install verification failed (cannot exec $_install_dir/codex)" >&2; exit 1; }
44+
`, types.InstallDir, pinnedVersion)
45+
}
2446

2547
func (Agent) RunCommand(prompt, modelProviderURL string) string {
2648
return shellquote.Join(
27-
"codex",
49+
types.InstallDir+"/codex",
2850
"exec",
2951
"--json",
3052
"--sandbox", "danger-full-access",

‎lib/batches/codingagent/codingagent.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ func RenderRunCommand(agentStep *batcheslib.CodingAgentStep, modelProviderURL st
3838
for _, binary := range a.ImageRequirements() {
3939
b.WriteString(failIfMissing(a.Type(), binary))
4040
}
41+
b.WriteString(a.InstallScript())
4142
b.WriteString(a.RunCommand(renderedPrompt.String(), prefixed))
4243
return b.String(), nil
4344
}
@@ -47,8 +48,8 @@ func RenderRunCommand(agentStep *batcheslib.CodingAgentStep, modelProviderURL st
4748
// if binary isn't on PATH.
4849
func failIfMissing(agentType batcheslib.CodingAgentType, binary string) string {
4950
msg := fmt.Sprintf(
50-
"codingAgent %q requires %q on PATH in the run container; ensure your image includes the %s binary",
51-
agentType, binary, binary,
51+
"codingAgent %q requires %q on PATH in the run container",
52+
agentType, binary,
5253
)
5354
return fmt.Sprintf("command -v %s >/dev/null 2>&1 || { echo %s >&2; exit 1; }\n",
5455
binary,

‎lib/batches/codingagent/types/types.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,16 +14,24 @@ const JobIDEnvVar = "SRC_BATCHES_JOB_ID"
1414

1515
const JobIDHeaderName = "X-Sourcegraph-Job-ID"
1616

17+
// InstallDir is where each Agent installs its pinned binary; invoked by
18+
// absolute path so we ignore any copy shipped by the image.
19+
const InstallDir = "/tmp/sg-codingagent/bin"
20+
1721
type Agent interface {
1822
Type() batcheslib.CodingAgentType
1923
// RunCommand receives the already-templated prompt and MUST shell-quote it.
2024
RunCommand(renderedPrompt, modelProviderURL string) string
2125
// ModelProviderRoutes returns routes whose WirePath is relative to the
2226
// agent type; the registry prefixes "/<agent-type>" before exposing them.
2327
ModelProviderRoutes() []ModelProviderRoute
24-
// ImageRequirements returns binaries the agent expects on PATH in the run
25-
// container. The registry emits a check before RunCommand.
28+
// ImageRequirements lists binaries that must be on PATH in the run
29+
// container (e.g. "curl" so InstallScript can fetch the agent). The
30+
// registry emits a `command -v` check for each before InstallScript.
2631
ImageRequirements() []string
32+
// InstallScript returns shell that installs the agent at a pinned
33+
// version into a Sourcegraph-owned scratch dir and prepends it to PATH.
34+
InstallScript() string
2735
}
2836

2937
// ModelProviderRoute is one wire→upstream mapping served by the Sourcegraph

0 commit comments

Comments
 (0)