✨ Add JSON output, review commands, and user auth for CLI

[Robdel12]

Summary

Comprehensive update to make the CLI fully scriptable and LLM-friendly:

JSON Output (--json)

• All commands support --json for machine-readable output
• Optional field selection: --json field1,field2
• Consistent output format: {status, data} or {status, error}

New Discovery Commands

• vizzly orgs - List organizations user has access to
• vizzly projects - List projects (with --org filter)
• vizzly builds - List and query builds
• vizzly comparisons - Query and search comparisons
• vizzly baselines - List local TDD baselines

New Review Commands

• vizzly approve <comparison-id> - Approve a comparison
• vizzly reject <comparison-id> -r "reason" - Reject with reason
• vizzly comment <build-id> "message" - Add build comment

User Authentication

• Config loader now falls back to user JWT from vizzly login
• Enables interactive CLI usage without project tokens
• Works alongside existing project token auth for CI

Developer Experience

• Help examples added to all new commands
• Workflow guidance in help output
• npm run cli script for local development

Test plan

All commands manually tested against local API:

• vizzly whoami --json - Shows auth status, user info, orgs
• vizzly orgs --json - Lists 2 orgs with role and project count
• vizzly projects --json - Lists projects with build counts
• vizzly builds --json --limit 3 - Lists builds with pagination
• vizzly builds -b <id> --json - Gets single build details
• vizzly builds -b <id> --comparisons --json - Build with comparisons
• vizzly comparisons -b <id> --json - Lists comparisons for build
• vizzly comparisons --id <id> --json - Gets single comparison
• vizzly comparisons --name "index" --json - Searches by name
• vizzly approve <id> --json - Approved, verified user ID in audit trail
• vizzly reject <id> -r "Testing CLI rejection" --json - Rejected with reason
• vizzly comment <build-id> "Testing build comments from CLI" --json - Comment created with user attribution
• vizzly baselines --json - Lists 21 local baselines
• vizzly api /api/sdk/builds -q limit=1 --json - Raw API access works
• vizzly builds --help - Shows examples and workflow guidance
• vizzly approve --help - Shows examples with workflow steps
• Human-readable output tested for all commands

[Robdel12]

@claude can you review this PR?

[claude]

Claude finished @Robdel12's task in 1m 57s —— View job

---

PR Review: ✨ Add JSON output and query commands

I've reviewed the code changes for this PR. Overall, this is a well-structured addition that successfully makes the CLI scriptable. The implementation is clean, follows existing patterns, and has good test coverage. Below are my findings:

---

✅ Strengths

1. Architecture & Design

• Clean separation of concerns: JSON formatting is centralized in output.js with consistent field selection via selectFields() and parseJsonFields() (output.js:82-153)
• Consistent API: All commands follow the same pattern for JSON output and field selection
• Proper stream separation: stdout for data, stderr for UI elements - critical for piping (docs/json-output.md:15-16)

2. Security

• Good POST endpoint whitelisting in api.js:187-198 - only allows approve/reject/comment mutations
• Token protection: Config command masks tokens showing only prefix (config-cmd.js:62)
• Input validation: All commands have validation functions

3. Code Quality

• Dependency injection: All commands support testable deps pattern
• Error handling: Comprehensive try/catch with proper error formatting in JSON mode
• Test coverage: New tests for builds, comparisons, config commands

4. Documentation

• Excellent JSON output reference doc (docs/json-output.md) with examples for every command
• Clear examples of field selection and scripting patterns

---

🔍 Issues & Recommendations

Critical

1. Security: API token exposure risk in config command (config-cmd.js:58-64)

The config command could expose API tokens if field selection is used:

// config-cmd.js:58-64
if (config.apiKey) {
  displayConfig.api = {
    url: config.apiUrl || config.baseUrl,
    tokenConfigured: true,
    tokenPrefix: `${config.apiKey.substring(0, 8)}...`,  // ⚠️ This is good
  };
}

However, when a user does:

vizzly config --json api

The field selection in output.js:135-153 would return the masked tokenPrefix, but if someone tries to get a nested field that doesn't exist in displayConfig but exists in the raw config object, they might be able to access the full token.

Recommendation: Ensure that field selection ONLY operates on displayConfig, not the raw config object. Review the implementation to confirm this is the case.

---

2. Missing validation for review commands (review.js)

The review commands (approve, reject, comment) don't have validation functions exported, unlike other commands:

// src/cli.js:774-806 - No validateApproveOptions, validateRejectOptions, validateCommentOptions
program
  .command('approve')
  .argument('<comparison-id>', 'Comparison ID to approve')
  .action(async (comparisonId, options) => {
    // No validation here
    await approveCommand(comparisonId, options, globalOptions);
  });

Recommendation: Add validation functions for consistency:

export function validateApproveOptions(comparisonId, options = {}) {
  let errors = [];
  if (!comparisonId || comparisonId.trim() === '') {
    errors.push('Comparison ID is required');
  }
  return errors;
}

---

High Priority

3. Inconsistent error handling in JSON mode (multiple files)

Some commands exit with code 1 on error in JSON mode without calling output.cleanup() first:

// api.js:174
output.data({ /* error data */ });
output.cleanup();
exit(1);
// ✅ Good - cleanup before exit

// But in some error paths:
output.error('...');
exit(1);
// ❌ Missing cleanup

Recommendation: Audit all error paths to ensure output.cleanup() is called before exit().

---

4. Field selection doesn't validate field existence (output.js:135-153)

The selectFields() function silently ignores missing fields:

function selectFields(obj, fields) {
  // ...
  for (let field of fields) {
    let value = getNestedValue(obj, field);
    if (value !== undefined) {  // ⚠️ Silently skips missing fields
      setNestedValue(result, field, value);
    }
  }
  return result;
}

This could confuse users who misspell field names. They'll get an empty object instead of an error.

Recommendation: Consider adding a --strict mode or warning when requested fields don't exist.

---

Medium Priority

5. API command allows arbitrary GET requests (api.js:61-70)

While POST is properly restricted, GET has no validation:

// api.js:49-55
let normalizedEndpoint = endpoint.startsWith('/')
  ? endpoint
  : `/${endpoint}`;
if (!normalizedEndpoint.startsWith('/api/')) {
  normalizedEndpoint = `/api${normalizedEndpoint}`;
}

This could allow users to hit internal endpoints if the API doesn't have proper authentication checks.

Recommendation: Consider adding a whitelist of allowed GET endpoint patterns, similar to the POST whitelist. Or at least document that this is a "power user" feature.

---

6. Baselines command has potential race condition (baselines.js:88-100)

if (existsSync(baselinesDir)) {
  baselineFiles = readdirSync(baselinesDir)
    .filter(f => f.endsWith('.png'))
    .map(f => {
      let filePath = join(baselinesDir, f);
      let stat = statSync(filePath);  // ⚠️ Could fail if file deleted between readdir and stat
      // ...
    });
}

Recommendation: Wrap statSync in try/catch to handle deleted files gracefully.

---

7. Missing JSDoc for some exported functions

While most functions have JSDoc comments, some are missing:

• formatBuildForJson in builds.js:174
• formatComparisonForJson in comparisons.js:188
• Field selection helper functions in output.js

Recommendation: Add JSDoc for consistency and better IDE support.

---

Low Priority / Nice to Have

8. Pagination could be more intuitive

The pagination info shows "Use --offset 20" but doesn't show the limit:

// builds.js:157-161
if (pagination.total > builds.length) {
  output.hint(
    `Showing ${builds.length} of ${pagination.total} builds. ` +
      `Use --offset ${filters.offset + filters.limit} to see more.`
  );
}

Recommendation: Include the full command: Use --offset ${filters.offset + filters.limit} --limit ${filters.limit} for copy-paste convenience.

---

9. JSON output structure inconsistency

Most commands wrap data in { status: 'data', data: {...} } but some don't always follow this:

// builds.js:71 - Direct output
output.data(formatBuildForJson(build, options.comparisons));

// vs comparisons.js:68 - Also direct
output.data(formatComparisonForJson(comparison));

According to docs/json-output.md:35-39, all output should have the wrapper. The output.data() function adds this (output.js:413), so this is actually correct. Just noting for clarity.

---

10. Test coverage gaps

While there are tests for new commands, I'd like to see:

• Error cases in review commands
• Field selection edge cases (nested fields, arrays, missing fields)
• API command POST endpoint validation

---

📋 Minor Issues

1. Unused import: getApiUrl is imported but only used in human-readable output for URL display (builds.js:11, 128)
2. Code duplication: getStatusIcon appears in both builds.js:296 and comparisons.js:392 - could be extracted to a shared utility
3. Magic numbers: Hardcoded limits like 20, 50, 100, 250 scattered throughout - consider constants
4. plans/menubar-app.md: This file seems unrelated to this PR's purpose - was it accidentally included?

---

🎯 Summary

Approval recommendation: ✅ Approve with minor fixes

This PR successfully achieves its goal of making the CLI scriptable. The implementation is solid, follows good patterns, and is well-documented. The security measures for the API command are appropriate.

Must fix before merge:

1. Validate that config command doesn't leak tokens via field selection (Critical ✨ Add npmignore file, reduce release size #1)
2. Add validation functions for review commands (Critical ♻️ Run lint before tests #2)

Should fix:
3. Audit error paths for cleanup consistency (High #3)
4. Add field selection validation or warnings (High #4)

Nice to have:
5. Consider GET endpoint restrictions for api command (Medium #5)
6. Add error handling for baseline file race condition (Medium #6)

The test failures shown in the Vizzly comment appear to be visual regression tests for the CLI help output, which makes sense given the new commands added. Those should be reviewed and approved in the Vizzly dashboard.

---

Great work on this PR! The JSON output feature will be very valuable for automation and AI agent integration. 🚀

---

[Robdel12]

@claude please rereview

[claude]

Claude finished @Robdel12's task in 2m 33s —— View job

---

PR Re-Review: ✨ Add JSON output and query commands

I've completed a thorough re-review of this PR. Great news: the two critical issues from my previous review have been addressed. The code is well-structured and follows good patterns. Below are my detailed findings.

---

✅ Critical Issues Resolved

1. Token Security (Previously Critical #1)

The config command properly isolates token data. Field selection operates exclusively on displayConfig (config-cmd.js:68, 94), which only contains the masked tokenPrefix (line 62). The raw config.apiKey is never exposed to field selection. ✅

2. Validation Functions Added (Previously Critical #2)

All review commands now have proper validation functions (review.js:311-358):

• validateApproveOptions() - Validates comparison ID (lines 311-317)
• validateRejectOptions() - Validates ID and reason requirement (lines 325-334)
• validateCommentOptions() - Validates build ID, message, and type enum (lines 343-358)

These are properly exported and follow the same pattern as other commands. ✅

---

🔍 New Findings

High Priority

1. Inconsistent error path cleanup (api.js:163-175)

When errors occur in JSON mode, some paths call output.cleanup() before exit, but the implementation is inconsistent:

// api.js:163-175 - GOOD, has cleanup
if (globalOptions.json) {
  output.data({ /* error */ });
  output.cleanup();  // ✅
  exit(1);
  return;
}

However, checking review.js shows a similar pattern. The concern is whether ALL error paths consistently clean up. A quick audit would be valuable.

Recommendation: Run a grep to ensure all exit(1) calls in error handlers are preceded by output.cleanup() when appropriate.

---

2. Field selection doesn't warn on missing fields (output.js:136-153)

The selectFields() function silently ignores non-existent fields:

// output.js:146-150
for (let field of fields) {
  let value = getNestedValue(obj, field);
  if (value !== undefined) {  // Silently skips missing fields
    setNestedValue(result, field, value);
  }
}

Impact: Users who typo field names get an empty object instead of helpful feedback:

vizzly builds --json "builld.id"  # Typo - returns {}

Recommendation: Consider adding a verbose warning when requested fields aren't found, or add a --strict mode that errors on missing fields.

---

3. Race condition in baselines command (baselines.js:88-100)

There's a potential TOCTOU (time-of-check-time-of-use) issue:

// baselines.js:88-91
if (existsSync(baselinesDir)) {
  baselineFiles = readdirSync(baselinesDir)
    .filter(f => f.endsWith('.png'))
    .map(f => {
      let filePath = join(baselinesDir, f);
      let stat = statSync(filePath);  // ⚠️ File could be deleted here

Recommendation: Wrap statSync() in try/catch to handle deleted files gracefully.

---

Medium Priority

4. Missing JSDoc for formatter functions

While most code is well-documented, these functions lack JSDoc:

• formatBuildForJson() in builds.js:174
• formatComparisonForJson() in comparisons.js:188
• Helper functions in output.js (getNestedValue, setNestedValue, selectFields)

Recommendation: Add JSDoc for consistency and better IDE autocomplete.

---

5. API command allows arbitrary GET requests (api.js:49-70)

The API command only restricts POST endpoints but allows any GET endpoint:

// api.js:49-54
let normalizedEndpoint = endpoint.startsWith('/')
  ? endpoint
  : `/${endpoint}`;
if (!normalizedEndpoint.startsWith('/api/')) {
  normalizedEndpoint = `/api${normalizedEndpoint}`;
}

While the API should have proper authentication, this is a "power user" feature that could potentially access internal endpoints if the API doesn't validate tokens correctly.

Current status: This is acceptable as long as the backend enforces proper authentication. The POST whitelist (lines 187-191) is good security practice.

---

6. Code duplication opportunity

The status icon mapping appears in multiple files:

• builds.js:296 (getStatusIcon)
• comparisons.js:392 (getStatusIcon)

Recommendation: Extract to a shared utility in src/utils/status.js to maintain consistency.

---

Low Priority

7. Magic numbers scattered throughout

Hardcoded limits (20, 50, 100, 250) appear in multiple files without constants. For example:

• builds.js:82 - limit: options.limit || 20
• comparisons.js - similar patterns

Recommendation: Define constants like DEFAULT_PAGE_SIZE = 20 in a shared config file.

---

8. Pagination hint could be more helpful (builds.js:157-161)

Current:

output.hint(`Showing ${builds.length} of ${pagination.total} builds. ` +
  `Use --offset ${filters.offset + filters.limit} to see more.`);

Recommendation: Include the full command for copy-paste convenience:

`Use --offset ${filters.offset + filters.limit} --limit ${filters.limit} to see more.`

---

9. Test coverage gaps

While the PR includes tests for new commands, these areas could use more coverage:

• Error cases in review commands (approve/reject/comment failures)
• Field selection edge cases (nested fields, arrays, missing fields)
• API command POST endpoint validation (testing the whitelist)

Not blocking, but would strengthen the test suite.

---

📊 Code Quality Assessment

Strengths:

• ✅ Consistent architecture across all new commands
• ✅ Proper dependency injection for testability
• ✅ Good separation of concerns (commands, output, validation)
• ✅ Excellent documentation in docs/json-output.md
• ✅ Security-conscious (token masking, POST whitelist)
• ✅ Proper error handling with graceful degradation

Minor improvements needed:

• Error path cleanup consistency audit
• Field selection user feedback
• Race condition handling in baselines
• JSDoc completeness

---

🎯 Recommendation

✅ Approve with minor improvements

This PR successfully achieves its goal of making the CLI fully scriptable and LLM-friendly. The implementation is solid, secure, and well-tested. The two critical issues from the previous review have been properly addressed.

Before merging (optional improvements):

1. ✅ DONE - Token security verified
2. ✅ DONE - Validation functions added
3. 🔶 Optional - Add field selection warnings for missing fields
4. 🔶 Optional - Wrap baselines statSync in try/catch
5. 🔶 Optional - Add JSDoc to formatter functions

Visual regression tests:
The 9 visual changes shown in the Vizzly comment appear to be CLI help output updates from the new commands. These should be reviewed and approved in the Vizzly dashboard as expected changes.

---

📝 Summary

This is a comprehensive, well-executed addition to the CLI. The JSON output system is thoughtfully designed with field selection, the new commands follow consistent patterns, and the security measures are appropriate. The code quality is high, and the documentation is excellent.

The issues identified above are minor refinements rather than blockers. Great work on this PR! 🚀

---

[vizzly-testing]

Vizzly - Visual Test Results

CLI Reporter - 7 changes need review

Status	Count
Passed	12
Changed	7
Auto-approved	12

Changes needing review (7)

fullscreen-viewer · Firefox · 1920×1080 · 5.8% diff

bulk-accept-dialog · Firefox · 1920×1080 · 10.3% diff

filter-failed-only · Firefox · 1920×1080 · 0.1% diff

viewer-slide-mode · Firefox · 1920×1080 · 0.5% diff

viewer-zoomed-100 · Firefox · 1920×1080 · 0.3% diff

filter-failed-only · Firefox · 375×703 · 0.2% diff

...and 1 more in Vizzly.

Review changes

CLI TUI - 1 change needs review

Status	Count
Passed	4
Changed	1
Auto-approved	4

Changes needing review (1)

vizzly-help · 1202×1430 · 691.8% diff

Review changes

---

_{rd/json-cli · a1f095ee}