From 4cb2664d9b98ccb628b1912ab537512556672f66 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Thu, 3 Sep 2026 03:04:10 +0000 Subject: [PATCH 1/3] render pr-reviewer stage 3 output as structured markdown instead of one prose blob MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stage 3 returned a single `summary` string that the coordinator posted verbatim as the review body, so a thorough review arrived as one unbroken paragraph mixing the verdict, the test evidence, every verified claim, and the blocking problem — unreadable on the PR page (see #5929). The stage now returns the review as separate fields (summary, scope, testEvidence, verified, concerns) and findings gain a title and an optional one-line suggestion. `server/lib/prReviewReport.js` renders those into the markdown a human reads: verdict banner, scope line, a blocking/non-blocking index anchored to path:line, test-evidence bullets with pass/fail/not-run icons, notes, and a collapsed verified-claims list. Inline comments get a blocking label, their title, and a GitHub ```suggestion block when the fix is one line. Rendering stays on the deterministic side of the model boundary: the model emits plain prose only, single-line fields collapse whitespace so a field cannot inject headings, a fenced suggestion is dropped rather than allowed to break out of the code block, and `reviewReportText` feeds every new field to the existing model-abuse scan. The body budget drops whole low-priority sections rather than truncating mid-sentence. A plain-string `summary` still renders, so an older stage body degrades to the previous single paragraph. --- server/lib/README.md | 1 + server/lib/index.js | 1 + server/lib/prReviewReport.js | 184 ++++++++++++++++++ server/lib/prReviewReport.test.js | 141 ++++++++++++++ server/services/issueWatcher.js | 63 ++++-- server/services/issueWatcher.test.js | 7 +- .../integrity.snapshot.json | 2 +- server/services/taskPromptDefaults/prompts.js | 56 ++++-- 8 files changed, 424 insertions(+), 31 deletions(-) create mode 100644 server/lib/prReviewReport.js create mode 100644 server/lib/prReviewReport.test.js diff --git a/server/lib/README.md b/server/lib/README.md index f2cbb3f35e..71544a3a23 100644 --- a/server/lib/README.md +++ b/server/lib/README.md @@ -474,6 +474,7 @@ pm` default, `NPM_CONFIG_PREFIX`, nvm/Volta) installed `codex` successfully and | `apiToolResource.js` | Builds the minimized semantic tool resource served at `/api/api-docs/tools.min.json` — only `x-portos-tool`-annotated operations, flattened to provider-neutral tool records with an HTTP binding and a shared error vocabulary. | | `asyncApiSpec.js` | Builds the AsyncAPI 3 Socket.IO document from the generated event catalog with direction-aware operations and explicit modeled/generated payload status. | | `prDisposition.js` | `resolvePrCompletion(metadata)` resolves the explicit `review-then-merge` / `merge-on-green` / `leave-open` policy, with legacy `reviewLoop` fallback; `leavesPrForHuman(task)` + `PR_STAYS_OPEN_TASK_TYPES` keep JIRA hand-offs open. Shared by the agent prompt builder and `agentWorktreeCleanup` so both halves agree. `resolvePrCreation({taskOpenPR, agentOwnsPr, prClaimVerified})` → a `PR_CREATION` tri-state (`never` / `if-missing` / `always`) naming who opens the change request for a completing worktree agent, so the runner, TUI, and direct-CLI completion paths cannot drift into double-firing `gh pr create`. | +| `prReviewReport.js` | Renders one structured pr-reviewer/issue-watcher PR decision into the markdown a human reads on the PR page — verdict banner, scope line, blocking/non-blocking finding index anchored to `path:line`, test-evidence bullets, notes, and a collapsed verified-claims list — plus `renderFindingBody` for an inline comment (label, title, body, optional ```suggestion``` block) and `reviewReportText` so the model-abuse scan still sees every model-authored string. Bounded by `MAX_REVIEW_BODY_CHARS`, dropping whole low-priority sections instead of truncating mid-sentence. Pure. | | `repoStateExpectations.js` | Post-completion repo-state audit, pure half. `resolveRepoStateExpectation({...})` answers whether one finished worktree agent should be audited — naming every not-audited path via `REPO_STATE_SKIPS` (a failed run is preserved for its retry; a review-loop follow-up or pr-watcher pending merge still owns the branch) — and returns just `staysOpen` + `prExpected`, from which `classifyRepoStateIssues(expectation, observed)` derives every check into `REPO_STATE_ISSUES` codes. Observations are tri-state: `null` ("could not ask") never produces an issue. `repoStateVerificationEnabled(app)` reads the per-app `verifyRepoStateOnCompletion` switch (unset = on). Probing + remediation live in `services/agentRepoStateVerification.js`. | | `shellCd.js` | `buildCdCommand(path, shell)` + `formatShellCommandLine(command, args, shell)` + `detectShellFlavor(shell, platform?)` + `quoteForShell(value, flavor, position?)` — build `cd` and arbitrary command-token lines for the shell a PTY session is ACTUALLY running (`cmd.exe` → Windows-escaped double quotes, PowerShell → doubled single quotes with `&` for a quoted command token, everything else → POSIX `shellQuote`). The Shell page's "cd to app" picker used to hard-code the POSIX form, which on Windows both mis-quoted the path and silently refused to cross drives (a bare `cd` does not switch drive). Flavor comes from the shell binary name, not the platform, so git-bash on Windows still gets POSIX quoting. `formatShellCommandLine` joins a command + argv into one quoted line and is shared by `agentTuiSpawning.js#buildTuiSpawnConfig` and the AI Providers page's "Launch in Shell" deep link, so a hand-launched TUI provider is quoted exactly the way the CoS runner would quote it. Renders the LINE only; the Enter byte that submits it is `SUBMIT_KEY` in `tuiHandshake.js`. | | `shellExit.js` | `buildRunThenExitCommand(commandLine, shell)` — "run this CLI, then close the shell with its status", in the dialect the session speaks. An agent TUI shell exists only to host one CLI and must die with it. The POSIX `cmd; exit $?` was applied everywhere and is actively wrong off-POSIX: in PowerShell `$?` is a BOOLEAN, so `exit $?` reports success as 1 and failure as 0 (inverted) — use `$LASTEXITCODE`, pre-seeded to 1 so a command that never ran still exits non-zero; in cmd.exe `;` is an argument separator, so the CLI is handed `;`/`exit`/`$?` as arguments — use `& exit`. Verified against node-pty on Windows 11 for pwsh 7, PowerShell 5.1 and cmd.exe. | diff --git a/server/lib/index.js b/server/lib/index.js index 33359dbfa2..9094b0b56f 100644 --- a/server/lib/index.js +++ b/server/lib/index.js @@ -451,6 +451,7 @@ export * from './openapiSpec.js'; export * from './openapiDowngrade.js'; export * from './apiToolResource.js'; export * from './prDisposition.js'; +export * from './prReviewReport.js'; export * from './repoStateExpectations.js'; export * from './shellCd.js'; export * from './shellExit.js'; diff --git a/server/lib/prReviewReport.js b/server/lib/prReviewReport.js new file mode 100644 index 0000000000..40aa6afde4 --- /dev/null +++ b/server/lib/prReviewReport.js @@ -0,0 +1,184 @@ +/** + * Structured pr-reviewer report → GitHub markdown. + * + * Stage 3 of the pr-reviewer pipeline used to hand the coordinator one + * `summary` string, which was posted verbatim as the review body: a single + * unbroken wall of prose mixing the verdict, the test evidence, every verified + * claim, and the blocking problem. Reviews are read by a human on the PR page, + * so the model now returns those as separate fields and the deterministic + * coordinator renders the markdown. The model never composes markup, which + * keeps rendering (and its length budget) on the trusted side of the boundary. + * + * Every field is optional and a plain-string `summary` still renders, so an + * older stage body — or a model that ignores the structured shape — degrades to + * the previous single-paragraph review rather than losing its review entirely. + * + * Pure: no I/O, no forge calls. + */ + +/** GitHub accepts far more, but a review a human can scan stays bounded. */ +export const MAX_REVIEW_BODY_CHARS = 8_000; +const MAX_SUMMARY_CHARS = 2_000; +const MAX_SCOPE_CHARS = 400; +const MAX_BULLET_CHARS = 600; +const MAX_BULLETS = 12; +const MAX_FINDING_TITLE_CHARS = 160; +const MAX_FINDING_BODY_CHARS = 3_000; +const MAX_SUGGESTION_CHARS = 2_000; +const TEST_STATUSES = ['pass', 'fail', 'not-run']; +const STATUS_ICON = { pass: '✅', fail: '❌', 'not-run': '⏭️' }; +const TRIM_NOTE = '_Some sections of this review were omitted to stay within the comment size limit._'; + +const VERDICT_BANNER = { + approve: '✅ **Approved**', + request_changes: '🔴 **Changes requested**', + defer: '💬 **Review — no verdict yet**', +}; + +/** Collapse whitespace runs so a model paragraph cannot inject list/heading markup mid-line. */ +function line(value, max) { + if (typeof value !== 'string') return ''; + return value.replace(/\s+/g, ' ').trim().slice(0, max); +} + +/** Keep author paragraph breaks, drop trailing whitespace and runaway blank lines. */ +function block(value, max) { + if (typeof value !== 'string') return ''; + return value.replace(/\r\n/g, '\n').replace(/[ \t]+$/gm, '').replace(/\n{3,}/g, '\n\n').trim().slice(0, max); +} + +function bullets(value) { + if (!Array.isArray(value)) return []; + return value.map((item) => line(item, MAX_BULLET_CHARS)).filter(Boolean).slice(0, MAX_BULLETS); +} + +function normalizeTestEvidence(value) { + if (!Array.isArray(value)) return []; + return value + .map((item) => { + const command = line(item?.command, MAX_BULLET_CHARS); + const detail = line(item?.detail, MAX_BULLET_CHARS); + if (!command && !detail) return null; + const status = TEST_STATUSES.includes(item?.status) ? item.status : 'not-run'; + return { command, status, detail }; + }) + .filter(Boolean) + .slice(0, MAX_BULLETS); +} + +/** + * Normalize the optional structured report fields of one stage-3 PR decision. + * Unknown/malformed entries drop out; the caller still owns verdict, findings, + * and every forge-state check. + */ +export function normalizeReviewReport(raw) { + return { + summary: block(raw?.summary, MAX_SUMMARY_CHARS), + scope: line(raw?.scope, MAX_SCOPE_CHARS), + testEvidence: normalizeTestEvidence(raw?.testEvidence), + verified: bullets(raw?.verified), + concerns: bullets(raw?.concerns), + }; +} + +/** Normalize the presentation fields of one inline finding (anchoring stays with the caller). */ +export function normalizeFindingPresentation(raw) { + const suggestion = block(raw?.suggestion, MAX_SUGGESTION_CHARS); + return { + title: line(raw?.title, MAX_FINDING_TITLE_CHARS), + body: block(raw?.body, MAX_FINDING_BODY_CHARS), + // A fenced block inside the suggestion would close GitHub's own fence and + // spill the rest of the comment into the diff as an applyable patch. + suggestion: suggestion.includes('```') ? '' : suggestion, + }; +} + +/** Every model-authored string in a report, for the model-abuse scan. */ +export function reviewReportText(raw) { + const report = normalizeReviewReport(raw); + return [ + report.summary, + report.scope, + ...report.testEvidence.flatMap((item) => [item.command, item.detail]), + ...report.verified, + ...report.concerns, + ].filter(Boolean); +} + +/** Markdown for one inline review comment. */ +export function renderFindingBody(finding, { blocking = true } = {}) { + const { title, body, suggestion } = normalizeFindingPresentation(finding); + const label = blocking ? '⛔ **Blocking**' : '💡 **Non-blocking**'; + return [ + title ? `${label} — ${title}` : label, + '', + body, + ...(suggestion ? ['', '```suggestion', suggestion, '```'] : []), + ].join('\n').trim(); +} + +function findingsSection(findings, heading, icon) { + if (findings.length === 0) return null; + return [ + `#### ${icon} ${heading} (${findings.length})`, + ...findings.map(({ comment, presentation }) => { + const label = presentation.title || line(presentation.body, 160); + return `- \`${comment.path}:${comment.line}\` — ${label}`; + }), + ].join('\n'); +} + +/** + * Render the review body a human reads on the PR page. Sections are emitted in + * priority order and dropped from the end once the budget is spent, so a long + * report loses its least important section instead of being cut mid-sentence. + */ +export function renderReviewBody({ + report, + verdict, + blockingFindings = [], + nonBlockingFindings = [], + appendix = '', +} = {}) { + const normalized = normalizeReviewReport(report); + const sections = [ + VERDICT_BANNER[verdict] || VERDICT_BANNER.defer, + normalized.summary || 'This change needs follow-up before it can merge.', + normalized.scope ? `**Scope:** ${normalized.scope}` : null, + findingsSection(blockingFindings, 'Blocking', '⛔'), + findingsSection(nonBlockingFindings, 'Non-blocking', '💡'), + normalized.testEvidence.length > 0 + ? ['#### Test evidence', ...normalized.testEvidence.map((item) => { + const icon = STATUS_ICON[item.status]; + const head = item.command ? `\`${item.command}\`` : item.detail; + const tail = item.command && item.detail ? ` — ${item.detail}` : ''; + return `- ${icon} ${head}${tail}`; + })].join('\n') + : null, + normalized.concerns.length > 0 + ? ['#### Notes', ...normalized.concerns.map((item) => `- ${item}`)].join('\n') + : null, + normalized.verified.length > 0 + ? ['
Claims verified against the code', '', + ...normalized.verified.map((item) => `- ${item}`), '
'].join('\n') + : null, + block(appendix, MAX_SUMMARY_CHARS) || null, + ].filter(Boolean); + + const kept = []; + // Reserve room for the trim note so adding it cannot push the body past the cap. + const budget = MAX_REVIEW_BODY_CHARS - TRIM_NOTE.length - 2; + let used = 0; + let dropped = false; + for (const section of sections) { + const cost = section.length + 2; + if (used + cost > budget) { + dropped = true; + continue; + } + kept.push(section); + used += cost; + } + if (dropped) kept.push(TRIM_NOTE); + return kept.join('\n\n'); +} diff --git a/server/lib/prReviewReport.test.js b/server/lib/prReviewReport.test.js new file mode 100644 index 0000000000..2e7687d95a --- /dev/null +++ b/server/lib/prReviewReport.test.js @@ -0,0 +1,141 @@ +import { describe, expect, it } from 'vitest'; +import { + MAX_REVIEW_BODY_CHARS, + normalizeReviewReport, + renderFindingBody, + renderReviewBody, + reviewReportText, +} from './prReviewReport.js'; + +const finding = (path, line, presentation) => ({ + comment: { path, line, side: 'RIGHT', body: 'rendered' }, + presentation: { title: '', body: '', suggestion: '', ...presentation }, +}); + +describe('renderReviewBody', () => { + it('renders a full structured report as scannable markdown sections', () => { + const body = renderReviewBody({ + verdict: 'request_changes', + report: { + summary: 'The docs are accurate, but the prescribed install command contradicts the repo install path.', + scope: 'docs-only change to two files under docs/', + testEvidence: [ + { command: 'npm test -w server', status: 'pass', detail: '412 passed' }, + { command: 'npx vitest', status: 'not-run', detail: 'no node_modules in the disposable worktree' }, + ], + verified: ['update.sh always ends on main — update.sh:132-143'], + concerns: ['The Windows entry point is not mentioned.'], + }, + blockingFindings: [finding('docs/SELF_UPDATE.md', 108, { title: 'Bare npm install dirties tracked lockfiles' })], + nonBlockingFindings: [finding('docs/SELF_UPDATE.md', 112, { title: 'Ordering rationale is downstream of line 108' })], + }); + + expect(body).toContain('🔴 **Changes requested**'); + expect(body).toContain('**Scope:** docs-only change to two files under docs/'); + expect(body).toContain('#### ⛔ Blocking (1)'); + expect(body).toContain('- `docs/SELF_UPDATE.md:108` — Bare npm install dirties tracked lockfiles'); + expect(body).toContain('#### 💡 Non-blocking (1)'); + expect(body).toContain('#### Test evidence'); + expect(body).toContain('- ✅ `npm test -w server` — 412 passed'); + expect(body).toContain('- ⏭️ `npx vitest` — no node_modules in the disposable worktree'); + expect(body).toContain('#### Notes'); + expect(body).toContain('
Claims verified against the code'); + }); + + it('still renders a legacy plain-string summary with no structured fields', () => { + const body = renderReviewBody({ verdict: 'approve', report: { summary: 'Looks good.' } }); + expect(body).toBe('✅ **Approved**\n\nLooks good.'); + }); + + it('falls back to a verdict banner and default sentence when the report is empty', () => { + expect(renderReviewBody({ verdict: 'defer', report: {} })) + .toContain('💬 **Review — no verdict yet**'); + expect(renderReviewBody({})).toContain('This change needs follow-up before it can merge.'); + }); + + it('appends the coordinator appendix as its own section', () => { + const body = renderReviewBody({ + verdict: 'request_changes', + report: { summary: 'Two problems.' }, + appendix: 'PortOS could not anchor one or more reported findings to this diff.', + }); + expect(body.endsWith('PortOS could not anchor one or more reported findings to this diff.')).toBe(true); + }); + + it('drops whole low-priority sections instead of truncating mid-sentence', () => { + const verified = Array.from({ length: 12 }, (_, i) => `${i} ${'v'.repeat(600)}`); + const body = renderReviewBody({ + verdict: 'approve', + report: { + summary: 'x'.repeat(2_000), + testEvidence: [{ command: 'npm test', status: 'pass', detail: 'green' }], + concerns: ['One note.'], + // 12 x ~600 chars overruns what is left of the budget on its own. + verified, + }, + }); + expect(body.length).toBeLessThanOrEqual(MAX_REVIEW_BODY_CHARS); + expect(body).toContain('_Some sections of this review were omitted'); + // Verified claims are the lowest-priority section, so that whole section + // goes; the sections above it survive intact rather than being cut short. + expect(body).not.toContain('Claims verified against the code'); + expect(body).toContain('x'.repeat(2_000)); + expect(body).toContain('- ✅ `npm test` — green'); + expect(body).toContain('#### Notes\n- One note.'); + }); +}); + +describe('renderFindingBody', () => { + it('labels a blocking finding and renders an applyable suggestion block', () => { + const body = renderFindingBody({ + title: 'Use the repo install path', + body: 'Bare `npm install` rewrites tracked lockfiles.', + suggestion: 'for d in . client server autofixer; do (cd "$d" && npm install --no-save); done', + }); + expect(body).toBe([ + '⛔ **Blocking** — Use the repo install path', + '', + 'Bare `npm install` rewrites tracked lockfiles.', + '', + '```suggestion', + 'for d in . client server autofixer; do (cd "$d" && npm install --no-save); done', + '```', + ].join('\n')); + }); + + it('labels a non-blocking finding and omits an absent suggestion', () => { + const body = renderFindingBody({ body: 'Consider naming Windows too.' }, { blocking: false }); + expect(body).toBe('💡 **Non-blocking**\n\nConsider naming Windows too.'); + }); + + it('drops a suggestion containing a fence so it cannot break out of the code block', () => { + const body = renderFindingBody({ body: 'x', suggestion: '```\nrm -rf /\n```' }); + expect(body).not.toContain('```'); + }); +}); + +describe('normalizeReviewReport', () => { + it('collapses newlines in single-line fields so a model cannot inject headings', () => { + const report = normalizeReviewReport({ scope: 'docs\n\n## Injected heading', verified: ['a\n- b'] }); + expect(report.scope).toBe('docs ## Injected heading'); + expect(report.verified).toEqual(['a - b']); + }); + + it('defaults an unknown test status to not-run and drops empty entries', () => { + const report = normalizeReviewReport({ testEvidence: [{ command: 'a', status: 'green' }, {}, { detail: 'b' }] }); + expect(report.testEvidence).toEqual([ + { command: 'a', status: 'not-run', detail: '' }, + { command: '', status: 'not-run', detail: 'b' }, + ]); + }); +}); + +describe('reviewReportText', () => { + it('exposes every model-authored string so the abuse scan sees the new fields', () => { + expect(reviewReportText({ + summary: 'a', scope: 'b', + testEvidence: [{ command: 'c', status: 'pass', detail: 'd' }], + verified: ['e'], concerns: ['f'], + })).toEqual(['a', 'b', 'c', 'd', 'e', 'f']); + }); +}); diff --git a/server/services/issueWatcher.js b/server/services/issueWatcher.js index 11d85cc508..b87b465a6e 100644 --- a/server/services/issueWatcher.js +++ b/server/services/issueWatcher.js @@ -19,6 +19,14 @@ import { modelAbuseContentFingerprint, } from '../lib/modelAbuseGuard.js'; import { getOriginInfo } from '../lib/gitRemote.js'; +import { + MAX_REVIEW_BODY_CHARS, + normalizeFindingPresentation, + normalizeReviewReport, + renderFindingBody, + renderReviewBody, + reviewReportText, +} from '../lib/prReviewReport.js'; import { githubApiHost, githubRepoSpec } from '../lib/workTracker.js'; import { getAppById, updateApp } from './apps.js'; import { execGh, ensureForgeReachable } from './github.js'; @@ -162,14 +170,16 @@ function normalizeFinding(finding, anchors) { const path = text(finding.path, 500); const side = String(finding.side || '').toUpperCase(); const line = Number(finding.line); - const body = text(finding.body, 3_000); - if (!path || side !== 'RIGHT' || !Number.isInteger(line) || line < 1 || !body) return null; + const presentation = normalizeFindingPresentation(finding); + if (!path || side !== 'RIGHT' || !Number.isInteger(line) || line < 1 || !presentation.body) return null; if (!anchors.has(`${path}\u0000${side}\u0000${line}`)) return null; + // A finding without an explicit boolean is blocking. The watcher must not + // turn an incomplete model response into an automatic merge. + const blocking = finding.blocking !== false; return { - comment: { path, side, line, body }, - // A finding without an explicit boolean is blocking. The watcher must not - // turn an incomplete model response into an automatic merge. - blocking: finding.blocking !== false, + comment: { path, side, line, body: renderFindingBody(finding, { blocking }) }, + presentation, + blocking, }; } @@ -184,7 +194,7 @@ function normalizeReviewDecision(value) { verdict, ciPolicy, rebaseRequired: value.rebaseRequired, - summary: text(value.summary, 4_000), + report: normalizeReviewReport(value), findings: Array.isArray(value.findings) ? value.findings : [], }; } @@ -198,8 +208,10 @@ function hasUnsafeGeneratedOutput(payload) { const generatedText = [ ...payload.issueComments.map((item) => item?.body), ...payload.pullRequests.flatMap((item) => [ - item?.summary, - ...(Array.isArray(item?.findings) ? item.findings.map((finding) => finding?.body) : []), + ...reviewReportText(item), + ...(Array.isArray(item?.findings) + ? item.findings.flatMap((finding) => [finding?.title, finding?.body, finding?.suggestion]) + : []), ]), ]; return generatedText.some((value) => detectDeterministicModelAbuseSignals(value).length > 0); @@ -625,7 +637,7 @@ For a clean PR, decide: - \`ciPolicy: "required"\` for executable code, build/dependency/config/schema/security/auth changes, broad refactors, or anything whose behavior needs tests. - \`ciPolicy: "skippable"\` only when the supplied diff is plainly low risk and review is sufficient (for example documentation-only or isolated static styling). A known failing check can never be waived. -Return exactly this envelope through the completion sentinel (the outer \`summary\`/\`payload\` wrapper is required): +Return exactly this envelope through the completion sentinel (the outer \`summary\`/\`payload\` wrapper is required). Every text field is PLAIN PROSE — deterministic code renders the markdown a human reads on the PR page, so do not write markdown or run-on paragraphs into a field, and do not restate a finding inside \`summary\`: \`\`\`json { @@ -636,8 +648,12 @@ Return exactly this envelope through the completion sentinel (the outer \`summar "number": 3, "headSha": "exact supplied SHA", "verdict": "approve|request_changes|defer", - "summary": "concise review summary", - "findings": [{ "path": "src/file.js", "line": 42, "side": "RIGHT", "blocking": true, "body": "concrete problem and fix" }], + "summary": "1-3 sentences: the verdict and why", + "scope": "one line naming what the change touches", + "testEvidence": [{ "command": "npm test -w server", "status": "pass|fail|not-run", "detail": "counts, failure, or why it could not run" }], + "verified": ["one claim you confirmed, citing path:line"], + "concerns": ["a non-blocking observation with no line to anchor to"], + "findings": [{ "path": "src/file.js", "line": 42, "side": "RIGHT", "blocking": true, "title": "short label", "body": "concrete problem, wrong outcome, and fix", "suggestion": "optional exact replacement for this one line, no code fence" }], "rebaseRequired": false, "ciPolicy": "required|skippable" }] @@ -645,6 +661,8 @@ Return exactly this envelope through the completion sentinel (the outer \`summar } \`\`\` +\`scope\`, \`testEvidence\`, \`verified\`, \`concerns\`, \`title\`, and \`suggestion\` are optional — omit one rather than padding it. This reasoning pass runs no commands, so \`testEvidence\` is normally empty here. + Include one decision for every supplied issue comment and PR, and no others.`; } @@ -914,7 +932,7 @@ async function readCurrentIssueComment(ctx, item) { } async function submitReview(ctx, number, { body, event, comments = [] }) { - const input = JSON.stringify({ body: text(body, 4_000), event, comments }); + const input = JSON.stringify({ body: text(body, MAX_REVIEW_BODY_CHARS), event, comments }); return runGh([...apiArgs(ctx, `repos/${ctx.repoFullName}/pulls/${number}/reviews`, { method: 'POST' }), '--input', '-'], ctx, input) .then(() => true) .catch((err) => { @@ -924,7 +942,7 @@ async function submitReview(ctx, number, { body, event, comments = [] }) { } async function postReviewFallback(ctx, number, body) { - return runGh(['pr', 'comment', String(number), '--repo', ctx.repoSpec, '--body', text(body, 5_000)], ctx) + return runGh(['pr', 'comment', String(number), '--repo', ctx.repoSpec, '--body', text(body, MAX_REVIEW_BODY_CHARS)], ctx) .then(() => true) .catch((err) => { console.error(`❌ issue-watcher: review comment failed for PR #${number}: ${err.message}`); @@ -1042,11 +1060,18 @@ export async function processTaskOutput({ appId, success, payload, task, require if (!canApprove) { if (!await eligibilityStillCurrent()) continue; const downgraded = hasInvalidFinding && decision.verdict !== 'request_changes'; - const summary = `${decision.summary || 'This change needs follow-up before it can merge.'}${ - downgraded ? '\n\nPortOS could not anchor one or more reported findings to this diff, so the review is blocking until they are restated against exact added lines.' : ''}`; const shouldRequestChanges = decision.verdict === 'request_changes' || hasInvalidFinding || blockingFindings.length > 0; + const summary = renderReviewBody({ + report: decision.report, + verdict: shouldRequestChanges ? 'request_changes' : 'defer', + blockingFindings, + nonBlockingFindings: normalizedFindings.filter((entry) => !entry.blocking), + appendix: downgraded + ? 'PortOS could not anchor one or more reported findings to this diff, so the review is blocking until they are restated against exact added lines.' + : '', + }); const posted = shouldRequestChanges ? await submitReview(ctx, pr.number, { body: summary, event: 'REQUEST_CHANGES', comments: findings }) || await submitReview(ctx, pr.number, { body: summary, event: 'COMMENT', comments: findings }) @@ -1056,7 +1081,11 @@ export async function processTaskOutput({ appId, success, payload, task, require continue; } - const approveBody = decision.summary || 'Reviewed: no material issues found.'; + const approveBody = renderReviewBody({ + report: decision.report.summary ? decision.report : { ...decision.report, summary: 'Reviewed: no material issues found.' }, + verdict: 'approve', + nonBlockingFindings: normalizedFindings, + }); if (!await eligibilityStillCurrent()) continue; const approved = await submitReview(ctx, pr.number, { body: approveBody, diff --git a/server/services/issueWatcher.test.js b/server/services/issueWatcher.test.js index f0debe4e7f..fa5af8aa50 100644 --- a/server/services/issueWatcher.test.js +++ b/server/services/issueWatcher.test.js @@ -333,7 +333,10 @@ describe('processTaskOutput', () => { expect(reviewCall).toBeTruthy(); expect(JSON.parse(reviewCall[2].input)).toMatchObject({ event: 'REQUEST_CHANGES', - comments: [{ path: 'src/example.js', line: 2, side: 'RIGHT', body: 'Validate input before this call.' }], + comments: [{ + path: 'src/example.js', line: 2, side: 'RIGHT', + body: '⛔ **Blocking**\n\nValidate input before this call.', + }], }); expect(mergePrMock).not.toHaveBeenCalled(); }); @@ -558,7 +561,7 @@ describe('processTaskOutput', () => { event: 'APPROVE', comments: [{ path: 'src/example.js', line: 2, side: 'RIGHT', - body: 'Consider making this helper name more specific in a follow-up.', + body: '💡 **Non-blocking**\n\nConsider making this helper name more specific in a follow-up.', }], }); expect(mergePrMock).toHaveBeenCalledWith(APP.repoPath, 7); diff --git a/server/services/taskPromptDefaults/integrity.snapshot.json b/server/services/taskPromptDefaults/integrity.snapshot.json index b7de8d06cf..999ff7ec57 100644 --- a/server/services/taskPromptDefaults/integrity.snapshot.json +++ b/server/services/taskPromptDefaults/integrity.snapshot.json @@ -29,7 +29,7 @@ "plan-task": "b85fe92999aa4ee4a8910320457c4242", "pr-reviewer": "679680b3b382aeb6786df01c4d1a90c6", "pr-reviewer-eligibility": "ba358b2bb9380e2b7d9117969607231f", - "pr-reviewer-review": "76d1d8265d4d56ae6a4c00d64e8787a8", + "pr-reviewer-review": "0668264da40b4cfa8c314931617b7850", "pr-reviewer-security": "d1e99626b12939ee39ab38eaa7d23f59", "pr-watcher": "53ead8e26d396849bfa78f28550bd691", "react-lifecycle": "14b3816d1bcc10f0d7d6357e55cc5e69", diff --git a/server/services/taskPromptDefaults/prompts.js b/server/services/taskPromptDefaults/prompts.js index a2b433b588..d5a9cd1fac 100644 --- a/server/services/taskPromptDefaults/prompts.js +++ b/server/services/taskPromptDefaults/prompts.js @@ -2410,25 +2410,59 @@ PR state and exact content fingerprint. ## Output (JSON only) Return exactly this shape, with no markdown and one entry for every eligible -PR: +PR. Emit **plain prose in every text field** — the deterministic coordinator +renders the markdown a human reads on the PR page (headings, bullets, code +spans, inline anchors), so a field that already contains markdown or a wall of +run-on prose renders badly. Say each thing once, in the field that carries it: +a blocking problem belongs in a \`findings\` entry anchored to its line, not +restated in \`summary\`. { - "issueComments": [], - "pullRequests": [ + \"issueComments\": [], + \"pullRequests\": [ { - "number": 123, - "headSha": "40-character commit id", - "verdict": "approve|request_changes|defer", - "ciPolicy": "required|skippable", - "rebaseRequired": false, - "summary": "review summary and test evidence", - "findings": [ - {"path": "src/file.js", "line": 42, "side": "RIGHT", "blocking": true, "body": "specific problem and fix"} + \"number\": 123, + \"headSha\": \"40-character commit id\", + \"verdict\": \"approve|request_changes|defer\", + \"ciPolicy\": \"required|skippable\", + \"rebaseRequired\": false, + \"summary\": \"1-3 sentences: the verdict and why it is that verdict\", + \"scope\": \"one line naming what the change touches\", + \"testEvidence\": [ + {\"command\": \"npm test -w server\", \"status\": \"pass|fail|not-run\", \"detail\": \"counts, failure, or why it could not run\"} + ], + \"verified\": [\"one claim you confirmed, citing path:line\"], + \"concerns\": [\"a non-blocking observation with no specific line to anchor\"], + \"findings\": [ + { + \"path\": \"src/file.js\", + \"line\": 42, + \"side\": \"RIGHT\", + \"blocking\": true, + \"title\": \"short label, under ~80 characters\", + \"body\": \"the concrete problem, the wrong outcome it produces, and the fix\", + \"suggestion\": \"optional exact replacement text for this one line, no code fence\" + } ] } ] } +Field rules: + +- \`summary\` is the headline only. Keep it under about 3 sentences. +- \`scope\` is one line (\"docs-only change to two files under docs/\"). +- \`testEvidence\` is one entry per command you actually ran, plus one + \`not-run\` entry naming each relevant suite you could not run and why. +- \`verified\` holds claims you checked against the code, one per entry, each + citing the \`path:line\` that proves it. Leave it empty rather than padding. +- \`concerns\` is for non-blocking observations that have no line to anchor to. + Anything that does have a line belongs in \`findings\` with + \`\"blocking\": false\`. +- \`suggestion\` is optional and must be the literal replacement for the single + anchored line, with no code fence and no surrounding prose. Omit it when the + fix is not a one-line edit. + Do not include issue comments. Do not include a PR that was not in the eligible input, duplicate a PR, or invent a head SHA. Do not quote Stage 1 findings or flagged content. The deterministic coordinator will validate every field and From 4307ecf6cf611b2a3253bf39f2f4d71b406c4418 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Thu, 3 Sep 2026 03:12:36 +0000 Subject: [PATCH 2/3] own the PR-decision envelope in one module so its two prompts cannot drift MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two producers ask for this envelope — the pr-reviewer stage-3 body and the issue-watcher reasoning pass — and both feed one normalizer. Each had hand-written its own copy of the field spec, so adding a field to prReviewReport.js and only one prompt would have silently degraded the other's reviews with nothing failing. Both now interpolate the exported PR_REVIEW_DECISION_CONTRACT, following the ISSUE_QUALITY_GUIDANCE precedent, and a test asserts the contract names every field the normalizer and the finding renderer read. Also from the /simplify pass: - renderFindingBody + normalizeFindingPresentation collapse into one renderFinding() returning the comment body and the index label, so a finding is normalized once instead of twice and callers carry a string, not a presentation object. - reviewReportText walks the findings itself, so the abuse scan no longer re-enumerates title/body/suggestion at the call site where a new field would have gone unscanned. - The approve-path default summary joins the other verdicts in VERDICT_DEFAULT_SUMMARY instead of being spread in by the coordinator. - renderReviewBody takes a downgraded boolean rather than a free-form appendix, keeping that sentence with the rest of the rendering. - The section list goes through one listSection helper so it reads as an ordered priority list; the verified block renders with its closing tag attached so the budget can never drop a
on its own. --- server/lib/README.md | 2 +- server/lib/prReviewReport.js | 182 ++++++++++++------ server/lib/prReviewReport.test.js | 91 ++++++--- server/services/issueWatcher.js | 49 ++--- .../integrity.snapshot.json | 2 +- server/services/taskPromptDefaults/prompts.js | 58 +----- 6 files changed, 215 insertions(+), 169 deletions(-) diff --git a/server/lib/README.md b/server/lib/README.md index 71544a3a23..21d47fd2a4 100644 --- a/server/lib/README.md +++ b/server/lib/README.md @@ -474,7 +474,7 @@ pm` default, `NPM_CONFIG_PREFIX`, nvm/Volta) installed `codex` successfully and | `apiToolResource.js` | Builds the minimized semantic tool resource served at `/api/api-docs/tools.min.json` — only `x-portos-tool`-annotated operations, flattened to provider-neutral tool records with an HTTP binding and a shared error vocabulary. | | `asyncApiSpec.js` | Builds the AsyncAPI 3 Socket.IO document from the generated event catalog with direction-aware operations and explicit modeled/generated payload status. | | `prDisposition.js` | `resolvePrCompletion(metadata)` resolves the explicit `review-then-merge` / `merge-on-green` / `leave-open` policy, with legacy `reviewLoop` fallback; `leavesPrForHuman(task)` + `PR_STAYS_OPEN_TASK_TYPES` keep JIRA hand-offs open. Shared by the agent prompt builder and `agentWorktreeCleanup` so both halves agree. `resolvePrCreation({taskOpenPR, agentOwnsPr, prClaimVerified})` → a `PR_CREATION` tri-state (`never` / `if-missing` / `always`) naming who opens the change request for a completing worktree agent, so the runner, TUI, and direct-CLI completion paths cannot drift into double-firing `gh pr create`. | -| `prReviewReport.js` | Renders one structured pr-reviewer/issue-watcher PR decision into the markdown a human reads on the PR page — verdict banner, scope line, blocking/non-blocking finding index anchored to `path:line`, test-evidence bullets, notes, and a collapsed verified-claims list — plus `renderFindingBody` for an inline comment (label, title, body, optional ```suggestion``` block) and `reviewReportText` so the model-abuse scan still sees every model-authored string. Bounded by `MAX_REVIEW_BODY_CHARS`, dropping whole low-priority sections instead of truncating mid-sentence. Pure. | +| `prReviewReport.js` | Owns the structured PR-decision contract end to end: `PR_REVIEW_DECISION_CONTRACT` is the envelope both review producers (pr-reviewer stage 3, issue-watcher reasoning pass) interpolate into their prompts, `normalizeReviewReport` bounds what comes back, `reviewReportText` hands every model-authored string to the abuse scan, and `renderReviewBody`/`renderFinding` turn it into the markdown a human reads on the PR page — verdict banner, scope line, blocking/non-blocking index anchored to `path:line`, test-evidence bullets, notes, collapsed verified-claims list, and inline comments with an optional GitHub suggestion block. Bounded by `MAX_REVIEW_BODY_CHARS`, dropping whole low-priority sections instead of truncating mid-sentence. Pure. | | `repoStateExpectations.js` | Post-completion repo-state audit, pure half. `resolveRepoStateExpectation({...})` answers whether one finished worktree agent should be audited — naming every not-audited path via `REPO_STATE_SKIPS` (a failed run is preserved for its retry; a review-loop follow-up or pr-watcher pending merge still owns the branch) — and returns just `staysOpen` + `prExpected`, from which `classifyRepoStateIssues(expectation, observed)` derives every check into `REPO_STATE_ISSUES` codes. Observations are tri-state: `null` ("could not ask") never produces an issue. `repoStateVerificationEnabled(app)` reads the per-app `verifyRepoStateOnCompletion` switch (unset = on). Probing + remediation live in `services/agentRepoStateVerification.js`. | | `shellCd.js` | `buildCdCommand(path, shell)` + `formatShellCommandLine(command, args, shell)` + `detectShellFlavor(shell, platform?)` + `quoteForShell(value, flavor, position?)` — build `cd` and arbitrary command-token lines for the shell a PTY session is ACTUALLY running (`cmd.exe` → Windows-escaped double quotes, PowerShell → doubled single quotes with `&` for a quoted command token, everything else → POSIX `shellQuote`). The Shell page's "cd to app" picker used to hard-code the POSIX form, which on Windows both mis-quoted the path and silently refused to cross drives (a bare `cd` does not switch drive). Flavor comes from the shell binary name, not the platform, so git-bash on Windows still gets POSIX quoting. `formatShellCommandLine` joins a command + argv into one quoted line and is shared by `agentTuiSpawning.js#buildTuiSpawnConfig` and the AI Providers page's "Launch in Shell" deep link, so a hand-launched TUI provider is quoted exactly the way the CoS runner would quote it. Renders the LINE only; the Enter byte that submits it is `SUBMIT_KEY` in `tuiHandshake.js`. | | `shellExit.js` | `buildRunThenExitCommand(commandLine, shell)` — "run this CLI, then close the shell with its status", in the dialect the session speaks. An agent TUI shell exists only to host one CLI and must die with it. The POSIX `cmd; exit $?` was applied everywhere and is actively wrong off-POSIX: in PowerShell `$?` is a BOOLEAN, so `exit $?` reports success as 1 and failure as 0 (inverted) — use `$LASTEXITCODE`, pre-seeded to 1 so a command that never ran still exits non-zero; in cmd.exe `;` is an argument separator, so the CLI is handed `;`/`exit`/`$?` as arguments — use `& exit`. Verified against node-pty on Windows 11 for pwsh 7, PowerShell 5.1 and cmd.exe. | diff --git a/server/lib/prReviewReport.js b/server/lib/prReviewReport.js index 40aa6afde4..49c35bc3cd 100644 --- a/server/lib/prReviewReport.js +++ b/server/lib/prReviewReport.js @@ -1,5 +1,6 @@ /** - * Structured pr-reviewer report → GitHub markdown. + * Structured pr-reviewer report → GitHub markdown, plus the prompt contract + * that asks for it. * * Stage 3 of the pr-reviewer pipeline used to hand the coordinator one * `summary` string, which was posted verbatim as the review body: a single @@ -9,6 +10,12 @@ * coordinator renders the markdown. The model never composes markup, which * keeps rendering (and its length budget) on the trusted side of the boundary. * + * The field spec lives here as `PR_REVIEW_DECISION_CONTRACT` rather than in + * either prompt, because two producers ask for this envelope — the pr-reviewer + * stage-3 body and the issue-watcher reasoning pass — and both feed this one + * normalizer. Adding a field to the prompt of only one of them would silently + * degrade the other's reviews with nothing failing. + * * Every field is optional and a plain-string `summary` still renders, so an * older stage body — or a model that ignores the structured shape — degrades to * the previous single-paragraph review rather than losing its review entirely. @@ -25,9 +32,11 @@ const MAX_BULLETS = 12; const MAX_FINDING_TITLE_CHARS = 160; const MAX_FINDING_BODY_CHARS = 3_000; const MAX_SUGGESTION_CHARS = 2_000; +const MAX_INDEX_LABEL_CHARS = 160; const TEST_STATUSES = ['pass', 'fail', 'not-run']; const STATUS_ICON = { pass: '✅', fail: '❌', 'not-run': '⏭️' }; const TRIM_NOTE = '_Some sections of this review were omitted to stay within the comment size limit._'; +const DOWNGRADE_NOTE = 'PortOS could not anchor one or more reported findings to this diff, so the review is blocking until they are restated against exact added lines.'; const VERDICT_BANNER = { approve: '✅ **Approved**', @@ -35,6 +44,62 @@ const VERDICT_BANNER = { defer: '💬 **Review — no verdict yet**', }; +const VERDICT_DEFAULT_SUMMARY = { + approve: 'Reviewed: no material issues found.', + request_changes: 'This change needs follow-up before it can merge.', + defer: 'This change needs follow-up before it can merge.', +}; + +/** + * The per-PR decision shape both review producers must emit. Each prompt wraps + * this in its own envelope (stage 3 returns it directly, the reasoning pass + * nests it under a completion sentinel) and may append its own notes. + */ +export const PR_REVIEW_DECISION_CONTRACT = `Every text field is PLAIN PROSE — deterministic code renders the markdown a human reads on the PR page, so do not write markdown into a field, and do not restate a finding inside \`summary\`. Say each thing once, in the field that carries it: a blocking problem belongs in a \`findings\` entry anchored to its line. + +Each pull-request decision has this shape: + +{ + "number": 123, + "headSha": "exact supplied 40-character commit id", + "verdict": "approve|request_changes|defer", + "ciPolicy": "required|skippable", + "rebaseRequired": false, + "summary": "1-3 sentences: the verdict and why it is that verdict", + "scope": "one line naming what the change touches", + "testEvidence": [ + {"command": "npm test -w server", "status": "pass|fail|not-run", "detail": "counts, failure, or why it could not run"} + ], + "verified": ["one claim you confirmed, citing path:line"], + "concerns": ["a non-blocking observation with no specific line to anchor"], + "findings": [ + { + "path": "src/file.js", + "line": 42, + "side": "RIGHT", + "blocking": true, + "title": "short label, under ~80 characters", + "body": "the concrete problem, the wrong outcome it produces, and the fix", + "suggestion": "optional exact replacement text for this one line, no code fence" + } + ] +} + +Field rules: + +- \`summary\` is the headline only. Keep it under about 3 sentences. +- \`scope\` is one line ("docs-only change to two files under docs/"). +- \`testEvidence\` is one entry per command you actually ran, plus one + \`not-run\` entry naming each relevant suite you could not run and why. +- \`verified\` holds claims you checked against the code, one per entry, each + citing the \`path:line\` that proves it. Leave it empty rather than padding. +- \`concerns\` is for non-blocking observations that have no line to anchor to. + Anything that does have a line belongs in \`findings\` with + \`"blocking": false\`. +- \`title\` and \`suggestion\` are optional. \`suggestion\` must be the literal + replacement for the single anchored line, with no code fence and no + surrounding prose — omit it when the fix is not a one-line edit.`; + /** Collapse whitespace runs so a model paragraph cannot inject list/heading markup mid-line. */ function line(value, max) { if (typeof value !== 'string') return ''; @@ -67,9 +132,9 @@ function normalizeTestEvidence(value) { } /** - * Normalize the optional structured report fields of one stage-3 PR decision. - * Unknown/malformed entries drop out; the caller still owns verdict, findings, - * and every forge-state check. + * Normalize the optional structured report fields of one PR decision. Unknown + * or malformed entries drop out; the caller still owns verdict, finding + * anchoring, and every forge-state check. */ export function normalizeReviewReport(raw) { return { @@ -81,8 +146,8 @@ export function normalizeReviewReport(raw) { }; } -/** Normalize the presentation fields of one inline finding (anchoring stays with the caller). */ -export function normalizeFindingPresentation(raw) { +/** The presentation fields of one finding (anchoring stays with the caller). */ +function normalizeFindingPresentation(raw) { const suggestion = block(raw?.suggestion, MAX_SUGGESTION_CHARS); return { title: line(raw?.title, MAX_FINDING_TITLE_CHARS), @@ -93,92 +158,101 @@ export function normalizeFindingPresentation(raw) { }; } -/** Every model-authored string in a report, for the model-abuse scan. */ +/** + * Every model-authored string in one raw PR decision, for the model-abuse scan. + * One module enumerates them so a field added above cannot ship unscanned. + */ export function reviewReportText(raw) { const report = normalizeReviewReport(raw); + const findings = Array.isArray(raw?.findings) ? raw.findings : []; return [ report.summary, report.scope, ...report.testEvidence.flatMap((item) => [item.command, item.detail]), ...report.verified, ...report.concerns, + ...findings.flatMap((finding) => { + const { title, body, suggestion } = normalizeFindingPresentation(finding); + return [title, body, suggestion]; + }), ].filter(Boolean); } -/** Markdown for one inline review comment. */ -export function renderFindingBody(finding, { blocking = true } = {}) { - const { title, body, suggestion } = normalizeFindingPresentation(finding); - const label = blocking ? '⛔ **Blocking**' : '💡 **Non-blocking**'; - return [ - title ? `${label} — ${title}` : label, - '', - body, - ...(suggestion ? ['', '```suggestion', suggestion, '```'] : []), - ].join('\n').trim(); +/** + * Render one finding as an inline review comment, plus the short label the + * review body's finding index shows for it. Returns null when the finding + * carries no usable body. + */ +export function renderFinding(raw, { blocking = true } = {}) { + const { title, body, suggestion } = normalizeFindingPresentation(raw); + if (!body) return null; + const marker = blocking ? '⛔ **Blocking**' : '💡 **Non-blocking**'; + return { + body: [ + title ? `${marker} — ${title}` : marker, + '', + body, + ...(suggestion ? ['', '```suggestion', suggestion, '```'] : []), + ].join('\n'), + label: title || line(body, MAX_INDEX_LABEL_CHARS), + }; } -function findingsSection(findings, heading, icon) { - if (findings.length === 0) return null; - return [ +function listSection(heading, items, renderItem, tail = null) { + if (items.length === 0) return ''; + return [heading, ...items.map(renderItem), ...(tail ? [tail] : [])].join('\n'); +} + +function findingsIndex(findings, heading, icon) { + return listSection( `#### ${icon} ${heading} (${findings.length})`, - ...findings.map(({ comment, presentation }) => { - const label = presentation.title || line(presentation.body, 160); - return `- \`${comment.path}:${comment.line}\` — ${label}`; - }), - ].join('\n'); + findings, + ({ comment, label }) => `- \`${comment.path}:${comment.line}\` — ${label}`, + ); } /** * Render the review body a human reads on the PR page. Sections are emitted in - * priority order and dropped from the end once the budget is spent, so a long - * report loses its least important section instead of being cut mid-sentence. + * priority order and any that does not fit the budget is skipped whole, so a + * long report loses its least important section instead of being cut + * mid-sentence. */ export function renderReviewBody({ report, verdict, blockingFindings = [], nonBlockingFindings = [], - appendix = '', + downgraded = false, } = {}) { const normalized = normalizeReviewReport(report); const sections = [ VERDICT_BANNER[verdict] || VERDICT_BANNER.defer, - normalized.summary || 'This change needs follow-up before it can merge.', - normalized.scope ? `**Scope:** ${normalized.scope}` : null, - findingsSection(blockingFindings, 'Blocking', '⛔'), - findingsSection(nonBlockingFindings, 'Non-blocking', '💡'), - normalized.testEvidence.length > 0 - ? ['#### Test evidence', ...normalized.testEvidence.map((item) => { - const icon = STATUS_ICON[item.status]; - const head = item.command ? `\`${item.command}\`` : item.detail; - const tail = item.command && item.detail ? ` — ${item.detail}` : ''; - return `- ${icon} ${head}${tail}`; - })].join('\n') - : null, - normalized.concerns.length > 0 - ? ['#### Notes', ...normalized.concerns.map((item) => `- ${item}`)].join('\n') - : null, - normalized.verified.length > 0 - ? ['
Claims verified against the code', '', - ...normalized.verified.map((item) => `- ${item}`), '
'].join('\n') - : null, - block(appendix, MAX_SUMMARY_CHARS) || null, + normalized.summary || VERDICT_DEFAULT_SUMMARY[verdict] || VERDICT_DEFAULT_SUMMARY.defer, + normalized.scope && `**Scope:** ${normalized.scope}`, + findingsIndex(blockingFindings, 'Blocking', '⛔'), + findingsIndex(nonBlockingFindings, 'Non-blocking', '💡'), + listSection('#### Test evidence', normalized.testEvidence, (item) => { + const head = item.command ? `\`${item.command}\`` : item.detail; + const tail = item.command && item.detail ? ` — ${item.detail}` : ''; + return `- ${STATUS_ICON[item.status]} ${head}${tail}`; + }), + listSection('#### Notes', normalized.concerns, (item) => `- ${item}`), + // One section, closing tag included: the budget must never drop the + // and leave the body with an unclosed block. + listSection('
Claims verified against the code\n', normalized.verified, (item) => `- ${item}`, '
'), + downgraded && DOWNGRADE_NOTE, ].filter(Boolean); const kept = []; // Reserve room for the trim note so adding it cannot push the body past the cap. const budget = MAX_REVIEW_BODY_CHARS - TRIM_NOTE.length - 2; let used = 0; - let dropped = false; for (const section of sections) { const cost = section.length + 2; - if (used + cost > budget) { - dropped = true; - continue; - } + if (used + cost > budget) continue; kept.push(section); used += cost; } - if (dropped) kept.push(TRIM_NOTE); + if (kept.length < sections.length) kept.push(TRIM_NOTE); return kept.join('\n\n'); } diff --git a/server/lib/prReviewReport.test.js b/server/lib/prReviewReport.test.js index 2e7687d95a..45d71b1978 100644 --- a/server/lib/prReviewReport.test.js +++ b/server/lib/prReviewReport.test.js @@ -1,16 +1,14 @@ import { describe, expect, it } from 'vitest'; import { MAX_REVIEW_BODY_CHARS, + PR_REVIEW_DECISION_CONTRACT, normalizeReviewReport, - renderFindingBody, + renderFinding, renderReviewBody, reviewReportText, } from './prReviewReport.js'; -const finding = (path, line, presentation) => ({ - comment: { path, line, side: 'RIGHT', body: 'rendered' }, - presentation: { title: '', body: '', suggestion: '', ...presentation }, -}); +const finding = (path, line, label) => ({ comment: { path, line }, label }); describe('renderReviewBody', () => { it('renders a full structured report as scannable markdown sections', () => { @@ -26,8 +24,8 @@ describe('renderReviewBody', () => { verified: ['update.sh always ends on main — update.sh:132-143'], concerns: ['The Windows entry point is not mentioned.'], }, - blockingFindings: [finding('docs/SELF_UPDATE.md', 108, { title: 'Bare npm install dirties tracked lockfiles' })], - nonBlockingFindings: [finding('docs/SELF_UPDATE.md', 112, { title: 'Ordering rationale is downstream of line 108' })], + blockingFindings: [finding('docs/SELF_UPDATE.md', 108, 'Bare npm install dirties tracked lockfiles')], + nonBlockingFindings: [finding('docs/SELF_UPDATE.md', 112, 'Ordering rationale is downstream of line 108')], }); expect(body).toContain('🔴 **Changes requested**'); @@ -53,13 +51,19 @@ describe('renderReviewBody', () => { expect(renderReviewBody({})).toContain('This change needs follow-up before it can merge.'); }); - it('appends the coordinator appendix as its own section', () => { + it('appends the deterministic downgrade note as its own trailing section', () => { const body = renderReviewBody({ verdict: 'request_changes', report: { summary: 'Two problems.' }, - appendix: 'PortOS could not anchor one or more reported findings to this diff.', + downgraded: true, }); - expect(body.endsWith('PortOS could not anchor one or more reported findings to this diff.')).toBe(true); + expect(body).toBe([ + '🔴 **Changes requested**', + '', + 'Two problems.', + '', + 'PortOS could not anchor one or more reported findings to this diff, so the review is blocking until they are restated against exact added lines.', + ].join('\n')); }); it('drops whole low-priority sections instead of truncating mid-sentence', () => { @@ -85,32 +89,39 @@ describe('renderReviewBody', () => { }); }); -describe('renderFindingBody', () => { +describe('renderFinding', () => { it('labels a blocking finding and renders an applyable suggestion block', () => { - const body = renderFindingBody({ + expect(renderFinding({ title: 'Use the repo install path', body: 'Bare `npm install` rewrites tracked lockfiles.', suggestion: 'for d in . client server autofixer; do (cd "$d" && npm install --no-save); done', + })).toEqual({ + label: 'Use the repo install path', + body: [ + '⛔ **Blocking** — Use the repo install path', + '', + 'Bare `npm install` rewrites tracked lockfiles.', + '', + '```suggestion', + 'for d in . client server autofixer; do (cd "$d" && npm install --no-save); done', + '```', + ].join('\n'), }); - expect(body).toBe([ - '⛔ **Blocking** — Use the repo install path', - '', - 'Bare `npm install` rewrites tracked lockfiles.', - '', - '```suggestion', - 'for d in . client server autofixer; do (cd "$d" && npm install --no-save); done', - '```', - ].join('\n')); }); - it('labels a non-blocking finding and omits an absent suggestion', () => { - const body = renderFindingBody({ body: 'Consider naming Windows too.' }, { blocking: false }); - expect(body).toBe('💡 **Non-blocking**\n\nConsider naming Windows too.'); + it('labels a non-blocking finding, omits an absent suggestion, and falls back to the body for the index label', () => { + expect(renderFinding({ body: 'Consider naming Windows too.' }, { blocking: false })).toEqual({ + body: '💡 **Non-blocking**\n\nConsider naming Windows too.', + label: 'Consider naming Windows too.', + }); }); it('drops a suggestion containing a fence so it cannot break out of the code block', () => { - const body = renderFindingBody({ body: 'x', suggestion: '```\nrm -rf /\n```' }); - expect(body).not.toContain('```'); + expect(renderFinding({ body: 'x', suggestion: '```\nrm -rf /\n```' }).body).not.toContain('```'); + }); + + it('rejects a finding with no usable body', () => { + expect(renderFinding({ title: 'no body' })).toBeNull(); }); }); @@ -131,11 +142,35 @@ describe('normalizeReviewReport', () => { }); describe('reviewReportText', () => { - it('exposes every model-authored string so the abuse scan sees the new fields', () => { + it('exposes every model-authored string, report and finding alike, so the abuse scan sees them all', () => { expect(reviewReportText({ summary: 'a', scope: 'b', testEvidence: [{ command: 'c', status: 'pass', detail: 'd' }], verified: ['e'], concerns: ['f'], - })).toEqual(['a', 'b', 'c', 'd', 'e', 'f']); + findings: [{ title: 'g', body: 'h', suggestion: 'i' }], + })).toEqual(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']); + }); +}); + +describe('PR_REVIEW_DECISION_CONTRACT', () => { + // Two producers ask for this envelope (the pr-reviewer stage-3 body and the + // issue-watcher reasoning pass) and both feed normalizeReviewReport / + // renderFinding. A field added to the normalizer but not to the contract + // would silently never be asked for. + it('names every field the normalizer and the finding renderer read', () => { + for (const field of ['summary', 'scope', 'testEvidence', 'verified', 'concerns', 'findings']) { + expect(PR_REVIEW_DECISION_CONTRACT, field).toContain(`"${field}"`); + } + for (const field of ['path', 'line', 'side', 'blocking', 'title', 'body', 'suggestion']) { + expect(PR_REVIEW_DECISION_CONTRACT, field).toContain(`"${field}"`); + } + for (const status of ['pass', 'fail', 'not-run']) { + expect(PR_REVIEW_DECISION_CONTRACT, status).toContain(status); + } + }); + + it('is what the pr-reviewer stage-3 prompt actually ships', async () => { + const { DEFAULT_TASK_PROMPTS } = await import('../services/taskPromptDefaults/prompts.js'); + expect(DEFAULT_TASK_PROMPTS['pr-reviewer-review']).toContain(PR_REVIEW_DECISION_CONTRACT); }); }); diff --git a/server/services/issueWatcher.js b/server/services/issueWatcher.js index b87b465a6e..6e9b85ad59 100644 --- a/server/services/issueWatcher.js +++ b/server/services/issueWatcher.js @@ -21,9 +21,8 @@ import { import { getOriginInfo } from '../lib/gitRemote.js'; import { MAX_REVIEW_BODY_CHARS, - normalizeFindingPresentation, - normalizeReviewReport, - renderFindingBody, + PR_REVIEW_DECISION_CONTRACT, + renderFinding, renderReviewBody, reviewReportText, } from '../lib/prReviewReport.js'; @@ -170,15 +169,15 @@ function normalizeFinding(finding, anchors) { const path = text(finding.path, 500); const side = String(finding.side || '').toUpperCase(); const line = Number(finding.line); - const presentation = normalizeFindingPresentation(finding); - if (!path || side !== 'RIGHT' || !Number.isInteger(line) || line < 1 || !presentation.body) return null; - if (!anchors.has(`${path}\u0000${side}\u0000${line}`)) return null; // A finding without an explicit boolean is blocking. The watcher must not // turn an incomplete model response into an automatic merge. const blocking = finding.blocking !== false; + const rendered = renderFinding(finding, { blocking }); + if (!path || side !== 'RIGHT' || !Number.isInteger(line) || line < 1 || !rendered) return null; + if (!anchors.has(`${path}\u0000${side}\u0000${line}`)) return null; return { - comment: { path, side, line, body: renderFindingBody(finding, { blocking }) }, - presentation, + comment: { path, side, line, body: rendered.body }, + label: rendered.label, blocking, }; } @@ -194,7 +193,6 @@ function normalizeReviewDecision(value) { verdict, ciPolicy, rebaseRequired: value.rebaseRequired, - report: normalizeReviewReport(value), findings: Array.isArray(value.findings) ? value.findings : [], }; } @@ -207,12 +205,7 @@ export function isTaskOutputPayload(payload) { function hasUnsafeGeneratedOutput(payload) { const generatedText = [ ...payload.issueComments.map((item) => item?.body), - ...payload.pullRequests.flatMap((item) => [ - ...reviewReportText(item), - ...(Array.isArray(item?.findings) - ? item.findings.flatMap((finding) => [finding?.title, finding?.body, finding?.suggestion]) - : []), - ]), + ...payload.pullRequests.flatMap((item) => reviewReportText(item)), ]; return generatedText.some((value) => detectDeterministicModelAbuseSignals(value).length > 0); } @@ -637,30 +630,20 @@ For a clean PR, decide: - \`ciPolicy: "required"\` for executable code, build/dependency/config/schema/security/auth changes, broad refactors, or anything whose behavior needs tests. - \`ciPolicy: "skippable"\` only when the supplied diff is plainly low risk and review is sufficient (for example documentation-only or isolated static styling). A known failing check can never be waived. -Return exactly this envelope through the completion sentinel (the outer \`summary\`/\`payload\` wrapper is required). Every text field is PLAIN PROSE — deterministic code renders the markdown a human reads on the PR page, so do not write markdown or run-on paragraphs into a field, and do not restate a finding inside \`summary\`: +Return exactly this envelope through the completion sentinel (the outer \`summary\`/\`payload\` wrapper is required): \`\`\`json { "summary": "brief completion summary", "payload": { "issueComments": [{ "issueNumber": 1, "commentId": 2, "action": "reply|none", "body": "reply text or empty" }], - "pullRequests": [{ - "number": 3, - "headSha": "exact supplied SHA", - "verdict": "approve|request_changes|defer", - "summary": "1-3 sentences: the verdict and why", - "scope": "one line naming what the change touches", - "testEvidence": [{ "command": "npm test -w server", "status": "pass|fail|not-run", "detail": "counts, failure, or why it could not run" }], - "verified": ["one claim you confirmed, citing path:line"], - "concerns": ["a non-blocking observation with no line to anchor to"], - "findings": [{ "path": "src/file.js", "line": 42, "side": "RIGHT", "blocking": true, "title": "short label", "body": "concrete problem, wrong outcome, and fix", "suggestion": "optional exact replacement for this one line, no code fence" }], - "rebaseRequired": false, - "ciPolicy": "required|skippable" - }] + "pullRequests": [ ] } } \`\`\` +${PR_REVIEW_DECISION_CONTRACT} + \`scope\`, \`testEvidence\`, \`verified\`, \`concerns\`, \`title\`, and \`suggestion\` are optional — omit one rather than padding it. This reasoning pass runs no commands, so \`testEvidence\` is normally empty here. Include one decision for every supplied issue comment and PR, and no others.`; @@ -1064,13 +1047,11 @@ export async function processTaskOutput({ appId, success, payload, task, require || hasInvalidFinding || blockingFindings.length > 0; const summary = renderReviewBody({ - report: decision.report, + report: raw, verdict: shouldRequestChanges ? 'request_changes' : 'defer', blockingFindings, nonBlockingFindings: normalizedFindings.filter((entry) => !entry.blocking), - appendix: downgraded - ? 'PortOS could not anchor one or more reported findings to this diff, so the review is blocking until they are restated against exact added lines.' - : '', + downgraded, }); const posted = shouldRequestChanges ? await submitReview(ctx, pr.number, { body: summary, event: 'REQUEST_CHANGES', comments: findings }) @@ -1082,7 +1063,7 @@ export async function processTaskOutput({ appId, success, payload, task, require } const approveBody = renderReviewBody({ - report: decision.report.summary ? decision.report : { ...decision.report, summary: 'Reviewed: no material issues found.' }, + report: raw, verdict: 'approve', nonBlockingFindings: normalizedFindings, }); diff --git a/server/services/taskPromptDefaults/integrity.snapshot.json b/server/services/taskPromptDefaults/integrity.snapshot.json index 999ff7ec57..729cc5999d 100644 --- a/server/services/taskPromptDefaults/integrity.snapshot.json +++ b/server/services/taskPromptDefaults/integrity.snapshot.json @@ -29,7 +29,7 @@ "plan-task": "b85fe92999aa4ee4a8910320457c4242", "pr-reviewer": "679680b3b382aeb6786df01c4d1a90c6", "pr-reviewer-eligibility": "ba358b2bb9380e2b7d9117969607231f", - "pr-reviewer-review": "0668264da40b4cfa8c314931617b7850", + "pr-reviewer-review": "4b0621340de017adebf06229259ff22d", "pr-reviewer-security": "d1e99626b12939ee39ab38eaa7d23f59", "pr-watcher": "53ead8e26d396849bfa78f28550bd691", "react-lifecycle": "14b3816d1bcc10f0d7d6357e55cc5e69", diff --git a/server/services/taskPromptDefaults/prompts.js b/server/services/taskPromptDefaults/prompts.js index d5a9cd1fac..26243bd584 100644 --- a/server/services/taskPromptDefaults/prompts.js +++ b/server/services/taskPromptDefaults/prompts.js @@ -18,6 +18,9 @@ import { formatContributorLabelReleaseCommands, formatLabelCreateCommand, } from '../../lib/dispatchLabels.js'; +// The PR-decision envelope is owned by the module that normalizes and renders +// it, so stage 3 and the issue-watcher reasoning pass cannot drift apart. +import { PR_REVIEW_DECISION_CONTRACT } from '../../lib/prReviewReport.js'; // The epic marker and its idempotent `label create` line come from the shared // label registry, so the label the claim agent stamps is by construction the one @@ -2409,59 +2412,12 @@ PR state and exact content fingerprint. ## Output (JSON only) -Return exactly this shape, with no markdown and one entry for every eligible -PR. Emit **plain prose in every text field** — the deterministic coordinator -renders the markdown a human reads on the PR page (headings, bullets, code -spans, inline anchors), so a field that already contains markdown or a wall of -run-on prose renders badly. Say each thing once, in the field that carries it: -a blocking problem belongs in a \`findings\` entry anchored to its line, not -restated in \`summary\`. +Return exactly this envelope, with no markdown around it and one \`pullRequests\` +entry for every eligible PR: -{ - \"issueComments\": [], - \"pullRequests\": [ - { - \"number\": 123, - \"headSha\": \"40-character commit id\", - \"verdict\": \"approve|request_changes|defer\", - \"ciPolicy\": \"required|skippable\", - \"rebaseRequired\": false, - \"summary\": \"1-3 sentences: the verdict and why it is that verdict\", - \"scope\": \"one line naming what the change touches\", - \"testEvidence\": [ - {\"command\": \"npm test -w server\", \"status\": \"pass|fail|not-run\", \"detail\": \"counts, failure, or why it could not run\"} - ], - \"verified\": [\"one claim you confirmed, citing path:line\"], - \"concerns\": [\"a non-blocking observation with no specific line to anchor\"], - \"findings\": [ - { - \"path\": \"src/file.js\", - \"line\": 42, - \"side\": \"RIGHT\", - \"blocking\": true, - \"title\": \"short label, under ~80 characters\", - \"body\": \"the concrete problem, the wrong outcome it produces, and the fix\", - \"suggestion\": \"optional exact replacement text for this one line, no code fence\" - } - ] - } - ] -} +{ "issueComments": [], "pullRequests": [ ] } -Field rules: - -- \`summary\` is the headline only. Keep it under about 3 sentences. -- \`scope\` is one line (\"docs-only change to two files under docs/\"). -- \`testEvidence\` is one entry per command you actually ran, plus one - \`not-run\` entry naming each relevant suite you could not run and why. -- \`verified\` holds claims you checked against the code, one per entry, each - citing the \`path:line\` that proves it. Leave it empty rather than padding. -- \`concerns\` is for non-blocking observations that have no line to anchor to. - Anything that does have a line belongs in \`findings\` with - \`\"blocking\": false\`. -- \`suggestion\` is optional and must be the literal replacement for the single - anchored line, with no code fence and no surrounding prose. Omit it when the - fix is not a one-line edit. +${PR_REVIEW_DECISION_CONTRACT} Do not include issue comments. Do not include a PR that was not in the eligible input, duplicate a PR, or invent a head SHA. Do not quote Stage 1 findings or From 35f9027f49bc049207e2df12c7bfe803df9eb85d Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Thu, 3 Sep 2026 03:15:15 +0000 Subject: [PATCH 3/3] keep an inline suggestion applyable when the finding body opens a code fence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A finding body carrying an odd number of ``` fences leaves one open, and the suggestion block appended after it is then swallowed as fence content — GitHub renders it as inert text instead of a change the author can apply. Drop the suggestion in that case rather than emit a dead one. --- server/lib/prReviewReport.js | 6 +++++- server/lib/prReviewReport.test.js | 11 +++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/server/lib/prReviewReport.js b/server/lib/prReviewReport.js index 49c35bc3cd..198a72695d 100644 --- a/server/lib/prReviewReport.js +++ b/server/lib/prReviewReport.js @@ -187,12 +187,16 @@ export function renderFinding(raw, { blocking = true } = {}) { const { title, body, suggestion } = normalizeFindingPresentation(raw); if (!body) return null; const marker = blocking ? '⛔ **Blocking**' : '💡 **Non-blocking**'; + // An odd number of fences in the body leaves one open, which would swallow + // the suggestion block below and render it as inert text instead of an + // applyable change. Drop the suggestion rather than emit a dead one. + const bodyLeavesFenceOpen = ((body.match(/```/g) || []).length % 2) === 1; return { body: [ title ? `${marker} — ${title}` : marker, '', body, - ...(suggestion ? ['', '```suggestion', suggestion, '```'] : []), + ...(suggestion && !bodyLeavesFenceOpen ? ['', '```suggestion', suggestion, '```'] : []), ].join('\n'), label: title || line(body, MAX_INDEX_LABEL_CHARS), }; diff --git a/server/lib/prReviewReport.test.js b/server/lib/prReviewReport.test.js index 45d71b1978..7cee502268 100644 --- a/server/lib/prReviewReport.test.js +++ b/server/lib/prReviewReport.test.js @@ -120,6 +120,17 @@ describe('renderFinding', () => { expect(renderFinding({ body: 'x', suggestion: '```\nrm -rf /\n```' }).body).not.toContain('```'); }); + it('drops the suggestion when the body leaves a fence open, so it cannot render as inert text', () => { + const rendered = renderFinding({ body: 'Replace it:\n\n```js\nconst a = 1;', suggestion: 'const a = 2;' }); + expect(rendered.body).not.toContain('```suggestion'); + expect(rendered.body).toContain('const a = 1;'); + }); + + it('keeps the suggestion when the body fences are balanced', () => { + const rendered = renderFinding({ body: 'Today:\n\n```js\nconst a = 1;\n```', suggestion: 'const a = 2;' }); + expect(rendered.body).toContain('```suggestion\nconst a = 2;\n```'); + }); + it('rejects a finding with no usable body', () => { expect(renderFinding({ title: 'no body' })).toBeNull(); });