Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 34 additions & 3 deletions .ai/test-plans/2026-07-17-16-28-integration-tests.md
Original file line number Diff line number Diff line change
Expand Up @@ -1134,6 +1134,37 @@ Unless overridden, tests use:

---

### IT-MSG-071: Nested CE payload — resourceId extracted correctly

- **Validates AC:** AC-MSG-030
- **Test Infrastructure:** NATS JetStream; nested JSON payload with `spec` object
- **Given** a CloudEvent with nested payload `{"resourceId":"res-nested","spec":{"replicas":3}}`
- **When** the message is consumed on the main topic
- **Then** `resourceId` MUST be extracted correctly (struct ignores nested fields)
- **And** the response CE MUST include the extracted `resourceId`
- **And** a nested cancel payload MUST correctly populate the deny list

### IT-MSG-072: Delete request produces deletion-acknowledged response

- **Validates AC:** AC-MSG-060
- **Test Infrastructure:** NATS JetStream; responses subscriber
- **Given** a `dcm.request.delete` CloudEvent is published to the main topic
- **When** the handler returns nil (success)
- **Then** the response CE type MUST be `dcm.agent.deletion-acknowledged`
- **And** the response CE data MUST include `status: "DELETING"`

### IT-MSG-073: publishResponseCE failure causes nak and redelivery

- **Validates AC:** AC-MSG-055
- **Test Infrastructure:** NATS JetStream; client with empty AgentName
- **Given** a client created with empty `AgentName` (FormatSource will fail)
- **When** a valid message is consumed and handler returns nil
- **Then** `publishResponseCE` MUST fail (FormatSource error)
- **And** the message MUST be nak'd
- **And** JetStream MUST redeliver the message (delivery count >= 2)

---

## Topic 8: Resource Operation Routing

### IT-RTE-010: Route creation to Ready embedded SP
Expand Down Expand Up @@ -1647,12 +1678,12 @@ Unless overridden, tests use:
| AC-MSG-018 | IT-MSG-040 |
| AC-MSG-020 | IT-MSG-050 |
| AC-MSG-025 | IT-MSG-060 |
| AC-MSG-030 | IT-MSG-070 |
| AC-MSG-030 | IT-MSG-070, IT-MSG-071 |
| AC-MSG-035 | IT-MSG-080 |
| AC-MSG-040 | IT-MSG-090 |
| AC-MSG-050 | IT-MSG-100 |
| AC-MSG-055 | IT-MSG-110 |
| AC-MSG-060 | IT-MSG-120 |
| AC-MSG-055 | IT-MSG-110, IT-MSG-073 |
| AC-MSG-060 | IT-MSG-120, IT-MSG-072 |
| AC-RTE-010 | IT-RTE-010, IT-RTE-015 |
| AC-RTE-020 | IT-RTE-020 |
| AC-RTE-030 | IT-RTE-030 |
Expand Down
7 changes: 6 additions & 1 deletion .ai/test-plans/2026-07-17-16-28-unit-tests.md
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,11 @@ This plan identifies pure logic functions suitable for isolated unit testing in
- UT-MSG-034: `"agent-prod.1"` (dot separator) — valid (dots allowed in NATS subjects)
- UT-MSG-035: `"agent-prod-1"` (alphanumeric + hyphens) — valid
- UT-MSG-036: Empty string — invalid
- UT-MSG-037: Contains `"!"` — invalid (whitelist regex rejects)
- UT-MSG-038: Contains `"@"` — invalid (whitelist regex rejects)
- UT-MSG-039: Contains `"/"` — invalid (whitelist regex rejects)
- UT-MSG-040: Contains underscore `"test_topic"` — valid (underscores allowed)
- UT-MSG-041: Exactly 255 characters — valid (boundary acceptance)

---

Expand Down Expand Up @@ -644,7 +649,7 @@ This plan identifies pure logic functions suitable for isolated unit testing in
| AC-HMN-005 | UT-HMN-070 |
| AC-DCM-050 | UT-DCM-010, UT-DCM-011–014, UT-DCM-020–023 |
| AC-DCM-061 | UT-DCM-030, UT-DCM-031–037 |
| AC-MSG-010 | UT-MSG-010, UT-MSG-020, UT-MSG-030–036 |
| AC-MSG-010 | UT-MSG-010, UT-MSG-020, UT-MSG-030–041 |
| AC-RTE-055 | UT-RTE-050, UT-RTE-051–053 |
| AC-RTE-070 | UT-RTE-010 |
| AC-RTE-075 | UT-RTE-030, UT-RTE-040 |
Expand Down
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,10 @@ test:
go run github.com/onsi/ginkgo/v2/ginkgo -r --randomize-all --fail-on-pending --skip-package=test/e2e

test-unit:
go run github.com/onsi/ginkgo/v2/ginkgo -r --randomize-all --fail-on-pending --label-filter=unit ./internal/config ./internal/httperror ./internal/provider ./internal/health/monitor ./internal/backoff ./internal/dcm ./cmd/environment-agent
go run github.com/onsi/ginkgo/v2/ginkgo -r --randomize-all --fail-on-pending --label-filter=unit ./internal/config ./internal/httperror ./internal/provider ./internal/health/monitor ./internal/backoff ./internal/dcm ./internal/messaging ./internal/cloudevent ./cmd/environment-agent

test-integration:
go run github.com/onsi/ginkgo/v2/ginkgo -r --randomize-all --fail-on-pending --label-filter=integration ./internal/apiserver ./internal/health ./internal/health/monitor ./internal/provider ./internal/dcm
go run github.com/onsi/ginkgo/v2/ginkgo -r --randomize-all --fail-on-pending --label-filter=integration ./internal/apiserver ./internal/health ./internal/health/monitor ./internal/provider ./internal/dcm ./internal/messaging

test-race:
go run github.com/onsi/ginkgo/v2/ginkgo -r --race --randomize-all --fail-on-pending --skip-package=test/e2e
Expand Down
47 changes: 30 additions & 17 deletions cmd/environment-agent/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package main
import (
"context"
"errors"
"fmt"
"log/slog"
"net"
"net/http"
Expand All @@ -19,21 +20,12 @@ import (
"github.com/dcm-project/environment-agent/internal/health"
"github.com/dcm-project/environment-agent/internal/health/monitor"
"github.com/dcm-project/environment-agent/internal/httperror"
"github.com/dcm-project/environment-agent/internal/messaging"
"github.com/dcm-project/environment-agent/internal/provider"
"github.com/dcm-project/environment-agent/internal/provider/service"
"github.com/dcm-project/environment-agent/internal/provider/store"
)

// TODO: replace with real MessagingStatus from the NATS/messaging subsystem.
type messagingStatus struct{}

func (messagingStatus) IsConnected() bool { return true }

// stubConsumerLagProvider returns 0 until Topic 7 provides real NATS consumer lag.
type stubConsumerLagProvider struct{}

func (stubConsumerLagProvider) ConsumerLag() int64 { return 0 }

// serviceTypeLister adapts ProviderService to dcm.ServiceTypeLister.
type serviceTypeLister struct {
providerSvc *service.ProviderService
Expand Down Expand Up @@ -107,27 +99,35 @@ func run(ctx context.Context) int {
}
providerSvc.RegisterEmbedded(cfg.Provider.EmbeddedSPs)

// Messaging client — must start before registrar (provides ConsumerLagProvider)
msgClient, topicMain, err := setupMessaging(cfg, logger)
if err != nil {
logger.Error("invalid topic name", "error", err)
return 1
}
if err := msgClient.Start(ctx); err != nil {
logger.Error("failed to start messaging client", "error", err)
return 1
}
defer msgClient.Stop()

// DCM Registrar — created before monitor starts so callbacks can be wired
// before any health transitions fire. Deferred after monitor so LIFO shuts
// registrar down first.
topicName := cfg.Messaging.TopicName
if topicName == "" {
topicName = cfg.Agent.Name
}
registrar, err := dcm.NewRegistrar(
dcm.RegistrarConfig{
AgentName: cfg.Agent.Name,
Environment: cfg.Agent.Environment,
Cost: cfg.Agent.Cost,
TopicName: topicName,
TopicName: topicMain,
RegistrationURL: cfg.DCM.RegistrationURL,
InitialBackoff: cfg.DCM.InitialBackoff,
MaxBackoff: cfg.DCM.MaxBackoff,
HeartbeatInterval: cfg.Heartbeat.Interval,
PrerequisiteRetryInterval: cfg.DCM.PrerequisiteRetryInterval,
},
&serviceTypeLister{providerSvc: providerSvc, logger: logger},
stubConsumerLagProvider{},
msgClient,
nil,
logger,
)
Expand Down Expand Up @@ -159,7 +159,7 @@ func run(ctx context.Context) int {
<-registrar.Done()
}()
Comment thread
qodo-code-review[bot] marked this conversation as resolved.

healthSvc := health.NewService(messagingStatus{})
healthSvc := health.NewService(msgClient)
strictHandler := handler.New(healthSvc, providerSvc)
h := oapigen.NewStrictHandlerWithOptions(strictHandler, nil, oapigen.StrictHTTPServerOptions{
ResponseErrorHandlerFunc: func(w http.ResponseWriter, r *http.Request, err error) {
Expand All @@ -177,3 +177,16 @@ func run(ctx context.Context) int {
logger.Info("Environment Agent stopped")
return 0
}

func setupMessaging(cfg *config.Config, logger *slog.Logger) (*messaging.Client, string, error) {
topics := messaging.DeriveTopicNames(cfg.Agent.Name, cfg.Messaging.TopicName)
if err := messaging.ValidateTopicName(topics.Main); err != nil {
return nil, "", fmt.Errorf("invalid topic name: %w", err)
}
Comment on lines +181 to +185

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Derived topics unvalidated 🐞 Bug ≡ Correctness

setupMessaging validates only the base topic (topics.Main), but DeriveTopicNames appends
".retry"/".cancel" which can exceed ValidateTopicName’s 255-character limit. This allows a
configuration that passes validation to later fail JetStream stream initialization at runtime.
Agent Prompt
## Issue description
Only `topics.Main` is validated, but the client uses derived subjects (`.retry`, `.cancel`) when creating streams. A 255-character base topic passes validation but produces derived subjects >255 characters, causing messaging startup to fail later.

## Issue Context
`DeriveTopicNames()` always appends suffixes; `Client.initStreams()` uses those derived values as JetStream subjects.

## Fix Focus Areas
- cmd/environment-agent/main.go[169-179]
- internal/messaging/topics.go[18-29]
- internal/messaging/topics.go[33-43]
- internal/messaging/client.go[183-201]

## Suggested fix
- Validate *all* derived subjects before constructing/starting the client:
  - `ValidateTopicName(topics.Main)`
  - `ValidateTopicName(topics.Retry)`
  - `ValidateTopicName(topics.Cancel)`
- Alternatively, enforce a stricter max length for the base topic so that derived names remain <=255 (but validating each derived value is clearer).

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6cf52ed and 8555d06 — added ValidateJetStreamSafeName (REQ-MSG-011), which rejects dots and caps the base name so every derived JetStream stream/consumer name (including the longest suffix, -cancel-consumer) stays within NATS's 255-char server-side limit. Wired into setupMessaging in main.go so an unsafe base name fails startup fast instead of only surfacing later as a stream-init failure.

client := messaging.NewClient(messaging.ClientConfig{
URL: cfg.Messaging.URL,
TopicName: topics.Main,
AgentName: cfg.Agent.Name,
}, logger)
return client, topics.Main, nil
}
1 change: 1 addition & 0 deletions cmd/environment-agent/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ var _ = Describe("run", Label("unit"), func() {
GinkgoT().Setenv("AGENT_ENVIRONMENT", "test")
GinkgoT().Setenv("AGENT_COST", "medium")
GinkgoT().Setenv("DCM_REGISTRATION_URL", "http://localhost:8080")
GinkgoT().Setenv("AGENT_MESSAGING_URL", "nats://localhost:4222")

ctx, cancel := context.WithCancel(context.Background())
cancel()
Expand Down
17 changes: 17 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@ go 1.25.5

require (
github.com/caarlos0/env/v11 v11.4.1
github.com/cloudevents/sdk-go/v2 v2.16.2
github.com/getkin/kin-openapi v0.144.0
github.com/go-chi/chi/v5 v5.3.1
github.com/google/uuid v1.6.0
github.com/nats-io/nats-server/v2 v2.14.4
github.com/nats-io/nats.go v1.52.0
github.com/oapi-codegen/nethttp-middleware v1.1.2
github.com/oapi-codegen/oapi-codegen/v2 v2.8.0
github.com/oapi-codegen/runtime v1.6.0
Expand All @@ -16,27 +19,41 @@ require (

require (
github.com/Masterminds/semver/v3 v3.4.0 // indirect
github.com/antithesishq/antithesis-sdk-go v0.7.2-default-no-op // indirect
github.com/apapsch/go-jsonmerge/v2 v2.0.0 // indirect
github.com/dprotaso/go-yit v0.0.0-20220510233725-9ba8df137936 // indirect
github.com/go-logr/logr v1.4.3 // indirect
github.com/go-openapi/jsonpointer v0.23.1 // indirect
github.com/go-openapi/swag/jsonname v0.26.0 // indirect
github.com/go-task/slim-sprig/v3 v3.0.0 // indirect
github.com/google/go-cmp v0.7.0 // indirect
github.com/google/go-tpm v0.9.8 // indirect
github.com/google/pprof v0.0.0-20260402051712-545e8a4df936 // indirect
github.com/gorilla/mux v1.8.1 // indirect
github.com/json-iterator/go v1.1.12 // indirect
github.com/klauspost/compress v1.19.0 // indirect
github.com/minio/highwayhash v1.0.4 // indirect
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/nats-io/jwt/v2 v2.8.2 // indirect
github.com/nats-io/nkeys v0.4.16 // indirect
github.com/nats-io/nuid v1.0.1 // indirect
github.com/oasdiff/yaml v0.1.1 // indirect
github.com/oasdiff/yaml3 v0.0.14 // indirect
github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 // indirect
github.com/speakeasy-api/jsonpath v0.6.3 // indirect
github.com/speakeasy-api/openapi v1.24.0 // indirect
github.com/vmware-labs/yaml-jsonpath v0.3.2 // indirect
go.uber.org/multierr v1.11.0 // indirect
go.uber.org/zap v1.27.0 // indirect
go.yaml.in/yaml/v3 v3.0.4 // indirect
golang.org/x/crypto v0.54.0 // indirect
golang.org/x/mod v0.38.0 // indirect
golang.org/x/net v0.57.0 // indirect
golang.org/x/sync v0.22.0 // indirect
golang.org/x/sys v0.47.0 // indirect
golang.org/x/text v0.40.0 // indirect
golang.org/x/time v0.15.0 // indirect
golang.org/x/tools v0.48.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)
Loading