Skip to content

fix: GPT session re-auth crash + cleaner ImplementAndJudge card - #181

Merged
elkaix merged 2 commits into
mainfrom
fix/gpt-reauth-crash-and-iaj-card
Jun 24, 2026
Merged

fix: GPT session re-auth crash + cleaner ImplementAndJudge card#181
elkaix merged 2 commits into
mainfrom
fix/gpt-reauth-crash-and-iaj-card

Conversation

@elkaix

@elkaix elkaix commented Jun 24, 2026

Copy link
Copy Markdown
Member

Summary

Two independent, small fixes (one commit each).

1. fix(core): GPT/ChatGPT mid-session credential crash → typed 401

When a ChatGPT (openai_codex) session token is invalidated mid-session — classically because the same OAuth credentials were reused on another machine, rotating the single-use refresh token so this machine's refresh is rejected — OAuthManager._apply_access_token(runtime, ref, "") blanks the live OpenAI client's api_key to "". The client was already constructed with a valid token, so this slips past the SDK's construction-time credential check and the next responses.create raises a bare TypeError from _validate_headers ("Could not resolve authentication method…"). That TypeError is neither OpenAIError nor httpx.HTTPError, so it escaped the provider and the soul's 401/connection recovery and surfaced as a fatal "Unexpected error".

The provider now converts that unresolved-credential TypeError into a typed APIStatusError(401) (gated on not self._client.api_key, so unrelated TypeErrors still propagate). APIStatusError subclasses ChatProviderError, so it routes into the existing refresh → re-auth path and the shell's "Type /login to re-authenticate" message instead of crashing.

Reproduced end-to-end (identical TypeError); fix verified for both stream=True and stream=False (the SDK raises at request-build time, so generate() catches both).

Out of scope (noted, not changed here): _apply_access_token(runtime, ref, "") discards a possibly-still-valid access token just because the refresh token died — a separate token-lifecycle change.

2. fix(tui): cleaner ImplementAndJudge card

The ImplementAndJudge chain tool had no dedicated renderer, so it fell back to the generic ImplementAndJudge(4 args: acceptance, base_prompt, brief, scope) arg dump. Added a dedicated renderer:

⏺ Implement & Judge — Rewrite Logo.astro GSAP: matchMedia + scoped context

Internal tool name stays ImplementAndJudge (registry/config/tests unchanged); only the on-screen label changes. The judge subagent name was already consistent across specs/profiles/docs — no naming changes needed.

Tests

  • packages/pythinker-core/tests/test_openai_responses_auth.py: blanked-credential → APIStatusError(401) for both stream modes; negative case (TypeError with non-empty key still propagates).
  • tests/ui_and_conv/test_tui_card_tool_renderers.py: clean label + brief, no arg dump; running state; a name-sync invariant so a rename can't silently fall back to the generic renderer.

Verification

make check-pythinker-core ✅ · make check-pythinker-code ✅ (ruff + format + pyright + ty) · pythinker-core suite 305 passed · TUI renderer suite 157 passed.

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Fixed crash when GPT/ChatGPT session tokens expire; users are now redirected to login instead of encountering an error.
  • UI Improvements

    • Enhanced "Implement & Judge" chain display to show a cleaner format with brief descriptions instead of raw argument details.

elkaix added 2 commits June 23, 2026 22:31
When OAuth refresh blanks the live client's api_key mid-session, the OpenAI
SDK raises a bare TypeError that bypassed error handlers. Map that case to
APIStatusError(401) so refresh and /login recovery can engage.
Add a dedicated tool renderer so the chain shows "Implement & Judge — <brief>"
instead of a raw argument dump. Document unreleased auth-recovery and TUI fixes.
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Two independent fixes: OpenAIResponses.generate() now catches TypeError caused by a blank api_key and converts it to a typed APIStatusError(401), routing expired session tokens into the standard re-auth flow. Separately, a new implement_judge TUI renderer displays ImplementAndJudge chains as "⏺ Implement & Judge — <brief>" with a collapsible result block instead of a raw argument dump.

Changes

OpenAI Session Token 401 Fix

Layer / File(s) Summary
TypeError-to-401 conversion in OpenAIResponses.generate()
packages/pythinker-core/src/pythinker_core/contrib/chat_provider/openai_responses.py, packages/pythinker-core/tests/test_openai_responses_auth.py
Imports APIStatusError, adds a TypeError catch that raises APIStatusError(401) when api_key is blank, re-raises otherwise. Tests cover both streaming/non-streaming 401 conversion and the pass-through of unrelated TypeErrors.

ImplementAndJudge TUI Renderer

Layer / File(s) Summary
Renderer implementation and wiring
src/pythinker_code/ui/shell/tool_renderers/implement_judge.py, src/pythinker_code/ui/shell/tool_renderers/__init__.py, tests/ui_and_conv/test_tui_card_tool_renderers.py
New module implements _compact(), _render_call(), _label_header(), _render_result(), and exports IMPLEMENT_JUDGE_RENDERER. The __init__.py lazy-imports and registers it. Three smoke tests assert friendly label rendering in completed and running states, and name-constant consistency.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

bug

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 21.43% which is insufficient. The required threshold is 70.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title follows conventional commits format with valid 'fix' type and clearly describes both fixes: GPT re-auth crash and cleaner ImplementAndJudge card display.
Description check ✅ Passed Description includes detailed explanation of both fixes, reproduction/verification steps, test coverage, and verification results. All required template sections are present and complete.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/gpt-reauth-crash-and-iaj-card

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 74.07407% with 14 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...er_code/ui/shell/tool_renderers/implement_judge.py 72.00% 10 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/pythinker-core/tests/test_openai_responses_auth.py`:
- Around line 20-21: Add the `@pytest.mark.asyncio` marker to both async test
functions in the file. The functions `test_blanked_credential_raises_typed_401`
and `test_typeerror_with_valid_key_propagates` are defined as `async def` but
lack the required marker. Since the project does not configure `asyncio_mode =
"auto"` in pytest configuration, these async functions will not be properly
awaited by pytest and will silently pass without executing their assertions.
Place `@pytest.mark.asyncio` on a new line immediately before each async
function definition, above any other decorators like `@pytest.mark.parametrize`.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 82f5d8a7-c1fd-42eb-b680-d00470dd2341

📥 Commits

Reviewing files that changed from the base of the PR and between 9677c57 and 2d4b1b0.

📒 Files selected for processing (6)
  • CHANGELOG.md
  • packages/pythinker-core/src/pythinker_core/contrib/chat_provider/openai_responses.py
  • packages/pythinker-core/tests/test_openai_responses_auth.py
  • src/pythinker_code/ui/shell/tool_renderers/__init__.py
  • src/pythinker_code/ui/shell/tool_renderers/implement_judge.py
  • tests/ui_and_conv/test_tui_card_tool_renderers.py

Comment thread packages/pythinker-core/tests/test_openai_responses_auth.py
@elkaix

elkaix commented Jun 24, 2026

Copy link
Copy Markdown
Member Author

All findings triaged on head 2d4b1b0:

  • Inline (asyncio markers): skipped — root pytest.ini already sets asyncio_mode = auto; matches existing pythinker-core async test convention; CI green.
  • Docstring coverage warning: skipped — CodeRabbit advisory only; not enforced by repo CI/lint gates; renderer follows existing tool-renderer docstring pattern.
  • Codecov patch note: skipped — codecov/patch check passed.

No code changes required.

@elkaix
elkaix merged commit b092fef into main Jun 24, 2026
42 checks passed
@elkaix
elkaix deleted the fix/gpt-reauth-crash-and-iaj-card branch July 17, 2026 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant