Add typed raw accessors with drift logging; migrate workspace tabs and resolver#94
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds defensive, raw-backed properties to Composer and Bubble, a new models/raw_access module with optional-field readers and schema-drift warnings, refactors workspace resolver/tabs to use these typed accessors and model objects, updates text extraction to accept model objects, and adds tests for drift-logging behavior. ChangesTyped Raw Accessor Layer
Sequence Diagram(s)sequenceDiagram
participant Client as service caller
participant Tabs as services.workspace_tabs
participant Resolver as services.workspace_resolver
participant Models as models.raw_access / models.conversation
Client->>Tabs: request tabs assembly (composer raw rows)
Tabs->>Models: Composer.from_dict / Composer properties (usage_data, model_name_from_config)
Tabs->>Models: conversation_header_bubble_id(header)
Tabs->>Resolver: determine_project_for_conversation(composer)
Resolver->>Models: composer_newly_created_files / composer_code_block_data / bubble helpers
Models-->>Resolver: validated lists/dicts or safe defaults
Resolver-->>Tabs: resolved project/workspace
Tabs-->>Client: assembled tab payloads
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/test_raw_accessors.py (1)
26-30: ⚡ Quick winMake the “missing key” setup explicit.
This test currently relies on
GOOD_COMPOSER_RAWnot containingnewlyCreatedFiles. To keep the test stable and self-descriptive, remove the key explicitly in-test before callingComposer.from_dict(...).Suggested patch
- bare = Composer.from_dict(GOOD_COMPOSER_RAW, composer_id="cid-2") + raw = dict(GOOD_COMPOSER_RAW) + raw.pop("newlyCreatedFiles", None) + bare = Composer.from_dict(raw, composer_id="cid-2")🤖 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 `@tests/test_raw_accessors.py` around lines 26 - 30, The test test_composer_newly_created_files_empty_when_key_missing should explicitly remove the 'newlyCreatedFiles' key from the input before calling Composer.from_dict to avoid relying on the global GOOD_COMPOSER_RAW shape; create a shallow or deep copy of GOOD_COMPOSER_RAW, pop the 'newlyCreatedFiles' key if present on that copy, then pass the modified dict into Composer.from_dict(..., composer_id="cid-2") and keep the existing with self.assertNoLogs and self.assertEqual(bare.newly_created_files, []). Ensure you do not mutate the global GOOD_COMPOSER_RAW variable.utils/text_extract.py (1)
24-27: ⚡ Quick winTyping precision:
bubble: dict | objectis effectively justobjectfor static type checking (sincedictis a subtype ofobject), so it loses most type value; narrow it to the specific “hasraw” shape (e.g., aProtocol) to keep mypy/pyright helpful.Suggested patch
+from typing import Any, Protocol + +class HasRaw(Protocol): + raw: dict[str, Any] + -def extract_text_from_bubble(bubble: dict | object) -> str: +def extract_text_from_bubble(bubble: dict[str, Any] | HasRaw) -> str: """Extract displayable text from a bubble object (text, richText, codeBlocks).""" - if hasattr(bubble, "raw"): - bubble = bubble.raw # type: ignore[union-attr] + if hasattr(bubble, "raw"): + bubble = bubble.raw🤖 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 `@utils/text_extract.py` around lines 24 - 27, The parameter typing for extract_text_from_bubble is too broad (dict | object) so static checkers lose precision; define a small Protocol (e.g., HasRaw with attribute raw: dict) or a TypedDict for the expected bubble shape and change the function signature to use that Protocol/TypedDict (e.g., bubble: HasRaw | dict) so mypy/pyright can infer attributes like raw and the contained fields used later in extract_text_from_bubble; update any imports and type comments to keep compatibility with Python versions in the project.models/conversation.py (1)
3-9: 💤 Low valueMove
_loggerbelow the imports to avoid E402.The
_logger = logging.getLogger(__name__)assignment at Line 7 sits betweenfrom typing import Anyand thefrom models.errors import SchemaErrorblock, so the latter is a module-level import that no longer appears at the top of the file. Most linters (flake8/ruffE402) will flag this.♻️ Proposed reordering
import logging from dataclasses import dataclass, field from typing import Any -_logger = logging.getLogger(__name__) - from models.errors import SchemaError from models.from_dict_validation import ( require_dict, require_key, require_non_empty_str, require_non_empty_str_field, require_type, ) + +_logger = logging.getLogger(__name__)🤖 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 `@models/conversation.py` around lines 3 - 9, _the module-level logger `_logger` is defined before the final import `from models.errors import SchemaError`, causing an import-after-code lint error (E402); move the `_logger = logging.getLogger(__name__)` line so it appears after all import statements (including `from models.errors import SchemaError`) — locate `_logger` and the import `SchemaError` in this file and reorder so all imports come first, then initialize `_logger`.
🤖 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 `@models/raw_access.py`:
- Around line 158-190: composer_headers currently calls optional_raw_list which
emits a missing-key WARNING, while composer_newly_created_files silently returns
[] when the key is absent; align them to the silent-on-missing contract by
changing composer_headers to mirror composer_newly_created_files: if data is a
Composer return data.full_conversation_headers_only, otherwise read value =
data.get("fullConversationHeadersOnly") when data is a dict, return [] if value
is None, warn only if value exists but is not a list (similar warning message
pattern used in composer_newly_created_files), and avoid relying on
optional_raw_list to emit missing-key warnings.
- Around line 203-242: The three accessors bubble_relevant_files,
bubble_attached_file_uris, and bubble_context silently drop values of the wrong
type instead of logging drift; change their dict-path handling to call the
existing helpers optional_raw_list (for keys "relevantFiles" and
"attachedFileCodeChunksUris") and optional_raw_dict (for key "context") rather
than manually checking types, so type-mismatches are logged consistently (remove
the duplicated extraction logic and return the helper results when present).
---
Nitpick comments:
In `@models/conversation.py`:
- Around line 3-9: _the module-level logger `_logger` is defined before the
final import `from models.errors import SchemaError`, causing an
import-after-code lint error (E402); move the `_logger =
logging.getLogger(__name__)` line so it appears after all import statements
(including `from models.errors import SchemaError`) — locate `_logger` and the
import `SchemaError` in this file and reorder so all imports come first, then
initialize `_logger`.
In `@tests/test_raw_accessors.py`:
- Around line 26-30: The test
test_composer_newly_created_files_empty_when_key_missing should explicitly
remove the 'newlyCreatedFiles' key from the input before calling
Composer.from_dict to avoid relying on the global GOOD_COMPOSER_RAW shape;
create a shallow or deep copy of GOOD_COMPOSER_RAW, pop the 'newlyCreatedFiles'
key if present on that copy, then pass the modified dict into
Composer.from_dict(..., composer_id="cid-2") and keep the existing with
self.assertNoLogs and self.assertEqual(bare.newly_created_files, []). Ensure you
do not mutate the global GOOD_COMPOSER_RAW variable.
In `@utils/text_extract.py`:
- Around line 24-27: The parameter typing for extract_text_from_bubble is too
broad (dict | object) so static checkers lose precision; define a small Protocol
(e.g., HasRaw with attribute raw: dict) or a TypedDict for the expected bubble
shape and change the function signature to use that Protocol/TypedDict (e.g.,
bubble: HasRaw | dict) so mypy/pyright can infer attributes like raw and the
contained fields used later in extract_text_from_bubble; update any imports and
type comments to keep compatibility with Python versions in the project.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 563cb83f-22bd-4ffb-a6d0-7d58c209538a
📒 Files selected for processing (6)
models/conversation.pymodels/raw_access.pyservices/workspace_resolver.pyservices/workspace_tabs.pytests/test_raw_accessors.pyutils/text_extract.py
bradjin8
force-pushed
the
feat/typed-accessor-methods
branch
from
1e98ae3 to
19eb9da
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/workspace_tabs.py (2)
136-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't treat an empty raw bubble payload as "missing".
bubble_map.get()returnsNoneonly when the id is absent. With the newBubble | dictbridge,if not bubble_entryalso drops{}payloads beforeBubble.from_dict(...)runs, so context-only bubbles can disappear silently in the single-tab path.Proposed fix
bubble_id = conversation_header_bubble_id(header, composer_id=composer_id) if not bubble_id: continue bubble_entry = bubble_map.get(bubble_id) - if not bubble_entry: + if bubble_entry is None: continue if isinstance(bubble_entry, Bubble): bubble = bubble_entry else:🤖 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 `@services/workspace_tabs.py` around lines 136 - 143, The code is incorrectly treating empty dict payloads as missing because it uses a falsy check; change the presence check to a strict None test so empty mappings are preserved — replace "if not bubble_entry" with "if bubble_entry is None" (or alternately check "if bubble_id not in bubble_map") before the isinstance/Bubble.from_dict branch to ensure {} payloads are passed into Bubble.from_dict rather than skipped.
573-617:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate summary rows with
Composer.from_dict()too.This path still bypasses the typed composer contract and reads
cd.get(...)directly. A drifted row can therefore appear inlist_workspace_tab_summaries()even thoughassemble_single_tab()andassemble_workspace_tabs()reject the same composer viaComposer.from_dict(), which leaves the sidebar and detail endpoints disagreeing on what a valid conversation is.Proposed fix
if not isinstance(cd, dict): parse_warnings.record_composer_skipped() continue try: + composer = Composer.from_dict(cd, composer_id=composer_id) if ( composer_id not in composer_id_to_ws and composer_id not in project_layouts_map ): project_layouts_map[composer_id] = load_project_layouts_for_composer( global_db, composer_id, ) pid = determine_project_for_conversation( - cd, composer_id, project_layouts_map, + composer, composer_id, project_layouts_map, project_name_map, workspace_path_map, workspace_entries, {}, composer_id_to_ws, invalid_workspace_ids, ) @@ - headers = cd.get("fullConversationHeadersOnly") or [] + headers = composer.full_conversation_headers_only if not headers: continue - title = cd.get("name") or f"Conversation {composer_id[:8]}" + title = composer.name or f"Conversation {composer_id[:8]}" - _early_model_config = cd.get("modelConfig") or {} - _early_model_name = _early_model_config.get("modelName") + _early_model_name = composer.model_name_from_config() _early_model_names = [_early_model_name] if _early_model_name and _early_model_name != "default" else None @@ tab_entry: dict = { "id": composer_id, "title": title, - "timestamp": to_epoch_ms(cd.get("lastUpdatedAt")) or to_epoch_ms(cd.get("createdAt")) or int(datetime.now().timestamp() * 1000), + "timestamp": to_epoch_ms(composer.last_updated_at) or to_epoch_ms(composer.created_at) or int(datetime.now().timestamp() * 1000), "messageCount": len(headers), } + except SchemaError as e: + _logger.warning( + "Failed to parse Composer from composerData:%s: %s", + composer_id, + e, + ) + parse_warnings.record_composer_skipped() + continue🤖 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 `@services/workspace_tabs.py` around lines 573 - 617, The summary-row path in list_workspace_tab_summaries currently reads raw composer dicts (cd) directly and can include drifted rows that Composer.from_dict() would reject; update the logic in list_workspace_tab_summaries to attempt constructing a typed Composer via Composer.from_dict(cd) (same validation used by assemble_single_tab()/assemble_workspace_tabs()), and if Composer.from_dict raises/returns invalid, skip that summary row (continue) so the sidebar and detail endpoints remain consistent; reference the composer dict variable cd and functions Composer.from_dict, assemble_single_tab, assemble_workspace_tabs when locating where to insert this validation.
🤖 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.
Outside diff comments:
In `@services/workspace_tabs.py`:
- Around line 136-143: The code is incorrectly treating empty dict payloads as
missing because it uses a falsy check; change the presence check to a strict
None test so empty mappings are preserved — replace "if not bubble_entry" with
"if bubble_entry is None" (or alternately check "if bubble_id not in
bubble_map") before the isinstance/Bubble.from_dict branch to ensure {} payloads
are passed into Bubble.from_dict rather than skipped.
- Around line 573-617: The summary-row path in list_workspace_tab_summaries
currently reads raw composer dicts (cd) directly and can include drifted rows
that Composer.from_dict() would reject; update the logic in
list_workspace_tab_summaries to attempt constructing a typed Composer via
Composer.from_dict(cd) (same validation used by
assemble_single_tab()/assemble_workspace_tabs()), and if Composer.from_dict
raises/returns invalid, skip that summary row (continue) so the sidebar and
detail endpoints remain consistent; reference the composer dict variable cd and
functions Composer.from_dict, assemble_single_tab, assemble_workspace_tabs when
locating where to insert this validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9b0fc91-65e8-46d2-955e-ee7f60388a4f
📒 Files selected for processing (2)
services/workspace_tabs.pytests/test_blob_parsing_fuzz.py
Must fixmodels/raw_access.py:150-155, services/workspace_tabs.py:887, services/workspace_resolver.py:132 — stop WARNING-logging when projectLayouts is absent on messageRequestContext rows scanned in bulk (e.g. warn_if_missing=False on optional_raw_list, or a probe-specific helper that only warns on wrong type) (most context blobs omit projectLayouts; current code emits one WARNING per row on every tab load and during infer_workspace_name_from_context, drowning real drift signals) Should fixmodels/conversation.py:73-85, models/raw_access.py:28-39, Doc/Issues/cppa-cursor-browser issue 90.md:9 — align missing-key policy with issue #90: either document “absent-by-design” fields (no warn) vs “drift-sensitive” fields (warn), or add missing-key warnings on the Composer/Bubble properties tests currently forbid (issue AC says warn on missing; properties and tests treat absent newlyCreatedFiles/relevantFiles as silent empty lists while optional_raw_* warns on missing keys) services/workspace_resolver.py:245-258 — narrow bubble_map to Mapping[str, Bubble | dict[str, Any]] (or Bubble only after full migration) in determine_project_for_conversation (issue #90 asks for mypy-verifiable flow from model to service; untyped dict hides misuse now that values are Bubble instances) models/raw_access.py:158-178, models/raw_access.py:181-196 — use the same missing-key/type policy in composer_headers / composer_newly_created_files dict branches as on Composer properties (dict path for fullConversationHeadersOnly returns [] with no drift log) (inconsistent drift detection between Composer instances and dict bridges in the resolver) Nice to havemodels/raw_access.py:103-126 — remove or wire up optional_raw_number (currently unused dead code) models/raw_access.py:181-253, models/conversation.py:73-246 — deduplicate property logic and composer_* / bubble_* bridge helpers into one implementation called by both (eval monolith-duplication theme; reduces fix-once-apply-twice risk on schema changes) services/workspace_tabs.py (remaining ~59 .get() calls, especially ctx.get(...) in _assemble_tab_from_composer_data) — follow-up accessors for messageRequestContext fields after the top-10 composer/bubble keys (issue #90 primary target only partly migrated; acceptable as follow-up if PR scope is intentionally bounded) |
|
@CodeRabbit, Do you think my comments review are valid? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== models/raw_access.py ==="
cat models/raw_access.py
echo ""
echo "=== models/conversation.py (relevant sections) ==="
sed -n '1,250p' models/conversation.py
echo ""
echo "=== services/workspace_resolver.py ==="
cat services/workspace_resolver.py
echo ""
echo "=== services/workspace_tabs.py (lines 860-920) ==="
sed -n '860,920p' services/workspace_tabs.py
echo ""
echo "=== Count .get() calls in workspace_tabs.py ==="
grep -n '\.get(' services/workspace_tabs.py | wc -l
echo "--- Lines with .get( in workspace_tabs.py ---"
grep -n '\.get(' services/workspace_tabs.py
echo ""
echo "=== tests/test_raw_accessors.py ==="
cat tests/test_raw_accessors.py
echo ""
echo "=== optional_raw_number usages across codebase ==="
rg -n 'optional_raw_number' --type py
echo ""
echo "=== bubble_map type annotations ==="
rg -n 'bubble_map' services/workspace_resolver.py services/workspace_tabs.py
echo ""
echo "=== determine_project_for_conversation signature ==="
rg -n 'determine_project_for_conversation' services/workspace_resolver.py -A 5
echo ""
echo "=== warn_missing_raw_key / optional_raw_list call sites for projectLayouts ==="
rg -n 'projectLayouts\|message_request_context_project_layouts' --type py
echo ""
echo "=== workspace_tabs.py full file line count ==="
wc -l services/workspace_tabs.py
echo ""
echo "=== workspace_tabs.py lines 1-100 ==="
sed -n '1,100p' services/workspace_tabs.pyLength of output: 50749
Must Fix✅ WARNING flood on absent
|
| Code path | Absent key behaviour |
|---|---|
Composer.newly_created_files |
silent return [] |
Bubble.relevant_files |
silent return [] |
optional_raw_list |
warn_missing_raw_key WARNING |
message_request_context_project_layouts |
WARNING (via optional_raw_list) |
The tests explicitly lock in the silent-empty-list behaviour via assertNoLogs("models.conversation", level="WARNING"). This directly contradicts the issue #90 acceptance criterion ("warn on missing"). Either the AC needs to be updated (documenting which fields are "absent-by-design" vs "drift-sensitive"), or the properties need to match the optional_raw_* semantics.
✅ bubble_map: dict untyped in determine_project_for_conversation (workspace_resolver.py:245-258)
Confirmed. The parameter is annotated as bare dict in the resolver:
# workspace_resolver.py line 245
bubble_map: dict,But callers pass properly typed values:
# workspace_tabs.py line 811
bubble_map: dict[str, Bubble] = {}
# workspace_tabs.py line 102
bubble_map: Mapping[str, Bubble | dict[str, Any]],The docstring still says {bubble_id: bubble_dict}, which is stale. Narrowing to Mapping[str, Bubble | dict[str, Any]] (or Mapping[str, Bubble] post-full-migration) would give mypy end-to-end coverage as #90 requires.
✅ Inconsistent drift detection in dict branches of composer_headers / composer_newly_created_files (raw_access.py:158-196)
Confirmed. The dict-path branches call .get() silently:
# composer_newly_created_files dict branch — no drift log on absent key
value = data.get("newlyCreatedFiles") if isinstance(data, dict) else None
if value is None:
return [] # silentThis is actually consistent with the Composer property (also silent when absent), but it means the bridge functions have a different contract from optional_raw_list / message_request_context_project_layouts. If the policy decision is "absent optional fields are silent", that needs to be documented and optional_raw_list (and the projectLayouts helper) must be updated. If the policy is "warn on missing", the dict bridges and properties need updating instead.
Nice to Have
✅ optional_raw_number is dead code (raw_access.py:103-126)
Confirmed. rg -n 'optional_raw_number' returns only the definition — no call sites anywhere. Composer uses its own _optional_counter helper instead. optional_raw_number can be removed or wired into _optional_counter.
✅ Property logic vs bridge-function duplication (raw_access.py:181-253, conversation.py:73-246)
Confirmed. Composer.newly_created_files and composer_newly_created_files's dict branch contain identical null-check → isinstance-check → warning → return logic. Same pattern repeated across all Bubble properties and bubble_* bridge functions. A single shared utility (the existing optional_raw_* helpers, if used consistently) would eliminate this.
✅ ~59 remaining .get() calls in workspace_tabs.py
Confirmed exactly. grep -c '\.get(' services/workspace_tabs.py = 59. Most are in _assemble_tab_from_composer_data (e.g., ctx.get("bubbleId"), ctx.get("gitStatusRaw"), m.get("inputTokens"), etc.) that are the primary #90 target. Acceptable as a follow-up if the PR scope is intentionally bounded to the top-10 composer/bubble keys.
Summary: All your "Must fix" and "Should fix" points are valid and backed by the code. The "Nice to have" items are also accurate. The most urgent one is the WARNING flood — it will make the logs unreadable in any real install with even a moderate number of conversations.
🐇 (\_/)
(•ᴥ•) Good catch on all counts.
|
I was asking you to find others yourself as well, @CodeRabbit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== utils/text_extract.py ==="
cat utils/text_extract.py
echo ""
echo "=== tests/test_blob_parsing_fuzz.py ==="
cat tests/test_blob_parsing_fuzz.py
echo ""
echo "=== models/conversation.py full ==="
cat models/conversation.py
echo ""
echo "=== models/raw_access.py full ==="
cat models/raw_access.py
echo ""
echo "=== services/workspace_tabs.py full ==="
cat services/workspace_tabs.py
echo ""
echo "=== services/workspace_resolver.py full ==="
cat services/workspace_resolver.py
echo ""
echo "=== tests/test_raw_accessors.py full ==="
cat tests/test_raw_accessors.py
echo ""
echo "=== Check for circular imports ==="
rg -n 'from models' --type py
echo "=== raw_access imports ==="
head -20 models/raw_access.py
echo ""
echo "=== bubble_timestamp_ms - any callers? ==="
rg -n 'bubble_timestamp_ms' --type py
echo ""
echo "=== HasBubbleRaw usage ==="
rg -n 'HasBubbleRaw' --type py
echo ""
echo "=== extract_text_from_bubble callers ==="
rg -n 'extract_text_from_bubble' --type py -A 2
echo ""
echo "=== Check conversation_header_bubble_id callers vs ctx.get bubbleId ==="
rg -n 'bubbleId\|bubble_id\|conversation_header_bubble_id' --type py
echo ""
echo "=== Check for assertNoLogs availability (Python 3.10+) ==="
python3 --version
echo ""
echo "=== Check composer_headers callers ==="
rg -n 'composer_headers\b' --type py
echo ""
echo "=== Check full_conversation_headers_only property ==="
rg -n 'full_conversation_headers_only' --type py -B 2 -A 5
echo ""
echo "=== Check last_updated_at / created_at on Composer ==="
rg -n 'last_updated_at\|created_at' models/conversation.py -A 2
echo ""
echo "=== Check _optional_counter definition ==="
rg -n '_optional_counter' models/conversation.py -A 8
echo ""
echo "=== Check thread safety / caching ==="
rg -n 'lru_cache\|cache\|lock\|thread' --type py
echo ""
echo "=== Check test coverage for bubble_timestamp_ms ==="
rg -n 'timestamp\|bubble_timestamp' tests/ --type py
echo ""
echo "=== Check Bubble.from_dict usage ==="
rg -n 'Bubble\.from_dict\|Composer\.from_dict' --type py -B 1 -A 3
echo ""
echo "=== Check model_name_from_config usage ==="
rg -n 'model_name_from_config' --type py -A 3
echo ""
echo "=== Check total cost fallback in workspace_tabs ==="
rg -n 'total_cost\|totalCost\|cost' services/workspace_tabs.py -A 2
echo ""
echo "=== Check workspace_resolver bubble_map annotation ==="
sed -n '240,270p' services/workspace_resolver.py
echo ""
echo "=== Check if optional_raw_number has bool guard (documented as treating bool as invalid) ==="
grep -n 'bool\|isinstance' models/raw_access.py
echo ""
echo "=== Check test_blob_parsing_fuzz type:ignore annotation ==="
grep -n 'type: ignore\|blacklist_categories' tests/test_blob_parsing_fuzz.py
echo ""
echo "=== Check missing __all__ / exports ==="
grep -n '__all__' models/raw_access.py models/conversation.py
echo ""
echo "=== Check whether composer_code_block_data dict branch returns None vs {} inconsistency ==="
grep -n 'code_block_data\|codeBlockData' models/raw_access.py models/conversation.py
echo ""
echo "=== Look for any bare except / broad exception handling ==="
rg -n 'except\s*:' --type py
echo ""
echo "=== Check workspace_tabs bubble_map population ==="
grep -n 'bubble_map' services/workspace_tabs.py -A 2
echo ""
echo "=== Check for Bubble instances where dict expected - extract_text_from_bubble call sites ==="
grep -n 'extract_text_from_bubble' services/workspace_tabs.py -B 2 -A 2
echo ""
echo "=== Check if Bubble has .raw attribute (required by HasBubbleRaw protocol) ==="
grep -n 'self\.raw\|raw:' models/conversation.py | head -30Length of output: 126842
Additional Must Fix🔴
|
| Severity | Location | Issue |
|---|---|---|
| Must | workspace_tabs.py:133, workspace_resolver.py:303 |
bubbleId WARNING flood per header |
| Must | workspace_tabs.py:288,416 |
Falsy-zero timestamp or-fallback bug |
| Must | tests/ |
Zero coverage for bubble_timestamp_ms() edge cases |
| Should | utils/text_extract.py:38 |
bubble = raw variable shadowing breaks mypy |
| Should | api/search.py:162 |
HasBubbleRaw protocol unused at migration target |
| Should | workspace_tabs.py:228-230 |
bubble.tool_results called twice |
| Should | models/conversation.py:250,282,286 |
token_count/thinking/thinking_duration_ms lack type validation |
| Should | workspace_tabs.py:617 |
messageCount counts raw headers, not renderable bubbles |
| Should | models/raw_access.py:41 |
optional_raw_value passes bool as number |
| Should | workspace_resolver.py:303-345 |
Headers iterated twice in fallback |
| Should | workspace_resolver.py:385-400 |
infer_invalid_workspace_aliases bypasses Composer.from_dict |
| NTH | models/conversation.py:316,147 |
bubble_timestamp_ms/model_name_from_config inconsistently not @property |
| NTH | models/conversation.py:188 |
Bubble.text naming footgun |
🐇 (_/)
(•ᴥ•) Found a fair bit more than I initially surfaced — thanks for the push.
|
bubble_map typing in determine_project_for_conversation issue is existed yet. |
|
bubble_timestamp_ms() test coverage test_bubble_timestamp_ms_prefers_created_at_and_handles_zero covers: createdAt=0, prefer createdAt over timestamp, neither key → None, createdAt=True → None (no log). Gap: no test that _bubble_entry_timestamp_ms keeps epoch 0 (does not fall back to now()). |
|
token_count / thinking / thinking_duration_ms drift token_count and thinking_duration_ms have type guards + warnings. thinking still returns raw .get("thinking") with no wrong-type warning (only "thinking" not in self.raw → None). |
|
@CodeRabbit. I found 3 should fixs are not addressed fully. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== infer_invalid_workspace_aliases signature and bubble_map usage ==="
rg -n 'infer_invalid_workspace_aliases\|bubble_map' services/workspace_resolver.py -A 3 -B 1
echo ""
echo "=== bubble_timestamp_ms tests ==="
rg -n 'bubble_timestamp_ms\|_bubble_entry_timestamp\|epoch\|keeps.*0\|zero.*fallback' tests/ --type py -A 5 -B 2
echo ""
echo "=== thinking property in models/conversation.py ==="
rg -n 'def thinking\|thinking_duration\|token_count' models/conversation.py -A 10
echo ""
echo "=== workspace_resolver.py lines 370-410 ==="
sed -n '370,415p' services/workspace_resolver.py
echo ""
echo "=== Check current state of bubble_map annotation at determine_project_for_conversation ==="
sed -n '240,260p' services/workspace_resolver.py
echo ""
echo "=== Check all bubble_map: dict occurrences ==="
rg -n 'bubble_map\s*:' --type py
echo ""
echo "=== falsy-zero timestamp or-fallback - current state ==="
rg -n 'bubble_timestamp_ms\(\)\|to_epoch_ms.*or\b' services/workspace_tabs.py -A 1 -B 1
echo ""
echo "=== tool_results double call ==="
sed -n '225,235p' services/workspace_tabs.py
echo ""
echo "=== optional_raw_value bool-as-number check ==="
sed -n '35,55p' models/raw_access.py
echo ""
echo "=== double-header iteration in fallback ==="
sed -n '295,360p' services/workspace_resolver.py
echo ""
echo "=== api/search.py extract_text_from_bubble call ==="
rg -n 'extract_text_from_bubble' --type py -A 2 -B 2
echo ""
echo "=== extract_text_from_bubble variable shadowing ==="
sed -n '30,55p' utils/text_extract.py
echo ""
echo "=== optional_raw_number call sites ==="
rg -n 'optional_raw_number' --type py
echo ""
echo "=== messageCount in summaries ==="
rg -n 'messageCount' services/workspace_tabs.py -A 2 -B 2
echo ""
echo "=== bubble_timestamp_ms and model_name_from_config - `@property` or method ==="
rg -n 'def bubble_timestamp_ms\|def model_name_from_config' models/conversation.py -B 2
echo ""
echo "=== Bubble.text property ==="
rg -n 'def text\b\|plain_text' models/conversation.py -A 5
echo ""
echo "=== bubbleId WARNING flood - conversation_header_bubble_id calls in bulk loops ==="
sed -n '125,145p' services/workspace_tabs.py
sed -n '295,315p' services/workspace_resolver.pyLength of output: 20890
Your 3 Corrections — All Confirmed1.
|
| Item | Status |
|---|---|
WARNING flood on projectLayouts/bubbleId |
warn_if_missing: bool = True param added to optional_raw_value — mechanism exists; depends on call sites passing False |
Falsy-zero or-fallback bug (workspace_tabs.py:288,416) |
rg found no matches for that pattern — likely fixed |
extract_text_from_bubble variable shadowing |
✅ Fixed — uses payload variable now |
api/search.py:162 HasBubbleRaw unused |
✅ Resolved — Bubble satisfies the protocol, call site now correct |
optional_raw_value passes bool as number |
✅ Fixed — explicit isinstance(value, bool) pre-check added |
| Double-header iteration in fallback | ✅ Fixed — now single-pass with comment |
messageCount counts raw headers |
✅ Documented via inline comment; not algorithmically changed (acceptable) |
infer_invalid_workspace_aliases bypasses Composer.from_dict |
❌ Still passes raw json.loads dict; bubble_map: dict annotation also unfixed (same as #1) |
optional_raw_number dead code |
❌ Still no call sites |
bubble.tool_results called twice |
Could not confirm from output — worth a targeted check |
| Missing-key policy docs ("absent-by-design" vs "drift-sensitive") | ❌ Still undocumented |
So the remaining open items are: infer_invalid_workspace_aliases (both the bubble_map annotation and the missing Composer.from_dict call), optional_raw_number dead code, thinking wrong-type warning, epoch-0 test gap, and the missing-key policy documentation from the original "should fix" list.
🐇 (\_/)
(•ᴥ•) Good catch on all three — my initial analysis had real inaccuracies.
|
@coderabbitai, Check again if the three findings are fixed or not. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== 1. infer_invalid_workspace_aliases bubble_map annotation ==="
rg -n 'infer_invalid_workspace_aliases\|bubble_map\s*:' services/workspace_resolver.py -A 3 -B 1
echo ""
echo "=== determine_project_for_conversation signature ==="
rg -n 'def determine_project_for_conversation' services/workspace_resolver.py -A 8
echo ""
echo "=== 2. bubble_timestamp_ms / _bubble_entry_timestamp tests ==="
rg -n 'bubble_timestamp_ms\|_bubble_entry_timestamp\|epoch.*0\|zero.*epoch\|keeps.*0\|timestamp.*0' tests/ --type py -A 6 -B 2
echo ""
echo "=== All tests related to timestamp ==="
cat tests/test_raw_accessors.py
echo ""
echo "=== 3. thinking property current state ==="
rg -n 'def thinking\b' models/conversation.py -A 15
echo ""
echo "=== token_count and thinking_duration_ms for comparison ==="
rg -n 'def token_count\|def thinking_duration_ms' models/conversation.py -A 15
echo ""
echo "=== infer_invalid_workspace_aliases full function ==="
sed -n '370,420p' services/workspace_resolver.py
echo ""
echo "=== Check for any test files mentioning epoch 0 or timestamp zero ==="
rg -n 'epoch\|timestamp.*0\|0.*timestamp\|createdAt.*0\|0.*createdAt' tests/ --type py -A 3 -B 1Length of output: 11896
1.
|
bradjin8
force-pushed
the
feat/typed-accessor-methods
branch
from
709f838 to
01ea37c
Compare
3 participants
Summary
models/raw_access.pywith shared optional-key readers and schema-driftWARNINGlogging when structurally important keys are missing or mistyped (projectLayouts,bubbleIdin headers, etc.).ComposerandBubble(newly_created_files,relevant_files,usage_data,bubble_timestamp_ms(), and related fields) so services can use model fields instead of silent.get()chains afterfrom_dictvalidation.services/workspace_tabs.pyandservices/workspace_resolver.pyto passComposer/Bubblethrough the hot path (bubble_mapstoresBubbleinstances;determine_project_for_conversationacceptsComposer | dict).tests/test_raw_accessors.pyfor drift logging and bridge helpers.Closes #90
Test plan
python -m pytest tests/test_raw_accessors.py -vpython -m pytest tests/test_workspace_assignment_fallback.py tests/test_models.py -vpython -m pytest tests/ -q(371 passed)python -m mypy models services/workspace_tabs.py services/workspace_resolver.pynewlyCreatedFiles,relevantFiles)Notes
.get()usage inworkspace_listing.py,api/search.py, andscripts/export.py.Summary by CodeRabbit
New Features
Bug Fixes
Tests