fix(review): address PR #126 review feedback (iteration 2)

Address review comments from @dealako, @prabodhcs, coderabbitai[bot],
copilot[bot]:

- committee_service.go: reject a missing Heimdall principal with 400 before
  the writer is called, rather than persisting an empty last_edited_by —
  matches the sibling write handlers (CreateCommitteeLink, etc.) (per
  @prabodhcs, @dealako, coderabbitai[bot]).
- committee_service.go: move the nil-writer guard ahead of GetBase so a
  misconfigured service fails fast without a wasted storage read (per
  @dealako, @prabodhcs).
- group_weekly_brief_writer.go: reject revision==0 as a 400 before the
  revision comparison — a persisted brief always has a KV revision >= 1, so
  a 0 token is a malformed/missing client value, not a conflict (per
  copilot[bot], @prabodhcs).
- design + regen: add dsl.Minimum(1) to the request and response revision
  attributes so an invalid token is rejected as 400 at the transport layer
  and the >=1 invariant is reflected in the schema (per copilot[bot]).
- group_weekly_brief_writer.go: normalize a zero Now to time.Now().UTC(),
  matching the generator, so an omitted Now uses the live window (per
  copilot[bot]).
- group_weekly_brief_writer.go: document why the option constructors carry
  the ForWriter suffix (package-level name collision with the reader/
  generator options; mirrors the generator's ForGenerator) (per @dealako).
- tests: add handler missing-principal -> 400; writer CAS-conflict +
  re-read-fails -> propagate original; writer revision==0 -> 400 (per
  @dealako).

Resolves 11 review threads; 1 (index-write-after-CAS edge) tracked as a
follow-up.

LFXV2-2194

Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>

Author: manishdixitlfx

cmd/committee-api/design/committee.go

@@ -1564,6 +1564,7 @@ var _ = dsl.Service("committee-service", func() {
 				dsl.Example("## This week\n\n- Shipped the thing.")
 			})
 			dsl.Attribute("revision", dsl.UInt64, "Optimistic-concurrency token from the brief being edited (GET /current)", func() {
+				dsl.Minimum(1)
 				dsl.Example(uint64(7))
 			})
 

cmd/committee-api/design/type.go

@@ -1133,6 +1133,7 @@ var GroupWeeklyBriefWithReadonlyAttributes = dsl.Type("group-weekly-brief-with-r
 		dsl.Example("jsmith")
 	})
 	dsl.Attribute("revision", dsl.UInt64, "Optimistic-concurrency token. Echo this back in PUT /current; a stale value yields 409.", func() {
+		dsl.Minimum(1)
 		dsl.Example(uint64(7))
 	})
 })

cmd/committee-api/service/committee_service.go

@@ -1785,6 +1785,11 @@ func (s *committeeServicesrvc) UpdateCurrentWeeklyBrief(ctx context.Context, p *
 	// Authorization (committee writer relation) is enforced at the edge by
 	// Heimdall before the request reaches this service; no in-code check here.
 
+	// Fail fast on misconfiguration before any storage I/O.
+	if s.weeklyBriefWriter == nil {
+		return nil, wrapError(ctx, errors.NewServiceUnavailable("weekly brief writer is not configured"))
+	}
+
 	// Verify the committee exists so a typo'd UID returns 404 for the committee
 	// rather than 404 for a missing brief.
 	base, _, err := s.committeeReaderOrchestrator.GetBase(ctx, p.UID)
@@ -1795,13 +1800,14 @@ func (s *committeeServicesrvc) UpdateCurrentWeeklyBrief(ctx context.Context, p *
 		return nil, wrapError(ctx, errors.NewNotFound("committee not found"))
 	}
 
-	if s.weeklyBriefWriter == nil {
-		return nil, wrapError(ctx, errors.NewServiceUnavailable("weekly brief writer is not configured"))
-	}
-
 	// PrincipalContextID is the caller's LFX username (Heimdall principal claim),
-	// recorded as last_edited_by.
+	// recorded as last_edited_by. Reject a missing principal rather than persist
+	// an empty editor — this endpoint's audit trail depends on it, and it mirrors
+	// the guard the sibling write handlers use (CreateCommitteeLink, etc.).
 	editedBy, _ := ctx.Value(constants.PrincipalContextID).(string)
+	if editedBy == "" {
+		return nil, wrapError(ctx, errors.NewValidation("unable to determine user identity from token"))
+	}
 
 	updated, err := s.weeklyBriefWriter.Update(ctx, service.GroupWeeklyBriefUpdateInput{
 		CommitteeUID: p.UID,

cmd/committee-api/service/group_weekly_brief_test.go

@@ -440,7 +440,8 @@ func TestUpdateCurrentWeeklyBrief_RevisionConflict(t *testing.T) {
 	writer := &stubGroupWeeklyBriefWriter{err: errors.NewRevisionMismatch(8)}
 	svc := newUpdateSvc(&model.CommitteeBase{}, writer)
 
-	res, err := svc.UpdateCurrentWeeklyBrief(context.Background(), &committeeservice.UpdateCurrentWeeklyBriefPayload{
+	ctx := context.WithValue(context.Background(), constants.PrincipalContextID, "alice")
+	res, err := svc.UpdateCurrentWeeklyBrief(ctx, &committeeservice.UpdateCurrentWeeklyBriefPayload{
 		UID: "c-1", BriefText: "x", Revision: 7,
 	})
 	require.Error(t, err)
@@ -456,7 +457,8 @@ func TestUpdateCurrentWeeklyBrief_BriefNotFound(t *testing.T) {
 	writer := &stubGroupWeeklyBriefWriter{err: errors.NewNotFound("no weekly brief exists for the current window")}
 	svc := newUpdateSvc(&model.CommitteeBase{}, writer)
 
-	res, err := svc.UpdateCurrentWeeklyBrief(context.Background(), &committeeservice.UpdateCurrentWeeklyBriefPayload{
+	ctx := context.WithValue(context.Background(), constants.PrincipalContextID, "alice")
+	res, err := svc.UpdateCurrentWeeklyBrief(ctx, &committeeservice.UpdateCurrentWeeklyBriefPayload{
 		UID: "c-1", BriefText: "x", Revision: 1,
 	})
 	require.Error(t, err)
@@ -470,7 +472,8 @@ func TestUpdateCurrentWeeklyBrief_EmptyBriefTextBadRequest(t *testing.T) {
 	writer := &stubGroupWeeklyBriefWriter{err: errors.NewValidation("brief_text is required")}
 	svc := newUpdateSvc(&model.CommitteeBase{}, writer)
 
-	res, err := svc.UpdateCurrentWeeklyBrief(context.Background(), &committeeservice.UpdateCurrentWeeklyBriefPayload{
+	ctx := context.WithValue(context.Background(), constants.PrincipalContextID, "alice")
+	res, err := svc.UpdateCurrentWeeklyBrief(ctx, &committeeservice.UpdateCurrentWeeklyBriefPayload{
 		UID: "c-1", BriefText: "", Revision: 1,
 	})
 	require.Error(t, err)
@@ -479,6 +482,23 @@ func TestUpdateCurrentWeeklyBrief_EmptyBriefTextBadRequest(t *testing.T) {
 	require.ErrorAs(t, err, &br)
 }
 
+func TestUpdateCurrentWeeklyBrief_MissingPrincipalBadRequest(t *testing.T) {
+	// No principal in context (e.g. a misconfigured edge) must not persist an
+	// empty last_edited_by — the handler rejects it with 400 before calling the
+	// writer, matching the sibling write handlers (CreateCommitteeLink, etc.).
+	writer := &stubGroupWeeklyBriefWriter{}
+	svc := newUpdateSvc(&model.CommitteeBase{}, writer)
+
+	res, err := svc.UpdateCurrentWeeklyBrief(context.Background(), &committeeservice.UpdateCurrentWeeklyBriefPayload{
+		UID: "c-1", BriefText: "x", Revision: 1,
+	})
+	require.Error(t, err)
+	assert.Nil(t, res)
+	var br *committeeservice.BadRequestError
+	require.ErrorAs(t, err, &br)
+	assert.False(t, writer.called, "writer must not be called when the principal is missing")
+}
+
 func TestUpdateCurrentWeeklyBrief_WriterNotConfigured(t *testing.T) {
 	// A nil writer is a misconfiguration → 503.
 	svc := &committeeServicesrvc{

gen/http/committee_service/client/cli.go

@@ -4558,6 +4558,7 @@ definitions:
                 description: Optimistic-concurrency token from the brief being edited (GET /current)
                 example: 7
                 format: int64
+                minimum: 1
         example:
             brief_text: |-
                 ## This week
@@ -5162,6 +5163,7 @@ definitions:
                 description: Optimistic-concurrency token. Echo this back in PUT /current; a stale value yields 409.
                 example: 7
                 format: int64
+                minimum: 1
             source_refs:
                 type: array
                 items: