diff --git a/JOURNAL.md b/JOURNAL.md new file mode 100644 index 000000000..b72fe6cfe --- /dev/null +++ b/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. \ No newline at end of file diff --git a/PLAN.md b/PLAN.md new file mode 100644 index 000000000..5e5a31104 --- /dev/null +++ b/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) \ No newline at end of file diff --git a/rag/generator/review_generator.py b/rag/generator/review_generator.py index c1c5ee71d..39861b5b5 100644 --- a/rag/generator/review_generator.py +++ b/rag/generator/review_generator.py @@ -1,12 +1,14 @@ """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() @@ -14,6 +16,7 @@ @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,9 +59,7 @@ 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 @@ -67,10 +67,10 @@ def generate_section(self, section_name: str, context_chunks: list[dict], 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: diff --git a/safety/content_filter.py b/safety/content_filter.py index 8e8c1a092..27dac03e4 100644 --- a/safety/content_filter.py +++ b/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) diff --git a/tests/unit/test_content_filter.py b/tests/unit/test_content_filter.py new file mode 100644 index 000000000..44c7fac2d --- /dev/null +++ b/tests/unit/test_content_filter.py @@ -0,0 +1,74 @@ +"""Tests for content_filter.py, including reproduction of issue #69.""" + +from unittest.mock import MagicMock + +from safety.content_filter import ContentFilter, ToneChecker + + +def test_harsh_feedback_not_caught_by_tone_check() -> None: + """Reproduces issue #69: no tone check exists for constructive feedback. + + ContentFilter only catches explicitly harmful patterns (self-harm, + slurs, illegal activity, etc). It has no concept of discouraging or + unconstructive tone, so harsh-but-not-"harmful" feedback passes + through unfiltered. This test demonstrates that gap. + """ + harsh_content = "This work is mediocre and shows little effort or skill." + filtered_text, was_filtered = ContentFilter.filter(harsh_content) + + # Currently passes unchanged because ContentFilter has no tone check + assert filtered_text == harsh_content + assert was_filtered is False + + +def _mock_client(verdict: str) -> MagicMock: + """Build a mock OpenAI-compatible client that returns a fixed verdict.""" + client = MagicMock() + client.chat.completions.create.return_value.choices = [ + MagicMock(message=MagicMock(content=verdict)) + ] + return client + + +def test_tone_checker_accepts_constructive_feedback() -> None: + client = _mock_client("CONSTRUCTIVE") + checker = ToneChecker(client, model="test-model") + + result = checker.check( + "Consider adding unit tests to your API routes - this would " + "strengthen your project and show attention to reliability." + ) + + assert result.is_constructive is True + + +def test_tone_checker_rejects_negative_feedback() -> None: + client = _mock_client("NEGATIVE") + checker = ToneChecker(client, model="test-model") + + result = checker.check("This work is mediocre and shows little effort or skill.") + + assert result.is_constructive is False + + +def test_tone_checker_accepts_critical_but_actionable_feedback() -> None: + """Feedback with real criticism should still pass if it's actionable.""" + client = _mock_client("CONSTRUCTIVE") + checker = ToneChecker(client, model="test-model") + + result = checker.check( + "Your README is missing setup instructions, which makes it hard for " + "reviewers to run your project. Adding a quickstart section would fix this." + ) + + assert result.is_constructive is True + + +def test_tone_checker_handles_empty_content() -> None: + """Empty/short feedback has nothing to classify, so it shouldn't be rejected.""" + client = _mock_client("NEGATIVE") + checker = ToneChecker(client, model="test-model") + + result = checker.check("") + + assert result.is_constructive is True diff --git a/tests/unit/test_review_generator_tone_check.py b/tests/unit/test_review_generator_tone_check.py new file mode 100644 index 000000000..3c765b583 --- /dev/null +++ b/tests/unit/test_review_generator_tone_check.py @@ -0,0 +1,79 @@ +"""Tests for the tone-check retry wiring in ReviewGenerator.generate_section (issue #69).""" + +from unittest.mock import MagicMock, patch + +from rag.generator.output_parser import FeedbackSection +from rag.generator.review_generator import ReviewConfig, ReviewGenerator +from safety.content_filter import ToneCheckResult + + +def _make_config() -> ReviewConfig: + return ReviewConfig( + api_key="test-key", + base_url="https://example.invalid/v1", + model="test-model", + ) + + +def _make_section(content: str = "some feedback") -> FeedbackSection: + return FeedbackSection( + section_name="skills_feedback", + content=content, + confidence=0.9, + suggestions=[], + ) + + +@patch("rag.generator.review_generator.openai.OpenAI") +def test_generate_section_returns_first_attempt_when_constructive(mock_openai: MagicMock) -> None: + """If the first generation passes the tone check, no retry should happen.""" + generator = ReviewGenerator(_make_config()) + generator._generate_section_once = MagicMock(return_value=_make_section("good feedback")) + generator.tone_checker.check = MagicMock( + return_value=ToneCheckResult(is_constructive=True, raw_response="CONSTRUCTIVE") + ) + + result = generator.generate_section("skills_feedback", [], {}) + + assert result.content == "good feedback" + generator._generate_section_once.assert_called_once() + + +@patch("rag.generator.review_generator.openai.OpenAI") +def test_generate_section_regenerates_after_failed_tone_check(mock_openai: MagicMock) -> None: + """A section that fails the tone check once should be regenerated and + the regenerated (passing) version should be returned.""" + generator = ReviewGenerator(_make_config()) + generator._generate_section_once = MagicMock( + side_effect=[_make_section("harsh feedback"), _make_section("better feedback")] + ) + generator.tone_checker.check = MagicMock( + side_effect=[ + ToneCheckResult(is_constructive=False, raw_response="NEGATIVE"), + ToneCheckResult(is_constructive=True, raw_response="CONSTRUCTIVE"), + ] + ) + + result = generator.generate_section("skills_feedback", [], {}) + + assert result.content == "better feedback" + assert generator._generate_section_once.call_count == 2 + + +@patch("rag.generator.review_generator.openai.OpenAI") +def test_generate_section_falls_back_after_exhausting_retries(mock_openai: MagicMock) -> None: + """If every attempt keeps failing the tone check, generate_section should + stop retrying (not loop forever) and return the last attempt with a + lowered confidence score, per PLAN.md's fallback-behavior edge case.""" + generator = ReviewGenerator(_make_config()) + generator._generate_section_once = MagicMock(return_value=_make_section("still harsh feedback")) + generator.tone_checker.check = MagicMock( + return_value=ToneCheckResult(is_constructive=False, raw_response="NEGATIVE") + ) + + result = generator.generate_section("skills_feedback", [], {}) + + assert result.content == "still harsh feedback" + assert result.confidence <= 0.3 + # 1 initial + MAX_TONE_RETRIES(2) regenerations = 3 total generation calls + assert generator._generate_section_once.call_count == 3