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:
@@ -235,10 +235,15 @@ class FrameworkAnalyzer(BaseAnalyzer):
|
|||||||
|
|
||||||
# Scripts
|
# Scripts
|
||||||
scripts = pkg.get("scripts", {})
|
scripts = pkg.get("scripts", {})
|
||||||
|
pkg_mgr = self.analysis.get("package_manager", "npm")
|
||||||
if "dev" in scripts:
|
if "dev" in scripts:
|
||||||
self.analysis["dev_command"] = "npm run dev"
|
self.analysis["dev_command"] = f"{pkg_mgr} run dev"
|
||||||
elif "start" in scripts:
|
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:
|
def _detect_go_framework(self, content: str) -> None:
|
||||||
"""Detect Go framework."""
|
"""Detect Go framework."""
|
||||||
|
|||||||
@@ -109,13 +109,15 @@ ELECTRON VALIDATION:
|
|||||||
### Handling Common Issues
|
### Handling Common Issues
|
||||||
|
|
||||||
**App Not Running:**
|
**App Not Running:**
|
||||||
If Electron app is not running or debug port is not accessible:
|
If the 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"
|
1. Check the project commands listed in the PROJECT CAPABILITIES section for a debug/MCP startup script
|
||||||
3. Add to QA report as "Manual verification required"
|
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):**
|
**Headless Environment (CI/CD):**
|
||||||
If running in headless environment without display:
|
If running in headless environment without display:
|
||||||
1. Skip interactive Electron validation
|
1. For UI changes: Document as critical issue — "Visual verification required but unavailable in headless environment"
|
||||||
2. Document: "Electron UI validation skipped - headless environment"
|
2. For non-UI changes: Skip interactive Electron validation and rely on automated tests
|
||||||
3. Rely on unit/integration tests for validation
|
|
||||||
|
|||||||
@@ -97,3 +97,14 @@ When testing UI elements, prefer these selector strategies:
|
|||||||
3. `button:contains("Text")` - By visible text
|
3. `button:contains("Text")` - By visible text
|
||||||
4. `.class-name` - CSS classes
|
4. `.class-name` - CSS classes
|
||||||
5. `input[name="..."]` - Form fields by name
|
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
|
VISUAL VERIFICATION:
|
||||||
1. Navigate to URL
|
- Verification required: YES/NO (reason: [which UI files changed or "no UI files in diff"])
|
||||||
2. Take screenshot
|
- Application started: YES/NO (method: [Electron MCP / Puppeteer / N/A])
|
||||||
3. Check for console errors
|
- Screenshots captured: [count]
|
||||||
4. Verify visual elements
|
- Visual criteria verified:
|
||||||
5. Test interactions
|
- "[criterion 1]": PASS/FAIL
|
||||||
|
- "[criterion 2]": PASS/FAIL
|
||||||
|
- Console errors: [list or "None"]
|
||||||
|
- Issues found: [list or "None"]
|
||||||
```
|
```
|
||||||
|
|
||||||
### 4.2: Console Error Check
|
**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.
|
||||||
|
|
||||||
**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
|
|
||||||
```
|
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
@@ -354,7 +399,7 @@ Create a comprehensive QA report:
|
|||||||
| Unit Tests | ✓/✗ | X/Y passing |
|
| Unit Tests | ✓/✗ | X/Y passing |
|
||||||
| Integration Tests | ✓/✗ | X/Y passing |
|
| Integration Tests | ✓/✗ | X/Y passing |
|
||||||
| E2E 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] |
|
| Project-Specific Validation | ✓/✗ | [summary based on project type] |
|
||||||
| Database Verification | ✓/✗ | [summary] |
|
| Database Verification | ✓/✗ | [summary] |
|
||||||
| Third-Party API Validation | ✓/✗ | [Context7 verification summary] |
|
| Third-Party API Validation | ✓/✗ | [Context7 verification summary] |
|
||||||
@@ -362,6 +407,14 @@ Create a comprehensive QA report:
|
|||||||
| Pattern Compliance | ✓/✗ | [summary] |
|
| Pattern Compliance | ✓/✗ | [summary] |
|
||||||
| Regression Check | ✓/✗ | [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
|
## Issues Found
|
||||||
|
|
||||||
### Critical (Blocks Sign-off)
|
### Critical (Blocks Sign-off)
|
||||||
@@ -505,7 +558,7 @@ All acceptance criteria verified:
|
|||||||
- Unit tests: PASS
|
- Unit tests: PASS
|
||||||
- Integration tests: PASS
|
- Integration tests: PASS
|
||||||
- E2E tests: PASS
|
- E2E tests: PASS
|
||||||
- Browser verification: PASS
|
- Visual verification: PASS
|
||||||
- Project-specific validation: PASS (or N/A)
|
- Project-specific validation: PASS (or N/A)
|
||||||
- Database verification: PASS
|
- Database verification: PASS
|
||||||
- Security review: 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]
|
active_caps = [k for k, v in capabilities.items() if v]
|
||||||
if active_caps:
|
if active_caps:
|
||||||
spec_context += (
|
spec_context += "Based on project analysis, the following verification requirements apply:\n\n"
|
||||||
"Based on project analysis, the following capabilities were detected:\n"
|
spec_context += "| Capability | Detected | Verification Requirement |\n"
|
||||||
)
|
spec_context += "|-----------|----------|-------------------------|\n"
|
||||||
for cap in active_caps:
|
|
||||||
cap_name = (
|
# NOTE: Keys must match those returned by detect_project_capabilities() in project_context.py.
|
||||||
cap.replace("is_", "").replace("has_", "").replace("_", " ").title()
|
# 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"
|
else:
|
||||||
spec_context += "\nRelevant validation tools have been included below.\n\n"
|
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:
|
else:
|
||||||
spec_context += (
|
spec_context += (
|
||||||
"No special project capabilities detected. Using standard validation.\n\n"
|
"No special project capabilities detected. Using standard validation.\n\n"
|
||||||
|
|||||||
Reference in New Issue
Block a user