Skip to content

Commit 44a9c88

Browse files
committed
fix: apply external review findings across recent checkpoints
- CRITICAL: GIT_CONTEXT_AGENT_TYPES used underscored reviewer names while registered type names are dashed (code-reviewer/security-reviewer), so reviewer agents silently missed the git-context injection; names fixed and a pin added asserting every gate name is a real profile key. - Foreground isolation requests now fail fast on Agent AND RunAgents instead of warning-and-proceeding unisolated (degraded behavior was presented as authoritative); warning pin updated to the new contract. - Unknown-config-key diagnostics now also run for explicit loads (--config-file / --config text) via single-source provenance. - Failure/timeout/cancel paths name the retained isolation worktree in the task output (retention is deliberate for resume, never silent). - Best-effort prune in overflow recovery logs its failure instead of contextlib.suppress. - supports_parallel flags annotated (: bool); test helpers cleaned (fail-fast _git asserts, unused _ListingClient params dropped).
1 parent a9c9059 commit 44a9c88

18 files changed

Lines changed: 99 additions & 40 deletions

File tree

src/pythinker_code/background/agent_runner.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,11 +137,13 @@ async def run(self) -> None:
137137
output.error(
138138
_timeout_recovery_message(timeout_s=self._timeout_s, agent_id=self._agent_id)
139139
)
140+
self._note_retained_worktree(output)
140141
else:
141142
# Internal timeout (e.g. aiohttp request) — treat as generic failure
142143
logger.exception("Background agent runner failed")
143144
self._finalize_safely(outcome="failed", reason=str(exc))
144145
output.error(_failure_recovery_message(reason=str(exc), agent_id=self._agent_id))
146+
self._note_retained_worktree(output)
145147
except asyncio.CancelledError:
146148
self._finalize_safely(outcome="killed", reason="Stopped by TaskStop")
147149
output.stage("cancelled")
@@ -156,6 +158,7 @@ async def run(self) -> None:
156158
logger.exception("Background agent runner failed")
157159
self._finalize_safely(outcome="failed", reason=str(exc))
158160
output.error(_failure_recovery_message(reason=str(exc), agent_id=self._agent_id))
161+
self._note_retained_worktree(output)
159162
finally:
160163
# Whatever happens in approval cleanup below, the dict pop must
161164
# run — it is the *only* place that removes this task from
@@ -272,6 +275,19 @@ async def _prepare_isolation_worktree(self, output: SubagentOutputWriter) -> Hos
272275
output.stage(f"worktree_created: {worktree}")
273276
return HostPath.unsafe_from_local_path(worktree)
274277

278+
def _note_retained_worktree(self, output: SubagentOutputWriter) -> None:
279+
"""Name the retained worktree on failure/timeout paths.
280+
281+
Retention on failure is deliberate — resume reuses the worktree and a
282+
post-mortem may need its state — but it must never be silent.
283+
"""
284+
if self._worktree_path is None:
285+
return
286+
output.stage(
287+
f"worktree_retained: {self._worktree_path} (resume reuses it; remove with "
288+
f"`git worktree remove {self._worktree_path}`)"
289+
)
290+
275291
async def _append_worktree_report(self, final_response: str) -> str:
276292
"""Tell the orchestrator where the isolated changes live.
277293

src/pythinker_code/config.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,21 @@ def _sole_non_none_type(annotation: Any) -> Any:
185185
return annotation
186186

187187

188+
def _report_unknown_keys_single_source(data: Any, source: str) -> None:
189+
"""Unknown-key diagnostics for single-source loads (--config/--config-file).
190+
191+
The scoped pipeline reports through its merge provenance; explicit loads
192+
bypass the merge, so build a one-source provenance here. Non-dict payloads
193+
are left for Config.model_validate to reject with its own error.
194+
"""
195+
if not isinstance(data, dict):
196+
return
197+
plain = {str(key): value for key, value in cast(dict[Any, Any], data).items()}
198+
provenance: dict[str, Any] = {}
199+
merged = _type_based_merge({}, plain, provenance, source)
200+
_report_unknown_config_keys(merged, provenance)
201+
202+
188203
def _report_unknown_config_keys(merged: dict[str, Any], provenance: dict[str, Any]) -> None:
189204
"""Warn (or raise under PYTHINKER_STRICT_CONFIG) for unconsumed keys."""
190205
unknown_paths = unknown_config_key_paths(Config, merged)
@@ -1217,6 +1232,7 @@ def load_config(config_file: Path | None = None) -> Config:
12171232
data = json.loads(config_text)
12181233
else:
12191234
data = tomlkit.loads(config_text)
1235+
_report_unknown_keys_single_source(data, str(config_file))
12201236
config = Config.model_validate(data)
12211237
except json.JSONDecodeError as e:
12221238
raise ConfigError(f"Invalid JSON in configuration file {config_file}: {e}") from e
@@ -1260,6 +1276,7 @@ def load_config_from_string(config_string: str) -> Config:
12601276
f"Invalid configuration text: {json_error}; {toml_error}"
12611277
) from toml_error
12621278

1279+
_report_unknown_keys_single_source(data, "--config text")
12631280
try:
12641281
config = Config.model_validate(data)
12651282
except ValidationError as e:

src/pythinker_code/soul/pythinkersoul.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1979,8 +1979,13 @@ async def _recover_from_context_overflow(self, step_no: int) -> bool:
19791979
step_no=step_no,
19801980
)
19811981
try:
1982-
with contextlib.suppress(Exception):
1982+
try:
19831983
await self.prune_context()
1984+
except Exception as prune_err:
1985+
logger.debug(
1986+
"Best-effort prune during overflow recovery failed: {error}",
1987+
error=prune_err,
1988+
)
19841989
await self.compact_context()
19851990
except Exception as compact_err:
19861991
from pythinker_code.telemetry.errors import report_handled_error

src/pythinker_code/subagents/core.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@
2525
from pythinker_code.subagents.store import SubagentStore
2626
from pythinker_code.wire.types import TextPart, ThinkPart
2727

28-
GIT_CONTEXT_AGENT_TYPES = frozenset({"explore", "review", "code_reviewer", "security_reviewer"})
28+
# NOTE: these must match the registered type names in agents/default/agent.yaml
29+
# (dashed), which _SUBAGENT_PROFILES also keys on — not the yaml file stems.
30+
GIT_CONTEXT_AGENT_TYPES = frozenset({"explore", "review", "code-reviewer", "security-reviewer"})
2931
"""Read-oriented agent types whose first prompt gets a git-context prefix.
3032
3133
Exploration and review both orient on repo state (branch, dirty files,

src/pythinker_code/tools/agent/__init__.py

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -297,14 +297,18 @@ async def __call__(self, params: Params) -> ToolReturnValue:
297297
),
298298
brief="Invalid fork_context",
299299
)
300+
if not params.run_in_background and params.isolation != "none":
301+
# Proceeding unisolated after an isolation request would present
302+
# degraded behavior as authoritative; fail fast instead.
303+
return ToolError(
304+
message=(
305+
"isolation='worktree' is only supported for background agents; "
306+
"set run_in_background=true or drop isolation."
307+
),
308+
brief="Invalid isolation",
309+
)
300310
if params.run_in_background:
301311
return await self._run_in_background(params)
302-
if params.isolation != "none":
303-
logger.warning(
304-
"isolation={isolation!r} has no effect on foreground agents; "
305-
"use run_in_background=True to enable isolation.",
306-
isolation=params.isolation,
307-
)
308312
await self._journal_foreground_agent_start(params, requested_type)
309313
timeout = params.effective_timeout
310314
try:
@@ -667,6 +671,16 @@ async def __call__(self, params: RunAgentsParams) -> ToolReturnValue:
667671
message="Subagents cannot launch other subagents.",
668672
brief="RunAgents unavailable",
669673
)
674+
if not params.run_in_background and params.isolation != "none":
675+
# Foreground children would share one tree despite the isolation
676+
# request; fail fast rather than proceed unisolated.
677+
return ToolError(
678+
message=(
679+
"isolation='worktree' is only supported for background child "
680+
"agents; set run_in_background=true or drop isolation."
681+
),
682+
brief="Invalid isolation",
683+
)
670684
if params.model is not None and params.model not in self._runtime.config.models:
671685
return ToolError(
672686
message=f"Unknown model alias: {params.model}",

src/pythinker_code/tools/file/glob.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ class Params(BaseModel):
3131

3232
class Glob(CallableTool2[Params]):
3333
name: str = "Glob"
34-
supports_parallel = True
34+
supports_parallel: bool = True
3535
description: str = load_desc(
3636
Path(__file__).parent / "glob.md",
3737
{

src/pythinker_code/tools/file/grep_local.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -719,7 +719,7 @@ def _smart_search_patterns(query: str) -> list[tuple[str, str]]:
719719

720720
class SmartSearch(CallableTool2[SmartSearchParams]):
721721
name: str = "SmartSearch"
722-
supports_parallel = True
722+
supports_parallel: bool = True
723723
description: str = (
724724
"Plan and run a small set of bounded local grep passes for a symbol or concept. "
725725
"Returns cited file/line spans and truncation guidance; use for exploration before "
@@ -789,7 +789,7 @@ async def __call__(self, params: SmartSearchParams) -> ToolReturnValue:
789789

790790
class Grep(CallableTool2[Params]):
791791
name: str = "Grep"
792-
supports_parallel = True
792+
supports_parallel: bool = True
793793
description: str = load_desc(Path(__file__).parent / "grep.md")
794794
params: type[Params] = Params
795795

src/pythinker_code/tools/file/read.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ def _validate_line_offset(self) -> "Params":
6363

6464
class ReadFile(CallableTool2[Params]):
6565
name: str = "ReadFile"
66-
supports_parallel = True
66+
supports_parallel: bool = True
6767
params: type[Params] = Params
6868

6969
def __init__(self, runtime: Runtime) -> None:

src/pythinker_code/tools/file/read_media.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ class Params(BaseModel):
4949

5050
class ReadMediaFile(CallableTool2[Params]):
5151
name: str = "ReadMediaFile"
52-
supports_parallel = True
52+
supports_parallel: bool = True
5353
params: type[Params] = Params
5454

5555
def __init__(self, runtime: Runtime) -> None:

src/pythinker_code/tools/mcp_resource/__init__.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ class ListParams(BaseModel):
2626

2727
class ListMcpResources(CallableTool2[ListParams]):
2828
name: str = "ListMcpResources"
29-
supports_parallel = True
29+
supports_parallel: bool = True
3030
params: type[ListParams] = ListParams
3131

3232
def __init__(self, toolset: PythinkerToolset) -> None:
@@ -75,7 +75,7 @@ class ReadParams(BaseModel):
7575

7676
class ReadMcpResource(CallableTool2[ReadParams]):
7777
name: str = "ReadMcpResource"
78-
supports_parallel = True
78+
supports_parallel: bool = True
7979
params: type[ReadParams] = ReadParams
8080

8181
def __init__(self, toolset: PythinkerToolset) -> None:

0 commit comments

Comments
 (0)