feat(sqs): HT-FIFO schema + validators + dormancy gate (Phase 3.D PR 2)

[bootjp]

Summary

Phase 3.D PR 2 (schema + validators + dormancy gate) per the §11 multi-PR rollout in docs/design/2026_04_26_proposed_sqs_split_queue_fifo.md (currently on PR #664).

Stacks on PR #679 (Phase 3.C): branched off feat/sqs-throttling-phase3c so the 3.D schema work shares the same base as the throttling implementation. Will be rebased once PR #679 merges to main.

• sqsQueueMeta gains PartitionCount uint32, FifoThroughputLimit string, DeduplicationScope string.
• partitionFor(meta, messageGroupId) uint32 implements §3.3 — FNV-1a over MessageGroupId, masked by PartitionCount-1 (validator-enforced power-of-two so the bitwise mask is equivalent to mod). Edge cases: PartitionCount 0/1 → 0, FifoThroughputLimit=perQueue short-circuits to 0, empty MessageGroupId → 0.
• validatePartitionConfig enforces the §3.2 cross-attribute rules: power-of-two [1, htfifoMaxPartitions=32], FifoThroughputLimit/DeduplicationScope are FIFO-only, {PartitionCount > 1, DeduplicationScope = "queue"} rejects with InvalidParameterValue at control-plane time so the operator sees the error before the first SendMessage.
• validatePartitionImmutability enforces the §3.2 immutability rule (the three HT-FIFO fields are immutable from CreateQueue onward). SetQueueAttributes is all-or-nothing — a request that touches a mutable attribute alongside an attempted immutable change rejects the whole request before persisting either.
• validatePartitionDormancyGate is the §11 PR 2 temporary gate: CreateQueue with PartitionCount > 1 rejects with InvalidAttributeValue("PartitionCount > 1 requires HT-FIFO data plane — not yet enabled"). The schema field exists in the meta type but no partitioned data can land. Removed in PR 5 in the same commit that wires the data-plane fanout — gate-and-lift is atomic so a half-deployed cluster can never accept a partitioned queue without the data plane to serve it.
• GetQueueAttributes("All") round-trips the configured HT-FIFO fields via addHTFIFOAttributes.

Test plan

• adapter/sqs_partitioning_test.go — 14 unit tests covering partitionFor (legacy zero/one, perQueue short-circuit, empty MessageGroupId, determinism, distribution within ±5% across 8 partitions on 100k samples, power-of-two masking always < N), isPowerOfTwo, validatePartitionConfig (power-of-two, max cap, FIFO-only, cross-attr InvalidParameterValue, single-partition + queue-dedup OK), validatePartitionDormancyGate (rejects > 1, allows 0/1, gate reason surfaces to the operator), validatePartitionImmutability (per-attribute change rejects, same-value no-op succeeds), htfifoAttributesPresent.
• adapter/sqs_partitioning_integration_test.go — 7 end-to-end tests: dormancy gate rejects PartitionCount > 1 (and the gate's "not yet enabled" message surfaces), allows PartitionCount=1, validator rejects non-power-of-two, FIFO-only rejection on Standard queue, cross-attr {PartitionCount=2, DeduplicationScope=queue} rejects, immutability SetQueueAttributes change rejects with same-value no-op succeeding, all-or-nothing combined mutable+immutable rejects without persisting the mutable change, GetQueueAttributes round-trip omits unset fields.
• All tests pass under -race.
• golangci-lint run ./adapter/... → 0 issues.

Self-review (5 lenses)

1. Data loss — Schema fields land on sqsQueueMeta and are persisted via the existing OCC Put on sqsQueueMetaKey. The dormancy gate runs at CreateQueue admission time; once PR 5 lifts the gate, no schema migration is needed (existing queues have PartitionCount=0 which is equivalent to 1). The keyspace stays untouched in this PR (PR 3 threads partitionIndex through sqsMsg*Key).
2. Concurrency / distributed failures — Validation is purely function-of-input; no shared state. The immutability check loads the meta in the same nextTxnReadTS snapshot that applyAttributes uses, so a concurrent SetQueueAttributes from another writer is caught by the existing OCC StartTS + ReadKeys fence (already in place for setQueueAttributesWithRetry). Dormancy gate runs at CreateQueue admission so a concurrent gate-lift mid-request is impossible.
3. Performance — partitionFor is one FNV-1a hash + one bitwise mask; constant time. validatePartitionConfig is field-comparison only, runs once per CreateQueue / SetQueueAttributes. addHTFIFOAttributes adds at most three map writes and only when fields are non-empty. Hot path SendMessage / ReceiveMessage is unaffected by this PR (no routing changes — those land in PR 5).
4. Data consistency — Cross-attribute rule {partitions, queue dedup} rejects at control-plane time so an operator can never end up with a created-but-unserviceable queue (the runtime path that would have surfaced the same error at first send is unreachable). Immutability rule prevents mid-life routing-key reconfiguration that would silently violate within-group FIFO ordering.
5. Test coverage — 14 unit + 7 integration tests cover the validator's full decision tree (each rule has a dedicated table-driven case), the dormancy gate's accept/reject paths with the operator-visible reason, the immutability all-or-nothing semantics, and partitionFor's edge cases plus a 100k-sample distribution test that catches non-uniform routing immediately. Lint is clean.

Out of scope (subsequent §11 rollout PRs)

• PR 3 — keyspace threading: partitionIndex through every sqsMsg*Key constructor, defaulting to 0 so existing queues stay byte-identical. Dormancy gate from this PR remains in place.
• PR 4 — routing layer + --sqsFifoPartitionMap flag + mixed-version gate (§8.5 capability advertisement + §8 leadership-refusal hook in kv/lease_state.go). Dormancy gate still in place.
• PR 5 — send/receive partition fanout + receipt-handle v2 codec. Removes the dormancy gate atomically with the data-plane fanout so the gate-and-lift land in one commit.
• PR 6 — PurgeQueue / DeleteQueue partition iteration + tombstone schema update + reaper.
• PR 7 — Jepsen HT-FIFO workload + metrics.
• PR 8 — partial-doc lifecycle bump.

Branch base

Branched off feat/sqs-throttling-phase3c (PR #679) which itself branches off docs/sqs-phase3-proposals (PR #664). Will rebase against main after both ancestors merge.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3c62738f-39e5-4702-947f-e1bcc8d9797c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/sqs-htfifo-phase3d-pr2

---

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.}

[gemini-code-assist]

Code Review

This pull request introduces HT-FIFO (High Throughput FIFO) support for SQS, adding new attributes such as PartitionCount, FifoThroughputLimit, and DeduplicationScope. It includes comprehensive validation logic for attribute immutability and cross-attribute consistency, alongside a temporary dormancy gate to manage the rollout phases. Feedback focuses on optimizing performance by implementing a manual FNV-1a loop to avoid heap allocations in the routing logic and short-circuiting metadata snapshots when HT-FIFO attributes are absent. Additionally, it is recommended to reuse existing error constants to reduce redundancy.

[gemini-code-assist]

The constant sqsErrInvalidParameterValue is redundant as sqsErrValidation (defined on line 57) already carries the same value "InvalidParameterValue". It is better to reuse the existing constant to maintain consistency and avoid confusion across the codebase.

[gemini-code-assist]

partitionFor is a performance-critical function for the HT-FIFO data plane. The current implementation using fnv.New64a() and []byte conversion causes heap allocations and interface overhead on every call. Since MessageGroupId is capped at 128 characters, a manual FNV-1a loop is significantly more efficient and avoids allocations entirely.

	h := uint64(14695981039346656037)
	for i := 0; i < len(messageGroupID); i++ {
		h ^= uint64(messageGroupID[i])
		h *= 1099511628211
	}
	// PartitionCount is a power of two (validator-enforced); mod is
	// equivalent to mask-AND. The mask is meta.PartitionCount - 1.
	// Computing the mask in uint64 first then narrowing to uint32 is
	// safe because htfifoMaxPartitions == 32 fits in uint32 trivially.
	mask := uint64(meta.PartitionCount - 1)
	return uint32(h & mask) //nolint:gosec // masked by (PartitionCount - 1) ≤ htfifoMaxPartitions − 1, fits in uint32.

[gemini-code-assist]

snapshotImmutableHTFIFO performs a heap allocation on every SetQueueAttributes call. Since most requests only modify mutable attributes (like VisibilityTimeout), we can use the htfifoAttributesPresent helper to skip the snapshot when no HT-FIFO attributes are provided in the request. This avoids redundant work before expensive operations, similar to short-circuiting snapshot checks.

Suggested change

	preApply := snapshotImmutableHTFIFO(meta)
	var preApply *sqsQueueMeta
	if htfifoAttributesPresent(attrs) {
	preApply = snapshotImmutableHTFIFO(meta)
	}

References

1. When checking if a configuration snapshot is up-to-date, short-circuit and return true if the current snapshot's index is strictly greater than the target index. Only compare configuration states if the indices are equal. Perform this check before any expensive FSM snapshot serialization to avoid redundant work.

[gemini-code-assist]

Use the existing sqsErrValidation constant instead of the redundant sqsErrInvalidParameterValue.

Suggested change

	return newSQSAPIError(http.StatusBadRequest, sqsErrInvalidParameterValue,
	return newSQSAPIError(http.StatusBadRequest, sqsErrValidation,

[gemini-code-assist]

Use the existing sqsErrValidation constant instead of the redundant sqsErrInvalidParameterValue.

Suggested change

	require.Equal(t, sqsErrInvalidParameterValue, apiErr.errorType,
	require.Equal(t, sqsErrValidation, apiErr.errorType,

[bootjp]

@claude review

[claude]

Claude finished @bootjp's task in 3m 37s —— View job

---

Code Review — Phase 3.D PR 2: HT-FIFO schema + validators + dormancy gate

• Gather context and read changed files
• Review adapter/sqs_partitioning.go
• Review adapter/sqs_catalog.go changes
• Review adapter/sqs_partitioning_test.go
• Review adapter/sqs_partitioning_integration_test.go
• Compile findings and post review

---

Overall this is a well-structured, well-documented rollout gate. The design-doc-first workflow is followed, the dormancy-gate pattern is sound (gate-and-lift in one atomic commit), and the self-review covers the five CLAUDE.md lenses. The test coverage is thorough for the validator surface. A few issues worth addressing before merge:

---

Bug: PartitionCount > 1 not restricted to FIFO queues — latent defect activated by PR 5

adapter/sqs_partitioning.go:121–147

validatePartitionConfig correctly rejects FifoThroughputLimit and DeduplicationScope on Standard queues, but has no analogous guard for PartitionCount > 1. After the dormancy gate is lifted in PR 5, a Standard queue sent with PartitionCount=2 would pass validatePartitionConfig and land. The data plane would then attempt FIFO partition routing on a non-FIFO queue, which the SQS spec doesn't define.

The fix belongs in the PartitionCount > 0 block:

if meta.PartitionCount > 1 && !meta.IsFIFO {
    return newSQSAPIError(http.StatusBadRequest, sqsErrInvalidAttributeValue,
        "PartitionCount > 1 is only valid on FIFO queues")
}

A corresponding unit test case (PartitionCount=2, IsFIFO=false → error) should accompany this. The dormancy gate masks the bug today but PR 5's gate-lift makes it live. Fix this →

---

Duplicate constant: sqsErrInvalidParameterValue shadows sqsErrValidation

adapter/sqs_catalog.go:57,68

sqsErrValidation            = "InvalidParameterValue"  // line 57 — existing
sqsErrInvalidParameterValue = "InvalidParameterValue"  // line 68 — new, same value

Both constants carry the identical string value. The comment on the new constant describes a semantic distinction (cross-attribute vs per-value errors), but AWS uses the same wire code (InvalidParameterValue) for both, so the distinction isn't real on the wire. The test in sqs_partitioning_test.go:190 asserts sqsErrInvalidParameterValue, which is worth keeping as documentation of intent — but the constant itself should be eliminated and sqsErrValidation used throughout, or the constant should be an alias:

// sqsErrInvalidParameterValue is an alias for sqsErrValidation; AWS uses the
// same code for both incoherent parameter combinations and general validation errors.
sqsErrInvalidParameterValue = sqsErrValidation

Or simply replace the three uses (sqs_partitioning.go:143, sqs_partitioning_test.go:190) with sqsErrValidation and drop the new constant. The Gemini reviewer flagged this too. Fix this →

---

Performance: partitionFor allocates on every call via fnv.New64a()

adapter/sqs_partitioning.go:83–90

h := fnv.New64a()
_, _ = h.Write([]byte(messageGroupID))

fnv.New64a() returns an interface (heap allocation), and []byte(messageGroupID) allocates on every invocation. Since partitionFor will be on the hot path for every SendMessage once PR 5 ships, an inline FNV-1a loop avoids both:

h := uint64(14695981039346656037)
for i := 0; i < len(messageGroupID); i++ {
    h ^= uint64(messageGroupID[i])
    h *= 1099511628211
}
mask := uint64(meta.PartitionCount - 1)
return uint32(h & mask) //nolint:gosec

This also makes the "hash/fnv" import unnecessary, removing a stdlib dependency from the hot-path file. Fix this →

---

Optimization: snapshotImmutableHTFIFO allocates on every SetQueueAttributes call

adapter/sqs_catalog.go:1370 (approx.)

The snapshot is taken unconditionally before applyAttributes, even when the request contains only mutable attributes (VisibilityTimeout, MessageRetentionPeriod, etc.). htfifoAttributesPresent already exists precisely for this check — use it to skip the snapshot when the request doesn't touch HT-FIFO fields:

var preApply *sqsQueueMeta
if htfifoAttributesPresent(attrs) {
    preApply = snapshotImmutableHTFIFO(meta)
}

validatePartitionImmutability already handles current == nil with a no-op return, so this is safe. Fix this →

---

Test: Integration test name misleads about what is actually tested

adapter/sqs_partitioning_integration_test.go:118

TestSQSServer_HTFIFO_RejectsQueueScopedDedupOnPartitioned sends PartitionCount=2 with DeduplicationScope=queue, but the dormancy gate fires first (as the comment acknowledges). The test only asserts HTTP 400 without checking __type or the message, so it cannot distinguish whether the dormancy gate or the cross-attr rule fired. The test name and the doc comment both claim to pin the cross-attribute rule, but the test doesn't actually reach that code path during PR 2–4.

The test should either:

1. Rename to TestSQSServer_HTFIFO_DormancyGateBlocksCrossAttrCombination and tighten its assertion to the dormancy gate message, or
2. Keep the name and add a comment clarifying it will need a second pass in PR 5 to verify the actual cross-attr rule fires post-gate-lift.

The unit test in sqs_partitioning_test.go:180 does correctly cover the cross-attr rule — the integration layer coverage gap is the issue.

---

Minor: Custom contains/indexOf instead of strings.Contains

adapter/sqs_partitioning_integration_test.go:276–287

The comment says "without pulling in strings just for one call" but each _test.go file manages its own imports; adding import "strings" here does not affect other files. Using strings.Contains is more idiomatic Go and simpler:

// no need for the indexOf helper at all
if !strings.Contains(msg, "not yet enabled") { ... }

This is minor, but the custom implementation is 11 lines for something the stdlib does in one. Fix this →

---

Summary

#	Severity	File	Issue
1	Bug (latent, live in PR 5)	sqs_partitioning.go:121	PartitionCount > 1 not FIFO-only — Standard queue would be silently accepted after gate lift
2	Medium	sqs_catalog.go:68	Duplicate constant sqsErrInvalidParameterValue = sqsErrValidation
3	Medium	sqs_partitioning.go:83	partitionFor heap-allocates on every call via fnv.New64a()
4	Medium	sqs_catalog.go:~1370	snapshotImmutableHTFIFO allocates unconditionally on every SetQueueAttributes
5	Low	sqs_partitioning_integration_test.go:118	Test name/comment claims cross-attr coverage but dormancy gate fires first
6	Nit	sqs_partitioning_integration_test.go:276	Custom contains/indexOf instead of strings.Contains

Issue #1 is the most important to fix before the gate lifts in PR 5. Issues #2–4 match the Gemini reviewer's findings and are worth fixing before merge to keep the codebase clean. Issues #5–6 are polish.

[bootjp]

3件の Gemini medium + cyclop fix を入れて、PR #679 の最新 throttling branch にも rebase しました(fbd68a6e)。

Gemini medium #1 — sqsErrInvalidParameterValue 削除
既存の sqsErrValidation = "InvalidParameterValue" と重複していたので削除し、sqs_partitioning.go + テストでも sqsErrValidation を直接使うように。validatePartitionConfig の rejection envelope の意図はインラインコメントで残しています。

Gemini medium #2 — snapshotImmutableHTFIFO heap alloc を skip
htfifoAttributesPresent(attrs) で gate して、HT-FIFO attribute が request にない時は snapshot を作らないように変更。VisibilityTimeout だけ更新するような common path では preApply == nil → immutability check も skip → snapshot allocation 0 になります。Correctness は同等(HT-FIFO attr がない request は HT-FIFO field を変更しようがないので check は always pass)。

Gemini medium #3 — partitionFor 手書き FNV-1a
hash/fnv.New64a + h.Write([]byte(...)) は []byte allocation + hash.Hash interface dispatch のコストを毎回払うので、string 上を直接ループする FNV-1a に置き換え。MessageGroupId は 128 chars cap なので bounded loop。Hot-path のルーティングコストが下がります。

Cyclop fix
上記 #2 の conditional preApply 分岐 + Throttle validator (PR #679 から rebase で入った) で trySetQueueAttributesOnce が 12 になったので、applyAndValidateSetAttributes ヘルパーに抽出。ceiling 内に戻りました。

Rebase note: PR #679 が新しい commit e3530e93 (review feedback fixes) を持つので、その上に rebase してから PR #681 の修正を積みました。force-push 済み。

Verified: go test -race ./adapter/... (3.C + 3.D テスト)clean, golangci-lint run clean.

@claude review

[claude]

Claude finished @bootjp's task in 4m 13s —— View job

---

Code Review — Phase 3.D PR 2: post-feedback round

• Gather context and read changed files
• Review adapter/sqs_partitioning.go (inline FNV-1a, FIFO-only guard)
• Review adapter/sqs_catalog.go changes (dedup const, conditional snapshot, cyclop refactor)
• Review adapter/sqs_partitioning_test.go
• Review adapter/sqs_partitioning_integration_test.go
• Compile findings and post review

---

All four Gemini medium / cyclop fixes are confirmed landed and correct. Three issues remain.

---

Bug (latent, unresolved from round 1): PartitionCount > 1 still accepted on Standard queues

adapter/sqs_partitioning.go:144–152

The !meta.IsFIFO block still only guards FifoThroughputLimit and DeduplicationScope, not PartitionCount > 1. The dormancy gate masks this today, but a Standard queue with PartitionCount=2 would pass validatePartitionConfig once PR 5 lifts the gate, because:

• PartitionCount=2 > 0 → power-of-two OK, ≤ 32 OK
• !meta.IsFIFO → FifoThroughputLimit=="" and DeduplicationScope=="" → no rejection
• PartitionCount=2 > 1 && DeduplicationScope=="queue" → DeduplicationScope is "" not "queue" → no rejection

The fix (same as round 1):

if !meta.IsFIFO {
    if meta.PartitionCount > 1 {
        return newSQSAPIError(http.StatusBadRequest, sqsErrInvalidAttributeValue,
            "PartitionCount > 1 is only valid on FIFO queues")
    }
    if meta.FifoThroughputLimit != "" { ... }
    if meta.DeduplicationScope != "" { ... }
}

Plus a unit test case (PartitionCount=2, IsFIFO=false → error). Fix this →

---

Comment wrong: TestSQSServer_HTFIFO_RejectsQueueScopedDedupOnPartitioned has two contradictory/incorrect claims

adapter/sqs_partitioning_integration_test.go:110–143

1) The function-level doc (line 114) says: "Test sets PartitionCount=1 to bypass dormancy" — but the test actually sends PartitionCount=2 (line 134).

2) The inline comment (lines 141–143) says: "dormancy gate fires first (PartitionCount > 1)" — but looking at createQueue in sqs_catalog.go, parseAttributesIntoMeta (line 832) runs validatePartitionConfig internally before validatePartitionDormancyGate is called (line 843). For {PartitionCount=2, DeduplicationScope=queue}, the cross-attr rule in validatePartitionConfig fires first; the dormancy gate is never reached.

The test itself is correct — it gets a 400 from the cross-attr rule, not the dormancy gate. But a reader following the comments will have a false model of which validator fires. Suggested fix: correct line 114 to say "test sends PartitionCount=2 to trigger the cross-attr rule; the schema validator fires before the dormancy gate so the cross-attr rejection is what the wire layer sees" and remove the misleading "dormancy fires first" assertion at lines 141–143. Fix this →

---

Nit (unresolved from round 1): custom contains/indexOf still present

adapter/sqs_partitioning_integration_test.go:273–287

The custom 11-line contains/indexOf pair was flagged in round 1. import "strings" + strings.Contains is one line and idiomatic. Not blocking.

---

Round-1 fixes confirmed

Fix	Status
Remove duplicate sqsErrInvalidParameterValue constant	✓ done
Conditional snapshotImmutableHTFIFO (gate on htfifoAttributesPresent)	✓ done
Inline FNV-1a in partitionFor (no heap alloc)	✓ done
Extract applyAndValidateSetAttributes to fix cyclop	✓ done

---

Summary

#	Severity	File	Issue
1	Bug (latent, live in PR 5)	sqs_partitioning.go:144	PartitionCount > 1 not FIFO-only — Standard queue accepted after gate lift
2	Low	sqs_partitioning_integration_test.go:110	Doc says "PartitionCount=1" but test sends 2; inline comment claims dormancy fires first but cross-attr rule fires first
3	Nit	sqs_partitioning_integration_test.go:273	Custom contains/indexOf instead of strings.Contains

Issue #1 is the only blocker before gate lift in PR 5. Issues #2–3 are polish.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fbd68a6e94

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Reject PartitionCount on non-FIFO queues

validatePartitionConfig enforces FIFO-only semantics for FifoThroughputLimit and DeduplicationScope but not PartitionCount, so a Standard queue can be created with PartitionCount=1 and persist HT-FIFO-only metadata. This creates an invalid configuration surface (and for >1, currently returns the dormancy-gate message instead of a FIFO-only validation error), which is likely to confuse operators and makes control-plane behavior inconsistent with the HT-FIFO model.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Disallow empty HT-FIFO attribute values

The applier currently treats an explicit empty string as valid for FifoThroughputLimit (and similarly for DeduplicationScope), so a request that sends the key with "" succeeds and silently acts like unset rather than failing fast. This can hide client/configuration mistakes and produces surprising control-plane outcomes (especially when users think they set a concrete throughput/dedup mode).

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[bootjp]

Round 2 review の 3 件対応(310c1caf):

Bug (latent, live in PR 5) — PartitionCount > 1 not FIFO-only
ご指摘の通り、validatePartitionConfig の !meta.IsFIFO block が FifoThroughputLimit / DeduplicationScope だけ guard していて、PartitionCount > 1 を見逃していました。Standard queue + PartitionCount=2 が PR 5 で dormancy gate が外れた後に validator を素通りしてしまう状態でした。Block に PartitionCount > 1 の guard を追加し、InvalidAttributeValue("PartitionCount > 1 is only valid on FIFO queues") を返すように。PartitionCount 0/1 は legacy single-partition layout なので Standard でも accept のまま。TestValidatePartitionConfig_StandardQueueRejectsHTFIFOAttrs を拡張して PartitionCount=2/4/8/16/32 → reject と 0/1 → accept をテーブルテスト化。

Low — TestSQSServer_HTFIFO_RejectsQueueScopedDedupOnPartitioned のコメント矛盾
docstring に "Test sets PartitionCount=1 to bypass dormancy" とあったのに実際には PartitionCount=2 を送っており、さらに inline コメントで "dormancy gate fires first" と書いていたが実際には validatePartitionConfig が parseAttributesIntoMeta 内で先に走るので cross-attr rule が wire layer の rejection になっていました。docstring を実際の control-plane order を反映する内容に書き換え、misleading な inline コメントを削除。テスト本体は 400 だけ assert する形にして、validator の発火順を変えるリファクタにも壊れないようにしています。

Nit — custom contains/indexOf 削除
strings.Contains に置き換え、helper 11 行を削除。

Verified: go test -race ./adapter/... (3.C + 3.D + idempotency tests) clean, golangci-lint run clean.

@claude review

[claude]

Claude finished @bootjp's task in 2m 22s —— View job

---

Code Review — Phase 3.D PR 2: round 3 (post-feedback)

• Read changed files
• Verify Bug fix: PartitionCount > 1 FIFO-only guard
• Verify Low fix: misleading test comments corrected
• Verify Nit fix: strings.Contains replacement
• Check for remaining issues
• Post final review

---

All three round-3 items are correctly implemented. The PR is clean.

Round-3 fixes confirmed

Bug (latent) — PartitionCount > 1 FIFO-only guard ✓
sqs_partitioning.go:152–155: the guard is correctly placed inside the !meta.IsFIFO block, after the power-of-two and cap checks, so PartitionCount=2 on a Standard queue hits PartitionCount > 1 is only valid on FIFO queues. PartitionCount=0/1 are correctly still accepted on Standard queues (both mean "single-partition layout"). The unit test now covers {2, 4, 8, 16, 32} → error and {0, 1} → accept.

Low — test comment contradictions fixed ✓
sqs_partitioning_integration_test.go:111–124: the docstring no longer claims PartitionCount=1; the new doc correctly describes that validatePartitionConfig runs inside parseAttributesIntoMeta before validatePartitionDormancyGate fires in createQueue, so the cross-attr rule is what the wire layer sees today. The test body asserts only the 400 status, remaining agnostic about which validator fires — correct choice; this won't break if createQueue's validator ordering is ever refactored.

Nit — strings.Contains ✓
Custom contains/indexOf helpers are gone. import "strings" is in place; both wire-layer message checks ("not yet enabled", "power of two") now use strings.Contains inline.

---

One open Codex concern (not addressed in round 3 — low priority)

The Codex P2 flag from round 1 about empty-string HT-FIFO attributes is still open. FifoThroughputLimit="" and DeduplicationScope="" are accepted by their appliers (sqs_catalog.go:481, sqs_catalog.go:495) and silently treated as "unset" rather than rejected with InvalidAttributeValue. The immutability check does catch an attempt to change a previously-set field back to "" via SetQueueAttributes (the snapshot comparison fires), so there's no correctness hole. The issue is purely UX at CreateQueue time — an operator passing an explicit empty string gets a no-op silently rather than an error.

This is a design choice (matches how some AWS SDKs "clear" attributes), not a blocker. Worth a note in the PR description for PR 5 so the gate-lift commit can decide whether to add a non-empty guard then.

---

Summary

All round-1, round-2, and round-3 review items are resolved. The latent bug (Standard queue + PartitionCount > 1 after gate lift) is now covered. The PR is ready to merge once PR #679 lands and the rebase completes.