feature/metadata.go (1)

159-187: 💤 Low value

Edge case: Parallel command with empty Shell and Exec encodes as empty array.

In encodeLifecycleCommand lines 175-181, if a parallel command entry has both cmd.Shell == "" and len(cmd.Exec) == 0, it encodes cmd.Exec (empty slice) as []. This produces {"name": []} in the output, which decodes back as an empty exec command. While this round-trips correctly (empty command is skipped during merge/execution), it's inconsistent with the single-command path which returns nil for empty commands.

This is unlikely to occur in practice (features/users shouldn't define empty parallel entries), so this is a minor observation.

🤖 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 `@feature/metadata.go` around lines 159 - 187, The encodeLifecycleCommand
function currently marshals parallel entries even when both cmd.Shell == "" and
len(cmd.Exec) == 0, producing {"name": []}; update the loop over c.Parallel in
encodeLifecycleCommand to skip entries that are empty (if cmd.Shell == "" &&
len(cmd.Exec) == 0 continue) and after building obj return nil when obj is empty
(if len(obj) == 0 return nil) so empty parallel maps are omitted like the
single-command path; reference vars: encodeLifecycleCommand, c.Parallel, obj,
cmd.

feature/parse.go (1)

112-118: 💤 Low value

Mount decode warnings are discarded.

config.DecodeMounts returns warnings (e.g., for unrecognized mount entries), but they're discarded here with _, err := config.DecodeMounts(...). Consider propagating these warnings to the caller or accumulating them for diagnostics. If this is intentional (e.g., feature metadata parsing is lenient), a brief comment would clarify.

🤖 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 `@feature/parse.go` around lines 112 - 118, The code is discarding warnings
returned by config.DecodeMounts; capture the warnings value (e.g., warnings, err
:= config.DecodeMounts(r.Mounts)) and either append them to the feature
metadata's diagnostic/warnings field (e.g., add to out.Warnings or similar on
config.FeatureMetadata) or return them to the caller alongside the error/result;
if ignoring warnings is intentional, add a concise comment above the call
explaining why. Update the block that currently assigns out.Mounts so it also
preserves and surfaces the DecodeMounts warnings via the existing metadata
warning mechanism (or document the intentional discard).

config/merge_metadata.go (1)

64-67: Update misleading comment on lines 64–65: it incorrectly implies user values are preserved on collision.

The comment states "an explicit user entry stays after dedup," but unionDedup keeps first occurrence, and since layers are passed first (line 66), layer values are preferred on collision. Test TestMergeMetadata_CapAdd_UnionDedup confirms this is intentional for union semantics (the capability is present either way), but the comment text contradicts the actual behavior.

Clarify the comment to explain union behavior correctly: "Layers contribute first, then user, dedup keeps first occurrence (layer values preferred on collision)."

🤖 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 `@config/merge_metadata.go` around lines 64 - 67, The comment above the
union/dedup logic is misleading: update the text to state that layers are
provided first and unionDedup keeps the first occurrence, so layer values are
preferred on collisions; specifically, revise the comment near the unionDedup
calls for cfg.CapAdd and cfg.SecurityOpt to mention layerStrings,
FeatureMetadata, and unionDedup semantics (layers contribute first, then user,
dedup keeps first occurrence — layer wins on collision).

test/integration/image_metadata_test.go (1)

100-103: 💤 Low value

Inconsistent skip message.

The first test uses t.Skip("integration tests skipped with -short") while this test uses t.Skip() with no message. Consider using a consistent skip message for clarity in test output.

♻️ Suggested fix

 func TestImageMetadata_UserOverrideWins(t *testing.T) {
 	if testing.Short() {
-		t.Skip()
+		t.Skip("integration tests skipped with -short")
 	}

🤖 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 `@test/integration/image_metadata_test.go` around lines 100 - 103, The test
TestImageMetadata_UserOverrideWins calls t.Skip() without a message; change it
to use the same skip message as the other integration test (e.g.,
t.Skip("integration tests skipped with -short")) so skip output is consistent —
update the t.Skip call in TestImageMetadata_UserOverrideWins accordingly.