-
Notifications
You must be signed in to change notification settings - Fork 360
feat(tools): add ask tool with pending question store and answer correlation #638
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| -- Pending questions store for the ask tool. | ||
| -- | ||
| -- When an agent calls the ask tool, the question + options are persisted here | ||
| -- so inbound interaction clicks can be correlated back to the original question. | ||
|
|
||
| CREATE TABLE IF NOT EXISTS pending_questions ( | ||
| question_id TEXT PRIMARY KEY, | ||
| agent_id TEXT NOT NULL, | ||
| channel_id TEXT NOT NULL, | ||
| question TEXT NOT NULL, | ||
| options TEXT NOT NULL, -- JSON array of AskOption | ||
| multi_select INTEGER NOT NULL DEFAULT 0, | ||
| message_ref TEXT, -- platform message id, for disabling buttons after answer | ||
| created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| resolved_at TIMESTAMP, | ||
| answer TEXT -- JSON array of picked labels | ||
| ); | ||
|
|
||
| -- Fast lookup by channel for pruning | ||
| CREATE INDEX IF NOT EXISTS idx_pending_questions_channel | ||
| ON pending_questions(channel_id, created_at DESC); | ||
|
|
||
| -- Fast lookup for resolution via inbound interaction click | ||
| CREATE INDEX IF NOT EXISTS idx_pending_questions_resolved | ||
| ON pending_questions(resolved_at); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Ask the user a question with selectable answer options. Use this when you need the user to pick between choices (which environment, which approach, proceed or not). Renders as buttons or a select menu on platforms that support them, and as a numbered list on text-only channels. The answer arrives as a future message — do not speculate about the answer; end your turn after asking. The user can also type a free-form response instead of clicking an option. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,225 @@ | ||
| //! Pending question store for the ask tool. | ||
| //! | ||
| //! Persists questions that the agent has asked the user so inbound interaction | ||
| //! clicks can be correlated back to the original question context. Restart-safe | ||
| //! by construction — questions survive process restarts. | ||
|
|
||
| use crate::error::Result; | ||
| use anyhow::Context as _; | ||
| use serde::{Deserialize, Serialize}; | ||
| use sqlx::{Row as _, SqlitePool}; | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Types | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| /// Default TTL for pending questions (7 days). | ||
| pub const DEFAULT_QUESTION_TTL_DAYS: i64 = 7; | ||
|
|
||
| /// A single option for the ask tool. | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct AskOption { | ||
| pub label: String, | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub description: Option<String>, | ||
| } | ||
|
|
||
| /// A persisted pending question row. | ||
| #[derive(Debug, Clone)] | ||
| pub struct PendingQuestion { | ||
| pub question_id: String, | ||
| pub agent_id: String, | ||
| pub channel_id: String, | ||
| pub question: String, | ||
| pub options: Vec<AskOption>, | ||
| pub multi_select: bool, | ||
| pub message_ref: Option<String>, | ||
| pub created_at: String, | ||
| pub resolved_at: Option<String>, | ||
| pub answer: Option<Vec<String>>, | ||
| } | ||
|
|
||
| /// Input for creating a pending question. | ||
| #[derive(Debug, Clone)] | ||
| pub struct NewQuestion { | ||
| pub question_id: String, | ||
| pub agent_id: String, | ||
| pub channel_id: String, | ||
| pub question: String, | ||
| pub options: Vec<AskOption>, | ||
| pub multi_select: bool, | ||
| pub message_ref: Option<String>, | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Store | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct QuestionStore { | ||
| pool: SqlitePool, | ||
| } | ||
|
|
||
| /// Timestamp matching SQLite's CURRENT_TIMESTAMP format so comparisons | ||
| /// against `datetime('now', …)` are consistent. | ||
| fn now_sqlite() -> String { | ||
| chrono::Utc::now().format("%Y-%m-%d %H:%M:%S").to_string() | ||
| } | ||
|
|
||
| impl QuestionStore { | ||
| pub fn new(pool: SqlitePool) -> Self { | ||
| Self { pool } | ||
| } | ||
|
|
||
| /// Insert a new pending question. | ||
| pub async fn insert(&self, question: &NewQuestion) -> Result<()> { | ||
| let options_json = serde_json::to_string(&question.options) | ||
| .context("failed to serialize question options")?; | ||
|
|
||
| sqlx::query( | ||
| r#" | ||
| INSERT INTO pending_questions | ||
| (question_id, agent_id, channel_id, question, options, multi_select, message_ref) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?) | ||
| "#, | ||
| ) | ||
| .bind(&question.question_id) | ||
| .bind(&question.agent_id) | ||
| .bind(&question.channel_id) | ||
| .bind(&question.question) | ||
| .bind(&options_json) | ||
| .bind(question.multi_select as i64) | ||
| .bind(&question.message_ref) | ||
| .execute(&self.pool) | ||
| .await | ||
| .context("failed to insert pending question")?; | ||
|
|
||
| // Prune expired questions in the background so the table does not | ||
| // grow unbounded. Failure is non-fatal — the next write retries. | ||
| let prune_pool = self.pool.clone(); | ||
| tokio::spawn(async move { | ||
| if let Err(error) = QuestionStore::new(prune_pool) | ||
| .prune_expired(DEFAULT_QUESTION_TTL_DAYS) | ||
| .await | ||
| { | ||
| tracing::warn!(%error, "background prune of pending_questions failed"); | ||
| } | ||
| }); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Look up a pending question by ID. | ||
| pub async fn get(&self, question_id: &str) -> Result<Option<PendingQuestion>> { | ||
| let row = sqlx::query( | ||
| r#" | ||
| SELECT question_id, agent_id, channel_id, question, options, multi_select, | ||
| message_ref, created_at, resolved_at, answer | ||
| FROM pending_questions | ||
| WHERE question_id = ? | ||
| "#, | ||
| ) | ||
| .bind(question_id) | ||
| .fetch_optional(&self.pool) | ||
| .await | ||
| .context("failed to fetch pending question")?; | ||
|
|
||
| match row { | ||
| Some(row) => Ok(Some(question_from_row(row)?)), | ||
| None => Ok(None), | ||
| } | ||
| } | ||
|
|
||
| /// Resolve a pending question with the given answer labels. | ||
| /// Returns Ok(true) if the question was found and unresolved, Ok(false) if | ||
| /// 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_sqlite(); | ||
|
|
||
| let affected = sqlx::query( | ||
| r#" | ||
| UPDATE pending_questions | ||
| SET resolved_at = ?, answer = ? | ||
| WHERE question_id = ? AND resolved_at IS NULL | ||
| "#, | ||
| ) | ||
| .bind(&now) | ||
| .bind(&answer_json) | ||
| .bind(question_id) | ||
| .execute(&self.pool) | ||
| .await | ||
| .context("failed to resolve pending question")? | ||
| .rows_affected(); | ||
|
|
||
| Ok(affected > 0) | ||
| } | ||
|
|
||
| /// Prune resolved questions older than the TTL, and unanswered questions | ||
| /// older than the TTL (expired). Returns the count of removed rows. | ||
| 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) | ||
| } | ||
|
Comment on lines
+160
to
+178
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚀 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 No caller exists. Without periodic pruning, 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Row mapping | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| fn question_from_row(row: sqlx::sqlite::SqliteRow) -> Result<PendingQuestion> { | ||
| let options_json: String = row | ||
| .try_get("options") | ||
| .context("failed to read question options")?; | ||
| let options: Vec<AskOption> = | ||
| serde_json::from_str(&options_json).context("failed to parse question options")?; | ||
|
|
||
| let answer_json: Option<String> = row | ||
| .try_get::<Option<String>, _>("answer") | ||
| .context("failed to read answer")?; | ||
| let answer = match answer_json { | ||
| Some(json) => Some(serde_json::from_str(&json).context("failed to parse question answer")?), | ||
| None => None, | ||
| }; | ||
|
|
||
| Ok(PendingQuestion { | ||
| question_id: row | ||
| .try_get("question_id") | ||
| .context("failed to read question_id")?, | ||
| agent_id: row.try_get("agent_id").context("failed to read agent_id")?, | ||
| channel_id: row | ||
| .try_get("channel_id") | ||
| .context("failed to read channel_id")?, | ||
| question: row.try_get("question").context("failed to read question")?, | ||
| options, | ||
| multi_select: row | ||
| .try_get::<i64, _>("multi_select") | ||
| .context("failed to read multi_select")? | ||
| != 0, | ||
| message_ref: row | ||
| .try_get::<Option<String>, _>("message_ref") | ||
| .context("failed to read message_ref")?, | ||
| created_at: row | ||
| .try_get("created_at") | ||
| .context("failed to read created_at")?, | ||
| resolved_at: row | ||
| .try_get::<Option<String>, _>("resolved_at") | ||
| .context("failed to read resolved_at")?, | ||
| answer, | ||
| }) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the result of
resolve; do not discard it.QuestionStore::resolvereturnsResult<bool>. Thefalsevalue 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 rendersanswered "…"for a losing concurrent click. The doc comment on line 142 promises the(expired)marker for that case. Thegetcheck 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
As per coding guidelines: "Don't silently discard errors. No
let _ =on Results. Handle them, log them, or propagate them."📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Coding guidelines