oficina / memoria Public

🤖 fix: a committed conflict marker, a gate citing a test that did not exist

Three from the merge review, all code, all mine.

.GITIGNORE WAS COMMITTED WITH CONFLICT MARKERS and make ci was green,
because git parses the marker lines as harmless patterns. A file no gate
in this repo can read, broken for two commits. Both blocks kept.

THE EGRESS GATE'S SIBLING EXEMPTION WAS PAID BY A TEST THAT DID NOT EXIST.
gates_test.go said "sibling.TestClientRefusesHostsOutsideTheTeam asserts it
against the real constructor". There was no such test, and every test in
the package built its own &http.Client{} — so nothing exercised
suite.NewSiblingClient at all, and `func New(...) { return
&Client{http: &http.Client{}} }` would have stayed green. AGENTS.md's
second named failure, sitting inside the comment that claimed the payment.

The test exists now and goes through New. New takes a suite.DialFunc, nil
in production, so the gate can watch the real constructor rather than a
substitute. Three mutations red — plain client, empty team, dropped hook —
and ALL THREE are caught by the same-team half. The foreign half passes
under every one of them, vacuously: with no dial hook the recorder sees
nothing, so "it was not dialled" is true because nothing was dialled. A
one-sided version of this test would have gone green against a constructor
with no guard in it, which is the exact mutation it exists to catch.

"ONE PREDICATE" WAS NOT TRUE OF PRODUCTION. main.go passed
Admits(cfg.Owners) to NewAuth and separately called SetOwners(cfg.Owners):
two closures over two copies guarding one rule, while handlers.go claimed
"isOwner is the only predicate over it, so there is no second value to
disagree with". The review's mutation — Admits(nil) in main — stayed green
because every test built its own.

Admits is deleted. app.Admit is the one predicate, a method reading the one
list, normalising through normaliseAddress rather than re-spelling it
inline as Admits did. SetOwners now runs before NewAuth so the dependency
is visible rather than merely true. Also unspliced Admits's doc comment,
which the merge had left attached to normaliseAddress.

make ci green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Paul Campbell pushed by paul@keymail.dev 1 month ago 6867f56f36d7545f1743fb7bc3333c33370f96bc parent a6ec4b9

8 files changed, +125 −28

.gitignore +1 −3
cmd/memoria/main.go +15 −7
internal/memoria/auth.go +21 −13
internal/memoria/sibling/sibling.go +10 −2
internal/memoria/sibling/sibling_test.go +75 −0
internal/memoriatest/admission_test.go +1 −1
internal/memoriatest/gates_test.go +1 −1
internal/memoriatest/signindial_test.go +1 −1

diff --git a/.gitignore b/.gitignore

index b4d0a0f..30d8b48 100644

--- a/.gitignore

+++ b/.gitignore

@@ -7,7 +7,6 @@

/memoria.db-wal

/memoria.db-shm

-<<<<<<< HEAD

# Go caches belong OUTSIDE this tree. A GOMODCACHE inside the repo makes

# `make fmt-check` walk every vendored dependency's source and fail on

# other people's formatting — which is how this rule was learnt, not a

@@ -15,7 +14,7 @@

# writable directory elsewhere rather than at a dotfile here.

/.gc/

/.gmc/

-=======

# The copy-review working directory: a string index, a server log and the

# submitted result. Working artefacts of a review, not source.

/copy-review/

@@ -25,4 +24,3 @@

# Stray build output from `go build ./tools/drive`.

/drive

->>>>>>> main

diff --git a/cmd/memoria/main.go b/cmd/memoria/main.go

index 5521e3e..f7615f1 100644

--- a/cmd/memoria/main.go

+++ b/cmd/memoria/main.go

@@ -59,22 +59,30 @@ func main() {

// building a real sender either.

// "CARLOS" is the prefix the suite uses, matching Sheets.

+ //

+ // The owners list is set FIRST, because app.Admit reads it: it is the

+ // one admission predicate, used by rastrillo at verification, by the

+ // sign-in guard before the classifier dials, and by requireOwner on

+ // the connector routes. Ordering it after NewAuth would work — Admit

+ // reads the list at call time — and would hide the dependency, which

+ // is how the two-copy version got here.

+ //

+ // In slice 0 the connector routes check the same list the admission

+ // gate uses, so every signed-in person is an owner. requireOwner says

+ // that out loud rather than implying a restriction the app does not

+ // yet have.

+ app.SetOwners(cfg.Owners)

a, err := memoria.NewAuth(d.Writer(), memoria.AuthEnv{

Origin: cfg.Origin,

InstanceKey: cfg.InstanceKey,

Owners: cfg.Owners,

Mailer: mail.FromEnv("CARLOS", logger),

- }, logger, memoria.Admits(cfg.Owners))

+ }, logger, app.Admit)

if err != nil {

logger.Error("wire sign-in", "err", err)

os.Exit(1)

}

- // The connector routes check this list. In slice 0 it is the same

- // list the admission gate uses, so every signed-in person is an

- // owner — requireOwner says that out loud rather than implying a

- // restriction the app does not yet have.

- app.SetOwners(cfg.Owners)

app.UseAuth(a)

app.MountAuth(mux, cfg.Origin)

diff --git a/internal/memoria/auth.go b/internal/memoria/auth.go

index bb86c04..0b87f84 100644

--- a/internal/memoria/auth.go

+++ b/internal/memoria/auth.go

@@ -149,26 +149,34 @@ func ParseOwners(raw string) []string {

return out

}

-// Admits builds the admission gate. Comparison is on the lowercased

-// address: addresses are case-sensitive before the @ per RFC 5321 and no

-// mainstream provider honours that, so this matches what an operator

-// expects when they type a list.

// normaliseAddress is the one spelling rule. identity() lowercases and

// trims what auth verified; the guard must do the same to what a form

// submitted, or the two disagree about whether an address is an owner —

// and a guard that disagrees with the thing it guards is the bug this

// branch already shipped once, in a different clause.

+//

+// Addresses are case-sensitive before the @ per RFC 5321 and no mainstream

+// provider honours that, so this matches what an operator expects when

+// they type a list.

func normaliseAddress(s string) string { return strings.ToLower(strings.TrimSpace(s)) }

-func Admits(owners []string) func(string) bool {

- allowed := make(map[string]struct{}, len(owners))

- for _, o := range owners {

- allowed[o] = struct{}{}

- }

- return func(address string) bool {

- _, ok := allowed[strings.ToLower(strings.TrimSpace(address))]

- return ok

- }

+// Admit is THE admission predicate: the one rastrillo runs at

+// verification, the one beginGuard runs before the classifier dials, and

+// the one requireOwner runs on the connector routes.

+//

+// A method on the app rather than a constructor over a list, because a

+// constructor gives a caller something to build twice — and it was built

+// twice. main.go passed `Admits(cfg.Owners)` to NewAuth and separately

+// called `SetOwners(cfg.Owners)`: two closures over two copies guarding

+// one rule, and a review's mutation (`Admits(nil)` in main) stayed green

+// because every test built its own. One list, one predicate, nothing to

+// pass a second version of.

+//

+// It normalises through normaliseAddress. The old constructor re-spelled

+// that inline, which is how a guard and the thing it guards start to

+// disagree.

+func (a *app) Admit(address string) bool {

+ return a.isOwner(normaliseAddress(address))

}

// NewAuth wires the sign-in flow. Exactly one per process: auth carries

diff --git a/internal/memoria/sibling/sibling.go b/internal/memoria/sibling/sibling.go

index ef58429..1055f75 100644

--- a/internal/memoria/sibling/sibling.go

+++ b/internal/memoria/sibling/sibling.go

@@ -57,8 +57,16 @@ type Client struct {

}

// New returns a Client bound to one team.

-func New(team string) *Client {

- return &Client{http: suite.NewSiblingClient(team, nil)}

+//

+// dial is the hook the team-bound client dials through: **nil in

+// production**, a recorder in a test. It is a parameter rather than a

+// package variable because the gate that pays for this package's egress

+// exemption has to exercise the real constructor — an exemption whose

+// payment is a test that builds its own plain client is not a payment at

+// all, which is what a review found here (§10 gate 15, and AGENTS.md's

+// rule about naming a test that is not there).

+func New(team string, dial suite.DialFunc) *Client {

+ return &Client{http: suite.NewSiblingClient(team, dial)}

}

// Authorize asks one sibling whether this person may read these items, at

diff --git a/internal/memoria/sibling/sibling_test.go b/internal/memoria/sibling/sibling_test.go

index de311b9..5307044 100644

--- a/internal/memoria/sibling/sibling_test.go

+++ b/internal/memoria/sibling/sibling_test.go

@@ -3,7 +3,9 @@ package sibling

import (

"context"

"encoding/json"

+ "errors"

"io"

+ "net"

"net/http"

"net/http/httptest"

"strings"

@@ -24,6 +26,10 @@ func asker(t *testing.T) suite.Subject {

return s

}

+// errRecorded stands in for a connection. Being called at all is the

+// finding; the value never matters.

+var errRecorded = errors.New("memoriatest: dial recorded, not made")

func pairs(n int) []suite.ItemRevision {

out := make([]suite.ItemRevision, n)

for i := range out {

@@ -223,3 +229,72 @@ func TestWithheldNamesTheSiblingAndTheReason(t *testing.T) {

}

+// This is the test gate 15 names as the price of this package's egress

+// exemption, and until 2026-09-06 it did not exist — the gate's comment

+// cited it by name while `sibling_test.go` contained nothing of the kind,

+// and every other test here built a plain `&http.Client{}` that exercised

+// none of the guard. AGENTS.md's second named failure, in the comment

+// claiming the payment.

+//

+// It goes through the REAL constructor, because that is the whole point: a

+// gate that exempts a package from the egress grep buys the exemption with

+// a runtime guard the grep cannot perform, and a test that builds its own

+// client performs it on something production never uses.

+//

+// TWO-SIDED. "A foreign host is refused" is satisfied by a client that

+// refuses everything, by a constructor that returns a broken client, and

+// by a dial hook that is never installed. So the same-team case asserts

+// the dial DOES happen and reaches the host asked for.

+//

+// MUTATION-TESTED 2026-09-06, each reverted — and all three are caught by

+// the SAME-TEAM half, which is the argument for having it:

+//

+// New returns &Client{http: &http.Client{}} -> same-team case red

+// New passes "" as the team -> same-team case red

+// the dial hook is dropped (nil passed through) -> same-team case red

+//

+// The foreign half passes under all three, and vacuously: with no dial

+// hook installed the recorder sees nothing, so "it was not dialled" is

+// true because nothing was dialled at all. A one-sided version of this

+// test — assert only that a foreign host is refused — would have gone

+// green against a constructor that returns a client with no guard in it,

+// which is precisely the mutation this gate exists to catch.

+func TestClientRefusesHostsOutsideTheTeam(t *testing.T) {

+ const team = "acme"

+ var dialed []string

+ rec := func(ctx context.Context, network, addr string) (net.Conn, error) {

+ dialed = append(dialed, addr)

+ return nil, errRecorded

+ }

+ c := New(team, rec)

+ ask := func(host string) []suite.Authorization {

+ s := Sibling{Name: "Sheets", BaseURL: "https://" + host, Token: "t"}

+ return c.Authorize(context.Background(), s, asker(t), pairs(2))

+ }

+ t.Run("a host in another team is refused before it is dialled", func(t *testing.T) {

+ dialed = nil

+ got := ask("evil.sheets." + suite.SuiteDomain)

+ for i, a := range got {

+ if a != suite.UNKNOWN {

+ t.Errorf("pair %d = %q, want UNKNOWN", i, a)

+ }

+ if len(dialed) != 0 {

+ t.Errorf("a foreign team's host was dialled: %v — the check must run "+

+ "before the transport, so a refused host costs no connection", dialed)

+ }

+ })

+ t.Run("this team's host is reached, or the refusal proves nothing", func(t *testing.T) {

+ dialed = nil

+ ask(team + ".sheets." + suite.SuiteDomain)

+ if len(dialed) == 0 {

+ t.Error("this team's own sibling was never dialled: the client refuses " +

+ "everything, or the dial hook is not installed, and in both cases " +

+ "the other half of this gate is measuring nothing")

+ }

+ })

diff --git a/internal/memoriatest/admission_test.go b/internal/memoriatest/admission_test.go

index 4ca297f..b238692 100644

--- a/internal/memoriatest/admission_test.go

+++ b/internal/memoriatest/admission_test.go

@@ -69,7 +69,7 @@ func admissionApp(t *testing.T, dbPath string, owners []string) (http.Handler, *

InstanceKey: "admission-gate-instance-key",

Owners: owners,

Mailer: mailer,

- }, logger, memoria.Admits(owners))

+ }, logger, a.Admit)

if err != nil {

t.Fatalf("NewAuth: %v", err)

}

diff --git a/internal/memoriatest/gates_test.go b/internal/memoriatest/gates_test.go

index 0d5179f..0f2c574 100644

--- a/internal/memoriatest/gates_test.go

+++ b/internal/memoriatest/gates_test.go

@@ -198,7 +198,7 @@ func TestNoOutboundNetworkInAppCode(t *testing.T) {

for _, f := range forbidden {

pats = append(pats, regexp.MustCompile(f))

}

- // EXACTLY ONE package may reach the network, and this is the list.

+ // EXACTLY TWO packages may reach the network, and this is the list.

// It is a list rather than a deleted gate because the connector had

// to dial api.notion.com and the cheapest way to make this test pass

diff --git a/internal/memoriatest/signindial_test.go b/internal/memoriatest/signindial_test.go

index 17c8dd9..e2194bc 100644

--- a/internal/memoriatest/signindial_test.go

+++ b/internal/memoriatest/signindial_test.go

@@ -98,7 +98,7 @@ func signinAppWithOwners(t *testing.T, owners []string) (http.Handler, *dialReco

InstanceKey: "signin-dial-gate-instance-key",

Owners: []string{dialTestOwner},

Mailer: &stubMailer{},

- }, logger, memoria.Admits([]string{dialTestOwner}))

+ }, logger, a.Admit)

if err != nil {

t.Fatalf("NewAuth: %v", err)

}