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 +- .../specs/2026-10-16-file-ops-design.md | 4 +- 7 files changed, 180 insertions(+), 42 deletions(-) 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…"]); diff --git a/docs/superpowers/specs/2026-10-16-file-ops-design.md b/docs/superpowers/specs/2026-10-16-file-ops-design.md index 15f12ad..5676d54 100644 --- a/docs/superpowers/specs/2026-10-16-file-ops-design.md +++ b/docs/superpowers/specs/2026-10-16-file-ops-design.md @@ -19,7 +19,7 @@ is overwritten. | kind | label | op | does | | --- | --- | --- | --- | -| pdf | To .md | (daemon `write`) | the model rewrites it as Markdown into `name.md` | +| pdf, 1 only | To .md | (daemon `write`) | the model rewrites it as Markdown into `name.md`; one PDF, one file | | pdf | To images | `pdf-images` | `pdftoppm -r 150 -png` into a folder `name (pages)` | | pdf, 2+ | Merge | `pdf-merge` | `qpdf --empty --pages ... -- merged.pdf`, in staging order | | video | Extract audio | `audio` | the track copied as it is when it is AAC (`.m4a`), MP3 or Opus (`ffprobe` tells), anything else encoded to `.mp3` at `-q:a 2` | @@ -34,7 +34,7 @@ is overwritten. Matching follows the presets: an operation is offered when every staged file is of one of its kinds. Merge needs two or more files, all PDF. -Compress fits any mix. Several staged files of one kind run the operation +Compress fits any mix. To .md is offered for exactly one PDF. Several staged files of one kind run the operation on each (four images, To PNG, four PNGs), except Merge and Compress, which make one result. -- cgit v1.2.3