mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-10 22:43:37 +00:00
Demote server.integrations.slack.default_channel to a plain string (#565)
The `server.integrations.slack.default_channel` field was typed
`Option<InterpString>` 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<InterpString>` → `Option<String>` 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<String>`.
- **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.<route>.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
<details>
<summary>Ran 8 stages in 41m 23s for $10.10</summary>
| 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** |
</details>
<details>
<summary>Ran <code>ImplementPlan.fabro</code> (11 nodes and 14
edges)</summary>
```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
}
```
</details>
⚒️ Generated with [Fabro](https://fabro.sh)
---------
Co-authored-by: Fabro <noreply@fabro.sh>
This commit is contained in:
parent
aadaa3f370
commit
96372c5a3c
7 changed files with 105 additions and 35 deletions
|
|
@ -234,7 +234,7 @@ pub struct SlackIntegrationLayer {
|
|||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub enabled: Option<bool>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub default_channel: Option<InterpString>,
|
||||
pub default_channel: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize, fabro_macros::Combine)]
|
||||
|
|
|
|||
|
|
@ -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(),
|
||||
}
|
||||
},
|
||||
),
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -2422,11 +2422,7 @@ pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result<Arc<AppS
|
|||
let slack_service = {
|
||||
let slack_settings = ¤t_server_settings.server.integrations.slack;
|
||||
if slack_settings.enabled {
|
||||
let default_channel = slack_settings
|
||||
.default_channel
|
||||
.as_ref()
|
||||
.map(|value| value.resolve(process_env_var).map_err(anyhow::Error::from))
|
||||
.transpose()?;
|
||||
let default_channel = slack_settings.default_channel.clone();
|
||||
let vault_guard = vault.try_read().ok();
|
||||
match resolve_slack_credentials_status_with_lookup(|name| {
|
||||
vault_guard
|
||||
|
|
|
|||
|
|
@ -7,7 +7,6 @@ use fabro_slack::config::{
|
|||
resolve_credentials_status_with_lookup as resolve_slack_credentials_status_with_lookup,
|
||||
};
|
||||
use fabro_static::EnvVars;
|
||||
use fabro_types::settings::InterpString;
|
||||
use fabro_types::settings::server::GithubIntegrationSettings;
|
||||
|
||||
use super::super::{
|
||||
|
|
@ -181,18 +180,11 @@ fn github_integration_status(
|
|||
)
|
||||
}
|
||||
|
||||
fn display_interp(state: &AppState, value: &InterpString) -> 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 {
|
||||
|
|
|
|||
|
|
@ -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]
|
||||
|
|
|
|||
|
|
@ -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<InterpString>,
|
||||
pub default_channel: Option<String>,
|
||||
}
|
||||
|
||||
impl Default for SlackIntegrationSettings {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue