Skip to content

Commit cba7ca4

Browse files
committed
Fix misleading breadcrumb, path traversal error type, show attachment test
Three review fixes: 1. attachments list: download breadcrumb now only shown when at least one attachment has a download URL, avoiding misleading hints. 2. writeBodyToFile: path traversal error now returns output.ErrUsage for structured JSON/agent output, consistent with files.go. 3. TestShowInjectsFieldScopedAttachments: verifies generic show injects content_attachments/description_attachments (not flat "attachments") when content has bc-attachment tags.
1 parent 3c7ae4a commit cba7ca4

2 files changed

Lines changed: 42 additions & 3 deletions

File tree

internal/commands/attachments.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -163,8 +163,9 @@ func runAttachmentsList(cmd *cobra.Command, arg, recordType string) error {
163163
Description: "Add comment",
164164
})
165165
}
166-
if len(attachments) > 0 {
167-
breadcrumbs = append(breadcrumbs, attachmentBreadcrumb(id, len(attachments)))
166+
downloadable := downloadableAttachments(attachments)
167+
if len(downloadable) > 0 {
168+
breadcrumbs = append(breadcrumbs, attachmentBreadcrumb(id, len(downloadable)))
168169
}
169170

170171
respOpts := []output.ResponseOption{
@@ -755,7 +756,7 @@ func writeBodyToFile(body io.Reader, dir, name string) (outputPath string, writt
755756
expectedPrefix += string(filepath.Separator)
756757
}
757758
if !strings.HasPrefix(absPath, expectedPrefix) {
758-
return "", 0, fmt.Errorf("invalid filename: path traversal detected")
759+
return "", 0, output.ErrUsage("Invalid filename: path traversal detected")
759760
}
760761

761762
outFile, err := createFile(outputPath)

internal/commands/show_test.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -675,3 +675,41 @@ func TestAllValidRecordTypesHandledInSwitch(t *testing.T) {
675675
})
676676
}
677677
}
678+
679+
// --- Attachment discovery tests ---
680+
681+
func TestShowInjectsFieldScopedAttachments(t *testing.T) {
682+
transport := &showTrackingTransport{
683+
responder: func(path string) (int, string) {
684+
return 200, `{
685+
"id": 42,
686+
"type": "Message",
687+
"title": "Design review",
688+
"content": "<p>See <bc-attachment url=\"https://example.com/a.png\" filename=\"a.png\"></bc-attachment></p>",
689+
"description": "<p>Also <bc-attachment url=\"https://example.com/b.pdf\" filename=\"b.pdf\"></bc-attachment></p>"
690+
}`
691+
},
692+
}
693+
app := showTestApp(t, transport)
694+
buf := &bytes.Buffer{}
695+
app.Output = output.New(output.Options{Format: output.FormatJSON, Writer: buf})
696+
697+
cmd := NewShowCmd()
698+
cmd.SetArgs([]string{"message", "42"})
699+
ctx := appctx.WithApp(context.Background(), app)
700+
cmd.SetContext(ctx)
701+
cmd.SetOut(&bytes.Buffer{})
702+
cmd.SetErr(&bytes.Buffer{})
703+
704+
err := cmd.Execute()
705+
require.NoError(t, err)
706+
707+
out := buf.String()
708+
// Field-scoped keys present
709+
assert.Contains(t, out, "content_attachments")
710+
assert.Contains(t, out, "description_attachments")
711+
// Flat "attachments" key not injected (avoids collision with native API field)
712+
assert.NotContains(t, out, `"attachments"`)
713+
// Notice mentions download command
714+
assert.Contains(t, out, "attachment(s)")
715+
}

0 commit comments

Comments
 (0)