amadan
Codex review of the spec: brief and findings, unedited
GPT-6 via Codex read the design cold at 6c757a4, the review pass §12 deferred until a reviewer could have a running instance. Six P1 and nine P2 findings, four of them reproduced with experiments: a presigned PUT can overwrite an already-verified asset; registering a known ref grants cross-deck read; an unknown text mark passes the write gate and then deletes text in the editor; layout conversion destroys concurrent edits; following a presentation outlives share-link revocation; removing an address leaves its sessions and agent tokens live. Committed unedited, per the house convention: the next spec revision marks each accepted finding (codex N) and says why it declined the rest.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 files changed,
+159
−0
CODEX-FINDINGS-SPEC.md+67 −0CODEX-REVIEW-BRIEF-SPEC.md+92 −0
Intent
Human
<task-notification> <task-id>belctn4sg</task-id> <tool-use-id>toolu_01J6rgmttgrkXXemn4tUSdhP</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina/c8f62d65-b2d1-4a65-b79e-6a8e82e1e5c6/tasks/belctn4sg.output</output-file> <status>completed</status> <summary>Background command "Run the Codex spec review (needs the sandbox off to write under ~/.codex)" completed (exit code 0)</summary> </task-notification>
Human
<task-notification> <task-id>a48192ac6313a5146</task-id> <tool-use-id>toolu_01M68DTQw3S5SNBXCz8DVfD6</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina/c8f62d65-b2d1-4a65-b79e-6a8e82e1e5c6/tasks/a48192ac6313a5146.output</output-file> <status>completed</status> <summary>Agent "Finish S3 Task 5" 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:** done — the prior implementer's tree was complete and correct; I verified it, added the one missing spec clause (§4.2: live surfaces stay HTML), and committed. **Commit:** `eca23fd feat: PDF export job, screens and API; thumbnails documented; warnings carry slide ids; wasm memory limits; WebP` (one commit, staged by name; `.superpowers/`, `releases/`, the controller's `CODEX-*.md` left untracked; not pushed). **Tests:** `node --test` 138 pass; `make ci` green; browser drive fails on `socket()` in the sandbox, green twice with the sandbox off (one earlier run during Rust compilation flaked on a browser wait — not reproducible, worth a controller re-run). **Gate:** full `10-gate` on a clean `git archive HEAD` export: `gate: ok`, exit 0 (both wasm artifacts match fresh builds; vectors and bundle unchanged). **Concerns:** (1) `blobs.Store.Put` is content-addressed, so there is no `exports/<sqid>/<job>.pdf` key — the job stores by hash and sweeps orphans; (2) held-bytes sweep is lazy (on access), caps bound memory; (3) status page uses `jobs.Get` (10-min TTL) while `.pdf` uses the app's own hour-long record, so the page can say "expired" while the file still downloads; (4) `pdftotext` absent here, that assertion skipped visibly; (5) release/deploy/push/discussion not done per instructions. **Report:** `/home/paulca/amadan.net/oficina/slides/.claude/worktrees/slice-1/.superpowers/sdd/2026-09-08-slice-3-export/task-5-report.md`</result> <usage><subagent_tokens>149615</subagent_tokens><tool_uses>44</tool_uses><duration_ms>646524</duration_ms></usage> </task-notification>
Attributed to this commit: 38.5 g CO₂e (4746493 tokens). This is an attribution, not a measurement — usage counted from this commit's own marker forward to the next one on slice-3-export, never backward from it. A turn that produced more than one commit books all of its tokens to the last one, and a concurrent session on this branch can add to the same figure. Some of it was priced using the shared default rate, because no model-specific factor row exists yet for at least one model involved. The Carbon view has the repo total, which is never derived from this attribution.
diff --git a/CODEX-FINDINGS-SPEC.md b/CODEX-FINDINGS-SPEC.md| new file mode 100644 |
| index 0000000..956b5df |
| --- /dev/null |
| +++ b/CODEX-FINDINGS-SPEC.md |
| @@ -0,0 +1,67 @@ |
| +Reviewed **`6c757a4`**. All source locations below refer to that commit. The worktree advanced to `eca23fd` during the review; its changes are outside this assessment. |
| + |
| +1. **P1 — Uploaded assets remain writable after verification.** |
| + **§2.7; `internal/slides/assets.go:176`, `:186`, `:367`; `internal/slides/thumbs.go:244`.** A member can request a PUT URL for a client-chosen ref, including an already registered object. Registration records verification, but subsequent thumbnail reads trust that record and read the object again. The pinned store implementation signs an unconditional PUT with an unsigned payload (`rastrillo@v0.26.0/blobs/s3.go:194`). Consequently, replacement bytes can invalidate verification and alter pictures in other decks without a document update. Registration also does not perform the rehash promised by its comment. |
| + **Instead:** upload into isolated staging keys, verify and hash the bytes, then promote them into immutable final objects. Never issue browser PUT capabilities for verified objects. I verified the source path, not a live bucket exploit. |
| + |
| +2. **P1 — Registration turns knowledge of an asset ref into permission to read it.** |
| + **§2.7, §5; `internal/slides/assets.go:279`, `:315`, `:367`, `:512`.** Registration checks access to the destination deck, then fetches any supplied ref and creates that deck’s verification record. It requires neither an upload receipt nor access to a source deck. A member who knows another deck’s image ref can register it in their own deck and satisfy both serving checks. This defeats the stated rule that knowing a content address is insufficient authorization. |
| + **Instead:** require a completed upload tied to the requesting principal and deck, or an explicitly authorized copy from a readable source deck. |
| + |
| +3. **P1 — The write gate validates the server’s projection, not the document the editor consumes.** |
| + **§2.6, §3.3; `internal/slides/live.go:331`; `internal/slides/deck/deck.go:173`; `core/src/schema.rs:203`; `fixtures/stage.mjs:174`.** An unknown formatting attribute disappears from the Rust JSON view, so parsing that view succeeds. The underlying Yjs document still contains it. ProseMirror encounters the unknown mark and deletes the affected `XmlText`; the installed, lockfile-pinned `y-prosemirror` 1.3.7 does this at `src/plugins/sync-plugin.js:839`. |
| + **Reproduced:** the committed wasm accepted text marked `unknown_mark`, returned the words normally, and ProseMirror’s conversion deleted those words. |
| + **Instead:** validate the actual shared text structure and marks before persistence, and make browser handling of unsupported content non-destructive. Keep the gate before the transaction; strengthen what it checks. |
| + |
| +4. **P1 — Layout conversion loses concurrent text edits.** |
| + **§2.3, §3.3; `core/src/layout.rs:109`; `fixtures/layout.mjs:103`.** When canvas-to-flow conversion changes reading order, it clones every block, deletes the original array contents, and inserts the copies. Concurrent text edits address the original shared objects, which have been deleted. This is content loss beyond the disclosed loss of placement. |
| + **Reproduced:** one client converted two out-of-order canvas blocks while another appended text. After exchanging updates, both clients lost the appended text. |
| + **Instead:** keep block objects stable and store ordering separately, such as ordered IDs referencing shared block maps. Add concurrent conversion/edit tests across both implementations. |
| + |
| +5. **P1 — Following a presentation escapes share-link revocation.** |
| + **§3.4, §5; `internal/slides/share.go:342`; `internal/slides/presentation.go:568`, `:181`; `internal/slides/static/present.js:89`.** Following a share link exposes the presentation session ID and uses `/p/{session}` for subsequent reads. Those reads require only a live presentation, not the original share token. Revoking the share link therefore leaves the recipient able to fetch fresh deck content until the presentation ends or expires. There is also a permission mismatch: members may create these public presentation capabilities, while ordinary share-link creation is owner-only (`internal/slides/app.go:138`). |
| + **Instead:** keep audience requests behind their originating share capability, including streams, fragments, thumbnails and assets. Decide explicitly who may publish an independently usable presentation link. |
| + |
| +6. **P1 — Removing a person from the allowlist leaves their existing authority active.** |
| + **§6.1, §7.1; `internal/slides/auth.go:257`, `:357`; `internal/slides/agents.go:184`, `:204`; `internal/slides/decks.go:164`.** The allowlist gates session creation. Existing sessions resolve their user row without checking current admission, and agent authentication requires only an unrevoked token and an existing operator row. The pinned `auth.RequireSession` also performs no admission recheck (`rastrillo@v0.26.0/auth/handlers.go:204`). Removing an address therefore does not remove its existing access, including agent writes to team decks. |
| + **Instead:** define offboarding in the lone-instance design and enforce current membership for sessions and agent operators. Explicitly address outstanding presentation capabilities as part of that lifecycle. |
| + |
| +7. **P2 — The “block tree” permits cycles and multiple parents.** |
| + **§0.1, §2.4; `core/src/ops.rs:605`; `fixtures/ops-model.mjs:62`; `internal/slides/render/render.go:392`, `:432`.** Child validation rejects self-reference and missing IDs, but permits longer cycles and shared children. Rendering first marks every referenced block as claimed, then starts only from unclaimed blocks. A cycle can therefore make its entire connected content disappear without producing an error. |
| + **Reproduced:** both JS operations and the committed Rust wasm accepted `A → B`, `B → A`, with a text child under A. The rendering traversal has no root from which to reach them. |
| + **Instead:** enforce unique IDs, one parent and acyclicity on accepted merged documents, with a deterministic policy for concurrent conflicting assignments. |
| + |
| +8. **P2 — Flow conversion does not preserve the template’s placement, and round trips discard regions.** |
| + **§2.3; `core/src/layout.rs:29`, `:51`, `:91`; `internal/slides/static/slides.css:299`, `:318`, `:348`.** Flow-to-canvas uses one fixed region table without consulting the template or measuring content. A centred title and a full-frame image therefore cannot receive their existing boxes. Canvas-to-flow never assigns `aside`, so a two-column slide loses that region after a round trip. |
| + **Instead:** obtain conversion geometry from the actual template layout. Specify which placement information survives repeated switching, and preserve recoverable placement metadata where practical. A generic “lossy” warning does not explain these concrete changes. |
| + |
| +9. **P2 — The renderer and stylesheet disagree about when the phone layer applies.** |
| + **§2.3, §2.5, §3.2; `internal/slides/viewer.go:160`; `internal/slides/render/render.go:138`; `internal/slides/static/slides.css:647`.** Canvas reflow requires an explicit Phone render, but ordinary viewer URLs default to Desktop even when opened on a phone. Meanwhile, CSS applies phone overrides to any slide container below 600px, including a narrow desktop editor stage. The same rule enlarges canvas text with minimum font sizes while retaining fractional boxes, defeating scale-to-fit geometry. |
| + **Instead:** define one effective viewport policy shared by HTML and CSS. Preserve explicit desktop previews, make ordinary phone viewing honour canvas reflow, and separate reflow typography from scaled-canvas typography. These conclusions are source-derived; I did not run browser layout checks. |
| + |
| +10. **P2 — Phone reading order is visual only.** |
| + **§2.5, §8; `internal/slides/render/render.go:464`, `:601`; `internal/slides/static/slides.css:711`.** Phone order becomes CSS `order`, leaving document and keyboard order unchanged. Readers using assistive technology can consequently encounter a different sequence from sighted readers. Canvas reflow also starts with stacking order, so bringing an object forward can change its reading position. |
| + **Instead:** distinguish semantic reading order from stacking order and render the effective phone sequence into the document structure. Test ordered text and links with keyboard and assistive-technology navigation. |
| + |
| +11. **P2 — Supported content has materially different meanings across renderers.** |
| + **§0.1, §2.4, §2.7, §4.** Multiple renderers do not themselves create a second deck, but the supported vocabulary needs a stronger common contract. At this commit, WebP is accepted (`internal/slides/assets.go:427`) but omitted by the SVG inline allowlist (`internal/slides/svg/svg.go:309`) and lacks an export decoder (`internal/slides/export.go:29`, `:248`). Nested SVG images explicitly ignore `fit` (`internal/slides/svg/svg.go:354`). Arbitrary font stacks reach HTML while layout substitutes embedded faces (`internal/slides/render/render.go:342`; `internal/slides/layout/layout.go:89`). |
| + **Instead:** normalize accepted images into formats every surface supports, honour the same fitting rules, and offer fonts the exporter can reproduce. Add cross-surface checks for these cases; separate golden files do not establish agreement. |
| + |
| +12. **P2 — Finished-PDF limits do not bound export’s working memory.** |
| + **§4.3; slice-3 global constraints; `internal/slides/export.go:120`, `:165`; `internal/slides/pdfcore/pdfcore.go:181`, `:239`.** Export retains all SVG pages and cached image bytes before conversion. The PDF host creates another framed copy and copies it into wasm. At this commit neither wasm runtime sets an application memory limit (`internal/slides/pdfcore/pdfcore.go:105`; `internal/slides/core/core.go:135`). A cap applied to the finished PDF cannot contain these earlier allocations or concurrent jobs. |
| + **Instead:** set guest limits, bound aggregate intermediate bytes and concurrent exports, and budget decoding and notes generation. Measure image-heavy decks and simultaneous jobs, not only text-only throughput. I did not measure peak memory. |
| + |
| +13. **P2 — Out-of-order slide fetches leave the projector permanently behind.** |
| + **§3.4; `internal/slides/static/present.js:76`.** The script records the announced slide before its fetch succeeds, then installs every response without checking whether a newer request superseded it. If slide 3 returns before slide 2, slide 2 overwrites it. Another event announcing slide 3 is ignored because the recorded index already says 3. Failed fetches have the same premature-success problem. |
| + **Reproduced:** reversing two responses left the displayed content at slide 2, the recorded index at 3, and a repeated slide-3 event triggered no fetch. |
| + **Instead:** track requested and displayed state separately, discard obsolete responses, and advance displayed state only after successful installation. |
| + |
| +14. **P2 — The design does not settle what editing a deck during a presentation means.** |
| + **§3.3–§3.4; `internal/slides/presentation.go:568`, `:592`, `:1240`; `internal/slides/static/present.js:80`.** Presentation state stores an index into the current deck. Inserting or moving slides before that index changes which slide it names. Content edits at the same index do not cause the presentation script to fetch again, so existing projectors and newly opened audience pages can show different content. |
| + **Instead:** choose explicitly between presenting a pinned revision and presenting live edits. For live presentation, track stable slide IDs and document sequence, define deletion behaviour, and notify viewers when the displayed slide changes without changing position. |
| + |
| +15. **P2 — Anonymous remote commands are attributed to the session creator.** |
| + **§6.2, ruling 7; `internal/slides/presentation.go:1140`, `:1219`.** A remote command authenticates a transferable capability, but its logged actor is always `p.Actor`. The server cannot establish that the capability holder is the person or agent who created the session. The unqualified “every write names its real actor” rule therefore does not hold for these writes. |
| + **Instead:** record a remote-capability principal and its issuer separately, with an explicit exception for anonymous presentation control. If named human attribution is required, pairing must establish that identity. |
| + |
| +**Verification limits:** The focused suite passed **35 tests**. The additional reproductions used unchanged JS files and the committed core wasm. I did not run the full build gate, a live deployment, a bucket exploit, or browser/accessibility checks. At `6c757a4`, neither router contains the export job endpoints (`internal/slides/app.go:99`; `internal/slides/api.go:91`), so I could not verify the claimed one-hour result cache, its caps, or job/download authorization at that revision. I made no repository changes. |
| \ No newline at end of file |
diff --git a/CODEX-REVIEW-BRIEF-SPEC.md b/CODEX-REVIEW-BRIEF-SPEC.md| new file mode 100644 |
| index 0000000..d91e61c |
| --- /dev/null |
| +++ b/CODEX-REVIEW-BRIEF-SPEC.md |
| @@ -0,0 +1,92 @@ |
| +# Codex review brief — the Slides design, revision 1 |
| + |
| +You are GPT-6, reading as an independent second reader. Claude drafted this |
| +spec and built three slices against it; nobody else has read it. §12 of the |
| +spec says the review pass waits until a reviewer can have a running |
| +instance instead of a document. That is now true, so this is that pass. |
| + |
| +Answer as findings, not as prose. Number them. For each: the section or |
| +file, what is wrong or unsound, why it matters, and what you would do |
| +instead. Mark severity P1 (a security hole, data loss, or a ruling that |
| +cannot hold), P2 (must fix before the next slice), P3 (worth doing). |
| +Cite code by path and line when you make a claim about the code. If you |
| +cannot verify something from the repository, say so rather than assuming. |
| + |
| +## What to read |
| + |
| +Repository: `oficina/slides`, this worktree, at commit `6c757a4` (branch |
| +`slice-3-export`). Three slices are built and deployed. |
| + |
| +- The spec: `docs/superpowers/specs/2026-09-07-slides-design.md` (revision 1, |
| + thirteen rulings in §1). §4.2 and §4.3 are being amended in a commit that |
| + is in flight as you read; judge the design, not those two paragraphs' |
| + wording. |
| +- The plans, which say what each slice meant to build: |
| + `docs/superpowers/plans/2026-09-07-slice-1-live.md`, |
| + `2026-09-07-slice-2-editor.md`, `2026-09-08-slice-3-export.md`. |
| +- `AGENTS.md` — the seams, and the traps a new session must know. |
| +- The suite doctrine this app consumes by ruling number: |
| + `../../../suite/docs/superpowers/specs/2026-09-05-suite-sharing-doctrine.md` |
| + (revision 8). The spec's §0.2 lists which rulings it leans on. |
| + |
| +The code, if you want to check a claim: `internal/slides/` (the app; |
| +`auth.go`, `decks.go`, `viewer.go`, `share.go`, `presentation.go`, |
| +`live.go`, `editor.go`, `assets.go`, `thumbs.go`, `export.go`, `api.go`, |
| +`agents.go`), `internal/slides/{core,pdfcore,layout,svg,fonts,render,deck,live,present}/`, |
| +`core/` and `core/pdf/` (the two Rust crates), `fixtures/` (the browser |
| +half and the op vocabulary that must mirror `core/src/ops.rs`). |
| + |
| +## The questions I most want a cold reader on |
| + |
| +1. **The one rule** (§0.1): "a slide is one block tree, rendered many |
| + ways". Three renderers now exist — `render.Slide` (HTML), `layout` + |
| + `svg` (pictures and PDF), and ProseMirror binding into the server's |
| + HTML in the browser. Does the rule still hold, or has it been broken in |
| + practice while the words stayed? |
| +2. **The hybrid model** (§2.3, ruling 1): flow templates and a canvas |
| + layout in one block tree, with a conversion between them |
| + (`core/src/layout.rs`). Is the conversion sound, or does it lose work in |
| + a way a person will hit? What happens to a deck that is switched back |
| + and forth? |
| +3. **The phone layer** (§2.5, ruling 2): a `phone` map per block, written |
| + by the editor's Phone view, read by the renderer, never read by print. |
| + Is "the desktop layout is always the base; the phone layer only adds" |
| + a rule that survives contact with real decks? |
| +4. **Trust in the write path.** Any member may POST arbitrary Yjs bytes |
| + (`live.go`); the core applies them, the app parses the resulting view |
| + and refuses a batch that would leave the deck unrenderable. Is that gate |
| + in the right place, and is there a shape it misses? |
| +5. **Capabilities** (§5, doctrine ruling 15): a share link is a hashed |
| + read-only token; a presentation session's sqid lets anyone view, its |
| + remote token lets a holder drive, its pairing code lets a phone find it. |
| + Four capability kinds across `share.go`, `presentation.go`, `thumbs.go` |
| + and `assets.go`. Is any of them stronger than it should be, or |
| + confusable with another? |
| +6. **Agents as principals** (§7, ruling 7): tokens minted in-app by a |
| + person, `agent:<id>` recorded as the actor, the API mounted outside |
| + CSRF because a bearer caller sends no browser headers. Sound? |
| +7. **The export path** (§4.2, §4.3, ruling 13): a Go layout engine |
| + measuring with embedded Liberation faces, an SVG per slide, and |
| + `svg2pdf` in a wasm module under wazero. With no object store bound the |
| + finished PDF is held in memory for an hour, capped. Where does this |
| + break first? |
| +8. **What the spec does not say** that a second reader would expect a |
| + presentation editor's design to settle before slice 4. |
| + |
| +## What is deliberately not built |
| + |
| +Spec §9 lists it with triggers: sign-in through the suite home (it is a |
| +skeleton), charts from Sheets, video and embeds, an MCP adapter, aspect |
| +ratios other than 16:9, comments, version history. Do not report those as |
| +gaps; do report it if one of them is load-bearing for something the spec |
| +claims today. |
| + |
| +## Conventions of this house, so you can judge against them |
| + |
| +Every user-visible string is a catalog key. Every screen but the editor |
| +works with scripts off. WCAG 2.2 AA with no exemptions. One static Go |
| +binary per team on the CARLOS platform, `CGO_ENABLED=0`, SQLite, no |
| +Chromium anywhere. `fixtures/` and the Rust core are two implementations |
| +of one op vocabulary, held together by committed vectors. The gate |
| +(`.amadan/ci.d/10-gate`) rebuilds both wasm modules and the browser bundle |
| +and diffs them against what is committed. |