From 07328a3412835ad3b2c2744200f3847f66f8568f Mon Sep 17 00:00:00 2001 From: peter Date: Wed, 2 Sep 2026 13:38:59 +0100 Subject: [PATCH] Fix issues #002-#006 found by the LLM test harness - #006 (high, data loss): make_room_for() no longer deletes unkept artifacts on a doomed request - checks whether eviction can possibly succeed before deleting anything. Also fixed a related latent bug found while testing: the function returned evicted Artifact objects after calling .delete() on them, so their .id was always None (Django clears pk on delete); now returns ids captured beforehand. - #003: added server-side field-length validation (source/title/ source_name/source_id/theme) on POST and PATCH, matching the model's own max_length limits, which were previously unenforced on the ingest path entirely. - #004: ported daybook_post.py's content-shape validation (html_fragment/html_full/deck/markdown) to the server, closing the gap for any client that doesn't go through the helper script. - #005: PATCH now rejects wrong-typed `tags` (previously silently wiped existing tags) and rejects immutable fields (content/source/kind/day) with 400 instead of silently no-op'ing them. - #002: deleted the misleading skills/daybook-posting/.env.example (a zero-dependency script that never reads .env files) and clarified SKILL.md/docs/llm_posting_helper.md that these must be real process/shell env vars. Adds tests/unit/ for storage-layer tests. 46 tests passing (was 25). Co-Authored-By: Claude Sonnet 5 --- artifacts/storage.py | 22 +++-- artifacts/views.py | 61 +++++++++++++ docs/api.md | 63 ++++++++------ docs/llm_posting_helper.md | 5 ++ issues/002-env-example-not-actually-loaded.md | 54 ++++++++++++ ...03-model-max-length-silently-unenforced.md | 68 +++++++++++++++ ...gment-content-not-validated-server-side.md | 71 +++++++++++++++ ...patch-silently-mishandles-invalid-input.md | 72 ++++++++++++++++ .../006-failed-post-still-evicts-artifacts.md | 81 +++++++++++++++++ skills/daybook-posting/SKILL.md | 5 +- tests/api/test_artifacts.py | 82 ++++++++++++++++++ tests/unit/__init__.py | 0 tests/unit/test_storage.py | 86 +++++++++++++++++++ 13 files changed, 635 insertions(+), 35 deletions(-) create mode 100644 issues/002-env-example-not-actually-loaded.md create mode 100644 issues/003-model-max-length-silently-unenforced.md create mode 100644 issues/004-html-fragment-content-not-validated-server-side.md create mode 100644 issues/005-patch-silently-mishandles-invalid-input.md create mode 100644 issues/006-failed-post-still-evicts-artifacts.md create mode 100644 tests/unit/__init__.py create mode 100644 tests/unit/test_storage.py diff --git a/artifacts/storage.py b/artifacts/storage.py index 36e7069..fda50b8 100644 --- a/artifacts/storage.py +++ b/artifacts/storage.py @@ -76,8 +76,10 @@ def user_quota_bytes(user): def make_room_for(user, incoming_bytes): """Evict oldest unkept artifacts until `incoming_bytes` fits under quota. - Returns the list of evicted artifacts (already deleted). Raises QuotaExceeded - if even removing every unkept artifact leaves no room. + Returns the ids of evicted artifacts (already deleted - Django clears an + instance's pk on .delete(), so ids are captured beforehand). Raises + QuotaExceeded, without deleting anything, if even evicting every unkept + artifact wouldn't free enough room. """ quota = user_quota_bytes(user) if incoming_bytes > quota: @@ -87,21 +89,27 @@ def make_room_for(user, incoming_bytes): if used + incoming_bytes <= quota: return [] - evicted = [] - unkept = ( + unkept = list( Artifact.objects.filter(owner=user, is_kept=False).order_by("created_at") ) + # Check whether eviction can possibly succeed before deleting anything - + # otherwise a doomed request would still destroy every unkept artifact on + # its way to failing anyway. + evictable_total = sum(artifact_total_bytes(artifact) for artifact in unkept) + if used - evictable_total + incoming_bytes > quota: + raise QuotaExceeded("Quota is full of kept artifacts; free space to post.") + + evicted = [] for artifact in unkept: if used + incoming_bytes <= quota: break # Freeing an artifact also frees the assets embedded in it. used -= artifact_total_bytes(artifact) + evicted_id = artifact.id delete_artifact_files(artifact) artifact.delete() - evicted.append(artifact) + evicted.append(evicted_id) - if used + incoming_bytes > quota: - raise QuotaExceeded("Quota is full of kept artifacts; free space to post.") return evicted diff --git a/artifacts/views.py b/artifacts/views.py index 9ee640c..f9eaf4b 100644 --- a/artifacts/views.py +++ b/artifacts/views.py @@ -47,6 +47,9 @@ from .storage import ( TEXT_KINDS = {Artifact.HTML_FRAGMENT, Artifact.HTML_FULL, Artifact.MARKDOWN, Artifact.DECK} +# Affects storage layout, renderer choice, or feed ordering - never editable after create. +IMMUTABLE_PATCH_FIELDS = {"content", "source", "kind", "day"} + REMEMBER_ME_SESSION_AGE = 60 * 60 * 24 * 30 # 30 days MAX_ASSET_BYTES = 50 * 1024 * 1024 # 50 MB per uploaded image @@ -112,6 +115,10 @@ class ArtifactListCreateView(APIView): if source_error: return source_error + content_error = validate_content_shape(kind, data["content"]) + if content_error: + return content_error + artifact = create_text_artifact(data, request, user) if isinstance(artifact, Response): return artifact @@ -206,11 +213,22 @@ class ArtifactDetailView(APIView): artifact = get_object_or_404(Artifact, pk=artifact_id, owner=user) data = request.data + not_editable = IMMUTABLE_PATCH_FIELDS & set(data.keys()) + if not_editable: + return Response( + {"detail": f"Not editable via PATCH: {', '.join(sorted(not_editable))}"}, + status=400, + ) + length_error = validate_field_lengths(data, ["title", "source_name", "source_id"]) + if length_error: + return length_error if "title" in data: if not data["title"]: return Response({"detail": "title cannot be blank"}, status=400) artifact.title = data["title"] if "tags" in data: + if not isinstance(data["tags"], (str, list)): + return Response({"detail": "tags must be a string or an array"}, status=400) artifact.tags = normalize_tags(data["tags"]) if "source_prompt" in data: artifact.source_prompt = data["source_prompt"] or "" @@ -691,7 +709,50 @@ def labels_with_colors(artifact, overrides=None): return labels_with_colors_list([artifact.source, *artifact.tags], overrides) +FIELD_LENGTH_LIMITS = { + "source": 80, + "title": 200, + "source_name": 120, + "source_id": 120, + "theme": 40, +} + + +def validate_field_lengths(data, fields): + for field in fields: + value = data.get(field) + if value and len(str(value)) > FIELD_LENGTH_LIMITS[field]: + return Response( + {"detail": f"{field} must be {FIELD_LENGTH_LIMITS[field]} characters or fewer"}, + status=400, + ) + return None + + +def validate_content_shape(kind, content): + lower = (content or "").lower() + if kind == Artifact.DECK and "... blocks"}, status=400 + ) + if kind == Artifact.HTML_FRAGMENT and any(tag in lower for tag in (""}, status=400 + ) + if kind == Artifact.MARKDOWN and not (content or "").strip(): + return Response({"detail": "markdown content cannot be blank"}, status=400) + return None + + def validate_source_fields(data): + length_error = validate_field_lengths(data, ["source", "title", "source_name", "source_id", "theme"]) + if length_error: + return length_error if source_type := data.get("source_type"): if source_type not in {choice[0] for choice in Artifact.SOURCE_TYPE_CHOICES}: return Response({"detail": "source_type must be agent, person, or system"}, status=400) diff --git a/docs/api.md b/docs/api.md index 97c1503..f2a986b 100644 --- a/docs/api.md +++ b/docs/api.md @@ -83,22 +83,28 @@ Required fields: | Field | Notes | |---|---| -| `source` | Required. | +| `source` | Required. Max 80 characters. | | `kind` | One of `html_fragment`, `html_full`, `markdown`, `deck`. | -| `title` | Required. | -| `content` | Required text content. | +| `title` | Required. Max 200 characters. | +| `content` | Required text content. Must match the shape `kind` implies — see below. | + +`content` shape is validated per `kind`: `html_fragment` must not contain +` -``` - -Removes the artifact's database row and its files (primary file, markdown -sidecar, and any attached assets). Returns `204` with no body on success. - -You can only delete artifacts your own token owns; others return `404`, as -does a second delete of the same id. - -```bash -curl -X DELETE "http://127.0.0.1:8001/api/artifacts/a1b2c3d4" \ - -H "Authorization: Bearer " -``` - -Not editable yet: +Not editable — sending any of these returns `400` naming the field, rather +than silently ignoring it: | Field | Reason | |---|---| @@ -383,6 +372,26 @@ Invoke-RestMethod ` -Body $body ``` +## Delete Artifact + +Endpoint: + +```http +DELETE /api/artifacts/{id} +Authorization: Bearer +``` + +Removes the artifact's database row and its files (primary file, markdown +sidecar, and any attached assets). Returns `204` with no body on success. + +You can only delete artifacts your own token owns; others return `404`, as +does a second delete of the same id. + +```bash +curl -X DELETE "http://127.0.0.1:8001/api/artifacts/a1b2c3d4" \ + -H "Authorization: Bearer " +``` + ## Rendered And Raw Content Rendered browser view: diff --git a/docs/llm_posting_helper.md b/docs/llm_posting_helper.md index cd8cf1d..0062790 100644 --- a/docs/llm_posting_helper.md +++ b/docs/llm_posting_helper.md @@ -8,6 +8,11 @@ The helper is intentionally zero-dependency and uses only Python's standard libr ## Configuration +Set these as real process/shell environment variables — the helper reads +`os.environ` directly and has no `.env` file support. Copying values into a +`.env` file in the working directory does nothing; export/`$env:`/`set` +them, or set them in your agent harness's own env config. + ```text DAYBOOK_URL=http://127.0.0.1:8001 DAYBOOK_TOKEN=your-personal-api-token diff --git a/issues/002-env-example-not-actually-loaded.md b/issues/002-env-example-not-actually-loaded.md new file mode 100644 index 0000000..6b09c4e --- /dev/null +++ b/issues/002-env-example-not-actually-loaded.md @@ -0,0 +1,54 @@ +# 002 — `.env.example` implies persistent config that `daybook_post.py` never loads + +**Status:** Fixed + +## Description + +`skills/daybook-posting/.env.example` ships alongside the posting helper: + +``` +DAYBOOK_URL=http://127.0.0.1:8001 +DAYBOOK_TOKEN=your-personal-api-token +DAYBOOK_SOURCE=agent +``` + +The presence of a `.env.example` file conventionally means "copy this to +`.env`, fill it in, and the tool picks it up automatically." That's not +true here. `daybook_post.py` is intentionally zero-dependency (no +`python-dotenv`, no manual `.env` parsing) and only reads real process +environment variables via `os.environ.get(...)` (confirmed: the only +`.env`-related string in the whole `skills/` tree is this example file +itself — nothing in `daybook_post.py` reads it). + +Found while doing a first interactive posting session against the `test` +account from a sibling project (`AGENT_DAYBOOK__LLM_TEST`): copying +`.env.example` to `.env` and populating it did nothing; the script still +required the vars to be exported into the shell (or set with `set`/ +`$env:` on Windows) for every new session/terminal. Neither `SKILL.md` +nor `docs/llm_posting_helper.md` mention this — both just say "use +environment variables" without clarifying that a `.env` file specifically +will not work unless the caller's shell/agent harness sources it itself. + +This is minor but wastes a round trip for exactly the audience this skill +targets (an agent following the docs literally). + +## Suggested Fix (pick one) + +- Simplest: delete `.env.example`, or rename/annotate it clearly as "copy + these lines into your shell profile / CI secrets / agent harness env + config — this file itself is not read automatically." +- Alternative: add ~10 lines of manual `.env` parsing to `daybook_post.py` + (still zero third-party deps — just read `.env` in the working directory + if present, without overriding already-set real env vars) if the goal + really is per-project persistent config. + +## Resolution + +Took the simplest option: deleted `skills/daybook-posting/.env.example` +entirely rather than adding `.env` parsing to a helper that's deliberately +zero-dependency. Its content was already duplicated in both `SKILL.md` and +`docs/llm_posting_helper.md`'s Configuration sections, so nothing was lost. +Also reworded both of those sections to say explicitly that these must be +real process/shell environment variables — the helper reads `os.environ` +directly and has no `.env` file support — rather than just "use environment +variables" without ruling out the natural `.env`-file assumption. diff --git a/issues/003-model-max-length-silently-unenforced.md b/issues/003-model-max-length-silently-unenforced.md new file mode 100644 index 0000000..2ca9996 --- /dev/null +++ b/issues/003-model-max-length-silently-unenforced.md @@ -0,0 +1,68 @@ +# 003 — `CharField(max_length=...)` limits are silently unenforced on ingest + +**Status:** Fixed + +## Description + +`artifacts/models.py` declares length caps on several `Artifact` fields — +`source` (80), `title` (200), `source_name` (120), `source_id` (120), +`theme` (40) — but nothing on the ingest path enforces them. `views.py`'s +`create_text_artifact`/`create_binary_artifact` call +`Artifact.objects.create(...)` directly, which does **not** run Django's +`full_clean()`/validators. SQLite (the DB in both dev and production per +`docs/deployment.md` line 197) also doesn't enforce `VARCHAR(n)` length as +a hard constraint on `INSERT`, so oversized values are written and read +back verbatim with no error at any layer. + +Confirmed against the local `test` account: + +1. `POST /api/artifacts` with a 250-character `title` (limit 200) → + `201`, title stored and returned in full, uncut. +2. `POST /api/artifacts` with a 150-character `source` (limit 80) → + `201`, `source` stored in full. Because `source_name` defaults to + `source` when omitted (`source_fields()` in `views.py`), this also + silently blew past `source_name`'s own 120-char limit in the same + request. + +For contrast, other validation on the same endpoint works correctly and +returns a clean `400` — e.g. an invalid `tag_colors` hex value +(`{"tag_colors": {"urgent": "notacolor"}}` → `400` with a helpful +message). So the endpoint clearly has a validation convention; these +five fields just aren't part of it. + +Practically: a title long enough could break feed-row layout in the UI; +`source` feeds directly into the on-disk storage path +(`artifacts_store/{source}/{day}/...`) and the sidebar/label system, so +an extreme value there has a larger blast radius than just cosmetics. + +## Reproduction + +```bash +curl -X POST http://127.0.0.1:8001/api/artifacts \ + -H "Authorization: Bearer " \ + -H "Content-Type: application/json" \ + -d "{\"source\":\"edge-case-test\",\"title\":\"$(python3 -c 'print("A"*250)')\",\"kind\":\"markdown\",\"content\":\"x\"}" +# -> 201, title returned uncut at 250 chars +``` + +## Suggested Fix + +Add explicit length checks alongside the existing required-field/kind +checks in `ArtifactListCreateView.post` and `ArtifactDetailView.patch` +(and the two `create_*_artifact` helpers), returning `400` with a message +naming the field and its limit — consistent with how `tag_colors` and +`source_type` are already handled. A shared `validate_field_lengths(data)` +helper (mirroring `validate_source_fields`) covering `source`, `title`, +`source_name`, `source_id`, `theme` would keep it in one place. + +## Resolution + +Added `validate_field_lengths(data, fields)` in `artifacts/views.py`, called +from `validate_source_fields` (covers `source`, `title`, `source_name`, +`source_id`, `theme` on both text and binary `POST`, since both paths already +routed through that function) and directly in `ArtifactDetailView.patch` +(covers `title`, `source_name`, `source_id` — the only length-limited fields +`PATCH` can touch). Returns `400` naming the field and its limit, matching +the existing `tag_colors`/`source_type` convention. Tests added in +`tests/api/test_artifacts.py` covering oversized `title`/`source` on create, +an exact-limit boundary case, and oversized `title` on patch. diff --git a/issues/004-html-fragment-content-not-validated-server-side.md b/issues/004-html-fragment-content-not-validated-server-side.md new file mode 100644 index 0000000..5c2e492 --- /dev/null +++ b/issues/004-html-fragment-content-not-validated-server-side.md @@ -0,0 +1,71 @@ +# 004 — `html_fragment`/`deck`/`html_full` content-shape rules exist only in the client helper, not the server + +**Status:** Fixed + +## Description + +`docs/api.md` and `skills/daybook-posting/SKILL.md` both describe a +content-shape contract per text `kind`: + +- `html_fragment` — "Wrapped with a Daybook theme" (body-only HTML, no + `doctype`/`html`/`body`). +- `html_full` — complete document, must contain ``. +- `deck` — top-level `
` blocks. + +`skills/daybook-posting/scripts/daybook_post.py`'s `validate_payload()` +enforces all three of these client-side before ever sending the request. +**`artifacts/views.py`'s `ArtifactListCreateView.post` / `create_text_artifact` +enforce none of them** — the only server-side checks are "kind is one of +the 4 text kinds" and "required fields present." Any client that talks to +`POST /api/artifacts` directly (curl, a different language, a +misconfigured agent) bypasses the contract entirely, and the server +accepts and stores whatever is sent. + +Confirmed: posting `kind: "html_fragment"` with a full +`......` as +`content` returns `201`, and the stored page (`GET /a/{id}`) is: + +```html + + + + ... + nested html fragment + + + +Injected

Nested doc

+ + +``` + +i.e. a doctype and a full `` nested inside the theme +wrapper's own `` — invalid markup that browsers will +silently "fix up" in unpredictable ways (duplicate/conflicting ``, +nested `<head>`, etc.), rather than the clean, themed fragment the kind +is supposed to guarantee. + +This is the same class of gap as #003: validation the docs promise is +implemented only in the optional client helper, not the server that's +supposed to be the actual contract enforcement point. Related to (but +distinct from) issue #003, which is about unenforced *length* limits +rather than unenforced *content shape*. + +## Suggested Fix + +Port `daybook_post.py`'s `validate_payload()` checks (or equivalent) into +`create_text_artifact`/`ArtifactListCreateView.post` server-side, returning +`400` with the same messages the helper already uses. This also closes the +gap for any future non-Python client. + +## Resolution + +Added `validate_content_shape(kind, content)` in `artifacts/views.py`, +ported directly from `daybook_post.py`'s `validate_payload()` (same rules, +same case-insensitive substring checks) so client and server agree: `deck` +requires `<section`, `html_fragment` rejects `<html`/`<!doctype`/`<body`, +`html_full` requires `<html`, and `markdown` rejects whitespace-only content +(which the old required-field check missed, since a non-empty +whitespace-only string is truthy). Called from `ArtifactListCreateView.post` +after the existing field/source validation. Tests added covering each +rejection case plus a well-formed `html_fragment` still succeeding. diff --git a/issues/005-patch-silently-mishandles-invalid-input.md b/issues/005-patch-silently-mishandles-invalid-input.md new file mode 100644 index 0000000..5fb034d --- /dev/null +++ b/issues/005-patch-silently-mishandles-invalid-input.md @@ -0,0 +1,72 @@ +# 005 — `PATCH /api/artifacts/{id}` silently mishandles invalid/immutable input instead of rejecting it + +**Status:** Fixed + +## Description + +`ArtifactDetailView.patch` (`artifacts/views.py`) applies whatever +recognized keys are present in the request body without validating their +type or rejecting fields it can't act on. Two confirmed effects: + +**1. A wrong-typed `tags` value silently wipes existing tags (data loss).** + +`normalize_tags()` returns `[]` for anything that isn't a `str` or +`list`. `ArtifactDetailView.patch` calls it unconditionally whenever +`"tags"` is a key in the request body, with no type check first. + +Reproduced: + +```bash +# Artifact has tags ["important", "keep-me"] +curl -X PATCH .../api/artifacts/a74de286 -d '{"tags": 12345}' ... +# -> 200, tags silently become [] +``` + +An agent that sends a malformed `tags` value (e.g. a stray int from a bad +template substitution) gets a `200` and loses the artifact's existing +tags with no indication anything went wrong. + +**2. Immutable fields (`content`, `source`, `kind`, `day`) are silently +no-ops instead of rejected.** + +`docs/api.md`'s "Not editable yet" table lists these fields and why, but +doesn't say what happens if a client sends them anyway. In practice: +`PATCH` with `{"content": "...", "source": "...", "kind": "...", "day": +"..."}` returns `200` with the artifact **unchanged** — the fields are +silently dropped, not rejected. An agent attempting to fix a typo in +`content` via `PATCH` (a very natural thing to try, since `PATCH` is the +only mutate-after-create endpoint) gets a success response that looks +like it worked but did nothing. + +Both are instances of the same root cause: `patch()` doesn't validate +before applying, unlike `POST`'s `validate_source_fields()`. + +## Suggested Fix + +- Type-check `tags` (must be `str` or `list`) before calling + `normalize_tags`; return `400` on other types instead of silently + emptying it. Same principle likely applies to `metadata` (already + checked) and could apply to `source_prompt`/`source_name`/`source_id` + (currently accept anything via implicit str coercion via Django, but + worth a pass). +- Either make sending an immutable field (`content`/`source`/`kind`/`day`) + in a `PATCH` body a `400` ("field is not editable"), or explicitly + document that unknown/immutable keys are ignored — right now the + behavior is undocumented and easy to mistake for success. + +## Resolution + +Went with explicit rejection for both, over silent-ignore-and-document: + +- `tags` is now type-checked (`str` or `list`) before `normalize_tags` runs; + any other type returns `400` and leaves existing tags untouched. +- Sending `content`, `source`, `kind`, or `day` in a `PATCH` body now returns + `400` naming the offending field(s), via a new `IMMUTABLE_PATCH_FIELDS` + check at the top of `ArtifactDetailView.patch`, before any other + processing — chosen over documenting the no-op because a `200` that does + nothing is easy to mistake for success. + +`docs/api.md`'s "Not editable" table now says so explicitly instead of just +listing reasons. Tests added: wrong-typed `tags` is rejected and leaves +existing tags intact, and each of the four immutable fields is rejected +individually. diff --git a/issues/006-failed-post-still-evicts-artifacts.md b/issues/006-failed-post-still-evicts-artifacts.md new file mode 100644 index 0000000..a5d56d1 --- /dev/null +++ b/issues/006-failed-post-still-evicts-artifacts.md @@ -0,0 +1,81 @@ +# 006 — A `POST` that ultimately fails with 413 still permanently deletes unkept artifacts + +**Status:** Fixed +**Severity:** High — irreversible data loss on a failed request + +## Description + +`make_room_for()` (`artifacts/storage.py`) is called from `create_text_artifact` +/`create_binary_artifact` *before* the new artifact is persisted. When the +incoming artifact doesn't fit even after evicting every unkept artifact, it +raises `QuotaExceeded` and the view returns `413`. But the eviction loop +inside `make_room_for` deletes each unkept artifact **as it iterates**, +*before* it can know whether the eventual total will succeed: + +```python +for artifact in unkept: + if used + incoming_bytes <= quota: + break + used -= artifact_total_bytes(artifact) + delete_artifact_files(artifact) + artifact.delete() + evicted.append(artifact) + +if used + incoming_bytes > quota: + raise QuotaExceeded("Quota is full of kept artifacts; free space to post.") +``` + +If the loop runs out of unkept artifacts to evict and still hasn't freed +enough room (because the remaining `is_kept=True` artifacts alone exceed +the new/current quota), it raises *after* having already deleted every +unkept artifact — there's no rollback. The caller gets a `413` that reads +like "nothing happened, free up space and retry," but in reality **every +unkept artifact the user had was just destroyed** for a request that +still failed. + +Reproduced against the `test` account (quota temporarily lowered to +demonstrate an unwinnable state — 1 kept artifact at 851 bytes vs. an +800-byte quota): + +``` +BEFORE: 1 kept artifact (851B) + 5 unkept artifacts (582/321/311/401/428B) +POST small markdown -> 413 "Quota is full of kept artifacts; free space to post." +AFTER: only the 1 kept artifact remains — all 5 unkept artifacts are gone + (both DB rows and on-disk files, confirmed via Django shell) +``` + +This is a normal, reachable state — not just an artificially shrunk quota. +Any user whose kept artifacts alone approach their quota will hit this the +moment they try to post anything that doesn't fit, and lose every unkept +artifact they had in the process, with a `413` response giving no +indication that a destructive side effect occurred. + +## Suggested Fix + +`make_room_for` should compute whether eviction *can* succeed before +deleting anything — e.g. sum `unkept` sizes up front and compare +`used - unkept_total + incoming_bytes` against `quota`; raise +`QuotaExceeded` immediately if even a hypothetical full eviction wouldn't +fit, without touching any artifact. Only perform actual deletions once +it's known they'll be sufficient. This makes the failure path side-effect +free, matching what a `413` response implies. + +## Resolution + +Fixed in `artifacts/storage.py`'s `make_room_for`: computes the total +recoverable bytes across all unkept artifacts up front and raises +`QuotaExceeded` immediately if even a full eviction wouldn't fit — no +artifact is touched on that path. The incremental eviction loop only runs +once success is already guaranteed. + +While testing this fix, found and fixed a related latent bug: the function +returned the evicted `Artifact` *objects* after calling `.delete()` on them, +but Django clears an instance's pk on delete, so every returned object's +`.id` was silently `None`. Nothing currently reads this return value, so it +had no live impact, but it broke the function's own documented contract. +Now returns the evicted ids (captured before deletion) instead of objects. + +Regression tests added in `tests/unit/test_storage.py`: an unwinnable +eviction leaves all unkept artifacts intact, a successful eviction only +evicts as many (oldest-first) as needed, and an over-quota single artifact +deletes nothing. diff --git a/skills/daybook-posting/SKILL.md b/skills/daybook-posting/SKILL.md index d3fa546..8eee070 100644 --- a/skills/daybook-posting/SKILL.md +++ b/skills/daybook-posting/SKILL.md @@ -24,7 +24,10 @@ The artifact URL is printed as the final stdout line. ## Configuration -Use environment variables: +Set these as real process/shell environment variables (`export` on +Linux/macOS, `$env:` or `set` on Windows, or your agent harness's own env +config) — the helper reads `os.environ` directly. There is no `.env` file +support; a `.env` file in the working directory is not read automatically. ```text DAYBOOK_URL=http://127.0.0.1:8001 diff --git a/tests/api/test_artifacts.py b/tests/api/test_artifacts.py index a2c7e71..333b527 100644 --- a/tests/api/test_artifacts.py +++ b/tests/api/test_artifacts.py @@ -274,3 +274,85 @@ class ArtifactAPITestCase(APITestCase): detail_response = self.client.get(reverse("artifact-detail-api", args=[artifact_id])) self.assertEqual(detail_response.status_code, 404) + + # -- field length limits (issue #003) ---------------------------------- + + def test_create_rejects_oversized_title(self): + response = self.create_artifact(title="A" * 201) + self.assertEqual(response.status_code, 400) + self.assertIn("title", response.data["detail"]) + + def test_create_rejects_oversized_source(self): + response = self.create_artifact(source="a" * 81) + self.assertEqual(response.status_code, 400) + self.assertIn("source", response.data["detail"]) + + def test_create_accepts_title_at_exact_limit(self): + response = self.create_artifact(title="A" * 200) + self.assertEqual(response.status_code, 201) + + def test_patch_rejects_oversized_title(self): + artifact_id = self.create_artifact().data["id"] + response = self.client.patch( + reverse("artifact-detail-api", args=[artifact_id]), + {"title": "A" * 201}, + format="json", + **self.auth_headers(self.token), + ) + self.assertEqual(response.status_code, 400) + + # -- content-shape validation (issue #004) ----------------------------- + + def test_create_rejects_nested_html_in_html_fragment(self): + response = self.create_artifact( + kind="html_fragment", content="<html><body>nested</body></html>" + ) + self.assertEqual(response.status_code, 400) + + def test_create_rejects_html_full_without_html_tag(self): + response = self.create_artifact(kind="html_full", content="<p>not a full document</p>") + self.assertEqual(response.status_code, 400) + + def test_create_rejects_deck_without_section(self): + response = self.create_artifact(kind="deck", content="<h1>Not a deck slide</h1>") + self.assertEqual(response.status_code, 400) + + def test_create_rejects_whitespace_only_markdown(self): + response = self.create_artifact(kind="markdown", content=" \n\t ") + self.assertEqual(response.status_code, 400) + + def test_create_accepts_well_formed_html_fragment(self): + response = self.create_artifact(kind="html_fragment", content="<p>fine</p>") + self.assertEqual(response.status_code, 201) + + # -- PATCH validation (issue #005) ------------------------------------- + + def test_patch_rejects_non_string_non_list_tags(self): + artifact_id = self.create_artifact(tags=["important", "keep-me"]).data["id"] + response = self.client.patch( + reverse("artifact-detail-api", args=[artifact_id]), + {"tags": 12345}, + format="json", + **self.auth_headers(self.token), + ) + self.assertEqual(response.status_code, 400) + + artifact = Artifact.objects.get(pk=artifact_id) + self.assertEqual(artifact.tags, ["important", "keep-me"]) + + def test_patch_rejects_immutable_fields(self): + artifact_id = self.create_artifact().data["id"] + for field, value in [ + ("content", "new content"), + ("source", "other-source"), + ("kind", "deck"), + ("day", "2020-01-01"), + ]: + response = self.client.patch( + reverse("artifact-detail-api", args=[artifact_id]), + {field: value}, + format="json", + **self.auth_headers(self.token), + ) + self.assertEqual(response.status_code, 400, field) + self.assertIn(field, response.data["detail"]) diff --git a/tests/unit/__init__.py b/tests/unit/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/unit/test_storage.py b/tests/unit/test_storage.py new file mode 100644 index 0000000..ddc075d --- /dev/null +++ b/tests/unit/test_storage.py @@ -0,0 +1,86 @@ +import shutil +import tempfile +from pathlib import Path + +from django.contrib.auth import get_user_model +from django.test import TestCase, override_settings + +from artifacts.models import Artifact, UserProfile +from artifacts.storage import QuotaExceeded, make_room_for + +User = get_user_model() + + +@override_settings() +class MakeRoomForTestCase(TestCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.artifacts_root = Path(tempfile.mkdtemp(prefix="daybook-test-storage-")) + cls.settings_override = override_settings(ARTIFACTS_ROOT=cls.artifacts_root) + cls.settings_override.enable() + + @classmethod + def tearDownClass(cls): + cls.settings_override.disable() + shutil.rmtree(cls.artifacts_root, ignore_errors=True) + super().tearDownClass() + + def setUp(self): + self.user = User.objects.create_user(username="quota-test", password="pw") + + def make_artifact(self, artifact_id, size_bytes, is_kept=False): + return Artifact.objects.create( + id=artifact_id, + owner=self.user, + source="test", + title=f"Artifact {artifact_id}", + kind=Artifact.MARKDOWN, + day="2026-01-01", + path=f"test/2026-01-01/{artifact_id}.html", + original_kind=Artifact.MARKDOWN, + size_bytes=size_bytes, + is_kept=is_kept, + ) + + def set_quota(self, quota_bytes): + UserProfile.objects.update_or_create(user=self.user, defaults={"quota_bytes": quota_bytes}) + + def test_unwinnable_eviction_deletes_nothing(self): + """Regression test for issue #006: a doomed post must not destroy + unkept artifacts on its way to still failing.""" + self.set_quota(1000) + self.make_artifact("kept0001", size_bytes=900, is_kept=True) + self.make_artifact("unkept01", size_bytes=50) + self.make_artifact("unkept02", size_bytes=40) + + with self.assertRaises(QuotaExceeded): + make_room_for(self.user, incoming_bytes=200) + + self.assertTrue(Artifact.objects.filter(id="unkept01").exists()) + self.assertTrue(Artifact.objects.filter(id="unkept02").exists()) + self.assertTrue(Artifact.objects.filter(id="kept0001").exists()) + + def test_successful_eviction_frees_only_what_is_needed(self): + self.set_quota(1000) + self.make_artifact("kept0001", size_bytes=500, is_kept=True) + self.make_artifact("oldest01", size_bytes=200) + self.make_artifact("newest01", size_bytes=200) + Artifact.objects.filter(id="oldest01").update(created_at="2026-01-01T00:00:00Z") + Artifact.objects.filter(id="newest01").update(created_at="2026-06-01T00:00:00Z") + + evicted = make_room_for(self.user, incoming_bytes=300) + + self.assertEqual(set(evicted), {"oldest01"}) + self.assertFalse(Artifact.objects.filter(id="oldest01").exists()) + self.assertTrue(Artifact.objects.filter(id="newest01").exists()) + self.assertTrue(Artifact.objects.filter(id="kept0001").exists()) + + def test_incoming_larger_than_quota_deletes_nothing(self): + self.set_quota(1000) + self.make_artifact("unkept01", size_bytes=50) + + with self.assertRaises(QuotaExceeded): + make_room_for(self.user, incoming_bytes=2000) + + self.assertTrue(Artifact.objects.filter(id="unkept01").exists())