Skip to content

Fix sync review source blockers #1940

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stranske merged 1 commit into from
Apr 27, 2026
Merged

Fix sync review source blockers #1940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 24 additions & 0 deletions .github/scripts/__tests__/coverage-monitor-summary.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,30 @@ test('includes PR source context coverage when configured', () => {
assert.match(formatMonitorMarkdown(summary), /pr-source-context \| warning/);
});

test('skips absent PR source context report when not configured', () => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'coverage-monitor-'));
const terminal = writeJson(dir, 'terminal.json', report('pass'));
const botAuth = writeJson(dir, 'bot-auth.json', report('pass'));

const summary = buildCoverageMonitorSummary({
terminal_report: terminal,
bot_auth_report: botAuth,
pr_source_context_report: path.join(dir, 'missing-pr-source.json'),
});

assert.equal(summary.status, 'pass');
assert.deepEqual(
summary.monitors.map((monitor) => monitor.label),
['terminal-disposition', 'bot-comment-auth']
);
});

test('does not configure PR source context coverage by default', () => {
const options = parseArgs([]);

assert.equal(options.pr_source_context_report, '');
});

test('surfaces warning blockers without activating hard-block policy', () => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'coverage-monitor-'));
const terminal = writeJson(
Expand Down
26 changes: 26 additions & 0 deletions .github/scripts/__tests__/source-context.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,32 @@ test('extractIssueNumberFromPull keeps existing issue resolution behavior', () =
);
});

test('extractIssueNumberFromPull ignores PR references in workflow source templates', () => {
const context = resolvePrSourceContext({
body: `
## Workflow Source

Started from:
- [ ] GitHub issue: #
- [x] Review follow-up from PR #315
- [ ] Direct PR / remote GitHub work
`,
head: { ref: 'review-followup/source-context' },
title: 'fix: address review follow-up',
});

assert.equal(extractIssueNumberFromPull({ body: 'Review follow-up from PR #315' }), null);
assert.equal(context.sourceType, SOURCE_TYPES.REVIEW_FOLLOWUP);
assert.equal(context.issueNumber, null);
assert.equal(context.requiresIssue, false);
});

test('extractIssueNumberFromPull requires explicit issue wording for body references', () => {
assert.equal(extractIssueNumberFromPull({ body: 'See PR #456 for context' }), null);
assert.equal(extractIssueNumberFromPull({ body: 'Related to issue #456' }), 456);
assert.equal(extractIssueNumberFromPull({ body: 'Closes #789' }), 789);
});

test('parseWorkflowSourceBlock reads source-context fields from hidden block', () => {
const block = parseWorkflowSourceBlock(`
<!-- workflow-source:start -->
Expand Down
11 changes: 8 additions & 3 deletions .github/scripts/coverage_monitor_summary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,14 @@ function buildCoverageMonitorSummary(options = {}) {
const terminal = summarizeReport(readJsonReport(options.terminal_report, 'terminal-disposition'));
const botAuth = summarizeReport(readJsonReport(options.bot_auth_report, 'bot-comment-auth'));
const monitors = [terminal, botAuth];
if (cleanString(options.pr_source_context_report)) {
const prSourceContextReportPath = cleanString(options.pr_source_context_report);
if (
prSourceContextReportPath &&
fs.existsSync(prSourceContextReportPath) &&
fs.statSync(prSourceContextReportPath).isFile()
) {
Comment on lines +143 to +148

Copilot AI Apr 27, 2026

Copy link

Choose a reason for hiding this comment

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

fs.statSync(prSourceContextReportPath) can throw (e.g., permission errors, broken symlinks, race between existsSync and statSync), which would crash the summary step. Consider wrapping the statSync call in a try/catch (treat errors as “report missing”/skip) or avoid the pre-stat and let readJsonReport() handle read errors safely.

Copilot uses AI. Check for mistakes.
monitors.push(
summarizeReport(readJsonReport(options.pr_source_context_report, 'pr-source-context'))
summarizeReport(readJsonReport(prSourceContextReportPath, 'pr-source-context'))
);
}
const status = overallStatus(monitors);
Expand Down Expand Up @@ -217,7 +222,7 @@ function parseArgs(argv = process.argv.slice(2)) {
bot_auth_report:
process.env.COVERAGE_MONITOR_BOT_AUTH_JSON || 'bot-comment-auth-coverage-summary.json',
pr_source_context_report:
process.env.COVERAGE_MONITOR_PR_SOURCE_CONTEXT_JSON || 'pr-source-context-coverage.json',
process.env.COVERAGE_MONITOR_PR_SOURCE_CONTEXT_JSON || '',
output_json:
process.env.COVERAGE_MONITOR_SUMMARY_JSON || 'coverage-monitor-summary.json',
output_md:
Expand Down
17 changes: 17 additions & 0 deletions .github/scripts/source_context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,20 @@ function labelNames(pull = {}) {
: [];
}

function hasExplicitIssueReferencePrefix(value) {
const prefix = cleanString(value)
.replace(/[>_[\]()`*~]/g, ' ')
.replace(/\s+/g, ' ');

if (/\b(?:pr|pull\s+request)\s*[:#-]?\s*$/i.test(prefix)) {
return false;
}

return /\b(?:close[sd]?|closing|fix(?:e[sd])?|fixing|resolve[sd]?|resolving|relate[sd]?\s+to|refs?|references?|issue|source\s+issue|github\s+issue)\s*[:#-]?\s*$/i.test(
prefix
);
}

function extractIssueNumberFromText(text) {
const value = String(text || '');
for (const match of value.matchAll(/#([0-9]+)/g)) {
Expand All @@ -124,6 +138,9 @@ function extractIssueNumberFromText(text) {
if (/\b(?:run|attempt|step|job|check|version|v)\s*$/i.test(preceding)) {
continue;
}
if (!hasExplicitIssueReferencePrefix(value.slice(Math.max(0, match.index - 80), match.index))) {
continue;
Comment on lines +141 to +142

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep title shorthand issue references detectable

Applying hasExplicitIssueReferencePrefix(...) to every #N token now also affects title parsing (because extractIssueNumberFromPull calls extractIssueNumberFromText on pull.title first). Titles that previously resolved issue links via shorthand like feature tweak (#123) no longer match, so these PRs fall through to source-context-unknown even when they are issue-sourced. This introduces false warnings/repair comments for a common title format that the prior implementation accepted.

Useful? React with 👍 / 👎.

}
const parsed = Number.parseInt(match[1], 10);
if (!Number.isNaN(parsed)) {
return parsed;
Expand Down
2 changes: 1 addition & 1 deletion scripts/aggregate_agent_metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,7 @@ def _summarise_verifier(
unsupported_model_dispositions[str(disposition)] += 1
elif is_verifier_terminal and model_metadata_required:
verifier_mode = str(entry.get("verifier_mode") or "").strip().lower()
if verifier_mode != "evaluate":
if verifier_mode and verifier_mode != "evaluate":
disposition = entry.get("disposition") or entry.get("terminal_state") or "unknown"
if _is_pre_contract_verifier_model_record(entry, model_metadata_required_after):
legacy_missing_verifier_model_metadata[str(disposition)] += 1
Expand Down
11 changes: 8 additions & 3 deletions templates/consumer-repo/.github/scripts/coverage_monitor_summary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,14 @@ function buildCoverageMonitorSummary(options = {}) {
const terminal = summarizeReport(readJsonReport(options.terminal_report, 'terminal-disposition'));
const botAuth = summarizeReport(readJsonReport(options.bot_auth_report, 'bot-comment-auth'));
const monitors = [terminal, botAuth];
if (cleanString(options.pr_source_context_report)) {
const prSourceContextReportPath = cleanString(options.pr_source_context_report);
if (
prSourceContextReportPath &&
fs.existsSync(prSourceContextReportPath) &&
fs.statSync(prSourceContextReportPath).isFile()
) {
Comment on lines +143 to +148

Copilot AI Apr 27, 2026

Copy link

Choose a reason for hiding this comment

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

fs.statSync(prSourceContextReportPath) can throw (e.g., permission errors, broken symlinks, race between existsSync and statSync), which would crash the summary step. Consider wrapping the statSync call in a try/catch (treat errors as “report missing”/skip) or avoid the pre-stat and let readJsonReport() handle read errors safely.

Copilot uses AI. Check for mistakes.
monitors.push(
summarizeReport(readJsonReport(options.pr_source_context_report, 'pr-source-context'))
summarizeReport(readJsonReport(prSourceContextReportPath, 'pr-source-context'))
);
}
const status = overallStatus(monitors);
Expand Down Expand Up @@ -217,7 +222,7 @@ function parseArgs(argv = process.argv.slice(2)) {
bot_auth_report:
process.env.COVERAGE_MONITOR_BOT_AUTH_JSON || 'bot-comment-auth-coverage-summary.json',
pr_source_context_report:
process.env.COVERAGE_MONITOR_PR_SOURCE_CONTEXT_JSON || 'pr-source-context-coverage.json',
process.env.COVERAGE_MONITOR_PR_SOURCE_CONTEXT_JSON || '',
output_json:
process.env.COVERAGE_MONITOR_SUMMARY_JSON || 'coverage-monitor-summary.json',
output_md:
Expand Down
17 changes: 17 additions & 0 deletions templates/consumer-repo/.github/scripts/source_context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,20 @@ function labelNames(pull = {}) {
: [];
}

function hasExplicitIssueReferencePrefix(value) {
const prefix = cleanString(value)
.replace(/[>_[\]()`*~]/g, ' ')
.replace(/\s+/g, ' ');

if (/\b(?:pr|pull\s+request)\s*[:#-]?\s*$/i.test(prefix)) {
return false;
}

return /\b(?:close[sd]?|closing|fix(?:e[sd])?|fixing|resolve[sd]?|resolving|relate[sd]?\s+to|refs?|references?|issue|source\s+issue|github\s+issue)\s*[:#-]?\s*$/i.test(
prefix
);
}

function extractIssueNumberFromText(text) {
const value = String(text || '');
for (const match of value.matchAll(/#([0-9]+)/g)) {
Expand All @@ -124,6 +138,9 @@ function extractIssueNumberFromText(text) {
if (/\b(?:run|attempt|step|job|check|version|v)\s*$/i.test(preceding)) {
continue;
}
if (!hasExplicitIssueReferencePrefix(value.slice(Math.max(0, match.index - 80), match.index))) {
continue;
}
const parsed = Number.parseInt(match[1], 10);
if (!Number.isNaN(parsed)) {
return parsed;
Expand Down
2 changes: 1 addition & 1 deletion templates/consumer-repo/scripts/aggregate_agent_metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,7 @@ def _summarise_verifier(
unsupported_model_dispositions[str(disposition)] += 1
elif is_verifier_terminal and model_metadata_required:
verifier_mode = str(entry.get("verifier_mode") or "").strip().lower()
if verifier_mode != "evaluate":
if verifier_mode and verifier_mode != "evaluate":
disposition = entry.get("disposition") or entry.get("terminal_state") or "unknown"
if _is_pre_contract_verifier_model_record(entry, model_metadata_required_after):
legacy_missing_verifier_model_metadata[str(disposition)] += 1
Expand Down
4 changes: 2 additions & 2 deletions tests/scripts/test_aggregate_agent_metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -911,7 +911,7 @@ def test_verifier_summary_counts_missing_model_metadata(
assert "Missing verifier model metadata: verifier-error (1)" in summary


def test_verifier_summary_counts_missing_model_metadata_for_unknown_mode(
def test_verifier_summary_ignores_missing_model_metadata_for_unknown_mode(
monkeypatch: pytest.MonkeyPatch,
) -> None:
monkeypatch.setenv(
Expand All @@ -931,7 +931,7 @@ def test_verifier_summary_counts_missing_model_metadata_for_unknown_mode(
]
)

assert verifier["missing_verifier_model_metadata"]["verifier-error"] == 1
assert verifier["missing_verifier_model_metadata"] == Counter()


def test_verifier_summary_suppresses_pre_contract_missing_model_metadata(
Expand Down
Loading