From 58a0c361b40a2dc4ffd8ab0415b920392b540e0c Mon Sep 17 00:00:00 2001 From: Tim Stranske Date: Sun, 26 Apr 2026 19:26:06 -0500 Subject: [PATCH] fix: address sync review source blockers --- .../coverage-monitor-summary.test.js | 24 +++++++++++++++++ .../scripts/__tests__/source-context.test.js | 26 +++++++++++++++++++ .github/scripts/coverage_monitor_summary.js | 11 +++++--- .github/scripts/source_context.js | 17 ++++++++++++ scripts/aggregate_agent_metrics.py | 2 +- .../scripts/coverage_monitor_summary.js | 11 +++++--- .../.github/scripts/source_context.js | 17 ++++++++++++ .../scripts/aggregate_agent_metrics.py | 2 +- tests/scripts/test_aggregate_agent_metrics.py | 4 +-- 9 files changed, 104 insertions(+), 10 deletions(-) diff --git a/.github/scripts/__tests__/coverage-monitor-summary.test.js b/.github/scripts/__tests__/coverage-monitor-summary.test.js index 7606e62d5..8d9fe7b26 100644 --- a/.github/scripts/__tests__/coverage-monitor-summary.test.js +++ b/.github/scripts/__tests__/coverage-monitor-summary.test.js @@ -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( diff --git a/.github/scripts/__tests__/source-context.test.js b/.github/scripts/__tests__/source-context.test.js index 2ee481e72..3824725b1 100644 --- a/.github/scripts/__tests__/source-context.test.js +++ b/.github/scripts/__tests__/source-context.test.js @@ -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(` diff --git a/.github/scripts/coverage_monitor_summary.js b/.github/scripts/coverage_monitor_summary.js index 790e59555..bf99428ac 100644 --- a/.github/scripts/coverage_monitor_summary.js +++ b/.github/scripts/coverage_monitor_summary.js @@ -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() + ) { monitors.push( - summarizeReport(readJsonReport(options.pr_source_context_report, 'pr-source-context')) + summarizeReport(readJsonReport(prSourceContextReportPath, 'pr-source-context')) ); } const status = overallStatus(monitors); @@ -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: diff --git a/.github/scripts/source_context.js b/.github/scripts/source_context.js index 8566ac635..274eab041 100644 --- a/.github/scripts/source_context.js +++ b/.github/scripts/source_context.js @@ -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)) { @@ -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; diff --git a/scripts/aggregate_agent_metrics.py b/scripts/aggregate_agent_metrics.py index 6bbd78d88..69454c373 100755 --- a/scripts/aggregate_agent_metrics.py +++ b/scripts/aggregate_agent_metrics.py @@ -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 diff --git a/templates/consumer-repo/.github/scripts/coverage_monitor_summary.js b/templates/consumer-repo/.github/scripts/coverage_monitor_summary.js index 790e59555..bf99428ac 100644 --- a/templates/consumer-repo/.github/scripts/coverage_monitor_summary.js +++ b/templates/consumer-repo/.github/scripts/coverage_monitor_summary.js @@ -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() + ) { monitors.push( - summarizeReport(readJsonReport(options.pr_source_context_report, 'pr-source-context')) + summarizeReport(readJsonReport(prSourceContextReportPath, 'pr-source-context')) ); } const status = overallStatus(monitors); @@ -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: diff --git a/templates/consumer-repo/.github/scripts/source_context.js b/templates/consumer-repo/.github/scripts/source_context.js index 8566ac635..274eab041 100644 --- a/templates/consumer-repo/.github/scripts/source_context.js +++ b/templates/consumer-repo/.github/scripts/source_context.js @@ -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)) { @@ -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; diff --git a/templates/consumer-repo/scripts/aggregate_agent_metrics.py b/templates/consumer-repo/scripts/aggregate_agent_metrics.py index 6bbd78d88..69454c373 100755 --- a/templates/consumer-repo/scripts/aggregate_agent_metrics.py +++ b/templates/consumer-repo/scripts/aggregate_agent_metrics.py @@ -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 diff --git a/tests/scripts/test_aggregate_agent_metrics.py b/tests/scripts/test_aggregate_agent_metrics.py index eedce9aff..28da0a2b8 100644 --- a/tests/scripts/test_aggregate_agent_metrics.py +++ b/tests/scripts/test_aggregate_agent_metrics.py @@ -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( @@ -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(