errorcounter: Allow provision of custom error counter

[andrewwormald]

Issue: #156

Allows the provision of a custom error counter as a build option.

[coderabbitai]

Walkthrough

This pull request promotes the error counter to the public API by adding an ErrorCounter interface in errors.go and adjusts the concrete implementation in internal/errorcounter (renamed to Counter). It adds a WithErrorCounter(ErrorCounter) BuildOption and threads the public ErrorCounter type through builder and workflow construction. Files including builder.go, pause.go, step.go, and workflow.go are updated to use the exported ErrorCounter. Tests (builder_internal_test.go) are added/updated to verify injection and the default counter initialisation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• builder.go — verify WithErrorCounter wiring and default initialisation in NewBuilder/Build
• builder_internal_test.go — confirm tests assert injected and default behaviour and adjusted mocks
• internal/errorcounter/errorcounter.go — review rename to Counter and constructor signature changes
• errors.go — confirm ErrorCounter interface matches implementations
• pause.go, step.go, workflow.go — ensure imports and type references updated consistently

Poem

I nibble code in morning light,
A counter freed from hidden night,
Built options cosy, snug and neat,
Errors tallied, hop by beat,
A tiny rabbit cheers the feat! 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: enabling custom error counter provision as a build option, which aligns with the changeset's core functionality.
Description check	✅ Passed	The description is directly related to the changeset, referencing the relevant issue and explaining the purpose of allowing custom error counter provision.

✨ Finishing touches

• 📝 Docstrings were successfully generated.

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch andreww-issues-156-implement-configurable-error-counter

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 988e7be and 24989fb.

📒 Files selected for processing (1)

• builder_internal_test.go (6 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

builder_internal_test.go (3)

internal/errorcounter/errorcounter.go (1)

• New (8-12)

builder.go (2)

• WithErrorCounter (334-338)
• NewBuilder (25-43)

connector.go (1)

• ConnectorConsumer (17-20)

🔇 Additional comments (3)

builder_internal_test.go (3)

304-306: Updated mock connector signatures look good

Renaming the unused parameters in mockConnector.Make and mockExternalConsumer.Recv to _ to match the ConnectorConsumer/connector interfaces while silencing unused-parameter warnings is clean and idiomatic. No functional concerns here.

Also applies to: 310-312

---

569-585: Good coverage for WithErrorCounter and default error counter initialisation

The dedicated tests for injecting a custom error counter and for ensuring a non-nil default errorCounter on Build nicely exercise the new configuration path and defaulting behaviour. Once the type/equality concern in the previous comment is addressed, these tests should give solid regression coverage for the new option.

After adjusting the variable typing/equality as suggested, please confirm go test ./internal/workflow/... (or your usual test command) passes to validate the wiring end-to-end.

---

120-120: ErrorCounter type is not defined in the codebase, but the type-safety concern is a valid Go pattern issue

The review identifies a legitimate Go typing anti-pattern: comparing concrete pointers against interface-typed fields using require.Equal with reflect.DeepEqual. This concern is valid regardless of current state.

Critical finding:
The ErrorCounter type referenced in workflow.go (line 48) as the field type for errorCounter is not defined anywhere in the repository. The codebase appears incomplete:

• builder_internal_test.go creates concrete *errorcounter.Counter instances (lines 120, 147, 564)
• Tests assert equality against wf.errorCounter (typed as undefined ErrorCounter)
• No interface or type definition for ErrorCounter exists in the visible codebase

The suggested pattern is still correct:

var customErrorCounter ErrorCounter = errorcounter.New()

This explicit typing avoids interface-wrapping confusion and makes intent clear, whether ErrorCounter is an interface or concrete type.

However, full verification requires:

1. The ErrorCounter type definition (currently missing)
2. Confirmation the code compiles and tests pass/fail
3. Understanding whether this is a new feature being added or incomplete branch state

The review's concern about type safety is sound Go practice, but cannot be definitively validated without the missing type definition.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

errors.go (1)

13-18: Clarify ErrorCounter’s contract (concurrency + return semantics)

Now that ErrorCounter is part of the public API, it would help implementers if the doc comment explicitly stated that:

• Implementations are expected to be safe for concurrent use by multiple goroutines.
• Add returns the updated count for the (err, labels...) combination.
• Count returns the current count.
• Clear resets the count (and whether it is acceptable to be a no-op if the key doesn’t exist).

This keeps custom implementations aligned with how the built-in counter is used throughout the workflow runtime.

internal/errorcounter/errorcounter.go (1)

8-12: Keep Counter as-is, but consider centralising key construction

The move to a concrete *Counter with mutex-protected Add/Count/Clear preserves existing behaviour and concurrency-safety.

As a small internal clean-up, you could factor out the repeated errMsg construction into a private helper (e.g. key(err error, labels ...string) string) so any future change to key format is made in one place only.

Also applies to: 14-17, 19-45

builder.go (1)

25-42: ErrorCounter wiring via builder is correct; consider softening the doc comment

The end-to-end wiring looks solid:

• NewBuilder sets a sensible default via errorcounter.New().
• buildOptions.errorCounter plus WithErrorCounter give a straightforward DI hook.
• Build only overrides workflow.errorCounter when a custom value is provided, so the default stays in place otherwise.
• Tests exercise both the custom and default paths.

One minor polish point: the WithErrorCounter comment currently references the internal package (errorcounter.New()). Since that package isn’t importable by external users, you might rephrase this to something like “The default is an in‑memory error counter created by NewBuilder.” to avoid exposing internal details in the public API docs.

Also applies to: 248-250, 269-278, 333-338

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1a633fe and 988e7be.

📒 Files selected for processing (7)

• builder.go (3 hunks)
• builder_internal_test.go (5 hunks)
• errors.go (1 hunks)
• internal/errorcounter/errorcounter.go (3 hunks)
• pause.go (1 hunks)
• step.go (1 hunks)
• workflow.go (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (5)

step.go (1)

errors.go (1)

• ErrorCounter (14-18)

workflow.go (1)

errors.go (1)

• ErrorCounter (14-18)

builder_internal_test.go (2)

internal/errorcounter/errorcounter.go (1)

• New (8-12)

builder.go (2)

• WithErrorCounter (334-338)
• NewBuilder (25-43)

builder.go (1)

errors.go (1)

• ErrorCounter (14-18)

pause.go (1)

errors.go (1)

• ErrorCounter (14-18)

🔇 Additional comments (4)

workflow.go (1)

95-102: Decoupling Workflow from internal errorcounter looks good

Switching errorCounter to the public ErrorCounter interface cleanly removes the dependency on the internal package and aligns with WithErrorCounter/NewBuilder wiring. No behavioural issues spotted here.

pause.go (1)

13-21: maybePause correctly switched to the public ErrorCounter

Updating the counter parameter to ErrorCounter cleanly hooks the injected/custom counter into the pause logic without altering behaviour. Call sites and usage (Add/Clear) remain consistent.

step.go (1)

65-101: ErrorCounter injection into stepConsumer is consistent

Passing w.errorCounter into stepConsumer and changing the parameter type to ErrorCounter cleanly wires the configurable counter through to the step logic. The integration with maybePause is consistent and doesn’t alter existing behaviour.

Also applies to: 103-114

builder_internal_test.go (1)

18-21: Tests give good coverage of custom/default error counter wiring

The added assertions in TestBuildOptions plus the new TestWithErrorCounter and TestDefaultErrorCounter nicely verify:

• A provided counter is plumbed through as‑is.
• A default, non‑nil counter exists when none is supplied.

This should catch regressions in the builder wiring around ErrorCounter.

Also applies to: 115-170, 569-585

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[coderabbitai]

Note

Docstrings generation - SUCCESS
Generated docstrings for this pull request at #160