Skip to content

fix: address sync review feedback #1995

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

Closed
stranske wants to merge 1 commit into from
Closed

fix: address sync review feedback #1995

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
62 changes: 60 additions & 2 deletions .github/scripts/__tests__/keepalive-orchestrator-gate-runner.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
'',
'<!-- auto-status-summary:start -->',
'## Automated Status Summary',
'',
'#### Tasks',
'- [x] Acceptance covered',
'',
'#### Acceptance Criteria',
'- [x] Tests pass',
'<!-- auto-status-summary:end -->',
].join('\n'),
});
const github = createGithub({
pull: pr,
Expand All @@ -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();
});
Expand All @@ -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',
'',
'<!-- auto-status-summary:start -->',
'## Automated Status Summary',
'',
'#### Tasks',
'- [x] Implementation started',
'',
'#### Acceptance Criteria',
'- [ ] Acceptance complete',
'<!-- auto-status-summary:end -->',
].join('\n'),
});
const github = createGithub({ pull: pr });

Expand All @@ -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();
});
68 changes: 65 additions & 3 deletions .github/scripts/keepalive_orchestrator_gate_runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '<!-- keepalive-draft-disposition -->';
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) {
Expand Down Expand Up @@ -79,6 +81,65 @@ function countMarkdownCheckboxes(body) {
return counts;
}

function countDraftDispositionCheckboxes(body) {
const normalized = String(body || '').replace(/\r\n/g, '\n');
const startMarker = '<!-- auto-status-summary:start -->';
const endMarker = '<!-- auto-status-summary:end -->';
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 = '';
Comment on lines +130 to +131

@chatgpt-codex-connector chatgpt-codex-connector Bot May 4, 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.

P1 Badge Keep counting checklist items across nested subheadings

When a draft PR body has a Tasks/Acceptance Criteria section that includes an internal heading (for example ### Phase 2 under #### Tasks), this branch clears section and stops collecting subsequent checkboxes, so unchecked items can be skipped and the PR can be auto-marked ready incorrectly. This is a real path because our parser explicitly allows subsection headers inside checklist sections (.github/scripts/issue_scope_parser.js notes that ### Phase 1 inside #### Tasks should remain content), so this logic can undercount unchecked work and bypass human routing.

Useful? React with 👍 / 👎.

}
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) {
Expand Down Expand Up @@ -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) {
Expand Down
7 changes: 3 additions & 4 deletions docs/LABELS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`

---

Expand Down
69 changes: 67 additions & 2 deletions scripts/langchain/followup_issue_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand Down Expand Up @@ -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] = []
Expand Down Expand Up @@ -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 = [
Expand Down Expand Up @@ -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)
Expand Down
68 changes: 65 additions & 3 deletions templates/consumer-repo/.github/scripts/keepalive_orchestrator_gate_runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '<!-- keepalive-draft-disposition -->';
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) {
Expand Down Expand Up @@ -79,6 +81,65 @@ function countMarkdownCheckboxes(body) {
return counts;
}

function countDraftDispositionCheckboxes(body) {
const normalized = String(body || '').replace(/\r\n/g, '\n');
const startMarker = '<!-- auto-status-summary:start -->';
const endMarker = '<!-- auto-status-summary:end -->';
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) {
Expand Down Expand Up @@ -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) {
Expand Down
Loading
Loading