feat(harness): add Lint() diagnostic method (ADR-0045 Phase 3 PR 1)

[ggallen]

Part of #2326

Summary

• Adds Lint() method to the Harness struct returning []Diagnostic — non-fatal warnings separate from Validate() (which returns hard errors)
• First lint rule: warns when role field is missing, preparing for Phase 4 which will make it required
• Adds the Phase 3 implementation plan (docs/plans/adr-0045-forge-portable-harness-phase3.md)

Details

New files:

• internal/harness/lint.go — DiagnosticSeverity type, Diagnostic struct with String(), and Harness.Lint() method
• internal/harness/lint_test.go — 6 subtests covering role presence/absence, severity formatting, and unknown severity fallback

No existing files modified. Validate() is unchanged. No callers of Lint() are added yet — that is Phase 3 PR 3.

100% code coverage on lint.go. All existing tests pass.

Test plan

• go test -v -run TestLint ./internal/harness/ — all 6 subtests pass
• go test -coverprofile=cover.out ./internal/harness/ && go tool cover -func=cover.out | grep lint.go — 100% coverage on lint.go
• make go-test — all existing tests pass
• make lint — passes
• make go-vet — passes

🤖 Generated with Claude Code

[github-actions]

Site preview

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

Commit: 3305c1a466bf51f8954c93757f56001cbbb868a3

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:43 AM UTC · Completed 11:53 AM UTC
Commit: 84ce820 · View workflow run →

[fullsend-ai-review]

Looks good to me

Previous run

Review

Findings

Low

• [doc-code-mismatch] docs/plans/adr-0045-forge-portable-harness-phase3.md:93 — Minor wording mismatch between the plan and the implementation. The plan specifies the lint message as "role is missing; it will be required in a future version" but the code in lint.go:47 uses "role is not set; it will be required in a future version". No behavioral impact, but could cause confusion if the plan is used as a specification.

Info

• [prior-finding-resolved] internal/harness/lint.go:20 — Prior finding [default-case-pattern] is resolved. The default case now uses fmt.Sprintf("DiagnosticSeverity(%d)", int(s)), matching the established pattern in LayerStatus.String() and Operation.String().

• [test-adequacy] internal/harness/lint_test.go — Tests cover the three key Lint() cases (role set, role empty, role+slug set), Diagnostic.String() formatting for warning/error/unknown severity, and verify nil-vs-empty-slice semantics. Adequate coverage for the scope of this PR.

• [scope-authorization] internal/harness/lint.go — PR correctly implements item 1 of Issue ADR-0045 Phase 3: Deprecate config.yaml agents: block #2326 (Add Lint() diagnostic method). Scope is appropriately limited to lint infrastructure and a single role-missing warning; remaining items 2–6 deferred to future PRs as described in the Phase 3 plan.

• [architectural-coherence] internal/harness/lint.go — Lint() method design matches ADR-0045's architectural requirement for a separate advisory path alongside Validate(). The []Diagnostic return type provides structured, non-fatal findings without breaking existing Validate() contracts.

Previous run (2)

Review

Findings

Low

• [default-case-pattern] internal/harness/lint.go:20 — Default case in DiagnosticSeverity.String() returns the literal string "unknown" instead of a type-prefixed format like DiagnosticSeverity(%d). This departs from the established pattern in LayerStatus.String() (layers.go:30) and Operation.String() (layers.go:60), both of which return fmt.Sprintf with the type name for unknown values. The test at lint_test.go:44 expects "unknown" accordingly — both would need updating to match the codebase pattern.
  Remediation: Change line 20 from return "unknown" to return fmt.Sprintf("DiagnosticSeverity(%d)", int(s)) and update lint_test.go:44 to match.

Info

• [test-adequacy] internal/harness/lint_test.go — Tests cover the three key cases (role set, role empty, role+slug set) and Diagnostic.String() formatting including unknown severity. Adequate coverage for the current single-rule Lint().

• [edge-case] internal/harness/lint.go:41 — Lint() does not guard against a nil receiver, but Validate() has the same pattern and the docstring explicitly requires calling after a successful Validate(). Consistent with existing convention.

• [scope-authorization] internal/harness/lint.go — PR correctly implements item 1 of Issue ADR-0045 Phase 3: Deprecate config.yaml agents: block #2326. Implementation adds Lint() returning []Diagnostic with a single rule (missing role warning), matching ADR-0045 Phase 3 requirements. Remaining items 2-6 appropriately deferred.

• [architectural-coherence] internal/harness/lint.go — The Lint() method design follows ADR-0045's architectural requirement for a separate advisory path alongside Validate(). The Diagnostic struct provides structured warnings without breaking existing callers.

Previous run (3)

Review

Findings

High

• [scope-mismatch] N/A - PR metadata — PR body says Closes #2326 but the title explicitly labels this as "Phase 3 PR 1", indicating more PRs will follow. The diff delivers only the Lint() method, its tests, the ADR annotation update, and the Phase 3 plan document. Issue ADR-0045 Phase 3: Deprecate config.yaml agents: block #2326 authorizes at least 6 work items. Using Closes will auto-close the tracking issue when this PR merges, preventing subsequent PRs from linking to their authorization.
  Remediation: Change PR body from Closes #2326 to Part of #2326 or Relates to #2326. Issue ADR-0045 Phase 3: Deprecate config.yaml agents: block #2326 should remain open until all Phase 3 items are delivered.

Low

• [phase-boundary-clarity] docs/ADRs/0045-forge-portable-harness-schema.md:145 — After the diff, line 145 reads: "In Phase 3, Lint() emits warnings when role is missing. In Phase 4, Validate() requires role." This implicitly conveys that Validate() is unchanged in Phase 3, but a reader unfamiliar with the codebase could wonder what Validate() does in Phase 3.
  Remediation: Consider adding "Validate() continues to allow missing role in Phase 3" for completeness, though the current text is not incorrect.

Info

• [test-adequacy] internal/harness/lint_test.go — Tests cover the three key cases (role set, role empty, role+slug set) and Diagnostic.String() formatting including unknown severity. Adequate coverage for the current single-rule Lint().

• [edge-case] internal/harness/lint.go:41 — Lint() does not guard against a nil receiver, but Validate() has the same pattern and the docstring explicitly requires calling after a successful Validate(). Consistent with existing convention.

• [code-organization] internal/harness/lint.go — Lint() returns []Diagnostic rather than error, departing from the Validate() pattern. This is architecturally intentional per ADR-0045's implementation note calling for a separate method for non-fatal diagnostics.

• [adr-immutability-compliance] docs/ADRs/0045-forge-portable-harness-schema.md — ADR-0045 has status Accepted. This PR makes two minor edits: (1) Line 145 changes Validate() to Lint(), (2) Lines 519-523 update future tense to present tense. Both are minor annotations permitted by AGENTS.md.

• [naming-alignment] docs/plans/adr-0045-forge-portable-harness-phase3.md — The new Phase 3 plan document filename follows the established convention from Phase 1 and Phase 2 plan documents.

Previous run (4)

Review

Findings

Low

• [documentation-comment-style] internal/harness/lint.go:13 — The String() method godoc says "implements fmt.Stringer" but the codebase pattern (e.g., internal/layers/layers.go:18) uses "returns a human-readable description of the status." The existing codebase does not use "implements" in godoc comments for String() methods.
  Remediation: Change comment to match the established pattern: // String returns a human-readable description of the diagnostic severity.

• [stale-implementation-note] docs/ADRs/0045-forge-portable-harness-schema.md:519 — The implementation note at lines 519-523 states "Phase 3 will need a separate Lint() method" using future tense, but this PR implements Phase 3 and adds the Lint() method. The note is now stale relative to the code.
  Remediation: Update lines 519-523 to use present tense reflecting that Lint() is now implemented.

Info

• [test-adequacy] internal/harness/lint_test.go — Tests cover the three key cases (role set, role empty, role+slug set) and Diagnostic.String() formatting including unknown severity. Adequate coverage for the current single-rule Lint().

• [edge-case] internal/harness/lint.go:41 — Lint() does not guard against a nil receiver, but Validate() has the same pattern and the docstring explicitly requires calling after a successful Validate(). Consistent with existing convention.

• [code-organization] internal/harness/lint.go — Lint() returns []Diagnostic rather than error, departing from the Validate() pattern. This is architecturally intentional per ADR-0045's implementation note calling for a separate method for non-fatal diagnostics.

Previous run (5)

Review

Findings

Low

• [missing-api-doc] docs/ADRs/0045-forge-portable-harness-schema.md:145 — The ADR states "Validate() emits warnings when role is missing" (line 145) but the implementation note at lines 519-523 already clarifies that a separate Lint() method is needed. Now that Lint() is being implemented, the earlier prose could be updated for consistency, though the ADR already self-corrects via the implementation note.
  Remediation: Consider updating line 145 to mention Lint() instead of Validate() for consistency, though this is a documentation polish item since the implementation note at lines 519-523 already provides the correct guidance.

Info

• [test-organization] internal/harness/lint_test.go — Test function naming follows the established TestType_Method pattern. TestLint tests the Lint method and TestDiagnostic_String tests the String method on Diagnostic type.

Previous run (6)

Review

Findings

Low

• [missing-doc] README.md:52 — The README lists implementation plans for ADR-0045 Phase 1 and Phase 2 but does not include a reference to the new Phase 3 plan document (docs/plans/adr-0045-forge-portable-harness-phase3.md) added in this PR.
  Remediation: Add a bullet point after the Phase 2 plan entry in README.md referencing the Phase 3 plan.

Info

• [test-organization] internal/harness/lint_test.go — Test function naming (TestLint, TestDiagnostic_String) follows the established TestType_Method pattern, addressing the prior review's suggestion to separate Diagnostic.String() tests from the TestLint function.

Previous run (7)

Review

Findings

Info

• [test-organization] internal/harness/lint_test.go:30 — Tests for Diagnostic.String() are grouped under TestLint. The established pattern in this codebase (e.g., TestLayerStatus_String in internal/layers/layers_test.go) is to create separate test functions for methods on different types.
  Remediation: Extract Diagnostic.String() tests to a separate TestDiagnostic_String function.

Previous run (8)

Review

Findings

High

• [missing-authorization] docs/plans/adr-0045-forge-portable-harness-phase3.md — PR adds non-trivial changes (340+ line implementation plan + new Go code with Lint() method) but has no linked issue. Non-trivial changes require explicit authorization via a linked issue.
  Remediation: Link this PR to an issue that authorizes the Phase 3 work for ADR-0045, or create a tracking issue.

Low

• [scope-alignment] internal/harness/lint.go — The PR combines the Phase 3 implementation plan document with the first implementation PR from that plan. The PR body explicitly acknowledges this bundling, so this is a minor style preference rather than accidental scope creep.
  Remediation: Consider whether the Phase 3 plan document should be a separate PR from the first implementation PR.

• [naming-convention] internal/harness/lint.go:13 — Function severityString is an unexported helper that takes a DiagnosticSeverity and returns a string — a textbook case for implementing the fmt.Stringer interface as a method on the type instead.
  Remediation: Convert to (s DiagnosticSeverity) String() string method to implement the Stringer interface.

• [error-handling-idiom] internal/harness/lint.go:38 — The Lint() method's godoc states "assumes the harness has already passed Validate" but there is no runtime guard. While a panic would be disproportionate for an advisory method, the precondition could be more prominent.
  Remediation: Document the precondition more prominently, or add a lightweight defensive check.

• [test-coverage] internal/harness/lint_test.go:9 — The test suite covers core paths well. The assert.Nil call on line 12 does verify the nil-return contract, but the "role empty" case lacks assert.NotNil(t, diags) to explicitly document the non-nil return contract.
  Remediation: Add assert.NotNil(t, diags) before the length/field checks in the "role empty" subtest.

Info

• [architectural-coherence] internal/harness/lint.go — The Lint() method design aligns well with ADR-0045's Phase 3 requirements. The ADR explicitly calls for a separate Lint() method, and the implementation correctly separates it from Validate().

• [code-organization] internal/harness/lint.go:13 — The severityString helper is placed between the const block and the Diagnostic type. Codebase convention places helpers after the types they operate on. See also: [naming-convention] finding at this location.

• [api-consistency] internal/harness/lint.go:38 — Lint() returns nil when no diagnostics are found, which is explicitly documented in the godoc. In Go, nil and empty slices are functionally equivalent for len/range/append, so this has no practical impact.

• [test-organization] internal/harness/lint_test.go:28 — Tests for Diagnostic.String() are grouped under TestLint. The codebase pattern of separate test functions per type could be followed by extracting to TestDiagnostic_String.

Previous run (9)

Review

Findings

Low

• [missing-doc] README.md:52 — The README lists implementation plans for ADR-0045 Phase 1 and Phase 2 but does not include a reference to the new Phase 3 plan document (docs/plans/adr-0045-forge-portable-harness-phase3.md) added in this PR.
  Remediation: Add a bullet point after the Phase 2 plan entry in README.md referencing the Phase 3 plan.

Info

• [test-organization] internal/harness/lint_test.go — Test function naming (TestLint, TestDiagnostic_String) follows the established TestType_Method pattern, addressing the prior review's suggestion to separate Diagnostic.String() tests from the TestLint function.

Previous run (10)

Review

Findings

Info

• [test-organization] internal/harness/lint_test.go:30 — Tests for Diagnostic.String() are grouped under TestLint. The established pattern in this codebase (e.g., TestLayerStatus_String in internal/layers/layers_test.go) is to create separate test functions for methods on different types.
  Remediation: Extract Diagnostic.String() tests to a separate TestDiagnostic_String function.

Previous run (11)

Review

Findings

High

• [missing-authorization] docs/plans/adr-0045-forge-portable-harness-phase3.md — PR adds non-trivial changes (340+ line implementation plan + new Go code with Lint() method) but has no linked issue. Non-trivial changes require explicit authorization via a linked issue.
  Remediation: Link this PR to an issue that authorizes the Phase 3 work for ADR-0045, or create a tracking issue.

Low

• [scope-alignment] internal/harness/lint.go — The PR combines the Phase 3 implementation plan document with the first implementation PR from that plan. The PR body explicitly acknowledges this bundling, so this is a minor style preference rather than accidental scope creep.
  Remediation: Consider whether the Phase 3 plan document should be a separate PR from the first implementation PR.

• [naming-convention] internal/harness/lint.go:13 — Function severityString is an unexported helper that takes a DiagnosticSeverity and returns a string — a textbook case for implementing the fmt.Stringer interface as a method on the type instead.
  Remediation: Convert to (s DiagnosticSeverity) String() string method to implement the Stringer interface.

• [error-handling-idiom] internal/harness/lint.go:38 — The Lint() method's godoc states "assumes the harness has already passed Validate" but there is no runtime guard. While a panic would be disproportionate for an advisory method, the precondition could be more prominent.
  Remediation: Document the precondition more prominently, or add a lightweight defensive check.

• [test-coverage] internal/harness/lint_test.go:9 — The test suite covers core paths well. The assert.Nil call on line 12 does verify the nil-return contract, but the "role empty" case lacks assert.NotNil(t, diags) to explicitly document the non-nil return contract.
  Remediation: Add assert.NotNil(t, diags) before the length/field checks in the "role empty" subtest.

Info

• [architectural-coherence] internal/harness/lint.go — The Lint() method design aligns well with ADR-0045's Phase 3 requirements. The ADR explicitly calls for a separate Lint() method, and the implementation correctly separates it from Validate().

• [code-organization] internal/harness/lint.go:13 — The severityString helper is placed between the const block and the Diagnostic type. Codebase convention places helpers after the types they operate on. See also: [naming-convention] finding at this location.

• [api-consistency] internal/harness/lint.go:38 — Lint() returns nil when no diagnostics are found, which is explicitly documented in the godoc. In Go, nil and empty slices are functionally equivalent for len/range/append, so this has no practical impact.

• [test-organization] internal/harness/lint_test.go:28 — Tests for Diagnostic.String() are grouped under TestLint. The codebase pattern of separate test functions per type could be followed by extracting to TestDiagnostic_String.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:21 PM UTC · Completed 12:29 PM UTC
Commit: 2856f29 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:42 PM UTC · Completed 12:53 PM UTC
Commit: 4e2f544 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:04 PM UTC · Completed 1:14 PM UTC
Commit: b46ee0a · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:26 PM UTC · Completed 1:40 PM UTC
Commit: 07ca307 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:59 PM UTC · Completed 2:11 PM UTC
Commit: cf654fa · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:29 PM UTC · Completed 2:40 PM UTC
Commit: 59062a6 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:51 PM UTC · Completed 3:01 PM UTC
Commit: de9ccd1 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:10 PM UTC · Completed 3:20 PM UTC
Commit: 3305c1a · View workflow run →

[ralphbean]

LGTM.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 7:23 PM UTC · Completed 7:31 PM UTC
Commit: 3305c1a · View workflow run →

[fullsend-ai-retro]

Retro: PR #2322 — feat(harness): add Lint() diagnostic method

Timeline

This human-authored PR added ~100 lines of Go code and ~340 lines of documentation. The review agent ran 9 times across 9 commit SHAs between 11:43 and 15:10 UTC. The PR was merged at 19:17 UTC after human approval from ralphbean at 16:36 UTC.

Observations

1. Drip-feed findings pattern — The first review (11:53) caught 3 low-severity issues (naming convention, error-handling idiom, test coverage). After the author fixed them, subsequent reviews surfaced 4 additional findings that should have been caught in the first pass: test-organization (run 2), documentation-comment-style (run 5), phase-boundary-clarity (run 6), and default-case-pattern (run 7). This drove unnecessary rework cycles.

2. Verdict instability — The review agent oscillated between APPROVED and CHANGES_REQUESTED across runs. Run 6 issued CHANGES_REQUESTED for a low-severity phase-boundary-clarity finding and an info-level edge-case comment — neither warranting a blocking verdict on a re-review.

3. Duplicate comment — The nil-receiver edge-case info finding was posted identically on runs 6 and 7, both concluding "consistent with existing convention, no change needed."

4. Token cost — 9 full review runs for a clean, well-tested 100-line Go change is disproportionate.

Proposals

No new proposals filed. All identified improvement opportunities are already tracked by existing open issues:

• #1582 — Review agent should catch all findings in the first pass to reduce rework cycles
• #1367 — Review agent surfaces new low-severity findings across multiple approved passes
• #1285 — Review agent should not regenerate unchanged inline comments on re-reviews
• #1013 — Review agent should deduplicate findings across iterations on the same PR
• #2029 — Review agent: use COMMENT verdict for re-reviews with only low-severity findings

This PR provides strong supporting evidence for the urgency of #1582 and #1367 in particular — the drip-feed pattern added ~7 hours of elapsed time and 8 unnecessary review runs to a clean, small change.