Two /simplify slices, and five bugs they turned up

gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Args.scala

@@ -41,7 +41,7 @@ object Args:
           if name.isEmpty then Left("empty flag: '--'")
           else if name.contains('=') then
             val (k, v) = name.span(_ != '=')
-            Right(()).flatMap(_ => loop(tail, positional, values + (k -> v.drop(1)), switches))
+            loop(tail, positional, values + (k -> v.drop(1)), switches)
           else if ValueFlags.contains(name) then
             tail match
               case v :: more if !v.startsWith("--") =>

gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Main.scala

@@ -38,18 +38,23 @@ object Main:
       if debug then message => System.err.println(s"gx[protocol] $message")
       else _ => ()
 
+    // One clock for the whole process: the audit log stamps its lines from the
+    // same source as every `updatedAt` and `lastSyncAt`, so a record and the
+    // line describing it cannot disagree about when it happened.
+    val clock: () => Long = () => System.currentTimeMillis()
+
     val env = CliEnv(
       store = LibraryStore.default(gxHome),
       policy = AccessPolicy.fromEnv(),
-      audit = Audit(runtime.resolve("audit.log.jsonl")),
+      audit = Audit(runtime.resolve("audit.log.jsonl"), clock),
       // The user's shell, not the process's idea of it. v1 learned this the hard
       // way: the desktop's working directory is an artifact of how it was
       // launched, so paths must be resolved where the human typed them.
       cwd = Paths.get("").toAbsolutePath,
       out = println,
       err = System.err.println,
       stdin = () => String(System.in.readAllBytes(), StandardCharsets.UTF_8), // V-16
-      now = () => System.currentTimeMillis(),
+      now = clock,
       desktopRunning = () => Main.desktopRunning(control, trace),
       rpc = (method, params) => Main.call(control, trace, method, params)
     )

gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala

@@ -6,6 +6,7 @@ import org.jpablo.graphexplorer.gxcore.rpc.ChannelError
 import org.jpablo.graphexplorer.gxcore.store.LibraryStore
 
 import java.nio.file.{Files, Path, Paths}
+import java.nio.file.attribute.PosixFilePermissions
 
 import scala.jdk.CollectionConverters.*
 
@@ -54,7 +55,9 @@ class CliSpec extends FunSuite:
     val env: CliEnv = CliEnv(
       store = store,
       policy = AccessPolicy(Nil, Nil),
-      audit = Audit(dir.resolve("audit.jsonl")),
+      // Same clock the CLI uses, so an audit line's timestamp is a fact a test
+      // can assert rather than whatever the wall clock said.
+      audit = Audit(dir.resolve("audit.jsonl"), () => clock),
       cwd = dir,
       out = s => out.append(s).append('\n'),
       err = s => err.append(s).append('\n'),
@@ -256,6 +259,148 @@ class CliSpec extends FunSuite:
     assert(r.stdout.contains("nothing bound"), r.stdout)
   }
 
+  /** A ref that resolves to nothing is a typo, not an empty selection. This
+    * used to be flat-mapped away: "(nothing bound to sync)" and exit 0, which
+    * tells a script that mistyped a name that it succeeded.
+    */
+  tmp.test("sync on a ref that matches nothing fails, it does not report success") { dir =>
+    dot(dir, "gen.dot", "v1")
+    val i = Run(dir)
+    i("import", "gen.dot", "--mode", "sync")
+
+    val s = Run(dir)
+    assertNotEquals(s("sync", "typo"), ExitCode.Ok, s.stdout)
+    assert(s.stderr.contains("no diagram matches"), s.stderr)
+    assert(!s.stdout.contains("nothing bound"), s.stdout)
+  }
+
+  /** A reconciliation that cannot be persisted is not a success. The three
+    * saves inside syncOne discarded their Either, so gx printed `Behind`,
+    * exited 0, and left the record on its old baseline — and the next run
+    * redid the same work from the same stale state, silently.
+    */
+  tmp.test("a sync that cannot save the record reports it and does not exit 0") { dir =>
+    val f = dot(dir, "gen.dot", "v1")
+    val i = Run(dir)
+    i("import", "gen.dot", "--mode", "pull")
+
+    Files.writeString(f, "v2") // the origin moves, so Pull has something to save
+
+    // Readable and listable, but nothing new can be created in it — so the
+    // scan still finds the record and only the WRITE fails.
+    val diagrams = dir.resolve("library").resolve("diagrams")
+    val restore  = Files.getPosixFilePermissions(diagrams)
+    Files.setPosixFilePermissions(diagrams, PosixFilePermissions.fromString("r-xr-xr-x"))
+    try
+      // Root ignores the mode bits; there is nothing to assert on such a box.
+      assume(
+        scala.util.Try(Files.createTempFile(diagrams, "probe", null)).isFailure,
+        "this user can write to a read-only directory"
+      )
+
+      val s = Run(dir)
+      assertNotEquals(s("sync", "--all"), ExitCode.Ok, s.stdout)
+      assert(s.stderr.contains("could not be saved"), s.stderr)
+    finally Files.setPosixFilePermissions(diagrams, restore)
+  }
+
+  // ------------------------------------------------------- ambiguous refs
+  //
+  // findInLibrary's own doc says ambiguity is reported rather than resolved by
+  // picking one. It returned None for "nothing matched" AND "several matched",
+  // so callers rendered both as "no diagram matches" — and the ones that fall
+  // back to a PATH did so on an ambiguous name.
+
+  /** Two records deliberately sharing a name; only their ids differ. */
+  private def twoNamed(dir: Path, name: String): Run =
+    dot(dir, "a.dot", "digraph { a }")
+    dot(dir, "b.dot", "digraph { b }")
+    val i = Run(dir)
+    i("import", "a.dot", "--name", name)
+    i("import", "b.dot", "--name", name)
+    i
+
+  tmp.test("an ambiguous ref is reported as ambiguous, with the ids to pick from") { dir =>
+    twoNamed(dir, "Shared Thing")
+    val r = Run(dir)
+    assertNotEquals(r("get", "Shared Thing"), ExitCode.Ok, r.stdout)
+    assert(r.stderr.contains("matches 2 diagrams"), r.stderr)
+    assert(!r.stderr.contains("no diagram matches"), "ambiguity reported as absence")
+  }
+
+  /** The wrong-diagram write the doc warns about, in its real form: with no
+    * ambiguity case, `set` fell through to treating the ref as a path and
+    * created a FILE called `shared` instead of refusing.
+    */
+  tmp.test("an ambiguous ref does not fall through to being a path") { dir =>
+    twoNamed(dir, "Shared Thing")
+    val r = Run(dir, stdinText = "digraph { c }")
+    assertNotEquals(r("set", "Shared Thing", "--stdin"), ExitCode.Ok, r.stdout)
+    assert(r.stderr.contains("matches 2 diagrams"), r.stderr)
+    assert(!Files.exists(dir.resolve("Shared Thing")), "set wrote a file named after an ambiguous ref")
+  }
+
+  tmp.test("an unambiguous id still resolves when its name is shared") { dir =>
+    val i  = twoNamed(dir, "Shared Thing")
+    val id = i.store.list().head.id
+    val r  = Run(dir)
+    assertEquals(r("get", id.value), ExitCode.Ok, r.stderr)
+  }
+
+  // ------------------------------------------------- sync × line endings
+  //
+  // `base` and `remote` are hashes of file BYTES, so `local` has to be measured
+  // the same way. Hashing the record's text with a fixed LF made every one of
+  // these read as a local edit that never happened.
+  //
+  // Note every OTHER sync test above uses text with no newline in it, where LF
+  // and CRLF are the same bytes — which is exactly how this survived.
+
+  private def crlf: String = "digraph G {\r\n  a -> b\r\n}\r\n"
+
+  tmp.test("a CRLF origin nobody has touched is InSync, not Ahead") { dir =>
+    dot(dir, "win.dot", crlf)
+    val i = Run(dir)
+    assertEquals(i("import", "win.dot", "--mode", "sync"), ExitCode.Ok, i.stderr)
+
+    // Nothing at all happens here. Both sides are exactly as imported.
+    val s = Run(dir)
+    assertEquals(s("sync", "--all"), ExitCode.Ok, s.stderr)
+    assert(s.stdout.contains("InSync"), s.stdout)
+    assertEquals(Files.readString(dir.resolve("win.dot")), crlf, "sync rewrote an untouched origin")
+  }
+
+  tmp.test("a byte-identical CRLF regeneration is Converged, not Diverged") { dir =>
+    val f = dot(dir, "win.dot", crlf)
+    val i = Run(dir)
+    i("import", "win.dot", "--mode", "sync")
+    val id = i.store.list().head.id
+
+    // The generator rewrites the same bytes; the store independently agrees.
+    val d = i.store.get(id).fold(x => fail(s"$x"), identity)
+    val next = "digraph G {\r\n  a -> c\r\n}\r\n"
+    i.store.save(d.copy(text = next))
+    Files.writeString(f, next)
+
+    val s = Run(dir)
+    assertEquals(s("sync", "--all"), ExitCode.Ok, s.stdout)
+    assert(s.stdout.contains("Converged"), s.stdout)
+  }
+
+  tmp.test("a CRLF origin that moves is Behind, and pull follows it") { dir =>
+    val f = dot(dir, "win.dot", crlf)
+    val i = Run(dir)
+    i("import", "win.dot", "--mode", "pull")
+
+    val next = "digraph G {\r\n  a -> b\r\n  b -> c\r\n}\r\n"
+    Files.writeString(f, next)
+
+    val s = Run(dir)
+    assertEquals(s("sync", "--all"), ExitCode.Ok, s.stderr)
+    assert(s.stdout.contains("Behind"), s.stdout)
+    assertEquals(s.store.list().head.text, next)
+  }
+
   // --------------------------------------------------------------- watch
 
   /** v1 had no way to observe changes without a window. This is the primitive a
@@ -476,6 +621,38 @@ class CliSpec extends FunSuite:
     assert(r.stderr.contains("denied root"), r.stderr)
   }
 
+  /** The audit log is the only place `source` is recorded, so WHEN an event
+    * happened has to come from the same clock as the record it describes.
+    * Audit stamped its own `System.currentTimeMillis()` instead — the one hole
+    * in a seam every other timestamp goes through.
+    */
+  tmp.test("an audit line is stamped from the injected clock, not the wall clock") { dir =>
+    dot(dir, "a.dot")
+    val r = Run(dir)
+    assertEquals(r("import", "a.dot"), ExitCode.Ok, r.stderr)
+
+    val line = r.env.audit.entries.headOption.getOrElse(fail("nothing was audited"))
+    val stamp = ujson.read(line).obj("timestampMs").num.toLong
+    // The fixture's clock starts at 1000 and ticks by one per read, so a real
+    // wall-clock stamp is thirteen digits and this assertion is unmissable.
+    assert(stamp > 1000L && stamp < 2000L, s"audit used a clock the test does not control: $stamp")
+  }
+
+  /** The guardrail has to hold on EVERY path-taking command, not most of them.
+    * `watch` took a ref that was not in the library, turned it straight into an
+    * origin, and started following it — the one way to point gx at a file the
+    * policy forbids.
+    */
+  tmp.test("watch refuses a denied path too, like every other command") { dir =>
+    val secret = Files.createDirectories(dir.resolve("secrets"))
+    Files.writeString(secret.resolve("a.dot"), "x")
+    val r   = Run(dir)
+    val env = r.env.copy(policy = AccessPolicy(Nil, List(secret)))
+    assertEquals(Cli.run(Vector("watch", "secrets/a.dot"), env), ExitCode.InvalidPathOrPolicy)
+    assert(r.stderr.contains("denied root"), r.stderr)
+    assert(!r.stderr.contains("watching "), "watch followed a denied path anyway")
+  }
+
   // ----------------------------------------------------------------- run
 
   /** The document tier, headless (D7.2). These are V-09's point restated for

gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/fs/Audit.scala

@@ -28,21 +28,31 @@ enum AuditEvent derives CanEqual:
   * Writes are best-effort by construction. Failing to record an action must
   * never fail the action: a full disk should not make the editor read-only.
   */
-final class Audit(path: Path):
+/** @param now the clock, injected for the same reason every other timestamp in
+  *            the CLI is: an event stamped from a hidden `currentTimeMillis`
+  *            cannot be pinned by a test, and this was the one hole left in an
+  *            otherwise complete seam.
+  */
+final class Audit(path: Path, now: () => Long = () => System.currentTimeMillis()):
   private val lock = Object()
 
   def record(event: AuditEvent): Unit =
     lock.synchronized:
       try
-        Option(path.getParent).foreach(Files.createDirectories(_))
-        val line = Audit.toJson(event) + "\n"
+        // Only a file we are about to CREATE needs its directory made and its
+        // mode set. Doing both per line cost two extra syscalls on every event,
+        // which `watch` pays once per change for as long as it runs — and
+        // testing existence still re-restricts a log deleted underneath us.
+        val existed = Files.isRegularFile(path)
+        if !existed then Option(path.getParent).foreach(Files.createDirectories(_))
+        val line = Audit.toJson(event, now()) + "\n"
         Files.write(
           path,
           line.getBytes(StandardCharsets.UTF_8),
           StandardOpenOption.CREATE,
           StandardOpenOption.APPEND
         )
-        Audit.restrictToOwner(path)
+        if !existed then Audit.restrictToOwner(path)
       catch case NonFatal(_) => () // never fail the operation being audited
 
   def entries: Vector[String] =
@@ -74,7 +84,7 @@ object Audit:
   private def obj(fields: (String, String)*): String =
     fields.map((k, v) => s""""$k":"${escape(v)}"""").mkString("{", ",", "}")
 
-  private[fs] def toJson(event: AuditEvent): String =
+  private[fs] def toJson(event: AuditEvent, timestampMs: Long): String =
     import AuditEvent.*
     val base = event match
       case Allowed(path, action)   => obj("event" -> "allowed", "path" -> path, "action" -> action)
@@ -93,4 +103,4 @@ object Audit:
       case WatchRemoved(uri)   => obj("event" -> "watch.removed", "uri" -> uri)
       case OriginMissing(path) => obj("event" -> "origin.missing", "path" -> path)
     // Timestamp is prepended rather than threaded through every case.
-    s"""{"timestampMs":${System.currentTimeMillis()},${base.drop(1)}"""
+    s"""{"timestampMs":$timestampMs,${base.drop(1)}"""

gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/rpc/ControlChannel.scala

@@ -20,11 +20,6 @@ enum ChannelError derives CanEqual:
   case Io(message: String)
   case Rpc(code: String, message: String, details: ujson.Obj)
 
-  def describe: String = this match
-    case NoDesktop(_)     => "no desktop is running"
-    case Io(message)      => message
-    case Rpc(_, message, _) => message
-
 /** The desktop's control channel: a unix socket carrying one JSON object per
   * line, request then response (D4).
   *

gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/store/LibraryStore.scala

@@ -110,9 +110,6 @@ final class LibraryStore(val root: Path) extends DiagramSink:
   def findByOrigin(origin: OriginUri): Vector[Diagram] =
     list().filter(_.binding.exists(_.origin == origin))
 
-  def inFolder(folder: FolderPath): Vector[Diagram] =
-    list().filter(_.folder == folder)
-
   // ------------------------------------------------------------ folders
 
   /** The tree, including folders that hold nothing.