From c0d07a4f0d82d4a6300089769839feba243472d6 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Thu, 6 Aug 2026 14:25:31 -0400 Subject: [PATCH 01/10] feat(#5988): introduce tracker.Client interface with forge adapter Adds internal/tracker, a narrower interface for issue-content read/write (title, body, comments) keyed by (project string, number int) instead of (owner, repo string, number int). This lets Jira, whose issues are keyed as PROJECT-123, implement the same interface later without a forced owner/repo split. forge.Client stays scoped to git-hosting operations. ForgeClient adapts any forge.Client (GitHub or GitLab) to tracker.Client by splitting the project string back into owner/repo. No behavior change: nothing calls tracker.Client yet. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/tracker/forge_client.go | 99 +++++++++++++++++++++ internal/tracker/tracker.go | 49 ++++++++++ internal/tracker/tracker_test.go | 148 +++++++++++++++++++++++++++++++ 3 files changed, 296 insertions(+) create mode 100644 internal/tracker/forge_client.go create mode 100644 internal/tracker/tracker.go create mode 100644 internal/tracker/tracker_test.go diff --git a/internal/tracker/forge_client.go b/internal/tracker/forge_client.go new file mode 100644 index 0000000000..533c879c73 --- /dev/null +++ b/internal/tracker/forge_client.go @@ -0,0 +1,99 @@ +package tracker + +import ( + "context" + "fmt" + "strconv" + "strings" + + "github.com/fullsend-ai/fullsend/internal/forge" +) + +// ForgeClient adapts a forge.Client to the tracker.Client interface. It +// works for both the GitHub and GitLab forge.Client implementations, since +// forge.Client already abstracts over the two — this adapter only needs to +// split the tracker's single "project" string back into the owner/repo +// pair that forge.Client expects, and convert forge.IssueComment's numeric +// ID to the string form tracker.Comment uses. +type ForgeClient struct { + forge forge.Client +} + +// NewForgeClient returns a tracker.Client backed by fc. +func NewForgeClient(fc forge.Client) *ForgeClient { + return &ForgeClient{forge: fc} +} + +// GetIssue implements Client. +func (c *ForgeClient) GetIssue(ctx context.Context, project string, number int) (*Issue, error) { + owner, repo := splitProject(project) + issue, err := c.forge.GetIssue(ctx, owner, repo, number) + if err != nil { + return nil, err + } + return &Issue{ + Number: issue.Number, + Title: issue.Title, + Body: issue.Body, + URL: issue.URL, + Labels: issue.Labels, + }, nil +} + +// ListComments implements Client. +func (c *ForgeClient) ListComments(ctx context.Context, project string, number int) ([]Comment, error) { + owner, repo := splitProject(project) + comments, err := c.forge.ListIssueComments(ctx, owner, repo, number) + if err != nil { + return nil, err + } + result := make([]Comment, len(comments)) + for i, fc := range comments { + result[i] = fromForgeComment(fc) + } + return result, nil +} + +// CreateComment implements Client. +func (c *ForgeClient) CreateComment(ctx context.Context, project string, number int, body string) (*Comment, error) { + owner, repo := splitProject(project) + comment, err := c.forge.CreateIssueComment(ctx, owner, repo, number, body) + if err != nil { + return nil, err + } + result := fromForgeComment(*comment) + return &result, nil +} + +// UpdateComment implements Client. +func (c *ForgeClient) UpdateComment(ctx context.Context, project string, commentID string, body string) error { + owner, repo := splitProject(project) + id, err := strconv.Atoi(commentID) + if err != nil { + return fmt.Errorf("tracker: comment ID %q is not numeric: %w", commentID, err) + } + return c.forge.UpdateIssueComment(ctx, owner, repo, id, body) +} + +func fromForgeComment(c forge.IssueComment) Comment { + return Comment{ + ID: strconv.Itoa(c.ID), + HTMLURL: c.HTMLURL, + Body: c.Body, + Author: c.Author, + CreatedAt: c.CreatedAt, + } +} + +// splitProject splits "group/subgroup/project" into owner="group/subgroup" +// and repo="project". GitHub projects are always single-level +// ("owner/repo"), which this also handles correctly since there's only one +// "/". GitLab projects may be nested under subgroups, hence splitting on +// the last "/" rather than the first. +func splitProject(project string) (owner, repo string) { + idx := strings.LastIndex(project, "/") + if idx < 0 { + return "", project + } + return project[:idx], project[idx+1:] +} diff --git a/internal/tracker/tracker.go b/internal/tracker/tracker.go new file mode 100644 index 0000000000..2fd56bea18 --- /dev/null +++ b/internal/tracker/tracker.go @@ -0,0 +1,49 @@ +// Package tracker defines a narrow, forge-agnostic interface for reading +// and writing issue content (title, body, comments), keyed by +// (project string, number int) rather than the (owner, repo string, number +// int) shape used by forge.Client. +// +// forge.Client already covers this surface for GitHub and GitLab, but it +// stays scoped to git-hosting operations — Jira is explicitly not a forge +// (it has no branches, pull requests, or CI). Keying by a single project +// string lets a future Jira implementation use its natural issue key +// (PROJECT-123) instead of forcing an owner/repo split that Jira doesn't +// have. +// +// This package only defines the interface and thin adapters over +// forge.Client (see ForgeClient). Nothing calls tracker.Client yet. +package tracker + +import "context" + +// Issue represents an issue's content, independent of the tracker backend. +type Issue struct { + Number int + Title string + Body string + URL string + Labels []string +} + +// Comment represents a comment on an issue. +// +// ID is a string rather than an int because not every tracker uses numeric +// comment IDs. Callers that need to update or delete a comment pass the ID +// back verbatim via UpdateComment. +type Comment struct { + ID string + HTMLURL string + Body string + Author string + CreatedAt string +} + +// Client abstracts issue-content read/write operations across trackers +// (GitHub, GitLab, and eventually Jira). Project identifies the issue's +// container: "owner/repo" for GitHub/GitLab, a Jira project key for Jira. +type Client interface { + GetIssue(ctx context.Context, project string, number int) (*Issue, error) + ListComments(ctx context.Context, project string, number int) ([]Comment, error) + CreateComment(ctx context.Context, project string, number int, body string) (*Comment, error) + UpdateComment(ctx context.Context, project string, commentID string, body string) error +} diff --git a/internal/tracker/tracker_test.go b/internal/tracker/tracker_test.go new file mode 100644 index 0000000000..4fd365e61e --- /dev/null +++ b/internal/tracker/tracker_test.go @@ -0,0 +1,148 @@ +package tracker + +import ( + "context" + "testing" + + "github.com/fullsend-ai/fullsend/internal/forge" +) + +func TestSplitProject(t *testing.T) { + tests := []struct { + input string + wantOwner string + wantRepo string + }{ + {"org/project", "org", "project"}, + {"group/subgroup/project", "group/subgroup", "project"}, + {"project", "", "project"}, + } + for _, tc := range tests { + owner, repo := splitProject(tc.input) + if owner != tc.wantOwner || repo != tc.wantRepo { + t.Errorf("splitProject(%q) = (%q, %q), want (%q, %q)", + tc.input, owner, repo, tc.wantOwner, tc.wantRepo) + } + } +} + +func TestForgeClient_GetIssue(t *testing.T) { + fc := forge.NewFakeClient() + fc.OpenIssues = map[string][]forge.Issue{ + "acme/widgets": { + {Number: 42, Title: "Widget is broken", Body: "details", URL: "https://example.com/42", Labels: []string{"bug"}}, + }, + } + + c := NewForgeClient(fc) + issue, err := c.GetIssue(context.Background(), "acme/widgets", 42) + if err != nil { + t.Fatalf("GetIssue returned error: %v", err) + } + if issue.Number != 42 || issue.Title != "Widget is broken" || issue.Body != "details" || issue.URL != "https://example.com/42" { + t.Errorf("GetIssue returned unexpected issue: %+v", issue) + } + if len(issue.Labels) != 1 || issue.Labels[0] != "bug" { + t.Errorf("GetIssue returned unexpected labels: %+v", issue.Labels) + } +} + +func TestForgeClient_GetIssue_NestedNamespace(t *testing.T) { + fc := forge.NewFakeClient() + fc.OpenIssues = map[string][]forge.Issue{ + "group/subgroup/project": { + {Number: 1, Title: "Nested"}, + }, + } + + c := NewForgeClient(fc) + issue, err := c.GetIssue(context.Background(), "group/subgroup/project", 1) + if err != nil { + t.Fatalf("GetIssue returned error: %v", err) + } + if issue.Title != "Nested" { + t.Errorf("GetIssue returned unexpected issue: %+v", issue) + } +} + +func TestForgeClient_GetIssue_NotFound(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + _, err := c.GetIssue(context.Background(), "acme/widgets", 99) + if !forge.IsNotFound(err) { + t.Errorf("GetIssue error = %v, want forge.ErrNotFound", err) + } +} + +func TestForgeClient_CreateAndListComments(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-bot" + + c := NewForgeClient(fc) + ctx := context.Background() + + created, err := c.CreateComment(ctx, "acme/widgets", 42, "hello there") + if err != nil { + t.Fatalf("CreateComment returned error: %v", err) + } + if created.Body != "hello there" || created.Author != "fullsend-bot" || created.ID == "" { + t.Errorf("CreateComment returned unexpected comment: %+v", created) + } + + comments, err := c.ListComments(ctx, "acme/widgets", 42) + if err != nil { + t.Fatalf("ListComments returned error: %v", err) + } + if len(comments) != 1 || comments[0].ID != created.ID || comments[0].Body != "hello there" { + t.Errorf("ListComments returned unexpected comments: %+v", comments) + } +} + +func TestForgeClient_UpdateComment(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + ctx := context.Background() + + created, err := c.CreateComment(ctx, "acme/widgets", 42, "original") + if err != nil { + t.Fatalf("CreateComment returned error: %v", err) + } + + if err := c.UpdateComment(ctx, "acme/widgets", created.ID, "updated"); err != nil { + t.Fatalf("UpdateComment returned error: %v", err) + } + + comments, err := c.ListComments(ctx, "acme/widgets", 42) + if err != nil { + t.Fatalf("ListComments returned error: %v", err) + } + if len(comments) != 1 || comments[0].Body != "updated" { + t.Errorf("ListComments after update returned unexpected comments: %+v", comments) + } +} + +func TestForgeClient_UpdateComment_InvalidID(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + + err := c.UpdateComment(context.Background(), "acme/widgets", "not-a-number", "updated") + if err == nil { + t.Fatal("UpdateComment with non-numeric ID should return an error") + } +} + +// staticClient is a minimal tracker.Client implementation used to verify +// the interface shape independent of the forge adapter. +type staticClient struct{} + +func (staticClient) GetIssue(_ context.Context, _ string, _ int) (*Issue, error) { return nil, nil } +func (staticClient) ListComments(_ context.Context, _ string, _ int) ([]Comment, error) { + return nil, nil +} +func (staticClient) CreateComment(_ context.Context, _ string, _ int, _ string) (*Comment, error) { + return nil, nil +} +func (staticClient) UpdateComment(_ context.Context, _ string, _ string, _ string) error { return nil } + +var _ Client = staticClient{} +var _ Client = (*ForgeClient)(nil) From 690c9a65434614518736f3b728095484b9655c37 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Thu, 6 Aug 2026 14:57:43 -0400 Subject: [PATCH 02/10] refactor(#5988): add number to tracker.Client.UpdateComment Jira's comment update endpoint (PUT /issue/{issueIdOrKey}/comment/{commentId}) needs the issue key, not just a comment ID, unlike GitHub/GitLab where a comment ID alone is enough. Add number to UpdateComment so a future Jira tracker.Client can reconstruct PROJECT-123; ForgeClient ignores it since forge.Client.UpdateIssueComment doesn't need it. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/tracker/forge_client.go | 7 +++++-- internal/tracker/tracker.go | 8 +++++++- internal/tracker/tracker_test.go | 8 +++++--- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/internal/tracker/forge_client.go b/internal/tracker/forge_client.go index 533c879c73..b1d9fc6803 100644 --- a/internal/tracker/forge_client.go +++ b/internal/tracker/forge_client.go @@ -65,8 +65,11 @@ func (c *ForgeClient) CreateComment(ctx context.Context, project string, number return &result, nil } -// UpdateComment implements Client. -func (c *ForgeClient) UpdateComment(ctx context.Context, project string, commentID string, body string) error { +// UpdateComment implements Client. number is unused: forge.Client's +// UpdateIssueComment identifies the comment by ID alone (GitHub/GitLab +// comment IDs are globally unique within the repo), unlike Jira which +// needs the issue key too. +func (c *ForgeClient) UpdateComment(ctx context.Context, project string, number int, commentID string, body string) error { owner, repo := splitProject(project) id, err := strconv.Atoi(commentID) if err != nil { diff --git a/internal/tracker/tracker.go b/internal/tracker/tracker.go index 2fd56bea18..8e8bffcfa7 100644 --- a/internal/tracker/tracker.go +++ b/internal/tracker/tracker.go @@ -45,5 +45,11 @@ type Client interface { GetIssue(ctx context.Context, project string, number int) (*Issue, error) ListComments(ctx context.Context, project string, number int) ([]Comment, error) CreateComment(ctx context.Context, project string, number int, body string) (*Comment, error) - UpdateComment(ctx context.Context, project string, commentID string, body string) error + // UpdateComment updates the comment identified by commentID on the + // issue (project, number). number is redundant for trackers whose + // comment IDs are globally unique (GitHub, GitLab), but Jira's + // update-comment endpoint requires the issue key alongside the + // comment ID, so it's part of the interface rather than left to a + // Jira-specific workaround. + UpdateComment(ctx context.Context, project string, number int, commentID string, body string) error } diff --git a/internal/tracker/tracker_test.go b/internal/tracker/tracker_test.go index 4fd365e61e..886c2b74d1 100644 --- a/internal/tracker/tracker_test.go +++ b/internal/tracker/tracker_test.go @@ -108,7 +108,7 @@ func TestForgeClient_UpdateComment(t *testing.T) { t.Fatalf("CreateComment returned error: %v", err) } - if err := c.UpdateComment(ctx, "acme/widgets", created.ID, "updated"); err != nil { + if err := c.UpdateComment(ctx, "acme/widgets", 42, created.ID, "updated"); err != nil { t.Fatalf("UpdateComment returned error: %v", err) } @@ -125,7 +125,7 @@ func TestForgeClient_UpdateComment_InvalidID(t *testing.T) { fc := forge.NewFakeClient() c := NewForgeClient(fc) - err := c.UpdateComment(context.Background(), "acme/widgets", "not-a-number", "updated") + err := c.UpdateComment(context.Background(), "acme/widgets", 42, "not-a-number", "updated") if err == nil { t.Fatal("UpdateComment with non-numeric ID should return an error") } @@ -142,7 +142,9 @@ func (staticClient) ListComments(_ context.Context, _ string, _ int) ([]Comment, func (staticClient) CreateComment(_ context.Context, _ string, _ int, _ string) (*Comment, error) { return nil, nil } -func (staticClient) UpdateComment(_ context.Context, _ string, _ string, _ string) error { return nil } +func (staticClient) UpdateComment(_ context.Context, _ string, _ int, _ string, _ string) error { + return nil +} var _ Client = staticClient{} var _ Client = (*ForgeClient)(nil) From 70a15d614cfb4c5268ca40939350aed637f2ed29 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Thu, 6 Aug 2026 16:00:09 -0400 Subject: [PATCH 03/10] fix(#5988): validate project string, fix tracker doc inconsistencies splitProject now returns an error for malformed project strings ("project", "/repo", "owner/") instead of silently forwarding an empty owner or repo into forge.Client calls. Also fixes two doc comments in tracker.go: the package doc described a full Jira issue key ("PROJECT-123") as the project string, contradicting the interface's separate number param; and the Comment.ID doc claimed UpdateComment could delete a comment (there's no DeleteComment) and overstated that comment IDs are non-numeric across trackers. Addresses review feedback from qodo-code-review and waynesun09 on PR #5993. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/tracker/forge_client.go | 34 +++++++++++++++++++++++++------- internal/tracker/tracker.go | 15 ++++++++------ internal/tracker/tracker_test.go | 22 +++++++++++++++++---- 3 files changed, 54 insertions(+), 17 deletions(-) diff --git a/internal/tracker/forge_client.go b/internal/tracker/forge_client.go index b1d9fc6803..b6b7c10a43 100644 --- a/internal/tracker/forge_client.go +++ b/internal/tracker/forge_client.go @@ -26,7 +26,10 @@ func NewForgeClient(fc forge.Client) *ForgeClient { // GetIssue implements Client. func (c *ForgeClient) GetIssue(ctx context.Context, project string, number int) (*Issue, error) { - owner, repo := splitProject(project) + owner, repo, err := splitProject(project) + if err != nil { + return nil, err + } issue, err := c.forge.GetIssue(ctx, owner, repo, number) if err != nil { return nil, err @@ -42,7 +45,10 @@ func (c *ForgeClient) GetIssue(ctx context.Context, project string, number int) // ListComments implements Client. func (c *ForgeClient) ListComments(ctx context.Context, project string, number int) ([]Comment, error) { - owner, repo := splitProject(project) + owner, repo, err := splitProject(project) + if err != nil { + return nil, err + } comments, err := c.forge.ListIssueComments(ctx, owner, repo, number) if err != nil { return nil, err @@ -56,7 +62,10 @@ func (c *ForgeClient) ListComments(ctx context.Context, project string, number i // CreateComment implements Client. func (c *ForgeClient) CreateComment(ctx context.Context, project string, number int, body string) (*Comment, error) { - owner, repo := splitProject(project) + owner, repo, err := splitProject(project) + if err != nil { + return nil, err + } comment, err := c.forge.CreateIssueComment(ctx, owner, repo, number, body) if err != nil { return nil, err @@ -70,7 +79,10 @@ func (c *ForgeClient) CreateComment(ctx context.Context, project string, number // comment IDs are globally unique within the repo), unlike Jira which // needs the issue key too. func (c *ForgeClient) UpdateComment(ctx context.Context, project string, number int, commentID string, body string) error { - owner, repo := splitProject(project) + owner, repo, err := splitProject(project) + if err != nil { + return err + } id, err := strconv.Atoi(commentID) if err != nil { return fmt.Errorf("tracker: comment ID %q is not numeric: %w", commentID, err) @@ -93,10 +105,18 @@ func fromForgeComment(c forge.IssueComment) Comment { // ("owner/repo"), which this also handles correctly since there's only one // "/". GitLab projects may be nested under subgroups, hence splitting on // the last "/" rather than the first. -func splitProject(project string) (owner, repo string) { +// +// It returns an error if project doesn't split into a non-empty owner and +// a non-empty repo, so callers don't silently forward malformed values +// (e.g. missing owner or repo) into forge.Client calls that require both. +func splitProject(project string) (owner, repo string, err error) { idx := strings.LastIndex(project, "/") if idx < 0 { - return "", project + return "", "", fmt.Errorf("tracker: invalid project %q: expected \"owner/repo\"", project) + } + owner, repo = project[:idx], project[idx+1:] + if owner == "" || repo == "" { + return "", "", fmt.Errorf("tracker: invalid project %q: owner and repo must both be non-empty", project) } - return project[:idx], project[idx+1:] + return owner, repo, nil } diff --git a/internal/tracker/tracker.go b/internal/tracker/tracker.go index 8e8bffcfa7..0fb4e4a866 100644 --- a/internal/tracker/tracker.go +++ b/internal/tracker/tracker.go @@ -6,9 +6,10 @@ // forge.Client already covers this surface for GitHub and GitLab, but it // stays scoped to git-hosting operations — Jira is explicitly not a forge // (it has no branches, pull requests, or CI). Keying by a single project -// string lets a future Jira implementation use its natural issue key -// (PROJECT-123) instead of forcing an owner/repo split that Jira doesn't -// have. +// string lets a future Jira implementation use its natural project key +// (e.g. "PROJECT") instead of forcing an owner/repo split that Jira +// doesn't have; the issue number is passed separately, as with GitHub and +// GitLab. // // This package only defines the interface and thin adapters over // forge.Client (see ForgeClient). Nothing calls tracker.Client yet. @@ -27,9 +28,11 @@ type Issue struct { // Comment represents a comment on an issue. // -// ID is a string rather than an int because not every tracker uses numeric -// comment IDs. Callers that need to update or delete a comment pass the ID -// back verbatim via UpdateComment. +// ID is a string for JSON round-tripping safety and to allow for +// non-numeric IDs from a possible future tracker, even though GitHub, +// GitLab, and Jira comment IDs are all numeric under the hood. Callers +// that need to update a comment pass the ID back verbatim via +// UpdateComment. type Comment struct { ID string HTMLURL string diff --git a/internal/tracker/tracker_test.go b/internal/tracker/tracker_test.go index 886c2b74d1..fcac90f6a1 100644 --- a/internal/tracker/tracker_test.go +++ b/internal/tracker/tracker_test.go @@ -12,13 +12,27 @@ func TestSplitProject(t *testing.T) { input string wantOwner string wantRepo string + wantErr bool }{ - {"org/project", "org", "project"}, - {"group/subgroup/project", "group/subgroup", "project"}, - {"project", "", "project"}, + {input: "org/project", wantOwner: "org", wantRepo: "project"}, + {input: "group/subgroup/project", wantOwner: "group/subgroup", wantRepo: "project"}, + {input: "project", wantErr: true}, + {input: "/repo", wantErr: true}, + {input: "owner/", wantErr: true}, + {input: "", wantErr: true}, } for _, tc := range tests { - owner, repo := splitProject(tc.input) + owner, repo, err := splitProject(tc.input) + if tc.wantErr { + if err == nil { + t.Errorf("splitProject(%q) = (%q, %q, ), want error", tc.input, owner, repo) + } + continue + } + if err != nil { + t.Errorf("splitProject(%q) returned unexpected error: %v", tc.input, err) + continue + } if owner != tc.wantOwner || repo != tc.wantRepo { t.Errorf("splitProject(%q) = (%q, %q), want (%q, %q)", tc.input, owner, repo, tc.wantOwner, tc.wantRepo) From ebd1662be5b4b214b551300586fa9e7a5b640e78 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Thu, 6 Aug 2026 16:58:24 -0400 Subject: [PATCH 04/10] feat(#5988): add tracker.ErrNotFound to define Client's NotFound contract The Client interface previously said nothing about error semantics, so a future consumer coded against tracker.Client alone had no forge-agnostic way to detect "not found" without reaching into internal/forge directly -- defeating the point of the abstraction. Add tracker.ErrNotFound / IsNotFound, document that implementations must satisfy it, and have ForgeClient translate forge.ErrNotFound into it via a small wrapNotFound helper applied to all four methods. Addresses review feedback from waynesun09 on PR #5993. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/tracker/forge_client.go | 20 ++++++++++++++++---- internal/tracker/tracker.go | 21 ++++++++++++++++++++- internal/tracker/tracker_test.go | 5 ++++- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/internal/tracker/forge_client.go b/internal/tracker/forge_client.go index b6b7c10a43..44abef7a88 100644 --- a/internal/tracker/forge_client.go +++ b/internal/tracker/forge_client.go @@ -32,7 +32,7 @@ func (c *ForgeClient) GetIssue(ctx context.Context, project string, number int) } issue, err := c.forge.GetIssue(ctx, owner, repo, number) if err != nil { - return nil, err + return nil, wrapNotFound(err) } return &Issue{ Number: issue.Number, @@ -51,7 +51,7 @@ func (c *ForgeClient) ListComments(ctx context.Context, project string, number i } comments, err := c.forge.ListIssueComments(ctx, owner, repo, number) if err != nil { - return nil, err + return nil, wrapNotFound(err) } result := make([]Comment, len(comments)) for i, fc := range comments { @@ -68,7 +68,7 @@ func (c *ForgeClient) CreateComment(ctx context.Context, project string, number } comment, err := c.forge.CreateIssueComment(ctx, owner, repo, number, body) if err != nil { - return nil, err + return nil, wrapNotFound(err) } result := fromForgeComment(*comment) return &result, nil @@ -87,7 +87,19 @@ func (c *ForgeClient) UpdateComment(ctx context.Context, project string, number if err != nil { return fmt.Errorf("tracker: comment ID %q is not numeric: %w", commentID, err) } - return c.forge.UpdateIssueComment(ctx, owner, repo, id, body) + return wrapNotFound(c.forge.UpdateIssueComment(ctx, owner, repo, id, body)) +} + +// wrapNotFound translates a forge.ErrNotFound-satisfying error into one +// that also satisfies tracker.ErrNotFound, so ForgeClient upholds the +// Client interface's NotFound contract without leaking forge as part of +// tracker.Client's error surface. Non-NotFound errors, including nil, +// pass through unchanged. +func wrapNotFound(err error) error { + if !forge.IsNotFound(err) { + return err + } + return fmt.Errorf("%w: %w", ErrNotFound, err) } func fromForgeComment(c forge.IssueComment) Comment { diff --git a/internal/tracker/tracker.go b/internal/tracker/tracker.go index 0fb4e4a866..0198cb731f 100644 --- a/internal/tracker/tracker.go +++ b/internal/tracker/tracker.go @@ -15,7 +15,23 @@ // forge.Client (see ForgeClient). Nothing calls tracker.Client yet. package tracker -import "context" +import ( + "context" + "errors" +) + +// ErrNotFound indicates a requested issue or comment was not found. +// Implementations of Client must return an error satisfying errors.Is(err, +// ErrNotFound) — checkable via IsNotFound — for missing resources, rather +// than requiring callers to reach into a specific tracker backend (e.g. +// forge.ErrNotFound) to detect this case. +var ErrNotFound = errors.New("not found") + +// IsNotFound reports whether err indicates a requested issue or comment +// was not found. +func IsNotFound(err error) bool { + return errors.Is(err, ErrNotFound) +} // Issue represents an issue's content, independent of the tracker backend. type Issue struct { @@ -44,6 +60,9 @@ type Comment struct { // Client abstracts issue-content read/write operations across trackers // (GitHub, GitLab, and eventually Jira). Project identifies the issue's // container: "owner/repo" for GitHub/GitLab, a Jira project key for Jira. +// +// Implementations must return an error satisfying IsNotFound when the +// requested issue or comment doesn't exist. type Client interface { GetIssue(ctx context.Context, project string, number int) (*Issue, error) ListComments(ctx context.Context, project string, number int) ([]Comment, error) diff --git a/internal/tracker/tracker_test.go b/internal/tracker/tracker_test.go index fcac90f6a1..6c7f561176 100644 --- a/internal/tracker/tracker_test.go +++ b/internal/tracker/tracker_test.go @@ -83,8 +83,11 @@ func TestForgeClient_GetIssue_NotFound(t *testing.T) { fc := forge.NewFakeClient() c := NewForgeClient(fc) _, err := c.GetIssue(context.Background(), "acme/widgets", 99) + if !IsNotFound(err) { + t.Errorf("GetIssue error = %v, want tracker.ErrNotFound", err) + } if !forge.IsNotFound(err) { - t.Errorf("GetIssue error = %v, want forge.ErrNotFound", err) + t.Errorf("GetIssue error = %v, want it to still satisfy forge.ErrNotFound", err) } } From 082fee886b1b6e910f79079761903b0bc1d20eaa Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Fri, 7 Aug 2026 15:17:04 +0000 Subject: [PATCH 05/10] fix: address review feedback on PR #5993 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Condense UpdateComment doc comment from 5 lines to 2 lines, matching the compact style of the other Client interface methods while retaining the key rationale (Jira requires the issue key to update a comment). PR title convention (feat → refactor) noted but requires manual change since sandbox cannot mutate the PR title. Addresses review feedback on #5993 --- internal/tracker/tracker.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/internal/tracker/tracker.go b/internal/tracker/tracker.go index 0198cb731f..1ab734c0ed 100644 --- a/internal/tracker/tracker.go +++ b/internal/tracker/tracker.go @@ -67,11 +67,7 @@ type Client interface { GetIssue(ctx context.Context, project string, number int) (*Issue, error) ListComments(ctx context.Context, project string, number int) ([]Comment, error) CreateComment(ctx context.Context, project string, number int, body string) (*Comment, error) - // UpdateComment updates the comment identified by commentID on the - // issue (project, number). number is redundant for trackers whose - // comment IDs are globally unique (GitHub, GitLab), but Jira's - // update-comment endpoint requires the issue key alongside the - // comment ID, so it's part of the interface rather than left to a - // Jira-specific workaround. + // UpdateComment updates the body of commentID on the issue (project, number). + // number is included because Jira requires the issue key to update a comment. UpdateComment(ctx context.Context, project string, number int, commentID string, body string) error } From bed9407a91fb1a0e199e5fe52df316d783990e5a Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Fri, 7 Aug 2026 15:42:27 +0000 Subject: [PATCH 06/10] fix: address review feedback on PR #5993 Add doc comments to GetIssue, ListComments, and CreateComment in the Client interface and ForgeClient methods to match the existing UpdateComment documentation style. Addresses review feedback on #5993 --- internal/tracker/forge_client.go | 9 ++++++--- internal/tracker/tracker.go | 3 +++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/internal/tracker/forge_client.go b/internal/tracker/forge_client.go index 44abef7a88..18f251fa32 100644 --- a/internal/tracker/forge_client.go +++ b/internal/tracker/forge_client.go @@ -24,7 +24,8 @@ func NewForgeClient(fc forge.Client) *ForgeClient { return &ForgeClient{forge: fc} } -// GetIssue implements Client. +// GetIssue implements Client by splitting project into owner/repo for the +// underlying forge call. func (c *ForgeClient) GetIssue(ctx context.Context, project string, number int) (*Issue, error) { owner, repo, err := splitProject(project) if err != nil { @@ -43,7 +44,8 @@ func (c *ForgeClient) GetIssue(ctx context.Context, project string, number int) }, nil } -// ListComments implements Client. +// ListComments implements Client by splitting project into owner/repo for +// the underlying forge call. func (c *ForgeClient) ListComments(ctx context.Context, project string, number int) ([]Comment, error) { owner, repo, err := splitProject(project) if err != nil { @@ -60,7 +62,8 @@ func (c *ForgeClient) ListComments(ctx context.Context, project string, number i return result, nil } -// CreateComment implements Client. +// CreateComment implements Client by splitting project into owner/repo for +// the underlying forge call. func (c *ForgeClient) CreateComment(ctx context.Context, project string, number int, body string) (*Comment, error) { owner, repo, err := splitProject(project) if err != nil { diff --git a/internal/tracker/tracker.go b/internal/tracker/tracker.go index 1ab734c0ed..13dd0122f7 100644 --- a/internal/tracker/tracker.go +++ b/internal/tracker/tracker.go @@ -64,8 +64,11 @@ type Comment struct { // Implementations must return an error satisfying IsNotFound when the // requested issue or comment doesn't exist. type Client interface { + // GetIssue returns the issue identified by project and number. GetIssue(ctx context.Context, project string, number int) (*Issue, error) + // ListComments returns all comments on the issue identified by project and number. ListComments(ctx context.Context, project string, number int) ([]Comment, error) + // CreateComment adds a new comment with the given body to the issue. CreateComment(ctx context.Context, project string, number int, body string) (*Comment, error) // UpdateComment updates the body of commentID on the issue (project, number). // number is included because Jira requires the issue key to update a comment. From 2ec6124ba834da13d4be7bd4ffb6dc937cf603c9 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Mon, 10 Aug 2026 10:55:48 -0400 Subject: [PATCH 07/10] fix(#5988): correct GitLab UpdateComment doc claim in ForgeClient The comment claimed GitLab comment IDs are globally unique like GitHub's, but GitLab's Notes API actually requires the issue/MR IID to address a note directly (see gitlab.LiveClient.updateOrDeleteNote's scan-based workaround). forge.Client's UpdateIssueComment doesn't expose that IID, so ForgeClient's GitLab path still hits the scan; the doc now says so instead of implying GitLab doesn't need the number at all. Assisted-by: Claude Sonnet 5 Signed-off-by: Ralph Bean --- internal/tracker/forge_client.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/tracker/forge_client.go b/internal/tracker/forge_client.go index 18f251fa32..86bd0a879c 100644 --- a/internal/tracker/forge_client.go +++ b/internal/tracker/forge_client.go @@ -77,10 +77,14 @@ func (c *ForgeClient) CreateComment(ctx context.Context, project string, number return &result, nil } -// UpdateComment implements Client. number is unused: forge.Client's -// UpdateIssueComment identifies the comment by ID alone (GitHub/GitLab -// comment IDs are globally unique within the repo), unlike Jira which -// needs the issue key too. +// UpdateComment implements Client. number is unused here: forge.Client's +// UpdateIssueComment takes only a comment ID, which is sufficient for +// GitHub (comment IDs are globally unique within the repo). GitLab +// actually needs the issue/MR IID to address a note directly — see +// gitlab.LiveClient.updateOrDeleteNote — but forge.Client's +// UpdateIssueComment doesn't expose one, so the GitLab path still falls +// back to that method's documented scan. Jira needs the issue key for an +// unrelated reason: it isn't a forge.Client implementation at all. func (c *ForgeClient) UpdateComment(ctx context.Context, project string, number int, commentID string, body string) error { owner, repo, err := splitProject(project) if err != nil { From 30a739bcfd4ae0fd33edc7db96124cd0692a2bb2 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Mon, 10 Aug 2026 10:55:56 -0400 Subject: [PATCH 08/10] fix(#5988): return NotFound from FakeClient.UpdateIssueComment, add tracker NotFound tests FakeClient.UpdateIssueComment returned nil for an unknown comment ID instead of forge.ErrNotFound, which meant nothing exercised ForgeClient.UpdateComment's wrapNotFound path through the fake. Also add the missing NotFound-contract tests for ListComments, CreateComment, and UpdateComment (only GetIssue had one). Assisted-by: Claude Sonnet 5 Signed-off-by: Ralph Bean --- internal/forge/fake.go | 2 +- internal/tracker/tracker_test.go | 38 ++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/internal/forge/fake.go b/internal/forge/fake.go index e6a7220638..45b8be144e 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -1382,7 +1382,7 @@ func (f *FakeClient) UpdateIssueComment(_ context.Context, owner, repo string, c } } } - return nil + return fmt.Errorf("%w: comment %d", ErrNotFound, commentID) } func (f *FakeClient) DeleteIssueComment(_ context.Context, _, _ string, commentID int) error { diff --git a/internal/tracker/tracker_test.go b/internal/tracker/tracker_test.go index 6c7f561176..395b688640 100644 --- a/internal/tracker/tracker_test.go +++ b/internal/tracker/tracker_test.go @@ -91,6 +91,44 @@ func TestForgeClient_GetIssue_NotFound(t *testing.T) { } } +func TestForgeClient_ListComments_NotFound(t *testing.T) { + fc := forge.NewFakeClient() + fc.Errors = map[string]error{"ListIssueComments": forge.ErrNotFound} + c := NewForgeClient(fc) + _, err := c.ListComments(context.Background(), "acme/widgets", 42) + if !IsNotFound(err) { + t.Errorf("ListComments error = %v, want tracker.ErrNotFound", err) + } + if !forge.IsNotFound(err) { + t.Errorf("ListComments error = %v, want it to still satisfy forge.ErrNotFound", err) + } +} + +func TestForgeClient_CreateComment_NotFound(t *testing.T) { + fc := forge.NewFakeClient() + fc.Errors = map[string]error{"CreateIssueComment": forge.ErrNotFound} + c := NewForgeClient(fc) + _, err := c.CreateComment(context.Background(), "acme/widgets", 42, "hello") + if !IsNotFound(err) { + t.Errorf("CreateComment error = %v, want tracker.ErrNotFound", err) + } + if !forge.IsNotFound(err) { + t.Errorf("CreateComment error = %v, want it to still satisfy forge.ErrNotFound", err) + } +} + +func TestForgeClient_UpdateComment_NotFound(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + err := c.UpdateComment(context.Background(), "acme/widgets", 42, "99", "updated") + if !IsNotFound(err) { + t.Errorf("UpdateComment error = %v, want tracker.ErrNotFound", err) + } + if !forge.IsNotFound(err) { + t.Errorf("UpdateComment error = %v, want it to still satisfy forge.ErrNotFound", err) + } +} + func TestForgeClient_CreateAndListComments(t *testing.T) { fc := forge.NewFakeClient() fc.AuthenticatedUser = "fullsend-bot" From 4633d49afa62f98363f3fef235e0b9b73c6a3eb6 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Mon, 10 Aug 2026 12:06:07 -0400 Subject: [PATCH 09/10] fix(#5988): avoid stuttered "not found: not found" in wrapNotFound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit forge.ErrNotFound's own text is "not found", so wrapping it with fmt.Errorf("%w: %w", ErrNotFound, err) produced doubled text like "not found: not found" or "not found: not found: comment 42" whenever a ForgeClient method's Error() was rendered directly (CLI output, logs). errors.Is/IsNotFound checks were unaffected, but the surfaced message was wrong. notFoundError wraps the forge error without repeating the sentinel text — its Error() returns the forge error's message verbatim, while Unwrap() []error still satisfies both tracker.IsNotFound and forge.IsNotFound. Reported by waynesun09 on PR #5993. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/tracker/forge_client.go | 12 +++++++++++- internal/tracker/tracker_test.go | 19 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/internal/tracker/forge_client.go b/internal/tracker/forge_client.go index 86bd0a879c..18b934523e 100644 --- a/internal/tracker/forge_client.go +++ b/internal/tracker/forge_client.go @@ -106,9 +106,19 @@ func wrapNotFound(err error) error { if !forge.IsNotFound(err) { return err } - return fmt.Errorf("%w: %w", ErrNotFound, err) + return ¬FoundError{err: err} } +// notFoundError makes a forge error also satisfy tracker.IsNotFound +// without repeating "not found" in its message — the wrapped forge error's +// text already says that. +type notFoundError struct { + err error +} + +func (e *notFoundError) Error() string { return e.err.Error() } +func (e *notFoundError) Unwrap() []error { return []error{ErrNotFound, e.err} } + func fromForgeComment(c forge.IssueComment) Comment { return Comment{ ID: strconv.Itoa(c.ID), diff --git a/internal/tracker/tracker_test.go b/internal/tracker/tracker_test.go index 395b688640..7fdf2bdc4c 100644 --- a/internal/tracker/tracker_test.go +++ b/internal/tracker/tracker_test.go @@ -2,6 +2,7 @@ package tracker import ( "context" + "strings" "testing" "github.com/fullsend-ai/fullsend/internal/forge" @@ -91,6 +92,24 @@ func TestForgeClient_GetIssue_NotFound(t *testing.T) { } } +func TestForgeClient_GetIssue_NotFound_NoStutteredMessage(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + _, err := c.GetIssue(context.Background(), "acme/widgets", 99) + if got := err.Error(); strings.Count(got, "not found") != 1 { + t.Errorf("GetIssue error = %q, want \"not found\" to appear exactly once", got) + } +} + +func TestForgeClient_UpdateComment_NotFound_NoStutteredMessage(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + err := c.UpdateComment(context.Background(), "acme/widgets", 42, "99", "updated") + if got := err.Error(); strings.Count(got, "not found") != 1 { + t.Errorf("UpdateComment error = %q, want \"not found\" to appear exactly once", got) + } +} + func TestForgeClient_ListComments_NotFound(t *testing.T) { fc := forge.NewFakeClient() fc.Errors = map[string]error{"ListIssueComments": forge.ErrNotFound} From 4ebdbeaa535b1df441c50386de7fcb63c156249e Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Mon, 10 Aug 2026 12:07:30 -0400 Subject: [PATCH 10/10] fix(#5988): give tracker.Comment/Issue.Body a named Body type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit waynesun09 flagged that a plain string Body doesn't account for Jira's ADF requirement — internal/forge/jira already models bodies as `any` for exactly this reason. Confirmed with a live spike against Jira's v3 API: posting a bare string as a comment body is rejected outright ("Comment body is not valid!"). Naively wrapping the raw Markdown string in a single ADF text node is worse than lossy — Jira's plain-text rendering path interprets stray Markdown characters as wiki-markup, so braces in a Go code sample broke the surrounding paragraph and a Markdown link got mangled into a dead in-page anchor. A properly structured ADF payload (codeBlock node, link mark, bulletList) rendered correctly. tracker.Body documents this contract so a future Jira Client implementation is responsible for real Markdown<->ADF conversion, rather than callers or the interface silently assuming a pass-through works. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/tracker/forge_client.go | 12 ++++++------ internal/tracker/tracker.go | 19 +++++++++++++++---- internal/tracker/tracker_test.go | 4 ++-- 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/internal/tracker/forge_client.go b/internal/tracker/forge_client.go index 18b934523e..77f7d3144e 100644 --- a/internal/tracker/forge_client.go +++ b/internal/tracker/forge_client.go @@ -38,7 +38,7 @@ func (c *ForgeClient) GetIssue(ctx context.Context, project string, number int) return &Issue{ Number: issue.Number, Title: issue.Title, - Body: issue.Body, + Body: Body(issue.Body), URL: issue.URL, Labels: issue.Labels, }, nil @@ -64,12 +64,12 @@ func (c *ForgeClient) ListComments(ctx context.Context, project string, number i // CreateComment implements Client by splitting project into owner/repo for // the underlying forge call. -func (c *ForgeClient) CreateComment(ctx context.Context, project string, number int, body string) (*Comment, error) { +func (c *ForgeClient) CreateComment(ctx context.Context, project string, number int, body Body) (*Comment, error) { owner, repo, err := splitProject(project) if err != nil { return nil, err } - comment, err := c.forge.CreateIssueComment(ctx, owner, repo, number, body) + comment, err := c.forge.CreateIssueComment(ctx, owner, repo, number, string(body)) if err != nil { return nil, wrapNotFound(err) } @@ -85,7 +85,7 @@ func (c *ForgeClient) CreateComment(ctx context.Context, project string, number // UpdateIssueComment doesn't expose one, so the GitLab path still falls // back to that method's documented scan. Jira needs the issue key for an // unrelated reason: it isn't a forge.Client implementation at all. -func (c *ForgeClient) UpdateComment(ctx context.Context, project string, number int, commentID string, body string) error { +func (c *ForgeClient) UpdateComment(ctx context.Context, project string, number int, commentID string, body Body) error { owner, repo, err := splitProject(project) if err != nil { return err @@ -94,7 +94,7 @@ func (c *ForgeClient) UpdateComment(ctx context.Context, project string, number if err != nil { return fmt.Errorf("tracker: comment ID %q is not numeric: %w", commentID, err) } - return wrapNotFound(c.forge.UpdateIssueComment(ctx, owner, repo, id, body)) + return wrapNotFound(c.forge.UpdateIssueComment(ctx, owner, repo, id, string(body))) } // wrapNotFound translates a forge.ErrNotFound-satisfying error into one @@ -123,7 +123,7 @@ func fromForgeComment(c forge.IssueComment) Comment { return Comment{ ID: strconv.Itoa(c.ID), HTMLURL: c.HTMLURL, - Body: c.Body, + Body: Body(c.Body), Author: c.Author, CreatedAt: c.CreatedAt, } diff --git a/internal/tracker/tracker.go b/internal/tracker/tracker.go index 13dd0122f7..68cbac9d15 100644 --- a/internal/tracker/tracker.go +++ b/internal/tracker/tracker.go @@ -33,11 +33,22 @@ func IsNotFound(err error) bool { return errors.Is(err, ErrNotFound) } +// Body is Markdown-formatted issue/comment text, as produced by GitHub and +// GitLab. Jira doesn't speak Markdown — its v3 API requires comment and +// description bodies in Atlassian Document Format (ADF) and rejects plain +// strings outright. A Jira Client implementation is responsible for +// converting Body to and from ADF; a naive pass-through (wrapping the raw +// Markdown string in a single ADF text node) doesn't just lose formatting, +// it actively corrupts content — Jira's plain-text rendering path +// interprets stray Markdown characters (e.g. braces in code samples) as +// wiki-markup and mangles the surrounding text. +type Body string + // Issue represents an issue's content, independent of the tracker backend. type Issue struct { Number int Title string - Body string + Body Body URL string Labels []string } @@ -52,7 +63,7 @@ type Issue struct { type Comment struct { ID string HTMLURL string - Body string + Body Body Author string CreatedAt string } @@ -69,8 +80,8 @@ type Client interface { // ListComments returns all comments on the issue identified by project and number. ListComments(ctx context.Context, project string, number int) ([]Comment, error) // CreateComment adds a new comment with the given body to the issue. - CreateComment(ctx context.Context, project string, number int, body string) (*Comment, error) + CreateComment(ctx context.Context, project string, number int, body Body) (*Comment, error) // UpdateComment updates the body of commentID on the issue (project, number). // number is included because Jira requires the issue key to update a comment. - UpdateComment(ctx context.Context, project string, number int, commentID string, body string) error + UpdateComment(ctx context.Context, project string, number int, commentID string, body Body) error } diff --git a/internal/tracker/tracker_test.go b/internal/tracker/tracker_test.go index 7fdf2bdc4c..5827bb8adf 100644 --- a/internal/tracker/tracker_test.go +++ b/internal/tracker/tracker_test.go @@ -213,10 +213,10 @@ func (staticClient) GetIssue(_ context.Context, _ string, _ int) (*Issue, error) func (staticClient) ListComments(_ context.Context, _ string, _ int) ([]Comment, error) { return nil, nil } -func (staticClient) CreateComment(_ context.Context, _ string, _ int, _ string) (*Comment, error) { +func (staticClient) CreateComment(_ context.Context, _ string, _ int, _ Body) (*Comment, error) { return nil, nil } -func (staticClient) UpdateComment(_ context.Context, _ string, _ int, _ string, _ string) error { +func (staticClient) UpdateComment(_ context.Context, _ string, _ int, _ string, _ Body) error { return nil }