feat: ImplementAndJudge chain, per-tool error reporting, and minimum-diff judge rubric - #178
Merged
Conversation
The new chain tool wraps AgentTool twice (implementer first, then judge with
the implementer's output baked into the packet) and optionally re-invokes the
implementer once on NEEDS_WORK with the judge's REQUIRED FIXES section
isolated under a '## Revision brief' heading. Two implementer invocations is
the hard cap; higher max_revisions values clamp to one.
Verdict parsing anchors on the judge's SUMMARY heading so preamble tokens
('PASS for the brief but...', 'BLOCKED would be overkill...') cannot outrank
the real verdict, and missing/SUMMARY-without-token fails closed to BLOCKED
rather than silently passing. The implementer's <coding_artifact> block is
extracted as data (never as instructions) and passed to the judge. Required
fixes are framed as untrusted feedback in the revision prompt so an embedded
directive in the judge text cannot steer the write-privileged implementer.
The chain refuses to launch from a non-root role, validates both child
subagent types and their execution policy / required MCP servers up front, and
reuses the orchestration-approval pattern from RunAgents so a session-approved
chain does not re-prompt per child.
Bad tool paths in agent.yaml used to surface as a bare 'Invalid tools: [...]' with the actual reason (module missing, class missing, or constructor exception) buried in the log file. The aggregated error now lists each failing tool with its per-tool reason, and a class-name miss logs a 'Did you mean <closest>?' hint. A constructor exception on one tool is now caught per-tool so the user gets one clear error naming the offending tool and the exception type instead of a bare traceback out of agent load. The whole load still aborts on any failure - agent.yaml tool references are hard requirements - but the message is now diagnosable from the traceback alone. This unblocks the stale-binary failure mode that hits users who build pythinker before a new tool lands.
…bric - default/agent.yaml: register pythinker_code.tools.agent:ImplementAndJudge alongside Agent and RunAgents so the chain is available out of the box without a custom agent spec. - default/judge.yaml: add the coding_artifact BLOCKED gate to the Context Gate (the judge must see the implementer's <coding_artifact> block when judging a code-change summary) and a Minimum-diff rubric dimension applying the reduction ladder (skip-need, reuse-stdlib, use-native, use-installed-dep, one-line, minimum) to every non-trivial diff the judge reviews. The ladder applies uniformly across review modes - no mode switch on the judge. - default/system.md: document the ImplementAndJudge default in the implementation playbook and the judge-gate guidance so parent agents prefer the chain over ad-hoc implementer+judge fan-out. - tests/core/test_agent_spec.py + test_default_agent.py: assert the new tool appears in the default agent's tool list across the spec snapshot tests so the registration does not drift.
Two static default skills replace ad-hoc prose in the system prompt with explicit, versionable content: - judge-minimum-diff: the reduction-ladder rubric the judge applies as a dimension on every non-trivial diff (skip-need, reuse-stdlib, use-native, use-installed-dep, one-line, minimum). - judge-overengineering-review: the parent-facing review checklist that walks through the same ladder before declaring a non-trivial change done. Both ship as skills/ directory entries so ReadSkill can load them on demand rather than bloating the always-on system prompt. PyInstaller datas entries are added in tests/utils/test_pyinstaller_utils.py so the PyInstaller one-file build picks them up alongside the other bundled skills.
…r reporting
- tests/core/test_implement_judge_chain.py (new): unit tests for the
chain's load-bearing primitives - verdict parsing (PASS / NEEDS_WORK /
BLOCKED, preamble-token-resistance, missing SUMMARY fails closed to
BLOCKED), artifact extraction, REQUIRED FIXES section isolation,
fingerprint stability across revisions, and pydantic param validation.
- tests/tools/test_implement_judge_load.py (new): regression test for
the loader path - confirms ImplementAndJudgeTool instantiates through
PythinkerToolset._load_tool with the same dependency-injection as
AgentTool so a default-agent startup cannot regress.
- tests/test_judge_branding.py (new): asserts the judge system prompt
mentions the minimum-diff rubric / reduction ladder so the always-on
policy does not drift away from the bundled skills.
- tests/core/test_load_agent.py: extend test_load_tools_invalid to
assert the aggregated error names the reason ('class or module not
found'), and add test_load_tools_aggregates_constructor_errors to
cover the new per-tool exception path with a monkeypatched _load_tool
that raises - the same per-tool-catch handles class-miss, module-miss,
and constructor-exception cases.
…set error reporting, and judge rubric User-facing bullets under ## Unreleased: - InvalidToolError now names the failing tool and the reason (per-tool aggregation + 'Did you mean' hint + per-tool exception catch). - Auto-chain implementer -> judge via ImplementAndJudge tool for non-trivial scoped edits; two implementer invocations is the hard cap. - Judge adopts the minimum-diff rubric dimension applied uniformly across review modes. - Bundled judge-minimum-diff and judge-overengineering-review default skills replace ad-hoc system prompt prose. The 'changelog-entry-required' CI check requires this for any change to shipped paths; without it the PR fails before review.
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 selected for processing (4)
📝 WalkthroughWalkthroughIntroduces ChangesImplementAndJudge chain, judge rubrics, agent wiring, and comprehensive tests
OpenAI Responses system-role normalization
Background-task UI refresh gating and update banner logic
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…lan doc - src/pythinker_code/tools/agent/__init__.py, tests/core/test_implement_judge_chain.py: s/unparseable/unparsable/ in two docstrings. The crate-ci/typos CI check flags 'unparseable' as a typo; 'unparsable' is the standard form. - tasks/implementer-judge-chain-plan.md: delete the tracked plan doc. It leaked the upstream 'ponytail' brand and is not part of the shipped artifact. - tests/test_judge_branding.py: remove the now-deleted plan doc from _BRAND_GUARD_TARGETS so test_brand_guard_targets_exist does not flag a stale entry.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![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: 6
🤖 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/agents/default/judge.yaml`:
- Line 43: The Minimum-diff rule in the judge.yaml file currently permits new
dependencies with only a one-line justification, which is inconsistent with the
repository's enforced dependency policy that requires maintainer approval plus
explicit security/approval justification. Update the Minimum-diff dependency
rule text (in the section starting with "Minimum-diff:") to require maintainer
approval and explicit inline justification/security/approval fields instead of
just a one-line justification, ensuring it aligns with the zero-new-bundled-deps
workflow that CI actually enforces. Also apply the same correction to any
mirrored skill text that references this rule.
In `@src/pythinker_code/tools/agent/__init__.py`:
- Around line 1138-1144: The documentation for the max_revisions field claims
higher values are "clamped" to MAX_IMPLEMENT_JUDGE_REVISIONS, but the schema
constraint le=MAX_IMPLEMENT_JUDGE_REVISIONS will reject values above the cap
instead of clamping them. Either remove the word "clamped" from the field
description to accurately reflect the rejection behavior, or implement actual
clamping logic in the parsing mechanism (using a validator or field_validator)
that accepts values above the cap and silently clamps them to
MAX_IMPLEMENT_JUDGE_REVISIONS while keeping the description as-is.
- Around line 1160-1161: The orchestration fingerprint dictionary includes
revision_index which causes the fingerprint to change for each revision attempt,
triggering re-approval requests on NEEDS_WORK retries. Remove revision_index
from the fingerprint dictionary (around line 1160 where the fingerprint is
constructed) so that all revisions of the same orchestration share the same
fingerprint and approval, preventing redundant approval prompts and maintaining
consistent orchestration behavior across retries.
- Around line 1172-1176: The verdict parsing in the code searches for the
verdict token starting from the end of the SUMMARY match to the end of output,
which allows verdicts from sections after SUMMARY to be accepted as valid. To
fix this, change the _IMPLEMENT_JUDGE_VERDICT_RE.search() call to search only
within the SUMMARY section itself by either searching within the summary.group()
text or limiting the search range to the SUMMARY match's boundaries (from
summary.start() to summary.end()). This ensures the verdict token must appear
within the SUMMARY section to be accepted, otherwise the function will correctly
return BLOCKED as the fail-closed default.
In `@tests/core/test_implement_judge_chain.py`:
- Line 53: The spell-check CI validation is failing on the word "unparseable"
used in the comment describing the judge chain behavior. Change the word
"unparseable" to "unparsable" in the comment that states "The chain must never
silently treat an unparseable judge reply as a pass" to use the correct spelling
and unblock the pipeline.
In `@tests/ui_and_conv/test_visualize_running_prompt.py`:
- Around line 2347-2360: Remove the monkeypatch of the private
_background_status_metadata helper method in the
test_background_status_truncates_after_dropping_metadata function, as it creates
unnecessary coupling to implementation details and triggers the Ruff ARG005
warning for the unused now parameter. Instead, configure the test to trigger the
truncation behavior through legitimate token-state inputs on the
CustomPromptSession object (such as _latest_todos or other public-facing state
properties), and verify the truncation behavior through the same rendered output
assertions without mocking internal methods.
🪄 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: 462fe7e6-b112-4c7c-9844-52392a9278b9
⛔ Files ignored due to path filters (1)
tasks/implementer-judge-chain-plan.mdis excluded by!tasks/**
📒 Files selected for processing (21)
CHANGELOG.mdpackages/pythinker-core/src/pythinker_core/contrib/chat_provider/openai_responses.pypackages/pythinker-core/tests/api_snapshot_tests/test_openai_responses.pysrc/pythinker_code/agents/default/agent.yamlsrc/pythinker_code/agents/default/judge.yamlsrc/pythinker_code/agents/default/system.mdsrc/pythinker_code/skills/judge-minimum-diff/SKILL.mdsrc/pythinker_code/skills/judge-overengineering-review/SKILL.mdsrc/pythinker_code/soul/toolset.pysrc/pythinker_code/tools/agent/__init__.pysrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/prompt.pytests/core/test_agent_spec.pytests/core/test_default_agent.pytests/core/test_implement_judge_chain.pytests/core/test_load_agent.pytests/test_judge_branding.pytests/tools/test_implement_judge_load.pytests/ui_and_conv/test_shell_welcome_info.pytests/ui_and_conv/test_visualize_running_prompt.pytests/utils/test_pyinstaller_utils.py
Address ImplementAndJudge review findings on PR #178: - Bound _parse_judge_verdict to the SUMMARY section so a stray PASS/NEEDS_WORK/BLOCKED token in a later section (e.g. EVIDENCE) can no longer be read as the verdict — fail closed to BLOCKED instead of leaking a false PASS on a quality gate. - Drop revision_index from the orchestration fingerprint so a NEEDS_WORK revision reuses the chain's single approval instead of re-prompting mid-chain after the implementer has already written, matching _run_agents_fingerprint. - Correct the max_revisions field docs: values above the cap are rejected at validation, not clamped. Adds regression tests for the later-section verdict leak and single-grant revision reuse.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pythinker_code/tools/agent/__init__.py (1)
1451-1454: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset final state when a child fails after an earlier revision.
On revision-path failures, stale state from the previous judge/implementer can leak into the final result: an implementer error after
NEEDS_WORKleaveslast_verdict/last_artifactfrom the prior attempt, and a judge error can leave a staleverdict_match. Fail closed toBLOCKEDand clear stale fields in both child-error branches.As per coding guidelines, observable output must distinguish failure from partial success, and C06 flags returns that blur failure states.
Proposed fix
last_implementer_output = self._child_result_output(impl_result) if impl_result.is_error: - last_implementer_error = impl_result.message + error_message = impl_result.message or "implementer subagent failed" + last_implementer_error = error_message + last_verdict = "BLOCKED" + last_verdict_raw = None + last_required_fixes = "judge not run because implementer subagent failed" + last_artifact = None break @@ if judge_result.is_error: # Treat a judge failure as BLOCKED for the current revision # and surface it — fail closed rather than silently pass. last_verdict = "BLOCKED" - last_required_fixes = f"judge subagent error: {judge_result.message}" + last_verdict_raw = None + error_message = judge_result.message or "judge subagent failed" + last_required_fixes = f"judge subagent error: {error_message}" breakAlso applies to: 1470-1475
🤖 Prompt for 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. In `@src/pythinker_code/tools/agent/__init__.py` around lines 1451 - 1454, When an implementer error occurs in the section around line 1451-1454 (in the if impl_result.is_error block following the last_implementer_output assignment), clear the stale state variables last_verdict and last_artifact to prevent them from leaking into the final result. Similarly, in the judge error handling section around lines 1470-1475, clear the stale verdict_match variable. In both error branches, ensure the final state is set to BLOCKED to properly distinguish failure from partial success, rather than allowing stale values from previous revision attempts to persist in the final result.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/pythinker_code/tools/agent/__init__.py`:
- Around line 1451-1454: When an implementer error occurs in the section around
line 1451-1454 (in the if impl_result.is_error block following the
last_implementer_output assignment), clear the stale state variables
last_verdict and last_artifact to prevent them from leaking into the final
result. Similarly, in the judge error handling section around lines 1470-1475,
clear the stale verdict_match variable. In both error branches, ensure the final
state is set to BLOCKED to properly distinguish failure from partial success,
rather than allowing stale values from previous revision attempts to persist in
the final result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0eb16344-8e26-4a3e-89c0-2e101dbbd107
📒 Files selected for processing (2)
src/pythinker_code/tools/agent/__init__.pytests/core/test_implement_judge_chain.py
…napshot - The wire handshake snapshot (tests_e2e/test_wire_protocol.py) was stale: this PR added the judge-minimum-diff and judge-overengineering-review bundled skills but never regenerated the handshake skills list, leaving CI red on test_initialize_handshake / test_initialize_external_tool_conflict. Regenerated to include both new skills. - ImplementAndJudge: an implementer error on the revision now fails closed to BLOCKED, clearing the prior revision's stale NEEDS_WORK verdict and artifact so they can't leak into the final result (mirrors the judge-error branch). Adds a regression test.
test_pending_activation_returns_full asserted the plan-absent (full reminder) branch but pointed plan_path at a hardcoded /tmp/plan.md. The provider checks plan_path.exists(), so on any machine where that file happens to exist the test hit the reentry branch and failed (green only on a clean /tmp, e.g. CI). Use a tmp_path file that is never created, mirroring the sibling reentry test.
This was referenced Jul 18, 2026
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.
Summary
Adds a one-shot
implementer → judgechain tool, surfaces the failing tool's reason inInvalidToolError, and adopts the minimum-diff rubric as a uniform judge dimension.ImplementAndJudgechain tool (pythinker_code.tools.agent)AgentTooltwice (implementer first, then judge with the implementer's output baked into the packet).NEEDS_WORKandmax_revisions >= 1, re-invokes the implementer once with the judge'sREQUIRED FIXESsection isolated under a## Revision briefheading, then re-judges.RunAgents.SUMMARYheading so preamble tokens ("PASS for the brief but…", "BLOCKED would be overkill…") cannot outrank the real verdict. Missing / SUMMARY-without-token fails closed toBLOCKED.<coding_artifact>block is extracted as data (never as instructions) and passed to the judge. Required fixes are framed as untrusted feedback in the revision prompt.InvalidToolErrorper-tool aggregation (soul/toolset.py)Bad tool paths in
agent.yamlused to surface asInvalid tools: [...]with the actual reason buried in the log file. The aggregated error now lists each failing tool with its per-tool reason and a "Did you mean ?" hint. A constructor exception on one tool is caught per-tool so the user gets one clear error instead of a bare traceback. Whole load still aborts on any failure.Judge minimum-diff rubric dimension (
agents/default/judge.yaml)Every non-trivial diff the judge reviews is now checked against the reduction ladder (skip-need → reuse-stdlib → use-native → use-installed-dep → one-line → minimum). The ladder applies uniformly across review modes — there is no mode switch on the judge.
Bundled skills (
src/pythinker_code/skills/judge-{minimum-diff,overengineering-review})Two static default skills replace ad-hoc prose in the system prompt with explicit, versionable content, registered in the PyInstaller
datasmanifest so the one-file build picks them up.Commits
feat(tools): add ImplementAndJudge chain toolfeat(toolset): surface per-tool reason in aggregated InvalidToolErrorfeat(agents): register ImplementAndJudge, adopt minimum-diff judge rubricfeat(skills): bundle judge-minimum-diff and judge-overengineering-reviewtest: cover ImplementAndJudge chain, judge branding, and toolset error reportingchore(release): add Unreleased changelog entriesThe branch also carries
fd6070d feat: editor bug fixes, Draft and auto save fixeswhich was already committed locally onmainahead oforigin/mainwhen this branch was cut. That commit is unrelated to the chain-tool scope but is included so the branch contains every local commit the author asked to publish.Test plan
make check-pythinker-code— ruff format/check + pyright + tymake test-pythinker-code— unit + e2euv run pytest tests/core/test_implement_judge_chain.py -v— chain primitives (verdict parsing, artifact extraction, fingerprint, REQUIRED FIXES isolation, param validation)uv run pytest tests/tools/test_implement_judge_load.py -v— loader regressionuv run pytest tests/core/test_load_agent.py::test_load_tools_invalid tests/core/test_load_agent.py::test_load_tools_aggregates_constructor_errors -v— new aggregated-error testsuv run pytest tests/test_judge_branding.py -v— minimum-diff rubric drift guarduv run pytest tests/utils/test_pyinstaller_utils.py::test_pyinstaller_datas -v— datas manifestRisk
ImplementAndJudgerather thanAgentwithsubagent_type=implementer); default-agent registration just makes the tool available.agent.yaml; the loader-regression testtest_implement_judge_loads_via_toolsetguards against the missing-class startup failure mode.InvalidToolErrorformat change: any caller that string-matches the oldInvalid tools: [...]shape will see the new multi-line message. Internal-only; no public API contract.Summary by CodeRabbit
Release Notes
New Features
ImplementAndJudgeauto-chaining for scoped, non-trivial code edits (single-tool workflow with revision feedback, capped to two runs).Bug Fixes
systemasdeveloper, including during model switches./update).Documentation