feat(tools): add ask tool with pending question store and answer correlation - #638
Conversation
…elation Add an `ask` tool that presents the user with a question and selectable answer options. Renders as buttons or a select menu on platforms that support interactive elements, and as a numbered list on text-only channels. Answers arrive as enriched interaction messages that include the original question context for the model. - New `AskTool` in `src/tools/ask.rs` — generates `InteractiveElements` from options and sends via the existing `RichMessage` path - New `QuestionStore` in `src/questions.rs` — SQLite-backed pending question CRUD with TIMESTAMP columns matching project conventions - New migration `20260810000002_pending_questions.sql` - Answer correlation in `src/agent/channel.rs` — resolves pending questions on inbound interaction clicks and renders enriched text - Tool description prompt template + registration in channel tools
WalkthroughAdds a SQLite-backed pending-question store, a selectable ChangesAsk interaction flow
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| }; | ||
|
|
||
| vec
There was a problem hiding this comment.
multi_select currently only changes the placeholder. The shared SelectMenu type and Discord renderer never set a max selected count, so Discord string selects still default to one selection. Either carry that through to the renderer, or reject multi_select until the UI can honor it.
| } | ||
|
|
||
| // Resolve the question so duplicate clicks get the expired path | ||
| let _ = store.resolve(question_id, &answer_labels).await; |
There was a problem hiding this comment.
This ignores whether the conditional update actually resolved the row. If two clicks arrive at the same time, both can be shown to the model as accepted answers even though only one update wins.
| let _ = store.resolve(question_id, &answer_labels).await; | |
| if !store.resolve(question_id, &answer_labels).await.unwrap_or(false) { | |
| return format!("[interaction: {action_id}] (expired)"); | |
| } |
| /// already resolved or not found. | ||
| pub async fn resolve(&self, question_id: &str, answer: &[String]) -> Result<bool> { | ||
| let answer_json = serde_json::to_string(answer).context("failed to serialize answer")?; | ||
| let now = now_iso(); |
There was a problem hiding this comment.
resolved_at is written as YYYY-MM-DDTHH:MM:SSZ, but prune_expired compares it lexicographically to SQLite datetime('now', ?) values shaped like YYYY-MM-DD HH:MM:SS. Because T sorts after a space, resolved rows on or after the cutoff date can fail to prune. Store resolved_at in the same format as created_at, or compare with datetime(resolved_at).
![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
🧹 Nitpick comments (2)
src/questions.rs (1)
73-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
qparameter.Use
questionornew_questioninstead ofq.As per coding guidelines: "Don't abbreviate variable names. Use
queuenotq,messagenotmsg,channelnotch."🤖 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/questions.rs` at line 73, Rename the insert method’s q parameter to question or new_question, and update every reference within insert to use the new descriptive name without changing behavior.Source: Coding guidelines
src/agent/channel.rs (1)
154-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRender the values as plain text, not
Debug.
{:?}onVec<String>emits["a", "b"]with quotes and escapes. This text goes to the model. Join the values instead.♻️ Proposed fix
if !values.is_empty() { - return format!("[interaction: {action_id} → {:?}]", values); + return format!("[interaction: {action_id} → {}]", values.join(", ")); }🤖 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/agent/channel.rs` around lines 154 - 157, Update the interaction formatting branch around the values collection to render non-empty values as plain text by joining the strings, replacing the Debug-style {:?} formatting in the format! call. Preserve the existing value-less output and interaction prefix.
🤖 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/agent/channel.rs`:
- Around line 185-193: Use the Result returned by QuestionStore::resolve in the
answer-handling flow instead of discarding it with let _. Propagate or handle
resolution errors according to the surrounding function’s conventions, and
return the documented expired marker when resolve succeeds with false so
concurrent duplicate clicks do not render an answered message.
In `@src/questions.rs`:
- Around line 63-65: Align the timestamp representation used by now_iso and the
SQLite retention query so resolved_at and created_at are compared consistently
with datetime('now', ?). Prefer emitting the SQLite CURRENT_TIMESTAMP format
from now_iso, or normalize resolved_at with datetime() in prune_expired; apply
the same fix to the related logic around the additional referenced range.
- Line 178: Update the answer retrieval and the analogous fields at the other
highlighted call sites to use `try_get::<Option<String>, _>` without `.ok()`,
preserving SQL NULL as `Ok(None)` while propagating or explicitly handling
decode errors. Do not convert `Err` results into silent `None` values.
- Around line 146-164: Call QuestionStore::prune_expired with
DEFAULT_QUESTION_TTL_DAYS from an appropriate periodic maintenance or startup
path, ensuring the operation runs automatically without requiring an external
caller. Reuse the existing QuestionStore instance and preserve its error
handling while preventing pending_questions from growing indefinitely.
In `@src/tools/ask.rs`:
- Around line 275-282: Remove the full question content from the info-level
tracing in the “ask tool sent question” log within the ask flow. Replace
question = %question with a non-sensitive summary such as its length or a
bounded truncated representation, while preserving the existing question_id,
channel_id, options_count, and multi_select fields.
---
Nitpick comments:
In `@src/agent/channel.rs`:
- Around line 154-157: Update the interaction formatting branch around the
values collection to render non-empty values as plain text by joining the
strings, replacing the Debug-style {:?} formatting in the format! call. Preserve
the existing value-less output and interaction prefix.
In `@src/questions.rs`:
- Line 73: Rename the insert method’s q parameter to question or new_question,
and update every reference within insert to use the new descriptive name without
changing behavior.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6bc9c76f-9fe6-4400-b043-002f9a1eab43
📒 Files selected for processing (8)
migrations/20260810000002_pending_questions.sqlprompts/en/tools/ask_description.md.j2src/agent/channel.rssrc/lib.rssrc/prompts/text.rssrc/questions.rssrc/tools.rssrc/tools/ask.rs
| if answer_labels.is_empty() { | ||
| return format!("[interaction: {action_id}] (expired — no matching options)"); | ||
| } | ||
|
|
||
| // Resolve the question so duplicate clicks get the expired path | ||
| let _ = store.resolve(question_id, &answer_labels).await; | ||
|
|
||
| let labels_str = answer_labels.join(", "); | ||
| format!("{sender_name} answered \"{}\": {labels_str}", q.question) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the result of resolve; do not discard it.
QuestionStore::resolve returns Result<bool>. The false value is the duplicate-click guard: it means another click already resolved the question. Line 190 discards both the error and the boolean, so this function still renders answered "…" for a losing concurrent click. The doc comment on line 142 promises the (expired) marker for that case. The get check on line 164 is a separate read, so a check-then-act race is possible between two clicks.
Branch on the returned value.
🐛 Proposed fix
- // Resolve the question so duplicate clicks get the expired path
- let _ = store.resolve(question_id, &answer_labels).await;
-
- let labels_str = answer_labels.join(", ");
- format!("{sender_name} answered \"{}\": {labels_str}", q.question)
+ // Resolve the question so duplicate clicks get the expired path.
+ // The conditional UPDATE is the authoritative winner check.
+ match store.resolve(question_id, &answer_labels).await {
+ Ok(true) => {
+ let labels_str = answer_labels.join(", ");
+ format!("{sender_name} answered \"{}\": {labels_str}", q.question)
+ }
+ Ok(false) => format!("[interaction: {action_id}] (expired)"),
+ Err(error) => {
+ tracing::warn!(%error, question_id, "failed to resolve pending question");
+ format!("[interaction: {action_id}] (expired)")
+ }
+ }As per coding guidelines: "Don't silently discard errors. No let _ = on Results. Handle them, log them, or propagate them."
📝 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.
| if answer_labels.is_empty() { | |
| return format!("[interaction: {action_id}] (expired — no matching options)"); | |
| } | |
| // Resolve the question so duplicate clicks get the expired path | |
| let _ = store.resolve(question_id, &answer_labels).await; | |
| let labels_str = answer_labels.join(", "); | |
| format!("{sender_name} answered \"{}\": {labels_str}", q.question) | |
| if answer_labels.is_empty() { | |
| return format!("[interaction: {action_id}] (expired — no matching options)"); | |
| } | |
| // Resolve the question so duplicate clicks get the expired path. | |
| // The conditional UPDATE is the authoritative winner check. | |
| match store.resolve(question_id, &answer_labels).await { | |
| Ok(true) => { | |
| let labels_str = answer_labels.join(", "); | |
| format!("{sender_name} answered \"{}\": {labels_str}", q.question) | |
| } | |
| Ok(false) => format!("[interaction: {action_id}] (expired)"), | |
| Err(error) => { | |
| tracing::warn!(%error, question_id, "failed to resolve pending question"); | |
| format!("[interaction: {action_id}] (expired)") | |
| } | |
| } |
🤖 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/agent/channel.rs` around lines 185 - 193, Use the Result returned by
QuestionStore::resolve in the answer-handling flow instead of discarding it with
let _. Propagate or handle resolution errors according to the surrounding
function’s conventions, and return the documented expired marker when resolve
succeeds with false so concurrent duplicate clicks do not render an answered
message.
Source: Coding guidelines
| pub async fn prune_expired(&self, ttl_days: i64) -> Result<u64> { | ||
| let cutoff = format!("-{} days", ttl_days); | ||
|
|
||
| let affected = sqlx::query( | ||
| r#" | ||
| DELETE FROM pending_questions | ||
| WHERE (resolved_at IS NOT NULL AND resolved_at < datetime('now', ?)) | ||
| OR (resolved_at IS NULL AND created_at < datetime('now', ?)) | ||
| "#, | ||
| ) | ||
| .bind(&cutoff) | ||
| .bind(&cutoff) | ||
| .execute(&self.pool) | ||
| .await | ||
| .context("failed to prune expired questions")? | ||
| .rows_affected(); | ||
|
|
||
| Ok(affected) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find callers of prune_expired and DEFAULT_QUESTION_TTL_DAYS.
rg -nP -C3 '\bprune_expired\s*\(' --type=rust
rg -nP -C3 '\bDEFAULT_QUESTION_TTL_DAYS\b' --type=rustRepository: spacedriveapp/spacebot
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- tracked Rust files ---'
git ls-files '*.rs' | sed -n '1,120p'
printf '%s\n' '--- all tracked references ---'
rg -n -P -C3 '\bprune_expired\s*\(|\bDEFAULT_QUESTION_TTL_DAYS\b' --hidden -g '!target' -g '!node_modules' . || true
printf '%s\n' '--- questions.rs structure ---'
ast-grep outline src/questions.rs --lang rust || true
printf '%s\n' '--- questions.rs relevant range ---'
cat -n src/questions.rs | sed -n '1,220p'Repository: spacedriveapp/spacebot
Length of output: 12465
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
import subprocess
from pathlib import Path
files = subprocess.check_output(["git", "ls-files", "*.rs"], text=True).splitlines()
patterns = ("prune_expired", "DEFAULT_QUESTION_TTL_DAYS", "QuestionStore")
for pattern in patterns:
matches = []
for filename in files:
text = Path(filename).read_text(errors="replace").splitlines()
for number, line in enumerate(text, 1):
if pattern in line:
matches.append(f"{filename}:{number}:{line.strip()}")
print(f"--- {pattern} ({len(matches)} matches) ---")
print("\n".join(matches) or "(none)")
PY
printf '%s\n' '--- non-Rust references ---'
rg -n -P -C2 '\bprune_expired\b|\bDEFAULT_QUESTION_TTL_DAYS\b|\bQuestionStore\b' --hidden -g '!target' -g '!node_modules' . || trueRepository: spacedriveapp/spacebot
Length of output: 3597
Schedule QuestionStore::prune_expired(DEFAULT_QUESTION_TTL_DAYS)
No caller exists. Without periodic pruning, pending_questions can grow without bound.
🤖 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/questions.rs` around lines 146 - 164, Call QuestionStore::prune_expired
with DEFAULT_QUESTION_TTL_DAYS from an appropriate periodic maintenance or
startup path, ensuring the operation runs automatically without requiring an
external caller. Reuse the existing QuestionStore instance and preserve its
error handling while preventing pending_questions from growing indefinitely.
- Add #[allow(clippy::too_many_arguments)] to AskTool::new (8 args) - Rename q parameter to question in QuestionStore::insert
- Align now_sqlite timestamp format with SQLite CURRENT_TIMESTAMP so
datetime comparisons in prune_expired are consistent
- Replace .ok() with try_get::<Option<String>, _> for proper error
propagation in question_from_row
- Log resolve failures instead of silently discarding them
- Remove full question text from info-level tracing (security)
- Auto-prune expired questions on insert to prevent unbounded growth
- Replace Debug-style {:?} formatting of values with plain join
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/questions.rs (2)
146-147: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject negative
ttl_daysvalues.For
ttl_days = -1, the code builds"--1 days". SQLite evaluates this invalid modifier asNULL, so both delete predicates match no rows. Validatettl_daysbefore constructingcutoff.🤖 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/questions.rs` around lines 146 - 147, Update prune_expired to validate ttl_days before constructing the cutoff string, rejecting negative values with the function’s existing error type; preserve the current expiration-pruning behavior for zero and positive values.
122-141: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftScope question resolution to the interaction channel.
pending_questionsstoreschannel_id, butgetandresolvematch onlyquestion_id. Pass the inbound channel ID through and include it in both predicates. Do not rely only on message routing for this authorization boundary.🤖 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/questions.rs` around lines 122 - 141, Update the pending-question lookup and resolution flow, including get and resolve, to accept the inbound channel ID and include channel_id alongside question_id in both SQL predicates. Propagate the channel ID from the interaction entry point through these calls, preserving the existing unresolved-question condition in resolve.
🤖 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.
Outside diff comments:
In `@src/questions.rs`:
- Around line 146-147: Update prune_expired to validate ttl_days before
constructing the cutoff string, rejecting negative values with the function’s
existing error type; preserve the current expiration-pruning behavior for zero
and positive values.
- Around line 122-141: Update the pending-question lookup and resolution flow,
including get and resolve, to accept the inbound channel ID and include
channel_id alongside question_id in both SQL predicates. Propagate the channel
ID from the interaction entry point through these calls, preserving the existing
unresolved-question condition in resolve.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: deab35a0-3737-47f4-b819-4ae45f017e95
📒 Files selected for processing (2)
src/questions.rssrc/tools/ask.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tools/ask.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/questions.rs (1)
97-107: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftCoalesce background pruning and index both expiration branches.
Each successful
QuestionStore::insertstarts a pruning task. Overlappingprune_expireddeletes repeat work and contend for SQLite writes during bursts. The current plan filterscreated_atafter usingresolved_atfor unanswered rows.Run pruning once per interval or from one periodic task. Add a composite
(resolved_at, created_at)index and verify both expiration branches use it. Keep cleanup atomic if you split theORcondition.🤖 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/questions.rs` around lines 97 - 107, Coalesce the background pruning launched by QuestionStore::insert into a single periodic or otherwise deduplicated task so concurrent inserts do not overlap prune_expired calls. Update the pending_questions schema with a composite index on (resolved_at, created_at), and adjust prune_expired so both expiration branches can use the index while preserving atomic cleanup when splitting the OR condition.
🤖 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.
Nitpick comments:
In `@src/questions.rs`:
- Around line 97-107: Coalesce the background pruning launched by
QuestionStore::insert into a single periodic or otherwise deduplicated task so
concurrent inserts do not overlap prune_expired calls. Update the
pending_questions schema with a composite index on (resolved_at, created_at),
and adjust prune_expired so both expiration branches can use the index while
preserving atomic cleanup when splitting the OR condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e512095-f2bc-49e6-abc3-a71f8ac884b6
📒 Files selected for processing (3)
src/agent/channel.rssrc/questions.rssrc/tools/ask.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tools/ask.rs
- src/agent/channel.rs
1 participant
Adds an
asktool — a first-class question primitive with selectable options, rendered as buttons or a select menu on platforms that support them, with a numbered-text fallback on text-only channels.What's included
src/tools/ask.rs— TheAskTool. Takes a question + 2–10 options, buildsInteractiveElements(buttons for ≤5 options, select menu otherwise), sends via the existingRichMessagepath, and persists to the question store. Answer enrichment tells the model to end its turn without speculating.src/questions.rs—QuestionStore: SQLite-backed CRUD for pending questions. Survives restarts. Timestamps follow project conventions (TIMESTAMPwithCURRENT_TIMESTAMPdefault).migrations/20260810000002_pending_questions.sql— Newpending_questionstable with indexes for channel lookup and resolution state.src/agent/channel.rs— Answer correlation at both flattening points. Interactions withask:-prefixedaction_ids are enriched from the store:Alice answered "Which environment?": staging. Already-resolved and expired questions get an(expired)marker. Non-ask interactions pass through unchanged.Tool registration and prompt template — Registered in
add_channel_tools/remove_channel_tools, available on every channel withallow_direct_reply.Rendering rules
What works today
Discord works end-to-end with zero adapter changes — the existing
RichMessage+Interactionpath handles buttons and select menus natively. Text-only channels get the numbered fallback.Not in this PR
Per the design doc (
docs/design-docs/ask-tool.md), the remaining phases are:InlineKeyboardMarkup+callback_queryVerification
cargo check— zero warningscargo fmt— cleancargo test --lib— 1150/1150 passedpreflight.sh --ci— passed