admin: address PR #669 review (route comment, ValidationError test, MaxBytes nil doc)

Three actionable findings from claude review on 91a48ef:

1) **Medium: stale route-table comment in `buildAPIMux`**
   `internal/admin/server.go`: the comment block listing the routes
   only mentioned the two GET S3 endpoints. Added the three new
   write endpoints (POST /buckets, DELETE /buckets/{name}, PUT
   /buckets/{name}/acl) so a future reader does not have to
   reverse-engineer the routing from `servePerBucket`.

2) **Medium: missing handler-level `*ValidationError` test**
   `s3_handler_test.go`: `writeBucketsError`'s `errors.As(err,
   &verr)` arm was exercised end-to-end via the bridge translation
   but had no direct handler-level coverage. Added
   `TestS3Handler_WriteEndpoints_ValidationErrorReturns400` —
   three sub-cases (create / put_acl / delete) injecting a
   `*ValidationError` directly into the stub source and asserting
   400 + `invalid_request` + the message. Locks the arm down so a
   future refactor that drops the `errors.As` branch fails CI
   immediately.

3) **Minor: `MaxBytesReader(nil, ...)` undocumented**
   `s3_handler.go`: added an explanatory comment at the call site.
   Passing nil is safe because `MaxBytesReader`'s only writer use
   is the connection-close optimisation (a comma-ok type assertion
   that no-ops on nil); the cap itself is enforced regardless. We
   pass nil because the helper takes only `io.Reader` so it remains
   usable from non-HTTP entry points (the upcoming AdminForward
   decoder reuses the same shape). The 413 path still works via
   `errAdminS3BodyTooLarge` matching on `*http.MaxBytesError`.

Findings #3 (failed-write audit log) and #5 (DisallowUnknownFields
error message) were noted as pre-existing patterns shared with the
Dynamo side; tracking them separately rather than diverging on
this slice.

Author: bootjp

internal/admin/s3_handler.go

@@ -437,6 +437,17 @@ func decodeAdminS3JSONBody[T any](body io.Reader) (T, error) {
 	if body == nil {
 		return zero, errors.New("request body is empty")
 	}
+	// nil ResponseWriter is safe here: MaxBytesReader's only use of
+	// the writer is the connection-close optimisation (sets the
+	// requestTooLarger flag on the writer when the cap is hit); the
+	// nil-check inside is a comma-ok type assertion, so passing nil
+	// just skips the optimisation. The cap itself is still enforced
+	// and surfaces as *http.MaxBytesError on the read, which the
+	// errors.As below catches and translates to errAdminS3BodyTooLarge.
+	// We do not have access to the *http.Request's writer at this
+	// layer (the helper takes only an io.Reader so the same code is
+	// reusable from non-HTTP entry points like AdminForward decode).
+	// Claude review on PR #669 flagged the nil as undocumented.
 	limited := http.MaxBytesReader(nil, io.NopCloser(body), adminS3CreateBodyLimit)
 	raw, err := io.ReadAll(limited)
 	if err != nil {

internal/admin/s3_handler_test.go

@@ -648,6 +648,58 @@ func TestS3Handler_DeleteBucket_NotLeaderReturns503(t *testing.T) {
 	require.Equal(t, "1", rec.Header().Get("Retry-After"))
 }
 
+func TestS3Handler_WriteEndpoints_ValidationErrorReturns400(t *testing.T) {
+	// Pin the writeBucketsError *ValidationError arm — exercised
+	// end-to-end via the bridge (adapter ErrAdminInvalid* →
+	// translateAdminBucketsError → &ValidationError{...}) but
+	// previously had no handler-level coverage. Locking the arm
+	// down protects against a future refactor that drops the
+	// errors.As branch and silently downgrades typed validation
+	// failures to 500 (Claude review on PR #669).
+	const msg = "invalid bucket name: uppercase letters not allowed"
+	t.Run("create", func(t *testing.T) {
+		src := &stubBucketsSource{createErr: &ValidationError{Message: msg}}
+		h := newS3HandlerForTest(src)
+		req := httptest.NewRequest(http.MethodPost, pathS3Buckets,
+			strings.NewReader(validCreateBucketBody()))
+		req = withFullPrincipal(req)
+		rec := httptest.NewRecorder()
+		h.ServeHTTP(rec, req)
+		require.Equal(t, http.StatusBadRequest, rec.Code)
+		require.Contains(t, rec.Body.String(), "invalid_request")
+		require.Contains(t, rec.Body.String(), msg)
+	})
+	t.Run("put_acl", func(t *testing.T) {
+		src := &stubBucketsSource{
+			buckets:   map[string]BucketSummary{"orders": {Name: "orders"}},
+			putACLErr: &ValidationError{Message: msg},
+		}
+		h := newS3HandlerForTest(src)
+		req := httptest.NewRequest(http.MethodPut, pathS3Buckets+"/orders/acl",
+			strings.NewReader(`{"acl":"public-read"}`))
+		req = withFullPrincipal(req)
+		rec := httptest.NewRecorder()
+		h.ServeHTTP(rec, req)
+		require.Equal(t, http.StatusBadRequest, rec.Code)
+		require.Contains(t, rec.Body.String(), "invalid_request")
+		require.Contains(t, rec.Body.String(), msg)
+	})
+	t.Run("delete", func(t *testing.T) {
+		src := &stubBucketsSource{
+			buckets:   map[string]BucketSummary{"orders": {Name: "orders"}},
+			deleteErr: &ValidationError{Message: msg},
+		}
+		h := newS3HandlerForTest(src)
+		req := httptest.NewRequest(http.MethodDelete, pathS3Buckets+"/orders", nil)
+		req = withFullPrincipal(req)
+		rec := httptest.NewRecorder()
+		h.ServeHTTP(rec, req)
+		require.Equal(t, http.StatusBadRequest, rec.Code)
+		require.Contains(t, rec.Body.String(), "invalid_request")
+		require.Contains(t, rec.Body.String(), msg)
+	})
+}
+
 func TestS3Handler_WriteEndpoints_RejectMissingPrincipal(t *testing.T) {
 	// Without a session principal in the context, writes must
 	// 401 — SessionAuth normally enforces this; principalForWrite

internal/admin/server.go

@@ -200,7 +200,10 @@ func (s *Server) APIHandler() http.Handler {
 //	GET    /admin/api/v1/dynamo/tables/{name}       (auth required)
 //	DELETE /admin/api/v1/dynamo/tables/{name}       (auth required, full role)
 //	GET    /admin/api/v1/s3/buckets                 (auth required)
+//	POST   /admin/api/v1/s3/buckets                 (auth required, full role)
 //	GET    /admin/api/v1/s3/buckets/{name}          (auth required)
+//	DELETE /admin/api/v1/s3/buckets/{name}          (auth required, full role)
+//	PUT    /admin/api/v1/s3/buckets/{name}/acl      (auth required, full role)
 //
 // Body limit applies uniformly. CSRF and Audit middleware apply to
 // write-capable protected endpoints; login and logout carry their own