Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions sgpt/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,21 @@
run_command,
)

FIXED_TEMP_MODELS = {
"gpt-5",
"gpt-5-mini",
"o1-mini",
"o1-preview",
"o2-mini",
"o3-mini",
"gpt-5-nano",
"gpt-5.1",
"gpt-5.2",
"gpt-5.2-pro",
"o1",
"o3",
}


Comment on lines 25 to 41
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FIXED_TEMP_MODELS set contains speculative future model names like "gpt-5.1", "gpt-5.2", "gpt-5.2-pro", "o2-mini", "gpt-5-nano", etc. that may not exist yet or may never exist. This approach requires the code to be updated whenever OpenAI releases new models. Consider using a prefix-based matching approach (e.g., checking if model starts with "gpt-5" or matches the pattern "o[0-9]") to automatically handle future models in these series without code changes.

Suggested change
FIXED_TEMP_MODELS = {
"gpt-5",
"gpt-5-mini",
"o1-mini",
"o1-preview",
"o2-mini",
"o3-mini",
"gpt-5-nano",
"gpt-5.1",
"gpt-5.2",
"gpt-5.2-pro",
"o1",
"o3",
}
import re
class FixedTempModels(set):
"""
Container for models that should use a fixed temperature.
Behaves like a normal set for explicitly listed model names, but also
treats models whose names match known fixed-temperature patterns as
members (e.g., any "gpt-5*" or "o[0-9]*" model).
"""
def __contains__(self, value: object) -> bool:
# Preserve normal membership behavior for explicitly listed models.
if super().__contains__(value):
return True
# Only apply pattern logic to string model names.
if not isinstance(value, str):
return False
# Match any gpt-5* model (e.g., gpt-5.1, gpt-5.2-pro, gpt-5-nano, etc.).
if value.startswith("gpt-5"):
return True
# Match any o[0-9]* family model (e.g., o1, o1-mini, o2-mini, o3, o3-mini).
if re.match(r"^o[0-9]", value):
return True
return False
FIXED_TEMP_MODELS = FixedTempModels(
{
"gpt-5",
"gpt-5-mini",
"o1-mini",
"o1-preview",
"o2-mini",
"o3-mini",
"o1",
"o3",
}
)

Copilot uses AI. Check for mistakes.
def main(
prompt: str = typer.Argument(
Expand Down Expand Up @@ -210,6 +225,8 @@ def main(

if repl:
# Will be in infinite loop here until user exits with Ctrl+C.
if model in FIXED_TEMP_MODELS:
temperature = 1
Comment on lines +228 to +229
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition uses in for set membership but the variable is reassigned instead of checked for the default case. When a user explicitly provides a --temperature argument (e.g., --temperature 0.5), this code will silently override their choice and set it to 1.0 for models in FIXED_TEMP_MODELS. Consider checking if the temperature is at its default value (0.0) before overriding, or at minimum warn the user that their temperature setting is being ignored.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The temperature is set to integer 1 instead of float 1.0. While Python will handle the type conversion, it's inconsistent with the default value of 0.0 defined at line 53 and could cause type confusion. Use 1.0 for consistency.

Copilot uses AI. Check for mistakes.
ReplHandler(repl, role_class, md).handle(
init_prompt=prompt,
model=model,
Expand All @@ -220,6 +237,8 @@ def main(
)

if chat:
if model in FIXED_TEMP_MODELS:
temperature = 1
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The temperature is set to integer 1 instead of float 1.0. While Python will handle the type conversion, it's inconsistent with the default value of 0.0 defined at line 53 and could cause type confusion. Use 1.0 for consistency.

Copilot uses AI. Check for mistakes.
full_completion = ChatHandler(chat, role_class, md).handle(
prompt=prompt,
model=model,
Expand All @@ -229,6 +248,8 @@ def main(
functions=function_schemas,
)
else:
if model in FIXED_TEMP_MODELS:
temperature = 1
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The temperature is set to integer 1 instead of float 1.0. While Python will handle the type conversion, it's inconsistent with the default value of 0.0 defined at line 53 and could cause type confusion. Use 1.0 for consistency.

Copilot uses AI. Check for mistakes.
full_completion = DefaultHandler(role_class, md).handle(
prompt=prompt,
model=model,
Expand All @@ -255,6 +276,8 @@ def main(
full_completion = session.prompt("", default=full_completion)
continue
elif option == "d":
if model in FIXED_TEMP_MODELS:
temperature = 1
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The temperature is set to integer 1 instead of float 1.0. While Python will handle the type conversion, it's inconsistent with the default value of 0.0 defined at line 53 and could cause type confusion. Use 1.0 for consistency.

Copilot uses AI. Check for mistakes.
DefaultHandler(DefaultRoles.DESCRIBE_SHELL.get_role(), md).handle(
full_completion,
model=model,
Expand Down
13 changes: 11 additions & 2 deletions tests/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,22 @@ def cmd_args(prompt="", **kwargs):


def comp_args(role, prompt, **kwargs):
model = kwargs.get("model", cfg.get("DEFAULT_MODEL"))

# Mirror production logic: GPT-5 and o-series models require temperature 1.0,
# while other models default to 0.0 in tests.
if model.startswith("gpt-5") or model.startswith("o"):
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition model.startswith("o") is too broad and will match any model name starting with "o", not just o-series models (o1, o2, o3). For example, this would incorrectly match models like "openai-test" or "opus-model". Consider using a more specific check such as checking if the model starts with "o" followed by a digit, or better yet, use the same set-based matching approach as in sgpt/app.py for consistency.

Suggested change
if model.startswith("gpt-5") or model.startswith("o"):
is_o_series = len(model) > 1 and model[0] == "o" and model[1].isdigit()
if model.startswith("gpt-5") or is_o_series:

Copilot uses AI. Check for mistakes.
expected_temp = 1.0
else:
expected_temp = 0.0
Comment on lines +49 to +56
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test logic uses prefix matching with startswith(), while the production code in sgpt/app.py uses exact set membership matching. This creates an inconsistency where tests will pass for models like "gpt-5.3" or "o4-mini" (which aren't in the FIXED_TEMP_MODELS set), but the production code won't apply the temperature adjustment for them. The test helper should use the same FIXED_TEMP_MODELS set from sgpt/app.py to ensure tests accurately reflect production behavior.

Copilot uses AI. Check for mistakes.

return {
"messages": [
{"role": "system", "content": role.role},
{"role": "user", "content": prompt},
],
"model": cfg.get("DEFAULT_MODEL"),
"temperature": 0.0,
"model": model,
"temperature": expected_temp,
"top_p": 1.0,
"stream": True,
**kwargs,
Expand Down
Loading