From 770bc32229547b6781b05a54cecc61de8a6ce700 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 20 Aug 2026 11:22:35 +0100 Subject: [PATCH] feat(notebooks): add --markdown to get, create, update, and edit Work with a notebook as a Markdown document instead of a JSON cells array. These call /api/unstable/notebooks, which the notebooks team has not promoted to /api/v2 yet, so the flag is documented as experimental. - get --markdown prints the document; create/update take a Markdown file - edit --markdown appends server-side via the content-fragment endpoint, so the rest of the document is not rewritten - Reject --jq alongside --markdown, and reject JSON files passed with --markdown before any request is made - Translate the backend's 5xx for notebooks that have no Markdown projection into an actionable message Co-Authored-By: Claude Opus 5 (1M context) --- docs/COMMANDS.md | 29 +- src/commands/notebooks.rs | 543 +++++++++++++++++++++++++++++++++- src/commands/skills_remote.rs | 5 +- src/main.rs | 95 +++++- src/raw_client.rs | 13 +- src/runbooks/engine.rs | 1 + src/util_ext.rs | 12 + 7 files changed, 679 insertions(+), 19 deletions(-) diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index d89bcc10..9ae81ea5 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -49,7 +49,7 @@ pup [options] # Nested commands | logs-restriction | list, get, create, update, delete, roles (list, add) | src/commands/logs_restriction.rs | ✅ | | processes | list | src/commands/processes.rs | ✅ | | users | list, get, roles, service-accounts (create, app-keys CRUD) | src/commands/users.rs | ✅ | -| notebooks | list, get, create, update, diff, delete, annotations (list, get-page, create, update, delete) | src/commands/notebooks.rs, src/commands/annotations.rs | ✅ | +| notebooks | list, get, create, update, edit, diff, delete (get/create/update/edit accept `--markdown`), annotations (list, get-page, create, update, delete) | src/commands/notebooks.rs, src/commands/annotations.rs | ✅ | | security | rules, signals, findings, content-packs, risk-scores | src/commands/security.rs | ✅ | | organizations | get, list | src/commands/organizations.rs | ✅ | | service-catalog | list, get | src/commands/service_catalog.rs | ✅ | @@ -273,6 +273,33 @@ steps) bypass `format_and_print` and do not honor `--jq`. ## Recent Enhancements +### Notebooks — Markdown representation (experimental) + +`notebooks get`, `create`, `update`, and `edit` accept `--markdown` to work with a +notebook as a Markdown document instead of a JSON cells array. These call +`/api/unstable/notebooks`, which the notebooks team has not yet promoted to +`/api/v2`; the path and response contract may still change. + +- `get --markdown` — print the notebook as Markdown (YAML frontmatter + body) +- `create --markdown --file doc.md` — create from a Markdown file +- `update --markdown --file doc.md` — replace the whole document +- `edit --markdown --file fragment.md` — append the fragment server-side + +Constraints worth knowing before relying on these: + +- **Rich-text notebooks only.** Notebooks created through the older cells API have + no Markdown projection, and the API returns an error for them. +- **`update --markdown` is lossy.** It replaces the entire document, and anything + Markdown cannot represent is dropped. The JSON path preserves more. +- **No conflict detection.** `document_revision` is returned but not enforced by + the API, so concurrent writers can overwrite each other on any write path. +- **No targeted edits.** `update` replaces and `edit` appends; there is no way to + modify one section in place. +- `--jq` is rejected with `--markdown`, since the output is not JSON. +- `--output` and agent mode have no effect under `--markdown`: the document is + printed as-is, with no envelope and no format conversion. This matches + `skills remote get`, the other command that emits raw Markdown. + ### v0.64.x — Error Tracking Issue Filters (SDK PRs #1568, #1480) - **error-tracking issues search** — new optional filter flags: diff --git a/src/commands/notebooks.rs b/src/commands/notebooks.rs index 6b1d8851..c886f7c7 100644 --- a/src/commands/notebooks.rs +++ b/src/commands/notebooks.rs @@ -12,6 +12,128 @@ use crate::util_ext; const SEARCH_PATH: &str = "/api/v2/notebooks/search"; const MAX_RESULTS: usize = 1000; +/// Markdown notebook routes. These are served from `/api/unstable` rather than +/// `/api/v2`: the notebooks team has not promoted them yet, so the path and the +/// response contract may still change. +const MARKDOWN_BASE: &str = "/api/unstable/notebooks"; +const MARKDOWN_MEDIA_TYPE: &str = "text/markdown"; + +/// Issue a markdown-negotiated request against the unstable notebooks routes. +/// +/// The generated client cannot reach these paths (they are absent from the +/// published OpenAPI spec), so they go through `raw_client` directly. +async fn markdown_request( + cfg: &Config, + method: &str, + path: &str, + body: Option, +) -> Result { + let content_type = body.is_some().then_some(MARKDOWN_MEDIA_TYPE); + let body_bytes = body.map(String::into_bytes); + raw_client::raw_request( + cfg, + method, + path, + &[], + body_bytes, + content_type, + MARKDOWN_MEDIA_TYPE, + &[], + ) + .await + .map_err(|error| translate_markdown_error(error, method)) +} + +/// Replace the backend's bare 5xx for an unprojectable notebook with something +/// the caller can act on. +/// +/// Matched on the detail string rather than the status code: the same failure +/// has been observed as both 500 and 502. +/// +/// The advice differs by method. On a write the render runs after the mutation, +/// so this error does not prove the write was rejected — telling the caller to +/// retry could duplicate a `create` or repeat a destructive `update`. +fn translate_markdown_error(error: anyhow::Error, method: &str) -> anyhow::Error { + if !error + .to_string() + .contains("Unable to render the notebook as Markdown") + { + return error; + } + // `context` rather than a fresh error, so the status, URL, and server body + // stay on the chain when this heuristic misfires. + if method.eq_ignore_ascii_case("GET") { + return error.context( + "this notebook has no Markdown representation \ + (notebooks created through the older cells API cannot be projected); \ + use the JSON form of this command instead", + ); + } + error.context( + "the notebook could not be rendered as Markdown after the write, \ + so it is unknown whether the change was applied; \ + check the notebook before retrying, and use the JSON form of this command for it", + ) +} + +/// Extract the notebook id from a creation `Location` header. +/// +/// Returns `None` for a shape this does not recognise, so an unexpected header +/// costs the id hint rather than failing a create that already succeeded. +fn created_notebook_id(location: &str) -> Option<&str> { + let id = location.trim_end_matches('/').rsplit('/').next()?; + (!id.is_empty() && id.chars().all(|c| c.is_ascii_digit())).then_some(id) +} + +/// Errors when the server answers with JSON instead: `--markdown` promises +/// markdown on stdout, and silently switching shape would break any caller +/// parsing the output. +fn decode_markdown_response(resp: raw_client::HttpResponse) -> Result { + // Exact match on the media type, parameters stripped, so a neighbour like + // `text/markdown-json` is not mistaken for Markdown. + let media_type = resp + .content_type + .split(';') + .next() + .unwrap_or_default() + .trim() + .to_ascii_lowercase(); + if media_type != MARKDOWN_MEDIA_TYPE { + anyhow::bail!( + "expected a Markdown response but the server returned {:?}", + resp.content_type + ); + } + if resp.bytes.iter().all(u8::is_ascii_whitespace) { + anyhow::bail!("the server returned an empty Markdown document"); + } + String::from_utf8(resp.bytes) + .map_err(|e| anyhow::anyhow!("notebook Markdown response was not valid UTF-8: {e}")) +} + +/// Read a markdown file, rejecting input that is obviously JSON. +/// +/// A JSON file reaching a `text/markdown` endpoint is accepted verbatim as +/// document text rather than rejected, so catching it here avoids silently +/// writing a serialized notebook into a notebook as prose. +fn read_markdown_file(file: &str) -> Result { + let content = + std::fs::read_to_string(file).map_err(|e| anyhow::anyhow!("failed to read {file}: {e}"))?; + // A byte-order mark would otherwise survive `trim` and hide the leading + // brace from the JSON check below. + let trimmed = content.trim_start_matches('\u{feff}').trim(); + if trimmed.is_empty() { + anyhow::bail!("{file} is empty"); + } + // Bare scalars parse as JSON too, so require a leading brace or bracket + // before treating the file as a misrouted JSON document. + let looks_like_json = trimmed.starts_with('{') || trimmed.starts_with('['); + if looks_like_json && serde_json::from_str::(trimmed).is_ok() { + anyhow::bail!("{file} contains JSON, not Markdown; drop --markdown to use the JSON API"); + } + Ok(content) +} + fn compact_validation_details(content: &str) -> Option { let parsed: serde_json::Value = serde_json::from_str(content).ok()?; let errors = parsed.get("errors")?.as_array()?; @@ -154,7 +276,13 @@ pub async fn search( .await } -pub async fn get(cfg: &Config, notebook_id: i64) -> Result<()> { +pub async fn get(cfg: &Config, notebook_id: i64, markdown: bool) -> Result<()> { + if markdown { + let path = format!("{MARKDOWN_BASE}/{notebook_id}"); + let resp = markdown_request(cfg, "GET", &path, None).await?; + util_ext::print_text_document(&decode_markdown_response(resp)?); + return Ok(()); + } let api = crate::make_api!(NotebooksAPI, cfg); let resp = api .get_notebook(notebook_id) @@ -172,7 +300,19 @@ pub async fn delete(cfg: &Config, notebook_id: i64) -> Result<()> { Ok(()) } -pub async fn create(cfg: &Config, file: &str) -> Result<()> { +pub async fn create(cfg: &Config, file: &str, markdown: bool) -> Result<()> { + if markdown { + let content = read_markdown_file(file)?; + let resp = markdown_request(cfg, "POST", MARKDOWN_BASE, Some(content)).await?; + // The Markdown projection carries no id, so the new notebook is only + // identifiable from `Location`. Reported on stderr so redirecting stdout + // still captures a clean document. + if let Some(id) = resp.location.as_deref().and_then(created_notebook_id) { + eprintln!("Created notebook {id}"); + } + util_ext::print_text_document(&decode_markdown_response(resp)?); + return Ok(()); + } let api = crate::make_api!(NotebooksAPI, cfg); let body: NotebookCreateRequest = util::read_json_file(file)?; let resp = api @@ -182,7 +322,14 @@ pub async fn create(cfg: &Config, file: &str) -> Result<()> { formatter::output(cfg, &resp) } -pub async fn update(cfg: &Config, notebook_id: i64, file: &str) -> Result<()> { +pub async fn update(cfg: &Config, notebook_id: i64, file: &str, markdown: bool) -> Result<()> { + if markdown { + let content = read_markdown_file(file)?; + let path = format!("{MARKDOWN_BASE}/{notebook_id}"); + let resp = markdown_request(cfg, "PATCH", &path, Some(content)).await?; + util_ext::print_text_document(&decode_markdown_response(resp)?); + return Ok(()); + } let api = crate::make_api!(NotebooksAPI, cfg); let body: NotebookUpdateRequest = util::read_json_file(file)?; let resp = api @@ -220,7 +367,17 @@ pub async fn diff( /// Append-only update: fetches the current notebook, appends cells from /// `file` (an array of cell objects), then writes the full modified notebook back. -pub async fn edit(cfg: &Config, notebook_id: i64, file: &str) -> Result<()> { +/// +/// With `markdown`, the append happens server-side against the content-fragment +/// endpoint instead, so the rest of the document is never rewritten. +pub async fn edit(cfg: &Config, notebook_id: i64, file: &str, markdown: bool) -> Result<()> { + if markdown { + let content = read_markdown_file(file)?; + let path = format!("{MARKDOWN_BASE}/{notebook_id}/content"); + let resp = markdown_request(cfg, "POST", &path, Some(content)).await?; + util_ext::print_text_document(&decode_markdown_response(resp)?); + return Ok(()); + } let api = crate::make_api!(NotebooksAPI, cfg); // Fetch current notebook so we can append without clobbering existing cells. @@ -267,6 +424,384 @@ mod tests { use crate::test_support::*; use mockito::Matcher; + fn markdown_response(content_type: &str, body: &str) -> super::raw_client::HttpResponse { + super::raw_client::HttpResponse { + content_type: content_type.to_string(), + bytes: body.as_bytes().to_vec(), + ..Default::default() + } + } + + #[test] + fn test_decode_markdown_response_returns_body() { + let resp = markdown_response("text/markdown; charset=utf-8", "## hi\n"); + assert_eq!(super::decode_markdown_response(resp).unwrap(), "## hi\n"); + } + + #[test] + fn test_decode_markdown_response_rejects_json() { + let resp = markdown_response("application/vnd.api+json", r#"{"data":{}}"#); + let err = super::decode_markdown_response(resp) + .unwrap_err() + .to_string(); + assert!(err.contains("expected a Markdown response"), "got: {err}"); + } + + #[test] + fn test_decode_markdown_response_rejects_empty_body() { + let resp = markdown_response("text/markdown", " \n "); + let err = super::decode_markdown_response(resp) + .unwrap_err() + .to_string(); + assert!(err.contains("empty Markdown document"), "got: {err}"); + } + + #[test] + fn test_decode_markdown_response_rejects_absent_content_type() { + let resp = markdown_response("", "## hi"); + let err = super::decode_markdown_response(resp) + .unwrap_err() + .to_string(); + assert!(err.contains("expected a Markdown response"), "got: {err}"); + } + + #[test] + fn test_translate_markdown_error_warns_write_outcome_is_unknown() { + // The render runs after the mutation, so a write must not be described + // as a no-op or invite a blind retry. + for method in ["POST", "PATCH"] { + let raw = anyhow::anyhow!( + "{method} /api/unstable/notebooks/1 failed (HTTP 500): \ + Unable to render the notebook as Markdown." + ); + let translated = super::translate_markdown_error(raw, method).to_string(); + assert!( + translated.contains("unknown whether the change was applied"), + "{method} not warned: {translated}" + ); + } + } + + #[test] + fn test_created_notebook_id_extracts_from_location() { + assert_eq!( + super::created_notebook_id("/api/unstable/notebooks/15335295"), + Some("15335295") + ); + assert_eq!( + super::created_notebook_id("https://api.datadoghq.com/api/unstable/notebooks/42/"), + Some("42") + ); + } + + #[test] + fn test_created_notebook_id_ignores_unrecognised_shapes() { + // An unexpected header should cost the hint, never the create. + for location in ["", "/api/unstable/notebooks", "/notebooks/abc", "/"] { + assert_eq!( + super::created_notebook_id(location), + None, + "unexpectedly parsed {location:?}" + ); + } + } + + #[tokio::test] + async fn test_notebooks_create_markdown_reports_new_id() { + let _lock = lock_env().await; + let mut server = mockito::Server::new_async().await; + let cfg = test_config(&server.url()); + let mock = server + .mock("POST", "/api/unstable/notebooks") + .with_status(201) + .with_header("content-type", "text/markdown; charset=utf-8") + .with_header("location", "/api/unstable/notebooks/987654") + .with_body("## created\n") + .create_async() + .await; + + let path = write_temp_json("pup_nb_create_id.md", "## created\n"); + let result = super::create(&cfg, path.to_str().unwrap(), true).await; + let _ = std::fs::remove_file(path); + + assert!(result.is_ok(), "create failed: {:?}", result.err()); + mock.assert_async().await; + cleanup_env(); + } + + #[tokio::test] + async fn test_notebooks_create_markdown_succeeds_without_location() { + let _lock = lock_env().await; + let mut server = mockito::Server::new_async().await; + let cfg = test_config(&server.url()); + let mock = server + .mock("POST", "/api/unstable/notebooks") + .with_status(201) + .with_header("content-type", "text/markdown; charset=utf-8") + .with_body("## created\n") + .create_async() + .await; + + let path = write_temp_json("pup_nb_create_no_loc.md", "## created\n"); + let result = super::create(&cfg, path.to_str().unwrap(), true).await; + let _ = std::fs::remove_file(path); + + assert!(result.is_ok(), "create failed: {:?}", result.err()); + mock.assert_async().await; + cleanup_env(); + } + + #[test] + fn test_decode_markdown_response_rejects_neighbouring_media_type() { + let resp = markdown_response("text/markdown-json", "## hi"); + let err = super::decode_markdown_response(resp) + .unwrap_err() + .to_string(); + assert!(err.contains("expected a Markdown response"), "got: {err}"); + } + + #[test] + fn test_read_markdown_file_rejects_bom_prefixed_json() { + let path = write_temp_json("pup_nb_bom_json.json", "\u{feff}{\"data\":{\"id\":\"1\"}}"); + let err = super::read_markdown_file(path.to_str().unwrap()) + .unwrap_err() + .to_string(); + let _ = std::fs::remove_file(path); + assert!(err.contains("contains JSON, not Markdown"), "got: {err}"); + } + + #[test] + fn test_read_markdown_file_reports_missing_file() { + let err = super::read_markdown_file("/nonexistent/pup-nb-missing.md") + .unwrap_err() + .to_string(); + assert!(err.contains("failed to read"), "got: {err}"); + } + + #[test] + fn test_decode_markdown_response_accepts_uppercase_content_type() { + let resp = markdown_response("Text/Markdown; charset=utf-8", "## hi\n"); + assert_eq!(super::decode_markdown_response(resp).unwrap(), "## hi\n"); + } + + #[test] + fn test_translate_markdown_error_preserves_original_on_chain() { + let raw = anyhow::anyhow!( + "GET /api/unstable/notebooks/1 failed (HTTP 500): \ + Unable to render the notebook as Markdown." + ); + let translated = super::translate_markdown_error(raw, "GET"); + // The friendly text fronts the chain; the server detail stays reachable. + assert!(translated + .to_string() + .contains("no Markdown representation")); + let chain = format!("{translated:#}"); + assert!(chain.contains("HTTP 500"), "source lost: {chain}"); + } + + #[test] + fn test_decode_markdown_response_rejects_invalid_utf8() { + let resp = super::raw_client::HttpResponse { + content_type: "text/markdown".to_string(), + bytes: vec![0xff, 0xfe], + ..Default::default() + }; + let err = super::decode_markdown_response(resp) + .unwrap_err() + .to_string(); + assert!(err.contains("not valid UTF-8"), "got: {err}"); + } + + #[test] + fn test_translate_markdown_error_explains_unprojectable_notebook() { + // The backend has returned this failure as both 500 and 502, so the + // translation must key on the detail string, not the status code. + for status in ["500", "502"] { + let raw = anyhow::anyhow!( + "GET https://api.datadoghq.com/api/unstable/notebooks/1 failed (HTTP {status}): \ + {{\"errors\":[{{\"detail\":\"Unable to render the notebook as Markdown.\"}}]}}" + ); + let translated = super::translate_markdown_error(raw, "GET").to_string(); + assert!( + translated.contains("no Markdown representation"), + "status {status} not translated: {translated}" + ); + } + } + + #[test] + fn test_translate_markdown_error_passes_other_errors_through() { + let raw = anyhow::anyhow!("HTTP 404 not found"); + assert_eq!( + super::translate_markdown_error(raw, "GET").to_string(), + "HTTP 404 not found" + ); + } + + #[test] + fn test_read_markdown_file_rejects_json_document() { + let path = write_temp_json("pup_nb_md_rejects_json.json", r#"{"data":{"id":"1"}}"#); + let err = super::read_markdown_file(path.to_str().unwrap()) + .unwrap_err() + .to_string(); + let _ = std::fs::remove_file(path); + assert!(err.contains("contains JSON, not Markdown"), "got: {err}"); + } + + #[test] + fn test_read_markdown_file_allows_bare_scalar_markdown() { + // `42` parses as valid JSON but is legitimate Markdown prose. + let path = write_temp_json("pup_nb_md_bare_scalar.md", "42"); + let content = super::read_markdown_file(path.to_str().unwrap()).unwrap(); + let _ = std::fs::remove_file(path); + assert_eq!(content, "42"); + } + + #[test] + fn test_read_markdown_file_rejects_empty() { + let path = write_temp_json("pup_nb_md_empty.md", " \n"); + let err = super::read_markdown_file(path.to_str().unwrap()) + .unwrap_err() + .to_string(); + let _ = std::fs::remove_file(path); + assert!(err.contains("is empty"), "got: {err}"); + } + + #[tokio::test] + async fn test_notebooks_get_markdown_requests_markdown_media_type() { + let _lock = lock_env().await; + let mut server = mockito::Server::new_async().await; + let cfg = test_config(&server.url()); + let mock = server + .mock("GET", "/api/unstable/notebooks/123") + .match_header("accept", "text/markdown") + .with_status(200) + .with_header("content-type", "text/markdown; charset=utf-8") + .with_body("---\ntitle: test\n---\n\n## hi\n") + .create_async() + .await; + + super::get(&cfg, 123, true).await.unwrap(); + mock.assert_async().await; + cleanup_env(); + } + + #[tokio::test] + async fn test_notebooks_get_markdown_translates_unprojectable_notebook() { + let _lock = lock_env().await; + let mut server = mockito::Server::new_async().await; + let cfg = test_config(&server.url()); + let mock = server + .mock("GET", "/api/unstable/notebooks/123") + .with_status(500) + .with_header("content-type", "application/vnd.api+json") + .with_body(r#"{"errors":[{"detail":"Unable to render the notebook as Markdown."}]}"#) + .create_async() + .await; + + let err = super::get(&cfg, 123, true).await.unwrap_err().to_string(); + mock.assert_async().await; + assert!(err.contains("no Markdown representation"), "got: {err}"); + cleanup_env(); + } + + #[tokio::test] + async fn test_notebooks_create_markdown_posts_document() { + let _lock = lock_env().await; + let mut server = mockito::Server::new_async().await; + let cfg = test_config(&server.url()); + let mock = server + .mock("POST", "/api/unstable/notebooks") + .match_header("content-type", "text/markdown") + .match_header("accept", "text/markdown") + .match_body("## new notebook\n") + .with_status(201) + .with_header("content-type", "text/markdown; charset=utf-8") + .with_body("---\ntitle: test\n---\n\n## new notebook\n") + .create_async() + .await; + + let path = write_temp_json("pup_nb_create_md.md", "## new notebook\n"); + let result = super::create(&cfg, path.to_str().unwrap(), true).await; + let _ = std::fs::remove_file(path); + + assert!(result.is_ok(), "create failed: {:?}", result.err()); + mock.assert_async().await; + cleanup_env(); + } + + #[tokio::test] + async fn test_notebooks_update_markdown_patches_document() { + let _lock = lock_env().await; + let mut server = mockito::Server::new_async().await; + let cfg = test_config(&server.url()); + let mock = server + .mock("PATCH", "/api/unstable/notebooks/123") + .match_header("content-type", "text/markdown") + .match_header("accept", "text/markdown") + .match_body("## replaced\n") + .with_status(200) + .with_header("content-type", "text/markdown; charset=utf-8") + .with_body("## replaced\n") + .create_async() + .await; + + let path = write_temp_json("pup_nb_update_md.md", "## replaced\n"); + let result = super::update(&cfg, 123, path.to_str().unwrap(), true).await; + let _ = std::fs::remove_file(path); + + assert!(result.is_ok(), "update failed: {:?}", result.err()); + mock.assert_async().await; + cleanup_env(); + } + + #[tokio::test] + async fn test_notebooks_edit_markdown_appends_to_content_endpoint() { + let _lock = lock_env().await; + let mut server = mockito::Server::new_async().await; + let cfg = test_config(&server.url()); + let mock = server + .mock("POST", "/api/unstable/notebooks/123/content") + .match_header("content-type", "text/markdown") + .match_header("accept", "text/markdown") + // The fragment only: edit must not resend the whole document. + .match_body("## appended\n") + .with_status(200) + .with_header("content-type", "text/markdown; charset=utf-8") + .with_body("## existing\n\n## appended\n") + .create_async() + .await; + + let path = write_temp_json("pup_nb_edit_md.md", "## appended\n"); + let result = super::edit(&cfg, 123, path.to_str().unwrap(), true).await; + let _ = std::fs::remove_file(path); + + assert!(result.is_ok(), "edit failed: {:?}", result.err()); + mock.assert_async().await; + cleanup_env(); + } + + #[tokio::test] + async fn test_notebooks_markdown_rejects_json_file_before_any_request() { + let _lock = lock_env().await; + let mut server = mockito::Server::new_async().await; + let cfg = test_config(&server.url()); + // Expect zero hits: the JSON check must short-circuit before the request. + let mock = server + .mock("POST", "/api/unstable/notebooks") + .expect(0) + .create_async() + .await; + + let path = write_temp_json("pup_nb_wrong_flag.json", r#"{"data":{"id":"1"}}"#); + let result = super::create(&cfg, path.to_str().unwrap(), true).await; + let _ = std::fs::remove_file(path); + + assert!(result.is_err()); + mock.assert_async().await; + cleanup_env(); + } + #[test] fn test_compact_validation_details_reassembles_character_errors() { let content = serde_json::json!({ diff --git a/src/commands/skills_remote.rs b/src/commands/skills_remote.rs index a48518b5..2e6578fb 100644 --- a/src/commands/skills_remote.rs +++ b/src/commands/skills_remote.rs @@ -86,10 +86,7 @@ pub async fn get( let path = format!("{SKILLS_PATH}/{}", util_ext::percent_encode(skill_id)); let markdown = get_markdown(cfg, &path, &query).await?; - print!("{markdown}"); - if !markdown.ends_with('\n') { - println!(); - } + util_ext::print_text_document(&markdown); Ok(()) } diff --git a/src/main.rs b/src/main.rs index 5b84c721..9e335914 100644 --- a/src/main.rs +++ b/src/main.rs @@ -6449,17 +6449,36 @@ enum NotebookActions { options: NotebookDiscoveryOptions, }, /// Get notebook details - Get { notebook_id: i64 }, + Get { + notebook_id: i64, + #[arg( + long, + help = "Print the notebook as Markdown (experimental raw API; rich-text notebooks only)" + )] + markdown: bool, + }, /// Create a new notebook Create { #[arg(long, help = "JSON file with notebook data (required)")] file: String, + #[arg( + long, + help = "Treat --file as Markdown instead of JSON (experimental raw API)" + )] + markdown: bool, }, /// Update a notebook (full replace) Update { notebook_id: i64, #[arg(long, help = "JSON file with notebook data (required)")] file: String, + #[arg( + long, + help = "Treat --file as Markdown instead of JSON (experimental raw API; \ + rich-text notebooks only). REPLACES the whole document, and drops \ + anything Markdown cannot represent — use 'edit --markdown' to append" + )] + markdown: bool, }, /// Diff a candidate JSON definition against the live notebook Diff { @@ -6486,6 +6505,13 @@ enum NotebookActions { help = "JSON file containing an array of cell objects to append (required)" )] file: String, + #[arg( + long, + help = "Treat --file as Markdown and APPEND it server-side (experimental raw API; \ + rich-text notebooks only). Existing content is preserved — use \ + 'update --markdown' to replace the document" + )] + markdown: bool, }, /// Delete a notebook Delete { notebook_id: i64 }, @@ -12415,6 +12441,21 @@ mod resolve_callback_port_tests { /// ignored. When the flag is absent, the format already resolved from env/config /// in `Config::from_env` is kept, so `DD_OUTPUT` / `PUP_OUTPUT` (and the format an /// extension inherits from its parent) survive. +/// `--markdown` prints raw Markdown, so a jq filter has nothing to run against. +/// +/// Keyed on the flag rather than `cfg.jq`, which is also populated from +/// `PUP_FILTER` for extension subprocesses — those would otherwise be unable to +/// use `--markdown` at all, and would be told to remove a flag they never passed. +/// `--output` gets no equivalent check because `Config` does not record whether +/// it was passed, so an explicit `--output json` is indistinguishable from the +/// default. +fn reject_jq_with_markdown(jq_flag_passed: bool, markdown: bool) -> anyhow::Result<()> { + if markdown && jq_flag_passed { + anyhow::bail!("--jq cannot be combined with --markdown (the response is not JSON)"); + } + Ok(()) +} + fn resolve_output_format( flag: Option<&str>, resolved: config::OutputFormat, @@ -12429,9 +12470,27 @@ fn resolve_output_format( #[cfg(test)] mod resolve_output_format_tests { + use super::reject_jq_with_markdown; use super::resolve_output_format; use crate::config::OutputFormat; + #[test] + fn jq_flag_with_markdown_is_rejected() { + assert!(reject_jq_with_markdown(true, true).is_err()); + } + + #[test] + fn jq_flag_without_markdown_is_allowed() { + assert!(reject_jq_with_markdown(true, false).is_ok()); + } + + #[test] + fn markdown_without_jq_flag_is_allowed() { + // An inherited PUP_FILTER sets cfg.jq but not the flag, so --markdown + // must still work for extension subprocesses. + assert!(reject_jq_with_markdown(false, true).is_ok()); + } + #[test] fn explicit_flag_overrides_resolved() { let got = resolve_output_format(Some("table"), OutputFormat::Json).unwrap(); @@ -12591,6 +12650,9 @@ async fn main_inner() -> anyhow::Result<()> { if cli.read_only { cfg.read_only = true; } + // Captured before the merge: `cfg.jq` also carries an inherited `PUP_FILTER`, + // which must not be mistaken for an explicit `--jq`. + let jq_flag_passed = cli.jq.is_some(); if cli.jq.is_some() { cfg.jq = cli.jq; } @@ -14502,14 +14564,24 @@ async fn main_inner() -> anyhow::Result<()> { ) .await?; } - NotebookActions::Get { notebook_id } => { - commands::notebooks::get(&cfg, notebook_id).await?; + NotebookActions::Get { + notebook_id, + markdown, + } => { + reject_jq_with_markdown(jq_flag_passed, markdown)?; + commands::notebooks::get(&cfg, notebook_id, markdown).await?; } - NotebookActions::Create { file } => { - commands::notebooks::create(&cfg, &file).await?; + NotebookActions::Create { file, markdown } => { + reject_jq_with_markdown(jq_flag_passed, markdown)?; + commands::notebooks::create(&cfg, &file, markdown).await?; } - NotebookActions::Update { notebook_id, file } => { - commands::notebooks::update(&cfg, notebook_id, &file).await?; + NotebookActions::Update { + notebook_id, + file, + markdown, + } => { + reject_jq_with_markdown(jq_flag_passed, markdown)?; + commands::notebooks::update(&cfg, notebook_id, &file, markdown).await?; } NotebookActions::Diff { notebook_id, @@ -14519,8 +14591,13 @@ async fn main_inner() -> anyhow::Result<()> { } => { commands::notebooks::diff(&cfg, notebook_id, &file, &only, &ignore).await?; } - NotebookActions::Edit { notebook_id, file } => { - commands::notebooks::edit(&cfg, notebook_id, &file).await?; + NotebookActions::Edit { + notebook_id, + file, + markdown, + } => { + reject_jq_with_markdown(jq_flag_passed, markdown)?; + commands::notebooks::edit(&cfg, notebook_id, &file, markdown).await?; } NotebookActions::Delete { notebook_id } => { commands::notebooks::delete(&cfg, notebook_id).await?; diff --git a/src/raw_client.rs b/src/raw_client.rs index ed14c965..d1d614bf 100644 --- a/src/raw_client.rs +++ b/src/raw_client.rs @@ -322,10 +322,14 @@ static OAUTH_EXCLUDED_ENDPOINTS: &[EndpointRequirement] = &[ // --------------------------------------------------------------------------- /// Raw HTTP response returned by [`raw_request`]. -#[derive(Debug)] +#[derive(Debug, Default)] pub struct HttpResponse { /// The `Content-Type` header value from the response, or an empty string if absent. pub content_type: String, + /// The `Location` header value, when the response carries one. Creation + /// endpoints report the new resource here even when the body itself has no + /// identifier. + pub location: Option, /// The raw response body bytes. pub bytes: Vec, } @@ -396,10 +400,16 @@ pub async fn raw_request( .and_then(|v| v.to_str().ok()) .unwrap_or("") .to_string(); + let resp_location = resp + .headers() + .get(reqwest::header::LOCATION) + .and_then(|v| v.to_str().ok()) + .map(str::to_string); if resp.status() == reqwest::StatusCode::NO_CONTENT { return Ok(HttpResponse { content_type: resp_ct, + location: resp_location, bytes: vec![], }); } @@ -407,6 +417,7 @@ pub async fn raw_request( let bytes = resp.bytes().await?.to_vec(); Ok(HttpResponse { content_type: resp_ct, + location: resp_location, bytes, }) } diff --git a/src/runbooks/engine.rs b/src/runbooks/engine.rs index 3f9441e7..69b4646f 100644 --- a/src/runbooks/engine.rs +++ b/src/runbooks/engine.rs @@ -554,6 +554,7 @@ async fn execute_http(cfg: &Config, step: &Step, vars: &HashMap) crate::raw_client::HttpResponse { content_type: resp_ct, bytes, + ..Default::default() } }; diff --git a/src/util_ext.rs b/src/util_ext.rs index bca69fe9..0b6f2243 100644 --- a/src/util_ext.rs +++ b/src/util_ext.rs @@ -163,6 +163,18 @@ pub fn read_to_string(mut reader: impl Read, err_context: &str) -> Result