Merge dev: KV API, card verification, test suite, and CI/CD

[Iktahana]

Summary

• KV CRUD API: Full REST endpoints for get/set/delete/head with batch operations and namespace/collection management
• Card Verification System: MVP card verification with MongoDB backend, vguang device integration, and POST-based API redesign
• Database Backends: BBolt, MongoDB, and Redis adapters implementing unified kv.KV interface with factory pattern
• Comprehensive Test Suite: Table-driven tests across all packages (config 100%, kv 100%, models 100%, redis 88%, bbolt 86%, handlers 78%)
• CI/CD Pipeline: GitHub Actions for linting, testing, codecov integration, and Docker image build/release workflow
• Documentation: API specification (OpenAPI 3.0), architecture docs with Mermaid diagrams, quickstart guide, and project management plan
• AI Rules Consolidation: Replaced 9 verbose rule files with 4 concise modular guides + CLAUDE.md

Test plan

• go build ./... passes
• go test ./... all 10 packages pass
• go vet ./... no issues
• go test -race ./... no data races
• CI pipeline runs successfully on PR

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Batch set/delete endpoints for bulk KV operations; standard KV CRUD and existence endpoints; card verification API with device authorization and time-based validity; BBolt added as a supported backend.
• Documentation
  • Full API specification, quickstart, examples, architecture, MVP and operational guides, and project/phase docs.
• Tests
  • Expanded unit and integration tests covering KV backends, handlers, models, and services.
• Chores
  • CI/release workflows, lint/coverage configs, env sample consolidated for multi-backend usage, and updated ignore rules.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a multi-backend KV service (BBolt/MongoDB/Redis) with HTTP handlers (CRUD, batch, namespace), MongoDB-backed card verification service, extensive tests and mocks, large documentation and CI/CD configs, plus configuration and linting setups. No breaking API removals; several new exported types and services introduced.

Changes

Cohort / File(s)	Summary
Docs & Governance .ai-rules/01-patterns.md, .ai-rules/02-testing.md, .ai-rules/03-guidelines.md, .ai-rules/important.md, CLAUDE.md, docs/... (many)	Adds design patterns, testing guidelines, development/security guidelines, architecture, API spec, quickstart, examples, project plans, phase report, MVP card verification docs and other documentation artifacts.
CI, Lint & Coverage .github/workflows/ci.yml, .github/workflows/release.yml, .golangci.yml, .testcoverage.yml, codecov.yml	Introduces CI workflow (lint/test/build), refines release flow (build artifact + separate GHCR push), adds golangci-lint config, coverage config and Codecov settings.
Env / Repo config .env.example, .gitignore, .clinerules	Reworks environment scaffold to support DATABASE (bbolt/mongodb/redis), adds backend-specific envs, updates .gitignore and project quick-reference.
Module deps go.mod	Adds/testing deps (testify, miniredis, godotenv, etc.), adjusts indirect entries.
Config internal/config/config.go, internal/config/config_test.go	Adds BackendBBolt constant and comprehensive LoadConfig tests validating backend selection and env parsing.
KV interfaces & factory tests internal/kv/kv_test.go, internal/database/factory_test.go	Adds NormalizeNamespace tests, error constant checks, and factory initialization tests across backends including missing-URI/error cases.
BBolt backend internal/database/bbolt/bbolt.go, internal/database/bbolt/bbolt_test.go	Stylistic octal literal changes, improved Ping/Close error handling; wide test coverage for BBolt CRUD, isolation, lifecycle.
Redis backend internal/database/redis/redis.go, internal/database/redis/redis_test.go	Minor stylistic tweaks; comprehensive Miniredis-backed tests covering CRUD, isolation and error cases.
MongoDB backend & mocks internal/database/mongodb/mongodb.go, internal/database/mongodb/*, internal/testing/mocks/mongodb_mock.go	Adds NewMongoDBKV and GetClient, changes ensureIndex to return error (best-effort usage), extensive MongoDB KV tests (scaffold + mock-based CRUD), and an in-memory MockClient/collections for tests.
Handlers — KV core internal/handlers/kv.go, internal/handlers/kv_test.go	Adds Get/Set/Delete/Head HTTP handlers, request/response types, JSON marshal helpers, and MockKV with full unit tests.
Handlers — Batch & List internal/handlers/batch.go, internal/handlers/batch_test.go	Adds BatchSet and BatchDelete handlers with per-operation aggregation (limit handling, JSON encoding) and ListKeys scaffolding (NOT_IMPLEMENTED); tests for batch flows and utilities.
Handlers — Namespace internal/handlers/namespace.go, internal/handlers/namespace_test.go	Adds namespace endpoints (list/collections/delete/get-info) with validation; most list/delete operations return 501 NOT_IMPLEMENTED; tests cover routing and error cases.
Handlers — Card verification internal/handlers/card.go, internal/handlers/card_test.go	Adds CardVerificationHandler and CardVerificationVguangHandler, parsing utilities, error→status mapping, and tests for header/body validation and utilities.
Handlers — Config & Health internal/handlers/config.go, internal/handlers/health.go, internal/handlers/handlers_test.go	Introduces exported Config *config.Config for handlers, health and root endpoints unchanged behaviorally; tests for health/root responses and version override.
Models & Services internal/models/card.go, internal/models/card_test.go, internal/services/card_service.go, internal/services/*_test.go	Adds Device and Card models (BSON tags), Card methods IsValid/HasDevice, new CardService (NewCardService, VerifyCard) with explicit error variables; extensive unit and integration-style tests using mock Mongo client.
Server entrypoint cmd/server/main.go, cmd/server/main_test.go	Initializes CardService when Mongo backend detected (type assertion), routes card endpoints conditionally, updates setupRoutes signature, improves startup/shutdown logging and KV lifecycle; adds scaffolded server tests.
Examples & utilities internal/database/example.go	Improves example error handling and deferred close logging; adds build/coverage tags.

Sequence Diagram(s)

sequenceDiagram
    actor Client
    participant Handler as HTTP Handler
    participant KV as KV Store
    participant Backend as Backend (BBolt/Redis/MongoDB)

    Client->>Handler: GET /api/v1/kv/:ns/:col/:key
    Handler->>Handler: Validate & normalize params
    Handler->>KV: Get(ctx, ns, col, key)
    KV->>Backend: Query storage
    Backend-->>KV: []byte or ErrKeyNotFound
    KV-->>Handler: value or error
    alt success
        Handler-->>Client: 200 OK (KVResponse)
    else not found
        Handler-->>Client: 404 Not Found (ErrorResponse)
    else invalid params
        Handler-->>Client: 400 Bad Request (ErrorResponse)
    end

Loading

sequenceDiagram
    actor Client
    participant Batch as Batch Handler
    participant KV as KV Store
    participant Backend as Backend (BBolt/Redis/MongoDB)

    Client->>Batch: POST /api/v1/kv/batch {ops[]}
    Batch->>Batch: Parse & validate (<=1000)
    loop per operation
        Batch->>KV: Set/Delete(ctx, ns, col, key)
        KV->>Backend: Execute op
        Backend-->>KV: result or error
        KV-->>Batch: result
        Batch->>Batch: aggregate results
    end
    Batch-->>Client: 200 OK (Batch response with counts, timestamp)

Loading

sequenceDiagram
    actor Client
    participant Handler as Card HTTP Handler
    participant Service as CardService
    participant Mongo as MongoDB

    Client->>Handler: POST /api/v1/namespaces/:ns (X-Device-SN, body cardNumber)
    Handler->>Service: VerifyCard(ctx, ns, deviceSN, cardNumber)
    Service->>Mongo: Find device by SN
    alt device not found
        Mongo-->>Service: nil
        Service-->>Handler: ErrDeviceNotFound
    else device found
        Mongo-->>Service: device
        Service->>Mongo: Find card by number
        alt card not found
            Mongo-->>Service: nil
            Service-->>Handler: ErrCardNotFound
        else card found
            Service->>Service: check authorization & time validity (±60s)
            alt authorized & valid
                Service-->>Handler: success
            else failure
                Service-->>Handler: specific error
            end
        end
    end
    alt success
        Handler-->>Client: 204 No Content
    else error
        Handler-->>Client: mapped HTTP status (400/403/404)
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Poem

🐰 I nibbled through docs, tests, and code so neat,
I stitched BBolt, Mongo, Redis—what a feat!
Handlers hop, batches bound, card checks take flight,
Mocks and CI hum softly into the night.
A rabbit's tiny cheer for this KV delight.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 48.03% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Merge dev: KV API, card verification, test suite, and CI/CD' clearly and concisely summarizes the main changes: KV API implementation, card verification system, comprehensive test suite, and CI/CD pipeline infrastructure.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch dev

---

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

[coderabbitai]

Note

Docstrings generation - SUCCESS
Generated docstrings for this pull request at #8

[coderabbitai]

Actionable comments posted: 5

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

🤖 Fix all issues with AI agents

In @.github/workflows/release.yml:
- Around line 68-79: The current workflow runs docker save against images that
were not loaded into the daemon because the build action used outputs:
type=docker,dest=/tmp/image.tar; replace the failing step that runs "docker save
${{ env.IMAGE_NAME }}:... | gzip > image.tar.gz" with a step that
gzip-compresses the tar produced by the build action (e.g. run: gzip -c
/tmp/image.tar > /tmp/image.tar.gz or tar -czf /tmp/image.tar.gz -C /tmp
image.tar), and update the Upload image artifact step to point to
/tmp/image.tar.gz (or the chosen compressed path) instead of image.tar.gz so you
upload the build-push-action's exported tarball rather than trying to save from
the local daemon.

In @.gitignore:
- Around line 27-29: The .gitignore currently only ignores the literal ".env"
which leaves other environment filename patterns vulnerable; update the ignore
rules to cover common variants by adding patterns such as ".env.*",
".env.*.local", ".env.local" and any environment-specific names you use (e.g.,
".env.production", ".env.development") so all those files are excluded from
commits; ensure you add the wildcarded entries (e.g., .env.*) rather than
individual filenames so new variants are automatically ignored.

In `@internal/handlers/batch.go`:
- Around line 298-303: The parseStringToInt function currently performs manual
digit accumulation in the loop and can overflow for very large inputs; replace
the custom parsing logic with strconv.Atoi (or strconv.ParseInt with bit-size
bounds) and propagate its error to handle invalid input and overflow, updating
any callers of parseStringToInt to expect the error return; ensure the function
signature still returns (int, error) and remove the manual result*10
accumulation and character checks in the loop.
- Around line 282-310: The parseStringToInt function duplicates standard library
behavior and misses handling for '+' prefixes and integer overflow; replace its
implementation by using strconv.Atoi inside parseStringToInt (or remove the
wrapper and call strconv.Atoi at call sites), ensure you import strconv, return
the error from strconv.Atoi directly so overflow and format errors are handled
correctly, and remove the manual digit-parsing logic in the parseStringToInt
function.

In `@internal/handlers/kv.go`:
- Around line 232-240: marshalJSON currently treats string values specially by
returning []byte(str) which produces non-JSON raw bytes and breaks
unmarshalJSON; remove the special-case for strings and always call
json.Marshal(value) inside marshalJSON so that strings are encoded with quotes
and all types are consistently JSON-encoded, keeping unmarshalJSON compatible
(update marshalJSON to use json.Marshal for every value and delete the string
branch).

🟡 Minor comments (21)

.ai-rules/important.md-25-27 (1)

25-27: ⚠️ Potential issue | 🟡 Minor

Add language specifier to fenced code block.

The code block lacks a language specifier. While this is a minor markdown lint issue, adding a specifier improves consistency and syntax highlighting.

📝 Proposed fix

-```
+```text
 type(scope): description

</details>

</blockquote></details>
<details>
<summary>internal/database/example.go-25-37 (1)</summary><blockquote>

`25-37`: _⚠️ Potential issue_ | _🟡 Minor_

**Potential double `Close()` on Ping failure.**

If `Ping` fails, `kv.Close()` is called at line 35, and then `log.Fatalf` will eventually trigger the deferred close at lines 25-29, calling `Close()` a second time. While `Close()` implementations should ideally be idempotent, this is worth verifying to avoid unexpected behavior or double-close errors.


<details>
<summary>🔧 Proposed fix to avoid double close</summary>

```diff
 	// Ping to check connection
 	if pingErr := kv.Ping(ctx); pingErr != nil {
-		_ = kv.Close()                                     //nolint:errcheck // Best effort close before exit
 		log.Fatalf("Failed to ping KV store: %v", pingErr) //nolint:gocritic // Example code intentionally exits
 	}

The deferred close will handle cleanup when log.Fatalf calls os.Exit. Alternatively, if immediate close before the fatal message is preferred, consider setting a flag to skip the deferred close.

docs/database-abstraction-layer.md-176-176 (1)

176-176: ⚠️ Potential issue | 🟡 Minor

Hyphenate the compound modifier.

Use “environment-variable-based selection” to match standard compound adjective form.

📝 Suggested edit

-**Advantage**: Switch backends without recompilation; environment variable based selection.
+**Advantage**: Switch backends without recompilation; environment-variable-based selection.

docs/database-abstraction-layer.md-118-131 (1)

118-131: ⚠️ Potential issue | 🟡 Minor

Add a language identifier to fenced blocks (MD040).

Several fenced blocks omit a language tag (e.g., this hierarchy block). Add text (or a specific language) to clear markdownlint warnings across the doc.

🛠️ Example fix (apply similarly to other blocks)

-```
+```text
 Namespace (logical isolation)
   ├── Collection 1 (data category)
   │   ├── Key 1 → Value (JSON bytes)
   │   ├── Key 2 → Value (JSON bytes)
   │   └── ...

docs/api-examples.md-120-126 (1)

120-126: ⚠️ Potential issue | 🟡 Minor

Unused import in Python example.

The json module is imported but not used in this example (line 122). The requests library handles JSON serialization via json=payload.

📝 Suggested fix

 import requests
-import json
 
 BASE_URL = "http://localhost:8080/api/v1"

internal/database/bbolt/bbolt.go-186-202 (1)

186-202: ⚠️ Potential issue | 🟡 Minor

Deferred close error is not propagated in Ping.

The error aggregation in the deferred Close() call doesn't actually affect the return value. The function returns testDB.View(...) directly on line 199, which doesn't include any close errors captured in the defer.

The err variable assigned in the defer (line 195) is a local variable that goes out of scope without being returned.

🐛 Proposed fix using named return

 // Ping checks if the connection is alive
-func (b *BBoltKV) Ping(ctx context.Context) error {
+func (b *BBoltKV) Ping(ctx context.Context) (err error) {
 	// Try to open a test database to verify the base directory is accessible
 	testDB, err := bbolt.Open(filepath.Join(b.baseDir, ".ping.db"), 0o600, nil)
 	if err != nil {
 		return errors.Join(kv.ErrConnectionFailed, err)
 	}
 	defer func() {
 		if closeErr := testDB.Close(); closeErr != nil {
 			err = errors.Join(err, closeErr)
 		}
 	}()
 
-	return testDB.View(func(tx *bbolt.Tx) error {
+	err = testDB.View(func(tx *bbolt.Tx) error {
 		return nil
 	})
+	return err
 }

docs/ARCHITECTURE.md-18-22 (1)

18-22: ⚠️ Potential issue | 🟡 Minor

Tighten wording: “API interface” is redundant.

Minor style fix for clarity.

Proposed fix

-Commander is a unified KV storage abstraction service that provides a single REST API interface for multiple database backends.
+Commander is a unified KV storage abstraction service that provides a single REST API for multiple database backends.

internal/models/card_test.go-234-251 (1)

234-251: ⚠️ Potential issue | 🟡 Minor

Remove duplicate case in case-sensitivity test.

The last subtest repeats the same lowercase input as the prior one, so it doesn’t add coverage.

Proposed fix

-	t.Run("different case no match uppercase", func(t *testing.T) {
-		assert.False(t, card.HasDevice("sn001"))
-	})
+	t.Run("different case no match mixed-case", func(t *testing.T) {
+		assert.False(t, card.HasDevice("Sn001"))
+	})

internal/handlers/handlers_test.go-135-142 (1)

135-142: ⚠️ Potential issue | 🟡 Minor

Handle the json.Unmarshal error.

The error from json.Unmarshal on line 137 is silently discarded. If unmarshalling fails, the subsequent assertion on response["version"] would fail with a confusing message rather than indicating the actual JSON parsing issue.

🔧 Proposed fix

 	// Parse response
 	var response map[string]interface{}
-	json.Unmarshal(w.Body.Bytes(), &response)
+	err := json.Unmarshal(w.Body.Bytes(), &response)
+	if err != nil {
+		t.Fatalf("Failed to parse response: %v", err)
+	}

docs/PHASE1_COMPLETION.md-178-184 (1)

178-184: ⚠️ Potential issue | 🟡 Minor

Coverage section header mismatch.

The section header says "Good Coverage (70-79%)" but includes BatchSetHandler at 65.7% and BatchDeleteHandler at 58.6%, which fall below that range. These should either be moved to a separate section (e.g., "Moderate Coverage (50-69%)") or the header should be adjusted.

📝 Suggested fix

 🟡 **Good Coverage (70-79%)**
 - SetKVHandler (71.4%)
 - DeleteNamespaceHandler (71.4%)
 - DeleteCollectionHandler (75.0%)
+
+🟡 **Moderate Coverage (50-69%)**
 - BatchSetHandler (65.7%)
 - BatchDeleteHandler (58.6%)

docs/mvp-card-verification.md-82-102 (1)

82-102: ⚠️ Potential issue | 🟡 Minor

vguang example URLs are missing the /vguang suffix.

The vguang-m350 endpoint is documented as POST /api/v1/namespaces/:namespace/device/:device_name/vguang (line 65), but the example requests on lines 86 and 101 are missing the /vguang suffix:

• Line 86: http://localhost:8080/api/v1/namespaces/org_4e8fb2461d71963a/device/SN20250112001
• Line 101: Same issue

These should include /vguang at the end to match the documented endpoint.

📝 Suggested fix for line 86

 curl -X POST \
   -d "11110011" \
-  http://localhost:8080/api/v1/namespaces/org_4e8fb2461d71963a/device/SN20250112001
+  http://localhost:8080/api/v1/namespaces/org_4e8fb2461d71963a/device/SN20250112001/vguang

docs/api-specification.yaml-1-17 (1)

1-17: ⚠️ Potential issue | 🟡 Minor

Add security scheme definitions to the OpenAPI specification.

The specification lacks a global security field and securitySchemes definitions. For a production API, this should document authentication requirements (e.g., API keys, OAuth2, JWT).

Proposed addition to components section

 components:
+  securitySchemes:
+    ApiKeyAuth:
+      type: apiKey
+      in: header
+      name: X-API-Key
   schemas:
     RootResponse:

And at the top level:

 tags:
   - name: Health
+
+security:
+  - ApiKeyAuth: []

internal/services/card_service_integration_test.go-123-148 (1)

123-148: ⚠️ Potential issue | 🟡 Minor

String conversion produces non-printable control characters.

string(rune(i)) where i ranges from 0-3 produces control characters (NUL, SOH, STX, ETX) rather than readable identifiers. This makes debugging difficult and may cause unexpected behavior with string comparisons.

Proposed fix

 	t.Run("different device statuses", func(t *testing.T) {
 		mockClient := mocks.NewMockClient()
 
 		statuses := []string{"active", "inactive", "disabled", "pending"}
 		for i, status := range statuses {
-			sn := "SN-" + string(rune(i))
+			sn := fmt.Sprintf("SN-%d", i)
 			device := &models.Device{
-				ID:     "device-" + string(rune(i)),
+				ID:     fmt.Sprintf("device-%d", i),
 				SN:     sn,
 				Status: status,
 			}
 			mockClient.SetupDevice("default", device)
 		}

Note: You'll need to add "fmt" to the imports.

internal/services/card_service_integration_test.go-464-498 (1)

464-498: ⚠️ Potential issue | 🟡 Minor

Load test only creates 10 unique entries due to modulo operation.

The loop creates 100 iterations but string(rune('0'+i%10)) only produces 10 unique values ('0'-'9'). Combined with likely map-based storage in the mock, this may overwrite entries rather than creating 100 distinct items. The assertion len(devices) > 0 is too weak to verify load handling.

Proposed fix

 	t.Run("handle many devices", func(t *testing.T) {
 		mockClient := mocks.NewMockClient()
 
 		// Add 100 devices
 		for i := 0; i < 100; i++ {
 			device := &models.Device{
-				SN:     "SN-" + string(rune('0'+i%10)),
+				SN:     fmt.Sprintf("SN-%03d", i),
 				Status: "active",
 			}
 			mockClient.SetupDevice("default", device)
 		}
 
 		devices := mockClient.GetAllDevices("default")
-		assert.True(t, len(devices) > 0)
+		assert.Equal(t, 100, len(devices))
 	})

internal/handlers/kv.go-67-71 (1)

67-71: ⚠️ Potential issue | 🟡 Minor

Potential information disclosure in error messages.

The error message includes err.Error() which could expose internal details (e.g., connection strings, internal paths) to API consumers. Consider logging the full error server-side and returning a generic message to clients.

🛡️ Suggested fix for safer error handling

 			c.JSON(http.StatusInternalServerError, ErrorResponse{
-				Message: "failed to retrieve key: " + err.Error(),
+				Message: "failed to retrieve key",
 				Code:    "INTERNAL_ERROR",
 			})
+			// Log the actual error for debugging
+			// log.Printf("GetKVHandler error: %v", err)

internal/handlers/kv.go-176-184 (1)

176-184: ⚠️ Potential issue | 🟡 Minor

DeleteKVHandler doesn't distinguish between key not found and other errors.

Unlike GetKVHandler which returns 404 for ErrKeyNotFound, DeleteKVHandler returns 500 for all errors including when the key doesn't exist. This is inconsistent. Typically, DELETE operations should return 404 if the resource doesn't exist, or 204/200 for idempotent success.

🐛 Fix to handle ErrKeyNotFound appropriately

 		// Delete value from KV store
 		ctx := c.Request.Context()
 		if err := kvStore.Delete(ctx, namespace, collection, key); err != nil {
+			if errors.Is(err, kv.ErrKeyNotFound) {
+				c.JSON(http.StatusNotFound, ErrorResponse{
+					Message: "key not found",
+					Code:    "KEY_NOT_FOUND",
+				})
+				return
+			}
 			c.JSON(http.StatusInternalServerError, ErrorResponse{
-				Message: "failed to delete key: " + err.Error(),
+				Message: "failed to delete key",
 				Code:    "INTERNAL_ERROR",
 			})
 			return
 		}

internal/database/mongodb/mongodb_crud_test.go-280-291 (1)

280-291: ⚠️ Potential issue | 🟡 Minor

Test name contradicts assertion - not idempotent behavior.

The test is named "delete is idempotent operation" but asserts that the second delete returns an error (line 290). By definition, an idempotent operation should produce the same result regardless of how many times it's called. If delete returns an error on the second call, it's not idempotent.

Either rename the test to reflect actual behavior, or adjust the mock/assertion to test true idempotency.

📝 Suggested fix - rename to reflect actual behavior

-	t.Run("delete is idempotent operation", func(t *testing.T) {
+	t.Run("delete returns error for already deleted key", func(t *testing.T) {
 		mock := &mockMongoDBKV{mockClient: mocks.NewMockClient()}
 		ctx := context.Background()

 		// Set and delete
 		mock.Set(ctx, "default", "test", "key", []byte("value"))
 		mock.Delete(ctx, "default", "test", "key")

-		// Second delete should fail
+		// Second delete returns ErrKeyNotFound
 		err := mock.Delete(ctx, "default", "test", "key")
 		assert.Error(t, err)
+		assert.Equal(t, kv.ErrKeyNotFound, err)
 	})

internal/testing/mocks/mongodb_mock.go-13-27 (1)

13-27: ⚠️ Potential issue | 🟡 Minor

FindOne mock returns empty SingleResult ignoring stored documents and FindError.

The FindOne method always returns an empty &mongo.SingleResult{} regardless of what's in Documents or whether FindError is set. This makes the mock less useful for testing find operations. Tests using this mock for FindOne scenarios may not behave as expected.

Consider using the FindOneFunc hook on MockClient instead, or documenting that MockCollection.FindOne is a no-op placeholder.

internal/testing/mocks/mongodb_mock.go-66-79 (1)

66-79: ⚠️ Potential issue | 🟡 Minor

Collection method returns nil which will cause panics if used directly.

The Collection method initializes a MockCollection in the map but returns nil. Any test code that calls mockDB.Collection("name").FindOne(...) will panic with a nil pointer dereference. The comment explains the intent ("mocking at a higher level"), but this could be a footgun.

Consider either:

1. Documenting this limitation prominently, or
2. Returning a typed mock that implements the needed interface methods

internal/testing/mocks/mongodb_mock.go-81-110 (1)

81-110: ⚠️ Potential issue | 🟡 Minor

Same nil return issue on Database method.

MockClient.Database also returns nil after populating the Databases map, with the same potential for nil pointer panics if used directly. The dual storage (Databases map vs Collections map) could also be confusing - the Setup/Get helpers use Collections while Database() uses Databases.

internal/handlers/namespace.go-140-164 (1)

140-164: ⚠️ Potential issue | 🟡 Minor

Handler returns success but provides no actual namespace data.

This handler returns 200 OK with a message "Namespace information retrieved", but the kvStore is never queried. The response contains only the normalized namespace and timestamp, with empty/zero values for Collections, KeyCount, and Size. This may mislead API consumers into thinking they received actual namespace statistics.

Consider either:

1. Returning 501 Not Implemented like the other handlers, or
2. Actually querying the kvStore for namespace information, or
3. Clarifying in the response message that this is a stub (e.g., "Namespace validated, detailed info not available").

🔧 Suggested fix (option 3)

 		c.JSON(http.StatusOK, NamespaceInfoResponse{
-			Message:   "Namespace information retrieved",
+			Message:   "Namespace validated (detailed info not implemented for this backend)",
 			Namespace: namespace,
 			Timestamp: time.Now().UTC().Format(time.RFC3339),
 		})

🧹 Nitpick comments (32)

.testcoverage.yml (1)

8-19: Consider consolidating coverage exclusions.

Both override (setting threshold to 0) and exclude.paths list the same files. This redundancy may be intentional if the tool uses these sections differently, but if not, consolidating to one approach would improve maintainability.

.golangci.yml (1)

23-25: Consider enabling gosec or scoping exclusions instead of disabling globally.

Disabling gosec removes a key security signal; if this isn’t strictly internal-only, consider enabling it with targeted excludes.

.github/workflows/ci.yml (1)

24-25: Pin golangci-lint to a specific version (e.g., v2.8.0) instead of @latest for reproducible CI.

Using @latest makes linting non-deterministic across CI runs and can introduce unexpected breakage. Follow the same pattern already used for Go version (1.25.5) and pinned actions (checkout@v4, setup-go@v5).

internal/database/mongodb/mongodb.go (1)

64-68: Index creation on every Get operation may impact performance.

Calling ensureIndex on every Get (and Set) operation incurs an extra round-trip to MongoDB. While MongoDB's CreateOne is idempotent and relatively fast for existing indexes, this pattern adds latency to every operation.

Consider caching which collections have been indexed (e.g., using a sync.Map or a set protected by a mutex) to skip redundant index creation calls after the first successful attempt per collection.

♻️ Sketch of caching pattern

type MongoDBKV struct {
	client       *mongo.Client
	uri          string
	indexedColls sync.Map // key: "namespace:collection", value: struct{}{}
}

func (m *MongoDBKV) ensureIndexOnce(ctx context.Context, namespace, collection string, coll *mongo.Collection) {
	key := namespace + ":" + collection
	if _, loaded := m.indexedColls.LoadOrStore(key, struct{}{}); loaded {
		return // already indexed
	}
	_ = m.ensureIndex(ctx, coll) // best effort
}

internal/config/config.go (2)

46-61: Consider validating required configuration per backend type.

When BackendMongoDB or BackendRedis is selected, the corresponding URI (MongoURI or RedisURI) is required for the backend to function. Currently, the config loads without validating that these are set, which defers the error to runtime when the database connection is attempted.

Consider adding validation in LoadConfig or a separate Validate() error method to fail fast with a clear message if a backend is selected but its required configuration is missing.

♻️ Example validation method

func (c *Config) Validate() error {
	switch c.KV.BackendType {
	case BackendMongoDB:
		if c.KV.MongoURI == "" {
			return errors.New("MONGODB_URI required when DATABASE=mongodb")
		}
	case BackendRedis:
		if c.KV.RedisURI == "" {
			return errors.New("REDIS_URI required when DATABASE=redis")
		}
	}
	return nil
}

---

63-64: Hardcoded version string.

The Version is hardcoded to "dev". For release builds, consider injecting the version at build time using -ldflags:

go build -ldflags="-X commander/internal/config.version=1.0.0" ./cmd/server

This enables accurate version reporting in production without code changes.

cmd/server/main_test.go (1)

29-32: Avoid permanent skipped tests.

Consider converting this into a minimal unit test (or track as a TODO issue) so it doesn’t remain skipped indefinitely.

docs/PHASE1_COMPLETION.md (1)

27-31: Clarify "not-implemented" status.

Line 30 shows a checkmark (✅) for "List keys" but notes it's "not-implemented for now". This is slightly confusing—consider using a different indicator (e.g., 🟡 or a note) to distinguish between fully implemented and placeholder/stub endpoints.

.ai-rules/01-patterns.md (2)

79-92: Add language specifiers to code blocks.

The code blocks for API URL structure (lines 81-86, 89-92) are missing language specifiers. Adding a specifier improves rendering and consistency.

📝 Suggested fix

 ## API URL Structure

-```
+```text
 GET/POST/DELETE/HEAD /api/v1/kv/{namespace}/{collection}/{key}
 POST/DELETE          /api/v1/kv/batch
 GET                  /api/v1/namespaces
 GET                  /api/v1/namespaces/{namespace}/collections

Card verification (MongoDB only):
- +text
POST /api/v1/namespaces/{namespace}
POST /api/v1/namespaces/{namespace}/device/{device_name}/vguang

---

47-55: Consider improving the success message.

The success message "Successfully" (line 49) is grammatically incomplete. Consider using a more complete phrase like "Success" or "Operation completed successfully".

📝 Suggested fix

         // 5. Respond
         c.JSON(http.StatusOK, KVResponse{
-            Message:   "Successfully",
+            Message:   "Success",
             Namespace: namespace,
             Key:       key,

internal/handlers/batch_test.go (1)

67-81: Clarify test expectations for invalid operation handling.

The test expects HTTP 200 for a batch set with an empty key. This is reasonable for batch operations that return partial success, but the assertion on Line 99 (resp.SuccessCount+resp.FailureCount) doesn't explicitly verify that this operation was counted as a failure. Consider adding an assertion to validate resp.FailureCount >= 1 for this specific test case to ensure the handler correctly identifies the invalid operation.

internal/handlers/card_test.go (2)

16-31: Using uninitialized mongo.Client{} may cause issues in future tests.

The tests create a CardService with &mongo.Client{} (a zero-value client). While this works for testing HTTP-level validation (missing headers, empty body), it will panic or produce undefined behavior if any test actually invokes database operations. Consider introducing a proper mock or interface for CardService to enable more comprehensive testing without relying on MongoDB connectivity.

---

51-68: Test assertion could be more specific.

The test comment says "will fail verification due to no mock data" but the assertion only checks NotEqual(t, http.StatusBadRequest, w.Code). This doesn't verify what status code is actually returned. Consider asserting the expected error status code explicitly (e.g., 404 or 500) based on what the handler returns when the database call fails.

Proposed improvement

 	// Should return error status (no mock DB)
-	assert.NotEqual(t, http.StatusBadRequest, w.Code)
+	// With uninitialized client, expect internal error or not found
+	assert.True(t, w.Code >= 400, "Expected error status code")
 	assert.Empty(t, w.Body.String())

docs/api-specification.yaml (2)

13-17: Update placeholder production server URL.

The production server URL https://api.example.com is a placeholder. Before release, update this to the actual production endpoint or remove it to avoid confusion.

---

574-600: Add maxItems constraint to the keys array.

The keys array in ListKeysResponse should have a maxItems constraint to align with the documented limit (10000) and prevent unbounded response sizes.

Proposed fix

     ListKeysResponse:
       type: object
       properties:
         message:
           type: string
         namespace:
           type: string
         collection:
           type: string
         keys:
           type: array
+          maxItems: 10000
           items:
             type: string
           example: ["key1", "key2", "key3"]

internal/services/card_service_integration_test.go (1)

14-59: Tests validate mock setup but don't exercise CardService.VerifyCard.

The test function is named "Integration Tests with Mock MongoDB" but it only verifies that the mock client can store and retrieve data. It doesn't actually invoke CardService.VerifyCard() to test the service's business logic. Consider calling the actual service method to validate the complete flow.

internal/services/card_service_test.go (2)

146-159: Consider isolating the MongoDB connection test.

This test attempts a real MongoDB connection with a fast timeout. While it gracefully skips when unavailable, it could cause flaky behavior in CI environments where network conditions vary. Consider moving this to a separate integration test file with a build tag.

---

169-279: Tests verify model behavior, not CardService.VerifyCard.

The function TestVerifyCardLogicBoundaries tests error string values and model methods (HasDevice, IsValid), but doesn't actually invoke CardService.VerifyCard(). This is fine for model-level testing, but consider renaming to TestCardModelBehavior to better reflect what's being tested.

cmd/server/main.go (1)

117-117: Unused kvStore parameter in setupRoutes.

The kvStore parameter is currently unused (named _) because all KV routes are commented out for the MVP. This leaves dead code in the initialization path. Consider either removing the parameter until KV routes are enabled, or documenting why it's kept for upcoming features.

internal/config/config_test.go (1)

8-33: Consider using t.Setenv instead of os.Clearenv and os.Setenv.

Using os.Clearenv() clears all environment variables including system ones (PATH, HOME, etc.), which can cause issues in parallel test execution or affect subsequent tests. Go 1.17+ provides t.Setenv() which automatically restores the original value after the test completes.

Proposed refactor for TestLoadConfig_MongoDB as example

 func TestLoadConfig_MongoDB(t *testing.T) {
-	os.Clearenv()
-	os.Setenv("DATABASE", "mongodb")
-	os.Setenv("MONGODB_URI", "mongodb://localhost:27017")
-	os.Setenv("SERVER_PORT", "9090")
-	os.Setenv("ENVIRONMENT", "TESTING")
+	// Clear relevant vars by setting empty, then set test values
+	t.Setenv("DATABASE", "mongodb")
+	t.Setenv("MONGODB_URI", "mongodb://localhost:27017")
+	t.Setenv("SERVER_PORT", "9090")
+	t.Setenv("ENVIRONMENT", "TESTING")
+	// Clear vars that should use defaults
+	t.Setenv("DATA_PATH", "")
+	t.Setenv("REDIS_URI", "")
 
 	cfg := LoadConfig()

Note: For testing defaults, you may need a helper that temporarily unsets specific variables rather than clearing all.

internal/database/mongodb/mongodb_crud_test.go (2)

21-35: Get method doesn't handle non-string document types correctly.

The mock's Get method only handles documents stored as strings (line 29), but the Set method stores values as string(value) which works. However, if someone stores a non-string type directly in Documents, the Get will return ErrKeyNotFound instead of the actual value. This inconsistency could mask bugs in tests.

Additionally, when the collection exists but the key doesn't, it falls through to line 34 returning ErrKeyNotFound, which is correct. However, for consistency with real MongoDB behavior, consider handling the case where the document exists but is not a string type.

💡 Suggested improvement for robustness

 func (m *mockMongoDBKV) Get(ctx context.Context, namespace, collection, key string) ([]byte, error) {
 	namespace = kv.NormalizeNamespace(namespace)
 	if _, exists := m.mockClient.Collections[namespace]; !exists {
 		return nil, kv.ErrKeyNotFound
 	}
 	if coll, exists := m.mockClient.Collections[namespace][collection]; exists {
 		if doc, found := coll.Documents[key]; found {
 			// Simulate BSON marshaling
 			if str, ok := doc.(string); ok {
 				return []byte(str), nil
 			}
+			// Handle []byte type as well
+			if b, ok := doc.([]byte); ok {
+				return b, nil
+			}
 		}
 	}
 	return nil, kv.ErrKeyNotFound
 }

---

474-498: Test name is misleading - no actual concurrency.

The test is named "concurrent namespace operations" but executes sequentially without goroutines or synchronization. This tests namespace isolation, not concurrent access. Consider renaming to "sequential namespace operations" or "namespace isolation with multiple namespaces" to accurately describe the test.

📝 Suggested rename

-	t.Run("concurrent namespace operations", func(t *testing.T) {
+	t.Run("multiple namespace isolation", func(t *testing.T) {

internal/database/mongodb/mongodb_test.go (2)

11-16: Connection failure test may be slow due to DNS resolution timeout.

Testing with mongodb://invalid-host:99999 will trigger DNS resolution which can take several seconds before failing. Consider using a shorter timeout or documenting the expected test duration.

💡 Consider using localhost with invalid port for faster failure

 func TestNewMongoDBKV_ConnectionFailed(t *testing.T) {
 	// Test with invalid URI - should fail to connect
-	_, err := NewMongoDBKV("mongodb://invalid-host:99999")
+	// Using localhost avoids DNS lookup delay
+	_, err := NewMongoDBKV("mongodb://localhost:1")
 	assert.Error(t, err)
 	assert.ErrorIs(t, err, kv.ErrConnectionFailed)
 }

---

43-66: Skipped tests provide limited value in CI.

While the scaffolding documents intended test coverage, these t.Skip() tests won't catch regressions. Consider:

1. Using testcontainers-go in CI for real MongoDB tests
2. Or marking these as integration tests with a build tag like //go:build integration

The current approach documents intent but provides no actual coverage.

internal/models/card.go (1)

6-30: Missing JSON tags may cause issues with HTTP API serialization.

Both Device and Card structs have bson tags but no json tags. If these models are ever serialized to JSON (e.g., in API responses), field names will use Go's default behavior (exported field names as-is). Consider adding json tags for explicit control over JSON serialization, especially for fields like _id which would serialize as ID in JSON.

💡 Add JSON tags for API compatibility

 type Device struct {
-	ID          string                 `bson:"_id"`
-	TenantID    string                 `bson:"tenant_id"`
+	ID          string                 `bson:"_id" json:"id"`
+	TenantID    string                 `bson:"tenant_id" json:"tenant_id"`
 	// ... similar for other fields
 }

internal/services/card_service.go (2)

49-54: Remove commented-out code or add TODO with tracking.

Commented-out status check is a code smell. If this is intentionally disabled, add a clear comment explaining why and when (if ever) it should be re-enabled. Better yet, use a configuration flag.

📝 Suggested improvement

-	// Status check disabled - accept devices regardless of status
-	// if device.Status != "active" {
-	// 	log.Printf("[CardVerification] Device not active: namespace=%s, device_sn=%s, status=%s",
-	// 		namespace, deviceSN, device.Status)
-	// 	return ErrDeviceNotActive
-	// }
+	// TODO(`#issue-number`): Device status check temporarily disabled for MVP.
+	// Re-enable once device status management is implemented.
+	_ = device.Status // Acknowledge unused field

---

74-86: Duplicate tolerance logic with Card.IsValid.

The tolerance check on lines 77 and 83 duplicates the ±60 seconds logic already in Card.IsValid(). If tolerance changes, it needs updating in two places. Consider refactoring to use Card.IsValid() result and then determining why it's invalid.

♻️ Suggested refactor to reduce duplication

 	// Step 4: Verify card is within valid time range (with ±60s tolerance)
 	now := time.Now()
 	if !card.IsValid(now) {
-		if now.Before(card.EffectiveAt.Add(-60 * time.Second)) {
+		tolerance := 60 * time.Second
+		if now.Before(card.EffectiveAt.Add(-tolerance)) {
 			log.Printf("[CardVerification] Card not yet valid: ...")
 			return ErrCardNotYetValid
 		}
-
-		log.Printf("[CardVerification] Card expired: ...")
+		// If not "not yet valid", must be expired
+		log.Printf("[CardVerification] Card expired: ...")
 		return ErrCardExpired
 	}

Alternatively, add methods to Card like IsNotYetValid(now) and IsExpired(now) to encapsulate the logic.

internal/handlers/batch.go (1)

232-267: ListKeysHandler has unused parameters and variables.

The handler parses namespace, collection, limit, and offset but never uses them since it returns NOT_IMPLEMENTED. This is dead code that may confuse readers.

Consider either:

1. Removing the parsing code until implementation
2. Using _ for variables if keeping for documentation

📝 Simplified scaffolding

 func ListKeysHandler(kvStore kv.KV) gin.HandlerFunc {
 	return func(c *gin.Context) {
-		namespace := c.Param("namespace")
-		collection := c.Param("collection")
-
-		// Validate parameters
-		if namespace == "" || collection == "" {
-			c.JSON(http.StatusBadRequest, ErrorResponse{
-				Message: "namespace and collection are required",
-				Code:    "INVALID_PARAMS",
-			})
-			return
-		}
-
-		// Parse query parameters
-		limit := 1000
-		offset := 0
-		if limitParam := c.Query("limit"); limitParam != "" {
-			if err := scanInt(limitParam, &limit); err != nil || limit > 10000 {
-				limit = 1000
-			}
-		}
-		if offsetParam := c.Query("offset"); offsetParam != "" {
-			_ = scanInt(offsetParam, &offset)
-		}
-
-		// Try to list keys (this may not be supported by all backends)
-		// For now, return a not-implemented response
+		// TODO: Implement key listing when backend support is added
 		c.JSON(http.StatusNotImplemented, ErrorResponse{
 			Message: "listing keys is not implemented for this backend",
 			Code:    "NOT_IMPLEMENTED",
 		})
 	}
 }

internal/handlers/kv_test.go (1)

332-355: TestNormalizeNamespace doesn't test actual normalization behavior.

The test name suggests testing namespace normalization (empty → "default"), but it only tests with explicit "default" namespace. Consider adding a test case that actually exercises the normalization:

📝 Add test for empty namespace normalization

func TestNormalizeNamespace_EmptyToDefault(t *testing.T) {
	mockKV := NewMockKV()
	gin.SetMode(gin.TestMode)
	router := gin.New()
	router.POST("/api/v1/kv/:namespace/:collection/:key", SetKVHandler(mockKV))
	router.GET("/api/v1/kv/:namespace/:collection/:key", GetKVHandler(mockKV))

	// Set with empty namespace (should normalize to "default")
	body := KVRequestBody{Value: "test"}
	bodyJSON, _ := json.Marshal(body)
	req, _ := http.NewRequest("POST", "/api/v1/kv//users/user1", bytes.NewBuffer(bodyJSON))
	req.Header.Set("Content-Type", "application/json")
	// Note: This may fail because Gin may not route empty namespace segments correctly
	// The actual normalization happens inside the handler after routing
}

Note: The current test is still valuable but the name is misleading since it tests successful operation with "default" namespace rather than the normalization from empty string.

internal/handlers/namespace.go (2)

44-55: Consider using blank identifier for unused parameter.

The kvStore parameter is unused. While this is intentional for a stub, some linters may flag it. Consider using _ kv.KV to explicitly indicate the parameter is unused, or adding a //nolint:unparam directive if the signature must be preserved for interface compliance.

🔧 Suggested fix

-func ListNamespacesHandler(kvStore kv.KV) gin.HandlerFunc {
+func ListNamespacesHandler(_ kv.KV) gin.HandlerFunc {

---

57-78: Same unused parameter consideration applies.

Same suggestion as ListNamespacesHandler - consider using blank identifier for the unused kvStore parameter.

internal/testing/mocks/mongodb_mock.go (1)

29-64: Mock operations don't modify internal state.

InsertOne, UpdateOne, and DeleteOne return mock results but don't actually modify the Documents map. This is acceptable for error injection testing, but be aware that state-based test assertions (e.g., verifying a document was inserted) won't work with these methods. The Setup* helper methods on MockClient should be used for state management.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In @.github/workflows/release.yml:
- Around line 141-142: The Docker Hub tag lines currently combine
DOCKERHUB_USERNAME with IMAGE_NAME (where IMAGE_NAME is set to
github.repository), causing malformed names like username/stayforge/commander;
change the release workflow to use only the repository name for Docker Hub tags
by introducing or using a separate variable (e.g., DOCKERHUB_IMAGE_NAME or
DOCKERHUB_REPO) that contains only github.event.repository.name (or the repo
basename) and replace references to ${ env.IMAGE_NAME } in the Docker Hub tag
steps with that new variable so tags become ${ secrets.DOCKERHUB_USERNAME }/${
env.DOCKERHUB_IMAGE_NAME }:${{ needs.build.outputs.version }} and similar for
:latest.

🧹 Nitpick comments (4)

.github/workflows/release.yml (1)

89-90: Buildx setup is unnecessary in the push job.

The push-ghcr job only uses docker load, docker tag, and docker push commands, which don't require Buildx. This step can be removed to simplify the workflow and reduce execution time.

♻️ Proposed fix

  push-ghcr:
    needs: build
    runs-on: ubuntu-latest
    permissions:
      contents: read
      packages: write

    steps:
-     - name: Set up Docker Buildx
-       uses: docker/setup-buildx-action@v3
-
      - name: Log in to GitHub Container Registry

internal/handlers/batch_test.go (1)

106-178: Isolate KV state per subtest to avoid order dependence.
The shared mock store is mutated across cases, which can couple outcomes to execution order as new cases are added. Consider reinitializing the store/router inside each subtest.

♻️ Suggested refactor

- mockKV := NewMockKV()
- gin.SetMode(gin.TestMode)
- router := gin.New()
- router.DELETE("/api/v1/kv/batch", BatchDeleteHandler(mockKV))
-
- // Setup initial data
- ctx := context.Background()
- testValue, _ := json.Marshal("test value")
- _ = mockKV.Set(ctx, "default", "users", "user1", testValue)
- _ = mockKV.Set(ctx, "default", "users", "user2", testValue)
+ gin.SetMode(gin.TestMode)

  for _, tt := range tests {
  	t.Run(tt.name, func(t *testing.T) {
+ 		mockKV := NewMockKV()
+ 		router := gin.New()
+ 		router.DELETE("/api/v1/kv/batch", BatchDeleteHandler(mockKV))
+
+ 		// Setup initial data
+ 		ctx := context.Background()
+ 		testValue, _ := json.Marshal("test value")
+ 		_ = mockKV.Set(ctx, "default", "users", "user1", testValue)
+ 		_ = mockKV.Set(ctx, "default", "users", "user2", testValue)
+
  		bodyJSON, _ := json.Marshal(tt.request)
  		req, _ := http.NewRequest("DELETE", "/api/v1/kv/batch", bytes.NewBuffer(bodyJSON))

internal/handlers/batch.go (2)

78-85: Add an explicit upper-bound check for batch size.
You already guard against empty operations; a direct > 1000 check enforces the cap even if validation is ever disabled or bypassed.

🔒 Suggested guard (apply to both handlers)

 		// Validate that we don't have too many operations
 		if len(req.Operations) == 0 {
 			c.JSON(http.StatusBadRequest, ErrorResponse{
 				Message: "at least one operation is required",
 				Code:    "EMPTY_OPERATIONS",
 			})
 			return
 		}
+		if len(req.Operations) > 1000 {
+			c.JSON(http.StatusBadRequest, ErrorResponse{
+				Message: "too many operations (max 1000)",
+				Code:    "TOO_MANY_OPERATIONS",
+			})
+			return
+		}

Also applies to: 157-164

---

248-258: Clamp negative limit/offset values.
scanInt allows negatives; a defensive clamp avoids unexpected behavior once list-keys is implemented.

♻️ Suggested adjustment

 		if limitParam := c.Query("limit"); limitParam != "" {
-			if err := scanInt(limitParam, &limit); err != nil || limit > 10000 {
+			if err := scanInt(limitParam, &limit); err != nil || limit > 10000 || limit < 0 {
 				limit = 1000
 			}
 		}
 		if offsetParam := c.Query("offset"); offsetParam != "" {
-			_ = scanInt(offsetParam, &offset) //nolint:errcheck // offset parsing failure is intentionally ignored, default 0 is used
+			if err := scanInt(offsetParam, &offset); err != nil || offset < 0 {
+				offset = 0
+			}
 		}