mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-06 08:18:58 +00:00
a591a1ca63
This branch contains both commits: 1. The action_id uniqueness fix from #252 (`fix(slack): make per-button action_id unique to satisfy Slack's invalid_blocks check`). 2. The new context+link fix (`feat(slack): render context_display and run link in interview messages`). The first commit needs to land (or be rebuilt by the maintainer workflow) before the second is meaningful, because without it multi-button gates still fail with `invalid_blocks` and the new context block never reaches Slack. Reviewing #252 first and this issue/PR second is the cleanest flow. ## What Two-file change in `lib/crates/fabro-slack/` and `lib/crates/fabro-server/`. The Slack interview message now includes the upstream stage's response (`context_display`) and a link back to the run, so a reviewer in Slack can decide A vs R without opening the web UI. ## Why See **#253** for the full repro, screenshots, and threat model. Short version: today the Slack message contains only the hexagon node's `label` plus the buttons, which is not enough information to act on. ## Diff shape - `question_to_blocks` gains a `run_web_url: Option<&str>` argument. - New helpers: - `header_section(question, run_web_url)`: bold question text, optional `stage \`{stage}\`` hint, and an "Open in Fabro" link when the URL is known. - `context_section(context_display)`: renders the upstream stage's response (e.g. plan summary, Dossier URLs) below the header, separated by a `divider`. Empty context_display is skipped so the message falls back cleanly to the old two-block shape. - `escape_slack_controls(text)`: HTML-entity escapes `&`, `<`, `>` in untrusted strings (question, stage, context_display, answered_blocks question and answer text). Neutralises LLM-produced payloads like `<!here>`, `<@U…>`, `<#C…>` while leaving Markdown formatting (`*bold*`, `_italic_`, `` `code` ``, `~strike~`) intact. Per https://docs.slack.dev/messaging/formatting-message-text/#escaping. - `truncate_to_limit`: clamps both the header text and the context block against Slack's documented 3000-character section text limit, with the truncation suffix counted against the budget so the result is always under the cap. Defends against pathological questions or oversized LLM responses producing `invalid_blocks`. - `server.rs`: `start_optional_slack_service`'s event subscriber calls `state.run_web_url(&envelope.event.run_id)` per event and forwards the result to `SlackService::handle_event`, which threads it into `question_to_blocks`. Returns `None` (and the link is omitted) when `server.web.enabled` is `false` or `server.web.url` is unset. ## Tests 10 new in `lib/crates/fabro-slack/src/blocks.rs`: - `header_includes_run_link_when_url_provided` - `header_omits_link_when_url_missing` - `header_shows_stage_when_present` - `header_truncates_when_inputs_exceed_section_limit` - `context_display_renders_between_header_and_actions` - `context_display_truncates_oversized_text_to_fit_slack_budget` - `empty_context_display_is_skipped` - `slack_control_chars_in_question_text_are_escaped` - `slack_control_chars_in_context_display_are_escaped` (covers `<!here>`/`<@U…>`/`<#C…>` neutralisation and verifies Markdown survives) - `answered_blocks_escape_slack_control_chars` 84/84 `fabro-slack` tests pass (was 74 after #252). `cargo +nightly-2026-04-14 fmt --check --all` and `cargo +nightly-2026-04-14 clippy -p fabro-slack -p fabro-server --all-targets -- -D warnings` both clean. ## Verified end-to-end Built a patched binary, swapped it for the brew install, triggered a fresh multi-choice approve gate against a real Slack workspace. The Slack message now renders with bold "Approve Plan" header, `stage \`approve\`` hint, "Open in Fabro" link, the upstream plan summary block (Dossier canonical and version URLs, `tmp-docs/fabro-plan.html` artifact path, the plan-summary bullets), a divider, and the `[A] Approve` and `[R] Revise` buttons. Reviewer can act on the gate from Slack alone. ## Latent observation, not in this diff `lib/crates/fabro-server/src/server.rs::AppState::run_web_url` has a comment saying it is snapshotted at create-time so attach replays remain stable when `server.web.url` changes, but the implementation reads current settings via `server_settings()`/`canonical_origin()`. This patch is unaffected (per-event call), but the comment looks stale. ## Closes Closes #253 if you choose to land this directly. Otherwise this PR is background material for the issue. --------- Co-authored-by: Bryan Helmkamp <bryan@brynary.com> |
||
|---|---|---|
| .. | ||
| crates | ||
| packages/fabro-api-client | ||