Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/pr-ci-failure.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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.

<!-- ci-gate:$SUITE_SHA -->"
fi
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/pr-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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 "<!-- cycgraph:pr-notice -->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 "<!-- cycgraph:pr-notice -->Something went wrong during the automated review, so this PR now carries the needs-human label. A later successful review clears it."
6 changes: 3 additions & 3 deletions .github/workflows/pr-revise.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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 "<!-- cycgraph:pr-notice -->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 "<!-- cycgraph:pr-notice -->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."
2 changes: 1 addition & 1 deletion .github/workflows/reconcile-pipeline.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
13 changes: 9 additions & 4 deletions ops/maintenance/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
19 changes: 2 additions & 17 deletions ops/maintenance/src/issue-fix/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -78,22 +78,7 @@ export function issueFix(): MaintenanceWorkflow<typeof params> {
// 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),
};
},

Expand Down
36 changes: 12 additions & 24 deletions ops/maintenance/src/issue-fix/tools/give-up.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,17 @@
* 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
*/

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. */
Expand All @@ -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);
},
});
}
4 changes: 3 additions & 1 deletion ops/maintenance/src/pr-review/tools/post-review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
51 changes: 37 additions & 14 deletions ops/maintenance/src/shared/repo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

The exported notice never uses its workflow parameter. So giveUpNeedsHuman, fatalNeedsHuman and all six callers pass a workflow name that has no effect. Either put the workflow name into the text (for example "issue-fix ran into a problem..."), or remove the parameter all the way up the chain.

Evidence

ESLint only covers packages/** (eslint.config.mjs), and no tsconfig sets noUnusedParameters, so lint and the compiler won't catch this.
Callers: issue-fix/index.ts:81, implement-ticket/index.ts:76, optimization-apply/index.ts:69, and the three give-up.ts tools.

return [
`I ran into a problem while working on this issue.`,

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

This notice drops the only recovery instruction the reader had, "Remove the label to re-queue it." Every other notice in this PR keeps its "...clears it" sentence, and an issue is the one case where a person has to act to recover. Add the re-queue sentence back. Also switch to the "Something went wrong..." wording the other notices use, instead of the first person.

Evidence

The README (ops/maintenance/README.md) still says "removing the label re-queues one". The old issue comments told readers to remove the label, and to dispatch the workflow to override the skip.

`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.
*/
Expand All @@ -280,38 +294,47 @@ 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}`,
};
}

/**
* An `onFatal` cleanup that flags the picked issue waiting on a human
* 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,
getPicked: () => number | undefined,
workflow: string,
needsHumanLabel: string,
token?: string,
ops?: FlagNeedsHumanOps,
): (error: unknown) => Promise<string | undefined> {
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)}`;
Expand Down
33 changes: 28 additions & 5 deletions ops/maintenance/test/repo.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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([]);
});
});
Loading