From 3d6bbd424c239039a8acf420f446d7d166cf6271 Mon Sep 17 00:00:00 2001 From: SixVolts Date: Sun, 27 Sep 2026 14:06:34 -0500 Subject: [PATCH 1/2] fix(shards): honor temperature 0 and web_search, restrict ephemeral shards, tighten tokens and Slack output - The stored temperature is used as set, 0 included; create defaults to 0.7 only when it is absent. - A shard that allowlists web_search gets a per-turn search budget. - The page-writing tools are classified as write-capable, so ephemeral shards can't use them. - Shard tokens get a longer lookup prefix; older tokens keep working until re-minted; token checks are concurrency-bounded. - PATCH can clear a shard's session idle window and schemas. - Slack output escapes model text and keeps autolinks; long replies split at safe boundaries; proactive sends are split. - Shard API streaming errors are OpenAI-style error events. - setup-slack.sh writes its env file 0600 and builds a new config from config.example.toml. Tests cover each change. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01UwpkZbULrgj6JLeSra78AT --- .../internal/adapter/shardapi/shardapi.go | 23 +++-- .../adapter/shardapi/shardapi_test.go | 23 +++++ .../internal/adapter/slack/format.go | 54 +++++++++-- .../internal/adapter/slack/safety_test.go | 94 +++++++++++++++++++ .../internal/adapter/slack/sender.go | 27 ++++-- .../adapter/slack/sender_policy_test.go | 2 + .../internal/adapter/slack/slack.go | 61 ++++++++---- familiar-gateway/internal/admin/shards.go | 47 +++++++--- .../internal/admin/shards_role_test.go | 62 +++++++++++- .../internal/pipeline/pipeline_shard_test.go | 35 +++++++ .../internal/pipeline/shard_overrides.go | 29 +++++- familiar-gateway/internal/pipeline/shards.go | 20 ++-- familiar-gateway/internal/shards/pg.go | 40 ++++++-- .../internal/shards/store_test.go | 47 +++++++++- familiar-gateway/internal/shards/types.go | 48 ++++++---- familiar-gateway/scripts/setup-slack.sh | 64 ++++++------- tests/e2e/flows/shards.spec.ts | 5 +- 17 files changed, 541 insertions(+), 140 deletions(-) create mode 100644 familiar-gateway/internal/adapter/slack/safety_test.go diff --git a/familiar-gateway/internal/adapter/shardapi/shardapi.go b/familiar-gateway/internal/adapter/shardapi/shardapi.go index 308af67..feecc22 100644 --- a/familiar-gateway/internal/adapter/shardapi/shardapi.go +++ b/familiar-gateway/internal/adapter/shardapi/shardapi.go @@ -14,8 +14,11 @@ // - ignore model / tools / temperature / max_tokens / user in the // body — the shard is the capability envelope, the caller cannot // expand it; -// - never run the router, the Familiar tiered prompt store, the -// preamble path, or pre-execution tool orchestration; +// - never run the Familiar tiered prompt store, the preamble path, +// or pre-execution tool orchestration; a shard pinned to a model +// or tier skips the classifier too, while an unpinned one is +// classified like a trusted turn (its web_search is granted by its +// allowlist either way, not by the classifier); // - skip memory retrieval entirely (shards see the shard prompt + // prior session turns, nothing else); // - tag all downstream writes (pipeline commit, extracted facts, @@ -34,7 +37,6 @@ import ( "context" "encoding/json" "errors" - "fmt" "io" "log" "net/http" @@ -566,11 +568,18 @@ func (h *Handler) handleStreaming( // the final JSON; routing metadata is out of band (logs). _, info, err := h.pipe.HandleShardStream(ctx, sess, userMsg, overrides, onChunk, nil, nil) if err != nil { + // Same as the non-streaming path: the detail goes to the log, + // the caller gets a generic error, as an error event (the + // OpenAI shape) and no "stop" finish. The error text was sent as + // reply content (upstream URLs, model ids) and the stream then + // finished "stop", reading as a successful completion. log.Printf("[shards] invoke failed: shard=%s err=%v", shardID, err) - send(choice{ - Index: 0, - Delta: &invokeMessage{Content: fmt.Sprintf("\n\n[error: %v]", err)}, - }) + mu.Lock() + _, _ = io.WriteString(w, `data: {"error":{"message":"invocation failed","type":"server_error"}}`+"\n\n") + _, _ = io.WriteString(w, "data: [DONE]\n\n") + flusher.Flush() + mu.Unlock() + return } finish := "stop" diff --git a/familiar-gateway/internal/adapter/shardapi/shardapi_test.go b/familiar-gateway/internal/adapter/shardapi/shardapi_test.go index d276d21..cb7b9f2 100644 --- a/familiar-gateway/internal/adapter/shardapi/shardapi_test.go +++ b/familiar-gateway/internal/adapter/shardapi/shardapi_test.go @@ -688,3 +688,26 @@ func TestInvoke_PipelineErrorReturns500(t *testing.T) { // grows or shrinks; no-op otherwise. var _ = fmt.Sprintf var _ = io.Discard + +// A failed streamed invocation ends with an error event, not the +// internal error text as reply content followed by a "stop" finish +// (which reads as a successful completion and leaked upstream URLs). +func TestInvoke_StreamingErrorIsAnErrorEvent(t *testing.T) { + _, _, pipe, h := buildFixtures(t) + pipe.err = errors.New(`LLM completion: Post "http://127.0.0.1:8081/v1/chat/completions": connection refused`) + body := `{"messages":[{"role":"user","content":"hi"}],"stream":true}` + rr := doInvoke(t, h, testShard, testEmail, testToken, body) + out := rr.Body.String() + if strings.Contains(out, "127.0.0.1") || strings.Contains(out, "connection refused") { + t.Errorf("internal error text reached the caller: %q", out) + } + if !strings.Contains(out, `"error":{"message":"invocation failed"`) { + t.Errorf("no error event: %q", out) + } + if strings.Contains(out, `"finish_reason":"stop"`) { + t.Errorf("a failed stream finished as stop: %q", out) + } + if !strings.HasSuffix(strings.TrimSpace(out), "data: [DONE]") { + t.Errorf("stream not terminated: %q", out) + } +} diff --git a/familiar-gateway/internal/adapter/slack/format.go b/familiar-gateway/internal/adapter/slack/format.go index 0794214..72d086b 100644 --- a/familiar-gateway/internal/adapter/slack/format.go +++ b/familiar-gateway/internal/adapter/slack/format.go @@ -2,6 +2,7 @@ package slack import ( "regexp" + "strconv" "strings" ) @@ -14,9 +15,13 @@ import ( // - --- / *** / ___ rule → a unicode divider line // - "- item" / "+ item" → "• item" // - **bold** → *bold* -// - [text](url) → +// - [text](url) and autolinks → / // - tags → stripped -// - backtick `code` and ``` ```code``` fences pass through untouched +// - backtick `code` and ``` ```code``` fences pass through, escaped +// - &, < and > are escaped everywhere, so text can't form a Slack +// control sequence (, <@U…>): content a tool fetched +// could make the bot ping a whole channel. Only the links built +// here keep their <…>. // // Code spans and fenced blocks are carved out first so the bold/link // transformations don't accidentally rewrite content the user wanted @@ -35,7 +40,7 @@ func toMrkdwn(s string) string { b.Grow(len(s)) for _, seg := range segments { if seg.code { - b.WriteString(seg.text) + b.WriteString(slackEscape(seg.text)) continue } b.WriteString(transformPlain(seg.text)) @@ -43,6 +48,13 @@ func toMrkdwn(s string) string { return b.String() } +// slackEscape escapes the three characters Slack gives meaning to. +func slackEscape(s string) string { + return slackEscaper.Replace(s) +} + +var slackEscaper = strings.NewReplacer("&", "&", "<", "<", ">", ">") + type codeSeg struct { text string code bool // true = do not transform @@ -117,20 +129,44 @@ var linkPattern = regexp.MustCompile(`\[([^\]]+)\]\(([^)\s]+)\)`) // htmlTagPattern matches simple HTML tags for stripping. var htmlTagPattern = regexp.MustCompile(`<(/?[a-zA-Z][a-zA-Z0-9]*)\b[^>]*>`) +// autolinkPattern matches a CommonMark autolink, or +// . It has to go before the tag strip, which took +// "" for a tag named https and deleted the URL. +var autolinkPattern = regexp.MustCompile(`<((?:https?|mailto):[^\s<>]+)>`) + func transformPlain(s string) string { // Order matters: - // 1. strip HTML before link rewriting so generated - // isn't re-stripped as a tag; - // 2. headings/rules/bullets are line-anchored and run before the + // 1. links (markdown and autolinks) become placeholders first, so + // the tag strip and the escaping leave them alone; they're + // restored as Slack links at the end, their text escaped; + // 2. strip HTML, then escape what's left of &, < and >; + // 3. headings/rules/bullets are line-anchored and run before the // inline bold pass — heading text may itself contain **bold**, // which the bold pass then handles; - // 3. the hr pattern (---) must run before bullets so a "---" line + // 4. the hr pattern (---) must run before bullets so a "---" line // isn't seen as a "-" bullet. + var links []string + hold := func(link string) string { + links = append(links, link) + return "\x00" + strconv.Itoa(len(links)-1) + "\x00" + } + s = linkPattern.ReplaceAllStringFunc(s, func(m string) string { + sub := linkPattern.FindStringSubmatch(m) + return hold("<" + slackEscape(sub[2]) + "|" + slackEscape(sub[1]) + ">") + }) + s = autolinkPattern.ReplaceAllStringFunc(s, func(m string) string { + return hold("<" + slackEscape(autolinkPattern.FindStringSubmatch(m)[1]) + ">") + }) s = htmlTagPattern.ReplaceAllString(s, "") + s = slackEscape(s) s = hrPattern.ReplaceAllString(s, mrkdwnRule) s = headingPattern.ReplaceAllString(s, "*$1*") s = bulletPattern.ReplaceAllString(s, "$1• ") s = boldPattern.ReplaceAllString(s, "*$1*") - s = linkPattern.ReplaceAllString(s, "<$2|$1>") - return s + return placeholderPattern.ReplaceAllStringFunc(s, func(m string) string { + i, _ := strconv.Atoi(strings.Trim(m, "\x00")) + return links[i] + }) } + +var placeholderPattern = regexp.MustCompile("\x00[0-9]+\x00") diff --git a/familiar-gateway/internal/adapter/slack/safety_test.go b/familiar-gateway/internal/adapter/slack/safety_test.go new file mode 100644 index 0000000..1e5d828 --- /dev/null +++ b/familiar-gateway/internal/adapter/slack/safety_test.go @@ -0,0 +1,94 @@ +package slack + +import ( + "context" + "strings" + "testing" + "unicode/utf8" +) + +// Text can't form Slack control sequences: a fetched page telling the +// model to print "" made the bot ping the channel. Autolinks +// survive (the tag strip deleted them); the links the converter builds +// keep their markup. +func TestToMrkdwn_EscapesAndKeepsLinks(t *testing.T) { + cases := []struct{ in, want string }{ + {"See for details", "See for details"}, + {"Hey and <@U12345>", "Hey <!channel> and <@U12345>"}, + {"Tom & Jerry bold", "Tom & Jerry bold"}, + {"[the docs](https://x.test/a?b=1&c=2)", ""}, + {"code: ``", "code: `<!here>`"}, + {"**bold** ", "*bold* "}, + } + for _, c := range cases { + if got := toMrkdwn(c.in); got != c.want { + t.Errorf("toMrkdwn(%q) = %q, want %q", c.in, got, c.want) + } + } +} + +// Splitting never halves a character or a link, prefers spaces, and +// keeps a code fence rendering as code on both sides. +func TestSplitMessage_RunesLinksAndFences(t *testing.T) { + // No newline or space: the cut lands on a rune boundary. + s := strings.Repeat("a", 9) + "é" + strings.Repeat("b", 10) + for _, c := range splitMessage(s, 10) { + if !utf8.ValidString(c) { + t.Errorf("chunk %q is not valid UTF-8", c) + } + } + // A space before the limit wins over a raw cut. + chunks := splitMessage("hello world again", 13) + if chunks[0] != "hello world " { + t.Errorf("chunk[0] = %q, want a cut at the space", chunks[0]) + } + // Not inside a link (one that fits in a chunk). + chunks = splitMessage("aaaaaaaaaatail", 20) + for _, c := range chunks { + if strings.Count(c, "<") != strings.Count(c, ">") { + t.Errorf("a link was cut: %q", chunks) + } + } + // A fence cut in two is closed and reopened. + code := "```\n" + strings.Repeat("line of code\n", 20) + "```" + chunks = splitMessage(code, 100) + if len(chunks) < 2 { + t.Fatalf("expected several chunks, got %d", len(chunks)) + } + for _, c := range chunks { + if strings.Count(c, "```")%2 != 0 { + t.Errorf("chunk leaves a fence open: %q", c) + } + if len(c) > 100 { + t.Errorf("chunk of %d bytes, over the limit", len(c)) + } + } +} + +// A long proactive post goes as several posts: Slack truncates text +// past 40,000 characters, and nothing split it. +func TestSendProactive_SplitsLongPosts(t *testing.T) { + api := &fakeSlackAPI{membersPage: [][]string{{}}} + srv := api.server(t) + s, err := NewSender("xoxb-test", srv.URL+"/api/") + if err != nil { + t.Fatal(err) + } + long := strings.Repeat("A research digest paragraph.\n", 3000) // ~87KB + if err := s.SendProactive(context.Background(), "C_TEAM", long); err != nil { + t.Fatal(err) + } + if len(api.texts) < 3 { + t.Fatalf("posted %d messages, want the digest split", len(api.texts)) + } + total := 0 + for _, txt := range api.texts { + if len(txt) > proactiveMaxLen { + t.Errorf("a post of %d bytes", len(txt)) + } + total += len(txt) + } + if total < len(long)-10 { + t.Errorf("posted %d of %d bytes", total, len(long)) + } +} diff --git a/familiar-gateway/internal/adapter/slack/sender.go b/familiar-gateway/internal/adapter/slack/sender.go index f148c78..ae7ed3c 100644 --- a/familiar-gateway/internal/adapter/slack/sender.go +++ b/familiar-gateway/internal/adapter/slack/sender.go @@ -62,10 +62,11 @@ func (s *Sender) SendDM(ctx context.Context, slackUserID, text string) error { } // SendProactive posts `text` to `channelID`. This is the method the -// scheduled-actions Slack deliverer calls. Long messages are sent as -// a single post — the slack-go client splits at the API level if -// needed. We do not use the 4k truncation the adapter applies to -// interactive replies because proactive briefings are commonly long. +// scheduled-actions Slack deliverer calls. Proactive briefings are +// commonly long: past proactiveMaxLen they go as several posts (split +// like interactive replies, at a larger size). They were sent as one +// post on the belief that slack-go splits long messages; it doesn't, +// and Slack truncates text past 40,000 characters without an error. func (s *Sender) SendProactive(ctx context.Context, channelID, text string) error { if channelID == "" { return fmt.Errorf("slack sender: empty channel_id") @@ -78,16 +79,22 @@ func (s *Sender) SendProactive(ctx context.Context, channelID, text string) erro // / "**" / "---" shows literally. Convert before posting — the // interactive adapter already does this on its own path. text = toMrkdwn(text) - _, _, err := s.api.PostMessageContext(ctx, channelID, - slack.MsgOptionText(text, false), - slack.MsgOptionDisableLinkUnfurl(), - ) - if err != nil { - return fmt.Errorf("slack post to %s: %w", channelID, err) + for _, chunk := range splitMessage(text, proactiveMaxLen) { + _, _, err := s.api.PostMessageContext(ctx, channelID, + slack.MsgOptionText(chunk, false), + slack.MsgOptionDisableLinkUnfurl(), + ) + if err != nil { + return fmt.Errorf("slack post to %s: %w", channelID, err) + } } return nil } +// proactiveMaxLen is the largest proactive post, in bytes: under +// Slack's 40,000-character limit whatever the characters. +const proactiveMaxLen = 35000 + // IsUserOrDMID reports a Slack id that names a person (U…, W…) or a DM // (D…) rather than a channel. chat.postMessage accepts a user id as // the channel and DMs that user from the bot. diff --git a/familiar-gateway/internal/adapter/slack/sender_policy_test.go b/familiar-gateway/internal/adapter/slack/sender_policy_test.go index c1774a8..e9a452c 100644 --- a/familiar-gateway/internal/adapter/slack/sender_policy_test.go +++ b/familiar-gateway/internal/adapter/slack/sender_policy_test.go @@ -14,6 +14,7 @@ import ( type fakeSlackAPI struct { mu sync.Mutex posted []string + texts []string membersErr string // e.g. "missing_scope" membersPage [][]string } @@ -44,6 +45,7 @@ func (f *fakeSlackAPI) server(t *testing.T) *httptest.Server { _ = r.ParseForm() f.mu.Lock() f.posted = append(f.posted, r.Form.Get("channel")) + f.texts = append(f.texts, r.Form.Get("text")) f.mu.Unlock() _ = json.NewEncoder(w).Encode(map[string]any{"ok": true, "channel": r.Form.Get("channel"), "ts": "1.2"}) }) diff --git a/familiar-gateway/internal/adapter/slack/slack.go b/familiar-gateway/internal/adapter/slack/slack.go index 15aea22..85bbb17 100644 --- a/familiar-gateway/internal/adapter/slack/slack.go +++ b/familiar-gateway/internal/adapter/slack/slack.go @@ -10,6 +10,7 @@ import ( "net/http" "strings" "time" + "unicode/utf8" "github.com/familiar/gateway/internal/config" "github.com/familiar/gateway/internal/engine" @@ -623,31 +624,53 @@ func (a *SlackAdapter) channelAllowed(channel string) bool { return false } -// splitMessage breaks text into chunks of at most maxLen characters, -// preferring to split at newline boundaries. +// splitMessage breaks text into chunks of at most maxLen bytes, +// preferring to split at a newline, then at a space. A chunk never +// ends inside a UTF-8 character (a 4,000-byte cut could halve an "é" +// into two invalid bytes) or inside a link, and a code +// fence cut in two is closed at the end of one chunk and reopened at +// the start of the next, so both halves render as code. func splitMessage(text string, maxLen int) []string { - if len(text) <= maxLen { - return []string{text} - } - var chunks []string - for len(text) > 0 { - if len(text) <= maxLen { - chunks = append(chunks, text) - break + for len(text) > maxLen { + cut := splitPoint(text, maxLen) + chunk, rest := text[:cut], text[cut:] + if strings.Count(chunk, "```")%2 == 1 { + // Inside a fence: leave room to close it here. + cut = splitPoint(text, maxLen-len("\n```")) + chunk, rest = text[:cut]+"\n```", "```\n"+text[cut:] } + chunks = append(chunks, chunk) + text = rest + } + return append(chunks, text) +} - // Find the last newline within the limit. - cutPoint := maxLen - if idx := strings.LastIndex(text[:maxLen], "\n"); idx > 0 { - cutPoint = idx + 1 +// splitPoint is where to end a chunk of at most maxLen bytes of text. +func splitPoint(text string, maxLen int) int { + window := text[:maxLen] + cut := 0 + if i := strings.LastIndex(window, "\n"); i > 0 { + cut = i + 1 + } else if i := strings.LastIndexAny(window, " \t"); i > 0 { + cut = i + 1 + } else { + cut = maxLen + for cut > 0 && !utf8.RuneStart(text[cut]) { + cut-- } - - chunks = append(chunks, text[:cutPoint]) - text = text[cutPoint:] } - - return chunks + // Not inside a link. + if lt := strings.LastIndex(text[:cut], "<"); lt > 0 && !strings.Contains(text[lt:cut], ">") { + cut = lt + } + if cut <= 0 { + cut = maxLen + for cut > 1 && !utf8.RuneStart(text[cut]) { + cut-- + } + } + return cut } // truncate shortens a string for logging. diff --git a/familiar-gateway/internal/admin/shards.go b/familiar-gateway/internal/admin/shards.go index 7878959..e6540e0 100644 --- a/familiar-gateway/internal/admin/shards.go +++ b/familiar-gateway/internal/admin/shards.go @@ -244,8 +244,8 @@ type mintTokenResponse struct { // shardCreateBody matches the frontend's create form. Every field is // required except model/tier preference (one or neither) and the -// optional schema blobs. Temperature defaults to 0.7 when zero to -// match the DB default. +// optional schema blobs. Temperature defaults to 0.7 (the DB default) +// when absent; 0 is kept (deterministic sampling). type shardCreateBody struct { ID string `json:"id"` Name string `json:"name"` @@ -260,7 +260,7 @@ type shardCreateBody struct { InputSchema json.RawMessage `json:"input_schema"` OutputSchema json.RawMessage `json:"output_schema"` MaxTokens int `json:"max_tokens"` - Temperature float32 `json:"temperature"` + Temperature *float32 `json:"temperature"` // SHARD-AUTH-SPEC Phase 1 scoping fields. All optional on // create — defaults match the DB column defaults // (console_access=false, chat_enabled=true, api_enabled=true). @@ -521,14 +521,13 @@ func (h *Handler) createShard(w http.ResponseWriter, r *http.Request) { if body.MaxTokens == 0 { body.MaxTokens = 2048 } - if body.Temperature == 0 && !strings.Contains(string(body.InputSchema), "temperature_zero") { - // Match the DB default. Callers who genuinely want 0.0 get it - // by submitting the shard twice (create then PATCH) — the - // form UI in Phase 1 doesn't expose 0.0 explicitly. This is - // a known rough edge tracked with the "temperature_zero" - // sentinel above; it's acceptable for Phase 1 ephemeral - // extractors whose authors set e.g. 0.1. - body.Temperature = 0.7 + // Absent means the DB default; an explicit 0 is kept. It was + // coerced to 0.7 (unless the input schema carried a + // "temperature_zero" sentinel), and 0 set later by PATCH was then + // ignored at run time. + temperature := float32(0.7) + if body.Temperature != nil { + temperature = *body.Temperature } // Default the scoping toggles to the DB column defaults so an @@ -562,7 +561,7 @@ func (h *Handler) createShard(w http.ResponseWriter, r *http.Request) { InputSchema: body.InputSchema, OutputSchema: body.OutputSchema, MaxTokens: body.MaxTokens, - Temperature: body.Temperature, + Temperature: temperature, ConsoleAccess: consoleAccess, ConsolePanels: body.ConsolePanels, BookAccess: body.BookAccess, @@ -663,11 +662,14 @@ func (h *Handler) patchShard(w http.ResponseWriter, r *http.Request) { if body.TierPreference != nil { merged.TierPreference = *body.TierPreference } + // A schema of "" or {} clears it (JSON null reads as "unchanged" + // here, so it can't); without a way to clear, a removed schema + // stayed in the DB and the API for good. if body.InputSchema != nil { - merged.InputSchema = *body.InputSchema + merged.InputSchema = clearableSchema(*body.InputSchema) } if body.OutputSchema != nil { - merged.OutputSchema = *body.OutputSchema + merged.OutputSchema = clearableSchema(*body.OutputSchema) } if body.MaxTokens != nil { merged.MaxTokens = *body.MaxTokens @@ -690,8 +692,14 @@ func (h *Handler) patchShard(w http.ResponseWriter, r *http.Request) { if body.APIEnabled != nil { merged.APIEnabled = *body.APIEnabled } + // 0 clears the override (back to the gateway default); there was + // no way to, once set. if body.SessionMaxAge != nil { - merged.SessionMaxAge = body.SessionMaxAge + if *body.SessionMaxAge == 0 { + merged.SessionMaxAge = nil + } else { + merged.SessionMaxAge = body.SessionMaxAge + } } // Re-validate the allowlist against the updated persistence mode. @@ -1033,3 +1041,12 @@ func (h *Handler) canRevokeToken(ctx context.Context, au AuthUser, tokenID strin } return false, nil } + +// clearableSchema is a PATCHed schema blob: "" or {} means none. +func clearableSchema(raw json.RawMessage) json.RawMessage { + switch strings.TrimSpace(string(raw)) { + case `""`, `{}`, "null", "": + return nil + } + return raw +} diff --git a/familiar-gateway/internal/admin/shards_role_test.go b/familiar-gateway/internal/admin/shards_role_test.go index 9997eb1..0d86f9a 100644 --- a/familiar-gateway/internal/admin/shards_role_test.go +++ b/familiar-gateway/internal/admin/shards_role_test.go @@ -23,6 +23,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "sync" "testing" "time" @@ -142,8 +143,10 @@ func (f *fakeShardStore) RevokeToken(_ context.Context, id string) error { } // Unused by the role-gated handlers — panic on accidental use. -func (f *fakeShardStore) UpdateShard(context.Context, *shards.Shard) error { - panic("UpdateShard not used") +func (f *fakeShardStore) UpdateShard(_ context.Context, sh *shards.Shard) error { + cp := *sh + f.shards[sh.ID] = &cp + return nil } func (f *fakeShardStore) DisableShard(context.Context, string) error { panic("DisableShard not used") } func (f *fakeShardStore) EnableShard(context.Context, string) error { panic("EnableShard not used") } @@ -458,3 +461,58 @@ func TestRevokeToken_UserCannotRevokeOthers(t *testing.T) { // FAMILIAR-WORKSPACE-SPEC Phase 0 (the gateway is API-only now; // bookmarks land on the workspace's hostname). The corresponding // regression test lives at familiar-workspace/cmd/workspace/main_test.go. + +// Temperature 0 on create is kept (it was coerced to 0.7 unless the +// input schema carried a sentinel); absent means the 0.7 default. +func TestCreateShard_TemperatureZeroKept(t *testing.T) { + store := newFakeShardStore() + h := makeShardsHandler(t, store) + create := func(id, extra string) *shards.Shard { + body := `{"id":"` + id + `","name":"` + id + `","persistence":"ephemeral","visibility":"isolated", + "scope_tag":"shard:` + id + `","system_prompt":"extract","tool_allowlist":[],"max_tokens":256` + extra + `}` + req := httptest.NewRequest("POST", "/console/api/shards", strings.NewReader(body)).WithContext( + ctxWithAuth(context.Background(), alisonUser())) + req = req.WithContext(context.WithValue(req.Context(), ContextKeyUserID, "alison")) + rec := httptest.NewRecorder() + h.createShard(rec, req) + if rec.Code != http.StatusCreated { + t.Fatalf("create %s = %d (%s)", id, rec.Code, rec.Body.String()) + } + return store.shards[id] + } + if got := create("deterministic", `,"temperature":0`); got.Temperature != 0 { + t.Errorf("temperature 0 stored as %v", got.Temperature) + } + if got := create("default-temp", ``); got.Temperature != 0.7 { + t.Errorf("absent temperature stored as %v, want 0.7", got.Temperature) + } +} + +// A session TTL override and the schemas can be cleared: nil read as +// "unchanged", so once set they stayed for good. +func TestPatchShard_ClearsTTLAndSchemas(t *testing.T) { + store := newFakeShardStore() + h := makeShardsHandler(t, store) + ttl := 1800 + store.shards["kiosk"] = &shards.Shard{ + ID: "kiosk", OwnerID: "alison", Name: "Kiosk", SystemPrompt: "p", + Persistence: shards.PersistencePersistent, Visibility: shards.VisibilityIsolated, + ScopeTag: "shard:kiosk", MaxTokens: 256, Temperature: 0.7, + SessionMaxAge: &ttl, InputSchema: []byte(`{"type":"object"}`), OutputSchema: []byte(`{"type":"string"}`), + ChatEnabled: true, APIEnabled: true, + } + req := httptest.NewRequest("PATCH", "/console/api/shards/kiosk", + strings.NewReader(`{"session_max_age":0,"input_schema":"","output_schema":{}}`)).WithContext( + ctxWithAuth(context.Background(), alisonUser())) + req = req.WithContext(context.WithValue(req.Context(), ContextKeyUserID, "alison")) + req.SetPathValue("id", "kiosk") + rec := httptest.NewRecorder() + h.patchShard(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("patch = %d (%s)", rec.Code, rec.Body.String()) + } + got := store.shards["kiosk"] + if got.SessionMaxAge != nil || len(got.InputSchema) != 0 || len(got.OutputSchema) != 0 { + t.Errorf("after clearing: ttl=%v input=%s output=%s", got.SessionMaxAge, got.InputSchema, got.OutputSchema) + } +} diff --git a/familiar-gateway/internal/pipeline/pipeline_shard_test.go b/familiar-gateway/internal/pipeline/pipeline_shard_test.go index 163fd7a..89e75f9 100644 --- a/familiar-gateway/internal/pipeline/pipeline_shard_test.go +++ b/familiar-gateway/internal/pipeline/pipeline_shard_test.go @@ -48,6 +48,41 @@ func TestOverridesForShard_CopiesBookAccess(t *testing.T) { } } +// The stored shard settings reach the envelope as the owner set them: +// temperature 0 is deterministic sampling (it became the provider's +// default), an allowlisted web_search is granted (it was refused on +// every turn of a pinned shard), and an ephemeral shard runs no tool +// that writes (a wiki page could be rewritten by a prompt injection). +func TestOverridesForShard_TemperatureSearchAndWrites(t *testing.T) { + sh := &shards.Shard{ + ID: "lookup", + Persistence: shards.PersistenceEphemeral, + Visibility: shards.VisibilityIsolated, + ToolAllowlist: []string{"web_search", "read_page", "update_page", "save_fact"}, + TierPreference: "tier3", + Temperature: 0, + } + ov := OverridesForShard(sh) + if ov.Temperature == nil || *ov.Temperature != 0 { + t.Errorf("Temperature = %v, want a pointer to 0", ov.Temperature) + } + if ov.SearchBudget <= 0 { + t.Errorf("SearchBudget = %d, want a grant for the allowlisted web_search", ov.SearchBudget) + } + if strings.Join(ov.ToolAllowlist, ",") != "web_search,read_page" { + t.Errorf("ephemeral allowlist = %v, want the write tools dropped", ov.ToolAllowlist) + } + sh.Persistence = shards.PersistencePersistent + sh.ToolAllowlist = []string{"read_page", "update_page"} + ov = OverridesForShard(sh) + if strings.Join(ov.ToolAllowlist, ",") != "read_page,update_page" { + t.Errorf("persistent allowlist = %v, want it unchanged", ov.ToolAllowlist) + } + if ov.SearchBudget != 0 { + t.Errorf("SearchBudget = %d without web_search allowlisted", ov.SearchBudget) + } +} + // assertMessagesContainSystem returns the content of the system message // in the recorded request, or the empty string if none is present. func recordedSystemMsg(msgs []testutil.RecordedMessage) string { diff --git a/familiar-gateway/internal/pipeline/shard_overrides.go b/familiar-gateway/internal/pipeline/shard_overrides.go index dc290c0..d3a93b5 100644 --- a/familiar-gateway/internal/pipeline/shard_overrides.go +++ b/familiar-gateway/internal/pipeline/shard_overrides.go @@ -23,17 +23,40 @@ func OverridesForShard(sh *shards.Shard) *ShardOverrides { TierHint: sh.TierPreference, MaxTokens: sh.MaxTokens, } - if sh.Temperature != 0 { - t := sh.Temperature - ov.Temperature = &t + // The stored temperature is what the owner set, 0 included: 0 means + // deterministic sampling, and treating it as "unset" ran the + // provider's default (0.7-0.8) instead. + t := sh.Temperature + ov.Temperature = &t + // An allowlisted web_search is a grant: shard turns are stamped + // SearchNone (no classifier) or classified like trusted turns, and + // either way web_search was refused or left to the classifier. + for _, tool := range sh.ToolAllowlist { + if tool == "web_search" { + ov.SearchBudget = shardWebSearchBudget + } } if sh.Persistence == shards.PersistenceEphemeral { ov.SkipSessionHydration = true ov.SkipCommit = true + // Ephemeral means no side effects: no tool that writes. Saving + // refuses them; a shard saved before a tool was known to write + // still lists it, so it's dropped here too. + kept := make([]string, 0, len(ov.ToolAllowlist)) + for _, tool := range ov.ToolAllowlist { + if !shards.IsWriteCapable(tool) { + kept = append(kept, tool) + } + } + ov.ToolAllowlist = kept } return ov } +// shardWebSearchBudget is how many web_search calls a turn of a shard +// that allowlists web_search may make. +const shardWebSearchBudget = 4 + // EphemeralOverrides is the scheduled-actions "ephemeral" envelope: // nothing but the prompt. No system prompt, no memory retrieval, no // tools (empty non-nil allowlist = nothing advertised, everything diff --git a/familiar-gateway/internal/pipeline/shards.go b/familiar-gateway/internal/pipeline/shards.go index dca6d98..26cf5b7 100644 --- a/familiar-gateway/internal/pipeline/shards.go +++ b/familiar-gateway/internal/pipeline/shards.go @@ -36,10 +36,11 @@ import ( // zero MaxTokens falls through to the pipeline default, Temperature // is a pointer because 0.0 is a valid explicit value. // - SearchBudget grants the envelope web_search calls; zero keeps -// the shard-path default (search disabled — shard turns bypass the -// classifier and are stamped SearchNone). Stored shards don't set -// it; it exists for purpose-built envelopes (research workers, -// RESEARCH-SKILL-SPEC §6.1). +// the shard-path default (a pinned shard is stamped SearchNone, an +// unpinned one gets its classifier's verdict). Stored shards that +// allowlist web_search get one (OverridesForShard), as do +// purpose-built envelopes (research workers, RESEARCH-SKILL-SPEC +// §6.1). type ShardOverrides struct { ShardID string @@ -68,12 +69,11 @@ type ShardOverrides struct { Temperature *float32 // SearchBudget grants this envelope N web_search calls for the - // turn. Shard turns bypass the classifier and are stamped - // SearchNone (shardClassifierOutput), which hard-disables - // web_search at dispatch; a positive SearchBudget lifts that for - // purpose-built envelopes (research workers). Zero keeps today's - // behavior — existing shards and ephemeral action envelopes never - // search. + // turn. A shard pinned to a model or tier bypasses the classifier + // and is stamped SearchNone (shardClassifierOutput), which + // hard-disables web_search at dispatch; a positive SearchBudget + // lifts that. Set for shards that allowlist web_search and for + // purpose-built envelopes (research workers). SearchBudget int } diff --git a/familiar-gateway/internal/shards/pg.go b/familiar-gateway/internal/shards/pg.go index d9160a8..37d93fb 100644 --- a/familiar-gateway/internal/shards/pg.go +++ b/familiar-gateway/internal/shards/pg.go @@ -30,9 +30,22 @@ import ( const TokenPlaintextPrefix = "shard_" // tokenPrefixLen is how many leading characters of the plaintext we -// keep unhashed for UI display and for indexing ValidateToken lookups. -// Matches FAMILIAR-SHARDS-PHASE1-SPEC §"shard_tokens table". -const tokenPrefixLen = 8 +// keep unhashed for UI display and for indexing ValidateToken lookups: +// the literal "shard_" plus 10 random characters (60 bits). It was 8, +// leaving 2 random characters: 4096 buckets, so a probe found the +// occupied ones cheaply and each guess into one cost a bcrypt compare +// (a CPU flood from an unauthenticated endpoint), and two tokens of one +// shard often showed the same prefix. +const tokenPrefixLen = len(TokenPlaintextPrefix) + 10 + +// legacyTokenPrefixLen is the prefix length of tokens minted before +// that; they validate until they are revoked and re-minted. +const legacyTokenPrefixLen = 8 + +// bcryptSlots caps concurrent token hash comparisons, so a flood of +// guesses into a legacy prefix bucket can't take every CPU the gateway +// (chat, memory) needs. +var bcryptSlots = make(chan struct{}, 4) // bcryptCost trades hash time for brute-force resistance. Default 10 // is ~60ms on commodity hardware, fine for the low-QPS invoke path. @@ -512,13 +525,17 @@ func (s *PGStore) ListTokens(ctx context.Context, shardID string) ([]*Token, err // caller cannot use the error to distinguish "we have a row with this // prefix" from "we don't" — a small timing hardening. func (s *PGStore) ValidateToken(ctx context.Context, plaintext string) (*Token, error) { - if len(plaintext) < tokenPrefixLen { + if len(plaintext) < legacyTokenPrefixLen { return nil, ErrTokenNotFound } - prefix := plaintext[:tokenPrefixLen] + legacy := plaintext[:legacyTokenPrefixLen] + prefix := legacy + if len(plaintext) >= tokenPrefixLen { + prefix = plaintext[:tokenPrefixLen] + } rows, err := s.db.QueryContext(ctx, - selectTokenColumns+`, token_hash FROM shard_tokens WHERE token_prefix = $1`, - prefix, + selectTokenColumns+`, token_hash FROM shard_tokens WHERE token_prefix = $1 OR token_prefix = $2`, + prefix, legacy, ) if err != nil { return nil, fmt.Errorf("shards: validate token: %w", err) @@ -530,7 +547,14 @@ func (s *PGStore) ValidateToken(ctx context.Context, plaintext string) (*Token, if err != nil { return nil, fmt.Errorf("shards: validate token: scan: %w", err) } - if bcrypt.CompareHashAndPassword([]byte(hash), []byte(plaintext)) != nil { + select { + case bcryptSlots <- struct{}{}: + case <-ctx.Done(): + return nil, ctx.Err() + } + mismatch := bcrypt.CompareHashAndPassword([]byte(hash), []byte(plaintext)) != nil + <-bcryptSlots + if mismatch { continue } if t.RevokedAt != nil { diff --git a/familiar-gateway/internal/shards/store_test.go b/familiar-gateway/internal/shards/store_test.go index 760ed70..ab231d1 100644 --- a/familiar-gateway/internal/shards/store_test.go +++ b/familiar-gateway/internal/shards/store_test.go @@ -394,7 +394,7 @@ func TestToken_ValidateUnknownReturnsNotFound(t *testing.T) { func TestToken_ValidateShortReturnsNotFound(t *testing.T) { s := setupShardStore(t) - // Fewer chars than tokenPrefixLen — should not hit the DB. + // Shorter than any prefix, old or new — should not hit the DB. _, err := s.ValidateToken(context.Background(), "abc") if !errors.Is(err, ErrTokenNotFound) { t.Errorf("err = %v, want ErrTokenNotFound", err) @@ -650,3 +650,48 @@ func TestValidateScopeTag_ReservesBookPrefix(t *testing.T) { t.Errorf("ordinary tag rejected: %v", err) } } + +// Wiki page writers are write tools too: an "ephemeral" shard (no side +// effects, the UI said) could rewrite a page. +func TestValidateAllowlist_EphemeralRejectsPageWriters(t *testing.T) { + for _, write := range []string{"create_page", "update_page", "append_to_page", "patch_page", "pin_page"} { + if err := ValidateAllowlist([]string{"read_page", write}, PersistenceEphemeral, nil); !errors.Is(err, ErrWriteToolOnEph) { + t.Errorf("ephemeral+%s: err = %v, want ErrWriteToolOnEph", write, err) + } + } + if err := ValidateAllowlist([]string{"read_page", "search_pages", "list_books"}, PersistenceEphemeral, nil); err != nil { + t.Errorf("read-only wiki tools refused: %v", err) + } +} + +// A token's lookup prefix carries 10 random characters (it had 2: 4096 +// buckets a probe could find, each guess into one costing a bcrypt +// compare). Tokens minted with the old 8-character prefix still work. +func TestToken_LongPrefixAndLegacyTokens(t *testing.T) { + s := setupShardStore(t) + ctx := context.Background() + if err := s.CreateShard(ctx, validShard("prefixes")); err != nil { + t.Fatal(err) + } + p1, t1, err := s.CreateToken(ctx, "owner", "prefixes", "one") + if err != nil { + t.Fatal(err) + } + _, t2, err := s.CreateToken(ctx, "owner", "prefixes", "two") + if err != nil { + t.Fatal(err) + } + if len(t1.TokenPrefix) != len(TokenPlaintextPrefix)+10 { + t.Errorf("prefix %q has %d characters, want %d", t1.TokenPrefix, len(t1.TokenPrefix), len(TokenPlaintextPrefix)+10) + } + if t1.TokenPrefix == t2.TokenPrefix { + t.Error("two tokens share a prefix") + } + // A token minted before the change: its row holds 8 characters. + if _, err := s.db.ExecContext(ctx, `UPDATE shard_tokens SET token_prefix = $1 WHERE id = $2`, p1[:8], t1.ID); err != nil { + t.Fatal(err) + } + if got, err := s.ValidateToken(ctx, p1); err != nil || got.ID != t1.ID { + t.Errorf("legacy-prefix token: %v, %v", got, err) + } +} diff --git a/familiar-gateway/internal/shards/types.go b/familiar-gateway/internal/shards/types.go index a4ace60..d122983 100644 --- a/familiar-gateway/internal/shards/types.go +++ b/familiar-gateway/internal/shards/types.go @@ -10,8 +10,9 @@ // allowlist, and a persistence/visibility mode. // // - Tokens are 1:1 with shards. A token's plaintext is returned exactly -// once from CreateToken; the store keeps a bcrypt hash and an 8-char -// prefix for UI disambiguation only. ValidateToken re-derives the +// once from CreateToken; the store keeps a bcrypt hash and a prefix +// ("shard_" plus 10 characters; 8 in all for tokens minted before) +// for lookup and UI disambiguation. ValidateToken re-derives the // prefix, looks up candidate rows, and bcrypt-compares. // // - The store enforces a small set of integrity rules the DB alone @@ -193,28 +194,37 @@ func ValidateScopeTag(tag string) error { // knowledge rows ("book:"). Shards may not use it. const ReservedBookScopePrefix = "book:" -// writeCapableMemoryTools are the tool names that mutate memory. An -// ephemeral shard's allowlist is rejected if it contains any of these. -// Names are bare (matching the globally-unique convention the skill -// registry enforces — see internal/skills.Registry.Register), not -// dotted . form; the admin UI may display them dotted -// for readability but storage and dispatch use bare names. +// writeCapableTools are the tool names that write: memory, and the wiki +// and notes pages. An ephemeral shard's allowlist is rejected if it +// contains any of these, and they are dropped from one at run time +// (pipeline.OverridesForShard). Only the memory tools were listed, so +// an "ephemeral" shard (the UI promising no side effects) could still +// rewrite a wiki page through a prompt injection. Names are bare +// (matching the globally-unique convention the skill registry +// enforces — see internal/skills.Registry.Register), not dotted +// . form. // // Kept in this package (rather than pulled from the skill registry) so -// validation is a pure data check with no import cycle. If the memory -// skill ever adds a new write-capable tool, update this list in -// lockstep — ephemeral-shard safety depends on it being complete. -var writeCapableMemoryTools = map[string]bool{ - "save_fact": true, // persists a fact to long-term memory - "remember": true, // explicit user-requested remember - "forget_fact": true, // deletes a memory - "correct_fact": true, // updates an existing memory +// validation is a pure data check with no import cycle. A new tool +// that writes must be added here; ephemeral-shard safety depends on it +// being complete. Tools composed at run time (skill packages) aren't +// covered. +var writeCapableTools = map[string]bool{ + "save_fact": true, // persists a fact to long-term memory + "remember": true, // explicit user-requested remember + "forget_fact": true, // deletes a memory + "correct_fact": true, // updates an existing memory + "create_page": true, + "update_page": true, + "append_to_page": true, + "patch_page": true, + "pin_page": true, } -// IsWriteCapable reports whether a tool name is known to mutate memory. +// IsWriteCapable reports whether a tool name is known to write. // Exported so the admin UI can surface the same answer the backend // uses (checkbox disabling for ephemeral shards). -func IsWriteCapable(tool string) bool { return writeCapableMemoryTools[tool] } +func IsWriteCapable(tool string) bool { return writeCapableTools[tool] } // ValidateAllowlist enforces: // - no duplicates (gives a clearer error than a UNIQUE constraint ever could) @@ -233,7 +243,7 @@ func ValidateAllowlist(tools []string, persistence PersistenceMode, known map[st return fmt.Errorf("shards: duplicate tool in allowlist: %q", t) } seen[t] = struct{}{} - if persistence == PersistenceEphemeral && writeCapableMemoryTools[t] { + if persistence == PersistenceEphemeral && writeCapableTools[t] { return fmt.Errorf("%w: %q", ErrWriteToolOnEph, t) } if known != nil && !known[t] { diff --git a/familiar-gateway/scripts/setup-slack.sh b/familiar-gateway/scripts/setup-slack.sh index c6423a4..dfe2195 100755 --- a/familiar-gateway/scripts/setup-slack.sh +++ b/familiar-gateway/scripts/setup-slack.sh @@ -242,49 +242,43 @@ if [[ -f "$CONFIG_FILE" ]]; then ok "Appended Slack config to ${CONFIG_FILE}" fi else - # Create a new config with defaults + slack section. - cat > "$CONFIG_FILE" < "$tmpfile" + echo "" >> "$tmpfile" + echo "$SLACK_TOML" >> "$tmpfile" + mv "$tmpfile" "$CONFIG_FILE" + ok "Created ${CONFIG_FILE} from config.example.toml — review it (models, embedder) before starting the gateway" + else + echo "$SLACK_TOML" > "$CONFIG_FILE" + warn "config.example.toml not found: ${CONFIG_FILE} holds only the Slack section; merge it into a full config" + fi fi -# Write tokens to env file. +# Write tokens to env file. It is created unreadable to others from the +# start (umask 077) and moved into place: writing it and then running +# chmod left the tokens readable in between (and an existing file kept +# its old mode until then). ENV_FILE="${CONFIG_DIR}/slack.env" -cat > "$ENV_FILE" < "$ENV_TMP" < Date: Sun, 27 Sep 2026 14:07:03 -0500 Subject: [PATCH 2/2] fix(shards-panel): edits keep book confinement and the prompt, Cancel mints nothing - The book picker lists the owner's books (archived ones marked), keeps books it can't name, and an edit sends book_access only once the picker is touched. - A console-only shard keeps its system prompt. - Cancel on the token label mints nothing. - Shard rows open from the keyboard. - A blank temperature is omitted; emptying the idle window or a schema clears it. Tests: shards-panel.spec.ts. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01UwpkZbULrgj6JLeSra78AT --- familiar-workspace/static/index.html | 2 +- familiar-workspace/static/panels/shards.js | 100 +++++--- tests/e2e/flows/shards-panel.spec.ts | 266 +++++++++++++++++++++ 3 files changed, 338 insertions(+), 30 deletions(-) create mode 100644 tests/e2e/flows/shards-panel.spec.ts diff --git a/familiar-workspace/static/index.html b/familiar-workspace/static/index.html index e6d8e5d..6d73234 100644 --- a/familiar-workspace/static/index.html +++ b/familiar-workspace/static/index.html @@ -1191,7 +1191,7 @@

Sign-in & access

-
Tick books the shard can read/write. Leave all unchecked to inherit your memberships.
+
Tick books the shard can read/write. Leave all unchecked to inherit all of the owner's memberships.
diff --git a/familiar-workspace/static/panels/shards.js b/familiar-workspace/static/panels/shards.js index 246a8bb..db768a5 100644 --- a/familiar-workspace/static/panels/shards.js +++ b/familiar-workspace/static/panels/shards.js @@ -122,13 +122,19 @@ // refreshShardBookCatalog pulls /console/api/books fresh. // Called from openShardCreate / openShardDetail so a book // created earlier in the same session shows up in the - // checklist without re-initing the whole panel. - async function refreshShardBookCatalog() { + // checklist without re-initing the whole panel. It lists the + // shard OWNER's books, archived ones included: a shard can only + // reach its owner's memberships, and an admin editing someone + // else's shard saw their own books instead (user_id is ignored + // for non-admins, who only edit their own shards). + async function refreshShardBookCatalog(ownerID) { + let url = "/console/api/books?archived=true"; + if (ownerID) url += "&user_id=" + encodeURIComponent(ownerID); try { - const books = await apiJSON("/console/api/books"); + const books = await apiJSON(url); shardsState.bookCatalog = (books && books.items) || []; } catch (e) { - shardsState.bookCatalog = shardsState.bookCatalog || []; + shardsState.bookCatalog = []; } } @@ -136,31 +142,47 @@ // the cached book catalog and pre-checks the IDs in `selected`. // Stores book IDs as the checkbox value so the form payload // matches what the backend stores in shards.book_access (UUIDs, - // not slugs or display names). + // not slugs or display names). A selected ID missing from the + // catalog (a personal book, or one the catalog fetch couldn't + // list) is shown checked as unavailable rather than dropped, and + // the form only sends book_access once the picker is touched + // (host.dataset.dirty): an unticked list means ALL the owner's + // books, so saving an unrelated edit silently widened the shard. function renderShardBookAccess(selected) { const host = document.getElementById("shard-book-access"); if (!host) return; host.innerHTML = ""; + host.dataset.dirty = "0"; + if (!host.dataset.wired) { + host.dataset.wired = "1"; + host.addEventListener("change", () => { host.dataset.dirty = "1"; }); + } const set = new Set(selected || []); const books = (shardsState && shardsState.bookCatalog) || []; - if (!books.length) { + const known = new Set(books.map((b) => b.id)); + const missing = Array.from(set).filter((id) => !known.has(id)); + if (!books.length && !missing.length) { const empty = document.createElement("div"); empty.className = "micro"; empty.textContent = "(no books yet — create one in the Wiki panel first)"; host.appendChild(empty); return; } - for (const b of books) { + const option = (id, text) => { const label = document.createElement("label"); label.className = "shard-checklist-option"; const cb = document.createElement("input"); cb.type = "checkbox"; - cb.value = b.id; - cb.checked = set.has(b.id); + cb.value = id; + cb.checked = set.has(id); label.appendChild(cb); - label.appendChild(document.createTextNode(" " + (b.name || b.slug || b.id))); + label.appendChild(document.createTextNode(" " + text)); host.appendChild(label); + }; + for (const b of books) { + option(b.id, (b.name || b.slug || b.id) + (b.archived_at ? " (archived)" : "")); } + for (const id of missing) option(id, "(unavailable) " + id); } // refreshSkillpackCatalog pulls the biddable-skills catalog: @@ -292,7 +314,17 @@ if (cls) td.className = cls; tr.appendChild(td); }; - addTd(s.name, "col-name"); + // The name is a button so the row opens from the keyboard; + // its click bubbles to the row's handler. + const nameTd = document.createElement("td"); + nameTd.className = "col-name"; + const open = document.createElement("button"); + open.type = "button"; + open.className = "row-open"; + open.textContent = s.name || s.id; + open.setAttribute("aria-label", "Open shard: " + (s.name || s.id)); + nameTd.appendChild(open); + tr.appendChild(nameTd); addTd(s.id, "col-id"); addTd((s.persistence || "").toUpperCase(), "col-status"); addTd((s.visibility || "").toUpperCase(), "col-status"); @@ -366,7 +398,7 @@ } async function openShardCreate() { - await refreshShardBookCatalog(); + await refreshShardBookCatalog(""); await refreshSkillpackCatalog(); shardsState.mode = "create"; shardsState.currentShard = null; @@ -412,10 +444,10 @@ } async function openShardDetail(id) { - await refreshShardBookCatalog(); await refreshSkillpackCatalog(); try { const s = await apiJSON("/console/api/shards/" + encodeURIComponent(id)); + await refreshShardBookCatalog(s.owner_id || ""); shardsState.mode = "edit"; shardsState.currentShard = s; document.getElementById("shard-detail-title").textContent = s.name || s.id; @@ -489,8 +521,9 @@ // the chat / api kill-switches. Both off ⇒ console-only shard // (a kiosk that never runs inference); skip the prompt // requirement and grey the field out so the form stays - // submittable. Either on ⇒ we'll route real prompts through - // here, so it's required and editable. + // submittable, keeping its text (it used to be cleared, and the + // save then erased the prompt for good). Either on ⇒ we'll route + // real prompts through here, so it's required and editable. function updateShardPromptState() { const chatEl = document.getElementById("shard-chat-enabled"); const apiEl = document.getElementById("shard-api-enabled"); @@ -501,7 +534,6 @@ promptEl.required = false; promptEl.disabled = true; promptEl.placeholder = "zombie shard — console only"; - promptEl.value = ""; } else { promptEl.required = true; promptEl.disabled = false; @@ -526,18 +558,19 @@ return; } - // SHARD-AUTH-SPEC scoping fields. Empty arrays explicitly - // mean "no panels / no books"; absence means "inherit - // everything", which the form represents as no checkboxes - // ticked + a blank book-access input. Server-side, the - // patch shape uses pointer fields so we send arrays only - // when the user has actually picked a subset. + // SHARD-AUTH-SPEC scoping fields. An empty list means + // "inherit everything" (every panel / all the owner's books). const consolePanels = Array.from( document.querySelectorAll('#shard-console-panels input:checked') ).map(cb => cb.value); const bookAccess = Array.from( document.querySelectorAll('#shard-book-access input:checked') ).map(cb => cb.value); + const editing = shardsState.mode !== "create"; + const bookHost = document.getElementById("shard-book-access"); + const sendBooks = !editing || (bookHost && bookHost.dataset.dirty === "1"); + const tempRaw = document.getElementById("shard-temperature").value.trim(); + const temperature = tempRaw === "" ? NaN : parseFloat(tempRaw); const sessionMaxRaw = document.getElementById("shard-session-max-age").value.trim(); const sessionMaxAge = sessionMaxRaw ? parseInt(sessionMaxRaw, 10) : null; @@ -553,30 +586,38 @@ model_preference: model, tier_preference: tier, max_tokens: parseInt(document.getElementById("shard-max-tokens").value, 10) || 2048, - temperature: parseFloat(document.getElementById("shard-temperature").value) || 0, console_access: document.getElementById("shard-console-access").checked, chat_enabled: document.getElementById("shard-chat-enabled").checked, api_enabled: document.getElementById("shard-api-enabled").checked, - // Always send the arrays — empty means "inherit", which - // the backend treats as nil (matches len()==0 path in - // loadShardPermissions). + // Empty means "inherit", which the backend treats as nil + // (matches len()==0 path in loadShardPermissions). console_panels: consolePanels, - book_access: bookAccess, }; + if (sendBooks) body.book_access = bookAccess; + // Blank temperature: the default on create, unchanged on edit + // (it was sent as 0, which is now honored as greedy sampling). + if (!isNaN(temperature)) body.temperature = temperature; if (sessionMaxAge && sessionMaxAge > 0) { body.session_max_age = sessionMaxAge; + } else if (editing) { + body.session_max_age = 0; // cleared: back to the gateway default } - // Parse optional schema blobs; empty string stays empty. + // Parse optional schema blobs. Blank omits the schema on + // create and clears it on edit ("" is the server's clear). const inSchema = document.getElementById("shard-input-schema").value.trim(); if (inSchema) { try { body.input_schema = JSON.parse(inSchema); } catch (err) { setError("shard-form-error", new Error("input_schema: invalid JSON")); return; } + } else if (editing) { + body.input_schema = ""; } const outSchema = document.getElementById("shard-output-schema").value.trim(); if (outSchema) { try { body.output_schema = JSON.parse(outSchema); } catch (err) { setError("shard-form-error", new Error("output_schema: invalid JSON")); return; } + } else if (editing) { + body.output_schema = ""; } // Imported-skill bindings ride along with the form but live in @@ -729,7 +770,8 @@ async function mintTokenForCurrent() { if (!shardsState.currentShard) return; const id = shardsState.currentShard.id; - const label = prompt("Token label (e.g. 'cannonball-prod'):", "") || ""; + const label = prompt("Token label (e.g. 'cannonball-prod'):", ""); + if (label === null) return; // Cancel mints nothing try { const data = await apiJSON("/console/api/shards/" + encodeURIComponent(id) + "/tokens", { method: "POST", diff --git a/tests/e2e/flows/shards-panel.spec.ts b/tests/e2e/flows/shards-panel.spec.ts new file mode 100644 index 0000000..990ed10 --- /dev/null +++ b/tests/e2e/flows/shards-panel.spec.ts @@ -0,0 +1,266 @@ +// shards-panel.spec.ts — the shard edit form keeps what it doesn't +// show: book confinement, the system prompt of a console-only shard, +// the temperature. Also: Cancel on the token label mints nothing, and +// shards open from the keyboard. Everything is set up through the +// console API; the form is driven through the real #panel-shards UI. + +import { test as base, expect, Page, BrowserContext, APIRequestContext } from "@playwright/test"; +import * as crypto from "node:crypto"; +import { start, GatewayStack } from "../fixtures/gateway"; +import { createTestUser, attachSession, TestUser } from "../fixtures/user"; + +const test = base.extend<{}, { stack: GatewayStack }>({ + stack: [ + async ({}, use) => { + const stack = await start({ admin: true }); + await use(stack); + await stack.stop(); + }, + { scope: "worker" }, + ], +}); + +function authed(user: TestUser) { + return { Cookie: user.cookieHeader, "Content-Type": "application/json" }; +} + +async function createShard(request: APIRequestContext, stack: GatewayStack, user: TestUser, extra: Record = {}) { + const id = `e2e-sp-${crypto.randomBytes(4).toString("hex")}`; + const resp = await request.post(`${stack.workspaceURL}/console/api/shards`, { + headers: authed(user), + data: { + id, + name: `Panel ${id}`, + description: "before", + persistence: "persistent", + visibility: "isolated", + scope_tag: `shard:${id}`, + system_prompt: "You answer questions about recipes, tersely.", + tool_allowlist: [], + ...extra, + }, + }); + expect(resp.ok(), `create shard: HTTP ${resp.status()} ${await resp.text()}`).toBeTruthy(); + return id; +} + +async function getShard(request: APIRequestContext, stack: GatewayStack, user: TestUser, id: string) { + const resp = await request.get(`${stack.workspaceURL}/console/api/shards/${id}`, { headers: authed(user) }); + expect(resp.ok()).toBeTruthy(); + return resp.json(); +} + +async function createBook(request: APIRequestContext, stack: GatewayStack, user: TestUser, name: string, archive = false) { + const resp = await request.post(`${stack.workspaceURL}/console/api/books`, { headers: authed(user), data: { name } }); + expect(resp.ok(), `create book: HTTP ${resp.status()}`).toBeTruthy(); + const b = await resp.json(); + if (archive) { + const a = await request.patch(`${stack.workspaceURL}/console/api/books/${b.slug}`, { + headers: authed(user), + data: { archive: true }, + }); + expect(a.ok(), `archive book: HTTP ${a.status()}`).toBeTruthy(); + } + return b as { id: string; slug: string }; +} + +async function openPanel(browser: any, stack: GatewayStack, user: TestUser): Promise<{ ctx: BrowserContext; page: Page }> { + const ctx = await browser.newContext(); + await attachSession(ctx, stack.workspaceURL, user); + const page = await ctx.newPage(); + await page.goto(stack.workspaceURL); + await expect(page.locator("#view-dashboard")).toBeVisible({ timeout: 15_000 }); + await page.evaluate(() => (window as any).appSwitchPanel("shards")); + await expect(page.locator("#panel-shards")).toBeVisible(); + await expect(page.locator("#shards-rows")).not.toContainText("Loading", { timeout: 10_000 }); + return { ctx, page }; +} + +// The list is hidden while a shard is open; go back to it first. +async function backToList(page: Page) { + if (await page.locator("#shard-detail-back").isVisible()) await page.locator("#shard-detail-back").click(); + await expect(page.locator("#shards-rows")).toBeVisible(); +} + +async function openShard(page: Page, id: string) { + await backToList(page); + await page.locator(`#shards-rows tr[data-id="${id}"]`).click(); + await expect(page.locator("#shard-id")).toHaveValue(id, { timeout: 10_000 }); +} + +async function save(page: Page, id: string) { + const done = page.waitForResponse( + (r) => r.url().endsWith(`/console/api/shards/${id}`) && r.request().method() === "PATCH", + ); + await page.locator('#shard-form button[type="submit"]').click(); + const resp = await done; + expect(resp.ok(), `PATCH: HTTP ${resp.status()} ${await resp.text()}`).toBeTruthy(); + await expect(page.locator("#shard-form-error")).toBeHidden(); +} + +async function openAdvanced(page: Page) { + const adv = page.locator("#shard-form .shard-advanced"); + if (!(await adv.evaluate((d: HTMLDetailsElement) => d.open))) await adv.locator("summary").click(); +} + +// An admin fixing a typo in someone else's confined shard saved +// book_access = the boxes ticked in a picker listing the ADMIN's books +// — none — and an empty list means all the owner's books. Archived and +// unlisted books were dropped the same way for the owner. The picker +// now lists the owner's books (archived ones marked), keeps IDs it +// can't name, and the save only sends book_access once it's touched. +test("an edit keeps the shard's book confinement", async ({ stack, browser, request }) => { + const owner = await createTestUser(); + const admin = await createTestUser({ role: "admin" }); + const recipes = await createBook(request, stack, owner, `Owner Recipes ${crypto.randomBytes(3).toString("hex")}`); + const old = await createBook(request, stack, owner, `Old Stuff ${crypto.randomBytes(3).toString("hex")}`, true); + const ghost = crypto.randomUUID(); + const books = [recipes.id, old.id, ghost]; + const id = await createShard(request, stack, owner, { book_access: books }); + + const { ctx, page } = await openPanel(browser, stack, admin); + try { + // The row opens from the keyboard (its name is a button). + const open = page.getByRole("button", { name: `Open shard: Panel ${id}` }); + await open.focus(); + await page.keyboard.press("Enter"); + await expect(page.locator("#shard-id")).toHaveValue(id, { timeout: 10_000 }); + + const picker = page.locator("#shard-book-access"); + await expect(picker.locator(`input[value="${recipes.id}"]`)).toBeChecked(); + await expect(picker.locator(`input[value="${old.id}"]`)).toBeChecked(); + await expect(picker.locator(`input[value="${ghost}"]`)).toBeChecked(); + await expect(picker).toContainText("(archived)"); + await expect(picker).toContainText(`(unavailable) ${ghost}`); + + await page.locator("#shard-description").fill("after"); + const patch = page.waitForRequest((r) => r.method() === "PATCH" && r.url().endsWith(`/shards/${id}`)); + await save(page, id); + expect(Object.keys((await patch).postDataJSON())).not.toContain("book_access"); + let s = await getShard(request, stack, owner, id); + expect(s.description).toBe("after"); + expect([...s.book_access].sort()).toEqual([...books].sort()); + + // Touching the picker does send it. + await openShard(page, id); + await picker.locator(`input[value="${ghost}"]`).uncheck(); + await save(page, id); + s = await getShard(request, stack, owner, id); + expect([...s.book_access].sort()).toEqual([recipes.id, old.id].sort()); + } finally { + await ctx.close(); + } +}); + +// Unticking both Chat and API emptied the prompt textarea, and the +// save persisted the empty prompt. +test("turning chat and API off keeps the system prompt", async ({ stack, browser, request }) => { + const owner = await createTestUser({ role: "admin" }); + const id = await createShard(request, stack, owner); + const { ctx, page } = await openPanel(browser, stack, owner); + try { + await openShard(page, id); + await page.locator("#shard-chat-enabled").uncheck(); + await page.locator("#shard-api-enabled").uncheck(); + await expect(page.locator("#shard-prompt")).toBeDisabled(); + await expect(page.locator("#shard-prompt")).toHaveValue("You answer questions about recipes, tersely."); + await save(page, id); + const s = await getShard(request, stack, owner, id); + expect(s.chat_enabled).toBe(false); + expect(s.api_enabled).toBe(false); + expect(s.system_prompt).toBe("You answer questions about recipes, tersely."); + + await openShard(page, id); + await page.locator("#shard-chat-enabled").check(); + await expect(page.locator("#shard-prompt")).toBeEnabled(); + await expect(page.locator("#shard-prompt")).toHaveValue("You answer questions about recipes, tersely."); + } finally { + await ctx.close(); + } +}); + +// Cancel on the label prompt fell through to the mint. +test("cancelling the token label mints nothing", async ({ stack, browser, request }) => { + const owner = await createTestUser({ role: "admin" }); + const id = await createShard(request, stack, owner); + const tokens = async () => { + const r = await request.get(`${stack.workspaceURL}/console/api/shards/${id}/tokens`, { headers: authed(owner) }); + expect(r.ok()).toBeTruthy(); + return ((await r.json()).items || []).length; + }; + const { ctx, page } = await openPanel(browser, stack, owner); + try { + await openShard(page, id); + let posts = 0; + page.on("request", (r) => { + if (r.method() === "POST" && r.url().endsWith(`/shards/${id}/tokens`)) posts++; + }); + page.once("dialog", (d) => d.dismiss()); + await page.locator("#shard-mint-token").click(); + await page.waitForTimeout(500); + expect(posts).toBe(0); + await expect(page.locator("#token-modal")).toBeHidden(); + expect(await tokens()).toBe(0); + + // OK still mints (the test can see a mint). + page.once("dialog", (d) => d.accept("kiosk")); + await page.locator("#shard-mint-token").click(); + await expect(page.locator("#token-modal")).toBeVisible({ timeout: 10_000 }); + expect(await tokens()).toBe(1); + } finally { + await ctx.close(); + } +}); + +// A blank temperature was sent as 0 (now honored as greedy sampling), +// and an emptied idle window or schema was never sent, so it stayed. +test("temperature, idle window and schemas save as shown", async ({ stack, browser, request }) => { + const owner = await createTestUser({ role: "admin" }); + const id = await createShard(request, stack, owner, { + temperature: 0.3, + session_max_age: 1800, + input_schema: { type: "object" }, + }); + const { ctx, page } = await openPanel(browser, stack, owner); + try { + await openShard(page, id); + await openAdvanced(page); + await page.locator("#shard-temperature").fill(""); + await page.locator("#shard-session-max-age").fill(""); + const schema = page.locator("#shard-form details.shard-schema").first(); + if (!(await schema.evaluate((d: HTMLDetailsElement) => d.open))) await schema.locator("summary").click(); + await page.locator("#shard-input-schema").fill(""); + await save(page, id); + let s = await getShard(request, stack, owner, id); + expect(s.temperature).toBeCloseTo(0.3, 5); + expect(s.session_max_age ?? null).toBeNull(); + expect(s.input_schema ?? null).toBeNull(); + + await openShard(page, id); + await openAdvanced(page); + await page.locator("#shard-temperature").fill("0"); + await save(page, id); + s = await getShard(request, stack, owner, id); + expect(s.temperature).toBe(0); + + // Create with the temperature blanked gets the default. + await backToList(page); + await page.locator("#shards-new").click(); + await expect(page.locator("#shard-id")).toBeEnabled(); + const nid = `e2e-sp-${crypto.randomBytes(4).toString("hex")}`; + await page.locator("#shard-id").fill(nid); + await page.locator("#shard-name").fill("Blank Temperature"); + await page.locator("#shard-prompt").fill("You are a test shard."); + await openAdvanced(page); + await page.locator("#shard-temperature").fill(""); + const created = page.waitForResponse( + (r) => r.url().endsWith("/console/api/shards") && r.request().method() === "POST", + ); + await page.locator('#shard-form button[type="submit"]').click(); + expect((await created).ok()).toBeTruthy(); + s = await getShard(request, stack, owner, nid); + expect(s.temperature).toBeCloseTo(0.7, 5); + } finally { + await ctx.close(); + } +});