feat(hooks): run custom step types as lifecycle hooks (kind: step) - #2658
Merged
Conversation
Add a `kind: step` hook that delegates to the workflow/custom-command step
registry, making every registered step type (container, http, toast, log, ...)
available on terraform lifecycle events. Name a step `type:` and pass its
parameters under `with:`; `on_failure` and `retry` are envelope-level policy
applied around the step (no import cycle — pkg/hooks imports pkg/runner/step).
Also plumb the operation outcome to hooks so they can report what happened:
- user hooks now fire on the failure path (not just success)
- a `when: success|failure|always` selector (default success) preserves
back-compat while letting hooks opt into failure firing
- `{{ .status }}`/`{{ .exit_code }}`/`{{ .error }}` template context and
ATMOS_HOOK_* env vars expose the outcome alongside component/stack
Includes a structured hook-envelope JSON schema (kind enum incl. step, when,
type, with), docs, PRD, changelog, and roadmap updates.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
June 24, 2026 01:29
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Erik Osterman (Cloud Posse) (osterman)
added
the
minor
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
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 (8)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds shared ChangesConditional execution and step-backed hooks
Sequence Diagram(s)sequenceDiagram
participant ExecuteWorkflow
participant workflowConditionContext
participant Condition
ExecuteWorkflow->>workflowConditionContext: build CI/status context
workflowConditionContext-->>ExecuteWorkflow: schema.ConditionContext
ExecuteWorkflow->>Condition: Evaluate(step.When)
alt matches
Condition-->>ExecuteWorkflow: true
ExecuteWorkflow->>ExecuteWorkflow: render and execute step
else does not match
Condition-->>ExecuteWorkflow: false
ExecuteWorkflow->>ExecuteWorkflow: mark step skipped
end
sequenceDiagram
participant runHooksWithOutput
participant runHooksOnErrorWithOutput
participant prepareHookContext
participant runUserHooks
participant Hooks
alt success path
runHooksWithOutput->>prepareHookContext: build hookContext
prepareHookContext-->>runHooksWithOutput: hookContext
runHooksWithOutput->>runUserHooks: run user hooks with RunSuccess
runUserHooks->>Hooks: RunAll(outcome)
runHooksWithOutput->>Hooks: RunCIHooksOptions(&hctx.atmosConfig, &hctx.info)
else error path
runHooksOnErrorWithOutput->>prepareHookContext: build hookContext
prepareHookContext-->>runHooksOnErrorWithOutput: hookContext
runHooksOnErrorWithOutput->>runUserHooks: run user hooks with RunFailure
runUserHooks->>Hooks: RunAll(outcome)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
🧹 Nitpick comments (1)
pkg/hooks/hooks.go (1)
126-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd perf tracking to
RunAll.
RunAllis a changed public method that performs preflight and hook engine execution, so it does not fit the trivial getter/setter or pure lookup exceptions.As per coding guidelines, “Add
defer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions.”🤖 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/hooks/hooks.go` around lines 126 - 132, Add performance tracking to the RunAll method in the Hooks struct. At the beginning of the RunAll function body, add a defer statement calling perf.Track with atmosConfig and the function identifier "pkg.hooks.RunAll", followed by a blank line before the existing outcome assignment. This aligns with the coding guidelines requiring performance tracking on all public methods that perform substantive operations.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/datafetcher/schema/atmos/manifest/1.0.json`:
- Around line 1337-1342: The current hook schema only accepts object types and
breaks backward compatibility with legacy include-string values like `some-hook:
!include ...`. Modify the additionalProperties definition for hooks by replacing
the single object schema with a oneOf structure that allows either a string type
(for legacy includes) or the existing object type with its properties and
additionalProperties. Apply this same oneOf pattern to any other schema copies
that define hooks in the manifest file to ensure consistency across all hook
definitions.
In `@pkg/hooks/command_engine.go`:
- Around line 271-273: The exported function BuildAtmosEnv is missing the
required performance tracking hook that should be added to all public functions.
Add the appropriate perf tracking hook at the beginning of the BuildAtmosEnv
function before the call to the internal buildAtmosEnv function, following the
same tracking pattern used in other exported functions in the same file.
In `@pkg/hooks/hooks.go`:
- Around line 155-158: The when filtering using RunsOnStatus(outcome.Status) is
applied after preflight checks, causing verifyAllBinaries to validate all hooks
regardless of whether they should run based on the when condition. This means on
a failure path, a success-only hook with missing commands can fail preflight
before failure hooks execute. Move the when status filtering earlier by checking
hook.RunsOnStatus(outcome.Status) before running preflight validation, or
refactor the preflight logic to skip validation for hooks that should not run
based on their when condition, ensuring failure-path hooks are not blocked by
non-applicable success-only hooks during preflight.
In `@website/src/data/roadmap.js`:
- Line 336: The roadmap entry for 'Run any step type as a lifecycle hook (kind:
step)' at line 336 is missing the required `pr` field that should reference the
pull request number associated with this shipped milestone. Add a `pr:
<pr-number>` property to this roadmap item object (following the same pattern as
the existing `prd`, `changelog`, and other metadata fields). Additionally,
locate the corresponding initiative object that contains this milestone and
update its `progress` percentage field to reflect the completion of this shipped
item, ensuring the roadmap metadata remains consistent across the file as per
the coding guidelines.
---
Nitpick comments:
In `@pkg/hooks/hooks.go`:
- Around line 126-132: Add performance tracking to the RunAll method in the
Hooks struct. At the beginning of the RunAll function body, add a defer
statement calling perf.Track with atmosConfig and the function identifier
"pkg.hooks.RunAll", followed by a blank line before the existing outcome
assignment. This aligns with the coding guidelines requiring performance
tracking on all public methods that perform substantive operations.
🪄 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: 4c1b00ba-c700-4fe4-a1fa-49dc83db8375
📒 Files selected for processing (15)
cmd/terraform/utils.godocs/prd/hooks-step-types.mdpkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema/config/global/1.0.jsonpkg/datafetcher/schema/stacks/stack-config/1.0.jsonpkg/hooks/command_engine.gopkg/hooks/hook.gopkg/hooks/hooks.gopkg/hooks/hooks_test.gopkg/hooks/kind.gopkg/hooks/step_engine.gopkg/hooks/step_engine_test.gowebsite/blog/2026-06-23-hooks-step-types.mdxwebsite/docs/stacks/hooks.mdxwebsite/src/data/roadmap.js
# Conflicts: # cmd/terraform/utils.go
… roadmap) - preflight/verifyAllBinaries skip hooks that won't run for the current outcome status, so a success-only hook with a missing binary cannot block failure-path (when: failure) hooks; extracted verifyHookBinary helper - hooks JSON schema per-hook value accepts `!include` strings again (oneOf string|object) across all three schema copies - add perf.Track to exported BuildAtmosEnv - roadmap: add pr: 2658 to the kind:step milestone and bump extensibility progress 93 -> 94 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ateData Size the augmented template-data map from a single len(section) instead of len(section)+3; CodeQL's go/allocation-size-overflow rule flags len(x)+N. The map grows as needed for the three outcome keys. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2658 +/- ##
==========================================
+ Coverage 80.40% 80.44% +0.04%
==========================================
Files 1400 1402 +2
Lines 132569 133010 +441
==========================================
+ Hits 106590 107006 +416
- Misses 20093 20104 +11
- Partials 5886 5900 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
# Conflicts: # pkg/hooks/command_engine.go # pkg/hooks/hooks.go # pkg/hooks/hooks_test.go # website/src/data/roadmap.js
Resource Changes Found for
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/datafetcher/schema/atmos/manifest/1.0.json (1)
1643-1646: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate hook
retrywith the actual retry schema.This block says hook retries use the workflow-step retry contract, but the schema currently accepts any object here. That weakens validation for a new public config surface and diverges from workflow steps. Reuse the concrete retry schema here and mirror that into the other schema copies.
🤖 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/datafetcher/schema/atmos/manifest/1.0.json` around lines 1643 - 1646, The hook retry field is too loosely validated because it currently uses a generic object instead of the workflow-step retry contract. Update the retry definition in the manifest schema to reference the concrete retry schema used by workflow steps, and apply the same change to the other duplicated schema copies so all public config surfaces stay consistent.
🧹 Nitpick comments (1)
internal/exec/workflow_utils_test.go (1)
1193-1215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a skipped-step-with-
identitycase here.This covers the simple skip path, but it will not catch the eager-auth regression where a
when: neverstep still fails during auth initialization before the skip check. A second case withIdentityset would lock that behavior down. As per coding guidelines, “All features need tests” and “Maintain 80% minimum test coverage.”🤖 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 `@internal/exec/workflow_utils_test.go` around lines 1193 - 1215, Add a second test case in TestExecuteWorkflow_SkipsStepWhenConditionIsFalse that sets WorkflowStep.Identity and still uses a never condition, so ExecuteWorkflow is exercised with auth initialization present. Reuse the existing workflow test setup and assert the step is skipped without error, verifying the skip happens before any eager auth path in ExecuteWorkflow/WorkflowStep handling.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 `@cmd/custom_command_integration_test.go`:
- Around line 271-276: The test helper writeCmd in
custom_command_integration_test.go relies on platform-specific shell commands
and should be replaced with a Go-native approach. Update the test to avoid
cmd/printf by using a helper subprocess launched via os.Executable() and
test-helper mode, or inject a fake runner and perform file writes with the Go
stdlib instead. Keep the change localized to writeCmd and the surrounding test
setup so the test no longer depends on shell behavior.
In `@internal/exec/workflow_utils.go`:
- Around line 379-382: The upfront auth setup in workflow step handling is still
running for steps that will be skipped by the `when` condition. Update the auth
pre-scan in `workflow_utils.go` so it uses the same `workflowConditionContext()`
gating as the `step.When.Evaluate(...)` check, or move auth initialization
behind the runnable-step path in the workflow step loop. Make sure the logic
around `needsAuth`, `step.When`, and the auth manager creation only applies to
steps that will actually execute.
In `@pkg/hooks/hooks.go`:
- Around line 44-47: The preflight cache is keyed too broadly by only HookEvent,
but Hooks.preflight now varies by event, status, and isCI, so a prior non-CI or
failure run can incorrectly skip checks for a later runnable hook set. Update
the caching in Hooks.preflight/related event tracking to key by the full
runnable-hook filter (including status and CI state, not just event) so
dependency installation and binary verification still run when the hook set
changes.
In `@pkg/schema/condition_test.go`:
- Around line 270-278: TestRegisterConditionPredicate mutates the package-global
predicate registry by registering custom-test-condition and leaves it behind for
later tests; add a t.Cleanup in TestRegisterConditionPredicate to restore the
prior state or delete that predicate after the assertions. Use
RegisterConditionPredicate and MustCondition to locate the test, and make sure
the cleanup keeps the package test run isolated.
In `@pkg/schema/condition.go`:
- Around line 14-31: The new public API in condition.go is missing Go doc
comments, so add concise documentation for each exported ConditionPredicate*
constant, ErrInvalidWhenCondition, and ConditionPredicateFunc. Follow Go
conventions by placing a comment starting with the symbol name immediately above
each declaration in the condition package so the extension points and error
meaning are clear to consumers.
In `@pkg/schema/workflow.go`:
- Around line 253-256: Keep the step/task conversion logic in sync with the new
say-step fields by updating the Task model and the conversion helpers in task.go
so Voice, Rate, and Print are preserved when normalizing a WorkflowStep into a
Task. Inspect the existing step field copying around the Task conversion methods
and add these three fields wherever the surrounding step properties are mapped,
ensuring any constructors, copy helpers, or serialization tags on Task reflect
the new schema fields.
In `@pkg/workflow/executor.go`:
- Around line 216-220: The workflow step condition context always hardcodes
Status to success in workflowConditionContext, so workflow steps can never match
when: failure. Update the evaluation path used by runSteps and
workflowConditionContext to either reject outcome-based predicates like failure
for workflow steps up front or pass a real step status into
schema.ConditionContext before evaluating step.When.
In `@tests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json`:
- Around line 124-194: The fixture schema is out of sync with the shipped
manifest schema because definitions.hooks still accepts arbitrary values instead
of the structured hook envelope. Update the atmos-manifest fixture to match
pkg/datafetcher/schema/atmos/manifest/1.0.json by aligning the hooks definition
with the new envelope shape and keeping the existing condition/when changes
consistent across the schema.
In `@website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json`:
- Around line 124-194: Workflow-step `when` currently accepts outcome predicates
like `success` and `failure`, but `workflow_utils` only evaluates steps with a
success status, so `failure` is schema-valid yet unreachable at runtime. Update
the condition schema around `condition_predicate` and `condition` to restrict
step-level usage to predicates that can actually be evaluated for steps, or
otherwise separate hook-only predicates from step predicates. Use the existing
`condition_predicate` and `condition` definitions as the place to narrow
validation so `failure` is rejected for workflow steps while remaining available
where it is supported.
---
Outside diff comments:
In `@pkg/datafetcher/schema/atmos/manifest/1.0.json`:
- Around line 1643-1646: The hook retry field is too loosely validated because
it currently uses a generic object instead of the workflow-step retry contract.
Update the retry definition in the manifest schema to reference the concrete
retry schema used by workflow steps, and apply the same change to the other
duplicated schema copies so all public config surfaces stay consistent.
---
Nitpick comments:
In `@internal/exec/workflow_utils_test.go`:
- Around line 1193-1215: Add a second test case in
TestExecuteWorkflow_SkipsStepWhenConditionIsFalse that sets
WorkflowStep.Identity and still uses a never condition, so ExecuteWorkflow is
exercised with auth initialization present. Reuse the existing workflow test
setup and assert the step is skipped without error, verifying the skip happens
before any eager auth path in ExecuteWorkflow/WorkflowStep handling.
🪄 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: 0905fbe4-328a-4a74-aebf-e1c7525ba360
📒 Files selected for processing (27)
cmd/cmd_utils.gocmd/custom_command_integration_test.gointernal/exec/workflow_utils.gointernal/exec/workflow_utils_test.gopkg/config/load.gopkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema/config/global/1.0.jsonpkg/datafetcher/schema_condition_validation_test.gopkg/hooks/command_engine.gopkg/hooks/hook.gopkg/hooks/hooks.gopkg/hooks/hooks_test.gopkg/hooks/kind.gopkg/hooks/step_engine_test.gopkg/schema/condition.gopkg/schema/condition_test.gopkg/schema/task.gopkg/schema/workflow.gopkg/workflow/executor.gopkg/workflow/executor_test.gotests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.jsontests/test-cases/native-ci-e2e.yamlwebsite/docs/cli/configuration/commands/command/steps.mdxwebsite/docs/stacks/hooks.mdxwebsite/docs/workflows/workflows/workflow/steps/step.mdxwebsite/src/data/roadmap.jswebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
✅ Files skipped from review due to trivial changes (3)
- website/docs/workflows/workflows/workflow/steps/step.mdx
- tests/test-cases/native-ci-e2e.yaml
- website/src/data/roadmap.js
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/hooks/kind.go
- pkg/hooks/command_engine.go
- pkg/hooks/hooks_test.go
- pkg/hooks/step_engine_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/schema/task_test.go (1)
250-282: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the new
Whenfield in these round-trip tests.
pkg/schema/task.gonow copiesWhenin both directions, but these fixtures only verify the say fields. Adding oneWhenassertion to each test keeps the conversion contract covered. As per coding guidelines, "All features need tests."Also applies to: 287-318
🤖 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/schema/task_test.go` around lines 250 - 282, The round-trip tests for Task conversion are missing coverage for the new When field, so update the TaskToWorkflowStep and related conversion test fixtures to set a When value on the Task and assert it is preserved on the resulting workflow step; use the Task.ToWorkflowStep and corresponding reverse-conversion test cases to verify When is copied in both directions alongside the existing fields.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 `@cmd/cmd_utils.go`:
- Around line 683-706: The runnable-step check in custom command execution is
being done too late, after dependency setup has already run. Move the
step-condition evaluation in the custom command flow (around the hasRunnableStep
logic in cmd_utils.go) ahead of ResolveCommandDependencies and EnsureTools, then
short-circuit early when no steps are runnable so skipped commands do not
install tools or fail unnecessarily.
In `@pkg/datafetcher/schema_condition_validation_test.go`:
- Around line 21-27: The schema validation tests are missing coverage for the
scalar-child all form, so add a valid workflow and hook case for the decoded
condition shape used by condition handling. Update the valid condition fixtures
in schema_condition_validation_test.go to include the scalar all operand
alongside the existing scalar/list/all/any/not entries, and make sure the test
exercises the same all:"ci" variant that condition_test.go already accepts so
schema and runtime stay aligned.
In `@pkg/datafetcher/schema/atmos/manifest/1.0.json`:
- Around line 166-173: The condition schema for the all branch is too narrow
because it only accepts an array, while the runtime and existing tests already
accept a scalar nested condition like {"all":"ci"}. Update the all property in
the step_condition definition (and the copied condition/properties/all shape) so
it allows either a single step_condition or an array of step_condition items,
keeping the existing minimum-items behavior only for the array form.
---
Outside diff comments:
In `@pkg/schema/task_test.go`:
- Around line 250-282: The round-trip tests for Task conversion are missing
coverage for the new When field, so update the TaskToWorkflowStep and related
conversion test fixtures to set a When value on the Task and assert it is
preserved on the resulting workflow step; use the Task.ToWorkflowStep and
corresponding reverse-conversion test cases to verify When is copied in both
directions alongside the existing fields.
🪄 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: 23ff495b-a539-4a73-96ab-0f6b17d54dea
📒 Files selected for processing (28)
cmd/cmd_utils.gocmd/custom_command_integration_test.gointernal/exec/workflow_utils.gointernal/exec/workflow_utils_test.gopkg/config/load.gopkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema/config/global/1.0.jsonpkg/datafetcher/schema_condition_validation_test.gopkg/hooks/command_engine.gopkg/hooks/hook.gopkg/hooks/hooks.gopkg/hooks/hooks_test.gopkg/hooks/kind.gopkg/hooks/step_engine_test.gopkg/schema/condition.gopkg/schema/condition_test.gopkg/schema/task.gopkg/schema/task_test.gopkg/schema/workflow.gopkg/workflow/executor.gopkg/workflow/executor_test.gotests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.jsontests/test-cases/native-ci-e2e.yamlwebsite/docs/cli/configuration/commands/command/steps.mdxwebsite/docs/stacks/hooks.mdxwebsite/docs/workflows/workflows/workflow/steps/step.mdxwebsite/src/data/roadmap.jswebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
✅ Files skipped from review due to trivial changes (4)
- website/docs/cli/configuration/commands/command/steps.mdx
- website/docs/workflows/workflows/workflow/steps/step.mdx
- tests/test-cases/native-ci-e2e.yaml
- website/src/data/roadmap.js
🚧 Files skipped from review as they are similar to previous changes (14)
- pkg/hooks/kind.go
- pkg/schema/workflow.go
- internal/exec/workflow_utils_test.go
- pkg/config/load.go
- internal/exec/workflow_utils.go
- pkg/datafetcher/schema/config/global/1.0.json
- website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
- pkg/hooks/command_engine.go
- pkg/hooks/hook.go
- pkg/hooks/step_engine_test.go
- pkg/schema/condition.go
- pkg/hooks/hooks_test.go
- pkg/hooks/hooks.go
- pkg/workflow/executor.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/datafetcher/schema/atmos/manifest/1.0.json (1)
1608-1661: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
workflow_retryfor the workflow stepretryto avoid drift.The new
workflow_retrydefinition is byte-for-byte identical to the inlineretryschema on workflow steps (Lines 1508-1561). The hooksretryalready points at#/definitions/workflow_retry; pointing the workflow step there too keeps a single source of truth so the two can't silently diverge.♻️ Replace the inline step retry with a `$ref` (Lines 1508-1561)
- "retry": { - "title": "retry", - "description": "Retry configuration for workflow step execution", - "oneOf": [ - { - "type": "string", - "pattern": "^!include" - }, - { - "type": "object", - "additionalProperties": false, - "properties": { - "max_attempts": { "type": "integer", "minimum": 1, "description": "Maximum number of retry attempts" }, - "initial_delay": { "type": "string", "description": "Initial delay between retries (e.g., '1s', '500ms')" }, - "max_delay": { "type": "string", "description": "Maximum delay between retries (e.g., '30s', '1m')" }, - "backoff_strategy": { "type": "string", "enum": ["constant","linear","exponential"], "description": "Strategy for increasing delay between retries" }, - "multiplier": { "type": "number", "minimum": 1, "description": "Multiplier for exponential/linear backoff" }, - "random_jitter": { "type": "number", "minimum": 0, "maximum": 1, "description": "Random jitter factor (0-1) to add to delays" }, - "max_elapsed_time": { "type": "string", "description": "Maximum total time for all retry attempts (e.g., '5m', '1h')" } - }, - "required": [] - } - ] - }, + "retry": { + "$ref": "`#/definitions/workflow_retry`" + },Mirror the same change in the other schema copies. As per the PR also editing
tests/fixtures/...andwebsite/static/...copies.🤖 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/datafetcher/schema/atmos/manifest/1.0.json` around lines 1608 - 1661, The workflow step retry schema is duplicated inline even though the identical workflow_retry definition already exists, so replace the inline retry object on the workflow step with a $ref to workflow_retry to keep a single source of truth. Update the schema entry in the workflow step definition so it points to the shared workflow_retry symbol, and mirror the same ref-based change in the other schema copies under tests/fixtures and website/static to keep all generated/docs copies consistent.
🤖 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.
Nitpick comments:
In `@pkg/datafetcher/schema/atmos/manifest/1.0.json`:
- Around line 1608-1661: The workflow step retry schema is duplicated inline
even though the identical workflow_retry definition already exists, so replace
the inline retry object on the workflow step with a $ref to workflow_retry to
keep a single source of truth. Update the schema entry in the workflow step
definition so it points to the shared workflow_retry symbol, and mirror the same
ref-based change in the other schema copies under tests/fixtures and
website/static to keep all generated/docs copies consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 841c3174-d3b7-4d4f-a669-b164dd14b624
📒 Files selected for processing (7)
cmd/cmd_utils.gopkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema/config/global/1.0.jsonpkg/datafetcher/schema_condition_validation_test.gopkg/schema/task_test.gotests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.jsonwebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/schema/task_test.go
- pkg/datafetcher/schema_condition_validation_test.go
- tests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
- website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
- pkg/datafetcher/schema/config/global/1.0.json
- cmd/cmd_utils.go
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
June 24, 2026 21:09
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/hooks-trivy/README.md (1)
3-13: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winProse contradicts the new
after.terraform.planevent.Line 3 now documents an
after.terraform.planhook, but lines 12–13 still say the scan "runs before plan" (and line 29 says "trivy runs first"). Withafter.terraform.planthe scan fires after the plan is generated — still before apply, but no longer "before plan". Please align the surrounding prose so readers aren't misled about ordering.📝 Suggested wording tweak
-- Static-analysis scan runs **before plan**, so security issues surface - before any infrastructure action. +- Static-analysis scan runs **after plan but before apply**, so security + issues surface before any infrastructure changes are applied.You'll likely want to revisit line 29 ("trivy runs first") and the "Run"/"Expected" section too.
🤖 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 `@examples/hooks-trivy/README.md` around lines 3 - 13, The README prose for the trivy hook is out of sync with the new after.terraform.plan event. Update the descriptive text in the trivy hook example to say the scan runs after the plan is generated (not before plan) and revise any “trivy runs first” wording in the Run/Expected section so the ordering matches the hook behavior. Keep the explanation aligned with the trivy example’s hook kind and event names so readers understand it executes after plan but before apply.
🤖 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.
Outside diff comments:
In `@examples/hooks-trivy/README.md`:
- Around line 3-13: The README prose for the trivy hook is out of sync with the
new after.terraform.plan event. Update the descriptive text in the trivy hook
example to say the scan runs after the plan is generated (not before plan) and
revise any “trivy runs first” wording in the Run/Expected section so the
ordering matches the hook behavior. Keep the explanation aligned with the trivy
example’s hook kind and event names so readers understand it executes after plan
but before apply.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b243cb5-b79f-4f58-a04b-65f39a270d33
📒 Files selected for processing (23)
examples/hooks-checkov/README.mdexamples/hooks-checkov/stacks/deploy/test.yamlexamples/hooks-custom-command/README.mdexamples/hooks-custom-command/stacks/deploy/test.yamlexamples/hooks-infracost/README.mdexamples/hooks-infracost/components/terraform/nat-gateway/versions.tfexamples/hooks-infracost/stacks/deploy/test.yamlexamples/hooks-kics/README.mdexamples/hooks-kics/stacks/deploy/test.yamlexamples/hooks-trivy/README.mdexamples/hooks-trivy/stacks/deploy/test.yamlgists/aws-store-hooks/stacks/producer.yamlpkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema/config/global/1.0.jsonpkg/datafetcher/schema/stacks/stack-config/1.0.jsonpkg/hooks/hook_test.gotests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.jsonwebsite/docs/cli/configuration/stores.mdxwebsite/docs/migration/terragrunt.mdxwebsite/docs/stacks/hooks.mdxwebsite/docs/tutorials/sharing-state/stores.mdxwebsite/src/data/roadmap.jswebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
✅ Files skipped from review due to trivial changes (5)
- website/docs/cli/configuration/stores.mdx
- examples/hooks-kics/README.md
- examples/hooks-checkov/README.md
- examples/hooks-infracost/components/terraform/nat-gateway/versions.tf
- website/src/data/roadmap.js
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/datafetcher/schema/atmos/manifest/1.0.json
- pkg/datafetcher/schema/config/global/1.0.json
- website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
- tests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
- website/docs/stacks/hooks.mdx
- pkg/datafetcher/schema/stacks/stack-config/1.0.json
Andriy Knysh (aknysh)
approved these changes
Jun 25, 2026
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.222.0-rc.10. |
This was referenced Jun 29, 2026
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
3 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
kind: stepcomponent-lifecycle hook that delegates to the workflow/custom-command step registry, making every registered step type (container,http,toast,log,markdown, …) runnable on terraform lifecycle events — name a steptype:and pass its parameters underwith:.when: success|failure|alwaysselector (defaultsuccess) controls outcome-based firing, and{{ .status }}/{{ .exit_code }}/{{ .error }}template context plusATMOS_HOOK_*env vars (alongside component/stack) let a hook announce exactly what happened.hooksJSON schema into a structured per-hook envelope (kindenum incl.step,events,on_failure,when,type,with,retry) across all three schema copies, kept non-breaking (additionalProperties: true).with:decode,whenfiltering, outcome template/env exposure, retry, andon_failure.why
store,command,infracost,checkov,kics,trivy,git); every new capability meant a new kind. Reusing the existing, well-tested step registry lets the whole step library work as hooks without forking the abstraction.after-*hooks fired only on success (cobra skipsPostRunEon error) and the outcome reached only CI hooks, never user hooks. Firing user hooks on failure withwhen+ outcome context closes that gap while defaulting to success-only so existing hooks (e.g.store) keep their behavior.references
docs/prd/hooks-step-types.md/stacks/hooks#kind-step-run-a-step-typeand#reacting-to-success-or-failurehttpstep type used in the Slack example lands in a separate PR; the bridge works today with every registered step type.