From d614244583e8eb001a1860c1e19b08b3c6f974f9 Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Tue, 12 May 2026 12:20:22 +0000 Subject: [PATCH 1/8] fix(pm): make inline checklist retries idempotent --- CHANGELOG.md | 4 +- src/integrations/README.md | 4 +- src/pm/_shared/inline-checklist.ts | 293 ++++++++++++++++-- src/pm/jira/adapter.ts | 10 +- src/pm/linear/adapter.ts | 51 +-- .../unit/pm/_shared/inline-checklist.test.ts | 117 +++++++ tests/unit/pm/jira/adapter.test.ts | 62 ++++ tests/unit/pm/linear/adapter.test.ts | 95 ++++-- 8 files changed, 542 insertions(+), 94 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 085c7ee7..a7f5ded2 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 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..474342ba 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 removed, 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. 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/inline-checklist.ts b/src/pm/_shared/inline-checklist.ts index f8063a71..fd9ef122 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,202 @@ interface SectionScan { lastContentIdx: number; } +interface ChecklistSectionSpan { + name: string; + startIdx: number; + endIdx: number; +} + +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]; + const mergedItems = collectMergedSectionItems(lines, sections); + const rewrittenFirstSection = rewriteChecklistSection( + lines.slice(first.startIdx, first.endIdx), + mergedItems, + ); + return removeDuplicateChecklistSections(lines, sections, rewrittenFirstSection); +} + +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; +} + +function collectMergedSectionItems( + lines: string[], + sections: ChecklistSectionSpan[], +): Map { + const mergedItems = new Map(); + for (const section of sections) { + for (const [name, checked] of collectSectionItems(lines, section)) { + mergedItems.set(name, (mergedItems.get(name) ?? false) || checked); + } + } + return mergedItems; +} + +function rewriteChecklistSection( + sectionLines: string[], + mergedItems: Map, +): string[] { + const state = { lines: [] as string[], seen: new Set(), lastCheckboxIdx: -1 }; + for (const line of sectionLines) { + rewriteChecklistSectionLine(state, line, mergedItems); + } + insertMissingChecklistItemLines(state, mergedItems); + return state.lines; +} + +function rewriteChecklistSectionLine( + state: { lines: string[]; seen: Set; lastCheckboxIdx: number }, + line: string, + mergedItems: Map, +): 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 checked = (mergedItems.get(itemName) ?? false) || cbMatch[1] === 'x'; + state.lines.push(`- [${checked ? 'x' : ' '}] ${itemName}`); + state.lastCheckboxIdx = state.lines.length - 1; +} + +function insertMissingChecklistItemLines( + state: { lines: string[]; seen: Set; lastCheckboxIdx: number }, + mergedItems: Map, +): void { + const missingItemLines: string[] = []; + for (const [itemName, checked] of mergedItems) { + if (!state.seen.has(itemName)) { + missingItemLines.push(`- [${checked ? 'x' : ' '}] ${itemName}`); + } + } + if (missingItemLines.length > 0) { + const insertIdx = state.lastCheckboxIdx === -1 ? 1 : state.lastCheckboxIdx + 1; + state.lines.splice(insertIdx, 0, ...missingItemLines); + } +} + +function removeDuplicateChecklistSections( + lines: string[], + sections: ChecklistSectionSpan[], + rewrittenFirstSection: string[], +): 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); + 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); +} + +function skipRemovedDuplicateSection( + lines: string[], + output: string[], + duplicate: ChecklistSectionSpan, +): number { + let nextIdx = duplicate.endIdx; + while (output.length > 0 && output[output.length - 1].trim() === '') output.pop(); + 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..1d5fcd05 100644 --- a/src/pm/linear/adapter.ts +++ b/src/pm/linear/adapter.ts @@ -13,8 +13,6 @@ import { linearClient } from '../../linear/client.js'; import { logger } from '../../utils/logging.js'; import { withDescriptionMutationLock } from '../_shared/description-mutation-lock.js'; import { - addItemToChecklist, - appendChecklistSection, buildChecklistId, findChecklistNameByHash, hashChecklistItemId, @@ -22,6 +20,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 +41,12 @@ 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. After each successful PUT, keep the new + * description here so the next locked mutation starts from the freshest + * in-process value instead of overwriting it with a stale read. * - * 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. - * - * 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. + * Scope: in-process only, matching `withDescriptionMutationLock`. */ const RECENT_DESCRIPTION_TTL_MS = 60_000; type RecentDescription = { description: string; timestamp: number }; @@ -65,7 +54,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 +72,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 +272,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 +286,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 +322,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, @@ -393,11 +377,6 @@ 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 ?? ''); diff --git a/tests/unit/pm/_shared/inline-checklist.test.ts b/tests/unit/pm/_shared/inline-checklist.test.ts index aa59276c..35cdf4f5 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,119 @@ 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'); + }); +}); + +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..ab824cb1 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,28 @@ 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); + }); }); // ========================================================================= From 665863b15eac7e0fa8f4ca78096275300437c279 Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Tue, 12 May 2026 12:39:31 +0000 Subject: [PATCH 2/8] fix(pm): preserve non-checkbox prose from duplicate checklist sections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `removeDuplicateChecklistSections()` previously skipped the entire span of a duplicate section, silently dropping any non-checkbox prose a user had added inside it (e.g. detail notes interleaved with items). Fix: before skipping a duplicate section, collect its non-checkbox lines (excluding the heading and blank lines), strip leading/trailing whitespace, and emit the preserved prose after the merged section content — keeping user-edited text while still converging duplicate checkbox rows into the first matching section. Adds a regression test pinning the data-loss path reported in review #3226378053 (MNG-741 follow-up). Co-Authored-By: Claude Sonnet 4.6 --- src/pm/_shared/inline-checklist.ts | 22 ++++++++++++++++++- .../unit/pm/_shared/inline-checklist.test.ts | 15 +++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/pm/_shared/inline-checklist.ts b/src/pm/_shared/inline-checklist.ts index fd9ef122..12d971fd 100644 --- a/src/pm/_shared/inline-checklist.ts +++ b/src/pm/_shared/inline-checklist.ts @@ -509,8 +509,28 @@ function skipRemovedDuplicateSection( output: string[], duplicate: ChecklistSectionSpan, ): number { - let nextIdx = duplicate.endIdx; + // Collect non-checkbox prose lines from the duplicate section. + // Checkbox items have already been merged into the first section's rewrite; + // the heading line itself is also dropped. Only non-generated prose survives. + const proseLines: string[] = []; + for (let i = duplicate.startIdx + 1; i < duplicate.endIdx; i++) { + if (!CHECKBOX_REGEX.test(lines[i])) { + proseLines.push(lines[i]); + } + } + // 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(''); diff --git a/tests/unit/pm/_shared/inline-checklist.test.ts b/tests/unit/pm/_shared/inline-checklist.test.ts index 35cdf4f5..d56b2c69 100644 --- a/tests/unit/pm/_shared/inline-checklist.test.ts +++ b/tests/unit/pm/_shared/inline-checklist.test.ts @@ -280,6 +280,21 @@ Keep me.`); 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'); + }); }); describe('upsertItemInChecklist', () => { From b4773796cbbe53750766d2c6eced047a33d0b236 Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Tue, 12 May 2026 13:00:14 +0000 Subject: [PATCH 3/8] fix(pm): insert merged checklist rows after trailing detail lines `insertMissingChecklistItemLines()` was inserting at `lastCheckboxIdx + 1`, which placed newly-merged rows *before* any non-blank detail/prose lines that followed the last checkbox in the first section. The result was that those detail lines were visually re-attributed to the wrong (newly-inserted) item. Example: `### AC\n- [ ] First\n Detail for First\n\n### AC\n- [ ] Second` produced `- [ ] First\n- [ ] Second\n Detail for First` instead of `- [ ] First\n Detail for First\n- [ ] Second`. Fix: scan forward from `lastCheckboxIdx + 1` through non-blank lines before setting the splice point, stopping at the first blank line (to avoid crossing section boundaries). This mirrors the semantics already used by `findChecklistInsertionIndex()` / `addItemToChecklist()`. Adds a focused regression test for the exact data-corruption scenario reported in review comment #3226513749. Co-Authored-By: Claude Sonnet 4.6 --- src/pm/_shared/inline-checklist.ts | 23 ++++++++++++++++++- .../unit/pm/_shared/inline-checklist.test.ts | 21 +++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/pm/_shared/inline-checklist.ts b/src/pm/_shared/inline-checklist.ts index 12d971fd..3ea77b42 100644 --- a/src/pm/_shared/inline-checklist.ts +++ b/src/pm/_shared/inline-checklist.ts @@ -467,7 +467,28 @@ function insertMissingChecklistItemLines( } } if (missingItemLines.length > 0) { - const insertIdx = state.lastCheckboxIdx === -1 ? 1 : state.lastCheckboxIdx + 1; + // Determine insertion point: immediately after the last checkbox AND any + // trailing non-blank detail/prose lines that follow it. Inserting at + // lastCheckboxIdx + 1 would place new rows before those detail lines, + // visually re-attributing them to the wrong (newly-inserted) item. + // We stop advancing at the first blank line so we never cross a section + // boundary or an intentional separator. + let insertIdx: number; + if (state.lastCheckboxIdx === -1) { + insertIdx = 1; // No checkboxes yet; insert right after the heading. + } else { + insertIdx = state.lastCheckboxIdx; + for (let i = state.lastCheckboxIdx + 1; i < state.lines.length; i++) { + if (state.lines[i].trim() !== '') { + // Non-blank detail/prose line — advance past it. + insertIdx = i; + } else { + // Blank line — stop; don't cross section boundaries. + break; + } + } + insertIdx += 1; + } state.lines.splice(insertIdx, 0, ...missingItemLines); } } diff --git a/tests/unit/pm/_shared/inline-checklist.test.ts b/tests/unit/pm/_shared/inline-checklist.test.ts index d56b2c69..f60121c6 100644 --- a/tests/unit/pm/_shared/inline-checklist.test.ts +++ b/tests/unit/pm/_shared/inline-checklist.test.ts @@ -295,6 +295,27 @@ Keep me.`); // 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')); + }); }); describe('upsertItemInChecklist', () => { From ee50f5826c0bff1248795b75915d9b95ced0d590 Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Tue, 12 May 2026 13:01:28 +0000 Subject: [PATCH 4/8] refactor(pm): extract findMissingItemsInsertionIndex to reduce complexity Extract the trailing-detail-line scan from insertMissingChecklistItemLines into its own named helper to bring cognitive complexity below the biome threshold (was 18, max is 15). Co-Authored-By: Claude Sonnet 4.6 --- src/pm/_shared/inline-checklist.ts | 47 ++++++++++++++++-------------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/src/pm/_shared/inline-checklist.ts b/src/pm/_shared/inline-checklist.ts index 3ea77b42..56a27f9d 100644 --- a/src/pm/_shared/inline-checklist.ts +++ b/src/pm/_shared/inline-checklist.ts @@ -456,6 +456,30 @@ function rewriteChecklistSectionLine( state.lastCheckboxIdx = state.lines.length - 1; } +/** + * 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, @@ -467,28 +491,7 @@ function insertMissingChecklistItemLines( } } if (missingItemLines.length > 0) { - // Determine insertion point: immediately after the last checkbox AND any - // trailing non-blank detail/prose lines that follow it. Inserting at - // lastCheckboxIdx + 1 would place new rows before those detail lines, - // visually re-attributing them to the wrong (newly-inserted) item. - // We stop advancing at the first blank line so we never cross a section - // boundary or an intentional separator. - let insertIdx: number; - if (state.lastCheckboxIdx === -1) { - insertIdx = 1; // No checkboxes yet; insert right after the heading. - } else { - insertIdx = state.lastCheckboxIdx; - for (let i = state.lastCheckboxIdx + 1; i < state.lines.length; i++) { - if (state.lines[i].trim() !== '') { - // Non-blank detail/prose line — advance past it. - insertIdx = i; - } else { - // Blank line — stop; don't cross section boundaries. - break; - } - } - insertIdx += 1; - } + const insertIdx = findMissingItemsInsertionIndex(state.lines, state.lastCheckboxIdx); state.lines.splice(insertIdx, 0, ...missingItemLines); } } From bcfd7bf583692f1fe7628e506000233223a6ef8a Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Tue, 12 May 2026 12:57:56 +0000 Subject: [PATCH 5/8] docs(pm): align linear checklist retry docs --- CHANGELOG.md | 2 +- src/integrations/README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a7f5ded2..b540971f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ 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 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). +- **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). diff --git a/src/integrations/README.md b/src/integrations/README.md index 474342ba..c5e31b8d 100644 --- a/src/integrations/README.md +++ b/src/integrations/README.md @@ -295,7 +295,7 @@ 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, 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 removed, 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. +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 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. From 682c95575ffbc584f03312158b536db99161e8f9 Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Tue, 12 May 2026 13:32:31 +0000 Subject: [PATCH 6/8] =?UTF-8?q?fix(pm):=20address=20review=20feedback=20?= =?UTF-8?q?=E2=80=94=20detail-line=20travel=20and=20cross-process=20sideca?= =?UTF-8?q?r?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Inline-checklist fix (review comment #3226669603): detail lines in a duplicate section now travel WITH their checkbox into the first section rather than being orphaned after a blank gap. `skipRemovedDuplicateSection` now distinguishes "new" items (detail is skipped — it's emitted in section 1) vs "existing" items (detail preserved as prose). `insertMissingChecklistItemLines` emits detail lines immediately after each newly-inserted checkbox. Cross-process sidecar fix (review comment #3226669608): replaced the in-process-only `recentDescriptions` cache with a dual-layer approach. After every successful `linearClient.updateIssue` call, the fresh description is written to a JSON sidecar file while the filesystem lock is held. The next process to acquire the lock reads the sidecar first, providing a durable cross-process fresh base even when Linear's GET still returns stale data. Adds regression tests for both scenarios. Co-Authored-By: Claude Sonnet 4.6 --- src/integrations/README.md | 2 +- src/pm/_shared/description-mutation-lock.ts | 69 +++++++- src/pm/_shared/inline-checklist.ts | 157 +++++++++++++++--- src/pm/linear/adapter.ts | 37 ++++- .../unit/pm/_shared/inline-checklist.test.ts | 24 +++ tests/unit/pm/linear/adapter.test.ts | 33 ++++ 6 files changed, 293 insertions(+), 29 deletions(-) diff --git a/src/integrations/README.md b/src/integrations/README.md index c5e31b8d..43413fb1 100644 --- a/src/integrations/README.md +++ b/src/integrations/README.md @@ -297,7 +297,7 @@ Different PM providers have different native concepts of "checklist". The `PMPro 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 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 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 56a27f9d..1d922d27 100644 --- a/src/pm/_shared/inline-checklist.ts +++ b/src/pm/_shared/inline-checklist.ts @@ -372,6 +372,18 @@ interface ChecklistSectionSpan { 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'); @@ -379,12 +391,21 @@ function dedupeChecklistSections(description: string, checklistName: string): st if (sections.length <= 1) return description; const first = sections[0]; - const mergedItems = collectMergedSectionItems(lines, sections); + // 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 rewrittenFirstSection = rewriteChecklistSection( lines.slice(first.startIdx, first.endIdx), mergedItems, ); - return removeDuplicateChecklistSections(lines, sections, rewrittenFirstSection); + return removeDuplicateChecklistSections( + lines, + sections, + rewrittenFirstSection, + firstSectionItems, + ); } function findChecklistInsertionIndex(lines: string[], checklistName: string): number { @@ -413,22 +434,72 @@ function findChecklistInsertionIndex(lines: string[], checklistName: string): nu return insertIdx; } -function collectMergedSectionItems( +/** + * 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 mergedItems = new Map(); +): Map { + const merged = new Map(); for (const section of sections) { - for (const [name, checked] of collectSectionItems(lines, section)) { - mergedItems.set(name, (mergedItems.get(name) ?? false) || checked); + 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 mergedItems; + return merged; } function rewriteChecklistSection( sectionLines: string[], - mergedItems: Map, + mergedItems: Map, ): string[] { const state = { lines: [] as string[], seen: new Set(), lastCheckboxIdx: -1 }; for (const line of sectionLines) { @@ -441,7 +512,7 @@ function rewriteChecklistSection( function rewriteChecklistSectionLine( state: { lines: string[]; seen: Set; lastCheckboxIdx: number }, line: string, - mergedItems: Map, + mergedItems: Map, ): void { const cbMatch = line.match(CHECKBOX_REGEX); if (!cbMatch) { @@ -451,9 +522,12 @@ function rewriteChecklistSectionLine( const itemName = cbMatch[2].trim(); if (state.seen.has(itemName)) return; state.seen.add(itemName); - const checked = (mergedItems.get(itemName) ?? false) || cbMatch[1] === 'x'; + 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; + // Note: we do NOT emit detail lines for existing items here — they come + // from the source sectionLines on subsequent iterations (preserved in-place). } /** @@ -482,12 +556,15 @@ function findMissingItemsInsertionIndex(lines: string[], lastCheckboxIdx: number function insertMissingChecklistItemLines( state: { lines: string[]; seen: Set; lastCheckboxIdx: number }, - mergedItems: Map, + mergedItems: Map, ): void { const missingItemLines: string[] = []; - for (const [itemName, checked] of mergedItems) { + for (const [itemName, info] of mergedItems) { if (!state.seen.has(itemName)) { - missingItemLines.push(`- [${checked ? 'x' : ' '}] ${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) { @@ -500,6 +577,7 @@ function removeDuplicateChecklistSections( lines: string[], sections: ChecklistSectionSpan[], rewrittenFirstSection: string[], + firstSectionItems: Set, ): string { const first = sections[0]; const output: string[] = []; @@ -511,7 +589,7 @@ function removeDuplicateChecklistSections( } const duplicate = findSectionStartingAt(sections, i); if (duplicate) { - i = skipRemovedDuplicateSection(lines, output, duplicate); + i = skipRemovedDuplicateSection(lines, output, duplicate, firstSectionItems); continue; } output.push(lines[i]); @@ -528,20 +606,59 @@ function findSectionStartingAt( 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 (items already in the first section): + * since those items are not being re-inserted, their duplicate-section + * detail cannot travel with them and is preserved as orphaned prose. + * + * - 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, ): number { - // Collect non-checkbox prose lines from the duplicate section. - // Checkbox items have already been merged into the first section's rewrite; - // the heading line itself is also dropped. Only non-generated prose survives. const proseLines: string[] = []; + let currentItemName: string | null = null; + let currentItemIsNew = false; + for (let i = duplicate.startIdx + 1; i < duplicate.endIdx; i++) { - if (!CHECKBOX_REGEX.test(lines[i])) { - proseLines.push(lines[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) { + // Detail line of an EXISTING item — preserve as prose since the item + // won't be re-inserted and its detail would otherwise be lost. + proseLines.push(line); } + // else: detail line of a NEW item — skipped; emitted 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(); diff --git a/src/pm/linear/adapter.ts b/src/pm/linear/adapter.ts index 1d5fcd05..02f8be01 100644 --- a/src/pm/linear/adapter.ts +++ b/src/pm/linear/adapter.ts @@ -11,7 +11,11 @@ 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 { + readLockedDescription, + withDescriptionMutationLock, + writeLockedDescription, +} from '../_shared/description-mutation-lock.js'; import { buildChecklistId, findChecklistNameByHash, @@ -42,11 +46,19 @@ 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. After each successful PUT, keep the new - * description here so the next locked mutation starts from the freshest - * in-process value instead of overwriting it with a stale read. + * return the previous description for seconds. After each successful PUT we + * store the new description in two places: + * + * 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, matching `withDescriptionMutationLock`. + * 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 }; @@ -365,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 { @@ -377,13 +395,18 @@ export class LinearPMProvider implements PMProvider { throw err; } - const cachedDescription = recallRecentDescription(issueId); + // Precedence: sidecar (cross-process durable) > in-process cache > + // Linear GET (may be stale due to eventual consistency). + const inProcessDescription = recallRecentDescription(issueId); + const freshDescription = sidecarDescription ?? inProcessDescription; const baseDescription = - cachedDescription !== undefined ? cachedDescription : (issue.description ?? ''); + freshDescription !== undefined ? freshDescription : (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 f60121c6..90d53ccd 100644 --- a/tests/unit/pm/_shared/inline-checklist.test.ts +++ b/tests/unit/pm/_shared/inline-checklist.test.ts @@ -316,6 +316,30 @@ Keep me.`); // 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); + }); }); describe('upsertItemInChecklist', () => { diff --git a/tests/unit/pm/linear/adapter.test.ts b/tests/unit/pm/linear/adapter.test.ts index ab824cb1..19e2d7f4 100644 --- a/tests/unit/pm/linear/adapter.test.ts +++ b/tests/unit/pm/linear/adapter.test.ts @@ -873,6 +873,39 @@ describe('LinearPMProvider', () => { 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); + }); }); // ========================================================================= From 2172030fe3e712866179290c36d804f02e5d668d Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Tue, 12 May 2026 14:00:04 +0000 Subject: [PATCH 7/8] fix(pm): keep duplicate-section detail attached and respect human edits in Linear sidecar Two data-corruption paths remain from the previous round: 1. **inline-checklist**: When section 1 had item "First" with no detail but the duplicate section had "First" with trailing detail, the detail was emitted as orphaned prose after a blank-line separator instead of being attached to the retained item. Fix: pre-scan section 1 to identify which items already carry their own detail; for items that do NOT have detail in section 1 but do have it in mergedItems (collected from a duplicate section), emit the detail immediately after the checkbox in the section-1 rewrite. Track the emitted items in `detailEmittedForItems` so `skipRemovedDuplicateSection` doesn't re-emit them as orphaned prose. 2. **Linear adapter**: The cross-process sidecar was preferred unconditionally over the provider read, so a human edit added to the Linear description after cascade's last write was silently dropped when a second cascade process ran. Fix: prefer the provider read when it contains the sidecar content (provider is current + has extra edits); fall back to the sidecar only when the provider read does NOT contain the sidecar (demonstrably stale Linear GET). Regression tests added for both scenarios. Co-Authored-By: Claude Sonnet 4.6 --- src/pm/_shared/inline-checklist.ts | 97 ++++++++++++++++--- src/pm/linear/adapter.ts | 34 ++++++- .../unit/pm/_shared/inline-checklist.test.ts | 24 +++++ tests/unit/pm/linear/adapter.test.ts | 35 +++++++ 4 files changed, 170 insertions(+), 20 deletions(-) diff --git a/src/pm/_shared/inline-checklist.ts b/src/pm/_shared/inline-checklist.ts index 1d922d27..5e63c941 100644 --- a/src/pm/_shared/inline-checklist.ts +++ b/src/pm/_shared/inline-checklist.ts @@ -396,15 +396,16 @@ function dedupeChecklistSections(description: string, checklistName: string): st // 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 rewrittenFirstSection = rewriteChecklistSection( + const { lines: rewrittenLines, detailEmittedForItems } = rewriteChecklistSection( lines.slice(first.startIdx, first.endIdx), mergedItems, ); return removeDuplicateChecklistSections( lines, sections, - rewrittenFirstSection, + rewrittenLines, firstSectionItems, + detailEmittedForItems, ); } @@ -500,19 +501,62 @@ function collectMergedSectionItemsWithDetail( function rewriteChecklistSection( sectionLines: string[], mergedItems: Map, -): string[] { +): { 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); + rewriteChecklistSectionLine( + state, + line, + mergedItems, + section1ItemsWithDetail, + detailEmittedForItems, + ); } insertMissingChecklistItemLines(state, mergedItems); - return state.lines; + 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) { @@ -526,8 +570,17 @@ function rewriteChecklistSectionLine( const checked = (info?.checked ?? false) || cbMatch[1] === 'x'; state.lines.push(`- [${checked ? 'x' : ' '}] ${itemName}`); state.lastCheckboxIdx = state.lines.length - 1; - // Note: we do NOT emit detail lines for existing items here — they come - // from the source sectionLines on subsequent iterations (preserved in-place). + // 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). } /** @@ -578,6 +631,7 @@ function removeDuplicateChecklistSections( sections: ChecklistSectionSpan[], rewrittenFirstSection: string[], firstSectionItems: Set, + detailEmittedForItems: Set, ): string { const first = sections[0]; const output: string[] = []; @@ -589,7 +643,13 @@ function removeDuplicateChecklistSections( } const duplicate = findSectionStartingAt(sections, i); if (duplicate) { - i = skipRemovedDuplicateSection(lines, output, duplicate, firstSectionItems); + i = skipRemovedDuplicateSection( + lines, + output, + duplicate, + firstSectionItems, + detailEmittedForItems, + ); continue; } output.push(lines[i]); @@ -613,9 +673,11 @@ function findSectionStartingAt( * 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 (items already in the first section): - * since those items are not being re-inserted, their duplicate-section - * detail cannot travel with them and is preserved as orphaned prose. + * - 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 @@ -629,6 +691,7 @@ function skipRemovedDuplicateSection( output: string[], duplicate: ChecklistSectionSpan, firstSectionItems: Set, + detailEmittedForItems: Set, ): number { const proseLines: string[] = []; let currentItemName: string | null = null; @@ -651,12 +714,16 @@ function skipRemovedDuplicateSection( } else if (currentItemName === null) { // Standalone prose before the first checkbox (or after a blank gap). proseLines.push(line); - } else if (!currentItemIsNew) { - // Detail line of an EXISTING item — preserve as prose since the item - // won't be re-inserted and its detail would otherwise be lost. + } 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 line of a NEW item — skipped; emitted in section 1. + // 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. diff --git a/src/pm/linear/adapter.ts b/src/pm/linear/adapter.ts index 02f8be01..2eccc8b8 100644 --- a/src/pm/linear/adapter.ts +++ b/src/pm/linear/adapter.ts @@ -395,12 +395,36 @@ export class LinearPMProvider implements PMProvider { throw err; } - // Precedence: sidecar (cross-process durable) > in-process cache > - // Linear GET (may be stale due to eventual consistency). + // 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 freshDescription = sidecarDescription ?? inProcessDescription; - const baseDescription = - freshDescription !== undefined ? freshDescription : (issue.description ?? ''); + 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 }); diff --git a/tests/unit/pm/_shared/inline-checklist.test.ts b/tests/unit/pm/_shared/inline-checklist.test.ts index 90d53ccd..e89b7653 100644 --- a/tests/unit/pm/_shared/inline-checklist.test.ts +++ b/tests/unit/pm/_shared/inline-checklist.test.ts @@ -340,6 +340,30 @@ Keep me.`); 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', () => { diff --git a/tests/unit/pm/linear/adapter.test.ts b/tests/unit/pm/linear/adapter.test.ts index 19e2d7f4..7639d959 100644 --- a/tests/unit/pm/linear/adapter.test.ts +++ b/tests/unit/pm/linear/adapter.test.ts @@ -906,6 +906,41 @@ describe('LinearPMProvider', () => { 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); + }); }); // ========================================================================= From c2c879c1f9a6df23f4cea6971850871a4a1f2732 Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Tue, 12 May 2026 14:18:59 +0000 Subject: [PATCH 8/8] chore(ci): rerun codeql analysis