diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 864688ed..bc0b1b4f 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -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: diff --git a/AGENTS.md b/AGENTS.md index b1dae0af..98492b77 100644 --- a/AGENTS.md +++ b/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 diff --git a/packages/homebrew-tap/generate-formula.py b/packages/homebrew-tap/generate-formula.py index 15331f1f..2bb077ea 100644 --- a/packages/homebrew-tap/generate-formula.py +++ b/packages/homebrew-tap/generate-formula.py @@ -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"), ) diff --git a/packages/homebrew-tap/pythinker-code.rb.tmpl b/packages/homebrew-tap/pythinker-code.rb.tmpl index d2ef7f88..56214017 100644 --- a/packages/homebrew-tap/pythinker-code.rb.tmpl +++ b/packages/homebrew-tap/pythinker-code.rb.tmpl @@ -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 diff --git a/packages/pythinker-review/src/pythinker_review/reviewers/common.py b/packages/pythinker-review/src/pythinker_review/reviewers/common.py index c1a2c38f..1dd6386b 100644 --- a/packages/pythinker-review/src/pythinker_review/reviewers/common.py +++ b/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") diff --git a/packages/pythinker-review/src/pythinker_review/reviewers/prompts/code_review.system.md b/packages/pythinker-review/src/pythinker_review/reviewers/prompts/code_review.system.md index 8c1b7004..13a8e94c 100644 --- a/packages/pythinker-review/src/pythinker_review/reviewers/prompts/code_review.system.md +++ b/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": "", + "evidence_snippet": "", "confidence_reason": "", "test_analysis": "", "suggested_regression_test": "", diff --git a/packages/pythinker-review/src/pythinker_review/reviewers/prompts/debug_review.system.md b/packages/pythinker-review/src/pythinker_review/reviewers/prompts/debug_review.system.md index d7dc7bf1..81147f4a 100644 --- a/packages/pythinker-review/src/pythinker_review/reviewers/prompts/debug_review.system.md +++ b/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": "", + "evidence_snippet": "", "confidence_reason": "", "reproduction": "", "test_analysis": "", diff --git a/packages/pythinker-review/src/pythinker_review/reviewers/prompts/deslopify_review.system.md b/packages/pythinker-review/src/pythinker_review/reviewers/prompts/deslopify_review.system.md index 355e7f49..ba903983 100644 --- a/packages/pythinker-review/src/pythinker_review/reviewers/prompts/deslopify_review.system.md +++ b/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.","title":"<≤80 chars>","rationale":"","category":"readability|performance|test_coverage|api_design|correctness","severity":"medium|low|info","file":"","start_line":1,"end_line":1,"confidence":0.0,"evidence_snippet":"","minimum_fix_scope":"","test_analysis":"","suggestion":{"summary":"","patch":""}}]} +{"findings":[{"rule_id":"deslopify.","title":"<≤80 chars>","rationale":"","category":"readability|performance|test_coverage|api_design|correctness","severity":"medium|low|info","file":"","start_line":1,"end_line":1,"confidence":0.0,"evidence_snippet":"","minimum_fix_scope":"","test_analysis":"","suggestion":{"summary":"","patch":""}}]} If you find no issues, return {"findings": []}. Output JSON only, no prose. diff --git a/packages/pythinker-review/src/pythinker_review/reviewers/prompts/security_review.system.md b/packages/pythinker-review/src/pythinker_review/reviewers/prompts/security_review.system.md index 651caa0d..877e2eed 100644 --- a/packages/pythinker-review/src/pythinker_review/reviewers/prompts/security_review.system.md +++ b/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": "", + "evidence_snippet": "", "confidence_reason": "", "exploitability": "", "minimum_fix_scope": "", diff --git a/packages/pythinker-review/src/pythinker_review/reviewers/schema.py b/packages/pythinker-review/src/pythinker_review/reviewers/schema.py index 42838c9b..a43cbbc4 100644 --- a/packages/pythinker-review/src/pythinker_review/reviewers/schema.py +++ b/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: diff --git a/packages/pythinker-review/tests/unit/test_reviewers.py b/packages/pythinker-review/tests/unit/test_reviewers.py index 6f91bc15..df346b2e 100644 --- a/packages/pythinker-review/tests/unit/test_reviewers.py +++ b/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```']) diff --git a/packages/pythinker-review/tests/unit/test_schema.py b/packages/pythinker-review/tests/unit/test_schema.py index 719b0551..f0310600 100644 --- a/packages/pythinker-review/tests/unit/test_schema.py +++ b/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( diff --git a/pyproject.toml b/pyproject.toml index e1a0fdcf..9c901108 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -175,5 +175,7 @@ reviewr = "reviewr" fnd = "fnd" edn = "edn" Encrypter = "Encrypter" +# Hex session IDs (e.g. 06ba6c38) contain "ba". +ba = "ba" uest = "uest" diff --git a/src/pythinker_code/agents/default/code_reviewer.yaml b/src/pythinker_code/agents/default/code_reviewer.yaml index e62deaf2..54e276ef 100644 --- a/src/pythinker_code/agents/default/code_reviewer.yaml +++ b/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" diff --git a/src/pythinker_code/agents/default/coder.yaml b/src/pythinker_code/agents/default/coder.yaml index e344cb05..5a43404a 100644 --- a/src/pythinker_code/agents/default/coder.yaml +++ b/src/pythinker_code/agents/default/coder.yaml @@ -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" @@ -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: diff --git a/src/pythinker_code/agents/default/debugger.yaml b/src/pythinker_code/agents/default/debugger.yaml index f824bb33..bd620d85 100644 --- a/src/pythinker_code/agents/default/debugger.yaml +++ b/src/pythinker_code/agents/default/debugger.yaml @@ -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: diff --git a/src/pythinker_code/agents/default/explore.yaml b/src/pythinker_code/agents/default/explore.yaml index a34ec95f..2344eaa5 100644 --- a/src/pythinker_code/agents/default/explore.yaml +++ b/src/pythinker_code/agents/default/explore.yaml @@ -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" @@ -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" diff --git a/src/pythinker_code/agents/default/implementer.yaml b/src/pythinker_code/agents/default/implementer.yaml index da5b65d7..91abe78b 100644 --- a/src/pythinker_code/agents/default/implementer.yaml +++ b/src/pythinker_code/agents/default/implementer.yaml @@ -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" @@ -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: diff --git a/src/pythinker_code/agents/default/plan.yaml b/src/pythinker_code/agents/default/plan.yaml index 967e51c5..d7cc8ed0 100644 --- a/src/pythinker_code/agents/default/plan.yaml +++ b/src/pythinker_code/agents/default/plan.yaml @@ -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" @@ -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" diff --git a/src/pythinker_code/agents/default/review.yaml b/src/pythinker_code/agents/default/review.yaml index a9eb233d..c0d7ba27 100644 --- a/src/pythinker_code/agents/default/review.yaml +++ b/src/pythinker_code/agents/default/review.yaml @@ -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" @@ -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" diff --git a/src/pythinker_code/agents/default/security_reviewer.yaml b/src/pythinker_code/agents/default/security_reviewer.yaml index ec93ec46..1d788df2 100644 --- a/src/pythinker_code/agents/default/security_reviewer.yaml +++ b/src/pythinker_code/agents/default/security_reviewer.yaml @@ -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" diff --git a/src/pythinker_code/agents/default/system.md b/src/pythinker_code/agents/default/system.md index 26869b4c..212e0fd3 100644 --- a/src/pythinker_code/agents/default/system.md +++ b/src/pythinker_code/agents/default/system.md @@ -39,6 +39,21 @@ For any codebase, architecture, debugging, security, performance, planning, or " **Professional handoff format:** For substantial tasks, keep a visible plan/todo and structure work as `context -> assessment -> plan -> execution -> verification -> residual risks`. Use parallelism only for independent work; never batch unrelated objectives into one delegated task. +**Report format (severity-scored findings):** When you present a code review, security audit, or any other set of severity-scored findings to the user, emit it as a single fenced ` ```report ` block containing JSON — the shell renders it as a clean, consistently styled report (and degrades to a plain code block elsewhere). Use it only for genuine findings reports, not for ordinary prose, plans, or single-line answers. Schema: + +```report +{ + "title": "Code Review Results", + "scope": "one-line context, e.g. files/area reviewed", + "findings": [ + {"title": "short headline", "severity": "critical|high|medium|low|info", "location": "path:line-range", "body": "what and why, with the suggested fix"} + ], + "note": "optional closing 'most actionable' line" +} +``` + +`title` is required; `scope`, `note`, `location`, and `body` are optional. `severity` must be one of the five listed values. Order does not matter — the renderer groups by severity (critical first) and derives the summary tally. Put narrative prose outside the block, before or after it. + # Engineering Discipline These principles govern every engineering response. They override speed: a slow right answer beats a fast wrong one. diff --git a/src/pythinker_code/agents/default/verifier.yaml b/src/pythinker_code/agents/default/verifier.yaml index 310aa7ff..2a73cf40 100644 --- a/src/pythinker_code/agents/default/verifier.yaml +++ b/src/pythinker_code/agents/default/verifier.yaml @@ -37,6 +37,7 @@ agent: Use this agent when the parent needs tests, lint, type checks, builds, or other validation gates run and reported without applying fixes. 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" @@ -46,7 +47,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" diff --git a/src/pythinker_code/auth/opencode_go.py b/src/pythinker_code/auth/opencode_go.py index 1fee1ac3..50bdfcf6 100644 --- a/src/pythinker_code/auth/opencode_go.py +++ b/src/pythinker_code/auth/opencode_go.py @@ -13,10 +13,26 @@ from pythinker_code.config import Config, LLMModel, LLMProvider, save_config from pythinker_code.utils.aiohttp import new_client_session +# OpenAI-compatible base: the OpenAI SDK appends "/chat/completions" (and the +# "/models" discovery path), so it includes the "/v1" segment. OPENCODE_GO_BASE_URL = "https://opencode.ai/zen/go/v1" +# Anthropic-compatible base: the Anthropic SDK appends "/v1/messages" itself, so +# this base must NOT include "/v1" or requests 404 at ".../go/v1/v1/messages". +OPENCODE_GO_ANTHROPIC_BASE_URL = "https://opencode.ai/zen/go" OPENCODE_GO_OPENAI_PROVIDER_KEY = "managed:opencode-go-openai" OPENCODE_GO_ANTHROPIC_PROVIDER_KEY = "managed:opencode-go-anthropic" OPENCODE_GO_DEFAULT_MODEL_ALIAS = "opencode-go/kimi-k2.6" +OPENCODE_GO_DEFAULT_CONTEXT = 262_000 + +# models.dev is OpenCode's own source of truth for model metadata (context +# window, display name). The Go /models endpoint returns ids only, so we +# enrich ids not in the curated catalog below from this catalog. +MODELS_DEV_API_URL = "https://models.dev/api.json" +MODELS_DEV_PROVIDER_ID = "opencode-go" +# The models.dev fetch is best-effort enrichment, so it must not stall login on +# the 120s default. A tight cap means a slow/partial endpoint degrades quickly +# to the curated catalog instead of holding the user for up to two minutes. +MODELS_DEV_TIMEOUT = aiohttp.ClientTimeout(total=15, sock_connect=8, sock_read=10) @dataclass(frozen=True, slots=True) @@ -42,8 +58,12 @@ def alias(self) -> str: OpenCodeGoModel("mimo-v2-omni", "MiMo-V2-Omni", OPENCODE_GO_OPENAI_PROVIDER_KEY), OpenCodeGoModel("mimo-v2.5-pro", "MiMo-V2.5-Pro", OPENCODE_GO_OPENAI_PROVIDER_KEY, 1_000_000), OpenCodeGoModel("mimo-v2.5", "MiMo-V2.5", OPENCODE_GO_OPENAI_PROVIDER_KEY, 1_000_000), - OpenCodeGoModel("qwen3.5-plus", "Qwen3.5 Plus", OPENCODE_GO_OPENAI_PROVIDER_KEY, 262_000), - OpenCodeGoModel("qwen3.6-plus", "Qwen3.6 Plus", OPENCODE_GO_OPENAI_PROVIDER_KEY, 262_000), + # Qwen models speak the Anthropic-shaped endpoint (models.dev routes them + # via @ai-sdk/anthropic); the OpenAI-shaped endpoint rejects them with + # "not supported for format oa-compat". + OpenCodeGoModel("qwen3.5-plus", "Qwen3.5 Plus", OPENCODE_GO_ANTHROPIC_PROVIDER_KEY, 262_000), + OpenCodeGoModel("qwen3.6-plus", "Qwen3.6 Plus", OPENCODE_GO_ANTHROPIC_PROVIDER_KEY, 262_000), + OpenCodeGoModel("qwen3.7-max", "Qwen3.7 Max", OPENCODE_GO_ANTHROPIC_PROVIDER_KEY, 1_000_000), OpenCodeGoModel("minimax-m2.5", "MiniMax M2.5", OPENCODE_GO_ANTHROPIC_PROVIDER_KEY, 205_000), OpenCodeGoModel("minimax-m2.7", "MiniMax M2.7", OPENCODE_GO_ANTHROPIC_PROVIDER_KEY, 205_000), ) @@ -69,7 +89,7 @@ def _apply_opencode_go_config( ) config.providers[OPENCODE_GO_ANTHROPIC_PROVIDER_KEY] = LLMProvider( type="anthropic", - base_url=OPENCODE_GO_BASE_URL, + base_url=OPENCODE_GO_ANTHROPIC_BASE_URL, api_key=api_key, ) @@ -98,38 +118,163 @@ def _model_by_id() -> dict[str, OpenCodeGoModel]: return {model.model_id: model for model in OPENCODE_GO_MODELS} -def _parse_discovered_models(data: object) -> tuple[OpenCodeGoModel, ...]: +@dataclass(frozen=True, slots=True) +class _ModelsDevMeta: + """The slice of models.dev metadata we consume for a single model id. + + ``is_anthropic`` is the authoritative API-shape signal: models.dev marks + Anthropic-shaped models with ``provider.npm == "@ai-sdk/anthropic"`` and + leaves the rest on the provider default (``@ai-sdk/openai-compatible``). + ``None`` means models.dev had no entry to derive a shape from. + """ + + display_name: str | None + max_context: int | None + is_anthropic: bool | None + + +MODELS_DEV_ANTHROPIC_NPM = "@ai-sdk/anthropic" + + +def _heuristic_provider_key(model_id: str) -> str: + """Last-ditch shape guess when models.dev and the catalog are both silent. + + OpenAI-compatible is the safe default (most Go models use it); only the + stable ``minimax-`` family is reliably Anthropic-shaped by name. + """ + if model_id.startswith("minimax-"): + return OPENCODE_GO_ANTHROPIC_PROVIDER_KEY + return OPENCODE_GO_OPENAI_PROVIDER_KEY + + +def _resolve_provider_key( + model_id: str, meta: _ModelsDevMeta | None, catalog: OpenCodeGoModel | None +) -> str: + """Pick the provider (API shape) for a model. + + models.dev is authoritative; the curated catalog is the offline fallback; + the name heuristic is the last resort. This is the fix for Qwen models + being rejected as ``not supported for format oa-compat`` — they are + Anthropic-shaped, which only models.dev (or the corrected catalog) knows. + """ + if meta is not None and meta.is_anthropic is not None: + return ( + OPENCODE_GO_ANTHROPIC_PROVIDER_KEY + if meta.is_anthropic + else (OPENCODE_GO_OPENAI_PROVIDER_KEY) + ) + if catalog is not None: + return catalog.provider_key + return _heuristic_provider_key(model_id) + + +def _derive_display_name(model_id: str) -> str: + return model_id.replace("-", " ").title() + + +def _extract_model_ids(data: object) -> list[str]: + """Pull the ordered list of model ids from the /models payload.""" if not isinstance(data, dict): - return () - payload = cast(dict[str, Any], data) - raw_items = payload.get("data") + return [] + raw_items = cast(dict[str, Any], data).get("data") if not isinstance(raw_items, list): - return () + return [] + ids: list[str] = [] + for item in cast(list[Any], raw_items): + if not isinstance(item, dict): + continue + model_id = cast(dict[str, Any], item).get("id") + if isinstance(model_id, str) and model_id: + ids.append(model_id) + return ids + + +def _parse_models_dev_metadata(data: object) -> dict[str, _ModelsDevMeta]: + """Extract display name, context, and API shape per opencode-go model id.""" + if not isinstance(data, dict): + return {} + provider = cast(dict[str, Any], data).get(MODELS_DEV_PROVIDER_ID) + if not isinstance(provider, dict): + return {} + models = cast(dict[str, Any], provider).get("models") + if not isinstance(models, dict): + return {} + default_npm = cast(dict[str, Any], provider).get("npm") + result: dict[str, _ModelsDevMeta] = {} + for model_id, entry in cast(dict[str, Any], models).items(): + if not isinstance(entry, dict): + continue + entry_d = cast(dict[str, Any], entry) + name = entry_d.get("name") + display_name = name if isinstance(name, str) and name else None + limit = entry_d.get("limit") + context = cast(dict[str, Any], limit).get("context") if isinstance(limit, dict) else None + max_context = context if isinstance(context, int) and context > 0 else None + model_provider = entry_d.get("provider") + npm = ( + cast(dict[str, Any], model_provider).get("npm") + if isinstance(model_provider, dict) + else None + ) + effective_npm = npm or default_npm + is_anthropic = ( + effective_npm == MODELS_DEV_ANTHROPIC_NPM if isinstance(effective_npm, str) else None + ) + result[model_id] = _ModelsDevMeta(display_name, max_context, is_anthropic) + return result + +def _build_models( + model_ids: list[str], + metadata: dict[str, _ModelsDevMeta], +) -> tuple[OpenCodeGoModel, ...]: + """Turn discovered ids into models. + + The /models list is authoritative for *which* models exist. For each id, + properties resolve in order: models.dev → curated catalog → default. This + lets the live path self-correct shape/context even if the catalog drifts. + """ known = _model_by_id() result: list[OpenCodeGoModel] = [] - for item in cast(list[dict[str, Any]], raw_items): - model_id = item.get("id") - if not isinstance(model_id, str) or model_id not in known: - continue - current = known[model_id] - context_length = item.get("context_length") - max_context_size = current.max_context_size - if isinstance(context_length, int) and context_length > 0: - max_context_size = context_length - display_name_raw = item.get("display_name") - display_name = str(display_name_raw) if display_name_raw else current.display_name + for model_id in model_ids: + catalog = known.get(model_id) + meta = metadata.get(model_id) + display_name = ( + (meta.display_name if meta else None) + or (catalog.display_name if catalog else None) + or _derive_display_name(model_id) + ) + max_context = ( + (meta.max_context if meta else None) + or (catalog.max_context_size if catalog else None) + or OPENCODE_GO_DEFAULT_CONTEXT + ) result.append( OpenCodeGoModel( - current.model_id, + model_id, display_name, - current.provider_key, - max_context_size, + _resolve_provider_key(model_id, meta, catalog), + max_context, ) ) return tuple(result) +async def _fetch_models_dev_metadata() -> dict[str, _ModelsDevMeta]: + """Best-effort metadata fetch. Returns {} on any failure so login still + succeeds (falling back to the curated catalog) when models.dev is + unreachable.""" + try: + async with ( + new_client_session(timeout=MODELS_DEV_TIMEOUT) as session, + session.get(MODELS_DEV_API_URL, raise_for_status=True) as response, + ): + payload = await response.json(content_type=None) + except (TimeoutError, aiohttp.ClientError, ValueError): + return {} + return _parse_models_dev_metadata(payload) + + async def _discover_opencode_go_models(api_key: str) -> tuple[OpenCodeGoModel, ...]: async with ( new_client_session() as session, @@ -140,7 +285,16 @@ async def _discover_opencode_go_models(api_key: str) -> tuple[OpenCodeGoModel, . ) as response, ): payload = await response.json(content_type=None) - return _parse_discovered_models(payload) + + model_ids = _extract_model_ids(payload) + if not model_ids: + return () + + # models.dev is the authority for API shape + context; fetch it on every + # login (best-effort) so the live list self-corrects even when our curated + # catalog drifts. Falls back to the catalog when models.dev is unreachable. + metadata = await _fetch_models_dev_metadata() + return _build_models(model_ids, metadata) async def login_opencode_go_api_key( diff --git a/src/pythinker_code/cli/__init__.py b/src/pythinker_code/cli/__init__.py index ad2b4925..95e401d7 100644 --- a/src/pythinker_code/cli/__init__.py +++ b/src/pythinker_code/cli/__init__.py @@ -778,6 +778,10 @@ async def _run(session_id: str | None, prefill_text: str | None = None) -> tuple "scratch: named per-session history retained", ], create=True, + # Idempotent: relaunching the same session must not append a + # second identical "session start" milestone (a genuine + # startup→resume transition has a different source/signature). + dedup_signature=f"source: {session_source}", ) scratchpad_section = render_scratchpad_section( scratchpad_status, diff --git a/src/pythinker_code/scratchpad.py b/src/pythinker_code/scratchpad.py index c2a81e5e..3c1d3176 100644 --- a/src/pythinker_code/scratchpad.py +++ b/src/pythinker_code/scratchpad.py @@ -523,16 +523,51 @@ def _session_marker(session_id: str) -> str: return f"" -def _normalize_labels(labels: Sequence[object] | None, *, short_id: str) -> list[str]: - raw = (f"session:{short_id}", *(labels or ())) +# Label keys that carry a single current value (latest wins). Everything else +# (e.g. ``kind``) accumulates as a unique-value set: multiple kinds in one +# session is meaningful recall signal, two ``source`` values is just noise. +_SINGLE_VALUED_LABEL_KEYS = frozenset({"session", "workspace", "ui", "source", "scope"}) + + +def _label_key(label: str) -> str: + return label.split(":", 1)[0] + + +def _collapse_labels(labels: Sequence[str]) -> list[str]: + """Dedup labels by key, preserving first-seen key order. + + Single-valued keys keep their latest value; multi-valued keys keep all + unique values. Prevents duplicate-key noise like ``source:startup | + source:resume`` while retaining a meaningful ``kind:todo | kind:agent``. + """ + order: list[str] = [] + single: dict[str, str] = {} + multi: dict[str, list[str]] = {} + for label in labels: + key = _label_key(label) + if key not in order: + order.append(key) + if key in _SINGLE_VALUED_LABEL_KEYS: + single[key] = label + else: + bucket = multi.setdefault(key, []) + if label not in bucket: + bucket.append(label) result: list[str] = [] - for label in raw: - clean = _clean_event_text(label, max_len=80) - if clean and clean not in result: - result.append(clean) + for key in order: + if key in single: + result.append(single[key]) + else: + result.extend(multi[key]) return result +def _normalize_labels(labels: Sequence[object] | None, *, short_id: str) -> list[str]: + raw = (f"session:{short_id}", *(labels or ())) + clean = [c for c in (_clean_event_text(label, max_len=80) for label in raw) if c] + return _collapse_labels(clean) + + def _merge_session_labels(existing: str, marker: str, labels: Sequence[str]) -> str: if not labels or marker not in existing: return existing @@ -545,10 +580,7 @@ def _merge_session_labels(existing: str, marker: str, labels: Sequence[str]) -> if label_index < len(lines) and lines[label_index].startswith("labels:"): current = [part.strip() for part in lines[label_index][len("labels:") :].split("|")] current = [part for part in current if part] - merged = list(current) - for label in labels: - if label not in merged: - merged.append(label) + merged = _collapse_labels([*current, *labels]) label_line = f"labels: {' | '.join(merged)}" if label_index < len(lines) and lines[label_index].startswith("labels:"): lines[label_index] = label_line @@ -558,6 +590,20 @@ def _merge_session_labels(existing: str, marker: str, labels: Sequence[str]) -> return "\n".join(lines) + trailing_newline +def _session_block_text(text: str, marker: str) -> str: + """Return the lines belonging to one session block (marker → next session).""" + lines = text.splitlines() + start = next((i for i, line in enumerate(lines) if line.strip() == marker), -1) + if start == -1: + return "" + end = len(lines) + for j in range(start + 1, len(lines)): + if lines[j].startswith("## Session "): + end = j + break + return "\n".join(lines[start:end]) + + def _cap_scratch_text(text: str) -> str: if len(text.encode("utf-8")) <= _MAX_SCRATCH_BYTES: return text @@ -655,7 +701,9 @@ def _append_scratch_event_to_file( title: str, details: Sequence[object] | None = None, labels: Sequence[object] | None = None, -) -> None: + dedup_signature: str | None = None, +) -> bool: + """Append one milestone. Returns ``True`` if written, ``False`` if deduped.""" try: with path.open("r+", encoding="utf-8") as fh, _scratch_file_lock(fh): fh.seek(0) @@ -681,6 +729,11 @@ def _append_scratch_event_to_file( fh.write(merged_existing) fh.truncate() existing = merged_existing + # Idempotency guard: if this exact milestone already exists in + # the session block (e.g. a duplicate "session start" from a + # relaunch), keep any merged labels but skip the event line. + if dedup_signature and dedup_signature in _session_block_text(existing, marker): + return False timestamp = datetime.now().strftime("%Y-%m-%d %H:%M") clean_title = _clean_event_text(title, max_len=_MAX_EVENT_TITLE) @@ -697,6 +750,7 @@ def _append_scratch_event_to_file( fh.seek(0) fh.write(capped_text) fh.truncate() + return True except OSError as exc: if is_transient_oserror(exc): raise TransientScratchpadError(str(exc)) from exc @@ -712,8 +766,14 @@ def append_scratch_event_sync( session_id: str | None = None, session_title: str | None = None, create: bool = False, + dedup_signature: str | None = None, ) -> ScratchpadAppendResult: - """Append one compact milestone to a local scratchpad. Never raises.""" + """Append one compact milestone to a local scratchpad. Never raises. + + When *dedup_signature* is given and already present in the session block, + the milestone is suppressed (idempotent) and the result reason is + ``"deduped"`` instead of ``"appended"``. + """ if not _is_local_host(): return ScratchpadAppendResult(False, "remote_host") path = session_scratch_path( @@ -740,14 +800,15 @@ def append_scratch_event_sync( fh.write(_DEFAULT_SCRATCHPAD_FILE) if not path.is_file(): return ScratchpadAppendResult(False, "not_a_file") - _append_scratch_event_to_file( + appended = _append_scratch_event_to_file( path, session_id=session_id, title=title, details=details, labels=labels, + dedup_signature=dedup_signature, ) - return ScratchpadAppendResult(True, "appended") + return ScratchpadAppendResult(appended, "appended" if appended else "deduped") except FileExistsError: return append_scratch_event_sync( work_dir, @@ -756,6 +817,7 @@ def append_scratch_event_sync( labels=labels, session_id=session_id, session_title=session_title, + dedup_signature=dedup_signature, create=False, ) except TransientScratchpadError: diff --git a/src/pythinker_code/ui/print/visualize.py b/src/pythinker_code/ui/print/visualize.py index d2f28f55..923ca9fb 100644 --- a/src/pythinker_code/ui/print/visualize.py +++ b/src/pythinker_code/ui/print/visualize.py @@ -138,10 +138,24 @@ def flush(self) -> None: message = Message(role="assistant", content=self._content_buffer) text = message.extract_text() if text: - print(text, flush=True) + _print_final_text(text) self._content_buffer.clear() +def _print_final_text(text: str) -> None: + """Print the final assistant text, rendering any ` ```report ` block as a + clean report. Plain prose is printed verbatim so non-report output is + byte-identical (and pipe-safe — Rich drops colour on a non-TTY stdout).""" + from pythinker_code.ui.shell.components.report import has_report_block, render_agent_body + + if not has_report_block(text): + print(text, flush=True) + return + from rich.console import Console + + Console().print(render_agent_body(text)) + + class FinalOnlyJsonPrinter(Printer): def __init__(self) -> None: self._content_buffer: list[ContentPart] = [] diff --git a/src/pythinker_code/ui/shell/__init__.py b/src/pythinker_code/ui/shell/__init__.py index 6a1be1a9..ee7478b4 100644 --- a/src/pythinker_code/ui/shell/__init__.py +++ b/src/pythinker_code/ui/shell/__init__.py @@ -312,6 +312,27 @@ def _extract_429_detail(exc: BaseException) -> dict[str, str]: return {"summary": summary, "hint": hint} +def _is_insufficient_credits_error(exc: BaseException) -> bool: + """Detect an out-of-credits / billing failure. + + OpenCode Go (and similar gateways) return ``401`` with a + ``{"error": {"type": "CreditsError", "message": "Insufficient balance ..."}}`` + body when the account has run out of credits. That is a billing problem, not + a stale credential, so we must not tell the user to ``/login`` again. + """ + err_type = "" + message = "" + body = getattr(exc, "body", None) + if isinstance(body, dict): + err = cast(dict[str, object], body).get("error") + if isinstance(err, dict): + typed_err = cast(dict[str, object], err) + err_type = str(typed_err.get("type") or "") + message = str(typed_err.get("message") or "") + haystack = f"{err_type} {message} {exc}".lower() + return "creditserror" in haystack or "insufficient balance" in haystack + + class Shell: def __init__( self, @@ -1197,11 +1218,19 @@ def _on_view_ready(view: Any) -> None: _t = _get_tui_tokens() logger.exception("LLM provider error:") if isinstance(e, APIStatusError) and e.status_code == 401: - console.print( - f"[{_t.error}]Authorization failed. Your session may have expired.[/]\n" - "[dim]Type [bold]/login[/bold] to re-authenticate.[/dim]\n" - f"[dim]Server: {e}[/dim]" - ) + if _is_insufficient_credits_error(e): + console.print( + f"[{_t.error}]Insufficient balance — your account is out of credits.[/]\n" + "[dim]This is a billing issue, not a login problem. Top up or manage " + "billing (see the link in the server message below), then retry.[/dim]\n" + f"[dim]Server: {e}[/dim]" + ) + else: + console.print( + f"[{_t.error}]Authorization failed. Your session may have expired.[/]\n" + "[dim]Type [bold]/login[/bold] to re-authenticate.[/dim]\n" + f"[dim]Server: {e}[/dim]" + ) elif isinstance(e, APIStatusError) and e.status_code == 402: console.print( f"[{_t.error}]Membership expired, please renew your plan[/]\n" diff --git a/src/pythinker_code/ui/shell/components/messages.py b/src/pythinker_code/ui/shell/components/messages.py index c6da944d..da3a6500 100644 --- a/src/pythinker_code/ui/shell/components/messages.py +++ b/src/pythinker_code/ui/shell/components/messages.py @@ -24,6 +24,7 @@ from rich.text import Text from pythinker_code.ui.shell.components.markdown import pythinker_markdown +from pythinker_code.ui.shell.components.report import render_agent_body from pythinker_code.ui.theme import tui_rich_style __all__ = [ @@ -96,7 +97,7 @@ def render_assistant_message( for i, item in enumerate(items): next_visible = i + 1 < len(items) if item.kind == "text": - blocks.append(pythinker_markdown(item.text.strip())) + blocks.append(render_agent_body(item.text.strip())) elif item.kind == "thinking": if hide_thinking: blocks.append(Text(hidden_thinking_label, style=thinking_style)) diff --git a/src/pythinker_code/ui/shell/components/render_utils.py b/src/pythinker_code/ui/shell/components/render_utils.py index 81a3a173..d0be12e4 100644 --- a/src/pythinker_code/ui/shell/components/render_utils.py +++ b/src/pythinker_code/ui/shell/components/render_utils.py @@ -16,8 +16,9 @@ _ELLIPSIS = "…" _ANSI_CSI_RE = re.compile(r"\x1b\[[0-?]*[ -/]*[@-~]") _ANSI_OSC_RE = re.compile(r"\x1b\][^\x07\x1b]*(?:\x07|\x1b\\)") +_ANSI_APC_RE = re.compile(r"\x1b_[^\x07\x1b]*(?:\x07|\x1b\\)") _ANSI_ST_RE = re.compile(r"\x1b\\") -_CONTROL_RE = re.compile(r"[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]") +_CONTROL_RE = re.compile(r"[\x00-\x08\x0b-\x0d\x0e-\x1f\x7f]") @dataclass(frozen=True, slots=True) @@ -111,16 +112,25 @@ def cell_width(text: str) -> int: return cell_len(text) -def truncate_to_width(text: str, max_width: int, *, ellipsis: str = _ELLIPSIS) -> str: +def truncate_to_width( + text: str, + max_width: int, + *, + ellipsis: str = _ELLIPSIS, + pad: bool = False, +) -> str: """Truncate *text* so its terminal cell width fits within *max_width*. If *max_width* is too small to hold the ellipsis, returns the leading - cells of *text* without an ellipsis. + cells of *text* without an ellipsis. When *pad* is true, right-pad the + result to exactly *max_width* terminal cells. """ if max_width <= 0: return "" if cell_len(text) <= max_width: - return text + if not pad: + return text + return text + " " * max(0, max_width - cell_len(text)) ellipsis_w = cell_len(ellipsis) if max_width <= ellipsis_w: # No room for the marker — fall back to plain truncation. @@ -132,7 +142,10 @@ def truncate_to_width(text: str, max_width: int, *, ellipsis: str = _ELLIPSIS) - break out.append(ch) used += w - return "".join(out) + result = "".join(out) + if pad: + result += " " * max(0, max_width - cell_len(result)) + return result budget = max_width - ellipsis_w used = 0 cut = 0 @@ -143,7 +156,10 @@ def truncate_to_width(text: str, max_width: int, *, ellipsis: str = _ELLIPSIS) - break used += w cut = i + 1 - return text[:cut] + ellipsis + result = text[:cut] + ellipsis + if pad: + result += " " * max(0, max_width - cell_len(result)) + return result def render_message_response(renderable: RenderableType) -> RenderableType: @@ -172,13 +188,14 @@ def dim(text: str | Text) -> Text: def sanitize_ansi(text: str) -> str: """Strip ANSI escape sequences and other unsafe control bytes from *text*. - Keeps newlines, carriage returns, and tabs. Use before feeding raw shell - output into a Rich renderable to avoid cursor-movement and color leaks + Keeps newlines and tabs, but strips carriage returns. Use before feeding raw + shell output into a Rich renderable to avoid cursor-movement and color leaks that break layout. """ no_csi = _ANSI_CSI_RE.sub("", text) no_osc = _ANSI_OSC_RE.sub("", no_csi) - no_st = _ANSI_ST_RE.sub("", no_osc) + no_apc = _ANSI_APC_RE.sub("", no_osc) + no_st = _ANSI_ST_RE.sub("", no_apc) return _CONTROL_RE.sub("", no_st) diff --git a/src/pythinker_code/ui/shell/components/report.py b/src/pythinker_code/ui/shell/components/report.py new file mode 100644 index 00000000..0b629329 --- /dev/null +++ b/src/pythinker_code/ui/shell/components/report.py @@ -0,0 +1,275 @@ +"""Standardized report renderer. + +One structured shape and one muted, roomy rendering for every report Pythinker +produces (code review, verify, security review, …). Reports reach the shell two +ways: + +* Python callers build a :class:`Report` and call :func:`render_report`. +* Skills/agents emit a ```` ```report ```` fenced block of JSON; the shell + splits it out of the surrounding markdown via :func:`render_agent_body` and + renders it through the same path. A malformed block is never swallowed — it + falls back to ordinary markdown (shown as a code block). + +Styling reuses the existing theme tokens (:func:`tui_rich_style`), so the +"clear, not bright" palette and dark/light support come for free. +""" + +from __future__ import annotations + +import json +import logging +import re +from dataclasses import dataclass +from typing import Any, Literal, cast, get_args + +from rich.console import Group, RenderableType +from rich.padding import Padding +from rich.rule import Rule +from rich.style import Style as RichStyle +from rich.text import Text + +from pythinker_code.ui.shell.components.markdown import pythinker_markdown +from pythinker_code.ui.theme import ThemeName, tui_rich_style + +_log = logging.getLogger(__name__) + +__all__ = [ + "Report", + "ReportFinding", + "Severity", + "has_report_block", + "parse_report_block", + "render_agent_body", + "render_report", +] + +Severity = Literal["critical", "high", "medium", "low", "info"] + +# Most-severe first — drives both grouping order and the summary tally. +_SEVERITY_ORDER: tuple[Severity, ...] = get_args(Severity) +_SEVERITY_SET = frozenset(_SEVERITY_ORDER) + +# severity -> (token name, bold). Muted theme tokens only; critical is the one +# emphasis (bold) so the eye lands on it without a brighter colour. +_SEVERITY_TOKEN: dict[Severity, tuple[str, bool]] = { + "critical": ("error", True), + "high": ("error", False), + "medium": ("warning", False), + "low": ("accent", False), + "info": ("muted", False), +} + +_DOT = "●" + + +@dataclass(frozen=True, slots=True) +class ReportFinding: + """One finding in a report.""" + + title: str + severity: Severity + location: str | None = None # e.g. "src/foo.py:42-58" + body: str = "" # markdown prose + + +@dataclass(frozen=True, slots=True) +class Report: + """A standardized report. The summary tally is derived, never supplied.""" + + title: str + scope: str | None = None + findings: tuple[ReportFinding, ...] = () + note: str | None = None # closing "most actionable" line + + +def _counts(findings: tuple[ReportFinding, ...]) -> dict[Severity, int]: + counts: dict[Severity, int] = dict.fromkeys(_SEVERITY_ORDER, 0) + for finding in findings: + counts[finding.severity] += 1 + return counts + + +def _severity_style(severity: Severity, theme: ThemeName | None) -> RichStyle: + token, bold = _SEVERITY_TOKEN[severity] + style = tui_rich_style(token, theme=theme) + return style + RichStyle(bold=True) if bold else style + + +def _summary_line(counts: dict[Severity, int], theme: ThemeName | None) -> Text: + line = Text() + first = True + for severity in _SEVERITY_ORDER: + count = counts[severity] + if not count: + continue + if not first: + line.append(" ") + first = False + line.append(f"{_DOT} ", style=_severity_style(severity, theme)) + line.append(f"{count} {severity}", style=tui_rich_style("text", theme=theme)) + if not counts["critical"] and not counts["high"]: + prefix = " " if not first else "" + line.append(f"{prefix}no critical or high", style=tui_rich_style("muted", theme=theme)) + return line + + +def _render_finding(finding: ReportFinding, theme: ThemeName | None) -> RenderableType: + rows: list[RenderableType] = [] + + title = Text() + title.append(f"{_DOT} ", style=_severity_style(finding.severity, theme)) + title.append(finding.title, style=tui_rich_style("text", theme=theme) + RichStyle(bold=True)) + rows.append(title) + + if finding.location: + rows.append(Text(f" {finding.location}", style=tui_rich_style("dim", theme=theme))) + + if finding.body.strip(): + rows.append(Padding(pythinker_markdown(finding.body.strip()), (0, 0, 0, 2))) + + return Group(*rows) + + +def render_report(report: Report, *, theme: ThemeName | None = None) -> RenderableType: + """Render *report* as a muted, roomy Rich renderable (no outer box).""" + counts = _counts(report.findings) + border = tui_rich_style("border_muted", theme=theme) + blank = Text("") + + rows: list[RenderableType] = [ + Text(report.title, style=tui_rich_style("text", theme=theme) + RichStyle(bold=True)), + ] + if report.scope: + rows += [blank, Text(report.scope, style=tui_rich_style("dim", theme=theme))] + rows += [blank, _summary_line(counts, theme)] + + for severity in _SEVERITY_ORDER: + group = [f for f in report.findings if f.severity == severity] + if not group: + continue + rows.append(blank) + rows.append(Rule(f" {severity.capitalize()} ", align="left", style=border, characters="─")) + for finding in group: + rows.append(blank) + rows.append(_render_finding(finding, theme)) + + if report.note: + rows += [ + blank, + Rule(style=border, characters="─"), + Text(report.note, style=tui_rich_style("muted", theme=theme)), + ] + + # One column of left breathing room; vertical roominess comes from the + # blank rows between sections and findings. + return Padding(Group(*rows), (0, 0, 0, 1)) + + +def parse_report_block(payload: str) -> Report | None: + """Deserialize a ```` ```report ```` block's JSON into a :class:`Report`. + + Returns ``None`` on any malformed payload so callers can fall back to + rendering the raw text — a bad block must never be swallowed. + """ + try: + parsed = json.loads(payload) + except (ValueError, TypeError) as exc: + _log.debug( + "parse_report_block: JSON decode failed (type=%s len=%d)", + type(payload).__name__, + len(payload), + exc_info=exc, + ) + return None + if not isinstance(parsed, dict): + return None + data = cast(dict[str, Any], parsed) + + title = data.get("title") + if not isinstance(title, str) or not title.strip(): + return None + + scope = data.get("scope") + scope = scope if isinstance(scope, str) and scope.strip() else None + note = data.get("note") + note = note if isinstance(note, str) and note.strip() else None + + raw_findings = data.get("findings") + if raw_findings is not None and not isinstance(raw_findings, list): + return None + + findings: list[ReportFinding] = [] + for raw in cast("list[Any]", raw_findings or []): + if not isinstance(raw, dict): + return None + entry = cast(dict[str, Any], raw) + f_title = entry.get("title") + severity = entry.get("severity") + if not isinstance(f_title, str) or not f_title.strip(): + return None + if severity not in _SEVERITY_SET: + return None + location = entry.get("location") + location = location if isinstance(location, str) and location.strip() else None + body = entry.get("body") + body = body if isinstance(body, str) else "" + findings.append( + ReportFinding(title=f_title, severity=severity, location=location, body=body) + ) + + return Report(title=title, scope=scope, findings=tuple(findings), note=note) + + +# A fenced block whose info string is exactly ``report`` (optionally followed by +# whitespace). Captures the JSON payload between the fences. +_REPORT_FENCE_RE = re.compile( + r"^[ \t]*```[ \t]*report[ \t]*\n(?P.*?)\n[ \t]*```[ \t]*$", + re.DOTALL | re.MULTILINE, +) + + +def has_report_block(text: str) -> bool: + """Whether *text* contains at least one well-formed ` ```report ` block. + + Used by output surfaces (e.g. the headless final-text printer) to decide + whether to route through :func:`render_agent_body` instead of emitting the + raw text. Only matches blocks that actually parse, so a malformed fence + leaves output unchanged. + """ + return any( + parse_report_block(m.group("payload")) is not None for m in _REPORT_FENCE_RE.finditer(text) + ) + + +def render_agent_body(text: str, *, theme: ThemeName | None = None) -> RenderableType: + """Render assistant text, promoting ```` ```report ```` blocks to reports. + + Non-report text renders via :func:`pythinker_markdown`; a valid report + block renders via :func:`render_report`; an invalid block is left in place + so the surrounding markdown shows it as an ordinary code block. + """ + segments: list[RenderableType] = [] + cursor = 0 + for match in _REPORT_FENCE_RE.finditer(text): + report = parse_report_block(match.group("payload")) + if report is None: + continue # malformed — leave it for the markdown renderer + before = text[cursor : match.start()].strip("\n") + if before: + segments.append(pythinker_markdown(before)) + segments.append(render_report(report, theme=theme)) + cursor = match.end() + + if not segments: + return pythinker_markdown(text) + + rest = text[cursor:].strip("\n") + if rest: + segments.append(pythinker_markdown(rest)) + + spaced: list[RenderableType] = [] + for i, segment in enumerate(segments): + if i: + spaced.append(Text("")) + spaced.append(segment) + return Group(*spaced) diff --git a/src/pythinker_code/ui/shell/components/settings_list.py b/src/pythinker_code/ui/shell/components/settings_list.py index 12722fbf..985165df 100644 --- a/src/pythinker_code/ui/shell/components/settings_list.py +++ b/src/pythinker_code/ui/shell/components/settings_list.py @@ -174,9 +174,16 @@ def cancel(self) -> None: def visible_window(self) -> tuple[int, int]: if not self.visible: return (0, 0) - max_visible = max(1, self.config.max_visible) - start = max(0, min(self.selected_idx - max_visible // 2, len(self.visible) - max_visible)) - end = min(start + max_visible, len(self.visible)) + budget = max(1, self.config.max_visible) + if len(self.visible) <= budget: + return (0, len(self.visible)) + # When the list overflows, items_text appends a scroll-indicator row, + # so the rendered content is one row taller than the slice. Reserve a + # row for it; otherwise the selected row can scroll under the indicator + # and off the bottom of the (budget-tall) window. Mirrors selector.py. + span = max(1, budget - 1) + start = max(0, min(self.selected_idx - span // 2, len(self.visible) - span)) + end = min(start + span, len(self.visible)) return (start, end) @@ -381,7 +388,9 @@ def _(event: KeyPressEvent) -> None: ), Window( FormattedTextControl(items_text), - height=Dimension(preferred=min(max(1, config.max_visible), 12), min=1), + # Size to the full slice (which reserves the scroll row); + # capping below max_visible would clip the selected row. + height=Dimension(preferred=max(1, config.max_visible), min=1), style="class:slash-completion-menu", ), Window( @@ -401,6 +410,9 @@ def _(event: KeyPressEvent) -> None: full_screen=False, style=get_prompt_style(), mouse_support=False, + # Erase the menu chrome on exit (apply or cancel) so it doesn't linger + # in the scrollback as a ghost menu. Mirrors selector.py. + erase_when_done=True, ) diff --git a/src/pythinker_code/ui/shell/selector.py b/src/pythinker_code/ui/shell/selector.py index 6bc34dbc..0c7097c7 100644 --- a/src/pythinker_code/ui/shell/selector.py +++ b/src/pythinker_code/ui/shell/selector.py @@ -259,11 +259,16 @@ def clear_filter(self) -> None: def visible_window(self) -> tuple[int, int]: if not self.visible: return (0, 0) - max_visible = max(1, self.config.max_visible) - if len(self.visible) <= max_visible: + budget = max(1, self.config.max_visible) + if len(self.visible) <= budget: return (0, len(self.visible)) - start = max(0, min(self.selected_idx - max_visible // 2, len(self.visible) - max_visible)) - end = min(start + max_visible, len(self.visible)) + # When the list overflows, items_text appends a scroll-indicator row, + # so the rendered content is one row taller than the slice. Reserve a + # row for it; otherwise the selected row can scroll under the indicator + # and off the bottom of the (budget-tall) window. + span = max(1, budget - 1) + start = max(0, min(self.selected_idx - span // 2, len(self.visible) - span)) + end = min(start + span, len(self.visible)) return (start, end) @@ -411,7 +416,10 @@ def on_any(event: KeyPressEvent) -> None: ), Window( FormattedTextControl(items_text), - height=Dimension(preferred=min(max(1, config.max_visible), 12), min=1), + # Render as many rows as the slice may produce (max_visible, + # which already includes the reserved scroll-indicator row). + # Capping below max_visible would clip the selected row. + height=Dimension(preferred=max(1, config.max_visible), min=1), style="class:slash-completion-menu", ), Window( @@ -434,6 +442,9 @@ def on_any(event: KeyPressEvent) -> None: full_screen=False, style=get_prompt_style(), mouse_support=False, + # Erase the selector chrome on exit (commit or cancel) so it doesn't + # linger in the scrollback as a ghost menu after Esc/Enter. + erase_when_done=True, ) diff --git a/src/pythinker_code/ui/shell/slash.py b/src/pythinker_code/ui/shell/slash.py index 440e8114..c15c0429 100644 --- a/src/pythinker_code/ui/shell/slash.py +++ b/src/pythinker_code/ui/shell/slash.py @@ -576,201 +576,22 @@ def _feedback_destination(soul: PythinkerSoul) -> tuple[str, dict[str, str]] | N return f"{pythinker_platform.base_url.rstrip('/')}/feedback", headers -def _feedback_github_config(soul: PythinkerSoul) -> tuple[str, str] | None: - """Return GitHub OAuth client_id and repo when direct user-owned issues are enabled.""" - import os - - feedback_config = soul.runtime.config.feedback - client_id = os.getenv("PYTHINKER_FEEDBACK_GITHUB_CLIENT_ID", "").strip() - if not client_id: - client_id = feedback_config.github_client_id.strip() - repo = os.getenv("PYTHINKER_FEEDBACK_GITHUB_REPO", "").strip() - if not repo: - repo = feedback_config.github_repo.strip() - if not client_id or not repo: - return None - return client_id, repo - - -def _feedback_issue_title(payload: dict[str, str | None]) -> str: - version = f" {payload['version']}" if payload.get("version") else "" - session = payload.get("session_id") or "" - suffix = f" ({session[:8]})" if session else "" - return f"[Pythinker CLI] Feedback{version}{suffix}" - - -def _feedback_issue_body(payload: dict[str, str | None]) -> str: - return "\n".join( - [ - "## User submission", - "", - payload.get("content") or "_(no comment)_", - "", - "## Context", - "", - "- Type: feedback", - f"- Session: {payload.get('session_id') or 'unknown'}", - f"- Version: {payload.get('version') or 'unknown'}", - f"- OS: {payload.get('os') or 'unknown'}", - f"- Model: {payload.get('model') or 'unknown'}", - ] - ) - - @registry.command @shell_mode_registry.command -async def feedback(app: Shell, args: str): - """Submit feedback to make Pythinker CLI better""" - import platform +def feedback(app: Shell, args: str): + """Open a GitHub issue to submit feedback or report a bug""" import webbrowser - import aiohttp - - from pythinker_code.constant import VERSION - from pythinker_code.ui.shell.oauth import current_model_key from pythinker_code.ui.theme import get_tui_tokens as _get_tok_fb - from pythinker_code.utils.aiohttp import new_client_session _t_fb = _get_tok_fb() - ISSUE_URL = "https://github.com/TechMatrix-labs/pythinker-code/issues" - - def _fallback_to_issues(): - if not webbrowser.open(ISSUE_URL): - console.print(f"Please submit feedback at [underline]{ISSUE_URL}[/underline].") - - soul = ensure_pythinker_soul(app) - if soul is None: - _fallback_to_issues() - return - - github_config = _feedback_github_config(soul) - destination = None if github_config is not None else _feedback_destination(soul) - if github_config is None and destination is None: - _fallback_to_issues() - return - - from prompt_toolkit import PromptSession - - prompt_session: PromptSession[str] = PromptSession() - try: - content = await prompt_session.prompt_async("Enter your feedback: ") - except (EOFError, KeyboardInterrupt): - console.print(f"[{_t_fb.muted}]Feedback cancelled.[/]") - return - - content = content.strip() - if not content: - console.print(f"[{_t_fb.warning}]Feedback cannot be empty.[/]") - return - - payload = { - "session_id": soul.runtime.session.id, - "content": content, - "version": VERSION, - "os": f"{platform.system()} {platform.release()}", - "model": current_model_key(soul), - } - - if github_config is not None: - client_id, repo = github_config - from pythinker_code.auth.github_feedback import ( - GitHubFeedbackError, - create_github_issue, - load_github_feedback_token, - login_github_feedback, - star_github_repo, - ) - - try: - token = load_github_feedback_token() - if token is None: - console.print(f"[{_t_fb.info}]GitHub login required to create the issue as you.[/]") - async for event in login_github_feedback(client_id): - if event.type == "waiting": - console.print(event.message, markup=False) - elif event.type in {"verification_url", "success", "error"}: - from rich.style import Style as _RichStyleFb - - _style_fb = None - if event.type == "success": - _style_fb = _RichStyleFb(color=_t_fb.success) - elif event.type == "error": - _style_fb = _RichStyleFb(color=_t_fb.error) - console.print(event.message, markup=False, style=_style_fb) - token = load_github_feedback_token() - if token is None: - console.print(f"[{_t_fb.error}]GitHub login did not produce a usable token.[/]") - return - with console.status(f"[{_t_fb.info}]Creating GitHub issue...[/]"): - issue = await create_github_issue( - repo, - token, - title=_feedback_issue_title(payload), - body=_feedback_issue_body(payload), - ) - from pythinker_code.telemetry import track - - track("feedback_submitted", destination="github") - if issue.html_url: - issue_url = _rich_escape(issue.html_url) - console.print(f"[{_t_fb.success}]GitHub issue created:[/] {issue_url}") - else: - console.print(f"[{_t_fb.success}]GitHub issue created.[/]") - - try: - star_answer = await prompt_session.prompt_async( - "Do you like Pythinker CLI? Star the GitHub repo? [y/N]: " - ) - except (EOFError, KeyboardInterrupt): - star_answer = "" - if star_answer.strip().lower() in {"y", "yes"}: - try: - with console.status(f"[{_t_fb.info}]Starring GitHub repo...[/]"): - await star_github_repo(repo, token) - track("github_repo_starred") - console.print(f"[{_t_fb.success}]Thanks for starring the repo![/]") - except (GitHubFeedbackError, TimeoutError, aiohttp.ClientError) as e: - console.print(f"[{_t_fb.warning}]Could not star the repo: {_rich_escape(e)}[/]") - except (GitHubFeedbackError, TimeoutError, aiohttp.ClientError) as e: - console.print(f"[{_t_fb.error}]Failed to create GitHub issue: {_rich_escape(e)}[/]") - _fallback_to_issues() - return - - assert destination is not None - feedback_url, headers = destination - - with console.status(f"[{_t_fb.info}]Submitting feedback...[/]"): - try: - async with ( - new_client_session() as session, - session.post( - feedback_url, - json=payload, - headers=headers, - raise_for_status=True, - ), - ): - pass - session_id = soul.runtime.session.id - from pythinker_code.telemetry import track + ISSUE_URL = "https://github.com/TechMatrix-labs/pythinker-code/issues/new/choose" - track("feedback_submitted") - console.print( - f"[{_t_fb.success}]Feedback submitted, thank you! " - f"Your session ID is: {session_id}[/]" - ) - except TimeoutError: - console.print(f"[{_t_fb.error}]Feedback submission timed out.[/]") - _fallback_to_issues() - except aiohttp.ClientError as e: - status = getattr(e, "status", None) - if status: - msg = f"Failed to submit feedback (HTTP {status})." - else: - msg = "Network error, failed to submit feedback." - console.print(f"[{_t_fb.error}]{msg}[/]") - _fallback_to_issues() + if webbrowser.open(ISSUE_URL): + console.print(f"[{_t_fb.success}]Opening GitHub issues in your browser...[/]") + else: + console.print(f"Please open: [underline]{ISSUE_URL}[/underline]") @registry.command(aliases=["report-error", "report"]) diff --git a/src/pythinker_code/ui/shell/visualize/_blocks.py b/src/pythinker_code/ui/shell/visualize/_blocks.py index 45875f61..3fac8375 100644 --- a/src/pythinker_code/ui/shell/visualize/_blocks.py +++ b/src/pythinker_code/ui/shell/visualize/_blocks.py @@ -27,6 +27,7 @@ from pythinker_code.ui.shell.components.markdown import ( markdown_commit_boundary, ) +from pythinker_code.ui.shell.components.report import render_agent_body from pythinker_code.ui.shell.console import console, current_console_width from pythinker_code.ui.shell.glyphs import TRANSCRIPT_ASSISTANT_MARKER, TRANSCRIPT_STATUS_MARKER from pythinker_code.ui.shell.mcp_status import mcp_startup_header @@ -231,7 +232,7 @@ def compose_final(self) -> RenderableType: remaining = self._pending_text() if not remaining: return Text("") - return self._wrap_bullet(Markdown(remaining)) + return self._wrap_bullet(render_agent_body(remaining)) def has_pending(self) -> bool: """Whether there is uncommitted content to flush.""" @@ -282,7 +283,7 @@ def _flush_committed(self) -> None: if not self._has_printed_bullet: # Leading blank row separates this step from the previous block. console.print() - console.print(self._wrap_bullet(Markdown(committed_text))) + console.print(self._wrap_bullet(render_agent_body(committed_text))) self._committed_len += boundary def _activity_snapshot( diff --git a/tasks/agent-behavior-findings.md b/tasks/agent-behavior-findings.md new file mode 100644 index 00000000..428dd994 --- /dev/null +++ b/tasks/agent-behavior-findings.md @@ -0,0 +1,124 @@ +# Agent behaviour review — `.pythinker` + `.pythinker-review` + +Diagnosis from runtime artifacts (review run `20260522015318-650c6d67`, scratch +sessions May 27–28). Evidence-based; root causes confirmed in source. + +## Status (applied 2026-05-28) +- **R1 — applied** (`reviewers/common.py`): retry now relays the concrete + validation error + an explicit title-length nudge. Tests: + `test_retry_prompt_surfaces_previous_validation_error`. +- **R2 — applied** (`reviewers/schema.py`): `title` is truncated (≤80, ellipsis) + via a before-validator instead of hard-failing the whole `ReviewerOutput`. + Tests: `test_reviewer_output_truncates_overlong_title`, + `test_overlong_title_is_truncated_not_dropped`. +- **S1 — applied** (`scratchpad.py` + `cli/__init__.py`): `append_scratch_event_sync` + gained an idempotent `dedup_signature`; the CLI passes `source:` so a + relaunch no longer appends a duplicate "session start". Tests: + `test_session_start_event_is_idempotent_per_signature`. +- **R4 — applied** (`reviewers/prompts/*.system.md`): the `evidence_snippet` + placeholder now demands VERBATIM, character-for-character copying (no + paraphrase/ellipses) across all four reviewer prompts, so the model's output + passes the containment validator instead of being dropped per-finding. +- **S2 — applied** (`scratchpad.py`): labels now collapse by key — + single-valued keys (`session/workspace/ui/source/scope`) keep the latest + value, multi-valued keys (`kind`) keep the unique set. Kills + `source:startup | source:resume` noise while retaining `kind:todo | kind:agent`. + Tests: `test_session_labels_collapse_single_valued_keys`. (Note: the original + delimiter-injection worry was already mitigated by `_clean_event_text`, which + strips `|`/`\r`/`\n`; this change addresses the residual duplicate-key noise.) +- **R3 — deliberately NOT applied.** Gating the run when a chunk is unreviewable + (`malformed_output`) is *correct* fail-closed behaviour for a security tool; + making it non-gating would let a file pass review unreviewed. R1+R2+R4 instead + cut the failure *rate* so runs pass legitimately. +- **S3 — deliberately NOT applied.** Auto-pruning scratch files conflicts with + the module's documented design (`scratchpad.py` header: retained as history + "unless the user explicitly asks for cleanup"). S1 already removes the main + growth driver (duplicate session-starts); broader retention is a product + decision for the user, not a silent change. + +Original findings below. + +## Review subsystem (the background review subagent) — highest impact + +### R1 [High] Retry never relays the real validation error → systematic schema misses lose the whole chunk +`reviewers/common.py:69-92`. On a parse/validation failure the harness retries +once, but `_RETRY_SUFFIX` (common.py:15) only says *"your response was not valid +JSON … reply with strict JSON only."* The captured Pydantic error (`last_error`, +common.py:87) is **never fed back to the model**. So when the failure is a +*content* violation (e.g. a title > 80 chars — valid JSON, invalid schema), the +retry message is actively misleading and the model has no reason to change. Both +attempts fail → `ok=False` → **every finding in that chunk is discarded.** +- Evidence: meta `chunk_failures` — `tests/ui_and_conv/test_prompt_tips.py` + failed with *4* `String should have at most 80 characters` errors; the whole + chunk's findings were lost. +- Fix direction: append `last_error` to the retry prompt so the model can + self-correct. + +### R2 [High] No field-level coercion at ingest; one bad field nukes the chunk +`reviewers/schema.py:16` `title: str = Field(max_length=80)`. `ReviewerOutput` +is parsed all-or-nothing via `model_validate_json` (common.py:84), so a single +over-long title (or any one out-of-range field) fails the *entire* output and +drops all sibling findings in the chunk. Smaller models (run model: MiniMax +M2.7) hit the 80-char cap routinely. +- Fix direction: soft-coerce on ingest (truncate title to 80 with ellipsis) + instead of hard-failing; or parse findings individually so one bad finding + doesn't take the rest down. + +### R3 [Med] Model-formatting noise gates the entire run +`engine/runner.py:163` `failed=(not allow_partial) and bool(real_failures)`, and +`real_failures` excludes `validation_error` but **includes** `malformed_output` +(runner.py:156). Result: 2 of 54 chunks failing on model-formatting (long title, +truncated JSON) flipped the whole run to `status: "failed"` despite 42 valid +findings delivered. `malformed_output` is a model-quality issue like +`validation_error`, not a runtime failure (timeout/llm_error/worker_error). +- Fix direction: treat `malformed_output` as non-gating (like validation_error), + or add a distinct "delivered with model-output gaps" status. + +### R4 [Low / mostly positive] Evidence-snippet validation is graceful but mismatch rate is high +`reviewers/validation.py:_snippet_matches` already falls back rendered-diff → +whitespace-compacted → on-disk slice → compacted file (good; these drop +**per-finding**, not per-chunk — runner.py:101-115). But 7 findings still failed +evidence match, i.e. the model paraphrases snippets beyond whitespace. +- Fix direction: emphasise verbatim copying in the reviewer prompt, or add a + token-overlap fuzzy match as a last-resort tier. + +## Scratchpad / session memory (`.pythinker/scratch`) + +### S1 [Med] "session start" journaling is not idempotent +`cli/__init__.py:761-781` emits a `session start` event unconditionally on every +CLI init for the session, with no guard against an existing start block for the +same session id. +- Evidence: session `7f7a8039` has **4** `session start / source: startup` + entries in 3 min; `06ba6c38`, `7c6a9676`, `f42f6caa` each have duplicate + same-minute startups. Defeats the file's stated "compact" purpose. +- Fix direction: skip if the last event for the session id is already a + session-start within N seconds, or only emit once per genuine create/resume. + +### S2 [Med] Label line: duplicate keys unmerged + unsanitised free text → parse/injection risk +The `labels:` line is `key:value` joined by ` | `. +- Duplicate keys are appended, not merged: `source:startup | source:resume`, + multiple `kind:` values (`kind:todo | kind:agent-batch | kind:agent`). Recall + on labels sees conflicting values. +- The raw session title is embedded as a scope label: + `scope:Goal: perform a deep code scan analysis of the…` — contains a colon, + spaces, an ellipsis, and could contain `|` or a newline from an arbitrary + title, corrupting any `:`/`|` splitter. +- Evidence: session `b241579b` labels line. +- Fix direction: dedupe/merge by key; slugify or escape the title before using + it as a label value (or store title separately, not as a label). + +### S3 [Low] Unbounded scratch growth; no-op sessions persist boilerplate +Most `untitled` files are 558 bytes = header + a single session-start (sessions +that did nothing). Combined with S1, the dir grows unbounded with low-value +files; no pruning/retention observed. +- Fix direction: lazy-create the file on first substantive event, and/or prune + near-empty session files on a retention policy. + +## Positive — validates the enhancement direction +Background agent-batch + todo journaling is coherent. Session `b241579b`: +4 background agents (`review`, `security-reviewer`, `explore`, `verifier`) +tracked start→completed, todo progressing 0→6 done, agent-type captured +(`7c6a9676`: `agent-type:code-reviewer`, `agent started / mode: foreground`). +The SetTodoList + background-subagent foundation being enhanced is sound; the +gaps above are in the *journaling* and *review-output* layers, not the +orchestration itself. diff --git a/tests/auth/test_opencode_go_auth.py b/tests/auth/test_opencode_go_auth.py index 066caf3c..b8c2a3e6 100644 --- a/tests/auth/test_opencode_go_auth.py +++ b/tests/auth/test_opencode_go_auth.py @@ -35,21 +35,27 @@ def test_opencode_go_model_catalog_contains_all_current_models(): "opencode-go/mimo-v2.5", "opencode-go/qwen3.5-plus", "opencode-go/qwen3.6-plus", + "opencode-go/qwen3.7-max", "opencode-go/minimax-m2.5", "opencode-go/minimax-m2.7", } - minimax = { - m.model_id: m.provider_key for m in OPENCODE_GO_MODELS if m.model_id.startswith("minimax-") + # Anthropic-shaped models (models.dev @ai-sdk/anthropic): both MiniMax and + # all three Qwen models. Everything else is OpenAI-compatible. + anthropic_ids = { + m.model_id for m in OPENCODE_GO_MODELS if m.provider_key == "managed:opencode-go-anthropic" } - assert minimax == { - "minimax-m2.5": "managed:opencode-go-anthropic", - "minimax-m2.7": "managed:opencode-go-anthropic", + assert anthropic_ids == { + "minimax-m2.5", + "minimax-m2.7", + "qwen3.5-plus", + "qwen3.6-plus", + "qwen3.7-max", } assert all( m.provider_key == "managed:opencode-go-openai" for m in OPENCODE_GO_MODELS - if not m.model_id.startswith("minimax-") + if m.model_id not in anthropic_ids ) @@ -72,6 +78,7 @@ def test_opencode_go_env_key_precedence(monkeypatch): def test_apply_opencode_go_config_writes_two_providers_and_default(): from pythinker_code.auth.opencode_go import ( + OPENCODE_GO_ANTHROPIC_BASE_URL, OPENCODE_GO_ANTHROPIC_PROVIDER_KEY, OPENCODE_GO_BASE_URL, OPENCODE_GO_OPENAI_PROVIDER_KEY, @@ -91,7 +98,9 @@ def test_apply_opencode_go_config_writes_two_providers_and_default(): assert openai_provider.type == "openai_legacy" assert anthropic_provider.type == "anthropic" assert openai_provider.base_url == OPENCODE_GO_BASE_URL - assert anthropic_provider.base_url == OPENCODE_GO_BASE_URL + # Anthropic base must omit "/v1" (the SDK appends "/v1/messages"). + assert anthropic_provider.base_url == OPENCODE_GO_ANTHROPIC_BASE_URL + assert anthropic_provider.base_url == "https://opencode.ai/zen/go" assert openai_provider.api_key.get_secret_value() == "ocgo-test" assert anthropic_provider.api_key.get_secret_value() == "ocgo-test" assert config.models["opencode-go/kimi-k2.6"].provider == OPENCODE_GO_OPENAI_PROVIDER_KEY @@ -217,39 +226,146 @@ async def fake_discover(api_key): assert config.default_model == "opencode-go/kimi-k2.6" +@pytest.mark.asyncio +async def test_fetch_models_dev_metadata_uses_short_best_effort_timeout(monkeypatch): + """The best-effort enrichment fetch must use a tight timeout so a slow + models.dev cannot block login for up to the 120s default.""" + from pythinker_code.auth import opencode_go + + captured: dict[str, aiohttp.ClientTimeout | None] = {} + + def fake_session(*, timeout=None): + captured["timeout"] = timeout + raise aiohttp.ClientError("unreachable") + + monkeypatch.setattr(opencode_go, "new_client_session", fake_session) + + result = await opencode_go._fetch_models_dev_metadata() + + assert result == {} # degrades gracefully to the curated catalog + assert captured["timeout"] is opencode_go.MODELS_DEV_TIMEOUT + assert opencode_go.MODELS_DEV_TIMEOUT.total is not None + assert opencode_go.MODELS_DEV_TIMEOUT.total <= 15 + + @pytest.mark.parametrize( - "payload, expected_aliases", + "payload, expected_ids", [ - (None, set()), - ({}, set()), - ({"data": "not a list"}, set()), - ({"data": [{"context_length": 1000}]}, set()), # missing id - ({"data": [{"id": "unknown-model"}]}, set()), # unknown id dropped - ({"data": [{"id": "kimi-k2.6"}]}, {"opencode-go/kimi-k2.6"}), + (None, []), + ({}, []), + ({"data": "not a list"}, []), + ({"data": [{"context_length": 1000}]}, []), # missing id + ({"data": [{"id": ""}]}, []), # empty id skipped + ({"data": [{"id": "kimi-k2.6"}, "bogus", {"id": "new-model"}]}, ["kimi-k2.6", "new-model"]), ], ) -def test_parse_discovered_models_handles_malformed_payloads(payload, expected_aliases): - from pythinker_code.auth.opencode_go import _parse_discovered_models +def test_extract_model_ids_handles_malformed_payloads(payload, expected_ids): + from pythinker_code.auth.opencode_go import _extract_model_ids + + assert _extract_model_ids(payload) == expected_ids + + +def test_build_models_uses_models_dev_shape_for_known_ids(): + from pythinker_code.auth.opencode_go import ( + OPENCODE_GO_ANTHROPIC_PROVIDER_KEY, + OPENCODE_GO_OPENAI_PROVIDER_KEY, + _build_models, + _ModelsDevMeta, + ) + + # models.dev is authoritative for shape + context, even for known ids: + # the Qwen catalog drift that routed them to OpenAI must self-correct. + result = _build_models( + ["kimi-k2.6", "qwen3.6-plus"], + { + "kimi-k2.6": _ModelsDevMeta("Kimi K2.6", 262_144, False), + "qwen3.6-plus": _ModelsDevMeta("Qwen3.6 Plus", 262_144, True), + }, + ) + by_id = {m.model_id: m for m in result} + assert by_id["kimi-k2.6"].provider_key == OPENCODE_GO_OPENAI_PROVIDER_KEY + assert by_id["qwen3.6-plus"].provider_key == OPENCODE_GO_ANTHROPIC_PROVIDER_KEY + assert by_id["qwen3.6-plus"].max_context_size == 262_144 + + +def test_build_models_surfaces_unknown_id_enriched_from_models_dev(): + from pythinker_code.auth.opencode_go import ( + OPENCODE_GO_ANTHROPIC_PROVIDER_KEY, + _build_models, + _ModelsDevMeta, + ) + + # A brand-new model the catalog has never heard of must still appear, + # carrying the context + Anthropic shape models.dev reports for it. + result = _build_models( + ["qwen3.8-max"], {"qwen3.8-max": _ModelsDevMeta("Qwen3.8 Max", 1_000_000, True)} + ) + by_id = {m.model_id: m for m in result} + assert by_id["qwen3.8-max"].alias == "opencode-go/qwen3.8-max" + assert by_id["qwen3.8-max"].display_name == "Qwen3.8 Max" + assert by_id["qwen3.8-max"].max_context_size == 1_000_000 + assert by_id["qwen3.8-max"].provider_key == OPENCODE_GO_ANTHROPIC_PROVIDER_KEY + + +def test_build_models_falls_back_to_catalog_then_heuristic_without_metadata(): + from pythinker_code.auth.opencode_go import ( + OPENCODE_GO_ANTHROPIC_PROVIDER_KEY, + OPENCODE_GO_DEFAULT_CONTEXT, + OPENCODE_GO_OPENAI_PROVIDER_KEY, + _build_models, + ) - result = _parse_discovered_models(payload) - assert {m.alias for m in result} == expected_aliases + # No models.dev metadata at all (e.g. unreachable): known ids fall back to + # the corrected catalog, unknown ids to the name heuristic. + result = _build_models(["qwen3.7-max", "future-model", "minimax-m9.9"], {}) + by_id = {m.model_id: m for m in result} + # Known Qwen keeps its (corrected) catalog Anthropic shape. + assert by_id["qwen3.7-max"].provider_key == OPENCODE_GO_ANTHROPIC_PROVIDER_KEY + # Unknown, unguessable → OpenAI default + derived name + default context. + assert by_id["future-model"].display_name == "Future Model" + assert by_id["future-model"].max_context_size == OPENCODE_GO_DEFAULT_CONTEXT + assert by_id["future-model"].provider_key == OPENCODE_GO_OPENAI_PROVIDER_KEY + # Unknown minimax-* heuristically routed to the Anthropic-shaped provider. + assert by_id["minimax-m9.9"].provider_key == OPENCODE_GO_ANTHROPIC_PROVIDER_KEY -def test_parse_discovered_models_overrides_context_length_only_for_positive_int(): - from pythinker_code.auth.opencode_go import _parse_discovered_models +def test_parse_models_dev_metadata_extracts_name_context_and_shape(): + from pythinker_code.auth.opencode_go import _ModelsDevMeta, _parse_models_dev_metadata payload = { - "data": [ - {"id": "kimi-k2.6", "context_length": "bogus"}, # wrong type, ignored - {"id": "glm-5", "context_length": -5}, # non-positive, ignored - {"id": "deepseek-v4-pro", "context_length": 999_000}, # valid override - ] + "opencode-go": { + "npm": "@ai-sdk/openai-compatible", + "models": { + # Anthropic shape via per-model provider override. + "qwen3.7-max": { + "name": "Qwen3.7 Max", + "limit": {"context": 1_000_000}, + "provider": {"npm": "@ai-sdk/anthropic"}, + }, + # Inherits the OpenAI-compatible provider default. + "kimi-k2.6": {"name": "Kimi K2.6", "limit": {"context": 262_144}}, + "glm-5": {"name": "GLM-5", "limit": {"context": -1}}, # bad context dropped + "bad": "not a dict", + }, + }, + "opencode": {"models": {"gpt-5": {"name": "GPT-5"}}}, # other provider ignored } - result = _parse_discovered_models(payload) - by_id = {m.model_id: m for m in result} - assert by_id["kimi-k2.6"].max_context_size == 262_000 # default preserved - assert by_id["glm-5"].max_context_size == 262_000 - assert by_id["deepseek-v4-pro"].max_context_size == 999_000 + result = _parse_models_dev_metadata(payload) + assert result == { + "qwen3.7-max": _ModelsDevMeta("Qwen3.7 Max", 1_000_000, True), + "kimi-k2.6": _ModelsDevMeta("Kimi K2.6", 262_144, False), + "glm-5": _ModelsDevMeta("GLM-5", None, False), + } + + +@pytest.mark.parametrize( + "payload", + [None, {}, {"opencode-go": "not a dict"}, {"opencode-go": {"models": "not a dict"}}], +) +def test_parse_models_dev_metadata_handles_malformed_payloads(payload): + from pythinker_code.auth.opencode_go import _parse_models_dev_metadata + + assert _parse_models_dev_metadata(payload) == {} @pytest.mark.asyncio diff --git a/tests/core/test_agent_spec.py b/tests/core/test_agent_spec.py index 23d843eb..821838a0 100644 --- a/tests/core/test_agent_spec.py +++ b/tests/core/test_agent_spec.py @@ -133,6 +133,7 @@ def test_load_default_agent_spec(): assert subagent_specs["coder"].allowed_tools == snapshot( [ "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", @@ -148,7 +149,6 @@ def test_load_default_agent_spec(): [ "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", ] @@ -245,6 +245,7 @@ def test_load_default_agent_spec(): assert subagent_specs["explore"].allowed_tools == snapshot( [ "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", @@ -259,7 +260,6 @@ def test_load_default_agent_spec(): [ "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", @@ -359,6 +359,7 @@ def test_load_default_agent_spec(): assert subagent_specs["plan"].model == snapshot(None) assert subagent_specs["plan"].allowed_tools == snapshot( [ + "pythinker_code.tools.todo:SetTodoList", "pythinker_code.tools.file:ReadFile", "pythinker_code.tools.file:ReadMediaFile", "pythinker_code.tools.file:Glob", @@ -373,7 +374,6 @@ def test_load_default_agent_spec(): [ "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", diff --git a/tests/core/test_auth_error_handling.py b/tests/core/test_auth_error_handling.py index b88e07e4..fba5c92e 100644 --- a/tests/core/test_auth_error_handling.py +++ b/tests/core/test_auth_error_handling.py @@ -341,6 +341,26 @@ async def test_403_propagates_as_api_status_error(runtime: Runtime, tmp_path: Pa assert exc_info.value.status_code == 403 +def test_insufficient_credits_error_detected_from_401_body_text() -> None: + """A 401 carrying an OpenCode Go CreditsError is a billing issue, not a + stale credential, so it must not route the user to /login.""" + from pythinker_code.ui.shell import _is_insufficient_credits_error + + credits = APIStatusError( + 401, + "Error code: 401 - {'type': 'error', 'error': {'type': 'CreditsError', " + "'message': 'Insufficient balance. Manage your billing here: " + "https://opencode.ai/workspace/x/billing'}}", + ) + assert _is_insufficient_credits_error(credits) is True + + +def test_stale_credential_401_not_treated_as_insufficient_credits() -> None: + from pythinker_code.ui.shell import _is_insufficient_credits_error + + assert _is_insufficient_credits_error(APIStatusError(401, "incorrect API KEY")) is False + + # --------------------------------------------------------------------------- # Tests: wire server _handle_prompt # --------------------------------------------------------------------------- diff --git a/tests/core/test_default_agent.py b/tests/core/test_default_agent.py index 573ec7e4..72f02e5d 100644 --- a/tests/core/test_default_agent.py +++ b/tests/core/test_default_agent.py @@ -59,6 +59,21 @@ async def test_default_agent(runtime: Runtime): **Professional handoff format:** For substantial tasks, keep a visible plan/todo and structure work as `context -> assessment -> plan -> execution -> verification -> residual risks`. Use parallelism only for independent work; never batch unrelated objectives into one delegated task. +**Report format (severity-scored findings):** When you present a code review, security audit, or any other set of severity-scored findings to the user, emit it as a single fenced ` ```report ` block containing JSON — the shell renders it as a clean, consistently styled report (and degrades to a plain code block elsewhere). Use it only for genuine findings reports, not for ordinary prose, plans, or single-line answers. Schema: + +```report +{ + "title": "Code Review Results", + "scope": "one-line context, e.g. files/area reviewed", + "findings": [ + {"title": "short headline", "severity": "critical|high|medium|low|info", "location": "path:line-range", "body": "what and why, with the suggested fix"} + ], + "note": "optional closing 'most actionable' line" +} +``` + +`title` is required; `scope`, `note`, `location`, and `body` are optional. `severity` must be one of the five listed values. Order does not matter — the renderer groups by severity (critical first) and derives the summary tally. Put narrative prose outside the block, before or after it. + # Engineering Discipline These principles govern every engineering response. They override speed: a slow right answer beats a fast wrong one. @@ -297,6 +312,7 @@ async def test_default_agent(runtime: Runtime): "allowlist", ( "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", @@ -316,6 +332,7 @@ async def test_default_agent(runtime: Runtime): "allowlist", ( "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", @@ -331,6 +348,7 @@ async def test_default_agent(runtime: Runtime): "allowlist", ( "pythinker_code.tools.shell:Shell", + "pythinker_code.tools.todo:SetTodoList", "pythinker_code.tools.file:ReadFile", "pythinker_code.tools.file:Grep", ), @@ -343,6 +361,7 @@ async def test_default_agent(runtime: Runtime): "allowlist", ( "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", @@ -360,6 +379,7 @@ async def test_default_agent(runtime: Runtime): None, "allowlist", ( + "pythinker_code.tools.todo:SetTodoList", "pythinker_code.tools.file:ReadFile", "pythinker_code.tools.file:ReadMediaFile", "pythinker_code.tools.file:Glob", @@ -378,6 +398,7 @@ async def test_default_agent(runtime: Runtime): "allowlist", ( "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", @@ -396,6 +417,7 @@ async def test_default_agent(runtime: Runtime): "allowlist", ( "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", @@ -410,6 +432,7 @@ async def test_default_agent(runtime: Runtime): "allowlist", ( "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", @@ -430,6 +453,7 @@ async def test_default_agent(runtime: Runtime): "allowlist", ( "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", @@ -491,15 +515,15 @@ async def test_default_agent_background_bash_guardrails(runtime: Runtime): **Available Built-in Agent Types** - `mocker`: The mock agent for testing purposes. (Tools: *, Model: inherit, Background: yes). -- `coder`: Good at general software engineering tasks. (Tools: Shell, ReadFile, ReadMediaFile, Glob, Grep, SmartSearch, WriteFile, StrReplaceFile, SearchWeb, FetchURL, Model: inherit, Background: yes). When to use: 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. -- `code-reviewer`: Diff-focused code review with severity-scored findings. (Tools: Shell, ReadFile, Grep, ReadSkill, SearchWeb, FetchURL, Model: inherit, Background: yes). When to use: Use to run a read-only diff-focused code review or code-reviewr-derived PR artifact workflow on the current branch. -- `debugger`: Failure/log/stack-trace root-cause analysis with reproduction evidence. (Tools: Shell, ReadFile, Grep, Model: inherit, Background: yes). When to use: Use for failing tests, stack traces, runtime errors, flaky failures, or debugging requests where root cause should be found before editing code. -- `explore`: Fast codebase exploration with prompt-enforced read-only behavior. (Tools: Shell, ReadFile, ReadMediaFile, Glob, Grep, SmartSearch, ReadSkill, SearchWeb, FetchURL, Model: inherit, Background: yes). When to use: 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. -- `plan`: Read-only implementation planning and architecture design. (Tools: ReadFile, ReadMediaFile, Glob, Grep, SmartSearch, ReadSkill, SearchWeb, FetchURL, Model: inherit, Background: yes). 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. -- `review`: Read-only code review with severity-scored findings. (Tools: Shell, ReadFile, ReadMediaFile, Glob, Grep, SmartSearch, ReadSkill, SearchWeb, FetchURL, Model: inherit, Background: yes). When to use: 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. -- `security-reviewer`: Diff-focused security review with validated findings. (Tools: Shell, ReadFile, Grep, SearchWeb, FetchURL, Model: inherit, Background: yes). When to use: Use to run a diff-only security review on the current branch. Can run in parallel with `code-reviewer`. -- `implementer`: Scoped implementation with minimal edits and verification. (Tools: Shell, ReadFile, ReadMediaFile, Glob, Grep, SmartSearch, WriteFile, StrReplaceFile, ReadSkill, SearchWeb, FetchURL, Model: inherit, Background: yes). When to use: Use this agent when the required code change is already specified and should be implemented with minimal edits and a quick verification pass. -- `verifier`: Read-only validation runner for tests, lint, and builds. (Tools: Shell, ReadFile, ReadMediaFile, Glob, Grep, SmartSearch, ReadSkill, Model: inherit, Background: yes). When to use: Use this agent when the parent needs tests, lint, type checks, builds, or other validation gates run and reported without applying fixes. +- `coder`: Good at general software engineering tasks. (Tools: Shell, SetTodoList, ReadFile, ReadMediaFile, Glob, Grep, SmartSearch, WriteFile, StrReplaceFile, SearchWeb, FetchURL, Model: inherit, Background: yes). When to use: 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. +- `code-reviewer`: Diff-focused code review with severity-scored findings. (Tools: Shell, SetTodoList, ReadFile, Grep, ReadSkill, SearchWeb, FetchURL, Model: inherit, Background: yes). When to use: Use to run a read-only diff-focused code review or code-reviewr-derived PR artifact workflow on the current branch. +- `debugger`: Failure/log/stack-trace root-cause analysis with reproduction evidence. (Tools: Shell, SetTodoList, ReadFile, Grep, Model: inherit, Background: yes). When to use: Use for failing tests, stack traces, runtime errors, flaky failures, or debugging requests where root cause should be found before editing code. +- `explore`: Fast codebase exploration with prompt-enforced read-only behavior. (Tools: Shell, SetTodoList, ReadFile, ReadMediaFile, Glob, Grep, SmartSearch, ReadSkill, SearchWeb, FetchURL, Model: inherit, Background: yes). When to use: 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. +- `plan`: Read-only implementation planning and architecture design. (Tools: SetTodoList, ReadFile, ReadMediaFile, Glob, Grep, SmartSearch, ReadSkill, SearchWeb, FetchURL, Model: inherit, Background: yes). 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. +- `review`: Read-only code review with severity-scored findings. (Tools: Shell, SetTodoList, ReadFile, ReadMediaFile, Glob, Grep, SmartSearch, ReadSkill, SearchWeb, FetchURL, Model: inherit, Background: yes). When to use: 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. +- `security-reviewer`: Diff-focused security review with validated findings. (Tools: Shell, SetTodoList, ReadFile, Grep, SearchWeb, FetchURL, Model: inherit, Background: yes). When to use: Use to run a diff-only security review on the current branch. Can run in parallel with `code-reviewer`. +- `implementer`: Scoped implementation with minimal edits and verification. (Tools: Shell, SetTodoList, ReadFile, ReadMediaFile, Glob, Grep, SmartSearch, WriteFile, StrReplaceFile, ReadSkill, SearchWeb, FetchURL, Model: inherit, Background: yes). When to use: Use this agent when the required code change is already specified and should be implemented with minimal edits and a quick verification pass. +- `verifier`: Read-only validation runner for tests, lint, and builds. (Tools: Shell, SetTodoList, ReadFile, ReadMediaFile, Glob, Grep, SmartSearch, ReadSkill, Model: inherit, Background: yes). When to use: Use this agent when the parent needs tests, lint, type checks, builds, or other validation gates run and reported without applying fixes. **Usage** diff --git a/tests/core/test_scratchpad.py b/tests/core/test_scratchpad.py index 68b5ab17..df646eab 100644 --- a/tests/core/test_scratchpad.py +++ b/tests/core/test_scratchpad.py @@ -459,12 +459,72 @@ async def test_append_scratch_event_does_not_duplicate_session_heading(tmp_path) assert "— second" in text +def test_session_labels_collapse_single_valued_keys(tmp_path): + wd = _hp(tmp_path) + sid = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + append_scratch_event_sync( + wd, + session_id=sid, + title="a", + labels=["source:startup", "kind:todo"], + create=True, + ) + append_scratch_event_sync( + wd, + session_id=sid, + title="b", + labels=["source:resume", "kind:agent"], + create=True, + ) + text = session_scratch_path(wd, session_id=sid).read_text(encoding="utf-8") + label_line = next(line for line in text.splitlines() if line.startswith("labels:")) + # Single-valued 'source' keeps only the latest value... + assert "source:resume" in label_line + assert "source:startup" not in label_line + # ...while multi-valued 'kind' accumulates both. + assert "kind:todo" in label_line + assert "kind:agent" in label_line + + def test_append_scratch_event_sync_missing_is_noop(tmp_path): result = append_scratch_event_sync(_hp(tmp_path), title="missing") assert result.appended is False assert result.reason == "missing" +def test_session_start_event_is_idempotent_per_signature(tmp_path): + wd = _hp(tmp_path) + sid = "11111111-2222-3333-4444-555555555555" + + def emit(source: str): + return append_scratch_event_sync( + wd, + session_id=sid, + title="session start", + details=[f"source: {source}"], + dedup_signature=f"source: {source}", + create=True, + ) + + first = emit("startup") + assert first.appended is True + assert first.reason == "appended" + + # A relaunch of the same session re-runs the startup path; the duplicate + # "session start / source: startup" milestone must be suppressed. + second = emit("startup") + assert second.appended is False + assert second.reason == "deduped" + + # A genuinely different transition (resume) is still recorded. + third = emit("resume") + assert third.appended is True + assert third.reason == "appended" + + text = session_scratch_path(wd, session_id=sid).read_text(encoding="utf-8") + assert text.count("— session start") == 2 + + async def test_append_verifies_git_once_per_session(tmp_path): # Regression guard: repeated journal writes must not re-run git verification # (rev-parse/ls-files/check-ignore) on every call. diff --git a/tests/core/test_subagent_builder.py b/tests/core/test_subagent_builder.py index 4efca922..f6f259d2 100644 --- a/tests/core/test_subagent_builder.py +++ b/tests/core/test_subagent_builder.py @@ -32,7 +32,7 @@ async def test_builder_builds_coder_with_write_tools(runtime): assert "StrReplaceFile" in tool_names assert "Agent" not in tool_names assert "AskUserQuestion" not in tool_names - assert "SetTodoList" not in tool_names + assert "SetTodoList" in tool_names @pytest.mark.skipif(platform.system() == "Windows", reason="Skipping test on Windows") @@ -55,6 +55,7 @@ async def test_builder_builds_explore_read_only_with_shell(runtime): assert "Shell" in tool_names assert "ReadFile" in tool_names assert "Grep" in tool_names + assert "SetTodoList" in tool_names assert "WriteFile" not in tool_names assert "StrReplaceFile" not in tool_names assert "Agent" not in tool_names @@ -80,6 +81,7 @@ async def test_builder_builds_plan_without_shell_or_write_tools(runtime): assert "ReadFile" in tool_names assert "Glob" in tool_names assert "SearchWeb" in tool_names + assert "SetTodoList" in tool_names assert "Shell" not in tool_names assert "WriteFile" not in tool_names assert "StrReplaceFile" not in tool_names diff --git a/tests/test_homebrew_formula.py b/tests/test_homebrew_formula.py index b314c26e..581bf145 100644 --- a/tests/test_homebrew_formula.py +++ b/tests/test_homebrew_formula.py @@ -37,14 +37,15 @@ def test_native_homebrew_formula_renders_release_tarballs() -> None: assert "include Language::Python::Virtualenv" not in formula assert 'version "1.2.3"' in formula - assert "pythinker-1.2.3-aarch64-apple-darwin.tar.gz" in formula - assert "pythinker-1.2.3-x86_64-apple-darwin.tar.gz" in formula - assert "pythinker-1.2.3-aarch64-unknown-linux-gnu.tar.gz" in formula - assert "pythinker-1.2.3-x86_64-unknown-linux-gnu.tar.gz" in formula + assert "pythinker-1.2.3-aarch64-apple-darwin-onedir.tar.gz" in formula + assert "pythinker-1.2.3-x86_64-apple-darwin-onedir.tar.gz" in formula + assert "pythinker-1.2.3-aarch64-unknown-linux-gnu-onedir.tar.gz" in formula + assert "pythinker-1.2.3-x86_64-unknown-linux-gnu-onedir.tar.gz" in formula assert "on_macos do" in formula assert "on_linux do" in formula assert "on_arm do" in formula assert "on_intel do" in formula + assert 'libexec.install Dir["*"]' in formula assert '(libexec/".pythinker-native").write "pythinker-native-build\\n"' in formula assert 'bin.write_exec_script libexec/"pythinker"' in formula diff --git a/tests/ui_and_conv/test_print_final_only.py b/tests/ui_and_conv/test_print_final_only.py index ab42ca96..da7cbc89 100644 --- a/tests/ui_and_conv/test_print_final_only.py +++ b/tests/ui_and_conv/test_print_final_only.py @@ -20,6 +20,40 @@ def test_final_only_text_printer_outputs_final_text(capsys): assert capsys.readouterr().out.strip() == "final msg" +def test_final_only_text_printer_plain_prose_is_byte_identical(capsys): + """Non-report output must be unchanged — same verbatim text, no framing.""" + printer = FinalOnlyTextPrinter() + printer.feed(TextPart(text="just a plain answer")) + printer.flush() + assert capsys.readouterr().out == "just a plain answer\n" + + +def test_final_only_text_printer_renders_report_block(capsys): + """A ` ```report ` block in the final text renders as a clean report, not raw JSON.""" + printer = FinalOnlyTextPrinter() + printer.feed( + TextPart( + text=( + "Summary line.\n\n" + "```report\n" + '{"title": "Audit Results", ' + '"findings": [{"title": "SQL injection", "severity": "high", ' + '"location": "db.py:42"}]}\n' + "```\n" + ) + ) + ) + printer.flush() + + out = capsys.readouterr().out + assert "Summary line." in out + assert "Audit Results" in out + assert "1 high" in out + assert "SQL injection" in out + assert "db.py:42" in out + assert '"severity"' not in out # rendered, not raw JSON + + def test_final_only_json_printer_outputs_final_message(capsys): printer = FinalOnlyJsonPrinter() printer.feed(StepBegin(n=1)) diff --git a/tests/ui_and_conv/test_report.py b/tests/ui_and_conv/test_report.py new file mode 100644 index 00000000..ee3e3846 --- /dev/null +++ b/tests/ui_and_conv/test_report.py @@ -0,0 +1,197 @@ +"""Tests for the standardized report renderer.""" + +from __future__ import annotations + +import pytest +from rich.console import Console + +from pythinker_code.ui.shell.components.report import ( + Report, + ReportFinding, + parse_report_block, + render_agent_body, + render_report, +) + + +def _plain(renderable, *, width: int = 80) -> str: + console = Console(width=width, no_color=True, legacy_windows=False) + with console.capture() as cap: + console.print(renderable) + return cap.get() + + +# --------------------------------------------------------------------------- +# rendering +# --------------------------------------------------------------------------- + + +def _sample_report() -> Report: + return Report( + title="Code Review Results", + scope="Reviewed 17 files across 3 clusters", + findings=( + ReportFinding( + "Inconsistent scroll-indicator fix", + "medium", + location="settings_list.py:177-180", + body="The off-by-one scroll bug is unfixed in the sibling.", + ), + ReportFinding("Slow optional fetch blocks login", "medium"), + ReportFinding("Missing test for erase_when_done", "low", location="selector.py:447"), + ReportFinding("Fallback chain is well-tested", "info"), + ), + note="Most actionable — fix the settings_list scroll bug.", + ) + + +@pytest.mark.parametrize("theme", ["dark", "light"]) +def test_render_report_includes_title_sections_and_locations(theme): + out = _plain(render_report(_sample_report(), theme=theme)) + assert "Code Review Results" in out + assert "Reviewed 17 files across 3 clusters" in out + # section headers for each present severity, omitted for absent ones + assert "Medium" in out + assert "Low" in out + assert "Info" in out + assert "Critical" not in out + assert "High" not in out + # findings + locations + assert "Inconsistent scroll-indicator fix" in out + assert "settings_list.py:177-180" in out + assert "Most actionable" in out + + +def test_render_report_summary_tally_and_no_critical_high(): + out = _plain(render_report(_sample_report())) + assert "2 medium" in out + assert "1 low" in out + assert "1 info" in out + assert "no critical or high" in out + + +def test_render_report_groups_in_severity_order_regardless_of_input(): + report = Report( + title="t", + findings=( + ReportFinding("i", "info"), + ReportFinding("c", "critical"), + ReportFinding("m", "medium"), + ), + ) + out = _plain(render_report(report)) + # Critical section must appear before Medium, which appears before Info. + assert out.index("Critical") < out.index("Medium") < out.index("Info") + assert "no critical or high" not in out # critical present + + +def test_render_report_empty_findings_is_safe(): + out = _plain(render_report(Report(title="Empty report"))) + assert "Empty report" in out + assert "no critical or high" in out + + +# --------------------------------------------------------------------------- +# parse_report_block +# --------------------------------------------------------------------------- + + +def test_parse_report_block_valid(): + payload = """ + { + "title": "R", + "scope": "s", + "note": "n", + "findings": [ + {"title": "f1", "severity": "high", "location": "a.py:1", "body": "b"}, + {"title": "f2", "severity": "low"} + ] + } + """ + report = parse_report_block(payload) + assert report is not None + assert report.title == "R" + assert report.scope == "s" + assert report.note == "n" + assert len(report.findings) == 2 + assert report.findings[0].severity == "high" + assert report.findings[0].location == "a.py:1" + assert report.findings[1].location is None + + +@pytest.mark.parametrize( + "payload", + [ + "not json", + "[]", # not an object + '{"scope": "x"}', # missing title + '{"title": ""}', # empty title + '{"title": "t", "findings": "nope"}', # findings not a list + '{"title": "t", "findings": [{"title": "f"}]}', # finding missing severity + '{"title": "t", "findings": [{"title": "f", "severity": "bogus"}]}', # bad severity + '{"title": "t", "findings": ["nope"]}', # finding not an object + ], +) +def test_parse_report_block_malformed_returns_none(payload): + assert parse_report_block(payload) is None + + +# --------------------------------------------------------------------------- +# render_agent_body — the fenced-block bridge +# --------------------------------------------------------------------------- + + +def test_render_agent_body_promotes_report_fence(): + text = ( + "Here is the review.\n\n" + "```report\n" + '{"title": "My Report", "findings": [{"title": "bug", "severity": "medium"}]}\n' + "```\n\n" + "Done." + ) + out = _plain(render_agent_body(text)) + assert "Here is the review." in out + assert "My Report" in out + assert "1 medium" in out # rendered as a report, not raw JSON + assert "Done." in out + assert '"severity"' not in out # JSON payload not shown verbatim + + +def test_render_agent_body_invalid_fence_falls_back_to_markdown(): + text = "```report\nthis is not json\n```" + out = _plain(render_agent_body(text)) + # Left as an ordinary fenced code block — content preserved, not swallowed. + assert "this is not json" in out + + +def test_render_agent_body_plain_markdown_unchanged(): + out = _plain(render_agent_body("# Heading\n\nSome **text**.")) + assert "Heading" in out + assert "text" in out + + +def test_streaming_commit_keeps_report_fence_atomic_and_renders(): + """Integration contract for the live shell: the incremental renderer + (_blocks._flush_committed) commits at markdown_commit_boundary and renders + the committed slice via render_agent_body. A complete report fence must + commit whole (not split mid-JSON) so it renders as a report, not raw text. + """ + from pythinker_code.ui.shell.components.markdown import markdown_commit_boundary + + text = ( + "Here is the review.\n\n" + "```report\n" + '{"title": "Code Review Results", "findings": ' + '[{"title": "Slow fetch", "severity": "medium", "location": "x.py:1"}]}\n' + "```\n\n" + "Trailing paragraph.\n" + ) + boundary = markdown_commit_boundary(text) + assert boundary is not None + committed = text[:boundary] + # The closed fence (open + close) is fully inside the committed slice. + assert committed.count("```") == 2 + out = _plain(render_agent_body(committed)) + assert "Code Review Results" in out + assert "1 medium" in out + assert '"severity"' not in out # rendered as a report, not raw JSON diff --git a/tests/ui_and_conv/test_selectors_simple.py b/tests/ui_and_conv/test_selectors_simple.py index b539f5f4..4d1f3147 100644 --- a/tests/ui_and_conv/test_selectors_simple.py +++ b/tests/ui_and_conv/test_selectors_simple.py @@ -264,3 +264,45 @@ def test_oauth_selector_logout_title(): action="logout", ) assert "log out" in config.title.lower() or "logout" in config.title.lower() + + +# --------------------------------------------------------------------------- +# scroll viewport — selected row must stay visible past the window edge +# --------------------------------------------------------------------------- + + +def test_selector_app_erases_chrome_on_exit(): + """The selector must erase its chrome on commit/cancel so it doesn't linger + in the scrollback as a ghost menu.""" + from pythinker_code.ui.shell.selector import ( + SelectorConfig, + _build_application, # type: ignore[reportPrivateUsage] + ) + + config = SelectorConfig(title="t", items=[SelectorItem("a", "Alpha")]) + app = _build_application(_SelectorState(config)) + assert app.erase_when_done is True + + +def test_overflowing_selector_keeps_selected_row_within_window(): + """Regression: navigating down an overflowing list must not scroll the + highlighted row under the scroll indicator and off the bottom. + + visible_window() must, for every selectable index, (a) include that index + and (b) produce no more content rows (slice + scroll indicator) than the + max_visible budget the item window is sized to. + """ + from pythinker_code.ui.shell.selector import SelectorConfig, SelectorHeader + + budget = 14 + items: list[SelectorItem[str] | SelectorHeader] = [SelectorHeader("Group")] + items += [SelectorItem(f"v{i}", f"Model {i:02}") for i in range(20)] + state = _SelectorState(SelectorConfig(title="t", items=items, max_visible=budget)) + + for target in state._selectable_indices(): # type: ignore[reportPrivateUsage] + state.selected_idx = target + start, end = state.visible_window() + has_scroll_row = start > 0 or end < len(state.visible) + content_rows = (end - start) + (1 if has_scroll_row else 0) + assert start <= target < end, f"selected {target} fell outside window {(start, end)}" + assert content_rows <= budget, f"content {content_rows} exceeds budget {budget}" diff --git a/tests/ui_and_conv/test_settings_selector.py b/tests/ui_and_conv/test_settings_selector.py index 0336504d..cd519749 100644 --- a/tests/ui_and_conv/test_settings_selector.py +++ b/tests/ui_and_conv/test_settings_selector.py @@ -136,4 +136,31 @@ def test_settings_list_visible_window_centers_selected_item(): ) state.move(3) - assert state.visible_window() == (2, 5) + # An overflowing list reserves one row of the budget for the scroll + # indicator, so the slice spans budget-1 = 2 rows: (start, start+2). + assert state.visible_window() == (2, 4) + + +def test_settings_list_overflow_keeps_selected_row_within_window(): + """Regression: navigating an overflowing settings list must not scroll the + highlighted row under the scroll indicator and off the bottom of the + (max_visible-tall) window. Mirrors the selector.py viewport guarantee. + """ + budget = 10 + state = _SettingsListState( + SettingsListConfig( + title="Settings", + max_visible=budget, + items=[ + SettingItem(id=str(i), label=f"Item {i:02}", current_value="x") for i in range(25) + ], + ) + ) + + for target in range(len(state.visible)): + state.selected_idx = target + start, end = state.visible_window() + has_scroll_row = start > 0 or end < len(state.visible) + content_rows = (end - start) + (1 if has_scroll_row else 0) + assert start <= target < end, f"selected {target} fell outside window {(start, end)}" + assert content_rows <= budget, f"content {content_rows} exceeds budget {budget}" diff --git a/tests/ui_and_conv/test_shell_feedback_slash.py b/tests/ui_and_conv/test_shell_feedback_slash.py index 33edcf6f..1a6a69e4 100644 --- a/tests/ui_and_conv/test_shell_feedback_slash.py +++ b/tests/ui_and_conv/test_shell_feedback_slash.py @@ -3,89 +3,13 @@ from __future__ import annotations from collections.abc import Awaitable -from contextlib import nullcontext -from pathlib import Path -from types import SimpleNamespace -from typing import cast -from unittest.mock import AsyncMock, Mock +from unittest.mock import Mock -import aiohttp -import pytest -from pydantic import SecretStr -from pythinker_core.tooling.empty import EmptyToolset - -from pythinker_code.auth.github_feedback import GitHubIssue -from pythinker_code.config import FeedbackConfig, LLMProvider, OAuthRef -from pythinker_code.soul.agent import Agent, Runtime -from pythinker_code.soul.context import Context -from pythinker_code.soul.pythinkersoul import PythinkerSoul -from pythinker_code.ui.shell import Shell from pythinker_code.ui.shell import slash as shell_slash from pythinker_code.ui.shell.slash import registry as shell_slash_registry from pythinker_code.ui.shell.slash import shell_mode_registry -def _make_shell_app(runtime: Runtime, tmp_path: Path) -> SimpleNamespace: - agent = Agent( - name="Test Agent", - system_prompt="Test system prompt.", - toolset=EmptyToolset(), - runtime=runtime, - ) - soul = PythinkerSoul(agent, context=Context(file_backend=tmp_path / "history.jsonl")) - return SimpleNamespace(soul=soul) - - -def _setup_feedback_provider(runtime: Runtime) -> None: - """Add a managed:pythinker-code provider with OAuth to the runtime config.""" - runtime.config.providers["managed:pythinker-code"] = LLMProvider( - type="pythinker", - base_url="https://api.pythinker.com/coding/v1", - api_key=SecretStr("test-api-key"), - oauth=OAuthRef(storage="file", key="oauth/pythinker-code"), - custom_headers={"x-canary-kfc": "always"}, - ) - - -def _mock_client_session(*, response_status=204, side_effect=None): - """Create a mock for new_client_session that simulates aiohttp behavior.""" - mock_response = AsyncMock() - mock_response.status = response_status - mock_response.__aenter__ = AsyncMock(return_value=mock_response) - mock_response.__aexit__ = AsyncMock(return_value=False) - - mock_session = AsyncMock() - if side_effect: - mock_session.post = Mock(side_effect=side_effect) - else: - mock_session.post = Mock(return_value=mock_response) - mock_session.__aenter__ = AsyncMock(return_value=mock_session) - mock_session.__aexit__ = AsyncMock(return_value=False) - - return Mock(return_value=mock_session) - - -def _setup_submission_mocks(monkeypatch, *, feedback_text="Great tool!", session_factory=None): - """Common mock setup for tests that reach the HTTP submission phase.""" - print_mock = Mock() - open_mock = Mock(return_value=True) - monkeypatch.setattr(shell_slash.console, "print", print_mock) - monkeypatch.setattr(shell_slash.console, "status", lambda *_a, **_kw: nullcontext()) - monkeypatch.setattr("webbrowser.open", open_mock) - monkeypatch.setattr( - "prompt_toolkit.PromptSession.prompt_async", - AsyncMock(return_value=feedback_text), - ) - if session_factory is not None: - monkeypatch.setattr("pythinker_code.utils.aiohttp.new_client_session", session_factory) - return print_mock, open_mock - - -# --------------------------------------------------------------------------- -# Registration -# --------------------------------------------------------------------------- - - class TestFeedbackRegistration: def test_registered_in_shell_registry(self) -> None: cmd = shell_slash_registry.find_command("feedback") @@ -97,311 +21,37 @@ def test_registered_in_shell_mode_registry(self) -> None: assert cmd is not None -# --------------------------------------------------------------------------- -# Guards → fallback to GitHub issues -# --------------------------------------------------------------------------- - - -class TestFeedbackGuards: - async def test_fallback_when_no_pythinker_soul(self, monkeypatch) -> None: - """When soul is not PythinkerSoul, should fallback to GitHub issues.""" - shell = Mock() - shell.soul = Mock() # not spec=PythinkerSoul - +class TestFeedbackOpensIssue: + def test_opens_new_issue_url(self, monkeypatch) -> None: open_mock = Mock(return_value=True) monkeypatch.setattr("webbrowser.open", open_mock) + monkeypatch.setattr(shell_slash.console, "print", Mock()) - ret = shell_slash.feedback(cast(Shell, shell), "") - if isinstance(ret, Awaitable): - await ret + shell = Mock() + ret = shell_slash.feedback(shell, "") + assert not isinstance(ret, Awaitable) open_mock.assert_called_once() - assert "issues" in open_mock.call_args.args[0] - - async def test_default_endpoint_without_provider( - self, runtime: Runtime, tmp_path: Path, monkeypatch - ) -> None: - """Feedback no longer requires managed:pythinker-code OAuth.""" - app = _make_shell_app(runtime, tmp_path) - - mock_session_factory = _mock_client_session(response_status=204) - _, open_mock = _setup_submission_mocks( - monkeypatch, feedback_text="No auth feedback", session_factory=mock_session_factory - ) - - ret = shell_slash.feedback(cast(Shell, app), "") - if isinstance(ret, Awaitable): - await ret - - mock_session = await mock_session_factory.return_value.__aenter__() - post_call = mock_session.post.call_args - assert post_call.args[0] == "https://api.pythinker.com/coding/v1/feedback" - assert post_call.kwargs["headers"] == {} - open_mock.assert_not_called() - - async def test_provider_without_oauth_uses_api_key( - self, runtime: Runtime, tmp_path: Path, monkeypatch - ) -> None: - """A managed provider can authenticate feedback with only its configured API key.""" - runtime.config.providers["managed:pythinker-code"] = LLMProvider( - type="pythinker", - base_url="https://api.pythinker.com/coding/v1", - api_key=SecretStr("test-api-key"), - oauth=None, - custom_headers={"x-canary-kfc": "always"}, - ) - app = _make_shell_app(runtime, tmp_path) - - mock_session_factory = _mock_client_session(response_status=204) - _, open_mock = _setup_submission_mocks( - monkeypatch, feedback_text="API key feedback", session_factory=mock_session_factory - ) - - ret = shell_slash.feedback(cast(Shell, app), "") - if isinstance(ret, Awaitable): - await ret - - mock_session = await mock_session_factory.return_value.__aenter__() - headers = mock_session.post.call_args.kwargs["headers"] - assert headers["Authorization"] == "Bearer test-api-key" - assert headers["x-canary-kfc"] == "always" - open_mock.assert_not_called() - - -# --------------------------------------------------------------------------- -# User input -# --------------------------------------------------------------------------- - - -class TestFeedbackUserInput: - @pytest.mark.parametrize("exc_type", [KeyboardInterrupt, EOFError]) - async def test_cancelled_on_interrupt( - self, runtime: Runtime, tmp_path: Path, monkeypatch, exc_type: type - ) -> None: - _setup_feedback_provider(runtime) - app = _make_shell_app(runtime, tmp_path) + url = open_mock.call_args.args[0] + assert "TechMatrix-labs/pythinker-code" in url + assert "new" in url + def test_prints_success_when_browser_opens(self, monkeypatch) -> None: + monkeypatch.setattr("webbrowser.open", Mock(return_value=True)) print_mock = Mock() monkeypatch.setattr(shell_slash.console, "print", print_mock) - monkeypatch.setattr( - "prompt_toolkit.PromptSession.prompt_async", - AsyncMock(side_effect=exc_type), - ) - ret = shell_slash.feedback(cast(Shell, app), "") - if isinstance(ret, Awaitable): - await ret + shell_slash.feedback(Mock(), "") - assert any("cancelled" in str(call) for call in print_mock.call_args_list) - - async def test_empty_feedback_rejected( - self, runtime: Runtime, tmp_path: Path, monkeypatch - ) -> None: - _setup_feedback_provider(runtime) - app = _make_shell_app(runtime, tmp_path) + output = " ".join(str(c) for c in print_mock.call_args_list) + assert "Opening" in output or "browser" in output.lower() + def test_prints_url_when_browser_fails(self, monkeypatch) -> None: + monkeypatch.setattr("webbrowser.open", Mock(return_value=False)) print_mock = Mock() monkeypatch.setattr(shell_slash.console, "print", print_mock) - monkeypatch.setattr( - "prompt_toolkit.PromptSession.prompt_async", - AsyncMock(return_value=" "), - ) - - ret = shell_slash.feedback(cast(Shell, app), "") - if isinstance(ret, Awaitable): - await ret - - assert any("empty" in str(call) for call in print_mock.call_args_list) - -# --------------------------------------------------------------------------- -# Submission: success & failure -# --------------------------------------------------------------------------- + shell_slash.feedback(Mock(), "") - -class TestFeedbackSubmission: - async def test_successful_submission( - self, runtime: Runtime, tmp_path: Path, monkeypatch - ) -> None: - _setup_feedback_provider(runtime) - app = _make_shell_app(runtime, tmp_path) - - mock_session_factory = _mock_client_session(response_status=204) - print_mock, _ = _setup_submission_mocks( - monkeypatch, feedback_text="Great tool!", session_factory=mock_session_factory - ) - - ret = shell_slash.feedback(cast(Shell, app), "") - if isinstance(ret, Awaitable): - await ret - - # Verify success message - assert any("submitted" in str(call) for call in print_mock.call_args_list) - - # Verify request URL - mock_session = await mock_session_factory.return_value.__aenter__() - post_call = mock_session.post.call_args - assert post_call.args[0] == "https://api.pythinker.com/coding/v1/feedback" - - # Verify custom_headers are included - headers = post_call.kwargs["headers"] - assert headers["x-canary-kfc"] == "always" - assert "Authorization" in headers - - # Verify payload fields - payload = post_call.kwargs["json"] - assert payload["content"] == "Great tool!" - assert payload["session_id"] == runtime.session.id - assert "version" in payload - assert "os" in payload - assert "model" in payload - - async def test_configured_endpoint_overrides_platform( - self, runtime: Runtime, tmp_path: Path, monkeypatch - ) -> None: - runtime.config.feedback = FeedbackConfig( - endpoint_url="https://feedback.pythinker.com/submit", - api_key=SecretStr("feedback-secret"), - custom_headers={"x-feedback-source": "cli"}, - ) - app = _make_shell_app(runtime, tmp_path) - - mock_session_factory = _mock_client_session(response_status=204) - _setup_submission_mocks( - monkeypatch, feedback_text="Configured feedback", session_factory=mock_session_factory - ) - - ret = shell_slash.feedback(cast(Shell, app), "") - if isinstance(ret, Awaitable): - await ret - - mock_session = await mock_session_factory.return_value.__aenter__() - post_call = mock_session.post.call_args - assert post_call.args[0] == "https://feedback.pythinker.com/submit" - headers = post_call.kwargs["headers"] - assert headers["Authorization"] == "Bearer feedback-secret" - assert headers["x-feedback-source"] == "cli" - - async def test_github_oauth_submission_creates_issue_as_user( - self, runtime: Runtime, tmp_path: Path, monkeypatch - ) -> None: - runtime.config.feedback = FeedbackConfig( - github_client_id="github-client-id", - github_repo="owner/repo", - ) - app = _make_shell_app(runtime, tmp_path) - - print_mock, open_mock = _setup_submission_mocks( - monkeypatch, - feedback_text="GitHub feedback", - ) - load_token = Mock(return_value="github-user-token") - create_issue = AsyncMock(return_value=GitHubIssue(number=37, html_url="https://issue/37")) - login = Mock() - monkeypatch.setattr( - "pythinker_code.auth.github_feedback.load_github_feedback_token", - load_token, - ) - monkeypatch.setattr("pythinker_code.auth.github_feedback.create_github_issue", create_issue) - monkeypatch.setattr("pythinker_code.auth.github_feedback.login_github_feedback", login) - - ret = shell_slash.feedback(cast(Shell, app), "") - if isinstance(ret, Awaitable): - await ret - - create_issue.assert_awaited_once() - await_args = create_issue.await_args - assert await_args is not None - assert await_args.kwargs["title"].startswith("[Pythinker CLI] Feedback") - assert "GitHub feedback" in await_args.kwargs["body"] - assert await_args.args == ("owner/repo", "github-user-token") - login.assert_not_called() - open_mock.assert_not_called() - assert any("GitHub issue created" in str(call) for call in print_mock.call_args_list) - - async def test_github_oauth_submission_can_star_repo_with_consent( - self, runtime: Runtime, tmp_path: Path, monkeypatch - ) -> None: - runtime.config.feedback = FeedbackConfig( - github_client_id="github-client-id", - github_repo="owner/repo", - ) - app = _make_shell_app(runtime, tmp_path) - - _setup_submission_mocks(monkeypatch) - monkeypatch.setattr( - "prompt_toolkit.PromptSession.prompt_async", - AsyncMock(side_effect=["GitHub feedback", "y"]), - ) - monkeypatch.setattr( - "pythinker_code.auth.github_feedback.load_github_feedback_token", - Mock(return_value="github-user-token"), - ) - monkeypatch.setattr( - "pythinker_code.auth.github_feedback.create_github_issue", - AsyncMock(return_value=GitHubIssue(number=37, html_url="https://issue/37")), - ) - star_repo = AsyncMock() - monkeypatch.setattr("pythinker_code.auth.github_feedback.star_github_repo", star_repo) - - ret = shell_slash.feedback(cast(Shell, app), "") - if isinstance(ret, Awaitable): - await ret - - star_repo.assert_awaited_once_with("owner/repo", "github-user-token") - - async def test_timeout_fallback(self, runtime: Runtime, tmp_path: Path, monkeypatch) -> None: - _setup_feedback_provider(runtime) - app = _make_shell_app(runtime, tmp_path) - - mock_session_factory = _mock_client_session(side_effect=TimeoutError("timed out")) - print_mock, open_mock = _setup_submission_mocks( - monkeypatch, session_factory=mock_session_factory - ) - - ret = shell_slash.feedback(cast(Shell, app), "") - if isinstance(ret, Awaitable): - await ret - - assert any("timed out" in str(call) for call in print_mock.call_args_list) - open_mock.assert_called_once() - - async def test_client_error_with_status_fallback( - self, runtime: Runtime, tmp_path: Path, monkeypatch - ) -> None: - _setup_feedback_provider(runtime) - app = _make_shell_app(runtime, tmp_path) - - error = aiohttp.ClientResponseError( - request_info=Mock(), history=(), status=500, message="Internal Server Error" - ) - mock_session_factory = _mock_client_session(side_effect=error) - print_mock, open_mock = _setup_submission_mocks( - monkeypatch, session_factory=mock_session_factory - ) - - ret = shell_slash.feedback(cast(Shell, app), "") - if isinstance(ret, Awaitable): - await ret - - assert any("HTTP 500" in str(call) for call in print_mock.call_args_list) - open_mock.assert_called_once() - - async def test_network_error_fallback( - self, runtime: Runtime, tmp_path: Path, monkeypatch - ) -> None: - _setup_feedback_provider(runtime) - app = _make_shell_app(runtime, tmp_path) - - error = aiohttp.ClientConnectionError("connection refused") - mock_session_factory = _mock_client_session(side_effect=error) - print_mock, open_mock = _setup_submission_mocks( - monkeypatch, session_factory=mock_session_factory - ) - - ret = shell_slash.feedback(cast(Shell, app), "") - if isinstance(ret, Awaitable): - await ret - - assert any("Network error" in str(call) for call in print_mock.call_args_list) - open_mock.assert_called_once() + output = " ".join(str(c) for c in print_mock.call_args_list) + assert "TechMatrix-labs/pythinker-code" in output diff --git a/tests/ui_and_conv/test_tui_components.py b/tests/ui_and_conv/test_tui_components.py index 26ccc9b4..a68cb4ec 100644 --- a/tests/ui_and_conv/test_tui_components.py +++ b/tests/ui_and_conv/test_tui_components.py @@ -52,6 +52,18 @@ def test_truncate_to_width_handles_cjk(): assert cell_width(out) <= 5 +def test_truncate_to_width_can_pad_to_exact_width(): + out = truncate_to_width("hi", 5, pad=True) + assert out == "hi " + assert cell_width(out) == 5 + + +def test_truncate_to_width_pads_truncated_output_to_exact_width(): + out = truncate_to_width("hello world", 8, pad=True) + assert out.endswith("…") or out.rstrip().endswith("…") + assert cell_width(out) == 8 + + def test_truncate_middle_to_visual_lines_preserves_head_and_tail(): result = truncate_middle_to_visual_lines( "\n".join(f"line {i}" for i in range(8)), @@ -116,8 +128,13 @@ def test_sanitize_ansi_strips_osc_with_st(): assert "linkafter" in cleaned -def test_sanitize_ansi_keeps_newlines_tabs(): - raw = "line 1\n\tline 2" +def test_sanitize_ansi_strips_apc_with_bel(): + raw = "before\x1b_pi:c\x07after" + assert sanitize_ansi(raw) == "beforeafter" + + +def test_sanitize_ansi_keeps_newlines_tabs_and_strips_carriage_returns(): + raw = "line 1\r\n\tline 2\r" assert sanitize_ansi(raw) == "line 1\n\tline 2"