Add feedback tone check (#69)

JOURNAL.md

@@ -0,0 +1,78 @@
+## Week 7 — Issue selection
+
+**Issue link:** https://github.com/ascherj/pathreview/issues/69
+
+**Issue title:** Add a "feedback tone check" that ensures all generated feedback is written constructively
+
+**Tier:** [ ] Tier 1  [x] Tier 2  [ ] Tier 3
+
+**Problem summary:**
+After PathReview generates feedback for a user, there is currently no check on whether that feedback is written constructively. This issue asks for a tone classification step to run after generation, using a prompt to judge whether each feedback section is constructive (actionable, specific, encouraging) or negative (discouraging, vague, dismissive). Sections that fail the check should be rejected and regenerated rather than shown to the user. The main files affected are `safety/content_filter.py` and `rag/generator/review_generator.py`, so the fix touches both the safety layer and the review generation pipeline.
+
+**Branch name:** feat/69-feedback-tone-check
+
+**Setup confirmation:** [x] App runs locally at localhost:5173
+
+**Cohort ledger:** [x] Issue added to cohort ledger
+
+## Week 8 — Reproduction & solution planning
+
+**Reproduction commit link:** https://github.com/SaimM2007/pathreview/commit/271b823
+
+**Reproduction summary:**
+I added a test showing that `ContentFilter.filter()` only catches explicitly harmful content and has no concept of constructive vs discouraging tone. A harsh but non-harmful feedback string passes through unchanged, confirming the tone check gap described in issue #69.
+
+**PLAN.md link:** https://github.com/SaimM2007/pathreview/blob/feat/69-feedback-tone-check/PLAN.md
+
+**Blockers or open questions:**
+Still unsure how strict the tone classification prompt should be without over-rejecting valid critical feedback.
+
+## Week 9 — Implementation & PR Submission
+
+### Check-in 1 (mid-week)
+
+**Current progress:**
+Implemented the `ToneChecker` class in `safety/content_filter.py`, alongside the existing `ContentFilter`. It takes the same LLM client used for review generation and classifies a piece of feedback text as constructive or negative using a dedicated prompt — distinguishing "critical but specific and actionable" feedback (which should pass) from "vague or dismissive" feedback (which should fail), per the distinction laid out in PLAN.md. Also handled the empty/short-content edge case identified in planning: `ToneChecker.check()` returns constructive without calling the LLM when there's nothing meaningful to classify, so it can't get stuck looping on blank input.
+
+**Next steps:**
+Wire `ToneChecker` into `ReviewGenerator.generate_section()` so every generated section is checked before being returned, with a capped retry/regeneration loop and a logged fallback if it never passes. Then write unit tests for both files and run `make check` / `make test-unit`.
+
+**Blockers:**
+None so far.
+
+---
+
+### Check-in 2 (end of week)
+
+**PR link:** https://github.com/ascherj/pathreview/pull/1013
+
+**Branch:** feat/69-feedback-tone-check
+
+**What you built:**
+Wired `ToneChecker` into `ReviewGenerator.generate_section()` — the original generation logic was extracted into a `_generate_section_once()` helper, and `generate_section()` now runs the tone check after each generation, regenerating up to `MAX_TONE_RETRIES` (2) times if a section fails. If a section still fails after all retries, the last attempt is returned with its confidence score lowered rather than looping indefinitely, and a `structlog` warning is logged so the fallback is visible in logs rather than silent.
+
+**Tests added or updated:**
+`tests/unit/test_content_filter.py` — added tests for `ToneChecker` covering constructive feedback, negative feedback, critical-but-specific feedback (must not be falsely flagged), and the empty-content edge case. `tests/unit/test_review_generator_tone_check.py` (new) — covers `generate_section()` passing on the first attempt, succeeding after one regeneration, and falling back correctly after exhausting all retries. Ran the full unit suite (428 tests) before and after this change: 53 pre-existing failures exist on `main` in unrelated modules (`review_service`, `resume_parser`, `security`, `skill_extractor`, etc.), and this branch introduces 0 new failures. Similarly, `make check` (lint) reports pre-existing issues across the repo that predate this branch — none in the files this PR touches.
+
+**Self-review confirmation:** [x] tests for this issue (`test_content_filter.py`, `test_review_generator_tone_check.py`) pass — 8/8  [x] no new lint or test failures introduced vs. `main`
+
+**Draft PR feedback received from:** None
+
+## Week 10 — Iteration & reflection
+
+**PR link:** https://github.com/ascherj/pathreview/pull/1013
+
+**Review feedback status:** No reviews or comments have been left on the PR as of this writing. Nothing to respond to yet.
+
+**Reflection:**
+
+I chose issue #69 (feedback tone check) because it was scoped enough to understand in an afternoon but still touched two modules, safety/ and rag/generator/, which forced me to actually trace how a feature moves through the codebase instead of just patching one file in isolation.
+
+The most useful moment was writing the reproduction test before touching any implementation. It made the vague issue description concrete: I could point to the exact line in ContentFilter.filter() where harsh-but-not-harmful feedback slipped through, instead of just describing the problem abstractly. That test also became the backbone for my later tests, since I extended the same file instead of starting from scratch.
+
+The trickiest design decision was the retry logic in generate_section(). My first instinct was to just flag failing sections and move on, but that would let bad tone reach users anyway. I ended up capping retries at 2 and falling back to the last attempt with a lowered confidence score and a logged warning, rather than looping forever or silently failing. I'm still not fully confident the tone classification prompt is tuned correctly (too strict could reject valid critical feedback, too lenient defeats the purpose), which is the open question I left on the PR.
+
+If I were starting over with what I know now, I'd write the retry/fallback test cases before writing the retry logic itself, rather than after. I found the edge cases (empty content, repeated failures) while implementing, not while planning, and PLAN.md would have been stronger if I'd thought through those cases up front instead of discovering them mid-build.
+
+**Blockers or open questions:**
+Waiting on maintainer review, no feedback received yet at time of submission.

PLAN.md

@@ -0,0 +1,32 @@
+## Solution plan
+
+**Issue:** Add a "feedback tone check" that ensures all generated feedback is written constructively (#69)
+https://github.com/ascherj/pathreview/issues/69
+
+### Understand
+Currently, `ReviewGenerator.generate_section()` in `rag/generator/review_generator.py` calls the LLM and returns the parsed feedback section directly, with no tone validation. `ContentFilter.filter()` in `safety/content_filter.py` only catches explicitly harmful content (self-harm, slurs, illegal activity) via regex, it has no concept of "constructive vs discouraging" tone, and it's never even called from the review generation flow. Expected behavior: every feedback section shown to a user should be constructive (actionable, specific, encouraging). Actual behavior: harsh or discouraging feedback can be generated and returned with zero checks, confirmed by the reproduction test in tests/unit/test_content_filter.py.
+
+### Map
+- `rag/generator/review_generator.py`: `generate_section()` and `generate_full_review()`, where the tone check needs to be called after generation
+- `safety/content_filter.py`: where a new `ToneChecker` class will live alongside `ContentFilter`
+- `tests/unit/test_content_filter.py`: where reproduction and future tests live
+
+### Plan
+1. Add a `ToneChecker` class in `safety/content_filter.py` that uses an LLM prompt to classify feedback as constructive or negative
+2. Wire `ToneChecker` into `ReviewGenerator.generate_section()` so every generated section gets checked before being returned
+3. If a section fails the check, regenerate it (retry the LLM call once or twice) instead of returning it as-is
+4. Add logging (using the existing `structlog` logger) when a section fails and gets regenerated
+5. Add unit tests covering both constructive and negative example feedback
+
+### Inputs & outputs
+Input: a generated `FeedbackSection` (or raw text) from the LLM. Output: either the same section (if it passes the tone check) or a regenerated section that passes.
+
+### Risks & unknowns
+- Adding another LLM call per section increases latency and cost, need to confirm this is acceptable
+- Risk of infinite retry loops if regenerated content keeps failing the tone check, need a max retry limit
+- Unsure how strict the tone classifier prompt should be, too strict could over-reject valid critical feedback
+
+### Edge cases
+- Feedback that's short/empty (edge case where there's little to classify)
+- Feedback that's constructive but still contains critical points, must not falsely flag as negative
+- Repeated regeneration failures (need fallback behavior, e.g. use last attempt with a warning flag rather than looping forever)

rag/generator/review_generator.py

@@ -1,19 +1,22 @@
 """LLM-based review generation."""
 
 from dataclasses import dataclass
-from typing import Optional
+
 import openai
 import structlog
 
+from safety.content_filter import ToneChecker
+
+from .output_parser import FeedbackSection, parse_review_output
 from .prompt_templates import get_template
-from .output_parser import parse_review_output, FeedbackSection
 
 logger = structlog.get_logger()
 
 
 @dataclass
 class ReviewConfig:
     """Configuration for review generation."""
+
     api_key: str
     base_url: str
     model: str
@@ -31,14 +34,13 @@ def __init__(self, config: ReviewConfig):
             config: ReviewConfig with API settings
         """
         self.config = config
-        self.client = openai.OpenAI(
-            api_key=config.api_key,
-            base_url=config.base_url
-        )
+        self.client = openai.OpenAI(api_key=config.api_key, base_url=config.base_url)
+        self.tone_checker = ToneChecker(self.client, self.config.model)
 
-    def generate_section(self, section_name: str, context_chunks: list[dict],
-                        profile_data: dict) -> FeedbackSection:
-        """Generate feedback for a specific section.
+    def _generate_section_once(
+        self, section_name: str, context_chunks: list[dict], profile_data: dict
+    ) -> FeedbackSection:
+        """Generate feedback for a section with a single LLM call (no tone check).
 
         Args:
             section_name: Section name (skills_feedback, projects_feedback, etc.)
@@ -57,20 +59,18 @@ def generate_section(self, section_name: str, context_chunks: list[dict],
         project_count = len(profile_data.get("projects", []))
 
         prompt = template.format(
-            context=context_text,
-            github_username=github_username,
-            project_count=project_count
+            context=context_text, github_username=github_username, project_count=project_count
         )
 
         # Call LLM
         response = self.client.chat.completions.create(
             model=self.config.model,
             messages=[
                 {"role": "system", "content": "You are an expert portfolio reviewer."},
-                {"role": "user", "content": prompt}
+                {"role": "user", "content": prompt},
             ],
             temperature=self.config.temperature,
-            max_tokens=self.config.max_tokens
+            max_tokens=self.config.max_tokens,
         )
 
         content = response.choices[0].message.content
@@ -84,14 +84,58 @@ def generate_section(self, section_name: str, context_chunks: list[dict],
 
         logger.warning("no_sections_parsed", section_name=section_name)
         return FeedbackSection(
+            section_name=section_name, content=content, confidence=0.6, suggestions=[]
+        )
+
+    def generate_section(
+        self, section_name: str, context_chunks: list[dict], profile_data: dict
+    ) -> FeedbackSection:
+        """Generate feedback for a section, retrying if it fails the tone check.
+
+        Args:
+            section_name: Section name (skills_feedback, projects_feedback, etc.)
+            context_chunks: Retrieved context chunks
+            profile_data: Profile metadata
+
+        Returns:
+            FeedbackSection with generated, tone-checked content
+        """
+        last_section = None
+
+        for attempt in range(ToneChecker.MAX_RETRIES + 1):
+            section = self._generate_section_once(section_name, context_chunks, profile_data)
+            last_section = section
+
+            tone_result = self.tone_checker.check(section.content)
+
+            if tone_result.is_constructive:
+                if attempt > 0:
+                    logger.info(
+                        "section_passed_after_retry", section_name=section_name, attempt=attempt
+                    )
+                return section
+
+            logger.warning(
+                "section_failed_tone_check",
+                section_name=section_name,
+                attempt=attempt,
+                verdict=tone_result.raw_response,
+            )
+
+        # Exhausted retries - return last attempt with a warning flag rather than
+        # looping forever or silently returning None
+        logger.warning(
+            "tone_check_retries_exhausted",
             section_name=section_name,
-            content=content,
-            confidence=0.6,
-            suggestions=[]
+            max_retries=ToneChecker.MAX_RETRIES,
         )
+        assert last_section is not None  # loop always runs at least once
+        last_section.confidence = min(last_section.confidence, 0.3)
+        return last_section
 
-    def generate_full_review(self, profile_data: dict,
-                            retrieved_chunks: list[dict]) -> list[FeedbackSection]:
+    def generate_full_review(
+        self, profile_data: dict, retrieved_chunks: list[dict]
+    ) -> list[FeedbackSection]:
         """Generate complete review across all sections.
 
         Args:
@@ -106,16 +150,14 @@ def generate_full_review(self, profile_data: dict,
             "projects_feedback",
             "presentation_feedback",
             "gaps_feedback",
-            "first_impression"
+            "first_impression",
         ]
 
         all_sections = []
 
         for section_name in section_names:
             try:
-                section = self.generate_section(
-                    section_name, retrieved_chunks, profile_data
-                )
+                section = self.generate_section(section_name, retrieved_chunks, profile_data)
 
                 # Add source citations if available
                 section = self._add_citations(section, retrieved_chunks)
@@ -124,15 +166,16 @@ def generate_full_review(self, profile_data: dict,
                 logger.info("section_generated", section=section_name)
 
             except Exception as e:
-                logger.error("section_generation_failed", section=section_name,
-                           error=str(e))
+                logger.error("section_generation_failed", section=section_name, error=str(e))
                 # Continue with remaining sections
-                all_sections.append(FeedbackSection(
-                    section_name=section_name,
-                    content=f"Error generating {section_name}",
-                    confidence=0.0,
-                    suggestions=[]
-                ))
+                all_sections.append(
+                    FeedbackSection(
+                        section_name=section_name,
+                        content=f"Error generating {section_name}",
+                        confidence=0.0,
+                        suggestions=[],
+                    )
+                )
 
         # Consolidate duplicates across similar projects
         all_sections = self._consolidate_feedback(all_sections)
@@ -160,8 +203,7 @@ def _format_context(chunks: list[dict]) -> str:
         return "\n\n".join(parts)
 
     @staticmethod
-    def _add_citations(section: FeedbackSection,
-                       retrieved_chunks: list[dict]) -> FeedbackSection:
+    def _add_citations(section: FeedbackSection, retrieved_chunks: list[dict]) -> FeedbackSection:
         """Add source citations to feedback section.
 
         Args:

safety/content_filter.py

@@ -1,6 +1,9 @@
 """Content filter for generated feedback."""
 
 import re
+from dataclasses import dataclass
+from typing import Any
+
 import structlog
 
 logger = structlog.get_logger()
@@ -37,6 +40,70 @@ def filter(text: str) -> tuple[str, bool]:
                 logger.warning("harmful_content_detected", pattern=pattern)
                 was_filtered = True
                 # Replace harmful phrases with neutral text
-                filtered_text = re.sub(pattern, "[CONTENT REMOVED]", filtered_text, flags=re.IGNORECASE)
+                filtered_text = re.sub(
+                    pattern, "[CONTENT REMOVED]", filtered_text, flags=re.IGNORECASE
+                )
 
         return filtered_text, was_filtered
+
+
+@dataclass
+class ToneCheckResult:
+    """Result of a tone classification check."""
+
+    is_constructive: bool
+    raw_response: str
+
+
+class ToneChecker:
+    """Classify generated feedback as constructive or negative using an LLM."""
+
+    MAX_RETRIES = 2
+
+    SYSTEM_PROMPT = (
+        "You are a strict but fair classifier. Given a piece of portfolio review "
+        "feedback, decide if it is CONSTRUCTIVE (actionable, specific, and "
+        "encouraging, even if it contains criticism) or NEGATIVE (discouraging, "
+        "vague, dismissive, or overly harsh with no actionable guidance). "
+        "Respond with exactly one word: CONSTRUCTIVE or NEGATIVE."
+    )
+
+    def __init__(self, client: Any, model: str):
+        """Initialize the tone checker.
+
+        Args:
+            client: An OpenAI-compatible chat client (reused from ReviewGenerator)
+            model: Model name to use for classification
+        """
+        self.client = client
+        self.model = model
+
+    def check(self, text: str) -> ToneCheckResult:
+        """Classify a piece of feedback text as constructive or not.
+
+        Args:
+            text: Feedback content to classify
+
+        Returns:
+            ToneCheckResult with the verdict and raw model response
+        """
+        if not text or not text.strip():
+            # Nothing to classify - don't reject on empty content
+            return ToneCheckResult(is_constructive=True, raw_response="")
+
+        response = self.client.chat.completions.create(
+            model=self.model,
+            messages=[
+                {"role": "system", "content": self.SYSTEM_PROMPT},
+                {"role": "user", "content": text},
+            ],
+            temperature=0.0,
+            max_tokens=5,
+        )
+        raw_response = response.choices[0].message.content.strip().upper()
+        is_ok = raw_response.startswith("CONSTRUCTIVE")
+
+        if not is_ok:
+            logger.warning("feedback_tone_check_failed", verdict=raw_response)
+
+        return ToneCheckResult(is_constructive=is_ok, raw_response=raw_response)