From 634d1275a88526a264b62c14681256e30f39b547 Mon Sep 17 00:00:00 2001 From: fullsend-code <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Sat, 29 Aug 2026 20:31:22 +0000 Subject: [PATCH 1/4] refactor(#6767): route status notifications through tracker.Client Status comments and reactions were coupled to forge.Client with owner/repo/number addressing. When a Jira issue triggers a run, the Jira issue number was passed to the forge as a GitHub issue number, producing a 404 because no such GitHub issue exists. Refactor statuscomment.Notifier and ReconcileOrphaned to accept tracker.Client instead of forge.Client. This routes status notifications to whichever tracker originated the run (GitHub, GitLab, or Jira) independently of the forge used for code output. Key changes: - Add DeleteComment to tracker.Client for start-comment cleanup - Add tracker.Reactor interface for optional emoji reactions; ForgeClient implements it, JiraClient does not - Notifier uses (project, number) addressing instead of (owner, repo, number) to match tracker.Client's model - ClientFactory returns tracker.Client instead of forge.Client - SetTriggerCommentID accepts string (tracker comment IDs are strings for JSON round-tripping safety) - Callers wrap forge.Client in tracker.NewForgeClient() - Add jira.LiveClient.DeleteComment for Jira comment deletion - Add ADR 0093 recording the provenance-based routing decision Pre-commit hooks could not be run in-sandbox (network-restricted); gofmt, go vet, ADR lints, and link checks were run directly. Closes #6767 --- ...093-tracker-routed-status-notifications.md | 144 +++++++ docs/architecture.md | 1 + internal/cli/reconcilestatus.go | 18 +- internal/cli/reconcilestatus_test.go | 8 +- internal/cli/run.go | 14 +- internal/forge/jira/client.go | 9 + internal/statuscomment/statuscomment.go | 128 +++--- internal/statuscomment/statuscomment_test.go | 400 +++++++++--------- internal/tracker/fake_jira.go | 11 + internal/tracker/forge_client.go | 62 +++ internal/tracker/jira_client.go | 7 + internal/tracker/tracker.go | 21 + internal/tracker/tracker_test.go | 4 + 13 files changed, 558 insertions(+), 269 deletions(-) create mode 100644 docs/ADRs/0093-tracker-routed-status-notifications.md diff --git a/docs/ADRs/0093-tracker-routed-status-notifications.md b/docs/ADRs/0093-tracker-routed-status-notifications.md new file mode 100644 index 0000000000..e2636207cb --- /dev/null +++ b/docs/ADRs/0093-tracker-routed-status-notifications.md @@ -0,0 +1,144 @@ +--- +title: "93. Route run-status notifications by event provenance" +status: Accepted +relates_to: + - agent-architecture + - agent-infrastructure +topics: + - tracker + - notifications + - status-comments + - jira + - portability +--- + +# 93. Route run-status notifications by event provenance + +Date: 2026-08-29 + +## Status + +Accepted + +Builds on: + +- Forge abstraction: [ADR 0005](0005-forge-abstraction-layer.md) +- Conversation surface / domain split: [ADR 0086](0086-conversation-surface-for-agent-participation.md) + +## Context + +Agent run-status notifications (start comments, completion comments, emoji +reactions, and orphan reconciliation) were coupled to `forge.Client` with +`owner/repo/number` addressing. This works when the triggering work item +and the code repository live on the same forge, but breaks when an external +issue tracker drives the work. + +The concrete failure: when a Jira issue triggers a code-agent run, the +Jira issue number (e.g. 6964193) is passed to `forge.Client` as a GitHub +issue number. No such GitHub issue exists, producing: + +``` +Failed to post start status: posting start comment: + create issue comment on #6964193: github api: 404 Not Found +``` + +The repository already has a tracker-neutral comment interface +(`tracker.Client`) with adapters for GitHub/GitLab (via `forge.Client`) +and Jira. ADR 0086 established the domain split: `forge.Client` for +git-hosting, `tracker.Client` for issue content, `conversation.Client` +for chat. Status notifications are issue content, not git-hosting +operations, so they belong on `tracker.Client`. + +## Options + +### A. Special-case Jira in the notifier + +Add Jira-specific branching to `statuscomment.Notifier` alongside the +existing `forge.Client` calls. Fast to implement but duplicates the +tracker abstraction and grows worse with each new tracker backend. + +### B. Route status notifications through `tracker.Client` (chosen) + +Replace `forge.Client` with `tracker.Client` in the status-notification +path. Comments route to whichever tracker originated the run. Reactions +become an optional capability (`tracker.Reactor` interface) since not all +trackers support emoji reactions. + +### C. Post status to both the tracker and the forge + +Dual-write so status appears on both the Jira issue and a corresponding +GitHub issue. Requires a matching GitHub issue to exist (or be created), +adds complexity, and doubles API calls for every status update. + +## Decision + +Adopt **Option B**. + +### Interface changes + +1. **`tracker.Client`** gains `DeleteComment` for cleaning up transient + start comments when completion is suppressed. + +2. **`tracker.Reactor`** is a new optional interface: + + ```go + type Reactor interface { + AddIssueReaction(ctx, project, number, content) (id, error) + DeleteIssueReaction(ctx, project, number, reactionID) error + AddCommentReaction(ctx, project, number, commentID, content) (id, error) + DeleteCommentReaction(ctx, project, number, commentID, reactionID) error + } + ``` + + `tracker.ForgeClient` implements `Reactor` (GitHub and GitLab support + emoji reactions). `tracker.JiraClient` does not (Jira has no + equivalent). Consumers type-assert to `Reactor` and silently skip + reaction operations when the tracker does not support them. + +### Notifier changes + +- `statuscomment.Notifier` accepts `tracker.Client` instead of + `forge.Client`. +- Addressing changes from `(owner, repo string, number int)` to + `(project string, number int)` to match `tracker.Client`'s + project-keyed model. +- `ClientFactory` returns `tracker.Client` instead of `forge.Client`. +- `SetTriggerCommentID` accepts `string` (tracker comment IDs are + strings for JSON round-tripping safety and non-numeric ID support). + +### ReconcileOrphaned changes + +- Accepts `tracker.Client` and `(project, number)` instead of + `forge.Client` and `(owner, repo, number)`. +- The `reconcile-status` CLI command wraps its forge client in + `tracker.NewForgeClient()` before calling `ReconcileOrphaned`. + +### Call-site wiring + +Existing forge-based callers (`setupStatusNotifierGitHub`, +`setupStatusNotifierGitLab`, `reconcile-status` command) construct +`tracker.NewForgeClient(forgeClient)` and pass the result. The +`ClientFactory` in the GitHub path returns +`tracker.NewForgeClient(gh.New(mintedToken))` so each token refresh +produces a tracker-wrapped client. + +For Jira-triggered runs, the caller would construct a +`tracker.JiraClient` instead. The plumbing for reading source-system +from the normalized event is a follow-on; the interface is ready for it. + +## Consequences + +- Status comments for Jira-triggered runs can now be routed to Jira + instead of producing a 404 on a non-existent GitHub issue. +- Reactions are silently skipped for trackers that do not support them + (e.g. Jira), rather than failing or falling back to an unrelated forge + issue. +- The `statuscomment` package no longer imports `internal/forge`, + depending only on `internal/tracker` and `internal/config`. +- Adding a new tracker backend (e.g. Linear, Azure DevOps) requires only + implementing `tracker.Client` (and optionally `tracker.Reactor`); + status notifications work automatically. +- The orphan reconciler uses the same tracker-aware routing, so + interrupted runs on Jira issues are also finalized correctly. +- `jira.LiveClient` gains a `DeleteComment` method to satisfy the + extended `tracker.Client` interface. diff --git a/docs/architecture.md b/docs/architecture.md index d3305c6362..c2875f35ee 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -43,6 +43,7 @@ the dedicated org-level `/.fullsend` config repo is deprecated - Forge abstraction: all forge operations go through the `forge.Client` interface, keeping the rest of the codebase forge-agnostic ([ADR 0005](ADRs/0005-forge-abstraction-layer.md)). - Conversation surface: agents participate in GitHub Discussions and later other chat systems through a narrow `conversation.Client` (parallel to `tracker.Client` for issue content), not by extending `forge.Client` ([ADR 0086](ADRs/0086-conversation-surface-for-agent-participation.md)). A **conversation** is the container (Discussion / Slack channel) with exactly one category and optional M:M labels; a **thread** is the top-level message plus replies that share its `parent_id` (`parent_id == id` on the root message). +- Tracker-routed status notifications: run-status comments and reactions route through `tracker.Client` (not `forge.Client`), so the notification destination is determined by event provenance — a Jira-triggered run posts status to Jira, a GitHub-triggered run posts to GitHub. Reactions are an optional `tracker.Reactor` capability; trackers without reaction support (e.g. Jira) silently skip them ([ADR 0093](ADRs/0093-tracker-routed-status-notifications.md)). - Installation model: ordered layer stack (install forward, uninstall reverse, analyze for status reporting) with idempotent operations. Current stack: config-repo → workflows → vendor-binary → secrets → inference → dispatch → enrollment ([ADR 0006](ADRs/0006-ordered-layer-model.md)). - Cross-repo dispatch: enrolled repos call `.fullsend` via `workflow_call`; a dispatch workflow mints OIDC tokens exchanged at a central token mint (GCP Cloud Function or Cloudflare Worker) for scoped GitHub App installation tokens per agent role. App PEM secrets are stored in Secret Manager (GCF mint), Worker secrets (CF mint), or the local filesystem (standalone mint), not the config repo ([ADR 0008](ADRs/0008-workflow-dispatch-for-cross-repo-dispatch.md)). - Shim workflow security: `pull_request_target` prevents PR authors from modifying the shim workflow. No long-lived secrets flow through the shim — OIDC tokens are issued by the GitHub runtime and scoped to the workflow run ([ADR 0009](ADRs/0009-pull-request-target-in-shim-workflows.md)). diff --git a/internal/cli/reconcilestatus.go b/internal/cli/reconcilestatus.go index 780ce44813..76bab84159 100644 --- a/internal/cli/reconcilestatus.go +++ b/internal/cli/reconcilestatus.go @@ -13,6 +13,7 @@ import ( gl "github.com/fullsend-ai/fullsend/internal/forge/gitlab" "github.com/fullsend-ai/fullsend/internal/mintclient" "github.com/fullsend-ai/fullsend/internal/statuscomment" + "github.com/fullsend-ai/fullsend/internal/tracker" ) var reconcileMintToken = mintclient.MintToken @@ -21,6 +22,12 @@ var reconcileNewForgeClient = func(token string) forge.Client { } var reconcileOrphaned = statuscomment.ReconcileOrphaned +// reconcileNewTrackerClient wraps a forge.Client in a tracker.ForgeClient +// for use by ReconcileOrphaned. +var reconcileNewTrackerClient = func(fc forge.Client) tracker.Client { + return tracker.NewForgeClient(fc) +} + func newReconcileStatusCmd() *cobra.Command { var ( repo string @@ -65,21 +72,23 @@ finalized, this is a no-op.`, return err } - var client forge.Client + var forgeClient forge.Client if forgePlatform == "gitlab" { var gitlabErr error - client, gitlabErr = newGitLabClientFromEnv("status reconciliation") + forgeClient, gitlabErr = newGitLabClientFromEnv("status reconciliation") if gitlabErr != nil { return gitlabErr } } else { var githubErr error - client, githubErr = reconcileGitHubClient(cmd, mintURL, role, repoName) + forgeClient, githubErr = reconcileGitHubClient(cmd, mintURL, role, repoName) if githubErr != nil { return githubErr } } + tc := reconcileNewTrackerClient(forgeClient) + var termReason statuscomment.TerminationReason switch reason { case "cancelled": @@ -110,7 +119,8 @@ finalized, this is a no-op.`, agentDescription := titleCase(strings.ReplaceAll(role, "-", " ")) - return reconcileOrphaned(cmd.Context(), client, owner, repoName, number, runID, runURL, sha, termReason, completionMode, jobStatus, wasSkipped, agentDescription) + project := owner + "/" + repoName + return reconcileOrphaned(cmd.Context(), tc, project, number, runID, runURL, sha, termReason, completionMode, jobStatus, wasSkipped, agentDescription) }, } diff --git a/internal/cli/reconcilestatus_test.go b/internal/cli/reconcilestatus_test.go index e2f88c9194..1158afefc8 100644 --- a/internal/cli/reconcilestatus_test.go +++ b/internal/cli/reconcilestatus_test.go @@ -15,6 +15,7 @@ import ( gh "github.com/fullsend-ai/fullsend/internal/forge/github" "github.com/fullsend-ai/fullsend/internal/mintclient" "github.com/fullsend-ai/fullsend/internal/statuscomment" + "github.com/fullsend-ai/fullsend/internal/tracker" ) // gitlabNoteServer returns an httptest.Server that handles GitLab note API calls @@ -322,6 +323,7 @@ func stubReconcileVars(t *testing.T, onReconcile func(completionMode, jobStatus t.Helper() origMint := reconcileMintToken origForge := reconcileNewForgeClient + origTracker := reconcileNewTrackerClient origReconcile := reconcileOrphaned reconcileMintToken = func(_ context.Context, _ mintclient.MintRequest) (*mintclient.MintResult, error) { @@ -335,13 +337,17 @@ func stubReconcileVars(t *testing.T, onReconcile func(completionMode, jobStatus t.Cleanup(srv.Close) return gh.New(token).WithBaseURL(srv.URL) } - reconcileOrphaned = func(_ context.Context, _ forge.Client, _, _ string, _ int, _, _, _ string, _ statuscomment.TerminationReason, completionMode, jobStatus string, wasSkipped bool, agentDescription string) error { + reconcileNewTrackerClient = func(fc forge.Client) tracker.Client { + return tracker.NewForgeClient(fc) + } + reconcileOrphaned = func(_ context.Context, _ tracker.Client, _ string, _ int, _, _, _ string, _ statuscomment.TerminationReason, completionMode, jobStatus string, wasSkipped bool, agentDescription string) error { onReconcile(completionMode, jobStatus, wasSkipped, agentDescription) return nil } t.Cleanup(func() { reconcileMintToken = origMint reconcileNewForgeClient = origForge + reconcileNewTrackerClient = origTracker reconcileOrphaned = origReconcile }) } diff --git a/internal/cli/run.go b/internal/cli/run.go index 975eb8442b..4965954a3c 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -17,6 +17,7 @@ import ( "regexp" "runtime" "sort" + "strconv" "strings" "sync" "sync/atomic" @@ -50,6 +51,7 @@ import ( "github.com/fullsend-ai/fullsend/internal/security" "github.com/fullsend-ai/fullsend/internal/statuscomment" "github.com/fullsend-ai/fullsend/internal/telemetry" + "github.com/fullsend-ai/fullsend/internal/tracker" "github.com/fullsend-ai/fullsend/internal/ui" "go.opentelemetry.io/otel/attribute" @@ -4271,16 +4273,17 @@ func setupStatusNotifierGitHub(notifyCfg config.StatusNotificationConfig, owner, runID = fmt.Sprintf("%d", time.Now().UnixNano()) } - n := statuscomment.New(nil, notifyCfg, owner, repo, sOpts.statusNum, sOpts.runURL, sha, runID) + project := owner + "/" + repo + n := statuscomment.New(nil, notifyCfg, project, sOpts.statusNum, sOpts.runURL, sha, runID) n.SetWarnFunc(func(format string, args ...any) { printer.StepWarn(fmt.Sprintf(format, args...)) }) if sOpts.statusComment != 0 { - n.SetTriggerCommentID(sOpts.statusComment) + n.SetTriggerCommentID(strconv.Itoa(sOpts.statusComment)) } canonRole := resolveRole(role) - n.SetClientFactory(func(ctx context.Context) (forge.Client, error) { + n.SetClientFactory(func(ctx context.Context) (tracker.Client, error) { result, err := statusMintToken(ctx, mintclient.MintRequest{ MintURL: mintURL, Role: canonRole, @@ -4295,7 +4298,7 @@ func setupStatusNotifierGitHub(notifyCfg config.StatusNotificationConfig, owner, if os.Getenv("GITHUB_ACTIONS") == "true" { fmt.Fprintf(os.Stderr, "::add-mask::%s\n", result.Token) } - return gh.New(result.Token), nil + return tracker.NewForgeClient(gh.New(result.Token)), nil }) return n, nil @@ -4325,7 +4328,8 @@ func setupStatusNotifierGitLab(notifyCfg config.StatusNotificationConfig, owner, runID = fmt.Sprintf("%d", time.Now().UnixNano()) } - n := statuscomment.New(client, notifyCfg, owner, repo, sOpts.statusNum, sOpts.runURL, sha, runID) + project := owner + "/" + repo + n := statuscomment.New(tracker.NewForgeClient(client), notifyCfg, project, sOpts.statusNum, sOpts.runURL, sha, runID) n.SetWarnFunc(func(format string, args ...any) { printer.StepWarn(fmt.Sprintf(format, args...)) }) diff --git a/internal/forge/jira/client.go b/internal/forge/jira/client.go index 9baaf36549..2c9a95eb3b 100644 --- a/internal/forge/jira/client.go +++ b/internal/forge/jira/client.go @@ -512,6 +512,15 @@ func (c *LiveClient) UpdateComment(ctx context.Context, issueIDOrKey, commentID, return nil } +// DeleteComment removes a comment by ID from the given issue. +func (c *LiveClient) DeleteComment(ctx context.Context, issueIDOrKey, commentID string) error { + path := "/issue/" + url.PathEscape(issueIDOrKey) + "/comment/" + url.PathEscape(commentID) + if err := c.do(ctx, http.MethodDelete, path, nil, nil); err != nil { + return fmt.Errorf("delete comment %s on %s: %w", commentID, issueIDOrKey, err) + } + return nil +} + // ListChangelog fetches all changelog entries for an issue, exhausting // pagination up to maxListPages pages. func (c *LiveClient) ListChangelog(ctx context.Context, issueIDOrKey string) ([]ChangelogEntry, error) { diff --git a/internal/statuscomment/statuscomment.go b/internal/statuscomment/statuscomment.go index 32bc4e982e..b8f8445eb2 100644 --- a/internal/statuscomment/statuscomment.go +++ b/internal/statuscomment/statuscomment.go @@ -1,5 +1,5 @@ // Package statuscomment posts agent start/completion status comments -// on issues and pull requests via the forge abstraction. +// on issues and pull requests via the tracker abstraction. // // Unlike internal/sticky, which manages persistent bot comments that // accumulate history across multiple runs (e.g. review output), @@ -7,6 +7,11 @@ // created when the agent begins, then updated or replaced on // completion (including cancellation). The two packages share the HTML-marker // convention but have different lifecycles and placement heuristics. +// +// Status comments are routed to the issue tracker that originated the +// run (GitHub, GitLab, or Jira) via tracker.Client, independently of +// the forge used for code output (branches, PRs/MRs). This separation +// is recorded in ADR 0093. package statuscomment import ( @@ -18,7 +23,7 @@ import ( "time" "github.com/fullsend-ai/fullsend/internal/config" - "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/internal/tracker" ) var validRunID = regexp.MustCompile(`^[a-zA-Z0-9_-]+$`) @@ -51,9 +56,9 @@ const ( // now is overridable in tests to fix the current time for ReconcileOrphaned. var now = time.Now -// ClientFactory returns a fresh forge.Client. It is called before each +// ClientFactory returns a fresh tracker.Client. It is called before each // API operation so the underlying token is never stale. -type ClientFactory func(ctx context.Context) (forge.Client, error) +type ClientFactory func(ctx context.Context) (tracker.Client, error) // RunInfo holds optional runtime/model metadata surfaced in the terminal // status comment footer. All fields are optional — omitted fields are @@ -67,17 +72,20 @@ type RunInfo struct { } // Notifier manages status comment lifecycle for a single agent run. +// It posts status comments to the tracker that originated the run, +// independently of the forge used for code output (branches, PRs/MRs). type Notifier struct { - client forge.Client + client tracker.Client + reactor tracker.Reactor // optional; nil when the tracker has no reaction support clientFactory ClientFactory cfg config.StatusNotificationConfig - owner, repo string + project string number int runURL string sha string marker string - startCommentID int + startCommentID string // startReactionID is in-memory only, unlike startCommentID which can be // recovered by ReconcileOrphaned via the HTML marker embedded in the // comment body. If the process is hard-killed between PostStart and @@ -85,7 +93,7 @@ type Notifier struct { // never cleaned up — there is no equivalent out-of-process reconciler // for reactions. See ReconcileOrphaned's doc comment. startReactionID int64 - triggerCommentID int + triggerCommentID string startTime time.Time now func() time.Time warnf func(string, ...any) @@ -95,20 +103,26 @@ type Notifier struct { // New creates a Notifier. The runID is embedded in the HTML marker comment // so multiple concurrent runs on the same issue don't collide. // It panics if runID contains characters outside [a-zA-Z0-9_-]. -func New(client forge.Client, cfg config.StatusNotificationConfig, - owner, repo string, number int, runURL, sha, runID string) *Notifier { - return &Notifier{ - client: client, - cfg: cfg, - owner: owner, - repo: repo, - number: number, - runURL: runURL, - sha: sha, - marker: mustBuildMarker(runID), - now: time.Now, - warnf: func(string, ...any) {}, - } +// +// project identifies the issue's container: "owner/repo" for +// GitHub/GitLab, a Jira project key for Jira. +func New(client tracker.Client, cfg config.StatusNotificationConfig, + project string, number int, runURL, sha, runID string) *Notifier { + n := &Notifier{ + client: client, + cfg: cfg, + project: project, + number: number, + runURL: runURL, + sha: sha, + marker: mustBuildMarker(runID), + now: time.Now, + warnf: func(string, ...any) {}, + } + if r, ok := client.(tracker.Reactor); ok { + n.reactor = r + } + return n } // SetWarnFunc sets a function called for non-fatal warnings (e.g. API @@ -123,7 +137,7 @@ func (n *Notifier) SetRunInfo(info RunInfo) { n.runInfo = &info } -// SetClientFactory sets a factory that mints a fresh forge.Client before +// SetClientFactory sets a factory that mints a fresh tracker.Client before // each API operation. When set, the static client passed to New is only // used if the factory is nil. func (n *Notifier) SetClientFactory(f ClientFactory) { @@ -136,7 +150,7 @@ func (n *Notifier) SetClientFactory(f ClientFactory) { // human collaborator would expect from a reply, not a reaction to the // whole thread. Has no effect on comments, which always post to the // issue/PR regardless of what triggered the run. -func (n *Notifier) SetTriggerCommentID(id int) { +func (n *Notifier) SetTriggerCommentID(id string) { n.triggerCommentID = id } @@ -147,7 +161,7 @@ func (n *Notifier) HasClientFactory() bool { // InvokeClientFactory calls the configured factory and returns the result. // Useful for verifying factory wiring in tests without triggering API calls. -func (n *Notifier) InvokeClientFactory(ctx context.Context) (forge.Client, error) { +func (n *Notifier) InvokeClientFactory(ctx context.Context) (tracker.Client, error) { if n.clientFactory == nil { return nil, fmt.Errorf("no client factory configured") } @@ -165,6 +179,11 @@ func (n *Notifier) refreshClient(ctx context.Context) error { return fmt.Errorf("minting fresh client: %w", err) } n.client = c + if r, ok := c.(tracker.Reactor); ok { + n.reactor = r + } else { + n.reactor = nil + } return nil } @@ -243,7 +262,7 @@ func (n *Notifier) PostStart(ctx context.Context, description string) error { if postComment { body := n.buildStartBody(description) - comment, err := n.client.CreateIssueComment(ctx, n.owner, n.repo, n.number, body) + comment, err := n.client.CreateComment(ctx, n.project, n.number, tracker.Body(body)) if err != nil { return fmt.Errorf("posting start comment: %w", err) } @@ -267,20 +286,30 @@ func (n *Notifier) PostStart(ctx context.Context, description string) error { // addReaction adds an emoji reaction, targeting the triggering comment // instead of the issue/PR when this run was invoked by a slash command. // See SetTriggerCommentID. +// +// Returns (0, nil) when the tracker does not support reactions. func (n *Notifier) addReaction(ctx context.Context, content string) (int64, error) { - if n.triggerCommentID != 0 { - return n.client.AddIssueCommentReaction(ctx, n.owner, n.repo, n.triggerCommentID, content) + if n.reactor == nil { + return 0, nil + } + if n.triggerCommentID != "" { + return n.reactor.AddCommentReaction(ctx, n.project, n.number, n.triggerCommentID, content) } - return n.client.AddIssueReaction(ctx, n.owner, n.repo, n.number, content) + return n.reactor.AddIssueReaction(ctx, n.project, n.number, content) } // deleteReaction removes a previously added reaction, mirroring the // comment-vs-issue targeting addReaction uses to add it. +// +// No-ops when the tracker does not support reactions. func (n *Notifier) deleteReaction(ctx context.Context, reactionID int64) error { - if n.triggerCommentID != 0 { - return n.client.DeleteIssueCommentReaction(ctx, n.owner, n.repo, n.triggerCommentID, reactionID) + if n.reactor == nil { + return nil + } + if n.triggerCommentID != "" { + return n.reactor.DeleteCommentReaction(ctx, n.project, n.number, n.triggerCommentID, reactionID) } - return n.client.DeleteIssueReaction(ctx, n.owner, n.repo, n.number, reactionID) + return n.reactor.DeleteIssueReaction(ctx, n.project, n.number, reactionID) } // PostCompletion posts or edits a completion comment with no extra @@ -310,7 +339,7 @@ func (n *Notifier) PostCompletionWithDetail(ctx context.Context, description, st completionTime := n.now().UTC() postComment := shouldPostCompletion(n.cfg.Comment.Completion, status) - cleanupComment := !postComment && n.startCommentID != 0 + cleanupComment := !postComment && n.startCommentID != "" cleanupReaction := n.startReactionID != 0 postReaction := shouldPostReactionCompletion(n.cfg.Reaction.Completion, status) @@ -331,7 +360,7 @@ func (n *Notifier) PostCompletionWithDetail(ctx context.Context, description, st // the swap can happen unconditionally here. n.postCompletionReaction(ctx, status, cleanupReaction, postReaction) if cleanupComment { - if err := n.client.DeleteIssueComment(ctx, n.owner, n.repo, n.startCommentID); err != nil { + if err := n.client.DeleteComment(ctx, n.project, n.number, n.startCommentID); err != nil { n.warnf("failed to delete start comment when completion suppressed: %v", err) } } @@ -340,24 +369,24 @@ func (n *Notifier) PostCompletionWithDetail(ctx context.Context, description, st body := n.buildCompletionBody(description, status, detail, completionTime) - if n.startCommentID != 0 { + if n.startCommentID != "" { agentPosted, startIsLast, err := n.analyzeTimeline(ctx) if err != nil { n.warnf("failed to analyze timeline, updating start comment in place: %v", err) - if err := n.client.UpdateIssueComment(ctx, n.owner, n.repo, n.startCommentID, body); err != nil { + if err := n.client.UpdateComment(ctx, n.project, n.number, n.startCommentID, tracker.Body(body)); err != nil { return fmt.Errorf("updating start comment with completion: %w", err) } } else if agentPosted || startIsLast { - if err := n.client.UpdateIssueComment(ctx, n.owner, n.repo, n.startCommentID, body); err != nil { + if err := n.client.UpdateComment(ctx, n.project, n.number, n.startCommentID, tracker.Body(body)); err != nil { return fmt.Errorf("updating start comment with completion: %w", err) } } else { - if _, err := n.client.CreateIssueComment(ctx, n.owner, n.repo, n.number, body); err != nil { + if _, err := n.client.CreateComment(ctx, n.project, n.number, tracker.Body(body)); err != nil { return fmt.Errorf("posting completion comment: %w", err) } } } else { - if _, err := n.client.CreateIssueComment(ctx, n.owner, n.repo, n.number, body); err != nil { + if _, err := n.client.CreateComment(ctx, n.project, n.number, tracker.Body(body)); err != nil { return fmt.Errorf("posting completion comment: %w", err) } } @@ -397,7 +426,7 @@ func (n *Notifier) postCompletionReaction(ctx context.Context, status string, cl // - agentPosted: whether the bot posted non-status output after the start comment // - startIsLast: whether the start comment is the last on the timeline func (n *Notifier) analyzeTimeline(ctx context.Context) (agentPosted, startIsLast bool, err error) { - comments, err := n.client.ListIssueComments(ctx, n.owner, n.repo, n.number) + comments, err := n.client.ListComments(ctx, n.project, n.number) if err != nil { return false, false, err } @@ -410,7 +439,7 @@ func (n *Notifier) analyzeTimeline(ctx context.Context) (agentPosted, startIsLas } } if startIdx < 0 { - n.warnf("start comment %d not found on timeline; it may have been deleted externally", n.startCommentID) + n.warnf("start comment %s not found on timeline; it may have been deleted externally", n.startCommentID) return false, false, nil } @@ -422,7 +451,7 @@ func (n *Notifier) analyzeTimeline(ctx context.Context) (agentPosted, startIsLas } for _, c := range comments[startIdx+1:] { - if c.Author == botUser && !strings.Contains(c.Body, "fullsend:agent-status:") { + if c.Author == botUser && !strings.Contains(string(c.Body), "fullsend:agent-status:") { agentPosted = true break } @@ -696,30 +725,31 @@ func statusEmoji(status string) string { // hard-killed run can leave a stray 👀 reaction behind indefinitely. // // Returns an error if runID contains characters outside [a-zA-Z0-9_-]. -func ReconcileOrphaned(ctx context.Context, client forge.Client, owner, repo string, number int, runID, runURL, sha string, reason TerminationReason, completionMode, jobStatus string, wasSkipped bool, agentDescription string) error { +func ReconcileOrphaned(ctx context.Context, client tracker.Client, project string, number int, runID, runURL, sha string, reason TerminationReason, completionMode, jobStatus string, wasSkipped bool, agentDescription string) error { marker, err := buildMarker(runID) if err != nil { return fmt.Errorf("building marker: %w", err) } - comments, err := client.ListIssueComments(ctx, owner, repo, number) + comments, err := client.ListComments(ctx, project, number) if err != nil { return fmt.Errorf("listing comments: %w", err) } for _, c := range comments { - if !strings.Contains(c.Body, marker) { + if !strings.Contains(string(c.Body), marker) { continue } // Already finalized — nothing to do. - if strings.Contains(c.Body, terminalTag) { + if strings.Contains(string(c.Body), terminalTag) { return nil } // Still in "Started" state — finalize it. - desc, startTimeStr := parseStartBody(c.Body) + desc, startTimeStr := parseStartBody(string(c.Body)) endTime := now().UTC() body := buildInterruptedBody(marker, runURL, sha, desc, startTimeStr, endTime, reason) - if err := client.UpdateIssueComment(ctx, owner, repo, c.ID, body); err != nil { + commentID := c.ID + if err := client.UpdateComment(ctx, project, number, commentID, tracker.Body(body)); err != nil { return fmt.Errorf("updating orphaned comment: %w", err) } return nil @@ -756,7 +786,7 @@ func ReconcileOrphaned(ctx context.Context, client forge.Client, owner, repo str if shouldSynthesize { endTime := now().UTC() body := buildInterruptedBody(marker, runURL, sha, agentDescription, "", endTime, synthReason) - if _, err := client.CreateIssueComment(ctx, owner, repo, number, body); err != nil { + if _, err := client.CreateComment(ctx, project, number, tracker.Body(body)); err != nil { return fmt.Errorf("creating synthesized interrupted comment: %w", err) } } diff --git a/internal/statuscomment/statuscomment_test.go b/internal/statuscomment/statuscomment_test.go index e962394749..a919d22a78 100644 --- a/internal/statuscomment/statuscomment_test.go +++ b/internal/statuscomment/statuscomment_test.go @@ -12,17 +12,19 @@ import ( "github.com/fullsend-ai/fullsend/internal/config" "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/internal/tracker" ) func fixedTime() time.Time { return time.Date(2026, 6, 3, 14, 34, 0, 0, time.UTC) } -func newTestNotifier(fc *forge.FakeClient, cfg config.StatusNotificationConfig) *Notifier { +func newTestNotifier(fc *forge.FakeClient, cfg config.StatusNotificationConfig) (*Notifier, *forge.FakeClient) { fc.AuthenticatedUser = "fullsend-bot[bot]" - n := New(fc, cfg, "org", "repo", 7, "https://ci/run/42", "a1b2c3d4e5f6789", "run-42") + tc := tracker.NewForgeClient(fc) + n := New(tc, cfg, "org/repo", 7, "https://ci/run/42", "a1b2c3d4e5f6789", "run-42") n.now = fixedTime - return n + return n, fc } func TestPostStart_CommentEnabled(t *testing.T) { @@ -30,7 +32,7 @@ func TestPostStart_CommentEnabled(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Reviewing this PR") require.NoError(t, err) @@ -48,7 +50,7 @@ func TestPostStart_CommentDisabled(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "disabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working on issue") require.NoError(t, err) @@ -59,7 +61,7 @@ func TestPostStart_CommentDisabled(t *testing.T) { func TestPostStart_DefaultEnabled(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{} - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) @@ -72,11 +74,11 @@ func TestPostCompletion_EditInPlace(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Reviewing this PR") require.NoError(t, err) - require.Equal(t, 1, n.startCommentID) + require.Equal(t, "1", n.startCommentID) completionTime := fixedTime().Add(7 * time.Minute) n.now = func() time.Time { return completionTime } @@ -97,7 +99,7 @@ func TestPostCompletion_NewComment_WhenInterveningHumanActivity(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Triaging issue") require.NoError(t, err) @@ -125,7 +127,7 @@ func TestPostCompletion_EditStart_WhenAgentPostedOutput(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Triaging issue") require.NoError(t, err) @@ -152,7 +154,7 @@ func TestPostCompletion_EditStart_WhenAgentAndHumanPosted(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Reviewing this PR") require.NoError(t, err) @@ -180,7 +182,7 @@ func TestPostCompletion_Cancelled(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) @@ -204,7 +206,7 @@ func TestPostCompletion_Skipped(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) @@ -226,7 +228,7 @@ func TestAllDisabled_NoAPICalls(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "disabled", Completion: "disabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) @@ -242,7 +244,8 @@ func TestAllDisabled_NoAPICalls(t *testing.T) { func TestRunURL_Omitted(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{} - n := New(fc, cfg, "org", "repo", 7, "", "abc123", "run-1") + tc := tracker.NewForgeClient(fc) + n := New(tc, cfg, "org/repo", 7, "", "abc123", "run-1") n.now = fixedTime err := n.PostStart(context.Background(), "Working") @@ -256,7 +259,8 @@ func TestRunURL_Omitted(t *testing.T) { func TestSHA_Omitted(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{} - n := New(fc, cfg, "org", "repo", 7, "https://ci/run/1", "", "run-1") + tc := tracker.NewForgeClient(fc) + n := New(tc, cfg, "org/repo", 7, "https://ci/run/1", "", "run-1") n.now = fixedTime err := n.PostStart(context.Background(), "Working") @@ -272,7 +276,7 @@ func TestPostCompletion_Failure(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Coding issue #42") require.NoError(t, err) @@ -290,7 +294,7 @@ func TestPostCompletionWithDetail_FailureShowsErrorMessage(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) require.NoError(t, n.PostStart(context.Background(), "Coding issue #99")) @@ -310,7 +314,7 @@ func TestPostCompletionWithDetail_FailureWithoutDetail(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) require.NoError(t, n.PostStart(context.Background(), "Coding issue #99")) @@ -323,7 +327,6 @@ func TestPostCompletionWithDetail_FailureWithoutDetail(t *testing.T) { require.Len(t, fc.UpdatedComments, 1) body := fc.UpdatedComments[0].Body assert.Contains(t, body, "❌ Failure") - // Ensure no parenthesized detail appears when detail is empty. assert.NotContains(t, body, "Failure (") } @@ -332,14 +335,13 @@ func TestPostCompletionWithDetail_FailureDetailSanitized(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) require.NoError(t, n.PostStart(context.Background(), "Working")) completionTime := fixedTime().Add(3 * time.Minute) n.now = func() time.Time { return completionTime } - // Error messages with newlines should be collapsed to a single line. err := n.PostCompletionWithDetail(context.Background(), "Working", "failure", "pre-script failed\nexit status 1") require.NoError(t, err) @@ -347,7 +349,7 @@ func TestPostCompletionWithDetail_FailureDetailSanitized(t *testing.T) { require.Len(t, fc.UpdatedComments, 1) body := fc.UpdatedComments[0].Body assert.Contains(t, body, "❌ Failure (pre-script failed exit status 1)") - assert.NotContains(t, body, "\n❌") // No newline break in the status label. + assert.NotContains(t, body, "\n❌") } func TestPostCompletion_UnknownStatus(t *testing.T) { @@ -355,7 +357,7 @@ func TestPostCompletion_UnknownStatus(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) @@ -373,7 +375,7 @@ func TestPostCompletion_NoStartComment(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "disabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) @@ -406,8 +408,9 @@ func TestShortSHA(t *testing.T) { func TestMarkerUniqueness(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{} - n1 := New(fc, cfg, "org", "repo", 7, "", "", "run-1") - n2 := New(fc, cfg, "org", "repo", 7, "", "", "run-2") + tc := tracker.NewForgeClient(fc) + n1 := New(tc, cfg, "org/repo", 7, "", "", "run-1") + n2 := New(tc, cfg, "org/repo", 7, "", "", "run-2") assert.NotEqual(t, n1.marker, n2.marker) assert.Contains(t, n1.marker, "run-1") assert.Contains(t, n2.marker, "run-2") @@ -452,7 +455,8 @@ func setNow(t *testing.T, fixed time.Time) { func TestReconcileOrphaned_InvalidRunID(t *testing.T) { fc := forge.NewFakeClient() - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "-->bad", "", "", ReasonTerminated, "", "", false, "") + tc := tracker.NewForgeClient(fc) + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "-->bad", "", "", ReasonTerminated, "", "", false, "") require.Error(t, err) assert.Contains(t, err.Error(), "invalid run ID") } @@ -462,11 +466,11 @@ func TestPostCompletion_CompletionDisabled_CleansUpStartComment(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "disabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) - require.Equal(t, 1, n.startCommentID) + require.Equal(t, "1", n.startCommentID) n.now = func() time.Time { return fixedTime().Add(time.Minute) } err = n.PostCompletion(context.Background(), "Working", "success") @@ -483,11 +487,11 @@ func TestPostCompletion_CancelledWithCompletionDisabled(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "disabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) - require.Equal(t, 1, n.startCommentID) + require.Equal(t, "1", n.startCommentID) n.now = func() time.Time { return fixedTime().Add(time.Minute) } err = n.PostCompletion(context.Background(), "Working", "cancelled") @@ -516,7 +520,8 @@ func TestRunURL_UnsafeDropped(t *testing.T) { t.Run(tt.name, func(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{} - n := New(fc, cfg, "org", "repo", 7, tt.url, "abc123", "run-1") + tc := tracker.NewForgeClient(fc) + n := New(tc, cfg, "org/repo", 7, tt.url, "abc123", "run-1") n.now = fixedTime err := n.PostStart(context.Background(), "Working") @@ -538,12 +543,13 @@ func TestAnalyzeTimeline_EmptyBotUser_FallsBackToPositionOnly(t *testing.T) { Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } fc.AuthenticatedUser = "" - n := New(fc, cfg, "org", "repo", 7, "https://ci/run/42", "a1b2c3d4e5f6789", "run-42") + tc := tracker.NewForgeClient(fc) + n := New(tc, cfg, "org/repo", 7, "https://ci/run/42", "a1b2c3d4e5f6789", "run-42") n.now = fixedTime err := n.PostStart(context.Background(), "Reviewing this PR") require.NoError(t, err) - require.Equal(t, 1, n.startCommentID) + require.Equal(t, "1", n.startCommentID) // Start is the last comment → should edit in place even without bot user identity. n.now = func() time.Time { return fixedTime().Add(5 * time.Minute) } @@ -560,13 +566,13 @@ func TestAnalyzeTimeline_EmptyBotUser_NewCommentWhenNotLast(t *testing.T) { Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } fc.AuthenticatedUser = "" - n := New(fc, cfg, "org", "repo", 7, "https://ci/run/42", "a1b2c3d4e5f6789", "run-42") + tc := tracker.NewForgeClient(fc) + n := New(tc, cfg, "org/repo", 7, "https://ci/run/42", "a1b2c3d4e5f6789", "run-42") n.now = fixedTime err := n.PostStart(context.Background(), "Reviewing this PR") require.NoError(t, err) - // Agent posts output, but since botUser is empty it can't be identified as agent output. fc.IssueComments["org/repo/7"] = append(fc.IssueComments["org/repo/7"], forge.IssueComment{ ID: 9999, Body: "Some output", @@ -577,7 +583,6 @@ func TestAnalyzeTimeline_EmptyBotUser_NewCommentWhenNotLast(t *testing.T) { err = n.PostCompletion(context.Background(), "Reviewing this PR", "success") require.NoError(t, err) - // Without bot identity, agentPosted is false and startIsLast is false → new comment. assert.Empty(t, fc.UpdatedComments, "should not edit start when bot identity unknown and not last") comments := fc.IssueComments["org/repo/7"] require.Len(t, comments, 3) @@ -589,24 +594,21 @@ func TestAnalyzeTimeline_UsesStartCommentAuthor(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - // Set AuthenticatedUser so FakeClient stamps the start comment Author. fc.AuthenticatedUser = "fullsend-bot[bot]" - // Inject GetAuthenticatedUser error to prove it is NOT called. - fc.Errors["GetAuthenticatedUser"] = fmt.Errorf("should not be called") - n := New(fc, cfg, "org", "repo", 7, "https://ci/run/42", "a1b2c3d4e5f6789", "run-42") + fc.Errors = map[string]error{"GetAuthenticatedUser": fmt.Errorf("should not be called")} + tc := tracker.NewForgeClient(fc) + n := New(tc, cfg, "org/repo", 7, "https://ci/run/42", "a1b2c3d4e5f6789", "run-42") n.now = fixedTime err := n.PostStart(context.Background(), "Reviewing this PR") require.NoError(t, err) - // Agent posts output (same bot author, no status marker). fc.CreateIssueComment(context.Background(), "org", "repo", 7, "Review findings here") n.now = func() time.Time { return fixedTime().Add(5 * time.Minute) } err = n.PostCompletion(context.Background(), "Reviewing this PR", "success") require.NoError(t, err) - // Bot user derived from start comment Author → agentPosted = true → edit in place. require.Len(t, fc.UpdatedComments, 1) assert.Equal(t, 1, fc.UpdatedComments[0].CommentID) assert.Contains(t, fc.UpdatedComments[0].Body, "Finished Reviewing this PR") @@ -626,7 +628,6 @@ func TestReconcileOrphaned_UpdatesStartedComment(t *testing.T) { fc.IssueComments = map[string][]forge.IssueComment{} setNow(t, time.Date(2026, 6, 3, 7, 12, 0, 0, time.UTC)) - // Simulate a "Started" comment left by a killed process. fc.IssueComments["org/repo/7"] = []forge.IssueComment{ { ID: 42, @@ -635,7 +636,8 @@ func TestReconcileOrphaned_UpdatesStartedComment(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") + tc := tracker.NewForgeClient(fc) + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -655,7 +657,6 @@ func TestReconcileOrphaned_SkipsAlreadyFinished(t *testing.T) { fc := forge.NewFakeClient() fc.IssueComments = map[string][]forge.IssueComment{} - // Comment already reached terminal state (via PostCompletion). fc.IssueComments["org/repo/7"] = []forge.IssueComment{ { ID: 42, @@ -664,7 +665,8 @@ func TestReconcileOrphaned_SkipsAlreadyFinished(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") + tc := tracker.NewForgeClient(fc) + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") require.NoError(t, err) assert.Empty(t, fc.UpdatedComments, "should not update already-finished comment") @@ -672,20 +674,19 @@ func TestReconcileOrphaned_SkipsAlreadyFinished(t *testing.T) { func TestReconcileOrphaned_NoMatchingComment(t *testing.T) { fc := forge.NewFakeClient() + tc := tracker.NewForgeClient(fc) - // No comments at all. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") require.NoError(t, err) assert.Empty(t, fc.UpdatedComments) } func TestReconcileOrphaned_OnFailure_SynthesizesWhenNoMarker(t *testing.T) { fc := forge.NewFakeClient() + tc := tracker.NewForgeClient(fc) setNow(t, time.Date(2026, 6, 3, 14, 0, 0, 0, time.UTC)) - // No comments at all — simulates on_failure mode where PostStart was suppressed - // and the process was hard-killed before PostCompletion ran. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "on_failure", "failure", false, "") + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "on_failure", "failure", false, "") require.NoError(t, err) comments := fc.IssueComments["org/repo/7"] @@ -704,8 +705,6 @@ func TestReconcileOrphaned_OnFailure_NoSynthesisWhenMarkerExists(t *testing.T) { fc.IssueComments = map[string][]forge.IssueComment{} setNow(t, time.Date(2026, 6, 3, 14, 0, 0, 0, time.UTC)) - // A start comment exists (shouldn't happen under on_failure, but if it does, - // reconcile should finalize it normally, not double-post). fc.IssueComments["org/repo/7"] = []forge.IssueComment{ { ID: 42, @@ -714,10 +713,10 @@ func TestReconcileOrphaned_OnFailure_NoSynthesisWhenMarkerExists(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "on_failure", "failure", false, "") + tc := tracker.NewForgeClient(fc) + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "on_failure", "failure", false, "") require.NoError(t, err) - // Should update the existing comment, not create a new one. require.Len(t, fc.UpdatedComments, 1) assert.Equal(t, 42, fc.UpdatedComments[0].CommentID) assert.Contains(t, fc.UpdatedComments[0].Body, "❌ Terminated") @@ -725,10 +724,9 @@ func TestReconcileOrphaned_OnFailure_NoSynthesisWhenMarkerExists(t *testing.T) { func TestReconcileOrphaned_EnabledMode_NoSynthesisWhenNoMarker(t *testing.T) { fc := forge.NewFakeClient() + tc := tracker.NewForgeClient(fc) - // No comments, default completion mode, unknown job status — should - // NOT synthesize. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") require.NoError(t, err) assert.Empty(t, fc.IssueComments, "should not synthesize for enabled mode") assert.Empty(t, fc.UpdatedComments) @@ -736,11 +734,9 @@ func TestReconcileOrphaned_EnabledMode_NoSynthesisWhenNoMarker(t *testing.T) { func TestReconcileOrphaned_EnabledMode_NoSynthesisWhenJobSucceeded(t *testing.T) { fc := forge.NewFakeClient() + tc := tracker.NewForgeClient(fc) - // No comments, default completion mode, job succeeded — the run - // completed and posted its own completion comment as expected; nothing - // to synthesize. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "enabled", "success", false, "") + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "enabled", "success", false, "") require.NoError(t, err) assert.Empty(t, fc.IssueComments) assert.Empty(t, fc.UpdatedComments) @@ -748,16 +744,10 @@ func TestReconcileOrphaned_EnabledMode_NoSynthesisWhenJobSucceeded(t *testing.T) func TestReconcileOrphaned_EnabledMode_SynthesizesOnFailureWithNoMarker(t *testing.T) { fc := forge.NewFakeClient() + tc := tracker.NewForgeClient(fc) setNow(t, time.Date(2026, 6, 3, 14, 0, 0, 0, time.UTC)) - // No comments, default ("") completion mode, job failed. A status - // comment should always exist for a run that reached the harness under - // the default mode — its absence alongside a failed job means the - // process crashed before it could post anything at all (e.g. during - // environment validation), leaving maintainers unable to tell "no - // review was triggered" from "review was attempted and failed - // silently." See #3635. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "failure", false, "Review") + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "failure", false, "Review") require.NoError(t, err) comments := fc.IssueComments["org/repo/7"] @@ -768,11 +758,10 @@ func TestReconcileOrphaned_EnabledMode_SynthesizesOnFailureWithNoMarker(t *testi func TestReconcileOrphaned_EnabledMode_SynthesizesOnCancelledWithNoMarker(t *testing.T) { fc := forge.NewFakeClient() + tc := tracker.NewForgeClient(fc) setNow(t, time.Date(2026, 6, 3, 14, 0, 0, 0, time.UTC)) - // Same as above, but the job was cancelled rather than failed, and - // completion is explicitly "enabled" rather than the implicit default. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonCancelled, "enabled", "cancelled", false, "") + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonCancelled, "enabled", "cancelled", false, "") require.NoError(t, err) comments := fc.IssueComments["org/repo/7"] @@ -782,11 +771,9 @@ func TestReconcileOrphaned_EnabledMode_SynthesizesOnCancelledWithNoMarker(t *tes func TestReconcileOrphaned_DisabledMode_NoSynthesisEvenOnFailure(t *testing.T) { fc := forge.NewFakeClient() + tc := tracker.NewForgeClient(fc) - // completion: disabled is an explicit opt-out of all status comments. - // Even though no marker exists and the job failed, we must not - // synthesize one — that would override the user's choice. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "disabled", "failure", false, "") + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "disabled", "failure", false, "") require.NoError(t, err) assert.Empty(t, fc.IssueComments, "disabled completion mode must never synthesize a comment") assert.Empty(t, fc.UpdatedComments) @@ -794,11 +781,9 @@ func TestReconcileOrphaned_DisabledMode_NoSynthesisEvenOnFailure(t *testing.T) { func TestReconcileOrphaned_OnFailure_NoSynthesisWhenJobSucceeded(t *testing.T) { fc := forge.NewFakeClient() + tc := tracker.NewForgeClient(fc) - // No comments — on_failure mode but the job succeeded. The agent completed - // normally and PostCompletion suppressed the comment. ReconcileOrphaned - // must NOT synthesize a false "Interrupted" comment. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "on_failure", "success", false, "") + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "on_failure", "success", false, "") require.NoError(t, err) assert.Empty(t, fc.IssueComments, "should not synthesize when job succeeded") assert.Empty(t, fc.UpdatedComments) @@ -806,10 +791,9 @@ func TestReconcileOrphaned_OnFailure_NoSynthesisWhenJobSucceeded(t *testing.T) { func TestReconcileOrphaned_OnFailure_NoSynthesisWhenJobStatusEmpty(t *testing.T) { fc := forge.NewFakeClient() + tc := tracker.NewForgeClient(fc) - // No comments — on_failure mode but jobStatus is empty (--job-status flag - // was omitted). Should NOT synthesize since the job outcome is unknown. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "on_failure", "", false, "") + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "on_failure", "", false, "") require.NoError(t, err) assert.Empty(t, fc.IssueComments, "should not synthesize when job status is unknown") assert.Empty(t, fc.UpdatedComments) @@ -817,22 +801,14 @@ func TestReconcileOrphaned_OnFailure_NoSynthesisWhenJobStatusEmpty(t *testing.T) func TestReconcileOrphaned_OnFailure_SynthesizesWhenSkippedEvenIfJobSucceeded(t *testing.T) { fc := forge.NewFakeClient() + tc := tracker.NewForgeClient(fc) setNow(t, time.Date(2026, 6, 3, 14, 0, 0, 0, time.UTC)) - // No comments — the run was skipped and the skip-reason comment itself - // failed to post (its error is only logged, not propagated to the job's - // exit code, so jobStatus is still "success"). wasSkipped must force - // synthesis so the failure isn't silently lost. See PR #5736. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "on_failure", "success", true, "") + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "on_failure", "success", true, "") require.NoError(t, err) comments := fc.IssueComments["org/repo/7"] require.Len(t, comments, 1, "should synthesize an interrupted comment despite jobStatus==success") - // The label is deliberately outcome-neutral: "no completion comment" is - // true whether the notifier failed to set up (never attempted a post) - // or its own skip-reason comment post failed — ReconcileOrphaned can't - // tell those apart, so it shouldn't assert a specific cause. See the - // review discussion on PR #5736. assert.Contains(t, comments[0].Body, "⏭️ Skipped (no completion comment)") assert.NotContains(t, comments[0].Body, "comment failed to post") assert.NotContains(t, comments[0].Body, "❌ Terminated") @@ -840,13 +816,10 @@ func TestReconcileOrphaned_OnFailure_SynthesizesWhenSkippedEvenIfJobSucceeded(t func TestReconcileOrphaned_OnFailure_SkippedWithRealCancellationKeepsReason(t *testing.T) { fc := forge.NewFakeClient() + tc := tracker.NewForgeClient(fc) setNow(t, time.Date(2026, 6, 3, 14, 0, 0, 0, time.UTC)) - // wasSkipped is true, but jobStatus is "cancelled" — the job was - // actually cancelled (unrelated to the skip-reason comment), so the - // passed-in reason should be preserved rather than relabeled as a - // comment-post failure. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonCancelled, "on_failure", "cancelled", true, "") + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonCancelled, "on_failure", "cancelled", true, "") require.NoError(t, err) comments := fc.IssueComments["org/repo/7"] @@ -857,22 +830,19 @@ func TestReconcileOrphaned_OnFailure_SkippedWithRealCancellationKeepsReason(t *t func TestReconcileOrphaned_EnabledMode_NoSynthesisWhenSkippedButNotOnFailure(t *testing.T) { fc := forge.NewFakeClient() + tc := tracker.NewForgeClient(fc) - // wasSkipped alone shouldn't trigger synthesis outside on_failure mode — - // completionMode must also be "on_failure". - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "success", true, "") + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "success", true, "") require.NoError(t, err) assert.Empty(t, fc.IssueComments, "should not synthesize when completionMode isn't on_failure") } func TestReconcileOrphaned_SynthesizedComment_UsesAgentDescription(t *testing.T) { fc := forge.NewFakeClient() + tc := tracker.NewForgeClient(fc) setNow(t, time.Date(2026, 6, 3, 14, 0, 0, 0, time.UTC)) - // No comments — synthesized comment heading should reflect the agent - // description so operators can tell which agent failed when multiple - // agents run against the same issue/PR. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "on_failure", "failure", false, "Triage") + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "on_failure", "failure", false, "Triage") require.NoError(t, err) comments := fc.IssueComments["org/repo/7"] @@ -884,7 +854,6 @@ func TestReconcileOrphaned_DifferentRunID(t *testing.T) { fc := forge.NewFakeClient() fc.IssueComments = map[string][]forge.IssueComment{} - // Comment from a different run. fc.IssueComments["org/repo/7"] = []forge.IssueComment{ { ID: 42, @@ -893,7 +862,8 @@ func TestReconcileOrphaned_DifferentRunID(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") + tc := tracker.NewForgeClient(fc) + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") require.NoError(t, err) assert.Empty(t, fc.UpdatedComments, "should not touch comment from different run") @@ -901,9 +871,10 @@ func TestReconcileOrphaned_DifferentRunID(t *testing.T) { func TestReconcileOrphaned_ListError(t *testing.T) { fc := forge.NewFakeClient() - fc.Errors["ListIssueComments"] = fmt.Errorf("api error") + fc.Errors = map[string]error{"ListIssueComments": fmt.Errorf("api error")} - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "", "", ReasonTerminated, "", "", false, "") + tc := tracker.NewForgeClient(fc) + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "", "", ReasonTerminated, "", "", false, "") require.Error(t, err) assert.Contains(t, err.Error(), "listing comments") } @@ -921,7 +892,8 @@ func TestReconcileOrphaned_NoURLOrSHA(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "", "", ReasonTerminated, "", "", false, "") + tc := tracker.NewForgeClient(fc) + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "", "", ReasonTerminated, "", "", false, "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -946,7 +918,8 @@ func TestReconcileOrphaned_SkipsAlreadyInterrupted(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") + tc := tracker.NewForgeClient(fc) + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") require.NoError(t, err) assert.Empty(t, fc.UpdatedComments, "should not re-update already-interrupted comment") @@ -964,18 +937,19 @@ func TestReconcileOrphaned_UpdateError(t *testing.T) { }, } - fc.Errors["UpdateIssueComment"] = fmt.Errorf("api rate limited") + fc.Errors = map[string]error{"UpdateIssueComment": fmt.Errorf("api rate limited")} - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") + tc := tracker.NewForgeClient(fc) + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") require.Error(t, err) assert.Contains(t, err.Error(), "updating orphaned comment") } func TestPostStart_ErrorPropagated(t *testing.T) { fc := forge.NewFakeClient() - fc.Errors["CreateIssueComment"] = fmt.Errorf("api down") + fc.Errors = map[string]error{"CreateIssueComment": fmt.Errorf("api down")} cfg := config.StatusNotificationConfig{} - n := newTestNotifier(fc, cfg) + n, _ := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.Error(t, err) @@ -987,11 +961,11 @@ func TestPostCompletion_CancelledWithNoStartComment(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "disabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) - assert.Equal(t, 0, n.startCommentID) + assert.Equal(t, "", n.startCommentID) n.now = func() time.Time { return fixedTime().Add(time.Minute) } err = n.PostCompletion(context.Background(), "Working", "cancelled") @@ -1008,14 +982,13 @@ func TestPostCompletion_AnalyzeTimelineError_UpdatesStartInPlace(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) - require.Equal(t, 1, n.startCommentID) + require.Equal(t, "1", n.startCommentID) - // Inject error into ListIssueComments so analyzeTimeline fails. - fc.Errors["ListIssueComments"] = fmt.Errorf("api timeout") + fc.Errors = map[string]error{"ListIssueComments": fmt.Errorf("api timeout")} var warnings []string n.SetWarnFunc(func(format string, args ...any) { @@ -1026,11 +999,9 @@ func TestPostCompletion_AnalyzeTimelineError_UpdatesStartInPlace(t *testing.T) { err = n.PostCompletion(context.Background(), "Working", "success") require.NoError(t, err) - // Should have warned about timeline analysis failure. require.Len(t, warnings, 1) assert.Contains(t, warnings[0], "failed to analyze timeline") - // Should fall back to updating the start comment in place to avoid orphaning it. require.Len(t, fc.UpdatedComments, 1, "should update start comment on timeline error") assert.Equal(t, 1, fc.UpdatedComments[0].CommentID) assert.Contains(t, fc.UpdatedComments[0].Body, "Finished Working") @@ -1051,7 +1022,8 @@ func TestReconcileOrphaned_CancelledReason(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonCancelled, "", "", false, "") + tc := tracker.NewForgeClient(fc) + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonCancelled, "", "", false, "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -1076,7 +1048,8 @@ func TestReconcileOrphaned_StartTimeNotParseable(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") + tc := tracker.NewForgeClient(fc) + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -1148,7 +1121,8 @@ func TestReconcileOrphaned_UnknownReasonDefaultsToTerminated(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", TerminationReason("unknown-value"), "", "", false, "") + tc := tracker.NewForgeClient(fc) + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", TerminationReason("unknown-value"), "", "", false, "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -1165,13 +1139,14 @@ func TestClientFactory_CalledBeforePostStart(t *testing.T) { fc2.AuthenticatedUser = "mint-bot[bot]" cfg := config.StatusNotificationConfig{} - n := New(fc1, cfg, "org", "repo", 7, "https://ci/run/42", "a1b2c3d", "run-42") + tc1 := tracker.NewForgeClient(fc1) + n := New(tc1, cfg, "org/repo", 7, "https://ci/run/42", "a1b2c3d", "run-42") n.now = fixedTime factoryCalled := false - n.SetClientFactory(func(ctx context.Context) (forge.Client, error) { + n.SetClientFactory(func(ctx context.Context) (tracker.Client, error) { factoryCalled = true - return fc2, nil + return tracker.NewForgeClient(fc2), nil }) err := n.PostStart(context.Background(), "Working") @@ -1188,21 +1163,20 @@ func TestClientFactory_CalledBeforePostCompletion(t *testing.T) { Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) fc2 := forge.NewFakeClient() fc2.AuthenticatedUser = "bot[bot]" - // Pre-populate fc2 with the same comments so analyzeTimeline works. fc2.IssueComments = map[string][]forge.IssueComment{ "org/repo/7": {fc.IssueComments["org/repo/7"][0]}, } completionFactoryCalled := false - n.SetClientFactory(func(ctx context.Context) (forge.Client, error) { + n.SetClientFactory(func(ctx context.Context) (tracker.Client, error) { completionFactoryCalled = true - return fc2, nil + return tracker.NewForgeClient(fc2), nil }) n.now = func() time.Time { return fixedTime().Add(5 * time.Minute) } @@ -1214,10 +1188,11 @@ func TestClientFactory_CalledBeforePostCompletion(t *testing.T) { func TestClientFactory_ErrorPropagated(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{} - n := New(fc, cfg, "org", "repo", 7, "", "", "run-42") + tc := tracker.NewForgeClient(fc) + n := New(tc, cfg, "org/repo", 7, "", "", "run-42") n.now = fixedTime - n.SetClientFactory(func(ctx context.Context) (forge.Client, error) { + n.SetClientFactory(func(ctx context.Context) (tracker.Client, error) { return nil, fmt.Errorf("mint service unavailable") }) @@ -1229,7 +1204,7 @@ func TestClientFactory_ErrorPropagated(t *testing.T) { func TestClientFactory_NilUsesStaticClient(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{} - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) @@ -1241,12 +1216,12 @@ func TestClientFactory_ErrorOnPostCompletion(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, _ := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) - n.SetClientFactory(func(ctx context.Context) (forge.Client, error) { + n.SetClientFactory(func(ctx context.Context) (tracker.Client, error) { return nil, fmt.Errorf("token expired") }) @@ -1261,11 +1236,11 @@ func TestClientFactory_CompletionDisabled_DeletePath(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "disabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) - require.Equal(t, 1, n.startCommentID) + require.NotEqual(t, "", n.startCommentID) fc2 := forge.NewFakeClient() fc2.AuthenticatedUser = "fullsend-bot[bot]" @@ -1274,9 +1249,9 @@ func TestClientFactory_CompletionDisabled_DeletePath(t *testing.T) { } factoryCalled := false - n.SetClientFactory(func(ctx context.Context) (forge.Client, error) { + n.SetClientFactory(func(ctx context.Context) (tracker.Client, error) { factoryCalled = true - return fc2, nil + return tracker.NewForgeClient(fc2), nil }) n.now = func() time.Time { return fixedTime().Add(time.Minute) } @@ -1292,10 +1267,10 @@ func TestClientFactory_BothDisabled_NoMint(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "disabled", Completion: "disabled"}, } - n := newTestNotifier(fc, cfg) + n, _ := newTestNotifier(fc, cfg) factoryCalled := false - n.SetClientFactory(func(ctx context.Context) (forge.Client, error) { + n.SetClientFactory(func(ctx context.Context) (tracker.Client, error) { factoryCalled = true return nil, fmt.Errorf("should not be called") }) @@ -1308,12 +1283,12 @@ func TestClientFactory_BothDisabled_NoMint(t *testing.T) { func TestHasClientFactory(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{} - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) assert.False(t, n.HasClientFactory(), "should be false when no factory set") - n.SetClientFactory(func(ctx context.Context) (forge.Client, error) { - return fc, nil + n.SetClientFactory(func(ctx context.Context) (tracker.Client, error) { + return tracker.NewForgeClient(fc), nil }) assert.True(t, n.HasClientFactory(), "should be true after SetClientFactory") } @@ -1323,17 +1298,17 @@ func TestClientFactory_CompletionDisabled_MintError(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "disabled"}, } - n := newTestNotifier(fc, cfg) + n, _ := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) - require.NotZero(t, n.startCommentID) + require.NotEqual(t, "", n.startCommentID) var warnings []string n.SetWarnFunc(func(format string, args ...any) { warnings = append(warnings, fmt.Sprintf(format, args...)) }) - n.SetClientFactory(func(ctx context.Context) (forge.Client, error) { + n.SetClientFactory(func(ctx context.Context) (tracker.Client, error) { return nil, fmt.Errorf("mint service down") }) @@ -1350,17 +1325,16 @@ func TestPostCompletion_OnFailure_SuppressedOnSuccess(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "on_failure"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) - assert.Equal(t, 0, n.startCommentID, "start comment should be auto-suppressed when completion is on_failure") + assert.Equal(t, "", n.startCommentID, "start comment should be auto-suppressed when completion is on_failure") n.now = func() time.Time { return fixedTime().Add(time.Minute) } err = n.PostCompletion(context.Background(), "Working", "success") require.NoError(t, err) - // No start comment was posted, so nothing to delete or update. assert.Empty(t, fc.IssueComments, "no comments should exist on success") assert.Empty(t, fc.UpdatedComments) assert.Empty(t, fc.DeletedComments) @@ -1371,17 +1345,16 @@ func TestPostCompletion_OnFailure_PostsOnFailure(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "on_failure"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Coding issue #42") require.NoError(t, err) - assert.Equal(t, 0, n.startCommentID, "start comment auto-suppressed") + assert.Equal(t, "", n.startCommentID, "start comment auto-suppressed") n.now = func() time.Time { return fixedTime().Add(10 * time.Minute) } err = n.PostCompletion(context.Background(), "Coding issue #42", "failure") require.NoError(t, err) - // No start comment to update — failure should create a new completion comment. assert.Empty(t, fc.UpdatedComments) comments := fc.IssueComments["org/repo/7"] require.Len(t, comments, 1, "should post completion on failure") @@ -1394,17 +1367,16 @@ func TestPostCompletion_OnFailure_PostsOnCancelled(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "on_failure"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) - assert.Equal(t, 0, n.startCommentID, "start comment auto-suppressed") + assert.Equal(t, "", n.startCommentID, "start comment auto-suppressed") n.now = func() time.Time { return fixedTime().Add(2 * time.Minute) } err = n.PostCompletion(context.Background(), "Working", "cancelled") require.NoError(t, err) - // No start comment to update — cancelled should create a new completion comment. assert.Empty(t, fc.UpdatedComments) comments := fc.IssueComments["org/repo/7"] require.Len(t, comments, 1, "should post completion on cancellation") @@ -1416,11 +1388,11 @@ func TestPostCompletion_OnFailure_NoStartComment_PostsOnFailure(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "disabled", Completion: "on_failure"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) - assert.Equal(t, 0, n.startCommentID) + assert.Equal(t, "", n.startCommentID) n.now = func() time.Time { return fixedTime().Add(time.Minute) } err = n.PostCompletion(context.Background(), "Working", "failure") @@ -1436,7 +1408,7 @@ func TestPostCompletion_OnFailure_NoStartComment_SuppressedOnSuccess(t *testing. cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "disabled", Completion: "on_failure"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) @@ -1455,11 +1427,11 @@ func TestPostCompletion_OnFailure_PostsOnSkipped(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "on_failure"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) - assert.Equal(t, 0, n.startCommentID, "start comment auto-suppressed") + assert.Equal(t, "", n.startCommentID, "start comment auto-suppressed") n.now = func() time.Time { return fixedTime().Add(time.Minute) } err = n.PostCompletion(context.Background(), "Working", "skipped") @@ -1473,21 +1445,21 @@ func TestClientFactory_CompletionDisabled_DeleteError(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "disabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) - require.NotZero(t, n.startCommentID) + require.NotEqual(t, "", n.startCommentID) fc2 := forge.NewFakeClient() - fc2.Errors["DeleteIssueComment"] = fmt.Errorf("forbidden") + fc2.Errors = map[string]error{"DeleteIssueComment": fmt.Errorf("forbidden")} var warnings []string n.SetWarnFunc(func(format string, args ...any) { warnings = append(warnings, fmt.Sprintf(format, args...)) }) - n.SetClientFactory(func(ctx context.Context) (forge.Client, error) { - return fc2, nil + n.SetClientFactory(func(ctx context.Context) (tracker.Client, error) { + return tracker.NewForgeClient(fc2), nil }) err = n.PostCompletion(context.Background(), "Working", "success") @@ -1501,7 +1473,7 @@ func TestPostCompletionWithDetail_SkippedShowsReason(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) require.NoError(t, n.PostStart(context.Background(), "Working")) @@ -1513,8 +1485,6 @@ func TestPostCompletionWithDetail_SkippedShowsReason(t *testing.T) { require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) - // A skip with no visible reason is the one thing a reader needs and - // the one thing the status line used to omit. assert.Contains(t, fc.UpdatedComments[0].Body, "⏭️ Skipped (PR #123 already addresses this issue)") } @@ -1551,14 +1521,12 @@ func TestSanitizeDetail_Truncates(t *testing.T) { assert.Equal(t, strings.Repeat("x", maxDetailLen)+"…", got) } -// A reason is script-controlled text, so it must not be able to forge the -// marker comments ReconcileOrphaned depends on, nor escape the status line. func TestPostCompletionWithDetail_DetailCannotForgeMarkers(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) require.NoError(t, n.PostStart(context.Background(), "Working")) err := n.PostCompletionWithDetail(context.Background(), "Working", "skipped", @@ -1566,24 +1534,17 @@ func TestPostCompletionWithDetail_DetailCannotForgeMarkers(t *testing.T) { require.NoError(t, err) body := fc.UpdatedComments[0].Body - // The escaped text may still read as "fullsend:agent-status", but it - // can no longer open an HTML comment, so only the real marker parses. assert.NotContains(t, body, ")") } func TestParagraphBreak_BetweenStatusAndMetadata(t *testing.T) { - // Verify that buildStartBody, buildCompletionBody, and - // buildInterruptedBody use \n\n (paragraph break) between the status - // line and the metadata line. A bare \n renders as inline whitespace - // on GitLab (strict CommonMark), collapsing the two lines into one. t.Run("start body", func(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{} - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Triaging issue") require.NoError(t, err) @@ -1598,7 +1559,7 @@ func TestParagraphBreak_BetweenStatusAndMetadata(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) require.NoError(t, n.PostStart(context.Background(), "Triaging issue")) n.now = func() time.Time { return fixedTime().Add(5 * time.Minute) } @@ -1623,7 +1584,8 @@ func TestParagraphBreak_BetweenStatusAndMetadata(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") + tc := tracker.NewForgeClient(fc) + err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -1634,7 +1596,6 @@ func TestParagraphBreak_BetweenStatusAndMetadata(t *testing.T) { } func TestBuildRunInfoFooter_DifferentModels(t *testing.T) { - // When requested != reported, show arrow format. info := &RunInfo{ Runtime: "pi", RequestedModel: "haiku", @@ -1647,7 +1608,6 @@ func TestBuildRunInfoFooter_DifferentModels(t *testing.T) { } func TestBuildRunInfoFooter_SameModel(t *testing.T) { - // When requested == reported, show single value. info := &RunInfo{ Runtime: "claude", RequestedModel: "sonnet", @@ -1660,7 +1620,6 @@ func TestBuildRunInfoFooter_SameModel(t *testing.T) { } func TestBuildRunInfoFooter_UnknownFieldsOmitted(t *testing.T) { - // Unknown fields omitted. info := &RunInfo{ Runtime: "pi", RequestedModel: "haiku", @@ -1683,7 +1642,7 @@ func TestCompletionBody_IncludesRunInfoFooter(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Code") require.NoError(t, err) @@ -1712,7 +1671,7 @@ func TestCompletionBody_RunInfoFooterWithModelDiff(t *testing.T) { cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Code") require.NoError(t, err) @@ -1741,7 +1700,7 @@ func TestPostStart_ReactionEnabled(t *testing.T) { cfg := config.StatusNotificationConfig{ Reaction: config.ReactionNotificationConfig{Start: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) @@ -1753,7 +1712,7 @@ func TestPostStart_ReactionEnabled(t *testing.T) { func TestPostStart_ReactionDisabledByDefault(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{} - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err) @@ -1767,7 +1726,7 @@ func TestPostCompletion_ReactionSuccess(t *testing.T) { Comment: config.CommentNotificationConfig{Start: "disabled", Completion: "disabled"}, Reaction: config.ReactionNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) require.NoError(t, n.PostStart(context.Background(), "Working")) require.Len(t, fc.AddedReactions, 1) @@ -1786,7 +1745,7 @@ func TestPostCompletion_ReactionFailure(t *testing.T) { Comment: config.CommentNotificationConfig{Start: "disabled", Completion: "disabled"}, Reaction: config.ReactionNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) require.NoError(t, n.PostStart(context.Background(), "Working")) require.NoError(t, n.PostCompletion(context.Background(), "Working", "failure")) @@ -1801,7 +1760,7 @@ func TestPostCompletion_ReactionOnFailure_SuppressedOnSuccess(t *testing.T) { Comment: config.CommentNotificationConfig{Start: "disabled", Completion: "disabled"}, Reaction: config.ReactionNotificationConfig{Start: "enabled", Completion: "on_failure"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) require.NoError(t, n.PostStart(context.Background(), "Working")) require.NoError(t, n.PostCompletion(context.Background(), "Working", "success")) @@ -1816,7 +1775,7 @@ func TestPostCompletion_ReactionOnFailure_FiresOnFailure(t *testing.T) { Comment: config.CommentNotificationConfig{Start: "disabled", Completion: "disabled"}, Reaction: config.ReactionNotificationConfig{Start: "enabled", Completion: "on_failure"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) require.NoError(t, n.PostStart(context.Background(), "Working")) require.NoError(t, n.PostCompletion(context.Background(), "Working", "failure")) @@ -1831,7 +1790,7 @@ func TestPostCompletion_ReactionDisabled_CleansUpStartReaction(t *testing.T) { Comment: config.CommentNotificationConfig{Start: "disabled", Completion: "disabled"}, Reaction: config.ReactionNotificationConfig{Start: "enabled", Completion: "disabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) require.NoError(t, n.PostStart(context.Background(), "Working")) require.NoError(t, n.PostCompletion(context.Background(), "Working", "success")) @@ -1846,7 +1805,7 @@ func TestPostCompletion_ReactionNoStartReaction(t *testing.T) { Comment: config.CommentNotificationConfig{Start: "disabled", Completion: "disabled"}, Reaction: config.ReactionNotificationConfig{Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) require.NoError(t, n.PostStart(context.Background(), "Working")) require.NoError(t, n.PostCompletion(context.Background(), "Working", "success")) @@ -1862,22 +1821,19 @@ func TestPostStart_ReactionErrorIsNonFatal(t *testing.T) { cfg := config.StatusNotificationConfig{ Reaction: config.ReactionNotificationConfig{Start: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, _ := newTestNotifier(fc, cfg) err := n.PostStart(context.Background(), "Working") require.NoError(t, err, "a failed reaction should not fail the run") } -// A comment API failure should leave the start reaction in place rather -// than swapping it to reflect a completion that was never successfully -// recorded — otherwise the reaction and comment tell contradictory stories. func TestPostCompletion_ReactionNotSwappedWhenCommentFails(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, Reaction: config.ReactionNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) + n, fc := newTestNotifier(fc, cfg) require.NoError(t, n.PostStart(context.Background(), "Working")) require.Len(t, fc.AddedReactions, 1) @@ -1898,8 +1854,8 @@ func TestPostStart_ReactionTargetsTriggeringComment(t *testing.T) { cfg := config.StatusNotificationConfig{ Reaction: config.ReactionNotificationConfig{Start: "enabled"}, } - n := newTestNotifier(fc, cfg) - n.SetTriggerCommentID(555) + n, fc := newTestNotifier(fc, cfg) + n.SetTriggerCommentID("555") require.NoError(t, n.PostStart(context.Background(), "Working")) @@ -1913,8 +1869,8 @@ func TestPostCompletion_ReactionTargetsTriggeringComment(t *testing.T) { cfg := config.StatusNotificationConfig{ Reaction: config.ReactionNotificationConfig{Start: "enabled", Completion: "enabled"}, } - n := newTestNotifier(fc, cfg) - n.SetTriggerCommentID(555) + n, fc := newTestNotifier(fc, cfg) + n.SetTriggerCommentID("555") require.NoError(t, n.PostStart(context.Background(), "Working")) require.NoError(t, n.PostCompletion(context.Background(), "Working", "success")) @@ -1925,3 +1881,27 @@ func TestPostCompletion_ReactionTargetsTriggeringComment(t *testing.T) { require.Len(t, fc.AddedCommentReactions, 2) assert.Equal(t, "+1", fc.AddedCommentReactions[1].Content) } + +// --- Tracker-agnostic destination test --- + +func TestNotifier_ReactionsSkippedForNonReactorTracker(t *testing.T) { + // Simulate a tracker.Client that does NOT implement tracker.Reactor + // (like a Jira client). Reactions should be silently skipped. + jiraFake, err := tracker.NewFakeJiraClient("https://acme.atlassian.net") + require.NoError(t, err) + + cfg := config.StatusNotificationConfig{ + Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "enabled"}, + Reaction: config.ReactionNotificationConfig{Start: "enabled", Completion: "enabled"}, + } + + n := New(jiraFake, cfg, "PROJ", 123, "https://ci/run/42", "a1b2c3d4e5f6789", "run-42") + n.now = fixedTime + + require.NoError(t, n.PostStart(context.Background(), "Code")) + require.NotEqual(t, "", n.startCommentID, "comment should still be posted") + assert.Equal(t, int64(0), n.startReactionID, "no reaction on non-Reactor tracker") + + n.now = func() time.Time { return fixedTime().Add(5 * time.Minute) } + require.NoError(t, n.PostCompletion(context.Background(), "Code", "success")) +} diff --git a/internal/tracker/fake_jira.go b/internal/tracker/fake_jira.go index 39fdb8e6c9..05d77dec1d 100644 --- a/internal/tracker/fake_jira.go +++ b/internal/tracker/fake_jira.go @@ -75,6 +75,17 @@ func (f *FakeJiraClient) UpdateComment(_ context.Context, issueIDOrKey, commentI return nil } +func (f *FakeJiraClient) DeleteComment(_ context.Context, issueIDOrKey, commentID string) error { + comments := f.Comments[issueIDOrKey] + for i, c := range comments { + if c.ID == commentID { + f.Comments[issueIDOrKey] = append(comments[:i], comments[i+1:]...) + return nil + } + } + return fmt.Errorf("delete comment %s on %s: %w", commentID, issueIDOrKey, forge.ErrNotFound) +} + var _ jiraClient = (*FakeJiraClient)(nil) // NewFakeJiraClient returns a tracker.Client backed by an in-memory fake diff --git a/internal/tracker/forge_client.go b/internal/tracker/forge_client.go index 77f7d3144e..42e47b83e4 100644 --- a/internal/tracker/forge_client.go +++ b/internal/tracker/forge_client.go @@ -97,6 +97,68 @@ func (c *ForgeClient) UpdateComment(ctx context.Context, project string, number return wrapNotFound(c.forge.UpdateIssueComment(ctx, owner, repo, id, string(body))) } +// DeleteComment implements Client by splitting project into owner/repo for +// the underlying forge call. +func (c *ForgeClient) DeleteComment(ctx context.Context, project string, number int, commentID string) error { + 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) + } + return wrapNotFound(c.forge.DeleteIssueComment(ctx, owner, repo, id)) +} + +// AddIssueReaction implements Reactor by splitting project into owner/repo +// for the underlying forge call. +func (c *ForgeClient) AddIssueReaction(ctx context.Context, project string, number int, content string) (int64, error) { + owner, repo, err := splitProject(project) + if err != nil { + return 0, err + } + return c.forge.AddIssueReaction(ctx, owner, repo, number, content) +} + +// DeleteIssueReaction implements Reactor by splitting project into +// owner/repo for the underlying forge call. +func (c *ForgeClient) DeleteIssueReaction(ctx context.Context, project string, number int, reactionID int64) error { + owner, repo, err := splitProject(project) + if err != nil { + return err + } + return c.forge.DeleteIssueReaction(ctx, owner, repo, number, reactionID) +} + +// AddCommentReaction implements Reactor by splitting project into +// owner/repo for the underlying forge call. +func (c *ForgeClient) AddCommentReaction(ctx context.Context, project string, number int, commentID string, content string) (int64, error) { + owner, repo, err := splitProject(project) + if err != nil { + return 0, err + } + id, err := strconv.Atoi(commentID) + if err != nil { + return 0, fmt.Errorf("tracker: comment ID %q is not numeric: %w", commentID, err) + } + return c.forge.AddIssueCommentReaction(ctx, owner, repo, id, content) +} + +// DeleteCommentReaction implements Reactor by splitting project into +// owner/repo for the underlying forge call. +func (c *ForgeClient) DeleteCommentReaction(ctx context.Context, project string, number int, commentID string, reactionID int64) error { + 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) + } + return c.forge.DeleteIssueCommentReaction(ctx, owner, repo, id, reactionID) +} + // 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 diff --git a/internal/tracker/jira_client.go b/internal/tracker/jira_client.go index a5a24eb0be..91f1d61c47 100644 --- a/internal/tracker/jira_client.go +++ b/internal/tracker/jira_client.go @@ -38,6 +38,7 @@ type jiraClient interface { ListComments(ctx context.Context, issueIDOrKey string) ([]jira.Comment, error) CreateComment(ctx context.Context, issueIDOrKey, body string) (*jira.Comment, error) UpdateComment(ctx context.Context, issueIDOrKey, commentID, body string) error + DeleteComment(ctx context.Context, issueIDOrKey, commentID string) error } var _ jiraClient = (*jira.LiveClient)(nil) @@ -121,6 +122,12 @@ func (c *JiraClient) UpdateComment(ctx context.Context, project string, number i return wrapNotFound(c.jira.UpdateComment(ctx, key, commentID, string(body))) } +// DeleteComment implements Client. +func (c *JiraClient) DeleteComment(ctx context.Context, project string, number int, commentID string) error { + key := issueKey(project, number) + return wrapNotFound(c.jira.DeleteComment(ctx, key, commentID)) +} + // fromJiraComment converts a jira.Comment to a tracker.Comment. HTMLURL is // deliberately left empty: Jira's comment permalink format isn't confirmed // against real Jira Cloud behavior, so rather than guess at a URL shape diff --git a/internal/tracker/tracker.go b/internal/tracker/tracker.go index 68cbac9d15..64a3a90b64 100644 --- a/internal/tracker/tracker.go +++ b/internal/tracker/tracker.go @@ -84,4 +84,25 @@ type Client interface { // 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 Body) error + // DeleteComment removes the comment identified by commentID from the issue. + // number is included because Jira requires the issue key to delete a comment. + DeleteComment(ctx context.Context, project string, number int, commentID string) error +} + +// Reactor is an optional capability for adding and removing emoji +// reactions on issues and comments. Not all trackers support reactions +// (e.g. Jira has no emoji-reaction model), so consumers should +// type-assert their tracker.Client to Reactor before calling reaction +// methods. Forge-backed tracker clients implement Reactor; Jira clients +// do not. +type Reactor interface { + // AddIssueReaction adds an emoji reaction to an issue or pull request. + // content is the reaction type (e.g. "eyes", "+1", "confused"). + AddIssueReaction(ctx context.Context, project string, number int, content string) (id int64, err error) + // DeleteIssueReaction removes a previously added issue reaction by ID. + DeleteIssueReaction(ctx context.Context, project string, number int, reactionID int64) error + // AddCommentReaction adds an emoji reaction to a specific comment. + AddCommentReaction(ctx context.Context, project string, number int, commentID string, content string) (id int64, err error) + // DeleteCommentReaction removes a previously added comment reaction by ID. + DeleteCommentReaction(ctx context.Context, project string, number int, commentID string, reactionID int64) error } diff --git a/internal/tracker/tracker_test.go b/internal/tracker/tracker_test.go index 5827bb8adf..cf149ff2c2 100644 --- a/internal/tracker/tracker_test.go +++ b/internal/tracker/tracker_test.go @@ -219,6 +219,10 @@ func (staticClient) CreateComment(_ context.Context, _ string, _ int, _ Body) (* func (staticClient) UpdateComment(_ context.Context, _ string, _ int, _ string, _ Body) error { return nil } +func (staticClient) DeleteComment(_ context.Context, _ string, _ int, _ string) error { + return nil +} var _ Client = staticClient{} var _ Client = (*ForgeClient)(nil) +var _ Reactor = (*ForgeClient)(nil) From 9c065bcdbc8e23a1437d0959a623280c7d1276f1 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Sat, 29 Aug 2026 20:58:51 +0000 Subject: [PATCH 2/4] fix: address review feedback on PR #6770 - Update stale package doc in internal/tracker/tracker.go: replace "Nothing calls tracker.Client yet" with current consumers (statuscomment and reconcilestatus) - Restore five test comments that explain non-obvious edge-case reasoning removed during the API surface refactoring: outcome-neutral synthesized label (#5736), script-controlled text must not forge markers, reaction consistency on comment failure, paragraph break for GitLab CommonMark, and crash-before-post synthesis (#3635) Addresses review feedback on #6770 --- internal/statuscomment/statuscomment_test.go | 19 +++++++++++++++++++ internal/tracker/tracker.go | 3 ++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/internal/statuscomment/statuscomment_test.go b/internal/statuscomment/statuscomment_test.go index a919d22a78..122cb83789 100644 --- a/internal/statuscomment/statuscomment_test.go +++ b/internal/statuscomment/statuscomment_test.go @@ -747,6 +747,11 @@ func TestReconcileOrphaned_EnabledMode_SynthesizesOnFailureWithNoMarker(t *testi tc := tracker.NewForgeClient(fc) setNow(t, time.Date(2026, 6, 3, 14, 0, 0, 0, time.UTC)) + // A status comment should always exist for a run that reached the harness + // under the default mode — its absence alongside a failed job means the + // process crashed before it could post anything at all (e.g. during + // environment validation), leaving maintainers unable to tell "no review + // was triggered" from "review was attempted and failed silently." See #3635. err := ReconcileOrphaned(context.Background(), tc, "org/repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "failure", false, "Review") require.NoError(t, err) @@ -809,6 +814,11 @@ func TestReconcileOrphaned_OnFailure_SynthesizesWhenSkippedEvenIfJobSucceeded(t comments := fc.IssueComments["org/repo/7"] require.Len(t, comments, 1, "should synthesize an interrupted comment despite jobStatus==success") + // The label is deliberately outcome-neutral: "no completion comment" is + // true whether the notifier failed to set up (never attempted a post) + // or its own skip-reason comment post failed — ReconcileOrphaned can't + // tell those apart, so it shouldn't assert a specific cause. See the + // review discussion on PR #5736. assert.Contains(t, comments[0].Body, "⏭️ Skipped (no completion comment)") assert.NotContains(t, comments[0].Body, "comment failed to post") assert.NotContains(t, comments[0].Body, "❌ Terminated") @@ -1521,6 +1531,8 @@ func TestSanitizeDetail_Truncates(t *testing.T) { assert.Equal(t, strings.Repeat("x", maxDetailLen)+"…", got) } +// A reason is script-controlled text, so it must not be able to forge the +// marker comments ReconcileOrphaned depends on, nor escape the status line. func TestPostCompletionWithDetail_DetailCannotForgeMarkers(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{ @@ -1540,6 +1552,10 @@ func TestPostCompletionWithDetail_DetailCannotForgeMarkers(t *testing.T) { assert.Contains(t, body, "⏭️ Skipped (evil <!-- fullsend:agent-status:999 -->)") } +// Verify that buildStartBody, buildCompletionBody, and +// buildInterruptedBody use \n\n (paragraph break) between the status +// line and the metadata line. A bare \n renders as inline whitespace +// on GitLab (strict CommonMark), collapsing the two lines into one. func TestParagraphBreak_BetweenStatusAndMetadata(t *testing.T) { t.Run("start body", func(t *testing.T) { fc := forge.NewFakeClient() @@ -1827,6 +1843,9 @@ func TestPostStart_ReactionErrorIsNonFatal(t *testing.T) { require.NoError(t, err, "a failed reaction should not fail the run") } +// A comment API failure should leave the start reaction in place rather +// than swapping it to reflect a completion that was never successfully +// recorded — otherwise the reaction and comment tell contradictory stories. func TestPostCompletion_ReactionNotSwappedWhenCommentFails(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{ diff --git a/internal/tracker/tracker.go b/internal/tracker/tracker.go index 64a3a90b64..6dc9cd605e 100644 --- a/internal/tracker/tracker.go +++ b/internal/tracker/tracker.go @@ -12,7 +12,8 @@ // GitLab. // // This package only defines the interface and thin adapters over -// forge.Client (see ForgeClient). Nothing calls tracker.Client yet. +// forge.Client (see ForgeClient). Consumers include statuscomment +// (run-status notifications) and reconcilestatus (orphan cleanup). package tracker import ( From 279cf28383db2b9b6caa009f34a67e3240e0f727 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Sun, 30 Aug 2026 15:38:55 +0000 Subject: [PATCH 3/4] test: add coverage for new tracker DeleteComment and Reactor methods Cover ForgeClient.DeleteComment, AddIssueReaction, DeleteIssueReaction, AddCommentReaction, DeleteCommentReaction; JiraClient.DeleteComment; FakeJiraClient via NewFakeJiraClient; and jira.LiveClient.DeleteComment. Raises tracker package coverage from 67% to 92%, satisfying the 80% codecov/patch threshold. Addresses review feedback on #6770 --- internal/forge/jira/client_test.go | 34 ++++++ internal/tracker/jira_client_test.go | 64 ++++++++++ internal/tracker/tracker_test.go | 168 +++++++++++++++++++++++++++ 3 files changed, 266 insertions(+) diff --git a/internal/forge/jira/client_test.go b/internal/forge/jira/client_test.go index 538d564fe5..c5131d4be0 100644 --- a/internal/forge/jira/client_test.go +++ b/internal/forge/jira/client_test.go @@ -396,6 +396,40 @@ func TestSetEntityProperty(t *testing.T) { require.NoError(t, err) } +// --------------------------------------------------------------------------- +// DeleteComment +// --------------------------------------------------------------------------- + +func TestDeleteComment(t *testing.T) { + t.Parallel() + client, mux := setupTest(t) + ctx := context.Background() + + mux.HandleFunc("/rest/api/3/issue/PROJ-1/comment/50001", func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, http.MethodDelete, r.Method) + w.WriteHeader(http.StatusNoContent) + }) + + err := client.DeleteComment(ctx, "PROJ-1", "50001") + require.NoError(t, err) +} + +func TestDeleteComment_NotFound(t *testing.T) { + t.Parallel() + client, mux := setupTest(t) + ctx := context.Background() + + mux.HandleFunc("/rest/api/3/issue/PROJ-1/comment/99999", func(w http.ResponseWriter, r *http.Request) { + writeJSON(t, w, http.StatusNotFound, map[string]any{ + "errorMessages": []string{"Comment does not exist."}, + }) + }) + + err := client.DeleteComment(ctx, "PROJ-1", "99999") + require.Error(t, err) + assert.Contains(t, err.Error(), "delete comment") +} + // --------------------------------------------------------------------------- // DeleteEntityProperty // --------------------------------------------------------------------------- diff --git a/internal/tracker/jira_client_test.go b/internal/tracker/jira_client_test.go index cf89f0574e..0c12c4034c 100644 --- a/internal/tracker/jira_client_test.go +++ b/internal/tracker/jira_client_test.go @@ -239,6 +239,51 @@ func TestJiraClient_UpdateComment(t *testing.T) { } } +func TestJiraClient_DeleteComment(t *testing.T) { + fc := &FakeJiraClient{} + c := newTestJiraClient(t, fc, "https://acme.atlassian.net") + ctx := context.Background() + + // Create a comment first so we have something to delete. + _, err := c.CreateComment(ctx, "PROJ", 42, "to be deleted") + if err != nil { + t.Fatalf("CreateComment returned error: %v", err) + } + + comments, err := c.ListComments(ctx, "PROJ", 42) + if err != nil { + t.Fatalf("ListComments returned error: %v", err) + } + if len(comments) != 1 { + t.Fatalf("expected 1 comment before delete, got %d", len(comments)) + } + + if err := c.DeleteComment(ctx, "PROJ", 42, comments[0].ID); err != nil { + t.Fatalf("DeleteComment returned error: %v", err) + } + + comments, err = c.ListComments(ctx, "PROJ", 42) + if err != nil { + t.Fatalf("ListComments returned error: %v", err) + } + if len(comments) != 0 { + t.Errorf("expected 0 comments after delete, got %d", len(comments)) + } +} + +func TestJiraClient_DeleteComment_NotFound(t *testing.T) { + fc := &FakeJiraClient{} + c := newTestJiraClient(t, fc, "https://acme.atlassian.net") + + err := c.DeleteComment(context.Background(), "PROJ", 42, "nonexistent") + if err == nil { + t.Fatal("DeleteComment on nonexistent comment: got nil error, want error") + } + if !IsNotFound(err) { + t.Errorf("DeleteComment error does not satisfy tracker.IsNotFound: %v", err) + } +} + func TestJiraClient_NotFoundWrapping(t *testing.T) { // JiraClient must wrap forge.ErrNotFound into tracker.ErrNotFound so // callers using tracker.IsNotFound get the expected result. Verify @@ -259,4 +304,23 @@ func TestJiraClient_NotFoundWrapping(t *testing.T) { } } +func TestNewFakeJiraClient(t *testing.T) { + c, err := NewFakeJiraClient("https://acme.atlassian.net") + if err != nil { + t.Fatalf("NewFakeJiraClient returned error: %v", err) + } + + ctx := context.Background() + created, err := c.CreateComment(ctx, "PROJ", 42, "test comment") + if err != nil { + t.Fatalf("CreateComment returned error: %v", err) + } + if created.ID == "" { + t.Error("CreateComment returned empty ID") + } + if created.Body != "test comment" { + t.Errorf("CreateComment body = %q, want %q", created.Body, "test comment") + } +} + var _ Client = (*JiraClient)(nil) diff --git a/internal/tracker/tracker_test.go b/internal/tracker/tracker_test.go index cf149ff2c2..da13762d03 100644 --- a/internal/tracker/tracker_test.go +++ b/internal/tracker/tracker_test.go @@ -205,6 +205,174 @@ func TestForgeClient_UpdateComment_InvalidID(t *testing.T) { } } +func TestForgeClient_DeleteComment(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "bot" + c := NewForgeClient(fc) + ctx := context.Background() + + created, err := c.CreateComment(ctx, "acme/widgets", 42, "to be deleted") + if err != nil { + t.Fatalf("CreateComment returned error: %v", err) + } + + if err := c.DeleteComment(ctx, "acme/widgets", 42, created.ID); err != nil { + t.Fatalf("DeleteComment 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) != 0 { + t.Errorf("expected 0 comments after delete, got %d", len(comments)) + } +} + +func TestForgeClient_DeleteComment_InvalidID(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + + err := c.DeleteComment(context.Background(), "acme/widgets", 42, "not-a-number") + if err == nil { + t.Fatal("DeleteComment with non-numeric ID should return an error") + } +} + +func TestForgeClient_DeleteComment_InvalidProject(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + + err := c.DeleteComment(context.Background(), "invalid", 42, "1") + if err == nil { + t.Fatal("DeleteComment with invalid project should return an error") + } +} + +func TestForgeClient_AddIssueReaction(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + ctx := context.Background() + + id, err := c.AddIssueReaction(ctx, "acme/widgets", 42, "eyes") + if err != nil { + t.Fatalf("AddIssueReaction returned error: %v", err) + } + if id == 0 { + t.Error("AddIssueReaction returned zero ID") + } + if len(fc.AddedReactions) != 1 || fc.AddedReactions[0].Content != "eyes" { + t.Errorf("unexpected reactions: %+v", fc.AddedReactions) + } +} + +func TestForgeClient_AddIssueReaction_InvalidProject(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + + _, err := c.AddIssueReaction(context.Background(), "invalid", 42, "eyes") + if err == nil { + t.Fatal("AddIssueReaction with invalid project should return an error") + } +} + +func TestForgeClient_DeleteIssueReaction(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + ctx := context.Background() + + id, err := c.AddIssueReaction(ctx, "acme/widgets", 42, "eyes") + if err != nil { + t.Fatalf("AddIssueReaction returned error: %v", err) + } + + if err := c.DeleteIssueReaction(ctx, "acme/widgets", 42, id); err != nil { + t.Fatalf("DeleteIssueReaction returned error: %v", err) + } +} + +func TestForgeClient_DeleteIssueReaction_InvalidProject(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + + err := c.DeleteIssueReaction(context.Background(), "invalid", 42, 1) + if err == nil { + t.Fatal("DeleteIssueReaction with invalid project should return an error") + } +} + +func TestForgeClient_AddCommentReaction(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + ctx := context.Background() + + id, err := c.AddCommentReaction(ctx, "acme/widgets", 42, "100", "+1") + if err != nil { + t.Fatalf("AddCommentReaction returned error: %v", err) + } + if id == 0 { + t.Error("AddCommentReaction returned zero ID") + } + if len(fc.AddedCommentReactions) != 1 || fc.AddedCommentReactions[0].Content != "+1" { + t.Errorf("unexpected comment reactions: %+v", fc.AddedCommentReactions) + } +} + +func TestForgeClient_AddCommentReaction_InvalidID(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + + _, err := c.AddCommentReaction(context.Background(), "acme/widgets", 42, "not-a-number", "+1") + if err == nil { + t.Fatal("AddCommentReaction with non-numeric comment ID should return an error") + } +} + +func TestForgeClient_AddCommentReaction_InvalidProject(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + + _, err := c.AddCommentReaction(context.Background(), "invalid", 42, "100", "+1") + if err == nil { + t.Fatal("AddCommentReaction with invalid project should return an error") + } +} + +func TestForgeClient_DeleteCommentReaction(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + ctx := context.Background() + + id, err := c.AddCommentReaction(ctx, "acme/widgets", 42, "100", "+1") + if err != nil { + t.Fatalf("AddCommentReaction returned error: %v", err) + } + + if err := c.DeleteCommentReaction(ctx, "acme/widgets", 42, "100", id); err != nil { + t.Fatalf("DeleteCommentReaction returned error: %v", err) + } +} + +func TestForgeClient_DeleteCommentReaction_InvalidID(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + + err := c.DeleteCommentReaction(context.Background(), "acme/widgets", 42, "not-a-number", 1) + if err == nil { + t.Fatal("DeleteCommentReaction with non-numeric comment ID should return an error") + } +} + +func TestForgeClient_DeleteCommentReaction_InvalidProject(t *testing.T) { + fc := forge.NewFakeClient() + c := NewForgeClient(fc) + + err := c.DeleteCommentReaction(context.Background(), "invalid", 42, "100", 1) + if err == nil { + t.Fatal("DeleteCommentReaction with invalid project 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{} From dd044f19ea5508f6991d32fbb78b816435f071d2 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Mon, 31 Aug 2026 18:43:57 +0000 Subject: [PATCH 4/4] docs: emphasize event-source routing and clarify Jira reaction support in ADR 0093 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restructure ADR 0093 to lead with dynamic event-source routing as the central architectural idea — the notification destination is determined by which tracker originated the event, not by the code-output forge. The forge.Client → tracker.Client refactoring is presented as a consequence of this routing decision rather than the headline. Correct the claim that Jira has no emoji-reaction support: Jira Cloud supports reactions on comments but not on issues. The Reactor interface remains optional because Jira's partial support doesn't fit the interface which includes issue-level reactions. Update architecture.md cross-reference, tracker.Reactor doc comment, statuscomment package/Notifier docs, and test comments to reflect the new framing consistently. Addresses #6770 --- ...093-tracker-routed-status-notifications.md | 114 ++++++++++++------ docs/architecture.md | 2 +- internal/statuscomment/statuscomment.go | 20 +-- internal/statuscomment/statuscomment_test.go | 4 +- internal/tracker/tracker.go | 13 +- 5 files changed, 99 insertions(+), 54 deletions(-) diff --git a/docs/ADRs/0093-tracker-routed-status-notifications.md b/docs/ADRs/0093-tracker-routed-status-notifications.md index e2636207cb..75d168be2f 100644 --- a/docs/ADRs/0093-tracker-routed-status-notifications.md +++ b/docs/ADRs/0093-tracker-routed-status-notifications.md @@ -1,5 +1,5 @@ --- -title: "93. Route run-status notifications by event provenance" +title: "93. Event-source routing for run-status notifications" status: Accepted relates_to: - agent-architecture @@ -12,7 +12,7 @@ topics: - portability --- -# 93. Route run-status notifications by event provenance +# 93. Event-source routing for run-status notifications Date: 2026-08-29 @@ -27,11 +27,17 @@ Builds on: ## Context -Agent run-status notifications (start comments, completion comments, emoji -reactions, and orphan reconciliation) were coupled to `forge.Client` with -`owner/repo/number` addressing. This works when the triggering work item -and the code repository live on the same forge, but breaks when an external -issue tracker drives the work. +When an agent run is triggered by an event — a GitHub issue comment, a +GitLab MR update, a Jira issue transition — the resulting status +notifications (start comments, completion comments, emoji reactions, +orphan reconciliation) must be posted back to the tracker that +*originated* the event, not necessarily to the forge that hosts the +code. + +Previously, status notifications were hardwired to `forge.Client` with +`owner/repo/number` addressing. This works when the triggering work +item and the code repository live on the same forge, but breaks when +an external issue tracker drives the work. The concrete failure: when a Jira issue triggers a code-agent run, the Jira issue number (e.g. 6964193) is passed to `forge.Client` as a GitHub @@ -42,12 +48,18 @@ Failed to post start status: posting start comment: create issue comment on #6964193: github api: 404 Not Found ``` +The key insight is that the notification destination should be +*dynamically determined by event provenance*: a Jira-triggered run +posts status to Jira, a GitHub-triggered run posts to GitHub, a +GitLab-triggered run posts to GitLab. The code repository and the +notification target are independent axes. + The repository already has a tracker-neutral comment interface (`tracker.Client`) with adapters for GitHub/GitLab (via `forge.Client`) and Jira. ADR 0086 established the domain split: `forge.Client` for git-hosting, `tracker.Client` for issue content, `conversation.Client` for chat. Status notifications are issue content, not git-hosting -operations, so they belong on `tracker.Client`. +operations, so they naturally route through `tracker.Client`. ## Options @@ -57,12 +69,20 @@ Add Jira-specific branching to `statuscomment.Notifier` alongside the existing `forge.Client` calls. Fast to implement but duplicates the tracker abstraction and grows worse with each new tracker backend. -### B. Route status notifications through `tracker.Client` (chosen) +### B. Dynamic event-source routing through `tracker.Client` (chosen) -Replace `forge.Client` with `tracker.Client` in the status-notification -path. Comments route to whichever tracker originated the run. Reactions -become an optional capability (`tracker.Reactor` interface) since not all -trackers support emoji reactions. +Let the event source determine the notification destination at +runtime. The notifier accepts a `tracker.Client`, and callers wire in +the appropriate tracker adapter based on which system originated the +event. This replaces `forge.Client` in the status-notification path +as a natural consequence: comments route to whichever tracker +originated the run. + +Reactions become an optional capability (`tracker.Reactor` interface) +since tracker reaction support varies. Jira Cloud supports reactions +on comments but not on issues; GitHub and GitLab support both. Making +`Reactor` optional keeps the interface clean for trackers with partial +or no reaction support. ### C. Post status to both the tracker and the forge @@ -72,7 +92,31 @@ adds complexity, and doubles API calls for every status update. ## Decision -Adopt **Option B**. +Adopt **Option B**: dynamically route status notifications based on +event provenance. + +The central change is architectural: the notification destination is +no longer hardwired to the code-output forge but is determined by the +event source at runtime. The interface refactoring (`forge.Client` → +`tracker.Client`) is a consequence of this routing decision — it makes +the notifier tracker-agnostic so any adapter can be wired in. + +### Event-source wiring + +Callers determine the tracker adapter based on the event source: + +- **GitHub-triggered runs**: callers construct + `tracker.NewForgeClient(ghClient)` — status posts to GitHub. +- **GitLab-triggered runs**: callers construct + `tracker.NewForgeClient(glClient)` — status posts to GitLab. +- **Jira-triggered runs**: callers construct a `tracker.JiraClient` — + status posts to Jira. + +The `ClientFactory` in the GitHub path returns +`tracker.NewForgeClient(gh.New(mintedToken))` so each token refresh +produces a tracker-wrapped client. The plumbing for reading the event +source from the normalized event and dynamically selecting the tracker +adapter is a follow-on; the interface is ready for it. ### Interface changes @@ -91,14 +135,18 @@ Adopt **Option B**. ``` `tracker.ForgeClient` implements `Reactor` (GitHub and GitLab support - emoji reactions). `tracker.JiraClient` does not (Jira has no - equivalent). Consumers type-assert to `Reactor` and silently skip - reaction operations when the tracker does not support them. + emoji reactions on both issues and comments). `tracker.JiraClient` + does not implement `Reactor` currently: Jira Cloud supports reactions + on comments but not on issues, and the partial support doesn't + cleanly fit the `Reactor` interface which includes issue-level + reactions. Adding Jira comment-reaction support is straightforward + once needed. Consumers type-assert to `Reactor` and silently skip + reaction operations when the tracker does not implement it. ### Notifier changes - `statuscomment.Notifier` accepts `tracker.Client` instead of - `forge.Client`. + `forge.Client`, making it tracker-agnostic. - Addressing changes from `(owner, repo string, number int)` to `(project string, number int)` to match `tracker.Client`'s project-keyed model. @@ -113,32 +161,22 @@ Adopt **Option B**. - The `reconcile-status` CLI command wraps its forge client in `tracker.NewForgeClient()` before calling `ReconcileOrphaned`. -### Call-site wiring - -Existing forge-based callers (`setupStatusNotifierGitHub`, -`setupStatusNotifierGitLab`, `reconcile-status` command) construct -`tracker.NewForgeClient(forgeClient)` and pass the result. The -`ClientFactory` in the GitHub path returns -`tracker.NewForgeClient(gh.New(mintedToken))` so each token refresh -produces a tracker-wrapped client. - -For Jira-triggered runs, the caller would construct a -`tracker.JiraClient` instead. The plumbing for reading source-system -from the normalized event is a follow-on; the interface is ready for it. - ## Consequences -- Status comments for Jira-triggered runs can now be routed to Jira - instead of producing a 404 on a non-existent GitHub issue. -- Reactions are silently skipped for trackers that do not support them - (e.g. Jira), rather than failing or falling back to an unrelated forge - issue. +- **Dynamic routing**: status notifications are directed to the tracker + that originated the event, not the code-output forge. A Jira-triggered + run posts status to Jira instead of producing a 404 on a non-existent + GitHub issue. +- Reactions are silently skipped for trackers that do not implement + `Reactor` (e.g. Jira, which supports comment reactions but not issue + reactions — partial support that doesn't fit the current interface). + Adding Jira comment-reaction support is a future option. - The `statuscomment` package no longer imports `internal/forge`, depending only on `internal/tracker` and `internal/config`. - Adding a new tracker backend (e.g. Linear, Azure DevOps) requires only implementing `tracker.Client` (and optionally `tracker.Reactor`); - status notifications work automatically. -- The orphan reconciler uses the same tracker-aware routing, so + status notifications work automatically via event-source routing. +- The orphan reconciler uses the same event-source routing, so interrupted runs on Jira issues are also finalized correctly. - `jira.LiveClient` gains a `DeleteComment` method to satisfy the extended `tracker.Client` interface. diff --git a/docs/architecture.md b/docs/architecture.md index c2875f35ee..196e5872a6 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -43,7 +43,7 @@ the dedicated org-level `/.fullsend` config repo is deprecated - Forge abstraction: all forge operations go through the `forge.Client` interface, keeping the rest of the codebase forge-agnostic ([ADR 0005](ADRs/0005-forge-abstraction-layer.md)). - Conversation surface: agents participate in GitHub Discussions and later other chat systems through a narrow `conversation.Client` (parallel to `tracker.Client` for issue content), not by extending `forge.Client` ([ADR 0086](ADRs/0086-conversation-surface-for-agent-participation.md)). A **conversation** is the container (Discussion / Slack channel) with exactly one category and optional M:M labels; a **thread** is the top-level message plus replies that share its `parent_id` (`parent_id == id` on the root message). -- Tracker-routed status notifications: run-status comments and reactions route through `tracker.Client` (not `forge.Client`), so the notification destination is determined by event provenance — a Jira-triggered run posts status to Jira, a GitHub-triggered run posts to GitHub. Reactions are an optional `tracker.Reactor` capability; trackers without reaction support (e.g. Jira) silently skip them ([ADR 0093](ADRs/0093-tracker-routed-status-notifications.md)). +- Event-source routing for status notifications: the notification destination for run-status comments and reactions is dynamically determined by event provenance — a Jira-triggered run posts status to Jira, a GitHub-triggered run posts to GitHub — rather than being hardwired to the code-output forge. Status notifications route through `tracker.Client`; reactions are an optional `tracker.Reactor` capability (Jira Cloud supports comment reactions but not issue reactions, so `Reactor` is not implemented for Jira currently) ([ADR 0093](ADRs/0093-tracker-routed-status-notifications.md)). - Installation model: ordered layer stack (install forward, uninstall reverse, analyze for status reporting) with idempotent operations. Current stack: config-repo → workflows → vendor-binary → secrets → inference → dispatch → enrollment ([ADR 0006](ADRs/0006-ordered-layer-model.md)). - Cross-repo dispatch: enrolled repos call `.fullsend` via `workflow_call`; a dispatch workflow mints OIDC tokens exchanged at a central token mint (GCP Cloud Function or Cloudflare Worker) for scoped GitHub App installation tokens per agent role. App PEM secrets are stored in Secret Manager (GCF mint), Worker secrets (CF mint), or the local filesystem (standalone mint), not the config repo ([ADR 0008](ADRs/0008-workflow-dispatch-for-cross-repo-dispatch.md)). - Shim workflow security: `pull_request_target` prevents PR authors from modifying the shim workflow. No long-lived secrets flow through the shim — OIDC tokens are issued by the GitHub runtime and scoped to the workflow run ([ADR 0009](ADRs/0009-pull-request-target-in-shim-workflows.md)). diff --git a/internal/statuscomment/statuscomment.go b/internal/statuscomment/statuscomment.go index b8f8445eb2..77b742178a 100644 --- a/internal/statuscomment/statuscomment.go +++ b/internal/statuscomment/statuscomment.go @@ -8,10 +8,11 @@ // completion (including cancellation). The two packages share the HTML-marker // convention but have different lifecycles and placement heuristics. // -// Status comments are routed to the issue tracker that originated the -// run (GitHub, GitLab, or Jira) via tracker.Client, independently of -// the forge used for code output (branches, PRs/MRs). This separation -// is recorded in ADR 0093. +// The notification destination is dynamically determined by event +// provenance: a Jira-triggered run posts status to Jira, a +// GitHub-triggered run posts to GitHub, independently of the forge +// used for code output (branches, PRs/MRs). This event-source routing +// is implemented via tracker.Client and recorded in ADR 0093. package statuscomment import ( @@ -72,11 +73,12 @@ type RunInfo struct { } // Notifier manages status comment lifecycle for a single agent run. -// It posts status comments to the tracker that originated the run, -// independently of the forge used for code output (branches, PRs/MRs). +// The notification destination is dynamically determined by the event +// source — callers wire in the appropriate tracker.Client adapter based +// on which system originated the triggering event. type Notifier struct { client tracker.Client - reactor tracker.Reactor // optional; nil when the tracker has no reaction support + reactor tracker.Reactor // optional; nil when the tracker does not implement Reactor clientFactory ClientFactory cfg config.StatusNotificationConfig project string @@ -287,7 +289,7 @@ func (n *Notifier) PostStart(ctx context.Context, description string) error { // instead of the issue/PR when this run was invoked by a slash command. // See SetTriggerCommentID. // -// Returns (0, nil) when the tracker does not support reactions. +// Returns (0, nil) when the tracker does not implement Reactor. func (n *Notifier) addReaction(ctx context.Context, content string) (int64, error) { if n.reactor == nil { return 0, nil @@ -301,7 +303,7 @@ func (n *Notifier) addReaction(ctx context.Context, content string) (int64, erro // deleteReaction removes a previously added reaction, mirroring the // comment-vs-issue targeting addReaction uses to add it. // -// No-ops when the tracker does not support reactions. +// No-ops when the tracker does not implement Reactor. func (n *Notifier) deleteReaction(ctx context.Context, reactionID int64) error { if n.reactor == nil { return nil diff --git a/internal/statuscomment/statuscomment_test.go b/internal/statuscomment/statuscomment_test.go index 122cb83789..f06f87fd02 100644 --- a/internal/statuscomment/statuscomment_test.go +++ b/internal/statuscomment/statuscomment_test.go @@ -1905,7 +1905,9 @@ func TestPostCompletion_ReactionTargetsTriggeringComment(t *testing.T) { func TestNotifier_ReactionsSkippedForNonReactorTracker(t *testing.T) { // Simulate a tracker.Client that does NOT implement tracker.Reactor - // (like a Jira client). Reactions should be silently skipped. + // (like a Jira client — Jira Cloud supports comment reactions but not + // issue reactions, so JiraClient doesn't implement the full Reactor + // interface). Reactions should be silently skipped. jiraFake, err := tracker.NewFakeJiraClient("https://acme.atlassian.net") require.NoError(t, err) diff --git a/internal/tracker/tracker.go b/internal/tracker/tracker.go index 6dc9cd605e..87b40b72d8 100644 --- a/internal/tracker/tracker.go +++ b/internal/tracker/tracker.go @@ -91,11 +91,14 @@ type Client interface { } // Reactor is an optional capability for adding and removing emoji -// reactions on issues and comments. Not all trackers support reactions -// (e.g. Jira has no emoji-reaction model), so consumers should -// type-assert their tracker.Client to Reactor before calling reaction -// methods. Forge-backed tracker clients implement Reactor; Jira clients -// do not. +// reactions on issues and comments. Tracker reaction support varies: +// GitHub and GitLab support reactions on both issues and comments; +// Jira Cloud supports reactions on comments but not on issues. Because +// the interface includes issue-level reactions, JiraClient does not +// implement Reactor currently — adding Jira comment-reaction support +// is straightforward once needed. Consumers should type-assert their +// tracker.Client to Reactor before calling reaction methods and +// silently skip reactions when the tracker does not implement it. type Reactor interface { // AddIssueReaction adds an emoji reaction to an issue or pull request. // content is the reaction type (e.g. "eyes", "+1", "confused").