Skip to content

Commit 3fcc9f5

Browse files
gtrrz-victorclaude
andcommitted
fix(trail): warn on failed body fetch during update
Address PR review: resolveTrailUpdateBody swallowed detail-fetch errors, so a failed fetch left the form's Body baseline blank and a subsequent edit could overwrite a description the user never saw. Return the fetch error and warn on stderr (mirroring runTrailShow) so the blank baseline is never silent. Also drop the redundant re-trim (fetchTrailDescription already trims) and refresh the helper/inline comments, which still described an unconditional "full-body wipe" that the change-detection logic already prevents. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01KY4W1R808FYPN32C0VKBGJCH
1 parent 2e41e41 commit 3fcc9f5

2 files changed

Lines changed: 51 additions & 12 deletions

File tree

cmd/entire/cli/trail_cmd.go

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1101,18 +1101,26 @@ func newTrailCreateRequest(title, body, branch, base, statusStr, typeStr, priori
11011101

11021102
// resolveTrailUpdateBody returns the body text to seed the interactive update
11031103
// form with. The list resource omits the description (it lives in
1104-
// body_document, detail-endpoint only), so update must fetch the detail body
1105-
// or it would seed an empty field and PATCH a full-body wipe. Best-effort:
1106-
// a failed or empty detail fetch falls back to the list body, mirroring
1107-
// runTrailShow.
1108-
func resolveTrailUpdateBody(ctx context.Context, client *api.Client, forge, owner, repo string, found *api.TrailResource) string {
1104+
// body_document, served only by the detail endpoint), so update must fetch the
1105+
// detail body — otherwise the form prefills from the empty list body and a
1106+
// user edit against that blank baseline can overwrite a description they never
1107+
// saw. Best-effort: on a failed detail fetch it returns the list body plus the
1108+
// error so the caller can warn (mirroring runTrailShow); an empty detail body
1109+
// (older/partial server) falls back to the list body with no error.
1110+
func resolveTrailUpdateBody(ctx context.Context, client *api.Client, forge, owner, repo string, found *api.TrailResource) (string, error) {
11091111
body := found.Body
11101112
if found.Number > 0 {
1111-
if bt, err := fetchTrailDescription(ctx, client, forge, owner, repo, found.Number); err == nil && strings.TrimSpace(bt) != "" {
1113+
bt, err := fetchTrailDescription(ctx, client, forge, owner, repo, found.Number)
1114+
if err != nil {
1115+
return body, err
1116+
}
1117+
// fetchTrailDescription already trims; a non-empty result supersedes
1118+
// the list body, an empty one (older/partial server) leaves it intact.
1119+
if bt != "" {
11121120
body = bt
11131121
}
11141122
}
1115-
return body
1123+
return body, nil
11161124
}
11171125

11181126
func newTrailUpdateCmd() *cobra.Command {
@@ -1238,9 +1246,14 @@ func runTrailUpdate(ctx context.Context, w, errW io.Writer, insecureHTTP bool, i
12381246
statusStr = string(metadata.Status)
12391247
title = metadata.Title
12401248
// The list resource omits the description; fetch the detail body so
1241-
// the form is prefilled with the current text instead of blank
1242-
// (blank + BodyChanged would PATCH a full-body wipe).
1243-
body = resolveTrailUpdateBody(ctx, client, forge, owner, repoName, found)
1249+
// the form prefills with the current text and change detection below
1250+
// compares against the real server value. Warn on a fetch failure so
1251+
// a blank baseline doesn't silently overwrite an unseen description.
1252+
seedBody, bodyErr := resolveTrailUpdateBody(ctx, client, forge, owner, repoName, found)
1253+
if bodyErr != nil {
1254+
fmt.Fprintf(errW, "Warning: could not load current trail body: %v\n", bodyErr)
1255+
}
1256+
body = seedBody
12441257
origStatus, origTitle, origBody := statusStr, title, body
12451258

12461259
form := NewAccessibleForm(

cmd/entire/cli/trail_cmd_test.go

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -604,7 +604,10 @@ func TestResolveTrailUpdateBody_PrefersDetailSnapshot(t *testing.T) {
604604
// The list resource omits the description, so found.Body is empty. The
605605
// seed must come from the detail endpoint, not the empty list body.
606606
found := &api.TrailResource{Number: 42, Body: ""}
607-
body := resolveTrailUpdateBody(t.Context(), client, "gh", "acme", "repo", found)
607+
body, err := resolveTrailUpdateBody(t.Context(), client, "gh", "acme", "repo", found)
608+
if err != nil {
609+
t.Fatalf("resolveTrailUpdateBody: %v", err)
610+
}
608611
if body != "the real body" {
609612
t.Fatalf("body = %q, want %q", body, "the real body")
610613
}
@@ -623,12 +626,35 @@ func TestResolveTrailUpdateBody_FallsBackToListBody(t *testing.T) {
623626

624627
client := api.NewClientWithBaseURL("tok", srv.URL)
625628
found := &api.TrailResource{Number: 42, Body: "list body"}
626-
body := resolveTrailUpdateBody(t.Context(), client, "gh", "acme", "repo", found)
629+
body, err := resolveTrailUpdateBody(t.Context(), client, "gh", "acme", "repo", found)
630+
if err != nil {
631+
t.Fatalf("resolveTrailUpdateBody: %v", err)
632+
}
627633
if body != "list body" {
628634
t.Fatalf("body = %q, want %q", body, "list body")
629635
}
630636
}
631637

638+
func TestResolveTrailUpdateBody_ReturnsErrorOnFetchFailure(t *testing.T) {
639+
t.Parallel()
640+
// A detail-fetch failure must be surfaced (not swallowed) so the caller can
641+
// warn: a blank baseline could otherwise silently overwrite an unseen body.
642+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
643+
w.WriteHeader(http.StatusInternalServerError)
644+
}))
645+
defer srv.Close()
646+
647+
client := api.NewClientWithBaseURL("tok", srv.URL)
648+
found := &api.TrailResource{Number: 42, Body: "list body"}
649+
body, err := resolveTrailUpdateBody(t.Context(), client, "gh", "acme", "repo", found)
650+
if err == nil {
651+
t.Fatal("expected error on fetch failure, got nil")
652+
}
653+
if body != "list body" {
654+
t.Fatalf("body = %q, want fallback %q", body, "list body")
655+
}
656+
}
657+
632658
func TestResolveCreateBranch(t *testing.T) {
633659
t.Parallel()
634660
tests := []struct {

0 commit comments

Comments
 (0)