[supersede #1968] [supersede #1853] feat(tools): add Feishu document operation tool with 13 actions#1986
Merged
Conversation
Contributor
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Documentation CHANGELOG.md |
Added feishu_doc tool entry under Unreleased -> Added section, listing all supported operations (read, write, append, create, list_blocks, get_block, update_block, delete_block, create_table, write_table_cells, create_table_with_values, upload_image, upload_file). |
Estimated code review effort
🎯 1 (Trivial) | ⏱️ ~3 minutes
Possibly related PRs
- [supersede #1853] feat(tools): add Feishu document operation tool with 13 actions #1968: Implements and exposes the FeishuDocTool in source code (src/tools/feishu_doc.rs and src/tools/mod.rs), corresponding directly to the CHANGELOG entry added in this PR.
Suggested labels
size: XL, risk: medium, tool: feishu_doc, channel
Suggested reviewers
- theonlyhennygod
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The description is incomplete. While it covers Summary, Change Metadata, Linked Issues, Security, and Compatibility, it is missing critical required sections: Label Snapshot (risk/size/scope/module labels), complete Validation Evidence with test outputs, complete Supersede Attribution with co-authored-by confirmation, i18n Follow-Through, Human Verification details, Side Effects/Blast Radius, and Rollback Plan specifics. | Add missing required sections: Label Snapshot (risk/size/scope labels), detailed validation evidence with full test outputs, Supersede Attribution with co-author trailers, i18n follow-through status, detailed Human Verification (scenarios/edge cases), Side Effects assessment, and complete Rollback command. Ensure all required checkboxes are explicitly answered. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly specifies the primary change: adding a Feishu document operation tool with 13 actions, and includes context about superseding previous PRs. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
supersede-pr-1968-20260226164943-141738-theirs
Comment @coderabbitai help to get the list of available commands and usage tips.
github-actions
Bot
added
core
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/feishu_doc.rs`:
- Around line 462-464: After parsing inputs with required_usize for row_size and
column_size (and parsing column_width with parse_column_width), validate that
row_size and column_size are > 0 and return a structured ToolResult error
instead of allowing 0 to proceed; update the handler that calls required_usize
(the scope where row_size, column_size, column_width are read) to check these
values and return an appropriate ToolResult error (with a clear message) when
either is 0, ensuring no panic or API call happens with zero dimensions.
- Around line 857-872: The function enable_link_share is currently discarding
the result of authed_request_with_query and always returning Ok(()), masking API
failures; change the call to await and propagate errors (e.g., use the `?`
operator on the result of self.authed_request_with_query(...).await) or
explicitly map Err into an anyhow::Error and return it so that failures from the
PATCH request (Method::PATCH) are not swallowed; locate enable_link_share and
replace the ignored assignment of the request result with proper error handling
of authed_request_with_query.
- Around line 978-983: The no-redirect HTTP client (no_redirect_client) is
created without a timeout so .send().await, resp.text().await, and the chunked
stream can hang; update the Client::builder() that constructs no_redirect_client
to set an explicit timeout (e.g., via
.timeout(std::time::Duration::from_secs(...)) or a configurable value) so the
overall request+body operations will fail fast on slow/stalled endpoints, and
propagate/map the timeout error consistently where .send().await,
resp.text().await, and the chunked streaming code handle errors.
In `@src/tools/mod.rs`:
- Around line 527-559: The FeishuDocTool is being instantiated
(FeishuDocTool::new) but not exposed in the agent's tool descriptions; update
the tool_descs vector in agent/loop_.rs to include an entry for "feishu_doc"
with a short description (e.g., ("feishu_doc", "Feishu/Lark document tool for
reading and writing docs")) so the agent can advertise and use the tool, and add
a brief "Added — feishu_doc: Feishu/Lark document tool" bullet under the
[Unreleased] section of CHANGELOG.md to document the new tool.
In `@tests/agent_e2e.rs`:
- Around line 870-916: The research-phase tests are too weak:
e2e_agent_research_phase_integration builds an Agent with ResearchPhaseConfig
and ResearchTrigger::Keywords but never sends a keyword-containing message, and
the Always-trigger test similarly lacks assertions that research actually ran;
update the tests to send at least one additional turn that contains a trigger
keyword (e.g., "please search for X" or "find details about Y") using
agent.turn(...) so the research path executes, then inspect the
RecordingProvider via recorded.lock() to assert an extra provider call occurred
and that the captured user message includes the research context/prefix (check
for "[Research" or whatever prefix your code uses) and that the number of
requests increased accordingly; likewise, enhance the Always-trigger test to
assert research context injection and increased provider invocations when
ResearchPhaseConfig.trigger is set to Always.
- Around line 850-853: The Err(e) arm in the research-phase match currently only
logs and continues; change it so only explicit auth/env-missing errors are
treated as soft-skips and all other errors cause the test to fail: in the Err(e)
branch (the match handling the research phase error) inspect the error (by type
or by checking e.to_string() for known markers like "credentials", "auth", or
"environment variable") and if it matches an auth/env-missing case print a
soft-skip message and return/continue; otherwise propagate the failure (panic!,
return Err(...), or assert) so the test fails and surfaces unexpected
regressions.
Collaborator
|
Maintainer scan (2026-02-27):
Next actions to unblock merge:
|
chumyin
force-pushed
the
supersede-pr-1968-20260226164943-141738-theirs
branch
from
9064959 to
cf1e1d8
Compare
github-actions
Bot
added
dependencies
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/tools/feishu_doc.rs (3)
857-872:⚠️ Potential issue | 🟠 Major
enable_link_sharecurrently masks API failures.The function ignores the request result (line 868-870) and always returns
Ok(()), so the warning logic inaction_create(lines 346-356) can never trigger.🐛 Suggested fix
async fn enable_link_share(&self, document_id: &str) -> anyhow::Result<()> { let url = format!( "{}/drive/v2/permissions/{}/public", self.api_base(), document_id ); let body = json!({ "link_share_entity": "anyone_readable", "external_access_entity": "open" }); let query = [("type", "docx".to_string())]; - let _ = self - .authed_request_with_query(Method::PATCH, &url, Some(body), Some(&query)) - .await; + let _ = self + .authed_request_with_query(Method::PATCH, &url, Some(body), Some(&query)) + .await?; Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/feishu_doc.rs` around lines 857 - 872, The enable_link_share function currently discards the result of authed_request_with_query and always returns Ok(()), hiding API failures; update enable_link_share to surface errors by returning the Result from authed_request_with_query (or mapping its Err into anyhow::Error) so callers like action_create can detect and log failures—locate enable_link_share and replace the ignored call to self.authed_request_with_query(...) with code that properly propagates or converts its error instead of unconditionally returning Ok(()).
978-983:⚠️ Potential issue | 🟠 MajorAdd timeout to the no-redirect download client.
The
no_redirect_clienthas no timeout configured, which can hang tool execution indefinitely on slow or stalled endpoints. This affects the initial.send().await, response body reads, and chunked streaming.💡 Suggested fix
+ use std::time::Duration; let no_redirect_client = reqwest::Client::builder() .redirect(reqwest::redirect::Policy::none()) + .connect_timeout(Duration::from_secs(10)) + .timeout(Duration::from_secs(30)) .build() .map_err(|e| anyhow::anyhow!("failed to build no-redirect HTTP client: {}", e))?;Note:
Durationis already imported at line 9, so only the builder changes are needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/feishu_doc.rs` around lines 978 - 983, The no-redirect reqwest client (created in no_redirect_client via reqwest::Client::builder() with redirect Policy::none()) lacks a timeout; update the builder to set a sensible timeout (e.g., .timeout(Duration::from_secs(10)) or another app-appropriate Duration) so requests, response body reads, and streaming cannot hang indefinitely—keep the rest of the builder and the subsequent .build().map_err(...) logic unchanged (Duration is already imported).
462-464:⚠️ Potential issue | 🟡 MinorReject zero-sized table dimensions before calling the API.
row_size/column_sizeaccept0, which should fail fast at input validation time rather than sending an invalid request to the API.💡 Suggested fix
let row_size = required_usize(args, "row_size")?; let column_size = required_usize(args, "column_size")?; + if row_size == 0 || column_size == 0 { + anyhow::bail!("'row_size' and 'column_size' must be greater than 0"); + } let column_width = parse_column_width(args)?;As per coding guidelines: Implement
Toolwith strict parameter schema; validate and sanitize all inputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/feishu_doc.rs` around lines 462 - 464, After parsing args, reject zero-sized table dimensions by validating the values returned from required_usize for row_size and column_size (and any parsed column_width if relevant) before proceeding to call the API: if row_size == 0 or column_size == 0 return an Err with a clear validation message (e.g. "row_size must be > 0" / "column_size must be > 0") from the function that constructs or runs the Tool; update the code around the calls to required_usize("row_size"), required_usize("column_size"), and parse_column_width to perform these checks and fail fast instead of sending an invalid request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/tools/feishu_doc.rs`:
- Around line 857-872: The enable_link_share function currently discards the
result of authed_request_with_query and always returns Ok(()), hiding API
failures; update enable_link_share to surface errors by returning the Result
from authed_request_with_query (or mapping its Err into anyhow::Error) so
callers like action_create can detect and log failures—locate enable_link_share
and replace the ignored call to self.authed_request_with_query(...) with code
that properly propagates or converts its error instead of unconditionally
returning Ok(()).
- Around line 978-983: The no-redirect reqwest client (created in
no_redirect_client via reqwest::Client::builder() with redirect Policy::none())
lacks a timeout; update the builder to set a sensible timeout (e.g.,
.timeout(Duration::from_secs(10)) or another app-appropriate Duration) so
requests, response body reads, and streaming cannot hang indefinitely—keep the
rest of the builder and the subsequent .build().map_err(...) logic unchanged
(Duration is already imported).
- Around line 462-464: After parsing args, reject zero-sized table dimensions by
validating the values returned from required_usize for row_size and column_size
(and any parsed column_width if relevant) before proceeding to call the API: if
row_size == 0 or column_size == 0 return an Err with a clear validation message
(e.g. "row_size must be > 0" / "column_size must be > 0") from the function that
constructs or runs the Tool; update the code around the calls to
required_usize("row_size"), required_usize("column_size"), and
parse_column_width to perform these checks and fail fast instead of sending an
invalid request.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
src/main.rssrc/tools/feishu_doc.rssrc/tools/mod.rstests/agent_e2e.rs
💤 Files with no reviewable changes (1)
- tests/agent_e2e.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main.rs
chumyin
force-pushed
the
supersede-pr-1968-20260226164943-141738-theirs
branch
from
cf1e1d8 to
8eb17b4
Compare
github-actions
Bot
added
ci
github-actions
Bot
added
tool
github-actions
Bot
added
ci
theonlyhennygod
approved these changes
Feb 28, 2026
Collaborator
theonlyhennygod
left a comment
theonlyhennygod
left a comment
There was a problem hiding this comment.
Approved per maintainer request; merging if conflict-free.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/agent/loop_.rs (1)
1883-1889: Keepfeishu_docprompt descriptor text consistent across both entry paths.
runandprocess_messagecurrently use different descriptions for the same tool, which can cause prompt-behavior drift between CLI and channel execution paths.♻️ Suggested consolidation
+const FEISHU_DOC_TOOL_DESC: &str = + "Manage Feishu/Lark documents: read, write, append, create, table operations, and media upload support."; @@ - tool_descs.push(( - "feishu_doc", - "Manage Feishu/Lark documents: read, write, append, create, and table operations with media upload support.", - )); + tool_descs.push(("feishu_doc", FEISHU_DOC_TOOL_DESC)); @@ - tool_descs.push(( - "feishu_doc", - "Manage Feishu/Lark documents with read/write/table operations.", - )); + tool_descs.push(("feishu_doc", FEISHU_DOC_TOOL_DESC));Also applies to: 2334-2340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/loop_.rs` around lines 1883 - 1889, The prompt descriptor for the "feishu_doc" tool is inconsistent between the two entry paths (the code that builds tool_descs in run and in process_message), causing different prompts depending on CLI vs channel execution; locate the tuples pushed into tool_descs referencing "feishu_doc" (e.g., the push in function run and the similar push in process_message) and replace the duplicated description strings so both use one canonical description text (choose one clear, complete string and apply it to both occurrences) to ensure identical prompt behavior across both paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/loop_.rs`:
- Line 2039: The interactive editor is created with rustyline::DefaultEditor
which uses an empty helper, so SlashCommandCompleter (the helper implementing
Completer/Hinter/Highlighter/Validator/Helper) is never wired and completion is
disabled; replace DefaultEditor with the generic Editor so you can call
set_helper(...) on the rl instance and pass an instance of SlashCommandCompleter
to enable completion and hints (use Editor::<SlashCommandCompleter, _>::new() or
Editor::new() then rl.set_helper(Some(SlashCommandCompleter::new(...)))).
---
Nitpick comments:
In `@src/agent/loop_.rs`:
- Around line 1883-1889: The prompt descriptor for the "feishu_doc" tool is
inconsistent between the two entry paths (the code that builds tool_descs in run
and in process_message), causing different prompts depending on CLI vs channel
execution; locate the tuples pushed into tool_descs referencing "feishu_doc"
(e.g., the push in function run and the similar push in process_message) and
replace the duplicated description strings so both use one canonical description
text (choose one clear, complete string and apply it to both occurrences) to
ensure identical prompt behavior across both paths.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/pub-prerelease.ymlCHANGELOG.mdsrc/agent/loop_.rssrc/tools/cron_add.rstests/agent_e2e.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/pub-prerelease.yml
- src/tools/cron_add.rs
| .build(), | ||
| )?; | ||
| rl.set_helper(Some(SlashCommandCompleter)); | ||
| let mut rl = rustyline::DefaultEditor::new()?; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, verify the existence and location of SlashCommandCompleter and the current DefaultEditor usage
rg -n "SlashCommandCompleter|DefaultEditor::new" src/agent/loop_.rs | head -20Repository: zeroclaw-labs/zeroclaw
Length of output: 390
🏁 Script executed:
#!/bin/bash
# Check the context around line 2039 to see the editor initialization
sed -n '2030,2050p' src/agent/loop_.rsRepository: zeroclaw-labs/zeroclaw
Length of output: 887
🏁 Script executed:
#!/bin/bash
# Look for the SlashCommandCompleter definition and any struct/impl blocks
rg -n "struct SlashCommandCompleter|impl.*SlashCommandCompleter" src/agent/loop_.rsRepository: zeroclaw-labs/zeroclaw
Length of output: 330
🏁 Script executed:
#!/bin/bash
# Check git history to see if this was a recent change and what it replaced
cd src/agent && git log --oneline -n 20 -- loop_.rsRepository: zeroclaw-labs/zeroclaw
Length of output: 140
🏁 Script executed:
#!/bin/bash
# Check if set_helper is called anywhere on the rl variable or nearby
sed -n '2030,2100p' src/agent/loop_.rs | grep -n "set_helper\|rl\."Repository: zeroclaw-labs/zeroclaw
Length of output: 298
🏁 Script executed:
#!/bin/bash
# Check rustyline module imports to understand what DefaultEditor is
rg -n "use rustyline|extern crate rustyline" src/agent/loop_.rs | head -10Repository: zeroclaw-labs/zeroclaw
Length of output: 304
🏁 Script executed:
#!/bin/bash
# Search for any historical usage of SlashCommandCompleter with the editor
rg -n "set_helper.*SlashCommand|Editor.*Slash" src/agent/Repository: zeroclaw-labs/zeroclaw
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check rustyline version and understand the Editor/DefaultEditor API
rg -n "rustyline" Cargo.toml | grep -v "^Binary"Repository: zeroclaw-labs/zeroclaw
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Look for Editor usage patterns in the codebase to see if there are other examples
rg -n "Editor::<|DefaultEditor" src/ --type rsRepository: zeroclaw-labs/zeroclaw
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Check the full context of how SlashCommandCompleter is used to understand intent
sed -n '85,160p' src/agent/loop_.rsRepository: zeroclaw-labs/zeroclaw
Length of output: 2230
🏁 Script executed:
#!/bin/bash
# Verify rustyline 17.0 API - check what DefaultEditor actually is
pip install rustyline-stubs 2>/dev/null || true
python3 <<'EOF'
# Check rustyline documentation for DefaultEditor type
print("In rustyline 17.0, DefaultEditor is a type alias for Editor::<(), DefaultHistory>")
print("This means it uses an empty () helper, so custom helpers cannot be set.")
print("\nTo use a custom helper like SlashCommandCompleter:")
print("1. Create Editor::<SlashCommandCompleter, DefaultHistory>::new()")
print("2. Call rl.set_helper(Some(SlashCommandCompleter))")
EOFRepository: zeroclaw-labs/zeroclaw
Length of output: 465
🏁 Script executed:
#!/bin/bash
# Final verification: confirm no set_helper is called anywhere in the file
rg "set_helper" src/agent/loop_.rsRepository: zeroclaw-labs/zeroclaw
Length of output: 48
SlashCommandCompleter is not wired to the interactive editor.
The SlashCommandCompleter struct (lines 90–149) implements all helper traits (Completer, Hinter, Highlighter, Validator, Helper), but DefaultEditor is just an alias for Editor::<(), DefaultHistory>, which uses an empty helper type and ignores custom helpers. This silently disables slash-command completion and hints at line 2039.
Wire the custom helper by using the generic Editor type and calling set_helper():
Suggested fix
- let mut rl = rustyline::DefaultEditor::new()?;
+ let mut rl =
+ rustyline::Editor::<SlashCommandCompleter, rustyline::history::DefaultHistory>::new()?;
+ rl.set_helper(Some(SlashCommandCompleter));📝 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
| let mut rl = rustyline::DefaultEditor::new()?; | |
| let mut rl = | |
| rustyline::Editor::<SlashCommandCompleter, rustyline::history::DefaultHistory>::new()?; | |
| rl.set_helper(Some(SlashCommandCompleter)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/loop_.rs` at line 2039, The interactive editor is created with
rustyline::DefaultEditor which uses an empty helper, so SlashCommandCompleter
(the helper implementing Completer/Hinter/Highlighter/Validator/Helper) is never
wired and completion is disabled; replace DefaultEditor with the generic Editor
so you can call set_helper(...) on the rl instance and pass an instance of
SlashCommandCompleter to enable completion and hints (use
Editor::<SlashCommandCompleter, _>::new() or Editor::new() then
rl.set_helper(Some(SlashCommandCompleter::new(...)))).
PR intake checks found warnings (non-blocking)Fast safe checks found advisory issues. CI lint/test/build gates still enforce merge quality.
Action items:
Detected Linear keys: RMN-140 Run logs: https://github.com/zeroclaw-labs/zeroclaw/actions/runs/22532450309 Detected blocking line issues (sample):
Detected advisory line issues (sample):
|
This was referenced Mar 2, 2026
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
2 participants
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
Problem
Current PR #1986 failed to merge due branch drift and repeated merge conflicts against the latest integration branch.
Why It Matters
This PR introduces a Feishu document tool with 13 actions. Keeping it blocked delays channel-lark feature delivery and leaves duplicate supersede tracks unresolved.
What Changed
supersede-pr-1968-20260226164943-141738-theirsonto latestorigin/dev.src/tools/mod.rswhile preserving Feishu tool registration.src/tools/feishu_doc.rs,src/main.rs,tests/agent_e2e.rs).devbaseline (not needed ondev).Change Metadata
feature+rebase/conflict-resolutiontools,channel-larkLinked Issue
RMN-140Supersede Attribution
dev.Validation Evidence
Commands
cargo check --locked --features channel-larkcargo check --features channel-larkResults
Security Impact
Risk
Mitigation
channel-larkfeature path and existing security policy gates.channels_config.feishu/channels_config.lark) and keeps explicit operation routing.Privacy and Data Hygiene
Compatibility
channel-larkbuilds remain unaffected.Rollback Plan
supersede-pr-1968-20260226164943-141738-theirs.channel-larkfeature path.Human Verification
dev.--locked.Summary by CodeRabbit