diff --git a/API-CONTRACT.md b/API-CONTRACT.md index 8283fbf..ea36657 100644 --- a/API-CONTRACT.md +++ b/API-CONTRACT.md @@ -214,9 +214,55 @@ Creates a new entity. Returns a single entity with full content. #### PUT /entities/:entityId -Updates an entity. +Updates an entity. **PARTIAL update** — see the contract below. -**Request:** Same shape as POST (partial updates supported). +##### The partial-update contract (Chronicle sweep R4, 2026-08-07) + +Every JSON update endpoint in Chronicle now reads a request body three ways: + +| what the body does with a key | what happens to the stored value | +|---|---| +| **absent** | **preserved** | +| present with a **value** | replaced | +| present and **explicitly `null`** | cleared (nullable columns only) | + +The distinction is real on the server, not incidental: the request structs +bind `patch.Field[T]`, which records presence during JSON decoding, so +"absent" and `null` are different things. Non-nullable columns have no +cleared state, so an explicit `null` on one of those preserves rather than +writing a zero. + +This **replaced** the previous behaviour, which had no contract at all — it +was whatever each Go field's type happened to do. Pointer fields preserved +on absence; value-typed fields (`string`, `bool`, `int`) wrote their zero. +Two consequences were live on this module's own traffic: + +- **`is_private` was value-typed.** `actor-sync.mjs` pushes `{name}` alone + on a rename, which bound `is_private = false` and **published a hidden + character entity to every player in the campaign.** The old wording of + this document said absent meant public. Nobody designed that; it was the + Go zero value being read as an intention. +- **`parent_id` was not on the request struct at all**, so every update + from this module detached the entity from the Chronicle hierarchy. + +Both are fixed on the server. Send only the fields you mean to change. + +**Request:** any subset of the POST shape, plus `parent_id`: + +```json +{ "name": "Renamed Character" } +``` + +```json +{ "parent_id": null } +``` +  ↑ explicitly unparents. Omitting `parent_id` leaves the parent alone. + +> **Version skew.** A module talking to a Chronicle older than sweep R4 still +> gets the old whole-replace behaviour. Where a client-side echo already +> exists for that reason — `ChronicleMarkerConfigDialog.#onSave` spreads the +> stored marker under the edited fields — it is kept: it is harmless against +> a merging server and load-bearing against an old one. #### DELETE /entities/:entityId Deletes an entity. @@ -253,9 +299,13 @@ Updates entity permissions. #### POST /entities/:entityId/reveal Toggles entity reveal state (NPC reveal to players). Body is exactly `{ "is_private": }` (a `*bool`); an explicit value matching the current -state is a no-op. This is the ONLY correct way to flip visibility from the -module — a bare `PUT /entities/:id` with just `{is_private}` 400s because -UpdateEntity requires a name. +state is a no-op. This remains the correct way to flip visibility from the +module. + +> Post-sweep-R4, a bare `PUT /entities/:id` with just `{is_private}` no longer +> 400s — an absent name means "not editing the name" rather than "empty name". +> Keep using `/reveal` anyway: it is the named, single-purpose route, and it +> works against every Chronicle version this module supports. **Request:** ```json @@ -620,7 +670,15 @@ Lists map markers (pins/notes on the map). Creates a map marker. #### PUT /maps/:mapId/markers/:markerId -Updates a map marker. +Updates a map marker. **PARTIAL update** — absent preserves, an explicit +`null` clears, a present value replaces (see "The partial-update contract" +under `PUT /entities/:entityId`). + +`foundry_id` is this module's pairing key. It stays clearable HERE — send +`{"foundry_id": null}` to unpair — while Chronicle's own web marker form, +which never sends the key, can no longer NULL it by omission. That omission +used to unpair every marker a GM edited in the browser, which showed up later +as duplicate markers on the next sync. #### DELETE /maps/:mapId/markers/:markerId Deletes a map marker. @@ -852,7 +910,17 @@ Creates a calendar event. - `recurrence_max_occurrences` — Maximum number of recurrences #### PUT /calendar/events/:eventId -Updates a calendar event. Same fields as POST. +Updates a calendar event. Same fields as POST, but it is a **PARTIAL update** +— absent preserves, an explicit `null` clears, a present value replaces (see +"The partial-update contract" under `PUT /entities/:entityId`). + +This is the endpoint `calendar-sync.mjs` pushes note edits to, from three +paths (`_onCalendariaNoteUpdated`, `_onLocalEventUpdate`, +`_onSimpleCalendarNoteUpdate`), each with a five-key body. Before sweep R4 +every one of those pushes also wrote `is_recurring = false`, `all_day = false` +and a cleared `entity_id`, because those were value-typed / clear-on-nil on +the server. They are preserved now. Keep the bodies narrow: a Foundry note +edit means the name, the date and the body, and nothing else. #### DELETE /calendar/events/:eventId Deletes a calendar event. diff --git a/CLAUDE.md b/CLAUDE.md index ce5dcac..20e1bcc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -102,6 +102,33 @@ Integration — Install & Updates". hits a `default:` that logs once per type per session. See `scripts/_calendar-subresources.mjs`, `tools/test-calendar-subresources.mjs`, `tools/test-calendar-subresource-routing.mjs` (FM-SYNC-SUBRESOURCES-P1). +- **Chronicle update endpoints are PARTIAL: absent preserves, an explicit + `null` clears, a present value replaces** (Chronicle sweep R4, 2026-08-07; + API-CONTRACT.md → "The partial-update contract"). Chronicle's request + structs bind `patch.Field[T]`, which records presence during decoding, so + absent and `null` are genuinely different. **Send only the fields you mean + to change**, and do NOT "harden" a narrow body by echoing the untouched + fields back: an echo re-arms the endpoint for the next writer and goes + stale — that is how `ChronicleMarkerConfigDialog` lost the pairing key. The + narrow bodies are pinned by `tools/test-partial-put-contract.mjs` + (`actor-sync`'s `{name}` rename push; `calendar-sync`'s three note-edit + pushes). Before the contract existed, `{name}` alone bound + `is_private=false` and **published a hidden character entity to every + player**, and the calendar pushes turned `is_recurring` and `all_day` off. + The one surviving echo is the marker dialog's spread, kept deliberately: + harmless against a merging server, load-bearing against a pre-R4 one. +- **Never walk a list with a small hard-coded page cap.** The two places that + need every entity in the campaign — `JournalSync.resyncAll` and the + dashboard's `_buildEntityGroups` — both had `while (hasMore && page <= 5)` + inline, a silent 500-entity ceiling: past it entities were never seen, and + the GM got a completed resync and a full-looking dashboard anyway. Both now + share `scripts/_entity-page-walk.mjs`, whose bound is 200 pages and whose + `truncated` flag MUST be surfaced by the caller. A bound is fine; a bound + nobody is told about is the defect. See `tools/test-entity-page-walk.mjs`. + Chronicle's server-side twin (`POST /sync` capped at 1000 with no cursor) + was fixed in Chronicle sweep R4 stage 18 — that endpoint now returns + `next_cursor`, which this module does not yet consume because it pulls via + `GET /entities`, not `POST /sync`. - WebSocket messages are routed by type through `SyncManager`. - Chronicle-side serving rules live in `chronicle-package.json` at repo root; CI validates it against `module.json` via `tools/check-package-descriptor.mjs`. diff --git a/scripts/_entity-page-walk.mjs b/scripts/_entity-page-walk.mjs new file mode 100644 index 0000000..1442d9a --- /dev/null +++ b/scripts/_entity-page-walk.mjs @@ -0,0 +1,77 @@ +/** + * Shared entity-list page walk. + * + * Both places that need "every entity in the campaign" — JournalSync.resyncAll + * and the dashboard's entity groups — had the same loop inline, and both + * stopped after five pages: + * + * while (hasMore && page <= 5) { … per_page=100 … } + * + * That is a hard 500-entity ceiling with no signal. Past it, entities were + * not synced late or partially; they were never seen at all, and the GM was + * shown a completed resync and a full-looking dashboard. Chronicle's own sync + * pull carried the matching ceiling on the server side, fixed in sweep R4 + * stage 18; fixing that half and leaving this one would have left the + * operator exactly as stuck. + * + * This module is pure — no Foundry globals, no api-client import — so it is + * unit-testable and both callers share one implementation instead of two that + * drift. See tools/test-entity-page-walk.mjs. + */ + +/** Page size used for entity list requests. Matches Chronicle's list default. */ +export const ENTITY_PAGE_SIZE = 100; + +/** + * Upper bound on pages walked in one pass: 200 pages x 100 = 20,000 entities. + * + * A bound is still wanted — a broken server that always answers with a full + * page would otherwise spin forever — but it is set where no real campaign + * reaches it, and unlike the old cap, hitting it is REPORTED rather than + * silently swallowed. A ceiling nobody is told about is the actual defect; + * the number is secondary. + */ +export const MAX_ENTITY_PAGES = 200; + +/** + * Walk the entity list to exhaustion. + * + * @param {(page: number, perPage: number) => Promise} fetchPage + * Fetches one page. Receives the 1-based page number and the page size. + * @param {(raw: unknown) => Array} normalize + * Unwraps the response into an array. Chronicle returns some list endpoints + * bare and some enveloped, so every caller must unwrap defensively; the + * caller passes in whichever unwrapper it already owns. + * @param {{pageSize?: number, maxPages?: number}} [opts] + * @returns {Promise<{entities: Array, truncated: boolean, pages: number}>} + * `truncated` is true only when the walk stopped at maxPages with a full + * page still coming back — i.e. entities certainly exist that this pass did + * not see. Callers must surface it. + */ +export async function walkEntityPages(fetchPage, normalize, opts = {}) { + const pageSize = opts.pageSize || ENTITY_PAGE_SIZE; + const maxPages = opts.maxPages || MAX_ENTITY_PAGES; + + const all = []; + let page = 1; + let truncated = false; + + for (;;) { + const batch = normalize(await fetchPage(page, pageSize)) || []; + if (batch.length === 0) break; + all.push(...batch); + + // A short page is the end of the list. Only a FULL page means there may + // be more, which is also why an exact multiple costs one extra request + // rather than guessing. + if (batch.length < pageSize) break; + + page += 1; + if (page > maxPages) { + truncated = true; + break; + } + } + + return { entities: all, truncated, pages: page }; +} diff --git a/scripts/actor-sync.mjs b/scripts/actor-sync.mjs index 8f1fa47..0f13d69 100644 --- a/scripts/actor-sync.mjs +++ b/scripts/actor-sync.mjs @@ -517,6 +517,21 @@ export class ActorSync { // Update name separately if changed, with conflict detection. if (change.name) { + // A rename means ONE thing, so the body carries one field. + // + // Chronicle's PUT /entities/:id is a partial update: an absent key + // preserves, an explicit null clears, a present value replaces + // (API-CONTRACT.md → "The partial-update contract"). Before that + // contract existed, `is_private` was a value-typed bool on the + // server's request struct, so THIS body — {name} alone — bound + // is_private=false and published a hidden character entity to every + // player in the campaign. The fix is the server's; what belongs here + // is the discipline that made it visible. + // + // Do NOT "fix" this by echoing is_private / type_label / parent_id + // back. Echoing re-arms the endpoint for the next writer and would + // reintroduce the same break the moment one of the echoed values is + // stale. Visibility has its own route: POST /entities/:id/reveal. const nameBody = { name: change.name }; const chronicleUpdatedAt = actor.getFlag(FLAG_SCOPE, 'chronicleUpdatedAt'); if (chronicleUpdatedAt) { diff --git a/scripts/calendar-sync.mjs b/scripts/calendar-sync.mjs index fa709e0..340d598 100644 --- a/scripts/calendar-sync.mjs +++ b/scripts/calendar-sync.mjs @@ -1337,6 +1337,13 @@ export class CalendarSync { const date = chronicleDateFromCalendariaStartDate(startDate); if (!date) return null; + // Six keys, and no more. This payload is used for BOTH create and update; + // on the update path Chronicle's PUT /calendar/events/:id is a partial + // update — absent preserves, explicit null clears, a value replaces + // (API-CONTRACT.md). Before that contract, the keys missing here also + // wrote is_recurring=false, all_day=false and a cleared entity_id onto + // the event. Do not widen this to echo them back: an echo re-arms the + // endpoint for the next writer and goes stale. return { name: name || 'Untitled Note', year: date.year, @@ -1460,6 +1467,13 @@ export class CalendarSync { } try { + // Narrow body ON PURPOSE: a Foundry note edit means the name, the date + // and the body. Chronicle's PUT /calendar/events/:id is a partial + // update — absent preserves, explicit null clears, a value replaces + // (API-CONTRACT.md). Before that contract, this exact five-key body + // also wrote is_recurring=false, all_day=false and a cleared entity_id + // onto the event, because those were value-typed / clear-on-nil on the + // server. Do not widen this to echo them back. await this._api.put(`/calendar/events/${chronicleId}`, { name: eventData.name || 'Untitled Event', year: eventData.year, @@ -1557,6 +1571,8 @@ export class CalendarSync { } try { + // Narrow body ON PURPOSE — see _onLocalEventUpdate for why this stays + // five keys and must not grow an echo of the event's other columns. await this._api.put(`/calendar/events/${chronicleId}`, { name: scData.name, year: scData.year, diff --git a/scripts/journal-sync.mjs b/scripts/journal-sync.mjs index 5dbf1b6..70356d9 100644 --- a/scripts/journal-sync.mjs +++ b/scripts/journal-sync.mjs @@ -16,6 +16,7 @@ import { _sanitizeIncomingHTML } from './_html-sanitizer.mjs'; import { defaultLevelForVisibility } from './_ownership.mjs'; import { isCalendarNoteJournal } from './calendar-sync.mjs'; import { _isAllowedImageHost, _describeRejection } from './_url-validation.mjs'; +import { walkEntityPages } from './_entity-page-walk.mjs'; /** * Validate and resolve a Chronicle entity's `image_path` to a safe src string. @@ -182,24 +183,21 @@ export class JournalSync { if (verbose) ui.notifications.info('Chronicle: fetching entities for resync…'); - // Paginated fetch — mirrors the dashboard's _buildEntityGroups page loop. + // Paginated fetch — shares the walk with the dashboard's + // _buildEntityGroups (scripts/_entity-page-walk.mjs). Both used to stop + // after five pages, so a campaign past 500 entities resynced only its + // first 500 and reported a clean finish. let allEntities = []; + let truncated = false; try { - let page = 1; - let hasMore = true; - while (hasMore && page <= 5) { - const result = await this._api.get(`/entities?per_page=100&page=${page}`); - const entities = Array.isArray(result) ? result + const walked = await walkEntityPages( + (page, perPage) => this._api.get(`/entities?per_page=${perPage}&page=${page}`), + (result) => (Array.isArray(result) ? result : (Array.isArray(result?.entities) ? result.entities - : (Array.isArray(result?.data) ? result.data : [])); - if (entities.length > 0) { - allEntities.push(...entities); - hasMore = entities.length === 100; - page++; - } else { - hasMore = false; - } - } + : (Array.isArray(result?.data) ? result.data : []))), + ); + allEntities = walked.entities; + truncated = walked.truncated; } catch (err) { const status = err?.status || null; console.warn(`Chronicle JournalSync.resyncAll: GET /entities failed (${status || 'network'})`, err); @@ -211,6 +209,16 @@ export class JournalSync { console.debug(`Chronicle: resyncAll fetched ${allEntities.length} entity(ies).`); + // A truncated walk means entities exist that this pass never saw. Say so + // — a resync that quietly covers part of the campaign and reports success + // is worse than one that refuses. + if (truncated) { + console.warn(`Chronicle: resyncAll stopped at ${allEntities.length} entities; the campaign has more.`); + ui.notifications.warn( + `Chronicle: resync covered the first ${allEntities.length} entities only — the campaign has more. Run it again or narrow your sync exclusions.` + ); + } + // Build a fast lookup of already-linked journals by chronicle entity id. const journalByEntityId = new Map(); for (const j of game.journal.contents) { diff --git a/scripts/map-viewer.mjs b/scripts/map-viewer.mjs index 4f31e7b..041881a 100644 --- a/scripts/map-viewer.mjs +++ b/scripts/map-viewer.mjs @@ -1170,7 +1170,17 @@ export class ChronicleMarkerConfigDialog extends HandlebarsApplicationMixin(Appl const safeCategory = CHRONICLE_MARKER_CATEGORIES.includes(category) ? category : 'note'; const safeVisibility = VISIBILITY_VALUES.includes(visibility) ? visibility : 'everyone'; + // `PUT /maps/:id/markers/:mid` is a FULL REPLACE: Chronicle binds the body + // into a struct with pointer fields and UPDATEs entity_id, visibility_rules + // and foundry_id unconditionally, so any of those keys missing from the + // body is written back as NULL — clearing the entity link, the per-user + // allow/deny list, and the module's own Foundry pairing key. Spread the + // stored marker under the edited fields (as PinConfigDialog.#onSave above + // does for local pins); the read-only keys the spread carries along + // (id, map_id, created_at, entity_name, …) are undeclared on the wire + // struct and ignored by the binder. const data = { + ...this._marker, name, description, x: this._marker.x, diff --git a/scripts/sync-dashboard.mjs b/scripts/sync-dashboard.mjs index 6a34ea1..e2dde0e 100644 --- a/scripts/sync-dashboard.mjs +++ b/scripts/sync-dashboard.mjs @@ -26,6 +26,7 @@ import { buildDiagnosticBundle } from './sync-diagnostic-bundle.mjs'; import { buildOverviewModel } from './_overview-model.mjs'; import { log, getLogBuffer } from './logger.mjs'; import { shouldSkipDatePush, isRealTimeRejection, notifyRealTimePushPaused } from './_realtime-date-guard.mjs'; +import { walkEntityPages } from './_entity-page-walk.mjs'; import { compareCalendarStructures } from './calendar-sync.mjs'; import { classifyCalendarSyncState } from './_calendar-sync-state.mjs'; import { projectSubresourcePanel } from './_calendar-subresources.mjs'; @@ -365,23 +366,20 @@ export class SyncDashboard extends HandlebarsApplicationMixin(ApplicationV2) { } const types = this._cache.entityTypes; - // Fetch all entities (paginated, up to 500 for now). + // Fetch all entities. Shares the walk with JournalSync.resyncAll + // (scripts/_entity-page-walk.mjs). Both used to stop after five pages, so + // a campaign past 500 entities showed a dashboard that looked complete + // and silently listed none of the rest. if (!this._cache.entities) { - const allEntities = []; - let page = 1; - let hasMore = true; - while (hasMore && page <= 5) { - const result = await this.api.get(`/entities?per_page=100&page=${page}`); - const entities = this._normalizeArray(result, 'entities'); - if (entities.length > 0) { - allEntities.push(...entities); - hasMore = entities.length === 100; - page++; - } else { - hasMore = false; - } + const walked = await walkEntityPages( + (page, perPage) => this.api.get(`/entities?per_page=${perPage}&page=${page}`), + (result) => this._normalizeArray(result, 'entities'), + ); + this._cache.entities = walked.entities; + this._cache.entitiesTruncated = walked.truncated; + if (walked.truncated) { + console.warn(`Chronicle: dashboard entity list stopped at ${walked.entities.length}; the campaign has more.`); } - this._cache.entities = allEntities; } const chronicleEntities = this._cache.entities || []; diff --git a/tools/test-entity-page-walk.mjs b/tools/test-entity-page-walk.mjs new file mode 100644 index 0000000..7b082c1 --- /dev/null +++ b/tools/test-entity-page-walk.mjs @@ -0,0 +1,133 @@ +#!/usr/bin/env node +/** + * Tests for the shared entity-list page walk (scripts/_entity-page-walk.mjs). + * + * The regression: both callers that need "every entity in the campaign" — + * JournalSync.resyncAll and the dashboard's _buildEntityGroups — stopped + * after five pages of 100. That is a hard 500-entity ceiling with no signal. + * Entities past it were not synced late; they were never seen, and the GM was + * shown a completed resync and a full-looking dashboard either way. + * + * Chronicle's server-side sync pull carried the matching ceiling, fixed in + * sweep R4 stage 18. Fixing the server and leaving the client capped at 500 + * would have left the operator exactly as stuck. + * + * Run: `node --test tools/test-entity-page-walk.mjs` + */ + +import test from 'node:test'; +import assert from 'node:assert/strict'; + +import { + walkEntityPages, + ENTITY_PAGE_SIZE, + MAX_ENTITY_PAGES, +} from '../scripts/_entity-page-walk.mjs'; + +/** Serves `total` entities in pages, recording how many requests it saw. */ +function pagedServer(total) { + const calls = []; + return { + calls, + fetchPage: async (page, perPage) => { + calls.push(page); + const start = (page - 1) * perPage; + if (start >= total) return { data: [] }; + const out = []; + for (let i = start; i < Math.min(start + perPage, total); i++) { + out.push({ id: `ent-${String(i).padStart(5, '0')}` }); + } + return { data: out }; + }, + }; +} + +const unwrap = (r) => (Array.isArray(r) ? r : (Array.isArray(r?.data) ? r.data : [])); + +test('walks past the old five-page ceiling and returns every entity', async () => { + const total = 1234; // Well past 5 x 100. + const server = pagedServer(total); + + const { entities, truncated } = await walkEntityPages(server.fetchPage, unwrap); + + assert.equal(entities.length, total, + 'the walk must return every entity; the old cap returned 500 and reported success'); + assert.equal(truncated, false); + assert.equal(entities[0].id, 'ent-00000'); + assert.equal(entities[total - 1].id, `ent-0${total - 1}`, + 'the last entity in the campaign must arrive'); + // No duplicates. + assert.equal(new Set(entities.map((e) => e.id)).size, total); +}); + +test('a short page ends the walk without an extra request', async () => { + const server = pagedServer(42); + const { entities, truncated } = await walkEntityPages(server.fetchPage, unwrap); + assert.equal(entities.length, 42); + assert.equal(truncated, false); + assert.deepEqual(server.calls, [1], 'a single short page must not be followed by a probe'); +}); + +test('an exact page multiple costs one confirming request and does not over-report', async () => { + const server = pagedServer(ENTITY_PAGE_SIZE * 2); + const { entities, truncated } = await walkEntityPages(server.fetchPage, unwrap); + assert.equal(entities.length, ENTITY_PAGE_SIZE * 2); + assert.equal(truncated, false); + assert.deepEqual(server.calls, [1, 2, 3], + 'a full final page cannot be distinguished from more without asking'); +}); + +test('an empty campaign yields nothing and is not reported as truncated', async () => { + const server = pagedServer(0); + const { entities, truncated } = await walkEntityPages(server.fetchPage, unwrap); + assert.deepEqual(entities, []); + assert.equal(truncated, false); +}); + +test('hitting the safety bound REPORTS truncation instead of swallowing it', async () => { + // A server that always answers with a full page — the runaway case the + // bound exists for. The old loop hit its bound and said nothing. + const alwaysFull = async (page, perPage) => ({ + data: Array.from({ length: perPage }, (_, i) => ({ id: `p${page}-${i}` })), + }); + + const { entities, truncated } = await walkEntityPages(alwaysFull, unwrap, { maxPages: 3 }); + + assert.equal(truncated, true, + 'a walk that stopped early must say so; a silent ceiling is the defect'); + assert.equal(entities.length, 3 * ENTITY_PAGE_SIZE); +}); + +test('the shipped bound is far above the old one', () => { + assert.ok(MAX_ENTITY_PAGES > 5, + 'the whole point is that 500 entities is not the ceiling any more'); + assert.ok(MAX_ENTITY_PAGES * ENTITY_PAGE_SIZE >= 20000, + 'the bound should sit past any real campaign'); +}); + +test('the walk honours a custom page size', async () => { + const server = pagedServer(25); + const { entities } = await walkEntityPages(server.fetchPage, unwrap, { pageSize: 10 }); + assert.equal(entities.length, 25); + assert.deepEqual(server.calls, [1, 2, 3]); +}); + +test('a normalizer that returns undefined does not throw', async () => { + const { entities, truncated } = await walkEntityPages( + async () => ({ unexpected: true }), + () => undefined, + ); + assert.deepEqual(entities, []); + assert.equal(truncated, false); +}); + +test('neither caller keeps a hard-coded five-page ceiling', async () => { + const { readFile } = await import('node:fs/promises'); + for (const file of ['../scripts/journal-sync.mjs', '../scripts/sync-dashboard.mjs']) { + const src = await readFile(new URL(file, import.meta.url), 'utf8'); + assert.ok(!/page\s*<=\s*5\b/.test(src), + `${file} still caps its entity walk at five pages`); + assert.ok(src.includes('walkEntityPages'), + `${file} must use the shared walk so the two cannot drift apart again`); + } +}); diff --git a/tools/test-marker-config-payload.mjs b/tools/test-marker-config-payload.mjs new file mode 100644 index 0000000..a50d645 --- /dev/null +++ b/tools/test-marker-config-payload.mjs @@ -0,0 +1,226 @@ +#!/usr/bin/env node +/** + * Regression pin for the partial-PUT marker bug + * (FM-MARKER-DIALOG-PARTIAL-PUT). + * + * `PUT /api/v1/campaigns/:id/maps/:mapID/markers/:markerID` is a FULL + * REPLACE. Chronicle binds the body into `apiUpdateMarkerRequest` (pointer + * fields for the optional columns) and `mapService.UpdateMarker` assigns + * every one of them onto the loaded row before + * `mapRepo.UpdateMarker` UPDATEs `entity_id`, `visibility_rules` and + * `foundry_id` unconditionally. A key absent from the JSON body therefore + * binds to nil and lands on disk as NULL. + * + * `ChronicleMarkerConfigDialog.#onSave` used to rebuild its payload from + * scratch with exactly eight keys — name, description, x, y, pin_category, + * color, icon, visibility — so every GM edit of a Chronicle marker from the + * Foundry map viewer silently cleared: + * + * - `entity_id` the marker → entity link (double-click navigation) + * - `visibility_rules` the per-user allow/deny list + * - `foundry_id` the module's OWN pairing key + * + * The fix spreads the stored marker under the edited fields, matching the + * sibling `PinConfigDialog.#onSave` in the same file. + * + * These tests drive the REAL save action off + * `ChronicleMarkerConfigDialog.DEFAULT_OPTIONS.actions['save-marker']` + * against a stubbed form, and assert on the object handed to the onSave + * callback (which map-sync.mjs passes verbatim as the PUT body). + * + * Run: `node --test tools/test-marker-config-payload.mjs` + */ + +import test from 'node:test'; +import assert from 'node:assert/strict'; + +/* ------------------------------------------------------------------ + Foundry globals — enough for map-viewer.mjs to evaluate at import. + ------------------------------------------------------------------ */ + +class StubApplicationV2 { + constructor(_options = {}) {} + close() {} + render() {} +} + +globalThis.foundry = { + applications: { + api: { ApplicationV2: StubApplicationV2, HandlebarsApplicationMixin: (b) => b }, + // `class X extends undefined` is a TypeError, so the journal page-sheet + // base must be a real class even though these tests never render it. + sheets: { journal: { JournalEntryPageSheet: class {} } }, + }, +}; +globalThis.game = { + user: { isGM: true, id: 'gm' }, + i18n: { localize: (k) => k, format: (k) => k }, + settings: { get: () => undefined, register: () => {} }, + journal: { contents: [] }, + modules: new Map(), +}; +globalThis.ui = { notifications: { warn: () => {}, error: () => {}, info: () => {} } }; +globalThis.Hooks = { on: () => {}, once: () => {}, callAll: () => {} }; +globalThis.CONFIG = { JournalEntryPage: { sheetClasses: {} } }; + +const { ChronicleMarkerConfigDialog } = await import('../scripts/map-viewer.mjs'); + +const SAVE_ACTION = ChronicleMarkerConfigDialog.DEFAULT_OPTIONS.actions['save-marker']; + +/** + * A marker exactly as Chronicle serves it: the editable fields, the three + * optional columns the bug dropped, plus the read-only/joined keys. + */ +function storedMarker(overrides = {}) { + return { + id: 'mk-1', + map_id: 'map-1', + name: 'Yawning Portal', + description: 'A tavern', + x: 12.5, + y: 40.25, + icon: 'fa-note-sticky', + color: '#94A3B8', + pin_category: 'note', + entity_id: 'ent-yawning-portal', + visibility: 'everyone', + visibility_rules: '{"allowed_users":["cu-7"]}', + foundry_id: 'JournalEntryPage.abc123', + created_by: 'user-1', + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-02T00:00:00Z', + entity_name: 'Yawning Portal', + entity_icon: 'fa-beer', + ...overrides, + }; +} + +/** + * Build a dialog whose `element` resolves the marker config form to a stub + * returning the given field values, then invoke the real save action. + * @returns {object} the payload handed to the onSave callback + */ +function runSave(marker, fields) { + let payload = null; + const dialog = new ChronicleMarkerConfigDialog({ + marker: { ...marker }, + mode: 'edit', + onSave: (data) => { payload = data; }, + }); + + const form = { + querySelector: (sel) => { + const m = sel.match(/^\[name="(.+)"\]$/); + const key = m && m[1]; + if (!(key in fields)) throw new Error(`test stub: unexpected form field ${sel}`); + return { value: fields[key] }; + }, + }; + dialog.element = { querySelector: (sel) => (sel === '.chronicle-marker-config-form' ? form : null) }; + + SAVE_ACTION.call(dialog, new Event('click'), null); + assert.ok(payload, 'save action must invoke the onSave callback'); + return payload; +} + +const EDIT_FIELDS = { + name: 'Yawning Portal', + description: 'A tavern', + pin_category: 'quest', + visibility: 'everyone', +}; + +/* ------------------------------------------------------------------ + The bug: optional columns must survive an edit. + ------------------------------------------------------------------ */ + +test('marker save payload carries entity_id through (full-replace PUT would NULL it)', () => { + const payload = runSave(storedMarker(), EDIT_FIELDS); + assert.equal(payload.entity_id, 'ent-yawning-portal'); +}); + +test('marker save payload carries visibility_rules through', () => { + const payload = runSave(storedMarker(), EDIT_FIELDS); + assert.equal(payload.visibility_rules, '{"allowed_users":["cu-7"]}'); +}); + +test('marker save payload carries foundry_id through (the module pairing key)', () => { + const payload = runSave(storedMarker(), EDIT_FIELDS); + assert.equal(payload.foundry_id, 'JournalEntryPage.abc123'); +}); + +test('every optional column Chronicle UPDATEs unconditionally is present in the payload', () => { + // repository.go UpdateMarker writes entity_id / visibility_rules / + // foundry_id with no COALESCE — an absent key is a NULL write. + const payload = runSave(storedMarker(), EDIT_FIELDS); + for (const key of ['entity_id', 'visibility_rules', 'foundry_id']) { + assert.ok(key in payload, `payload must declare ${key}`); + } +}); + +test('a stored marker with null optional columns still round-trips as null (no invention)', () => { + const payload = runSave( + storedMarker({ entity_id: null, visibility_rules: null, foundry_id: null }), + EDIT_FIELDS, + ); + assert.equal(payload.entity_id, null); + assert.equal(payload.visibility_rules, null); + assert.equal(payload.foundry_id, null); +}); + +/* ------------------------------------------------------------------ + The edits themselves must still win over the spread. + ------------------------------------------------------------------ */ + +test('edited fields override the stored values', () => { + const payload = runSave(storedMarker(), { + name: ' The Yawning Portal ', + description: ' Durnan pours ', + pin_category: 'quest', + visibility: 'dm_only', + }); + assert.equal(payload.name, 'The Yawning Portal'); + assert.equal(payload.description, 'Durnan pours'); + assert.equal(payload.pin_category, 'quest'); + assert.equal(payload.visibility, 'dm_only'); + // color + icon are derived from the chosen category, not from the store. + assert.equal(payload.color, '#8B5CF6'); + assert.equal(payload.icon, 'fa-scroll'); +}); + +test('coordinates come from the stored marker (the dialog has no x/y fields)', () => { + const payload = runSave(storedMarker(), EDIT_FIELDS); + assert.equal(payload.x, 12.5); + assert.equal(payload.y, 40.25); +}); + +test('an out-of-range category or visibility still falls back to the safe default', () => { + const payload = runSave(storedMarker(), { + name: 'X', + description: '', + pin_category: 'javascript:alert(1)', + visibility: 'everyone_plus', + }); + assert.equal(payload.pin_category, 'note'); + assert.equal(payload.visibility, 'everyone'); +}); + +test('an empty name falls back to "Marker" (service rejects an empty name)', () => { + const payload = runSave(storedMarker(), { + name: ' ', + description: '', + pin_category: 'note', + visibility: 'everyone', + }); + assert.equal(payload.name, 'Marker'); +}); + +/* ------------------------------------------------------------------ + The spread must not smuggle an optimistic-concurrency token. + ------------------------------------------------------------------ */ + +test('spread does not set expected_updated_at (updated_at is a different wire key)', () => { + const payload = runSave(storedMarker(), EDIT_FIELDS); + assert.equal(payload.expected_updated_at, undefined, + 'apiUpdateMarkerRequest reads expected_updated_at; spreading updated_at must not engage concurrency'); +}); diff --git a/tools/test-partial-put-contract.mjs b/tools/test-partial-put-contract.mjs new file mode 100644 index 0000000..6e74b65 --- /dev/null +++ b/tools/test-partial-put-contract.mjs @@ -0,0 +1,166 @@ +#!/usr/bin/env node +/** + * Source-level pins for the module's half of Chronicle's partial-update + * contract (Chronicle sweep R4, 2026-08-07). + * + * The contract, documented in API-CONTRACT.md → "The partial-update + * contract": an ABSENT key preserves the stored value, an EXPLICIT null + * clears it, a present value replaces it. Chronicle's request structs bind + * `patch.Field[T]`, which records presence during JSON decoding, so absent + * and null are genuinely different. + * + * That contract is what makes this module's narrow bodies SAFE. Before it, + * they were data loss: + * + * - `actor-sync.mjs` pushes `{name}` alone on a rename. Chronicle's + * `apiUpdateEntityRequest.IsPrivate` was a value-typed `bool`, so the + * absent key bound `false` and PUBLISHED a hidden character entity to + * every player in the campaign. The struct had no `parent_id` member at + * all, so the same push also detached the entity from the hierarchy. + * - `calendar-sync.mjs` pushes five-key bodies from three paths. Each also + * wrote `is_recurring=false`, `all_day=false` and a cleared `entity_id`. + * + * Both were fixed on the server. What this file defends is the OTHER + * direction: that nobody "repairs" these clients by echoing the untouched + * fields back. An echo re-arms the endpoint for the next writer, and it goes + * stale — which is exactly how the marker dialog lost the pairing key. + * + * These are source-level assertions, not runtime ones, because the thing + * being pinned is the SHAPE of a request body that a hook builds — the same + * reason Chronicle pins its own templ clients by reading their source. + * + * Run: `node --test tools/test-partial-put-contract.mjs` + */ + +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; + +const repoRoot = join(dirname(fileURLToPath(import.meta.url)), '..'); +const read = (rel) => readFileSync(join(repoRoot, rel), 'utf8'); + +/** + * Returns the top-level keys of the first object literal that starts at + * `startIndex`, by walking braces so nested objects do not leak keys. + */ +function topLevelKeys(src, startIndex) { + const open = src.indexOf('{', startIndex); + assert.notEqual(open, -1, 'no object literal found'); + let depth = 0; + let end = -1; + for (let i = open; i < src.length; i++) { + const ch = src[i]; + if (ch === '{' || ch === '[' || ch === '(') depth++; + else if (ch === '}' || ch === ']' || ch === ')') { + depth--; + if (depth === 0) { end = i; break; } + } + } + assert.notEqual(end, -1, 'unterminated object literal'); + const body = src.slice(open + 1, end); + + // Strip nested literals and comments so only top-level keys remain. + const stripped = body + .replace(/\/\/[^\n]*/g, '') + .replace(/\/\*[\s\S]*?\*\//g, ''); + const keys = []; + let d = 0; + let line = ''; + for (const ch of stripped) { + if (ch === '{' || ch === '[' || ch === '(') d++; + if (ch === '}' || ch === ']' || ch === ')') d--; + if (ch === ',' && d === 0) { line = ''; continue; } + if (d === 0) { + line += ch; + if (ch === ':') { + const m = line.match(/([A-Za-z_][A-Za-z0-9_]*)\s*:$/); + if (m) keys.push(m[1]); + line = ''; + } + } + } + return keys.sort(); +} + +test('actor-sync: a rename pushes only {name}', () => { + const src = read('scripts/actor-sync.mjs'); + const idx = src.indexOf('const nameBody ='); + assert.notEqual(idx, -1, 'nameBody literal not found — did the rename push move?'); + assert.deepEqual( + topLevelKeys(src, idx), + ['name'], + 'the rename push must carry ONLY name. Echoing is_private / type_label / parent_id back ' + + 're-arms the endpoint for the next writer and goes stale; visibility has its own route ' + + '(POST /entities/:id/reveal).' + ); +}); + +test('actor-sync: the only other key on the rename push is the concurrency token', () => { + const src = read('scripts/actor-sync.mjs'); + // expected_updated_at is optimistic concurrency, not a data field — it is + // added conditionally after construction, so it never appears in the + // literal above. Anything ELSE assigned onto nameBody would be a data write. + const assignments = [...src.matchAll(/nameBody\.([A-Za-z_][A-Za-z0-9_]*)\s*=/g)].map((m) => m[1]); + assert.deepEqual( + [...new Set(assignments)].sort(), + ['expected_updated_at'], + 'something other than the concurrency token is being assigned onto the rename body' + ); +}); + +test('calendar-sync: the Calendaria note payload stays six keys', () => { + const src = read('scripts/calendar-sync.mjs'); + const idx = src.indexOf("name: name || 'Untitled Note',"); + assert.notEqual(idx, -1, 'the Calendaria note payload moved'); + const open = src.lastIndexOf('return {', idx); + assert.deepEqual( + topLevelKeys(src, open), + ['day', 'description', 'month', 'name', 'visibility', 'year'].sort(), + 'the Calendaria note payload grew or shrank. A Foundry note edit means the name, the date, ' + + 'the body and the visibility — the server preserves everything absent, so echoing more is ' + + 'stale data waiting to be written.' + ); +}); + +test('calendar-sync: every inline PUT body to /calendar/events stays five keys', () => { + const src = read('scripts/calendar-sync.mjs'); + // Find the PUT call sites by their URL, not by a field name — the create + // path uses the same field expressions and would otherwise be matched. + const marker = 'this._api.put(`/calendar/events/'; + const bodies = []; + for (let i = src.indexOf(marker); i !== -1; i = src.indexOf(marker, i + 1)) { + const comma = src.indexOf(', ', src.indexOf('`,', i)); + const after = src.slice(comma + 2, comma + 3); + if (after !== '{') continue; // e.g. the Calendaria path, which passes a variable + bodies.push(topLevelKeys(src, comma + 2)); + } + assert.equal(bodies.length, 2, 'expected exactly two inline PUT bodies (legacy Calendaria + SimpleCalendar)'); + for (const keys of bodies) { + assert.deepEqual( + keys, + ['day', 'description', 'month', 'name', 'year'], + 'an update push changed shape; keep it to what a Foundry note edit means. The server ' + + 'preserves every absent key now, so echoing more is stale data waiting to be written.' + ); + } +}); + +test('the contract is documented where the endpoints are described', () => { + // Whitespace-normalised: the doc is hard-wrapped, so a phrase may straddle + // a line break without having changed. + const doc = read('API-CONTRACT.md').replace(/\s+/g, ' '); + for (const phrase of [ + 'The partial-update contract', + 'an explicit `null`', + 'published a hidden character entity to every player', + '`parent_id` was not on the request struct at all', + ]) { + assert.ok( + doc.includes(phrase), + `API-CONTRACT.md no longer states ${JSON.stringify(phrase)}. The wire semantics of an ` + + 'absent key are the whole contract; a module author who cannot read them here will guess.' + ); + } +});