feat(cli): native blockstor CLI speaking Kubernetes directly

[Andrei Kvapil (kvaps)]

Adds blockstor, a native CLI that reproduces the command surface operators already know and speaks the Kubernetes API directly, so the upstream python client can be dropped as a runtime dependency.

Going straight to the CRDs is not just one hop shorter — it is more correct. The store layer is already a reusable library, so the CLI gets the same DTOs the REST apiserver would return without duplicating a line of wire↔CRD translation. And because a CLI reads through a non-cached client rather than an informer cache behind N replicas, the cross-replica cache lag the apiserver carries retry machinery for cannot occur here at all.

The grammar is the one operators and this repository's harnesses already type: blockstor resource list and blockstor r l, storage-pool create and sp c, three-token snapshot resource restore. Exit codes keep the convention scripts branch on — 0 success, 2 a client-side rejection, 10 an API-level failure. Tables are built as metav1.Table and the CRDs gained the printer columns they never had, so kubectl get and the CLI agree on what a row looks like. Colour is preserved, gated on a TTY, and applied so that stripping the escapes reproduces the plain rendering byte for byte.

Where the controller already owns a decision, the CLI calls it rather than reimplementing it: placement goes through pkg/placer, the same code the resource-group controllers run. Two answers to "where should this replica go?" would drift apart the moment either changed. Several contracts that differ per verb are preserved rather than smoothed over — an explicit placement request fails on a shortfall while a group spawn defers to the rebalance reconciler; a resize refuses to shrink without --force, and the size bounds hold even with it.

error-reports is deliberately absent: the reports are a ring buffer in the controller process's memory, so a client that speaks to the API server has nothing to list. encryption enter-passphrase verifies the passphrase against the cluster Secret in constant time and succeeds, noting on stderr that the controller's own in-memory flag — which only drives the Suspended/Available column in its REST view, and gates nothing — is untouched.

The upstream python client is GPL. Its source was not read, quoted or translated. What is reproduced here is the interface — command names, flag names, column names, colour semantics — taken from this repository's own tests, scripts and parity documentation, and the implementation is written against the blockstor API types.

Design notes and the full test plan are in docs/cli-design.md.

Testing

• go test ./... — green; 148 test cases across dispatch, flag parsing, rendering, views, machine output and every write verb. They run in the existing Unit tests CI job, which enumerates packages dynamically.
• golangci-lint run ./... — 0 issues.
• A registry test fails if the grammar advertises a command nothing implements, and another fails if a noun grows set-property without list-properties and delete-property.
• Not yet run: the live tests/e2e/cli-matrix suite against a stand, pointed at blockstor instead of the python client. That is the acceptance criterion for actually dropping the dependency and is the natural follow-up.

Summary by CodeRabbit

• New Features
  • Added the blockstor CLI entrypoint and expanded its command coverage (nodes, resources, snapshots, pools, resource groups, properties, encryption, placement, DRBD options).
  • Improved CLI output with human tables, machine-readable JSON (-m), aliases, help handling, and optional semantic color plus a paste-friendly layout.
  • Enhanced Kubernetes CRD listings (kubectl get) with additional printer columns for Node, Resource, ResourceDefinition, ResourceGroup, Snapshot, and StoragePool.
• Tests
  • Added/extended CLI, output, registry, color, machine-output, and CRD printer-column validation tests.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

Blockstor CLI

Layer / File(s)	Summary
CLI foundation and output contracts internal/cli/app.go, internal/cli/command/*, internal/cli/flags.go, internal/cli/color/*, internal/cli/output/*, internal/cli/table/*	Adds command resolution, flag parsing, help, exit-code handling, color modes, machine-readable JSON, and table rendering.
Resource views internal/cli/view/*	Adds Kubernetes table views for resources, nodes, pools, snapshots, definitions, groups, volumes, placement data, and physical devices.
Object and property commands internal/cli/write*.go, internal/cli/props.go, internal/cli/pool.go, internal/cli/physical.go	Adds lifecycle commands, property-bag operations, storage-pool and volume-group handling, physical-device pool creation, size parsing, and validation.
Placement, resource, and node workflows internal/cli/place.go, internal/cli/resource.go, internal/cli/node.go	Adds auto-placement, resource disk migration/toggling, node evacuation/restoration/loss handling, and related tests.
Snapshot, encryption, and DRBD operations internal/cli/snapshot.go, internal/cli/encryption.go, internal/cli/drbdopts.go, pkg/drbd/flagkeys.go	Adds snapshot restore and batch creation, passphrase Secret operations, and DRBD option property mapping.
Executable and API presentation wiring cmd/blockstor/main.go, Makefile, api/v1alpha1/*, config/crd/bases/*, docs/cli-design.md	Builds the blockstor binary, configures Kubernetes access, adds CRD printer columns, validates generated columns, and documents the CLI design.

Estimated code review effort: 5 (Critical) | ~120 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly captures the main change: a native blockstor CLI that talks directly to Kubernetes.
Docstring Coverage	✅ Passed	Docstring coverage is 89.79% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/blockstor-cli

🔧 Fix failing CI

• Fix failing CI in branch feat/blockstor-cli

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[coderabbitai]

Actionable comments posted: 9

🧹 Nitpick comments (4)

api/v1alpha1/printcolumns_test.go (1)

42-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Pin column types and JSONPaths too.

The test accepts correct names with broken type or JSONPath, allowing blank or incorrect kubectl get output. Assert the ordered Name, Type, and JSONPath for each served column.

🤖 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 `@api/v1alpha1/printcolumns_test.go` around lines 42 - 48, Update the
print-column expectations in the test around the `want` map to include each
column’s ordered `Name`, `Type`, and `JSONPath`, rather than names alone.
Compare the served column definitions against these complete expectations so
incorrect or blank types and paths fail while preserving column order.

internal/cli/handlers.go (1)

136-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate handler definition for resource list-volumes and volume list.

Lines 136-141 are byte-identical to the volume list handler at lines 78-83 (same fetch, filter, view, and state columns). Extract a shared handler variable so the two aliases can't silently diverge if one is updated later.

♻️ Suggested consolidation

-	"volume list": listing("resources",
-		fetchResources,
-		keepResource,
-		func(resources []apiv1.Resource, _ *runContext) *metav1.Table { return view.VolumeList(resources) },
-		"State",
-	),
+	"volume list": volumeListHandler,

-	"resource list-volumes": listing("resources",
-		fetchResources,
-		keepResource,
-		func(resources []apiv1.Resource, _ *runContext) *metav1.Table { return view.VolumeList(resources) },
-		"State",
-	),
+	"resource list-volumes": volumeListHandler,

//nolint:gochecknoglobals // static dispatch table
var volumeListHandler = listing("resources",
	fetchResources,
	keepResource,
	func(resources []apiv1.Resource, _ *runContext) *metav1.Table { return view.VolumeList(resources) },
	"State",
)

🤖 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 `@internal/cli/handlers.go` around lines 136 - 141, Extract the duplicated
listing definition into a shared volumeListHandler variable, using the existing
fetchResources, keepResource, view.VolumeList, and "State" configuration.
Replace both the "resource list-volumes" and "volume list" entries in the
dispatch table with this shared handler so the aliases remain synchronized.

internal/cli/view/resource.go (1)

221-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated "first non-terminal volume" scan between worstVolume and isFaulty.

Both functions independently walk res.Volumes, skip empty DiskState, and check terminalStates for the same "non-converged" criterion. Keeping this logic in one place would prevent the display (worstVolume) and the --faulty filter (isFaulty) from silently diverging if the terminal-state classification changes later.

♻️ Proposed consolidation

+// nonTerminalVolume returns the first volume whose disk state is
+// reported and not converged.
+func nonTerminalVolume(res *apiv1.Resource) *apiv1.Volume {
+	for i := range res.Volumes {
+		state := strings.ToLower(res.Volumes[i].State.DiskState)
+		if state == "" {
+			continue
+		}
+
+		if _, terminal := terminalStates[state]; !terminal {
+			return &res.Volumes[i]
+		}
+	}
+
+	return nil
+}
+
 func worstVolume(res *apiv1.Resource) *apiv1.Volume {
 	if len(res.Volumes) == 0 {
 		return nil
 	}
-
-	for i := range res.Volumes {
-		state := strings.ToLower(res.Volumes[i].State.DiskState)
-		if state == "" {
-			continue
-		}
-
-		if _, terminal := terminalStates[state]; !terminal {
-			return &res.Volumes[i]
-		}
-	}
-
+	if v := nonTerminalVolume(res); v != nil {
+		return v
+	}
 	return &res.Volumes[0]
 }
 
 func isFaulty(res *apiv1.Resource) bool {
-	for i := range res.Volumes {
-		state := strings.ToLower(res.Volumes[i].State.DiskState)
-		if state == "" {
-			continue
-		}
-
-		if _, terminal := terminalStates[state]; !terminal {
-			return true
-		}
-	}
-
-	return false
+	return nonTerminalVolume(res) != nil
 }

Also applies to: 261-278

🤖 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 `@internal/cli/view/resource.go` around lines 221 - 240, Consolidate the
duplicated volume-state scan used by worstVolume and isFaulty into a shared
helper that selects the first non-terminal volume while skipping empty DiskState
values. Update both callers to reuse this helper and preserve worstVolume’s
fallback to the first volume when no non-terminal volume exists, keeping
terminalStates as the single classification source.

internal/cli/snapshot.go (1)

286-338: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Redundant per-node listing, and a silent empty-pool fallback.

Two things in this pair of functions:

• sourcePoolOn re-lists all replicas of srcRD (a Kubernetes API call) once per node inside placeRestored's loop. Hoisting the ListByDefinition call outside the loop avoids N redundant round-trips for an N-node restore.
• If the source definition has no replica with a StorPoolName set at all, fallback stays "" and sourcePoolOn returns ("", nil) — no error. placeRestored then stamps that empty string via stampProp on the new replica rather than surfacing a failure, which could silently create a replica with a blank storage-pool property.

♻️ Proposed fix: hoist the list call out of the loop

 func placeRestored(ctx context.Context, run *runContext, srcRD, rdName string, snap *apiv1.Snapshot) error {
 	nodes := run.Flags.Nodes
 	if len(nodes) == 0 {
 		nodes = snap.Nodes
 	}
 
+	replicas, err := run.Store.Resources().ListByDefinition(ctx, srcRD)
+	if err != nil {
+		return fmt.Errorf("list replicas of %s: %w", srcRD, err)
+	}
+
 	for _, node := range nodes {
 		res := &apiv1.Resource{Name: rdName, NodeName: node}
 
-		pool, err := sourcePoolOn(ctx, run, srcRD, node)
-		if err != nil {
-			return err
-		}
+		pool := sourcePoolFor(replicas, node)
 
 		stampProp(res, storPoolNameProp, pool)
 
-		err = run.Store.Resources().Create(ctx, res)
+		err = run.Store.Resources().Create(ctx, res)
 		if err != nil {
 			return fmt.Errorf("create restored replica %s on %s: %w", rdName, node, err)
 		}
 	}
 
 	return nil
 }

Please confirm whether stampProp treats an empty value as "leave unset" (matching pre-restore behavior when no pool is pinned) or writes an explicit empty property that downstream code might misinterpret as "no default pool" versus "unset".

🤖 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 `@internal/cli/snapshot.go` around lines 286 - 338, Update placeRestored to
call Resources().ListByDefinition once before iterating nodes, then pass the
retrieved replicas into sourcePoolOn instead of re-listing per node. Change
sourcePoolOn to return an error when no replica has a non-empty
storPoolNameProp, and ensure placeRestored propagates that error before stamping
the property; verify stampProp’s empty-value behavior and preserve the intended
unset-versus-empty semantics.

🤖 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 `@config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml`:
- Around line 24-26: The CRD printer columns use array-valued fields for the
Layers/Nodes summaries. In
config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml:24-26,
config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml:24-26, and
config/crd/bases/blockstor.cozystack.io_snapshots.yaml:24-26, replace those
paths with the controller-provided scalar string summary/status field, or remove
the columns if no such field exists.

In `@docs/cli-design.md`:
- Around line 15-21: Add a shell or console language tag to the fenced command
example in the CLI command documentation, changing the opening fence from an
untyped fence while leaving the command contents unchanged.

In `@internal/cli/definition.go`:
- Around line 186-211: Update resourceGroupQuerySizeInfo to compute
maxVolumeSizeKib using the selected group and candidate pools before the
machine-output branch, then pass machineOut the same size-information payload
represented by view.SizeInfoRows, including the resource-group name, computed
maximum size, and pools. Preserve the existing table rendering behavior and
ensure query-max-volume-size machine output reports the computed size rather
than raw pools alone.

In `@internal/cli/flags.go`:
- Around line 76-107: The valueFlags table currently treats -l separately from
--layer-list, causing assign() to store the short form under a different key
than resourceDefinitionModify reads. Update the flag alias configuration around
valueFlags so -l is folded onto the canonical --layer-list key, ensuring both
forms populate Values["layer-list"] and trigger the same behavior.

In `@internal/cli/node.go`:
- Around line 211-225: Update patchNodeFlags to use NodeStore.PatchNodeSpec
instead of the current Get-then-wholesale Update sequence. Build the patch from
the requested flag change using setFlag semantics, preserve the existing
node-not-found and update error context, and ensure concurrent node flag edits
are merged rather than overwritten.

In `@internal/cli/physical.go`:
- Around line 42-84: Update physicalStorageCreateDevicePool and the
device-stamping flow around stampDevices to track which devices were
successfully stamped, then perform best-effort compensating cleanup if a later
device lookup fails or StoragePools().Create returns a non-AlreadyExists error.
Cleanup must remove the pool attachment from only those devices, preserve the
original operation error, and avoid changing the existing AlreadyExists
behavior.

In `@internal/cli/resource.go`:
- Around line 147-180: Update migrateDisk to reject a self-referential migration
when the migrate-from value src equals the destination dst, returning the
existing migration validation error before fetching or stamping the destination
resource. Preserve normal source validation and migration behavior when src and
dst differ.

In `@internal/cli/write_more.go`:
- Around line 156-195: Update volumeDefinitionCreate to validate sizeKib with
the same checkResize bounds used by volumeDefinitionSetSize before constructing
or storing the VolumeDefinition. Return the validation error and preserve the
existing explicit and automatic numbering flows.

In `@internal/cli/write.go`:
- Around line 57-90: Eliminate the stale read/update window between setProperty
and objectProps.set by changing the setter contract to accept a mutation
callback or single-key delta instead of a precomputed property map. Update
setProperty to pass an add/delete operation, and have objectProps.set/apply
perform the fresh GET, mutate the retrieved bag, and update it, preserving
deletion for empty values; add conflict retry if supported by the existing store
patterns.

---

Nitpick comments:
In `@api/v1alpha1/printcolumns_test.go`:
- Around line 42-48: Update the print-column expectations in the test around the
`want` map to include each column’s ordered `Name`, `Type`, and `JSONPath`,
rather than names alone. Compare the served column definitions against these
complete expectations so incorrect or blank types and paths fail while
preserving column order.

In `@internal/cli/handlers.go`:
- Around line 136-141: Extract the duplicated listing definition into a shared
volumeListHandler variable, using the existing fetchResources, keepResource,
view.VolumeList, and "State" configuration. Replace both the "resource
list-volumes" and "volume list" entries in the dispatch table with this shared
handler so the aliases remain synchronized.

In `@internal/cli/snapshot.go`:
- Around line 286-338: Update placeRestored to call Resources().ListByDefinition
once before iterating nodes, then pass the retrieved replicas into sourcePoolOn
instead of re-listing per node. Change sourcePoolOn to return an error when no
replica has a non-empty storPoolNameProp, and ensure placeRestored propagates
that error before stamping the property; verify stampProp’s empty-value behavior
and preserve the intended unset-versus-empty semantics.

In `@internal/cli/view/resource.go`:
- Around line 221-240: Consolidate the duplicated volume-state scan used by
worstVolume and isFaulty into a shared helper that selects the first
non-terminal volume while skipping empty DiskState values. Update both callers
to reuse this helper and preserve worstVolume’s fallback to the first volume
when no non-terminal volume exists, keeping terminalStates as the single
classification source.

🪄 Autofix (Beta)

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: c4e32cd8-3f3f-4ba2-b793-61160c1f433c

📥 Commits

Reviewing files that changed from the base of the PR and between b873285 and e20b56f.

📒 Files selected for processing (59)

• Makefile
• api/v1alpha1/node_types.go
• api/v1alpha1/printcolumns_test.go
• api/v1alpha1/resource_types.go
• api/v1alpha1/resourcedefinition_types.go
• api/v1alpha1/resourcegroup_types.go
• api/v1alpha1/snapshot_types.go
• api/v1alpha1/storagepool_types.go
• cmd/blockstor/main.go
• config/crd/bases/blockstor.cozystack.io_nodes.yaml
• config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml
• config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml
• config/crd/bases/blockstor.cozystack.io_resources.yaml
• config/crd/bases/blockstor.cozystack.io_snapshots.yaml
• config/crd/bases/blockstor.cozystack.io_storagepools.yaml
• docs/cli-design.md
• internal/cli/app.go
• internal/cli/app_test.go
• internal/cli/color/color.go
• internal/cli/color/color_test.go
• internal/cli/command/registry.go
• internal/cli/command/registry_test.go
• internal/cli/definition.go
• internal/cli/definition_test.go
• internal/cli/drbdopts.go
• internal/cli/drbdopts_test.go
• internal/cli/encryption.go
• internal/cli/encryption_test.go
• internal/cli/flags.go
• internal/cli/handlers.go
• internal/cli/help.go
• internal/cli/node.go
• internal/cli/node_test.go
• internal/cli/output/machine.go
• internal/cli/output/machine_test.go
• internal/cli/physical.go
• internal/cli/place.go
• internal/cli/place_test.go
• internal/cli/pool.go
• internal/cli/pool_test.go
• internal/cli/props.go
• internal/cli/props_test.go
• internal/cli/resource.go
• internal/cli/resource_test.go
• internal/cli/snapshot.go
• internal/cli/snapshot_test.go
• internal/cli/table/table.go
• internal/cli/table/table_test.go
• internal/cli/view/node.go
• internal/cli/view/resource.go
• internal/cli/view/resource_test.go
• internal/cli/view/views.go
• internal/cli/view/views_test.go
• internal/cli/write.go
• internal/cli/write_more.go
• internal/cli/write_more_test.go
• internal/cli/write_test.go
• pkg/drbd/flagkeys.go
• pkg/drbd/flagkeys_test.go

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the CRD files and inspect the relevant statuses.spec.columns entries.
for f in \
  config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml \
  config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml \
  config/crd/bases/blockstor.cozystack.io_snapshots.yaml
do
  echo "===== $f ====="
  if [ -f "$f" ]; then
    nl -ba "$f" | sed -n '1,80p'
  else
    echo "missing"
  fi
done

echo "===== search for printer columns and relevant spec fields ====="
rg -n "jsonPath:|\\.spec\\.(layerStack|selectFilter\\.layerStack|nodes)|type: string|selectFilter|layerStack|nodes:" config/crd/bases -S

Repository: cozystack/blockstor

Length of output: 275

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

for f in \
  config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml \
  config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml \
  config/crd/bases/blockstor.cozystack.io_snapshots.yaml
do
  echo "===== $f ====="
  if [ -f "$f" ]; then
    cat -n "$f" | sed -n '1,90p'
  else
    echo "missing"
  fi
done

echo "===== search for printer columns and relevant spec fields ====="
rg -n "jsonPath:|spec\.(layerStack|selectFilter\.layerStack|nodes)|type: string|selectFilter|layerStack|nodes:" config/crd/bases -S

Repository: cozystack/blockstor

Length of output: 35857

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
from pathlib import Path
import re
import yaml

files = [
    Path("config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml"),
    Path("config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml"),
    Path("config/crd/bases/blockstor.cozystack.io_snapshots.yaml"),
]

columns = []
for p in files:
    docs = list(yaml.safe_load_all(p.read_text()))
    for doc in docs:
        if not doc or doc.get("kind") != "CustomResourceDefinition":
            continue
        name = doc["metadata"]["name"]
        schema = doc["spec"]["versions"][0]["schema"]["openAPIV3Schema"]["properties"]["spec"]
        for col in doc["spec"]["versions"][0]["additionalPrinterColumns"]:
            path = col["jsonPath"]
            # Normalize JSONPath slices/indices for lookup.
            lookup = [p.strip() for p in re.sub(r'(\[\d+?\])', lambda m: '.'+m.group(1), path.split(".spec")[-1]).split(".") if p] if ".spec" in path else []
            current = schema
            prop_path = []
            for part in lookup:
                if part.startswith("["):
                    idx = int(part.strip("[]"))
                    if isinstance(current, list):
                        if idx < 0 or idx >= len(current):
                            break
                        current = current[idx]
                        prop_path.append(part)
                        continue
                else:
                    next_elem = None
                    for prop in current.get("properties", {}) if isinstance(current, dict) else []:
                        if prop == "properties":
                            continue
                        pattern = prop.replace("*", ".*")
                        if re.fullmatch(pattern, part):
                            next_elem = (prop, current["properties"][prop])
                            break
                    if not next_elem:
                        break
                    prop_path.append(next_elem[0])
                    current = next_elem[1]
            current_type = current.get("type") if isinstance(current, dict) else None
            columns.append((name, col["line"] if hasattr(col, "line") else None, path, col["type"], current_type, prop_path))

print("printer_columns_analysis")
for name, line, jmp, declared, current_type, prop_path in columns:
    print(f"{name}:{jmp}:{declared}:current_type={current_type}:path={prop_path}")
PY

Repository: cozystack/blockstor

Length of output: 274

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Manually parse the relevant CRD schema property/type declarations without PyYAML.
python3 - <<'PY'
from pathlib import Path
import re

files = [
    Path("config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml"),
    Path("config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml"),
    Path("config/crd/bases/blockstor.cozystack.io_snapshots.yaml"),
]

queries = {
    "ResourceDefinition": ".spec.layerStack",
    "ResourceGroup": ".spec.selectFilter.layerStack",
    "Snapshot": ".spec.nodes",
}

for p in files:
    text = p.read_text()
    docs = re.split(r'^---\n', text, flags=re.M)
    for doc in docs:
        if 'name: blockstor.cozystack.io_' in doc:
            name = doc.split("metadata:\n  name: ")[1].split("\n", 1)[0].strip() if 'metadata:\n  name: ' in doc else "unknown"
            columns_match = re.search(r'^\s*- jsonPath:\s*(.*?)\n(?:\s*\S+.*?\n)+', doc, re.S|re.M)
            columns_match = re.search(r'additionalPrinterColumns:(.*?)^    name: v1alpha1', doc, re.S|re.M)
            columns_text = columns_match.group(1) if columns_match else ""
            cols = []
            for block in re.split(r'^\s*- jsonPath:\s*', columns_text, flags=re.M)[1:]:
                jp = block.split("\n", 1)[0].strip()
                tp = re.search(r'\n\s*type:\s*(\S+)', block)
                cols.append((jp, tp.group(1) if tp else None))
            print(f"===== {p.name}: kind={name} columns =====")
            for jp, tp in cols:
                if jp == queries.get(name):
                    type_type_match = re.search(re.escape(jp.replace(".", r"\."))+r"\n\s*type:\s*(\S+)", doc)
                    array_match = re.search(r'\n\s*type:\s*array\n(?:\s+\S+.*?\n)+?\s+\S+.*?\n\s*type:\s*string', doc, re.S)
                    # simple next type after exact path
                    type_type_match = re.search(re.escape(jp)+r"\n(\s*)type:\s*(\S+)", doc)
                    print(f"{jp} declared_printer={tp} schema_type={type_type_match.group(2) if type_type_match else None}")
                    if name == "Snapshot":
                        # inspect .spec.nodes schema block
                        n = doc.find(re.escape(".spec.nodes"))
                        start = doc.find("properties:", n)
                        snippet = doc[start:start+800] if start > 0 else ""
                        print("nodes schema block lines 120-137:")
                        lines = doc.splitlines()
                        for i, line in enumerate(lines[120:140], 121):
                            print(f"{i}: {line}")
PY

echo "===== Kubernetes JSONPath printer-column behavior docs references ====="
curl -sSL 'https://kubernetes.io/docs/reference/using-api/api-concepts/#standard-output-formats' \
  | grep -n -i -C 3 'additionalPrinterColumns\|JSONPath\|type:' \
  | head -n 120 || true

curl -sSL 'https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#custom-columns' \
  | grep -n -i -C 3 'additionalPrinterColumns\|JSONPath\|type:' \
  | head -n 120 || true

Repository: cozystack/blockstor

Length of output: 37084

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
from pathlib import Path
import re

files = [
    Path("config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml"),
    Path("config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml"),
    Path("config/crd/bases/blockstor.cozystack.io_snapshots.yaml"),
]

for p in files:
    text = p.read_text()
    docs = re.split(r'^---\n', text, flags=re.M)
    for doc in docs:
        if 'name: blockstor.cozystack.io_' not in doc:
            continue
        name = re.search(r'name:\s*([\w+-]+)\.blockstor\.cozystack\.io', doc)
        name = name.group(1) if name else "unknown"
        schema = doc.split("openAPIV3Schema:", 1)[1] if "openAPIV3Schema:" in doc else doc
        for jp, col_type in [
            ("ResourceDefinition", ".spec.layerStack", "string"),
            ("ResourceGroup", ".spec.selectFilter.layerStack", "string"),
            ("Snapshot", ".spec.nodes", "string"),
        ]:
            if name != jp:
                continue
            # Extract the immediate next type after the exact jsonPath.
            m = re.search(re.escape(jp) + r"\s*\n\s*type:\s*(\S+)", schema)
            print(f"{name}: jsonPath={jp} printerType={col_type} schemaType={m.group(1) if m else None}")
PY

echo "===== source usages of relevant fields ====="
rg -n "layerStack|selectFilter|selectFilter\.layerStack|nodes:" . --glob '!vendor/**' --glob '!dist/**' --glob '!build/**' | sed -n '1,220p'

Repository: cozystack/blockstor

Length of output: 18508

---

🌐 Web query:

Kubernetes CustomResourceDefinition additionalPrinterColumns JSONPath array result type mismatch printer-column omitted

💡 Result:

In Kubernetes CustomResourceDefinitions (CRDs), the additionalPrinterColumns configuration is designed to extract values from a custom resource to display in kubectl table output [1][2]. If a JSONPath expression targets an array, the observed behavior—where the column may appear empty, behave inconsistently, or fail to display multiple elements—is a known area of historical complexity in Kubernetes [3][4][5]. Key Technical Context: 1. Historical Limitation: Early versions of Kubernetes often restricted or inconsistently handled JSONPaths that returned array types in additionalPrinterColumns [3][5]. Historically, the system might have only evaluated and printed the first matching result, or failed to handle the array-to-string conversion expected for a flat table column [3][5]. 2. Improved Support: Subsequent updates, such as the changes introduced to improve support for more complex JSONPaths, have allowed for better handling of paths that return multiple values [4]. However, these are still subject to the requirement that the resulting data must be representable as a string within the table cell [1][6]. 3. Requirement for Simple Paths: While documentation often refers to "simple JSON paths" [1][6], "simple" in this context usually refers to the structure of the path itself rather than the complexity of the data it points to. If the JSONPath evaluates to an object or an array, kubectl must effectively serialize that data into a string format to display it in a table column [7]. Troubleshooting Steps: If you are experiencing a type mismatch or missing data: - Ensure the type defined in your CustomResourceColumnDefinition (e.g., string, integer) matches the expected output of your JSONPath [1][6]. If the path returns an array, defining the column type as string is generally required, and kubectl will typically join the array elements into a comma-separated string [7]. - Verify the JSONPath expression is valid. For example, using [*] can help ensure you are selecting all elements, but be mindful of the performance cost of complex path evaluations on the API server [4][5]. - If the output remains empty or missing, it may indicate that the API server is unable to resolve the path or that the resulting data type is incompatible with the declared column type. As a workaround, some users find it necessary to manage this data at the application layer by creating specific, scalar fields in the CRD status (e.g., a pre-computed string field) rather than relying on dynamic JSONPath extraction for complex data structures [4][5].

Citations:

• 1: https://kubernetes.io/docs/reference/kubernetes-api/apiextensions/custom-resource-definition-v1/
• 2: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/
• 3: additionalPrinterColumns for CRD's doesn't work in k8s 1.11 for columns with array data kubernetes/kubectl#517
• 4: Adding support for complex json paths in AdditionalPrinterColumns kubernetes/kubernetes#101205
• 5: https://groups.google.com/g/kubernetes-sig-api-machinery/c/GxXWe6T8DoM
• 6: https://www.bookstack.cn/read/kubernetes-1.30-en/a615272337ab930a.md
• 7: https://oneuptime.com/blog/post/2026-02-09-custom-resource-printer-columns/view

---

🌐 Web query:

Kubernetes API machinery printer columns JSONPath array result type string omit

💡 Result:

In Kubernetes Custom Resource Definition (CRD) additional printer columns, the support for arrays in JSONPath is limited and often yields unexpected results. When you use a JSONPath that targets an array (e.g.,.spec.items[*].name), the Kubernetes server-side printing logic typically returns only the first match rather than the full array [1][2]. This behavior is a known limitation that persists in many versions, where the system does not automatically join or format array elements for display in the column [1][2]. Regarding the "omit" or "empty" result type: 1. Handling Missing Data: If a field is missing, the column may appear empty [3][4]. While Kubernetes JSONPath generally allows accessing fields, you cannot explicitly configure an "omit" property in the CRD definition to hide the column if the value is missing. The visibility of a column is instead governed by its priority [5][6]. 2. Column Priority: You can use the priority field in the CustomResourceColumnDefinition [5]. Columns with a priority greater than 0 are considered "wide" columns and are only displayed when the user explicitly requests wider output (e.g., via -o wide), effectively omitting them from standard output if space is limited [5][6]. 3. Escaping: If you are encountering empty results for fields that you believe exist, ensure you are correctly escaping special characters (such as periods or slashes in labels) using backslashes (e.g.,.metadata.labels.key.name) [3][7][4]. Single quotes are sometimes required around the path for specific characters to be parsed correctly [3]. Because there is no native "omit if empty" feature for printer columns, if your goal is to handle complex array data or conditional visibility, the recommended workaround is often to define a specific status field in your API that pre-formats the data (e.g., as a single string) [2]. This allows you to point the printer column to a simple string field rather than an array, ensuring consistent output [2].

Citations:

• 1: additionalPrinterColumns for CRD's doesn't work in k8s 1.11 for columns with array data kubernetes/kubectl#517
• 2: https://groups.google.com/g/kubernetes-sig-api-machinery/c/GxXWe6T8DoM
• 3: https://stackoverflow.com/questions/74464069/json-path-semantics-different-in-kubectl-and-additional-printer-columns-in-custo
• 4: [BUG] UI - additionalPrinterColumns values empty when fields have periods or slashes in them rancher/dashboard#10777
• 5: https://docs.w3cub.com/kubernetes/reference/kubernetes-api/extend-resources/custom-resource-definition-v1/index
• 6: https://kubebuilder.io/reference/markers/crd
• 7: https://kubernetes.io/docs/reference/kubectl/jsonpath/

---

🌐 Web query:

Kubernetes API printer columns jsonpath arrays type string join comma kubectl get -o wide

💡 Result:

In Kubernetes, CustomResourceDefinition (CRD) additionalPrinterColumns do not support complex JSONPath templates or native array joining [1][2][3]. The jsonPath field in a CRD specification is restricted to simple JSONPath expressions that must start with a dot (.) and point to a specific field [2][3]. If you use a JSONPath that returns an array (e.g., .spec.items[*]), Kubernetes will typically display only the first element of that array rather than joining the elements with commas [1][4]. For scenarios where you need to display joined, formatted, or combined data (like X/Y or comma-separated lists) in a table format, you have the following options: 1. Use kubectl -o custom-columns: Unlike CRD printer columns, the kubectl custom-columns output format supports more flexible JSONPath templates [5]. You can command line the desired output without modifying the CRD: kubectl get -o custom-columns=NAME:.metadata.name,MY_COL:.spec.items[*] 2. Process in the application or controller: Since CRD printer columns are limited, the community standard approach is to create a specific, pre-formatted string field in your CRD's status block (e.g., status.itemsSummary). Your controller or operator then computes this joined string (e.g., using strings.Join()) and updates the object status [1]. You then point your CRD additionalPrinterColumns to that pre-computed string field [1]. 3. Use alternative output formats: If you need complex manipulation or formatting of lists for human-readable output, consider using kubectl -o json or kubectl -o yaml piped into tools like jq (e.g., kubectl get <resource> -o json | jq '.items[].spec.items | join(",")'), which natively supports advanced data transformation and string joining [6].

Citations:

• 1: https://groups.google.com/g/kubernetes-sig-api-machinery/c/GxXWe6T8DoM
• 2: https://groups.google.com/g/kubernetes-dev/c/hYLa75_q_fk
• 3: feature: support the (X/Y) display mode for the printcolumn field in CR resource kubernetes/kubernetes#127246
• 4: Human readable status from conditions for a CRD using additionalPrinterColumns kubernetes/kubernetes#67268
• 5: https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/get/customcolumn.go
• 6: https://kubernetes.io/docs/reference/kubectl/jsonpath/

---

Use a scalar summary field for populated array printer columns.

These paths target array-valued CRD fields, so the Layers/Nodes columns can render inconsistent or omitted values in kubectl table output. Replace them with a controller-provided string summary/status field, or remove the columns.

• config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml:24
• config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml:24
• config/crd/bases/blockstor.cozystack.io_snapshots.yaml:24

📍 Affects 3 files

• config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml#L24-L26 (this comment)
• config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml#L24-L26
• config/crd/bases/blockstor.cozystack.io_snapshots.yaml#L24-L26

🤖 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 `@config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml` around
lines 24 - 26, The CRD printer columns use array-valued fields for the
Layers/Nodes summaries. In
config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml:24-26,
config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml:24-26, and
config/crd/bases/blockstor.cozystack.io_snapshots.yaml:24-26, replace those
paths with the controller-provided scalar string summary/status field, or remove
the columns if no such field exists.

Source: MCP tools

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add a language tag to the fenced command block.

Use ```shell or ```console instead of an untyped fence so Markdown tooling can validate and render the example consistently.

🧰 Tools

🪛 markdownlint-cli2 (0.23.0)

[warning] 15-15: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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 `@docs/cli-design.md` around lines 15 - 21, Add a shell or console language tag
to the fenced command example in the CLI command documentation, changing the
opening fence from an untyped fence while leaving the command contents
unchanged.

Source: Linters/SAST tools

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Machine-readable output omits the computed max-volume-size / resource-group context.

The table branch reports ResourceGroup and MaxVolumeSize via view.SizeInfoRows(name, maxVolumeSizeKib(pools, ...), pools), but the machine branch (return machineOut(run, pools)) only serializes the raw candidate pools list — maxVolumeSizeKib is never computed on that path, and name never appears in the payload. For query-max-volume-size in particular, a script using -m gets an unrelated pool listing instead of the computed maximum size.

🐛 Proposed fix to include the computed size info in machine output

 	if run.Flags.Machine {
-		return machineOut(run, pools)
+		maxKib := maxVolumeSizeKib(pools, int(group.SelectFilter.PlaceCount))
+		return machineOut(run, view.SizeInfoRows(name, maxKib, pools))
 	}

📝 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

	func resourceGroupQuerySizeInfo(ctx context.Context, run *runContext) error {
	if len(run.Flags.Positionals) < 1 {
	return fmt.Errorf("%w: query needs a resource group", command.ErrUsage)
	}
	name := run.Flags.Positionals[0]
	group, err := run.Store.ResourceGroups().Get(ctx, name)
	if err != nil {
	return fmt.Errorf("get resource group %s: %w", name, err)
	}
	pools, err := candidatePools(ctx, run, &group)
	if err != nil {
	return err
	}
	if run.Flags.Machine {
	return machineOut(run, pools)
	}
	tbl := &metav1.Table{ColumnDefinitions: view.SizeInfoColumns()}
	tbl.Rows = view.SizeInfoRows(name, maxVolumeSizeKib(pools, int(group.SelectFilter.PlaceCount)), pools)
	return run.render(tbl)
	}
	func resourceGroupQuerySizeInfo(ctx context.Context, run *runContext) error {
	if len(run.Flags.Positionals) < 1 {
	return fmt.Errorf("%w: query needs a resource group", command.ErrUsage)
	}
	name := run.Flags.Positionals[0]
	group, err := run.Store.ResourceGroups().Get(ctx, name)
	if err != nil {
	return fmt.Errorf("get resource group %s: %w", name, err)
	}
	pools, err := candidatePools(ctx, run, &group)
	if err != nil {
	return err
	}
	if run.Flags.Machine {
	maxKib := maxVolumeSizeKib(pools, int(group.SelectFilter.PlaceCount))
	return machineOut(run, view.SizeInfoRows(name, maxKib, pools))
	}
	tbl := &metav1.Table{ColumnDefinitions: view.SizeInfoColumns()}
	tbl.Rows = view.SizeInfoRows(name, maxVolumeSizeKib(pools, int(group.SelectFilter.PlaceCount)), pools)
	return run.render(tbl)
	}

🤖 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 `@internal/cli/definition.go` around lines 186 - 211, Update
resourceGroupQuerySizeInfo to compute maxVolumeSizeKib using the selected group
and candidate pools before the machine-output branch, then pass machineOut the
same size-information payload represented by view.SizeInfoRows, including the
resource-group name, computed maximum size, and pools. Preserve the existing
table rendering behavior and ensure query-max-volume-size machine output reports
the computed size rather than raw pools alone.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check which canonical key downstream handlers expect for -o's long form(s).
rg -nP '"output-fmt"|"output-version"|Values\["output' --type=go

Repository: cozystack/blockstor

Length of output: 157

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== files =="
git ls-files | rg '(^|/)internal/cli/(flags|definition)\.go$|(^|/)cmd/|(^|/)internal/' | sed -n '1,120p'

echo
echo "== flags.go outline/stat =="
wc -l internal/cli/flags.go
ast-grep outline internal/cli/flags.go || true

echo
echo "== definition.go relevant =="
wc -l internal/cli/definition.go
rg -n 'Values|layer-list|--output|output-fmt|output-version|short|-o|-l' internal/cli/definition.go internal/cli/flags.go || true

echo
echo "== focused source around assign/resourceDefinitionModify == "
sed -n '1,130p' internal/cli/flags.go
echo "---"
sed -n '180,245p' internal/cli/flags.go
echo "---"
sed -n '40,90p' internal/cli/definition.go

Repository: cozystack/blockstor

Length of output: 13832

---

Fold short -l onto the canonical --layer-list key.

assign() stores -l <x> in Values["l"], but resourceDefinitionModify reads run.Flags.Values["layer-list"]; --layer-list works, while -l is accepted but has no effect. Add -l alongside --layer-list so both forms update the same handler key.

🤖 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 `@internal/cli/flags.go` around lines 76 - 107, The valueFlags table currently
treats -l separately from --layer-list, causing assign() to store the short form
under a different key than resourceDefinitionModify reads. Update the flag alias
configuration around valueFlags so -l is folded onto the canonical --layer-list
key, ensuring both forms populate Values["layer-list"] and trigger the same
behavior.

[coderabbitai]

🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win

Lost-update race: patchNodeFlags uses Get+Update instead of PatchNodeSpec.

This is the exact TOCTOU pattern the NodeStore.PatchNodeSpec API was built to close — its own doc comment calls out this precise scenario as Bug 205: a wholesale Update after a plain Get "silently dropped" concurrent edits like a "re-evacuate loop adding EVICTED, an operator stamping a different flag." nodeEvacuate/nodeRestore both go through this path, so a concurrent node update landing between the Get and Update here is silently clobbered instead of retried. internal/cli/resource.go's patchResource already uses the equivalent PatchResourceSpec for this reason — this function should match.

🔒 Proposed fix using PatchNodeSpec

 func patchNodeFlags(ctx context.Context, run *runContext, name, flag string, want bool) error {
-	node, err := run.Store.Nodes().Get(ctx, name)
-	if err != nil {
-		return fmt.Errorf("get node %s: %w", name, err)
-	}
-
-	node.Flags = setFlag(node.Flags, flag, want)
-
-	err = run.Store.Nodes().Update(ctx, &node)
-	if err != nil {
+	err := run.Store.Nodes().PatchNodeSpec(ctx, name, func(node *apiv1.Node) error {
+		node.Flags = setFlag(node.Flags, flag, want)
+		return nil
+	})
+	if err != nil {
 		return fmt.Errorf("update node %s: %w", name, err)
 	}
 
 	return nil
 }

📝 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

	func patchNodeFlags(ctx context.Context, run *runContext, name, flag string, want bool) error {
	node, err := run.Store.Nodes().Get(ctx, name)
	if err != nil {
	return fmt.Errorf("get node %s: %w", name, err)
	}
	node.Flags = setFlag(node.Flags, flag, want)
	err = run.Store.Nodes().Update(ctx, &node)
	if err != nil {
	return fmt.Errorf("update node %s: %w", name, err)
	}
	return nil
	}
	func patchNodeFlags(ctx context.Context, run *runContext, name, flag string, want bool) error {
	err := run.Store.Nodes().PatchNodeSpec(ctx, name, func(node *apiv1.Node) error {
	node.Flags = setFlag(node.Flags, flag, want)
	return nil
	})
	if err != nil {
	return fmt.Errorf("update node %s: %w", name, err)
	}
	return nil
	}

🤖 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 `@internal/cli/node.go` around lines 211 - 225, Update patchNodeFlags to use
NodeStore.PatchNodeSpec instead of the current Get-then-wholesale Update
sequence. Build the patch from the requested flag change using setFlag
semantics, preserve the existing node-not-found and update error context, and
ensure concurrent node flag edits are merged rather than overwritten.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

No rollback if StoragePools().Create (or a later device lookup) fails after devices are already stamped.

stampDevices runs first and persists AttachTo on each matched device; StoragePools().Create runs afterward. If create fails with a real error (not AlreadyExists), or if a later device in a multi-device call isn't found, the already-stamped device(s) are left pointing at a pool CR that was never created, with no automatic cleanup. The satellite will keep failing to reconcile that attach request until manually fixed.

The ordering is clearly a deliberate trade-off against the opposite failure (an orphaned, un-attachable pool) — but the current code accepts the orphaned-device failure mode silently. Consider a best-effort compensating cleanup on the failure path:

♻️ Suggested compensating cleanup

 	err = run.Store.StoragePools().Create(ctx, pool)
 	if err != nil && !isAlreadyExists(err) {
+		// Best-effort: don't leave devices pointing at a pool that was
+		// never created.
+		unstampDevices(ctx, run, node, devices)
 		return fmt.Errorf("create storage pool %s on %s: %w", poolName, node, err)
 	}

Also applies to: 149-180

🤖 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 `@internal/cli/physical.go` around lines 42 - 84, Update
physicalStorageCreateDevicePool and the device-stamping flow around stampDevices
to track which devices were successfully stamped, then perform best-effort
compensating cleanup if a later device lookup fails or StoragePools().Create
returns a non-AlreadyExists error. Cleanup must remove the pool attachment from
only those devices, preserve the original operation error, and avoid changing
the existing AlreadyExists behavior.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== files =="
git ls-files | rg '(^|/)internal/cli/resource\.go$|reconciler|migration|migrate|BlockstorMigratingFrom|UpToDate|Diskless' || true

echo "== resource.go outline =="
ast-grep outline internal/cli/resource.go --view compact || true

echo "== relevant resource.go lines =="
sed -n '120,270p' internal/cli/resource.go

echo "== constants related to migrating props =="
rg -n "BlockstorMigratingFrom|migratingFromProp|storage-pool|migrate-from|cmd.Migrate|migrateDisk|createMigrationTarget|migrate-disk" -S .

Repository: cozystack/blockstor

Length of output: 50376

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== controller migration contract lines =="
sed -n '1,120p' internal/controller/resource_migration_controller.go
sed -n '160,230p' internal/controller/resource_migration_controller.go

echo "== controller migration test lines =="
sed -n '1,90p' internal/controller/resource_migration_controller_test.go
sed -n '130,195p' internal/controller/resource_migration_controller_test.go

echo "== satellite migration/prune relevant lines =="
sed -n '5360,5485p' pkg/satellite/reconciler.go
sed -n '3345,3405p' pkg/satellite/reconciler.go
sed -n '3775,3820p' pkg/satellite/reconciler.go

echo "== validateMigrateSrc relevant lines =="
sed -n '392,455p' pkg/rest/resource_toggle_disk.go

echo "== CLI validateMigrateSrc and dispatch =="
sed -n '1,80p' internal/cli/resource.go
sed -n '45,75p' internal/cli/resource.go

echo "== read-only invariant probe: CLI/REST validation functions and migrator predicates =="
python3 - <<'PY'
from pathlib import Path

for path in [Path("internal/cli/resource.go"), Path("pkg/rest/resource_toggle_disk.go"), Path("internal/controller/resource_migration_controller.go"), Path("pkg/satellite/reconciler.go")]:
    text = path.read_text()
    print(f"\n--- {path} ---")
    for tag in [
        "func validateMigrateNodeSrc",
        "func validateMigrateSrc",
        "if src == dst",
        "if srcNode == dstNode",
        "BlockstorMigratingFrom",
        "MigratingFromProp",
        "UpToDate",
        "clearMigratingFrom",
        "delete source", "DELETE",
    ]:
        lines = [n for n,l in enumerate(text.splitlines(),1) if tag.lower() in l.lower()]
        print(f"{tag}: {lines[:30]}")
PY

Repository: cozystack/blockstor

Length of output: 30515

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== ResourceMigrationReconciler Reconcile source =="
sed -n '62,120p' internal/controller/resource_migration_controller.go

echo "== REST migration body tests relevant lines =="
sed -n '140,230p' pkg/rest/resource_toggle_disk_test.go
sed -n '1,140p' pkg/rest/migrate_disk_bodyless_test.go

echo "== parse delete source expression =="
python3 - <<'PY'
from pathlib import Path
text = Path("internal/controller/resource_migration_controller.go").read_text()
for name in ("delete src Resource via Kubernetes client.Delete", "deletes the source Resource CRD"):
    print(text.find(name))
print("contains `.Delete(` nearby source prune? ", ".Delete(" in text)
print(text.splitlines()[119:124])
PY

Repository: cozystack/blockstor

Length of output: 10405

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== exact ResourceMigrationReconciler Reconcile body =="
python3 - <<'PY'
from pathlib import Path
text = Path("internal/controller/resource_migration_controller.go").read_text().splitlines()
after = next(i for i,l in enumerate(text,1) if "stamped and clears" in l or '"resource", dst.Name, "src", srcNode)' in l)
for i in range(max(1, after-14), min(len(text), after+20)+1):
    print(f"{i:4}: {text[i-1]}")
PY

echo "== source prune test expectations =="
python3 - <<'PY'
from pathlib import Path
text = Path("internal/controller/resource_migration_controller_test.go").read_text()
for needle in ["src after dst UpToDate", "src must be deleted", "src Resource still present", "Get src"]:
    idx = text.find(needle)
    print(f"== {needle} @ {idx} ==")
    for line in text[max(0, idx-600): min(len(text), idx+800)].splitlines():
        print(line)
PY

echo "== REST self-node validation context =="
python3 - <<'PY'
from pathlib import Path
text = Path("pkg/rest/resource_toggle_disk.go").read_text().splitlines()
for i,l in enumerate(text,1):
    if '"blockstorMigrateDiskDuplicateSource"' in l or 'srcNode == dstNode' in l or 'get resource' in l.lower():
        lo=max(1,i-12); hi=min(len(text),i+22)
        print(f"\n--- {i} ---")
        for n in range(lo, hi+1):
            print(f"{n}: {text[n-1]}")
PY

Repository: cozystack/blockstor

Length of output: 6606

---

Reject self-referential --migrate-from before stamping the migration destination.

migrateDisk does not enforce src != dst, and the migration reconciler later deletes <rd>.<src-node> once BlockstorMigratingFrom is resolved. Passing the same node as both destination and --migrate-from can therefore mark a replica as its own source and prune its only diskful copy after the destination volumes are considered UpToDate.

🛡️ Proposed guard

 func migrateDisk(ctx context.Context, run *runContext, dst, rdName string) error {
 	src := run.Flags.Values["migrate-from"]
 	pool := run.Flags.Values["storage-pool"]
+
+	if src == dst {
+		return fmt.Errorf("%w: migrate-from source and destination are both %s", command.ErrUsage, dst)
+	}
 
 	srcRes, err := run.Store.Resources().Get(ctx, rdName, src)

📝 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

	func migrateDisk(ctx context.Context, run *runContext, dst, rdName string) error {
	src := run.Flags.Values["migrate-from"]
	pool := run.Flags.Values["storage-pool"]
	srcRes, err := run.Store.Resources().Get(ctx, rdName, src)
	if err != nil {
	return fmt.Errorf("migrate-disk: source replica %s on %s: %w", rdName, src, err)
	}
	if slices.Contains(srcRes.Flags, apiv1.ResourceFlagDiskless) {
	return fmt.Errorf("migrate-disk: source replica %s on %s has no diskful storage to migrate: %w",
	rdName, src, errNothingToMigrate)
	}
	if srcRes.State.InUse != nil && *srcRes.State.InUse {
	return fmt.Errorf("migrate-disk: source replica %s on %s is Primary InUse: %w",
	rdName, src, errSourceInUse)
	}
	_, err = run.Store.Resources().Get(ctx, rdName, dst)
	if isNotFound(err) {
	return createMigrationTarget(ctx, run, dst, rdName, pool, src)
	}
	if err != nil {
	return fmt.Errorf("get resource %s on %s: %w", rdName, dst, err)
	}
	return patchResource(ctx, run, dst, rdName, func(res *apiv1.Resource) {
	stampProp(res, storPoolNameProp, pool)
	stampProp(res, migratingFromProp, src)
	res.Flags = setFlag(res.Flags, apiv1.ResourceFlagDiskless, false)
	})
	}
	func migrateDisk(ctx context.Context, run *runContext, dst, rdName string) error {
	src := run.Flags.Values["migrate-from"]
	pool := run.Flags.Values["storage-pool"]
	if src == dst {
	return fmt.Errorf("%w: migrate-from source and destination are both %s", command.ErrUsage, dst)
	}
	srcRes, err := run.Store.Resources().Get(ctx, rdName, src)
	if err != nil {
	return fmt.Errorf("migrate-disk: source replica %s on %s: %w", rdName, src, err)
	}
	if slices.Contains(srcRes.Flags, apiv1.ResourceFlagDiskless) {
	return fmt.Errorf("migrate-disk: source replica %s on %s has no diskful storage to migrate: %w",
	rdName, src, errNothingToMigrate)
	}
	if srcRes.State.InUse != nil && *srcRes.State.InUse {
	return fmt.Errorf("migrate-disk: source replica %s on %s is Primary InUse: %w",
	rdName, src, errSourceInUse)
	}
	_, err = run.Store.Resources().Get(ctx, rdName, dst)
	if isNotFound(err) {
	return createMigrationTarget(ctx, run, dst, rdName, pool, src)
	}
	if err != nil {
	return fmt.Errorf("get resource %s on %s: %w", rdName, dst, err)
	}
	return patchResource(ctx, run, dst, rdName, func(res *apiv1.Resource) {
	stampProp(res, storPoolNameProp, pool)
	stampProp(res, migratingFromProp, src)
	res.Flags = setFlag(res.Flags, apiv1.ResourceFlagDiskless, false)
	})
	}

🤖 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 `@internal/cli/resource.go` around lines 147 - 180, Update migrateDisk to
reject a self-referential migration when the migrate-from value src equals the
destination dst, returning the existing migration validation error before
fetching or stamping the destination resource. Preserve normal source validation
and migration behavior when src and dst differ.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Lost-update race between setProperty's GET and objectProps.set's GET.

setProperty reads the bag once (line 73) to compute the new map, then hands that map to accessor.set, which re-fetches the object (line 155) purely to get current state/resourceVersion but then unconditionally overwrites its bag with the map computed from the earlier read (line 160). Any key another writer added/removed on that bag between the two GETs is silently lost — the final Update never reflects the second, fresher read's props.

This is the same class of bug the store package works hard to defend against elsewhere (see CreateAutoNumbered's conflict-retry + live re-verification for volume-definition numbering), but here there's no retry-on-conflict at all: two concurrent set-property/delete-property calls on the same object (a realistic scenario for automation/runbooks) can silently clobber each other's keys.

A fix would route the single-key delta (add/delete) through accessor.set itself, so it mutates the bag returned by its own fresh GET (ideally with conflict-retry), rather than accepting a fully pre-computed map from a stale read.

♻️ Sketch of one approach

 type propertyAccessor struct {
 	args int
 	get  func(context.Context, store.Store, []string) (map[string]string, error)
-	set  func(context.Context, store.Store, []string, map[string]string) error
+	// apply mutates the object's own freshly-read bag in place (add/delete a
+	// single key) and persists it, closing the race window between read and write.
+	apply func(context.Context, store.Store, []string, func(map[string]string)) error
 }

Then objectProps's set/apply would do a single GET, mutate *bag(&obj) via the callback, and update — with setProperty/deleteProperty passing the add/delete closure instead of a precomputed map.

Also applies to: 137-170

🤖 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 `@internal/cli/write.go` around lines 57 - 90, Eliminate the stale read/update
window between setProperty and objectProps.set by changing the setter contract
to accept a mutation callback or single-key delta instead of a precomputed
property map. Update setProperty to pass an add/delete operation, and have
objectProps.set/apply perform the fresh GET, mutate the retrieved bag, and
update it, preserving deletion for empty values; add conflict retry if supported
by the existing store patterns.

[IvanHunters]

Verdict: NOT LGTM

Build and go test ./... are green, but several behavioral defects reach paying clusters. 5 blockers + 4 minor.

Blockers

1. Size bounds bypassed on create/spawn + unchecked int64 overflow — internal/cli/write_more.go.
   checkResize (floor 4 MiB / ceiling 16 TiB) is called only from set-size (:252). volume-definition create (:156) and resource-group spawn-resources (:258) write SizeKib with no bounds check, and ParseSize multiplies value*multiplier unchecked — ParseSize("17179869184T") = (0, nil). So volume-definition create rd 17179869184T stores sizeKib: 0. Per the code's own comment the satellite then loops on drbdadm create-md forever — a silent hang on legal input, with no Event/Ready=False. No server-side backstop exists (CRD sizeKib has no minimum, no CEL, no admission webhook). write_more_test.go:54 even pins a sub-floor 1024K create as success. Fix: enforce the floor/ceiling (and an overflow guard) on create and spawn; correct the test.

2. Flags parsed but never consumed, silently wrong output — internal/cli/flags.go, handlers.go.
   --storage-pools, -o/--output-fmt/--output-version, --limit, --controllers, -p/--pastable have zero readers. r l -o json prints the human table with exit 0; sp l --storage-pools X does not filter. A script doing r l -o json | jq gets malformed input with no error. Fix: either wire these flags to behavior or reject them as unsupported.

3. --faulty misses connection failures — internal/cli/view/resource.go:265.
   isFaulty inspects only volume DiskState, never LayerObject.Drbd.Connections. A replica with local disk UpToDate but a StandAlone/NetworkFailure peer (split-brain) is dropped by --faulty, contradicting the troubleshooting runbooks. Fix: treat a non-Connected DRBD connection as faulty.

4. --faulty ignored in machine mode — internal/cli/handlers.go:234.
   The -m branch serializes the set filtered only by node/resource; FaultyOnly is applied only on the human render path. r l --faulty -m returns ALL replicas. Fix: apply the faulty filter before machine serialization.

5. Multi-line cell breaks the box table and the awk -F'|' contract — internal/cli/view/views.go:262, table/table.go.
   selectFilterCell joins parts with \n and the renderer writes them verbatim, while table.go's docstring declares the pipe layout a parsing contract. Any resource-group with a StoragePool/LayerStack renders a row split mid-cell. Fix: render multi-value cells without embedded newlines (or escape them).

Minor

6. ParseSize rejects 10GiB/10Gi/10GB despite the comment promising it tolerates them; the iB trim is dead code (the switch keys on the last byte first). write_more.go:67-84.
7. query-size-info overestimates the max placeable size: no per-node pool dedup, does not exclude PoolMissing, ignores SelectFilter.StoragePoolList, diverges from the real placer on all three. definition.go:215.
8. node delete-property n1 key oops (extra positional) silently SETS key=oops instead of deleting. props.go:37 to write.go:69.
9. Bool flag with inline value: --force=false enables Force (opposite of intent); -p=secret drops the value. flags.go:130.

Note

The constant-time passphrase compare uses crypto/subtle correctly, but returns 0 immediately on a length mismatch (passphrase length leaks) and runs client-side after the full Secret was already read via the caller's RBAC, so the timing threat model in the comment does not apply here.

[coderabbitai]

Actionable comments posted: 3

🤖 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 `@internal/cli/review_fixes_test.go`:
- Around line 103-125: Extend TestNoCellBreaksTheRowLayout with a direct
table-renderer case containing a cell value such as “before\nafter”. Render the
table and assert the newline-containing value is sanitized into a single table
row, while preserving the existing row-layout validation.
- Around line 229-232: Strengthen the inline boolean coverage in the test around
app.Run: assert that --force=false is accepted and produces the expected
domain/non-usage failure rather than merely any non-zero exit, then add a
separate --force=true invocation that succeeds. Keep the existing newApp setup
and command arguments, changing only the assertions needed to distinguish parsed
false from invalid usage.

In `@internal/cli/table/table.go`:
- Around line 115-129: Update Options.line to construct pastable rows directly
from the cells and widths instead of post-processing the bordered output, so
literal " | " sequences within headers or cell values remain unchanged. Preserve
the existing alignment, trimming, color handling, and trailing-newline behavior
while removing the separator-based ReplaceAll transformation.

🪄 Autofix (Beta)

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: 9672ba0d-12c4-42f1-a703-4a2c94d83dda

📥 Commits

Reviewing files that changed from the base of the PR and between e20b56f and 7ec29b1.

📒 Files selected for processing (13)

• internal/cli/app.go
• internal/cli/definition.go
• internal/cli/encryption.go
• internal/cli/flags.go
• internal/cli/handlers.go
• internal/cli/place.go
• internal/cli/props.go
• internal/cli/review_fixes_test.go
• internal/cli/table/table.go
• internal/cli/view/resource.go
• internal/cli/view/views.go
• internal/cli/write_more.go
• internal/cli/write_more_test.go

🚧 Files skipped from review as they are similar to previous changes (11)

• internal/cli/app.go
• internal/cli/handlers.go
• internal/cli/view/resource.go
• internal/cli/definition.go
• internal/cli/encryption.go
• internal/cli/write_more_test.go
• internal/cli/write_more.go
• internal/cli/props.go
• internal/cli/place.go
• internal/cli/flags.go
• internal/cli/view/views.go

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Exercise an actual newline-containing cell.

Lines 107-112 seed only newline-free values, so this test can pass without validating newline sanitization. Add a direct table-renderer case with a cell such as "before\nafter" and assert it produces one table row.

🤖 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 `@internal/cli/review_fixes_test.go` around lines 103 - 125, Extend
TestNoCellBreaksTheRowLayout with a direct table-renderer case containing a cell
value such as “before\nafter”. Render the table and assert the
newline-containing value is sanitized into a single table row, while preserving
the existing row-layout validation.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Distinguish false from an invalid inline flag.

At Line 230, any non-zero exit passes—including rejection of --force=false as invalid usage. Assert the expected non-usage/domain failure and add a --force=true case that succeeds, so the test proves inline booleans are parsed rather than rejected.

🤖 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 `@internal/cli/review_fixes_test.go` around lines 229 - 232, Strengthen the
inline boolean coverage in the test around app.Run: assert that --force=false is
accepted and produces the expected domain/non-usage failure rather than merely
any non-zero exit, then add a separate --force=true invocation that succeeds.
Keep the existing newApp setup and command arguments, changing only the
assertions needed to distinguish parsed false from invalid usage.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Preserve cell values when rendering pastable rows.

At Line 125, ReplaceAll(" | ", " ") also rewrites literal | within a header or cell value. Build the bare row directly instead of post-processing the bordered representation.

Proposed fix

 func (o Options) line(cells, headers []string, widths []int, painted map[string]struct{}) string {
-	rendered := line(cells, headers, widths, painted, o.Color)
-	if !o.Pastable {
-		return rendered
-	}
-
-	bare := strings.TrimPrefix(rendered, "| ")
-	bare = strings.ReplaceAll(bare, " | ", "  ")
-	bare = strings.TrimSuffix(bare, " |\n")
-
-	return strings.TrimRight(bare, " ") + "\n"
+	if !o.Pastable {
+		return line(cells, headers, widths, painted, o.Color)
+	}
+
+	var bare strings.Builder
+	for i, cell := range cells {
+		if i > 0 {
+			bare.WriteString("  ")
+		}
+		rendered := cell
+		if _, ok := painted[headers[i]]; ok {
+			rendered = paint.PaintState(cell, o.Color)
+		}
+		bare.WriteString(rendered)
+		bare.WriteString(strings.Repeat(" ", widths[i]-displayWidth(cell)))
+	}
+	return strings.TrimRight(bare.String(), " ") + "\n"
 }

📝 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

	// line renders one row, bordered or bare.
	func (o Options) line(cells, headers []string, widths []int, painted map[string]struct{}) string {
	rendered := line(cells, headers, widths, painted, o.Color)
	if !o.Pastable {
	return rendered
	}
	// Strip the leading "| " and the pipe separators, leaving the
	// alignment the widths already produced.
	bare := strings.TrimPrefix(rendered, "| ")
	bare = strings.ReplaceAll(bare, " | ", " ")
	bare = strings.TrimSuffix(bare, " |\n")
	return strings.TrimRight(bare, " ") + "\n"
	}
	// line renders one row, bordered or bare.
	func (o Options) line(cells, headers []string, widths []int, painted map[string]struct{}) string {
	if !o.Pastable {
	return line(cells, headers, widths, painted, o.Color)
	}
	var bare strings.Builder
	for i, cell := range cells {
	if i > 0 {
	bare.WriteString(" ")
	}
	rendered := cell
	if _, ok := painted[headers[i]]; ok {
	rendered = paint.PaintState(cell, o.Color)
	}
	bare.WriteString(rendered)
	bare.WriteString(strings.Repeat(" ", widths[i]-displayWidth(cell)))
	}
	return strings.TrimRight(bare.String(), " ") + "\n"
	}

🤖 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 `@internal/cli/table/table.go` around lines 115 - 129, Update Options.line to
construct pastable rows directly from the cells and widths instead of
post-processing the bordered output, so literal " | " sequences within headers
or cell values remain unchanged. Preserve the existing alignment, trimming,
color handling, and trailing-newline behavior while removing the separator-based
ReplaceAll transformation.

[Andrei Kvapil (kvaps)]

Thanks — all nine hold up against the code. Nothing here was a false positive, and two of them were pinned the wrong way round by my own tests. Fixed in 7ec29b1.

1. Size bounds and overflow. Confirmed on both halves. checkResize guarded only set-size; volume-definition create and rg spawn-resources wrote SizeKib unchecked. ParseSize("17179869184T") overflows int64 to exactly zero, and zero is the one value the satellite cannot fail on — per its own comment it loops on drbdadm create-md forever. I checked for a server-side backstop and there is none: no minimum on the CRD field, no CEL rule, no webhook. Bounds now apply on every path that writes a size, and the multiplication is checked rather than assumed. write_more_test.go did pin a sub-floor 1024K create as success; that case is gone and replaced with floor, ceiling and overflow rejections.

2. Flags parsed but never consumed. Confirmed — all five had zero readers. --storage-pools now filters (it has 46 call sites in the harness, so this was live), --limit caps, --pastable renders without the box, -o/--output-fmt either selects the machine envelope or is rejected, and --output-version rejects anything but v1. --controllers I kept accepting, because the harness wrapper passes it, but it now prints a notice: it names a REST endpoint this client does not use, and pointing it at one cluster while the kubeconfig names another must not silently read the other one.

3. --faulty and connection state. Confirmed. IsFaulty now also treats a non-Connected DRBD peer as faulty, so the UpToDate-disk-plus-StandAlone-peer case the troubleshooting runbooks send operators to find is no longer dropped.

4. --faulty in machine mode. Confirmed. The filter moved from the render path into the keep predicate, so -m narrows the same way the table does.

5. Multi-line cell. Confirmed — selectFilterCell joined with \n and the renderer wrote it verbatim, splitting the row mid-cell. Now joined with ; , and there is a test asserting every emitted line starts with a border character.

6. Confirmed, including the dead iB trim: the switch keyed on the last byte before the trim ran, so 10GiB could never reach it. Suffix stripping now happens first and 10GiB / 10Gi / 10GB parse.

7. Confirmed on all three counts. The query now dedups candidates per node (a node with three eligible pools still hosts one replica), skips PoolMissing, and honours SelectFilter.StoragePoolList. It remains deliberately more conservative than the controller — the thin-pool oversubscription policy lives in pkg/rest and is not reusable — which is documented in docs/cli-design.md.

8. Confirmed. delete-property now truncates the positionals to the key before delegating, so a stray trailing argument cannot turn a delete into a set of the key being removed.

9. Confirmed both ways. A value-less flag given an inline value now parses it as a boolean (--force=false disables) or rejects it; -p=secret is a usage error rather than a silently dropped passphrase. The recognise-and-apply switches were merged into one, since listing the names twice is how a flag ends up recognised but never acted on.

On the note: you are right and the comment was overclaiming. This runs client-side after the caller's own RBAC already let them read the Secret, so there is no remote attacker to time, and ConstantTimeCompare leaks the length regardless. I kept the constant-time compare — it costs nothing — but rewrote the comment to say what is actually true and to point at the controller's REST path as the place where the property matters.

Each finding has a regression test in internal/cli/review_fixes_test.go, plus the corrected size cases in write_more_test.go.

[IvanHunters]

Verdict

NOT LGTM — builds clean and all tests pass, but a set of behavioural defects in the CLI (empty-passphrase acceptance, partial-write-then-blocked-retry, exit-code and machine-output gaps) need addressing. No cluster-state surface (no charts/migrations/RBAC/CRD schema changes), so upgrade/fresh-install phases are N/A.

Findings

[MAJOR] internal/cli/encryption.go:44, empty passphrase is accepted silently
encryptionPassphrase returns a positional without checking for emptiness. encryption create-passphrase "" (or -p "", since -p parses as the boolean --pastable and "" falls through to positionals) writes an empty master key to the Secret and exits 0. passphrase.Read then returns "" for both a missing Secret and an empty value, so enter-passphrase <real> reports "no passphrase; create one first" while create-passphrase <real> reports "already set … mismatch" — two contradictory diagnoses with no CLI path out (manual Secret deletion required), and on a fresh cluster this weakens volume encryption to an empty key. Reject an empty passphrase as command.ErrUsage.

[MAJOR] internal/cli/place.go:253, spawn-resources creates the ResourceDefinition before validating sizes
spawnDefinition runs before the ParseSize/checkVolumeSize loop, and volumes are created one at a time. rg spawn grp pvc-x 32X creates pvc-x, then errors on the bad size; the corrected retry rg spawn grp pvc-x 32M fails in spawnDefinition (a plain non-idempotent Create) with "already exists". A size typo leaves an orphan definition/partial volumes and blocks the natural retry until a manual delete. This contradicts the validate-before-write discipline the PR itself applies in snapshotRestoreVolumeDefinition. Validate all sizes before the first write.

[MINOR] internal/cli/handlers.go applyLimit, malformed/negative --limit is swallowed (fail-open)
--limit banana returns the full list with exit 0 and no diagnostic; --limit 0 returns zero rows (inverting the usual "0 == unlimited"). Every other numeric flag wraps command.ErrUsage (exit 2). Validate --limit at parse time and decide --limit 0 explicitly; add a test for the malformed case.

[MINOR] internal/cli/help.go:31, per-command --help exits 2
isHelpRequest inspects only argv[0], so blockstor r l --help is rejected as "unknown flag" with exit 2. The upstream argparse client prints per-command help with exit 0.

[MINOR] internal/cli/app.go, color.ParseMode runs after StoreFor
With an unavailable kubeconfig, r l --color=bogus exits 10 ("load kubeconfig") instead of 2, so the same class of client-side error is classified differently depending on cluster reachability, and a known-invalid invocation still opens a cluster connection. Move ParseMode before StoreFor.

[MINOR] internal/cli/definition.go:204, query-size-info -m drops the computed max size
The machine branch emits only pools; the computed maxVolumeSizeKib — the whole point of the command, and the table's headline column — exists only in the table branch. -m consumers cannot obtain it.

[MINOR] internal/cli/pool.go volumeGroupList, vg list -m drops the parent resource-group
Machine output is a flattened []VolumeGroup with no parent-group name; across two or more groups the rows are ambiguous. The table has a ResourceGroup column, the JSON does not.

[MINOR] internal/cli/drbdopts.go applyDRBDFlags, contradictory set/unset is nondeterministic
Iterating flags.Values (a map), rd drbd-options pvc-x --max-buffers=8000 --unset-max-buffers resolves to set or delete depending on map iteration order. Reject the contradiction or define precedence.

[MINOR] internal/cli/view/resource.go:99, sync percentage is dead in production
stateCell prints SyncTarget(NN%) only when VolumeSizesKib is populated, but the only production caller (handlers.go:77) never populates it — only the unit test does (resource_test.go:161). During resync the operator sees a bare SyncTarget, though docs/cli-design.md promises the percentage and color.normalise deliberately strips (NN%). The test is also vacuous coverage for a path production never takes. Populate VolumeSizesKib in the resource list handler or drop the feature and the doc claim.

[MINOR] internal/cli/output/machine.go:46, MachineSingle is dead code
Zero callers; every -m path goes through MachineList (double-nested [[...]]). The godoc asserts singletons are emitted flat, but no verb does so and no test covers it. Wire the intended verbs to it with a test, or drop it and correct the doc.

[MINOR] internal/cli/snapshot.go snapshotCreateMultiple, partial batch write
Snapshots are created one at a time with GroupSize = len(pairs); a failure on the Nth leaves a group whose members are fewer than its declared GroupSize. (Controller-side consequence under a suspend-io barrier not verified here.)

Caveats

• Exit-code model is internally consistent (usage/parse maps to 2, everything else to 10) but its upstream parity is unverified: semantic refusals (shrink-without---force, size-out-of-bounds, passphrase mismatch, snapshot rollback) return 10, not 2. If the upstream client returns 2 for any of them, a script branching on the code misclassifies a permanent client-side rejection as a retryable API failure. Pin these codes with tests.
• Hermetic review: no live cluster contacted. This PR has no cluster-state surface (no charts, migrations, RBAC, CRD schema/storage changes; printer columns are additive), so there is no upgrade/fresh-install path to exercise.

Recommended follow-ups

• Run the tests/e2e/cli-matrix suite pointed at blockstor instead of the python client (the author's stated acceptance criterion). It is the only layer that can confirm real-cluster exit codes, machine-output jq paths, and server-side table parity.