diff --git a/CHANGELOG.md b/CHANGELOG.md index 085c7ee7..b540971f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,9 +12,11 @@ All notable user-visible changes to CASCADE are documented here. The format is l ### Fixed +- **Linear and JIRA inline checklist writes are now idempotent across provider/tool retries.** The shared markdown checklist engine now upserts by exact `### {Checklist Name}` heading and exact item text, merges duplicate inline sections into the first matching section, preserves non-checkbox prose from collapsed duplicate sections, preserves checked state when duplicate rows disagree, and keeps Trello on native checklist APIs. Linear also caches accepted description writes in-process so stale readback no longer overwrites or replays checklist appends into duplicate blocks. See Linear issue [MNG-741](https://linear.app/mongrel/issue/MNG-741/make-linear-checklist-mutations-idempotent-after-visibility-timeouts). + - **`cascade-tools scm reply-to-review-comment` now accepts `--body-file`, and `create-pr-review --comment` handles one inline comment object ergonomically.** `ReplyToReviewComment` now exposes the same generated body file-input contract as the other SCM comment commands. Array-of-object CLI params still prefer JSON arrays, but one top-level JSON object is normalized to a one-item array, while `null` and primitive JSON values fail early with the structured `json-parse` envelope instead of reaching the GitHub client. See Linear issues [MNG-736](https://linear.app/mongrel/issue/MNG-736/fix-cascade-tools-scm-review-command-cli-drift), [MNG-731](https://linear.app/mongrel/issue/MNG-731/friction-tooling-low-cascade-tools-reply-to-review-comment-rejects), and [MNG-729](https://linear.app/mongrel/issue/MNG-729/friction-tooling-low-create-pr-review-comment-accepted-an-object-but). -- **Linear inline checklist creation now waits for description visibility before releasing its mutation lock.** Linear can briefly return stale issue descriptions after accepting `updateIssue()`, so CASCADE now polls `getIssue()` under the existing per-issue description lock until the new markdown is visible. Parallel `AddChecklist` flows no longer fail with `Checklist not found in description` when one flow creates a checklist heading immediately before another appends items. See Linear issue [MNG-685](https://linear.app/issue/MNG-685). +- **Linear inline checklist creation now preserves freshly written descriptions across stale readback windows.** Linear can briefly return stale issue descriptions after accepting `updateIssue()`, so CASCADE records successful writes under the existing per-issue description lock and uses that in-process value as the next mutation's base while the provider catches up. Parallel `AddChecklist` flows no longer fail with `Checklist not found in description` when one flow creates a checklist heading immediately before another appends items. See Linear issue [MNG-685](https://linear.app/issue/MNG-685). - **Linear and JIRA inline checklist updates no longer lose sibling checklist rows during concurrent updates.** Both providers rewrite the whole issue description for checklist mutations, so their read/mutate/write path is now serialized per provider/work item with a stale-safe temp-file lock and provider-only retry semantics. The shared inline checklist parser also keeps scanning through prose, indented detail lines, and bullet detail lines until the next heading, so `ReadWorkItem` reports every visible checkbox row under a checklist heading. See Linear issue [MNG-656](https://linear.app/issue/MNG-656). diff --git a/src/integrations/README.md b/src/integrations/README.md index 4987afd3..43413fb1 100644 --- a/src/integrations/README.md +++ b/src/integrations/README.md @@ -295,9 +295,9 @@ Different PM providers have different native concepts of "checklist". The `PMPro **Why inline markdown for Linear and JIRA?** Both providers support markdown checkboxes natively in their description editors but lack a dedicated lightweight checklist primitive — sub-issues and subtasks are full work items, which clutters boards when used for things like acceptance criteria or implementation steps. Inline markdown matches Trello's lightweight semantics without creating orphan issues. See [spec 008](../../docs/specs/008-inline-checklists.md.done) for full rationale. -The shared engine that parses, appends, toggles, and removes inline checklist items lives at `src/pm/_shared/inline-checklist.ts` and is consumed by both the Linear and JIRA adapters. +The shared engine that parses, upserts, toggles, and removes inline checklist items lives at `src/pm/_shared/inline-checklist.ts` and is consumed by both the Linear and JIRA adapters. Inline checklist creation is idempotent by exact markdown heading and exact item text: repeated writes for the same `### {Checklist Name}` section merge into the first matching section, duplicate sections with the same heading are collapsed while preserving non-checkbox prose, duplicate rows converge to one row, and checked state wins if any duplicate row is checked. This is intentional for CASCADE-generated checklists and keeps provider/tool retries from duplicating `Implementation Steps` or `Acceptance Criteria` blocks. Trello remains on native checklists and does not use this markdown merge behavior. -Because Linear and JIRA checklist mutations rewrite the whole description, their adapters serialize the full read/mutate/write operation with `withDescriptionMutationLock(provider, workItemId, fn)` from `src/pm/_shared/description-mutation-lock.ts`. Keep future inline-description mutations inside that guard; otherwise concurrent `cascade-tools pm update-checklist-item` processes can overwrite each other's description snapshots without a provider-side conflict error. Initial checklist creation for inline-description providers should implement `createChecklistWithItems` so `AddChecklist` can create the section and all starting rows in one locked description mutation instead of `createChecklist` plus one write per item. Linear mutations must also keep the lock held until `linearClient.getIssue()` observes the markdown just written by `linearClient.updateIssue()`, because Linear can briefly serve stale descriptions after accepting a write. Releasing the lock before read-after-write convergence lets the next queued checklist mutation start from an old snapshot and miss a newly created `###` checklist heading. +Because Linear and JIRA checklist mutations rewrite the whole description, their adapters serialize the full read/mutate/write operation with `withDescriptionMutationLock(provider, workItemId, fn)` from `src/pm/_shared/description-mutation-lock.ts`. Keep future inline-description mutations inside that guard; otherwise concurrent `cascade-tools pm update-checklist-item` processes can overwrite each other's description snapshots without a provider-side conflict error. Initial checklist creation for inline-description providers should implement `createChecklistWithItems` so `AddChecklist` can create the section and all starting rows in one locked description mutation instead of `createChecklist` plus one write per item. Linear can briefly serve stale descriptions after accepting `linearClient.updateIssue()`, so Linear records each successful description write in an in-process recent-description cache and uses that cached value as the next locked mutation's base while the provider catches up. Do not replay an accepted append solely because readback is stale; rely on idempotent upsert helpers plus the recent-description cache to keep retries and consecutive writes from duplicating or losing inline checklist content. Because `withDescriptionMutationLock` is a **filesystem** lock shared by separate `cascade-tools` processes, Linear also writes a durable cross-process sidecar (via `writeLockedDescription` / `readLockedDescription` in `description-mutation-lock.ts`) while holding the lock after each successful PUT. The next process that acquires the same lock reads the sidecar as its fresh base, preventing it from overwriting the previous process's accepted write with a stale Linear snapshot. The default description-lock wait budget is intentionally lower than the checklist tool timeout: the lock waits up to 45s, while `AddChecklist`, `PMUpdateChecklistItem`, and `PMDeleteChecklistItem` allow 60s. Keep that relationship when adjusting either value so a queued, legitimate Linear/JIRA checklist mutation is not aborted by the outer tool runner before the lock wait can complete. diff --git a/src/pm/_shared/description-mutation-lock.ts b/src/pm/_shared/description-mutation-lock.ts index ae333ea1..8de5ad8b 100644 --- a/src/pm/_shared/description-mutation-lock.ts +++ b/src/pm/_shared/description-mutation-lock.ts @@ -1,6 +1,6 @@ import { randomUUID } from 'node:crypto'; import { constants } from 'node:fs'; -import { mkdir, open, readFile, stat, unlink } from 'node:fs/promises'; +import { mkdir, open, readFile, stat, unlink, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -22,6 +22,73 @@ const DEFAULT_STALE_MS = 120_000; const DEFAULT_POLL_MS = 25; const LOCK_DIR_ENV = 'CASCADE_DESCRIPTION_MUTATION_LOCK_DIR'; +/** + * TTL for the cross-process description sidecar files. Must comfortably + * exceed the provider's eventual-consistency window so the fresh base + * written by one process is still readable by the next process that + * acquires the lock. 60 s matches the in-process recent-description + * cache TTL in the Linear adapter. + */ +const SIDECAR_TTL_MS = 60_000; + +function getSidecarPath(lockDir: string, provider: string, workItemId: string): string { + return join(lockDir, `${sanitizePathPart(provider)}-${sanitizePathPart(workItemId)}.desc.json`); +} + +/** + * Read the description written by the most-recent locked mutation for + * `(provider, workItemId)`. Returns `undefined` when no recent sidecar + * exists or it has expired. + * + * **Must be called while holding the description mutation lock** for the + * same (provider, workItemId) pair so the read-and-use is atomic across + * concurrent processes. + */ +export async function readLockedDescription( + provider: string, + workItemId: string, + options: DescriptionMutationLockOptions = {}, +): Promise { + const settings = resolveOptions(options); + const sidecarPath = getSidecarPath(settings.lockDir, provider, workItemId); + try { + const raw = await readFile(sidecarPath, 'utf8'); + const data = JSON.parse(raw) as { description: string; timestamp: number }; + if (typeof data.description !== 'string' || typeof data.timestamp !== 'number') { + return undefined; + } + if (Date.now() - data.timestamp > SIDECAR_TTL_MS) { + await unlink(sidecarPath).catch(() => {}); + return undefined; + } + return data.description; + } catch { + return undefined; + } +} + +/** + * Persist the description that was just written so that a subsequent locked + * mutation in a **different process** can use it as a fresh base rather than + * reading a stale snapshot from the PM provider. + * + * **Must be called while holding the description mutation lock** for the + * same (provider, workItemId) pair, immediately after a successful PUT. + * Best-effort: failures are non-fatal because the in-process cache still + * handles same-process retries. + */ +export async function writeLockedDescription( + provider: string, + workItemId: string, + description: string, + options: DescriptionMutationLockOptions = {}, +): Promise { + const settings = resolveOptions(options); + await mkdir(settings.lockDir, { recursive: true }); + const sidecarPath = getSidecarPath(settings.lockDir, provider, workItemId); + await writeFile(sidecarPath, JSON.stringify({ description, timestamp: Date.now() }), 'utf8'); +} + export async function withDescriptionMutationLock( provider: string, workItemId: string, diff --git a/src/pm/_shared/inline-checklist.ts b/src/pm/_shared/inline-checklist.ts index f8063a71..5e63c941 100644 --- a/src/pm/_shared/inline-checklist.ts +++ b/src/pm/_shared/inline-checklist.ts @@ -43,6 +43,34 @@ export function parseInlineChecklists(description: string): ParsedChecklist[] { return state.checklists; } +export function checklistSectionContainsItems( + description: string, + checklistName: string, + items: { name: string; checked?: boolean }[], +): boolean { + const deduped = dedupeChecklistSections(description, checklistName); + const section = findChecklistSection(deduped.split('\n'), checklistName); + if (!section) return false; + const sectionItems = collectSectionItems(deduped.split('\n'), section); + return items.every((item) => { + const actual = sectionItems.get(item.name); + if (actual === undefined) return false; + return item.checked === undefined || actual === item.checked || actual === true; + }); +} + +export function checklistItemStateSatisfied( + description: string, + checklistName: string, + itemName: string, + checked: boolean, +): boolean { + const deduped = dedupeChecklistSections(description, checklistName); + const section = findChecklistSection(deduped.split('\n'), checklistName); + if (!section) return false; + return collectSectionItems(deduped.split('\n'), section).get(itemName) === checked; +} + interface ParseState { checklists: ParsedChecklist[]; current: ParsedChecklist | null; @@ -176,6 +204,26 @@ export function appendChecklistSection( return `${description.trimEnd()}\n\n${section}`; } +export function upsertChecklistSection( + description: string, + checklistName: string, + items: { name: string; checked: boolean }[], +): string { + const deduped = dedupeChecklistSections(description, checklistName); + const lines = deduped ? deduped.split('\n') : []; + const section = findChecklistSection(lines, checklistName); + + if (!section) { + return appendChecklistSection(deduped, checklistName, items); + } + + if (items.length === 0) return deduped; + return dedupeChecklistSections( + appendChecklistSection(deduped, checklistName, items), + checklistName, + ); +} + // --------------------------------------------------------------------------- // Adding a single item // --------------------------------------------------------------------------- @@ -187,29 +235,7 @@ export function addItemToChecklist( checked = false, ): string { const lines = description.split('\n'); - const heading = `### ${checklistName}`; - let insertIdx = -1; - let inSection = false; - - for (let i = 0; i < lines.length; i++) { - if (lines[i] === heading) { - inSection = true; - insertIdx = i; - continue; - } - if (inSection) { - if (CHECKBOX_REGEX.test(lines[i])) { - insertIdx = i; - } else if (HEADING_REGEX.test(lines[i])) { - break; - } else if (lines[i].trim() !== '') { - // Non-empty detail/prose line — advance insertIdx so new items land - // after all trailing detail belonging to the previous item, not before it. - insertIdx = i; - } - } - } - + const insertIdx = findChecklistInsertionIndex(lines, checklistName); if (insertIdx === -1) { throw new Error(`Checklist section "${checklistName}" not found in description`); } @@ -219,6 +245,31 @@ export function addItemToChecklist( return lines.join('\n'); } +export function upsertItemInChecklist( + description: string, + checklistName: string, + itemName: string, + checked = false, +): string { + const deduped = dedupeChecklistSections(description, checklistName); + const lines = deduped.split('\n'); + const section = findChecklistSection(lines, checklistName); + if (!section) { + throw new Error(`Checklist section "${checklistName}" not found in description`); + } + + const existing = findItemLineInSection(lines, section, itemName); + if (existing !== -1) { + const existingChecked = lines[existing].match(CHECKBOX_REGEX)?.[1] === 'x'; + if (checked && !existingChecked) { + lines[existing] = `- [x] ${itemName}`; + } + return lines.join('\n'); + } + + return addItemToChecklist(deduped, checklistName, itemName, checked); +} + // --------------------------------------------------------------------------- // Toggling an item // --------------------------------------------------------------------------- @@ -315,6 +366,430 @@ interface SectionScan { lastContentIdx: number; } +interface ChecklistSectionSpan { + name: string; + startIdx: number; + endIdx: number; +} + +/** + * An item collected during duplicate-section merging, carrying both its + * checked state and any trailing detail/prose lines that belong to it + * (non-blank, non-checkbox, non-heading lines immediately following the + * checkbox with no blank-line gap). + */ +interface MergedItemInfo { + checked: boolean; + /** Detail/prose lines immediately following this item's checkbox row. */ + detail: string[]; +} + +function dedupeChecklistSections(description: string, checklistName: string): string { + if (!description) return description; + const lines = description.split('\n'); + const sections = scanChecklistSections(lines).filter((section) => section.name === checklistName); + if (sections.length <= 1) return description; + + const first = sections[0]; + // Track items already in the first section so we know which items from + // duplicate sections are "new" (need to be inserted with their detail) + // vs "existing" (detail would be orphaned and should be preserved as prose). + const firstSectionItems = new Set(collectSectionItems(lines, first).keys()); + const mergedItems = collectMergedSectionItemsWithDetail(lines, sections); + const { lines: rewrittenLines, detailEmittedForItems } = rewriteChecklistSection( + lines.slice(first.startIdx, first.endIdx), + mergedItems, + ); + return removeDuplicateChecklistSections( + lines, + sections, + rewrittenLines, + firstSectionItems, + detailEmittedForItems, + ); +} + +function findChecklistInsertionIndex(lines: string[], checklistName: string): number { + const heading = `### ${checklistName}`; + let insertIdx = -1; + let inSection = false; + + for (let i = 0; i < lines.length; i++) { + if (lines[i] === heading) { + inSection = true; + insertIdx = i; + continue; + } + if (!inSection) continue; + if (CHECKBOX_REGEX.test(lines[i])) { + insertIdx = i; + } else if (HEADING_REGEX.test(lines[i])) { + break; + } else if (lines[i].trim() !== '') { + // Non-empty detail/prose line — advance insertIdx so new items land + // after all trailing detail belonging to the previous item, not before it. + insertIdx = i; + } + } + + return insertIdx; +} + +/** + * Collect all items from a single section, each with trailing detail lines. + * + * A "detail line" is any non-blank, non-checkbox, non-heading line that + * immediately follows a checkbox with no blank-line gap between them. These + * are treated as belonging to their preceding item and must travel with it + * when the section is merged elsewhere. + */ +function collectSectionItemsWithDetail( + lines: string[], + section: ChecklistSectionSpan, +): Map { + const items = new Map(); + let currentName: string | null = null; + for (let i = section.startIdx + 1; i < section.endIdx; i++) { + const line = lines[i]; + const cbMatch = line.match(CHECKBOX_REGEX); + if (cbMatch) { + currentName = cbMatch[2].trim(); + const checked = cbMatch[1] === 'x'; + const existing = items.get(currentName); + if (!existing) { + items.set(currentName, { checked, detail: [] }); + } else if (checked) { + existing.checked = true; + } + } else if (HEADING_REGEX.test(line)) { + break; + } else if (currentName !== null) { + if (line.trim() === '') { + currentName = null; // blank line ends detail attachment + } else { + items.get(currentName)?.detail.push(line); + } + } + } + return items; +} + +function collectMergedSectionItemsWithDetail( + lines: string[], + sections: ChecklistSectionSpan[], +): Map { + const merged = new Map(); + for (const section of sections) { + for (const [name, info] of collectSectionItemsWithDetail(lines, section)) { + const existing = merged.get(name); + if (!existing) { + // First occurrence — keep its checked state and detail. + merged.set(name, { checked: info.checked, detail: info.detail }); + } else { + // Subsequent occurrence — merge checked state (any checked wins). + if (info.checked) existing.checked = true; + // Detail: first non-empty occurrence wins to avoid duplicating detail. + if (existing.detail.length === 0 && info.detail.length > 0) { + existing.detail.push(...info.detail); + } + } + } + } + return merged; +} + +function rewriteChecklistSection( + sectionLines: string[], + mergedItems: Map, +): { lines: string[]; detailEmittedForItems: Set } { + // Pre-scan section1 to know which items already carry their own detail inline + // (a non-blank, non-heading line immediately following the checkbox with no + // blank gap). Items in this set keep their detail from sectionLines; items + // NOT in this set receive their detail (if any) from mergedItems so that + // detail contributed only by a duplicate section stays attached to the item. + const section1ItemsWithDetail = collectItemsWithDetailFromSection(sectionLines); + const detailEmittedForItems = new Set(); + const state = { lines: [] as string[], seen: new Set(), lastCheckboxIdx: -1 }; + for (const line of sectionLines) { + rewriteChecklistSectionLine( + state, + line, + mergedItems, + section1ItemsWithDetail, + detailEmittedForItems, + ); + } + insertMissingChecklistItemLines(state, mergedItems); + return { lines: state.lines, detailEmittedForItems }; +} + +/** + * Return the set of item names in `sectionLines` that have at least one + * non-blank detail/prose line immediately following their checkbox (no + * blank-line gap). Used to distinguish items whose detail already lives in + * section 1 (leave it in-place) from items whose detail must be pulled from + * `mergedItems` (emit inline to keep it attached). + */ +function collectItemsWithDetailFromSection(sectionLines: string[]): Set { + const withDetail = new Set(); + let currentItem: string | null = null; + for (let i = 1; i < sectionLines.length; i++) { + const line = sectionLines[i]; + const cb = line.match(CHECKBOX_REGEX); + if (cb) { + currentItem = cb[2].trim(); + } else if (HEADING_REGEX.test(line)) { + break; + } else if (currentItem !== null) { + if (line.trim() === '') { + currentItem = null; + } else { + withDetail.add(currentItem); + } + } + } + return withDetail; +} + +function rewriteChecklistSectionLine( + state: { lines: string[]; seen: Set; lastCheckboxIdx: number }, + line: string, + mergedItems: Map, + section1ItemsWithDetail: Set, + detailEmittedForItems: Set, +): void { + const cbMatch = line.match(CHECKBOX_REGEX); + if (!cbMatch) { + state.lines.push(line); + return; + } + const itemName = cbMatch[2].trim(); + if (state.seen.has(itemName)) return; + state.seen.add(itemName); + const info = mergedItems.get(itemName); + const checked = (info?.checked ?? false) || cbMatch[1] === 'x'; + state.lines.push(`- [${checked ? 'x' : ' '}] ${itemName}`); + state.lastCheckboxIdx = state.lines.length - 1; + // If the item has no detail in section1 but mergedItems carries detail + // (contributed by a duplicate section), emit it now so the detail stays + // attached to this item rather than surfacing later as orphaned prose. + if (!section1ItemsWithDetail.has(itemName) && info?.detail && info.detail.length > 0) { + for (const dl of info.detail) { + state.lines.push(dl); + } + detailEmittedForItems.add(itemName); + } + // Otherwise: detail comes from the source sectionLines on subsequent + // iterations (preserved in-place). +} + +/** + * Find where to splice new checkbox rows after the last existing checkbox. + * + * We scan forward past any non-blank detail/prose lines that immediately follow + * `lastCheckboxIdx` and stop at the first blank line (section boundary). This + * ensures newly-merged rows land *after* detail belonging to the previous item, + * not before it — which would visually re-attribute those detail lines to the + * wrong (newly-inserted) item. + */ +function findMissingItemsInsertionIndex(lines: string[], lastCheckboxIdx: number): number { + if (lastCheckboxIdx === -1) { + return 1; // No checkboxes yet; insert right after the heading. + } + let insertIdx = lastCheckboxIdx; + for (let i = lastCheckboxIdx + 1; i < lines.length; i++) { + if (lines[i].trim() !== '') { + insertIdx = i; // Non-blank detail/prose line — advance past it. + } else { + break; // Blank line — stop; don't cross section boundaries. + } + } + return insertIdx + 1; +} + +function insertMissingChecklistItemLines( + state: { lines: string[]; seen: Set; lastCheckboxIdx: number }, + mergedItems: Map, +): void { + const missingItemLines: string[] = []; + for (const [itemName, info] of mergedItems) { + if (!state.seen.has(itemName)) { + missingItemLines.push(`- [${info.checked ? 'x' : ' '}] ${itemName}`); + // Carry the item's detail lines immediately after its checkbox so they + // remain attached to the item rather than becoming orphaned prose. + missingItemLines.push(...info.detail); + } + } + if (missingItemLines.length > 0) { + const insertIdx = findMissingItemsInsertionIndex(state.lines, state.lastCheckboxIdx); + state.lines.splice(insertIdx, 0, ...missingItemLines); + } +} + +function removeDuplicateChecklistSections( + lines: string[], + sections: ChecklistSectionSpan[], + rewrittenFirstSection: string[], + firstSectionItems: Set, + detailEmittedForItems: Set, +): string { + const first = sections[0]; + const output: string[] = []; + for (let i = 0; i < lines.length; ) { + if (i === first.startIdx) { + output.push(...rewrittenFirstSection); + i = first.endIdx; + continue; + } + const duplicate = findSectionStartingAt(sections, i); + if (duplicate) { + i = skipRemovedDuplicateSection( + lines, + output, + duplicate, + firstSectionItems, + detailEmittedForItems, + ); + continue; + } + output.push(lines[i]); + i++; + } + + return output.join('\n').trimEnd(); +} + +function findSectionStartingAt( + sections: ChecklistSectionSpan[], + lineIdx: number, +): ChecklistSectionSpan | undefined { + return sections.find((section) => section.startIdx === lineIdx); +} + +/** + * Skip a duplicate checklist section during convergence, selectively + * preserving content that can't be safely re-attached to merged items. + * + * Checkbox rows are dropped (they've been merged into the first section + * rewrite). The heading itself is also dropped. What we keep: + * + * - Detail lines of EXISTING items whose detail was NOT already emitted + * inline in the first-section rewrite: since those items are not being + * re-inserted, their detail cannot travel with them and is preserved as + * prose. Items whose detail WAS emitted inline (tracked in + * `detailEmittedForItems`) are skipped to avoid duplication. + * + * - Truly standalone prose: non-checkbox lines that appear before the + * first checkbox or after a blank-line separator (not attached to any + * checkbox item). + * + * Detail lines of NEW items (items NOT in firstSectionItems) are skipped — + * they are emitted alongside their checkbox row by insertMissingChecklistItemLines. + */ +function skipRemovedDuplicateSection( + lines: string[], + output: string[], + duplicate: ChecklistSectionSpan, + firstSectionItems: Set, + detailEmittedForItems: Set, +): number { + const proseLines: string[] = []; + let currentItemName: string | null = null; + let currentItemIsNew = false; + + for (let i = duplicate.startIdx + 1; i < duplicate.endIdx; i++) { + const line = lines[i]; + const cbMatch = line.match(CHECKBOX_REGEX); + if (cbMatch) { + currentItemName = cbMatch[2].trim(); + // "New" items (not in the first section) are inserted into section 1 + // by insertMissingChecklistItemLines; their detail travels with them. + currentItemIsNew = !firstSectionItems.has(currentItemName); + } else if (HEADING_REGEX.test(line)) { + break; + } else if (line.trim() === '') { + // Blank line — end of any item's detail attachment. + currentItemName = null; + currentItemIsNew = false; + } else if (currentItemName === null) { + // Standalone prose before the first checkbox (or after a blank gap). + proseLines.push(line); + } else if (!currentItemIsNew && !detailEmittedForItems.has(currentItemName)) { + // Detail line of an EXISTING item — preserve as prose ONLY when the + // detail was NOT already emitted inline in the rewritten first section. + // (Items in detailEmittedForItems had their detail pulled from + // mergedItems and emitted right after their checkbox — re-emitting + // here would duplicate or orphan that detail.) + proseLines.push(line); + } + // else: detail of a NEW item (emitted by insertMissingChecklistItemLines), + // or detail of an existing item already emitted inline in section 1. + } + + // Trim leading/trailing blank lines so we don't emit orphaned whitespace. + while (proseLines.length > 0 && proseLines[0].trim() === '') proseLines.shift(); + while (proseLines.length > 0 && proseLines[proseLines.length - 1].trim() === '') proseLines.pop(); + + while (output.length > 0 && output[output.length - 1].trim() === '') output.pop(); + + if (proseLines.length > 0) { + // Preserve prose from the duplicate section after the merged content. + output.push(''); + output.push(...proseLines); + } + + let nextIdx = duplicate.endIdx; + while (nextIdx < lines.length && lines[nextIdx].trim() === '') nextIdx++; + if (nextIdx < lines.length && output.length > 0 && output[output.length - 1].trim() !== '') { + output.push(''); + } + return nextIdx; +} + +function scanChecklistSections(lines: string[]): ChecklistSectionSpan[] { + const sections: ChecklistSectionSpan[] = []; + for (let i = 0; i < lines.length; i++) { + const match = lines[i].match(H3_REGEX); + if (!match) continue; + let endIdx = lines.length; + for (let j = i + 1; j < lines.length; j++) { + if (HEADING_REGEX.test(lines[j])) { + endIdx = j; + break; + } + } + sections.push({ name: match[1], startIdx: i, endIdx }); + } + return sections; +} + +function findChecklistSection(lines: string[], checklistName: string): ChecklistSectionSpan | null { + return scanChecklistSections(lines).find((section) => section.name === checklistName) ?? null; +} + +function collectSectionItems(lines: string[], section: ChecklistSectionSpan): Map { + const items = new Map(); + for (let i = section.startIdx + 1; i < section.endIdx; i++) { + const match = lines[i].match(CHECKBOX_REGEX); + if (!match) continue; + const name = match[2].trim(); + items.set(name, (items.get(name) ?? false) || match[1] === 'x'); + } + return items; +} + +function findItemLineInSection( + lines: string[], + section: ChecklistSectionSpan, + itemName: string, +): number { + for (let i = section.startIdx + 1; i < section.endIdx; i++) { + const match = lines[i].match(CHECKBOX_REGEX); + if (match && match[2].trim() === itemName) return i; + } + return -1; +} + function scanSection(lines: string[], checklistName: string, targetItemName: string): SectionScan { const heading = `### ${checklistName}`; let headingIdx = -1; diff --git a/src/pm/jira/adapter.ts b/src/pm/jira/adapter.ts index 137be7aa..f5d1f9e6 100644 --- a/src/pm/jira/adapter.ts +++ b/src/pm/jira/adapter.ts @@ -8,8 +8,6 @@ import { jiraClient } from '../../jira/client.js'; import { logger } from '../../utils/logging.js'; import { withDescriptionMutationLock } from '../_shared/description-mutation-lock.js'; import { - addItemToChecklist, - appendChecklistSection, buildChecklistId, findChecklistNameByHash, hashChecklistItemId, @@ -17,6 +15,8 @@ import { parseInlineChecklists, removeChecklistItem, toggleChecklistItem, + upsertChecklistSection, + upsertItemInChecklist, } from '../_shared/inline-checklist.js'; import type { ContainerId, LabelId } from '../ids.js'; import { parseContainerId } from '../ids.js'; @@ -282,7 +282,7 @@ export class JiraPMProvider implements PMProvider { } async createChecklist(workItemId: string, name: string): Promise { - await this.updateDescription(workItemId, (desc) => appendChecklistSection(desc, name, [])); + await this.updateDescription(workItemId, (desc) => upsertChecklistSection(desc, name, [])); return { id: buildChecklistId(workItemId, name), name, @@ -297,7 +297,7 @@ export class JiraPMProvider implements PMProvider { items: ChecklistItemDraft[], ): Promise { await this.updateDescription(workItemId, (desc) => - appendChecklistSection( + upsertChecklistSection( desc, name, items.map((item) => ({ name: item.name, checked: item.checked ?? false })), @@ -332,7 +332,7 @@ export class JiraPMProvider implements PMProvider { if (!checklistName) { throw new Error(`Checklist not found in description: ${checklistId}`); } - return addItemToChecklist(desc, checklistName, name, checked); + return upsertItemInChecklist(desc, checklistName, name, checked); }); } diff --git a/src/pm/linear/adapter.ts b/src/pm/linear/adapter.ts index 6356d5c8..2eccc8b8 100644 --- a/src/pm/linear/adapter.ts +++ b/src/pm/linear/adapter.ts @@ -11,10 +11,12 @@ import { resolveLabelId as sharedResolveLabelId } from '../../integrations/pm/_shared/label-id-resolver.js'; import { linearClient } from '../../linear/client.js'; import { logger } from '../../utils/logging.js'; -import { withDescriptionMutationLock } from '../_shared/description-mutation-lock.js'; import { - addItemToChecklist, - appendChecklistSection, + readLockedDescription, + withDescriptionMutationLock, + writeLockedDescription, +} from '../_shared/description-mutation-lock.js'; +import { buildChecklistId, findChecklistNameByHash, hashChecklistItemId, @@ -22,6 +24,8 @@ import { parseInlineChecklists, removeChecklistItem, toggleChecklistItem, + upsertChecklistSection, + upsertItemInChecklist, } from '../_shared/inline-checklist.js'; import type { LinearConfig } from '../config.js'; import type { ContainerId, LabelId } from '../ids.js'; @@ -41,23 +45,20 @@ import type { /** * In-process read-after-write cache of recently-PUT issue descriptions. * - * Linear's API is eventually consistent — a GET issued moments after a PUT - * can return the previous description for seconds. Polling the GET until - * visibility (the prior approach in commit fad4dda1) was too aggressive - * (1s timeout) and DOSed itself: planning runs MNG-741 / MNG-736 / MNG-739 - * (2026-05-12) all failed with "Linear description visibility timed out" - * even though every PUT succeeded on Linear's side. + * Linear's API is eventually consistent: a GET issued moments after a PUT can + * return the previous description for seconds. After each successful PUT we + * store the new description in two places: * - * The new contract: after each successful PUT, store the new description - * here. The next `updateDescription` call consults the cache before - * mutating — if the GET returned a stale value within the consistency - * window, the cached value wins. After TTL the entry is evicted and the - * GET becomes authoritative again. + * 1. This in-process map — fast, zero I/O, covers same-process retries and + * consecutive mutations within the same `cascade-tools` invocation. * - * Scope: in-process only. Cross-process races against Linear's eventual - * consistency are NOT new to this fix and were never solved by the - * visibility wait — the existing `withDescriptionMutationLock` is - * process-local too. + * 2. A filesystem sidecar file (via `writeLockedDescription`) — durable + * across process boundaries. Because `withDescriptionMutationLock` is a + * filesystem lock shared by all `cascade-tools` processes on the same + * host, a second process can acquire the lock after this one releases it + * with an empty in-process cache. The sidecar, written while holding the + * lock, provides that process with the fresh base so it doesn't overwrite + * the first process's accepted write with a stale Linear snapshot. */ const RECENT_DESCRIPTION_TTL_MS = 60_000; type RecentDescription = { description: string; timestamp: number }; @@ -65,7 +66,6 @@ const recentDescriptions = new Map(); function rememberRecentDescription(issueId: string, description: string): void { recentDescriptions.set(issueId, { description, timestamp: Date.now() }); - // Lazy cleanup — keep the map small without a setInterval. if (recentDescriptions.size > 200) { const cutoff = Date.now() - RECENT_DESCRIPTION_TTL_MS; for (const [id, entry] of recentDescriptions.entries()) { @@ -84,14 +84,10 @@ function recallRecentDescription(issueId: string): string | undefined { return entry.description; } -/** - * Test-only escape hatch — each test starts with an empty cache so module- - * level state doesn't leak between cases. NOT exported via the public API; - * called only from `tests/unit/pm/linear/adapter.test.ts`. - */ export function __resetRecentDescriptionsForTests(): void { recentDescriptions.clear(); } + const CASCADE_STATUS_KEYS = new Set([ 'backlog', 'todo', @@ -288,7 +284,7 @@ export class LinearPMProvider implements PMProvider { } async createChecklist(workItemId: string, name: string): Promise { - await this.updateDescription(workItemId, (desc) => appendChecklistSection(desc, name, [])); + await this.updateDescription(workItemId, (desc) => upsertChecklistSection(desc, name, [])); return { id: buildChecklistId(workItemId, name), name, @@ -302,12 +298,12 @@ export class LinearPMProvider implements PMProvider { name: string, items: ChecklistItemDraft[], ): Promise { + const checklistItems = items.map((item) => ({ + name: item.name, + checked: item.checked ?? false, + })); await this.updateDescription(workItemId, (desc) => - appendChecklistSection( - desc, - name, - items.map((item) => ({ name: item.name, checked: item.checked ?? false })), - ), + upsertChecklistSection(desc, name, checklistItems), ); return { @@ -338,7 +334,7 @@ export class LinearPMProvider implements PMProvider { if (!checklistName) { throw new Error(`Checklist not found in description: ${checklistId}`); } - return addItemToChecklist(desc, checklistName, name, checked); + return upsertItemInChecklist(desc, checklistName, name, checked); }); logger.debug('[Linear] addChecklistItem — appended inline checkbox', { workItemId: parsed.workItemId, @@ -381,6 +377,12 @@ export class LinearPMProvider implements PMProvider { issueId: string, mutate: (desc: string) => string, ): Promise { + // Read the cross-process sidecar first — if a different process wrote a + // description under this lock before us, its sidecar is more authoritative + // than a potentially-stale Linear GET. Called here (inside the lock body) + // so the read is serialized with any concurrent writes. + const sidecarDescription = await readLockedDescription('linear', issueId); + for (let attempt = 0; attempt < 2; attempt++) { let issue: Awaited>; try { @@ -393,18 +395,42 @@ export class LinearPMProvider implements PMProvider { throw err; } - // Read-after-write consistency: if we recently PUT a newer description - // for this issue and the GET above returned the stale pre-PUT value - // (Linear's eventual-consistency window), use the cached fresh value - // as the source of truth for the mutation. Without this, consecutive - // in-process updates can read-modify-write over each other. - const cachedDescription = recallRecentDescription(issueId); - const baseDescription = - cachedDescription !== undefined ? cachedDescription : (issue.description ?? ''); + // Choose the base description carefully to handle two competing risks: + // 1. Linear's eventual consistency: a GET issued shortly after a PUT + // can return the pre-write description. Our sidecar / in-process + // cache records the last accepted write so we don't overwrite it. + // 2. External human edits: if a user has edited the description in + // Linear *after* our last write, the provider read is the freshest + // state and must win — otherwise we'd silently drop their changes. + // + // Heuristic: if the provider read *contains* our last known-good write + // (the sidecar / in-process cache), it is at least as current — prefer + // it so external edits that arrived after our write are preserved. + // If the provider read does NOT contain our last write, it is + // demonstrably stale (Linear hasn't propagated our PUT yet) — fall back + // to the sidecar / in-process cache. + const inProcessDescription = recallRecentDescription(issueId); + const bestKnown = sidecarDescription ?? inProcessDescription; + let baseDescription: string; + if (bestKnown !== undefined) { + const providerDesc = issue.description ?? ''; + if (providerDesc.trimEnd().includes(bestKnown.trimEnd())) { + // Provider read is at least as current as our last write — use it + // to preserve any external edits that arrived after that write. + baseDescription = providerDesc; + } else { + // Provider read is stale — use our last known-good write as the base. + baseDescription = bestKnown; + } + } else { + baseDescription = issue.description ?? ''; + } const newDesc = mutate(baseDescription); try { await linearClient.updateIssue(issueId, { description: newDesc }); rememberRecentDescription(issueId, newDesc); + // Persist for the next process that acquires this lock. + await writeLockedDescription('linear', issueId, newDesc); return; } catch (err) { if (attempt === 0) { diff --git a/tests/unit/pm/_shared/inline-checklist.test.ts b/tests/unit/pm/_shared/inline-checklist.test.ts index aa59276c..e89b7653 100644 --- a/tests/unit/pm/_shared/inline-checklist.test.ts +++ b/tests/unit/pm/_shared/inline-checklist.test.ts @@ -2,10 +2,14 @@ import { describe, expect, it } from 'vitest'; import { addItemToChecklist, appendChecklistSection, + checklistItemStateSatisfied, + checklistSectionContainsItems, hashChecklistItemId, parseInlineChecklists, removeChecklistItem, toggleChecklistItem, + upsertChecklistSection, + upsertItemInChecklist, } from '../../../../src/pm/_shared/inline-checklist.js'; // --------------------------------------------------------------------------- @@ -213,6 +217,203 @@ describe('addItemToChecklist', () => { }); }); +// --------------------------------------------------------------------------- +// upsertChecklistSection / upsertItemInChecklist +// --------------------------------------------------------------------------- + +describe('upsertChecklistSection', () => { + it('appends when the section is missing', () => { + const result = upsertChecklistSection('Existing text.', '✅ AC', [ + { name: 'First', checked: false }, + ]); + expect(result).toBe('Existing text.\n\n### ✅ AC\n- [ ] First'); + }); + + it('does not duplicate an existing section or item on retry', () => { + let desc = upsertChecklistSection('', '✅ AC', [{ name: 'First', checked: false }]); + desc = upsertChecklistSection(desc, '✅ AC', [{ name: 'First', checked: false }]); + + expect(desc).toBe('### ✅ AC\n- [ ] First'); + }); + + it('merges requested rows into an empty existing section', () => { + const result = upsertChecklistSection('### ✅ AC', '✅ AC', [ + { name: 'First', checked: false }, + ]); + expect(result).toBe('### ✅ AC\n- [ ] First'); + }); + + it('deduplicates duplicate sections while preserving first section position and unrelated prose', () => { + const desc = `Intro. + +### ✅ AC +- [ ] First + +Middle prose. + +### ✅ AC +- [x] First +- [ ] Second + +## Next +Keep me.`; + + const result = upsertChecklistSection(desc, '✅ AC', [{ name: 'Third', checked: false }]); + + expect(result).toBe(`Intro. + +### ✅ AC +- [x] First +- [ ] Second +- [ ] Third + +Middle prose. + +## Next +Keep me.`); + }); + + it('does not merge different headings', () => { + const desc = '### ✅ AC\n- [ ] First\n\n### Dependencies\n- [ ] First'; + const result = upsertChecklistSection(desc, '✅ AC', [{ name: 'Second', checked: false }]); + + expect(result).toContain('### ✅ AC\n- [ ] First\n- [ ] Second'); + expect(result).toContain('### Dependencies\n- [ ] First'); + }); + + it('preserves non-checkbox prose from duplicate sections instead of silently dropping it', () => { + // Regression: MNG-741 — user-edited prose inside a duplicate checklist section + // must survive the convergence pass (data-loss path reported in review #3226378053). + const desc = '### AC\n- [ ] First\n\n### AC\n- [x] First\nThis note should stay.\nMore detail.'; + + const result = upsertChecklistSection(desc, 'AC', []); + + expect(result).toContain('This note should stay.'); + expect(result).toContain('More detail.'); + // The duplicate heading must be collapsed into one. + expect(result.match(/^### AC$/gm)).toHaveLength(1); + // Checkbox items are merged correctly. + expect(result).toContain('- [x] First'); + }); + + it('inserts merged rows after trailing detail lines so they stay attached to their parent item', () => { + // Regression: MNG-741 — rows merged from a duplicate section were inserted at + // lastCheckboxIdx + 1, which placed them BEFORE any trailing detail/prose + // belonging to the last existing checkbox. The detail was then visually + // re-attributed to the newly inserted item (review comment #3226513749). + // ### AC\n- [ ] First\n Detail for First\n\n### AC\n- [ ] Second + // must NOT become: - [ ] First\n- [ ] Second\n Detail for First + // but instead: - [ ] First\n Detail for First\n- [ ] Second + const desc = '### AC\n- [ ] First\n Detail for First\n\n### AC\n- [ ] Second'; + + const result = upsertChecklistSection(desc, 'AC', []); + + expect(result).toBe('### AC\n- [ ] First\n Detail for First\n- [ ] Second'); + // Heading appears exactly once. + expect(result.match(/^### AC$/gm)).toHaveLength(1); + // Detail line is still present and not displaced. + expect(result).toContain(' Detail for First'); + // Detail immediately follows First, not Second. + expect(result.indexOf(' Detail for First')).toBeLessThan(result.indexOf('- [ ] Second')); + }); + + it('detail line from a duplicate section travels with its checkbox into the first section', () => { + // Exact reviewer repro (review comment #3226669603): + // upsertChecklistSection("### AC\n- [ ] First\n\n### AC\n- [ ] Second\n Detail for Second", "AC", []) + // must NOT orphan " Detail for Second" after a blank line; + // it must be emitted immediately after "- [ ] Second" in the merged section. + const desc = '### AC\n- [ ] First\n\n### AC\n- [ ] Second\n Detail for Second'; + + const result = upsertChecklistSection(desc, 'AC', []); + + // Heading appears exactly once. + expect(result.match(/^### AC$/gm)).toHaveLength(1); + // Both items present. + expect(result).toContain('- [ ] First'); + expect(result).toContain('- [ ] Second'); + // Detail line is present and immediately follows Second (not orphaned after a blank line). + expect(result).toContain(' Detail for Second'); + const secondIdx = result.indexOf('- [ ] Second'); + const detailIdx = result.indexOf(' Detail for Second'); + const blankAfterSecond = result.indexOf('\n\n', secondIdx); + // Detail must appear after Second but before any blank line gap (i.e. no orphan gap). + expect(detailIdx).toBeGreaterThan(secondIdx); + expect(blankAfterSecond === -1 || detailIdx < blankAfterSecond).toBe(true); + }); + + it('detail from duplicate section stays attached when existing item in section 1 has no prior detail', () => { + // Reviewer repro (review comment #3226887541): + // When section 1 has "- [ ] First" with no detail but the duplicate has + // "- [ ] First\n Detail for duplicate First", convergence must NOT emit + // " Detail for duplicate First" as orphaned prose after a blank line. + // It must be attached to "First" in the merged section. + const desc = '### AC\n- [ ] First\n\n### AC\n- [ ] First\n Detail for duplicate First'; + + const result = upsertChecklistSection(desc, 'AC', []); + + // Heading appears exactly once. + expect(result.match(/^### AC$/gm)).toHaveLength(1); + // Item present. + expect(result).toContain('- [ ] First'); + // Detail is present. + expect(result).toContain(' Detail for duplicate First'); + // Detail must immediately follow First with no blank-line gap (not orphaned). + const firstIdx = result.indexOf('- [ ] First'); + const detailIdx = result.indexOf(' Detail for duplicate First'); + const blankAfterFirst = result.indexOf('\n\n', firstIdx); + expect(detailIdx).toBeGreaterThan(firstIdx); + expect(blankAfterFirst === -1 || detailIdx < blankAfterFirst).toBe(true); + }); +}); + +describe('upsertItemInChecklist', () => { + it('adds missing item to an existing section', () => { + const result = upsertItemInChecklist('### AC\n- [ ] Existing', 'AC', 'New item'); + expect(result).toBe('### AC\n- [ ] Existing\n- [ ] New item'); + }); + + it('does not append an identical row on retry', () => { + const result = upsertItemInChecklist('### AC\n- [ ] Existing', 'AC', 'Existing'); + expect(result).toBe('### AC\n- [ ] Existing'); + }); + + it('preserves checked state when any duplicate row is checked', () => { + const desc = '### AC\n- [ ] Item\n\n### AC\n- [x] Item'; + const result = upsertItemInChecklist(desc, 'AC', 'Item'); + expect(result).toBe('### AC\n- [x] Item'); + }); + + it('throws when checklist section does not exist', () => { + expect(() => upsertItemInChecklist('No checklist here.', 'AC', 'Item')).toThrow(); + }); +}); + +describe('semantic checklist predicates', () => { + it('recognizes requested checklist items after duplicate section convergence', () => { + const desc = '### AC\n- [ ] First\n\n### AC\n- [x] Second'; + + expect( + checklistSectionContainsItems(desc, 'AC', [ + { name: 'First', checked: false }, + { name: 'Second', checked: true }, + ]), + ).toBe(true); + }); + + it('treats checked rows as satisfying unchecked create requests', () => { + expect( + checklistSectionContainsItems('### AC\n- [x] First', 'AC', [ + { name: 'First', checked: false }, + ]), + ).toBe(true); + }); + + it('requires exact state for item update satisfaction', () => { + expect(checklistItemStateSatisfied('### AC\n- [x] First', 'AC', 'First', true)).toBe(true); + expect(checklistItemStateSatisfied('### AC\n- [x] First', 'AC', 'First', false)).toBe(false); + }); +}); + // --------------------------------------------------------------------------- // toggleChecklistItem // --------------------------------------------------------------------------- diff --git a/tests/unit/pm/jira/adapter.test.ts b/tests/unit/pm/jira/adapter.test.ts index 5592c061..a7b70665 100644 --- a/tests/unit/pm/jira/adapter.test.ts +++ b/tests/unit/pm/jira/adapter.test.ts @@ -80,6 +80,7 @@ describe('JiraPMProvider', () => { beforeEach(() => { vi.resetAllMocks(); provider = new JiraPMProvider(mockConfig); + process.env.CASCADE_DESCRIPTION_MUTATION_LOCK_DIR = `/tmp/cascade-jira-test-locks-${process.pid}-${Date.now()}-${Math.random()}`; mockAdfToPlainText.mockReturnValue('plain text description'); mockMarkdownToAdf.mockReturnValue({ type: 'doc', version: 1, content: [] }); // Default: no media nodes found (most tests don't need media extraction) @@ -640,6 +641,48 @@ describe('JiraPMProvider', () => { expect(result.items[0].id).toMatch(/^cl-[0-9a-f]{8}$/); }); + it('does not duplicate checklist sections on repeated bulk creation', async () => { + let markdown = 'Existing.\n\n### ✅ AC\n- [x] Done item'; + mockJiraClient.getIssue.mockResolvedValue({ + fields: { description: { type: 'doc', content: [] } }, + }); + mockAdfToPlainText.mockImplementation(() => markdown); + mockMarkdownToAdf.mockImplementation((nextMarkdown) => nextMarkdown); + mockJiraClient.updateIssue.mockImplementation(async (_id, updates) => { + markdown = updates.description as string; + }); + + await provider.createChecklistWithItems('PROJ-1', '✅ AC', [ + { name: 'First item' }, + { name: 'Done item' }, + ]); + await provider.createChecklistWithItems('PROJ-1', '✅ AC', [ + { name: 'First item' }, + { name: 'Done item' }, + ]); + + expect(markdown).toBe('Existing.\n\n### ✅ AC\n- [x] Done item\n- [ ] First item'); + expect(markdown.match(/^### ✅ AC$/gm)).toHaveLength(1); + expect(markdown.match(/First item/g)).toHaveLength(1); + }); + + it('merges duplicate checklist sections through the ADF round trip', async () => { + mockJiraClient.getIssue.mockResolvedValue({ + fields: { description: { type: 'doc', content: [] } }, + }); + mockAdfToPlainText.mockReturnValue( + '### ✅ AC\n- [ ] First\n\n### ✅ AC\n- [x] First\n- [ ] Second', + ); + mockMarkdownToAdf.mockImplementation((nextMarkdown) => nextMarkdown); + mockJiraClient.updateIssue.mockResolvedValue(undefined); + + await provider.createChecklistWithItems('PROJ-1', '✅ AC', [{ name: 'Third' }]); + + expect(mockMarkdownToAdf).toHaveBeenCalledWith( + '### ✅ AC\n- [x] First\n- [ ] Second\n- [ ] Third', + ); + }); + it('preserves concurrent bulk-created checklist sections', async () => { let markdown = 'Existing.'; mockJiraClient.getIssue.mockResolvedValue({ @@ -718,6 +761,25 @@ describe('JiraPMProvider', () => { 'Invalid JIRA checklist ID', ); }); + + it('does not duplicate a markdown checkbox on retry', async () => { + let markdown = '### ✅ AC\n- [x] Existing'; + mockJiraClient.getIssue.mockResolvedValue({ + fields: { description: { type: 'doc', content: [] } }, + }); + mockAdfToPlainText.mockImplementation(() => markdown); + mockMarkdownToAdf.mockImplementation((nextMarkdown) => nextMarkdown); + mockJiraClient.updateIssue.mockImplementation(async (_id, updates) => { + markdown = updates.description as string; + }); + + const checklist = await provider.createChecklist('PROJ-1', '✅ AC'); + await provider.addChecklistItem(checklist.id, 'Existing'); + await provider.addChecklistItem(checklist.id, 'Existing'); + + expect(markdown).toBe('### ✅ AC\n- [x] Existing'); + expect(markdown.match(/Existing/g)).toHaveLength(1); + }); }); describe('updateChecklistItem (inline)', () => { diff --git a/tests/unit/pm/linear/adapter.test.ts b/tests/unit/pm/linear/adapter.test.ts index e6cf2eb8..7639d959 100644 --- a/tests/unit/pm/linear/adapter.test.ts +++ b/tests/unit/pm/linear/adapter.test.ts @@ -102,6 +102,7 @@ describe('LinearPMProvider', () => { beforeEach(() => { provider = new LinearPMProvider(defaultConfig); + process.env.CASCADE_DESCRIPTION_MUTATION_LOCK_DIR = `/tmp/cascade-linear-test-locks-${process.pid}-${Date.now()}-${Math.random()}`; vi.clearAllMocks(); __resetRecentDescriptionsForTests(); }); @@ -563,12 +564,38 @@ describe('LinearPMProvider', () => { expect(result.items[0].id).toMatch(/^cl-[0-9a-f]{8}$/); }); - // MNG-741 regression (2026-05-12): the original fix at commit fad4dda1 - // polled Linear's GET after every PUT with a 1s deadline, then THREW - // on timeout. Linear's eventual-consistency window is routinely >1s - // under load — every planning run hit this and got bounced. The new - // contract: skip the visibility poll entirely, and provide read-after- - // write consistency via an in-process recent-description cache. + it('does not duplicate checklist sections on repeated bulk creation', async () => { + const state = mockIssueDescription('Existing.\n\n### ✅ AC\n- [x] Done item'); + + await provider.createChecklistWithItems('issue-uuid', '✅ AC', [ + { name: 'First item' }, + { name: 'Done item' }, + ]); + await provider.createChecklistWithItems('issue-uuid', '✅ AC', [ + { name: 'First item' }, + { name: 'Done item' }, + ]); + + expect(state.description).toBe('Existing.\n\n### ✅ AC\n- [x] Done item\n- [ ] First item'); + expect(state.description?.match(/^### ✅ AC$/gm)).toHaveLength(1); + expect(state.description?.match(/First item/g)).toHaveLength(1); + expect(state.description).toContain('- [x] Done item'); + }); + + it('keeps duplicate provider echoes idempotent', async () => { + let description = 'Existing.\n\n### ✅ AC\n- [ ] First item'; + mockGetIssue.mockImplementation(async () => makeIssue({ description })); + mockUpdateIssue.mockImplementation(async (_id, updates: { description?: string }) => { + description = `${updates.description ?? description}\n\n### ✅ AC\n- [ ] First item`; + return makeIssue({ description }); + }); + + await expect( + provider.createChecklistWithItems('issue-uuid', '✅ AC', [{ name: 'First item' }]), + ).resolves.toBeDefined(); + expect(mockUpdateIssue).toHaveBeenCalledTimes(1); + }); + it('uses recent-description cache instead of polling Linear after a PUT', async () => { let description = 'Existing.'; let staleDescription: string | null = null; @@ -592,23 +619,9 @@ describe('LinearPMProvider', () => { expect(description).toBe('Existing.\n\n### ✅ AC\n- [ ] First item'); }); - it('proceeds without throwing when Linear NEVER returns the updated description (MNG-741)', async () => { - // Worst-case path: Linear's GET continues to return the stale description - // forever after the PUT (e.g. the read replica is broken or just very slow - // — Linear's eventual-consistency window can exceed seconds under load). - // Previously this would throw "Linear description visibility timed out" - // after 1s and the agent's `cascade-tools pm add-checklist` invocation - // would exit non-zero, skip the sidecar write, and the run would fail the - // requiresPMWrite gate. Prod incident: MNG-741 / MNG-736 / MNG-739 - // (2026-05-12, run 1ce6ed4a). New contract: PUT succeeded from Linear's - // perspective; trust the cache for subsequent in-process operations, - // never throw on visibility-poll failure. + it('proceeds without throwing when Linear never returns the updated description', async () => { const persistedDescription = 'Existing.'; - mockGetIssue.mockImplementation(async () => - // IMPORTANT: GET always returns the stale pre-PUT value, simulating - // Linear's read replica being permanently behind for the test. - makeIssue({ description: persistedDescription }), - ); + mockGetIssue.mockImplementation(async () => makeIssue({ description: persistedDescription })); mockUpdateIssue.mockImplementation(async (_id, _updates: { description?: string }) => makeIssue({ description: persistedDescription }), ); @@ -616,13 +629,8 @@ describe('LinearPMProvider', () => { const checklist = await provider.createChecklist('issue-uuid', '✅ AC'); expect(checklist.name).toBe('✅ AC'); - // Subsequent operation finds the checklist via the in-process cache, - // even though GET keeps returning the stale pre-PUT description. await expect(provider.addChecklistItem(checklist.id, 'First item')).resolves.toBeUndefined(); - // The last PUT body MUST include the appended item. If the cache wasn't - // used, mutate() would run against the stale GET → no checklist found → - // throw (or, worse, silently overwrite without the section). const lastPut = mockUpdateIssue.mock.calls[mockUpdateIssue.mock.calls.length - 1]; expect(lastPut[1].description).toContain('### ✅ AC'); expect(lastPut[1].description).toContain('- [ ] First item'); @@ -739,6 +747,17 @@ describe('LinearPMProvider', () => { const lastCall = mockUpdateIssue.mock.calls[mockUpdateIssue.mock.calls.length - 1]; expect(lastCall[1].description).toContain('- [x] Done item'); }); + + it('does not duplicate a markdown checkbox on retry', async () => { + const state = mockIssueDescription('### ✅ AC\n- [x] Existing'); + + const checklist = await provider.createChecklist('issue-uuid', '✅ AC'); + await provider.addChecklistItem(checklist.id, 'Existing'); + await provider.addChecklistItem(checklist.id, 'Existing'); + + expect(state.description).toBe('### ✅ AC\n- [x] Existing'); + expect(state.description?.match(/Existing/g)).toHaveLength(1); + }); }); describe('updateChecklistItem (inline)', () => { @@ -832,6 +851,96 @@ describe('LinearPMProvider', () => { expect(mockGetIssue).toHaveBeenCalledTimes(1); expect(mockUpdateIssue).not.toHaveBeenCalled(); }); + + it('does not duplicate checklist content when Linear readback stays stale', async () => { + let description = 'Existing.'; + mockGetIssue.mockImplementation(async () => + makeIssue({ + description: description !== 'Existing.' ? 'Existing.' : description, + }), + ); + mockUpdateIssue.mockImplementation(async (_id, updates: { description?: string }) => { + description = updates.description ?? description; + return makeIssue({ description }); + }); + + await provider.createChecklistWithItems('issue-uuid', '✅ AC', [{ name: 'First item' }]); + expect(mockUpdateIssue).toHaveBeenCalledTimes(1); + + await provider.createChecklistWithItems('issue-uuid', '✅ AC', [{ name: 'First item' }]); + + expect(description).toBe('Existing.\n\n### ✅ AC\n- [ ] First item'); + expect(description.match(/^### ✅ AC$/gm)).toHaveLength(1); + expect(description.match(/First item/g)).toHaveLength(1); + }); + + it('uses cross-process sidecar as fresh base when in-process cache is cold (different process)', async () => { + // Regression: review comment #3226669608 — the in-process recentDescriptions cache + // doesn't help a *second OS process* that acquires the lock after the first exits. + // The sidecar file written while holding the lock provides the cross-process fresh base. + // + // Simulate: process A writes checklist → clears in-process cache (new process B) → + // Linear GET returns stale description → process B must read sidecar, not Linear. + let description = 'Existing.'; + const staleDescription = 'Existing.'; // Linear always returns pre-write state (stale) + mockGetIssue.mockImplementation(async () => makeIssue({ description: staleDescription })); + mockUpdateIssue.mockImplementation(async (_id, updates: { description?: string }) => { + description = updates.description ?? description; + return makeIssue({ description }); + }); + + // Process A: create checklist — sidecar is written while lock is held. + await provider.createChecklistWithItems('issue-uuid', '✅ AC', [{ name: 'First item' }]); + expect(mockUpdateIssue).toHaveBeenCalledTimes(1); + + // Simulate process boundary: wipe the in-process cache. + // Without the sidecar, the next call would base off stale Linear GET. + __resetRecentDescriptionsForTests(); + + // Process B: retry with the same args while Linear still returns stale description. + await provider.createChecklistWithItems('issue-uuid', '✅ AC', [{ name: 'First item' }]); + + // The sidecar (written by process A) must have served as the fresh base so the + // checklist section and item appear exactly once — not duplicated. + expect(description).toBe('Existing.\n\n### ✅ AC\n- [ ] First item'); + expect(description.match(/^### ✅ AC$/gm)).toHaveLength(1); + expect(description.match(/First item/g)).toHaveLength(1); + }); + + it('preserves human edits that arrived between cascade writes (review #3226887552)', async () => { + // Reviewer repro: the sidecar was preferred unconditionally over the + // provider read, so a human edit that appeared in Linear *after* cascade's + // last write was silently dropped when a second cascade process added items. + // The fix: prefer the provider read when it already contains cascade's last + // known-good write (i.e. the provider is current, not stale). + let description = 'Existing.'; + mockGetIssue.mockImplementation(async () => makeIssue({ description })); + mockUpdateIssue.mockImplementation(async (_id, updates: { description?: string }) => { + description = updates.description ?? description; + return makeIssue({ description }); + }); + + // Process A: create checklist. + await provider.createChecklistWithItems('issue-uuid', '✅ AC', [{ name: 'First item' }]); + expect(description).toBe('Existing.\n\n### ✅ AC\n- [ ] First item'); + + // Human edits the description directly in Linear after cascade's write. + description += '\nHuman edit that should survive.'; + + // Simulate process boundary: clear in-process cache (new process B). + __resetRecentDescriptionsForTests(); + + // Process B: Linear GET now returns the human-edited description (current), + // while the sidecar still has the pre-human-edit snapshot. + await provider.createChecklistWithItems('issue-uuid', '✅ AC', [{ name: 'Second item' }]); + + // Human edit and both cascade items must be present, without duplication. + expect(description).toContain('Human edit that should survive.'); + expect(description).toContain('- [ ] First item'); + expect(description).toContain('- [ ] Second item'); + expect(description.match(/First item/g)).toHaveLength(1); + expect(description.match(/Second item/g)).toHaveLength(1); + }); }); // =========================================================================