From 7ede10d8acf7cf10945723dd20adc641a2675985 Mon Sep 17 00:00:00 2001 From: Will McMahan Date: Fri, 25 Sep 2026 07:48:43 -0500 Subject: [PATCH] fix: clear up maint error messages --- .github/workflows/pr-ci-failure.yml | 2 +- .github/workflows/pr-review.yml | 2 +- .github/workflows/pr-revise.yml | 6 +-- .github/workflows/reconcile-pipeline.yml | 2 +- ops/maintenance/README.md | 13 +++-- ops/maintenance/src/issue-fix/index.ts | 19 +------ .../src/issue-fix/tools/give-up.ts | 36 +++++-------- .../src/pr-review/tools/post-review.ts | 4 +- ops/maintenance/src/shared/repo.ts | 51 ++++++++++++++----- ops/maintenance/test/repo.test.ts | 33 ++++++++++-- 10 files changed, 97 insertions(+), 71 deletions(-) diff --git a/.github/workflows/pr-ci-failure.yml b/.github/workflows/pr-ci-failure.yml index d006d5b3..b55b856a 100644 --- a/.github/workflows/pr-ci-failure.yml +++ b/.github/workflows/pr-ci-failure.yml @@ -120,7 +120,7 @@ jobs: else gh label create needs-human -R "$REPO" 2>/dev/null || true gh pr edit -R "$REPO" "$pr" --add-label needs-human || true - gh pr comment -R "$REPO" "$pr" --body "CI failed on this PR again: $checks_url. The loop already tried once, so this PR now carries the needs-human label and waits on you — a later green run, successful revision, or successful review clears it. + gh pr comment -R "$REPO" "$pr" --body "Something went wrong and CI is still failing after an automated fix attempt, so this PR now carries the needs-human label. A later green run, successful revision, or successful review clears it. " fi diff --git a/.github/workflows/pr-review.yml b/.github/workflows/pr-review.yml index 8c8d8756..cf27d15f 100644 --- a/.github/workflows/pr-review.yml +++ b/.github/workflows/pr-review.yml @@ -109,4 +109,4 @@ jobs: pr=${{ github.event.pull_request.number || inputs.pr }} gh label create needs-human 2>/dev/null || true gh pr edit "$pr" --add-label needs-human || true - gh pr comment "$pr" --body "pr-review ran but failed before posting a review — see ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} for the log. This PR now carries the needs-human label; a later successful review clears it." + gh pr comment "$pr" --body "Something went wrong during the automated review, so this PR now carries the needs-human label. A later successful review clears it." diff --git a/.github/workflows/pr-revise.yml b/.github/workflows/pr-revise.yml index 5cb9baa9..4f34ab33 100644 --- a/.github/workflows/pr-revise.yml +++ b/.github/workflows/pr-revise.yml @@ -101,8 +101,8 @@ jobs: --pr ${{ github.event.pull_request.number || github.event.issue.number || inputs.pr }} --checks "npm run build:libs,npm run lint:eslint,npm test" - # A failed run must not be silent on the PR it was working: the - # comment names the run so the log is one click away. + # A failed run must not be silent on the PR it was working. The + # comment stays generic; the details live in the run log. - name: Leave a failure trace on the PR if: failure() env: @@ -111,4 +111,4 @@ jobs: pr=${{ github.event.pull_request.number || github.event.issue.number || inputs.pr }} gh label create needs-human 2>/dev/null || true gh pr edit "$pr" --add-label needs-human || true - gh pr comment "$pr" --body "pr-revise ran but failed before delivering a revision — see ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} for the log. This PR now carries the needs-human label; the findings above still stand, and a later successful revision clears it." + gh pr comment "$pr" --body "Something went wrong during the automated revision, so this PR now carries the needs-human label. The findings above still stand, and a later successful revision clears it." diff --git a/.github/workflows/reconcile-pipeline.yml b/.github/workflows/reconcile-pipeline.yml index ac7c7d82..3596722d 100644 --- a/.github/workflows/reconcile-pipeline.yml +++ b/.github/workflows/reconcile-pipeline.yml @@ -81,5 +81,5 @@ jobs: echo "PR #$pr has had no activity since before $stale_before — flagging needs-human" gh label create needs-human -R "$REPO" 2>/dev/null || true gh pr edit -R "$REPO" "$pr" --add-label needs-human || true - gh pr comment -R "$REPO" "$pr" --body "This maintenance PR has had no activity for over four hours and is not making progress — its checks may be stuck, auto-merge may be unable to complete, or it may be waiting on a decision. The reconciler flags it \`needs-human\` so it is visible and the one-fix-in-flight slot is freed. Review, merge, or close it; or remove the label to let the loop retry." + gh pr comment -R "$REPO" "$pr" --body "Something went wrong and this maintenance PR has stopped making progress, so it now carries the \`needs-human\` label. Review, merge, or close it, or remove the label to let the loop retry." done diff --git a/ops/maintenance/README.md b/ops/maintenance/README.md index 6c440f85..cbb60330 100644 --- a/ops/maintenance/README.md +++ b/ops/maintenance/README.md @@ -233,17 +233,22 @@ labels the PR `needs-human` instead of cycling, and a green run on a labeled PR clears the label. An issue whose fix spends its whole retry budget without green checks -is labeled `needs-human` too, with the failing output in a comment; the -picker skips labeled issues, and removing the label re-queues one. +is labeled `needs-human` too; the picker skips labeled issues, and +removing the label re-queues one. When the loop gives up on a PR — a review inconclusive after its diff-only fallback, a review that could not be submitted, a failed revision run, or an exhausted rounds cap — the PR is labeled -`needs-human` with a comment saying why, so limbo is never silent: the +`needs-human` with a short comment, so limbo is never silent: the PR list shows exactly which PRs wait on a decision, the dispatcher's idle-gate notice names them, and a later successful review or revision clears the label on its own. That label, unlabeling an issue, disabling -auto-merge, and closing a PR are where the human hand remains. Enable "Allow +auto-merge, and closing a PR are where the human hand remains. + +Failure comments are deliberately generic. They say something went +wrong and name the label, never the cause: error messages, check output, +and GitHub API errors can carry paths, tokens, or environment details, +so those stay in the workflow run's log. Enable "Allow auto-merge" in the repository settings; without it the merge step falls back to a direct merge, which only succeeds when the checks are already green. diff --git a/ops/maintenance/src/issue-fix/index.ts b/ops/maintenance/src/issue-fix/index.ts index d54c1d81..8561317a 100644 --- a/ops/maintenance/src/issue-fix/index.ts +++ b/ops/maintenance/src/issue-fix/index.ts @@ -27,7 +27,7 @@ import type { EvalAssertion } from '@cycgraph/orchestrator'; import { deliveryNodes } from '@cycgraph/tools/git'; -import { flagNeedsHuman } from '../shared/repo.js'; +import { fatalNeedsHuman } from '../shared/repo.js'; import { stripCloses, templateEvidence } from '../shared/pr-template.js'; import { buildIssueFixContext, params } from './context.js'; import type { Params } from './context.js'; @@ -78,22 +78,7 @@ export function issueFix(): MaintenanceWorkflow { // An engine-level death (budget breach) bypasses the giveup node, // which would leave the picked issue approved-but-orphaned: its // label event already fired, so nothing re-queues it. - onFatal: async (error: unknown) => { - const issue = c.picked.issue; - if (issue === undefined) return undefined; - try { - const reason = error instanceof Error ? error.message : String(error); - const result = await flagNeedsHuman(c.repoRoot, issue, [ - `issue-fix died before finishing (${reason}), so this issue now carries the \`${c.maintenance.labels.needsHuman}\` label and the picker skips it.`, - 'Remove the label to re-queue it, or dispatch issue-fix naming this issue to override the skip.', - ].join('\n'), { label: c.maintenance.labels.needsHuman, ...(c.token !== undefined ? { token: c.token } : {}) }); - return result.flagged - ? `fatal-run cleanup: #${issue} flagged ${c.maintenance.labels.needsHuman}` - : `fatal-run cleanup on #${issue}: ${result.detail}`; - } catch (cleanupError) { - return `fatal-run cleanup failed on #${issue}: ${cleanupError instanceof Error ? cleanupError.message : String(cleanupError)}`; - } - }, + onFatal: fatalNeedsHuman(c.repoRoot, () => c.picked.issue, 'issue-fix', c.maintenance.labels.needsHuman, c.token), }; }, diff --git a/ops/maintenance/src/issue-fix/tools/give-up.ts b/ops/maintenance/src/issue-fix/tools/give-up.ts index 00c2c492..cef16e30 100644 --- a/ops/maintenance/src/issue-fix/tools/give-up.ts +++ b/ops/maintenance/src/issue-fix/tools/give-up.ts @@ -2,9 +2,9 @@ * give_up — flag the picked issue when no pull request can be delivered. * * One node serves every dead end in the graph — a finding already gone, a - * spent gate budget, an unapproved review — so the comment names which one - * sent the run here and carries that path's evidence. A detached run has no - * issue to flag. The label makes the picker skip the issue until a human + * spent gate budget, an unapproved review. The issue gets a generic notice; + * which dead end sent the run here is reported only in the tool result. A + * detached run has no issue to flag. The label makes the picker skip the issue until a human * intervenes. * * @module maintenance/issue-fix/tools/give-up @@ -12,7 +12,7 @@ import { z } from 'zod'; import { tool } from '@cycgraph/orchestrator'; -import { flagNeedsHuman } from '../../shared/repo.js'; +import { giveUpNeedsHuman } from '../../shared/repo.js'; import type { IssueFixContext } from '../context.js'; /** The give-up tool, bound to the run's context. */ @@ -30,31 +30,19 @@ export function giveUpTool(c: IssueFixContext) { review_check_result: z.unknown().optional(), }), timeoutMs: 60_000, - execute: async ({ pick_result, baseline_result, judge_result, checks_result, review_check_result }) => { + execute: async ({ pick_result, baseline_result, judge_result, review_check_result }) => { const issue = (pick_result as { issue_number?: number } | undefined)?.issue_number; if (issue === undefined) return { flagged: false, detail: 'detached run — nothing to flag' }; - const baseline = baseline_result as { has_target?: boolean; detail?: string } | undefined; - const review = review_check_result as { approved?: boolean; round?: number; detail?: string } | undefined; + const baseline = baseline_result as { has_target?: boolean } | undefined; + const review = review_check_result as { round?: number } | undefined; // The order mirrors the edge conditions: no target, then the spent // gate budget, then the unapproved review. - const [cause, evidence] = baseline?.has_target === false - ? ['the finding it names is no longer in the tree, so there is nothing to fix', String(baseline.detail ?? '')] + const cause = baseline?.has_target === false + ? 'the finding it names is no longer in the tree' : (judge_result?.attempts ?? 0) >= 3 - ? ['three consecutive gate failures without green checks', String((checks_result as { output?: unknown } | undefined)?.output ?? '')] - : [`the reviewer never approved after ${String(review?.round ?? 0)} round(s)`, String(review?.detail ?? '')]; - const result = await flagNeedsHuman(repoRoot, issue, [ - `issue-fix ended without a pull request — ${cause} — so this issue now carries the \`${ctx.labels.needsHuman}\` label and the picker skips it.`, - 'Remove the label to re-queue it, close the issue if it is already settled, or investigate the evidence below.', - '', - '```', - evidence.slice(-1_500), - '```', - ].join('\n'), { label: ctx.labels.needsHuman, ...auth }); - return { - flagged: result.flagged, - issue_number: issue, - detail: result.flagged ? `flagged #${issue}; ${result.detail}` : result.detail, - }; + ? 'three consecutive gate failures without green checks' + : `the reviewer never approved after ${String(review?.round ?? 0)} round(s)`; + return giveUpNeedsHuman(repoRoot, issue, 'issue-fix', cause, ctx.labels.needsHuman, auth); }, }); } diff --git a/ops/maintenance/src/pr-review/tools/post-review.ts b/ops/maintenance/src/pr-review/tools/post-review.ts index 7781f307..29d4712a 100644 --- a/ops/maintenance/src/pr-review/tools/post-review.ts +++ b/ops/maintenance/src/pr-review/tools/post-review.ts @@ -288,10 +288,12 @@ export function postReviewTool(c: ReviewContext) { // 4. Reconcile the needs-human label with the outcome. A failed // submission leaves the same visible trace, best effort — the comment // rides a different endpoint, so one failing does not imply the other. + // The failure detail stays in the tool result: GitHub's error text + // is not fit for a public PR. if (!submission.ok) { await setPrLabels(repoRoot, p.pr, { add: [ctx.labels.needsHuman] }, auth); await commentOnPr(repoRoot, p.pr, - `${reviewMarker('notice', provenance)}\nThe review was written but could not be submitted (${submission.detail}); this PR now carries the \`needs-human\` label. Re-run the PR review workflow, or review by hand — a later successful review clears the label.`, + `${reviewMarker('notice', provenance)}\nSomething went wrong while submitting the automated review, so this PR now carries the \`needs-human\` label. Re-run the PR review workflow, or review by hand — a later successful review clears the label.`, auth); } else if (capReached) { await setPrLabels(repoRoot, p.pr, { add: [ctx.labels.needsHuman] }, auth); diff --git a/ops/maintenance/src/shared/repo.ts b/ops/maintenance/src/shared/repo.ts index 50670cf5..c76bc27a 100644 --- a/ops/maintenance/src/shared/repo.ts +++ b/ops/maintenance/src/shared/repo.ts @@ -266,10 +266,24 @@ export async function flagNeedsHuman( return { flagged: true, detail: comment.detail }; } +/** + * The comment a flagged issue receives. It is deliberately generic: a + * give-up cause, check output, or error message can carry paths, tokens, + * or environment details, so the specifics stay in the run's own log and + * never reach the public issue. + */ +export function needsHumanNotice(workflow: string, needsHumanLabel: string): string { + return [ + `I ran into a problem while working on this issue.`, + `I've added the \`${needsHumanLabel}\` label for your attention.`, + ].join('\n'); +} + /** * The give-up node's action: flag the picked issue waiting on a human - * because the run ended without a pull request, naming which dead end - * (`cause`) sent it here so the comment is actionable. The result carries + * because the run ended without a pull request. The posted comment is + * {@link needsHumanNotice}; `cause` names which dead end sent the run + * here and is reported only in the returned `detail`. The result carries * `flagged`, which `run.ts` reads into the run's `gave_up` field, so a * tried-and-abandoned run reports distinctly from a nothing-to-do one. */ @@ -280,11 +294,17 @@ export async function giveUpNeedsHuman( cause: string, needsHumanLabel: string, options: { token?: string; ops?: FlagNeedsHumanOps } = {}, -): Promise<{ flagged: boolean; detail: string }> { - return flagNeedsHuman(repoRoot, issueNumber, [ - `${workflow} ended without a pull request — ${cause} — so this issue now carries the \`${needsHumanLabel}\` label and the picker skips it.`, - 'Remove the label to re-queue it, close the issue if it is already settled, or investigate.', - ].join('\n'), { label: needsHumanLabel, ...(options.token !== undefined ? { token: options.token } : {}), ...(options.ops !== undefined ? { ops: options.ops } : {}) }); +): Promise<{ flagged: boolean; issue_number: number; detail: string }> { + const result = await flagNeedsHuman(repoRoot, issueNumber, needsHumanNotice(workflow, needsHumanLabel), { + label: needsHumanLabel, + ...(options.token !== undefined ? { token: options.token } : {}), + ...(options.ops !== undefined ? { ops: options.ops } : {}), + }); + return { + flagged: result.flagged, + issue_number: issueNumber, + detail: result.flagged ? `flagged #${issueNumber} (${cause}); ${result.detail}` : `${cause}; ${result.detail}`, + }; } /** @@ -292,7 +312,8 @@ export async function giveUpNeedsHuman( * after an engine-level death (a budget breach) that bypasses the * give-up node — without it, the issue stays approved-but-orphaned, its * label event already spent, so nothing re-queues it. `getPicked` reads - * the issue held in the build's closure. + * the issue held in the build's closure. The error is reported in the + * returned line only; the issue gets {@link needsHumanNotice}. */ export function fatalNeedsHuman( repoRoot: string, @@ -300,18 +321,20 @@ export function fatalNeedsHuman( workflow: string, needsHumanLabel: string, token?: string, + ops?: FlagNeedsHumanOps, ): (error: unknown) => Promise { return async (error) => { const issue = getPicked(); if (issue === undefined) return undefined; + const reason = error instanceof Error ? error.message : String(error); try { - const reason = error instanceof Error ? error.message : String(error); - const result = await flagNeedsHuman(repoRoot, issue, [ - `${workflow} died before finishing (${reason}), so this issue now carries the \`${needsHumanLabel}\` label and the picker skips it.`, - 'Remove the label to re-queue it, or dispatch the workflow naming this issue to override the skip.', - ].join('\n'), { label: needsHumanLabel, ...(token !== undefined ? { token } : {}) }); + const result = await flagNeedsHuman(repoRoot, issue, needsHumanNotice(workflow, needsHumanLabel), { + label: needsHumanLabel, + ...(token !== undefined ? { token } : {}), + ...(ops !== undefined ? { ops } : {}), + }); return result.flagged - ? `fatal-run cleanup: #${issue} flagged ${needsHumanLabel}` + ? `fatal-run cleanup: #${issue} flagged ${needsHumanLabel} after: ${reason}` : `fatal-run cleanup on #${issue}: ${result.detail}`; } catch (cleanupError) { return `fatal-run cleanup failed on #${issue}: ${cleanupError instanceof Error ? cleanupError.message : String(cleanupError)}`; diff --git a/ops/maintenance/test/repo.test.ts b/ops/maintenance/test/repo.test.ts index 12638687..d80a5823 100644 --- a/ops/maintenance/test/repo.test.ts +++ b/ops/maintenance/test/repo.test.ts @@ -12,7 +12,7 @@ import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; import { promisify } from 'node:util'; import { RUNTIME_CONFIG_ENV_VARS } from '@cycgraph/orchestrator/internal'; -import { MAINTENANCE_SECRET_ENV_VARS, NEEDS_HUMAN_LABEL, WORKFLOW_MENTION, checksEnv, flagNeedsHuman, giveUpNeedsHuman, maintenanceRunEnv, maintenanceSecrets, repoMap, stripMentions } from '../src/shared/repo.js'; +import { MAINTENANCE_SECRET_ENV_VARS, NEEDS_HUMAN_LABEL, WORKFLOW_MENTION, checksEnv, fatalNeedsHuman, flagNeedsHuman, giveUpNeedsHuman, maintenanceRunEnv, maintenanceSecrets, needsHumanNotice, repoMap, stripMentions } from '../src/shared/repo.js'; const exec = promisify(execFile); @@ -393,13 +393,36 @@ describe('flagNeedsHuman', () => { expect(calls.label).toEqual(['blocked']); }); - it('giveUpNeedsHuman flags the issue with a cause-bearing give-up comment', async () => { + it('giveUpNeedsHuman comments a generic notice and keeps the cause out of it', async () => { const { ops, calls } = fakeOps(true); + const CAUSE = 'the reviewer never approved after 3 round(s)'; - const result = await giveUpNeedsHuman('/repo', 7, 'implement-ticket', 'the reviewer never approved after 3 round(s)', 'needs-human', { ops }); + const result = await giveUpNeedsHuman('/repo', 7, 'implement-ticket', CAUSE, 'needs-human', { ops }); - expect(result.flagged).toBe(true); expect(calls.label).toEqual(['needs-human']); - expect(calls.comment[0]).toContain('implement-ticket ended without a pull request — the reviewer never approved after 3 round(s)'); + expect(calls.comment).toEqual([needsHumanNotice('implement-ticket', 'needs-human')]); + expect(calls.comment[0]).not.toContain(CAUSE); + expect(result).toEqual({ flagged: true, issue_number: 7, detail: `flagged #7 (${CAUSE}); commented` }); + }); + + it('fatalNeedsHuman comments a generic notice and reports the error only in its result', async () => { + const { ops, calls } = fakeOps(true); + const SECRET_ERROR = new Error('token ghp_abc123 rejected at /home/runner/work/repo'); + + const line = await fatalNeedsHuman('/repo', () => 7, 'issue-fix', 'needs-human', undefined, ops)(SECRET_ERROR); + + expect(calls.comment).toEqual([needsHumanNotice('issue-fix', 'needs-human')]); + expect(calls.comment[0]).not.toContain('ghp_abc123'); + expect(line).toBe(`fatal-run cleanup: #7 flagged needs-human after: ${SECRET_ERROR.message}`); + }); + + it('fatalNeedsHuman does nothing when no issue was picked', async () => { + const { ops, calls } = fakeOps(true); + + const line = await fatalNeedsHuman('/repo', () => undefined, 'issue-fix', 'needs-human', undefined, ops)(new Error('boom')); + + expect(line).toBeUndefined(); + expect(calls.label).toEqual([]); + expect(calls.comment).toEqual([]); }); });