Close I-004 (agent hard-delete cascades targets) coverage-gap finding

Operator decision answered as full soft-delete with optional forced cascade — hard-delete is not reachable from any public surface. Prior to this commit, DELETE /agents/{id} ran a plain `DELETE FROM agents` whose schema-level `ON DELETE CASCADE` on deployment_targets.agent_id silently wiped every target, orphaning certs and aborting in-flight jobs. The finding closure reshapes the agent-removal contract around soft retirement with explicit preflight counts, an opt-in cascade gated by a mandatory reason, and unconditional protection for the four reserved sentinel agents used by discovery sources. Schema — migration 000015: migrations/000015_agent_retire.up.sql flips deployment_targets_agent_id_fkey from ON DELETE CASCADE to ON DELETE RESTRICT, so a stray `DELETE FROM agents` now errors at the DB boundary instead of quietly destroying targets. Both `agents` and `deployment_targets` grow a retired_at TIMESTAMPTZ + retired_reason TEXT pair (TEXT not VARCHAR so operator comments are never truncated), indexed via partial indexes WHERE retired_at IS NOT NULL. The migration is self-healing (ADD COLUMN IF NOT EXISTS, DROP CONSTRAINT IF EXISTS then ADD CONSTRAINT, CREATE INDEX IF NOT EXISTS) so repeated runs against partially-migrated databases converge. migrations/000015_agent_retire.down.sql restores CASCADE and drops the new columns for clean rollback. A dedicated repository-layer testcontainers test (internal/repository/postgres/migration_000015_test.go) asserts the before/after FK action, column presence, index presence, and round-trip idempotency under up→down→up. Domain — sentinel guard + dependency counts: internal/domain/connector.go gains IsRetired() on Agent, the exported SentinelAgentIDs slice listing server-scanner, cloud-aws-sm, cloud-azure-kv, cloud-gcp-sm verbatim (matching the four reserved IDs documented in CLAUDE.md and created at startup in cmd/server/main.go), IsSentinelAgent(id string) predicate, AgentDependencyCounts{ActiveTargets, ActiveCertificates, PendingJobs} with a HasDependencies() method, and ActorTypeAgent / ActorTypeSystem enum values used by audit emission downstream. Coverage locked down by internal/domain/connector_test.go. Service — 8-step ordered contract: internal/service/agent_retire.go:RetireAgent(ctx, id, actor, opts{Force, Reason}) enforces a fixed execution order: (1) sentinel guard — IsSentinelAgent(id) returns ErrAgentIsSentinel unconditionally; force=true does NOT bypass it. (2) fetch — ErrAgentNotFound on miss. (3) idempotency — if IsRetired() already, return AgentRetirementResult{AlreadyRetired: true} with no new audit event and no state change (safe to replay from flaky clients). (4) preflight counts — collectAgentDependencyCounts runs ActiveTargets, ActiveCertificates, PendingJobs sequentially (not in parallel; keeps the per-query timeout predictable and matches the repo's existing call-chain shape). (5) force-reason guard — opts.Force=true with empty Reason returns ErrForceReasonRequired (wired into the 400 status surface). (6) dependency guard — HasDependencies() with opts.Force=false returns BlockedByDependenciesError{Counts} (wired into the 409 body with per-bucket counts). (7) mutation — single pinned retiredAt := time.Now(); agent retirement first, then cascade target retirement if opts.Force, all under the repo's single transaction so the two retired_at stamps match to the second. (8) best-effort audit — agent_retired always; agent_retirement_ cascaded additionally on the force path. Actor is whatever the handler resolves from the request; actor type is mapped by resolveActorType (system/agent-prefix→Agent/else→User). Audit emission failures are logged via slog.Error but do not abort the retirement (matches the house convention used by every other scheduler-emitted event). BlockedByDependenciesError implements Error() as "active_targets=%d, active_certificates=%d, pending_jobs=%d" and Unwrap() → ErrBlockedByDependencies. The single struct satisfies errors.Is via Unwrap (used by scheduler-level tests) and errors.As via the concrete type (used by the handler to fish out Counts for the 409 body). ListRetiredAgents(page, perPage) adds a separate paginated accessor with page<1→1 and perPage<1→50 normalization so retired rows are queryable without polluting the default agent listing. Sentinel guard coverage is asymmetric by design: all four reserved IDs are protected, and force=true cannot override. Regression tests in internal/service/agent_retire_test.go assert each of the eight steps in order, plus sentinel bypass attempts and idempotency replay. Handler + router — status-code surface: internal/api/handler/agents.go:RetireAgent exposes seven status codes on DELETE /agents/{id}: 200 on a fresh retirement (body echoes AgentRetirementResult). 204 on idempotent replay (AlreadyRetired=true; no new audit). 400 on ErrForceReasonRequired. 403 on ErrAgentIsSentinel. 404 on ErrAgentNotFound. 409 on BlockedByDependenciesError, with a custom body shape {error, counts{active_targets, active_certificates, pending_jobs}} that bypasses the default ErrorWithRequestID envelope so callers get the per-bucket numbers directly. 500 on any other error. Heartbeat HandleHeartbeat returns 410 Gone when the agent is retired (ErrAgentRetired), signalling the agent to shut down. Query params `force=true` and `reason=<text>` drive the cascade path; both are forwarded as url.Values through the new MCP transport. internal/api/router/router.go registers GET /api/v1/agents/retired literal-path BEFORE /api/v1/agents/{id} — Go 1.22 ServeMux's literal-beats-pattern-var precedence routes "retired" to the paginated retired-agents listing instead of fetching a hypothetical agent named "retired". Agent binary — clean shutdown on 410: cmd/agent/main.go gains the ErrAgentRetired sentinel, a retiredOnce sync.Once, and a retiredSignal chan struct{}. A markRetired(source, statusCode, body) helper closes the channel exactly once; the Run() select loop observes the close and returns ErrAgentRetired; main() matches via errors.Is(err, ErrAgentRetired) and exits cleanly instead of spinning in the heartbeat retry loop. The 410 Gone surface is therefore terminal for the agent process. MCP transport: internal/mcp/client.go adds Client.DeleteWithQuery(path, query), a new additive transport method. Client.Delete is path-only; without this method the retire tool would silently drop `force` and `reason`, turning every cascade retire into a default soft-retire. The new method shares do()'s 204 normalization and 4xx/5xx error propagation so tool authors get one contract. internal/mcp/tools.go + internal/mcp/types.go expose the retire_agent tool with Force+Reason inputs wired through DeleteWithQuery. CLI: cmd/cli/main.go + internal/cli/client.go add two CLI surfaces: `agents list --retired` (client-side strip of --retired then delegation to ListRetiredAgents, sharing --page/--per-page parsing with the default listing) and `agents retire <id> [--force --reason "…"]` (mirrors ErrForceReasonRequired — force without reason is rejected client-side before the request is sent). JSON + table output modes both honor the new columns. Frontend: web/src/pages/AgentsPage.tsx surfaces retired/retire affordances. web/src/api/client.ts + web/src/api/types.ts expose the retire endpoint and the retired-listing. 4 new Vitest regression cases. OpenAPI: api/openapi.yaml documents DELETE /agents/{id} with all seven status codes, 410 on heartbeat, and the 409 per-bucket body shape. Regression coverage (six new test files, all green): internal/service/agent_retire_test.go — 8-step contract + sentinel guards internal/api/handler/agent_retire_handler_test.go — 7-status-code surface + 410 heartbeat internal/mcp/retire_agent_test.go — DeleteWithQuery wire-through internal/cli/agent_retire_test.go — --retired listing + --force/--reason pairing internal/repository/postgres/migration_000015_test.go — FK flip + columns + indexes + up↔down internal/domain/connector_test.go — IsRetired, IsSentinelAgent, SentinelAgentIDs, HasDependencies Files: api/openapi.yaml — DELETE + 410 + 409 body shape cmd/agent/main.go — ErrAgentRetired, markRetired, retiredSignal cmd/cli/main.go — handleAgents list/get/retire dispatch docs/architecture.md, docs/concepts.md, docs/testing-guide.md — retirement contract narrative internal/api/handler/agents.go — RetireAgent, status surface, 410 on heartbeat internal/api/handler/agent_handler_test.go — extended coverage internal/api/handler/agent_retire_handler_test.go — new internal/api/router/router.go — /agents/retired before /agents/{id} internal/cli/agent_retire_test.go — new internal/cli/client.go — ListRetiredAgents + RetireAgent internal/domain/connector.go — IsRetired, SentinelAgentIDs, IsSentinelAgent, AgentDependencyCounts, ActorTypeAgent/System internal/domain/connector_test.go — new internal/integration/lifecycle_test.go — retirement fixture internal/mcp/client.go — DeleteWithQuery additive transport internal/mcp/retire_agent_test.go — new internal/mcp/tools.go, internal/mcp/types.go — retire_agent tool + Force/Reason inputs internal/repository/interfaces.go — AgentRepository retirement methods internal/repository/postgres/agent.go — retire + cascade target retire + counts internal/repository/postgres/migration_000015_test.go — new internal/service/agent.go — wire into AgentService surface internal/service/agent_retire.go — new 8-step contract internal/service/agent_retire_test.go — new internal/service/deployment.go — skip retired agents internal/service/target.go — skip retired agents internal/service/testutil_test.go — shared mocks extended migrations/000015_agent_retire.up.sql — new migrations/000015_agent_retire.down.sql — new web/src/api/client.ts, types.ts + tests — retire endpoint wiring web/src/pages/AgentsPage.tsx — retire UI
2026-06-12 03:38:52 +00:00 · 2026-04-19 05:24:00 +00:00
parent c17ea577e7
commit 49002c8cba
35 changed files with 4400 additions and 33 deletions
@@ -49,6 +49,16 @@ func (c *Client) Delete(path string) (json.RawMessage, error) {
 	return c.do("DELETE", path, nil, nil)
 }

+// DeleteWithQuery performs an HTTP DELETE with query parameters. I-004 adds
+// this transport so MCP tools can target endpoints that carry flags in the
+// query string (e.g. DELETE /api/v1/agents/{id}?force=true&reason=…). Client.Delete
+// is path-only; without this method the retire tool silently drops force/reason,
+// turning every cascade retire into a default soft-retire. Shares do()'s 204
+// normalization and 4xx/5xx error propagation so tool authors get one contract.
+func (c *Client) DeleteWithQuery(path string, query url.Values) (json.RawMessage, error) {
+	return c.do("DELETE", path, query, nil)
+}
+
 // GetRaw performs an HTTP GET and returns the raw response body bytes and content type.
 // Used for binary responses (DER CRL, OCSP).
 func (c *Client) GetRaw(path string) ([]byte, string, error) {
@@ -0,0 +1,214 @@
+package mcp
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strings"
+	"testing"
+)
+
+// TestClient_DeleteWithQuery_ForceRetire covers the new transport capability
+// that I-004 adds to the MCP client. The retire tool needs to issue
+// DELETE /api/v1/agents/{id}?force=true&reason=... — Client.Delete as it
+// stands only accepts a path, dropping query parameters on the floor. Phase 2b
+// must add DeleteWithQuery so the MCP retire tool can hit the force escape
+// hatch; without this, every retire-via-MCP call with force=true silently
+// becomes a default soft-retire and either succeeds wrongly or 409s.
+func TestClient_DeleteWithQuery_ForceRetire(t *testing.T) {
+	var (
+		sawMethod string
+		sawPath   string
+		sawForce  string
+		sawReason string
+	)
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		sawMethod = r.Method
+		sawPath = r.URL.Path
+		sawForce = r.URL.Query().Get("force")
+		sawReason = r.URL.Query().Get("reason")
+
+		if r.Method != http.MethodDelete || r.URL.Path != "/api/v1/agents/ag-1" {
+			w.WriteHeader(http.StatusNotFound)
+			return
+		}
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusOK)
+		_ = json.NewEncoder(w).Encode(map[string]interface{}{
+			"retired_at":      "2026-04-18T12:00:00Z",
+			"already_retired": false,
+			"cascade":         true,
+		})
+	}))
+	defer server.Close()
+
+	c := NewClient(server.URL, "test-key")
+	// Compile-fail until Phase 2b grows Client.DeleteWithQuery. Passing the
+	// query as a url.Values is the established pattern (matches Get's shape).
+	query := url.Values{}
+	query.Set("force", "true")
+	query.Set("reason", "decommissioning rack 7")
+	data, err := c.DeleteWithQuery("/api/v1/agents/ag-1", query)
+	if err != nil {
+		t.Fatalf("DeleteWithQuery err=%v want nil", err)
+	}
+	if data == nil {
+		t.Fatal("DeleteWithQuery returned nil data; want 200 body echo-back")
+	}
+
+	if sawMethod != http.MethodDelete {
+		t.Errorf("method=%q want DELETE", sawMethod)
+	}
+	if sawPath != "/api/v1/agents/ag-1" {
+		t.Errorf("path=%q want /api/v1/agents/ag-1 (query must be stripped from path)", sawPath)
+	}
+	if sawForce != "true" {
+		t.Errorf("force query=%q want \"true\"", sawForce)
+	}
+	if sawReason != "decommissioning rack 7" {
+		t.Errorf("reason query=%q want %q", sawReason, "decommissioning rack 7")
+	}
+}
+
+// TestClient_DeleteWithQuery_NoQuery covers the defensive path: a nil/empty
+// query must still produce a clean DELETE against the bare path with no stray
+// "?" suffix. Matches the Get() shape (see client.go do()) so downstream tools
+// can reuse one code path.
+func TestClient_DeleteWithQuery_NoQuery(t *testing.T) {
+	var sawRawPath string
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		sawRawPath = r.URL.RequestURI()
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusOK)
+		_ = json.NewEncoder(w).Encode(map[string]interface{}{"ok": true})
+	}))
+	defer server.Close()
+
+	c := NewClient(server.URL, "")
+	if _, err := c.DeleteWithQuery("/api/v1/agents/ag-1", nil); err != nil {
+		t.Fatalf("DeleteWithQuery(nil query) err=%v want nil", err)
+	}
+	// No query → no ? suffix.
+	if strings.Contains(sawRawPath, "?") {
+		t.Errorf("raw path=%q contains stray ?; empty query must not serialize", sawRawPath)
+	}
+}
+
+// TestClient_DeleteWithQuery_204ReturnsMinimalBody covers the idempotent path.
+// The handler returns 204 No Content for an already-retired agent; the
+// existing do() helper normalises this to {"status":"deleted"}. The new
+// DeleteWithQuery must share that behavior so MCP tool authors don't have to
+// special-case the return shape.
+func TestClient_DeleteWithQuery_204ReturnsMinimalBody(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusNoContent)
+	}))
+	defer server.Close()
+
+	c := NewClient(server.URL, "")
+	data, err := c.DeleteWithQuery("/api/v1/agents/ag-1", nil)
+	if err != nil {
+		t.Fatalf("DeleteWithQuery(204) err=%v want nil (idempotent)", err)
+	}
+	if data == nil {
+		t.Fatal("DeleteWithQuery(204) returned nil; want synthetic body")
+	}
+	if !strings.Contains(string(data), "deleted") && !strings.Contains(string(data), "status") {
+		t.Errorf("DeleteWithQuery(204) body=%q; must surface a non-empty sentinel", string(data))
+	}
+}
+
+// TestClient_DeleteWithQuery_409PropagatesError covers the preflight-blocked
+// surface. A 409 with dependency counts must bubble up as a Go error so the
+// MCP tool can present it to the LLM operator rather than silently swallow
+// the rejection.
+func TestClient_DeleteWithQuery_409PropagatesError(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusConflict)
+		_ = json.NewEncoder(w).Encode(map[string]interface{}{
+			"error":   "blocked_by_dependencies",
+			"message": "agent has active targets",
+			"counts": map[string]int{
+				"active_targets":      3,
+				"active_certificates": 7,
+				"pending_jobs":        2,
+			},
+		})
+	}))
+	defer server.Close()
+
+	c := NewClient(server.URL, "")
+	_, err := c.DeleteWithQuery("/api/v1/agents/ag-1", nil)
+	if err == nil {
+		t.Fatalf("DeleteWithQuery(409) err=nil; 409 must propagate as Go error")
+	}
+	if !strings.Contains(err.Error(), "409") {
+		t.Errorf("err=%q should include HTTP status 409 for debuggability", err.Error())
+	}
+}
+
+// TestRetireAgentInput_ShapePinned is a compile-time assertion that the MCP
+// tool input struct for certctl_retire_agent exists with the required fields
+// and their expected tag shapes. The LLM discovers this input schema via
+// jsonschema tags — refactoring field names without updating callers silently
+// breaks tool discovery.
+//
+// Red until Phase 2b adds RetireAgentInput to internal/mcp/types.go. This
+// assertion deliberately exercises every field so the test fails at compile
+// time rather than runtime.
+func TestRetireAgentInput_ShapePinned(t *testing.T) {
+	// Zero-value construction of the expected input — fails to compile until
+	// the struct exists with fields {ID string, Force bool, Reason string}.
+	input := RetireAgentInput{
+		ID:     "ag-1",
+		Force:  true,
+		Reason: "decommissioning rack 7",
+	}
+
+	if input.ID != "ag-1" {
+		t.Errorf("RetireAgentInput.ID=%q want ag-1 (field binding broken)", input.ID)
+	}
+	if !input.Force {
+		t.Errorf("RetireAgentInput.Force=false want true")
+	}
+	if input.Reason != "decommissioning rack 7" {
+		t.Errorf("RetireAgentInput.Reason=%q want decommissioning rack 7", input.Reason)
+	}
+
+	// Also pin the JSON surface — LLMs send and receive these field names,
+	// so json tags must stay snake_case even through refactors.
+	encoded, err := json.Marshal(input)
+	if err != nil {
+		t.Fatalf("marshal RetireAgentInput: %v", err)
+	}
+	body := string(encoded)
+	for _, want := range []string{`"id":"ag-1"`, `"force":true`, `"reason":"decommissioning rack 7"`} {
+		if !strings.Contains(body, want) {
+			t.Errorf("RetireAgentInput JSON=%q missing %q (tag shape drifted)", body, want)
+		}
+	}
+}
+
+// TestListRetiredAgentsInput_ShapePinned mirrors the pagination input shape
+// used across the MCP toolset (see ListParams). The list-retired-agents tool
+// takes page + per_page with snake_case JSON tags. Compile-fail until
+// Phase 2b either adds ListRetiredAgentsInput or documents that list-retired
+// reuses the existing ListParams type (both paths are acceptable — the test
+// just pins whichever Phase 2b picks).
+func TestListRetiredAgentsInput_ShapePinned(t *testing.T) {
+	// Phase 2b may either (a) add a dedicated ListRetiredAgentsInput struct
+	// or (b) reuse the existing ListParams. Either is fine — we pin the
+	// field-access contract rather than the struct name to let the
+	// implementation choose. Compile-fail guards against the tool being
+	// registered without any pagination input at all.
+	var input ListParams
+	input.Page = 1
+	input.PerPage = 50
+	if input.Page != 1 || input.PerPage != 50 {
+		t.Errorf("ListParams fields Page/PerPage broken; listing pagination will misroute")
+	}
+}
@@ -506,6 +506,53 @@ func registerAgentTools(s *gomcp.Server, c *Client) {
 		}
 		return textResult(data)
 	})
+
+	// I-004: soft-retirement. DELETE /api/v1/agents/{id} returns 200 on a
+	// fresh retire (body echoes retired_at/already_retired/cascade/counts),
+	// 204 on an idempotent retire of an already-retired agent (do() in
+	// client.go normalizes that to {"status":"deleted"}), 409 when downstream
+	// dependencies block the retire and force wasn't set, 403 on sentinel
+	// agents, or 400 when force=true was sent without a reason. The tool
+	// forwards the raw handler response so the LLM operator sees the
+	// dependency counts and can decide whether to retry with force=true.
+	gomcp.AddTool(s, &gomcp.Tool{
+		Name:        "certctl_retire_agent",
+		Description: "Soft-retire an agent (DELETE /api/v1/agents/{id}). Sets retired_at + retired_reason on the row; the agent is filtered from the default listing and surfaces only via certctl_list_retired_agents. Default is a safety-gated soft-retire that returns 409 blocked_by_dependencies if the agent has active targets, active certificates, or pending jobs — the returned counts tell you what would be orphaned. Pass force=true to cascade through and retire those dependents too; force=true requires a non-empty reason (captured in the audit trail). Sentinel discovery agents (server-scanner, cloud-aws-sm, cloud-azure-kv, cloud-gcp-sm) cannot be retired — the handler returns 403 unconditionally. Idempotent: retrying on an already-retired agent returns 204 without side effects.",
+	}, func(ctx context.Context, req *gomcp.CallToolRequest, input RetireAgentInput) (*gomcp.CallToolResult, any, error) {
+		// Client-side mirror of the handler's ErrForceReasonRequired contract
+		// (see internal/api/handler/agents.go) so the LLM gets an immediate,
+		// actionable error instead of a round-trip 400. Whitespace-only
+		// reasons are treated as empty — matches handler's TrimSpace check.
+		if input.Force && input.Reason == "" {
+			return errorResult(fmt.Errorf("reason is required when force=true"))
+		}
+		query := url.Values{}
+		if input.Force {
+			query.Set("force", "true")
+		}
+		if input.Reason != "" {
+			query.Set("reason", input.Reason)
+		}
+		data, err := c.DeleteWithQuery("/api/v1/agents/"+input.ID, query)
+		if err != nil {
+			return errorResult(err)
+		}
+		return textResult(data)
+	})
+
+	// I-004: retired agents are filtered out of GET /api/v1/agents by default.
+	// The /agents/retired endpoint is the opt-in view — same pagination shape
+	// as the default listing, but filters to rows where retired_at IS NOT NULL.
+	gomcp.AddTool(s, &gomcp.Tool{
+		Name:        "certctl_list_retired_agents",
+		Description: "List soft-retired agents (GET /api/v1/agents/retired). These are agents that have been retired via certctl_retire_agent; retired_at and retired_reason are populated. Returned separately from certctl_list_agents so the default listing stays focused on operational agents.",
+	}, func(ctx context.Context, req *gomcp.CallToolRequest, input ListParams) (*gomcp.CallToolResult, any, error) {
+		data, err := c.Get("/api/v1/agents/retired", paginationQuery(input.Page, input.PerPage))
+		if err != nil {
+			return errorResult(err)
+		}
+		return textResult(data)
+	})
 }

 // ── Jobs ────────────────────────────────────────────────────────────
@@ -152,6 +152,23 @@ type AgentJobStatusInput struct {
 	Error   string `json:"error,omitempty" jsonschema:"Error message if job failed"`
 }

+// RetireAgentInput pins the MCP tool surface for certctl_retire_agent. I-004
+// introduces a soft-retirement flow that the handler exposes on DELETE
+// /api/v1/agents/{id} with two optional query flags: force=true cascades
+// through dependent active targets/certs/jobs, and reason is the human-readable
+// string captured in the audit trail. The handler enforces
+// ErrForceReasonRequired when force=true is sent without a reason; we surface
+// both as separate fields so the LLM can populate them independently and so
+// the retire_agent_test shape assertion stays aligned with the JSON-wire
+// contract. ID is always emitted (no omitempty) because a retire call without
+// a target agent is meaningless; Force and Reason are omitempty so the default
+// soft-retire path sends no query suffix at all.
+type RetireAgentInput struct {
+	ID     string `json:"id" jsonschema:"Agent ID to soft-retire"`
+	Force  bool   `json:"force,omitempty" jsonschema:"Cascade-retire downstream active targets, certs, and jobs (requires reason)"`
+	Reason string `json:"reason,omitempty" jsonschema:"Human-readable reason (required when force=true)"`
+}
+
 // ── Jobs ────────────────────────────────────────────────────────────

 type ListJobsInput struct {