diff --git a/CHANGELOG.md b/CHANGELOG.md index 46cc1947..085c7ee7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ All notable user-visible changes to CASCADE are documented here. The format is l ### Fixed +- **`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 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/gadgets/README.md b/src/gadgets/README.md index b0848c2d..9e1c4004 100644 --- a/src/gadgets/README.md +++ b/src/gadgets/README.md @@ -40,6 +40,8 @@ comments: { The canonical parameter name always wins. Aliases are additive; suggestions returned by the fuzzy-matcher always point at the canonical form. +For array-of-object CLI params, the canonical input remains a JSON array. The shared parser also accepts one top-level JSON object and normalizes it to a one-element array so aliases like `--comment '{"path":"src/x.ts","line":1,"body":"nit"}'` behave the way agents naturally expect. Arrays pass through unchanged, and the parser validates only the top-level JSON shape; it does not inspect individual array entries. + ### `cli.fileInputAlternatives` Opt-in `---file ` escape hatches for long or JSON-shaped payloads that don't survive shell quoting. Pair with `parseAs: 'json'` for array-of-object / object params so the file contents are `JSON.parse`-d before reaching the gadget. @@ -169,7 +171,7 @@ Two tuning constants live in `src/gadgets/shared/cli/parseErrors.ts`: `MAX_FLAG_ ## Reference: `createPRReviewDef` -`src/gadgets/github/definitions.ts` is the reference gadget for spec 014 — it carries `cliAliases: ['comment']` on `comments`, `fileInputAlternatives` entries for `--body-file` (long review summaries) and `--comments-file` (JSON inline comments), and a well-formed `examples` block. Read it before you add a new gadget with array-of-object parameters. +`src/gadgets/github/definitions.ts` is the reference gadget for spec 014 — it carries `cliAliases: ['comment']` on `comments`, `fileInputAlternatives` entries for `--body-file` (long review summaries, reply bodies, and comment updates) and `--comments-file` (JSON inline comments), and a well-formed `examples` block. Read it before you add a new gadget with array-of-object parameters. ## Reference: `ReportFriction` diff --git a/src/gadgets/github/definitions.ts b/src/gadgets/github/definitions.ts index 43407d03..f5963b76 100644 --- a/src/gadgets/github/definitions.ts +++ b/src/gadgets/github/definitions.ts @@ -610,6 +610,13 @@ export const replyToReviewCommentDef: ToolDefinition = { ], cli: { autoResolved: ownerRepoAutoResolved, + fileInputAlternatives: [ + { + paramName: 'body', + fileFlag: 'body-file', + description: 'Read reply body from file (use - for stdin)', + }, + ], }, }; diff --git a/src/gadgets/shared/cli/params.ts b/src/gadgets/shared/cli/params.ts index 70d07782..970f4322 100644 --- a/src/gadgets/shared/cli/params.ts +++ b/src/gadgets/shared/cli/params.ts @@ -44,6 +44,58 @@ function parseJsonOrError( } } +function describeJsonTopLevel(value: unknown): string { + if (value === null) return 'null'; + if (Array.isArray(value)) return 'array'; + return typeof value; +} + +function normalizeArrayOfObjectJsonOrError( + parsed: unknown, + flag: string, + paramDef: ParameterDefinition, + fileAlt: FileInputAlternative | undefined, + example: unknown, + sink: ErrorSink, +): unknown { + if (paramDef.type !== 'array' || paramDef.items !== 'object') { + return parsed; + } + + if (Array.isArray(parsed)) { + return parsed; + } + + if (parsed !== null && typeof parsed === 'object') { + return [parsed]; + } + + const fileHint = fileAlt ? ` Or pass --${fileAlt.fileFlag} (or - for stdin).` : ''; + return emitCliError({ + type: 'json-parse', + flag, + message: `Expected JSON array or object, got ${describeJsonTopLevel(parsed)}`, + got: JSON.stringify(parsed), + expected: expectedShapeFor(paramDef, example), + hint: `Pass a JSON array of objects or one JSON object to normalize into a single-item array.${fileHint}`, + stdout: sink.stdout, + stderr: sink.stderr, + exit: sink.exit, + }); +} + +function parseJsonParamOrError( + raw: string, + flag: string, + paramDef: ParameterDefinition, + fileAlt: FileInputAlternative | undefined, + example: unknown, + sink: ErrorSink, +): unknown { + const parsed = parseJsonOrError(raw, flag, paramDef, fileAlt, example, sink); + return normalizeArrayOfObjectJsonOrError(parsed, flag, paramDef, fileAlt, example, sink); +} + function resolveFileInputParam( name: string, paramDef: ParameterDefinition, @@ -59,7 +111,14 @@ function resolveFileInputParam( if (typeof fileFlagValue === 'string' && fileFlagValue.length > 0) { const contents = readFileInput(fileFlagValue); if (fileAlt.parseAs === 'json') { - resolvedParams[name] = parseJsonOrError(contents, name, paramDef, fileAlt, example, sink); + resolvedParams[name] = parseJsonParamOrError( + contents, + name, + paramDef, + fileAlt, + example, + sink, + ); return; } resolvedParams[name] = contents; @@ -69,7 +128,14 @@ function resolveFileInputParam( if (directValue !== undefined && directValue !== null) { if (paramDef.type === 'array' && paramDef.items === 'object') { const asString = typeof directValue === 'string' ? directValue : JSON.stringify(directValue); - resolvedParams[name] = parseJsonOrError(asString, name, paramDef, fileAlt, example, sink); + resolvedParams[name] = parseJsonParamOrError( + asString, + name, + paramDef, + fileAlt, + example, + sink, + ); return; } if (typeof directValue === 'string') { @@ -103,7 +169,7 @@ function resolveObjectParam( if (typeof rawValue !== 'string') { return; } - resolvedParams[name] = parseJsonOrError(rawValue, name, paramDef, undefined, example, sink); + resolvedParams[name] = parseJsonParamOrError(rawValue, name, paramDef, undefined, example, sink); } function resolveArrayOfObjectParam( @@ -118,7 +184,7 @@ function resolveArrayOfObjectParam( const rawValue = flags[name]; if (rawValue === undefined) return; const asString = typeof rawValue === 'string' ? rawValue : JSON.stringify(rawValue); - resolvedParams[name] = parseJsonOrError(asString, name, paramDef, fileAlt, example, sink); + resolvedParams[name] = parseJsonParamOrError(asString, name, paramDef, fileAlt, example, sink); } function resolveStandardParam( diff --git a/src/pm/linear/adapter.ts b/src/pm/linear/adapter.ts index df4c791c..6356d5c8 100644 --- a/src/pm/linear/adapter.ts +++ b/src/pm/linear/adapter.ts @@ -38,8 +38,60 @@ import type { WorkItemLabel, } from '../types.js'; -const DESCRIPTION_VISIBILITY_TIMEOUT_MS = 1_000; -const DESCRIPTION_VISIBILITY_POLL_MS = 25; +/** + * 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. + * + * 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. + */ +const RECENT_DESCRIPTION_TTL_MS = 60_000; +type RecentDescription = { description: string; timestamp: number }; +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()) { + if (entry.timestamp < cutoff) recentDescriptions.delete(id); + } + } +} + +function recallRecentDescription(issueId: string): string | undefined { + const entry = recentDescriptions.get(issueId); + if (!entry) return undefined; + if (Date.now() - entry.timestamp > RECENT_DESCRIPTION_TTL_MS) { + recentDescriptions.delete(issueId); + return 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', @@ -341,10 +393,18 @@ export class LinearPMProvider implements PMProvider { throw err; } - const newDesc = mutate(issue.description ?? ''); + // 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 ?? ''); + const newDesc = mutate(baseDescription); try { await linearClient.updateIssue(issueId, { description: newDesc }); - await this.waitForDescriptionVisibility(issueId, newDesc); + rememberRecentDescription(issueId, newDesc); return; } catch (err) { if (attempt === 0) { @@ -356,30 +416,6 @@ export class LinearPMProvider implements PMProvider { } } - private async waitForDescriptionVisibility( - issueId: string, - expectedDescription: string, - ): Promise { - const startedAt = Date.now(); - const deadline = startedAt + DESCRIPTION_VISIBILITY_TIMEOUT_MS; - const expected = normalizeDescriptionForVisibility(expectedDescription); - - while (true) { - const issue = await linearClient.getIssue(issueId); - if (normalizeDescriptionForVisibility(issue.description ?? '') === expected) { - return; - } - - if (Date.now() >= deadline) { - throw new Error( - `Linear description visibility timed out for issue ${issueId} after ${DESCRIPTION_VISIBILITY_TIMEOUT_MS}ms`, - ); - } - - await sleep(Math.min(DESCRIPTION_VISIBILITY_POLL_MS, Math.max(0, deadline - Date.now()))); - } - } - private logDescriptionRetry(issueId: string, err: unknown): void { logger.warn('[Linear] Description provider update failed; retrying once', { issueId, @@ -455,11 +491,3 @@ export class LinearPMProvider implements PMProvider { }; } } - -function normalizeDescriptionForVisibility(description: string): string { - return description.replace(/\r\n/g, '\n').replace(/\r/g, '\n'); -} - -function sleep(ms: number): Promise { - return new Promise((resolve) => setTimeout(resolve, ms)); -} diff --git a/tests/unit/backends/shared-nativeToolPrompts.test.ts b/tests/unit/backends/shared-nativeToolPrompts.test.ts index fb0747b1..16e6daac 100644 --- a/tests/unit/backends/shared-nativeToolPrompts.test.ts +++ b/tests/unit/backends/shared-nativeToolPrompts.test.ts @@ -25,7 +25,11 @@ import { buildTaskPrompt, buildToolGuidance, } from '../../../src/backends/shared/nativeToolPrompts.js'; -import { createPRReviewDef, updatePRCommentDef } from '../../../src/gadgets/github/definitions.js'; +import { + createPRReviewDef, + replyToReviewCommentDef, + updatePRCommentDef, +} from '../../../src/gadgets/github/definitions.js'; import { readWorkItemDef } from '../../../src/gadgets/pm/definitions.js'; import { generateToolManifest } from '../../../src/gadgets/shared/manifestGenerator.js'; @@ -195,6 +199,13 @@ describe('buildToolGuidance', () => { expect(result).toContain('[--body-file ]'); expect(result).toContain('Read comment body from file (use - for stdin)'); }); + + it('renders ReplyToReviewComment body-file guidance from definition metadata', () => { + const result = buildToolGuidance([generateToolManifest(replyToReviewCommentDef)]); + + expect(result).toContain('[--body-file ]'); + expect(result).toContain('Read reply body from file (use - for stdin)'); + }); }); describe('formatParam — optional string param', () => { diff --git a/tests/unit/backends/toolManifests.test.ts b/tests/unit/backends/toolManifests.test.ts index 83079e23..473e4eb6 100644 --- a/tests/unit/backends/toolManifests.test.ts +++ b/tests/unit/backends/toolManifests.test.ts @@ -158,4 +158,17 @@ describe('getToolManifests', () => { 'body-file': { type: 'string' }, }); }); + + it('ReplyToReviewComment has body-file support', () => { + const manifests = getToolManifests(); + const replyToReviewComment = manifests.find((m) => m.name === 'ReplyToReviewComment'); + expect(replyToReviewComment).toBeDefined(); + expect(replyToReviewComment?.parameters).toMatchObject({ + body: { type: 'string', required: true }, + 'body-file': { + type: 'string', + description: 'Read reply body from file (use - for stdin)', + }, + }); + }); }); diff --git a/tests/unit/cli/cli-command-factory.test.ts b/tests/unit/cli/cli-command-factory.test.ts index a20aaa5f..d326f113 100644 --- a/tests/unit/cli/cli-command-factory.test.ts +++ b/tests/unit/cli/cli-command-factory.test.ts @@ -340,6 +340,97 @@ describe('cliCommandFactory — flag generation', () => { expect.objectContaining({ config: { key: 'value', num: 42 } }), ); }); + + it('passes canonical array-of-object JSON arrays unchanged', async () => { + const coreFn = vi.fn().mockResolvedValue('ok'); + const comments = [{ path: 'src/index.ts', line: 12, body: 'Please handle null here.' }]; + const def = makeToolDef({ + parameters: { + comments: { + type: 'array', + items: 'object', + describe: 'Inline comments', + optional: true, + cliAliases: ['comment'], + }, + }, + }); + const Cmd = createCLICommand(def, coreFn); + const cmd = new Cmd(['--comments', JSON.stringify(comments)], makeMockConfig() as never); + await cmd.run(); + + expect(coreFn).toHaveBeenCalledWith(expect.objectContaining({ comments })); + }); + + it('resolves singular array-of-object aliases with JSON arrays', async () => { + const coreFn = vi.fn().mockResolvedValue('ok'); + const comments = [{ path: 'src/index.ts', line: 12, body: 'Please handle null here.' }]; + const def = makeToolDef({ + parameters: { + comments: { + type: 'array', + items: 'object', + describe: 'Inline comments', + optional: true, + cliAliases: ['comment'], + }, + }, + }); + const Cmd = createCLICommand(def, coreFn); + const cmd = new Cmd(['--comment', JSON.stringify(comments)], makeMockConfig() as never); + await cmd.run(); + + expect(coreFn).toHaveBeenCalledWith(expect.objectContaining({ comments })); + }); + + it('normalizes singular array-of-object aliases with one JSON object', async () => { + const coreFn = vi.fn().mockResolvedValue('ok'); + const comment = { path: 'src/index.ts', line: 12, body: 'Please handle null here.' }; + const def = makeToolDef({ + parameters: { + comments: { + type: 'array', + items: 'object', + describe: 'Inline comments', + optional: true, + cliAliases: ['comment'], + }, + }, + }); + const Cmd = createCLICommand(def, coreFn); + const cmd = new Cmd(['--comment', JSON.stringify(comment)], makeMockConfig() as never); + await cmd.run(); + + expect(coreFn).toHaveBeenCalledWith(expect.objectContaining({ comments: [comment] })); + }); + + it('rejects primitive JSON for array-of-object params before calling the core function', async () => { + const coreFn = vi.fn().mockResolvedValue('ok'); + const def = makeToolDef({ + parameters: { + comments: { + type: 'array', + items: 'object', + describe: 'Inline comments', + optional: true, + cliAliases: ['comment'], + }, + }, + }); + const Cmd = createCLICommand(def, coreFn); + const cmd = new Cmd(['--comment', '"not an array"'], makeMockConfig() as never); + const logSpy = vi.spyOn(cmd, 'log'); + await expect(cmd.run()).rejects.toThrow(); + + expect(coreFn).not.toHaveBeenCalled(); + const output = JSON.parse(logSpy.mock.calls[0][0] as string) as { + success: boolean; + error: { type: string; flag?: string }; + }; + expect(output.success).toBe(false); + expect(output.error.type).toBe('json-parse'); + expect(output.error.flag).toBe('comments'); + }); }); // --------------------------------------------------------------------------- diff --git a/tests/unit/cli/file-input-flags.test.ts b/tests/unit/cli/file-input-flags.test.ts index ec111053..2acf1a95 100644 --- a/tests/unit/cli/file-input-flags.test.ts +++ b/tests/unit/cli/file-input-flags.test.ts @@ -50,6 +50,9 @@ vi.mock('../../../src/gadgets/github/core/postPRComment.js', () => ({ vi.mock('../../../src/gadgets/github/core/updatePRComment.js', () => ({ updatePRComment: vi.fn().mockResolvedValue({ id: 456 }), })); +vi.mock('../../../src/gadgets/github/core/replyToReviewComment.js', () => ({ + replyToReviewComment: vi.fn().mockResolvedValue('Reply posted successfully'), +})); import CreateWorkItem from '../../../src/cli/pm/create-work-item.js'; import PostComment from '../../../src/cli/pm/post-comment.js'; @@ -58,10 +61,12 @@ import UpdateWorkItem from '../../../src/cli/pm/update-work-item.js'; import CreatePR from '../../../src/cli/scm/create-pr.js'; import CreatePRReview from '../../../src/cli/scm/create-pr-review.js'; import PostPRComment from '../../../src/cli/scm/post-pr-comment.js'; +import ReplyToReviewComment from '../../../src/cli/scm/reply-to-review-comment.js'; import UpdatePRComment from '../../../src/cli/scm/update-pr-comment.js'; import { createPR } from '../../../src/gadgets/github/core/createPR.js'; import { createPRReview } from '../../../src/gadgets/github/core/createPRReview.js'; import { postPRComment } from '../../../src/gadgets/github/core/postPRComment.js'; +import { replyToReviewComment } from '../../../src/gadgets/github/core/replyToReviewComment.js'; import { updatePRComment } from '../../../src/gadgets/github/core/updatePRComment.js'; import { createWorkItem } from '../../../src/gadgets/pm/core/createWorkItem.js'; import { postComment } from '../../../src/gadgets/pm/core/postComment.js'; @@ -439,6 +444,58 @@ describe('CreatePRReview --body-file', () => { }), ); }); + + it('resolves the --comment alias with one JSON object', async () => { + const comment = { path: 'src/index.ts', line: 12, body: 'Please handle null here.' }; + const cmd = new CreatePRReview( + [ + '--prNumber', + '42', + '--event', + 'REQUEST_CHANGES', + '--body', + 'Needs a small change', + '--comment', + JSON.stringify(comment), + ], + mockConfig as never, + ); + await cmd.run(); + + expect(createPRReview).toHaveBeenCalledWith( + expect.objectContaining({ + body: 'Needs a small change', + comments: [comment], + }), + ); + }); + + it('rejects primitive JSON passed through the --comment alias', async () => { + const cmd = new CreatePRReview( + [ + '--prNumber', + '42', + '--event', + 'REQUEST_CHANGES', + '--body', + 'Needs a small change', + '--comment', + '"not an array"', + ], + mockConfig as never, + ); + const logSpy = vi.spyOn(cmd, 'log'); + await expect(cmd.run()).rejects.toThrow(); + + expect(createPRReview).not.toHaveBeenCalled(); + const output = JSON.parse(logSpy.mock.calls[0][0] as string) as { + success: boolean; + error: { type: string; flag?: string }; + }; + expect(output.success).toBe(false); + expect(output.error.type).toBe('json-parse'); + expect(output.error.flag).toBe('comments'); + }); }); describe('PostPRComment --body-file', () => { @@ -567,3 +624,73 @@ describe('UpdatePRComment --body-file', () => { expect(output.error.flag).toBe('body'); }); }); + +describe('ReplyToReviewComment --body-file', () => { + const originalEnv = process.env; + + beforeEach(() => { + process.env = { + ...originalEnv, + CASCADE_REPO_OWNER: 'owner', + CASCADE_REPO_NAME: 'repo', + }; + }); + + afterEach(() => { + process.env = originalEnv; + }); + + it('reads reply body from file', async () => { + const filePath = writeTempFile('reply.md', 'Review reply from file'); + const cmd = new ReplyToReviewComment( + ['--prNumber', '42', '--commentId', '789', '--body-file', filePath], + mockConfig as never, + ); + await cmd.run(); + + expect(replyToReviewComment).toHaveBeenCalledWith( + 'owner', + 'repo', + 42, + 789, + 'Review reply from file', + ); + }); + + it('prefers --body-file over --body', async () => { + const filePath = writeTempFile('reply.md', 'from file'); + const cmd = new ReplyToReviewComment( + ['--prNumber', '42', '--commentId', '789', '--body', 'from flag', '--body-file', filePath], + mockConfig as never, + ); + await cmd.run(); + + expect(replyToReviewComment).toHaveBeenCalledWith('owner', 'repo', 42, 789, 'from file'); + }); + + it('still works with inline --body flag', async () => { + const cmd = new ReplyToReviewComment( + ['--prNumber', '42', '--commentId', '789', '--body', 'inline body'], + mockConfig as never, + ); + await cmd.run(); + + expect(replyToReviewComment).toHaveBeenCalledWith('owner', 'repo', 42, 789, 'inline body'); + }); + + it('errors when neither --body nor --body-file is provided (spec 014 envelope)', async () => { + const cmd = new ReplyToReviewComment( + ['--prNumber', '42', '--commentId', '789'], + mockConfig as never, + ); + const logSpy = vi.spyOn(cmd, 'log'); + await expect(cmd.run()).rejects.toThrow(); + const output = JSON.parse(logSpy.mock.calls[0][0] as string) as { + success: boolean; + error: { type: string; flag?: string }; + }; + expect(output.success).toBe(false); + expect(output.error.type).toBe('missing-required'); + expect(output.error.flag).toBe('body'); + }); +}); diff --git a/tests/unit/gadgets/github/definitions.test.ts b/tests/unit/gadgets/github/definitions.test.ts index aef7f3df..ccf8a250 100644 --- a/tests/unit/gadgets/github/definitions.test.ts +++ b/tests/unit/gadgets/github/definitions.test.ts @@ -254,6 +254,15 @@ describe('GitHub SCM gadget definitions', () => { expect(replyToReviewCommentDef.parameters.commentId?.required).toBe(true); expect(replyToReviewCommentDef.parameters.body?.required).toBe(true); }); + + it('has body file input alternative', () => { + const bodyAlt = replyToReviewCommentDef.cli?.fileInputAlternatives?.find( + (a) => a.paramName === 'body', + ); + expect(bodyAlt).toBeDefined(); + expect(bodyAlt?.fileFlag).toBe('body-file'); + expect(bodyAlt?.description).toBeTruthy(); + }); }); // ─── UpdatePRComment specific ───────────────────────────────────────────── diff --git a/tests/unit/gadgets/shared/cli/params.test.ts b/tests/unit/gadgets/shared/cli/params.test.ts index 288e76a0..2bf71ea5 100644 --- a/tests/unit/gadgets/shared/cli/params.test.ts +++ b/tests/unit/gadgets/shared/cli/params.test.ts @@ -89,6 +89,78 @@ describe('CLI parameter resolution', () => { }); }); + it('wraps a top-level object for array-of-object file inputs', () => { + const filePath = writeTempFile('comments.json', '{"path":"from-file.ts"}'); + const params = resolveDirectParams( + def, + { + body: 'hello', + 'comments-file': filePath, + }, + new Map([['comments', fileAlt]]), + new Map([['owner', autoResolved]]), + makeSink(), + ); + + expect(params).toEqual({ + body: 'hello', + comments: [{ path: 'from-file.ts' }], + }); + }); + + it('wraps a top-level object for inline array-of-object values', () => { + const params = resolveDirectParams( + def, + { + body: 'hello', + comments: '{"path":"inline.ts"}', + }, + new Map([['comments', fileAlt]]), + new Map([['owner', autoResolved]]), + makeSink(), + ); + + expect(params).toEqual({ + body: 'hello', + comments: [{ path: 'inline.ts' }], + }); + }); + + it('keeps arrays unchanged without validating individual entries', () => { + const params = resolveDirectParams( + def, + { + body: 'hello', + comments: '["AddChecklist-compatible string entry",{"path":"inline.ts"}]', + }, + new Map([['comments', fileAlt]]), + new Map([['owner', autoResolved]]), + makeSink(), + ); + + expect(params).toEqual({ + body: 'hello', + comments: ['AddChecklist-compatible string entry', { path: 'inline.ts' }], + }); + }); + + it('emits json-parse when array-of-object JSON has an impossible top-level shape', () => { + const sink = makeSink(); + expect(() => + resolveDirectParams( + def, + { + body: 'hello', + comments: '"not an array"', + }, + new Map([['comments', fileAlt]]), + new Map([['owner', autoResolved]]), + sink, + ), + ).toThrow('exit'); + expect(sink.exit).toHaveBeenCalledWith(1); + }); + it('emits missing-required when neither inline nor file value is present', () => { expect(() => resolveDirectParams( diff --git a/tests/unit/pm/linear/adapter.test.ts b/tests/unit/pm/linear/adapter.test.ts index 7bae68fd..e6cf2eb8 100644 --- a/tests/unit/pm/linear/adapter.test.ts +++ b/tests/unit/pm/linear/adapter.test.ts @@ -36,7 +36,10 @@ vi.mock('../../../../src/linear/client.js', () => ({ }, })); -import { LinearPMProvider } from '../../../../src/pm/linear/adapter.js'; +import { + __resetRecentDescriptionsForTests, + LinearPMProvider, +} from '../../../../src/pm/linear/adapter.js'; // --------------------------------------------------------------------------- // Helpers @@ -100,6 +103,7 @@ describe('LinearPMProvider', () => { beforeEach(() => { provider = new LinearPMProvider(defaultConfig); vi.clearAllMocks(); + __resetRecentDescriptionsForTests(); }); it('has type "linear"', () => { @@ -559,7 +563,13 @@ describe('LinearPMProvider', () => { expect(result.items[0].id).toMatch(/^cl-[0-9a-f]{8}$/); }); - it('waits for Linear read-after-write visibility before the next checklist append', async () => { + // 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('uses recent-description cache instead of polling Linear after a PUT', async () => { let description = 'Existing.'; let staleDescription: string | null = null; mockGetIssue.mockImplementation(async () => { @@ -582,6 +592,42 @@ 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. + 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 }), + ); + mockUpdateIssue.mockImplementation(async (_id, _updates: { description?: string }) => + makeIssue({ description: persistedDescription }), + ); + + 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'); + }); + it('preserves multiple concurrently-created checklist sections despite stale Linear reads', async () => { let description = 'Existing.'; let staleDescription: string | null = null;