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 ef35f5fd..f460fccd 100644 --- a/skills/vizzly/references/cli-context.md +++ b/skills/vizzly/references/cli-context.md @@ -196,3 +196,31 @@ 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. `--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 +``` + +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. +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..642a5974 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,10 @@ 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('-v, --verbose', 'Verbose output (shorthand for --log-level debug)') .option( '--log-level ', @@ -1383,6 +1399,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 +1411,13 @@ 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. + Workflow: 1. List comparisons: vizzly comparisons -b 2. Review the changes in the web UI or via URLs in the output @@ -1413,6 +1437,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 +1449,13 @@ 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. + Workflow: 1. List comparisons: vizzly comparisons -b 2. Review the changes in the web UI or via URLs in the output @@ -1443,22 +1475,37 @@ Workflow: program .command('comment') - .description('Add a comment to a build') - .argument('', 'Build ID to comment on') - .argument('', 'Comment message') + .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('--layer ', 'Image: current (default), baseline, diff') + .option('--region ', "Region in the layer's bitmap pixels") + .option('--point ', 'Point as 0-100 percentages') .option( - '-t, --type ', - 'Comment type: general, approval, rejection', - 'general' + '--reply-to ', + 'Reply to a comment on the same build or screenshot' ) + .argument('', 'Build ID to comment on') + .argument('', 'Comment message') .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. 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..b40c9456 --- /dev/null +++ b/src/domains/auth/agent-attribution.js @@ -0,0 +1,101 @@ +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 (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).', + '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) { + 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 to choose the agent.`, + '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 || 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. Use --via-agent to choose the agent.', + '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..6b070f72 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), @@ -165,7 +151,10 @@ 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 } : {}), }; } @@ -198,14 +187,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 +196,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 +371,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, @@ -411,10 +387,24 @@ export async function commentCommand( output.data({ created: true, buildId, + ...(options.screenshot + ? { layer: getCommentLayer(response, options) } + : {}), comment: response.comment, }), - writeHumanSuccess: output => { + 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); output.labelValue('Message', message); }, @@ -454,6 +444,49 @@ export function validateRejectOptions(comparisonId, options = {}) { return errors; } +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 ( + 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 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 @@ -469,11 +502,26 @@ export function validateCommentOptions(buildId, message, options = {}) { if (!message || message.trim() === '') { errors.push('Comment message is required'); } - if ( - options.type && - !['general', 'approval', 'rejection'].includes(options.type) - ) { - errors.push('--type must be one of: general, approval, rejection'); + 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.point) + errors.push('Use either --region or --point, not both'); + for (let [flag, parse] of [ + ['region', parseCommentRegion], + ['point', parseCommentPoint], + ]) { + if (!options[flag]) continue; + try { + parse(options[flag]); + } 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..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', + "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 10a13b76..f608c978 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); @@ -332,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'], @@ -354,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); }); }); @@ -378,10 +415,238 @@ 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('attributes recognized agents and lets callers identify the agent explicitly', 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', + ], + [[], { ...conflict, VIZZLY_VIA_AGENT: 'opencode' }, 'opencode'], + [ + [], + { + ...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; + 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 } + ); + 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 }); + } + }); + }); + + 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'], + ]) { + let result = await runCLI([ + '--json', + 'comment', + 'build-1', + 'Note', + ...flags, + ]); + assert.equal(result.code, 1); + assert.match(result.stdout + result.stderr, /region|layer|point/); + } + }); +}); + +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 }); + } + }); +});