From e48cc2074feb71ab498a9afabe7ed659594e3ddf Mon Sep 17 00:00:00 2001 From: Onur Solmaz Date: Wed, 1 Apr 2026 19:00:39 +0200 Subject: [PATCH 1/2] fix(api): support resolver-owned spec metadata --- api/create_admission.go | 10 +++ api/create_admission_test.go | 136 +++++++++++++++++++++++++++++++++++ api/extensions.go | 2 + 3 files changed, 148 insertions(+) diff --git a/api/create_admission.go b/api/create_admission.go index 5433a11..c383803 100644 --- a/api/create_admission.go +++ b/api/create_admission.go @@ -262,6 +262,16 @@ func applyPresetCreateResolverMutations(body *createRequest, response extensionR return presetCreateMutationResult{}, err } body.Spec.AgentRef = mergedAgentRef + specAnnotations, err := mergeMetadataStrict(body.Spec.Annotations, response.Mutations.Spec.Annotations, "spec annotation") + if err != nil { + return presetCreateMutationResult{}, err + } + body.Spec.Annotations = specAnnotations + specLabels, err := mergeMetadataStrict(body.Spec.Labels, response.Mutations.Spec.Labels, "spec label") + if err != nil { + return presetCreateMutationResult{}, err + } + body.Spec.Labels = specLabels } annotations, err := mergeMetadataStrict(body.Annotations, response.Mutations.Annotations, "annotation") if err != nil { diff --git a/api/create_admission_test.go b/api/create_admission_test.go index fc05b1a..271293a 100644 --- a/api/create_admission_test.go +++ b/api/create_admission_test.go @@ -403,6 +403,98 @@ func TestCreateSpritzStoresResolvedRuntimePolicy(t *testing.T) { } } +func TestCreateSpritzStoresResolvedSpecMetadata(t *testing.T) { + s := newCreateSpritzTestServer(t) + resolver := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + defer r.Body.Close() + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "status": "resolved", + "mutations": map[string]any{ + "spec": map[string]any{ + "serviceAccountName": "dev-agent-ag-123", + "annotations": map[string]string{ + "sidecar.istio.io/inject": "true", + }, + "labels": map[string]string{ + "example.com/network-profile": "github", + }, + }, + }, + }) + })) + defer resolver.Close() + + s.presets = presetCatalog{ + byID: []runtimePreset{{ + ID: "devbox", + Name: "Devbox", + Image: "example.com/devbox:latest", + NamePrefix: "devbox", + InstanceClass: "dev-runtime", + }}, + } + s.instanceClasses = instanceClassCatalog{ + byID: map[string]instanceClass{ + "dev-runtime": { + ID: "dev-runtime", + Version: "v1", + Creation: instanceClassCreationPolicy{ + RequireOwner: true, + RequiredResolvedFields: []string{ + requiredResolvedFieldServiceAccountName, + }, + }, + }, + }, + } + s.extensions = extensionRegistry{ + resolvers: []configuredResolver{{ + id: "runtime-binding", + extensionType: extensionTypeResolver, + operation: extensionOperationPresetCreateResolve, + match: extensionMatchRule{ + presetIDs: map[string]struct{}{"devbox": {}}, + }, + transport: configuredHTTPTransport{ + url: resolver.URL, + timeout: time.Second, + }, + }}, + } + + e := echo.New() + secured := e.Group("", s.authMiddleware()) + secured.POST("/api/spritzes", s.createSpritz) + + body := []byte(`{ + "name":"devbox-ocean", + "presetId":"devbox", + "spec":{} + }`) + req := httptest.NewRequest(http.MethodPost, "/api/spritzes", bytes.NewReader(body)) + req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON) + req.Header.Set("X-Spritz-User-Id", "user-1") + rec := httptest.NewRecorder() + + e.ServeHTTP(rec, req) + + if rec.Code != http.StatusCreated { + t.Fatalf("expected status 201, got %d: %s", rec.Code, rec.Body.String()) + } + + stored := &spritzv1.Spritz{} + if err := s.client.Get(context.Background(), client.ObjectKey{Name: "devbox-ocean", Namespace: s.namespace}, stored); err != nil { + t.Fatalf("expected created spritz resource: %v", err) + } + if stored.Spec.Annotations["sidecar.istio.io/inject"] != "true" { + t.Fatalf("expected resolved spec annotation, got %#v", stored.Spec.Annotations) + } + if stored.Spec.Labels["example.com/network-profile"] != "github" { + t.Fatalf("expected resolved spec label, got %#v", stored.Spec.Labels) + } +} + func TestCreateSpritzProvisionerPresetResolverReplaysWithResolvedBinding(t *testing.T) { s := newCreateSpritzTestServer(t) configureProvisionerTestServer(s) @@ -800,6 +892,50 @@ func TestCreateSpritzProvisionerRejectsManualRuntimePolicyForResolverRequiredFie } } +func TestCreateSpritzProvisionerRejectsManualSpecAnnotations(t *testing.T) { + s := newCreateSpritzTestServer(t) + configureProvisionerTestServer(s) + s.presets = presetCatalog{ + byID: []runtimePreset{{ + ID: "zeno", + Name: "Zeno", + Image: "example.com/zeno:latest", + NamePrefix: "zeno", + }}, + } + s.provisioners.allowedPresetIDs = map[string]struct{}{"zeno": {}} + + e := echo.New() + secured := e.Group("", s.authMiddleware()) + secured.POST("/api/spritzes", s.createSpritz) + + body := []byte(`{ + "presetId":"zeno", + "ownerId":"user-123", + "idempotencyKey":"manual-spec-annotations", + "spec":{ + "annotations":{ + "sidecar.istio.io/inject":"true" + } + } + }`) + req := httptest.NewRequest(http.MethodPost, "/api/spritzes", bytes.NewReader(body)) + req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON) + req.Header.Set("X-Spritz-User-Id", "zenobot") + req.Header.Set("X-Spritz-Principal-Type", "service") + req.Header.Set("X-Spritz-Principal-Scopes", "spritz.instances.create,spritz.instances.assign_owner") + rec := httptest.NewRecorder() + + e.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("expected status 400, got %d: %s", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "spec.annotations are not allowed") { + t.Fatalf("expected spec.annotations request-surface error, got %s", rec.Body.String()) + } +} + func TestCreateSpritzProvisionerRejectsManualRuntimePolicyWhenResolverOnlySetsServiceAccount(t *testing.T) { s := newCreateSpritzTestServer(t) configureProvisionerTestServer(s) diff --git a/api/extensions.go b/api/extensions.go index bcf1699..e8b7d80 100644 --- a/api/extensions.go +++ b/api/extensions.go @@ -125,6 +125,8 @@ type extensionResolverSpecMutation struct { ServiceAccountName string `json:"serviceAccountName,omitempty"` AgentRef *spritzv1.SpritzAgentRef `json:"agentRef,omitempty"` RuntimePolicy *spritzv1.SpritzRuntimePolicy `json:"runtimePolicy,omitempty"` + Annotations map[string]string `json:"annotations,omitempty"` + Labels map[string]string `json:"labels,omitempty"` } type configuredResolver struct { From 72ce1ee9e26118eba31c88e541873545b1b57db5 Mon Sep 17 00:00:00 2001 From: Onur Solmaz Date: Wed, 1 Apr 2026 19:05:51 +0200 Subject: [PATCH 2/2] fix(slack-gateway): preserve ACP reply whitespace --- acptext/go.mod | 3 + acptext/text.go | 53 ++++++++ acptext/text_test.go | 37 ++++++ api/acp_prompt.go | 49 +------- api/go.mod | 3 + ...ack-channel-gateway-implementation-plan.md | 25 ++++ integrations/slack-gateway/acp_client.go | 53 ++------ integrations/slack-gateway/backend_client.go | 2 +- integrations/slack-gateway/gateway_test.go | 117 +++++++++++++++++- integrations/slack-gateway/go.mod | 7 +- 10 files changed, 253 insertions(+), 96 deletions(-) create mode 100644 acptext/go.mod create mode 100644 acptext/text.go create mode 100644 acptext/text_test.go diff --git a/acptext/go.mod b/acptext/go.mod new file mode 100644 index 0000000..e973c6b --- /dev/null +++ b/acptext/go.mod @@ -0,0 +1,3 @@ +module spritz.sh/acptext + +go 1.25.0 diff --git a/acptext/text.go b/acptext/text.go new file mode 100644 index 0000000..aa8c5fe --- /dev/null +++ b/acptext/text.go @@ -0,0 +1,53 @@ +package acptext + +import ( + "fmt" + "strings" +) + +// Extract returns the readable text content represented by one ACP payload +// without trimming or normalizing whitespace. +func Extract(value any) string { + switch typed := value.(type) { + case nil: + return "" + case string: + return typed + case []any: + parts := make([]string, 0, len(typed)) + for _, item := range typed { + text := Extract(item) + if text == "" { + continue + } + parts = append(parts, text) + } + return strings.Join(parts, "\n") + case map[string]any: + if text, ok := typed["text"].(string); ok { + return text + } + if content, ok := typed["content"]; ok { + return Extract(content) + } + if resource, ok := typed["resource"]; ok { + return Extract(resource) + } + if uri, ok := typed["uri"].(string); ok { + return uri + } + return "" + default: + return fmt.Sprint(typed) + } +} + +// JoinChunks concatenates ACP chunk payloads without injecting separators or +// trimming whitespace at chunk boundaries. +func JoinChunks(values []any) string { + var builder strings.Builder + for _, value := range values { + builder.WriteString(Extract(value)) + } + return builder.String() +} diff --git a/acptext/text_test.go b/acptext/text_test.go new file mode 100644 index 0000000..1698548 --- /dev/null +++ b/acptext/text_test.go @@ -0,0 +1,37 @@ +package acptext + +import "testing" + +func TestExtractPreservesWhitespaceInTextBlocks(t *testing.T) { + got := Extract([]any{ + map[string]any{"text": "hello"}, + map[string]any{"text": " world"}, + map[string]any{"text": "\nagain"}, + }) + want := "hello\n world\n\nagain" + if got != want { + t.Fatalf("expected %q, got %q", want, got) + } +} + +func TestExtractSupportsResourceBlocks(t *testing.T) { + if got := Extract(map[string]any{"resource": map[string]any{"text": "resource text"}}); got != "resource text" { + t.Fatalf("expected resource text, got %q", got) + } + if got := Extract(map[string]any{"resource": map[string]any{"uri": "file://workspace/report.txt"}}); got != "file://workspace/report.txt" { + t.Fatalf("expected resource uri, got %q", got) + } +} + +func TestJoinChunksPreservesChunkBoundaryWhitespaceAndNewlines(t *testing.T) { + got := JoinChunks([]any{ + []any{map[string]any{"text": "I'll "}}, + []any{map[string]any{"text": "spawn a dedicated agent for you using the"}}, + []any{map[string]any{"text": "\nSpritz controls.\n\nThe"}}, + []any{map[string]any{"text": " Slack account could not be resolved.\n"}}, + }) + want := "I'll spawn a dedicated agent for you using the\nSpritz controls.\n\nThe Slack account could not be resolved.\n" + if got != want { + t.Fatalf("expected %q, got %q", want, got) + } +} diff --git a/api/acp_prompt.go b/api/acp_prompt.go index 78c4529..a0f222f 100644 --- a/api/acp_prompt.go +++ b/api/acp_prompt.go @@ -6,6 +6,8 @@ import ( "fmt" "strings" "time" + + "spritz.sh/acptext" ) type acpPromptResult struct { @@ -156,50 +158,5 @@ func assistantTextFromACPUpdates(updates []map[string]any) string { } chunks = append(chunks, update["content"]) } - return joinACPTextChunks(chunks) -} - -func joinACPTextChunks(values []any) string { - var builder strings.Builder - for _, value := range values { - builder.WriteString(extractACPText(value)) - } - return builder.String() -} - -func extractACPText(value any) string { - switch typed := value.(type) { - case nil: - return "" - case string: - return typed - case []any: - parts := make([]string, 0, len(typed)) - for _, item := range typed { - text := extractACPText(item) - if text == "" { - continue - } - parts = append(parts, text) - } - return strings.Join(parts, "\n") - case map[string]any: - if text, ok := typed["text"].(string); ok { - return text - } - if content, ok := typed["content"]; ok { - return extractACPText(content) - } - if resource, ok := typed["resource"].(map[string]any); ok { - if text, ok := resource["text"].(string); ok { - return text - } - if uri, ok := resource["uri"].(string); ok { - return uri - } - } - return "" - default: - return fmt.Sprint(typed) - } + return acptext.JoinChunks(chunks) } diff --git a/api/go.mod b/api/go.mod index e6c5a2b..9b5d6d7 100644 --- a/api/go.mod +++ b/api/go.mod @@ -14,6 +14,7 @@ require ( k8s.io/apimachinery v0.35.0 k8s.io/client-go v0.35.0 sigs.k8s.io/controller-runtime v0.22.4 + spritz.sh/acptext v0.0.0-00010101000000-000000000000 spritz.sh/operator v0.0.0-00010101000000-000000000000 ) @@ -77,4 +78,6 @@ require ( sigs.k8s.io/yaml v1.6.0 // indirect ) +replace spritz.sh/acptext => ../acptext + replace spritz.sh/operator => ../operator diff --git a/docs/2026-03-24-slack-channel-gateway-implementation-plan.md b/docs/2026-03-24-slack-channel-gateway-implementation-plan.md index b317faa..d4df99a 100644 --- a/docs/2026-03-24-slack-channel-gateway-implementation-plan.md +++ b/docs/2026-03-24-slack-channel-gateway-implementation-plan.md @@ -445,6 +445,29 @@ The gateway should also not mark delivery success just because session exchange returned `resolved`. Success means the prompt has actually been handed off to the runtime and the normal reply path can continue. +## ACP Reply Text Integrity + +The Slack gateway must treat ACP assistant text as lossless content, not as +display text that may be normalized. + +That means: + +- `agent_message_chunk` text must be assembled without trimming individual + chunks +- spaces and newlines at chunk boundaries are part of the payload and must be + preserved +- the gateway may trim only for emptiness checks at the final boundary, not as + part of text extraction or chunk joining +- channel adapters should reuse one shared ACP text extraction and chunk-join + helper instead of reimplementing their own whitespace rules + +If this contract is violated, the provider-visible reply can silently corrupt +content even when the runtime output is correct. Typical failures are: + +- merged words across chunk boundaries +- lost paragraph breaks +- flattened lists or code blocks + ## Threading Defaults Phase 1 should keep channel behavior predictable: @@ -519,6 +542,8 @@ Before calling Phase 1 done, verify: 16. Duplicate Slack webhook deliveries converge on the same pending delivery. 17. The first recovered Slack turn is not marked successful until the prompt is actually accepted by ACP. +18. Multiline assistant replies preserve spaces and newlines across ACP chunk + boundaries. ## Follow-ups diff --git a/integrations/slack-gateway/acp_client.go b/integrations/slack-gateway/acp_client.go index ce5c517..52e99f5 100644 --- a/integrations/slack-gateway/acp_client.go +++ b/integrations/slack-gateway/acp_client.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/gorilla/websocket" + "spritz.sh/acptext" ) type acpRPCMessage struct { @@ -57,7 +58,7 @@ func (g *slackGateway) promptConversation(ctx context.Context, serviceToken, nam }, nil); err != nil { return "", false, err } - var reply strings.Builder + chunks := make([]any, 0, 8) if _, promptSent, err := client.call(ctx, "session/prompt", map[string]any{ "sessionId": sessionID, "prompt": []map[string]any{{ @@ -74,15 +75,15 @@ func (g *slackGateway) promptConversation(ctx context.Context, serviceToken, nam if err := json.Unmarshal(message.Params, &payload); err != nil { return } - if strings.TrimSpace(stringValue(payload.Update["sessionUpdate"])) != "agent_message_chunk" { + if strings.TrimSpace(fmt.Sprint(payload.Update["sessionUpdate"])) != "agent_message_chunk" { return } - reply.WriteString(extractACPText(payload.Update["content"])) + chunks = append(chunks, payload.Update["content"]) }); err != nil { - return strings.TrimSpace(reply.String()), promptSent, err + return acptext.JoinChunks(chunks), promptSent, err } - text := strings.TrimSpace(reply.String()) - if text == "" { + text := acptext.JoinChunks(chunks) + if strings.TrimSpace(text) == "" { return "", true, fmt.Errorf("agent returned an empty reply") } return text, true, nil @@ -164,43 +165,3 @@ func (c *acpPromptClient) call(ctx context.Context, method string, params any, o return message.Result, delivered, nil } } - -func extractACPText(value any) string { - switch typed := value.(type) { - case nil: - return "" - case string: - return typed - case []any: - parts := make([]string, 0, len(typed)) - for _, item := range typed { - if text := extractACPText(item); text != "" { - parts = append(parts, text) - } - } - return strings.Join(parts, "\n") - case map[string]any: - if text := stringValue(typed["text"]); text != "" { - return text - } - if content, ok := typed["content"]; ok { - return extractACPText(content) - } - if resource, ok := typed["resource"]; ok { - return extractACPText(resource) - } - if uri := stringValue(typed["uri"]); uri != "" { - return uri - } - } - return "" -} - -func stringValue(value any) string { - switch typed := value.(type) { - case string: - return strings.TrimSpace(typed) - default: - return "" - } -} diff --git a/integrations/slack-gateway/backend_client.go b/integrations/slack-gateway/backend_client.go index 5eea1e9..655bace 100644 --- a/integrations/slack-gateway/backend_client.go +++ b/integrations/slack-gateway/backend_client.go @@ -229,7 +229,7 @@ func (g *slackGateway) bootstrapConversation(ctx context.Context, serviceToken, func (g *slackGateway) postSlackMessage(ctx context.Context, token, channel, text, threadTS string) (string, error) { body := map[string]any{ "channel": strings.TrimSpace(channel), - "text": strings.TrimSpace(text), + "text": text, } if threadTS = strings.TrimSpace(threadTS); threadTS != "" { body["thread_ts"] = threadTS diff --git a/integrations/slack-gateway/gateway_test.go b/integrations/slack-gateway/gateway_test.go index be768bb..d3e6815 100644 --- a/integrations/slack-gateway/gateway_test.go +++ b/integrations/slack-gateway/gateway_test.go @@ -20,6 +20,7 @@ import ( "time" "github.com/gorilla/websocket" + "spritz.sh/acptext" ) func TestOAuthCallbackStoresInstallationAndUpsertsRegistry(t *testing.T) { @@ -266,7 +267,7 @@ func TestOAuthCallbackReturnsBadGatewayWhenBackendUpsertFails(t *testing.T) { } func TestExtractACPTextSupportsResourceBlocks(t *testing.T) { - resourceText := extractACPText(map[string]any{ + resourceText := acptext.Extract(map[string]any{ "resource": map[string]any{ "text": "resource text", }, @@ -275,7 +276,7 @@ func TestExtractACPTextSupportsResourceBlocks(t *testing.T) { t.Fatalf("expected resource text, got %q", resourceText) } - resourceURI := extractACPText(map[string]any{ + resourceURI := acptext.Extract(map[string]any{ "resource": map[string]any{ "uri": "file://workspace/report.txt", }, @@ -1648,6 +1649,118 @@ func TestPromptConversationRejectsInteractivePermissionRequests(t *testing.T) { } } +func TestPromptConversationPreservesChunkBoundaryWhitespaceAndNewlines(t *testing.T) { + upgrader := websocket.Upgrader{CheckOrigin: func(r *http.Request) bool { return true }} + spritz := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/acp/conversations/conv-1/connect" { + t.Fatalf("unexpected spritz path %s", r.URL.Path) + } + conn, err := upgrader.Upgrade(w, r, nil) + if err != nil { + t.Fatalf("upgrade failed: %v", err) + } + defer conn.Close() + for { + _, payload, err := conn.ReadMessage() + if err != nil { + return + } + var message map[string]any + if err := json.Unmarshal(payload, &message); err != nil { + t.Fatalf("decode ws payload: %v", err) + } + switch message["method"] { + case "initialize": + _ = conn.WriteJSON(map[string]any{"jsonrpc": "2.0", "id": message["id"], "result": map[string]any{"protocolVersion": 1}}) + case "session/load": + _ = conn.WriteJSON(map[string]any{"jsonrpc": "2.0", "id": message["id"], "result": map[string]any{}}) + case "session/prompt": + for _, chunk := range []string{ + "I'll ", + "spawn a dedicated agent for you using the", + "\nSpritz controls.\n\nThe", + " Slack account could not be resolved.\n", + } { + _ = conn.WriteJSON(map[string]any{ + "jsonrpc": "2.0", + "method": "session/update", + "params": map[string]any{ + "update": map[string]any{ + "sessionUpdate": "agent_message_chunk", + "content": []map[string]any{{ + "type": "text", + "text": chunk, + }}, + }, + }, + }) + } + _ = conn.WriteJSON(map[string]any{"jsonrpc": "2.0", "id": message["id"], "result": map[string]any{}}) + return + default: + t.Fatalf("unexpected ACP method %#v", message["method"]) + } + } + })) + defer spritz.Close() + + cfg := config{ + SpritzBaseURL: spritz.URL, + HTTPTimeout: 5 * time.Second, + } + gateway := newSlackGateway(cfg, slog.New(slog.NewTextHandler(io.Discard, nil))) + + reply, promptSent, err := gateway.promptConversation( + t.Context(), + "owner-token", + "spritz-staging", + "conv-1", + "session-1", + "/home/dev", + "hello", + ) + if err != nil { + t.Fatalf("promptConversation returned error: %v", err) + } + if !promptSent { + t.Fatalf("expected prompt delivery to be marked as sent") + } + want := "I'll spawn a dedicated agent for you using the\nSpritz controls.\n\nThe Slack account could not be resolved.\n" + if reply != want { + t.Fatalf("expected reply %q, got %q", want, reply) + } +} + +func TestPostSlackMessagePreservesTextWhitespace(t *testing.T) { + var payload map[string]any + slackAPI := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/chat.postMessage" { + t.Fatalf("unexpected slack path %s", r.URL.Path) + } + if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { + t.Fatalf("decode slack post body: %v", err) + } + writeJSON(w, http.StatusOK, map[string]any{"ok": true, "ts": "1711387376.000100"}) + })) + defer slackAPI.Close() + + gateway := newSlackGateway( + config{ + SlackAPIBaseURL: slackAPI.URL, + HTTPTimeout: 5 * time.Second, + }, + slog.New(slog.NewTextHandler(io.Discard, nil)), + ) + + text := "\nFirst line\n\n- bullet\n" + if _, err := gateway.postSlackMessage(t.Context(), "xoxb-installed", "C_1", text, ""); err != nil { + t.Fatalf("postSlackMessage returned error: %v", err) + } + if payload["text"] != text { + t.Fatalf("expected text %q, got %#v", text, payload["text"]) + } +} + func TestProcessMessageEventPostsFallbackAfterPromptTimeout(t *testing.T) { var slackPayloads struct { sync.Mutex diff --git a/integrations/slack-gateway/go.mod b/integrations/slack-gateway/go.mod index 6dd82f2..2ac93ab 100644 --- a/integrations/slack-gateway/go.mod +++ b/integrations/slack-gateway/go.mod @@ -2,4 +2,9 @@ module spritz.sh/integrations/slack-gateway go 1.25.0 -require github.com/gorilla/websocket v1.5.3 +require ( + github.com/gorilla/websocket v1.5.3 + spritz.sh/acptext v0.0.0-00010101000000-000000000000 +) + +replace spritz.sh/acptext => ../../acptext