Conversation
- Merge skill/search+load, gateway/search+execute, claim/5-actions, review/5-actions, artifact/6-actions, memory/6-actions into one consolidated #[tool(...)] method each with action dispatch - Reconcile with #182 (fetch target branch before branching) and #183 (deferred claim release, mark_completed split)
|
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)
📝 WalkthroughWalkthroughClaim completion now keeps leases active until explicit release, and ownership checks require active claims. MCP tools are consolidated into action-based skill, artifact, claim, review, gateway, and memory interfaces with per-action validation and dispatch. ChangesClaim lifecycle and MCP tool consolidation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant AgentflareMcp
participant BackendHandlers
MCPClient->>AgentflareMcp: Submit action-based MCP request
AgentflareMcp->>AgentflareMcp: Validate action-specific fields
AgentflareMcp->>BackendHandlers: Dispatch skill, artifact, claim, review, tool, or memory action
BackendHandlers-->>AgentflareMcp: Return result or mapped error
AgentflareMcp-->>MCPClient: Return structured response
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/agentflare-backend/src/item.rs`:
- Around line 442-456: The mark_completed flow must prevent a lease owner change
between is_owner and update_state. Make the ownership predicate atomic with the
completion write, or wrap the get/state lookup, ownership validation, and
update_state operations in a transaction that preserves the check through the
write; retain the existing Ok(false) behavior for callers who do not own the
claim.
In `@src/mcp_server.rs`:
- Around line 266-335: Add a dedicated optional sub-action field to
MemoryRequest for curate operations, documenting the allowed
update/delete/pin/unpin values. In the curate dispatch logic around
handle_curate, populate CurateInput.action from this new field rather than
req.action, and validate or reject missing sub-actions consistently with
handle_curate’s supported operations.
- Around line 1570-1581: Remove the unused base and head locals from the
"record" handler. Pass req.base.as_deref() and req.head.as_deref() directly to
compute_diff, preserving its existing master/HEAD defaults and avoiding the
partial move of req.
🪄 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: bd994882-c605-470d-99df-7d82f402e73b
📒 Files selected for processing (3)
crates/agentflare-backend/src/item.rscrates/agentflare-db-kit/src/claim.rssrc/mcp_server.rs
…tion regressions CI was red: ~35 test call sites in mcp_server.rs still called the old per-action methods (artifact_publish/get/list/delete/diff/search, skill_search/load, gateway_search/execute) that this consolidation removed. Migrated them all to the unified action-dispatch tools. Also fixes real bugs the consolidation introduced along the way: - artifact()'s get/diff/search arms called store methods with a signature they no longer have (get takes no version, diff's to is a required u32, and there's no store.search at all) — restored the pre-consolidation logic (get_version fallback, latest-version resolution, inline list+filter+snippet search). - review()'s "consensus" arm called crate::review::consensus with the wrong arguments entirely (a Connection/ids instead of findings+changed-lines); now loads findings and computes the diff/changed-lines first, matching "record"'s already-correct pattern. Also dropped record's dead base/head locals that shadowed the actual req.base/req.head usage. - memory()'s "curate" arm passed the outer "curate" action into CurateInput.action, so update/delete/pin/unpin could never be reached; added a dedicated curate_action field. - mark_completed's ownership check and completion write weren't in the same transaction, leaving a window for a concurrent release+claim by a different owner to complete the item out from under its new owner. Also ran cargo fmt across the branch (had never been formatted).
# Conflicts: # src/mcp_server.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components.rs`:
- Around line 50-69: The cleanup flow around remove_claude_mcp_server and the
lean-ctx register(&LEANCTX) caller must preserve user-owned entries and avoid
cleanup after failed registration. Track whether the existing mcpServers[name]
entry is installer-owned, make registration return a typed success result that
reflects write failure, and invoke removal only after confirmed successful
registration and only for the installer-created entry.
- Around line 357-363: Update the stale skill-tool descriptions in the component
list, specifically the entries advertising skill_search and skill_load, to
describe the consolidated action-based MCP skill contract. Ensure init output no
longer directs users to the legacy standalone tool names, while preserving the
surrounding component descriptions and terminology.
🪄 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: c7f70300-74cf-42fb-9309-3c587cb1042b
📒 Files selected for processing (4)
src/components.rssrc/gateway_integrations.rssrc/mcp_server.rssrc/rule_text.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mcp_server.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components.rs`:
- Around line 50-69: The cleanup flow around remove_claude_mcp_server and the
lean-ctx register(&LEANCTX) caller must preserve user-owned entries and avoid
cleanup after failed registration. Track whether the existing mcpServers[name]
entry is installer-owned, make registration return a typed success result that
reflects write failure, and invoke removal only after confirmed successful
registration and only for the installer-created entry.
- Around line 357-363: Update the stale skill-tool descriptions in the component
list, specifically the entries advertising skill_search and skill_load, to
describe the consolidated action-based MCP skill contract. Ensure init output no
longer directs users to the legacy standalone tool names, while preserving the
surrounding component descriptions and terminology.
🪄 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: c7f70300-74cf-42fb-9309-3c587cb1042b
📒 Files selected for processing (4)
src/components.rssrc/gateway_integrations.rssrc/mcp_server.rssrc/rule_text.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mcp_server.rs
🛑 Comments failed to post (2)
src/components.rs (2)
50-69: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve user-owned entries and gate cleanup on registration success.
remove_claude_mcp_serverdeletesmcpServers[name]solely by key. The caller then removeslean-ctxafterregister(&LEANCTX)without checking for a write failure (Lines 383–387), so failed registration—or a manually configured entry—can leave Claude with no working lean-ctx server. Return typed registration success and remove only an installer-owned entry after confirmed registration.🤖 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/components.rs` around lines 50 - 69, The cleanup flow around remove_claude_mcp_server and the lean-ctx register(&LEANCTX) caller must preserve user-owned entries and avoid cleanup after failed registration. Track whether the existing mcpServers[name] entry is installer-owned, make registration return a typed success result that reflects write failure, and invoke removal only after confirmed successful registration and only for the installer-created entry.
357-363: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update the stale skill-tool descriptions alongside this terminology change.
The same component list still advertises
skill_search/skill_loadat Lines 395–407. After consolidating skill operations into an action-based MCP tool, init output will direct users to legacy tool names. Update those descriptions to the consolidated skill action contract.🤖 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/components.rs` around lines 357 - 363, Update the stale skill-tool descriptions in the component list, specifically the entries advertising skill_search and skill_load, to describe the consolidated action-based MCP skill contract. Ensure init output no longer directs users to the legacy standalone tool names, while preserving the surrounding component descriptions and terminology.
This was referenced Jul 15, 2026
getappz
added a commit
that referenced
this pull request
Jul 20, 2026
* wip(asset-store): migrate asset storage onto agentflare-store documents+blobs (#185) Replace agentflare-backend::asset's storage with agentflare-store's documents+blobs (content-addressed blob store with dedup/unref), and route handoff/asset MCP tool handlers through it. * fix(asset-store): deleted assets stay gettable, backfill can be skipped forever - Store::doc_get() didn't filter deleted_at, so asset get/delete on an already-deleted id returned 200 instead of not-found (regression from agentflare_backend::asset::get, worse now that delete also physically purges the blob once ref_count hits 0 -- soft-deleted docs pointed at already-gone content). - with_store()'s one-time legacy backfill only fires when backend_db is unlocked at call time; attach always nests with_store inside with_backend_db so it never got a chance there. Trigger it once, unlocked, at the top of attach so first-run migration isn't dependent on call order. - replaced a lock().unwrap() in the backfill path with a graceful skip on a poisoned mutex, matching the rest of with_store/with_backend_db. * fix(asset-store): satisfy CI clippy -D warnings (collapsible-if, redundant-closure) * fix(asset-store): remove needless borrow flagged by clippy -D warnings * style(asset-store): cargo fmt * fix(asset-store): address CodeRabbit review findings on PR #282 - backfill_legacy_assets: don't abort the whole batch on one bad legacy asset; skip and log it, migrate the rest, and always write the completion marker. Previously a single unreadable asset meant the marker never got written, so every later with_store() call replayed the entire batch, re-upserting already-migrated docs and bumping their version/history each time. - attach: open backend_db (and release it) before triggering the backfill check, so a totally fresh instance's very first attach call can actually backfill, not just the second one onward. - delete: soft-delete the document before unref'ing its blob, so a failure between the two steps can't leave the last reference's content purged while the asset still reads as live. Agentflare-Agent: claude-code_2-1-215_agent Agentflare-Branch: item-185-asset-store-migration
This was referenced Jul 24, 2026
This was referenced Aug 13, 2026
Merged
3 tasks
getappz
pushed a commit
that referenced
this pull request
Aug 25, 2026
Discovery tick dispatches purely on the ready-for-work label, so items #184/#185/#186/#187 (go/no-go candidates from #166's spec) whose own description says "Decision pending — not dispatched" got auto-dispatched and re-dispatched across multiple agents anyway -- the prose was never actually enforced. Add a needs-decision label that blocks run_discovery_tick even while ready-for-work is also present. Stripping ready-for-work alone wouldn't have been durable: redispatch unconditionally re-attaches it, so the new label has to keep gating on its own until a human clears it. Agentflare-Agent: claude-code Agentflare-Branch: fix/dispatch-failure-ceiling-any-reason
getappz
pushed a commit
that referenced
this pull request
Aug 25, 2026
reconcile_orphaned_jobs already tries to release a dead job's claim via release_and_comment under a with_owner_override scope before calling restore_ready_for_work, but that release is best-effort (let _ = ...) and its failure is silent. When it silently doesn't take, restore_ready_for_work still puts ready-for-work back on the item -- so run_discovery_tick sees it as dispatchable, but the actual dispatch (and redispatch) both refuse with "blocked_by_live_claim" until the claim's TTL naturally expires (up to 4h). Reproduced live on items #185/#187: both sat stuck for the better part of an hour with no visible error, owner strings confirmed to be their own now- dead job's claim. restore_ready_for_work now releases the claim itself too, using the same owner string reconcile_orphaned_jobs already constructs -- defense in depth, not a replacement for the earlier release, so it only ever touches the dead job's own lease. Agentflare-Agent: claude-code Agentflare-Branch: fix/dispatch-failure-ceiling-any-reason
getappz
pushed a commit
that referenced
this pull request
Aug 26, 2026
Reimplemented from scratch against current master -- the prior branch had overwritten sources.rs's SkillEntry scanning adapter (load.rs's default_sources/scan_sources/validate_entry dependency) with an unrelated SkillSpec/TOML module and committed a pile of debug scratch scripts, and was badly stale besides. See PR #614's close comment. Per item #185 (candidate b of item #166's design-spec): a `category` frontmatter field, defaulting to the first tag when unset, plus one read-only skill_categories MCP tool. - crates/skill-registry: category column (0004_category.sql), threaded through Frontmatter/SkillEntry/BundleEntry (so it round-trips through hub export/import, not just local scans) and db::rebuild's insert. New search::list_categories/skills_in_category read helpers and matching Registry methods. - skill_categories MCP tool: omit `category` for every category with its skill count (most populated first); pass one to list its skills. - Three pre-existing SkillEntry literals in src/cli/skill.rs (export, hub push, DB-only-source carry-forward) updated for the new field. Verified: cargo build/clippy/fmt clean against CI's exact invocations; 70 skill-registry tests (7 new) + 246 mcp_server tests + 12 cli::skill tests all pass. Agentflare-Branch: task/185-category-taxonomy-skill-categories-tool Agentflare-Item: 185-category-taxonomy-skill-categories-tool
4 tasks
getappz
added a commit
that referenced
this pull request
Aug 26, 2026
Reimplemented from scratch against current master -- the prior branch had overwritten sources.rs's SkillEntry scanning adapter (load.rs's default_sources/scan_sources/validate_entry dependency) with an unrelated SkillSpec/TOML module and committed a pile of debug scratch scripts, and was badly stale besides. See PR #614's close comment. Per item #185 (candidate b of item #166's design-spec): a `category` frontmatter field, defaulting to the first tag when unset, plus one read-only skill_categories MCP tool. - crates/skill-registry: category column (0004_category.sql), threaded through Frontmatter/SkillEntry/BundleEntry (so it round-trips through hub export/import, not just local scans) and db::rebuild's insert. New search::list_categories/skills_in_category read helpers and matching Registry methods. - skill_categories MCP tool: omit `category` for every category with its skill count (most populated first); pass one to list its skills. - Three pre-existing SkillEntry literals in src/cli/skill.rs (export, hub push, DB-only-source carry-forward) updated for the new field. Verified: cargo build/clippy/fmt clean against CI's exact invocations; 70 skill-registry tests (7 new) + 246 mcp_server tests + 12 cli::skill tests all pass. Agentflare-Branch: task/185-category-taxonomy-skill-categories-tool Agentflare-Item: 185-category-taxonomy-skill-categories-tool Co-authored-by: shiva <shiva@gosysinfo.tech>
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.
Merges skill/search+load, gateway/search+execute, claim/5-actions, review/5-actions, artifact/6-actions, memory/6-actions into one consolidated #[tool(...)] method each with action dispatch.
Reconciles with #182 (fetch target branch before branching) and #183 (deferred claim release, mark_completed split).
Closes #46.
Summary by CodeRabbit
toolaction-dispatch terminology.