Skip to content

Introduce test plans for ai#7

Merged
gabriel-farache merged 5 commits into
dcm-project:mainfrom
gabriel-farache:feat/test-plans
Jul 24, 2026
Merged

Introduce test plans for ai#7
gabriel-farache merged 5 commits into
dcm-project:mainfrom
gabriel-farache:feat/test-plans

Conversation

@gabriel-farache

@gabriel-farache gabriel-farache commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Depends on #2

Summary

  • Add unit and integration test plans with cross-model review refinements

Made with Cursor

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Bootstrap environment agent docs/test plans and align OpenAPI + generated Go APIs

✨ Enhancement 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Add design decisions, detailed spec, and unit/integration test plan documents for v1alpha1.
• Align OpenAPI contract (pagination + schema fixes) and regenerate server/client/types accordingly.
• Tidy dev tooling and docs: spectral lint target, README endpoints table, and .ai ignores.
Diagram

graph TD
  A["OpenAPI spec"] --> B["Go codegen"] --> C["API types"] --> D["HTTP server stubs"]
  B --> E["HTTP client SDK"]
  A --> F["Design spec"]
  A --> G["Test plans"]
  H["Makefile (AEP lint)"] --> A
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Keep ListProviders response inline (no ProviderList schema)
  • ➕ Avoids introducing an extra schema/type in generated code
  • ➕ Slightly smaller surface area for model types
  • ➖ Harder to evolve (pagination fields become ad-hoc)
  • ➖ Worse reuse across languages/clients; less consistent with other list APIs
2. Defer pagination parameters until implementation exists
  • ➕ Avoids short-term interface churn in generated server/client
  • ➖ Spec and acceptance criteria already expect list semantics; deferring increases future breaking changes
  • ➖ Pagination is easiest to standardize early while API is v1alpha1
3. Retain 503 response shape in generated health client/server types
  • ➕ Explicitly documents shutdown/unavailable behavior in generated code
  • ➖ If 503 is intentionally out-of-contract, generated handlers/clients shouldn’t model it
  • ➖ Maintaining unused responses increases drift between contract and behavior

Recommendation: Current approach is the best trade-off: treat OpenAPI as the contract source of truth, standardize list pagination early via a dedicated ProviderList schema, and regenerate the Go server/client/types to prevent drift. The only caution is to call out the breaking signature change for ListProviders (new params object) in release notes/upgrade guidance if any downstream code exists.

Files changed (12) +4297 / -109

Enhancement (5) +185 / -103
openapi.yamlFix provider schemas and add ListProviders pagination contract +35/-19

Fix provider schemas and add ListProviders pagination contract

• Renames Provider.last_check to last_check_time, introduces ProviderList (results + next_page_token), and updates GET /providers to return ProviderList. Adds pagination query params (max_page_size, page_token), updates 401 descriptions to indicate reserved auth, adjusts error type example, and removes the modeled 503 response where it no longer matches the intended contract.

api/v1alpha1/openapi.yaml

spec.gen.goUpdate embedded swaggerSpec blob after OpenAPI changes +41/-38

Update embedded swaggerSpec blob after OpenAPI changes

• Refreshes the generated embedded OpenAPI JSON payload to match the updated api/v1alpha1/openapi.yaml content.

api/v1alpha1/spec.gen.go

types.gen.goRegenerate models for ProviderList, pagination params, and renamed fields +19/-2

Regenerate models for ProviderList, pagination params, and renamed fields

• Renames Provider.LastCheck to Provider.LastCheckTime to match OpenAPI, adds ProviderList model, and adds ListProvidersParams for max_page_size and page_token. Keeps generated types consistent with updated response shapes and query parameters.

api/v1alpha1/types.gen.go

server.gen.goRegenerate server interface/wrapper to accept ListProviders query params +40/-21

Regenerate server interface/wrapper to accept ListProviders query params

• Updates the ServerInterface ListProviders signature to include ListProvidersParams, binds query parameters in the wrapper, and switches the 200 response type to ProviderList. Removes now-obsolete generated response plumbing tied to the dropped 503 response.

internal/api/server/server.gen.go

client.gen.goRegenerate client to support ListProviders params and ProviderList response +50/-23

Regenerate client to support ListProviders params and ProviderList response

• Updates client interfaces to accept ListProvidersParams, adds query-string encoding for max_page_size and page_token, and parses the 200 response into ProviderList. Removes parsing/fields for a dropped 503 health response to stay aligned with the OpenAPI contract.

pkg/client/client.gen.go

Documentation (5) +4108 / -5
environment-agent.decisions.mdAdd environment-agent design decision records (DD-010..DD-100) +122/-0

Add environment-agent design decision records (DD-010..DD-100)

• Introduces a consolidated set of design decisions covering hybrid provider model, health semantics, messaging pull model, retry/cancel strategy, persistence expectations, and liveness reporting. Each decision includes rationale and links to related requirements.

.ai/decisions/environment-agent.decisions.md

environment-agent.spec.mdAdd full v1alpha1 specification with requirements + acceptance criteria +1794/-0

Add full v1alpha1 specification with requirements + acceptance criteria

• Adds a comprehensive spec describing architecture, topic breakdown, requirements, configuration keys, and BDD-style acceptance criteria. Explicitly positions api/v1alpha1/openapi.yaml as the API contract source of truth and aligns requirement priorities (e.g., AEP/middleware guidance).

.ai/specs/environment-agent.spec.md

2026-07-17-16-28-integration-tests.mdAdd integration test plan spanning all topics and cross-cutting behaviors +1571/-0

Add integration test plan spanning all topics and cross-cutting behaviors

• Defines BDD-focused integration tests using real dependencies (HTTP server, filesystem persistence, NATS JetStream, mock DCM and SP servers). Covers lifecycle/shutdown semantics, error handling, routing behaviors, and observable outcomes across the agent’s topic areas.

.ai/test-plans/2026-07-17-16-28-integration-tests.md

2026-07-17-16-28-unit-tests.mdAdd unit test plan for pure logic (validation, slotting, deny-list, state) +615/-0

Add unit test plan for pure logic (validation, slotting, deny-list, state)

• Enumerates isolated unit tests for configuration validation, ID/pattern validation, service-type slot management semantics, and deny-list (LRU + consume-on-use) behavior. Separates algorithmic testing from integration-level observability and documents caller-trust boundaries.

.ai/test-plans/2026-07-17-16-28-unit-tests.md

README.mdCorrect and expand documented API endpoints +6/-5

Correct and expand documented API endpoints

• Updates the endpoints table to reflect the providers list/get routes and removes the outdated status endpoint reference. Clarifies that GET /providers returns a list including health and adds GET /providers/{provider_id}.

README.md

Other (2) +4 / -1
.gitignoreIgnore AI review/plan working directories +3/-0

Ignore AI review/plan working directories

• Adds .ai/reviews/ and .ai/plans/ to prevent committing generated/iterative AI artifacts.

.gitignore

MakefileRun Spectral via npx for AEP linting +1/-1

Run Spectral via npx for AEP linting

• Updates the check-aep target to invoke @stoplight/spectral-cli through npx, reducing dependency on a globally installed spectral binary.

Makefile

@gabriel-farache gabriel-farache changed the title Bootstrap environment agent with spec, OpenAPI fixes, and test plans Introduce test plans for ai Jul 20, 2026
@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Remediation recommended

1. Pagination bounds unenforced 🐞 Bug ≡ Correctness
Description
The OpenAPI contract adds max_page_size with min/max/default constraints, but the generated server
wrapper only parses the query value and forwards it without enforcing the bounds or applying the
default, so contract compliance depends entirely on downstream handler logic. This can lead to
out-of-contract behavior (e.g., max_page_size=0 or 101) and potential resource amplification if
a handler forgets to clamp/validate.
Code

internal/api/server/server.gen.go[R92-122]

+	// Parameter object where we will unmarshal all parameters from the context
+	var params ListProvidersParams
+
+	// ------------- Optional query parameter "max_page_size" -------------
+
+	err = runtime.BindQueryParameterWithOptions("form", true, false, "max_page_size", r.URL.Query(), &params.MaxPageSize, runtime.BindQueryParameterOptions{Type: "integer", Format: ""})
+	if err != nil {
+		var requiredError *runtime.RequiredParameterError
+		if errors.As(err, &requiredError) {
+			siw.ErrorHandlerFunc(w, r, &RequiredParamError{ParamName: "max_page_size"})
+		} else {
+			siw.ErrorHandlerFunc(w, r, &InvalidParamFormatError{ParamName: "max_page_size", Err: err})
+		}
+		return
+	}
+
+	// ------------- Optional query parameter "page_token" -------------
+
+	err = runtime.BindQueryParameterWithOptions("form", true, false, "page_token", r.URL.Query(), &params.PageToken, runtime.BindQueryParameterOptions{Type: "string", Format: ""})
+	if err != nil {
+		var requiredError *runtime.RequiredParameterError
+		if errors.As(err, &requiredError) {
+			siw.ErrorHandlerFunc(w, r, &RequiredParamError{ParamName: "page_token"})
+		} else {
+			siw.ErrorHandlerFunc(w, r, &InvalidParamFormatError{ParamName: "page_token", Err: err})
+		}
+		return
+	}
+
	handler := http.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		siw.Handler.ListProviders(w, r)
+		siw.Handler.ListProviders(w, r, params)
Evidence
The OpenAPI spec declares explicit numeric bounds/default for max_page_size, but the generated
server wrapper only performs type binding and immediately calls the handler with the resulting
params object, with no range checks or default application in between.

api/v1alpha1/openapi.yaml[59-74]
internal/api/server/server.gen.go[86-123]

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

### Issue description
`max_page_size` is documented with `minimum: 1`, `maximum: 100`, and `default: 100`, but the generated HTTP wrapper only binds the query parameter and passes it through. Without centralized validation/defaulting, every implementation of `ListProviders` must remember to enforce the contract, and missing it can cause incorrect responses and/or overly large payloads.

### Issue Context
- OpenAPI defines constraints/defaults for pagination.
- The generated server wrapper binds `max_page_size`/`page_token` but does not clamp, reject, or default.

### Fix Focus Areas
- internal/api/server/server.gen.go[89-122]
- api/v1alpha1/openapi.yaml[59-74]

### Suggested fix
- Add a non-generated strict middleware (recommended) that runs for operationID `ListProviders` and:
 - If `MaxPageSize == nil`, set it to `100`.
 - If `MaxPageSize != nil` and is outside `[1,100]`, return a `ListProviders400ApplicationProblemPlusJSONResponse` (or map to your standardized error).
- Add unit tests for: omitted `max_page_size`, `0`, `1`, `100`, `101`, and a non-integer value.
- Ensure the server wiring uses `NewStrictHandler(ssi, []StrictMiddlewareFunc{...})` so validation is always applied.

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



Informational

2. check-aep needs Node 🐞 Bug ⚙ Maintainability
Description
make check-aep now runs Spectral via npx, which requires Node.js/npm (and typically network
access to fetch the package), but the documented prerequisites only mention installing Spectral and
do not mention Node/npm. This can cause local make check-aep failures on clean developer
environments.
Code

Makefile[74]

+	npx --yes @stoplight/spectral-cli lint --fail-severity=warn ./api/v1alpha1/openapi.yaml
Evidence
The Makefile change replaces a direct spectral invocation with npx ... spectral-cli, while the
README prerequisite list still calls out Spectral but does not mention Node/npm, creating a mismatch
between documented setup and required tooling.

Makefile[73-74]
README.md[26-32]

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 Makefile’s `check-aep` target now invokes Spectral via `npx`, introducing a Node.js/npm prerequisite. The README prerequisites currently only mention installing Spectral, which is no longer sufficient guidance for running `make check-aep`.

### Issue Context
- `npx` is part of the Node.js/npm toolchain.
- Users following the README may install Spectral but still fail if Node/npm is missing.

### Fix Focus Areas
- Makefile[73-74]
- README.md[26-32]

### Suggested fix
- Update README prerequisites to include Node.js/npm (or explicitly state that `make check-aep` uses `npx`).
- Optionally improve robustness by making `check-aep` prefer a locally installed `spectral` binary if present, and fallback to `npx` otherwise (with a clear error message if neither is available).

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


Grey Divider

Qodo Logo

Comment thread internal/api/server/server.gen.go
Comment thread Makefile
gabriel-farache and others added 3 commits July 20, 2026 18:30
BDD-driven test plans covering all 120 acceptance criteria from the spec.
Unit tests target pure logic (config validation, slot management, deny list
LRU, backoff calculation, CloudEvent construction, health state machine).
Integration tests cover observable behaviors across all 9 topics and
cross-cutting concerns with full AC↔TC traceability matrices.

Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: gabriel-farache <gfarache@redhat.com>
…limitation

Go's net/http does not provide a mechanism to inject HTTP responses
before force-closing connections after shutdown timeout. Change the MUST
receive HTTP 503 to MAY receive HTTP 503 or connection reset, reflecting
the actual behavior of srv.Close() sending TCP RST.

Ref: MF-R1 from review panel (unanimous across 4 models)
Decision: DD-110

Assisted by: Claude Code - Opus 4.6

Signed-off-by: gabriel-farache <gfarache@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Update test plan for IT-HTTP-050 to match the spec amendment: verify
connection termination after shutdown timeout using raw TCP with partial
request body, instead of asserting HTTP 503 which Go stdlib cannot
guarantee.

Ref: MF-R2 from review panel

Assisted by: Claude Code - Opus 4.6

Signed-off-by: gabriel-farache <gfarache@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

@gciavarrini gciavarrini left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These plans are AI-authored for AI consumers so diffiuclt to review for a human. for my comments i used an ai agent to verify if they are complian with the enhancement

Comment thread .ai/test-plans/2026-07-17-16-28-integration-tests.md
Comment thread .ai/test-plans/2026-07-17-16-28-integration-tests.md
Comment thread .ai/test-plans/2026-07-17-16-28-integration-tests.md
Comment thread .ai/test-plans/2026-07-17-16-28-integration-tests.md
Comment thread .ai/test-plans/2026-07-17-16-28-unit-tests.md
Comment thread .ai/test-plans/2026-07-17-16-28-unit-tests.md
Comment thread .ai/test-plans/2026-07-17-16-28-unit-tests.md
Comment thread .ai/test-plans/2026-07-17-16-28-integration-tests.md
Comment thread .ai/test-plans/2026-07-17-16-28-integration-tests.md Outdated
Comment thread .ai/test-plans/2026-07-17-16-28-integration-tests.md Outdated
gabriel-farache and others added 2 commits July 21, 2026 15:07
Add missing service-type-unavailable CloudEvent (REQ-HMN-145, AC-HMN-085)
per enhancement. Amend REQ-RTE-170 and AC-RTE-080 for immediate cancel
processing of retry-held requests, aligning with enhancement semantics.

Add decision records:
- DD-110: deny list consume-on-use and LRU eviction rationale
- DD-120: SP registration lease expiry deferred to post-v1alpha1
- DD-130: immediate cancel processing for retry-held requests

Signed-off-by: gabriel-farache <gfarache@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…view

Add IT-HMN-135 for service-type-unavailable CloudEvent validation.
Add IT-RTE-065 for deletion request queuing when SP is Unhealthy.
Amend IT-RTE-120 for immediate cancel handling in retry topic.
Update traceability matrix with new AC→TC mappings.

Signed-off-by: gabriel-farache <gfarache@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@gabriel-farache
gabriel-farache merged commit 7a58506 into dcm-project:main Jul 24, 2026
6 checks passed
gabriel-farache added a commit that referenced this pull request Jul 24, 2026
Add missing service-type-unavailable CloudEvent (REQ-HMN-145, AC-HMN-085)
per enhancement. Amend REQ-RTE-170 and AC-RTE-080 for immediate cancel
processing of retry-held requests, aligning with enhancement semantics.

Add decision records:
- DD-110: deny list consume-on-use and LRU eviction rationale
- DD-120: SP registration lease expiry deferred to post-v1alpha1
- DD-130: immediate cancel processing for retry-held requests

Signed-off-by: gabriel-farache <gfarache@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@gabriel-farache
gabriel-farache deleted the feat/test-plans branch July 24, 2026 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants