Skip to content

Preserve relay origin when replying by local event ID #174

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.

Sign up for GitHub

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

Open
signet-bot-codex wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Preserve relay origin when replying by local event ID #174

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 196 additions & 0 deletions src/commands/send.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,10 @@ pub fn send_message(
data["reply_to"] = serde_json::json!(reply_to);
// Resolve to local event ID
if let Some(local_id) = resolve_reply_to_local(db, reply_to) {
data["reply_to"] = serde_json::json!(
portable_reply_to(db, reply_to, local_id)
.ok_or("Invalid --reply-to: relay origin is malformed or ambiguous")?
);
data["reply_to_local"] = serde_json::json!(local_id);

// Ack-on-ack loop prevention
Expand Down Expand Up @@ -534,6 +538,33 @@ fn resolve_reply_to_local(db: &HcomDb, reply_to: &str) -> Option<i64> {
}
}

/// A bare local reference to an imported message must travel as its origin
/// reference. Keep explicit qualified references and genuine local references
/// unchanged. Refuse corrupt/duplicate relay identities rather than exporting a
/// device-local number that could identify an unrelated message elsewhere.
fn portable_reply_to(db: &HcomDb, supplied: &str, local_id: i64) -> Option<String> {
if crate::relay::control::split_device_suffix(supplied.trim()).is_some() {
return Some(supplied.to_string());
}
let raw: String = db
.conn()
.query_row(
"SELECT data FROM events WHERE id = ? AND type = 'message'",
[local_id],
|row| row.get(0),
)
.ok()?;
let data: serde_json::Value = serde_json::from_str(&raw).ok()?;
let Some(relay) = data.get("_relay") else {
return Some(supplied.to_string());
};
let origin_id = relay.get("id")?.as_i64()?;
let short = relay.get("short")?.as_str()?;
let qualified = format!("{origin_id}:{short}");
crate::relay::control::split_device_suffix(&qualified)?;
(resolve_reply_to_local(db, &qualified) == Some(local_id)).then_some(qualified)

@greptile-apps greptile-apps Bot Oct 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Replies slow as history grows

Each valid bare reply to an imported message now searches every message to rule out duplicate origins. resolve_reply_to_local checks JSON fields with no matching index, and both cmd_send and send_message run this check. Reply time therefore grows with the whole message history, unlike the previous lookup by local ID. Add an index for the origin fields so this required check stays cheap.

@signet-bot-codex signet-bot-codex Oct 22, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, confirmed. EXPLAIN QUERY PLAN for the resolver's query is SEARCH events USING INDEX idx_type_instance (type=?), so it visits every message row and evaluates the JSON. On a database with about 70,000 messages a lookup that finds no match takes about 0.3 s. Qualified --reply-to <id>:<device> replies have paid this once since #125; with this change a bare reply to an imported message pays it up to twice (the early refusal in cmd_send and the check in send_message), and replies to local messages pay nothing new. The proper fix is an index on the two _relay fields, for example a partial expression index on json_extract(data, '$._relay.short') and json_extract(data, '$._relay.id') where type = 'message'. It would speed up the existing qualified path too, so I would rather offer it as a separate change than widen this one. Happy to add it here if you prefer.

@greptile-apps greptile-apps Bot Oct 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for confirming and measuring the impact. Please keep the index as a separate change: this PR can stay focused on portable reply references, while the expression index can address the scan for both the existing qualified path and the new validation path. The measured 0.3 s lookup is useful context for prioritizing that follow-up.

}

/// Get thread from an event (for --reply-to thread inheritance).
fn get_thread_from_event(db: &HcomDb, event_id: i64) -> Option<String> {
db.conn()
Expand Down Expand Up @@ -836,6 +867,11 @@ pub fn cmd_send(db: &HcomDb, args: &SendArgs, ctx: Option<&CommandContext>) -> i

if let Some(ref reply_to) = envelope.reply_to {
if let Some(local_id) = resolve_reply_to_local(db, reply_to) {
// Refuse before any inline bundle is created; send_message repeats this check for its direct callers.
if portable_reply_to(db, reply_to, local_id).is_none() {
eprintln!("Error: Invalid --reply-to: relay origin is malformed or ambiguous");
return 1;
}
if envelope.thread.is_none()
&& let Some(parent_thread) = get_thread_from_event(db, local_id)
{
Expand Down Expand Up @@ -2077,6 +2113,166 @@ mod tests {
cleanup_test_db(path);
}

#[test]
#[serial]
fn test_cmd_send_bare_imported_reply_preserves_origin() {
let (db, path, _env) = setup_test_db();
db.conn().execute(
"INSERT INTO instances (name, created_at) VALUES ('sender', 1000.0), ('target', 1000.0)",
[],
).unwrap();
// A bare reply names the receiving device's local row (100), which differs
// from the origin event ID (42).
insert_imported_remote_message(
&db,
100,
serde_json::json!(42),
"BOXE",
"remote request",
Some("origin-thread"),
Some("request"),
);
db.conn().execute(
"INSERT INTO events (id, type, instance, timestamp, data)
VALUES (42, 'message', 'target', '2026-09-04T00:00:00Z', ?)",
[serde_json::json!({"from":"target", "text":"local request", "intent":"request"}).to_string()],
).unwrap();
for (supplied, expected, local) in [
("100", "42:BOXE", 100),
("42:BOXE", "42:BOXE", 100),
("42", "42", 42),
] {
let mut args = SendArgs::try_parse_from([
"send",
"@target",
"--from",
"sender",
"--intent",
"ack",
"--reply-to",
supplied,
"--",
"reply text",
])
.unwrap();
args.had_separator = true;
assert_eq!(cmd_send(&db, &args, None), 0);
let raw: String = db
.conn()
.query_row(
"SELECT data FROM events WHERE type = 'message' ORDER BY id DESC LIMIT 1",
[],
|r| r.get(0),
)
.unwrap();
let data: serde_json::Value = serde_json::from_str(&raw).unwrap();
assert_eq!(data["reply_to"], expected);
assert_eq!(data["reply_to_local"], local);
assert_eq!(data["intent"], "ack");
assert_eq!(data["text"], "reply text");
assert_eq!(data["from"], "sender");
assert_eq!(data["delivered_to"], serde_json::json!(["target"]));
if local == 100 {
assert_eq!(data["thread"], "origin-thread");
}
}
cleanup_test_db(path);
}

#[test]
#[serial]
fn test_cmd_send_bare_imported_reply_invalid_origin_emits_no_message() {
let (db, path, _env) = setup_test_db();
db.conn().execute(
"INSERT INTO instances (name, created_at) VALUES ('sender', 1000.0), ('target', 1000.0)",
[],
).unwrap();
insert_imported_remote_message(
&db,
100,
serde_json::json!(42),
"BOXE",
"request",
None,
Some("request"),
);
insert_imported_remote_message(
&db,
101,
serde_json::json!(42),
"BOXE",
"duplicate",
None,
Some("request"),
);
insert_imported_remote_message(
&db,
102,
serde_json::json!("43"),
"BOXE",
"bad ID type",
None,
Some("request"),
);
insert_imported_remote_message(
&db,
103,
serde_json::json!(44),
"boxe",
"bad suffix",
None,
Some("request"),
);
let count = || {
db.conn()
.query_row("SELECT COUNT(*) FROM events", [], |r| r.get::<_, i64>(0))
.unwrap()
};
let before = count();
for supplied in ["100", "101", "102", "103", "999", "42:BOXE"] {
let mut args = SendArgs::try_parse_from([
"send",
"@target",
"--from",
"sender",
"--reply-to",
supplied,
"--",
"reply",
])
.unwrap();
args.had_separator = true;
assert_eq!(cmd_send(&db, &args, None), 1, "must refuse {supplied}");
assert_eq!(count(), before, "must not store a reply to {supplied}");
}
// The refusal comes before an inline bundle, so a refused reply creates no bundle event either.
let mut args = SendArgs::try_parse_from([
"send",
"@target",
"--from",
"sender",
"--reply-to",
"100",
"--title",
"context",
"--description",
"bundle with a refused reply",
"--events",
"100",
"--files",
"src/main.rs",
"--transcript",
"1-2:normal",
"--",
"reply",
])
.unwrap();
args.had_separator = true;
assert_eq!(cmd_send(&db, &args, None), 1, "must refuse a bundled reply");
assert_eq!(count(), before, "must not create a bundle for a refused reply");
cleanup_test_db(path);
}

#[test]
#[serial]
fn test_cmd_send_unresolved_remote_reply_exits_one() {
Expand Down
Loading