mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-07-09 17:28:58 +00:00
fix(actions): preserve failed inline review comments (#81)
This commit is contained in:
zhouzhihao
GitHub
parent
7fcb9db6a3
commit
c323c6b40c
4 changed files with 283 additions and 59 deletions
84
.github/workflows/ocr-review.yml
vendored
84
.github/workflows/ocr-review.yml
vendored
|
|
@ -136,7 +136,7 @@ jobs:
|
|||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: context.issue.number,
|
||||
body: `⚠️ **OpenCodeReview** encountered an error:\n\`\`\`\n${stderr}\n\`\`\``
|
||||
body: `⚠️ **OpenCodeReview** encountered an error:\n${fencedBlock(stderr)}`
|
||||
});
|
||||
}
|
||||
return;
|
||||
|
|
@ -208,27 +208,17 @@ jobs:
|
|||
reviewComment.side = 'RIGHT';
|
||||
}
|
||||
|
||||
reviewComments.push(reviewComment);
|
||||
reviewComments.push({ comment, reviewComment });
|
||||
}
|
||||
|
||||
// Submit as a single PR review with all comments
|
||||
const totalCount = comments.length;
|
||||
const inlineCount = reviewComments.length;
|
||||
const summaryCount = commentsWithoutLine.length;
|
||||
let summaryBody = `🔍 **OpenCodeReview** found **${totalCount}** issue(s) in this PR.`;
|
||||
if (totalCount > 0) {
|
||||
summaryBody += `\n- ✅ ${inlineCount} posted as inline comment(s)`;
|
||||
summaryBody += `\n- 📝 ${summaryCount} posted as summary (missing line info)`;
|
||||
}
|
||||
if (warnings.length > 0) {
|
||||
summaryBody += `\n\n⚠️ ${warnings.length} warning(s) occurred during review.`;
|
||||
}
|
||||
let summaryBody = buildSummaryBody(totalCount, inlineCount, summaryCount, warnings);
|
||||
|
||||
// Add comments without line info to summary body
|
||||
for (const { comment, body } of commentsWithoutLine) {
|
||||
summaryBody += '\n\n---\n\n';
|
||||
summaryBody += formatCommentMarkdown(comment);
|
||||
}
|
||||
summaryBody += formatSummaryComments(commentsWithoutLine);
|
||||
|
||||
// Statistics tracking
|
||||
let successCount = 0;
|
||||
|
|
@ -243,7 +233,7 @@ jobs:
|
|||
commit_id: commitSha,
|
||||
body: summaryBody,
|
||||
event: 'COMMENT',
|
||||
comments: reviewComments
|
||||
comments: reviewComments.map(({ reviewComment }) => reviewComment)
|
||||
});
|
||||
successCount = reviewComments.length;
|
||||
console.log(`Successfully posted review with ${successCount} inline comments (${commentsWithoutLine.length} in summary)`);
|
||||
|
|
@ -252,7 +242,7 @@ jobs:
|
|||
console.log('Falling back to posting comments individually...');
|
||||
|
||||
// Fallback: post comments one by one
|
||||
for (const reviewComment of reviewComments) {
|
||||
for (const { comment, reviewComment } of reviewComments) {
|
||||
try {
|
||||
await github.rest.pulls.createReview({
|
||||
owner: context.repo.owner,
|
||||
|
|
@ -267,26 +257,27 @@ jobs:
|
|||
console.log(`Successfully posted comment for ${reviewComment.path}`);
|
||||
} catch (innerE) {
|
||||
failedCount++;
|
||||
failedComments.push({ comment: reviewComment, error: innerE.message });
|
||||
failedComments.push({ comment, error: innerE.message });
|
||||
console.log(`Failed to post comment for ${reviewComment.path}: ${innerE.message}`);
|
||||
}
|
||||
}
|
||||
|
||||
// Post summary comment with statistics
|
||||
let finalBody = summaryBody;
|
||||
let finalBody = buildSummaryBody(totalCount, successCount, commentsWithoutLine.length + failedComments.length, warnings);
|
||||
finalBody += formatSummaryComments(commentsWithoutLine);
|
||||
finalBody += `\n\n---\n\n📊 **Posting Statistics:**`;
|
||||
finalBody += `\n- ✅ Successfully posted: ${successCount} comment(s)`;
|
||||
if (failedCount > 0) {
|
||||
finalBody += `\n- ❌ Failed to post: ${failedCount} comment(s)`;
|
||||
}
|
||||
|
||||
// Add failed comments details
|
||||
// Add failed comments as summary content so review feedback is not lost.
|
||||
if (failedComments.length > 0) {
|
||||
finalBody += '\n\n<details><summary>❌ Failed Comments Details</summary>\n\n';
|
||||
finalBody += '\n\n---\n\n### ⚠️ Inline comments shown in summary';
|
||||
for (const { comment, error } of failedComments) {
|
||||
finalBody += `- \`${comment.path}\`: ${error}\n`;
|
||||
finalBody += '\n\n---\n\n';
|
||||
finalBody += formatCommentMarkdown(comment, error);
|
||||
}
|
||||
finalBody += '\n</details>';
|
||||
}
|
||||
|
||||
await github.rest.issues.createComment({
|
||||
|
|
@ -303,29 +294,64 @@ jobs:
|
|||
// Add code suggestion if available
|
||||
if (comment.suggestion_code && comment.existing_code) {
|
||||
body += '\n\n**Suggestion:**\n';
|
||||
body += '```suggestion\n';
|
||||
body += comment.suggestion_code;
|
||||
if (!comment.suggestion_code.endsWith('\n')) body += '\n';
|
||||
body += '```';
|
||||
body += fencedBlock(comment.suggestion_code, 'suggestion');
|
||||
}
|
||||
|
||||
return body;
|
||||
}
|
||||
|
||||
function formatCommentMarkdown(comment) {
|
||||
function formatCommentMarkdown(comment, error) {
|
||||
let md = `### 📄 \`${comment.path}\``;
|
||||
if (comment.start_line && comment.end_line) {
|
||||
md += ` (L${comment.start_line}-L${comment.end_line})`;
|
||||
}
|
||||
md += '\n\n';
|
||||
if (error) {
|
||||
md += `⚠️ GitHub could not post this as an inline comment: ${error}\n\n`;
|
||||
}
|
||||
md += comment.content || '';
|
||||
|
||||
if (comment.suggestion_code && comment.existing_code) {
|
||||
md += '\n\n<details><summary>💡 Suggested Change</summary>\n\n';
|
||||
md += '**Before:**\n```\n' + comment.existing_code + '\n```\n\n';
|
||||
md += '**After:**\n```\n' + comment.suggestion_code + '\n```\n\n';
|
||||
md += '**Before:**\n' + fencedBlock(comment.existing_code) + '\n\n';
|
||||
md += '**After:**\n' + fencedBlock(comment.suggestion_code) + '\n\n';
|
||||
md += '</details>';
|
||||
}
|
||||
|
||||
return md;
|
||||
}
|
||||
|
||||
function buildSummaryBody(totalCount, inlineCount, summaryCount, warnings) {
|
||||
let body = `🔍 **OpenCodeReview** found **${totalCount}** issue(s) in this PR.`;
|
||||
if (totalCount > 0) {
|
||||
body += `\n- ✅ ${inlineCount} posted as inline comment(s)`;
|
||||
body += `\n- 📝 ${summaryCount} posted as summary`;
|
||||
}
|
||||
if (warnings.length > 0) {
|
||||
body += `\n\n⚠️ ${warnings.length} warning(s) occurred during review.`;
|
||||
}
|
||||
return body;
|
||||
}
|
||||
|
||||
function formatSummaryComments(summaryComments) {
|
||||
let body = '';
|
||||
for (const { comment } of summaryComments) {
|
||||
body += '\n\n---\n\n';
|
||||
body += formatCommentMarkdown(comment);
|
||||
}
|
||||
return body;
|
||||
}
|
||||
|
||||
function fencedBlock(content, language = '') {
|
||||
const text = String(content || '');
|
||||
const fence = safeFence(text);
|
||||
let block = fence + language + '\n' + text;
|
||||
if (!text.endsWith('\n')) block += '\n';
|
||||
return block + fence;
|
||||
}
|
||||
|
||||
function safeFence(content) {
|
||||
const matches = String(content || '').match(/`+/g) || [];
|
||||
const maxTicks = matches.reduce((max, ticks) => Math.max(max, ticks.length), 0);
|
||||
return '`'.repeat(Math.max(3, maxTicks + 1));
|
||||
}
|
||||
|
|
|
|||
|
|
@ -136,7 +136,7 @@ jobs:
|
|||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: context.issue.number,
|
||||
body: `⚠️ **OpenCodeReview** encountered an error:\n\`\`\`\n${stderr}\n\`\`\``
|
||||
body: `⚠️ **OpenCodeReview** encountered an error:\n${fencedBlock(stderr)}`
|
||||
});
|
||||
}
|
||||
return;
|
||||
|
|
@ -208,27 +208,17 @@ jobs:
|
|||
reviewComment.side = 'RIGHT';
|
||||
}
|
||||
|
||||
reviewComments.push(reviewComment);
|
||||
reviewComments.push({ comment, reviewComment });
|
||||
}
|
||||
|
||||
// Submit as a single PR review with all comments
|
||||
const totalCount = comments.length;
|
||||
const inlineCount = reviewComments.length;
|
||||
const summaryCount = commentsWithoutLine.length;
|
||||
let summaryBody = `🔍 **OpenCodeReview** found **${totalCount}** issue(s) in this PR.`;
|
||||
if (totalCount > 0) {
|
||||
summaryBody += `\n- ✅ ${inlineCount} posted as inline comment(s)`;
|
||||
summaryBody += `\n- 📝 ${summaryCount} posted as summary (missing line info)`;
|
||||
}
|
||||
if (warnings.length > 0) {
|
||||
summaryBody += `\n\n⚠️ ${warnings.length} warning(s) occurred during review.`;
|
||||
}
|
||||
let summaryBody = buildSummaryBody(totalCount, inlineCount, summaryCount, warnings);
|
||||
|
||||
// Add comments without line info to summary body
|
||||
for (const { comment, body } of commentsWithoutLine) {
|
||||
summaryBody += '\n\n---\n\n';
|
||||
summaryBody += formatCommentMarkdown(comment);
|
||||
}
|
||||
summaryBody += formatSummaryComments(commentsWithoutLine);
|
||||
|
||||
// Statistics tracking
|
||||
let successCount = 0;
|
||||
|
|
@ -243,7 +233,7 @@ jobs:
|
|||
commit_id: commitSha,
|
||||
body: summaryBody,
|
||||
event: 'COMMENT',
|
||||
comments: reviewComments
|
||||
comments: reviewComments.map(({ reviewComment }) => reviewComment)
|
||||
});
|
||||
successCount = reviewComments.length;
|
||||
console.log(`Successfully posted review with ${successCount} inline comments (${commentsWithoutLine.length} in summary)`);
|
||||
|
|
@ -252,7 +242,7 @@ jobs:
|
|||
console.log('Falling back to posting comments individually...');
|
||||
|
||||
// Fallback: post comments one by one
|
||||
for (const reviewComment of reviewComments) {
|
||||
for (const { comment, reviewComment } of reviewComments) {
|
||||
try {
|
||||
await github.rest.pulls.createReview({
|
||||
owner: context.repo.owner,
|
||||
|
|
@ -267,26 +257,27 @@ jobs:
|
|||
console.log(`Successfully posted comment for ${reviewComment.path}`);
|
||||
} catch (innerE) {
|
||||
failedCount++;
|
||||
failedComments.push({ comment: reviewComment, error: innerE.message });
|
||||
failedComments.push({ comment, error: innerE.message });
|
||||
console.log(`Failed to post comment for ${reviewComment.path}: ${innerE.message}`);
|
||||
}
|
||||
}
|
||||
|
||||
// Post summary comment with statistics
|
||||
let finalBody = summaryBody;
|
||||
let finalBody = buildSummaryBody(totalCount, successCount, commentsWithoutLine.length + failedComments.length, warnings);
|
||||
finalBody += formatSummaryComments(commentsWithoutLine);
|
||||
finalBody += `\n\n---\n\n📊 **Posting Statistics:**`;
|
||||
finalBody += `\n- ✅ Successfully posted: ${successCount} comment(s)`;
|
||||
if (failedCount > 0) {
|
||||
finalBody += `\n- ❌ Failed to post: ${failedCount} comment(s)`;
|
||||
}
|
||||
|
||||
// Add failed comments details
|
||||
// Add failed comments as summary content so review feedback is not lost.
|
||||
if (failedComments.length > 0) {
|
||||
finalBody += '\n\n<details><summary>❌ Failed Comments Details</summary>\n\n';
|
||||
finalBody += '\n\n---\n\n### ⚠️ Inline comments shown in summary';
|
||||
for (const { comment, error } of failedComments) {
|
||||
finalBody += `- \`${comment.path}\`: ${error}\n`;
|
||||
finalBody += '\n\n---\n\n';
|
||||
finalBody += formatCommentMarkdown(comment, error);
|
||||
}
|
||||
finalBody += '\n</details>';
|
||||
}
|
||||
|
||||
await github.rest.issues.createComment({
|
||||
|
|
@ -303,29 +294,64 @@ jobs:
|
|||
// Add code suggestion if available
|
||||
if (comment.suggestion_code && comment.existing_code) {
|
||||
body += '\n\n**Suggestion:**\n';
|
||||
body += '```suggestion\n';
|
||||
body += comment.suggestion_code;
|
||||
if (!comment.suggestion_code.endsWith('\n')) body += '\n';
|
||||
body += '```';
|
||||
body += fencedBlock(comment.suggestion_code, 'suggestion');
|
||||
}
|
||||
|
||||
return body;
|
||||
}
|
||||
|
||||
function formatCommentMarkdown(comment) {
|
||||
function formatCommentMarkdown(comment, error) {
|
||||
let md = `### 📄 \`${comment.path}\``;
|
||||
if (comment.start_line && comment.end_line) {
|
||||
md += ` (L${comment.start_line}-L${comment.end_line})`;
|
||||
}
|
||||
md += '\n\n';
|
||||
if (error) {
|
||||
md += `⚠️ GitHub could not post this as an inline comment: ${error}\n\n`;
|
||||
}
|
||||
md += comment.content || '';
|
||||
|
||||
if (comment.suggestion_code && comment.existing_code) {
|
||||
md += '\n\n<details><summary>💡 Suggested Change</summary>\n\n';
|
||||
md += '**Before:**\n```\n' + comment.existing_code + '\n```\n\n';
|
||||
md += '**After:**\n```\n' + comment.suggestion_code + '\n```\n\n';
|
||||
md += '**Before:**\n' + fencedBlock(comment.existing_code) + '\n\n';
|
||||
md += '**After:**\n' + fencedBlock(comment.suggestion_code) + '\n\n';
|
||||
md += '</details>';
|
||||
}
|
||||
|
||||
return md;
|
||||
}
|
||||
|
||||
function buildSummaryBody(totalCount, inlineCount, summaryCount, warnings) {
|
||||
let body = `🔍 **OpenCodeReview** found **${totalCount}** issue(s) in this PR.`;
|
||||
if (totalCount > 0) {
|
||||
body += `\n- ✅ ${inlineCount} posted as inline comment(s)`;
|
||||
body += `\n- 📝 ${summaryCount} posted as summary`;
|
||||
}
|
||||
if (warnings.length > 0) {
|
||||
body += `\n\n⚠️ ${warnings.length} warning(s) occurred during review.`;
|
||||
}
|
||||
return body;
|
||||
}
|
||||
|
||||
function formatSummaryComments(summaryComments) {
|
||||
let body = '';
|
||||
for (const { comment } of summaryComments) {
|
||||
body += '\n\n---\n\n';
|
||||
body += formatCommentMarkdown(comment);
|
||||
}
|
||||
return body;
|
||||
}
|
||||
|
||||
function fencedBlock(content, language = '') {
|
||||
const text = String(content || '');
|
||||
const fence = safeFence(text);
|
||||
let block = fence + language + '\n' + text;
|
||||
if (!text.endsWith('\n')) block += '\n';
|
||||
return block + fence;
|
||||
}
|
||||
|
||||
function safeFence(content) {
|
||||
const matches = String(content || '').match(/`+/g) || [];
|
||||
const maxTicks = matches.reduce((max, ticks) => Math.max(max, ticks.length), 0);
|
||||
return '`'.repeat(Math.max(3, maxTicks + 1));
|
||||
}
|
||||
|
|
|
|||
|
|
@ -6,7 +6,8 @@
|
|||
"ocr": "bin/ocr.js"
|
||||
},
|
||||
"scripts": {
|
||||
"postinstall": "node scripts/install.js"
|
||||
"postinstall": "node scripts/install.js",
|
||||
"test:github-actions": "node scripts/github-actions/post-review-comments.test.js"
|
||||
},
|
||||
"repository": {
|
||||
"type": "git",
|
||||
|
|
|
|||
171
scripts/github-actions/post-review-comments.test.js
Normal file
171
scripts/github-actions/post-review-comments.test.js
Normal file
|
|
@ -0,0 +1,171 @@
|
|||
#!/usr/bin/env node
|
||||
"use strict";
|
||||
|
||||
const assert = require("assert");
|
||||
const fs = require("fs");
|
||||
const path = require("path");
|
||||
const vm = require("vm");
|
||||
|
||||
const repoRoot = path.join(__dirname, "..", "..");
|
||||
const workflowFiles = [
|
||||
".github/workflows/ocr-review.yml",
|
||||
"examples/github_actions/ocr-review.yml",
|
||||
];
|
||||
|
||||
function extractPostReviewScript(workflowPath) {
|
||||
const text = fs.readFileSync(path.join(repoRoot, workflowPath), "utf8");
|
||||
const lines = text.split("\n");
|
||||
|
||||
for (let i = 0; i < lines.length; i++) {
|
||||
const line = lines[i];
|
||||
const marker = line.match(/^(\s*)script:\s*\|\s*$/);
|
||||
if (!marker) continue;
|
||||
|
||||
const blockIndent = marker[1].length + 2;
|
||||
const block = [];
|
||||
for (let j = i + 1; j < lines.length; j++) {
|
||||
const current = lines[j];
|
||||
if (current.trim() === "") {
|
||||
block.push("");
|
||||
continue;
|
||||
}
|
||||
const indent = current.match(/^ */)[0].length;
|
||||
if (indent < blockIndent) break;
|
||||
block.push(current.slice(blockIndent));
|
||||
}
|
||||
|
||||
const script = block.join("\n");
|
||||
if (script.includes("/tmp/ocr-result.json")) {
|
||||
return script;
|
||||
}
|
||||
}
|
||||
|
||||
throw new Error(`post review script not found in ${workflowPath}`);
|
||||
}
|
||||
|
||||
function mockFs(resultText, stderrText) {
|
||||
return {
|
||||
readFileSync(file) {
|
||||
if (file === "/tmp/ocr-result.json") return resultText;
|
||||
if (file === "/tmp/ocr-stderr.log") return stderrText;
|
||||
throw new Error(`unexpected read: ${file}`);
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
function mockGithub(options) {
|
||||
const createReviewCalls = [];
|
||||
const issueComments = [];
|
||||
|
||||
return {
|
||||
createReviewCalls,
|
||||
issueComments,
|
||||
rest: {
|
||||
pulls: {
|
||||
get: async () => ({ data: { head: { sha: "head-sha" } } }),
|
||||
createReview: async (params) => {
|
||||
createReviewCalls.push(params);
|
||||
if (createReviewCalls.length === 1 && options.bulkError) {
|
||||
throw new Error(options.bulkError);
|
||||
}
|
||||
if (createReviewCalls.length > 1 && options.individualError) {
|
||||
throw new Error(options.individualError);
|
||||
}
|
||||
return { data: {} };
|
||||
},
|
||||
},
|
||||
issues: {
|
||||
createComment: async (params) => {
|
||||
issueComments.push(params);
|
||||
return { data: {} };
|
||||
},
|
||||
},
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
async function runPostReviewScript(workflowPath, options) {
|
||||
const script = extractPostReviewScript(workflowPath);
|
||||
const github = mockGithub(options);
|
||||
const context = {
|
||||
repo: { owner: "owner", repo: "repo" },
|
||||
issue: { number: 123 },
|
||||
eventName: "pull_request_target",
|
||||
payload: { pull_request: { head: { sha: "head-sha" } } },
|
||||
};
|
||||
const sandbox = {
|
||||
github,
|
||||
context,
|
||||
console: { log() {} },
|
||||
require(name) {
|
||||
if (name === "fs") return options.fs;
|
||||
throw new Error(`unexpected require: ${name}`);
|
||||
},
|
||||
};
|
||||
|
||||
await vm.runInNewContext(`(async () => {\n${script}\n})()`, sandbox, {
|
||||
timeout: 1000,
|
||||
});
|
||||
|
||||
return github;
|
||||
}
|
||||
|
||||
async function testFailedInlineCommentsAreSummarized(workflowPath) {
|
||||
const result = {
|
||||
comments: [
|
||||
{
|
||||
path: "docs/no-line.md",
|
||||
content:
|
||||
"No-line content with a fenced block:\n\n```js\nconsole.log('still visible');\n```",
|
||||
existing_code: "",
|
||||
suggestion_code: "",
|
||||
start_line: 0,
|
||||
end_line: 0,
|
||||
},
|
||||
{
|
||||
path: "src/app.js",
|
||||
content: "Failed inline content must remain visible in the PR summary.",
|
||||
existing_code: "oldCall();",
|
||||
suggestion_code: "newCall();",
|
||||
start_line: 10,
|
||||
end_line: 10,
|
||||
},
|
||||
],
|
||||
warnings: [],
|
||||
};
|
||||
|
||||
const github = await runPostReviewScript(workflowPath, {
|
||||
fs: mockFs(JSON.stringify(result), ""),
|
||||
bulkError: 'Unprocessable Entity: "Line could not be resolved"',
|
||||
individualError: 'Unprocessable Entity: "Line could not be resolved"',
|
||||
});
|
||||
|
||||
assert.strictEqual(github.createReviewCalls.length, 2);
|
||||
assert.strictEqual(github.issueComments.length, 1);
|
||||
const body = github.issueComments[0].body;
|
||||
assert.match(body, /No-line content with a fenced block/);
|
||||
assert.match(body, /Failed inline content must remain visible/);
|
||||
assert.match(body, /Line could not be resolved/);
|
||||
}
|
||||
|
||||
async function testErrorCommentUsesSafeFence(workflowPath) {
|
||||
const github = await runPostReviewScript(workflowPath, {
|
||||
fs: mockFs("not json", "stderr includes a fence\n```js\nbroken();\n```"),
|
||||
});
|
||||
|
||||
assert.strictEqual(github.issueComments.length, 1);
|
||||
const body = github.issueComments[0].body;
|
||||
assert.match(body, /\n````\nstderr includes a fence/);
|
||||
}
|
||||
|
||||
async function main() {
|
||||
for (const workflowPath of workflowFiles) {
|
||||
await testFailedInlineCommentsAreSummarized(workflowPath);
|
||||
await testErrorCommentUsesSafeFence(workflowPath);
|
||||
}
|
||||
}
|
||||
|
||||
main().catch((err) => {
|
||||
console.error(err);
|
||||
process.exit(1);
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue