From a5ded2854a198158069cf9f5ffccf6d382ccb063 Mon Sep 17 00:00:00 2001 From: ysyneu Date: Thu, 5 Nov 2026 01:07:25 -0800 Subject: [PATCH 1/3] feat(safari): artifact-download; bound byte transfers by progress, not total time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reading an artifact took three commands and hand-parsing a token (gallery-get → sign → cut t out of download_url → artifact-stream), and the generated artifact-stream printed "OK" for a JSON file and truncated files over 10MB. - `safari artifact-download ` resolves the ID (also from a console link), signs, streams to the server's file name (base name only) or --output (- for stdout), and removes a partial file on failure. - `safari artifact-stream --t` stays at its path-name for callers holding a download_url, now curated on the SDK's streaming Artifacts.Download. - Timeouts by kind of call: JSON calls keep the SDK's 30s overall timeout; byte transfers (artifact download, session export, file/skill/CSV uploads) have no total limit and are cancelled after 60s with no bytes moved, with an error that says so. Requires go-flashduty with Artifacts.Download. --- go.mod | 2 +- go.sum | 2 + internal/cli/artifact_download.go | 202 +++++++++++++++++++++++++ internal/cli/artifact_download_test.go | 144 ++++++++++++++++++ internal/cli/coverage_test.go | 4 + internal/cli/root.go | 1 + internal/cli/safari_send.go | 7 +- internal/cli/session.go | 7 +- internal/cli/transfer.go | 68 +++++++++ internal/cli/upload.go | 14 +- internal/cli/zz_generated_artifacts.go | 54 ------- internal/cli/zz_generated_manifest.go | 1 - internal/cmd/cligen/main.go | 6 + skills/flashduty/reference/safari.md | 12 +- 14 files changed, 452 insertions(+), 72 deletions(-) create mode 100644 internal/cli/artifact_download.go create mode 100644 internal/cli/artifact_download_test.go create mode 100644 internal/cli/transfer.go diff --git a/go.mod b/go.mod index cbd4317..cf5c983 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/flashcatcloud/flashduty-cli go 1.26.0 require ( - github.com/flashcatcloud/go-flashduty v0.16.0 + github.com/flashcatcloud/go-flashduty v0.16.1-0.20261105090239-bf4b1ab78074 github.com/mattn/go-runewidth v0.0.30 github.com/spf13/cobra v1.10.2 github.com/spf13/pflag v1.0.10 diff --git a/go.sum b/go.sum index 70aad9a..26de3b6 100644 --- a/go.sum +++ b/go.sum @@ -3,6 +3,8 @@ github.com/clipperhouse/uax29/v2 v2.2.0/go.mod h1:EFJ2TJMRUaplDxHKj1qAEhCtQPW2tJ github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= github.com/flashcatcloud/go-flashduty v0.16.0 h1:YHrd9NaJZReWv1u1iUSLh89HNCeFP+ZaL1tBvLFHrA8= github.com/flashcatcloud/go-flashduty v0.16.0/go.mod h1:YpHiTYXR5NXBI/rGRZfUy537XMkhdCkwA8NW1QoRHwk= +github.com/flashcatcloud/go-flashduty v0.16.1-0.20261105090239-bf4b1ab78074 h1:uhJ3M9xUKS08DlomkR0Ag8qgDKXxpKIWqJV+HtorJ80= +github.com/flashcatcloud/go-flashduty v0.16.1-0.20261105090239-bf4b1ab78074/go.mod h1:YpHiTYXR5NXBI/rGRZfUy537XMkhdCkwA8NW1QoRHwk= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= github.com/mattn/go-runewidth v0.0.30 h1:+KUuiDA4fF0R1p5FeueHefjDm+GIM+kWfFnDjybOPgk= diff --git a/internal/cli/artifact_download.go b/internal/cli/artifact_download.go new file mode 100644 index 0000000..c310534 --- /dev/null +++ b/internal/cli/artifact_download.go @@ -0,0 +1,202 @@ +package cli + +import ( + "cmp" + "fmt" + "io" + "mime" + "net/url" + "os" + "path/filepath" + "regexp" + "strings" + + "github.com/flashcatcloud/go-flashduty" + "github.com/spf13/cobra" +) + +// artifactRefRe finds an artifact ID (art_) or presented-file ID (pf_) on its +// own or inside a console link such as /ai-sre/artifacts/art_… . +var artifactRefRe = regexp.MustCompile(`\b(art|pf)_[0-9A-Za-z]{16,}\b`) + +func attachSafariArtifactDownload(root *cobra.Command) { + g := genGroup(root, "safari", "AI SRE API") + genAddLeaf(g, newArtifactDownloadCmd()) + genAddLeaf(g, newArtifactStreamCmd()) +} + +// newArtifactStreamCmd is the path-name command for GET /safari/artifact/stream: +// it takes the signed token itself. artifact-download is the one to use; this +// serves callers that already hold a download_url from artifact-sign. +func newArtifactStreamCmd() *cobra.Command { + var token, output string + + cmd := &cobra.Command{ + Use: "artifact-stream --t ", + Short: "Download a file by signed token", + Long: `Download a file using the token of a download_url from artifact-sign (the t +query parameter; valid for 5 minutes). To download by artifact ID, file ID or +console link, use artifact-download instead — it does the signing itself. + +The file is saved like artifact-download does: under the server's file name, or +at --output (- for stdout). + +API: GET /safari/artifact/stream (artifact-read-stream)`, + Example: ` flashduty safari artifact-stream --t "$TOKEN" -o report.html`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + if token == "" { + return fmt.Errorf("--t is required") + } + return runCommand(cmd, args, func(ctx *RunContext) error { + return downloadSigned(ctx, token, "", output) + }) + }, + } + cmd.Flags().StringVar(&token, "t", "", "Token from the download_url of artifact-sign (required)") + cmd.Flags().StringVarP(&output, "output", "o", "", "Save to this path (- for stdout); default: the file's own name in the current directory") + return cmd +} + +func newArtifactDownloadCmd() *cobra.Command { + var output string + + cmd := &cobra.Command{ + Use: "artifact-download ", + Short: "Download an artifact or session file", + Long: `Download a published artifact or a file a session presented, and save it. + +The argument is an artifact ID (art_…), a presented-file ID (pf_… — the +file_id of a present_files result), or a console link containing either, such +as a link to an artifact page. + +The file is saved under the name the server gives it in the current directory, +or at --output; --output - writes the bytes to stdout. The command prints where +it saved the file. + +The transfer has no total time limit; it is cancelled only after 60s with no +bytes moved. + +API: POST /safari/artifact/gallery/get, POST /safari/artifact/sign, GET /safari/artifact/stream`, + Example: ` flashduty safari artifact-download art_VnfrWic8UbB9q3EfYR4Gmg + flashduty safari artifact-download pf_SdhEA5fbZJGnHzwrNJMMSB --output report.html + flashduty safari artifact-download "https://console.flashcat.cloud/ai-sre/artifacts/art_VnfrWic8UbB9q3EfYR4Gmg" -o - | head`, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + return runCommand(cmd, args, func(ctx *RunContext) error { + return runArtifactDownload(ctx, args[0], output) + }) + }, + } + cmd.Flags().StringVarP(&output, "output", "o", "", "Save to this path (- for stdout); default: the file's own name in the current directory") + return cmd +} + +type artifactDownloadResult struct { + Path string `json:"path" toon:"path"` + Bytes int64 `json:"bytes" toon:"bytes"` + ContentType string `json:"content_type" toon:"content_type"` + FileID string `json:"file_id,omitempty" toon:"file_id,omitempty"` +} + +func runArtifactDownload(ctx *RunContext, ref, output string) error { + m := artifactRefRe.FindStringSubmatch(ref) + if m == nil { + return fmt.Errorf("%q has no artifact ID (art_…) or file ID (pf_…)", ref) + } + fileID := m[0] + if m[1] == "art" { + item, _, err := ctx.Client.Artifacts.ReadGet(cmdContext(ctx.Cmd), &flashduty.ArtifactIDRequest{ArtifactID: m[0]}) + if err != nil { + return err + } + fileID = item.FileID + } + + signed, _, err := ctx.Client.Artifacts.ReadSign(cmdContext(ctx.Cmd), &flashduty.ArtifactSignRequest{FileID: fileID}) + if err != nil { + return err + } + token, err := signedToken(signed.DownloadURL) + if err != nil { + return err + } + return downloadSigned(ctx, token, fileID, output) +} + +// downloadSigned streams the file behind a signed token to output (see +// artifact-download for the output rules). fileID only labels the result and +// names the file when the server gives no name. +func downloadSigned(ctx *RunContext, token, fileID, output string) error { + tr := startTransfer(cmdContext(ctx.Cmd)) + rc, resp, err := ctx.Client.Artifacts.Download(tr.ctx, token) + if err != nil { + return tr.done(err) + } + defer func() { _ = rc.Close() }() + body := tr.track(rc) + + if output == "-" { + _, err = io.Copy(ctx.Writer, body) + return tr.done(err) + } + if output == "" { + output = attachmentName(resp.Header.Get("Content-Disposition"), cmp.Or(fileID, "download")) + } + n, err := saveFile(output, body) + if err = tr.done(err); err != nil { + return err + } + + res := artifactDownloadResult{Path: output, Bytes: n, ContentType: resp.Header.Get("Content-Type"), FileID: fileID} + if ctx.Structured() { + return ctx.Printer.Print(res, nil) + } + ctx.WriteResult(fmt.Sprintf("Saved %s (%d bytes, %s)", res.Path, res.Bytes, res.ContentType)) + return nil +} + +// signedToken extracts the t parameter of a sign response's download_url. +func signedToken(downloadURL string) (string, error) { + u, err := url.Parse(downloadURL) + if err != nil { + return "", fmt.Errorf("unexpected download_url %q: %w", downloadURL, err) + } + t := u.Query().Get("t") + if t == "" { + return "", fmt.Errorf("download_url %q carries no token", downloadURL) + } + return t, nil +} + +// attachmentName returns the base name of a Content-Disposition filename, or +// fallback when there is none usable. Only the base name is kept so a server +// name can never place the file outside the current directory. +func attachmentName(disposition, fallback string) string { + _, params, err := mime.ParseMediaType(disposition) + if err != nil { + return fallback + } + name := filepath.Base(strings.ReplaceAll(params["filename"], `\`, "/")) + if name == "." || name == "/" || name == ".." || name == "" { + return fallback + } + return name +} + +// saveFile writes r to path and removes the partial file if the copy fails. +func saveFile(path string, r io.Reader) (int64, error) { + f, err := os.Create(path) + if err != nil { + return 0, err + } + n, err := io.Copy(f, r) + if cerr := f.Close(); err == nil { + err = cerr + } + if err != nil { + _ = os.Remove(path) + return 0, err + } + return n, nil +} diff --git a/internal/cli/artifact_download_test.go b/internal/cli/artifact_download_test.go new file mode 100644 index 0000000..c2ab36e --- /dev/null +++ b/internal/cli/artifact_download_test.go @@ -0,0 +1,144 @@ +package cli + +import ( + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/flashcatcloud/go-flashduty" +) + +// artifactServer fakes gallery-get → sign → stream for one artifact whose file +// is a JSON document (the shape the typed SDK path used to swallow). +func artifactServer(t *testing.T, file string, stall time.Duration) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/safari/artifact/gallery/get": + body, _ := io.ReadAll(r.Body) + if !strings.Contains(string(body), `"artifact_id":"art_AAAAAAAAAAAAAAAAAAAAAA"`) { + t.Errorf("gallery/get body = %s", body) + } + _, _ = io.WriteString(w, `{"data":{"artifact_id":"art_AAAAAAAAAAAAAAAAAAAAAA","file_id":"pf_BBBBBBBBBBBBBBBBBBBBBB"}}`) + case "/safari/artifact/sign": + body, _ := io.ReadAll(r.Body) + if !strings.Contains(string(body), `"file_id":"pf_BBBBBBBBBBBBBBBBBBBBBB"`) { + t.Errorf("sign body = %s", body) + } + _, _ = io.WriteString(w, `{"data":{"download_url":"/safari/artifact/stream?mode=download&t=TOK%2B1","expires_in":300}}`) + case "/safari/artifact/stream": + if got := r.URL.Query().Get("t"); got != "TOK+1" { + t.Errorf("stream t = %q", got) + } + w.Header().Set("Content-Type", "application/json") + w.Header().Set("Content-Disposition", `attachment; filename="../../etc/data.json"`) + w.(http.Flusher).Flush() + time.Sleep(stall) + _, _ = io.WriteString(w, file) + default: + t.Errorf("unexpected path %s", r.URL.Path) + } + })) + t.Cleanup(srv.Close) + newClientFn = func() (*flashduty.Client, error) { + return flashduty.NewClient("test-key", flashduty.WithBaseURL(srv.URL)) + } + return srv +} + +func TestArtifactDownloadFromConsoleLinkSavesServerName(t *testing.T) { + saveAndResetGlobals(t) + const file = `{"error":{"code":"x"},"rows":[1,2]}` + artifactServer(t, file, 0) + t.Chdir(t.TempDir()) + + out, err := execCommand("safari", "artifact-download", "https://console.example/ai-sre/artifacts/art_AAAAAAAAAAAAAAAAAAAAAA") + if err != nil { + t.Fatalf("artifact-download: %v", err) + } + got, err := os.ReadFile("data.json") // base name only, never ../../etc + if err != nil || string(got) != file { + t.Fatalf("saved file = %q, %v; want %q", got, err, file) + } + if !strings.Contains(out, "Saved data.json (35 bytes, application/json)") { + t.Fatalf("output = %q", out) + } +} + +func TestArtifactDownloadOutputFlagAndStructured(t *testing.T) { + saveAndResetGlobals(t) + artifactServer(t, "hello", 0) + dst := filepath.Join(t.TempDir(), "x.txt") + + out, err := execCommand("safari", "artifact-download", "pf_BBBBBBBBBBBBBBBBBBBBBB", "-o", dst, "--json") + if err != nil { + t.Fatalf("artifact-download: %v", err) + } + var res artifactDownloadResult + if err := json.Unmarshal([]byte(out), &res); err != nil || res.Path != dst || res.Bytes != 5 || res.FileID != "pf_BBBBBBBBBBBBBBBBBBBBBB" { + t.Fatalf("result = %+v (%v) from %q", res, err, out) + } +} + +func TestArtifactDownloadStdout(t *testing.T) { + saveAndResetGlobals(t) + artifactServer(t, "line1\nline2\n", 0) + + out, err := execCommand("safari", "artifact-download", "pf_BBBBBBBBBBBBBBBBBBBBBB", "-o", "-") + if err != nil || out != "line1\nline2\n" { + t.Fatalf("stdout = %q, %v", out, err) + } +} + +func TestArtifactDownloadRejectsRefWithoutID(t *testing.T) { + saveAndResetGlobals(t) + _, err := execCommand("safari", "artifact-download", "https://console.example/ai-sre/artifacts") + if err == nil || !strings.Contains(err.Error(), "no artifact ID") { + t.Fatalf("err = %v", err) + } +} + +func TestTransferStallCancelsAndRemovesPartialFile(t *testing.T) { + saveAndResetGlobals(t) + old := transferIdleTimeout + transferIdleTimeout = 50 * time.Millisecond + t.Cleanup(func() { transferIdleTimeout = old }) + artifactServer(t, "late", 500*time.Millisecond) + dir := t.TempDir() + dst := filepath.Join(dir, "x.txt") + + _, err := execCommand("safari", "artifact-download", "pf_BBBBBBBBBBBBBBBBBBBBBB", "-o", dst) + if !errors.Is(err, errTransferStalled) { + t.Fatalf("err = %v, want errTransferStalled", err) + } + if _, statErr := os.Stat(dst); !os.IsNotExist(statErr) { + t.Fatalf("partial file left behind: %v", statErr) + } +} + +func TestTransferProgressKeepsSlowTransferAlive(t *testing.T) { + old := transferIdleTimeout + transferIdleTimeout = 80 * time.Millisecond + t.Cleanup(func() { transferIdleTimeout = old }) + + tr := startTransfer(t.Context()) + pr, pw := io.Pipe() + go func() { + for range 5 { // 5×40ms = 200ms total, more than the idle timeout + time.Sleep(40 * time.Millisecond) + _, _ = pw.Write([]byte("x")) + } + _ = pw.Close() + }() + n, err := io.Copy(io.Discard, tr.track(pr)) + if err = tr.done(err); err != nil || n != 5 || tr.ctx.Err() == nil { + t.Fatalf("n=%d err=%v ctxErr=%v", n, err, tr.ctx.Err()) + } +} diff --git a/internal/cli/coverage_test.go b/internal/cli/coverage_test.go index 6d3fa70..b4e0762 100644 --- a/internal/cli/coverage_test.go +++ b/internal/cli/coverage_test.go @@ -27,6 +27,10 @@ var curatedOperationIDs = map[string]bool{ // upload methods (see upload.go). "mapping-data-write-upload": true, "skill-write-upload": true, + // Returns a user's file verbatim (any type and size), served by the + // hand-written SDK Artifacts.Download and curated commands (see + // artifact_download.go). + "artifact-read-stream": true, // Path parameter plus X-DSID header; the response is also the data // source's native (non-enveloped) payload. Served by a curated command // built on the hand-written SDK method (see monit_prometheus_label_values.go). diff --git a/internal/cli/root.go b/internal/cli/root.go index 8627444..ec77450 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -160,6 +160,7 @@ func init() { // its path-is-king leaf to the (now-existing) generated `safari` group so the // operation stays reachable at safari session-export. attachSafariSessionExport(rootCmd) + attachSafariArtifactDownload(rootCmd) attachSafariAutomationTriggerFire(rootCmd) // monit-prometheus-read-label-values carries a path parameter, so it is diff --git a/internal/cli/safari_send.go b/internal/cli/safari_send.go index d412590..b56422d 100644 --- a/internal/cli/safari_send.go +++ b/internal/cli/safari_send.go @@ -207,10 +207,11 @@ func uploadSessionFile(ctx *RunContext, sessionID, path string) (flashduty.Sessi return flashduty.SessionMessagePart{}, err } defer func() { _ = f.Close() }() - att, _, err := ctx.Client.Sessions.UploadAttachment(cmdContext(ctx.Cmd), &flashduty.SessionAttachmentUploadRequest{ - SessionID: sessionID, Filename: name, File: f, + tr := startTransfer(cmdContext(ctx.Cmd)) + att, _, err := ctx.Client.Sessions.UploadAttachment(tr.ctx, &flashduty.SessionAttachmentUploadRequest{ + SessionID: sessionID, Filename: name, File: tr.track(f), }) - if err != nil { + if err = tr.done(err); err != nil { return flashduty.SessionMessagePart{}, err } return att.FilePart(), nil diff --git a/internal/cli/session.go b/internal/cli/session.go index 4e002b0..d177d4f 100644 --- a/internal/cli/session.go +++ b/internal/cli/session.go @@ -326,16 +326,17 @@ func buildSessionExportCmd(use string) *cobra.Command { return err } - rc, _, err := ctx.Client.Sessions.Export(cmdContext(ctx.Cmd), &flashduty.SessionExportRequest{ + tr := startTransfer(cmdContext(ctx.Cmd)) + rc, _, err := ctx.Client.Sessions.Export(tr.ctx, &flashduty.SessionExportRequest{ SessionID: ctx.Args[0], IncludeSubagents: includeSubagents, }) if err != nil { - return err + return tr.done(err) } defer func() { _ = rc.Close() }() - return writeSessionExport(ctx.Writer, format, rc) + return tr.done(writeSessionExport(ctx.Writer, format, tr.track(rc))) }) }, } diff --git a/internal/cli/transfer.go b/internal/cli/transfer.go new file mode 100644 index 0000000..0147ff6 --- /dev/null +++ b/internal/cli/transfer.go @@ -0,0 +1,68 @@ +package cli + +import ( + "context" + "errors" + "fmt" + "io" + "time" +) + +// Timeouts differ by kind of call. A JSON call is small, so the SDK client's +// overall timeout (30s) bounds it. A byte transfer — file upload, file +// download, transcript export — runs as long as its size and link need, so the +// SDK serves it without an overall timeout and the CLI bounds it by progress +// instead: it is cancelled only after transferIdleTimeout with no bytes moved. +// The idle clock also covers the wait for the response to start and for the +// reply after the last byte is sent. +var transferIdleTimeout = 60 * time.Second + +// errTransferStalled reports a transfer cancelled for lack of progress. +var errTransferStalled = errors.New("transfer stalled") + +// transfer is the context of one byte transfer: pass ctx to the SDK call, wrap +// the transferred stream with track, and pass the call's error through done. +type transfer struct { + ctx context.Context + cancel context.CancelCauseFunc + timer *time.Timer +} + +func startTransfer(parent context.Context) *transfer { + ctx, cancel := context.WithCancelCause(parent) + return &transfer{ + ctx: ctx, + cancel: cancel, + timer: time.AfterFunc(transferIdleTimeout, func() { cancel(errTransferStalled) }), + } +} + +// track wraps r so every read that moves bytes restarts the idle clock. +func (t *transfer) track(r io.Reader) io.Reader { + return &progressReader{r: r, timer: t.timer} +} + +// done stops the idle clock and, when the transfer failed because it stalled, +// says so instead of the bare "context canceled" the stall produced. +func (t *transfer) done(err error) error { + t.timer.Stop() + stalled := errors.Is(context.Cause(t.ctx), errTransferStalled) + t.cancel(nil) + if err != nil && stalled { + return fmt.Errorf("%w: no bytes moved for %s, so it was cancelled; retry, and check the network path if it repeats", errTransferStalled, transferIdleTimeout) + } + return err +} + +type progressReader struct { + r io.Reader + timer *time.Timer +} + +func (p *progressReader) Read(b []byte) (int, error) { + n, err := p.r.Read(b) + if n > 0 { + p.timer.Reset(transferIdleTimeout) + } + return n, err +} diff --git a/internal/cli/upload.go b/internal/cli/upload.go index 715f57c..dd7e002 100644 --- a/internal/cli/upload.go +++ b/internal/cli/upload.go @@ -70,13 +70,14 @@ API: POST /enrichment/mapping/data/upload (mapping-data-write-upload)`, } defer func() { _ = file.Close() }() - _, err = ctx.Client.AlertEnrichment.MappingDataWriteUpload(cmdContext(ctx.Cmd), &flashduty.MappingDataUploadRequest{ + tr := startTransfer(cmdContext(ctx.Cmd)) + _, err = ctx.Client.AlertEnrichment.MappingDataWriteUpload(tr.ctx, &flashduty.MappingDataUploadRequest{ SchemaID: schemaID, DoNotTruncateFirst: doNotTruncateFirst, - File: file, + File: tr.track(file), Filename: name, }) - if err != nil { + if err = tr.done(err); err != nil { return err } ctx.WriteResult("OK: POST /enrichment/mapping/data/upload") @@ -122,14 +123,15 @@ API: POST /safari/skill/upload (skill-write-upload)`, } defer func() { _ = file.Close() }() - item, _, err := ctx.Client.Skills.WriteUpload(cmdContext(ctx.Cmd), &flashduty.SkillUploadRequest{ - File: file, + tr := startTransfer(cmdContext(ctx.Cmd)) + item, _, err := ctx.Client.Skills.WriteUpload(tr.ctx, &flashduty.SkillUploadRequest{ + File: tr.track(file), Filename: name, TeamID: teamID, Replace: replace, SkillID: skillID, }) - if err != nil { + if err = tr.done(err); err != nil { return err } if ctx.Structured() { diff --git a/internal/cli/zz_generated_artifacts.go b/internal/cli/zz_generated_artifacts.go index e02cae5..21446d3 100644 --- a/internal/cli/zz_generated_artifacts.go +++ b/internal/cli/zz_generated_artifacts.go @@ -306,59 +306,6 @@ Response fields ('data' envelope is unwrapped — these fields are at the top le return cmd } -func genArtifactsReadStreamCmd() *cobra.Command { - var dataJSON string - var fT string - var fMode string - cmd := &cobra.Command{ - Use: "artifact-stream", - Short: "Download or preview a file", - Long: `Download or preview a file. - -Download or preview a file's bytes using a signed token. - -API: GET /safari/artifact/stream (artifact-read-stream) - -Request fields: - --t string (required) — Signed token issued by 'POST /safari/artifact/sign'. Bound to the calling account and person; valid for 5 minutes. - --mode string — 'download' (default) serves the file as an attachment; 'preview' serves it inline for browser display. Any other value falls back to 'download'. [download, preview] -`, - RunE: func(cmd *cobra.Command, args []string) error { - return runCommand(cmd, args, func(ctx *RunContext) error { - body, err := genAssembleBody(dataJSON, func(body map[string]any) error { - if cmd.Flags().Changed("t") { - body["t"] = fT - } - if cmd.Flags().Changed("mode") { - body["mode"] = fMode - } - return nil - }) - if err != nil { - return err - } - req := new(flashduty.ArtifactsReadStreamRequest) - if err := genBindBody(ctx.Cmd.ErrOrStderr(), body, req); err != nil { - return err - } - resp, err := ctx.Client.Artifacts.ReadStream(cmdContext(ctx.Cmd), req) - if err != nil { - return err - } - if resp != nil && len(resp.Raw) > 0 { - return ctx.WriteRaw(resp.Raw) - } - ctx.WriteResult("OK: GET /safari/artifact/stream") - return nil - }) - }, - } - cmd.Flags().StringVar(&fT, "t", "", "Signed token issued by 'POST /safari/artifact/sign'. Bound to the calling account and person; valid for 5 minutes. (required)") - cmd.Flags().StringVar(&fMode, "mode", "", "'download' (default) serves the file as an attachment; 'preview' serves it inline for browser display. Any other value falls back to 'download'. [download, preview]") - cmd.Flags().StringVar(&dataJSON, "data", "", "Full request body as JSON; positional arguments and typed flags override its fields. Accepts inline JSON, or - to read stdin.") - return cmd -} - func genArtifactsWriteDeleteCmd() *cobra.Command { var dataJSON string var fArtifactID string @@ -714,7 +661,6 @@ func registerGeneratedArtifacts(root *cobra.Command) { genAddLeaf(gSafari, genArtifactsReadGetFileStateCmd()) genAddLeaf(gSafari, genArtifactsReadListCmd()) genAddLeaf(gSafari, genArtifactsReadSignCmd()) - genAddLeaf(gSafari, genArtifactsReadStreamCmd()) genAddLeaf(gSafari, genArtifactsWriteDeleteCmd()) genAddLeaf(gSafari, genArtifactsWritePublishCmd()) genAddLeaf(gSafari, genArtifactsWriteShareEnableCmd()) diff --git a/internal/cli/zz_generated_manifest.go b/internal/cli/zz_generated_manifest.go index f16b703..323f402 100644 --- a/internal/cli/zz_generated_manifest.go +++ b/internal/cli/zz_generated_manifest.go @@ -22,7 +22,6 @@ var generatedOpIDs = []string{ "artifact-read-get-file-state", "artifact-read-list", "artifact-read-sign", - "artifact-read-stream", "artifact-write-delete", "artifact-write-publish", "artifact-write-share-enable", diff --git a/internal/cmd/cligen/main.go b/internal/cmd/cligen/main.go index b496680..ae85ec8 100644 --- a/internal/cmd/cligen/main.go +++ b/internal/cmd/cligen/main.go @@ -294,6 +294,12 @@ func collectServices(paths, schemas map[string]any, sdk map[string]map[string]me // method. Bounded binary downloads (text/csv, application/octet-stream) are not // streams: the generated command streams Response.Raw for them. func isStreamingOp(o map[string]any) bool { + // The artifact stream returns a user's file verbatim (any type, any size); + // go-flashduty serves it as the hand-written Artifacts.Download, and the + // curated `safari artifact-download` covers it. + if str(o, "operationId") == "artifact-read-stream" { + return true + } content := asMap(asMap(asMap(o["responses"])["200"])["content"]) if _, hasJSON := content["application/json"]; hasJSON { return false diff --git a/skills/flashduty/reference/safari.md b/skills/flashduty/reference/safari.md index 00b51ef..61b4b69 100644 --- a/skills/flashduty/reference/safari.md +++ b/skills/flashduty/reference/safari.md @@ -105,6 +105,10 @@ Update A2A agent - `--team-id` int64 — Reassign team scope; omit to leave unchanged. Moving to a team requires membership in it; moving to account scope ('0') requires the account owner or an admin. If the team changes without 'environments' in the same request, the current runner entries must stay selectable by the caller or the update is rejected. - body-only (`--data`): auth_config (object) +### artifact-download +Download an artifact or session file +- `--output` string + ### artifact-gallery-delete Remove artifact from gallery - `` (positional, required) string — Artifact ID ('art_' prefix). Also the key of the public-share link. @@ -163,10 +167,10 @@ Create signed file URLs - `--share-token` string — Optional session share-link token. Needed only when the caller reaches the file through a shared session link rather than account membership. - response: single object (`data` unwrapped to the top level) — fields: content_type (string); download_url (string); expires_in (integer); frame_base (string); frame_expires_in (integer); frame_url (string); name (string); preview_url (string); size (integer) -### artifact-stream -Download or preview a file -- `--mode` string — 'download' (default) serves the file as an attachment; 'preview' serves it inline for browser display. Any other value falls back to 'download'. · enum: download | preview -- `--t` string (required) — Signed token issued by 'POST /safari/artifact/sign'. Bound to the calling account and person; valid for 5 minutes. +### artifact-stream --t +Download a file by signed token +- `--output` string +- `--t` string ### automation-rule-create Create Automation rule From 8d687f06cc8396c14226ec27365cb2414140db86 Mon Sep 17 00:00:00 2001 From: ysyneu Date: Thu, 5 Nov 2026 01:10:45 -0800 Subject: [PATCH 2/3] fix(safari): artifact-download keeps an earlier file when a transfer fails Write through a temp file beside the destination and rename it into place, so a failed or stalled download leaves neither a partial file nor a truncated earlier one; devices and FIFOs (-o /dev/null) are written directly. Reject an artifact that resolves to no file_id. Bumps go-flashduty for download-token redaction in logged URLs. --- go.mod | 2 +- go.sum | 2 ++ internal/cli/artifact_download.go | 33 ++++++++++++++++++++++---- internal/cli/artifact_download_test.go | 12 +++++++--- 4 files changed, 40 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index cf5c983..fcc2447 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/flashcatcloud/flashduty-cli go 1.26.0 require ( - github.com/flashcatcloud/go-flashduty v0.16.1-0.20261105090239-bf4b1ab78074 + github.com/flashcatcloud/go-flashduty v0.16.1-0.20261105090952-5be8d556bcd3 github.com/mattn/go-runewidth v0.0.30 github.com/spf13/cobra v1.10.2 github.com/spf13/pflag v1.0.10 diff --git a/go.sum b/go.sum index 26de3b6..6f1be32 100644 --- a/go.sum +++ b/go.sum @@ -5,6 +5,8 @@ github.com/flashcatcloud/go-flashduty v0.16.0 h1:YHrd9NaJZReWv1u1iUSLh89HNCeFP+Z github.com/flashcatcloud/go-flashduty v0.16.0/go.mod h1:YpHiTYXR5NXBI/rGRZfUy537XMkhdCkwA8NW1QoRHwk= github.com/flashcatcloud/go-flashduty v0.16.1-0.20261105090239-bf4b1ab78074 h1:uhJ3M9xUKS08DlomkR0Ag8qgDKXxpKIWqJV+HtorJ80= github.com/flashcatcloud/go-flashduty v0.16.1-0.20261105090239-bf4b1ab78074/go.mod h1:YpHiTYXR5NXBI/rGRZfUy537XMkhdCkwA8NW1QoRHwk= +github.com/flashcatcloud/go-flashduty v0.16.1-0.20261105090952-5be8d556bcd3 h1:rChSIGUT1HeKKcRnUSSof0kOFDkLp38P/OsAi0ZqZvE= +github.com/flashcatcloud/go-flashduty v0.16.1-0.20261105090952-5be8d556bcd3/go.mod h1:YpHiTYXR5NXBI/rGRZfUy537XMkhdCkwA8NW1QoRHwk= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= github.com/mattn/go-runewidth v0.0.30 h1:+KUuiDA4fF0R1p5FeueHefjDm+GIM+kWfFnDjybOPgk= diff --git a/internal/cli/artifact_download.go b/internal/cli/artifact_download.go index c310534..63eb398 100644 --- a/internal/cli/artifact_download.go +++ b/internal/cli/artifact_download.go @@ -110,6 +110,9 @@ func runArtifactDownload(ctx *RunContext, ref, output string) error { if err != nil { return err } + if item.FileID == "" { + return fmt.Errorf("artifact %s has no file to download", m[0]) + } fileID = item.FileID } @@ -184,18 +187,38 @@ func attachmentName(disposition, fallback string) string { return name } -// saveFile writes r to path and removes the partial file if the copy fails. +// saveFile writes r to path. A file is written to a temp file beside it and +// renamed into place, so a failed transfer leaves neither a partial file nor a +// damaged earlier one. A destination that exists and is not a regular file (a +// device or FIFO such as /dev/null) is written directly. func saveFile(path string, r io.Reader) (int64, error) { - f, err := os.Create(path) + if fi, err := os.Stat(path); err == nil && !fi.Mode().IsRegular() { + f, err := os.OpenFile(path, os.O_WRONLY, 0) + if err != nil { + return 0, err + } + n, err := io.Copy(f, r) + if cerr := f.Close(); err == nil { + err = cerr + } + return n, err + } + tmp, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".part-*") if err != nil { return 0, err } - n, err := io.Copy(f, r) - if cerr := f.Close(); err == nil { + n, err := io.Copy(tmp, r) + if err == nil { + err = tmp.Chmod(0o644) + } + if cerr := tmp.Close(); err == nil { err = cerr } + if err == nil { + err = os.Rename(tmp.Name(), path) + } if err != nil { - _ = os.Remove(path) + _ = os.Remove(tmp.Name()) return 0, err } return n, nil diff --git a/internal/cli/artifact_download_test.go b/internal/cli/artifact_download_test.go index c2ab36e..4d27705 100644 --- a/internal/cli/artifact_download_test.go +++ b/internal/cli/artifact_download_test.go @@ -105,7 +105,7 @@ func TestArtifactDownloadRejectsRefWithoutID(t *testing.T) { } } -func TestTransferStallCancelsAndRemovesPartialFile(t *testing.T) { +func TestTransferStallKeepsEarlierFileAndLeavesNoPartial(t *testing.T) { saveAndResetGlobals(t) old := transferIdleTimeout transferIdleTimeout = 50 * time.Millisecond @@ -113,13 +113,19 @@ func TestTransferStallCancelsAndRemovesPartialFile(t *testing.T) { artifactServer(t, "late", 500*time.Millisecond) dir := t.TempDir() dst := filepath.Join(dir, "x.txt") + if err := os.WriteFile(dst, []byte("earlier"), 0o644); err != nil { + t.Fatal(err) + } _, err := execCommand("safari", "artifact-download", "pf_BBBBBBBBBBBBBBBBBBBBBB", "-o", dst) if !errors.Is(err, errTransferStalled) { t.Fatalf("err = %v, want errTransferStalled", err) } - if _, statErr := os.Stat(dst); !os.IsNotExist(statErr) { - t.Fatalf("partial file left behind: %v", statErr) + if got, _ := os.ReadFile(dst); string(got) != "earlier" { + t.Fatalf("earlier file changed to %q", got) + } + if entries, _ := os.ReadDir(dir); len(entries) != 1 { + t.Fatalf("temp file left behind: %v", entries) } } From d10e8d74036d35293114cb8e14f260ad52e130f4 Mon Sep 17 00:00:00 2001 From: ysyneu Date: Thu, 5 Nov 2026 01:17:54 -0800 Subject: [PATCH 3/3] fix(safari): slow readers are not stalls; server-named files never replace local ones A response body's idle clock now runs only while a read waits on the network, so piping a download or export into a slow consumer is not reported as a stalled transfer. Without --output, a server-chosen name that already exists is refused instead of replaced. The temp file is created with 0666 so the umask decides the saved file's mode. go mod tidy drops stale go.sum lines. --- go.sum | 4 --- internal/cli/artifact_download.go | 13 ++++---- internal/cli/artifact_download_test.go | 41 ++++++++++++++++++++++++++ internal/cli/session.go | 2 +- internal/cli/transfer.go | 22 +++++++++++++- 5 files changed, 71 insertions(+), 11 deletions(-) diff --git a/go.sum b/go.sum index 6f1be32..e61cdba 100644 --- a/go.sum +++ b/go.sum @@ -1,10 +1,6 @@ github.com/clipperhouse/uax29/v2 v2.2.0 h1:ChwIKnQN3kcZteTXMgb1wztSgaU+ZemkgWdohwgs8tY= github.com/clipperhouse/uax29/v2 v2.2.0/go.mod h1:EFJ2TJMRUaplDxHKj1qAEhCtQPW2tJSwu5BF98AuoVM= github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= -github.com/flashcatcloud/go-flashduty v0.16.0 h1:YHrd9NaJZReWv1u1iUSLh89HNCeFP+ZaL1tBvLFHrA8= -github.com/flashcatcloud/go-flashduty v0.16.0/go.mod h1:YpHiTYXR5NXBI/rGRZfUy537XMkhdCkwA8NW1QoRHwk= -github.com/flashcatcloud/go-flashduty v0.16.1-0.20261105090239-bf4b1ab78074 h1:uhJ3M9xUKS08DlomkR0Ag8qgDKXxpKIWqJV+HtorJ80= -github.com/flashcatcloud/go-flashduty v0.16.1-0.20261105090239-bf4b1ab78074/go.mod h1:YpHiTYXR5NXBI/rGRZfUy537XMkhdCkwA8NW1QoRHwk= github.com/flashcatcloud/go-flashduty v0.16.1-0.20261105090952-5be8d556bcd3 h1:rChSIGUT1HeKKcRnUSSof0kOFDkLp38P/OsAi0ZqZvE= github.com/flashcatcloud/go-flashduty v0.16.1-0.20261105090952-5be8d556bcd3/go.mod h1:YpHiTYXR5NXBI/rGRZfUy537XMkhdCkwA8NW1QoRHwk= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= diff --git a/internal/cli/artifact_download.go b/internal/cli/artifact_download.go index 63eb398..56298de 100644 --- a/internal/cli/artifact_download.go +++ b/internal/cli/artifact_download.go @@ -137,7 +137,7 @@ func downloadSigned(ctx *RunContext, token, fileID, output string) error { return tr.done(err) } defer func() { _ = rc.Close() }() - body := tr.track(rc) + body := tr.trackBody(rc) if output == "-" { _, err = io.Copy(ctx.Writer, body) @@ -145,6 +145,10 @@ func downloadSigned(ctx *RunContext, token, fileID, output string) error { } if output == "" { output = attachmentName(resp.Header.Get("Content-Disposition"), cmp.Or(fileID, "download")) + // The name came from the server: never let it replace a local file. + if _, err := os.Lstat(output); err == nil { + return tr.done(fmt.Errorf("%s already exists; pass --output to choose the path (an explicit path is overwritten)", output)) + } } n, err := saveFile(output, body) if err = tr.done(err); err != nil { @@ -203,14 +207,13 @@ func saveFile(path string, r io.Reader) (int64, error) { } return n, err } - tmp, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".part-*") + // 0o666 so the umask decides the mode, as for any file the shell creates. + tmpName := filepath.Join(filepath.Dir(path), fmt.Sprintf(".%s.part-%d", filepath.Base(path), os.Getpid())) + tmp, err := os.OpenFile(tmpName, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o666) if err != nil { return 0, err } n, err := io.Copy(tmp, r) - if err == nil { - err = tmp.Chmod(0o644) - } if cerr := tmp.Close(); err == nil { err = cerr } diff --git a/internal/cli/artifact_download_test.go b/internal/cli/artifact_download_test.go index 4d27705..9fd5997 100644 --- a/internal/cli/artifact_download_test.go +++ b/internal/cli/artifact_download_test.go @@ -1,6 +1,7 @@ package cli import ( + "context" "encoding/json" "errors" "io" @@ -97,6 +98,46 @@ func TestArtifactDownloadStdout(t *testing.T) { } } +func TestArtifactDownloadDefaultNameNeverReplacesLocalFile(t *testing.T) { + saveAndResetGlobals(t) + artifactServer(t, "new", 0) + t.Chdir(t.TempDir()) + if err := os.WriteFile("data.json", []byte("mine"), 0o644); err != nil { + t.Fatal(err) + } + + _, err := execCommand("safari", "artifact-download", "pf_BBBBBBBBBBBBBBBBBBBBBB") + if err == nil || !strings.Contains(err.Error(), "already exists") { + t.Fatalf("err = %v", err) + } + if got, _ := os.ReadFile("data.json"); string(got) != "mine" { + t.Fatalf("local file changed to %q", got) + } +} + +func TestTransferBodyIgnoresSlowConsumer(t *testing.T) { + old := transferIdleTimeout + transferIdleTimeout = 50 * time.Millisecond + t.Cleanup(func() { transferIdleTimeout = old }) + + tr := startTransfer(t.Context()) + body := tr.trackBody(strings.NewReader("abc")) + buf := make([]byte, 1) + for { + n, err := body.Read(buf) + if n > 0 { + time.Sleep(80 * time.Millisecond) // consumer slower than the idle timeout + } + if err != nil { + break + } + } + if err := context.Cause(tr.ctx); err != nil { + t.Fatalf("slow consumer cancelled the transfer: %v", err) + } + _ = tr.done(nil) +} + func TestArtifactDownloadRejectsRefWithoutID(t *testing.T) { saveAndResetGlobals(t) _, err := execCommand("safari", "artifact-download", "https://console.example/ai-sre/artifacts") diff --git a/internal/cli/session.go b/internal/cli/session.go index d177d4f..7d7447e 100644 --- a/internal/cli/session.go +++ b/internal/cli/session.go @@ -336,7 +336,7 @@ func buildSessionExportCmd(use string) *cobra.Command { } defer func() { _ = rc.Close() }() - return tr.done(writeSessionExport(ctx.Writer, format, tr.track(rc))) + return tr.done(writeSessionExport(ctx.Writer, format, tr.trackBody(rc))) }) }, } diff --git a/internal/cli/transfer.go b/internal/cli/transfer.go index 0147ff6..f641f20 100644 --- a/internal/cli/transfer.go +++ b/internal/cli/transfer.go @@ -37,11 +37,19 @@ func startTransfer(parent context.Context) *transfer { } } -// track wraps r so every read that moves bytes restarts the idle clock. +// track wraps an upload source: the SDK reads it only as fast as the network +// drains the request, so every read that moves bytes restarts the idle clock. func (t *transfer) track(r io.Reader) io.Reader { return &progressReader{r: r, timer: t.timer} } +// trackBody wraps a response body. The clock runs only while a read waits on +// the network: time the consumer spends with the bytes (a slow pipe such as +// `| less`) is not a stall. +func (t *transfer) trackBody(r io.Reader) io.Reader { + return &bodyReader{r: r, timer: t.timer} +} + // done stops the idle clock and, when the transfer failed because it stalled, // says so instead of the bare "context canceled" the stall produced. func (t *transfer) done(err error) error { @@ -54,6 +62,18 @@ func (t *transfer) done(err error) error { return err } +type bodyReader struct { + r io.Reader + timer *time.Timer +} + +func (b *bodyReader) Read(p []byte) (int, error) { + b.timer.Reset(transferIdleTimeout) + n, err := b.r.Read(p) + b.timer.Stop() + return n, err +} + type progressReader struct { r io.Reader timer *time.Timer