From 96372c5a3ce7d1bc06aefb216dcc137634e18e47 Mon Sep 17 00:00:00 2001 From: "fabro-sh-fabro[bot]" <296591931+fabro-sh-fabro[bot]@users.noreply.github.com> Date: Fri, 10 Jul 2026 08:29:54 -0400 Subject: [PATCH] Demote `server.integrations.slack.default_channel` to a plain string (#565) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `server.integrations.slack.default_channel` field was typed `Option` but was never documented as interpolable — every doc example uses a plain channel name like `#releases`. It resolved only env vars, only once at server startup, and that capability was inherited from a uniform schema-staging design, not a deliberate feature. This brings it in line with every other server-scope config field, which were already demoted to plain literals under the project rule that interpolation belongs to fields resolved with run context. ## What changed - **Type** (`fabro-types`, `fabro-config` layers): `Option` → `Option` in `SlackIntegrationSettings` and `SlackIntegrationLayer`. - **Demotion warning** (`resolve/server.rs`): calls `warn_if_demoted_template` at resolve time with the field path `server.integrations.slack.default_channel`, matching the pattern used for earlier server-field demotions. A value still containing a `{{ env.* }}`-shaped token is stored verbatim and triggers a startup warning — no resolution, no error. - **Startup wiring** (`server.rs`): the `value.resolve(process_env_var)` call and its error mapping are deleted; the literal string is passed directly to `SlackService::new`, which already accepts `Option`. - **System status handler** (`handler/system.rs`): removed the now-unnecessary `display_interp` helper that called `resolve_or_source`; the field is cloned directly into the metadata map. - **Wire shape**: unchanged. `InterpString` serialized as its raw source string, so stored/wire JSON is identical before and after. The OpenAPI spec is untouched. ## What is not changing Per-run Slack channels — `run.notifications..slack.channel` and `run.interviews.slack.channel` — remain `InterpString` with variable substitution at run creation. Those are the intended interpolating surface and are correct as-is. ## Migration signal Anyone who placed a `{{ env.NAME }}` token in `server.integrations.slack.default_channel` (only possible during ~3 months of nightly builds) will see a startup warning naming the field. The value is treated as a literal; no data is lost and startup does not fail. ## Interview-prompt routing observation (step 4) The interview-prompt posting path checks `run.interviews.slack.channel` first and falls back to the server default only when the run-scope field is absent — the preference already exists. No routing change is needed or made here. ### Fabro Details
Ran 8 stages in 41m 23s for $10.10 | Stage | Duration | Cost | Retries | |---|---|---|---| | start | 0s | – | 0 | | toolchain | 1s | – | 0 | | preflight_compile | 2m 41s | – | 0 | | preflight_lint | 2m 42s | – | 0 | | implement | 0s | – | 0 | | simplify_fable | 27m 43s | $10.10 | 0 | | simplify_gpt | 0s | – | 0 | | verify | 7m 44s | – | 0 | | **Total** | **41m 23s** | **$10.10** | **0** |
Ran ImplementPlan.fabro (11 nodes and 14 edges) ```dot digraph ImplementPlan { graph [ goal="Implement and simplify", model_stylesheet=" * { model: claude-opus-4-8; } " ] rankdir=LR start [shape=Mdiamond, label="Start"] exit [shape=Msquare, label="Exit"] toolchain [label="Toolchain", shape=parallelogram, script="command -v cargo >/dev/null || { curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && sudo ln -sf $HOME/.cargo/bin/* /usr/local/bin/; }; cargo --version 2>&1", max_retries=0] preflight_compile [label="Preflight Compile", shape=parallelogram, script="cargo check -q --workspace 2>&1", max_retries=0] preflight_lint [label="Preflight Lint", shape=parallelogram, script="cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1", max_retries=0] fix_lints [label="Fix Lints", prompt="The preflight lint step failed. Read the build output from context and fix all clippy lint warnings.", max_visits=3] implement [label="Implement", prompt="Read the plan file referenced in the goal and implement every step. Make all the code changes described in the plan. Use red/green TDD. Be sure to use the rust-style-guide skill to help you follow this repo's Rust style conventions.", model="gpt-55", reasoning_effort="xhigh"] simplify_fable [label="Simplify (Fable)", prompt="@prompts/simplify.md", model="claude-fable-5", reasoning_effort="xhigh"] simplify_gpt [label="Simplify (GPT-55)", prompt="@prompts/simplify.md", model="gpt-55"] verify [label="Verify", shape=parallelogram, timeout="1800s", script="git fetch origin main 2>&1 && git merge --no-edit --no-stat origin/main 2>&1 && cargo +nightly-2026-04-14 fmt --all 2>&1 && cargo dev docs refresh 2>&1 && cargo +nightly-2026-04-14 fmt --check --all 2>&1 && { command -v rg >/dev/null 2>&1 || { echo 'rg is required for verify'; exit 127; }; } && ! rg -n 'AuthMode::Disabled|RunAuthMethod|RunSubjectProvenance|\bActorRef\b|\bActorKind\b|AuthenticatedSubject|AuthenticatedService|AuthorizeRunScoped|AuthorizeRunBlob|AuthorizeStageArtifact|AuthorizeCommandLog|auth_method\s*==\s*\"disabled\"' lib/crates apps lib/packages docs/public/api-reference/fabro-api.yaml 2>&1 && cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings 2>&1 && cargo nextest run --workspace --status-level slow --profile ci 2>&1 && cargo dev docs check 2>&1 && bun install --frozen-lockfile 2>&1 && (cd apps/fabro-web && bun run typecheck) 2>&1 && (cd apps/fabro-web && bun run test) 2>&1 && (cd lib/packages/fabro-api-client && bun run typecheck) 2>&1 && cargo dev build -- -p fabro-cli --release 2>&1", goal_gate=true, retry_target="fixup"] fixup [label="Fixup", prompt="The verify step failed. Read the build output from context and fix all format, clippy, Rust test, docs, TypeScript typecheck/test, and build failures.", max_visits=3] start -> toolchain toolchain -> preflight_compile [condition="outcome=succeeded"] toolchain -> exit preflight_compile -> preflight_lint [condition="outcome=succeeded"] preflight_compile -> exit preflight_lint -> implement [condition="outcome=succeeded"] preflight_lint -> fix_lints fix_lints -> preflight_lint implement -> simplify_fable -> simplify_gpt -> verify verify -> exit [condition="outcome=succeeded"] verify -> fixup fixup -> verify } ```
⚒️ Generated with [Fabro](https://fabro.sh) --------- Co-authored-by: Fabro --- lib/crates/fabro-config/src/layers/server.rs | 2 +- lib/crates/fabro-config/src/resolve/server.rs | 12 +++- .../fabro-config/src/tests/resolve_server.rs | 49 ++++++++++++++++ lib/crates/fabro-server/src/server.rs | 6 +- .../fabro-server/src/server/handler/system.rs | 10 +--- lib/crates/fabro-server/src/server/tests.rs | 58 ++++++++++++++----- lib/crates/fabro-types/src/settings/server.rs | 3 +- 7 files changed, 105 insertions(+), 35 deletions(-) diff --git a/lib/crates/fabro-config/src/layers/server.rs b/lib/crates/fabro-config/src/layers/server.rs index fb5cde79f..351d4da68 100644 --- a/lib/crates/fabro-config/src/layers/server.rs +++ b/lib/crates/fabro-config/src/layers/server.rs @@ -234,7 +234,7 @@ pub struct SlackIntegrationLayer { #[serde(default, skip_serializing_if = "Option::is_none")] pub enabled: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub default_channel: Option, + pub default_channel: Option, } #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize, fabro_macros::Combine)] diff --git a/lib/crates/fabro-config/src/resolve/server.rs b/lib/crates/fabro-config/src/resolve/server.rs index cfeffc415..39b41eaf9 100644 --- a/lib/crates/fabro-config/src/resolve/server.rs +++ b/lib/crates/fabro-config/src/resolve/server.rs @@ -364,9 +364,15 @@ fn resolve_integrations(layer: Option<&ServerIntegrationsLayer>) -> ServerIntegr enabled: false, default_channel: None, }, - |slack| SlackIntegrationSettings { - enabled: slack.enabled.unwrap_or(true), - default_channel: slack.default_channel.clone(), + |slack| { + warn_if_demoted_template( + "server.integrations.slack.default_channel", + slack.default_channel.as_deref(), + ); + SlackIntegrationSettings { + enabled: slack.enabled.unwrap_or(true), + default_channel: slack.default_channel.clone(), + } }, ), } diff --git a/lib/crates/fabro-config/src/tests/resolve_server.rs b/lib/crates/fabro-config/src/tests/resolve_server.rs index a8619f3be..e4eecb353 100644 --- a/lib/crates/fabro-config/src/tests/resolve_server.rs +++ b/lib/crates/fabro-config/src/tests/resolve_server.rs @@ -145,6 +145,55 @@ _version = 1 assert!(settings.integrations.slack.default_channel.is_none()); } +#[test] +fn resolve_slack_default_channel_passes_literal_through() { + let settings = resolve_server(&parse( + r##" +_version = 1 + +[server.integrations.slack] +default_channel = "#releases" +"##, + )); + + assert_eq!( + settings.integrations.slack.default_channel.as_deref(), + Some("#releases") + ); + // Wire shape is unchanged by the plain-string demotion: the field still + // serializes as its raw string. + let slack = serde_json::to_value(&settings.integrations.slack) + .expect("slack settings should serialize"); + assert_eq!( + slack, + serde_json::json!({ + "enabled": true, + "default_channel": "#releases", + }) + ); +} + +#[test] +fn resolve_slack_default_channel_keeps_template_token_literal() { + // `server.integrations.slack.default_channel` is a plain literal now: a + // `{{ env.* }}` token is stored verbatim and never interpolated. Per-run + // Slack channels (`run.notifications`, `run.interviews.slack`) remain the + // interpolating surface. + let settings = resolve_server(&parse( + r#" +_version = 1 + +[server.integrations.slack] +default_channel = "{{ env.SLACK_DEFAULT_CHANNEL }}" +"#, + )); + + assert_eq!( + settings.integrations.slack.default_channel.as_deref(), + Some("{{ env.SLACK_DEFAULT_CHANNEL }}") + ); +} + #[test] fn server_sandbox_defaults_all_providers_enabled() { let settings = ServerSettingsBuilder::from_toml( diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index f51a299b6..8ba881fe1 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -2422,11 +2422,7 @@ pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result String { - value.resolve_or_source(|name| (state.env_lookup)(name)) -} - fn slack_integration_status(state: &AppState) -> SystemIntegrationStatus { let settings = &state.server_settings().server.integrations.slack; let mut metadata = BTreeMap::new(); if let Some(default_channel) = settings.default_channel.as_ref() { - metadata.insert( - "default_channel".to_string(), - display_interp(state, default_channel), - ); + metadata.insert("default_channel".to_string(), default_channel.clone()); } if !settings.enabled { diff --git a/lib/crates/fabro-server/src/server/tests.rs b/lib/crates/fabro-server/src/server/tests.rs index 214bff225..45e26df15 100644 --- a/lib/crates/fabro-server/src/server/tests.rs +++ b/lib/crates/fabro-server/src/server/tests.rs @@ -2073,23 +2073,24 @@ fn slack_app_state_with_settings_and_secret_sources( .expect("slack test app state should build") } +fn slack_test_vault_tokens() -> [(&'static str, &'static str, SecretType); 2] { + [ + ( + EnvVars::FABRO_SLACK_BOT_TOKEN, + "xoxb-test", + SecretType::Token, + ), + ( + EnvVars::FABRO_SLACK_APP_TOKEN, + "xapp-test", + SecretType::Token, + ), + ] +} + #[test] fn slack_service_ignores_vault_tokens_when_config_is_absent() { - let state = slack_app_state_with_secret_sources( - &[ - ( - EnvVars::FABRO_SLACK_BOT_TOKEN, - "xoxb-test", - SecretType::Token, - ), - ( - EnvVars::FABRO_SLACK_APP_TOKEN, - "xapp-test", - SecretType::Token, - ), - ], - HashMap::new(), - ); + let state = slack_app_state_with_secret_sources(&slack_test_vault_tokens(), HashMap::new()); assert!(state.slack_service.is_none()); } @@ -2132,6 +2133,33 @@ enabled = true assert_eq!(connection.status, IntegrationConnectionState::Connecting); assert!(connection.last_connected_at.is_none()); assert!(connection.last_error.is_none()); + assert!(service.default_channel.is_none()); +} + +#[test] +fn slack_service_receives_configured_default_channel_verbatim() { + let state = slack_app_state_with_settings_and_secret_sources( + server_settings_from_toml( + r##" +_version = 1 + +[server.auth] +methods = ["dev-token"] + +[server.integrations.slack] +enabled = true +default_channel = "#releases" +"##, + ), + &slack_test_vault_tokens(), + HashMap::new(), + ); + + let service = state + .slack_service + .as_ref() + .expect("slack service should be enabled by config and vault tokens"); + assert_eq!(service.default_channel.as_deref(), Some("#releases")); } #[test] diff --git a/lib/crates/fabro-types/src/settings/server.rs b/lib/crates/fabro-types/src/settings/server.rs index 59055027c..ef8da68b8 100644 --- a/lib/crates/fabro-types/src/settings/server.rs +++ b/lib/crates/fabro-types/src/settings/server.rs @@ -12,7 +12,6 @@ use serde::de::Error as _; use serde::{Deserialize, Deserializer, Serialize, Serializer}; use super::duration::Duration; -use super::interp::InterpString; /// A structurally resolved `[server]` view for consumers. /// @@ -258,7 +257,7 @@ pub struct GithubIntegrationSettings { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct SlackIntegrationSettings { pub enabled: bool, - pub default_channel: Option, + pub default_channel: Option, } impl Default for SlackIntegrationSettings {