feat(updater): tier 1 — notify admin and pad users of available updates#7601
Open
JohnMcLear wants to merge 23 commits intoether:developfrom
Open
feat(updater): tier 1 — notify admin and pad users of available updates#7601JohnMcLear wants to merge 23 commits intoether:developfrom
JohnMcLear wants to merge 23 commits intoether:developfrom
Conversation
Four-tier opt-in self-update subsystem (off / notify / manual / auto / autonomous). GitHub Releases as source of truth; install-method auto-detection with admin override; in-process execution with supervisor restart; 60s drain + announce; auto-rollback on health-check failure with crash-loop guard. Pad-side severe/ vulnerable badge that does not leak the running version. Top-level adminEmail with escalating cadence (weekly while vulnerable, monthly while severe). Refs: docs/superpowers/specs/2026-04-25-auto-update-design.md
Bite-sized TDD task breakdown for shipping Tier 1 notify only: - VersionChecker, InstallMethodDetector, UpdatePolicy, Notifier, state modules - /admin/update/status (admin-auth) and /api/version-status (public, no version leak) - Admin UI banner + read-only update page + nav link - Pad-side severe/vulnerable footer badge - Settings: updates.* block + top-level adminEmail - Tests: vitest unit + mocha integration + Playwright admin/pad - CHANGELOG + doc/admin/updates.md PRs 2-4 (manual/auto/autonomous) get their own plans after PR 1 lands.
…eader Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
typeof null === 'object' meant {email:null} passed the old isValid check,
which would crash downstream Notifier code reading email.severeAt. Likewise,
an array would pass the typeof latest === 'object' branch. Introduce
isPlainObject helper (null-safe, Array.isArray guard) and use it for both
fields. Adds two regression tests covering the exact broken inputs.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Register expressCreateServer/shutdown hooks in ep.json and implement the boot-wiring module that detects install method, starts the polling interval and runs the notifier dedupe pass each tick. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ints Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add UpdateStatusPayload to the zustand store, a persistent UpdateBanner rendered in the App layout, a /update page showing version details and changelog, and a Bell nav link — all wired to the /admin/update/status endpoint added in Task 10. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ling - Export stateFilePath from index.ts and import it in updateStatus.ts (removes local duplicate) - Import getEpVersion from Settings.ts in both index.ts and updateStatus.ts (removes two local definitions) - Fix misleading 'backing off' log message — no backoff is implemented, just retries at next interval - Remove EMPTY_STATE_FOR_TESTS re-export from state.ts; state.test.ts now imports EMPTY_STATE directly from types.ts - Add .update-banner and .update-page CSS rules to admin/src/index.css Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
Author
|
/review |
Code Review by Qodo
1. updates.tier defaults to notify
|
Review Summary by Qodofeat(updater): Tier 1 — notify admin and pad users of available updates
WalkthroughsDescription• Implements **Tier 1 (notify)** of the four-tier auto-update design: periodic GitHub Releases polling with ETag caching writes to var/update-state.json • **Backend modules** (src/node/updater/): pure functions for semantic version comparison, GitHub API polling, install method detection, update policy evaluation, and email notification cadence/deduplication • **Admin UI**: banner on every admin page + dedicated /admin/update page displaying current/latest versions, install method, tier, and changelog • **Pad UI**: discreet footer badge (hidden when up-to-date, shows text when severely outdated or vulnerable) via public /api/version-status endpoint that never leaks version strings • **Email notifications**: optional escalating cadence (weekly while vulnerable, monthly while severely outdated) via new adminEmail setting; real SMTP transport deferred to follow-up PR • **Settings**: new updates.* block with tier, source, channel, installMethod, checkIntervalHours, githubRepo; defaults to tier: "notify" (set to "off" to disable) • **Comprehensive test coverage**: vitest unit tests for all pure modules, mocha integration tests for endpoints, Playwright tests for admin/pad UI • **Documentation**: admin guide (doc/admin/updates.md), design specification, implementation plan, and i18n (13 keys in en.json) • Tier 1 contains **no execution code**; PRs 2–4 (manual click, auto with grace window, autonomous in maintenance window) are designed and will land in subsequent releases Diagramflowchart LR
GitHub["GitHub Releases API"]
Poller["VersionChecker<br/>ETag-cached polling"]
State["update-state.json<br/>Atomic persistence"]
Notifier["Notifier<br/>Cadence & dedup"]
AdminUI["Admin UI<br/>Banner + Page"]
PadUI["Pad UI<br/>Footer badge"]
PublicAPI["GET /api/version-status<br/>No version leak"]
AdminAPI["GET /admin/update/status<br/>Full payload"]
Email["Email notifications<br/>Logged, not sent"]
GitHub -->|fetch| Poller
Poller -->|write| State
State -->|read| Notifier
Notifier -->|decide| Email
State -->|read| AdminAPI
State -->|read| PublicAPI
AdminAPI -->|fetch| AdminUI
PublicAPI -->|fetch| PadUI
File Changes1. src/node/updater/index.ts
|
Code Review by Qodo
1. updates.tier defaults to notify
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+427
to
+438
| /** | ||
| * Self-update subsystem (PR 1: tier 1 only). | ||
| * Tier "off" disables the version check entirely. Default "notify" shows a banner when behind. | ||
| */ | ||
| updates: { | ||
| tier: 'notify', | ||
| source: 'github', | ||
| channel: 'stable', | ||
| installMethod: 'auto', | ||
| checkIntervalHours: 6, | ||
| githubRepo: 'ether/etherpad', | ||
| }, |
There was a problem hiding this comment.
1. updates.tier defaults to notify 📘 Rule violation ☼ Reliability
The new updater subsystem is enabled by default via updates.tier: "notify", so existing installs get new behavior without explicit opt-in. This violates the requirement that new features be feature-flagged and disabled by default.
Agent Prompt
## Issue description
The updater feature is enabled by default because `updates.tier` defaults to `notify`, which violates the requirement that new features be disabled by default.
## Issue Context
This PR introduces a new updater subsystem (polling + UI/API surfacing). To comply with the feature-flag rule, the default configuration must keep the feature off unless explicitly enabled.
## Fix Focus Areas
- src/node/utils/Settings.ts[427-438]
- settings.json.template[178-190]
- settings.json.docker[187-200]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+29
to
+60
| export const expressCreateServer = ( | ||
| _hookName: string, | ||
| {app}: ArgsExpressType, | ||
| cb: Function, | ||
| ): void => { | ||
| // Public endpoint. Cached for 60s. Returns only an enum — no version string. | ||
| app.get('/api/version-status', async (_req: any, res: any) => { | ||
| const now = Date.now(); | ||
| if (now - badgeCache.at > BADGE_CACHE_MS) { | ||
| badgeCache = {value: await computeOutdated(), at: now}; | ||
| } | ||
| res.json({outdated: badgeCache.value}); | ||
| }); | ||
|
|
||
| // Admin-protected. webaccess.ts already gates /admin/* with admin auth. | ||
| app.get('/admin/update/status', async (_req: any, res: any) => { | ||
| const state = await loadState(stateFilePath()); | ||
| const current = getEpVersion(); | ||
| const installMethod = getDetectedInstallMethod(); | ||
| const policy = state.latest | ||
| ? evaluatePolicy({installMethod, tier: settings.updates.tier, current, latest: state.latest.version}) | ||
| : null; | ||
| res.json({ | ||
| currentVersion: current, | ||
| latest: state.latest, | ||
| lastCheckAt: state.lastCheckAt, | ||
| installMethod, | ||
| tier: settings.updates.tier, | ||
| policy, | ||
| vulnerableBelow: state.vulnerableBelow, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
2. Update status routes ignore off 📘 Rule violation ☼ Reliability
The new /api/version-status and /admin/update/status routes are always registered, so behavior changes even if updates.tier is set to off. This violates the requirement that disabling the feature flag preserves prior behavior.
Agent Prompt
## Issue description
`/api/version-status` and `/admin/update/status` are registered regardless of `settings.updates.tier`, so the updater feature cannot be fully disabled.
## Issue Context
Even if `updates.tier` is configured to `off`, the server still exposes new endpoints (a behavior change vs pre-PR). To meet the feature-flag requirement, the route-registration hook should no-op when `tier === 'off'`.
## Fix Focus Areas
- src/node/hooks/express/updateStatus.ts[29-60]
- src/ep.json[105-118]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+43
to
+59
| // Admin-protected. webaccess.ts already gates /admin/* with admin auth. | ||
| app.get('/admin/update/status', async (_req: any, res: any) => { | ||
| const state = await loadState(stateFilePath()); | ||
| const current = getEpVersion(); | ||
| const installMethod = getDetectedInstallMethod(); | ||
| const policy = state.latest | ||
| ? evaluatePolicy({installMethod, tier: settings.updates.tier, current, latest: state.latest.version}) | ||
| : null; | ||
| res.json({ | ||
| currentVersion: current, | ||
| latest: state.latest, | ||
| lastCheckAt: state.lastCheckAt, | ||
| installMethod, | ||
| tier: settings.updates.tier, | ||
| policy, | ||
| vulnerableBelow: state.vulnerableBelow, | ||
| }); |
There was a problem hiding this comment.
3. Admin status leaks version 🐞 Bug ⛨ Security
GET /admin/update/status is not guaranteed to be admin-only because webaccess only treats /admin-auth* as “admin” and the default requireAuthentication=false allows the request through, leaking currentVersion publicly. Even with authentication enabled, any authenticated non-admin can hit the endpoint because requireAdmin is false for this path.
Agent Prompt
### Issue description
`/admin/update/status` currently relies on a comment assumption (“webaccess gates /admin/*”) that is not true with the current `webaccess.ts` logic. As a result, the endpoint can be reachable by unauthenticated users when `requireAuthentication=false` (default) and by non-admin authenticated users when `requireAuthentication=true`, leaking `currentVersion`.
### Issue Context
- `webaccess.ts` uses `requireAdmin = req.path.toLowerCase().startsWith('/admin-auth')`, so `/admin/update/status` is not treated as admin-only.
- The endpoint returns sensitive-ish info (`currentVersion`) that the PR explicitly tries not to expose publicly.
### Fix Focus Areas
- src/node/hooks/express/updateStatus.ts[29-60]
- src/node/hooks/express/webaccess.ts[58-60]
- src/node/hooks/express/webaccess.ts[127-134]
- src/tests/backend/specs/updateStatus.ts[76-87]
### Suggested fix
- In the `/admin/update/status` handler, require `req.session?.user?.is_admin === true`.
- If no session user: respond `401`.
- If session user exists but not admin: respond `403`.
- Add/extend integration tests to cover:
- default `requireAuthentication=false` still denies access
- authenticated non-admin user denied
- admin user allowed
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+35
to
+41
| app.get('/api/version-status', async (_req: any, res: any) => { | ||
| const now = Date.now(); | ||
| if (now - badgeCache.at > BADGE_CACHE_MS) { | ||
| badgeCache = {value: await computeOutdated(), at: now}; | ||
| } | ||
| res.json({outdated: badgeCache.value}); | ||
| }); |
There was a problem hiding this comment.
4. Async handlers unprotected 🐞 Bug ☼ Reliability
The new Express routes use async handlers directly, so exceptions (for example, loadState() throwing on EPERM/IO errors) will not be forwarded to Express error handling in this codebase’s established pattern. This can cause unhandled promise rejections and/or hung requests.
Agent Prompt
### Issue description
`updateStatus.ts` registers `async` Express handlers without wrapping them to forward rejections to `next(err)`. In this repo, other handlers explicitly `.catch(next)`, implying Express is not expected to handle async rejections automatically.
### Issue Context
`loadState()` can throw for non-ENOENT read failures (permissions, IO), and those errors would currently bypass Express error handling.
### Fix Focus Areas
- src/node/hooks/express/updateStatus.ts[29-60]
- src/node/hooks/express/importexport.ts[27-71]
### Suggested fix
- Change both routes to the established pattern:
- `app.get(path, (req, res, next) => { (async () => { ... })().catch((err) => next(err || new Error(err))); });`
- Optionally add a small helper (e.g. `wrapAsync`) local to the file to reduce duplication.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+427
to
+438
| /** | ||
| * Self-update subsystem (PR 1: tier 1 only). | ||
| * Tier "off" disables the version check entirely. Default "notify" shows a banner when behind. | ||
| */ | ||
| updates: { | ||
| tier: 'notify', | ||
| source: 'github', | ||
| channel: 'stable', | ||
| installMethod: 'auto', | ||
| checkIntervalHours: 6, | ||
| githubRepo: 'ether/etherpad', | ||
| }, |
There was a problem hiding this comment.
1. updates.tier defaults to notify 📘 Rule violation ☼ Reliability
The new updater subsystem is enabled by default because updates.tier defaults to notify, which starts background polling on boot. This violates the requirement that new features be feature-flagged and disabled by default to avoid changing behavior for existing deployments.
Agent Prompt
## Issue description
The updater subsystem is a new feature but is enabled by default because `updates.tier` defaults to `notify`, which starts polling on boot.
## Issue Context
Compliance requires new features to be behind a feature flag and disabled by default. The code currently supports disabling via `updates.tier: "off"`, but the default value is `notify` in both runtime defaults and the settings templates.
## Fix Focus Areas
- src/node/utils/Settings.ts[427-438]
- settings.json.template[178-190]
- settings.json.docker[187-200]
- src/node/updater/index.ts[108-116]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+43
to
+60
| // Admin-protected. webaccess.ts already gates /admin/* with admin auth. | ||
| app.get('/admin/update/status', async (_req: any, res: any) => { | ||
| const state = await loadState(stateFilePath()); | ||
| const current = getEpVersion(); | ||
| const installMethod = getDetectedInstallMethod(); | ||
| const policy = state.latest | ||
| ? evaluatePolicy({installMethod, tier: settings.updates.tier, current, latest: state.latest.version}) | ||
| : null; | ||
| res.json({ | ||
| currentVersion: current, | ||
| latest: state.latest, | ||
| lastCheckAt: state.lastCheckAt, | ||
| installMethod, | ||
| tier: settings.updates.tier, | ||
| policy, | ||
| vulnerableBelow: state.vulnerableBelow, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
2. Admin status endpoint public 🐞 Bug ⛨ Security
GET /admin/update/status does not verify req.session.user.is_admin, and webaccess treats only /admin-auth* as admin-protected, so with default requireAuthentication=false the endpoint can be accessed unauthenticated and leaks currentVersion, latest, and vulnerableBelow. The included test only forces auth by flipping requireAuthentication=true, which masks the default behavior.
Agent Prompt
### Issue description
`/admin/update/status` is currently reachable without admin authentication under the default configuration (because webaccess only treats `/admin-auth*` as admin-protected and `requireAuthentication` defaults to false). This leaks operational/security-relevant information.
### Issue Context
Admin auth in this codebase is triggered by requesting `/admin-auth/*` (see `webaccess.ts`). New admin-only JSON endpoints should either:
- live under `/admin-auth/...` so the global middleware enforces admin auth, and/or
- explicitly check `req.session.user?.is_admin` and reject otherwise.
### Fix Focus Areas
- src/node/hooks/express/updateStatus.ts[43-60]
- admin/src/components/UpdateBanner.tsx[11-16]
- src/tests/backend/specs/updateStatus.ts[76-87]
### Expected change
1. Move the endpoint to `/admin-auth/update/status` **or** add an explicit admin check in the handler (prefer doing both for defense-in-depth).
2. Update the admin UI fetch URL accordingly.
3. Update/add an integration test that asserts the endpoint is unauthorized when `requireAuthentication` is left at its default (false) and no admin session is present.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+100
to
+106
| const startPolling = (): void => { | ||
| const intervalMs = Math.max(1, settings.updates.checkIntervalHours) * 60 * 60 * 1000; | ||
| if (timer) clearInterval(timer); | ||
| timer = setInterval(() => { void performCheck(); }, intervalMs); | ||
| // Run an immediate first check, but don't block boot. | ||
| setTimeout(() => { void performCheck(); }, 5000); | ||
| }; |
There was a problem hiding this comment.
3. Updater checks can overlap 🐞 Bug ☼ Reliability
startPolling() schedules performCheck() via both setInterval and an initial setTimeout without any in-flight guard, so slow GitHub fetches can cause concurrent performCheck() executions that mutate the shared cached state and race on writing update-state.json. shutdown() clears only the interval, not the initial timeout, so a check can still fire after shutdown has begun.
Agent Prompt
### Issue description
The updater poll loop can execute multiple `performCheck()` runs concurrently (interval tick + initial timeout, plus any later overlap if a check runs longer than the interval). Because `performCheck()` mutates shared in-memory state and writes it to disk, concurrent runs can cause state corruption/out-of-order writes and duplicate email decisions.
### Issue Context
`performCheck()` uses `getCurrentState()` which caches a singleton `inMemoryState`, then mutates it and calls `saveState()`.
### Fix Focus Areas
- src/node/updater/index.ts[36-106]
- src/node/updater/index.ts[118-121]
### Expected change
1. Add an in-flight guard (e.g., `let checkInFlight = false;`) so overlapping ticks return early or coalesce.
2. Store the `setTimeout()` handle and clear it in `shutdown()`.
3. Consider changing to a self-scheduling loop (`setTimeout` scheduled *after* an awaited `performCheck()` completes) to guarantee serialization.
4. (Optional) Add a unit/integration test that simulates a slow fetcher to ensure `performCheck()` is not entered concurrently.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ships Tier 1 (notify) of the four-tier auto-update design at
docs/superpowers/specs/2026-04-25-auto-update-design.md.var/update-state.json./admin/updatepage (read-only) with version, install method, tier, changelog./api/version-statusnever leaks the running version string.adminEmailsetting (weekly while vulnerable, monthly while severely outdated). Real SMTP transport deferred to a follow-up; PR 1 ships the cadence/dedupe machinery.updates.*settings block with a default oftier: "notify". Settier: "off"to disable entirely.See
doc/admin/updates.mdfor full configuration.Architecture
src/node/updater/— pure modules:types,versionCompare,state(atomic write + schema validation),InstallMethodDetector,UpdatePolicy,VersionChecker(fetcher abstraction),Notifier(cadence decider) + boot wiring (index.ts).src/node/hooks/express/updateStatus.ts— public/api/version-status(cached 60s, no version leak) and admin/admin/update/status.src/node/utils/Settings.ts+settings.json.template+settings.json.docker.admin/src/components/UpdateBanner.tsx,admin/src/pages/UpdatePage.tsx, store slice, route, nav link, CSS.src/static/js/pad_version_badge.ts,src/templates/pad.html,src/static/css/pad.css.src/locales/en.json.doc/admin/updates.md(new),CHANGELOG.md(Unreleased section).Test plan
versionCompare,state,InstallMethodDetector,UpdatePolicy,VersionChecker,Notifier/admin/update/status(admin auth, shape) and/api/version-status(no version leak, severe state)null/severe/vulnerablepnpm ts-checkclean,pnpm run build:uisucceedsNotes
(would send email). Follow-up PR will add nodemailer or rely on a notification plugin.fix(updater)commits address bugs surfaced during code review (semver regex four-part rejection, null-email/array schema validation, ETag preservation on prerelease, tagChanged email cadence).🤖 Generated with Claude Code