bucket labels

[giurgiur99]

Fixes #1354

Changes proposed in this PR:

• Add optional label (human-readable name) to storage buckets — accepted on create, stored, and returned in all bucket reads
• Auto-generate a friendly default name (unique-names-generator) when no label is provided on create

[giurgiur99]

/run-security-scan

[alexcos20]

AI automated code review (Gemini 3).

Overall risk: low

Summary:
This is a well-designed PR that introduces a 'label' property for persistent storage buckets. The SQLite schema migration is handled cleanly, input validation is thorough, authorization properly restricts updates to the bucket owner, and the edge cases are well covered by integration tests. LGTM!

Comments:
• [INFO][style] Excellent approach to migrating the SQLite database. Using PRAGMA table_info followed by an ALTER TABLE combined with a check for 'duplicate column name' securely prevents race conditions and handles backward compatibility without requiring an external migration library.
• [INFO][style] Great forethought on leaving the ON CONFLICT clause as-is. This correctly ensures that any subsequent system re-syncs or upserts of the bucket don't accidentally overwrite or erase a user's custom label.
• [INFO][security] Good job implementing a reasonable length restriction (MAX_BUCKET_LABEL_LENGTH) and strict type-checking for the optional label property. This prevents potential malicious payloads or unintentional memory/DB bloat.
• [INFO][style] Excellent test coverage. Including negative assertions for cross-account manipulation (403), unknown buckets (404), length bounds checking, and empty-string clearing behavior proves the robustness of the implementation.

[giurgiur99]

/run-security-scan

[alexcos20]

AI automated code review (Gemini 3).

Overall risk: low

Summary:
This PR cleanly implements bucket renaming functionality, seamlessly integrating it with the existing persistent storage system. The HTTP routes, core handlers, validation, and database operations are well-structured. Excellent use of unique-names-generator for default label generation and the tests thoroughly cover authorization and validation logic. I have left one warning regarding the SQLite ALTER TABLE migration error handling and one informational note regarding PATCH semantics. Once the DB migration error handling is hardened, LGTM!

Comments:
• [WARNING][bug] Ignoring all errors from the ALTER TABLE query can mask critical database issues (e.g., corrupted database, disk full, file locks). It is safer to only ignore the specific error that occurs when the column already exists.

-        this.db.run(
-          `ALTER TABLE persistent_storage_buckets ADD COLUMN label TEXT`,
-          () => {
-            // Ignore error if column already exists
-            this.dbReady = true
-            resolve()
-          }
-        )
+        this.db.run(
+          `ALTER TABLE persistent_storage_buckets ADD COLUMN label TEXT`,
+          (err) => {
+            if (err && !err.message.includes('duplicate column name')) {
+              reject(err)
+              return
+            }
+            this.dbReady = true
+            resolve()
+          }
+        )

• [INFO][style] If task.label is undefined (omitted from the HTTP request body), it defaults to null, effectively clearing the label. In standard REST API PATCH semantics, omitting a field should leave it unchanged, whereas explicitly passing null or "" clears it. Since label is currently the only updatable field for a bucket, this is benign, but consider explicitly checking task.label !== undefined for future extensibility if more fields become updatable via this command.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

_{Tip: disable this comment in your organization's Code Review settings.}

[giurgiur99]

/run-security-scan

[alexcos20]

AI automated code review (Gemini 3).

Overall risk: low

Summary:
The PR implements a highly functional bucket labeling feature, integrating cleanly with the existing persistent storage system. The changes introduce proper RESTful updates (via PATCH), robust default label generation, and comprehensive tests. The overall design and quality are excellent. A few minor recommendations are provided regarding SQLite error handling during migration and error type checking. LGTM!

Comments:
• [WARNING][bug] The current database migration silently ignores all errors. If an actual database error occurs (such as database corruption, I/O error, or locking issues), it will be swallowed, which can lead to hard-to-debug failures later. It is highly recommended to explicitly check for the "duplicate column" error and reject on other unexpected errors.

-        this.db.run(
-          `ALTER TABLE persistent_storage_buckets ADD COLUMN label TEXT`,
-          () => {
-            // Ignore error if column already exists
-            this.dbReady = true
-            resolve()
-          }
-        )
+        this.db.run(
+          `ALTER TABLE persistent_storage_buckets ADD COLUMN label TEXT`,
+          (err) => {
+            if (err && !err.message.includes('duplicate column name')) {
+              reject(err)
+              return
+            }
+            this.dbReady = true
+            resolve()
+          }
+        )

• [INFO][style] Relying on string matching for error types (message.toLowerCase().includes('not found')) can be brittle if the error message is ever modified in PersistentStorageFactory. Consider defining a custom error class (e.g., PersistentStorageNotFoundError) similar to PersistentStorageAccessDeniedError to make this error-handling block more robust and type-safe.
• [INFO][other] If a user explicitly passes an empty string ("") as the label intent on creating an unlabeled bucket from the start, this fallback logic will override it with a randomly generated name (because "" is falsy). This is acceptable since they can clear the label via an update request later, but it's worth keeping in mind in case you want to allow creating unlabeled buckets outright by checking label !== undefined instead.

[greptile-apps]

Greptile Summary

This PR adds an optional human-readable label field to persistent storage buckets — accepted on creation, auto-generated via unique-names-generator when omitted, stored in SQLite (with a migration for existing databases), and returned in all bucket-read responses. A new PATCH /persistentStorage/buckets/:bucketId endpoint and PersistentStorageUpdateBucketHandler let owners rename or clear a bucket's label after creation.

• DB migration (PersistentStorageFactory): ALTER TABLE … ADD COLUMN label TEXT runs after CREATE TABLE IF NOT EXISTS; only "duplicate column name" errors are suppressed, so genuine failures surface cleanly. The ON CONFLICT upsert intentionally leaves label untouched, preventing a re-registration from clobbering a user rename.
• updateBucketLabel (PersistentStorageFactory): fetches and verifies ownership before calling dbUpdateBucketLabel; the boolean result is checked and throws on false, so a mid-flight deletion is detected rather than silently returning a stale label.
• Validation (persistentStorage.ts handler): validateOptionalLabel rejects non-strings and strings over 256 chars, but accepts whitespace-only values which are later silently normalized to null or a generated name — callers receive no validation error for label: \" \".

Confidence Score: 4/5

Safe to merge; all core paths (migration, ownership check, race-condition detection) are handled correctly.

Both concerns raised in previous review threads have been addressed: the migration surfaces non-duplicate-column SQLite errors, and the dbUpdateBucketLabel return value is now checked. The only remaining finding is that validateOptionalLabel accepts whitespace-only strings that are silently normalized away, which is a UX inconsistency but not a correctness or security issue.

src/components/core/handler/persistentStorage.ts — validateOptionalLabel should reject whitespace-only strings to match actual storage behavior.

Important Files Changed

Filename	Overview
src/components/persistentStorage/PersistentStorageFactory.ts	Core DB layer extended with label column: migration properly gates on "duplicate column name", dbUpsertBucket intentionally skips label on re-upsert, and updateBucketLabel correctly checks the return value of dbUpdateBucketLabel. No new blocking issues.
src/components/core/handler/persistentStorage.ts	New PersistentStorageUpdateBucketHandler with auth, ownership, and label validation. Minor: validateOptionalLabel silently accepts whitespace-only strings that are later normalized to null/generated-name; redundant bucketId guard after validateCommandParameters.
src/components/httpRoutes/persistentStorage.ts	New PATCH route for label updates, consistent with existing route patterns. express.json() included. label: req.body?.label correctly maps undefined body to no-op PATCH semantics.
src/components/persistentStorage/PersistentStorageLocalFS.ts	Auto-generates a friendly adjective-animal label via unique-names-generator when no label is provided on bucket creation; stores and returns the final label.
src/test/integration/persistentStorage.test.ts	Good coverage: creates with label, auto-label default, rename, clear by empty string, 403 for non-owner, 404 for unknown bucket, validation rejection for over-long label.
src/@types/commands.ts	Adds label?: string to PersistentStorageCreateBucketCommand and new PersistentStorageUpdateBucketCommand interface; both changes look correct.
src/utils/constants.ts	Adds PERSISTENT_STORAGE_UPDATE_BUCKET to both PROTOCOL_COMMANDS and SUPPORTED_PROTOCOL_COMMANDS — correct and complete.

Sequence Diagram

sequenceDiagram
    participant Client
    participant HTTPRoute as HTTP Route
    participant Handler as UpdateBucketHandler
    participant Factory as PersistentStorageFactory
    participant DB as SQLite

    Client->>HTTPRoute: "PATCH /buckets/:bucketId?consumerAddress=&signature=&nonce="
    HTTPRoute->>Handler: handle(task)
    Handler->>Handler: verifyParamsAndRateLimits()
    Handler->>Handler: validateTokenOrSignature()
    Handler->>Factory: updateBucketLabel(bucketId, label, owner)
    Factory->>Factory: validateBucket(bucketId)
    Factory->>DB: dbGetBucket(bucketId)
    DB-->>Factory: BucketRow
    Factory->>Factory: ownership check
    alt "label === undefined"
        Factory-->>Handler: return existing label (no-op)
    else label provided
        Factory->>Factory: normalize label
        Factory->>DB: dbUpdateBucketLabel(bucketId, owner, normalized)
        DB-->>Factory: "changes === 1?"
        alt "changes === 0"
            Factory-->>Handler: throw Bucket not found
        else
            Factory-->>Handler: return normalized label
        end
    end
    Handler-->>HTTPRoute: "stream { bucketId, label }"
    HTTPRoute-->>Client: "200 { bucketId, label }"

Loading

_{Reviews (2): Last reviewed commit: "fix review comments" | Re-trigger Greptile}