[LFXV2-2989] feat(weekly-brief): expose error_reason on brief payload for no_sources - #173
Conversation
When a committee has a quiet week (no meetings, members, mailing, or votes in the lookback window), the generator finalizes the brief as state="error" with internal reason "no_sources". Previously this reason was only logged; the API payload carried a bare error state, forcing clients to render generic error styling for a normal, non-failure outcome. Persist the finalization reason on the GroupWeeklyBrief domain model as ErrorReason, set it in finalizeError for both "no_sources" and "ai_error" cases, add the error_reason attribute to the Goa design (regenerated), and emit it conditionally in domainGroupWeeklyBriefToGoa. The BFF can now read error_reason="no_sources" and render a calm empty state instead of failure styling. Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
WalkthroughThe change records machine-readable error reasons on weekly briefs and exposes them through the committee API. Tests verify ChangesWeekly brief error reason propagation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Exposes persisted weekly-brief failure reasons so clients can distinguish quiet weeks from AI failures.
Changes:
- Adds and persists
error_reason. - Exposes the field through Goa-generated API types.
- Tests
no_sourcesandai_errorfinalization paths.
Reviewed changes
Copilot reviewed 7 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/service/group_weekly_brief_generator.go |
Persists finalization reasons. |
internal/service/group_weekly_brief_generator_test.go |
Verifies both reason codes. |
internal/domain/model/group_weekly_brief.go |
Adds the domain field. |
cmd/committee-api/design/type.go |
Defines the API attribute. |
cmd/committee-api/service/committee_service.go |
Converts the field to Goa output. |
gen/committee_service/service.go |
Adds the generated service field. |
gen/http/committee_service/server/types.go |
Updates server response types. |
gen/http/committee_service/server/encode_decode.go |
Marshals the field server-side. |
gen/http/committee_service/client/types.go |
Updates client response types. |
gen/http/committee_service/client/encode_decode.go |
Unmarshals the field client-side. |
gen/http/openapi.yaml |
Updates OpenAPI 2 documentation. |
gen/http/openapi.json |
Updates JSON OpenAPI output. |
gen/http/openapi3.yaml |
Updates OpenAPI 3 documentation. |
Files not reviewed (5)
- gen/committee_service/service.go: Generated file
- gen/http/committee_service/client/encode_decode.go: Generated file
- gen/http/committee_service/client/types.go: Generated file
- gen/http/committee_service/server/encode_decode.go: Generated file
- gen/http/committee_service/server/types.go: Generated file
…ontract
- Gate error_reason serialization on state=="error" in domainGroupWeeklyBriefToGoa
so an edited or generated brief never carries the field, even if the
persisted struct has a leftover non-empty value (e.g. after a PUT on an
errored brief)
- Remove dsl.Example("no_sources") from the error_reason design attribute;
it was combining with state's "generated" example in Goa-generated OpenAPI
output, producing a contradictory state+reason pair
- Add error_reason to the group_weekly_brief Data Schema in docs/indexer-contract.md
to keep the planned indexer contract in sync with the domain struct
Generated with [Claude Code](https://claude.ai/code)
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 17 changed files in this pull request and generated no new comments.
Files not reviewed (7)
- gen/committee_service/service.go: Generated file
- gen/http/cli/committee/cli.go: Generated file
- gen/http/committee_service/client/cli.go: Generated file
- gen/http/committee_service/client/encode_decode.go: Generated file
- gen/http/committee_service/client/types.go: Generated file
- gen/http/committee_service/server/encode_decode.go: Generated file
- gen/http/committee_service/server/types.go: Generated file
Suppressed comments (2)
cmd/committee-api/service/committee_service.go:2005
- The generator tests only verify that the reason reaches the persisted domain object; no service/API test exercises this new conditional response mapping. Add coverage showing an error brief returns
error_reason, and a non-error brief with a stale domain value omits it. Otherwise the PR's central API behavior—and the state gate added after the prior review—can regress while the current tests remain green.
if b.State == model.GroupWeeklyBriefStateError && b.ErrorReason != "" {
v := b.ErrorReason
out.ErrorReason = &v
cmd/committee-api/design/type.go:1220
- Removing the explicit field example did not fix the contradictory API examples: Goa now synthesizes an arbitrary
error_reason, while the regenerated OpenAPI still pairs it withstate: generated(for example,gen/http/openapi.yaml:5200-5203and:5523-5527). This contradicts this field's absent-on-non-error contract and does not tell consumers that the supported values areno_sourcesandai_error. Define coherent design/result examples (and expose the supported values in the schema), then regenerategen/.
dsl.Attribute("error_reason", dsl.String, "Machine-readable reason for the error state; absent on non-error briefs")
dealako
left a comment
dealako
left a comment
There was a problem hiding this comment.
Hi @andrest50 — thanks for the quick follow-up on the review feedback from the last round. This PR wires error_reason through the full stack (domain → generator → Goa conversion → generated types → contract docs) so clients can tell a quiet week (no_sources) apart from a generation failure (ai_error), and the fix commit already closed out two of the three issues Copilot raised previously (the stale-value gate and the contract doc).
I ran a security/privacy pass, a correctness/perf/test-coverage pass, and a style/API-compliance/docs pass in parallel, then reconciled against CodeRabbit/Copilot's existing comments.
🔴 Blocking: 0
🟡 Minor: 2
- No service-level test pins the
error_reasonexposure gate indomainGroupWeeklyBriefToGoa(cmd/committee-api/service/committee_service.go) — this is the exact behavior that needed a follow-up fix once already, and it's still only covered indirectly. - The generated OpenAPI examples still pair
error_reasonwithstate: generated(verified in currentgen/http/openapi.yaml) — dropping the field-levelExample()didn't stop Goa from synthesizing a contradictory value; this is the same gap Copilot flagged last round and it isn't closed yet.
⚪ Nit: 1
- The success-path generator test doesn't assert
ErrorReasonstays empty.
❔ Question: 0
Security and data-privacy review turned up nothing — the field is a machine-readable enum, gated on error state, never logged or persisted with PII, and test fixtures are synthetic.
I agree with Copilot's two still-open suppressed comments (test coverage and the example contradiction) and have restated them above as first-class findings since the fix commit didn't fully close them; everything else Copilot/CodeRabbit raised earlier already has a "Fixed" reply from you and checks out against current code.
✅ Approved with minor comments
| v := b.UpdatedAt.UTC().Format(time.RFC3339) | ||
| out.UpdatedAt = &v | ||
| } | ||
| if b.State == model.GroupWeeklyBriefStateError && b.ErrorReason != "" { |
There was a problem hiding this comment.
[minor] No service-level test for the error_reason exposure gate
Issue: domainGroupWeeklyBriefToGoa only exposes error_reason when b.State == model.GroupWeeklyBriefStateError && b.ErrorReason != "", but there is no test at this layer (checked cmd/committee-api/service/group_weekly_brief_test.go and committee_service_test.go) that exercises this conditional response mapping directly.
Proof: The generator tests (internal/service/group_weekly_brief_generator_test.go) only verify the domain object's State/ErrorReason, never the Goa conversion. Grepping the service test files for ErrorReason/domainGroupWeeklyBriefToGoa turns up no hits.
Why it matters: This exact gate was added in a follow-up fix commit (448dbb5) after review feedback that a PUT-transitioned brief could leak a stale error_reason. Without a test pinned on this function, that fix can silently regress while the rest of the suite stays green — this is the same concern Copilot raised on the prior round and it hasn't been closed with a test.
Fix: Add a table test for domainGroupWeeklyBriefToGoa (or the nearest exported wrapper) covering: (1) state=error + non-empty reason → exposed, (2) state=generated/edited with a stale non-empty ErrorReason on the domain object → omitted, (3) state=error + empty reason → omitted.
Agreement with bot: agree with Copilot's suppressed comment on this same line raising the same gap — restating here as a first-class finding since it wasn't resolved by the fix commit.
| dsl.Enum("empty", "generating", "generated", "edited", "approved", "error") | ||
| dsl.Example("generated") | ||
| }) | ||
| dsl.Attribute("error_reason", dsl.String, "Machine-readable reason for the error state; absent on non-error briefs") |
There was a problem hiding this comment.
[minor] Generated OpenAPI examples still pair error_reason with state: generated
Issue: Dropping dsl.Example("no_sources") from the error_reason attribute (commit 448dbb5) did not fix the contradictory generated example — Goa still synthesizes a placeholder value for the field and pairs it with state: generated in the regenerated spec.
Proof: Verified directly in the current gen/http/openapi.yaml (lines ~5203/5224, ~5270/5291, ~5558/5583): error_reason: Quidem qui voluptas dicta. sits in the same example object as state: generated. Same as Copilot flagged on the prior round on this line.
Why it matters: This attribute is documented as "absent on non-error briefs." A generated example that pairs it with a non-error state actively misleads API consumers reading the generated docs, and doesn't communicate the two supported values (no_sources, ai_error).
Fix: Define an explicit Result-level example (via dsl.Example on the type/result, not the individual attribute) that shows a coherent error-state brief with state: <error> and error_reason: no_sources (or ai_error), then regenerate gen/. Removing the field-level example alone doesn't stop Goa's example synthesizer from inventing a value for a required-looking field inside the existing non-error example.
Summary
ErrorReason string(json:"error_reason,omitempty") to theGroupWeeklyBriefdomain modelfinalizeError—"no_sources"for quiet-week briefs,"ai_error"for AI generation failureserror_reasonvia the Goa design (regenerated) anddomainGroupWeeklyBriefToGoaconverterErrorReasonon both error pathsPreviously, when a committee had no activity in the lookback window the API returned a bare
state: "error", indistinguishable from a real failure. The BFF and UI can now readerror_reason: "no_sources"and render a calm empty state ("Quiet week — no committee activity to summarize") instead of generic error styling.Ticket
LFXV2-2989
🤖 Generated with Claude Code