Commit af9388f
authored
feat: ImplementAndJudge chain, per-tool error reporting, and minimum-diff judge rubric (#178)
* feat: editor bug fixes, Draft and auto save fixes.
* feat(tools): add ImplementAndJudge chain tool
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.
* feat(toolset): surface per-tool reason in aggregated InvalidToolError
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.
* feat(agents): register ImplementAndJudge, adopt minimum-diff judge rubric
- 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.
* feat(skills): bundle judge-minimum-diff and judge-overengineering-review
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.
* test: cover ImplementAndJudge chain, judge branding, and toolset error 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.
* chore(release): add Unreleased changelog entries for chain tool, toolset 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.
* fix(pr): address spell-check and brand-guard findings, drop tracked plan 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.
* fix(agent): bound judge verdict to SUMMARY and reuse chain approval
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.
* fix(agent): fail closed on revision implementer error; refresh wire snapshot
- 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: make plan-mode pending-activation test hermetic
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.1 parent bf2f703 commit af9388f
23 files changed
Lines changed: 1770 additions & 27 deletions
File tree
- packages/pythinker-core
- src/pythinker_core/contrib/chat_provider
- tests/api_snapshot_tests
- src/pythinker_code
- agents/default
- skills
- judge-minimum-diff
- judge-overengineering-review
- soul
- tools/agent
- ui/shell
- tests_e2e
- tests
- core
- tools
- ui_and_conv
- utils
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
18 | 76 | | |
19 | 77 | | |
20 | 78 | | |
| |||
Lines changed: 8 additions & 7 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
159 | 159 | | |
160 | 160 | | |
161 | 161 | | |
162 | | - | |
163 | | - | |
164 | | - | |
165 | | - | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
166 | 167 | | |
167 | 168 | | |
168 | 169 | | |
| |||
235 | 236 | | |
236 | 237 | | |
237 | 238 | | |
238 | | - | |
239 | | - | |
| 239 | + | |
| 240 | + | |
240 | 241 | | |
241 | 242 | | |
242 | 243 | | |
243 | 244 | | |
244 | | - | |
| 245 | + | |
245 | 246 | | |
246 | 247 | | |
247 | 248 | | |
| |||
Lines changed: 60 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
| 6 | + | |
6 | 7 | | |
7 | 8 | | |
8 | 9 | | |
| |||
531 | 532 | | |
532 | 533 | | |
533 | 534 | | |
| 535 | + | |
| 536 | + | |
| 537 | + | |
| 538 | + | |
| 539 | + | |
| 540 | + | |
| 541 | + | |
| 542 | + | |
| 543 | + | |
| 544 | + | |
| 545 | + | |
| 546 | + | |
| 547 | + | |
| 548 | + | |
| 549 | + | |
| 550 | + | |
| 551 | + | |
| 552 | + | |
| 553 | + | |
| 554 | + | |
| 555 | + | |
| 556 | + | |
| 557 | + | |
| 558 | + | |
| 559 | + | |
| 560 | + | |
| 561 | + | |
| 562 | + | |
| 563 | + | |
| 564 | + | |
| 565 | + | |
| 566 | + | |
| 567 | + | |
| 568 | + | |
| 569 | + | |
| 570 | + | |
| 571 | + | |
| 572 | + | |
| 573 | + | |
| 574 | + | |
| 575 | + | |
| 576 | + | |
| 577 | + | |
| 578 | + | |
| 579 | + | |
| 580 | + | |
| 581 | + | |
| 582 | + | |
| 583 | + | |
| 584 | + | |
| 585 | + | |
| 586 | + | |
| 587 | + | |
| 588 | + | |
| 589 | + | |
| 590 | + | |
| 591 | + | |
| 592 | + | |
| 593 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
| 10 | + | |
10 | 11 | | |
11 | 12 | | |
12 | 13 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
18 | 18 | | |
19 | 19 | | |
20 | 20 | | |
| 21 | + | |
21 | 22 | | |
22 | 23 | | |
23 | 24 | | |
| |||
39 | 40 | | |
40 | 41 | | |
41 | 42 | | |
| 43 | + | |
42 | 44 | | |
43 | 45 | | |
44 | 46 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
97 | 97 | | |
98 | 98 | | |
99 | 99 | | |
| 100 | + | |
| 101 | + | |
100 | 102 | | |
101 | 103 | | |
102 | 104 | | |
| |||
121 | 123 | | |
122 | 124 | | |
123 | 125 | | |
| 126 | + | |
| 127 | + | |
124 | 128 | | |
125 | 129 | | |
126 | 130 | | |
| |||
0 commit comments