feat(#3697): add on_failure mode for comment.completion status notifications - #5736
feat(#3697): add on_failure mode for comment.completion status notifications#5736ralphbean wants to merge 17 commits into
Conversation
ralphbean
added
the
fullsend-fix
|
🤖 Review · |
Site previewPreview: https://a589e5f5-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 5:47 PM UTC · Completed 6:01 PM UTC |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
Next steps:
Previous run (3)ReviewFindingsMedium
Low
Next steps:
Previous run (4)ReviewFindingsLow
Previous run (5)ReviewFindingsLow
Previous run (6)ReviewFindingsLow
Previous run (7)ReviewFindingsMedium
Low
Previous run (8)ReviewFindingsMedium
Low
Previous run (9)ReviewFindingsMedium
Low
Previous run (10)ReviewFindingsMedium
Low
Previous run (11)ReviewFindingsCritical
Medium
Low
Next steps:
Previous run (12)ReviewFindingsMedium
Low
Previous run (13)ReviewFindingsMedium
Low
Previous run (14)ReviewFindingsLow
Previous run (15)ReviewFindingsLow
Labels: PR adds a new user-facing config option (on_failure mode) with documentation updates |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
PR Summary by QodoAdd on_failure mode for completion status comments
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
|
🤖 Finished Review · ❌ Failure · Started 6:03 PM UTC · Completed 6:04 PM UTC |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad findings (medium+ severity)
Posting the medium+ findings from a 4-agent review pass (claude-coder, claude-researcher, grok-review-agent, gemini-code-review). Two additional medium findings couldn't be attached inline since they're in files outside this PR's diff:
[MEDIUM] operations.md still documents the old enabled/disabled-only completion values
docs/guides/getting-started/operations.md:150-162 still shows completion: enabled # "enabled" (default) | "disabled", unchanged by this PR — no mention of on_failure. Meanwhile running-agents-locally.md's cross-reference was repointed to the new customizing-agents.md#status-notifications section, leaving two docs pages disagreeing about valid values for the same key. Suggest replacing operations.md's section with a pointer to the new canonical location instead of maintaining two copies.
[MEDIUM] reconcile-status/ReconcileOrphaned ignores status_notifications entirely
internal/cli/reconcilestatus.go and internal/statuscomment/statuscomment.go's ReconcileOrphaned take no config input and always finalize an orphaned marker to "Interrupted" regardless of completion's configured value. This is pre-existing behavior, but this PR turns "silent on success" into a documented guarantee without addressing the narrow race where a process succeeds but is hard-killed before its deferred PostCompletion runs — the reconciler would still surface a false "Interrupted" notice under on_failure. Worth a doc note or a test capturing this known interaction.
f832f7a to
1999cd4
Compare
|
🤖 Review · |
|
🤖 Finished Review · ✅ Success · Started 8:50 PM UTC · Completed 9:04 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
20b4437 to
6131423
Compare
|
🤖 Finished Review · ✅ Success · Started 9:38 PM UTC · Completed 9:55 PM UTC |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
[HIGH] Required behaviour CI check is red at current HEAD — PR is currently BLOCKED
(Noting this in the review body rather than as an inline comment since the referenced test file, e2e/behaviour/features/dispatch/url-dispatch.feature:26, is not part of this PR's diff.)
The behaviour required check is failing on the PR's current HEAD (6131423) and mergeStateStatus is BLOCKED. This isn't a stale/cached result: commit 6131423 ("fix(#3697): auto-suppress start comment when completion is on_failure") triggered a fresh CI run (run 30493021917, job 90715292701) that ran to completion and failed again — this time with 4 scenarios failing (pr-ping, fork-pr-sync, url-ping, enabled-ping, all failing with a generic "did not complete successfully" error, clustered within a ~26s window around the ~13–14 minute mark of the run) versus only 1 scenario failing on the prior commit's run (1999cd47, run 30489904095).
The changing failure count/set across consecutive runs of the same suite is a strong flake/infrastructure signature (e.g. rate-limiting, a stuck dispatch queue, or the suite's own time-boxed guard pending the harness CEL cutover) rather than a deterministic regression — this PR's diff (docs, internal/config/config.go, internal/statuscomment/statuscomment.go + tests) has zero file overlap with e2e/behaviour or dispatch/harness-resolution code in either commit, which reinforces that this is very unlikely to be caused by this change.
Regardless of root cause: this is a currently-red required check on a PR that GitHub reports as BLOCKED, and it hasn't been raised elsewhere in this PR's comment/review history yet.
Suggestion: Don't merge on a red required check. Re-run behaviour once more (or loop in CI/infra to rule out GitHub API rate-limiting or runner contention) — given the failing scenario set changed between runs and now spans 4 unrelated harness scenarios simultaneously, this looks like shared test-infrastructure flakiness rather than something to fix in this PR's code, but it needs an explicit green run (or a documented infra ticket) before merging.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
ready-for-merge
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…cations 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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…e 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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
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 <rbean@redhat.com> Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
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 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
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
Signed-off-by: Ralph Bean <rbean@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
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 <rbean@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
When config loading fails in reconcile-status, the warning now mentions that the default completion mode will be used. Signed-off-by: Ralph Bean <rbean@redhat.com> Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
efad69d to
d6dcdc9
Compare
|
🤖 Finished Review · ✅ Success · Started 9:45 PM UTC · Completed 10:06 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings (medium severity)
[MEDIUM] on_failure crash-visibility guarantee is GitHub-only; GitLab CI scaffold has no equivalent reconciliation step
internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml (not modified by this PR, so no inline diff line is available)
The on_failure crash-visibility guarantee (ReconcileOrphaned synthesizing an "Interrupted" comment when a hard-killed process leaves no marker) is wired up only in action.yml's "Finalize orphaned status comment" step, which calls fullsend reconcile-status --fullsend-dir ... --job-status .... There's no equivalent reconcile-status (or reconcile) invocation anywhere under internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/. The GitLab scaffold job runs fullsend run directly under set -euo pipefail with no after_script/cleanup stage.
Since on_failure suppresses the start-comment marker entirely (statuscomment.go's PostStart: commentEnabled(n.cfg.Comment.Start) && n.cfg.Comment.Completion != "on_failure"), a hard-killed fullsend process under completion: on_failure on GitLab CI produces zero visible trace of the failure — worse than the pre-existing gap for other completion modes (which at least leave a stale "Started" comment). docs/guides/user/customizing-agents.md documents on_failure ("successful runs leave no trace, but failures still surface") with no platform caveat about this GitHub-only synthesis.
Suggestion: either wire an equivalent reconciliation call into the GitLab scaffold (e.g., an after_script/on-failure stage invoking fullsend reconcile-status --fullsend-dir .fullsend --job-status "$CI_JOB_STATUS"), or explicitly document in customizing-agents.md that on_failure's hard-kill visibility guarantee currently only applies to the GitHub Actions composite action.
…nt 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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
| // 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")) { |
There was a problem hiding this comment.
[MEDIUM] Synthesized comment mislabels a successful-but-skipped run as "Terminated"
When wasSkipped is true and no marker comment is found (the pre-script decided to skip and PostCompletionWithDetail's own skip-reason comment failed to post — its error is only logged, not propagated to the job's exit code, per the code comment at line 512), ReconcileOrphaned synthesizes a comment via buildInterruptedBody(..., reason). reason defaults to ReasonTerminated (internal/cli/reconcilestatus.go's --reason flag defaults to "terminated", and action.yml only passes --reason cancelled when JOB_STATUS=="cancelled" — see action.yml lines 479-480). reasonLabel() then renders "❌ Terminated" for any non-cancelled reason.
So a run that completed fully successfully, where only the skip-reason comment itself failed to post, will surface an "Interrupted ... Terminated" comment on the issue/PR — indistinguishable from an actual hard-kill/crash. This is confirmed by the PR's own new test, TestReconcileOrphaned_OnFailure_SynthesizesWhenSkippedEvenIfJobSucceeded (statuscomment_test.go:694-708), which explicitly asserts assert.Contains(comments[0].Body, "❌ Terminated") for this exact skip-success scenario.
This wasSkipped synthesis path was only just introduced in the latest commit (ce5879e), and this mislabeling hasn't been raised yet.
Suggestion: Give the wasSkipped-but-jobStatus-success synthesis branch its own reason/label (e.g. a new TerminationReason like ReasonSkipCommentFailed rendering "
|
🤖 Finished Review · ✅ Success · Started 2:34 PM UTC · Completed 2:54 PM UTC Commit: |
| RUN_ID: ${{ github.run_id }} | ||
| RUN_URL: ${{ inputs.run-url }} | ||
| JOB_STATUS: ${{ job.status }} | ||
| WAS_SKIPPED: ${{ steps.run.outputs.skipped }} |
There was a problem hiding this comment.
[medium] logic-error
The reconcile step passes ${{ job.status }} as JOB_STATUS, but job.status reflects the cumulative outcome of ALL prior steps in the job. This PR moves the Upload fullsend artifacts step before the reconcile step (both use if: always()), so if the upload fails after a successful fullsend run, job.status will be 'failure' and ReconcileOrphaned will synthesize a spurious 'Interrupted' comment — falsely claiming the agent was terminated when it completed normally.
Suggested fix: Pass ${{ steps.run.outcome }} instead of ${{ job.status }} for the JOB_STATUS env var.
| // 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() | ||
|
|
There was a problem hiding this comment.
[low] edge-case
PostStart suppresses the start comment when completion is on_failure by checking n.cfg.Comment.Completion != 'on_failure'. A user who sets start: enabled and completion: on_failure will not see a start comment, which may be surprising. The behavior is deliberate and documented: posting then deleting a start comment on success still triggers a GitHub notification pointing to a deleted comment, which defeats the noise-reduction purpose.
| // 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 |
There was a problem hiding this comment.
[low] edge-case
The synthesis condition completionMode == 'on_failure' && (wasSkipped || (jobStatus != '' && jobStatus != 'success')) accepts any non-empty, non-'success' jobStatus value. Any unexpected value (e.g., a typo in the --job-status flag) would trigger synthesis. The risk is low since the flag value is controlled by action.yml, not user input.
| // 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. | ||
| // |
There was a problem hiding this comment.
[low] api-shape
ReconcileOrphaned now has 13 positional parameters (up from 9), adding completionMode, jobStatus, wasSkipped, and agentDescription. The codebase uses options struct patterns elsewhere (e.g., config.LoadOpts). Pre-existing concern worsened by four parameters.
| └── --forge <platform> # Forge platform (github, gitlab); auto-detected from CI env | ||
| ├── --forge <platform> # Forge platform (github, gitlab); auto-detected from CI env | ||
| ├── --fullsend-dir <path> # Path to fullsend config directory (completion mode detection) | ||
| └── --job-status <string> # Job outcome from CI runner (e.g. success, failure, cancelled) |
There was a problem hiding this comment.
[low] missing-doc
The --was-skipped flag was added to the reconcile-status command but was not added to the CLI tree documentation. The PR added --fullsend-dir and --job-status to the CLI tree but omitted --was-skipped.
2 participants
Summary
on_failureas a valid value forstatus_notifications.comment.completionon_failure, the start comment is silently cleaned up (deleted)on_failureforcomment.start(no outcome to evaluate yet)Phase 1 of #3697 — comment-only changes. Reaction support is a follow-up.
Test plan
on_failurefor completion, rejects for startPostCompletionwithon_failuresuppresses on success, fires on failure/cancelledfullsend run triagewithon_failureconfig on a test issue🤖 Generated with Claude Code