From 192d8f9a74641252c5ee4ad33c1b2dc5e707b8dd Mon Sep 17 00:00:00 2001 From: Robert DeLuca Date: Wed, 4 Nov 2026 01:44:03 -0600 Subject: [PATCH 1/3] =?UTF-8?q?=E2=9C=A8=20Add=20agent=20disclosure=20and?= =?UTF-8?q?=20screenshot=20region=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Disclose known agent assistance on review writes without transmitting harness identifiers. Support precise screenshot feedback and share credential selection, scope checks, and attribution with raw API commands. --- skills/vizzly/references/cli-context.md | 21 ++ src/adapters/cli/cli.js | 49 ++++- src/domains/api/api.commands.js | 1 + src/domains/auth/agent-attribution.js | 108 ++++++++++ src/domains/auth/cloud-client.js | 7 + src/domains/review/review.commands.js | 154 ++++++++------- src/domains/review/review.commands.test.js | 2 +- tests/cli/review/review-cli.test.js | 220 ++++++++++++++++++++- 8 files changed, 484 insertions(+), 78 deletions(-) create mode 100644 src/domains/auth/agent-attribution.js diff --git a/skills/vizzly/references/cli-context.md b/skills/vizzly/references/cli-context.md index ef35f5fd..5ccf001c 100644 --- a/skills/vizzly/references/cli-context.md +++ b/skills/vizzly/references/cli-context.md @@ -196,3 +196,24 @@ vizzly context review-queue --source --json Use IDs and page values from the API response. Pass cursors back unchanged, keep the same query, and request only what you need. Fetch comments when they help explain a review. + + +### Comments and agent disclosure + +After the user authorizes a review action, comment on the build or the exact +screenshot area using IDs from discovery. Regions use the original bitmap's +pixels, with the origin at its top-left; use the image dimensions, not the viewport. + +```bash +vizzly comment "Navigation overlaps the heading" --screenshot --region 20,40,300,80 --json +vizzly comment "Follow-up" --screenshot --reply-to --json +``` + +Review decisions and comments automatically disclose Codex, Claude Code, Pi, +Gemini CLI or OpenCode when the harness exports its identification variable. +The logged-in user remains the author; disclosure never grants permissions. +Use `--via-agent ` for another agent or to resolve conflicting markers. +`VIZZLY_VIA_AGENT` provides an environment override. Explicit flags win. +`--no-agent-attribution` or `VIZZLY_VIA_AGENT=none` omits disclosure. +Only the reported agent name/slug is sent; session IDs and prompts stay local. +Do not treat reported disclosure as verified identity or permission to act. diff --git a/src/adapters/cli/cli.js b/src/adapters/cli/cli.js index f172e733..9e65197a 100644 --- a/src/adapters/cli/cli.js +++ b/src/adapters/cli/cli.js @@ -382,7 +382,13 @@ function findNestedCommandRequest( return null; } - let valueOptions = new Set(['-c', '--config', '--token', '--log-level']); + let valueOptions = new Set([ + '-c', + '--config', + '--token', + '--log-level', + '--via-agent', + ]); for (let i = startIndex; i < args.length; i++) { let arg = args[i]; @@ -419,7 +425,13 @@ function findRequestedCommand( commandNames, nestedCommandNames = new Map() ) { - let valueOptions = new Set(['-c', '--config', '--token', '--log-level']); + let valueOptions = new Set([ + '-c', + '--config', + '--token', + '--log-level', + '--via-agent', + ]); let args = argv.slice(2); let jsonValueCandidate = null; @@ -544,6 +556,11 @@ program .version(getPackageVersion()) .option('-c, --config ', 'Config file path') .option('--token ', 'Vizzly API token') + .option( + '--via-agent ', + 'Disclose the agent assisting a review or comment' + ) + .option('--no-agent-attribution', 'Omit agent disclosure for this action') .option('-v, --verbose', 'Verbose output (shorthand for --log-level debug)') .option( '--log-level ', @@ -1383,6 +1400,7 @@ api program .command('approve') + .option('--auth ', 'Credential source: auto, user, token', 'auto') .description('Approve a comparison') .argument('', 'Comparison ID to approve') .option('-m, --comment ', 'Optional comment explaining the approval') @@ -1394,6 +1412,9 @@ Examples: $ vizzly approve abc123 -m "LGTM" # Approve with comment $ vizzly approve abc123 --json # Output as JSON for scripting +Known agents are disclosed automatically. Override with --via-agent or +omit disclosure with --no-agent-attribution. VIZZLY_VIA_AGENT sets the default. + Workflow: 1. List comparisons: vizzly comparisons -b 2. Review the changes in the web UI or via URLs in the output @@ -1413,6 +1434,7 @@ Workflow: program .command('reject') + .option('--auth ', 'Credential source: auto, user, token', 'auto') .description('Reject a comparison') .argument('', 'Comparison ID to reject') .option('-r, --reason ', 'Required reason for rejection') @@ -1424,6 +1446,9 @@ Examples: $ vizzly reject abc123 --reason "Needs design review" $ vizzly reject abc123 -r "Regression" --json +Known agents are disclosed automatically. Override with --via-agent or +omit disclosure with --no-agent-attribution. VIZZLY_VIA_AGENT sets the default. + Workflow: 1. List comparisons: vizzly comparisons -b 2. Review the changes in the web UI or via URLs in the output @@ -1443,12 +1468,22 @@ Workflow: program .command('comment') - .description('Add a comment to a build') + .option('--auth ', 'Credential source: auto, user, token', 'auto') + .description('Add a comment to a build or screenshot') + .option('--screenshot ', 'Screenshot ID in this build') + .option( + '--region ', + 'Screenshot region in bitmap pixels from the top-left' + ) + .option( + '--reply-to ', + 'Reply to a comment on the same build or screenshot' + ) .argument('', 'Build ID to comment on') .argument('', 'Comment message') .option( '-t, --type ', - 'Comment type: general, approval, rejection', + 'Comment type: general, approval, rejection, question', 'general' ) .addHelpText( @@ -1459,6 +1494,12 @@ Examples: $ vizzly comment abc123 "Approved" -t approval $ vizzly comment abc123 "Please fix the header" -t rejection $ vizzly comment abc123 "CI feedback" --json + $ vizzly comment "Header overlaps" --screenshot --region 20,40,300,80 + $ vizzly comment "Reviewed" --via-agent codex + +Agent disclosure is automatic in known harnesses. VIZZLY_VIA_AGENT sets an +override; use --no-agent-attribution (or VIZZLY_VIA_AGENT=none) to omit it. +Only agent details are sent; session and thread IDs stay local. Workflow: 1. Get build ID: vizzly builds --branch main diff --git a/src/domains/api/api.commands.js b/src/domains/api/api.commands.js index 947fa972..fdfd7863 100644 --- a/src/domains/api/api.commands.js +++ b/src/domains/api/api.commands.js @@ -196,6 +196,7 @@ export async function apiCommand( token, credential, command: 'api', + attribution: globalOptions, allowNoToken: Boolean(options.schemaDiscovery), }); diff --git a/src/domains/auth/agent-attribution.js b/src/domains/auth/agent-attribution.js new file mode 100644 index 00000000..b5f925be --- /dev/null +++ b/src/domains/auth/agent-attribution.js @@ -0,0 +1,108 @@ +import { VizzlyError } from '../../shared/vizzly-error.js'; + +let agents = [ + { + slug: 'codex', + name: 'Codex', + matches: env => Boolean(env.CODEX_THREAD_ID || env.CODEX_SESSION_ID), + }, + { + slug: 'claude-code', + name: 'Claude Code', + matches: env => env.CLAUDECODE === '1', + }, + { slug: 'pi', name: 'Pi', matches: env => Boolean(env.PI_SESSION_ID) }, + { + slug: 'gemini-cli', + name: 'Gemini CLI', + matches: env => env.GEMINI_CLI === '1', + }, + { slug: 'opencode', name: 'OpenCode', matches: env => env.OPENCODE === '1' }, +]; + +function agentVia(slug) { + if (!/^[a-z][a-z0-9]*(?:-[a-z0-9]+)*$/.test(slug) || slug.length > 48) { + throw new VizzlyError( + 'Agent slug must be lowercase words separated by hyphens (maximum 48 characters).', + 'INVALID_AGENT_ATTRIBUTION' + ); + } + return { + type: 'agent', + slug, + name: agents.find(agent => agent.slug === slug)?.name || slug, + version: null, + }; +} + +export function resolveAgentAttribution(options = {}, env = process.env) { + if (options.viaAgent && options.agentAttribution === false) { + throw new VizzlyError( + 'Choose --via-agent or --no-agent-attribution.', + 'INVALID_AGENT_ATTRIBUTION' + ); + } + if (options.agentAttribution === false) return null; + if (options.viaAgent) return agentVia(options.viaAgent); + if (env.VIZZLY_VIA_AGENT) + return env.VIZZLY_VIA_AGENT === 'none' + ? null + : agentVia(env.VIZZLY_VIA_AGENT); + let matches = agents.filter(agent => agent.matches(env)); + if (matches.length > 1) { + throw new VizzlyError( + `Multiple agents detected: ${matches.map(agent => agent.name).join(', ')}. Use --via-agent or --no-agent-attribution.`, + 'AMBIGUOUS_AGENT_ATTRIBUTION' + ); + } + return matches.length ? agentVia(matches[0].slug) : null; +} + +export function attributeReviewRequest( + endpoint, + fetchOptions, + options = {}, + env = process.env +) { + let path = endpoint.split('?')[0].replace(/\/+$/, ''); + let reviewWrite = + /^\/api\/visual-review\/builds\/[^/]+(?:\/comparisons\/[^/]+|\/ocr-dynamic-content\/[^/]+)?\/decision$/.test( + path + ); + let commentWrite = + /^\/api\/sdk\/builds\/[^/]+(?:\/screenshots\/[^/]+)?\/comments$/.test(path); + if ( + fetchOptions.method?.toUpperCase() !== 'POST' || + (!reviewWrite && !commentWrite) + ) + return fetchOptions; + let via = resolveAgentAttribution(options, env); + let body = JSON.parse(fetchOptions.body || '{}'); + if (!body || typeof body !== 'object' || Array.isArray(body)) { + throw new VizzlyError( + 'Review and comment requests require a JSON object.', + 'INVALID_REQUEST_BODY' + ); + } + let overridden = + options.viaAgent || + options.agentAttribution === false || + env.VIZZLY_VIA_AGENT; + if (overridden || !Object.hasOwn(body, 'via')) { + let { via: _previous, ...content } = body; + body = via ? { ...content, via } : content; + } else if ( + via && + (!body.via || + body.via.type !== via.type || + body.via.slug !== via.slug || + body.via.name !== via.name || + (body.via.version ?? null) !== via.version) + ) { + throw new VizzlyError( + 'Request via conflicts with the detected agent. Select --via-agent or --no-agent-attribution.', + 'AMBIGUOUS_AGENT_ATTRIBUTION' + ); + } + return { ...fetchOptions, body: JSON.stringify(body) }; +} diff --git a/src/domains/auth/cloud-client.js b/src/domains/auth/cloud-client.js index 972aa434..c0d18362 100644 --- a/src/domains/auth/cloud-client.js +++ b/src/domains/auth/cloud-client.js @@ -14,6 +14,7 @@ import { } from '../../shared/cloud-protocol.js'; import { getPackageVersion } from '../../shared/package-info.js'; import { AuthError, VizzlyError } from '../../shared/vizzly-error.js'; +import { attributeReviewRequest } from './agent-attribution.js'; import { createInsufficientScopeError } from './auth-errors.js'; /** @@ -71,6 +72,12 @@ export function createApiClient(options = {}) { * @returns {Promise} Parsed JSON response */ async function request(endpoint, fetchOptions = {}, isRetry = false) { + fetchOptions = attributeReviewRequest( + endpoint, + fetchOptions, + options.attribution, + options.env + ); let { responseType = 'json', retryAuthentication = ['GET', 'HEAD'].includes( diff --git a/src/domains/review/review.commands.js b/src/domains/review/review.commands.js index 0d115d0c..4fb4ee1e 100644 --- a/src/domains/review/review.commands.js +++ b/src/domains/review/review.commands.js @@ -1,19 +1,20 @@ -import { createUserSession } from '../auth/auth.service.js'; /** * Review commands - approve, reject, and comment on comparisons/builds */ import { randomUUID as defaultRandomUUID } from 'node:crypto'; import * as defaultOutput from '../../shared/output.js'; +import { VizzlyError } from '../../shared/vizzly-error.js'; import { createApiClient as defaultCreateApiClient } from '../auth/cloud-client.js'; -import { getAccessToken as defaultGetAccessToken } from '../auth/token-store.js'; +import { requireCredentialScope } from '../auth/credential.rules.js'; +import { resolveCredential } from '../auth/credential.service.js'; +import { showCredential } from '../auth/credential-presentation.js'; import { loadConfig as defaultLoadConfig } from '../config/config-loader.js'; function createReviewDeps(deps = {}) { return { loadConfig: deps.loadConfig || defaultLoadConfig, createApiClient: deps.createApiClient || defaultCreateApiClient, - getAccessToken: deps.getAccessToken || defaultGetAccessToken, randomUUID: deps.randomUUID || defaultRandomUUID, output: deps.output || defaultOutput, exit: deps.exit || (code => process.exit(code)), @@ -33,41 +34,36 @@ async function loadReviewConfig({ loadConfig, options, globalOptions }) { return await loadConfig(globalOptions.config, allOptions); } -function isProjectToken(token) { - return typeof token === 'string' && token.startsWith('vzt_'); -} - -async function getReviewToken(config, getAccessToken) { - if (config.userToken) { - return config.userToken; - } - - let userToken = await getAccessToken(); - if (userToken) { - return userToken; - } - - if (config.apiKey && !isProjectToken(config.apiKey)) { - return config.apiKey; - } - - return null; -} - -function createReviewClient({ createApiClient, config, command, token }) { - let credential = { - token, - source: token === config.userToken ? 'user' : 'config', - kind: 'user', - }; - if (credential.source === 'user') { - credential.session = createUserSession({ baseUrl: config.apiUrl, command }); +function createReviewClient({ + createApiClient, + config, + command, + globalOptions, + options, + output, +}) { + let credential = resolveCredential(config, { + ...globalOptions, + ...options, + command, + }); + if (credential.kind === 'project') { + throw new VizzlyError( + 'User login required for review actions. VIZZLY_TOKEN and --token take precedence; omit a project token to use vizzly login.', + 'USER_AUTH_REQUIRED' + ); } + requireCredentialScope( + credential, + command === 'comment' ? 'comments:write' : 'reviews:write' + ); + if (!globalOptions.json) showCredential(output, credential); return createApiClient({ baseUrl: config.apiUrl, - token, + token: credential.token, credential, command, + attribution: globalOptions, }); } @@ -96,8 +92,7 @@ async function runReviewMutation({ deps, configure = true, }) { - let { loadConfig, createApiClient, getAccessToken, output, exit } = - createReviewDeps(deps); + let { loadConfig, createApiClient, output, exit } = createReviewDeps(deps); if (configure) { configureOutput(output, globalOptions); @@ -105,24 +100,15 @@ async function runReviewMutation({ try { let config = await loadReviewConfig({ loadConfig, options, globalOptions }); - let token = await getReviewToken(config, getAccessToken); - - if (!token) { - output.error('User login required for review actions'); - output.hint('Run "vizzly login" to approve, reject, or comment'); - output.cleanup(); - exit(1); - return; - } - - output.startSpinner(spinnerMessage); - let client = createReviewClient({ createApiClient, config, command, - token, + globalOptions, + options, + output, }); + output.startSpinner(spinnerMessage); let response = await client.request(endpoint, { method: 'POST', ...jsonBody(requestBody), @@ -166,6 +152,8 @@ export function createCommentBody(message, options = {}) { return { content: message, type: options.type || 'general', + ...(options.region ? { region: parseCommentRegion(options.region) } : {}), + ...(options.replyTo ? { parentCommentId: options.replyTo } : {}), }; } @@ -198,14 +186,8 @@ async function runComparisonDecision({ deps, configure = true, }) { - let { - loadConfig, - createApiClient, - getAccessToken, - randomUUID, - output, - exit, - } = createReviewDeps(deps); + let { loadConfig, createApiClient, randomUUID, output, exit } = + createReviewDeps(deps); if (configure) { configureOutput(output, globalOptions); @@ -213,22 +195,15 @@ async function runComparisonDecision({ try { let config = await loadReviewConfig({ loadConfig, options, globalOptions }); - let token = await getReviewToken(config, getAccessToken); - if (!token) { - output.error('User login required for review actions'); - output.hint('Run "vizzly login" to approve, reject, or comment'); - output.cleanup(); - exit(1); - return; - } - - output.startSpinner(spinnerMessage); let client = createReviewClient({ createApiClient, config, command, - token, + globalOptions, + options, + output, }); + output.startSpinner(spinnerMessage); let comparisonResponse = await client.request( `/api/sdk/comparisons/${comparisonId}` ); @@ -395,7 +370,7 @@ export async function commentCommand( return await runReviewMutation({ command: 'comment', - endpoint: `/api/sdk/builds/${buildId}/comments`, + endpoint: `/api/sdk/builds/${buildId}${options.screenshot ? `/screenshots/${options.screenshot}` : ''}/comments`, failureMessage: 'Failed to add comment', globalOptions, options, @@ -413,8 +388,13 @@ export async function commentCommand( buildId, comment: response.comment, }), - writeHumanSuccess: output => { + writeHumanSuccess: (output, response) => { output.complete('Comment added'); + if (options.screenshot) + output.labelValue('Screenshot', options.screenshot); + if (options.region) output.labelValue('Region', options.region); + if (response.comment?.attribution?.via) + output.labelValue('Agent', response.comment.attribution.via.name); output.labelValue('Build', buildId); output.labelValue('Message', message); }, @@ -461,6 +441,25 @@ export function validateRejectOptions(comparisonId, options = {}) { * @param {Object} options - Command options * @returns {string[]} Array of error messages */ +export function parseCommentRegion(value) { + let numbers = value.split(',').map(Number); + if ( + numbers.length !== 4 || + value.split(',').some(part => !part.trim()) || + numbers.some(number => !Number.isSafeInteger(number)) || + numbers[0] < 0 || + numbers[1] < 0 || + numbers[2] <= 0 || + numbers[3] <= 0 + ) { + throw new Error( + '--region requires x,y,width,height in bitmap pixels; x/y must be nonnegative and width/height positive integers' + ); + } + let [x, y, width, height] = numbers; + return { x, y, width, height }; +} + export function validateCommentOptions(buildId, message, options = {}) { let errors = []; if (!buildId || buildId.trim() === '') { @@ -471,9 +470,22 @@ export function validateCommentOptions(buildId, message, options = {}) { } if ( options.type && - !['general', 'approval', 'rejection'].includes(options.type) + !['general', 'approval', 'rejection', 'question'].includes(options.type) ) { - errors.push('--type must be one of: general, approval, rejection'); + errors.push( + '--type must be one of: general, approval, rejection, question' + ); + } + if (options.region && !options.screenshot) + errors.push('--region requires --screenshot'); + if (options.region && options.replyTo) + errors.push('Replies inherit their location; omit --region'); + if (options.region) { + try { + parseCommentRegion(options.region); + } catch (error) { + errors.push(error.message); + } } return errors; } diff --git a/src/domains/review/review.commands.test.js b/src/domains/review/review.commands.test.js index ac8b9312..1599c7a7 100644 --- a/src/domains/review/review.commands.test.js +++ b/src/domains/review/review.commands.test.js @@ -39,7 +39,7 @@ describe('commands/review', () => { [ 'Build ID is required', 'Comment message is required', - '--type must be one of: general, approval, rejection', + '--type must be one of: general, approval, rejection, question', ] ); }); diff --git a/tests/cli/review/review-cli.test.js b/tests/cli/review/review-cli.test.js index 10a13b76..838d8c17 100644 --- a/tests/cli/review/review-cli.test.js +++ b/tests/cli/review/review-cli.test.js @@ -146,7 +146,9 @@ async function withReviewApiServer(callback) { if ( req.method === 'POST' && - req.url === `/api/sdk/builds/${state.buildId}/comments` + (req.url === `/api/sdk/builds/${state.buildId}/comments` || + req.url === + `/api/sdk/builds/${state.buildId}/screenshots/shot-1/comments`) ) { let body = await readJson(req); state.comments.push(body); @@ -378,10 +380,224 @@ describe('commands/review CLI', () => { env, }); assert.equal(result.code, 1); - assert.match(result.stderr, /User login required/); + assert.match(result.stderr, /[Ll]ogin required/); assert.equal(state.decisionRequests.length, 0); assert.equal(state.comments.length, 0); assert.equal(state.refreshRequests, 0); }); }); }); + +describe('agent review disclosure through the CLI', () => { + for (let [slug, name, markers] of [ + [ + 'codex', + 'Codex', + { + CODEX_THREAD_ID: 'private-thread', + CODEX_SESSION_ID: 'private-session', + }, + ], + ['claude-code', 'Claude Code', { CLAUDECODE: '1' }], + ['pi', 'Pi', { PI_SESSION_ID: 'private-pi-session' }], + ['gemini-cli', 'Gemini CLI', { GEMINI_CLI: '1' }], + ['opencode', 'OpenCode', { OPENCODE: '1' }], + ]) { + it(`discloses ${name} on approvals, rejections and region comments without sending marker values`, async () => { + await withReviewApiServer(async ({ apiUrl, state }) => { + let { cwd, vizzlyHome } = await createWorkspace(apiUrl); + try { + let env = { + VIZZLY_API_URL: apiUrl, + VIZZLY_HOME: vizzlyHome, + ...markers, + }; + for (let args of [ + ['approve', state.comparisonId], + ['reject', state.comparisonId, '--reason', 'Header overlaps'], + [ + 'comment', + state.buildId, + 'Header overlaps', + '--screenshot', + 'shot-1', + '--region', + '0,20,200,80', + ], + ]) { + let result = await runCLI(['--json', '--no-color', ...args], { + cwd, + env, + }); + assert.equal(result.code, 0, result.stderr + result.stdout); + } + let bodies = [ + ...state.decisionRequests.map(request => request.body), + ...state.comments, + ]; + assert.equal(bodies.length, 3); + for (let body of bodies) { + assert.deepEqual(body.via, { + type: 'agent', + slug, + name, + version: null, + }); + assert.doesNotMatch( + JSON.stringify(body), + /private-thread|private-session|private-pi-session|SESSION_ID|THREAD_ID/ + ); + } + assert.deepEqual(state.comments[0].region, { + x: 0, + y: 20, + width: 200, + height: 80, + }); + } finally { + rmSync(cwd, { recursive: true, force: true }); + } + }); + }); + } + + it('handles conflicts, explicit overrides, opt-out and false-positive environments before mutation', async () => { + await withReviewApiServer(async ({ apiUrl, state }) => { + let { cwd, vizzlyHome } = await createWorkspace(apiUrl); + try { + let base = { VIZZLY_API_URL: apiUrl, VIZZLY_HOME: vizzlyHome }; + let conflict = { ...base, CODEX_THREAD_ID: 'secret', CLAUDECODE: '1' }; + let blocked = await runCLI( + ['--json', 'comment', state.buildId, 'Conflict'], + { cwd, env: conflict } + ); + assert.equal(blocked.code, 1); + assert.match(blocked.stdout, /AMBIGUOUS_AGENT_ATTRIBUTION/); + assert.equal(state.comments.length, 0); + for (let [flags, env, slug] of [ + [ + ['--via-agent', 'pi'], + { ...conflict, VIZZLY_VIA_AGENT: 'gemini-cli' }, + 'pi', + ], + [['--no-agent-attribution'], conflict, null], + [[], { ...conflict, VIZZLY_VIA_AGENT: 'opencode' }, 'opencode'], + [[], { ...conflict, VIZZLY_VIA_AGENT: 'none' }, null], + [ + [], + { + ...base, + TERM_PROGRAM: 'vscode', + VSCODE_PID: '1', + AGENT: '1', + CI: 'true', + GEMINI_CLI: '0', + CLAUDECODE: '0', + }, + null, + ], + [[], base, null], + ]) { + let result = await runCLI( + ['--json', ...flags, 'comment', state.buildId, 'Review note'], + { cwd, env } + ); + assert.equal(result.code, 0, result.stdout + result.stderr); + assert.equal(state.comments.at(-1).via?.slug ?? null, slug); + } + let raw = await runCLI( + [ + '--json', + '--via-agent', + 'codex', + 'api', + `/api/sdk/builds/${state.buildId}/comments`, + '-X', + 'POST', + '-d', + '{"content":"Raw review note"}', + ], + { cwd, env: base } + ); + assert.equal(raw.code, 0, raw.stdout + raw.stderr); + assert.equal(state.comments.at(-1).via.slug, 'codex'); + let count = state.comments.length; + for (let flags of [ + ['--via-agent', 'BAD'], + ['--via-agent', 'codex', '--no-agent-attribution'], + ]) { + let result = await runCLI( + ['--json', ...flags, 'comment', state.buildId, 'Invalid'], + { cwd, env: base } + ); + assert.equal(result.code, 1); + assert.match(result.stdout, /INVALID_AGENT_ATTRIBUTION/); + } + assert.equal(state.comments.length, count); + } finally { + rmSync(cwd, { recursive: true, force: true }); + } + }); + }); + + it('rejects malformed or unscoped regions before sending comments', async () => { + for (let flags of [ + ['--region', '1,2,3,4'], + ['--screenshot', 'shot-1', '--region', '-1,2,3,4'], + ['--screenshot', 'shot-1', '--region', '1,2,0,4'], + ['--screenshot', 'shot-1', '--region', '1,,3,4'], + ]) { + let result = await runCLI([ + '--json', + 'comment', + 'build-1', + 'Note', + ...flags, + ]); + assert.equal(result.code, 1); + assert.match(result.stdout + result.stderr, /region/); + } + }); +}); + +it('review commands honor explicit project tokens and granted user scopes', async () => { + await withReviewApiServer(async ({ apiUrl, state }) => { + let { cwd, vizzlyHome } = await createWorkspace(apiUrl); + try { + let env = { + VIZZLY_API_URL: apiUrl, + VIZZLY_HOME: vizzlyHome, + VIZZLY_TOKEN: 'vzt_explicit-project', + }; + let denied = await runCLI( + ['--json', 'comment', state.buildId, 'Must not impersonate'], + { cwd, env } + ); + assert.equal(denied.code, 1); + assert.match(denied.stdout, /USER_AUTH_REQUIRED/); + assert.equal(state.comments.length, 0); + let user = await runCLI( + ['--json', 'comment', state.buildId, 'Chosen user', '--auth', 'user'], + { cwd, env } + ); + assert.equal(user.code, 0, user.stdout + user.stderr); + assert.equal(state.comments.length, 1); + await saveFixtureAuth(vizzlyHome, apiUrl, { + authorization: { ...fixtureAuthorization, scopes: ['read'] }, + }); + delete env.VIZZLY_TOKEN; + for (let args of [ + ['comment', state.buildId, 'No write scope'], + ['approve', state.comparisonId], + ]) { + let denied = await runCLI(['--json', ...args], { cwd, env }); + assert.equal(denied.code, 1); + assert.match(denied.stdout, /INSUFFICIENT_SCOPE/); + } + assert.equal(state.comments.length, 1); + assert.equal(state.decisionRequests.length, 0); + } finally { + rmSync(cwd, { recursive: true, force: true }); + } + }); +}); From 545603c48f4fc28cef35d334efbeb9f1bb9560eb Mon Sep 17 00:00:00 2001 From: Robert DeLuca Date: Thu, 5 Nov 2026 18:21:10 -0600 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=90=9B=20Remove=20the=20agent=20discl?= =?UTF-8?q?osure=20opt-out?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keep disclosure on for recognized agent harnesses. Explicit settings can name the agent, and invalid settings stop the review write. --- skills/vizzly/references/cli-context.md | 1 - src/adapters/cli/cli.js | 13 +++++------ src/domains/auth/agent-attribution.js | 29 ++++++++++--------------- tests/cli/review/review-cli.test.js | 23 ++++++++++++++------ 4 files changed, 33 insertions(+), 33 deletions(-) diff --git a/skills/vizzly/references/cli-context.md b/skills/vizzly/references/cli-context.md index 5ccf001c..2d9407bd 100644 --- a/skills/vizzly/references/cli-context.md +++ b/skills/vizzly/references/cli-context.md @@ -214,6 +214,5 @@ Gemini CLI or OpenCode when the harness exports its identification variable. The logged-in user remains the author; disclosure never grants permissions. Use `--via-agent ` for another agent or to resolve conflicting markers. `VIZZLY_VIA_AGENT` provides an environment override. Explicit flags win. -`--no-agent-attribution` or `VIZZLY_VIA_AGENT=none` omits disclosure. Only the reported agent name/slug is sent; session IDs and prompts stay local. Do not treat reported disclosure as verified identity or permission to act. diff --git a/src/adapters/cli/cli.js b/src/adapters/cli/cli.js index 9e65197a..e252af6e 100644 --- a/src/adapters/cli/cli.js +++ b/src/adapters/cli/cli.js @@ -560,7 +560,6 @@ program '--via-agent ', 'Disclose the agent assisting a review or comment' ) - .option('--no-agent-attribution', 'Omit agent disclosure for this action') .option('-v, --verbose', 'Verbose output (shorthand for --log-level debug)') .option( '--log-level ', @@ -1412,8 +1411,8 @@ Examples: $ vizzly approve abc123 -m "LGTM" # Approve with comment $ vizzly approve abc123 --json # Output as JSON for scripting -Known agents are disclosed automatically. Override with --via-agent or -omit disclosure with --no-agent-attribution. VIZZLY_VIA_AGENT sets the default. +Known agents are disclosed automatically. Set the agent with --via-agent +or VIZZLY_VIA_AGENT. Workflow: 1. List comparisons: vizzly comparisons -b @@ -1446,8 +1445,8 @@ Examples: $ vizzly reject abc123 --reason "Needs design review" $ vizzly reject abc123 -r "Regression" --json -Known agents are disclosed automatically. Override with --via-agent or -omit disclosure with --no-agent-attribution. VIZZLY_VIA_AGENT sets the default. +Known agents are disclosed automatically. Set the agent with --via-agent +or VIZZLY_VIA_AGENT. Workflow: 1. List comparisons: vizzly comparisons -b @@ -1497,8 +1496,8 @@ Examples: $ vizzly comment "Header overlaps" --screenshot --region 20,40,300,80 $ vizzly comment "Reviewed" --via-agent codex -Agent disclosure is automatic in known harnesses. VIZZLY_VIA_AGENT sets an -override; use --no-agent-attribution (or VIZZLY_VIA_AGENT=none) to omit it. +Known agents are disclosed automatically. Set the agent with --via-agent +or VIZZLY_VIA_AGENT. Only agent details are sent; session and thread IDs stay local. Workflow: diff --git a/src/domains/auth/agent-attribution.js b/src/domains/auth/agent-attribution.js index b5f925be..b40c9456 100644 --- a/src/domains/auth/agent-attribution.js +++ b/src/domains/auth/agent-attribution.js @@ -21,6 +21,12 @@ let agents = [ ]; function agentVia(slug) { + if (slug === 'none') { + throw new VizzlyError( + 'Use an agent slug. Agent attribution cannot be turned off.', + 'INVALID_AGENT_ATTRIBUTION' + ); + } if (!/^[a-z][a-z0-9]*(?:-[a-z0-9]+)*$/.test(slug) || slug.length > 48) { throw new VizzlyError( 'Agent slug must be lowercase words separated by hyphens (maximum 48 characters).', @@ -36,22 +42,12 @@ function agentVia(slug) { } export function resolveAgentAttribution(options = {}, env = process.env) { - if (options.viaAgent && options.agentAttribution === false) { - throw new VizzlyError( - 'Choose --via-agent or --no-agent-attribution.', - 'INVALID_AGENT_ATTRIBUTION' - ); - } - if (options.agentAttribution === false) return null; - if (options.viaAgent) return agentVia(options.viaAgent); - if (env.VIZZLY_VIA_AGENT) - return env.VIZZLY_VIA_AGENT === 'none' - ? null - : agentVia(env.VIZZLY_VIA_AGENT); + let override = options.viaAgent || env.VIZZLY_VIA_AGENT; + if (override) return agentVia(override); let matches = agents.filter(agent => agent.matches(env)); if (matches.length > 1) { throw new VizzlyError( - `Multiple agents detected: ${matches.map(agent => agent.name).join(', ')}. Use --via-agent or --no-agent-attribution.`, + `Multiple agents detected: ${matches.map(agent => agent.name).join(', ')}. Use --via-agent to choose the agent.`, 'AMBIGUOUS_AGENT_ATTRIBUTION' ); } @@ -84,10 +80,7 @@ export function attributeReviewRequest( 'INVALID_REQUEST_BODY' ); } - let overridden = - options.viaAgent || - options.agentAttribution === false || - env.VIZZLY_VIA_AGENT; + let overridden = options.viaAgent || env.VIZZLY_VIA_AGENT; if (overridden || !Object.hasOwn(body, 'via')) { let { via: _previous, ...content } = body; body = via ? { ...content, via } : content; @@ -100,7 +93,7 @@ export function attributeReviewRequest( (body.via.version ?? null) !== via.version) ) { throw new VizzlyError( - 'Request via conflicts with the detected agent. Select --via-agent or --no-agent-attribution.', + 'Request via conflicts with the detected agent. Use --via-agent to choose the agent.', 'AMBIGUOUS_AGENT_ATTRIBUTION' ); } diff --git a/tests/cli/review/review-cli.test.js b/tests/cli/review/review-cli.test.js index 838d8c17..83e5c2c4 100644 --- a/tests/cli/review/review-cli.test.js +++ b/tests/cli/review/review-cli.test.js @@ -461,7 +461,7 @@ describe('agent review disclosure through the CLI', () => { }); } - it('handles conflicts, explicit overrides, opt-out and false-positive environments before mutation', async () => { + it('attributes recognized agents and lets callers identify the agent explicitly', async () => { await withReviewApiServer(async ({ apiUrl, state }) => { let { cwd, vizzlyHome } = await createWorkspace(apiUrl); try { @@ -480,9 +480,7 @@ describe('agent review disclosure through the CLI', () => { { ...conflict, VIZZLY_VIA_AGENT: 'gemini-cli' }, 'pi', ], - [['--no-agent-attribution'], conflict, null], [[], { ...conflict, VIZZLY_VIA_AGENT: 'opencode' }, 'opencode'], - [[], { ...conflict, VIZZLY_VIA_AGENT: 'none' }, null], [ [], { @@ -522,17 +520,28 @@ describe('agent review disclosure through the CLI', () => { assert.equal(raw.code, 0, raw.stdout + raw.stderr); assert.equal(state.comments.at(-1).via.slug, 'codex'); let count = state.comments.length; - for (let flags of [ - ['--via-agent', 'BAD'], - ['--via-agent', 'codex', '--no-agent-attribution'], + let detected = { ...base, CODEX_THREAD_ID: 'private-thread' }; + for (let [flags, env] of [ + [['--via-agent', 'BAD'], detected], + [['--via-agent', 'none'], detected], + [[], { ...detected, VIZZLY_VIA_AGENT: 'none' }], ]) { let result = await runCLI( ['--json', ...flags, 'comment', state.buildId, 'Invalid'], - { cwd, env: base } + { cwd, env } ); assert.equal(result.code, 1); assert.match(result.stdout, /INVALID_AGENT_ATTRIBUTION/); } + let removedFlag = await runCLI( + ['--no-agent-attribution', 'comment', state.buildId, 'Invalid'], + { cwd, env: detected } + ); + assert.equal(removedFlag.code, 1); + assert.match( + removedFlag.stderr, + /unknown option.*--no-agent-attribution/ + ); assert.equal(state.comments.length, count); } finally { rmSync(cwd, { recursive: true, force: true }); From 17bb98c1046b4542250dc7930b0380313554baab Mon Sep 17 00:00:00 2001 From: Robert DeLuca Date: Fri, 6 Nov 2026 00:54:06 -0600 Subject: [PATCH 3/3] =?UTF-8?q?=E2=9C=A8=20Add=20comment=20layers=20and=20?= =?UTF-8?q?retire=20comment=20types?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add --layer current|baseline|diff and --point x,y to vizzly comment. Both require --screenshot and are rejected with --reply-to, since replies inherit their thread's layer and location. Remove --type; the server still defaults it to general. --- docs/json-output.md | 5 ++ skills/vizzly/references/cli-context.md | 12 ++- src/adapters/cli/cli.js | 29 ++++--- src/domains/review/review.commands.js | 78 ++++++++++++++----- src/domains/review/review.commands.test.js | 90 +++++++++++++++++++--- tests/cli/review/review-cli.test.js | 52 +++++++++++-- 6 files changed, 217 insertions(+), 49 deletions(-) diff --git a/docs/json-output.md b/docs/json-output.md index fc6b3ec4..a660d224 100644 --- a/docs/json-output.md +++ b/docs/json-output.md @@ -748,6 +748,7 @@ vizzly reject --reason "Unexpected regression" --json ```bash vizzly comment "Needs design review" --json +vizzly comment "Old footer spacing" --screenshot --layer baseline --region 40,200,320,120 --json ``` ```json @@ -756,11 +757,15 @@ vizzly comment "Needs design review" --json "data": { "created": true, "buildId": "build_123", + "layer": "baseline", "comment": {} } } ``` +`layer` is included for screenshot comments (`current`, `baseline`, or +`diff`). + Sign in with `vizzly login` to approve, reject, or comment. Project tokens can't perform review actions. diff --git a/skills/vizzly/references/cli-context.md b/skills/vizzly/references/cli-context.md index 2d9407bd..f460fccd 100644 --- a/skills/vizzly/references/cli-context.md +++ b/skills/vizzly/references/cli-context.md @@ -201,11 +201,19 @@ help explain a review. ### Comments and agent disclosure After the user authorizes a review action, comment on the build or the exact -screenshot area using IDs from discovery. Regions use the original bitmap's -pixels, with the origin at its top-left; use the image dimensions, not the viewport. +screenshot area using IDs from discovery. `--layer current|baseline|diff` +picks the image you're commenting on and defaults to `current`. Regions use +that layer's original bitmap pixels, with the origin at its top-left; use the +image dimensions, not the viewport. Current and diff share the current image's +pixels, while baseline uses the baseline image, which can be a different +height. `--point x,y` pins a spot as integer percentages (0-100) instead of a +region. Replies inherit their thread's layer and location, so omit `--layer`, +`--region`, and `--point` with `--reply-to`. ```bash vizzly comment "Navigation overlaps the heading" --screenshot --region 20,40,300,80 --json +vizzly comment "Old footer spacing" --screenshot --layer baseline --region 40,200,320,120 --json +vizzly comment "Button shifted" --screenshot --layer diff --point 50,12 --json vizzly comment "Follow-up" --screenshot --reply-to --json ``` diff --git a/src/adapters/cli/cli.js b/src/adapters/cli/cli.js index e252af6e..642a5974 100644 --- a/src/adapters/cli/cli.js +++ b/src/adapters/cli/cli.js @@ -1411,6 +1411,10 @@ Examples: $ vizzly approve abc123 -m "LGTM" # Approve with comment $ vizzly approve abc123 --json # Output as JSON for scripting +Regions use the chosen layer's bitmap pixels. Current and diff share the +current image's pixels; baseline uses the baseline image, which can differ in +height. Replies inherit their thread's layer and location. + Known agents are disclosed automatically. Set the agent with --via-agent or VIZZLY_VIA_AGENT. @@ -1445,6 +1449,10 @@ Examples: $ vizzly reject abc123 --reason "Needs design review" $ vizzly reject abc123 -r "Regression" --json +Regions use the chosen layer's bitmap pixels. Current and diff share the +current image's pixels; baseline uses the baseline image, which can differ in +height. Replies inherit their thread's layer and location. + Known agents are disclosed automatically. Set the agent with --via-agent or VIZZLY_VIA_AGENT. @@ -1470,32 +1478,31 @@ program .option('--auth ', 'Credential source: auto, user, token', 'auto') .description('Add a comment to a build or screenshot') .option('--screenshot ', 'Screenshot ID in this build') - .option( - '--region ', - 'Screenshot region in bitmap pixels from the top-left' - ) + .option('--layer ', 'Image: current (default), baseline, diff') + .option('--region ', "Region in the layer's bitmap pixels") + .option('--point ', 'Point as 0-100 percentages') .option( '--reply-to ', 'Reply to a comment on the same build or screenshot' ) .argument('', 'Build ID to comment on') .argument('', 'Comment message') - .option( - '-t, --type ', - 'Comment type: general, approval, rejection, question', - 'general' - ) .addHelpText( 'after', ` Examples: $ vizzly comment abc123 "Looks good overall" - $ vizzly comment abc123 "Approved" -t approval - $ vizzly comment abc123 "Please fix the header" -t rejection $ vizzly comment abc123 "CI feedback" --json $ vizzly comment "Header overlaps" --screenshot --region 20,40,300,80 + $ vizzly comment "Old footer spacing" --screenshot --layer baseline --region 40,200,320,120 + $ vizzly comment "Button shifted" --screenshot --layer diff --point 50,12 + $ vizzly comment "Agreed" --screenshot --reply-to $ vizzly comment "Reviewed" --via-agent codex +Regions use the chosen layer's bitmap pixels. Current and diff share the +current image's pixels; baseline uses the baseline image, which can differ in +height. Replies inherit their thread's layer and location. + Known agents are disclosed automatically. Set the agent with --via-agent or VIZZLY_VIA_AGENT. Only agent details are sent; session and thread IDs stay local. diff --git a/src/domains/review/review.commands.js b/src/domains/review/review.commands.js index 4fb4ee1e..6b070f72 100644 --- a/src/domains/review/review.commands.js +++ b/src/domains/review/review.commands.js @@ -151,8 +151,9 @@ export function createRejectionBody(options = {}) { export function createCommentBody(message, options = {}) { return { content: message, - type: options.type || 'general', + ...(options.layer ? { layer: options.layer } : {}), ...(options.region ? { region: parseCommentRegion(options.region) } : {}), + ...(options.point ? parseCommentPoint(options.point) : {}), ...(options.replyTo ? { parentCommentId: options.replyTo } : {}), }; } @@ -386,13 +387,22 @@ export async function commentCommand( output.data({ created: true, buildId, + ...(options.screenshot + ? { layer: getCommentLayer(response, options) } + : {}), comment: response.comment, }), writeHumanSuccess: (output, response) => { output.complete('Comment added'); if (options.screenshot) output.labelValue('Screenshot', options.screenshot); + if (options.screenshot) + output.labelValue('Layer', getCommentLayer(response, options)); if (options.region) output.labelValue('Region', options.region); + if (options.point) { + let { positionX, positionY } = parseCommentPoint(options.point); + output.labelValue('Point', `${positionX}%, ${positionY}%`); + } if (response.comment?.attribution?.via) output.labelValue('Agent', response.comment.attribution.via.name); output.labelValue('Build', buildId); @@ -434,13 +444,12 @@ export function validateRejectOptions(comparisonId, options = {}) { return errors; } -/** - * Validate comment command options - * @param {string} buildId - Build ID - * @param {string} message - Comment message - * @param {Object} options - Command options - * @returns {string[]} Array of error messages - */ +let COMMENT_LAYERS = ['current', 'baseline', 'diff']; + +function getCommentLayer(response, options) { + return response.comment?.layer || options.layer || 'current'; +} + export function parseCommentRegion(value) { let numbers = value.split(',').map(Number); if ( @@ -460,6 +469,31 @@ export function parseCommentRegion(value) { return { x, y, width, height }; } +export function parseCommentPoint(value) { + let parts = value.split(','); + let numbers = parts.map(Number); + if ( + numbers.length !== 2 || + parts.some(part => !part.trim()) || + numbers.some( + number => !Number.isSafeInteger(number) || number < 0 || number > 100 + ) + ) { + throw new Error( + '--point requires x,y as integer percentages from 0 to 100' + ); + } + let [positionX, positionY] = numbers; + return { positionX, positionY }; +} + +/** + * Validate comment command options + * @param {string} buildId - Build ID + * @param {string} message - Comment message + * @param {Object} options - Command options + * @returns {string[]} Array of error messages + */ export function validateCommentOptions(buildId, message, options = {}) { let errors = []; if (!buildId || buildId.trim() === '') { @@ -468,21 +502,23 @@ export function validateCommentOptions(buildId, message, options = {}) { if (!message || message.trim() === '') { errors.push('Comment message is required'); } - if ( - options.type && - !['general', 'approval', 'rejection', 'question'].includes(options.type) - ) { - errors.push( - '--type must be one of: general, approval, rejection, question' - ); + if (options.layer && !COMMENT_LAYERS.includes(options.layer)) + errors.push(`--layer must be one of: ${COMMENT_LAYERS.join(', ')}`); + for (let flag of ['layer', 'region', 'point']) { + if (!options[flag]) continue; + if (!options.screenshot) errors.push(`--${flag} requires --screenshot`); + if (options.replyTo) + errors.push(`Replies inherit their thread's ${flag}; omit --${flag}`); } - if (options.region && !options.screenshot) - errors.push('--region requires --screenshot'); - if (options.region && options.replyTo) - errors.push('Replies inherit their location; omit --region'); - if (options.region) { + if (options.region && options.point) + errors.push('Use either --region or --point, not both'); + for (let [flag, parse] of [ + ['region', parseCommentRegion], + ['point', parseCommentPoint], + ]) { + if (!options[flag]) continue; try { - parseCommentRegion(options.region); + parse(options[flag]); } catch (error) { errors.push(error.message); } diff --git a/src/domains/review/review.commands.test.js b/src/domains/review/review.commands.test.js index 1599c7a7..11e9fc16 100644 --- a/src/domains/review/review.commands.test.js +++ b/src/domains/review/review.commands.test.js @@ -31,17 +31,68 @@ describe('commands/review', () => { it('validates comment inputs', () => { assert.deepStrictEqual( - validateCommentOptions('build-1', 'Looks good', { type: 'approval' }), + validateCommentOptions('build-1', 'Looks good', {}), [] ); + assert.deepStrictEqual(validateCommentOptions('', '', {}), [ + 'Build ID is required', + 'Comment message is required', + ]); + }); + + it('validates screenshot layers, regions, and points', () => { + let screenshot = 'shot-1'; + for (let layer of ['current', 'baseline', 'diff']) { + assert.deepStrictEqual( + validateCommentOptions('build-1', 'Note', { + screenshot, + layer, + region: '40,200,320,120', + }), + [] + ); + } + assert.deepStrictEqual( + validateCommentOptions('build-1', 'Note', { + screenshot, + point: '0,100', + }), + [] + ); + assert.deepStrictEqual( + validateCommentOptions('build-1', 'Note', { layer: 'before' }), + [ + '--layer must be one of: current, baseline, diff', + '--layer requires --screenshot', + ] + ); assert.deepStrictEqual( - validateCommentOptions('', '', { type: 'unknown' }), + validateCommentOptions('build-1', 'Note', { + screenshot, + replyTo: 'comment-1', + layer: 'baseline', + point: '10,10', + }), [ - 'Build ID is required', - 'Comment message is required', - '--type must be one of: general, approval, rejection, question', + "Replies inherit their thread's layer; omit --layer", + "Replies inherit their thread's point; omit --point", ] ); + assert.deepStrictEqual( + validateCommentOptions('build-1', 'Note', { + screenshot, + region: '1,2,3,4', + point: '10,10', + }), + ['Use either --region or --point, not both'] + ); + for (let point of ['101,5', '5', '1.5,2', ',2', '-1,2']) { + assert.deepStrictEqual( + validateCommentOptions('build-1', 'Note', { screenshot, point }), + ['--point requires x,y as integer percentages from 0 to 100'], + point + ); + } }); }); @@ -56,15 +107,36 @@ describe('commands/review', () => { }); assert.deepStrictEqual(createCommentBody('Needs follow-up', {}), { content: 'Needs follow-up', - type: 'general', }); assert.deepStrictEqual( - createCommentBody('Approved', { type: 'approval' }), + createCommentBody('Old footer spacing', { + screenshot: 'shot-1', + layer: 'baseline', + region: '40,200,320,120', + }), { - content: 'Approved', - type: 'approval', + content: 'Old footer spacing', + layer: 'baseline', + region: { x: 40, y: 200, width: 320, height: 120 }, } ); + assert.deepStrictEqual( + createCommentBody('Button shifted', { + screenshot: 'shot-1', + layer: 'diff', + point: '50,12', + }), + { + content: 'Button shifted', + layer: 'diff', + positionX: 50, + positionY: 12, + } + ); + assert.deepStrictEqual( + createCommentBody('Agreed', { replyTo: 'comment-1' }), + { content: 'Agreed', parentCommentId: 'comment-1' } + ); }); }); }); diff --git a/tests/cli/review/review-cli.test.js b/tests/cli/review/review-cli.test.js index 83e5c2c4..f608c978 100644 --- a/tests/cli/review/review-cli.test.js +++ b/tests/cli/review/review-cli.test.js @@ -334,17 +334,52 @@ describe('commands/review CLI', () => { 'comment', state.buildId, 'Looks good', - '--type', - 'approval', + '--screenshot', + 'shot-1', + '--layer', + 'baseline', + '--region', + '40,200,320,120', ], { cwd, env } ); assert.equal(commented.code, 0, commented.stderr); assert.match(commented.stdout + commented.stderr, /Comment added/); assert.match(commented.stdout + commented.stderr, /Looks good/); + assert.match(commented.stdout + commented.stderr, /Layer: baseline/); assert.deepEqual(state.comments, [ - { content: 'Looks good', type: 'approval' }, + { + content: 'Looks good', + layer: 'baseline', + region: { x: 40, y: 200, width: 320, height: 120 }, + }, ]); + let pinned = await runCLI( + [ + '--json', + 'comment', + state.buildId, + 'Button shifted', + '--screenshot', + 'shot-1', + '--point', + '50,12', + ], + { cwd, env } + ); + assert.equal(pinned.code, 0, pinned.stderr); + assert.equal(parseResult(pinned).layer, 'current'); + assert.deepEqual(state.comments.at(-1), { + content: 'Button shifted', + positionX: 50, + positionY: 12, + }); + let removedType = await runCLI( + ['comment', state.buildId, 'Old client', '--type', 'approval'], + { cwd, env } + ); + assert.equal(removedType.code, 1); + assert.match(removedType.stderr, /unknown option.*--type/); state.commentStatus = 503; let failed = await runCLI( ['--json', 'comment', state.buildId, 'Follow up'], @@ -356,7 +391,7 @@ describe('commands/review CLI', () => { parseResult(failed).error.message, /Comment service unavailable/ ); - assert.equal(state.comments.length, 2); + assert.equal(state.comments.length, 3); assert.equal(state.refreshRequests, 0); }); }); @@ -549,9 +584,14 @@ describe('agent review disclosure through the CLI', () => { }); }); - it('rejects malformed or unscoped regions before sending comments', async () => { + it('rejects malformed or unscoped locations before sending comments', async () => { for (let flags of [ ['--region', '1,2,3,4'], + ['--layer', 'baseline'], + ['--screenshot', 'shot-1', '--layer', 'before'], + ['--screenshot', 'shot-1', '--reply-to', 'c-1', '--layer', 'baseline'], + ['--screenshot', 'shot-1', '--point', '101,5'], + ['--screenshot', 'shot-1', '--point', '5,5', '--region', '1,2,3,4'], ['--screenshot', 'shot-1', '--region', '-1,2,3,4'], ['--screenshot', 'shot-1', '--region', '1,2,0,4'], ['--screenshot', 'shot-1', '--region', '1,,3,4'], @@ -564,7 +604,7 @@ describe('agent review disclosure through the CLI', () => { ...flags, ]); assert.equal(result.code, 1); - assert.match(result.stdout + result.stderr, /region/); + assert.match(result.stdout + result.stderr, /region|layer|point/); } }); });