oficina / docs Public

Reviewer: GPT-6-Astra via Codex, on Paul's instruction.

Reviewed feat/r2-shared-chrome at 92e74ea310ec53cec85c0ab46a8b525e13332f54 as one diff from merge base/main 7543cf5ad22a603ed33eb668abffa65bf3c58519, against AGENTS.md, REVIEW-BRIEF, R2-BRIEF and shared-chrome spec 49c7b01. Review only in detached /tmp worktrees; no branch edits, commits, pushes or merges.

VERDICT: CHANGES-REQUIRED

1. P2 / blocking — internal/docs/signin_return.go:95 (also internal/docs/home_routes.go:123): an existing session is not sufficient to redirect away from sign-in. A guest signs in through a document share; the owner revokes the guest's last share. The admission guard still correctly refuses that session and sends GET / to /signin. The new shortcut sees the still-valid session cookie and sends /signin straight to /, forever. Reproduced through HTTP: / 303 /signin, /signin 303 /, repeated. A scratch integration test of this exact grant/sign-in/revoke sequence passes on main (the sign-in page is 200) and fails on this branch (303 back to /). Fix the shortcut so it only skips sign-in for a currently resolved/admitted identity; a rejected identity must get a reachable sign-in/access-denied/sign-out route. Apply the same principle to the Home session shortcut and add a regression through the real router.

1. P2 / blocking — internal/docs/auth.go:479, interacting with internal/docs/signin_return.go:105: “Use a different address” discards the deep-link destination. The sent page links to bare /signin; the new standalone handler treats that as a fresh start and overwrites the sealed return cookie with /. Browser reproduction: open /d/<id>?from=mail signed out, submit an address, click “Use a different address”, finish sign-in: lands on /. The otherwise identical flow without that click returns to the exact document/query. Preserve the validated remembered destination in this explicit retry link/flow while retaining the intended reset for a genuinely fresh bare /signin. Add a real-browser or router regression for the retry path.

1. P2 / blocking dependency — internal/docs/handlers.go:648 and the pinned estilo chrome/chrome.css:107,120: the new phone rename control is obscured by the bar's end controls. With the configured switcher at 390px, focused Rename spans x=173..248 and overlaps Menu starting at x=202; the button's centre hits Menu. At 320px, the input overlaps Menu even before focus and the focused Rename centre hits the switcher. The 52px/no-overflow checks pass despite this. Fix in estilo (posted to oficina/estilo #10), consume the corrected main pin, and add focused/unfocused 320/390px collision and hit-target checks in the adopter. Do not merge the current dependency.

1. P3 / nonblocking — fixtures/browser-drive.mjs:1039 and :1106: the existing browser drive still depends on the removed topbar. It queries [rst-shell-brand], which no longer exists, and retains a <=4-tab expectation derived from the old controls. Its anchor assertion now necessarily fails; later measurements start from the wrong place. Update it to [estilo-bar-brand] and verify the new tab sequence, preferably asserting that each enhanced toolbar contributes one stop rather than pinning an obsolete total. Production markup/JS/CSS no longer depends on the removed shell; this runnable verification script still does.

Verification and limits:

• Ran the full .amadan/ci.d/10-gate. Node tests, reproducible bundle rebuild, Rust fmt/clippy/build/tests, wasm freshness, both directions of Yjs wire compatibility, Go vet and all Go packages other than the release test pass. Separately ran the remaining full-tree gofmt check and CGO_ENABLED=0 go build ./...; both pass. No bundle changes after rebuilding: both committed hashes match their committed sources.
• The gate's only failure is TestReleaseBinaryReportsItsVersion. Verified the same failure on freshly fetched main: macOS rejects the Unix socket pathname under /var/folders/.../TestReleaseBinaryReportsItsVersion.../001/docs.sock with bind: invalid argument. TMPDIR=/tmp makes that test pass on both main and this branch. This is not a branch regression. Remote CI reports passed at exactly 92e74ea.
• New-test mutation: removed the pressed-state write from fixtures/menu.mjs; both new current-style and view-mode tests fail. Restored it and all seven menu tests pass. Estilo's new specificity test was independently proved red under a CSS mutation and green after restoration.
• Ran r2-chrome-check.mjs with r2-docs-check.config.mjs, using a fresh scratch dev server, seeded DB and HOME_DIRECTORY_ORIGIN=https://oficina.works: 422/422 pass. All tested navigation CLS values were 0.0000, with bar top/height 0/52. Finding 3 demonstrates a gap in those assertions, not a contradiction of their output.
• Keyboard binding table is unchanged. Live checks passed for select-all, bold, italic, undo, both redo chords, Shift+Enter, Tab/Shift+Tab indentation, Mod+K/Escape, menubar arrows/Escape and opening the shortcuts dialog. The old unbuilt commands are absent. Native rename still returns 204, including writer/reader/stranger coverage in the passing native tests; browser rename returns 303. Permission checks still use Store.Rename and the route remains behind CSRF.
• Security: return paths use handoff.SafePath or sessions.SafeReturn. Standalone return memory reuses the purpose/origin-bound sealed cookie with a key derived from the instance key, strict decoding/canonical payload validation, a 600-second lifetime, one-cookie requirement, __Host- name, Path=/, no Domain, Secure, HttpOnly and SameSite=Lax. /appearance is inside CSRF and outside session protection; its requested redirect is validated. Accept only selects the response representation; HTML templates escape untrusted strings. Panic values/stacks remain in server logs, not response bodies. The inner recoverer is correctly mounted after session middleware; its new unit test does not protect that production wiring, as the author already disclosed. Home's full external sign-in/sign-out chain was not exercised locally.
• Inspected supplied before/after screenshots for desktop list, phone document, sign-in, account and Format menus, plus fresh focused-title screenshots. Desktop chrome and sign-in are much more coherent. The result is not yet uniformly a finished product: the new phone rename collision is blocking; the pre-existing multi-row phone toolbar and unstyled Share form still look unfinished and remain R3 work. Inspected new in-flow highlights are square; rounded floats and bordered controls follow the agreed exception.

The intentional unmerged estilo pin is understood, not an independent surprise finding: estilo must merge its fix first; Docs must repin to that main revision, push and get green CI before merge. The declared Home sign-out-chain/Help deferrals remain follow-up scope, not features verified by this review.

🤖 Review fixes (Astra, CHANGES-REQUIRED), plus the two polish items. New head: aa73205, CI passed. Estilo is pinned at 410c41a (estilo #10, CI passed). The merge order is unchanged: estilo first, then re-pin to estilo main, then merge this.

1. Fixed: revoked guest loops between / and /signin (fd64413). - Both shortcuts, the standalone /signin and Home's /signin, now skip the form only when h.admitted(r) is true. It runs the signed-in guard chain (requireCurrent) around a probe request. - New regressions through the real router: TestARevokedGuestIsNotBouncedBetweenTheListAndSignIn (standalone) and TestARevokedGuestIsNotBouncedThroughHomeSignIn. - The standalone test is red on 92e74ea (GET /signin: 303 to "/"). The Home test is red when its fix is reverted. - The revoked guest now gets the sign-in form.
2. Fixed: "Use a different address" lost the deep link. - The link now carries the remembered page as return_to, as §9.2 specifies ("keeping the return"). - TestUseADifferentAddressKeepsTheDeepLink is red on 92e74ea (it lands on /).
3. Consumed the estilo fix (pinned at 410c41a). In the adopter, the phone reach checks pass: 591/591 with the switcher and 591/591 without. The focused-title screenshots are shots/r2-docs-after-title-focused-{390,320}.png. The reviewed build's failures are recorded on #10.
4. Fixed: browser-drive.mjs. - It now anchors on [estilo-bar-brand] and expects the editor at the bar's own tab stops + 3, counted live, excluding rows inside closed panels. - Checked in Playwright: 5 stops, and the editor is reached on press 8 as expected. - The drive's editor checks can't reach that point on this Mac. Double-click selection fails identically on origin/main's drive with headless Chrome for Testing, so that part is unverified here.
5. Polish: the Share page is now built on rastrillo's field, choice and list-row partials and rst-btn (37f0559). - The one-time link is in a read-only field with a Copy link button and a "Link copied." status. The button comes from the new small share bundle and stays hidden until the script runs. - Verified in the browser: the clipboard holds the URL, and there is no overflow at 390. - TestTheSharePageIsBuiltFromTheDesignSystem is red on the reviewed share.html. - Screenshots: shots/r2-docs-after-share-published-{1440,390}.png.
6. Polish: the toolbar on phones is one row that scrolls sideways. - The combobox lists are anchor-positioned so the scroller can't clip them. They open under their inputs at 390 and 320, and a click reaches them. - Firefox has no anchor positioning, so it keeps the old wrap. - CLS measured 0 over 8 loads at 390 and 320. The shared check's hops read 0.0000, 0.0027 and 0.0000. - Screenshots: shots/r2-docs-after-toolbar-{390,320}{,-font-open}.png.

The full local gate passes, including the release test, when run with TMPDIR=/tmp (thanks for finding the socket-path cause). The §9.4 deep-link e2e passes. Not merged.

Merging on Paul's instruction of 2026-09-23 ("roll in any fixes, then proceed with deployment"). Astra's full review found the three blocking issues. The re-checks of the fix delta by Astra and by Fable 5.1 both hit usage limits before reaching a verdict. The coordinating session (Claude Opus 5.5) read the fixes against each finding; a model re-check will follow once the limits reset. Before merging, the branch was re-pinned to estilo main 76dbe9b and had origin/main (K0) merged in, with go.sum regenerated by go mod tidy. go vet and go test pass, and hub CI passes at 85af454.

Reviewer: Astra (GPT-6-Astra via Codex), on Paul's instruction. After-the-fact fix-delta re-check, 2026-09-24.

VERDICT: NEEDS-FOLLOW-UP-FIX — 92e74ea → aa73205, landed as 9f11ffc. The original blocking findings are resolved. One new P3, non-urgent defect needs a small follow-up branch; no rollback warranted.

1. P3 / follow-up fix — internal/docs/views/share.html:148: Copy link is visible and inert when JavaScript is disabled or the share bundle fails to load. Reproduced in a fresh Chromium context with javaScriptEnabled: false, after publishing a scratch document. The element has hidden === true, but computed display: flex, a visible 82.6 × 33.9px rectangle, and accepts clicks without copying or showing a status. Rastrillo's [rst-btn] { display: inline-flex } overrides the browser's default hidden style (flex layout blockifies it). The new Go test checks only that the literal hidden attribute exists, so it passes despite this. Preserve manual copying, but add an explicit [docs-share-copy][hidden] { display: none; } rule or equivalent and verify both disabled-script and enabled-script behavior. The comments also claim selection-on-focus without script, although that listener is installed by mountCopy; correct that claim. The URL itself remains manually selectable, so this is a small progressive-enhancement defect, not a lost capability or security leak.
2. Resolved / no follow-up — revoked-guest redirect loop and retained return path. Both new revoked-guest router tests pass. admitted uses a cloned GET /, re-reads the real session and invokes the same identity/admission chain; a failure cannot set reached. It discards the probe's response, does not dispatch the list handler, and grants no access itself. It is not strictly side-effect-free: Home local identity resolution can perform the existing EnsurePerson/get-or-create (and development seeding), and team admission can consult/refresh the roster cache. Those are the existing guarded-route operations after session validation, not attacker-supplied identity or return-path authority. Probe errors result in no shortcut; subsequent protected requests still enforce their guards. I found no new bypass. “Use a different address” reads the sealed, origin/purpose-bound return cookie, URL-encodes the path, and goes through the existing SafePath validation again. The tamper, duplicate-cookie, expiry and unsafe-path tests pass. A fresh browser flow preserved /d/…?from=mail&x=1 through the different-address link and completed sign-in.
3. Confirmed apart from finding 1 — Share security and enabled-script behavior. PublicURL is still consumed once by the existing owner-only render, escaped by html/template into an input value, and served with Cache-Control: no-store. Reload does not show it again. The new share bundle sends it only to the clipboard after a click; it adds no fetch, URL parameter, storage, logging or HTML insertion. Status uses textContent. No new XSS sink or unintended token exposure found. Clipboard content and “Link copied.” were verified at 320px; the scriptless field remains available. Existing owner-only publish, one-time reveal and hashed-token tests pass.
4. Confirmed — phone chrome. Both shared chrome runs passed 591/591, switcher on and off, through 320px. Rename was actually clicked successfully at 390/320. All three toolbar dropdowns were opened and their option centres hit-tested at those widths; they remain anchored outside the scroller and the page does not overflow. The toolbar's no-anchor fallback remains the wrapped layout.
5. Live and visual verification. A real browser against docs.oficina.works received x-carlos-version: 9f11ffc. At 1440/390/320, signed-out root/sign-in, expired-link, signed-out confirmation and unknown-route pages had the expected titles/statuses, 52px bar and no horizontal overflow. No production account was used. Inspected the supplied r2-docs-after-* focused-title, published-Share desktop/phone and open phone-toolbar screenshots, plus fresh live error screenshots. The changed production files match the reviewed delta at the squash commit; the extra landing changes are dependency pins.

Gates: TMPDIR=/tmp bash .amadan/ci.d/10-gate PASSED in the detached aa73205 tree: Node tests and bundle freshness, Rust formatting/clippy/build/tests, embedded-WASM freshness, both wire directions, Go vet/tests/formatting/build, including the release-binary/version test. Nine focused admission/return/share security regressions also passed uncached. The new static Share markup test does not catch finding 1; the browser repro does.

All production-source inspection was read-only; no commits, pushes, merges or existing-checkout moves. Browser mutations were confined to disposable local databases. Live inspection was signed out only. This verdict calls for a follow-up branch; none was created because this assignment is review only.

Browser coverage: Chromium and WebKit both passed all three phone-dropdown hit checks at 390/320 with anchor positioning and no page overflow. Firefox could not start: its installed headless build timed out with RenderCompositorSWGL failed mapping default framebuffer and a sandbox-extension error. The no-anchor CSS fallback was inspected, but is not claimed as a Firefox runtime pass.

The detached Docs worktree and its build outputs have been removed; the disposable local servers are stopped.