Conversation
Backlog: TASK-12088
Backlog: TASK-12089
Backlog: TASK-12090
Address PR 2567 review follow-ups
Prepare 0.1.34 release metadata
📝 WalkthroughWalkthroughThis PR bumps the release to 0.1.34, fixes credential persistence gating in the frontend config hook, extracts job-event SQL filter fragments into a shared module, replaces print diagnostics with logging in WebSearch_APIs.py, migrates LGTM suppression comments to CodeQL directives across many files, introduces a new standalone MCP Unified docs corpus package (local import, SQLite/FTS5 store, optional URL acquisition, host adapter), delivers a large frontend hardening pass (URL/avatar security, character/assistant selection, chat streaming watchdog, Playground cockpit UI, extension background persistence, auth/streaming hardening, audio/TTS reliability, Zustand store migrations), implements a Workspace Assistant Defaults feature end-to-end, and hardens test infrastructure with a pytest quarantine mechanism, raised coverage gates, a perf-nightly workflow, and numerous new regression tests. Changes0.1.34 Release Preparation
Standalone MCP Unified Docs Corpus
Jobs Event Filter SQL Fragment Extraction
WebSearch Diagnostic Logging Refactor
CodeQL Suppression Hygiene and Misc Backend Fixes
Frontend URL, Avatar, and Payload Security Hardening
Character and Assistant Selection / Greeting Flow
Character Chat Streaming Watchdog and Abort Lifecycle
Playground Cockpit UI Rework
Extension Background Session Persistence and Storage Shims
Auth Token Refresh and Streaming Request Hardening
Audio/TTS Recording and Playback Reliability
Zustand Store Persistence Migrations
Workspace Assistant Defaults Feature
Test Infrastructure Hardening
Estimated code review effort: 5 (Critical) | ~180 minutes Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request prepares the repository for the 0.1.34 release by updating version metadata, adding release helper tests, restoring frontend auth persistence for reloads, migrating legacy LGTM comments to scoped CodeQL annotations, and moving Jobs event-filter SQL construction behind DB helpers. Review feedback highlights two issues: first, an incorrect use of standard library logging with Loguru's opt attribute in WebSearch_APIs.py which silences debug logs; second, an ineffective safety check in Helper_Scripts/release.py that can bypass validation if the post-release reference anchor is missing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
PR Summary by QodoRelease 0.1.34: metadata bump, CodeQL hygiene, and auth persistence fixes
AI Description
Diagram
High-Level Assessment
Files changed (62)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
74 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tldw_Server_API/app/core/Web_Scraping/WebSearch_APIs.py (1)
2229-2251: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueInconsistent redaction: DuckDuckGo smoke test logs raw result fields unredacted.
Other provider smoke tests (Brave, Google, Kagi, Searx, Tavily) route raw results through
_redact_websearch_log_value(...)before debug-logging. This DuckDuckGo test logsresult['title'],result['href'],result['body']directly without that pass, breaking the pattern established elsewhere in this same diff.♻️ Suggested consistency fix
for result in results: - logging.debug(f"DuckDuckGo title: {result['title']}") - logging.debug(f"DuckDuckGo URL: {result['href']}") - logging.debug(f"DuckDuckGo snippet: {result['body']}") + logging.debug(f"DuckDuckGo title: {_redact_websearch_log_value(result.get('title'))}") + logging.debug(f"DuckDuckGo URL: {_redact_websearch_log_value(result.get('href'))}") + logging.debug(f"DuckDuckGo snippet: {_redact_websearch_log_value(result.get('body'))}")🤖 Prompt for 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. In `@tldw_Server_API/app/core/Web_Scraping/WebSearch_APIs.py` around lines 2229 - 2251, The DuckDuckGo smoke test in test_search_duckduckgo is logging raw result fields directly, unlike the other provider smoke tests in WebSearch_APIs. Update the debug logging for each item in results to pass title, href, and body through _redact_websearch_log_value(...) before logging, and keep the existing parsing flow with parse_duckduckgo_results unchanged.
🤖 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 `@tldw_Server_API/app/core/Web_Scraping/WebSearch_APIs.py`:
- Around line 2539-2545: The raw-results debug logging in the WebSearch_APIs
path is overly defensive because `logging` is actually the Loguru logger alias,
so the `getattr(logging, "opt", None)` guard is unnecessary. Update the logging
around the raw payload message to use the Loguru logger directly in this code
path, either by importing `logger` explicitly or by calling `opt(lazy=True)` on
`logging` without the runtime fallback, keeping the behavior in the raw results
logging block clear and consistent.
---
Outside diff comments:
In `@tldw_Server_API/app/core/Web_Scraping/WebSearch_APIs.py`:
- Around line 2229-2251: The DuckDuckGo smoke test in test_search_duckduckgo is
logging raw result fields directly, unlike the other provider smoke tests in
WebSearch_APIs. Update the debug logging for each item in results to pass title,
href, and body through _redact_websearch_log_value(...) before logging, and keep
the existing parsing flow with parse_duckduckgo_results unchanged.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a608b738-a2e3-4a56-ad9b-7b6f6f8d9316
⛔ Files ignored due to path filters (1)
Docs/mkdocs.ymlis excluded by!docs/**
📒 Files selected for processing (61)
CHANGELOG.mdHelper_Scripts/release.pyREADME.mdapps/mcp-unified/src/mcp_unified/py.typedapps/tldw-frontend/e2e/smoke/chat-openui-dynamic-ui.spec.tsapps/tldw-frontend/e2e/smoke/chat-sticky-composer.spec.tsapps/tldw-frontend/e2e/smoke/smoke.setup.tsapps/tldw-frontend/e2e/ux-audit/knowledge-readiness-recovery.spec.tsapps/tldw-frontend/e2e/workflows/chat-cockpit.real-server.spec.tsapps/tldw-frontend/e2e/workflows/media-review.spec.tsapps/tldw-frontend/e2e/workflows/onboarding-ingestion-first.spec.tsapps/tldw-frontend/extension/shims/runtime-bootstrap.tsapps/tldw-frontend/hooks/__tests__/useConfig.networking.test.tsxapps/tldw-frontend/hooks/useConfig.tsxapps/tldw-frontend/scripts/chat-uat-driver.mjsbacklog/tasks/task-12088 - Address-PR-2567-review-follow-ups-on-dev.mdbacklog/tasks/task-12089 - Address-current-main-CodeQL-alerts-in-PR-2568.mdbacklog/tasks/task-12090 - Rebase-PR-2568-and-address-review-comments.mdbacklog/tasks/task-12091 - Prep-0.1.34-release-from-current-dev.mdpyproject.tomltldw_Server_API/app/api/v1/endpoints/agent_orchestration.pytldw_Server_API/app/api/v1/endpoints/audio/audio_voice_conversion.pytldw_Server_API/app/api/v1/endpoints/chatbooks.pytldw_Server_API/app/api/v1/endpoints/media/navigation.pytldw_Server_API/app/api/v1/endpoints/outputs.pytldw_Server_API/app/api/v1/endpoints/persona.pytldw_Server_API/app/api/v1/endpoints/rag_unified.pytldw_Server_API/app/api/v1/endpoints/reading.pytldw_Server_API/app/api/v1/endpoints/storage_download.pytldw_Server_API/app/api/v1/endpoints/vn_assets.pytldw_Server_API/app/api/v1/endpoints/watchlists.pytldw_Server_API/app/core/DB_Management/Sync_DB.pytldw_Server_API/app/core/DB_Management/db_path_utils.pytldw_Server_API/app/core/DB_Management/guardian_db_resolver.pytldw_Server_API/app/core/DB_Management/jobs_sql_fragments.pytldw_Server_API/app/core/Ingestion_Media_Processing/Audio/Audio_Files.pytldw_Server_API/app/core/Ingestion_Media_Processing/Audio/Audio_Transcription_Parakeet_MLX.pytldw_Server_API/app/core/Ingestion_Media_Processing/download_utils.pytldw_Server_API/app/core/Jobs/manager.pytldw_Server_API/app/core/Local_LLM/handler_utils.pytldw_Server_API/app/core/Local_LLM/llamacpp_inventory_service.pytldw_Server_API/app/core/MCP_unified/tests/test_web_research_module.pytldw_Server_API/app/core/Metrics/metrics_manager.pytldw_Server_API/app/core/Monitoring/notification_service.pytldw_Server_API/app/core/Notes_Tasks/service.pytldw_Server_API/app/core/Personalization/companion_user_ids.pytldw_Server_API/app/core/RAG/rag_service/payload_exemplars.pytldw_Server_API/app/core/Storage/generated_file_helpers.pytldw_Server_API/app/core/Sync/v2/blob_store.pytldw_Server_API/app/core/Sync/v2/factory.pytldw_Server_API/app/core/TTS/adapters/omnivoice_runtime.pytldw_Server_API/app/core/VN_Assets/storage.pytldw_Server_API/app/core/Web_Scraping/WebSearch_APIs.pytldw_Server_API/app/services/mcp_hub_path_enforcement_service.pytldw_Server_API/tests/CI/test_required_workflow_contracts.pytldw_Server_API/tests/Docs/test_release_docs_contract.pytldw_Server_API/tests/Jobs/test_jobs_event_filter_sql.pytldw_Server_API/tests/Metrics/test_sensitive_label_hashing.pytldw_Server_API/tests/TTS_NEW/unit/test_omnivoice_sidecar_server.pytldw_Server_API/tests/Utils/test_image_validation.pytldw_Server_API/tests/Web_Scraping/test_phase3_3_sanitizers.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 62 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
…aults-web-client Add Workspace Assistant Defaults V1
Validate release anchors, redact/lazy websearch diagnostics, harden MCP docs import/store policy paths, and fix frontend lifecycle/test issues raised on PR #2571.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… F1) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t F3) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding: pytest_collection_modifyitems receives the full session item list even in subdir conftests; the unscoped loop skipped the entire suite in mixed runs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s (audit F2) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding: MINIMAL_TEST_INCLUDE_AUDIO is global state that reload-based tests reset, making the audio cases flake to 404 in full-suite runs. Replaced with always-registered protected routes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… (audit F7) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…o 0.5s (audit F5) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add meta-test enforcing every unconditional pytest.mark.skip carries a reason=. Triage of the 36 existing bare-skip sites under tldw_Server_API/tests/**/test_*.py (audit estimated ~40) found all already carry specific, accurate reasons on inspection - meta-test passes with zero offenders, no site changes needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…udit F9 claim Review finding: the assert message contains 'pytest.mark.skip' so the scan flagged its own file, passing only by luck of word placement. Triage found 0 reason-less skips (audit's 40 were all already reasoned). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…F10)
sanitize_filename properties pass unmodified as specified. The chunk_text
content-preservation property required adjustment: Chunker._sanitize_input
(chunker.py:1355-1445) is a documented security-sanitization step that
chunk_text always runs first, and it legitimately mutates content in two
ways that a naive "word survives verbatim" property doesn't account for:
- Bidirectional-override control chars (U+202A-E, U+2066-9) are
unconditionally replaced with a space (chunker.py:1428-1437, "could be
used for spoofing"), which can split a token. Repro: chunk_text("ab")
-> ["a b"], token "ab" vanishes. Excluded these 9 codepoints from
the fuzzed alphabet since they change tokenization, not just content.
- NFC normalization (chunker.py:1391-1394, "to prevent various
unicode-based attacks") is gated on preserving character *count*, not
character *identity*: U+F900 (CJK COMPATIBILITY IDEOGRAPH-F900) has a
canonical singleton decomposition to U+8C48, same length, both render as
"豈". Hypothesis's shrinker converged on text='豈' as the minimal
counterexample. Adjusted the per-word expectation to mirror the same
length-preserving-NFC rule instead of asserting raw identity.
Both are the documented, intended effect of the security sanitizer, not
bugs, so the properties were corrected (with line-cited justification in
the test file) rather than xfailed. Re-validated at 5000+ examples across
multiple seeds after the fix. Full tests/unit CI-gated scope: 260 passed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding: per-word NFC gating in the test diverged from the sanitizer's whole-string gate (repro: 'é 敖'), a latent flake. Expectations now come from Chunker()._sanitize_input directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… F4) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Scheduled workflow at 06:00 UTC daily + manual trigger - Runs performance test suite (skipped by design; enable with PERF=1) - Uploads junit artifact to GitHub for trend analysis - Modeled on jobs-suite.yml patterns (checkout pin, setup-python-deps action) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding: all perf tests are skipif-gated on PERF; without it the nightly job would skip 6/6 forever (same pattern as RUN_JOBS in jobs-suite). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
run_local_ci disables plugin autoload but never loaded pytest-asyncio, so any changed async test failed under make ci-local (pre-existing; surfaced by this branch's new async suites). Aligns the base command with backend-required.yml. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… RateLimiting Final review findings: ci.yml shards run explicit file lists from the quarantined dirs (norecursedirs never applied to CLI args), so those jobs now set RUN_QUARANTINED=1 to keep running them; tests/RateLimiting added to a shard to satisfy the shard-coverage guard; audit docs corrected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t env, comments) - factor 3x-duplicated quarantine hook into tests/_plugins/quarantine.py; drop dead path-equality check; warn instead of silently skipping on unresolvable item paths - remove inert CHARACTER_RATE_LIMIT_ENABLED setenv from rate-limit fixture - hoist meta-test self-path to module constant - document sanitizer-purity assumption in chunker fuzz; expand malformed-body rationale comments in error-path sweep - add CI-shard nuance to audit executive summary Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…p (Jobs outbox) Pre-existing: line 19 reassigned pytestmark after line 12 conditionally appended a skip marker, silently discarding it. Found during 2026-07-02 audit remediation review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- contract test: coverage floor assertion 5 -> 12 (+ AuthNZ step) - contract test: fix shard-coverage checks stale after later ci.yml edits (Workspaces persona file, RateLimiting file); also fill in the 4 ci.yml matrix job blocks missing the RateLimiting shard entry - skip-reason meta-test: ignore commented-out markers, scan conftest.py too - perf-nightly: concurrency group, persist-credentials off, SHA-pin upload-artifact - frontend coverage step: pipefail so failures show as allowed-failures - quarantine skip reason + burn-down doc link tracking issue #2581 - audit docs: drop stale OAuth-gap claim in 4.2, replace outdated R3 snippet Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
test/ci: remediate 2026-07-02 testing audit (F1-F10)
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Helper_Scripts/release.py (1)
284-323: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnchor split logic is correct and matches the new tests.
The three anchors (current release line, beyond-release, post-release) are each validated independently with distinct
ValueErrormessages, consistent withtest_release_helper_raises_when_post_release_anchor_is_missingand the success test intest_release_docs_contract.py.One minor nit: the
replacementslist (lines 287-293) now holds a single entry, making the loop abstraction unnecessary — could be inlined for clarity, though not required.🤖 Prompt for 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. In `@Helper_Scripts/release.py` around lines 284 - 323, The anchor handling in update_readme_release_references is correct, but the replacements list/loop is now overkill because it contains only the current release line entry. Simplify the logic by inlining that single re.subn call inside update_readme_release_references and keep the distinct ValueError message for the current release anchor, while leaving the branch_line_pattern and post-release checks unchanged.apps/mcp-unified/src/mcp_unified/docs/importers/local.py (1)
37-65: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRun deferred keyword pruning even when a later file fails.
upsert_document(..., prune_orphan_keywords=False)commits per file, so a decode/parse/size error after earlier upserts exits before Line 64 and leaves stale orphan keywords. Wrap the loop withtry/finallyand prune when any document was imported.Proposed fix
- for file_path in files: - parsed = self._parse_file(file_path) - chunk_texts = chunks_from_text(parsed.text) - chunks = [ - { - "text": chunk, - "citation": f"{file_path.name}:{idx + 1}", - } - for idx, chunk in enumerate(chunk_texts) - ] - document_id = self.store.upsert_document( - scope=scope, - title=parsed.title, - document_type=parsed.document_type, - canonical_uri=parsed.canonical_uri, - source_path=parsed.source_path, - source_url=parsed.source_url, - text=parsed.text, - sections=[asdict(section) for section in parsed.sections], - chunks=chunks, - keywords=keyword_tuple, - collection_names=collection_tuple, - metadata={"importer": "local"}, - prune_orphan_keywords=False, - ) - imported.append({"id": document_id, "title": parsed.title, "chunks": len(chunks)}) - - if imported: - self.store.prune_orphan_keywords(scope=scope) + try: + for file_path in files: + parsed = self._parse_file(file_path) + chunk_texts = chunks_from_text(parsed.text) + chunks = [ + { + "text": chunk, + "citation": f"{file_path.name}:{idx + 1}", + } + for idx, chunk in enumerate(chunk_texts) + ] + document_id = self.store.upsert_document( + scope=scope, + title=parsed.title, + document_type=parsed.document_type, + canonical_uri=parsed.canonical_uri, + source_path=parsed.source_path, + source_url=parsed.source_url, + text=parsed.text, + sections=[asdict(section) for section in parsed.sections], + chunks=chunks, + keywords=keyword_tuple, + collection_names=collection_tuple, + metadata={"importer": "local"}, + prune_orphan_keywords=False, + ) + imported.append({"id": document_id, "title": parsed.title, "chunks": len(chunks)}) + finally: + if imported: + self.store.prune_orphan_keywords(scope=scope)🤖 Prompt for 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. In `@apps/mcp-unified/src/mcp_unified/docs/importers/local.py` around lines 37 - 65, The local importer currently prunes orphan keywords only after the loop, so a failure in _parse_file or store.upsert_document can skip cleanup after earlier per-file commits. Update the import flow in the local importer’s main loop to use try/finally around the file iteration, track whether any documents were successfully imported, and call self.store.prune_orphan_keywords(scope=scope) in the finally block when at least one document was upserted.apps/packages/ui/src/components/Option/Playground/Playground.tsx (1)
1567-1585: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnreachable duplicate check.
The
if (routeCharacterIntentId) return;at Line 1583-1585 can never execute with a truthy value: the identical check at Line 1574-1576 already returns earlier in the same synchronous call, beforerouteCharacterIntentId's captured value could change. This is dead code.🧹 Proposed cleanup
if (shouldRestorePersistedSessionOnInit) { const restored = await restoreSession(); if (restored) return; } - if (routeCharacterIntentId) { - return; - } if (readSidepanelChatWebUiHandoffFromLocation()) { return; }🤖 Prompt for 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. In `@apps/packages/ui/src/components/Option/Playground/Playground.tsx` around lines 1567 - 1585, The second routeCharacterIntentId guard in initializePlayground is unreachable because the same truthy check already returns earlier in the callback. Remove the duplicate if (routeCharacterIntentId) return; block from Playground.tsx and keep the earlier guard near initializePlayground’s route handling so the flow stays unchanged and dead code is eliminated.apps/packages/ui/src/components/Common/ChatGreetingPicker.tsx (1)
330-343: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo busy/disabled state on the Select button while persistence is in flight.
The guard prevents a second server write, but the button stays fully enabled and unchanged during the async
addChatMessagecall, so a user who clicks again gets silent no-op feedback. Consider disabling the button (or showing a spinner) whilegreetingPersistInFlightRef.currentis true for clearer feedback.🤖 Prompt for 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. In `@apps/packages/ui/src/components/Common/ChatGreetingPicker.tsx` around lines 330 - 343, The Select button in ChatGreetingPicker should reflect the in-flight persistence state instead of staying fully interactive. Update the button logic around handleSelectFirstMessage and the render of the Select action to use greetingPersistInFlightRef.current so it is disabled, or shows a loading/busy indicator, while addChatMessage is running. Keep the existing selectedOption/setMessages guards, but add the in-flight state to the button’s disabled handling and visual feedback so repeated clicks are clearly blocked.apps/packages/ui/src/hooks/useMessage.tsx (1)
1608-1619: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrectly resolves the previously-flagged controller-clobbering issue.
characterChatModehere receivescontrollerdirectly as a parameter (line 1234) and the watchdog callscontroller?.abort()on that specific instance rather than re-deriving it from shared state — so a newer turn overwriting the sharedabortControllerstate can't disable this turn's watchdog. Thefinallycleanup now correctly uses a functional update (current === controller ? null : current) instead of unconditionally nulling, addressing the prior review comment about clobbering a newer turn's controller.Note for context:
apps/packages/ui/src/hooks/chat/useChatActions.ts's siblingcharacterChatModeimplementation does not receive the controller directly and instead re-derives it from a shared ref at watchdog-setup time — which reintroduces a related race (see comment on that file). Given three independent copies of this watchdog now exist acrossuseCharacterChatMode.ts,useChatActions.ts, and this file, and they've already drifted once, extracting the watchdog + controller-ownership logic into a shared helper (taking the controller as an explicit parameter, as this file anduseCharacterChatMode.tsalready do) would prevent further divergence.Also applies to: 1979-1985
🤖 Prompt for 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. In `@apps/packages/ui/src/hooks/useMessage.tsx` around lines 1608 - 1619, The watchdog and controller-ownership logic in characterChatMode/useMessage has drifted across multiple copies, so extract it into a shared helper to keep behavior consistent. Move the inactivity timer setup/reset and cleanup into a reusable helper that takes the controller as an explicit parameter, and update characterChatMode plus the sibling implementations in useChatActions/useCharacterChatMode to call that helper instead of duplicating the logic. Ensure the helper preserves the existing functional cleanup behavior so it only clears the matching controller instance.
🤖 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 `@apps/packages/ui/src/components/Sidepanel/Chat/TtsClipsDrawer.tsx`:
- Around line 254-304: The aria-label for the play/stop button in TtsClipsDrawer
is hardcoded in English via playButtonLabel, while the adjacent Tooltip already
uses localized strings. Update the button label generation in the list.map(clip)
block to use t(...) with the same translation keys/pattern as the Tooltip,
including the clip label as a variable, so screen readers get localized text.
Keep the aria-label in sync with the playingClipId state and the existing
play/stop behavior.
In
`@apps/packages/ui/src/hooks/chat/__tests__/useChatActions.persona.integration.test.tsx`:
- Around line 345-416: This test is not covering the inherited assistant path
because useChatActions still sees selectedAssistant as truthy, so
inheritedTrackedAssistant never activates. Update the setup in
useChatActions.persona.integration.test.tsx so selectedAssistant is null while
keeping the other inherited inputs, and verify effectiveAssistantState.mode is
"plain" under the mocked useChatSettingsRecord/chatSettings so the
inheritedTrackedAssistant guard in useChatActions can run. Keep the existing
assertions on createChatMock, savePlaygroundSessionMock, and normalChatModeMock
to validate the fresh-chat inheritance behavior.
In `@apps/packages/ui/src/hooks/useMessageOption.tsx`:
- Around line 39-52: Extract the duplicated PersonaMemoryMode type and
normalizePersonaMemoryMode helper into a shared module and import them
everywhere they are used. Update useMessageOption, useChatActions,
personaServerChat, and WorkspaceChatPanel to reference the shared definitions
instead of local copies so the normalization logic stays consistent and cannot
drift.
In `@apps/packages/ui/src/hooks/usePersonaLiveControl.tsx`:
- Around line 338-346: The ws.onopen handler in usePersonaLiveControl should
ignore late events after the connection attempt has already failed or the
component is no longer mounted. Add the same settled/mounted guard used by the
failure path before calling setStreamState("open") and resolve(ws), and make
sure it checks the current connection attempt state so a timed-out or closed
socket cannot reopen the stream state.
In `@backlog/tasks/task-12115` -
Rebase-PR-2316-on-latest-dev-and-fix-CI-failures.md:
- Around line 2-7: The task document is reusing an existing identifier, which
makes tracking and backlinks ambiguous. Update the frontmatter in the task doc
so it has its own unique id instead of TASK-12115, and keep the rest of the
metadata intact. Use the task title and references to confirm you are editing
the correct document.
In `@IMPLEMENTATION_PLAN_pr2571_review_comments.md`:
- Around line 1-40: The markdown in
IMPLEMENTATION_PLAN_pr2571_review_comments.md needs heading normalization to
satisfy markdownlint. Add a single top-level heading for the plan document, and
ensure each stage heading has the required blank lines before and after it so
the sections are parsed consistently. Keep the existing stage content and
structure intact while updating the headings in the implementation plan blocks.
In `@tldw_Server_API/app/core/Web_Scraping/WebSearch_APIs.py`:
- Around line 113-128: The redaction logic in _redact_websearch_log_text only
masks exact query keys, so common credential parameters can still leak in logs.
Update the key check used when building safe_pairs to match broader sensitive
names such as access_token, refresh_token, client_secret, password, and similar
variants, while keeping the recursive value redaction behavior intact. Make sure
the fallback _WEBSEARCH_SENSITIVE_PAIR_RE path still catches sensitive key/value
pairs in non-URL text.
In `@tldw_Server_API/tests/Web_Scraping/test_phase3_3_sanitizers.py`:
- Around line 58-60: The fake logger’s debug method is too permissive when _lazy
is enabled, so it can hide behavior that real Loguru would reject. Update the
debug implementation in the test helper to enforce Loguru’s lazy=True contract
by validating that every arg is callable whenever self._lazy is true and raising
the same kind of failure for plain values; keep the existing message formatting
path in sync with the debug() helper used by the sanitizer tests.
In `@tldw_Server_API/tests/Workspaces/test_workspaces_api.py`:
- Around line 70-104: The workspace test suite has duplicated Persona setup
logic in _create_workspace_test_persona and the matching helper in
test_workspace_assistant_defaults_api, so extract the shared
create_persona_profile setup into a common helper or fixture under
tests/Workspaces/conftest.py. Keep the existing defaults and payload shape in
the shared helper, then update both test files to call it so Persona creation
stays consistent as the contract changes.
---
Outside diff comments:
In `@apps/mcp-unified/src/mcp_unified/docs/importers/local.py`:
- Around line 37-65: The local importer currently prunes orphan keywords only
after the loop, so a failure in _parse_file or store.upsert_document can skip
cleanup after earlier per-file commits. Update the import flow in the local
importer’s main loop to use try/finally around the file iteration, track whether
any documents were successfully imported, and call
self.store.prune_orphan_keywords(scope=scope) in the finally block when at least
one document was upserted.
In `@apps/packages/ui/src/components/Common/ChatGreetingPicker.tsx`:
- Around line 330-343: The Select button in ChatGreetingPicker should reflect
the in-flight persistence state instead of staying fully interactive. Update the
button logic around handleSelectFirstMessage and the render of the Select action
to use greetingPersistInFlightRef.current so it is disabled, or shows a
loading/busy indicator, while addChatMessage is running. Keep the existing
selectedOption/setMessages guards, but add the in-flight state to the button’s
disabled handling and visual feedback so repeated clicks are clearly blocked.
In `@apps/packages/ui/src/components/Option/Playground/Playground.tsx`:
- Around line 1567-1585: The second routeCharacterIntentId guard in
initializePlayground is unreachable because the same truthy check already
returns earlier in the callback. Remove the duplicate if
(routeCharacterIntentId) return; block from Playground.tsx and keep the earlier
guard near initializePlayground’s route handling so the flow stays unchanged and
dead code is eliminated.
In `@apps/packages/ui/src/hooks/useMessage.tsx`:
- Around line 1608-1619: The watchdog and controller-ownership logic in
characterChatMode/useMessage has drifted across multiple copies, so extract it
into a shared helper to keep behavior consistent. Move the inactivity timer
setup/reset and cleanup into a reusable helper that takes the controller as an
explicit parameter, and update characterChatMode plus the sibling
implementations in useChatActions/useCharacterChatMode to call that helper
instead of duplicating the logic. Ensure the helper preserves the existing
functional cleanup behavior so it only clears the matching controller instance.
In `@Helper_Scripts/release.py`:
- Around line 284-323: The anchor handling in update_readme_release_references
is correct, but the replacements list/loop is now overkill because it contains
only the current release line entry. Simplify the logic by inlining that single
re.subn call inside update_readme_release_references and keep the distinct
ValueError message for the current release anchor, while leaving the
branch_line_pattern and post-release checks unchanged.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54c8ae9a-199c-4b50-b41e-4eb204e71fb5
⛔ Files ignored due to path filters (2)
Docs/Product/Workspace_Persona_Defaults_PRD.mdis excluded by!docs/**Docs/superpowers/plans/2026-07-03-chat-focus-fullscreen.mdis excluded by!docs/**
📒 Files selected for processing (96)
.github/workflows/ci.yml.github/workflows/frontend-ux-gates.ymlHelper_Scripts/release.pyIMPLEMENTATION_PLAN_pr2571_review_comments.mdapps/mcp-unified/src/mcp_unified/docs/acquisition/extract.pyapps/mcp-unified/src/mcp_unified/docs/acquisition/models.pyapps/mcp-unified/src/mcp_unified/docs/acquisition/policy.pyapps/mcp-unified/src/mcp_unified/docs/importers/html.pyapps/mcp-unified/src/mcp_unified/docs/importers/local.pyapps/mcp-unified/src/mcp_unified/docs/models.pyapps/mcp-unified/src/mcp_unified/docs/standalone.pyapps/mcp-unified/src/mcp_unified/docs/store/sqlite.pyapps/packages/ui/src/components/Common/ChatGreetingPicker.tsxapps/packages/ui/src/components/Common/__tests__/ChatGreetingPicker.test.tsxapps/packages/ui/src/components/Layouts/Layout.tsxapps/packages/ui/src/components/Layouts/__tests__/Layout.shell-overrides.test.tsxapps/packages/ui/src/components/Option/ChatWorkspace/ChatWorkspaceConsole.tsxapps/packages/ui/src/components/Option/ChatWorkspace/ChatWorkspacePage.tsxapps/packages/ui/src/components/Option/ChatWorkspace/InspectorRail.tsxapps/packages/ui/src/components/Option/ChatWorkspace/WorkspaceChatPanel.tsxapps/packages/ui/src/components/Option/ChatWorkspace/__tests__/ChatWorkspacePage.test.tsxapps/packages/ui/src/components/Option/ChatWorkspace/__tests__/InspectorRail.test.tsxapps/packages/ui/src/components/Option/ChatWorkspace/__tests__/WorkspaceChatPanel.test.tsxapps/packages/ui/src/components/Option/ChatWorkspace/types.tsapps/packages/ui/src/components/Option/KnowledgeQA/KnowledgeQAProvider.tsxapps/packages/ui/src/components/Option/KnowledgeQA/__tests__/KnowledgeQAProvider.persistence.test.tsxapps/packages/ui/src/components/Option/Playground/Playground.tsxapps/packages/ui/src/components/Option/Playground/__tests__/Playground.cockpit-controls.test.tsxapps/packages/ui/src/components/Option/Playground/__tests__/Playground.cockpit-shell.test.tsxapps/packages/ui/src/components/Option/ResearchWorkspace/WorkspaceHeader.tsxapps/packages/ui/src/components/Option/ResearchWorkspace/__tests__/WorkspaceHeader.test.tsxapps/packages/ui/src/components/Sidepanel/Chat/TtsClipsDrawer.tsxapps/packages/ui/src/components/Sidepanel/Chat/__tests__/TtsClipsDrawer.overlap.test.tsxapps/packages/ui/src/context/index.tsxapps/packages/ui/src/hooks/__tests__/useAudioRecorder.test.tsapps/packages/ui/src/hooks/__tests__/useMessageOption.assistant-overlay.test.tsxapps/packages/ui/src/hooks/__tests__/usePersonaLiveControl.test.tsxapps/packages/ui/src/hooks/chat/__tests__/personaServerChat.test.tsapps/packages/ui/src/hooks/chat/__tests__/useChatActions.persona.integration.test.tsxapps/packages/ui/src/hooks/chat/personaServerChat.tsapps/packages/ui/src/hooks/chat/useChatActions.tsapps/packages/ui/src/hooks/useAudioRecorder.tsapps/packages/ui/src/hooks/useMessage.tsxapps/packages/ui/src/hooks/useMessageOption.tsxapps/packages/ui/src/hooks/usePersonaLiveControl.tsxapps/packages/ui/src/services/__tests__/tldw-api-client.connection-sync.test.tsapps/packages/ui/src/services/__tests__/tldw-api-client.workspace-api.test.tsapps/packages/ui/src/services/persona-stream.tsapps/packages/ui/src/services/tldw/TldwApiClient.tsapps/packages/ui/src/services/tldw/TldwModels.tsapps/packages/ui/src/services/tldw/domains/chat-rag.tsapps/packages/ui/src/services/tldw/domains/workspace-api.tsapps/packages/ui/src/store/__tests__/workspace.test.tsapps/packages/ui/src/store/workspace-slices/workspace-list-slice.tsapps/packages/ui/src/store/workspace.tsapps/packages/ui/src/types/workspace-assistant-defaults.tsapps/packages/ui/src/types/workspace.tsapps/tldw-frontend/__tests__/components/layout/WebLayout.chat-scroll-contract.test.tsxapps/tldw-frontend/__tests__/extension/plasmo-storage-watch.test.tsxapps/tldw-frontend/__tests__/extension/runtime-bootstrap.test.tsapps/tldw-frontend/e2e/smoke/chat-openui-dynamic-ui.spec.tsapps/tldw-frontend/e2e/smoke/chat-sticky-composer.spec.tsapps/tldw-frontend/e2e/smoke/smoke.setup.tsapps/tldw-frontend/e2e/ux-audit/knowledge-readiness-recovery.spec.tsapps/tldw-frontend/e2e/workflows/chat-cockpit.real-server.spec.tsapps/tldw-frontend/extension/shims/runtime-bootstrap.tsapps/tldw-frontend/scripts/chat-uat-driver.mjsbacklog/tasks/task-12115 - Make-chat-focus-mode-truly-fullscreen.mdbacklog/tasks/task-12115 - Rebase-PR-2316-on-latest-dev-and-fix-CI-failures.mdbacklog/tasks/task-12116 - Address-PR-2571-review-comments.mdbacklog/tasks/task-2278 - Implement-Workspace-Assistant-Defaults-backend-storage.mdbacklog/tasks/task-2318 - Plan-Workspace-Assistant-Defaults-implementation.mdbacklog/tasks/task-2318.1 - Map-Workspace-Assistant-Defaults-in-WebUI-client-and-store.mdbacklog/tasks/task-2318.2 - Expose-Workspace-Assistant-Defaults-API-validation-and-effective-state.mdbacklog/tasks/task-2318.3 - Add-Workspace-default-assistant-settings-UI.mdbacklog/tasks/task-2318.4 - Apply-Workspace-Persona-defaults-to-Chat-Workspace-startup.mdbacklog/tasks/task-2318.5 - Close-out-Workspace-Assistant-Defaults-V1-verification.mdbacklog/tasks/task-2318.6 - Address-Workspace-Assistant-Defaults-PR-review-feedback.mdtldw_Server_API/Config_Files/mcp_modules.yamltldw_Server_API/app/api/v1/endpoints/media/navigation.pytldw_Server_API/app/api/v1/endpoints/workspaces.pytldw_Server_API/app/api/v1/schemas/workspace_schemas.pytldw_Server_API/app/core/MCP_unified/modules/implementations/docs_module.pytldw_Server_API/app/core/MCP_unified/tests/test_web_research_module.pytldw_Server_API/app/core/Sync/v2/blob_store.pytldw_Server_API/app/core/Web_Scraping/WebSearch_APIs.pytldw_Server_API/tests/CI/test_required_workflow_contracts.pytldw_Server_API/tests/Docs/test_release_docs_contract.pytldw_Server_API/tests/MCP_unified/docs/test_docs_acquisition_extract.pytldw_Server_API/tests/MCP_unified/docs/test_docs_acquisition_policy.pytldw_Server_API/tests/MCP_unified/docs/test_docs_importers.pytldw_Server_API/tests/Sync/test_sync_v2_security.pytldw_Server_API/tests/Utils/test_image_validation.pytldw_Server_API/tests/Web_Scraping/test_phase3_3_sanitizers.pytldw_Server_API/tests/Workspaces/test_workspace_assistant_defaults_api.pytldw_Server_API/tests/Workspaces/test_workspaces_api.py
💤 Files with no reviewable changes (3)
- tldw_Server_API/tests/Utils/test_image_validation.py
- apps/packages/ui/src/services/persona-stream.ts
- apps/mcp-unified/src/mcp_unified/docs/acquisition/extract.py
Comment on lines
+254
to
+304
| {list.map((clip) => { | ||
| const clipLabel = clip.textPreview || clip.utterance || clip.id | ||
| const playButtonLabel = | ||
| playingClipId === clip.id | ||
| ? `Stop clip ${clipLabel}` | ||
| : `Play clip ${clipLabel}` | ||
| return ( | ||
| <div | ||
| key={clip.id} | ||
| className="flex flex-col gap-2 rounded-xl border border-border bg-surface2/70 p-3" | ||
| > | ||
| <Tooltip title={clip.utterance || clip.textPreview}> | ||
| <div className="text-sm text-text truncate"> | ||
| {clip.textPreview || clip.utterance} | ||
| </div> | ||
| </Tooltip> | ||
| <Tooltip | ||
| title={ | ||
| clip.segments.length > 1 | ||
| ? t( | ||
| "playground:ttsClips.downloadAll", | ||
| "Download all segments" | ||
| ) | ||
| : t("playground:ttsClips.download", "Download") | ||
| } | ||
| > | ||
| <button | ||
| type="button" | ||
| onClick={() => handleDownload(clip)} | ||
| className="rounded-md border border-border bg-surface px-2 py-1 text-text-muted hover:bg-surface2 hover:text-text" | ||
| <div className="flex flex-wrap items-center gap-2 text-[11px] text-text-muted"> | ||
| <span>{formatTimestamp(clip.createdAt)}</span> | ||
| <span>{t("playground:ttsClips.segmentsLabel", { count: clip.segments.length, defaultValue: "{{count}} segments" })}</span> | ||
| {clip.provider && ( | ||
| <Tag color="blue" className="!text-[10px]"> | ||
| {clip.provider} | ||
| </Tag> | ||
| )} | ||
| {clip.voice && ( | ||
| <Tag color="purple" className="!text-[10px]"> | ||
| {clip.voice} | ||
| </Tag> | ||
| )} | ||
| </div> | ||
| <div className="flex items-center gap-2"> | ||
| <Tooltip | ||
| title={ | ||
| playingClipId === clip.id | ||
| ? t("playground:tts.stop", "Stop") | ||
| : t("playground:tts.play", "Play") | ||
| } | ||
| > | ||
| <Download className="h-4 w-4" /> | ||
| </button> | ||
| </Tooltip> | ||
| <Tooltip title={t("common:delete", "Delete") as string}> | ||
| <button | ||
| type="button" | ||
| onClick={() => handleDelete(clip)} | ||
| className="rounded-md border border-border bg-surface px-2 py-1 text-text-muted hover:bg-surface2 hover:text-text" | ||
| <button | ||
| type="button" | ||
| aria-label={playButtonLabel} | ||
| onClick={() => handleTogglePlay(clip)} | ||
| className="rounded-md border border-border bg-surface px-2 py-1 text-text-muted hover:bg-surface2 hover:text-text" | ||
| > | ||
| {playingClipId === clip.id ? ( | ||
| <Square className="h-4 w-4 text-danger" /> | ||
| ) : ( | ||
| <Volume2 className="h-4 w-4" /> | ||
| )} | ||
| </button> | ||
| </Tooltip> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
aria-label not localized, unlike the adjacent tooltip.
playButtonLabel is hardcoded English (Play clip ... / Stop clip ...) while the Tooltip text a few lines below uses t("playground:tts.play", ...) / t("playground:tts.stop", ...). Screen reader users on non-English locales will hear English labels despite localized visible tooltips.
♻️ Proposed fix
- const playButtonLabel =
- playingClipId === clip.id
- ? `Stop clip ${clipLabel}`
- : `Play clip ${clipLabel}`
+ const playButtonLabel =
+ playingClipId === clip.id
+ ? t("playground:ttsClips.stopClipLabel", "Stop clip {{label}}", { label: clipLabel })
+ : t("playground:ttsClips.playClipLabel", "Play clip {{label}}", { label: clipLabel })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {list.map((clip) => { | |
| const clipLabel = clip.textPreview || clip.utterance || clip.id | |
| const playButtonLabel = | |
| playingClipId === clip.id | |
| ? `Stop clip ${clipLabel}` | |
| : `Play clip ${clipLabel}` | |
| return ( | |
| <div | |
| key={clip.id} | |
| className="flex flex-col gap-2 rounded-xl border border-border bg-surface2/70 p-3" | |
| > | |
| <Tooltip title={clip.utterance || clip.textPreview}> | |
| <div className="text-sm text-text truncate"> | |
| {clip.textPreview || clip.utterance} | |
| </div> | |
| </Tooltip> | |
| <Tooltip | |
| title={ | |
| clip.segments.length > 1 | |
| ? t( | |
| "playground:ttsClips.downloadAll", | |
| "Download all segments" | |
| ) | |
| : t("playground:ttsClips.download", "Download") | |
| } | |
| > | |
| <button | |
| type="button" | |
| onClick={() => handleDownload(clip)} | |
| className="rounded-md border border-border bg-surface px-2 py-1 text-text-muted hover:bg-surface2 hover:text-text" | |
| <div className="flex flex-wrap items-center gap-2 text-[11px] text-text-muted"> | |
| <span>{formatTimestamp(clip.createdAt)}</span> | |
| <span>{t("playground:ttsClips.segmentsLabel", { count: clip.segments.length, defaultValue: "{{count}} segments" })}</span> | |
| {clip.provider && ( | |
| <Tag color="blue" className="!text-[10px]"> | |
| {clip.provider} | |
| </Tag> | |
| )} | |
| {clip.voice && ( | |
| <Tag color="purple" className="!text-[10px]"> | |
| {clip.voice} | |
| </Tag> | |
| )} | |
| </div> | |
| <div className="flex items-center gap-2"> | |
| <Tooltip | |
| title={ | |
| playingClipId === clip.id | |
| ? t("playground:tts.stop", "Stop") | |
| : t("playground:tts.play", "Play") | |
| } | |
| > | |
| <Download className="h-4 w-4" /> | |
| </button> | |
| </Tooltip> | |
| <Tooltip title={t("common:delete", "Delete") as string}> | |
| <button | |
| type="button" | |
| onClick={() => handleDelete(clip)} | |
| className="rounded-md border border-border bg-surface px-2 py-1 text-text-muted hover:bg-surface2 hover:text-text" | |
| <button | |
| type="button" | |
| aria-label={playButtonLabel} | |
| onClick={() => handleTogglePlay(clip)} | |
| className="rounded-md border border-border bg-surface px-2 py-1 text-text-muted hover:bg-surface2 hover:text-text" | |
| > | |
| {playingClipId === clip.id ? ( | |
| <Square className="h-4 w-4 text-danger" /> | |
| ) : ( | |
| <Volume2 className="h-4 w-4" /> | |
| )} | |
| </button> | |
| </Tooltip> | |
| {list.map((clip) => { | |
| const clipLabel = clip.textPreview || clip.utterance || clip.id | |
| const playButtonLabel = | |
| playingClipId === clip.id | |
| ? t("playground:ttsClips.stopClipLabel", "Stop clip {{label}}", { label: clipLabel }) | |
| : t("playground:ttsClips.playClipLabel", "Play clip {{label}}", { label: clipLabel }) | |
| return ( | |
| <div | |
| key={clip.id} | |
| className="flex flex-col gap-2 rounded-xl border border-border bg-surface2/70 p-3" | |
| > | |
| <Tooltip title={clip.utterance || clip.textPreview}> | |
| <div className="text-sm text-text truncate"> | |
| {clip.textPreview || clip.utterance} | |
| </div> | |
| </Tooltip> | |
| <div className="flex flex-wrap items-center gap-2 text-[11px] text-text-muted"> | |
| <span>{formatTimestamp(clip.createdAt)}</span> | |
| <span>{t("playground:ttsClips.segmentsLabel", { count: clip.segments.length, defaultValue: "{{count}} segments" })}</span> | |
| {clip.provider && ( | |
| <Tag color="blue" className="!text-[10px]"> | |
| {clip.provider} | |
| </Tag> | |
| )} | |
| {clip.voice && ( | |
| <Tag color="purple" className="!text-[10px]"> | |
| {clip.voice} | |
| </Tag> | |
| )} | |
| </div> | |
| <div className="flex items-center gap-2"> | |
| <Tooltip | |
| title={ | |
| playingClipId === clip.id | |
| ? t("playground:tts.stop", "Stop") | |
| : t("playground:tts.play", "Play") | |
| } | |
| > | |
| <button | |
| type="button" | |
| aria-label={playButtonLabel} | |
| onClick={() => handleTogglePlay(clip)} | |
| className="rounded-md border border-border bg-surface px-2 py-1 text-text-muted hover:bg-surface2 hover:text-text" | |
| > | |
| {playingClipId === clip.id ? ( | |
| <Square className="h-4 w-4 text-danger" /> | |
| ) : ( | |
| <Volume2 className="h-4 w-4" /> | |
| )} | |
| </button> | |
| </Tooltip> |
🤖 Prompt for 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.
In `@apps/packages/ui/src/components/Sidepanel/Chat/TtsClipsDrawer.tsx` around
lines 254 - 304, The aria-label for the play/stop button in TtsClipsDrawer is
hardcoded in English via playButtonLabel, while the adjacent Tooltip already
uses localized strings. Update the button label generation in the list.map(clip)
block to use t(...) with the same translation keys/pattern as the Tooltip,
including the clip label as a variable, so screen readers get localized text.
Keep the aria-label in sync with the playingClipId state and the existing
play/stop behavior.
Comment on lines
+345
to
+416
| it("routes an inherited workspace persona default when no explicit assistant is selected", async () => { | ||
| const inheritedWorkspaceAssistant = { | ||
| kind: "persona" as const, | ||
| id: "workspace-helper", | ||
| name: "Workspace Helper", | ||
| metadata: { | ||
| selectionMode: "tracked", | ||
| source: "workspace", | ||
| personaMemoryMode: "read_write" | ||
| } | ||
| } | ||
| const options = { | ||
| ...createHookOptions(), | ||
| selectedAssistant: inheritedWorkspaceAssistant, | ||
| inheritedAssistant: inheritedWorkspaceAssistant, | ||
| inheritedPersonaMemoryMode: "read_write" as const | ||
| } | ||
| createChatMock.mockResolvedValueOnce({ | ||
| id: "workspace-persona-chat", | ||
| title: "Workspace persona chat", | ||
| assistant_kind: "persona", | ||
| assistant_id: "workspace-helper", | ||
| persona_memory_mode: "read_write" | ||
| }) | ||
| const { result } = renderHook(() => useChatActions(options as any)) | ||
|
|
||
| await act(async () => { | ||
| await result.current.onSubmit({ | ||
| message: "Use workspace persona", | ||
| image: "" | ||
| }) | ||
| }) | ||
|
|
||
| expect(createChatMock).toHaveBeenCalledWith({ | ||
| assistant_kind: "persona", | ||
| assistant_id: "workspace-helper", | ||
| persona_memory_mode: "read_write", | ||
| state: "in-progress", | ||
| topic_label: undefined, | ||
| cluster_id: undefined, | ||
| source: undefined, | ||
| external_ref: undefined | ||
| }) | ||
| expect(options.setServerChatAssistantKind).toHaveBeenCalledWith("persona") | ||
| expect(options.setServerChatAssistantId).toHaveBeenCalledWith( | ||
| "workspace-helper" | ||
| ) | ||
| expect(savePlaygroundSessionMock).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| serverChatId: "workspace-persona-chat", | ||
| trackedAssistantKind: "persona", | ||
| trackedAssistantId: "workspace-helper", | ||
| trackedAssistantDisplayName: "Workspace Helper", | ||
| serverChatPersonaMemoryMode: "read_write" | ||
| }) | ||
| ) | ||
| expect(normalChatModeMock).toHaveBeenCalledWith( | ||
| "Use workspace persona", | ||
| "", | ||
| false, | ||
| [], | ||
| [], | ||
| expect.any(AbortSignal), | ||
| expect.objectContaining({ | ||
| assistantIdentity: { | ||
| name: "Workspace Helper", | ||
| avatarUrl: undefined | ||
| }, | ||
| serverChatId: "workspace-persona-chat" | ||
| }) | ||
| ) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Test doesn't actually exercise the "no explicit assistant selected" inheritance path.
The test is titled "routes an inherited workspace persona default when no explicit assistant is selected", but it sets selectedAssistant: inheritedWorkspaceAssistant — a truthy value. In useChatActions.ts, inheritedTrackedAssistant short-circuits to null whenever selectedAssistant is truthy (apps/packages/ui/src/hooks/chat/useChatActions.ts lines 542-554: if (... || selectedAssistant || ...) { return null }). So routingSelectedAssistant here resolves via the plain selectedAssistant fallback, not via the new inheritedTrackedAssistant derivation this PR adds — the test passes regardless of whether that new "inherit into a fresh chat when nothing is explicitly selected" logic works.
To actually cover the intended scenario, selectedAssistant should be null (all of createHookOptions()'s other guard inputs — messages: [], history: [], serverChatId: null, serverChatAssistantKind: null, serverChatAssistantId: null — already satisfy the rest of the inheritedTrackedAssistant guard).
♻️ Suggested fix
const options = {
...createHookOptions(),
- selectedAssistant: inheritedWorkspaceAssistant,
+ selectedAssistant: null,
inheritedAssistant: inheritedWorkspaceAssistant,
inheritedPersonaMemoryMode: "read_write" as const
}Please confirm effectiveAssistantState.mode resolves to "plain" under this test's mocked useChatSettingsRecord/chatSettings (needed for inheritedTrackedAssistant to activate) before applying — if it doesn't, the guard may need a closer look for testability.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("routes an inherited workspace persona default when no explicit assistant is selected", async () => { | |
| const inheritedWorkspaceAssistant = { | |
| kind: "persona" as const, | |
| id: "workspace-helper", | |
| name: "Workspace Helper", | |
| metadata: { | |
| selectionMode: "tracked", | |
| source: "workspace", | |
| personaMemoryMode: "read_write" | |
| } | |
| } | |
| const options = { | |
| ...createHookOptions(), | |
| selectedAssistant: inheritedWorkspaceAssistant, | |
| inheritedAssistant: inheritedWorkspaceAssistant, | |
| inheritedPersonaMemoryMode: "read_write" as const | |
| } | |
| createChatMock.mockResolvedValueOnce({ | |
| id: "workspace-persona-chat", | |
| title: "Workspace persona chat", | |
| assistant_kind: "persona", | |
| assistant_id: "workspace-helper", | |
| persona_memory_mode: "read_write" | |
| }) | |
| const { result } = renderHook(() => useChatActions(options as any)) | |
| await act(async () => { | |
| await result.current.onSubmit({ | |
| message: "Use workspace persona", | |
| image: "" | |
| }) | |
| }) | |
| expect(createChatMock).toHaveBeenCalledWith({ | |
| assistant_kind: "persona", | |
| assistant_id: "workspace-helper", | |
| persona_memory_mode: "read_write", | |
| state: "in-progress", | |
| topic_label: undefined, | |
| cluster_id: undefined, | |
| source: undefined, | |
| external_ref: undefined | |
| }) | |
| expect(options.setServerChatAssistantKind).toHaveBeenCalledWith("persona") | |
| expect(options.setServerChatAssistantId).toHaveBeenCalledWith( | |
| "workspace-helper" | |
| ) | |
| expect(savePlaygroundSessionMock).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| serverChatId: "workspace-persona-chat", | |
| trackedAssistantKind: "persona", | |
| trackedAssistantId: "workspace-helper", | |
| trackedAssistantDisplayName: "Workspace Helper", | |
| serverChatPersonaMemoryMode: "read_write" | |
| }) | |
| ) | |
| expect(normalChatModeMock).toHaveBeenCalledWith( | |
| "Use workspace persona", | |
| "", | |
| false, | |
| [], | |
| [], | |
| expect.any(AbortSignal), | |
| expect.objectContaining({ | |
| assistantIdentity: { | |
| name: "Workspace Helper", | |
| avatarUrl: undefined | |
| }, | |
| serverChatId: "workspace-persona-chat" | |
| }) | |
| ) | |
| }) | |
| it("routes an inherited workspace persona default when no explicit assistant is selected", async () => { | |
| const inheritedWorkspaceAssistant = { | |
| kind: "persona" as const, | |
| id: "workspace-helper", | |
| name: "Workspace Helper", | |
| metadata: { | |
| selectionMode: "tracked", | |
| source: "workspace", | |
| personaMemoryMode: "read_write" | |
| } | |
| } | |
| const options = { | |
| ...createHookOptions(), | |
| selectedAssistant: null, | |
| inheritedAssistant: inheritedWorkspaceAssistant, | |
| inheritedPersonaMemoryMode: "read_write" as const | |
| } | |
| createChatMock.mockResolvedValueOnce({ | |
| id: "workspace-persona-chat", | |
| title: "Workspace persona chat", | |
| assistant_kind: "persona", | |
| assistant_id: "workspace-helper", | |
| persona_memory_mode: "read_write" | |
| }) | |
| const { result } = renderHook(() => useChatActions(options as any)) | |
| await act(async () => { | |
| await result.current.onSubmit({ | |
| message: "Use workspace persona", | |
| image: "" | |
| }) | |
| }) | |
| expect(createChatMock).toHaveBeenCalledWith({ | |
| assistant_kind: "persona", | |
| assistant_id: "workspace-helper", | |
| persona_memory_mode: "read_write", | |
| state: "in-progress", | |
| topic_label: undefined, | |
| cluster_id: undefined, | |
| source: undefined, | |
| external_ref: undefined | |
| }) | |
| expect(options.setServerChatAssistantKind).toHaveBeenCalledWith("persona") | |
| expect(options.setServerChatAssistantId).toHaveBeenCalledWith( | |
| "workspace-helper" | |
| ) | |
| expect(savePlaygroundSessionMock).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| serverChatId: "workspace-persona-chat", | |
| trackedAssistantKind: "persona", | |
| trackedAssistantId: "workspace-helper", | |
| trackedAssistantDisplayName: "Workspace Helper", | |
| serverChatPersonaMemoryMode: "read_write" | |
| }) | |
| ) | |
| expect(normalChatModeMock).toHaveBeenCalledWith( | |
| "Use workspace persona", | |
| "", | |
| false, | |
| [], | |
| [], | |
| expect.any(AbortSignal), | |
| expect.objectContaining({ | |
| assistantIdentity: { | |
| name: "Workspace Helper", | |
| avatarUrl: undefined | |
| }, | |
| serverChatId: "workspace-persona-chat" | |
| }) | |
| ) | |
| }) |
🤖 Prompt for 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.
In
`@apps/packages/ui/src/hooks/chat/__tests__/useChatActions.persona.integration.test.tsx`
around lines 345 - 416, This test is not covering the inherited assistant path
because useChatActions still sees selectedAssistant as truthy, so
inheritedTrackedAssistant never activates. Update the setup in
useChatActions.persona.integration.test.tsx so selectedAssistant is null while
keeping the other inherited inputs, and verify effectiveAssistantState.mode is
"plain" under the mocked useChatSettingsRecord/chatSettings so the
inheritedTrackedAssistant guard in useChatActions can run. Keep the existing
assertions on createChatMock, savePlaygroundSessionMock, and normalChatModeMock
to validate the fresh-chat inheritance behavior.
Comment on lines
+39
to
+52
| type PersonaMemoryMode = "read_only" | "read_write"; | ||
|
|
||
| type UseMessageOptionOptions = { | ||
| forceCompareEnabled?: boolean; | ||
| scope?: ChatScope; | ||
| inheritedAssistant?: AssistantSelection | null; | ||
| inheritedPersonaMemoryMode?: PersonaMemoryMode | null; | ||
| }; | ||
|
|
||
| const normalizePersonaMemoryMode = ( | ||
| value: unknown, | ||
| ): PersonaMemoryMode | null => { | ||
| return value === "read_only" || value === "read_write" ? value : null; | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate PersonaMemoryMode/normalizePersonaMemoryMode definitions across files.
Graph evidence shows PersonaMemoryMode/normalizePersonaMemoryMode also defined/used in useChatActions.ts, personaServerChat.ts, and locally in WorkspaceChatPanel.tsx. Redefining the same small pure type/helper in multiple places in this cohort risks drift (e.g. subtly different normalization rules creeping in over time).
Consider extracting PersonaMemoryMode and normalizePersonaMemoryMode into a shared module (e.g. alongside WorkspacePersonaMemoryMode in types/workspace.ts, or a small persona-memory-mode.ts util) and importing it from there in all four locations.
🤖 Prompt for 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.
In `@apps/packages/ui/src/hooks/useMessageOption.tsx` around lines 39 - 52,
Extract the duplicated PersonaMemoryMode type and normalizePersonaMemoryMode
helper into a shared module and import them everywhere they are used. Update
useMessageOption, useChatActions, personaServerChat, and WorkspaceChatPanel to
reference the shared definitions instead of local copies so the normalization
logic stays consistent and cannot drift.
Comment on lines
338
to
346
| ws.onopen = () => { | ||
| settled = true | ||
| clearConnectTimer() | ||
| if (streamConnectRejectRef.current === reject) { | ||
| streamConnectRejectRef.current = null | ||
| } | ||
| streamConnectPromiseRef.current = null | ||
| setStreamState("open") | ||
| resolve(ws) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard late onopen after a failed handshake.
failConnect() marks the attempt settled, but a delayed onopen can still run and set streamState to "open" for a socket that timed out/closed. Add the same settled/mounted guard used by the failure path.
Suggested fix
ws.onopen = () => {
+ if (settled || !mountedRef.current) return
settled = true
clearConnectTimer()
if (streamConnectRejectRef.current === reject) {
streamConnectRejectRef.current = null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ws.onopen = () => { | |
| settled = true | |
| clearConnectTimer() | |
| if (streamConnectRejectRef.current === reject) { | |
| streamConnectRejectRef.current = null | |
| } | |
| streamConnectPromiseRef.current = null | |
| setStreamState("open") | |
| resolve(ws) | |
| ws.onopen = () => { | |
| if (settled || !mountedRef.current) return | |
| settled = true | |
| clearConnectTimer() | |
| if (streamConnectRejectRef.current === reject) { | |
| streamConnectRejectRef.current = null | |
| } | |
| streamConnectPromiseRef.current = null | |
| setStreamState("open") | |
| resolve(ws) |
🤖 Prompt for 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.
In `@apps/packages/ui/src/hooks/usePersonaLiveControl.tsx` around lines 338 - 346,
The ws.onopen handler in usePersonaLiveControl should ignore late events after
the connection attempt has already failed or the component is no longer mounted.
Add the same settled/mounted guard used by the failure path before calling
setStreamState("open") and resolve(ws), and make sure it checks the current
connection attempt state so a timed-out or closed socket cannot reopen the
stream state.
Comment on lines
+2
to
+7
| id: TASK-12115 | ||
| title: Rebase PR 2316 on latest dev and fix CI failures | ||
| status: Done | ||
| references: | ||
| - https://github.com/rmusser01/tldw_server/pull/2316 | ||
| --- |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Assign a unique task id here.
TASK-12115 already belongs to the focus-mode task, so reusing it here makes task tracking and backlinks ambiguous. Give this doc its own id before merge.
🤖 Prompt for 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.
In `@backlog/tasks/task-12115` -
Rebase-PR-2316-on-latest-dev-and-fix-CI-failures.md around lines 2 - 7, The task
document is reusing an existing identifier, which makes tracking and backlinks
ambiguous. Update the frontmatter in the task doc so it has its own unique id
instead of TASK-12115, and keep the rest of the metadata intact. Use the task
title and references to confirm you are editing the correct document.
Comment on lines
+1
to
+40
| ## Stage 1: Backend release and WebSearch review fixes | ||
| **Goal**: Address valid release-helper, WebSearch logging, Sync path-validation, media navigation CodeQL comment placement, and test-marker feedback. | ||
| **Success Criteria**: Release helper raises when the post-release README anchor is absent; WebSearch diagnostic logs are redacted and lazy; Sync temp path inputs are validated at the helper boundary; CodeQL comments sit adjacent to alerting regex calls; touched backend tests have pytest markers. | ||
| **Tests**: Targeted pytest for release docs, WebSearch sanitizer/logging, Sync blob store boundary checks, and media navigation where practical. | ||
| **Status**: Complete | ||
|
|
||
| ## Stage 2: MCP docs corpus review fixes | ||
| **Goal**: Resolve docs package correctness/configuration comments with local, minimal changes. | ||
| **Success Criteria**: Text extraction has no duplicate branch; NormalizedURL default construction stores a string canonical URL; locked_down policy rejects ignored allow settings; IP-literal URL prefixes fail fast; HTML inline whitespace is preserved; non-UTF-8 local imports raise DocsError; SQLite connections use timeout/WAL and PRAGMA identifier allowlists; keyword orphan pruning is moved out of per-document writes; public host adapter methods have docstrings; default docs module respects robots. | ||
| **Tests**: Targeted MCP docs pytest modules for acquisition, importers, schema store, standalone mount, and host adapter. | ||
| **Status**: Complete | ||
|
|
||
| ## Stage 3: Frontend correctness and test hygiene | ||
| **Goal**: Fix frontend review findings that affect runtime correctness or test reliability. | ||
| **Success Criteria**: Audio recorder clears recorder ref on synchronous start failure; greeting selection cannot double-post while persistence is in flight; character stream watchdog aborts only its owned controller; persona connect timeout is cleared on unmount; route character intent signatures are scoped to resolved intent fields; brittle/leaky tests are hardened; standalone UX gate stages Next static assets. | ||
| **Tests**: Targeted Vitest suites for useAudioRecorder, ChatGreetingPicker, character chat watchdog, usePersonaLiveControl, Playground route intent, TTS drawer test, WebLayout contract, and plasmo storage watch. | ||
| **Status**: Complete | ||
|
|
||
| ## Stage 4: CodeQL review annotations and PR metadata | ||
| **Goal**: Address CodeQL PR review comments without changing test semantics or leaking secrets. | ||
| **Success Criteria**: Test-only URL substring and credential-storage alerts are either fixed with stronger assertions/helpers or annotated directly at test-only sinks; the human-authored PR change-summary gate is documented as a requester action; remaining broad "consider" comments are triaged as intentionally out of scope when they are not tied to a current failure. | ||
| **Tests**: Relevant CI/unit tests plus static search for moved CodeQL annotations. | ||
| **Status**: Complete | ||
|
|
||
| **Notes**: The concrete CodeQL/test issues were addressed. Broad low-priority "consider extracting shared helper" suggestions were intentionally not folded into this release-merge cleanup because they would refactor unrelated UI areas without fixing a current behavioral failure. The PR's human-authored `Change summary` gate remains a requester action per `Docs/superpowers/AI_GENERATED_PR_CHANGE_SUMMARY_POLICY_2026_04_17.md`. | ||
|
|
||
| ## Stage 5: Verification and finalization | ||
| **Goal**: Verify touched scope, update Backlog.md, commit, and push to the PR branch. | ||
| **Success Criteria**: Targeted Python and frontend tests pass or any blocker is documented; Bandit runs on touched Python scope; Backlog task includes touched files, verification, and final summary; changes are committed and pushed to update PR #2571. | ||
| **Tests**: `python -m pytest ...`, `bunx vitest run ...`, Bandit for touched Python paths, plus `git status`/diff review. | ||
| **Status**: Complete | ||
|
|
||
| **Verification Results**: | ||
| - Backend focused pytest: 25 passed, 5 warnings. | ||
| - MCP docs focused pytest: 7 passed, 3 warnings. | ||
| - MCP docs schema-store pytest: 17 passed, 3 warnings. | ||
| - Frontend focused Vitest suite: 7 files passed, 90 tests passed. | ||
| - Persona live follow-up Vitest: 1 file passed, 12 tests passed. | ||
| - `git diff --check`: clean. | ||
| - Bandit touched Python scope: completed with existing low-severity baseline findings in untouched `WebSearch_APIs.py` lines only (`B311`, `B101`); no new findings in changed lines. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Normalize the markdown heading structure.
Add a top-level heading and blank lines around each section header; otherwise this file will keep failing markdownlint.
🧰 Tools
🪛 LanguageTool
[grammar] ~37-~37: Use a hyphen to join words.
Context: ...ytest: 17 passed, 3 warnings. - Frontend focused Vitest suite: 7 files passed, 90...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.22.1)
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
[warning] 7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 13-13: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 19-19: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 27-27: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for 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.
In `@IMPLEMENTATION_PLAN_pr2571_review_comments.md` around lines 1 - 40, The
markdown in IMPLEMENTATION_PLAN_pr2571_review_comments.md needs heading
normalization to satisfy markdownlint. Add a single top-level heading for the
plan document, and ensure each stage heading has the required blank lines before
and after it so the sections are parsed consistently. Keep the existing stage
content and structure intact while updating the headings in the implementation
plan blocks.
Source: Linters/SAST tools
Comment on lines
+113
to
+128
| def _redact_websearch_log_text(value: str) -> str: | ||
| parsed = urlparse(value) | ||
| if parsed.scheme in {"http", "https"} and parsed.netloc: | ||
| safe_pairs = [ | ||
| ( | ||
| key, | ||
| "[REDACTED]" | ||
| if key.strip().lower() in _WEBSEARCH_SENSITIVE_LOG_KEYS | ||
| else redacted_value, | ||
| ) | ||
| for key, raw_value in parse_qsl(parsed.query, keep_blank_values=True) | ||
| for redacted_value in (_redact_websearch_log_text(raw_value),) | ||
| ] | ||
| safe_query = urlencode(safe_pairs, doseq=True, quote_via=quote) | ||
| return urlunparse(parsed._replace(query=safe_query, fragment="")) | ||
| return _WEBSEARCH_SENSITIVE_PAIR_RE.sub(lambda match: f"{match.group(1)}=[REDACTED]", value) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Expand redaction beyond exact token/key names.
Line 120 only redacts exact key matches, so common credential parameters like access_token, refresh_token, and client_secret remain visible in debug logs.
🛡️ Suggested fix
_WEBSEARCH_SENSITIVE_LOG_KEYS = {
+ "access_token",
"api_key",
"apikey",
"authorization",
+ "client_secret",
"firecrawl_api_key",
"google_search_api_key",
+ "id_token",
"key",
+ "password",
+ "refresh_token",
+ "secret",
"serper_search_api_key",
"tavily_search_api_key",
"token",As per coding guidelines, “Never log sensitive information (API keys, passwords).”
🤖 Prompt for 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.
In `@tldw_Server_API/app/core/Web_Scraping/WebSearch_APIs.py` around lines 113 -
128, The redaction logic in _redact_websearch_log_text only masks exact query
keys, so common credential parameters can still leak in logs. Update the key
check used when building safe_pairs to match broader sensitive names such as
access_token, refresh_token, client_secret, password, and similar variants,
while keeping the recursive value redaction behavior intact. Make sure the
fallback _WEBSEARCH_SENSITIVE_PAIR_RE path still catches sensitive key/value
pairs in non-URL text.
Source: Coding guidelines
Comment on lines
+58
to
+60
| def debug(self, message, *args, **_kwargs): | ||
| rendered_args = [arg() if self._lazy and callable(arg) else arg for arg in args] | ||
| self.debug_messages.append(str(message).format(*rendered_args)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from loguru import logger
logger.remove()
logger.add(lambda _: None, level="DEBUG")
try:
logger.opt(lazy=True).debug("{}: {}", "label", lambda: "payload")
except TypeError as exc:
print(type(exc).__name__)
PYRepository: rmusser01/tldw_server
Length of output: 2061
🌐 Web query:
Loguru lazy=True semantics callable arguments TypeError plain value debug documentation
💡 Result:
In Loguru, when using opt(lazy=True), all keyword arguments passed to the logging method must be callables (typically functions or lambdas) that take no arguments [1][2]. Loguru expects these callables to be invoked only if the log message is actually processed by a sink (i.e., if the logging level is high enough) [3][4]. If you pass a plain value (e.g., an integer, string, or object) instead of a callable when lazy=True is enabled, Loguru will raise a TypeError [1][2]. This is a deliberate design choice to enforce that only "expensive" operations are deferred [3][4]. To fix this, you must wrap the value in a lambda or function: Incorrect: logger.opt(lazy=True).debug("Result: {x}", x=my_expensive_value) # Raises TypeError Correct: logger.opt(lazy=True).debug("Result: {x}", x=lambda: my_expensive_value) Loguru's documentation explicitly notes that opt(lazy=True) loggers accept only typing.Callable[[], typing.Any] arguments [1][2]. Using this pattern prevents the unnecessary execution of functions or evaluation of expressions when the log level would otherwise suppress the output [5][4].
Citations:
- 1: https://loguru.readthedocs.io/en/latest/api/type%5Fhints.html
- 2: https://loguru.readthedocs.io/en/stable/api/type_hints.html
- 3: https://loguru.readthedocs.io/en/stable/_modules/loguru/_logger.html
- 4: https://loguru.readthedocs.io/en/latest/overview.html
- 5: [Question] Why do string formatting happen for messages that will not be logged? Delgan/loguru#568
Enforce Loguru’s lazy=True contract in the fake logger. debug() should reject non-callable args when _lazy is set; otherwise this test can pass even though real Loguru raises on plain values.
🤖 Prompt for 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.
In `@tldw_Server_API/tests/Web_Scraping/test_phase3_3_sanitizers.py` around lines
58 - 60, The fake logger’s debug method is too permissive when _lazy is enabled,
so it can hide behavior that real Loguru would reject. Update the debug
implementation in the test helper to enforce Loguru’s lazy=True contract by
validating that every arg is callable whenever self._lazy is true and raising
the same kind of failure for plain values; keep the existing message formatting
path in sync with the debug() helper used by the sanitizer tests.
Comment on lines
+70
to
+104
| def _create_workspace_test_persona( | ||
| db: CharactersRAGDB, | ||
| *, | ||
| persona_id: str = "persona-1", | ||
| user_id: str = "1", | ||
| name: str = "Workspace Persona", | ||
| ) -> str: | ||
| """Create a test Persona profile for workspace endpoint tests. | ||
|
|
||
| Args: | ||
| db: CharactersRAGDB test database that already contains "Test Char". | ||
| persona_id: Stable Persona id to write into the profile row. | ||
| user_id: Owner user id for permission-scoped Persona lookups. | ||
| name: Display name to store on the Persona profile. | ||
|
|
||
| Returns: | ||
| The Persona id returned by ``db.create_persona_profile``. | ||
|
|
||
| Side Effects: | ||
| Asserts that the "Test Char" character card exists, then creates a | ||
| session-scoped Persona profile linked to that character. | ||
| """ | ||
| character = db.get_character_card_by_name("Test Char") | ||
| assert character is not None # nosec B101 | ||
| return db.create_persona_profile( | ||
| { | ||
| "id": persona_id, | ||
| "user_id": user_id, | ||
| "name": name, | ||
| "character_card_id": int(character["id"]), | ||
| "mode": "session_scoped", | ||
| "system_prompt": "You support workspace tests.", | ||
| "is_active": True, | ||
| } | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated persona-creation helper across two test files.
_create_workspace_test_persona here is nearly identical to _create_persona in tldw_Server_API/tests/Workspaces/test_workspace_assistant_defaults_api.py (same create_persona_profile payload shape, same defaults pattern). Consider extracting a shared helper/fixture into a conftest.py under tests/Workspaces/ to avoid the two copies drifting apart as the Persona payload contract evolves.
🤖 Prompt for 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.
In `@tldw_Server_API/tests/Workspaces/test_workspaces_api.py` around lines 70 -
104, The workspace test suite has duplicated Persona setup logic in
_create_workspace_test_persona and the matching helper in
test_workspace_assistant_defaults_api, so extract the shared
create_persona_profile setup into a common helper or fixture under
tests/Workspaces/conftest.py. Keep the existing defaults and payload shape in
the shared helper, then update both test files to call it so Persona creation
stays consistent as the contract changes.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
apps/mcp-unified/src/mcp_unified/docs/acquisition/policy.py (1)
304-316: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject legacy IPv4 hosts in URL prefixes too.
ipaddress.ip_address()only catches canonical IP literals here, butevaluate()also denies legacy IPv4 forms via_is_source_host_denied(). That leaves prefixes like0177.0.0.1or0x7f.1accepted in config and never usable at runtime. Reuse the same host-denial logic when parsing prefixes so permanently denied hosts are rejected up front.🤖 Prompt for 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. In `@apps/mcp-unified/src/mcp_unified/docs/acquisition/policy.py` around lines 304 - 316, The prefix parser in _parse_prefix currently rejects only canonical IP literals, but it should also reject legacy IPv4-style hosts that evaluate() already denies. Update _parse_prefix to apply the same host-denial check used by _is_source_host_denied() after normalize_url() so prefixes like 0177.0.0.1 or 0x7f.1 fail validation up front. Keep the existing dot-segment and IP-literal checks, and ensure the new rejection is surfaced as a ValueError tied to the same field_name context.apps/packages/ui/src/services/tldw/TldwApiClient.ts (1)
2611-2618: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the stale regression expectations
apps/packages/ui/src/services/__tests__/tldw-api-client.chat-sanitization-regression.test.tsstill expectsTldwApiClient.createChatCompletion()to rewrite successful payloads to{ error: "Chat completion failed." }. That no longer matches the verbatim return path, so these assertions need to be updated or removed.🤖 Prompt for 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. In `@apps/packages/ui/src/services/tldw/TldwApiClient.ts` around lines 2611 - 2618, The chat sanitization regression test is asserting the old behavior where TldwApiClient.createChatCompletion rewrites successful responses into an error object, but the implementation now returns the parsed payload verbatim via createJsonResponseLike. Update apps/packages/ui/src/services/__tests__/tldw-api-client.chat-sanitization-regression.test.ts to match the new success path by removing or changing the stale expectations for { error: "Chat completion failed." } and, if needed, assert the returned response body is preserved unchanged.apps/packages/ui/src/components/Option/Playground/__tests__/Playground.cockpit-shell.test.tsx (1)
656-711: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
waitForover a rawsetTimeout(0)for async assertions.A single macrotask tick doesn't retry/poll like
waitFor, which the rest of this test suite (and sibling files) uses for similar async hydration assertions. This is more flake-prone if the component's async chain takes more than one tick to settle.🤖 Prompt for 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. In `@apps/packages/ui/src/components/Option/Playground/__tests__/Playground.cockpit-shell.test.tsx` around lines 656 - 711, The test in Playground.cockpit-shell.test.tsx uses a raw setTimeout(0) for the rehydration assertion, which is flaky because it only waits one macrotask and does not retry. Replace that final delay/assertion with waitFor in the same test that exercises Playground, routerLocationState, and tldwClientState.getCharacter so the check polls until the async hydration settles.apps/packages/ui/src/components/Layouts/Layout.tsx (1)
677-720: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftGlobal override slot has no ownership tracking — concurrent hook consumers can clobber each other.
useOptionLayoutShellOverridesis now exported for use across the tree. It writes to a single, sharedsetOverridesslot and its cleanup unconditionally callsappliedSetOverrides?.(null). Since a child'suseEffectcleanup runs before its parent's on unmount, if a nested caller (e.g., a focus-mode component) unmounts/toggles off while an outer caller (e.g.,NestedLayoutContentrequestinghideHeader/hideSidebarfor the route) is still mounted and wants its own overrides applied, the child's cleanup will reset the shared state tonulland the parent won't reapply its overrides unless its own effect dependencies change.Consider tracking the currently "owning" request (e.g., only clear if the current global value still matches what this hook instance applied) or maintaining a small override stack so unrelated consumers don't clobber each other.
🤖 Prompt for 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. In `@apps/packages/ui/src/components/Layouts/Layout.tsx` around lines 677 - 720, useOptionLayoutShellOverrides currently writes to a shared LayoutShellContext/globalShell override slot and its cleanup unconditionally clears it, so one hook instance can clobber another. Update the effect in useOptionLayoutShellOverrides to track ownership of the applied LayoutShellOverrides (for example by comparing the currently stored overrides before clearing, or by introducing a small stack/owner token) so the cleanup only removes overrides if this instance still owns them. Make sure the fix covers both shell.setOverrides and globalShell?.setOverrides paths and preserves the requestedOverrides.sourcePath identity when validating ownership.apps/packages/ui/src/hooks/useAudioRecorder.ts (1)
125-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract shared teardown between
onerrorandonstop.Both handlers duplicate
stopTimer()/stopMediaTracks()/setStatus("idle")/recorderRef.current = null. A smallfinishRecording()helper would reduce duplication (the recorded-blob-specific logic inonstopcould remain separate).🤖 Prompt for 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. In `@apps/packages/ui/src/hooks/useAudioRecorder.ts` around lines 125 - 146, Both recorder handlers duplicate the same teardown steps in useAudioRecorder’s MediaRecorder callbacks. Extract the shared cleanup in recorder.onerror and recorder.onstop into a small finishRecording helper that performs stopTimer(), stopMediaTracks(), setStatus("idle"), and recorderRef.current = null, while keeping the recorded blob creation and setBlob/setDurationMs logic only in onstop.apps/packages/ui/src/components/Sidepanel/Chat/TtsClipsDrawer.tsx (1)
128-166: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard the stale cleanup in
handleTogglePlay. A superseded invocation can reachfinallyafter a newer clip has already setplayingClipId, so the unconditionalsetPlayingClipId(null)clears the active indicator while audio keeps playing. Add an ownership check (abortRef.current === controller) or a generation token before clearing state.🤖 Prompt for 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. In `@apps/packages/ui/src/components/Sidepanel/Chat/TtsClipsDrawer.tsx` around lines 128 - 166, The stale cleanup in handleTogglePlay can clear the active play state after a newer clip has already started, because the finally block always calls setPlayingClipId(null). Update the ownership handling in TtsClipsDrawer’s handleTogglePlay by checking abortRef.current against the local controller, or by using a generation/token guard, so only the currently active invocation can clear playingClipId.apps/packages/ui/src/hooks/chat/useChatActions.ts (1)
1807-1824: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPass the submit-time AbortController into this watchdog.
turnControlleris re-derived fromactiveAbortControllerRefafter several awaits, so a newer turn can replace the ref before the timer is armed. In that case the timeout won't abort the stalled stream, and the staleinactivityAbortedflag can still surfaceStreamInactivityTimeouton a response that later completes. Thread the controller captured inonSubmitthrough here instead of reading the shared ref.🤖 Prompt for 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. In `@apps/packages/ui/src/hooks/chat/useChatActions.ts` around lines 1807 - 1824, The stream inactivity watchdog in useChatActions is pulling the abort controller from activeAbortControllerRef too late, which can let a newer turn replace it before the timer starts. Thread the submit-time AbortController captured in onSubmit through the streaming/watchdog path and use that controller directly in resetInactivityTimer instead of re-deriving turnController from the shared ref, so the timeout always aborts the correct stalled stream and does not set a stale inactivityAborted flag.apps/packages/ui/src/hooks/useMessageOption.tsx (1)
273-343: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear the inherited assistant snapshot when the draft assistant context disappears.
OptionLayoutstays mounted across options-route navigation, souseMessageOption()state persists. Because the effect only clearsinheritedAssistantSnapshotwhenselectedAssistantis set, a captured persona can survive afteropts.inheritedAssistantis no longer a persona and leak into a later chat, which also mislabelsselectedAssistantSourceas"workspace".🤖 Prompt for 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. In `@apps/packages/ui/src/hooks/useMessageOption.tsx` around lines 273 - 343, The inherited assistant snapshot is only being cleared when selectedAssistant is present, so useMessageOption can keep stale persona state after the draft assistant context disappears. Update the React effect that manages inheritedAssistantSnapshot to also reset it whenever opts.inheritedAssistant is no longer a valid persona (or inheritedAssistantCandidate becomes null), not just when selectedAssistant changes. Use the existing symbols useMessageOption, inheritedAssistantSnapshot, inheritedAssistantCandidate, and assistantSelectionsMatch to keep the snapshot aligned with the current draft assistant context and prevent stale workspace attribution.
🤖 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 `@apps/packages/ui/package.json`:
- Line 39: The Vitest dependencies are on mismatched releases, with vitest not
aligned to `@vitest/coverage-v8`. Update the package.json dependency versions
together so vitest and the `@vitest/`* package use the same 4.0.18 release,
keeping the versions consistent in the UI package.
In `@apps/packages/ui/src/components/Option/ChatWorkspace/WorkspaceChatPanel.tsx`:
- Around line 160-190: The workspace assistant provenance logic in
WorkspaceChatPanel is incorrectly falling back to "explicit" when the inherited
workspace default persona changes and the selected snapshot still refers to the
prior persona. Update the assistantSource/usingWorkspaceAssistant decision so
the preserved workspace source from useMessageOption remains authoritative
across default changes, instead of relying only on the id match against
inheritedAssistant. Also remove or rewrite the unreachable
selectedAssistantSource === "workspace" && !selectedAssistant branch, and make
sure runtimeSelectedPersonaLabel still resolves from the preserved workspace
persona when assistantSource stays "workspace".
In
`@apps/packages/ui/src/components/Option/ResearchWorkspace/WorkspaceHeader.tsx`:
- Around line 810-865: Both handleSaveDefaultAssistant and
handleClearDefaultAssistant are swallowing the thrown request error and always
showing fallback copy. Update the catch blocks to capture the exception from
tldwClient.patchWorkspace(), read its message when available, and surface that
backend detail in the default assistant error state instead of replacing every
failure with the same generic text.
In
`@apps/packages/ui/src/hooks/chat/__tests__/useChatActions.persona.integration.test.tsx`:
- Around line 345-361: The test setup in
useChatActions.persona.integration.test.tsx does not cover the claimed “no
explicit assistant selected” fallback because selectedAssistant is still set to
the inherited workspace assistant, which bypasses the inherited-tracked path in
useChatActions.ts. Update this case so selectedAssistant is null while keeping
inheritedAssistant and inheritedPersonaMemoryMode set, allowing
routingSelectedAssistant to exercise the fallback logic for a fresh chat with no
draft selection.
In `@apps/packages/ui/src/hooks/chat/useChatActions.ts`:
- Around line 171-179: Duplicate PersonaMemoryMode and
normalizePersonaMemoryMode are defined here and in personaServerChat.ts, so
consolidate them into the shared helper. Export the type and
normalizePersonaMemoryMode from personaServerChat.ts, then import and reuse them
in useChatActions.ts instead of keeping a local copy. Keep the existing call
sites intact and remove the duplicate definitions to prevent future drift.
In `@backlog/tasks/task-12115` -
Rebase-PR-2316-on-latest-dev-and-fix-CI-failures.md:
- Line 4: The task metadata is inconsistent because the final summary says the
work is complete but the DoD checklist remains unchecked. Update the checklist
in the task summary to match the recorded verification by checking off the
completed items in the DoD section, and make sure the entries under the task
content (including the checklist items around the referenced range) reflect the
completed status.
In `@backlog/tasks/task-12116` - Address-PR-2571-review-comments.md:
- Line 4: The task is marked Done while the final summary still contains an
unresolved blocker note about the PR Change summary being a requester-owned
merge gate. Update the task state to match reality by either changing the status
back to an active state if the blocker remains, or removing the blocker note
from the summary if it is no longer needed. Make sure the status field and the
final summary text are consistent.
In `@backlog/tasks/task-12117` - Fix-PR-2571-release-CI-failures.md:
- Line 4: The task metadata is inconsistent with the final summary: the status
still says In Progress and the DoD checklist is unchecked even though the
summary indicates completion. Update the task header status and the checklist
items in the markdown so they match the recorded completed state, using the
task’s status field and the DoD checkboxes in the document.
In `@tldw_Server_API/app/api/v1/endpoints/workspaces.py`:
- Around line 206-276: The degraded persona branches in
_effective_workspace_assistant_default currently omit label, so the UI only sees
an opaque assistant_id when a persona is unavailable or deleted. Update the
persona_unavailable and persona_deleted paths to populate label from the
resolved profile data, using profile.get("name") or deleted_profile.get("name")
with a fallback to stored.assistant_id, while preserving the existing
degraded_reason and other fields in WorkspaceEffectiveAssistantDefault.
In `@tldw_Server_API/app/core/Web_Scraping/WebSearch_APIs.py`:
- Around line 113-128: The _redact_websearch_log_text helper still leaves URL
userinfo exposed when parsing http/https URLs, because the urlparse branch only
redacts query parameters and returns before the regex fallback. Update
_redact_websearch_log_text so it also strips parsed.username/parsed.password
from the netloc before urlunparse, or otherwise ensures user:pass@host is
replaced with a safe host-only form while keeping the existing query redaction
behavior.
---
Outside diff comments:
In `@apps/mcp-unified/src/mcp_unified/docs/acquisition/policy.py`:
- Around line 304-316: The prefix parser in _parse_prefix currently rejects only
canonical IP literals, but it should also reject legacy IPv4-style hosts that
evaluate() already denies. Update _parse_prefix to apply the same host-denial
check used by _is_source_host_denied() after normalize_url() so prefixes like
0177.0.0.1 or 0x7f.1 fail validation up front. Keep the existing dot-segment and
IP-literal checks, and ensure the new rejection is surfaced as a ValueError tied
to the same field_name context.
In `@apps/packages/ui/src/components/Layouts/Layout.tsx`:
- Around line 677-720: useOptionLayoutShellOverrides currently writes to a
shared LayoutShellContext/globalShell override slot and its cleanup
unconditionally clears it, so one hook instance can clobber another. Update the
effect in useOptionLayoutShellOverrides to track ownership of the applied
LayoutShellOverrides (for example by comparing the currently stored overrides
before clearing, or by introducing a small stack/owner token) so the cleanup
only removes overrides if this instance still owns them. Make sure the fix
covers both shell.setOverrides and globalShell?.setOverrides paths and preserves
the requestedOverrides.sourcePath identity when validating ownership.
In
`@apps/packages/ui/src/components/Option/Playground/__tests__/Playground.cockpit-shell.test.tsx`:
- Around line 656-711: The test in Playground.cockpit-shell.test.tsx uses a raw
setTimeout(0) for the rehydration assertion, which is flaky because it only
waits one macrotask and does not retry. Replace that final delay/assertion with
waitFor in the same test that exercises Playground, routerLocationState, and
tldwClientState.getCharacter so the check polls until the async hydration
settles.
In `@apps/packages/ui/src/components/Sidepanel/Chat/TtsClipsDrawer.tsx`:
- Around line 128-166: The stale cleanup in handleTogglePlay can clear the
active play state after a newer clip has already started, because the finally
block always calls setPlayingClipId(null). Update the ownership handling in
TtsClipsDrawer’s handleTogglePlay by checking abortRef.current against the local
controller, or by using a generation/token guard, so only the currently active
invocation can clear playingClipId.
In `@apps/packages/ui/src/hooks/chat/useChatActions.ts`:
- Around line 1807-1824: The stream inactivity watchdog in useChatActions is
pulling the abort controller from activeAbortControllerRef too late, which can
let a newer turn replace it before the timer starts. Thread the submit-time
AbortController captured in onSubmit through the streaming/watchdog path and use
that controller directly in resetInactivityTimer instead of re-deriving
turnController from the shared ref, so the timeout always aborts the correct
stalled stream and does not set a stale inactivityAborted flag.
In `@apps/packages/ui/src/hooks/useAudioRecorder.ts`:
- Around line 125-146: Both recorder handlers duplicate the same teardown steps
in useAudioRecorder’s MediaRecorder callbacks. Extract the shared cleanup in
recorder.onerror and recorder.onstop into a small finishRecording helper that
performs stopTimer(), stopMediaTracks(), setStatus("idle"), and
recorderRef.current = null, while keeping the recorded blob creation and
setBlob/setDurationMs logic only in onstop.
In `@apps/packages/ui/src/hooks/useMessageOption.tsx`:
- Around line 273-343: The inherited assistant snapshot is only being cleared
when selectedAssistant is present, so useMessageOption can keep stale persona
state after the draft assistant context disappears. Update the React effect that
manages inheritedAssistantSnapshot to also reset it whenever
opts.inheritedAssistant is no longer a valid persona (or
inheritedAssistantCandidate becomes null), not just when selectedAssistant
changes. Use the existing symbols useMessageOption, inheritedAssistantSnapshot,
inheritedAssistantCandidate, and assistantSelectionsMatch to keep the snapshot
aligned with the current draft assistant context and prevent stale workspace
attribution.
In `@apps/packages/ui/src/services/tldw/TldwApiClient.ts`:
- Around line 2611-2618: The chat sanitization regression test is asserting the
old behavior where TldwApiClient.createChatCompletion rewrites successful
responses into an error object, but the implementation now returns the parsed
payload verbatim via createJsonResponseLike. Update
apps/packages/ui/src/services/__tests__/tldw-api-client.chat-sanitization-regression.test.ts
to match the new success path by removing or changing the stale expectations for
{ error: "Chat completion failed." } and, if needed, assert the returned
response body is preserved unchanged.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54c8ae9a-199c-4b50-b41e-4eb204e71fb5
⛔ Files ignored due to path filters (4)
Docs/Product/Workspace_Persona_Defaults_PRD.mdis excluded by!docs/**Docs/superpowers/plans/2026-07-02-testing-audit-remediation.mdis excluded by!docs/**Docs/superpowers/plans/2026-07-03-chat-focus-fullscreen.mdis excluded by!docs/**apps/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (122)
.github/workflows/ci.yml.github/workflows/coverage-required.yml.github/workflows/frontend-required.yml.github/workflows/frontend-ux-gates.yml.github/workflows/perf-nightly.ymlHelper_Scripts/ci/run_local_ci.pyHelper_Scripts/release.pyIMPLEMENTATION_PLAN_pr2571_review_comments.mdapps/mcp-unified/src/mcp_unified/docs/acquisition/extract.pyapps/mcp-unified/src/mcp_unified/docs/acquisition/models.pyapps/mcp-unified/src/mcp_unified/docs/acquisition/policy.pyapps/mcp-unified/src/mcp_unified/docs/importers/html.pyapps/mcp-unified/src/mcp_unified/docs/importers/local.pyapps/mcp-unified/src/mcp_unified/docs/models.pyapps/mcp-unified/src/mcp_unified/docs/standalone.pyapps/mcp-unified/src/mcp_unified/docs/store/sqlite.pyapps/packages/ui/package.jsonapps/packages/ui/src/components/Common/ChatGreetingPicker.tsxapps/packages/ui/src/components/Common/__tests__/ChatGreetingPicker.test.tsxapps/packages/ui/src/components/Layouts/Layout.tsxapps/packages/ui/src/components/Layouts/__tests__/Layout.shell-overrides.test.tsxapps/packages/ui/src/components/Option/ChatWorkspace/ChatWorkspaceConsole.tsxapps/packages/ui/src/components/Option/ChatWorkspace/ChatWorkspacePage.tsxapps/packages/ui/src/components/Option/ChatWorkspace/InspectorRail.tsxapps/packages/ui/src/components/Option/ChatWorkspace/WorkspaceChatPanel.tsxapps/packages/ui/src/components/Option/ChatWorkspace/__tests__/ChatWorkspacePage.test.tsxapps/packages/ui/src/components/Option/ChatWorkspace/__tests__/InspectorRail.test.tsxapps/packages/ui/src/components/Option/ChatWorkspace/__tests__/WorkspaceChatPanel.test.tsxapps/packages/ui/src/components/Option/ChatWorkspace/types.tsapps/packages/ui/src/components/Option/KnowledgeQA/KnowledgeQAProvider.tsxapps/packages/ui/src/components/Option/KnowledgeQA/__tests__/KnowledgeQAProvider.persistence.test.tsxapps/packages/ui/src/components/Option/Playground/Playground.tsxapps/packages/ui/src/components/Option/Playground/__tests__/Playground.cockpit-controls.test.tsxapps/packages/ui/src/components/Option/Playground/__tests__/Playground.cockpit-shell.test.tsxapps/packages/ui/src/components/Option/Playground/__tests__/Playground.responsive-parity.guard.test.tsapps/packages/ui/src/components/Option/ResearchWorkspace/WorkspaceHeader.tsxapps/packages/ui/src/components/Option/ResearchWorkspace/__tests__/WorkspaceHeader.test.tsxapps/packages/ui/src/components/Sidepanel/Chat/TtsClipsDrawer.tsxapps/packages/ui/src/components/Sidepanel/Chat/__tests__/TtsClipsDrawer.overlap.test.tsxapps/packages/ui/src/context/index.tsxapps/packages/ui/src/hooks/__tests__/useAudioRecorder.test.tsapps/packages/ui/src/hooks/__tests__/useMessageOption.assistant-overlay.test.tsxapps/packages/ui/src/hooks/__tests__/usePersonaLiveControl.test.tsxapps/packages/ui/src/hooks/chat/__tests__/personaServerChat.test.tsapps/packages/ui/src/hooks/chat/__tests__/useChatActions.persona.integration.test.tsxapps/packages/ui/src/hooks/chat/personaServerChat.tsapps/packages/ui/src/hooks/chat/useChatActions.tsapps/packages/ui/src/hooks/useAudioRecorder.tsapps/packages/ui/src/hooks/useMessage.tsxapps/packages/ui/src/hooks/useMessageOption.tsxapps/packages/ui/src/hooks/usePersonaLiveControl.tsxapps/packages/ui/src/services/__tests__/tldw-api-client.connection-sync.test.tsapps/packages/ui/src/services/__tests__/tldw-api-client.workspace-api.test.tsapps/packages/ui/src/services/persona-stream.tsapps/packages/ui/src/services/tldw/TldwApiClient.tsapps/packages/ui/src/services/tldw/TldwModels.tsapps/packages/ui/src/services/tldw/domains/chat-rag.tsapps/packages/ui/src/services/tldw/domains/workspace-api.tsapps/packages/ui/src/store/__tests__/workspace.test.tsapps/packages/ui/src/store/workspace-slices/workspace-list-slice.tsapps/packages/ui/src/store/workspace.tsapps/packages/ui/src/types/workspace-assistant-defaults.tsapps/packages/ui/src/types/workspace.tsapps/packages/ui/vitest.config.tsapps/tldw-frontend/__tests__/components/layout/WebLayout.chat-scroll-contract.test.tsxapps/tldw-frontend/__tests__/extension/plasmo-storage-watch.test.tsxapps/tldw-frontend/__tests__/extension/runtime-bootstrap.test.tsapps/tldw-frontend/e2e/smoke/chat-openui-dynamic-ui.spec.tsapps/tldw-frontend/e2e/smoke/chat-sticky-composer.spec.tsapps/tldw-frontend/e2e/smoke/smoke.setup.tsapps/tldw-frontend/e2e/ux-audit/knowledge-readiness-recovery.spec.tsapps/tldw-frontend/e2e/workflows/chat-cockpit.real-server.spec.tsapps/tldw-frontend/extension/shims/runtime-bootstrap.tsapps/tldw-frontend/scripts/chat-uat-driver.mjsapps/tldw-frontend/vitest.config.tsaudits/2026-07-02-quarantined-suites.mdaudits/2026-07-02-testing-implementation-audit.mdbacklog/tasks/task-12115 - Make-chat-focus-mode-truly-fullscreen.mdbacklog/tasks/task-12115 - Rebase-PR-2316-on-latest-dev-and-fix-CI-failures.mdbacklog/tasks/task-12116 - Address-PR-2571-review-comments.mdbacklog/tasks/task-12117 - Fix-PR-2571-release-CI-failures.mdbacklog/tasks/task-2278 - Implement-Workspace-Assistant-Defaults-backend-storage.mdbacklog/tasks/task-2318 - Plan-Workspace-Assistant-Defaults-implementation.mdbacklog/tasks/task-2318.1 - Map-Workspace-Assistant-Defaults-in-WebUI-client-and-store.mdbacklog/tasks/task-2318.2 - Expose-Workspace-Assistant-Defaults-API-validation-and-effective-state.mdbacklog/tasks/task-2318.3 - Add-Workspace-default-assistant-settings-UI.mdbacklog/tasks/task-2318.4 - Apply-Workspace-Persona-defaults-to-Chat-Workspace-startup.mdbacklog/tasks/task-2318.5 - Close-out-Workspace-Assistant-Defaults-V1-verification.mdbacklog/tasks/task-2318.6 - Address-Workspace-Assistant-Defaults-PR-review-feedback.mdpyproject.tomltldw_Server_API/Config_Files/mcp_modules.yamltldw_Server_API/app/api/v1/endpoints/media/navigation.pytldw_Server_API/app/api/v1/endpoints/workspaces.pytldw_Server_API/app/api/v1/schemas/workspace_schemas.pytldw_Server_API/app/core/MCP_unified/modules/implementations/docs_module.pytldw_Server_API/app/core/MCP_unified/tests/test_web_research_module.pytldw_Server_API/app/core/Sync/v2/blob_store.pytldw_Server_API/app/core/Web_Scraping/WebSearch_APIs.pytldw_Server_API/tests/CI/test_required_workflow_contracts.pytldw_Server_API/tests/CI/test_skip_markers_have_reasons.pytldw_Server_API/tests/Character_Chat_NEW/conftest.pytldw_Server_API/tests/Docs/test_release_docs_contract.pytldw_Server_API/tests/Embeddings/conftest.pytldw_Server_API/tests/Evaluations/test_error_scenarios.pytldw_Server_API/tests/Evaluations/unit/test_circuit_breaker.pytldw_Server_API/tests/Jobs/test_jobs_events_outbox_postgres.pytldw_Server_API/tests/MCP_unified/docs/test_docs_acquisition_extract.pytldw_Server_API/tests/MCP_unified/docs/test_docs_acquisition_policy.pytldw_Server_API/tests/MCP_unified/docs/test_docs_importers.pytldw_Server_API/tests/RateLimiting/__init__.pytldw_Server_API/tests/RateLimiting/test_character_rate_limiter_429.pytldw_Server_API/tests/Storage/test_storage_user_routes.pytldw_Server_API/tests/Sync/test_sync_v2_security.pytldw_Server_API/tests/TTS_NEW/conftest.pytldw_Server_API/tests/Utils/test_image_validation.pytldw_Server_API/tests/Web_Scraping/test_phase3_3_sanitizers.pytldw_Server_API/tests/Workspaces/test_workspace_assistant_defaults_api.pytldw_Server_API/tests/Workspaces/test_workspaces_api.pytldw_Server_API/tests/_plugins/quarantine.pytldw_Server_API/tests/integration/test_error_paths_top_endpoints.pytldw_Server_API/tests/unit/test_chunker_fuzz.pytldw_Server_API/tests/unit/test_sanitize_filename_fuzz.py
💤 Files with no reviewable changes (5)
- apps/packages/ui/src/services/persona-stream.ts
- pyproject.toml
- tldw_Server_API/tests/Utils/test_image_validation.py
- tldw_Server_API/tests/Jobs/test_jobs_events_outbox_postgres.py
- apps/mcp-unified/src/mcp_unified/docs/acquisition/extract.py
| "@testing-library/user-event": "^14.6.1", | ||
| "@types/react": "^18.3.3", | ||
| "@types/react-dom": "^18.3.1", | ||
| "@vitest/coverage-v8": "4.0.18", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the pinned vitest version alongside the new coverage-v8 entry
rg -n '"vitest"' apps/packages/ui/package.json
rg -n '"`@vitest/coverage-v8`"' apps/packages/ui/package.jsonRepository: rmusser01/tldw_server
Length of output: 256
🌐 Web query:
Vitest @vitest/coverage-v8 version mismatch warning same version docs
💡 Result:
Vitest requires the vitest package and all @vitest/* extension packages (including @vitest/coverage-v8) to use the exact same version [1][2]. If you see a version mismatch warning, it indicates that your installed versions are not synchronized, which is an unsupported configuration that may cause bugs or runtime errors [1][3]. To resolve this: 1. Update your dependencies to ensure all Vitest-related packages are at the same version [1][4]. 2. For example, if you are using vitest@4.1.7, ensure that @vitest/coverage-v8 is also explicitly set to 4.1.7 in your package.json [5][6]. 3. After updating your package.json, run your package manager's install command (e.g., npm install, pnpm install, or yarn install) to update your lockfile and ensure the versions are correctly aligned [5][6]. Running mixed versions is known to cause issues such as "Invalid transform mode" errors or other runtime failures during test execution [4][7]. You can verify your installed versions by checking your project's package.json or by inspecting the lockfile generated by your package manager [5][6].
Citations:
- 1: fix(coverage): warn if
vitestand@vitest/*versions don't match vitest-dev/vitest#6317 - 2: fix(coverage): warn if
vitestand@vitest/*versions don't match vitest-dev/vitest#6317 - 3: vitest-dev/vitest@e662c7b2c
- 4: Test Coverage Error vitest-dev/vitest#4691
- 5: ci: package.json/lockfile mismatch — @vitest/coverage-v8@4.1.7 needs vitest@4.1.7 but lock has 2.1.9 mizcausevic-dev/tableau-permission-audit-lab#11
- 6: [CI Failure Doctor] Dependabot PR #11024 broke CI: npm peer dependency conflict between vitest@4.0.17 and @vitest/coverage-v8@4.0.10 github/gh-aw#11030
- 7: fix(deps): add @vitest/coverage-v8 to match vitest version promptfoo/promptfoo#6122
Align Vitest package versions
vitest is still ^4.0.16 while @vitest/coverage-v8 is 4.0.18. Keep the Vitest packages on the same release to avoid the version-mismatch warning and test issues.
🤖 Prompt for 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.
In `@apps/packages/ui/package.json` at line 39, The Vitest dependencies are on
mismatched releases, with vitest not aligned to `@vitest/coverage-v8`. Update the
package.json dependency versions together so vitest and the `@vitest/`* package
use the same 4.0.18 release, keeping the versions consistent in the UI package.
Comment on lines
+160
to
+190
| const selectedMatchesWorkspaceAssistant = | ||
| selectedAssistantSource === "workspace" && | ||
| selectedAssistant?.kind === "persona" && | ||
| inheritedAssistant?.kind === "persona" && | ||
| selectedAssistant.id === inheritedAssistant.id | ||
| const restoredWorkspaceAssistant = | ||
| !selectedAssistant && | ||
| inheritedAssistant?.kind === "persona" && | ||
| serverChatAssistantKind === "persona" && | ||
| serverChatAssistantId === inheritedAssistant.id | ||
| const usingWorkspaceAssistant = | ||
| Boolean(inheritedAssistant) && | ||
| (selectedMatchesWorkspaceAssistant || | ||
| restoredWorkspaceAssistant || | ||
| (selectedAssistantSource === "workspace" && | ||
| !selectedAssistant && | ||
| messages.length === 0)) | ||
| const assistantSource: ChatWorkspaceAssistantSource = usingWorkspaceAssistant | ||
| ? "workspace" | ||
| : selectedAssistant || serverChatAssistantKind || serverChatAssistantId | ||
| ? "explicit" | ||
| : effectiveAssistantDefault?.status === "unavailable" | ||
| ? "unavailable" | ||
| : "none" | ||
| const runtimeSelectedPersonaLabel = | ||
| assistantSource === "workspace" | ||
| ? inheritedAssistant?.name ?? selectedAssistant?.name ?? null | ||
| : selectedAssistant?.name ?? | ||
| (assistantSource === "explicit" && serverChatAssistantKind === "persona" | ||
| ? "Persona" | ||
| : null) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== WorkspaceChatPanel outline ==\n'
ast-grep outline apps/packages/ui/src/components/Option/ChatWorkspace/WorkspaceChatPanel.tsx --view expanded || true
printf '\n== useMessageOption outline ==\n'
fd -a 'useMessageOption.tsx' apps/packages/ui/src/components apps/packages/ui/src/hooks apps/packages/ui/src 2>/dev/null || true
printf '\n== WorkspaceChatPanel.test.tsx locations ==\n'
fd -a 'WorkspaceChatPanel.test.tsx' apps/packages/ui/src 2>/dev/null || trueRepository: rmusser01/tldw_server
Length of output: 1026
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant source slices once the files are located.
for f in \
apps/packages/ui/src/components/Option/ChatWorkspace/WorkspaceChatPanel.tsx \
apps/packages/ui/src/components/Option/ChatWorkspace/WorkspaceChatPanel.test.tsx \
apps/packages/ui/src/components/Option/ChatWorkspace/useMessageOption.tsx
do
if [ -f "$f" ]; then
printf '\n== %s (%s lines) ==\n' "$f" "$(wc -l < "$f")"
cat -n "$f" | sed -n '1,260p'
fi
doneRepository: rmusser01/tldw_server
Length of output: 10279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== useMessageOption.tsx relevant slice ==\n'
cat -n apps/packages/ui/src/hooks/useMessageOption.tsx | sed -n '240,360p'
printf '\n== WorkspaceChatPanel.test.tsx relevant slice ==\n'
cat -n apps/packages/ui/src/components/Option/ChatWorkspace/__tests__/WorkspaceChatPanel.test.tsx | sed -n '1,320p'
printf '\n== Search for provenance/default-change coverage ==\n'
rg -n "workspace provenance|keeps workspace provenance|default persona|assistantSource|selectedAssistantSource|selectedAssistant" apps/packages/ui/src/components/Option/ChatWorkspace/__tests__/WorkspaceChatPanel.test.tsx apps/packages/ui/src/hooks/useMessageOption.tsxRepository: rmusser01/tldw_server
Length of output: 22474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== useMessageOption.tsx selectedAssistantSource slice ==\n'
cat -n apps/packages/ui/src/hooks/useMessageOption.tsx | sed -n '360,395p'
printf '\n== WorkspaceChatPanel.test.tsx workspace provenance slice ==\n'
cat -n apps/packages/ui/src/components/Option/ChatWorkspace/__tests__/WorkspaceChatPanel.test.tsx | sed -n '520,760p'Repository: rmusser01/tldw_server
Length of output: 11421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== useMessageOption.tsx helper slice ==\n'
cat -n apps/packages/ui/src/hooks/useMessageOption.tsx | sed -n '1,120p'
printf '\n== useMessageOption.tsx effectiveSelectedAssistant slice ==\n'
cat -n apps/packages/ui/src/hooks/useMessageOption.tsx | sed -n '338,380p'
printf '\n== Search helper definitions ==\n'
rg -n "assistantSelectionsMatch|resolveEffectiveAssistantState|type EffectiveAssistantState" apps/packages/ui/src/hooks/useMessageOption.tsxRepository: rmusser01/tldw_server
Length of output: 7821
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== WorkspaceChatPanel submit overrides slice ==\n'
cat -n apps/packages/ui/src/components/Option/ChatWorkspace/WorkspaceChatPanel.tsx | sed -n '252,380p'
printf '\n== Search for assistantSource-dependent override logic ==\n'
rg -n "assistantSource|assistant_kind|assistant_id|persona_memory_mode|runtimeSelectedPersonaLabel" apps/packages/ui/src/components/Option/ChatWorkspace/WorkspaceChatPanel.tsxRepository: rmusser01/tldw_server
Length of output: 6090
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n apps/packages/ui/src/components/Option/ChatWorkspace/__tests__/WorkspaceChatPanel.test.tsx | sed -n '737,840p'Repository: rmusser01/tldw_server
Length of output: 1411
Workspace provenance can flip to explicit when the workspace default persona changes
inheritedAssistant is recomputed from the current effectiveAssistantDefault, but useMessageOption can still preserve the original workspace persona in its snapshot. If the workspace default changes from persona-A to persona-B mid-session, selectedAssistantSource can remain "workspace" while selectedAssistant still points to persona-A; the id check then fails and assistantSource falls back to "explicit", dropping the workspace override fields on submit.
The selectedAssistantSource === "workspace" && !selectedAssistant branch also looks unreachable here, since this component’s selectedAssistant is the hook’s effective selection and must be truthy whenever the source is "workspace".
🤖 Prompt for 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.
In `@apps/packages/ui/src/components/Option/ChatWorkspace/WorkspaceChatPanel.tsx`
around lines 160 - 190, The workspace assistant provenance logic in
WorkspaceChatPanel is incorrectly falling back to "explicit" when the inherited
workspace default persona changes and the selected snapshot still refers to the
prior persona. Update the assistantSource/usingWorkspaceAssistant decision so
the preserved workspace source from useMessageOption remains authoritative
across default changes, instead of relying only on the id match against
inheritedAssistant. Also remove or rewrite the unreachable
selectedAssistantSource === "workspace" && !selectedAssistant branch, and make
sure runtimeSelectedPersonaLabel still resolves from the preserved workspace
persona when assistantSource stays "workspace".
Comment on lines
+810
to
+865
| const handleSaveDefaultAssistant = async () => { | ||
| if (!defaultAssistantWorkspace) return | ||
| const loadedWorkspaceId = defaultAssistantWorkspace.id?.trim() | ||
| if (!loadedWorkspaceId) return | ||
| if (!defaultAssistantPersonaId.trim()) { | ||
| setDefaultAssistantError( | ||
| t( | ||
| "playground:workspace.defaultAssistantSelectRequired", | ||
| "Select a Persona before saving." | ||
| ) | ||
| ) | ||
| return | ||
| } | ||
| if ( | ||
| defaultAssistantMemoryMode === "read_write" && | ||
| !defaultAssistantConfirmReadWrite | ||
| ) { | ||
| setDefaultAssistantError( | ||
| t( | ||
| "playground:workspace.defaultAssistantReadWriteConfirmRequired", | ||
| "Confirm read-write memory access before saving." | ||
| ) | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| setDefaultAssistantSaving(true) | ||
| setDefaultAssistantError(null) | ||
| try { | ||
| const updatedWorkspace = await tldwClient.patchWorkspace(loadedWorkspaceId, { | ||
| version: defaultAssistantWorkspace.version, | ||
| assistantDefaults: buildDefaultAssistantPayload(), | ||
| ...(defaultAssistantMemoryMode === "read_write" | ||
| ? { confirmReadWriteAssistantDefault: true } | ||
| : {}) | ||
| }) | ||
| applyDefaultAssistantWorkspaceState(updatedWorkspace) | ||
| syncDefaultAssistantWorkspaceState(updatedWorkspace) | ||
| setDefaultAssistantModalOpen(false) | ||
| messageApi.success( | ||
| t( | ||
| "playground:workspace.defaultAssistantSaved", | ||
| "Default assistant updated" | ||
| ) | ||
| ) | ||
| } catch { | ||
| setDefaultAssistantError( | ||
| t( | ||
| "playground:workspace.defaultAssistantSaveError", | ||
| "Could not save default assistant settings." | ||
| ) | ||
| ) | ||
| } finally { | ||
| setDefaultAssistantSaving(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the component and related client methods
git ls-files 'apps/packages/ui/src/components/Option/ResearchWorkspace/WorkspaceHeader.tsx' \
'apps/packages/ui/src/**' \
'apps/packages/**' | sed -n '1,120p'
echo
echo "== WorkspaceHeader handlers =="
grep -nE 'handleSaveDefaultAssistant|handleClearDefaultAssistant|defaultAssistantError|patchWorkspace|catch' \
apps/packages/ui/src/components/Option/ResearchWorkspace/WorkspaceHeader.tsx | sed -n '1,200p'
echo
echo "== tldwClient definition/usages =="
grep -RIn --exclude-dir=.git --exclude='*.map' --exclude='*.min.*' \
-E 'patchWorkspace\(|class .*Client|interface .*Client|type .*Client|throw new Error|new Error\(' \
apps/packages | sed -n '1,240p'Repository: rmusser01/tldw_server
Length of output: 48265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WorkspaceHeader outline =="
ast-grep outline apps/packages/ui/src/components/Option/ResearchWorkspace/WorkspaceHeader.tsx --view expanded | sed -n '1,240p'
echo
echo "== WorkspaceHeader relevant lines =="
sed -n '800,900p' apps/packages/ui/src/components/Option/ResearchWorkspace/WorkspaceHeader.tsx | cat -n
echo
echo "== TldwApiClient outline =="
ast-grep outline apps/packages/ui/src/services/tldw/TldwApiClient.ts --view expanded | sed -n '1,260p'
echo
echo "== patchWorkspace implementation =="
grep -nA120 -B40 'patchWorkspace' apps/packages/ui/src/services/tldw/TldwApiClient.ts | sed -n '1,220p'Repository: rmusser01/tldw_server
Length of output: 16085
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the real patchWorkspace implementation and shared request/error helpers.
rg -n --no-heading \
-e 'patchWorkspace\s*\(' \
-e 'class .*TldwApiClient|export class .*TldwApiClient|new Error\(' \
apps/packages/ui/src/services/tldw apps/packages/ui/src | sed -n '1,260p'
echo
echo "== Nearby client file names =="
fd -a 'TldwApiClient.ts' apps/packages/ui/src
fd -a '*ApiClient*.ts' apps/packages/ui/src
echo
echo "== If the client is split, inspect the likely request helper files =="
rg -n --no-heading \
-e 'fetch\(' \
-e 'axios' \
-e 'response\.json\(' \
-e 'throw .*Error' \
apps/packages/ui/src/services/tldw apps/packages/ui/src/services | sed -n '1,260p'Repository: rmusser01/tldw_server
Length of output: 33594
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workspace-api outline around patchWorkspace =="
ast-grep outline apps/packages/ui/src/services/tldw/domains/workspace-api.ts --view expanded | sed -n '680,880p'
echo
echo "== patchWorkspace implementation =="
sed -n '1230,1315p' apps/packages/ui/src/services/tldw/domains/workspace-api.ts | cat -n
echo
echo "== request helpers in workspace-api =="
sed -n '1,220p' apps/packages/ui/src/services/tldw/domains/workspace-api.ts | cat -n
echo
echo "== tests for patchWorkspace errors =="
sed -n '500,650p' apps/packages/ui/src/services/__tests__/tldw-api-client.workspace-api.test.ts | cat -nRepository: rmusser01/tldw_server
Length of output: 16545
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== background-proxy request implementation =="
ast-grep outline apps/packages/ui/src/services/background-proxy.ts --view expanded | sed -n '1,260p'
echo
echo "== background-proxy relevant lines =="
grep -nA220 -B80 'function bgRequest\|const bgRequest\|export async function bgRequest\|export const bgRequest' \
apps/packages/ui/src/services/background-proxy.ts | sed -n '1,260p'
echo
echo "== request-core helpers =="
fd -a 'request-core*' apps/packages/ui/src/services
rg -n --no-heading 'response\.error|throw new Error|reject\(' apps/packages/ui/src/services/background-proxy.ts apps/packages/ui/src/services/request-core* apps/packages/ui/src/services -g '!**/node_modules/**' | sed -n '1,260p'
echo
echo "== workspace API tests for rejected patchWorkspace if any =="
rg -n --no-heading 'patchWorkspace\("ws-1".*reject|mockRejectedValue|assistant defaults|default assistant' \
apps/packages/ui/src/services/__tests__/tldw-api-client.workspace-api.test.ts \
apps/packages/ui/src/components/Option/ResearchWorkspace -g '!**/node_modules/**' | sed -n '1,260p'Repository: rmusser01/tldw_server
Length of output: 41239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== bgRequestImpl error handling around runtime response =="
sed -n '730,860p' apps/packages/ui/src/services/background-proxy.ts | cat -n
echo
echo "== bgRequestImpl backend response handling =="
sed -n '1035,1070p' apps/packages/ui/src/services/background-proxy.ts | cat -n
echo
echo "== request-core error shaping =="
sed -n '1,260p' apps/packages/ui/src/services/tldw/request-core.ts | cat -n
echo
echo "== workspace API save/clear tests (if present) =="
rg -n --no-heading 'default assistant|defaultAssistant|patchWorkspace\(' \
apps/packages/ui/src/components/Option/ResearchWorkspace \
apps/packages/ui/src/services/__tests__/tldw-api-client.workspace-api.test.ts | sed -n '1,220p'Repository: rmusser01/tldw_server
Length of output: 34254
Surface the request error instead of always showing the fallback copy
handleSaveDefaultAssistant and handleClearDefaultAssistant replace every failure with the same hardcoded message. tldwClient.patchWorkspace() already throws an Error whose .message carries the backend detail when available, so this hides actionable 400/422/conflict feedback from the user.
🤖 Prompt for 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.
In `@apps/packages/ui/src/components/Option/ResearchWorkspace/WorkspaceHeader.tsx`
around lines 810 - 865, Both handleSaveDefaultAssistant and
handleClearDefaultAssistant are swallowing the thrown request error and always
showing fallback copy. Update the catch blocks to capture the exception from
tldwClient.patchWorkspace(), read its message when available, and surface that
backend detail in the default assistant error state instead of replacing every
failure with the same generic text.
Comment on lines
+345
to
+361
| it("routes an inherited workspace persona default when no explicit assistant is selected", async () => { | ||
| const inheritedWorkspaceAssistant = { | ||
| kind: "persona" as const, | ||
| id: "workspace-helper", | ||
| name: "Workspace Helper", | ||
| metadata: { | ||
| selectionMode: "tracked", | ||
| source: "workspace", | ||
| personaMemoryMode: "read_write" | ||
| } | ||
| } | ||
| const options = { | ||
| ...createHookOptions(), | ||
| selectedAssistant: inheritedWorkspaceAssistant, | ||
| inheritedAssistant: inheritedWorkspaceAssistant, | ||
| inheritedPersonaMemoryMode: "read_write" as const | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test doesn't actually exercise the "no explicit assistant selected" fallback it claims to test.
options.selectedAssistant is set to the same inheritedWorkspaceAssistant object. Per useChatActions.ts, inheritedTrackedAssistant short-circuits to null whenever selectedAssistant is truthy (line ~545), so routingSelectedAssistant here just resolves to selectedAssistant directly — the inherited-fallback code path (used when a fresh chat has no draft selection at all) is never exercised. To actually cover that path, selectedAssistant should be null here.
🧪 Suggested fix
const options = {
...createHookOptions(),
- selectedAssistant: inheritedWorkspaceAssistant,
+ selectedAssistant: null,
inheritedAssistant: inheritedWorkspaceAssistant,
inheritedPersonaMemoryMode: "read_write" as const
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("routes an inherited workspace persona default when no explicit assistant is selected", async () => { | |
| const inheritedWorkspaceAssistant = { | |
| kind: "persona" as const, | |
| id: "workspace-helper", | |
| name: "Workspace Helper", | |
| metadata: { | |
| selectionMode: "tracked", | |
| source: "workspace", | |
| personaMemoryMode: "read_write" | |
| } | |
| } | |
| const options = { | |
| ...createHookOptions(), | |
| selectedAssistant: inheritedWorkspaceAssistant, | |
| inheritedAssistant: inheritedWorkspaceAssistant, | |
| inheritedPersonaMemoryMode: "read_write" as const | |
| } | |
| it("routes an inherited workspace persona default when no explicit assistant is selected", async () => { | |
| const inheritedWorkspaceAssistant = { | |
| kind: "persona" as const, | |
| id: "workspace-helper", | |
| name: "Workspace Helper", | |
| metadata: { | |
| selectionMode: "tracked", | |
| source: "workspace", | |
| personaMemoryMode: "read_write" | |
| } | |
| } | |
| const options = { | |
| ...createHookOptions(), | |
| selectedAssistant: null, | |
| inheritedAssistant: inheritedWorkspaceAssistant, | |
| inheritedPersonaMemoryMode: "read_write" as const | |
| } |
🤖 Prompt for 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.
In
`@apps/packages/ui/src/hooks/chat/__tests__/useChatActions.persona.integration.test.tsx`
around lines 345 - 361, The test setup in
useChatActions.persona.integration.test.tsx does not cover the claimed “no
explicit assistant selected” fallback because selectedAssistant is still set to
the inherited workspace assistant, which bypasses the inherited-tracked path in
useChatActions.ts. Update this case so selectedAssistant is null while keeping
inheritedAssistant and inheritedPersonaMemoryMode set, allowing
routingSelectedAssistant to exercise the fallback logic for a fresh chat with no
draft selection.
Comment on lines
+171
to
+179
| type PersonaMemoryMode = "read_only" | "read_write" | ||
|
|
||
| const loadActorSettings = () => import("@/services/actor-settings") | ||
| const STREAMING_UPDATE_INTERVAL_MS = 80 | ||
| const toChatSubmitResult = normalizeChatSubmitResult | ||
|
|
||
| const normalizePersonaMemoryMode = (value: unknown): PersonaMemoryMode | null => | ||
| value === "read_only" || value === "read_write" ? value : null | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate PersonaMemoryMode/normalizePersonaMemoryMode also defined in personaServerChat.ts.
personaServerChat.ts (lines 8-11) already defines an identical PersonaMemoryMode type and normalizePersonaMemoryMode helper. Consolidating (export from personaServerChat.ts and import here) avoids the two copies silently diverging later.
♻️ Suggested consolidation
-type PersonaMemoryMode = "read_only" | "read_write"
-
-const loadActorSettings = () => import("`@/services/actor-settings`")
-const STREAMING_UPDATE_INTERVAL_MS = 80
-const toChatSubmitResult = normalizeChatSubmitResult
-
-const normalizePersonaMemoryMode = (value: unknown): PersonaMemoryMode | null =>
- value === "read_only" || value === "read_write" ? value : null
+import { normalizePersonaMemoryMode, type PersonaMemoryMode } from "./personaServerChat"
+
+const loadActorSettings = () => import("`@/services/actor-settings`")
+const STREAMING_UPDATE_INTERVAL_MS = 80
+const toChatSubmitResult = normalizeChatSubmitResult(requires exporting both from personaServerChat.ts)
🤖 Prompt for 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.
In `@apps/packages/ui/src/hooks/chat/useChatActions.ts` around lines 171 - 179,
Duplicate PersonaMemoryMode and normalizePersonaMemoryMode are defined here and
in personaServerChat.ts, so consolidate them into the shared helper. Export the
type and normalizePersonaMemoryMode from personaServerChat.ts, then import and
reuse them in useChatActions.ts instead of keeping a local copy. Keep the
existing call sites intact and remove the duplicate definitions to prevent
future drift.
| --- | ||
| id: TASK-12115 | ||
| title: Rebase PR 2316 on latest dev and fix CI failures | ||
| status: Done |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Check off the completed DoD items.
The final summary says this work is complete, but the DoD checklist is still entirely unchecked. Please sync the checklist with the recorded verification so the task metadata is unambiguous.
Also applies to: 31-39
🤖 Prompt for 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.
In `@backlog/tasks/task-12115` -
Rebase-PR-2316-on-latest-dev-and-fix-CI-failures.md at line 4, The task metadata
is inconsistent because the final summary says the work is complete but the DoD
checklist remains unchecked. Update the checklist in the task summary to match
the recorded verification by checking off the completed items in the DoD
section, and make sure the entries under the task content (including the
checklist items around the referenced range) reflect the completed status.
| --- | ||
| id: TASK-12116 | ||
| title: Address PR 2571 review comments | ||
| status: Done |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the state mismatch before marking this task Done.
The final summary still says the PR Change summary is a requester-owned merge gate, so the task does not read as fully complete yet. Either move the status back to an active state, or remove that blocker note if it is no longer required.
Also applies to: 40-42
🤖 Prompt for 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.
In `@backlog/tasks/task-12116` - Address-PR-2571-review-comments.md at line 4, The
task is marked Done while the final summary still contains an unresolved blocker
note about the PR Change summary being a requester-owned merge gate. Update the
task state to match reality by either changing the status back to an active
state if the blocker remains, or removing the blocker note from the summary if
it is no longer needed. Make sure the status field and the final summary text
are consistent.
| --- | ||
| id: TASK-12117 | ||
| title: Fix PR 2571 release CI failures | ||
| status: In Progress |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the status and checklist with the final summary.
The summary says the CI failures were fixed and validation completed, but the task is still marked In Progress and none of the DoD boxes are checked. Please update the metadata to match the recorded completion state, or keep it open until there is still outstanding work.
Also applies to: 34-48
🤖 Prompt for 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.
In `@backlog/tasks/task-12117` - Fix-PR-2571-release-CI-failures.md at line 4, The
task metadata is inconsistent with the final summary: the status still says In
Progress and the DoD checklist is unchecked even though the summary indicates
completion. Update the task header status and the checklist items in the
markdown so they match the recorded completed state, using the task’s status
field and the DoD checkboxes in the document.
Comment on lines
+206
to
+276
| def _effective_workspace_assistant_default( | ||
| *, | ||
| db: CharactersRAGDB, | ||
| stored: WorkspaceAssistantDefaults | None, | ||
| user_id: str, | ||
| invalid_stored_default: bool = False, | ||
| persona_profile_cache: WorkspacePersonaProfileCache | None = None, | ||
| ) -> WorkspaceEffectiveAssistantDefault: | ||
| """Resolve a Workspace assistant default into a permission-safe client projection.""" | ||
| if invalid_stored_default: | ||
| return WorkspaceEffectiveAssistantDefault( | ||
| status="unavailable", | ||
| source="workspace", | ||
| degraded_reason="invalid_default", | ||
| ) | ||
| if stored is None: | ||
| return WorkspaceEffectiveAssistantDefault(status="none", source="none") | ||
|
|
||
| if stored.assistant_kind != "persona": | ||
| return WorkspaceEffectiveAssistantDefault( | ||
| status="unavailable", | ||
| source="workspace", | ||
| degraded_reason="unsupported_assistant_kind", | ||
| ) | ||
|
|
||
| profile = _get_workspace_persona_profile( | ||
| db=db, | ||
| assistant_id=stored.assistant_id, | ||
| user_id=user_id, | ||
| include_deleted=False, | ||
| cache=persona_profile_cache, | ||
| ) | ||
| if profile is not None: | ||
| if not bool(profile.get("is_active", True)): | ||
| return WorkspaceEffectiveAssistantDefault( | ||
| status="unavailable", | ||
| source="workspace", | ||
| assistant_kind="persona", | ||
| assistant_id=stored.assistant_id, | ||
| persona_memory_mode=stored.persona_memory_mode, | ||
| degraded_reason="persona_unavailable", | ||
| ) | ||
| return WorkspaceEffectiveAssistantDefault( | ||
| status="available", | ||
| source="workspace", | ||
| assistant_kind="persona", | ||
| assistant_id=stored.assistant_id, | ||
| label=str(profile.get("name") or stored.assistant_id), | ||
| persona_memory_mode=stored.persona_memory_mode, | ||
| ) | ||
|
|
||
| deleted_profile = _get_workspace_persona_profile( | ||
| db=db, | ||
| assistant_id=stored.assistant_id, | ||
| user_id=user_id, | ||
| include_deleted=True, | ||
| cache=persona_profile_cache, | ||
| ) | ||
| degraded_reason = ( | ||
| "persona_deleted" | ||
| if deleted_profile is not None | ||
| else "permission_denied" | ||
| ) | ||
| return WorkspaceEffectiveAssistantDefault( | ||
| status="unavailable", | ||
| source="workspace", | ||
| assistant_kind="persona", | ||
| assistant_id=stored.assistant_id, | ||
| persona_memory_mode=stored.persona_memory_mode, | ||
| degraded_reason=degraded_reason, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Consider surfacing persona label in degraded states too.
label is only populated in the "available" branch (line 253). The "persona_unavailable" branch (240-247) has profile in scope and the "persona_deleted" branch (257-276) fetches deleted_profile, but neither passes profile.get("name")/deleted_profile.get("name") into label. Surfacing the name would let the WorkspaceHeader UI show which persona became unavailable/deleted instead of just an opaque ID.
♻️ Proposed enrichment
if not bool(profile.get("is_active", True)):
return WorkspaceEffectiveAssistantDefault(
status="unavailable",
source="workspace",
assistant_kind="persona",
assistant_id=stored.assistant_id,
+ label=str(profile.get("name") or stored.assistant_id),
persona_memory_mode=stored.persona_memory_mode,
degraded_reason="persona_unavailable",
) degraded_reason = (
"persona_deleted"
if deleted_profile is not None
else "permission_denied"
)
return WorkspaceEffectiveAssistantDefault(
status="unavailable",
source="workspace",
assistant_kind="persona",
assistant_id=stored.assistant_id,
+ label=str(deleted_profile.get("name")) if deleted_profile else None,
persona_memory_mode=stored.persona_memory_mode,
degraded_reason=degraded_reason,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _effective_workspace_assistant_default( | |
| *, | |
| db: CharactersRAGDB, | |
| stored: WorkspaceAssistantDefaults | None, | |
| user_id: str, | |
| invalid_stored_default: bool = False, | |
| persona_profile_cache: WorkspacePersonaProfileCache | None = None, | |
| ) -> WorkspaceEffectiveAssistantDefault: | |
| """Resolve a Workspace assistant default into a permission-safe client projection.""" | |
| if invalid_stored_default: | |
| return WorkspaceEffectiveAssistantDefault( | |
| status="unavailable", | |
| source="workspace", | |
| degraded_reason="invalid_default", | |
| ) | |
| if stored is None: | |
| return WorkspaceEffectiveAssistantDefault(status="none", source="none") | |
| if stored.assistant_kind != "persona": | |
| return WorkspaceEffectiveAssistantDefault( | |
| status="unavailable", | |
| source="workspace", | |
| degraded_reason="unsupported_assistant_kind", | |
| ) | |
| profile = _get_workspace_persona_profile( | |
| db=db, | |
| assistant_id=stored.assistant_id, | |
| user_id=user_id, | |
| include_deleted=False, | |
| cache=persona_profile_cache, | |
| ) | |
| if profile is not None: | |
| if not bool(profile.get("is_active", True)): | |
| return WorkspaceEffectiveAssistantDefault( | |
| status="unavailable", | |
| source="workspace", | |
| assistant_kind="persona", | |
| assistant_id=stored.assistant_id, | |
| persona_memory_mode=stored.persona_memory_mode, | |
| degraded_reason="persona_unavailable", | |
| ) | |
| return WorkspaceEffectiveAssistantDefault( | |
| status="available", | |
| source="workspace", | |
| assistant_kind="persona", | |
| assistant_id=stored.assistant_id, | |
| label=str(profile.get("name") or stored.assistant_id), | |
| persona_memory_mode=stored.persona_memory_mode, | |
| ) | |
| deleted_profile = _get_workspace_persona_profile( | |
| db=db, | |
| assistant_id=stored.assistant_id, | |
| user_id=user_id, | |
| include_deleted=True, | |
| cache=persona_profile_cache, | |
| ) | |
| degraded_reason = ( | |
| "persona_deleted" | |
| if deleted_profile is not None | |
| else "permission_denied" | |
| ) | |
| return WorkspaceEffectiveAssistantDefault( | |
| status="unavailable", | |
| source="workspace", | |
| assistant_kind="persona", | |
| assistant_id=stored.assistant_id, | |
| persona_memory_mode=stored.persona_memory_mode, | |
| degraded_reason=degraded_reason, | |
| ) | |
| def _effective_workspace_assistant_default( | |
| *, | |
| db: CharactersRAGDB, | |
| stored: WorkspaceAssistantDefaults | None, | |
| user_id: str, | |
| invalid_stored_default: bool = False, | |
| persona_profile_cache: WorkspacePersonaProfileCache | None = None, | |
| ) -> WorkspaceEffectiveAssistantDefault: | |
| """Resolve a Workspace assistant default into a permission-safe client projection.""" | |
| if invalid_stored_default: | |
| return WorkspaceEffectiveAssistantDefault( | |
| status="unavailable", | |
| source="workspace", | |
| degraded_reason="invalid_default", | |
| ) | |
| if stored is None: | |
| return WorkspaceEffectiveAssistantDefault(status="none", source="none") | |
| if stored.assistant_kind != "persona": | |
| return WorkspaceEffectiveAssistantDefault( | |
| status="unavailable", | |
| source="workspace", | |
| degraded_reason="unsupported_assistant_kind", | |
| ) | |
| profile = _get_workspace_persona_profile( | |
| db=db, | |
| assistant_id=stored.assistant_id, | |
| user_id=user_id, | |
| include_deleted=False, | |
| cache=persona_profile_cache, | |
| ) | |
| if profile is not None: | |
| if not bool(profile.get("is_active", True)): | |
| return WorkspaceEffectiveAssistantDefault( | |
| status="unavailable", | |
| source="workspace", | |
| assistant_kind="persona", | |
| assistant_id=stored.assistant_id, | |
| label=str(profile.get("name") or stored.assistant_id), | |
| persona_memory_mode=stored.persona_memory_mode, | |
| degraded_reason="persona_unavailable", | |
| ) | |
| return WorkspaceEffectiveAssistantDefault( | |
| status="available", | |
| source="workspace", | |
| assistant_kind="persona", | |
| assistant_id=stored.assistant_id, | |
| label=str(profile.get("name") or stored.assistant_id), | |
| persona_memory_mode=stored.persona_memory_mode, | |
| ) | |
| deleted_profile = _get_workspace_persona_profile( | |
| db=db, | |
| assistant_id=stored.assistant_id, | |
| user_id=user_id, | |
| include_deleted=True, | |
| cache=persona_profile_cache, | |
| ) | |
| degraded_reason = ( | |
| "persona_deleted" | |
| if deleted_profile is not None | |
| else "permission_denied" | |
| ) | |
| return WorkspaceEffectiveAssistantDefault( | |
| status="unavailable", | |
| source="workspace", | |
| assistant_kind="persona", | |
| assistant_id=stored.assistant_id, | |
| label=str(deleted_profile.get("name")) if deleted_profile else None, | |
| persona_memory_mode=stored.persona_memory_mode, | |
| degraded_reason=degraded_reason, | |
| ) |
🤖 Prompt for 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.
In `@tldw_Server_API/app/api/v1/endpoints/workspaces.py` around lines 206 - 276,
The degraded persona branches in _effective_workspace_assistant_default
currently omit label, so the UI only sees an opaque assistant_id when a persona
is unavailable or deleted. Update the persona_unavailable and persona_deleted
paths to populate label from the resolved profile data, using
profile.get("name") or deleted_profile.get("name") with a fallback to
stored.assistant_id, while preserving the existing degraded_reason and other
fields in WorkspaceEffectiveAssistantDefault.
Comment on lines
+113
to
+128
| def _redact_websearch_log_text(value: str) -> str: | ||
| parsed = urlparse(value) | ||
| if parsed.scheme in {"http", "https"} and parsed.netloc: | ||
| safe_pairs = [ | ||
| ( | ||
| key, | ||
| "[REDACTED]" | ||
| if key.strip().lower() in _WEBSEARCH_SENSITIVE_LOG_KEYS | ||
| else redacted_value, | ||
| ) | ||
| for key, raw_value in parse_qsl(parsed.query, keep_blank_values=True) | ||
| for redacted_value in (_redact_websearch_log_text(raw_value),) | ||
| ] | ||
| safe_query = urlencode(safe_pairs, doseq=True, quote_via=quote) | ||
| return urlunparse(parsed._replace(query=safe_query, fragment="")) | ||
| return _WEBSEARCH_SENSITIVE_PAIR_RE.sub(lambda match: f"{match.group(1)}=[REDACTED]", value) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tldw_Server_API/app/core/Web_Scraping/WebSearch_APIs.py"
echo "== outline =="
ast-grep outline "$file" --view expanded | sed -n '1,220p'
echo
echo "== relevant lines =="
sed -n '100,145p' "$file" | cat -n
echo
echo "== usages of _redact_websearch_log_text =="
rg -n "_redact_websearch_log_text|_WEBSEARCH_SENSITIVE_LOG_KEYS|_WEBSEARCH_SENSITIVE_PAIR_RE" "$file"
echo
echo "== search for URL redaction helpers elsewhere =="
rg -n "urlparse|urlunparse|parse_qsl|urlencode|quote_via|REDACT" tldw_Server_API/app/core/Web_Scraping -g '*.py'Repository: rmusser01/tldw_server
Length of output: 14272
Redact URL userinfo too tldw_Server_API/app/core/Web_Scraping/WebSearch_APIs.py:113-128 still leaves user:pass@host intact because the URL branch only strips query params and skips the regex fallback. That can leak credentials in debug logs.
🤖 Prompt for 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.
In `@tldw_Server_API/app/core/Web_Scraping/WebSearch_APIs.py` around lines 113 -
128, The _redact_websearch_log_text helper still leaves URL userinfo exposed
when parsing http/https URLs, because the urlparse branch only redacts query
parameters and returns before the regex fallback. Update
_redact_websearch_log_text so it also strips parsed.username/parsed.password
from the netloc before urlunparse, or otherwise ensures user:pass@host is
replaced with a safe host-only form while keeping the existing query redaction
behavior.
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
2 participants
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.
Change summary
Pending human-authored change summary before merge per
Docs/superpowers/AI_GENERATED_PR_CHANGE_SUMMARY_POLICY_2026_04_17.md.Technical summary
Merge current
devintomainfor the0.1.34release train. This carries the release metadata prepared in PR #2570 plus the post-0.1.33follow-up work from PR #2568.Highlights:
0.1.34.0.1.34CHANGELOG entry covering work since0.1.33.0.1.34.mcp-unifiedtyped-marker/package-data coverage.Local verification before PR creation
git fetch origin main devgit rev-list --left-right --count origin/main...origin/dev->2 7dev -> mainPR before creating this one.Release boundary
After this PR merges and required checks are green on
main, run the formalmake releasepath frommain. The release helper intentionally refuses to cut formal releases fromdev.Summary by cubic
Ships v0.1.34 with audit-driven WebUI/extension hardening, a standalone docs corpus in
mcp_unified.docs(local import, SQLite/FTS5 search, optional safe URL ingestion), a fullscreen chat focus mode, Workspace Assistant Defaults V1, and stronger CI gates (coverage floor + nightly perf). Finalizes PR #2571/#2568 follow-ups, fixes llama.cpp WebUI auth, sanitizes external links, and runs UX gates on Next.js standalone.New Features
mcp_unified.docswith local markdown/html/text import, SQLite/FTS5 store + search, alias resolution, bounded context packs, a mount factory, and a thin server adapter; optional URL ingestion with DNS/IP validation, redirect-aware fetcher, and static/rich extractors (docs-webextra).Bug Fixes
safeExternalUrl/openExternalUrlat anchor and window.open sinks (Knowledge, Items, Reading List, Message sources), markdown link sanitization, CSP and security headers; block avatar SSRF in PNG export (same-origin/allowlist) and add size caps for avatar uploads and character-card imports; broaden request-body redaction and clear history on logout.pytest_asyncio; release helper anchor checks tightened.Written for commit 473fa2c. Summary will update on new commits.