From ae94667bbe8493cded8a421ca588606bceff6a9d Mon Sep 17 00:00:00 2001 From: Christopher Date: Mon, 7 Sep 2026 11:19:06 +1000 Subject: [PATCH] fix(review): recalibrate interactive findings --- .../skills/pr-interactive-review/SKILL.md | 20 +- .../scripts/review-site.ts | 632 +++++++++++++----- .../plugins/pr-interactive-review.test.ts | 249 ++++++- 3 files changed, 741 insertions(+), 160 deletions(-) diff --git a/plugins/engineering/skills/pr-interactive-review/SKILL.md b/plugins/engineering/skills/pr-interactive-review/SKILL.md index 2185e99..494de40 100644 --- a/plugins/engineering/skills/pr-interactive-review/SKILL.md +++ b/plugins/engineering/skills/pr-interactive-review/SKILL.md @@ -30,12 +30,16 @@ After the review completes, actively enrich every finding without modifying `rev { "#1": { "what_actually_happens": "When an operator saves a label containing markup, the page renders it and the browser executes it.", + "what_actually_happens_evidence": { + "triggering_setup": "tests/config-label.test.ts creates and saves a label containing markup.", + "observable_outcome": "src/config-label.test.ts asserts the rendered label is interpreted as markup." + }, "expected_suggested": "When that label is saved, the page displays the characters as text after encoding at the rendering boundary." } } ``` -`what_actually_happens` must state a triggering setup/action and observable failure. `expected_suggested` must state the expected resulting behavior and correction. This sidecar is presentation context only: it must not change review scope, personas, severity, validation, deduplication, or required response. If direct evidence cannot support either statement, omit that field; the site labels the gap instead of inventing a scenario. `prepare` rejects malformed sidecars and IDs that are not review findings. +`what_actually_happens` must state a triggering setup/action and observable failure. It is rendered as a present defect only when `what_actually_happens_evidence` identifies both the concrete triggering setup/reachability and the observable outcome from a reviewed repository test or fixture, saved configuration/preset, production registry/declaration, reachable callsite, or explicit requirement. Without both, `prepare` removes the unsupported assertion and renders an evidence gap: treat it as an open question, not a defect. `expected_suggested` must state the expected resulting behavior and correction. Compatibility or future-risk observations must stay distinct from present defects and use an evidence-seeking open question until present reachability is proven. This sidecar is presentation context only: it must not change review scope, personas, validation, deduplication, or required response. `prepare` rejects malformed sidecars and IDs that are not review findings. Specifications may be private when the user authorizes access. Use the appropriate host tool to read or extract an authorized local file, document, or URL. Derive only concise labeled primer fields from that material, then pass the derived text with `--spec`; never put the original source content in this public repository. @@ -103,7 +107,7 @@ bun "$SKILL_DIR/scripts/review-site.ts" serve \ The warning is part of the command contract. Do not expose a review site that contains material the intended network audience may not read. -## Comment handoff loop +## Comment handoff and lifecycle loop Reviewers can save general comments or comments attached to a finding. The browser sends only validated, bounded JSON to the local server. Comments are atomically written to `comments.json` in the external workspace; comment bodies are not logged. @@ -113,17 +117,23 @@ To respond as the assistant, first inspect only unanswered local comments: curl -sS "http://127.0.0.1:/api/comments?status=unanswered" ``` -Use the returned comment `id`, formulate an evidence-based response from the review artifact and reviewed code, then save it locally: +Re-ground every reviewer correction against the review artifact and reviewed code. Never defend an initial finding merely because it appears in the original artifact. Save the evidence-based reply locally, then revise the finding when the correction narrows, reclassifies, or disproves it: ```bash curl -sS -X POST "http://127.0.0.1:/api/comments//replies" \ -H 'content-type: application/json' \ --data '{"role":"assistant","author":"Assistant","body":"Verified response with the required next action."}' + +curl -sS -X POST "http://127.0.0.1:/api/findings/%231/revisions" \ + -H 'content-type: application/json' \ + --data '{"commentId":"","status":"withdrawn","rationale":"Reviewed fixture and registry show this configuration path is not reachable.","scenario":{"actualHappens":null,"actualTriggerEvidence":null,"actualOutcomeEvidence":null}}' ``` -The page renders replies with an Assistant label. Refresh unanswered comments until the queue is empty. Never treat this local operation as authority to post a GitHub comment; GitHub posting requires a separate explicit, user-confirmed feature. +The lifecycle states are `active`, `question`, and `withdrawn`. Every revision requires the attached reviewer comment ID and a rationale. The site preserves the immutable original claim, comments/replies, and append-only revision history, while current status, severity, scenario, active counts, and verdict are recalculated for presentation. Withdrawn findings remain auditable but do not block the verdict. Use `question` when evidence remains incomplete rather than asserting a current defect. + +Refresh the site and unanswered queue until the queue is empty. Never treat this local operation as authority to post a GitHub comment; GitHub posting requires a separate explicit, user-confirmed feature. ## Completion 1. Confirm the review artifact was consumed as JSON, not markdown. -2. Browser-check the local site: business context comes first; every finding shows `What actually happens` and `Expected / suggested` (or an explicit evidence gap); severity filters and search work; the responsive layout works; a local comment and assistant reply render; a GitHub remote produces a reviewed-commit line link. +2. Browser-check the local site: business context comes first; active findings, open questions, and withdrawn findings are visibly separate; verdict/counts exclude withdrawn findings; every finding shows `What actually happens` and `Expected / suggested` (or an explicit evidence gap); severity/status filters and search work; the responsive layout works; a local comment, assistant reply, and lifecycle revision render; a GitHub remote produces a reviewed-commit line link. 3. State the workspace path and loopback URL. Do not include comment text, credentials, or source contents in the report. diff --git a/plugins/engineering/skills/pr-interactive-review/scripts/review-site.ts b/plugins/engineering/skills/pr-interactive-review/scripts/review-site.ts index c1c4a4c..9445936 100644 --- a/plugins/engineering/skills/pr-interactive-review/scripts/review-site.ts +++ b/plugins/engineering/skills/pr-interactive-review/scripts/review-site.ts @@ -12,33 +12,52 @@ const SEVERITIES = new Set(['P0', 'P1', 'P2', 'P3']); type JsonObject = Record; -export interface ReviewFinding { - id: string; +export type FindingStatus = 'active' | 'question' | 'withdrawn'; + +export interface FindingScenario { + actualHappens: string | null; + expectedSuggested: string | null; + actualTriggerEvidence: string | null; + actualOutcomeEvidence: string | null; + actualEvidenceGap: string | null; + expectedEvidenceGap: string | null; +} + +export interface FindingClaim { title: string; severity: 'P0' | 'P1' | 'P2' | 'P3'; + requiredResponse: string; + scenario: FindingScenario; +} + +export interface FindingRevision { + id: string; + commentId: string; + status: FindingStatus; + rationale: string; + changes: Partial; + createdAt: string; +} + +export interface ReviewFinding extends FindingClaim { + id: string; + status: FindingStatus; + original: FindingClaim; + revisions: FindingRevision[]; file: string; line: number; endLine: number; confidence: number | string; - requiredResponse: string; reviewers: string[]; evidence: string[]; firstEvidence: string | null; sourceLink: string | null; - scenario: FindingScenario; excerpts: { before: CodeExcerpt | null; after: CodeExcerpt | null; }; } -export interface FindingScenario { - actualHappens: string | null; - expectedSuggested: string | null; - actualEvidenceGap: string | null; - expectedEvidenceGap: string | null; -} - export interface CodeExcerpt { startLine: number; endLine: number; @@ -62,12 +81,13 @@ export interface PrimerField { } export interface StoredReview { - version: 1; + version: 2; repository: string; githubRepository: string | null; prNumber: number; reviewedCommit: string; title: string; + originalVerdict: string; verdict: string; intent: string; primer: BusinessPrimer; @@ -216,12 +236,15 @@ function readReviewInput(value: unknown): ReviewInput { }; } -function missingScenario(): FindingScenario { +function missingScenario( + actualEvidenceGap = 'Evidence gap: the scenario sidecar does not provide a specific triggering setup/reachability and observable outcome.', +): FindingScenario { return { actualHappens: null, expectedSuggested: null, - actualEvidenceGap: - 'Evidence gap: the scenario sidecar does not provide a specific triggering setup/action and observable failure.', + actualTriggerEvidence: null, + actualOutcomeEvidence: null, + actualEvidenceGap, expectedEvidenceGap: 'Evidence gap: the scenario sidecar does not provide a specific expected behavior and correction.', }; @@ -241,11 +264,39 @@ function normalizeScenario(value: unknown, field: string): FindingScenario { 8000, false, ); - const missing = missingScenario(); + const rawEvidence = value.what_actually_happens_evidence; + if (rawEvidence !== undefined && !isRecord(rawEvidence)) + throw new Error(`${field}.what_actually_happens_evidence must be an object`); + const triggerEvidence = rawEvidence + ? boundedString( + rawEvidence.triggering_setup, + `${field}.what_actually_happens_evidence.triggering_setup`, + 8000, + false, + ) + : null; + const outcomeEvidence = rawEvidence + ? boundedString( + rawEvidence.observable_outcome, + `${field}.what_actually_happens_evidence.observable_outcome`, + 8000, + false, + ) + : null; + const hasActualEvidence = Boolean( + actualHappens && triggerEvidence && outcomeEvidence, + ); + const missing = missingScenario( + actualHappens + ? 'Evidence gap: the asserted scenario lacks both a cited triggering setup/reachability and observable outcome; treat it as an open question, not an active defect.' + : undefined, + ); return { - actualHappens, + actualHappens: hasActualEvidence ? actualHappens : null, expectedSuggested, - actualEvidenceGap: actualHappens ? null : missing.actualEvidenceGap, + actualTriggerEvidence: hasActualEvidence ? triggerEvidence : null, + actualOutcomeEvidence: hasActualEvidence ? outcomeEvidence : null, + actualEvidenceGap: hasActualEvidence ? null : missing.actualEvidenceGap, expectedEvidenceGap: expectedSuggested ? null : missing.expectedEvidenceGap, }; } @@ -298,15 +349,19 @@ function normalizeFinding(value: unknown): ReviewFinding { ) { throw new Error('finding.confidence must be a string or number'); } + const title = boundedString(value.title, 'finding.title', 1000) as string; + const scenario = missingScenario(); + const claim: FindingClaim = { title, severity, requiredResponse, scenario }; return { id: findingId(value['#']), - title: boundedString(value.title, 'finding.title', 1000) as string, - severity, + ...claim, + status: 'active', + original: { ...claim, scenario: { ...scenario } }, + revisions: [], file: safeRelativePath(value.file, 'finding.file'), line, endLine, confidence, - requiredResponse, reviewers: stringArray(value.reviewers, 'finding.reviewers'), evidence: stringArray(value.evidence, 'finding.evidence', 64), firstEvidence: boundedString( @@ -317,7 +372,6 @@ function normalizeFinding(value: unknown): ReviewFinding { ), sourceLink: null, excerpts: { before: null, after: null }, - scenario: missingScenario(), }; } @@ -547,13 +601,14 @@ function validBaseCommit(value: string | undefined): string | null { async function writeJsonAtomically( path: string, value: unknown, + maximumBytes = MAX_REQUIREMENTS_BYTES, ): Promise { + const serialized = `${JSON.stringify(value, null, 2)}\n`; + if (Buffer.byteLength(serialized, 'utf8') > maximumBytes) + throw new Error(`${basename(path)} exceeds ${maximumBytes} bytes`); await mkdir(resolve(path, '..'), { recursive: true, mode: 0o700 }); const temporary = `${path}.${randomUUID()}.tmp`; - await writeFile(temporary, `${JSON.stringify(value, null, 2)}\n`, { - encoding: 'utf8', - mode: 0o600, - }); + await writeFile(temporary, serialized, { encoding: 'utf8', mode: 0o600 }); await rename(temporary, path); } @@ -584,6 +639,26 @@ async function readScenarioSidecar( return scenarios; } +function preserveFindingLifecycle( + fresh: ReviewFinding, + previous: ReviewFinding | undefined, +): ReviewFinding { + if (!previous?.revisions.length) return fresh; + return { + ...fresh, + title: previous.title, + severity: previous.severity, + requiredResponse: previous.requiredResponse, + scenario: { ...previous.scenario }, + status: previous.status, + original: { + ...previous.original, + scenario: { ...previous.original.scenario }, + }, + revisions: [...previous.revisions], + }; +} + export async function prepareReview( options: PrepareOptions, ): Promise { @@ -628,47 +703,90 @@ export async function prepareReview( new Set(rawFindings.map((finding) => finding.id)), ) : new Map(); - const findings = rawFindings.map((finding) => ({ - ...finding, - scenario: scenarios.get(finding.id) ?? finding.scenario, - sourceLink: buildGitHubLineLink( - githubRepository, - artifact.scope.head_sha, - finding.file, - finding.line, - finding.endLine, - ), - excerpts: { - before: baseCommit - ? codeExcerpt(repoPath, baseCommit, finding.file, finding.line) - : null, - after: codeExcerpt( - repoPath, + const findings = rawFindings.map((finding) => { + const scenario = scenarios.get(finding.id) ?? finding.scenario; + return { + ...finding, + scenario, + original: { ...finding.original, scenario: { ...scenario } }, + sourceLink: buildGitHubLineLink( + githubRepository, artifact.scope.head_sha, finding.file, finding.line, + finding.endLine, ), - }, - })); + excerpts: { + before: baseCommit + ? codeExcerpt(repoPath, baseCommit, finding.file, finding.line) + : null, + after: codeExcerpt( + repoPath, + artifact.scope.head_sha, + finding.file, + finding.line, + ), + }, + }; + }); const workspace = workspaceFor( stateRoot(options.dataDir), repositoryStorageKey(remote, githubRepository), target.number, ); - const review: StoredReview = { - version: 1, - repository: githubRepository ?? repositoryStorageKey(remote, null), - githubRepository, - prNumber: target.number, - reviewedCommit: artifact.scope.head_sha, - title: artifact.title ?? `Pull request #${target.number}`, - verdict: artifact.verdict, - intent: artifact.intent, - primer: buildBusinessPrimer(specification, artifact.intent), - findings, - generatedAt: (options.now ?? new Date()).toISOString(), - }; - await writeJsonAtomically(join(workspace, 'review.json'), review); + const review = await withLock(join(workspace, 'review.json'), async () => { + let previousReview: StoredReview | null = null; + try { + previousReview = await loadStoredReview(workspace); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; + } + if ( + previousReview?.reviewedCommit !== artifact.scope.head_sha && + previousReview?.findings.some((finding) => finding.revisions.length) + ) { + throw new Error( + 'Refusing to replace lifecycle history from a different reviewed commit', + ); + } + if ( + previousReview?.findings.some( + (finding) => + finding.revisions.length && + !findings.some((candidate) => candidate.id === finding.id), + ) + ) { + throw new Error( + 'Refusing to discard lifecycle history for a finding missing from the review artifact', + ); + } + const previousFindings = new Map( + previousReview?.findings.map((finding) => [finding.id, finding]) ?? [], + ); + const lifecycleFindings = findings.map((finding) => + preserveFindingLifecycle(finding, previousFindings.get(finding.id)), + ); + const next: StoredReview = { + version: 2, + repository: githubRepository ?? repositoryStorageKey(remote, null), + githubRepository, + prNumber: target.number, + reviewedCommit: artifact.scope.head_sha, + title: artifact.title ?? `Pull request #${target.number}`, + originalVerdict: previousReview?.findings.some( + (finding) => finding.revisions.length, + ) + ? previousReview.originalVerdict + : artifact.verdict, + verdict: currentVerdict(lifecycleFindings), + intent: artifact.intent, + primer: buildBusinessPrimer(specification, artifact.intent), + findings: lifecycleFindings, + generatedAt: (options.now ?? new Date()).toISOString(), + }; + await writeJsonAtomically(join(workspace, 'review.json'), next); + return next; + }); const commentsPath = join(workspace, 'comments.json'); try { await readFile(commentsPath, 'utf8'); @@ -682,15 +800,116 @@ export async function prepareReview( return { workspace, review }; } +function storedScenario(value: unknown, field: string): FindingScenario { + if (!isRecord(value)) return missingScenario(); + return normalizeScenario( + { + what_actually_happens: value.actualHappens, + expected_suggested: value.expectedSuggested, + what_actually_happens_evidence: { + triggering_setup: value.actualTriggerEvidence, + observable_outcome: value.actualOutcomeEvidence, + }, + }, + field, + ); +} + +function legacyScenario(value: unknown, field: string): FindingScenario { + const normalized = storedScenario(value, field); + if (!isRecord(value)) return normalized; + const actualHappens = boundedString( + value.actualHappens, + `${field}.actualHappens`, + 8000, + false, + ); + return actualHappens + ? { + ...normalized, + actualHappens, + actualEvidenceGap: + 'Legacy scenario retained for audit: it lacks separate reachability and observable-outcome provenance.', + } + : normalized; +} +function currentVerdict(findings: ReviewFinding[]): string { + const active = findings.filter((finding) => finding.status === 'active'); + const questions = findings.filter((finding) => finding.status === 'question'); + if (!active.length) + return questions.length + ? `No active findings; ${questions.length} open question${questions.length === 1 ? '' : 's'}` + : 'No active findings'; + const bySeverity = [...SEVERITIES] + .filter((severity) => active.some((finding) => finding.severity === severity)) + .map( + (severity) => + `${severity}: ${active.filter((finding) => finding.severity === severity).length}`, + ); + return `${active.length} active finding${active.length === 1 ? '' : 's'} (${bySeverity.join(', ')})${questions.length ? `; ${questions.length} open question${questions.length === 1 ? '' : 's'}` : ''}`; +} + +function migrateStoredReview(value: unknown): StoredReview { + if (!isRecord(value) || !Array.isArray(value.findings)) + throw new Error('Invalid stored review'); + if (value.version === 2) return value as unknown as StoredReview; + if (value.version !== 1) throw new Error('Unsupported stored review version'); + const originalVerdict = boundedString(value.verdict, 'review.verdict', 200) as string; + const findings = value.findings.map((rawFinding, index) => { + const scenario = storedScenario(rawFinding.scenario, `review.findings[${index}].scenario`); + const originalScenario = legacyScenario( + rawFinding.scenario, + `review.findings[${index}].scenario`, + ); + const title = boundedString( + rawFinding.title, + `review.findings[${index}].title`, + 1000, + ) as string; + const severity = boundedString( + rawFinding.severity, + `review.findings[${index}].severity`, + 2, + ) as ReviewFinding['severity']; + if (!SEVERITIES.has(severity)) + throw new Error(`review.findings[${index}].severity is invalid`); + const requiredResponse = boundedString( + rawFinding.requiredResponse, + `review.findings[${index}].requiredResponse`, + 8000, + ) as string; + const claim: FindingClaim = { title, severity, requiredResponse, scenario }; + const original: FindingClaim = { + ...claim, + scenario: originalScenario, + }; + return { + ...(rawFinding as unknown as Omit), + ...claim, + status: 'active' as const, + original, + revisions: [], + }; + }); + return { + ...(value as unknown as Omit), + version: 2, + originalVerdict, + verdict: currentVerdict(findings), + findings, + }; +} + export async function loadStoredReview( workspace: string, ): Promise { - return JSON.parse( - await readBoundedFile( - join(resolve(workspace), 'review.json'), - MAX_REQUIREMENTS_BYTES, - ), - ) as StoredReview; + const path = join(resolve(workspace), 'review.json'); + const raw = JSON.parse( + await readBoundedFile(path, MAX_REQUIREMENTS_BYTES), + ) as unknown; + const review = migrateStoredReview(raw); + if (raw !== review) await writeJsonAtomically(path, review); + return review; } async function readCommentStore(workspace: string): Promise { @@ -715,6 +934,7 @@ async function withLock( path: string, operation: () => Promise, ): Promise { + await mkdir(resolve(path, '..'), { recursive: true, mode: 0o700 }); const lock = `${path}.lock`; for (let attempt = 0; attempt < 100; attempt += 1) { try { @@ -741,6 +961,73 @@ function commentAuthor(value: unknown): string { return author ? author.replace(/[\r\n]/g, ' ') : 'Reviewer'; } +function revisedScenario( + value: unknown, + current: FindingScenario, +): FindingScenario { + if (!isRecord(value)) throw new Error('revision.scenario must be an object'); + return normalizeScenario( + { + what_actually_happens: + value.actualHappens === undefined + ? current.actualHappens + : value.actualHappens, + expected_suggested: + value.expectedSuggested === undefined + ? current.expectedSuggested + : value.expectedSuggested, + what_actually_happens_evidence: { + triggering_setup: + value.actualTriggerEvidence === undefined + ? current.actualTriggerEvidence + : value.actualTriggerEvidence, + observable_outcome: + value.actualOutcomeEvidence === undefined + ? current.actualOutcomeEvidence + : value.actualOutcomeEvidence, + }, + }, + 'revision.scenario', + ); +} + +function revisionFor( + body: JsonObject, + finding: ReviewFinding, +): Omit { + const status = boundedString(body.status, 'status', 16) as FindingStatus; + if (!['active', 'question', 'withdrawn'].includes(status)) + throw new Error('status must be active, question, or withdrawn'); + const commentId = boundedString(body.commentId, 'commentId', 36) as string; + if (!/^[0-9a-f]{8}-(?:[0-9a-f]{4}-){3}[0-9a-f]{12}$/.test(commentId)) + throw new Error('commentId must be a UUID'); + const rationale = boundedString(body.rationale, 'rationale', 8000) as string; + const changes: Partial = {}; + if (body.title !== undefined) + changes.title = boundedString(body.title, 'title', 1000) as string; + if (body.severity !== undefined) { + const severity = boundedString(body.severity, 'severity', 2) as ReviewFinding['severity']; + if (!SEVERITIES.has(severity)) + throw new Error('severity must be P0, P1, P2, or P3'); + changes.severity = severity; + } + if (body.requiredResponse !== undefined) + changes.requiredResponse = boundedString( + body.requiredResponse, + 'requiredResponse', + 8000, + ) as string; + if (body.scenario !== undefined) + changes.scenario = revisedScenario(body.scenario, finding.scenario); + return { commentId, status, rationale, changes }; +} + +function requestContentLength(request: Request): Response | null { + const contentLength = Number(request.headers.get('content-length') ?? '0'); + return contentLength > MAX_REQUEST_BYTES + ? textResponse('Request body is too large', 413) + : null; +} export async function readBoundedRequestBody( request: Request, ): Promise { @@ -835,12 +1122,12 @@ export function renderReviewPage(review: StoredReview): string { * { box-sizing: border-box; } body { margin: 0; } a { color: #164ea6; } button, input, textarea { font: inherit; } header { background: #13233f; color: #fff; padding: 1.25rem max(1rem, calc((100vw - 1200px) / 2)); } header p { margin: .35rem 0 0; color: #d8e4fa; overflow-wrap: anywhere; } .shell { width: min(1200px, calc(100% - 2rem)); min-width: 0; margin: 1.25rem auto 3rem; } .panel, article { min-width: 0; background: #fff; border: 1px solid #d9e0eb; border-radius: .75rem; box-shadow: 0 1px 2px #13233f0d; } -.panel { padding: 1.25rem; margin-bottom: 1rem; } h1,h2,h3 { margin-top: 0; overflow-wrap: anywhere; } h2 { font-size: 1.2rem; } h3 { font-size: 1rem; margin-bottom: .35rem; } +.panel { padding: 1.25rem; margin-bottom: 1rem; } h1,h2,h3 { margin-top: 0; overflow-wrap: anywhere; } h2 { font-size: 1.2rem; } h3 { font-size: 1rem; margin-bottom: .35rem; } p, li, label, strong, .meta, .gap { overflow-wrap: anywhere; word-break: break-word; } .context-grid { display: grid; grid-template-columns: repeat(2, minmax(0, 1fr)); gap: .8rem; } .context-item { min-width: 0; border-left: 3px solid #8aa9d6; padding-left: .7rem; } .context-item pre { white-space: pre-wrap; overflow-wrap: anywhere; } .gap { color: #7a2b17; font-style: italic; } .controls { display: grid; grid-template-columns: minmax(0, 1fr) auto; gap: .75rem; align-items: center; } .filters { display: flex; flex-wrap: wrap; gap: .4rem; } button { cursor: pointer; border: 1px solid #9aa9bd; border-radius: .35rem; background: #fff; padding: .4rem .65rem; } button[aria-pressed="true"] { background: #164ea6; color: #fff; border-color: #164ea6; } input, textarea { width: 100%; min-width: 0; border: 1px solid #9aa9bd; border-radius: .35rem; padding: .55rem; } textarea { min-height: 5rem; resize: vertical; } #findings { display: grid; min-width: 0; gap: 1rem; } -.finding { min-width: 0; padding: 1.2rem; } .finding-top, .finding-top > *, .excerpt-grid, .excerpt-grid > * { min-width: 0; } .finding-top { display: flex; align-items: flex-start; gap: .7rem; justify-content: space-between; } .tag { display: inline-block; border-radius: 999px; padding: .15rem .5rem; font-weight: 700; font-size: .78rem; background: #e8edf5; } .P0 { background: #ffe1df; color: #912018; } .P1 { background: #ffefd6; color: #824300; } .P2 { background: #e7f0ff; color: #164ea6; } .P3 { background: #edf0f4; color: #4c596d; } -.meta { color: #536176; font-size: .9rem; overflow-wrap: anywhere; } pre { max-width: 100%; min-width: 0; overflow-x: auto; white-space: pre; padding: .8rem; background: #101928; color: #e8eef8; border-radius: .35rem; font-size: .82rem; } .excerpt-grid { display: grid; grid-template-columns: repeat(2, minmax(0, 1fr)); gap: .75rem; } +.finding { min-width: 0; padding: 1.2rem; } .finding-top, .finding-top > *, .excerpt-grid, .excerpt-grid > * { min-width: 0; } .finding-top { display: flex; align-items: flex-start; gap: .7rem; justify-content: space-between; } .tag { display: inline-block; border-radius: 999px; padding: .15rem .5rem; font-weight: 700; font-size: .78rem; background: #e8edf5; } .P0 { background: #ffe1df; color: #912018; } .P1 { background: #ffefd6; color: #824300; } .P2 { background: #e7f0ff; color: #164ea6; } .P3 { background: #edf0f4; color: #4c596d; } .active { background: #ffe1df; color: #912018; } .question { background: #e7f0ff; color: #164ea6; } .withdrawn { background: #edf0f4; color: #4c596d; } +.meta { color: #536176; font-size: .9rem; } pre { max-width: 100%; min-width: 0; overflow-x: auto; white-space: pre; padding: .8rem; background: #101928; color: #e8eef8; border-radius: .35rem; font-size: .82rem; } .excerpt-grid { display: grid; grid-template-columns: repeat(2, minmax(0, 1fr)); gap: .75rem; } .comment { margin-top: .65rem; padding: .7rem; border-left: 3px solid #c5d0e0; background: #f8fafc; } .assistant { border-left-color: #38966a; } .comment p { white-space: pre-wrap; } .reply-form { margin-top: .6rem; } .hidden { display: none; } .empty { color: #536176; } .small { font-size: .85rem; } .warning { color: #7a2b17; } @media (max-width: 720px) { .shell { width: calc(100% - 1rem); margin-top: .5rem; } .context-grid, .excerpt-grid, .controls { grid-template-columns: 1fr; } .finding-top { display: block; } .finding-top .tag { margin-top: .5rem; } } @@ -850,13 +1137,13 @@ input, textarea { width: 100%; min-width: 0; border: 1px solid #9aa9bd; border-r
Interactive PR review

Loading structured review...

Business context

Context precedes architecture and findings. Missing evidence is explicit.

-

Findings

+

Findings

General comments

@@ -998,7 +1294,7 @@ function validateWriteRequest(request: Request, url: URL): Response | null { } const origin = request.headers.get('origin'); if (origin && origin !== url.origin) { - return textResponse('Cross-origin comment writes are not allowed', 403); + return textResponse('Cross-origin writes are not allowed', 403); } return null; } @@ -1009,18 +1305,15 @@ export function createReviewServer( port = 0, ) { const resolvedWorkspace = resolve(workspace); - let reviewPromise: Promise | null = null; - const review = async (): Promise => { - reviewPromise ??= loadStoredReview(resolvedWorkspace); - return reviewPromise; - }; + const review = (): Promise => loadStoredReview(resolvedWorkspace); return Bun.serve({ hostname: host, port, async fetch(request) { const url = new URL(request.url); try { - if (request.method === 'GET' && url.pathname === '/') + const pathname = decodeURIComponent(url.pathname); + if (request.method === 'GET' && pathname === '/') return new Response(renderReviewPage(await review()), { headers: { 'content-type': 'text/html; charset=utf-8', @@ -1029,9 +1322,9 @@ export function createReviewServer( "default-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-inline'; connect-src 'self'; base-uri 'none'; form-action 'self'", }, }); - if (request.method === 'GET' && url.pathname === '/api/review') + if (request.method === 'GET' && pathname === '/api/review') return json(await review()); - if (request.method === 'GET' && url.pathname === '/api/comments') { + if (request.method === 'GET' && pathname === '/api/comments') { const comments = (await readCommentStore(resolvedWorkspace)).comments; const unanswered = url.searchParams.get('status') === 'unanswered'; return json({ @@ -1045,14 +1338,11 @@ export function createReviewServer( : comments, }); } - if (request.method === 'POST' && url.pathname === '/api/comments') { + if (request.method === 'POST' && pathname === '/api/comments') { const writeError = validateWriteRequest(request, url); if (writeError) return writeError; - const contentLength = Number( - request.headers.get('content-length') ?? '0', - ); - if (contentLength > MAX_REQUEST_BYTES) - return textResponse('Request body is too large', 413); + const lengthError = requestContentLength(request); + if (lengthError) return lengthError; const body = parseJsonRequest(await readBoundedRequestBody(request)); const finding = body.findingId === null || body.findingId === undefined @@ -1088,16 +1378,13 @@ export function createReviewServer( return json({ comment }, 201); } const replyMatch = /^\/api\/comments\/([0-9a-f-]{36})\/replies$/.exec( - url.pathname, + pathname, ); if (request.method === 'POST' && replyMatch?.[1]) { const writeError = validateWriteRequest(request, url); if (writeError) return writeError; - const contentLength = Number( - request.headers.get('content-length') ?? '0', - ); - if (contentLength > MAX_REQUEST_BYTES) - return textResponse('Request body is too large', 413); + const lengthError = requestContentLength(request); + if (lengthError) return lengthError; const body = parseJsonRequest(await readBoundedRequestBody(request)); if (body.role !== 'assistant') return textResponse('Replies must declare assistant role', 400); @@ -1126,6 +1413,49 @@ export function createReviewServer( ); return json({ comment }, 201); } + const revisionMatch = /^\/api\/findings\/(#[1-9]\d*)\/revisions$/.exec( + pathname, + ); + if (request.method === 'POST' && revisionMatch?.[1]) { + const writeError = validateWriteRequest(request, url); + if (writeError) return writeError; + const lengthError = requestContentLength(request); + if (lengthError) return lengthError; + const body = parseJsonRequest(await readBoundedRequestBody(request)); + const result = await withLock( + join(resolvedWorkspace, 'review.json'), + async () => { + const current = await loadStoredReview(resolvedWorkspace); + const target = current.findings.find( + (item) => item.id === revisionMatch[1], + ); + if (!target) throw new Error('Finding not found'); + const revision = revisionFor(body, target); + const comment = ( + await readCommentStore(resolvedWorkspace) + ).comments.find((item) => item.id === revision.commentId); + if (!comment || comment.findingId !== target.id) + throw new Error( + 'commentId must identify a comment on this finding', + ); + const applied: FindingRevision = { + ...revision, + id: randomUUID(), + createdAt: new Date().toISOString(), + }; + Object.assign(target, applied.changes); + target.status = applied.status; + target.revisions.push(applied); + current.verdict = currentVerdict(current.findings); + await writeJsonAtomically( + join(resolvedWorkspace, 'review.json'), + current, + ); + return { finding: target, verdict: current.verdict }; + }, + ); + return json(result, 201); + } return textResponse('Not found', 404); } catch (error) { const message = @@ -1221,7 +1551,7 @@ export async function main( ); if (!LOOPBACK_HOSTS.has(host)) process.stderr.write( - 'WARNING: review site is exposed beyond loopback; anyone who can reach this host can read review data and submit local comments.\n', + 'WARNING: review site is exposed beyond loopback; anyone who can reach this host can read review data and submit local comments or lifecycle revisions.\n', ); const portText = option(values, 'port'); const port = portText === undefined ? 0 : Number(portText); diff --git a/tests/unit/plugins/pr-interactive-review.test.ts b/tests/unit/plugins/pr-interactive-review.test.ts index 4a08f0a..d70ff6a 100644 --- a/tests/unit/plugins/pr-interactive-review.test.ts +++ b/tests/unit/plugins/pr-interactive-review.test.ts @@ -15,6 +15,7 @@ import { buildBusinessPrimer, buildGitHubLineLink, createReviewServer, + loadStoredReview, prepareReview, readBoundedRequestBody, renderReviewPage, @@ -84,6 +85,12 @@ async function fixture(withScenario = true) { '#1': { what_actually_happens: 'When an operator submits a label containing markup, the page renders the markup and the browser executes it.', + what_actually_happens_evidence: { + triggering_setup: + 'tests/config-label.test.ts saves a label containing markup.', + observable_outcome: + 'tests/config-label.test.ts observes the label rendered as markup.', + }, expected_suggested: 'When an operator submits that label, the page displays the characters as text after encoding the value at the rendering boundary.', }, @@ -186,11 +193,65 @@ describe('pr-interactive-review', () => { const scenario = prepared.review.findings[0]?.scenario; expect(scenario?.actualHappens).toBeNull(); expect(scenario?.expectedSuggested).toBeNull(); - expect(scenario?.actualEvidenceGap).toContain('triggering setup/action'); + expect(scenario?.actualEvidenceGap).toContain('triggering setup/reachability'); expect(scenario?.expectedEvidenceGap).toContain( 'expected behavior and correction', ); }); + it('downgrades sidecar scenarios without reachability and outcome evidence', async () => { + const prepared = await fixture(); + await writeFile( + prepared.scenarios, + JSON.stringify({ + '#1': { + what_actually_happens: + 'A polished but unsupported configuration path fails.', + expected_suggested: 'The configuration path succeeds.', + }, + }), + ); + const refreshed = await prepareReview({ + reviewJsonPath: prepared.artifact, + pr: '123', + repoPath: prepared.repository, + dataDir: prepared.state, + scenariosPath: prepared.scenarios, + }); + expect(refreshed.review.findings[0]?.scenario).toEqual( + expect.objectContaining({ + actualHappens: null, + actualEvidenceGap: expect.stringContaining('asserted scenario lacks'), + expectedSuggested: 'The configuration path succeeds.', + }), + ); + }); + it('labels an evidence-only scenario as a gap without an actual claim', async () => { + const prepared = await fixture(); + await writeFile( + prepared.scenarios, + JSON.stringify({ + '#1': { + what_actually_happens_evidence: { + triggering_setup: 'A reachable test fixture.', + observable_outcome: 'An observed test result.', + }, + }, + }), + ); + const refreshed = await prepareReview({ + reviewJsonPath: prepared.artifact, + pr: '123', + repoPath: prepared.repository, + dataDir: prepared.state, + scenariosPath: prepared.scenarios, + }); + expect(refreshed.review.findings[0]?.scenario).toEqual( + expect.objectContaining({ + actualHappens: null, + actualEvidenceGap: expect.stringContaining('specific triggering'), + }), + ); + }); it('merges scenario sidecars by stable finding ID and rejects invalid entries', async () => { const prepared = await fixture(); @@ -240,6 +301,10 @@ describe('pr-interactive-review', () => { 'When an operator submits a label containing markup, the page renders the markup and the browser executes it.', expectedSuggested: 'When an operator submits that label, the page displays the characters as text after encoding the value at the rendering boundary.', + actualTriggerEvidence: + 'tests/config-label.test.ts saves a label containing markup.', + actualOutcomeEvidence: + 'tests/config-label.test.ts observes the label rendered as markup.', actualEvidenceGap: null, expectedEvidenceGap: null, }); @@ -267,16 +332,19 @@ describe('pr-interactive-review', () => { expect(page).not.toContain('Assistant reply'); }); - it('contains long reviewed lines within finding cards and scrollable code blocks', async () => { + it('contains long prose fields and reviewed lines without breaking narrow cards', async () => { const prepared = await fixture(); - const longSourceLine = 'x'.repeat(4096); + const longText = 'x'.repeat(4096); const review: StoredReview = { ...prepared.review, + title: longText, findings: prepared.review.findings.map((finding) => ({ ...finding, + title: longText, + scenario: { ...finding.scenario, actualHappens: longText }, excerpts: { before: null, - after: { startLine: 1, endLine: 1, content: longSourceLine }, + after: { startLine: 1, endLine: 1, content: longText }, }, })), }; @@ -284,14 +352,56 @@ describe('pr-interactive-review', () => { expect(review.findings[0]?.excerpts.after?.content).toHaveLength(4096); expect(page).toContain('#findings { display: grid; min-width: 0;'); expect(page).toContain('.finding { min-width: 0;'); + expect(page).toContain('aria-label="Finding status filters"'); + expect(page).toContain("withdrawn: 'Withdrawn findings'"); + expect(page).toContain("el('h3', 'Lifecycle history')"); expect(page).toContain( '.finding-top, .finding-top > *, .excerpt-grid, .excerpt-grid > * { min-width: 0; }', ); + expect(page).toContain('p, li, label, strong, .meta, .gap { overflow-wrap: anywhere; word-break: break-word; }'); expect(page).toContain( 'pre { max-width: 100%; min-width: 0; overflow-x: auto; white-space: pre;', ); }); + it('migrates version-1 reviews to lifecycle records without asserting unproven scenarios', async () => { + const prepared = await fixture(); + const legacy = JSON.parse( + await readFile(join(prepared.workspace, 'review.json'), 'utf8'), + ) as Record; + legacy.version = 1; + delete legacy.originalVerdict; + const finding = (legacy.findings as Array>)[0]; + delete finding.status; + delete finding.original; + delete finding.revisions; + delete finding.scenario; + finding.scenario = { + actualHappens: 'An unproven path currently fails.', + expectedSuggested: 'The path succeeds.', + actualEvidenceGap: null, + expectedEvidenceGap: null, + }; + legacy.verdict = 'Ready with fixes'; + await writeFile(join(prepared.workspace, 'review.json'), JSON.stringify(legacy)); + const migrated = await loadStoredReview(prepared.workspace); + expect(migrated.version).toBe(2); + expect(migrated.originalVerdict).toBe('Ready with fixes'); + expect(migrated.verdict).toBe('1 active finding (P1: 1)'); + expect(migrated.findings[0]).toEqual( + expect.objectContaining({ status: 'active', revisions: [] }), + ); + expect(migrated.findings[0]?.scenario.actualHappens).toBeNull(); + expect(migrated.findings[0]?.scenario.actualEvidenceGap).toContain( + 'asserted scenario lacks', + ); + expect(migrated.findings[0]?.original.scenario.actualHappens).toBe( + 'An unproven path currently fails.', + ); + expect( + JSON.parse(await readFile(join(prepared.workspace, 'review.json'), 'utf8')), + ).toEqual(migrated); + }); it('validates routes, atomically saves comments, and renders assistant replies', async () => { const prepared = await fixture(); const server = createReviewServer(prepared.workspace, '127.0.0.1', 0); @@ -425,4 +535,135 @@ describe('pr-interactive-review', () => { server.stop(true); } }); + it('reclassifies and withdraws findings through comment-linked atomic revisions', async () => { + const prepared = await fixture(); + const server = createReviewServer(prepared.workspace, '127.0.0.1', 0); + try { + const base = `http://127.0.0.1:${server.port}`; + const createComment = async (body: string) => { + const response = await fetch(`${base}/api/comments`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ findingId: '#1', body }), + }); + expect(response.status).toBe(201); + return (await response.json()) as { comment: { id: string } }; + }; + const first = await createComment( + 'The repository has no saved configuration that reaches this path.', + ); + const route = `${base}/api/findings/${encodeURIComponent('#1')}/revisions`; + expect( + ( + await fetch(route, { + method: 'POST', + headers: { 'content-type': 'text/plain' }, + body: '{}', + }) + ).status, + ).toBe(415); + expect( + ( + await fetch(route, { + method: 'POST', + headers: { + 'content-type': 'application/json', + origin: 'https://untrusted.example', + }, + body: JSON.stringify({}), + }) + ).status, + ).toBe(403); + expect( + ( + await fetch(route, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ + commentId: '00000000-0000-0000-0000-000000000000', + status: 'withdrawn', + rationale: 'Not linked.', + }), + }) + ).status, + ).toBe(400); + const question = await fetch(route, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ + commentId: first.comment.id, + status: 'question', + rationale: 'Reachability remains unproven after reviewing the correction.', + severity: 'P3', + scenario: { + actualHappens: null, + actualTriggerEvidence: null, + actualOutcomeEvidence: null, + }, + }), + }); + expect(question.status).toBe(201); + const questioned = (await (await fetch(`${base}/api/review`)).json()) as StoredReview; + expect(questioned.verdict).toBe('No active findings; 1 open question'); + expect(questioned.findings[0]).toEqual( + expect.objectContaining({ status: 'question', severity: 'P3' }), + ); + const second = await createComment( + 'The fixture and production registry confirm the path is unreachable.', + ); + const withdrawn = await fetch(route, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ + commentId: second.comment.id, + status: 'withdrawn', + rationale: 'The correction disproves the affected configuration path.', + }), + }); + expect(withdrawn.status).toBe(201); + const revised = (await (await fetch(`${base}/api/review`)).json()) as StoredReview; + expect(revised.verdict).toBe('No active findings'); + expect(revised.findings[0]).toEqual( + expect.objectContaining({ + status: 'withdrawn', + original: expect.objectContaining({ + title: 'Escape untrusted label', + severity: 'P1', + }), + revisions: [ + expect.objectContaining({ status: 'question', commentId: first.comment.id }), + expect.objectContaining({ status: 'withdrawn', commentId: second.comment.id }), + ], + }), + ); + const comments = (await (await fetch(`${base}/api/comments`)).json()) as { + comments: Array<{ body: string }>; + }; + expect(comments.comments.map((comment) => comment.body)).toEqual([ + 'The repository has no saved configuration that reaches this path.', + 'The fixture and production registry confirm the path is unreachable.', + ]); + expect( + (await readdir(prepared.workspace)).some((name) => name.endsWith('.tmp')), + ).toBe(false); + const reprepared = await prepareReview({ + reviewJsonPath: prepared.artifact, + pr: '123', + repoPath: prepared.repository, + dataDir: prepared.state, + scenariosPath: prepared.scenarios, + }); + expect(reprepared.review.findings[0]).toEqual( + expect.objectContaining({ + status: 'withdrawn', + severity: 'P3', + revisions: expect.arrayContaining([ + expect.objectContaining({ status: 'withdrawn' }), + ]), + }), + ); + } finally { + server.stop(true); + } + }); });