feat(provider): implement SP registration (topic 3)

[gabriel-farache]

Summary

• Implements service provider registration endpoint (POST /registrations) that accepts provider metadata and returns 201 with the created resource
• Adds endpoint URL validation, duplicate detection, and persistent JSON file-backed store with atomic writes
• Introduces domain error types with RFC 7807 problem detail responses and context-aware Store/Service interfaces

Test plan

• Unit tests for service layer validation logic (duplicate detection, URL format)
• Integration tests covering full HTTP request/response lifecycle
• Verify 201 response with correct body on successful registration
• Verify 409 conflict on duplicate endpoint
• Verify 400 bad request on invalid input
• Verify RFC 7807 error response format

Made with Cursor

[qodo-code-review]

PR Summary by Qodo

Implement persistent service provider registration lifecycle

✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Implements create, list, and get flows for embedded and external service providers.
• Enforces semantic validation and service-type uniqueness with RFC 7807 responses.
• Persists registrations atomically and restores provider slots during startup.

Diagram

sequenceDiagram
  actor Client
  participant API as API Router
  participant Handler
  participant Service
  participant Registry
  participant Store as JSON Store
  Client->>API: POST provider
  API->>Handler: Validated request
  Handler->>Handler: Semantic validation
  Handler->>Service: Register provider
  Service->>Registry: Claim or move slot
  Service->>Store: Atomic save
  Store-->>Service: Persisted provider
  Service-->>Handler: Provider result
  alt Validation or conflict
    Handler-->>Client: RFC 7807 error
  else Registration succeeds
    Handler-->>Client: 201 or 200 provider
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use SQLite persistence

• ➕ Provides transactional uniqueness and registration updates
• ➕ Avoids rewriting the complete provider collection
• ➕ Supports richer queries and future schema evolution
• ➖ Adds operational and migration complexity
• ➖ Introduces a heavier dependency for a low-volume local registry
• ➖ Requires careful database lifecycle management

2. Derive slots from the store

• ➕ Removes duplicated registry and persistence state
• ➕ Avoids registry-store divergence after write failures
• ➕ Centralizes uniqueness enforcement behind the Store interface
• ➖ File-backed conflict checks require repeated full-file reads
• ➖ Needs stronger transactional Store operations for concurrent updates
• ➖ Makes embedded and external slot handling less explicit

Recommendation: The current Store abstraction, in-memory slot registry, and atomic JSON replacement are appropriate for a lightweight, single-process agent with few registrations. Retain this design, but ensure registry mutations are rolled back when persistence fails; adopt SQLite only if registration volume, multi-process access, or stronger transactional requirements emerge.

Files changed (17) +1493 / -60

Enhancement (9) +625 / -15

main.goWire provider registration into agent startup +23/-2

Wire provider registration into agent startup

• Constructs the file store, slot registry, and provider service; restores persisted entries and registers configured embedded providers. It also installs RFC 7807 handling for strict-handler failures.

cmd/environment-agent/main.go

server.goDefer semantic constraints to provider handlers +43/-0

Defer semantic constraints to provider handlers

• Adds request-URI context middleware and customizes OpenAPI validation to bypass handler-owned regex and parameter constraints. Structural request validation remains in middleware while semantic failures can return 422.

internal/apiserver/server.go

handler.goImplement provider HTTP operations +86/-13

Implement provider HTTP operations

• Implements provider creation, idempotent re-registration, listing, and lookup through the provider service. Maps validation, conflict, and not-found outcomes to typed RFC 7807 responses.

internal/handler/handler.go

provider.goAdd provider validation and slot registry +115/-0

Add provider validation and slot registry

• Adds provider ID, schema-version, and endpoint validators plus UUID generation. Introduces a synchronized registry enforcing exclusive service-type ownership and atomic slot moves.

internal/provider/provider.go

errors.goDefine provider domain errors +17/-0

Define provider domain errors

• Introduces typed conflict and not-found error codes for stable service-to-handler error mapping.

internal/provider/service/errors.go

service.goImplement provider registration service +184/-0

Implement provider registration service

• Coordinates create-or-update registration, service-type ownership, listing, lookup, persisted-state restoration, and embedded provider initialization. Converts stored records into API resources with server-managed identifiers and timestamps.

internal/provider/service/service.go

file.goAdd atomic JSON file persistence +103/-0

Add atomic JSON file persistence

• Implements synchronized save, list, and lookup operations over a JSON file. Writes use a temporary file followed by rename to avoid exposing partial contents.

internal/provider/store/file.go

store.goDefine the provider persistence boundary +28/-0

Define the provider persistence boundary

• Adds a context-aware Store interface and the persistent representation for provider metadata, type, IDs, and timestamps.

internal/provider/store/store.go

requestctx.goExpose request URIs through context +26/-0

Expose request URIs through context

• Adds middleware and a typed helper for carrying the request URI into handlers, enabling RFC 7807 instance fields.

internal/requestctx/requestctx.go

Tests (3) +814 / -0

provider_integration_test.goCover provider registration end to end +668/-0

Cover provider registration end to end

• Exercises startup behavior, embedded providers, registration and renewal, persistence, slot conflicts, service-type moves, request validation, generated IDs, and error response formats against a real server.

internal/provider/provider_integration_test.go

provider_unit_test.goTest validation, IDs, and slot ownership +133/-0

Test validation, IDs, and slot ownership

• Adds boundary tests for provider IDs and schema versions, UUID generation checks, and registry claim and move behavior.

internal/provider/provider_unit_test.go

suite_test.goInitialize the provider Ginkgo suite +13/-0

Initialize the provider Ginkgo suite

• Adds the package-level test entry point and Gomega failure integration.

internal/provider/suite_test.go

Other (5) +54 / -45

MakefileInclude provider suites in test targets +2/-2

Include provider suites in test targets

• Adds the provider package to both unit and integration Ginkgo test commands.

Makefile

openapi.yamlMark handler-owned validation constraints +2/-0

Mark handler-owned validation constraints

• Annotates provider ID and schema-version constraints so middleware can defer their semantic validation to the handler and preserve 422 responses.

api/v1alpha1/openapi.yaml

spec.gen.goRegenerate the embedded OpenAPI specification +41/-41

Regenerate the embedded OpenAPI specification

• Refreshes the compressed generated specification to include the handler-validation annotations.

api/v1alpha1/spec.gen.go

go.modPromote UUID generation to a direct dependency +1/-1

Promote UUID generation to a direct dependency

• Makes google/uuid a direct dependency for generated provider identifiers.

go.mod

config.goAdd service provider configuration +8/-1

Add service provider configuration

• Introduces environment-backed configuration for embedded service types and the registration persistence path.

internal/config/config.go

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Re-registration ignores requested ID ✓ Resolved 🐞 Bug ≡ Correctness

Description

Register computes the supplied ?id= value, but the existing-name update path never assigns it to
existing.ID. A successful re-registration therefore retains and returns the old ID instead of
using the requested provider ID.

Code

internal/provider/service/service.go[R50-60]

+	if existing != nil {
+		if existing.ServiceType != serviceType {
+			if err := s.registry.Move(name, existing.ServiceType, serviceType); err != nil {
+				return nil, false, &DomainError{Code: ErrCodeConflict, Message: err.Error()}
+			}
+		}
+		existing.Endpoint = endpoint
+		existing.ServiceType = serviceType
+		existing.SchemaVersion = schemaVersion
+		existing.DisplayName = displayName
+		existing.UpdateTime = now

Evidence

The requested ID is selected into the local id variable, but only the new-provider constructor
uses it. The existing-provider branch updates other mutable fields and returns the unchanged stored
ID, contrary to the requirement that a supplied query ID be used.

internal/provider/service/service.go[36-64]
.ai/specs/environment-agent.spec.md[357-360]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A supplied provider ID is ignored when a provider with the same natural-key name already exists.

## Issue Context
Define ID update semantics, validate uniqueness before changing an ID, and test same-name re-registration with an explicit `?id=` value.

## Fix Focus Areas
- internal/provider/service/service.go[36-64]
- internal/provider/store/file.go[52-58]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Duplicate provider IDs accepted ✓ Resolved 🐞 Bug ≡ Correctness

Description

New registration checks uniqueness only by provider name and saves caller-provided IDs without
querying GetByID. Two providers can consequently share one resource ID, and GET by that ID returns
whichever duplicate occurs first in the file.

Code

internal/provider/service/service.go[43]

+	existing, err := s.store.GetByName(ctx, name)

Evidence

Register accepts or generates an ID but checks only GetByName before inserting. FileStore replaces
records by name and GetByID returns the first matching array element, while the API contract
identifies id as unique.

internal/provider/service/service.go[36-46]
internal/provider/store/file.go[31-41]
internal/provider/store/file.go[52-70]
api/v1alpha1/openapi.yaml[240-246]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Distinct provider names can be registered with the same caller-supplied resource ID, making ID-based retrieval ambiguous.

## Issue Context
Check ID ownership atomically inside registration and return a conflict when another provider already owns the requested ID. Cover both creation and ID-changing update behavior.

## Fix Focus Areas
- internal/provider/service/service.go[32-85]
- internal/provider/store/file.go[31-41]
- internal/provider/store/file.go[52-70]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Schema mismatch does not fail ✓ Resolved 🐞 Bug ☼ Reliability

Description

FileStore validates only JSON syntax and accepts semantically invalid records such as [{}].
LoadPersisted then claims an empty slot and startup succeeds instead of failing fast on persistence
schema mismatch.

Code

internal/provider/store/file.go[R86-90]

+	var providers []StoredProvider
+	if err := json.Unmarshal(data, &providers); err != nil {
+		return nil, err
+	}
+	return providers, nil

Evidence

readFile only unmarshals into StoredProvider values, so missing JSON fields become valid Go zero
values. LoadPersisted accepts those values and Registry.Claim permits the first empty
name/service-type pair, despite the explicit requirement to fail startup on persistence schema
mismatch.

internal/provider/store/file.go[75-90]
internal/provider/service/service.go[113-124]
internal/provider/provider.go[83-91]
.ai/specs/environment-agent.spec.md[367-374]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Syntactically valid but schema-invalid persistence records are accepted during startup.

## Issue Context
Validate every decoded record before mutating the registry, including required fields, IDs, provider type, timestamps, and uniqueness constraints. Return an error for any invalid record so startup follows the existing fail-fast path.

## Fix Focus Areas
- internal/provider/store/file.go[75-90]
- internal/provider/service/service.go[113-124]
- internal/provider/store/store.go[17-27]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (3)

4. Failed saves corrupt slots ✓ Resolved 🐞 Bug ☼ Reliability

Description

Register mutates slot ownership before calling Store.Save, but does not roll back when persistence
fails. A failed create leaves a phantom claimed slot, while a failed move frees the old slot even
though durable data still assigns it to that provider.

Code

internal/provider/service/service.go[52]

+			if err := s.registry.Move(name, existing.ServiceType, serviceType); err != nil {

Evidence

Register calls Registry.Move or Registry.Claim before Store.Save. Claim and Move immediately mutate
the slot map, while FileStore.Save can subsequently fail during file writing or rename without any
rollback.

internal/provider/service/service.go[50-84]
internal/provider/provider.go[83-105]
internal/provider/store/file.go[93-102]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Registry ownership is changed before persistence succeeds. If `Store.Save` fails, the in-memory registry and durable provider data disagree.

## Issue Context
Both new claims and service-type moves need transactional behavior. Add failing-store tests for create and update paths.

## Fix Focus Areas
- internal/provider/service/service.go[50-84]
- internal/provider/provider.go[83-105]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Disabled embedded providers persist ✓ Resolved 🐞 Bug ≡ Correctness

Description

RegisterEmbedded persists embedded providers, and LoadPersisted restores every stored provider
before applying the current enabled list. Removing a service type from AGENT_EMBEDDED_SPS therefore
leaves that embedded provider active and occupying its slot after restart.

Code

internal/provider/service/service.go[162]

+		if err := s.store.Save(context.Background(), sp); err != nil {

Evidence

Embedded records are written to the same store as external providers, and LoadPersisted claims all
records without checking their type or current configuration. Repository requirements state that
embedded providers must be explicitly enabled and only enabled providers may be registered at
startup.

internal/provider/service/service.go[113-165]
cmd/environment-agent/main.go[69-73]
.ai/specs/environment-agent.spec.md[341-345]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Embedded providers disabled in current configuration remain persisted, listed, and registered after restart.

## Issue Context
Only external registrations should survive independently of the current embedded-provider configuration, or startup must filter stale embedded records before claiming slots.

## Fix Focus Areas
- internal/provider/service/service.go[113-165]
- internal/provider/store/store.go[17-27]
- cmd/environment-agent/main.go[69-73]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Optional provider data discarded ✓ Resolved 🐞 Bug ≡ Correctness

Description

CreateProvider accepts operations and metadata through the Provider request schema but does not
pass either field to Register. The fields are silently lost from persistence and the successful
response.

Code

internal/handler/handler.go[R51-59]

+	result, isNew, err := h.provider.Register(
+		ctx,
+		body.Name,
+		body.Endpoint,
+		body.ServiceType,
+		body.SchemaVersion,
+		body.DisplayName,
+		request.Params.Id,
+	)

Evidence

The generated request model contains Metadata and Operations, but the handler's service call omits
both. StoredProvider and toAPI also have no representation for these values, proving they cannot
survive registration.

api/v1alpha1/types.gen.go[115-126]
internal/handler/handler.go[51-59]
internal/provider/store/store.go[17-27]
internal/provider/service/service.go[168-183]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Client-supplied `operations` and `metadata` are accepted but silently discarded during registration.

## Issue Context
Thread supported fields through the handler, service, persistence model, and API conversion. If these fields are intentionally unsupported for registration, remove or reject them in the request contract instead of accepting and dropping them.

## Fix Focus Areas
- internal/handler/handler.go[35-76]
- internal/provider/service/service.go[30-85]
- internal/provider/service/service.go[168-183]
- internal/provider/store/store.go[17-27]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}