fix(#5448): merge validation_loop at field level during composition

[myukitty]

Description

When a child harness inherits from a base harness that defines validation_loop and the child only overrides specific fields (such as validation_loop.schema to provide a custom validation schema), mergeBaseIntoChild, mergeForgeConfigInto, and mergeForgeConfig previously treated ValidationLoop as a whole-struct replacement (only carrying forward PreflightCheck). As a result, the base's script, max_iterations, and feedback_mode were silently discarded, causing subsequent validation errors:
validation_loop.script is required when validation_loop is set

Solution

• Implement mergeValidationLoopInto to perform field-level merge for ValidationLoop across all composition paths (mergeBaseIntoChild, mergeForgeConfigInto, and mergeForgeConfig).
• Ensure child/override values take precedence while preserving base-defined fields for omitted keys.
• Add unit tests across compose_test.go and forge_test.go verifying field-level merge, conflict resolution, and immutability of source configs.

Related Issue

Fixes #5448

[github-actions]

Thank you for your interest in contributing to fullsend, @myukitty.

This project uses a vouch system for first-time contributors. Before submitting a pull request, you need to be vouched by a maintainer.

To get vouched:

1. Open a Vouch Request discussion.
2. Describe what you want to change and why.
3. Write in your own words — do not have an AI generate the request.
4. A maintainer will comment /vouch if approved.
5. Once vouched, open a new PR (preferred) or reopen this one.

See CONTRIBUTING.md for details.

[github-actions]

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

[qodo-code-review]

PR Summary by Qodo

Merge validation loop fields during harness composition

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Merge validation-loop fields across harness inheritance and forge composition.
• Preserve inherited defaults while allowing child and forge overrides.
• Add precedence and immutability coverage for every composition path.

Diagram

graph TD
  Base["Base Harness"] --> BaseCompose["Base Composition"] --> Merge["Validation Merge"] --> Resolved["Resolved Harness"]
  Child["Child Harness"] --> BaseCompose
  Top["Top Config"] --> ForgeCompose["Forge Resolution"] --> Merge
  Forge["Forge Override"] --> ForgeCompose

Loading

High-Level Assessment

The shared explicit merge helper is the best approach because all composition paths require identical precedence rules, while the forge path can copy its override before merging to preserve immutability. Reflection-based or generic struct merging would obscure field-specific zero-value semantics without meaningful benefit.

Files changed (4) +173 / -13

Bug fix (2) +37 / -13

compose.goApply field-level validation-loop merging during composition +29/-11

Apply field-level validation-loop merging during composition

• Adds a shared validation-loop merge helper that preserves child values and fills unset fields from the base. Uses it for both harness inheritance and nested forge configuration composition.

internal/harness/compose.go

forge.goMerge forge validation-loop overrides without mutating inputs +8/-2

Merge forge validation-loop overrides without mutating inputs

• Replaces whole-struct forge validation-loop assignment with field-level composition. Copies the forge override before filling omitted fields from the top-level harness configuration.

internal/harness/forge.go

Tests (2) +136 / -0

compose_test.goCover validation-loop inheritance and override precedence +82/-0

Cover validation-loop inheritance and override precedence

• Adds tests confirming omitted validation-loop fields inherit base values in harness and forge composition. Also verifies child values win when both configurations define the same fields.

internal/harness/compose_test.go

forge_test.goTest forge field merging and source immutability +54/-0

Test forge field merging and source immutability

• Adds coverage for partial forge validation-loop overrides inheriting top-level values. Verifies merging does not mutate the original forge configuration.

internal/harness/forge_test.go

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Forge partial loops fail validation 🐞 Bug ≡ Correctness

Description

Public loading paths call validateForge before ResolveForge, so a forge loop that only overrides
schema is rejected for missing script before the new merge can inherit the top-level script. The
added test bypasses this failure by calling ResolveForge directly.

Code

internal/harness/forge.go[R207-209]

+			merged := *fc.ValidationLoop
+			mergeValidationLoopInto(h.ValidationLoop, &merged)
+			h.ValidationLoop = &merged

Relevance

●●● Strong

Accepted history favors explicit fixes for composition-order validation gaps and field-level
inheritance semantics.

PR-#2582

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

validateForge rejects every non-nil forge validation loop whose own Script is empty, while
public loaders invoke that validation before ResolveForge. Consequently, the new fallback merge is
unreachable for the schema-only forge override demonstrated by the added direct-call test.

internal/harness/forge.go[105-115]
internal/harness/forge.go[203-210]
internal/harness/harness.go[363-382]
internal/harness/compose.go[151-159]
internal/harness/forge_test.go[209-235]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Forge-specific `validation_loop` overrides cannot omit `script`, even though the new field-level merge is intended to inherit it from the top-level loop. Public loading paths validate forge blocks before resolving them, so schema-only overrides fail before reaching the merge.

## Issue Context
Update forge validation to account for the effective merged validation loop, while retaining required-script and URL validation after inheritance. Add a test through a public loading path rather than invoking `ResolveForge` directly.

## Fix Focus Areas
- internal/harness/forge.go[105-115]
- internal/harness/forge.go[203-210]
- internal/harness/forge_test.go[209-235]
- internal/harness/harness.go[363-382]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context

✅ Compliance rules (platform): 56 rules

✅ Skills: writing-user-docs, writing-adrs

Tip of the day

💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-code-review]

1. Forge partial loops fail validation 🐞 Bug ≡ Correctness

Public loading paths call validateForge before ResolveForge, so a forge loop that only overrides
schema is rejected for missing script before the new merge can inherit the top-level script. The
added test bypasses this failure by calling ResolveForge directly.

Agent Prompt

## Issue description
Forge-specific `validation_loop` overrides cannot omit `script`, even though the new field-level merge is intended to inherit it from the top-level loop. Public loading paths validate forge blocks before resolving them, so schema-only overrides fail before reaching the merge.

## Issue Context
Update forge validation to account for the effective merged validation loop, while retaining required-script and URL validation after inheritance. Add a test through a public loading path rather than invoking `ResolveForge` directly.

## Fix Focus Areas
- internal/harness/forge.go[105-115]
- internal/harness/forge.go[203-210]
- internal/harness/forge_test.go[209-235]
- internal/harness/harness.go[363-382]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools