fix(#5448): use field-level merge for validation_loop in base composition#5450
fix(#5448): use field-level merge for validation_loop in base composition#5450fullsend-ai-coder[bot] wants to merge 3 commits into
Conversation
…tion The mergeBaseIntoChild() function used whole-struct replacement for ValidationLoop: if the child's pointer was non-nil (which happens when any sub-field is set in YAML), the base's entire ValidationLoop was discarded. This caused validation failures when a child harness set only validation_loop.schema — the base's script, max_iterations, and feedback_mode were lost. Add a mergeValidationLoop() helper that merges individual fields (Script, Schema, MaxIterations, FeedbackMode) where child non-zero values win and base fills gaps. Apply the same field-level merge in mergeForgeConfigInto() (base composition of forge configs) and mergeForgeConfig() (forge resolution). Note: golangci-lint was not available in the sandbox. go vet passed. Closes #5448
|
🤖 Finished Review · ✅ Success · Started 8:52 AM UTC · Completed 9:13 AM UTC |
Site previewPreview: https://26109217-site.fullsend-ai.workers.dev Commit: |
ReviewFindingsHigh
Medium
Low
Labels: PR modifies harness composition logic in internal/harness/ |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the review comment for full details.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
internal/harness/compose.go:426: [low] doc-code-mismatch
The doc comment on mergeValidationLoop states 'If only one is non-nil, returns a copy of it' but the code returns the original pointer, not a copy. The aliasing matches prior behavior and is harmless in current call sites, but the doc comment is inaccurate.
Suggested fix: Fix the doc comment to say 'returns it (not a copy)' or change the branches to return a copy.
internal/harness/forge.go(file-level): Line 67 · [low] validation-gap
validateForge requires script when validation_loop is set. With field-level merge, a forge could set only schema and inherit script from top level, but validateForge runs before ResolveForge and would reject this. Pre-existing issue, not introduced by this PR.
fullsend-ai-review
Bot
added
the
component/harness
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 10:15 AM UTC · Completed 10:20 AM UTC |
The comment claimed single-non-nil returns produced a copy, but the code returns the original pointer. Update the comment to say "returns it as-is" to match actual behavior. Addresses review feedback on #5450
🔧 Fix agent — iteration 1 (human-triggered)Addressed 1 of 2 review findings. Fixed the doc-code mismatch on mergeValidationLoop. Disagreed with 1 finding (pre-existing validation ordering issue, out of scope for this PR). Fixed (1):
Disagreed (1):
Tests: passed Updated by fullsend fix agent |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated multi-agent review pass (3 independent reviewer agents, findings cross-verified against the PR head branch). 4 findings survived triage at MEDIUM+ severity: 1 HIGH, 3 MEDIUM. Summary:
- A
DiffHarness/migrate-customizationsround-trip regression: the merge side moved to field-level semantics but the diff side (inverse) is still whole-struct, so re-composing a diffed-out child can silently backfill fields from base that the child intentionally left empty. - ADR-0045 still documents the old whole-struct-replace rule for
validation_loopin three places and wasn't updated alongside this behavior change. - An explicit
validation_loop: {}now silently and fully inherits the base block (bypassing the previous "script required" load error) as a side effect of zero-value-as-unset semantics, resolving an ADR-0045 open question without documenting or testing it. - The forge-resolution call site's field-level merge (
ResolveForge/mergeForgeConfig) can't actually be exercised through any real YAML-loading path due to a pre-existingvalidateForgeguard, and the new unit test for it bypasses that guard rather than proving the real path works.
See inline comments for file/line detail and suggested fixes. No LOW/INFO findings are included per review policy.
| // mergeValidationLoop merges two ValidationLoop pointers field by field. | ||
| // Child non-zero values win; base fills gaps. Returns the merged result. | ||
| // If both are nil, returns nil. If only one is non-nil, returns it as-is. | ||
| func mergeValidationLoop(base, child *ValidationLoop) *ValidationLoop { |
There was a problem hiding this comment.
[HIGH] — DiffHarness was not updated to match the new field-level merge, breaking the base/child round-trip used by migrate-customizations
ADR-0045's Consequences section states the base: merge rules and their inverse (DiffHarness) must change together ("Changes to merge rules must be reflected in both directions"), but this PR only updates the merge side. DiffHarness (internal/harness/diff.go:179-186) and diffForgeConfig (diff.go:470-473) still treat ValidationLoop as whole-struct: if child.ValidationLoop != nil && !reflect.DeepEqual(child.ValidationLoop, base.ValidationLoop) { result.Child.ValidationLoop = child.ValidationLoop }. When old whole-struct-replace merge was in effect, copying the entire child struct into the diff output was correct. With the new field-level mergeValidationLoop, it's not: if the child's ValidationLoop differs from base in just one field (say Script) while another field (say Schema) is genuinely empty/unset for that child, the whole child struct — including that genuine empty Schema — gets copied into the "minimal" diff. Re-composing that diffed-out child against the same base via mergeBaseIntoChild then backfills Schema from base, silently changing effective validation config.
This is reachable in production: internal/cli/migrate.go:479 calls harness.DiffHarness(&upstreamHarness, &customizedHarness, ...) as the core of the migrate-customizations command (ADR-0064), which converts standalone customized harnesses into base:-composed ones — exactly the diff → later-remerge path described above.
Suggestion: Diff ValidationLoop field-by-field in DiffHarness/diffForgeConfig (mirroring mergeValidationLoop's field set), consistent with how RunnerEnv/Env are already diffed field-by-field elsewhere in the same file. Add a round-trip test where base sets schema/feedback_mode and the differing child intentionally leaves them zero.
(Independently identified by 2 of 3 review agents, both verified by tracing the actual code paths.)
| // - Slices (skills, plugins, providers, api_servers): base + child (concatenated) | ||
| // - Maps (runner_env): base merged with child; child keys win | ||
| // - Pointer structs (validation_loop, security): child replaces if non-nil | ||
| // - Pointer structs (validation_loop): field-level merge; child non-zero fields win |
There was a problem hiding this comment.
[MEDIUM] premature-decision — ADR-0045 still documents whole-struct replacement for validation_loop; not updated alongside this behavior change
This comment (and its counterpart in forge.go's mergeForgeConfig doc) says the merge follows "ADR-0045 merge rules," but docs/ADRs/0045-forge-portable-harness-schema.md wasn't touched by this PR and still documents the old rule in three places: the forge inheritance table ("validation_loop | Forge value replaces top-level value entirely"), the base-composition merge-rules list ("validation_loop: child replaces base entirely (if non-nil)"), and the Open Questions section (still reasons about validation_loop on the premise "non-nil (including zero struct) = override entirely"). The ADR is the authoritative source these code comments explicitly cite; leaving it stale means the documented architecture no longer matches the code, and the ADR's own nil-vs-empty open question for validation_loop is left looking unresolved when this PR actually changes that behavior (see the other comment on this file re: validation_loop: {}).
Suggestion: Amend ADR-0045's inheritance table, base-merge-rules list, and open-questions note for validation_loop to describe the new field-level merge, in this PR or a fast-follow.
(Flagged independently by 2 of 3 review agents; both quoted the exact stale ADR passages.)
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 9:21 AM UTC · Completed 9:35 AM UTC |
Address review findings from waynesun09 on PR #5450: - DiffHarness/diffForgeConfig: diff ValidationLoop field-by-field (matching mergeValidationLoop semantics) so the diff→merge round-trip used by migrate-customizations is consistent. Add diffValidationLoop helper and round-trip tests. - mergeValidationLoop: special-case an all-zero child (validation_loop: {}) to return it as-is, preserving the pre-field-merge "script required" rejection instead of silent full inheritance from base. - validateForge: allow forge validation_loop without script when the top-level harness has validation_loop.script set (the script will be inherited via field-level merge in ResolveForge). Add LoadWithOpts end-to-end tests proving the real loading path works. Addresses review feedback on #5450
🔧 Fix agent — iteration 2 (human-triggered)Addressed 3 of 4 review findings from waynesun09. Fixed DiffHarness/diffForgeConfig round-trip regression (HIGH), added zero-value guard for mergeValidationLoop (MEDIUM), and relaxed validateForge to make forge field-level merge reachable through real loading paths (MEDIUM). Disagreed with ADR-0045 update (MEDIUM) per AGENTS.md immutability policy for Accepted ADRs. Fixed (3):
Disagreed (1):
Tests: passed Updated by fullsend fix agent |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Re-review of 36dcd1e1e against the 4 findings from the previous round.
Resolved (threads closed):
- The
validation_loop: {}full-inheritance bypass (base composition) is fixed —mergeValidationLoopnow special-cases a non-nil-but-all-zero child soValidate()'s "script is required" check still rejects it, matching pre-field-merge behavior. - The forge-resolution field-level-merge reachability gap is fixed —
validateForgenow allows an empty forge-level script when the top level has one, and a realLoadWithOptsend-to-end test proves the partial-override path actually works, not just the helper in isolation.
Still open:
- ADR-0045 is untouched by this commit and still documents whole-struct
validation_loopreplacement in multiple places (inheritance table, base-merge rules list, Open Questions). - The
DiffHarnessround-trip fix below is real progress but doesn't fully close the gap — see the follow-up comment on the round-trip finding.
New (introduced by this commit):
- The two fixes above don't compose cleanly at the forge level — see the new comment on
compose.go.
| // equal, or child is nil (removal is handled by the caller). This mirrors | ||
| // mergeValidationLoop's field set so diff→merge round-trips correctly for | ||
| // non-zero field values. | ||
| func diffValidationLoop(base, child *ValidationLoop) *ValidationLoop { |
There was a problem hiding this comment.
[HIGH] — Follow-up on the round-trip finding above (still open): the fix narrows but does not close the gap
36dcd1e1e added diffValidationLoop and switched DiffHarness/diffForgeConfig to field-level diff, matching the merge side's granularity. That closes the specific whole-struct-vs-field-level mismatch, but the underlying round-trip problem is only partially fixed.
diffValidationLoop and mergeValidationLoop both use the Go zero value as the only signal for "not set, inherit from base." They can't distinguish "this field equals base's value, omit it from the diff" from "this field's real value is the zero value, and it differs from a non-zero base value." Concretely: base = {Script:"base.sh", Schema:"base-schema.json", MaxIterations:5, FeedbackMode:"append"}; a customized child overrides Script and intentionally carries no schema: child = {Script:"custom.sh", Schema:"", MaxIterations:5, FeedbackMode:"append"}. diffValidationLoop(base, child) produces {Script:"custom.sh", Schema:"", MaxIterations:0, FeedbackMode:""}. Feeding that back through mergeValidationLoop(base, diff) yields Schema:"base-schema.json" — silently reintroducing schema validation the customized harness never wanted. Still reachable via migrate-customizations (internal/cli/migrate.go's DiffHarness call). The new round-trip tests (TestDiffHarness_ValidationLoopFieldLevelRoundTrip, TestDiffForgeConfig_ValidationLoopFieldLevel) only exercise children whose non-overridden fields already equal base's, so they pass without covering this.
Suggestion: A zero-value-comparison diff/merge can't express "explicitly zero, differs from non-zero base." Track field presence explicitly (pointer fields, or a presence map/bitmask alongside ValidationLoop) instead of inferring it from the value, or explicitly document this as an accepted migrate-customizations limitation and add a test that locks in the (documented) behavior rather than leaving it to silently regress.
(Independently confirmed — with an identical empirical repro — by all 4 review agents in this round.)
|
|
||
| // Non-nil but all-zero child = explicit empty override (e.g., `validation_loop: {}`). | ||
| // Return as-is to preserve the pre-field-merge rejection behavior. | ||
| if *child == (ValidationLoop{}) { |
There was a problem hiding this comment.
[HIGH] — New in 36dcd1e1e: this all-zero-child special case (added to fix the validation_loop: {} finding) breaks forge-level inheritance, contradicting validateForge's own comment
This commit's two fixes don't compose. This special case (if *child == (ValidationLoop{}) { return child }) is correct for the base→child composition path it was written for, but mergeValidationLoop is shared by every caller, including mergeForgeConfig (forge.go) and mergeForgeConfigInto (compose.go), where a non-nil-but-all-zero forge-level block should mean "no forge-specific override, inherit everything" — matching how skills/runner_env already treat an empty forge override — not "explicit override, reject."
Concretely, this breaks the exact scenario validateForge's new topHasScript relaxation was written to support: a fully-populated top-level validation_loop plus forge.<platform>.validation_loop: {} (a literal empty mapping, which unmarshals to a non-nil pointer to an all-zero struct, not nil). validateForge lets this load, on the comment's own promise that "the script will be inherited via field-level merge in ResolveForge" — but ResolveForge → mergeForgeConfig hits this special case and returns the empty struct as-is, discarding the top-level Script/Schema/MaxIterations/FeedbackMode entirely. Validate() then rejects the load with validation_loop.script is required when validation_loop is set — a confusing failure for a config validateForge itself just approved, on the very promise that just got broken. Confirmed end-to-end via LoadWithOpts; the same defect reproduces through mergeForgeConfigInto's base-chain forge composition. Neither new test (TestLoadWithOpts_ForgePartialValidationLoopInheritsScript, TestLoadWithOpts_ForgeEmptyValidationLoopNoTopLevelScript) covers a fully-empty forge block with a populated top level, so this shipped uncaught.
Suggestion: Move the all-zero-child "reject" special case out of the shared helper and into the mergeBaseIntoChild call site specifically, or add a parameter so mergeForgeConfig/mergeForgeConfigInto can opt out and fall through to normal field-by-field backfill for an all-zero child. Add end-to-end coverage for "top-level populated + forge validation_loop: {}" on both the ResolveForge and LoadWithBase paths.
(Independently confirmed — across the ResolveForge and base-chain forge-composition call sites — by all 4 review agents in this round.)
2 participants
Summary
Fix
validation_loopbase composition to use field-level merge instead of whole-struct replacement. When a child harness sets only onevalidation_loopfield (e.g.,schema), the remaining fields are now inherited from the base, consistent with howenvandrunner_envmerge.Related Issue
Fixes #5448
Changes
mergeValidationLoop(base, child *ValidationLoop)helper that merges individual fields (Script,Schema,MaxIterations,FeedbackMode) — child non-zero values win, base fills gapsmergeBaseIntoChild()with field-level merge via the new helpermergeForgeConfigInto()(forge-level base composition) andmergeForgeConfig()(forge resolution)Testing
go test -race -cover ./internal/harness/...passes (89.7% coverage)go vet ./internal/harness/...passesmergeValidationLoopcovering all nil/partial/full combinationsmergeForgeConfigIntoandResolveForgeChecklist
!for breaking changes)Closes #5448
Post-script verification
agent/5448-validation-loop-merge)9525311fa3ff7c81ed791130ee7ce972a71b3eda..HEAD)