scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-10-09 03:20:56 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

fix(slack): make per-button action_id unique (re #251 ) (#252)

Browse source 
Heads up:
[CONTRIBUTING.md](https://github.com/fabro-sh/fabro/blob/main/CONTRIBUTING.md)
says you don't accept outside PRs, *"Instead of accepting outside pull
requests, we accept bug reports and feature requests as GitHub Issues."*
I filed the canonical bug report as **#251**, and that's where any
actual discussion belongs.

This PR is a courtesy ready-made diff in case it's useful to whoever
supervises the AI workflow that lands this fix. Feel free to close it
without comment; nothing is being asked of you here. I just thought it'd
be useful to have a reference for what I did locally to fix it.

## What

Two-file behaviour fix in `lib/crates/fabro-slack/`: every interview
button on a multi-button gate now gets a Slack-unique `action_id`. Today
they all share `"interview.answer"`, so Slack rejects the
`chat.postMessage` with `invalid_blocks` and
`SlackService::handle_event` silently drops the error.

## Why

Multi-button Slack interview gates never reach Slack. Full reproducer,
MITM-captured `invalid_blocks` response, and root-cause walkthrough are
in **#251**.

## Diff shape

- `blocks.rs`: each button gets a unique suffix.
- `YesNo` / `Confirmation`: `interview.answer.yes` /
`interview.answer.no`
- `MultipleChoice`: `interview.answer.<index>` (index, not raw key, to
dodge Slack's 255-char `action_id` cap and any author-supplied charset
surprises; selected key still rides in the button `value`)
- `interaction.rs`: `parse_interaction` accepts both the legacy
exact-prefix shape (in-flight buttons keep working across upgrade) and
the new suffixed shape, via a pre-computed `ANSWER_ACTION_ID_PREFIX_DOT`
constant so the parse hot path doesn't `format!` on every event.
- Tests: +5 in `interaction.rs` (suffixed yes/no, suffixed multi-choice,
legacy exact prefix, lookalike `interview.answers.yes` rejected,
prefix-sync assertion). Updated the existing block-builder tests to
assert uniqueness instead of the old single constant. One fixture each
in `dispatch.rs` and `connection.rs` updated to the suffixed shape; one
legacy fixture left in each to document backwards compatibility.

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` clean.

## Verified end-to-end

Built a patched `fabro` binary, swapped it for the brew install,
triggered a fresh multi-choice `Approve Plan` gate against a real Slack
workspace, message rendered correctly in the configured channel with two
clickable `[A] Approve` and `[R] Revise` buttons. Before the patch, the
exact same gate produced zero Slack output and only the swallowed
`invalid_blocks` was visible via MITM.

## Suggested follow-up (separate concern, not in this diff)

The silent error swallow in `SlackService::handle_event` (`if let
Ok(posted) = self.client.post_message(...)`) is what hid this bug. Worth
logging at `WARN`. Mentioned in #251 as a separate item.

## Closes

Closes #251 if you choose to land this directly. Otherwise this PR is
just background material for the issue.
This commit is contained in:
David Julia 2026-05-13 05:32:43 -06:00 • committed by GitHub
parent 6480c333ff
commit 69ce83be43
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
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}.")
);
}
}