From 28c4a110ed0c43d378612bffcfd1686cbfa5c583 Mon Sep 17 00:00:00 2001 From: SaimM2007 Date: Sun, 26 Jul 2026 00:52:22 -0400 Subject: [PATCH 1/9] docs: add JOURNAL.md for week 7 issue selection --- JOURNAL.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 JOURNAL.md diff --git a/JOURNAL.md b/JOURNAL.md new file mode 100644 index 000000000..6d1959b87 --- /dev/null +++ b/JOURNAL.md @@ -0,0 +1,16 @@ +## 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:** +Right now, PathReview generates feedback for users without checking whether the tone is constructive. This means the app could produce feedback that reads as harsh or discouraging without any safeguard catching it before it reaches the user. This issue asks for a tone check step added to the feedback generation pipeline, likely inside the review or agent logic, that verifies generated feedback is written supportively and flags or rewrites anything that isn't. A successful fix ensures every piece of feedback shown to a user is honest but constructive in tone. + +**Branch name:** feat/69-feedback-tone-check + +**Setup confirmation:** [x] App runs locally at localhost:5173 + +**Cohort ledger:** [x] Issue added to cohort ledger \ No newline at end of file From 7d86cae8ccedbaf07d7b4a9e96f909deec9eb0b7 Mon Sep 17 00:00:00 2001 From: SaimM2007 Date: Sun, 26 Jul 2026 00:57:10 -0400 Subject: [PATCH 2/9] docs: refine Week 7 problem summary with issue details --- JOURNAL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/JOURNAL.md b/JOURNAL.md index 6d1959b87..a357ab0ff 100644 --- a/JOURNAL.md +++ b/JOURNAL.md @@ -7,7 +7,7 @@ **Tier:** [ ] Tier 1 [x] Tier 2 [ ] Tier 3 **Problem summary:** -Right now, PathReview generates feedback for users without checking whether the tone is constructive. This means the app could produce feedback that reads as harsh or discouraging without any safeguard catching it before it reaches the user. This issue asks for a tone check step added to the feedback generation pipeline, likely inside the review or agent logic, that verifies generated feedback is written supportively and flags or rewrites anything that isn't. A successful fix ensures every piece of feedback shown to a user is honest but constructive in tone. +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 From 271b823b5a3c6cfdaf6b3ac6e88dc234ed0ba7e4 Mon Sep 17 00:00:00 2001 From: SaimM2007 Date: Sat, 1 Aug 2026 23:43:21 -0400 Subject: [PATCH 3/9] test(safety): reproduce issue #69, tone check gap in content_filter --- tests/unit/test_content_filter.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 tests/unit/test_content_filter.py diff --git a/tests/unit/test_content_filter.py b/tests/unit/test_content_filter.py new file mode 100644 index 000000000..7bca18a63 --- /dev/null +++ b/tests/unit/test_content_filter.py @@ -0,0 +1,19 @@ +"""Tests for content_filter.py, including reproduction of issue #69.""" + +from safety.content_filter import ContentFilter + + +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 From 9d6d228d8e6836dfb0ad7b1c657e4194e2823872 Mon Sep 17 00:00:00 2001 From: SaimM2007 Date: Sat, 1 Aug 2026 23:44:36 -0400 Subject: [PATCH 4/9] docs: add PLAN.md for issue #69 --- PLAN.md | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 PLAN.md 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 From 47fadd125d51d568c801cecc8388cbaa100576fa Mon Sep 17 00:00:00 2001 From: SaimM2007 Date: Sat, 1 Aug 2026 23:47:05 -0400 Subject: [PATCH 5/9] docs: add Week 8 journal entry --- JOURNAL.md | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/JOURNAL.md b/JOURNAL.md index a357ab0ff..56adfa4a2 100644 --- a/JOURNAL.md +++ b/JOURNAL.md @@ -13,4 +13,16 @@ After PathReview generates feedback for a user, there is currently no check on w **Setup confirmation:** [x] App runs locally at localhost:5173 -**Cohort ledger:** [x] Issue added to cohort ledger \ No newline at end of file +**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. \ No newline at end of file From 88a1105be0e17b6e6aba89e6e7f1d1da4d3041a3 Mon Sep 17 00:00:00 2001 From: SaimM2007 Date: Sun, 9 Aug 2026 03:06:56 -0400 Subject: [PATCH 6/9] feat: add tone check for generated feedback (#69) --- JOURNAL.md | 33 +++++++- .../unit/test_review_generator_tone_check.py | 79 +++++++++++++++++++ 2 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 tests/unit/test_review_generator_tone_check.py diff --git a/JOURNAL.md b/JOURNAL.md index 56adfa4a2..a5b224621 100644 --- a/JOURNAL.md +++ b/JOURNAL.md @@ -25,4 +25,35 @@ I added a test showing that `ContentFilter.filter()` only catches explicitly har **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. \ No newline at end of file +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:** [paste your PR link here once opened] + +**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. Confirmed via `git stash` that `main` has 183 pre-existing lint errors and 53 pre-existing test failures, none in files touched by this change — this branch introduces 0 new lint errors and 0 new test failures. + +**Self-review confirmation:** [x] make check passes [x] make test-unit passes + +**Draft PR feedback received from:** None \ No newline at end of file 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 From ef881058d85df18f629da026fcc73525f4970ee5 Mon Sep 17 00:00:00 2001 From: SaimM2007 Date: Sun, 9 Aug 2026 03:29:37 -0400 Subject: [PATCH 7/9] feat: add tone check for generated feedback (#69) --- JOURNAL.md | 4 +- rag/generator/review_generator.py | 108 +++++++++++++++++++++--------- safety/content_filter.py | 69 ++++++++++++++++++- tests/unit/test_content_filter.py | 57 +++++++++++++++- 4 files changed, 201 insertions(+), 37 deletions(-) diff --git a/JOURNAL.md b/JOURNAL.md index a5b224621..ae6c514a5 100644 --- a/JOURNAL.md +++ b/JOURNAL.md @@ -52,8 +52,8 @@ None so far. 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. Confirmed via `git stash` that `main` has 183 pre-existing lint errors and 53 pre-existing test failures, none in files touched by this change — this branch introduces 0 new lint errors and 0 new test failures. +`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] make check passes [x] make test-unit passes +**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 \ 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 index 7bca18a63..44c7fac2d 100644 --- a/tests/unit/test_content_filter.py +++ b/tests/unit/test_content_filter.py @@ -1,6 +1,8 @@ """Tests for content_filter.py, including reproduction of issue #69.""" -from safety.content_filter import ContentFilter +from unittest.mock import MagicMock + +from safety.content_filter import ContentFilter, ToneChecker def test_harsh_feedback_not_caught_by_tone_check() -> None: @@ -17,3 +19,56 @@ def test_harsh_feedback_not_caught_by_tone_check() -> None: # 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 From 5ab8c0bcaf2a4cf6da847f31b754fb79b9e34c81 Mon Sep 17 00:00:00 2001 From: SaimM2007 Date: Sun, 9 Aug 2026 03:34:23 -0400 Subject: [PATCH 8/9] docs: add PR link to journal --- JOURNAL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/JOURNAL.md b/JOURNAL.md index ae6c514a5..037214f64 100644 --- a/JOURNAL.md +++ b/JOURNAL.md @@ -44,7 +44,7 @@ None so far. ### Check-in 2 (end of week) -**PR link:** [paste your PR link here once opened] +**PR link:** https://github.com/ascherj/pathreview/pull/1013 **Branch:** feat/69-feedback-tone-check From a4fb179353935ac3e5686dc0d0f34877a405a038 Mon Sep 17 00:00:00 2001 From: SaimM2007 Date: Thu, 13 Aug 2026 01:24:16 -0400 Subject: [PATCH 9/9] docs: add Week 10 reflection --- JOURNAL.md | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/JOURNAL.md b/JOURNAL.md index 037214f64..b72fe6cfe 100644 --- a/JOURNAL.md +++ b/JOURNAL.md @@ -56,4 +56,23 @@ Wired `ToneChecker` into `ReviewGenerator.generate_section()` — the original g **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 \ No newline at end of file +**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