fix(qa): enforce visual verification for UI changes and inject startup commands (#1784)
* fix(qa): enforce visual verification for UI changes and inject startup commands QA agents were silently skipping visual verification even for UI changes, leading to unverified CSS/layout regressions. This makes visual verification mandatory when UI files are in the diff, injects project startup commands into the QA context so agents can self-start dev servers, and surfaces a structured verification requirements table based on detected capabilities. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(qa): address PR review findings for qa-validation - Handle both dict and list formats for services in QA prompt builder, matching the defensive pattern already used in project_context.py - Use detected package_manager instead of hardcoding 'npm' in dev_command - Rename 'Browser verification' to 'Visual verification' in Phase 10 completion signal to match the renamed Phase 4 section Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Andy
@@ -235,10 +235,15 @@ class FrameworkAnalyzer(BaseAnalyzer):
|
||||
|
||||
# Scripts
|
||||
scripts = pkg.get("scripts", {})
|
||||
pkg_mgr = self.analysis.get("package_manager", "npm")
|
||||
if "dev" in scripts:
|
||||
self.analysis["dev_command"] = "npm run dev"
|
||||
self.analysis["dev_command"] = f"{pkg_mgr} run dev"
|
||||
elif "start" in scripts:
|
||||
self.analysis["dev_command"] = "npm run start"
|
||||
self.analysis["dev_command"] = f"{pkg_mgr} run start"
|
||||
|
||||
# Capture available scripts for downstream consumers (QA agents, init.sh)
|
||||
if scripts:
|
||||
self.analysis["scripts"] = dict(scripts)
|
||||
|
||||
def _detect_go_framework(self, content: str) -> None:
|
||||
"""Detect Go framework."""
|
||||
|
||||
@@ -109,13 +109,15 @@ ELECTRON VALIDATION:
|
||||
### Handling Common Issues
|
||||
|
||||
**App Not Running:**
|
||||
If Electron app is not running or debug port is not accessible:
|
||||
1. Document that Electron validation was skipped
|
||||
2. Note reason: "App not running with --remote-debugging-port=9222"
|
||||
3. Add to QA report as "Manual verification required"
|
||||
If the Electron app is not running or debug port is not accessible:
|
||||
|
||||
1. Check the project commands listed in the PROJECT CAPABILITIES section for a debug/MCP startup script
|
||||
2. Try starting the app with the appropriate command
|
||||
3. If the app still cannot be started:
|
||||
- **For specs with UI changes**: This is a CRITICAL blocking issue. Mark as **REJECTED** — visual verification is mandatory for UI changes and cannot be skipped
|
||||
- **For non-UI changes**: Document as "Electron validation skipped — no UI files changed" and proceed with code-based review
|
||||
|
||||
**Headless Environment (CI/CD):**
|
||||
If running in headless environment without display:
|
||||
1. Skip interactive Electron validation
|
||||
2. Document: "Electron UI validation skipped - headless environment"
|
||||
3. Rely on unit/integration tests for validation
|
||||
1. For UI changes: Document as critical issue — "Visual verification required but unavailable in headless environment"
|
||||
2. For non-UI changes: Skip interactive Electron validation and rely on automated tests
|
||||
|
||||
@@ -97,3 +97,14 @@ When testing UI elements, prefer these selector strategies:
|
||||
3. `button:contains("Text")` - By visible text
|
||||
4. `.class-name` - CSS classes
|
||||
5. `input[name="..."]` - Form fields by name
|
||||
|
||||
### Handling Common Issues
|
||||
|
||||
**Dev Server Not Running:**
|
||||
If the development server is not running or the page cannot be loaded:
|
||||
|
||||
1. Check the project commands listed in the PROJECT CAPABILITIES section for the dev server command
|
||||
2. Start the dev server and wait for it to be ready
|
||||
3. If the server cannot be started:
|
||||
- **For specs with UI changes**: This is a CRITICAL blocking issue. Mark as **REJECTED** — visual verification is mandatory for UI changes
|
||||
- **For non-UI changes**: Document as "Browser validation skipped — no UI files changed" and proceed with code-based review
|
||||
|
||||
@@ -126,41 +126,86 @@ E2E TESTS:
|
||||
|
||||
---
|
||||
|
||||
## PHASE 4: BROWSER VERIFICATION (If Frontend)
|
||||
## PHASE 4: VISUAL / UI VERIFICATION
|
||||
|
||||
For each page/component in the QA Acceptance Criteria:
|
||||
### 4.0: Determine Verification Scope (MANDATORY — DO NOT SKIP)
|
||||
|
||||
### 4.1: Navigate and Screenshot
|
||||
Review the file list from your Phase 0 git diff. Classify each changed file:
|
||||
|
||||
**UI files** (require visual verification):
|
||||
- Component files: .tsx, .jsx, .vue, .svelte, .astro
|
||||
- Style files: .css, .scss, .less, .sass
|
||||
- Files containing Tailwind classes, CSS-in-JS, or inline style changes
|
||||
- Files in directories: components/, pages/, views/, layouts/, styles/, renderer/
|
||||
|
||||
**Non-UI files** (do not require visual verification):
|
||||
- Backend logic: .py, .go, .rs, .java (without template rendering)
|
||||
- Configuration: .json, .yaml, .toml, .env (unless theme/style config)
|
||||
- Tests: *.test.*, *.spec.*
|
||||
- Documentation: .md, .txt
|
||||
|
||||
**Decision**:
|
||||
- If ANY changed file is a UI file → visual verification is REQUIRED below
|
||||
- If the spec describes visual/layout/CSS/styling changes → visual verification is REQUIRED
|
||||
- If NEITHER applies → document "Phase 4: N/A — no visual changes detected in diff" and proceed to Phase 5
|
||||
|
||||
**CRITICAL**: For UI changes, code review alone is NEVER sufficient verification. CSS properties interact with layout context, parent constraints, and specificity in ways that cannot be reliably verified by reading code alone. You MUST see the rendered result.
|
||||
|
||||
### 4.1: Start the Application
|
||||
|
||||
Check the PROJECT CAPABILITIES section above for available startup commands.
|
||||
|
||||
**For Electron apps** (if Electron MCP tools are available):
|
||||
1. Check if app is already running:
|
||||
```
|
||||
Tool: mcp__electron__get_electron_window_info
|
||||
```
|
||||
2. If not running, look for a debug/MCP script in the startup commands above and run it:
|
||||
```bash
|
||||
cd [frontend-path] && npm run dev:debug
|
||||
```
|
||||
Wait 15 seconds, then retry `get_electron_window_info`.
|
||||
|
||||
**For web frontends** (if Puppeteer tools are available):
|
||||
1. Start dev server using the dev_command from the startup commands above
|
||||
2. Wait for the server to be listening on the expected port
|
||||
3. Navigate with Puppeteer:
|
||||
```
|
||||
Tool: mcp__puppeteer__puppeteer_navigate
|
||||
Args: {"url": "http://localhost:[port]"}
|
||||
```
|
||||
|
||||
### 4.2: Capture and Verify Screenshots
|
||||
|
||||
For EACH visual success criterion in the spec:
|
||||
1. Navigate to the affected screen/component
|
||||
2. Set up test conditions (e.g., create long text to test overflow)
|
||||
3. Take a screenshot:
|
||||
- Electron: `mcp__electron__take_screenshot`
|
||||
- Web: `mcp__puppeteer__puppeteer_screenshot`
|
||||
4. Examine the screenshot and verify the criterion is met
|
||||
5. Document: "[Criterion]: VERIFIED via screenshot" or "FAILED: [what you observed]"
|
||||
|
||||
### 4.3: Check Console for Errors
|
||||
|
||||
- Electron: `mcp__electron__read_electron_logs` with `{"logType": "console", "lines": 50}`
|
||||
- Web: `mcp__puppeteer__puppeteer_evaluate` with `{"script": "window.__consoleErrors || []"}`
|
||||
|
||||
### 4.4: Document Findings
|
||||
|
||||
```
|
||||
# Use browser automation tools
|
||||
1. Navigate to URL
|
||||
2. Take screenshot
|
||||
3. Check for console errors
|
||||
4. Verify visual elements
|
||||
5. Test interactions
|
||||
VISUAL VERIFICATION:
|
||||
- Verification required: YES/NO (reason: [which UI files changed or "no UI files in diff"])
|
||||
- Application started: YES/NO (method: [Electron MCP / Puppeteer / N/A])
|
||||
- Screenshots captured: [count]
|
||||
- Visual criteria verified:
|
||||
- "[criterion 1]": PASS/FAIL
|
||||
- "[criterion 2]": PASS/FAIL
|
||||
- Console errors: [list or "None"]
|
||||
- Issues found: [list or "None"]
|
||||
```
|
||||
|
||||
### 4.2: Console Error Check
|
||||
|
||||
**CRITICAL**: Check for JavaScript errors in the browser console.
|
||||
|
||||
```
|
||||
# Check browser console for:
|
||||
- Errors (red)
|
||||
- Warnings (yellow)
|
||||
- Failed network requests
|
||||
```
|
||||
|
||||
### 4.3: Document Findings
|
||||
|
||||
```
|
||||
BROWSER VERIFICATION:
|
||||
- [Page/Component]: PASS/FAIL
|
||||
- Console errors: [list or "None"]
|
||||
- Visual check: PASS/FAIL
|
||||
- Interactions: PASS/FAIL
|
||||
```
|
||||
**If you cannot start the application for visual verification of UI changes**: This is a BLOCKING issue. Do NOT silently skip — document it as a critical issue and REJECT, requesting startup instructions be fixed.
|
||||
|
||||
---
|
||||
|
||||
@@ -354,7 +399,7 @@ Create a comprehensive QA report:
|
||||
| Unit Tests | ✓/✗ | X/Y passing |
|
||||
| Integration Tests | ✓/✗ | X/Y passing |
|
||||
| E2E Tests | ✓/✗ | X/Y passing |
|
||||
| Browser Verification | ✓/✗ | [summary] |
|
||||
| Visual Verification | ✓/✗/N/A | [Screenshot count] or "No UI changes" |
|
||||
| Project-Specific Validation | ✓/✗ | [summary based on project type] |
|
||||
| Database Verification | ✓/✗ | [summary] |
|
||||
| Third-Party API Validation | ✓/✗ | [Context7 verification summary] |
|
||||
@@ -362,6 +407,14 @@ Create a comprehensive QA report:
|
||||
| Pattern Compliance | ✓/✗ | [summary] |
|
||||
| Regression Check | ✓/✗ | [summary] |
|
||||
|
||||
## Visual Verification Evidence
|
||||
|
||||
If UI files were changed:
|
||||
- Screenshots taken: [count and description of each]
|
||||
- Console log check: [error count or "Clean"]
|
||||
|
||||
If skipped: [Explicit justification — must reference git diff showing no UI files changed]
|
||||
|
||||
## Issues Found
|
||||
|
||||
### Critical (Blocks Sign-off)
|
||||
@@ -505,7 +558,7 @@ All acceptance criteria verified:
|
||||
- Unit tests: PASS
|
||||
- Integration tests: PASS
|
||||
- E2E tests: PASS
|
||||
- Browser verification: PASS
|
||||
- Visual verification: PASS
|
||||
- Project-specific validation: PASS (or N/A)
|
||||
- Database verification: PASS
|
||||
- Security review: PASS
|
||||
|
||||
@@ -529,18 +529,82 @@ This shows only changes made in the spec branch since it diverged from `{base_br
|
||||
|
||||
"""
|
||||
|
||||
# Add capability summary for transparency
|
||||
# Add capability summary as verification requirements table
|
||||
active_caps = [k for k, v in capabilities.items() if v]
|
||||
if active_caps:
|
||||
spec_context += (
|
||||
"Based on project analysis, the following capabilities were detected:\n"
|
||||
)
|
||||
for cap in active_caps:
|
||||
cap_name = (
|
||||
cap.replace("is_", "").replace("has_", "").replace("_", " ").title()
|
||||
spec_context += "Based on project analysis, the following verification requirements apply:\n\n"
|
||||
spec_context += "| Capability | Detected | Verification Requirement |\n"
|
||||
spec_context += "|-----------|----------|-------------------------|\n"
|
||||
|
||||
# NOTE: Keys must match those returned by detect_project_capabilities() in project_context.py.
|
||||
# If new capabilities are added there, update this dict to avoid silent omission.
|
||||
cap_requirements = {
|
||||
"is_electron": (
|
||||
"Electron Desktop App",
|
||||
"UI changes REQUIRE Electron MCP visual verification (screenshots)",
|
||||
),
|
||||
"is_web_frontend": (
|
||||
"Web Frontend",
|
||||
"UI changes REQUIRE browser-based visual verification (screenshots)",
|
||||
),
|
||||
"is_tauri": ("Tauri Desktop App", "UI changes REQUIRE visual verification"),
|
||||
"is_expo": (
|
||||
"Expo Mobile App",
|
||||
"UI changes require device/simulator verification",
|
||||
),
|
||||
"is_react_native": (
|
||||
"React Native App",
|
||||
"UI changes require device/simulator verification",
|
||||
),
|
||||
"is_nextjs": ("Next.js App", "Page changes require browser verification"),
|
||||
"is_nuxt": ("Nuxt App", "Page changes require browser verification"),
|
||||
"has_api": ("API Endpoints", "Endpoint changes require API testing"),
|
||||
"has_database": (
|
||||
"Database",
|
||||
"Schema changes require migration verification",
|
||||
),
|
||||
}
|
||||
|
||||
for cap_key in active_caps:
|
||||
if cap_key in cap_requirements:
|
||||
name, req = cap_requirements[cap_key]
|
||||
spec_context += f"| {name} | YES | {req} |\n"
|
||||
|
||||
spec_context += "\n"
|
||||
|
||||
# Inject startup commands from project_index services
|
||||
# Handle both dict format (services by name) and list format
|
||||
services = project_index.get("services", {})
|
||||
if isinstance(services, dict):
|
||||
services_iter = services.items()
|
||||
elif isinstance(services, list):
|
||||
services_iter = (
|
||||
(svc.get("name", f"service-{i}"), svc)
|
||||
for i, svc in enumerate(services)
|
||||
if isinstance(svc, dict)
|
||||
)
|
||||
spec_context += f"- {cap_name}\n"
|
||||
spec_context += "\nRelevant validation tools have been included below.\n\n"
|
||||
else:
|
||||
services_iter = iter([])
|
||||
for svc_name, svc in services_iter:
|
||||
svc_scripts = svc.get("scripts", {})
|
||||
dev_cmd = svc.get("dev_command", "")
|
||||
if svc_scripts or dev_cmd:
|
||||
spec_context += f"**{svc_name} service commands:**\n"
|
||||
if dev_cmd:
|
||||
spec_context += f"- Dev server: `{dev_cmd}`\n"
|
||||
# Surface debug/MCP scripts specifically
|
||||
debug_scripts = {
|
||||
k: v
|
||||
for k, v in svc_scripts.items()
|
||||
if any(term in k for term in ("debug", "mcp", "test", "e2e"))
|
||||
}
|
||||
if debug_scripts:
|
||||
pkg_mgr = svc.get("package_manager", "npm")
|
||||
for script_name, script_cmd in debug_scripts.items():
|
||||
spec_context += f"- {script_name}: `{pkg_mgr} run {script_name}` ({script_cmd})\n"
|
||||
spec_context += "\n"
|
||||
|
||||
spec_context += "Match changed files from the git diff against these capabilities to determine which verification phases are MANDATORY.\n\n"
|
||||
else:
|
||||
spec_context += (
|
||||
"No special project capabilities detected. Using standard validation.\n\n"
|
||||
|
||||
Reference in New Issue
Block a user