oficina / meet Public

settings: fix round 1 — no-store, WriteHeader, autofocus

Review verdict: needs fixes, minor. Addresses all six items:
- Cache-Control: no-store on both mint pages (agent_once and
  link_once), with a test on both.
- A settings-list test asserting no 64-hex (TokenHash-shaped) string
  ever renders.
- Autofocus removed from settings.html's Name field (the vendored
  field partial forbids it on a settings panel).
- renderSettings now owns its one WriteHeader call (took a status
  parameter); a forced-store-error test reproduces the prior 422-
  instead-of-500 bug and confirms the fix.
- errAgentDuplicate reconfirmed wired end to end (fix round 1's own
  test, unmodified).
- Corrected the report's test count (nine, not ten, before this
  round's two additions).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CzvnuKhLzudvq7jZ7SvVt2

Paul Campbell pushed by paul@keymail.dev 1 month ago e8517d1973f04d5b57132482d41ca6b639b35115 parent e9539a3

5 files changed, +83 −15

docs/superpowers/copy/2026-09-10-slice-2-strings.md +6 −6
internal/meet/handlers_meetings.go +1 −0
internal/meet/handlers_settings.go +18 −8
internal/meet/templates/settings.html +1 −1
internal/meettest/agents_test.go +57 −0

CI passed — run details

diff --git a/docs/superpowers/copy/2026-09-10-slice-2-strings.md b/docs/superpowers/copy/2026-09-10-slice-2-strings.md

index ceb4596..efdc9b8 100644

--- a/docs/superpowers/copy/2026-09-10-slice-2-strings.md

+++ b/docs/superpowers/copy/2026-09-10-slice-2-strings.md

@@ -292,10 +292,10 @@ in this file.

| Revoked | `settings.html`, a revoked row (`ui/status-pill`, default neutral tone) | The one-word state for a row that used to show its principal. A pill, not a link, because there is no action left to take from here — spec's "state is never colour alone" (AGENTS.md): the tone is neutral and the word is what actually carries the meaning. |

| No agents yet. | `settings.html`, the empty list (`ui/empty-state`) | Deliberately terser than "No meetings yet. Make one above and share its link." (`index.html`'s own empty state): an agent is a less familiar idea than a meeting, and this screen's create form sits directly under the heading with no other section between — the empty state does not need to teach what the form immediately beneath it already explains at a glance. |

| Give the agent a name of 1 to 64 letters, digits, - or _. | `handlers_settings.go`'s `createAgentHandler`, beside the Name field on `errAgentName` | Spells out the rule `agentNameRe` (`agents.go`) enforces, the same register as "Keep the title under 200 characters." — the limit stated as a fact about the field, not a complaint about what was typed. The submitted name is never echoed back inside this sentence (only into the field's own `Value`, `ui/field`'s ordinary redisplay), so a name containing something worth not repeating verbatim in a message never has to be. |

-| You already have 20 live agents. Revoke one before creating another. | `handlers_settings.go`'s `createAgentHandler`, beside the Name field when the operator is at the cap | The refusal for the twenty-live-agent ceiling the Task 1 review asked for. States the number and the way out, the same shape as the sign-in form's "Too many sign-in attempts just now. Wait a few minutes and try again." Enforced today by the handler's own count (`liveAgentCount`, `maxLiveAgents`) rather than by the store: `store.createAgent` does not yet return a sentinel for this, so the `errAgentLimit` branch this sentence also serves is unreachable until a later fix round wires the check into `createAgent` itself — see `handlers_settings.go`'s comment on `maxLiveAgents` and the task's report. |

-| You already have a live agent with that name. | `handlers_settings.go`'s `createAgentHandler`, beside the Name field on `errAgentDuplicate` | Scaffolding only, per the same Task 1 addendum: `store.createAgent` does not check for a live duplicate name yet, so nothing in this slice can actually produce this refusal — the sentence and its `errors.Is` branch exist now so a later fix round's store-side check renders correctly the moment it starts returning `errAgentDuplicate`, with no copy change needed then. |

+| You already have 20 live agents. Revoke one before creating another. | `handlers_settings.go`'s `createAgentHandler`, beside the Name field on `errAgentLimit` | The refusal for the twenty-live-agent ceiling the Task 1 review asked for. States the number and the way out, the same shape as the sign-in form's "Too many sign-in attempts just now. Wait a few minutes and try again." As of fix round 1, `store.createAgent` itself counts live agents (inside the same transaction as the insert) and returns `errAgentLimit` at twenty; the handler only renders the sentence — see `agents.go` and the task's report. |

+| You already have a live agent with that name. | `handlers_settings.go`'s `createAgentHandler`, beside the Name field on `errAgentDuplicate` | As of fix round 1, `store.createAgent` checks for a live duplicate name itself (a plain count query in the same transaction, no unique index — a revoked name is free to reuse at once) and returns `errAgentDuplicate`; the handler only renders the sentence. See `agents.go` and the task's report. |

-Two sentinel errors, `errAgentLimit` and `errAgentDuplicate`

-(`handlers_settings.go`), are declared outside `agents.go` for the

-duration of this task only, and are not user-facing strings themselves

-— see the report's Deviations for why, and what should move where.

+Two sentinel errors, `errAgentLimit` and `errAgentDuplicate`, live in

+`agents.go` beside `errAgentName` as of fix round 1 — see the task's

+report for the move from `handlers_settings.go`, where they were

+declared as scaffolding for this task's duration only.

diff --git a/internal/meet/handlers_meetings.go b/internal/meet/handlers_meetings.go

index 742b4a6..c65614a 100644

--- a/internal/meet/handlers_meetings.go

+++ b/internal/meet/handlers_meetings.go

@@ -129,6 +129,7 @@ func (a *app) startCall(w http.ResponseWriter, r *http.Request) {

// query string lands in the edge's access logs (Sheets' ruling), and

// the page is the one time the token exists in plaintext.

func (a *app) renderLinkOnce(w http.ResponseWriter, r *http.Request, m Meeting, token string, replaced bool) {

+ w.Header().Set("Cache-Control", "no-store")

render(w, "link_once", a.page(r, map[string]any{

"Sqid": m.Sqid, "Title": a.store.title(m), "Starts": a.fmtStart(m.StartsAt),

"Link": a.linkURL(token), "Replaced": replaced,

diff --git a/internal/meet/handlers_settings.go b/internal/meet/handlers_settings.go

index 4a9a575..777cb3a 100644

--- a/internal/meet/handlers_settings.go

+++ b/internal/meet/handlers_settings.go

@@ -21,13 +21,21 @@ type settingsRow struct {

}

func (a *app) settings(w http.ResponseWriter, r *http.Request) {

- a.renderSettings(w, r, "", "")

+ a.renderSettings(w, r, http.StatusOK, "", "")

}

// renderSettings is GET /settings and POST /settings/agents' shared

// render: name and nameErr repopulate the create form on a validation

// failure, exactly as renderIndex repopulates the meeting form.

-func (a *app) renderSettings(w http.ResponseWriter, r *http.Request, name, nameErr string) {

+//

+// status is written here, and only here (Task 2 review): a caller

+// that wrote its own header first and then called this could double

+// call WriteHeader the moment agentsFor below failed and this fell

+// through to ErrorPage's own — a superfluous, logged call, and on some

+// versions of net/http a header stuck at the first value regardless of

+// the second. Taking status as a parameter keeps the one call in the

+// one place that knows whether it actually got to render at all.

+func (a *app) renderSettings(w http.ResponseWriter, r *http.Request, status int, name, nameErr string) {

p, _ := currentPerson(r)

agents, err := a.store.agentsFor(r.Context(), p)

if err != nil {

@@ -42,6 +50,7 @@ func (a *app) renderSettings(w http.ResponseWriter, r *http.Request, name, nameE

Revoked: ag.RevokedAt != nil, Principal: principal(ag, p),

})

}

+ w.WriteHeader(status)

render(w, "settings", a.page(r, map[string]any{

"Agents": rows, "Name": name, "NameError": nameErr,

}))

@@ -61,20 +70,21 @@ func (a *app) createAgentHandler(w http.ResponseWriter, r *http.Request) {

if err != nil {

switch {

case errors.Is(err, errAgentName):

- w.WriteHeader(http.StatusUnprocessableEntity)

- a.renderSettings(w, r, name, "Give the agent a name of 1 to 64 letters, digits, - or _.")

+ a.renderSettings(w, r, http.StatusUnprocessableEntity, name, "Give the agent a name of 1 to 64 letters, digits, - or _.")

case errors.Is(err, errAgentLimit):

- w.WriteHeader(http.StatusUnprocessableEntity)

- a.renderSettings(w, r, name, "You already have 20 live agents. Revoke one before creating another.")

+ a.renderSettings(w, r, http.StatusUnprocessableEntity, name, "You already have 20 live agents. Revoke one before creating another.")

case errors.Is(err, errAgentDuplicate):

- w.WriteHeader(http.StatusUnprocessableEntity)

- a.renderSettings(w, r, name, "You already have a live agent with that name.")

+ a.renderSettings(w, r, http.StatusUnprocessableEntity, name, "You already have a live agent with that name.")

default:

a.logger.Error("meet: create agent", "err", err)

a.ErrorPage(w, r, http.StatusInternalServerError, "")

}

return

}

+ // The token exists in plaintext on this response only (spec §8.1);

+ // a cache must never keep a copy of it around (Task 2 review, same

+ // property renderLinkOnce already holds for a meeting link).

+ w.Header().Set("Cache-Control", "no-store")

render(w, "agent_once", a.page(r, map[string]any{

"Name": ag.Name, "Principal": principal(ag, p), "Token": token,

}))

diff --git a/internal/meet/templates/settings.html b/internal/meet/templates/settings.html

index 885b417..91bd11c 100644

--- a/internal/meet/templates/settings.html

+++ b/internal/meet/templates/settings.html

@@ -25,7 +25,7 @@

<h2>Create an agent</h2>

- {{template "field" dict "ID" "name" "Name" "name" "Label" "Name" "Required" true "Maxlength" "64" "Error" .NameError "Value" .Name "Autofocus" true}}

+ {{template "field" dict "ID" "name" "Name" "name" "Label" "Name" "Required" true "Maxlength" "64" "Error" .NameError "Value" .Name}}

<div rst-form-actions><button type="submit" rst-btn="primary lg">Create an agent</button></div>

</form>

</section>

diff --git a/internal/meettest/agents_test.go b/internal/meettest/agents_test.go

index 1273fa0..1d9cfe2 100644

--- a/internal/meettest/agents_test.go

+++ b/internal/meettest/agents_test.go

@@ -16,6 +16,10 @@ import (

var (

agentTokenRe = regexp.MustCompile(`mt_[A-Za-z0-9_-]+`)

revokeHrefRe = regexp.MustCompile(`/settings/agents/(\d+)/revoke`)

+ // hex64Re is the shape of TokenHash (hex.EncodeToString of a

+ // SHA-256 sum): nothing rendered to a browser should ever match

+ // it, whether that would be the plaintext token or its own hash.

+ hex64Re = regexp.MustCompile(`\b[0-9a-f]{64}\b`)

)

func extractToken(t *testing.T, body string) string {

@@ -84,6 +88,9 @@ func TestCreatingAnAgentShowsTheTokenOnceWithThePrincipal(t *testing.T) {

if strings.Contains(listBody, token) {

t.Fatalf("the settings list shows the token:\n%s", listBody)

}

+ if hex64Re.MatchString(listBody) {

+ t.Fatalf("the settings list contains a 64-hex string (TokenHash):\n%s", listBody)

+ }

if !strings.Contains(listBody, "scheduler~paul@example.com") {

t.Fatalf("the settings list lacks the principal for a live agent:\n%s", listBody)

}

@@ -147,6 +154,56 @@ func TestRevokeConfirmsAndRevokes(t *testing.T) {

}

+// Review round 1, item 1: the token and a meeting link both exist in

+// plaintext on exactly one response each (spec §8.1; Sheets' ruling

+// for a link). A cache must never become a second place either one

+// survives.

+func TestMintPagesAreNeverCached(t *testing.T) {

+ x, _, _ := memberWithMeeting(t)

+ c := x.browser(t)

+ c.signIn("paul@example.com")

+ link := c.post("/meetings", url.Values{"title": {"Board meeting 2"}, "starts_at": {""}})

+ if link.Code != http.StatusOK {

+ t.Fatalf("create meeting: %d\n%s", link.Code, link.Body.String())

+ }

+ if got := link.Header().Get("Cache-Control"); got != "no-store" {

+ t.Fatalf("link_once Cache-Control: %q, want %q", got, "no-store")

+ }

+ agent := c.post("/settings/agents", url.Values{"name": {"scheduler"}})

+ if agent.Code != http.StatusOK {

+ t.Fatalf("create agent: %d\n%s", agent.Code, agent.Body.String())

+ }

+ if got := agent.Header().Get("Cache-Control"); got != "no-store" {

+ t.Fatalf("agent_once Cache-Control: %q, want %q", got, "no-store")

+ }

+// Review round 1, item 4: renderSettings is the only place that calls

+// WriteHeader for either of its two callers' responses. Before that

+// fix, a validation refusal (which wants 422) whose own renderSettings

+// then hit a genuine store error still answered 422: a real

+// ResponseWriter — and httptest's own recorder, which matches it here

+// on purpose — keeps only the first WriteHeader call and silently

+// drops the second. Dropping the agents table forces that second,

+// genuine error while refusing a bad name, which itself never touches

+// the database, so the only way this test can pass is if nothing wrote

+// 422 before the store read had a chance to fail.

+func TestCreateRefusalWithABrokenStoreAnswersFiveHundredNotTheFormsRefusal(t *testing.T) {

+ x, _, _ := memberWithMeeting(t)

+ c := x.browser(t)

+ c.signIn("paul@example.com")

+ if err := x.db.G.Exec("DROP TABLE agents").Error; err != nil {

+ t.Fatal(err)

+ }

+ rec := c.post("/settings/agents", url.Values{"name": {"has a space"}})

+ if rec.Code != http.StatusInternalServerError {

+ t.Fatalf("name refusal with a broken store: %d, want %d", rec.Code, http.StatusInternalServerError)

+ }

// A foreign or unknown agent id 404s — never a 403 — for both the

// confirm page and the POST.

func TestForeignOrUnknownAgentIDIs404(t *testing.T) {