diff --git a/CHANGELOG.md b/CHANGELOG.md index 5460664ec..1e6971fc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,10 +8,29 @@ All notable changes to this project are documented in this file. The format is b > was still unreleased (GH issue #253). Those entries have been moved to the > release they actually shipped in, verified with `git tag --contains`. > Versions not listed below still have no entry; consult the commit log. +> +> 0.0.28 through 0.0.37 were released before the release workflow cut this +> file (#572), so their changes are still listed under `[Unreleased]` and will +> land in the next dated section (#584). ## [Unreleased] ### Fixed +- File storage: `DELETE /api/file_storage/files/{id}` and the bulk delete now drop the stored + bytes only after the request transaction commits, so a failed commit no longer leaves a live + row with no bytes. A new hourly `file_storage.reap_objects` beat task (needs + `background_tasks`) retries objects of soft-deleted rows and removes objects left by uploads + whose row never committed, after twice the upload-session TTL (#582). +- File storage: cancelling an upload still waiting for a free slot no longer uploads it later in + the background (#588). +- Tenants: the organisation settings page and its menu item are registered only when the + `settings` module is enabled; without it `/tenants/settings` used to 500 with an + `ImportError`. `file_storage` now orders after `background_tasks` when both are installed (#585). +- Docker: **Behaviour change.** With `SM_USERS_BOOTSTRAP_PASSWORD` unset, the entrypoint now + generates a random first-boot admin password and prints it (the compose file already said so), + instead of seeding the public `changeme`. An explicit `changeme` is refused when + `SM_ENVIRONMENT` is `production` (the image default). `docs/release.md`'s local rehearsal now + runs in a disposable worktree instead of ending with `git reset --hard HEAD~1` (#587). - Retained logs: a NUL character no longer drops the whole log batch on Postgres. - Users: after a self-service password change the same browser kept its page session but every `/api/users/me*` call (including "Sign out everywhere") answered 401, because the diff --git a/README.md b/README.md index 20250f511..a98f44421 100644 --- a/README.md +++ b/README.md @@ -68,20 +68,19 @@ docker build -t simple-module-python . docker run --rm -p 8000:8000 -v simple-module-python-data:/app/data simple-module-python ``` -**Logging in.** With no `SM_USERS_BOOTSTRAP_*` set, the container seeds -**`admin@example.com` / `changeme`** — the same pair `.env.example` uses for -local dev — and says so on every boot that uses it: +**Logging in.** With no `SM_USERS_BOOTSTRAP_PASSWORD` set, the container seeds +`admin@example.com` with a randomly generated password. The users module logs it +once when the first admin is created: ``` -entrypoint: WARNING - no SM_USERS_BOOTSTRAP_PASSWORD set, so the -entrypoint: first-boot admin is the public default: -entrypoint: admin@example.com / changeme +first-boot admin created; store this password securely: admin@example.com / ``` -Pass `-e SM_USERS_BOOTSTRAP_EMAIL=… -e SM_USERS_BOOTSTRAP_PASSWORD=…` to seed -your own instead, and change it before the container is reachable by anyone but -you. The seed only applies while the users table is empty, so on a reused volume -the existing password still stands — reset it with +Read it with `docker logs`. Pass `-e SM_USERS_BOOTSTRAP_EMAIL=… -e SM_USERS_BOOTSTRAP_PASSWORD=…` +to seed your own instead; an explicit `changeme` is refused because the image runs +with `SM_ENVIRONMENT=production`. The seed only applies while the users table is +empty, so on a reused volume the existing password still stands and no credentials +are logged — reset it with `smpy users create-admin --email … --password … --force`. `make docker-compose-app` (or `docker compose up --build app`) runs the same @@ -101,7 +100,7 @@ bundle rather than a Vite dev server. Useful overrides: |---|---|---| | `SM_SECRET_KEY` | generated per start | Persist sessions across restarts | | `SM_DATABASE_URL` | `sqlite+aiosqlite:////app/data/app.db` | Point at Postgres | -| `SM_USERS_BOOTSTRAP_EMAIL` / `_PASSWORD` | `admin@example.com` / `changeme` | Seed a first admin nobody else can guess | +| `SM_USERS_BOOTSTRAP_EMAIL` / `_PASSWORD` | `admin@example.com` / generated and logged | Seed a first admin with a password you chose | | `SM_TRUSTED_PROXY` | unset | Set to `*` behind a reverse proxy, so logs record the visitor's IP and not the proxy's | **No background tasks.** The image skips installing the Celery module diff --git a/docker-compose.yml b/docker-compose.yml index e9dfae974..928cfd204 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -14,7 +14,10 @@ services: - SM_SECRET_KEY # First-boot admin seed, applied only while the users table is empty. # Unset, the entrypoint seeds admin@example.com with a generated - # password and prints it — see `docker compose logs app`. + # password; the users module logs it only when creating the admin. + # See `docker compose logs app`. + # Generated credentials do not reset existing accounts. An explicit + # `changeme` password is refused in production. - SM_USERS_BOOTSTRAP_EMAIL - SM_USERS_BOOTSTRAP_PASSWORD # Set to `*` when this sits behind a reverse proxy that terminates TLS, diff --git a/docker/entrypoint.sh b/docker/entrypoint.sh index 4a3c55eee..1e4eac819 100755 --- a/docker/entrypoint.sh +++ b/docker/entrypoint.sh @@ -9,7 +9,7 @@ # place for one), so any that are still unset get an ephemeral random # value — enough to boot and log in, gone on the next start. # 2. Seed an admin, so a bare `docker run` lands on a login you can pass -# (admin@example.com / changeme unless SM_USERS_BOOTSTRAP_* say otherwise). +# (admin@example.com / a generated password unless overridden). # 3. Apply migrations. A fresh SQLite volume has no tables at all, and the # boot-time SM010 check fails the app in production when the DB revision # is behind head. @@ -32,24 +32,21 @@ if [ -n "$_ephemeral" ]; then echo "entrypoint: sessions and any reset/verification links they sign die on restart — set these to persist them." >&2 fi -# The seed the users module applies only while its table is empty, so this -# can't overwrite an account on a persistent volume — and without it a fresh -# container serves a login page nobody holds credentials for. The default is -# deliberately a well-known one: this image is a starting point you are meant -# to log straight into. Anything running where that matters should pass both -# vars, which is why the banner below is loud rather than silent. +# The users module seeds only while its table is empty. These credentials +# are for first boot, not a password reset for an existing volume. : "${SM_USERS_BOOTSTRAP_EMAIL:=admin@example.com}" -: "${SM_USERS_BOOTSTRAP_PASSWORD:=changeme}" +if [ -z "${SM_USERS_BOOTSTRAP_PASSWORD:-}" ]; then + SM_USERS_BOOTSTRAP_PASSWORD="$(python -c 'import secrets; print(secrets.token_urlsafe(32))')" + export SM_USERS_BOOTSTRAP_PASSWORD_GENERATED=1 +fi export SM_USERS_BOOTSTRAP_EMAIL SM_USERS_BOOTSTRAP_PASSWORD if [ "$SM_USERS_BOOTSTRAP_PASSWORD" = "changeme" ]; then - echo "entrypoint: WARNING - no SM_USERS_BOOTSTRAP_PASSWORD set, so the" >&2 - echo "entrypoint: first-boot admin is the public default:" >&2 - echo "entrypoint: $SM_USERS_BOOTSTRAP_EMAIL / changeme" >&2 - echo "entrypoint: Seeded only while no user exists — an existing install is" >&2 - echo "entrypoint: left alone. Set SM_USERS_BOOTSTRAP_EMAIL/_PASSWORD before" >&2 - echo "entrypoint: first boot, or change it later with:" >&2 - echo "entrypoint: smpy users create-admin --email ... --password ... --force" >&2 + if [ "${SM_ENVIRONMENT:-production}" = "production" ]; then + echo "entrypoint: refusing SM_USERS_BOOTSTRAP_PASSWORD=changeme in production; set a secure password or leave it unset to generate one." >&2 + exit 1 + fi + echo "entrypoint: WARNING - SM_USERS_BOOTSTRAP_PASSWORD is the public default; set a secure password before deploying." >&2 fi # `upgrade heads` (plural) applies every per-module migration branch; diff --git a/docs/reference/deployment.md b/docs/reference/deployment.md index 0741b8abf..c33ed30e0 100644 --- a/docs/reference/deployment.md +++ b/docs/reference/deployment.md @@ -35,11 +35,12 @@ make docker-compose-app # same image via the `app` service in docker-compose production secret (`SM_SECRET_KEY`, `SM_USERS_RESET_PASSWORD_TOKEN_SECRET`, `SM_USERS_VERIFICATION_TOKEN_SECRET`) with an ephemeral random value so a bare `docker run` boots — set them yourself for anything that must survive a -restart. It also seeds `admin@example.com` / `changeme` (warning printed on -every boot that uses it) unless `SM_USERS_BOOTSTRAP_EMAIL` / `_PASSWORD` say -otherwise; that seed only lands while the users table is empty, so it never -overwrites an existing account. A deployment that anyone else can reach should -set both vars before first boot — the default password is public. The checklist above still applies: that image is a demo/local +restart. It also seeds `admin@example.com` with a randomly generated password +logged by the users module only when the first admin is created, unless +`SM_USERS_BOOTSTRAP_EMAIL` / `_PASSWORD` say otherwise. Store generated credentials securely and restrict access to logs. +The seed only lands while the users table is empty, so it never overwrites an +existing account; credentials generated on later starts do not reset passwords. +An explicit `changeme` password is refused in production. The checklist above still applies: that image is a demo/local target, not a production deployment (SQLite, ephemeral secrets). That image carries no Celery: the build passes diff --git a/docs/reference/env-vars.md b/docs/reference/env-vars.md index b1f700433..e2a2b7b20 100644 --- a/docs/reference/env-vars.md +++ b/docs/reference/env-vars.md @@ -82,6 +82,7 @@ Most users settings (signup, OAuth/OIDC credentials, rate limits; SMTP lives in |---|---|---| | `SM_USERS_BOOTSTRAP_EMAIL` | unset | Auto-creates an admin if set **and** the users table is empty. | | `SM_USERS_BOOTSTRAP_PASSWORD` | unset | Paired with the email above. | +| `SM_USERS_BOOTSTRAP_PASSWORD_GENERATED` | unset | Internal entrypoint marker (`1`): log generated credentials at WARNING only when the bootstrap admin is created. | | `SM_USERS_BOOTSTRAP_USER_EMAIL` | unset | Optional second non-admin seed user (handy in dev). | | `SM_USERS_BOOTSTRAP_USER_PASSWORD` | unset | Paired with the user email above. | | `SM_USERS_RESET_PASSWORD_TOKEN_SECRET` | dev placeholder | HMAC key for password-reset tokens. **Must** be overridden in production (boot fails otherwise). | diff --git a/docs/release.md b/docs/release.md index 0c54c2642..c33acdb7e 100644 --- a/docs/release.md +++ b/docs/release.md @@ -134,7 +134,7 @@ Expected wall time: 5–8 minutes. ### Step 4. CHANGELOG and GitHub Release notes -`finalize` runs `scripts/cut_changelog.py X.Y.Z` before the bump commit: `## [Unreleased]` becomes `## [X.Y.Z] - YYYY-MM-DD`, a fresh empty `## [Unreleased]` is inserted above it and the compare links are updated. Re-running is a no-op once the section exists. Keep `[Unreleased]` current in every user-visible PR; whatever is there at release time is the release's notes. +`finalize` enables `cut-changelog` on the shared `.github/actions/bump-version` action, which runs `scripts/cut_changelog.py X.Y.Z` before the bump commit: `## [Unreleased]` becomes `## [X.Y.Z] — YYYY-MM-DD`, a fresh empty `## [Unreleased]` is inserted above it and the compare links are updated. Re-running is a no-op once the section exists. Keep `[Unreleased]` current in every user-visible PR; whatever is there at release time is the release's notes. The GitHub Release is created with `--notes-file` from that dated section (`scripts/cut_changelog.py X.Y.Z --notes`), falling back to `--generate-notes` only when the section is empty. @@ -160,16 +160,24 @@ A test (`framework/cli/tests/test_template_requires_framework.py`) pins that the ## Local pre-flight (optional but recommended) -Before running the workflow, you can rehearse the whole pipeline offline. Nothing leaves your machine: +Before running the workflow, rehearse in a disposable worktree. Nothing is published. +The worktree starts at `HEAD`; uncommitted changes in your original checkout are +not included and remain untouched. Run the following from the repository root: ```bash +# 0. Isolate version edits and build artifacts from your working checkout +release_checkout="$PWD" +release_rehearsal=$(mktemp -d "${TMPDIR:-/tmp}/smpy-release.XXXXXX") +git worktree add --detach "$release_rehearsal/worktree" HEAD +cd "$release_rehearsal/worktree" + # 1. Check every workspace package is currently at 0.0.1 (or your expected base) uv run python scripts/bump_version.py 0.0.1 --check # 2. Dry-run the bump (writes nothing, shows what would change) uv run python scripts/bump_version.py 0.0.2 --dry-run -# 3. Actually bump (commit on a throwaway branch if you want to keep it) +# 3. Actually bump inside the disposable worktree uv run python scripts/bump_version.py 0.0.2 npm install --package-lock-only @@ -186,11 +194,14 @@ mkdir -p dist-npm && for p in packages/*/; do npm pack "$p" --pack-destination d ls dist-py/ | wc -l # expect 30 (a wheel + sdist for each of the 15 buildable packages; only 13 are later published) ls dist-npm/ | wc -l # expect 3 -# 7. Revert (if you don't actually want to release) -git reset --hard HEAD~1 +# 7. Discard only the rehearsal worktree, including its version edits +cd "$release_checkout" +git worktree remove --force "$release_rehearsal/worktree" +rmdir "$release_rehearsal" ``` If step 5 fails for any package, the workflow will fail the same way — fix it before dispatching. +Run step 7 even if an earlier step fails; it removes only the disposable worktree. --- diff --git a/modules/file_storage/file_storage/backends/filesystem.py b/modules/file_storage/file_storage/backends/filesystem.py index 52ff6aea2..f583df97a 100644 --- a/modules/file_storage/file_storage/backends/filesystem.py +++ b/modules/file_storage/file_storage/backends/filesystem.py @@ -2,9 +2,12 @@ from __future__ import annotations +import asyncio import contextlib +import os import uuid from collections.abc import AsyncIterator +from datetime import datetime from pathlib import Path import aiofiles @@ -82,6 +85,29 @@ async def delete(self, key: str) -> None: path = self._resolve(key) path.unlink(missing_ok=True) + async def list_stale_objects(self, cutoff: datetime) -> AsyncIterator[str]: + """Yield keys last modified before the reconciliation cutoff.""" + keys = await asyncio.to_thread(self._collect_stale_objects, cutoff.timestamp()) + for key in keys: + yield key + + def _collect_stale_objects(self, cutoff_ts: float) -> list[str]: + keys = [] + for directory, dirs, files in os.walk(self.root): + if Path(directory) == self.root: + dirs[:] = [name for name in dirs if name != constants.FS_MULTIPART_DIR] + for name in files: + path = Path(directory) / name + relative = path.relative_to(self.root) + # Preserve the previous shard/key-directory/file glob depth. + if len(relative.parts) < 3: + continue + # Another delete (including this sweep) may have removed a variant. + with contextlib.suppress(FileNotFoundError): + if path.is_file() and path.stat().st_mtime < cutoff_ts: + keys.append(relative.as_posix().split("/", 1)[1]) + return keys + async def exists(self, key: str) -> bool: return self._resolve(key).exists() diff --git a/modules/file_storage/file_storage/backends/s3.py b/modules/file_storage/file_storage/backends/s3.py index f5263a5bb..119750ebe 100644 --- a/modules/file_storage/file_storage/backends/s3.py +++ b/modules/file_storage/file_storage/backends/s3.py @@ -8,6 +8,7 @@ from __future__ import annotations from collections.abc import AsyncGenerator, AsyncIterator +from datetime import datetime from tempfile import SpooledTemporaryFile from typing import Any @@ -129,6 +130,15 @@ async def delete(self, key: str) -> None: except Exception as exc: raise StorageBackendError(f"S3 delete_object failed: {exc}") from exc + async def list_stale_objects(self, cutoff: datetime) -> AsyncIterator[str]: + """Yield keys last modified before the reconciliation cutoff.""" + async with self._client() as client: + pages = client.get_paginator("list_objects_v2") + async for page in pages.paginate(Bucket=self.bucket): + for obj in page.get("Contents", []): + if obj["LastModified"] < cutoff: + yield obj["Key"] + async def exists(self, key: str) -> bool: try: async with self._client() as client: diff --git a/modules/file_storage/file_storage/constants.py b/modules/file_storage/file_storage/constants.py index fa7aa86ab..f3690e190 100644 --- a/modules/file_storage/file_storage/constants.py +++ b/modules/file_storage/file_storage/constants.py @@ -9,6 +9,7 @@ from __future__ import annotations +import re from typing import Final # ── Module identity ────────────────────────────────────────────────── @@ -21,6 +22,7 @@ MODULE_PASCAL: Final = "FileStorage" MODULE_DISPLAY_NAME: Final = "Files" _MODULE_SETTINGS: Final = "Settings" +_MODULE_BACKGROUND_TASKS: Final = "BackgroundTasks" # ── Configuration ──────────────────────────────────────────────────── ENV_PREFIX: Final = "SM_FILE_STORAGE_" @@ -121,6 +123,7 @@ class UploadStatus: class Task: """Celery task names (``tasks.py``; only loaded when background_tasks is installed).""" + REAP_OBJECTS: Final = "file_storage.reap_objects" REAP_STALE_UPLOADS: Final = "file_storage.reap_stale_uploads" COMPUTE_CHECKSUM: Final = "file_storage.compute_checksum" @@ -244,3 +247,7 @@ class I18nKey: PUBLIC_CORS_METHODS: Final = ("GET", "HEAD", "OPTIONS") PUBLIC_CORS_ALLOW_HEADERS: Final = ("Range", "If-Range", "If-None-Match") PUBLIC_CORS_EXPOSE_HEADERS: Final = ("Content-Range", "Accept-Ranges", "ETag", "Content-Length") + +# Keys this module writes: ``{tenant}/YYYY/MM/DD/{uuid hex}[.ext]``. Reapers only +# ever touch objects shaped like this, never foreign ones in a shared store. +OWN_KEY: Final = re.compile(r"^[^/]+/\d{4}/\d{2}/\d{2}/[0-9a-f]{32}(\.[^/]*)?$") diff --git a/modules/file_storage/file_storage/deletes.py b/modules/file_storage/file_storage/deletes.py index bd400ceb9..2270d0694 100644 --- a/modules/file_storage/file_storage/deletes.py +++ b/modules/file_storage/file_storage/deletes.py @@ -40,7 +40,7 @@ async def get( ) -> StoredFile: ... async def delete_many( - self, file_ids: Sequence[uuid.UUID], *, owner: str | None = None + self, file_ids: Sequence[uuid.UUID], *, owner: str | None = None, drop_object: bool = True ) -> list[StoredFile]: """Soft-delete every id that still resolves, returning the rows removed. @@ -63,6 +63,8 @@ async def delete_many( # One flush for the batch: the DB rows are what the next page load # reads, so they must all be marked before any object is dropped. await self.db.flush() + if not drop_object: + return rows for row in rows: # Every object is dropped independently. The rows are already # marked deleted, so letting one unreachable object abort the loop @@ -71,14 +73,7 @@ async def delete_many( # no row left pointing at them. A failure here is a janitor's # problem, not the caller's. try: - try: - await self.backend.delete(row.key) - finally: - # Variants go even when the original's delete fails. - await thumbnails.delete_variants(self.backend, row.key) - except StorageNotFoundError: - # Acceptably absent — eg. a previous delete partially succeeded. - pass + await self.drop_object(row) except Exception: _logger.exception( "file_storage.bulk_delete_object_failed key=%s — row is deleted, " diff --git a/modules/file_storage/file_storage/endpoints/api.py b/modules/file_storage/file_storage/endpoints/api.py index 0a6be2aed..ac89e2118 100644 --- a/modules/file_storage/file_storage/endpoints/api.py +++ b/modules/file_storage/file_storage/endpoints/api.py @@ -3,6 +3,7 @@ from __future__ import annotations import uuid +from functools import partial from typing import Literal from fastapi import ( @@ -19,6 +20,7 @@ ) from fastapi.responses import RedirectResponse from simple_module_core.events import EventBus +from simple_module_db.callbacks import register_on_commit from simple_module_db.pagination import PageParams from simple_module_hosting.i18n_deps import TranslatorDep from simple_module_hosting.pagination import page_params @@ -250,8 +252,9 @@ async def bulk_delete_files( subscriber that mirrors or reindexes files cannot tell a bulk delete from a run of single ones. """ - rows = await service.delete_many(body.ids, owner=owner) + rows = await service.delete_many(body.ids, owner=owner, drop_object=False) for row in rows: + register_on_commit(service.db, partial(service.drop_object, row)) await bus.publish(FileDeleted(file_id=row.id, key=row.key)) return BulkDeleteResult(deleted=len(rows), ids=[row.id for row in rows]) @@ -269,7 +272,8 @@ async def delete_file( owner: str | None = OwnerScope, ) -> None: try: - row = await service.delete(file_id, owner=owner) + row = await service.delete(file_id, owner=owner, drop_object=False) except StoredFileNotFoundError as exc: raise not_found(t) from exc + register_on_commit(service.db, partial(service.drop_object, row)) await bus.publish(FileDeleted(file_id=row.id, key=row.key)) diff --git a/modules/file_storage/file_storage/module.py b/modules/file_storage/file_storage/module.py index 0ef50c6c2..b1d5264cd 100644 --- a/modules/file_storage/file_storage/module.py +++ b/modules/file_storage/file_storage/module.py @@ -37,7 +37,8 @@ class FileStorageModule(ModuleBase): view_prefix=constants.ROUTE_PREFIX_VIEW, # Kept after Settings so the module order is unchanged; registering # settings no longer needs it (the framework owns the registry, #500). - optional_depends_on=[constants._MODULE_SETTINGS], + # Task/checksum integration also needs BackgroundTasks loaded first when present. + optional_depends_on=[constants._MODULE_SETTINGS, constants._MODULE_BACKGROUND_TASKS], i18n_audience="admin", ) diff --git a/modules/file_storage/file_storage/object_reaper.py b/modules/file_storage/file_storage/object_reaper.py new file mode 100644 index 000000000..c5537c04a --- /dev/null +++ b/modules/file_storage/file_storage/object_reaper.py @@ -0,0 +1,88 @@ +"""Reconcile old stored objects against committed metadata across all tenants.""" + +from __future__ import annotations + +import asyncio +import logging +import re +from datetime import UTC, datetime, timedelta + +from simple_module_db import all_tenants +from sqlalchemy import select +from sqlmodel import col + +from file_storage import constants, thumbnails +from file_storage.models import StoredFile, UploadSession + +logger = logging.getLogger(__name__) + +_VARIANT_SUFFIX = re.compile(r"\.w\d+\.webp$") +_BATCH = 500 + + +def _referenced(session_factory, backend_id: str, originals: list[str]) -> set[str]: + """The subset of ``originals`` a live file row or an active upload session still owns.""" + with session_factory() as db, all_tenants(): + live = db.execute( + select(StoredFile.key).where( + col(StoredFile.key).in_(originals), StoredFile.backend == backend_id + ) + ).scalars() + active = db.execute( + select(UploadSession.key).where( + col(UploadSession.key).in_(originals), + UploadSession.status == constants.UploadStatus.ACTIVE, + ) + ).scalars() + return {*live, *active} + + +async def _drop(backend, key: str, original: str) -> bool: + try: + await backend.delete(key) + # Variants are removed with their original; a stale variant alone needs no sweep. + if key == original: + await thumbnails.delete_variants(backend, key) + except Exception: + logger.exception("file_storage.object_reap_failed key=%s", key) + return False + return True + + +async def _reap_batch(backend, session_factory, batch: list[tuple[str, str]]) -> int: + # Sync session: run off the event loop so a slow query does not stall the listing. + referenced = await asyncio.to_thread( + _referenced, session_factory, backend.backend_id, [o for _, o in batch] + ) + removed = 0 + for key, original in batch: + if original not in referenced and await _drop(backend, key, original): + removed += 1 + return removed + + +async def reap_objects(backend, session_factory, *, grace: timedelta) -> int: + """Retry deleted objects and reap rolled-back uploads after a grace period. + + Listing is optional for third-party providers. Only module-shaped original + keys are considered; thumbnails are removed with their original. The grace + period must exceed the deployment's maximum transaction duration. Keys are + checked against the database in batches, not one query pair per object. + """ + lister = getattr(backend, "list_stale_objects", None) + if lister is None: + return 0 + cutoff = datetime.now(UTC) - grace + removed = 0 + batch: list[tuple[str, str]] = [] + async for key in lister(cutoff): + original = _VARIANT_SUFFIX.sub("", key) + if not constants.OWN_KEY.fullmatch(original): + continue + batch.append((key, original)) + if len(batch) >= _BATCH: + removed += await _reap_batch(backend, session_factory, batch) + batch = [] + if batch: + removed += await _reap_batch(backend, session_factory, batch) + return removed diff --git a/modules/file_storage/file_storage/pages/constants.ts b/modules/file_storage/file_storage/pages/constants.ts index 7eed5ad8c..5a374b025 100644 --- a/modules/file_storage/file_storage/pages/constants.ts +++ b/modules/file_storage/file_storage/pages/constants.ts @@ -36,3 +36,10 @@ export const RELOAD_PROPS: string[] = [ 'uploaders', 'used_bytes', ]; + +// Files in a batch have no ordering dependency on one another, so uploading +// strictly one at a time makes a 10-file drop take the sum of every file's +// duration. A small worker pool instead bounds the batch to roughly the +// slowest file's duration without opening enough simultaneous connections to +// swamp the server or the browser's per-origin connection limit. +export const UPLOAD_CONCURRENCY = 4; diff --git a/modules/file_storage/file_storage/pages/upload-queue.ts b/modules/file_storage/file_storage/pages/upload-queue.ts index 56e7aa351..ec3dbc92c 100644 --- a/modules/file_storage/file_storage/pages/upload-queue.ts +++ b/modules/file_storage/file_storage/pages/upload-queue.ts @@ -1,7 +1,7 @@ import { router } from '@inertiajs/react'; import { useCallback, useRef, useState } from 'react'; -import { RELOAD_PROPS, ROUTES } from './constants'; +import { RELOAD_PROPS, ROUTES, UPLOAD_CONCURRENCY } from './constants'; import type { RemoteSession } from './resumable-upload'; import { discardSession, findSession, runResumable } from './resumable-upload'; import { loadSessionId } from './resume-store'; @@ -34,13 +34,6 @@ function nextId(): string { return `upload-${counter}`; } -// Files in a batch have no ordering dependency on one another, so uploading -// strictly one at a time makes a 10-file drop take the sum of every file's -// duration. A small worker pool instead bounds the batch to roughly the -// slowest file's duration without opening enough simultaneous connections to -// swamp the server or the browser's per-origin connection limit. -const UPLOAD_CONCURRENCY = 4; - /** * Uploads files with bounded concurrency, reporting byte progress per file. * @@ -199,7 +192,9 @@ export function useUploadQueue({ directThresholdBytes }: UploadQueueOptions = {} while (next < list.length) { const index = next; next += 1; - const outcome = await upload(queued[index].id, list[index]); + const jobId = queued[index].id; + if (!files.current.has(jobId)) continue; + const outcome = await upload(jobId, list[index]); if (outcome === 'done') uploaded += 1; // A cancel is the user's own doing — it needs no toast and no row. else if (outcome === 'error') failed.push(list[index].name); diff --git a/modules/file_storage/file_storage/tasks.py b/modules/file_storage/file_storage/tasks.py index d7cc49f9c..e6b842908 100644 --- a/modules/file_storage/file_storage/tasks.py +++ b/modules/file_storage/file_storage/tasks.py @@ -15,7 +15,6 @@ import asyncio import logging -import re import uuid from datetime import UTC, datetime, timedelta @@ -26,6 +25,10 @@ logger = logging.getLogger(__name__) BEAT_SCHEDULE = { + "file-storage-reap-objects": { + "task": constants.Task.REAP_OBJECTS, + "schedule": 3600.0, + }, "file-storage-reap-stale-uploads": { "task": constants.Task.REAP_STALE_UPLOADS, "schedule": 3600.0, @@ -115,7 +118,7 @@ async def _abort_all(backend, targets: list[tuple[str, str]]) -> set[tuple[str, # Keys this module mints: ``///
/<32 hex>``. -_OWN_KEY = re.compile(r"^[^/]+/\d{4}/\d{2}/\d{2}/[0-9a-f]{32}(\.[^/]*)?$") +_OWN_KEY = constants.OWN_KEY async def _abort_orphans(backend, older_than: timedelta, tracked: set[str]) -> int: @@ -169,3 +172,20 @@ def compute_checksum(file_id: str, tenant_id: str) -> str | None: if row is not None and not row.checksum_sha256: row.checksum_sha256 = digest return digest + + +@shared_task(name=constants.Task.REAP_OBJECTS) +def reap_objects() -> int: + """Reconcile completed objects, with twice the upload TTL as a safety window.""" + from background_tasks.sync_db import sync_session + + from file_storage.object_reaper import reap_objects as reconcile + + backend, settings = _backend_and_settings() + return asyncio.run( + reconcile( + backend, + sync_session, + grace=timedelta(seconds=2 * settings.upload_session_ttl_seconds), + ) + ) diff --git a/modules/file_storage/tests-js/file_storage_upload_queue_cancellation.test.ts b/modules/file_storage/tests-js/file_storage_upload_queue_cancellation.test.ts new file mode 100644 index 000000000..21fc89d3a --- /dev/null +++ b/modules/file_storage/tests-js/file_storage_upload_queue_cancellation.test.ts @@ -0,0 +1,72 @@ +import { act, renderHook } from '@testing-library/react'; +import { afterEach, expect, test, vi } from 'vitest'; + +vi.mock('@inertiajs/react', () => ({ + router: { + reload: vi.fn((options?: { onFinish?: () => void }) => options?.onFinish?.()), + }, +})); + +vi.mock('../file_storage/pages/constants', async (importOriginal) => ({ + ...(await importOriginal()), + UPLOAD_CONCURRENCY: 1, +})); + +import { useUploadQueue } from '../file_storage/pages/upload-queue'; + +class PendingXhr { + static instances: PendingXhr[] = []; + status = 201; + responseText = '{}'; + upload = { addEventListener: vi.fn() }; + private handlers = new Map void>(); + + constructor() { + PendingXhr.instances.push(this); + } + + open() {} + send() {} + + addEventListener(type: string, callback: () => void) { + this.handlers.set(type, callback); + } + + finish() { + this.handlers.get('load')?.(); + } +} + +afterEach(() => vi.unstubAllGlobals()); + +test('cancelling a queued upload prevents its transport from starting', async () => { + PendingXhr.instances = []; + vi.stubGlobal('XMLHttpRequest', PendingXhr); + const { result } = renderHook(() => useUploadQueue()); + let batch!: ReturnType; + act(() => { + batch = result.current.start([ + new File(['first'], 'first.txt'), + new File(['second'], 'second.txt'), + ]); + }); + + expect(result.current.jobs).toHaveLength(2); + expect(PendingXhr.instances).toHaveLength(1); + const cancelledId = result.current.jobs[1].id; + act(() => result.current.cancel(cancelledId)); + expect(result.current.jobs.map((job) => job.name)).toEqual(['first.txt']); + + await act(async () => { + PendingXhr.instances[0].finish(); + // Let the worker claim its next job before checking the transport count. + await Promise.resolve(); + await Promise.resolve(); + }); + expect(PendingXhr.instances).toHaveLength(1); + await act(async () => { + expect(await batch).toEqual({ uploaded: 1, failed: [] }); + }); + expect(result.current.jobs).toEqual([]); + expect(result.current.busy).toBe(false); +}); diff --git a/modules/file_storage/tests/test_file_storage_commit_cleanup.py b/modules/file_storage/tests/test_file_storage_commit_cleanup.py new file mode 100644 index 000000000..5bcee5db9 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_commit_cleanup.py @@ -0,0 +1,36 @@ +"""Storage bytes survive a failed request commit (#582).""" + +from unittest.mock import AsyncMock + +import pytest +from file_storage import thumbnails +from simple_module_db.session import RequestSession + + +@pytest.mark.parametrize("bulk", [False, True]) +async def test_delete_commit_failure_keeps_bytes(authenticated_client, storage, monkeypatch, bulk): + uploaded = await authenticated_client.post( + "/api/file-storage/upload", files={"file": ("a.txt", b"payload", "text/plain")} + ) + assert uploaded.status_code == 201 + row = uploaded.json() + variant = thumbnails.variant_key(row["key"], 128) + + async def body(): + yield b"thumbnail" + + await storage.backend.put(variant, body(), content_type="image/webp", size=9) + with monkeypatch.context() as patch: + patch.setattr(RequestSession, "commit", AsyncMock(side_effect=RuntimeError("commit"))) + if bulk: + response = await authenticated_client.post( + "/api/file-storage/files/bulk-delete", json={"ids": [row["id"]]} + ) + else: + response = await authenticated_client.delete(f"/api/file-storage/files/{row['id']}") + assert response.status_code == 500 + assert await storage.backend.exists(row["key"]) + assert await storage.backend.exists(variant) + response = await authenticated_client.get(f"/api/file-storage/files/{row['id']}/download") + assert response.status_code == 200 + assert response.content == b"payload" diff --git a/modules/file_storage/tests/test_file_storage_object_reaper.py b/modules/file_storage/tests/test_file_storage_object_reaper.py new file mode 100644 index 000000000..b632f6671 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_object_reaper.py @@ -0,0 +1,47 @@ +"""The object janitor removes rolled-back uploads and retries failed deletes.""" + +import asyncio +import os +import uuid +from datetime import timedelta + +from background_tasks import sync_db +from file_storage import tasks +from file_storage.models import StoredFile +from file_storage.object_reaper import reap_objects +from simple_module_db import tenant_context +from test_file_storage_upload_tasks import worker # noqa: F401 + + +def test_reconcile_spares_live_recent_and_foreign_objects(worker): # noqa: F811 + keys = [f"tenant-a/2026/10/15/{uuid.uuid4().hex}.bin" for _ in range(4)] + keys.append("foreign-app/export.bin") + + async def store(): + for key in keys: + + async def body(): + yield b"bytes" + + await worker.put(key, body(), content_type="x/y", size=5) + if key != keys[3]: + os.utime(worker._resolve(key), (1, 1)) + + asyncio.run(store()) + with sync_db.sync_session() as db, tenant_context("tenant-a"): + for key, deleted in [(keys[0], False), (keys[1], True)]: + db.add( + StoredFile( + key=key, + filename="a", + content_type="x/y", + size_bytes=5, + backend=worker.backend_id, + tenant_id="tenant-a", + is_deleted=deleted, + ) + ) + # keys[2] has no committed row, as after an upload commit failure. + assert tasks.reap_objects() == 2 + assert [worker._resolve(key).exists() for key in keys] == [True, False, False, True, True] + assert asyncio.run(reap_objects(worker, sync_db.sync_session, grace=timedelta(hours=1))) == 0 diff --git a/modules/file_storage/tests/test_file_storage_optional_ordering.py b/modules/file_storage/tests/test_file_storage_optional_ordering.py new file mode 100644 index 000000000..cc5e69029 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_optional_ordering.py @@ -0,0 +1,12 @@ +"""Optional task integration must load after its provider when installed.""" + +from background_tasks.module import BackgroundTasksModule +from file_storage.module import FileStorageModule +from simple_module_core.discovery import topological_sort + + +def test_background_tasks_loads_before_file_storage(): + storage = FileStorageModule() + tasks = BackgroundTasksModule() + assert tasks.meta.name in storage.meta.optional_depends_on + assert topological_sort([storage, tasks]) == [tasks, storage] diff --git a/modules/file_storage/tests/test_filesystem_stale_objects.py b/modules/file_storage/tests/test_filesystem_stale_objects.py new file mode 100644 index 000000000..5d3bb1667 --- /dev/null +++ b/modules/file_storage/tests/test_filesystem_stale_objects.py @@ -0,0 +1,36 @@ +"""Filesystem reconciliation excludes in-progress uploads and runs off-thread.""" + +import os +import threading +from datetime import UTC, datetime +from pathlib import Path + +from file_storage import constants +from file_storage.backends.filesystem import FilesystemBackend + + +async def test_stale_objects_skip_parts_and_walk_in_worker(tmp_path, monkeypatch): + backend = FilesystemBackend(tmp_path) + old = backend._resolve("old/object.bin") + fresh = backend._resolve("fresh/object.bin") + short = backend._resolve("x") + part = tmp_path / constants.FS_MULTIPART_DIR / ("a" * 32) / "1" + for path in (old, fresh, short, part): + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(b"data") + os.utime(path, (1, 1)) + os.utime(fresh, (200, 200)) + original_walk = os.walk + caller_thread = threading.get_ident() + walked = [] + + def checked_walk(root): + assert threading.get_ident() != caller_thread + for entry in original_walk(root): + walked.append(Path(entry[0])) + yield entry + + monkeypatch.setattr(os, "walk", checked_walk) + keys = [key async for key in backend.list_stale_objects(datetime.fromtimestamp(100, UTC))] + assert set(keys) == {"old/object.bin"} + assert not any(constants.FS_MULTIPART_DIR in path.parts for path in walked) diff --git a/modules/tenants/tenants/endpoints/views.py b/modules/tenants/tenants/endpoints/views.py index 9a388942f..3b3908199 100644 --- a/modules/tenants/tenants/endpoints/views.py +++ b/modules/tenants/tenants/endpoints/views.py @@ -29,6 +29,7 @@ from tenants.resolver import memberships_for router = APIRouter() +settings_router = APIRouter() def _perm(request: Request, permission: str) -> bool: @@ -97,7 +98,7 @@ async def members( ) -@router.get( +@settings_router.get( "/settings", response_model=None, dependencies=[Depends(RequiresPermission(PERM_TENANT_SETTINGS))], diff --git a/modules/tenants/tenants/module.py b/modules/tenants/tenants/module.py index d4d7bccd7..c857c1a01 100644 --- a/modules/tenants/tenants/module.py +++ b/modules/tenants/tenants/module.py @@ -60,6 +60,13 @@ async def members(db: Any, tenant_id: str, user_ids: Collection[str]) -> set[str class TenantsModule(ModuleBase): + # ModuleBase instances are built per app. register_settings runs before + # register_routes/register_menu_items, so _app is set when those hooks read it. + _app: FastAPI | None = None + + def _settings_available(self) -> bool: + return self._app is not None and getattr(self._app.state, "settings", None) is not None + meta = ModuleMeta( name=c.DISPLAY_NAME, route_prefix="/api/tenants", @@ -88,6 +95,7 @@ def register_settings(self, app: FastAPI) -> None: from tenants.settings import TenantsSettings from tenants.shared_props import tenant_shared_props + self._app = app register_module_settings( app, c.MODULE_PACKAGE, @@ -113,11 +121,14 @@ def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None from tenants.endpoints.admin import api as admin_api from tenants.endpoints.api import router as api from tenants.endpoints.views import router as views + from tenants.endpoints.views import settings_router # Admin first: "/admin" must not be captured by "/{tenant_id}/…". api_router.include_router(admin_api) api_router.include_router(api) view_router.include_router(views) + if self._settings_available(): + view_router.include_router(settings_router) def register_admin_routes(self, admin_router: APIRouter) -> None: from tenants.endpoints.admin import views as admin_views @@ -146,17 +157,18 @@ def register_menu_items(self, registry: MenuRegistry) -> None: permissions=[c.PERM_MEMBERS_VIEW], ) ) - registry.add( - MenuItem( - label="Organisation settings", - label_key="tenants.nav.settings", - url="/tenants/settings", - icon="settings", - order=92, - section=MenuSection.SIDEBAR, - permissions=[c.PERM_TENANT_SETTINGS], + if self._settings_available(): + registry.add( + MenuItem( + label="Organisation settings", + label_key="tenants.nav.settings", + url="/tenants/settings", + icon="settings", + order=92, + section=MenuSection.SIDEBAR, + permissions=[c.PERM_TENANT_SETTINGS], + ) ) - ) registry.add( MenuItem( label="Tenants", diff --git a/modules/tenants/tests/test_tenants_optional_settings.py b/modules/tenants/tests/test_tenants_optional_settings.py new file mode 100644 index 000000000..45138627e --- /dev/null +++ b/modules/tenants/tests/test_tenants_optional_settings.py @@ -0,0 +1,30 @@ +"""Organisation settings are available only with the optional Settings module.""" + +import pytest +from simple_module_hosting.settings import Settings + + +@pytest.fixture(params=[False, True], ids=["without-settings", "with-settings"]) +def settings(settings: Settings, request: pytest.FixtureRequest) -> Settings: + modules = ["Auth", "Users", "Tenants"] + if request.param: + modules.append("Settings") + return settings.model_copy(update={"modules_enabled": modules}) + + +async def test_settings_route_and_menu_follow_module_availability(app, tenant_client): + enabled = getattr(app.state, "settings", None) is not None + registered_urls = {item.url for item in app.state.sm.menu_registry.all_items} + assert ("/tenants/settings" in registered_urls) is enabled + async with tenant_client() as me: + page = await me.client.get("/tenants/", headers={"X-Inertia": "true"}) + assert page.status_code == 200, page.text + urls = {item["url"] for item in page.json()["props"]["menus"]["sidebar"]} + assert "/tenants/members" in urls + assert ("/tenants/settings" in urls) is enabled + + response = await me.client.get("/tenants/settings", headers={"X-Inertia": "true"}) + assert response.status_code == (200 if enabled else 404), response.text + if enabled: + assert response.json()["component"] == "Tenants/Settings" + assert response.json()["props"]["tenant"]["id"] == me.tenant_id diff --git a/modules/users/tests/test_bootstrap.py b/modules/users/tests/test_bootstrap.py index adfaf4483..d69280623 100644 --- a/modules/users/tests/test_bootstrap.py +++ b/modules/users/tests/test_bootstrap.py @@ -247,3 +247,31 @@ async def test_bootstrap_from_env_creates_admin_when_empty_and_configured(users_ assert user.is_superuser is True assert user.is_verified is True assert user.is_active is True + + +@pytest.mark.parametrize("generated", [False, True]) +async def test_bootstrap_logs_generated_password_only_once( + users_app_empty, monkeypatch, caplog, generated +): + if generated: + monkeypatch.setenv("SM_USERS_BOOTSTRAP_PASSWORD_GENERATED", "1") + fake_app = _make_fake_app( + users_app_empty.state.sm.db.session_factory, + bootstrap_email="first@test.example", + bootstrap_password="GeneratedPass1!", + ) + with caplog.at_level("WARNING", logger="users.bootstrap"): + await bootstrap_admin_from_env(fake_app) + first_messages = list(caplog.messages) + caplog.clear() + # A restart generates a different unused password for an existing install. + fake_app.state.users.settings.bootstrap_password = "UnusedPass2!" + await bootstrap_admin_from_env(fake_app) + assert caplog.messages == [] + if generated: + assert first_messages == [ + "first-boot admin created; store this password securely: " + "first@test.example / GeneratedPass1!" + ] + else: + assert first_messages == [] diff --git a/modules/users/users/bootstrap.py b/modules/users/users/bootstrap.py index 9345d5933..addd25378 100644 --- a/modules/users/users/bootstrap.py +++ b/modules/users/users/bootstrap.py @@ -254,7 +254,13 @@ async def bootstrap_admin_from_env(app: FastAPI) -> None: return try: - await create_admin(session, email=email, password=password) + result = await create_admin(session, email=email, password=password) + if result.created and os.environ.get("SM_USERS_BOOTSTRAP_PASSWORD_GENERATED") == "1": + logger.warning( + "first-boot admin created; store this password securely: %s / %s", + email, + password, + ) if user_email and user_password: await create_standard_user( session, diff --git a/scripts/tests/test_scripts_operator_safety.py b/scripts/tests/test_scripts_operator_safety.py new file mode 100644 index 000000000..edaebf6d2 --- /dev/null +++ b/scripts/tests/test_scripts_operator_safety.py @@ -0,0 +1,81 @@ +"""Regression coverage for container credentials and release rehearsal safety.""" + +import os +import subprocess +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +ENTRYPOINT = REPO_ROOT / "docker/entrypoint.sh" +PASSWORD_ENV = "SM_USERS_BOOTSTRAP_PASSWORD" + + +def _run_entrypoint(tmp_path: Path, **overrides: str) -> subprocess.CompletedProcess[str]: + alembic = tmp_path / "alembic" + alembic.write_text('#!/bin/sh\necho "migration-stub"\n') + alembic.chmod(0o755) + env = {key: value for key, value in os.environ.items() if not key.startswith("SM_")} + env.update( + PATH=f"{tmp_path}:{env['PATH']}", + SM_SECRET_KEY="test-secret", + SM_USERS_RESET_PASSWORD_TOKEN_SECRET="test-reset-secret", + SM_USERS_VERIFICATION_TOKEN_SECRET="test-verification-secret", + ) + env.update(overrides) + return subprocess.run( + [ + "sh", + str(ENTRYPOINT), + "sh", + "-c", + f'printf "%s\\n%s" "$SM_USERS_BOOTSTRAP_PASSWORD_GENERATED" "${PASSWORD_ENV}"', + ], + env=env, + capture_output=True, + text=True, + check=False, + ) + + +@pytest.mark.parametrize("password", [None, ""]) +@pytest.mark.parametrize("environment", ["development", "production"]) +def test_scripts_entrypoint_generates_and_marks_private_password( + tmp_path: Path, password: str | None, environment: str +): + overrides = {} if password is None else {PASSWORD_ENV: password} + first = _run_entrypoint(tmp_path, SM_ENVIRONMENT=environment, **overrides) + second = _run_entrypoint(tmp_path, SM_ENVIRONMENT=environment, **overrides) + assert first.returncode == second.returncode == 0 + credential = first.stdout.splitlines()[-1] + assert len(credential) >= 32 + assert credential != second.stdout.splitlines()[-1] + assert credential not in first.stderr + assert "admin@example.com" not in first.stderr + assert first.stdout.splitlines()[-2] == "1" + + +def test_scripts_entrypoint_preserves_private_explicit_password(tmp_path: Path): + password = "operator-provided-password" + result = _run_entrypoint(tmp_path, **{PASSWORD_ENV: password}) + assert result.returncode == 0 + assert result.stdout.splitlines()[-1] == password + assert result.stdout.splitlines()[-2] == "" + assert password not in result.stderr + + +def test_scripts_entrypoint_rejects_public_password_in_production(tmp_path: Path): + result = _run_entrypoint(tmp_path, SM_ENVIRONMENT="production", **{PASSWORD_ENV: "changeme"}) + assert result.returncode != 0 + assert "migration-stub" not in result.stdout + assert PASSWORD_ENV in result.stderr + + +def test_scripts_release_rehearsal_uses_disposable_worktree(): + document = (REPO_ROOT / "docs/release.md").read_text() + rehearsal = document.split("## Local pre-flight", 1)[1].split("## Troubleshooting", 1)[0] + assert "git reset" not in rehearsal + assert "git worktree add --detach" in rehearsal + assert "git worktree remove --force" in rehearsal + assert rehearsal.index("git worktree add") < rehearsal.index("scripts/bump_version.py") + assert rehearsal.index("git worktree remove") > rehearsal.index("uv build") diff --git a/scripts/tests/test_scripts_release_changelog.py b/scripts/tests/test_scripts_release_changelog.py new file mode 100644 index 000000000..0043e91e4 --- /dev/null +++ b/scripts/tests/test_scripts_release_changelog.py @@ -0,0 +1,54 @@ +"""Guard the release changelog cut and its documented heading format.""" + +from pathlib import Path + +import pytest + +from scripts.cut_changelog import cut, main, section_body + +REPO_ROOT = Path(__file__).resolve().parents[2] + + +def test_finalize_cuts_changelog_before_commit_and_extracts_dated_notes() -> None: + workflow = (REPO_ROOT / ".github/workflows/release.yml").read_text(encoding="utf-8") + finalize = workflow.split("\n finalize:\n", 1)[1] + bump = finalize.index("uses: ./.github/actions/bump-version") + enabled = finalize.index('cut-changelog: "true"', bump) + stage = finalize.index("git add -A") + commit = finalize.index('git commit -m "release: v${VERSION}"') + notes = finalize.index( + 'uv run python scripts/cut_changelog.py "${VERSION}" --notes > release-notes.md' + ) + assert bump < enabled < stage < commit < notes + + action = (REPO_ROOT / ".github/actions/bump-version/action.yml").read_text(encoding="utf-8") + cut_step = action.split("- name: Cut CHANGELOG section", 1)[1] + assert "if: inputs.cut-changelog == 'true'" in cut_step + assert 'run: uv run python scripts/cut_changelog.py "${{ inputs.version }}"' in cut_step + assert "--notes" not in cut_step + + +@pytest.mark.parametrize("separator", ["-", "—"]) +def test_cut_and_notes_accept_existing_heading_separators( + tmp_path: Path, capsys: pytest.CaptureFixture[str], separator: str +) -> None: + text = ( + "# Changelog\n\n## [Unreleased]\n\n- new fix\n\n" + f"## [0.0.27] {separator} 2026-08-06\n\n- old fix\n" + ) + path = tmp_path / "CHANGELOG.md" + path.write_text(text, encoding="utf-8") + args = ["0.0.28", "--file", str(path), "--date", "2026-10-22"] + assert main(args) == 0 + released = path.read_text(encoding="utf-8") + assert "## [Unreleased]\n\n## [0.0.28] — 2026-10-22" in released + assert section_body(released, "Unreleased") == "" + assert cut(released, "0.0.28", "2026-10-23") == released + assert main([*args, "--notes"]) == 0 + assert capsys.readouterr().out == "- new fix\n" + assert path.read_text(encoding="utf-8") == released + + +def test_release_docs_match_generated_heading_format() -> None: + docs = (REPO_ROOT / "docs/release.md").read_text(encoding="utf-8") + assert "`## [X.Y.Z] — YYYY-MM-DD`" in docs