feat(review): add deterministic reviewer target resolution - #208
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReviewer subagents now receive structured, deterministic Git targets. Scopes are validated, Git metadata is bounded and escaped, and resolved targets propagate through foreground, background, and multi-agent execution with HEAD-drift checks. ChangesReviewer Git scope pipeline
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant AgentTool
participant resolve_review_target
participant ForegroundRunner
participant BackgroundTaskManager
participant prepare_soul
AgentTool->>resolve_review_target: resolve structured review_target
resolve_review_target-->>AgentTool: ResolvedReviewTarget
AgentTool->>ForegroundRunner: launch with resolved target
AgentTool->>BackgroundTaskManager: create task with resolved target
ForegroundRunner->>prepare_soul: prepare SubagentRunSpec
BackgroundTaskManager->>prepare_soul: prepare background SubagentRunSpec
prepare_soul-->>ForegroundRunner: composed review prompt
prepare_soul-->>BackgroundTaskManager: composed review prompt
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
elkaix
marked this pull request as ready for review
July 16, 2026 00:16
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pythinker_code/subagents/git_context.py`:
- Around line 170-176: Only derive and append the project name in the
approved-host branch of the remote URL handling, after _sanitize_remote_url
returns a valid value. Update the logic around _sanitize_remote_url and
_parse_project_name so rejected hosts cannot expose project names, while
preserving existing escaping and limits for approved remotes.
In `@tests/background/test_manager.py`:
- Around line 393-469: The test should verify prompt and resolved-target
transport through the public background-task flow rather than directly invoking
BackgroundAgentRunner._run_core. Launch the task via create_agent_task, stub
only the model-execution boundary, and assert the resulting observable prompt
snapshot or output contains both the original caller prompt and
resolved_review_target; remove mocks and assertions tied to prepare_soul,
_mark_task_running, _prepare_isolation_worktree, and SubagentRunSpec internals.
In `@tests/subagents/test_review_target.py`:
- Around line 373-440: Rewrite these tests to exercise the public
resolve_review_target entry point rather than patching _run_resolver_git or
invoking _resolve_commit, _resolve_head, and _try_resolve_commit directly.
Provide controlled Git results through the run_git boundary, preserving
assertions for fatal verification errors, missing refs, quiet invocation
behavior, and sanitized error messages by checking the resulting public resolver
outcomes. Apply the same change to the tests in the referenced additional range.
- Line 158: Correct the assertion in the relevant test by checking the prompt’s
actual field spelling, selected_base_ref, when verifying that main was not
selected; otherwise remove the trivially true assertion. Keep the test’s
intended protection against accidental selection of main.
In `@tests/tools/test_agent_tool.py`:
- Around line 2434-2448: Remove the `_journal_foreground_agent_start` mock and
the `events` tracking used only to assert journaling order in the affected test.
Keep the `resolve_before_allocation` assertion that
`runtime.subagent_store.list_instances() == []`, since it verifies the
observable resolution-before-allocation contract without coupling the test to
private implementation sequencing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ab4c8aa-374b-4f51-92d3-24399ab92b33
⛔ Files ignored due to path filters (6)
docs/en/release-notes/changelog.mdis excluded by!docs/**docs/superpowers/plans/2026-07-15-deterministic-review-target-resolution.mdis excluded by!docs/**docs/superpowers/specs/2026-07-15-review-target-resolution-design.mdis excluded by!docs/**tasks/agent-harness-adoption-plan.mdis excluded by!tasks/**tasks/lessons.mdis excluded by!tasks/**tasks/todo.mdis excluded by!tasks/**
📒 Files selected for processing (21)
CHANGELOG.mdsrc/pythinker_code/background/agent_runner.pysrc/pythinker_code/background/manager.pysrc/pythinker_code/subagents/core.pysrc/pythinker_code/subagents/git_context.pysrc/pythinker_code/subagents/review_target.pysrc/pythinker_code/subagents/runner.pysrc/pythinker_code/tools/agent/__init__.pysrc/pythinker_code/tools/agent/description.mdsrc/pythinker_code/utils/trust.pytests/background/test_manager.pytests/background/test_task_metadata.pytests/core/test_default_agent.pytests/core/test_prepare_soul.pytests/subagents/test_git_context_gate.pytests/subagents/test_review_target.pytests/test_git_context.pytests/tools/test_agent_tool.pytests/tools/test_tool_descriptions.pytests/tools/test_tool_schemas.pytests/utils/test_trust.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pythinker_code/subagents/core.py`:
- Around line 126-128: Update _compose_review_prompt to escape or neutralize
reserved review-task and review-target boundary tags in caller_prompt before
interpolation, preventing forged target sections while preserving the
authoritative target.prompt. Add a regression test verifying malicious caller
input still produces exactly one <review-target> block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2caa79c7-b0e3-4e7a-b1d5-9a00e08bc46c
⛔ Files ignored due to path filters (2)
tasks/lessons.mdis excluded by!tasks/**tasks/todo.mdis excluded by!tasks/**
📒 Files selected for processing (8)
src/pythinker_code/subagents/core.pysrc/pythinker_code/subagents/git_context.pysrc/pythinker_code/subagents/review_target.pysrc/pythinker_code/tools/agent/__init__.pytests/background/test_manager.pytests/subagents/test_review_target.pytests/test_git_context.pytests/tools/test_agent_tool.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
No issue was specified.
Description
Reviewer subagents previously inferred their Git scope from free-form prompt text and could
recompute a different merge base after dispatch. This change adds a structured, pre-dispatch
target contract for
review,code-reviewer, andsecurity-reviewer.auto,uncommitted,base, andcommittargets to explicit Git anchors before childallocation.
background execution.
handling.
HEADis pinned and revalidated, while index/worktreecontent remains live.
This gives reviewer children deterministic scope without coupling them to the standalone review
engine or changing non-reviewer behavior.
Validation
make check-pythinker-code— passed; Ruff and ty clean, 1,262 files formatted, Pyright 0errors/0 warnings/0 informations.
289 passed, 1 warning.7131 passed, 9 skipped, 1 deselected, 1 xfailed.tests_e2esuite —65 passed, 4 skipped.C01/C03/C05/C06/C08/C12/C13/C14/C15 passed.
git diff --check origin/main...HEAD— clean.Known Baseline Failure
make test-pythinker-coderemains nonzero solely at:tests/e2e/test_shell_pty_e2e.py::test_shell_cancel_running_command_kills_process_and_recoversThe isolated test fails identically on this branch and clean
main: Escape is ignored and thefive-second command completes. The branch Make run reported
1 failed, 7131 passed, 9 skipped, 1 xfailedbefore its separatetests_e2ephase. This is documented as a repository baseline blocker,not a passing gate or feature-owned regression.
Checklist
background behavior.
CHANGELOG.mdUnreleased entry and synced the generated docs changelog.make test-pythinker-codeis green — blocked by the reproducible clean-main PTY failuredescribed above.
Summary by CodeRabbit
review_targetoption for fresh reviewer agents, including propagation through foreground/background runs and orchestration fingerprints.review_target.