Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions rust/crates/sift_cli/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ Keep the skill accurate to the CLI and `sift_mcp` tool surfaces. In particular:
- Teach agents to use `agent doctor`, `install`, and `update` instead of editing
one client. They must obtain explicit user approval before running
`agent update --allow-destructive` and tell the user to reload the client.
- Do not cite flag names or enumerate flag-gated tools in prose docs; the registry
is the source of truth and prose copies go stale.

Write in direct voice and keep it concise. The skill is loaded under context
pressure, so every line should change what the agent does.
Expand Down
3 changes: 3 additions & 0 deletions rust/crates/sift_cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).

### What's New

- Some MCP tools are now enabled per account by feature flags resolved at server
startup. A tool absent from the tool list needs its account flag enabled and an
MCP restart.
- Explore links now carry one source type. The MCP `explore_url` tool accepts
only `asset_ids` and `run_ids`, and rejects them together with
`INVALID_PARAMS` unless the new `include_assets_and_runs` flag is set. This
Expand Down
3 changes: 0 additions & 3 deletions rust/crates/sift_cli/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
[features]
test-reports = ["sift_mcp/test-reports"]

[package]
name = "sift_cli"
version = "0.5.0"
Expand Down
6 changes: 4 additions & 2 deletions rust/crates/sift_cli/assets/skills/sift/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,10 @@ exists.
- **Test results:** `list_test_reports`, `list_test_steps`,
`list_test_measurements`, `count_test_steps`, `count_test_measurements`.
These, along with the `create_test_report` and `append_test_measurements`
writes below, are gated behind the `test-reports` Cargo feature (default on).
A server built with `--no-default-features` will not expose them.
writes below, are enabled per account by feature flags resolved when the MCP
server starts, so they may be absent from the tool list. Enabling them requires
an account setting and an MCP restart. Tell the account owner to contact Sift
to enable them, then restart the MCP client.
- **Data:** `get_data` writes channel data to a Parquet file. `sql` queries
Parquet files. `upload_dataset` streams a Parquet dataset into Sift.
- **Links:** `explore_url`.
Expand Down
3 changes: 2 additions & 1 deletion rust/crates/sift_cli/src/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ pub struct McpArgs {
#[arg(long)]
pub disable_update_check: bool,

/// Disable non-essential network traffic. Release checks are controlled
/// Disable non-essential network traffic. Feature-flag resolution at startup
/// is essential traffic and remains enabled. Release checks are controlled
/// separately by `--disable-update-check`.
#[arg(long)]
pub disable_nonessential_traffic: bool,
Expand Down
10 changes: 10 additions & 0 deletions rust/crates/sift_cli/src/cmd/mcp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,15 @@ pub async fn run(ctx: Context, args: McpArgs, app_uri: String) -> Result<ExitCod
if client_event_config.is_none() {
tracing::info!("non-essential traffic is disabled");
}
let feature_flags = sift_mcp::FeatureFlags::fetch(&ctx.rest_uri, &ctx.api_key)
Comment thread
lineville marked this conversation as resolved.
.await
.unwrap_or_else(|error| {
tracing::warn!(
error = format!("{error:#}"),
"failed to fetch feature flags; flag-gated tools are disabled"
);
sift_mcp::FeatureFlags::default()
});

let credentials = Credentials::Config {
uri: ctx.grpc_uri,
Expand All @@ -62,6 +71,7 @@ pub async fn run(ctx: Context, args: McpArgs, app_uri: String) -> Result<ExitCod
cli_version,
update_check,
client_event_config,
feature_flags,
)
.await
{
Expand Down
31 changes: 12 additions & 19 deletions rust/crates/sift_mcp/CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -638,25 +638,18 @@ and `derive_and_upload` are the reference implementations.

## Reference — feature flags

The crate defines Cargo features to let downstream consumers strip individual tool domains from
the built server. Feature-gated modules follow the same rules as everything else, plus these:

- **`test-reports`** (default on). Gates `service::test_reports`, `tool::test_reports`, the
`test_report_service` field and its construction in `server/mod.rs`, the
`Self::test_reports_router()` merge, and `UrlService::build_test_report_url`. Building with
`--no-default-features` yields a server without the `list_test_reports`, `list_test_steps`,
`list_test_measurements`, `count_test_steps`, `count_test_measurements`, `create_test_report`,
and `append_test_measurements` tools. All other tools remain available.

When gating a domain behind a feature:

- Wrap every declaration and reference — `pub mod`, `use`, struct field, service construction,
router merge, `Self { ... }` init, and any helper on a cross-domain service (see
`UrlService::build_test_report_url`) that only that domain calls.
- Verify both `cargo build -p sift_mcp` and `cargo build -p sift_mcp --no-default-features`
build clean, with no dead-code warnings from unused helpers left behind.
- Update the tool inventory in `rust/crates/sift_cli/assets/skills/sift/SKILL.md` to name the
feature next to the affected tools, so agents know why a tool they expected may be missing.
The server resolves account feature flags at startup with the user's API key. It requests
`GET {rest_uri}/api/v1/feature-flags/variants` with a 5-second timeout, then removes tools whose
flag is disabled. Most tools are unflagged and always available. Flag changes apply after an MCP
restart.

- To put a tool behind a flag, add its `(tool name, flag name)` pair to `TOOL_FEATURE_FLAGS` in
`feature_flags.rs`.
- If fetching flags fails, the server starts normally with all flag-gated tools disabled.
- Gated tools still need entries in `tool_events.json` and tests. The registry-drift and
event-invariant tests in `server/test.rs` cover gated tools through the all-flags-enabled path.
- Do not cite flag names or enumerate flag-gated tools in prose docs, including `CLAUDE.md`,
`SKILL.md`, `CHANGELOG`, or PR text. The registry is the source of truth; prose copies go stale.

---

Expand Down
3 changes: 0 additions & 3 deletions rust/crates/sift_mcp/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,6 @@ polars = { workspace = true, features = ["lazy", "parquet", "sql"] }
tokio-stream.workspace = true
reqwest = { workspace = true, features = ["json"] }

[features]
test-reports = []

[dev-dependencies]
sift_test_util.workspace = true
tokio-stream.workspace = true
Expand Down
39 changes: 24 additions & 15 deletions rust/crates/sift_mcp/src/client_event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,24 +96,29 @@ pub(crate) fn event_for_tool(tool_name: &str) -> Option<&'static str> {
}

#[cfg(test)]
pub(crate) async fn start_event_server() -> (String, tokio::task::JoinHandle<Vec<u8>>) {
pub(crate) async fn start_http_server(
response: Vec<u8>,
) -> (String, tokio::task::JoinHandle<Vec<u8>>) {
use tokio::{
io::{AsyncReadExt, AsyncWriteExt},
net::TcpListener,
};

fn request_length(request: &[u8]) -> Option<usize> {
let header_end = request
.windows(4)
.position(|window| window == b"\r\n\r\n")?;
let headers = std::str::from_utf8(&request[..header_end]).ok()?;
fn request_complete(request: &[u8]) -> bool {
let header_end = request.windows(4).position(|window| window == b"\r\n\r\n");
let Some(header_end) = header_end else {
return false;
};
let Ok(headers) = std::str::from_utf8(&request[..header_end]) else {
return false;
};
let content_length = headers.lines().find_map(|line| {
let (name, value) = line.split_once(':')?;
name.eq_ignore_ascii_case("content-length")
.then(|| value.trim().parse::<usize>().ok())
.flatten()
})?;
Some(header_end + 4 + content_length)
});
content_length.is_none_or(|length| request.len() >= header_end + 4 + length)
}

let listener = TcpListener::bind("127.0.0.1:0").await.unwrap();
Expand All @@ -129,22 +134,26 @@ pub(crate) async fn start_event_server() -> (String, tokio::task::JoinHandle<Vec
"the client closed before the request was complete"
);
request.extend_from_slice(&buffer[..count]);
if request_length(&request).is_some_and(|length| request.len() >= length) {
if request_complete(&request) {
break;
}
}
stream
.write_all(
b"HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: 2\r\nconnection: close\r\n\r\n{}",
)
.await
.unwrap();
stream.write_all(&response).await.unwrap();
request
});

(format!("http://{address}"), server)
}

#[cfg(test)]
pub(crate) async fn start_event_server() -> (String, tokio::task::JoinHandle<Vec<u8>>) {
start_http_server(
b"HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: 2\r\nconnection: close\r\n\r\n{}"
.to_vec(),
)
.await
}

#[cfg(test)]
mod tests {
use super::{ClientEventConfig, ClientEventReporter, start_event_server};
Expand Down
137 changes: 137 additions & 0 deletions rust/crates/sift_mcp/src/feature_flags.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
use std::{collections::HashMap, time::Duration};

use anyhow::{Context, Result};
use serde::Deserialize;

const FEATURE_FLAGS_PATH: &str = "/api/v1/feature-flags/variants";
const REQUEST_TIMEOUT: Duration = Duration::from_secs(5);

pub(crate) static TOOL_FEATURE_FLAGS: &[(&str, &str)] = &[
("list_test_reports", "test-reports"),
("list_test_steps", "test-reports"),
("list_test_measurements", "test-reports"),
("count_test_steps", "test-reports"),
("count_test_measurements", "test-reports"),
("create_test_report", "test-reports"),
("append_test_measurements", "test-reports"),
];

#[derive(Clone, Debug, Default, Deserialize)]
pub struct FeatureFlags {
#[serde(default)]
variants: HashMap<String, FeatureFlagVariant>,
}

#[derive(Clone, Debug, Deserialize)]
struct FeatureFlagVariant {
#[serde(default)]
value: String,
}

impl FeatureFlags {
pub fn enabled(&self, flag: &str) -> bool {
self.variants
.get(flag)
.is_some_and(|variant| !variant.value.is_empty() && variant.value != "off")
}

pub async fn fetch(rest_uri: &str, api_key: &str) -> Result<Self> {
let endpoint = format!("{}{FEATURE_FLAGS_PATH}", rest_uri.trim_end_matches('/'));
reqwest::Client::new()
.get(endpoint)
.timeout(REQUEST_TIMEOUT)
.bearer_auth(api_key)
.send()
.await
.context("feature flag request failed")?
.error_for_status()
.context("feature flag request returned an error status")?
.json()
.await
.context("failed to parse feature flag response")
}
}

#[cfg(test)]
mod tests {
use std::collections::HashMap;

use super::{FeatureFlagVariant, FeatureFlags};
use crate::client_event::start_http_server;

fn response(status: &str, body: &str) -> Vec<u8> {
format!(
"HTTP/1.1 {status}\r\ncontent-type: application/json\r\ncontent-length: {}\r\nconnection: close\r\n\r\n{body}",
body.len()
)
.into_bytes()
}

fn flags(value: Option<&str>) -> FeatureFlags {
let variants = value.map_or_else(HashMap::new, |value| {
HashMap::from([(
"test-flag".to_string(),
FeatureFlagVariant {
value: value.to_string(),
},
)])
});
FeatureFlags { variants }
}

#[test]
fn enabled_requires_a_non_off_variant() {
assert!(!flags(None).enabled("test-flag"));
assert!(!flags(Some("off")).enabled("test-flag"));
assert!(!flags(Some("")).enabled("test-flag"));
assert!(flags(Some("on")).enabled("test-flag"));
assert!(flags(Some("experimental")).enabled("test-flag"));
}

#[test]
fn deserializes_feature_flag_response() {
let flags: FeatureFlags = serde_json::from_str(
r#"{"variants":{"some-flag":{"value":"on"},"other":{"value":"off"},"bare":{}}}"#,
)
.unwrap();

assert!(flags.enabled("some-flag"));
assert!(!flags.enabled("other"));
assert!(!flags.enabled("bare"));
}

#[test]
fn empty_response_disables_all_flags() {
let flags: FeatureFlags = serde_json::from_str("{}").unwrap();

assert!(!flags.enabled("test-flag"));
}

#[tokio::test]
async fn fetches_feature_flags_with_the_expected_request() {
let (rest_uri, server) = start_http_server(response(
"200 OK",
r#"{"variants":{"test-reports":{"value":"on"}}}"#,
))
.await;

let flags = FeatureFlags::fetch(&format!("{rest_uri}/"), "test-key")
.await
.unwrap();
assert!(flags.enabled("test-reports"));

let request = String::from_utf8(server.await.unwrap()).unwrap();
let (headers, _) = request.split_once("\r\n\r\n").unwrap();
assert!(headers.starts_with("GET /api/v1/feature-flags/variants HTTP/1.1"));
assert!(
headers
.lines()
.any(|line| line.eq_ignore_ascii_case("authorization: Bearer test-key"))
);

let (rest_uri, server) =
start_http_server(response("500 Internal Server Error", "{}")).await;
assert!(FeatureFlags::fetch(&rest_uri, "test-key").await.is_err());
server.await.unwrap();
}
}
9 changes: 9 additions & 0 deletions rust/crates/sift_mcp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ use tokio::sync::watch;
mod client_event;
pub use client_event::ClientEventConfig;

mod feature_flags;
pub use feature_flags::FeatureFlags;

mod server;
use server::SiftMcpServer;

Expand Down Expand Up @@ -109,13 +112,15 @@ pub async fn run_with_update_check(
cli_version,
update_check,
None,
FeatureFlags::default(),
)
.await
}

/// Runs the server, reporting anonymous tool-call events only when a client
/// event config is supplied. `None` leaves the server silent, which is what
/// `sift-cli mcp --disable-nonessential-traffic` passes.
#[allow(clippy::too_many_arguments)]
pub async fn run_with_client_events(
credentials: Credentials,
use_tls: bool,
Expand All @@ -125,6 +130,7 @@ pub async fn run_with_client_events(
cli_version: String,
update_check: Option<UpdateCheckReceiver>,
client_event_config: Option<ClientEventConfig>,
feature_flags: FeatureFlags,
) -> Result<()> {
let client_event_reporter =
client_event::ClientEventReporter::from_config(client_event_config, &cli_version);
Expand All @@ -138,6 +144,7 @@ pub async fn run_with_client_events(
cli_version,
update_check,
client_event_reporter,
feature_flags,
},
)
.await
Expand All @@ -150,6 +157,7 @@ struct RunConfig {
cli_version: String,
update_check: Option<UpdateCheckReceiver>,
client_event_reporter: client_event::ClientEventReporter,
feature_flags: FeatureFlags,
}

async fn run_server(credentials: Credentials, use_tls: bool, config: RunConfig) -> Result<()> {
Expand All @@ -167,6 +175,7 @@ async fn run_server(credentials: Credentials, use_tls: bool, config: RunConfig)
config.cli_version,
config.update_check,
config.client_event_reporter,
config.feature_flags,
)
.serve(stdio())
.await
Expand Down
Loading
Loading