scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-09-24 00:51:19 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

fix(slack): make per-button action_id unique to satisfy Slack's invalid_blocks check

Browse source 
Slack rejects chat.postMessage with `invalid_blocks` when two block elements
in the same message share an `action_id`. The interview block builder
previously stamped every button with the constant `"interview.answer"`, so
any multi-button question (yes/no, confirmation, multiple_choice with N>1)
came back with:

  {
    "ok": false,
    "error": "invalid_blocks",
    "errors": [
      "`action_id` \"interview.answer\" already exists [json-pointer:/blocks/1/elements/1/action_id]"
    ]
  }

SlackService::handle_event in fabro-server swallows the error via
`if let Ok(posted) = self.client.post_message(...)`, so this failure was
invisible at INFO log levels.

Outbound: append a per-button suffix to the action_id:
  - YesNo/Confirmation -> `interview.answer.yes` / `interview.answer.no`
  - MultipleChoice     -> `interview.answer.<index>`

The selected option key is still carried in the button `value` payload, so
the action_id only needs to be unique for routing — not semantic. Using the
option index (not the raw key) keeps suffixes short, ASCII, and dodges
Slack's 255-char action_id cap when author-supplied option keys are long.

Inbound: parse_interaction now matches both the legacy exact-prefix shape
(for in-flight messages posted by older builds) and the suffixed shape via a
pre-computed `ANSWER_ACTION_ID_PREFIX_DOT` constant (avoids per-parse
format! allocation). A lookalike like `interview.answers.yes` is correctly
rejected.

Tests:
- 4 new parse_interaction tests (suffixed yes/no, suffixed multi-choice,
  legacy exact prefix, lookalike rejection)
- Updated existing block-builder tests to assert per-button uniqueness and
  prove the option key still survives in the value payload
- Sync test that ANSWER_ACTION_ID_PREFIX_DOT matches the canonical prefix
- Updated one dispatch.rs and one connection.rs fixture to the suffixed
  shape for clarity; left one legacy fixture in each to document the
  backwards-compatibility branch
- 74/74 fabro-slack tests pass (was 69/69)
- `cargo +nightly-2026-04-14 fmt --check --all` and
  `cargo +nightly-2026-04-14 clippy -p fabro-slack --all-targets -- -D warnings`
  both clean

Verified end-to-end against a real Slack workspace: a multiple_choice
"Approve Plan" gate now renders correctly in the configured channel with
distinct [A] Approve / [R] Revise buttons.
This commit is contained in:
David Julia 2026-05-12 21:01:16 -06:00
parent 264cac3c64
commit 3819b6e19f
No known key found for this signature in database
4 changed files with 136 additions and 11 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

45
lib/crates/fabro-slack/src/blocks.rs
View file

@ -4,11 +4,23 @@ use serde_json::{Value, json};
use crate::payload::{SlackActionPayload, encode_action_value};
const ANSWER_ACTION_ID: &str = "interview.answer";
pub(crate) const ANSWER_ACTION_ID_PREFIX: &str = "interview.answer";
const MULTI_SELECT_BLOCK_ID: &str = "interview.checkboxes";
const MULTI_SELECT_ACTION_ID: &str = "interview.select";
const MULTI_SELECT_SUBMIT_ACTION_ID: &str = "interview.submit";
/// Build a Slack-unique `action_id` for an interview button.
///
/// Slack requires `action_id`s to be unique within a single message and caps
/// them at 255 characters. The selected option is carried in the button
/// `value` payload, so the `action_id` only needs to be unique — it doesn't
/// have to encode the selection. Suffixes are short, fixed-shape tokens
/// (`yes`, `no`, or the option index) to avoid any character-set or length
/// concerns when option keys are author-supplied.
fn answer_action_id(suffix: &str) -> String {
format!("{ANSWER_ACTION_ID_PREFIX}.{suffix}")
}
fn text_block(text: &str) -> Value {
json!({
"type": "section",
@ -48,11 +60,11 @@ pub fn question_to_blocks(run_id: &str, question_id: &str, question: &Question)
button("Yes", &encode_action_value(&SlackActionPayload::Yes {
run_id: run_id.to_string(),
qid: question_id.to_string(),
}), ANSWER_ACTION_ID),
}), &answer_action_id("yes")),
button("No", &encode_action_value(&SlackActionPayload::No {
run_id: run_id.to_string(),
qid: question_id.to_string(),
}), ANSWER_ACTION_ID),
}), &answer_action_id("no")),
]
});
vec![section, actions]
@ -61,7 +73,8 @@ pub fn question_to_blocks(run_id: &str, question_id: &str, question: &Question)
let elements: Vec<Value> = question
.options
.iter()
.map(|opt| {
.enumerate()
.map(|(idx, opt)| {
button(
&opt.label,
&encode_action_value(&SlackActionPayload::Selected {
@ -69,7 +82,7 @@ pub fn question_to_blocks(run_id: &str, question_id: &str, question: &Question)
qid: question_id.to_string(),
key: opt.key.clone(),
}),
ANSWER_ACTION_ID,
&answer_action_id(&idx.to_string()),
)
})
.collect();
@ -185,7 +198,23 @@ mod tests {
let elements = actions["elements"].as_array().unwrap();
assert_eq!(elements.len(), 3);
assert_eq!(elements[0]["text"]["text"], "Rust");
assert_eq!(elements[0]["action_id"], ANSWER_ACTION_ID);
assert_eq!(elements[0]["action_id"], "interview.answer.0");
assert_eq!(elements[1]["action_id"], "interview.answer.1");
assert_eq!(elements[2]["action_id"], "interview.answer.2");
// Slack requires action_id to be unique within a message.
let ids: std::collections::HashSet<&str> = elements
.iter()
.map(|e| e["action_id"].as_str().unwrap())
.collect();
assert_eq!(ids.len(), elements.len());
// The option key remains in the button `value` payload so the server
// can still route the answer regardless of suffix scheme.
assert!(
elements[0]["value"]
.as_str()
.unwrap()
.contains("\"key\":\"rs\"")
);
assert!(
elements[0]["value"]
.as_str()
@ -217,7 +246,9 @@ mod tests {
let actions = &blocks_json[1];
let elements = actions["elements"].as_array().unwrap();
assert_eq!(elements[0]["action_id"], ANSWER_ACTION_ID);
assert_eq!(elements[0]["action_id"], "interview.answer.yes");
assert_eq!(elements[1]["action_id"], "interview.answer.no");
assert_ne!(elements[0]["action_id"], elements[1]["action_id"]);
let value = elements[0]["value"].as_str().unwrap();
assert!(value.contains("\"run_id\":\"run-7\""));
assert!(value.contains("\"qid\":\"q-7\""));

2
lib/crates/fabro-slack/src/connection.rs
View file

@ -225,7 +225,7 @@ mod tests {
"team": { "id": "T123" },
"user": { "id": "U123", "name": "ada" },
"actions": [{
"action_id": "interview.answer",
"action_id": "interview.answer.yes",
"type": "button",
"value": "{\"kind\":\"yes\",\"run_id\":\"run-1\",\"qid\":\"q-1\"}"
}]

2
lib/crates/fabro-slack/src/dispatch.rs
View file

@ -77,7 +77,7 @@ mod tests {
"team": { "id": "T123" },
"user": { "id": "U123", "name": "ada" },
"actions": [{
"action_id": "interview.answer",
"action_id": "interview.answer.yes",
"type": "button",
"value": "{\"kind\":\"yes\",\"run_id\":\"run-1\",\"qid\":\"q-1\"}"
}]

98
lib/crates/fabro-slack/src/interaction.rs
View file

@ -1,13 +1,23 @@
use fabro_interview::Answer;
use serde_json::Value;
use crate::blocks::ANSWER_ACTION_ID_PREFIX;
use crate::payload::{self, SlackActionPayload, SlackAnswerSubmission};
const MULTI_SELECT_BLOCK_ID: &str = "interview.checkboxes";
const MULTI_SELECT_ACTION_ID: &str = "interview.select";
const ANSWER_ACTION_ID: &str = "interview.answer";
const MULTI_SELECT_SUBMIT_ACTION_ID: &str = "interview.submit";
/// Buttons for the same question must each have a unique `action_id`, so the
/// outbound side stamps `interview.answer.<suffix>` per element. This matches
/// either the exact prefix (legacy compatibility for messages posted before
/// the suffix scheme) or the suffixed form (current).
const ANSWER_ACTION_ID_PREFIX_DOT: &str = "interview.answer.";
fn is_answer_action(action_id: &str) -> bool {
action_id == ANSWER_ACTION_ID_PREFIX || action_id.starts_with(ANSWER_ACTION_ID_PREFIX_DOT)
}
/// Parses a Slack interaction payload and returns a server-routable answer
/// submission.
pub fn parse_interaction(payload: &Value) -> Option<SlackAnswerSubmission> {
@ -25,7 +35,7 @@ pub fn parse_interaction(payload: &Value) -> Option<SlackAnswerSubmission> {
let action_type = action["type"].as_str().unwrap_or("button");
let answer = match action_type {
"button" if action_id == ANSWER_ACTION_ID => match routed {
"button" if is_answer_action(action_id) => match routed {
SlackActionPayload::Yes { .. } => Answer::yes(),
SlackActionPayload::No { .. } => Answer::no(),
SlackActionPayload::Selected { key, .. } => Answer {
@ -255,4 +265,88 @@ mod tests {
});
assert!(parse_interaction(&payload).is_none());
}
/// Suffixed `action_id`s (per-button uniqueness for Slack) must still
/// route to the correct answer.
#[test]
fn parse_suffixed_yes_action_id() {
let payload = serde_json::json!({
"type": "block_actions",
"team": { "id": "T123" },
"user": { "id": "U123", "name": "ada" },
"actions": [{
"action_id": "interview.answer.yes",
"type": "button",
"value": "{\"kind\":\"yes\",\"run_id\":\"run-1\",\"qid\":\"q-1\"}"
}]
});
let submission = parse_interaction(&payload).unwrap();
assert_eq!(submission.answer.value, AnswerValue::Yes);
}
#[test]
fn parse_suffixed_multiple_choice_action_id() {
// `interview.answer.<index>` is what `question_to_blocks` now produces
// for multiple_choice questions.
let payload = serde_json::json!({
"type": "block_actions",
"team": { "id": "T123" },
"user": { "id": "U123", "name": "ada" },
"actions": [{
"action_id": "interview.answer.2",
"type": "button",
"value": "{\"kind\":\"selected\",\"run_id\":\"run-1\",\"qid\":\"q-1\",\"key\":\"py\"}"
}]
});
let submission = parse_interaction(&payload).unwrap();
assert_eq!(
submission.answer.value,
AnswerValue::Selected("py".to_string())
);
}
/// Legacy `action_id` without a suffix must still parse so messages
/// posted by older Fabro builds remain clickable after upgrade.
#[test]
fn parse_legacy_unsuffixed_action_id() {
let payload = serde_json::json!({
"type": "block_actions",
"team": { "id": "T123" },
"user": { "id": "U123", "name": "ada" },
"actions": [{
"action_id": "interview.answer",
"type": "button",
"value": "{\"kind\":\"yes\",\"run_id\":\"run-1\",\"qid\":\"q-1\"}"
}]
});
let submission = parse_interaction(&payload).unwrap();
assert_eq!(submission.answer.value, AnswerValue::Yes);
}
/// Action ids that merely share a prefix but are not the answer family
/// must not be misrouted (no false-positive prefix match).
#[test]
fn rejects_lookalike_action_id() {
let payload = serde_json::json!({
"type": "block_actions",
"team": { "id": "T123" },
"user": { "id": "U123", "name": "ada" },
"actions": [{
"action_id": "interview.answers.yes",
"type": "button",
"value": "{\"kind\":\"yes\",\"run_id\":\"run-1\",\"qid\":\"q-1\"}"
}]
});
assert!(parse_interaction(&payload).is_none());
}
/// The dotted prefix constant must stay in sync with the canonical
/// prefix so outbound and inbound never drift.
#[test]
fn dotted_prefix_constant_matches_canonical_prefix() {
assert_eq!(
ANSWER_ACTION_ID_PREFIX_DOT,
format!("{ANSWER_ACTION_ID_PREFIX}.")
);
}
}