Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .coderabbit.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ reviews:
pre_merge_checks:
title:
mode: "warning"
requirements: "Follow conventional commits: type(scope)?: description. Valid types: feat, fix, chore, docs, refactor, test, ci, perf."
requirements: "Follow conventional commits: type(scope)?: description. Valid types: feat, fix, test, refactor, chore, style, docs, perf, build, ci, revert."
description:
mode: "warning"
issue_assessment:
Expand Down
5 changes: 5 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
12 changes: 8 additions & 4 deletions packages/homebrew-tap/generate-formula.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,15 @@ def asset_name(self, version: str) -> str:
return self.asset_name_template.format(version=version)


# Use the onedir PyInstaller artifacts: a onefile binary re-extracts its full
# ~70MB payload to a temp dir on every launch, which makes macOS cold starts
# take 10+ seconds (Gatekeeper re-validates every extracted file each run) and
# is fragile. The onedir build extracts nothing at runtime.
NATIVE_TARGETS = (
NativeTarget("MACOS_ARM", "pythinker-{version}-aarch64-apple-darwin.tar.gz"),
NativeTarget("MACOS_INTEL", "pythinker-{version}-x86_64-apple-darwin.tar.gz"),
NativeTarget("LINUX_ARM", "pythinker-{version}-aarch64-unknown-linux-gnu.tar.gz"),
NativeTarget("LINUX_X86_64", "pythinker-{version}-x86_64-unknown-linux-gnu.tar.gz"),
NativeTarget("MACOS_ARM", "pythinker-{version}-aarch64-apple-darwin-onedir.tar.gz"),
NativeTarget("MACOS_INTEL", "pythinker-{version}-x86_64-apple-darwin-onedir.tar.gz"),
NativeTarget("LINUX_ARM", "pythinker-{version}-aarch64-unknown-linux-gnu-onedir.tar.gz"),
NativeTarget("LINUX_X86_64", "pythinker-{version}-x86_64-unknown-linux-gnu-onedir.tar.gz"),
)


Expand Down
7 changes: 6 additions & 1 deletion packages/homebrew-tap/pythinker-code.rb.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,12 @@ class PythinkerCode < Formula
end

def install
libexec.install "pythinker"
# Onedir PyInstaller build: a "pythinker" launcher next to an "_internal"
# directory. Homebrew chdirs into the tarball's single "pythinker/" root,
# so Dir["*"] is the launcher plus _internal. Install the whole tree into
# libexec and put an exec wrapper on PATH so the launcher resolves
# _internal next to its real location.
libexec.install Dir["*"]
(libexec/".pythinker-native").write "pythinker-native-build\n"
bin.write_exec_script libexec/"pythinker"
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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")


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Original file line number Diff line number Diff line change
Expand Up @@ -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>",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
Expand Down
42 changes: 42 additions & 0 deletions packages/pythinker-review/tests/unit/test_reviewers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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```'])
Expand Down
40 changes: 40 additions & 0 deletions packages/pythinker-review/tests/unit/test_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 2 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -175,5 +175,7 @@ reviewr = "reviewr"
fnd = "fnd"
edn = "edn"
Encrypter = "Encrypter"
# Hex session IDs (e.g. 06ba6c38) contain "ba".
ba = "ba"
uest = "uest"

Comment thread
coderabbitai[bot] marked this conversation as resolved.
1 change: 1 addition & 0 deletions src/pythinker_code/agents/default/code_reviewer.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion src/pythinker_code/agents/default/coder.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ agent:
Use this agent for non-trivial software engineering work that may require reading files, editing code, running commands, and returning a compact but technically complete summary to the parent agent.
allowed_tools:
- "pythinker_code.tools.shell:Shell"
- "pythinker_code.tools.todo:SetTodoList"
- "pythinker_code.tools.file:ReadFile"
- "pythinker_code.tools.file:ReadMediaFile"
- "pythinker_code.tools.file:Glob"
Expand All @@ -47,7 +48,6 @@ agent:
exclude_tools:
- "pythinker_code.tools.agent:Agent"
- "pythinker_code.tools.ask_user:AskUserQuestion"
- "pythinker_code.tools.todo:SetTodoList"
- "pythinker_code.tools.plan:ExitPlanMode"
- "pythinker_code.tools.plan.enter:EnterPlanMode"
subagents:
1 change: 1 addition & 0 deletions src/pythinker_code/agents/default/debugger.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ agent:
Use for failing tests, stack traces, runtime errors, flaky failures, or debugging requests where root cause should be found before editing code.
allowed_tools:
- "pythinker_code.tools.shell:Shell"
- "pythinker_code.tools.todo:SetTodoList"
- "pythinker_code.tools.file:ReadFile"
- "pythinker_code.tools.file:Grep"
exclude_tools:
Expand Down
2 changes: 1 addition & 1 deletion src/pythinker_code/agents/default/explore.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ agent:
Fast agent specialized for exploring codebases. Use this when you need to quickly find files by patterns (e.g. "src/**/*.yaml"), search code for keywords (e.g. "database connection"), or answer questions about the codebase (e.g. "how does the auth module work?"). When calling this agent, specify the desired thoroughness level: "quick" for basic searches, "medium" for moderate exploration, or "thorough" for comprehensive analysis across multiple locations and naming conventions. Use this agent for any read-only exploration that will clearly require more than 3 tool calls. Prefer launching multiple explore agents concurrently when investigating independent questions.
allowed_tools:
- "pythinker_code.tools.shell:Shell"
- "pythinker_code.tools.todo:SetTodoList"
- "pythinker_code.tools.file:ReadFile"
- "pythinker_code.tools.file:ReadMediaFile"
- "pythinker_code.tools.file:Glob"
Expand All @@ -61,7 +62,6 @@ agent:
exclude_tools:
- "pythinker_code.tools.agent:Agent"
- "pythinker_code.tools.ask_user:AskUserQuestion"
- "pythinker_code.tools.todo:SetTodoList"
- "pythinker_code.tools.plan:ExitPlanMode"
- "pythinker_code.tools.plan.enter:EnterPlanMode"
- "pythinker_code.tools.file:WriteFile"
Expand Down
2 changes: 1 addition & 1 deletion src/pythinker_code/agents/default/implementer.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ agent:
Use this agent when the required code change is already specified and should be implemented with minimal edits and a quick verification pass.
allowed_tools:
- "pythinker_code.tools.shell:Shell"
- "pythinker_code.tools.todo:SetTodoList"
- "pythinker_code.tools.file:ReadFile"
- "pythinker_code.tools.file:ReadMediaFile"
- "pythinker_code.tools.file:Glob"
Expand All @@ -47,7 +48,6 @@ agent:
exclude_tools:
- "pythinker_code.tools.agent:Agent"
- "pythinker_code.tools.ask_user:AskUserQuestion"
- "pythinker_code.tools.todo:SetTodoList"
- "pythinker_code.tools.plan:ExitPlanMode"
- "pythinker_code.tools.plan.enter:EnterPlanMode"
subagents:
2 changes: 1 addition & 1 deletion src/pythinker_code/agents/default/plan.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ agent:
when_to_use: |
Use this agent when the parent agent needs a step-by-step implementation plan, key file identification, and architectural trade-off analysis before code changes are made.
allowed_tools:
- "pythinker_code.tools.todo:SetTodoList"
- "pythinker_code.tools.file:ReadFile"
- "pythinker_code.tools.file:ReadMediaFile"
- "pythinker_code.tools.file:Glob"
Expand All @@ -62,7 +63,6 @@ agent:
exclude_tools:
- "pythinker_code.tools.agent:Agent"
- "pythinker_code.tools.ask_user:AskUserQuestion"
- "pythinker_code.tools.todo:SetTodoList"
- "pythinker_code.tools.plan:ExitPlanMode"
- "pythinker_code.tools.plan.enter:EnterPlanMode"
- "pythinker_code.tools.shell:Shell"
Expand Down
2 changes: 1 addition & 1 deletion src/pythinker_code/agents/default/review.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ agent:
Use this agent for read-only code review after changes are made or when the parent needs severity-scored findings before deciding what to fix.
allowed_tools:
- "pythinker_code.tools.shell:Shell"
- "pythinker_code.tools.todo:SetTodoList"
- "pythinker_code.tools.file:ReadFile"
- "pythinker_code.tools.file:ReadMediaFile"
- "pythinker_code.tools.file:Glob"
Expand All @@ -50,7 +51,6 @@ agent:
exclude_tools:
- "pythinker_code.tools.agent:Agent"
- "pythinker_code.tools.ask_user:AskUserQuestion"
- "pythinker_code.tools.todo:SetTodoList"
- "pythinker_code.tools.plan:ExitPlanMode"
- "pythinker_code.tools.plan.enter:EnterPlanMode"
- "pythinker_code.tools.file:WriteFile"
Expand Down
1 change: 1 addition & 0 deletions src/pythinker_code/agents/default/security_reviewer.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ agent:
Use to run a diff-only security review on the current branch. Can run in parallel with `code-reviewer`.
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.web:SearchWeb"
Expand Down
Loading
Loading