Repository navigation
Conversation
…neration to nanoid item/claim MCP tools (get, update, update_state, delete, claim, heartbeat, release, done, add_label, remove_label; claim's target param) now accept either a UUID or a numeric sequence_id (bare or #-prefixed), resolved via agentflare_backend::item::resolve_id scoped to the repo's linked project. Not-found numeric ids return the same not-found shape as an unmatched UUID. Closes #184. Also switches db_kit::ids::new_id() from uuid::Uuid::now_v7() to nanoid::nanoid!(), updating every caller across agentflare-artifacts and agentflare-backend (asset/comment/label/project/state/webhook/workspace). cargo build --workspace --all-features, cargo test --workspace (630+ passing across the bin plus every crate), cargo fmt --check, and cargo clippy --workspace --all-features -D warnings all clean (the one remaining clippy hit is the pre-existing Windows-only agent_launch.rs test import, tracked separately as item #169, unrelated to this change).
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change replaces UUID-based generation with NanoID-backed identifiers, adds sequence-ID resolution for items, routes MCP item and claim actions through canonical IDs, and updates related tests and schema descriptions. ChangesIdentifier migration and MCP resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant AgentflareMcp
participant ItemResolver
participant BackendAction
MCPClient->>AgentflareMcp: submit item or claim action
AgentflareMcp->>ItemResolver: resolve UUID or sequence_id
ItemResolver-->>AgentflareMcp: return canonical item ID
AgentflareMcp->>BackendAction: execute action with canonical item ID
BackendAction-->>MCPClient: return action result
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: 5
🤖 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/comment.rs`:
- Line 41: Update comment ID generation in the comment persistence flow around
new_id so IDs retain insertion-time ordering, or introduce and populate an
explicit monotonic insertion-order column used by the is_latest query. Ensure
the created_at DESC, id DESC ordering cannot misclassify comments created within
the same second.
In `@crates/agentflare-backend/src/item.rs`:
- Around line 197-218: Normalize the input in the sequence-resolution function
before the `parse::<i64>()` branch by removing one leading `#` prefix, then
parse and query using the normalized value while preserving the existing
direct-ID fallback for non-numeric identifiers.
- Around line 192-221: Update item action handlers in src/mcp_server/item.rs so
every backend operation accepting an item ID first passes the supplied
identifier through agentflare_backend::item::resolve_id, including item_get’s
current direct get call. Use the resolved UUID for subsequent backend operations
while preserving project scoping and existing behavior for UUID inputs.
In `@src/mcp_server/item.rs`:
- Around line 147-154: Update resolve_item_id to remove exactly one leading “#”
from id_or_seq before calling agentflare_backend::item::resolve_id, while
leaving canonical IDs and unprefixed sequence IDs unchanged. Add regression
coverage confirming mutation resolution accepts the documented “#<sequence_id>”
form.
In `@src/mcp_server/types.rs`:
- Around line 570-574: Update item_get to pass the requested ID through
resolve_item_id before calling get, so numeric IDs resolve against the current
project’s sequence_id while canonical UUIDs remain supported. Preserve the
existing get behavior after resolution and use the current project context
required by resolve_item_id.
🪄 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: 71bc228e-3b7a-4d09-b025-0f6ef630191c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
crates/agentflare-artifacts/Cargo.tomlcrates/agentflare-artifacts/src/store.rscrates/agentflare-backend/Cargo.tomlcrates/agentflare-backend/src/asset.rscrates/agentflare-backend/src/comment.rscrates/agentflare-backend/src/item.rscrates/agentflare-backend/src/label.rscrates/agentflare-backend/src/project.rscrates/agentflare-backend/src/state.rscrates/agentflare-backend/src/webhook.rscrates/agentflare-backend/src/workspace.rscrates/agentflare-db-kit/Cargo.tomlcrates/agentflare-db-kit/src/ids.rssrc/mcp_server/claim.rssrc/mcp_server/item.rssrc/mcp_server/tests/action_tests.rssrc/mcp_server/tests/artifact_tests.rssrc/mcp_server/types.rs
💤 Files with no reviewable changes (1)
- crates/agentflare-backend/Cargo.toml
| body: &str, | ||
| ) -> Result<ItemComment> { | ||
| let id = uuid::Uuid::now_v7().to_string(); | ||
| let id = db_kit::ids::new_id(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not use NanoID ordering to identify the latest comment.
is_latest still uses created_at DESC, id DESC; after this change, NanoIDs are random rather than UUIDv7 time-ordered, so same-second comments can be misclassified. Keep time-ordered IDs for this table or add an explicit monotonic insertion-order column.
🤖 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-backend/src/comment.rs` at line 41, Update comment ID
generation in the comment persistence flow around new_id so IDs retain
insertion-time ordering, or introduce and populate an explicit monotonic
insertion-order column used by the is_latest query. Ensure the created_at DESC,
id DESC ordering cannot misclassify comments created within the same second.
Comment on lines
+147
to
+154
| pub(crate) fn resolve_item_id( | ||
| &self, | ||
| conn: &Connection, | ||
| id_or_seq: &str, | ||
| ) -> Result<String, ErrorData> { | ||
| let project = self.resolve_project(conn)?; | ||
| agentflare_backend::item::resolve_id(conn, Some(&project.id), id_or_seq) | ||
| .map_err(map_backend_err) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize #<sequence_id> before backend resolution.
Line 153 forwards #42 unchanged, but resolve_id only parses raw integers and otherwise treats it as a canonical ID. All resolved mutation actions therefore reject the documented #-prefixed form. Strip one leading # before delegation and add regression coverage.
Proposed fix
pub(crate) fn resolve_item_id(
&self,
conn: &Connection,
id_or_seq: &str,
) -> Result<String, ErrorData> {
let project = self.resolve_project(conn)?;
- agentflare_backend::item::resolve_id(conn, Some(&project.id), id_or_seq)
+ let id_or_seq = id_or_seq.strip_prefix('#').unwrap_or(id_or_seq);
+ agentflare_backend::item::resolve_id(conn, Some(&project.id), id_or_seq)
.map_err(map_backend_err)
}📝 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
| pub(crate) fn resolve_item_id( | |
| &self, | |
| conn: &Connection, | |
| id_or_seq: &str, | |
| ) -> Result<String, ErrorData> { | |
| let project = self.resolve_project(conn)?; | |
| agentflare_backend::item::resolve_id(conn, Some(&project.id), id_or_seq) | |
| .map_err(map_backend_err) | |
| pub(crate) fn resolve_item_id( | |
| &self, | |
| conn: &Connection, | |
| id_or_seq: &str, | |
| ) -> Result<String, ErrorData> { | |
| let project = self.resolve_project(conn)?; | |
| let id_or_seq = id_or_seq.strip_prefix('#').unwrap_or(id_or_seq); | |
| agentflare_backend::item::resolve_id(conn, Some(&project.id), id_or_seq) | |
| .map_err(map_backend_err) |
🤖 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/mcp_server/item.rs` around lines 147 - 154, Update resolve_item_id to
remove exactly one leading “#” from id_or_seq before calling
agentflare_backend::item::resolve_id, while leaving canonical IDs and unprefixed
sequence IDs unchanged. Add regression coverage confirming mutation resolution
accepts the documented “#<sequence_id>” form.
…refix, fix flaky nanoid test Review of task/184 (item #186) found the PR didn't actually cover its own motivating case: item_get -- the exact call from #184's bug report (item(get, id="178")) -- was never wired to resolve_item_id, and resolve_id's numeric parse didn't strip a leading '#', so #-prefixed sequence ids silently fell through to the UUID passthrough branch instead of resolving, despite both the commit message and the tool's own schema description claiming that support. - item_get now resolves through resolve_item_id, closing the original gap - resolve_id strips a leading '#' before parsing as numeric - Added the tests #184 explicitly required and that were missing: bare numeric, #-prefixed numeric, not-found numeric, project-scoped lookup, end-to-end via the item MCP tool - Fixed a flaky test: handoff_tool_requires_recipient_and_assigns_item asserted a filename via item_id.to_lowercase(), which doesn't match production's actual AgentflareMcp::slugify() transform -- slugify also collapses '_' to '-', which to_lowercase() doesn't, so the assertion failed whenever a randomly-generated nanoid id happened to contain an underscore. Now asserts against the real transform. cargo test --workspace (632 passing, 0 failed), cargo fmt --check, and cargo clippy --workspace --all-features -D warnings all clean.
Owner
Author
Review (item #186)Found 3 issues, all fixed in commit 113ee63:
Also added the tests #184's spec explicitly required and that were missing entirely: bare numeric,
One thing flagged but not fixed (out of scope for this PR): nanoid's default alphabet includes |
3 tasks done
getappz
added a commit
that referenced
this pull request
Sep 16, 2026
…paths.rs`, new crate added). This is ready to hand off. (#689) ## Status Implemented Step 1 of #257's config-centralization plan: - New `crates/agentflare-config` crate with `home()` (identical logic/env var to the old `paths::home()`), a new `config_dir(namespace: &str)` per the design spec (`~/.config/agentflare/<namespace>`), and `test_support::with_temp_home` (moved verbatim, including its 3 regression tests for the Windows leftover-file/thread-contention/env-cleanup flakes). - `src/paths.rs::home()` is now a one-line wrapper (`agentflare_config::home()`); `test_support::with_temp_home` there is a re-export so the ~80 existing `crate::paths::test_support::with_temp_home` call sites are untouched. `with_temp_cwd` stayed in `paths.rs` (out of scope per the task). - Registered the new crate in the workspace and root `Cargo.toml`. **Design note:** the spec says `agentflare-config` should have no dependency on any existing agentflare crate, but `with_temp_home` must share `agent_registry::detect::PATH_LOCK` — using a separate mutex would reintroduce the exact `set_var` data race the original code's comments warn about (concurrent `set_var` calls on different vars are UB regardless of which lock guards which). `agent-registry` is a leaf crate (no agentflare-crate deps of its own), so this doesn't create a cycle. I judged correctness over the aspirational purity for this zero-behavior-change step; worth confirming with whoever does step 2+. **Verification (all synchronous, sccache disabled — the ambient `sccache` wrapper in this sandbox is broken and throws spurious `EROFS` errors on `.d` file writes, unrelated to this change):** - `cargo test -p agentflare-config` — 4/4 pass - `cargo test -p agentflare --bin agentflare -- paths::` — 4/4 pass (including the new wrapper delegation test) - `cargo check --workspace` — clean (only pre-existing `unsafe_code=warn` noise) - `cargo fmt --check` and `cargo clippy --no-deps` on the new crate — clean No concerns beyond the design-spec deviation noted above. Agentflare-Branch: task/260-agentflare-config-crate-step-1-extract-h Agentflare-Item: 260 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
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
item/claimMCP tools now accept either a UUID or a numericsequence_id(bare or#-prefixed) wherever an item id is taken —get,update,update_state,delete,claim,heartbeat,release,done,add_label,remove_label, andclaim'stargetparam. Resolved viaagentflare_backend::item::resolve_id, scoped to the repo's linked project. Not-found numeric ids return the same shape as an unmatched UUID. Closes feat(gateway): put lean-ctx behind the gateway, rename gateway_search/execute to tool_search/tool_execute #184.db_kit::ids::new_id()fromuuid::Uuid::now_v7()tonanoid::nanoid!(), with every caller updated (agentflare-artifacts, and agentflare-backend's asset/comment/label/project/state/webhook/workspace).Test plan
cargo build --workspace --all-features— cleancargo test --workspace— 630+ tests passing across the bin and every crate, 0 failedcargo fmt --check— cleancargo clippy --workspace --all-features -- -D warnings -A unsafe_code -A clippy::pedantic— clean (excluding the pre-existing Windows-onlyagent_launch.rstest-import warning, already tracked separately as item handoff: assign items + attach versioned assets instead of raw artifacts #169, unrelated to this change)Summary by CodeRabbit
New Features
Improvements