feat(sqs/admin): SigV4-bypass admin entrypoints + SPA queues pages#670
feat(sqs/admin): SigV4-bypass admin entrypoints + SPA queues pages#670
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 25 minutes and 6 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces SQS admin capabilities to the dashboard, allowing users to list, describe, and delete queues. It includes a new SQS handler in the admin package, an adapter bridge, and a React-based frontend for queue management. Feedback focuses on removing a stale field from the queue summary DTO and ensuring that empty queue lists are serialized as empty arrays to prevent potential frontend crashes.
| CreatedAt time.Time `json:"created_at,omitempty"` | ||
| Attributes map[string]string `json:"attributes,omitempty"` | ||
| Counters QueueCounters `json:"counters"` | ||
| CountersTruncated bool `json:"counters_truncated,omitempty"` |
There was a problem hiding this comment.
The CountersTruncated field appears to be stale. The pull request description explicitly mentions that this field was dropped because the underlying counter implementation in main does not expose truncation information, and the corresponding frontend UI was also removed. Removing this field from the DTO aligns with the rule to remove dead or unreachable code paths to improve code clarity and maintainability.
References
- Remove dead or unreachable code paths, such as asynchronous handlers for tasks that are always performed synchronously, to improve code clarity and maintainability.
| if err := json.NewEncoder(w).Encode(listQueuesResponse{Queues: names}); err != nil { | ||
| h.logger.LogAttrs(r.Context(), slog.LevelWarn, "admin sqs list response encode failed", | ||
| slog.String("error", err.Error()), | ||
| ) | ||
| } |
There was a problem hiding this comment.
If the names slice is nil (which can occur if the underlying storage returns a nil slice for an empty result set), the JSON response will be {"queues": null}. This can cause the frontend to crash when it attempts to access .length or .map() on the queues property in SqsList.tsx. It is safer to ensure that an empty list is always represented as an empty array [] in the JSON output.
| if err := json.NewEncoder(w).Encode(listQueuesResponse{Queues: names}); err != nil { | |
| h.logger.LogAttrs(r.Context(), slog.LevelWarn, "admin sqs list response encode failed", | |
| slog.String("error", err.Error()), | |
| ) | |
| } | |
| if names == nil { | |
| names = []string{} | |
| } | |
| if err := json.NewEncoder(w).Encode(listQueuesResponse{Queues: names}); err != nil { | |
| h.logger.LogAttrs(r.Context(), slog.LevelWarn, "admin sqs list response encode failed", | |
| slog.String("error", err.Error()), | |
| ) | |
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba690577de
ℹ️ 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".
| default: | ||
| return err |
There was a problem hiding this comment.
When AdminDeleteQueue loses leadership between its initial leader check and deleteQueueWithRetry dispatch, the adapter can return wrapped KV leader errors (not adapter.ErrAdminNotLeader), and this switch falls through to default, so the handler emits a 500 instead of the intended 503/Retry-After retry signal. This is a real runtime path in multi-node clusters during elections; the Dynamo bridge already handles the same churn via isLeaderChurnError, so SQS deletes currently regress to non-retryable failures under failover.
Useful? React with 👍 / 👎.
|
Pushed addressing both medium findings from your review:
@claude review |
…s [] Two medium findings from Gemini review on PR #670: 1. CountersTruncated field was stale on internal/admin.QueueSummary. The PR description called out that the field was dropped because main's sqsApproxCounters does not expose truncation, but I missed removing the JSON-side mirror in internal/admin/sqs_handler.go. The SPA was never reading it (SqsDetail.tsx already had the reference removed in this PR), so the field was load-bearing for nothing. Removed. 2. handleList serialised an empty queue catalog as {"queues": null} when AdminListQueues returned nil. The SPA iterates the array directly and would crash on null. Normalise nil to []string{} immediately before encoding so the response shape is always {"queues": []} even on the empty case. No new tests because the existing internal/admin race test suite already exercises the encoder path (would have caught a json.Marshal break) and the empty-catalog case is exactly what the SPA hits the moment a fresh node comes up — manual smoke during PR #649 already verified the SPA renders an empty list cleanly when the array is []. Verified with go build ./..., go test -race ./internal/admin/..., go test -race -run TestSQS ./adapter/, golangci-lint run ./adapter/... ./internal/admin/... — all clean, no //nolint.
|
Claude finished @bootjp's task in 3m 27s —— View job Review
Confirmed fixes ✓Both pushed fixes landed correctly:
P1 —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a8a1d7603
ℹ️ 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".
| writeJSONError(w, http.StatusBadRequest, "invalid_queue_name", "queue name is required") | ||
| return | ||
| } | ||
| if err := h.source.AdminDeleteQueue(r.Context(), principal, name); err != nil { |
There was a problem hiding this comment.
handleDelete checks live authorization with principalCanWrite, but it still calls AdminDeleteQueue with the original JWT principal. In the specific case where a key is promoted from read_only to full while the JWT is still valid, this path authorizes the request and then immediately fails in the adapter because the forwarded role is stale, so queue deletes incorrectly return forbidden until the user logs in again.
Useful? React with 👍 / 👎.
Three findings from Claude review on PR #670: 1. (P1) translateAdminQueuesError did not catch leader-churn errors. AdminDeleteQueue passes the upfront isVerifiedSQSLeader check, then dispatches deleteQueueWithRetry which can hit a kv coordinator that just lost leadership. The resulting kv.ErrLeaderNotFound / adapter.ErrNotLeader / wrapped "not leader" suffixes were not in the translator's switch — they fell to default and the admin handler rendered a generic 500 instead of the spec'd 503 + Retry-After: 1. Added the `case isLeaderChurnError(err)` arm mirroring translateAdminTablesError's identical fix from PR #634. 2. (P2) No tests for translateAdminQueuesError. Mirrored the three Dynamo equivalents in main_admin_test.go: - TestTranslateAdminQueuesError_LeaderChurn covers every kv sentinel + canonical wrapped-suffix variant. - TestTranslateAdminQueuesError_LeaderPhraseInMiddleOfMessage pins the HasSuffix matcher behaviour against false positives on user-supplied error messages mid-string. - TestTranslateAdminQueuesError_UnrelatedErrorPassesThrough confirms the detector does not swallow innocent "leader" mentions outside the canonical phrase set. 3. (Low / polish) SqsList.tsx subtitle leaked the Go file path "adapter/sqs_admin.go" and the internal milestone name "Phase 3.A" to end users — DynamoList / S3List don't do this. Replaced with operator-facing prose describing what the page does. Verified: - go test -run TestTranslateAdminQueuesError . — passes - go build ./... clean - go test -race ./internal/admin/... + go test -race -run TestSQS ./adapter/ — pass - golangci-lint run ./adapter/... ./internal/admin/... ./... — 0 issues - cd web/admin && npm run lint (tsc --strict) clean
|
Pushed addressing all three findings from your review (1 P1, 1 P2, 1 polish): P1 — leader-churn mapping: P2 — error-translator tests: added three table-driven tests parallel to the Dynamo equivalents:
Polish — SPA subtitle: dropped the leaked Skipped the
@claude review |
|
Claude finished @bootjp's task in 3m 18s —— View job Review
Confirmed fixes ✓All three findings from the previous Claude review landed correctly: P1 — P2 — Three new table-driven tests —
Polish — SPA subtitle — P1 —
|
Replaces PR #659, which conflicted heavily after main moved (PR #649 squashed; PR #658 added S3 admin endpoints; the Approximate counters implementation now lives directly in adapter/sqs_catalog.go). This PR: Backend (adapter/sqs_admin.go + internal/admin/sqs_handler.go): - SQSServer.AdminListQueues / AdminDescribeQueue / AdminDeleteQueue are SigV4-bypass entrypoints, mirroring the AdminListTables / AdminListBuckets pattern. - AdminDescribeQueue uses the existing scanApproxCounters from sqs_catalog.go (already on main) so the admin path returns the same Visible / NotVisible / Delayed numbers as GetQueueAttributes("All") would, taken at one snapshot read TS. - sqsQueuesBridge in main_admin.go re-shapes adapter.AdminQueueSummary into admin.QueueSummary, keeping internal/admin free of the heavy adapter dependency tree — same pattern as dynamoTablesBridge / s3BucketsBridge. - admin.QueuesSource is opt-in; deployments that don't run --sqsAddress leave /admin/api/v1/sqs/* off the wire and the SPA renders a soft "endpoint pending" notice on the 404. - Role re-evaluation against the live RoleStore on DELETE so a downgraded key cannot keep mutating with a still-valid JWT. - apiRouteTable.dispatch refactored: resourceHandlerFor extracted so the dispatcher stays under cyclop=10 as new resources land (Dynamo, S3, SQS, future). Frontend (web/admin/src/pages/SqsList.tsx, SqsDetail.tsx): - /sqs queue list with refresh + per-row link to detail. - /sqs/:name detail showing FIFO badge, counters card (Visible / In-flight / Delayed), raw attributes table, and a Delete confirmation Modal gated by RequireFullAccess. - api/client.ts gains listQueues / describeQueue / deleteQueue with the same AbortSignal pattern used for cluster / dynamo / s3 reads. - Layout nav adds an SQS tab between DynamoDB and S3. Out of scope (recorded in the SQS partial design doc §16.2): - PurgeQueue from the SPA. Underlying purgeQueueWithRetry is on main; the admin entrypoint is a trivial follow-up. - Send / Peek / CreateQueue from the SPA. Each needs its own adapter entrypoint and form UX; deferred to keep this PR focused. Verified with go build ./..., go test -race ./internal/admin/..., go test -race -run TestSQS ./adapter/, go test -run TestStartAdmin ., golangci-lint run ./adapter/... ./internal/admin/... ./... (0 issues, no //nolint), and cd web/admin && npm run build.
…s [] Two medium findings from Gemini review on PR #670: 1. CountersTruncated field was stale on internal/admin.QueueSummary. The PR description called out that the field was dropped because main's sqsApproxCounters does not expose truncation, but I missed removing the JSON-side mirror in internal/admin/sqs_handler.go. The SPA was never reading it (SqsDetail.tsx already had the reference removed in this PR), so the field was load-bearing for nothing. Removed. 2. handleList serialised an empty queue catalog as {"queues": null} when AdminListQueues returned nil. The SPA iterates the array directly and would crash on null. Normalise nil to []string{} immediately before encoding so the response shape is always {"queues": []} even on the empty case. No new tests because the existing internal/admin race test suite already exercises the encoder path (would have caught a json.Marshal break) and the empty-catalog case is exactly what the SPA hits the moment a fresh node comes up — manual smoke during PR #649 already verified the SPA renders an empty list cleanly when the array is []. Verified with go build ./..., go test -race ./internal/admin/..., go test -race -run TestSQS ./adapter/, golangci-lint run ./adapter/... ./internal/admin/... — all clean, no //nolint.
Three findings from Claude review on PR #670: 1. (P1) translateAdminQueuesError did not catch leader-churn errors. AdminDeleteQueue passes the upfront isVerifiedSQSLeader check, then dispatches deleteQueueWithRetry which can hit a kv coordinator that just lost leadership. The resulting kv.ErrLeaderNotFound / adapter.ErrNotLeader / wrapped "not leader" suffixes were not in the translator's switch — they fell to default and the admin handler rendered a generic 500 instead of the spec'd 503 + Retry-After: 1. Added the `case isLeaderChurnError(err)` arm mirroring translateAdminTablesError's identical fix from PR #634. 2. (P2) No tests for translateAdminQueuesError. Mirrored the three Dynamo equivalents in main_admin_test.go: - TestTranslateAdminQueuesError_LeaderChurn covers every kv sentinel + canonical wrapped-suffix variant. - TestTranslateAdminQueuesError_LeaderPhraseInMiddleOfMessage pins the HasSuffix matcher behaviour against false positives on user-supplied error messages mid-string. - TestTranslateAdminQueuesError_UnrelatedErrorPassesThrough confirms the detector does not swallow innocent "leader" mentions outside the canonical phrase set. 3. (Low / polish) SqsList.tsx subtitle leaked the Go file path "adapter/sqs_admin.go" and the internal milestone name "Phase 3.A" to end users — DynamoList / S3List don't do this. Replaced with operator-facing prose describing what the page does. Verified: - go test -run TestTranslateAdminQueuesError . — passes - go build ./... clean - go test -race ./internal/admin/... + go test -race -run TestSQS ./adapter/ — pass - golangci-lint run ./adapter/... ./internal/admin/... ./... — 0 issues - cd web/admin && npm run lint (tsc --strict) clean
Codex P2 + Claude P1 follow-up on PR #670. Before this commit, handleDelete called principalCanWrite(principal) which resolved the live role from the RoleStore for the *gate decision* but never updated principal.Role itself. The unmodified principal — still carrying the JWT's stale role — flowed downstream to AdminDeleteQueue, which independently checked principal.Role.canWrite() and rejected the request as ErrAdminForbidden on the promotion edge case (JWT read_only / store full). The user saw 403 and had to log out + back in for the new role to take effect. Refactor mirrors DynamoHandler.principalForWrite (dynamo_handler.go): principalCanWrite is replaced with principalForWrite that returns (AuthPrincipal, bool). On success, the principal carries the *live* role override; failure paths write the response and return false so the caller short-circuits. Five new test cases in internal/admin/sqs_handler_test.go: - DeleteQueue_LivePromotion: pins the actual fix — JWT read_only + store full → 204 succeeds AND src.lastDeletePrincipal.Role is RoleFull (not RoleReadOnly) at the source. - DeleteQueue_LiveRevocation: symmetric — JWT full + store read_only → 403, source not reached. - DeleteQueue_KeyRemovedFromStore: JWT authenticated but key rotated out of the role config → 403. - DeleteQueue_NoRoleStore: single-tenant default — handler trusts JWT role; full→204, read_only→403. - ListQueues_EmptyArrayNotNull: regression net for the prior nil→[] fix; pins {"queues":[]} on empty catalog. Verified: go build ./..., go test -race ./internal/admin/..., golangci-lint run ./adapter/... ./internal/admin/... — all clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 741b5619b1
ℹ️ 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".
| Name: name, | ||
| IsFIFO: meta.IsFIFO, | ||
| Generation: meta.Generation, | ||
| CreatedAt: hlcToTime(meta.CreatedAtHLC), |
There was a problem hiding this comment.
AdminDescribeQueue fills created_at from meta.CreatedAtHLC, but SQS metadata explicitly tracks wall-clock creation time in CreatedAtMillis and CreatedAtHLC can be zero (e.g., older records or when no clock was available). In that case hlcToTime(0) resolves to the Unix epoch, so the admin API returns a bogus 1970-01-01 creation time instead of the real timestamp (or omitting it). This makes queue metadata inaccurate in the SPA for valid persisted queues.
Useful? React with 👍 / 👎.
…lsites Rebase residue: ee521be added the keyvizSampler parameter to startAdminServer; this updates the six test callsites that still pass the pre-rebase argument count.
bootjp
force-pushed
the
feat/sqs-admin-queues-handler
branch
from
741b561 to
d665c4b
Compare
|
Rebased on P1 —
New tests (
Verification
@claude review |
|
Claude finished @bootjp's task in 4m 2s —— View job Review
Confirmed fixes ✓P1 — 5 new tests —
All five are logically correct and the stub's Rebase conflict resolutions — P1 —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d665c4bfff
ℹ️ 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".
| out := map[string]string{ | ||
| "VisibilityTimeout": strconv.FormatInt(meta.VisibilityTimeoutSeconds, 10), | ||
| "MessageRetentionPeriod": strconv.FormatInt(meta.MessageRetentionSeconds, 10), | ||
| "DelaySeconds": strconv.FormatInt(meta.DelaySeconds, 10), | ||
| "ReceiveMessageWaitTimeSeconds": strconv.FormatInt(meta.ReceiveMessageWaitSeconds, 10), |
There was a problem hiding this comment.
The helper says it mirrors GetQueueAttributes("All") minus counters, but this map omits non-counter fields that queueMetaToAttributes includes, notably QueueArn, CreatedTimestamp, and LastModifiedTimestamp. That makes admin describe payloads incomplete versus the canonical SQS attribute surface and breaks parity checks between admin and SigV4 outputs.
Useful? React with 👍 / 👎.
Claude P1 on PR #670 sixth-round review. AdminDescribeQueue was producing CreatedAt from hlcToTime(meta.CreatedAtHLC), but sqsQueueMeta documents HLC as "unsuitable for wall-clock display" and the SigV4 path (sqs_catalog.go:942) reads CreatedAtMillis. Two failure modes the old code exhibited: 1. Records where CreatedAtHLC == 0 (older entries, or zero by accident) rendered as 1970-01-01T00:00:00Z because hlcToTime(0) returns time.Unix(0, 0).UTC() -- not a Go zero time, so omitempty would not drop the field and the SPA created_at guard (created_at ? formatted : "-") would happily render the epoch as a real date. 2. Even with both fields populated, the HLC physical half can differ from the wall-clock CreatedAtMillis by up to hlcPhysicalWindowMs (3 s). Fix: extract adminQueueSummary helper that derives CreatedAt as time.UnixMilli(meta.CreatedAtMillis).UTC() when positive, else a zero time.Time. Mirrors the SigV4 catalog path. New unit tests pin both branches: - TestAdminQueueSummary_CreatedAtUsesMillisNotHLC/zero_millis_yields_zero_time_even_with_HLC_populated - TestAdminQueueSummary_CreatedAtUsesMillisNotHLC/positive_millis_round-trips_via_time.UnixMilli_UTC
|
Pushed addressing the P1 from your sixth-round review. P1 —
var createdAt time.Time
if meta.CreatedAtMillis > 0 {
createdAt = time.UnixMilli(meta.CreatedAtMillis).UTC()
}Both failure modes you flagged are now addressed:
New tests (
Verified:
The minor observation about @claude review |
|
Claude finished @bootjp's task in 2m 8s —— View job Review
Confirmed fix ✓P1 — var createdAt time.Time
if meta.CreatedAtMillis > 0 {
createdAt = time.UnixMilli(meta.CreatedAtMillis).UTC()
}Both failure modes are pinned by the two new tests (
The All prior fixes ( P1 —
|
| Field | Source | Gap |
|---|---|---|
QueueArn |
s.queueArn(name) → "arn:aws:sqs:"+region+":000000000000:"+name |
Absent |
CreatedTimestamp |
meta.CreatedAtMillis / sqsMillisPerSecond |
Absent (but created_at is surfaced as a typed field) |
LastModifiedTimestamp |
meta.LastModifiedAtMillis / sqsMillisPerSecond |
Absent — not surfaced anywhere |
CreatedTimestamp is redundant with the dedicated AdminQueueSummary.CreatedAt field, so its absence from the attributes map is defensible and the SPA renders it correctly from detail.data.created_at. No real gap there.
LastModifiedTimestamp is the live defect: it's missing from both the typed struct and the attributes map, so operators have no way to see when a queue was last modified from the admin UI — SetQueueAttributes updates LastModifiedAtMillis (sqs_catalog.go:1036) but that value never surfaces here.
QueueArn is architecturally constrained — adminQueueSummary is a pure helper (by design, for unit-testability) and doesn't have the server's region. However, the ARN formula is mechanical ("arn:aws:sqs:" + region + ":000000000000:" + name); it could be passed as a parameter to metaAttributesForAdmin from AdminDescribeQueue, which does have access to s.effectiveRegion().
Minimal fix — corrects the two real gaps and the docstring:
// metaAttributesForAdmin renders the non-counter queue config attributes.
// Mirrors queueMetaToAttributes("All") except counters (surfaced as a typed
// struct) and CreatedTimestamp (surfaced as AdminQueueSummary.CreatedAt).
// Includes QueueArn and LastModifiedTimestamp.
func metaAttributesForAdmin(meta *sqsQueueMeta, queueArn string) map[string]string {
out := map[string]string{
"QueueArn": queueArn,
"VisibilityTimeout": strconv.FormatInt(meta.VisibilityTimeoutSeconds, 10),
// ... (existing fields unchanged) ...
}
if meta.LastModifiedAtMillis > 0 {
out["LastModifiedTimestamp"] = strconv.FormatInt(meta.LastModifiedAtMillis/sqsMillisPerSecond, 10)
}
if meta.RedrivePolicy != "" {
out["RedrivePolicy"] = meta.RedrivePolicy
}
return out
}Then adminQueueSummary passes s.queueArn(name) through — but adminQueueSummary is currently a package-level function, not a method. The cleanest approach is either to pass queueArn string as a fourth parameter to adminQueueSummary, or to move the metaAttributesForAdmin call up into AdminDescribeQueue (which is a method and has s.queueArn).
Everything else ✓
principalForWritelive-role forwarding confirmed atsqs_handler.go:246–266.translateAdminQueuesErrorisLeaderChurnErrorarm — confirmed atmain_admin.go:214–226.- Three
TestTranslateAdminQueuesError_*tests — confirmed. - nil→[] normalisation at
sqs_handler.go:167–169— confirmed. scanApproxCounterserror (sqs_admin.go:69) — still unwrapped, butscanApproxCounterswraps internally and the linter is satisfied; no change needed.- Both new
TestAdminQueueSummary_CreatedAtUsesMillisNotHLCsubtests are logically correct and uses3HLCPhysicalShiftwhich is the correct constant for this package. - SPA subtitle (
SqsList.tsx) — confirmed clean operator prose, no source path or milestone reference.
Bottom line: The CreatedAt P1 and its tests are correct. The one remaining issue before merge is that metaAttributesForAdmin is missing QueueArn and LastModifiedTimestamp while its docstring claims parity with queueMetaToAttributes("All"). LastModifiedTimestamp is the live gap for operators; QueueArn requires passing the ARN as a parameter.
Claude P1 on PR #670 seventh-round review. metaAttributesForAdmin docstring claimed parity with queueMetaToAttributes("All") but QueueArn and LastModifiedTimestamp were absent. LastModifiedTimestamp is the live operator-visible gap: SetQueueAttributes updates meta.LastModifiedAtMillis but the value never surfaced in the admin UI, so operators had no way to tell when a queue config was last touched. QueueArn is the AWS-shaped identifier the SPA shows for change tracking; the SigV4 path always emits it. CreatedTimestamp deliberately stays out of the attrs map -- it is surfaced as the typed AdminQueueSummary.CreatedAt field instead so the SPA does not have to round-trip strings. adminQueueSummary now takes queueArn as a fourth argument; the caller (AdminDescribeQueue) threads s.queueArn(name) through. The helper stays method-free so it remains unit-testable without a coordinator. Updated docstrings to match. New tests in adapter/sqs_admin_test.go pin the contract: - TestMetaAttributesForAdmin_IncludesQueueArnAndLastModified/QueueArn_always_present - /LastModifiedTimestamp_emitted_in_unix_seconds_when_populated - /LastModifiedTimestamp_omitted_when_zero - /CreatedTimestamp_deliberately_not_in_map_(typed_field_instead) Existing TestAdminQueueSummary_CreatedAtUsesMillisNotHLC subtests updated for the new signature.
|
Pushed addressing the seventh-round P1. P1 — Both fields are now emitted, mirroring out := map[string]string{
"QueueArn": queueArn,
// ... existing config fields ...
}
if mod := meta.LastModifiedAtMillis; mod > 0 {
out["LastModifiedTimestamp"] = strconv.FormatInt(mod/sqsMillisPerSecond, 10)
}
New tests (
The two existing Verified:
@claude review |
|
Claude finished @bootjp's task in 2m 15s —— View job Review
|
| Field | queueMetaToAttributes("All") |
metaAttributesForAdmin |
|---|---|---|
VisibilityTimeout |
✓ | ✓ |
MessageRetentionPeriod |
✓ | ✓ |
DelaySeconds |
✓ | ✓ |
ReceiveMessageWaitTimeSeconds |
✓ | ✓ |
MaximumMessageSize |
✓ | ✓ |
FifoQueue |
✓ | ✓ |
ContentBasedDeduplication |
✓ | ✓ |
QueueArn |
✓ | ✓ (now added) |
CreatedTimestamp |
✓ | deliberately omitted (typed AdminQueueSummary.CreatedAt) |
LastModifiedTimestamp |
✓ (conditional > 0) |
✓ (now added, same condition) |
Approximate* counters |
✓ | deliberately omitted (typed AdminQueueCounters) |
RedrivePolicy |
✓ (conditional) | ✓ (conditional) |
Parity is now complete for all non-counter, non-CreatedTimestamp fields. Docstring (sqs_admin.go:127–143) accurately documents the two deliberate omissions with clear rationale.
adminQueueSummary signature update — queueArn string added as 4th param, threaded from AdminDescribeQueue via s.queueArn(name) (sqs_admin.go:71). Helper stays method-free for unit-testability.
4 new subtests — sqs_admin_test.go:69–125 are correct and comprehensive:
QueueArn_always_present— verifies field and value.LastModifiedTimestamp_emitted_in_unix_seconds_when_populated— pins the millis→seconds conversion (wantMillis/sqsMillisPerSecond) with a concrete reference timestamp.LastModifiedTimestamp_omitted_when_zero— guards the 1970 epoch regression class.CreatedTimestamp_deliberately_not_in_map_(typed_field_instead)— excellent regression guard; pins the deliberate omission so a future contributor doesn't "fix" it back and create two sources of truth for the same field.
Existing TestAdminQueueSummary_CreatedAtUsesMillisNotHLC subtests — correctly updated for the new testQueueArn constant at sqs_admin_test.go:35 and 50.
SPA rendering — SqsDetail.tsx:87–99 uses Object.entries(detail.data.attributes).map(...), so QueueArn and LastModifiedTimestamp surface automatically in the "Configuration" section with no SPA changes needed. ✓
All prior fixes intact:
principalForWritelive-role forwarding — confirmed atsqs_handler.go:237.translateAdminQueuesErrorisLeaderChurnErrorarm — confirmed atmain_admin.go:216, 228.
Nit — PR reference in production docstring (CLAUDE.md §Conventions)
sqs_admin.go:141–143:
// Claude P1 on PR #670 caught both gaps — the prior
// docstring claimed parity with queueMetaToAttributes("All") but
// QueueArn and LastModifiedTimestamp were absent.CLAUDE.md explicitly says: "Don't reference the current task, fix, or callers... since those belong in the PR description and rot as the codebase evolves." The preceding lines already explain the rationale for each field (operators need it for change-tracking, etc.), so the PR attribution is redundant and will become stale. Drop those three lines and the docstring stands on its own. Small fix if you want to clean it up before merge, but not blocking.
Everything else ✓
sqsMillisPerSecond = 1000(fromsqs_messages.go:55) — the test's division is arithmetically correct.adminQueueSummaryis method-free as promised; thequeueArnparam keeps it fully unit-testable.- The conditional
LastModifiedTimestampguard (> 0) matches the SigV4 path atsqs_catalog.go:946exactly.
Bottom line: The P1 is fully fixed and its tests are the best in the file. The nit about the PR reference in the docstring is cosmetic and CLAUDE.md-governed — clean it up or leave it, either way this PR is ready to merge.
CLAUDE.md Conventions: "Don't reference the current task, fix, or callers (added for the X flow, handles the case from issue #123) since those belong in the PR description and rot as the codebase evolves." Removes four PR-attribution lines added across this PR's earlier commits: - adapter/sqs_admin.go: metaAttributesForAdmin docstring tail. - internal/admin/sqs_handler.go handleList: nil normalisation note. - internal/admin/sqs_handler.go principalForWrite: docstring tail. - main_admin.go translateAdminQueuesError: leader-churn arm note. The why-this-line-exists rationale stays in each spot; only the "Codex P2 + Claude P1 on PR #N caught the bug" sentences and the sibling-PR cross-references are dropped. Flagged as a non-blocking nit by Claude P1 on PR #670 seventh-round review (the docstring on metaAttributesForAdmin specifically); the other three are the same convention violation in adjacent code so they go in the same cleanup.
|
Cleaned up the CLAUDE.md-governed nit you flagged ("Don't reference the current task, fix, or callers since those belong in the PR description and rot as the codebase evolves"). Dropped four PR-attribution lines added across this PR's earlier commits, in the spots where the convention applies:
Same cleanup pattern across all four: the why-this-line-exists rationale stays (the bug class, the failure mode, the mirror with the Dynamo path), only the "Codex P2 + Claude P1 on PR #N caught the bug" sentence and sibling-PR cross-references are removed. The history lives in the PR descriptions and Verified: I didn't touch the older PR references at @claude review |
|
Claude finished @bootjp's task in 1m 39s —— View job Review
Confirmed cleanup ✓All four attribution removals landed cleanly:
Grep confirms the only remaining PR/reviewer attributions in All prior fixes still intact ✓
This PR is ready to merge. Every finding across all eight review rounds has been addressed. The codebase is in the state the CLAUDE.md conventions require: no PR/reviewer attributions in production comments, full parity between the admin and SigV4 surfaces, live-role forwarding on delete, and 503+Retry-After on leader churn. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f0e26b992
ℹ️ 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".
| Name string `json:"name"` | ||
| IsFIFO bool `json:"is_fifo"` | ||
| Generation uint64 `json:"generation"` | ||
| CreatedAt time.Time `json:"created_at,omitempty"` |
There was a problem hiding this comment.
created_at is declared as time.Time with omitempty, but this handler uses goccy/go-json, which still serializes zero time.Time values (e.g. "0001-01-01T00:00:00Z") instead of omitting them. For queues where CreatedAtMillis is unset, the API therefore returns a bogus timestamp and the SPA renders a fake ancient date instead of —, so the current "zero time means omitted" logic does not hold.
Useful? React with 👍 / 👎.
| const [deleting, setDeleting] = useState(false); | ||
| const [deleteError, setDeleteError] = useState<string | null>(null); | ||
| const navigate = useNavigate(); | ||
| const writeAllowed = session?.role === "full"; |
There was a problem hiding this comment.
The delete button is shown only when session?.role === "full", but that role is captured at login and can become stale while the token is still valid. In the promoted-key scenario (read_only → full), backend delete now succeeds via live role revalidation, yet the SPA still hides the action until re-login, so operators cannot perform queue deletes from the UI despite being currently authorized.
Useful? React with 👍 / 👎.
…ate (Codex P2) Two P2 findings from the Codex review on commit 0f0e26b: (1) created_at "0001-01-01T00:00:00Z" leak. QueueSummary.CreatedAt was time.Time with json:"created_at,omitempty", but neither encoding/json nor goccy/go-json drops a zero time.Time value -- it serialises as "0001-01-01T00:00:00Z" and the SPA renders a fake ancient date instead of the "--" placeholder its created_at ? formatted : "--" guard implies. Fix: changed CreatedAt to *time.Time on the wire-facing QueueSummary struct. The bridge in main_admin.go converts a zero time.Time on the adapter side to nil so omitempty drops the field at the wire layer. New regression test TestSqsHandler_DescribeQueue_ZeroCreatedAtIsOmittedOnTheWire exercises the wire body and asserts the literal "0001-01-01..." string never appears, complementing the Go-side IsZero() check the adapter unit test already pins. (2) SPA delete button hidden on JWT role staleness. The promoted read_only -> full key path principalForWrite added in this PR correctly forwards the live role to AdminDeleteQueue, but SqsDetail.tsx gated the delete button on session?.role === "full" (the JWT role cached in sessionStorage at login). Promoted users saw the button hidden until re-login, defeating the live-role fix end-to-end. Fix: dropped the JWT-based gating on SqsDetail. The backend's principalForWrite is the authoritative gate. A read_only operator who clicks delete gets 403 in the modal's error area, which is clearer feedback than a silently missing button. (S3List/S3Detail and DynamoList/DynamoDetail have the same pattern but are out of scope for this PR -- they need the same treatment in a follow-up or a shared session-refresh endpoint.)
|
Codexの2件のP2指摘を反映しました。 P2 (1) — Codexの指摘どおり、
新しい回帰テスト P2 (2) — SPA の delete ボタンが JWT の古いロールで隠れる このPRで入れた
なお @claude review |
|
Claude finished @bootjp's task in 2m 47s —— View job Review
Confirmed fixes ✓P2 (1) —
CreatedAt *time.Time `json:"created_at,omitempty"`
var createdAt *time.Time
if !in.CreatedAt.IsZero() {
t := in.CreatedAt
createdAt = &t
}The three-way behavior is fully correct:
Adapter side (
P2 (2) — SPA delete button ungated from JWT role
Nit — PR/reviewer attributions in test docstrings (CLAUDE.md §Conventions)The prior cleanup round cleared these from production code. The test file still has them at three spots:
// Mirrors stubTablesSource in dynamo_handler_test.go: records the
// principal that flowed through to the source so tests can assert
// live-role forwarding (Codex P2 + Claude P1 on PR #670).
// TestSqsHandler_ListQueues_EmptyArrayNotNull pins the nil→[]
// normalisation added in the Gemini PR #670 medium fix. Without it
// TestSqsHandler_DescribeQueue_ZeroCreatedAtIsOmittedOnTheWire pins
// the wire-level fix for the Codex P2 finding on PR #670: time.TimeCLAUDE.md: "Don't reference the current task, fix, or callers … since those belong in the PR description and rot as the codebase evolves." The rationale in each docstring (what the bug was, what the test pins) is genuinely useful; it's only the Everything else ✓
Bottom line: Both P2 fixes are correct and well-tested. The only remaining item is the cosmetic PR-attribution cleanup in three test docstrings — same CLAUDE.md convention as the prior round's production-code cleanup, missed in the test file. |
Same CLAUDE.md cleanup as 0f0e26b applied to the test file: the prior round cleaned production code but left the test file references behind. Why-rationale stays (the bug class, the contract the test pins); only the "Codex P2 + Claude P1 on PR #670", "Gemini PR #670 medium fix", and "Codex P2 finding on PR #670" attributions are dropped. Three docstrings touched: - stubQueuesSource live-role recorder. - TestSqsHandler_DeleteQueue_LivePromotion. - TestSqsHandler_ListQueues_EmptyArrayNotNull. - TestSqsHandler_DescribeQueue_ZeroCreatedAtIsOmittedOnTheWire.
|
テストファイル側の PR/reviewer attribution を CLAUDE.md convention に合わせて削除しました( 各 docstring の why-rationale (バグの分類、テストが pin する contract) は保持し、 Verified: これで全 review round の指摘がクリアになったはずです。マージ可能です。 @claude review |
|
Claude finished @bootjp's task in 3m 21s —— View job Review
Confirmed —
|
| Fix | Location | Status |
|---|---|---|
CountersTruncated absent |
sqs_handler.go:35–42 |
✓ |
nil→[] list normalisation |
sqs_handler.go:174–176 |
✓ |
principalForWrite live-role forwarding |
sqs_handler.go:244–279 |
✓ |
translateAdminQueuesError leader-churn arm |
main_admin.go:227–237 |
✓ |
CreatedAt uses CreatedAtMillis, not HLC |
sqs_admin.go:86–88 |
✓ |
metaAttributesForAdmin has QueueArn + LastModifiedTimestamp |
sqs_admin.go:142–160 |
✓ |
*time.Time pointer + nil-bridge in convertAdminQueueSummary |
sqs_handler.go:39, main_admin.go:192–196 |
✓ |
| SPA delete button ungated from JWT role | SqsDetail.tsx:14–21 |
✓ |
| SPA subtitle — clean operator prose | SqsList.tsx:13–15 |
✓ |
Nit — three remaining PR attributions in adapter test file and main_admin_test.go
The prior cleanup round (7c63a7b0) caught the three instances in sqs_handler_test.go but missed these:
adapter/sqs_admin_test.go:11–14 — change:
// TestAdminQueueSummary_CreatedAtUsesMillisNotHLC pins the regression
// from the fifth-round Claude review on PR #670: the admin
// AdminDescribeQueue path was producing CreatedAt from
// hlcToTime(meta.CreatedAtHLC), but sqsQueueMeta documents HLC asto:
// TestAdminQueueSummary_CreatedAtUsesMillisNotHLC pins the contract that
// AdminDescribeQueue derives CreatedAt from CreatedAtMillis, not HLC.
// sqsQueueMeta documents HLC asadapter/sqs_admin_test.go:60–67 — change:
// TestMetaAttributesForAdmin_IncludesQueueArnAndLastModified pins the
// gap from the seventh-round Claude review on PR #670: the prior
// metaAttributesForAdmin docstring claimed parity with
// queueMetaToAttributes("All") but neither QueueArn nor
// LastModifiedTimestamp was emitted. QueueArn is the AWS-shaped
// identifier the SPA shows for change-tracking; LastModifiedTimestamp
// is updated on SetQueueAttributes and is the only handle operators
// have on "when did somebody last touch this queue's config".to:
// TestMetaAttributesForAdmin_IncludesQueueArnAndLastModified pins parity
// with queueMetaToAttributes("All"): metaAttributesForAdmin must emit
// QueueArn (the AWS-shaped identifier the SPA shows) and
// LastModifiedTimestamp (updated on SetQueueAttributes and the only
// handle operators have on "when did somebody last touch this queue's
// config").main_admin_test.go:436 — remove the last sentence of the TestTranslateAdminQueuesError_LeaderChurn comment:
// Codex P2 + Claude P1 on PR #670 confirmed the original gap.I prepared these three edits as a local commit (15d2f63) but push was blocked by repo permissions. The changes are purely cosmetic doc cleanup — same class as the prior rounds. Not blocking; apply the diff above and this PR is ready to merge.
Bottom line: Every substantive finding across all review rounds is confirmed fixed. The three remaining cosmetic lines are in test docstrings only. Once those are pushed, the PR is clean end-to-end.
Catches the three locations the prior round (7c63a7b) missed: adapter/sqs_admin_test.go (TestAdminQueueSummary_CreatedAtUsesMillisNotHLC, TestMetaAttributesForAdmin_IncludesQueueArnAndLastModified) and main_admin_test.go (TestTranslateAdminQueuesError_LeaderChurn). Same pattern: the why-rationale stays, only the "fifth-round Claude review on PR #670" / "seventh-round Claude review on PR #670" / "Codex P2 + Claude P1 on PR #670" attributions are dropped. Pre-existing PR #634 references at main_admin_test.go:373 / :397 were added by a prior merged PR and are out of scope here.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
最後の3箇所をクリーンアップしました(
Each docstring の why-rationale (バグ class、test が pin する contract) は保持。
Verified: これで全ての review round の指摘がクリアです。 @claude review |
|
Claude finished @bootjp's task in 1m 33s —— View job Review
Confirmed cleanup ✓All three attribution removals from
The two remaining All functional fixes confirmed intact ✓
This PR is ready to merge. All findings across all review rounds are confirmed resolved: no PR/reviewer attributions remain in any of the newly-added files, all functional correctness fixes are intact, and the test suite covers every fixed regression path. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c63a7b09d
ℹ️ 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".
| if strings.TrimSpace(name) == "" { | ||
| return nil, false, ErrAdminSQSValidation |
There was a problem hiding this comment.
AdminDescribeQueue currently treats any non-blank string as a valid queue name, so malformed names (for example bad*name or overlength names) flow into metadata lookup and are reported as not found instead of a 400 validation error. That contradicts this file’s own ErrAdminSQSValidation contract (“missing or syntactically-bad queue name”) and diverges from the regular SQS path that uses validateQueueName; AdminDeleteQueue has the same gap. Reusing canonical queue-name validation here would preserve consistent client-visible semantics for invalid input.
Useful? React with 👍 / 👎.
1 participant
Summary
Replaces #659, which has unresolvable conflicts now that main has moved on (PR #649 squashed into main; PR #658 added the S3 admin endpoints; the Approximate counters implementation now lives directly in
adapter/sqs_catalog.goviascanApproxCounters). Rather than a multi-day rebase, this PR re-applies the unique SQS admin code on a fresh branch off current main.What survived from #659:
adapter/sqs_admin.go—SQSServer.AdminListQueues/AdminDescribeQueue/AdminDeleteQueue(SigV4-bypass entrypoints, same shape asAdminListTables/AdminListBuckets).internal/admin/sqs_handler.go— HTTP handler for/admin/api/v1/sqs/queues{,/{name}}with role re-evaluation on DELETE.web/admin/src/pages/SqsList.tsx/SqsDetail.tsx— SPA pages for the queues view + delete confirmation.What changed during the re-apply:
AdminQueueCountersis nowint64(matchessqsApproxCountersfrom main; bridge does no width conversion).AdminDescribeQueuecalls main'sscanApproxCountersinstead of the duplicatecomputeApproxCountersfrom the old branch — same numeric output, single implementation.CountersTruncatedfield; main's counter type doesn't expose truncation. SPA's "truncated" pill came out with it.apiRouteTable.dispatchrefactored to extractresourceHandlerForso the dispatcher stays under cyclop=10 as new resources land.Backend
MapRoleStoreon everyDELETEso a downgraded key cannot keep mutating with a still-valid JWT (Codex P1 pattern from earlier admin PRs).admin.QueuesSourceis opt-in: deployments without--sqsAddressleave/admin/api/v1/sqs/*off the wire; the SPA renders a soft "endpoint pending" notice on the 404, mirroring the Tables / Bucketsnilcontract.main_admin.go(sqsQueuesBridge,convertAdminQueueSummary,translateAdminQueuesError) keepsinternal/adminfree of the heavy adapter dependency tree, same architectural pattern as Dynamo and S3.Frontend
Modalgated byRequireFullAccess.api/client.tsgainslistQueues/describeQueue/deleteQueuewith the sameAbortSignalpattern used forcluster/dynamo/s3reads.Out of scope (recorded in
docs/design/2026_04_24_proposed_sqs_compatible_adapter.mdSection 14, deferred per the SQS partial doc §16.2)purgeQueueWithRetryadapter method is on main; the admin entrypoint is a trivial follow-up.Test plan
go build ./...— cleango test -race ./internal/admin/...— passesgo test -race -run TestSQS ./adapter/— passesgo test -run TestStartAdmin .— passesgolangci-lint run ./adapter/... ./internal/admin/... ./...—0 issues., no//nolintcd web/admin && npm run build— 49 modules, 199 KB JS / 61 KB gzip + 14.7 KB CSS--sqsAddress :4566 --adminEnabled --adminAllowInsecureDevCookie, create a queue, send a few messages, hit/admin/sqs/<name>→ counters matchGetQueueAttributes("All"), Delete dialog returns to list.Self-review (5 lenses)
deleteQueueWithRetryOCC path; counters are read-only. No new write paths.sqsApproxCounterScanLimit; admin reads are cheap point lookups + one bounded scan.AdminDescribeQueueand SigV4GetQueueAttributesboth callscanApproxCountersat a freshnextTxnReadTS, so a single point in time produces the same counters via either surface.nilQueues argument added tostartAdminServercall sites; the new bridge is exercised by the cross-package build itself.Stacking
This PR is independent — branched from current
main(which has the merged versions of #649 / #658 / #650 / counter implementation). Closing #659 in favour of this clean rewrite.