test(harness): add YAML nil-vs-empty unmarshaling semantics tests

[ggallen]

Summary

• Adds internal/harness/yaml_semantics_test.go pinning gopkg.in/yaml.v3 unmarshaling behavior for all Harness fields where nil-vs-empty semantics matter for base composition (PR 4) and forge merging (PR 1)
• Tests exercise raw yaml.Unmarshal (no Validate(), no Load()) to document the library's contract, not harness validation logic
• Covers slices (Skills, Plugins, Providers, AllowedRemoteResources), struct slices (HostFiles, APIServers), maps (RunnerEnv, APIServer.Env), pointer-to-structs (ValidationLoop, Security and all nested configs), and pointer-to-bools (all *bool fields in the security tree)

PR 7 of the ADR-0045 implementation plan. No dependencies — parallel with all other PRs.

Test plan

• go test ./internal/harness/ -run TestYAMLSemantics -v — all 47 subtests pass
• make go-test — full suite passes
• make lint — clean

🤖 Generated with Claude Code

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:10 PM UTC · Completed 4:17 PM UTC
Commit: 4ed6da4 · View workflow run →

[fullsend-ai-review]

Review

Findings

No findings.

Previous run

Review

Findings

No findings.

Previous run (2)

Review

Findings

Low

• [dead-code] internal/harness/yaml_semantics_test.go:20 — The sliceCase struct type is declared inside TestYAMLSemantics_Slices but never used. The test uses a different anonymous struct for the fields variable (with different field names and signatures). This is dead code from an earlier iteration of the table-driven test design.
  Remediation: Remove the unused sliceCase type declaration (lines 20–24).

• [test-inadequate] internal/harness/yaml_semantics_test.go:297 — 5 SandboxHooks *bool fields (SecretRedactPostTool, UnicodePostTool, ContextSuppressPostTool, CanaryPreTool, CanaryPostTool) have zero test coverage in the file. 3 HostScanners *bool fields (ContextInjection, SSRFValidator, SecretRedactor) are checked for nil-when-absent in TestYAMLSemantics_NestedPointerStructs but not tested for explicit true/false unmarshaling in TestYAMLSemantics_PointerBools. The sandbox_hooks/empty_is_non_nil_zero subtest only asserts nil on Tirith, SSRFPreTool, and ToolAllowlistPreTool, omitting the other 5 *bool fields. Since all *bool fields share identical yaml.v3 semantics, practical risk is negligible — but the PR claims coverage of "all Harness fields where nil-vs-empty semantics matter."
  Remediation: Either add the missing *bool fields to TestYAMLSemantics_PointerBools and the sandbox_hooks/empty_is_non_nil_zero nil assertions, or soften the PR description to "representative fields" rather than "all fields."

[fullsend-ai-review]

🤖 Review · Started 4:27 PM UTC
Commit: 4ed6da4 · View workflow run →

[github-actions]

Site preview

Preview: https://ff192726-site.fullsend-ai.workers.dev

Commit: ed2d3d0abc9a57d7c99944a0b4a388cc08f30ee5

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:27 PM UTC · Completed 4:32 PM UTC
Commit: 4ed6da4 · View workflow run →

[ralphbean]

LGTM. One minor note inline.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:09 PM UTC · Completed 8:15 PM UTC
Commit: ed2d3d0 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 8:48 PM UTC · Completed 8:54 PM UTC
Commit: ed2d3d0 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2129 — test(harness): add YAML nil-vs-empty unmarshaling semantics tests

This PR followed a healthy workflow with good outcomes across all optimization lenses:

• Review quality: The review bot caught two valid issues on the first pass (dead code and incomplete *bool field coverage). Human reviewer ralphbean added genuine value by suggesting explicit-null semantics tests — a nuanced edge case the bot missed. All feedback was actionable.
• Rework rate: Two rounds of fixes (one for bot findings, one for human suggestion) — reasonable for the scope.
• Token cost: Three review agent runs (~614 KB total artifacts). The second and third runs found no issues, which is expected after fixes were applied.
• Time to resolution: ~4.5 hours from open to merge, with most time spent waiting for human review (~3.5 hours). Agent interactions were fast (7 minutes per review run).

No new proposals filed. All potential improvement areas are already covered by existing open issues:

• Redundant dispatch triggers: Reduce redundant review dispatches triggered by non-code events #893, Add concurrency group to dispatch-review job in fullsend.yaml #981, Skip review dispatch when HEAD SHA was already reviewed and approved #963, Deduplicate review runs on rapid successive pushes #1418, Skip re-review when PR diff is unchanged after rebase/reopen #1356, and 14+ others
• First-pass review completeness (bot missing the explicit-null edge case): Review agent should catch all findings in the first pass to reduce rework cycles #1582
• pull_request_review event filtering: Filter pull_request_review trigger to only fire on changes_requested #1125, dispatch: filter bot-triggered pull_request_review events to prevent review self-triggering loop #1271