From 35ed517340e10a5e8cb628fc2ddb398bbb868963 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Thu, 3 Sep 2026 16:20:00 +0000 Subject: [PATCH] refactor: break manuscriptReview <-> manuscriptFix static import cycle (#5918) Move the review-comment store (schema, sanitizers, persistence, accessors) into pipeline/manuscriptComments.js. manuscriptReview.js keeps seed orchestration and re-exports the store API; manuscriptFix.js reads comments through the leaf. Drops the #5918 baseline entry from the import-cycle ratchet. --- .../services/pipeline/manuscriptComments.js | 313 +++++++++++++++++ server/services/pipeline/manuscriptFix.js | 6 +- .../pipeline/manuscriptFixUndo.test.js | 2 +- server/services/pipeline/manuscriptReview.js | 320 ++---------------- server/services/serviceImportCycles.test.js | 2 - 5 files changed, 340 insertions(+), 303 deletions(-) create mode 100644 server/services/pipeline/manuscriptComments.js diff --git a/server/services/pipeline/manuscriptComments.js b/server/services/pipeline/manuscriptComments.js new file mode 100644 index 0000000000..6e6faa9d0e --- /dev/null +++ b/server/services/pipeline/manuscriptComments.js @@ -0,0 +1,313 @@ +/** + * Pipeline — Manuscript Review comment store ("Finish the draft" comments) + * + * Leaf module owning the persisted editorial-findings comment set the user + * works through in the manuscript editor: each comment can be jumped-to, given + * an AI fix, edited, and accepted into the manuscript (or dismissed). Findings + * are otherwise ephemeral — this is what makes "Finish the draft" actionable + * across reloads. + * + * Stored as a sibling of the series record at + * `data/pipeline-series/{id}/manuscript-review.json`, so it travels with the + * series folder on share/sync without bloating the LWW-merged series + * `index.json` (the review is an independent, larger document with its own + * write cadence). Writes serialize on a per-series tail (single tail per shared + * file, per AGENTS.md). + * + * This split breaks the former manuscriptReview <-> manuscriptFix static + * import cycle (#5918): both manuscriptReview.js (seed/orchestration) and + * manuscriptFix.js (fix generation/accept) import the store accessors from + * here, and manuscriptReview.js imports only the fix SHAPERS from + * manuscriptFix.js. Nothing in this module's dependency closure imports it + * back — keep it that way (no imports from manuscriptReview.js or + * manuscriptFix.js). + */ + +import { join } from 'path'; +import { randomUUID } from 'crypto'; +import { atomicWrite, readJSONFile } from '../../lib/fileUtils.js'; +import { createKeyedFileWriteQueue } from '../../lib/fileWriteQueue.js'; +import { seriesStore, listSeries } from './series.js'; +import { REPLACEMENT_STRATEGIES, replacementStrategyForCategory } from './arcPlanner.js'; +import { emitRecordUpdated } from '../sharing/recordEvents.js'; + +// Storage-layout version for the review document. Bump + migrate if the +// comment shape changes in a way older peers can't read. +export const SCHEMA_VERSION = 1; + +export const COMMENT_STATUSES = Object.freeze(['open', 'accepted', 'dismissed']); +const STATUS_SET = new Set(COMMENT_STATUSES); + +// Why a dismissal was made (#1605). A plain dismiss (`dismissReason: null`) +// means "won't fix" — the finding is real but the user is leaving it. A +// `false-positive` reason means "this check is wrong here" — it feeds the +// per-check quality view so broken checks are tracked instead of silently +// re-surfacing the same bad finding every run. Only meaningful when +// `status === 'dismissed'`. Optional + additive, so the synced review doc stays +// backward-compatible with older peers (who simply ignore the field). +export const DISMISS_REASONS = Object.freeze(['false-positive']); +const DISMISS_REASON_SET = new Set(DISMISS_REASONS); + +const REVIEW_FILE = 'manuscript-review.json'; +const reviewPath = (seriesId) => join(seriesStore().recordDir(seriesId), REVIEW_FILE); + +// Per-series write tail (the review file is distinct per series, so each only +// serializes against itself). One canonical single-tail queue per series id. +export const queueReviewWrite = createKeyedFileWriteQueue(); + +const emptyReview = () => ({ schemaVersion: SCHEMA_VERSION, comments: [] }); + +const clampStr = (v, max) => (typeof v === 'string' ? v.trim().slice(0, max) : ''); + +function sanitizeFix(raw) { + if (!raw || typeof raw !== 'object') return null; + const find = typeof raw.find === 'string' ? raw.find : ''; + const replace = typeof raw.replace === 'string' ? raw.replace : ''; + const edits = Array.isArray(raw.edits) + ? raw.edits + .map((e) => { + if (!e || typeof e !== 'object') return null; + const editFind = typeof e.find === 'string' ? e.find : ''; + const editReplace = typeof e.replace === 'string' ? e.replace : ''; + if (!editFind && !editReplace) return null; + const out = { + issueNumber: Number.isInteger(e.issueNumber) ? e.issueNumber : null, + issueId: typeof e.issueId === 'string' ? e.issueId : null, + stageId: typeof e.stageId === 'string' ? e.stageId : null, + title: clampStr(e.title, 200), + find: editFind, + replace: editReplace, + note: clampStr(e.note, 1000), + }; + if (e.fuzzy === true) out.fuzzy = true; + return out; + }) + .filter(Boolean) + : []; + if (!find && !replace && edits.length === 0) return null; + const out = { find, replace }; + if (edits.length) out.edits = edits; + if (raw.fuzzy === true) out.fuzzy = true; + return out; +} + +// Snapshot of the manuscript text a fix overwrote, captured at accept-time so +// the finding can be undone back to its pre-edit state (#1609). `priorText` is +// the section's FULL stage text before the fix applied (full restore handles +// every edit shape — multi-edit, deletion, full-page rewrite — exactly, unlike +// a span-level reverse-splice); `appliedHash` is a fingerprint of the text the +// accept WROTE, so undo can detect later edits and refuse to clobber them. Only +// carried while `status === 'accepted'` (a status flip drops it, mirroring +// `dismissReason`). Optional + additive → older peers ignore it and the synced +// review doc stays backward-compatible (no schema bump needed). +function sanitizeAcceptedSnapshot(raw) { + if (!raw || typeof raw !== 'object' || !Array.isArray(raw.sections)) return null; + const sections = raw.sections + .map((s) => { + if (!s || typeof s !== 'object') return null; + const issueId = typeof s.issueId === 'string' && s.issueId ? s.issueId : null; + const stageId = typeof s.stageId === 'string' && s.stageId ? s.stageId : null; + const priorText = typeof s.priorText === 'string' ? s.priorText : null; + if (!issueId || !stageId || priorText == null) return null; + const out = { issueId, stageId, priorText }; + if (typeof s.appliedHash === 'string' && s.appliedHash) out.appliedHash = s.appliedHash; + return out; + }) + .filter(Boolean); + if (!sections.length) return null; + return { + acceptedAt: typeof raw.acceptedAt === 'string' ? raw.acceptedAt : new Date().toISOString(), + sections, + }; +} + +// Shape one stored comment. Tolerant of partial/legacy records so a hand-edited +// or older-peer file round-trips without dropping fields. +export function sanitizeComment(raw) { + if (!raw || typeof raw !== 'object') return null; + const problem = clampStr(raw.problem, 2000); + if (!problem) return null; + const category = clampStr(raw.category, 40) || 'other'; + const severity = ['high', 'medium', 'low'].includes(raw.severity) ? raw.severity : 'medium'; + const status = STATUS_SET.has(raw.status) ? raw.status : 'open'; + return { + id: typeof raw.id === 'string' && raw.id ? raw.id : `mrc-${randomUUID()}`, + issueNumber: Number.isInteger(raw.issueNumber) ? raw.issueNumber : null, + issueId: typeof raw.issueId === 'string' ? raw.issueId : null, + stageId: typeof raw.stageId === 'string' ? raw.stageId : null, + severity, + // The check's NATIVE per-finding level (#1596), independent of any per-check + // severity override. Carried so that clearing an override re-grades the + // finding back to its true native level (not a guessed default). Defaults to + // `severity` for legacy/older-peer comments written before this field (all of + // which predate overrides, so native == severity is correct). Optional + + // additive → the synced review doc stays backward-compatible. + nativeSeverity: ['high', 'medium', 'low'].includes(raw.nativeSeverity) ? raw.nativeSeverity : severity, + category, + // Optional per-check sub-classification of the finding (#1626) — e.g. + // `dialogue.on-the-nose` tags each finding `exposition` / `emotion-tell` / + // `relationship-report` so the editor sees *why* a line reads on-the-nose. + // `null` for checks that don't sub-classify, legacy records, and older peers. + // Validated by the producing check against its own allow-list, so a stray + // value never reaches here; the clamp is a belt-and-suspenders bound for + // hand-edited / older-peer files. Optional + additive → the synced review doc + // stays backward-compatible (no schema bump needed). + subtype: clampStr(raw.subtype, 40) || null, + location: clampStr(raw.location, 200), + problem, + suggestion: clampStr(raw.suggestion, 8000), + // How `suggestion` should be read: 'full-page' = it's a complete replacement + // document (comic-structure panel rewrite); 'delta' = it's advice. Trust a + // valid stored value, else derive from category so legacy comments (written + // before this field existed) and older peers still classify correctly. + replacementStrategy: REPLACEMENT_STRATEGIES.has(raw.replacementStrategy) + ? raw.replacementStrategy + : replacementStrategyForCategory(category), + anchorQuote: clampStr(raw.anchorQuote, 400), + // Which editorial check produced this finding (#1284). `null` for findings + // from the manuscript-completeness pass (and older peers / legacy records) + // — those predate the registry, so they group as a single un-checked set. + // Optional + additive, so the synced review doc stays backward-compatible. + checkId: typeof raw.checkId === 'string' && raw.checkId ? raw.checkId : null, + // Fingerprint of the content the editorial check analyzed when it raised this + // finding (#1345) — the runner stamps it so the editor can flag the finding + // `stale` once the manuscript/canon drifts. `null` for completeness-pass + // findings, older peers, and legacy records (treated as never-stale). + // Optional + additive, so the synced review doc stays backward-compatible. + sourceContentHash: typeof raw.sourceContentHash === 'string' && raw.sourceContentHash ? raw.sourceContentHash : null, + status, + // Dismissal reason (#1605) — only carried while dismissed. A status flip + // back to open/accepted drops it here so a re-opened finding can't keep a + // stale `false-positive` mark. Legacy/older-peer records lack the field and + // sanitize to `null` (a plain "won't fix" dismiss). + dismissReason: raw.status === 'dismissed' && DISMISS_REASON_SET.has(raw.dismissReason) + ? raw.dismissReason + : null, + fix: sanitizeFix(raw.fix), + // Pre-edit snapshot for undo (#1609) — only meaningful once accepted, so a + // re-open/dismiss flip drops it (the undo path also clears it explicitly). + acceptedSnapshot: status === 'accepted' + ? sanitizeAcceptedSnapshot(raw.acceptedSnapshot) + : null, + sourceRunId: typeof raw.sourceRunId === 'string' ? raw.sourceRunId : null, + createdAt: typeof raw.createdAt === 'string' ? raw.createdAt : new Date().toISOString(), + updatedAt: typeof raw.updatedAt === 'string' ? raw.updatedAt : new Date().toISOString(), + }; +} + +function sanitizeReview(raw) { + if (!raw || typeof raw !== 'object' || !Array.isArray(raw.comments)) return emptyReview(); + return { + schemaVersion: SCHEMA_VERSION, + comments: raw.comments.map(sanitizeComment).filter(Boolean), + }; +} + +export async function readReview(seriesId) { + // `null` = file absent (distinct from a present-but-empty review). + const raw = await readJSONFile(reviewPath(seriesId), null); + return raw == null ? emptyReview() : sanitizeReview(raw); +} + +export async function writeReview(seriesId, review) { + await atomicWrite(reviewPath(seriesId), sanitizeReview(review)); +} + +/** + * Read the persisted review for a series. Returns an empty review (never null) + * when none has been generated yet. + */ +export async function getReview(seriesId) { + return readReview(seriesId); +} + +/** + * Patch a single comment (status flip, attach/clear a generated fix, edit the + * replacement text). Last-write-wins on `updatedAt`. Returns the updated + * comment, or throws if the id is unknown. + */ +export async function updateComment(seriesId, commentId, patch) { + return queueReviewWrite(seriesId, async () => { + const review = await readReview(seriesId); + const idx = review.comments.findIndex((c) => c.id === commentId); + if (idx === -1) { + throw Object.assign(new Error(`Comment not found: ${commentId}`), { code: 'PIPELINE_REVIEW_NOT_FOUND' }); + } + const cur = review.comments[idx]; + const merged = { ...cur }; + if (patch.status !== undefined && STATUS_SET.has(patch.status)) merged.status = patch.status; + // Dismissal reason (#1605). `null` is an explicit clear; a valid reason sets + // it. sanitizeComment below drops it anyway when the resulting status isn't + // `dismissed`, so re-opening a false-positive can't leave the mark behind. + if (patch.dismissReason !== undefined) { + merged.dismissReason = DISMISS_REASON_SET.has(patch.dismissReason) ? patch.dismissReason : null; + } + // `fix: null` is an explicit clear; absent leaves it untouched. + if (patch.fix !== undefined) merged.fix = sanitizeFix(patch.fix); + // Pre-edit undo snapshot (#1609). `null` is an explicit clear (the undo + // path); absent leaves it untouched. sanitizeComment below drops it anyway + // whenever the resulting status isn't `accepted`, so re-opening a finding + // can't leave a stale snapshot behind. + if (patch.acceptedSnapshot !== undefined) merged.acceptedSnapshot = patch.acceptedSnapshot; + merged.updatedAt = new Date().toISOString(); + const next = { ...review, comments: review.comments.map((c, i) => (i === idx ? sanitizeComment(merged) : c)) }; + await writeReview(seriesId, next); + // Sibling-doc change → fire a series `updated` event so the review + // propagates to peers / re-exports to subscribed buckets (see + // seedReviewFromFindings in manuscriptReview.js). + emitRecordUpdated('series', seriesId); + return next.comments[idx]; + }); +} + +/** + * Sync-orchestrator entry: merge a remote peer's review into local state, + * last-write-wins per comment on `updatedAt`. Mirrors `mergeIssuesFromSync`. + */ +export async function mergeReviewFromSync(seriesId, remoteReview) { + const remote = sanitizeReview(remoteReview); + return queueReviewWrite(seriesId, async () => { + const local = await readReview(seriesId); + const byId = new Map(local.comments.map((c) => [c.id, c])); + for (const rc of remote.comments) { + const lc = byId.get(rc.id); + // Strict-newer (`>`) so an equal-clock echo is a skip, matching the + // `mergeIssuesFromSync` LWW guard. With `>=`, a peer re-sending a comment + // we already hold at the same timestamp re-adopts + re-writes it every + // sync cycle (write amplification + non-convergence) — the same bug + // catalogSync.js fixed this release. + if (!lc || new Date(rc.updatedAt).getTime() > new Date(lc.updatedAt).getTime()) { + byId.set(rc.id, rc); + } + } + const next = { schemaVersion: SCHEMA_VERSION, comments: [...byId.values()] }; + await writeReview(seriesId, next); + return next; + }); +} + +// Internal: used by manuscriptFix to read a single comment without a full +// round-trip ceremony. Returns null when absent. +export async function getComment(seriesId, commentId) { + const review = await readReview(seriesId); + return review.comments.find((c) => c.id === commentId) || null; +} + +/** + * Locate a finding/comment across ALL series by its (globally-unique) comment + * id. Findings live per-series in each series' manuscript-review.json, so a + * deep-link that carries only a commentId (e.g. one shared from elsewhere) has + * to resolve the owning series before the editor can open it (#1608). Returns + * `{ seriesId, comment }` for the first series whose review contains the id, or + * `null` when no series owns it. Comment ids are UUID-based (`randomUUID`) so a + * match is unambiguous; deleted series are skipped (listSeries default). + */ +export async function locateComment(commentId) { + if (typeof commentId !== 'string' || !commentId) return null; + const series = await listSeries(); + for (const s of series) { + const comment = await getComment(s.id, commentId); + if (comment) return { seriesId: s.id, comment }; + } + return null; +} diff --git a/server/services/pipeline/manuscriptFix.js b/server/services/pipeline/manuscriptFix.js index c1e330e2b8..7032f57531 100644 --- a/server/services/pipeline/manuscriptFix.js +++ b/server/services/pipeline/manuscriptFix.js @@ -8,8 +8,8 @@ * replacement before accepting; accept applies the selected edits through the * serialized stage-write path (which snapshots the prior text into runHistory). * - * Read this alongside manuscriptReview.js (where the comment + fix persist) and - * arcPlanner.js (which owns the completeness pass that creates the comments). + * Read this alongside manuscriptComments.js (where the comment store persists) + * and arcPlanner.js (which owns the completeness pass that creates the comments). */ import { randomUUID, createHash } from 'crypto'; @@ -18,7 +18,7 @@ import { planManuscriptPass, estimateTokens } from '../../lib/contextBudget.js'; import { getSeries, MANUSCRIPT_TYPES } from './series.js'; import { getIssue, updateStageWithLatest, updateStagesWithLatest } from './issues.js'; import { collectManuscriptSections, stageVersionsOf, sectionsCorpus, manuscriptSectionHeader } from './arcPlanner.js'; -import { getComment, updateComment } from './manuscriptReview.js'; +import { getComment, updateComment } from './manuscriptComments.js'; import { escapeRegExp } from '../../lib/textUtils.js'; export const ERR_VALIDATION = 'PIPELINE_MANUSCRIPT_FIX_VALIDATION'; diff --git a/server/services/pipeline/manuscriptFixUndo.test.js b/server/services/pipeline/manuscriptFixUndo.test.js index 66190e0916..00f92766cc 100644 --- a/server/services/pipeline/manuscriptFixUndo.test.js +++ b/server/services/pipeline/manuscriptFixUndo.test.js @@ -43,7 +43,7 @@ vi.mock('./issues.js', () => ({ })), })); -vi.mock('./manuscriptReview.js', () => ({ +vi.mock('./manuscriptComments.js', () => ({ getComment: vi.fn(async (_seriesId, id) => mockComments.get(id) || null), updateComment: vi.fn(async (_seriesId, id, patch) => { const cur = mockComments.get(id); diff --git a/server/services/pipeline/manuscriptReview.js b/server/services/pipeline/manuscriptReview.js index 1eb6a651d8..2d5404b64a 100644 --- a/server/services/pipeline/manuscriptReview.js +++ b/server/services/pipeline/manuscriptReview.js @@ -1,223 +1,39 @@ /** - * Pipeline — Manuscript Review ("Finish the draft" comments) + * Pipeline — Manuscript Review seeding ("Finish the draft" findings) * - * Persists the editorial findings from the manuscript-completeness pass as a - * Word-style comment set the user works through in the manuscript editor: - * each comment can be jumped-to, given an AI fix, edited, and accepted into the - * manuscript (or dismissed). Findings are otherwise ephemeral — this is what - * makes "Finish the draft" actionable across reloads. + * Orchestration over the manuscriptComments.js comment store: merges fresh + * manuscript-completeness / editorial-check findings into the persisted + * Word-style comment set the user works through in the manuscript editor. * - * Stored as a sibling of the series record at - * `data/pipeline-series/{id}/manuscript-review.json`, so it travels with the - * series folder on share/sync without bloating the LWW-merged series - * `index.json` (the review is an independent, larger document with its own - * write cadence). Writes serialize on a per-series tail (single tail per shared - * file, per AGENTS.md). + * The store itself (schema, sanitizers, persistence, accessors) lives in + * ./manuscriptComments.js and is re-exported below for existing importers; the + * fix SHAPERS come from ./manuscriptFix.js, which reads/writes comments + * through the store module — so the former review <-> fix static import cycle + * is broken (#5918). */ -import { join } from 'path'; -import { randomUUID } from 'crypto'; -import { atomicWrite, readJSONFile } from '../../lib/fileUtils.js'; -import { createKeyedFileWriteQueue } from '../../lib/fileWriteQueue.js'; -import { seriesStore, listSeries } from './series.js'; -import { collectManuscriptSections, REPLACEMENT_STRATEGIES, replacementStrategyForCategory } from './arcPlanner.js'; +import { collectManuscriptSections } from './arcPlanner.js'; import { shapeAnchoredEdit, fixFromEdits } from './manuscriptFix.js'; +import { SCHEMA_VERSION, sanitizeComment, readReview, writeReview, queueReviewWrite } from './manuscriptComments.js'; import { emitRecordUpdated } from '../sharing/recordEvents.js'; -// Storage-layout version for the review document. Bump + migrate if the -// comment shape changes in a way older peers can't read. -const SCHEMA_VERSION = 1; - -export const COMMENT_STATUSES = Object.freeze(['open', 'accepted', 'dismissed']); -const STATUS_SET = new Set(COMMENT_STATUSES); - -// Why a dismissal was made (#1605). A plain dismiss (`dismissReason: null`) -// means "won't fix" — the finding is real but the user is leaving it. A -// `false-positive` reason means "this check is wrong here" — it feeds the -// per-check quality view so broken checks are tracked instead of silently -// re-surfacing the same bad finding every run. Only meaningful when -// `status === 'dismissed'`. Optional + additive, so the synced review doc stays -// backward-compatible with older peers (who simply ignore the field). -export const DISMISS_REASONS = Object.freeze(['false-positive']); -const DISMISS_REASON_SET = new Set(DISMISS_REASONS); +// Store accessors re-exported for existing importers (routes, seriesReview, +// editorialScore, readerPanel, sync receive paths, tests). +export { + COMMENT_STATUSES, + DISMISS_REASONS, + SCHEMA_VERSION, + getReview, + getComment, + updateComment, + mergeReviewFromSync, + locateComment, +} from './manuscriptComments.js'; // Re-run modes for seedReviewFromFindings — see its doc comment. Exported so the // route's Zod enum validates against the same source (mirrors COMMENT_STATUSES). export const REVIEW_RUN_MODES = Object.freeze(['merge', 'fresh']); -const REVIEW_FILE = 'manuscript-review.json'; -const reviewPath = (seriesId) => join(seriesStore().recordDir(seriesId), REVIEW_FILE); - -// Per-series write tail (the review file is distinct per series, so each only -// serializes against itself). One canonical single-tail queue per series id. -const queueReviewWrite = createKeyedFileWriteQueue(); - -const emptyReview = () => ({ schemaVersion: SCHEMA_VERSION, comments: [] }); - -const clampStr = (v, max) => (typeof v === 'string' ? v.trim().slice(0, max) : ''); - -function sanitizeFix(raw) { - if (!raw || typeof raw !== 'object') return null; - const find = typeof raw.find === 'string' ? raw.find : ''; - const replace = typeof raw.replace === 'string' ? raw.replace : ''; - const edits = Array.isArray(raw.edits) - ? raw.edits - .map((e) => { - if (!e || typeof e !== 'object') return null; - const editFind = typeof e.find === 'string' ? e.find : ''; - const editReplace = typeof e.replace === 'string' ? e.replace : ''; - if (!editFind && !editReplace) return null; - const out = { - issueNumber: Number.isInteger(e.issueNumber) ? e.issueNumber : null, - issueId: typeof e.issueId === 'string' ? e.issueId : null, - stageId: typeof e.stageId === 'string' ? e.stageId : null, - title: clampStr(e.title, 200), - find: editFind, - replace: editReplace, - note: clampStr(e.note, 1000), - }; - if (e.fuzzy === true) out.fuzzy = true; - return out; - }) - .filter(Boolean) - : []; - if (!find && !replace && edits.length === 0) return null; - const out = { find, replace }; - if (edits.length) out.edits = edits; - if (raw.fuzzy === true) out.fuzzy = true; - return out; -} - -// Snapshot of the manuscript text a fix overwrote, captured at accept-time so -// the finding can be undone back to its pre-edit state (#1609). `priorText` is -// the section's FULL stage text before the fix applied (full restore handles -// every edit shape — multi-edit, deletion, full-page rewrite — exactly, unlike -// a span-level reverse-splice); `appliedHash` is a fingerprint of the text the -// accept WROTE, so undo can detect later edits and refuse to clobber them. Only -// carried while `status === 'accepted'` (a status flip drops it, mirroring -// `dismissReason`). Optional + additive → older peers ignore it and the synced -// review doc stays backward-compatible (no schema bump needed). -function sanitizeAcceptedSnapshot(raw) { - if (!raw || typeof raw !== 'object' || !Array.isArray(raw.sections)) return null; - const sections = raw.sections - .map((s) => { - if (!s || typeof s !== 'object') return null; - const issueId = typeof s.issueId === 'string' && s.issueId ? s.issueId : null; - const stageId = typeof s.stageId === 'string' && s.stageId ? s.stageId : null; - const priorText = typeof s.priorText === 'string' ? s.priorText : null; - if (!issueId || !stageId || priorText == null) return null; - const out = { issueId, stageId, priorText }; - if (typeof s.appliedHash === 'string' && s.appliedHash) out.appliedHash = s.appliedHash; - return out; - }) - .filter(Boolean); - if (!sections.length) return null; - return { - acceptedAt: typeof raw.acceptedAt === 'string' ? raw.acceptedAt : new Date().toISOString(), - sections, - }; -} - -// Shape one stored comment. Tolerant of partial/legacy records so a hand-edited -// or older-peer file round-trips without dropping fields. -function sanitizeComment(raw) { - if (!raw || typeof raw !== 'object') return null; - const problem = clampStr(raw.problem, 2000); - if (!problem) return null; - const category = clampStr(raw.category, 40) || 'other'; - const severity = ['high', 'medium', 'low'].includes(raw.severity) ? raw.severity : 'medium'; - const status = STATUS_SET.has(raw.status) ? raw.status : 'open'; - return { - id: typeof raw.id === 'string' && raw.id ? raw.id : `mrc-${randomUUID()}`, - issueNumber: Number.isInteger(raw.issueNumber) ? raw.issueNumber : null, - issueId: typeof raw.issueId === 'string' ? raw.issueId : null, - stageId: typeof raw.stageId === 'string' ? raw.stageId : null, - severity, - // The check's NATIVE per-finding level (#1596), independent of any per-check - // severity override. Carried so that clearing an override re-grades the - // finding back to its true native level (not a guessed default). Defaults to - // `severity` for legacy/older-peer comments written before this field (all of - // which predate overrides, so native == severity is correct). Optional + - // additive → the synced review doc stays backward-compatible. - nativeSeverity: ['high', 'medium', 'low'].includes(raw.nativeSeverity) ? raw.nativeSeverity : severity, - category, - // Optional per-check sub-classification of the finding (#1626) — e.g. - // `dialogue.on-the-nose` tags each finding `exposition` / `emotion-tell` / - // `relationship-report` so the editor sees *why* a line reads on-the-nose. - // `null` for checks that don't sub-classify, legacy records, and older peers. - // Validated by the producing check against its own allow-list, so a stray - // value never reaches here; the clamp is a belt-and-suspenders bound for - // hand-edited / older-peer files. Optional + additive → the synced review doc - // stays backward-compatible (no schema bump needed). - subtype: clampStr(raw.subtype, 40) || null, - location: clampStr(raw.location, 200), - problem, - suggestion: clampStr(raw.suggestion, 8000), - // How `suggestion` should be read: 'full-page' = it's a complete replacement - // document (comic-structure panel rewrite); 'delta' = it's advice. Trust a - // valid stored value, else derive from category so legacy comments (written - // before this field existed) and older peers still classify correctly. - replacementStrategy: REPLACEMENT_STRATEGIES.has(raw.replacementStrategy) - ? raw.replacementStrategy - : replacementStrategyForCategory(category), - anchorQuote: clampStr(raw.anchorQuote, 400), - // Which editorial check produced this finding (#1284). `null` for findings - // from the manuscript-completeness pass (and older peers / legacy records) - // — those predate the registry, so they group as a single un-checked set. - // Optional + additive, so the synced review doc stays backward-compatible. - checkId: typeof raw.checkId === 'string' && raw.checkId ? raw.checkId : null, - // Fingerprint of the content the editorial check analyzed when it raised this - // finding (#1345) — the runner stamps it so the editor can flag the finding - // `stale` once the manuscript/canon drifts. `null` for completeness-pass - // findings, older peers, and legacy records (treated as never-stale). - // Optional + additive, so the synced review doc stays backward-compatible. - sourceContentHash: typeof raw.sourceContentHash === 'string' && raw.sourceContentHash ? raw.sourceContentHash : null, - status, - // Dismissal reason (#1605) — only carried while dismissed. A status flip - // back to open/accepted drops it here so a re-opened finding can't keep a - // stale `false-positive` mark. Legacy/older-peer records lack the field and - // sanitize to `null` (a plain "won't fix" dismiss). - dismissReason: raw.status === 'dismissed' && DISMISS_REASON_SET.has(raw.dismissReason) - ? raw.dismissReason - : null, - fix: sanitizeFix(raw.fix), - // Pre-edit snapshot for undo (#1609) — only meaningful once accepted, so a - // re-open/dismiss flip drops it (the undo path also clears it explicitly). - acceptedSnapshot: status === 'accepted' - ? sanitizeAcceptedSnapshot(raw.acceptedSnapshot) - : null, - sourceRunId: typeof raw.sourceRunId === 'string' ? raw.sourceRunId : null, - createdAt: typeof raw.createdAt === 'string' ? raw.createdAt : new Date().toISOString(), - updatedAt: typeof raw.updatedAt === 'string' ? raw.updatedAt : new Date().toISOString(), - }; -} - -function sanitizeReview(raw) { - if (!raw || typeof raw !== 'object' || !Array.isArray(raw.comments)) return emptyReview(); - return { - schemaVersion: SCHEMA_VERSION, - comments: raw.comments.map(sanitizeComment).filter(Boolean), - }; -} - -async function readReview(seriesId) { - // `null` = file absent (distinct from a present-but-empty review). - const raw = await readJSONFile(reviewPath(seriesId), null); - return raw == null ? emptyReview() : sanitizeReview(raw); -} - -async function writeReview(seriesId, review) { - await atomicWrite(reviewPath(seriesId), sanitizeReview(review)); -} - -/** - * Read the persisted review for a series. Returns an empty review (never null) - * when none has been generated yet. - */ -export async function getReview(seriesId) { - return readReview(seriesId); -} - // Stable identity for a finding so re-running completeness (or an editorial // check) doesn't duplicate a still-open comment the user hasn't acted on yet. // `checkId` is part of the key so the same anchor flagged by two different @@ -428,93 +244,3 @@ export async function seedReviewFromFindings(seriesId, findings, { runId = null, return next; }); } - -/** - * Patch a single comment (status flip, attach/clear a generated fix, edit the - * replacement text). Last-write-wins on `updatedAt`. Returns the updated - * comment, or throws if the id is unknown. - */ -export async function updateComment(seriesId, commentId, patch) { - return queueReviewWrite(seriesId, async () => { - const review = await readReview(seriesId); - const idx = review.comments.findIndex((c) => c.id === commentId); - if (idx === -1) { - throw Object.assign(new Error(`Comment not found: ${commentId}`), { code: 'PIPELINE_REVIEW_NOT_FOUND' }); - } - const cur = review.comments[idx]; - const merged = { ...cur }; - if (patch.status !== undefined && STATUS_SET.has(patch.status)) merged.status = patch.status; - // Dismissal reason (#1605). `null` is an explicit clear; a valid reason sets - // it. sanitizeComment below drops it anyway when the resulting status isn't - // `dismissed`, so re-opening a false-positive can't leave the mark behind. - if (patch.dismissReason !== undefined) { - merged.dismissReason = DISMISS_REASON_SET.has(patch.dismissReason) ? patch.dismissReason : null; - } - // `fix: null` is an explicit clear; absent leaves it untouched. - if (patch.fix !== undefined) merged.fix = sanitizeFix(patch.fix); - // Pre-edit undo snapshot (#1609). `null` is an explicit clear (the undo - // path); absent leaves it untouched. sanitizeComment below drops it anyway - // whenever the resulting status isn't `accepted`, so re-opening a finding - // can't leave a stale snapshot behind. - if (patch.acceptedSnapshot !== undefined) merged.acceptedSnapshot = patch.acceptedSnapshot; - merged.updatedAt = new Date().toISOString(); - const next = { ...review, comments: review.comments.map((c, i) => (i === idx ? sanitizeComment(merged) : c)) }; - await writeReview(seriesId, next); - // Sibling-doc change → fire a series `updated` event so the review - // propagates to peers / re-exports to subscribed buckets (see seed above). - emitRecordUpdated('series', seriesId); - return next.comments[idx]; - }); -} - -/** - * Sync-orchestrator entry: merge a remote peer's review into local state, - * last-write-wins per comment on `updatedAt`. Mirrors `mergeIssuesFromSync`. - */ -export async function mergeReviewFromSync(seriesId, remoteReview) { - const remote = sanitizeReview(remoteReview); - return queueReviewWrite(seriesId, async () => { - const local = await readReview(seriesId); - const byId = new Map(local.comments.map((c) => [c.id, c])); - for (const rc of remote.comments) { - const lc = byId.get(rc.id); - // Strict-newer (`>`) so an equal-clock echo is a skip, matching the - // `mergeIssuesFromSync` LWW guard. With `>=`, a peer re-sending a comment - // we already hold at the same timestamp re-adopts + re-writes it every - // sync cycle (write amplification + non-convergence) — the same bug - // catalogSync.js fixed this release. - if (!lc || new Date(rc.updatedAt).getTime() > new Date(lc.updatedAt).getTime()) { - byId.set(rc.id, rc); - } - } - const next = { schemaVersion: SCHEMA_VERSION, comments: [...byId.values()] }; - await writeReview(seriesId, next); - return next; - }); -} - -// Internal: used by manuscriptFix to read a single comment without a full -// round-trip ceremony. Returns null when absent. -export async function getComment(seriesId, commentId) { - const review = await readReview(seriesId); - return review.comments.find((c) => c.id === commentId) || null; -} - -/** - * Locate a finding/comment across ALL series by its (globally-unique) comment - * id. Findings live per-series in each series' manuscript-review.json, so a - * deep-link that carries only a commentId (e.g. one shared from elsewhere) has - * to resolve the owning series before the editor can open it (#1608). Returns - * `{ seriesId, comment }` for the first series whose review contains the id, or - * `null` when no series owns it. Comment ids are UUID-based (`randomUUID`) so a - * match is unambiguous; deleted series are skipped (listSeries default). - */ -export async function locateComment(commentId) { - if (typeof commentId !== 'string' || !commentId) return null; - const series = await listSeries(); - for (const s of series) { - const comment = await getComment(s.id, commentId); - if (comment) return { seriesId: s.id, comment }; - } - return null; -} diff --git a/server/services/serviceImportCycles.test.js b/server/services/serviceImportCycles.test.js index 301f36bd5d..359e446ef6 100644 --- a/server/services/serviceImportCycles.test.js +++ b/server/services/serviceImportCycles.test.js @@ -79,8 +79,6 @@ const KNOWN_CYCLIC_COMPONENTS = [ }, // #5917 — caption resolution and subject derivation import each other. { issue: 5917, members: ['loraDatasetCaption.js', 'loraDatasetGenerate.js'] }, - // #5918 — the fixer reaches back for the review-comment store accessors. - { issue: 5918, members: ['pipeline/manuscriptFix.js', 'pipeline/manuscriptReview.js'] }, // #5919 — the receive path reaches subscription state back through the barrel. { issue: 5919, members: ['sharing/peerSync.js', 'sharing/peerSyncReceive.js'] }, ];