[AI] Centralize default max_tokens#205
Conversation
|
👋 Thanks for the PR, @sjsjsjjs534! To get this PR reviewed, please ⭐ star this repo — it's free and helps others discover it. Once starred, run A |
| monkeypatch.setattr(urllib.request, "urlopen", mock_urlopen) | ||
| monkeypatch.setattr("termstory.config.load_config", lambda: {}) | ||
| monkeypatch.setattr(ai, "_get_all_active_project_contexts", lambda: []) | ||
| monkeypatch.setattr(ai, "DEFAULT_MAX_TOKENS", 1234, raising=False) |
There was a problem hiding this comment.
The
raising=False flag is not needed here since DEFAULT_MAX_TOKENS is definitively exported from termstory.ai. With raising=False, a typo in the attribute name (e.g. DEFAULT_MAX_TOKEN) would silently set a new attribute on the module instead of patching the existing one, the generators would still read the old 1500 value, and all five assertions would fail with 1500 != 1234 — but the monkeypatch itself would give no warning. Dropping the flag makes the intent clear and lets pytest raise immediately if the name is ever wrong.
| monkeypatch.setattr(ai, "DEFAULT_MAX_TOKENS", 1234, raising=False) | |
| monkeypatch.setattr(ai, "DEFAULT_MAX_TOKENS", 1234) |
|
miku /check-star |
|
👋 Thanks for the PR, @sjsjsjjs534! To get this PR reviewed, please ⭐ star this repo — it's free and helps others discover it. Once starred, run A |
|
/rewrite |
|
miku ✅ PR description rewritten! Check the PR body above. PreviewSummaryThis PR centralizes the hardcoded ChangesCore
Tests
Fixes
Breaking ChangesNone TestingRun the AI test suite with isolated config directories: XDG_CONFIG_HOME=/private/tmp/termstory-test-config XDG_DATA_HOME=/private/tmp/termstory-test-data .venv/bin/python -m pytest tests/test_ai.py -qRun the full test suite: XDG_CONFIG_HOME=/private/tmp/termstory-test-config XDG_DATA_HOME=/private/tmp/termstory-test-data .venv/bin/python -m pytest tests/ -qNotesThe constant |
|
/triage |
|
miku ✅ Marked as triaged ( |
|
|
||
| # Module-level defaults — used when config is unavailable. | ||
| DEFAULT_MAX_TOKENS: int = 1500 | ||
| _DEFAULT_MAX_FAILURES: int = 3 |
There was a problem hiding this comment.
| _DEFAULT_MAX_FAILURES: int = 3 | |
| # Module-level defaults — used when config is unavailable. | |
| # Note: DEFAULT_MAX_TOKENS is public (no underscore prefix) because tests | |
| # override it via monkeypatch.setattr. The other defaults stay private. | |
| DEFAULT_MAX_TOKENS: int = 1500 | |
| _DEFAULT_MAX_FAILURES: int = 3 | |
| _DEFAULT_COOLDOWN_SECONDS: float = 60.0 |
The naming is inconsistent: DEFAULT_MAX_TOKENS (public) vs _DEFAULT_MAX_FAILURES (private). Either the public one should also be private, or the others should be public. Currently DEFAULT_MAX_TOKENS is the only public constant and the convention isn't clear.
| resp_payload = {"choices": [{"message": {"content": "ok"}}]} | ||
| return MockResponse(json.dumps(resp_payload).encode("utf-8")) | ||
|
|
||
| monkeypatch.setattr(urllib.request, "urlopen", mock_urlopen) |
There was a problem hiding this comment.
The new test test_long_form_generators_share_default_max_tokens (75 lines) overlaps with the existing test_default_max_tokens_propagates_to_request_body test, which already verifies that DEFAULT_MAX_TOKENS propagates to the request body. The new one exercises generate_rpg_bio and 4 other generators — that's useful but could be folded into the existing test as additional assertions rather than a separate test that mostly duplicates the assertion pattern.
There was a problem hiding this comment.
Pull request overview
This PR centralizes the long-form AI generators’ default max_tokens value by introducing a single exported constant (DEFAULT_MAX_TOKENS) in termstory/ai.py, then updates the generators and tests to reference it for consistency and easier tuning.
Changes:
- Added
DEFAULT_MAX_TOKENSintermstory/ai.pyand replaced hardcoded1500at the five long-form generator call sites. - Updated
_send_llm_request’s defaultmax_tokensparameter to reference the new constant. - Updated
tests/test_ai.pyto assert againstai.DEFAULT_MAX_TOKENSand added a regression test intended to ensure all long-form generators share the same default.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| termstory/ai.py | Introduces DEFAULT_MAX_TOKENS and updates long-form generator calls (and _send_llm_request default) to use it. |
| tests/test_ai.py | Updates assertions to use the constant and adds a regression test for shared defaults. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| model_name: str, | ||
| provider: str, | ||
| max_tokens: int = 1500, | ||
| max_tokens: int = DEFAULT_MAX_TOKENS, | ||
| timeout: float = 30.0 | ||
| ) -> Optional[str]: |
|
👋 Thanks for the PR, @sjsjsjjs534! To get this PR reviewed, please ⭐ star this repo — it's free and helps others discover it. Once starred, run A |
|
@copilot resolve the merge conflicts in this pull request |
948eed3 to
58363e0
Compare
|
No reviewable files after applying ignore patterns. |
|
👋 Thanks for the PR, @sjsjsjjs534! To get this PR reviewed, please ⭐ star this repo — it's free and helps others discover it. Once starred, run A |
Summary
This PR centralizes the hardcoded
1500token limit used across five long-form AI generator functions into a single exportedDEFAULT_MAX_TOKENSconstant intermstory/ai.py, improving maintainability and ensuring consistent default behavior across all generators.Changes
Core
DEFAULT_MAX_TOKENS: int = 1500to centralize the default token limit for long-form AI generators._send_llm_requestfunction's default parameter frommax_tokens: int = 1500tomax_tokens: int = DEFAULT_MAX_TOKENS.generate_timeframe_summaryto useDEFAULT_MAX_TOKENSinstead of hardcoded1500when calling_send_llm_request.generate_wrapped_summaryto useDEFAULT_MAX_TOKENSinstead of hardcoded1500when calling_send_llm_request.translate_git_angerto useDEFAULT_MAX_TOKENSinstead of hardcoded1500when calling_send_llm_request.predict_bugs_from_sessionsto useDEFAULT_MAX_TOKENSinstead of hardcoded1500when calling_send_llm_request.generate_rpg_bioto useDEFAULT_MAX_TOKENSinstead of hardcoded1500when calling_send_llm_request.Tests
sys,SimpleNamespace, andimport termstory.ai as aito support the new regression test.test_timeframe_summary_max_tokensto referenceai.DEFAULT_MAX_TOKENSinstead of the hardcoded value1500in the assertion.test_long_form_generators_share_default_max_tokensthat patchesDEFAULT_MAX_TOKENSto1234and verifies all five long-form generators (generate_timeframe_summary,generate_wrapped_summary,translate_git_anger,predict_bugs_from_sessions,generate_rpg_bio) pick up the changed value.Fixes
max_tokens=1500duplicated across 5 call sites #192 — Centralizes hardcoded token limit into a shared constant for better maintainabilityBreaking Changes
None
Testing
Run the AI test suite with isolated config directories:
Run the full test suite:
Notes
The constant
DEFAULT_MAX_TOKENSis exported at the module level, making it available for external configuration or testing purposes. The regression test usesmonkeypatch.setattrwithraising=Falseto override the constant at runtime, ensuring that all five generators consistently reference the shared default rather than hardcoded values.