Skip to content

Commit 213ded4

Browse files
committed
style(sqs): strip PR/round attributions from .go comments per CLAUDE.md (PR #664 round 14)
Claude low on round 13.1: ~25+ comments across the SQS Go files reference review attributions ('Codex P1 on PR #679', 'CodeRabbit Major on PR #664 round 11', etc.). CLAUDE.md is explicit: 'Don't reference the current task, fix, or callers ... since those belong in the PR description and rot as the codebase evolves.' Sweep across: - adapter/sqs_throttle.go - adapter/sqs_catalog.go - adapter/sqs_partitioning.go - adapter/sqs_messages.go - adapter/sqs_throttle_test.go - adapter/sqs_partitioning_test.go - adapter/sqs_throttle_integration_test.go - adapter/sqs_partitioning_integration_test.go - adapter/sqs_catalog_test.go - adapter/sqs_query_protocol.go - adapter/sqs_query_protocol_test.go Each comment now retains the substantive WHY (why the design choice was made) and drops the parenthetical attribution to a specific reviewer / PR number / round. Design docs (.md files) are exempt — those are archival records where review attribution is acceptable practice. go test -race ./adapter/... pass; golangci-lint ./... clean. No behaviour change.
1 parent 8d6e214 commit 213ded4

11 files changed

Lines changed: 114 additions & 125 deletions

adapter/sqs_catalog.go

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ type sqsQueueMeta struct {
8686
// This is the throttle-bucket-key field — keying by Incarnation
8787
// instead of Generation prevents PurgeQueue from resetting the
8888
// in-memory token bucket while still isolating delete+recreate
89-
// incarnations from each other (Codex P2 on PR #664 round 9).
89+
// incarnations from each other.
9090
Incarnation uint64 `json:"incarnation,omitempty"`
9191
CreatedAtHLC uint64 `json:"created_at_hlc,omitempty"`
9292
IsFIFO bool `json:"is_fifo,omitempty"`
@@ -674,9 +674,7 @@ func validateThrottleConfig(meta *sqsQueueMeta) error {
674674
// `ThrottleDefaultCapacity=5, ThrottleDefaultRefillPerSecond=1`
675675
// would be accepted but make every full SendMessageBatch /
676676
// DeleteMessageBatch (charge=10) permanently unserviceable —
677-
// the bucket can never accumulate the 10 tokens. Codex P1 on
678-
// PR #679 round 5 caught the gap; the design doc note in §3.2
679-
// claiming Default* is exempt was wrong about the fall-through.
677+
// the bucket can never accumulate the 10 tokens.
680678
if err := validateThrottlePair("ThrottleDefault", t.DefaultCapacity, t.DefaultRefillPerSecond, true); err != nil {
681679
return err
682680
}
@@ -805,9 +803,8 @@ func throttleConfigEqual(a, b *sqsQueueThrottle) bool {
805803
// without PartitionCount is stored as 0 while a queue created with
806804
// explicit PartitionCount=1 is stored as 1, so strict equality would
807805
// have CreateQueue reject the second call as "different attributes"
808-
// even though the queues are semantically identical (Codex P2 on
809-
// PR #679 round 6.1). normalisePartitionCount maps both to 1 for the
810-
// idempotency check.
806+
// even though the queues are semantically identical.
807+
// normalisePartitionCount maps both to 1 for the idempotency check.
811808
func htfifoAttributesEqual(a, b *sqsQueueMeta) bool {
812809
return normalisePartitionCount(a.PartitionCount) == normalisePartitionCount(b.PartitionCount) &&
813810
a.FifoThroughputLimit == b.FifoThroughputLimit &&
@@ -1006,8 +1003,8 @@ func (s *SQSServer) tryCreateQueueOnce(ctx context.Context, requested *sqsQueueM
10061003
if _, err := s.coordinator.Dispatch(ctx, req); err != nil {
10071004
return false, errors.WithStack(err)
10081005
}
1009-
// Drop any throttle bucket that survived a delete-then-create race
1010-
// (Codex P2 on PR #679 round 5). DeleteQueue invalidates after its
1006+
// Drop any throttle bucket that survived a delete-then-create
1007+
// race. DeleteQueue invalidates after its
10111008
// commit, but a sendMessage holding pre-delete meta can recreate
10121009
// a bucket between that invalidate and this CreateQueue commit;
10131010
// invalidating again here on a genuine create (not the idempotent
@@ -1447,12 +1444,11 @@ func (s *SQSServer) setQueueAttributes(w http.ResponseWriter, r *http.Request) {
14471444
// reset the bucket on every unrelated SetQueueAttributes (e.g.
14481445
// VisibilityTimeout-only update), giving any caller a way to
14491446
// silently restore a noisy tenant's burst capacity by writing a
1450-
// no-op SetQueueAttributes (Codex P1 on PR #679, refined on PR
1451-
// #664 round 9). Gating on key-presence alone is not enough either
1452-
// — a same-value Throttle* write would still pass the presence
1453-
// check and invalidate, so a caller could repeat their own current
1454-
// throttle config to bump the bucket back to full capacity (Codex
1455-
// P1 on PR #664 round 9). trySetQueueAttributesOnce therefore
1447+
// no-op SetQueueAttributes. Gating on key-presence alone is not
1448+
// enough either — a same-value Throttle* write would still pass
1449+
// the presence check and invalidate, so a caller could repeat
1450+
// their own current throttle config to bump the bucket back to
1451+
// full capacity. trySetQueueAttributesOnce therefore
14561452
// compares the old and new throttle configs under the same Raft
14571453
// read snapshot used for the commit and reports whether the values
14581454
// actually moved. The bucket reconciliation in loadOrInit also
@@ -1492,8 +1488,7 @@ func (s *SQSServer) setQueueAttributesWithRetry(ctx context.Context, queueName s
14921488
// sqsAPIError the caller forwards to writeSQSErrorFromErr.
14931489
//
14941490
// preApply snapshot allocation is gated on htfifoAttributesPresent
1495-
// so the common "mutable-only update" path stays alloc-free per the
1496-
// Gemini medium feedback on PR #681.
1491+
// so the common "mutable-only update" path stays alloc-free.
14971492
func applyAndValidateSetAttributes(meta *sqsQueueMeta, attrs map[string]string) error {
14981493
var preApply *sqsQueueMeta
14991494
if htfifoAttributesPresent(attrs) {
@@ -1528,7 +1523,7 @@ func applyAndValidateSetAttributes(meta *sqsQueueMeta, attrs map[string]string)
15281523
// the post-apply meta's Throttle config differs from the pre-apply
15291524
// snapshot — the caller uses it to gate the cache invalidation so
15301525
// that a no-op same-value SetQueueAttributes does not reset the
1531-
// bucket to full capacity (Codex P1 on PR #664 round 9).
1526+
// bucket to full capacity.
15321527
func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName string, attrs map[string]string) (bool, bool, error) {
15331528
readTS := s.nextTxnReadTS(ctx)
15341529
meta, exists, err := s.loadQueueMetaAt(ctx, queueName, readTS)

adapter/sqs_catalog_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ func TestSQSServer_CatalogCreateIsIdempotent(t *testing.T) {
143143
// notice the diff and the call must reject as QueueNameExists.
144144
// Without this case a bug in throttleConfigEqual (e.g. always
145145
// returning true) would slip past the existing VisibilityTimeout-
146-
// only test (Claude review on PR #679 round 2).
146+
// only test.
147147
withThrottle := map[string]any{
148148
"QueueName": "idempotent",
149149
"Attributes": map[string]string{

adapter/sqs_messages.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ type sqsChangeVisibilityInput struct {
297297
// prepareSendMessage decodes the SendMessage payload and resolves
298298
// the queue name. Throttle charging happens after the meta load in
299299
// validateSend so we don't pay an extra meta read just to discover
300-
// throttling is off (Gemini high on PR #679).
300+
// throttling is off.
301301
func (s *SQSServer) prepareSendMessage(w http.ResponseWriter, r *http.Request) (sqsSendMessageInput, string, bool) {
302302
var in sqsSendMessageInput
303303
if err := decodeSQSJSONInput(r, &in); err != nil {
@@ -565,9 +565,9 @@ func (s *SQSServer) receiveMessage(w http.ResponseWriter, r *http.Request) {
565565
return
566566
}
567567
// Throttle check uses the loaded meta's throttle config so we
568-
// don't pay an extra meta read just to discover throttling is off
569-
// (Gemini high on PR #679). Sits AFTER the QueueDoesNotExist
570-
// branch — a missing queue should not consume a Recv token.
568+
// don't pay an extra meta read just to discover throttling is
569+
// off. Sits AFTER the QueueDoesNotExist branch — a missing queue
570+
// should not consume a Recv token.
571571
if !s.chargeQueueWithThrottle(w, queueName, bucketActionReceive, 1, meta.Throttle, meta.Incarnation) {
572572
return
573573
}

adapter/sqs_partitioning.go

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -80,16 +80,16 @@ func partitionFor(meta *sqsQueueMeta, messageGroupID string) uint32 {
8080
return 0
8181
}
8282
// Inlined FNV-1a 32-bit over the string to avoid the []byte
83-
// allocation hash/fnv.New32a + h.Write would force (Gemini medium
84-
// on PR #681). MessageGroupId is capped at 128 chars by validation,
83+
// allocation hash/fnv.New32a + h.Write would force.
84+
// MessageGroupId is capped at 128 chars by validation,
8585
// so this loop bounds at 128 iterations of integer arithmetic per
8686
// SendMessage — measurably faster than the hash.Hash interface
8787
// path on the routing hot path. The 32-bit variant keeps the
8888
// computation in uint32 throughout, sidestepping the uint64 →
89-
// uint32 narrowing that the 64-bit variant would have required for
90-
// the partition mask AND (CodeRabbit nit on PR #664 round 7 — the
91-
// previous implementation needed `//nolint:gosec G115` to silence
92-
// a false positive on the safe-by-construction narrow). PartitionCount
89+
// uint32 narrowing that the 64-bit variant would have required
90+
// for the partition mask AND (which gosec G115 would otherwise
91+
// flag and force a //nolint suppression on a safe-by-construction
92+
// narrow). PartitionCount
9393
// ≤ htfifoMaxPartitions = 32 so log2(PartitionCount) ≤ 5; only the
9494
// low 5 bits of the hash ever survive the mask, and 32-bit FNV-1a
9595
// is more than enough entropy to spread MessageGroupId values
@@ -146,15 +146,14 @@ func validatePartitionConfig(meta *sqsQueueMeta) error {
146146
}
147147
}
148148
if meta.PartitionCount > 1 && meta.DeduplicationScope == htfifoDedupeScopeQueue {
149-
// sqsErrValidation is "InvalidParameterValue" (Gemini medium
150-
// on PR #681 — uses the existing constant rather than a
151-
// duplicate-value alias).
149+
// sqsErrValidation is "InvalidParameterValue"; uses the
150+
// existing constant rather than a duplicate-value alias.
152151
return newSQSAPIError(http.StatusBadRequest, sqsErrValidation,
153152
"queue-scoped deduplication is incompatible with multi-partition FIFO because the dedup key cannot be globally unique across partitions without a cross-partition OCC transaction")
154153
}
155154
// FifoThroughputLimit=perMessageGroupId requires an explicit
156-
// PartitionCount > 1 (CodeRabbit Major on PR #664 round 11). §7.2
157-
// of the design used to suggest "infer a sensible default, e.g.
155+
// PartitionCount > 1. §7.2 of the design used to suggest
156+
// "infer a sensible default, e.g.
158157
// 8" for HT-FIFO callers that omit PartitionCount, but a hidden
159158
// default makes CreateQueue idempotency depend on deployment
160159
// state — the same wire payload could resolve to a 4-partition
@@ -173,8 +172,8 @@ func validatePartitionConfig(meta *sqsQueueMeta) error {
173172
// validatePartitionShape enforces the structural rules on
174173
// PartitionCount: power-of-two and within the per-queue cap. Split
175174
// out of validatePartitionConfig to keep that function under the
176-
// cyclop ceiling once the round-12 perMessageGroupId-requires-explicit
177-
// rule landed.
175+
// cyclop ceiling once the perMessageGroupId-requires-explicit rule
176+
// landed.
178177
func validatePartitionShape(meta *sqsQueueMeta) error {
179178
if meta.PartitionCount == 0 {
180179
return nil
@@ -194,8 +193,8 @@ func validatePartitionShape(meta *sqsQueueMeta) error {
194193
// the HT-FIFO attributes. PartitionCount > 1 only makes sense on FIFO
195194
// queues (HT-FIFO is by definition a FIFO feature). Without this guard
196195
// a Standard queue with PartitionCount=2 would slip past the validator
197-
// once PR 5 lifts the dormancy gate (Claude review on PR #681 round 2
198-
// caught this). PartitionCount=0 and 1 are accepted because both mean
196+
// once PR 5 lifts the dormancy gate. PartitionCount=0 and 1 are
197+
// accepted because both mean
199198
// "single-partition layout" which is valid on Standard queues.
200199
func validateStandardQueueRejectsHTFIFO(meta *sqsQueueMeta) error {
201200
if meta.PartitionCount > 1 {
@@ -248,7 +247,7 @@ func validatePartitionDormancyGate(meta *sqsQueueMeta) error {
248247
// queue stored with 0, or 0 on a queue stored with 1) is treated as
249248
// the no-op it semantically is — strict equality would reject with
250249
// "PartitionCount is immutable" even though the partition layout
251-
// hasn't changed (Claude Low on PR #679 round 6.2).
250+
// hasn't changed.
252251
func validatePartitionImmutability(current, requested *sqsQueueMeta) error {
253252
if current == nil || requested == nil {
254253
return nil

adapter/sqs_partitioning_integration_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -155,10 +155,10 @@ func TestSQSServer_HTFIFO_ImmutabilitySetQueueAttributesRejects(t *testing.T) {
155155

156156
url := mustCreateFIFOWithThroughputLimit(t, node, "htfifo-immutable.fifo", htfifoThroughputPerQueue)
157157

158-
// Try to flip FifoThroughputLimit. Must reject (immutability +
159-
// the round-12 rule that perMessageGroupId requires
160-
// PartitionCount > 1 both fire on this attempt; either one
161-
// rejecting is the correct outcome from the wire).
158+
// Try to flip FifoThroughputLimit. Must reject — both the
159+
// immutability rule and the rule that perMessageGroupId requires
160+
// PartitionCount > 1 fire on this attempt; either one rejecting
161+
// is the correct outcome from the wire.
162162
status, out := callSQS(t, node, sqsSetQueueAttributesTarget, map[string]any{
163163
"QueueUrl": url,
164164
"Attributes": map[string]string{

adapter/sqs_partitioning_test.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -164,10 +164,10 @@ func TestValidatePartitionConfig_RejectsAboveMax(t *testing.T) {
164164
// reject with InvalidAttributeValue. Setting them silently on a
165165
// Standard queue would advertise unsupported behaviour.
166166
//
167-
// PartitionCount > 1 is also FIFO-only (Claude review on PR #681
168-
// round 2 caught the gap) — without the guard a Standard queue
169-
// with PartitionCount=2 would slip past the validator after PR 5
170-
// lifts the dormancy gate. PartitionCount 0/1 are still accepted
167+
// PartitionCount > 1 is also FIFO-only — without the guard a
168+
// Standard queue with PartitionCount=2 would slip past the validator
169+
// after PR 5 lifts the dormancy gate. PartitionCount 0/1 are still
170+
// accepted
171171
// on Standard queues because both mean "single-partition layout".
172172
func TestValidatePartitionConfig_StandardQueueRejectsHTFIFOAttrs(t *testing.T) {
173173
t.Parallel()
@@ -210,7 +210,7 @@ func TestValidatePartitionConfig_RejectsQueueScopedDedupOnPartitioned(t *testing
210210
}
211211

212212
// TestValidatePartitionConfig_PerMessageGroupIDRequiresExplicitPartitionCount
213-
// pins the CodeRabbit Major fix on PR #664 round 11: an HT-FIFO
213+
// pins the no-implicit-default rule: an HT-FIFO
214214
// CreateQueue request that sets FifoThroughputLimit=perMessageGroupId
215215
// without an explicit PartitionCount > 1 must be rejected. Earlier
216216
// drafts of §7.2 proposed inferring a server-side default (e.g. 8),
@@ -310,7 +310,7 @@ func TestValidatePartitionImmutability_RejectsAnyChange(t *testing.T) {
310310
}
311311

312312
// TestValidatePartitionImmutability_PartitionCountZeroAndOneEquivalent
313-
// pins the Claude Low fix on PR #679 round 6.2 / 6.3. The on-disk
313+
// pins the 0/1 normalisation: the on-disk
314314
// PartitionCount=0 ("unset") is canonical-equivalent to an explicit
315315
// PartitionCount=1 ("single partition"), so a SetQueueAttributes
316316
// that reaffirms the default ought to be a no-op rather than a hard
@@ -362,8 +362,8 @@ func TestHTFIFOAttributesPresent(t *testing.T) {
362362
}
363363

364364
// TestHTFIFOAttributesEqual_PartitionCountZeroAndOneEquivalent pins
365-
// the Codex P2 fix on PR #679 round 6.1: validatePartitionConfig
366-
// documents PartitionCount=0 (unset) and =1 (explicit single
365+
// the idempotency normalisation: validatePartitionConfig documents
366+
// PartitionCount=0 (unset) and =1 (explicit single
367367
// partition) as semantically identical legacy/single-partition
368368
// routing, so CreateQueue idempotency must treat them as equal —
369369
// otherwise a queue created without PartitionCount (stored as 0) is

adapter/sqs_query_protocol.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -147,9 +147,9 @@ func readQueryForm(r *http.Request) (url.Values, error) {
147147
return nil, newSQSAPIError(http.StatusBadRequest, sqsErrMalformedRequest, "missing request URL")
148148
}
149149
// r.URL.Query() returns a fresh map on each call so we can adopt
150-
// it directly as the base instead of copying entries one-by-one
151-
// (Gemini high-priority on PR #662). On GET we are done; on POST
152-
// the body's form values are merged into the same map.
150+
// it directly as the base instead of copying entries one-by-one.
151+
// On GET we are done; on POST the body's form values are merged
152+
// into the same map.
153153
values := r.URL.Query()
154154
if r.Method == http.MethodGet {
155155
return values, nil
@@ -508,8 +508,8 @@ func errorTypeForStatus(status int) string {
508508
// like the AWS RequestId: 22 chars of base32 (no padding). Base32
509509
// emits 1 char per 5 input bits, so 22 chars require ceil(22*5/8)=14
510510
// random bytes (110 bits, comfortably more entropy than a UUID-v4).
511-
// Gemini medium on PR #662 flagged the prior 16-byte source — that
512-
// produces a 26-char ID, not 22, and the comment was a lie.
511+
// A 16-byte source would produce a 26-char ID, not 22, so the byte
512+
// count is pinned at 14 to match the documented length.
513513
func newQueryRequestID() string {
514514
var raw [14]byte
515515
if _, err := rand.Read(raw[:]); err != nil {

adapter/sqs_query_protocol_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -148,8 +148,8 @@ func TestCollectIndexedKVPairs_Empty(t *testing.T) {
148148
}
149149

150150
// TestNewQueryRequestID_Length pins the AWS shape: 22 base32 chars.
151-
// Gemini medium on PR #662 caught the prior 26-char output that
152-
// contradicted the function's own doc comment.
151+
// A regression where this function returned 26 chars contradicted
152+
// its own doc comment, so the length is asserted explicitly.
153153
func TestNewQueryRequestID_Length(t *testing.T) {
154154
t.Parallel()
155155
for i := 0; i < 64; i++ {

0 commit comments

Comments
 (0)