✨ Make finalize command resilient to missing builds

[Robdel12]

Summary

• Add global --strict flag for all CLI commands to opt into hard failures
• Handle 404 errors gracefully in finalize command (warn + exit 0 by default)
• Show helpful debugging hints when builds are missing (in verbose mode)
• Add tests for new resilient behavior

Why

The finalize command was breaking CI pipelines when no parallel build existed (e.g., tests were skipped, no screenshots uploaded, or mismatched parallel-id). This is a poor UX for CI workflows where Vizzly tests might be conditionally skipped.

Behavior

Default (resilient):

vizzly finalize abc123
# No build found for parallel ID: abc123 - finalize skipped.
# Exit code: 0

Strict mode:

vizzly finalize abc123 --strict
# Error: No build found for parallel ID: abc123
# Exit code: 1

Test plan

• Run node --test tests/commands/finalize.test.js - all 17 tests pass
• Run full test suite - all 1860 tests pass
• Manual test: vizzly finalize nonexistent-id exits 0 with warning
• Manual test: vizzly finalize nonexistent-id --strict exits 1 with error

Closes VIZ-108

[claude]

Code Review

This PR adds a global --strict flag and implements resilient error handling for missing builds in the finalize command. The implementation is solid with good test coverage. Here's my feedback:

✅ Strengths

1. Clear Problem & Solution: Addresses a real CI/CD pain point where missing builds shouldn't break pipelines by default
2. Well-Tested: 112 lines of new tests covering strict/non-strict modes and verbose output
3. Good UX: Helpful error messages with actionable debugging hints
4. Consistent Pattern: Follows existing 5xx error handling pattern
5. Proper DI: Uses dependency injection for testability
6. Backward Compatible: Default behavior is non-breaking (resilient mode)

🔍 Code Quality Observations

src/commands/finalize.js:109-133

The 404 handling logic is clear and well-structured. Minor suggestions:

1. Conditional logic clarity (L110): Consider extracting isStrict check earlier for readability:

   if (status === 404) {
     let isStrict = globalOptions.strict;
     let hints = [
       '  • No screenshots were uploaded with this parallel-id',
       '  • Tests were skipped or failed before capturing screenshots',
       '  • The parallel-id does not match what was used during test runs',
     ];

   This avoids repeating the same hints array in two places (L116-118 and L127-129).

2. Return consistency (L132): The non-strict return includes reason: 'no-build-found' while the 5xx handler at L104 doesn't. Consider if 5xx should also include a reason for consistency.

src/cli.js:239

The --strict flag description is clear. However, consider:

1. Documentation: Should this flag be documented in README.md or docs/ for discoverability?
2. Future-proofing: The description says "Fail on any error" but currently only affects 404/5xx handling. If other commands adopt this pattern, ensure they honor the flag consistently.

🧪 Test Coverage

The tests are comprehensive:

• ✅ Non-strict mode returns success on 404
• ✅ Strict mode fails on 404
• ✅ Verbose mode shows helpful hints
• ✅ Exit codes are validated
• ✅ Output messages are verified

Minor suggestion: Consider adding a test for the interaction between --strict and 5xx errors to document expected behavior (should 5xx respect strict mode, or always be resilient?).

🎯 Consistency Across Commands

Other commands have similar 5xx handling (run.js, status.js, upload.js). Questions to consider:

1. Should these commands also support --strict for consistency?
2. Is there a centralized error handling utility that could enforce this pattern?
3. Should the 5xx resilient behavior also respect --strict mode?

If the answer is "not yet," that's fine - this PR can be the first step. Just worth considering for future work.

🔒 Security & Performance

• ✅ No security concerns
• ✅ No performance implications (same code paths, just different exit codes)
• ✅ No new dependencies

📝 Documentation

The PR description is excellent. Consider adding:

1. Migration guide: For users who might have workflows relying on the old behavior (fail on 404)
2. Changelog entry: Document this as a behavioral change
3. Example use cases: When to use --strict vs default mode

✨ Suggestions (Optional Enhancements)

1. DRY improvement: Extract the hints array to avoid duplication:

   const MISSING_BUILD_HINTS = [
     '  • No screenshots were uploaded with this parallel-id',
     '  • Tests were skipped or failed before capturing screenshots', 
     '  • The parallel-id does not match what was used during test runs',
   ];

2. JSON output: In --json mode, ensure the skipped result is machine-parseable:

   if (globalOptions.json) {
     output.data({ success: true, skipped: true, reason: 'no-build-found' });
   }

3. Metrics/telemetry: Consider logging when builds are skipped (for debugging production issues)

Summary

Approval: ✅ Approve with minor suggestions

This is a solid PR that solves a real problem with good test coverage. The code is clean, testable, and follows existing patterns. The minor suggestions above are about consistency and DRY principles, but they're not blockers.

Recommended action: Merge after considering:

1. Whether to extract the duplicated hints array
2. Whether to document the flag in README/docs
3. Whether other commands should adopt the same pattern (can be a follow-up PR)

Great work addressing VIZ-108! 🎉

[vizzly-testing]

Vizzly - Visual Test Results

CLI Reporter - 8 changes need review

Status	Count
Passed	11
Changed	8
Auto-approved	11

Changes needing review (8)

viewer-toggle-mode · Firefox · 1920×1080 · 0.0% diff

search-homepage · Firefox · 1920×1080 · 0.1% diff

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

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

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

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

...and 2 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 · 166.0% diff

Review changes

---

_{robert/viz-108 · 45003598}