fix: same-origin WS, ESC turn cleanup, recall framing, web token bootstrap - #104
Merged
Conversation
…ersion - Auto-populate allowed origins in local mode too: with token auth on, the origin check is enforced, and the previously empty allowlist rejected every request carrying an Origin header — breaking all session-stream WebSocket handshakes with 403/1006. REST GETs worked only because browsers omit Origin on same-origin GET fetches. - Standardize the web/vis startup banners on a shared PYTHINKER wordmark in utils/server.py, replacing the legacy upstream art. - Serve the installed CLI version at runtime via /api/config (GlobalConfig.version); the web UI header now prefers it over the Vite build-time constant, which goes stale when the CLI is upgraded without a frontend rebuild. Rebuilt the bundled static assets. - Add regression tests for local-mode origin population and the empty-allowlist reject-all semantics.
… agent glyphs Selectively adopted from an earlier TUI iteration, re-based onto the current design system: - report.py: detect top-level "Label: body" lines in report-like assistant prose and render them as structured sections, with conservative guards so ordinary paragraphs stay plain Markdown. - markdown.py: wide multi-column report tables render as stacked records so long paths and prose wrap in one generous value column instead of being sliced mid-word across narrow grid cells; compact tables keep the bordered grid. - todo: normalize LLM-supplied status aliases (complete/completed/ finished -> done, canceled -> cancelled) via a before-validator. - agent renderer: status glyphs (check/cross/dot) and type-first row layout for subagent activity. Deliberately NOT adopted (superseded by the current standardized design): the question-marker and markdown-palette recolors, and the space-separated tool header format with column-grid wrapping, which conflicts with the pinned parenthesized header style.
GHSA-866g-f22w-33x8 (uncontrolled resource consumption) affects @ai-sdk/provider-utils <=3.0.97, which every ai@5.x release pins; the patched 4.x line ships only with ai@6. The web UI imports the ai package exclusively for types (ChatStatus, FileUIPart, ToolUIPart, LanguageModelUsage), so the major bump is type-level only — tsc and biome pass unchanged. The remaining elliptic advisory (GHSA-848j-6mx2-7j84, low) has no patched release in any version; it enters via vite-plugin-node-polyfills -> crypto-browserify at build time only, and the bundle never includes it because only the path and url polyfills are enabled.
…strap Three field-found fixes: - memory/recall: frame the recalled-memory block as background context from past sessions, not an instruction. Without the guard the model could treat a recalled note or stale todo as the current request (e.g. answering a plain "ping" by resuming an old code-review task). Open todos are now labelled reference-only. - background tasks: track tasks spawned during the current interactive turn and kill exactly those on ESC. Previously a background subagent launched mid-turn survived the interrupt, finished later, and re-delivered the abandoned task via its completion notification. Earlier turns' tasks are deliberately left running. - web auth: consume the URL token before React mounts instead of in a component effect. Mount-time data fetches fired first and sent a stale localStorage token from a previous server run, yielding 401s on first load.
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR hardens interrupt handling for background tasks, strengthens recall context framing to prevent hallucination, normalizes todo status fields, reorders web auth token consumption, improves UI rendering for tables and reports, exposes version information to the frontend, consolidates banner assets, and enforces web origin deduplication. ChangesInterrupt handling, recall hardening, and UI improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/pythinker_code/web/api/config.py`:
- Around line 121-124: The try/except around get_version() silently swallows
errors; update the except to capture the exception (e.g., except Exception as e)
and log it before falling back to cli_version = ""—use the module logger (or
logging.getLogger(__name__)) and logger.exception or logger.error(...,
exc_info=True) so the failure of get_version() is observable while preserving
the existing fallback behavior for cli_version.
In `@tests/web/test_web_origins.py`:
- Around line 47-73: The test's fake_uvicorn_run callback declares an unused
positional parameter named args which triggers linters; rename that parameter to
_args or _ (in the fake_uvicorn_run definition inside
test_local_mode_populates_allowed_origins) to indicate it is intentionally
unused, keeping the kwargs handling and captured_port logic and leaving
monkeypatch.setattr("uvicorn.run", fake_uvicorn_run) and calls to run_web_server
unchanged.
In `@web/src/hooks/usePythinkerVersion.ts`:
- Around line 7-17: The current fetch logic uses the module-level
serverVersionPromise and never resets it, so a failed fetch permanently blocks
retries; update the fetch flow so that when fetchServerVersion (or the code that
assigns serverVersionPromise) catches an error or returns null it resets
serverVersionPromise = null and logs the error (use processLogger or console) to
allow a subsequent mount to retry; specifically modify fetchServerVersion's
catch block (or the promise assignment site that uses
apiClient.config.getGlobalConfigApiConfigGet) to clear serverVersionPromise on
failure and emit a brief error log.
🪄 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 Plus
Run ID: 2a96cf52-99d0-48e1-87ac-4855904c71d3
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
src/pythinker_code/background/manager.pysrc/pythinker_code/memory/recall.pysrc/pythinker_code/tools/todo/__init__.pysrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/components/markdown.pysrc/pythinker_code/ui/shell/components/report.pysrc/pythinker_code/ui/shell/tool_renderers/agent.pysrc/pythinker_code/utils/server.pysrc/pythinker_code/vis/app.pysrc/pythinker_code/web/api/config.pysrc/pythinker_code/web/app.pytasks/todo.mdtests/background/test_manager.pytests/core/test_recall_provider.pytests/tools/test_todo.pytests/ui/test_shell_markdown.pytests/ui_and_conv/test_report.pytests/ui_and_conv/test_shell_interrupt_cleanup.pytests/web/test_web_origins.pyweb/package.jsonweb/src/App.tsxweb/src/bootstrap.tsxweb/src/components/pythinker-code-brand.tsxweb/src/hooks/usePythinkerVersion.tsweb/src/lib/api/models/GlobalConfig.ts
💤 Files with no reviewable changes (1)
- web/src/App.tsx
- web config API: log get_version() failures instead of swallowing them, so an operator can see when the version banner falls back to empty - usePythinkerVersion: reset the shared promise and log on a failed/empty fetch so a transient error no longer permanently disables the backend version banner for the session - test_web_origins: rename unused *args to *_args to signal intent - CHANGELOG: add the missing ## Unreleased entry for this PR's web fixes (unblocks the required changelog-entry-required check) - AGENTS.md: document the changelog-entry-before-PR requirement as a gotcha to stop this check repeatedly blocking PRs
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bundle of field-found fixes around the web server, interrupt handling, and memory recall, plus TUI report polish and a dependency advisory fix.
Web origins, banner & version sync (0d0d549)
TUI report rendering (94327b3)
Interrupt & recall hardening (8cb8efa)
Root-caused from a real session where a fresh-session "ping" was answered by resuming an old code-review task, and ESC failed to stop a background subagent:
begin_turn()/kill_turn_tasks()registry. ESC now kills exactly the background tasks spawned by the interrupted turn; earlier turns' tasks deliberately survive. Previously a mid-turn background subagent survived the interrupt and re-delivered the abandoned task via its completion notification.?token=URL parameter is consumed before React mounts. Previously a component effect ran after mount-time data fetches, which went out with a stale localStorage token from a prior server run and 401'd on first load (GET /api/sessions/?archived=true).Dependency advisory (44f46f7)
ai5.x → 6.x to clear the@ai-sdk/provider-utilsadvisory.Testing
tests/background, recall (tests/core/test_recall_*), and shell suites, including 5 new regression tests (tests/ui_and_conv/test_shell_interrupt_cleanup.py, turn-kill tests intests/background/test_manager.py, recall framing pin intests/core/test_recall_provider.py).tsc -b+ biome clean on the web frontend;npm run buildsucceeds.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation