diff --git a/.github/extensions/open-pr-board/README.md b/.github/extensions/open-pr-board/README.md index bf663baf7..d8be29276 100644 --- a/.github/extensions/open-pr-board/README.md +++ b/.github/extensions/open-pr-board/README.md @@ -117,6 +117,14 @@ action writes to GitHub. Review results require the exact queue token from the current review request. Results arriving after a manual move, reset, or replacement review are rejected. +`suggestedComment` is public feedback ready to post verbatim as the maintainer +addressing the PR author, with concrete evidence and actionable changes or a clear +decision. Maintainer-only advice stays in internal fields such as `rationale`, +`risks`, and `checkNext`; board terminology and claims of completed GitHub actions +do not belong in the comment. Uncertainty must remain explicit. If there is no +useful author-facing feedback, the field is omitted or empty. Existing stored +comments are not rewritten; re-review a PR to generate a new suggestion. + ## Regression tests Run `node --experimental-vm-modules --test eng/board-periodic-refresh.test.mjs` to check diff --git a/.github/extensions/open-pr-board/lib/review-prompt.mjs b/.github/extensions/open-pr-board/lib/review-prompt.mjs index 075846b7a..8d52db696 100644 --- a/.github/extensions/open-pr-board/lib/review-prompt.mjs +++ b/.github/extensions/open-pr-board/lib/review-prompt.mjs @@ -5,6 +5,16 @@ export const RECOMMENDATION_GUIDE = [ "`close` — the submission does not belong in this repository and no reasonable revision would change that (including vendor funnels and non-developer domains).", ].join("\n"); +const PUBLIC_COMMENT_GUIDE = [ + "`suggestedComment` is public text ready to post verbatim as the maintainer addressing the PR author directly, in a natural maintainer voice, not an agent report to the maintainer.", + "State concrete evidence (files, lines, or links where available) and actionable changes the author can make, or a clear evidence-backed decision about the contribution.", + "Keep maintainer-only advice, verification tasks, and uncertainty about how to act in internal fields such as `rationale`, `risks`, and `checkNext`, not in the public comment.", + "Do not tell the maintainer what to do: avoid phrases such as 'I recommend closing this submission', 'Have a maintainer confirm...', or 'Confirm the required approval'. Do not mention board buckets, queue tokens, internal calibration, or internal recommendation labels.", + "Preserve uncertainty: distinguish observed facts from unresolved questions, ask the author only for information they can reasonably provide, and do not turn a tentative internal recommendation into a settled public decision.", + "Do not invent claims of completed remote actions (for example, 'I closed this PR', 'approved', or 'enabled auto-merge'); recording a review performs none of them.", + "If there is no useful author-facing feedback, omit `suggestedComment` or use an empty string; do not manufacture a comment merely to fill the field.", +].join("\n"); + /** Shared schema for a single recorded review. Reused directly as the `record_review` input schema. */ export const REVIEW_FIELDS_SCHEMA = { type: "object", @@ -21,7 +31,7 @@ export const REVIEW_FIELDS_SCHEMA = { enum: ["close", "request-changes", "needs-insight", "approve"], description: "Which bucket the pull request belongs in.", }, - rationale: { type: "string", description: "One sentence explaining the deciding factor, shown on the card." }, + rationale: { type: "string", description: "Internal: one sentence explaining the deciding factor, shown on the card." }, summary: { type: "string", description: "What the pull request actually changes." }, repoFit: { type: "string", description: "Repository fit and meaningful value for Copilot users." }, compliance: { type: "string", description: "Deterministic checks: frontmatter, naming, README regeneration, manifests." }, @@ -30,11 +40,11 @@ export const REVIEW_FIELDS_SCHEMA = { description: "Knowledge-uplift verdict (high / partial / none) with the specific content a capable model would not produce unprompted, plus overlap with existing resources.", }, - risks: { type: "string", description: "Correctness, security, licensing, or harmful-content concerns." }, + risks: { type: "string", description: "Internal: correctness, security, licensing, or harmful-content concerns." }, validation: { type: "string", description: "Evidence the contributor tested or validated the change." }, confidence: { type: "string", enum: ["low", "medium", "high"], description: "Confidence in the recommendation." }, - checkNext: { type: "string", description: "What a maintainer should verify by hand before acting." }, - suggestedComment: { type: "string", description: "A suggested review comment, ready to edit and post." }, + checkNext: { type: "string", description: "Internal: what a maintainer should verify by hand before acting; not instructions for the PR author." }, + suggestedComment: { type: "string", description: PUBLIC_COMMENT_GUIDE }, }, additionalProperties: false, }; @@ -95,7 +105,7 @@ function recordingRules(instanceId) { return [ `Record every result by calling \`invoke_canvas_action\` with instanceId \`${instanceId}\`, actionName \`record_review\`, and input \`{ "reviews": [ ... ] }\`.`, "Each review object needs `number`, `queueId`, `recommendation`, and `rationale`; fill in `summary`, `repoFit`, `compliance`, `differentiation`, `risks`, `validation`, `confidence`, `checkNext`, and `suggestedComment` whenever you have something real to say.", - "`suggestedComment` should be the comment you would leave on the pull request — specific, actionable, and written for the author.", + PUBLIC_COMMENT_GUIDE, "You may record in batches as you go. If the canvas is closed, re-open it first with `open_canvas` using canvasId `open-pr-board` and that same instanceId.", "Check `gh pr view --json state` before recording: closed or merged PRs are dropped from the board and `record_review` skips them as \"not on the board\". A \"stale review result\" skip means the queueId was already consumed; call `get_pr` to see what is stored rather than re-recording.", "", diff --git a/.github/extensions/open-pr-board/review-guidance.md b/.github/extensions/open-pr-board/review-guidance.md index 5b2529754..7d74a8d7b 100644 --- a/.github/extensions/open-pr-board/review-guidance.md +++ b/.github/extensions/open-pr-board/review-guidance.md @@ -95,15 +95,38 @@ For every pull request: SKILL.md criteria — fill in the ones that mattered, leave the rest out. The exception is `differentiation`: always fill it for content PRs, leading with the knowledge-uplift classification and its evidence. -- `checkNext` is for the maintainer: the one thing to verify before acting. -- `suggestedComment` is prefilled into the board's comment box, so write it as a comment - addressed to the contributor. Be specific, name files and line numbers, and keep the - tone of an ordinary maintainer review. +- `rationale`, `risks`, and `checkNext` are internal review fields. Keep advice about + what the maintainer should do or verify there; `checkNext` is the next verification + before acting. +- `suggestedComment` is public text prefilled into the board's comment box and may be + posted verbatim. Write as the maintainer addressing the PR author directly, in a + natural maintainer voice, not as an agent reporting to the maintainer. State concrete + evidence (files, lines, or links where available) and actionable changes the author + can make, or a clear evidence-backed decision about the contribution. +- Do not put agent-to-maintainer instructions in the public comment, such as + "I recommend closing this submission", "Have a maintainer confirm...", or + "Confirm the required approval". Do not mention board buckets, queue tokens, + internal calibration, or internal recommendation labels. +- Preserve uncertainty: separate observed facts from unresolved questions. Ask the + author only for information they can reasonably provide. A tentative recommendation + is not a settled public decision; a maintainer-only judgement belongs in `checkNext`. + If there is no useful author-facing feedback, omit `suggestedComment` or use an + empty string rather than manufacturing a comment. +- Do not claim completed remote actions such as closing, approving, or enabling + auto-merge. Recording a review performs none of those actions. - `confidence` is `low`, `medium`, or `high`. Use `low` freely; it is a signal, not a failure. - On a re-review, maintainer free-text guidance is a strong prior. Verify it against the evidence and cite what confirms or contradicts it; it does not replace the evidence. +For example (only when supported by the actual evidence): + +| Internal advice | Post-ready `suggestedComment` | +| --------------- | ----------------------------- | +| `rationale`: Recommend closing because this is an off-topic marketing workflow. | We cannot accept this contribution because its ad-campaign workflow is outside this repository's focus on GitHub Copilot development workflows. | +| `checkNext`: Confirm whether the author tested in Copilot. | The validation notes describe only a Claude Code run. Please test this skill in GitHub Copilot and include the steps and observed result. | +| `checkNext`: Have a maintainer resolve the policy question before deciding. | Omit the field or use `""` if there is no useful question or change for the author. | + ## Recording mechanics - Check `gh pr view --json state` before recording. Closed or merged PRs drop diff --git a/eng/open-pr-board.test.mjs b/eng/open-pr-board.test.mjs index 8dc283e59..4a7685e93 100644 --- a/eng/open-pr-board.test.mjs +++ b/eng/open-pr-board.test.mjs @@ -323,7 +323,7 @@ test("a head change after approval reports partial success instead of retrying t test("review schema requires a nonempty queue token and a one-sentence rationale", () => { assert.ok(REVIEW_FIELDS_SCHEMA.required.includes("queueId")); assert.equal(REVIEW_FIELDS_SCHEMA.properties.queueId.minLength, 1); - assert.match(REVIEW_FIELDS_SCHEMA.properties.rationale.description, /One sentence/); + assert.match(REVIEW_FIELDS_SCHEMA.properties.rationale.description, /one sentence/i); }); test("only the current token records a review; moves, rereviews, and reset invalidate old results", async (t) => { @@ -729,6 +729,50 @@ const promptItem = { }; const promptArgs = { instanceId: "board-1", repo: "example/repo", guidancePath: "guidance.md" }; +test("all public-comment prompt surfaces separate author feedback from internal maintainer advice", () => { + const surfaces = [ + ["record_review schema", REVIEW_FIELDS_SCHEMA.properties.suggestedComment.description], + ["initial review", buildReviewPrompt({ + ...promptArgs, items: [promptItem], + decisions: [{ number: 2, kind: "close", comment: "Internal calibration context" }], + }).split("--- Agent brief ---")[1]], + ["re-review", buildRereviewPrompt({ + ...promptArgs, item: promptItem, + guidance: "Have a maintainer confirm the required approval.", + previousReview: { recommendation: "needs-insight", rationale: "Unresolved policy", reviewedAt: "2026-10-01" }, + }).split("--- Agent brief ---")[1]], + ]; + for (const [name, text] of surfaces) { + assert.ok(text, name); + for (const requirement of [ + /ready to post verbatim as the maintainer addressing the PR author directly/, + /natural maintainer voice, not an agent report to the maintainer/, + /concrete evidence \(files, lines, or links where available\) and actionable changes/, + /clear evidence-backed decision/, + /internal fields such as `rationale`, `risks`, and `checkNext`, not in the public comment/, + /Do not tell the maintainer what to do/, + /I recommend closing this submission/, + /Have a maintainer confirm/, + /Confirm the required approval/, + /Do not mention board buckets, queue tokens, internal calibration, or internal recommendation labels/, + /Preserve uncertainty: distinguish observed facts from unresolved questions/, + /ask the author only for information they can reasonably provide/, + /do not turn a tentative internal recommendation into a settled public decision/, + /Do not invent claims of completed remote actions/, + /recording a review performs none of them/, + /omit `suggestedComment` or use an empty string/, + /do not manufacture a comment/, + ]) { + assert.match(text, requirement, `${name}: ${requirement}`); + } + } + for (const field of ["rationale", "risks", "checkNext"]) { + assert.match(REVIEW_FIELDS_SCHEMA.properties[field].description, /^Internal:/, field); + } + assert.ok(!REVIEW_FIELDS_SCHEMA.required.includes("suggestedComment"), "No public comment is required"); + assert.equal(REVIEW_FIELDS_SCHEMA.properties.suggestedComment.minLength, undefined, "Empty comments remain valid"); +}); + test("initial and re-review agent briefs accept valid YAML scalar styles while preserving metadata checks", () => { const prompts = [ buildReviewPrompt({ ...promptArgs, items: [promptItem], decisions: [] }),