fix(messages): preserve forward snapshots and attachment uid analysis#78
Merged
Conversation
Co-authored-by: GPT-5 Codex <noreply@openai.com>
Co-authored-by: GPT-5 Codex <noreply@openai.com>
Co-authored-by: GPT-5 Codex <noreply@openai.com>
Co-authored-by: GPT-5 Codex <noreply@openai.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughVersion 3.6.3 adds forward snapshot caching, snapshot-first forward retrieval, attachment UID resolution in multimodal analysis, a history deduplication ordering change, version bumps, and related docs/tests updates. ChangesForward snapshot, attachment, and release flow
Sequence Diagram(s)sequenceDiagram
participant MessageHandler
participant register_message_attachments
participant snapshot_forward_tree
participant get_forward_msg_handler
participant load_forward_snapshot
participant save_forward_snapshot
participant get_forward_messages
MessageHandler->>register_message_attachments: snapshot_forward_messages=True
register_message_attachments->>snapshot_forward_tree: scope_key, forward_id, get_forward_messages
snapshot_forward_tree->>load_forward_snapshot: lookup cached tree
alt cache miss
load_forward_snapshot-->>snapshot_forward_tree: empty
snapshot_forward_tree->>get_forward_messages: fetch forward nodes
get_forward_messages-->>snapshot_forward_tree: nodes
snapshot_forward_tree->>save_forward_snapshot: persist snapshot
end
get_forward_msg_handler->>load_forward_snapshot: scope_key, forward_id
alt snapshot hit
load_forward_snapshot-->>get_forward_msg_handler: cached nodes
else snapshot miss
get_forward_msg_handler->>get_forward_messages: fetch forward nodes
get_forward_messages-->>get_forward_msg_handler: nodes or error
alt fetch fails
get_forward_msg_handler-->>get_forward_msg_handler: structured unavailable response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Undefined/attachments/segments.py (1)
369-383: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWire
snapshot_nested_forward_messagesinto the snapshot decision.Line 370 adds a documented compatibility alias, but the body never reads it, so callers passing only
snapshot_nested_forward_messages=Trueget a silent no-op instead of snapshotting.Proposed fix
visited_forward_ids: set[str] = set() + should_snapshot_forward_messages = ( + snapshot_forward_messages or snapshot_nested_forward_messages + )Then use
should_snapshot_forward_messagesin the forward branch:- snapshot_forward_messages + should_snapshot_forward_messages @@ - if snapshot_forward_messages: + if should_snapshot_forward_messages:🤖 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 `@src/Undefined/attachments/segments.py` around lines 369 - 383, The compatibility alias `snapshot_nested_forward_messages` is documented in `scan_message_segments` but never used, so forwarding-only callers see no snapshot behavior. Update the forward-message snapshot decision in `scan_message_segments` (or the existing `should_snapshot_forward_messages` helper/branch) to treat `snapshot_nested_forward_messages=True` the same as `snapshot_forward_messages=True`, so either flag enables snapshotting.
🤖 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 `@docs/deployment.md`:
- Around line 5-6: The blockquote in the deployment docs is broken by a blank
line, which triggers MD028; update the quoted text so it remains one continuous
blockquote. In the affected markdown section, keep the “Release 下载提示” paragraph
and the following sentence in the same quote format without any empty line
between them, and ensure the blockquote syntax is consistent throughout
docs/deployment.md.
In `@src/Undefined/attachments/forward_snapshot.py`:
- Around line 24-25: The per-snapshot lock cache in _snapshot_locks is unbounded
and never evicted, unlike _snapshot_inflight, so update the snapshot
coordination flow in forward_snapshot.py to clean up locks after the last
in-flight task for a given key completes. Make the cleanup happen in the same
code paths that create and await the per-snapshot lock/task entries so the dict
cannot retain one asyncio.Lock per unique snapshot forever, while preserving the
existing locking behavior around _snapshot_locks and _snapshot_inflight.
In `@src/Undefined/attachments/segments.py`:
- Around line 599-630: The forward attachment loading in snapshot mode can leave
`forward_nodes` empty after `load_forward_snapshot`, which prevents the direct
resolver from ever running. Update the logic in the attachment expansion path
around `snapshot_forward_tree`, `load_forward_snapshot`, and
`_fetch_forward_nodes` so that an empty or unreadable snapshot falls back to
`_fetch_forward_nodes(forward_id)` before relying on `visited_forward_ids`. Keep
the visited check only for preventing repeated resolution, not for suppressing
the fallback when no nodes were loaded.
In
`@src/Undefined/skills/agents/file_analysis_agent/tools/analyze_multimodal/handler.py`:
- Line 6: The multimodal handler currently imports and uses a repo-local
`scope_from_context` fallback, which violates the skills boundary and bypasses
the intended injected scope flow. Remove the direct `Undefined.attachments`
dependency from the handler and rely only on `context`-provided `scope_key` or
`get_scope_from_context` inside the `analyze_multimodal`/handler path, letting
the existing “缺少会话作用域” error branch cover missing wiring.
In `@tests/test_file_analysis_attachment_uid.py`:
- Around line 237-239: The async test in test_file_analysis_attachment_uid is
still doing blocking disk checks via Path.is_file() and Path.read_bytes() on the
event loop. Update that assertion block to perform the file existence/read
verification through asyncio.to_thread(...) or the repo’s async I/O helper,
keeping the same checks but avoiding synchronous filesystem calls in this test.
In `@tests/test_forward_snapshot.py`:
- Around line 28-32: The forward snapshot tests are leaving module-level state
behind when patching the cache directory, which makes cases order-dependent. In
each test that monkeypatches FORWARD_SNAPSHOT_CACHE_DIR, also reset the
forward_snapshot module’s _snapshot_locks and _snapshot_inflight state before
using forward_snapshot helpers so reused forward IDs don’t collide across tests;
apply this consistently in the affected test cases that reference
forward_snapshot and its snapshot cache setup.
---
Outside diff comments:
In `@src/Undefined/attachments/segments.py`:
- Around line 369-383: The compatibility alias
`snapshot_nested_forward_messages` is documented in `scan_message_segments` but
never used, so forwarding-only callers see no snapshot behavior. Update the
forward-message snapshot decision in `scan_message_segments` (or the existing
`should_snapshot_forward_messages` helper/branch) to treat
`snapshot_nested_forward_messages=True` the same as
`snapshot_forward_messages=True`, so either flag enables snapshotting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a3a6a38b-f8a2-4137-8936-c206c8b1b752
⛔ Files ignored due to path filters (5)
apps/undefined-chat/package-lock.jsonis excluded by!**/package-lock.jsonapps/undefined-chat/src-tauri/Cargo.lockis excluded by!**/*.lockapps/undefined-console/package-lock.jsonis excluded by!**/package-lock.jsonapps/undefined-console/src-tauri/Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
CHANGELOG.mdREADME.mdapps/undefined-chat/package.jsonapps/undefined-chat/src-tauri/Cargo.tomlapps/undefined-chat/src-tauri/tauri.conf.jsonapps/undefined-console/package.jsonapps/undefined-console/src-tauri/Cargo.tomlapps/undefined-console/src-tauri/tauri.conf.jsondocs/app.mddocs/build.mddocs/configuration.mddocs/deployment.mddocs/pipelines.mddocs/usage.mdpyproject.tomlsrc/Undefined/__init__.pysrc/Undefined/ai/prompts/current_input.pysrc/Undefined/attachments/__init__.pysrc/Undefined/attachments/forward_snapshot.pysrc/Undefined/attachments/segments.pysrc/Undefined/handlers/message_flow.pysrc/Undefined/skills/agents/file_analysis_agent/README.mdsrc/Undefined/skills/agents/file_analysis_agent/prompt.mdsrc/Undefined/skills/agents/file_analysis_agent/tools/analyze_multimodal/handler.pysrc/Undefined/skills/toolsets/messages/get_forward_msg/handler.pysrc/Undefined/utils/paths.pytests/test_attachments.pytests/test_file_analysis_attachment_uid.pytests/test_forward_snapshot.pytests/test_get_forward_msg_tool.pytests/test_prompt_builder_cognitive_query.py
Co-authored-by: GPT-5 Codex <noreply@openai.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
messages.get_forward_msgfall back to local snapshots when OneBot cannot re-read nested forwards.file_analysis_agentmultimodal analysis to resolve internalpic_/file_attachment UIDs in the current session scope.Tests
ruff format --check,ruff check,mypySummary by CodeRabbit