From e51bcdbf653e9f2cde5fdeabdc4584d20bafad1f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 07:58:46 +0000 Subject: [PATCH 1/3] sweep R3 stage 3: the foundry marker dialog stops wiping entity, visibility rules and pairing key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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), `mapService.UpdateMarker` assigns every one of them onto the loaded row, and `mapRepo.UpdateMarker` UPDATEs `entity_id`, `visibility_rules` and `foundry_id` with no COALESCE. A key absent from the JSON body therefore binds to nil and lands on disk as NULL. `ChronicleMarkerConfigDialog.#onSave` rebuilt 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 the marker's entity link, its per-user allow/deny list, and the module's own Foundry pairing key. Spread the stored marker under the edited fields, matching the sibling `PinConfigDialog.#onSave` in the same file (and the Chronicle web UI's own marker form, which carries entity_id on the PUT). The dialog already holds a full copy of the marker. 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; `updated_at` is not `expected_updated_at`, so the spread does not accidentally engage optimistic concurrency. tools/test-marker-config-payload.mjs drives the real save action off DEFAULT_OPTIONS.actions['save-marker'] against a stubbed form and pins all three columns through an edit, plus the edits-win, safe-fallback and no-concurrency-token behaviours. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Aa645128uaFxbQyvabMizq --- scripts/map-viewer.mjs | 10 ++ tools/test-marker-config-payload.mjs | 226 +++++++++++++++++++++++++++ 2 files changed, 236 insertions(+) create mode 100644 tools/test-marker-config-payload.mjs 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/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'); +}); From f3ffa900363c862b442c5c5b9125903b0182ec2a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 11:29:00 +0000 Subject: [PATCH 2/3] sweep R4 stage 6: the module's narrow bodies are safe now, and the contract says why MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chronicle's cross-repo half of this sweep made every JSON update endpoint a partial update: an absent key preserves, an explicit null clears, a present value replaces. That is what makes this module's bodies correct rather than lossy. Before it, actor-sync's {name} rename push bound is_private=false on the server and PUBLISHED a hidden character entity to every player, and calendar-sync's five-key note pushes turned is_recurring and all_day off and cleared the entity link. API-CONTRACT.md said the opposite — that an absent is_private meant public. It says the contract now, at the endpoints it governs (entities, calendar events, map markers), with the version-skew note that a pre-R4 Chronicle still whole-replaces. The client change is discipline, not payload. Each narrow body carries a comment saying it is narrow ON PURPOSE and must not be "hardened" by echoing untouched fields back — an echo re-arms the endpoint for the next writer and goes stale, which is exactly how the marker dialog lost its pairing key. tools/test-partial-put-contract.mjs pins the shapes by parsing the real object literals. The marker dialog's spread stays: harmless against a merging server, load-bearing against an older one. Red-then-green: adding is_private/parent_id back to nameBody reddens "actor-sync: a rename pushes only {name}". Full suite 770/770. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Aa645128uaFxbQyvabMizq --- API-CONTRACT.md | 82 ++++++++++++-- CLAUDE.md | 15 +++ scripts/actor-sync.mjs | 15 +++ scripts/calendar-sync.mjs | 16 +++ tools/test-partial-put-contract.mjs | 166 ++++++++++++++++++++++++++++ 5 files changed, 287 insertions(+), 7 deletions(-) create mode 100644 tools/test-partial-put-contract.mjs 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..aab525a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -102,6 +102,21 @@ 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. - 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/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/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.' + ); + } +}); From 0d17f9c6391c161f4315e2398ce76e87d747167d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 21:29:40 +0000 Subject: [PATCH 3/3] sweep R4 stage 19: the module's own resync stopped at 500 and said it was done MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chronicle stage 18 lifted the server-side ceiling on the sync pull. The module carried the matching one on its own side, and fixing only the server would have left the operator exactly as stuck. Both places that need every entity in the campaign had this loop inline: while (hasMore && page <= 5) { … per_page=100 … } JournalSync.resyncAll and the dashboard's _buildEntityGroups. A hard 500-entity ceiling with no signal in either. Past it, entities were not synced slowly or partially — they were never fetched, and the GM was shown a completed resync and a full-looking dashboard regardless. The dashboard's own comment even said "up to 500 for now", which is the kind of note that stops being read. Replaced both with the shared, pure scripts/_entity-page-walk.mjs. It keeps a bound — a broken server that always answers with a full page would otherwise spin forever — but sets it at 200 pages (20,000 entities), past any real campaign, and returns `truncated` when it stops early. A bound is fine. A bound nobody is told about is the defect. resyncAll now warns the GM by name when the walk was truncated: "resync covered the first N entities only — the campaign has more". The dashboard warns to console and records it on its cache. One implementation rather than two, for the same reason _realtime-date-guard and _calendar-subresources exist: two copies of a walk drift, and this pair had already drifted in their unwrappers while sharing the same wrong ceiling. tools/test-entity-page-walk.mjs, proven red then green by restoring MAX_ENTITY_PAGES=5 and dropping the truncated flag: - 1234 entities all arrive, in order, with no duplicates, including the last. With the old ceiling: "expected 500 to equal 1234". - a short page ends the walk with no extra request; an exact multiple costs one confirming request, because a full final page cannot be told from more without asking. - hitting the bound sets truncated. With the old loop it did not, which was the whole bug: "a walk that stopped early must say so". - an empty campaign, a custom page size, and a normalizer returning undefined. - a source pin: neither caller may keep a `page <= 5` literal, and both must route through walkEntityPages so they cannot drift apart again. CLAUDE.md records the rule and points at Chronicle's server-side twin, including the honest note that this module does not consume the new next_cursor because it pulls via GET /entities rather than POST /sync. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Aa645128uaFxbQyvabMizq --- CLAUDE.md | 12 +++ scripts/_entity-page-walk.mjs | 77 ++++++++++++++++++ scripts/journal-sync.mjs | 38 +++++---- scripts/sync-dashboard.mjs | 28 ++++--- tools/test-entity-page-walk.mjs | 133 ++++++++++++++++++++++++++++++++ 5 files changed, 258 insertions(+), 30 deletions(-) create mode 100644 scripts/_entity-page-walk.mjs create mode 100644 tools/test-entity-page-walk.mjs diff --git a/CLAUDE.md b/CLAUDE.md index aab525a..20e1bcc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -117,6 +117,18 @@ Integration — Install & Updates". 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/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/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`); + } +});