Skip to content

feat: add failOnError option to abort pipeline on notification failures - #54

Open
adamrtalbot wants to merge 2 commits into
mainfrom
claude/issue-45-20260220-1321
Open

feat: add failOnError option to abort pipeline on notification failures#54
adamrtalbot wants to merge 2 commits into
mainfrom
claude/issue-45-20260220-1321

Conversation

@adamrtalbot

Copy link
Copy Markdown
Collaborator

Implements the failOnError configuration option requested in #45.

When failOnError = true, any Slack notification failure will throw a RuntimeException and abort the pipeline. When false (default), errors are logged as warnings and the pipeline continues.

Closes #45

Generated with Claude Code

Add a `failOnError` configuration option (default: false) that causes
the pipeline to fail when Slack notifications cannot be delivered.

When `failOnError = true`, any notification failure (sending messages,
uploading files, adding reactions, or progress updates) will throw a
RuntimeException, aborting the pipeline. When false (default), errors
are logged as warnings and the pipeline continues.

Senders (WebhookSlackSender, BotSlackSender) now throw exceptions on
failure instead of silently swallowing errors. SlackObserver wraps all
notification calls and delegates error handling to the new
`handleNotificationError` helper based on the failOnError config.

Usage:
```nextflow
slack {
    failOnError = true  // default: false
}
```

Closes #45

Co-authored-by: Adam Talbot <adamrtalbot@users.noreply.github.com>
@claude

claude Bot commented Feb 20, 2026

Copy link
Copy Markdown

Review: feat: add failOnError option

The implementation is clean — the handleNotificationError helper centralizes the logic well, and the exception-propagation refactor in the senders is solid.

Issues

Documentation missing
failOnError is not documented in docs/reference/api.md (the config reference table) or docs/usage/guide.md. Every other config option like validateOnStartup is listed there — this one should be too.

No example config
example/configs/ has configs for each notable feature but none for failOnError. Even a one-liner in an existing config (e.g. 01-minimal.config) would help users discover it.

Missing tests in SlackExtensionTest
The slackMessage(), slackMessage(Map), and slackFileUpload() functions in SlackExtension all have the failOnError branch, but SlackExtensionTest.groovy has zero coverage of that path. The observer tests cover the flag well — the extension needs the same.

Silent behavior change at default settings
With failOnError = false (the default), reaction failures previously logged at DEBUG. They now log at WARN (via handleNotificationError). This affects existing users who haven't opted in to failOnError and should be called out in the changelog/docs.

Document failOnError, add extension tests, preserve DEBUG logging for
reaction failures when disabled, and align sender tests with throw behavior.

Generated by Codex

Co-authored-by: Cursor <cursoragent@cursor.com>
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Code Review

This PR adds a failOnError config option that converts all Slack notification failures from silent log-and-continue into pipeline-aborting RuntimeExceptions. The refactor is clean overall: senders now throw, handleNotificationError centralizes the log/throw decision in the observer, and tests are updated to match. A few issues need attention before merging.


Findings (most severe first)

WebhookSlackSender.groovy:101failOnError=true is silently ignored for webhook file uploads

WebhookSlackSender.uploadFile only logs a warning and returns without throwing:

void uploadFile(Path filePath, Map options) {
    log.warn "Slack plugin: File upload is not supported with webhooks..."
}

Because it never throws, uploadConfiguredFiles in the observer completes without entering the catch block, so handleNotificationError is never called and the pipeline never aborts — even when failOnError = true. The docs promise this setting aborts on "messages, file uploads, reactions, or extension calls", but this case is silently violated. Fix: throw an UnsupportedOperationException from WebhookSlackSender.uploadFile so the observer's catch path picks it up normally.


SlackExtension.groovy:88,146,232log.error before conditional rethrow is inconsistent with handleNotificationError

All three extension catch blocks unconditionally log at log.error before conditionally rethrowing:

} catch (Exception e) {
    def msg = "Slack plugin: Error sending message: ${e.message}"
    log.error msg, e          // always ERROR, even when failOnError=false
    if (observer.config?.failOnError) {
        throw new RuntimeException(msg, e)
    }
}

handleNotificationError intentionally uses log.warn before throwing (reserving log.error for the framework's own exception reporting). The extension does the opposite: logs ERROR first, then throws — likely producing two ERROR-level entries per failure when failOnError=true. When failOnError=false it logs a recoverable failure at ERROR instead of WARN. Change to log.warn to match handleNotificationError.


BotSlackSender.groovy:124,132 — Dead null/false checks after methods that now always throw

getUploadUrl returns a non-null Map or throws — never null. uploadFileContent returns true or throws — never false. Both guards are unreachable:

def uploadInfo = getUploadUrl(filename, fileSize)
if (!uploadInfo) {           // dead: getUploadUrl never returns null
    throw new RuntimeException(...)
}
...
if (!uploadFileContent(uploadUrl, filePath)) {   // dead: always returns true or throws
    throw new RuntimeException(...)
}

Remove both dead branches.


WebhookSlackSender.groovy:24-27 — Stale class docstring

The class doc still reads:

 * - Graceful error handling (never fails workflow)

but sendMessage now throws RuntimeException on non-200 responses. Update the docstring to match the new contract.

@adamrtalbot

Copy link
Copy Markdown
Collaborator Author

Addressed Claude review in ac9a548:

  • Documented failOnError in docs/reference/api.md and docs/usage/guide.md.
  • Added commented example in example/configs/01-minimal.config.
  • Added SlackExtensionTest coverage for failOnError true/false paths.
  • Reaction failures log at DEBUG when failOnError=false (preserves prior behavior); other failures still WARN.
  • Updated CHANGELOG.md and aligned sender tests with throw-on-failure semantics.

./gradlew test passes locally (151 tests).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow nf-slack to fail the pipeline on notification errors

1 participant