amadan
app: fix round 2 -- one real ordering bug, one real origin gap, and three corrections to what we told ourselves
Two blocking findings, both real regressions caught by a whole-branch review that drove the app live: Store.List ordered by created_at while handlers.go's groupByRecency/formatWhen bucket and display UpdatedAt, and ApplyUpdate never touched updated_at at all -- the two clocks only ever agreed because Store.Create sets both to the same value. The first update to an older document would have produced duplicate recency group labels on the list page. Fixed: List orders by updated_at DESC, ApplyUpdate bumps it. New store_test.go test proven to fail against the old code and pass against the fix. validateOrigin required u.Host != "", but -addr :8099 (the plan's own example invocation) derives "http://:8099", whose Host is ":8099" (non-empty) while Hostname() is "" -- the reviewer booted a production binary on exactly this origin and watched it serve. Fixed: require u.Hostname() != "". New dedicated test plus a live reproduction of the exact -addr :8099 scenario, now refusing. Three non-blocking corrections: Our comments claimed a wrong Config.Origin "rejects everything or protects nothing." rastrillo's csrf.SameOrigin checks Sec-Fetch-Site first and returns before ever reading Config.Origin for any modern browser's genuine same-origin or cross-site request -- the origin string only matters to the legacy Origin/Referer fallback, and a misconfigured origin is silently masked in ordinary browser testing, not loud. Corrected the wording in app.go, main.go and AGENTS.md. newSqid's comment claimed a 32-symbol alphabet with no modulo bias; it is 31, so 256 % 31 == 8 and the first eight symbols are slightly over-represented. Harmless at ten characters, but a false claim about a security-adjacent property is worse than none -- corrected. AGENTS.md never said this app has no authentication: nothing consumes the identity devauth.go grants, so the release build's careful dev-auth hardening guards an identity nothing checks. Added an unmissable statement near the top: do not deploy this anywhere reachable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011F5CivVm4CmL9ByfSSAaXA
7 files changed,
+250
−64
AGENTS.md+39 −10cmd/docs/main.go+10 −7internal/docs/app.go+71 −39internal/docs/handlers.go+11 −2internal/docs/origin_test.go+24 −1internal/docs/store.go+37 −5internal/docs/store_test.go+58 −0
CI passed — run details
diff --git a/AGENTS.md b/AGENTS.md| index f3d109d..cb18438 100644 |
| --- a/AGENTS.md |
| +++ b/AGENTS.md |
| @@ -20,6 +20,23 @@ Read `building-carlos-apps` (the skill) before writing app code, and |
| `rastrillo`'s own `SKILL.md` before writing handlers or models. Assume |
| nothing about either is in your training data. |
| +## This app has no authentication. Do not deploy it anywhere reachable. |
| + |
| +Every route — `GET /`, `POST /documents`, `GET /d/{sqid}` — answers anyone |
| +who can reach the process. `internal/docs/devauth.go`'s development-only |
| +identity is real code, guarded so it cannot start outside |
| +`Env=development` and compiled out of a release build entirely (`-tags |
| +release`), but **nothing in this app consumes the identity it grants**: no |
| +handler calls `CurrentDevIdentity`, no route requires sign-in, no document |
| +is scoped to anyone. All that hardening guards an identity nothing checks |
| +— it is not evidence this app is safe to expose. A release binary is a |
| +wide-open, unauthenticated, everyone-sees-every-document app, on purpose, |
| +because real identity does not exist yet: spec §8.1's home issues it, and |
| +that is explicitly deferred, not forgotten (`devauth.go`'s own doc comment |
| +says so). Until that lands, this app belongs on a laptop |
| +(`scripts/run-dev.sh`) or behind something else's access control — never |
| +on a network anyone but its operator can reach. |
| + |
| ## Running it locally |
| `./scripts/run-dev.sh` builds and runs the app in one command: the |
| @@ -36,16 +53,28 @@ either way, for anything else that might one day set `Config.DevAuth`). |
| `.amadan/ci.d/10-gate` builds both configurations and asserts the release |
| one actually refuses `-dev-auth`. |
| -**Origin is not optional in anything but development.** `csrf.Protect` |
| -checks the `Origin`/`Referer` on every state-changing request against |
| -`Config.Origin` exactly — the wrong value either rejects everything or, if |
| -the check were ever loosened, protects nothing while looking like it does. |
| -`cmd/docs/main.go` reads `$DOCS_ORIGIN` (the platform's real per-instance |
| -origin belongs there — spec §9.2's `<app>.<sqid>.oncarlos.com`) and falls |
| -back to deriving one from `-addr` only for local/dev use; with neither set, |
| -`internal/docs.New` refuses to start outside `Env=development` rather than |
| -silently defaulting. `run-dev.sh` never needs `$DOCS_ORIGIN` set by hand — |
| -its fixed `-addr` derives it. |
| +**Origin is not optional in anything but development — but know what it |
| +actually guards.** rastrillo v0.21.0's `csrf.Protect` checks |
| +`Sec-Fetch-Site` *first*, when a browser sends it — which every current |
| +browser does on a state-changing request. `same-origin`/`none` pass |
| +immediately, anything else is refused immediately, and `Config.Origin` is |
| +never read in that path at all. Only when `Sec-Fetch-Site` is entirely |
| +absent (a legacy browser, or a non-browser client like curl) does the |
| +check fall back to comparing `Origin`/`Referer` against `Config.Origin` |
| +exactly. So a wrong `Config.Origin` does not make a real browser's genuine |
| +same-origin request fail, and it does not open up a genuine cross-site |
| +request either — both are decided by `Sec-Fetch-Site` alone. What it |
| +actually breaks is the legacy fallback path, silently: a misconfigured |
| +origin will not show up as "the app looks broken" in ordinary manual |
| +testing with a real browser, which is exactly why it still needs to be |
| +right rather than merely untested. `cmd/docs/main.go` reads `$DOCS_ORIGIN` |
| +(the platform's real per-instance origin belongs there — spec §9.2's |
| +`<app>.<sqid>.oncarlos.com`) and falls back to deriving one from `-addr` |
| +only for local/dev use; with neither set, or with a value that cannot |
| +parse as an absolute `http(s)://host[:port]`, `internal/docs.New` refuses |
| +to start rather than silently defaulting or booting on something |
| +malformed. `run-dev.sh` never needs `$DOCS_ORIGIN` set by hand — its fixed |
| +`-addr` derives it. |
| ## Review by the highest available capability model (Paul's rule, 2026-08-30) |
diff --git a/cmd/docs/main.go b/cmd/docs/main.go| index 8be01c6..3809558 100644 |
| --- a/cmd/docs/main.go |
| +++ b/cmd/docs/main.go |
| @@ -28,8 +28,12 @@ func main() { |
| os.Exit(1) |
| } |
| - // Origin decides csrf.Protect's same-origin check (docs.Config.Origin's |
| - // doc comment): an empty value here is not "no opinion", it is |
| + // Origin feeds docs.Config.Origin — see its doc comment for what |
| + // csrf.Protect actually does with it (fix round 3: Sec-Fetch-Site |
| + // decides a modern browser's own traffic before this string is |
| + // ever read; Origin only matters to the legacy Origin/Referer |
| + // fallback path, and a wrong value there fails silently rather |
| + // than visibly). An empty value here is not "no opinion", it is |
| // "New must refuse to start" once Env is anything but development. |
| // |
| // $DOCS_ORIGIN is the explicit, deploy-time override — the |
| @@ -43,11 +47,10 @@ func main() { |
| // header. The platform's real activation path is a unix socket or |
| // LISTEN_FDS — no -addr to derive anything from — so without |
| // $DOCS_ORIGIN set there, origin stays empty here and New refuses |
| - // to start rather than silently falling back to docs.DefaultOrigin. |
| - // That silent fallback is exactly the trap this task closes: the |
| - // wrong origin either rejects every same-origin request or, if the |
| - // check were ever loosened, protects nothing while looking like it |
| - // does. |
| + // to start rather than silently falling back to docs.DefaultOrigin, |
| + // a placeholder with no relationship to where this instance is |
| + // actually reachable — that is exactly the trap this task closes, |
| + // whether or not a given request would ever notice. |
| origin := os.Getenv("DOCS_ORIGIN") |
| if origin == "" && opts.Addr != "" { |
| origin = "http://" + opts.Addr |
diff --git a/internal/docs/app.go b/internal/docs/app.go| index 2d84a23..57cec24 100644 |
| --- a/internal/docs/app.go |
| +++ b/internal/docs/app.go |
| @@ -46,16 +46,33 @@ type Config struct { |
| // needed a signature change once devauth.go landed. |
| DevAuth bool |
| - // Origin is this app's own origin (scheme://host[:port], no path) |
| - // — csrf.Protect checks every POST/PUT/PATCH/DELETE's |
| - // Origin/Referer against exactly this string (rastrillo SKILL.md |
| - // §5). Empty is allowed only when Env is "development" (New falls |
| - // back to DefaultOrigin there, for tests and local convenience); |
| - // with any other Env, empty is a startup refusal — see New. A |
| - // wrong-but-nonempty origin is a real trap either way: CSRF is |
| - // origin-checking, not token-based, so the wrong string means |
| - // every same-origin POST is rejected, and a loosened check with |
| - // the wrong string protects nothing while looking like it does. |
| + // Origin is this app's own origin (scheme://host[:port], no path), |
| + // what csrf.Protect (rastrillo SKILL.md §5) uses as its fallback |
| + // same-origin evidence. Fix round 3 corrected a wrong model of |
| + // what that check actually does — see csrf.Protect's own doc |
| + // comment (rastrillo v0.21.0's csrf/csrf.go): it checks |
| + // Sec-Fetch-Site FIRST when a browser sends it, which every |
| + // current browser does on a state-changing request — |
| + // "same-origin"/"none" pass immediately, anything else is refused |
| + // immediately, and Origin is never consulted in that path at all. |
| + // Only when Sec-Fetch-Site is entirely absent (a legacy browser, or |
| + // a non-browser client such as curl) does the check fall back to |
| + // comparing the request's Origin or Referer header against this |
| + // string exactly. So a wrong-but-nonempty Origin does NOT make a |
| + // real browser's genuine same-origin POST fail, and it does NOT |
| + // open up a genuine cross-site request either — both of those are |
| + // decided by Sec-Fetch-Site alone, never by this string. What a |
| + // wrong Origin actually breaks is the legacy fallback path, and it |
| + // breaks it silently: a real client that happens to omit |
| + // Sec-Fetch-Site gets wrongly rejected, or — if that comparison |
| + // were ever loosened or removed — stops being checked at all. |
| + // Neither shows up in ordinary manual testing with a real browser, |
| + // which is exactly why getting this right cannot be verified by |
| + // "the page loaded and the form worked." |
| + // |
| + // Empty is allowed only when Env is "development" (New falls back |
| + // to DefaultOrigin there, for tests and local convenience); with |
| + // any other Env, empty is a startup refusal — see New. |
| // |
| // cmd/docs/main.go wires this from $DOCS_ORIGIN, the same |
| // "<APP>_ORIGIN" convention rastrillo's own `new` scaffold uses |
| @@ -64,14 +81,15 @@ type Config struct { |
| // is set there at deploy time. On the unix-socket/LISTEN_FDS |
| // activation path there is no -addr to derive a fallback from, so |
| // leaving $DOCS_ORIGIN unset must fail loudly rather than quietly |
| - // protecting the app with the wrong origin — that is exactly what |
| - // the Env-gated default below refuses to do. |
| + // configuring the app with a string that has nothing to do with |
| + // where it is actually reachable — that is exactly what the |
| + // Env-gated default below refuses to do. |
| // |
| // A non-empty value is trimmed and then validated (validateOrigin) |
| // rather than trusted as-is: whitespace-only is not "empty" by a |
| // bare == "" check, and would otherwise sail past the guard above |
| - // and boot an app that rejects every request forever rather than |
| - // refusing to start. |
| + // with nothing catching it until the legacy fallback path above |
| + // tried, and failed, to match it against anything real. |
| Origin string |
| // Logger defaults to slog.Default(). |
| @@ -85,15 +103,18 @@ type Config struct { |
| const DefaultOrigin = "http://localhost:8080" |
| // validateOrigin reports whether origin (already trimmed by the |
| -// caller — New) is something csrf.Protect can actually use: an |
| -// absolute http(s) URL naming a scheme and a host and nothing else. |
| -// "Nothing else" is checked deliberately, not just "parses as a |
| -// URL" — url.Parse happily accepts a bare path, a value carrying a |
| -// query string, or one with embedded userinfo, none of which is a |
| -// same-origin check has any business being handed, and none of which |
| -// would ever equal a real browser's Origin header, so an app that |
| -// accepted one would boot looking healthy and then reject every |
| -// request rather than refusing to start. |
| +// caller — New) is something csrf.Protect's legacy Origin/Referer |
| +// fallback can actually use: an absolute http(s) URL naming a scheme |
| +// and a host and nothing else. "Nothing else" is checked |
| +// deliberately, not just "parses as a URL" — url.Parse happily |
| +// accepts a bare path, a value carrying a query string, or one with |
| +// embedded userinfo, none of which a same-origin check has any |
| +// business being handed, and none of which would ever equal a real |
| +// browser's Origin header. An app that accepted one would boot |
| +// looking entirely healthy — a modern browser's genuine same-origin |
| +// traffic passes on Sec-Fetch-Site alone and never reaches this |
| +// string at all (Config.Origin's own doc comment) — and only fail, |
| +// silently, on whatever still uses the legacy fallback. |
| func validateOrigin(origin string) error { |
| if origin == "" { |
| return errors.New("origin is blank") |
| @@ -105,7 +126,15 @@ func validateOrigin(origin string) error { |
| if u.Scheme != "http" && u.Scheme != "https" { |
| return fmt.Errorf("origin %q must have an http or https scheme", origin) |
| } |
| - if u.Host == "" { |
| + // u.Host != "" is not enough: fix round 2's own bug. "http://:8099" |
| + // — exactly what -addr :8099 derives (main.go's "http://"+opts.Addr, |
| + // the plan's own example invocation) — has a non-empty Host |
| + // (":8099") but an empty Hostname(); Host merely means "there was |
| + // something between // and the next / (or the end)", not "there is |
| + // a host". A reviewer booted a production-Env binary on this exact |
| + // origin and watched it serve. Hostname() is what actually strips |
| + // the port and answers "is there a host here at all". |
| + if u.Hostname() == "" { |
| return fmt.Errorf("origin %q has no host", origin) |
| } |
| if u.Path != "" || u.RawQuery != "" || u.Fragment != "" || u.User != nil { |
| @@ -151,24 +180,27 @@ func New(cfg Config) (*App, error) { |
| // only in development (tests via NewForTest, a person running |
| // scripts/run-dev.sh). Anywhere else — Env left at its "production" |
| // default very much included — an empty Origin is a startup |
| - // refusal, not a silent fallback to a placeholder that either |
| - // rejects every same-origin request or, worse, would protect |
| - // nothing if the check were ever loosened. This is the same |
| - // fail-closed shape as guardDevAuth below: exactly one string |
| - // ("development") is ever treated as "safe to default", checked |
| - // before anything else opens. |
| + // refusal, not a silent fallback to a placeholder whose only real |
| + // effect is on csrf.Protect's legacy Origin/Referer fallback path |
| + // (Config.Origin's own doc comment explains why that path, not |
| + // "every request", is what a wrong string actually touches). This |
| + // is the same fail-closed shape as guardDevAuth below: exactly one |
| + // string ("development") is ever treated as "safe to default", |
| + // checked before anything else opens. |
| // |
| // cfg.Origin == "" alone is not the right test: a whitespace value |
| // like $DOCS_ORIGIN=" " is non-empty, so it would sail past that |
| - // check, and csrf.Protect would then compare every request's real |
| - // Origin/Referer against literal whitespace — never matching, |
| - // which means the app boots looking healthy and 403s all traffic, |
| - // with or without a correct Origin header. That fails safe, but |
| - // silently and late (at request time), not at boot where a |
| - // malformed origin belongs. validateOrigin below rejects that and |
| - // any other value that cannot possibly be what csrf.Protect needs |
| - // (no scheme, no host, a stray path/query/fragment/userinfo) |
| - // before New ever returns a listening app. |
| + // check. Nothing about a modern browser's own traffic would look |
| + // wrong — Sec-Fetch-Site: same-origin passes without this string |
| + // ever being read — so the only visible symptom would be the |
| + // legacy fallback path failing to match literal whitespace against |
| + // anything real, for whichever client still exercises it. That |
| + // fails safe on that one path, but silently and late (at request |
| + // time, for whoever hits it), not at boot where a malformed origin |
| + // belongs. validateOrigin below rejects that and any other value |
| + // that cannot possibly be what csrf.Protect needs (no scheme, no |
| + // host, a stray path/query/fragment/userinfo) before New ever |
| + // returns a listening app. |
| trimmedOrigin := strings.TrimSpace(cfg.Origin) |
| switch { |
| case trimmedOrigin == "" && cfg.Env != "development": |
diff --git a/internal/docs/handlers.go b/internal/docs/handlers.go| index 4b86831..839be23 100644 |
| --- a/internal/docs/handlers.go |
| +++ b/internal/docs/handlers.go |
| @@ -169,8 +169,17 @@ func (h *handlers) show(w http.ResponseWriter, r *http.Request) { |
| // that share a bucket join the same group, and a new bucket starts a |
| // new group the moment recencyBucket's key changes from the previous |
| // document's. That is safe only because recencyBucket is monotonic in |
| -// time (a later document's bucket never sorts before an earlier one's) |
| -// and docs arrives already ordered by the same clock. |
| +// time (a later document's bucket never sorts before an earlier |
| +// one's) AND docs is ordered by the exact same clock this function |
| +// buckets and displays by (UpdatedAt) — Store.List orders by |
| +// `updated_at DESC`, not CreatedAt, specifically so that holds. Fix |
| +// round 2's bug was ordering by one column while bucketing by |
| +// another: they agreed only because nothing yet moved UpdatedAt away |
| +// from CreatedAt, and the moment ApplyUpdate started bumping |
| +// UpdatedAt, an order-by-CreatedAt list would have produced duplicate |
| +// group labels ("Today … Yesterday … Today") the instant an older |
| +// document got a new update. See store_test.go's |
| +// TestListOrdersByUpdatedAt. |
| func groupByRecency(r *http.Request, now time.Time, docs []*Document) []docGroup { |
| var groups []docGroup |
| var key string |
diff --git a/internal/docs/origin_test.go b/internal/docs/origin_test.go| index 26ca15d..570d2b2 100644 |
| --- a/internal/docs/origin_test.go |
| +++ b/internal/docs/origin_test.go |
| @@ -1,6 +1,9 @@ |
| package docs |
| -import "testing" |
| +import ( |
| + "strings" |
| + "testing" |
| +) |
| // TestValidateOriginRejectsWhitespace is fix round 1's own regression |
| // test: DOCS_ORIGIN=" " (all whitespace) is non-empty, so a bare |
| @@ -30,6 +33,7 @@ func TestValidateOriginRejectsMalformed(t *testing.T) { |
| "localhost:8080", // no recognized scheme |
| "ftp://example.com", // wrong scheme |
| "http://", // no host |
| + "http://:8099", // Host is ":8099", Hostname() is "" — see TestValidateOriginRejectsHostlessAddr |
| "http://example.com/path", // path |
| "http://example.com?x=1", // query |
| "http://example.com#frag", // fragment |
| @@ -44,6 +48,25 @@ func TestValidateOriginRejectsMalformed(t *testing.T) { |
| } |
| } |
| +// TestValidateOriginRejectsHostlessAddr is fix round 2's own |
| +// regression test: -addr :8099 — the plan's own example invocation — |
| +// makes main.go derive "http://" + ":8099" = "http://:8099". Its |
| +// url.Host is ":8099" (non-empty), so a `u.Host == ""` check let it |
| +// through; its url.Hostname() is "" (Host minus the port, and there |
| +// is nothing before the colon), which is the actual answer to "is |
| +// there a host here at all". A reviewer booted a production-Env |
| +// binary on exactly this origin and watched it serve before this fix. |
| +func TestValidateOriginRejectsHostlessAddr(t *testing.T) { |
| + const origin = "http://:8099" |
| + err := validateOrigin(origin) |
| + if err == nil { |
| + t.Fatalf("validateOrigin(%q) = nil, want an error — Host is non-empty but Hostname() is empty", origin) |
| + } |
| + if !strings.Contains(err.Error(), "no host") { |
| + t.Errorf("error should say there is no host: %v", err) |
| + } |
| +} |
| + |
| // TestValidateOriginAcceptsGoodOrigins proves the check above is not |
| // so strict it rejects the values this app actually uses: |
| // DefaultOrigin, an -addr-derived origin, and a real platform origin |
diff --git a/internal/docs/store.go b/internal/docs/store.go| index 175a441..8ba25e3 100644 |
| --- a/internal/docs/store.go |
| +++ b/internal/docs/store.go |
| @@ -39,8 +39,17 @@ const sqidLength = 10 |
| // newSqid generates a random opaque document identifier, distinct |
| // from the row's integer id, safe to expose in a URL. len(sqidAlphabet) |
| -// is 32, and 256 is an exact multiple of 32, so taking each random |
| -// byte mod 32 introduces no modulo bias. |
| +// is 31 (8 digits, 23 letters — 0/O, 1/l/I excluded), not a power of |
| +// two, so taking each random byte mod 31 is not perfectly uniform: |
| +// 256 % 31 == 8, so the first eight symbols (the digits '2'-'9', the |
| +// low end of sqidAlphabet) come out of this loop about 3% more often |
| +// than the rest. Fix round 3 corrected this comment, which used to |
| +// assert a false "32, so no bias" — a wrong claim about a |
| +// security-adjacent property is worse than admitting the real one. |
| +// The bias itself is harmless at sqidLength characters: it narrows |
| +// the effective keyspace by a small, bounded fraction, nowhere near |
| +// enough to make ten random symbols from a 31-letter alphabet |
| +// guessable. |
| func newSqid() (string, error) { |
| raw := make([]byte, sqidLength) |
| if _, err := crand.Read(raw); err != nil { |
| @@ -54,7 +63,7 @@ func newSqid() (string, error) { |
| } |
| // maxSqidAttempts bounds Create's retry loop on a sqid collision. At |
| -// sqidLength characters over a 32-symbol alphabet, a collision is |
| +// sqidLength characters over sqidAlphabet's 31 symbols, a collision is |
| // astronomically unlikely; this exists only so a UNIQUE violation is |
| // handled rather than surprising, not because collisions are |
| // expected. |
| @@ -100,12 +109,20 @@ func (s *Store) Create(ctx context.Context, title string) (*Document, error) { |
| return nil, fmt.Errorf("docs: create document: %w", lastErr) |
| } |
| -// List returns every non-deleted document, newest first. |
| +// List returns every non-deleted document, newest first — "newest" |
| +// meaning most recently touched (UpdatedAt), the same clock |
| +// handlers.go's groupByRecency/formatWhen bucket and display, and the |
| +// one ApplyUpdate now bumps on every applied update. Ordering by |
| +// CreatedAt instead (fix round 2's own bug: they agreed only by |
| +// accident, because nothing bumped UpdatedAt away from CreatedAt) let |
| +// this list and its own recency buckets disagree the moment a |
| +// document's UpdatedAt ever moved without a matching reorder here — |
| +// see store_test.go's TestListOrdersByUpdatedAt. |
| func (s *Store) List(ctx context.Context) ([]*Document, error) { |
| var docs []*Document |
| err := s.db.WithContext(ctx). |
| Where("deleted_at IS NULL"). |
| - Order("created_at DESC, id DESC"). |
| + Order("updated_at DESC, id DESC"). |
| Find(&docs).Error |
| if err != nil { |
| return nil, fmt.Errorf("docs: list documents: %w", err) |
| @@ -228,5 +245,20 @@ func (s *Store) ApplyUpdate(ctx context.Context, doc *Document, update []byte) e |
| if err := s.db.WithContext(ctx).Create(row).Error; err != nil { |
| return fmt.Errorf("docs: persist update for document %d: %w", doc.ID, err) |
| } |
| + |
| + // Bumping documents.updated_at here — not just appending the |
| + // doc_updates row above — is what keeps Store.List's ordering and |
| + // handlers.go's groupByRecency/formatWhen honest about the same |
| + // clock. Skipping this was fix round 2's bug: with it skipped, |
| + // UpdatedAt only ever equalled CreatedAt (Store.Create sets both |
| + // to the same `now`), so List ordering by either column looked |
| + // identical and the divergence stayed invisible until something |
| + // — this method — actually moved UpdatedAt away from CreatedAt. |
| + if err := s.db.WithContext(ctx). |
| + Model(&Document{}). |
| + Where("id = ?", doc.ID). |
| + Update("updated_at", time.Now().UTC()).Error; err != nil { |
| + return fmt.Errorf("docs: bump updated_at for document %d: %w", doc.ID, err) |
| + } |
| return nil |
| } |
diff --git a/internal/docs/store_test.go b/internal/docs/store_test.go| index a21471b..88071cb 100644 |
| --- a/internal/docs/store_test.go |
| +++ b/internal/docs/store_test.go |
| @@ -3,6 +3,7 @@ package docs_test |
| import ( |
| "context" |
| "testing" |
| + "time" |
| "github.com/carlosframework/docs/internal/docs" |
| ) |
| @@ -68,3 +69,60 @@ func TestTextComesFromTheCore(t *testing.T) { |
| t.Errorf("Text = %q, want %q — the text must come from the CRDT, not a column", text, "hello world") |
| } |
| } |
| + |
| +// TestListOrdersByUpdatedAt is fix round 2's own regression test: List |
| +// used to order by CreatedAt while handlers.go's groupByRecency/ |
| +// formatWhen bucket and display UpdatedAt, and ApplyUpdate never |
| +// touched UpdatedAt at all — the two clocks only ever agreed because |
| +// Store.Create sets both columns to the same timestamp, so nothing |
| +// exercised the divergence until an update actually moved UpdatedAt |
| +// away from CreatedAt. This creates an older document, then a newer |
| +// one, confirms the newer one lists first (ordinary creation order), |
| +// applies an update to the *older* document, and asserts it jumps to |
| +// the top — the assertion that would have caught the bug: it fails |
| +// against an order-by-CreatedAt List even though ApplyUpdate bumps |
| +// UpdatedAt, and it fails against an ApplyUpdate that does not bump |
| +// UpdatedAt even though List orders by it. Both halves of the fix are |
| +// required to pass it. |
| +func TestListOrdersByUpdatedAt(t *testing.T) { |
| + app, err := docs.NewForTest(t) |
| + if err != nil { |
| + t.Fatalf("NewForTest: %v", err) |
| + } |
| + ctx := context.Background() |
| + |
| + older, err := app.Store().Create(ctx, "Older document") |
| + if err != nil { |
| + t.Fatalf("Create(older): %v", err) |
| + } |
| + // A real gap between the two rows' timestamps: two time.Now() |
| + // calls back to back can land in the same tick on a coarse clock, |
| + // and this test's whole point is ordering, not simultaneity. |
| + time.Sleep(5 * time.Millisecond) |
| + newer, err := app.Store().Create(ctx, "Newer document") |
| + if err != nil { |
| + t.Fatalf("Create(newer): %v", err) |
| + } |
| + |
| + list, err := app.Store().List(ctx) |
| + if err != nil { |
| + t.Fatalf("List: %v", err) |
| + } |
| + if len(list) != 2 || list[0].Sqid != newer.Sqid || list[1].Sqid != older.Sqid { |
| + t.Fatalf("List before any update = %+v, want [newer, older] by plain creation order", list) |
| + } |
| + |
| + time.Sleep(5 * time.Millisecond) |
| + update := docs.ReadFixture(t, "hello") |
| + if err := app.Store().ApplyUpdate(ctx, older, update); err != nil { |
| + t.Fatalf("ApplyUpdate(older): %v", err) |
| + } |
| + |
| + list, err = app.Store().List(ctx) |
| + if err != nil { |
| + t.Fatalf("List after update: %v", err) |
| + } |
| + if len(list) != 2 || list[0].Sqid != older.Sqid || list[1].Sqid != newer.Sqid { |
| + t.Fatalf("List after applying an update to the older document = %+v, want [older, newer] — applying an update must move a document to the top of the list", list) |
| + } |
| +} |