Skip to content

Commit d30fb97

Browse files
MacAttakclaude
andcommitted
fix(output-binary): resolve all verify WARN findings
Security: escape $tokenFlag and SchemaPath in templates. Wiring: add enum constraint to output.format in JSON Schema. Tests: strengthen panic tests with value assertions, verify intermediate YAML keys in round-trips, harden whitespace mimeType assertion, add null to schema rejection table, require os.Stdout.Stat() specifically, verify comment context for string schemas, and replace fragile string-index return block isolation with line-based parsing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 82dbfb0 commit d30fb97

File tree

8 files changed

+83
-48
lines changed

8 files changed

+83
-48
lines changed

internal/codegen/cli_go.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -725,7 +725,7 @@ func init() {
725725
{{$goName}}Cmd.Flags().StringVar(&{{$goName}}FlagOutput, "output", "", "Output file path for binary data")
726726
{{- end}}
727727
{{- if $hasAuth}}
728-
{{$goName}}Cmd.Flags().StringVar(&{{$goName}}Token, "{{$tokenFlag}}", "", "Auth token (overrides {{$tokenEnv | esc}} env var)")
728+
{{$goName}}Cmd.Flags().StringVar(&{{$goName}}Token, "{{$tokenFlag | esc}}", "", "Auth token (overrides {{$tokenEnv | esc}} env var)")
729729
{{- end}}
730730
rootCmd.AddCommand({{$goName}}Cmd)
731731
}
@@ -833,7 +833,7 @@ var {{.GoName}}Cmd = &cobra.Command{
833833
token = os.Getenv("{{$tokenEnv | esc}}")
834834
}
835835
if token == "" {
836-
return fmt.Errorf("auth required: set {{$tokenEnv | esc}} or pass --{{$tokenFlag}}")
836+
return fmt.Errorf("auth required: set {{$tokenEnv | esc}} or pass --{{$tokenFlag | esc}}")
837837
}
838838
_ = token // passed to the entrypoint via environment
839839
{{- end}}

internal/codegen/cli_go_output_test.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -252,10 +252,8 @@ func TestGoCLI_BinaryOutput_TTYDetection_HasStatCall(t *testing.T) {
252252
files := generateCLI(t, manifestBinaryTool())
253253
content := fileContent(t, files, "internal/commands/screenshot.go")
254254

255-
assert.True(t,
256-
strings.Contains(content, "os.Stdout.Stat()") ||
257-
strings.Contains(content, "Stat()"),
258-
"binary tool RunE must call os.Stdout.Stat() for TTY detection")
255+
assert.Contains(t, content, "os.Stdout.Stat()",
256+
"binary tool RunE must call os.Stdout.Stat() specifically for TTY detection")
259257
}
260258

261259
func TestGoCLI_BinaryOutput_TTYDetection_ChecksModeCharDevice(t *testing.T) {

internal/codegen/mcp_typescript.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -577,7 +577,7 @@ import { validateRequest } from "../auth/middleware.js";
577577
// Tool: {{.ToolName}}
578578
// Description: {{.Description}}
579579
{{- if .SchemaPath}}
580-
// Output schema: {{.SchemaPath}} (resolved at build time)
580+
// Output schema: {{.SchemaPath | esc}} (resolved at build time)
581581
{{- end}}
582582
583583
// Input schema for {{.ToolName}}

internal/codegen/mcp_typescript_output_test.go

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -239,8 +239,8 @@ func TestTSMCP_OutputSchema_StringSchemaEmitsComment(t *testing.T) {
239239
files := generateTSMCP(t, m)
240240
content := fileContent(t, files, "src/tools/get_data.ts")
241241

242-
assert.Contains(t, content, "schemas/output.json",
243-
"string schema path must appear in generated code (as a comment)")
242+
assert.Contains(t, content, "// Output schema: schemas/output.json",
243+
"string schema path must appear inside a // comment line")
244244
}
245245

246246
func TestTSMCP_OutputSchema_StringSchemaDoesNotEmitInlineOutputSchema(t *testing.T) {
@@ -376,26 +376,36 @@ func TestTSMCP_BinaryOutput_DoesNotReturnTextType(t *testing.T) {
376376
files := generateTSMCP(t, m)
377377
content := fileContent(t, files, "src/tools/screenshot.ts")
378378

379-
// The handler function body is between handle_screenshot and the export.
380-
// We need to check the handler's return, not the entire file.
379+
// Isolate the handler function body (from handle_screenshot to the next
380+
// top-level function declaration or export). This is more robust than
381+
// trying to find "return {" and guessing closing braces.
381382
handlerIdx := strings.Index(content, "handle_screenshot")
382383
require.Greater(t, handlerIdx, 0, "handle_screenshot must exist")
383384
afterHandler := content[handlerIdx:]
384385

385-
// Look for the return statement area. A binary tool must NOT have
386-
// { type: "text", text: "..." } as its return.
387-
// Allow "text" in comments or descriptions, but NOT in the return content array.
388-
returnIdx := strings.Index(afterHandler, "return {")
389-
require.Greater(t, returnIdx, 0, "handler must have a return statement")
390-
returnBlock := afterHandler[returnIdx:]
391-
// Take just the return block (up to the next function or end of handler)
392-
endIdx := strings.Index(returnBlock, "}\n}")
393-
if endIdx > 0 {
394-
returnBlock = returnBlock[:endIdx+3]
386+
// The handler ends at the next top-level declaration (export/function/const at col 0).
387+
// Split into lines and collect until we hit a line starting with a
388+
// top-level keyword after the handler signature.
389+
lines := strings.Split(afterHandler, "\n")
390+
var handlerLines []string
391+
started := false
392+
for _, line := range lines {
393+
if !started {
394+
started = true
395+
handlerLines = append(handlerLines, line)
396+
continue
397+
}
398+
// Stop at next top-level declaration
399+
trimmed := strings.TrimSpace(line)
400+
if strings.HasPrefix(trimmed, "export ") || strings.HasPrefix(trimmed, "/**") {
401+
break
402+
}
403+
handlerLines = append(handlerLines, line)
395404
}
396405

397-
assert.NotContains(t, returnBlock, `type: "text"`,
398-
"binary tool return block must NOT contain type: \"text\"")
406+
handlerBody := strings.Join(handlerLines, "\n")
407+
assert.NotContains(t, handlerBody, `type: "text"`,
408+
"binary tool handler must NOT contain type: \"text\"")
399409
}
400410

401411
func TestTSMCP_BinaryOutput_IncludesBase64Encoding(t *testing.T) {

internal/manifest/types_output_test.go

Lines changed: 44 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -752,7 +752,12 @@ func TestOutput_RoundTrip_AllFields(t *testing.T) {
752752
marshalled, err := yaml.Marshal(original)
753753
require.NoError(t, err)
754754

755-
roundTripped, err := Parse(strings.NewReader(string(marshalled)))
755+
yamlStr := string(marshalled)
756+
assert.Contains(t, yamlStr, "schema:", "intermediate YAML must contain schema key")
757+
assert.Contains(t, yamlStr, "mimeType:", "intermediate YAML must contain mimeType key")
758+
assert.Contains(t, yamlStr, "format:", "intermediate YAML must contain format key")
759+
760+
roundTripped, err := Parse(strings.NewReader(yamlStr))
756761
require.NoError(t, err)
757762

758763
if diff := cmp.Diff(original, roundTripped); diff != "" {
@@ -779,7 +784,12 @@ func TestOutput_RoundTrip_InlineSchemaWithMimeType(t *testing.T) {
779784
marshalled, err := yaml.Marshal(original)
780785
require.NoError(t, err)
781786

782-
roundTripped, err := Parse(strings.NewReader(string(marshalled)))
787+
yamlStr := string(marshalled)
788+
assert.Contains(t, yamlStr, "schema:", "intermediate YAML must contain schema key")
789+
assert.Contains(t, yamlStr, "mimeType:", "intermediate YAML must contain mimeType key")
790+
assert.Contains(t, yamlStr, "type: object", "intermediate YAML must contain inline schema type")
791+
792+
roundTripped, err := Parse(strings.NewReader(yamlStr))
783793
require.NoError(t, err)
784794

785795
if diff := cmp.Diff(original, roundTripped); diff != "" {
@@ -1250,16 +1260,24 @@ func TestOutput_Schema_IntegerValueDoesNotPanic(t *testing.T) {
12501260
format: json
12511261
schema: 42
12521262
`
1253-
// This should not panic. Whether it parses successfully or errors is
1254-
// implementation-dependent, but it must not crash.
1263+
// Must not panic. Parse may succeed (storing int in any) or error —
1264+
// either is acceptable, but we assert the outcome explicitly.
1265+
var parsed *Toolkit
1266+
var parseErr error
12551267
func() {
12561268
defer func() {
12571269
if r := recover(); r != nil {
1258-
t.Errorf("Parse panicked on integer schema: %v", r)
1270+
t.Fatalf("Parse panicked on integer schema: %v", r)
12591271
}
12601272
}()
1261-
_, _ = Parse(strings.NewReader(input))
1273+
parsed, parseErr = Parse(strings.NewReader(input))
12621274
}()
1275+
if parseErr != nil {
1276+
return // error is an acceptable outcome
1277+
}
1278+
require.Len(t, parsed.Tools, 1)
1279+
assert.Equal(t, 42, parsed.Tools[0].Output.Schema,
1280+
"integer schema should be stored as-is in the any field")
12631281
}
12641282

12651283
func TestOutput_Schema_BooleanValueDoesNotPanic(t *testing.T) {
@@ -1270,14 +1288,22 @@ func TestOutput_Schema_BooleanValueDoesNotPanic(t *testing.T) {
12701288
format: json
12711289
schema: true
12721290
`
1291+
var parsed *Toolkit
1292+
var parseErr error
12731293
func() {
12741294
defer func() {
12751295
if r := recover(); r != nil {
1276-
t.Errorf("Parse panicked on boolean schema: %v", r)
1296+
t.Fatalf("Parse panicked on boolean schema: %v", r)
12771297
}
12781298
}()
1279-
_, _ = Parse(strings.NewReader(input))
1299+
parsed, parseErr = Parse(strings.NewReader(input))
12801300
}()
1301+
if parseErr != nil {
1302+
return
1303+
}
1304+
require.Len(t, parsed.Tools, 1)
1305+
assert.Equal(t, true, parsed.Tools[0].Output.Schema,
1306+
"boolean schema should be stored as-is in the any field")
12811307
}
12821308

12831309
func TestOutput_Schema_ArrayValueDoesNotPanic(t *testing.T) {
@@ -1290,12 +1316,20 @@ func TestOutput_Schema_ArrayValueDoesNotPanic(t *testing.T) {
12901316
- item1
12911317
- item2
12921318
`
1319+
var parsed *Toolkit
1320+
var parseErr error
12931321
func() {
12941322
defer func() {
12951323
if r := recover(); r != nil {
1296-
t.Errorf("Parse panicked on array schema: %v", r)
1324+
t.Fatalf("Parse panicked on array schema: %v", r)
12971325
}
12981326
}()
1299-
_, _ = Parse(strings.NewReader(input))
1327+
parsed, parseErr = Parse(strings.NewReader(input))
13001328
}()
1329+
if parseErr != nil {
1330+
return
1331+
}
1332+
require.Len(t, parsed.Tools, 1)
1333+
assert.IsType(t, []any{}, parsed.Tools[0].Output.Schema,
1334+
"array schema should be stored as []any in the any field")
13011335
}

internal/manifest/validate_output_test.go

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -921,25 +921,17 @@ func TestValidateOutput_MultipleTools_ErrorReferencesCorrectIndex(t *testing.T)
921921
// ---------------------------------------------------------------------------
922922

923923
func TestValidateOutput_BinaryWithWhitespaceMimeType(t *testing.T) {
924-
// A mimeType that is only whitespace should be treated as empty/missing.
925-
// This catches implementations that only check `mimeType == ""` without
926-
// trimming. Note: this depends on whether the implementation trims.
927-
// If the implementation does NOT trim, this test documents that " " is
928-
// accepted (and the test should be adjusted). Either way, the test forces
929-
// a conscious decision.
924+
// A mimeType that is only whitespace must be treated as empty/missing.
925+
// The implementation uses strings.TrimSpace, so " " is rejected.
930926
tk := validToolkitWithOutput(Output{
931927
Format: "binary",
932928
MimeType: " ",
933929
})
934930

935931
errs := Validate(tk)
936-
// We check for the error — a robust implementation should reject whitespace-only.
937932
ve := findErrorByRule(errs, "binary-requires-mimetype")
938-
// If this assertion fails, it means whitespace-only mimeType is accepted.
939-
// Adjust the test if that is the intended behavior.
940-
if ve != nil {
941-
assert.Equal(t, SeverityError, ve.Severity,
942-
"Whitespace-only mimeType should be rejected as missing")
943-
}
944-
// At minimum, we ensure no panic or unexpected error type.
933+
require.NotNil(t, ve,
934+
"whitespace-only mimeType must trigger binary-requires-mimetype")
935+
assert.Equal(t, SeverityError, ve.Severity,
936+
"whitespace-only mimeType should be rejected as missing")
945937
}

schema_output_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -486,6 +486,7 @@ func TestSchemaOutput_OutputAsNonObject_Fails(t *testing.T) {
486486
{name: "number", value: `42`},
487487
{name: "array", value: `["json"]`},
488488
{name: "boolean", value: `true`},
489+
{name: "null", value: `null`},
489490
}
490491
for _, tc := range wrongTypes {
491492
t.Run(tc.name, func(t *testing.T) {

schemas/toolwright.schema.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@
9191
"output": {
9292
"type": "object",
9393
"properties": {
94-
"format": { "type": "string" },
94+
"format": { "type": "string", "enum": ["json", "text", "binary"] },
9595
"schema": {
9696
"oneOf": [
9797
{ "type": "string" },

0 commit comments

Comments
 (0)