diff --git a/plugins/engineering/skills/pr-interactive-review/SKILL.md b/plugins/engineering/skills/pr-interactive-review/SKILL.md index 494de40..1ed3ccb 100644 --- a/plugins/engineering/skills/pr-interactive-review/SKILL.md +++ b/plugins/engineering/skills/pr-interactive-review/SKILL.md @@ -54,6 +54,50 @@ Non-goals: Redesigning permission roles ``` `--requirements` remains a direct file-read option only for a repository-relative path. The helper rejects paths outside the repository and `.git`; use host extraction plus `--spec` for any external specification reference. +### Reproducible presentation + +Create `interactive-presentation.json` beside the scenario sidecar for every review. This is a constrained content model for the shared renderer, not a per-PR template: never generate custom HTML, CSS, or JavaScript. Ground every sentence in the structured review, supplied requirements, or reviewed repository evidence. + +```json +{ + "eyebrow": "Runtime seam review", + "headline": "One queue connects two services.", + "summary": "The change establishes the first validated Dispatcher-to-Runner workflow handoff.", + "context_cards": [ + { + "label": "System boundary", + "title": "Dispatcher to Runner", + "body": "The Dispatcher starts resolved work; the Runner validates and completes the workflow.", + "tone": "neutral" + }, + { + "label": "Operational risk", + "title": "A false-ready worker hides the broken seam", + "body": "The smoke must prove polling, not merely observe a startup log.", + "tone": "problem" + } + ], + "mental_model": { + "title": "The execution path", + "summary": "Each stage owns one boundary and hands a single contract forward.", + "steps": [ + { + "label": "01 / Dispatch", + "title": "Start workflow", + "body": "Create a workflow on the configured Runner queue." + }, + { + "label": "02 / Validate", + "title": "Reject malformed input", + "body": "Validate the shared payload before any execution work." + } + ] + } +} +``` + +`eyebrow`, `headline`, and `summary` are required. Use at most six context cards and six mental-model steps. Card `tone` is `neutral`, `problem`, or `outcome`. Keep labels short, make the headline state the review's central conclusion, and use the mental model only when a real sequence or system boundary helps the reviewer decide. Omit `mental_model` rather than inventing decorative steps. The renderer supplies the navigation rail, verdict, metrics, cards, disclosures, finding layout, and responsive behavior; the sidecar supplies only grounded copy. + ## Create a reusable workspace @@ -64,6 +108,7 @@ SKILL_DIR="" bun "$SKILL_DIR/scripts/review-site.ts" prepare \ --review-json "/review.json" \ --scenarios "/interactive-scenarios.json" \ + --presentation "/interactive-presentation.json" \ --pr 123 \ --spec "Who configures: Release managers Operational problem: Manual approval queues delay configuration changes @@ -80,11 +125,13 @@ For a repository-relative requirements reference, use: bun "$SKILL_DIR/scripts/review-site.ts" prepare \ --review-json "/review.json" \ --scenarios "/interactive-scenarios.json" \ + --presentation "/interactive-presentation.json" \ --pr https://github.com/example-org/sample-service/pull/123 \ --requirements docs/requirements.md ``` -`prepare` prints the per-repository, per-PR workspace path. It validates the review artifact, scenario sidecar, PR identifier, finding file paths, sizes, and requirements reference. It generates GitHub source links only when the runtime `origin` remote is GitHub. Links pin the reviewed commit and exact cited line range. Non-GitHub remotes receive no external link. +`prepare` prints the per-repository, per-PR workspace path. It validates the review artifact, scenario and presentation sidecars, PR identifier, finding file paths, sizes, and requirements reference. It generates GitHub source links only when the runtime `origin` remote is GitHub. Links pin the reviewed commit and exact cited line range. Non-GitHub remotes receive no external link. Existing workspaces without presentation metadata still render through the same shell using the review title, intent, and primer fields as safe fallbacks. + For focused code context, `prepare` reads only the cited relative paths at the reviewed commit. Pass `--base-commit ` only when the review already supplied a verified exact base SHA; the helper does not rediscover PR scope. When no base or reviewed object is locally readable, the site labels that gap instead of substituting current-worktree content. @@ -94,7 +141,8 @@ For focused code context, `prepare` reads only the cited relative paths at the r bun "$SKILL_DIR/scripts/review-site.ts" serve --workspace "" ``` -Open the printed loopback URL. The page places **Business context** before findings, includes severity navigation and search, exact reviewed-commit links, required response, reviewers, confidence, structured evidence, available focused excerpts, and comments. +Open the printed loopback URL. The shared page renders a sticky review map, current verdict, headline and metrics, business context, an optional mental model, status-grouped findings, search and filters, exact reviewed-commit links, paired actual/expected scenarios, required responses, and local comments. Supplied requirements, referenced evidence, lifecycle history, code excerpts, and per-finding discussion use collapsed disclosures so large reviews remain scannable. + LAN or public exposure is opt-in and must be deliberate: @@ -135,5 +183,5 @@ Refresh the site and unanswered queue until the queue is empty. Never treat this ## Completion 1. Confirm the review artifact was consumed as JSON, not markdown. -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. +2. Browser-check the local site with representative review data, not an empty fixture: the desktop view has the navigation rail, verdict, hero metrics, business context, and indexed finding links; the narrow view stacks cleanly with no document-level horizontal overflow; raw requirements, evidence, and code are closed by default; active findings, open questions, and withdrawn findings are visibly separate; verdict/counts exclude withdrawn findings; every finding shows paired `What actually happens` and `Expected / suggested` content (or an explicit evidence gap); severity/status filters and search work; a local comment, assistant reply, and lifecycle revision render; and 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 9445936..31ea2b0 100644 --- a/plugins/engineering/skills/pr-interactive-review/scripts/review-site.ts +++ b/plugins/engineering/skills/pr-interactive-review/scripts/review-site.ts @@ -79,6 +79,33 @@ export interface PrimerField { value: string | null; evidenceGap: string | null; } +export type PresentationTone = 'neutral' | 'problem' | 'outcome'; + +export interface PresentationCard { + label: string; + title: string; + body: string; + tone: PresentationTone; +} + +export interface PresentationStep { + label: string; + title: string; + body: string; +} + +export interface ReviewPresentation { + eyebrow: string; + headline: string; + summary: string; + contextCards: PresentationCard[]; + mentalModel: { + title: string; + summary: string | null; + steps: PresentationStep[]; + } | null; +} + export interface StoredReview { version: 2; @@ -91,6 +118,8 @@ export interface StoredReview { verdict: string; intent: string; primer: BusinessPrimer; + presentation: ReviewPresentation | null; + findings: ReviewFinding[]; generatedAt: string; } @@ -133,6 +162,8 @@ export interface PrepareOptions { repoPath?: string; dataDir?: string; scenariosPath?: string; + presentationPath?: string; + specification?: string; requirementsPath?: string; baseCommit?: string; @@ -638,6 +669,125 @@ async function readScenarioSidecar( } return scenarios; } +function recordArray( + value: unknown, + field: string, + maximumEntries: number, +): JsonObject[] { + if (value === undefined) return []; + if (!Array.isArray(value) || value.length > maximumEntries) + throw new Error( + `${field} must be an array of at most ${maximumEntries} objects`, + ); + return value.map((item, index) => { + if (!isRecord(item)) throw new Error(`${field}[${index}] must be an object`); + return item; + }); +} + +function normalizePresentation( + value: unknown, + field = 'presentation', +): ReviewPresentation | null { + if (value === undefined || value === null) return null; + if (!isRecord(value)) throw new Error(`${field} must be an object`); + const contextCards = recordArray( + value.context_cards ?? value.contextCards, + `${field}.context_cards`, + 6, + ).map((card, index): PresentationCard => { + const tone = (boundedString( + card.tone, + `${field}.context_cards[${index}].tone`, + 16, + false, + ) ?? 'neutral') as PresentationTone; + if (!['neutral', 'problem', 'outcome'].includes(tone)) + throw new Error( + `${field}.context_cards[${index}].tone must be neutral, problem, or outcome`, + ); + return { + label: boundedString( + card.label, + `${field}.context_cards[${index}].label`, + 120, + ) as string, + title: boundedString( + card.title, + `${field}.context_cards[${index}].title`, + 300, + ) as string, + body: boundedString( + card.body, + `${field}.context_cards[${index}].body`, + 3000, + ) as string, + tone, + }; + }); + const rawMentalModel = value.mental_model ?? value.mentalModel; + let mentalModel: ReviewPresentation['mentalModel'] = null; + if (rawMentalModel !== undefined && rawMentalModel !== null) { + if (!isRecord(rawMentalModel)) + throw new Error(`${field}.mental_model must be an object`); + const steps = recordArray( + rawMentalModel.steps, + `${field}.mental_model.steps`, + 6, + ); + if (!steps.length) + throw new Error(`${field}.mental_model.steps must not be empty`); + mentalModel = { + title: boundedString( + rawMentalModel.title, + `${field}.mental_model.title`, + 300, + ) as string, + summary: boundedString( + rawMentalModel.summary, + `${field}.mental_model.summary`, + 3000, + false, + ), + steps: steps.map((step, index) => ({ + label: boundedString( + step.label, + `${field}.mental_model.steps[${index}].label`, + 120, + ) as string, + title: boundedString( + step.title, + `${field}.mental_model.steps[${index}].title`, + 300, + ) as string, + body: boundedString( + step.body, + `${field}.mental_model.steps[${index}].body`, + 3000, + ) as string, + })), + }; + } + return { + eyebrow: boundedString(value.eyebrow, `${field}.eyebrow`, 200) as string, + headline: boundedString(value.headline, `${field}.headline`, 500) as string, + summary: boundedString(value.summary, `${field}.summary`, 3000) as string, + contextCards, + mentalModel, + }; +} + +async function readPresentationSidecar( + path: string, +): Promise { + const value = JSON.parse( + await readBoundedFile(resolve(path), MAX_REQUIREMENTS_BYTES), + ) as unknown; + const presentation = normalizePresentation(value); + if (!presentation) throw new Error('presentation must be an object'); + return presentation; +} + function preserveFindingLifecycle( fresh: ReviewFinding, @@ -703,6 +853,10 @@ export async function prepareReview( new Set(rawFindings.map((finding) => finding.id)), ) : new Map(); + const preparedPresentation = options.presentationPath + ? await readPresentationSidecar(options.presentationPath) + : undefined; + const findings = rawFindings.map((finding) => { const scenario = scenarios.get(finding.id) ?? finding.scenario; return { @@ -781,6 +935,11 @@ export async function prepareReview( verdict: currentVerdict(lifecycleFindings), intent: artifact.intent, primer: buildBusinessPrimer(specification, artifact.intent), + presentation: + preparedPresentation ?? + (previousReview?.reviewedCommit === artifact.scope.head_sha + ? previousReview.presentation + : null), findings: lifecycleFindings, generatedAt: (options.now ?? new Date()).toISOString(), }; @@ -852,8 +1011,19 @@ function currentVerdict(findings: ReviewFinding[]): string { 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 === 2) { + if (value.presentation !== undefined) { + normalizePresentation(value.presentation, 'review.presentation'); + return value as unknown as StoredReview; + } + return { + ...(value as unknown as StoredReview), + presentation: null, + }; + } 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`); @@ -896,6 +1066,8 @@ function migrateStoredReview(value: unknown): StoredReview { version: 2, originalVerdict, verdict: currentVerdict(findings), + presentation: normalizePresentation(value.presentation, 'review.presentation'), + findings, }; } @@ -1118,167 +1290,550 @@ export function renderReviewPage(review: StoredReview): string { ${htmlEscape(review.title)} - Interactive review -
Interactive PR review

Loading structured review...

-
-

Business context

Context precedes architecture and findings. Missing evidence is explicit.

-

Findings

-
-

General comments

-
+ +
+ +
+
+

Structured pull request review

+

Loading review…

+

+
+
+
+
+
+

Business primer

Understand the change first

+

The operational context stays ahead of implementation detail. Missing context is explicit but does not dominate the review.

+
+
+
+ + +
+

Review findings

Inspect the failure path

+
+
+ +
+
+
+
+

Discussion

General comments

+

Add a general comment

+
+
+
+
+
@@ -1521,6 +2076,8 @@ export async function main( 'repo', 'data-dir', 'scenarios', + 'presentation', + 'spec', 'requirements', 'base-commit', @@ -1531,6 +2088,8 @@ export async function main( repoPath: option(values, 'repo'), dataDir: option(values, 'data-dir'), scenariosPath: option(values, 'scenarios'), + presentationPath: option(values, 'presentation'), + specification: option(values, 'spec'), requirementsPath: option(values, 'requirements'), baseCommit: option(values, 'base-commit'), @@ -1564,7 +2123,7 @@ export async function main( return; } throw new Error( - 'Usage: review-site.ts prepare --review-json --scenarios --pr [--spec | --requirements ] [--base-commit ]\n review-site.ts serve --workspace [--host 127.0.0.1] [--port 0] [--expose]', + 'Usage: review-site.ts prepare --review-json --scenarios --pr [--presentation ] [--spec | --requirements ] [--base-commit ]\n review-site.ts serve --workspace [--host 127.0.0.1] [--port 0] [--expose]', ); } diff --git a/src/core/marketplace.ts b/src/core/marketplace.ts index 954bc63..e6900de 100644 --- a/src/core/marketplace.ts +++ b/src/core/marketplace.ts @@ -885,6 +885,15 @@ export async function findMarketplace( await findMarketplaceRegistration(name, sourceLocation, workspacePath) )?.entry ?? null; } +interface MarketplaceUpdateGitClient { + raw(args: string[]): Promise; + checkout(branch: string): Promise; +} + +interface MarketplaceUpdateDeps { + createGit(path: string): MarketplaceUpdateGitClient; + pull(path: string): Promise; +} /** * Update marketplace(s) by pulling latest changes @@ -893,6 +902,7 @@ export async function findMarketplace( export async function updateMarketplace( name?: string, workspacePath?: string, + deps: Partial = {}, ): Promise> { const userRegistry = await loadRegistry(); let projectRegistry: MarketplaceRegistry | undefined; @@ -991,7 +1001,7 @@ export async function updateMarketplace( const storedBranch = marketplace.source.type === 'github' ? parseLocation(marketplace.source.location).branch : undefined; - const git = simpleGit(marketplace.path); + const git = (deps.createGit ?? simpleGit)(marketplace.path); let targetBranch: string; if (storedBranch) { @@ -1024,7 +1034,7 @@ export async function updateMarketplace( } await git.checkout(targetBranch); - await pull(marketplace.path); + await (deps.pull ?? pull)(marketplace.path); // Update lastUpdated in the entry (mutates in place for scope tracking) marketplace.lastUpdated = new Date().toISOString(); diff --git a/tests/unit/core/marketplace-update.test.ts b/tests/unit/core/marketplace-update.test.ts index 984674b..e921a2a 100644 --- a/tests/unit/core/marketplace-update.test.ts +++ b/tests/unit/core/marketplace-update.test.ts @@ -1,29 +1,33 @@ -import { describe, it, expect, beforeEach, afterEach, mock } from 'bun:test'; -import { mkdirSync, readFileSync, writeFileSync, rmSync } from 'node:fs'; +import { afterEach, beforeEach, describe, expect, it, mock } from 'bun:test'; +import { mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; +import { updateMarketplace } from '../../../src/core/marketplace.js'; import { stubHomeDir } from '../../helpers/env.js'; // Track calls for assertions const simpleGitCalls: Array<{ method: string; args: unknown[] }> = []; const pullCalls: Array<{ path: string }> = []; -// Create a mock simple-git instance -function createMockGit(overrides: Record unknown> = {}) { +function createMockGit( + overrides: { + raw?: (args: string[]) => Promise; + checkout?: (branch: string) => Promise; + } = {}, +) { return { - raw: mock((...args: unknown[]) => { - simpleGitCalls.push({ method: 'raw', args }); - if (overrides.raw) return overrides.raw(...args); + raw: mock((args: string[]) => { + simpleGitCalls.push({ method: 'raw', args: [args] }); + if (overrides.raw) return overrides.raw(args); // Default: symbolic-ref returns origin/main - const rawArgs = args[0] as string[]; - if (rawArgs?.[0] === 'symbolic-ref') { + if (args[0] === 'symbolic-ref') { return Promise.resolve('origin/main'); } return Promise.resolve(''); }), - checkout: mock((...args: unknown[]) => { - simpleGitCalls.push({ method: 'checkout', args }); - if (overrides.checkout) return overrides.checkout(...args); + checkout: mock((branch: string) => { + simpleGitCalls.push({ method: 'checkout', args: [branch] }); + if (overrides.checkout) return overrides.checkout(branch); return Promise.resolve(); }), }; @@ -31,33 +35,14 @@ function createMockGit(overrides: Record unknown let currentMockGit = createMockGit(); -mock.module('simple-git', () => ({ - default: () => currentMockGit, -})); - -// Mock the git module's pull function -mock.module('../../../src/core/git.js', () => ({ - createGitEnv: () => ({ - ...process.env, - GIT_TERMINAL_PROMPT: '0', - GIT_LFS_SKIP_SMUDGE: '1', - }), - pull: mock((path: string) => { - pullCalls.push({ path }); - return Promise.resolve(); - }), - cloneToTemp: mock(() => Promise.resolve('/tmp/fake')), - cloneTo: mock(() => Promise.resolve()), - repoExists: mock(() => Promise.resolve(true)), - refExists: mock(() => Promise.resolve(true)), - cleanupTempDir: mock(() => Promise.resolve()), - classifyError: (error: Error) => error, - gitHubUrl: (owner: string, repo: string) => `https://github.com/${owner}/${repo}.git`, - GitCloneError: class extends Error {}, -})); - -// Must import after mock.module -const { updateMarketplace } = await import('../../../src/core/marketplace.js'); +function marketplaceUpdateDeps() { + return { + createGit: () => currentMockGit, + pull: async (path: string) => { + pullCalls.push({ path }); + }, + }; +} describe('updateMarketplace', () => { let restoreHomeDir: () => void; @@ -102,7 +87,11 @@ describe('updateMarketplace', () => { }); it('should checkout default branch before pulling', async () => { - const results = await updateMarketplace('test-mp'); + const results = await updateMarketplace( + 'test-mp', + undefined, + marketplaceUpdateDeps(), + ); expect(results).toHaveLength(1); expect(results[0].success).toBe(true); @@ -122,19 +111,22 @@ describe('updateMarketplace', () => { it('should use remote show origin to detect master branch when symbolic-ref fails', async () => { currentMockGit = createMockGit({ - raw: (...args: unknown[]) => { - const rawArgs = args[0] as string[]; - if (rawArgs?.[0] === 'symbolic-ref') { + raw: (args: string[]) => { + if (args[0] === 'symbolic-ref') { return Promise.reject(new Error('fatal: ref not found')); } - if (rawArgs?.[0] === 'remote' && rawArgs?.[1] === 'show') { + if (args[0] === 'remote' && args[1] === 'show') { return Promise.resolve(' HEAD branch: master\n Remote branches:\n'); } return Promise.resolve(''); }, }); - const results = await updateMarketplace('test-mp'); + const results = await updateMarketplace( + 'test-mp', + undefined, + marketplaceUpdateDeps(), + ); expect(results).toHaveLength(1); expect(results[0].success).toBe(true); @@ -146,19 +138,22 @@ describe('updateMarketplace', () => { it('should fallback to main when both symbolic-ref and remote show fail', async () => { currentMockGit = createMockGit({ - raw: (...args: unknown[]) => { - const rawArgs = args[0] as string[]; - if (rawArgs?.[0] === 'symbolic-ref') { + raw: (args: string[]) => { + if (args[0] === 'symbolic-ref') { return Promise.reject(new Error('fatal: ref not found')); } - if (rawArgs?.[0] === 'remote') { + if (args[0] === 'remote') { return Promise.reject(new Error('fatal: unable to access')); } return Promise.resolve(''); }, }); - const results = await updateMarketplace('test-mp'); + const results = await updateMarketplace( + 'test-mp', + undefined, + marketplaceUpdateDeps(), + ); expect(results).toHaveLength(1); expect(results[0].success).toBe(true); @@ -189,7 +184,11 @@ describe('updateMarketplace', () => { simpleGitCalls.length = 0; - const results = await updateMarketplace('test-mp-branch'); + const results = await updateMarketplace( + 'test-mp-branch', + undefined, + marketplaceUpdateDeps(), + ); expect(results).toHaveLength(1); expect(results[0].success).toBe(true); @@ -227,7 +226,11 @@ describe('updateMarketplace', () => { }), ); - const results = await updateMarketplace('unsafe'); + const results = await updateMarketplace( + 'unsafe', + undefined, + marketplaceUpdateDeps(), + ); expect(results).toHaveLength(1); expect(results[0].success).toBe(false); @@ -264,7 +267,11 @@ describe('updateMarketplace', () => { }), ); - const results = await updateMarketplace(); + const results = await updateMarketplace( + undefined, + undefined, + marketplaceUpdateDeps(), + ); expect(results).toHaveLength(2); const registry = JSON.parse(readFileSync(registryPath, 'utf-8')); diff --git a/tests/unit/core/workspace-modify.test.ts b/tests/unit/core/workspace-modify.test.ts index 1844b6c..c38113e 100644 --- a/tests/unit/core/workspace-modify.test.ts +++ b/tests/unit/core/workspace-modify.test.ts @@ -5,12 +5,32 @@ import { tmpdir } from 'node:os'; import { dump, load } from 'js-yaml'; import type { WorkspaceConfig } from '../../../src/models/workspace-config.js'; +// Bun module mocks update live bindings and persist across test files. Keep the +// unrelated pull export functional so this verification stub cannot disable +// later integration tests that exercise real cached repositories. +async function pullWithGit(path: string): Promise { + const result = Bun.spawnSync(['git', '-C', path, 'pull'], { + env: { + ...process.env, + GIT_LFS_SKIP_SMUDGE: '1', + GIT_TERMINAL_PROMPT: '0', + }, + stdout: 'pipe', + stderr: 'pipe', + }); + if (result.exitCode !== 0) { + throw new Error( + `git pull failed in ${path}: ${result.stderr.toString().trim()}`, + ); + } +} + // Mock git module to avoid network calls in verifyGitHubUrlExists mock.module('../../../src/core/git.js', () => ({ repoExists: async () => true, cloneToTemp: async () => '', cloneTo: async () => {}, - pull: async () => {}, + pull: pullWithGit, refExists: async () => false, cleanupTempDir: async () => {}, gitHubUrl: (owner: string, repo: string) => `https://github.com/${owner}/${repo}.git`, diff --git a/tests/unit/plugins/pr-interactive-review.test.ts b/tests/unit/plugins/pr-interactive-review.test.ts index d70ff6a..990e752 100644 --- a/tests/unit/plugins/pr-interactive-review.test.ts +++ b/tests/unit/plugins/pr-interactive-review.test.ts @@ -317,6 +317,137 @@ describe('pr-interactive-review', () => { (await readdir(prepared.workspace)).some((name) => name.endsWith('.tmp')), ).toBe(false); }); + it('stores a bounded presentation sidecar for reusable editorial context', async () => { + const prepared = await fixture(); + const presentation = join(prepared.repository, 'interactive-presentation.json'); + await writeFile( + presentation, + JSON.stringify({ + eyebrow: 'Runtime seam review', + headline: 'One queue connects two services.', + summary: 'Review the handoff before implementation detail.', + context_cards: [ + { + label: 'System boundary', + title: 'Dispatcher to Runner', + body: 'The Dispatcher starts work and the Runner owns execution.', + tone: 'problem', + }, + ], + mental_model: { + title: 'The execution path', + summary: 'One shared contract crosses the Temporal boundary.', + steps: [ + { + label: '01 / Dispatch', + title: 'Start workflow', + body: 'Create the workflow with the configured task queue.', + }, + ], + }, + }), + ); + const refreshed = await prepareReview({ + reviewJsonPath: prepared.artifact, + pr: '123', + repoPath: prepared.repository, + dataDir: prepared.state, + scenariosPath: prepared.scenarios, + presentationPath: presentation, + }); + expect(refreshed.review.presentation).toEqual({ + eyebrow: 'Runtime seam review', + headline: 'One queue connects two services.', + summary: 'Review the handoff before implementation detail.', + contextCards: [ + { + label: 'System boundary', + title: 'Dispatcher to Runner', + body: 'The Dispatcher starts work and the Runner owns execution.', + tone: 'problem', + }, + ], + mentalModel: { + title: 'The execution path', + summary: 'One shared contract crosses the Temporal boundary.', + steps: [ + { + label: '01 / Dispatch', + title: 'Start workflow', + body: 'Create the workflow with the configured task queue.', + }, + ], + }, + }); + const preserved = await prepareReview({ + reviewJsonPath: prepared.artifact, + pr: '123', + repoPath: prepared.repository, + dataDir: prepared.state, + scenariosPath: prepared.scenarios, + }); + expect(preserved.review.presentation).toEqual(refreshed.review.presentation); + await writeFile( + presentation, + JSON.stringify({ + eyebrow: 'Review', + headline: 'Too many cards', + summary: 'Reject unbounded presentation content.', + context_cards: Array.from({ length: 7 }, (_, index) => ({ + label: `Card ${index}`, + title: 'Title', + body: 'Body', + })), + }), + ); + await expect( + prepareReview({ + reviewJsonPath: prepared.artifact, + pr: '123', + repoPath: prepared.repository, + dataDir: prepared.state, + presentationPath: presentation, + }), + ).rejects.toThrow('array of at most 6 objects'); + await writeFile(presentation, 'null'); + await expect( + prepareReview({ + reviewJsonPath: prepared.artifact, + pr: '123', + repoPath: prepared.repository, + dataDir: prepared.state, + presentationPath: presentation, + }), + ).rejects.toThrow('presentation must be an object'); + const changedArtifact = JSON.parse( + await readFile(prepared.artifact, 'utf8'), + ) as { scope: { head_sha: string } }; + changedArtifact.scope.head_sha = + 'fedcba9876543210fedcba9876543210fedcba98'; + await writeFile(prepared.artifact, JSON.stringify(changedArtifact)); + const changedCommit = await prepareReview({ + reviewJsonPath: prepared.artifact, + pr: '123', + repoPath: prepared.repository, + dataDir: prepared.state, + }); + expect(changedCommit.review.presentation).toBeNull(); + }); + it('rejects invalid persisted presentation metadata before browser rendering', async () => { + const prepared = await fixture(); + const reviewPath = join(prepared.workspace, 'review.json'); + const stored = JSON.parse(await readFile(reviewPath, 'utf8')) as Record< + string, + unknown + >; + stored.presentation = { contextCards: {} }; + await writeFile(reviewPath, JSON.stringify(stored)); + await expect(loadStoredReview(prepared.workspace)).rejects.toThrow( + 'presentation.context_cards must be an array', + ); + }); + + it('escapes untrusted page values and keeps business context before findings', async () => { const prepared = await fixture(); @@ -331,6 +462,26 @@ describe('pr-interactive-review', () => { ); expect(page).not.toContain('Assistant reply'); }); + it('renders a navigable review shell with heavy detail collapsed by default', async () => { + const prepared = await fixture(); + const page = renderReviewPage(prepared.review); + expect(page).toContain('class="review-shell"'); + expect(page).toContain('id="review-nav"'); + expect(page).toContain('class="hero-metrics"'); + expect(page).toContain("disclosure('Supplied requirements'"); + expect(page).toContain("disclosure('Referenced evidence"); + expect(page).toContain("disclosure('Focused code context'"); + expect(page).toContain('@media (max-width: 900px)'); + expect(page).toContain( + '', + ); + expect(page).toContain('renderFindingNav(visible);'); + const renderFindings = page.indexOf('function renderFindings()'); + expect( + page.indexOf('state.observer.disconnect()', renderFindings), + ).toBeLessThan(page.indexOf('root.replaceChildren()', renderFindings)); + }); + it('contains long prose fields and reviewed lines without breaking narrow cards', async () => { const prepared = await fixture(); @@ -354,13 +505,13 @@ describe('pr-interactive-review', () => { 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("disclosure('Lifecycle history'"); expect(page).toContain( - '.finding-top, .finding-top > *, .excerpt-grid, .excerpt-grid > * { min-width: 0; }', + '.context-grid, .model-grid, .scenario-grid, .excerpt-grid { grid-template-columns: 1fr; }', ); - expect(page).toContain('p, li, label, strong, .meta, .gap { overflow-wrap: anywhere; word-break: break-word; }'); + expect(page).toContain('h1, h2, h3, h4, p, li, label, strong, .meta, .gap, .rail-link-label { overflow-wrap: anywhere; word-break: break-word; }'); expect(page).toContain( - 'pre { max-width: 100%; min-width: 0; overflow-x: auto; white-space: pre;', + 'pre { max-width: 100%; min-width: 0; margin: 0; overflow-x: auto; white-space: pre;', ); });