From bba716bc44f8fa4b28507c0f343e0a83301f72b3 Mon Sep 17 00:00:00 2001 From: firejune Date: Tue, 10 Nov 2026 01:06:40 +0900 Subject: [PATCH 1/4] Read act@5 and /4 parts verdicts and publish draft panels (#377) Co-Authored-By: Jinni <342598851+fj-jinni[bot]@users.noreply.github.com> --- src/job-publication.ts | 3 +- src/parts-act.ts | 5 +- src/parts-verdict-record.ts | 38 ++++++- src/review-act.ts | 56 ++++++++- tests/parts-act.test.ts | 200 ++++++++++++++++++++++++++++++++- tests/parts-verdict-v2.test.ts | 67 ++++++++++- 6 files changed, 354 insertions(+), 15 deletions(-) diff --git a/src/job-publication.ts b/src/job-publication.ts index 09024f2..7ef5b29 100644 --- a/src/job-publication.ts +++ b/src/job-publication.ts @@ -446,7 +446,8 @@ export function prepareDraftManifest(root: string, project: JsonObject, id: stri references: context.parts!.images.map((image, index) => ({ id: `image-${index}`, label: `${image.role} ${image.path}`, type: 'image', path: relative(root, resolve(root, dirname(context.parts!.manifest.path), image.path)).split('\\').join('/') })), animations: [], checkpoints: [], authoring: files.filter(file => /\/(rig|motion|mesh_report)\.json$/.test(String(file.path))) - .map(file => ({ label: String(file.path).split('/').at(-1), path: file.path })) }; + .map(file => ({ label: String(file.path).split('/').at(-1), path: file.path })), + panels: { schema: 'rig-review/panels@1', left: ['parts.list'], stage: ['parts.canvas'], toolbar: ['parts.layers'], side: ['parts.issues', 'act.submit'] } }; const path = `review-draft-${id}.json`; const bytes = Buffer.from(`${JSON.stringify(manifest, null, 2)}\n`); if (existsSync(join(root, path))) { diff --git a/src/parts-act.ts b/src/parts-act.ts index 33e892b..811b45c 100644 --- a/src/parts-act.ts +++ b/src/parts-act.ts @@ -44,7 +44,10 @@ export function assertPartsActSource(root: string, project: JsonObject, act: Par for (const image of act.source.parts.images) check({ path: join(dirname(act.source.parts.manifest.path), image.path), sha256: image.sha256 }); const manifest = JSON.parse(publicationFile(root, act.source.parts.manifest.path).toString('utf8')) as { parts: { name: string }[] }; const names = manifest.parts.map(part => part.name).sort(); - if (!isDeepStrictEqual(names, act.value.parts.map(part => part.partId).sort())) fail('verdict part membership differs from the bound manifest'); + if (act.schema === 'rig-review/act@5') { + const manifestNames = new Set(names); + if (act.value.issues.some(issue => issue.parts.some(part => !manifestNames.has(part.name)))) fail('verdict part membership differs from the bound manifest'); + } else if (!isDeepStrictEqual(names, act.value.parts.map(part => part.partId).sort())) fail('verdict part membership differs from the bound manifest'); if (act.source.model) { if (!(draft.files as JsonObject[]).some(file => file.path === act.source.model!.path && file.sha256 === act.source.model!.sha256)) fail('displayed model is outside the selected draft'); check(act.source.model); diff --git a/src/parts-verdict-record.ts b/src/parts-verdict-record.ts index 17272ab..7557f45 100644 --- a/src/parts-verdict-record.ts +++ b/src/parts-verdict-record.ts @@ -23,11 +23,30 @@ export function decodePartsRead(bytes: Buffer): JsonObject { let value: unknown; try { value = JSON.parse(new TextDecoder('utf-8', { fatal: true }).decode(bytes)); } catch { return fail('PARTS_REVIEW_CLI_INVALID', 'CLI stdout must be a UTF-8 JSON object'); } - if (!object(value) || !['rig-review-parts-verdict/1', 'rig-review-parts-verdict/2'].includes(String(value.schema)) + if (!object(value) || !['rig-review-parts-verdict/1', 'rig-review-parts-verdict/2', 'rig-review-parts-verdict/4'].includes(String(value.schema)) || !['approved', 'changes-requested', 'pending'].includes(String(value.status)) || !object(value.tool) || typeof value.tool.name !== 'string' || !value.tool.name || typeof value.tool.version !== 'string' || !value.tool.version || !object(value.source) || !sha(value.source.sha256) || !Array.isArray(value.source.files) || !value.source.files.length - || !value.source.files.every(file => object(file) && sha(file.sha256)) || !object(value.parts)) { + || !value.source.files.every(file => object(file) && sha(file.sha256))) { + return fail('PARTS_REVIEW_CLI_INVALID', 'CLI response needs a supported schema, status, tool, source hashes and per-part outcomes'); + } + if (value.schema === 'rig-review-parts-verdict/4') { + if (typeof value.mode !== 'string' || !value.mode || typeof value.updatedAt !== 'string' || !value.updatedAt + || !['approved', 'changes-requested'].includes(String(value.status)) || !Array.isArray(value.issues) + || (value.status === 'approved') !== (value.issues.length === 0)) { + return fail('PARTS_REVIEW_CLI_INVALID', 'CLI response needs a supported schema, status, tool, source hashes and issues'); + } + for (const issue of value.issues) { + if (!object(issue) || typeof issue.id !== 'string' || !issue.id || typeof issue.summary !== 'string' || !issue.summary.trim() + || !Array.isArray(issue.parts) || !issue.parts.length + || !issue.parts.every(part => object(part) && typeof part.name === 'string' && part.name.length > 0 && sha(part.imageHash)) + || new Set(issue.parts.map(part => (part as JsonObject).name)).size !== issue.parts.length) { + return fail('PARTS_REVIEW_CLI_INVALID', 'each CLI issue needs id, summary and unique part bindings'); + } + } + return value; + } + if (!object(value.parts)) { return fail('PARTS_REVIEW_CLI_INVALID', 'CLI response needs a supported schema, status, tool, source hashes and per-part outcomes'); } for (const part of Object.values(value.parts)) { @@ -49,6 +68,11 @@ function partsStdout(root: string, evidence: JsonObject, path: string): Buffer { } export function partsVerdictNames(verdict: JsonObject): { flagged: string[]; noted: string[] } { + if (Array.isArray(verdict.issues)) { + const names = [...new Set(verdict.issues.flatMap(issue => object(issue) && Array.isArray(issue.parts) + ? issue.parts.filter(object).map(part => String(part.name)) : []))]; + return { flagged: names, noted: [...names] }; + } const entries = Object.entries(verdict.parts as JsonObject); return { flagged: entries.filter(([, part]) => (part as JsonObject).outcome === 'flagged').map(([name]) => name), @@ -182,10 +206,11 @@ export function recordedPartsVerdict(root: string, project: JsonObject, draftId: const bytes = receipt.schema === PARTS_CLI_RECEIPT ? partsStdout(root, binding, 'signoffs.parts-review') : publicationFile(realpathSync(root), String(binding.path)); let partsFile: string; - const spec = receipt.schema === PARTS_CLI_RECEIPT ? PARTS_CLI_RECEIPT : 'rig-review/act@2'; - if (evidence !== undefined && (!object(evidence) || evidence.kind !== 'parts-verdict' - || (evidence.spec !== undefined && evidence.spec !== spec))) return fail('PARTS_VERDICT_RECEIPT_INVALID', 'signoffs.parts-review.evidence has an invalid receipt spec'); + let spec: string; if (receipt.schema === PARTS_CLI_RECEIPT) { + spec = PARTS_CLI_RECEIPT; + if (evidence !== undefined && (!object(evidence) || evidence.kind !== 'parts-verdict' + || (evidence.spec !== undefined && evidence.spec !== spec))) return fail('PARTS_VERDICT_RECEIPT_INVALID', 'signoffs.parts-review.evidence has an invalid receipt spec'); const verdict = decodePartsRead(bytes); assertPartsReadDraft(root, project, verdict, draftId, options); if (verdict.status !== 'approved') return fail('PARTS_VERDICT_NOT_APPROVED', `signoffs.parts-review.evidence for draft ${draftId} is not approved`); @@ -194,6 +219,9 @@ export function recordedPartsVerdict(root: string, project: JsonObject, draftId: } else { if (receipt.schema !== 'rig-create/review-act@1') return fail('PARTS_VERDICT_RECEIPT_INVALID', 'signoffs.parts-review.evidence names an unsupported receipt spec'); const act = decodeReviewAct(bytes.toString('utf8'), 'parts-verdict'); + spec = act.schema; + if (evidence !== undefined && (!object(evidence) || evidence.kind !== 'parts-verdict' + || (evidence.spec !== undefined && evidence.spec !== spec))) return fail('PARTS_VERDICT_RECEIPT_INVALID', 'signoffs.parts-review.evidence has an invalid receipt spec'); if (act.source.selector.id !== draftId) return fail('PARTS_VERDICT_DRAFT_MISMATCH', `signoffs.parts-review.draft_id differs from the retained act for ${draftId}`); const draft = (project.drafts as JsonObject[]).find(draft => draft.id === draftId); const draftRoot = `derived/drafts/${draftId}`; diff --git a/src/review-act.ts b/src/review-act.ts index 109b392..4efde7b 100644 --- a/src/review-act.ts +++ b/src/review-act.ts @@ -13,8 +13,28 @@ import { collectItems, asArray, isObject } from './check.ts'; import { stepHash } from './step-hash.ts'; import type { Json, JsonObject } from './templates.ts'; +export type PartsIssue = { + readonly id: string; + readonly summary: string; + readonly parts: readonly { readonly name: string; readonly imageHash: string }[]; + readonly sourceHash: string; + readonly rigRegion: JsonObject; + readonly imageRegion: JsonObject | null; + readonly view: JsonObject; + readonly crop: JsonObject; + readonly nativeCrop: JsonObject | null; +}; export type InterviewPicksAct = Extract; -export type PartsVerdictAct = Extract; +export type PartsVerdictActV2 = Extract; +export type PartsVerdictActV5 = Omit & { + readonly schema: 'rig-review/act@5'; + readonly value: { + readonly verdict: 'approved' | 'changes-requested'; + readonly issues: readonly PartsIssue[]; + readonly words: string; + }; +}; +export type PartsVerdictAct = PartsVerdictActV2 | PartsVerdictActV5; export type ReviewVerdictAct = Extract & { source: JobModelSourceBinding }; export type CheckpointAct = Extract; export type SignoffAct = Extract; @@ -40,7 +60,8 @@ export function decodeReviewAct(bytes: string, kind: 'review-verdict' | 'parts-v let act: JsonObject; try { const parsed: unknown = JSON.parse(bytes); if (!object(parsed)) throw Error(); act = parsed; } catch { return failure('REVIEW_ACT_INVALID', 'act is not a JSON object'); } - if (act.schema !== 'rig-review/act@2' && !(kind === 'review-verdict' && act.schema === 'rig-review/act@3')) failure('REVIEW_ACT_SCHEMA_UNSUPPORTED', `act schema ${String(act.schema)} is unsupported, including legacy rig-review/act@1`); + if (act.schema !== 'rig-review/act@2' && !(kind === 'review-verdict' && act.schema === 'rig-review/act@3') + && !(kind === 'parts-verdict' && act.schema === 'rig-review/act@5')) failure('REVIEW_ACT_SCHEMA_UNSUPPORTED', `act schema ${String(act.schema)} is unsupported, including legacy rig-review/act@1`); const source = act.source; const parts = object(source) ? source.parts : null; const version = object(parts) && typeof parts.enumeratorVersion === 'string' ? parts.enumeratorVersion : ''; @@ -98,8 +119,35 @@ export function decodeReviewAct(bytes: string, kind: 'review-verdict' | 'parts-v || !Array.isArray(parts.images) || parts.images.some(row => !file(row) || typeof row.role !== 'string') || source.model !== null && !file(source.model) || source.direction !== null && !file(source.direction) || source.plan !== null && (!object(source.plan) || !Array.isArray(source.plan.steps) - || source.plan.steps.some(step => !object(step) || !id(step.id) || !sha(step.hash))) - || !Array.isArray(value.parts) || !value.parts.length) failure('REVIEW_ACT_INVALID', 'parts verdict source or rows are malformed'); + || source.plan.steps.some(step => !object(step) || !id(step.id) || !sha(step.hash)))) failure('REVIEW_ACT_INVALID', 'parts verdict source or rows are malformed'); + if (act.schema === 'rig-review/act@5') { + if (!['approved', 'changes-requested'].includes(String(value.verdict)) || !Array.isArray(value.issues) + || value.parts !== undefined) failure('REVIEW_ACT_INVALID', 'parts verdict source or issues are malformed'); + const issueIds = new Set(); + for (const issue of value.issues) { + if (!object(issue) || !id(issue.id) || issueIds.has(issue.id) + || typeof issue.summary !== 'string' || !issue.summary.trim() + || !Array.isArray(issue.parts) || !issue.parts.length + || !sha(issue.sourceHash) || !object(issue.rigRegion) + || !('imageRegion' in issue) || (issue.imageRegion !== null && !object(issue.imageRegion)) + || !object(issue.view) || !object(issue.crop) + || !('nativeCrop' in issue) || (issue.nativeCrop !== null && !object(issue.nativeCrop))) { + failure('REVIEW_ACT_INVALID', 'parts verdict issue is malformed'); + } + issueIds.add(issue.id); + const partNames = new Set(); + for (const part of issue.parts) { + if (!object(part) || !id(part.name) || !sha(part.imageHash) || partNames.has(part.name) + || !parts.images.some(img => object(img) && img.role === 'part' && img.path === `parts/${part.name}.png` && img.sha256 === part.imageHash)) { + failure('REVIEW_ACT_INVALID', 'parts verdict issue part is invalid, duplicated or outside the bound parts manifest'); + } + partNames.add(part.name); + } + } + if ((value.verdict === 'approved') !== (value.issues.length === 0)) failure('REVIEW_ACT_INVALID', 'parts verdict contradicts its issues'); + return act as unknown as PartsVerdictAct; + } + if (!Array.isArray(value.parts) || !value.parts.length) failure('REVIEW_ACT_INVALID', 'parts verdict source or rows are malformed'); const rows = value.parts as JsonObject[]; const ids = new Set(); for (const row of rows) { diff --git a/tests/parts-act.test.ts b/tests/parts-act.test.ts index 6fe10fb..aa40194 100644 --- a/tests/parts-act.test.ts +++ b/tests/parts-act.test.ts @@ -2,7 +2,7 @@ import { reviewStdout } from './review-read-fixture.ts'; import { partsReviewCommand } from '../src/parts-review.ts'; import { afterEach, expect, test } from 'bun:test'; import { randomUUID } from 'node:crypto'; -import { cpSync, existsSync, mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from 'node:fs'; +import { cpSync, existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import type { JobAct, JobPartOutcome, JobVerdict } from '../node_modules/@rig-c/review/src/job-contract.ts'; @@ -21,8 +21,10 @@ import { advanceCommand } from '../src/progress-writers.ts'; import { diffClassesCommand } from '../src/diff-classes.ts'; import { signoffCommand } from '../src/signoff.ts'; import { structureSnapshotCommand } from '../src/structure-snapshot.ts'; -import { partsActFindings } from '../src/parts-act.ts'; -import { decodeReviewAct } from '../src/review-act.ts'; +import { assertPartsActSource, partsActDraftHash, partsActFindings } from '../src/parts-act.ts'; +import { prepareDraftManifest } from '../src/job-publication.ts'; +import { recordedPartsVerdict } from '../src/parts-verdict-record.ts'; +import { decodeReviewAct, type PartsIssue, type PartsVerdictActV5 } from '../src/review-act.ts'; import { EXAMPLE_NOW } from '../scripts/example-time.ts'; import type { JsonObject } from '../src/templates.ts'; @@ -204,6 +206,7 @@ for (const [verdict, part, mesh] of [ const recorded = JSON.parse(result.stdout); expect(recorded.verdict).toBe(verdict); const evidence = decodeReviewAct(readFileSync(join(root, recorded.evidence.path), 'utf8'), 'parts-verdict'); + if (evidence.schema !== 'rig-review/act@2') throw Error(evidence.schema); expect(evidence.value.parts.every(row => row.part.outcome === part && row.mesh.outcome === mesh)).toBe(true); expect(evidence.value.words).toBe(' Synthetic parts review.\r\n'); expect(read(root).signoffs).toEqual(before.signoffs); expect(read(root).stage).toBe(before.stage); @@ -415,3 +418,194 @@ test('CLI and act receipts share current-verdict ordering', async () => { expect(latestPartsVerdictFinding(read(root), 'chain', 'signoffs[0].draft_id')?.code).toBe('PARTS_VERDICT_LATEST_NOT_APPROVED'); expect(partsActFindings(root, read(root))).toEqual([]); }, MULTI_SPAWN_TIMEOUT_MS); + +test('act@5 issue-list parts verdict decodes, enforces verdict/issues invariant and checks manifest inclusion and draft panels', () => { + const root = fixture(true); + const project = read(root); + const manifestFile = prepareDraftManifest(root, project, 'chain'); + const publishedManifest = JSON.parse(readFileSync(join(root, manifestFile), 'utf8')) as JsonObject; + expect(publishedManifest.panels).toEqual({ + schema: 'rig-review/panels@1', + left: ['parts.list'], + stage: ['parts.canvas'], + toolbar: ['parts.layers'], + side: ['parts.issues', 'act.submit'], + }); + + const inputDir = join(root, 'derived/drafts/chain/input'); + const images = partsEnumerator.enumerate(inputDir).map(row => ({ + ...row, + sha256: jobHash(readFileSync(join(inputDir, row.path))), + })); + const partImages = images.filter(img => img.role === 'part'); + const firstPart = partImages[0]!; + const firstPartName = firstPart.path.slice('parts/'.length, -'.png'.length); + const manifestPath = 'derived/drafts/chain/input/parts.json'; + const manifestSha = jobHash(readFileSync(join(root, manifestPath))); + const modelPath = 'derived/drafts/chain/out/check/build/skeleton.model.json'; + const modelSha = jobHash(readFileSync(join(root, modelPath))); + const steps = ((project.plan as JsonObject).steps as JsonObject[]).map(step => ({ + id: String(step.id), + hash: stepHash(step), + })); + + const baseSource = { + sourceHash: 'a'.repeat(64), + projectHash: jobHash(readFileSync(join(root, 'project.json'))), + package: { id: 'create-draft-chain', revision: String(publishedManifest.revision) }, + selector: { kind: 'draft' as const, id: 'chain' }, + model: { path: modelPath, sha256: modelSha }, + direction: null, + optionSet: null, + appliedResult: null, + plan: { steps }, + parts: { + enumeratorVersion: partsEnumerator.version, + manifest: { path: manifestPath, sha256: manifestSha }, + images, + }, + }; + + const sampleIssue = (overrides: Partial = {}): PartsIssue => ({ + id: 'issue-1', + summary: 'Ragged sleeve cuff seam', + parts: [{ name: firstPartName, imageHash: firstPart.sha256 }], + sourceHash: 'b'.repeat(64), + rigRegion: { x: 10, y: 12, width: 24, height: 18 }, + imageRegion: { x: 2, y: 4, width: 24, height: 18 }, + view: { background: 'checker' }, + crop: { path: `comment-crops/${'c'.repeat(64)}.png`, sha256: 'c'.repeat(64), width: 24, height: 18 }, + nativeCrop: null, + ...overrides, + }); + + const makeActV5 = (value: Record, sourceOverrides: Partial = {}): Record => ({ + schema: 'rig-review/act@5', + actId: 'act-v5-1', + jobId: 'job-v5-1', + askId: 'ask-v5-1', + publicationId: 'pub-v5-1', + kind: 'parts-verdict', + authority: { actor: 'human', channel: 'review-ui', reviewer: { id: 'synthetic-owner', role: 'owner' } }, + at: new Date(EXAMPLE_NOW).toISOString(), + source: { ...baseSource, ...sourceOverrides }, + value, + }); + + // 1. act@5 approved with zero issues is accepted + const approvedBytes = JSON.stringify(makeActV5({ verdict: 'approved', issues: [], words: 'All parts clean.' })); + const approvedAct = decodeReviewAct(approvedBytes, 'parts-verdict'); + expect(approvedAct.schema).toBe('rig-review/act@5'); + expect(approvedAct.value.verdict).toBe('approved'); + if (approvedAct.schema !== 'rig-review/act@5') throw Error(approvedAct.schema); + expect(approvedAct.value.issues).toEqual([]); + assertPartsActSource(root, project, approvedAct); + + // 2. act@5 approved with one or more issues is refused + expect(() => + decodeReviewAct( + JSON.stringify(makeActV5({ verdict: 'approved', issues: [sampleIssue()], words: 'Contradictory.' })), + 'parts-verdict', + ), + ).toThrow(expect.objectContaining({ code: 'REVIEW_ACT_INVALID' })); + + // 3. act@5 changes-requested with zero issues is refused + expect(() => + decodeReviewAct( + JSON.stringify(makeActV5({ verdict: 'changes-requested', issues: [], words: 'No issues.' })), + 'parts-verdict', + ), + ).toThrow(expect.objectContaining({ code: 'REVIEW_ACT_INVALID' })); + + // 4. act@5 changes-requested with valid subset of parts is accepted by both decodeReviewAct and assertPartsActSource + const changesAct = decodeReviewAct( + JSON.stringify(makeActV5({ verdict: 'changes-requested', issues: [sampleIssue({ imageRegion: null })], words: 'Fix cuff.' })), + 'parts-verdict', + ); + expect(changesAct.schema).toBe('rig-review/act@5'); + assertPartsActSource(root, project, changesAct); + + // 5. an issue naming a part outside the bound manifest is refused in decodeReviewAct and in assertPartsActSource + expect(() => + decodeReviewAct( + JSON.stringify( + makeActV5({ + verdict: 'changes-requested', + issues: [sampleIssue({ parts: [{ name: 'nonexistent_part', imageHash: firstPart.sha256 }] })], + words: 'Unknown part.', + }), + ), + 'parts-verdict', + ), + ).toThrow(expect.objectContaining({ code: 'REVIEW_ACT_INVALID' })); + + expect(() => + decodeReviewAct( + JSON.stringify( + makeActV5({ + verdict: 'changes-requested', + issues: [ + sampleIssue({ + parts: [ + { name: firstPartName, imageHash: firstPart.sha256 }, + { name: firstPartName, imageHash: firstPart.sha256 }, + ], + }), + ], + words: 'Duplicate part in one issue.', + }), + ), + 'parts-verdict', + ), + ).toThrow(expect.objectContaining({ code: 'REVIEW_ACT_INVALID' })); + + const forgedUnboundAct = makeActV5({ + verdict: 'changes-requested', + issues: [sampleIssue({ parts: [{ name: 'nonexistent_part', imageHash: firstPart.sha256 }] })], + words: 'Forged manifest mismatch.', + }) as unknown as PartsVerdictActV5; + expect(() => assertPartsActSource(root, project, forgedUnboundAct)).toThrow( + expect.objectContaining({ code: 'REVIEW_ACT_SOURCE_STALE' }), + ); + + // 6. act@2 per-part rows still decode unchanged + const actV2 = decodeReviewAct( + JSON.stringify({ + ...makeActV5({}), + schema: 'rig-review/act@2', + value: { + verdict: 'approved', + parts: partImages.map(img => ({ + partId: img.path.slice('parts/'.length, -'.png'.length), + part: { outcome: 'clean', note: '' }, + mesh: { outcome: 'clean', note: '' }, + })), + words: 'Legacy v2 approved.', + }, + }), + 'parts-verdict', + ); + expect(actV2.schema).toBe('rig-review/act@2'); + assertPartsActSource(root, project, actV2); + + // 7. recordedPartsVerdict preserves rig-review/act@5 as the evidence spec + const evidencePath = `evidence/review-acts/${jobHash(JSON.stringify([approvedAct.jobId, approvedAct.actId]))}.json`; + mkdirSync(join(root, 'evidence/review-acts'), { recursive: true }); + writeFileSync(join(root, evidencePath), approvedBytes); + project.review_acts = [ + { + schema: 'rig-create/review-act@1', + kind: 'parts-verdict', + jobId: approvedAct.jobId, + actId: approvedAct.actId, + draft_id: 'chain', + version_hash: partsActDraftHash(project, approvedAct), + bytes: approvedBytes, + sha256: jobHash(approvedBytes), + evidence: { path: evidencePath, sha256: jobHash(approvedBytes) }, + }, + ]; + write(root, project); + const recorded = recordedPartsVerdict(root, project, 'chain'); + expect(recorded.evidence.spec).toBe('rig-review/act@5'); +}); diff --git a/tests/parts-verdict-v2.test.ts b/tests/parts-verdict-v2.test.ts index 8695c57..a1e6276 100644 --- a/tests/parts-verdict-v2.test.ts +++ b/tests/parts-verdict-v2.test.ts @@ -2,7 +2,7 @@ import { expect, test } from 'bun:test'; import { readFileSync } from 'node:fs'; import { join } from 'node:path'; -import { decodePartsRead } from '../src/parts-verdict-record.ts'; +import { decodePartsRead, partsVerdictNames } from '../src/parts-verdict-record.ts'; for (const [folder, file, schema, comments] of [ ['rig-review-1.5.0', 'parts-review.json', 'rig-review-parts-verdict/1', undefined], @@ -19,3 +19,68 @@ for (const [folder, file, schema, comments] of [ expect(value).toEqual(JSON.parse(bytes.toString('utf8'))); }); } + +test('rig-review-parts-verdict/4 decodes and partsVerdictNames returns deduplicated issue parts in encounter order', () => { + const sha = 'a'.repeat(64); + const baseV4 = { + schema: 'rig-review-parts-verdict/4', + mode: 'parts', + tool: { name: '@rig-c/review', version: '2.8.0' }, + source: { sha256: sha, files: [{ path: 'parts.json', sha256: sha }] }, + updatedAt: '2026-11-10T00:00:00.000Z', + }; + + const approved = decodePartsRead(Buffer.from(JSON.stringify({ ...baseV4, status: 'approved', issues: [] }))); + expect(approved.schema).toBe('rig-review-parts-verdict/4'); + expect(partsVerdictNames(approved)).toEqual({ flagged: [], noted: [] }); + + const changesRequested = decodePartsRead( + Buffer.from( + JSON.stringify({ + ...baseV4, + status: 'changes-requested', + issues: [ + { + id: 'issue-1', + summary: 'Shoulder seam gap', + parts: [ + { name: 'arm_r', imageHash: sha }, + { name: 'torso', imageHash: sha }, + ], + sourceHash: sha, + rigRegion: { x: 0, y: 0, width: 10, height: 10 }, + imageRegion: null, + view: { background: 'checker' }, + crop: { path: `comment-crops/${sha}.png`, sha256: sha, width: 10, height: 10 }, + nativeCrop: null, + }, + { + id: 'issue-2', + summary: 'Torso and head fringe', + parts: [ + { name: 'torso', imageHash: sha }, + { name: 'head', imageHash: sha }, + ], + sourceHash: sha, + rigRegion: { x: 5, y: 5, width: 8, height: 8 }, + imageRegion: { x: 1, y: 1, width: 8, height: 8 }, + view: { background: 'checker' }, + crop: { path: `comment-crops/${sha}.png`, sha256: sha, width: 8, height: 8 }, + nativeCrop: null, + }, + ], + }), + ), + ); + expect(partsVerdictNames(changesRequested)).toEqual({ + flagged: ['arm_r', 'torso', 'head'], + noted: ['arm_r', 'torso', 'head'], + }); + + expect(() => + decodePartsRead(Buffer.from(JSON.stringify({ ...baseV4, status: 'approved', issues: changesRequested.issues }))), + ).toThrow(expect.objectContaining({ code: 'PARTS_REVIEW_CLI_INVALID' })); + expect(() => + decodePartsRead(Buffer.from(JSON.stringify({ ...baseV4, status: 'changes-requested', issues: [] }))), + ).toThrow(expect.objectContaining({ code: 'PARTS_REVIEW_CLI_INVALID' })); +}); From dc45fc860bdf281177bc375b5a4709920cabb61f Mon Sep 17 00:00:00 2001 From: firejune Date: Tue, 10 Nov 2026 01:09:32 +0900 Subject: [PATCH 2/4] Add /4 refusal cases to decodePartsRead tests (#377) Co-Authored-By: Jinni <342598851+fj-jinni[bot]@users.noreply.github.com> --- tests/parts-verdict-v2.test.ts | 40 ++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/tests/parts-verdict-v2.test.ts b/tests/parts-verdict-v2.test.ts index a1e6276..9e366fe 100644 --- a/tests/parts-verdict-v2.test.ts +++ b/tests/parts-verdict-v2.test.ts @@ -83,4 +83,44 @@ test('rig-review-parts-verdict/4 decodes and partsVerdictNames returns deduplica expect(() => decodePartsRead(Buffer.from(JSON.stringify({ ...baseV4, status: 'changes-requested', issues: [] }))), ).toThrow(expect.objectContaining({ code: 'PARTS_REVIEW_CLI_INVALID' })); + expect(() => + decodePartsRead( + Buffer.from( + JSON.stringify({ + ...baseV4, + status: 'changes-requested', + issues: [ + { + id: 'issue-dup', + summary: 'Duplicate part name', + parts: [ + { name: 'torso', imageHash: sha }, + { name: 'torso', imageHash: sha }, + ], + }, + ], + }), + ), + ), + ).toThrow(expect.objectContaining({ code: 'PARTS_REVIEW_CLI_INVALID' })); + expect(() => + decodePartsRead( + Buffer.from( + JSON.stringify({ + ...baseV4, + status: 'changes-requested', + issues: [ + { + id: 'issue-blank', + summary: ' ', + parts: [{ name: 'torso', imageHash: sha }], + }, + ], + }), + ), + ), + ).toThrow(expect.objectContaining({ code: 'PARTS_REVIEW_CLI_INVALID' })); + expect(() => + decodePartsRead(Buffer.from(JSON.stringify({ ...baseV4, status: 'pending', issues: [] }))), + ).toThrow(expect.objectContaining({ code: 'PARTS_REVIEW_CLI_INVALID' })); }); From 01cd75e6e53632ef27247ba36ed9bbe28b0a3da0 Mon Sep 17 00:00:00 2001 From: firejune Date: Tue, 10 Nov 2026 01:17:31 +0900 Subject: [PATCH 3/4] Align act@5 and /4 issue test fixtures with producer comment/2 shape (#377) Co-Authored-By: Jinni <342598851+fj-jinni[bot]@users.noreply.github.com> --- tests/parts-act.test.ts | 26 ++++++++++++++++++++++---- tests/parts-verdict-v2.test.ts | 32 +++++++++++++++++++++++++++++--- 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/tests/parts-act.test.ts b/tests/parts-act.test.ts index aa40194..28beaf4 100644 --- a/tests/parts-act.test.ts +++ b/tests/parts-act.test.ts @@ -24,7 +24,7 @@ import { structureSnapshotCommand } from '../src/structure-snapshot.ts'; import { assertPartsActSource, partsActDraftHash, partsActFindings } from '../src/parts-act.ts'; import { prepareDraftManifest } from '../src/job-publication.ts'; import { recordedPartsVerdict } from '../src/parts-verdict-record.ts'; -import { decodeReviewAct, type PartsIssue, type PartsVerdictActV5 } from '../src/review-act.ts'; +import { decodeReviewAct, type PartsVerdictActV5 } from '../src/review-act.ts'; import { EXAMPLE_NOW } from '../scripts/example-time.ts'; import type { JsonObject } from '../src/templates.ts'; @@ -466,14 +466,26 @@ test('act@5 issue-list parts verdict decodes, enforces verdict/issues invariant }, }; - const sampleIssue = (overrides: Partial = {}): PartsIssue => ({ + const baseView = { + mode: 'item', + camera: { zoom: 2, center: { x: 14, y: 13 }, viewport: { width: 800, height: 600 } }, + cameraSync: true, + referenceCamera: null, + background: 'checker', + alpha: false, + footprints: true, + mesh: false, + }; + + const sampleIssue = (overrides: Record = {}): Record => ({ + schema: 'rig-review-parts-comment/2', id: 'issue-1', summary: 'Ragged sleeve cuff seam', parts: [{ name: firstPartName, imageHash: firstPart.sha256 }], sourceHash: 'b'.repeat(64), rigRegion: { x: 10, y: 12, width: 24, height: 18 }, imageRegion: { x: 2, y: 4, width: 24, height: 18 }, - view: { background: 'checker' }, + view: baseView, crop: { path: `comment-crops/${'c'.repeat(64)}.png`, sha256: 'c'.repeat(64), width: 24, height: 18 }, nativeCrop: null, ...overrides, @@ -519,7 +531,13 @@ test('act@5 issue-list parts verdict decodes, enforces verdict/issues invariant // 4. act@5 changes-requested with valid subset of parts is accepted by both decodeReviewAct and assertPartsActSource const changesAct = decodeReviewAct( - JSON.stringify(makeActV5({ verdict: 'changes-requested', issues: [sampleIssue({ imageRegion: null })], words: 'Fix cuff.' })), + JSON.stringify( + makeActV5({ + verdict: 'changes-requested', + issues: [sampleIssue({ imageRegion: null, view: { ...baseView, mode: 'assembled' } })], + words: 'Fix cuff.', + }), + ), 'parts-verdict', ); expect(changesAct.schema).toBe('rig-review/act@5'); diff --git a/tests/parts-verdict-v2.test.ts b/tests/parts-verdict-v2.test.ts index 9e366fe..e2b67f5 100644 --- a/tests/parts-verdict-v2.test.ts +++ b/tests/parts-verdict-v2.test.ts @@ -34,6 +34,18 @@ test('rig-review-parts-verdict/4 decodes and partsVerdictNames returns deduplica expect(approved.schema).toBe('rig-review-parts-verdict/4'); expect(partsVerdictNames(approved)).toEqual({ flagged: [], noted: [] }); + const itemView = { + mode: 'item', + camera: { zoom: 2, center: { x: 4, y: 4 }, viewport: { width: 800, height: 600 } }, + cameraSync: true, + referenceCamera: null, + background: 'checker', + alpha: false, + footprints: true, + mesh: false, + }; + const assembledView = { ...itemView, mode: 'assembled' }; + const changesRequested = decodePartsRead( Buffer.from( JSON.stringify({ @@ -41,6 +53,7 @@ test('rig-review-parts-verdict/4 decodes and partsVerdictNames returns deduplica status: 'changes-requested', issues: [ { + schema: 'rig-review-parts-comment/2', id: 'issue-1', summary: 'Shoulder seam gap', parts: [ @@ -50,11 +63,12 @@ test('rig-review-parts-verdict/4 decodes and partsVerdictNames returns deduplica sourceHash: sha, rigRegion: { x: 0, y: 0, width: 10, height: 10 }, imageRegion: null, - view: { background: 'checker' }, + view: assembledView, crop: { path: `comment-crops/${sha}.png`, sha256: sha, width: 10, height: 10 }, nativeCrop: null, }, { + schema: 'rig-review-parts-comment/2', id: 'issue-2', summary: 'Torso and head fringe', parts: [ @@ -63,11 +77,23 @@ test('rig-review-parts-verdict/4 decodes and partsVerdictNames returns deduplica ], sourceHash: sha, rigRegion: { x: 5, y: 5, width: 8, height: 8 }, - imageRegion: { x: 1, y: 1, width: 8, height: 8 }, - view: { background: 'checker' }, + imageRegion: null, + view: assembledView, crop: { path: `comment-crops/${sha}.png`, sha256: sha, width: 8, height: 8 }, nativeCrop: null, }, + { + schema: 'rig-review-parts-comment/2', + id: 'issue-3', + summary: 'Head rim nick', + parts: [{ name: 'head', imageHash: sha }], + sourceHash: sha, + rigRegion: { x: 6, y: 6, width: 4, height: 4 }, + imageRegion: { x: 1, y: 1, width: 4, height: 4 }, + view: itemView, + crop: { path: `comment-crops/${sha}.png`, sha256: sha, width: 4, height: 4 }, + nativeCrop: null, + }, ], }), ), From 44fe682ad1c32918860d6cda5ada37bfdb531640 Mon Sep 17 00:00:00 2001 From: firejune Date: Tue, 10 Nov 2026 01:21:14 +0900 Subject: [PATCH 4/4] Accept retained legacy draft manifests without panels (#377) Co-Authored-By: Jinni <342598851+fj-jinni[bot]@users.noreply.github.com> --- src/job-publication.ts | 9 ++++++--- tests/parts-act.test.ts | 13 +++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/job-publication.ts b/src/job-publication.ts index 7ef5b29..a64d6c6 100644 --- a/src/job-publication.ts +++ b/src/job-publication.ts @@ -441,17 +441,20 @@ export function prepareDraftManifest(root: string, project: JsonObject, id: stri const models = files.filter(file => String(file.path).endsWith('/skeleton.model.json')); if (models.length > 1) refuse('JOB_PUBLICATION_INVALID', `drafts[${id}]: multiple compiled models are ambiguous; retain one draft output`); const revision = jobHash(`${JSON.stringify(draft, null, 2)}\n`); - const manifest = { schema: 'rig-review/1', id: `create-draft-${id}`, revision, title: `draft ${id}`, + const legacyManifest = { schema: 'rig-review/1', id: `create-draft-${id}`, revision, title: `draft ${id}`, ...(models.length ? { model: models[0].path } : {}), references: context.parts!.images.map((image, index) => ({ id: `image-${index}`, label: `${image.role} ${image.path}`, type: 'image', path: relative(root, resolve(root, dirname(context.parts!.manifest.path), image.path)).split('\\').join('/') })), animations: [], checkpoints: [], authoring: files.filter(file => /\/(rig|motion|mesh_report)\.json$/.test(String(file.path))) - .map(file => ({ label: String(file.path).split('/').at(-1), path: file.path })), + .map(file => ({ label: String(file.path).split('/').at(-1), path: file.path })) }; + const manifest = { ...legacyManifest, panels: { schema: 'rig-review/panels@1', left: ['parts.list'], stage: ['parts.canvas'], toolbar: ['parts.layers'], side: ['parts.issues', 'act.submit'] } }; const path = `review-draft-${id}.json`; const bytes = Buffer.from(`${JSON.stringify(manifest, null, 2)}\n`); + const legacyBytes = Buffer.from(`${JSON.stringify(legacyManifest, null, 2)}\n`); if (existsSync(join(root, path))) { - if (!publicationFile(root, path).equals(bytes)) refuse('JOB_PUBLICATION_INVALID', `drafts[${id}]: retained publication manifest differs; restore it before reconciling`); + const existing = publicationFile(root, path); + if (!existing.equals(bytes) && !existing.equals(legacyBytes)) refuse('JOB_PUBLICATION_INVALID', `drafts[${id}]: retained publication manifest differs; restore it before reconciling`); } else writeFileSync(join(root, path), bytes, { flag: 'wx' }); return path; } diff --git a/tests/parts-act.test.ts b/tests/parts-act.test.ts index 28beaf4..2d3cd07 100644 --- a/tests/parts-act.test.ts +++ b/tests/parts-act.test.ts @@ -431,6 +431,19 @@ test('act@5 issue-list parts verdict decodes, enforces verdict/issues invariant toolbar: ['parts.layers'], side: ['parts.issues', 'act.submit'], }); + expect(prepareDraftManifest(root, project, 'chain')).toBe(manifestFile); + + const { panels: _panels, ...legacyManifest } = publishedManifest; + const legacyBytes = `${JSON.stringify(legacyManifest, null, 2)}\n`; + writeFileSync(join(root, manifestFile), legacyBytes); + expect(prepareDraftManifest(root, project, 'chain')).toBe(manifestFile); + expect(readFileSync(join(root, manifestFile), 'utf8')).toBe(legacyBytes); + + writeFileSync(join(root, manifestFile), `${JSON.stringify({ ...legacyManifest, title: 'tampered' }, null, 2)}\n`); + expect(() => prepareDraftManifest(root, project, 'chain')).toThrow( + expect.objectContaining({ code: 'JOB_PUBLICATION_INVALID' }), + ); + writeFileSync(join(root, manifestFile), `${JSON.stringify(publishedManifest, null, 2)}\n`); const inputDir = join(root, 'derived/drafts/chain/input'); const images = partsEnumerator.enumerate(inputDir).map(row => ({