Accept legacy share links and report patch drift

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/<shortId>`, 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/<id>`, 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

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()),

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<Option<Workspace>, D::Error> {
+    Ok(Option::<Workspace>::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 {

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/<id>`, or `hackmd.io/@owner/slug` URL.
+    /// workspace. With an `s/<shortId>` 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<Workspace>,
+    /// Internal API ID, `hackmd.io/<id>`, `hackmd.io/@owner/slug` URL, or legacy
+    /// `hackmd.io/s/<shortId>` 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/<shortId>` 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/<id>`, or `hackmd.io/@owner/slug` URL.
+    /// workspace. With an `s/<shortId>` 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<Workspace>,
+    /// Internal API ID, `hackmd.io/<id>`, `hackmd.io/@owner/slug` URL, or legacy
+    /// `hackmd.io/s/<shortId>` 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/<shortId>` 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/<shortId> to resolve, and refresh does not apply"
     )]
     RestoreNeedsId,
     #[error(
@@ -688,7 +702,9 @@ pub(crate) async fn delete_note(
 ) -> Result<Result<DeleteNoteOutput, NoteResolution>, 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");

src/note/edit.rs

@@ -4,15 +4,15 @@ 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},
 };
 
 /// What `hackmd_update_note` passes on when given a `patch`.
 #[derive(Debug)]
 pub(crate) struct EditNoteInput {
-    pub(crate) workspace: Workspace,
+    pub(crate) workspace: Option<Workspace>,
     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<String>,
 }
 
 #[derive(Debug, Error)]
@@ -119,21 +124,32 @@ pub(crate) async fn edit_note(
     drop(current);
     let patch_path = crate::note::patch::patch_path(&note.workspace, &note.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(&note.workspace, &note.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, &note(title, old)),
+            Scenario::new("PATCH", "/v1/notes/note-id", 202, ""),
+            Scenario::new(
+                "GET",
+                "/v1/notes/note-id",
+                200,
+                &note(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(),