refactor(volume): simplify state, rename source to volume_id, allow name on REST create - #1208
Open
G4614 wants to merge 1 commit into
Open
refactor(volume): simplify state, rename source to volume_id, allow name on REST create#1208G4614 wants to merge 1 commit into
G4614 wants to merge 1 commit into
Conversation
…ame on REST create Three related cleanups to the managed-volume feature (boxlite-ai#1191/boxlite-ai#1192/boxlite-ai#1056): 1. VolumeState: drops the pending_create/pending_delete stages (the reconciler's per-volume Redis lock already distinguishes queued from in-progress, so a DB-level pending stage was redundant) and renames deleting/deleted to destroying/destroyed to match BoxState's vocabulary. New enum: creating/ready/destroying/destroyed/error. Includes a pre-deploy migration that widens volume_state_enum and migrates existing rows; old labels stay defined on the Postgres enum (can't be dropped) but unused, same pattern as the ssh_access table noted in migrations/README.md. As a side effect this also fixes a pre-existing gap: the reconciler only polled the old pending_* states, so a crash mid-creation orphaned the row forever — it now polls creating/destroying directly and picks interrupted volumes back up on the next tick. 2. REST VolumeSpec: source (scheme-qualified, 'volume://<id>') is now volume_id (bare id) on the new /v1 REST surface. The deprecated host_path fallback is unchanged (still requires the volume:// scheme for existing clients built against it). 3. POST /v1/volumes now accepts an optional name, enforced unique within the organization by the existing VolumeService.create() check — the endpoint previously always defaulted to the server-assigned id since it passed an empty DTO. Test plan: - yarn nx run api:build — clean - yarn nx run api:test — 65/67 suites pass; the 2 failures (usage.service.integration.spec.ts, job.service.claim.integration.spec.ts) need a real local Postgres unrelated to volumes and fail identically on main - yarn nx run dashboard:build — clean (VolumeState consumers updated) - (cd apps/libs/api-client-go && go build ./...) — clean - cargo check -p boxlite --features rest && cargo test rest::types — clean - cargo fmt --all -- --check — clean
📦 BoxLite review — couldn't completepowered by BoxLite |
📝 WalkthroughWalkthroughThe PR simplifies volume lifecycle states, adds named volume creation, renames managed-volume input from ChangesVolume lifecycle state simplification
REST volume contract updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VolumeClient
participant BoxliteVolumeController
participant VolumeService
participant VolumeManager
participant Dashboard
VolumeClient->>BoxliteVolumeController: submit named volume request
BoxliteVolumeController->>VolumeService: create volume with name
VolumeService->>VolumeManager: persist CREATING volume
Dashboard->>VolumeService: request deletion
VolumeService->>VolumeManager: process DESTROYING volume
VolumeManager-->>Dashboard: publish DESTROYED state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/box/managers/volume.manager.ts (1)
199-209: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTreat
BucketAlreadyOwnedByYouas success during creation recovery.When a resumed
CREATINGvolume already has its bucket, S3 returns this error in Regions other thanus-east-1. The current catch block changes the volume toERRORbefore applying tags or savingREADY. Handle this error as success, then apply tags and persistREADY.🤖 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 `@apps/api/src/box/managers/volume.manager.ts` around lines 199 - 209, Update handleCreating to catch and ignore the S3 BucketAlreadyOwnedByYou error from CreateBucketCommand, treating it as successful creation. Continue through tag application and persist the volume as READY for this case, while preserving ERROR handling for all other failures.
🤖 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.
Inline comments:
In
`@apps/api/src/migrations/pre-deploy/1786200000000-simplify-volume-state-migration.ts`:
- Around line 17-26: The migration currently adds PostgreSQL enum values and
uses them within the same transaction, which is unsupported. Configure TypeORM
for per-migration transaction control, mark the enum-addition queries in the
migration’s `up` method as non-transactional, and move the volume state updates
plus default change into a later migration so the new values are committed
before use.
In `@apps/dashboard/src/components/VolumeTable.tsx`:
- Line 193: Apply isVolumeDeletable(row.original) to the per-row checkbox and
delete-menu action so DESTROYED and DESTROYING volumes cannot be selected or
activated, using disabled semantics rather than only pointer-events styling. In
the delete action handler, re-check isVolumeDeletable before invoking onDelete,
while preserving processingVolumeAction handling for in-progress operations.
In `@openapi/box.openapi.yaml`:
- Around line 1829-1845: Add an anyOf constraint to the filesystem mount schema
alongside the existing required guest_path declaration, requiring either
volume_id or the deprecated host_path property while preserving their current
definitions and compatibility behavior.
- Around line 197-201: Update the requestBody definition for the
CreateVolumeRequest schema to set required: true, ensuring clients must provide
a JSON body before the controller passes it to VolumeService.create.
In `@src/boxlite/src/rest/types.rs`:
- Around line 226-234: Normalize managed volume sources in the REST conversion
implemented by From<&VolumeSpec> for CreateBoxVolumeSpec so volume_id contains
only the bare ID, regardless of whether host_path is volume://<id> or already
normalized. Preserve non-managed paths unchanged, and add regression coverage
for both prefixed SDK inputs and already-bare IDs.
---
Outside diff comments:
In `@apps/api/src/box/managers/volume.manager.ts`:
- Around line 199-209: Update handleCreating to catch and ignore the S3
BucketAlreadyOwnedByYou error from CreateBucketCommand, treating it as
successful creation. Continue through tag application and persist the volume as
READY for this case, while preserving ERROR handling for all other failures.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b2fa48d-2d9d-421a-971b-9303972c5258
📒 Files selected for processing (23)
apps/api/src/box/entities/volume.entity.tsapps/api/src/box/enums/volume-state.enum.tsapps/api/src/box/managers/volume.manager.tsapps/api/src/box/services/volume.service.spec.tsapps/api/src/box/services/volume.service.tsapps/api/src/boxlite-rest/boxlite-volume.controller.spec.tsapps/api/src/boxlite-rest/boxlite-volume.controller.tsapps/api/src/boxlite-rest/dto/create-box.dto.spec.tsapps/api/src/boxlite-rest/dto/create-box.dto.tsapps/api/src/boxlite-rest/dto/create-volume.dto.tsapps/api/src/boxlite-rest/mappers/box-to-box.mapper.spec.tsapps/api/src/boxlite-rest/mappers/box-to-box.mapper.tsapps/api/src/migrations/pre-deploy/1786200000000-simplify-volume-state-migration.tsapps/dashboard/src/components/VolumeTable.tsxapps/dashboard/src/components/VolumeTable/useVolumeCommands.tsxapps/dashboard/src/hooks/useVolumeWsSync.tsapps/dashboard/src/pages/Volumes.tsxapps/libs/api-client-go/api/openapi.yamlapps/libs/api-client-go/model_volume_state.goapps/libs/api-client/src/docs/VolumeState.mdapps/libs/api-client/src/models/volume-state.tsopenapi/box.openapi.yamlsrc/boxlite/src/rest/types.rs
Comment on lines
+17
to
+26
| await queryRunner.query(`ALTER TYPE "public"."volume_state_enum" ADD VALUE IF NOT EXISTS 'destroying'`) | ||
| await queryRunner.query(`ALTER TYPE "public"."volume_state_enum" ADD VALUE IF NOT EXISTS 'destroyed'`) | ||
|
|
||
| await queryRunner.query(`UPDATE "volume" SET "state" = 'creating' WHERE "state" = 'pending_create'`) | ||
| await queryRunner.query( | ||
| `UPDATE "volume" SET "state" = 'destroying' WHERE "state" IN ('pending_delete', 'deleting')`, | ||
| ) | ||
| await queryRunner.query(`UPDATE "volume" SET "state" = 'destroyed' WHERE "state" = 'deleted'`) | ||
|
|
||
| await queryRunner.query(`ALTER TABLE "volume" ALTER COLUMN "state" SET DEFAULT 'creating'`) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify that migration execution is explicitly non-transactional before
# relying on newly-added PostgreSQL enum values in the same migration.
rg -n -C 3 'migrationsTransactionMode|transaction\s*=' apps \
-g '*.ts' -g '*.js'
sed -n '1,80p' apps/api/src/migrations/pre-deploy/1786200000000-simplify-volume-state-migration.tsRepository: boxlite-ai/boxlite
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- migration ---'
sed -n '1,140p' apps/api/src/migrations/pre-deploy/1786200000000-simplify-volume-state-migration.ts
printf '%s\n' '--- migration configuration and runners ---'
rg -n -C 4 'migrationsTransactionMode|transaction\s*[:=]|runMigrations|MigrationExecutor|pre-deploy|migrations' apps \
-g '*.ts' -g '*.js' -g '*.json' -g '*.yml' -g '*.yaml' || true
printf '%s\n' '--- relevant package metadata ---'
rg -n -C 3 '"typeorm"|"migration"' apps package.json package-lock.json npm-shrinkwrap.json yarn.lock pnpm-lock.yaml 2>/dev/null || trueRepository: boxlite-ai/boxlite
Length of output: 21855
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- datasource options ---'
sed -n '1,75p' apps/api/src/migrations/data-source.ts
sed -n '65,115p' apps/api/src/main.ts
sed -n '60,100p' apps/api/src/app.module.ts
printf '%s\n' '--- all migration transaction settings ---'
rg -n -C 5 'migrations\s*:\s*\{|transaction\s*[:=]|MigrationExecutor|setOptions\s*\(' . \
-g '*.ts' -g '*.js' -g '*.json' -g '!node_modules' -g '!dist' -g '!build' || true
printf '%s\n' '--- TypeORM version and local source availability ---'
rg -n -C 2 '"typeorm"\s*:' apps/package.json package.json
fd -HI 'migration-executor|MigrationExecutor' . -t f -E node_modules -E dist -E build || trueRepository: boxlite-ai/boxlite
Length of output: 13935
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
migration = Path("apps/api/src/migrations/pre-deploy/1786200000000-simplify-volume-state-migration.ts").read_text()
datasource = Path("apps/api/src/migrations/data-source.ts").read_text()
predeploy = Path("apps/api/src/migrations/pre-deploy/data-source.ts").read_text()
app = Path("apps/api/src/app.module.ts").read_text()
add_destroying = migration.index("ADD VALUE IF NOT EXISTS 'destroying'")
use_destroying = migration.index("SET \"state\" = 'destroying'")
add_destroyed = migration.index("ADD VALUE IF NOT EXISTS 'destroyed'")
use_destroyed = migration.index("SET \"state\" = 'destroyed'")
print({
"destroying_added_before_use": add_destroying < use_destroying,
"destroyed_added_before_use": add_destroyed < use_destroyed,
"datasource_transaction_option": bool(re.search(r"migrations\s*:\s*\{[^}]*transaction|migrationsTransactionMode", datasource, re.S)),
"predeploy_transaction_option": bool(re.search(r"migrations\s*:\s*\{[^}]*transaction|migrationsTransactionMode", predeploy, re.S)),
"app_transaction_option": bool(re.search(r"migrations\s*:\s*\{[^}]*transaction|migrationsTransactionMode", app, re.S)),
})
PYRepository: boxlite-ai/boxlite
Length of output: 342
🌐 Web query:
PostgreSQL ALTER TYPE ADD VALUE new enum value cannot be used until after transaction commit official documentation
💡 Result:
According to the official PostgreSQL documentation, if the ALTER TYPE... ADD VALUE command is executed within a transaction block, the new enum value cannot be used until that transaction has been committed [1][2][3]. This restriction exists because the database must prevent uncommitted enum values from being stored in tables, which could potentially break indexes on enum columns [4]. Historical versions of PostgreSQL (prior to version 12) strictly disallowed the execution of this command entirely within transaction blocks; however, this restriction was subsequently relaxed to allow the command, provided the new value is not referenced until after the transaction commits [5][4].
Citations:
- 1: https://www.postgresql.org/docs/current/sql-altertype.html
- 2: https://www.postgresql.org/docs/19/sql-altertype.html
- 3: https://www.postgresql.org/docs/18/sql-altertype.html
- 4: https://www.postgresql.org/message-id/E1bgxFm-0005uI-1P%40gemulon.postgresql.org
- 5: https://www.postgresql.org/docs/11/sql-altertype.html
🌐 Web query:
TypeORM 0.3 migrations transaction default all official documentation
💡 Result:
In TypeORM 0.3, the default behavior for running migrations is to wrap all migrations into a single transaction [1][2][3]. This is controlled by the migrationsTransactionMode setting, which defaults to all [1][4]. The available options for migrationsTransactionMode are: - all (default): Wraps the entire migration process into a single transaction [1][4]. - each: Wraps each individual migration file into its own separate transaction [1][4]. - none: Disables automatic transaction wrapping for migrations [1][4]. In addition to the global migrationsTransactionMode setting, you can override transaction behavior on a per-migration basis by setting the transaction property (a boolean) within your migration class [2][3]. Note that this per-migration override only functions when the global mode is set to each or none [2][3].
Citations:
- 1: https://typeorm.io/docs/migrations/setup/
- 2: https://typeorm.io/docs/migrations/faking/
- 3: https://github.com/typeorm/typeorm/blob/master/docs/migrations.md
- 4: https://orkhan.gitbook.io/typeorm/docs/docs/migrations/02-setup.md
Commit enum values before using them.
Configure TypeORM with per-migration transactions. Run the enum additions with transaction = false, then run the updates and default change in a later migration.
🤖 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
`@apps/api/src/migrations/pre-deploy/1786200000000-simplify-volume-state-migration.ts`
around lines 17 - 26, The migration currently adds PostgreSQL enum values and
uses them within the same transaction, which is unsupported. Configure TypeORM
for per-migration transaction control, mark the enum-addition queries in the
migration’s `up` method as non-transactional, and move the volume state updates
plus default change into a later migration so the new values are committed
before use.
| key={row.id} | ||
| data-state={row.getIsSelected() && 'selected'} | ||
| className={`${processingVolumeAction[row.original.id] || row.original.state === VolumeState.PENDING_DELETE || row.original.state === VolumeState.DELETING ? 'opacity-50 pointer-events-none' : ''}`} | ||
| className={`${processingVolumeAction[row.original.id] || row.original.state === VolumeState.DESTROYING ? 'opacity-50 pointer-events-none' : ''}`} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Apply deletion eligibility to every row control.
The new checks protect header selection, but the per-row checkbox still accepts DESTROYED rows. The delete menu also checks only processingVolumeAction. pointer-events-none does not provide a semantic or keyboard guard for DESTROYING.
Use isVolumeDeletable(row.original) to disable the row checkbox and delete action. Re-check the predicate before calling onDelete. Otherwise, stale terminal rows can be selected or submitted for deletion, and bulk confirmation can silently discard them.
Also applies to: 310-313
🤖 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 `@apps/dashboard/src/components/VolumeTable.tsx` at line 193, Apply
isVolumeDeletable(row.original) to the per-row checkbox and delete-menu action
so DESTROYED and DESTROYING volumes cannot be selected or activated, using
disabled semantics rather than only pointer-events styling. In the delete action
handler, re-check isVolumeDeletable before invoking onDelete, while preserving
processingVolumeAction handling for in-progress operations.
Comment on lines
+197
to
+201
| requestBody: | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: "#/components/schemas/CreateVolumeRequest" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 '`@Body`\(\).*CreateVolumeDto|createVolumeDto\.name|CreateVolumeRequest' \
apps/api/src openapi/box.openapi.yaml \
-g '*.ts' -g '*.yaml'Repository: boxlite-ai/boxlite
Length of output: 4246
Require the JSON body.
requestBody omits required, so clients may omit it. The controller passes @Body() directly to VolumeService.create, which evaluates createVolumeDto.name; an omitted body can cause a runtime error. Add required: true, or normalize an absent body before calling the service.
🤖 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 `@openapi/box.openapi.yaml` around lines 197 - 201, Update the requestBody
definition for the CreateVolumeRequest schema to set required: true, ensuring
clients must provide a JSON body before the controller passes it to
VolumeService.create.
Comment on lines
+1829
to
+1845
| Filesystem mount attached to a box. One of `volume_id` or the | ||
| deprecated `host_path` is required. | ||
| required: [guest_path] | ||
| properties: | ||
| source: | ||
| volume_id: | ||
| type: string | ||
| description: | | ||
| Scheme-qualified mount source. Use `volume://<volume_id>` for managed | ||
| volumes. | ||
| example: volume://vol_01K2EXAMPLE | ||
| description: Id of a managed volume created via `POST /volumes`. | ||
| example: vol_01K2EXAMPLE | ||
| host_path: | ||
| type: string | ||
| deprecated: true | ||
| description: | | ||
| Deprecated alias for `source`, kept for backward compatibility with | ||
| existing /v1 clients built against the pre-managed-volumes schema. | ||
| Use `source` with the `volume://<volume_id>` scheme instead; this | ||
| field will be removed in a future API version. | ||
| Deprecated alias for `volume_id`, kept for backward compatibility | ||
| with existing /v1 clients built against the pre-`volume_id` | ||
| schema. Still requires the `volume://<volume_id>` scheme those | ||
| clients send. Use `volume_id` instead; this field will be removed | ||
| in a future API version. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Encode the required managed-volume reference.
The description requires volume_id or host_path, but the schema accepts a VolumeSpec with only guest_path. Add an anyOf constraint so generated clients and validators reject mounts without a volume reference.
Proposed schema fix
VolumeSpec:
type: object
+ anyOf:
+ - required: [volume_id]
+ - required: [host_path]
required: [guest_path]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Filesystem mount attached to a box. One of `volume_id` or the | |
| deprecated `host_path` is required. | |
| required: [guest_path] | |
| properties: | |
| source: | |
| volume_id: | |
| type: string | |
| description: | | |
| Scheme-qualified mount source. Use `volume://<volume_id>` for managed | |
| volumes. | |
| example: volume://vol_01K2EXAMPLE | |
| description: Id of a managed volume created via `POST /volumes`. | |
| example: vol_01K2EXAMPLE | |
| host_path: | |
| type: string | |
| deprecated: true | |
| description: | | |
| Deprecated alias for `source`, kept for backward compatibility with | |
| existing /v1 clients built against the pre-managed-volumes schema. | |
| Use `source` with the `volume://<volume_id>` scheme instead; this | |
| field will be removed in a future API version. | |
| Deprecated alias for `volume_id`, kept for backward compatibility | |
| with existing /v1 clients built against the pre-`volume_id` | |
| schema. Still requires the `volume://<volume_id>` scheme those | |
| clients send. Use `volume_id` instead; this field will be removed | |
| in a future API version. | |
| Filesystem mount attached to a box. One of `volume_id` or the | |
| deprecated `host_path` is required. | |
| anyOf: | |
| - required: [volume_id] | |
| - required: [host_path] | |
| required: [guest_path] | |
| properties: | |
| volume_id: | |
| type: string | |
| description: Id of a managed volume created via `POST /volumes`. | |
| example: vol_01K2EXAMPLE | |
| host_path: | |
| type: string | |
| deprecated: true | |
| description: | | |
| Deprecated alias for `volume_id`, kept for backward compatibility | |
| with existing /v1 clients built against the pre-`volume_id` | |
| schema. Still requires the `volume://<volume_id>` scheme those | |
| clients send. Use `volume_id` instead; this field will be removed | |
| in a future API version. |
🤖 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 `@openapi/box.openapi.yaml` around lines 1829 - 1845, Add an anyOf constraint
to the filesystem mount schema alongside the existing required guest_path
declaration, requiring either volume_id or the deprecated host_path property
while preserving their current definitions and compatibility behavior.
Comment on lines
226
to
234
| impl From<&crate::runtime::options::VolumeSpec> for CreateBoxVolumeSpec { | ||
| fn from(volume: &crate::runtime::options::VolumeSpec) -> Self { | ||
| Self { | ||
| source: managed_volume_source(&volume.host_path), | ||
| // `--mount src=volume://<id>,...` already strips the scheme | ||
| // before storing the bare id in `host_path` (see | ||
| // `normalize_managed_volume_source` in the CLI) — the wire field | ||
| // takes that bare id directly, no scheme wrapping needed. | ||
| volume_id: volume.host_path.clone(), | ||
| guest_path: volume.guest_path.clone(), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the type and normalization helpers before inspecting call sites.
ast-grep outline src/boxlite/src/runtime/options.rs --items all
# Find VolumeSpec constructors, managed-volume normalization, and persisted forms.
rg -n -U -C 4 --type rust \
'VolumeSpec\s*\{|host_path\s*:|normalize_managed_volume_source|volume://' \
src/boxlite/srcRepository: boxlite-ai/boxlite
Length of output: 22437
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Trace managed-volume parsing and every path that creates or converts VolumeSpec.
rg -n -U -C 6 --type rust \
'normalize_managed_volume_source|volume://|VolumeSpec\s*\{|CreateBoxVolumeSpec|\.volumes|volumes\s*:' \
.
# Inspect the exact implementations for the discovered normalization and conversion paths.
files=$(rg -l --type rust \
'normalize_managed_volume_source|CreateBoxVolumeSpec|VolumeSpec\s*\{' . | head -20)
for file in $files; do
echo "===== $file ====="
sed -n '1,260p' "$file" | grep -n -E \
-B 12 -A 28 'normalize_managed_volume_source|CreateBoxVolumeSpec|VolumeSpec\s*\{' || true
doneRepository: boxlite-ai/boxlite
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -U -C 6 --type rust \
'normalize_managed_volume_source|volume://|VolumeSpec\s*\{|CreateBoxVolumeSpec|\.volumes|volumes\s*:' .
files=$(rg -l --type rust \
'normalize_managed_volume_source|CreateBoxVolumeSpec|VolumeSpec\s*\{' . | head -20)
for file in $files; do
echo "===== $file ====="
sed -n '1,260p' "$file" | grep -n -E \
-B 12 -A 28 'normalize_managed_volume_source|CreateBoxVolumeSpec|VolumeSpec\s*\{' || true
doneRepository: boxlite-ai/boxlite
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the complete conversion paths that preserve scheme-qualified managed sources.
sed -n '250,305p' sdks/node/src/options.rs
sed -n '585,655p' sdks/python/src/options.rs
sed -n '635,685p' sdks/python/src/options.rs
sed -n '930,1002p' src/cli/src/cli.rs
# Locate the REST conversion call sites and the backend path that sends BoxOptions.
rg -n -U -C 8 --type rust \
'CreateBoxVolumeSpec|BoxOptions.*volumes|volumes.*CreateBox|create_box|create\(' \
src/boxlite/src/rest src/boxlite/src/runtime src/cli/src sdks/node/src sdks/python/srcRepository: boxlite-ai/boxlite
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '250,305p' sdks/node/src/options.rs
sed -n '585,655p' sdks/python/src/options.rs
sed -n '635,685p' sdks/python/src/options.rs
sed -n '930,1002p' src/cli/src/cli.rs
rg -n -U -C 8 --type rust \
'CreateBoxVolumeSpec|BoxOptions.*volumes|volumes.*CreateBox|create_box|create\(' \
src/boxlite/src/rest src/boxlite/src/runtime src/cli/src sdks/node/src sdks/python/srcRepository: boxlite-ai/boxlite
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read-only source-level verifier for the representation invariant and REST mapping.
python3 - <<'PY'
from pathlib import Path
node = Path("sdks/node/src/options.rs").read_text()
python = Path("sdks/python/src/options.rs").read_text()
cli = Path("src/cli/src/cli.rs").read_text()
rest = Path("src/boxlite/src/rest/types.rs").read_text()
checks = {
"Node accepts scheme-qualified source": 'if !source.starts_with("volume://")' in node,
"Node stores source unchanged": "host_path,\n guest_path: v.guest_path" in node,
"Python accepts scheme-qualified source": 'if !source.starts_with("volume://")' in python,
"Python stores source unchanged": "host_path: v.host" in python,
"CLI strips scheme": 'source.strip_prefix("volume://")' in cli,
"REST copies host_path directly": "volume_id: volume.host_path.clone()" in rest,
"REST maps every BoxOptions volume": ".map(CreateBoxVolumeSpec::from)" in rest,
}
for name, result in checks.items():
print(f"{name}: {'PASS' if result else 'FAIL'}")
assert all(checks.values())
# Model the relevant transformations without executing repository code.
scheme = "volume://vol_123"
cli_id = scheme.removeprefix("volume://")
sdk_id = scheme
wire_from_cli = cli_id
wire_from_sdk = sdk_id
print(f"CLI host_path -> wire volume_id: {wire_from_cli!r}")
print(f"SDK host_path -> wire volume_id: {wire_from_sdk!r}")
assert wire_from_cli == "vol_123"
assert wire_from_sdk == "volume://vol_123"
assert wire_from_sdk != "vol_123"
PYRepository: boxlite-ai/boxlite
Length of output: 512
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
node = Path("sdks/node/src/options.rs").read_text()
python = Path("sdks/python/src/options.rs").read_text()
cli = Path("src/cli/src/cli.rs").read_text()
rest = Path("src/boxlite/src/rest/types.rs").read_text()
checks = {
"Node accepts scheme-qualified source": 'if !source.starts_with("volume://")' in node,
"Node stores source unchanged": "host_path,\n guest_path: v.guest_path" in node,
"Python accepts scheme-qualified source": 'if !source.starts_with("volume://")' in python,
"Python stores source unchanged": "host_path: v.host" in python,
"CLI strips scheme": 'source.strip_prefix("volume://")' in cli,
"REST copies host_path directly": "volume_id: volume.host_path.clone()" in rest,
"REST maps every BoxOptions volume": ".map(CreateBoxVolumeSpec::from)" in rest,
}
for name, result in checks.items():
print(f"{name}: {'PASS' if result else 'FAIL'}")
assert all(checks.values())
scheme = "volume://vol_123"
cli_id = scheme.removeprefix("volume://")
sdk_id = scheme
print(f"CLI host_path -> wire volume_id: {cli_id!r}")
print(f"SDK host_path -> wire volume_id: {sdk_id!r}")
assert cli_id == "vol_123"
assert sdk_id == "volume://vol_123"
assert sdk_id != "vol_123"
PYRepository: boxlite-ai/boxlite
Length of output: 512
Normalize managed volume sources before REST serialization
Node and Python adapters preserve volume://<id> in VolumeSpec.host_path. The REST mapper then sends that value unchanged as volume_id. Normalize these SDK inputs, or strip the scheme in the REST mapper, and add regression tests for both paths.
🤖 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 `@src/boxlite/src/rest/types.rs` around lines 226 - 234, Normalize managed
volume sources in the REST conversion implemented by From<&VolumeSpec> for
CreateBoxVolumeSpec so volume_id contains only the bare ID, regardless of
whether host_path is volume://<id> or already normalized. Preserve non-managed
paths unchanged, and add regression coverage for both prefixed SDK inputs and
already-bare IDs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three related cleanups to the managed-volume feature landed by #1191/#1192/#1056.
1.
VolumeStatesimplification — dropspending_create/pending_delete(the reconciler's per-volume Redis lock already distinguishes queued from in-progress, so a DB-level pending stage was redundant) and renamesdeleting/deletedtodestroying/destroyedto matchBoxState's vocabulary. New enum:creating/ready/destroying/destroyed/error.Includes a pre-deploy migration that widens
volume_state_enumand migrates existing rows; old labels stay defined on the Postgres enum (can't be dropped without a full type swap) but unused — same accepted pattern as thessh_accesstable noted inmigrations/README.md.Side benefit: this also fixes a pre-existing gap where a crash mid-creation orphaned the row forever, since the reconciler only re-polled the
pending_*states, nevercreating/deletingthemselves. It now pollscreating/destroyingdirectly and picks interrupted volumes back up on the next tick.2.
source→volume_id— the RESTVolumeSpec.sourcefield (scheme-qualified,volume://<id>) is nowvolume_id(bare id) on the/v1REST surface. The deprecatedhost_pathfallback is unchanged (still requires thevolume://scheme for existing clients built against it).3.
POST /v1/volumesaccepts an optionalname— enforced unique within the organization by the existingVolumeService.create()check. The endpoint previously always defaulted to the server-assigned id since it passed an empty DTO regardless of what the caller sent.Test plan:
yarn nx run api:build— cleanyarn nx run api:test— 65/67 suites pass; the 2 failures (usage.service.integration.spec.ts,job.service.claim.integration.spec.ts) need a real local Postgres, are unrelated to volumes, and fail identically on mainyarn nx run dashboard:build— clean (VolumeState consumers updated)cd apps/libs/api-client-go && go build ./...— cleancargo check -p boxlite --features rest && cargo test rest::types— cleancargo fmt --all -- --check— cleanSummary by CodeRabbit
volume_id, with deprecatedhost_pathcompatibility retained.creating,ready,destroying,destroyed, anderror.