Skip to content

Commit 3a2a4a8

Browse files
wolfeidauwolfeidau
authored
Merge pull request #237 from buildkite/refactor/remove-redundant-validations
refactor: remove redundant manual arg validation in tool handlers
2 parents 89ab15f + 436afba commit 3a2a4a8

16 files changed

Lines changed: 107 additions & 531 deletions

‎pkg/buildkite/annotations.go‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -36,18 +36,6 @@ func ListAnnotations() (mcp.Tool, mcp.ToolHandlerFor[ListAnnotationsArgs, any],
3636
ctx, span := trace.Start(ctx, "buildkite.ListAnnotations")
3737
defer span.End()
3838

39-
if args.OrgSlug == "" {
40-
return utils.NewToolResultError("org_slug is required"), nil, nil
41-
}
42-
43-
if args.PipelineSlug == "" {
44-
return utils.NewToolResultError("pipeline_slug is required"), nil, nil
45-
}
46-
47-
if args.BuildNumber == "" {
48-
return utils.NewToolResultError("build_number is required"), nil, nil
49-
}
50-
5139
paginationParams := paginationFromArgs(args.Page, args.PerPage)
5240

5341
span.SetAttributes(

‎pkg/buildkite/artifacts.go‎

Lines changed: 0 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -112,16 +112,6 @@ func ListArtifactsForBuild() (mcp.Tool, mcp.ToolHandlerFor[ListArtifactsForBuild
112112
ctx, span := trace.Start(ctx, "buildkite.ListArtifactsForBuild")
113113
defer span.End()
114114

115-
if args.OrgSlug == "" {
116-
return utils.NewToolResultError("org_slug is required"), nil, nil
117-
}
118-
if args.PipelineSlug == "" {
119-
return utils.NewToolResultError("pipeline_slug is required"), nil, nil
120-
}
121-
if args.BuildNumber == "" {
122-
return utils.NewToolResultError("build_number is required"), nil, nil
123-
}
124-
125115
paginationParams := paginationFromArgs(args.Page, args.PerPage)
126116

127117
span.SetAttributes(
@@ -168,19 +158,6 @@ func ListArtifactsForJob() (mcp.Tool, mcp.ToolHandlerFor[ListArtifactsForJobArgs
168158
ctx, span := trace.Start(ctx, "buildkite.ListArtifactsForJob")
169159
defer span.End()
170160

171-
if args.OrgSlug == "" {
172-
return utils.NewToolResultError("org_slug is required"), nil, nil
173-
}
174-
if args.PipelineSlug == "" {
175-
return utils.NewToolResultError("pipeline_slug is required"), nil, nil
176-
}
177-
if args.BuildNumber == "" {
178-
return utils.NewToolResultError("build_number is required"), nil, nil
179-
}
180-
if args.JobID == "" {
181-
return utils.NewToolResultError("job_id is required"), nil, nil
182-
}
183-
184161
paginationParams := paginationFromArgs(args.Page, args.PerPage)
185162

186163
span.SetAttributes(
@@ -229,10 +206,6 @@ func GetArtifact() (mcp.Tool, mcp.ToolHandlerFor[GetArtifactArgs, any], []string
229206
ctx, span := trace.Start(ctx, "buildkite.GetArtifact")
230207
defer span.End()
231208

232-
if args.URL == "" {
233-
return utils.NewToolResultError("url is required"), nil, nil
234-
}
235-
236209
artifactURL := args.URL
237210

238211
// Validate the URL format and scheme

‎pkg/buildkite/artifacts_test.go‎

Lines changed: 0 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -167,106 +167,6 @@ func TestGetArtifact(t *testing.T) {
167167
assert.Contains(textContent.Text, `"data":"VGhpcyBpcyB0ZXN0IGFydGlmYWN0IGNvbnRlbnQ="`)
168168
}
169169

170-
func TestListArtifactsForBuild_MissingParameters(t *testing.T) {
171-
assert := require.New(t)
172-
173-
ctx := ContextWithDeps(context.Background(), ToolDependencies{ArtifactsClient: &MockArtifactsClient{}})
174-
175-
_, handler, _ := ListArtifactsForBuild()
176-
177-
// Test missing org parameter
178-
req := createMCPRequest(t, map[string]any{})
179-
result, _, err := handler(ctx, req, ListArtifactsForBuildArgs{
180-
PipelineSlug: "test-pipeline",
181-
BuildNumber: "123",
182-
})
183-
assert.NoError(err)
184-
assert.NotNil(result)
185-
assert.Contains(getTextResult(t, result).Text, "org_slug is required")
186-
187-
// Test missing pipeline_slug parameter
188-
result, _, err = handler(ctx, req, ListArtifactsForBuildArgs{
189-
OrgSlug: "test-org",
190-
BuildNumber: "123",
191-
})
192-
assert.NoError(err)
193-
assert.NotNil(result)
194-
assert.Contains(getTextResult(t, result).Text, "pipeline_slug is required")
195-
196-
// Test missing build_number parameter
197-
result, _, err = handler(ctx, req, ListArtifactsForBuildArgs{
198-
OrgSlug: "test-org",
199-
PipelineSlug: "test-pipeline",
200-
})
201-
assert.NoError(err)
202-
assert.NotNil(result)
203-
assert.Contains(getTextResult(t, result).Text, "build_number is required")
204-
}
205-
206-
func TestListArtifactsForJob_MissingParameters(t *testing.T) {
207-
assert := require.New(t)
208-
209-
ctx := ContextWithDeps(context.Background(), ToolDependencies{ArtifactsClient: &MockArtifactsClient{}})
210-
211-
_, handler, _ := ListArtifactsForJob()
212-
213-
// Test missing org parameter
214-
req := createMCPRequest(t, map[string]any{})
215-
result, _, err := handler(ctx, req, ListArtifactsForJobArgs{
216-
PipelineSlug: "test-pipeline",
217-
BuildNumber: "123",
218-
JobID: "123456-abcdef-123abc-456def",
219-
})
220-
assert.NoError(err)
221-
assert.NotNil(result)
222-
assert.Contains(getTextResult(t, result).Text, "org_slug is required")
223-
224-
// Test missing pipeline_slug parameter
225-
result, _, err = handler(ctx, req, ListArtifactsForJobArgs{
226-
OrgSlug: "test-org",
227-
BuildNumber: "123",
228-
JobID: "123456-abcdef-123abc-456def",
229-
})
230-
assert.NoError(err)
231-
assert.NotNil(result)
232-
assert.Contains(getTextResult(t, result).Text, "pipeline_slug is required")
233-
234-
// Test missing build_number parameter
235-
result, _, err = handler(ctx, req, ListArtifactsForJobArgs{
236-
OrgSlug: "test-org",
237-
PipelineSlug: "test-pipeline",
238-
JobID: "123456-abcdef-123abc-456def",
239-
})
240-
assert.NoError(err)
241-
assert.NotNil(result)
242-
assert.Contains(getTextResult(t, result).Text, "build_number is required")
243-
244-
// Test missing job_id parameter
245-
result, _, err = handler(ctx, req, ListArtifactsForJobArgs{
246-
OrgSlug: "test-org",
247-
PipelineSlug: "test-pipeline",
248-
BuildNumber: "123",
249-
})
250-
assert.NoError(err)
251-
assert.NotNil(result)
252-
assert.Contains(getTextResult(t, result).Text, "job_id is required")
253-
}
254-
255-
func TestGetArtifact_MissingParameters(t *testing.T) {
256-
assert := require.New(t)
257-
258-
ctx := ContextWithDeps(context.Background(), ToolDependencies{ArtifactsClient: &MockArtifactsClient{}})
259-
260-
_, handler, _ := GetArtifact()
261-
262-
// Test missing url parameter
263-
req := createMCPRequest(t, map[string]any{})
264-
result, _, err := handler(ctx, req, GetArtifactArgs{})
265-
assert.NoError(err)
266-
assert.NotNil(result)
267-
assert.Contains(getTextResult(t, result).Text, "url is required")
268-
}
269-
270170
func TestGetArtifact_ErrorResponse(t *testing.T) {
271171
assert := require.New(t)
272172

‎pkg/buildkite/builds.go‎

Lines changed: 0 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -156,11 +156,6 @@ func ListBuilds() (mcp.Tool, mcp.ToolHandlerFor[ListBuildsArgs, any], []string)
156156
ctx, span := trace.Start(ctx, "buildkite.ListBuilds")
157157
defer span.End()
158158

159-
// Validate required parameters
160-
if args.OrgSlug == "" {
161-
return utils.NewToolResultError("org_slug parameter is required"), nil, nil
162-
}
163-
164159
span.SetAttributes(
165160
attribute.String("org_slug", args.OrgSlug),
166161
attribute.String("pipeline_slug", args.PipelineSlug),
@@ -286,17 +281,6 @@ func GetBuildTestEngineRuns() (mcp.Tool, mcp.ToolHandlerFor[GetBuildTestEngineRu
286281
ctx, span := trace.Start(ctx, "buildkite.GetBuildTestEngineRuns")
287282
defer span.End()
288283

289-
// Validate required parameters
290-
if args.OrgSlug == "" {
291-
return utils.NewToolResultError("org_slug parameter is required"), nil, nil
292-
}
293-
if args.PipelineSlug == "" {
294-
return utils.NewToolResultError("pipeline_slug parameter is required"), nil, nil
295-
}
296-
if args.BuildNumber == "" {
297-
return utils.NewToolResultError("build_number parameter is required"), nil, nil
298-
}
299-
300284
span.SetAttributes(
301285
attribute.String("org_slug", args.OrgSlug),
302286
attribute.String("pipeline_slug", args.PipelineSlug),
@@ -341,17 +325,6 @@ func GetBuild() (mcp.Tool, mcp.ToolHandlerFor[GetBuildArgs, any], []string) {
341325
ctx, span := trace.Start(ctx, "buildkite.GetBuild")
342326
defer span.End()
343327

344-
// Validate required parameters
345-
if args.OrgSlug == "" {
346-
return utils.NewToolResultError("org_slug parameter is required"), nil, nil
347-
}
348-
if args.PipelineSlug == "" {
349-
return utils.NewToolResultError("pipeline_slug parameter is required"), nil, nil
350-
}
351-
if args.BuildNumber == "" {
352-
return utils.NewToolResultError("build_number parameter is required"), nil, nil
353-
}
354-
355328
span.SetAttributes(
356329
attribute.String("org_slug", args.OrgSlug),
357330
attribute.String("pipeline_slug", args.PipelineSlug),

‎pkg/buildkite/builds_test.go‎

Lines changed: 0 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"testing"
77

88
"github.com/buildkite/go-buildkite/v4"
9-
"github.com/modelcontextprotocol/go-sdk/mcp"
109
"github.com/stretchr/testify/require"
1110
)
1211

@@ -466,42 +465,6 @@ func TestGetBuildTestEngineRunsNoBuildTestEngine(t *testing.T) {
466465
assert.Equal("null", textContent.Text)
467466
}
468467

469-
func TestGetBuildTestEngineRunsMissingParameters(t *testing.T) {
470-
assert := require.New(t)
471-
472-
ctx := ContextWithDeps(context.Background(), ToolDependencies{BuildsClient: &MockBuildsClient{}})
473-
474-
_, handler, _ := GetBuildTestEngineRuns()
475-
476-
// Test missing org parameter
477-
request := createMCPRequest(t, map[string]any{})
478-
result, _, err := handler(ctx, request, GetBuildTestEngineRunsArgs{
479-
PipelineSlug: "pipeline",
480-
BuildNumber: "1",
481-
})
482-
assert.NoError(err)
483-
assert.True(result.IsError)
484-
assert.Contains(result.Content[0].(*mcp.TextContent).Text, "org_slug")
485-
486-
// Test missing pipeline_slug parameter
487-
result, _, err = handler(ctx, request, GetBuildTestEngineRunsArgs{
488-
OrgSlug: "org",
489-
BuildNumber: "1",
490-
})
491-
assert.NoError(err)
492-
assert.True(result.IsError)
493-
assert.Contains(result.Content[0].(*mcp.TextContent).Text, "pipeline_slug")
494-
495-
// Test missing build_number parameter
496-
result, _, err = handler(ctx, request, GetBuildTestEngineRunsArgs{
497-
OrgSlug: "org",
498-
PipelineSlug: "pipeline",
499-
})
500-
assert.NoError(err)
501-
assert.True(result.IsError)
502-
assert.Contains(result.Content[0].(*mcp.TextContent).Text, "build_number")
503-
}
504-
505468
func TestCreateBuild(t *testing.T) {
506469
assert := require.New(t)
507470

‎pkg/buildkite/cluster_queue.go‎

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -40,14 +40,6 @@ func ListClusterQueues() (mcp.Tool, mcp.ToolHandlerFor[ListClusterQueuesArgs, an
4040
ctx, span := trace.Start(ctx, "buildkite.ListClusterQueues")
4141
defer span.End()
4242

43-
if args.OrgSlug == "" {
44-
return utils.NewToolResultError("org_slug is required"), nil, nil
45-
}
46-
47-
if args.ClusterID == "" {
48-
return utils.NewToolResultError("cluster_id is required"), nil, nil
49-
}
50-
5143
paginationParams := paginationFromArgs(args.Page, args.PerPage)
5244

5345
span.SetAttributes(
@@ -92,18 +84,6 @@ func GetClusterQueue() (mcp.Tool, mcp.ToolHandlerFor[GetClusterQueueArgs, any],
9284
ctx, span := trace.Start(ctx, "buildkite.GetClusterQueue")
9385
defer span.End()
9486

95-
if args.OrgSlug == "" {
96-
return utils.NewToolResultError("org_slug is required"), nil, nil
97-
}
98-
99-
if args.ClusterID == "" {
100-
return utils.NewToolResultError("cluster_id is required"), nil, nil
101-
}
102-
103-
if args.QueueID == "" {
104-
return utils.NewToolResultError("queue_id is required"), nil, nil
105-
}
106-
10787
span.SetAttributes(
10888
attribute.String("org_slug", args.OrgSlug),
10989
attribute.String("cluster_id", args.ClusterID),

‎pkg/buildkite/clusters.go‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,6 @@ func ListClusters() (mcp.Tool, mcp.ToolHandlerFor[ListClustersArgs, any], []stri
3838
ctx, span := trace.Start(ctx, "buildkite.ListClusters")
3939
defer span.End()
4040

41-
if args.OrgSlug == "" {
42-
return utils.NewToolResultError("org_slug is required"), nil, nil
43-
}
44-
4541
paginationParams := paginationFromArgs(args.Page, args.PerPage)
4642

4743
span.SetAttributes(
@@ -85,14 +81,6 @@ func GetCluster() (mcp.Tool, mcp.ToolHandlerFor[GetClusterArgs, any], []string)
8581
ctx, span := trace.Start(ctx, "buildkite.GetCluster")
8682
defer span.End()
8783

88-
if args.OrgSlug == "" {
89-
return utils.NewToolResultError("org_slug is required"), nil, nil
90-
}
91-
92-
if args.ClusterID == "" {
93-
return utils.NewToolResultError("cluster_id is required"), nil, nil
94-
}
95-
9684
span.SetAttributes(
9785
attribute.String("org_slug", args.OrgSlug),
9886
attribute.String("cluster_id", args.ClusterID),

‎pkg/buildkite/jobs.go‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -44,20 +44,6 @@ func UnblockJob() (mcp.Tool, mcp.ToolHandlerFor[UnblockJobArgs, any], []string)
4444
ctx, span := trace.Start(ctx, "buildkite.UnblockJob")
4545
defer span.End()
4646

47-
// Validate required parameters
48-
if args.OrgSlug == "" {
49-
return utils.NewToolResultError("org_slug parameter is required"), nil, nil
50-
}
51-
if args.PipelineSlug == "" {
52-
return utils.NewToolResultError("pipeline_slug parameter is required"), nil, nil
53-
}
54-
if args.BuildNumber == "" {
55-
return utils.NewToolResultError("build_number parameter is required"), nil, nil
56-
}
57-
if args.JobID == "" {
58-
return utils.NewToolResultError("job_id parameter is required"), nil, nil
59-
}
60-
6147
span.SetAttributes(
6248
attribute.String("org_slug", args.OrgSlug),
6349
attribute.String("pipeline_slug", args.PipelineSlug),

0 commit comments

Comments
 (0)