adapter/dynamodb: item-level admin RPCs (Phase 2a)#805
Conversation
…nDeleteItem
Phase 2a of docs/design/2026_05_22_proposed_admin_data_browser.md.
Adds the four item-level RPCs the design doc §3.1.1 calls for,
backed by the adapter's existing internal helpers (scanItems,
loadTableSchemaAt + readLogicalItemAt, putItemWithRetry,
deleteItemWithRetry). No new write paths — each admin RPC is a
gated wrapper over an existing internal helper.
## Surface
- AdminAttributeValue (exported mirror of the internal flat AWS-
wire shape, recursive for L and M)
- AdminItem, AdminItemKey, AdminScanResult, AdminScanOptions
- AdminScanTable(ctx, principal, name, opts) - read role required;
Limit clamped to [1, 100] with default 25 per design §3.1.1
- AdminGetItem(ctx, principal, name, key) - returns
(nil, false, nil) on absence so admin handlers can render 404
without sniffing sentinels (matches AdminDescribeTable)
- AdminPutItem(ctx, principal, name, item) - write role required
- AdminDeleteItem(ctx, principal, name, key) - write role required
- ErrAdminDynamoNotFound / ErrAdminDynamoValidation sentinels
- translateDynamoAdminError maps dynamoAPIError(ResourceNotFound)
to ErrAdminDynamoNotFound and dynamoAPIError(ValidationException)
to ErrAdminDynamoValidation, leaving the original message
available via AdminErrorMessage for the wrapped error chain
## Authorization
- Reads: principal.Role.canRead() (RoleReadOnly or RoleFull)
- Writes: principal.Role.canWrite() (RoleFull only)
- Leader check via isVerifiedDynamoLeader on every call (the
underlying scan / put / delete already verify, but the up-front
check returns ErrAdminNotLeader rather than the adapter's
503-shaped sqsAPIError)
## Caller audit (semantic-change rule)
No existing signatures changed. The internal helpers scanItems,
loadTableSchemaAt, readLogicalItemAt, putItemWithRetry, and
deleteItemWithRetry are unchanged; the admin RPCs are pure
gated wrappers.
The exported AdminAttributeValue type mirrors the unexported
attributeValue field-for-field; conversion functions
(adminToInternalAttributeValue / internalToAdminAttributeValue and
their map variants) are unit-tested via
TestAdminAttributeValue_NestedRoundTrip for the recursive L / M
case.
## Tests
12 new unit tests:
- HappyPath put-then-get round-trip with String / Number / Bool
- GetItem missing - (nil, false, nil)
- GetItem on missing table - ErrAdminDynamoNotFound
- DeleteItem happy path
- Scan happy path (3 items)
- Scan limit clamping (under cap, over cap)
- Scan cursor round-trip across two pages
- PutItem / DeleteItem read-only forbidden
- Scan with role-less principal forbidden
- PutItem with empty table name validation
- GetItem with empty key validation
- clampAdminScanLimit truth table
- AdminAttributeValue nested L/M round-trip
Verification:
- go test -race ./adapter/... passes (the 12 new tests + all
existing dynamo admin tests)
- golangci-lint run ./adapter/... 0 issues
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (4)
📝 WalkthroughWalkthroughAdds an admin DynamoDB adapter: admin wire types and JSON codec, AdminScanTable/AdminGetItem/AdminPutItem/AdminDeleteItem RPCs with authorization/leader checks, validation and limit clamping, bidirectional attribute conversions preserving empty containers, error translation, and comprehensive tests. ChangesAdmin DynamoDB Operations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@claude review |
|
Claude finished @bootjp's task in 4m 55s —— View job Code Review: adapter/dynamodb: item-level admin RPCs (Phase 2a)
Overall: LGTM with two findings to address before Phase 3 HTTP handlers land — one real behavioral gap (medium) and one cosmetic/contract concern (low). The structure, auth gates, and test suite are solid. Finding 1 — Medium:
|
![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: b48f7b7215
ℹ️ 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".
| ExclusiveStartKey: adminToInternalAttributeMap(opts.ExclusiveStart), | ||
| Limit: &limit, | ||
| } | ||
| out, err := d.scanItems(ctx, scanInput) |
There was a problem hiding this comment.
AdminScanTable allows read_only principals but delegates to d.scanItems, whose read-schema path calls ensureLegacyTableMigration and can dispatch write transactions when a legacy table is encountered. In clusters that still have pre-migration schemas, a read-only admin scan can therefore mutate metadata/data and repeatedly generate Raft write load (for example during dashboard polling), which violates the read-only authorization contract and can impact production stability.
Useful? React with 👍 / 👎.
| internalKey := adminToInternalAttributeMap(key) | ||
| current, found, err := d.readLogicalItemAt(ctx, schema, internalKey, readTS) | ||
| if err != nil { | ||
| return nil, false, translateDynamoAdminError(err) |
There was a problem hiding this comment.
When the key shape is wrong (e.g., missing the table hash key but len(key) > 0), readLogicalItemAt returns plain errors like missing hash key attribute; translateDynamoAdminError does not classify these as validation and falls through to a generic wrapped error. This means malformed AdminGetItem inputs can be surfaced as internal failures instead of ErrAdminDynamoValidation, breaking the documented error contract and preventing correct 400 handling by callers.
Useful? React with 👍 / 👎.
![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 administrative CRUD operations for DynamoDB items, including Scan, Get, Put, and Delete functionality. It defines the necessary data structures and provides conversion logic between internal and admin-facing attribute types, supported by a comprehensive test suite. The review feedback identifies several critical improvements: implementing recursion depth checks in conversion functions to prevent potential DoS attacks, performing deep copies of byte slices and sets for data isolation, returning empty slices instead of nil to ensure consistent JSON serialization, and standardizing error translation across all admin RPCs.
| func internalToAdminAttributeValue(v attributeValue) AdminAttributeValue { | ||
| out := AdminAttributeValue{ | ||
| S: v.S, N: v.N, B: v.B, BOOL: v.BOOL, NULL: v.NULL, | ||
| SS: v.SS, NS: v.NS, BS: v.BS, | ||
| } | ||
| if len(v.L) > 0 { | ||
| out.L = make([]AdminAttributeValue, len(v.L)) | ||
| for i, e := range v.L { | ||
| out.L[i] = internalToAdminAttributeValue(e) | ||
| } | ||
| } | ||
| if len(v.M) > 0 { | ||
| out.M = make(map[string]AdminAttributeValue, len(v.M)) | ||
| for k, e := range v.M { | ||
| out.M[k] = internalToAdminAttributeValue(e) | ||
| } | ||
| } | ||
| return out | ||
| } | ||
|
|
||
| func adminToInternalAttributeValue(v AdminAttributeValue) attributeValue { | ||
| out := attributeValue{ | ||
| S: v.S, N: v.N, B: v.B, BOOL: v.BOOL, NULL: v.NULL, | ||
| SS: v.SS, NS: v.NS, BS: v.BS, | ||
| } | ||
| if len(v.L) > 0 { | ||
| out.L = make([]attributeValue, len(v.L)) | ||
| for i, e := range v.L { | ||
| out.L[i] = adminToInternalAttributeValue(e) | ||
| } | ||
| } | ||
| if len(v.M) > 0 { | ||
| out.M = make(map[string]attributeValue, len(v.M)) | ||
| for k, e := range v.M { | ||
| out.M[k] = adminToInternalAttributeValue(e) | ||
| } | ||
| } | ||
| return out |
There was a problem hiding this comment.
The recursive conversion functions internalToAdminAttributeValue and adminToInternalAttributeValue lack a recursion depth check. To prevent unbounded resource usage and potential DoS from external requests, apply a fixed bound to the nesting depth. While the internal attributeValue type enforces a limit of 32, the exported AdminAttributeValue type should also enforce the maxAttributeValueNestingDepth limit.
References
- To prevent unbounded memory growth and potential OOM issues, apply a fixed bound to collections that can grow from external requests.
- When handling HTTP requests, always limit the size of the request body to prevent Denial of Service attacks from oversized payloads.
| S: v.S, N: v.N, B: v.B, BOOL: v.BOOL, NULL: v.NULL, | ||
| SS: v.SS, NS: v.NS, BS: v.BS, | ||
| } |
There was a problem hiding this comment.
The conversion functions perform shallow copies of byte slices and string/binary sets. Since AdminAttributeValue is an exported type that might be returned to external callers or passed between layers, it is safer to perform deep copies (cloning) of these fields to ensure data isolation and prevent side effects if the underlying buffers are reused or modified.
| S: v.S, N: v.N, B: v.B, BOOL: v.BOOL, NULL: v.NULL, | |
| SS: v.SS, NS: v.NS, BS: v.BS, | |
| } | |
| S: v.S, N: v.N, B: bytes.Clone(v.B), BOOL: v.BOOL, NULL: v.NULL, | |
| SS: slices.Clone(v.SS), NS: slices.Clone(v.NS), BS: cloneBinarySet(v.BS), |
| // (the scanItems output) into the admin-facing AdminItem slice. Nil | ||
| // in, nil out so callers see the contract directly. | ||
| func adminItemsFromInternal(items []map[string]attributeValue) []AdminItem { | ||
| if len(items) == 0 { |
There was a problem hiding this comment.
Returning nil for an empty items slice results in a null JSON value. To avoid silently dropping entries during serialization and maintain consistency in the response shape for the client, return an empty array [] instead.
return []AdminItem{}References
- Avoid silently dropping entries during serialization or normalization. Provide a specific handling path for incomplete entries to ensure they are preserved and avoid state inconsistencies.
| readTS := d.nextTxnReadTS() | ||
| schema, exists, err := d.loadTableSchemaAt(ctx, tableName, readTS) | ||
| if err != nil { | ||
| return nil, false, errors.WithStack(err) |
There was a problem hiding this comment.
For consistency with the rest of the admin RPCs, errors from loadTableSchemaAt should be passed through translateDynamoAdminError. Although loadTableSchemaAt typically returns exists=false for missing tables, using the translator ensures that any unexpected validation or resource errors are correctly mapped to admin sentinels.
| return nil, false, errors.WithStack(err) | |
| return nil, false, translateDynamoAdminError(err) |
… key Address PR #805 r1 review (Codex P1 / P2, Claude bot Low). ## P1: read-only contract violation in AdminScanTable scanItems dispatches through prepareReadSchema, which calls ensureLegacyTableMigration if the table is on the legacy key layout — that path proposes Raft writes to migrate the schema in-place. A read-role admin reaching AdminScanTable could trigger those writes (a) violating the read-only authorization contract and (b) generating one round-trip of write load per dashboard poll until the migration finishes. Pre-load the schema with loadTableSchemaAt at the admin entry point and refuse with ErrAdminDynamoValidation if needsLegacyKeyMigration() is true. The error message points the operator at the SigV4 surface (the one place migrations are intended to fire). ## P2: opaque error on malformed AdminGetItem key readLogicalItemAt → itemKeyFromAttributes surfaces a plain errors.New chain on a missing/mistyped hash key, which the admin HTTP bridge would render as a 500 instead of the 400 contract. Pre-validate the key shape via schema.itemKeyFromAttributes inside AdminGetItem and wrap any error as ErrAdminDynamoValidation so the bridge can map it to 400. To stay under cyclop's max=10 the read-path preamble (role gate, leader check, table-name + non-empty-key validation, schema load, legacy refusal) is factored into adminLoadReadableSchema, which both AdminGetItem and any future read-path admin RPC can reuse. ## Low: empty Items rendered as null adminItemsFromInternal returned nil for empty input, which JSON marshals as "Items": null — the SPA's items.map(...) call would crash. make([]AdminItem, 0) renders as "Items": [], so drop the nil short-circuit. ## Tests Four new tests: - TestDynamoDB_AdminScanTable_EmptyTableReturnsEmptySlice — locks down the [] vs null contract - TestDynamoDB_AdminPutItem_MissingTable — write path returns ErrAdminDynamoNotFound when the schema is absent - TestDynamoDB_AdminDeleteItem_MissingTable — same on delete - TestDynamoDB_AdminGetItem_MalformedKey — missing hash key surfaces ErrAdminDynamoValidation, not the internal chain go test -race ./adapter/... passes. golangci-lint --config=.golangci.yaml run ./adapter/... clean.
|
@claude review r1 review fixes pushed (commit 295e957): Codex P1 (read-only contract): AdminScanTable now pre-loads the schema via loadTableSchemaAt and refuses with ErrAdminDynamoValidation when needsLegacyKeyMigration() is true, before scanItems gets a chance to trigger ensureLegacyTableMigration. The error message points operators at the SigV4 surface for the one-time migration. Codex P2 (opaque malformed-key error): AdminGetItem now calls schema.itemKeyFromAttributes(internalKey) up-front and wraps any error as ErrAdminDynamoValidation, so the HTTP bridge can map it to 400 instead of the plain errors.New chain returned by readLogicalItemAt. To stay under cyclop max=10, the read-path preamble (role + leader + name/key validation + schema load + legacy refusal) is factored into adminLoadReadableSchema, reused by AdminGetItem. Claude bot Low (Items: null): adminItemsFromInternal now returns an empty slice via make([]AdminItem, 0) so JSON marshals as 'Items: []' rather than 'Items: null'. Tests added:
go test -race ./adapter/... passes; golangci-lint clean (0 issues). |
|
Claude finished @bootjp's task in 3m 23s —— View job Code Review (r2) — adapter/dynamodb: item-level admin RPCs (Phase 2a)
All r1 fixes are correctly implemented. Here's the pass-by-pass verdict: Codex P1 — Legacy migration gate ✅
One subtle point, no action required: the legacy pre-check uses the Codex P2 / Claude r0 Finding 1 — Malformed key → ErrAdminDynamoValidation ✅
The Confirmed the legacy-gate consistency: if Claude r0 Finding 2 —
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
adapter/dynamodb_admin_items.go (2)
227-228: 💤 Low valueMissing sentinel error documentation.
AdminDeleteItemlacks the sentinel error documentation thatAdminPutItemand other methods have. For consistency, consider adding the same sentinel list.Suggested documentation addition
// AdminDeleteItem removes one item by primary key. Write role // required. +// +// Sentinels: +// - ErrAdminForbidden — principal lacks write role +// - ErrAdminNotLeader — follower +// - ErrAdminDynamoNotFound — table absent +// - ErrAdminDynamoValidation — empty / malformed input func (d *DynamoDBServer) AdminDeleteItem(...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/dynamodb_admin_items.go` around lines 227 - 228, AdminDeleteItem is missing the sentinel error documentation that other admin methods (e.g., AdminPutItem) include; update the comment block above the AdminDeleteItem function to list the same sentinel errors (such as ErrNotFound, ErrInvalidKey, ErrUnauthorized, etc.) used across the adapter, matching wording and formatting used in AdminPutItem so documentation is consistent and discoverable for callers and tools that parse sentinel error docs.
43-56: 💤 Low valueJSON tags missing on
AdminScanResultandAdminScanOptionsfields.
AdminItemandAdminItemKeyuse lowercase snake_case JSON tags (attributes,partition_key,sort_key), butAdminScanResultandAdminScanOptionshave no tags. If these are serialized to JSON for wire responses, the field names will be PascalCase (Items,LastEvaluatedKey,Limit,ExclusiveStart) which may not match client expectations.Consider adding consistent JSON tags if these types appear on the wire:
Suggested tags for consistency
type AdminScanResult struct { - Items []AdminItem - LastEvaluatedKey map[string]AdminAttributeValue + Items []AdminItem `json:"items"` + LastEvaluatedKey map[string]AdminAttributeValue `json:"last_evaluated_key,omitempty"` } ... type AdminScanOptions struct { - Limit int - ExclusiveStart map[string]AdminAttributeValue + Limit int `json:"limit,omitempty"` + ExclusiveStart map[string]AdminAttributeValue `json:"exclusive_start,omitempty"` }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/dynamodb_admin_items.go` around lines 43 - 56, AdminScanResult and AdminScanOptions are missing JSON tags so their serialized field names will be exported PascalCase; update the struct field tags to match the existing snake_case style used by AdminItem/AdminItemKey (e.g. Items -> "items", LastEvaluatedKey -> "last_evaluated_key", Limit -> "limit", ExclusiveStart -> "exclusive_start") by adding json:`...` tags to the AdminScanResult and AdminScanOptions field declarations so wire serialization is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@adapter/dynamodb_admin_items.go`:
- Around line 227-228: AdminDeleteItem is missing the sentinel error
documentation that other admin methods (e.g., AdminPutItem) include; update the
comment block above the AdminDeleteItem function to list the same sentinel
errors (such as ErrNotFound, ErrInvalidKey, ErrUnauthorized, etc.) used across
the adapter, matching wording and formatting used in AdminPutItem so
documentation is consistent and discoverable for callers and tools that parse
sentinel error docs.
- Around line 43-56: AdminScanResult and AdminScanOptions are missing JSON tags
so their serialized field names will be exported PascalCase; update the struct
field tags to match the existing snake_case style used by AdminItem/AdminItemKey
(e.g. Items -> "items", LastEvaluatedKey -> "last_evaluated_key", Limit ->
"limit", ExclusiveStart -> "exclusive_start") by adding json:`...` tags to the
AdminScanResult and AdminScanOptions field declarations so wire serialization is
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c823301c-eb1d-48cf-a61e-e7900a717c2d
📒 Files selected for processing (2)
adapter/dynamodb_admin_items.goadapter/dynamodb_admin_items_test.go
…elds Addresses gemini r1 review on PR #805: ## security-high: depth-cap admin AttributeValue input The internal attributeValue type bounds JSON nesting at maxAttributeValueNestingDepth=32 inside the wire parser. The admin RPCs accept the exported AdminAttributeValue type from (Phase 3) HTTP handlers that JSON-decode untrusted bodies, and Go encoding/json does NOT bound recursion depth - a deeply nested payload would build a deep AdminAttributeValue and then recurse through adminToInternalAttributeValue. Add validateAdminAttributeMapDepth and call it from all four admin entry points (AdminScanTable.ExclusiveStart, AdminGetItem key, AdminPutItem item, AdminDeleteItem key) right after the existing length checks. Over-depth surfaces as ErrAdminDynamoValidation so the Phase 3 bridge maps it to 400. ## medium: deep-copy byte/set fields in conversion AdminAttributeValue is the exported boundary type; mutating admin.B / admin.SS / admin.NS / admin.BS after conversion must not bleed into the internal copy (or vice-versa). The strings inside SS/NS are immutable so slices.Clone is enough; B and the inner buffers of BS are []byte so bytes.Clone each one. Reuses the existing cloneBinarySet from dynamodb_types.go. ## Caller audit (semantic-change rule) grep confirms internalToAdminAttribute* / adminToInternalAttribute* have no callers outside this file. The depth check returns a new ErrAdminDynamoValidation path on inputs deeper than 32; the admin HTTP bridge (Phase 3) already needs to map this sentinel to 400 from the other entry-point validations, so no caller-side work is required. ## Tests - TestDynamoDB_AdminPutItem_RejectsDeepNesting - boundary payload (cap+1 nesting) is rejected with ErrAdminDynamoValidation - TestAdminAttributeValue_DeepCopyIsolation - mutating the source after conversion does not bleed into the converted value, in both directions go test -race ./adapter/... passes. golangci-lint clean.
|
@claude review Round 2 (commit gemini security-high (recursion depth): Added gemini medium (shallow byte/set copies): Tests added:
Caller audit:
Skipping the gemini medium "route loadTableSchemaAt errors through translateDynamoAdminError" — the translator only maps |
|
Claude finished @bootjp's task in 2m 42s —— View job Code Review (r3) — adapter/dynamodb: item-level admin RPCs (Phase 2a)
All r2 fixes are correct. LGTM — ready to merge after CI green. Depth guard (
|
…e-bot r3 nits) Two non-blocking polish items from claude-bot r3 on PR #805. Both are pre-Phase-3 cleanup — neither blocks the current PR, but the JSON-tag fix is a real Phase 3 dependency so worth landing now. ## JSON tags on AdminScanResult / AdminScanOptions AdminItem and AdminItemKey use snake_case json tags ("attributes", "partition_key"). AdminScanResult and AdminScanOptions were missing tags entirely, so the encoding/json default would render them as PascalCase ("Items", "Limit") on the Phase 3 HTTP wire — inconsistent with the rest of the admin API surface. Add snake_case tags matching the existing style: - AdminScanResult.Items -> "items" - AdminScanResult.LastEvaluatedKey -> "last_evaluated_key,omitempty" - AdminScanOptions.Limit -> "limit,omitempty" - AdminScanOptions.ExclusiveStart -> "exclusive_start,omitempty" ## AdminDeleteItem sentinel doc AdminPutItem documents its four sentinels; AdminDeleteItem only said "Write role required." Add the same four-sentinel block so the API surface is documented uniformly. ## Verification - go test -race ./adapter/... passes (no behavior change; the type change is wire-shape only). - golangci-lint clean. - No caller audit needed — the JSON tags only affect Phase 3 marshalers that do not yet exist. Existing test assertions read fields by Go name, not JSON name.
|
@claude review Round 3 (commit JSON tags on
No behavior change. If r3 is clean I think Phase 2a is ready to merge — open to a final pass. |
|
Claude finished @bootjp's task in 28s —— View job Code Review (r4) — adapter/dynamodb: item-level admin RPCs (Phase 2a)
Both r3 nits are correctly applied. JSON tags on
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ac83ae9f5
ℹ️ 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".
| S: v.S, N: v.N, B: bytes.Clone(v.B), BOOL: v.BOOL, NULL: v.NULL, | ||
| SS: slices.Clone(v.SS), NS: slices.Clone(v.NS), BS: cloneBinarySet(v.BS), | ||
| } | ||
| if len(v.L) > 0 { |
There was a problem hiding this comment.
Treating containers as present only when len(...) > 0 drops valid DynamoDB values for empty lists/maps ({"L": []} / {"M": {}}). In internalToAdminAttributeValue this causes AdminGet/AdminScan to lose those attributes, and in the reverse converter it turns admin writes of empty containers into zero-value attributes that later fail kind detection (because attributeValue type checks use nil/non-nil, not length). This breaks round-tripping and can surface as unexpected write failures for otherwise valid items.
Useful? React with 👍 / 👎.
…Codex r3 P1) Codex r3 on PR #805 caught a round-trip bug in the AdminAttributeValue <-> attributeValue recursive converters: both branches gated descent on `len(v.L) > 0` / `len(v.M) > 0`, silently dropping the empty-but-present case ({"L": []} / {"M": {}}) — a legitimate AWS DynamoDB attribute value. The damage flows in two directions: - internalToAdminAttributeValue called from AdminGetItem / AdminScanTable would surface the stored empty container as nil, losing the attribute on read. - adminToInternalAttributeValue called from AdminPutItem on an admin-supplied empty container would lose the kind tag — attributeValue.hasListType / hasMapType use `!= nil` for presence detection (dynamodb_types.go:101-107), not length — so the write would persist a zero-kind value that later fails kind checks downstream. Fix: replace both gates with `!= nil`. The internal kind-detection helpers already use `!= nil`; the converters now match, so empty containers round-trip faithfully. ## Caller audit (semantic-change rule) grep confirms internalToAdminAttributeValue / adminToInternalAttributeValue have no callers outside this file. The behavior change is "empty L/M now preserved" — this was a correctness gap, not a contract; callers that previously saw empty containers as nil were already getting a buggy view. ## Regression test TestAdminAttributeValue_EmptyContainersRoundTrip asserts both directions: admin -> internal -> admin must preserve len-0 L/M as non-nil, and the intermediate internal attributeValue must satisfy hasListType() / hasMapType() so OCC and other downstream readers see the empty container, not a missing field. Verified the regression test FAILS against a hand-rolled revert to the pre-fix `len > 0` gate: "empty L must round-trip as non-nil (got nil = silently dropped)" ## Verification - go test -race ./adapter/... passes (all existing tests + 1 new). - golangci-lint clean.
|
@claude review Round 4 (commit Preserve empty L/M attribute values in conversion: The recursive converters gated descent on Fixed both directions: gates are now Caller audit: the recursive converters have no callers outside this file. The behavior change is purely "empty L/M now preserved" — this was a correctness gap, not a documented contract; pre-fix callers were already getting a buggy view. Regression test:
|
|
Claude finished @bootjp's task in 2m 43s —— View job Code Review (r4) — adapter/dynamodb: item-level admin RPCs (Phase 2a)
Codex r3 P1 —
|
… empty L/M on wire PR #805 round 5 — claude-bot r4 low: encoding/json's `omitempty` tag drops slices/maps of len 0 regardless of nil-ness, so the default struct marshaler would have silently dropped {"L": []} and {"M": {}} on the Phase 3 HTTP wire — even with the r4 fix preserving the Go-level non-nil-empty distinction. The simpler fix claude-bot suggested (remove omitempty from L/M only) has the wrong wire shape: every non-list / non-map admin attribute would serialize "L": null and "M": null alongside the populated field, violating the AWS-wire "exactly one field set per AttributeValue" invariant. Better fix: give AdminAttributeValue a custom MarshalJSON / UnmarshalJSON that delegates to the internal attributeValue's existing AWS-wire codec via the conversion functions: - MarshalJSON: adminToInternalAttributeValue → internal.MarshalJSON. Detects the exactly-one populated field, picks the matching AWS kind tag, and renders {"L": []} for empty-but-present. - UnmarshalJSON: internal.UnmarshalJSON (which enforces the 32-deep nesting cap) → internalToAdminAttributeValue. This inherits two properties from the internal codec: 1. Empty containers preserved: {"L": []} / {"M": {}} round-trip faithfully through both directions. 2. Fail-closed multi-field detection: a zero-field or multi-field AdminAttributeValue surfaces as a marshal error rather than producing an invalid wire payload an SDK would silently misinterpret. This is a NEW error path Phase 3 HTTP handlers must surface as 500 — but Phase 3 callers were going to produce single-field values anyway (it's the AWS contract). ## Caller audit (semantic-change rule) AdminAttributeValue is exported but Phase 2a has no JSON consumers — only in-memory conversions. The Phase 3 admin HTTP bridge is the only future caller; it must: - Surface json.Marshal errors as 500 (standard HTTP handling). - Construct AdminAttributeValue with exactly one field set (the AWS-wire contract). Both are correctness properties the bridge would need anyway. ## Regression tests - TestAdminAttributeValue_EmptyContainersJSONRoundTrip: asserts {"L": []} marshals/unmarshals as non-nil empty; same for {"M": {}}. - TestAdminAttributeValue_JSONMarshalRejectsMultiField: asserts zero-field and multi-field AdminAttributeValue both fail to marshal (fail-closed invariant). ## Verification - go test -race ./adapter/... passes. - golangci-lint clean.
|
@claude review Round 5 (commit The simpler "drop omitempty from L/M" fix you suggested would put This inherits two properties from the internal codec:
Caller audit: Phase 2a has no JSON consumers (all in-memory). The Phase 3 admin HTTP bridge is the only future caller and must (a) surface Regression tests:
The depth check inherits from the internal
|
|
Claude finished @bootjp's task in 2m 47s —— View job Code Review (r5) — adapter/dynamodb: item-level admin RPCs (Phase 2a)
The r4 low finding is correctly and cleanly resolved. The custom codec approach is the right call — here's why it works end-to-end. Approach justification ✅The PR author's reasoning is correct. Dropping Custom
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51413ce93e
ℹ️ 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 err := validateAdminAttributeMapDepth(item.Attributes); err != nil { | ||
| return err | ||
| } | ||
| in := putItemInput{ |
There was a problem hiding this comment.
AdminPutItem only checks nesting depth before converting AdminAttributeValue into internal values, so structurally invalid values (for example a non-key attribute with zero/multiple kind fields, or NULL:false constructed by a Go caller) can reach putItemWithRetry. Those cases fail later in storage encoding as non-dynamoErrValidation errors, so translateDynamoAdminError falls through to a generic internal error instead of ErrAdminDynamoValidation, violating this method’s documented malformed-input contract and surfacing avoidable 500s.
Useful? React with 👍 / 👎.
…x r5 P2)
The custom MarshalJSON / UnmarshalJSON landed in r5 protects the
Phase 3 HTTP boundary, but Go-level callers constructing
AdminAttributeValue programmatically can still pass zero-field
or multi-field values. Without this check those bypass the JSON
codec entirely, reach putItemWithRetry / readLogicalItemAt, and
the storage codec surfaces a non-dynamoErrValidation error that
translateDynamoAdminError can't recognise — rendering as a 500
instead of the documented 400 contract.
Fix: validateAdminAttributeMapKinds walks the admin-supplied
attribute tree and returns ErrAdminDynamoValidation if any leaf
AdminAttributeValue has a populated-kind-field count != 1.
Recurses into L and M for the container cases. Called from all
four admin entry points right after the existing depth check:
- AdminScanTable.ExclusiveStart
- AdminGetItem key
- AdminPutItem item.Attributes (the main path the bug
description targets)
- AdminDeleteItem key
countAdminKindFields uses a slice-of-bools + loop pattern
(cyclomatic complexity 3) instead of the obvious 10-if chain
that would trip the cyclop=10 cap.
## Caller audit (semantic-change rule)
This adds a new ErrAdminDynamoValidation path at all four admin
entry points. The change is fail-closed (previously: 500 on
malformed kind shape; now: 400). No external callers exist yet
(Phase 2a) — only the test file. Future Phase 3 HTTP handlers
already need to surface ErrAdminDynamoValidation from the
existing depth check, so the contract is uniform.
## Regression tests
- TestDynamoDB_AdminPutItem_RejectsZeroFieldAttribute: a
zero-kind attribute on the non-key column surfaces as
ErrAdminDynamoValidation, not the storage codec's 500-shape
error.
- TestDynamoDB_AdminPutItem_RejectsMultiFieldAttribute: same
for a two-kind attribute (S + N).
## Verification
- go test -race ./adapter/... passes (all existing + 2 new).
- golangci-lint clean (countAdminKindFields uses the slice
pattern to stay under cyclop=10).
|
@claude review Round 6 (commit Validate attribute kind at admin entry points: The r5 custom Added Caller audit (semantic-change rule): Adds a new Regression tests:
Note on CI: The prior r5 run failed on
|
|
Claude finished @bootjp's task in 1m 20s —— View job Code Review (r6) — adapter/dynamodb: item-level admin RPCs (Phase 2a)
Codex r5 P2 —
|
| Entry point | Depth check line | Kind check line | Order |
|---|---|---|---|
AdminScanTable |
157 | 160 | depth → kind ✅ |
AdminGetItem |
192 | 195 | depth → kind ✅ |
AdminPutItem |
268 | 271 | depth → kind ✅ |
AdminDeleteItem |
305 | 308 | depth → kind ✅ |
Regression tests ✅
TestDynamoDB_AdminPutItem_RejectsZeroFieldAttribute (line 522): Constructs a map containing AdminAttributeValue{} (no kind field set → countAdminKindFields = 0). Asserts errors.Is(err, ErrAdminDynamoValidation). Correct — this is the zero-field case bypassing the JSON codec.
TestDynamoDB_AdminPutItem_RejectsMultiFieldAttribute (line 540): Constructs a map containing {S: &s, N: &n} (two kind fields set → countAdminKindFields = 2). Asserts errors.Is(err, ErrAdminDynamoValidation). Correct — this is the multi-field case.
Both tests use createTableForItemTests to create the table first (correct — the table-not-found check fires before the kind check in AdminPutItem, so the table must exist). ✅
Notes on skipped AdminGetItem / AdminScanTable kind-check tests
The tests cover AdminPutItem for both invalid-kind cases. For completeness, AdminGetItem / AdminDeleteItem / AdminScanTable would benefit from analogous tests (malformed key attribute hits validateAdminAttributeMapKinds before itemKeyFromAttributes), but this is low priority — the guard is the same code path for all four entry points, and the failure mode is the same sentinel.
TTL flake note ✅
The prior CI failure (TestRedis_ExpiredKey_BecomesInvisible with PX=200ms) is a known timing-sensitive flake under -race scheduling on the 3-node Raft setup. Unrelated to anything in adapter/dynamodb_admin_items.go. Nothing to action here.
LGTM. Phase 2a is ready to merge after CI green. No outstanding findings.
All findings from r0–r6 (Codex P1/P2, Claude r0 Finding 1/2, Gemini security-high/medium, coderabbit nitpicks, Codex r3 P1, Codex r5 P2) have been resolved. The implementation is correct, complete, and well-tested.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
adapter/dynamodb_admin_items.go (1)
486-501: 💤 Low valueImplicit ordering dependency with depth validation.
checkAdminAttributeKindrecurses through L/M without its own depth bound, relying onvalidateAdminAttributeMapDepthbeing called first. All current call sites follow the correct order, but this coupling could be documented or enforced by combining both checks into a single validation pass.This is a minor observation; the current approach works correctly for all admin entry points.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/dynamodb_admin_items.go` around lines 486 - 501, The recursive function checkAdminAttributeKind relies on an external depth check (validateAdminAttributeMapDepth), creating an implicit ordering dependency; update checkAdminAttributeKind to enforce the map/list depth limit itself (or accept a depth parameter) so it no longer depends on callers to call validateAdminAttributeMapDepth first—specifically, modify checkAdminAttributeKind (which already calls countAdminKindFields and recurses over v.L and v.M) to track current depth and abort with ErrAdminDynamoValidation when exceeding the allowed depth (or call validateAdminAttributeMapDepth at the start), ensuring the depth check and kind-field validation occur in the same pass.adapter/dynamodb_admin_items_test.go (1)
203-227: ⚡ Quick winPrefer table-driven tests for repeated gate/error scenarios.
These tests are structurally repetitive and are good candidates for a single table-driven case set (operation, principal, expected sentinel). This will reduce duplication and make adding new gate cases cheaper.
As per coding guidelines, "
**/*_test.go: Unit tests must be co-located with packages in*_test.gofiles; prefer table-driven test cases."Also applies to: 229-240, 301-327
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/dynamodb_admin_items_test.go` around lines 203 - 227, Combine the repetitive TestDynamoDB_AdminPutItem_ReadOnlyForbidden and TestDynamoDB_AdminDeleteItem_ReadOnlyForbidden (and the other similar blocks at 229-240 and 301-327) into a single table-driven test that iterates over cases describing the operation (e.g., "Put" -> call srv.AdminPutItem, "Delete" -> call srv.AdminDeleteItem), the table name to create via createTableForItemTests, the principal (readOnlyAdminPrincipal) and the expected sentinel error (ErrAdminForbidden); inside the loop createNode/shutdown per subtest, call the appropriate method for each case, and assert errors.Is(err, ErrAdminForbidden) so you remove duplication while preserving the same setup (srv, createTableForItemTests, readOnlyAdminPrincipal) and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@adapter/dynamodb_admin_items_test.go`:
- Around line 203-227: Combine the repetitive
TestDynamoDB_AdminPutItem_ReadOnlyForbidden and
TestDynamoDB_AdminDeleteItem_ReadOnlyForbidden (and the other similar blocks at
229-240 and 301-327) into a single table-driven test that iterates over cases
describing the operation (e.g., "Put" -> call srv.AdminPutItem, "Delete" -> call
srv.AdminDeleteItem), the table name to create via createTableForItemTests, the
principal (readOnlyAdminPrincipal) and the expected sentinel error
(ErrAdminForbidden); inside the loop createNode/shutdown per subtest, call the
appropriate method for each case, and assert errors.Is(err, ErrAdminForbidden)
so you remove duplication while preserving the same setup (srv,
createTableForItemTests, readOnlyAdminPrincipal) and assertions.
In `@adapter/dynamodb_admin_items.go`:
- Around line 486-501: The recursive function checkAdminAttributeKind relies on
an external depth check (validateAdminAttributeMapDepth), creating an implicit
ordering dependency; update checkAdminAttributeKind to enforce the map/list
depth limit itself (or accept a depth parameter) so it no longer depends on
callers to call validateAdminAttributeMapDepth first—specifically, modify
checkAdminAttributeKind (which already calls countAdminKindFields and recurses
over v.L and v.M) to track current depth and abort with ErrAdminDynamoValidation
when exceeding the allowed depth (or call validateAdminAttributeMapDepth at the
start), ensuring the depth check and kind-field validation occur in the same
pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b75d999-21c0-47ec-9d24-14d72dfdf049
📒 Files selected for processing (2)
adapter/dynamodb_admin_items.goadapter/dynamodb_admin_items_test.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e446c1e9bd
ℹ️ 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 _, err := schema.itemKeyFromAttributes(internalKey); err != nil { | ||
| return nil, false, errors.Wrap(ErrAdminDynamoValidation, err.Error()) | ||
| } | ||
| current, found, err := d.readLogicalItemAt(ctx, schema, internalKey, readTS) |
There was a problem hiding this comment.
After adminLoadReadableSchema returns readTS, AdminGetItem immediately reads from storage with that timestamp but never pins it via d.pinReadTS. In environments with concurrent writes/compaction, minRetainedTS can advance between schema load and item read, so readLogicalItemAt may fail with compacted-snapshot errors for otherwise valid requests; the SigV4 getItem path explicitly pins for this exact race window.
Useful? React with 👍 / 👎.
| if countAdminKindFields(v) != 1 { | ||
| return errors.Wrap(ErrAdminDynamoValidation, "attribute value must have exactly one kind field set") | ||
| } |
There was a problem hiding this comment.
checkAdminAttributeKind only checks that exactly one kind field is set, so AdminAttributeValue{NULL:&false} passes validation even though it is malformed in Dynamo wire semantics. Fresh evidence in this patch is that the new validator gate still allows this, and adminToInternalAttributeValue forwards it to attributeValue.MarshalJSON, which always serializes NULL as true, silently rewriting caller input instead of returning ErrAdminDynamoValidation.
Useful? React with 👍 / 👎.
…r6 P1+P2)
## P1: pin AdminGetItem readTS to close compaction race
AdminGetItem read the schema and the item at the same readTS but
never pinned it. Between adminLoadReadableSchema's
loadTableSchemaAt and readLogicalItemAt, concurrent MVCC GC can
advance minRetainedTS past readTS, surfacing compacted-snapshot
errors for otherwise valid requests. The SigV4 getItem path
already pins for this exact window (dynamodb.go:1386).
Fix: extend adminLoadReadableSchema's return signature with the
ActiveTimestampToken acquired right after readTS = nextTxnReadTS().
On success the caller defers readPin.Release(); on every error
path inside the helper the pin is released before returning so
the caller never has to handle release on the error path.
AdminScanTable does not need the same treatment: it uses its
readTS only for one Pebble Get (the legacy-migration schema
load); the data scan that follows runs through d.scanItems,
which generates its own readTS and pins it internally
(dynamodb.go:2079).
AdminPutItem / AdminDeleteItem don't read at a stable readTS —
they go straight to putItemWithRetry / deleteItemWithRetry,
which own their own write-path timestamp lifecycle.
## P2: reject NULL=false in admin attribute validation
The AWS-wire contract for NULL is "boolean kind tag with value
true". A caller passing AdminAttributeValue{NULL: &false} passes
the count==1 check (NULL is the one populated field) but is
malformed — the internal MarshalJSON always serializes NULL as
true (dynamodb_types.go:67), silently rewriting the caller's
input and masking the bug.
Fix: checkAdminAttributeKind now additionally rejects
v.NULL != nil && !*v.NULL with ErrAdminDynamoValidation.
## Caller audit (semantic-change rule)
- adminLoadReadableSchema's signature changed: now returns
*kv.ActiveTimestampToken too. Only caller is AdminGetItem in
this same file. Updated.
- checkAdminAttributeKind has a new fail-closed path: NULL=false
was previously accepted (and silently rewritten to NULL=true on
the wire). Now returns ErrAdminDynamoValidation. The same
sentinel Phase 3 HTTP handlers already need for the depth /
multi-kind checks; no new contract surface.
## Regression test
TestDynamoDB_AdminPutItem_RejectsNullFalse:
- NULL=&false → ErrAdminDynamoValidation
- NULL=&true → accepted (sanity)
The P1 fix's race window is timing-sensitive and not testable
without injecting GC; the test that the helper signature change
preserves correctness is implicitly covered by all existing
AdminGetItem tests passing.
## Verification
- go test -race ./adapter/... passes (all existing + 1 new).
- golangci-lint clean.
|
@claude review Round 7 (commit P1 — Pin AdminGetItem readTS: Scope: Only AdminGetItem needs the pin. AdminScanTable's readTS is one-shot for the legacy-migration check; P2 — Reject NULL=false: Caller audit:
Regression test:
|
|
Claude finished @bootjp's task in 2m 0s —— View job Code Review (r7) — adapter/dynamodb: item-level admin RPCs (Phase 2a)
Both r6 fixes are correctly implemented. Here's the pass-by-pass verdict. P1 —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d0f24e994
ℹ️ 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".
| func (a AdminAttributeValue) MarshalJSON() ([]byte, error) { | ||
| internal := adminToInternalAttributeValue(a) | ||
| return internal.MarshalJSON() |
There was a problem hiding this comment.
AdminAttributeValue.MarshalJSON currently forwards directly to internal.MarshalJSON without validating NULL, so json.Marshal(AdminAttributeValue{NULL:&false}) succeeds and emits {"NULL":true} instead of failing. This silently rewrites caller input, which violates the fail-closed boundary contract documented for this type and can hide malformed admin payloads from Go callers (including future admin bridge code) that expect invalid wire values to be rejected.
Useful? React with 👍 / 👎.
…N (Codex r7 P2)
r7 added NULL=false rejection inside the four admin RPC entry
points via validateAdminAttributeMapKinds. But a direct
json.Marshal(AdminAttributeValue{NULL:&false}) — including from
future admin response builders or Phase 3 handler code that
constructs the value programmatically — would still slip past:
AdminAttributeValue.MarshalJSON delegated straight to
internal.MarshalJSON, which detects the NULL kind by nil-ness
of the pointer (not its bool value) and always emits true,
silently rewriting caller input.
Fix: MarshalJSON now calls checkAdminAttributeDepth followed by
checkAdminAttributeKind before forwarding. Depth comes first to
avoid a stack-overflow risk on a pathological deeply-nested
Go-constructed value (the four RPC entry points already validate
depth before reaching this codec; a direct json.Marshal call
does not).
This also fold-fixes the implicit-ordering coupling coderabbit
r7 flagged on checkAdminAttributeKind: MarshalJSON now satisfies
the depth precondition itself.
## Caller audit (semantic-change rule)
AdminAttributeValue.MarshalJSON's new fail-closed paths:
- depth > 32 → ErrAdminDynamoValidation (was: stack overflow)
- count != 1 → ErrAdminDynamoValidation (was: "invalid attribute
value" from internal.MarshalJSON — different shape)
- NULL=&false → ErrAdminDynamoValidation (was: silent rewrite
to {"NULL": true})
Phase 3 HTTP handlers will see json.Marshal returning a wrapped
ErrAdminDynamoValidation; they already need to map that sentinel
to 400 from the depth / kind checks at the four RPC entry points,
so the contract is uniform.
The existing TestAdminAttributeValue_JSONMarshalRejectsMultiField
asserts `require.Error` (any error) — the change from the
internal "invalid attribute value" error to the wrapped
ErrAdminDynamoValidation passes that assertion. Behavior is
strictly stronger.
## Regression test
TestAdminAttributeValue_JSONMarshalRejectsMultiField extended:
- NULL=&false at the marshal boundary → ErrAdminDynamoValidation
(previously would have silently rewritten to {"NULL": true})
- NULL=&true → marshals as {"NULL": true} (sanity, unchanged)
## Verification
- go test -race ./adapter/... passes.
- golangci-lint clean.
|
@claude review Round 8 (commit Reject NULL=false in MarshalJSON: r7 added NULL=false rejection at the four admin RPC entry points, but a direct
Bonus: this also fold-fixes the implicit-ordering coupling coderabbit r7 flagged on Caller audit: Three new fail-closed paths in
Phase 3 HTTP handlers see The existing Regression test: extended
|
|
Claude finished @bootjp's task in 1m 59s —— View job Code Review (r8) — adapter/dynamodb: item-level admin RPCs (Phase 2a)
All r8 fixes are correctly implemented. Codex r7 P2 —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a065aed45e
ℹ️ 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".
| ExclusiveStartKey: adminToInternalAttributeMap(opts.ExclusiveStart), | ||
| Limit: &limit, | ||
| } | ||
| out, err := d.scanItems(ctx, scanInput) |
There was a problem hiding this comment.
AdminScanTable checks schema.needsLegacyKeyMigration() before calling d.scanItems, but that check is not synchronized with scanItems' internal prepareReadSchema -> ensureLegacyTableMigration path. If a table enters migration between these two calls, a read-only admin scan can still execute migration writes, violating the read-only contract this method is trying to enforce. This is reproducible under concurrent migration start/progress and admin polling, because the final scan path still contains write-capable migration logic.
Useful? React with 👍 / 👎.
…t run PR #805 round 9 — addresses Codex r8 P2. AdminScanTable's entry-point check inspects schema.needsLegacyKeyMigration() at one readTS and then calls d.scanItems, which internally calls prepareReadSchema → ensureLegacyTableMigration at a NEW readTS. A migration that starts between the two reads slips past the entry-point check and reaches the write-path migration trigger, violating the read-only contract AdminScanTable is trying to enforce. Fix: introduce an unexported context-value tag adminReadOnlyContextKey set by withAdminReadOnlyContext. AdminScanTable wraps the context before calling scanItems so the tag propagates down to ensureLegacyTableMigrationLocked, where a new early-exit refuses to migrate and returns ErrAdminDynamoValidation instead. The entry-point needsLegacyKeyMigration check stays as the happy-path fast rejection; the context tag is the belt-and-braces close on the TOCTOU window. ## Scope - AdminScanTable: tags ctx → safe. - AdminGetItem: does NOT call ensureLegacyTableMigration in its read path (uses adminLoadReadableSchema + readLogicalItemAt which are pure reads). No tag needed today; the entry-point legacy check remains the primary refusal. - AdminPutItem / AdminDeleteItem: write-role admins; migration triggers from their underlying SigV4-shared write paths are legitimate and expected. ## Caller audit (semantic-change rule) ensureLegacyTableMigrationLocked gains a new ErrAdminDynamoValidation return path when adminReadOnlyContextKey is set on ctx. Only caller setting that key is AdminScanTable (this file). All existing callers (SigV4 getItem / queryItems / scanItems / transactGetItems / transactWriteItems / executeStatement / putItem / updateItem / deleteItem) never set the key, so their migration trigger behaviour is unchanged. ## Regression test TestDynamoDB_EnsureLegacyTableMigration_RefusesAdminReadOnlyContext sets up a legacy-format table, confirms it genuinely needs migration, then calls ensureLegacyTableMigration with the admin read-only tag and asserts: - error matches ErrAdminDynamoValidation - the schema is STILL needs-migration afterward (no writes happened — control on the no-write contract) ## Verification - go test -race ./adapter/... passes (existing tests + 1 new). - golangci-lint clean.
|
@claude review Round 9 (commit Close TOCTOU on AdminScanTable's read-only contract: Entry-point Fix: unexported context-value tag Scope:
Caller audit: Regression test:
|
|
Claude finished @bootjp's task in 3m 8s —— View job Code Review (r9) — adapter/dynamodb: item-level admin RPCs (Phase 2a)
All r9 fixes are correctly implemented. LGTM — ready to merge after CI green. Context key pattern ✅
Placement in
|
## Summary **Phase 2b** of `docs/design/2026_05_22_implemented_admin_data_browser.md` — the S3 object-level admin RPCs that mirror the Phase 2a (PR #805) DynamoDB item-level surface. Four methods on `*adapter.S3Server`, each delivered as its own commit: | Slice | Method | Role | Notes | |---|---|---|---| | 1 | `AdminDeleteObject(ctx, principal, bucket, key) error` | write | Idempotent (missing object → nil, matching AWS S3 + SigV4 path) | | 2 | `AdminPutObject(ctx, principal, bucket, key, body io.Reader, contentType string) error` | write | Streaming upload, 100 MiB cap (`ErrAdminUploadTooLarge`); chunk-pipeline matches SigV4 putObject | | 3 | `AdminGetObject(ctx, principal, bucket, key) (io.ReadCloser, AdminObject, error)` | read | Lazy chunk-fetching reader bounded to ~1 chunk; caller MUST Close to release the read-pin | | 4 | `AdminListObjects(ctx, principal, bucket, opts) (AdminObjectListing, error)` | read | Prefix + delimiter; reuses SigV4 `s3ContinuationToken` cursor shape; `MaxKeys [1, 1000]` default 100 | Total: ~1100 LOC code + ~800 LOC tests across 4 commits, no changes to existing code paths. ## Authorization | Operation | Gate | |---|---| | Get / List | `principal.Role.canRead()` (RoleReadOnly or RoleFull) | | Put / Delete | `principal.Role.canWrite()` (RoleFull only) | | All | `isVerifiedS3Leader(ctx)` up-front check + `validateS3BucketName` for the bucket arg | ## Sentinels | Sentinel | Maps to | |---|---| | `ErrAdminForbidden` (reused from dynamodb_admin.go) | 403 | | `ErrAdminNotLeader` (reused) | 503 | | `ErrAdminBucketNotFound` (reused from s3_admin.go) | 404 | | `ErrAdminInvalidBucketName` (reused) | 400 | | `ErrAdminObjectNotFound` (new) | 404 (only on AdminGetObject; AdminDeleteObject is idempotent) | | `ErrAdminInvalidObjectKey` (new) | 400 | | `ErrAdminUploadTooLarge` (new) | 413 | ## Caller audit (semantic-change rule) No existing signatures changed. The SigV4 paths (`getObject`, `putObject`, `deleteObject`, `listObjectsV2`) and their helpers (`loadBucketMetaAt`, `loadObjectManifestAt`, `cleanupManifestBlobsAsync`, `encode/decodeS3ContinuationToken`, `s3keys.*`) are unchanged. The admin RPCs are gated wrappers that produce the same Raft `OperationGroup` shapes as the SigV4 path. Two new private helpers: `adminDeleteObjectTxn` (slice 1), `adminPutObjectStream` (slice 2), `adminLoadObjectForRead` (slice 3). All file-local. The chunk-pipeline `//nolint:cyclop,gocognit,nestif` on `adminPutObjectStream` and the scan-loop nolint on `AdminListObjects` mirror the existing precedent on SigV4 `putObject` / `listObjectsV2` (same N-stage sequential flow with per-stage cleanup-on-error). ## Risk Low. The Admin methods are gated wrappers over the same primitives (`coordinator.Dispatch`, `store.ScanAt`, `store.GetAt`, `s3keys.*`) the SigV4 path uses today. No new write codepath, no new on-disk format. ## Self-review (5 passes) 1. **Data loss** — All writes go through the same `OperationGroup` shape as SigV4 (manifest put + bucket-fence put on AdminPutObject; manifest Del on AdminDeleteObject). Cleanup-on-error during chunked upload synchronously deletes orphan blobs before returning to the caller, matching SigV4's `cleanupManifestBlobs` discipline. 2. **Concurrency** — Leader-only via `isVerifiedS3Leader`. Reads pin `readTS` for the duration of the manifest+chunks fetch (released by `ReadCloser.Close`); list-path pin released by `defer`. `retryS3Mutation` wraps `AdminDeleteObject` per the SigV4 precedent. 3. **Performance** — `AdminGetObject` memory is bounded to ~`s3ChunkSize` (1 MiB) regardless of object size. `AdminPutObject` chunks at `s3ChunkBatchOps` per dispatch. `AdminListObjects` paginates at `s3ListPageSize`. 4. **Consistency** — Manifest reads use the same `loadObjectManifestAt` path as SigV4. List cursors are generation-fenced (token mismatch → reject) so a re-created bucket cannot leak stale entries to a continuing walk. 5. **Test coverage** — 28 new unit tests across the four slices: happy paths + role denials + missing-bucket + missing-object + idempotency + chunk-boundary streaming + pagination round-trip + content-type defaulting + Close-after-Close + oversized-body rejection. ## Test plan - [x] `go test -race -count=1 -timeout=180s ./adapter/...` — passes - [x] `golangci-lint --config=.golangci.yaml run ./adapter/...` — clean (the four nolint markers match the SigV4 precedents) - [ ] CI on this PR (Jepsen / build / CodeQL) ## Phase plan - Phase 2a: DynamoDB item RPCs — **shipped** (#805) - Phase 2b: this PR (S3 object RPCs) - Phase 3: HTTP handlers + bridges + integration tests - Phase 4: SPA DynamoDetail Items tab - Phase 5: SPA S3Detail Objects tab + Upload - Phase 6: Doc rename `_proposed_` → `_implemented_`
1 participant
Summary
Phase 2a of
docs/design/2026_05_22_implemented_admin_data_browser.md(PR #801) — DynamoDB item-level admin RPCs:AdminScanTable(ctx, principal, name, opts) (AdminScanResult, error)— read role;Limitclamped to[1, 100]with default25; returnsLastEvaluatedKeyfor cursor pagination (no internal looping on partial pages per §3.1.1).AdminGetItem(ctx, principal, name, key) (*AdminItem, bool, error)— read role; returns(nil, false, nil)on absence so admin handlers can render 404 without sniffing sentinels.AdminPutItem(ctx, principal, name, item) error— write role; create-or-replace.AdminDeleteItem(ctx, principal, name, key) error— write role.AdminAttributeValueis the exported mirror of the internalattributeValue(same AWS-wire JSON shape, recursive forL/M). Conversion functions sit between admin and adapter types.Authorization
principal.Role.canRead()(RoleReadOnly or RoleFull)principal.Role.canWrite()(RoleFull only)isVerifiedDynamoLeaderup-front checkCaller audit (semantic-change rule)
No existing signatures changed. The internal helpers (
scanItems,loadTableSchemaAt,readLogicalItemAt,putItemWithRetry,deleteItemWithRetry) are unchanged; the admin RPCs are pure gated wrappers.Risk
Low. All four admin entrypoints delegate to existing internal helpers that ship in production today; the additions are role gates + sentinel translation.
Self-review (5 passes)
*WithRetryhelper that the SigV4 path uses. No new persistence paths.isVerifiedDynamoLeader. Reads usenextTxnReadTS()for the MVCC snapshot. Cursor pagination uses the internalExclusiveStartKeysemantics (no race because the cursor itself encodes the item key).maxAttributeValueNestingDepth=32constant the wire parser already enforces.readTS(matches the SigV4 GetItem's lease-confirm-then-read pattern). The admin path does NOT do the lease-read step (admin reads are rare, not steady-state), but the gain over the lease-confirmed SigV4 path is bounded by the leader-step-down window — same trade-off the SQS admin peek made.Test plan
go test -race -count=1 -timeout=180s ./adapter/...— 12 new tests + all existing dynamo admin tests passmake lintclean (0 issues)Phase plan
_proposed_→_implemented_Summary by CodeRabbit
New Features
Tests