oficina / slides Public

<task-notification> <task-id>a8f31e63316cf8932</task-id> <tool-use-id>toolu_01EbBFCrCokCGVW7nahSmSjh</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina/2420c44b-a27a-4e0d-bbe7-8b3ca0464c21/tasks/a8f31e63316cf8932.output</output-file> <status>completed</status> <summary>Agent "Implement Task 7: drive API" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>**Status:** complete, `make ci` green (vet, gofmt, tests, migration check). **Commit:** `d8ec71e` — `feat: drive API with agent principals` (branch `worktree-slice-1`, not pushed). **Tests:** 14 new in `internal/slidestest/{agents,api}_test.go` — all 7 brief-named ones plus 7 more; all failed before implementation, all pass now. **Key findings / concerns:** - CSRF ruling confirmed: `csrf.SameOrigin` explicitly refuses a request with no `Sec-Fetch-Site`/`Origin`/`Referer`, so `/api/v1/` is mounted on the ServeMux ahead of the chi router, outside `csrf.Protect`, gated by the bearer alone. Explained in `api.go`, `app.go` and `docs/api.md`. - The `op` index in a 422 is real, not inferred: the Rust core already prefixes rejections with `op {i}:`, and the handler parses it back. - Error shape follows the task constraint (`{"error":"invalid","op":N,"message":…}`), not the brief's literal `{"error":"&lt;core message&gt;"}` — `error` is a fixed identifier on every status. - `actorFor` kept its `(u User)` signature; the API composes `agent:&lt;id&gt;` from the resolved principal instead of a request-scoped switch inside a function every screen calls. - Copy was **not** put through the `copy-review` gate (22 new catalog keys + `docs/api.md`) — it needs an interactive session. - The 409 conflict branch is untested (no concurrency seam in the harness); `register_asset` has no upload endpoint yet; no in-process rate limit on the API. **Report:** `/home/paulca/amadan.net/oficina/slides/.claude/worktrees/slice-1/.superpowers/sdd/2026-09-07-slice-1-live/task-7-report.md`</result> <usage><subagent_tokens>225098</subagent_tokens><tool_uses>103</tool_uses><duration_ms>1124652</duration_ms></usage> </task-notification>

<task-notification> <task-id>aff4729d477c0b906</task-id> <tool-use-id>toolu_01Lj1JT4TaaZrE5k4UdHEJMR</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina/2420c44b-a27a-4e0d-bbe7-8b3ca0464c21/tasks/aff4729d477c0b906.output</output-file> <status>completed</status> <summary>Agent "Review Task 7 (spec + quality)" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>## Spec Compliance **Verdict: compliant.** Every named risk in the review brief was checked against the actual code and passes. - **Bearer gate failure modes** (`internal/slides/agents.go:597-645` in the diff, i.e. `bearerActor`): empty header → `bearerToken` returns `""` (`presentation.go:1240-1247`, reused not duplicated) → 401 `unauthorized`; wrong prefix → `EqualFold` fails → same 401; revoked and never-issued tokens both fall through `gorm.ErrRecordNotFound` to the identical 401 (no oracle); a constant-time compare runs after the indexed hash lookup. Verified with `TestApiNeverReadsCookie` (revoked path in `TestRevokedTokenIs401`). - **HTML reachable with a bearer?** No — `/api/v1` is mounted on the raw `http.ServeMux` (`app.go`), `bearerActor` is only `Use()`d inside `apiRouter()`. `TestApiNeverReadsCookie` proves the mirror: `GET /` with a bearer and no cookie still redirects to sign-in (303), and `GET /api/v1/decks` with a valid session cookie and no bearer is 401. - **CSRF ruling.** Verified independently by reading `/home/paulca/go/pkg/mod/amadan.net/rastrillo/rastrillo@v0.26.0/csrf/csrf.go:25-45`: `SameOrigin` falls through all three checks to a bare `return false` when `Sec-Fetch-Site`, `Origin`, and `Referer` are all absent, which is exactly what a non-browser client sends. The report's claim is accurate. - **Ops id-minting before validation.** `mintOpIDs` (`api.go`) mutates the local `ops` slice in place, but the response body on a rejected batch (`apiInvalid`, called from `apiOps`'s `default` branch) only ever carries `error`/`message`/`op` — the `ids` map computed in `applyAgentOps` is discarded on every non-nil error return, so a refused batch never leaks minted ids. Confirmed against `core/src/ops.rs:1092-1121`'s two-pass check/apply split, which really does write nothing on a rejected batch. - **`If-None-Match`.** `matchesETag` (`api.go`) handles a comma-separated list, `*`, and weak (`W/`) validators per RFC 9110, checked before `openDeck` runs (skips the wasm replay entirely on a 304). - **1 MiB limit.** `readJSON` wraps `r.Body` in `http.MaxBytesReader(w, r.Body, maxAPIBody)` and unwraps `*http.MaxBytesError` via `errors.As` to answer 413; exercised by `TestApiRefusesBodiesItCannotRead`. - **`present` reuse.** `apiPresent` calls the same `livePresentations`/`createPresentation` (`presentation.go`) that `startPresenting` uses, scoped by `actor` the same way, so repeated calls re-pair rather than duplicate — matches `TestApiPresentStartsATalk`'s "same session twice" assertion. - **Actor rendering escaping.** `lastChangeBy`/`rowSub` write the badge string (`🤖 name~email`) into a plain Go `string` field consumed by `html/template` (`render.go` imports `html/template`), so it is contextually auto-escaped; no `template.HTML` cast anywhere in the diff. - Global constraints — `Agent` model shape, token format (`sl_`+40 `[A-Za-z0-9]`, `crypto/rand` with proper modulo-bias rejection, SHA-256 hex, constant-time compare), principal `&lt;name&gt;~&lt;email&gt;`, `agent:&lt;id&gt;` actor on every write, endpoints/JSON shapes, error identifiers, "Last change by" resolution, `docs/api.md` coverage, catalog-keyed copy, scriptless forms, rastrillo §3-4 scoping — all checked directly against `models.go`, `agents.go`, `api.go`, `decks.go`, `en.toml`, `agents.html`, and `docs/api.md`, and all match. - Spot-checked every `docs/api.md` curl example's route and JSON shape against the handlers and against `core/src/ops.rs`'s actual `Op` field names (`markdown`, `new_id`/`new_block_ids`, etc., not the spec's looser prose) — all accurate. ## Strengths - The CSRF-mount decision is independently correct, not just asserted — I read `csrf.go` myself and it holds. - The refused-batch/id-minting interaction is subtle and handled right: ids are written into the batch before the core's two-pass check so later ops in the same batch can reference them, but never surfaced to the caller unless the whole batch actually committed. - `applyAgentOps`'s single retry re-reads the deck row and reuses the ids already baked into `ops` rather than re-minting, so a retried batch can't hand back different ids for the same logical request. - Test coverage is broad and adversarial in the right places: cookie-vs-bearer isolation tested both directions, cross-operator visibility (`TestApiListsWhatTheOperatorCanSee`, `TestAgentsAreTheirOperatorsAlone`), duplicate_slide's multi-id minting, ETag staleness, and all three body-refusal paths (415/413/422). - `actorLabels`/`lastChangeBy` are two queries regardless of row count, not N+1, and degrade gracefully (empty label) rather than leaking a raw `person:7` on a deleted actor. - `docs/api.md` is accurate against the actual handlers, not the spec's paraphrase — it reflects the real field names (`markdown`, not `text`), which is what an implementer actually needs. ## Issues #### Critical None found. #### Important - **Copy not run through the mandatory `copy-review` gate.** 22 new catalog keys and all of `docs/api.md` shipped without the review the skill requires before user-facing language is written into a file. The implementer flagged this themselves (report §Concerns 1). The prose reads well on inspection, but it hasn't been through the required gate, so it should not be treated as final. Not a defect in the code under review, but it's a real gap that should close before this ships. #### Minor - `mintOpIDs`'s per-attempt `blocks` map is recomputed from a freshly reopened view on the 409 retry, but `new_block_ids` (once already minted on attempt 0) is not regenerated — so a `duplicate_slide` chain inside one batch that depends on a later op's block count could theoretically use a stale count if a concurrent writer changed the *source* slide's block count in the retry window. Extremely narrow (needs a genuine 409 race plus a same-batch nested `duplicate_slide`), and the report already discloses the 409 path as untested end-to-end (`api.go`'s `applyAgentOps`). Worth a follow-up test once the harness has a seam for concurrent writers, not worth blocking on. - Self-disclosed, reasonable-as-is perf notes (`api.go`'s `apiListDecks` opening every deck for a slide count; `apiGetDeck` reading the view twice) mirror pre-existing costs `decks.go`'s own list screen already pays — no new problem introduced, just inherited. - `app.go`'s comment that `/api/v1` is mounted "ahead of" the chi router implies registration order matters for `http.ServeMux` routing; it doesn't — `ServeMux` picks the most specific pattern regardless of order. Harmless (the more-specific pattern always wins here), but the comment overstates the mechanism. ## Assessment **Task quality:** Approved **Reasoning:** Every named risk in the review brief was verified against the actual code (not just the report's claims) and holds up: the CSRF-mount rationale is correct per an independent read of `csrf.go`, the bearer gate fails closed and uniformly across revoked/malformed/empty/wrong-prefix cases, minted ids never leak on a refused batch, `If-None-Match` and the 1 MiB body limit are implemented correctly, `present` correctly reuses the existing presentation machinery, and actor strings are safely auto-escaped. Tests are substantive and check the properties that matter (isolation, atomicity of refused batches, visibility scoping) rather than just happy paths. `docs/api.md` was spot-checked endpoint-by-endpoint against the handlers and the core's real op vocabulary and is accurate. The only outstanding item is procedural — the mandatory copy-review gate wasn't run — which the implementer already surfaced and which doesn't affect the correctness of the code itself.</result> <usage><subagent_tokens>147569</subagent_tokens><tool_uses>41</tool_uses><duration_ms>348319</duration_ms></usage> </task-notification>

<task-notification> <task-id>a3c00ca80f09a5f5b</task-id> <tool-use-id>toolu_01BCgxbNA497iQ5TL8D3cAhG</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina/2420c44b-a27a-4e0d-bbe7-8b3ca0464c21/tasks/a3c00ca80f09a5f5b.output</output-file> <status>completed</status> <summary>Agent "Final whole-branch review" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>I did this review myself in five passes (docs and ledger; router, auth, API and presenting; decks, share, viewer, hub and templates; catalog, tests, gate and docs; the canary and one experiment in a scratch copy under `~/.claude/tmp`, the worktree untouched). `go vet`, `gofmt`, `rastrillo migration check` and `go test ./...` are green on `d8ec71e`. ### Summary Slice 1 is a careful, well-seamed implementation: the security surfaces (allowlist oracle, hashed capabilities, missing-vs-revoked, cookie/bearer isolation, Referer redirect, CSP/Referrer-Policy on the canary) hold up under cross-cutting inspection, and the canary answers the right shapes (`/api/version` → `d8ec71e`, JSON 401 on `/api/v1`, HTML 404 on `/s/`, `/p/`, `/r/`). Two Important defects sit in the write path: the optimistic-lock retry in `applyOps` is dead code (a real conflict returns a wrapped `gorm.ErrDuplicatedKey`, not `errDeckBusy`, so the API answers 500 where `docs/api.md` promises 409, proven by a test in the scratch copy), and an agent's `set_meta` never updates the `decks` row, so the list, the API list and the settings page drift from the document. Nothing is Critical: no data is lost or exposed by either. ### Plan alignment Deviations, all recorded in the ledger and justified except where noted: - `slides.New` returns `(*http.ServeMux, *app, error)` with `Close` (ledger, Task 3). Fine. - Concern packages nested under `internal/slides/` (ruling; `migration check` needs one dir). - Sqids: plan said `sqids-go` from `ID`, min length 8; `decks.go:33-58` mints 10 random chars over Docs' alphabet. Better than the plan (unguessable, no ID leak) but **not recorded as a deviation**. Note it in the ledger. - `decks.html` has no Present/share affordances (ruling, Task 6): one entry point in the viewer. Fine. - `applyOps` signature grew a `rowUpdates` map (fix round, Task 3). Fine. - Spec §3.4 "the slide's transition" is not rendered on `/p/{session}`; `transition` is in the model only. Not mentioned anywhere; Minor. - `present.js` is 117 lines, under the plan's 120. Everything else in the file map exists where the plan put it. ### Findings #### Critical None. #### Important 1. **The write-conflict retry never fires; a real conflict is a 500, not a 409.** `internal/slides/decks.go:262-273` inserts `DeckUpdate{Seq: d.Seq+1}` *before* the `WHERE id = ? AND seq = ?` bump at `:278`. rastrillo's `db.Open` sets `TranslateError: true` (`db/db.go:97`), so under the `(deck_id, seq)` unique index the loser's insert returns `gorm.ErrDuplicatedKey`, wrapped at `:271` as `"record deck %s update %d: %w"`, and `editDeck` (`:322`) and `applyAgentOps` (`api.go:470`) see `!errors.Is(err, errDeckBusy)` and give up. Reproduced in the scratch copy: second writer got `slides: record deck … update 1: duplicated key not allowed`, `errors.Is(err, gorm.ErrDuplicatedKey) == true`, never `errDeckBusy`. Data is safe (the transaction rolls back), but two agents on one deck get a 500 with `{"error":"internal"}` and an HTML writer gets the 500 page, while `docs/api.md:65,178` promise 409. Fix: in `applyOps`, `if errors.Is(err, gorm.ErrDuplicatedKey) { return errDeckBusy }` on the insert (or do the seq bump first and insert second), and add the two-handles-one-seq test above to `decks_test.go`. 2. **An agent's `set_meta` leaves the `decks` row stale.** `api.go:396` calls `applyOps(..., nil)` with no `rowUpdates`, so `title`/`theme` in the document move but `decks.title`/`decks.theme` do not. `decks.go:456` (list), `api.go:243` (API list `title`), and `decks.go:730` (settings pre-fill) all read the row; the viewer reads `view.Meta.Title`. `models.go:64-66` says "both move together on every write" and `docs/api.md:247` documents `set_meta` as the way to set the title. Fix: after a successful batch in `applyAgentOps`, derive `rowUpdates` from the resulting view's `Meta.Title`/`Meta.Theme` (the view is already returned by `applyOps`) and fold them into the same transaction, or have `apiOps` scan the batch for `set_meta` and pass `title`/`theme` as `rowUpdates`, validating `theme` with `validTheme`. 3. **Agent tokens ride a GET URL.** `agents.go:451` redirects to `/agents?new=sl_…`, a 238-bit long-lived write credential landing in browser history, the CARLOS edge's access log, and the `Referer` of every same-origin click made from that page. The share and remote tokens are URL capabilities by spec design, so their `?new=`/`?t=` is the accepted trade; the agent token is meant for an `Authorization` header only and is worse to leak. Fix: render `agents.html` straight from the POST with a 200 (the double-submit guard in `rastrillo.js` and a `Cache-Control: no-store` cover the PRG concern), or stash the token server-side under a one-shot random nonce and redirect with the nonce. 4. **`deleteDeck` does not cascade `presentations`.** `decks.go:876-892` deletes updates, grants and share links but not `Presentation` rows. A live session on a deleted deck keeps its pairing code "taken" for 12 h (`createPresentation`, `presentation.go:340`), `liveSessionFor` still finds it, `/p/{sqid}` answers 404 from `deckOf` while `/p/{sqid}/command` still writes to the row, and the audience never hears `ended`. Fix: `tx.Where("deck_id = ?", d.ID).Delete(&amp;Presentation{})` in the same transaction, and `a.hub.Publish(sqid, Event{Ended: true})` for any live one after commit. 5. **`slides.viewer.empty.body` still says "Add one from the editor"** (`locales/en.toml:181`). Confirmed. There is no editor; an agent or the API is the way. Fix the sentence before merge since it renders on a real state. #### Minor - `createPresentation` (`presentation.go:352`) string-matches `"UNIQUE"` via `isUniqueViolation` while `createDeckRow` (`decks.go:355`) uses `gorm.ErrDuplicatedKey`; with `TranslateError` on, the driver text may not contain `UNIQUE` at all. Use `errors.Is(err, gorm.ErrDuplicatedKey)` in both. - `share.html:56-58`: the Revoke button has no accessible name naming the link, while `agents.html:52` does (`aria-label="Revoke {principal}"`). Add `RevokeAria` with the created date. - `saveDeckSettings` writes the row through `applyOps`' `WHERE id = ? AND seq = ?` (`decks.go:278`), i.e. not owner-scoped in SQL; ownership is established by `ownedDeckFor` just before. Acceptable given team decks are meant to be writable, but note it against SKILL.md §3. - `presenterNotes` shows the pairing block only for `holder &amp;&amp; presenter` (`presentation.go:735`); a session started via `POST /api/v1/decks/{sqid}/present` has `Actor = agent:N`, so a person opening `notes_url` never sees the QR/code. The JSON carries both, so it works, but say so in `docs/api.md:194-216`. - `/s/{token}` performs a write (`views + 1`, `share.go:319`) and a full wasm replay per request from an unauthenticated holder; `/p/{session}/stream` lets any sqid holder open unbounded SSE goroutines (5 min each). Bounded per request, unbounded in count; Docs made the same trade. Worth an edge rate limit note in AGENTS.md. - `readJSON` (`api.go:188`) does not reject trailing bytes after the JSON value; add `dec.Decode(&amp;struct{}{}) != io.EOF` → 422. - `PRODUCT.md:151` says "121 keys today"; `en.toml` has 142. The ledger's "off by 2" is now off by 21. - `app.go:170` comment says `/api/v1` is mounted "ahead of" the chi router; `ServeMux` picks by pattern specificity, not order (ledger already parked this). - Spec §3.4 transitions (`fade`) are not rendered on the surface; either render `data-transition` and a CSS rule or record it as slice 2. - `presentation.go:1003` `heartbeat.Reset` on an undrained timer relies on Go 1.23+ semantics; `go.mod` says 1.25, so fine, but pin the reason in a comment. **Done well:** `gateBegin`/`sameAnswer` (auth.go:120-208) is the most careful allowlist-oracle defence I have seen in the family; `visibleDecks`' parenthesised OR; hashed capabilities with constant-time compares everywhere; `presentStream` subscribing before re-reading the row; the hub dropping rather than skipping; the API mounted on the `ServeMux` outside `csrf.Protect` with `bearerActor` reading only the header (verified: `/api/v1/decks` with a cookie is 401, and no HTML route accepts an agent bearer, `presentCommand`'s bearer is the remote token); `render.Slide` filtering hrefs and always emitting `alt`; the templates carry no literal English (scanned: only the "Slides" brand, a recorded ruling), bare `rst-*` grammar with only allowed utility classes, 56 px remote targets, blackout as words; the test suite hits the real router, the SSE test is bounded by a 10 s context and there is not a `time.Sleep` anywhere. ### Deferred/parked triage Fix before merge: - Task 8 minor: `slides.viewer.empty.body` mentions an editor (Important 5 above). - Task 7 minor "409 branch untested": it is untestable because it is unreachable (Important 1). The test I wrote above is the missing one. - Task 8 minor: PRODUCT.md key count (one line). Can wait (follow-up issue each): - Copy-review of every new string by Paul (Tasks 1, 5, 6, 7): not a code defect; gate on Paul. - Task 3: `decks.go` size; `theme.invalid` integration test. Task 4: `render.go` size; `meta.aspect`; indent 2-3 untested; CSS unseen in a browser (walk the canary once signed in). - Task 5/7: hand-written `readonly` field, upstream request to rastrillo `field-text`. - Task 6: Present twice mints a fresh token (documented); two simultaneous presenters pick the newest session; `goto` out-of-range is in fact tested (`present_test.go:246`); bearer callers to `/p/*/command` must send `Origin` (documented in `api.go`). - Task 7: no API rate limit; nested `duplicate_slide` on a 409 retry (moot until Important 1 lands, then re-check `mintOpIDs` is idempotent across the retry — it is, ids are written into `ops` on attempt 0). - Task 1 minors (Sec-Fetch-Site making Origin dead in the test POST helper is already covered by `postLegacy`; `extractEnvFlag` now refuses a valueless `-env`). - Task 9: DNS CNAME for `slides.oficina.works` and the `SLIDES_ORIGIN` flip; the branch discussion and `amadan ci status` exit 0 are still owed per AGENTS.md before the squash. ### Verdict: Needs fixes Blocking: (1) map the duplicate-key insert to `errDeckBusy` so the retry and the documented 409 exist, with the conflict test; (2) sync `decks.title`/`theme` on an agent `set_meta`; (3) keep the agent token out of the GET URL; (4) cascade `presentations` on deck delete; (5) reword `slides.viewer.empty.body`. All five are small, contained changes; after them, plus the branch discussion and a green `amadan ci status`, this is ready to squash.</result> <usage><subagent_tokens>267139</subagent_tokens><tool_uses>68</tool_uses><duration_ms>475017</duration_ms></usage> </task-notification>