feat(harness): require role field in Validate()

[ggallen]

Summary

• Promotes the missing role: check from a Lint() warning to a Validate() hard error, as specified in ADR-0045 Phase 4 PR 1
• Every scaffold harness already sets role:, so this is non-breaking for existing users
• Removes the now-redundant lint diagnostic for missing role
• Updates all test fixtures across harness, CLI run, lock, and lock-all tests to include role: where needed

Context

This is Phase 4, PR 1 of ADR-0045 (Forge-Portable Harness Schema). The role: field was introduced in Phase 2 as optional with a lint warning. This PR makes it required at validation time, enforcing the contract for all harness configurations going forward.

Test plan

• go test ./internal/harness/ — all harness validation, lint, forge, compose, and integration tests pass
• go test ./internal/cli/ — all run, lock, and lock-all tests pass
• go test ./internal/... — full internal test suite passes
• Pre-commit hooks pass (gofmt, go vet, etc.)

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Require harness role during validation and update CLI/harness fixtures
✨ Enhancement 🧪 Tests 🕐 40+ Minutes

Description

• Enforce role: as a required harness field via Validate().
• Remove the deprecated lint warning for missing role:.
• Update CLI and harness tests/fixtures to include role: everywhere needed.

Diagram

graph TD
  U["Developer / CI"] --> CLI["CLI run/lock"] --> Load["Harness Load"] --> V["Harness Validate()"] --> R{"role present?"}
  R -->|"no"| E["Error: role field is required"]
  R -->|"yes"| OK["Proceed (lock/run)"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep warning for one more release (grace period)

• ➕ Avoids breaking any unknown legacy harnesses that relied on the earlier optional field
• ➕ Provides a clearer migration window (warning → error)
• ➖ Delays enforcing ADR-0045 Phase 4 contract
• ➖ Leaves schema enforcement inconsistent (lint vs validate)

2. Versioned schema / feature-flagged enforcement

• ➕ Allows strict validation only under a declared schema version or flag
• ➕ Provides explicit opt-in for older configs
• ➖ Adds ongoing complexity to schema handling and documentation
• ➖ Not aligned with the ADR phase goal of making the field mandatory now

Recommendation: Given ADR-0045 Phase 4 intent and the claim that scaffolded harnesses already set role, promoting the check to Validate() is the right approach. The current PR cleanly removes the deprecated lint path and updates fixtures to match the enforced contract; a grace-period approach is the main alternative if unexpected legacy configs exist.

Files changed (10) +178 / -98

Enhancement (2) +8 / -16

harness.goMake role required in Harness.Validate() +8/-7

Make role required in Harness.Validate()

• Promotes role from optional validation (only validated when present) to a required field. 'Validate()' now errors with "role field is required" when Role is empty, and still enforces role format/double-hyphen constraints.

internal/harness/harness.go

lint.goRemove missing-role lint diagnostic +0/-9

Remove missing-role lint diagnostic

• Deletes the lint warning that previously flagged empty 'role' as a future requirement. With role now required at validation time, lint no longer emits any diagnostics for role.

internal/harness/lint.go

Tests (8) +170 / -82

lock_all_test.goAdd role to lock-all harness fixtures +25/-14

Add role to lock-all harness fixtures

• Updates numerous harness YAML strings used by lock-all tests to include a 'role:' field. This keeps CLI lock-all behaviors passing now that harness validation requires role.

internal/cli/lock_all_test.go

lock_test.goAdd role to lock command harness fixtures +23/-19

Add role to lock command harness fixtures

• Adds 'role: test' to harness contents used throughout lock-related tests, including base/URL cases. Ensures lock workflows remain valid under the stricter harness validation.

internal/cli/lock_test.go

run_test.goFail early on missing role and update run fixtures +11/-18

Fail early on missing role and update run fixtures

• Updates run test harness fixtures to include 'role:' and changes the missing-role test to expect a hard error instead of a printed lint warning. Aligns CLI run behavior with the new validation requirement.

internal/cli/run_test.go

compose_test.goAdd role to composed/base harness test YAML +43/-0

Add role to composed/base harness test YAML

• Updates base/child harness YAML in composition tests to include 'role:' in all relevant fixtures. Prevents composition tests from failing due to the new required-field validation.

internal/harness/compose_test.go

forge_test.goSet Role in forge validation/load tests +11/-0

Set Role in forge validation/load tests

• Adds 'Role: "test"' to Harness structs and adds 'role:' to YAML fixtures used by forge-related load/validate tests. Keeps these tests focused on forge behavior rather than failing on missing role.

internal/harness/forge_test.go

harness_test.goUpdate harness load/validate tests for required role +51/-19

Update harness load/validate tests for required role

• Adds 'role:' to YAML fixtures and populates 'Role' in many Harness struct-based validation tests. Replaces the prior expectation that missing role loads successfully with an assertion that it errors.

internal/harness/harness_test.go

integration_test.goUpdate integration expectations for role presence +5/-1

Update integration expectations for role presence

• Updates integration fixtures to include 'role:' and adjusts assertions accordingly (role is no longer empty after load). Keeps backward-compat style tests aligned with the new schema requirement.

internal/harness/integration_test.go

lint_test.goRemove missing-role lint-warning coverage +1/-11

Remove missing-role lint-warning coverage

• Adjusts lint tests to reflect that valid harnesses return nil diagnostics and removes the test asserting a warning when role is empty. Aligns lint test expectations with the simplified lint behavior.

internal/harness/lint_test.go

[github-actions]

Site preview

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

Commit: 30571fad24fa3f5602de467d5c80dc8d67d4a880

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 9:30 PM UTC · Completed 9:41 PM UTC
Commit: c24bc2e · View workflow run →

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Looks good to me.

Findings

Low

• [omitempty-consistency] internal/harness/harness.go:201 — The Role struct field retains yaml:"role,omitempty" despite the field now being required in Validate(). This is inconsistent with the Agent field (also required, no omitempty). No runtime impact since no production code marshals Harness structs, but the tag is misleading for a required field.

• [dead-code] internal/harness/lint.go:40 — After removing the role-empty diagnostic, Lint() always returns nil. Callers in internal/cli/run.go and internal/cli/lock.go still iterate over h.Lint(), but this code path is now dead. The Phase 4 plan explicitly retains this as scaffolding for future lint rules — no immediate action required. Consider adding a comment documenting that Lint() is kept for future extensibility.

• [test-adequacy] internal/harness/integration_test.go:15 — TestLoadWithBase_BackwardCompat adds role: test to the fixture and asserts h.Role == "test", but the name still says "BackwardCompat." The test no longer validates backward compatibility without role — it tests minimal-valid-harness-with-role. Consider renaming (e.g., TestLoadWithBase_MinimalValid).

Previous run

Looks good to me.

Findings

Low

• [dead-code] internal/harness/lint.go:40 — After removing the role-empty diagnostic, Lint() always returns nil. Callers in internal/cli/run.go and internal/cli/lock.go still iterate over h.Lint(), but this code path is now dead. The Phase 4 plan explicitly retains this as scaffolding for future lint rules — no immediate action required.

• [test-adequacy] internal/harness/integration_test.go:15 — TestLoadWithBase_BackwardCompat adds role: test to the fixture and asserts h.Role == "test", but the name still says "BackwardCompat." The test no longer validates backward compatibility without role — it tests minimal-valid-harness-with-role. Consider renaming (e.g., TestLoadWithBase_MinimalValid).

• [validation-location-consistency] internal/harness/harness.go:201 — Removing omitempty from the Role struct tag deviates from the pattern used by other validated fields (Slug, Model) which keep omitempty and rely solely on Validate() to enforce requirements. The behavioral impact is zero since no code currently marshals Harness structs, but the inconsistency is worth noting.

Previous run (2)

Review

Findings

Medium

• [stale-api-documentation] docs/ADRs/0045-forge-portable-harness-schema.md:475 — The Go struct definition in ADR-0045 shows the Role field with yaml:"role,omitempty" tag, but this PR changes the tag to yaml:"role" (removing omitempty) since role is now required in Validate(). The ADR is not updated in this PR, so the code example is now stale.
  Remediation: Update the struct tag in the ADR from yaml:"role,omitempty" to yaml:"role" to match the new requirement.

Low

• [dead-code] internal/harness/lint.go:40 — After removing the role-empty diagnostic, Lint() becomes a no-op that always returns nil. The callers in internal/cli/run.go and internal/cli/lock.go still iterate over h.Lint() and emit diagnostics, but this code path is now dead. The Phase 4 plan explicitly retains this as scaffolding for future lint rules — no immediate action required.

• [test-adequacy] internal/harness/lint_test.go:15 — The remaining lint subtests ("valid harness returns nil" and "role and slug set") pass trivially since Lint() always returns nil regardless of input. They serve as regression tests for the Lint interface but no longer test meaningful behavior. Consider repurposing when future lint rules are added.

• [stale-plan-reference] docs/plans/adr-0045-forge-portable-harness-phase4.md — This plan describes PR 1 (require role in Validate()) as future work to be implemented, but this PR (feat(harness): require role field in Validate() #2446) implements it. Consider adding a note that PR 1 has been completed.

---

Labels: PR modifies harness validation and lint logic in internal/harness/.

Previous run (3)

Review

Findings

Medium

• [dead-code] internal/harness/lint.go:40 — After removing the role-empty diagnostic, Lint() becomes a no-op that always returns nil. The callers in internal/cli/run.go and internal/cli/lock.go still iterate over h.Lint() and emit diagnostics, but this code path is now dead. Retaining the function and its callers as scaffolding for future lint rules is defensible — no immediate action required.

Low

• [dead-code] internal/cli/run_test.go:1834 — TestRunAgent_NoLintWarningWithRole and TestRunLock_NoLintWarningWithRole verify that no lint warning is emitted when role is set. After removing the role lint diagnostic, Lint() always returns nil regardless of role, so these tests pass trivially and no longer test meaningful behavior. Consider removing them or repurposing for future lint rules.

---

Labels: PR modifies harness validation and lint logic in internal/harness/

---

Labels: PR modifies harness validation and lint logic in internal/harness/.

Previous run (4)

Review

Findings

Low

• [struct-tag-inconsistency] internal/harness/harness.go:201 — The Role field retains yaml:"role,omitempty" even though it is now required. Compare with the Agent field (line 198) which uses yaml:"agent" without omitempty. The omitempty tag only affects marshaling output, not unmarshaling, so this is not a runtime bug — but it creates a misleading signal that Role is optional.
  Remediation: Change the struct tag from yaml:"role,omitempty" to yaml:"role" to match the convention for required fields.

Info

• [scope-alignment] The change matches ADR-0045 Phase 4 PR 1 scope exactly. No scope creep detected.
• [edge-case] internal/harness/compose.go:366 — When both a child harness and its base omit role, the composed harness will have an empty role and Validate() will now reject it. This is correct/intentional behavior.

---

Labels: PR modifies harness validation and lint logic in internal/harness/

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:46 PM UTC · Completed 10:00 PM UTC
Commit: c13afdb · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:07 PM UTC · Completed 10:20 PM UTC
Commit: 8bd73d9 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 10:25 PM UTC · Completed 10:37 PM UTC
Commit: ed8c563 · View workflow run →

[ralphbean]

LGTM.

[waynesun09]

ADR-0045 Review Squad — 11 findings (medium+ severity). Reviewed for correctness, security, and ADR alignment.

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 3:35 PM UTC · Completed 3:47 PM UTC
Commit: 30571fa · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 4:25 PM UTC · Completed 4:34 PM UTC
Commit: 30571fa · View workflow run →

[fullsend-ai-retro]

Retro: PR #2446 — feat(harness): require role field in Validate()

This was a human-authored PR (by ggallen) implementing ADR-0045 Phase 4 PR 1, making the role field required in harness validation. Single commit, 11 files changed, +180/−162 lines.

Timeline

1. June 18 21:26 — PR created. Qodo-code-review bot, codecov, and site preview ran within minutes.
2. June 22 14:23 — ralphbean approved ("LGTM").
3. June 22 15:09–15:13 — waynesun09 posted 11 medium-severity findings as the "ADR-0045 Review Squad." Key findings: omitempty consistency, dead Lint() code, test assertion tightening, stale plan status.
4. June 22 15:26:37–15:26:59 — ggallen replied to all 11 findings individually, submitting 11 separate COMMENTED reviews in 22 seconds. Fixed 5 findings, acknowledged 6.
5. June 22 15:26–15:27 — The 11 individual review replies triggered 11+ pull_request_review fullsend.yaml workflow runs within 22 seconds. Most were cancelled by concurrency groups, but the fan-out still consumed CI resources.
6. June 22 ~15:41 — fullsend-ai-review bot posted its consolidated review ("Looks good to me" with 3 low-severity findings). The agent's findings overlapped substantially with waynesun09's earlier human review.
7. June 22 16:18 — PR merged.

Observations

Review quality gap (noted, not proposed): The agent review found the same issues as the human reviewer but rated them all as low severity, while the human rated them as medium — and 5 led to actual code fixes. The agent's "Looks good to me" verdict was overly optimistic given that actionable improvements were identified. This is partially tracked by #1525 (review agent misses analysis that Review Squad catches). Since the agent found the issues but under-weighted them rather than missing them entirely, severity calibration may warrant separate tracking — but per #2502 guidance, a single-instance observation isn't sufficient evidence for a systemic proposal.

Workflow fan-out from review replies: The most concrete improvement opportunity. See proposal below.

Proposals filed

• Filter COMMENTED pull_request_review events from review dispatch (in fullsend-ai/fullsend)