Browser toolbox D: approval screenshots

PRIMITIVES.md

@@ -127,7 +127,18 @@ MCP servers, validated against a toolspec.
   cookies + localStorage after each acting call and on graceful stop into a
   per-session Secret — the one Secret the runner is RBAC-allowed to write, never
   model-visible — and re-injects it when the pod wakes after a reap, so a login
-  survives pod churn (`pkg/agent/tool/browser/capture.go`).
+  survives pod churn (`pkg/agent/tool/browser/capture.go`). Phase D attaches a
+  **redacted** page screenshot to every browser approval/human-step card — the
+  acting-approval (`web_host#act`) plan-gate ask, `request_human_step`'s
+  confirm/code prompts, and the CAPTCHA notice — so the human deciding sees the
+  same page the agent does, not just its summary: credential-filled fields are
+  painted opaque black (`pkg/agent/tool/browser/screenshot_capture.go` resolves
+  each filled field's box via `dom.GetBoxModel` and fails closed on any
+  unresolved box; `pkg/agent/tool/browser/screenshot_redact.go` does the pixel
+  paint-over) before the PNG is ever stored, and the screenshot is attached to
+  `channelevents.InteractionRequestPayload.Screenshot` — never placed in the
+  model's context. Slack uploads it into the approval thread, the web chat
+  embeds it inline, and the local TUI links it via a signed artifact-view URL.
 - **Binaries:** `internal/cmd/streamclient/` (claims a streaming ToolCall via
   the gateway); tool execution is otherwise driven from `internal/cmd/runner/`
 

pkg/agent/runner/browser_screenshot.go

@@ -0,0 +1,202 @@
+package runner
+
+import (
+	"bytes"
+	"context"
+	"fmt"
+	"io"
+	"log/slog"
+
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	browsertool "github.com/authzed/openagentprimitives/pkg/agent/tool/browser"
+	spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1"
+	"github.com/authzed/openagentprimitives/pkg/channels/channelevents"
+	"github.com/authzed/openagentprimitives/pkg/memory"
+	memartifact "github.com/authzed/openagentprimitives/pkg/memory/kinds/artifact"
+	"github.com/authzed/openagentprimitives/pkg/platform/artifacts"
+	"github.com/authzed/openagentprimitives/pkg/platform/artifactstore"
+)
+
+// screenshotInlineMaxBytes is the ArtifactRenderSpec.Payload inline cap
+// (pkg/apis/v1alpha1/artifactrender_types.go:57-65: "Payload inline, ≤256
+// KiB. Exactly one of Payload or PayloadRef MUST be set."). A redacted
+// screenshot at or under this size is attached inline; a larger one is
+// written to the artifact store first and referenced by PayloadRef.
+const screenshotInlineMaxBytes = 256 << 10
+
+// Fixed metadata for every screenshot artifact this package creates: one
+// filename/alt-text for the whole feature, so it only needs saying once.
+// The renderer kind and MIME are NOT literals here — see
+// channelevents.ScreenshotRendererKind / ScreenshotMIME for why (this
+// package is under mime_opinion_guard_test.go).
+const (
+	screenshotFilename = "page.png"
+	screenshotAltText  = "current page (credential fields redacted)"
+)
+
+// screenshotArtifactPutter is the subset of artifactstore.Store that
+// storeScreenshotArtifact needs: Put, to store a screenshot too large to
+// inline. Narrowed (mirroring secretOutputStatusWriter in loop_secretout.go
+// and uiResourceStatusWriter in loop_uiresource.go) so a test can provide a
+// minimal recorder without constructing a full artifactstore.Store.
+type screenshotArtifactPutter interface {
+	Put(ctx context.Context, key string, r io.Reader) (artifactstore.Ref, error)
+}
+
+// screenshotStoreDeps bundles the backend wiring storeScreenshotArtifact
+// needs to turn redacted screenshot bytes into a durable ArtifactRender CR:
+// the CR client, the artifact store (consulted only when the PNG exceeds the
+// inline cap), and the session's UID for the CR's owner-ref.
+//
+// NewRenderName and NewArtifactID are optional id seams: nil means
+// production (artifacts.RenderName and memory.NewID(memartifact.Kind{}),
+// the same pair artifacts.Service itself defaults to) — mirroring
+// artifacts.Service's own WithRenderNameMinter/newID seams so tests can
+// assert on a deterministic CR name and artifact id instead of a random one.
+type screenshotStoreDeps struct {
+	Client        client.Client
+	ArtifactStore screenshotArtifactPutter
+	SessionUID    types.UID
+
+	NewRenderName func(session string) string
+	NewArtifactID func() string
+}
+
+// storeScreenshotArtifact persists a redacted page screenshot (the PNG bytes
+// Task 2 produces) as an ArtifactRender{Kind:"image"} CR and returns the
+// AttachmentRef the interaction request payload's Screenshot field carries.
+//
+// Mirrors the CR-build shape in pkg/agent/tool/meta/artifact_prepare.go:264-274
+// and pkg/platform/artifacts.NewRender: owned by the session (by UID, as
+// controller, blocking deletion), labeled with the artifact id. Unlike
+// artifact_prepare / persistWidget (loop_uiresource.go) this does NOT poll the
+// CR to Ready: a screenshot attaches to a live interaction request, not a
+// model-facing handle, and AttachmentRef's own contract (see the doc comment
+// on channelevents.AttachmentRef) is that the consumer re-Gets the CR and
+// reads status.outputRef at delivery time — there is nothing for this call to
+// wait on.
+//
+// A PNG at or under screenshotInlineMaxBytes is attached inline
+// (Spec.Payload); a larger one is written to the artifact store first and
+// referenced by Spec.PayloadRef. Exactly one of the two is ever set.
+func storeScreenshotArtifact(ctx context.Context, deps screenshotStoreDeps, sess channelevents.SessionRef, png []byte) (*channelevents.AttachmentRef, error) {
+	// Fail closed on a nil Client with an ERROR, not a nil-interface panic:
+	// deps.Client is a plain client.Client interface field (Loop.Client), and a
+	// Loop built without one (a kubectl-driven session, today's e2e in-process
+	// harness — see Loop.screenshotStoreDeps' doc) must degrade to "ship no
+	// screenshot" like any other capture/store failure, never crash the
+	// dispatch goroutine calling it.
+	if deps.Client == nil {
+		return nil, fmt.Errorf("storeScreenshotArtifact: no k8s client wired (Loop.Client is nil)")
+	}
+	newRenderName := deps.NewRenderName
+	if newRenderName == nil {
+		newRenderName = artifacts.RenderName
+	}
+	newArtifactID := deps.NewArtifactID
+	if newArtifactID == nil {
+		newArtifactID = func() string { return memory.NewID(memartifact.Kind{}) }
+	}
+
+	artifactID := newArtifactID()
+	renderName := newRenderName(sess.Name)
+
+	spec := spiceboxv1alpha1.ArtifactRenderSpec{
+		Kind:     channelevents.ScreenshotRendererKind,
+		Filename: screenshotFilename,
+		AltText:  screenshotAltText,
+	}
+	if len(png) <= screenshotInlineMaxBytes {
+		spec.Payload = png
+	} else {
+		if deps.ArtifactStore == nil {
+			return nil, fmt.Errorf("storeScreenshotArtifact: screenshot is %d bytes (> %d-byte inline cap) but no artifact store is wired", len(png), screenshotInlineMaxBytes)
+		}
+		key := fmt.Sprintf("%s/%s/%s/%s", sess.Namespace, sess.Name, artifactID, screenshotFilename)
+		ref, err := deps.ArtifactStore.Put(ctx, key, bytes.NewReader(png))
+		if err != nil {
+			return nil, fmt.Errorf("storeScreenshotArtifact: store screenshot bytes: %w", err)
+		}
+		spec.PayloadRef = string(ref)
+	}
+
+	cr := artifacts.NewRender(renderName, sess.Namespace, sess.Name, deps.SessionUID, artifactID, spec, nil)
+	if err := deps.Client.Create(ctx, cr); err != nil {
+		return nil, fmt.Errorf("storeScreenshotArtifact: create ArtifactRender: %w", err)
+	}
+
+	return &channelevents.AttachmentRef{
+		RenderName: cr.Name,
+		ArtifactID: artifactID,
+		MIME:       channelevents.ScreenshotMIME,
+		Filename:   screenshotFilename,
+		AltText:    screenshotAltText,
+	}, nil
+}
+
+// screenshotStoreDeps builds storeScreenshotArtifact's backend wiring from
+// this Loop's own fields: l.Client creates the ArtifactRender CR — the same
+// value applyUIResource's own CR create+poll uses (loop.go's
+// uiResourceDeps{Client: l.Client, ...}) — and l.AgentSession's UID owns it
+// (mirrors loop.go's own "var sessionUID types.UID; if l.AgentSession != nil
+// { sessionUID = l.AgentSession.UID }" guard — empty when Loop.AgentSession is
+// unset, e.g. a kubectl-driven session or a unit test).
+//
+// ArtifactStore is deliberately left nil: Loop carries no write-capable
+// artifactstore.Store today. l.Artifacts (*artifacts.Service) manages
+// ArtifactRender CR headers/revisions over memory.Memory, and l.ArtifactReader
+// is read-only (modality.ArtifactReader) — neither is a screenshotArtifactPutter.
+// That means only the <=screenshotInlineMaxBytes inline Payload path is
+// reachable from this wiring; a screenshot over the cap makes
+// storeScreenshotArtifact return an error, which the caller
+// (captureBrowserScreenshotFn below) logs and treats as "ship no screenshot" —
+// same as any other capture/store failure, per the best-effort/fail-closed
+// contract. A single-viewport redacted PNG is well under 256 KiB in practice,
+// so this degrades gracefully rather than requiring a new store seam to be
+// threaded through Loop for this feature.
+func (l *Loop) screenshotStoreDeps() screenshotStoreDeps {
+	var sessionUID types.UID
+	if l.AgentSession != nil {
+		sessionUID = l.AgentSession.UID
+	}
+	return screenshotStoreDeps{
+		Client:     l.Client,
+		SessionUID: sessionUID,
+	}
+}
+
+// captureBrowserScreenshotFn builds the best-effort, fail-closed closure a
+// runnerHost's captureScreenshot field carries: capture a redacted screenshot
+// via cap, store it as an ArtifactRender through l, and return the
+// AttachmentRef to attach to a browser interaction payload — or nil, having
+// logged why, on ANY capture or store failure. A capture/store failure NEVER
+// surfaces as an error to the caller; the approval publish always proceeds.
+//
+// Shared by BOTH places a browser-sourced ask gets published (D-R3: one
+// browser session, one capturer, two publish call sites, because
+// request_human_step is itself a browser tool): executeToolContained wires it
+// onto the per-call host it builds for the web_host#act acting-approval
+// (plan_phase ask), and humanStepRequester.Confirm/RequestCode wire it onto
+// the host THEY bind for the login_human_step ask — two different *runnerHost
+// values (the latter is bound fresh per round-trip via hostBinder, see its
+// doc), but the SAME Loop and the SAME capturer, which is all this closure
+// needs.
+func (l *Loop) captureBrowserScreenshotFn(sess channelevents.SessionRef, cap browsertool.BrowserScreenshotCapturer) func(context.Context) *channelevents.AttachmentRef {
+	return func(ctx context.Context) *channelevents.AttachmentRef {
+		png, err := cap.CaptureRedacted(ctx)
+		if err != nil {
+			slog.Info("browser screenshot: capture failed; publishing text-only",
+				"session", sess.Namespace+"/"+sess.Name, "err", err.Error())
+			return nil
+		}
+		ref, err := storeScreenshotArtifact(ctx, l.screenshotStoreDeps(), sess, png)
+		if err != nil {
+			slog.Info("browser screenshot: store failed; publishing text-only",
+				"session", sess.Namespace+"/"+sess.Name, "err", err.Error())
+			return nil
+		}
+		return ref
+	}
+}

pkg/agent/runner/browser_screenshot_test.go

@@ -0,0 +1,174 @@
+package runner
+
+import (
+	"bytes"
+	"context"
+	"errors"
+	"io"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
+
+	spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1"
+	"github.com/authzed/openagentprimitives/pkg/channels/channelevents"
+	"github.com/authzed/openagentprimitives/pkg/platform/artifactstore"
+)
+
+// browserScreenshotTestScheme builds a scheme carrying the ArtifactRender CRD
+// type. package runner (this file) cannot reuse another file's internal-test
+// scheme helper across _test.go files of the same package that declare their
+// own (see apptoolsGrantTestScheme's doc comment) — each file in this
+// package's test suite mints its own, so this one does too.
+func browserScreenshotTestScheme(t *testing.T) *runtime.Scheme {
+	t.Helper()
+	s := runtime.NewScheme()
+	require.NoError(t, spiceboxv1alpha1.AddToScheme(s), "AddToScheme")
+	return s
+}
+
+// fakeScreenshotPutter is a minimal screenshotArtifactPutter recorder: it
+// never touches real storage, just remembers the keys/bytes it was asked to
+// Put and hands back a fixed Ref.
+type fakeScreenshotPutter struct {
+	ref   artifactstore.Ref
+	err   error
+	calls []struct {
+		key   string
+		bytes []byte
+	}
+}
+
+func (f *fakeScreenshotPutter) Put(_ context.Context, key string, r io.Reader) (artifactstore.Ref, error) {
+	b, err := io.ReadAll(r)
+	if err != nil {
+		return "", err
+	}
+	f.calls = append(f.calls, struct {
+		key   string
+		bytes []byte
+	}{key: key, bytes: b})
+	if f.err != nil {
+		return "", f.err
+	}
+	return f.ref, nil
+}
+
+func TestStoreScreenshotArtifact_InlineVsRef(t *testing.T) {
+	scheme := browserScreenshotTestScheme(t)
+	sess := channelevents.SessionRef{Namespace: "default", Name: "s1"}
+
+	t.Run("PNG under the 256 KiB cap is stored inline", func(t *testing.T) {
+		c := fake.NewClientBuilder().WithScheme(scheme).Build()
+		putter := &fakeScreenshotPutter{ref: "mem://should-not-be-used"}
+		deps := screenshotStoreDeps{
+			Client:        c,
+			ArtifactStore: putter,
+			SessionUID:    types.UID("sess-uid-1"),
+		}
+		small := bytes.Repeat([]byte{0xAB}, 1024) // < 256 KiB
+
+		ref, err := storeScreenshotArtifact(context.Background(), deps, sess, small)
+		require.NoError(t, err)
+		require.NotNil(t, ref)
+
+		assert.NotEmpty(t, ref.RenderName)
+		assert.NotEmpty(t, ref.ArtifactID)
+		assert.Equal(t, "image/png", ref.MIME)
+		assert.Equal(t, "page.png", ref.Filename)
+		assert.Equal(t, "current page (credential fields redacted)", ref.AltText)
+
+		var cr spiceboxv1alpha1.ArtifactRender
+		require.NoError(t, c.Get(context.Background(), client.ObjectKey{Namespace: sess.Namespace, Name: ref.RenderName}, &cr),
+			"the created ArtifactRender must be readable back by the returned RenderName")
+		assert.Equal(t, ref.RenderName, cr.Name)
+		assert.Equal(t, "image", cr.Spec.Kind)
+		assert.Equal(t, small, []byte(cr.Spec.Payload), "small PNG must be inline")
+		assert.Empty(t, cr.Spec.PayloadRef, "small PNG must not set PayloadRef")
+		assert.Equal(t, ref.ArtifactID, cr.Labels["artifact.agentprimitives.authzed.com/id"])
+		assert.Empty(t, putter.calls, "artifactstore.Put must not be called for an inline-sized PNG")
+	})
+
+	t.Run("PNG over the 256 KiB cap is stored by ref", func(t *testing.T) {
+		c := fake.NewClientBuilder().WithScheme(scheme).Build()
+		putter := &fakeScreenshotPutter{ref: "mem://default/s1/screenshot.png"}
+		deps := screenshotStoreDeps{
+			Client:        c,
+			ArtifactStore: putter,
+			SessionUID:    types.UID("sess-uid-1"),
+		}
+		big := bytes.Repeat([]byte{0xCD}, 300*1024) // > 256 KiB
+
+		ref, err := storeScreenshotArtifact(context.Background(), deps, sess, big)
+		require.NoError(t, err)
+		require.NotNil(t, ref)
+		assert.Equal(t, "image/png", ref.MIME)
+
+		var cr spiceboxv1alpha1.ArtifactRender
+		require.NoError(t, c.Get(context.Background(), client.ObjectKey{Namespace: sess.Namespace, Name: ref.RenderName}, &cr))
+		assert.Nil(t, cr.Spec.Payload, "big PNG must not be inlined")
+		assert.Equal(t, "mem://default/s1/screenshot.png", cr.Spec.PayloadRef, "big PNG must be stored by ref")
+
+		require.Len(t, putter.calls, 1, "artifactstore.Put must be called exactly once for an oversized PNG")
+		assert.Equal(t, big, putter.calls[0].bytes, "the exact PNG bytes must reach the artifact store")
+	})
+
+	t.Run("artifact store Put failure is returned, not swallowed", func(t *testing.T) {
+		c := fake.NewClientBuilder().WithScheme(scheme).Build()
+		putErr := errors.New("boom: store unavailable")
+		putter := &fakeScreenshotPutter{err: putErr}
+		deps := screenshotStoreDeps{
+			Client:        c,
+			ArtifactStore: putter,
+			SessionUID:    types.UID("sess-uid-1"),
+		}
+		big := bytes.Repeat([]byte{0xEF}, 300*1024)
+
+		ref, err := storeScreenshotArtifact(context.Background(), deps, sess, big)
+		require.Error(t, err)
+		assert.Nil(t, ref)
+		assert.ErrorIs(t, err, putErr)
+	})
+
+	t.Run("CR Create failure is returned, not swallowed", func(t *testing.T) {
+		createErr := errors.New("boom: create refused")
+		c := fake.NewClientBuilder().WithScheme(scheme).
+			WithInterceptorFuncs(interceptor.Funcs{
+				Create: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.CreateOption) error {
+					return createErr
+				},
+			}).
+			Build()
+		deps := screenshotStoreDeps{
+			Client:     c,
+			SessionUID: types.UID("sess-uid-1"),
+		}
+		small := bytes.Repeat([]byte{0x01}, 16)
+
+		ref, err := storeScreenshotArtifact(context.Background(), deps, sess, small)
+		require.Error(t, err)
+		assert.Nil(t, ref)
+	})
+
+	t.Run("deterministic name/id seams are honored", func(t *testing.T) {
+		c := fake.NewClientBuilder().WithScheme(scheme).Build()
+		deps := screenshotStoreDeps{
+			Client:        c,
+			ArtifactStore: &fakeScreenshotPutter{},
+			SessionUID:    types.UID("sess-uid-1"),
+			NewRenderName: func(session string) string { return "ar-" + session + "-fixed" },
+			NewArtifactID: func() string { return "artifact-fixed" },
+		}
+		small := bytes.Repeat([]byte{0x02}, 16)
+
+		ref, err := storeScreenshotArtifact(context.Background(), deps, sess, small)
+		require.NoError(t, err)
+		assert.Equal(t, "ar-s1-fixed", ref.RenderName)
+		assert.Equal(t, "artifact-fixed", ref.ArtifactID)
+	})
+}