feat(agent): add environment agent support, remove direct SP management

[gabriel-farache]

Summary

Replaces direct HTTP-based Service Provider (SP) provisioning with agent-based provisioning over NATS/CloudEvents, completing the move to an agent-only architecture. Each resource is now routed to an agent selected by policy (SelectedAgent) instead of a provider, and instance creation is dispatched asynchronously to that agent instead of calling a provider's HTTP API directly.

What changed

• Agent domain (internal/agent/): registration, health monitoring, service-type matching, OpenAPI spec + generated client/server.
• NATS integration (internal/sp/messaging, internal/sp/consumer, internal/sp/pending): publishes create/delete/cancel CloudEvents to the selected agent's topic, consumes agent responses via JetStream, and retries pending/queued requests on timeout.
• Placement & Policy: agent-aware routing in CreateRun, RehydrateResource, ReEvaluateWithExclude; AgentName replaces provider selection on Resource and ServiceTypeInstance.
• Removed: all provider-specific code (internal/sp/*/provider, api/sp/*/provider, pkg/sp/client/provider), the PENDING_PROVIDER status, and provider-specific error types.
• Data integrity: FK constraints (ON DELETE RESTRICT) enforce that Resource/ServiceTypeInstance agent references stay valid.

Test plan

• go build ./...
• go test ./... -count=1
• make lint — 0 issues
• sp subsystem test suite (agent-routed create/get/list/delete, including deferred and acknowledged hard-delete)

Made with Cursor

[gabriel-farache]

Do not merged until the agent is ready

[qodo-code-review]

PR Summary by Qodo

Add environment agent provisioning via NATS; remove direct SP provider management

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

AI Description

• Introduce an Agent API/domain for agent registration, listing, and heartbeats.
• Route provisioning through NATS JetStream (publish requests, consume agent responses, sweep
 timeouts).
• Remove direct service-provider management and enforce agent FK integrity in DB/models/tests.

Diagram

graph TD
  HTTP["HTTP API"] --> PLC["Placement svc"] --> POL["Policy svc"]
  HTTP --> AAPI["Agent API"] --> DB[("Postgres")]
  PLC --> DB
  PLC --> NATS{{"NATS JetStream"}} --> AG{{"Env agent"}}
  NATS --> RC["Response consumer"] --> DB
  SWP["Pending sweep"] --> DB

  subgraph Legend
    direction LR
    _svc["Service"] ~~~ _db[("Database")] ~~~ _ext{{"External"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Adopt a CloudEvents SDK for envelope/validation

• ➕ Improves spec compliance and validation for attributes/extensions.
• ➕ Reduces ad-hoc parsing and makes event evolution safer.
• ➖ Adds dependency and integration work during an already-large migration.
• ➖ May be unnecessary if the envelope remains minimal.

2. Use NATS request/reply for provisioning acknowledgements

• ➕ Clear per-request correlation semantics and simpler consumer topology.
• ➕ Avoids managing a shared response stream/consumer durable state.
• ➖ Long-running provisioning still needs persistence and retries; request/reply alone won’t solve it.
• ➖ Harder to replay/audit compared to JetStream streams.

3. Store outbound events in an outbox table (transactional outbox)

• ➕ Stronger delivery guarantees (publish after DB commit) and easier recovery.
• ➕ Avoids losing publishes when DB write succeeds but NATS publish fails.
• ➖ More schema/worker complexity; requires an additional dispatcher loop.
• ➖ May be overkill if best-effort publishing is acceptable initially.

Recommendation: The chosen JetStream-based async flow is a good fit for agent provisioning and aligns with the new pending/queued lifecycle handling. If delivery guarantees become critical, consider a transactional outbox next; a CloudEvents SDK can be layered on later if event interoperability/validation needs grow.

Files changed (79) +6105 / -2249

Enhancement (22) +1792 / -465

openapi.yamlAdd Agent API OpenAPI spec +357/-0

Add Agent API OpenAPI spec

• Introduces the v1alpha1 Agent API for agent registration, listing, and heartbeat reporting.

api/agent/v1alpha1/openapi.yaml

handler.goImplement Agent API handlers +172/-0

Implement Agent API handlers

• Implements List/Create (register-or-update)/Get/Heartbeat handlers and maps service errors to problem+json responses.

internal/agent/handlers/v1alpha1/handler.go

monitor.goAdd agent heartbeat monitor +80/-0

Add agent heartbeat monitor

• Adds a periodic sweep that marks agents unavailable when they miss the configured heartbeat timeout.

internal/agent/healthcheck/monitor.go

agent.goAdd AgentService business logic +164/-0

Add AgentService business logic

• Implements register-or-update, list, get, and heartbeat with consumer lag threshold and monotonic heartbeat handling.

internal/agent/service/agent.go

errors.goDefine agent service error types +32/-0

Define agent service error types

• Adds typed service errors (validation/not-found/internal) consumed by the Agent HTTP handlers.

internal/agent/service/errors.go

agent.goAdd GORM agent store +151/-0

Add GORM agent store

• Implements persisted CRUD, filtering, ready listing, and health status updates for Agent entities.

internal/agent/store/agent/agent.go

agent.goAdd Agent domain model +29/-0

Add Agent domain model

• Defines the Agent GORM model including environment, supported service types, topic name, and heartbeat/health status fields.

internal/agent/store/model/agent.go

run.goWire agent services and NATS provisioning into startup +98/-57

Wire agent services and NATS provisioning into startup

• Adds agent store/service/handler wiring, starts agent health monitor, initializes JetStream publisher/response consumer, and runs pending/queued sweep; removes provider handler/service wiring.

internal/app/run.go

service_client.goPass agent context to policy evaluation +3/-3

Pass agent context to policy evaluation

• Updates placement’s policy client to support available/excluded agent lists used by agent selection.

internal/placement/policy/service_client.go

types.goReplace provider selection with agent selection in policy types +6/-4

Replace provider selection with agent selection in policy types

• Extends EvaluateRequest with AvailableAgents/ExcludeAgents and renames SelectedProvider to SelectedAgent in responses.

internal/placement/policy/types.go

placement.goMake placement agent-aware and persist AgentName +94/-34

Make placement agent-aware and persist AgentName

• Fetches ready agents for policy input, requires SelectedAgent, persists AgentName on resources, and passes agent_name into SPRM create/rehydrate requests.

internal/placement/service/placement.go

types.goAdd agent_name to SPRM create request type +1/-0

Add agent_name to SPRM create request type

• Extends CreateResourceRequest with optional agent_name for agent-routed provisioning.

internal/placement/sprm/types.go

resource.goAdd Resource.AgentName with FK to Agent.Name +12/-8

Add Resource.AgentName with FK to Agent.Name

• Adds agent_name and GORM belongsTo association to Agent(Name) with ON DELETE RESTRICT; makes provider_name optional.

internal/placement/store/model/resource.go

evaluation.goParse selected_agent and agent_constraints from OPA output +31/-23

Parse selected_agent and agent_constraints from OPA output

• Replaces service-provider outputs with agent selection and constraints (allow list, patterns, environment constraints).

internal/policy/opa/evaluation.go

constraints.goImplement accumulated agent constraints and remove SP constraints +113/-94

Implement accumulated agent constraints and remove SP constraints

• Adds agent constraint merge/validate helpers and OPA input mapping; removes service-provider constraint accumulation logic.

internal/policy/service/constraints.go

evaluation.goUpdate policy evaluation to select an agent +53/-50

Update policy evaluation to select an agent

• Updates evaluation flow to incorporate agent availability/exclusion inputs and return SelectedAgent decisions.

internal/policy/service/evaluation.go

response_consumer.goAdd JetStream response consumer for agent replies +181/-0

Add JetStream response consumer for agent replies

• Creates a durable JetStream consumer on the agent response subject, maps events to instance status updates, and triggers delete publish on cancel-rejected.

internal/sp/consumer/response_consumer.go

publisher.goAdd JetStream publisher for agent provisioning requests +45/-0

Add JetStream publisher for agent provisioning requests

• Implements CloudEvents-style JSON envelope publishing for create/delete/cancel requests via JetStream.

internal/sp/messaging/publisher.go

types.goDefine CloudEvent types and payload structs for agent messaging +28/-0

Define CloudEvent types and payload structs for agent messaging

• Adds constants for CE types/source/specversion and typed payloads for create/delete/cancel requests.

internal/sp/messaging/types.go

sweep.goAdd sweep for timed-out pending/queued instances +96/-0

Add sweep for timed-out pending/queued instances

• Adds periodic DB sweeps for timed-out pending/queued instances, incrementing retries or marking failed/cancelled.

internal/sp/pending/sweep.go

service_type_instance.goPublish create requests to NATS and stop calling provider HTTP +39/-192

Publish create requests to NATS and stop calling provider HTTP

• Creates instances with an initial pending status, publishes create events to agent subjects when configured, and removes provider HTTP integration for create/delete paths.

internal/sp/service/resource_manager/service_type_instance.go

service_type_instance.goAdd ServiceTypeInstance.AgentName FK and pending timestamp tracking +7/-0

Add ServiceTypeInstance.AgentName FK and pending timestamp tracking

• Adds agent_name with belongsTo Agent(Name) ON DELETE RESTRICT and pending_started_at for pending/queued timeout handling.

internal/sp/store/model/service_type_instance.go

Refactor (14) +72 / -148

openapi.yamlMake provider_name optional; simplify deletion enum +1/-4

Make provider_name optional; simplify deletion enum

• Removes provider_name from required fields and drops PENDING_PROVIDER from deletion_status values.

api/sp/v1alpha1/resource_manager/openapi.yaml

db.goMigrate Agent model and stop migrating Provider model +2/-1

Migrate Agent model and stop migrating Provider model

• Adds AutoMigrate for agent model and removes provider model migration to support agent-only architecture.

internal/app/db.go

openapi.goAdd Agent OpenAPI validation; remove provider validation +17/-20

Add Agent OpenAPI validation; remove provider validation

• Loads agent OpenAPI spec and validates /agents routes; removes provider spec usage.

internal/app/openapi.go

local_client.goAdjust placement local client wiring +1/-1

Adjust placement local client wiring

• Updates local placement client wiring to align with agent-aware placement flow and updated interfaces.

internal/catalog/placement/local_client.go

errors.goRename provider-centric placement errors to agent-centric +4/-4

Rename provider-centric placement errors to agent-centric

• Updates error messages and types to reflect agent-based routing rather than provider selection.

internal/placement/service/errors.go

service_client.goUpdate SPRM client wiring +1/-1

Update SPRM client wiring

• Adjusts placement→SPRM client implementation to align with updated request types including agent_name.

internal/placement/sprm/service_client.go

errors.goRename provider constraint errors to agent constraint errors +3/-3

Rename provider constraint errors to agent constraint errors

• Renames exported error helpers/messages to match agent-constraint semantics.

internal/policy/service/errors.go

scheduler.goSimplify cleanup scheduler for agent-only mode +15/-50

Simplify cleanup scheduler for agent-only mode

• Removes provider health/retry branches and marks deferred deletions complete directly (no provider calls).

internal/sp/cleanup/scheduler.go

errors.goRename provider error mapping to provisioning error mapping +2/-2

Rename provider error mapping to provisioning error mapping

• Maps the renamed provisioning error code to a 422 problem+json response.

internal/sp/handlers/resource_manager/errors.go

errors.goRename ProviderError to ProvisioningError +8/-8

Rename ProviderError to ProvisioningError

• Renames provider-centric error codes and helpers to provisioning-centric equivalents and updates client-error classification.

internal/sp/service/errors.go

convert.goRemove provider response helper type +0/-6

Remove provider response helper type

• Deletes ProviderResponse type that was only used by direct provider HTTP provisioning.

internal/sp/service/resource_manager/convert.go

db.goUpdate SP store DB setup after provider removal +1/-1

Update SP store DB setup after provider removal

• Adjusts store initialization code paths to match the simplified store interface without provider accessors.

internal/sp/store/db.go

service_instance.goRemove provider deletion-pending helpers and add MarkDeletionComplete +17/-39

Remove provider deletion-pending helpers and add MarkDeletionComplete

• Drops provider-dependent deletion status transitions and adds a deletion completion marker used by the cleanup scheduler.

internal/sp/store/resource_manager/service_instance.go

store.goRemove Provider() accessor from SP store interface +0/-8

Remove Provider() accessor from SP store interface

• Simplifies the store interface and datastore implementation to only expose service type instance storage.

internal/sp/store/store.go

Tests (27) +1791 / -1520

handler_suite_test.goAdd agent handler Ginkgo suite +3/-3

Add agent handler Ginkgo suite

• Adds suite setup for agent handler tests.

internal/agent/handlers/v1alpha1/handler_suite_test.go

handler_test.goAdd Agent handler tests +169/-0

Add Agent handler tests

• Adds tests covering registration idempotency, listing, get-by-id, and heartbeat endpoint behavior.

internal/agent/handlers/v1alpha1/handler_test.go

healthcheck_suite_test.goAdd agent healthcheck test suite +2/-2

Add agent healthcheck test suite

• Adds suite setup for agent health monitor tests.

internal/agent/healthcheck/healthcheck_suite_test.go

monitor_test.goTest agent heartbeat timeout handling +126/-0

Test agent heartbeat timeout handling

• Validates transition to unavailable based on create_time/last_heartbeat and heartbeat timeout settings.

internal/agent/healthcheck/monitor_test.go

agent_suite_test.goAdd AgentService test suite +13/-0

Add AgentService test suite

• Adds suite setup for agent service tests.

internal/agent/service/agent_suite_test.go

agent_test.goAdd AgentService unit tests +180/-0

Add AgentService unit tests

• Tests validation, create/update semantics, heartbeat status transitions (ready/congested), and listing filters.

internal/agent/service/agent_test.go

agent_suite_test.goAdd agent store test suite +3/-3

Add agent store test suite

• Adds suite setup for agent store tests.

internal/agent/store/agent/agent_suite_test.go

agent_test.goAdd agent store tests +254/-0

Add agent store tests

• Tests agent persistence operations, filters, ordering, and health/ready list behaviors.

internal/agent/store/agent/agent_test.go

openapi_validation_test.goRemove provider OpenAPI route validation tests +0/-6

Remove provider OpenAPI route validation tests

• Drops tests for /providers request validation after provider API removal.

internal/app/openapi_validation_test.go

placement_test.goUpdate placement tests for agent selection flow +157/-258

Update placement tests for agent selection flow

• Refactors tests to assert SelectedAgent behavior, AgentName persistence, and updated policy/SPRM request shapes.

internal/placement/service/placement_test.go

resource_test.goUpdate resource store tests for Agent model/FK +2/-1

Update resource store tests for Agent model/FK

• Updates migrations and fixtures to include Agent records so FK constraints are satisfied.

internal/placement/store/resource_test.go

evaluation_test.goUpdate OPA parsing tests for agent fields +11/-11

Update OPA parsing tests for agent fields

• Updates tests to validate selected_agent and agent_constraints parsing behavior.

internal/policy/opa/evaluation_test.go

agent_constraints_test.goAdd tests for agent constraint merging/validation +82/-0

Add tests for agent constraint merging/validation

• Adds dedicated coverage for allow list/pattern/environment constraint intersection and validation failures.

internal/policy/service/agent_constraints_test.go

constraints_test.goRemove service-provider constraint tests +0/-80

Remove service-provider constraint tests

• Deletes SP constraint tests now superseded by agent constraint coverage.

internal/policy/service/constraints_test.go

evaluation_test.goUpdate evaluation tests for agent selection +117/-13

Update evaluation tests for agent selection

• Refactors tests to assert SelectedAgent behavior and updated constraint enforcement.

internal/policy/service/evaluation_test.go

scheduler_test.goUpdate cleanup scheduler tests +51/-63

Update cleanup scheduler tests

• Updates tests to reflect agent-only cleanup semantics and simplified deletion completion.

internal/sp/cleanup/scheduler_test.go

consumer_test.goAdjust NATS consumer tests +2/-1

Adjust NATS consumer tests

• Updates existing NATS consumer tests to align with the updated consumer wiring and agent-only behavior.

internal/sp/consumer/consumer_test.go

response_consumer_test.goTest agent response consumption and status mapping +203/-0

Test agent response consumption and status mapping

• Adds tests for CloudEvent parsing, ack/nack behavior, status transitions, and cancel-rejected follow-up publish.

internal/sp/consumer/response_consumer_test.go

handler_test.goUpdate SPRM handler tests for agent-only provisioning +42/-158

Update SPRM handler tests for agent-only provisioning

• Removes provider HTTP mocks and updates migrations to include Agent model for FK constraints.

internal/sp/handlers/resource_manager/handler_test.go

messaging_suite_test.goAdd messaging test suite scaffolding +3/-3

Add messaging test suite scaffolding

• Adds Ginkgo suite setup for messaging/publisher tests.

internal/sp/messaging/messaging_suite_test.go

publisher_test.goTest agent request publishing envelopes +93/-0

Test agent request publishing envelopes

• Validates publish behavior and payload envelope shape for provisioning events.

internal/sp/messaging/publisher_test.go

pending_suite_test.goAdd pending sweep test suite +13/-0

Add pending sweep test suite

• Adds suite scaffolding for the pending/queued sweep component tests.

internal/sp/pending/pending_suite_test.go

sweep_test.goTest pending/queued timeout sweeping +166/-0

Test pending/queued timeout sweeping

• Adds tests for retry exhaustion, retry increments, and queued timeout cancellation logic.

internal/sp/pending/sweep_test.go

service_type_instance_test.goUpdate instance service tests for agent messaging flow +85/-464

Update instance service tests for agent messaging flow

• Refactors tests to remove provider HTTP scaffolding and validate JetStream publisher integration and new deletion semantics.

internal/sp/service/resource_manager/service_type_instance_test.go

service_instance_test.goUpdate service instance store tests for agent FK and deletion completion +2/-153

Update service instance store tests for agent FK and deletion completion

• Updates migrations/fixtures to include Agent records and validates updated deletion status behavior.

internal/sp/store/resource_manager/service_instance_test.go

store_test.goRemove provider accessor assertions from store tests +0/-9

Remove provider accessor assertions from store tests

• Drops tests that asserted provider store access via the SP store interface.

internal/sp/store/store_test.go

service_instance_test.goUpdate SP subsystem tests for agent-only architecture +12/-292

Update SP subsystem tests for agent-only architecture

• Refactors subsystem tests to remove provider setup/assumptions and align with agent-routed instance provisioning lifecycle.

test/subsystem/sp/service_instance_test.go

Other (16) +2450 / -116

MakefileInclude agent make targets +1/-0

Include agent make targets

• Adds wiring so agent codegen/tests can be invoked from the top-level build tooling.

Makefile

spec.gen.cfgConfigure Agent spec generation +1/-1

Configure Agent spec generation

• Adds oapi-codegen configuration for generating the embedded Agent OpenAPI spec helper.

api/agent/v1alpha1/spec.gen.cfg

spec.gen.goGenerated Agent spec loader +147/-0

Generated Agent spec loader

• Generated code to load/serve the Agent OpenAPI spec for runtime validation and tooling.

api/agent/v1alpha1/spec.gen.go

types.gen.cfgConfigure Agent types generation +1/-1

Configure Agent types generation

• Adds oapi-codegen configuration for generating Agent API types.

api/agent/v1alpha1/types.gen.cfg

types.gen.goGenerated Agent API types +166/-0

Generated Agent API types

• Generated Go types for Agent resources and request/response payloads.

api/agent/v1alpha1/types.gen.go

spec.gen.goRegenerate SPRM spec helper +43/-43

Regenerate SPRM spec helper

• Regenerates spec code to match updated SPRM OpenAPI definitions.

api/sp/v1alpha1/resource_manager/spec.gen.go

types.gen.goRegenerate SPRM types (optional provider_name) +3/-10

Regenerate SPRM types (optional provider_name)

• Updates generated SPRM types to reflect optional provider_name and updated deletion_status enum.

api/sp/v1alpha1/resource_manager/types.gen.go

server.gen.cfgConfigure Agent server generation +1/-1

Configure Agent server generation

• Adds oapi-codegen server configuration for the internal strict Agent server interface.

internal/agent/api/server/server.gen.cfg

server.gen.goGenerated Agent API server bindings +912/-0

Generated Agent API server bindings

• Generated chi server/router bindings for Agent endpoints (strict interface).

internal/agent/api/server/server.gen.go

config.goAdd AgentConfig to monolith config +10/-0

Add AgentConfig to monolith config

• Adds agent heartbeat timeout, consumer lag threshold, pending/queued timeouts, and retry configuration.

internal/app/config.go

server.gen.goRegenerate SPRM server for optional provider_name and enum changes +3/-10

Regenerate SPRM server for optional provider_name and enum changes

• Updates generated code to drop PENDING_PROVIDER and make provider_name optional.

internal/sp/api/resource_manager/server.gen.go

config.goRemove provider healthcheck config from SP domain config +4/-13

Remove provider healthcheck config from SP domain config

• Drops HealthCheck configuration from SP config now that provider management is removed.

internal/sp/config/config.go

agent.mkAdd agent codegen and test make targets +37/-0

Add agent codegen and test make targets

• Adds make targets to generate agent OpenAPI types/spec/server/client and run agent tests.

make/agent.mk

sp.mkRemove SP provider codegen targets +4/-37

Remove SP provider codegen targets

• Deletes provider API codegen and spectral targets, leaving only SPRM-related generation/checks.

make/sp.mk

client.gen.cfgConfigure Agent API client generation +8/-0

Configure Agent API client generation

• Adds oapi-codegen configuration for generating the Agent API client package.

pkg/agent/client/client.gen.cfg

client.gen.goGenerated Agent API client +1109/-0

Generated Agent API client

• Generated HTTP client for interacting with the Agent API endpoints.

pkg/agent/client/client.gen.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. Cancel subject misrouted ✗ Dismissed 🐞 Bug ≡ Correctness ⭐ New

Description

pending.Sweep.notifyAgentOfCancel publishes cancel events to agentTopic+".cancel" even though agents
register a full topic_name that is used directly for create/delete publishes. This can prevent
cancel requests from reaching the old agent during self-heal, allowing it to continue acting on an
in-flight request and causing duplicate/stray provisioning.

Code

internal/sp/pending/sweep.go[R328-331]

+	case err == nil:
+		if pubErr := s.publisher.PublishCancel(ctx, subject+".cancel", messaging.CancelPayload{
+			ResourceID:  inst.ID,
+			ServiceType: inst.ServiceType,

Evidence

The cancel path uniquely appends ".cancel" to the resolved agent topic, while the agent API
documents topic_name as the full subject and other request paths (create/delete) publish directly to
that topic. This inconsistency makes cancel routing incorrect relative to the established contract
used elsewhere in the codebase.

internal/sp/pending/sweep.go[321-341]
internal/sp/messaging/publisher.go[55-65]
internal/sp/service/resource_manager/service_type_instance.go[84-99]
internal/sp/service/resource_manager/service_type_instance.go[157-163]
api/agent/v1alpha1/openapi.yaml[247-251]

Agent prompt

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

### Issue description
`pending.Sweep.notifyAgentOfCancel` publishes cancels to `subject + ".cancel"`, but the agent contract and other control-plane publish paths treat `topic_name` as the full NATS subject.

### Issue Context
- `topic_name` is documented as a NATS topic name like `dcm.agent.env-agent-west-1`.
- Instance create/delete publish directly to the resolved `topic_name`.
- Only the cancel notification path appends an extra suffix, likely making the message invisible to agents.

### Fix Focus Areas
- internal/sp/pending/sweep.go[328-333]

### Recommended fix
- Publish cancel to `subject` (the resolved `agent.TopicName`) the same way create/delete do.
- If you intended per-operation subjects (e.g., `.cancel`, `.delete`, `.create`), centralize subject construction in `internal/sp/messaging` and update *all* request publishers and agent `topic_name` semantics accordingly (plus tests/spec docs).

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

---

2. Agent list failure bypass ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

PlacementService logs and continues when ListReadyAgents fails, passing an empty AvailableAgents
list into policy evaluation. Because policy validation explicitly skips membership/environment
checks when availableAgents is empty, agent/environment constraints can be bypassed during
agent-listing failures, leading to misrouted provisioning or repeated failures.

Code

internal/placement/service/placement.go[R94-97]

+		agents, err := s.agentLister.ListReadyAgents(ctx)
+		if err != nil {
+			log.Error("Failed to list available agents", "error", err)
+		} else {

Evidence

Placement swallows agent-listing errors, resulting in an empty AvailableAgents list sent to
policy. Policy validation explicitly documents that empty availableAgents skips membership checks,
and evaluation skips environment validation when the agent environment cannot be looked up from
availableAgents, enabling fail-open constraint bypass.

internal/placement/service/placement.go[92-99]
internal/policy/service/constraints.go[79-91]
internal/policy/service/evaluation.go[231-241]
internal/placement/service/placement.go[363-369]
internal/placement/service/placement.go[514-520]

Agent prompt

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

### Issue description
Placement proceeds even if listing ready agents fails, which turns an operational error into an empty `AvailableAgents` set. The policy layer intentionally treats empty `availableAgents` as “skip membership/environment validation”, so this becomes a fail-open path that can violate placement constraints.

### Issue Context
- `PlacementService.CreateRun` (and also `RehydrateResource` / `ReEvaluateWithExclude`) logs list errors but continues.
- `ConstraintContext.ValidateAgent` only enforces membership when `availableAgents` is non-empty.
- `EvaluatePolicies` skips environment validation if it can't look up the agent environment due to an empty list.

### Fix Focus Areas
- internal/placement/service/placement.go[92-100]
- internal/placement/service/placement.go[363-369]
- internal/placement/service/placement.go[514-520]

### Recommended fix
- Treat `ListReadyAgents` failure as a hard error (return an InternalError) when `agentLister` is configured.
- Keep a successful empty result distinct from a failed query (e.g., return error vs. empty slice).
- Optionally, add tests to ensure placement fails closed on agent-listing errors.

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

---

3. Queued cancel clobbers status ✓ Resolved 🐞 Bug ≡ Correctness

Description

cancelQueuedInstance updates the DB to status="cancelled" even if PublishCancel fails and without
guarding that the instance is still "queued", so a transient publish failure or concurrent status
update can leave the control-plane in an incorrect state. This can also overwrite newer statuses set
by the agent response consumer.

Code

internal/sp/pending/sweep.go[194]

+	s.db.WithContext(ctx).Model(inst).Update("status", "cancelled")

Evidence

The sweeper always performs a DB status update to "cancelled" after attempting publish, even on
publish error, and it does so without a conditional predicate; meanwhile ResponseConsumer updates
statuses based on incoming agent events, so this update can race/overwrite or create DB/agent
divergence when publish fails.

internal/sp/pending/sweep.go[182-195]
internal/sp/consumer/response_consumer.go[143-175]

Agent prompt

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

## Issue description
`cancelQueuedInstance` marks instances as `cancelled` unconditionally. If `PublishCancel` fails (or another component updates status between query and update), the DB state can diverge from reality or clobber a newer state.

## Issue Context
The queued sweep runs concurrently with the agent response consumer, which also updates instance statuses based on agent events.

## Fix Focus Areas
- internal/sp/pending/sweep.go[182-195]

## Suggested fix
- Only transition to `cancelled` when the cancel publish succeeds.
- Make the DB update conditional (e.g., `WHERE id = ? AND status = 'queued'`) so it cannot overwrite a newer status.
- If publish fails, leave status as `queued` so the next sweep cycle can retry the cancel publish (or introduce an explicit `cancel_requested` intermediate state if you need to separate “requested” from “acknowledged”).

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

---

View high (3)

4. No agent delete requests ✓ Resolved 🐞 Bug ≡ Correctness

Description

DeleteInstance and the cleanup scheduler finalize deletions purely in the DB (hard delete or set
deletion_status=DELETED) without publishing any delete/cancel request to an agent, which can leak
real provisioned resources under agent-only provisioning.

Code

internal/sp/service/resource_manager/service_type_instance.go[R147-165]

	if deferred {
		if instance.DeletionStatus != nil {
-			// Already marked — reset retry count so scheduler picks it up again
			if resetErr := s.store.ServiceTypeInstance().ResetRetryCount(ctx, instanceID); resetErr != nil {
-				log.Error("Failed to reset retry count for instance", "instance_id", instanceID, "error", resetErr)
+				log.Error("Failed to reset retry count", "instance_id", instanceID, "error", resetErr)
			}
		} else {
-			// Mark as pending deletion
			if markErr := s.store.ServiceTypeInstance().MarkForDeletion(ctx, instanceID); markErr != nil {
				return service.NewInternalError(fmt.Sprintf("failed to mark instance %s for deletion: %v", instanceID, markErr))
			}
		}
-
-		log.Info("Scheduled deferred deletion of instance from provider", "instance_id", instance.ID, "provider_name", instance.ProviderName)
+		log.Info("Scheduled deferred deletion", "instance_id", instanceID)
		return nil
	}

-	// Non-deferred: attempt SP deletion and DB hard-delete
-	deleteErr := s.DeleteFromProvider(ctx, instance)
-	if deleteErr == nil {
-		return nil
+	if err = s.store.ServiceTypeInstance().HardDelete(ctx, instanceID); err != nil {
+		return service.NewInternalError(fmt.Sprintf("failed to delete instance %s: %v", instanceID, err))
	}

-	log.Error(
-		"Failed to delete instance from provider",
-		"instance_id", instance.ID,
-		"provider_name", instance.ProviderName,
-		"error", deleteErr,
-	)
-
-	// For already-pending/failed instances, reset retry count so scheduler picks it up again
-	if instance.DeletionStatus != nil {
-		if resetErr := s.store.ServiceTypeInstance().ResetRetryCount(ctx, instanceID); resetErr != nil {
-			log.Error("Failed to reset retry count for instance", "instance_id", instanceID, "error", resetErr)
-		}
-	}
-	return service.NewProviderError(fmt.Sprintf("failed to delete instance (%s): %v", instanceID, deleteErr))
-}
-
-// DeleteFromProvider deletes the instance from its service provider and, on
-// success, hard-deletes the database record.
-func (s *InstanceService) DeleteFromProvider(ctx context.Context, instance *model.ServiceTypeInstance) error {
-	log := logging.FromContext(ctx)
-	log.Debug("Deleting instance from provider", "instance_id", instance.ID, "provider_name", instance.ProviderName)
-
-	provider, err := s.store.Provider().GetByName(ctx, instance.ProviderName)
-	if err != nil {
-		if errors.Is(err, providerstore.ErrProviderNotFound) {
-			return fmt.Errorf("provider '%s' not found", instance.ProviderName)
-		}
-		return fmt.Errorf("failed to retrieve provider: %w", err)
-	}
-
-	if err = s.deleteInstanceWithProvider(ctx, provider.Endpoint, instance.ID); err != nil {
-		return err
-	}
-
-	log.Info("Instance deleted successfully",
-		"instance_id", instance.ID,
-		"provider_name", instance.ProviderName,
-	)
-
-	if err = s.store.ServiceTypeInstance().HardDelete(ctx, instance.ID); err != nil {
-		return fmt.Errorf("failed to delete database record for instance %s: %w", instance.ID, err)
-	}
-
-	log.Info("Deleted instance from DB record", "instance_id", instance.ID)
+	log.Info("Instance deleted", "instance_id", instance.ID)

Evidence

The updated delete path contains no publisher calls and the scheduler now directly marks DELETED;
meanwhile, delete publishing exists only in the cancel-rejected handler, meaning standard deletes do
not trigger an agent request.

internal/sp/service/resource_manager/service_type_instance.go[134-166]
internal/sp/cleanup/scheduler.go[76-85]
internal/sp/consumer/response_consumer.go[159-178]

Agent prompt

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

## Issue description
The deletion flow no longer contacts an external provisioner: `DeleteInstance` either hard-deletes the instance row or marks it for deferred cleanup, and the cleanup scheduler then marks `deletion_status` as `DELETED` without sending any agent delete/cancel request. In an agent-owned provisioning model this can leave external resources running indefinitely.

## Issue Context
- The only `PublishDelete` call in the codebase is in the response consumer’s cancel-rejected handler, which is not the normal delete path.
- The scheduler explicitly logs “Agent-routed instance cleanup: marking DELETED” but does not publish anything.

## Fix Focus Areas
- internal/sp/service/resource_manager/service_type_instance.go[134-167]
- internal/sp/cleanup/scheduler.go[76-85]
- internal/sp/consumer/response_consumer.go[159-178]

## Implementation sketch
1. Inject `messaging.Publisher` into the cleanup scheduler (and/or into `InstanceService.DeleteInstance`).
2. On delete/deferred cleanup, publish a `dcm.request.delete` (or cancel) CloudEvent to the correct agent topic using the instance’s `AgentName`/agent record.
3. Do not mark `deletion_status=DELETED` (or hard-delete) until an agent acknowledgment event is received via `ResponseConsumer` (or implement a robust reconciliation mechanism).
4. Add retry/backoff behavior for failed publish and for missing AgentName (treat as error or fallback routing).

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

---

5. Pending sweep no retries ✓ Resolved 🐞 Bug ☼ Reliability

Description

The pending/queued sweep filters on pending_started_at and agent_name, but instance creation does
not set PendingStartedAt/AgentName, and the sweep only increments retry_count without re-publishing
or otherwise retrying timed-out requests.

Code

internal/sp/pending/sweep.go[R63-80]

+func (s *Sweep) sweepPending() {
+	cutoff := time.Now().Add(-s.pendingTimeout)
+	var instances []model.ServiceTypeInstance
+	err := s.db.Where("status = ? AND pending_started_at < ? AND agent_name IS NOT NULL", "pending", cutoff).
+		Find(&instances).Error
+	if err != nil {
+		slog.Error("sweep: failed to query pending instances", "error", err)
+		return
+	}
+
+	for i := range instances {
+		inst := &instances[i]
+		if inst.RetryCount >= s.maxRetries {
+			s.db.Model(inst).Updates(map[string]any{"status": "failed", "status_message": "retries exhausted"})
+			continue
+		}
+		s.db.Model(inst).Update("retry_count", gorm.Expr("retry_count + 1"))
+	}

Evidence

Sweep explicitly requires pending_started_at and agent_name, but CreateInstance does not set
them; and the sweep implementation only updates DB fields (retry_count/status) without any publish
call, so it cannot re-drive provisioning on its own.

internal/sp/pending/sweep.go[63-96]
internal/sp/service/resource_manager/service_type_instance.go[50-56]
internal/sp/store/model/service_type_instance.go[20-24]

Agent prompt

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

## Issue description
The new sweep queries for timed-out `pending`/`queued` instances by `pending_started_at` and `agent_name`, but `CreateInstance` does not populate those fields. Even for rows it does find, the sweep only increments `retry_count` (or flips to failed) and never re-publishes create/cancel/delete requests, so it cannot actually recover stuck provisioning.

## Issue Context
- `ServiceTypeInstance` now has `AgentName` and `PendingStartedAt`.
- `CreateInstance` sets `Status: "pending"` but does not set `PendingStartedAt` or `AgentName`.

## Fix Focus Areas
- internal/sp/pending/sweep.go[63-96]
- internal/sp/service/resource_manager/service_type_instance.go[50-56]
- internal/sp/store/model/service_type_instance.go[20-24]

## Implementation sketch
1. When creating a pending instance, set `PendingStartedAt = time.Now()` and persist `AgentName`.
2. Decide what a "retry" means: either re-publish the create request to the agent topic, or transition status to a state consumed by a worker that republishes.
3. In the sweep, on timeout and under max retries, perform the retry action (publish + update `pending_started_at` and `retry_count`).
4. Add tests asserting (a) instances become eligible for sweep and (b) sweep triggers a retry action.

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

---

6. Agent selection dropped ✓ Resolved 🐞 Bug ≡ Correctness

Description

PlacementService sets sprmRequest.AgentName from the policy decision, but the SPRM in-process
adapter drops AgentName and InstanceService publishes create events to a subject derived from
service_type, so provisioning won’t be routed to the selected agent and
ServiceTypeInstance.AgentName remains unset.

Code

internal/placement/service/placement.go[R117-121]

	sprmRequest := sprm.CreateResourceRequest{
-		ID:           resourceIDStr,
-		Spec:         policyResponse.EvaluatedSpec,
-		ProviderName: providerName,
+		ID:        resourceIDStr,
+		Spec:      policyResponse.EvaluatedSpec,
+		AgentName: policyResponse.SelectedAgent,
	}

Evidence

Placement is now explicitly selecting an agent and passing it to SPRM, but the SPRM adapter only
forwards ProviderName/Spec and the instance service publishes to a subject built from
service_type, so the selected agent is never used and AgentName is not persisted.

internal/placement/service/placement.go[117-121]
internal/placement/sprm/types.go[9-15]
internal/placement/sprm/service_client.go[23-31]
internal/sp/service/resource_manager/service_type_instance.go[50-74]
internal/sp/store/model/service_type_instance.go[20-24]
internal/agent/service/agent.go[28-31]

Agent prompt

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

## Issue description
Placement produces an agent decision (`SelectedAgent`) and passes it to SPRM, but the SPRM adapter ignores `AgentName` and the instance service publishes create requests to `dcm.agent.<serviceType>` instead of the selected agent’s topic. This breaks agent-only routing and also leaves `ServiceTypeInstance.AgentName` unset, which later logic relies on.

## Issue Context
- `sprm.CreateResourceRequest` already has `AgentName`, and placement sets it.
- The in-process SPRM adapter builds an `api/sp` `ServiceTypeInstance` body without `AgentName`, so the instance service cannot persist it.
- Agent registration stores agent-specific topics prefixed with `dcm.agent.`, suggesting requests should be published to the selected agent’s topic.

## Fix Focus Areas
- internal/placement/sprm/service_client.go[23-31]
- internal/placement/service/placement.go[117-121]
- internal/sp/service/resource_manager/service_type_instance.go[50-74]
- internal/sp/store/model/service_type_instance.go[20-24]
- internal/agent/service/agent.go[28-31]

## Implementation sketch
1. Extend the SPRM adapter request/body so `AgentName` is forwarded into the SP instance creation path (either by adding `agent_name` to the SP RM API types or by passing it out-of-band in the local adapter).
2. Persist `AgentName` on `model.ServiceTypeInstance` during creation.
3. Publish create requests to the selected agent’s topic (e.g., use the agent’s registered `TopicName`, or build `dcm.agent.<agentName>` consistently), not `service_type`.
4. Add a regression test asserting that `SelectedAgent` flows from placement → SPRM adapter → instance record → publish subject.

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

---

7. claimRetry ignores DB errors ✓ Resolved 🐞 Bug ☼ Reliability

Description

claimRetry returns success based only on RowsAffected and never checks result.Error, so DB update
failures are treated the same as "not claimed" and the caller logs a misleading "already claimed"
message. This masks operational DB failures and can stall retries without clear diagnostics.

Code

internal/sp/pending/sweep.go[R158-161]

+	result := s.db.WithContext(ctx).Model(&model.ServiceTypeInstance{}).
+		Where("id = ? AND status = ?", id, "pending").
+		Updates(map[string]any{
+			"retry_count":        gorm.Expr("retry_count + 1"),

Evidence

claimRetry does an Updates() call but returns only RowsAffected>0; retryPendingInstance treats false
as "already claimed". If Updates() fails (DB outage, etc.), RowsAffected may be 0 and the error is
silently dropped and misclassified.

internal/sp/pending/sweep.go[155-165]
internal/sp/pending/sweep.go[102-105]

Agent prompt

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

## Issue description
`claimRetry` ignores `result.Error`, so DB errors are indistinguishable from normal 0-row outcomes.

## Issue Context
Callers treat `false` as a benign concurrency outcome and log accordingly.

## Fix Focus Areas
- internal/sp/pending/sweep.go[155-165]
- internal/sp/pending/sweep.go[102-105]

## Suggested fix
- Change `claimRetry` signature to return `(bool, error)`.
- If `result.Error != nil`, return `(false, err)` and log at error level in the caller.
- Keep the existing `RowsAffected > 0` check for the benign "someone else moved it" case.

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

---

8. Agent update hides DB errors ✓ Resolved 🐞 Bug ☼ Reliability

Description

AgentStore.Update ignores the error from the existence-check Count() query, so a DB error can be
misclassified as ErrAgentNotFound. This can cause callers to treat operational failures as “not
found” and apply incorrect error handling.

Code

internal/agent/store/agent/agent.go[R97-103]

+func (s *AgentStore) Update(ctx context.Context, agent model.Agent) (*model.Agent, error) {
+	var count int64
+	s.db.WithContext(ctx).Model(&model.Agent{}).Where("id = ?", agent.ID).Count(&count)
+	if count == 0 {
+		return nil, ErrAgentNotFound
+	}
+	if err := s.db.WithContext(ctx).Save(&agent).Error; err != nil {

Evidence

The Count() call’s returned *gorm.DB is ignored, so Update can return ErrAgentNotFound purely due to
an unhandled query error.

internal/agent/store/agent/agent.go[97-107]

Agent prompt

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

### Issue description
`AgentStore.Update` calls `Count(&count)` but does not check the returned `.Error`. If the query fails, `count` may remain 0 and the function returns `ErrAgentNotFound` instead of the real DB error.

### Issue Context
This makes failures harder to debug and can lead to incorrect control flow in the service/handler layer.

### Fix Focus Areas
- internal/agent/store/agent/agent.go[97-106]

### What to change
- Capture and return the error from the count query:
 - `tx := s.db.WithContext(ctx).Model(...).Where(...); if err := tx.Count(&count).Error; err != nil { return nil, err }`
- Alternatively, remove the pre-count and use an update/save approach that checks `RowsAffected` plus `Error` in one operation.

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

---

9. Sweep counts skipped retries ✓ Resolved 🐞 Bug ☼ Reliability

Description

pending.Sweep.sweepPending increments retry_count and refreshes pending_started_at before verifying
it can republish (publisher configured, agent ready, agent topic resolvable), so instances can reach
maxRetries and be marked failed even when no publish was attempted. This can cause false “retries
exhausted” failures when agents are temporarily unavailable/misconfigured or when publisher is nil.

Code

internal/sp/pending/sweep.go[R84-119]

+		if inst.RetryCount >= s.maxRetries {
+			s.db.Model(&model.ServiceTypeInstance{}).
+				Where("id = ? AND status = ?", inst.ID, "pending").
+				Updates(map[string]any{"status": "failed", "status_message": "retries exhausted"})
+			slog.Info("sweep: pending instance retries exhausted", "instance_id", inst.ID)
+			continue
+		}
+
+		now := time.Now()
+		claim := s.db.Model(&model.ServiceTypeInstance{}).
+			Where("id = ? AND status = ?", inst.ID, "pending").
+			Updates(map[string]any{
+				"retry_count":        gorm.Expr("retry_count + 1"),
+				"pending_started_at": now,
+			})
+		if claim.RowsAffected == 0 {
+			continue
+		}
+
+		if s.publisher != nil && inst.AgentName != nil {
+			if !s.isAgentReady(ctx, *inst.AgentName) {
+				slog.Info("sweep: agent not ready, skipping re-publish", "instance_id", inst.ID, "agent_name", *inst.AgentName)
+				continue
+			}
+			subject, ok := s.resolveSubject(ctx, *inst.AgentName)
+			if !ok {
+				continue
+			}
+			if pubErr := s.publisher.PublishCreate(ctx, subject, messaging.CreatePayload{
+				ResourceID:  inst.ID,
+				ServiceType: inst.ServiceType,
+				Spec:        inst.Spec,
+			}); pubErr != nil {
+				slog.Error("sweep: re-publish failed", "instance_id", inst.ID, "error", pubErr)
+			}
+		}

Evidence

The code marks instances failed when retry_count exceeds maxRetries, but also increments retry_count
before checking publisher/agent readiness and before attempting PublishCreate; early-continue paths
therefore still consume retries without a publish attempt.

internal/sp/pending/sweep.go[84-101]
internal/sp/pending/sweep.go[92-119]

Agent prompt

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

### Issue description
`sweepPending()` consumes retries (and advances `pending_started_at`) before it knows it can actually republish. This can mark instances as failed due to retry exhaustion even when no NATS publish occurred.

### Issue Context
Current sequence:
1) If `retry_count >= maxRetries` -> mark failed.
2) Else unconditionally `retry_count++` and `pending_started_at = now`.
3) Only then check `publisher != nil`, agent readiness, and topic resolution; on failure it `continue`s without publishing.

### Fix Focus Areas
- internal/sp/pending/sweep.go[84-119]

### What to change
- Reorder logic so that you only increment `retry_count` / update `pending_started_at` when you are going to attempt a publish (or after a publish attempt).
- Consider separate handling for:
 - `publisher == nil`: skip without consuming retries.
 - agent not ready / agent not found / topic resolution failure: decide whether to skip without consuming retries, or transition to a terminal/explicit status (but don’t count as a publish retry unless that’s intended).
- Optionally pass a cancellable context through sweep instead of `context.Background()` to avoid long operations during shutdown.

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

---

View medium (2)

10. Poison response redelivery ✓ Resolved 🐞 Bug ☼ Reliability

Description

ResponseConsumer NAKs on any UpdateStatus error (including instance-not-found) and uses
context.Background() per message, so malformed/out-of-order events can redeliver indefinitely and
the consumer can’t be cleanly cancelled via the Start() context.

Code

internal/sp/consumer/response_consumer.go[R149-153]

+	stiStore := c.store.ServiceTypeInstance()
+	if err := stiStore.UpdateStatus(ctx, data.ResourceID, newStatus, ""); err != nil {
+		slog.Error("failed to update status, nacking", "resource_id", data.ResourceID, "error", err)
+		_ = msg.NakWithDelay(5 * time.Second)
+		return

Evidence

The consumer creates context.Background() and NAKs when UpdateStatus fails; UpdateStatus returns
ErrInstanceNotFound when the row is missing, so those messages will be retried indefinitely rather
than discarded.

internal/sp/consumer/response_consumer.go[122-153]
internal/sp/store/resource_manager/service_instance.go[158-172]

Agent prompt

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

## Issue description
`ResponseConsumer.handleMessage` creates a background context and NAKs on any status update error. Since `UpdateStatus` returns `ErrInstanceNotFound` when no row exists, responses for unknown/deleted IDs will be retried forever, potentially backing up the response stream.

## Issue Context
- The consumer already ACKs and discards malformed JSON; it should also treat non-retryable store errors similarly.
- Using `context.Background()` ignores cancellation/shutdown semantics.

## Fix Focus Areas
- internal/sp/consumer/response_consumer.go[122-153]
- internal/sp/store/resource_manager/service_instance.go[158-172]

## Implementation sketch
1. Use a context derived from `Start(ctx)` (store it on the consumer or pass it into `consumeLoop/handleMessage`).
2. If `UpdateStatus` returns `ErrInstanceNotFound`, ACK the message (or send to a dead-letter subject) instead of NAK.
3. Consider bounding delivery attempts via JetStream consumer config (MaxDeliver) and/or emitting metrics/logs for poison messages.

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

---

11. Request stream provisioning gap ✓ Resolved 🐞 Bug ☼ Reliability

Description

The app constructs a JetStream Publisher and publishes agent requests, but no production code
creates the request stream/binding used by the publisher; if the stream isn’t pre-provisioned,
PublishCreate/PublishDelete will fail and instances will remain "pending" because publish errors are
only logged.

Code

internal/app/run.go[R140-143]

+		publisher = messaging.NewPublisher(agentJS)
+
+		responseConsumer := spconsumer.NewResponseConsumer(agentJS, spDataStore)
+		if err := responseConsumer.Start(ctx); err != nil {

Evidence

Run() creates a Publisher but does not create any JetStream stream; Publisher.publish relies on
js.Publish; and the publisher unit test must create the stream explicitly before publish calls
succeed.

internal/app/run.go[112-148]
internal/sp/messaging/publisher.go[31-44]
internal/sp/messaging/publisher_test.go[45-52]
internal/sp/service/resource_manager/service_type_instance.go[64-74]

Agent prompt

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

## Issue description
`Run()` wires up `messaging.Publisher` and uses it to publish CloudEvents via JetStream, but it never creates the agent-request stream (e.g., `dcm-agent-requests` / `dcm.agent.>`). If deployments do not guarantee external stream provisioning, request publishing will fail at runtime; the instance service logs the publish error and still returns a created instance stuck in `pending`.

## Issue Context
- `Publisher.publish` uses `js.Publish` directly.
- The unit test explicitly creates the stream before publishing.
- In `CreateInstance`, publish failure does not fail the request or change instance status.

## Fix Focus Areas
- internal/app/run.go[112-148]
- internal/sp/messaging/publisher.go[31-44]
- internal/sp/messaging/publisher_test.go[45-52]
- internal/sp/service/resource_manager/service_type_instance.go[64-74]

## Implementation sketch
Option A (recommended): On startup (when NATS enabled), `CreateOrUpdateStream` for `messaging.StreamName` with `messaging.StreamSubjectBinding`.
Option B: Make `NewPublisher` (or a separate initializer) idempotently ensure the stream exists.
Also consider: if publish fails, update instance status/message to a terminal/visible failure state or enqueue for retry so it does not stay `pending` silently.

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

---

12. Agent readiness test bypassed ⊘ Outdated 🐞 Bug ⚙ Maintainability

Description

The "does not consume retries when agent is not ready" test passes a nil publisher, so
readyAgentSubject returns early on the publisher guard and never exercises the agent health-status
check. This reduces test coverage for the agent-readiness behavior.

Code

internal/sp/pending/sweep_test.go[R78-80]

+		agentSt := agentstore.NewAgent(db)
+		sweep = pending.NewSweep(db, nil, agentSt, 30*time.Second, 60*time.Second, 5*time.Second, 3)
+		ctx = context.Background()

Evidence

readyAgentSubject returns false immediately when publisher is nil, before checking agent health, and
the test constructs Sweep with a nil publisher while marking the agent unavailable; therefore the
agent health path is not executed by the test.

internal/sp/pending/sweep.go[123-144]
internal/sp/pending/sweep_test.go[73-91]

Agent prompt

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

## Issue description
The test intended to cover "agent not ready" currently exits via the earlier `publisher == nil` guard.

## Issue Context
`readyAgentSubject` checks publisher nil before checking agent health.

## Fix Focus Areas
- internal/sp/pending/sweep_test.go[73-92]
- internal/sp/pending/sweep.go[120-147]

## Suggested fix
- Either rename the test to reflect what it actually covers (publisher not configured), OR
- Provide a real publisher (e.g., start an embedded NATS JetStream like `internal/sp/messaging/publisher_test.go` does) and assert that when agent health is not READY, no message is published and retry_count is unchanged.

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

---

Tip of the day

💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[jordigilh]

This PR removes pkg/sp/client/provider, internal/sp/api/provider, internal/sp/healthcheck, and the direct-HTTP createInstanceWithProvider dispatch path, replacing Create with a fire-and-forget NATS publish to dcm.agent.<service_type>.

Flagging a downstream collision: osac-service-provider is mid-flight on Phase 1, built directly against exactly what's being removed here — it imports pkg/sp/client/provider for registration (internal/registration/registration.go), and its two-health-endpoint design (DD-010) exists specifically because internal/sp/healthcheck.Monitor polls each registered provider's {endpoint}/health. That Phase-1 choice was deliberate — see DD-050 in osac-sp.spec.md and enhancements#95 — precisely because environment-agent's registration handler was judged too immature for a first release.

Is this migration coordinated with osac-service-provider (and any other SP currently on the direct-HTTP provider model)? If pkg/sp/client/provider is going away, Phase-1 SPs will need a heads-up and a migration path before this merges, since it'd break their registration/health-check integration outright.

[gabriel-farache]

/agentic_review

[gabriel-farache]

This PR removes pkg/sp/client/provider, internal/sp/api/provider, internal/sp/healthcheck, and the direct-HTTP createInstanceWithProvider dispatch path, replacing Create with a fire-and-forget NATS publish to dcm.agent.<service_type>.

Flagging a downstream collision: osac-service-provider is mid-flight on Phase 1, built directly against exactly what's being removed here — it imports pkg/sp/client/provider for registration (internal/registration/registration.go), and its two-health-endpoint design (DD-010) exists specifically because internal/sp/healthcheck.Monitor polls each registered provider's {endpoint}/health. That Phase-1 choice was deliberate — see DD-050 in osac-sp.spec.md and enhancements#95 — precisely because environment-agent's registration handler was judged too immature for a first release.

Is this migration coordinated with osac-service-provider (and any other SP currently on the direct-HTTP provider model)? If pkg/sp/client/provider is going away, Phase-1 SPs will need a heads-up and a migration path before this merges, since it'd break their registration/health-check integration outright.

@jordigilh This PR will be ready to merge only once the PR on https://github.com/dcm-project/environment-agent will all be merged and that the agent behaviour will have been validated standalone, before that, we can't merge this PR as there would be no agent ready to consume and reply

[qodo-code-review]

Code review by qodo was updated up to the latest commit 9ac411a

[gabriel-farache]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 1d8fb4d

[jordigilh]

Re-reviewed after bf449ad. All 4 open threads from my side are addressed with real subsystem coverage (retry-exhaustion path, and 3 sibling-reassignment scenarios against the real NATS sweep + CAS) — resolved them. Also ran a broadened 68-linter sweep (dupl/goconst/gocyclo/err113/forcetypeassert/gosec/mnd/etc., generated code excluded) scoped to this PR's diff: 0 issues.

Approving. Two non-blocking-for-review items before merge though:

• DCO is failing: a7e583c is missing Signed-off-by.
• auth-subsystem / blackbox shows cancelled (stalled at Keycloak startup, timed out) — passed on 2f32c37 two commits back and this commit only touches test/subsystem/sp/*, so looks like infra flake, not a regression. Worth a re-run before merge.

[gabriel-farache]

@jordigilh I'll rewrite commit history to fix DCO and the CI will re-run so I hope all will pass

[gabriel-farache]

dcm-project/shared-workflows#32 to try to debug why the failing CI cancels and is not progressing

[jordigilh]

TOCTOU: a completed deletion can be silently regressed to FAILED

MarkDeletionCompleteFromAgent (and MarkDeletionComplete above it) correctly update deletion_status unconditionally to DELETED once the DB accepts the row — good, that part is atomic and race-free on its own.

The problem is MarkDeletionFailed (a few lines above, unchanged by this PR):

func (s *ServiceTypeInstanceStore) MarkDeletionFailed(ctx context.Context, id string) error {
	result := s.db.WithContext(ctx).
		Model(&model.ServiceTypeInstance{}).
		Where("id = ?", id).
		Update("deletion_status", DeletionStatusFailed)
	...
}

Unlike every other transition here, MarkDeletionFailed has no guard on the current deletion_status — it will happily stamp FAILED over a row that a concurrent goroutine just marked DELETED.

Concrete race, now made easier to hit by this PR's new agent-scoped completion path:

1. Deletion is in flight; some retry/backoff loop is about to time out and call MarkDeletionFailed(id).
2. Concurrently, the CE-driven handler receives the real completion event and calls MarkDeletionCompleteFromAgent(id, agentName) → row is now DELETED.
3. Step 1's MarkDeletionFailed call, still in flight, lands after step 2 and overwrites deletion_status back to FAILED — with no error, since RowsAffected is 1 either way.

Net effect: an instance that's actually deleted (and possibly already GC'd downstream) reports FAILED forever, since nothing re-checks deletion_status afterward.

Suggested fix: give MarkDeletionFailed the same guard pattern used elsewhere — e.g. Where("id = ? AND deletion_status NOT IN (?)", id, []string{"DELETED"}) — and treat RowsAffected == 0 as "already resolved, not an error" the same way MarkDeletionComplete/MarkDeletionCompleteFromAgent treat it via ErrInstanceNotFound.

[gciavarrini]

Addressed here 1ccdf43

[jordigilh]

Minor: duplicate-key errors from Create aren't classified as ConflictError

Not a correctness bug (the DB's primary key still prevents any actual duplicate row/corruption), but worth flagging alongside the other TOCTOU-adjacent findings on this file: Create returns whatever error GORM/the driver surfaces for a duplicate-ID insert as-is, unclassified.

func (s *ServiceTypeInstanceStore) Create(ctx context.Context, instance model.ServiceTypeInstance) (*model.ServiceTypeInstance, error) {
	operation := func() (*model.ServiceTypeInstance, error) {
		if err := s.db.WithContext(ctx).Clauses(clause.Returning{}).Create(&instance).Error; err != nil {
			return nil, err
		}
		return &instance, nil
	}

	return backoff.Retry(ctx, operation, s.retryOptsFunc()...)
}

Two consequences:

• Callers get a generic 500 instead of a 409 Conflict for what's actually a client-side "this ID already exists" error.
• backoff.Retry will retry a unique-constraint violation several times before giving up, since nothing marks it non-retryable — pure wasted latency/DB load, since retrying can't ever make a duplicate-key error succeed.

Suggested fix: detect the duplicate-key case (e.g. errors.Is/driver-specific code check, same idea as apierrors.IsAlreadyExists used in the k8s-based SPs) and either return a ConflictError directly or mark it non-retryable for backoff.Retry, mirroring how MarkDeletionFailed's sibling methods already special-case "not found" via ErrInstanceNotFound.

[gciavarrini]

Addressed here a546782

[jordigilh]

Pre-merge blocker: all 4 subsystem CI jobs are pinned to a diagnostic branch, not main

This line (and the identical pin on policy-subsystem, catalog-subsystem, sp-subsystem below it) points at dcm-project/shared-workflows/.github/workflows/black-box.yaml@fix/blackbox-timeout-diagnostics instead of @main. The commit that introduced this (0ad5a1a) is itself titled Tmp debug: subsystem black-box jobs now use @fix/blackbox-timeout-diagnostics, so this looks like debugging scaffolding rather than an intended permanent change.

Merging with this in place means every subsystem job on main (and release/v*) depends on someone else's WIP branch in shared-workflows — if that branch is force-pushed, rebased, or deleted after merge, this repo's CI breaks with no local signal why. Please revert these 4 lines back to @main (or to whatever tag/SHA shared-workflows lands the timeout fix on) before merging.

Flagging this as the one outstanding item from re-auditing the diff since the last approval — everything else since then (bf449ad's new subsystem tests, the setup_test.go fixture-embedding change) checked out fine.

[gciavarrini]

diagnostic branch removed 78fe04e

[jenniferubah]

All my comments are for follow up smaller scoped PRs as this PR is already too large.