Skip to content

Commit 4bfa6ce

Browse files
committed
fix: address CodeRabbit review findings on PR #188
- Add openai_codex to the max_output_tokens kwarg-override map: it builds the same OpenAIResponses provider as openai_responses, so the compaction cap was silently no-op'ing (falling back to max_tokens) for ChatGPT/Codex-backed sessions. - Add direct capped_chat_provider coverage for all provider-type -> kwarg mappings, including the openai_codex case above. - Extract the duplicated Runtime(...) rebuild in test_pythinkersoul_stuck_loop.py into a shared helper.
1 parent cc53fbd commit 4bfa6ce

3 files changed

Lines changed: 48 additions & 23 deletions

File tree

src/pythinker_code/llm.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ def model_name(self) -> str:
6666
# providers, plus the first-party `pythinker` provider, all use `max_tokens`).
6767
_MAX_OUTPUT_TOKENS_KWARG_OVERRIDES: dict[str, str] = {
6868
"openai_responses": "max_output_tokens",
69+
"openai_codex": "max_output_tokens",
6970
"google_genai": "max_output_tokens",
7071
"gemini": "max_output_tokens",
7172
"vertexai": "max_output_tokens",

tests/core/test_compaction_overflow.py

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
from pythinker_core.chat_provider import APIStatusError
1919
from pythinker_core.message import Message
2020

21-
from pythinker_code.llm import LLM
21+
from pythinker_code.llm import LLM, capped_chat_provider
2222
from pythinker_code.soul.compaction import SimpleCompaction
2323
from pythinker_code.wire.types import TextPart
2424

@@ -46,6 +46,42 @@ def _fake_llm() -> LLM:
4646
return cast(LLM, SimpleNamespace(chat_provider=_FakeChatProvider(), provider_config=None))
4747

4848

49+
def _fake_llm_with_provider_type(provider_type: str) -> LLM:
50+
return cast(
51+
LLM,
52+
SimpleNamespace(
53+
chat_provider=_FakeChatProvider(),
54+
provider_config=SimpleNamespace(type=provider_type),
55+
),
56+
)
57+
58+
59+
@pytest.mark.parametrize(
60+
("provider_type", "expected_kwarg"),
61+
[
62+
("openai_legacy", "max_tokens"),
63+
("anthropic", "max_tokens"),
64+
("pythinker", "max_tokens"),
65+
("openai_responses", "max_output_tokens"),
66+
# ChatGPT/Codex sessions build the same OpenAIResponses provider as
67+
# "openai_responses" (see create_llm's "openai_codex" case), so they
68+
# take the same max_output_tokens kwarg, not the max_tokens default.
69+
("openai_codex", "max_output_tokens"),
70+
("google_genai", "max_output_tokens"),
71+
("gemini", "max_output_tokens"),
72+
("vertexai", "max_output_tokens"),
73+
],
74+
)
75+
def test_capped_chat_provider_picks_kwarg_by_provider_type(
76+
provider_type: str, expected_kwarg: str
77+
) -> None:
78+
llm = _fake_llm_with_provider_type(provider_type)
79+
80+
capped_chat_provider(llm, 4000)
81+
82+
assert cast(_FakeChatProvider, llm.chat_provider).generation_kwargs == {expected_kwarg: 4000}
83+
84+
4985
def _overflow_error() -> APIStatusError:
5086
return APIStatusError(400, "This model's maximum context length is exceeded")
5187

tests/core/test_pythinkersoul_stuck_loop.py

Lines changed: 10 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -128,11 +128,8 @@ async def __call__(self, params: _NoParams) -> ToolReturnValue:
128128
return ToolOk(output="ok", message="ok")
129129

130130

131-
def _make_soul(
132-
runtime: Runtime, provider: _ScriptedToolCallProvider, tmp_path: Path
133-
) -> tuple[Context, PythinkerSoul]:
134-
llm = LLM(chat_provider=provider, max_context_size=100_000, capabilities=set())
135-
runtime = Runtime(
131+
def _rebuild_runtime_with_llm(runtime: Runtime, llm: LLM) -> Runtime:
132+
return Runtime(
136133
config=runtime.config,
137134
llm=llm,
138135
session=runtime.session,
@@ -149,6 +146,13 @@ def _make_soul(
149146
skills_dirs=runtime.skills_dirs,
150147
role=runtime.role,
151148
)
149+
150+
151+
def _make_soul(
152+
runtime: Runtime, provider: _ScriptedToolCallProvider, tmp_path: Path
153+
) -> tuple[Context, PythinkerSoul]:
154+
llm = LLM(chat_provider=provider, max_context_size=100_000, capabilities=set())
155+
runtime = _rebuild_runtime_with_llm(runtime, llm)
152156
agent = Agent(
153157
name="Stuck Test Agent",
154158
system_prompt="Stuck test prompt.",
@@ -166,23 +170,7 @@ def _make_soul_with_pythinker_toolset(
166170
"""Like `_make_soul`, but with a real `PythinkerToolset` — required to exercise the
167171
identical-call repeat backstop, which is tracked on `PythinkerToolset` specifically."""
168172
llm = LLM(chat_provider=provider, max_context_size=100_000, capabilities=set())
169-
runtime = Runtime(
170-
config=runtime.config,
171-
llm=llm,
172-
session=runtime.session,
173-
builtin_args=runtime.builtin_args,
174-
denwa_renji=runtime.denwa_renji,
175-
approval=runtime.approval,
176-
labor_market=runtime.labor_market,
177-
environment=runtime.environment,
178-
notifications=runtime.notifications,
179-
background_tasks=runtime.background_tasks,
180-
skills=runtime.skills,
181-
oauth=runtime.oauth,
182-
additional_dirs=runtime.additional_dirs,
183-
skills_dirs=runtime.skills_dirs,
184-
role=runtime.role,
185-
)
173+
runtime = _rebuild_runtime_with_llm(runtime, llm)
186174
toolset = PythinkerToolset()
187175
toolset.add(_BoomTool())
188176
toolset.add(_OkTool())

0 commit comments

Comments
 (0)