From bb9442a07cebe70c3492cca31f31a8270bb83134 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Fri, 16 Oct 2026 16:52:45 +0200 Subject: fix(assistant): review of the Do row and fileops The canvas: a To .md ended any other way than its own Stop (the footer Stop, a question, a drop, a daemon restart, an older daemon refusing the command) left the card at Working forever. The footer Stop goes through stopOp(), asking and dropping end the operation first, a disconnect ends it, and an error while nothing else runs is taken as its outcome. Each fileops run gets its own Process, so a stopped run's late answer is never read as the next one's. To .md takes one PDF, as one file is written. Compress... no longer flips back when file(1) refines a kind, long errors are elided, and a desktop with no documents folder gets Documents/Assistant rather than ~/Assistant. fileops: an animated GIF to PNG or JPEG left one file per frame under names the rename never found; the first frame is taken. JPEG flattens transparency onto white. Zipping a folder holding the output folder read its own growing archive without end; only regular files outside it are added, which also skips FIFOs and dangling links. qpdf warnings no longer fail a merge. A folder's free name keeps its dots, a lone .gz or .xz is decompressed rather than refused, and a dotfile's archive is not hidden. Tools die with fileops even when it is killed outright, a killed run's leftovers are swept on the next run, a stop between fork and bookkeeping cannot miss the child, and a late stop cannot add a second answer. Error lines lose bsdtar's doubled prefix and its delayed-exit noise. Co-Authored-By: Claude Opus 5.5 --- desktop/assistant/Assistant.qml | 61 +++++++++++++++++-------- desktop/assistant/Card.qml | 11 ++++- desktop/assistant/Ops.js | 7 +-- desktop/assistant/fileops | 94 +++++++++++++++++++++++++++++++++------ desktop/assistant/test_fileops.py | 41 +++++++++++++++++ desktop/assistant/test_ops.js | 4 +- 6 files changed, 178 insertions(+), 40 deletions(-) (limited to 'desktop/assistant') diff --git a/desktop/assistant/Assistant.qml b/desktop/assistant/Assistant.qml index b1329b1..f76c38d 100644 --- a/desktop/assistant/Assistant.qml +++ b/desktop/assistant/Assistant.qml @@ -73,7 +73,11 @@ Scope { readonly property bool busy: root.state !== "idle" // A daemon gone mid-reply sends no idle; its hello on return redraws. - onConnectedChanged: if (!root.connected) { root.state = "idle"; root.runningTool = ""; } + onConnectedChanged: if (!root.connected) { + root.state = "idle"; + root.runningTool = ""; + if (root.opRunning && root.opDaemon) { root.opRunning = false; root.opError = "assistant gone"; } + } // One row per turn. tools is a comma-joined string: a ListModel role // holding an array becomes a nested model, which a Repeater can read but @@ -117,6 +121,7 @@ Scope { function attach(paths) { const files = paths.filter(p => p !== "").map(Files.file); if (!files.length) return; + if (root.opRunning) root.stopOp(); // a drop is a new task root.fresh(); root.clearOp(); root.staged = files; @@ -151,31 +156,42 @@ Scope { root.send({ type: "write", files: paths, prompt: root.mdPrompt, dir: root.outDir, name: name }); } else { root.opDaemon = false; - ops.command = [root.fileops, o.op, root.outDir].concat(paths); - ops.running = false; - ops.running = true; + // A Process per run: a stopped run's late answer must not be + // read as the next run's. + root.opProc = opRun.createObject(root, { command: [root.fileops, o.op, root.outDir].concat(paths) }); } } + property var opProc: null + function stopOp() { if (!root.opRunning) return; if (root.opDaemon) root.send({ type: "stop" }); - else ops.running = false; // SIGTERM: fileops kills its tool, removes the partial + else if (root.opProc) { + root.opProc.running = false; // fileops kills its tool, removes the partial + root.opProc = null; + } root.opRunning = false; root.opError = "stopped"; } - Process { - id: ops - stdout: StdioCollector { - onStreamFinished: { - if (!root.opRunning || root.opDaemon) return; - root.opRunning = false; - try { - const r = JSON.parse(text.trim().split("\n").pop()); - if (r.ok) root.opOut = r.out; else root.opError = r.error; - } catch (e) { - root.opError = "fileops gave no answer"; + Component { + id: opRun + Process { + id: proc + running: true + stdout: StdioCollector { + onStreamFinished: { + if (root.opProc !== proc) { proc.destroy(); return; } // stopped, or superseded + root.opProc = null; + root.opRunning = false; + try { + const r = JSON.parse(text.trim().split("\n").pop()); + if (r.ok) root.opOut = r.out; else root.opError = r.error; + } catch (e) { + root.opError = "fileops gave no answer"; + } + proc.destroy(); } } } @@ -185,7 +201,11 @@ Scope { running: true command: ["xdg-user-dir", "DOCUMENTS"] stdout: StdioCollector { - onStreamFinished: if (text.trim().startsWith("/")) root.outDir = text.trim() + "/Assistant" + // With no documents folder set, xdg-user-dir answers $HOME itself. + onStreamFinished: { + const d = text.trim(); + if (d.startsWith("/") && d !== Quickshell.env("HOME")) root.outDir = d + "/Assistant"; + } } } @@ -232,7 +252,8 @@ Scope { // error, and the chips must still be there to fix it. chatting keeps the // chat layout meanwhile, so the card does not shrink while a PDF is read. function ask(text) { - if (!root.connected || root.blocked || (text.trim() === "" && !root.staged.length)) return false; + if (!root.connected || root.blocked || root.opRunning + || (text.trim() === "" && !root.staged.length)) return false; root.notice = ""; root.chatting = true; root.send({ type: "ask", text: text, files: root.staged.map(f => f.path) }); @@ -324,7 +345,9 @@ Scope { if (root.opDaemon && root.opRunning) { root.opRunning = false; root.opOut = [e.path]; } break; case "error": - if (e.turn && root.opDaemon && root.opRunning) { + // A write's outcome. An older daemon answers it with a plain + // error and no turn; while nothing else runs, that is the write's. + if (root.opDaemon && root.opRunning && (e.turn || root.state === "idle")) { root.opRunning = false; root.opError = e.message; break; diff --git a/desktop/assistant/Card.qml b/desktop/assistant/Card.qml index 069d3a0..f4dc71d 100644 --- a/desktop/assistant/Card.qml +++ b/desktop/assistant/Card.qml @@ -310,7 +310,10 @@ Rectangle { // Do: local operations (Ops.js, run by fileops or the daemon's write). // Compress… swaps the group for its formats. Hidden while one runs. property bool compressing: false - Connections { target: card.canvas; function onStagedChanged() { card.compressing = false; } } + // Only a different set of files resets it: refine() re-assigns staged + // when file(1) answers, which is not a change of files. + readonly property string stagedPaths: card.canvas.staged.map(f => f.path).join("\n") + onStagedPathsChanged: card.compressing = false Group { id: dos @@ -341,6 +344,9 @@ Rectangle { Text { anchors.verticalCenter: parent.verticalCenter + // Bounded: an error can run to 300 characters. + width: Math.min(implicitWidth, card.width - 28 - status.spacing * 3 - 220) + elide: Text.ElideRight text: status.c.opRunning ? (status.c.opDaemon && status.c.opChars ? `${status.c.opLabel}… ${(status.c.opChars / 1000).toFixed(1)}k characters` @@ -501,7 +507,8 @@ Rectangle { danger: card.canvas.busy enabled: card.canvas.connected && (card.canvas.busy || !card.canvas.blocked) onClicked: { - if (card.canvas.busy) card.canvas.send({ type: "stop" }); + if (card.canvas.opRunning) card.canvas.stopOp(); + else if (card.canvas.busy) card.canvas.send({ type: "stop" }); else if (card.canvas.ask(input.text)) input.text = ""; input.forceActiveFocus(); } diff --git a/desktop/assistant/Ops.js b/desktop/assistant/Ops.js index 2761a2d..4d7ef96 100644 --- a/desktop/assistant/Ops.js +++ b/desktop/assistant/Ops.js @@ -14,11 +14,12 @@ // The file state's Do row. op is a fileops operation, or "md" for the // daemon's write, or "compress", which opens the COMPRESS formats. kinds // lists the staged kinds it fits ("any" fits every mix); min is the number -// of files it needs; format is the suffix a file already has when the +// of files it needs and max the most it takes (To .md writes one file from +// one PDF); format is the suffix a file already has when the // operation would not change it. icon is a Nerd Font codepoint, checked in // Inconsolata Nerd Font's cmap. const OPS = [ - { label: "To .md", icon: "\uf48a", op: "md", kinds: ["pdf"] }, + { label: "To .md", icon: "\uf48a", op: "md", kinds: ["pdf"], max: 1 }, { label: "To images", icon: "\uf1c5", op: "pdf-images", kinds: ["pdf"] }, { label: "Merge", icon: "\uf419", op: "pdf-merge", kinds: ["pdf"], min: 2 }, { label: "Extract audio", icon: "\uf001", op: "audio", kinds: ["video"] }, @@ -49,6 +50,6 @@ function ext(name) { function offered(files) { if (!files.length) return []; return OPS.filter(o => (o.kinds.includes("any") || files.every(f => o.kinds.includes(f.kind))) - && files.length >= (o.min ?? 1) + && files.length >= (o.min ?? 1) && files.length <= (o.max ?? Infinity) && !(o.format && files.every(f => o.format.includes(ext(f.name))))); } diff --git a/desktop/assistant/fileops b/desktop/assistant/fileops index 4bfaee0..78a624b 100755 --- a/desktop/assistant/fileops +++ b/desktop/assistant/fileops @@ -18,6 +18,7 @@ the first free name; SIGTERM kills the running tool and removes what was half written. """ +import ctypes import json import os import shutil @@ -39,16 +40,19 @@ class Fail(Exception): def finish(result, code): + signal.signal(signal.SIGTERM, signal.SIG_IGN) # one answer: a late stop cannot add another print(json.dumps(result), flush=True) sys.exit(code) def stop(*_): + signal.signal(signal.SIGTERM, signal.SIG_IGN) if current is not None: try: os.killpg(current.pid, signal.SIGKILL) except ProcessLookupError: pass + current.wait() # gone before its folder is removed for t in temps: shutil.rmtree(t, ignore_errors=True) if os.path.isdir(t) else _unlink(t) finish({"ok": False, "error": "stopped"}, 1) @@ -61,22 +65,46 @@ def _unlink(path): pass +def die_with_us(): + # In the child: SIGKILL when fileops dies, however it dies. A stop is a + # SIGTERM that fileops handles; a shell reload can be a SIGKILL it + # cannot, and the tool, in its own session, would run on. + ctypes.CDLL("libc.so.6").prctl(1, signal.SIGKILL) # PR_SET_PDEATHSIG + + def run(argv): # Own process group, so a stop takes ffmpeg and its children too. global current if shutil.which(argv[0]) is None: hint = ": link it into ~/bin" if argv[0] == "trackcrop" else "" raise Fail(f"{argv[0]} not found{hint}") - current = subprocess.Popen(argv, stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, - stderr=subprocess.PIPE, text=True, start_new_session=True) + # A stop between the fork and current being set would miss the child. + signal.pthread_sigmask(signal.SIG_BLOCK, {signal.SIGTERM}) + try: + current = subprocess.Popen(argv, stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, + stderr=subprocess.PIPE, text=True, start_new_session=True, + preexec_fn=die_with_us) + finally: + signal.pthread_sigmask(signal.SIG_UNBLOCK, {signal.SIGTERM}) out, err = current.communicate() rc, current = current.returncode, None if rc: - last = err.strip().splitlines()[-1:] or [f"exit status {rc}"] - raise Fail(f"{argv[0]}: {last[0]}"[:300]) + raise Fail(f"{argv[0]}: {reason(argv[0], err, rc)}"[:300]) return out +def reason(tool, err, rc): + # The line that says what went wrong, without the tool's own prefix: + # bsdtar ends on "Error exit delayed from previous errors", the cause + # being earlier. + lines = [l.strip() for l in err.splitlines() if l.strip()] + lines = [l for l in lines if "Error exit delayed" not in l] or lines + line = lines[-1] if lines else f"exit status {rc}" + while line.startswith(tool + ":"): + line = line[len(tool) + 1:].strip() + return line + + def split(name): # ("report", ".pdf"), keeping a compound suffix whole: "a.tar.gz". low = name.lower() @@ -95,9 +123,10 @@ def stem(path): return os.path.splitext(name)[0] -def free(outdir, name): +def free(outdir, name, folder=False): + # A folder's name is whole: "v1.2 notes (pages)" has no suffix to keep. path = os.path.join(outdir, name) - base, ext = split(name) + base, ext = (name, "") if folder else split(name) n = 1 while os.path.lexists(path): path = os.path.join(outdir, f"{base} ({n}){ext}") @@ -108,12 +137,12 @@ def free(outdir, name): def make(outdir, name, build, folder=False): # build(tmp) writes the result at tmp; it keeps the suffix, which is how # ffmpeg and magick choose the format. - tmp = os.path.join(outdir, f".fileops-{os.getpid()}-{len(temps)}{split(name)[1]}") + tmp = os.path.join(outdir, f".fileops-{os.getpid()}-{len(temps)}{'' if folder else split(name)[1]}") temps.append(tmp) if folder: os.mkdir(tmp) build(tmp) - final = free(outdir, name) + final = free(outdir, name, folder) os.rename(tmp, final) temps.remove(tmp) return final @@ -160,17 +189,39 @@ def each(op, outdir, f): if op == "trackcrop": return make(outdir, f"{s} (tracked).mp4", lambda t: run(["trackcrop", f, t])) if op in ("png", "jpeg", "webp"): - return make(outdir, f"{s}.{'jpg' if op == 'jpeg' else op}", lambda t: run(["magick", f, t])) + # [0]: an animated GIF or WebP would come out as one file per frame, + # under names the rename never finds. JPEG has no transparency, so + # it is flattened onto white rather than left to turn black. + flat = ["-background", "white", "-flatten"] if op == "jpeg" else [] + src = f + "[0]" if op in ("png", "jpeg") else f + return make(outdir, f"{s}.{'jpg' if op == 'jpeg' else op}", + lambda t: run(["magick", src, *flat, t])) if op == "half": return make(outdir, f"{s} (50%){ext}", lambda t: run(["magick", f, "-resize", "50%", t])) if op == "extract": + # A lone compressed file (note.txt.gz) is no archive to bsdtar -x. + low = f.lower() + if low.endswith((".gz", ".xz", ".bz2", ".zst")) and not low.endswith( + (".tar.gz", ".tar.xz", ".tar.bz2", ".tar.zst")): + # Straight to the file: the content need not be text. + return make(outdir, s, lambda t: run(["sh", "-c", 'exec bsdcat -- "$1" > "$2"', + "sh", f, t])) return make(outdir, s, lambda t: run(["bsdtar", "-xf", f, "-C", t]), folder=True) raise Fail(f"unknown operation {op}") def compress(op, outdir, files): ext = {"zip": ".zip", "tgz": ".tar.gz", "txz": ".tar.xz", "tbz2": ".tar.bz2"}[op] - name = (stem(files[0]) if len(files) == 1 else "archive") + ext + # A dotfile's archive is not hidden: .bashrc gives bashrc.zip. + name = ((stem(files[0]).lstrip(".") if len(files) == 1 else "") or "archive") + ext + out = os.path.realpath(outdir) + + def wanted(p): + # Regular files only, and none inside the output folder: zipping a + # folder that holds it would read the archive being written, which + # never ends. A FIFO would block, a dangling link would fail. + real = os.path.realpath(p) + return os.path.isfile(p) and not real.startswith(out + os.sep) def build(t): if op == "zip": @@ -180,16 +231,28 @@ def compress(op, outdir, files): for root, _, names in os.walk(f): for n in names: p = os.path.join(root, n) - z.write(p, os.path.relpath(p, os.path.dirname(f))) + if wanted(p): + z.write(p, os.path.relpath(p, os.path.dirname(f))) else: z.write(f, os.path.basename(f)) else: with tarfile.open(t, "w:" + {"tgz": "gz", "txz": "xz", "tbz2": "bz2"}[op]) as tar: for f in files: - tar.add(f, arcname=os.path.basename(f)) + tar.add(f, arcname=os.path.basename(f), + filter=lambda i: None if i.isfifo() or i.isdev() else i) return make(outdir, name, build) +def sweep(outdir): + # Leftovers of a run that was killed outright (its pid is gone). + for n in os.listdir(outdir): + if n.startswith(".fileops-"): + pid = n.split("-")[1] + if pid.isdigit() and not os.path.exists(f"/proc/{pid}"): + p = os.path.join(outdir, n) + shutil.rmtree(p, ignore_errors=True) if os.path.isdir(p) else _unlink(p) + + def main(argv): if len(argv) < 3: raise Fail("usage: fileops ") @@ -197,14 +260,17 @@ def main(argv): if not os.path.isabs(outdir): raise Fail("the output folder must be an absolute path") for f in files: - if not os.path.exists(f): + if not (os.path.isabs(f) and os.path.exists(f)): raise Fail(f"cannot read {os.path.basename(f)}") os.makedirs(outdir, exist_ok=True) + sweep(outdir) if op == "pdf-merge": if len(files) < 2: raise Fail("merging needs two or more PDFs") + # --warning-exit-0: a PDF with a repaired xref merges fine, and qpdf + # would otherwise exit 3 and the result be thrown away. return [make(outdir, "merged.pdf", - lambda t: run(["qpdf", "--empty", "--pages", *files, "--", t]))] + lambda t: run(["qpdf", "--warning-exit-0", "--empty", "--pages", *files, "--", t]))] if op in ("zip", "tgz", "txz", "tbz2"): return [compress(op, outdir, files)] return [each(op, outdir, f) for f in files] diff --git a/desktop/assistant/test_fileops.py b/desktop/assistant/test_fileops.py index 850d1d7..8fd8449 100755 --- a/desktop/assistant/test_fileops.py +++ b/desktop/assistant/test_fileops.py @@ -122,6 +122,47 @@ with tempfile.TemporaryDirectory() as src, tempfile.TemporaryDirectory() as out: r, _ = fileops("audio", out, g) assert not r["ok"] and "audio" in r["error"], r # a GIF has no audio track + # From review: each of these failed or misbehaved once. + anim = os.path.join(src, "anim.gif") + sh("magick", "-size", "8x8", "xc:red", "xc:blue", "-loop", "0", anim) + [ap] = one("png", anim) # first frame, no stray frame files + assert mime(ap) == "image/png" and not [n for n in os.listdir(out) if n.startswith(".")] + clear = os.path.join(src, "clear.png") + sh("magick", "-size", "8x8", "xc:none", clear) + [cj] = one("jpeg", clear) # transparency flattened onto white + assert subprocess.run(["magick", cj, "-format", "%[fx:int(255*p{0,0}.r)]", "info:"], + capture_output=True, text=True).stdout == "255" + dotted = os.path.join(src, "v1.2 notes.pdf") + sh("magick", "-size", "50x50", "xc:white", dotted) + names = [os.path.basename(one("pdf-images", dotted)[0]) for _ in range(2)] + assert names == ["v1.2 notes (pages)", "v1.2 notes (pages) (1)"], names + gz = os.path.join(src, "note.txt.gz") + sh("sh", "-c", f"gzip -c {os.path.join(src, 'note.txt')} > '{gz}'") + [gx] = one("extract", gz) # a single compressed file + assert os.path.basename(gx) == "note.txt" and open(gx).read() == "hello\n" + pgz = os.path.join(src, "pic.png.gz") + sh("sh", "-c", f"gzip -c '{png}' > '{pgz}'") + [px] = one("extract", pgz) # binary content survives + assert open(px, "rb").read() == open(png, "rb").read() + with open(os.path.join(src, ".bashrc"), "w") as fh: + fh.write("x\n") + [dz] = one("zip", os.path.join(src, ".bashrc")) + assert os.path.basename(dz) == "bashrc.zip" + with tempfile.TemporaryDirectory() as tree: # the output folder inside the input + inner = os.path.join(tree, "Assistant") + with open(os.path.join(tree, "a.txt"), "w") as fh: + fh.write("a\n") + os.mkfifo(os.path.join(tree, "pipe")) + os.symlink("/nonexistent", os.path.join(tree, "dangling")) + r, rc = fileops("zip", inner, tree) + assert r["ok"], r + import zipfile + assert sorted(zipfile.ZipFile(r["out"][0]).namelist()) == [os.path.basename(tree) + "/a.txt"] + stale = os.path.join(out, ".fileops-999999-0.png") + open(stale, "w").close() + one("png", png) # a dead run's leftovers are swept + assert not os.path.exists(stale) + # SIGTERM mid-encode (the canvas's Stop): no ffmpeg left, no partial file. long_clip = os.path.join(src, "long.mp4") sh("ffmpeg", "-v", "error", "-f", "lavfi", "-i", "testsrc=duration=20:size=1280x720:rate=30", diff --git a/desktop/assistant/test_ops.js b/desktop/assistant/test_ops.js index 6d53376..c016241 100644 --- a/desktop/assistant/test_ops.js +++ b/desktop/assistant/test_ops.js @@ -24,8 +24,8 @@ const f = (name, kind) => ({ path: "/x/" + name, name, kind }); const labels = files => O.offered(files).map(o => o.label); assert.deepStrictEqual(labels([f("a.pdf", "pdf")]), ["To .md", "To images", "Compress…"]); -assert.deepStrictEqual(labels([f("a.pdf", "pdf"), f("b.pdf", "pdf")]), - ["To .md", "To images", "Merge", "Compress…"]); +assert.deepStrictEqual(labels([f("a.pdf", "pdf"), f("b.pdf", "pdf")]), // To .md: one PDF only + ["To images", "Merge", "Compress…"]); assert.deepStrictEqual(labels([f("a.pdf", "pdf"), f("p.png", "image")]), ["Compress…"]); // An image's own format is not offered. assert.deepStrictEqual(labels([f("p.png", "image")]), ["To JPEG", "To WebP", "Resize 50%", "Compress…"]); -- cgit v1.2.3