Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/cli/src/github/comments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VALID_COMMENT_TYPES duplicates the MaxsimCommentMeta['type'] union in types.ts, which can drift over time. Consider defining a single exported const array of comment types and deriving both the union type and the runtime Set/type-guard from it to keep them in sync.

Copilot uses AI. Check for mistakes.

/** 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<string>).has(type)) return null;

const attrs: Record<string, unknown> = { type };

// Parse space-separated key=value pairs
Expand Down
70 changes: 51 additions & 19 deletions packages/cli/src/github/discussions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Comment on lines +231 to +232
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pagination loop can become infinite if GitHub returns hasNextPage: true but endCursor is null/empty (then after is never sent and the same page is requested repeatedly). Add a guard that returns an error (or breaks) when hasNextPage is true but endCursor is missing.

Suggested change
hasNextPage = pageInfo?.hasNextPage ?? false;
endCursor = pageInfo?.endCursor ?? null;
const nextHasNextPage = pageInfo?.hasNextPage ?? false;
const nextEndCursor = pageInfo?.endCursor ?? null;
// Guard against a potential infinite loop if GitHub reports hasNextPage
// but does not provide an endCursor for the next page.
if (nextHasNextPage && !nextEndCursor) {
break;
}
hasNextPage = nextHasNextPage;
endCursor = nextEndCursor;

Copilot uses AI. Check for mistakes.
}

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(
Expand Down
9 changes: 5 additions & 4 deletions packages/cli/src/github/projects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensureProjectBoard hard-codes code: 'UNKNOWN' when the GraphQL update fails, which loses the more specific updateResult.code classification from ghExec (e.g., FORBIDDEN/RATE_LIMITED). Propagate updateResult.code instead so callers can handle failures consistently.

Suggested change
code: 'UNKNOWN',
code: updateResult.code ?? 'UNKNOWN',

Copilot uses AI. Check for mistakes.
};
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/github/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 4 additions & 0 deletions packages/cli/tests/unit/comments.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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('<!-- maxsim:type=INVALID_TYPE -->')).toBeNull();
});
});

describe('formatIssueMeta', () => {
Expand Down
28 changes: 27 additions & 1 deletion packages/cli/tests/unit/github-discussions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
},
},
Expand Down Expand Up @@ -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 ───────────────────────────────────────────────────────────
Expand Down
55 changes: 55 additions & 0 deletions packages/cli/tests/unit/github-projects.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'));

Expand Down
Loading