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
4 changes: 2 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ tapes-capture = { version = "0.1" }
# is built at runtime from a document this binary has never seen, which is the
# one surface that cannot be written by hand. `direct-http` is `DirectHttp`,
# the no-redirect transport this CLI talks to a tapes server with.
tapes-client = { version = "0.4", features = [
tapes-client = { version = "0.5", features = [
"cli",
"direct-http",
] }
Expand Down
65 changes: 17 additions & 48 deletions crates/tapesctl/src/api/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
//! # Why the read commands still print `serde_json::Value`
//!
//! The named methods on [`tapes_client::CoreClient`] return the vendored
//! contract's models, and every command that *renders* a response β€” search,
//! seed β€” uses them. The `<resource> <method>` commands
//! contract's models, and every command that *renders* a response β€” seed β€”
//! uses them. The `<resource> <method>` commands
//! do not render: they print the server's document, and a document that had
//! been through a model would be missing whatever fields this build predates.
//! For those, [`tapes_client::CoreClient::call`] is the documented escape
Expand Down Expand Up @@ -62,8 +62,8 @@ pub fn connect(base: Url) -> ApiClient {
/// The contract's own spellings are the only ones accepted, case-folded and
/// trimmed the way this CLI has always accepted them. Refusing here rather than
/// letting the server answer 400 costs no round trip and names the alternatives
/// β€” see [`crate::error::Error::InvalidPayloadDetail`] and its sibling, whose
/// wording a test holds to [`ContractEnum::VALUES`].
/// β€” see [`crate::error::Error::InvalidPayloadDetail`], whose wording a test
/// holds to [`ContractEnum::VALUES`].
pub fn parse_grain<E: ContractEnum + DeserializeOwned>(raw: &str) -> Option<E> {
serde_json::from_value(Value::String(raw.trim().to_ascii_lowercase())).ok()
}
Expand All @@ -86,8 +86,8 @@ mod tests {
use crate::api::contract::ops;
use serde_json::json;
use tapes_client::core::models::params::ContractParams;
use tapes_client::core::models::{ExportDetail, PayloadDetail, SessionListParams};
use wiremock::matchers::{method, path, query_param};
use tapes_client::core::models::{PayloadDetail, SessionListParams};
use wiremock::matchers::{method, path};
use wiremock::{Mock, MockServer, ResponseTemplate};

fn client_for(server: &MockServer) -> ApiClient {
Expand Down Expand Up @@ -156,34 +156,30 @@ mod tests {
#[test]
fn a_user_typed_grain_resolves_to_the_contracts_own_spelling() {
assert_eq!(
parse_grain::<ExportDetail>("SPANS"),
Some(ExportDetail::Spans)
parse_grain::<PayloadDetail>("PREVIEW"),
Some(PayloadDetail::Preview)
);
assert_eq!(
parse_grain::<PayloadDetail>(" preview "),
Some(PayloadDetail::Preview),
parse_grain::<PayloadDetail>(" full "),
Some(PayloadDetail::Full),
);
assert_eq!(parse_grain::<PayloadDetail>("hologram"), None);
}

#[test]
fn the_refusal_messages_name_exactly_the_values_the_contract_declares() {
// The two messages spell their alternatives inline, because a user
// reading one wants the answer and not a cross-reference. This is what
// keeps that spelling honest: a contract that grows a grain fails here
// rather than teaching the user a stale set.
fn the_refusal_message_names_exactly_the_values_the_contract_declares() {
// The message spells its alternatives inline, because a user reading
// it wants the answer and not a cross-reference. This is what keeps
// that spelling honest: a contract that grows a grain fails here
// rather than teaching the user a stale set. Its export sibling is
// held the same way in `ports::export`, where the accepted set now
// lives.
let payload = crate::error::error::InvalidPayloadDetailSnafu { payload: "x" }
.build()
.to_string();
for value in PayloadDetail::VALUES {
assert!(payload.contains(value), "{value:?} missing from: {payload}");
}
let detail = crate::error::error::InvalidExportDetailSnafu { detail: "x" }
.build()
.to_string();
for value in ExportDetail::VALUES {
assert!(detail.contains(value), "{value:?} missing from: {detail}");
}
}

#[test]
Expand Down Expand Up @@ -236,31 +232,4 @@ mod tests {
assert!(rendered.contains("400"), "got: {rendered}");
assert!(rendered.contains("invalid cursor"), "got: {rendered}");
}

#[tokio::test]
async fn a_search_response_is_decoded_through_the_shipped_model() {
let server = MockServer::start().await;
Mock::given(method("GET"))
.and(path("/v1/cassettes/search/spans"))
.and(query_param("query", "hooks"))
.and(query_param("top_k", "5"))
.respond_with(ResponseTemplate::new(200).set_body_string(
r#"{"query":"hooks","count":1,"results":[{"trace_id":"t-1","a_new_field":1}]}"#,
))
.mount(&server)
.await;

let got = client_for(&server)
.search_spans(&tapes_client::core::models::SearchSpansParams {
query: "hooks".to_owned(),
top_k: Some(5),
})
.await
.unwrap();

// A field the model has never heard of is ignored rather than fatal:
// an additive server change must not blank a page of results.
assert_eq!(got.count, 1);
assert_eq!(got.results[0].trace_id, "t-1");
}
}
52 changes: 0 additions & 52 deletions crates/tapesctl/src/api/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,12 @@ pub const EXPOSED_OPERATIONS: &[(&str, &str)] = &[
(ops::GET_SESSION, "tapesctl sessions get"),
(ops::GET_SESSION_TRACES, "tapesctl sessions traces"),
(ops::LIST_RAW_TURNS, "tapesctl sessions raw-turns"),
(ops::EXPORT_SESSION, "tapesctl export"),
(ops::LIST_TRACES, "tapesctl traces list"),
(
ops::GET_TRACE,
"tapesctl traces get, and the spans-list projection",
),
(ops::GET_SPAN, "tapesctl spans get"),
(ops::SEARCH_SPANS, "tapesctl search"),
(ops::SEED_DEMO, "tapesctl seed"),
(
ops::LIST_CASSETTES,
Expand Down Expand Up @@ -99,60 +97,10 @@ pub const UNEXPOSED_OPERATIONS: &[(&str, &str)] = &[
"updateSession",
"session rename/edit; not ported from the Go CLI, which never had it either",
),
(
"exportSessions",
"bulk export window; tapesctl export is per-session today, the bulk port is future work",
),
(
"getStats",
"aggregate stats; not ported from the Go CLI yet",
),
(
"listSessionSkills",
"core's copy of a surface the skills cassette owns; tapesctl reaches skills through the \
discovered `cassettes skills` commands, and these routes are deleted from core with the \
cassette cutover's batched removal",
),
(
"listSkills",
"core's copy of a cassette-owned surface; see listSessionSkills",
),
(
"createSkill",
"core's copy of a cassette-owned surface; see listSessionSkills",
),
(
"getSkill",
"core's copy of a cassette-owned surface; see listSessionSkills",
),
(
"updateSkill",
"core's copy of a cassette-owned surface; see listSessionSkills",
),
(
"deleteSkill",
"core's copy of a cassette-owned surface; see listSessionSkills",
),
(
"duplicateSkill",
"core's copy of a cassette-owned surface; see listSessionSkills",
),
(
"getSkillMarkdown",
"core's copy of a cassette-owned surface; see listSessionSkills",
),
(
"listSkillVersions",
"core's copy of a cassette-owned surface; see listSessionSkills",
),
(
"publishSkill",
"core's copy of a cassette-owned surface; see listSessionSkills",
),
(
"generateSkill",
"core's copy of a cassette-owned surface; see listSessionSkills",
),
];

#[cfg(test)]
Expand Down
44 changes: 43 additions & 1 deletion crates/tapesctl/src/api/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,27 @@ fn payload_of(raw: Option<&str>) -> Result<Option<PayloadDetail>> {
}
}

/// Split repeatable `--filter key=value` flags into wire pairs.
///
/// Only the flag's own grammar is checked β€” there must be a `=`, with a
/// non-empty key before it. The key is data: cassettes claim filter params on
/// the sessions listing at runtime, so which names mean anything is decided
/// by the deployment at request time, and validating names here would only
/// make this binary disagree with the server it talks to. Refusing the
/// malformed spelling here rather than sending it costs no round trip and
/// names the expected shape.
fn parse_filters(flags: &[String]) -> Result<Vec<(String, String)>> {
flags
.iter()
.map(|flag| {
flag.split_once('=')
.filter(|(key, _)| !key.is_empty())
.map(|(key, value)| (key.to_owned(), value.to_owned()))
.context(error::InvalidFilterFlagSnafu { flag: flag.clone() })
})
.collect()
}

/// Print a JSON document the way every read command does.
pub fn print_json(value: &serde_json::Value) -> Result<()> {
let rendered = serde_json::to_string_pretty(value).context(error::RenderJsonSnafu)?;
Expand All @@ -90,6 +111,7 @@ pub async fn sessions(command: SessionsCommand) -> Result<()> {
match command {
SessionsCommand::List(args) => {
let client = resolve_client(&args.api)?;
let claimed = parse_filters(&args.filter)?;
let mut values = SessionListParams {
limit: args.limit.map(narrow),
cursor: args.cursor,
Expand All @@ -110,7 +132,13 @@ pub async fn sessions(command: SessionsCommand) -> Result<()> {
if let Some(direction) = args.direction {
values.push(("direction", direction));
}
let value: Value = client.call(ops::LIST_SESSIONS, values).await?;
// Claimed pairs ride the sealed method's own channel: appended to
// the query after the declared parameters, verbatim and in order,
// with the response passed through untouched. No client-side
// filtering β€” server-side fail-open governs what a key means.
let value: Value = client
.call_with_claimed(ops::LIST_SESSIONS, values, &claimed)
.await?;
if args.json {
print_json(&value)
} else {
Expand Down Expand Up @@ -217,10 +245,24 @@ mod tests {
harness_session_id: None,
harness_id: None,
auth_subject: None,
filter: Vec::new(),
json: false,
}
}

#[tokio::test]
async fn a_malformed_filter_flag_fails_before_any_request() {
// No `=` means no pair to send; the refusal happens here, with the
// expected shape named, rather than as a server round trip.
let server = MockServer::start().await;
let mut args = sessions_list_args(server.uri());
args.filter = vec!["no-equals".to_owned()];
let result = sessions(SessionsCommand::List(args)).await;

assert!(result.is_err(), "got: {result:?}");
assert!(server.received_requests().await.unwrap().is_empty());
}

#[test]
fn a_manually_constructed_missing_url_is_an_error() {
// Normal CLI parsing supplies the localhost default. This covers the
Expand Down
11 changes: 11 additions & 0 deletions crates/tapesctl/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,17 @@ pub struct SessionsListArgs {
#[arg(long)]
pub auth_subject: Option<String>,

/// Extra filter, repeatable, as `key=value`; sent through as `?key=value`.
///
/// The param name is data, not a name this binary knows: a deployment's
/// cassettes can claim extra filter params on the sessions listing at
/// runtime, so which keys mean anything is the server's to decide. Pairs
/// are passed through verbatim and in order β€” nothing is validated,
/// normalized, or filtered client-side β€” and a key no admitted cassette
/// claims is ignored by the server.
#[arg(long = "filter", value_name = "KEY=VALUE")]
pub filter: Vec<String>,

/// Print the raw JSON response instead of the table.
///
/// `sessions list` renders a table by default; this restores the
Expand Down
12 changes: 12 additions & 0 deletions crates/tapesctl/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,18 @@ pub enum Error {
schema: String,
},

/// A `--filter` flag that does not spell `key=value`.
///
/// The only thing checked is the flag's own grammar β€” a `=` with a
/// non-empty key before it. The key itself travels as data: which params
/// mean anything is the server's question, so there is nothing else this
/// binary could validate without guessing at a deployment's cassettes.
#[snafu(display("invalid --filter {flag:?} (expected key=value)"))]
InvalidFilterFlag {
/// What the user typed.
flag: String,
},

/// `--schema` was passed for a harness that speaks exactly one schema.
///
/// Refused rather than ignored: a flag that silently does nothing reads,
Expand Down
20 changes: 11 additions & 9 deletions crates/tapesctl/src/plugin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -311,15 +311,17 @@ fn run_in(args: &PluginInstallArgs, machine: &Machine) -> Result<()> {
// be describing a different operation than the one it stands in
// for. Only what is actually there is listed β€” an absent superseded
// copy is nothing this run would do.
for superseded in artifact.superseded_paths(home) {
if superseded.exists() {
println!(
"tapesctl: would remove superseded {} (loaded alongside {} by {})",
superseded.display(),
artifact.file_name(),
harness.id(),
);
}
for superseded in artifact
.superseded_paths(home)
.into_iter()
.filter(|superseded| superseded.exists())
{
println!(
"tapesctl: would remove superseded {} (loaded alongside {} by {})",
superseded.display(),
artifact.file_name(),
harness.id(),
);
}
}
return Ok(());
Expand Down
Loading
Loading