fix: v1.6.0 verification bugs — opencode filePath guard hole, search web/store arms - #287
Conversation
Agentflare-Agent: claude-code_2-1-216_harness Agentflare-Branch: fix/v160-verification-bugs
…nto store arm Agentflare-Agent: claude-code_2-1-216_harness Agentflare-Branch: fix/v160-verification-bugs
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds ChangesHook redirect compatibility
MCP search aggregation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant search_store
participant artifact_search_hits
participant ArtifactStore
Client->>search_store: submit search query
search_store->>artifact_search_hits: search query
artifact_search_hits->>ArtifactStore: list artifacts
ArtifactStore-->>artifact_search_hits: artifact records
artifact_search_hits-->>search_store: matched artifact hits
search_store-->>Client: grouped results with artifact matches
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/hook_redirect.rs (1)
125-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate path extraction and handle non-string values robustly.
The current
or_elsechain onValue::getwill stop checking fallbacks if an earlier key exists but its value is not a string (e.g.,{"file_path": null, "filePath": "src/main.rs"}). In that scenario,getyieldsSome(&Value::Null), bypassing theor_elseblocks, and the subsequentas_stryieldsNone.Using an array and
find_mapis both more robust against hallucinated inputs and removes the duplication. You can introduce a helper likefn extract_target_path<'a>(v: &'a Value) -> Option<&'a str> { ["file_path", "path", "filePath"].into_iter().find_map(|k| v.get(k)?.as_str()) }and apply it across these sites:
src/hook_redirect.rs#L125-L131: Replace the inlineor_elsechain with.and_then(extract_target_path)?.src/hook_redirect.rs#L161-L164: Replace the inlineor_elsechain withextract_target_path(ti)(which then cleanly chains into.map(Path::new)).🤖 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/hook_redirect.rs` around lines 125 - 131, Consolidate target-path extraction in src/hook_redirect.rs at lines 125-131 and 161-164 by adding an extract_target_path helper that checks file_path, path, and filePath with find_map and only accepts string values. Replace the anchor’s inline chain with and_then(extract_target_path)? and the sibling’s chain with extract_target_path(ti), preserving its existing Path::new mapping.src/mcp_server/artifact.rs (1)
135-144: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffPotential N+1 I/O issue during artifact search.
Calling
store.get(&summary.id)inside thestore.list()loop introduces an N+1 query pattern. Ifstore.getperforms disk I/O or network requests, this will scale poorly as the number of artifacts grows.Consider batching these fetches or implementing a dedicated full-text search (FTS) method directly in the
ArtifactStorelayer if performance degrades in the future.🤖 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/artifact.rs` around lines 135 - 144, Update the artifact search flow around the store.list loop to avoid calling store.get for every summary. Prefer a batch retrieval or dedicated full-text search operation in ArtifactStore that evaluates content together with name and description, while preserving the existing matching and result behavior.
🤖 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/mcp_server/artifact.rs`:
- Around line 145-159: Update the snippet logic in the content search path to
cache the lowercased content, use that same string for both find and slicing,
and return the snippet from those matching lowercase bytes. Preserve the
existing 40-byte context and character-boundary adjustments while avoiding
repeated allocation and mismatched indices against the original content.
---
Nitpick comments:
In `@src/hook_redirect.rs`:
- Around line 125-131: Consolidate target-path extraction in
src/hook_redirect.rs at lines 125-131 and 161-164 by adding an
extract_target_path helper that checks file_path, path, and filePath with
find_map and only accepts string values. Replace the anchor’s inline chain with
and_then(extract_target_path)? and the sibling’s chain with
extract_target_path(ti), preserving its existing Path::new mapping.
In `@src/mcp_server/artifact.rs`:
- Around line 135-144: Update the artifact search flow around the store.list
loop to avoid calling store.get for every summary. Prefer a batch retrieval or
dedicated full-text search operation in ArtifactStore that evaluates content
together with name and description, while preserving the existing matching and
result behavior.
🪄 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: c5f66919-90eb-4e44-88f3-c9b3eb162147
📒 Files selected for processing (4)
src/hook_redirect.rssrc/mcp_server/artifact.rssrc/mcp_server/search.rssrc/mcp_server/tests/search_tests.rs
Keeps master's num_results clamp (limit is already clamped at assignment) plus this branch's extract_content/follow_links: false flags for the web search arm. Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: fix/v160-verification-bugs
content.to_lowercase().find() returns a byte offset into the lowercased string, then used it to slice the original content — if lowercasing changes a character's byte length (some Unicode case-folding), the offset no longer lines up with the original string. Cache the lowercased string and slice from it instead of the original. Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: fix/v160-verification-bugs
1 participant
Fixes three bugs found while live-verifying the v1.6.0 release (agentflare work items #252, #253, #254 — item tracker numbers, not GitHub issues).
1. Branch guard: opencode native edits on master were unguarded (item #252, high)
hook_redirect.rsonly readfile_path/pathfrom tool_input, but opencode native tools send camelCasefilePath(forwarded raw bybranch-guard.js). Since #283 removed the host-cwd fallback, the target repo resolved toNoneand the guard silently allowed default-branch edits from opencode. Both lookup sites now includefilePath.Verified live with the debug binary: camelCase payload on a master-checkout file now denies; non-repo file still allows; snake_case unchanged. Two regression tests added (pure classify + temp-git-repo integration test asserting the deny).
2. Search web arm always errored (item #253)
The web arm forwarded
max_resultsto rivalsearchweb_search, whose schema takesnum_results(1..=20) — every call failed schema validation. Now sendsnum_resultsclamped to 1..=20, withextract_content/follow_linksoff (crawl defaults are too heavy for a search-arm result list).3. Search store arm returned 0 despite matching artifacts (item #254)
Artifacts live in the artifacts store, not agentflare-store docs (and asset docs carry empty FTS content — bytes are in blobs), so the store arm could never match what its description promises. The artifact substring scan is extracted into a shared
artifact_search_hitshelper (the artifact toolsearchaction now delegates to it, −42 duplicated lines) and the store arm folds matches in as anartifactgroup. Integration test: publish → store search finds it.Verification
cargo fmt --checkcleancargo clippy --workspace --all-features -- -D warnings: only the two pre-existing Windows-local dead-code errors indaemon_autostart.rs(unix-cfg-only helpers, present on master, untouched here)cargo test --workspace: 695 passed / 0 failed in the main crate, all other suites greenSummary by CodeRabbit
New Features
filePathpayloads.Bug Fixes
filePath.