fix: recover wrapped classifier JSON for phase 8 demo

[Galzi1]

Summary

This PR fixes the classifier failure mode exposed by the latest Phase 8 demo attempt and updates the Phase 8 demo tracking docs with the current live status.

Why

The fresh demo run from github-pr-kb-demo reproduced the same blocker as the earlier content-demo attempt: the workflow reached classify, but model responses were not always bare top-level JSON, so classification fell through to repeated parse failures and no KB article was published.

What changed

• harden PRClassifier to recover JSON objects from:
  • bare JSON responses,
  • fenced json blocks,
  • prose-wrapped responses that still contain a valid JSON object
• add regression coverage for fenced and prose-wrapped classifier output
• update .planning/phases/08-github-action-readme/08-DEMO-COMPLETE.md with:
  • PR #3 merged status,
  • workflow run 24266078977 outcome,
  • the classification-failure-only diagnosis,
  • the current remediation status in github-pr-kb
• include the accompanying risk review artifact for the Phase 8 demo-completion work

Validation

• python -m ruff check src tests
• .venv\Scripts\python.exe -m pytest tests\test_classifier.py -q
• .venv\Scripts\python.exe -m pytest tests -q

Phase 8 context

This branch is intentionally based on gsd/phase-08-github-action-readme, not main, so the PR describes the incremental fix and status update on top of that Phase 8 workstream.

[qodo-code-review]

Review Summary by Qodo

Recover wrapped classifier JSON from model responses

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Harden classifier JSON parsing to recover wrapped/fenced model output
  - Extract JSON from bare JSON, markdown fenced blocks, and prose-wrapped responses
  - Add regex-based fence detection and multi-candidate parsing strategy
• Add regression tests for fenced and prose-wrapped JSON responses
• Update classification error handling to use new robust parser

Diagram

flowchart LR
  ModelOutput["Model output<br/>bare/fenced/prose"] -->|_parse_classification_response| JSONExtractor["Extract JSON<br/>from candidates"]
  JSONExtractor -->|fence regex| FencedJSON["Fenced code block"]
  JSONExtractor -->|raw decode| ProseJSON["Prose-wrapped JSON"]
  JSONExtractor -->|direct parse| BareJSON["Bare JSON"]
  FencedJSON --> ValidDict["Valid dict"]
  ProseJSON --> ValidDict
  BareJSON --> ValidDict
  ValidDict -->|return| Classifier["Classifier uses<br/>parsed result"]

Loading

File Changes

1. src/github_pr_kb/classifier.py 🐞 Bug fix +40/-3

Add robust JSON extraction from wrapped model output

• Add re import and _JSON_FENCE_RE regex pattern to detect markdown fenced JSON blocks
• Implement _parse_classification_response() function that attempts to extract JSON from multiple
 candidate formats: bare JSON, fenced code blocks, and prose-wrapped JSON
• Replace direct json.loads() call in _classify_comment() with new robust parser function
• Improve error handling to check for None result instead of catching JSONDecodeError

src/github_pr_kb/classifier.py

---

2. tests/test_classifier.py 🧪 Tests +45/-0

Add regression tests for wrapped JSON parsing

• Add test_markdown_fenced_json_is_parsed() to verify classifier handles markdown-fenced JSON
 responses
• Add test_prose_wrapped_json_is_parsed() to verify classifier handles JSON wrapped in prose text
• Both tests validate that classifications are correctly extracted and _failed_count remains zero

tests/test_classifier.py

---

3. .planning/phases/08-github-action-readme/08-DEMO-COMPLETE-RISK-REVIEW.md 📝 Documentation +139/-0

Risk review and mitigation for demo completion

• Comprehensive risk analysis of Phase 8 demo completion plan
• Identifies 8 key risks including unresolved classifier failure mode, insufficient observability,
 and overclaiming confidence
• Provides devil's advocacy and failure-of-imagination checks
• Recommends promoting diagnosis to hard gate, adding comment-input contract, and tightening exit
 criteria

.planning/phases/08-github-action-readme/08-DEMO-COMPLETE-RISK-REVIEW.md

---

View more (1)

4. .planning/phases/08-github-action-readme/08-DEMO-COMPLETE.md 📝 Documentation +303/-0

Phase 8 demo completion plan and status tracking

• Documents complete Phase 8 demo completion status and remaining work
• Provides diagnosis of PR #1 classification failures as malformed model output/parse mismatch
• Defines mandatory gates, comment-input contract, five-minute triage checklist, and
 article-fidelity checklist
• Includes detailed todo checklist with completion signals and expected final repo state
• Captures current status after PR #3 merge showing classification failures need fixing

.planning/phases/08-github-action-readme/08-DEMO-COMPLETE.md

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX Issues (0)

1. Parsed JSON schema unchecked ☑ 🐞 ≡

Description

_parse_classification_response returns the first JSON object it can decode (including from arbitrary
“{…}” substrings) without verifying it contains the required classification fields. PRClassifier
then applies defaults via result.get(...) and persists to classification-index.json, so an
unrelated/partial dict can silently become a permanent cache hit for that comment body.

Code

src/github_pr_kb/classifier.py[R78-110]

+def _parse_classification_response(text: str) -> dict | None:
+    """Extract a JSON object from bare, fenced, or prose-wrapped model output."""
+    stripped = text.strip()
+    if not stripped:
+        return None
+
+    decoder = json.JSONDecoder()
+    candidates: list[str] = [stripped]
+    candidates.extend(match.group(1).strip() for match in _JSON_FENCE_RE.finditer(stripped))
+
+    seen: set[str] = set()
+    for candidate in candidates:
+        if not candidate or candidate in seen:
+            continue
+        seen.add(candidate)
+        try:
+            parsed = json.loads(candidate)
+        except json.JSONDecodeError:
+            continue
+        if isinstance(parsed, dict):
+            return parsed
+
+    for idx, char in enumerate(stripped):
+        if char != "{":
+            continue
+        try:
+            parsed, _ = decoder.raw_decode(stripped[idx:])
+        except json.JSONDecodeError:
+            continue
+        if isinstance(parsed, dict):
+            return parsed
+
+    return None

Evidence

The new parser returns the first decoded dict it finds (bare, fenced, or any substring starting at
'{') and does not validate required keys. The classification path subsequently treats any dict as
usable (defaults category/confidence/summary) and writes it to the hash-based cache index, making
the outcome sticky on future runs.

src/github_pr_kb/classifier.py[78-110]
src/github_pr_kb/classifier.py[250-273]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`_parse_classification_response()` can return *any* decoded JSON object, but the classifier contract requires a specific schema (`category`, `confidence`, `summary`). Because `_classify_comment()` uses `.get(..., default)` and then caches the result by body hash, a valid-but-wrong dict (e.g. missing keys) can be silently accepted and cached, preventing future retries for that comment.
### Issue Context
The PR intentionally broadens parsing to recover JSON from fenced/prose-wrapped model outputs. That broader extraction increases the chance of accidentally decoding a non-classification dict that appears earlier in the response.
### Fix Focus Areas
- src/github_pr_kb/classifier.py[78-110]
- src/github_pr_kb/classifier.py[250-273]
### What to change
- Add a small validator (or extend `_parse_classification_response`) that only accepts a parsed dict if:
- `category` is present and is a string (and optionally is in `VALID_CATEGORIES`),
- `confidence` is present and can be parsed as a float,
- `summary` is present and is a string.
- If a candidate dict fails validation, continue searching (next fenced candidate / next `{` occurrence) rather than returning it.
- If no valid schema dict is found, return `None` so the failure path (warning + failed counter) is used and nothing is cached.
- Add a unit test asserting that a response containing an unrelated JSON dict before the real classification (or a `{}` response) is rejected (i.e., counts as failed and does not populate `_index`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more