From a035b3c308194e0d9953d5cf742d352e83cbbe08 Mon Sep 17 00:00:00 2001 From: Sven Date: Wed, 25 Mar 2026 13:49:37 +0100 Subject: [PATCH] fix: enforce spec compliance in GitHub module (projects, discussions, comments) - projects.ts: replace console.warn with proper error return on failed Status field mutation, ensuring callers see ok:false instead of silent continuation - discussions.ts: add cursor-based auto-pagination to listDiscussions so all pages are fetched instead of only the first - comments.ts: add runtime validation of comment type field against the known set, rejecting unrecognized types with null instead of blindly casting via `as` - types.ts: extend MaxsimCommentMeta type union with phase-complete and checkpoint variants - Update all corresponding unit tests Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/cli/src/github/comments.ts | 6 +- packages/cli/src/github/discussions.ts | 70 ++++++++++++++----- packages/cli/src/github/projects.ts | 9 +-- packages/cli/src/github/types.ts | 2 +- packages/cli/tests/unit/comments.test.ts | 4 ++ .../cli/tests/unit/github-discussions.test.ts | 28 +++++++- .../cli/tests/unit/github-projects.test.ts | 55 +++++++++++++++ 7 files changed, 148 insertions(+), 26 deletions(-) diff --git a/packages/cli/src/github/comments.ts b/packages/cli/src/github/comments.ts index f75e8aba..750160dc 100644 --- a/packages/cli/src/github/comments.ts +++ b/packages/cli/src/github/comments.ts @@ -74,12 +74,16 @@ export function parseIssueState(body: string): MaxsimIssueState | null { }; } +const VALID_COMMENT_TYPES = new Set(['plan', 'research', 'context', 'progress', 'verification', 'summary', 'error', 'escalation', 'handoff', 'user-intent', 'phase-complete', 'checkpoint'] as const); + /** Extract comment metadata from the first line HTML comment. */ export function parseCommentMeta(body: string): MaxsimCommentMeta | null { const match = body.match(COMMENT_META_REGEX); if (!match) return null; - const type = match[1] as MaxsimCommentMeta['type']; + const type = match[1]; + if (!(VALID_COMMENT_TYPES as ReadonlySet).has(type)) return null; + const attrs: Record = { type }; // Parse space-separated key=value pairs diff --git a/packages/cli/src/github/discussions.ts b/packages/cli/src/github/discussions.ts index 5ced5d60..c4b6a67a 100644 --- a/packages/cli/src/github/discussions.ts +++ b/packages/cli/src/github/discussions.ts @@ -162,10 +162,19 @@ export function listDiscussions( } const hasCategory = categoryId !== undefined; - const query = ` - query($owner: String!, $repo: String!, $first: Int!${hasCategory ? ', $categoryId: ID!' : ''}) { + + const allNodes: RawDiscussion[] = []; + let hasNextPage = true; + let endCursor: string | null = null; + + while (hasNextPage) { + const afterClause = endCursor ? `, after: $after` : ''; + const afterVar = endCursor ? ', $after: String!' : ''; + + const query = ` + query($owner: String!, $repo: String!, $first: Int!${hasCategory ? ', $categoryId: ID!' : ''}${afterVar}) { repository(owner: $owner, name: $repo) { - discussions(first: $first${hasCategory ? ', categoryId: $categoryId' : ''}) { + discussions(first: $first${hasCategory ? ', categoryId: $categoryId' : ''}${afterClause}) { nodes { number id @@ -176,31 +185,54 @@ export function listDiscussions( createdAt updatedAt } + pageInfo { + hasNextPage + endCursor + } } } } `.trim(); - const args = [ - 'api', 'graphql', - '-f', `query=${query}`, - '-f', `owner=${owner}`, - '-f', `repo=${repoName}`, - '-F', `first=${first}`, - ]; + const args = [ + 'api', 'graphql', + '-f', `query=${query}`, + '-f', `owner=${owner}`, + '-f', `repo=${repoName}`, + '-F', `first=${first}`, + ]; - if (hasCategory) { - args.push('-f', `categoryId=${categoryId}`); - } + if (hasCategory) { + args.push('-f', `categoryId=${categoryId}`); + } - const result = ghJson<{ - data: { repository: { discussions: { nodes: RawDiscussion[] } } }; - }>(args); + if (endCursor) { + args.push('-f', `after=${endCursor}`); + } - if (!result.ok) return result; + const result = ghJson<{ + data: { + repository: { + discussions: { + nodes: RawDiscussion[]; + pageInfo: { hasNextPage: boolean; endCursor: string | null }; + }; + }; + }; + }>(args); + + if (!result.ok) return result; + + const discussions = result.data?.data?.repository?.discussions; + const nodes = discussions?.nodes ?? []; + allNodes.push(...nodes); + + const pageInfo = discussions?.pageInfo; + hasNextPage = pageInfo?.hasNextPage ?? false; + endCursor = pageInfo?.endCursor ?? null; + } - const nodes = result.data?.data?.repository?.discussions?.nodes ?? []; - return { ok: true, data: nodes.map(mapDiscussion) }; + return { ok: true, data: allNodes.map(mapDiscussion) }; } export function getDiscussion( diff --git a/packages/cli/src/github/projects.ts b/packages/cli/src/github/projects.ts index 03170e63..09776197 100644 --- a/packages/cli/src/github/projects.ts +++ b/packages/cli/src/github/projects.ts @@ -257,10 +257,11 @@ export async function ensureProjectBoard( ]); if (!updateResult.ok) { - console.warn( - `Warning: Failed to add missing Status options (${missingColumns.map((c) => c.name).join(', ')}): ${updateResult.error}. ` + - 'You may need to add them manually via the GitHub Projects UI.', - ); + return { + ok: false, + error: `Failed to add missing Status options (${missingColumns.map((c) => c.name).join(', ')}): ${updateResult.error}`, + code: 'UNKNOWN', + }; } } diff --git a/packages/cli/src/github/types.ts b/packages/cli/src/github/types.ts index 6401bbc1..5e2602d9 100644 --- a/packages/cli/src/github/types.ts +++ b/packages/cli/src/github/types.ts @@ -133,7 +133,7 @@ export interface MaxsimIssueState { /** Structured comment metadata from HTML comment markers. */ export interface MaxsimCommentMeta { - type: 'plan' | 'research' | 'context' | 'progress' | 'verification' | 'summary' | 'error' | 'escalation' | 'handoff' | 'user-intent'; + type: 'plan' | 'research' | 'context' | 'progress' | 'verification' | 'summary' | 'error' | 'escalation' | 'handoff' | 'user-intent' | 'phase-complete' | 'checkpoint'; phase?: number; task?: number; [key: string]: unknown; diff --git a/packages/cli/tests/unit/comments.test.ts b/packages/cli/tests/unit/comments.test.ts index dbf33b2d..47323501 100644 --- a/packages/cli/tests/unit/comments.test.ts +++ b/packages/cli/tests/unit/comments.test.ts @@ -120,6 +120,10 @@ describe('parseCommentMeta', () => { it('returns null for non-maxsim comments', () => { expect(parseCommentMeta('Just a normal comment')).toBeNull(); }); + + it('returns null for invalid comment types', () => { + expect(parseCommentMeta('')).toBeNull(); + }); }); describe('formatIssueMeta', () => { diff --git a/packages/cli/tests/unit/github-discussions.test.ts b/packages/cli/tests/unit/github-discussions.test.ts index bdb04a9e..6c4e14a8 100644 --- a/packages/cli/tests/unit/github-discussions.test.ts +++ b/packages/cli/tests/unit/github-discussions.test.ts @@ -119,12 +119,16 @@ function createDiscussionResponse(discussion: typeof MOCK_DISCUSSION_RAW = MOCK_ } /** Build the GraphQL response envelope for a discussions list query. */ -function listDiscussionsResponse(discussions: typeof MOCK_DISCUSSION_RAW[] = [MOCK_DISCUSSION_RAW]): string { +function listDiscussionsResponse( + discussions: typeof MOCK_DISCUSSION_RAW[] = [MOCK_DISCUSSION_RAW], + pageInfo: { hasNextPage: boolean; endCursor: string | null } = { hasNextPage: false, endCursor: null }, +): string { return JSON.stringify({ data: { repository: { discussions: { nodes: discussions, + pageInfo, }, }, }, @@ -322,6 +326,28 @@ describe('listDiscussions', () => { const ghArgs = graphqlCall?.[1] as string[]; expect(ghArgs).toContain('owner=customowner'); }); + + it('paginates through multiple pages and returns all discussions', () => { + const disc1 = { ...MOCK_DISCUSSION_RAW, number: 1, id: 'D_1', title: 'Discussion 1' }; + const disc2 = { ...MOCK_DISCUSSION_RAW, number: 2, id: 'D_2', title: 'Discussion 2' }; + const disc3 = { ...MOCK_DISCUSSION_RAW, number: 3, id: 'D_3', title: 'Discussion 3' }; + + // First page: 2 discussions with hasNextPage=true + // Second page: 1 discussion with hasNextPage=false + setupExecMock( + listDiscussionsResponse([disc1, disc2], { hasNextPage: true, endCursor: 'cursor_abc' }), + listDiscussionsResponse([disc3], { hasNextPage: false, endCursor: null }), + ); + + const result = listDiscussions(); + + expect(result.ok).toBe(true); + if (!result.ok) return; + expect(result.data).toHaveLength(3); + expect(result.data[0].number).toBe(1); + expect(result.data[1].number).toBe(2); + expect(result.data[2].number).toBe(3); + }); }); // ── getDiscussion ─────────────────────────────────────────────────────────── diff --git a/packages/cli/tests/unit/github-projects.test.ts b/packages/cli/tests/unit/github-projects.test.ts index 1faa67f5..358f1afe 100644 --- a/packages/cli/tests/unit/github-projects.test.ts +++ b/packages/cli/tests/unit/github-projects.test.ts @@ -656,6 +656,61 @@ describe('ensureProjectBoard', () => { expect(graphqlCalls.length).toBe(1); }); + it('returns ok: false when GraphQL mutation to add missing Status options fails', async () => { + // Status field exists but is missing columns, and the GraphQL update fails. + // We need a custom mock because setupExecMock catches all `gh api` calls + // as getRepoInfo type-detection. Here we differentiate `gh api graphql` + // (the mutation) from `gh api /users/...` (the type check). + const partialFields = { + fields: [ + { + id: 'PVTSSF_status', + name: 'Status', + type: 'SINGLE_SELECT', + options: [ + { id: 'opt_backlog', name: 'Backlog', color: 'GRAY' }, + ], + }, + ], + }; + const projectList = { projects: [MOCK_PROJECT] }; + const ghResponses = [ + JSON.stringify(projectList), + JSON.stringify(partialFields), + ]; + let callIndex = 0; + execFileSyncMock.mockImplementation((_cmd: string, args?: readonly string[]) => { + const argList = args ?? []; + + // git calls used by getRepoInfo + if (_cmd === 'git') { + if (argList[0] === 'remote') return REPO_REMOTE_URL; + } + + // gh api /users/* call used to detect org/user type + if (_cmd === 'gh' && argList[0] === 'api' && String(argList[1]).startsWith('/users/')) { + return 'User'; + } + + // gh api graphql — the mutation call should fail + if (_cmd === 'gh' && argList[0] === 'api' && argList[1] === 'graphql') { + throw new Error('gh: GraphQL mutation failed'); + } + + // All other gh calls — dispatch from the queue + const response = ghResponses[callIndex++]; + if (response instanceof Error) throw response; + return response ?? ''; + }); + + const result = await ensureProjectBoard('My Board', 'testorg'); + + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.code).toBe('UNKNOWN'); + expect(result.error).toContain('Failed to add missing Status options'); + }); + it('propagates error from findProject', async () => { setupExecMock(new Error('gh: 401 Unauthorized'));