Skip to content

Stop offering a signup visibility that nothing reads - #547

Merged
richshaw merged 4 commits into
mainfrom
claude/issue-517-visibility
Oct 22, 2026
Merged

richshaw merged 4 commits into
mainfrom
claude/issue-517-visibility

Conversation

@richshaw

Copy link
Copy Markdown
Owner

Fixes #517.

The problem

signups.visibility was written but never read: every /s/[slug] page is noindex, signups aren't in the sitemap, and nothing lists them, so public, unlisted and password all behaved the same. Yet update_signup offered to change it ("public or unlisted") and reported success, the read tools echoed it back, and PATCH /api/signups/[id] stored visibility: "password" with a 200 when no password exists anywhere.

The fix

  • REST refuses it. SignupUpdateInputSchema now refuses any visibility with invalid_input (field: "visibility", message "visibility can't be changed: anyone with a signup's link can open it"), and updateSignup no longer writes the column. I made it refuse rather than drop the key silently: dropping it would still return 200 for a change that never happened, which is the bug. No screen sends visibility on update.
  • MCP stops offering and reporting it. update_signup drops it from its input and description (the comment beside organizerDisplayName now covers both), and signupCore no longer includes it, so list_signups, get_signup and the write tools' results don't report it.
  • Create is unchanged. New signups are still stored as unlisted, and the column stays (no migration).
  • docs/connect-ai-assistant.md no longer lists visibility among what an assistant can change. CHANGELOG entry under Changed.

Tests

  • src/schemas/signups.test.ts: create still defaults to unlisted; update refuses public, unlisted and password on the visibility path; an update without it parses.
  • src/services/signups.db.test.ts: updateSignup with visibility: 'password' returns invalid_input on visibility and leaves the stored row unlisted.
  • src/mcp/tools/signups-write.test.ts: replaces the password test. update_signup's schema and description no longer mention visibility, and a call that passes one reaches the service without it.
  • src/mcp/tools/read.test.ts: the list_signups summary no longer has visibility.

Ran and passed in this session: pnpm lint && pnpm typecheck && pnpm knip && pnpm test (1300 tests) and pnpm test:db (493 tests).


Generated by Claude Code

Every signup opens for anyone with its link, so public, unlisted and
password all behaved the same. PATCH /api/signups/[id] now refuses
visibility instead of storing it and returning 200, and the MCP
update_signup tool no longer offers it, nor do the read tools report it.
New signups are still stored as unlisted.

Fixes #517.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H1Tj2ePc6ZEVotL7ZZZQGx
@richshaw richshaw added pipeline:ux Changes what a person using OpenSignup sees or does pipeline:working A session is working on it right now labels Oct 21, 2026 — with Claude
@richshaw
richshaw requested a balanced review from Copilot October 21, 2026 21:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@richshaw richshaw added pipeline:tophat pipeline:working A session is working on it right now and removed pipeline:working A session is working on it right now labels Oct 21, 2026 — with Claude

Copy link
Copy Markdown
Owner Author

Tophat: passed

Ran 6f9d4f6 locally with pnpm dev, Postgres 16, Chromium at 1280×900 and 390×844. Reproduced the bug first on the base, 1d3a73c.

Check Result What I saw
Bug on base: PATCH /api/signups/[id] with {"visibility":"password"} ✅ reproduced 200, and the row stored password
Same PATCH on the PR head with password, public and unlisted ✅ 400 invalid_input, field: "visibility", message "visibility can't be changed: anyone with a signup's link can open it"
PATCH with title plus visibility together ✅ 400, and the title wasn't saved (the row kept the old title and unlisted)
PATCH with title only, and with settings: {sendReminders:false} (what the Build tab sends) ✅ Both 200 and saved
Create a signup ✅ Stored as unlisted, as before
Organizer flow in the browser: new signup, add a slot in the Build tab, publish, open the public page as a visitor ✅ Every step worked, no console errors, no errors in the server log
Phone width: organizer dashboard ✅ Renders fine, no console errors
MCP: update_signup schema and description, read tools' results ✅ No screen sends visibility (checked with grep). pnpm test src/schemas/signups.test.ts src/mcp/tools passed 74 tests, and the DB tests for src/services/signups.db.test.ts plus src/mcp passed 81. I didn't call tools/call with a live bearer token.

For the owner (UX): nothing changes on any screen. What changes is what callers see. An API call or AI assistant that tries to change a signup's visibility now gets a clear refusal, where before it got a success that did nothing. update_signup no longer offers visibility, and list_signups/get_signup no longer report it. One thing to note: the REST GET /api/signups/[id] still returns visibility: "unlisted", and the PR leaves that alone on purpose. The error also carries received: "string", expected: "undefined" from the zod parse, which reads a little odd next to the message but is harmless. The screenshots only show that the flows beside the change still work: builder, publish, public page and the phone dashboard.

Screenshots are in https://claude.ai/code/session_01QZ2HeVDg9JFNBm9LAbvCEQ.


Generated by Claude Code

@richshaw richshaw added pipeline:review pipeline:working A session is working on it right now and removed pipeline:tophat pipeline:working A session is working on it right now labels Oct 21, 2026 — with Claude

@richshaw richshaw left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: passed (round 1)

The PR stops offering a signup visibility that nothing reads, and it does that cleanly. SignupUpdateInputSchema now refuses any visibility with invalid_input on that path. updateSignup no longer writes the column. update_signup drops the key from both its input and its description, and signupCore no longer reports it, so list_signups, get_signup and the write tools' results all lose it in one place. The docs and CHANGELOG are updated, and the CHANGELOG tells script authors what to change. I grepped src: nothing on any screen sends visibility on update, and the only writers left are create paths that hard-code unlisted (new-signup action, Magic Compose persist, MCP fixtures). Every item in the issue's "Done when" is covered, with tests on the schema, the service (DB) and the MCP tool.

Blocking (0)
Optional (1):

  • src/schemas/signups.ts:92: SignupCreateInputSchema still accepts visibility: "password", so POST /api/signups can still store a password visibility that has no password. The PR leaves create alone on purpose, and nothing reads the column, so nothing is exposed. If you want create to match, a later change could narrow it to unlisted.

Earlier threads: none. Copilot couldn't review because of its quota.
Checks: CI is green on 6f9d4f6 (lint-type-test, e2e, CodeQL). The tophat passed on this same SHA, so no runtime code has changed since. I ran nothing locally beyond the grep, because CI on this SHA covers lint, types, knip, unit and DB tests.

Why the owner: this is UX (pipeline:ux), but no screen changes. What changes is what API and AI-assistant callers see. A PATCH or update_signup that tries to change visibility now gets a clear refusal, where before it got a success that did nothing, and the read tools no longer report visibility. The tophat comment has the request and response details. The REST GET /api/signups/[id] still returns visibility, on purpose.

https://claude.ai/code/session_01L2LvNSAMut42zeFoyLbWxg


Generated by Claude Code

@richshaw richshaw added pipeline:owner Passed; waits for the owner (UX or high-risk) and removed pipeline:review pipeline:working A session is working on it right now labels Oct 21, 2026 — with Claude
@richshaw richshaw added pipeline:merge and removed pipeline:ux Changes what a person using OpenSignup sees or does pipeline:owner Passed; waits for the owner (UX or high-risk) labels Oct 21, 2026

Copy link
Copy Markdown
Owner Author

Approved by the owner at 6f9d4f6.

The branch is behind main, so I updated it. It merges once CI is green on the new head.

https://claude.ai/code/session_0115MCPHsyBah5fBHmYV2aVR


Generated by Claude Code

@richshaw richshaw added the pipeline:working A session is working on it right now label Oct 21, 2026 — with Claude
@richshaw richshaw removed the pipeline:working A session is working on it right now label Oct 21, 2026 — with Claude
@richshaw richshaw added pipeline:working A session is working on it right now pipeline:fix and removed pipeline:merge pipeline:working A session is working on it right now labels Oct 22, 2026 — with Claude

Copy link
Copy Markdown
Owner Author

Not merged. After #541, #542 and #546 landed, this PR conflicts with main, so it goes to pipeline:fix for a merge of main. Every other gate passed on 53af790. The owner's approval at 6f9d4f6 carries over as long as that merge is the only new commit.

https://claude.ai/code/session_017ozNhwhSPGb1Aq4KLbjyGh


Generated by Claude Code

@richshaw richshaw added the pipeline:working A session is working on it right now label Oct 22, 2026 — with Claude

Copy link
Copy Markdown
Owner Author

Merged main into the branch in e8353a6. The only conflict was in CHANGELOG, where #541's entry and this PR's entry landed on the same line. I kept both, this PR's first. No runtime code changed.

  • Ran in this session: pnpm lint && pnpm typecheck && pnpm knip && pnpm test (1311 passed) and pnpm test:db (all 34 files, 501 tests).
  • CI is green on e8353a6: lint-type-test, e2e and CodeQL.
  • The PR has no commits of its own since the owner approved 6f9d4f6, only merges from main. The approval and the passing review and tophat still stand, so this goes straight back to pipeline:merge.

https://claude.ai/code/session_01My1XhNGLuKW2vitjpCKAzH


Generated by Claude Code

@richshaw richshaw added pipeline:merge pipeline:working A session is working on it right now and removed pipeline:fix pipeline:working A session is working on it right now labels Oct 22, 2026 — with Claude
@richshaw
richshaw merged commit 15deb24 into main Oct 22, 2026
5 checks passed
@richshaw
richshaw deleted the claude/issue-517-visibility branch October 22, 2026 02:53
@richshaw richshaw removed pipeline:merge pipeline:working A session is working on it right now labels Oct 22, 2026 — with Claude
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

update_signup offers to change a signup's visibility, which nothing reads, and the API accepts "password" with no password

3 participants