feat(stoneintg-1350): add support for nested componentGroups

[14rcole]

Allows users to add a ComponentGroup to another ComponentGroup's
spec.Components field. The resulting data structure is a DAG in which
all leaf nodes are Components and all non-leaves are ComponentGroups.

When a Component is built, its ComponentGroups will be gathered and
snapshots will be created for them. The Snapshot will contain the GCL
and updated images for that ComponentGroup and all ComponentGroups
nested within it. After that snapshot is created the snapshot controller
will create snapshots for all ComponentGroups that contain the first
ComponentGroup.

This allows Konflux to support complex use-cases for ComponentGroups in
which users have multi-tiered applications with subgroups that must be
tested independently

• Commit messages are descriptive enough (hints)
• Code coverage from testing does not decrease and new code is covered (check the PR coverage on codecov)
• Controllers diagrams are updated when PR changes controllers code (if applicable)

[gemini-code-assist]

Code Review

This pull request introduces support for nested ComponentGroups and the automatic creation of parent snapshots. It updates the ComponentGroup API to allow referencing other ComponentGroups, removes the deprecated Dependents field, and implements the EnsureParentSnapshotsExist method in the snapshot adapter along with new loader methods. The feedback highlights several important improvements: refactoring the status condition helpers to update the snapshot in-memory to avoid redundant API calls and double-patching, avoiding retrying on permanent semantic errors, returning slices directly instead of pointers to slices in the loader interface, and adding a nil check to prevent a potential runtime panic when dereferencing nested component groups.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[fullsend-ai-review]

Review

Findings

High

• [consumer-completeness] internal/controller/componentgroup/componentgroup_adapter.go:92 — alignGCLWithSpecComponents iterates over all Spec.Components without filtering out entries where Kind is componentGroup. With the new nesting feature, Spec.Components can contain references to nested ComponentGroups, which have no ComponentVersion set (zero value). This creates spurious GCL entries keyed as childGroupName/ (empty version string), polluting the GlobalCandidateList.
  Remediation: Add a check at the top of the loop body to skip entries where strings.EqualFold(component.Kind, "componentGroup").

• [consumer-completeness] internal/controller/snapshot/snapshot_adapter.go:1023 — prepareGroupSnapshot iterates over a.componentGroup.Spec.Components without filtering out entries where Kind is componentGroup. For such entries, the code will call FindMatchingSnapshotComponent and FetchSnapshotComponentFromGCL using the ComponentGroup's name as if it were a Component name, causing incorrect snapshot composition.
  Remediation: Add if strings.EqualFold(groupComponent.Kind, "componentGroup") { continue } at the start of the loop body.

• [missing-doc] docs/snapshot-controller.md — The snapshot controller Mermaid diagram does not include the new EnsureParentSnapshotsExist adapter function added to the reconciliation loop in snapshot_controller.go. This is a significant new controller operation that should be documented.
  Remediation: Add EnsureParentSnapshotsExist to the flowchart with its flow (check condition → fetch parent CGs → prepare parent snapshots → set condition).

• [breaking-api] api/v1beta2/componentgroup_types.go:47 — Dependents []string field removed from ComponentGroupSpec. Any external consumers writing this field will have data silently ignored after upgrade. While this is a v1beta2 API, the removal should be coordinated.
  Remediation: Verify no other repos use spec.dependents. Provide migration documentation for converting Dependents references to nested ComponentReferences with kind: componentGroup.

• [breaking-api] api/v1beta2/componentgroup_types.go:72 — ComponentVersion changed from +required to +optional. Existing validation in consuming code may assume ComponentVersion is always non-zero.
  Remediation: Ensure clients handle ComponentReferences where componentVersion may be absent (when kind: componentGroup).

• [breaking-api] loader/loader.go:86 — Three new methods added to ObjectLoader interface: GetComponentGroupsContainingComponentGroup, GetAllComponentGroupsInNamespace, GetNestedComponentGroupsForComponentGroup. Any external implementations of this interface will fail to compile.
  Remediation: Coordinate with repos that implement ObjectLoader.

• [breaking-api] snapshot/create.go — Multiple exported function signatures changed: PrepareSnapshotForPipelineRun (new loader param), PrepareSnapshot (new loader param, logr.Logger → helpers.IntegrationLogger), GetSnapshotComponentsFromGCL (returns map instead of slice), FetchSnapshotComponentFromGCL (new componentVersion param, map types). While these are likely internal to this repo, any external Go module importing the snapshot package will break.
  Remediation: Verify no external repos import these packages. If they do, coordinate updates.

Medium

• [consumer-completeness] internal/controller/buildpipeline/buildpipeline_adapter.go:766 — The loop iterating over componentGroup.Spec.Components to annotate snapshots on PR group failure does not skip componentGroup-kind entries, issuing unnecessary API calls with incorrect component names.
  Remediation: Add a kind check to skip componentGroup entries.

• [architectural-coherence] api/v1beta2/componentgroup_types.go — Breaking API change removes Dependents field without documented migration path or deprecation strategy. CLAUDE.md and project docs do not mention ComponentGroup nesting or the design rationale for this model change.
  Remediation: Document design rationale and provide migration strategy.

• [missing-doc] docs/snapshot-controller.md — ParentSnapshotsCreatedCondition is a new status condition not documented anywhere.
  Remediation: Add documentation for the new condition.

Low

• [retry-behavior] internal/controller/snapshot/snapshot_adapter.go:164 — retry.OnError uses func(_ error) bool { return true } as retry predicate, retrying ALL errors including non-transient ones. This follows a pre-existing pattern in the codebase but is worth noting for future improvement.

• [denial-of-service] snapshot/create.go — Multi-hop cycles that pass the webhook are caught at runtime by getAllNestedSnapshotComponents, which returns an error immediately. The controller requeues with exponential backoff, bounding the impact, but webhook-time validation would prevent the error path entirely.

• [cross-tenant-isolation] loader/loader.go — GetNestedComponentGroupsForComponentGroup resolves nested references by bare name within the same namespace. In Konflux's namespace-per-tenant model this is safe, but the assumption should be documented.

• [edge-case] snapshot/components.go:48 — GetAllNewComponentsInSnapshot returns empty slice (not error) when snapshot has no recognized type label. In PrepareParentSnapshot, this means a parent snapshot could be created purely from GCL data without any newly-built component.

• [edge-case] internal/webhook/v1beta2/componentgroup_webhook.go:115 — Webhook validateSpecComponents only detects self-referencing cycles. Larger cycles are deferred to runtime detection (documented in code).

• [logic-error] snapshot/create.go:242 — In PrepareParentSnapshot, the git source annotation is set when len(newSnapshotComponents) == 1 regardless of snapshot type. The code itself has a NOTE comment questioning this behavior for override snapshots.

• [doc-style] snapshot/create.go — PrepareParentSnapshot godoc says "creates a snapshot" but the Prepare* pattern in this file means "returns without creating in cluster".

• [naming-convention] snapshot/create.go — Comment uses "dict" (Python terminology) instead of "map" (Go terminology).

• [naming-convention] snapshot/components.go:92 — Function comment has typo: "GetAllnewComponentsInSnapshot" should be "GetAllNewComponentsInSnapshot".

• [code-organization] snapshot/create.go:44 — MaxDepth constant lacks godoc documentation.

• [error-handling-idiom] snapshot/gcl.go:167 — FetchSnapshotComponentFromGCL takes componentVersion parameter but doesn't include it in error messages for debuggability.

• [missing-doc] README.md — Snapshot Controller section doesn't mention parent snapshot creation for nested ComponentGroups.

Info

• [input-validation] config/crd/bases/appstudio.redhat.com_componentgroups.yaml — The CRD validation pattern for kind field uses (?i) inline flag. This is valid RE2/Go regexp syntax and works correctly.

• [architectural-coherence] snapshot/create.go:1482 — GCL merging semantics (parent overrides child on duplicate ComponentVersion keys) documented in code comment but not in formal docs.

• [test-coverage] — No e2e tests for full DAG traversal (component build → child snapshot → parent snapshot chain). Unit tests cover individual functions well.

• [missing-authorization] — No linked GitHub issue. PR title references Jira ticket STONEINTG-1350. The project convention uses Jira for tracking; a public GitHub issue would improve traceability.

Previous run

Review

Findings

Medium

• [CRD Breaking Change] api/v1beta2/componentgroup_types.go:41 — ComponentReference.ComponentVersion changed from +required to +optional. While this is a v1beta2 (beta) API where breaking changes are acceptable, this is a backward-incompatible CRD schema change. Existing clients expecting ComponentVersion to always be present should be considered.
  Remediation: Ensure migration path is documented. Consider conversion webhook if existing resources rely on this field always being present.

• [CRD Breaking Change - Field Removal] api/v1beta2/componentgroup_types.go:44 — The Dependents field has been removed from ComponentGroupSpec. Existing ComponentGroup resources with the dependents field set will have that data silently ignored. The replacement (nested ComponentGroups via Kind=componentGroup in spec.components) is the purpose of this PR, but no migration guide is provided.
  Remediation: Provide a migration guide for users currently using the Dependents field explaining how to achieve equivalent functionality with nested ComponentGroups.

• [logic error] helpers/component_group.go:33 — GetComponentNamesFromComponentGroup iterates all Spec.Components and returns their names without filtering by Kind. After this PR, Spec.Components can contain entries of kind componentGroup. Callers in buildpipeline_adapter.go and snapshot_adapter.go use the result to filter PipelineRuns by component label. ComponentGroup-kind entries would add their names to the filter list, potentially matching unrelated PipelineRuns.
  Remediation: Update GetComponentNamesFromComponentGroup to skip entries where strings.EqualFold(component.Kind, "componentGroup") is true.

• [logic error] internal/controller/snapshot/snapshot_adapter.go:149 — In the loop over parentCGs, when PrepareParentSnapshot or CreateSnapshotWithCollisionHandling fails for one parent, the function immediately returns RequeueWithError. The deferred patch then fires and persists partial status. If the deferred patch failed on a prior run, those entries were lost and the function could re-create already-created parent snapshots since there is no cluster-level idempotency guard.
  Remediation: Before creating a parent snapshot, check whether a snapshot owned by the parentComponentGroup with the ChildSnapshotAnnotation matching a.snapshot.Name already exists in the cluster.

• [consumer completeness] snapshot/create.go:230 — PrepareParentSnapshot sets SnapshotGitSourceRepoURLAnnotation inside a loop over newSnapshotComponents. Each iteration overwrites the annotation. For group/override snapshots with multiple components from different repos, only the final component's URL survives.
  Remediation: Either skip setting the annotation for parent snapshots with multiple new components, or collect all URLs into a structured annotation.

• [Incomplete Cycle Detection / DoS] internal/webhook/v1beta2/componentgroup_webhook.go:70 — The validating webhook only detects self-referencing cycles (component.Name == componentGroupName). Two ComponentGroups created concurrently can form a mutual cycle that the webhook misses. The runtime cycle detection in getAllNestedSnapshotComponents catches this and returns an error, but EnsureParentSnapshotsExist will RequeueWithError indefinitely, permanently consuming controller reconciliation capacity.
  Remediation: Set a terminal (non-retriable) condition on the snapshot when a cycle is detected, instead of requeueing forever. Alternatively, walk the graph at webhook time with a depth limit.

Low

• [test adequacy] internal/controller/snapshot/snapshot_adapter_test.go:597 — Tests only cover a single parent ComponentGroup scenario. No test for multiple parents or partial-failure (where one parent snapshot succeeds but a subsequent one fails).
• [missing-documentation] config/samples/appstudio_v1beta2_componentgroup.yaml — The sample demonstrates nested ComponentGroups but no user-facing documentation explains this new capability.
• [missing-documentation] api/v1beta2/componentgroup_types.go — The Kind field and ComponentVersion optionality changes are documented inline in CRD comments, but no external user documentation covers these API changes.
• [comment-style] snapshot/create.go:1656 — Comment for upsertMultipleComponentImages uses Python terminology "dict" instead of Go's "map".

Info

• [missing-diagram-update] docs/snapshot-controller.md — The snapshot controller Mermaid flowchart does not include the newly added EnsureParentSnapshotsExist function.

Previous run (2)

Review

Findings

Medium

• [logic-error] snapshot/gcl.go:167 — In FetchSnapshotComponentFromGCL, the invalid-component fallback checks only invalidComponent.Name == componentName without also matching the version. When the same component name exists with multiple versions (one invalid, one valid), a lookup for the valid version will miss the map entry (keyed by name/version), then the name-only fallback will match the invalid entry and incorrectly return an error for the valid version.
  Remediation: Also compare invalidComponent.Version against componentVersion in the invalid-component loop.

• [logic-error] snapshot/create.go:268 — In getAllNestedSnapshotComponents, the usedComponentGroups slice is passed by value to recursive calls, but Go slices share their underlying array when capacity permits. In a diamond-shaped DAG (Root → A, B; A → C; B → C), sibling branches can corrupt each other's visited-set, potentially causing false-positive cycle detection errors.
  Remediation: Before recursing, copy the slice into a new backing array: visited := make([]string, len(usedComponentGroups)); copy(visited, usedComponentGroups). Or switch to a map[string]bool for the visited set.

• [denial-of-service] snapshot/create.go — Webhook validation (validateSpecComponents) only catches direct self-references (A→A). Larger cycles (A→B→A) can form via TOCTOU race between concurrent creates/updates. Runtime detection with MaxDepth=15 mitigates stack overflow, but cycles cause persistent reconciliation errors and requeues.
  Remediation: Consider querying existing ComponentGroups in the webhook's ValidateCreate/ValidateUpdate to detect multi-hop cycles at admission time.

• [stale-doc] config/samples/appstudio_v1beta2_componentgroup.yaml — The sample file is being updated in this PR but may have residual inconsistencies between the componentBranch and componentVersion patterns used across entries.
  Remediation: Ensure the sample consistently demonstrates the new nested ComponentGroup pattern.

Low

• [edge-case] internal/controller/snapshot/snapshot_adapter.go:1023 — In prepareGroupSnapshot, the loop over Spec.Components does not skip Kind=componentGroup entries, leading to unnecessary API calls and misleading log messages for ComponentGroup references.
  Remediation: Add a guard at the top of the loop: if strings.EqualFold(groupComponent.Kind, "componentGroup") { continue }.

• [off-by-one] snapshot/create.go:1492 — The depth check if depth > MaxDepth allows processing at depth 15 and rejects at 16. Actual max nesting is 16 levels, not 15 as the error message claims.
  Remediation: Change to if depth >= MaxDepth.

• [comment-spacing] api/v1beta2/componentgroup_types.go:30 — Double space in comment: "Kind is the type of resource being referenced". This also propagates to the CRD YAML description.
  Remediation: Remove extra space.

• [naming-convention] snapshot/components.go:91 — Function comment says GetAllnewComponentsInSnapshot but function is named GetAllNewComponentsInSnapshot (capital N).
  Remediation: Fix comment to match function name.

• [code-organization] snapshot/create.go:1451 — Comment "NOTE: should we skip this annotation for override snaphots?" contains a typo and is an unresolved question in production code.
  Remediation: Resolve the question or convert to a formal TODO with ticket reference.

• [doc-style] snapshot/create.go:1656 — Comment uses "dict" instead of Go-idiomatic "map".
  Remediation: Change "dict" to "map".

• [missing-doc] docs/snapshot-controller.md — Snapshot controller docs don't include the new EnsureParentSnapshotsExist operation added to the reconciliation loop.
  Remediation: Add section documenting EnsureParentSnapshotsExist in the controller workflow.

Info

• [error-handling-idiom] loader/loader.go:883 — Error format uses %+v instead of %w for error wrapping in GetNestedComponentGroupsForComponentGroup, losing the error chain for errors.Is/errors.As checks.
  Remediation: Use fmt.Errorf with %w.

Previous run (3)

Review

Findings

High

• [breaking-api] api/v1beta2/componentgroup_types.go:44 — The Dependents field is removed from ComponentGroupSpec and ComponentVersion changed from +required to +optional with a new Kind field added. This is a breaking CRD schema change affecting any external systems that read/write ComponentGroup CRs. Existing CRs using spec.dependents must be migrated to the nested spec.components[].kind: componentGroup pattern.
  Remediation: Provide migration documentation and ensure external consumers are aware of the schema changes before merging.

• [breaking-api] loader/loader.go:43 — ObjectLoader interface extended with 3 new required methods (GetComponentGroupsContainingComponentGroup, GetAllComponentGroupsInNamespace, GetNestedComponentGroupsForComponentGroup). Any external implementations will fail to compile.
  Remediation: External implementations must implement the new methods.

Medium

• [logic-error] internal/controller/snapshot/snapshot_adapter.go:1023 — prepareGroupSnapshot iterates over all Spec.Components without filtering out Kind=componentGroup entries. For CG entries, groupComponent.ComponentVersion.Name is empty, producing broken lookup keys in FetchSnapshotComponentFromGCL, unnecessary API calls, and misleading log messages.
  Remediation: Add if strings.EqualFold(groupComponent.Kind, "componentgroup") { continue } at the top of the loop.

• [consumer-completeness] helpers/component_group.go:33 — GetComponentNamesFromComponentGroup returns names of ALL entries in Spec.Components including Kind=componentGroup entries. Callers use this list to filter PipelineRuns by component name, which is semantically incorrect for CG entries.
  Remediation: Filter out entries with Kind=componentGroup (case-insensitive).

• [consumer-completeness] snapshot/validation.go:40 — ValidateOverrideSnapshotComponents builds knownComponents from all Spec.Components including CG entries. CG entries produce keys with empty version that never match, causing valid override components in nested CGs to be incorrectly rejected.
  Remediation: Skip Kind=componentGroup entries when building the knownComponents set.

• [logic-error] snapshot/create.go — In PrepareParentSnapshot, SnapshotGitSourceRepoURLAnnotation is set inside a loop over newSnapshotComponents. For group snapshot children with multiple components from different repos, only the last component's URL is retained.
  Remediation: Only set the annotation if there is exactly one new component, or document best-effort behavior.

• [denial-of-service] internal/webhook/v1beta2/componentgroup_webhook.go:751 — Webhook only prevents self-referencing cycles (A→A). Multi-hop cycles (A→B→A) are deferred to runtime. Users can create cyclic ComponentGroups that only fail during snapshot creation. Runtime cycle detection with MaxDepth=15 prevents actual DoS, but webhook-time validation would provide better UX.
  Remediation: Consider adding multi-hop cycle detection in the validating webhook.

• [error-handling-gap] internal/controller/snapshot/snapshot_adapter.go:140 — In EnsureParentSnapshotsExist, deferred status patch swallows errors after retries. If parent snapshot creation succeeds but the status patch fails, parent snapshots may be created again on re-reconciliation.

• [missing-authorization] — No linked GitHub issue for a 1500+ line non-trivial feature. CONTRIBUTING.md states Jira references cannot replace proper descriptions. The Jira ticket STONEINTG-1350 is referenced in the commit scope but is not publicly accessible.
  Remediation: Create a public GitHub issue describing the feature and link it to this PR.

• [design-direction] api/v1beta2/componentgroup_types.go:44 — Removing Dependents and replacing with nested containment is an architectural pivot from a push to pull model. No ADR or design document is referenced in the PR.
  Remediation: Document the architectural decision and provide migration guidance.

• [scope-exceeded] config/samples/appstudio_v1beta2_componentgroup.yaml — The sample file is the only documentation of the migration from Dependents to nested ComponentGroups. No migration guide or backward compatibility statement is provided.

• [validation-error-format] internal/webhook/v1beta2/componentgroup_webhook.go:743 — validateSpecComponents uses fmt.Errorf() for validation errors. Structured field errors (field.Invalid()) would provide better UX with proper field paths.

• [error-handling-idiom] snapshot/create.go — Multiple error messages use %+v instead of %w for error wrapping. Codebase convention is %w for proper error chain propagation.

• [missing-doc] docs/snapshot-controller.md — The flowchart does not include the new EnsureParentSnapshotsExist reconciliation step added to the snapshot controller.

• [breaking-api] snapshot/create.go — PrepareSnapshotForPipelineRun, PrepareSnapshot, GetSnapshotComponentsFromGCL, and FetchSnapshotComponentFromGCL all have changed signatures or return types.

Low

• [denial-of-service] snapshot/create.go:245 — getAllNestedSnapshotComponents bounds depth at 15 but not total nodes visited. Width explosion is bounded in practice by namespace quotas and RBAC.

• [input-validation] internal/webhook/v1beta2/componentgroup_webhook.go:33 — CRD regex pattern (?i) flag is standard Go regex and supported by Kubernetes CRD validation. Runtime code uses strings.EqualFold consistently.

• [doc-style] snapshot/components.go:80 — Comment says GetAllnewComponentsInSnapshot (lowercase n) but function is GetAllNewComponentsInSnapshot.

• [scope-creep] vendor/ — Application-api version bump not explained in PR body.

• [design-direction] AGENTS.md — Does not mention ComponentGroups can reference other ComponentGroups.

• [missing-doc] README.md — Does not mention nested ComponentGroup functionality.

• [edge-case] snapshot/components.go:47 — GetAllNewComponentsInSnapshot returns empty components without warning if GroupSnapshotInfoAnnotation is missing from a group snapshot.

Previous run (4)

Review

Findings

Medium

• [logic-error] internal/controller/snapshot/snapshot_adapter.go:1023 — prepareGroupSnapshot iterates over componentGroup.Spec.Components which now includes entries with Kind="componentGroup". The loop does not skip these entries. When it reaches FetchSnapshotComponentFromGCL, componentGroup-kind entries have empty ComponentVersion, producing a lookup key like childGroupName/ which won't match anything. Returns nil, nil and logs misleading "no GCL entries found". If a ComponentGroup name collides with a component name in invalidComponents, it could incorrectly return an error.
  Remediation: Add a filter at the top of the loop body to skip entries where strings.EqualFold(groupComponent.Kind, "componentgroup"), consistent with how getSpecComponentsAndVersionsMap and GetComponentGroupsForComponentVersion already skip such entries.

• [scope-exceeded] api/v1beta2/componentgroup_types.go — The PR removes the Dependents field (an existing feature for parent-to-child relationships) while adding the nested ComponentGroup model. The title says "add support for nested componentGroups" but also removes functionality. This constitutes scope beyond pure addition and should be communicated.
  Remediation: Clarify in the PR description whether Dependents was deprecated, explain why the relationship direction was inverted (explicit nesting vs. dependents list), and document a migration path for any existing users of the Dependents field.

• [stale-doc] docs/snapshot-controller.md — The snapshot controller flowchart does not show the new EnsureParentSnapshotsExist operation added to the reconciliation loop or the ParentSnapshotsCreated condition.
  Remediation: Update the flowchart to include EnsureParentSnapshotsExist after EnsureIntegrationPipelineRunsExist, documenting when it runs, what it does, and the condition it sets.

Low

• [edge-case] internal/webhook/v1beta2/componentgroup_webhook.go:136 — validateSpecComponents only detects direct self-references. Two-hop cycles (A→B, B→A) pass webhook validation. Runtime cycle detection in getNestedSnapshotComponents handles this with clear error messages including the cycle path, but earlier detection at the webhook would provide better UX.

• [error-handling] internal/controller/snapshot/snapshot_adapter.go:453 — retry.OnError uses func(_ error) bool { return true } as predicate, retrying ALL errors including non-transient ones. Consider restricting to transient errors.

• [edge-case] snapshot/create.go — flattenSnapshotComponentsMap iterates over a Go map with non-deterministic order. Snapshot component ordering varies between runs, which could make snapshots harder to compare and debug.

• [resource-exhaustion] loader/loader.go:1091 — GetAllComponentGroupsInNamespace lists ALL ComponentGroups without pagination. This pattern pre-exists in the codebase but is now called from additional paths.

• [error-handling-idiom] loader/loader.go:865 — GetNestedComponentGroupsForComponentGroup uses %+v instead of %w for error wrapping, breaking the error chain for errors.Is/errors.As.

• [stale-doc] README.md — README describes snapshot creation behavior but doesn't mention parent snapshot creation for nested ComponentGroups.

Info

• [design-direction] internal/controller/snapshot/snapshot_adapter.go — EnsureParentSnapshotsExist introduces a bottom-up snapshot propagation pattern (child triggers parent) that is not yet documented in architecture docs.

• [missing-doc] No migration guide for users moving from the Dependents field to nested ComponentGroups via the Kind field.

Previous run (5)

Review

Findings

Medium

• [CRD API Breaking Change] api/v1beta2/componentgroup_types.go:44 — The Dependents field is removed from ComponentGroupSpec. This is a breaking change to the CRD schema for the v1beta2 API. While beta APIs allow breaking changes per Kubernetes conventions, verify no production users rely on this field.
  Remediation: Consider a deprecation period or verify no production users rely on this field.

• [error handling / retry logic] internal/controller/snapshot/snapshot_adapter.go:161 — The retry.OnError wrapping CreateSnapshotWithCollisionHandling uses func(_ error) bool { return true }, retrying on ALL errors including permanent ones. CreateSnapshotWithCollisionHandling already handles AlreadyExists collisions internally.
  Remediation: Change the retry predicate to only retry on transient/retryable errors, or remove the outer retry.

• [Architectural Coherence] api/v1beta2/componentgroup_types.go:30 — The PR introduces a design shift from push-based dependents to pull-based nested containment without documenting the architectural rationale. The PR body explains the model but no ADR or design doc captures the decision.
  Remediation: Document the architectural decision in an ADR or AGENTS.md.

• [Documentation Coherence] docs/snapshot-controller.md — The snapshot controller documentation does not reflect the new EnsureParentSnapshotsExist adapter method, the ParentSnapshotsCreatedCondition, or the nested ComponentGroup model.
  Remediation: Update docs/snapshot-controller.md and AGENTS.md to document nested ComponentGroups, parent snapshot creation, and the new condition.

• [error-formatting] snapshot/create.go — Multiple uses of fmt.Errorf with %+v instead of %w for error wrapping in getNestedSnapshotComponents and PrepareParentSnapshot, which prevents errors.Is/errors.As from working correctly.
  Remediation: Change %+v to %w for proper error wrapping.

• [Vendored Dependency Breaking Change] vendor/.../snapshot_types.go — New ParentSnapshots field in SnapshotStatus changes the Snapshot CRD contract. While additive and backward-compatible, services that marshal/unmarshal Snapshot status strictly should be updated.
  Remediation: Ensure all services in the Konflux ecosystem update to a compatible application-api version.

Low

• [race condition / idempotency] internal/controller/snapshot/snapshot_adapter.go:130 — On re-reconcile after partial failure, duplicate parent snapshots could theoretically be created if the deferred status patch fails. The code comments explicitly acknowledge this tradeoff.

• [edge case / missing type handling] snapshot/components.go:30 — GetAllNewComponentsInSnapshot returns an empty slice for unrecognized snapshot types. Could silently produce parent snapshots without child component overrides if a new snapshot type is added.

• [webhook validation gap] internal/webhook/v1beta2/componentgroup_webhook.go:111 — The webhook only detects direct self-reference cycles. Multi-node cycles are caught at runtime by getNestedSnapshotComponents.

• [missing separator] snapshot/utils.go:48 — joinInvalidComponentNamesAndVersions concatenates entries without separators.

• [DAG diamond duplication] snapshot/create.go:165 — usedComponentGroups is passed by value, causing redundant processing of shared nodes in diamond DAG patterns.

• [Information Disclosure] internal/controller/snapshot/snapshot_adapter.go:156 — Raw error messages stored in user-visible Snapshot status.

• [CRD API Change] api/v1beta2/componentgroup_types.go:41 — ComponentVersion changed from required to optional. Backward compatible for existing CRs.

• [CRD API Extension] api/v1beta2/componentgroup_types.go:32 — New Kind field with default component ensures backward compatibility.

• [naming-consistency] loader/loader.go:813 — Doc comments on new functions don't follow established patterns.

• [comment-style] snapshot/create.go — Missing/informal doc comments on new functions.

Info

• [trust-model] snapshot/create.go:216 — Child snapshot components override parent GCL entries by design. Access control is via namespace-level RBAC.

• [input-validation] api/v1beta2/componentgroup_types.go:62 — Kind field uses case-insensitive regex with consistent EqualFold comparisons. No impact.

Previous run (6)

Review

Findings

High

• [breaking-api] api/v1beta2/componentgroup_types.go:44 — The Dependents field ([]string) has been removed from ComponentGroupSpec without a deprecation path. This is a breaking change to the v1beta2 CRD. While the field was never functionally implemented and the v1beta2 API is a beta API where breaking changes are expected, existing ComponentGroups with this field set will lose that data. The feature is being replaced by nested ComponentGroups before Dependents was ever used in production.
  Remediation: Provide a deprecation strategy or document the breaking change explicitly. If ComponentGroups are not yet in production use, add a note in the PR description and release notes confirming this is safe.

Medium

• [api-contract] snapshot/create.go — The function getSnapshotComponentsFromGCL changed return types from ([]SnapshotComponent, []ComponentState) to (map[string]SnapshotComponent, map[ComponentState]InvalidComponentReason). Verify that all callers in snapshot_adapter.go (e.g., prepareGroupSnapshot) are updated for the new signature. The diff is internally consistent but callers outside the diff need verification.
  Remediation: Verify all callers of getSnapshotComponentsFromGCL / GetSnapshotComponentsFromGCL are updated.

• [design-direction] snapshot/create.go — The original architecture used a push model (Dependents field: parent declares children) but this PR implements a pull model (nested ComponentGroups: children referenced by parents via spec.Components). This architectural change is not documented in an ADR or design doc.
  Remediation: Document the architectural decision to use nested ComponentGroups instead of Dependents in an ADR or design doc.

• [stale-doc] docs/snapshot-controller.md — The PR adds EnsureParentSnapshotsExist() to the snapshot controller reconciliation chain (line 186 of snapshot_controller.go) but the Mermaid diagram in snapshot-controller.md is not updated. The new ParentSnapshotsCreatedCondition status condition is also not documented.
  Remediation: Add EnsureParentSnapshotsExist() to the Mermaid flowchart and document the ParentSnapshotsCreatedCondition.

• [edge-case] internal/controller/snapshot/snapshot_adapter.go — In EnsureParentSnapshotsExist, retry.OnError uses func(_ error) bool { return true } which retries ALL errors including permanent ones (validation failures, forbidden, etc.), adding unnecessary latency and API load for non-transient errors.
  Remediation: Use a selective retry predicate: func(err error) bool { return k8sErrors.IsConflict(err) || k8sErrors.IsServerTimeout(err) }.

• [edge-case] snapshot/components.go — GetAllNewComponentsInSnapshot silently returns an empty slice for unrecognized snapshot types (snapshots with no type label or unknown types). In PrepareParentSnapshot, this means the parent snapshot would contain only GCL data with no child updates, testing stale images.
  Remediation: Return an error or log a warning when the snapshot type is unrecognized.

• [logic-error] snapshot/create.go — Using v1beta2.ComponentState as a map key for invalidComponents relies on Go struct comparability. ComponentState contains a *metav1.Time pointer field (LastPromotedBuildTime), meaning two entries with the same name/version but different pointer addresses create separate map entries. Also, joinInvalidComponentNamesAndVersions iterates this map non-deterministically, potentially causing flapping annotation values.
  Remediation: Use a string key (e.g., name/version) for the invalidComponents map, similar to how snapshotComponents uses helpers.GetComponentVersionString.

• [edge-case] internal/webhook/v1beta2/componentgroup_webhook.go:733 — Webhook only validates self-referencing cycles (A contains A). Multi-hop cycles (A→B→A) are deferred to runtime detection via getNestedSnapshotComponents. TOCTOU race: two concurrent updates could create a cycle that neither individual validation catches. The runtime detection prevents infinite recursion but doesn't prevent the invalid state from being persisted.
  Remediation: Consider cross-ComponentGroup cycle detection in ValidateUpdate by listing referenced ComponentGroups transitively.

• [code-organization] snapshot/components.go:1 — New file is missing the standard Apache 2.0 copyright header that all other Go files in this codebase have (companion test file components_test.go has it).
  Remediation: Add the standard copyright header.

Low

• [breaking-api] api/v1beta2/componentgroup_types.go:72 — ComponentVersion changed from +required to +optional. The webhook validateSpecComponents() correctly enforces that component-kind entries must have ComponentVersion set, so validation is not actually broken. Minor schema documentation mismatch.

• [breaking-api] loader/loader.go:85 — Four new methods added to the ObjectLoader interface. This is an internal interface (implemented only by loader and mockLoader within this repository), so external breakage is not a concern.

• [missing-doc] api/v1beta2/componentgroup_types.go — The new Kind field and nested ComponentGroup feature have no dedicated user-facing documentation, though the CRD schema descriptions and the updated config/samples/appstudio_v1beta2_componentgroup.yaml provide basic discoverability.

• [edge-case] internal/controller/snapshot/snapshot_adapter.go — If the deferred status patch fails, parent snapshots may be created again on requeue. The code explicitly acknowledges and documents this trade-off.

• [naming-convention] snapshot/components.go:15 — Godoc comment has typo GetAllnewComponentsInSnapshot (lowercase 'n') that doesn't match function name. Also has snaphot typo on line 57.

• [input-validation] api/v1beta2/componentgroup_types.go:66 — CRD validation pattern (?i)^(component|componentGroup)$ uses (?i) inline flag. Code uses strings.EqualFold consistently as a safety net, so this is low risk.

Info

• [missing-authorization] The PR references STONEINTG-1350 (Jira ticket) in the title, which provides authorization tracing. No GitHub issue is linked, which is a process difference rather than a defect.

Previous run (7)

Review

Findings

Medium

• [edge-case] internal/webhook/v1beta2/componentgroup_webhook.go:101 — The validateSpecComponents webhook only prevents a ComponentGroup from containing itself (component.Name == componentGroupName) but does not detect mutual cycles (A→B→A). Two concurrent CG create/update operations can race past the webhook and form a cycle. The runtime cycle detection in getNestedSnapshotComponents (visited-set + depth limit of 15) prevents infinite loops, but users will encounter runtime errors instead of clean admission-time validation. The code acknowledges this with a comment: "Cycle detection since validation webhook is at risk of a race condition."
  Remediation: Consider querying existing ComponentGroups during webhook validation to detect transitive cycles, or document the runtime-only cycle detection as a known limitation.

• [edge-case] internal/controller/snapshot/snapshot_adapter.go:155 — EnsureParentSnapshotsExist uses retry.OnError with a blanket retry predicate (func(_ error) bool { return true }) for CreateSnapshotWithCollisionHandling. This retries ALL errors including non-transient ones (validation errors, permission errors, quota exceeded), masking the distinction between transient and permanent failures.
  Remediation: Replace the blanket predicate with one that only retries transient errors (conflicts, timeouts), similar to how retry.RetryOnConflict is used elsewhere in the same function.

• [stale-doc] docs/snapshot-controller.md:1 — The snapshot controller Mermaid flowchart documents all reconciliation steps but does not include the newly added EnsureParentSnapshotsExist. A reader consulting this doc would not know the snapshot controller now creates parent snapshots for nested ComponentGroups.
  Remediation: Add a new flowchart section for EnsureParentSnapshotsExist covering guard conditions, parent CG discovery, and snapshot creation.

Low

• [code-organization] snapshot/components.go:18 — GetAllNewComponentsInSnapshot has non-idiomatic Go parameter ordering: ctx context.Context appears after *Snapshot. Go convention (and this codebase) places ctx first. Same issue in getNestedSnapshotComponents in snapshot/create.go.
  Remediation: Reorder parameters so ctx context.Context is first.

• [edge-case] snapshot/utils.go:51 — joinInvalidComponentNamesAndVersions iterates over a map, producing non-deterministic output ordering in error messages and annotations.
  Remediation: Sort map keys before iterating.

Info

• [breaking-api] config/crd/bases/appstudio.redhat.com_componentgroups.yaml — Breaking CRD schema changes: (1) dependents field removed — existing CRs with this field will have data silently dropped; (2) componentVersion changed from required to optional. These are deliberate design changes. Ensure migration documentation or release notes cover these for users with existing ComponentGroups.

Previous run (8)

Review

Findings

High

• [logic-error] internal/controller/componentgroup/componentgroup_adapter.go:92 — alignGCLWithSpecComponents iterates all Spec.Components and accesses component.ComponentVersion.Name without checking component.Kind. With this PR, Spec.Components can now contain entries of kind componentGroup which have no ComponentVersion set (zero-value empty string). This will create spurious GCL entries with empty version strings for nested ComponentGroups. The Kind guard was added in getSpecComponentsAndVersionsMap (snapshot/create.go) but not here.
  Remediation: Add if strings.EqualFold(component.Kind, "componentGroup") { continue } at the top of the loop body in alignGCLWithSpecComponents.

• [logic-error] snapshot/components.go:23 — GetAllNewComponentsInSnapshot returns an empty slice for group snapshots (the IsGroupSnapshot branch has a TODO and no implementation). This function is called by PrepareParentSnapshot to determine which components from the child snapshot should override GCL entries in the parent. For group snapshots, no child components will be upserted into the parent snapshot, producing a parent snapshot with only stale GCL data.
  Remediation: Implement the group snapshot branch, or add a guard in EnsureParentSnapshotsExist to skip parent snapshot creation for group snapshots until PR feat(STONEINTG-1519): create PR group snapshots from ComponentGroups #1549 is merged.

Medium

• [logic-error] snapshot/validation.go:41 — ValidateOverrideSnapshotComponents builds a knownComponents set from componentGroup.Spec.Components by accessing component.ComponentVersion.Name for every entry without checking Kind. After this PR, entries of kind componentGroup will produce keys with an empty version, polluting the set.
  Remediation: Add a Kind check to skip componentGroup entries when building the knownComponents map.

• [logic-error] loader/loader.go:236 — GetComponentGroupsForComponentVersion iterates all Spec.Components and accesses ref.ComponentVersion.Name without checking Kind. ComponentGroup entries have empty ComponentVersion.Name, causing unnecessary comparisons.
  Remediation: Add if strings.EqualFold(ref.Kind, "componentGroup") { continue } before the name/version comparison.

• [unauthorized-change] api/v1beta2/componentgroup_types.go:8 — The PR removes the Dependents field from ComponentGroupSpec without mentioning this breaking API change in the PR description. While this is a v1beta2 API and the nested ComponentGroups mechanism replaces Dependents, the breaking change should be explicitly documented.
  Remediation: Update PR description to clearly state this is a breaking change replacing the Dependents mechanism.

• [stale-doc] docs/snapshot-controller.md — The snapshot controller's Mermaid flow diagram does not document the new EnsureParentSnapshotsExist() function added to the reconcile loop. The PR checklist notes that controller diagrams should be updated.
  Remediation: Add a new section to the Mermaid diagram documenting EnsureParentSnapshotsExist().

• [webhook-validation-gap] internal/webhook/v1beta2/componentgroup_webhook.go:73 — The webhook validateSpecComponents only checks direct self-reference (A→A) but not deeper cycles (A→B→A). While runtime cycle detection exists in getNestedSnapshotComponents with MaxDepth=15, the invalid cyclic state persists in the cluster and causes repeated reconciliation failures until manually fixed.
  Remediation: Add best-effort deeper cycle check in the webhook, or document that indirect cycles are detected at runtime.

• [error-handling-gap] internal/controller/snapshot/snapshot_adapter.go:101 — In EnsureParentSnapshotsExist, the deferred status patch logs an error but does not propagate it. If the patch fails, ContinueProcessing() is returned and the ParentSnapshotsCreatedCondition is never persisted, potentially causing duplicate parent snapshot creation on next reconciliation.
  Remediation: Return the patch error so the reconciler requeues with an error.

• [design-smell] snapshot/create.go:43 — MaxDepth=15 is a hardcoded constant with no documentation explaining the choice. This differs from the DAG validation approach in pkg/dag/verify.go which uses Kahn's algorithm.
  Remediation: Document the design decision for MaxDepth=15.

• [design-smell] snapshot/components.go:40 — GetAllNewComponentsInSnapshot has a TODO for GroupSnapshots depending on PR feat(STONEINTG-1519): create PR group snapshots from ComponentGroups #1549. The function is used in the critical path (PrepareParentSnapshot) but returns an empty slice for GroupSnapshots.
  Remediation: Complete the implementation or block parent snapshot creation for GroupSnapshots. See also: [logic-error] finding at this location.

• [godoc-convention] snapshot/components.go:25 — Godoc comment says GetAllnewComponentsInSnapshot (lowercase 'n') but the function is GetAllNewComponentsInSnapshot (uppercase 'N').
  Remediation: Fix the godoc to match the function name.

Low

• [scope-exceeded] api/v1beta2/componentgroup_types.go — The PR bundles multiple architectural changes (polymorphic Kind field, ComponentVersion required→optional, Dependents removal, new controller operation, recursive DAG traversal, application-api dependency update). All are cohesive parts of the nested ComponentGroups feature.

• [error-handling-gap] internal/controller/snapshot/snapshot_adapter.go:148 — retry.OnError with func(_ error) bool { return true } retries all errors including permanent ones. This matches existing codebase patterns and DefaultRetry limits exposure.

• [test-inadequate] snapshot/components_test.go — No test for the group snapshot case in GetAllNewComponentsInSnapshot. Expected since the implementation is a TODO.

• [missing-authorization] PR references STONEINTG-1350 Jira ticket but no accessible issue URL is provided.

• [godoc-completeness] loader/loader.go — New exported functions GetComponentGroupsContainingComponentGroup, GetAllComponentGroupsInNamespace, GetNestedComponentGroupsForComponentGroup lack proper godoc comments.

• [godoc-completeness] gitops/snapshot.go — New exported functions ParentSnapshotsCreated, SetParentSnapshotsCreatedCondition, AddParentSnapshotDataToSnapshotStatus lack godoc comments.

• [error-handling-idiom] snapshot/create.go — Error wrapping uses %+v instead of %w in getNestedSnapshotComponents and PrepareParentSnapshot, breaking the error chain for errors.Is/errors.As.

• [edge-case] snapshot/create.go — The invalidComponents map uses v1beta2.ComponentState as a key, which contains pointer fields (LastPromotedBuildTime). Works now but is fragile if key construction changes.

Previous run (9)

Review

Findings

High

• [logic-error] snapshot/components.go:23 — GetAllNewComponentsInSnapshot has a TODO stub for group snapshots that returns an empty slice. When a group snapshot triggers EnsureParentSnapshotsExist, PrepareParentSnapshot calls this function and receives no components to upsert. Parent snapshots created from group child snapshots will contain only GCL images without the newly-built images, producing incorrect snapshots.
  Remediation: Implement the group snapshot branch before merging, or guard EnsureParentSnapshotsExist to skip group snapshots until the implementation is complete.

• [stale-reference] config/samples/appstudio_v1beta2_componentgroup.yaml:34 — The sample YAML still contains the dependents field (lines 34–37), but the PR removes Dependents from the CRD schema and Go types. The sample is now inconsistent with the API and would either fail CRD validation or be silently pruned.
  Remediation: Remove the dependents: section from the sample. The nested relationship is already demonstrated by the new kind: componentGroup entry on line 33.

Medium

• [edge-case] internal/controller/snapshot/snapshot_adapter.go:145 — Failed parent snapshot creation is never retried. When PrepareParentSnapshot or CreateSnapshotWithCollisionHandling fails, an entry with Created: false is written to snapshot.Status.ParentSnapshots. On the next reconcile, the if _, ok := a.snapshot.Status.ParentSnapshots[name]; ok { continue } check skips all existing entries regardless of their Created status, so failed parent snapshots are permanently stuck.
  Remediation: Change the skip condition to also check Created == true: if data, ok := ...; ok && data.Created { continue }.

• [missing-test] internal/webhook/v1beta2/componentgroup_webhook.go:70 — The validating webhook checks that componentGroup-kind entries don't set ComponentVersion, but performs no cycle detection — not even basic self-reference validation. A ComponentGroup can reference itself or form cycles through concurrent creates/updates. While runtime cycle detection exists in getNestedSnapshotComponents, the error only surfaces at snapshot creation time rather than at admission, causing repeated reconciliation work until MaxDepth is hit.
  Remediation: At minimum, reject self-references in the webhook (a CG referencing itself by name). Full DAG validation is acknowledged as race-prone but self-reference is safe to check.

• [api-contract] config/crd/bases/appstudio.redhat.com_componentgroups.yaml:90 — The CRD schema still marks componentVersion as a required field on ComponentReference, but componentGroup-kind entries should not have it set. Users creating componentGroup-kind entries must provide a zero-value componentVersion: {name: ""} to satisfy the schema, which is confusing. The sample omits componentVersion on the child-cg entry, which would fail CRD validation.
  Remediation: Make componentVersion optional in the CRD (remove +required marker or change to +optional) and enforce its presence for component-kind entries via the webhook.

• [stale-doc] docs/snapshot-controller.md — The snapshot controller flowchart does not document the new EnsureParentSnapshotsExist operation added to the reconcile chain. This operation creates snapshots for parent ComponentGroups and introduces the ParentSnapshotsCreatedCondition status condition — a significant addition to the controller lifecycle.
  Remediation: Update the mermaid flowchart to include the new operation and its interaction with the ParentSnapshotsCreated condition.

• [logic-error] snapshot/create.go:208 — In PrepareParentSnapshot, the loop over newSnapshotComponents overwrites SnapshotGitSourceRepoURLAnnotation on each iteration. For override snapshots (which return all components), only the last component's git source URL is preserved.
  Remediation: Set the annotation only once (e.g., from the first component with a valid git source), or document that this annotation is intentionally single-valued.

Low

• [edge-case] snapshot/create.go:170 — flattenSnapshotComponentsMap iterates over a Go map, producing non-deterministic component ordering in snapshots. Two reconciles for the same input may produce snapshots with components in different orders, which can cause spurious diffs in downstream comparison logic.
  Remediation: Sort the output slice by component name+version before returning.

• [pattern-violation] internal/controller/snapshot/snapshot_controller.go:190 — AdapterInterface is not updated to include EnsureParentSnapshotsExist, leaving the documentation interface inconsistent with the actual reconcile handler.
  Remediation: Add EnsureParentSnapshotsExist() (controller.OperationResult, error) to the interface.

Info

• [missing-authorization] — This ~1540-line feature PR references STONEINTG-1350 (Jira) but has no linked GitHub issue. Per CONTRIBUTING.md, non-trivial contributions require early communication and descriptions cannot be replaced by referencing private Jira links. The Jira scope prefix provides traceability but not public authorization context.

• [incomplete-doc] snapshot/components.go:23 — The group snapshot branch contains a TODO referencing PR feat(STONEINTG-1519): create PR group snapshots from ComponentGroups #1549 without specifying the repository or explaining what functionality is needed. Clarify the dependency and whether this PR can be merged independently.

Previous run (10)

Review

Findings

High

• [logic-error] snapshot/create.go:288 — In getNestedSnapshotComponents, the depth parameter is never incremented in the recursive call. The call passes depth unchanged (getNestedSnapshotComponents(&nestedComponentGroup, usedComponentGroups, depth, ...)), making the depth > MaxDepth guard dead code. While cycle detection via usedComponentGroups catches true cycles, a chain of >15 unique ComponentGroups would recurse without limit.
  Remediation: Change the recursive call to pass depth+1 instead of depth.

• [logic-error] snapshot/components.go:21 — GetAllNewComponentsInSnapshot returns an empty slice for group snapshots (the IsGroupSnapshot branch contains only a TODO comment). When EnsureParentSnapshotsExist creates grandparent snapshots from a group snapshot, GetAllNewComponentsInSnapshot returns [], so upsertMultipleComponentImages inserts nothing. Parent snapshots in cascade scenarios will contain only stale GCL images, not the newly-built images.
  Remediation: Implement the group snapshot branch before merging, or guard EnsureParentSnapshotsExist to skip group snapshots and document the limitation.

• [edge-case] internal/controller/snapshot/snapshot_adapter.go:110 — EnsureParentSnapshotsExist does not check snapshot.Status.ParentSnapshots[parentCG.Name].Created before attempting to create each parent snapshot. On requeue after partial failure, snapshots already created for earlier parent ComponentGroups are re-attempted. Since PrepareParentSnapshot generates a new name each time, CreateSnapshotWithCollisionHandling succeeds and creates duplicates.
  Remediation: At the top of the loop, check a.snapshot.Status.ParentSnapshots[parentCG.Name].Created == true and skip already-created parents to make the operation idempotent.

• [breaking-api] api/v1beta2/componentgroup_types.go — The Dependents []string field is removed from ComponentGroupSpec without a deprecation period or migration path. Existing ComponentGroup resources using this field will lose data. The sample file config/samples/appstudio_v1beta2_componentgroup.yaml still references dependents in comments (lines 34-37), creating user confusion.
  Remediation: Either provide a migration guide documenting how the nested ComponentGroup model replaces Dependents, or retain the field as deprecated during a transition period. Remove stale dependents references from sample files.

Medium

• [race-condition] internal/controller/snapshot/snapshot_adapter.go:108 — The deferred status patch uses a MergeFrom patch computed at function entry. If another controller modifies the snapshot between DeepCopy() and the deferred patch, the merge may conflict or silently lose data. The retry predicate func(_ error) bool { return true } retries all errors including conflicts, but the in-memory snapshot is stale after a conflict so retries will keep failing or overwrite concurrent changes.
  Remediation: Use a more targeted retry predicate (e.g., only IsConflict) and re-fetch the snapshot on conflict before re-patching.

• [error-handling-idiom] loader/loader.go:800 — Error wrapping uses %+v instead of %w, breaking the error chain for errors.Is and errors.As checks: fmt.Errorf("Could not get componentGroup %s nested in componentGroup %s: %+v", ...).
  Remediation: Change %+v to %w and fix capitalization to lowercase: fmt.Errorf("failed to get componentGroup %s nested in componentGroup %s: %w", ...).

• [stale-doc] docs/snapshot-controller.md — The snapshot controller now includes EnsureParentSnapshotsExist in the reconciliation operations list, but the controller documentation does not describe this new operation or its flow.
  Remediation: Add a section documenting EnsureParentSnapshotsExist, including when it runs, what it does, and how errors are handled.

• [error-handling-idiom] snapshot/create.go — Multiple error messages use title case ("Error getting nested snapshot components", "Cycle found in nested componentGroups") inconsistent with codebase convention of lowercase error messages.
  Remediation: Use lowercase: "error getting nested snapshot components: %w", "cycle found in nested componentGroups: %+v".

• [logging-pattern] internal/controller/snapshot/snapshot_adapter.go:131 — Log statement a.logger.Info("Created parent snapshots for snapshot", a.snapshot) passes the snapshot object directly instead of using structured key-value pairs.
  Remediation: Change to a.logger.Info("Created parent snapshots for snapshot", "snapshot", a.snapshot.Name).

Low

• [documentation-comment-format] snapshot/components.go:9 — Function comment has typo: GetAllnewComponentsInSnapshot should be GetAllNewComponentsInSnapshot (capital N).

• [naming-convention] snapshot/utils.go:31 — New exported type InvalidComponentReason lacks a doc comment. Codebase convention requires doc comments on all exported types.

• [error-message-format] internal/controller/snapshot/snapshot_adapter.go:115,124 — Error messages "Could not prepare new snapshot" and "Could not create snapshot in cluster" use title case; convention is lowercase ("failed to ...")

[fullsend-ai-review]

See the review comment for full details.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 58.69565% with 114 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.05%. Comparing base (40bb4d9) to head (cf4db38).

Files with missing lines	Patch %	Lines
snapshot/create.go	68.31%	21 Missing and 11 partials ⚠️
internal/controller/snapshot/snapshot_adapter.go	55.10%	17 Missing and 5 partials ⚠️
gitops/snapshot.go	0.00%	16 Missing ⚠️
internal/webhook/v1beta2/componentgroup_webhook.go	26.31%	10 Missing and 4 partials ⚠️
loader/loader.go	70.58%	8 Missing and 2 partials ⚠️
snapshot/components.go	76.66%	4 Missing and 3 partials ⚠️
snapshot/gcl.go	0.00%	7 Missing ⚠️
loader/loader_mock.go	60.00%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1587      +/-   ##
==========================================
- Coverage   73.18%   73.05%   -0.13%     
==========================================
  Files          64       65       +1     
  Lines        8708     8923     +215     
==========================================
+ Hits         6373     6519     +146     
- Misses       1684     1734      +50     
- Partials      651      670      +19     

Flag	Coverage Δ
e2e-tests	46.61% <1.44%> (-0.86%)	⬇️
unit-tests	65.78% <58.69%> (-0.12%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
api/v1beta2/componentgroup_types.go	100.00% <ø> (ø)
.../controller/buildpipeline/buildpipeline_adapter.go	72.05% <100.00%> (ø)
...nternal/controller/snapshot/snapshot_controller.go	63.63% <100.00%> (-2.42%)	⬇️
snapshot/utils.go	92.59% <100.00%> (ø)
loader/loader_mock.go	43.05% <60.00%> (+1.26%)	⬆️
snapshot/components.go	76.66% <76.66%> (ø)
snapshot/gcl.go	46.59% <0.00%> (-0.54%)	⬇️
loader/loader.go	71.00% <70.58%> (+1.36%)	⬆️
internal/webhook/v1beta2/componentgroup_webhook.go	60.00% <26.31%> (-20.65%)	⬇️
gitops/snapshot.go	78.45% <0.00%> (-1.40%)	⬇️
... and 2 more

... and 6 files with indirect coverage changes

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40bb4d9...cf4db38. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Review · Started 4:52 PM UTC
Commit: dbae38e · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:04 PM UTC · Completed 5:18 PM UTC
Commit: dbae38e · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[jsztuka]

lgtm

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:04 AM UTC · Completed 11:19 AM UTC
Commit: ffde3b2 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:44 PM UTC · Completed 7:01 PM UTC
Commit: 218f229 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[dirgim]

LGTM

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:31 PM UTC · Completed 1:47 PM UTC
Commit: 218f229 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 3:13 PM UTC · Ended 3:25 PM UTC
Commit: 218f229 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:28 PM UTC · Completed 4:02 PM UTC
Commit: 218f229 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:29 PM UTC · Completed 8:47 PM UTC
Commit: 218f229 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[14rcole]

/retest

[14rcole]

/retest

[kasemAlem]

/retest

[dirgim]

/retest

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 1:42 PM UTC · Completed 1:52 PM UTC
Commit: 7acff03 · View workflow run →

[fullsend-ai-retro]

Retro: PR #1587 — feat(stoneintg-1350): add support for nested componentGroups

Timeline: Opened June 4 by 14rcole, merged June 22 (18 days). Large feature PR (29 files) adding nested ComponentGroup support. Three human reviewers approved (jsztuka, dirgim, kasemAlem). 12 fullsend review agent runs executed, 10 succeeded, 2 cancelled.

What went well

• Review quality was strong. The review agent identified genuine critical bugs early: a depth parameter that was never incremented in a recursive call (dead code), a nil-pointer dereference risk in snapshot/utils.go, and a logic error in GetComponentGroupsForComponentVersion referencing the wrong variable. These are the kinds of findings that justify automated review.
• Human reviewers engaged productively with both the bot findings and each other. dirgim's review on Jun 16 led to meaningful simplification of the component fetch logic.

What could go better

• Massive review redundancy. 12 review workflow runs consumed compute and tokens, but only ~3-4 were needed (one per unique commit SHA). On June 15 alone, 4 runs executed against the identical commit ffde3b2. Roughly 67% of runs were unnecessary.
• Repeated findings. The "breaking-api" finding on componentgroup_types.go was posted 6 separate times across reviews. The group snapshot TODO was flagged 4 times. This creates noise for human reviewers.
• Stale CHANGES_REQUESTED after human approval. The review agent continued posting CHANGES_REQUESTED verdicts after 3 human reviewers had approved, on findings they had explicitly acknowledged as intentional design decisions (breaking API change, incomplete group snapshot support deferred to a follow-up PR).

Existing issue coverage

All improvement opportunities identified are already tracked by open issues in fullsend-ai/fullsend:

• Same-commit skip: #963 — Skip review dispatch when HEAD SHA was already reviewed
• Finding dedup: #1500 — Do not re-request changes for unchanged findings; #1285 — Do not regenerate unchanged inline comments on re-reviews
• Dispatch throttling: #1418 — Deduplicate review runs on rapid successive pushes
• Human approval awareness: #1922 — Do not override human approval with stale findings; #1583 — Recognize human-resolved findings and stop re-flagging

No new proposals filed — implementing the existing backlog items above would have prevented the redundancy observed on this PR.