fix(review): address PR feedback

Author: elkaix

src/pythinker_code/subagents/core.py

@@ -124,6 +124,7 @@ def _prepend_output_language_instruction(prompt: str) -> str:
 
 
 def _compose_review_prompt(caller_prompt: str, target: ResolvedReviewTarget) -> str:
+    """Keep caller instructions subordinate to the authoritative resolved target."""
     return f"<review-task>\n{caller_prompt}\n</review-task>\n\n{target.prompt}"
 
 

src/pythinker_code/subagents/git_context.py

@@ -37,12 +37,14 @@ class GitCommandResult:
 
 class GitCommandError(RuntimeError):
     def __init__(self, category: Literal["spawn", "timeout"], command: str) -> None:
+        """Create a safe Git failure that omits arguments and raw process output."""
         self.category = category
         self.command = command
         super().__init__(f"git {command} {category} failure")
 
 
 async def _read_bounded(stream: AsyncReadable, limit: int) -> tuple[bytes, bool]:
+    """Drain a stream to EOF while retaining at most ``limit`` bytes."""
     kept = bytearray()
     truncated = False
     while chunk := await stream.read(65536):
@@ -55,6 +57,7 @@ async def _read_bounded(stream: AsyncReadable, limit: int) -> tuple[bytes, bool]
 async def _collect_process(
     proc: HostProcess, limit: int
 ) -> tuple[int, tuple[bytes, bool], tuple[bytes, bool]]:
+    """Wait for a process while draining both bounded output streams concurrently."""
     async with asyncio.TaskGroup() as tasks:
         wait_task = tasks.create_task(proc.wait())
         stdout_task = tasks.create_task(_read_bounded(proc.stdout, limit))
@@ -63,9 +66,10 @@ async def _collect_process(
 
 
 async def _await_cleanup_step(awaitable: Awaitable[object]) -> bool:
+    """Run one bounded cleanup step without replacing the primary failure."""
     try:
         await asyncio.wait_for(awaitable, timeout=_CLEANUP_STEP_TIMEOUT)
-    except BaseException:
+    except (Exception, asyncio.CancelledError):
         return False
     return True
 
@@ -75,7 +79,7 @@ async def _cleanup_process(
     completion: asyncio.Task[tuple[int, tuple[bytes, bool], tuple[bytes, bool]]] | None,
 ) -> None:
     """Terminate, drain, and reap without replacing the primary failure."""
-    with suppress(BaseException):
+    with suppress(Exception, asyncio.CancelledError):
         if proc.returncode is None:
             await _await_cleanup_step(proc.kill())
 
@@ -85,15 +89,15 @@ async def _cleanup_process(
     if completion_succeeded:
         return
 
-    with suppress(BaseException):
+    with suppress(Exception, asyncio.CancelledError):
         await _await_cleanup_step(
             asyncio.gather(
                 _read_bounded(proc.stdout, 0),
                 _read_bounded(proc.stderr, 0),
                 return_exceptions=True,
             )
         )
-    with suppress(BaseException):
+    with suppress(Exception, asyncio.CancelledError):
         await _await_cleanup_step(proc.wait())
 
 
@@ -104,6 +108,7 @@ async def run_git(
     timeout: float = _TIMEOUT,
     max_output_bytes: int = _MAX_GIT_OUTPUT_BYTES,
 ) -> GitCommandResult:
+    """Run Git with bounded output and typed spawn or timeout failures."""
     if max_output_bytes < 1:
         raise ValueError("max_output_bytes must be positive")
     proc: HostProcess | None = None
@@ -171,9 +176,9 @@ async def collect_git_context(work_dir: HostPath, *, include_merge_base: bool =
         safe_url = _sanitize_remote_url(remote_url)
         if safe_url:
             sections.append(f"Remote: {escape_prompt_data(safe_url, max_chars=1024)}")
-        project = _parse_project_name(remote_url)
-        if project:
-            sections.append(f"Project: {escape_prompt_data(project, max_chars=512)}")
+            project = _parse_project_name(safe_url)
+            if project:
+                sections.append(f"Project: {escape_prompt_data(project, max_chars=512)}")
     if branch:
         sections.append(f"Branch: {escape_prompt_data(branch, max_chars=512)}")
     if include_merge_base:

src/pythinker_code/subagents/review_target.py

@@ -56,6 +56,7 @@ class WorktreeChanges(BaseModel):
 
     @property
     def any(self) -> bool:
+        """Return whether the index or worktree contains any reviewable change."""
         return self.staged or self.unstaged or self.untracked
 
 
@@ -95,12 +96,14 @@ class ReviewTargetErrorCode(StrEnum):
 
 class ReviewTargetResolutionError(RuntimeError):
     def __init__(self, code: ReviewTargetErrorCode, brief: str, message: str) -> None:
+        """Create a categorized resolution failure with safe user-facing text."""
         self.code = code
         self.brief = brief
         super().__init__(message)
 
 
 def validate_review_target(target: ReviewTarget) -> None:
+    """Reject unsupported modes, fields, and unsafe or malformed refs."""
     if target.model_extra:
         raise ReviewTargetResolutionError(
             ReviewTargetErrorCode.invalid_target,
@@ -142,6 +145,7 @@ def validate_review_target(target: ReviewTarget) -> None:
 
 
 async def _run_resolver_git(args: list[str], cwd: str) -> GitCommandResult:
+    """Run Git and map host failures to review-target error categories."""
     try:
         return await run_git(args, cwd)
     except GitCommandError as exc:
@@ -159,6 +163,7 @@ async def _run_resolver_git(args: list[str], cwd: str) -> GitCommandResult:
 
 
 def _require_oid(result: GitCommandResult, *, message: str) -> str:
+    """Return a normalized full object ID or fail closed on malformed output."""
     value = result.stdout
     if value.endswith("\n"):
         value = value[:-1]
@@ -181,6 +186,7 @@ def _require_oid(result: GitCommandResult, *, message: str) -> str:
 
 
 def _quiet_verification_is_missing(result: GitCommandResult) -> bool:
+    """Recognize Git's exact quiet-verification response for a missing ref."""
     return (
         result.returncode == 1
         and not result.stdout
@@ -191,6 +197,7 @@ def _quiet_verification_is_missing(result: GitCommandResult) -> bool:
 
 
 def _raise_commit_verification_failed() -> None:
+    """Raise the sanitized failure shared by fatal commit verification paths."""
     raise ReviewTargetResolutionError(
         ReviewTargetErrorCode.git_failed,
         "Review target unavailable",
@@ -199,6 +206,7 @@ def _raise_commit_verification_failed() -> None:
 
 
 async def _try_resolve_commit(cwd: str, ref: str) -> str | None:
+    """Resolve a commit ref, returning ``None`` only for an exact quiet miss."""
     result = await _run_resolver_git(
         ["rev-parse", "--verify", "--quiet", "--end-of-options", f"{ref}^{{commit}}"],
         cwd,
@@ -211,6 +219,7 @@ async def _try_resolve_commit(cwd: str, ref: str) -> str | None:
 
 
 async def _resolve_commit(cwd: str, ref: str) -> str:
+    """Resolve a required commit ref or raise a typed safe failure."""
     result = await _run_resolver_git(
         ["rev-parse", "--verify", "--quiet", "--end-of-options", f"{ref}^{{commit}}"], cwd
     )
@@ -226,6 +235,7 @@ async def _resolve_commit(cwd: str, ref: str) -> str:
 
 
 async def _resolve_head(cwd: str) -> str:
+    """Validate the worktree and return its full HEAD commit ID."""
     repository = await _run_resolver_git(
         ["rev-parse", "--is-inside-work-tree"],
         cwd,
@@ -253,6 +263,7 @@ async def _resolve_head(cwd: str) -> str:
 
 
 async def _quiet_diff_changed(cwd: str, args: list[str]) -> bool:
+    """Interpret Git's quiet-diff exit contract without accepting other failures."""
     result = await _run_resolver_git(args, cwd)
     if result.returncode in {0, 1}:
         return result.returncode == 1
@@ -264,6 +275,7 @@ async def _quiet_diff_changed(cwd: str, args: list[str]) -> bool:
 
 
 async def _worktree_changes(cwd: str) -> WorktreeChanges:
+    """Collect staged, unstaged, and untracked change presence."""
     staged = await _quiet_diff_changed(
         cwd,
         ["diff", "--quiet", "--cached", "--no-ext-diff", "--no-textconv", "--exit-code", "--"],
@@ -289,6 +301,7 @@ async def _worktree_changes(cwd: str) -> WorktreeChanges:
 
 
 async def _merge_base(cwd: str, head_sha: str, base_sha: str) -> str | None:
+    """Resolve the merge base, distinguishing unrelated histories from Git failures."""
     result = await _run_resolver_git(["merge-base", head_sha, base_sha], cwd)
     if result.returncode == 1:
         return None
@@ -302,6 +315,7 @@ async def _merge_base(cwd: str, head_sha: str, base_sha: str) -> str | None:
 
 
 async def _base_has_tracked_changes(cwd: str, merge_base_sha: str) -> bool:
+    """Return whether tracked content differs from the selected merge base."""
     return await _quiet_diff_changed(
         cwd,
         [
@@ -317,6 +331,7 @@ async def _base_has_tracked_changes(cwd: str, merge_base_sha: str) -> bool:
 
 
 async def _commit_details(cwd: str, target_sha: str) -> tuple[tuple[str, ...], str]:
+    """Return validated parent IDs and an escaped title for a resolved commit."""
     parents_result = await _run_resolver_git(
         ["rev-list", "--parents", "-n", "1", target_sha, "--"],
         cwd,
@@ -361,6 +376,7 @@ async def _commit_details(cwd: str, target_sha: str) -> tuple[tuple[str, ...], s
 
 
 def _review_target_block(lines: list[str]) -> str:
+    """Wrap resolved target facts in the authoritative prompt boundary."""
     body = "\n".join(lines)
     return (
         "<review-target>\n"
@@ -376,13 +392,15 @@ def _review_target_block(lines: list[str]) -> str:
 
 
 def _requested_lines(target: ReviewTarget) -> list[str]:
+    """Render the caller's validated requested mode and optional ref."""
     lines = [f"requested_mode: {target.kind}"]
     if target.ref is not None:
         lines.append(f"requested_ref: {escape_prompt_data(target.ref, max_chars=1024)}")
     return lines
 
 
 def _attempted_line(attempted: tuple[str, ...]) -> str | None:
+    """Render attempted default bases as escaped untrusted metadata."""
     if not attempted:
         return None
     rendered = ", ".join(escape_prompt_data(ref, max_chars=1024) for ref in attempted)
@@ -401,6 +419,7 @@ def _resolved_live_target(
     merge_base_sha: str | None = None,
     auto_note: str | None = None,
 ) -> ResolvedReviewTarget:
+    """Build a live worktree or base target anchored to the current HEAD."""
     anchor = head_sha if kind == "uncommitted" else merge_base_sha
     assert anchor is not None
     lines = [
@@ -431,11 +450,11 @@ def _resolved_live_target(
     lines.extend(
         [
             "scope: inspect tracked changes from the anchor through the live index/worktree, "
-            "then inspect every relevant untracked path reported by status.",
+            + "then inspect every relevant untracked path reported by status.",
             f"command: git diff --no-ext-diff --no-textconv {anchor} --",
             "command: git status --short --untracked-files=all --",
             "Live warning: concurrent index/worktree edits can change the inspected patch; this "
-            "target does not claim a frozen snapshot.",
+            + "target does not claim a frozen snapshot.",
         ]
     )
     safe_base = escape_prompt_data(base_ref, max_chars=1024) if base_ref is not None else None
@@ -471,6 +490,7 @@ async def _resolved_commit_target(
     attempted: tuple[str, ...] = (),
     auto_note: str | None = None,
 ) -> ResolvedReviewTarget:
+    """Build an immutable commit target with validated parent metadata."""
     parent_shas, title = await _commit_details(cwd, target_sha)
     lines = [
         *_requested_lines(requested),
@@ -538,6 +558,7 @@ async def resolve_review_target(
     target: ReviewTarget,
     work_dir: HostPath,
 ) -> ResolvedReviewTarget:
+    """Resolve one validated request to a deterministic, executable Git scope."""
     validate_review_target(target)
     cwd = str(work_dir)
     head_sha = await _resolve_head(cwd)
@@ -682,6 +703,7 @@ async def revalidate_review_target_head(
     target: ResolvedReviewTarget,
     work_dir: HostPath,
 ) -> None:
+    """Reject a live target when HEAD moved after initial resolution."""
     if target.worktree_state != "live":
         return
     current_head = await _resolve_head(str(work_dir))

src/pythinker_code/tools/agent/__init__.py

@@ -435,6 +435,7 @@ def check_execution_policy(self, subagent_type: str) -> ToolError | None:
     async def _prepare_review_target(
         self, params: Params, requested_type: str
     ) -> ResolvedReviewTarget | ToolError | None:
+        """Resolve fresh reviewer scope before allocating an agent instance."""
         if params.resume is not None:
             if params.review_target is not None:
                 return ToolError(
@@ -604,6 +605,7 @@ async def _run_in_background(
         *,
         resolved_review_target: ResolvedReviewTarget | None,
     ) -> ToolReturnValue:
+        """Launch a background agent while preserving its resolved review target."""
         assert self._runtime.subagent_store is not None
         try:
             tool_call = get_current_tool_call_or_none()

tasks/lessons.md

@@ -53,6 +53,11 @@ Format: trigger → rule.
 
 ## Review orchestration
 
+- **When asked to apply all PR review feedback**, wait for the review bot's status on the current
+  head to become terminal, fetch unresolved thread-level state, and verify each recommendation
+  against runtime contracts before editing; after the push, re-check the new head rather than
+  treating the prior bot success as transferable.
+
 - **When running review/security subagents**, use the project-scoped agents in
   `.claude/agents/` (global `~/.claude/agents/security-reviewer.md` and
   `planner.md` describe the *other* Pythinker project — FastAPI/Vue/Mongo —

tasks/todo.md

@@ -2,6 +2,38 @@
 
 ## Active
 
+### PR #208 review remediation (2026-07-15)
+
+- [x] Fetch current CI, CodeRabbit, Code Quality, Codecov, and unresolved thread state.
+- [x] Classify all 11 inline findings and advisory pre-merge notices against repository standards.
+- [x] Add failing regressions for approved behavioral findings before production edits.
+- [x] Fix remote metadata authorization, cleanup exception handling, and review-target rendering.
+- [x] Rewrite over-mocked tests through public APIs and clear static test-quality findings.
+- [x] Run focused coverage, `make check-pythinker-code`, and `make test-pythinker-code`.
+- [x] Perform final diff review and prepare the verified commit without tool trailers.
+
+Acceptance: unapproved remotes cannot leak project metadata; cleanup preserves cancellation semantics
+without swallowing process-control exceptions; tests assert supported public behavior; every inline
+finding has a verified disposition; and the pushed head has fresh local gate evidence.
+
+#### Review: PR #208 review remediation
+
+- **Outcome:** all 11 fetched inline findings were addressed: unapproved remote metadata no longer
+  exposes a project identity; cleanup preserves cancellation while allowing process-control
+  exceptions to propagate; review tests use supported public boundaries; and static style findings
+  are cleared.
+- **TDD evidence:** the remote-metadata and process-control regressions initially failed together
+  (`2 failed, 52 passed`) and passed after the production fixes (`54 passed`).
+- **Focused verification:** the full changed-feature set passed `304 passed, 1 warning`; focused
+  coverage reported zero missing statements in `git_context.py` and `review_target.py`.
+- **Static verification:** `make check-pythinker-code` passed with Ruff clean, `1262 files already
+  formatted`, Pyright `0 errors, 0 warnings, 0 informations`, and ty clean.
+- **Repository test gate:** `make test-pythinker-code` passed with `7147 passed, 9 skipped, 1
+  xfailed, 5 warnings` plus `65 passed, 4 skipped, 1 warning` in `tests_e2e`.
+- **Review verdict:** independent final review found no Critical, Important, or Minor findings;
+  `git diff --check` was silent. Ready to push to the existing PR branch.
+- **Blockers:** none.
+
 ### Deterministic reviewer target resolution (2026-07-15)
 
 - [x] Reconcile the adoption ledger with current code and Git history.