fix(handoff): dedicated handoff MCP tool with required recipient - #153
Merged
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe MCP server now supports recipient-directed handoffs with validated, trimmed metadata and runtime sender identity. Prompt guidance and tests map recipient briefs to the new ChangesAgent handoff flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HandoffCaller
participant MCPServer
participant ArtifactStore
participant RecipientInbox
HandoffCaller->>MCPServer: call handoff with recipient, name, and content
MCPServer->>MCPServer: validate and trim required fields
MCPServer->>ArtifactStore: publish recipient-routed artifact with sender identity
ArtifactStore->>RecipientInbox: store artifact in recipient inbox
MCPServer-->>HandoffCaller: return artifact metadata and recipient
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (1)
crates/agentflare-artifacts/src/lib.rs (1)
482-513: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the post-
keep()update path.These cases only publish new artifacts. Add an update that supplies
senderwhile omittingrecipient, so the inheritedNoneis verified to produceInvalidInputand leave the existing artifact unchanged.🤖 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 `@crates/agentflare-artifacts/src/lib.rs` around lines 482 - 513, The test publish_rejects_sender_without_recipient currently covers only new artifacts; extend it with an update to an existing artifact using keep() that sets sender while omitting recipient. Assert the update returns InvalidInput and verify the original artifact remains unchanged, including its existing recipient/content or other relevant fields.
🤖 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 `@crates/agentflare-artifacts/src/lib.rs`:
- Around line 499-512: Strengthen the whitespace-recipient rejection test around
publish: assert that the returned error is specifically the expected
InvalidInput kind rather than only checking is_err(). Replace the
store.list(None) assertion with direct inspection of store.base_path(),
confirming no files or directories were created after rejection.
---
Nitpick comments:
In `@crates/agentflare-artifacts/src/lib.rs`:
- Around line 482-513: The test publish_rejects_sender_without_recipient
currently covers only new artifacts; extend it with an update to an existing
artifact using keep() that sets sender while omitting recipient. Assert the
update returns InvalidInput and verify the original artifact remains unchanged,
including its existing recipient/content or other relevant fields.
🪄 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: 11bc70a6-30d4-443f-8767-92e42ad4291b
📒 Files selected for processing (2)
crates/agentflare-artifacts/src/lib.rscrates/agentflare-artifacts/src/store.rs
Comment on lines
+499
to
+512
| // A blank/whitespace recipient is treated the same as missing. | ||
| assert!(store | ||
| .publish(&PublishRequest { | ||
| name: "orphan-handoff".into(), | ||
| content: "for claude".into(), | ||
| session_id: "s".into(), | ||
| sender: Some("opencode".into()), | ||
| recipient: Some(" ".into()), | ||
| ..Default::default() | ||
| }) | ||
| .is_err()); | ||
|
|
||
| // Nothing was written on rejection. | ||
| assert!(store.list(None).unwrap().is_empty()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the blank-recipient error and filesystem state.
The whitespace case only checks is_err(), so it could accept the wrong error kind. list(None).is_empty() also misses orphan files/directories without valid metadata. Assert InvalidInput and inspect store.base_path() directly.
Proposed test strengthening
- assert!(store
+ let blank_err = store
.publish(&PublishRequest {
name: "orphan-handoff".into(),
content: "for claude".into(),
session_id: "s".into(),
sender: Some("opencode".into()),
recipient: Some(" ".into()),
..Default::default()
})
- .is_err());
+ .unwrap_err();
+ assert_eq!(blank_err.kind(), std::io::ErrorKind::InvalidInput);
- assert!(store.list(None).unwrap().is_empty());
+ assert!(std::fs::read_dir(store.base_path()).unwrap().next().is_none());📝 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
| // A blank/whitespace recipient is treated the same as missing. | |
| assert!(store | |
| .publish(&PublishRequest { | |
| name: "orphan-handoff".into(), | |
| content: "for claude".into(), | |
| session_id: "s".into(), | |
| sender: Some("opencode".into()), | |
| recipient: Some(" ".into()), | |
| ..Default::default() | |
| }) | |
| .is_err()); | |
| // Nothing was written on rejection. | |
| assert!(store.list(None).unwrap().is_empty()); | |
| // A blank/whitespace recipient is treated the same as missing. | |
| let blank_err = store | |
| .publish(&PublishRequest { | |
| name: "orphan-handoff".into(), | |
| content: "for claude".into(), | |
| session_id: "s".into(), | |
| sender: Some("opencode".into()), | |
| recipient: Some(" ".into()), | |
| ..Default::default() | |
| }) | |
| .unwrap_err(); | |
| assert_eq!(blank_err.kind(), std::io::ErrorKind::InvalidInput); | |
| // Nothing was written on rejection. | |
| assert!(std::fs::read_dir(store.base_path()).unwrap().next().is_none()); |
🤖 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 `@crates/agentflare-artifacts/src/lib.rs` around lines 499 - 512, Strengthen
the whitespace-recipient rejection test around publish: assert that the returned
error is specifically the expected InvalidInput kind rather than only checking
is_err(). Replace the store.list(None) assertion with direct inspection of
store.base_path(), confirming no files or directories were created after
rejection.
Handoffs were published via artifact_publish, whose recipient is an optional field — an intended handoff published without one lands in no inbox (artifact_list filters on recipient) and vanishes silently, as an opencode handoff to claude-code did. Add a dedicated `handoff` MCP tool whose recipient is a required, non-Option field, so the schema itself makes an unaddressed handoff unrepresentable. It sets sender to the runtime's own identity and routes to the recipient's inbox. The handoff prompt now directs <recipient> <brief> to this tool. artifact_publish is unchanged and stays for plain shareable pages (a store-layer guard can't tell the two apart because sender is auto-defaulted).
getappz
force-pushed
the
fix/handoff-requires-recipient
branch
from
July 11, 2026 14:09
26893f3 to
09356d9
Compare
getappz
changed the title
…defeat exact-match inbox lookups
This was referenced Jul 13, 2026
getappz
added a commit
that referenced
this pull request
Jul 18, 2026
Merged
6 tasks
getappz
added a commit
that referenced
this pull request
Jul 18, 2026
…r+hybrid), blobs, leases (#244) * feat: rebrand embedded skill cards ponytail→flare + FLARE_OUTPUT_MODEL env-var * chore: uninstall caveman — remove from KNOWN_COMPRESSION_PLUGINS, update backup path and prompt * feat(agentflare-store): initial crate - kv, documents (CRUD+FTS+vector), blobs, leases Imports the agentflare-store implementation for item #147, previously left as uncommitted working-tree state with zero git history. Store skeleton, kv, documents CRUD/FTS5/vector search/hybrid fusion, blob storage, and a leases re-export of db_kit::claim. Wires the crate into the workspace members list. Applies three plan-mandated fixes during import: drop deps unused anywhere in the crate (agentflare-artifacts, sha2, hex, uuid, chrono, mime_guess), use BLAKE3 instead of SHA-256 for blob hashing per the plan's global constraints, and a clippy nit in the hybrid-search chain (redundant into_iter()). Verified: cargo build --workspace --all-features, cargo clippy -p agentflare-store -- -D warnings, cargo test --workspace (27/27 in this crate, full workspace suite green). Known gaps vs the 8-task plan, tracked as follow-up work: - embeddings module is a 50-line stub (cosine_similarity/normalize only) - Task 5's vendoring of lean-ctx's model/tokenizer/download pipeline never happened, so doc_vec_search/doc_hybrid_search are untestable end-to-end - blob storage chunks bytes into SQLite rather than content-addressed files on disk under a blobs dir, as the plan specified - documents schema is missing title/doc_type/blob_hash/mime/tags/session_id/ source/version/history and has no versioning API - Store holds a bare rusqlite::Connection (not parking_lot::Mutex-wrapped), so it isn't Sync and can't be shared across threads as the plan's Arc<Store> design requires - Task 8 (migrate state.json onto this crate's kv store) hasn't been started; src/state.rs is untouched and agentflare-store isn't yet a dependency of the main package * feat(agentflare-store): initial crate - kv, documents (CRUD+FTS+vector), blobs, leases * fix(agentflare-store): wrap doc upsert in a transaction, preserve blob_hash/mime in history, fix broken embedding dimension probe - doc_upsert_with_opts issued ~8 unwrapped statements against a WAL-mode, multi-process-shared SQLite file; wrap the whole upsert in a transaction so concurrent readers/crashes can't observe a half-written document. - store_doc_history never captured blob_hash/mime, so doc_history()/ doc_get_version() always returned blank metadata for old versions; capture and store them, add a regression test. - doc_vec_search silently swallowed row-decode errors via filter_map(Result::ok), making a real DB error look like zero matches; propagate them instead. - detect_dimensions() passed a remote https:// model URL into ort's commit_from_file (which expects a local path), so EmbeddingEngine::load() always failed when the embeddings feature was used; pass the local model path that load_model() already resolves. - removed dead .meta sidecar file writes/deletes in blobs.rs (never read), a duplicate is_bert_punctuation() in embedding_pipeline/mod.rs, a no-op if/else in embed() with identical branches, and two warnings (unused import, unused mut). * fix(agentflare-store): dedupe normalize, cargo fmt, clippy clean - embedding_pipeline::pooling::normalize_l2/l2_norm duplicated embed::normalize with a looser epsilon (f32::EPSILON vs 1e-12) and l2_norm had zero callers. Removed both; embed::normalize is now the single L2-normalize implementation, used by embed() and embed_query(). - cargo fmt across the crate (documents.rs, mod.rs, model_registry.rs, pooling.rs, kv.rs -- download.rs/tokenizer.rs left as-is, in-flight elsewhere). - clippy -D warnings clean: dead model_id field (added a model_id() accessor alongside the existing dimensions() getter), a manual index loop over sum flagged by clippy::needless_range_loop, and a let-and-return in model_directory(). * fix(agentflare-store): dispatch tokenizer by model.type, implement BPE decoding * fix(agentflare-store): re-verify size+SHA-256 of already-present model files * fix(agentflare-store): cargo fmt download.rs after merging #153's fix * feat(agentflare-store): wire state.json onto the store's kv table (Task 8) src/store.rs::open() previously cached the store behind a OnceLock -- dead code (nothing called it) and would have broken test isolation the moment something did, since AGENTFLARE_HOME_OVERRIDE changes per-test but a cached singleton would keep pointing at whichever home dir opened it first. Open fresh per call instead, matching memory::store::open()'s existing pattern in this codebase. state::load()/save() now read/write through agentflare-store's kv table under the active/version_cache keys, with a one-time import of any legacy state.json via agentflare_store::migrate::migrate_state_json on first load against a store that has neither key yet. Public API unchanged (State, load(), save(), state_path()), so no callers change. * fix(agentflare-store): stop colliding with db.rs's agentflare.db, use store.db store_path() picked ~/.agentflare/agentflare.db -- the same file src/db.rs already owns as its single source-of-truth relational store (claims, handoffs, review_findings, gateway_secrets), with its own separate migration list. Two independent migration systems targeting one file is a real hazard, not just an aesthetic collision. agentflare-store is a different kind of storage (blobs, FTS+vector docs, kv) so a separate file is correct -- it just needed a name that doesn't collide. * fix(agentflare-store): make blob/doc/migrate writes atomic and race-free - blob_store/blob_unref: wrap the exists-check + insert/decrement + cascade in an Immediate transaction so concurrent connections can't race on the same hash; compensate for orphaned disk writes on failure. - doc_upsert_with_opts: open the transaction before reading the current version so concurrent upserts can't both read version N and both write N+1; also makes new-document insert + FTS sync atomic. - doc_hard_delete: delete history/vector/FTS rows before the parent row, all in one transaction. - migrate_state_json: one transaction for every key plus the completion marker, so a partial failure can't desync the marker from what migrated. * fix(agentflare-store): surface errors instead of silently swallowing them - cosine_similarity: return None on mismatched vector lengths instead of letting zip() silently drop the excess coordinates. - blob_get: propagate disk read I/O errors (permissions, disk failure) instead of folding them into 'blob not found'. - read_lockfile: distinguish a missing lockfile (fine, first run) from an unreadable or malformed one (error) instead of treating both as 'no pins', which made every present file look unverified and skip the tamper check. - ensure_model: abort if a corrupt model file can't be deleted, instead of proceeding while the corrupt file is still on disk. - resolve_model: error on an invalid AGENTFLARE_EMBEDDING_MODEL instead of silently falling back to the default model. * fix(agentflare-store): collision-resistant model slug, name-based ONNX I/O, correct WordPiece fallback - CustomModelSpec::storage_slug: hash repo+revision into the cache dir name instead of truncating the revision to 16 chars, which let two different (repo, revision) pairs collide on the same cache directory. - EmbeddingEngine::load_model: resolve ONNX input/output tensors by declared name (with a positional fallback for the two inputs every model has) instead of blind index 0/1/2, which breaks if a model export ever reorders its inputs/outputs. - wordpiece_encode: an unsegmentable word now collapses to a single [UNK], matching standard WordPiece, instead of one UNK per unmatched character interleaved with whatever subwords did match. - vec_search_ranks_by_similarity test: use directionally distinct embeddings; the old ones were collinear and scored identically regardless of magnitude, so the test wasn't exercising rank order. * fix(flare-output): treat blank model env vars as unset, keep pre-rename backups discoverable - call_via_api: a blank FLARE_OUTPUT_MODEL/CAVEMAN_MODEL value no longer wins over the next fallback in the chain. - backup_path_for (OutOfTree): the backup namespace directory was renamed from 'caveman' to 'flare-output' with a hard rename and no fallback, so a user's pre-rename backups became invisible to both the 'backup already exists' guard and Report::original_path. Now resolves to an existing legacy path if the new one doesn't have one yet. * fix(flare-output): build legacy caveman test path by component, not string replace
This was referenced Aug 18, 2026
Closed
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.
Problem
A handoff routes a work product to another agent's inbox —
artifact_listfilters on therecipientenvelope field. Handoffs were published throughartifact_publish, whererecipientis optional. An intended handoff published without one lands in no inbox and vanishes silently — exactly what happened to an opencode → claude-code handoff (the "for Claude" intent lived only in the free-text name, not a routing field).Why not a store-layer guard
The first attempt rejected
publish()whensenderwas set butrecipientwas missing. That was wrong:mcp_server.rsauto-defaultssenderto the agent identity on every publish, so a plain/artifact publish(a shared design doc, a review) also hassenderset and legitimately no recipient. The guard rejected those too, breaking the normal publish path (caught byartifact_publish_defaults_sender_to_agent_identity). At the store layer, "intended handoff, forgot recipient" and "plain publish" are indistinguishable.Fix: make it unrepresentable
Add a dedicated
handoffMCP tool whoserecipientis a required, non-Optionfield — the JSON schema itself rejects an unaddressed handoff before it reaches the store. The tool setssenderto the runtime's own identity and routes to the recipient's inbox. The handoff prompt now directs<recipient> <brief>to this tool instead of rawartifact_publish.artifact_publishis unchanged — plain shareable pages keep working, sender-default intact.Tests
handoff_tool_requires_recipient_and_routes_to_inbox— blank recipient →INVALID_PARAMS; a real handoff lands in the recipient's inbox with the correct sender.artifact_publish_defaults_sender_to_agent_identitypasses again (regression gone).Summary by CodeRabbit