Repository navigation
docs(dependencies): document dependencies.components for describe affected - #2391
Merged
Merged
Conversation
…ected Update docs and JSON Schema to reflect the new `dependencies.components` format used by `atmos describe affected`, `atmos describe dependents`, and CI/CD integrations: - describe-affected: lead with `dependencies.components` + `kind: file|folder` in the file/folder reference entries; convert the dependents example to the new format; keep a backward-compat note pointing to legacy `settings.depends_on`. - dependencies/components: correct the Merge Behavior section (default is replace, not append; opt into append via `settings.list_merge_strategy`); add a migration callout clarifying that `namespace`/`tenant`/`environment`/ `stage` are unsupported (use templated `stack`). - dependencies/index: rebalance the intro so `dependencies.tools` and `dependencies.components` get equal billing; relabel the duplicate Related Documentation entry as Legacy `settings.depends_on`; link to describe affected. - atmos-manifest schema (website + tests fixture): allow `dependencies.components` with `component`, `stack`, `kind`, `path` (was previously rejected by `additionalProperties: false`). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
May 5, 2026 15:35
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Erik Osterman (Cloud Posse) (osterman)
added
the
no-release
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 (3)
📝 WalkthroughWalkthroughThe PR expands the Atmos manifest ChangesDependencies schema, parsing, docs, and tests
Sequence Diagram(s)(Skipped.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/docs/stacks/dependencies/components.mdx (1)
339-344:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMerge behavior row is now inconsistent with the updated section above.
Line 342 still says
dependencies.componentsuses append merge, but Line 160 now documents default replace (append is opt-in). Please align the table row to avoid contradictory guidance.🤖 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 `@website/docs/stacks/dependencies/components.mdx` around lines 339 - 344, Update the Merge behavior cell for `dependencies.components` in the table so it matches the updated section above: change the current "Append (simple)" to reflect the new default replace behavior with append as opt-in (e.g., "Default replace (append opt-in)"). Edit the row that references `dependencies.components` (the table header uses `settings.depends_on` vs `dependencies.components`) so the Merge behavior entry is consistent with the documentation text about default replace and optional append.
🤖 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 `@website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json`:
- Around line 1565-1588: The schema currently allows empty/invalid dependency
objects; update dependencies_component_entry to enforce valid shapes by
replacing the loose properties-only definition with a oneOf containing two
variants: (1) a "component dependency" schema that requires "component"
(string), allows optional "stack" and "kind", and explicitly forbids "kind"
being "file" or "folder" (use not + enum), and disallows "path"; and (2) a "path
dependency" schema that requires "path" (string) and requires "kind" to be one
of ["file","folder"], and disallows "component" and "stack"; keep
additionalProperties: false. Use the existing symbol names
dependencies_component_entry and property names component, stack, kind, path to
locate where to apply the change.
---
Outside diff comments:
In `@website/docs/stacks/dependencies/components.mdx`:
- Around line 339-344: Update the Merge behavior cell for
`dependencies.components` in the table so it matches the updated section above:
change the current "Append (simple)" to reflect the new default replace behavior
with append as opt-in (e.g., "Default replace (append opt-in)"). Edit the row
that references `dependencies.components` (the table header uses
`settings.depends_on` vs `dependencies.components`) so the Merge behavior entry
is consistent with the documentation text about default replace and optional
append.
🪄 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: a86437e3-0866-4792-bfea-d178aa16f6e9
📒 Files selected for processing (5)
tests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.jsonwebsite/docs/cli/commands/describe/describe-affected.mdxwebsite/docs/stacks/dependencies/components.mdxwebsite/docs/stacks/dependencies/index.mdxwebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
![atmos-pro[bot]](https://avatars.githubusercontent.com/in/927493?s=60&v=4){kind=small}
Address CodeRabbit feedback: `dependencies_component_entry` previously allowed empty objects and other invalid combinations. Add an `anyOf` constraint requiring either: - `component` (component-to-component dependency), or - `kind: file|folder` + `path` (path-based dependency). Validated against 10 positive/negative cases with `jsonschema` (Draft 2020-12); matches the field semantics in `pkg/schema/dependencies.go`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2391 +/- ##
==========================================
+ Coverage 78.54% 78.58% +0.03%
==========================================
Files 1144 1144
Lines 109879 109984 +105
==========================================
+ Hits 86304 86426 +122
+ Misses 18787 18763 -24
- Partials 4788 4795 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
… keys) Reconciles design smells from the v1.210.0 dependencies.components shape without breaking existing configurations: - `name:` is now the canonical, preferred alias for `component:` inside `dependencies.components[]` entries. - `dependencies.files` and `dependencies.folders` are new sibling keys for path-based dependencies, replacing the awkward inline `kind: file` / `kind: folder` entries inside `components[]`. A new `Dependencies.Normalize()` method reconciles both surfaces into a single internal representation: `name → component` alias resolution (with conflict error), and bidirectional mirroring between the typed `Files`/`Folders` slices and `Components[]` so downstream filters keep working unchanged. JSON manifest schemas, user docs, and the PRD are updated. Legacy `kind: file/folder` and `component:` shapes still parse and behave identically; docs mark them as legacy and point to the canonical sibling keys. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
dismissed stale reviews from atmos-pro[bot] and coderabbitai[bot]
via
May 5, 2026 21:11
76608c1
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
pkg/schema/dependencies_test.go (2)
258-269: 💤 Low valueOptional: tighten this dedup test by also asserting on
Components.The current assertion only verifies
d.Filesis deduped, but the real risk isd.Componentsaccumulating duplicate(kind, path)entries — see the related comment inpkg/schema/dependencies.go. An assertion here would catch any regression in the mirror step.♻️ Suggested addition
require.NoError(t, d.Normalize()) // Files should appear exactly once in the typed view. assert.Equal(t, []string{"configs/shared.json"}, d.Files) + + // Components should also contain exactly one entry for the shared path, + // not one from the inline shape plus one mirrored from the sibling key. + fileEntries := 0 + for _, c := range d.Components { + if c.IsFileDependency() && c.Path == "configs/shared.json" { + fileEntries++ + } + } + assert.Equal(t, 1, fileEntries, "duplicate (kind,path) entries in Components") })🤖 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/dependencies_test.go` around lines 258 - 269, The test for deduping should also assert that the Components slice is deduped: after creating the Dependencies with Files and Components and calling d.Normalize(), add an assertion that d.Components equals []ComponentDependency{{Kind: "file", Path: "configs/shared.json"}} (or the canonical single entry) so the mirror step in Dependencies.Normalize() doesn't produce duplicate (Kind, Path) entries; reference the Dependencies struct, the Components field and the ComponentDependency type when updating the test.
141-294: ⚡ Quick winSuggestion: add an idempotency test for
Normalize.A round-trip test (
Normalizetwice in a row, assert state is unchanged the second time) would lock in the contract that callers can safely re-normalize after a merge. Today this would fail onComponentsduplication — see the companion comment onpkg/schema/dependencies.go.♻️ Sketch
t.Run("Normalize is idempotent", func(t *testing.T) { d := &Dependencies{ Components: []ComponentDependency{{Name: "vpc"}}, Files: []string{"configs/lambda.json"}, Folders: []string{"src/handler"}, } require.NoError(t, d.Normalize()) snapshot := *d // shallow copy of slices is fine; we compare contents below componentsBefore := append([]ComponentDependency(nil), d.Components...) filesBefore := append([]string(nil), d.Files...) foldersBefore := append([]string(nil), d.Folders...) require.NoError(t, d.Normalize()) assert.Equal(t, componentsBefore, d.Components, "Components must not change on second Normalize") assert.Equal(t, filesBefore, d.Files) assert.Equal(t, foldersBefore, d.Folders) _ = snapshot })🤖 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/dependencies_test.go` around lines 141 - 294, Add an idempotency unit test that calls Dependencies.Normalize() twice and asserts the second call does not change Dependencies state: create a Dependencies value (use Components with Name "vpc" and Files/Folders like "configs/lambda.json"/"src/handler"), call d.Normalize(), copy slices into componentsBefore/filesBefore/foldersBefore, call d.Normalize() again and assert Components, Files, and Folders equal their "before" copies; reference the Normalize method and types Dependencies and ComponentDependency in the test to locate where to add it in pkg/schema/dependencies_test.go.docs/prd/component-dependencies.md (1)
132-151: 💤 Low valueMinor: PRD merge-behavior section doesn't call out the default, unlike
components.mdx.The user-facing page (
website/docs/stacks/dependencies/components.mdxline 174) now correctly states "By default,dependencies.componentsuses replace merge behavior… To opt into append-merge behavior, setsettings.list_merge_strategy: append." This PRD section is technically correct (it qualifies append-merge as conditional on the setting) but doesn't state the default, and the worked example below showing both deps merging will read as "the way it works" to a quick reader. Worth a one-line tweak so the two docs match.♻️ Suggested wording
### Merge Behavior -`dependencies.components` uses **append merge** behavior when `list_merge_strategy: append` is configured in `atmos.yaml`. Child stacks add their dependencies to parent dependencies. +`dependencies.components` uses **replace merge** by default — a child stack's list replaces the parent's. To opt in to append-merge (child dependencies extend the parent's list), set `settings.list_merge_strategy: append` in `atmos.yaml`. + +The example below assumes `list_merge_strategy: append` is enabled.🤖 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 `@docs/prd/component-dependencies.md` around lines 132 - 151, The PRD's "Merge Behavior" section omits the default merge behavior; update the paragraph to state that dependencies.components defaults to "replace merge" and only uses append merge when settings.list_merge_strategy: append is set—mention the specific symbols dependencies.components and settings.list_merge_strategy so readers know where to change the behavior and align this doc with components.mdx.website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json (2)
1571-1604: ⚡ Quick winTighten path entries with
minLength: 1to catch empty strings at validate time.
dependencies.filesanddependencies.foldersaccept any string today, including"". The Go normalizer inpkg/schema/dependencies.gosilently skips empty paths during sibling mirroring, so the user gets no signal that their YAML is wrong (e.g., a stray template result). AminLength: 1on the items moves that error to schema validation, where it belongs.♻️ Proposed change
"dependencies_files": { … "oneOf": [ { "type": "string", "pattern": "^!include" }, { "type": "array", "items": { "type": "string", + "minLength": 1, "description": "File path (relative to the repository root). Changes to this file mark the declaring component as affected." } } ] }, "dependencies_folders": { … "oneOf": [ { "type": "string", "pattern": "^!include" }, { "type": "array", "items": { "type": "string", + "minLength": 1, "description": "Folder path (relative to the repository root). Changes to any file under this folder mark the declaring component as affected." } } ] },Apply the same to the matching fixture under
tests/fixtures/schemas/....🤖 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 `@website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json` around lines 1571 - 1604, Add "minLength: 1" to the string items for both "dependencies_files" and "dependencies_folders" schemas so empty strings are rejected at validation time; update the "items" object under each schema's array branch (the item with "type": "string") to include minLength: 1, and apply the same change to the corresponding test fixture schema(s) under tests/fixtures/schemas to ensure tests reflect the tightened validation.
1631-1646: ⚖️ Poor tradeoffSchema edge case:
anyOfpermits{ name, kind: file, path }which the Go side would mishandle.The current
anyOfstructure allows an entry to satisfy thename-required branch while also carryingkind: file+path. When both are present, the Go code'sIsFileDependency()check (Kind-based discrimination) wins: the entry is treated as a file dependency,Namegets resolved intoComponent, and theComponentends up orphaned.This pattern isn't used anywhere in existing fixtures or examples, but it could emerge from generated or templated YAML. If you want to enforce true mutual exclusion, switch to
oneOfand addnotconstraints to the component branches. Worth verifying first—it's a stricter validation that could break edge-case manifests in the wild.🤖 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 `@website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json` around lines 1631 - 1646, The anyOf validation allows entries with both "name" and "kind":"file"/"path" which the Go IsFileDependency() logic will treat as a file and orphan the Name/Component; change the schema's anyOf to oneOf and tighten branches: make the "name" branch forbid "kind" and "path" (use a "not" with required ["kind","path"] or explicit "properties" with "not"), likewise make the "component" branch forbid "kind" and "path", and keep the file/folder branch requiring "kind" and "path" with kind enum ["file","folder"]; ensure these constraints reference the same JSON object that currently uses anyOf so Name and Component cannot co-exist with kind/path.website/docs/stacks/dependencies/components.mdx (1)
134-145: 💤 Low valueMinor: align example folder paths across the page.
This block uses
src/lambda/my-functionwhile the lower "How It Works" example at line 247–248 and elsewhere on the page usessrc/lambda/handler. Picking one path makes the doc easier to scan, but no functional impact.🤖 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 `@website/docs/stacks/dependencies/components.mdx` around lines 134 - 145, The example uses inconsistent folder paths: change the folders entry that currently lists "src/lambda/my-function" to the same path used elsewhere ("src/lambda/handler") so examples are consistent; update that "folders" example (and any other occurrences of "src/lambda/my-function") to "src/lambda/handler" within the document so the "folders" example, the "How It Works" example, and other references match.tests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json (1)
1631-1645: 💤 Low valueSchema permissively allows mixing
name/componentwith legacykind: file|folder+path.The
anyOfstructure accepts an entry like{ name: vpc, kind: file, path: foo }because the first branch matches onnamealone. The normalizer promotesnametocomponentand doesn't reject the mixed style—it normalizes without validating mutual exclusivity.If the intent is to keep component-style and path-based forms distinct, consider using
oneOfwith explicit exclusion constraints in the schema, or add runtime validation innormalizeComponentEntries()to reject mixed entries. Since backward compatibility is a stated goal (legacy path-based entries are supported), the current permissive approach aligns with that intent, though it risks accidental mixing.🤖 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 `@tests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json` around lines 1631 - 1645, The schema's anyOf allows mixed-style entries (e.g., {name, kind, path}) which slips through normalization; either make the schema enforce exclusivity by replacing the anyOf with oneOf and explicit exclusion clauses for the branches (ensure branches: {required: ["name"], not: {required: ["kind","path"]}}, {required: ["component"], not: {required: ["kind","path"]}}, and the legacy branch {required: ["kind","path"], properties.kind.enum contains ["file","folder"]}) or add runtime validation in normalizeComponentEntries() to detect and reject objects that contain both component-style keys ("name" or "component") and legacy keys ("kind" or "path") before normalizing.
🤖 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 `@internal/exec/describe_affected_components.go`:
- Around line 544-559: Replace the duplicate fast-path check in
describe_affected_components.go with a call to the existing hasDependencyEntries
helper (from describe_dependents.go) to keep logic centralized, then when
calling deps.Normalize() on the decoded deps variable, change the log.Debug call
to log.Warn and include the returned error in the warning message so Normalize
failures are surfaced (e.g., "failed to normalize dependencies section" + err)
rather than silently dropping file/folder deps.
In `@internal/exec/describe_dependents.go`:
- Around line 471-481: The Normalize error on deps.Normalize() is currently
debug-logged and swallowed, causing silent partial normalization; change
getComponentDependencies to return an error (update its signature and all
callers) and, when deps.Normalize() returns normErr, return that error (or at
minimum log it at Error/Warn level) instead of just log.Debug so misconfigured
stacks fail loudly; ensure callers handle the propagated error and that the
behavior references deps.Normalize() and getComponentDependencies.
In `@pkg/schema/dependencies.go`:
- Around line 133-176: Normalize is not idempotent because
mirrorSiblingsIntoComponents blindly appends synthetic ComponentDependency
entries for Files/Folders causing duplicates; modify
mirrorSiblingsIntoComponents to first build a set/map of existing (kind, path)
pairs from d.Components and only append a new ComponentDependency{Kind:
dependencyKindFile / dependencyKindFolder, Path: p} when that (kind,path) is not
already present, marking it in the map as you append; keep using the same
symbols (Normalize, mirrorSiblingsIntoComponents, d.Components, d.Files,
d.Folders, ComponentDependency, dependencyKindFile, dependencyKindFolder) so
repeated Normalize calls do not produce duplicate entries.
In `@website/docs/stacks/dependencies/components.mdx`:
- Around line 210-223: The tip currently lists valid list_merge_strategy values
but omits "append"; update the tip text in components.mdx to list "replace"
(default), "append", and "merge" so it matches the settings reference and
example; specifically edit the tip under the example that mentions Other valid
`list_merge_strategy` values to include `append`, keeping the reference link to
[`settings` reference] intact and ensuring `list_merge_strategy` is spelled
exactly as shown in the diff.
In `@website/docs/stacks/dependencies/index.mdx`:
- Line 14: The sentence incorrectly groups `components`, `files`, and `folders`
as all defining ordering; update the paragraph so it clearly states that `tools`
pins CLI versions and `components` controls ordering between Atmos components,
while `files` and `folders` are only watch-paths consumed by `atmos describe
affected` and CI/CD integrations (they do not affect execution order); use the
keys `tools`, `components`, `files`, and `folders` in the revised text to make
roles explicit.
- Around line 264-266: Update the documentation note "Legacy inline shape" to
remove or correct the incorrect release reference to v1.210.0: either delete the
"shipped in v1.210.0" clause or replace it with the accurate release/planned
version and optionally cite the correct commit/PR, and make sure the text still
points readers to the newer keys `dependencies.files` and `dependencies.folders`
as replacements for the inline `dependencies.components[]` `kind: file` / `kind:
folder` shape (refer to the block labeled "Legacy inline shape" and the
`dependencies.components[]` wording when applying the change).
---
Nitpick comments:
In `@docs/prd/component-dependencies.md`:
- Around line 132-151: The PRD's "Merge Behavior" section omits the default
merge behavior; update the paragraph to state that dependencies.components
defaults to "replace merge" and only uses append merge when
settings.list_merge_strategy: append is set—mention the specific symbols
dependencies.components and settings.list_merge_strategy so readers know where
to change the behavior and align this doc with components.mdx.
In `@pkg/schema/dependencies_test.go`:
- Around line 258-269: The test for deduping should also assert that the
Components slice is deduped: after creating the Dependencies with Files and
Components and calling d.Normalize(), add an assertion that d.Components equals
[]ComponentDependency{{Kind: "file", Path: "configs/shared.json"}} (or the
canonical single entry) so the mirror step in Dependencies.Normalize() doesn't
produce duplicate (Kind, Path) entries; reference the Dependencies struct, the
Components field and the ComponentDependency type when updating the test.
- Around line 141-294: Add an idempotency unit test that calls
Dependencies.Normalize() twice and asserts the second call does not change
Dependencies state: create a Dependencies value (use Components with Name "vpc"
and Files/Folders like "configs/lambda.json"/"src/handler"), call d.Normalize(),
copy slices into componentsBefore/filesBefore/foldersBefore, call d.Normalize()
again and assert Components, Files, and Folders equal their "before" copies;
reference the Normalize method and types Dependencies and ComponentDependency in
the test to locate where to add it in pkg/schema/dependencies_test.go.
In `@tests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json`:
- Around line 1631-1645: The schema's anyOf allows mixed-style entries (e.g.,
{name, kind, path}) which slips through normalization; either make the schema
enforce exclusivity by replacing the anyOf with oneOf and explicit exclusion
clauses for the branches (ensure branches: {required: ["name"], not: {required:
["kind","path"]}}, {required: ["component"], not: {required: ["kind","path"]}},
and the legacy branch {required: ["kind","path"], properties.kind.enum contains
["file","folder"]}) or add runtime validation in normalizeComponentEntries() to
detect and reject objects that contain both component-style keys ("name" or
"component") and legacy keys ("kind" or "path") before normalizing.
In `@website/docs/stacks/dependencies/components.mdx`:
- Around line 134-145: The example uses inconsistent folder paths: change the
folders entry that currently lists "src/lambda/my-function" to the same path
used elsewhere ("src/lambda/handler") so examples are consistent; update that
"folders" example (and any other occurrences of "src/lambda/my-function") to
"src/lambda/handler" within the document so the "folders" example, the "How It
Works" example, and other references match.
In `@website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json`:
- Around line 1571-1604: Add "minLength: 1" to the string items for both
"dependencies_files" and "dependencies_folders" schemas so empty strings are
rejected at validation time; update the "items" object under each schema's array
branch (the item with "type": "string") to include minLength: 1, and apply the
same change to the corresponding test fixture schema(s) under
tests/fixtures/schemas to ensure tests reflect the tightened validation.
- Around line 1631-1646: The anyOf validation allows entries with both "name"
and "kind":"file"/"path" which the Go IsFileDependency() logic will treat as a
file and orphan the Name/Component; change the schema's anyOf to oneOf and
tighten branches: make the "name" branch forbid "kind" and "path" (use a "not"
with required ["kind","path"] or explicit "properties" with "not"), likewise
make the "component" branch forbid "kind" and "path", and keep the file/folder
branch requiring "kind" and "path" with kind enum ["file","folder"]; ensure
these constraints reference the same JSON object that currently uses anyOf so
Name and Component cannot co-exist with kind/path.
🪄 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: ed08c609-ea1e-4fbd-b7ef-37362754e536
📒 Files selected for processing (11)
docs/prd/component-dependencies.mdinternal/exec/describe_affected_components.gointernal/exec/describe_affected_utils_2_test.gointernal/exec/describe_dependents.gopkg/schema/dependencies.gopkg/schema/dependencies_test.gotests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.jsonwebsite/docs/cli/commands/describe/describe-affected.mdxwebsite/docs/stacks/dependencies/components.mdxwebsite/docs/stacks/dependencies/index.mdxwebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
🚧 Files skipped from review as they are similar to previous changes (1)
- website/docs/cli/commands/describe/describe-affected.mdx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/exec/describe_affected_components.go (1)
549-559: ⚡ Quick winDecode-error branch should log too, mirroring the Normalize branch.
Same shape concern as in
describe_dependents.go: amapstructure.Decodefailure silently returnsnil, which in this hot path means all file/folder deps for the component get dropped without a peep — only theNormalizefailure now warns. Adding alog.Warnon the decode error path keeps both sibling failures visible at the same level.♻️ Suggested log on decode failure
var deps schema.Dependencies - if err := mapstructure.Decode(depsSection, &deps); err != nil { - return nil - } + if err := mapstructure.Decode(depsSection, &deps); err != nil { + log.Warn("invalid dependencies section; file/folder deps may be silently ignored", "error", err) + return nil + } if err := deps.Normalize(); err != nil { log.Warn("invalid dependencies section; file/folder deps may be silently ignored", "error", err) return 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 `@internal/exec/describe_affected_components.go` around lines 549 - 559, The mapstructure.Decode failure for depsSection currently returns nil silently; add a warning log similar to the Normalize branch so decode errors are visible. In the block where mapstructure.Decode(depsSection, &deps) returns err, call log.Warn with a descriptive message (e.g., "invalid dependencies section; decode failed") and include the error (err) as a field, then continue to return nil as before; keep the existing deps.Normalize check and its log.Warn intact. Ensure you reference mapstructure.Decode, schema.Dependencies/deps, depsSection, and log.Warn when making the change.pkg/schema/dependencies_test.go (1)
141-306: 💤 Low valueComprehensive coverage of the new normalization paths.
Good split between name-alias scenarios and the files/folders mirroring scenarios, and the idempotence test (lines 295–305) directly nails the duplication concern from the earlier review round. One small follow-up if you want to widen the safety net later: the idempotence test only re-checks
d.Components; consider also assertingd.Files/d.Foldersare stable across the secondNormalizecall so a future regression inbackfillTypedSlicesFromInlinewould also fail.♻️ Optional tweak to also assert typed slices stay stable
t.Run("is idempotent — calling twice produces the same result", func(t *testing.T) { d := &Dependencies{ Components: []ComponentDependency{{Component: "vpc"}}, Files: []string{"configs/lambda.json"}, Folders: []string{"src/handler"}, } require.NoError(t, d.Normalize()) first := append([]ComponentDependency(nil), d.Components...) + firstFiles := append([]string(nil), d.Files...) + firstFolders := append([]string(nil), d.Folders...) require.NoError(t, d.Normalize()) assert.Equal(t, first, d.Components, "second Normalize must not append duplicates") + assert.Equal(t, firstFiles, d.Files, "Files must be stable across Normalize calls") + assert.Equal(t, firstFolders, d.Folders, "Folders must be stable across Normalize calls") })🤖 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/dependencies_test.go` around lines 141 - 306, The idempotence subtest in TestDependencies_Normalize_FilesFoldersSiblings only checks that d.Components is unchanged after a second call to Normalize; extend the test to also capture and assert that d.Files and d.Folders remain identical before and after the second Normalize to guard against regressions in backfillTypedSlicesFromInline (use the same pattern used for Components: copy the slices into local variables, call d.Normalize() again, and assert equality against the originals).internal/exec/describe_dependents.go (1)
468-481: ⚡ Quick winDecode error path is silently swallowed.
If
mapstructure.Decode(depsSection, &deps)fails (e.g., a user typesdependencies.components: "vpc"as a string instead of a list), theerr == nilguard silently falls through to the legacysettings.depends_onpath with no signal to the user. The companionNormalizefailure now warns — this branch should match for consistency and so config typos in the new format don't masquerade as "no dependencies declared".♻️ Suggested log on decode failure
if hasDependencyEntries(depsSection) { var deps schema.Dependencies - if err := mapstructure.Decode(depsSection, &deps); err == nil { + if err := mapstructure.Decode(depsSection, &deps); err != nil { + log.Warn("invalid dependencies section; entries may be silently ignored", "error", err) + } else { if normErr := deps.Normalize(); normErr != nil { log.Warn("invalid dependencies section; entries may be silently ignored", "error", normErr) } if len(deps.Components) > 0 { return deps.Components, settingsSection, dependencySourceDependenciesComponents } } }🤖 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/describe_dependents.go` around lines 468 - 481, The current branch ignores errors from mapstructure.Decode when parsing depsSection, allowing malformed new-format dependencies to silently fall through to the legacy settings.depends_on path; update the block that calls mapstructure.Decode(depsSection, &deps) (in the code handling componentMap[cfg.DependenciesSectionName]) so that on a non-nil err you log a clear warning/error (including the decode error) and return/stop processing this branch (so it does not fall back to settings.depends_on), keeping the existing Normalize() warning behavior for normalization errors; reference symbols: componentMap, cfg.DependenciesSectionName, hasDependencyEntries, mapstructure.Decode, deps (schema.Dependencies), deps.Normalize, dependencySourceDependenciesComponents, settingsSection.internal/exec/describe_affected_utils_2_test.go (1)
617-713: 💤 Low valueSolid v2 surface coverage with the right equivalence and dedup checks.
The v1↔v2 equivalence subtest plus the dedup-by-path subtest are exactly what this surface needed. Optional follow-up if/when you have spare cycles: a precedence test where the v2 sibling keys (
files/folders) coexist with a populatedsettings.depends_onwould close the symmetry with the existing "prefers dependencies.components over settings.depends_on" case (line 572).🤖 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/describe_affected_utils_2_test.go` around lines 617 - 713, Add a new unit test that verifies precedence when v2 sibling keys ("files"/"folders") coexist with a populated settings.depends_on: construct a componentSection (or component map) that includes both "dependencies": {"files": [...], "folders": [...]} and a "settings": {"depends_on": [...] } entry (where depends_on contains entries that would also map to file/folder deps), call getFileFolderDependencies and assert that only the v2 sibling keys produce file/folder dependencies (i.e., settings.depends_on is ignored for file/folder resolution); use the same assertion patterns as nearby tests (require.Len/assert.ElementsMatch/assert.Equal) and reference getFileFolderDependencies, "files"/"folders", and "settings.depends_on" to locate the correct behavior to test.
🤖 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 `@website/docs/stacks/dependencies/index.mdx`:
- Line 239: The example in the dependencies.components block uses the old key
"name" (line showing "- name: vpc") which doesn't match the v2 manifest shape;
update the example to use "component: vpc" and ensure any sibling examples
follow the v2 keys ("component", "stack", "kind", "path") so the docs reflect
the new schema used by dependencies.components.
---
Nitpick comments:
In `@internal/exec/describe_affected_components.go`:
- Around line 549-559: The mapstructure.Decode failure for depsSection currently
returns nil silently; add a warning log similar to the Normalize branch so
decode errors are visible. In the block where mapstructure.Decode(depsSection,
&deps) returns err, call log.Warn with a descriptive message (e.g., "invalid
dependencies section; decode failed") and include the error (err) as a field,
then continue to return nil as before; keep the existing deps.Normalize check
and its log.Warn intact. Ensure you reference mapstructure.Decode,
schema.Dependencies/deps, depsSection, and log.Warn when making the change.
In `@internal/exec/describe_affected_utils_2_test.go`:
- Around line 617-713: Add a new unit test that verifies precedence when v2
sibling keys ("files"/"folders") coexist with a populated settings.depends_on:
construct a componentSection (or component map) that includes both
"dependencies": {"files": [...], "folders": [...]} and a "settings":
{"depends_on": [...] } entry (where depends_on contains entries that would also
map to file/folder deps), call getFileFolderDependencies and assert that only
the v2 sibling keys produce file/folder dependencies (i.e., settings.depends_on
is ignored for file/folder resolution); use the same assertion patterns as
nearby tests (require.Len/assert.ElementsMatch/assert.Equal) and reference
getFileFolderDependencies, "files"/"folders", and "settings.depends_on" to
locate the correct behavior to test.
In `@internal/exec/describe_dependents.go`:
- Around line 468-481: The current branch ignores errors from
mapstructure.Decode when parsing depsSection, allowing malformed new-format
dependencies to silently fall through to the legacy settings.depends_on path;
update the block that calls mapstructure.Decode(depsSection, &deps) (in the code
handling componentMap[cfg.DependenciesSectionName]) so that on a non-nil err you
log a clear warning/error (including the decode error) and return/stop
processing this branch (so it does not fall back to settings.depends_on),
keeping the existing Normalize() warning behavior for normalization errors;
reference symbols: componentMap, cfg.DependenciesSectionName,
hasDependencyEntries, mapstructure.Decode, deps (schema.Dependencies),
deps.Normalize, dependencySourceDependenciesComponents, settingsSection.
In `@pkg/schema/dependencies_test.go`:
- Around line 141-306: The idempotence subtest in
TestDependencies_Normalize_FilesFoldersSiblings only checks that d.Components is
unchanged after a second call to Normalize; extend the test to also capture and
assert that d.Files and d.Folders remain identical before and after the second
Normalize to guard against regressions in backfillTypedSlicesFromInline (use the
same pattern used for Components: copy the slices into local variables, call
d.Normalize() again, and assert equality against the originals).
🪄 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: d72602b2-5ebe-444e-b6db-eb8881408fe3
📒 Files selected for processing (7)
internal/exec/describe_affected_components.gointernal/exec/describe_affected_utils_2_test.gointernal/exec/describe_dependents.gopkg/schema/dependencies.gopkg/schema/dependencies_test.gowebsite/docs/stacks/dependencies/components.mdxwebsite/docs/stacks/dependencies/index.mdx
Erik Osterman (Cloud Posse) (osterman)
dismissed stale reviews from coderabbitai[bot] and atmos-pro[bot]
via
May 9, 2026 12:31
f1806d8
Erik Osterman (Cloud Posse) (osterman)
dismissed stale reviews from coderabbitai[bot] and atmos-pro[bot]
via
May 27, 2026 00:42
4996d95
Andriy Knysh (aknysh)
approved these changes
May 27, 2026
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.220.0-rc.4. |
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
atmos describe affecteddocs to lead with the newdependencies.components(kind: file|folder+path:) format for path-based dependencies; keep a short backward-compat note pointing to legacysettings.depends_on.describe affectedpage fromsettings.depends_ontodependencies.components.stacks/dependencies/components.mdx: the default is replace, not append; append requires opting in viasettings.list_merge_strategy: append. Add an "Opt-in append" subsection and link to thesettingsreference.stacks/dependencies/components.mdxclarifying thatnamespace/tenant/environment/stageare not supported in the new format — use a templatedstack:instead.stacks/dependencies/index.mdxsodependencies.toolsanddependencies.componentsget equal billing in the intro, use cases, and component-dependencies subsection. Remove the duplicate Related Documentation entry and relabel the legacy link as "Legacysettings.depends_on". Add a link toatmos describe affected.website/static/schemas/...and the matching test fixture) sodependencies.componentsis allowed and validatescomponent,stack,kind,path. Previously rejected byadditionalProperties: false.why
dependencies.componentspage existed, but the highest-traffic surface (describe affected) and the JSON Schema still only documented/allowed the legacysettings.depends_onmap — driving users to the deprecated format and making the new format fail IDE/SchemaStore validation.stacks/dependencies/components.mdxcontradicted the announcement blog and the actual code (internal/exec/describe_dependents_test.go:1093–1154confirms default = replace, append is opt-in viasettings.list_merge_strategy). Users following the docs would have built a wrong mental model of inheritance.settings.depends_on(withnamespace/tenant/environment/stage) todependencies.components(with templatedstack:) was only discoverable via the migration table at the bottom of the page; surfacing it as a callout reduces confusion for users porting existing configs.references
pkg/schema/dependencies.go— canonical field set fordependencies.componentsinternal/exec/describe_dependents_test.go:1093–1154— confirms default merge is replace, append requiressettings.list_merge_strategy: appendwebsite/blog/2026-03-14-dependencies-components.mdx— original announcementcd website && npm run buildsucceeds (no broken-link errors)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests