docs: tighten code comments and add comment guidance to code style

Remove issue-number references and reviewer-facing context from the inline
and XML doc comments added for the consumer-pump observability change, and
narrow the remaining comments to explaining the code itself.

Add a Comments section to .agent_instructions/code_style.md: no issue/PR links
in comments, comments explain the why of non-obvious code (not the what or the
change history), reviewer rationale belongs in the ADR, keep comments concise.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: iancooper

.agent_instructions/code_style.md

@@ -36,4 +36,9 @@
   - As we support multiple schedulers, these should use their own assembly.
   - As we support multiple locking providers, these should use their own assembly.  
 - Default to a class per source file approach, unless one class clearly exists as the details of another.
+- Comments
+  - Do not put links or references to issues or PRs (e.g. `(issue #4089)`, `#4207`) in code or XML doc comments. The traceability lives in git history and the ADR, not in the source.
+  - A comment is for code that needs explanation — typically the *why* behind a non-obvious decision. Do not narrate *what* the code does, restate the code, or describe the change relative to how the code used to be.
+  - Inline comments are not notes to a reviewer. Context about the fix, the alternatives considered, the previous behaviour, or the history of a change is reviewer-facing rationale and belongs in the ADR, not in an inline comment. A future maintainer reading the code only needs what helps them understand the code as it now stands.
+  - Keep comments concise. Prefer one tight line over a paragraph.
 

src/Paramore.Brighter.ServiceActivator/Proactor.cs

@@ -153,7 +153,7 @@ private async Task EventLoop()
                     // receive span covers only the broker call so its Duration reflects broker latency, not dispatch
                     Activity? receiveSpan = null;
                     Message? message = null;
-                    // the header is serialized once for the receive span and reused by the process span (issue #4089)
+                    // serialized once on the receive span and reused by the process span
                     string? headerJson = null;
                     try
                     {
@@ -162,7 +162,6 @@ private async Task EventLoop()
                             receiveSpan = Tracer?.CreateReceiveSpan(Channel.RoutingKey, MessagingSystem.InternalBus, InstrumentationOptions);
                             message = await Channel.ReceiveAsync(TimeOut);
                             headerJson = Tracer?.EnrichReceiveSpan(receiveSpan, message, InstrumentationOptions);
-                            // propagate consumer baggage once per message, independent of how many spans we create
                             Tracer?.PropagateConsumerContext(message);
                         }
                         catch (ChannelFailureException ex) when (ex.InnerException is BrokenCircuitException)

src/Paramore.Brighter.ServiceActivator/Reactor.cs

@@ -112,7 +112,7 @@ public void Run()
                     // receive span covers only the broker call so its Duration reflects broker latency, not dispatch
                     Activity? receiveSpan = null;
                     Message? message = null;
-                    // the header is serialized once for the receive span and reused by the process span (issue #4089)
+                    // serialized once on the receive span and reused by the process span
                     string? headerJson = null;
                     try
                     {
@@ -121,7 +121,6 @@ public void Run()
                             receiveSpan = Tracer?.CreateReceiveSpan(Channel.RoutingKey, MessagingSystem.InternalBus, InstrumentationOptions);
                             message = Channel.Receive(TimeOut);
                             headerJson = Tracer?.EnrichReceiveSpan(receiveSpan, message, InstrumentationOptions);
-                            // propagate consumer baggage once per message, independent of how many spans we create
                             Tracer?.PropagateConsumerContext(message);
                         }
                         catch (ChannelFailureException ex) when (ex.InnerException is BrokenCircuitException)

src/Paramore.Brighter/Observability/BrighterTracer.cs

@@ -189,8 +189,7 @@ public void AddExceptionToSpan(Activity? span, IEnumerable<Exception> exceptions
             tags.Add(BrighterSemanticConventions.MessageId, message.Id.Value);
             tags.Add(BrighterSemanticConventions.MessageType, message.Header.MessageType.ToString());
             tags.Add(BrighterSemanticConventions.MessageBodySize, message.Body.Memory.Length);
-            //reuse the header serialized once for the receive span (issue #4089); when this span is created without a
-            //preceding receive span (none threaded in) we serialize the as-received header here instead
+            //reuse the header the receive span already serialized; serialize here only when there was no receive span
             tags.Add(BrighterSemanticConventions.MessageHeaders,
                 serializedHeader ?? JsonSerializer.Serialize(message.Header, JsonSerialisationOptions.Options));
             tags.Add(BrighterSemanticConventions.ConversationId, message.Header.CorrelationId.Value);
@@ -214,9 +213,6 @@ public void AddExceptionToSpan(Activity? span, IEnumerable<Exception> exceptions
 
         try
         {
-            //baggage propagation (correlationId, SetBaggage) is no longer done here: it is a consumer-context concern,
-            //done once per message by the pump via PropagateConsumerContext, not per span. TraceStateString is
-            //per-activity so it stays here.
             activity.TraceStateString = traceState;
 
             Activity.Current = activity;
@@ -225,8 +221,7 @@ public void AddExceptionToSpan(Activity? span, IEnumerable<Exception> exceptions
         }
         catch (Exception ex)
         {
-            //the activity has already been started; if enriching it throws the caller never receives it and so
-            //cannot end it via its try/finally, so we must end it here to avoid leaking a started activity
+            //the activity is already started; end it here so a throw during enrichment cannot leak it past the caller
             activity.SetStatus(ActivityStatusCode.Error, ex.Message);
             EndSpan(activity);
             throw;
@@ -305,8 +300,7 @@ public void AddExceptionToSpan(Activity? span, IEnumerable<Exception> exceptions
             span.AddTag(BrighterSemanticConventions.MessageId, message.Id.Value);
             span.AddTag(BrighterSemanticConventions.MessageType, message.Header.MessageType.ToString());
             span.AddTag(BrighterSemanticConventions.MessageBodySize, message.Body.Memory.Length);
-            //serialize the as-received header once; the caller threads this string into CreateSpan(Process, ...) so the
-            //process span reuses it rather than serializing the header a second time per message (issue #4089)
+            //returned so the process span can reuse it instead of serializing the header again
             serializedHeader = JsonSerializer.Serialize(message.Header, JsonSerialisationOptions.Options);
             span.AddTag(BrighterSemanticConventions.MessageHeaders, serializedHeader);
             span.AddTag(BrighterSemanticConventions.ConversationId, message.Header.CorrelationId.Value);
@@ -315,9 +309,7 @@ public void AddExceptionToSpan(Activity? span, IEnumerable<Exception> exceptions
         if (options.HasFlag(InstrumentationOptions.RequestBody))
             span.AddTag(BrighterSemanticConventions.MessageBody, message.Body.Value);
 
-        // Propagate the producer's tracestate onto the receive span. Done here (not in CreateReceiveSpan) because this value
-        // comes from the message and isn't known until the broker call returns. TraceStateString is per-activity; consumer
-        // baggage propagation is a separate, once-per-message concern handled by PropagateConsumerContext.
+        // not set in CreateReceiveSpan because the tracestate comes from the message, unknown until the broker call returns
         if (!string.IsNullOrEmpty(message.Header.TraceState?.Value))
             span.TraceStateString = message.Header.TraceState!.Value;
 
@@ -327,8 +319,7 @@ public void AddExceptionToSpan(Activity? span, IEnumerable<Exception> exceptions
     /// <summary>
     /// Propagates the producer's baggage onto the consumer side for a received message: lifts the message's
     /// <see cref="MessageHeader.CorrelationId"/> into its <see cref="MessageHeader.Baggage"/> and sets it as the ambient
-    /// OpenTelemetry baggage for the current execution context. Called once per received message by the pump — independent
-    /// of how many spans (receive, process) are created — so the propagation runs exactly once rather than per span.
+    /// OpenTelemetry baggage for the current execution context. Called once per received message by the pump.
     /// </summary>
     /// <param name="message">The <see cref="Message"/> that was received</param>
     public void PropagateConsumerContext(Message message)

src/Paramore.Brighter/Observability/IAmABrighterTracer.cs

@@ -45,9 +45,8 @@ public interface IAmABrighterTracer : IDisposable
     /// <param name="message">What is the <see cref="Message"/> that we received; if they have a traceparentid we will use that as a parent for this trace</param>
     /// <param name="messagingSystem">What is the messaging system that we are receiving a message from</param>
     /// <param name="options">The <see cref="InstrumentationOptions"/> for how deep should the instrumentation go</param>
-    /// <param name="serializedHeader">The header serialized once by <see cref="EnrichReceiveSpan"/> for the receive span,
-    /// threaded in so the process span reuses it instead of serializing the header a second time per message (issue #4089);
-    /// when null (no preceding receive span) the header is serialized here</param>
+    /// <param name="serializedHeader">The header already serialized by <see cref="EnrichReceiveSpan"/>, reused here instead
+    /// of serializing it again; when null (no preceding receive span) the header is serialized by this method</param>
     /// <returns></returns>
     Activity? CreateSpan(
         MessagePumpSpanOperation operation,
@@ -78,8 +77,8 @@ public interface IAmABrighterTracer : IDisposable
     /// <param name="span">The receive span to enrich; no-op if null</param>
     /// <param name="message">The <see cref="Message"/> that was received</param>
     /// <param name="options">The <see cref="InstrumentationOptions"/> for how deep should the instrumentation go</param>
-    /// <returns>The header serialized once for this message (so it can be threaded into <see cref="CreateSpan(MessagePumpSpanOperation,Message,MessagingSystem,InstrumentationOptions,string)"/>
-    /// and not serialized again), or null if there was no span to enrich or messaging instrumentation is disabled</returns>
+    /// <returns>The serialized header, to pass to <see cref="CreateSpan(MessagePumpSpanOperation,Message,MessagingSystem,InstrumentationOptions,string)"/>
+    /// so it is not serialized again; null if there was no span to enrich or messaging instrumentation is disabled</returns>
     string? EnrichReceiveSpan(
         Activity? span,
         Message message,
@@ -88,8 +87,7 @@ public interface IAmABrighterTracer : IDisposable
     /// <summary>
     /// Propagates the producer's baggage onto the consumer side for a received message: lifts the message's
     /// <see cref="MessageHeader.CorrelationId"/> into its <see cref="MessageHeader.Baggage"/> and sets it as the ambient
-    /// OpenTelemetry baggage for the current execution context. Called once per received message by the pump so the
-    /// propagation runs exactly once, independent of how many spans (receive, process) are created.
+    /// OpenTelemetry baggage for the current execution context. Called once per received message by the pump.
     /// </summary>
     /// <param name="message">The <see cref="Message"/> that was received</param>
     void PropagateConsumerContext(Message message);

tests/Paramore.Brighter.Core.Tests/Observability/MessageDispatch/When_A_Message_Has_A_Malformed_Correlation_Id_The_Pump_Continues.cs

@@ -85,8 +85,7 @@ public MalformedCorrelationIdPumpObservabilityTests()
                 Channel = channel, TimeOut = TimeSpan.FromMilliseconds(5000)
             };
 
-            //a serviceable message whose correlation id is rejected by W3C Baggage validation: propagating it as baggage
-            //throws, but observability must never tear down the pump - the message should still be dispatched
+            //"bad=value" is rejected by W3C Baggage validation, so propagating it as baggage throws
             var message = new Message(
                 new MessageHeader(_myEvent.Id, _routingKey, MessageType.MT_EVENT, correlationId: new Id("bad=value")),
                 new MessageBody(JsonSerializer.Serialize(_myEvent, JsonSerialisationOptions.Options))
@@ -100,10 +99,10 @@ public MalformedCorrelationIdPumpObservabilityTests()
         [Fact]
         public void When_a_message_has_a_malformed_correlation_id_the_pump_continues()
         {
-            //Act - a malformed correlation id makes baggage propagation throw; the pump must not propagate that out
+            //Act
             var exception = Record.Exception(() => _messagePump.Run());
 
-            //Assert - the pump ran to its QUIT message without tearing down, and the poisoned message was still dispatched
+            //Assert - the pump reached its QUIT message and still dispatched the poisoned message
             Assert.Null(exception);
             Assert.True(_receivedMessages.ContainsKey(nameof(MyEventHandler)));
             Assert.Equal(MessagePumpStatus.MP_STOPPED, _messagePump.Status);

tests/Paramore.Brighter.Core.Tests/Observability/MessageDispatch/When_A_Message_Is_Dispatched_The_Header_Is_Serialized_Once.cs

@@ -103,7 +103,6 @@ public MessageHeaderSerializationObservabilityTests()
                 Channel = channel, TimeOut = TimeSpan.FromMilliseconds(5000)
             };
 
-            //a single serviceable message (MT_EVENT) is the steady-state hot path the perf regression affects
             _message = new Message(
                 new MessageHeader(_myEvent.Id, _routingKey, MessageType.MT_EVENT, replyTo: new RoutingKey("io.paramorebrighter.myevent")),
                 new MessageBody(JsonSerializer.Serialize(_myEvent, JsonSerialisationOptions.Options))
@@ -122,7 +121,6 @@ public void When_a_message_is_dispatched_the_header_is_serialized_once_for_both_
             _traceProvider.ForceFlush();
 
             // Assert
-            //a serviceable message produces both a receive span (broker-call timing) and a process span (dispatch timing)
             var receiveSpan = _exportedActivities.FirstOrDefault(a =>
                 a.DisplayName == $"{_message.Header.Topic} {MessagePumpSpanOperation.Receive.ToSpanName()}"
                 && a.TagObjects.Any(to => to is { Value: not null, Key: BrighterSemanticConventions.MessageType } && Enum.Parse<MessageType>(to.Value.ToString() ?? string.Empty) == MessageType.MT_EVENT));
@@ -133,17 +131,13 @@ public void When_a_message_is_dispatched_the_header_is_serialized_once_for_both_
                 && a.TagObjects.Any(to => to is { Value: not null, Key: BrighterSemanticConventions.MessageType } && Enum.Parse<MessageType>(to.Value.ToString() ?? string.Empty) == MessageType.MT_EVENT));
             Assert.NotNull(processSpan);
 
-            //both spans must still carry the serialized header with the correct value (diagnostic contract preserved)
             var expectedHeaderJson = JsonSerializer.Serialize(_message.Header, JsonSerialisationOptions.Options);
             var receiveHeaderJson = receiveSpan!.TagObjects.Single(to => to.Key == BrighterSemanticConventions.MessageHeaders).Value;
             var processHeaderJson = processSpan!.TagObjects.Single(to => to.Key == BrighterSemanticConventions.MessageHeaders).Value;
             Assert.Equal(expectedHeaderJson, receiveHeaderJson);
             Assert.Equal(expectedHeaderJson, processHeaderJson);
 
-            //the header must be serialized only ONCE per serviceable message: both spans share the SAME string instance.
-            //Today the receive span (EnrichReceiveSpan) and the process span (CreateSpan) each call
-            //JsonSerializer.Serialize(message.Header, ...) independently, producing two distinct (never-interned)
-            //string instances - so this reference-equality assertion fails until the serialization is computed once and reused.
+            //both spans share one serialized header (Assert.Same: a never-interned string, so equal references prove reuse)
             Assert.Same(receiveHeaderJson, processHeaderJson);
         }
     }

tests/Paramore.Brighter.Core.Tests/Observability/MessageDispatch/When_A_Message_With_A_Correlation_Id_Is_Dispatched_Both_Spans_Share_One_Header.cs

@@ -103,8 +103,6 @@ public MessageHeaderCorrelationIdObservabilityTests()
                 Channel = channel, TimeOut = TimeSpan.FromMilliseconds(5000)
             };
 
-            //a serviceable message carrying a correlation id - the value the reviewer noted could be dropped from the
-            //process span when it diverged from the receive span (issue #4089 review, point 1)
             _message = new Message(
                 new MessageHeader(_myEvent.Id, _routingKey, MessageType.MT_EVENT, correlationId: new Id(CorrelationId),
                     replyTo: new RoutingKey("io.paramorebrighter.myevent")),
@@ -137,17 +135,14 @@ public void When_a_message_with_a_correlation_id_is_dispatched_both_spans_share_
             var receiveHeaderJson = (string?)receiveSpan!.TagObjects.Single(to => to.Key == BrighterSemanticConventions.MessageHeaders).Value;
             var processHeaderJson = (string?)processSpan!.TagObjects.Single(to => to.Key == BrighterSemanticConventions.MessageHeaders).Value;
 
-            //the correlation id is conveyed by the serialized header (as the top-level CorrelationId field) so it is NOT
-            //lost from either span; it also rides on the dedicated ConversationId tag below
+            //the correlation id is carried in the serialized header as the top-level CorrelationId field
             Assert.Contains(CorrelationId, receiveHeaderJson);
             Assert.Contains(CorrelationId, processHeaderJson);
 
-            //both spans must carry the IDENTICAL serialized header (the as-received header), serialized exactly once and
-            //shared - so the process span never diverges from the receive span (issue #4089 review, point 1)
+            //both spans share one serialized header (Assert.Same: a never-interned string, so equal references prove reuse)
             Assert.Equal(receiveHeaderJson, processHeaderJson);
             Assert.Same(receiveHeaderJson, processHeaderJson);
 
-            //the correlation id is also exposed as the dedicated conversation id tag on both spans
             Assert.Equal(CorrelationId, receiveSpan.TagObjects.Single(to => to.Key == BrighterSemanticConventions.ConversationId).Value);
             Assert.Equal(CorrelationId, processSpan.TagObjects.Single(to => to.Key == BrighterSemanticConventions.ConversationId).Value);
         }

tests/Paramore.Brighter.Core.Tests/Observability/MessageDispatch/When_Creating_A_Process_Span_Baggage_Propagation_Is_Decoupled.cs

@@ -51,25 +51,22 @@ public CreateProcessSpanBaggageDecoupledObservabilityTests()
     [Fact]
     public void When_creating_a_process_span_baggage_propagation_is_decoupled()
     {
-        //Arrange - a correlation id whose value is rejected by Baggage validation. Baggage propagation is no longer done
-        //inside CreateSpan (it moved to PropagateConsumerContext, called once per message by the pump), so creating the
-        //process span must NOT touch baggage and therefore must NOT throw - the started activity can never leak this way.
+        //"bad=value" is rejected by Baggage validation; CreateSpan no longer propagates baggage, so it must not throw
         var message = new Message(
             new MessageHeader(Id.Random(), _routingKey, MessageType.MT_EVENT, correlationId: new Id("bad=value")),
             new MessageBody("{}"));
 
         //Act
         var processSpan = _tracer.CreateSpan(MessagePumpSpanOperation.Process, message, MessagingSystem.InternalBus);
 
-        //Assert - the process span was created normally and, once ended, is exported (not leaked)
+        //Assert
         Assert.NotNull(processSpan);
         _tracer.EndSpan(processSpan);
         _traceProvider.ForceFlush();
         Assert.Contains(_exportedActivities, a =>
             a.DisplayName == $"{_routingKey} {MessagePumpSpanOperation.Process.ToSpanName()}");
 
-        //the malformed correlation id is rejected only when baggage is actually propagated - now the responsibility of
-        //PropagateConsumerContext - so that is the single place the validation surfaces
+        //baggage propagation is the single place the malformed value is now rejected
         Assert.Throws<ArgumentException>(() => _tracer.PropagateConsumerContext(message));
     }