From 87f9eca15a0fceb977b62dbefdd280cc1770d357 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Wed, 29 Jul 2026 13:39:03 -0400 Subject: [PATCH 01/22] feat(#3697): add on_failure mode for comment.completion status notifications MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allow status_notifications.comment.completion to be set to "on_failure", which posts a completion comment only when the agent fails or is cancelled. On success the start comment is silently removed. This reduces notification noise while still surfacing failures. - Extend config validation to accept "on_failure" for completion fields (rejected for start fields where there is no outcome yet) - Add shouldPostCompletion() helper that evaluates on_failure against the agent outcome status - Replace commentEnabled() with shouldPostCompletion() in PostCompletion - Add unit tests covering all on_failure × status combinations - Update operations.md to document the new option Part of #3697 (phase 1 — comment changes only; reaction support is a follow-up) Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- docs/guides/getting-started/operations.md | 4 +- internal/config/config.go | 9 +- internal/config/config_test.go | 55 ++++++++++ internal/statuscomment/statuscomment.go | 18 +++- internal/statuscomment/statuscomment_test.go | 101 +++++++++++++++++++ 5 files changed, 179 insertions(+), 8 deletions(-) diff --git a/docs/guides/getting-started/operations.md b/docs/guides/getting-started/operations.md index 44b63c700f..20c2ff566a 100644 --- a/docs/guides/getting-started/operations.md +++ b/docs/guides/getting-started/operations.md @@ -185,7 +185,7 @@ defaults: status_notifications: comment: start: enabled # "enabled" (default) | "disabled" - completion: enabled # "enabled" (default) | "disabled" + completion: enabled # "enabled" (default) | "on_failure" | "disabled" ``` For per-repo installs, set it at the top level of `.fullsend/config.yaml`: @@ -199,6 +199,8 @@ status_notifications: When `status_notifications` is omitted, comments default to enabled. +Setting `completion` to `on_failure` posts a completion comment only when the agent fails or is cancelled. On success, the start comment is silently removed. This reduces notification noise while still surfacing failures. + The composite action accepts four optional inputs for status notifications: | Input | Description | diff --git a/internal/config/config.go b/internal/config/config.go index c6799d1ed3..3b1dec3f3c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -450,12 +450,13 @@ func validateStatusNotifications(cfg *StatusNotificationConfig) error { if cfg == nil { return nil } - validCommentValues := []string{"", "enabled", "disabled"} - if !slices.Contains(validCommentValues, cfg.Comment.Start) { + validStartValues := []string{"", "enabled", "disabled"} + if !slices.Contains(validStartValues, cfg.Comment.Start) { return fmt.Errorf("invalid status_notifications.comment.start %q: must be \"enabled\" or \"disabled\"", cfg.Comment.Start) } - if !slices.Contains(validCommentValues, cfg.Comment.Completion) { - return fmt.Errorf("invalid status_notifications.comment.completion %q: must be \"enabled\" or \"disabled\"", cfg.Comment.Completion) + validCompletionValues := []string{"", "enabled", "disabled", "on_failure"} + if !slices.Contains(validCompletionValues, cfg.Comment.Completion) { + return fmt.Errorf("invalid status_notifications.comment.completion %q: must be \"enabled\", \"on_failure\", or \"disabled\"", cfg.Comment.Completion) } return nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 694c39609c..31abfb3692 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -876,6 +876,61 @@ func TestOrgConfigValidate_InvalidCommentCompletion(t *testing.T) { assert.Contains(t, err.Error(), "status_notifications.comment.completion") } +func TestOrgConfigValidate_OnFailureCompletion(t *testing.T) { + cfg := &orgConfig{ + Version: "1", + Dispatch: DispatchConfig{Platform: "github-actions"}, + Defaults: RepoDefaults{ + Roles: []string{"fullsend"}, + MaxImplementationRetries: 2, + StatusNotifications: &StatusNotificationConfig{ + Comment: CommentNotificationConfig{Completion: "on_failure"}, + }, + }, + } + assert.NoError(t, cfg.Validate(), "on_failure should be valid for comment.completion") +} + +func TestOrgConfigValidate_OnFailureStart_Rejected(t *testing.T) { + cfg := &orgConfig{ + Version: "1", + Dispatch: DispatchConfig{Platform: "github-actions"}, + Defaults: RepoDefaults{ + Roles: []string{"fullsend"}, + MaxImplementationRetries: 2, + StatusNotifications: &StatusNotificationConfig{ + Comment: CommentNotificationConfig{Start: "on_failure"}, + }, + }, + } + err := cfg.Validate() + assert.Error(t, err, "on_failure should be rejected for comment.start") + assert.Contains(t, err.Error(), "status_notifications.comment.start") +} + +func TestParseOrgConfig_OnFailureCompletion(t *testing.T) { + yamlData := ` +version: "1" +dispatch: + platform: github-actions +defaults: + roles: + - fullsend + max_implementation_retries: 2 + status_notifications: + comment: + start: disabled + completion: on_failure +agents: [] +repos: {} +` + cfg, err := ParseOrgConfig([]byte(yamlData)) + require.NoError(t, err) + require.NotNil(t, cfg.StatusNotifications()) + assert.Equal(t, "disabled", cfg.StatusNotifications().Comment.Start) + assert.Equal(t, "on_failure", cfg.StatusNotifications().Comment.Completion) +} + func TestOrgConfigMarshal_WithStatusNotifications(t *testing.T) { cfg := &orgConfig{ Version: "1", diff --git a/internal/statuscomment/statuscomment.go b/internal/statuscomment/statuscomment.go index b771b7f414..d866f491bd 100644 --- a/internal/statuscomment/statuscomment.go +++ b/internal/statuscomment/statuscomment.go @@ -123,6 +123,17 @@ func commentEnabled(val string) bool { return val == "" || val == "enabled" } +// shouldPostCompletion reports whether a completion comment should be +// posted given the configured value and the agent outcome status. +func shouldPostCompletion(val, status string) bool { + switch val { + case "on_failure": + return status != "success" + default: + return commentEnabled(val) + } +} + // PostStart posts a start comment on the issue/PR. func (n *Notifier) PostStart(ctx context.Context, description string) error { n.startTime = n.now().UTC() @@ -168,9 +179,10 @@ func (n *Notifier) PostCompletion(ctx context.Context, description, status strin func (n *Notifier) PostCompletionWithDetail(ctx context.Context, description, status, detail string) error { completionTime := n.now().UTC() - if !commentEnabled(n.cfg.Comment.Completion) { - // Completion comments disabled — clean up the start comment so it - // doesn't remain orphaned in its "Started" state. + if !shouldPostCompletion(n.cfg.Comment.Completion, status) { + // Completion comment suppressed (disabled or on_failure with success) — + // clean up the start comment so it doesn't remain orphaned in its + // "Started" state. if n.startCommentID != 0 { if err := n.refreshClient(ctx); err != nil { n.warnf("failed to mint token for start comment cleanup: %v", err) diff --git a/internal/statuscomment/statuscomment_test.go b/internal/statuscomment/statuscomment_test.go index 1df71ac072..c72964ca4b 100644 --- a/internal/statuscomment/statuscomment_test.go +++ b/internal/statuscomment/statuscomment_test.go @@ -1077,6 +1077,107 @@ func TestClientFactory_CompletionDisabled_MintError(t *testing.T) { assert.Contains(t, warnings[0], "mint service down") } +// --- on_failure completion tests --- + +func TestPostCompletion_OnFailure_SuppressedOnSuccess(t *testing.T) { + fc := forge.NewFakeClient() + cfg := config.StatusNotificationConfig{ + Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "on_failure"}, + } + n := newTestNotifier(fc, cfg) + + err := n.PostStart(context.Background(), "Working") + require.NoError(t, err) + require.Equal(t, 1, n.startCommentID) + + n.now = func() time.Time { return fixedTime().Add(time.Minute) } + err = n.PostCompletion(context.Background(), "Working", "success") + require.NoError(t, err) + + // Success should suppress the completion comment and clean up the start comment. + assert.Empty(t, fc.UpdatedComments, "should not update start comment on success") + require.Len(t, fc.DeletedComments, 1, "should delete start comment on success") + assert.Equal(t, 1, fc.DeletedComments[0]) +} + +func TestPostCompletion_OnFailure_PostsOnFailure(t *testing.T) { + fc := forge.NewFakeClient() + cfg := config.StatusNotificationConfig{ + Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "on_failure"}, + } + n := newTestNotifier(fc, cfg) + + err := n.PostStart(context.Background(), "Coding issue #42") + require.NoError(t, err) + + n.now = func() time.Time { return fixedTime().Add(10 * time.Minute) } + err = n.PostCompletion(context.Background(), "Coding issue #42", "failure") + require.NoError(t, err) + + // Failure should post the completion comment. + require.Len(t, fc.UpdatedComments, 1) + assert.Contains(t, fc.UpdatedComments[0].Body, "Finished Coding issue #42") + assert.Contains(t, fc.UpdatedComments[0].Body, "❌ Failure") +} + +func TestPostCompletion_OnFailure_PostsOnCancelled(t *testing.T) { + fc := forge.NewFakeClient() + cfg := config.StatusNotificationConfig{ + Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "on_failure"}, + } + n := newTestNotifier(fc, cfg) + + err := n.PostStart(context.Background(), "Working") + require.NoError(t, err) + + n.now = func() time.Time { return fixedTime().Add(2 * time.Minute) } + err = n.PostCompletion(context.Background(), "Working", "cancelled") + require.NoError(t, err) + + // Cancelled is non-success, so completion should fire. + require.Len(t, fc.UpdatedComments, 1) + assert.Contains(t, fc.UpdatedComments[0].Body, "⚠️ Cancelled") +} + +func TestPostCompletion_OnFailure_NoStartComment_PostsOnFailure(t *testing.T) { + fc := forge.NewFakeClient() + cfg := config.StatusNotificationConfig{ + Comment: config.CommentNotificationConfig{Start: "disabled", Completion: "on_failure"}, + } + n := newTestNotifier(fc, cfg) + + err := n.PostStart(context.Background(), "Working") + require.NoError(t, err) + assert.Equal(t, 0, n.startCommentID) + + n.now = func() time.Time { return fixedTime().Add(time.Minute) } + err = n.PostCompletion(context.Background(), "Working", "failure") + require.NoError(t, err) + + comments := fc.IssueComments["org/repo/7"] + require.Len(t, comments, 1, "should post completion on failure") + assert.Contains(t, comments[0].Body, "❌ Failure") +} + +func TestPostCompletion_OnFailure_NoStartComment_SuppressedOnSuccess(t *testing.T) { + fc := forge.NewFakeClient() + cfg := config.StatusNotificationConfig{ + Comment: config.CommentNotificationConfig{Start: "disabled", Completion: "on_failure"}, + } + n := newTestNotifier(fc, cfg) + + err := n.PostStart(context.Background(), "Working") + require.NoError(t, err) + + n.now = func() time.Time { return fixedTime().Add(time.Minute) } + err = n.PostCompletion(context.Background(), "Working", "success") + require.NoError(t, err) + + assert.Empty(t, fc.IssueComments, "should not post anything on success with on_failure") + assert.Empty(t, fc.UpdatedComments) + assert.Empty(t, fc.DeletedComments) +} + func TestClientFactory_CompletionDisabled_DeleteError(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{ From e5d30a72b465ee1c965e176c01807431bf5fecde Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Wed, 29 Jul 2026 13:45:57 -0400 Subject: [PATCH 02/22] docs(#3697): move status notifications to user-facing config guide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The status_notifications docs were in operations.md (infrastructure guide). Move them to customizing-agents.md where users configure agent behavior, and update the cross-reference from running-agents-locally.md. Also revert the on_failure addition from operations.md — the authoritative docs now live in the user guide. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- docs/guides/getting-started/operations.md | 4 +--- docs/guides/user/customizing-agents.md | 24 ++++++++++++++++++++++ docs/guides/user/running-agents-locally.md | 2 +- 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/docs/guides/getting-started/operations.md b/docs/guides/getting-started/operations.md index 20c2ff566a..44b63c700f 100644 --- a/docs/guides/getting-started/operations.md +++ b/docs/guides/getting-started/operations.md @@ -185,7 +185,7 @@ defaults: status_notifications: comment: start: enabled # "enabled" (default) | "disabled" - completion: enabled # "enabled" (default) | "on_failure" | "disabled" + completion: enabled # "enabled" (default) | "disabled" ``` For per-repo installs, set it at the top level of `.fullsend/config.yaml`: @@ -199,8 +199,6 @@ status_notifications: When `status_notifications` is omitted, comments default to enabled. -Setting `completion` to `on_failure` posts a completion comment only when the agent fails or is cancelled. On success, the start comment is silently removed. This reduces notification noise while still surfacing failures. - The composite action accepts four optional inputs for status notifications: | Input | Description | diff --git a/docs/guides/user/customizing-agents.md b/docs/guides/user/customizing-agents.md index 5d02c3d3dc..48e572ebc1 100644 --- a/docs/guides/user/customizing-agents.md +++ b/docs/guides/user/customizing-agents.md @@ -525,6 +525,30 @@ my-repo/ │ └── harness/code.yaml # Repo-specific harness configuration ``` +## Status Notifications + +Agent workflows post status comments on issues and PRs when they start and complete. Control this with `status_notifications` in `config.yaml`: + +```yaml +defaults: + status_notifications: + comment: + start: enabled # "enabled" (default) | "disabled" + completion: enabled # "enabled" (default) | "on_failure" | "disabled" +``` + +When `status_notifications` is omitted entirely, both start and completion comments default to enabled. + +### Completion modes + +| Value | Behavior | +|-------|----------| +| `enabled` | Always post a completion comment (default) | +| `on_failure` | Post only when the agent fails or is cancelled; on success, silently remove the start comment | +| `disabled` | Never post a completion comment; silently remove the start comment | + +`on_failure` is useful when you want to reduce notification noise — successful runs leave no trace, but failures still surface. + ## Disabling Agents To disable an agent (including built-in scaffold agents) without removing diff --git a/docs/guides/user/running-agents-locally.md b/docs/guides/user/running-agents-locally.md index 398404bbfc..7bd719491a 100644 --- a/docs/guides/user/running-agents-locally.md +++ b/docs/guides/user/running-agents-locally.md @@ -270,7 +270,7 @@ fullsend run triage \ For GitLab repositories, use `--forge gitlab` instead of `--mint-url`. The agent reads `GITLAB_TOKEN` from the environment and does not require the mint service. See the [operations guide](../getting-started/operations.md#gitlab-ci) for required environment variables. Status comment behavior is configured via `status_notifications` in -`config.yaml`. See the [operations guide](../getting-started/operations.md#status-notifications). +`config.yaml`. See [Status Notifications](customizing-agents.md#status-notifications). ## Run from a container From 819e6c3fc9e64f5f5068bc3437f6b89483b003ca Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Wed, 29 Jul 2026 16:47:12 -0400 Subject: [PATCH 03/22] docs(#3697): fix stale status notification doc comment and deduplicate operations.md Update CommentNotificationConfig doc comment to list the valid values per field now that start and completion accept different sets. Replace the duplicated status notifications prose in operations.md with a cross-reference to the canonical section in customizing-agents.md. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- docs/guides/getting-started/operations.md | 23 +---------------------- docs/guides/user/customizing-agents.md | 13 ++++++++++++- internal/config/config.go | 3 ++- 3 files changed, 15 insertions(+), 24 deletions(-) diff --git a/docs/guides/getting-started/operations.md b/docs/guides/getting-started/operations.md index 44b63c700f..5b287c55fc 100644 --- a/docs/guides/getting-started/operations.md +++ b/docs/guides/getting-started/operations.md @@ -176,28 +176,7 @@ gcloud services enable \ ## Status notifications -Agent workflows post status comments on issues and PRs when they start and complete. This behavior is controlled by the `status_notifications` section in `config.yaml`. - -For per-org installs, nest it under `defaults`: - -```yaml -defaults: - status_notifications: - comment: - start: enabled # "enabled" (default) | "disabled" - completion: enabled # "enabled" (default) | "disabled" -``` - -For per-repo installs, set it at the top level of `.fullsend/config.yaml`: - -```yaml -status_notifications: - comment: - start: enabled - completion: enabled -``` - -When `status_notifications` is omitted, comments default to enabled. +See [Status Notifications](../user/customizing-agents.md#status-notifications) for configuring start and completion comments. The composite action accepts four optional inputs for status notifications: diff --git a/docs/guides/user/customizing-agents.md b/docs/guides/user/customizing-agents.md index 48e572ebc1..3bd4f92894 100644 --- a/docs/guides/user/customizing-agents.md +++ b/docs/guides/user/customizing-agents.md @@ -527,7 +527,9 @@ my-repo/ ## Status Notifications -Agent workflows post status comments on issues and PRs when they start and complete. Control this with `status_notifications` in `config.yaml`: +Agent workflows post status comments on issues and PRs when they start and complete. Control this with `status_notifications` in `config.yaml`. + +For per-org installs, nest it under `defaults`: ```yaml defaults: @@ -537,6 +539,15 @@ defaults: completion: enabled # "enabled" (default) | "on_failure" | "disabled" ``` +For per-repo installs, set it at the top level of `.fullsend/config.yaml`: + +```yaml +status_notifications: + comment: + start: enabled + completion: enabled +``` + When `status_notifications` is omitted entirely, both start and completion comments default to enabled. ### Completion modes diff --git a/internal/config/config.go b/internal/config/config.go index 3b1dec3f3c..ecad0a2b6d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -112,7 +112,8 @@ type StatusNotificationConfig struct { } // CommentNotificationConfig controls start/completion comments. -// Valid values: "enabled" (default when parent is set), "disabled". +// Valid start values: "enabled" (default), "disabled". +// Valid completion values: "enabled" (default), "on_failure", "disabled". type CommentNotificationConfig struct { Start string `yaml:"start,omitempty"` Completion string `yaml:"completion,omitempty"` From 75a56e16d67be7f1f59c3be26b122b1bf095d9fd Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Wed, 29 Jul 2026 17:35:44 -0400 Subject: [PATCH 04/22] fix(#3697): auto-suppress start comment when completion is on_failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When completion is set to on_failure, posting a start comment and then deleting it on success still triggers a GitHub notification pointing to a deleted comment — defeating the purpose of reducing noise. Now the start comment is automatically suppressed regardless of the start setting when completion is on_failure. Also fixes the cleanup warning message to say "suppressed" instead of "disabled" (covers both cases), and clarifies docs that status_notifications is org-level only. Signed-off-by: Ralph Bean Assisted-by: Claude claude-opus-4-6 Signed-off-by: Ralph Bean --- docs/guides/user/customizing-agents.md | 4 +-- internal/statuscomment/statuscomment.go | 9 ++++-- internal/statuscomment/statuscomment_test.go | 30 ++++++++++++-------- 3 files changed, 27 insertions(+), 16 deletions(-) diff --git a/docs/guides/user/customizing-agents.md b/docs/guides/user/customizing-agents.md index 3bd4f92894..9debbbc5fd 100644 --- a/docs/guides/user/customizing-agents.md +++ b/docs/guides/user/customizing-agents.md @@ -555,10 +555,10 @@ When `status_notifications` is omitted entirely, both start and completion comme | Value | Behavior | |-------|----------| | `enabled` | Always post a completion comment (default) | -| `on_failure` | Post only when the agent fails or is cancelled; on success, silently remove the start comment | +| `on_failure` | Post only when the agent fails or is cancelled; the start comment is automatically suppressed to avoid notification noise | | `disabled` | Never post a completion comment; silently remove the start comment | -`on_failure` is useful when you want to reduce notification noise — successful runs leave no trace, but failures still surface. +`on_failure` is useful when you want to reduce notification noise — successful runs leave no trace, but failures still surface. When `completion` is set to `on_failure`, the start comment is automatically suppressed regardless of the `start` setting, because posting and then deleting a start comment would still trigger a GitHub notification pointing to a deleted comment. ## Disabling Agents diff --git a/internal/statuscomment/statuscomment.go b/internal/statuscomment/statuscomment.go index d866f491bd..904c1a51ce 100644 --- a/internal/statuscomment/statuscomment.go +++ b/internal/statuscomment/statuscomment.go @@ -135,10 +135,15 @@ func shouldPostCompletion(val, status string) bool { } // PostStart posts a start comment on the issue/PR. +// +// When completion is set to "on_failure", the start comment is automatically +// suppressed regardless of the start setting. Posting a start comment that +// gets deleted on success would still trigger a GitHub notification pointing +// to a deleted comment — defeating the purpose of reducing noise. func (n *Notifier) PostStart(ctx context.Context, description string) error { n.startTime = n.now().UTC() - if commentEnabled(n.cfg.Comment.Start) { + if commentEnabled(n.cfg.Comment.Start) && n.cfg.Comment.Completion != "on_failure" { if err := n.refreshClient(ctx); err != nil { return err } @@ -187,7 +192,7 @@ func (n *Notifier) PostCompletionWithDetail(ctx context.Context, description, st if err := n.refreshClient(ctx); err != nil { n.warnf("failed to mint token for start comment cleanup: %v", err) } else if err := n.client.DeleteIssueComment(ctx, n.owner, n.repo, n.startCommentID); err != nil { - n.warnf("failed to delete start comment when completion disabled: %v", err) + n.warnf("failed to delete start comment when completion suppressed: %v", err) } } return nil diff --git a/internal/statuscomment/statuscomment_test.go b/internal/statuscomment/statuscomment_test.go index c72964ca4b..727d1a9c6d 100644 --- a/internal/statuscomment/statuscomment_test.go +++ b/internal/statuscomment/statuscomment_test.go @@ -1088,16 +1088,16 @@ func TestPostCompletion_OnFailure_SuppressedOnSuccess(t *testing.T) { err := n.PostStart(context.Background(), "Working") require.NoError(t, err) - require.Equal(t, 1, n.startCommentID) + assert.Equal(t, 0, 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) - // Success should suppress the completion comment and clean up the start comment. - assert.Empty(t, fc.UpdatedComments, "should not update start comment on success") - require.Len(t, fc.DeletedComments, 1, "should delete start comment on success") - assert.Equal(t, 1, fc.DeletedComments[0]) + // 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) } func TestPostCompletion_OnFailure_PostsOnFailure(t *testing.T) { @@ -1109,15 +1109,18 @@ func TestPostCompletion_OnFailure_PostsOnFailure(t *testing.T) { err := n.PostStart(context.Background(), "Coding issue #42") require.NoError(t, err) + assert.Equal(t, 0, 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) - // Failure should post the completion comment. - require.Len(t, fc.UpdatedComments, 1) - assert.Contains(t, fc.UpdatedComments[0].Body, "Finished Coding issue #42") - assert.Contains(t, fc.UpdatedComments[0].Body, "❌ Failure") + // 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") + assert.Contains(t, comments[0].Body, "Finished Coding issue #42") + assert.Contains(t, comments[0].Body, "❌ Failure") } func TestPostCompletion_OnFailure_PostsOnCancelled(t *testing.T) { @@ -1129,14 +1132,17 @@ func TestPostCompletion_OnFailure_PostsOnCancelled(t *testing.T) { err := n.PostStart(context.Background(), "Working") require.NoError(t, err) + assert.Equal(t, 0, 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) - // Cancelled is non-success, so completion should fire. - require.Len(t, fc.UpdatedComments, 1) - assert.Contains(t, fc.UpdatedComments[0].Body, "⚠️ Cancelled") + // 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") + assert.Contains(t, comments[0].Body, "⚠️ Cancelled") } func TestPostCompletion_OnFailure_NoStartComment_PostsOnFailure(t *testing.T) { From b95181e0226ccd215160a3d030e342307925cc5b Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Thu, 30 Jul 2026 12:30:47 -0400 Subject: [PATCH 05/22] fix(#3697): tighten on_failure predicate and fix orphan reconciliation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review-driven fixes: 1. shouldPostCompletion used status != "success", so "skipped" runs triggered completion comments under on_failure — contradicting the documented behavior ("only on failure or cancellation"). Tighten to an allowlist: failure, cancelled, timeout. 2. on_failure suppresses the start comment marker, so ReconcileOrphaned could not detect hard-kills (SIGKILL/OOM) — the process death went completely silent. Teach ReconcileOrphaned to accept completionMode and synthesize an "Interrupted" comment when on_failure is configured and no marker is found. Plumb --fullsend-dir through reconcile-status so it can load the org config. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- action.yml | 3 + internal/cli/reconcilestatus.go | 33 +++++-- internal/statuscomment/statuscomment.go | 25 ++++- internal/statuscomment/statuscomment_test.go | 97 +++++++++++++++++--- 4 files changed, 132 insertions(+), 26 deletions(-) diff --git a/action.yml b/action.yml index 1742e9dc23..f4d60c3532 100644 --- a/action.yml +++ b/action.yml @@ -421,6 +421,7 @@ runs: RUN_URL: ${{ inputs.run-url }} JOB_STATUS: ${{ job.status }} PR_HEAD_SHA_INPUT: ${{ inputs.pr-head-sha }} + FULLSEND_DIR: ${{ inputs.fullsend-dir }} run: | set -euo pipefail # When the fullsend process is hard-killed (SIGKILL, OOM, segfault), @@ -451,8 +452,10 @@ runs: COMMIT_SHA="${GITHUB_SHA}" fi + FULLSEND_DIR="${FULLSEND_DIR:-${GITHUB_WORKSPACE}}" RECONCILE_FLAGS=(--repo "${STATUS_REPO}" --number "${STATUS_NUMBER}" --run-id "${RUN_ID}") RECONCILE_FLAGS+=(--mint-url "${MINT_URL}" --role "${AGENT}") + RECONCILE_FLAGS+=(--fullsend-dir "${FULLSEND_DIR}") if [[ -n "${RUN_URL}" ]]; then RECONCILE_FLAGS+=(--run-url "${RUN_URL}") fi diff --git a/internal/cli/reconcilestatus.go b/internal/cli/reconcilestatus.go index 2658cead54..c8f4daf441 100644 --- a/internal/cli/reconcilestatus.go +++ b/internal/cli/reconcilestatus.go @@ -7,6 +7,7 @@ import ( "github.com/spf13/cobra" + "github.com/fullsend-ai/fullsend/internal/config" "github.com/fullsend-ai/fullsend/internal/forge" gh "github.com/fullsend-ai/fullsend/internal/forge/github" gl "github.com/fullsend-ai/fullsend/internal/forge/gitlab" @@ -21,15 +22,16 @@ var reconcileNewForgeClient = func(token string) forge.Client { func newReconcileStatusCmd() *cobra.Command { var ( - repo string - number int - runID string - runURL string - sha string - reason string - mintURL string - role string - forgeFlag string + repo string + number int + runID string + runURL string + sha string + reason string + mintURL string + role string + forgeFlag string + fullsendDir string ) cmd := &cobra.Command{ @@ -82,7 +84,17 @@ finalized, this is a no-op.`, default: termReason = statuscomment.ReasonTerminated } - return statuscomment.ReconcileOrphaned(cmd.Context(), client, owner, repoName, number, runID, runURL, sha, termReason) + + completionMode := "" + if fullsendDir != "" { + if writer, err := config.LoadConfigWriter(fullsendDir, config.LoadOpts{MissingOK: true}); err == nil && writer != nil { + if ocr, ok := writer.(config.OrgConfigReader); ok && ocr.StatusNotifications() != nil { + completionMode = ocr.StatusNotifications().Comment.Completion + } + } + } + + return statuscomment.ReconcileOrphaned(cmd.Context(), client, owner, repoName, number, runID, runURL, sha, termReason, completionMode) }, } @@ -95,6 +107,7 @@ finalized, this is a no-op.`, cmd.Flags().StringVar(&mintURL, "mint-url", "", "mint service URL for on-demand token (default: $FULLSEND_MINT_URL)") cmd.Flags().StringVar(&role, "role", "", "agent role for minting (required with --mint-url)") cmd.Flags().StringVar(&forgeFlag, "forge", "", `forge platform (e.g. "github", "gitlab"); auto-detected from CI env vars when omitted`) + cmd.Flags().StringVar(&fullsendDir, "fullsend-dir", "", "path to fullsend config directory (used to detect completion mode for orphan synthesis)") _ = cmd.MarkFlagRequired("repo") _ = cmd.MarkFlagRequired("number") _ = cmd.MarkFlagRequired("run-id") diff --git a/internal/statuscomment/statuscomment.go b/internal/statuscomment/statuscomment.go index 904c1a51ce..d11824947b 100644 --- a/internal/statuscomment/statuscomment.go +++ b/internal/statuscomment/statuscomment.go @@ -128,7 +128,7 @@ func commentEnabled(val string) bool { func shouldPostCompletion(val, status string) bool { switch val { case "on_failure": - return status != "success" + return status == "failure" || status == "cancelled" || status == "timeout" default: return commentEnabled(val) } @@ -457,13 +457,19 @@ func statusEmoji(status string) string { // completion and interrupted comment bodies. If found in a non-terminal // state, it updates the comment to "Interrupted" and tags it as terminal. // +// completionMode is the configured comment.completion value ("enabled", +// "on_failure", or "disabled"). When "on_failure", no start comment marker +// is created — so the absence of a marker means the process may have been +// hard-killed before PostCompletion could run. In that case, a new +// "Interrupted" comment is synthesized so the failure is still surfaced. +// // This function is designed to be called from an out-of-process cleanup // mechanism (e.g., a GitHub Actions post-job step) that runs even when the // fullsend process is killed. It does not require a Notifier instance since // the process that created it is gone. // // 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) error { +func ReconcileOrphaned(ctx context.Context, client forge.Client, owner, repo string, number int, runID, runURL, sha string, reason TerminationReason, completionMode string) error { marker, err := buildMarker(runID) if err != nil { return fmt.Errorf("building marker: %w", err) @@ -492,8 +498,19 @@ func ReconcileOrphaned(ctx context.Context, client forge.Client, owner, repo str return nil } - // No matching comment found — either PostStart never ran, or the comment - // was already deleted. Both are fine. + // No matching comment found. When completion is "on_failure", the start + // comment is intentionally suppressed — so an absent marker doesn't mean + // "nothing happened." It may mean the process was hard-killed before + // PostCompletion could run. Synthesize an "Interrupted" comment so the + // failure is visible. See PR #5736. + if completionMode == "on_failure" { + endTime := now().UTC() + body := buildInterruptedBody(marker, runURL, sha, "", "", endTime, reason) + if _, err := client.CreateIssueComment(ctx, owner, repo, number, body); err != nil { + return fmt.Errorf("creating synthesized interrupted comment: %w", err) + } + } + return nil } diff --git a/internal/statuscomment/statuscomment_test.go b/internal/statuscomment/statuscomment_test.go index 727d1a9c6d..35c28f0bac 100644 --- a/internal/statuscomment/statuscomment_test.go +++ b/internal/statuscomment/statuscomment_test.go @@ -387,7 +387,7 @@ 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) + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "-->bad", "", "", ReasonTerminated, "") require.Error(t, err) assert.Contains(t, err.Error(), "invalid run ID") } @@ -570,7 +570,7 @@ func TestReconcileOrphaned_UpdatesStartedComment(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated) + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -599,7 +599,7 @@ func TestReconcileOrphaned_SkipsAlreadyFinished(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated) + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "") require.NoError(t, err) assert.Empty(t, fc.UpdatedComments, "should not update already-finished comment") @@ -609,11 +609,65 @@ func TestReconcileOrphaned_NoMatchingComment(t *testing.T) { fc := forge.NewFakeClient() // No comments at all. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated) + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "") require.NoError(t, err) assert.Empty(t, fc.UpdatedComments) } +func TestReconcileOrphaned_OnFailure_SynthesizesWhenNoMarker(t *testing.T) { + fc := forge.NewFakeClient() + 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") + require.NoError(t, err) + + comments := fc.IssueComments["org/repo/7"] + require.Len(t, comments, 1, "should synthesize an interrupted comment") + body := comments[0].Body + assert.Contains(t, body, "") + assert.Contains(t, body, "") + assert.Contains(t, body, "❌ Terminated") + assert.Contains(t, body, "Ended 2:00 PM UTC") + assert.Contains(t, body, "Commit: `abc1234`") + assert.Contains(t, body, "[View workflow run →](https://ci/run/99)") +} + +func TestReconcileOrphaned_OnFailure_NoSynthesisWhenMarkerExists(t *testing.T) { + fc := forge.NewFakeClient() + 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, + Body: "\n🤖 Code · Started 6:43 AM UTC", + Author: "fullsend-bot[bot]", + }, + } + + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "on_failure") + 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") +} + +func TestReconcileOrphaned_EnabledMode_NoSynthesisWhenNoMarker(t *testing.T) { + fc := forge.NewFakeClient() + + // No comments, default completion mode — should NOT synthesize. + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "") + require.NoError(t, err) + assert.Empty(t, fc.IssueComments, "should not synthesize for enabled mode") + assert.Empty(t, fc.UpdatedComments) +} + func TestReconcileOrphaned_DifferentRunID(t *testing.T) { fc := forge.NewFakeClient() fc.IssueComments = map[string][]forge.IssueComment{} @@ -627,7 +681,7 @@ func TestReconcileOrphaned_DifferentRunID(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated) + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "") require.NoError(t, err) assert.Empty(t, fc.UpdatedComments, "should not touch comment from different run") @@ -637,7 +691,7 @@ func TestReconcileOrphaned_ListError(t *testing.T) { fc := forge.NewFakeClient() fc.Errors["ListIssueComments"] = fmt.Errorf("api error") - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "", "", ReasonTerminated) + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "", "", ReasonTerminated, "") require.Error(t, err) assert.Contains(t, err.Error(), "listing comments") } @@ -655,7 +709,7 @@ func TestReconcileOrphaned_NoURLOrSHA(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "", "", ReasonTerminated) + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "", "", ReasonTerminated, "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -680,7 +734,7 @@ func TestReconcileOrphaned_SkipsAlreadyInterrupted(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated) + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "") require.NoError(t, err) assert.Empty(t, fc.UpdatedComments, "should not re-update already-interrupted comment") @@ -700,7 +754,7 @@ func TestReconcileOrphaned_UpdateError(t *testing.T) { fc.Errors["UpdateIssueComment"] = fmt.Errorf("api rate limited") - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated) + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "") require.Error(t, err) assert.Contains(t, err.Error(), "updating orphaned comment") } @@ -785,7 +839,7 @@ func TestReconcileOrphaned_CancelledReason(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonCancelled) + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonCancelled, "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -810,7 +864,7 @@ func TestReconcileOrphaned_StartTimeNotParseable(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated) + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -882,7 +936,7 @@ func TestReconcileOrphaned_UnknownReasonDefaultsToTerminated(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", TerminationReason("unknown-value")) + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", TerminationReason("unknown-value"), "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -1184,6 +1238,25 @@ func TestPostCompletion_OnFailure_NoStartComment_SuppressedOnSuccess(t *testing. assert.Empty(t, fc.DeletedComments) } +func TestPostCompletion_OnFailure_SuppressedOnSkipped(t *testing.T) { + fc := forge.NewFakeClient() + cfg := config.StatusNotificationConfig{ + Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "on_failure"}, + } + n := newTestNotifier(fc, cfg) + + err := n.PostStart(context.Background(), "Working") + require.NoError(t, err) + assert.Equal(t, 0, n.startCommentID, "start comment auto-suppressed") + + n.now = func() time.Time { return fixedTime().Add(time.Minute) } + err = n.PostCompletion(context.Background(), "Working", "skipped") + require.NoError(t, err) + + assert.Empty(t, fc.IssueComments, "skipped is not a failure — should not post with on_failure") + assert.Empty(t, fc.UpdatedComments) +} + func TestClientFactory_CompletionDisabled_DeleteError(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{ From 970d562dd8c65072448ccd3d8b4fae90f6996d85 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Thu, 30 Jul 2026 17:07:57 +0000 Subject: [PATCH 06/22] fix: prevent false interrupted comments on successful on_failure runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ReconcileOrphaned synthesized false "Interrupted" comments on every successful run with on_failure completion mode. The flow: PostStart suppressed (no marker) → agent succeeds → PostCompletion suppressed → reconcile finds no marker → creates false "Interrupted" comment. Pass job status through action.yml → CLI → ReconcileOrphaned and skip synthesis when the job succeeded — a missing marker then means the agent completed normally, not that it was hard-killed. Also: log warning on config load errors instead of swallowing silently, add --fullsend-dir and --job-status to CLI docs, mention timeout in on_failure docs. Addresses review feedback on #5736 --- action.yml | 1 + docs/guides/dev/cli-internals.md | 4 +- docs/guides/user/customizing-agents.md | 4 +- internal/cli/reconcilestatus.go | 9 ++++- internal/statuscomment/statuscomment.go | 14 +++++-- internal/statuscomment/statuscomment_test.go | 42 +++++++++++++------- 6 files changed, 51 insertions(+), 23 deletions(-) diff --git a/action.yml b/action.yml index f4d60c3532..d2426cb8c5 100644 --- a/action.yml +++ b/action.yml @@ -456,6 +456,7 @@ runs: RECONCILE_FLAGS=(--repo "${STATUS_REPO}" --number "${STATUS_NUMBER}" --run-id "${RUN_ID}") RECONCILE_FLAGS+=(--mint-url "${MINT_URL}" --role "${AGENT}") RECONCILE_FLAGS+=(--fullsend-dir "${FULLSEND_DIR}") + RECONCILE_FLAGS+=(--job-status "${JOB_STATUS}") if [[ -n "${RUN_URL}" ]]; then RECONCILE_FLAGS+=(--run-url "${RUN_URL}") fi diff --git a/docs/guides/dev/cli-internals.md b/docs/guides/dev/cli-internals.md index ddae5dcca7..31c996cc91 100644 --- a/docs/guides/dev/cli-internals.md +++ b/docs/guides/dev/cli-internals.md @@ -118,7 +118,9 @@ fullsend ├── --reason # Termination reason: terminated or cancelled (default: terminated) ├── --mint-url # Mint service URL for on-demand token (default: $FULLSEND_MINT_URL) ├── --role # Agent role for minting (required with --mint-url) - └── --forge # Forge platform (github, gitlab); auto-detected from CI env + ├── --forge # Forge platform (github, gitlab); auto-detected from CI env + ├── --fullsend-dir # Path to fullsend config directory (completion mode detection) + └── --job-status # Job outcome from CI runner (e.g. success, failure, cancelled) ``` ### Command Decomposition diff --git a/docs/guides/user/customizing-agents.md b/docs/guides/user/customizing-agents.md index 9debbbc5fd..e10bc3dc8b 100644 --- a/docs/guides/user/customizing-agents.md +++ b/docs/guides/user/customizing-agents.md @@ -555,10 +555,10 @@ When `status_notifications` is omitted entirely, both start and completion comme | Value | Behavior | |-------|----------| | `enabled` | Always post a completion comment (default) | -| `on_failure` | Post only when the agent fails or is cancelled; the start comment is automatically suppressed to avoid notification noise | +| `on_failure` | Post only when the agent fails, times out, or is cancelled; the start comment is automatically suppressed to avoid notification noise | | `disabled` | Never post a completion comment; silently remove the start comment | -`on_failure` is useful when you want to reduce notification noise — successful runs leave no trace, but failures still surface. When `completion` is set to `on_failure`, the start comment is automatically suppressed regardless of the `start` setting, because posting and then deleting a start comment would still trigger a GitHub notification pointing to a deleted comment. +`on_failure` is useful when you want to reduce notification noise — successful runs leave no trace, but failures and timeouts still surface. When `completion` is set to `on_failure`, the start comment is automatically suppressed regardless of the `start` setting, because posting and then deleting a start comment would still trigger a GitHub notification pointing to a deleted comment. ## Disabling Agents diff --git a/internal/cli/reconcilestatus.go b/internal/cli/reconcilestatus.go index c8f4daf441..bdc17fd8a7 100644 --- a/internal/cli/reconcilestatus.go +++ b/internal/cli/reconcilestatus.go @@ -32,6 +32,7 @@ func newReconcileStatusCmd() *cobra.Command { role string forgeFlag string fullsendDir string + jobStatus string ) cmd := &cobra.Command{ @@ -87,14 +88,17 @@ finalized, this is a no-op.`, completionMode := "" if fullsendDir != "" { - if writer, err := config.LoadConfigWriter(fullsendDir, config.LoadOpts{MissingOK: true}); err == nil && writer != nil { + writer, err := config.LoadConfigWriter(fullsendDir, config.LoadOpts{MissingOK: true}) + if err != nil { + fmt.Fprintf(os.Stderr, "warning: could not load config from %s: %v\n", fullsendDir, err) + } else if writer != nil { if ocr, ok := writer.(config.OrgConfigReader); ok && ocr.StatusNotifications() != nil { completionMode = ocr.StatusNotifications().Comment.Completion } } } - return statuscomment.ReconcileOrphaned(cmd.Context(), client, owner, repoName, number, runID, runURL, sha, termReason, completionMode) + return statuscomment.ReconcileOrphaned(cmd.Context(), client, owner, repoName, number, runID, runURL, sha, termReason, completionMode, jobStatus) }, } @@ -108,6 +112,7 @@ finalized, this is a no-op.`, cmd.Flags().StringVar(&role, "role", "", "agent role for minting (required with --mint-url)") cmd.Flags().StringVar(&forgeFlag, "forge", "", `forge platform (e.g. "github", "gitlab"); auto-detected from CI env vars when omitted`) cmd.Flags().StringVar(&fullsendDir, "fullsend-dir", "", "path to fullsend config directory (used to detect completion mode for orphan synthesis)") + cmd.Flags().StringVar(&jobStatus, "job-status", "", "job outcome from the CI runner (e.g. success, failure, cancelled)") _ = cmd.MarkFlagRequired("repo") _ = cmd.MarkFlagRequired("number") _ = cmd.MarkFlagRequired("run-id") diff --git a/internal/statuscomment/statuscomment.go b/internal/statuscomment/statuscomment.go index d11824947b..5d0179635a 100644 --- a/internal/statuscomment/statuscomment.go +++ b/internal/statuscomment/statuscomment.go @@ -463,13 +463,19 @@ func statusEmoji(status string) string { // hard-killed before PostCompletion could run. In that case, a new // "Interrupted" comment is synthesized so the failure is still surfaced. // +// jobStatus is the GitHub Actions job status (e.g., "success", "failure", +// "cancelled"). When completionMode is "on_failure" and jobStatus is +// "success", synthesis is skipped — the absence of a marker means the +// agent completed normally and suppressed its comment, not that it was +// hard-killed. +// // This function is designed to be called from an out-of-process cleanup // mechanism (e.g., a GitHub Actions post-job step) that runs even when the // fullsend process is killed. It does not require a Notifier instance since // the process that created it is gone. // // 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 string) error { +func ReconcileOrphaned(ctx context.Context, client forge.Client, owner, repo string, number int, runID, runURL, sha string, reason TerminationReason, completionMode, jobStatus string) error { marker, err := buildMarker(runID) if err != nil { return fmt.Errorf("building marker: %w", err) @@ -502,8 +508,10 @@ func ReconcileOrphaned(ctx context.Context, client forge.Client, owner, repo str // comment is intentionally suppressed — so an absent marker doesn't mean // "nothing happened." It may mean the process was hard-killed before // PostCompletion could run. Synthesize an "Interrupted" comment so the - // failure is visible. See PR #5736. - if completionMode == "on_failure" { + // failure is visible — but only when the job actually failed or was + // cancelled. A successful job with no marker means PostCompletion + // suppressed the comment as designed. See PR #5736. + if completionMode == "on_failure" && jobStatus != "success" { endTime := now().UTC() body := buildInterruptedBody(marker, runURL, sha, "", "", endTime, reason) if _, err := client.CreateIssueComment(ctx, owner, repo, number, body); err != nil { diff --git a/internal/statuscomment/statuscomment_test.go b/internal/statuscomment/statuscomment_test.go index 35c28f0bac..890d9817e5 100644 --- a/internal/statuscomment/statuscomment_test.go +++ b/internal/statuscomment/statuscomment_test.go @@ -387,7 +387,7 @@ 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, "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "-->bad", "", "", ReasonTerminated, "", "") require.Error(t, err) assert.Contains(t, err.Error(), "invalid run ID") } @@ -570,7 +570,7 @@ func TestReconcileOrphaned_UpdatesStartedComment(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -599,7 +599,7 @@ func TestReconcileOrphaned_SkipsAlreadyFinished(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "") require.NoError(t, err) assert.Empty(t, fc.UpdatedComments, "should not update already-finished comment") @@ -609,7 +609,7 @@ func TestReconcileOrphaned_NoMatchingComment(t *testing.T) { fc := forge.NewFakeClient() // No comments at all. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "") require.NoError(t, err) assert.Empty(t, fc.UpdatedComments) } @@ -620,7 +620,7 @@ func TestReconcileOrphaned_OnFailure_SynthesizesWhenNoMarker(t *testing.T) { // 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") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "on_failure", "failure") require.NoError(t, err) comments := fc.IssueComments["org/repo/7"] @@ -649,7 +649,7 @@ 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") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "on_failure", "failure") require.NoError(t, err) // Should update the existing comment, not create a new one. @@ -662,12 +662,24 @@ func TestReconcileOrphaned_EnabledMode_NoSynthesisWhenNoMarker(t *testing.T) { fc := forge.NewFakeClient() // No comments, default completion mode — should NOT synthesize. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "") require.NoError(t, err) assert.Empty(t, fc.IssueComments, "should not synthesize for enabled mode") assert.Empty(t, fc.UpdatedComments) } +func TestReconcileOrphaned_OnFailure_NoSynthesisWhenJobSucceeded(t *testing.T) { + fc := forge.NewFakeClient() + + // 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") + require.NoError(t, err) + assert.Empty(t, fc.IssueComments, "should not synthesize when job succeeded") + assert.Empty(t, fc.UpdatedComments) +} + func TestReconcileOrphaned_DifferentRunID(t *testing.T) { fc := forge.NewFakeClient() fc.IssueComments = map[string][]forge.IssueComment{} @@ -681,7 +693,7 @@ func TestReconcileOrphaned_DifferentRunID(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "") require.NoError(t, err) assert.Empty(t, fc.UpdatedComments, "should not touch comment from different run") @@ -691,7 +703,7 @@ func TestReconcileOrphaned_ListError(t *testing.T) { fc := forge.NewFakeClient() fc.Errors["ListIssueComments"] = fmt.Errorf("api error") - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "", "", ReasonTerminated, "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "", "", ReasonTerminated, "", "") require.Error(t, err) assert.Contains(t, err.Error(), "listing comments") } @@ -709,7 +721,7 @@ func TestReconcileOrphaned_NoURLOrSHA(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "", "", ReasonTerminated, "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "", "", ReasonTerminated, "", "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -734,7 +746,7 @@ func TestReconcileOrphaned_SkipsAlreadyInterrupted(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "") require.NoError(t, err) assert.Empty(t, fc.UpdatedComments, "should not re-update already-interrupted comment") @@ -754,7 +766,7 @@ func TestReconcileOrphaned_UpdateError(t *testing.T) { fc.Errors["UpdateIssueComment"] = fmt.Errorf("api rate limited") - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "") require.Error(t, err) assert.Contains(t, err.Error(), "updating orphaned comment") } @@ -839,7 +851,7 @@ func TestReconcileOrphaned_CancelledReason(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonCancelled, "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonCancelled, "", "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -864,7 +876,7 @@ func TestReconcileOrphaned_StartTimeNotParseable(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -936,7 +948,7 @@ func TestReconcileOrphaned_UnknownReasonDefaultsToTerminated(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", TerminationReason("unknown-value"), "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", TerminationReason("unknown-value"), "", "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) From 8a2f328d018316d6e25dbed56d357a2be66f6fa1 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Thu, 30 Jul 2026 13:41:31 -0400 Subject: [PATCH 07/22] fix: uppercase WARNING prefix to match codebase convention Signed-off-by: Ralph Bean Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/cli/reconcilestatus.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/cli/reconcilestatus.go b/internal/cli/reconcilestatus.go index bdc17fd8a7..aaaa0245df 100644 --- a/internal/cli/reconcilestatus.go +++ b/internal/cli/reconcilestatus.go @@ -90,7 +90,7 @@ finalized, this is a no-op.`, if fullsendDir != "" { writer, err := config.LoadConfigWriter(fullsendDir, config.LoadOpts{MissingOK: true}) if err != nil { - fmt.Fprintf(os.Stderr, "warning: could not load config from %s: %v\n", fullsendDir, err) + fmt.Fprintf(os.Stderr, "WARNING: could not load config from %s: %v\n", fullsendDir, err) } else if writer != nil { if ocr, ok := writer.(config.OrgConfigReader); ok && ocr.StatusNotifications() != nil { completionMode = ocr.StatusNotifications().Comment.Completion From 6c661469f4983377168fc87f21b374629cf56063 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Thu, 30 Jul 2026 14:13:22 -0400 Subject: [PATCH 08/22] fix(#3697): guard against empty jobStatus in ReconcileOrphaned When --job-status is omitted, jobStatus defaults to an empty string which satisfies != "success" and would trigger spurious synthesis of an "Interrupted" comment. Add an empty-string check so synthesis only fires when we actually know the job failed. Signed-off-by: Ralph Bean Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/statuscomment/statuscomment.go | 8 ++++---- internal/statuscomment/statuscomment_test.go | 11 +++++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/internal/statuscomment/statuscomment.go b/internal/statuscomment/statuscomment.go index 5d0179635a..fef172c358 100644 --- a/internal/statuscomment/statuscomment.go +++ b/internal/statuscomment/statuscomment.go @@ -465,9 +465,9 @@ func statusEmoji(status string) string { // // jobStatus is the GitHub Actions job status (e.g., "success", "failure", // "cancelled"). When completionMode is "on_failure" and jobStatus is -// "success", synthesis is skipped — the absence of a marker means the -// agent completed normally and suppressed its comment, not that it was -// hard-killed. +// "success" or empty, synthesis is skipped — "success" means the agent +// completed normally and suppressed its comment, and empty means the job +// outcome is unknown (e.g., --job-status was omitted). // // This function is designed to be called from an out-of-process cleanup // mechanism (e.g., a GitHub Actions post-job step) that runs even when the @@ -511,7 +511,7 @@ func ReconcileOrphaned(ctx context.Context, client forge.Client, owner, repo str // failure is visible — but only when the job actually failed or was // cancelled. A successful job with no marker means PostCompletion // suppressed the comment as designed. See PR #5736. - if completionMode == "on_failure" && jobStatus != "success" { + if completionMode == "on_failure" && jobStatus != "" && jobStatus != "success" { endTime := now().UTC() body := buildInterruptedBody(marker, runURL, sha, "", "", endTime, reason) if _, err := client.CreateIssueComment(ctx, owner, repo, number, body); err != nil { diff --git a/internal/statuscomment/statuscomment_test.go b/internal/statuscomment/statuscomment_test.go index 890d9817e5..d96a773687 100644 --- a/internal/statuscomment/statuscomment_test.go +++ b/internal/statuscomment/statuscomment_test.go @@ -680,6 +680,17 @@ func TestReconcileOrphaned_OnFailure_NoSynthesisWhenJobSucceeded(t *testing.T) { assert.Empty(t, fc.UpdatedComments) } +func TestReconcileOrphaned_OnFailure_NoSynthesisWhenJobStatusEmpty(t *testing.T) { + fc := forge.NewFakeClient() + + // 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", "") + require.NoError(t, err) + assert.Empty(t, fc.IssueComments, "should not synthesize when job status is unknown") + assert.Empty(t, fc.UpdatedComments) +} + func TestReconcileOrphaned_DifferentRunID(t *testing.T) { fc := forge.NewFakeClient() fc.IssueComments = map[string][]forge.IssueComment{} From 31e54c5bd77b8d2caa7df5b44263c66c675c8140 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Thu, 30 Jul 2026 14:40:38 -0400 Subject: [PATCH 09/22] fix: include fallback context in config loading warning When config loading fails in reconcile-status, the warning now mentions that the default completion mode will be used. Signed-off-by: Ralph Bean Assisted-by: Claude claude-opus-4-6 Signed-off-by: Ralph Bean --- internal/cli/reconcilestatus.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/cli/reconcilestatus.go b/internal/cli/reconcilestatus.go index aaaa0245df..68fc9f12f8 100644 --- a/internal/cli/reconcilestatus.go +++ b/internal/cli/reconcilestatus.go @@ -90,7 +90,7 @@ finalized, this is a no-op.`, if fullsendDir != "" { writer, err := config.LoadConfigWriter(fullsendDir, config.LoadOpts{MissingOK: true}) if err != nil { - fmt.Fprintf(os.Stderr, "WARNING: could not load config from %s: %v\n", fullsendDir, err) + fmt.Fprintf(os.Stderr, "WARNING: could not load config from %s: %v; using default completion mode\n", fullsendDir, err) } else if writer != nil { if ocr, ok := writer.(config.OrgConfigReader); ok && ocr.StatusNotifications() != nil { completionMode = ocr.StatusNotifications().Comment.Completion From 33b3d1adc327c313830e0f00322310a186e2199f Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Thu, 30 Jul 2026 15:24:17 -0400 Subject: [PATCH 10/22] test: add on_failure + timeout coverage for shouldPostCompletion Adds TestPostCompletion_OnFailure_PostsOnTimeout to exercise the timeout status under on_failure completion mode, closing a test gap flagged in review. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/statuscomment/statuscomment_test.go | 22 ++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/internal/statuscomment/statuscomment_test.go b/internal/statuscomment/statuscomment_test.go index d96a773687..d35409174b 100644 --- a/internal/statuscomment/statuscomment_test.go +++ b/internal/statuscomment/statuscomment_test.go @@ -1280,6 +1280,28 @@ func TestPostCompletion_OnFailure_SuppressedOnSkipped(t *testing.T) { assert.Empty(t, fc.UpdatedComments) } +func TestPostCompletion_OnFailure_PostsOnTimeout(t *testing.T) { + fc := forge.NewFakeClient() + cfg := config.StatusNotificationConfig{ + Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "on_failure"}, + } + n := newTestNotifier(fc, cfg) + + err := n.PostStart(context.Background(), "Working") + require.NoError(t, err) + assert.Equal(t, 0, n.startCommentID, "start comment auto-suppressed") + + n.now = func() time.Time { return fixedTime().Add(5 * time.Minute) } + err = n.PostCompletion(context.Background(), "Working", "timeout") + require.NoError(t, err) + + // No start comment to update — timeout 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 timeout") + assert.Contains(t, comments[0].Body, "⚠️ Timeout") +} + func TestClientFactory_CompletionDisabled_DeleteError(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{ From f92c3dc2cf690f0e5b207a242c3b2e03738707d7 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Thu, 30 Jul 2026 16:44:55 -0400 Subject: [PATCH 11/22] fix: remove unreachable "timeout" status from on_failure allowlist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No production code path ever assigns status="timeout" — run.go maps context.DeadlineExceeded to "cancelled" via ctx.Err(). Remove the dead branch from shouldPostCompletion, drop the test that exercised it directly with a synthetic value, and update the user-facing docs to match. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- docs/guides/user/customizing-agents.md | 4 ++-- internal/statuscomment/statuscomment.go | 2 +- internal/statuscomment/statuscomment_test.go | 22 -------------------- 3 files changed, 3 insertions(+), 25 deletions(-) diff --git a/docs/guides/user/customizing-agents.md b/docs/guides/user/customizing-agents.md index e10bc3dc8b..9debbbc5fd 100644 --- a/docs/guides/user/customizing-agents.md +++ b/docs/guides/user/customizing-agents.md @@ -555,10 +555,10 @@ When `status_notifications` is omitted entirely, both start and completion comme | Value | Behavior | |-------|----------| | `enabled` | Always post a completion comment (default) | -| `on_failure` | Post only when the agent fails, times out, or is cancelled; the start comment is automatically suppressed to avoid notification noise | +| `on_failure` | Post only when the agent fails or is cancelled; the start comment is automatically suppressed to avoid notification noise | | `disabled` | Never post a completion comment; silently remove the start comment | -`on_failure` is useful when you want to reduce notification noise — successful runs leave no trace, but failures and timeouts still surface. When `completion` is set to `on_failure`, the start comment is automatically suppressed regardless of the `start` setting, because posting and then deleting a start comment would still trigger a GitHub notification pointing to a deleted comment. +`on_failure` is useful when you want to reduce notification noise — successful runs leave no trace, but failures still surface. When `completion` is set to `on_failure`, the start comment is automatically suppressed regardless of the `start` setting, because posting and then deleting a start comment would still trigger a GitHub notification pointing to a deleted comment. ## Disabling Agents diff --git a/internal/statuscomment/statuscomment.go b/internal/statuscomment/statuscomment.go index fef172c358..ead19d1e8c 100644 --- a/internal/statuscomment/statuscomment.go +++ b/internal/statuscomment/statuscomment.go @@ -128,7 +128,7 @@ func commentEnabled(val string) bool { func shouldPostCompletion(val, status string) bool { switch val { case "on_failure": - return status == "failure" || status == "cancelled" || status == "timeout" + return status == "failure" || status == "cancelled" default: return commentEnabled(val) } diff --git a/internal/statuscomment/statuscomment_test.go b/internal/statuscomment/statuscomment_test.go index d35409174b..d96a773687 100644 --- a/internal/statuscomment/statuscomment_test.go +++ b/internal/statuscomment/statuscomment_test.go @@ -1280,28 +1280,6 @@ func TestPostCompletion_OnFailure_SuppressedOnSkipped(t *testing.T) { assert.Empty(t, fc.UpdatedComments) } -func TestPostCompletion_OnFailure_PostsOnTimeout(t *testing.T) { - fc := forge.NewFakeClient() - cfg := config.StatusNotificationConfig{ - Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "on_failure"}, - } - n := newTestNotifier(fc, cfg) - - err := n.PostStart(context.Background(), "Working") - require.NoError(t, err) - assert.Equal(t, 0, n.startCommentID, "start comment auto-suppressed") - - n.now = func() time.Time { return fixedTime().Add(5 * time.Minute) } - err = n.PostCompletion(context.Background(), "Working", "timeout") - require.NoError(t, err) - - // No start comment to update — timeout 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 timeout") - assert.Contains(t, comments[0].Body, "⚠️ Timeout") -} - func TestClientFactory_CompletionDisabled_DeleteError(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{ From 2a8c0c7ba1d01cf9ee6db4ed163d6109a96d8b09 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Thu, 30 Jul 2026 16:45:05 -0400 Subject: [PATCH 12/22] test: add CLI-level tests for --fullsend-dir config loading Extract reconcileOrphaned into a package-level func var (matching the existing pattern for reconcileMintToken and reconcileNewForgeClient) so CLI tests can stub it and assert the completionMode plumbing. Three new tests cover: - valid org config with on_failure: mode is passed through - malformed config.yaml: warning emitted, falls back to empty mode - missing config.yaml (MissingOK): falls back to empty mode Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/cli/reconcilestatus.go | 3 +- internal/cli/reconcilestatus_test.go | 119 +++++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 1 deletion(-) diff --git a/internal/cli/reconcilestatus.go b/internal/cli/reconcilestatus.go index 68fc9f12f8..8948d51112 100644 --- a/internal/cli/reconcilestatus.go +++ b/internal/cli/reconcilestatus.go @@ -19,6 +19,7 @@ var reconcileMintToken = mintclient.MintToken var reconcileNewForgeClient = func(token string) forge.Client { return gh.New(token) } +var reconcileOrphaned = statuscomment.ReconcileOrphaned func newReconcileStatusCmd() *cobra.Command { var ( @@ -98,7 +99,7 @@ finalized, this is a no-op.`, } } - return statuscomment.ReconcileOrphaned(cmd.Context(), client, owner, repoName, number, runID, runURL, sha, termReason, completionMode, jobStatus) + return reconcileOrphaned(cmd.Context(), client, owner, repoName, number, runID, runURL, sha, termReason, completionMode, jobStatus) }, } diff --git a/internal/cli/reconcilestatus_test.go b/internal/cli/reconcilestatus_test.go index 1548591454..90848e0e6c 100644 --- a/internal/cli/reconcilestatus_test.go +++ b/internal/cli/reconcilestatus_test.go @@ -4,6 +4,8 @@ import ( "context" "net/http" "net/http/httptest" + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -12,6 +14,7 @@ import ( "github.com/fullsend-ai/fullsend/internal/forge" gh "github.com/fullsend-ai/fullsend/internal/forge/github" "github.com/fullsend-ai/fullsend/internal/mintclient" + "github.com/fullsend-ai/fullsend/internal/statuscomment" ) // gitlabNoteServer returns an httptest.Server that handles GitLab note API calls @@ -312,3 +315,119 @@ func TestNewReconcileStatusCmd_GitLabNoMintRequired(t *testing.T) { err := cmd.Execute() require.NoError(t, err) } + +// stubReconcileVars replaces the three package-level func vars used by +// newReconcileStatusCmd and returns a teardown function. +func stubReconcileVars(t *testing.T, onReconcile func(completionMode, jobStatus string)) { + t.Helper() + origMint := reconcileMintToken + origForge := reconcileNewForgeClient + origReconcile := reconcileOrphaned + + reconcileMintToken = func(_ context.Context, _ mintclient.MintRequest) (*mintclient.MintResult, error) { + return &mintclient.MintResult{Token: "ghs_stub_token"}, nil + } + reconcileNewForgeClient = func(token string) forge.Client { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte("[]")) + })) + 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) error { + onReconcile(completionMode, jobStatus) + return nil + } + t.Cleanup(func() { + reconcileMintToken = origMint + reconcileNewForgeClient = origForge + reconcileOrphaned = origReconcile + }) +} + +func TestNewReconcileStatusCmd_FullsendDir_OnFailureConfig(t *testing.T) { + dir := t.TempDir() + err := os.WriteFile(filepath.Join(dir, "config.yaml"), []byte(`version: "1" +dispatch: + platform: github-actions +defaults: + status_notifications: + comment: + completion: on_failure +`), 0o644) + require.NoError(t, err) + + var gotMode, gotStatus string + stubReconcileVars(t, func(completionMode, jobStatus string) { + gotMode = completionMode + gotStatus = jobStatus + }) + t.Setenv("FULLSEND_MINT_URL", "") + + cmd := newReconcileStatusCmd() + cmd.SetArgs([]string{ + "--repo", "org/repo", + "--number", "7", + "--run-id", "run-1", + "--mint-url", "https://mint.example.com", + "--role", "review", + "--fullsend-dir", dir, + "--job-status", "failure", + }) + + err = cmd.Execute() + require.NoError(t, err) + assert.Equal(t, "on_failure", gotMode) + assert.Equal(t, "failure", gotStatus) +} + +func TestNewReconcileStatusCmd_FullsendDir_MalformedConfig(t *testing.T) { + dir := t.TempDir() + err := os.WriteFile(filepath.Join(dir, "config.yaml"), []byte(`{{{not valid yaml`), 0o644) + require.NoError(t, err) + + var gotMode string + stubReconcileVars(t, func(completionMode, _ string) { + gotMode = completionMode + }) + t.Setenv("FULLSEND_MINT_URL", "") + + cmd := newReconcileStatusCmd() + cmd.SetArgs([]string{ + "--repo", "org/repo", + "--number", "7", + "--run-id", "run-1", + "--mint-url", "https://mint.example.com", + "--role", "review", + "--fullsend-dir", dir, + }) + + err = cmd.Execute() + require.NoError(t, err) + assert.Equal(t, "", gotMode, "malformed config should fall back to empty completionMode") +} + +func TestNewReconcileStatusCmd_FullsendDir_MissingConfig(t *testing.T) { + dir := t.TempDir() // no config.yaml written + + var gotMode string + stubReconcileVars(t, func(completionMode, _ string) { + gotMode = completionMode + }) + t.Setenv("FULLSEND_MINT_URL", "") + + cmd := newReconcileStatusCmd() + cmd.SetArgs([]string{ + "--repo", "org/repo", + "--number", "7", + "--run-id", "run-1", + "--mint-url", "https://mint.example.com", + "--role", "review", + "--fullsend-dir", dir, + }) + + err := cmd.Execute() + require.NoError(t, err) + assert.Equal(t, "", gotMode, "missing config should fall back to empty completionMode") +} From 96dc517b05e5d449a018b2fc5f6c51711b84fac7 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Fri, 31 Jul 2026 10:20:14 -0400 Subject: [PATCH 13/22] fix: log diagnostic when reconcile-status config isn't org-shaped Addresses waynesun09's review on internal/cli/reconcilestatus.go: when --fullsend-dir is set but the loaded config doesn't satisfy OrgConfigReader (or StatusNotifications() is nil), completionMode silently stayed "" with no diagnostic. Now logs an INFO line so operators can distinguish "not an org config" from "org config loaded, on_failure just isn't configured" when debugging why Interrupted comments never appear. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/cli/reconcilestatus.go | 13 ++++++++++--- internal/cli/reconcilestatus_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/internal/cli/reconcilestatus.go b/internal/cli/reconcilestatus.go index 8948d51112..422acde0c6 100644 --- a/internal/cli/reconcilestatus.go +++ b/internal/cli/reconcilestatus.go @@ -90,11 +90,18 @@ finalized, this is a no-op.`, completionMode := "" if fullsendDir != "" { writer, err := config.LoadConfigWriter(fullsendDir, config.LoadOpts{MissingOK: true}) - if err != nil { + switch { + case err != nil: fmt.Fprintf(os.Stderr, "WARNING: could not load config from %s: %v; using default completion mode\n", fullsendDir, err) - } else if writer != nil { - if ocr, ok := writer.(config.OrgConfigReader); ok && ocr.StatusNotifications() != nil { + default: + ocr, ok := writer.(config.OrgConfigReader) + switch { + case ok && ocr.StatusNotifications() != nil: completionMode = ocr.StatusNotifications().Comment.Completion + case ok: + fmt.Fprintf(os.Stderr, "INFO: no status_notifications configured at %s; using default completion mode\n", fullsendDir) + default: + fmt.Fprintf(os.Stderr, "INFO: %s is not an org config (status_notifications is org-level only); using default completion mode\n", fullsendDir) } } } diff --git a/internal/cli/reconcilestatus_test.go b/internal/cli/reconcilestatus_test.go index 90848e0e6c..c08715a6a6 100644 --- a/internal/cli/reconcilestatus_test.go +++ b/internal/cli/reconcilestatus_test.go @@ -431,3 +431,27 @@ func TestNewReconcileStatusCmd_FullsendDir_MissingConfig(t *testing.T) { require.NoError(t, err) assert.Equal(t, "", gotMode, "missing config should fall back to empty completionMode") } + +func TestNewReconcileStatusCmd_FullsendDir_MissingConfig_LogsDiagnostic(t *testing.T) { + dir := t.TempDir() // no config.yaml written + + stubReconcileVars(t, func(_, _ string) {}) + t.Setenv("FULLSEND_MINT_URL", "") + + cmd := newReconcileStatusCmd() + cmd.SetArgs([]string{ + "--repo", "org/repo", + "--number", "7", + "--run-id", "run-1", + "--mint-url", "https://mint.example.com", + "--role", "review", + "--fullsend-dir", dir, + }) + + var err error + stderr := captureStderr(t, func() { + err = cmd.Execute() + }) + require.NoError(t, err) + assert.Contains(t, stderr, "status_notifications", "should log a diagnostic distinguishing 'not an org config' from a real load error") +} From e9d24889da55d50e049d372bc52860092dbdde64 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Fri, 31 Jul 2026 14:03:33 -0400 Subject: [PATCH 14/22] fix: run status-comment Finalize step after other always() steps job.status is read at the point the "Finalize orphaned status comment" step executes. If a later always() step (e.g. artifact upload) fails after the agent succeeded, job.status was already captured as success, so on_failure mode never synthesizes the interrupted comment and the run looks clean despite ultimately failing. Move Finalize to run last among the always() steps so job.status reflects the job's true final outcome. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- action.yml | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/action.yml b/action.yml index d2426cb8c5..8cdcb1debd 100644 --- a/action.yml +++ b/action.yml @@ -409,7 +409,16 @@ runs: "${STATUS_FLAGS[@]+"${STATUS_FLAGS[@]}"}" \ "${MINT_FLAGS[@]+"${MINT_FLAGS[@]}"}" + - name: Upload fullsend artifacts + if: always() && inputs.agent != '__install_only__' + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: fullsend-${{ inputs.agent }} + path: ${{ github.workspace }}/output + - name: Finalize orphaned status comment + # Runs last (after all other always() steps) so job.status reflects + # the job's true final outcome, not just the fullsend run outcome. if: always() && inputs.agent != '__install_only__' && inputs.status-repo != '' && inputs.status-number != '' && inputs.mint-url != '' shell: bash env: @@ -469,10 +478,3 @@ runs: fullsend reconcile-status "${RECONCILE_FLAGS[@]+"${RECONCILE_FLAGS[@]}"}" || { echo "::warning::Could not reconcile orphaned status comment" } - - - name: Upload fullsend artifacts - if: always() && inputs.agent != '__install_only__' - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 - with: - name: fullsend-${{ inputs.agent }} - path: ${{ github.workspace }}/output From db5c82bd668b610b4d34b5cb2e719347a6b53874 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Fri, 31 Jul 2026 14:03:41 -0400 Subject: [PATCH 15/22] fix: post on_failure completion comment for skipped runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit shouldPostCompletion treated a "skipped" status (set when a pre-script determines no work is needed) the same as success under on_failure mode, so skipped runs produced zero comments — no start (auto-suppressed) and no completion. That silently discards the skip reason the pre-script feature exists to surface. Treat "skipped" as a case that should post under on_failure, alongside failure and cancelled, and document it in the completion-modes table. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- docs/guides/user/customizing-agents.md | 2 +- internal/statuscomment/statuscomment.go | 2 +- internal/statuscomment/statuscomment_test.go | 5 ++--- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/docs/guides/user/customizing-agents.md b/docs/guides/user/customizing-agents.md index 9debbbc5fd..dba689bbfc 100644 --- a/docs/guides/user/customizing-agents.md +++ b/docs/guides/user/customizing-agents.md @@ -555,7 +555,7 @@ When `status_notifications` is omitted entirely, both start and completion comme | Value | Behavior | |-------|----------| | `enabled` | Always post a completion comment (default) | -| `on_failure` | Post only when the agent fails or is cancelled; the start comment is automatically suppressed to avoid notification noise | +| `on_failure` | Post when the agent fails, is cancelled, or is skipped by a pre-script; the start comment is automatically suppressed to avoid notification noise | | `disabled` | Never post a completion comment; silently remove the start comment | `on_failure` is useful when you want to reduce notification noise — successful runs leave no trace, but failures still surface. When `completion` is set to `on_failure`, the start comment is automatically suppressed regardless of the `start` setting, because posting and then deleting a start comment would still trigger a GitHub notification pointing to a deleted comment. diff --git a/internal/statuscomment/statuscomment.go b/internal/statuscomment/statuscomment.go index ead19d1e8c..95a4a97c8e 100644 --- a/internal/statuscomment/statuscomment.go +++ b/internal/statuscomment/statuscomment.go @@ -128,7 +128,7 @@ func commentEnabled(val string) bool { func shouldPostCompletion(val, status string) bool { switch val { case "on_failure": - return status == "failure" || status == "cancelled" + return status == "failure" || status == "cancelled" || status == "skipped" default: return commentEnabled(val) } diff --git a/internal/statuscomment/statuscomment_test.go b/internal/statuscomment/statuscomment_test.go index d96a773687..1175b5027d 100644 --- a/internal/statuscomment/statuscomment_test.go +++ b/internal/statuscomment/statuscomment_test.go @@ -1261,7 +1261,7 @@ func TestPostCompletion_OnFailure_NoStartComment_SuppressedOnSuccess(t *testing. assert.Empty(t, fc.DeletedComments) } -func TestPostCompletion_OnFailure_SuppressedOnSkipped(t *testing.T) { +func TestPostCompletion_OnFailure_PostsOnSkipped(t *testing.T) { fc := forge.NewFakeClient() cfg := config.StatusNotificationConfig{ Comment: config.CommentNotificationConfig{Start: "enabled", Completion: "on_failure"}, @@ -1276,8 +1276,7 @@ func TestPostCompletion_OnFailure_SuppressedOnSkipped(t *testing.T) { err = n.PostCompletion(context.Background(), "Working", "skipped") require.NoError(t, err) - assert.Empty(t, fc.IssueComments, "skipped is not a failure — should not post with on_failure") - assert.Empty(t, fc.UpdatedComments) + assert.Len(t, fc.IssueComments, 1, "skip reason is informative signal — should post even with on_failure") } func TestClientFactory_CompletionDisabled_DeleteError(t *testing.T) { From e73c3389eacea51ccc3ecbacc9109fef8e8960b0 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Thu, 6 Aug 2026 14:33:49 -0400 Subject: [PATCH 16/22] fix: synthesize interrupted comment on skip-post failure, include agent name Two findings from review round on PR #5736: - waynesun09 (HIGH): ReconcileOrphaned's synthesis guard treated jobStatus=="success" as proof nothing needs posting. But a skipped run whose PostCompletionWithDetail call itself fails also reports jobStatus=="success" (the post error is only logged, never surfaces as a job failure), so the skip reason was silently lost. Thread action.yml's outputs.skipped through as --was-skipped, and allow synthesis when wasSkipped is true regardless of jobStatus. - waynesun09 (MEDIUM): the synthesized "Interrupted" comment had no agent identity, so in a repo running multiple agents against the same issue/PR there was no way to tell which one failed. Thread the --role value through as an agent description used for the comment heading, matching the formatting already used for start/completion comments in run.go. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- action.yml | 4 + internal/cli/reconcilestatus.go | 6 +- internal/cli/reconcilestatus_test.go | 79 ++++++++++++++++++-- internal/statuscomment/statuscomment.go | 21 ++++-- internal/statuscomment/statuscomment_test.go | 75 ++++++++++++++----- 5 files changed, 154 insertions(+), 31 deletions(-) diff --git a/action.yml b/action.yml index 8cdcb1debd..fe1a5f826a 100644 --- a/action.yml +++ b/action.yml @@ -429,6 +429,7 @@ runs: RUN_ID: ${{ github.run_id }} RUN_URL: ${{ inputs.run-url }} JOB_STATUS: ${{ job.status }} + WAS_SKIPPED: ${{ steps.run.outputs.skipped }} PR_HEAD_SHA_INPUT: ${{ inputs.pr-head-sha }} FULLSEND_DIR: ${{ inputs.fullsend-dir }} run: | @@ -466,6 +467,9 @@ runs: RECONCILE_FLAGS+=(--mint-url "${MINT_URL}" --role "${AGENT}") RECONCILE_FLAGS+=(--fullsend-dir "${FULLSEND_DIR}") RECONCILE_FLAGS+=(--job-status "${JOB_STATUS}") + if [[ "${WAS_SKIPPED}" == "true" ]]; then + RECONCILE_FLAGS+=(--was-skipped) + fi if [[ -n "${RUN_URL}" ]]; then RECONCILE_FLAGS+=(--run-url "${RUN_URL}") fi diff --git a/internal/cli/reconcilestatus.go b/internal/cli/reconcilestatus.go index 422acde0c6..516a9fe9dd 100644 --- a/internal/cli/reconcilestatus.go +++ b/internal/cli/reconcilestatus.go @@ -34,6 +34,7 @@ func newReconcileStatusCmd() *cobra.Command { forgeFlag string fullsendDir string jobStatus string + wasSkipped bool ) cmd := &cobra.Command{ @@ -106,7 +107,9 @@ finalized, this is a no-op.`, } } - return reconcileOrphaned(cmd.Context(), client, owner, repoName, number, runID, runURL, sha, termReason, completionMode, jobStatus) + agentDescription := titleCase(strings.ReplaceAll(role, "-", " ")) + + return reconcileOrphaned(cmd.Context(), client, owner, repoName, number, runID, runURL, sha, termReason, completionMode, jobStatus, wasSkipped, agentDescription) }, } @@ -121,6 +124,7 @@ finalized, this is a no-op.`, cmd.Flags().StringVar(&forgeFlag, "forge", "", `forge platform (e.g. "github", "gitlab"); auto-detected from CI env vars when omitted`) cmd.Flags().StringVar(&fullsendDir, "fullsend-dir", "", "path to fullsend config directory (used to detect completion mode for orphan synthesis)") cmd.Flags().StringVar(&jobStatus, "job-status", "", "job outcome from the CI runner (e.g. success, failure, cancelled)") + cmd.Flags().BoolVar(&wasSkipped, "was-skipped", false, "whether the pre-script decided to skip the run (forces synthesis under on_failure even when --job-status is success)") _ = cmd.MarkFlagRequired("repo") _ = cmd.MarkFlagRequired("number") _ = cmd.MarkFlagRequired("run-id") diff --git a/internal/cli/reconcilestatus_test.go b/internal/cli/reconcilestatus_test.go index c08715a6a6..f521706f55 100644 --- a/internal/cli/reconcilestatus_test.go +++ b/internal/cli/reconcilestatus_test.go @@ -318,7 +318,7 @@ func TestNewReconcileStatusCmd_GitLabNoMintRequired(t *testing.T) { // stubReconcileVars replaces the three package-level func vars used by // newReconcileStatusCmd and returns a teardown function. -func stubReconcileVars(t *testing.T, onReconcile func(completionMode, jobStatus string)) { +func stubReconcileVars(t *testing.T, onReconcile func(completionMode, jobStatus string, wasSkipped bool, agentDescription string)) { t.Helper() origMint := reconcileMintToken origForge := reconcileNewForgeClient @@ -335,8 +335,8 @@ 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) error { - onReconcile(completionMode, jobStatus) + reconcileOrphaned = func(_ context.Context, _ forge.Client, _, _ string, _ int, _, _, _ string, _ statuscomment.TerminationReason, completionMode, jobStatus string, wasSkipped bool, agentDescription string) error { + onReconcile(completionMode, jobStatus, wasSkipped, agentDescription) return nil } t.Cleanup(func() { @@ -359,7 +359,7 @@ defaults: require.NoError(t, err) var gotMode, gotStatus string - stubReconcileVars(t, func(completionMode, jobStatus string) { + stubReconcileVars(t, func(completionMode, jobStatus string, _ bool, _ string) { gotMode = completionMode gotStatus = jobStatus }) @@ -388,7 +388,7 @@ func TestNewReconcileStatusCmd_FullsendDir_MalformedConfig(t *testing.T) { require.NoError(t, err) var gotMode string - stubReconcileVars(t, func(completionMode, _ string) { + stubReconcileVars(t, func(completionMode, _ string, _ bool, _ string) { gotMode = completionMode }) t.Setenv("FULLSEND_MINT_URL", "") @@ -412,7 +412,7 @@ func TestNewReconcileStatusCmd_FullsendDir_MissingConfig(t *testing.T) { dir := t.TempDir() // no config.yaml written var gotMode string - stubReconcileVars(t, func(completionMode, _ string) { + stubReconcileVars(t, func(completionMode, _ string, _ bool, _ string) { gotMode = completionMode }) t.Setenv("FULLSEND_MINT_URL", "") @@ -435,7 +435,7 @@ func TestNewReconcileStatusCmd_FullsendDir_MissingConfig(t *testing.T) { func TestNewReconcileStatusCmd_FullsendDir_MissingConfig_LogsDiagnostic(t *testing.T) { dir := t.TempDir() // no config.yaml written - stubReconcileVars(t, func(_, _ string) {}) + stubReconcileVars(t, func(_, _ string, _ bool, _ string) {}) t.Setenv("FULLSEND_MINT_URL", "") cmd := newReconcileStatusCmd() @@ -455,3 +455,68 @@ func TestNewReconcileStatusCmd_FullsendDir_MissingConfig_LogsDiagnostic(t *testi require.NoError(t, err) assert.Contains(t, stderr, "status_notifications", "should log a diagnostic distinguishing 'not an org config' from a real load error") } + +func TestNewReconcileStatusCmd_WasSkippedFlag_Threaded(t *testing.T) { + var gotSkipped bool + stubReconcileVars(t, func(_, _ string, wasSkipped bool, _ string) { + gotSkipped = wasSkipped + }) + t.Setenv("FULLSEND_MINT_URL", "") + + cmd := newReconcileStatusCmd() + cmd.SetArgs([]string{ + "--repo", "org/repo", + "--number", "7", + "--run-id", "run-1", + "--mint-url", "https://mint.example.com", + "--role", "review", + "--job-status", "success", + "--was-skipped", + }) + + err := cmd.Execute() + require.NoError(t, err) + assert.True(t, gotSkipped) +} + +func TestNewReconcileStatusCmd_WasSkippedFlag_DefaultsFalse(t *testing.T) { + var gotSkipped bool + stubReconcileVars(t, func(_, _ string, wasSkipped bool, _ string) { + gotSkipped = wasSkipped + }) + t.Setenv("FULLSEND_MINT_URL", "") + + cmd := newReconcileStatusCmd() + cmd.SetArgs([]string{ + "--repo", "org/repo", + "--number", "7", + "--run-id", "run-1", + "--mint-url", "https://mint.example.com", + "--role", "review", + }) + + err := cmd.Execute() + require.NoError(t, err) + assert.False(t, gotSkipped) +} + +func TestNewReconcileStatusCmd_AgentDescription_DerivedFromRole(t *testing.T) { + var gotDescription string + stubReconcileVars(t, func(_, _ string, _ bool, agentDescription string) { + gotDescription = agentDescription + }) + t.Setenv("FULLSEND_MINT_URL", "") + + cmd := newReconcileStatusCmd() + cmd.SetArgs([]string{ + "--repo", "org/repo", + "--number", "7", + "--run-id", "run-1", + "--mint-url", "https://mint.example.com", + "--role", "code-review", + }) + + err := cmd.Execute() + require.NoError(t, err) + assert.Equal(t, "Code Review", gotDescription) +} diff --git a/internal/statuscomment/statuscomment.go b/internal/statuscomment/statuscomment.go index 95a4a97c8e..6e8ae51665 100644 --- a/internal/statuscomment/statuscomment.go +++ b/internal/statuscomment/statuscomment.go @@ -467,7 +467,15 @@ func statusEmoji(status string) string { // "cancelled"). When completionMode is "on_failure" and jobStatus is // "success" or empty, synthesis is skipped — "success" means the agent // completed normally and suppressed its comment, and empty means the job -// outcome is unknown (e.g., --job-status was omitted). +// outcome is unknown (e.g., --job-status was omitted). wasSkipped overrides +// this: it's true when the pre-script itself decided to skip the run, which +// means jobStatus can be "success" even though PostCompletionWithDetail's +// skip-reason comment failed to post (its error is only logged, not +// propagated to the job's exit code). See PR #5736. +// +// agentDescription is used as the heading for a synthesized "Interrupted" +// comment (e.g. "Code" for the code agent), so operators can tell which +// agent failed when multiple agents run against the same issue/PR. // // This function is designed to be called from an out-of-process cleanup // mechanism (e.g., a GitHub Actions post-job step) that runs even when the @@ -475,7 +483,7 @@ func statusEmoji(status string) string { // the process that created it is gone. // // 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) error { +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 { marker, err := buildMarker(runID) if err != nil { return fmt.Errorf("building marker: %w", err) @@ -509,11 +517,12 @@ func ReconcileOrphaned(ctx context.Context, client forge.Client, owner, repo str // "nothing happened." It may mean the process was hard-killed before // PostCompletion could run. Synthesize an "Interrupted" comment so the // failure is visible — but only when the job actually failed or was - // cancelled. A successful job with no marker means PostCompletion - // suppressed the comment as designed. See PR #5736. - if completionMode == "on_failure" && jobStatus != "" && jobStatus != "success" { + // cancelled, or the run was skipped and its own skip-reason comment + // failed to post. A successful, non-skipped job with no marker means + // PostCompletion suppressed the comment as designed. See PR #5736. + if completionMode == "on_failure" && (wasSkipped || (jobStatus != "" && jobStatus != "success")) { endTime := now().UTC() - body := buildInterruptedBody(marker, runURL, sha, "", "", endTime, reason) + body := buildInterruptedBody(marker, runURL, sha, agentDescription, "", endTime, reason) if _, err := client.CreateIssueComment(ctx, owner, repo, number, 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 1175b5027d..2d85acb83c 100644 --- a/internal/statuscomment/statuscomment_test.go +++ b/internal/statuscomment/statuscomment_test.go @@ -387,7 +387,7 @@ 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, "", "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "-->bad", "", "", ReasonTerminated, "", "", false, "") require.Error(t, err) assert.Contains(t, err.Error(), "invalid run ID") } @@ -570,7 +570,7 @@ func TestReconcileOrphaned_UpdatesStartedComment(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -599,7 +599,7 @@ func TestReconcileOrphaned_SkipsAlreadyFinished(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "") + err := ReconcileOrphaned(context.Background(), fc, "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") @@ -609,7 +609,7 @@ func TestReconcileOrphaned_NoMatchingComment(t *testing.T) { fc := forge.NewFakeClient() // No comments at all. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") require.NoError(t, err) assert.Empty(t, fc.UpdatedComments) } @@ -620,7 +620,7 @@ func TestReconcileOrphaned_OnFailure_SynthesizesWhenNoMarker(t *testing.T) { // 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") + err := ReconcileOrphaned(context.Background(), fc, "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"] @@ -649,7 +649,7 @@ 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") + err := ReconcileOrphaned(context.Background(), fc, "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. @@ -662,7 +662,7 @@ func TestReconcileOrphaned_EnabledMode_NoSynthesisWhenNoMarker(t *testing.T) { fc := forge.NewFakeClient() // No comments, default completion mode — should NOT synthesize. - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "") + err := ReconcileOrphaned(context.Background(), fc, "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) @@ -674,7 +674,7 @@ func TestReconcileOrphaned_OnFailure_NoSynthesisWhenJobSucceeded(t *testing.T) { // 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") + err := ReconcileOrphaned(context.Background(), fc, "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) @@ -685,12 +685,53 @@ func TestReconcileOrphaned_OnFailure_NoSynthesisWhenJobStatusEmpty(t *testing.T) // 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", "") + err := ReconcileOrphaned(context.Background(), fc, "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) } +func TestReconcileOrphaned_OnFailure_SynthesizesWhenSkippedEvenIfJobSucceeded(t *testing.T) { + fc := forge.NewFakeClient() + 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, "") + require.NoError(t, err) + + comments := fc.IssueComments["org/repo/7"] + require.Len(t, comments, 1, "should synthesize an interrupted comment despite jobStatus==success") + assert.Contains(t, comments[0].Body, "❌ Terminated") +} + +func TestReconcileOrphaned_EnabledMode_NoSynthesisWhenSkippedButNotOnFailure(t *testing.T) { + fc := forge.NewFakeClient() + + // 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, "") + 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() + 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") + require.NoError(t, err) + + comments := fc.IssueComments["org/repo/7"] + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, "🤖 Triage · ❌ Terminated") +} + func TestReconcileOrphaned_DifferentRunID(t *testing.T) { fc := forge.NewFakeClient() fc.IssueComments = map[string][]forge.IssueComment{} @@ -704,7 +745,7 @@ func TestReconcileOrphaned_DifferentRunID(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "") + err := ReconcileOrphaned(context.Background(), fc, "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") @@ -714,7 +755,7 @@ func TestReconcileOrphaned_ListError(t *testing.T) { fc := forge.NewFakeClient() fc.Errors["ListIssueComments"] = fmt.Errorf("api error") - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "", "", ReasonTerminated, "", "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "", "", ReasonTerminated, "", "", false, "") require.Error(t, err) assert.Contains(t, err.Error(), "listing comments") } @@ -732,7 +773,7 @@ func TestReconcileOrphaned_NoURLOrSHA(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "", "", ReasonTerminated, "", "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "", "", ReasonTerminated, "", "", false, "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -757,7 +798,7 @@ func TestReconcileOrphaned_SkipsAlreadyInterrupted(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "") + err := ReconcileOrphaned(context.Background(), fc, "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") @@ -777,7 +818,7 @@ func TestReconcileOrphaned_UpdateError(t *testing.T) { fc.Errors["UpdateIssueComment"] = fmt.Errorf("api rate limited") - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") require.Error(t, err) assert.Contains(t, err.Error(), "updating orphaned comment") } @@ -862,7 +903,7 @@ func TestReconcileOrphaned_CancelledReason(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonCancelled, "", "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonCancelled, "", "", false, "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -887,7 +928,7 @@ func TestReconcileOrphaned_StartTimeNotParseable(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) @@ -959,7 +1000,7 @@ func TestReconcileOrphaned_UnknownReasonDefaultsToTerminated(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", TerminationReason("unknown-value"), "", "") + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", TerminationReason("unknown-value"), "", "", false, "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) From 7100f1c7e977117e535610157ecc3665c0a2cae8 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Mon, 10 Aug 2026 11:56:46 -0400 Subject: [PATCH 17/22] fix: update stale ReconcileOrphaned call missing new params TestParagraphBreak_BetweenStatusAndMetadata/interrupted_body was merged from main with the pre-#5736 4-argument ReconcileOrphaned signature, but this branch already extended the function with completionMode, jobStatus, wasSkipped, and agentDescription parameters. go vet failed the merge with "not enough arguments in call to ReconcileOrphaned". Pass the same zero-value arguments used by the other call sites in this file that don't exercise the on_failure synthesis path. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/statuscomment/statuscomment_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/statuscomment/statuscomment_test.go b/internal/statuscomment/statuscomment_test.go index 2d85acb83c..a38384e28f 100644 --- a/internal/statuscomment/statuscomment_test.go +++ b/internal/statuscomment/statuscomment_test.go @@ -1475,7 +1475,7 @@ func TestParagraphBreak_BetweenStatusAndMetadata(t *testing.T) { }, } - err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated) + err := ReconcileOrphaned(context.Background(), fc, "org", "repo", 7, "run-99", "https://ci/run/99", "abc1234def", ReasonTerminated, "", "", false, "") require.NoError(t, err) require.Len(t, fc.UpdatedComments, 1) From 83118873338f9c29c4610d9356eadb5ee9d780b2 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Mon, 10 Aug 2026 12:22:21 -0400 Subject: [PATCH 18/22] docs: add missing --was-skipped flag to reconcile-status CLI tree fullsend-ai-review pointed out that --was-skipped was added to the reconcile-status command in this PR but never landed in the CLI tree documentation, unlike --fullsend-dir and --job-status. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- docs/guides/dev/cli-internals.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/guides/dev/cli-internals.md b/docs/guides/dev/cli-internals.md index 31c996cc91..17f8cf66db 100644 --- a/docs/guides/dev/cli-internals.md +++ b/docs/guides/dev/cli-internals.md @@ -120,7 +120,8 @@ fullsend ├── --role # Agent role for minting (required with --mint-url) ├── --forge # Forge platform (github, gitlab); auto-detected from CI env ├── --fullsend-dir # Path to fullsend config directory (completion mode detection) - └── --job-status # Job outcome from CI runner (e.g. success, failure, cancelled) + ├── --job-status # Job outcome from CI runner (e.g. success, failure, cancelled) + └── --was-skipped # Pre-script decided to skip the run; forces synthesis under on_failure ``` ### Command Decomposition From b4fec8775e75880b7ad5fa6fb8f87a4bd511e59b Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Mon, 10 Aug 2026 12:45:57 -0400 Subject: [PATCH 19/22] fix: distinguish skip-comment-post-failure from hard termination Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/statuscomment/statuscomment.go | 28 +++++++++++++++++++- internal/statuscomment/statuscomment_test.go | 20 +++++++++++++- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/internal/statuscomment/statuscomment.go b/internal/statuscomment/statuscomment.go index 6e8ae51665..6a4eb98f9a 100644 --- a/internal/statuscomment/statuscomment.go +++ b/internal/statuscomment/statuscomment.go @@ -33,6 +33,15 @@ type TerminationReason string const ( ReasonTerminated TerminationReason = "terminated" ReasonCancelled TerminationReason = "cancelled" + + // ReasonSkipCommentFailed is used when the pre-script decided to skip + // the run and PostCompletionWithDetail's own skip-reason comment failed + // to post, but the job otherwise completed normally (jobStatus is + // "success" or unknown). This is semantically distinct from a hard + // kill or job cancellation — the agent ran to completion, only its own + // comment-post attempt failed — so it gets a dedicated label rather + // than falling through to ReasonTerminated. See PR #5736. + ReasonSkipCommentFailed TerminationReason = "skip_comment_failed" ) // now is overridable in tests to fix the current time for ReconcileOrphaned. @@ -522,7 +531,17 @@ func ReconcileOrphaned(ctx context.Context, client forge.Client, owner, repo str // PostCompletion suppressed the comment as designed. See PR #5736. if completionMode == "on_failure" && (wasSkipped || (jobStatus != "" && jobStatus != "success")) { endTime := now().UTC() - body := buildInterruptedBody(marker, runURL, sha, agentDescription, "", endTime, reason) + synthReason := reason + // wasSkipped with no other failure/cancellation signal means the + // agent ran fine — only its own skip-reason comment failed to + // post. That's not a hard kill or cancellation, so label it + // distinctly rather than defaulting to ReasonTerminated. If + // jobStatus does show failure/cancellation, the passed-in reason + // already reflects that real outcome, so leave it alone. + if wasSkipped && (jobStatus == "" || jobStatus == "success") { + synthReason = ReasonSkipCommentFailed + } + body := buildInterruptedBody(marker, runURL, sha, agentDescription, "", endTime, synthReason) if _, err := client.CreateIssueComment(ctx, owner, repo, number, body); err != nil { return fmt.Errorf("creating synthesized interrupted comment: %w", err) } @@ -580,6 +599,13 @@ func reasonLabel(reason TerminationReason, description string) (statusLabel, hea } else { heading = "Agent run cancelled" } + case ReasonSkipCommentFailed: + statusLabel = "⏭️ Skipped (comment failed to post)" + if description != "" { + heading = description + } else { + heading = "Agent run skipped" + } default: statusLabel = "❌ Terminated" if description != "" { diff --git a/internal/statuscomment/statuscomment_test.go b/internal/statuscomment/statuscomment_test.go index a38384e28f..d2822c8c72 100644 --- a/internal/statuscomment/statuscomment_test.go +++ b/internal/statuscomment/statuscomment_test.go @@ -704,7 +704,25 @@ func TestReconcileOrphaned_OnFailure_SynthesizesWhenSkippedEvenIfJobSucceeded(t comments := fc.IssueComments["org/repo/7"] require.Len(t, comments, 1, "should synthesize an interrupted comment despite jobStatus==success") - assert.Contains(t, comments[0].Body, "❌ Terminated") + assert.Contains(t, comments[0].Body, "⏭️ Skipped (comment failed to post)") + assert.NotContains(t, comments[0].Body, "❌ Terminated") +} + +func TestReconcileOrphaned_OnFailure_SkippedWithRealCancellationKeepsReason(t *testing.T) { + fc := forge.NewFakeClient() + 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, "") + require.NoError(t, err) + + comments := fc.IssueComments["org/repo/7"] + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, "⚠️ Cancelled") + assert.NotContains(t, comments[0].Body, "Skipped (comment failed to post)") } func TestReconcileOrphaned_EnabledMode_NoSynthesisWhenSkippedButNotOnFailure(t *testing.T) { From 46c5d58feae966c8eb776943df4f182cb304604c Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Mon, 10 Aug 2026 16:43:55 -0400 Subject: [PATCH 20/22] fix(reconcile-status): use ConfigWriter.StatusNotifications() directly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ConfigWriter already embeds StatusNotificationsReader via ConfigReader, so type-asserting to OrgConfigReader before calling StatusNotifications() was unnecessary — and wrong for per-repo installs, since perRepoConfig doesn't implement OrgConfigReader. That made the type assertion fail silently for per-repo configs, leaving completionMode empty even when completion: on_failure was set, so ReconcileOrphaned would never detect the mode and could miss synthesizing interrupted comments on failure. Also switches the leftover diagnostic to the "WARNING:" prefix used everywhere else in the codebase, instead of introducing a new "INFO:" one. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/cli/reconcilestatus.go | 17 ++++++------- internal/cli/reconcilestatus_test.go | 36 ++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/internal/cli/reconcilestatus.go b/internal/cli/reconcilestatus.go index 516a9fe9dd..5993c01070 100644 --- a/internal/cli/reconcilestatus.go +++ b/internal/cli/reconcilestatus.go @@ -95,14 +95,15 @@ finalized, this is a no-op.`, case err != nil: fmt.Fprintf(os.Stderr, "WARNING: could not load config from %s: %v; using default completion mode\n", fullsendDir, err) default: - ocr, ok := writer.(config.OrgConfigReader) - switch { - case ok && ocr.StatusNotifications() != nil: - completionMode = ocr.StatusNotifications().Comment.Completion - case ok: - fmt.Fprintf(os.Stderr, "INFO: no status_notifications configured at %s; using default completion mode\n", fullsendDir) - default: - fmt.Fprintf(os.Stderr, "INFO: %s is not an org config (status_notifications is org-level only); using default completion mode\n", fullsendDir) + // ConfigWriter embeds StatusNotificationsReader (via + // ConfigReader) directly, so this works for both org + // and per-repo configs — no need to type-assert to + // OrgConfigReader, which per-repo configs don't + // implement. + if sn := writer.StatusNotifications(); sn != nil { + completionMode = sn.Comment.Completion + } else { + fmt.Fprintf(os.Stderr, "WARNING: no status_notifications configured at %s; using default completion mode\n", fullsendDir) } } } diff --git a/internal/cli/reconcilestatus_test.go b/internal/cli/reconcilestatus_test.go index f521706f55..e2f88c9194 100644 --- a/internal/cli/reconcilestatus_test.go +++ b/internal/cli/reconcilestatus_test.go @@ -382,6 +382,42 @@ defaults: assert.Equal(t, "failure", gotStatus) } +func TestNewReconcileStatusCmd_FullsendDir_PerRepoConfig_OnFailure(t *testing.T) { + dir := t.TempDir() + // No "dispatch"/"repos"/"inference"/"defaults" key, so this is + // detected as a per-repo config, not an org config. + err := os.WriteFile(filepath.Join(dir, "config.yaml"), []byte(`version: "1" +roles: ["review"] +status_notifications: + comment: + completion: on_failure +`), 0o644) + require.NoError(t, err) + + var gotMode, gotStatus string + stubReconcileVars(t, func(completionMode, jobStatus string, _ bool, _ string) { + gotMode = completionMode + gotStatus = jobStatus + }) + t.Setenv("FULLSEND_MINT_URL", "") + + cmd := newReconcileStatusCmd() + cmd.SetArgs([]string{ + "--repo", "org/repo", + "--number", "7", + "--run-id", "run-1", + "--mint-url", "https://mint.example.com", + "--role", "review", + "--fullsend-dir", dir, + "--job-status", "failure", + }) + + err = cmd.Execute() + require.NoError(t, err) + assert.Equal(t, "on_failure", gotMode, "per-repo config should be able to configure completion mode") + assert.Equal(t, "failure", gotStatus) +} + func TestNewReconcileStatusCmd_FullsendDir_MalformedConfig(t *testing.T) { dir := t.TempDir() err := os.WriteFile(filepath.Join(dir, "config.yaml"), []byte(`{{{not valid yaml`), 0o644) From 91993421bf981ad3f7f7decc2033b043caae61fa Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Mon, 10 Aug 2026 17:04:47 -0400 Subject: [PATCH 21/22] fix: soften skip-comment label and synthesize orphan comments in default mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ReasonSkipCommentFailed's "comment failed to post" label asserted a specific cause ReconcileOrphaned can't actually verify — a missing completion comment under on_failure could equally mean the notifier never got set up. Reword it to be outcome-neutral. Also extend orphan synthesis to the default/"enabled" completion mode: previously it only fired for on_failure, leaving the #3635 blind spot open for the common case (agent crashes before posting anything at all). "disabled" mode remains an explicit opt-out and is unaffected. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/statuscomment/statuscomment.go | 88 ++++++++++++-------- internal/statuscomment/statuscomment_test.go | 71 +++++++++++++++- 2 files changed, 123 insertions(+), 36 deletions(-) diff --git a/internal/statuscomment/statuscomment.go b/internal/statuscomment/statuscomment.go index 6a4eb98f9a..3e654ba044 100644 --- a/internal/statuscomment/statuscomment.go +++ b/internal/statuscomment/statuscomment.go @@ -35,12 +35,16 @@ const ( ReasonCancelled TerminationReason = "cancelled" // ReasonSkipCommentFailed is used when the pre-script decided to skip - // the run and PostCompletionWithDetail's own skip-reason comment failed - // to post, but the job otherwise completed normally (jobStatus is - // "success" or unknown). This is semantically distinct from a hard - // kill or job cancellation — the agent ran to completion, only its own - // comment-post attempt failed — so it gets a dedicated label rather - // than falling through to ReasonTerminated. See PR #5736. + // the run but no completion comment ended up recorded for it, while the + // job otherwise completed normally (jobStatus is "success" or + // unknown). The missing comment could mean the notifier failed to set + // up (no post was ever attempted) or that PostCompletionWithDetail's + // own skip-reason comment failed to post — this function can't tell + // those apart, so the label stays outcome-neutral rather than + // asserting a specific cause. It's semantically distinct from a hard + // kill or job cancellation — the agent ran to completion — so it gets + // a dedicated label rather than falling through to ReasonTerminated. + // See PR #5736. ReasonSkipCommentFailed TerminationReason = "skip_comment_failed" ) @@ -467,20 +471,28 @@ func statusEmoji(status string) string { // state, it updates the comment to "Interrupted" and tags it as terminal. // // completionMode is the configured comment.completion value ("enabled", -// "on_failure", or "disabled"). When "on_failure", no start comment marker -// is created — so the absence of a marker means the process may have been -// hard-killed before PostCompletion could run. In that case, a new -// "Interrupted" comment is synthesized so the failure is still surfaced. +// "on_failure", or "disabled"). It changes what an absent marker means: +// +// - "on_failure": no start comment marker is ever created, so an absent +// marker doesn't by itself indicate a problem. It may mean the process +// was hard-killed before PostCompletion could run. See PR #5736. +// - "" or "enabled" (the default): a status comment should exist for +// every run that reached the harness, win or lose. An absent marker +// here means the process crashed before it could post anything at +// all (e.g. during environment validation) — a blind spot where +// maintainers can't tell "no review was triggered" from "review was +// attempted and failed silently." See #3635. +// - "disabled": an explicit opt-out of all status comments. An absent +// marker is never synthesized in this mode, regardless of outcome. // // jobStatus is the GitHub Actions job status (e.g., "success", "failure", -// "cancelled"). When completionMode is "on_failure" and jobStatus is -// "success" or empty, synthesis is skipped — "success" means the agent -// completed normally and suppressed its comment, and empty means the job -// outcome is unknown (e.g., --job-status was omitted). wasSkipped overrides -// this: it's true when the pre-script itself decided to skip the run, which -// means jobStatus can be "success" even though PostCompletionWithDetail's -// skip-reason comment failed to post (its error is only logged, not -// propagated to the job's exit code). See PR #5736. +// "cancelled"). Synthesis is skipped when jobStatus is "success" or +// empty — "success" means the run completed normally, and empty means the +// job outcome is unknown (e.g., --job-status was omitted). wasSkipped +// overrides this for "on_failure" mode only: it's true when the pre-script +// itself decided to skip the run, which means jobStatus can be "success" +// even though no completion comment ended up recorded for it (its error is +// only logged, not propagated to the job's exit code). See PR #5736. // // agentDescription is used as the heading for a synthesized "Interrupted" // comment (e.g. "Code" for the code agent), so operators can tell which @@ -521,26 +533,36 @@ func ReconcileOrphaned(ctx context.Context, client forge.Client, owner, repo str return nil } - // No matching comment found. When completion is "on_failure", the start - // comment is intentionally suppressed — so an absent marker doesn't mean - // "nothing happened." It may mean the process was hard-killed before - // PostCompletion could run. Synthesize an "Interrupted" comment so the - // failure is visible — but only when the job actually failed or was - // cancelled, or the run was skipped and its own skip-reason comment - // failed to post. A successful, non-skipped job with no marker means - // PostCompletion suppressed the comment as designed. See PR #5736. - if completionMode == "on_failure" && (wasSkipped || (jobStatus != "" && jobStatus != "success")) { - endTime := now().UTC() - synthReason := reason + // No matching comment found. Whether that's cause for synthesizing an + // "Interrupted" comment depends on completionMode — see the doc + // comment above for the three cases. "disabled" is excluded from both + // branches below: the user opted out of all status comments, so an + // absent marker is never treated as a problem there. + jobFailed := jobStatus != "" && jobStatus != "success" + shouldSynthesize := false + synthReason := reason + switch { + case completionMode == "on_failure" && (wasSkipped || jobFailed): + shouldSynthesize = true // wasSkipped with no other failure/cancellation signal means the - // agent ran fine — only its own skip-reason comment failed to - // post. That's not a hard kill or cancellation, so label it + // agent ran fine — no completion comment ended up recorded for + // it. That's not a hard kill or cancellation, so label it // distinctly rather than defaulting to ReasonTerminated. If // jobStatus does show failure/cancellation, the passed-in reason // already reflects that real outcome, so leave it alone. - if wasSkipped && (jobStatus == "" || jobStatus == "success") { + if wasSkipped && !jobFailed { synthReason = ReasonSkipCommentFailed } + case commentEnabled(completionMode) && jobFailed: + // Default/"enabled" completion mode: a marker should always exist + // for a run that reached the harness. Its absence alongside a + // failed or cancelled job means the process crashed before it + // could post anything at all. See #3635. + shouldSynthesize = true + } + + if shouldSynthesize { + endTime := now().UTC() body := buildInterruptedBody(marker, runURL, sha, agentDescription, "", endTime, synthReason) if _, err := client.CreateIssueComment(ctx, owner, repo, number, body); err != nil { return fmt.Errorf("creating synthesized interrupted comment: %w", err) @@ -600,7 +622,7 @@ func reasonLabel(reason TerminationReason, description string) (statusLabel, hea heading = "Agent run cancelled" } case ReasonSkipCommentFailed: - statusLabel = "⏭️ Skipped (comment failed to post)" + statusLabel = "⏭️ Skipped (no completion comment)" if description != "" { heading = description } else { diff --git a/internal/statuscomment/statuscomment_test.go b/internal/statuscomment/statuscomment_test.go index d2822c8c72..2cf1e792df 100644 --- a/internal/statuscomment/statuscomment_test.go +++ b/internal/statuscomment/statuscomment_test.go @@ -661,13 +661,72 @@ func TestReconcileOrphaned_OnFailure_NoSynthesisWhenMarkerExists(t *testing.T) { func TestReconcileOrphaned_EnabledMode_NoSynthesisWhenNoMarker(t *testing.T) { fc := forge.NewFakeClient() - // No comments, default completion mode — should NOT synthesize. + // 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, "") require.NoError(t, err) assert.Empty(t, fc.IssueComments, "should not synthesize for enabled mode") assert.Empty(t, fc.UpdatedComments) } +func TestReconcileOrphaned_EnabledMode_NoSynthesisWhenJobSucceeded(t *testing.T) { + fc := forge.NewFakeClient() + + // 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, "") + require.NoError(t, err) + assert.Empty(t, fc.IssueComments) + assert.Empty(t, fc.UpdatedComments) +} + +func TestReconcileOrphaned_EnabledMode_SynthesizesOnFailureWithNoMarker(t *testing.T) { + fc := forge.NewFakeClient() + 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") + require.NoError(t, err) + + comments := fc.IssueComments["org/repo/7"] + require.Len(t, comments, 1, "should synthesize an interrupted comment for a crash before any comment posted") + assert.Contains(t, comments[0].Body, "❌ Terminated") + assert.Contains(t, comments[0].Body, "Review") +} + +func TestReconcileOrphaned_EnabledMode_SynthesizesOnCancelledWithNoMarker(t *testing.T) { + fc := forge.NewFakeClient() + 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, "") + require.NoError(t, err) + + comments := fc.IssueComments["org/repo/7"] + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, "⚠️ Cancelled") +} + +func TestReconcileOrphaned_DisabledMode_NoSynthesisEvenOnFailure(t *testing.T) { + fc := forge.NewFakeClient() + + // 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, "") + require.NoError(t, err) + assert.Empty(t, fc.IssueComments, "disabled completion mode must never synthesize a comment") + assert.Empty(t, fc.UpdatedComments) +} + func TestReconcileOrphaned_OnFailure_NoSynthesisWhenJobSucceeded(t *testing.T) { fc := forge.NewFakeClient() @@ -704,7 +763,13 @@ func TestReconcileOrphaned_OnFailure_SynthesizesWhenSkippedEvenIfJobSucceeded(t comments := fc.IssueComments["org/repo/7"] require.Len(t, comments, 1, "should synthesize an interrupted comment despite jobStatus==success") - assert.Contains(t, comments[0].Body, "⏭️ Skipped (comment failed to post)") + // 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") } @@ -722,7 +787,7 @@ func TestReconcileOrphaned_OnFailure_SkippedWithRealCancellationKeepsReason(t *t comments := fc.IssueComments["org/repo/7"] require.Len(t, comments, 1) assert.Contains(t, comments[0].Body, "⚠️ Cancelled") - assert.NotContains(t, comments[0].Body, "Skipped (comment failed to post)") + assert.NotContains(t, comments[0].Body, "Skipped (no completion comment)") } func TestReconcileOrphaned_EnabledMode_NoSynthesisWhenSkippedButNotOnFailure(t *testing.T) { From fa6d15ad7d9d5b7689eddb7b5b10db94c8b0b773 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 11 Aug 2026 10:27:54 -0400 Subject: [PATCH 22/22] docs: document enabled-mode orphan comment synthesis The final commit on this branch (91993421) extended ReconcileOrphaned's synthesis logic to the default "enabled" completion mode, not just on_failure, but the user-facing docs weren't updated to disclose it. Add a line to the Completion modes section explaining the behavior. Reported-by: waynesun09 Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- docs/guides/user/customizing-agents.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/guides/user/customizing-agents.md b/docs/guides/user/customizing-agents.md index dba689bbfc..674424d74c 100644 --- a/docs/guides/user/customizing-agents.md +++ b/docs/guides/user/customizing-agents.md @@ -560,6 +560,8 @@ When `status_notifications` is omitted entirely, both start and completion comme `on_failure` is useful when you want to reduce notification noise — successful runs leave no trace, but failures still surface. When `completion` is set to `on_failure`, the start comment is automatically suppressed regardless of the `start` setting, because posting and then deleting a start comment would still trigger a GitHub notification pointing to a deleted comment. +In `enabled` mode (the default), a hard crash or cancellation that happens before the agent could post anything at all is also surfaced after the fact: a post-job cleanup step synthesizes an "Interrupted" comment so the run doesn't silently vanish. + ## Disabling Agents To disable an agent (including built-in scaffold agents) without removing