fix: clear up maint error messages - #401
Conversation
wmcmahan
left a comment
wmcmahan
left a comment
There was a problem hiding this comment.
Advisory review by the pr-review workflow — the human merge decision stands either way.
VERDICT: REVISE
The PR removes causes, check output and error text from public comments without leaking them elsewhere, but the new issue notice ignores its workflow argument and no longer tells readers how to re-queue.
Good work otherwise: submission.detail and the fatal error still reach the tool result, so the run log keeps them, and the new tests use the real fatalNeedsHuman and giveUpNeedsHuman with exact toEqual and toBe checks.
2 findings are posted as comments on the diff.
Reviewed at 7ede10d · with claude-opus-5-5 · workflow run
| * 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 { |
There was a problem hiding this comment.
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.
| */ | ||
| export function needsHumanNotice(workflow: string, needsHumanLabel: string): string { | ||
| return [ | ||
| `I ran into a problem while working on this issue.`, |
There was a problem hiding this comment.
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.
This branch was successfully deployed
1 participant
No description provided.