diff --git a/README.md b/README.md index cf5836e..1effb90 100644 --- a/README.md +++ b/README.md @@ -1,24 +1,25 @@ # Interview Coach -AI-powered interview coaching platform. Users select an interviewer, listen to a spoken question, record their answer, and receive structured feedback on delivery, tone, and answer quality. +AI-powered interview coaching platform. Users select an interviewer, choose feedback timing, listen to a spoken question, record their answer, and receive structured feedback on delivery, tone, and answer quality. ## How it works -1. User selects an interviewer voice and presses **Start Interview** +1. User selects an interviewer voice and feedback timing, then presses **Start Interview** 2. Interviewer introduces themselves via TTS (`POST /speech/tts`) 3. Interviewer reads the question aloud; recording starts automatically 4. User answers and presses **I'm Done Answering** 5. Audio → Groq Whisper → timestamped transcript segments (`POST /speech/transcribe`) 6. Each segment → local wav2vec2 model → arousal / dominance / valence scores 7. Scores + transcript displayed per segment on the interview screen -8. *(Coming)* Transcript + scores + question → LLM → structured feedback (`POST /feedback/generate`) -9. *(Coming)* Frontend renders full scorecard +8. Transcript + scores + question → Groq LLM → structured feedback (`POST /feedback/generate`) +9. Scorecard renders per-question feedback (strengths, improvements, delivery notes, model answer, transcript scores) or full-session summary at end of interview (`POST /feedback/generate-session`) ## Prerequisites Install these once at the OS level before running setup: - **Python 3.9+** +- **Node.js 18+** — download from https://nodejs.org (LTS release) - **pnpm** — `npm install -g pnpm` - **ffmpeg** — required to decode audio files: @@ -54,8 +55,8 @@ cp backend/.env.example backend/.env Open `backend/.env` and fill in your keys: ``` -GROQ_API_KEY=your_key_here # https://console.groq.com (speech-to-text + TTS) -ANTHROPIC_API_KEY=your_key_here # https://console.anthropic.com (LLM feedback, coming) +GROQ_API_KEY=your_key_here # https://console.groq.com (TTS + speech-to-text + LLM feedback) +ANTHROPIC_API_KEY=your_key_here # https://console.anthropic.com (reserved) OPENAI_API_KEY=your_key_here # Reserved ``` @@ -116,16 +117,26 @@ backend/ requirements.txt — Python dependencies services/ tone_delivery_analyzer/ — local wav2vec2 emotion model (arousal/dominance/valence) - speech_to_text/ — Groq Whisper transcription + TTS - llm/ — LLM feedback generation (coming) - text_analysis/ — transcript scoring (coming) + speech_to_text/ — Groq Whisper transcription + Orpheus TTS + llm/ — Groq LLM feedback generation (/feedback/generate, /feedback/generate-session) + text_analysis/ — transcript scoring (reserved) frontend/ app/ page.tsx — interview UI (home page) - InterviewClient.tsx — full interview state machine + InterviewClient.tsx — full interview state machine (recording, VAD, TTS, feedback) layout.tsx / globals.css — root layout and styles - scorecard/ — scorecard display components + components/ + MicWaveform/ — live mic waveform visualizer + scorecard/ + ScorecardPanel.tsx — per-question scorecard (calls /feedback/generate, renders result) + SessionScorecardPanel.tsx — full-session scorecard (calls /feedback/generate-session) + components/ + Scorecard/ — top-level scorecard layout + DeliveryScores/ — arousal/dominance/valence score display + TranscriptFeedbackScores/ — clarity/structure/relevance/conciseness display + QualitativeFeedback/ — strengths, improvements, delivery notes + ModelAnswer/ — example answer display dev/ flow/ — pipeline visualization dev page transcribe/ — transcription dev/debug page @@ -137,6 +148,11 @@ frontend/ types.ts — shared TypeScript types mocks.ts — mock data for UI development pipelineStages.ts — pipeline stage definitions + sessionAdapter.ts — builds ReviewContextPayload from question + segments + feedbackSpeech.ts — converts feedback responses to TTS scripts + aggregateReviewPayload.ts — aggregates per-question answers into session payload + speech/ + tts.ts — frontend helper for POST /speech/tts docs/ architecture.md — system architecture and data flow @@ -155,11 +171,25 @@ docs/ | Layer | Technology | | ---------------- | -------------------------------------------------------------- | | Frontend | Next.js 16, React 19, TypeScript, Tailwind CSS 4 | +| Component docs | Storybook 10 | | Backend | FastAPI (Python) | | Text-to-speech | Groq Orpheus (`canopylabs/orpheus-v1-english`) | | Speech-to-text | Groq Whisper (`whisper-large-v3-turbo`) | | Tone/delivery | `audeering/wav2vec2-large-robust-12-ft-emotion-msp-dim` (local)| -| LLM feedback | Anthropic Claude (coming) | +| LLM feedback | Groq (`openai/gpt-oss-20b`) | | Package manager | pnpm | -| Frontend deploy | Vercel (planned) | -| Backend deploy | Render or Fly.io (planned) | +| Frontend deploy | TBD | +| Backend deploy | TBD | + +## Backend endpoints + +| Method | Path | Status | Purpose | +| ------ | --------------------------- | -------- | -------------------------------------------------------------- | +| GET | `/health` | done | Liveness check | +| GET | `/emotion/health` | done | Confirms emotion model is loaded | +| POST | `/speech/tts` | done | Text → WAV audio (Groq Orpheus TTS) | +| POST | `/speech/transcribe` | done | Audio → per-segment transcript + emotion scores | +| POST | `/emotion/analyze` | done | Audio → single arousal / dominance / valence score | +| POST | `/feedback/generate` | done | Transcript + scores + question → per-answer LLM feedback | +| POST | `/feedback/generate-session`| done | Multiple Q&A pairs → holistic session feedback + per-question reviews | +| POST | `/analysis/*` | reserved | Transcript text analysis (reserved) | diff --git a/backend/.env.example b/backend/.env.example index 305d08f..7af466a 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -1,6 +1,6 @@ # Copy this file to .env and fill in your keys before running the backend. # .env is gitignored — never commit it. -# All three keys are required for the full pipeline to function. +# GROQ_API_KEY is required. ANTHROPIC_API_KEY and OPENAI_API_KEY are reserved and not needed for the full pipeline. # Required for speech-to-text (Groq Whisper) AND LLM feedback (Groq gpt-oss-20b). # https://console.groq.com diff --git a/backend/README.md b/backend/README.md index 0572302..81c19c7 100644 --- a/backend/README.md +++ b/backend/README.md @@ -11,7 +11,8 @@ For setup and running instructions see the [root README](../README.md). | POST | `/speech/tts` | done | Text → WAV audio via Groq Orpheus TTS | | POST | `/speech/transcribe` | done | Audio file → per-segment transcript + emotion scores | | POST | `/emotion/analyze` | done | Audio file → single arousal / dominance / valence score | -| POST | `/feedback/generate` | todo | Transcript + scores + question → structured LLM feedback | +| POST | `/feedback/generate` | done | Transcript + scores + question → per-answer LLM feedback (Groq) | +| POST | `/feedback/generate-session` | done | Multiple Q&A pairs → holistic session feedback + per-question reviews | ## Structure @@ -26,9 +27,10 @@ services/ emotion_model.py — model class definitions (do not modify) run_emotion.py — standalone CLI for testing the model directly llm/ - router.py — POST /feedback/generate (not yet implemented) + router.py — POST /feedback/generate and POST /feedback/generate-session (Groq LLM feedback) + schemas.py — Pydantic request/response models for feedback endpoints text_analysis/ - router.py — reserved for transcript scoring (not yet implemented) + router.py — reserved — no endpoints yet ``` ## POST /speech/tts @@ -93,6 +95,75 @@ curl -X POST http://localhost:8000/emotion/analyze \ -F "file=@path/to/audio.mp3" ``` +## POST /feedback/generate + +Generates per-answer feedback using Groq (`openai/gpt-oss-20b`). Accepts a question and full transcript with per-segment delivery scores; returns transcript quality scores, qualitative feedback, and a model answer. + +**Request body (JSON):** +```json +{ + "question": { "id": "1", "text": "Tell me about yourself." }, + "transcript": { + "text": "I have three years of experience...", + "segments": [ + { "start": 0.0, "end": 2.4, "text": "I have three years...", "arousal": 0.61, "dominance": 0.55, "valence": 0.32 } + ] + } +} +``` + +**Response:** +```json +{ + "transcriptScores": { "clarity": 0.8, "structure": 0.7, "relevance": 0.9, "conciseness": 0.75 }, + "feedback": { + "summary": "You answered confidently and stayed on topic.", + "strengths": ["Strong opening", "Specific examples"], + "improvements": ["Tighten the closing", "Vary your pace"], + "deliveryNotes": "Your arousal stayed high throughout — good energy." + }, + "modelAnswer": { "text": "I have three years of experience in..." } +} +``` + +Requires `GROQ_API_KEY` in `backend/.env`. + +## POST /feedback/generate-session + +Generates holistic session feedback across all answers in a single Groq call. Accepts an array of question + transcript pairs; returns session-level summary and per-question reviews. + +**Request body (JSON):** +```json +{ + "answers": [ + { + "question": { "id": "1", "text": "Tell me about yourself." }, + "transcript": { "text": "...", "segments": [ ... ] } + } + ] +} +``` + +**Response:** +```json +{ + "overallSummary": "You showed consistent confidence across questions.", + "overallStrengths": ["Clear communication", "Strong examples"], + "overallImprovements": ["Improve conciseness", "Vary sentence structure"], + "overallDeliveryNotes": "Arousal trended high — sustained energy throughout.", + "questionReviews": [ + { + "question": { "id": "1", "text": "Tell me about yourself." }, + "transcriptScores": { "clarity": 0.8, "structure": 0.7, "relevance": 0.9, "conciseness": 0.75 }, + "feedback": { "summary": "...", "strengths": [], "improvements": [], "deliveryNotes": "..." }, + "modelAnswer": { "text": "..." } + } + ] +} +``` + +Requires `GROQ_API_KEY` in `backend/.env`. + ## Score interpretation | Dimension | Low (→ 0) | High (→ 1) | diff --git a/docs/agent-workflows/branch-change-impact-audit.md b/docs/agent-workflows/branch-change-impact-audit.md index e13a471..660ec29 100644 --- a/docs/agent-workflows/branch-change-impact-audit.md +++ b/docs/agent-workflows/branch-change-impact-audit.md @@ -37,16 +37,18 @@ Check for: - mixed old/new flows that prevent a clean fallback - changes that partially switch a feature without fully gating it -### 3. Feature-flag involvement +### 3. Env-var or behavioral mode changes -If the diff touches anything related to feature flags or env-gated behavior (`featureFlag`, `featureGate`, `NEXT_PUBLIC_*` toggles, `flags`, or similar): +If the diff introduces or modifies `NEXT_PUBLIC_*` env vars used for behavioral gating (not just API base URL configuration), or adds new behavioral mode branches with distinct execution paths: - Note which files and call sites are involved -- Ask the user: "Feature flag changes detected. Would you like me to run the feature flag gating review before continuing the audit?" +- Ask the user: "Behavioral gating changes detected. Would you like me to run the feature flag gating review before continuing the audit?" - If the user says yes, read and run `docs/agent-workflows/feature-flag-gating-review.md` and include its output as a subsection before continuing - If the user says no, note it and continue the audit without the flag review -If no flag involvement is detected, state that explicitly and continue. +This project has no formal feature flag registry. Simple UI state variables like `feedbackMode` do not require this review unless their branching logic is complex. + +If no env-var or behavioral mode changes are detected, state that explicitly and continue. ### 4. Integration consistency diff --git a/docs/agent-workflows/code-quality-review.md b/docs/agent-workflows/code-quality-review.md index 7512302..d3f818b 100644 --- a/docs/agent-workflows/code-quality-review.md +++ b/docs/agent-workflows/code-quality-review.md @@ -50,12 +50,11 @@ Check for: - invalid assumptions - incorrect prop or state flow - race conditions -- unhandled promise rejections - missing error paths - `process.env.*` values referenced inside a type-guarded branch without first being extracted to a const — TypeScript does not narrow `process.env` re-reads reliably; the guard may pass but the spread or argument receives `undefined` - stub or placeholder methods whose parameter types are inconsistent with the method's documented purpose — a method with no callers yet can still have a wrong signature; verify parameters match what the implementation will actually need -- `await` expressions in API route handlers that sit outside any try/catch block — if the awaited call throws, the route bypasses the standard `{ success: false }` JSON error shape and returns a raw Next.js 500; every async operation after the auth check must be covered by a catch block -- stub or placeholder implementations in production code paths that throw unconditionally — a stub reachable by live production requests must return a safe fallback (e.g. 403, 501, or null) rather than throw; throwing is only acceptable in dead code or methods with no real callers on any active request path +- Unhandled promise rejections in client-side `fetch` calls to the FastAPI backend — every call should be wrapped in try/catch; unhandled rejections cause silent failures in the recording, transcription, or feedback pipeline +- stub or placeholder implementations in production code paths that throw unconditionally — a FastAPI router stub reachable by live requests must return a safe fallback (e.g. 501) rather than raise; raising is only acceptable in dead code or stubs with no active request path - resource allocations that require explicit cleanup — for any resource allocated with an explicit close, cancel, release, or destroy method (e.g. `ImageBitmap.close()`, `ReadableStreamDefaultReader.cancel()`, `AbortController` timers, canvas contexts, sockets, file handles), enumerate every exit path from the enclosing block (success return, throw, early return, implicit fall-through) and verify cleanup fires on each; a cleanup call that exists on the happy path only is a resource leak Decision rule: @@ -102,7 +101,7 @@ Check for: - unnecessary complexity - duplication that creates real maintenance risk - parallel literal lists in the same module (for example a string-union type maintained alongside a discriminated union with the same variants, or two arrays of the same enum values that must stay in sync) -- duplicate TypeScript types that mirror Zod schemas (or API DTOs) in separate files without `z.infer` or a single source of truth — when both exist, derived types should come from the schema so drift is impossible at compile time +- duplicate TypeScript types that mirror backend Pydantic response shapes in separate files — types should be defined once in `lib/interview-coach/types.ts` and imported where needed, not redefined per-component - inconsistent approaches to the same pattern - JS-managed UI state that native HTML or CSS already handles cleanly @@ -122,7 +121,6 @@ Check changed code against project conventions in `AGENTS.md`, specifically: - Deep relative imports where the `@/` alias works - Missing `'use client'` on components that use MediaRecorder, browser APIs, or client-side `fetch` to the backend - Raw `` or `` for internal navigation instead of `next/image` / `next/link` -- Zod (or equivalent) validation missing from new Next.js Route Handlers that accept body/query input - Exported async functions missing an explicit return type on public API/client surfaces Decision rule: @@ -182,39 +180,12 @@ Run this section when the diff touches `backend/` Python services/routers **or** 2. **No duplicated ML.** Frontend does not call OpenAI/Anthropic directly for pipeline steps owned by the backend. 3. **Upload flow.** Recording/upload uses `FormData` (or equivalent) matching backend expectations; loading and error states are handled in UI. 4. **Typed responses.** Scorecard and pipeline types align with backend response shapes (arousal/dominance/valence, transcript fields, feedback sections). -5. **Presentational/container split.** Data-fetching pages do not embed large presentational JSX without a story-friendly presentational child. +5. **Component decomposition.** Client components with meaningful visual states expose props suitable for Storybook stories; do not embed non-trivial UI logic in a single monolithic client component when splitting would enable isolated state testing via stories. Decision rule: - Flag failures that would break the record → analyze → scorecard flow or leak secrets. -### 10. New API route security - -Run this section whenever the diff contains a new file under `app/api/` or `pages/api/`. - -For each new API route file, perform these checks: - -1. **Authentication gate (if applicable).** Interview Coach may not have auth yet. If the route is documented or intended to be public (health proxies, dev-only), say so. If the route should be protected, confirm an auth check is the first async operation before data access or side effects. - -2. **SSRF prevention (outbound routes only).** If the route makes any outbound HTTP request (via `fetch`, `http.request`, `https.request`, or any HTTP client), confirm: - - A blocklist or allowlist is applied before the request is made and after any DNS resolution - - For IP blocklists: cross-reference against the complete IANA IPv4 Special-Purpose Address Registry. The minimum complete set is: `0.0.0.0/8` ("this" network), `10.0.0.0/8` (RFC1918), `100.64.0.0/10` (CGNAT), `127.0.0.0/8` (loopback), `169.254.0.0/16` (link-local), `172.16.0.0/12` (RFC1918), `192.0.0.0/24` (IANA special), `192.0.2.0/24` (TEST-NET-1), `192.88.99.0/24` (6to4 relay), `192.168.0.0/16` (RFC1918), `198.18.0.0/15` (benchmarking), `198.51.100.0/24` (TEST-NET-2), `203.0.113.0/24` (TEST-NET-3), `224.0.0.0/4` (multicast), `240.0.0.0/4` (reserved). A blocklist covering only RFC1918 + loopback is incomplete. - - DNS resolution happens before the request is made so attacker-controlled DNS cannot redirect to a private address after the blocklist check passes - -3. **Cache-Control semantics.** If the route sets a `Cache-Control` response header, verify the directive matches the route's sensitivity (user-specific coaching data must use `private` or `no-store`, not `public`). - -4. **Input validation.** Confirm all query params, path params, and request body fields are validated (Zod or equivalent) before use. Unvalidated string params passed to database queries, file paths, or outbound URLs are injection vectors. - -5. **Response size and content-type limits.** If the route proxies or streams external content: - - A maximum byte limit must be enforced during streaming (not just on `Content-Length`, which can be omitted or spoofed) - - The upstream `Content-Type` must be validated against an allowlist before being forwarded to the client - -6. **Error shape consistency.** Confirm all error paths return the project-standard `{ success: false, error: "..." }` JSON shape. Any `await` outside a try/catch bypasses this shape and returns a raw Next.js 500 — covered by Priority 1, but re-verify here for new routes since the full surface is being established for the first time. - -Decision rule: - -- Each check above is a security or correctness requirement. Flag every failure. - ## Constraints - Flag the structural concern and its impact, but do not propose redesigns @@ -226,7 +197,7 @@ Decision rule: Changed files are those returned by `git diff --name-only origin/main...HEAD` (or the confirmed target). Run from `frontend/` when the diff includes frontend files; from `backend/` for Python-only changes. Report results for each command that exists: -- `pnpm test` / `pnpm test -- --findRelatedTests ` — only if a `test` script exists in `frontend/package.json`; otherwise state "not configured" +- `pnpm test` / `pnpm test -- --findRelatedTests ` — not configured in this project; state "not configured — skipped" and proceed - `pnpm typecheck` (frontend) - `pnpm lint` (frontend) - `pnpm build` (frontend) diff --git a/docs/agent-workflows/feature-flag-gating-review.md b/docs/agent-workflows/feature-flag-gating-review.md index 337bf93..b7d35da 100644 --- a/docs/agent-workflows/feature-flag-gating-review.md +++ b/docs/agent-workflows/feature-flag-gating-review.md @@ -8,6 +8,19 @@ Verifies that a feature flag **fully controls the intended behavior** and suppor --- +## Project context (Interview Coach) + +This project has no formal feature flag system or flag registry. Apply this workflow when: + +- A new `NEXT_PUBLIC_*` env var is introduced that gates behavior (not just configures an API URL like `NEXT_PUBLIC_API_URL`) +- A new behavioral mode is added that must cleanly support rollback (e.g., a new `feedbackMode` variant or a new interview flow toggle) + +For simple API configuration env vars and pure UI state variables like `feedbackMode` (perQuestion/endOfInterview), this review is optional unless the behavior branch is complex enough to warrant verification. When in doubt, use `code-quality-review.md` instead. + +When running this review: treat the env var or toggle as the "flag." The flag registry check does not apply — there is no registry in this project. + +--- + ## Goal Verify that a feature flag cleanly separates the enabled and disabled behaviors without mixed-mode execution, bypassed call sites, or hidden dependencies. diff --git a/docs/agent-workflows/feature-implementation-planning.md b/docs/agent-workflows/feature-implementation-planning.md index 2a2ae59..5751723 100644 --- a/docs/agent-workflows/feature-implementation-planning.md +++ b/docs/agent-workflows/feature-implementation-planning.md @@ -23,22 +23,23 @@ Create a clear implementation plan before code is written so the scope, file imp Include: 1. Context — why the change is being made -2. Backend API changes — new or changed FastAPI routes, request/response shapes, env vars in `backend/.env` +2. Backend API changes — new or changed FastAPI routes, Pydantic request/response schemas, env vars in `backend/.env`; note which external service is called (Groq Whisper, Groq Orpheus, Groq LLM, or the local wav2vec2 model) 3. Complete file manifest — every new or changed file, grouped by category 4. Implementation order — phased sequence of work -5. Integration points — where the new code connects to existing systems -6. Verification — how the change will be validated end to end +5. Integration points — where the new code connects to existing systems (frontend fetch calls, FastAPI routers, backend services) +6. Verification — how the change will be validated end to end (manual QA with both feedback modes; confirm both /feedback/generate and /feedback/generate-session paths if feedback is involved) ## File manifest guidance Group files by category such as: -- backend routers and services (`backend/services/`) -- types files (frontend `types.ts`, backend Pydantic models if used) -- frontend API client modules (`lib/api.ts` or feature `lib/`) -- presentational components + containers + `*.module.css` + `*.stories.tsx` -- App Router pages (`app/`) -- tests (when test runner exists) +- backend routers and Pydantic schemas (`backend/services//router.py`, `schemas.py`) +- TypeScript types (`frontend/lib/interview-coach/types.ts`) +- frontend lib helpers (`frontend/lib/interview-coach/`, `frontend/lib/speech/`) +- frontend API client module (`frontend/lib/api.ts` if centralizing fetch calls) +- presentational components + `*.module.css` + `*.stories.tsx` +- App Router pages (`frontend/app/`) +- Storybook stories (no test runner configured; stories are the coverage mechanism) - docs (`backend/README.md`, `AGENTS.md` if conventions change) ## Usage diff --git a/docs/agent-workflows/figma-design-to-code.md b/docs/agent-workflows/figma-design-to-code.md index 145f5ad..131060a 100644 --- a/docs/agent-workflows/figma-design-to-code.md +++ b/docs/agent-workflows/figma-design-to-code.md @@ -62,7 +62,7 @@ The MCP reference output is generic React + Tailwind. Before implementing: - [ ] Recording/scorecard UI uses presentational components with mock-friendly props for Storybook - [ ] No backend secrets in client code; API calls go through the FastAPI client module -There is no shared `app/components/ui/` library yet — reuse existing components in the branch or create new ones with `.module.css` + `.stories.tsx` per `AGENTS.md`. +There is no shared `app/components/ui/` library yet — reuse existing components in the branch or create new ones with `.module.css` + `.stories.tsx` per `AGENTS.md`. Note: the `storybook` script is not yet in `frontend/package.json`; create stories alongside components regardless — they are required by convention even before the script is wired up. ### Step 3 — Map design tokens diff --git a/docs/agent-workflows/manual-qa-checklist-generator.md b/docs/agent-workflows/manual-qa-checklist-generator.md index 5911b57..4ab1aef 100644 --- a/docs/agent-workflows/manual-qa-checklist-generator.md +++ b/docs/agent-workflows/manual-qa-checklist-generator.md @@ -40,7 +40,14 @@ Include only the failure or edge cases that are realistic and relevant to the ch ### 4. Mode-dependent checks -If the change depends on feature flags, environments, or auth modes, include separate checks for each meaningful mode. +If the change depends on behavioral modes or environments, include separate checks for each meaningful mode. + +For Interview Coach, always include separate checks for both feedback modes when the recording or feedback pipeline is affected: + +- **perQuestion mode** — scorecard appears after each answer via `POST /feedback/generate`; interviewer reads feedback aloud via TTS; user can advance to next question +- **endOfInterview mode** — answers are collected silently across all questions; session scorecard appears after "End Interview" via `POST /feedback/generate-session`; interviewer reads session summary aloud + +Also include checks for the full scorecard rendering path: DeliveryScores (arousal/dominance/valence), TranscriptFeedbackScores (clarity/structure/relevance/conciseness), QualitativeFeedback (summary/strengths/improvements/deliveryNotes), and ModelAnswer. ## Constraints diff --git a/docs/agent-workflows/pre-merge-full-review.md b/docs/agent-workflows/pre-merge-full-review.md index 1ac4bb8..70ecb6d 100644 --- a/docs/agent-workflows/pre-merge-full-review.md +++ b/docs/agent-workflows/pre-merge-full-review.md @@ -339,15 +339,14 @@ Each phase's sub-workflow doc may add more, but at minimum every phase runs thes **Phase 4 — Test suite quality:** -- Run tests when configured: `pnpm test -- ` from `frontend/` if `package.json` defines a `test` script; otherwise record "tests not configured — skipped". Failure → finding per failing test. -- Run the full test suite if any source file (not test file) changed in a way that could affect untouched tests. Failure → finding. -- Coverage check on new components/services if the project has a coverage threshold: report new code with no test → finding. -- Pattern check: new `.skip`, `.only`, `xit`, `xdescribe` in changed test files → finding. +- `pnpm test` is not configured in this project — record "tests not configured — skipped" in the Coverage report. +- Storybook story coverage check: for every new component or component with new visual states in the changed file list, confirm a `ComponentName.stories.tsx` file exists with a named story for each state (idle, recording, processing, error, etc.) — missing story → finding. +- Pattern check: new `.skip`, `.only`, `xit`, `xdescribe` in changed test or story files → finding. **Phase 5 — Feature flag gating:** -- Pattern check: every new code path gated by a flag must have the flag-off behavior verifiable in code (grep for the flag identifier, confirm an else branch or early return exists) → finding if not. -- Pattern check: new flag identifiers must appear in the flag registry/config → finding if missing. +- Pattern check: every new `NEXT_PUBLIC_*` env var used for behavioral gating (not just API base URL configuration) must have a defined fallback or off-path verifiable in code → finding if not. +- Pattern check: new behavioral mode branches (e.g. `feedbackMode` variants) must have both the on-path and off-path verifiable in code with no partial execution → finding if not. - Specific patterns the sub-workflow doc lists. **Phase 6 — PR description:** @@ -408,8 +407,8 @@ Whenever the user has approved a fix and you are about to edit a file: 5. Make only the approved edit. Do not bundle in adjacent improvements. 6. **Review the new code before reporting it done.** After making the edit, re-read every line you just wrote as if you were reviewing a colleague's code — not code you wrote: - Would you flag any of these lines in a Phase 2 review? If yes, surface it as a new finding. - - Does the fix enforce its intended invariant at every layer the call path touches — not just the layer you edited? (If you changed a DB query, do all callers handle the new return value correctly? If you added a guard, do all branches after the guard also behave correctly?) - - If `pnpm test` exists, run `pnpm test -- --findRelatedTests ` for frontend edits. If any test fails, treat it as a new finding and fix it now. If no test script exists, run `pnpm typecheck` and `pnpm lint` on touched frontend files instead. + - Does the fix enforce its intended invariant at every layer the call path touches — not just the layer you edited? (If you changed a FastAPI response shape, do all frontend callers that parse that response handle the new shape correctly? If you added a guard, do all branches after the guard also behave correctly?) + - `pnpm test` is not configured in this project — run `pnpm typecheck` and `pnpm lint` on touched frontend files after any edit. - If you discover a problem while reviewing your own fix, add it to the Open findings list. Do not silently edit it under the existing approval. 7. After the edit, report what you changed using the post-edit report shape below. 8. Re-read the file before any further edit to it. @@ -590,23 +589,23 @@ Otherwise: Run if any of the following are true: -- Any `.test.ts` or `.test.tsx` files are in the changed file list -- New components or services were introduced (check whether tests were added or are missing) +- Any `.stories.tsx` files are in the changed file list +- New components or services were introduced (check whether stories were added or are missing) -If not applicable, print the Session State Block, tell the user "Phase 4 skipped — no test changes and no new components or services detected," and ask if they are ready to move to Phase 5. Stop. +If not applicable, print the Session State Block, tell the user "Phase 4 skipped — no story changes and no new components detected," and ask if they are ready to move to Phase 5. Stop. Otherwise: -1. Run the Phase 4 deterministic floor: test suite scoped to changed test files, full suite if source files changed, coverage check on new components/services, pattern check for `.skip` / `.only` / `xit` / `xdescribe`. +1. Run the Phase 4 deterministic floor: `pnpm test` is not configured — record "not configured — skipped"; run Storybook story coverage check on new/modified components; pattern check for `.skip` / `.only` / `xit` / `xdescribe` in story files. 2. Read `docs/agent-workflows/test-suite-quality-review.md` in full and run any additional checks it defines. -3. Add subjective observations after deterministic checks (test naming, assertion quality, missing edge cases), each labeled `Subjective`. +3. Add subjective observations after deterministic checks (story naming, state coverage, missing edge-case stories), each labeled `Subjective`. 4. Print phase narrative + Session State Block + Gate Block (with Coverage report). Stop. ## Phase 5 — Feature flag gating review (`feature-flag-gating-review.md`) If the feature flag review was already completed during Phase 1, print the Session State Block, tell the user "Phase 5 already completed during Phase 1 — see flag review output above," and ask if they are ready to move to Phase 6. Stop. -Otherwise, run if any file references feature flags or env-gated behavior (`featureFlag`, `featureGate`, `NEXT_PUBLIC_*` toggles, or similar). +Otherwise, run if any file introduces new `NEXT_PUBLIC_*` env vars used for behavioral gating (not just API base URL configuration), or adds new behavioral mode branches that require on/off path verification. If not applicable, print the Session State Block, tell the user "Phase 5 skipped — no feature flag changes detected," and ask if they are ready to move to Phase 6. Stop. @@ -681,7 +680,7 @@ Before sending any response in this workflow, verify: - [ ] Am I about to make any edit not on the Approved Changes Ledger? If so, stop and ask. - [ ] Did I check for intentional-behavior comments before any edit I'm about to make? - [ ] If the finding I'm about to fix changes I/O, network, security, auth, or error-handling behavior: did I enumerate what user-facing features and external services currently route through this code path, and confirm the fix preserves their existing behavior? If not, do that before editing. -- [ ] After each edit: did I re-read the lines I just wrote as if reviewing a colleague's code (not my own), check that the fix enforces its invariant at every layer the call path touches, and run `pnpm test -- --findRelatedTests ` when a test script exists (otherwise typecheck/lint)? Test failures are findings — not items to defer. +- [ ] After each edit: did I re-read the lines I just wrote as if reviewing a colleague's code (not my own), check that the fix enforces its invariant at every layer the call path touches, and run `pnpm typecheck` and `pnpm lint` on touched frontend files? Failures are findings — not items to defer. - [ ] Am I using any forbidden phrases ("follow-up PR," "ship as-is," "defer," "optional fix," etc.)? - [ ] Am I framing findings as "would you like to fix" instead of "I will fix unless you waive"? - [ ] If this is a phase with a deterministic floor (Phases 1–5), did I actually execute every required command from the Phase deterministic check list, and did I include the Coverage report in the Gate Block with verbatim command results? diff --git a/docs/agent-workflows/test-suite-quality-review.md b/docs/agent-workflows/test-suite-quality-review.md index 0e083d7..fa6f70d 100644 --- a/docs/agent-workflows/test-suite-quality-review.md +++ b/docs/agent-workflows/test-suite-quality-review.md @@ -6,7 +6,7 @@ Reviews the **quality and simplicity of the existing test suite** — whether te **Does not:** check for test coverage gaps in newly written code (that is handled by `code-quality-review.md`). Does not run tests or generate new tests. Scope is existing test quality only, not coverage metrics. -**Note:** Interview Coach may not have a frontend test runner configured yet. If no tests exist, state that plainly and limit the review to Storybook story quality per `AGENTS.md` when `.stories.tsx` files are in scope. +**Note:** Interview Coach has no frontend test runner configured. `pnpm test` does not exist in `frontend/package.json`. When invoked, limit this review to **Storybook story quality** — `.stories.tsx` files are the primary coverage mechanism for this project. State "no test runner configured" at the top and proceed directly to story review. --- @@ -80,16 +80,6 @@ Apply the same standard to every test category that exists in the project: - focused on important behavior - not overengineered -## End-to-end test review requirements - -If end-to-end tests exist, check whether they: - -- focus on the most important real user flows -- avoid trying to cover every variation and edge case -- avoid becoming long, fragile, or over-scripted -- avoid excessive setup, orchestration, or custom utilities unless absolutely necessary -- feel like a practical validation of core workflows rather than a large QA automation system - ## Specific things to flag - tests that are too complex for their value @@ -113,14 +103,12 @@ State clearly: - whether the suite complies with the simplicity requirement - whether it feels like realistic student-level work or feels too advanced, overbuilt, or artificial -### 2. Breakdown by test type +### 2. Breakdown by coverage type -Organize into sections only for the test categories that actually exist, such as: +This project's primary coverage mechanism is Storybook stories. Organize into sections only for coverage types that actually exist: -- Unit tests -- Integration tests -- Component or UI tests -- End-to-end tests +- Storybook stories (`.stories.tsx`) — required for all reusable components and components with meaningful visual states +- Unit tests — only if any `.test.ts` or `.test.tsx` files exist (currently none configured) ### 3. File-by-file or area-by-area review