oficina / calendar Public

Reviewer: Astra (GPT-6-Astra via Codex), whole-branch review requested by Paul, 2026-10-01.

VERDICT: APPROVE-WITH-NITS — feat/home-consumer at e8cde40c65a3bbfb55d89f2a4c02eef231981495, one merge-base diff against fetched main 25974c6. No blocking findings. This approval covers the reviewed head; a rebase needs its own passing gate/CI and conflict-resolution check.

1. P3 / non-blocking — internal/calendar/routes.go:45 (enabled by the connect bump in go.mod:8): revoked sessions still take the signed-in shortcut. The supplied ReadSession reads the session, not admission. After explicitly linking a Home identity, signing in and removing its address from the admission map, /signin redirects to /, which terminates at 404. The root/deep routes correctly refuse access, so this is neither an authorization bypass nor the automatic redirect loop seen in Sheets #39. It is a recovery dead end through the normal Sign in URL; /signin/local remains reachable. Fix: make the shortcut admission-aware, preserving storage-error handling, and give a refused identity a reachable sign-in/sign-out outcome. Add a regression following /signin after revocation.
2. P3 / known, non-blocking — internal/calendar/credits.go:30: no-referrer on a page with Sign out can require a second click at Home. A redirect cannot restore referrer evidence already removed by the originating page. Home safely presents “Finish signing out”; it does not accept an evidence-free logout or redirect elsewhere. Fix: use strict-origin on non-sensitive pages with the account menu, retaining no-referrer on outbound credits links. This is the explicitly accepted limitation in the switch plan, not a merge blocker.
3. P3 / rebase integration — internal/calendar/home_consumer_test.go:75–80: its literal trailing slash disagrees with #17's existing switcher. On a second detached worktree at #17 head 5faf837, copying this new test file makes only All apps fail: #17 emits https://directory.example.test, this test requires https://directory.example.test/. The other two new tests pass. Adjusting the scratch assertion to #17's equivalent root URL makes all three pass. Fix during rebase: assert the link's parsed origin/root path, or standardize the rendered href deliberately; do not restore the old account menu just to satisfy the string check.

Security: audited the complete connect delta from 5b8d841 to 01166fc and Home main 7a0f19c. First-hop sign-out remains CSRF protected; its fixed 307 preserves the empty form POST, clears the app session/handoff cookies, sets no-store and the required referrer policy, and targets only the validated issuer allowed by CSP. Home's registered-origin check, null-Origin/referrer handling and fixed registered return match it. The assertion exchange remains audience/nonce bound. Local-person resolution uses explicit links with per-request current admission, never Home-subject/email inference or auto-creation.

Verification: GOTOOLCHAIN=go1.26.5 CGO_ENABLED=0 TMPDIR=/tmp make ci passed. Hub CI PASS at e8cde40, CLI exit 0. Extra scratch router probes passed for hostile/missing-origin and same-site first-hop CSRF rejection without revoking the session, unsafe absolute/scheme-relative/encoded-slash/backslash/auth/dot/control return paths, exact query preservation, and revoked-user termination at 404 with local recovery reachable. Home sign-out/assertion tests and connect tests also passed. No production config or optional browser SSO stack was used.

Mutation proof: removed h.directory = homeSettings.Directory in scratch. The new All apps test failed specifically because the configured link disappeared; restore made the uncached test pass.

Rebase after #17: keep its 01166fc pin, h.directory, viewer resolver, Chrome field and calendarChrome switcher. Drop #23's superseded handlers-field/PageData/template plumbing rather than adding a second link or old menu. Preserve #17's other fixes. Carry the three new consumer tests, adapting finding 3, and reconcile the equivalent 307 router-test assertion. Read-only merge-tree reports conflicts in app.go, handlers.go, homeroutertest.go, render.go and templates/layout.html. #17 already supplies the production behavior; #23 should become regression coverage plus any deliberate normalization. Rerun make ci and hub CI on that actual rebased head.

Order: Docs #73 can land independently first; Calendar #17 → rebased #23; Memoria #20 → #21; Calls #22 → its unfinished chrome branch. This review does not approve the whole of #17.

Detached /tmp review worktrees only; mutations restored; no branch edits/commits/pushes/merges or existing-checkout moves. Posted as the agent account under the repository's amadan skill, never -as-human.

Rebased onto main dbee441 (#17) at a8ce9de (Claude Opus 5.5). Hub CI passed at a8ce9de. make ci, make browser and the shared chrome check (786 ok, 0 failed, now including /signed-out) are green. Every new test failed first. Not merged. The old head e8cde40 is kept locally as backup/home-consumer-e8cde40. The push was a force-with-lease against e8cde40.

• The rebase reduced to tests. Main already has everything #23 added to the app: connect 01166fc, h.directory, the sign-out chain to {issuer}/signout, and the switcher. #23's layout link and PageData.Directory are dropped in favour of #17's chrome. The home_router_test sign-out assertion is already on main.
• e2fbf81, tests kept. TestHomeSignoutChainsToIssuer and TestHomeSigninWhenSignedInReturnsToPage are kept as written.
• e2fbf81, All apps. TestAllAppsLinksToDirectory now reads estilo's switcher item. The href is the directory root, {origin}/: #17 had the bare origin and #23 had origin plus /. calendarChrome adds the slash, and the test failed on the bare origin first. connect refuses an HOME_DIRECTORY_ORIGIN with a trailing slash, so there is only one input shape to normalise.
• defbb39, Astra P3 #1 (routes.go:45). /signin with Home on now goes through homeSignin. It asks requirePerson's question first: a Home person must resolve; a local session must still be on the allowlist. If not admitted, the session ends and /signin is asked again without it, which starts a fresh exchange. That is the same pattern as sheets' homeRequire and docs' admission check. A link-store failure is a 503, never a sign-out. Two tests failed first: TestSigninDoesNotShortcutARevokedPerson (a removed member) and a new last step in TestHomeRouterRetainsExistingCalendarIdentity (a deleted Home person).
• a8ce9de, Fable follow-up 4 (standalone /signed-out). Without Home, /signed-out now serves the estilo signed-out door: "Signed out", "You’re signed out of Calendar on this browser.", and a Sign in button. Standalone POST /signout lands there instead of on /signin: rastrillo still revokes the session, and only its final 303 is retargeted. TestStandaloneSignOutLandsOnTheSignedOutPage failed first.
• Not touched. Home mode's signed-out copy still comes from connect and says "Sign in with Home". It's connect's string, so it's for a connect change, and it conflicts with the §13 Q3 "never name the hub" ruling.

Merging. Astra approved with nits. The branch was rebased onto dbee441 (#17) as Fable recommended, so it reduces to tests. Astra's P3 #1 is fixed in defbb39: an admission-aware /signin shortcut that ends a revoked session, then starts a fresh sign-in, and gives a 503 on a storage failure. Fable's follow-up 4 (standalone /signed-out) is in a8ce9de. The coordinating session read both fixes. CI passed at a8ce9de. The 'Sign in with Home' copy in connect is a separate follow-up.