feat(agent): improve reviewer resilience, auth mapping, and shell reporting

Author: elkaix

AGENTS.md

@@ -32,6 +32,11 @@ subagents, skills, web/visualization UIs, and multi-provider LLM authentication.
   and agent spec semantics need tests/docs when changed.
 - **Do not modify git config, skip hooks, force-push, reset hard, or delete branches/worktrees**
   unless the user explicitly asks and confirms the destructive action.
+- **Always check the CodeRabbit review before merging a PR.** Before merging (`gh pr merge` or the
+  GitHub UI), confirm CodeRabbit has finished reviewing the PR's head commit — its `CodeRabbit`
+  commit status is `success`, not `pending`/`failure` or absent — and read the review summary and
+  any "Actionable comments posted: N" findings. Do not merge while CodeRabbit is still reviewing or
+  on an unreviewed commit; surface unresolved actionable findings instead of merging past them.
 
 ## Quick commands
 

packages/pythinker-review/src/pythinker_review/reviewers/common.py

@@ -12,10 +12,29 @@
 from pythinker_review.reviewers.schema import RawFinding, ReviewerOutput
 from pythinker_review.store.models import ChunkFailureReason
 
-_RETRY_SUFFIX = (
-    "\n\nIMPORTANT: Your previous response was not valid JSON for the given schema. "
-    "Reply with strict JSON only, no prose, no markdown fences."
-)
+_RETRY_ERROR_BUDGET = 600
+
+
+def _retry_suffix(last_error: str) -> str:
+    """Build the retry instruction, surfacing the concrete validation error.
+
+    The first version only said "reply with valid JSON", which is useless when
+    the failure is a *content* violation (e.g. a title over the length cap) on
+    otherwise-valid JSON — the model has no signal about what to change. We now
+    relay the actual parser/validation error so the model can self-correct.
+    """
+    suffix = (
+        "\n\nIMPORTANT: Your previous response could not be parsed into the required "
+        "schema. Reply with strict JSON only — no prose, no markdown fences — and make "
+        "every field satisfy the schema (in particular keep each finding 'title' to 80 "
+        "characters or fewer)."
+    )
+    detail = " ".join(last_error.split())
+    if detail:
+        if len(detail) > _RETRY_ERROR_BUDGET:
+            detail = f"{detail[:_RETRY_ERROR_BUDGET]} …"
+        suffix += f"\n\nValidation error from your previous attempt: {detail}"
+    return suffix
 
 
 @dataclass(frozen=True, slots=True)
@@ -89,7 +108,7 @@ async def complete_typed_json[T: BaseModel](
             return TypedReviewerResult(
                 False, failure_reason="malformed_output", failure_message=last_error
             )
-        prompt = prompt + _RETRY_SUFFIX
+        prompt = user + _retry_suffix(last_error)
     return TypedReviewerResult(False, failure_reason="malformed_output")
 
 

packages/pythinker-review/src/pythinker_review/reviewers/prompts/code_review.system.md

@@ -49,7 +49,7 @@ Schema:
       "start_line": 1,
       "end_line": 1,
       "confidence": 0.0,
-      "evidence_snippet": "<code excerpt visible in the diff/context>",
+      "evidence_snippet": "<code copied VERBATIM from the diff/context — must match character-for-character; do not paraphrase, reformat, or add ellipses>",
       "confidence_reason": "<why this confidence is justified>",
       "test_analysis": "<optional coverage assessment for the changed behavior>",
       "suggested_regression_test": "<optional focused test to add>",

packages/pythinker-review/src/pythinker_review/reviewers/prompts/debug_review.system.md

@@ -25,7 +25,7 @@ Schema:
       "start_line": 1,
       "end_line": 1,
       "confidence": 0.0,
-      "evidence_snippet": "<optional excerpt>",
+      "evidence_snippet": "<optional; if given, copy code VERBATIM from the diff/context — character-for-character, no paraphrase or ellipses>",
       "confidence_reason": "<optional why this is likely the root cause>",
       "reproduction": "<optional command/log evidence>",
       "test_analysis": "<optional failing/passing test interpretation>",

packages/pythinker-review/src/pythinker_review/reviewers/prompts/deslopify_review.system.md

@@ -10,6 +10,6 @@ Rules:
 - Output strict JSON only.
 
 Schema:
-{"findings":[{"rule_id":"deslopify.<dotted id>","title":"<≤80 chars>","rationale":"<markdown>","category":"readability|performance|test_coverage|api_design|correctness","severity":"medium|low|info","file":"<repo-relative POSIX path>","start_line":1,"end_line":1,"confidence":0.0,"evidence_snippet":"<optional code excerpt>","minimum_fix_scope":"<smallest deletion/consolidation/reuse scope>","test_analysis":"<why tests preserve or should cover this>","suggestion":{"summary":"<one sentence>","patch":"<optional unified diff>"}}]}
+{"findings":[{"rule_id":"deslopify.<dotted id>","title":"<≤80 chars>","rationale":"<markdown>","category":"readability|performance|test_coverage|api_design|correctness","severity":"medium|low|info","file":"<repo-relative POSIX path>","start_line":1,"end_line":1,"confidence":0.0,"evidence_snippet":"<optional; if given, copy code VERBATIM from the diff/context — character-for-character, no paraphrase or ellipses>","minimum_fix_scope":"<smallest deletion/consolidation/reuse scope>","test_analysis":"<why tests preserve or should cover this>","suggestion":{"summary":"<one sentence>","patch":"<optional unified diff>"}}]}
 
 If you find no issues, return {"findings": []}. Output JSON only, no prose.

packages/pythinker-review/src/pythinker_review/reviewers/prompts/security_review.system.md

@@ -32,7 +32,7 @@ Schema:
       "start_line": 1,
       "end_line": 1,
       "confidence": 0.0,
-      "evidence_snippet": "<optional code excerpt>",
+      "evidence_snippet": "<optional; if given, copy code VERBATIM from the diff/context — character-for-character, no paraphrase or ellipses>",
       "confidence_reason": "<optional validation reasoning>",
       "exploitability": "<optional attacker path and preconditions>",
       "minimum_fix_scope": "<optional smallest safe mitigation scope>",

packages/pythinker-review/src/pythinker_review/reviewers/schema.py

@@ -4,16 +4,18 @@
 
 from typing import Self
 
-from pydantic import BaseModel, ConfigDict, Field, model_validator
+from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator
 
 from pythinker_review.store.models import Category, Severity, Suggestion
 
+_MAX_TITLE_LEN = 80
+
 
 class RawFinding(BaseModel):
     model_config = ConfigDict(extra="forbid")
 
     rule_id: str
-    title: str = Field(max_length=80)
+    title: str
     rationale: str
     category: Category
     severity: Severity
@@ -30,6 +32,16 @@ class RawFinding(BaseModel):
     suggested_regression_test: str | None = None
     minimum_fix_scope: str | None = None
 
+    @field_validator("title", mode="before")
+    @classmethod
+    def _truncate_title(cls, value: object) -> object:
+        # Models (especially smaller ones) routinely exceed the title budget.
+        # Truncate rather than hard-fail: a length violation used to fail the
+        # whole ReviewerOutput parse, discarding *every* finding in the chunk.
+        if isinstance(value, str) and len(value) > _MAX_TITLE_LEN:
+            return value[: _MAX_TITLE_LEN - 1].rstrip() + "…"
+        return value
+
     @model_validator(mode="after")
     def validate_range(self) -> Self:
         if self.end_line < self.start_line:

packages/pythinker-review/tests/unit/test_reviewers.py

@@ -87,6 +87,48 @@ async def test_security_review_retries_once_on_malformed_then_succeeds() -> None
     assert len(llm.calls) == 2
 
 
+@pytest.mark.asyncio
+async def test_retry_prompt_surfaces_previous_validation_error() -> None:
+    # The retry must relay the concrete parser error so the model can
+    # self-correct, not just repeat a generic "reply with valid JSON".
+    llm = FakeReviewLLM(scripted=["not valid json at all", '{"findings": []}'])
+    result = await run_code_review_pass(chunk=_chunk(), llm=llm, timeout_s=10.0)
+    assert result.ok
+    assert len(llm.calls) == 2
+    retry_prompt = llm.calls[1][1]
+    assert "Validation error from your previous attempt" in retry_prompt
+    assert retry_prompt != llm.calls[0][1]
+
+
+@pytest.mark.asyncio
+async def test_overlong_title_is_truncated_not_dropped() -> None:
+    # A single finding with an over-long title used to fail the whole chunk.
+    # It must now survive (truncated) rather than discard sibling findings.
+    payload = json.dumps(
+        {
+            "findings": [
+                {
+                    "rule_id": "review.x",
+                    "title": "T" * 200,
+                    "rationale": "...",
+                    "category": "correctness",
+                    "severity": "low",
+                    "file": "x.py",
+                    "start_line": 1,
+                    "end_line": 1,
+                    "confidence": 0.6,
+                }
+            ]
+        }
+    )
+    llm = FakeReviewLLM(scripted=[payload])
+    result = await run_code_review_pass(chunk=_chunk(), llm=llm, timeout_s=10.0)
+    assert result.ok
+    assert len(result.findings) == 1
+    assert len(result.findings[0].title) == 80
+    assert len(llm.calls) == 1  # parsed on the first attempt, no retry needed
+
+
 @pytest.mark.asyncio
 async def test_reviewer_accepts_json_inside_markdown_fence() -> None:
     llm = FakeReviewLLM(scripted=['```json\n{"findings": []}\n```'])

packages/pythinker-review/tests/unit/test_schema.py

@@ -28,6 +28,46 @@ def test_reviewer_output_parses_minimal_payload() -> None:
     assert out.findings[0].severity is Severity.medium
 
 
+def test_reviewer_output_truncates_overlong_title() -> None:
+    # An over-long title must not fail the whole parse (which would drop every
+    # finding in the chunk); it is truncated to the budget instead.
+    out = ReviewerOutput.model_validate(
+        {
+            "findings": [
+                {
+                    "rule_id": "r",
+                    "title": "T" * 200,
+                    "rationale": "...",
+                    "category": "correctness",
+                    "severity": "low",
+                    "file": "a.py",
+                    "start_line": 1,
+                    "end_line": 1,
+                    "confidence": 0.5,
+                }
+            ]
+        }
+    )
+    title = out.findings[0].title
+    assert len(title) == 80
+    assert title.endswith("…")
+
+
+def test_reviewer_output_keeps_short_title_unchanged() -> None:
+    finding = RawFinding(
+        rule_id="r",
+        title="Short title",
+        rationale="r",
+        category=Category.correctness,
+        severity=Severity.low,
+        file="a",
+        start_line=1,
+        end_line=1,
+        confidence=0.5,
+    )
+    assert finding.title == "Short title"
+
+
 def test_reviewer_output_rejects_lines_under_one() -> None:
     with pytest.raises(ValidationError):
         RawFinding(

src/pythinker_code/agents/default/code_reviewer.yaml

@@ -51,6 +51,7 @@ agent:
     Use to run a read-only diff-focused code review or code-reviewr-derived PR artifact workflow on the current branch.
   allowed_tools:
     - "pythinker_code.tools.shell:Shell"
+    - "pythinker_code.tools.todo:SetTodoList"
     - "pythinker_code.tools.file:ReadFile"
     - "pythinker_code.tools.file:Grep"
     - "pythinker_code.tools.skill:ReadSkill"