Repository navigation
feat(workflows): http step type (webhook alias) with retries - #2641
Merged
Merged
Conversation
Add a native `type: webhook` step for workflows and custom commands that performs an HTTP request (GET/POST/PUT/PATCH/DELETE/HEAD/OPTIONS) with query-string parameters, headers, and a request body (raw or form/JSON). Requests get a per-attempt timeout and retries that compose with the existing `retry:` policy. Because extended/registry step types are not wrapped by the legacy retry.Do path, the handler applies retry itself via retry.WithPredicate with HTTP-aware classification: transport errors, 5xx, and 429 retry by default; other 4xx fail fast; retry.conditions regexes force additional cases. Success criteria are configurable via expect.status and expect.response (regex). The response body and status are captured as step value/metadata for downstream steps. Includes schema fields on WorkflowStep and Task, JSON manifest updates, docs, an example, a changelog blog post, and a roadmap milestone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
requested review from
a team
as code owners
June 20, 2026 15:20
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Erik Osterman (Cloud Posse) (osterman)
added
the
minor
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a native ChangesHTTP Step Type
Sequence Diagram(s)sequenceDiagram
participant Workflow as Workflow Engine
participant HTTPHandler
participant RetryPolicy
participant HTTPClient as http.Client
participant Endpoint as Remote Endpoint
Workflow->>HTTPHandler: Execute(ctx, step, vars)
HTTPHandler->>HTTPHandler: buildHTTPRequest(step, vars)
HTTPHandler->>HTTPHandler: resolveHTTPTimeout(step)
HTTPHandler->>RetryPolicy: run with shouldRetryHTTP
loop each attempt
RetryPolicy->>HTTPHandler: performHTTPRequest(ctx + deadline)
HTTPHandler->>HTTPClient: Do(req)
HTTPClient->>Endpoint: HTTP request
Endpoint-->>HTTPClient: response
HTTPClient-->>HTTPHandler: response
HTTPHandler->>HTTPHandler: read body, flatten headers
HTTPHandler->>HTTPHandler: httpExpect.check(status, body)
HTTPHandler-->>RetryPolicy: result or httpError
end
RetryPolicy-->>HTTPHandler: final result or error
HTTPHandler->>HTTPHandler: buildHTTPError()
HTTPHandler-->>Workflow: StepResult or sentinel error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
pkg/runner/step/webhook_test.go (1)
356-382: ⚡ Quick winAdd the matching negative-path test for custom retry conditions.
You validate recovery on
400whenretry.conditionsincludes^400, but there’s no paired case proving400does not retry when that condition is absent.As per coding guidelines, “Include negative-path tests for recovery logic: whenever a test verifies that a recovery/fallback triggers under condition X, add a corresponding test that verifies the recovery does NOT trigger when condition X is absent.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/runner/step/webhook_test.go` around lines 356 - 382, Add a negative-path test that verifies a 400 status code does NOT retry when the retry condition is absent or does not match. Create a new test function similar to TestWebhookHandler_RetryConditions that sets up the same webhook server returning 400 on the first call, but configure the step with either no retry conditions or retry conditions that do not match 400 (e.g., a pattern like `^500 `). Verify that the handler.Execute call fails or returns an error on the first attempt without retrying (by asserting that calls equals 1).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/runner/step/webhook_test.go`:
- Line 107: The Get("webhook") function returns both a handler and a boolean
indicating success, but the code is ignoring the boolean return value with an
underscore. In each test where Get("webhook") is called (at lines 107, 141, 172,
200, 224, 256, 289, 318, 341, 370, 401, and 421), capture the boolean return
value instead of ignoring it and add an assertion to verify it is true, so that
if the webhook handler registration fails the test will fail with a clear error
message rather than panicking later when methods like Validate() are called on
the nil handler.
- Around line 163-166: The require.NoError call inside the httptest handler
goroutine passed to httptest.NewServer is unsafe because FailNow cannot be
called from non-test goroutines and can cause flaky failures. Instead of calling
require.NoError directly in the handler when r.ParseForm() is executed, declare
an error variable outside the handler, capture any error from r.ParseForm() into
it within the handler, and then check that error variable with require.NoError
in the main test scope after the server request completes. This ensures all
assertion operations happen in the test goroutine where they are safe.
In `@pkg/runner/step/webhook.go`:
- Around line 379-382: Replace the dynamic fmt.Errorf call on line 381 (and
similar dynamic error returns on lines 414, 428, 439, and 446) with static
sentinel errors from errors/errors.go. For each failure path (headers resolution
via ResolveEnvMap, query resolution, body resolution, form resolution, and JSON
encoding), identify or create the appropriate static sentinel error and use it
to wrap the original error properly, maintaining error chain information using
the existing error wrapping pattern in the codebase. Ensure all error return
statements follow the static-error contract to enable consistent errors.Is
handling.
- Around line 365-373: After the successful url.Parse call in the webhook
validation logic, add an explicit check to ensure the parsed URL is absolute
(has a scheme). If the URL is not absolute, immediately return an error with the
same error building pattern used for the parse error, including appropriate
context about step name and URL value, to fail fast with clean config feedback
rather than allowing relative URLs to proceed and fail later during client.Do
execution.
- Around line 238-241: The io.ReadAll(resp.Body) call on line 238 reads the
entire response body without any size limit, which can cause memory spikes with
large responses. Wrap resp.Body with io.LimitReader before passing it to
io.ReadAll to enforce a maximum read size. This prevents unbounded memory
consumption when processing webhook responses. If needed, you can optionally
detect and return a dedicated sentinel value when the response exceeds the
configured limit to distinguish between truncation and genuine errors.
In `@pkg/schema/workflow.go`:
- Line 105: The comment for the Method field in the workflow schema is missing
OPTIONS from the list of supported HTTP methods. Update the comment string for
the Method field to include OPTIONS in the enumerated list of supported HTTP
verbs, keeping the list consistent with what the webhook schema actually
supports. This ensures the documentation and comments accurately reflect the
available functionality.
---
Nitpick comments:
In `@pkg/runner/step/webhook_test.go`:
- Around line 356-382: Add a negative-path test that verifies a 400 status code
does NOT retry when the retry condition is absent or does not match. Create a
new test function similar to TestWebhookHandler_RetryConditions that sets up the
same webhook server returning 400 on the first call, but configure the step with
either no retry conditions or retry conditions that do not match 400 (e.g., a
pattern like `^500 `). Verify that the handler.Execute call fails or returns an
error on the first attempt without retrying (by asserting that calls equals 1).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eb22eef0-b10c-46f9-9460-d308c3a39a2e
📒 Files selected for processing (14)
errors/errors.goexamples/workflow-webhook/README.mdexamples/workflow-webhook/atmos.yamlexamples/workflow-webhook/workflows/webhook.yamlpkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/runner/step/webhook.gopkg/runner/step/webhook_e2e_test.gopkg/runner/step/webhook_test.gopkg/schema/task.gopkg/schema/workflow.gowebsite/blog/2026-06-20-webhook-step-type.mdxwebsite/docs/workflows/_partials/_step-types.mdxwebsite/docs/workflows/workflows.mdxwebsite/src/data/roadmap.js
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2641 +/- ##
==========================================
+ Coverage 80.36% 80.41% +0.05%
==========================================
Files 1399 1400 +1
Lines 132224 132569 +345
==========================================
+ Hits 106267 106611 +344
+ Misses 20076 20066 -10
- Partials 5881 5892 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
- Replace dynamic fmt.Errorf returns with static sentinel errors (ErrTemplateEvaluation / ErrWebhookRequestFailed) per the static-error contract. - Bound webhook response-body reads with io.LimitReader (4 MiB) to avoid memory blowups on large/error responses. - Reject relative URLs in buildWebhookRequest so they fail fast instead of being retried as transport errors. - Add OPTIONS to the Method field comment to match supported verbs. - Add mustGetWebhookHandler helper asserting registration; move ParseForm assertion out of the httptest handler goroutine. - Expand tests to cover error/helper paths, raising patch coverage above the 80% gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The step is a general-purpose, verb-agnostic outbound HTTP client, not an inbound webhook receiver. Rename the canonical type to `http` and keep `webhook` as a working alias for the fire-a-notification use case. - Add first-class alias support to the step registry: NewBaseHandler is now variadic for aliases, Get() resolves aliases, and List/Count report only the canonical entry (no duplicate step). Aliases are exposed via an optional GetAliases() interface so the StepHandler interface (and its mock) are unchanged. - Rename webhook.go/test/e2e -> http*.go and all Webhook*/webhook* symbols to HTTP*/http*; register as "http" with "webhook" alias. - Rename WebhookExpect -> HTTPExpect (workflow.go, task.go) and the 7 ErrWebhook* sentinels -> ErrHTTPStep* in errors/errors.go. - JSON manifest: url-required conditional now matches type in [http, webhook]; descriptions updated to "http step type". - Rename example examples/workflow-webhook -> examples/http-webhooks (type: http, webhook noted as alias). - Docs/blog/roadmap: step-types reference, retry.conditions link, blog slug http-step-type, roadmap changelog slug; all note the webhook alias. - Add a contract test asserting the webhook alias resolves to the http handler and is not listed as a distinct step type. - Drive-by: extract a const for a repeated "secret %s" literal in azure_keyvault_store.go to satisfy the repo-wide lint hook. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/runner/step/http.go (1)
532-540: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject non-positive timeouts so they fail fast.
If
timeoutresolves to0sor a negative value,context.WithTimeoutinExecuteproduces an already-expired context. Every attempt then returnsDeadlineExceeded, whichshouldRetryHTTPtreats as a retryable transport error, so the step burns through all retries instantly and surfaces a misleading "request failed" message. A quick guard gives clear config feedback instead.🛡️ Suggested guard
parsed, err := time.ParseDuration(resolved) if err != nil { return 0, errUtils.Build(errUtils.ErrInvalidDuration). WithCause(err). WithContext("step", step.Name). WithContext("value", resolved). Err() } + if parsed <= 0 { + return 0, errUtils.Build(errUtils.ErrInvalidDuration). + WithContext("step", step.Name). + WithContext("value", resolved). + WithHint("Set 'timeout' to a positive duration (e.g. 30s)"). + Err() + } return parsed, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/runner/step/http.go` around lines 532 - 540, The timeout parsing function needs to validate that the parsed duration is positive before returning it. After the time.ParseDuration call succeeds, add a guard check to ensure the parsed duration is greater than zero. If the parsed duration is less than or equal to zero, return an error using the same errUtils.Build pattern already shown in the code, with appropriate context about the step name and invalid timeout value. This guard should be placed immediately after the successful parse and before the return statement to provide clear configuration feedback instead of misleading retry failures.
🧹 Nitpick comments (2)
pkg/runner/step/http_test.go (1)
49-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd compile-time schema field sentinels for test contracts.
These tests rely heavily on
schema.WorkflowStep/schema.HTTPExpectfield names but do not include compile guards. Add package-level sentinels so schema field renames fail at compile time.Suggested minimal guard block
+var ( + _ = schema.WorkflowStep{ + Name: "", Type: "", URL: "", Method: "", + Headers: map[string]string{}, Query: map[string]string{}, + Body: "", Form: map[string]string{}, Timeout: "", Retry: &schema.RetryConfig{}, + Expect: &schema.HTTPExpect{Status: []int{}, Response: []string{}}, + } + _ = schema.HTTPExpect{Status: []int{}, Response: []string{}} +)As per coding guidelines, “Add compile-time sentinels for schema field references in tests … so a field rename immediately fails the build.”
Also applies to: 235-248, 470-470
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/runner/step/http_test.go` around lines 49 - 92, Add compile-time schema field sentinels at the package level in the test file to ensure that schema field renames fail the build immediately. Create a sentinel function that references all the schema fields used in the TestHTTPHandler_Validate test (such as Name, Type, URL, Method, Body, Form, Expect, Status, and Response fields from schema.WorkflowStep and schema.HTTPExpect structs) by accessing them directly on empty struct instances. This guard function will cause compilation to fail if any of these field names are renamed in the schema, preventing silent test failures. Add this sentinel function near the top of the test file after the package declaration.Source: Coding guidelines
pkg/runner/step/http.go (1)
543-556: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRelease idle connections after each
Execute.A fresh
http.Transportis built perExecutecall, and nothing tears its connection pool down, so idle sockets linger untilhttpIdleTimeout(30s). A workflow with many http steps can stack these up. A one-liner inExecutekeeps things tidy.♻️ Suggested cleanup in
Executeclient := newHTTPClient() + defer client.CloseIdleConnections()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/runner/step/http.go` around lines 543 - 556, Fresh http.Transport instances are created per Execute call by the newHTTPClient() function, but their connection pools are never cleaned up, causing idle sockets to linger until the httpIdleTimeout expires. Add a call to CloseIdleConnections() on the http.Transport in the Execute method after the HTTP request completes to immediately release idle connections and prevent accumulation of sockets in workflows with many HTTP steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/runner/step/http_test.go`:
- Around line 443-444: The hardcoded URL with port 1 is not deterministic across
different test runners and can cause intermittent failures. Replace the
`http://127.0.0.1:1` URL with a dynamically allocated but
guaranteed-to-be-closed port: create a temporary listener on `:0` which will be
assigned an available port, capture that address, close the listener, and then
use that address in the URL field for the test request. This ensures the port
will refuse connections deterministically since it was just released by the
closed listener.
---
Outside diff comments:
In `@pkg/runner/step/http.go`:
- Around line 532-540: The timeout parsing function needs to validate that the
parsed duration is positive before returning it. After the time.ParseDuration
call succeeds, add a guard check to ensure the parsed duration is greater than
zero. If the parsed duration is less than or equal to zero, return an error
using the same errUtils.Build pattern already shown in the code, with
appropriate context about the step name and invalid timeout value. This guard
should be placed immediately after the successful parse and before the return
statement to provide clear configuration feedback instead of misleading retry
failures.
---
Nitpick comments:
In `@pkg/runner/step/http_test.go`:
- Around line 49-92: Add compile-time schema field sentinels at the package
level in the test file to ensure that schema field renames fail the build
immediately. Create a sentinel function that references all the schema fields
used in the TestHTTPHandler_Validate test (such as Name, Type, URL, Method,
Body, Form, Expect, Status, and Response fields from schema.WorkflowStep and
schema.HTTPExpect structs) by accessing them directly on empty struct instances.
This guard function will cause compilation to fail if any of these field names
are renamed in the schema, preventing silent test failures. Add this sentinel
function near the top of the test file after the package declaration.
In `@pkg/runner/step/http.go`:
- Around line 543-556: Fresh http.Transport instances are created per Execute
call by the newHTTPClient() function, but their connection pools are never
cleaned up, causing idle sockets to linger until the httpIdleTimeout expires.
Add a call to CloseIdleConnections() on the http.Transport in the Execute method
after the HTTP request completes to immediately release idle connections and
prevent accumulation of sockets in workflows with many HTTP steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fe68410d-4772-496a-82d7-8074d62bed0e
📒 Files selected for processing (17)
errors/errors.goexamples/http-webhooks/README.mdexamples/http-webhooks/atmos.yamlexamples/http-webhooks/workflows/http.yamlpkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/runner/step/handler_base.gopkg/runner/step/http.gopkg/runner/step/http_e2e_test.gopkg/runner/step/http_test.gopkg/runner/step/registry.gopkg/schema/task.gopkg/schema/workflow.gopkg/store/azure_keyvault_store.gowebsite/blog/2026-06-20-http-step-type.mdxwebsite/docs/workflows/_partials/_step-types.mdxwebsite/docs/workflows/workflows.mdxwebsite/src/data/roadmap.js
💤 Files with no reviewable changes (1)
- examples/http-webhooks/atmos.yaml
✅ Files skipped from review due to trivial changes (5)
- examples/http-webhooks/README.md
- website/docs/workflows/_partials/_step-types.mdx
- website/docs/workflows/workflows.mdx
- pkg/store/azure_keyvault_store.go
- website/blog/2026-06-20-http-step-type.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- website/src/data/roadmap.js
- pkg/datafetcher/schema/atmos/manifest/1.0.json
- pkg/schema/task.go
…-type # Conflicts: # pkg/store/azure_keyvault_store.go
Erik Osterman (Cloud Posse) (osterman)
changed the title
Contributor
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
…k-step-type Integrates PR #2626, which deleted the monolithic workflows.mdx/commands.mdx docs and replaced them with a nested per-topic structure where each step type gets its own page under workflows/workflow/steps/type/<name>.mdx. Conflict resolution: - pkg/schema/{workflow,task}.go: keep both the HTTP step fields (URL/Method/ Headers/Query/Body/Form/Expect) and main's new container step fields + Outputs, including the Task<->WorkflowStep conversions. - Adopt main's new docs structure; drop the now-obsolete edits to the deleted monolithic workflows.mdx and the old _step-types.mdx layout. Refactor the http step docs to the new convention: - Add website/docs/workflows/workflows/workflow/steps/type/http.mdx (Intro + fields + polling + result, matching sibling pages). - Register http in the _step-types.mdx overview and the steps/type.mdx Command Types table; note the webhook alias. - Migrate the retry.conditions reference into steps/retry.mdx. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
Andriy Knysh (aknysh)
approved these changes
Jun 24, 2026
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.222.0-rc.9. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
httpworkflow/custom-command step (type: http) that performs an HTTP request — any verb (GET/POST/PUT/PATCH/DELETE/HEAD/OPTIONS),querystring parameters,headers, and a request body viabody(raw) orform(urlencoded, or JSON whenContent-Typeis JSON).webhookas a first-class alias forhttp(type: webhookbehaves identically) for the fire-a-notification use case. This adds alias support to the step registry:NewBaseHandleris variadic for aliases,Get()resolves aliases, andList/Countreport only the canonical entry (no duplicate step type).timeoutandretrythat composes with the existingretry:policy; retry is HTTP-aware (transport errors,5xx, and429retry by default, other4xxfail fast, andretry.conditionsregexes force additional cases).expect.status(codes) andexpect.response(regexes); the response body and status are captured as the step's value/metadata for downstream steps.WorkflowStepandTask(so it works in both workflows and custom commands) plus theHTTPExpectstruct,ErrHTTPStep*sentinels, JSON manifest updates, docs, anexamples/http-webhooksexample, a changelog blog post, and a roadmap milestone.why
curl, which isn't portable (Windows), is awkward to template, and gets no first-class timeout/retry handling.httpis the accurate name (an inbound callback receiver is what "webhook" conventionally means);webhookis retained as an alias so the evocative name still works.retry.Dopath thatshell/atmosuse, so the handler applies retry itself viaretry.WithPredicate— which is what enables status-code-aware retry decisions a generic wrapper can't make.references
website/blog/2026-06-20-http-step-type.mdx