diff --git a/docs/tools.md b/docs/tools.md index 9ac5591..b80463a 100644 --- a/docs/tools.md +++ b/docs/tools.md @@ -7,9 +7,13 @@ explains how they fit together. ## Finding notes Every tool that takes a `note_ref` accepts either a note's internal ID or its -URL, such as `https://hackmd.io/@owner/slug`. If a URL matches no note, or more -than one, the tool does not guess: it returns the candidates so the agent can -pick. +URL, such as `https://hackmd.io/@owner/slug`; a trailing `/download` is +ignored. A legacy share link, `https://hackmd.io/s/`, names no +workspace: with a `team_path` only that team is searched, and without one the +personal notes and then every team (a team that cannot be listed fails the +search, and the error suggests `team_path`). If a URL matches no note, or +more than one, the tool does not guess: it returns the candidates so the agent +can pick. Notes the tools return carry a `note_url`, `https://hackmd.io/`, which opens personal and team notes alike. It is a link, not an invite: whoever follows it @@ -78,10 +82,12 @@ write and one upload with it. An edit landing between that read and the write is reverted; `expected_hash` works here too and catches any made before it. The `title` field always takes effect. The body sets a title only when a note is created without one (a front-matter `title:`, then the first H1, then -"Untitled"), and later body edits never change it, so `hackmd_pull_note` and -`hackmd_push_note` report `title_drift` when the body's own title no longer -matches the listed one. They report it only when the body as last synced gave -the listed title (a title that always differed may be deliberate, and a first +"Untitled"), and later body edits never change it, so `hackmd_pull_note`, +`hackmd_push_note`, and a `patch` edit on `hackmd_update_note` report +`title_drift` when the body's own title no longer matches the listed one; +set `title` in a separate update if the listing should follow. They report it +only when the body as last synced (for a patch, the body before it) gave the +listed title (a title that always differed may be deliberate, and a first pull has nothing to compare), only when they can read both titles for certain (plain front matter, or an H1 that is the body's first line of text), and push only on a push or a no-op, so a missing `title_drift` does not mean the titles diff --git a/src/client.rs b/src/client.rs index a874dcc..f246398 100644 --- a/src/client.rs +++ b/src/client.rs @@ -2117,7 +2117,7 @@ mod tests { &crate::fixture::scratch_files(), crate::note::list::ListNotesInput { source: crate::note::list::NoteSource::Workspace, - workspace: Workspace::Personal, + workspace: None, limit: 100, offset: 4_900, query: Some("roadmap".to_owned()), diff --git a/src/models.rs b/src/models.rs index 24f0df0..d234728 100644 --- a/src/models.rs +++ b/src/models.rs @@ -5,10 +5,12 @@ use serde::{Deserialize, Serialize}; /// family for both route shapes. /// /// On the wire it is a nullable team path: `null` for the personal workspace, -/// the team's path for a team. That is the shape tool input takes (as -/// `team_path`) and tool output reports, so an agent can pass back what it was -/// given. The older tagged object (`{"kind": "team", "team_path": ...}`) is -/// still read; only the sync state store still writes it, through [`tagged`]. +/// the team's path for a team. That is the shape tool output reports and tool +/// input takes (as `team_path`), so an agent can pass back what it was given; +/// an input read through [`given`] holds `None` for `null`, which each tool +/// treats as the personal workspace except where it searches more widely. The +/// older tagged object (`{"kind": "team", "team_path": ...}`) is still read; +/// only the sync state store still writes it, through [`tagged`]. #[derive(Debug, Clone, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] pub(crate) enum Workspace { /// Operate on the authenticated user's personal workspace. @@ -97,6 +99,17 @@ impl schemars::JsonSchema for Workspace { } } +/// `deserialize_with` for an optional `team_path` input: a team, or `None` +/// when none was named. Older clients send the tagged personal object as +/// their default on every call, so it means "none named" too, exactly as +/// `null` and omission do. +pub(crate) fn given<'de, D: serde::Deserializer<'de>>( + deserializer: D, +) -> Result, D::Error> { + Ok(Option::::deserialize(deserializer)? + .filter(|workspace| *workspace != Workspace::Personal)) +} + /// `#[serde(with = "crate::models::tagged")]` for sync state, which keeps /// writing the tagged form so a sidecar stays readable by older builds. pub(crate) mod tagged { diff --git a/src/note/crud.rs b/src/note/crud.rs index d58dbb9..5b560ef 100644 --- a/src/note/crud.rs +++ b/src/note/crud.rs @@ -71,13 +71,20 @@ pub(crate) struct CreateNoteInput { pub(crate) struct UpdateNoteInput { /// Team path (from `hackmd_get_me`) when a direct internal note ID belongs /// to a team; omit for personal notes. `@owner/slug` URLs name their own - /// workspace. - #[serde(default, rename = "team_path", alias = "workspace")] - pub(crate) workspace: Workspace, - /// Internal API ID, `hackmd.io/`, or `hackmd.io/@owner/slug` URL. + /// workspace. With an `s/` link, only this team is searched; + /// omitted, the personal notes and then every team are. + #[serde( + default, + rename = "team_path", + alias = "workspace", + deserialize_with = "crate::models::given" + )] + pub(crate) workspace: Option, + /// Internal API ID, `hackmd.io/`, `hackmd.io/@owner/slug` URL, or legacy + /// `hackmd.io/s/` share link. A trailing `/download` is ignored. pub(crate) note_ref: String, /// Bypass the 60-second account and note-list caches when resolving an - /// `@owner/slug` URL. + /// `@owner/slug` URL or `s/` link. #[serde(default)] pub(crate) refresh: bool, /// New listed title. It always takes effect, whatever the body's @@ -161,16 +168,23 @@ impl UpdateNoteInput { pub(crate) struct DeleteNoteInput { /// Team path (from `hackmd_get_me`) when a direct internal note ID belongs /// to a team; omit for personal notes. `@owner/slug` URLs name their own - /// workspace. - #[serde(default, rename = "team_path", alias = "workspace")] - pub(crate) workspace: Workspace, - /// Internal API ID, `hackmd.io/`, or `hackmd.io/@owner/slug` URL. + /// workspace. With an `s/` link, only this team is searched; + /// omitted, the personal notes and then every team are. + #[serde( + default, + rename = "team_path", + alias = "workspace", + deserialize_with = "crate::models::given" + )] + pub(crate) workspace: Option, + /// Internal API ID, `hackmd.io/`, `hackmd.io/@owner/slug` URL, or legacy + /// `hackmd.io/s/` share link. A trailing `/download` is ignored. /// With `restore`, the internal ID from `hackmd_list_notes` with source /// `trash`. #[serde(alias = "note_id")] pub(crate) note_ref: String, /// Bypass the 60-second account and note-list caches when resolving an - /// `@owner/slug` URL. + /// `@owner/slug` URL or `s/` link. #[serde(default)] pub(crate) refresh: bool, /// Bring a personal note back from trash instead of deleting it. @@ -237,7 +251,7 @@ pub(crate) enum CrudError { )] MalformedExpectedHash, #[error( - "restore takes the internal note ID from hackmd_list_notes with source trash; a trashed note has no live @owner/slug to resolve, and refresh does not apply" + "restore takes the internal note ID from hackmd_list_notes with source trash; a trashed note has no live @owner/slug or s/ to resolve, and refresh does not apply" )] RestoreNeedsId, #[error( @@ -688,7 +702,9 @@ pub(crate) async fn delete_note( ) -> Result, CrudError> { // Resolving a slug searches the live workspace list, where a trashed note // never appears, so it could only end in a confusing not-found. - if input.restore && (input.refresh || crate::note::reference::is_slug_url(&input.note_ref)) { + if input.restore + && (input.refresh || crate::note::reference::resolves_by_listing(&input.note_ref)) + { return Err(CrudError::RestoreNeedsId); } let resolution = crate::note::reference::resolve_note_ref( @@ -780,6 +796,7 @@ mod tests { } for input in [ json!({"note_ref": "https://hackmd.io/@alice/slug", "restore": true}), + json!({"note_ref": "https://hackmd.io/s/SkE33UTHf", "restore": true}), json!({"note_ref": "id", "restore": true, "refresh": true}), ] { let input = serde_json::from_value(input).expect("input should deserialize"); diff --git a/src/note/edit.rs b/src/note/edit.rs index 5cfcfc4..3684072 100644 --- a/src/note/edit.rs +++ b/src/note/edit.rs @@ -4,7 +4,7 @@ use thiserror::Error; use crate::{ client::{HackmdClient, HackmdError}, models::Workspace, - note::body::into_stored, + note::body::{body_title, into_stored}, note::patch::PatchError, note::reference::{NoteRefError, NoteResolution}, }; @@ -12,7 +12,7 @@ use crate::{ /// What `hackmd_update_note` passes on when given a `patch`. #[derive(Debug)] pub(crate) struct EditNoteInput { - pub(crate) workspace: Workspace, + pub(crate) workspace: Option, pub(crate) note_ref: String, pub(crate) refresh: bool, pub(crate) patch: String, @@ -32,6 +32,11 @@ pub(crate) struct EditNoteOutput { pub(crate) changed: bool, /// Size of the body now stored; `hackmd_get_note` returns the body itself. pub(crate) bytes: usize, + /// The title the patched body implies when it differs from the listed + /// one, as `hackmd_push_note` reports it: a patch that renames the H1 + /// leaves the listing unchanged, so set `title` in a separate update. + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) title_drift: Option, } #[derive(Debug, Error)] @@ -119,21 +124,32 @@ pub(crate) async fn edit_note( drop(current); let patch_path = crate::note::patch::patch_path(¬e.workspace, ¬e.note_id); // Matching hunks against a large body is CPU work, kept off the runtime. - let (updated, changed) = crate::local::offload(|| { + let (updated, changed, synced_title) = crate::local::offload(|| { // An added line may carry a lone `\r`; HackMD would store it as `\n`. let updated = crate::note::patch::apply_note_patch(&content, &input.patch, &patch_path) .map(into_stored)?; let changed = updated.as_str() != content.as_str(); - Ok::<_, PatchError>((updated, changed)) + // The body before the patch stands in for the last-synced one: drift + // is reported only when its title matched the listing. + let synced_title = changed.then(|| body_title(&content)).flatten(); + Ok::<_, PatchError>((updated, changed, synced_title)) })?; // The old body is not needed past here; a large note is held once less // through the write and its read-back. drop(content); + let mut title_drift = None; if changed { - client + // Judged against the read-back's title, as push does, so a rename + // made meanwhile is respected. Only the title is kept: the read-back + // body is another full copy of the note. + let listed = client .write_note_body(¬e.workspace, ¬e.note_id, &updated) - .await?; + .await? + .title; + title_drift = crate::local::offload(|| { + crate::note::body::title_drift(&listed, &updated, || synced_title) + }); } Ok(Ok(EditNoteOutput { workspace: note.workspace, @@ -142,6 +158,7 @@ pub(crate) async fn edit_note( patch_path, changed, bytes: updated.len(), + title_drift, })) } @@ -157,7 +174,7 @@ mod tests { fn input(patch: &str) -> EditNoteInput { EditNoteInput { - workspace: Workspace::Personal, + workspace: None, note_ref: "note-id".to_owned(), refresh: false, patch: patch.to_owned(), @@ -173,7 +190,7 @@ mod tests { let fixture = SequenceServer::spawn_repeating([(200, OLD), (202, ""), (200, OLD)]); let client = fixture.client(); let input = EditNoteInput { - workspace: Workspace::Personal, + workspace: None, note_ref: "note-id".to_owned(), refresh: false, patch: @@ -252,6 +269,125 @@ mod tests { server.finish(); } + /// One patch edit: the note as read, then the PATCH and its read-back. + async fn patch_once(title: &str, old: &str, patch_lines: &str) -> super::EditNoteOutput { + patch_with_readback(title, title, old, patch_lines).await + } + + /// As [`patch_once`], with the read-back listing `read_back_title`. + async fn patch_with_readback( + title: &str, + read_back_title: &str, + old: &str, + patch_lines: &str, + ) -> super::EditNoteOutput { + let patch = format!( + "*** Begin Patch\n*** Update File: notes/note-id.md\n{patch_lines}*** End Patch" + ); + let new = crate::note::patch::apply_note_patch(old, &patch, "notes/note-id.md") + .expect("the test patch applies"); + let note = |title: &str, content: &str| { + serde_json::json!({"id": "note-id", "title": title, "content": content}).to_string() + }; + let server = SequenceServer::spawn_scenarios([ + Scenario::new("GET", "/v1/notes/note-id", 200, ¬e(title, old)), + Scenario::new("PATCH", "/v1/notes/note-id", 202, ""), + Scenario::new( + "GET", + "/v1/notes/note-id", + 200, + ¬e(read_back_title, &new), + ), + ]); + let output = edit_note(&server.client(), input(&patch)) + .await + .expect("edit should succeed") + .expect("reference should resolve"); + server.finish(); + output + } + + #[tokio::test] + async fn a_patch_that_renames_the_h1_reports_title_drift() { + let output = patch_once( + "Old name", + "# Old name\n\nbody\n", + "@@\n-# Old name\n+# New name\n", + ) + .await; + assert_eq!(output.title_drift.as_deref(), Some("New name")); + } + + #[tokio::test] + async fn drift_follows_a_rename_made_during_the_patch() { + // The listing already shows the new H1 when the write is read back. + let output = patch_with_readback( + "Old name", + "New name", + "# Old name\n\nbody\n", + "@@\n-# Old name\n+# New name\n", + ) + .await; + assert_eq!(output.title_drift, None); + } + + #[tokio::test] + async fn a_patch_that_leaves_the_h1_reports_no_drift() { + let output = patch_once("Old name", "# Old name\n\nbody\n", "@@\n-body\n+text\n").await; + assert_eq!(output.title_drift, None); + } + + #[tokio::test] + async fn a_heading_that_never_matched_the_listing_is_not_drift() { + // A listed title set apart from the H1 on purpose stays silent, as in push. + let output = patch_once( + "Listed", + "# Heading\n\nbody\n", + "@@\n-# Heading\n+# Renamed\n", + ) + .await; + assert_eq!(output.title_drift, None); + } + + #[tokio::test] + async fn a_patch_that_retitles_the_front_matter_reports_title_drift() { + let output = patch_once( + "Old name", + "---\ntitle: Old name\n---\nbody\n", + "@@\n-title: Old name\n+title: New name\n", + ) + .await; + assert_eq!(output.title_drift.as_deref(), Some("New name")); + } + + #[tokio::test] + async fn a_first_h1_added_by_a_patch_is_not_drift() { + // The body gave no title before, so nothing says the listing followed + // it. + let output = patch_once("Listed", "body\n", "@@\n+# Heading\n body\n").await; + assert_eq!(output.title_drift, None); + } + + #[tokio::test] + async fn a_patch_that_changes_nothing_writes_nothing_and_reports_no_drift() { + let server = SequenceServer::spawn_scenarios([Scenario::new( + "GET", + "/v1/notes/note-id", + 200, + r##"{"id":"note-id","title":"Listed","content":"# Other\n"}"##, + )]); + let output = edit_note( + &server.client(), + input("*** Begin Patch\n*** Update File: notes/note-id.md\n@@\n-# Other\n+# Other\n*** End Patch"), + ) + .await + .expect("edit should succeed") + .expect("reference should resolve"); + assert!(!output.changed); + assert_eq!(output.title_drift, None); + server.finish(); + } + #[tokio::test] async fn an_expected_hash_refuses_a_body_that_changed_since_it_was_read() { let server = SequenceServer::spawn_scenarios([Scenario::new( @@ -393,9 +529,9 @@ mod tests { let output = edit_note( &server.client(), EditNoteInput { - workspace: Workspace::Team { + workspace: Some(Workspace::Team { team_path: "core".to_owned(), - }, + }), note_ref: "note-id".to_owned(), refresh: false, patch: patch.to_owned(), diff --git a/src/note/get.rs b/src/note/get.rs index 86b70af..280f7f2 100644 --- a/src/note/get.rs +++ b/src/note/get.rs @@ -22,14 +22,21 @@ use crate::{ pub(crate) struct GetNoteInput { /// Team path (from `hackmd_get_me`) when a direct internal note ID belongs /// to a team; omit for personal notes. `@owner/slug` URLs name their own - /// workspace. - #[serde(default, rename = "team_path", alias = "workspace")] - pub(crate) workspace: Workspace, - /// Internal API ID, `hackmd.io/`, or `hackmd.io/@owner/slug` URL. + /// workspace. With an `s/` link, only this team is searched; + /// omitted, the personal notes and then every team are. + #[serde( + default, + rename = "team_path", + alias = "workspace", + deserialize_with = "crate::models::given" + )] + pub(crate) workspace: Option, + /// Internal API ID, `hackmd.io/`, `hackmd.io/@owner/slug` URL, or legacy + /// `hackmd.io/s/` share link. A trailing `/download` is ignored. /// Give this or `local_path`, not both. pub(crate) note_ref: Option, /// Bypass the 60-second account and note-list caches when resolving an - /// `@owner/slug` URL. + /// `@owner/slug` URL or `s/` link. #[serde(default)] pub(crate) refresh: bool, /// Instead of `note_ref`: the absolute path of a Markdown file tracked by @@ -140,7 +147,7 @@ pub(crate) async fn get_note( (None, Some(local_path)) => { // The record names the note, so a team or a cache bypass here would // only look like it mattered. - if input.workspace != Workspace::Personal || input.refresh || !input.include_content { + if input.workspace.is_some() || input.refresh || !input.include_content { return Err(GetNoteError::SyncFilters); } let sync = check_note_sync(client, files, &local_path).await?; @@ -226,6 +233,44 @@ mod tests { note::reference::ResolvedNoteRef, }; + /// `null` is what clients send for an unset optional field, and older + /// clients send the tagged personal object by default, so both mean "not + /// given", as omission does. Both aliases read the same. + #[test] + fn a_null_team_path_is_the_same_as_none_given() { + let team_path = |input: serde_json::Value| { + serde_json::from_value::(input) + .expect("input should deserialize") + .workspace + }; + assert_eq!(team_path(json!({"note_ref": "id"})), None); + assert_eq!( + team_path(json!({"note_ref": "id", "team_path": null})), + None + ); + assert_eq!( + team_path(json!({"note_ref": "id", "workspace": null})), + None + ); + assert_eq!( + team_path(json!({"note_ref": "id", "workspace": {"kind": "personal"}})), + None + ); + let core = Some(Workspace::Team { + team_path: "core".to_owned(), + }); + assert_eq!( + team_path(json!({"note_ref": "id", "team_path": "core"})), + core + ); + assert_eq!( + team_path( + json!({"note_ref": "id", "workspace": {"kind": "team", "team_path": "core"}}) + ), + core + ); + } + fn response() -> NoteResponse { serde_json::from_value(json!({ "id": "internal-id", @@ -380,7 +425,7 @@ mod tests { &fixture.client(), &crate::fixture::scratch_files(), GetNoteInput { - workspace: Workspace::Personal, + workspace: None, note_ref: Some("https://hackmd.io/@alice/slug".to_owned()), refresh: false, local_path: None, diff --git a/src/note/image.rs b/src/note/image.rs index 00acc12..8d42b34 100644 --- a/src/note/image.rs +++ b/src/note/image.rs @@ -23,15 +23,22 @@ const IMAGE_MAX_BYTES: u64 = 10 * 1024 * 1024; pub(crate) struct UploadNoteImageInput { /// Accepted, as on every other tool, so older callers are not rejected /// outright. Not advertised: the upload route is the same for every note, - /// so this only cross-checks the owner of an `@owner/slug` URL. - #[serde(default, rename = "team_path", alias = "workspace")] + /// so this only cross-checks the owner of an `@owner/slug` URL and limits + /// the search for an `s/` link to that team. + #[serde( + default, + rename = "team_path", + alias = "workspace", + deserialize_with = "crate::models::given" + )] #[schemars(skip)] - pub(crate) workspace: Workspace, - /// Internal note ID, `hackmd.io/`, or `hackmd.io/@owner/slug` URL. + pub(crate) workspace: Option, + /// Internal note ID, `hackmd.io/`, `hackmd.io/@owner/slug` URL, or legacy + /// `hackmd.io/s/` share link. A trailing `/download` is ignored. /// Personal and team notes both accept uploads. pub(crate) note_ref: String, /// Bypass the 60-second account and note-list caches when resolving an - /// `@owner/slug` URL. + /// `@owner/slug` URL or `s/` link. #[serde(default)] pub(crate) refresh: bool, /// Absolute path to a local image file. Give this or `image_url`. @@ -366,7 +373,7 @@ mod tests { confirm_large_file: bool, ) -> UploadNoteImageInput { UploadNoteImageInput { - workspace: crate::models::Workspace::Personal, + workspace: None, note_ref: note_ref.to_owned(), refresh: false, image_path: Some(path.to_path_buf()), @@ -627,7 +634,7 @@ mod tests { confirm_large_file: bool, ) -> Result { let input = UploadNoteImageInput { - workspace: crate::models::Workspace::Personal, + workspace: None, note_ref: "id".to_owned(), refresh: false, image_path: None, diff --git a/src/note/list.rs b/src/note/list.rs index 56b57da..d77642e 100644 --- a/src/note/list.rs +++ b/src/note/list.rs @@ -26,8 +26,13 @@ pub(crate) struct ListNotesInput { /// personal workspace. History and trash are account-wide and take no /// team; for `tracked` a team narrows the records to that team's notes, /// and omitting it lists every workspace's. - #[serde(default, rename = "team_path", alias = "workspace")] - pub(crate) workspace: Workspace, + #[serde( + default, + rename = "team_path", + alias = "workspace", + deserialize_with = "crate::models::given" + )] + pub(crate) workspace: Option, /// Maximum notes returned after filtering (default 20, maximum 100). #[serde(default = "default_limit")] #[schemars(range(min = 1, max = 100))] @@ -196,15 +201,14 @@ fn list_tracked( } // An omitted team lists every workspace's records, not only personal ones: - // `Workspace` cannot tell "omitted" from "personal", and listing everything - // is what a caller without a team means here. + // that is what a caller without a team means here. Ok(list_tracked_notes( files, &ListTrackedNotesInput { limit: input.limit, offset: input.offset, query: input.query.as_deref(), - team: Some(&input.workspace).filter(|team| **team != Workspace::Personal), + team: input.workspace.as_ref(), }, )?) } @@ -219,7 +223,7 @@ pub(crate) async fn list_notes( } // History and trash are account-wide and never cached. let account_wide = |name| { - if input.workspace != Workspace::Personal { + if input.workspace.is_some() { Err(ListNotesError::AccountWideSource(name)) } else if input.refresh { Err(ListNotesError::UncachedRefresh(name)) @@ -238,9 +242,10 @@ pub(crate) async fn list_notes( let site = client.site_url(); match input.source { NoteSource::Workspace => { - let notes = client.list_notes(&input.workspace, input.refresh).await?; + let workspace = input.workspace.clone().unwrap_or_default(); + let notes = client.list_notes(&workspace, input.refresh).await?; Ok(filter_sort_page(site, ¬es, &input, sort, |_| { - input.workspace.clone() + workspace.clone() })) } NoteSource::History => { @@ -379,7 +384,7 @@ mod tests { fn input() -> ListNotesInput { ListNotesInput { source: NoteSource::Workspace, - workspace: Workspace::Personal, + workspace: None, limit: 20, offset: 0, query: None, @@ -423,9 +428,9 @@ mod tests { // A team narrows the records; this one is personal. let mut other_team = input(); other_team.source = NoteSource::Tracked; - other_team.workspace = Workspace::Team { + other_team.workspace = Some(Workspace::Team { team_path: "core".to_owned(), - }; + }); let ListOutput::Tracked(output) = list_notes(&client, &files, other_team) .await .expect("a team filter should list") @@ -663,9 +668,9 @@ mod tests { for source in [NoteSource::History, NoteSource::Trash] { let mut invalid = input(); invalid.source = source; - invalid.workspace = Workspace::Team { + invalid.workspace = Some(Workspace::Team { team_path: "core".to_owned(), - }; + }); assert!(matches!( remote(&client, invalid).await, Err(ListNotesError::AccountWideSource(_)) @@ -682,6 +687,32 @@ mod tests { } } + /// `null`, omission, and the older tagged personal object all name no + /// team, so an account-wide source passes its check and reaches the + /// network (here, the missing token). + #[tokio::test] + async fn account_wide_sources_accept_every_form_of_no_team() { + let client = HackmdClient::new(Config::for_tests()).expect("client should build"); + for team_path in [ + json!({}), + json!({"team_path": null}), + json!({"workspace": {"kind": "personal"}}), + ] { + let mut input = json!({"source": "history"}); + input + .as_object_mut() + .expect("an object") + .extend(team_path.as_object().expect("an object").clone()); + let input: ListNotesInput = + serde_json::from_value(input).expect("input should deserialize"); + assert_eq!(input.workspace, None); + assert!(matches!( + remote(&client, input).await, + Err(ListNotesError::Api(_)) + )); + } + } + #[tokio::test] async fn rejects_out_of_range_limit_before_network_io() { let client = HackmdClient::new(Config::for_tests()).expect("client should build"); diff --git a/src/note/reference.rs b/src/note/reference.rs index 3c307b0..957554b 100644 --- a/src/note/reference.rs +++ b/src/note/reference.rs @@ -44,12 +44,21 @@ pub(crate) enum NoteResolution { pub(crate) enum NoteRefError { #[error("note_ref must be a non-empty internal note ID or https://hackmd.io URL")] Empty, - #[error("note_ref URL must use https://hackmd.io and contain an ID or @owner/slug path")] + #[error( + "note_ref URL must use https://hackmd.io and contain an ID, @owner/slug, or s/ path" + )] InvalidUrl, #[error( "team_path {team_path:?} does not match the URL's owner @{owner}; omit team_path, since the URL names the workspace" )] WorkspaceMismatch { team_path: String, owner: String }, + #[error( + "{source}; this came from a team listing while searching every workspace for s/{short_id}, so pass team_path to search only the team that holds the note" + )] + ShortLinkTeam { + short_id: String, + source: HackmdError, + }, #[error(transparent)] Api(#[from] HackmdError), } @@ -62,33 +71,63 @@ impl crate::reply::ToolError for NoteRefError { Self::Empty | Self::InvalidUrl | Self::WorkspaceMismatch { .. } => { ErrorKind::InvalidInput } - Self::Api(error) => error.kind(), + Self::ShortLinkTeam { source: error, .. } | Self::Api(error) => error.kind(), } } } enum ParsedNoteRef { Direct(String), - Scoped { owner: String, slug: String }, + Scoped { + owner: String, + slug: String, + }, + /// A legacy `https://hackmd.io/s/` share link. It names no + /// workspace, so the note is looked up by `shortId`. + Short(String), } +/// Resolves `note_ref`. `team_path` is `None` when the caller gave none: a +/// direct ID is then personal, and an `s/` link is searched for in +/// every workspace. pub(crate) async fn resolve_note_ref( client: &HackmdClient, - workspace: Workspace, + team_path: Option, + note_ref: &str, + refresh: bool, +) -> Result { + resolve_note_ref_from(client, team_path, Workspace::Personal, note_ref, refresh).await +} + +/// As [`resolve_note_ref`], with a direct ID given no `team_path` read in +/// `home` rather than the personal workspace. +pub(crate) async fn resolve_note_ref_from( + client: &HackmdClient, + team_path: Option, + home: Workspace, note_ref: &str, refresh: bool, ) -> Result { match parse_note_ref(note_ref)? { ParsedNoteRef::Direct(note_id) => Ok(NoteResolution::Resolved { - note: ResolvedNoteRef { workspace, note_id }, + note: ResolvedNoteRef { + workspace: team_path.unwrap_or(home), + note_id, + }, }), + ParsedNoteRef::Short(short_id) => { + resolve_short_id(client, team_path, &short_id, refresh).await + } ParsedNoteRef::Scoped { owner, slug } => { // A URL names its own workspace. A team_path that names another is // a mistake to report, not a hint to drop. - if let Workspace::Team { team_path } = workspace - && team_path != owner + if let Some(Workspace::Team { team_path }) = &team_path + && *team_path != owner { - return Err(NoteRefError::WorkspaceMismatch { team_path, owner }); + return Err(NoteRefError::WorkspaceMismatch { + team_path: team_path.clone(), + owner, + }); } let workspace = if *owner == *client.user_path(refresh).await? { Workspace::Personal @@ -107,20 +146,66 @@ pub(crate) async fn resolve_note_ref( } } -/// Whether `note_ref` names a note by `@owner/slug`, which only finds notes -/// that are live in a workspace list. -pub(crate) fn is_slug_url(note_ref: &str) -> bool { - matches!(parse_note_ref(note_ref), Ok(ParsedNoteRef::Scoped { .. })) +/// Finds a `/s/` link's note. A `team_path` limits the search to that +/// workspace; without one, personal notes and then every team are listed, and +/// a `shortId` found in more than one workspace is reported as ambiguous. A +/// team whose list fails fails the search, since the note may be there, but +/// the error says that `team_path` narrows it. +async fn resolve_short_id( + client: &HackmdClient, + team_path: Option, + short_id: &str, + refresh: bool, +) -> Result { + let by_short_id = |note: &NoteResponse, query: &str| note.short_id.as_deref() == Some(query); + let candidates = if let Some(workspace) = team_path { + let notes = client.list_notes(&workspace, refresh).await?; + candidates_in(&workspace, short_id, ¬es, by_short_id) + } else { + let teams = client.list_teams(refresh).await?; + let mut candidates = Vec::new(); + let workspaces = + std::iter::once(Workspace::Personal).chain(teams.iter().map(|team| Workspace::Team { + team_path: team.path.clone(), + })); + for workspace in workspaces { + let notes = client + .list_notes(&workspace, refresh) + .await + .map_err(|source| match workspace { + Workspace::Personal => NoteRefError::from(source), + Workspace::Team { .. } => NoteRefError::ShortLinkTeam { + short_id: short_id.to_owned(), + source, + }, + })?; + candidates.extend(candidates_in(&workspace, short_id, ¬es, by_short_id)); + } + candidates + }; + Ok(resolution(format!("s/{short_id}"), candidates)) +} + +/// Whether resolving `note_ref` searches workspace lists (`@owner/slug` or +/// `s/`), which hold only live notes. +pub(crate) fn resolves_by_listing(note_ref: &str) -> bool { + matches!( + parse_note_ref(note_ref), + Ok(ParsedNoteRef::Scoped { .. } | ParsedNoteRef::Short(_)) + ) } fn parse_note_ref(note_ref: &str) -> Result { + // The host is case-insensitive, as the URL parser treats it below. + const HOST: &str = "hackmd.io/"; let value = note_ref.trim(); if value.is_empty() { return Err(NoteRefError::Empty); } let normalized_url = value - .strip_prefix("hackmd.io/") - .map(|path| format!("https://hackmd.io/{path}")); + .get(..HOST.len()) + .filter(|prefix| prefix.eq_ignore_ascii_case(HOST)) + .map(|_| format!("https://{HOST}{}", &value[HOST.len()..])); if !value.contains("://") && normalized_url.is_none() { return Ok(ParsedNoteRef::Direct(value.to_owned())); } @@ -139,8 +224,19 @@ fn parse_note_ref(note_ref: &str) -> Result { .filter(|segment| !segment.is_empty()) .map(percent_decode) .collect::, _>>()?; - match segments.as_slice() { + // A trailing `download` is the published Markdown of the same note. It is + // dropped only from a complete reference, so a slug that is itself + // `download` still names its note. + let segments = match segments.as_slice() { + [id, last] if last == "download" && !id.starts_with('@') && id != "s" => &segments[..1], + [_, _, last] if last == "download" => &segments[..2], + all => all, + }; + match segments { + // `s` alone is the share-link prefix without its shortId. + [note_id] if note_id == "s" => Err(NoteRefError::InvalidUrl), [note_id] if !note_id.starts_with('@') => Ok(ParsedNoteRef::Direct(note_id.clone())), + [prefix, short_id] if prefix == "s" => Ok(ParsedNoteRef::Short(short_id.clone())), [owner, slug] if owner.starts_with('@') && owner.len() > 1 && !slug.is_empty() => { Ok(ParsedNoteRef::Scoped { owner: owner[1..].to_owned(), @@ -187,7 +283,22 @@ fn unique_matches( where F: Fn(&NoteResponse, &str) -> bool, { - let candidates = notes + resolution( + query.to_owned(), + candidates_in(workspace, query, notes, matches), + ) +} + +fn candidates_in( + workspace: &Workspace, + query: &str, + notes: &[NoteResponse], + matches: F, +) -> Vec +where + F: Fn(&NoteResponse, &str) -> bool, +{ + notes .iter() .filter(|note| matches(note, query)) .map(|note| NoteCandidate { @@ -197,21 +308,20 @@ where short_id: note.short_id.clone(), permalink: note.permalink.clone(), }) - .collect::>(); - match candidates.as_slice() { - [] => NoteResolution::NotFound { - query: query.to_owned(), - }, - [candidate] => NoteResolution::Resolved { + .collect() +} + +/// One candidate resolves; none or several are reported, never guessed. +fn resolution(query: String, candidates: Vec) -> NoteResolution { + match <[NoteCandidate; 1]>::try_from(candidates) { + Ok([only]) => NoteResolution::Resolved { note: ResolvedNoteRef { - workspace: workspace.clone(), - note_id: candidate.note_id.clone(), + workspace: only.workspace, + note_id: only.note_id, }, }, - _ => NoteResolution::Ambiguous { - query: query.to_owned(), - candidates, - }, + Err(candidates) if candidates.is_empty() => NoteResolution::NotFound { query }, + Err(candidates) => NoteResolution::Ambiguous { query, candidates }, } } @@ -252,6 +362,38 @@ mod tests { assert!( matches!(parse_note_ref("https://hackmd.io/@core/slug"), Ok(ParsedNoteRef::Scoped { owner, slug }) if owner == "core" && slug == "slug") ); + assert!( + matches!(parse_note_ref("https://hackmd.io/@core/slug/download"), Ok(ParsedNoteRef::Scoped { owner, slug }) if owner == "core" && slug == "slug") + ); + assert!( + matches!(parse_note_ref("https://hackmd.io/s/SkE33UTHf?type=view"), Ok(ParsedNoteRef::Short(id)) if id == "SkE33UTHf") + ); + // A slug that is itself `download`, with and without the suffix. + for url in [ + "https://hackmd.io/@core/download", + "https://hackmd.io/@core/download/download", + ] { + assert!( + matches!(parse_note_ref(url), Ok(ParsedNoteRef::Scoped { owner, slug }) if owner == "core" && slug == "download"), + "{url}" + ); + } + assert!( + matches!(parse_note_ref("https://hackmd.io/api-id/download"), Ok(ParsedNoteRef::Direct(id)) if id == "api-id") + ); + assert!( + matches!(parse_note_ref("https://hackmd.io/s/SkE33UTHf/download"), Ok(ParsedNoteRef::Short(id)) if id == "SkE33UTHf") + ); + assert!( + matches!(parse_note_ref("hackmd.io/s/SkE33UTHf"), Ok(ParsedNoteRef::Short(id)) if id == "SkE33UTHf") + ); + // The host is case-insensitive, with or without a scheme; the path is not. + assert!( + matches!(parse_note_ref("HackMD.io/s/SkE33UTHf"), Ok(ParsedNoteRef::Short(id)) if id == "SkE33UTHf") + ); + assert!( + matches!(parse_note_ref("HACKMD.IO/@core/slug"), Ok(ParsedNoteRef::Scoped { owner, slug }) if owner == "core" && slug == "slug") + ); } #[test] @@ -282,6 +424,12 @@ mod tests { "https://example.com/id", "http://hackmd.io/id", "https://hackmd.io/@owner", + "https://hackmd.io/@core/slug/extra", + "https://hackmd.io/s", + "https://hackmd.io/s/", + // Only a lowercase `download` is known to be the published copy. + "https://hackmd.io/api-id/Download", + "https://hackmd.io/s/SkE33UTHf/Download", ] { assert!(parse_note_ref(value).is_err(), "accepted {value:?}"); } @@ -295,10 +443,38 @@ mod tests { "https://hackmd.io/short-id", "hackmd.io/short-id", ] { - let result = resolve_note_ref(&client, Workspace::Personal, value, false) + let result = resolve_note_ref(&client, None, value, false) .await .expect("direct reference should resolve without an API token"); - assert!(matches!(result, NoteResolution::Resolved { .. })); + assert!( + matches!(&result, NoteResolution::Resolved { note } if note.workspace == Workspace::Personal && !note.note_id.is_empty()) + ); + } + let core = Workspace::Team { + team_path: "core".to_owned(), + }; + let result = resolve_note_ref(&client, Some(core.clone()), "internal-id", false) + .await + .expect("a direct ID should resolve"); + assert!(matches!(&result, NoteResolution::Resolved { note } if note.workspace == core)); + // With no team_path, push reads a direct ID in the tracked note's own + // workspace; a given team_path still wins. + let other = Workspace::Team { + team_path: "other".to_owned(), + }; + for (team_path, expected) in [(None, &core), (Some(other.clone()), &other)] { + let result = super::resolve_note_ref_from( + &client, + team_path, + core.clone(), + "internal-id", + false, + ) + .await + .expect("a direct ID should resolve"); + assert!( + matches!(&result, NoteResolution::Resolved { note } if note.workspace == *expected) + ); } } @@ -319,14 +495,9 @@ mod tests { ), ]); let client = fixture.client(); - let result = resolve_note_ref( - &client, - Workspace::Personal, - "https://hackmd.io/@alice/slug", - false, - ) - .await - .expect("personal URL should resolve"); + let result = resolve_note_ref(&client, None, "https://hackmd.io/@alice/slug", false) + .await + .expect("personal URL should resolve"); assert!( matches!(result, NoteResolution::Resolved { note } if note.workspace == Workspace::Personal && note.note_id == "personal-id") ); @@ -340,9 +511,9 @@ mod tests { .expect("client should build"); let error = resolve_note_ref( &client, - Workspace::Team { + Some(Workspace::Team { team_path: "other".to_owned(), - }, + }), "https://hackmd.io/@alice/slug", false, ) @@ -384,22 +555,12 @@ mod tests { ]); let client = fixture.client_with_cache(); - let first = resolve_note_ref( - &client, - Workspace::Personal, - "https://hackmd.io/@alice/slug", - false, - ) - .await - .expect("first URL should resolve"); - let refreshed = resolve_note_ref( - &client, - Workspace::Personal, - "https://hackmd.io/@alice/slug", - true, - ) - .await - .expect("refreshed URL should resolve"); + let first = resolve_note_ref(&client, None, "https://hackmd.io/@alice/slug", false) + .await + .expect("first URL should resolve"); + let refreshed = resolve_note_ref(&client, None, "https://hackmd.io/@alice/slug", true) + .await + .expect("refreshed URL should resolve"); assert!(matches!(first, NoteResolution::Resolved { note } if note.note_id == "old")); assert!(matches!(refreshed, NoteResolution::Resolved { note } if note.note_id == "new")); @@ -435,8 +596,7 @@ mod tests { ), ]); let client = fixture.client_with_cache(); - let resolve = - |url: &'static str| resolve_note_ref(&client, Workspace::Personal, url, false); + let resolve = |url: &'static str| resolve_note_ref(&client, None, url, false); for _ in 0..2 { let team = resolve("https://hackmd.io/@core/slug") @@ -460,6 +620,191 @@ mod tests { assert_eq!(fixture.finish().len(), 4, "one request per resource"); } + /// A search of every workspace: the team list (team `core`), then the + /// personal notes, then core's notes, answered with `core`'s status. + fn every_workspace(personal: &str, core: (u16, &str)) -> [Scenario; 3] { + [ + Scenario::new( + "GET", + "/v1/teams", + 200, + r#"[{"id":"t","name":"Core","path":"core"}]"#, + ), + Scenario::new("GET", "/v1/notes", 200, personal), + Scenario::new("GET", "/v1/teams/core/notes", core.0, core.1), + ] + } + + #[tokio::test] + async fn a_short_link_without_team_path_searches_personal_then_teams() { + let fixture = SequenceServer::spawn_scenarios(every_workspace( + r#"[{"id":"mine","title":"Mine","shortId":"other"}]"#, + ( + 200, + r#"[{"id":"team-id","title":"Team","shortId":"SkE33UTHf","permalink":"c-linked-list"}]"#, + ), + )); + let result = resolve_note_ref( + &fixture.client(), + None, + "https://hackmd.io/s/SkE33UTHf", + false, + ) + .await + .expect("short link should resolve"); + assert!( + matches!(result, NoteResolution::Resolved { note } if note.workspace == Workspace::Team { team_path: "core".to_owned() } && note.note_id == "team-id") + ); + fixture.finish(); + } + + #[tokio::test] + async fn a_short_link_with_team_path_lists_only_that_team() { + let fixture = SequenceServer::spawn_scenarios([Scenario::new( + "GET", + "/v1/teams/core/notes", + 200, + r#"[{"id":"team-id","title":"Team","shortId":"SkE33UTHf"}]"#, + )]); + let result = resolve_note_ref( + &fixture.client(), + Some(Workspace::Team { + team_path: "core".to_owned(), + }), + "https://hackmd.io/s/missing", + false, + ) + .await + .expect("lookup should answer"); + assert!(matches!(result, NoteResolution::NotFound { query } if query == "s/missing")); + fixture.finish(); + } + + #[tokio::test] + async fn a_short_id_in_two_workspaces_is_ambiguous() { + let fixture = SequenceServer::spawn_scenarios(every_workspace( + r#"[{"id":"mine","title":"Mine","shortId":"dup"}]"#, + (200, r#"[{"id":"team-id","title":"Team","shortId":"dup"}]"#), + )); + let result = resolve_note_ref(&fixture.client(), None, "hackmd.io/s/dup", false) + .await + .expect("lookup should answer"); + assert!( + matches!(&result, NoteResolution::Ambiguous { candidates, .. } if candidates.len() == 2), + "{result:?}" + ); + fixture.finish(); + } + + #[tokio::test] + async fn a_short_download_link_to_a_personal_note_resolves_there() { + let fixture = SequenceServer::spawn_scenarios([ + Scenario::new("GET", "/v1/teams", 200, "[]"), + Scenario::new( + "GET", + "/v1/notes", + 200, + r#"[{"id":"mine","title":"Mine","shortId":"SkE33UTHf"}]"#, + ), + ]); + let result = resolve_note_ref( + &fixture.client(), + None, + "https://hackmd.io/s/SkE33UTHf/download", + false, + ) + .await + .expect("short link should resolve"); + assert!( + matches!(result, NoteResolution::Resolved { note } if note.workspace == Workspace::Personal && note.note_id == "mine") + ); + fixture.finish(); + } + + #[tokio::test] + async fn a_short_id_twice_in_one_workspace_is_ambiguous() { + let fixture = SequenceServer::spawn_scenarios([Scenario::new( + "GET", + "/v1/teams/core/notes", + 200, + r#"[{"id":"a","title":"A","shortId":"dup"},{"id":"b","title":"B","shortId":"dup"}]"#, + )]); + let result = resolve_note_ref( + &fixture.client(), + Some(Workspace::Team { + team_path: "core".to_owned(), + }), + "https://hackmd.io/s/dup", + false, + ) + .await + .expect("lookup should answer"); + assert!( + matches!(&result, NoteResolution::Ambiguous { query, candidates } if query == "s/dup" && candidates.len() == 2), + "{result:?}" + ); + fixture.finish(); + } + + #[tokio::test] + async fn repeated_short_links_reuse_the_cache_until_refresh() { + let [teams, old, core] = every_workspace( + r#"[{"id":"old","title":"Old","shortId":"SkE33UTHf"}]"#, + (200, "[]"), + ); + let [teams_again, new, core_again] = every_workspace( + r#"[{"id":"new","title":"New","shortId":"SkE33UTHf"}]"#, + (200, "[]"), + ); + let fixture = + SequenceServer::spawn_scenarios([teams, old, core, teams_again, new, core_again]); + let client = fixture.client_with_cache(); + let resolve = + |refresh| resolve_note_ref(&client, None, "https://hackmd.io/s/SkE33UTHf", refresh); + for _ in 0..2 { + let cached = resolve(false).await.expect("short link should resolve"); + assert!(matches!(cached, NoteResolution::Resolved { note } if note.note_id == "old")); + } + let refreshed = resolve(true).await.expect("short link should resolve"); + assert!(matches!(refreshed, NoteResolution::Resolved { note } if note.note_id == "new")); + assert_eq!( + fixture.finish().len(), + 6, + "one request per resource per refresh" + ); + } + + #[tokio::test] + async fn a_failed_team_listing_says_team_path_narrows_the_search() { + let fixture = SequenceServer::spawn_scenarios(every_workspace( + r#"[{"id":"mine","title":"Mine","shortId":"SkE33UTHf"}]"#, + (403, r#"{"error":"forbidden"}"#), + )); + let error = resolve_note_ref( + &fixture.client(), + None, + "https://hackmd.io/s/SkE33UTHf", + false, + ) + .await + .expect_err("a team that cannot be listed may hold the note"); + assert!( + matches!(&error, super::NoteRefError::ShortLinkTeam { short_id, .. } if short_id == "SkE33UTHf") + ); + // The hint keeps the kind of the error it wraps. + assert_eq!( + crate::reply::ToolError::kind(&error), + crate::reply::ErrorKind::Forbidden + ); + assert!( + error + .to_string() + .ends_with("; this came from a team listing while searching every workspace for s/SkE33UTHf, so pass team_path to search only the team that holds the note"), + "{error}" + ); + fixture.finish(); + } + #[tokio::test] async fn scoped_team_slug_resolves_through_team_discovery() { let fixture = SequenceServer::spawn_scenarios([ @@ -483,14 +828,9 @@ mod tests { ), ]); let client = fixture.client(); - let result = resolve_note_ref( - &client, - Workspace::Personal, - "https://hackmd.io/@core/slug", - false, - ) - .await - .expect("team URL should resolve"); + let result = resolve_note_ref(&client, None, "https://hackmd.io/@core/slug", false) + .await + .expect("team URL should resolve"); assert!( matches!(result, NoteResolution::Resolved { note } if note.workspace == Workspace::Team { team_path: "core".to_owned() } && note.note_id == "team-id") ); diff --git a/src/server.rs b/src/server.rs index 35c35f7..8f546d4 100644 --- a/src/server.rs +++ b/src/server.rs @@ -90,7 +90,7 @@ pub(crate) async fn run_stdio() -> Result<(), Box> { instructions = "Before editing an existing note, fetch it with hackmd_get_note, preserve unrelated passages, and pass its body_hash as expected_hash to hackmd_update_note. Prefer a context-checked patch. On a conflict, fetch again and rebuild the change; do not blindly retry an uncertain write or create. HackMD has no atomic conditional writes, so concurrent edits can still race.\n\ Locate notes with hackmd_list_notes metadata filters, then fetch only relevant candidates to search their bodies. There is no full-text search or revision diff; history is the account's history list, not edit history. Do not pick arbitrarily among ambiguous destinations.\n\ Use hackmd_get_me to identify team_path for shared workspaces. folder_ids is the ancestor chain of the note's folder; parent_folder_id sets that folder.\n\ - When sharing, check the intended audience and read_permission, including unconfirmed_fields after creation. When present, note_url identifies a note but grants no access and is not an invite or published link. This server does not create invite links or publish pages; use the HackMD web UI. The title field always wins; the body supplies a title only when a note is created without one, and editing a body's H1 later never renames the note, so pull and push report title_drift." + When sharing, check the intended audience and read_permission, including unconfirmed_fields after creation. When present, note_url identifies a note but grants no access and is not an invite or published link. This server does not create invite links or publish pages; use the HackMD web UI. The title field always wins; the body supplies a title only when a note is created without one, and editing a body's H1 later never renames the note, so pull, push, and patch edits report title_drift." )] impl rmcp::ServerHandler for HackmdServer { /// Wraps every dispatch in one span and one retry scope, so each tool call @@ -323,6 +323,28 @@ mod tests { assert_eq!(Arc::strong_count(&client), 1); } + /// An optional `team_path` advertises the same nullable string as before. + #[test] + fn optional_team_paths_keep_the_nullable_string_schema() { + let tools = HackmdServer::router().list_all(); + for name in [ + "hackmd_get_note", + "hackmd_list_notes", + "hackmd_update_note", + "hackmd_delete_note", + "hackmd_pull_note", + "hackmd_push_note", + ] { + let tool = tools + .iter() + .find(|tool| tool.name == name) + .expect("tool should exist"); + let team_path = &tool.input_schema["properties"]["team_path"]; + assert_eq!(team_path["type"], json!(["string", "null"]), "{name}"); + assert!(team_path.get("anyOf").is_none(), "{name}"); + } + } + #[test] fn tools_have_generated_schemas_and_exact_annotations() { let tools = HackmdServer::router().list_all(); diff --git a/src/server/note.rs b/src/server/note.rs index 1ac54c2..dc4eeff 100644 --- a/src/server/note.rs +++ b/src/server/note.rs @@ -45,7 +45,7 @@ impl HackmdServer { #[tool( name = "hackmd_get_note", - description = "Get one HackMD note with full content, normalized metadata, folder_ids, the exact patch_path a hackmd_update_note patch needs, and body_hash for its expected_hash; with note_ref, include_content=false leaves the body out and keeps the rest. Given local_path instead of note_ref, report that tracked file's sync state without writing: in_sync, local_changed, remote_changed, or conflict, with SHA-256 hashes. For @owner/slug references, refresh=true bypasses the 60-second caches.", + description = "Get one HackMD note with full content, normalized metadata, folder_ids, the exact patch_path a hackmd_update_note patch needs, and body_hash for its expected_hash; with note_ref, include_content=false leaves the body out and keeps the rest. Given local_path instead of note_ref, report that tracked file's sync state without writing: in_sync, local_changed, remote_changed, or conflict, with SHA-256 hashes. For @owner/slug and s/ references, refresh=true bypasses the 60-second caches.", annotations( title = "Get HackMD Note", read_only_hint = true, @@ -94,7 +94,7 @@ impl HackmdServer { #[tool( name = "hackmd_update_note", - description = "Edit a HackMD note's body with a patch (the default and safe way), or update its metadata, or replace the whole body with content. The destructive hint is for content; a patch is context-checked and changes only the body lines its hunks name. It applies only when every hunk's context matches the current body exactly once, then the write is confirmed by reading it back. Pass expected_hash (body_hash from hackmd_get_note) with any update to refuse the write if the body changed since you read it. A metadata-only update reads the current body and sends it back with the change: an edit landing between that read and the write is reverted, and expected_hash catches any made before it. An explicit title always takes effect; body edits never rename the note. Format:\n*** Begin Patch\n*** Update File: \n@@ optional anchor line\n context line\n-removed line\n+added line\n*** End Patch\nText after @@ is an anchor that must equal exactly one line, ignoring leading and trailing whitespace; the hunk then applies after it, and an addition-only hunk is inserted directly below it. A line range such as @@ -3,4 +3,5 @@ is not an anchor. A hunk closed by *** End of File must match the end of the body, and an addition-only one appends there. A patch goes in a call of its own. content overwrites the complete body, and nothing this server offers can undo it. For @owner/slug references, refresh=true bypasses the 60-second caches.", + description = "Edit a HackMD note's body with a patch (the default and safe way), or update its metadata, or replace the whole body with content. The destructive hint is for content; a patch is context-checked and changes only the body lines its hunks name. It applies only when every hunk's context matches the current body exactly once, then the write is confirmed by reading it back. Pass expected_hash (body_hash from hackmd_get_note) with any update to refuse the write if the body changed since you read it. A metadata-only update reads the current body and sends it back with the change: an edit landing between that read and the write is reverted, and expected_hash catches any made before it. An explicit title always takes effect; body edits never rename the note. Format:\n*** Begin Patch\n*** Update File: \n@@ optional anchor line\n context line\n-removed line\n+added line\n*** End Patch\nText after @@ is an anchor that must equal exactly one line, ignoring leading and trailing whitespace; the hunk then applies after it, and an addition-only hunk is inserted directly below it. A line range such as @@ -3,4 +3,5 @@ is not an anchor. A hunk closed by *** End of File must match the end of the body, and an addition-only one appends there. A patch that changes the title the body implies (front-matter title: or a leading H1) reports it as title_drift when the listed title matched the body's title before the patch; set title in a separate update. A patch goes in a call of its own. content overwrites the complete body, and nothing this server offers can undo it. For @owner/slug and s/ references, refresh=true bypasses the 60-second caches.", annotations( title = "Update HackMD Note", read_only_hint = false, @@ -125,7 +125,7 @@ impl HackmdServer { #[tool( name = "hackmd_delete_note", - description = "Delete a HackMD note, or with restore: true bring a personal note back from trash (note_ref is then the internal ID from hackmd_list_notes with source: trash). Only personal notes can be restored through the API. For @owner/slug references, refresh=true bypasses the 60-second caches.", + description = "Delete a HackMD note, or with restore: true bring a personal note back from trash (note_ref is then the internal ID from hackmd_list_notes with source: trash). Only personal notes can be restored through the API. For @owner/slug and s/ references, refresh=true bypasses the 60-second caches.", annotations( title = "Delete or Restore HackMD Note", read_only_hint = false, @@ -153,7 +153,7 @@ impl HackmdServer { #[tool( name = "hackmd_upload_note_image", - description = "Upload an image to a HackMD note (personal or team) and return its HackMD CDN link. Give image_path for a local file, or image_url to re-host an image from a public https URL (such as an imgur link in a note being migrated) without saving it locally first. publicly_readable is what one signed-out HEAD of the link showed right after the upload: true when an image or a redirect to presigned storage came back, false when it was refused, null when the check could not run or proved nothing. A false usually means the note is not guest-readable, and signed-out readers will not see the image until it is. A local image must lie under HACKMD_MCP_WORKSPACE_ROOT; with no root configured, local uploads are refused. image_url is fetched only over https on the default port from a host whose every address is public, following at most 5 redirects. Either way the bytes must start with a PNG, JPEG, GIF, or WebP signature. Files above 5 MiB require confirmation; files above 10 MiB are refused. For @owner/slug references, refresh=true bypasses the 60-second caches.", + description = "Upload an image to a HackMD note (personal or team) and return its HackMD CDN link. Give image_path for a local file, or image_url to re-host an image from a public https URL (such as an imgur link in a note being migrated) without saving it locally first. publicly_readable is what one signed-out HEAD of the link showed right after the upload: true when an image or a redirect to presigned storage came back, false when it was refused, null when the check could not run or proved nothing. A false usually means the note is not guest-readable, and signed-out readers will not see the image until it is. A local image must lie under HACKMD_MCP_WORKSPACE_ROOT; with no root configured, local uploads are refused. image_url is fetched only over https on the default port from a host whose every address is public, following at most 5 redirects. Either way the bytes must start with a PNG, JPEG, GIF, or WebP signature. Files above 5 MiB require confirmation; files above 10 MiB are refused. For @owner/slug and s/ references, refresh=true bypasses the 60-second caches.", annotations( title = "Upload HackMD Note Image", read_only_hint = false, diff --git a/src/server/sync.rs b/src/server/sync.rs index 89cbca3..fe84f75 100644 --- a/src/server/sync.rs +++ b/src/server/sync.rs @@ -33,7 +33,7 @@ impl HackmdServer { #[tool( name = "hackmd_pull_note", - description = "Pull one HackMD note's exact Markdown body to an absolute local .md path and record a private sync baseline, which starts tracking the file. An existing file needs overwrite_local: true. A tracked file with unpushed edits, or a file with no usable sync record whose content differs from the note, also needs discard_local_changes: true; a file that already matches the note is simply adopted. For @owner/slug references, refresh=true bypasses the 60-second caches. The result returns body_hash, the sha256 of the bytes just written, matching the form hackmd_push_note and hackmd_get_note report. When it replaced a file it could read as text, changes is a bounded diff from that file to the note. title_drift is the title the body now gives (front-matter title:, or an H1 that is its first line of text) when the listed title differs although the previously synced body gave the listed title; it is left out on a first pull, when the titles always differed, and when a title cannot be read for certain. HackMD never renames a note for a body edit, so pass it as title to hackmd_update_note if the listing should follow.", + description = "Pull one HackMD note's exact Markdown body to an absolute local .md path and record a private sync baseline, which starts tracking the file. An existing file needs overwrite_local: true. A tracked file with unpushed edits, or a file with no usable sync record whose content differs from the note, also needs discard_local_changes: true; a file that already matches the note is simply adopted. For @owner/slug and s/ references, refresh=true bypasses the 60-second caches. The result returns body_hash, the sha256 of the bytes just written, matching the form hackmd_push_note and hackmd_get_note report. When it replaced a file it could read as text, changes is a bounded diff from that file to the note. title_drift is the title the body now gives (front-matter title:, or an H1 that is its first line of text) when the listed title differs although the previously synced body gave the listed title; it is left out on a first pull, when the titles always differed, and when a title cannot be read for certain. HackMD never renames a note for a body edit, so pass it as title to hackmd_update_note if the listing should follow.", annotations( title = "Pull HackMD Note", read_only_hint = false, diff --git a/src/sync/pull.rs b/src/sync/pull.rs index d076ffc..169c0bb 100644 --- a/src/sync/pull.rs +++ b/src/sync/pull.rs @@ -23,13 +23,20 @@ use crate::{ pub(crate) struct PullNoteInput { /// Team path (from `hackmd_get_me`) when a direct internal note ID belongs /// to a team; omit for personal notes. `@owner/slug` URLs name their own - /// workspace. - #[serde(default, rename = "team_path", alias = "workspace")] - pub(crate) workspace: Workspace, - /// Internal API ID, `hackmd.io/`, or `hackmd.io/@owner/slug` URL. + /// workspace. With an `s/` link, only this team is searched; + /// omitted, the personal notes and then every team are. + #[serde( + default, + rename = "team_path", + alias = "workspace", + deserialize_with = "crate::models::given" + )] + pub(crate) workspace: Option, + /// Internal API ID, `hackmd.io/`, `hackmd.io/@owner/slug` URL, or legacy + /// `hackmd.io/s/` share link. A trailing `/download` is ignored. pub(crate) note_ref: String, /// Bypass the 60-second account and note-list caches when resolving an - /// `@owner/slug` URL. + /// `@owner/slug` URL or `s/` link. #[serde(default)] pub(crate) refresh: bool, /// Absolute destination path ending in `.md` for the exact remote body. @@ -332,7 +339,7 @@ mod tests { use serde_json::json; use super::{NoteResolution, PullNoteError, PullNoteInput, PullNoteOutput, pull_note}; - use crate::{client::HackmdClient, config::Config, local::LocalFiles, models::Workspace}; + use crate::{client::HackmdClient, config::Config, local::LocalFiles}; #[tokio::test] async fn a_workspace_root_stops_a_write_outside_it() { @@ -342,7 +349,7 @@ mod tests { let client = HackmdClient::new(Config::for_tests()).expect("client should build"); let files = LocalFiles::new(directory.path().join("state"), Some(root)); let input = PullNoteInput { - workspace: Workspace::Personal, + workspace: None, note_ref: "note-id".to_owned(), refresh: false, local_path: directory.path().join("outside.md"), @@ -379,7 +386,7 @@ mod tests { &client, &files, PullNoteInput { - workspace: Workspace::Personal, + workspace: None, note_ref: "note/id".to_owned(), refresh: false, local_path: destination.clone(), @@ -474,7 +481,7 @@ mod tests { &fixture.client(), &files, PullNoteInput { - workspace: Workspace::Personal, + workspace: None, note_ref: "large-note".to_owned(), refresh: false, local_path: destination.clone(), @@ -515,7 +522,7 @@ mod tests { let existing = directory.path().join("note.md"); fs::write(&existing, "local").expect("fixture should write"); let input = PullNoteInput { - workspace: Workspace::Personal, + workspace: None, note_ref: "id".to_owned(), refresh: false, local_path: existing, @@ -532,7 +539,7 @@ mod tests { let non_markdown = directory.path().join("note.txt"); fs::write(&non_markdown, "local").expect("fixture should write"); let input = PullNoteInput { - workspace: Workspace::Personal, + workspace: None, note_ref: "id".to_owned(), refresh: false, local_path: non_markdown, @@ -547,7 +554,7 @@ mod tests { )); let input = PullNoteInput { - workspace: Workspace::Personal, + workspace: None, note_ref: "id".to_owned(), refresh: false, local_path: { @@ -567,7 +574,7 @@ mod tests { let missing_parent = directory.path().join("missing/note.md"); let input = PullNoteInput { - workspace: Workspace::Personal, + workspace: None, note_ref: "id".to_owned(), refresh: false, local_path: missing_parent.clone(), @@ -608,7 +615,7 @@ mod tests { fs::write(&editor_path, "saved mid-pull").expect("editor save should write"); })]); let input = PullNoteInput { - workspace: Workspace::Personal, + workspace: None, note_ref: "note-id".to_owned(), refresh: false, local_path: local_path.clone(), @@ -632,7 +639,7 @@ mod tests { fn overwrite_input(local_path: &std::path::Path, discard_local_changes: bool) -> PullNoteInput { PullNoteInput { - workspace: Workspace::Personal, + workspace: None, note_ref: "note-id".to_owned(), refresh: false, local_path: local_path.to_path_buf(), diff --git a/src/sync/push.rs b/src/sync/push.rs index fdccd74..2315cde 100644 --- a/src/sync/push.rs +++ b/src/sync/push.rs @@ -35,14 +35,21 @@ pub(crate) struct PushNoteInput { /// reference resolves to the note the sync record names. Omit it to save /// the lookup. pub(crate) note_ref: Option, - /// Team path (from `hackmd_get_me`) for a direct internal `note_ref`. - /// Omitted, the tracked note's own workspace is assumed; `@owner/slug` - /// URLs name their own. Without `note_ref`, one naming another workspace - /// than the sync record is refused. - #[serde(default, rename = "team_path", alias = "workspace")] - pub(crate) workspace: Workspace, + /// Team path (from `hackmd_get_me`) for a direct internal `note_ref`, or + /// the one team to search for an `s/` link. Omitted, a direct ID + /// is read in the tracked note's own workspace and a short link is + /// searched for in every workspace; `@owner/slug` URLs name their own. + /// Without `note_ref`, one naming another workspace than the sync record + /// is refused. + #[serde( + default, + rename = "team_path", + alias = "workspace", + deserialize_with = "crate::models::given" + )] + pub(crate) workspace: Option, /// Bypass the 60-second account and note-list caches when resolving an - /// `@owner/slug` `note_ref`. + /// `@owner/slug` or `s/` `note_ref`. #[serde(default)] pub(crate) refresh: bool, /// `safe` (default) writes only when the remote still matches the @@ -203,8 +210,10 @@ pub(crate) async fn push_note( // it is redundant but harmless, as it always was; one that names another // workspace means the caller has the wrong file or note in mind. if input.note_ref.is_none() - && input.workspace != Workspace::Personal - && input.workspace != note.workspace + && input + .workspace + .as_ref() + .is_some_and(|workspace| *workspace != note.workspace) { return Err(PushNoteError::TrackedWorkspaceMismatch); } @@ -212,15 +221,14 @@ pub(crate) async fn push_note( // A bare note ID without team_path is read in the tracked note's own // workspace: the cross-check is about which note, and omitting // team_path should not make a team note look like a different one. - let workspace = match input.workspace { - Workspace::Personal if !crate::note::reference::is_slug_url(note_ref) => { - note.workspace.clone() - } - other => other, - }; - let resolution = - crate::note::reference::resolve_note_ref(client, workspace, note_ref, input.refresh) - .await?; + let resolution = crate::note::reference::resolve_note_ref_from( + client, + input.workspace.clone(), + note.workspace.clone(), + note_ref, + input.refresh, + ) + .await?; let NoteResolution::Resolved { note: named } = resolution else { return Ok(Err(resolution)); }; @@ -656,7 +664,7 @@ mod tests { PushNoteInput { local_path: path.to_path_buf(), note_ref: None, - workspace: Workspace::Personal, + workspace: None, refresh: false, strategy, confirm, @@ -1470,7 +1478,7 @@ mod tests { "baseline", ); assert!(matches!( - push_note(&client, &files, pushing(team(stray), false)).await, + push_note(&client, &files, pushing(Some(team(stray)), false)).await, Err(PushNoteError::TrackedWorkspaceMismatch) )); } @@ -1485,7 +1493,7 @@ mod tests { // The record's own team and an unused refresh go through, as they did // before the check existed: they get as far as the network. - for (workspace, refresh) in [(team("core"), false), (Workspace::Personal, true)] { + for (workspace, refresh) in [(Some(team("core")), false), (None, true)] { assert!(matches!( push_note(&client, &files, pushing(workspace, refresh)).await, Err(PushNoteError::Api(_)) @@ -1613,4 +1621,50 @@ mod tests { Err(PushNoteError::TrackingMismatch) )); } + + #[tokio::test] + async fn a_short_link_to_another_workspaces_note_is_refused() { + let directory = tempfile::tempdir().expect("temp directory should create"); + let local_path = directory.path().join("note.md"); + fs::write(&local_path, "baseline").expect("local fixture should write"); + let files = crate::fixture::tracked_files_in( + Workspace::Team { + team_path: "core".to_owned(), + }, + directory.path(), + "note-id", + &local_path, + "baseline", + ); + + // The link names no workspace, so every one is searched rather than + // only the tracked team, and the personal note it names is reported. + let fixture = SequenceServer::spawn_scenarios([ + Scenario::new( + "GET", + "/v1/teams", + 200, + r#"[{"id":"t","name":"Core","path":"core"}]"#, + ), + Scenario::new( + "GET", + "/v1/notes", + 200, + r#"[{"id":"mine","title":"Mine","shortId":"SkE33UTHf"}]"#, + ), + Scenario::new( + "GET", + "/v1/teams/core/notes", + 200, + r#"[{"id":"note-id","title":"Note","shortId":"other"}]"#, + ), + ]); + let mut named = input(&local_path, PushStrategy::Safe, false); + named.note_ref = Some("https://hackmd.io/s/SkE33UTHf".to_owned()); + assert!(matches!( + push_note(&fixture.client(), &files, named).await, + Err(PushNoteError::TrackingMismatch) + )); + fixture.finish(); + } } diff --git a/src/sync/tracking.rs b/src/sync/tracking.rs index e600a37..c44dc24 100644 --- a/src/sync/tracking.rs +++ b/src/sync/tracking.rs @@ -18,7 +18,7 @@ pub(crate) struct ListTrackedNotesInput<'a> { pub(crate) offset: usize, /// Case-insensitive match against the note ID and the local path. pub(crate) query: Option<&'a str>, - /// Only this team's records, when given. + /// Only this workspace's records, when given. pub(crate) team: Option<&'a Workspace>, }