From 427be7671422a9ff2a1f236e746c87af0fda7ded Mon Sep 17 00:00:00 2001 From: Tim Stranske Date: Mon, 4 May 2026 03:21:15 -0500 Subject: [PATCH] fix: address sync review feedback --- ...keepalive-orchestrator-gate-runner.test.js | 62 ++++++++++++++++- .../keepalive_orchestrator_gate_runner.js | 68 +++++++++++++++++- docs/LABELS.md | 7 +- scripts/langchain/followup_issue_generator.py | 69 ++++++++++++++++++- .../keepalive_orchestrator_gate_runner.js | 68 +++++++++++++++++- templates/consumer-repo/docs/LABELS.md | 5 +- tests/test_followup_issue_generator.py | 69 +++++++++++++++++++ 7 files changed, 331 insertions(+), 17 deletions(-) diff --git a/.github/scripts/__tests__/keepalive-orchestrator-gate-runner.test.js b/.github/scripts/__tests__/keepalive-orchestrator-gate-runner.test.js index c37cbd72f..5a58f9362 100644 --- a/.github/scripts/__tests__/keepalive-orchestrator-gate-runner.test.js +++ b/.github/scripts/__tests__/keepalive-orchestrator-gate-runner.test.js @@ -413,7 +413,21 @@ test('runKeepaliveGate converts checklist-complete draft PRs to ready for review const pr = makePullRequest({ draft: true, labels: ['agents:keepalive', 'agent:codex'], - body: '- [x] Acceptance covered\n- [x] Tests pass', + body: [ + '### Review Checklist', + '- [ ] CI passes with updated workflows', + '- [ ] No repo-specific customizations were overwritten', + '', + '', + '## Automated Status Summary', + '', + '#### Tasks', + '- [x] Acceptance covered', + '', + '#### Acceptance Criteria', + '- [x] Tests pass', + '', + ].join('\n'), }); const github = createGithub({ pull: pr, @@ -435,6 +449,7 @@ test('runKeepaliveGate converts checklist-complete draft PRs to ready for review assert.equal(outputs.reason, ''); assert.equal(github.__calls.graphql.length, 1); assert.equal(github.__calls.graphql[0].variables.pullRequestId, 'PR_node_17'); + assert.equal(github.__calls.commentsCreated.length, 0); assert.ok(summary.entries.some((entry) => entry.text?.includes('marked ready for review'))); restore(); }); @@ -447,7 +462,20 @@ test('runKeepaliveGate routes incomplete draft PRs to human', async () => { const pr = makePullRequest({ draft: true, labels: ['agents:keepalive', 'agent:codex'], - body: '- [x] Implementation started\n- [ ] Acceptance complete', + body: [ + '### Review Checklist', + '- [x] CI passes with updated workflows', + '', + '', + '## Automated Status Summary', + '', + '#### Tasks', + '- [x] Implementation started', + '', + '#### Acceptance Criteria', + '- [ ] Acceptance complete', + '', + ].join('\n'), }); const github = createGithub({ pull: pr }); @@ -467,3 +495,33 @@ test('runKeepaliveGate routes incomplete draft PRs to human', async () => { assert.match(github.__calls.commentsCreated[0].body, /Draft PR requires human disposition/); restore(); }); + +test('runKeepaliveGate ignores PR-template checkboxes when deciding draft readiness', async () => { + const { core, outputs } = createCore(); + const gateStub = async () => createGateResult(); + const { runKeepaliveGate, restore } = loadRunnerWithGate(gateStub); + + const pr = makePullRequest({ + draft: true, + labels: ['agents:keepalive', 'agent:codex'], + body: [ + '### Review Checklist', + '- [x] CI passes with updated workflows', + '- [x] No repo-specific customizations were overwritten', + ].join('\n'), + }); + const github = createGithub({ pull: pr }); + + await runKeepaliveGate({ + core, + github, + context: { repo: { owner: 'octo', repo: 'demo' }, runId: 45 }, + env: makeEnv({ KEEPALIVE_MAX_RETRIES: '5' }), + }); + + assert.equal(outputs.proceed, 'false'); + assert.equal(outputs.reason, 'pr-draft-needs-human'); + assert.equal(github.__calls.graphql.length, 0); + assert.equal(github.__calls.commentsCreated.length, 1); + restore(); +}); diff --git a/.github/scripts/keepalive_orchestrator_gate_runner.js b/.github/scripts/keepalive_orchestrator_gate_runner.js index 2667f8be0..938170c7b 100644 --- a/.github/scripts/keepalive_orchestrator_gate_runner.js +++ b/.github/scripts/keepalive_orchestrator_gate_runner.js @@ -14,6 +14,8 @@ const PAUSE_LABEL = 'agents:paused'; const NEEDS_HUMAN_LABEL = 'needs-human'; const NEEDS_ATTENTION_LABEL = 'agent:needs-attention'; const DRAFT_DISPOSITION_MARKER = ''; +const TASK_SECTION_ALIASES = new Set(['tasks', 'task', 'task list', 'implementation', 'to do', 'todo', 'to-do']); +const ACCEPTANCE_SECTION_ALIASES = new Set(['acceptance criteria', 'acceptance', 'definition of done', 'done criteria']); function normaliseLabelName(label) { if (!label) { @@ -79,6 +81,65 @@ function countMarkdownCheckboxes(body) { return counts; } +function countDraftDispositionCheckboxes(body) { + const normalized = String(body || '').replace(/\r\n/g, '\n'); + const startMarker = ''; + const endMarker = ''; + const startIndex = normalized.indexOf(startMarker); + const endIndex = normalized.indexOf(endMarker); + const segment = startIndex !== -1 && endIndex !== -1 && endIndex > startIndex + ? normalized.slice(startIndex + startMarker.length, endIndex) + : normalized; + const lines = segment.split('\n'); + const scopedLines = []; + let section = ''; + let insideCodeBlock = false; + + const normaliseHeading = (line) => { + const trimmed = String(line || '').trim(); + if (!trimmed) { + return ''; + } + let value = trimmed.replace(/^#{1,6}\s+/, ''); + const boldMatch = value.match(/^(?:\*\*|__)(.+?)(?:\*\*|__)\s*:?\s*$/); + if (boldMatch) { + value = boldMatch[1]; + } + return value.replace(/\s*:\s*$/, '').trim().toLowerCase(); + }; + + const isHeading = (line) => { + const trimmed = String(line || '').trim(); + return /^#{1,6}\s+\S/.test(trimmed) || /^(?:\*\*|__)(.+?)(?:\*\*|__)\s*:?\s*$/.test(trimmed); + }; + + for (const line of lines) { + if (/^(`{3,}|~{3,})/.test(line.trim())) { + insideCodeBlock = !insideCodeBlock; + continue; + } + if (insideCodeBlock) { + continue; + } + if (isHeading(line)) { + const heading = normaliseHeading(line); + if (TASK_SECTION_ALIASES.has(heading)) { + section = 'tasks'; + } else if (ACCEPTANCE_SECTION_ALIASES.has(heading)) { + section = 'acceptance'; + } else if (section) { + section = ''; + } + continue; + } + if (section === 'tasks' || section === 'acceptance') { + scopedLines.push(line); + } + } + + return countMarkdownCheckboxes(scopedLines.join('\n')); +} + async function addLabelsIfMissing({ github, owner, repo, prNumber, labels, currentLabels, core, summary }) { const toAdd = labels.filter((label) => label && !currentLabels.has(label.toLowerCase())); if (!toAdd.length) { @@ -394,14 +455,15 @@ async function runKeepaliveGate({ core, github, context, env }) { } let draftRequiresHuman = false; if (pr.draft) { - const checkboxCounts = countMarkdownCheckboxes(pr.body || ''); + const checkboxCounts = countDraftDispositionCheckboxes(pr.body || ''); summary .addRaw( - `Pull request is draft; evaluating disposition (checked=${checkboxCounts.checked}, unchecked=${checkboxCounts.unchecked}).` + `Pull request is draft; evaluating keepalive checklist disposition (checked=${checkboxCounts.checked}, unchecked=${checkboxCounts.unchecked}).` ) .addEOL(); - const allChecklistWorkComplete = checkboxCounts.checked > 0 && checkboxCounts.unchecked === 0; + const totalChecklistItems = checkboxCounts.checked + checkboxCounts.unchecked; + const allChecklistWorkComplete = totalChecklistItems > 0 && checkboxCounts.unchecked === 0; if (allChecklistWorkComplete) { const ready = await markDraftReadyForReview({ github, pr, core, summary }); if (ready) { diff --git a/docs/LABELS.md b/docs/LABELS.md index d7ae3d6e2..caa0e706b 100644 --- a/docs/LABELS.md +++ b/docs/LABELS.md @@ -333,13 +333,12 @@ These labels trigger the post-merge verifier workflow on a merged PR. - Extracted concerns from verification - Scores below 7/10 - Suggested tasks for addressing issues -3. Posts comment on original PR linking to new issue -4. Removes the `verify:create-issue` label after completion -5. Adds `agents:optimize` label to new issue for agent formatting +3. Labels the follow-up issue with `agents:auto-pilot` and `from:verification` +4. Removes the `verify:create-issue` label after processing **Use Case:** User-triggered creation of follow-up work from verification feedback. Replaces automatic issue creation which was too aggressive. -**Workflow:** `agents-verify-to-issue-v2.yml` +**Workflow:** `agents-80-pr-event-hub.yml` --- diff --git a/scripts/langchain/followup_issue_generator.py b/scripts/langchain/followup_issue_generator.py index c1f6773d2..1421ba7d1 100755 --- a/scripts/langchain/followup_issue_generator.py +++ b/scripts/langchain/followup_issue_generator.py @@ -85,6 +85,25 @@ "not ready", } NON_PASS_DETAIL_LIMIT = 10 +REPO_LOCAL_HINTS = ( + "staging_", + "migration", + "database", + "schema", + "table", + "pytest", + "test_database_strategy", +) +WORKFLOW_SYNC_HINTS = ( + "workflows-owned scripts", + ".github/workflows/", + ".github/ template", + "workflow-sync", + "template directories", + "aggregate_agent_metrics.py", + "source_context.js", + "agents_pr_meta_update_body.js", +) LOGGER = logging.getLogger(__name__) @@ -259,6 +278,42 @@ def _is_advisory_concern(concern: str) -> bool: return any(re.search(pattern, text) for pattern in ADVISORY_PATTERNS) +def _select_followup_acceptance_criteria( + acceptance_criteria: list[str], blocking_concerns: list[str] +) -> list[str]: + """Keep follow-up acceptance criteria aligned with the active concern surface.""" + if not acceptance_criteria: + return [] + + concern_text = " ".join(blocking_concerns).lower() + acceptance_text = " ".join(acceptance_criteria).lower() + has_repo_local_acceptance = any(hint in acceptance_text for hint in REPO_LOCAL_HINTS) + has_workflow_sync_acceptance = any(hint in acceptance_text for hint in WORKFLOW_SYNC_HINTS) + repo_local_focus = any(hint in concern_text for hint in REPO_LOCAL_HINTS) or ( + has_repo_local_acceptance and has_workflow_sync_acceptance + ) + if not repo_local_focus: + return acceptance_criteria[:10] + + filtered = [ + criterion + for criterion in acceptance_criteria + if not any(hint in criterion.lower() for hint in WORKFLOW_SYNC_HINTS) + ] + return (filtered or acceptance_criteria)[:10] + + +def _should_emphasize_repo_local_summary( + acceptance_criteria: list[str], concerns: list[str] +) -> bool: + acceptance_text = " ".join(acceptance_criteria).lower() + concern_text = " ".join(concerns).lower() + has_repo_local_acceptance = any(hint in acceptance_text for hint in REPO_LOCAL_HINTS) + has_workflow_sync_acceptance = any(hint in acceptance_text for hint in WORKFLOW_SYNC_HINTS) + has_repo_local_concern = any(hint in concern_text for hint in REPO_LOCAL_HINTS) + return has_repo_local_concern or (has_repo_local_acceptance and has_workflow_sync_acceptance) + + def _split_concerns(concerns: list[str]) -> tuple[list[str], list[str]]: blocking: list[str] = [] advisory: list[str] = [] @@ -1575,8 +1630,10 @@ def _generate_without_llm( task = f"Address: {task}" tasks.append(task) - # Use original unmet acceptance criteria - acceptance_criteria = original_issue.acceptance_criteria[:10] + # Keep acceptance criteria focused on the active implementation surface. + acceptance_criteria = _select_followup_acceptance_criteria( + original_issue.acceptance_criteria, blocking_concerns + ) # Build body body_parts = [ @@ -1766,6 +1823,14 @@ def _build_why_section( if needs_human_reason: parts.append(needs_human_reason) + if _should_emphasize_repo_local_summary( + original_issue.acceptance_criteria, verification_data.concerns + ): + parts.append( + "The follow-up scope is repo-local: keep the PR summary centered on " + "migration and database-test evidence and avoid unrelated workflow-sync criteria." + ) + parts.append("This follow-up addresses the remaining gaps with improved task structure.") return " ".join(parts) diff --git a/templates/consumer-repo/.github/scripts/keepalive_orchestrator_gate_runner.js b/templates/consumer-repo/.github/scripts/keepalive_orchestrator_gate_runner.js index 2667f8be0..938170c7b 100644 --- a/templates/consumer-repo/.github/scripts/keepalive_orchestrator_gate_runner.js +++ b/templates/consumer-repo/.github/scripts/keepalive_orchestrator_gate_runner.js @@ -14,6 +14,8 @@ const PAUSE_LABEL = 'agents:paused'; const NEEDS_HUMAN_LABEL = 'needs-human'; const NEEDS_ATTENTION_LABEL = 'agent:needs-attention'; const DRAFT_DISPOSITION_MARKER = ''; +const TASK_SECTION_ALIASES = new Set(['tasks', 'task', 'task list', 'implementation', 'to do', 'todo', 'to-do']); +const ACCEPTANCE_SECTION_ALIASES = new Set(['acceptance criteria', 'acceptance', 'definition of done', 'done criteria']); function normaliseLabelName(label) { if (!label) { @@ -79,6 +81,65 @@ function countMarkdownCheckboxes(body) { return counts; } +function countDraftDispositionCheckboxes(body) { + const normalized = String(body || '').replace(/\r\n/g, '\n'); + const startMarker = ''; + const endMarker = ''; + const startIndex = normalized.indexOf(startMarker); + const endIndex = normalized.indexOf(endMarker); + const segment = startIndex !== -1 && endIndex !== -1 && endIndex > startIndex + ? normalized.slice(startIndex + startMarker.length, endIndex) + : normalized; + const lines = segment.split('\n'); + const scopedLines = []; + let section = ''; + let insideCodeBlock = false; + + const normaliseHeading = (line) => { + const trimmed = String(line || '').trim(); + if (!trimmed) { + return ''; + } + let value = trimmed.replace(/^#{1,6}\s+/, ''); + const boldMatch = value.match(/^(?:\*\*|__)(.+?)(?:\*\*|__)\s*:?\s*$/); + if (boldMatch) { + value = boldMatch[1]; + } + return value.replace(/\s*:\s*$/, '').trim().toLowerCase(); + }; + + const isHeading = (line) => { + const trimmed = String(line || '').trim(); + return /^#{1,6}\s+\S/.test(trimmed) || /^(?:\*\*|__)(.+?)(?:\*\*|__)\s*:?\s*$/.test(trimmed); + }; + + for (const line of lines) { + if (/^(`{3,}|~{3,})/.test(line.trim())) { + insideCodeBlock = !insideCodeBlock; + continue; + } + if (insideCodeBlock) { + continue; + } + if (isHeading(line)) { + const heading = normaliseHeading(line); + if (TASK_SECTION_ALIASES.has(heading)) { + section = 'tasks'; + } else if (ACCEPTANCE_SECTION_ALIASES.has(heading)) { + section = 'acceptance'; + } else if (section) { + section = ''; + } + continue; + } + if (section === 'tasks' || section === 'acceptance') { + scopedLines.push(line); + } + } + + return countMarkdownCheckboxes(scopedLines.join('\n')); +} + async function addLabelsIfMissing({ github, owner, repo, prNumber, labels, currentLabels, core, summary }) { const toAdd = labels.filter((label) => label && !currentLabels.has(label.toLowerCase())); if (!toAdd.length) { @@ -394,14 +455,15 @@ async function runKeepaliveGate({ core, github, context, env }) { } let draftRequiresHuman = false; if (pr.draft) { - const checkboxCounts = countMarkdownCheckboxes(pr.body || ''); + const checkboxCounts = countDraftDispositionCheckboxes(pr.body || ''); summary .addRaw( - `Pull request is draft; evaluating disposition (checked=${checkboxCounts.checked}, unchecked=${checkboxCounts.unchecked}).` + `Pull request is draft; evaluating keepalive checklist disposition (checked=${checkboxCounts.checked}, unchecked=${checkboxCounts.unchecked}).` ) .addEOL(); - const allChecklistWorkComplete = checkboxCounts.checked > 0 && checkboxCounts.unchecked === 0; + const totalChecklistItems = checkboxCounts.checked + checkboxCounts.unchecked; + const allChecklistWorkComplete = totalChecklistItems > 0 && checkboxCounts.unchecked === 0; if (allChecklistWorkComplete) { const ready = await markDraftReadyForReview({ github, pr, core, summary }); if (ready) { diff --git a/templates/consumer-repo/docs/LABELS.md b/templates/consumer-repo/docs/LABELS.md index 3aa7c41e8..bcb1a09c8 100644 --- a/templates/consumer-repo/docs/LABELS.md +++ b/templates/consumer-repo/docs/LABELS.md @@ -333,9 +333,8 @@ These labels trigger the post-merge verifier workflow on a merged PR. - Extracted concerns from verification - Scores below 7/10 - Suggested tasks for addressing issues -3. Posts comment on original PR linking to new issue -4. Removes the `verify:create-issue` label after completion -5. Adds `agents:optimize` label to new issue for agent formatting +3. Labels the follow-up issue with `agents:auto-pilot` and `from:verification` +4. Removes the `verify:create-issue` label after processing **Use Case:** User-triggered creation of follow-up work from verification feedback. Replaces automatic issue creation which was too aggressive. diff --git a/tests/test_followup_issue_generator.py b/tests/test_followup_issue_generator.py index e537206b9..a063a7606 100755 --- a/tests/test_followup_issue_generator.py +++ b/tests/test_followup_issue_generator.py @@ -11,6 +11,8 @@ from scripts.langchain.followup_issue_generator import ( OriginalIssueData, VerificationData, + _build_why_section, + _select_followup_acceptance_criteria, extract_original_issue_data, extract_verification_data, generate_disposition_comment, @@ -695,6 +697,73 @@ def test_handles_missing_sections(self): class TestGenerateFollowupIssue: """Tests for generate_followup_issue function.""" + def test_select_followup_acceptance_criteria_filters_workflow_sync_items_for_repo_local_db_work( + self, + ): + acceptance = [ + "The follow-up PR does not add or modify Workflows-owned scripts in Pension-Data", + "The file tests/test_database_strategy.py exists and contains test functions for staging_consultant_engagements", + "At least one test verifies that after running migrations, the staging_consultant_engagements table exists in the database", + ] + concerns = [ + "Missing migration-path verification for staging_consultant_engagements table creation" + ] + + selected = _select_followup_acceptance_criteria(acceptance, concerns) + + assert len(selected) == 2 + assert all("Workflows-owned scripts" not in item for item in selected) + assert any("staging_consultant_engagements" in item for item in selected) + + def test_select_followup_acceptance_criteria_keeps_workflow_items_when_not_repo_local( + self, + ): + acceptance = [ + "The follow-up PR does not add or modify Workflows-owned scripts in Pension-Data", + "The follow-up PR does not modify files under .github/workflows/", + ] + concerns = ["Address verifier metadata mismatch in generated issue text"] + + selected = _select_followup_acceptance_criteria(acceptance, concerns) + + assert selected == acceptance + + def test_select_followup_acceptance_criteria_filters_workflow_items_by_acceptance_mix( + self, + ): + acceptance = [ + "The follow-up PR does not add or modify Workflows-owned scripts in Pension-Data", + "The file tests/test_database_strategy.py exists and contains test functions for staging_consultant_engagements", + ] + concerns = ["Clarify issue text in follow-up summary"] + + selected = _select_followup_acceptance_criteria(acceptance, concerns) + + assert selected == [ + "The file tests/test_database_strategy.py exists and contains test functions for staging_consultant_engagements" + ] + + def test_build_why_section_emphasizes_repo_local_summary_for_mixed_surface(self): + verification = VerificationData( + concerns=["Missing migration-path verification for staging_consultant_engagements"], + tasks_attempted=1, + tasks_completed=0, + iteration_count=1, + ) + issue = OriginalIssueData( + number=339, + acceptance_criteria=[ + "The follow-up PR does not add or modify Workflows-owned scripts in Pension-Data", + "At least one test verifies that after running migrations, the staging_consultant_engagements table exists in the database", + ], + ) + + why = _build_why_section(verification, issue, pr_number=362, verdict="FAIL") + + assert "repo-local" in why + assert "migration and database-test evidence" in why + assert "workflow-sync criteria" in why + def test_generate_without_llm(self): """Generate follow-up issue using structured extraction only.""" verification_data = VerificationData(