feat: persist agent provider error details

[gabriel-farache]

Narrow down from #78

Currently, the Control-Plane completely drops the error details it received from the agent (this was found when doing dcm-project/environment-agent#53) so the root cause is actually lost to the user and to the admin as well.
This PR:

• Decodes agent error events and log their provider error details.
• Persists the failure reason and structured error data for inspection.

Managing of the delete process to record the error is out of the scope of this PR, it may be addressed in a later one once the deletion process is unified

Changing the API to show the error to the user is also out of the scope of this PR

[qodo-code-review]

PR Summary by Qodo

Persist agent failure reasons and log provider diagnostics

✨ Enhancement 🧪 Tests 🕐 10-20 Minutes

AI Description

• Decode agent error details so provider failures retain a useful reason.
• Save the best available failure message on the instance and log structured diagnostics.
• Test message fallbacks, unchanged non-error behavior, and rejection of superseded-agent events.

Diagram

graph TD
  A["Agent events"] --> B["Response consumer"] --> C{"Error event?"} -->|Yes| D["Select failure reason"] --> E["Diagnostic logs"] --> F["Instance status"]
  C -->|No| F

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Persist structured diagnostics in a JSON column

• ➕ Retains provider status codes and other details for durable inspection and querying.
• ➖ Requires a schema change and decisions about exposing and retaining provider data.

Recommendation: Using the existing status_message field is the smaller change for surfacing a failure reason. The PR logs structured provider diagnostics but does not persist them as structured data; use a dedicated column if durable inspection of status codes and other details is a requirement.

Files changed (2) +120 / -8

Enhancement (1) +31 / -2

response_consumer.goExtract and record agent failure details +31/-2

Extract and record agent failure details

• Decodes error classification, detail message, and provider error fields from agent events. For error events, logs structured diagnostics and passes the best available message to the existing guarded status update.

internal/sp/consumer/response_consumer.go

Tests (1) +89 / -6

response_consumer_test.goCover failure-message persistence and diagnostic logging +89/-6

Cover failure-message persistence and diagnostic logging

• Adds integration coverage for provider diagnostics, message fallbacks, non-error status messages, and rejection of a superseded agent's error. Extends event-publishing helpers to supply error payloads.

internal/sp/consumer/response_consumer_test.go

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Users cannot see failure reasons ✗ Dismissed

Description

handleMessage passes the selected provider message to UpdateStatusFrom, but ModelToAPI does
not include StatusMessage and the instance response schema has no such field. After an accepted
failure event, get and list requests therefore return the failed status without the reason this
change records.

Code

internal/sp/consumer/response_consumer.go[257]

+	applied, err := stiStore.UpdateStatusFrom(ctx, data.ResourceID, fromStatuses, data.AgentName, newStatus, statusMessage)

Evidence

The changed call persists the message through the store, while the conversion used by get and list
omits it and the API representation has no field for it.

internal/sp/consumer/response_consumer.go[228-257]
internal/sp/store/resource_manager/service_instance.go[214-231]
internal/sp/service/resource_manager/convert.go[12-30]
internal/sp/service/resource_manager/service_type_instance.go[204-212]
internal/sp/service/resource_manager/service_type_instance.go[260-275]
api/sp/v1alpha1/resource_manager/types.gen.go[54-89]

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 consumer persists a failure reason that SP clients cannot retrieve.
## Fix Focus Areas
- internal/sp/consumer/response_consumer.go[231-257]
- internal/sp/service/resource_manager/convert.go[12-30]
- api/sp/v1alpha1/resource_manager/openapi.yaml[304-365]
## Recommended Fix
Add a read-only status message to the instance API schema, regenerate the API types, map the stored value in ModelToAPI, and test a get response after an agent failure.

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

---

2. Rejected agent errors pollute logs ✗ Dismissed

Description

handleMessage emits agent reported error with provider diagnostics before UpdateStatusFrom
checks the current agent and allowed status. A stale or duplicate event is logged as an error even
when the transition is rejected, and a database failure repeats that log on each delivery retry.

Code

internal/sp/consumer/response_consumer.go[246]

+		slog.Error("agent reported error", attrs...)

Evidence

The new error log precedes the store call. The store atomically rejects agent or status mismatches,
and the existing stale-agent test demonstrates such an event can reach this branch without changing
the instance.

internal/sp/consumer/response_consumer.go[228-275]
internal/sp/store/resource_manager/service_instance.go[214-231]
internal/sp/consumer/response_consumer_test.go[267-281]

Agent prompt

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

## Issue description
Error diagnostics are emitted before the consumer knows whether the event was accepted.
## Fix Focus Areas
- internal/sp/consumer/response_consumer.go[239-275]
- internal/sp/consumer/response_consumer_test.go[267-281]
## Recommended Fix
Emit the provider-diagnostic error log only when UpdateStatusFrom returns applied=true. Keep rejected and retryable events on their existing distinct logging paths, and test that stale events do not produce an accepted-error log.

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

---

3. Provider error codes are not retained ✗ Dismissed

Description

handleMessage logs Details.ProviderError.StatusCode and the error classification but passes only
a single message string to UpdateStatusFrom. When an accepted event contains a provider status
code, the instance record retains neither that code nor the classification, so they cannot be
inspected from persisted failure data.

Code

internal/sp/consumer/response_consumer.go[R240-244]

+		if data.Details.ProviderError.StatusCode != nil {
+			attrs = append(attrs, "provider_status_code", data.Details.ProviderError.StatusCode)
+		}
+		if data.Details.ProviderError.Message != "" {
+			attrs = append(attrs, "provider_error_message", data.Details.ProviderError.Message)

Evidence

The newly decoded code and classification appear in the log attributes, but the update receives only
statusMessage; the store writes only status and status_message, and the model has no structured
error field.

internal/sp/consumer/response_consumer.go[228-257]
internal/sp/store/resource_manager/service_instance.go[214-231]
internal/sp/store/model/service_type_instance.go[10-19]

Agent prompt

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

## Issue description
Provider status codes and classifications are logged but omitted from the persisted failure record.
## Fix Focus Areas
- internal/sp/consumer/response_consumer.go[228-257]
- internal/sp/store/model/service_type_instance.go[10-19]
- internal/sp/store/resource_manager/service_instance.go[214-231]
## Recommended Fix
Add storage for the structured error fields and write them atomically with the accepted failure transition. Test that the provider status code and classification remain available from the stored instance.

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

---

Context sources

Review mode: ⚖️ Balanced: This changes runtime event decoding, failure-status persistence, and logging on an error-handling path, requiring a careful single-pass review but not enough independent logic for extended review.

Tip of the day

💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[gciavarrini]

This slog.Error runs before UpdateStatusFrom, so a superseded agent's error event logs "agent reported error" even though the transition is rejected. Better moving it after the applied check.

Also, an agent reporting a provider error is expected operation, not a control plane fault. What about using Warn so error-level monitoring stays focused on internal failures?

[gabriel-farache]

I can use warn instead of error I am good with it

As for the log placement, I would like to keep it where it is as even if the transaction fail for some DB issue, the information is still important to be logged. Plus, moving the log statement out of the condition will require having a boolean to know if we need to log something.
I already rejected it from qodo: #80 (comment)

Is that OK if I leave the log statement where it currently is?