Serve capabilities through one serializer and add GET /api/v1/executions

The two "refactor later" items from REVIEW-2026-07-30.md; they share the wire types, so they land together. A capability had four divergent wire shapes — the HTTP handler, the MCP adapter, pkg/client, and Maven's vendored copy of it. There is now a single definition in pkg/client, mapped from domain by internal/wire and used by the HTTP list/create/get paths and all four MCP surfaces. It lives in pkg/client rather than internal so external consumers need not vendor internal/domain, and so producer and consumer are literally the same type. The unified shape is a strict superset of all four predecessors; nothing was dropped. It adds enabled and requires_confirmation to the list responses (never omitempty — an absent bool reads as unknown, not false), capability_id to the MCP and client shapes, and the timing/attribute/version fields previously only on get-by-ID. target_types and the list itself now serialize as [] rather than null. Both `id` and `capability_id` are deliberately kept, carrying the same value. Maven decodes `id`; the spec and the rest of the API say `capability_id`. Bearer auth is already a breaking change for that consumer, and stacking a second silent one is the wrong trade — the redundancy stays until every consumer is confirmed on capability_id, then `id` goes in an announced removal. A test pins this and says so. GET /api/v1/executions?entity_id=&since=&limit= implements spec §4.5, which the Command Center needs. `since` reuses the changes-feed cursor convention rather than inventing a second paging idiom. That cursor is the row's implicit SQLite rowid, which is safe only while nothing deletes executions and nothing VACUUMs — both would renumber and silently invalidate outstanding cursors. If retention is ever added, this must become an explicit monotonic column first; the constraint is documented at the query site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uea55zaiWuEByEDC4UBSdd
2026-07-30 23:40:20 +04:00
parent 9e6b995538
commit dda4acfbb6
10 changed files with 926 additions and 67 deletions
@@ -0,0 +1,207 @@
+package wire
+
+import (
+	"encoding/json"
+	"testing"
+	"time"
+
+	"github.com/kami/hexis/internal/domain"
+	"github.com/kami/hexis/pkg/client"
+)
+
+func sampleCapability() *domain.Capability {
+	now := time.Date(2026, 7, 30, 12, 0, 0, 0, time.UTC)
+	return &domain.Capability{
+		ID:                   "cap_docker_restart",
+		Name:                 "docker.restart_container",
+		Description:          "Restart a container",
+		TargetTypes:          []string{"container"},
+		TargetEntityID:       "",
+		Provider:             "workspace_mcp",
+		Operation:            "docker.restart_container",
+		Risk:                 domain.RiskMedium,
+		ReadOnly:             false,
+		ExpectedSideEffects:  "container restarts",
+		RequiresConfirmation: true,
+		Enabled:              true,
+		TimeoutSeconds:       30,
+		Attributes:           map[string]any{"allowlist": "docker"},
+		CreatedAt:            now,
+		UpdatedAt:            now,
+		Version:              1,
+	}
+}
+
+func marshalCapability(t *testing.T, c *domain.Capability) map[string]any {
+	t.Helper()
+	data, err := json.Marshal(Capability(c))
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
+	var out map[string]any
+	if err := json.Unmarshal(data, &out); err != nil {
+		t.Fatalf("unmarshal: %v", err)
+	}
+	return out
+}
+
+// TestCapability_EmitsBothIDAliases pins the deliberate redundancy documented
+// on client.Capability. Maven's consumer reads `id`; the spec and the rest of
+// the API say `capability_id`. Hexis is already breaking that consumer with
+// bearer auth, so dropping either alias here would be a second, silent break.
+// If this test is ever changed, it must be as an announced removal, not a
+// tidy-up.
+func TestCapability_EmitsBothIDAliases(t *testing.T) {
+	out := marshalCapability(t, sampleCapability())
+
+	id, hasID := out["id"]
+	capID, hasCapID := out["capability_id"]
+	if !hasID {
+		t.Error("`id` missing — Maven's vendored client decodes this field")
+	}
+	if !hasCapID {
+		t.Error("`capability_id` missing — this is the ECOSYSTEM-SPEC.md §4.1 name")
+	}
+	if id != capID {
+		t.Errorf("aliases disagree: id=%v capability_id=%v", id, capID)
+	}
+	if id != "cap_docker_restart" {
+		t.Errorf("unexpected id %v", id)
+	}
+}
+
+// TestCapability_CarriesServerDerivedGuards pins the review finding that
+// listCapabilities omitted `enabled` and `requires_confirmation`, leaving a
+// client unable to distinguish a callable capability from one that would be
+// refused with 403. Both are always present, even when false.
+func TestCapability_CarriesServerDerivedGuards(t *testing.T) {
+	out := marshalCapability(t, sampleCapability())
+
+	if got, ok := out["enabled"].(bool); !ok || !got {
+		t.Errorf("enabled: expected true, got %#v", out["enabled"])
+	}
+	if got, ok := out["requires_confirmation"].(bool); !ok || !got {
+		t.Errorf("requires_confirmation: expected true, got %#v", out["requires_confirmation"])
+	}
+
+	// Both must be emitted even at their zero value — `omitempty` here would
+	// read to a client as "unknown", not "false".
+	c := sampleCapability()
+	c.Enabled = false
+	c.RequiresConfirmation = false
+	out = marshalCapability(t, c)
+	if _, ok := out["enabled"]; !ok {
+		t.Error("`enabled` omitted when false; it must always be present")
+	}
+	if _, ok := out["requires_confirmation"]; !ok {
+		t.Error("`requires_confirmation` omitted when false; it must always be present")
+	}
+}
+
+// TestCapability_IsASupersetOfEveryPreviousShape guards against a silent field
+// removal for consumers of any of the four shapes this serializer replaced:
+// the HTTP list response, the HTTP get response (raw domain.Capability), the
+// MCP adapter's map, and pkg/client.
+func TestCapability_IsASupersetOfEveryPreviousShape(t *testing.T) {
+	out := marshalCapability(t, sampleCapability())
+
+	required := []string{
+		// union of the old listCapabilities map and the MCP adapter map
+		"id", "capability_id", "name", "description", "target_types",
+		"provider", "operation", "risk", "read_only", "expected_side_effects",
+		// previously only on the raw domain.Capability returned by GET by ID
+		"requires_confirmation", "enabled", "timeout_seconds", "attributes",
+		"created_at", "updated_at", "version",
+	}
+	for _, k := range required {
+		if _, ok := out[k]; !ok {
+			t.Errorf("field %q dropped by the unified serializer", k)
+		}
+	}
+}
+
+// TestCapability_TargetTypesNeverNull — clients range over this array; `null`
+// is a decode hazard for the non-Go consumers the Command Center will add.
+func TestCapability_TargetTypesNeverNull(t *testing.T) {
+	c := sampleCapability()
+	c.TargetTypes = nil
+	out := marshalCapability(t, c)
+
+	tt, ok := out["target_types"].([]any)
+	if !ok {
+		t.Fatalf("target_types is %#v, want []", out["target_types"])
+	}
+	if len(tt) != 0 {
+		t.Errorf("expected empty array, got %v", tt)
+	}
+}
+
+// TestCapabilities_EmptySliceIsNotNull — the HTTP list endpoint must render
+// `[]`, not `null`, when an entity has no capabilities.
+func TestCapabilities_EmptySliceIsNotNull(t *testing.T) {
+	data, err := json.Marshal(Capabilities(nil))
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
+	if string(data) != "[]" {
+		t.Errorf("expected [], got %s", string(data))
+	}
+}
+
+// TestCapability_RoundTripsThroughPublicClient closes the loop: what the
+// server serializes is exactly what pkg/client — and therefore Maven —
+// decodes. A drift between producer and consumer shape is the whole class of
+// bug this unification exists to remove.
+func TestCapability_RoundTripsThroughPublicClient(t *testing.T) {
+	src := sampleCapability()
+	data, err := json.Marshal(Capability(src))
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
+
+	var got client.Capability
+	if err := json.Unmarshal(data, &got); err != nil {
+		t.Fatalf("decode into client.Capability: %v", err)
+	}
+
+	if got.ID != src.ID || got.CapabilityID != src.ID {
+		t.Errorf("id round-trip: ID=%q CapabilityID=%q want %q", got.ID, got.CapabilityID, src.ID)
+	}
+	if got.EffectiveID() != src.ID {
+		t.Errorf("EffectiveID() = %q, want %q", got.EffectiveID(), src.ID)
+	}
+	if got.Name != src.Name || got.ReadOnly != src.ReadOnly || got.Risk != src.Risk {
+		t.Errorf("field drift: %+v", got)
+	}
+	if !got.Enabled || !got.RequiresConfirmation {
+		t.Errorf("guard fields lost in round trip: %+v", got)
+	}
+	if got.Version != src.Version || !got.CreatedAt.Equal(src.CreatedAt) {
+		t.Errorf("metadata lost in round trip: %+v", got)
+	}
+}
+
+// TestCapability_EffectiveIDToleratesEitherAlias covers a peer (an older Hexis
+// or a hand-rolled client) that sends only one of the two names.
+func TestCapability_EffectiveIDToleratesEitherAlias(t *testing.T) {
+	cases := map[string]string{
+		`{"id":"cap_x"}`:                         "cap_x",
+		`{"capability_id":"cap_y"}`:              "cap_y",
+		`{"id":"cap_z","capability_id":"cap_z"}`: "cap_z",
+	}
+	for body, want := range cases {
+		var c client.Capability
+		if err := json.Unmarshal([]byte(body), &c); err != nil {
+			t.Fatalf("decode %s: %v", body, err)
+		}
+		if got := c.EffectiveID(); got != want {
+			t.Errorf("%s: EffectiveID() = %q, want %q", body, got, want)
+		}
+	}
+}
+
+func TestCapability_NilIsZeroValue(t *testing.T) {
+	if got := Capability(nil); got.EffectiveID() != "" {
+		t.Errorf("nil capability produced %+v", got)
+	}
+}