Skip to content

Commit 555107e

Browse files
committed
fix: address CodeRabbit review findings on goal continuation and todos
- Goal auto-continuation now requires the primary turn to end cleanly (no_tool_calls): a tool rejection or stuck primary turn no longer triggers continuations, matching the rule already applied between continuation turns. Ralph-loop runs never continue (own strategy). - Soften the SetTodoList single-in_progress invariant from a hard rejection to a corrective notice: pythinker's parallel-subagent fan-out legitimately tracks one in_progress sub-todo per running child (system.md orchestration rules), so rejecting such lists would break the documented workflow. The tool description, system.md status-discipline bullet, and changelog wording are reconciled to state the sequential rule and its fan-out exception. - Document compact_prompt as nullable in the config reference, fix a malformed report fence token in tasks/todo.md, and add boundary tests for goal.max_continuations (1-10). Declined (with rationale): mechanical enforcement of the blocked-audit three-strike gate inside UpdateGoal — Codex itself enforces it as a prompt contract, and 'same blocking condition' is semantic, so code enforcement would misfire on legitimate impasses. H1 headings for the prompt markdown assets — injected prompt files conventionally start with body text in this repo (init.md, compact.md) and there is no markdownlint gate.
1 parent 70a6ea0 commit 555107e

11 files changed

Lines changed: 84 additions & 27 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ GitHub Releases page; `0.8.0` is the new starting line.
1717

1818
- **New `/goal` command: goal-driven execution ported from Codex CLI.** `/goal <objective>` sets a persistent thread goal the agent pursues across turns until it is verifiably complete. The objective is stored in session state (survives restarts and context compaction), kicks off work immediately with a success-criteria derivation prompt, and is re-injected on later turns as a continuation reminder carrying Codex's fidelity rules (no scope-shrinking, no easier-to-test substitutes) and evidence-based completion audit — the agent may only claim completion after proving every requirement against current state, and the user confirms with `/goal clear`. Subcommands: `view`, `pause`, `resume`, `clear`. Objectives are injected as untrusted data (`<objective>` framing), never as higher-priority instructions.
1919
- **New `/best-practices` command (alias `/bp`).** Injects opt-in engineering best-practice guidance distilled from the Codex CLI system prompts — code-change discipline, dirty-worktree safety (never revert changes you didn't make), specific-to-broad testing strategy, todo hygiene, progress-update cadence, debugging methodology, and final-answer style — into the session context without consuming a turn. `/best-practices <section>` injects a single section.
20-
- **SetTodoList enforces the single-`in_progress` invariant.** Todo lists with more than one `in_progress` item are now rejected with a corrective error (ported from Codex's plan-tool contract), and the system prompt gains matching status-discipline guidance: no single-step lists, no `pending``done` jumps, no batch-completing after the fact.
20+
- **SetTodoList nudges the single-`in_progress` discipline.** Todo lists with more than one `in_progress` item now get a corrective notice (ported from Codex's plan-tool contract, softened because parallel-subagent fan-out legitimately tracks one `in_progress` sub-todo per running child), and the system prompt gains matching status-discipline guidance: no single-step lists, no `pending``done` jumps, no batch-completing after the fact.
2121
- **`UpdateGoal` tool + opt-in goal auto-continuation: the full "loop until verified".** The agent can now mark the active `/goal` `complete` (only after the evidence-based completion audit) or `blocked` (only after Codex's strict three-strike blocked audit) via the new root-only `UpdateGoal` tool, which stops goal reminders and continuations; `/goal resume` reactivates either state. With `goal.auto_continue = true` (new config table, default off, `max_continuations` 1–10 capped at 3 by default), each user message is followed by automatic continuation turns toward the active goal — carrying the Codex continuation prompt — until the goal is marked, a tool call is rejected, or the cap is reached, with a budget-style wrap-up instruction on the final continuation.
2222
- **Approval-mode-aware validation guidance.** Auto/yolo-mode injections now tell the agent to proactively run tests and lint before finishing (no user present to confirm), while the back-to-interactive reminder defers slow test/lint commands to user confirmation except for test-related tasks — ported from the Codex CLI validation philosophy.
2323
- **`compact_prompt` config override.** A new optional top-level config key replaces the built-in compaction summarization prompt for both manual and automatic compaction; a `/compact` focus argument is still appended on top, and leaving it unset preserves current behavior.

docs/en/configuration/config-files.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ The configuration file contains the following top-level configuration items:
3838
| `models` | `table` | Model configuration |
3939
| `loop_control` | `table` | Agent loop control parameters |
4040
| `goal` | `table` | Thread-goal (`/goal`) behavior, including auto-continuation |
41-
| `compact_prompt` | `string` | Override the built-in compaction summarization prompt; unset keeps the default handoff-structured prompt (a `/compact` focus argument is still appended on top) |
41+
| `compact_prompt` | `string \| null` | Override the built-in compaction summarization prompt; `null`/unset keeps the default handoff-structured prompt (a `/compact` focus argument is still appended on top) |
4242
| `background` | `table` | Background task runtime parameters |
4343
| `services` | `table` | External service configuration (search, fetch) |
4444
| `mcp` | `table` | MCP client configuration |

src/pythinker_code/agents/default/system.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ For any non-trivial request, decompose before acting:
168168
- Preview the terrain first: scan the directory structure, file headers, and relevant module boundaries before choosing an implementation path.
169169
- **`SetTodoList` marks the start of execution, not planning.** Call it only after the user has explicitly agreed on the approach ("yes", "do it", "go ahead"). Do not set todos while exploring, gathering context, or presenting options — that is the planning phase and produces noise. Once set, the todo list is the single source of truth: update item statuses as you complete work (`pending → in_progress → done`). Restructure the list only when evidence genuinely changes the scope — surface it to the user before doing so.
170170
- **Granular todos, not umbrella todos.** Each todo must name a single concrete deliverable a human can recognize as "this part is done." Avoid umbrella titles like "Determine X" or "Investigate Y" that cover hours of parallel work — they freeze the progress UI while real work happens underneath. If a single todo would stay `in_progress` for more than ~3 minutes, it is too coarse: split it before launching work.
171-
- **Status discipline.** Do not make single-step todo lists or pad simple work with filler steps. Never jump an item from `pending` to `done` — set it `in_progress` first, keeping at most one item `in_progress` at a time — and never batch-complete multiple items after the fact. End the turn with every item `done` or explicitly `cancelled`.
171+
- **Status discipline.** Do not make single-step todo lists or pad simple work with filler steps. Never jump an item from `pending` to `done` — set it `in_progress` first, keeping at most one item `in_progress` at a time for your own sequential work (parallel-subagent fan-out is the exception: one `in_progress` sub-todo per running child, per the rule below) — and never batch-complete multiple items after the fact. End the turn with every item `done` or explicitly `cancelled`.
172172
- **Progress cadence.** Post a short Progress note (1-2 sentences) when you uncover a meaningful insight or change direction — notes replace, not duplicate, narration in your final text. Before the first tool call of substantial work, state the goal, constraints, and next steps. Announce longer heads-down stretches and summarize what you learned when you resume; call out plan changes explicitly in the next update.
173173
- **One todo per dispatched child.** When you launch `RunAgents` with N children, the visible todo list MUST contain one in_progress sub-todo per child (or per independent objective the batch covers) **before** the batch starts. Update each sub-todo to `done` as that child returns — do not wait for the whole batch to finish to flip a single umbrella todo. Same rule applies to multiple parallel `Agent` calls in the same turn.
174174
- Split broad work into independent chunks; use parallel tool calls or focused subagents for chunks that do not depend on each other. Scale the number of agents to the task's independent subparts — a single lookup needs none, a small comparison 2-4 — and prefer the fewest that cover the work; over-provisioning burns the multi-agent token premium.

src/pythinker_code/soul/pythinkersoul.py

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1002,6 +1002,7 @@ async def run(
10021002
user_message = Message(role="user", content=user_input)
10031003
text_input = user_message.extract_text(" ").strip()
10041004

1005+
primary_outcome: TurnOutcome | None = None
10051006
if command_call := parse_slash_command_call(text_input):
10061007
command = self._find_slash_command(command_call.name)
10071008
if command is None:
@@ -1018,7 +1019,7 @@ async def run(
10181019
)
10191020
await runner.run(self, "")
10201021
else:
1021-
await self._turn(user_message)
1022+
primary_outcome = await self._turn(user_message)
10221023

10231024
# --- Stop hook (max 1 re-trigger to prevent infinite loop) ---
10241025
if not self._stop_hook_active:
@@ -1039,8 +1040,8 @@ async def run(
10391040
self._stop_hook_active = False
10401041
break
10411042

1042-
if command_call is None:
1043-
await self._run_goal_continuations()
1043+
if primary_outcome is not None:
1044+
await self._run_goal_continuations(primary_outcome)
10441045

10451046
wire_send(TurnEnd())
10461047
turn_finished = True
@@ -1100,15 +1101,17 @@ async def run(
11001101
reset_current_approval_source(approval_source_token)
11011102
self._prompt_queue_lock.release()
11021103

1103-
async def _run_goal_continuations(self) -> None:
1104+
async def _run_goal_continuations(self, primary_outcome: TurnOutcome) -> None:
11041105
"""Auto-continue toward the active /goal after the primary turn.
11051106
11061107
Ported from Codex CLI's automatic goal continuations, bounded per user
11071108
submission by ``goal.max_continuations``. Hard stops (cancellation,
11081109
MaxStepsReached, provider errors) propagate out of ``_turn`` and end
1109-
the loop together with the run; a rejected tool call or a goal marked
1110-
complete/blocked (via UpdateGoal) ends it gracefully.
1110+
the loop together with the run; a rejected tool call, a stuck turn, or
1111+
a goal marked complete/blocked (via UpdateGoal) ends it gracefully.
11111112
"""
1113+
if primary_outcome.stop_reason != "no_tool_calls":
1114+
return
11121115
goal_config = self._runtime.config.goal
11131116
if not goal_config.auto_continue or self.is_subagent or self.plan_mode:
11141117
return

src/pythinker_code/tools/todo/__init__.py

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -67,18 +67,21 @@ def __init__(self, runtime: Runtime) -> None:
6767
async def __call__(self, params: Params) -> ToolReturnValue:
6868
if params.todos is None:
6969
return self._read_todos()
70+
result = self._write_todos(params.todos)
7071
in_progress = sum(1 for todo in params.todos if todo.status == "in_progress")
7172
if in_progress > 1:
72-
return ToolReturnValue(
73-
is_error=True,
74-
output=(
75-
"Invalid todo list: at most one item can be in_progress at a time. "
76-
"Resubmit with exactly one in_progress item."
77-
),
78-
message="Invalid todo list",
79-
display=[],
73+
# Codex plan-tool contract, softened: parallel-subagent fan-out
74+
# legitimately tracks one in_progress sub-todo per running child.
75+
base_output = result.output if isinstance(result.output, str) else ""
76+
result = ToolReturnValue(
77+
is_error=False,
78+
output=base_output
79+
+ "\nNote: keep at most one item in_progress at a time for your own "
80+
"sequential work; multiple in_progress items are expected only while "
81+
"tracking parallel subagents (one sub-todo per running child).",
82+
message=result.message,
83+
display=result.display,
8084
)
81-
result = self._write_todos(params.todos)
8285
if self._runtime.role == "root" and len(params.todos) >= 3:
8386
await self._journal_todo_update(params.todos)
8487
return result

src/pythinker_code/tools/todo/set_todo_list.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ Once the todo list is set, it is the single source of truth for in-progress work
1313

1414
Once you finish a subtask/milestone, update its status before moving to the next item.
1515

16-
At most one item can be in_progress at a time — lists with more than one are rejected. Do not jump an item from `pending` to `done`: set it `in_progress` first, and do not batch-complete multiple items after the fact.
16+
Keep at most one item in_progress at a time for your own sequential work — the only exception is parallel-subagent fan-out, where one in_progress sub-todo per running child is expected. Do not jump an item from `pending` to `done`: set it `in_progress` first, and do not batch-complete multiple items after the fact.
1717

1818
**Do NOT use this tool:**
1919

tasks/todo.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ Scouted via 6-explorer workflow + synthesis; primary sources re-read before port
6868

6969
## Out of scope (observed, logged)
7070

71-
- Codex P0-P3 JSON review schema (pythinker has its own ```report contract).
71+
- Codex P0-P3 JSON review schema (pythinker has its own `report` fenced-block contract).
7272
- Per-goal token budgets ({{ token_budget }} vars) — no per-goal usage meter yet.
7373
- User prompt-template shadowing (a user `goal.md` template vs builtin) — builtin
7474
soul commands and prompt templates share the slash namespace; collision behavior

tests/core/test_config.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -645,3 +645,19 @@ def test_load_config_no_args_uses_scope_resolution(tmp_path, monkeypatch):
645645
config = load_config()
646646
assert config.theme == "light"
647647
assert "user" in config.source_scopes
648+
649+
650+
def test_goal_config_bounds():
651+
"""goal.max_continuations is clamped to 1-10 by validation."""
652+
import pytest
653+
from pydantic import ValidationError
654+
655+
from pythinker_code.config import GoalConfig
656+
657+
assert GoalConfig().max_continuations == 3
658+
assert GoalConfig(max_continuations=1).max_continuations == 1
659+
assert GoalConfig(max_continuations=10).max_continuations == 10
660+
with pytest.raises(ValidationError):
661+
GoalConfig(max_continuations=0)
662+
with pytest.raises(ValidationError):
663+
GoalConfig(max_continuations=11)

tests/core/test_goal_auto_continuation.py

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,3 +157,36 @@ async def test_no_continuation_in_plan_mode(self, runtime: Runtime, tmp_path: Pa
157157
turn_mock = soul._turn
158158
assert isinstance(turn_mock, AsyncMock)
159159
assert turn_mock.await_count == 1
160+
161+
async def test_no_continuation_when_primary_turn_rejected(
162+
self, runtime: Runtime, tmp_path: Path
163+
) -> None:
164+
"""A tool rejection in the primary turn must not trigger continuations."""
165+
runtime.config.goal.auto_continue = True
166+
runtime.session.state.goal = GoalState(objective="ship it", status="active")
167+
soul = _make_soul(runtime, tmp_path)
168+
169+
turn_mock = soul._turn
170+
assert isinstance(turn_mock, AsyncMock)
171+
turn_mock.return_value = TurnOutcome(
172+
stop_reason="tool_rejected", final_message=None, step_count=1
173+
)
174+
175+
await soul.run("do the thing")
176+
177+
assert turn_mock.await_count == 1
178+
179+
async def test_no_continuation_when_primary_turn_stuck(
180+
self, runtime: Runtime, tmp_path: Path
181+
) -> None:
182+
runtime.config.goal.auto_continue = True
183+
runtime.session.state.goal = GoalState(objective="ship it", status="active")
184+
soul = _make_soul(runtime, tmp_path)
185+
186+
turn_mock = soul._turn
187+
assert isinstance(turn_mock, AsyncMock)
188+
turn_mock.return_value = TurnOutcome(stop_reason="stuck", final_message=None, step_count=1)
189+
190+
await soul.run("do the thing")
191+
192+
assert turn_mock.await_count == 1

tests/tools/test_todo.py

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -344,10 +344,12 @@ async def test_subagent_malformed_individual_item(self, runtime: Runtime):
344344

345345

346346
class TestSingleInProgressInvariant:
347-
"""Ported from Codex CLI's plan tool contract (plan_spec.rs):
348-
at most one step can be in_progress at a time."""
347+
"""Ported from Codex CLI's plan tool contract (plan_spec.rs): at most one
348+
step in_progress at a time — softened to a notice because pythinker's
349+
parallel-subagent fan-out legitimately tracks one in_progress sub-todo
350+
per running child (system.md orchestration rules)."""
349351

350-
async def test_two_in_progress_items_rejected(
352+
async def test_multiple_in_progress_accepted_with_notice(
351353
self, set_todo_list_tool: SetTodoList, runtime: Runtime
352354
):
353355
result = await set_todo_list_tool(
@@ -358,10 +360,10 @@ async def test_two_in_progress_items_rejected(
358360
]
359361
)
360362
)
361-
assert result.is_error
363+
assert not result.is_error
362364
assert "at most one" in result.output
363-
# The invalid list must not be persisted.
364-
assert runtime.session.state.todos == []
365+
# The list is persisted despite the notice (parallel fan-out is legal).
366+
assert len(runtime.session.state.todos) == 2
365367

366368
async def test_exactly_one_in_progress_accepted(self, set_todo_list_tool: SetTodoList):
367369
result = await set_todo_list_tool(

0 commit comments

Comments
 (0)