diff --git a/.github/ISSUE_TEMPLATE/idea-ticket.md b/.github/ISSUE_TEMPLATE/idea-ticket.md index 5a10f113..8fd93da4 100644 --- a/.github/ISSUE_TEMPLATE/idea-ticket.md +++ b/.github/ISSUE_TEMPLATE/idea-ticket.md @@ -1,8 +1,8 @@ --- name: MCP agent-loop idea -about: An idea for a new capability or change, for the researcher agent to assess (see CLAUDE.md in the iterate repo) — not for bug reports, use "Bug report" for those. +about: An idea or feature request for the feature lead agent to design (see CLAUDE.md in dkackman/harnest) — not for bug reports, use "Bug report" for those. title: "" -labels: ["idea", "owner:researcher"] +labels: ["idea", "owner:lead"] --- **idea:** what capability or change this is proposing @@ -12,10 +12,19 @@ labels: ["idea", "owner:researcher"] **links:** any prior art, reference docs, upstream libraries, or related issues diff --git a/.github/codeql/dw-security/DwPathSanitizers.qll b/.github/codeql/dw-security/DwPathSanitizers.qll index c15df645..a2891aec 100644 --- a/.github/codeql/dw-security/DwPathSanitizers.qll +++ b/.github/codeql/dw-security/DwPathSanitizers.qll @@ -37,14 +37,14 @@ private import semmle.python.dataflow.new.DataFlow private import semmle.python.security.dataflow.PathInjectionCustomizations /** - * Holds if `name` is a `dw.security` function that returns a path confined + * Holds if `name` is a `dw.security` (or `dw.locations`) function that returns a path confined * to a base directory it was given. */ private predicate pathValidatorName(string name) { name = [ "validate_path", "validate_workflow_path", "validate_output_path", - "validate_prompt_path", "safe_join_path" + "validate_prompt_path", "safe_join_path", "validate_media_path" ] } diff --git a/CLAUDE.md b/CLAUDE.md index a8b7efef..a95e104a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -153,7 +153,9 @@ references) are documented above in *Workflow sources* and *Type System*. `"output:ltx2/Gyre/latest/still.png"`. The name is `//` under the output root, and `latest` in the run-id position picks the newest run that holds the file (run ids sort by their UTC timestamp; a failed or fully-cached run holds - only a manifest and is skipped). Resolved in `realize_args` beside `asset:` (`dw/runs.py`), + only a manifest and is skipped), and `v` there picks the run whose version is N + (below) - exactly that run, with no fallback to an older one. Either is a selector only + where run directories are, and the realized workflow pins both to the run id. Resolved in `realize_args` beside `asset:` (`dw/runs.py`), against the output root `Workflow.run` activates, and confined to it - A generated file becomes a stable input with `POST /api/assets/keep` (gallery "Keep as asset", MCP `keep_output`): it is hard-linked, else copied, from the workspace's outputs @@ -368,6 +370,45 @@ same reason - default setup cannot load a pack. `JobManager.realized` finds the file. `exports` is a reserved workspace name: `POST /api/jobs/{id}/export` gathers one finished job into `/exports//` and `GET /exports/.zip` streams it. +- **A run has a number, and it is not derived from the listing** - a file's name + is per *step*, so four runs of one workflow write four files called + `AcornWarsCutAndScore-film.7-0.0.mp4` and the gallery drew four identical + captions: the run id told them apart but is not something anyone says out + loud, so an agent had no way to name one of them to a person. Every run now + takes an ordinal, `assign_run_version` (`dw/runs.py`) at the moment + `Workflow.run` opens the run directory, recorded as `version` in + `manifest.json` and read back by `run_versions`. Assigned once and never + recomputed, which is the point: deleting a middle run leaves a gap rather + than sliding every later number down, so "version 5" still means the same + run tomorrow. Assignment is `max(recorded) + 1` over *every* sibling + manifest, not one past the newest - run ids are chronological only to the + second, and within one second the spec digest decides the sort, which is + exactly what three quick reruns hit. The number is on disk from the moment + the run opens - a `status: "running"` manifest is written before the first + step and rewritten in full at the end - so a hard kill does not lose it and + a second process opening a run of the same workflow sees it. A run with no + recorded number (made before the field, or killed before even that first + manifest) is ranked: the unrecorded runs older than every recorded one take + the numbers beneath the lowest, later ones continue from the highest before + them. A ranked number would move when an older sibling is deleted, so + `record_run_versions` writes it into the manifest on the two write paths - + a run opening and a run directory being deleted; the listing never writes, + and a run with no manifest at all is left ranked. A gap in the numbers is + not only a deletion: a failed run or a fully cached rerun takes a number + and may have no media for the gallery to show under it. `GET + /api/gallery` and the metadata route carry `version` and `run_id` + (`run_versions` read once per identity per listing, not per file), and + `?folder=&version=` lists one run's files. The number is also a name: + `output:/v4/`. The `run_start` event carries it, the job + records it (`run_version`, a `jobs.sqlite` column) and the export README and + zip download name (`-v4-.zip`) carry it too. MCP + `list_gallery` teaches the vocabulary and takes `folder`/`version`, and the + web UI reads the field only - a `v4` chip on the gallery card, the jobs list + and the job page, the run id in the gallery's detail pane. Nothing on disk is + renamed, so `output:` references, the step cache and `keep_output` are + untouched. Two limits taken deliberately: deleting the *newest* run frees + its number for reuse (the high-water mark lived in the manifest that went + with it), and the flat layout has no runs, so `version` is null there. - **Result subfolders**: a step's `result.subfolder` (`dw/subfolders.py`) puts its files in a subfolder of the run directory - `/final/x.mp4` - by convention `final` or `intermediate`; the engine treats no name specially and there is no default. @@ -552,9 +593,45 @@ same reason - default setup cannot load a pack. name into the JSON. An entry violation is reported at `arguments.shots[0].num_frames`, and the rule is reported beside the field in the catalog's `lists` block as well as in `constraints` +- **A joined video records its shots, measured** — `concat_videos`, + `dissolve_videos` and both `run_chain` returns set `AudioVideo.shots` + (`dw/shots.py`): one `{name, start_frame, num_frames, start_sample, + num_samples}` per input. The frames are partitioned, and the samples are + read off the waveform the join built, never derived from the frames, so a + shot's overrun stays visible (#385). Every other `AudioVideo` constructor + carries, rescales (`interpolate_frames`), re-measures (`pair_audio`) or + drops them, and `tests/test_shots.py` enumerates the constructor sites with + `ast`, so a new one fails until someone decides for it. `Result.save` + keeps them as plain data in `saved_shots` (path -> shots), which survives + the step cache's stripped copy. The manifest entry and `step_end` carry + `shots`, renamed `shot@` from the step's `videos` references (as + `selected_field` does). `recorded_shots` (`dw/runs.py`) reads them back for + `get_gallery_metadata`'s `media.shots` and for `get_output_frames(seams=true)` + without `boundaries`. The mp4 itself carries nothing yet - **Step cache**: a process-wide singleton (`dw/step_cache.py`) consulted by every `Workflow.run`, including server jobs; entries are keyed by `(workflow id, step name)` and validated against the output *root*, never the per-run directory - a run directory is new every execution and would defeat the cache; disabled entirely when the workflow sets no `seed`; a hit reports the earlier run's files with `reused: true` and writes nothing new; `memory clear` drops it. This is why "Run again" on a seeded workflow finishes instantly and generates nothing - the job page says so when every step was reused, and `POST /api/jobs/{id}/rerun` with `{"new_seed": true}` (MCP `rerun_job(new_seed=True)`) draws a fresh seed into the workflow's seed variable, which is the way to get a different image +- **Assessment probes measure a finished file and say where to look, and + decide nothing** (`dw/tasks/assess.py`, #387) - `analyze_shots`, + `analyze_seams` and `analyze_sync_drift` each read a video streaming + (thumbnails only, never the full frame list, so a long cut is cheap) and + answer a JSON dict of measurements plus `findings`, the ones that crossed a + threshold in `dw/assessment_rules.py`'s table; nothing in the engine acts + on a finding. These are the `returns: "json"` task kind, listed separately + in `list_tasks`' `assessment` (probes stay in `commands` too), and a step + on one must save `"result": {"content_type": "application/json"}` - + anything else fails validation. Shot boundaries resolve in order: the step's `shots` argument, + the video's own carried shots, the run manifest beside the file, else the + whole file as one shot (`shots_source` says which). A shot's `hard_cut: + true` field suppresses the `seam_frame_jump` rule at the seam it opens - a + cut meant as a cut. `tests/test_assessment_rules.py` pins the rules table + to real probe fields, so a rename cannot leave a rule reading nothing. `GET /api/gallery/{name}/assess` (MCP `assess_output`, #388, + `dw/server/assess.py`) runs the applicable probes in the server process on + one decode - a sync route, so it answers beside a GPU job rather than + queueing - with the shots the run's manifest (or an asset's keep sidecar) + recorded, merging `findings`/`rules_applied`/`rules_skipped` and naming + each inapplicable probe in `not_applicable`; `probe=` returns one probe's + full body and is whitelisted before the name is read. ## JSON Workflow Structure diff --git a/README.md b/README.md index ffcb9491..e8e2ae23 100644 --- a/README.md +++ b/README.md @@ -83,7 +83,7 @@ once; without it the run fails partway through with a 401/403 from the Hub. ## Drive it from an agent -Then just ask. The agent has 58 tools covering the whole surface — the +Then just ask. The agent has 59 tools covering the whole surface — the workflow catalog, the real diffusers pipeline signatures, the job queue, the gallery, the model cache: @@ -122,6 +122,23 @@ and outputs — so two agents, or an agent and you in the browser, share the GPU without saving over each other. An agent calls `use_workspace` once and the rest of the session lands there. +**Feedback from a session.** At the end of a working session, ask the agent +what got in its way: bugs, gaps, misleading skill text, tools it reached for +and couldn't find. Have it file each one as an issue on +`dkackman/diffusers-workflow` with the `field-report` label, for example: + +> File each bug or gap you hit as an issue on dkackman/diffusers-workflow +> with the label `field-report`. + +The label marks a report as coming from real use, not from the automated test +loop. The agent loop (see [Agent Loop](docs/AGENT_LOOP.md)) picks the report up +like any other issue when you filed it yourself; a report filed under any +other GitHub login is parked for the maintainer to review first (relabelled +`owner:don` + `status:needs-approval`), since the loop must not act +unattended on third-party text in a public repo, and is only handed to the +loop, or not, after that review. The label is also what feature planning +reads as evidence of demand. + The complete tool reference, client configuration for other MCP hosts, and the troubleshooting table: [MCP Server](docs/MCP.md). Workspaces in depth: [Workspaces](docs/WORKSPACES.md). diff --git a/docs/AGENT_LOOP.md b/docs/AGENT_LOOP.md index 600ece0b..a4583e0f 100644 --- a/docs/AGENT_LOOP.md +++ b/docs/AGENT_LOOP.md @@ -28,6 +28,15 @@ of scripted MCP calls (`regression-suite-*.md` in the `iterate` repo) against the live server and files/comments on Issues for anything that regresses. It doesn't participate in the implementer/tester handoff. +## Outside filings + +The loop runs as one GitHub login (the repo owner's) and must not act +unattended on text filed by anyone else in this public repo. An issue filed +under a different login — including a `field-report` from someone else's +session — is relabelled `owner:don` + `status:needs-approval` before either +role works it, and is handed to the loop, or not, only after the maintainer +reviews it. + ## Reading a ticket Tickets use the **MCP agent-loop ticket** issue template. Two label diff --git a/docs/MCP.md b/docs/MCP.md index a9ee2f1f..f0af5c48 100644 --- a/docs/MCP.md +++ b/docs/MCP.md @@ -187,7 +187,7 @@ Nothing in this sequence costs GPU time. ## Tool reference -58 tools in six groups. Names and arguments below are transcribed from +59 tools in six groups. Names and arguments below are transcribed from `dw_mcp/server.py` — nothing here is renamed or reshaped for the docs. ### Catalog (read-only) @@ -226,8 +226,8 @@ when no single workflow covers it. | `get_health()` | — | Check that the server is alive, and which machine answered: `version`, `device`, whether a model process is currently resident (`worker_alive`), the job running now and the queue depth. `worker_alive: false` is the normal idle state on a server that has not run a job since startup or the last memory clear - not a fault - the on-demand worker starts with the next job (#206) | | `get_server_info()` | — | What this installation can do and where it keeps things: `device` (the accelerator a run will use), `version`, the `workspace` this session is working in and the workflow/asset/output/prompt `directories` of *that* workspace, the bind address and port, whether a token is required, and whether MCP is mounted. Check the device before authoring - a CUDA-only choice (bitsandbytes, `torch.compile`, flash attention) is not available on an `mps` or `cpu` server. `runtime` (#222) reports the Python version, torch version and the CUDA version torch was built against, the NVIDIA driver version (when `nvidia-smi` is reachable), and the installed versions of diffusers, transformers, accelerate, bitsandbytes, peft, safetensors and sentencepiece (`null` for one not installed) - for diagnosing an environment mismatch between boxes without shelling in | | `list_jobs(limit=20, status=None, workspace=None)` | optional `limit` (newest N), `status` (one state or a comma-separated set of `queued`, `running`, `succeeded`, `failed`, `cancelled`), `workspace` | List queued, running and recent jobs, **newest first**. Bounded by default: the unbounded listing was over a client's tool-result limit on a server with a few months of history, which made it a tool that could not be called at all. `total` says how many matched and `truncated`/`next` say so when the answer was cut - raise `limit` or narrow with `status`. Without `workspace`, a named workspace lists its own jobs and the default one lists every job the server holds | -| `list_gallery(limit=50, subfolder=None, only_orphans=False, workspace=None)` | `limit`, `subfolder`, `only_orphans`, `workspace` | List generated output files, newest first. A name is `//`, where `` may sit in the subfolder the step chose (`final/episode.mp4`); each entry carries `folder` (the workflow) and `subfolder` (by convention `final` or `intermediate`, `''` when the step chose none, any path the workflow wrote otherwise), and `subfolder="final"` lists only deliverables. Each entry also carries a ready-made `url`, already scoped to the workspace that made it - a hand-built `/outputs/` URL 404s for anything but the default workspace. `only_orphans=True` inverts the call: instead of files, it returns run directories with no media anywhere under them (`runs`, each `{name, mtime}`) - a run whose output was deleted before `delete_output` could remove it by name, or one that failed before writing anything; `subfolder` does not apply in this mode, and `name` is exactly what `delete_output` accepts (#170). `workspace` names the workspace for this one call without switching the session to it - the same pin `run_workflow` takes, so a job run into another workspace stays reachable from the session that queued it | -| `get_gallery_metadata(name, envelope=False, workspace=None)` | `name`, `workspace` | Get the metadata embedded in a generated file — or, when `name` is an `asset:` reference, what an *input* asset holds (`source` says which; `job` is null for an asset). Reading an input's duration, frame count, fps and sample rate before a run is how a caller learns the `total_frames`, `fps` and `sample_rate` a workflow expects it to supply: the exact workflow and arguments that produced it, and, for audio/video, a `media` block (duration, rate, channels, fps, size, peak/mean dBFS). `envelope=true` adds `media.envelope` — `rms_dbfs` and `peak_dbfs` one entry per second — which is what locates something in a track rather than measuring the whole of it. `workspace` names the workspace for this one call without switching the session to it - the same pin `run_workflow` takes, so a job run into another workspace stays reachable from the session that queued it | +| `list_gallery(limit=50, subfolder=None, only_orphans=False, workspace=None, folder=None, version=None)` | `limit`, `subfolder`, `only_orphans`, `workspace`, `folder`, `version` | List generated output files, newest first. A name is `//`, where `` may sit in the subfolder the step chose (`final/episode.mp4`); each entry carries `folder` (the workflow) and `subfolder` (by convention `final` or `intermediate`, `''` when the step chose none, any path the workflow wrote otherwise), and `subfolder="final"` lists only deliverables. Each entry carries `run_id` and `version` - that run's ordinal among the workflow's runs, which is how one of several runs that wrote the same basename is named to a person: the web UI labels the same file `v5`. The number is assigned when the run opens and never renumbered, so deleting a run leaves a gap rather than sliding the rest down (a failed run, or a rerun that reused every step, leaves one too - it took a number and may have nothing to list), and it is `null` under the flat output layout, which has no runs. `folder` with `version` lists that one run's files, and `output:/v5/` names one in a workflow; every other tool takes `name`. Each entry also carries a ready-made `url`, already scoped to the workspace that made it - a hand-built `/outputs/` URL 404s for anything but the default workspace. `only_orphans=True` inverts the call: instead of files, it returns run directories with no media anywhere under them (`runs`, each `{name, mtime}`) - a run whose output was deleted before `delete_output` could remove it by name, or one that failed before writing anything; `subfolder` does not apply in this mode, and `name` is exactly what `delete_output` accepts (#170). `workspace` names the workspace for this one call without switching the session to it - the same pin `run_workflow` takes, so a job run into another workspace stays reachable from the session that queued it | +| `get_gallery_metadata(name, envelope=False, workspace=None)` | `name`, `workspace` | Get the metadata embedded in a generated file — or, when `name` is an `asset:` reference, what an *input* asset holds (`source` says which; `job` is null for an asset). Reading an input's duration, frame count, fps and sample rate before a run is how a caller learns the `total_frames`, `fps` and `sample_rate` a workflow expects it to supply: the exact workflow and arguments that produced it, and, for audio/video, a `media` block (duration, rate, channels, fps, size, peak/mean dBFS). Only an image (PNG/JPEG/WebP) embeds `metadata` this way — it is always null for audio and video, and `next` then points at `get_job_workflow(job_id)` when `job` is known, or says a kept asset carries no provenance at all when it isn't. `envelope=true` adds `media.envelope` — `rms_dbfs` and `peak_dbfs` one entry per second — which is what locates something in a track rather than measuring the whole of it. `media.shots` is set on an output joined from shots: one `{name, start_frame, num_frames, start_sample, num_samples}` per shot, as the join measured them (see the run manifest in docs/WORKFLOW_GUIDE.md), else null. `workspace` names the workspace for this one call without switching the session to it - the same pin `run_workflow` takes, so a job run into another workspace stays reachable from the session that queued it | ### Media @@ -235,9 +235,10 @@ when no single workflow covers it. | --- | --- | --- | | `get_output_image(name, max_dimension=768, workspace=None, crop=None)` | `name`, `max_dimension`, `workspace`, `crop` | Look at a generated image, downscaled to `max_dimension` on its longest side. Returns the image plus a text part reporting `original_size`, `returned_size` and `bytes`, so a downscale is never silent. `crop` is `[x, y, width, height]` in the original's pixels, cut before the downscale and reported back clamped - the way to see a region of a 2K still at 100%, where the whole would be shrunk past what a small element or a decode-tiling seam can be judged at. `workspace` names the workspace for this one call without switching the session to it - the same pin `run_workflow` takes, so a job run into another workspace stays reachable from the session that queued it | | `get_output_audio(name, start=None, duration=None, workspace=None)` | `name`, `start`, `duration`, `workspace` | Listen to a generated soundtrack as base64 - an audio output, or the track muxed into a video (#193) - in its own encoding when served whole, WAV when extracted or excerpted. No downscale exists for audio, so a whole clip over the 4MB budget is refused rather than cut (#204); ask for the part instead with `start` and `duration` in seconds, and the text part names what was cut (`excerpt: 2.0s from 10.0s of 240.0s`) so a slice is never mistaken for the whole. `get_gallery_metadata`'s envelope says where in a track to look. `workspace` names the workspace for this one call without switching the session to it | -| `get_output_frames(name, at=None, seams=None, count=None, boundaries=None, names=None, max_dimension=512, hear=None, workspace=None)` | `name`, `at`, `seams`, `count`, `boundaries`, `names`, `max_dimension`, `hear`, `workspace` | See a generated video as frames, since there is no video content type over MCP (#193). One selector per call: `count` for an evenly spaced contact sheet, `at` for moments (seconds or `"frame:N"`), `seams` (true, or seam numbers from 1) for the last frame before and first frame after each join side by side (each pair carries `difference`, the mean pixel change across the join, 0-255 - rank seams by it and look at the worst) - `boundaries` is each later shot's first frame - the running sum of the shots' `frame_count` from `get_gallery_metadata` on their own `intermediate/` files - and `names` names them. Tiles are fitted to `max_dimension` and, when the set would exceed the 4MB budget, shrunk together rather than dropped; the text part lists each tile and says so. `hear=N` also returns N seconds of soundtrack centred on each `at` moment, after its image - the way to check a hit point or lip-sync without reconciling two clocks; a mute clip keeps its frames and says `no soundtrack` | +| `get_output_frames(name, at=None, seams=None, count=None, boundaries=None, names=None, max_dimension=512, hear=None, workspace=None)` | `name`, `at`, `seams`, `count`, `boundaries`, `names`, `max_dimension`, `hear`, `workspace` | See a generated video as frames, since there is no video content type over MCP (#193). One selector per call: `count` for an evenly spaced contact sheet, `at` for moments (seconds or `"frame:N"`), `seams` (true, or seam numbers from 1) for the last frame before and first frame after each join side by side (each pair carries `difference`, the mean pixel change across the join, 0-255 - rank seams by it and look at the worst). On an output joined from shots (`concat_videos`, `dissolve_videos`, a chained step) `seams` alone is enough: the seams and their names are the `media.shots` its run recorded. For any other file pass `boundaries` - each later shot's first frame, the running sum of the shots' `frame_count` from `get_gallery_metadata` on their own `intermediate/` files - and `names` to name them; either one given overrides the recorded value. Tiles are fitted to `max_dimension` and, when the set would exceed the 4MB budget, shrunk together rather than dropped; the text part lists each tile and says so. `hear=N` also returns N seconds of soundtrack centred on each `at` moment, after its image - the way to check a hit point or lip-sync without reconciling two clocks; a mute clip keeps its frames and says `no soundtrack` | | `get_output_text(name, max_characters=20000, workspace=None)` | `name`, `max_characters`, `workspace` | Read a text output — a prompt enhancement, or any step whose result is `text/plain` or JSON. Reports the file's real length and whether it was truncated. `workspace` names the workspace for this one call without switching the session to it - the same pin `run_workflow` takes, so a job run into another workspace stays reachable from the session that queued it | -| `download_output(name, destination=None, overwrite=False, workspace=None)` | `name`, `destination`, `overwrite`, `workspace` | Save one output file to local disk, of any content type. `destination` may be a full path, a directory, or omitted to save under the output's own name in the current working directory; `~` expands and missing parent directories are created. `overwrite=True` is required to replace a file already at the resolved path. Over a `dw.serve --mcp` endpoint the file lands on the server, so the destination is confined to that workspace and a relative one is joined onto it. Returns nothing to the conversation but where the file landed — unlike the other media tools, the point is a file on disk, not a payload in context. Writes on the machine running the MCP server - over `dw.serve --mcp` that is the GPU box. A write that fails there (a path that exists only on the client, for instance) comes back as an error naming the server-side write and the client-side alternatives, not as an anonymous tool failure. `workspace` names the workspace for this one call without switching the session to it - the same pin `run_workflow` takes, so a job run into another workspace stays reachable from the session that queued it | +| `assess_output(name, probe=None, detail=False, workspace=None)` | `name` (a gallery name or `asset:`), `probe`, `detail`, `workspace` | Measure a finished cut on the server, without queueing: the assessment probes (`analyze_shots`, `analyze_seams`, `analyze_sync_drift`) run in the server process over one decode of the file, beside whatever job holds the GPU. Shot boundaries are the ones its run recorded (the manifest for an output, the `keep_output` sidecar for an asset). The answer merges every applicable probe's `findings` (`{rule, severity, at, value, threshold, says}`) with `rules_applied`, `rules_skipped` and `not_applicable` (`{probe: why}` - a still, a mute file, a file with no recorded shots); `detail=true` adds each probe's full body under `probes`, and `probe=` returns that one probe's full body. `probe` is checked against the three names before anything else is read, and an unknown one is refused naming them. Findings are places to look, not verdicts: drill in with `get_output_frames(seams=[n])` / `get_output_audio(start, duration)`. See *Assessing a run's output* in docs/WORKFLOW_GUIDE.md | +| `download_output(name, destination=None, overwrite=False, workspace=None)` | `name`, `destination`, `overwrite`, `workspace` | Save one output file to local disk, of any content type. `destination` may be a full path or a directory; `~` expands and missing parent directories are created. `overwrite=True` is required to replace a file already at the resolved path. Over the stdio `dw-mcp`, omitting `destination` saves under the output's own name in the current working directory. Over a `dw.serve --mcp` endpoint the file lands on the server confined to that workspace (a relative path is joined onto it), and `destination` is required there - an omitted one is refused rather than dropped loose in the workspace root, where nothing can find or delete it later (#353); use the `url` `list_gallery` reports, `get_output_image`/`get_output_audio`/`get_output_frames` for inline content, or `keep_output` to make it a named asset instead. Returns nothing to the conversation but where the file landed — unlike the other media tools, the point is a file on disk, not a payload in context. Writes on the machine running the MCP server - over `dw.serve --mcp` that is the GPU box. A write that fails there (a path that exists only on the client, for instance) comes back as an error naming the server-side write and the client-side alternatives, not as an anonymous tool failure. `workspace` names the workspace for this one call without switching the session to it - the same pin `run_workflow` takes, so a job run into another workspace stays reachable from the session that queued it | | `delete_output(name=None, workspace=None, job_id=None)` | exactly one of `name` / `job_id`, `workspace` | Permanently remove one generated file from the output directory, or - with `name` a `/` run directory, or with `job_id` - a whole run. By `job_id` the run directory is read from the job record (`run_dir`) and the reply adds `job_id` and the resolved `run_dir` to the usual `name` / `deleted` / `run_swept`; a job that never wrote a run directory, or an unknown one, is an error. `workspace` names the workspace for this one call without switching the session to it - the same pin `run_workflow` takes, so a job run into another workspace stays reachable from the session that queued it; a `job_id` delete with no `workspace` goes to the workspace the job ran in | ### Authoring, assets and workspaces @@ -304,9 +305,9 @@ references written in the same session. | `run_workflow(workflow_path=None, inline_workflow=None, arguments=None, acknowledged_cost=False, workspace=None, wait_seconds=0)` | exactly one of `workflow_path` (a catalog name from `list_workflows`, with or without `.json`, or a path to a workflow file on the server) or `inline_workflow`, optional `arguments`, `acknowledged_cost`, `workspace`, `wait_seconds` | Queue a workflow for generation. Returns as soon as the job is queued - unless `wait_seconds` is above 0, in which case the call then waits on the queued job exactly as `wait_for_job(job_id, timeout_seconds=wait_seconds)` would (same 55 s cap per call, clamped not honoured) and the result carries the queued-job fields plus that wait's (`status`, `still_running`, `waited_seconds`, `timeout_requested_seconds`, `timeout_applied_seconds`, `timeout_capped`, the slim `job`); when the cap covers the job's runtime one call is the run and the wait, and a `still_running: true` result is followed with `wait_for_job` as before. `workspace` names the workspace for this one call without switching the session to it - use it to pin a job whose `output:` or `asset:` references live in a workspace other than the session's - `acknowledged_cost` is `true` or the bound `{fingerprint, minutes, downloads}` from the validate plan; a 409 means the plan changed and the message carries the new estimate, and nothing is waited on | | `get_job(job_id)` | `job_id` | Get a job's status, warnings, output manifest, error and traceback; each manifest entry's `subfolder` is the in-run subfolder the step declared - by convention `final` for the deliverable, `intermediate` for scratch, `''` for none. A running job also carries `progress` (below) | | `get_job_workflow(job_id)` | `job_id` | The REST equivalent is `GET /api/jobs/{id}/workflow` (see [SERVER.md](SERVER.md#jobs-api)). The workflow the job actually ran. `realized: true` means every mutable input is pinned (arguments, seed, prompts, `output:latest`); `false` means the job predates run tracking and this is the definition as submitted. Pass it to `save_workflow` to keep it under a name | -| `export_job(job_id, overwrite=False)` | `job_id`, `overwrite` | Gather one finished job into `/exports//` on the server: the realized workflow, the run's manifest, the job row, a README, and copies of the assets, earlier-run inputs and outputs. Returns the directory, a zip URL, the file list with sizes and the total. The three JSON files are in the zip, not repeated here - get_job_workflow and get_job serve them individually. **The directory is on the machine running the server**, like `download_output`'s destination - fetch the zip URL and unpack it into `exports/` under the session's working directory (a deliverable, not a temp file); the archive already unpacks into one folder named after the job id | +| `export_job(job_id, overwrite=False)` | `job_id`, `overwrite` | Gather one finished job into `/exports//` on the server: the realized workflow, the run's manifest, the job row, a README, and copies of the assets, earlier-run inputs and outputs. Returns the directory, a zip URL, the file list with sizes and the total. The three JSON files are in the zip, not repeated here - get_job_workflow and get_job serve them individually. **The directory is on the machine running the server**, like `download_output`'s destination. `auth_required` says whether opening the zip needs this server's bearer token, which this agent cannot attach to someone else's fetch (#353): when false, fetch `open_url` yourself and unpack it into `exports/` under the session's working directory (a deliverable, not a temp file) - the archive already unpacks into one folder named after the job id, so do not create that folder first; when true, hand `open_url` to the person instead of fetching it | | `get_job_events(job_id, after=-1, limit=200)` | `job_id`, `after`, `limit` | Get a page of a job's progress events | -| `wait_for_job(job_id, timeout_seconds=20)` | `job_id`, `timeout_seconds` | Block until a job reaches a terminal status, or `timeout_seconds` elapses. **One call blocks for at most 55 seconds** — a larger `timeout_seconds` is clamped, not honoured, because no MCP client holds a tool call open for a generation's real runtime, so budget one call per ~55s of the job. Every reply carries `waited_seconds`, `timeout_requested_seconds`, `timeout_applied_seconds` and `timeout_capped`, so a capped return is distinguishable from an elapsed one. Use instead of hand-polling `get_job`/`get_job_events` in a loop; if it returns `still_running: true`, call it again. Returns a slim job - status, warnings, error, and the manifest once finished - without the arguments; `get_job` has those. A running job also carries `progress` (below) | +| `wait_for_job(job_id, timeout_seconds=20)` | `job_id`, `timeout_seconds` | Block until a job reaches a terminal status, or `timeout_seconds` elapses. **One call blocks for at most 55 seconds** — a larger `timeout_seconds` is clamped, not honoured, because no MCP client holds a tool call open for a generation's real runtime, so budget one call per ~55s of the job. Every reply carries `waited_seconds`, `timeout_requested_seconds`, `timeout_applied_seconds` and `timeout_capped`, so a capped return is distinguishable from an elapsed one. Use instead of hand-polling `get_job`/`get_job_events` in a loop; if it returns `still_running: true`, call it again. Returns a slim job - status, warnings, error, `run_id` and `run_version` (the run's `v5`, as the gallery labels it), and the manifest once finished - without the arguments; `get_job` has those. A running job also carries `progress` (below) | | `cancel_job(job_id)` | `job_id` | Ask a queued or running job to stop | | `clear_memory()` | — | Drop every loaded pipeline and the step cache, freeing VRAM/RAM immediately instead of waiting for the next job to evict one model for another. Also drops the step cache, so a seeded workflow that would otherwise reuse cached results regenerates on its next run. Refused with a 409 while a job is running or queued - the queue is FIFO, so wait for it to finish and retry rather than expecting this call to block until it does (#221) | | `rerun_job(job_id, acknowledged_cost=False, new_seed=False)` | `job_id`, `acknowledged_cost`, `new_seed` | Queue a fresh job from a previous job's stored specification. Costs GPU time, so it passes the same gate as `run_workflow`. `new_seed=true` draws a fresh seed into the workflow's seed variable — without it a seeded workflow's rerun repeats its arguments exactly and the step cache serves the whole run from the earlier one's files (`reused: true`), generating nothing. `get_job_workflow`'s `seed_variable` says whether there is one - `acknowledged_cost` is `true` or the bound `{fingerprint, minutes, downloads}` from the validate plan; a 409 means the plan changed and the message carries the new estimate | @@ -444,7 +445,10 @@ refused, and an existing file is left alone unless the caller passes Over `dw.serve --mcp` the write happens **on the server**, and there the destination is confined to that workspace: an absolute or `~` path outside it is refused, and a relative one is joined onto the workspace rather than -onto whatever the server process's working directory happens to be. The +onto whatever the server process's working directory happens to be. +`destination` is required over this transport - an omitted one is refused +rather than dropped loose in the workspace root, where nothing can find or +delete it later (#353). The transport is what distinguishes the two - on stdio "local disk" is genuinely the caller's own machine, over HTTP it is the operator's. Confinement is on the resolved real path, not a substring test, because an absolute path needs diff --git a/docs/RELEASING.md b/docs/RELEASING.md index a99f7945..e2e01d78 100644 --- a/docs/RELEASING.md +++ b/docs/RELEASING.md @@ -7,16 +7,130 @@ notes from commits at tag time (see below). This section is a scratch pad for items a branch's author wants the next release note to name; clear it when a release ships. -- `get_output_audio` now reads `GET /api/gallery/{name}/audio`, extracts a - video's soundtrack, and takes `start`/`duration` (moved off the - `/outputs` mount, which used to refuse `video/mp4`). -- New tool `get_output_frames` (route `GET /api/gallery/{name}/frames`): - a video seen as moments, a contact sheet, or seam pairs. -- `get_output_frames` takes `hear` (soundtrack around each `at` moment) and - reports `difference` per seam pair. +### 0.4.0 + +The auto-generated notes for this range are a single merge line, since the +work landed on `develop` without PRs. Paste this section into the GitHub +release body once the tag has published (`gh release edit v0.4.0 +--notes-file ...`). + +**Breaking and behaviour changes** + +- `download_output` over a `dw.serve --mcp` endpoint refuses a call with no + `destination`. It used to write into the server's own directory (#353). +- Untrusted workflows are refused in more cases (#409-#413): + - a `*_type` that doesn't resolve to a class, or that isn't a kind a + workflow constructs: a diffusers or transformers model, pipeline, + scheduler, tokenizer or processor, a quantization config, an auto + factory, a diffusers reference/condition type or an attention processor. + A plain `torch` class such as `torch.nn.Linear` is now refused; + - `constant:` walks through `_` names or out of the allowed packages; + - URLs with backslashes; + - `text/html` and `text/xml` result types; + - media hosts that aren't globally routable, including 100.64/10 (CGNAT, + and so Tailscale); + - more than 5 redirects; + - images over 50M pixels. + + Listings and export zips drop symlinks that escape their root. + `--trust-workflows` lifts all of these. +- `run_workflow` validates the caller's `arguments` when it queues the job + (#414/#415). `validate_workflow(arguments={})` checks a run with no values + supplied, not just the document (#364). +- A fractional value for an int variable is refused (#338), and so is a + still image passed as a video argument (#347). +- `templates/minimax/music` normalizes to -3 dBFS instead of -1, so its output + is quieter (#362). +- Every response carries `X-Content-Type-Options: nosniff` and + `X-Frame-Options: DENY`. Active document types under `/outputs` and + `/inputs` are served with `Content-Security-Policy: sandbox`. +- A validate-time probe reads only a literal media path that the run itself + would be allowed to read. +- A dict or list passed to a string-typed variable is refused (#433). + `templates/ltx2/keyframes` takes `first_image`/`last_image` as plain + strings, not `{"location": ...}` (#431/#433). +- `loop_frames` returns float32 frames in [0, 1] instead of uint8, the shape + `LTX2ReferenceCondition` needs; a keyframe condition still wants + `frames_as_array`. `ltx2/reference-sheet`'s default asset is now + `asset:reference_sheet.jpg` (#444). +- `validate_workflow` refuses a `components` name the pipeline doesn't + register; `duration_head` is gone from the in-context LTX-2 templates + (#442). +- A `{"media_type": "image"}` reference on a video argument loads as a + one-frame still (#443). +- `pair_audio fit: "video"` always fits, and warns on any nonzero gap + (#428/#429). `concat_videos` and `dissolve_videos` pad a short joined + track to the frame grid, warning (`joined_audio_padded_to_frames`) only + when the pad is a frame or more; a residual the AAC mux trims off is + logged, or warned as `joined_audio_short_after_mux` from a frame up. + `media.shots` is measured against the file as written (#426/#435/#454). + Neither warns about resampling inputs that agree to a pinned + `sample_rate` (#453). +- New warnings: `match_levels_near_silent` (#434), and `shot_span_overrun` + from the probes plus a validate-time check (#425). +- Error text changed: `delete_workspace` (#437/#438), the sub-workflow path + refusal names the places it looked (#422), and `/outputs/asset:...` misses + name the asset without server paths. + +**New** + +- The `assess_output` tool and `GET /api/gallery/{name}/assess`, plus the + probe tasks `analyze_shots`, `analyze_seams` and `analyze_sync_drift` + (#387/#388). +- A joined video records its shot boundaries (`media.shots`). + `get_output_frames(seams=true)` uses them, so it no longer needs + `boundaries` (#385). +- Run versions (`v`): + - `list_gallery` returns `run_id`/`version` and filters by `folder` and + `version`; + - `output:/v/` references; + - `wait_for_job` returns `run_version`; + - export zips download as `-vN-.zip`. +- `list_gallery(media=true)` adds durations, and `output:` names work in + gallery reads (#356). +- `DW_PUBLIC_URL` adds absolute URLs to gallery and export responses. + `export_job` also returns `auth_required` and `open_url` (#353). +- A `grade` task for images and video: exposure, contrast, saturation and + temperature/tint (#349). +- The `templates/minimax/shots-batch` H3 template (#352). +- Every generative template takes a `seed` argument (#351). +- `normalize_audio(target_lufs)`, and `integrated_lufs` plus true peak in + media metadata (#361). +- `gain_audio` with no region gains the whole track (#395). +- `world_fade_out_ms` on `assemble-and-score` (#339). +- Download progress shows in `phase_detail` (#343). `phase_stall` events + now read as informational (#357). +- `workflow`, `inline_workflow` and `prompt` also accept a JSON string. A + mistyped workflow name gets suggestions from the catalog (#397). +- Host caches are released when each job ends (#368), and the skills point + at `clear_memory`. +- `get_job_events(kinds=...)` and `?kinds=` on the event-log route; a kind + matches an event's `event` or its `kind`, so `["phase_stall"]` selects + one warning type (#436). +- `get_memory` reports the step cache's `entries` and `retained_bytes` + (#418). +- `get_output_image` and `/outputs` resolve `asset:` references (#445), and + `get_output_frames(seams=true)` works on linked assets (#430). +- Compact `assess_output` lists each finding once (#427). Shots are named by + their source when joined inputs already carry shots (#432). +- A task-only workflow's run history counts, so its estimate can quote + `basis: observed` (#439). The Music 3 hint no longer shows on video + (#441). + +**Fixes** + +- The step cache's retained-byte count no longer only grows (#418). +- `templates/ltx2/keyframes` (#431), `restore-decompression` (#442) and + `reference-sheet` (#444) run with their own defaults again. +- Joined audio and shot maps stay on the frame grid through repeated joins + (#423, #426, #428, #435). Releases are cut by pushing a `v` tag. CI does the rest. +Before merging `develop` into `master`, run `scripts/preflight.sh` and get it +passing. It covers more than CI: ruff over the whole repo rather than +`dw dw_mcp tests`, and the UI's Playwright e2e tests, which CI doesn't run. + ```bash scripts/release.sh 0.38.0 scripts/release.sh 0.38.0-alpha.1 "UI front end" # optional tag message diff --git a/docs/REMOTE.md b/docs/REMOTE.md index d8b3d507..2e26a1b0 100644 --- a/docs/REMOTE.md +++ b/docs/REMOTE.md @@ -33,6 +33,21 @@ Check it from the laptop: `hostname` and `device` are there so you can tell which machine answered. +If an agent on this box will ever export a job or list gallery/asset URLs to +hand to a person who isn't at a terminal on the box itself, set +`DW_API_TOKEN` and also set `DW_PUBLIC_URL` to this server's origin (for +example `https://dw.example.com`, or `http://:8765` with no proxy): + + DW_API_TOKEN= DW_PUBLIC_URL=https://dw.example.com dw-serve --host 0.0.0.0 --mcp + +Without it, `export_job`, `list_gallery` and the upload/asset routes only +return paths relative to this server (`/exports/job-1.zip`) - correct for a +browser already pointed at the box, useless handed to someone who isn't. +With `DW_PUBLIC_URL` set (or the equivalent `public_url` setting), those +responses add an `absolute_url` / `absolute_zip_url` built from it; nothing +guesses this from request headers, so an unconfigured server omits the +field rather than composing a wrong origin. + ## Browser Open `http://:8765`. Click the key icon next to the theme toggle, @@ -55,8 +70,12 @@ both require the token in an `Authorization: Bearer` header - the Two things differ from the local stdio setup: - `download_output` writes on the GPU box (where the MCP server runs), not - on your laptop. Use `get_output_image` / `get_output_text` to see a - result, or open `http://:8765/outputs/` in the browser. + on your laptop, so an omitted destination is refused rather than dropped + loose in the workspace root - nothing on your laptop would find or + delete it there (#353). Pass an explicit destination inside the + workspace to save one anyway, or use `get_output_image` / + `get_output_text` to see a result, or open + `http://:8765/outputs/` in the browser. - The connection is a plain HTTP call per tool invocation; there is no subprocess to restart. - `use_workspace`/`create_workspace` pin *this box's* one MCP client, shared diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 9c109781..a832a6b9 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -16,9 +16,9 @@ diffusers-workflow validates all file paths, user inputs, and URLs to protect ag ### Input Validation - `validate_variable_name()` — Alphanumeric, underscore, hyphen only (pattern: `^[a-zA-Z_][a-zA-Z0-9_-]*$`), max 100 chars -- `validate_string_input()` — Max length (default 1000 chars), no null bytes, no control characters other than tab/newline/CR +- `validate_string_input()` — Max length, no null bytes, no control characters other than tab/newline/CR. Every caller that checks a caller-supplied variable value (`dw/variables.py`, `dw/run.py`, the REPL) passes `MAX_VARIABLE_VALUE_LENGTH`, 20,000 characters; file names and paths pass their own, shorter caps, so the function's bare default of 1000 is not the limit anything is held to. A variable's *default*, written in the definition, is not capped separately: the author controls the file, and the whole file is capped at 50MB - `validate_json_size()` — Limits JSON files to 50MB -- `validate_url()` — Scheme must be `http` or `https`; must have a non-empty domain (`netloc`) +- `validate_url()` — Scheme must be `http` or `https`; must have a non-empty domain (`netloc`); may not contain a backslash. `urllib.parse` and the HTTP client disagree on which host `http://169.254.169.254\@example.com/` names, so the host the check approved need not be the one dialed; a `\` that belongs in a path is written `%5C` - `validate_constant_name()` — Guards `constant:` references before import: dotted-name pattern only, module must already be importable, and anything callable is refused - `safe_join_path()` — Joins path components after rejecting any that contain `..`, `/`, or `\\`. Defined in `security.py` but not currently called elsewhere in `dw/`. @@ -76,11 +76,38 @@ stay inside this set and load untrusted; a workflow that needs to reach outside it - a community pipeline module from somewhere else, a custom scheduler package - needs `--trust-workflows`. -A dotted `constant:` reference is gated the same way -(`dw/type_helpers.load_constant_from_name`): the module it names is -imported before `fetch_constant` gets to refuse a callable, so the import -itself is what the gate has to stop. A bare name (`constant:SOME_NAME`) -reads from `diffusers` and is always allowed. +The package is not the whole check, because an allowed package holds +things other than classes and re-exports modules outside itself. Untrusted, +two more rules apply (`dw/type_helpers.py`): + +- **A type reference must resolve to a class.** A `*_type`/`config_type` + value is constructed with the workflow's own arguments, so + `"torch.hub.load"` - in `torch`, and a function that fetches and runs a + GitHub repo's code - is refused as "not a class", as is a module or any + other object. Under a `dtype` or `*_dtype` key a `torch.dtype` + (`"torch.bfloat16"`) is accepted too, since that is data rather than + something called. A bare name (`"FluxPipeline"`) resolves against + `diffusers` and is held to the same rule. `validate_workflow` reports the + refusal at the key's path, for every key the run loads as a type + (`from_pretrained_arguments.torch_dtype` as much as `config_type`). +- **A `constant:` walk stays inside the package.** A dotted `constant:` + reference is gated like a type (the module it names is imported before + `fetch_constant` gets to refuse a callable, so the import itself is what + the gate has to stop), and then every step of the walk is checked: no + segment may start with `_`, checked before anything imports, and no module + the walk passes through may sit outside the allowed packages - + `constant:torch.os.environ` starts in `torch` and ends in the server's + environment, and is refused at `torch.os`. Reading a field off a value + declared in an allowed module still works + (`...ltx2.utils.GEMMA4_PROMPT_ENHANCEMENT_CONFIG.max_new_tokens`). A bare + name (`constant:SOME_NAME`) reads from `diffusers`. `validate_workflow` + resolves every literal `constant:` in a step the way the run does and + reports a refusal at its path (a variable's default at `variables.`), + so nothing is queued to find out. + +Every `*_type` and `constant:` in the bundled catalog satisfies both rules, +pinned by `tests/test_workflow_trust.py`'s catalog sweep. `--trust-workflows` +lifts both, as it lifts the package check. Two `from_pretrained_arguments` keys are refused untrusted as well, for every component and pipeline: `trust_remote_code` (runs the model repo's @@ -120,7 +147,20 @@ One policy now answers all of it, untrusted: cannot carry the expansion out. - **An `http(s)` URL** must not resolve to an address inside the deployment - loopback, link-local (`169.254.0.0/16`, the cloud metadata address), - private ranges. Checked after DNS resolution, not on the literal string. + private ranges, and anything else that is not globally routable + (`is_global`). Checked after DNS resolution, not on the literal string. + That last rule covers `100.64.0.0/10`, the shared address space, which is + **Tailscale's tailnet range** (and Alibaba's metadata address): a workflow + that fetches media from another machine on your tailnet is refused unless + it runs under `--trust-workflows`. +- **Every redirect is re-checked.** A media fetch (`safe_get`) never lets the + HTTP client follow a redirect on its own: it follows at most 5 hops + (`MAX_MEDIA_REDIRECTS`), and each `Location` passes the same scheme and + host policy before it is dialed. A public URL answering `302` to + `http://127.0.0.1:8765/api/server` is refused with the target named, and + nothing is fetched from it. Images are decoded from the fetched bytes and + still go through diffusers' `load_image`, so EXIF orientation and RGB + conversion are unchanged. - **`remote_text_encoder.url`** is https-only, and the HuggingFace token is attached only for `huggingface.co`, `huggingface.cloud` and `hf.space`. An endpoint elsewhere is still reachable; it just does not get the credential. @@ -213,10 +253,17 @@ SecurityError - **Path traversal** — Cannot access files outside allowed directories - **Command injection** — No shell interpretation is used anywhere in `dw/`; `sanitize_command_args()` is available as a guard should a subprocess call be added - **Resource exhaustion** — File size limits prevent memory exhaustion +- **Decompression bombs** — an image a caller names is decoded at no more than `MAX_DECODE_PIXELS` (50M; an 8K frame is 33M), checked after `Image.open` and before any decode: `get_output_image` (crops included) refuses it, the gallery thumbnail answers 413, and embedded metadata is read from the PNG header chunks without decoding. Video and audio decode are not limited - **Malicious URLs** — Only http/https schemes allowed, and an untrusted workflow may not name a host inside the deployment (SSRF) - **Arbitrary file read through a media argument** — a location a workflow supplies is confined to the roots it may read (`dw/locations.py`) +- **Script on the UI's origin through an output** — a step's + `result.content_type` may not be `text/html` or `text/xml` (compared + without case or parameters): validation refuses it at + `steps[i].result.content_type` and the writer refuses it again + (`dw/content_types.py`). A file of an active type that reaches `/outputs` + or `/inputs` anyway is served with `Content-Security-Policy: sandbox` ## Testing diff --git a/docs/SERVER.md b/docs/SERVER.md index f357d87c..bc0d36d6 100644 --- a/docs/SERVER.md +++ b/docs/SERVER.md @@ -441,7 +441,8 @@ The editor's forms come from these; they are just as usable from scripts: gallery entry carries `folder` (the workflow identity, the run id dropped) and `subfolder` (what followed the run id - the `final`/`intermediate` a step's `result.subfolder` chose, `''` when it chose none); `?folder=` and - `?subfolder=` filter independently, and the reply's `folders` and + `?subfolder=` filter independently (`?version=` too - with `?folder=`, + the one run the gallery labels `v4`), and the reply's `folders` and `subfolders` list every distinct value over the whole tree, `''` always a member of each so root-level files stay selectable - `GET /api/gallery/{name:path}/download` — download an output file @@ -586,6 +587,19 @@ network: attacker's page carries its own `Origin` while `Host` is whatever resolved. Scheme and port are ignored, so a TLS-terminating proxy that forwards `Host` unchanged needs no configuration. + An `Origin` that cannot be parsed is refused the same way (403), not + answered with a 500. +- Every response carries `X-Content-Type-Options: nosniff` and + `X-Frame-Options: DENY`: a browser renders nothing as a type the server + did not declare, and no page elsewhere can frame the UI. The UI itself + carries no Content-Security-Policy yet. +- `/outputs` and `/inputs` share the UI's origin, where the API token lives + in localStorage, so a file served as an active document type + (`text/html`, `application/xhtml+xml`, `text/xml`, `application/xml`, + `image/svg+xml`) carries `Content-Security-Policy: sandbox`: it opens + under an opaque origin with no script. Range and ETag answers are + unchanged. The engine does not write `text/html` or `text/xml` results + at all (below), so such a file is one planted on disk. - Requests carrying a `Host` header that names neither a loopback address nor the configured `--host` are rejected (400). A wildcard bind (`--host 0.0.0.0` or `::`) skips this check - clients reach such a @@ -632,7 +646,8 @@ When a token is configured, every `/api/*` request must carry `Authorization: Bearer ` or gets a 401. The UI's own static files and `/outputs` (generated media) stay reachable without it - the page has to load far enough for a user to enter the token, and an ``/`" + + +@pytest.fixture +def tree(tmp_path): + root = tmp_path / "ws" + for sub in ("workflows", "outputs", "assets", "prompts"): + (root / sub).mkdir(parents=True) + ui = tmp_path / "ui" + ui.mkdir() + (ui / "index.html").write_text("dw") + (root / "outputs" / "run.png").write_bytes(b"\x89PNG\r\n\x1a\n") + (root / "outputs" / "note.txt").write_text(SCRIPT) + return {"root": root, "ui": ui, "tmp": tmp_path} + + +@pytest.fixture +def client(tree, monkeypatch): + monkeypatch.setenv(TRUST_WORKFLOWS_ENV_VAR, "0") + root = tree["root"] + manager = JobManager( + str(root / "outputs"), + worker_manager=ScriptedWorkerManager(success_script), + history_path=str(tree["tmp"] / "jobs.sqlite"), + ) + app = create_app( + workflow_dir=str(root / "workflows"), + output_dir=str(root / "outputs"), + job_manager=manager, + prompt_dir=str(root / "prompts"), + asset_dir=str(root / "assets"), + workspace=str(root), + ui_dir=str(tree["ui"]), + ) + with TestClient(app, base_url="http://localhost") as test_client: + yield test_client + + +def _page(content_type): + return { + "id": "page", + "steps": [ + { + "name": "t", + "task": {"command": "compose_text", "arguments": {"parts": [SCRIPT]}}, + "result": {"content_type": content_type}, + } + ], + } + + +def _document_is_inert(response): + """A response a browser will not execute as a same-origin document: + not an active type, or forced to download, or sandboxed by CSP.""" + content_type = response.headers.get("content-type", "").split(";")[0].strip() + disposition = response.headers.get("content-disposition", "") + csp = response.headers.get("content-security-policy", "") + return ( + content_type not in ACTIVE_TYPES + or disposition.startswith("attachment") + or "sandbox" in csp + ) + + +# ------------------------------------------------ active content in outputs + + +class TestActiveOutputs: + """A workflow - which an MCP agent may author - could write an .html or + .xml output, and /outputs needs no token and shares the UI's origin. + Validation and the writer refuse those two result types (dw/ + content_types.py); a planted file still reaches /outputs, so it is + served sandboxed.""" + + @pytest.mark.parametrize( + "content_type", ["text/html", "text/xml", "Text/HTML; charset=utf-8"] + ) + def test_validation_refuses_it_at_its_path(self, monkeypatch, content_type): + from dw.workflow import Workflow + + monkeypatch.setenv(TRUST_WORKFLOWS_ENV_VAR, "0") + errors = Workflow(_page(content_type), "", "").validation_errors() + assert [e["path"] for e in errors] == ["steps[0].result.content_type"] + assert content_type in errors[0]["message"] + + @pytest.mark.parametrize("content_type", ["text/plain", "application/json"]) + def test_an_inert_text_type_still_validates(self, monkeypatch, content_type): + from dw.workflow import Workflow + + monkeypatch.setenv(TRUST_WORKFLOWS_ENV_VAR, "0") + assert Workflow(_page(content_type), "", "").validation_errors() == [] + + @pytest.mark.parametrize("content_type", ["text/html", "text/xml"]) + def test_the_writer_refuses_it_without_validation( + self, tmp_path, monkeypatch, content_type + ): + """A run that skipped validation still writes nothing active.""" + from dw.security import InvalidInputError + from dw.workflow import Workflow + + monkeypatch.setenv(TRUST_WORKFLOWS_ENV_VAR, "0") + workflow = Workflow(_page(content_type), str(tmp_path / "out"), "") + with pytest.raises(InvalidInputError, match=content_type): + workflow.run({}) + assert not [ + p for p in (tmp_path / "out").rglob("*") if p.suffix in (".html", ".xml") + ] + + def test_the_api_refuses_to_queue_it(self, client): + response = client.post("/api/jobs", json={"workflow": _page("text/html")}) + assert response.status_code == 400, response.text + assert "text/html" in response.text + + @pytest.mark.parametrize( + "name", ["page.html", "page.xhtml", "page.xml", "page.svg"] + ) + def test_outputs_does_not_serve_it_as_a_live_document(self, client, tree, name): + (tree["root"] / "outputs" / name).write_text(SCRIPT) + response = client.get(f"/outputs/{name}") + assert response.status_code == 200 + assert _document_is_inert(response), response.headers + + def test_the_sandbox_keeps_range_and_etag(self, client, tree): + (tree["root"] / "outputs" / "page.html").write_text(SCRIPT) + ranged = client.get("/outputs/page.html", headers={"Range": "bytes=0-3"}) + assert ranged.status_code == 206 + assert ranged.headers["content-security-policy"] == "sandbox" + etag = client.get("/outputs/page.html").headers["etag"] + cached = client.get("/outputs/page.html", headers={"If-None-Match": etag}) + assert cached.status_code == 304 + + def test_an_inert_output_is_not_sandboxed(self, client): + for path in ("/outputs/run.png", "/outputs/note.txt"): + assert "content-security-policy" not in client.get(path).headers + + def test_a_text_output_is_served_as_plain_text(self, client): + response = client.get("/outputs/note.txt") + assert response.headers["content-type"].startswith("text/plain") + + @pytest.mark.parametrize( + "name", ["page.html", "page.svg", "page.xml", "page.xhtml"] + ) + def test_an_upload_cannot_plant_one(self, client, name): + response = client.post(f"/api/uploads?filename={name}", content=SCRIPT.encode()) + assert response.status_code == 400 + + def test_keep_output_cannot_carry_one_into_assets(self, client, tree): + (tree["root"] / "outputs" / "page.html").write_text(SCRIPT) + response = client.post( + "/api/assets/keep", json={"name": "page.html", "asset_name": "cast.html"} + ) + if response.status_code < 400: + assert _document_is_inert(client.get("/inputs/cast.html")) + + def test_keep_output_cannot_rename_one_to_svg(self, client, tree): + (tree["root"] / "outputs" / "page.html").write_text(SCRIPT) + response = client.post( + "/api/assets/keep", json={"name": "page.html", "asset_name": "cast.svg"} + ) + assert response.status_code == 400 + assert not (tree["root"] / "assets" / "cast.svg").exists() + + +# ------------------------------------------------------------ the headers + +UI_AND_MEDIA = ["/", "/index.html", "/outputs/run.png", "/outputs/note.txt"] + + +class TestBrowserHeaders: + @pytest.mark.xfail( + strict=True, + reason="deferred by #407: no Content-Security-Policy on the UI, so " + "nothing limits what a script injected into the page may load or " + "where it may send the token", + ) + def test_the_ui_carries_a_content_security_policy(self, client): + response = client.get("/") + assert response.status_code == 200 + assert "script-src" in response.headers.get( + "content-security-policy", "" + ) or "default-src" in response.headers.get("content-security-policy", "") + + def test_the_ui_cannot_be_framed(self, client): + response = client.get("/") + frame_options = response.headers.get("x-frame-options", "").upper() + csp = response.headers.get("content-security-policy", "") + assert frame_options in ("DENY", "SAMEORIGIN") or "frame-ancestors" in csp + + @pytest.mark.parametrize("path", UI_AND_MEDIA) + def test_nosniff(self, client, path): + response = client.get(path) + assert response.status_code == 200, path + assert response.headers.get("x-content-type-options") == "nosniff" + + def test_the_api_answers_json_as_json(self, client): + """The one thing that keeps an API body from being rendered as a + page today: its declared type.""" + response = client.get("/api/health") + assert response.headers["content-type"].startswith("application/json") + + +# ------------------------------------------------------------------ CORS + + +class TestCrossOriginReads: + EVIL = "https://evil.example" + + @pytest.mark.parametrize( + "path", ["/api/health", "/api/workflows", "/outputs/run.png", "/"] + ) + def test_no_response_grants_a_foreign_origin_read_access(self, client, path): + response = client.get(path, headers={"Origin": self.EVIL}) + assert "access-control-allow-origin" not in response.headers + assert "access-control-allow-credentials" not in response.headers + + def test_a_preflight_from_a_foreign_origin_is_refused(self, client): + response = client.options( + "/api/jobs", + headers={ + "Origin": self.EVIL, + "Access-Control-Request-Method": "POST", + "Access-Control-Request-Headers": "authorization,content-type", + }, + ) + assert response.status_code >= 400 + assert "access-control-allow-origin" not in response.headers + + def test_a_same_origin_page_is_not_refused(self, client): + response = client.get("/api/health", headers={"Origin": "http://localhost"}) + assert response.status_code == 200 + + +# ------------------------------------------------- download file names + + +class TestDownloadNames: + """A file name on disk ends up in a Content-Disposition header. It must + not be able to add a header or break out of the quoted value.""" + + @pytest.mark.parametrize( + "name", + [ + 'quote"; filename="evil.html.png', + "semi;colon.png", + "unicode-‮txt.png", + "crlf\r\nX-Injected: 1.png", + "lf\nSet-Cookie: dw=1.png", + ], + ) + def test_a_hostile_name_cannot_inject_a_header(self, client, tree, name): + try: + (tree["root"] / "outputs" / name).write_bytes(b"\x89PNG") + except OSError: + pytest.skip("this filesystem refuses the name") + from urllib.parse import quote + + response = client.get(f"/api/gallery/{quote(name)}/download") + assert "x-injected" not in response.headers + assert "set-cookie" not in response.headers + disposition = response.headers.get("content-disposition", "") + assert "\r" not in disposition and "\n" not in disposition + # the name, however spelled, is one parameter, not a second filename= + assert disposition.count("filename=") <= 1 + + def test_the_gallery_listing_reports_a_hostile_name_as_data(self, client, tree): + """Escaping is the UI's job (ui/e2e/security.spec.ts); the API's is + to report the name exactly, inside JSON, so nothing is pre-rendered.""" + name = "x\"'>.png" + (tree["root"] / "outputs" / name).write_bytes(b"\x89PNG") + response = client.get("/api/gallery") + assert response.headers["content-type"].startswith("application/json") + names = [entry["name"] for entry in response.json()["files"]] + assert name in names + json.dumps(names) # round-trips as plain data diff --git a/tests/test_select.py b/tests/test_select.py index ac5148a9..98907922 100644 --- a/tests/test_select.py +++ b/tests/test_select.py @@ -112,8 +112,3 @@ def test_it_runs_through_the_task_dispatch(self): ) assert result == "b" - - def test_command_registered(self): - from dw.tasks.task import _COMMAND_REGISTRY - - assert "select" in _COMMAND_REGISTRY diff --git a/tests/test_server.py b/tests/test_server.py index 56f1b165..48f4e0ca 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -407,6 +407,40 @@ def test_submit_validation(server): assert client.app.state.job_manager.worker_manager.commands == [] +def test_submit_refuses_a_content_type_that_only_resolves_active_via_arguments(server): + # #414: a literal "text/html" is refused before queueing (#410); the same + # value reached through "variable:ct" and the caller's own arguments used + # to slip past submit_job's schema-only validate() and queue a job that + # then failed at save time - the same call POST /api/validate already + # refused + workflow = { + "id": "se-f035", + "variables": {"ct": "text/plain"}, + "steps": [ + { + "name": "t", + "task": {"command": "compose_text", "arguments": {"parts": ["x"]}}, + "result": {"content_type": "variable:ct"}, + } + ], + } + with server(success_script) as client: + validated = client.post( + "/api/validate", + json={"workflow": workflow, "arguments": {"ct": "text/html"}}, + ).json() + assert validated["valid"] is False + + response = client.post( + "/api/jobs", + json={"workflow": workflow, "arguments": {"ct": "text/html"}}, + ) + assert response.status_code == 400 + assert "text/html" in response.json()["detail"] + # nothing reached the worker + assert client.app.state.job_manager.worker_manager.commands == [] + + def test_submit_accepts_a_stored_workflow_name(server, tmp_path): """The names /api/workflows hands out are what an agent has in hand, so they must be submittable as-is - with or without .json, nested included.""" @@ -536,6 +570,16 @@ def test_submit_rejects_a_traversal_shaped_name(server): assert client.app.state.job_manager.worker_manager.commands == [] +def test_submit_names_a_suggestion_for_a_short_workflow_path(server): + """#397: a workflow_path that's a catalog entry's trailing segment (the + name a skill or earlier turn is likely to say) points at the entry + rather than a bare 400.""" + with server(success_script) as client: + response = client.post("/api/jobs", json={"workflow_path": "asic"}) + assert response.status_code == 400 + assert "did you mean Basic?" in response.json()["detail"] + + def test_validate_accepts_a_stored_workflow_name(server, tmp_path): with server(success_script) as client: result = client.post("/api/validate", json={"workflow_path": "Basic"}).json() @@ -682,6 +726,22 @@ def test_workflow_browsing_and_confinement(server): assert client.get("/api/workflows/../secret").status_code == 404 assert client.get("/api/workflows/nope").status_code == 404 + # #397: a name that is a catalog entry's trailing segment points at + # the entry it's short for, rather than a bare 404 + client.put("/api/workflows/sub/Basic", json={"workflow": valid_workflow()}) + missed = client.get("/api/workflows/nope2") + assert missed.status_code == 404 + assert "did you mean" not in missed.json()["detail"] + + short = client.get("/api/workflows/Basic") + assert ( + short.status_code == 200 + ) # the top-level name still shadows the nested one + + suggested = client.get("/api/workflows/sub%2FBasik") + assert suggested.status_code == 404 + assert "sub/Basic" in suggested.json()["detail"] + def test_configures_resolves_against_the_listing(server): with server(success_script) as client: @@ -1189,13 +1249,6 @@ def test_bearer_token_gates_the_api_when_configured(tmp_path): assert client.get(f"/api/jobs/{job['id']}/events").status_code == 401 -def test_no_token_configured_means_no_auth(server): - """The default, unconfigured behavior is unchanged: no token means no - Authorization check at all.""" - with server(success_script) as client: - assert client.get("/api/health").status_code == 200 - - def test_save_workflow_roundtrip_and_confinement(server, tmp_path): with server(success_script) as client: workflow = valid_workflow("saved") @@ -1305,6 +1358,59 @@ def test_gallery_metadata_describes_audio_and_video(server, tmp_path): assert still["media"] is None +def test_gallery_metadata_accepts_an_output_reference(server, tmp_path): + """A name copied from an 'output:' reference used to 404 with "path does + not exist" instead of resolving - the prefix was joined straight into the + path rather than stripped first (#356).""" + from tests.test_media_info import write_wav + + with server(success_script) as client: + outputs = tmp_path / "outputs" + write_wav(outputs / "score-gen.0-0.0.wav", seconds=2.0) + + plain = client.get("/api/gallery/score-gen.0-0.0.wav/metadata") + prefixed = client.get("/api/gallery/output:score-gen.0-0.0.wav/metadata") + + assert prefixed.status_code == plain.status_code == 200 + assert prefixed.json()["media"]["duration_seconds"] == pytest.approx( + 2.0, abs=0.01 + ) + + +def test_gallery_output_reference_traversal_is_still_refused(server, tmp_path): + """Stripping the 'output:' prefix must not open a new escape - the + stripped remainder still goes through validate_path (#356).""" + with server(success_script) as client: + response = client.get("/api/gallery/output:../jobs.sqlite/metadata") + assert response.status_code == 404 + assert (tmp_path / "jobs.sqlite").exists() + + +def test_gallery_lists_media_duration_when_asked(server, tmp_path): + """size and mtime are misleading proxies for a take's length - a + bitrate difference can make a shorter file the bigger one - so + ?media=true adds duration_seconds per entry, bounded by the page + returned rather than the whole library (#356).""" + from tests.test_media_info import write_mp4, write_wav + + with server(success_script) as client: + outputs = tmp_path / "outputs" + write_wav(outputs / "score-gen.0-0.0.wav", seconds=2.0) + write_mp4(outputs / "shot-gen.0-0.0.mp4", frames=12, fps=6) + + default = client.get("/api/gallery").json() + assert all("duration_seconds" not in e for e in default["files"]) + + with_media = client.get("/api/gallery", params={"media": "true"}).json() + by_name = {e["name"]: e for e in with_media["files"]} + assert by_name["score-gen.0-0.0.wav"]["duration_seconds"] == pytest.approx( + 2.0, abs=0.01 + ) + assert by_name["shot-gen.0-0.0.mp4"]["duration_seconds"] == pytest.approx( + 2.0, abs=0.1 + ) + + def test_gallery_audio_extracts_a_videos_soundtrack(server, tmp_path): """get_output_audio refused video/mp4 outright, so a generated clip's soundtrack could only be heard by fetching the file and demuxing it by @@ -1517,6 +1623,118 @@ def test_gallery_frames_returns_the_moments_asked_for(server, tmp_path): assert _png_of(body["tiles"][0]).size == (32, 16) +def test_gallery_frames_seams_read_a_joined_outputs_recorded_shots(server, tmp_path): + """#385: `seams` without `boundaries` was a 400 - the file carried no + seams of its own. An output whose run recorded shots for it now answers + from the manifest, named as recorded; `media.shots` in the metadata is + the same list. A file whose run recorded none still needs `boundaries`.""" + import json + + from tests.test_media_frames import write_ramp_mp4 + + with server(success_script) as client: + run = tmp_path / "outputs" / "cut" / "20260924-000000-0123abcd" + run.mkdir(parents=True) + write_ramp_mp4(run / "cut.mp4", frames=24, fps=6, width=64, height=32) + write_ramp_mp4(run / "other.mp4", frames=24, fps=6, width=64, height=32) + shots = [ + { + "name": "shot@wide", + "start_frame": 0, + "num_frames": 10, + "start_sample": None, + "num_samples": None, + }, + { + "name": "shot@close", + "start_frame": 10, + "num_frames": 14, + "start_sample": None, + "num_samples": None, + }, + ] + (run / "manifest.json").write_text( + json.dumps( + { + "steps": [ + {"step": "cut", "files": ["cut.mp4"], "shots": shots}, + {"step": "other", "files": ["other.mp4"]}, + ] + } + ) + ) + name = "cut/20260924-000000-0123abcd/cut.mp4" + + response = client.get(f"/api/gallery/{name}/frames", params={"seams": "true"}) + assert response.status_code == 200, response.text + (tile,) = response.json()["tiles"] + assert tile["label"] == "seam 1: shot@wide | shot@close" + + metadata = client.get(f"/api/gallery/{name}/metadata").json() + assert metadata["media"]["shots"] == shots + + refused = client.get( + "/api/gallery/cut/20260924-000000-0123abcd/other.mp4/frames", + params={"seams": "true"}, + ) + assert refused.status_code == 400 + assert "boundaries" in refused.json()["detail"] + + +def test_gallery_frames_seams_read_a_linked_assets_recorded_shots( + asset_server, tmp_path +): + """#430: `seams` without `boundaries` on a linked asset (`keep_output`'s + sidecar manifest, `record_kept_shots`) was refused with "this file's run + recorded no shots for it" even though `get_gallery_metadata`'s + `media.shots` had them - the route only ever checked the *output* + manifest (`recorded_shots`), never the asset sidecar (`shots_beside`) + that `get_gallery_metadata`/`assess_output` already read.""" + import json + + from tests.test_media_frames import write_ramp_mp4 + + with asset_server(success_script) as client: + assets = tmp_path / "assets" + assets.mkdir(parents=True, exist_ok=True) + write_ramp_mp4(assets / "cast.mp4", frames=24, fps=6, width=64, height=32) + shots = [ + { + "name": "shot@wide", + "start_frame": 0, + "num_frames": 10, + "start_sample": None, + "num_samples": None, + }, + { + "name": "shot@close", + "start_frame": 10, + "num_frames": 14, + "start_sample": None, + "num_samples": None, + }, + ] + (assets / "manifest.json").write_text( + json.dumps( + { + "steps": [ + {"step": "keep_output", "files": ["cast.mp4"], "shots": shots} + ] + } + ) + ) + + response = client.get( + "/api/gallery/asset:cast.mp4/frames", params={"seams": "true"} + ) + assert response.status_code == 200, response.text + (tile,) = response.json()["tiles"] + assert tile["label"] == "seam 1: shot@wide | shot@close" + + metadata = client.get("/api/gallery/asset:cast.mp4/metadata").json() + assert metadata["media"]["shots"] == shots + + def test_gallery_frames_crop_names_the_same_source_region_at_any_max_dimension( server, tmp_path ): @@ -1993,6 +2211,129 @@ def test_gallery_reports_and_filters_by_subfolder(server, tmp_path): assert finals["subfolders"] == full["subfolders"] +def test_gallery_reports_each_run_version(server, tmp_path): + """Four runs of one workflow write the same basename, so the gallery + label alone cannot tell them apart. Every entry carries the run it came + from and that run's ordinal - 'v4' - which is the handle an agent quotes + and a person finds in the grid.""" + import json as _json + + from PIL import Image + + def _run(identity, run_id, version=None): + run = tmp_path / "outputs" / identity / run_id + (run / "final").mkdir(parents=True) + Image.new("RGB", (2, 2)).save(run / "final" / "film.7-0.0.png") + manifest = {"run_id": run_id} + if version is not None: + manifest["version"] = version + (run / "manifest.json").write_text(_json.dumps(manifest)) + return run + + with server(success_script) as client: + _run("acorn/cut", "20260901-120000-aaaaaaaa", version=1) + # v2 was deleted; v3 keeps its number rather than sliding down + _run("acorn/cut", "20260903-120000-cccccccc", version=3) + # a run from before the field existed is ranked, not dropped + _run("acorn/score", "20260902-120000-bbbbbbbb") + # the flat layout has no runs at all + (tmp_path / "outputs" / "ltx").mkdir() + Image.new("RGB", (2, 2)).save(tmp_path / "outputs" / "ltx" / "flat.png") + + by_name = {f["name"]: f for f in client.get("/api/gallery").json()["files"]} + + first = by_name["acorn/cut/20260901-120000-aaaaaaaa/final/film.7-0.0.png"] + assert first["version"] == 1 + assert first["run_id"] == "20260901-120000-aaaaaaaa" + third = by_name["acorn/cut/20260903-120000-cccccccc/final/film.7-0.0.png"] + assert third["version"] == 3 + # the two runs are indistinguishable by label alone - which is the + # whole reason the version is here + assert first["label"] == third["label"] == "film.7-0.0.png" + # numbering is per workflow identity, so another workflow's first + # run is its own v1 + assert ( + by_name["acorn/score/20260902-120000-bbbbbbbb/final/film.7-0.0.png"][ + "version" + ] + == 1 + ) + assert by_name["ltx/flat.png"]["version"] is None + assert by_name["ltx/flat.png"]["run_id"] == "" + + # "show me v3": folder and version together list exactly that run + names = [ + f["name"] + for f in client.get( + "/api/gallery", params={"folder": "acorn/cut", "version": 3} + ).json()["files"] + ] + assert names == ["acorn/cut/20260903-120000-cccccccc/final/film.7-0.0.png"] + # version alone spans workflows: each one's v1 + v1 = client.get("/api/gallery", params={"version": 1}).json()["files"] + assert {f["folder"] for f in v1} == {"acorn/cut", "acorn/score"} + + +def test_gallery_metadata_names_the_run_and_its_version(server, tmp_path): + """After "look at version 3", the next call is usually this one - so it + answers with the run and the ordinal rather than making the caller go + back to the listing to confirm it read the right file.""" + import json as _json + + from PIL import Image + + with server(success_script) as client: + run = tmp_path / "outputs" / "acorn/cut" / "20260903-120000-cccccccc" + (run / "final").mkdir(parents=True) + Image.new("RGB", (2, 2)).save(run / "final" / "film.7-0.0.png") + (run / "manifest.json").write_text(_json.dumps({"version": 3})) + + body = client.get( + "/api/gallery/acorn/cut/20260903-120000-cccccccc" + "/final/film.7-0.0.png/metadata" + ).json() + assert body["run_id"] == "20260903-120000-cccccccc" + assert body["version"] == 3 + + +def test_deleting_an_older_run_renumbers_none_of_its_siblings(server, tmp_path): + """Runs from before versions existed are ranked, so removing the oldest + would slide every later one down a number. The delete pins the + siblings' numbers into their manifests first - both for a whole run + directory and for the last file of a run, which sweeps the directory.""" + import json as _json + + from PIL import Image + + identity = tmp_path / "outputs" / "acorn" / "cut" + run_ids = [f"2026090{day}-120000-aaaaaaaa" for day in (1, 2, 3, 4)] + with server(success_script) as client: + for run_id in run_ids: + (identity / run_id).mkdir(parents=True) + Image.new("RGB", (2, 2)).save(identity / run_id / "film.png") + (identity / run_id / "manifest.json").write_text( + _json.dumps({"run_id": run_id}) + ) + + def versions(): + return { + f["run_id"]: f["version"] + for f in client.get("/api/gallery").json()["files"] + } + + assert versions() == dict(zip(run_ids, (1, 2, 3, 4))) + # the whole run directory + assert client.delete(f"/api/gallery/acorn/cut/{run_ids[0]}").status_code == 200 + assert versions() == dict(zip(run_ids[1:], (2, 3, 4))) + # the last file of a run, which takes its directory with it + assert ( + client.delete(f"/api/gallery/acorn/cut/{run_ids[1]}/film.png").status_code + == 200 + ) + assert not (identity / run_ids[1]).exists() + assert versions() == dict(zip(run_ids[2:], (3, 4))) + + def test_gallery_only_orphans_lists_media_less_run_directories(server, tmp_path): """#170: a run whose output was deleted before `delete_output` could remove it by name, or one that failed before writing anything, has no @@ -2089,6 +2430,29 @@ def test_gallery_thumbnail_is_smaller_than_the_original(server, tmp_path): assert client.get("/api/gallery/clip.mp4/thumbnail").status_code == 404 +def test_output_file_route_resolves_an_asset_reference(asset_server, tmp_path): + """#445: get_output_image/get_output_text hit '/outputs/' directly, + and an 'asset:' name there used to 404 with no hint - every sibling + gallery route (metadata, /frames, /audio, /assess) already resolves one.""" + from PIL import Image + + with asset_server(success_script) as client: + Image.new("RGB", (4, 4), "red").save(tmp_path / "assets" / "portrait.png") + + response = client.get("/outputs/asset:portrait.png") + + assert response.status_code == 200 + assert response.headers["content-type"] == "image/png" + + missing = client.get("/outputs/asset:nothing.png") + assert missing.status_code == 404 + # /outputs is outside the token gate: the miss still says what went + # wrong, but never names a server path + detail = missing.json()["detail"] + assert "nothing.png" in detail + assert str(tmp_path) not in detail + + def test_gallery_urls_change_when_a_file_is_rewritten(server, tmp_path): """A rerun overwrites the same name - the URL must move or the browser keeps showing the image it already cached.""" @@ -2208,6 +2572,20 @@ def test_gallery_delete_and_job_linkage(server, tmp_path): assert (tmp_path / "jobs.sqlite").exists() +def test_gallery_delete_accepts_an_output_reference(server, tmp_path): + """The same 'output:' prefix delete_output rejected before #356.""" + from PIL import Image + + with server(success_script) as client: + outputs = tmp_path / "outputs" + Image.new("RGB", (4, 4)).save(outputs / "victim.png") + + response = client.delete("/api/gallery/output:victim.png") + + assert response.status_code == 200 + assert not (outputs / "victim.png").exists() + + def test_upload_media_saves_file_and_returns_absolute_path(server, tmp_path): with server(success_script) as client: response = client.post( @@ -2233,6 +2611,23 @@ def test_upload_media_saves_file_and_returns_absolute_path(server, tmp_path): assert fetched.content == b"not-really-png-bytes" +def test_upload_media_adds_an_absolute_url_when_a_public_url_is_configured( + server, monkeypatch +): + # #353: a client with no way to learn this server's origin otherwise - + # an MCP-only agent - gets an absolute_url only when an operator + # configured one; nothing derives an origin from request headers. + monkeypatch.setenv("DW_PUBLIC_URL", "https://dw.example.com") + with server(success_script) as client: + body = client.post( + "/api/uploads", + params={"filename": "source-image.png"}, + content=b"not-really-png-bytes", + ).json() + + assert body["absolute_url"] == f"https://dw.example.com{body['url']}" + + @pytest.fixture def asset_server(tmp_path): """A server with an asset library configured, which is where uploads go.""" @@ -2475,35 +2870,12 @@ def test_listing_assets_without_a_library_is_empty_not_an_error(server): assert body["shadowed"] == [] -def test_an_audio_file_can_be_uploaded(asset_server, tmp_path): - """A workflow's audio reference is built from a .wav - refusing it would - leave one input kind with no way onto the machine.""" - with asset_server(success_script) as client: - response = client.post( - "/api/uploads", params={"filename": "voice.wav"}, content=b"riff" - ) - assert response.status_code == 201 - assert response.json()["path"].startswith("asset:uploads/") - assert response.json()["path"].endswith(".wav") - - def test_the_asset_library_is_reported(asset_server, tmp_path): with asset_server(success_script) as client: directories = client.get("/api/server").json()["directories"] assert directories["assets"] == str(tmp_path / "assets") -def test_without_an_asset_library_uploads_keep_the_old_shape(server, tmp_path): - with server(success_script) as client: - body = client.post( - "/api/uploads", - params={"filename": "source-image.png"}, - content=b"bytes", - ).json() - assert os.path.isabs(body["path"]) - assert body["url"].startswith("/outputs/uploads/") - - def test_upload_media_rejects_disallowed_extension(server): with server(success_script) as client: response = client.post( @@ -2571,34 +2943,6 @@ def test_workflow_listing_carries_details(server): assert listing["details"]["Basic"]["kinds"] == [] -def test_workflow_details_name_the_template_a_model_config_configures(server): - """A model config is a tuned instance of a template, and a client that - cannot see which is which shows it as just another catalog entry - the - thing the two-tree layout exists to stop.""" - with server(success_script) as client: - client.put( - "/api/workflows/templates/text-to-image", - json={"workflow": valid_workflow("tti")}, - ) - - workflow = valid_workflow("tuned") - workflow["configures"] = "templates/text-to-image" - client.put("/api/workflows/models/Tuned", json={"workflow": workflow}) - - listing = client.get("/api/workflows").json() - - assert listing["details"]["models/Tuned"]["configures"] == ( - "templates/text-to-image" - ) - - -def test_a_workflow_that_configures_nothing_says_so(server): - with server(success_script) as client: - listing = client.get("/api/workflows").json() - - assert listing["details"]["Basic"]["configures"] == "" - - def test_the_listing_filters_and_compacts(server): with server(success_script) as client: client.put( @@ -3827,35 +4171,53 @@ def test_event_log_pages_with_after_and_limit(server): assert rest["truncated"] is False -def test_event_log_clamps_a_negative_after(server): +def test_event_log_filters_by_kinds(server): with server(success_script) as client: job_id = client.post("/api/jobs", json={"workflow": valid_workflow()}).json()[ "id" ] wait_for_status(client, job_id, TERMINAL_STATES) + unfiltered = client.get(f"/api/jobs/{job_id}/event-log").json()["events"] + assert {event["event"] for event in unfiltered} >= { + "step_start", + "pipeline_step", + }, "test needs a job with more than one kind of event" - body = client.get(f"/api/jobs/{job_id}/event-log?after=-99").json() + body = client.get(f"/api/jobs/{job_id}/event-log?kinds=step_start").json() - assert body["events"][0]["seq"] == 0 + assert body["events"], "the requested kind should still be present" + assert {event["event"] for event in body["events"]} == {"step_start"} + assert len(body["events"]) < len(unfiltered) -def test_event_log_serves_a_historical_jobs_persisted_events(server): - """A job recovered from sqlite is a plain dict, but its event tail was - persisted with it - that is what makes last night's failure explainable.""" +def test_event_log_filters_by_multiple_kinds(server): with server(success_script) as client: - manager = client.app.state.job_manager - manager.get = lambda job_id: {"id": job_id, "status": "failed"} - manager.history.events_for = lambda job_id: [ - {"seq": 0, "event": "phase", "phase": "loading"}, - {"seq": 1, "event": "job_status", "status": "failed"}, + job_id = client.post("/api/jobs", json={"workflow": valid_workflow()}).json()[ + "id" ] + wait_for_status(client, job_id, TERMINAL_STATES) - body = client.get("/api/jobs/historical/event-log").json() + body = client.get( + f"/api/jobs/{job_id}/event-log?kinds=step_start&kinds=step_end" + ).json() - assert [event["seq"] for event in body["events"]] == [0, 1] - assert body["last_seq"] == 1 - assert body["truncated"] is False - assert body["note"] is None + assert body["events"] + assert {event["event"] for event in body["events"]} <= { + "step_start", + "step_end", + } + + +def test_event_log_clamps_a_negative_after(server): + with server(success_script) as client: + job_id = client.post("/api/jobs", json={"workflow": valid_workflow()}).json()[ + "id" + ] + wait_for_status(client, job_id, TERMINAL_STATES) + + body = client.get(f"/api/jobs/{job_id}/event-log?after=-99").json() + + assert body["events"][0]["seq"] == 0 def test_event_log_pages_a_historical_jobs_events(server): @@ -3950,6 +4312,7 @@ def test_a_recorded_job_reads_back_through_the_event_log_route(server): body = client.get("/api/jobs/recorded/event-log").json() assert [event["seq"] for event in body["events"]] == [0, 1] + assert body["last_seq"] == 1 assert body["events"][0]["phase"] == "loading" assert body["status"] == "complete" assert body["truncated"] is False @@ -3975,21 +4338,6 @@ def test_a_recorded_job_whose_log_was_dropped_says_so_through_the_route(server): assert f"last {MAX_PERSISTED_EVENTS}" in body["note"] -def test_workflow_details_name_their_variables(server): - """The listing says which knobs a workflow takes, so an agent picking a - workflow to run knows what to pass without fetching each candidate's - full definition. Names only - the defaults of every workflow on disk - are an order of magnitude more payload on a listing the UI reloads.""" - with server(success_script) as client: - workflow = valid_workflow("knobby") - workflow["variables"] = {"prompt": "a cat", "steps": 25} - client.put("/api/workflows/Knobby", json={"workflow": workflow}) - - details = client.get("/api/workflows").json()["details"] - assert details["Knobby"]["variable_names"] == ["prompt", "steps"] - assert details["Knobby"]["variables"] == 2 - - def test_workflow_details_describe_their_lists(server): """A list-driven workflow's listing says what an entry carries, so an agent can write the list without opening the definition.""" diff --git a/tests/test_server_assess.py b/tests/test_server_assess.py new file mode 100644 index 00000000..17894e52 --- /dev/null +++ b/tests/test_server_assess.py @@ -0,0 +1,282 @@ +"""Tests for GET /api/gallery/{name}/assess (#388). + +The route runs the real probes over real mp4 files written with PyAV - no +mocked probe - with the shot boundaries in a run manifest (an output) or in +the sidecar keep_output writes (an asset), the two places the route reads +them from. +""" + +import json + +import numpy +import pytest +from fastapi.testclient import TestClient +from PIL import Image + +from dw.runs import MANIFEST_FILE_NAME, record_kept_shots +from dw.server.app import create_app +from dw.server.jobs import JobManager +from dw.shots import shot_record +from tests.test_server import ( + ScriptedWorkerManager, + hanging_script, + success_script, + valid_workflow, + wait_for_status, +) + +FPS = 24 +SAMPLE_RATE = 48000 +FRAMES_PER_SHOT = 24 +SAMPLES_PER_SHOT = SAMPLE_RATE * FRAMES_PER_SHOT // FPS +RUN = "cut/20260923-120000-abcdef01" +CUT = f"{RUN}/final/cut.mp4" + + +def write_cut(path, amplitudes): + """A real mp4 of len(amplitudes) shots: one grey level and one tone + level per shot, hard cuts between them.""" + import av + + path.parent.mkdir(parents=True, exist_ok=True) + container = av.open(str(path), "w") + video = container.add_stream("libx264", rate=FPS) + video.width, video.height, video.pix_fmt = 64, 64, "yuv420p" + audio = container.add_stream("aac", rate=SAMPLE_RATE) + audio.layout = "stereo" + + rng = numpy.random.default_rng(0) + for shot in range(len(amplitudes)): + for _ in range(FRAMES_PER_SHOT): + grey = 60 + 60 * shot + rng.normal(0, 3, size=(64, 64, 3)) + arr = numpy.clip(grey, 0, 255).astype(numpy.uint8) + for packet in video.encode(av.VideoFrame.from_ndarray(arr, "rgb24")): + container.mux(packet) + + t = numpy.arange(SAMPLES_PER_SHOT) / SAMPLE_RATE + tone = numpy.sin(2 * numpy.pi * 440.0 * t) + track = numpy.concatenate([amp * tone for amp in amplitudes]).astype(numpy.float32) + track = numpy.tile(track, (2, 1)) + for start in range(0, track.shape[1], 1024): + chunk = av.AudioFrame.from_ndarray( + numpy.ascontiguousarray(track[:, start : start + 1024]), + format="fltp", + layout="stereo", + ) + chunk.sample_rate = SAMPLE_RATE + chunk.pts = start + for packet in audio.encode(chunk): + container.mux(packet) + for packet in audio.encode(): + container.mux(packet) + for packet in video.encode(): + container.mux(packet) + container.close() + + +def shots_of(count): + return [ + shot_record( + f"s{index}", + index * FRAMES_PER_SHOT, + FRAMES_PER_SHOT, + index * SAMPLES_PER_SHOT, + SAMPLES_PER_SHOT, + ) + for index in range(count) + ] + + +def write_run(outputs, amplitudes=(0.1, 0.1, 0.4)): + """A stepped cut in a run directory, its shots in the run's manifest.""" + run_dir = outputs / RUN + write_cut(run_dir / "final" / "cut.mp4", amplitudes) + manifest = { + "steps": [ + { + "step": "join", + "files": ["final/cut.mp4"], + "shots": shots_of(len(amplitudes)), + } + ] + } + (run_dir / MANIFEST_FILE_NAME).write_text(json.dumps(manifest)) + + +@pytest.fixture +def server(tmp_path): + workflow_dir = tmp_path / "workflows" + workflow_dir.mkdir() + (tmp_path / "assets").mkdir() + + def make(script=success_script): + manager = JobManager( + str(tmp_path / "outputs"), + worker_manager=ScriptedWorkerManager(script), + history_path=str(tmp_path / "jobs.sqlite"), + ) + app = create_app( + workflow_dir=str(workflow_dir), + output_dir=str(tmp_path / "outputs"), + job_manager=manager, + asset_dir=str(tmp_path / "assets"), + ) + return TestClient(app, base_url="http://localhost") + + return make + + +def test_a_stepped_cut_answers_the_seam_finding(server, tmp_path): + with server() as client: + write_run(tmp_path / "outputs") + answer = client.get(f"/api/gallery/{CUT}/assess") + + assert answer.status_code == 200 + body = answer.json() + assert body["name"] == CUT + assert body["source"] == "output" + assert body["kind"] == "video" + assert body["shots_source"] == "manifest" + steps = [f for f in body["findings"] if f["rule"] == "seam_level_step"] + assert [f["at"]["seam"] for f in steps] == [2] + for found in body["findings"]: + assert set(found) == {"rule", "severity", "at", "value", "threshold", "says"} + assert {"seam_level_step", "shot_level_spread", "sync_drift"} <= set( + body["rules_applied"] + ) + assert body["not_applicable"] == {} + assert "probes" not in body + + +def test_detail_adds_every_probes_full_body(server, tmp_path): + with server() as client: + write_run(tmp_path / "outputs") + body = client.get(f"/api/gallery/{CUT}/assess?detail=true").json() + + assert set(body["probes"]) == { + "analyze_shots", + "analyze_seams", + "analyze_sync_drift", + } + assert len(body["probes"]["analyze_seams"]["seams"]) == 2 + + +def test_one_probe_answers_its_full_body(server, tmp_path): + with server() as client: + write_run(tmp_path / "outputs") + body = client.get(f"/api/gallery/{CUT}/assess?probe=analyze_seams").json() + + assert body["probe"] == "analyze_seams" + assert len(body["seams"]) == 2 + assert body["seams"][1]["level_step_db"] > 3.0 + assert "findings" in body and "rules_applied" in body + + +def test_an_unknown_probe_is_refused_before_the_name_is_read(server): + with server() as client: + # the file does not exist: the whitelist answers first + answer = client.get("/api/gallery/missing.mp4/assess?probe=analyze_vibes") + + assert answer.status_code == 400 + detail = answer.json()["detail"] + for probe in ("analyze_shots", "analyze_seams", "analyze_sync_drift"): + assert probe in detail + + +def test_a_still_is_not_applicable_to_any_probe(server, tmp_path): + with server() as client: + outputs = tmp_path / "outputs" + outputs.mkdir(exist_ok=True) + Image.new("RGB", (8, 8)).save(outputs / "still.png") + body = client.get("/api/gallery/still.png/assess").json() + + assert body["kind"] == "image" + assert body["findings"] == [] + assert set(body["not_applicable"]) == { + "analyze_shots", + "analyze_seams", + "analyze_sync_drift", + } + assert "still" in body["not_applicable"]["analyze_seams"] + + +def test_a_file_with_no_recorded_shots_says_seams_do_not_apply(server, tmp_path): + with server() as client: + write_cut(tmp_path / "outputs" / "loose.mp4", (0.1, 0.4)) + body = client.get("/api/gallery/loose.mp4/assess").json() + + assert body["shots_source"] == "none" + assert "analyze_seams" in body["not_applicable"] + assert "analyze_shots" not in body["not_applicable"] + + +def test_an_asset_is_assessed_with_the_shots_kept_beside_it(server, tmp_path): + with server() as client: + assets = tmp_path / "assets" + write_cut(assets / "episode.mp4", (0.1, 0.1, 0.4)) + record_kept_shots(str(assets), "episode.mp4", shots_of(3)) + body = client.get("/api/gallery/asset:episode.mp4/assess").json() + + assert body["source"] == "asset" + assert body["shots_source"] == "manifest" + assert [ + f["at"]["seam"] for f in body["findings"] if f["rule"] == "seam_level_step" + ] == [2] + + +def test_an_output_reference_resolves(server, tmp_path): + with server() as client: + write_run(tmp_path / "outputs") + answer = client.get(f"/api/gallery/output:{CUT}/assess") + + assert answer.status_code == 200 + assert answer.json()["name"] == CUT + + +def test_an_overrunning_shot_is_reported_once_not_per_probe(server, tmp_path): + # #427: analyze_shots, analyze_seams and analyze_sync_drift all raise the + # same shot_span_overrun finding (via the shared _shot_span_findings), so + # the compact merge listed one real overrun three times. + with server() as client: + run_dir = tmp_path / "outputs" / RUN + write_cut(run_dir / "final" / "cut.mp4", (0.1, 0.1, 0.4)) + overrunning = shots_of(3) + overrunning[-1]["num_samples"] += 1000 + manifest = { + "steps": [ + {"step": "join", "files": ["final/cut.mp4"], "shots": overrunning} + ] + } + (run_dir / MANIFEST_FILE_NAME).write_text(json.dumps(manifest)) + + body = client.get(f"/api/gallery/{CUT}/assess").json() + + overrun = [f for f in body["findings"] if f["rule"] == "shot_span_overrun"] + assert len(overrun) == 1 + assert overrun[0]["at"] == {"shot": "s2"} + + +def test_a_path_outside_the_outputs_is_refused(server): + with server() as client: + answer = client.get("/api/gallery/..%2Fsecret.mp4/assess") + + assert answer.status_code == 404 + + +def test_it_answers_while_a_job_holds_the_worker(server, tmp_path): + """The route is not a job: it answers while one is running, and queues + nothing behind it.""" + with server(hanging_script) as client: + write_run(tmp_path / "outputs") + running = client.post("/api/jobs", json={"workflow": valid_workflow()}).json() + wait_for_status(client, running["id"], ["running"]) + + answer = client.get(f"/api/gallery/{CUT}/assess") + jobs = client.get("/api/jobs").json() + + client.post(f"/api/jobs/{running['id']}/cancel") + + assert answer.status_code == 200 + assert answer.json()["findings"] + listed = jobs["jobs"] if isinstance(jobs, dict) else jobs + assert len(listed) == 1 diff --git a/tests/test_server_exports.py b/tests/test_server_exports.py index 95163a52..b73544c4 100644 --- a/tests/test_server_exports.py +++ b/tests/test_server_exports.py @@ -4,6 +4,7 @@ import io import json import os +import time import zipfile import pytest @@ -56,8 +57,10 @@ def exporting_script(command): json.dump( { "run_id": RUN_ID, + "version": 4, "status": "completed", "seed": 7, + "workflow": {"identity": "server_test"}, "steps": [{"step": "gen", "files": ["still.png"]}], }, file, @@ -66,6 +69,7 @@ def exporting_script(command): "type": "progress", "event": "run_start", "run_id": RUN_ID, + "version": 4, "identity": "server_test", "run_dir": RUN_DIR, } @@ -282,6 +286,8 @@ def test_the_readme_names_the_job_and_says_how_to_run_it(self, server): assert job_id in readme assert "python -m dw.run workflow.json" in readme assert "Git LFS" in readme + # which run, in the form the gallery labels it + assert f"`{RUN_ID}` - version 4" in readme def test_the_job_s_own_asset_dir_is_used_not_the_export_s_workspace( self, server, workspace_root @@ -347,6 +353,69 @@ def test_a_second_export_without_overwrite_is_409(self, server): forced = client.post(f"/api/jobs/{job_id}/export?overwrite=true") assert forced.status_code == 201 + def test_auth_required_reflects_whether_a_token_is_configured( + self, workspace_root, tmp_path + ): + # #353: an MCP-only agent has no way to attach a bearer token to a + # fetch on the person's behalf, so export_job's `next` hint branches + # on this field rather than assuming the zip is open to fetch. + manager = JobManager( + workspace_root.outputs, + worker_manager=ScriptedWorkerManager(exporting_script), + history_path=str(tmp_path / "jobs.sqlite"), + workflow_dir=workspace_root.workflows, + ) + app = create_app( + workflow_dir=workspace_root.workflows, + output_dir=workspace_root.outputs, + job_manager=manager, + prompt_dir=workspace_root.prompts, + asset_dir=workspace_root.assets, + workspace=workspace_root.root, + token="s3cr3t", + ) + with TestClient(app, base_url="http://localhost") as client: + headers = {"Authorization": "Bearer s3cr3t"} + submitted = client.post( + "/api/jobs", + json={"workflow": valid_workflow(), "arguments": {}}, + headers=headers, + ).json() + deadline = time.time() + 5.0 + detail = None + while time.time() < deadline: + detail = client.get( + f"/api/jobs/{submitted['id']}", headers=headers + ).json() + if detail["status"] in TERMINAL_STATES: + break + time.sleep(0.02) + assert detail["status"] in TERMINAL_STATES + body = client.post( + f"/api/jobs/{submitted['id']}/export", headers=headers + ).json() + + assert body["auth_required"] is True + + def test_an_absolute_zip_url_is_added_when_a_public_url_is_configured( + self, server, monkeypatch + ): + monkeypatch.setenv("DW_PUBLIC_URL", "https://dw.example.com") + with server() as client: + job_id = finished(client) + body = client.post(f"/api/jobs/{job_id}/export").json() + + assert ( + body["absolute_zip_url"] == f"https://dw.example.com/exports/{job_id}.zip" + ) + + def test_no_absolute_zip_url_when_no_public_url_is_configured(self, server): + with server() as client: + job_id = finished(client) + body = client.post(f"/api/jobs/{job_id}/export").json() + + assert "absolute_zip_url" not in body + class TestExportWithoutAWorkspace: def test_a_server_with_no_workspace_root_answers_409_not_a_crash( @@ -376,6 +445,10 @@ def test_it_lists_the_same_entries_as_the_directory(self, server): response = client.get(f"/exports/{job_id}.zip") assert response.status_code == 200 + # the saved file says which run it is; the URL and the entries + # inside keep the job id + disposition = response.headers["content-disposition"] + assert f"server_test-v4-{job_id}.zip" in disposition archive = zipfile.ZipFile(io.BytesIO(response.content)) assert sorted(archive.namelist()) == sorted( f"{job_id}/{entry['path']}" for entry in body["files"] diff --git a/tests/test_server_guides.py b/tests/test_server_guides.py index 570ca4b5..76ea607e 100644 --- a/tests/test_server_guides.py +++ b/tests/test_server_guides.py @@ -193,6 +193,23 @@ def test_the_tasks_guide_indexes_speech_generation(self): assert "Speech Generation" in tasks["sections"] + def test_the_assessment_section_states_the_procedure_and_authority(self): + """#388: assess_output's guide section - procedure, findings shape, + the authority rule and the remediation table - is reachable by name, + and names every rule the table in dw/assessment_rules.py holds.""" + from dw.assessment_rules import RULES + + guide = guides.get_guide("workflows", section="Assessing a run's output") + content = guide["content"] + + assert guide["section"] == "Assessing a run's output" + for word in ("`assess_output`", "not a verdict", "recut", "regenerate"): + assert word in content, word + for field in ("rule", "severity", "at", "value", "threshold", "says"): + assert field in content, field + for rule in RULES: + assert f"`{rule['name']}`" in content, rule["name"] + def test_the_authoring_section_is_reachable_by_name(self): guide = guides.get_guide( "workflows", section="authoring-a-workflow-from-an-agent" diff --git a/tests/test_server_info.py b/tests/test_server_info.py index 68e28f00..c878e946 100644 --- a/tests/test_server_info.py +++ b/tests/test_server_info.py @@ -9,6 +9,7 @@ from dw.server.app import create_app from dw.server.jobs import JobManager from dw.server import netinfo +from dw.workspace import Workspace, create_workspace from tests.test_server import ScriptedWorkerManager, success_script @@ -99,9 +100,54 @@ def test_prompt_dir_may_be_absent(tmp_path): assert c.get("/api/server").json()["directories"]["prompts"] is None +def test_directories_are_scoped_to_the_requested_workspace(tmp_path): + """#389: a mounted download_output confines a write against + directories.workspace, so this route has to answer per the ?workspace= + a caller (or the client's session pin) actually names, not the server's + own default - a caller pinned to a named workspace was writing into the + default workspace's tree with no error.""" + root = Workspace(tmp_path / "studio", "flag").ensure() + named = create_workspace(root, "session-a") + manager = JobManager( + root.outputs, + worker_manager=ScriptedWorkerManager(success_script), + history_path=str(tmp_path / "jobs.sqlite"), + workflow_dir=root.workflows, + ) + app = create_app( + workflow_dir=root.workflows, + output_dir=root.outputs, + prompt_dir=root.prompts, + asset_dir=root.assets, + job_manager=manager, + workspace=root.root, + ) + with TestClient(app, base_url="http://localhost") as c: + default_directories = c.get("/api/server").json()["directories"] + scoped_directories = c.get("/api/server?workspace=session-a").json()[ + "directories" + ] + + assert default_directories["workspace"] == root.root + assert default_directories["workflows"] == root.workflows + assert default_directories["assets"] == root.assets + assert default_directories["outputs"] == root.outputs + assert default_directories["prompts"] == root.prompts + + assert scoped_directories["workspace"] == named.root + assert scoped_directories["workflows"] == named.workflows + assert scoped_directories["assets"] == named.assets + assert scoped_directories["outputs"] == named.outputs + assert scoped_directories["prompts"] == named.prompts + + assert scoped_directories["workspace"] != default_directories["workspace"] + + def test_auth_required_and_token_never_disclosed(tmp_path): token = "s3cr3t-token-value" with client(tmp_path, token=token) as c: + # gated like every other API route + assert c.get("/api/server").status_code == 401 response = c.get("/api/server", headers={"Authorization": f"Bearer {token}"}) assert response.status_code == 200 body = response.json() @@ -127,15 +173,6 @@ def numbers(value): assert len(token) not in numbers(body) -def test_requires_the_token_like_every_other_api_route(tmp_path): - with client(tmp_path, token="abc123") as c: - assert c.get("/api/server").status_code == 401 - assert ( - c.get("/api/server", headers={"Authorization": "Bearer abc123"}).status_code - == 200 - ) - - def test_mcp_mounted_reported(tmp_path): pytest.importorskip("mcp", reason="the mcp extra is not installed") with client(tmp_path, mcp=True) as c: @@ -175,12 +212,24 @@ def test_netinfo_falls_back_to_stdlib_without_psutil(monkeypatch): def no_psutil(): raise ImportError("no psutil") + import socket + + def fake_getaddrinfo(host, port): + return [ + (socket.AF_INET6, socket.SOCK_STREAM, 0, "", ("2001:db8::5%eth0", 0, 0, 0)), + (socket.AF_INET, socket.SOCK_STREAM, 0, "", ("127.0.0.1", 0)), + (socket.AF_INET, socket.SOCK_STREAM, 0, "", ("192.168.1.50", 0)), + ] + monkeypatch.setattr(netinfo, "_psutil_addresses", no_psutil) - entries = netinfo.local_addresses() - assert isinstance(entries, list) - for entry in entries: - assert entry["interface"] is None - assert entry["family"] in ("IPv4", "IPv6") + monkeypatch.setattr(netinfo.socket, "getaddrinfo", fake_getaddrinfo) + # the outbound probe finds an address getaddrinfo already had: reported once + monkeypatch.setattr(netinfo, "_outbound_address", lambda: "192.168.1.50") + + assert netinfo.local_addresses() == [ + {"address": "192.168.1.50", "family": "IPv4", "interface": None}, + {"address": "2001:db8::5", "family": "IPv6", "interface": None}, + ] def test_usable_filters(monkeypatch): diff --git a/tests/test_server_jobs.py b/tests/test_server_jobs.py index d5f4d2ef..17a90a61 100644 --- a/tests/test_server_jobs.py +++ b/tests/test_server_jobs.py @@ -28,6 +28,7 @@ def tracked_script(command): "run_id": RUN_ID, "identity": "server_test", "run_dir": RUN_DIR, + "version": 4, } yield {"type": "success", "message": "ok", "run_count": 1, "manifest": []} @@ -53,12 +54,41 @@ def finished_job(manager): return job +def test_submit_checks_content_type_against_the_callers_arguments(manager): + """A document-default 'text/html' content_type that the caller's own + argument overrides to 'text/plain' must queue - JobManager.submit used + to validate the unsubstituted document (loaded.validate(), no + arguments), refusing a run that validate_workflow had already accepted + for the same call (#415, the run_workflow mirror of #414).""" + definition = { + "id": "se-415", + "variables": {"ct": "text/html"}, + "steps": [ + { + "name": "t", + "task": {"command": "compose_text", "arguments": {"parts": ["x"]}}, + "result": {"content_type": "variable:ct"}, + } + ], + } + job = manager.submit( + workflow=definition, arguments={"ct": "text/plain"}, base_dir=None + ) + deadline = time.time() + 5 + while job.status not in TERMINAL_STATES and time.time() < deadline: + time.sleep(0.01) + assert job.status == "succeeded", job.error + + def test_run_start_populates_the_job(manager): job = finished_job(manager) assert job.run_id == RUN_ID assert job.run_dir == RUN_DIR assert job.summary()["run_id"] == RUN_ID assert job.detail()["run_dir"] == RUN_DIR + # the ordinal the gallery shows for this run's files + assert job.run_version == 4 + assert job.summary()["run_version"] == 4 def test_both_persist_and_read_back(manager): @@ -66,6 +96,10 @@ def test_both_persist_and_read_back(manager): historical = manager.history.get(job.id) assert historical["run_id"] == RUN_ID assert historical["run_dir"] == RUN_DIR + assert historical["run_version"] == 4 + # and in the polled list, not only the detail + (summary,) = manager.history.recent_summaries() + assert summary["run_version"] == 4 def test_realized_reads_the_file_the_run_wrote(manager, tmp_path): @@ -167,8 +201,8 @@ def for_each_script(command): command.get("workflow_dir"), ) # Mirrors dw/worker.py's _handle_execute: validated against the - # defaults first, then run() substitutes and expands the real arguments. - workflow.validate() + # caller's own arguments (#415), which run() then substitutes and expands. + workflow.validate(arguments=command["arguments"]) workflow.run(command["arguments"], {}) yield { "type": "success", diff --git a/tests/test_server_workspaces.py b/tests/test_server_workspaces.py index 9fd381a7..872c7a2c 100644 --- a/tests/test_server_workspaces.py +++ b/tests/test_server_workspaces.py @@ -2,6 +2,7 @@ root, each with its own workflows, assets and outputs, all sharing the one prompt library.""" +import json import os import pytest @@ -251,10 +252,6 @@ def test_saving_a_workflow_with_a_relative_sub_workflow_path( ) assert response.status_code == 200 - def test_an_unknown_workspace_query_param_is_a_404(self, server): - with server() as client: - assert client.get("/api/workflows?workspace=nope").status_code == 404 - def test_a_traversal_attempt_as_a_workspace_name_is_a_400(self, server): with server() as client: response = client.get("/api/workflows", params={"workspace": "../x"}) @@ -393,14 +390,16 @@ def test_asset_listing_entries_carry_a_fetchable_url(self, server, workspace_roo assert fetched.status_code == 200 assert fetched.content == b"iris" - def test_get_workflow_reports_its_origin_and_writability( - self, server, workspace_root - ): + def test_get_workflow_reports_its_origin_and_writability(self, server): + # a named workspace's own library is as writable as the default's - + # the headers follow the workspace the route was scoped to with server() as client: + client.post("/api/workspaces", json={"name": "shots"}) client.put( - "/api/workflows/Basic", json={"workflow": valid_workflow("mine")} + "/api/workflows/Basic?workspace=shots", + json={"workflow": valid_workflow("mine")}, ) - response = client.get("/api/workflows/Basic") + response = client.get("/api/workflows/Basic?workspace=shots") assert response.status_code == 200 assert response.headers["x-workflow-origin"] == "workspace" assert response.headers["x-workflow-writable"] == "true" @@ -448,8 +447,28 @@ def test_it_links_rather_than_copying_when_it_can(self, server, workspace_root): "/api/assets/keep", json={"name": "Gyre/run/clip.mp4"} ).json() assert body["reference"] == "asset:clip.mp4" - if body["linked"]: - assert os.stat(source).st_ino == os.stat(body["path"]).st_ino + # outputs and assets share one temporary filesystem, so a link is + # always possible here + assert body["linked"] is True + assert os.stat(source).st_ino == os.stat(body["path"]).st_ino + + def test_it_copies_when_it_cannot_link(self, server, workspace_root, monkeypatch): + """A different filesystem, or one with no links, still keeps the + output - as a separate copy of the same bytes.""" + source = self.written(workspace_root.outputs, "Gyre/run/clip.mp4", b"clip") + + def no_links(*args, **kwargs): + raise OSError("cross-device link") + + monkeypatch.setattr(os, "link", no_links) + with server() as client: + body = client.post( + "/api/assets/keep", json={"name": "Gyre/run/clip.mp4"} + ).json() + assert body["linked"] is False + assert os.stat(source).st_ino != os.stat(body["path"]).st_ino + with open(body["path"], "rb") as kept: + assert kept.read() == b"clip" def test_the_name_defaults_to_the_files_own(self, server, workspace_root): self.written(workspace_root.outputs, "Gyre/run/still.png") @@ -498,6 +517,58 @@ def test_a_destination_cannot_leave_the_library( ) assert response.status_code == 400 + def test_a_kept_outputs_shots_survive_and_report_through_the_gallery( + self, server, workspace_root + ): + """#393: keeping a cut copied only its bytes, so a joined video's shot + boundaries were unreachable from the asset it became - the gallery + metadata for a kept asset carried no `shots` at all, where the same + file's metadata as an output did. keep_output now carries the run's + recorded shots into a manifest sidecar beside the asset, which is the + same convention `shots_beside` (and so every assessment probe) already + reads.""" + from .test_media_info import write_mp4 + + run_dir = os.path.join(workspace_root.outputs, "Gyre/20260905-101500-aaaaaaaa") + os.makedirs(run_dir, exist_ok=True) + write_mp4(os.path.join(run_dir, "cut.mp4"), frames=18, fps=6) + shots = [ + { + "name": "a", + "start_frame": 0, + "num_frames": 10, + "start_sample": 0, + "num_samples": 100, + }, + { + "name": "b", + "start_frame": 10, + "num_frames": 8, + "start_sample": 100, + "num_samples": 80, + }, + ] + manifest = {"steps": [{"step": "concat", "files": ["cut.mp4"], "shots": shots}]} + with open(os.path.join(run_dir, "manifest.json"), "w") as handle: + json.dump(manifest, handle) + + with server() as client: + kept = client.post( + "/api/assets/keep", + json={ + "name": "Gyre/20260905-101500-aaaaaaaa/cut.mp4", + "asset_name": "qa-cast/cut.mp4", + }, + ) + assert kept.status_code == 201 + + metadata = client.get("/api/gallery/asset:qa-cast/cut.mp4/metadata").json() + assert metadata["source"] == "asset" + assert metadata["media"]["shots"] == shots + + sidecar = os.path.join(workspace_root.assets, "qa-cast", "manifest.json") + assert os.path.isfile(sidecar) + def test_keeping_stays_inside_the_workspace(self, server, workspace_root): """The source is read from the named workspace's outputs and the copy lands in its assets - neither reaches the default workspace.""" @@ -642,23 +713,6 @@ def test_a_name_cannot_leave_the_library(self, server, workspace_root, name): class TestRunning: - def test_a_job_runs_in_the_workspace_it_named(self, server, workspace_root): - with server() as client: - client.post("/api/workspaces", json={"name": "shots"}) - client.put( - "/api/workflows/Mine?workspace=shots", - json={"workflow": valid_workflow("mine")}, - ) - response = client.post( - "/api/jobs", json={"workflow_path": "Mine", "workspace": "shots"} - ) - assert response.status_code == 201 - detail = wait_for_status( - client, response.json()["id"], {"succeeded", "failed"} - ) - - assert detail["status"] == "succeeded" - def test_enhance_runs_in_the_selected_workspace(self, server): """The enhance job used to be submitted unscoped, so its text landed in the default workspace's outputs while the editor read it back @@ -710,10 +764,14 @@ def test_rerun_stays_in_the_workspace_it_ran_in(self, server, workspace_root): "/api/workflows/Mine?workspace=shots", json={"workflow": valid_workflow("mine")}, ) - original = client.post( + submitted = client.post( "/api/jobs", json={"workflow_path": "Mine", "workspace": "shots"} - ).json() - wait_for_status(client, original["id"], {"succeeded", "failed"}) + ) + # a stored workflow found only in the named workspace runs there + assert submitted.status_code == 201 + original = submitted.json() + detail = wait_for_status(client, original["id"], {"succeeded", "failed"}) + assert detail["status"] == "succeeded" rerun = client.post(f"/api/jobs/{original['id']}/rerun") assert rerun.status_code == 201 diff --git a/tests/test_shot_span_preflight.py b/tests/test_shot_span_preflight.py new file mode 100644 index 00000000..e62f933d --- /dev/null +++ b/tests/test_shot_span_preflight.py @@ -0,0 +1,124 @@ +"""A `shots` argument to an assessment probe reaching past a +statically-knowable video's real length, warned about at validate time +rather than only at run time - #425. + +Exercises the real decode path (`probe_media` against a genuine mp4 file) +rather than a mock of it, mirroring tests/test_slice_preflight.py (#402). +""" + +import os +import tempfile + +from dw.runs import activate_output_root, deactivate_output_root +from dw.shot_span_preflight import shot_span_warnings +from dw.workflow import workflow_from_definition + +from tests.test_assess import write_mp4 + + +def workflow_dir_with_asset(monkeypatch, name, frames): + base_dir = tempfile.mkdtemp() + asset_dir = os.path.join(base_dir, "assets") + os.makedirs(asset_dir) + write_mp4(os.path.join(asset_dir, name), frames=frames, fps=24) + monkeypatch.setenv("DW_ASSET_DIR", asset_dir) + return base_dir + + +def seams_workflow(video, shots): + return { + "id": "check", + "steps": [ + { + "name": "seams", + "task": { + "command": "analyze_seams", + "arguments": {"video": video, "shots": shots}, + }, + "result": {"content_type": "application/json"}, + } + ], + } + + +class TestTheCheck: + def test_a_shots_record_past_a_short_asset_is_warned(self, monkeypatch): + # Mirrors the issue's own repro shape: a shot reaching well past the + # file's real frame count. + base_dir = workflow_dir_with_asset(monkeypatch, "clip.mp4", frames=248) + definition = seams_workflow( + "asset:clip.mp4", + [ + {"name": "a", "start_frame": 0, "num_frames": 124}, + {"name": "b", "start_frame": 124, "num_frames": 300}, + ], + ) + + warnings = shot_span_warnings(definition, base_dir=base_dir) + + assert len(warnings) == 1 + assert "'b'" in warnings[0] + assert "176 past the file's 248 frames" in warnings[0] + + def test_shots_within_the_source_validate_clean(self, monkeypatch): + base_dir = workflow_dir_with_asset(monkeypatch, "clip.mp4", frames=248) + definition = seams_workflow( + "asset:clip.mp4", + [ + {"name": "a", "start_frame": 0, "num_frames": 124}, + {"name": "b", "start_frame": 124, "num_frames": 124}, + ], + ) + + assert shot_span_warnings(definition, base_dir=base_dir) == [] + + def test_a_previous_result_video_is_left_to_the_run(self): + definition = seams_workflow( + "previous_result:make_cut", + [{"name": "a", "start_frame": 0, "num_frames": 300}], + ) + + assert shot_span_warnings(definition) == [] + + def test_an_output_reference_too_short_is_warned(self): + output_root = tempfile.mkdtemp() + run_dir = os.path.join(output_root, "cut", "20260101-000000-abc") + os.makedirs(run_dir) + write_mp4(os.path.join(run_dir, "final.mp4"), frames=48, fps=24) + definition = seams_workflow( + "output:cut/20260101-000000-abc/final.mp4", + [{"name": "a", "start_frame": 0, "num_frames": 300}], + ) + + token = activate_output_root(output_root) + try: + warnings = shot_span_warnings(definition) + finally: + deactivate_output_root(token) + + assert len(warnings) == 1 + assert "final.mp4" not in warnings[0] + assert "252 past the file's 48 frames" in warnings[0] + + def test_nothing_is_reported_for_a_definition_with_no_probe_step(self): + assert shot_span_warnings({"steps": [{"name": "a", "task": {}}]}) == [] + + +class TestWiredIntoTheWorkflow: + def test_reachable_from_the_workflow_method(self, monkeypatch): + base_dir = workflow_dir_with_asset(monkeypatch, "clip.mp4", frames=248) + definition = seams_workflow( + "asset:clip.mp4", + [ + {"name": "a", "start_frame": 0, "num_frames": 124}, + {"name": "b", "start_frame": 124, "num_frames": 300}, + ], + ) + workflow = workflow_from_definition( + definition, os.path.join(base_dir, "workflow.json") + ) + + warnings = workflow.shot_span_warnings() + + assert len(warnings) == 1 + assert "'b'" in warnings[0] diff --git a/tests/test_shots.py b/tests/test_shots.py new file mode 100644 index 00000000..1a0aee0d --- /dev/null +++ b/tests/test_shots.py @@ -0,0 +1,1104 @@ +""" +Unit tests for shot boundaries (#385): where each input landed on a video a +step joined from several, recorded on the `AudioVideo` a join returns +(`dw/shots.py`), carried into the step's manifest entry and read back by +`dw.runs.recorded_shots`. + +Covers: every `AudioVideo(` constructor site in `dw/` is accounted for by an +explicit decision (populates / carries / rescales / remeasures / none); +`concat_videos`, `dissolve_videos` and `run_chain` populate shots that +partition their output; a track measured longer than the frames imply is +recorded as measured, not derived; `pair_audio` re-measures the sample side +against a new track; `slice_audio` drops shots entirely (its output is audio, +which has no picture to partition); `rescaled_shots` is exercised directly for +`interpolate_frames`; and a join's shots survive a save/manifest/ +`recorded_shots` round trip. +""" + +import ast +import json +import os +from unittest.mock import patch + +import numpy +from PIL import Image + +from dw.pipeline_processors.chain import run_chain +from dw.result import AudioVideo, Result +from dw.runs import MANIFEST_FILE_NAME, recorded_shots +from dw.shots import ( + carried_shots, + named_shots, + remeasured_shots, + rescaled_shots, + shot_reference_names, + shot_record, + shots_for_file, + step_shots, +) +from dw.tasks.audio_utils import frames_to_samples, slice_audio +from dw.tasks.concat_videos import concat_videos +from dw.tasks.dissolve_videos import dissolve_videos +from dw.tasks.pair_audio import pair_audio + +REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +DW_ROOT = os.path.join(REPO_ROOT, "dw") + + +def frames(count, color=(0, 0, 0)): + return [Image.new("RGB", (4, 4), color) for _ in range(count)] + + +def audio_video(num_frames, level, fps=4, sample_rate=100): + samples = int(num_frames / fps * sample_rate) + audio = numpy.full((2, samples), float(level), dtype=numpy.float32) + return AudioVideo(frames(num_frames), audio, sample_rate, fps=fps) + + +# --------------------------------------------------------------------------- +# 1. Every AudioVideo(...) construction site in dw/ is accounted for +# --------------------------------------------------------------------------- + +# (relative path, enclosing function name) -> (decision, expected call count) +# +# A decision is one of: +# "populates" - builds a fresh shots list for a video it joined +# "carries" - copies an unchanged input's shots across (carried_shots) +# "rescales" - stretches an input's shots to a new frame count (rescaled_shots) +# "remeasures" - keeps the frame side, re-measures the sample side (remeasured_shots) +# "none" - the video is not joined from named inputs; no shots kwarg at all +EXPECTED_SITES = { + ("dw/tasks/concat_videos.py", "concat_videos"): ("populates", 1), + ("dw/tasks/dissolve_videos.py", "dissolve_videos"): ("populates", 1), + ("dw/pipeline_processors/chain.py", "run_chain"): ("populates", 2), + ("dw/tasks/task.py", "_per_frame"): ("carries", 1), + ("dw/tasks/stabilize.py", "stabilize_video"): ("carries", 1), + ("dw/tasks/interpolate_frames.py", "interpolate_frames"): ("rescales", 1), + ("dw/tasks/pair_audio.py", "pair_audio"): ("remeasures", 1), + ("dw/tasks/video_utils.py", "_decode_audio_video"): ("none", 1), + ("dw/result.py", "pair_audio_with_frames"): ("none", 1), +} + +_SHOTS_HELPER_BY_DECISION = { + "populates": None, # builds its own list - no single shared helper + "carries": "carried_shots", + "rescales": "rescaled_shots", + "remeasures": "remeasured_shots", +} + + +def _iter_python_files(root): + for dirpath, _dirnames, filenames in os.walk(root): + for name in filenames: + if name.endswith(".py"): + yield os.path.join(dirpath, name) + + +def _enclosing_function(tree, call_node): + """The innermost function/method def that contains `call_node`, or None.""" + best = None + for node in ast.walk(tree): + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + if node.lineno <= call_node.lineno and getattr( + node, "end_lineno", node.lineno + ) >= getattr(call_node, "end_lineno", call_node.lineno): + if best is None or node.lineno > best.lineno: + best = node + return best.name if best else None + + +def _is_audio_video_call(call_node): + func = call_node.func + if isinstance(func, ast.Name): + return func.id == "AudioVideo" + if isinstance(func, ast.Attribute): + return func.attr == "AudioVideo" + return False + + +def _find_audio_video_calls(): + """Every `AudioVideo(...)` call in dw/, as (relative path, function, node).""" + found = [] + for path in _iter_python_files(DW_ROOT): + with open(path, encoding="utf-8") as handle: + source = handle.read() + try: + tree = ast.parse(source, filename=path) + except SyntaxError: + continue + relative = os.path.relpath(path, REPO_ROOT).replace(os.sep, "/") + for node in ast.walk(tree): + if isinstance(node, ast.Call) and _is_audio_video_call(node): + function_name = _enclosing_function(tree, node) + found.append((relative, function_name, node)) + return found + + +def test_every_audio_video_constructor_site_is_decided(): + """Every `AudioVideo(` call site in dw/ is one of the sites this module + decided for (dw/shots.py's own module docstring: "tests/test_shots.py + fails on a constructor site nobody decided for.").""" + calls = _find_audio_video_calls() + + counts = {} + for relative, function_name, _node in calls: + counts[(relative, function_name)] = counts.get((relative, function_name), 0) + 1 + + unexpected = sorted(set(counts) - set(EXPECTED_SITES)) + assert not unexpected, ( + "AudioVideo(...) constructed at a site with no recorded decision - " + "decide for it in tests/test_shots.py and dw/shots.py: " + f"{unexpected}" + ) + + missing = sorted(set(EXPECTED_SITES) - set(counts)) + assert not missing, ( + "A previously-decided AudioVideo(...) site has disappeared - update " + f"EXPECTED_SITES in tests/test_shots.py: {missing}" + ) + + for site, (_decision, expected_count) in EXPECTED_SITES.items(): + assert counts[site] == expected_count, ( + f"{site} now constructs AudioVideo {counts[site]} time(s), " + f"expected {expected_count}" + ) + + +def test_every_non_none_site_passes_shots_explicitly(): + """A site that decided its video is (or is not) joined from named inputs + says so with an explicit `shots=` keyword - "none" is the one decision + that is allowed to omit it entirely.""" + calls = _find_audio_video_calls() + for relative, function_name, node in calls: + decision, _count = EXPECTED_SITES[(relative, function_name)] + keyword_names = {kw.arg for kw in node.keywords if kw.arg is not None} + if decision == "none": + continue + assert "shots" in keyword_names, ( + f"{relative}:{node.lineno} ({function_name}, decision={decision!r}) " + "does not pass shots= explicitly" + ) + + +# --------------------------------------------------------------------------- +# 2 & 3. concat_videos populates shots that partition its output, and an +# overrun is measured rather than derived +# --------------------------------------------------------------------------- + + +class TestConcatVideosShots: + def test_shots_partition_frames_and_samples(self): + videos = [audio_video(4, 1), audio_video(6, 2), audio_video(5, 3)] + + result = concat_videos(videos, fps=4) + + assert [shot["name"] for shot in result.shots] == [ + "video 1", + "video 2", + "video 3", + ] + starts = [shot["start_frame"] for shot in result.shots] + counts = [shot["num_frames"] for shot in result.shots] + assert starts == [0, 4, 10] + assert sum(counts) == len(result.frames) + + sample_starts = [shot["start_sample"] for shot in result.shots] + sample_counts = [shot["num_samples"] for shot in result.shots] + assert sample_starts[0] == 0 + assert sum(sample_counts) == result.audio.shape[1] + + def test_shot_reference_names_name_shot_at_members(self): + videos = [audio_video(4, 1), audio_video(4, 2)] + + result = concat_videos(videos, fps=4) + named = named_shots( + result.shots, + shot_reference_names( + ["previous_result:shot@a.frames", "previous_result:shot@b.frames"] + ), + ) + + # The prefix strips only "previous_result:" - the member keeps its + # "shot@" marker, which is how a for_each member is named elsewhere + assert [shot["name"] for shot in named] == ["shot@a", "shot@b"] + + def test_shot_reference_names_name_an_ordinary_step(self): + """A `previous_result:` reference that does not name a `shot@` + for_each member still names a shot - after the step it points at, + so a shot from an ordinary step (a `pair_audio`, a chain) is + traceable rather than falling back to "video N" (#396).""" + videos = [audio_video(4, 1), audio_video(4, 2)] + + result = concat_videos(videos, fps=4) + named = named_shots( + result.shots, + shot_reference_names( + ["asset:ep31-shot1-return.mp4", "previous_result:shot2d"] + ), + ) + + # position 0 keeps whatever name the join itself gave it - only the + # previous_result reference at position 1 is renamed + assert [shot["name"] for shot in named] == ["video 1", "shot2d"] + + def test_no_audio_input_leaves_sample_fields_none(self): + result = concat_videos([frames(4), frames(3)]) + + assert result.shots is not None + for shot in result.shots: + assert shot["start_sample"] is None + assert shot["num_samples"] is None + + def test_asset_literal_shots_are_named_by_file_not_absolute_path(self): + """An `asset:`/`output:` reference is resolved to its absolute + server path before the task ever runs (dw/workflow.py's + realize_args), so naming a shot by the string the join received + leaked that path onto every consumer of the shots - the manifest, + get_gallery_metadata, and the seams route's labels (#390). Only the + file name means anything off this box.""" + resolved = ( + "/home/don/diffusers-workspace/common/assets/qa-cast/ep3-shot1-incident.mp4" + ) + videos = [resolved, audio_video(4, 2)] + + with patch( + "dw.tasks.concat_videos.load_audio_video", return_value=audio_video(4, 1) + ): + result = concat_videos(videos, fps=4) + + assert result.shots[0]["name"] == "ep3-shot1-incident.mp4" + assert "/" not in result.shots[0]["name"] + + def test_an_overrun_track_is_measured_not_derived(self): + """One input's audio runs 267 samples longer than its frames alone + would imply - concat_videos records the measured length of the + joined track, not a count derived from frame/fps arithmetic.""" + fps, sample_rate = 4, 100 + first = audio_video(4, 1, fps=fps, sample_rate=sample_rate) + second = audio_video(4, 2, fps=fps, sample_rate=sample_rate) + overrun = 267 + second.audio = numpy.concatenate( + [ + second.audio, + numpy.full((2, overrun), 2.0, dtype=numpy.float32), + ], + axis=1, + ) + + result = concat_videos([first, second], fps=fps) + + frame_derived_samples = int(4 / fps * sample_rate) + second_shot = result.shots[1] + assert second_shot["num_samples"] == frame_derived_samples + overrun + assert second_shot["num_samples"] == result.audio.shape[1] - int( + 4 / fps * sample_rate + ) + + def test_an_inner_input_nests_its_own_shots(self): + """#399: a video that is itself an earlier join's output carries its + own `.shots` - concat_videos flattens those into the joined output, + offset onto where the whole input landed, rather than collapsing + them to one record for the whole file.""" + fps, sample_rate = 4, 100 + inner_shots = [ + shot_record("shot@accuse", 0, 4, start_sample=0, num_samples=100), + shot_record("shot@deflect", 4, 4, start_sample=100, num_samples=100), + ] + nested = AudioVideo( + frames(8), + numpy.full((2, 200), 1.0, dtype=numpy.float32), + sample_rate, + fps=fps, + shots=inner_shots, + ) + trailing = audio_video(4, 2, fps=fps, sample_rate=sample_rate) + + result = concat_videos([nested, trailing], fps=fps) + + names = [shot["name"] for shot in result.shots] + assert names == ["shot@accuse", "shot@deflect", "video 2"] + starts = [shot["start_frame"] for shot in result.shots] + assert starts == [0, 4, 8] + sample_starts = [shot["start_sample"] for shot in result.shots] + assert sample_starts[:2] == [0, 100] + + def test_a_trimmed_inner_input_clips_and_clears_its_shots(self): + """The same nested input, but trimmed as the second video - the + trim can cut into or through an inner shot; one entirely inside it + is dropped, one straddling it survives clipped and re-based, and + either way its sample fields are cleared because the crossfade + draws from the trimmed material (dw/shots.py's trimmed_shots).""" + fps, sample_rate = 4, 100 + inner_shots = [ + shot_record("shot@a", 0, 2, start_sample=0, num_samples=50), + shot_record("shot@b", 2, 6, start_sample=50, num_samples=150), + ] + nested = AudioVideo( + frames(8), + numpy.full((2, 200), 1.0, dtype=numpy.float32), + sample_rate, + fps=fps, + shots=inner_shots, + ) + leading = audio_video(4, 1, fps=fps, sample_rate=sample_rate) + + result = concat_videos([leading, nested], trim_frames=3, fps=fps) + + names = [shot["name"] for shot in result.shots] + assert names == ["video 1", "shot@b"] + trimmed = result.shots[1] + assert trimmed["start_frame"] == 4 + assert trimmed["num_frames"] == 5 + assert trimmed["start_sample"] is None + assert trimmed["num_samples"] is None + + +# --------------------------------------------------------------------------- +# 4. dissolve_videos +# --------------------------------------------------------------------------- + + +class TestDissolveVideosShots: + def test_shots_partition_frames_with_overlap_recorded(self): + result = dissolve_videos([frames(10), frames(10), frames(10)], 3) + + starts = [shot["start_frame"] for shot in result.shots] + counts = [shot["num_frames"] for shot in result.shots] + assert sum(counts) == len(result.frames) + assert starts[0] == 0 + assert starts[1] == starts[0] + counts[0] + assert starts[2] == starts[1] + counts[1] + + assert "overlap_frames" not in result.shots[0] + assert result.shots[1]["overlap_frames"] == 3 + assert result.shots[2]["overlap_frames"] == 3 + + def test_sample_fields_partition_the_track_when_audio_is_present(self): + def clip(level): + samples = 250 + audio = numpy.full((2, samples), float(level), dtype=numpy.float32) + return AudioVideo(frames(10), audio, 100, fps=4) + + result = dissolve_videos([clip(1), clip(2), clip(3)], 3, fps=4) + + sample_starts = [shot["start_sample"] for shot in result.shots] + sample_counts = [shot["num_samples"] for shot in result.shots] + assert sample_starts[0] == 0 + assert sum(sample_counts) == result.audio.shape[1] + + def test_asset_literal_shots_are_named_by_file_not_absolute_path(self): + """Same leak as concat_videos (#390): a resolved asset: reference + arrives here as an absolute server path, and only its file name + belongs on a consumer-facing shot name.""" + resolved = ( + "/home/don/diffusers-workspace/common/assets/qa-cast/ep3-shot2-reply.mp4" + ) + videos = [resolved, frames(10)] + + with patch( + "dw.tasks.dissolve_videos.load_audio_video", + return_value=frames(10), + ): + result = dissolve_videos(videos, 3) + + assert result.shots[0]["name"] == "ep3-shot2-reply.mp4" + assert "/" not in result.shots[0]["name"] + + def test_an_inner_input_nests_its_own_shots(self): + """#399: same fix as concat_videos - an input already carrying + `.shots` from an earlier join keeps its inner seams instead of + collapsing to a single record for the whole file. A dissolve maps a + clip's own frame j to joined frame frame_starts[index] + j exactly + (no head trim), so the offset is a straight add with no clipping.""" + fps, sample_rate = 4, 100 + inner_shots = [ + shot_record("shot@accuse", 0, 4, start_sample=0, num_samples=100), + shot_record("shot@deflect", 4, 4, start_sample=100, num_samples=100), + ] + nested = AudioVideo( + frames(8), + numpy.full((2, 200), 1.0, dtype=numpy.float32), + sample_rate, + fps=fps, + shots=inner_shots, + ) + leading = AudioVideo( + frames(10), + numpy.full((2, 250), 2.0, dtype=numpy.float32), + sample_rate, + fps=fps, + ) + + result = dissolve_videos([leading, nested], 3, fps=fps) + + names = [shot["name"] for shot in result.shots] + assert names == ["video 1", "shot@accuse", "shot@deflect"] + frame_offset = 10 - 3 # frame_starts[1] + starts = [shot["start_frame"] for shot in result.shots] + assert starts[1:] == [frame_offset, frame_offset + 4] + # overlap_frames marks the dissolve's head - the first nested + # sub-shot only, since the seam is between videos, not inside one + assert result.shots[1]["overlap_frames"] == 3 + assert "overlap_frames" not in result.shots[2] + + def test_a_nested_input_not_last_has_its_tail_shot_trimmed(self): + """#405: a non-nested video's shot already stops at + frame_starts[index + 1], excluding the frames the next video's + dissolve blends into - a nested video followed by another video must + get the same trim on its *last* inner shot, or that shot's + num_frames overlaps the next video's recorded start_frame. Only the + earlier test's case (nested video last) was covered before; this + pins the case where a nested video sits in the middle.""" + fps, sample_rate, dissolve_frames = 24, 44100, 12 + inner_shots = [ + shot_record("shot@x", 0, 112, start_sample=0, num_samples=205800), + shot_record( + "shot@receipt", 112, 124, start_sample=205800, num_samples=227850 + ), + ] + nested = AudioVideo( + frames(236), + numpy.full( + (2, frames_to_samples(236, fps, sample_rate)), 1.0, dtype=numpy.float32 + ), + sample_rate, + fps=fps, + shots=inner_shots, + ) + leading = AudioVideo( + frames(608), + numpy.full( + (2, frames_to_samples(608, fps, sample_rate)), 2.0, dtype=numpy.float32 + ), + sample_rate, + fps=fps, + ) + trailing = AudioVideo( + frames(124), + numpy.full( + (2, frames_to_samples(124, fps, sample_rate)), 3.0, dtype=numpy.float32 + ), + sample_rate, + fps=fps, + ) + + result = dissolve_videos([leading, nested, trailing], dissolve_frames, fps=fps) + + by_name = {shot["name"]: shot for shot in result.shots} + # frame_starts[1] = 608 - 12 = 596; the nested video's own inner + # shots land at 596 and 596 + 112 = 708 + assert by_name["shot@x"]["start_frame"] == 596 + assert by_name["shot@x"]["num_frames"] == 112 + assert by_name["shot@receipt"]["start_frame"] == 708 + # the un-trimmed inner shot was 124 frames; the dissolve into the + # trailing video eats the last 12 of them + assert by_name["shot@receipt"]["num_frames"] == 112 + # the next video's recorded start_frame picks up exactly where the + # trimmed shot ends - no overlap between the two records + frame_starts = [shot["start_frame"] for shot in result.shots] + assert frame_starts[-1] == by_name["shot@receipt"]["start_frame"] + 112 + + def test_a_nested_shots_sample_start_is_derived_not_rescaled(self): + """#405 secondary: an inner shot's start_sample used to be rescaled + from its own already-rounded stored value (native_rate -> + target_rate), which compounds rounding when the nested input was + itself the product of an earlier, already-rounded join. Passing fps + derives it from the shot's new frame position instead, the same + frames_to_samples rule dissolve_videos already uses for the + top-level seam (#401) - so a nested shot's recorded start_sample + always agrees with what pair_audio would independently measure for + the same frame boundary.""" + fps, sample_rate, dissolve_frames = 24, 44100, 12 + # An inner start_sample that is deliberately *not* what + # frames_to_samples would give for its native rate, simulating + # drift already compounded by an earlier join + inner_shots = [ + shot_record("shot@a", 0, 112, start_sample=0, num_samples=205799), + shot_record("shot@b", 112, 124, start_sample=205799, num_samples=227851), + ] + nested = AudioVideo( + frames(236), + numpy.full( + (2, frames_to_samples(236, fps, sample_rate)), 1.0, dtype=numpy.float32 + ), + sample_rate, + fps=fps, + shots=inner_shots, + ) + leading = AudioVideo( + frames(608), + numpy.full( + (2, frames_to_samples(608, fps, sample_rate)), 2.0, dtype=numpy.float32 + ), + sample_rate, + fps=fps, + ) + + result = dissolve_videos([leading, nested], dissolve_frames, fps=fps) + + by_name = {shot["name"]: shot for shot in result.shots} + expected = frames_to_samples(by_name["shot@b"]["start_frame"], fps, sample_rate) + assert by_name["shot@b"]["start_sample"] == expected + + def test_seam_sample_start_matches_the_frame_to_sample_conversion(self): + """#401: dissolve_videos' crossfade window used to floor its + ms->sample conversion while every other tool that places a frame on + a track (frames_to_samples, remeasured_shots) rounds - so a shot's + start_sample recorded here could land one sample below what + pair_audio would recompute for the same frame boundary. The seam's + recorded start_sample must agree with frames_to_samples for the same + frame offset, fps and rate.""" + fps, sample_rate, dissolve_frames = 24, 32000, 8 + + def clip(num_frames, level): + samples = frames_to_samples(num_frames, fps, sample_rate) + audio = numpy.full((2, samples), float(level), dtype=numpy.float32) + return AudioVideo(frames(num_frames), audio, sample_rate, fps=fps) + + result = dissolve_videos([clip(20, 1), clip(20, 2)], dissolve_frames, fps=fps) + + frame_starts = [shot["start_frame"] for shot in result.shots] + assert frame_starts[1] == 12 # 20 - dissolve_frames + + expected_sample_start = frames_to_samples(frame_starts[1], fps, sample_rate) + assert result.shots[1]["start_sample"] == expected_sample_start + + def test_shots_survive_a_pair_audio_round_trip_unchanged(self): + """#401: a shot passed through pair_audio unchanged in frames must + come out unchanged in samples - the invariant the two tools' sample + math is required to agree on.""" + fps, sample_rate, dissolve_frames = 24, 32000, 8 + + def clip(num_frames, level): + samples = frames_to_samples(num_frames, fps, sample_rate) + audio = numpy.full((2, samples), float(level), dtype=numpy.float32) + return AudioVideo(frames(num_frames), audio, sample_rate, fps=fps) + + joined = dissolve_videos([clip(20, 1), clip(20, 2)], dissolve_frames, fps=fps) + + new_track = numpy.zeros((2, joined.audio.shape[1]), dtype=numpy.float32) + paired = pair_audio(joined, new_track, sample_rate=sample_rate, fps=fps) + + assert [shot["start_frame"] for shot in paired.shots] == [ + shot["start_frame"] for shot in joined.shots + ] + assert [shot["start_sample"] for shot in paired.shots] == [ + shot["start_sample"] for shot in joined.shots + ] + + +# --------------------------------------------------------------------------- +# 5. run_chain +# --------------------------------------------------------------------------- + + +class _FakePipeline: + def __init__(self, output_factory): + self.output_factory = output_factory + self.calls = [] + + def _run_once(self, arguments): + self.calls.append(arguments) + return self.output_factory(arguments, len(self.calls) - 1) + + +def _video_output(arguments, index, num_frames=4): + color = (50 * index % 256, 100, 150) + made = frames(num_frames, color) + if "image" in arguments: + made[0] = arguments["image"] + from types import SimpleNamespace + + return SimpleNamespace(frames=[made]) + + +class TestRunChainShots: + def test_segments_are_named_and_partition_the_output(self): + pipeline = _FakePipeline(_video_output) + + result = run_chain(pipeline, {"segments": 3, "trim_frames": 1}, {}) + + assert [shot["name"] for shot in result.shots] == [ + "segment 1", + "segment 2", + "segment 3", + ] + counts = [shot["num_frames"] for shot in result.shots] + starts = [shot["start_frame"] for shot in result.shots] + assert counts == [4, 3, 3] + assert starts == [0, 4, 7] + assert sum(counts) == len(result.frames) + + def test_video_only_chain_has_no_sample_fields(self): + pipeline = _FakePipeline(_video_output) + + result = run_chain(pipeline, {"segments": 2}, {}) + + for shot in result.shots: + assert shot["start_sample"] is None + assert shot["num_samples"] is None + + +# --------------------------------------------------------------------------- +# 6. pair_audio recomputes the sample fields +# --------------------------------------------------------------------------- + + +class TestPairAudioShots: + def test_recomputes_sample_fields_against_the_new_track(self): + fps, rate = 4, 100 + shots = [ + shot_record("a", 0, 4, start_sample=999, num_samples=999), + shot_record("b", 4, 4, start_sample=999, num_samples=999), + ] + video = AudioVideo(frames(8), None, None, fps=fps, shots=shots) + new_track = numpy.zeros((2, 250), dtype=numpy.float32) + + paired = pair_audio(video, new_track, sample_rate=rate) + + # Frame side is untouched + assert [shot["start_frame"] for shot in paired.shots] == [0, 4] + assert [shot["num_frames"] for shot in paired.shots] == [4, 4] + + assert paired.shots[0]["start_sample"] == round(0 / fps * rate) + assert paired.shots[1]["start_sample"] == round(4 / fps * rate) + # The last shot ends at the waveform's own length + last = paired.shots[-1] + assert last["start_sample"] + last["num_samples"] == new_track.shape[1] + assert sum(shot["num_samples"] for shot in paired.shots) == new_track.shape[1] + + def test_no_frame_rate_clears_the_sample_side(self): + shots = [shot_record("a", 0, 4, start_sample=1, num_samples=2)] + video = AudioVideo(frames(4), None, None, fps=None, shots=shots) + new_track = numpy.zeros((2, 100), dtype=numpy.float32) + + paired = pair_audio(video, new_track, sample_rate=100) + + assert paired.shots[0]["start_sample"] is None + assert paired.shots[0]["num_samples"] is None + + def test_remeasured_shots_directly(self): + """dw.shots.remeasured_shots in isolation, the function pair_audio calls.""" + shots = [shot_record("a", 0, 5), shot_record("b", 5, 5)] + + remeasured = remeasured_shots(shots, fps=5, sample_rate=10, total_samples=20) + + assert remeasured[0]["start_sample"] == 0 + assert remeasured[1]["start_sample"] == 10 + assert remeasured[1]["num_samples"] == 10 + + assert remeasured_shots(shots, fps=None, sample_rate=10, total_samples=20) == [ + {**shot, "start_sample": None, "num_samples": None} for shot in shots + ] + assert remeasured_shots(None, fps=5, sample_rate=10, total_samples=20) is None + + def test_last_shot_absorbs_a_track_off_the_frame_grid(self): + """#423: the last shot's num_samples is allowed to disagree with + round(num_frames * sample_rate / fps) by whatever slop the track + pair_audio was handed carries - every earlier boundary must still + match the frame->sample formula exactly.""" + fps, sample_rate = 24, 44100 + shots = [ + shot_record("a", 0, 1068), + shot_record("b", 1068, 124), + ] + frame_grid_end = round(1192 / fps * sample_rate) # 2190300 + + remeasured = remeasured_shots( + shots, fps=fps, sample_rate=sample_rate, total_samples=frame_grid_end + 1 + ) + + assert remeasured[0]["start_sample"] == round(0 / fps * sample_rate) + assert remeasured[1]["start_sample"] == round(1068 / fps * sample_rate) + last = remeasured[-1] + assert last["start_sample"] + last["num_samples"] == frame_grid_end + 1 + assert last["num_samples"] != round(124 * sample_rate / fps) + + +# --------------------------------------------------------------------------- +# 7. slice_audio drops shots +# --------------------------------------------------------------------------- + + +class TestSliceAudioDropsShots: + def test_slicing_a_joined_video_returns_a_track_with_no_shots(self): + shots = [shot_record("a", 0, 4, 0, 100), shot_record("b", 4, 4, 100, 100)] + video = AudioVideo( + frames(8), + numpy.zeros((2, 200), dtype=numpy.float32), + 100, + fps=4, + shots=shots, + ) + + sliced = slice_audio(video, start_frame=0, num_frames=4, fps=4) + + assert getattr(sliced, "shots", None) is None + + +# --------------------------------------------------------------------------- +# 8. interpolate_frames / rescaled_shots +# --------------------------------------------------------------------------- + + +class TestRescaledShots: + """interpolate_frames calls rescaled_shots(source_shots, multiplier) + directly (dw/tasks/interpolate_frames.py); driving it through the real + task needs a loaded RIFE model, so the function is exercised here.""" + + def test_frame_counts_and_partition_after_doubling(self): + shots = [shot_record("a", 0, 5), shot_record("b", 5, 5)] + + rescaled = rescaled_shots(shots, multiplier=2) + + # (10 - 1) * 2 + 1 = 19 total frames + assert rescaled[0]["start_frame"] == 0 + assert rescaled[1]["start_frame"] == 10 + last_end = rescaled[1]["start_frame"] + rescaled[1]["num_frames"] + assert last_end == 19 + + def test_sample_fields_are_cleared(self): + shots = [shot_record("a", 0, 5, start_sample=0, num_samples=50)] + + rescaled = rescaled_shots(shots, multiplier=2) + + assert rescaled[0]["start_sample"] is None + assert rescaled[0]["num_samples"] is None + + def test_overlap_frames_scales_with_the_multiplier(self): + shots = [ + shot_record("a", 0, 5), + {**shot_record("b", 5, 5), "overlap_frames": 3}, + ] + + rescaled = rescaled_shots(shots, multiplier=2) + + assert rescaled[1]["overlap_frames"] == 6 + + def test_empty_input_returns_none(self): + assert rescaled_shots(None, multiplier=2) is None + assert rescaled_shots([], multiplier=2) is None + + +class TestCarriedShots: + def test_deep_copies_so_the_source_is_unaffected(self): + source = AudioVideo( + frames(4), None, None, shots=[shot_record("a", 0, 4, 0, 10)] + ) + + copied = carried_shots(source) + copied[0]["num_frames"] = 999 + + assert source.shots[0]["num_frames"] == 4 + + def test_no_shots_returns_none(self): + source = AudioVideo(frames(4), None, None) + + assert carried_shots(source) is None + + +# --------------------------------------------------------------------------- +# 9. Round trip: join -> Result.save -> manifest -> recorded_shots +# --------------------------------------------------------------------------- + + +class TestRoundTrip: + """Exercises the same functions Workflow.run does (Result.save, + dw.shots.step_shots, and dw.runs.recorded_shots) with a real join's + output, without spinning up a full model-backed Workflow.run - the + workflow's own step loop (dw/workflow.py) glues these together with no + logic of its own beyond what is called here.""" + + def test_shots_survive_save_and_recorded_shots(self, tmp_path): + videos = [audio_video(4, 1), audio_video(6, 2)] + joined = concat_videos(videos, fps=4) + + result = Result({"content_type": "video/mp4", "save": True}) + result.add_result(joined) + + run_dir = tmp_path / "concat-demo" / "20260923-120000-abcdef01" + run_dir.mkdir(parents=True) + # The actual mux is exercised by dw's own result-saving tests + # (tests/test_concat_videos.py's TestPreviousResultChainAudioFit + # patches the same three names); what this test pins is what + # Result.save records in saved_shots and how the manifest step reads + # it back, not the codec. + with ( + patch("dw.result.encode_video"), + patch("dw.result.export_to_video"), + patch("dw.result.is_av_available", return_value=True), + ): + saved_files = result.save(str(run_dir), "concat-demo-join.0") + + assert saved_files + assert result.saved_shots + + relative_files = [os.path.relpath(path, run_dir) for path in saved_files] + manifest_shots = step_shots(result.saved_shots, saved_files) + assert manifest_shots is not None + assert [shot["name"] for shot in manifest_shots] == ["video 1", "video 2"] + assert sum(shot["num_frames"] for shot in manifest_shots) == len(joined.frames) + + manifest = { + "steps": [ + { + "step": "join", + "files": relative_files, + "shots": manifest_shots, + } + ] + } + with open(run_dir / MANIFEST_FILE_NAME, "w") as handle: + json.dump(manifest, handle) + + relative_path = f"concat-demo/20260923-120000-abcdef01/{relative_files[0]}" + read_back = recorded_shots(str(tmp_path), relative_path) + + assert read_back == manifest_shots + + def test_step_shots_renames_reach_the_artifact_a_later_step_would_read( + self, tmp_path + ): + """A later step's `previous_result:cut` reads the same artifact + object `Result.save` extracted (`Result._artifacts_for`'s cache, by + identity) - not a fresh copy - so a probe reading `previous_result: + cut` after this step must see the step-named shot too, not just the + manifest built alongside it. Renaming only a deep copy for the + manifest left the artifact itself carrying the join's `video 2` + fallback (#396 follow-up).""" + videos = [audio_video(4, 1), audio_video(4, 2)] + joined = concat_videos(videos, fps=4) + + result = Result({"content_type": "video/mp4", "save": True}) + result.add_result(joined) + + run_dir = tmp_path / "cut-demo" / "20260924-000000-abcdef01" + run_dir.mkdir(parents=True) + with ( + patch("dw.result.encode_video"), + patch("dw.result.export_to_video"), + patch("dw.result.is_av_available", return_value=True), + ): + saved_files = result.save(str(run_dir), "cut-demo-join.0") + + step_shots( + result.saved_shots, + saved_files, + references=["asset:ep31-shot1-return.mp4", "previous_result:shot2d"], + ) + + # What a later step reads via previous_result:cut (get_artifacts, the + # same-identity artifact) - not the manifest's deep copy + artifact = result.get_artifacts()[0] + assert [shot["name"] for shot in artifact.shots] == [ + "video 1", + "shot2d", + ] + + def test_a_reference_still_renames_a_later_input_when_an_earlier_one_nests( + self, tmp_path + ): + """#432: a `previous_result:` reference naming the *last* of two + inputs stopped renaming anything once the *first* input carried its + own inner shots (a multi-shot dissolve, say) - the join's flattened + `shots` list then held more entries than the step's `videos` list did + references, and the old positional zip in named_shots refused to + rename any of them rather than just the ones it could not place. The + nested input's own shot names must survive untouched; only the + single-shot trailing input takes the step's override name.""" + fps, sample_rate = 4, 100 + inner_shots = [ + shot_record("shot@accuse", 0, 4, start_sample=0, num_samples=100), + shot_record("shot@deflect", 4, 4, start_sample=100, num_samples=100), + shot_record("shot@resolve", 8, 4, start_sample=200, num_samples=100), + ] + nested = AudioVideo( + frames(12), + numpy.full((2, 300), 1.0, dtype=numpy.float32), + sample_rate, + fps=fps, + shots=inner_shots, + ) + trailing = audio_video(4, 2, fps=fps, sample_rate=sample_rate) + + joined = concat_videos([nested, trailing], fps=fps) + + result = Result({"content_type": "video/mp4", "save": True}) + result.add_result(joined) + + run_dir = tmp_path / "cut-demo" / "20260925-000000-abcdef01" + run_dir.mkdir(parents=True) + with ( + patch("dw.result.encode_video"), + patch("dw.result.export_to_video"), + patch("dw.result.is_av_available", return_value=True), + ): + saved_files = result.save(str(run_dir), "cut-demo-join.0") + + manifest_shots = step_shots( + result.saved_shots, + saved_files, + references=["asset:ep63-episode.mp4", "previous_result:paired"], + ) + + assert [shot["name"] for shot in manifest_shots] == [ + "shot@accuse", + "shot@deflect", + "shot@resolve", + "paired", + ] + # And the artifact a later previous_result: step would read is + # renamed the same way, not just the manifest's copy + artifact = result.get_artifacts()[0] + assert [shot["name"] for shot in artifact.shots] == [ + "shot@accuse", + "shot@deflect", + "shot@resolve", + "paired", + ] + # The transient source_index tag never leaks onto a consumer surface + assert all("source_index" not in shot for shot in manifest_shots) + assert all("source_index" not in shot for shot in artifact.shots) + + def test_recorded_shots_is_none_outside_a_run_directory(self, tmp_path): + # The flat layout - no run id segment - has no manifest to read shots from + assert recorded_shots(str(tmp_path), "workflow/still.png") is None + + def test_recorded_shots_is_none_when_the_step_was_reused(self, tmp_path): + run_dir = tmp_path / "wf" / "20260923-120000-abcdef01" + run_dir.mkdir(parents=True) + manifest = { + "steps": [ + { + "step": "join", + "files": ["out.mp4"], + "shots": [shot_record("a", 0, 4, 0, 10)], + "reused": True, + } + ] + } + with open(run_dir / MANIFEST_FILE_NAME, "w") as handle: + json.dump(manifest, handle) + + assert ( + recorded_shots(str(tmp_path), "wf/20260923-120000-abcdef01/out.mp4") is None + ) + + +# --------------------------------------------------------------------------- +# 10. shots_for_file with multi-file entries +# --------------------------------------------------------------------------- + + +class TestShotsForFile: + def test_multi_file_entry_returns_only_that_files_shots_without_file_key(self): + shots = [ + {**shot_record("a", 0, 4, 0, 10), "file": "one.mp4"}, + {**shot_record("b", 0, 5, 0, 12), "file": "two.mp4"}, + ] + + own = shots_for_file(shots, "two.mp4", ["one.mp4", "two.mp4"]) + + assert len(own) == 1 + assert own[0]["name"] == "b" + assert "file" not in own[0] + + def test_single_file_entry_matches_by_the_step_files_list(self): + shots = [shot_record("a", 0, 4, 0, 10)] + + assert shots_for_file(shots, "solo.mp4", ["solo.mp4"]) == shots + assert shots_for_file(shots, "other.mp4", ["solo.mp4"]) is None + + def test_no_shots_returns_none(self): + assert shots_for_file(None, "solo.mp4", ["solo.mp4"]) is None + assert shots_for_file([], "solo.mp4", ["solo.mp4"]) is None + + +# 11. Workflow.run: the manifest entry and step_end carry the shots, named by +# the `gather:shot` the step wrote + + +class _ShotResult: + """Stands in for dw.result.Result: writes one file and, for the join, + reports the shots its video carried the way Result.save does.""" + + def __init__(self, shots=None): + self.result_list = [] + self.saved_files = [] + self.saved_shots = {} + self.selected = None + self._shots = shots + + def save(self, output_dir, base_name): + os.makedirs(output_dir, exist_ok=True) + path = os.path.join(output_dir, f"{base_name}.mp4") + with open(path, "wb") as handle: + handle.write(b"") + self.saved_files = [path] + if self._shots: + self.saved_shots = {path: self._shots} + return self.saved_files + + +def test_workflow_run_names_the_joined_shots_by_their_members(tmp_path): + """The real Workflow.run path: `gather:shot` expands to + `previous_result:shot@` references, and the join's positional + shots are renamed from them in the manifest, manifest.json and + step_end - and read back by recorded_shots.""" + from dw.events import RunContext + from dw.step import Step + from dw.step_cache import step_cache + from dw.workflow import Workflow + + joined = [ + shot_record("video 1", 0, 24, 0, 16000), + shot_record("video 2", 24, 30, 16000, 20267), + ] + definition = { + "id": "shots_round_trip", + "steps": [ + { + "name": "shot", + "for_each": [{"name": "wide"}, {"name": "close"}], + "task": {"command": "stabilize_video", "arguments": {"clip": "x"}}, + "result": {"content_type": "video/mp4"}, + }, + { + "name": "cut", + "task": { + "command": "concat_videos", + "arguments": {"videos": "gather:shot"}, + }, + "result": {"content_type": "video/mp4"}, + }, + ], + } + + def fake_step_run(self, previous_results, previous_pipelines, step_action): + return _ShotResult(joined if self.name == "cut" else None) + + step_cache.clear() + events = [] + workflow = Workflow(definition, str(tmp_path), str(tmp_path / "shots.json")) + with patch.object(Step, "run", fake_step_run): + workflow.run({}, context=RunContext(on_event=events.append)) + + (entry,) = [e for e in workflow.manifest if e["step"] == "cut"] + names = [shot["name"] for shot in entry["shots"]] + assert names == ["shot@wide", "shot@close"] + assert [shot["num_samples"] for shot in entry["shots"]] == [16000, 20267] + + (step_end,) = [ + e for e in events if e["event"] == "step_end" and e.get("step") == "cut" + ] + assert step_end["shots"] == entry["shots"] + + with open(os.path.join(workflow._run_dir, "manifest.json")) as handle: + manifest = json.load(handle) + (written,) = [e for e in manifest["steps"] if e["step"] == "cut"] + assert written["shots"] == entry["shots"] + + relative = os.path.relpath( + os.path.join(workflow._run_dir, written["files"][0]), str(tmp_path) + ).replace(os.sep, "/") + assert recorded_shots(str(tmp_path), relative) == entry["shots"] diff --git a/tests/test_slice_preflight.py b/tests/test_slice_preflight.py new file mode 100644 index 00000000..db338658 --- /dev/null +++ b/tests/test_slice_preflight.py @@ -0,0 +1,148 @@ +"""A slice_audio source shorter than the requested slice, warned about at +validate time rather than only at run time - #402. + +Exercises the real decode path (`probe_media` against a genuine wav file) +rather than a mock of it, so a fixture shorter than its declared slice is +the same file the run itself would have padded with silence. +""" + +import os +import tempfile +import wave + +import numpy + +from dw.runs import activate_output_root, deactivate_output_root +from dw.slice_preflight import slice_past_end_warnings +from dw.workflow import workflow_from_definition + + +def write_wav(path, seconds=2.0, sample_rate=8000): + t = numpy.arange(int(seconds * sample_rate)) / sample_rate + samples = (numpy.sin(2 * numpy.pi * 220 * t) * 0.5 * 32767).astype(" 0 + + gone.unlink() + + assert cache.get("w", step_data, 42, set(), "/out", True) is None + assert cache._retained_bytes == 0 + + +def test_stats_reports_the_stale_drop_that_freed_its_bytes(tmp_path): + """#418 follow-up: stats() is the read-only view of the same accounting + the stale-drop test above exercises directly - entries/retained_bytes + must go back down through this surface too, since it's the one a caller + outside the process (get_memory) actually reads.""" + cache = StepCache() + assert cache.stats() == { + "entries": 0, + "max_entries": cache.max_entries, + "retained_bytes": 0, + "max_retained_bytes": cache.max_retained_bytes, + } + + step_data = {"name": "gen", "pipeline": {"arguments": {"prompt": "a cat"}}} + gone = tmp_path / "gone.png" + gone.write_bytes(b"x") + result = FakeResult("first", [str(gone)], result_list=[{"videos": _frames(5)}]) + cache.put("w", step_data, 42, result, "/out", True) + + stats = cache.stats() + assert stats["entries"] == 1 + assert stats["retained_bytes"] > 0 + + gone.unlink() + assert cache.get("w", step_data, 42, set(), "/out", True) is None + + assert cache.stats() == { + "entries": 0, + "max_entries": cache.max_entries, + "retained_bytes": 0, + "max_retained_bytes": cache.max_retained_bytes, + } + + def test_deep_equal_compares_array_values_by_content_rather_than_raising(): """A realized step argument can hold a numpy array (or a tensor), whose == yields an array, not a bool - that must not abort the run with 'truth @@ -273,6 +324,60 @@ def test_the_default_cap_fits_a_maximal_for_each_run(): assert StepCache.DEFAULT_MAX_ENTRIES >= 2 * MAX_FOR_EACH_ENTRIES + 8 +def test_result_bytes_sums_array_frames_and_ignores_scalars(): + result = FakeResult("video", result_list=[{"videos": _frames(10), "fps": 24}]) + assert _result_bytes(result) == 10 * 64 * 64 * 3 + + +def test_result_bytes_does_not_double_count_a_shared_object(): + # get_artifact_list's fitted-in-place audio can be referenced by more + # than one artifact in the same result_list - the byte budget should + # not charge for it twice + audio = np.zeros(1000, dtype=np.float32) + result = FakeResult("av", result_list=[{"audio": audio}, {"audio": audio}]) + assert _result_bytes(result) == audio.nbytes + + +def test_step_cache_evicts_retained_entries_over_the_byte_budget(): + """Every shot@ member of a for_each group is legitimately retained (the + gather step genuinely reads all of them), but nothing bounded how much + decoded media that retention pins resident once the run that needed it + is done (#368) - the byte budget is the bound, on top of the entry cap.""" + frame_bytes = 64 * 64 * 3 + cache = StepCache(max_entries=128, max_retained_bytes=4 * frame_bytes) + for name in ("shot@a", "shot@b", "shot@c"): + result = FakeResult(name, result_list=[{"videos": _frames(2)}]) + cache.put("w", {"name": name}, 1, result, "/out", True) + + # 3 entries x 2 frames each = 6 frames worth, over the 4-frame budget - + # the least recently used (shot@a) is evicted despite being well under + # the entry-count cap + assert cache.get("w", {"name": "shot@a"}, 1, set(), "/out", True) is None + assert cache.get("w", {"name": "shot@b"}, 1, set(), "/out", True) is not None + assert cache.get("w", {"name": "shot@c"}, 1, set(), "/out", True) is not None + + +def test_step_cache_has_a_default_byte_budget(): + cache = StepCache() + assert cache.max_retained_bytes == StepCache.DEFAULT_MAX_RETAINED_BYTES + + +def test_step_cache_never_evicts_the_only_retained_entry_over_budget(): + cache = StepCache(max_retained_bytes=1) + result = FakeResult("big", result_list=[{"videos": _frames(5)}]) + cache.put("w", {"name": "only"}, 1, result, "/out", True) + + assert cache.get("w", {"name": "only"}, 1, set(), "/out", True) is result + + +def test_step_cache_unretained_result_does_not_count_against_the_byte_budget(): + cache = StepCache(max_retained_bytes=1) + heavy = FakeResult("heavy", result_list=[{"videos": _frames(5)}]) + cache.put("w", {"name": "unretained"}, 1, heavy, "/out", False) + + assert cache._retained_bytes == 0 + + def test_step_cache_clear(): cache = StepCache() step_data = {"name": "gen", "pipeline": {"arguments": {"prompt": "a cat"}}} diff --git a/tests/test_strip_exif_and_watermark.py b/tests/test_strip_exif_and_watermark.py index ec56964e..4ead8b66 100644 --- a/tests/test_strip_exif_and_watermark.py +++ b/tests/test_strip_exif_and_watermark.py @@ -1,6 +1,8 @@ """Tests for strip_exif and add_watermark image processing commands.""" import unittest + +import numpy as np from PIL import Image from PIL.PngImagePlugin import PngInfo @@ -61,15 +63,20 @@ def test_modifies_pixels(self): self.assertNotEqual(img.tobytes(), result.tobytes()) def test_default_text(self): - # Should not raise with defaults img = Image.new("RGB", (400, 200)) result = add_watermark(img) - self.assertIsInstance(result, Image.Image) + # The defaults draw "AI Generated", pixel for pixel + expected = add_watermark(img, text="AI Generated") + other = add_watermark(img, text="SOMETHING ELSE") + self.assertNotEqual(result.tobytes(), img.tobytes()) + self.assertEqual(result.tobytes(), expected.tobytes()) + self.assertNotEqual(result.tobytes(), other.tobytes()) def test_custom_text(self): img = Image.new("RGB", (400, 200)) result = add_watermark(img, text="DO NOT DISTRIBUTE") - self.assertIsInstance(result, Image.Image) + self.assertNotEqual(result.tobytes(), img.tobytes()) + self.assertNotEqual(result.tobytes(), add_watermark(img).tobytes()) def test_all_positions(self): img = Image.new("RGB", (400, 200)) @@ -79,19 +86,33 @@ def test_all_positions(self): def test_invalid_position_falls_back(self): img = Image.new("RGB", (400, 200)) - # Unknown position should fall back to bottom-right - result = add_watermark(img, position="nonsense") - self.assertIsInstance(result, Image.Image) + # Unknown position should fall back to bottom-right, pixel for pixel + result = add_watermark(img, position="nonsense", opacity=255) + expected = add_watermark(img, position="bottom-right", opacity=255) + elsewhere = add_watermark(img, position="top-left", opacity=255) + self.assertEqual(result.tobytes(), expected.tobytes()) + self.assertNotEqual(result.tobytes(), elsewhere.tobytes()) def test_custom_color(self): img = Image.new("RGB", (400, 200)) - result = add_watermark(img, color=(255, 0, 0)) - self.assertIsInstance(result, Image.Image) + result = add_watermark(img, color=(255, 0, 0), opacity=255) + # Red text on black: every drawn pixel is some shade of pure red + pixels = np.asarray(result).reshape(-1, 3) + drawn = pixels[pixels.any(axis=1)] + self.assertTrue(len(drawn)) + self.assertFalse(drawn[:, 1:].any()) + self.assertEqual(drawn[:, 0].max(), 255) def test_custom_font_size(self): img = Image.new("RGB", (400, 200)) - result = add_watermark(img, font_size=24) - self.assertIsInstance(result, Image.Image) + + def inked(font_size): + result = add_watermark(img, text="W", font_size=font_size, opacity=255) + return int(np.asarray(result).any(axis=2).sum()) + + # the auto size here is max(12, 200 // 30) = 12; a larger size draws more + self.assertGreater(inked(48), inked(24)) + self.assertGreater(inked(24), inked(0)) def test_rgba_input_converted(self): img = Image.new("RGBA", (200, 100)) @@ -100,8 +121,13 @@ def test_rgba_input_converted(self): def test_dispatch_via_process_image(self): img = Image.new("RGB", (200, 100)) - result = process_image(img, "add_watermark", "cpu", {"text": "TEST"}) - self.assertIsInstance(result, Image.Image) + result = process_image( + img, "add_watermark", "cpu", {"text": "TEST", "opacity": 255} + ) + # the kwargs reach add_watermark: same pixels as a direct call + expected = add_watermark(img, text="TEST", opacity=255) + self.assertEqual(result.tobytes(), expected.tobytes()) + self.assertNotEqual(result.tobytes(), img.tobytes()) if __name__ == "__main__": diff --git a/tests/test_task.py b/tests/test_task.py index 74184497..8a99c734 100644 --- a/tests/test_task.py +++ b/tests/test_task.py @@ -1,4 +1,6 @@ +import io import pytest +from types import SimpleNamespace from unittest.mock import patch from dw.tasks.task import Task from PIL import Image @@ -65,28 +67,21 @@ def test_image_processor_command_dispatches_without_registry_entry(): assert result.size == (4, 4) -@pytest.mark.skip(reason="Requires network access to external URLs which may be flaky") -def test_gather_images_task(): - task_def = { - "command": "gather_images", - "arguments": { - "urls": [ - "https://pbs.twimg.com/media/Gf5iaDGXsAA0R30?format=jpg&name=small", - "https://pbs.twimg.com/media/Gf7vNQJXoAAY5Cm?format=jpg&name=small", - ] - }, - } +def test_gather_images_task_dispatches_to_gather(): + urls = ["https://example.com/a.jpg", "https://example.com/b.jpg"] + responses = [] + for size in ((4, 4), (8, 8)): + buffer = io.BytesIO() + Image.new("RGB", size).save(buffer, format="PNG") + responses.append(SimpleNamespace(content=buffer.getvalue())) + task_def = {"command": "gather_images", "arguments": {"urls": urls}} task = Task(task_def, "cpu") - result = task.run(task_def["arguments"]) - assert isinstance(result, list), "Expected a list of images from gather_images" + with patch("dw.tasks.gather.safe_get", side_effect=responses) as safe_get: + result = task.run(task_def["arguments"]) -def test_gather_inputs_task(): - task_def = {"command": "gather_inputs", "inputs": ["value1", "value2"]} - task = Task(task_def, "cpu") - result = task.run(task_def["inputs"]) - assert isinstance(result, list), "Expected a list of inputs from gather_inputs" - assert "value1" in result and "value2" in result, "Should gather all passed inputs" + assert [image.size for image in result] == [(4, 4), (8, 8)] + assert [call.args[0] for call in safe_get.call_args_list] == urls def test_format_chat_message_task(): @@ -119,7 +114,6 @@ def test_format_chat_message_task(): assert text_inputs[1]["content"] == "unit_test", "User message content mismatch" -@pytest.mark.skip(reason="Test not fully implemented yet") def test_batch_decode_post_process_task(): # We use a mock pipeline to simulate previous_pipelines behavior. class MockPipeline: @@ -145,10 +139,8 @@ def post_process_generation(self, generated_text, task): } task = Task(task_def, "cpu") result = task.run(task_def["arguments"], previous_pipelines=mock_previous_pipelines) - assert result == [ - "decoded-foo", - "decoded-bar", - ], "Should return batch-decoded strings" + # The first decoded sequence, post-processed and read back under the task key + assert result == "decoded-foo" class TestTaskDevice: diff --git a/tests/test_task_domains.py b/tests/test_task_domains.py index a94c0dc4..c071187e 100644 --- a/tests/test_task_domains.py +++ b/tests/test_task_domains.py @@ -15,7 +15,9 @@ from dw.task_domains import ( TASK_ARGUMENT_DOMAINS, + CLOSED_UNIT, NON_NEGATIVE, + NON_POSITIVE, POSITIVE, as_number, task_argument_errors, @@ -59,9 +61,14 @@ def test_every_argument_is_a_parameter_of_its_command(self): parameters = {p["name"] for p in describe_task(command)["parameters"]} assert set(domains) <= parameters, command - def test_every_domain_is_one_of_the_two(self): + def test_every_domain_is_one_of_the_declared_kinds(self): for domains in TASK_ARGUMENT_DOMAINS.values(): - assert set(domains.values()) <= {POSITIVE, NON_NEGATIVE} + assert set(domains.values()) <= { + POSITIVE, + NON_NEGATIVE, + NON_POSITIVE, + CLOSED_UNIT, + } class TestAsNumber: @@ -100,6 +107,27 @@ def test_a_zero_target_rate_is_refused(self): assert len(errors) == 1 assert errors[0]["path"] == "steps[0].task.arguments.target_sample_rate" + def test_a_positive_target_lufs_is_refused(self): + errors = errors_for( + "normalize_audio", {"audio": "asset:bed.wav", "target_lufs": 3.0} + ) + assert len(errors) == 1 + assert errors[0]["path"] == "steps[0].task.arguments.target_lufs" + + def test_an_out_of_range_temperature_is_refused(self): + errors = errors_for("grade", {"media": "asset:a.png", "temperature": 5.0}) + assert len(errors) == 1 + assert errors[0]["path"] == "steps[0].task.arguments.temperature" + assert "-1.0 and 1.0" in errors[0]["message"] + + def test_an_out_of_range_tint_is_refused(self): + errors = errors_for("grade", {"media": "asset:a.png", "tint": -1.5}) + assert len(errors) == 1 + assert errors[0]["path"] == "steps[0].task.arguments.tint" + + def test_a_boundary_temperature_is_fine(self): + assert errors_for("grade", {"media": "asset:a.png", "temperature": 1.0}) == [] + def test_a_zero_offset_is_fine(self): assert ( errors_for( diff --git a/tests/test_task_signature_errors.py b/tests/test_task_signature_errors.py index c92d9c6f..1d449663 100644 --- a/tests/test_task_signature_errors.py +++ b/tests/test_task_signature_errors.py @@ -24,6 +24,8 @@ class of mistake a free pre-flight most obviously exists for (#141). An ) from dw.tasks.task import Task +REPO_ROOT = pathlib.Path(__file__).resolve().parent.parent + def task_step(command, arguments, name="a"): return { @@ -111,6 +113,76 @@ def test_a_free_form_command_accepts_anything(self): assert errors_for("gather_inputs", {"whatever": 1}) == [] +class TestARequiredArgumentFedByANullVariable: + """A step that names the argument by `variable:name`, where `name`'s + value is null, is not a step that "does not supply" it (#364) - the + error carries a `variable` key so a caller with no arguments of its own + can tell the two apart.""" + + def test_the_repro_carries_the_variable_key_and_a_clearer_message(self): + written = { + "id": "sig", + "variables": {"audio": None}, + "steps": [ + task_step( + "resample_audio", + {"audio": "variable:audio", "target_sample_rate": 16000}, + ) + ], + } + # replace_variables drops a variable: reference resolved to null + # from its containing dict (#209) - this is what the expanded + # definition looks like once that has happened + expanded = { + "id": "sig", + "steps": [task_step("resample_audio", {"target_sample_rate": 16000})], + } + errors = task_signature_errors(expanded, written_definition=written) + assert len(errors) == 1 + assert errors[0]["variable"] == "audio" + assert errors[0]["path"] == "steps[0].task.arguments.audio" + assert "variable 'audio'" in errors[0]["message"] + assert "does not supply" not in errors[0]["message"] + + def test_no_written_definition_keeps_the_original_wording(self): + """Without the author-written form to compare against - the #141 + call sites already in the codebase before #364 - nothing changes.""" + errors = errors_for("resample_audio", {"target_sample_rate": 16000}) + assert "variable" not in errors[0] + assert "does not supply" in errors[0]["message"] + + def test_a_genuinely_missing_argument_is_unaffected(self): + """No `variable:` reference at all in the written step - still the + plain #141 message, even with a written_definition available.""" + written = { + "id": "sig", + "steps": [task_step("resample_audio", {"target_sample_rate": 16000})], + } + errors = task_signature_errors(written, written_definition=written) + assert "variable" not in errors[0] + assert "does not supply" in errors[0]["message"] + + def test_a_variable_not_declared_is_unaffected(self): + """`variable:audio` written but nothing declares `audio` - not the + shape #364 covers, so the original wording stands.""" + written = { + "id": "sig", + "steps": [ + task_step( + "resample_audio", + {"audio": "variable:audio", "target_sample_rate": 16000}, + ) + ], + } + expanded = { + "id": "sig", + "steps": [task_step("resample_audio", {"target_sample_rate": 16000})], + } + errors = task_signature_errors(expanded, written_definition=written) + assert "variable" not in errors[0] + assert "does not supply" in errors[0]["message"] + + class TestTheRunTimeBackstop: """The static pass sees literals. A required argument that arrived from a variable or an earlier step and resolved to nothing reaches the command, @@ -138,13 +210,13 @@ class TestTheCatalogItself: @pytest.mark.parametrize( "path", sorted( - str(p) - for p in list(pathlib.Path("workflows").rglob("*.json")) - + list(pathlib.Path("dw/workflows").glob("*.json")) + str(p.relative_to(REPO_ROOT)) + for p in list((REPO_ROOT / "workflows").rglob("*.json")) + + list((REPO_ROOT / "dw" / "workflows").glob("*.json")) ), ) def test_workflow_has_no_task_signature_error(self, path): - definition = json.loads(pathlib.Path(path).read_text()) + definition = json.loads((REPO_ROOT / path).read_text()) if not isinstance(definition, dict) or "steps" not in definition: pytest.skip("not a workflow") assert task_signature_errors(definition) == [] diff --git a/tests/test_teacache.py b/tests/test_teacache.py index 644ab154..d7faf668 100644 --- a/tests/test_teacache.py +++ b/tests/test_teacache.py @@ -101,16 +101,6 @@ def _call(bound_forward, timestep_value): ) -def test_duplicate_timestep_raises_runtime_error(): - """Two forward calls with the identical timestep (true CFG) must raise.""" - _, bound_forward = _make_bound_forward() - - _call(bound_forward, 0.9) # first call: no prior timestep, always allowed - - with pytest.raises(RuntimeError, match="true classifier-free guidance"): - _call(bound_forward, 0.9) # duplicate timestep: simulates the uncond pass - - def test_duplicate_timestep_error_names_both_features(): """The guard's error message must name both TeaCache and true CFG.""" _, bound_forward = _make_bound_forward() diff --git a/tests/test_tensor_image.py b/tests/test_tensor_image.py index 20ad2a60..c145742c 100644 --- a/tests/test_tensor_image.py +++ b/tests/test_tensor_image.py @@ -43,11 +43,6 @@ def test_dtype_cast(self): tensor = pil_to_float_tensor(image, "cpu", dtype=torch.float64) assert tensor.dtype == torch.float64 - def test_dtype_defaults_to_float32(self): - image = Image.new("RGB", (4, 4), color=(1, 2, 3)) - tensor = pil_to_float_tensor(image, "cpu") - assert tensor.dtype == torch.float32 - class TestFloatTensorToPil: def test_accepts_batched_and_unbatched(self): @@ -128,12 +123,6 @@ def test_near_half_maps_by_rounding_not_floor(self): assert truncated == 127 assert pixel[0] != truncated - def test_half_value_rounds_up_not_down(self): - """0.5/255-scaled exact half (127.5) rounds to nearest even (128).""" - tensor = torch.full((1, 3, 1, 1), 127.5 / 255.0) - image = float_tensor_to_pil(tensor) - assert image.getpixel((0, 0)) == (128, 128, 128) - class TestModuleAdoption: """Verify upscale.py and interpolate_frames.py actually route through the diff --git a/tests/test_text_generation.py b/tests/test_text_generation.py index e8a5173f..e7fe2327 100644 --- a/tests/test_text_generation.py +++ b/tests/test_text_generation.py @@ -336,14 +336,5 @@ def test_other_devices_still_use_device_map(self, mock_pipeline): self.assertNotIn("device", kwargs) -class TestTextGenerationRegistration(unittest.TestCase): - """Test that text_generation is registered as a task command.""" - - def test_command_registered(self): - from dw.tasks.task import _COMMAND_REGISTRY - - self.assertIn("text_generation", _COMMAND_REGISTRY) - - if __name__ == "__main__": unittest.main() diff --git a/tests/test_text_sections.py b/tests/test_text_sections.py index 33e732c1..ac2f7d44 100644 --- a/tests/test_text_sections.py +++ b/tests/test_text_sections.py @@ -90,7 +90,7 @@ def test_no_sections_requested_is_a_passthrough(): def h3_prompts(): """Every hand-written H3 prompt in the examples, as (file, key, text).""" found = [] - pattern = os.path.join(REPO_ROOT, "workflows", "minimax", "MiniMaxH3*.json") + pattern = os.path.join(REPO_ROOT, "workflows", "templates", "minimax", "*.json") for path in sorted(glob.glob(pattern)): with open(path, encoding="utf-8") as handle: workflow = json.load(handle) @@ -100,6 +100,12 @@ def h3_prompts(): return found +def test_the_sweep_finds_the_shipped_prompts(): + """The parametrized test below collects nothing, and so passes, if the + templates move again.""" + assert h3_prompts() + + @pytest.mark.parametrize("name,key,prompt", h3_prompts()) def test_a_hand_written_prompt_passes_through_unchanged(name, key, prompt): """The trim must be a no-op on a prompt that is already well formed. diff --git a/tests/test_type_helpers.py b/tests/test_type_helpers.py index c8208024..8a36f030 100644 --- a/tests/test_type_helpers.py +++ b/tests/test_type_helpers.py @@ -15,11 +15,9 @@ class TestGetType: """Test getting type from module""" def test_get_type_from_diffusers(self): - # This would work if diffusers is installed - # For testing, we'll use a built-in type + import diffusers - result = get_type("sys", "version") - assert result is not None + assert get_type("diffusers", "DiffusionPipeline") is diffusers.DiffusionPipeline def test_get_type_invalid_module(self): with pytest.raises(ModuleNotFoundError): @@ -33,12 +31,6 @@ def test_get_type_invalid_attribute(self): class TestLoadTypeFromName: """Test loading type by name from diffusers""" - def test_load_type_with_full_path(self): - # Test with fully qualified name - result = load_type_from_full_name("os.path.join") - assert callable(result) - assert result.__name__ == "join" - def test_load_type_invalid_full_path(self): with pytest.raises(ModuleNotFoundError): load_type_from_full_name("fake.module.Type") diff --git a/tests/test_validate_arguments.py b/tests/test_validate_arguments.py index 0171ae98..87fa1954 100644 --- a/tests/test_validate_arguments.py +++ b/tests/test_validate_arguments.py @@ -30,6 +30,27 @@ def typed_workflow(): return workflow +def null_variable_workflow(): + """A required task argument fed by `variable:audio`, where `audio`'s + declared default is null - the #364 repro. A fine document (the step + does supply the argument, just not yet a value); a run left as-is + would fail.""" + return { + "id": "null_var", + "variables": {"audio": None}, + "steps": [ + { + "name": "n", + "task": { + "command": "normalize_audio", + "arguments": {"audio": "variable:audio"}, + }, + "result": {"content_type": "audio/wav"}, + } + ], + } + + def placeholder_workflow(): """A stored default that names no file in this workspace, the shape of `templates/ltx2/reference-sheet` and its siblings (#166): a bare call @@ -47,6 +68,8 @@ def server(tmp_path): json.dump(typed_workflow(), file) with open(os.path.join(root.workflows, "Placeholder.json"), "w") as file: json.dump(placeholder_workflow(), file) + with open(os.path.join(root.workflows, "NullVariable.json"), "w") as file: + json.dump(null_variable_workflow(), file) with open(os.path.join(root.assets, "iris.png"), "wb") as file: file.write(b"not really a png, but it is a file under that name") with open(os.path.join(root.prompts, "hero.json"), "w") as file: @@ -411,6 +434,84 @@ def test_submission_and_validation_give_the_same_message(self, server): assert validated["errors"][0]["message"] in refused.json()["detail"] +class TestNullVariableArgument: + """#364: a required task argument fed by `variable:name` where `name`'s + declared value is null is a fine document - `save_workflow` accepts it, + and `validate_workflow` called with no `arguments` at all must agree, + since that is the same "check the document" question. The moment the + caller names arguments of their own - even `{}` - it is a real run + being checked, and one that leaves the variable null is a hard error. + """ + + def test_no_arguments_key_at_all_is_a_warning_not_an_error(self, server): + """The literal repro: no `arguments` field in the request body.""" + with server() as client: + response = client.post( + "/api/validate", json={"workflow_path": "NullVariable"} + ).json() + + assert response["valid"] is True + assert response["errors"] == [] + assert any("audio" in warning for warning in response["warnings"]) + + def test_an_inline_document_with_no_arguments_is_a_warning_too(self, server): + with server() as client: + response = client.post( + "/api/validate", json={"workflow": null_variable_workflow()} + ).json() + + assert response["valid"] is True + assert any("audio" in warning for warning in response["warnings"]) + + def test_an_explicit_empty_arguments_dict_is_still_a_hard_error(self, server): + """`{}` names a run with no values supplied, distinct from omitting + `arguments` entirely - the variable is still null for that run.""" + with server() as client: + response = client.post( + "/api/validate", + json={"workflow_path": "NullVariable", "arguments": {}}, + ).json() + + assert response["valid"] is False + assert response["errors"][0]["variable"] == "audio" + + def test_arguments_that_still_leave_it_null_are_a_hard_error(self, server): + with server() as client: + response = client.post( + "/api/validate", + json={ + "workflow_path": "NullVariable", + "arguments": {"audio": None}, + }, + ).json() + + assert response["valid"] is False + assert response["errors"][0]["variable"] == "audio" + + def test_arguments_that_supply_a_value_pass(self, server): + with server() as client: + response = client.post( + "/api/validate", + json={ + "workflow_path": "NullVariable", + "arguments": {"audio": "asset:iris.png"}, + }, + ).json() + + assert response["valid"] is True + assert response["errors"] == [] + + def test_save_workflow_accepts_the_document_with_a_warning(self, server): + with server() as client: + response = client.put( + "/api/workflows/NullVariableSaved", + json={"workflow": null_variable_workflow()}, + ) + + assert response.status_code == 200 + assert any("audio" in warning for warning in response.json()["warnings"]) + + def test_an_entry_key_no_step_reads_is_a_warning_not_an_error(server): workflow = { "id": "cut", diff --git a/tests/test_variables.py b/tests/test_variables.py index 75df3762..5743fed4 100644 --- a/tests/test_variables.py +++ b/tests/test_variables.py @@ -275,6 +275,25 @@ def test_set_variables_string_too_long_raises(): set_variables(values, variables) +def test_set_variables_dict_override_of_a_string_default_raises(): + """A media variable that used to be declared as {'location': ...} and is + now a plain string (templates/ltx2/keyframes' first_image/last_image, + #433) must refuse a caller still passing the old dict shape rather than + silently stringifying it - str({'location': 'x'}) never raises, so the + dict's repr would otherwise reach the workflow as a bogus path.""" + variables = {"first_image": "https://example/x.png"} + + with pytest.raises(ValueError, match="takes a plain string"): + set_variables({"first_image": {"location": "asset:x.png"}}, variables) + + +def test_set_variables_list_override_of_a_string_default_raises(): + variables = {"prompt": "a cat"} + + with pytest.raises(ValueError, match="takes a plain string"): + set_variables({"prompt": ["a", "b"]}, variables) + + def test_set_variables_list_entry_too_long_raises_the_same_error(): """A string nested inside a list-valued argument (a for_each entry's prompt, say) is exactly as reachable as a top-level one, and must be @@ -295,7 +314,21 @@ def test_set_variables_list_with_ordinary_strings_passes_unchanged(): set_variables(values, variables) - assert variables["shots"] == [{"name": "a", "prompt": "a cat"}, {"name": "b"}] + +def test_set_variables_already_realized_object_passes_through_a_string_default(): + """A sub-workflow argument built from 'previous_result:' is already + a live object (an AudioTrack, here) by the time it reaches set_variables - + coercing it through the declared variable's own type (a string + 'asset:...' default) called str() on the object and produced its Python + repr, which a downstream task then tried to read as a file path (#404).""" + from dw.result import AudioTrack + + track = AudioTrack(audio=[0.0, 0.1, 0.2], sample_rate=44100) + variables = {"score": "asset:score.wav"} + + set_variables({"score": track}, variables) + + assert variables["score"] is track def test_argument_errors_reports_a_too_long_entry_under_the_list_argument(): @@ -315,6 +348,22 @@ def test_argument_errors_reports_a_too_long_entry_under_the_list_argument(): assert "too long" in errors[0]["message"] +def test_argument_errors_reports_a_dict_passed_for_a_string_variable(): + """The #433 shape: validate_workflow must flag a dict passed for a + variable whose default is a plain string, the same class of error as + a too-long string or a fractional int override - not pass it through + to fail at run time with a path built from the dict's repr.""" + definition = { + "variables": {"first_image": "https://example/x.png"}, + } + arguments = {"first_image": {"location": "asset:qa-cast/priya-portrait.jpg"}} + + errors = argument_errors(definition, arguments) + + assert [error["path"] for error in errors] == ["arguments.first_image"] + assert "takes a plain string" in errors[0]["message"] + + class TestResolveVariableValues: """A list-valued variable's entries may name other variables - a shot entry says "from_file": "variable:character_a_voice" and one variable diff --git a/tests/test_video_extensions.py b/tests/test_video_extensions.py new file mode 100644 index 00000000..3ad7a6b0 --- /dev/null +++ b/tests/test_video_extensions.py @@ -0,0 +1,107 @@ +"""A video argument's extension, refused for free when it is already knowable. + +#347: `loop_frames`'s `video` argument took a still image by its own +docstring, but `asset:sheet.png` validated clean and then died inside +`fetch_video`'s extension gate in the first seconds of the run. +""" + +import tempfile + +from dw.video_extensions import _extension_problem, video_extension_errors +from dw.workflow import workflow_from_definition + + +def workflow_holding(video): + return { + "id": "holding", + "steps": [ + { + "name": "hold", + "task": { + "command": "loop_frames", + "arguments": {"video": video, "num_frames": 121}, + }, + "result": {"content_type": "video/mp4", "fps": 24}, + } + ], + } + + +class TestTheFault: + def test_a_still_asset_names_the_media_type_form(self): + problem = _extension_problem("asset:sheet.png") + + assert "media_type" in problem + assert "asset:sheet.png" in problem + + def test_a_video_asset_is_fine(self): + assert _extension_problem("asset:clip.mp4") is None + + def test_a_video_output_reference_is_fine(self): + assert _extension_problem("output:t/20260914-171601-adeee23c/i/x.mp4") is None + + def test_an_extension_the_run_would_also_refuse_is_named(self): + assert _extension_problem("asset:notes.txt") is not None + + def test_a_url_is_left_to_the_run(self): + """`fetch_video` never gates a URL's extension, so this pass must not + refuse one either - #347's binding scope.""" + assert _extension_problem("https://example.com/sheet.png") is None + + def test_a_deferred_reference_is_not_yet_knowable(self): + assert _extension_problem("previous_result:make_image") is None + assert _extension_problem("variable:video_path") is None + + def test_a_path_with_no_extension_is_not_this_passs_complaint(self): + assert _extension_problem("asset:sheet") is None + + def test_a_non_string_is_not_this_passs_complaint(self): + assert _extension_problem(None) is None + assert ( + _extension_problem({"media_type": "video", "location": "asset:x.mp4"}) + is None + ) + + +class TestTheValidationPass: + def test_a_still_by_key_convention_is_refused_before_the_queue(self): + workflow = workflow_from_definition( + workflow_holding("asset:sheet.png"), tempfile.mkdtemp() + ) + + problems = workflow.validation_errors() + + assert any( + problem["path"] == "steps[0].task.arguments.video" for problem in problems + ) + + def test_the_media_type_form_validates(self): + workflow = workflow_from_definition( + workflow_holding({"media_type": "image", "location": "asset:sheet.png"}), + tempfile.mkdtemp(), + ) + + assert workflow.validation_errors() == [] + + def test_a_media_type_video_reference_checks_its_location(self): + workflow = workflow_from_definition( + workflow_holding({"media_type": "video", "location": "asset:sheet.png"}), + tempfile.mkdtemp(), + ) + + problems = workflow.validation_errors() + + assert any( + problem["path"] == "steps[0].task.arguments.video.location" + for problem in problems + ) + + def test_a_real_video_asset_validates(self): + workflow = workflow_from_definition( + workflow_holding("asset:clip.mp4"), tempfile.mkdtemp() + ) + + assert workflow.validation_errors() == [] + + def test_nothing_is_reported_for_a_definition_with_no_video_arguments(self): + assert video_extension_errors({"steps": [{"name": "a", "task": {}}]}) == [] diff --git a/tests/test_video_utils.py b/tests/test_video_utils.py index 114ff3fe..587c7b0a 100644 --- a/tests/test_video_utils.py +++ b/tests/test_video_utils.py @@ -361,6 +361,100 @@ def test_the_rate_reaches_the_paired_video(self, tmp_path): assert paired.fps == 24 + def test_a_loaded_video_argument_carries_the_run_s_shots(self, tmp_path): + """A video loaded by path (an asset:/output: reference, already + resolved to a local file by the time fetch_video sees it) carries + the shots its run's manifest recorded, the way it already carries + the file's fps - #398.""" + import json + + from dw.arguments import fetch_video + from dw.runs import MANIFEST_FILE_NAME + from dw.tasks.video_utils import FrameList + + run_dir = tmp_path / "ep42" / "20260101-000000-abcdef01" + run_dir.mkdir(parents=True) + path = self.write_video(run_dir / "ep42-film.mp4", fps=24, num_frames=24) + shots = [ + { + "name": "shot@accuse", + "start_frame": 0, + "num_frames": 12, + "start_sample": None, + "num_samples": None, + }, + { + "name": "shot@deflect", + "start_frame": 12, + "num_frames": 12, + "start_sample": None, + "num_samples": None, + }, + ] + manifest = { + "steps": [ + {"step": "concat_videos", "files": ["ep42-film.mp4"], "shots": shots} + ] + } + (run_dir / MANIFEST_FILE_NAME).write_text(json.dumps(manifest)) + + frames = fetch_video(path) + + assert isinstance(frames, FrameList) + assert [shot["name"] for shot in frames.shots] == [ + "shot@accuse", + "shot@deflect", + ] + + def test_pair_audio_remeasures_the_shots_a_loaded_video_carries(self, tmp_path): + """The other half of #398: pair_audio's own remeasuring, fed a + video loaded from a path rather than built by an earlier step in + the same workflow.""" + import json + + from dw.arguments import fetch_video + from dw.runs import MANIFEST_FILE_NAME + from dw.tasks.pair_audio import pair_audio + + run_dir = tmp_path / "ep42" / "20260101-000000-abcdef01" + run_dir.mkdir(parents=True) + path = self.write_video(run_dir / "ep42-film.mp4", fps=24, num_frames=24) + shots = [ + { + "name": "shot@accuse", + "start_frame": 0, + "num_frames": 12, + "start_sample": None, + "num_samples": None, + }, + { + "name": "shot@deflect", + "start_frame": 12, + "num_frames": 12, + "start_sample": None, + "num_samples": None, + }, + ] + manifest = { + "steps": [ + {"step": "concat_videos", "files": ["ep42-film.mp4"], "shots": shots} + ] + } + (run_dir / MANIFEST_FILE_NAME).write_text(json.dumps(manifest)) + + paired = pair_audio( + video=fetch_video(path), + audio=numpy.zeros((2, 16000), dtype=numpy.float32), + sample_rate=16000, + ) + + assert [shot["name"] for shot in paired.shots] == [ + "shot@accuse", + "shot@deflect", + ] + assert paired.shots[0]["start_sample"] == 0 + assert paired.shots[1]["start_sample"] == round(12 / 24 * 16000) + def test_audio_is_fitted_to_the_frames_own_duration(self, tmp_path): """The codec pads the last block; joined shot after shot that padding would walk the sound off the picture.""" @@ -399,6 +493,121 @@ def test_a_disallowed_extension_is_refused(self, tmp_path): load_audio_video(str(payload)) +class TestVideoFileReference: + """#367. get_frame/get_first_frame/get_last_frame only need one frame; a + VideoFileReference lets get_frame seek to it with PyAV instead of + decoding the whole clip through fetch_video/load_video.""" + + def write_long_clip(self, path, num_frames=300, fps=30, marked=()): + """A clip whose frames are black except the given indexes, which are + pure red - a marker robust to a lossy codec's compression noise, + unlike a unique near-black shade per frame.""" + from diffusers.utils.export_utils import encode_video + + marked = set(marked) + frames = [ + Image.new("RGB", (8, 8), (255, 0, 0) if index in marked else (0, 0, 0)) + for index in range(num_frames) + ] + encode_video(frames, fps=fps, output_path=str(path)) + return str(path) + + def assert_is_red(self, frame): + r, g, b = frame.getpixel((0, 0)) + assert r > 128 and r > g + 64 and r > b + 64 + + def assert_is_black(self, frame): + r, g, b = frame.getpixel((0, 0)) + assert r < 96 + + def test_get_frame_seeks_rather_than_decoding_the_whole_clip(self, tmp_path): + from dw.tasks.video_utils import VideoFileReference + + path = self.write_long_clip(tmp_path / "long.mp4", marked=[250]) + ref = VideoFileReference(path) + + self.assert_is_red(get_frame(ref, 250)) + self.assert_is_black(get_frame(ref, 100)) + + def test_negative_indexes_count_from_the_end(self, tmp_path): + from dw.tasks.video_utils import VideoFileReference + + path = self.write_long_clip(tmp_path / "long.mp4", marked=[299]) + ref = VideoFileReference(path) + + self.assert_is_red(get_frame(ref, -1)) + + def test_an_out_of_range_index_names_the_frame_count(self, tmp_path): + from dw.tasks.video_utils import VideoFileReference + + path = self.write_long_clip(tmp_path / "long.mp4") + ref = VideoFileReference(path) + + with pytest.raises(ValueError, match="past the end of a 300-frame clip"): + get_frame(ref, 999999) + + def test_process_video_dispatches_first_and_last_through_the_reference( + self, tmp_path + ): + from dw.tasks.video_utils import VideoFileReference + + path = self.write_long_clip(tmp_path / "long.mp4", marked=[0, 299]) + ref = VideoFileReference(path) + + first = process_video(ref, "get_first_frame", "cpu", {}) + last = process_video(ref, "get_last_frame", "cpu", {}) + + self.assert_is_red(first) + self.assert_is_red(last) + + def test_realize_args_builds_a_reference_without_calling_load_video( + self, tmp_path, monkeypatch + ): + """The whole point of #367: a get_frame step's 'video' must not go + through the eager, whole-clip fetch_video/load_video path.""" + import dw.arguments as arguments_module + from dw.tasks.video_utils import VideoFileReference + + path = self.write_long_clip(tmp_path / "long.mp4", marked=[250]) + + def _boom(*args, **kwargs): + raise AssertionError("load_video must not be called for get_frame (#367)") + + monkeypatch.setattr(arguments_module, "load_video", _boom) + + task = { + "command": "get_frame", + "arguments": {"video": path, "frame_index": 250}, + } + arguments_module.realize_args(task, base_dir=str(tmp_path)) + + video = task["arguments"]["video"] + assert isinstance(video, VideoFileReference) + self.assert_is_red(get_frame(video, 250)) + + def test_a_deferred_previous_result_reference_is_left_unchanged(self, tmp_path): + import dw.arguments as arguments_module + + task = { + "command": "get_frame", + "arguments": {"video": "previous_result:shot", "frame_index": 0}, + } + arguments_module.realize_args(task, base_dir=str(tmp_path)) + + assert task["arguments"]["video"] == "previous_result:shot" + + def test_a_variable_reference_is_left_unchanged(self, tmp_path): + import dw.arguments as arguments_module + + task = { + "command": "get_frame", + "arguments": {"video": "variable:my_video"}, + } + arguments_module.realize_args(task, base_dir=str(tmp_path)) + + assert task["arguments"]["video"] == "variable:my_video" + + class TestIsVideo: def test_the_shapes_that_are_videos(self): import numpy @@ -437,6 +646,35 @@ def test_a_still_becomes_a_run_of_the_asked_for_length(self): assert looped.shape == (121, 4, 8, 3) assert (looped[0] == looped[120]).all() + def test_the_result_survives_diffusers_own_video_preprocessing(self): + """#444. loop_frames feeds LTX2ReferenceCondition.frames, which + diffusers' VaeImageProcessor.preprocess normalizes as `2 * x - 1` + with no /255 rescaling for a raw ndarray - so a uint8 [0, 255] array + (what frames_as_array itself returns) comes out at up to ~509 + instead of [-1, 1], garbage into the VAE encoder. This runs the real + diffusers path rather than asserting the array's own range.""" + from diffusers.video_processor import VideoProcessor + + looped = loop_frames(Image.new("RGB", (64, 32), "red"), 4) + + tensor = VideoProcessor(vae_scale_factor=8).preprocess_video(looped) + + assert tensor.min().item() >= -1.0 + assert tensor.max().item() <= 1.0 + + def test_the_result_is_float32_scaled_to_0_1(self): + """#444. A raw ndarray reaches diffusers' VaeImageProcessor untouched + - no /255 rescaling happens downstream - so loop_frames has to hand + back data already in the [0, 1] range its own reference-conditioning + caller (LTX2ReferenceCondition.frames) expects, not the uint8 [0, 255] + frames_as_array itself returns.""" + looped = loop_frames(Image.new("RGB", (8, 4), "red"), 4) + + assert looped.dtype == numpy.float32 + assert looped.max() <= 1.0 + assert looped.min() >= 0.0 + assert numpy.isclose(looped[0, 0, 0, 0], 1.0) # red's R channel is 255 + def test_a_short_clip_laps_round_and_the_last_lap_is_trimmed(self): frames = numpy.stack( [numpy.full((2, 2, 3), value, dtype=numpy.uint8) for value in (1, 2, 3)] @@ -444,7 +682,10 @@ def test_a_short_clip_laps_round_and_the_last_lap_is_trimmed(self): looped = loop_frames(frames, 7) - assert [int(frame[0][0][0]) for frame in looped] == [1, 2, 3, 1, 2, 3, 1] + assert numpy.allclose( + [float(frame[0][0][0]) for frame in looped], + [1 / 255, 2 / 255, 3 / 255, 1 / 255, 2 / 255, 3 / 255, 1 / 255], + ) def test_a_clip_longer_than_the_request_is_trimmed(self): frames = numpy.zeros((10, 2, 2, 3), dtype=numpy.uint8) @@ -572,6 +813,60 @@ def test_a_numeric_string_count_is_taken(self, clip): assert frame_grid(clip, count="4", tile_width=32).size == (2 * 32, 2 * 16) + def test_a_list_of_stills_loaded_through_fetch_video_tiles(self): + # #443: the reported repro - two character portraits passed to + # frame_grid's `video` argument as {"media_type": "image", ...} + # references, the way validate_workflow's own hint tells a caller to + # - loaded through the real fetch_video path, not a mock of it + import os + import tempfile + from dw.arguments import fetch_video + from dw.tasks.video_utils import frame_grid + + with tempfile.TemporaryDirectory() as temp_dir: + Image.new("RGB", (64, 32), "red").save(os.path.join(temp_dir, "a.jpg")) + Image.new("RGB", (64, 32), "blue").save(os.path.join(temp_dir, "b.jpg")) + + video = fetch_video( + [ + {"media_type": "image", "location": "a.jpg"}, + {"media_type": "image", "location": "b.jpg"}, + ], + base_dir=temp_dir, + ) + + grid = frame_grid(video, count=2, tile_width=64, label=False) + + assert grid.size == (2 * 64, 1 * 32) + + +class TestGetFrameOnAStill: + """#443's audit: get_frame shares fetch_video's loading path, so a still + handed to it through the same media_type reference must not raise.""" + + def test_get_frame_of_a_bare_still_returns_the_still(self): + from dw.tasks.video_utils import get_frame + + still = Image.new("RGB", (8, 4), "green") + + assert get_frame(still, 0) == still + + def test_get_frame_of_a_still_loaded_through_fetch_video(self): + import os + import tempfile + from dw.arguments import fetch_video + from dw.tasks.video_utils import get_frame + + with tempfile.TemporaryDirectory() as temp_dir: + Image.new("RGB", (8, 4), "green").save(os.path.join(temp_dir, "s.png")) + + video = fetch_video( + {"media_type": "image", "location": "s.png"}, base_dir=temp_dir + ) + frame = get_frame(video, 0) + + assert frame.size == (8, 4) + class TestFitAudioToFrames: """Codec padding trimmed off a generated track, in whatever layout the diff --git a/tests/test_worker.py b/tests/test_worker.py index 75f652b1..54108a92 100644 --- a/tests/test_worker.py +++ b/tests/test_worker.py @@ -135,6 +135,12 @@ def test_worker_memory_status(worker_process): assert "gpu_device_name" in info assert "gpu_memory_allocated_mb" in info assert "gpu_memory_reserved_mb" in info + assert info["step_cache"] == { + "entries": 0, + "max_entries": 128, + "retained_bytes": 0, + "max_retained_bytes": 4 * 1024**3, + } def test_worker_clear_memory(worker_process): @@ -149,74 +155,6 @@ def test_worker_clear_memory(worker_process): assert "gpu_available" in info -@pytest.mark.skipif( - not os.path.exists(TEST_WORKFLOW_PATH), - reason=f"test workflow not found: {TEST_WORKFLOW_PATH}", -) -@requires_accelerator -def test_worker_with_simple_workflow(worker_process, tmp_path): - """Worker executes a real workflow and reuses cached models on a second run.""" - cmd_queue, res_queue, worker = worker_process - - output_dir = str(tmp_path / "test_outputs") - os.makedirs(output_dir, exist_ok=True) - - cmd_queue.put( - { - "type": "execute", - "workflow_path": TEST_WORKFLOW_PATH, - "arguments": {}, - "output_dir": output_dir, - "log_level": "INFO", - } - ) - - # First message after spawn (or after a fresh command with no prior - # traffic) must tolerate child import cost; workflow execution itself - # (model load + inference) is also slow, so keep the generous timeout - # for every message in this loop rather than switching to the short one. - success = False - saw_workflow_loaded = False - while True: - result = res_queue.get(timeout=WORKER_READY_TIMEOUT) - result_type = result.get("type") - - if result_type == "workflow_loaded": - saw_workflow_loaded = True - elif result_type == "success": - success = True - break - elif result_type == "error": - pytest.fail(f"Workflow execution error: {result['message']}") - - assert success - assert saw_workflow_loaded - - # Run again to exercise the model-reuse/caching path. - cmd_queue.put( - { - "type": "execute", - "workflow_path": TEST_WORKFLOW_PATH, - "arguments": {}, - "output_dir": output_dir, - "log_level": "INFO", - } - ) - - second_run_count = None - while True: - result = res_queue.get(timeout=WORKER_READY_TIMEOUT) - result_type = result.get("type") - - if result_type == "success": - second_run_count = result["run_count"] - break - elif result_type == "error": - pytest.fail(f"Workflow execution error on second run: {result['message']}") - - assert second_run_count == 2 - - @pytest.mark.skipif( not os.path.exists(TEST_WORKFLOW_PATH), reason=f"test workflow not found: {TEST_WORKFLOW_PATH}", diff --git a/tests/test_worker_execute.py b/tests/test_worker_execute.py index c17b9a74..e1da3ccf 100644 --- a/tests/test_worker_execute.py +++ b/tests/test_worker_execute.py @@ -22,7 +22,7 @@ def __init__(self, behavior=None): self.behavior = behavior self.manifest = [{"step": "s", "files": ["/out/a.png"]}] - def validate(self): + def validate(self, arguments=None): pass def run( @@ -146,6 +146,7 @@ def test_shutdown_during_run_cancels_then_flags_shutdown(): class StubResult: saved_files = [] + result_list = [] def test_full_cleanup_clears_step_cache(): @@ -392,3 +393,75 @@ def test_a_workflow_switch_forgets_the_prior_keys(): command={"workflow_path": "other.json", "arguments": {}, "output_dir": "/tmp"}, ) assert worker.prior_step_keys == {} + + +def test_execute_validates_against_the_callers_arguments_not_the_default(tmp_path): + """#415: a document-default 'text/html' content_type that the caller's + own argument overrides to 'text/plain' must actually run, not just queue. + + JobManager.submit() (fixed for #415's first bounce) checks the caller's + arguments before handing the command to the worker, but _handle_execute + itself called workflow.validate() with none - so the job queued, then + failed at execution against the unsubstituted default. This drives a + real Workflow (not StubWorkflow, which stubs validate() to a no-op) + through the actual worker path, the one StubWorkflow-based tests above + cannot catch.""" + from dw.workflow import workflow_from_definition + + worker = _make_worker() + definition = { + "id": "se-415", + "variables": {"ct": "text/html"}, + "steps": [ + { + "name": "t", + "task": { + "command": "compose_text", + "arguments": {"parts": ["x"]}, + }, + "result": {"content_type": "variable:ct"}, + } + ], + } + with patch( + "dw.worker.workflow_from_definition", + lambda data, out, base_dir=None, workflow_dir=None: workflow_from_definition( + data, out, base_dir, workflow_dir + ), + ): + worker._handle_execute( + { + "workflow": definition, + "arguments": {"ct": "text/plain"}, + "output_dir": str(tmp_path), + } + ) + messages = _drain(worker.result_queue) + types = [m["type"] for m in messages] + assert "success" in types, messages + success = next(m for m in messages if m["type"] == "success") + files = success["manifest"][0]["files"] + assert len(files) == 1 + assert (tmp_path / files[0]).read_text() == "x" + + +def test_between_run_cleanup_releases_host_caches_without_clearing_pipelines(): + """#368: a job's own cleanup left ~10GB resident that only clear_memory + reclaimed - the pinned-host staging buffers of group_offload and the + glibc arenas a released pipeline's weights were read into. Neither is + touched by gc.collect()/empty_device_cache() alone, so the light, + every-job cleanup must also call release_host_caches() - and must keep + loaded_pipelines/shared_components warm while doing it, since those + exist for exactly this (inter-run) cleanup to leave alone. + """ + worker = _make_worker() + worker.loaded_pipelines["warm-key"] = object() + worker.shared_components["warm-component"] = object() + + with patch("dw.worker.release_host_caches", return_value=512.0) as released: + worker._cleanup_between_runs() + + released.assert_called_once() + # the whole point: still-warm state for the next run survives this call + assert "warm-key" in worker.loaded_pipelines + assert "warm-component" in worker.shared_components diff --git a/tests/test_workflow.py b/tests/test_workflow.py index 35f7feac..6ce79c28 100644 --- a/tests/test_workflow.py +++ b/tests/test_workflow.py @@ -32,29 +32,12 @@ def test_workflow_validation_invalid(invalid_workflow_json, tmp_path): assert "Validation error" in str(exc_info.value) -def test_workflow_name(valid_workflow_json, tmp_path): - workflow = Workflow(valid_workflow_json, str(tmp_path), "") - assert workflow.name == "test_workflow" - - def test_workflow_from_file(test_data_dir, tmp_path): workflow_path = os.path.join(test_data_dir, "workflows", "valid_workflow.json") workflow = workflow_from_file(workflow_path, str(tmp_path)) assert isinstance(workflow, Workflow) -def test_workflow_variables_property(valid_workflow_json, tmp_path): - workflow = Workflow(valid_workflow_json, str(tmp_path), "") - assert "prompt" in workflow.variables - assert workflow.variables["prompt"] == "test prompt" - - -def test_workflow_argument_template(valid_workflow_json, tmp_path): - workflow = Workflow(valid_workflow_json, str(tmp_path), "") - # Should return empty dict if no argument_template - assert workflow.argument_template == {} - - def test_workflow_security_validation(tmp_path): from dw.security import SecurityError @@ -353,6 +336,29 @@ def test_builtin_sub_workflow_loads_under_a_confined_workflow(self, tmp_path): assert isinstance(child, Workflow) assert child.name == "test_job" + def test_a_traversing_builtin_name_names_the_builtin_root(self, tmp_path): + # #422: the refusal named only the rejected name, not where + # 'builtin:' looks + from dw.security import InvalidInputError + from dw.workflow_sources import builtin_root + + workflow_dir = tmp_path / "workflows" + workflow_dir.mkdir() + parent = Workflow( + {"id": "parent", "steps": []}, + str(tmp_path / "outputs"), + str(workflow_dir / "__inline__.json"), + str(workflow_dir), + ) + step = {"name": "child", "workflow": {"path": "builtin:../../x.json"}} + + with pytest.raises(InvalidInputError) as exc_info: + parent.create_step_action(step, {}, {}, 42, "cpu") + + message = str(exc_info.value) + assert "../../x.json" in message + assert builtin_root() in message + class TestSubWorkflowPathsAcrossTheCatalog: """A template under templates/ names a model config under models/ as @@ -409,7 +415,7 @@ def test_a_parent_directory_step_inside_the_root_is_allowed(self, tmp_path): device="cpu", ) - assert action is not None + assert action.name == "child" def test_a_parent_directory_step_escaping_the_root_is_refused(self, tmp_path): import json @@ -523,7 +529,7 @@ def test_an_unconfined_run_may_climb_to_a_sibling_catalog_folder(self, tmp_path) device="cpu", ) - assert action is not None + assert action.name == "child" def test_a_file_outside_any_catalog_is_confined_to_its_own_directory( self, tmp_path @@ -1041,12 +1047,6 @@ def test_a_name_that_resolves_nowhere_says_where_it_looked(self, tmp_path): assert "does-not-exist.json" in message assert "outside the root" not in message - def test_a_relative_path_beside_the_file_still_wins(self, tmp_path): - """The '../models/x.json' form every template uses is unchanged.""" - action = self._resolve(tmp_path, "minimax/ref2va.json") - - assert action.name == "child" - class TestComposedStepSavesOnce: """A sub-workflow step that declares a result owns the file: the child's @@ -1122,6 +1122,95 @@ def test_a_composed_file_carries_the_parent_step_name(self, tmp_path): ) +class TestSubWorkflowPreviousResultArgument: + """A 'previous_result:' argument folded into a sub-workflow step is + already a live object (an AudioTrack here) by the time it reaches the + child's declared variables - coercing it through the variable's own + default type (a string 'asset:' reference) used to call str() on the + object and hand the child's task the object's Python repr instead of the + track, which then failed as a bogus file path (#404).""" + + def _compose(self, tmp_path, monkeypatch): + import soundfile + import numpy + + workflows = tmp_path / "workflows" + assets = workflows / "assets" + assets.mkdir(parents=True) + soundfile.write( + str(assets / "tone.wav"), + numpy.zeros((16000, 1), dtype=numpy.float32), + 16000, + ) + monkeypatch.setenv("DW_ASSET_DIR", str(assets)) + + child = { + "id": "child", + "variables": {"score": "asset:score.wav"}, + "steps": [ + { + "name": "cut", + "task": { + "command": "slice_audio", + "arguments": { + "audio": "variable:score", + "start_seconds": 0, + "duration_seconds": 0.5, + }, + }, + "result": {"content_type": "audio/wav"}, + } + ], + } + (workflows / "child.json").write_text(json.dumps(child)) + + parent = { + "id": "parent", + "steps": [ + { + "name": "bed", + "task": { + "command": "slice_audio", + "arguments": { + "audio": "asset:tone.wav", + "start_seconds": 0, + "duration_seconds": 1.0, + }, + }, + "result": {"content_type": "audio/wav"}, + }, + { + "name": "sub", + "workflow": { + "path": "child.json", + "arguments": {"score": "previous_result:bed"}, + }, + "result": {"content_type": "audio/wav"}, + }, + ], + } + parent_path = workflows / "parent.json" + parent_path.write_text(json.dumps(parent)) + + from dw.workflow import workflow_from_file + + workflow = workflow_from_file( + str(parent_path), str(tmp_path / "outputs"), str(workflows) + ) + workflow.run({}, {}) + return workflow + + def test_the_live_result_reaches_the_child_task_unchanged( + self, tmp_path, monkeypatch + ): + """This used to raise: 'Refusing to read an audio argument at + : it resolves outside every + directory this workflow may read' (#404).""" + workflow = self._compose(tmp_path, monkeypatch) + + assert [entry["step"] for entry in workflow.manifest] == ["bed", "sub"] + + class TestSubWorkflowValidation: """A sub-workflow path that cannot resolve is a validation error, not a run that fails 0.6 s in after the pre-flight said valid (#89).""" diff --git a/tests/test_workflow_sources.py b/tests/test_workflow_sources.py index 4eb478be..48f7a3b3 100644 --- a/tests/test_workflow_sources.py +++ b/tests/test_workflow_sources.py @@ -17,6 +17,7 @@ resolve_in_source, resolve_sub_workflow, source_for_path, + suggest_workflow_names, workflow_names, workflow_sources, writable_source, @@ -111,6 +112,49 @@ def test_a_path_knows_which_source_it_belongs_to(self, roots, tmp_path): assert source_for_path(sources, str(tmp_path / "elsewhere.json")) is None +class TestSuggestions: + """#397: a caller who knows a catalog entry by its short name gets a + pointer to the real one rather than a bare 404.""" + + def test_a_unique_path_suffix_is_suggested(self, roots): + workspace, examples = roots + sources = workflow_sources(str(workspace), [str(examples)]) + assert suggest_workflow_names(sources, "Gyre") == ["ltx2/Gyre"] + + def test_a_typo_falls_back_to_a_close_spelling_match(self, roots): + workspace, examples = roots + sources = workflow_sources(str(workspace), [str(examples)]) + assert suggest_workflow_names(sources, "Shered") == ["Shared"] + + def test_nothing_close_suggests_nothing(self, roots): + workspace, examples = roots + sources = workflow_sources(str(workspace), [str(examples)]) + assert suggest_workflow_names(sources, "zzz-completely-unrelated") == [] + + def test_the_real_catalog_suggests_the_full_template_path(self): + # #397's own repro: a skill or an earlier turn names a template by + # its short id, not its catalog path + repo_root = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + sources = workflow_sources( + os.path.join(repo_root, "workflows"), include_builtin=True + ) + assert suggest_workflow_names(sources, "dialogue-short") == [ + "templates/minimax/dialogue-short" + ] + + def test_a_typo_on_a_short_name_still_finds_the_full_catalog_path(self): + # The tester's own follow-up: "dialog-short" scores 0.92 against the + # entry's own name but 0.55 against the full path, so comparing + # full paths missed a real typo entirely + repo_root = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + sources = workflow_sources( + os.path.join(repo_root, "workflows"), include_builtin=True + ) + assert suggest_workflow_names(sources, "dialog-short") == [ + "templates/minimax/dialogue-short" + ] + + class TestSubWorkflowResolution: """A composed step's relative path is confined to the root it is handed back with, so a name that climbs out of the catalog is never resolved - @@ -146,6 +190,29 @@ def test_a_climb_out_of_the_confinement_is_refused_not_resolved(self, catalog): "../../Outside.json", str(root / "templates"), str(root) ) + def test_a_climb_out_refusal_says_where_it_looked(self, catalog): + # #422: the refusal named only the rejected path, not the search + # path it was judged against + root, outside = catalog + with pytest.raises(SecurityError) as exc_info: + resolve_sub_workflow( + "../../Outside.json", str(root / "templates"), str(root) + ) + message = str(exc_info.value) + assert "Looked in" in message + assert str(root) in message + assert str(outside) in message # the underlying PathTraversalError + # already names the resolved (rejected) path + + def test_an_absolute_path_outside_every_source_says_where_it_looked(self, catalog): + root, outside = catalog + with pytest.raises(SecurityError) as exc_info: + resolve_sub_workflow(str(outside), str(root / "templates"), str(root)) + message = str(exc_info.value) + assert str(outside) in message + assert "Looked in" in message + assert str(root) in message + def test_an_unconfined_caller_still_confines_to_the_catalog(self, catalog): """No confine_to (a bare CLI run) confines to the catalog root the run itself would use - the nearest ancestor named 'workflows' - so diff --git a/tests/test_workflow_step_cache.py b/tests/test_workflow_step_cache.py index d36f2fcd..fe545965 100644 --- a/tests/test_workflow_step_cache.py +++ b/tests/test_workflow_step_cache.py @@ -126,8 +126,10 @@ def __deepcopy__(self, memo): original_realize_args = workflow_module.realize_args - def realize_and_poison(target, base_dir): - original_realize_args(target, base_dir) + def realize_and_poison(target, base_dir, apply_key_conventions=True): + original_realize_args( + target, base_dir, apply_key_conventions=apply_key_conventions + ) if isinstance(target, list): # the steps list, not the variables dict target[0]["pipeline"]["arguments"]["image"] = NotCopyable() diff --git a/tests/test_workflow_trust.py b/tests/test_workflow_trust.py index 3285decd..f2e3ba8a 100644 --- a/tests/test_workflow_trust.py +++ b/tests/test_workflow_trust.py @@ -51,6 +51,9 @@ def _untrust(monkeypatch): class TestTrustFlag: def test_set_trust_workflows_true(self, monkeypatch): + # conftest already trusts; start untrusted so this can fail + _untrust(monkeypatch) + assert workflows_are_trusted() is False set_trust_workflows(True) assert workflows_are_trusted() is True @@ -110,10 +113,51 @@ class TestLoadTypeFromFullName: def test_in_ecosystem_dotted_type_works_untrusted(self, monkeypatch): _untrust(monkeypatch) - result = load_type_from_full_name("torch.bfloat16") + result = load_type_from_full_name( + "diffusers.EulerDiscreteScheduler", "scheduler_type" + ) + import diffusers + + assert result is diffusers.EulerDiscreteScheduler + + def test_an_in_ecosystem_class_of_no_constructible_kind_is_refused( + self, monkeypatch + ): + # In 'torch', so the package allowlist passes it - but it is not a + # model, pipeline, config or other kind an untrusted workflow may + # construct (security.is_constructible_class) + _untrust(monkeypatch) + with pytest.raises(UntrustedWorkflowError): + load_type_from_full_name("torch.nn.Linear", "component_type") + + def test_a_dtype_resolves_untrusted_under_a_dtype_key(self, monkeypatch): + _untrust(monkeypatch) import torch - assert result is torch.bfloat16 + for key in ("dtype", "torch_dtype", "compute_dtype"): + assert load_type_from_full_name("torch.bfloat16", key) is torch.bfloat16 + + @pytest.mark.parametrize("key", [None, "component_type", "config_type"]) + def test_a_dtype_is_refused_untrusted_under_a_type_key(self, monkeypatch, key): + _untrust(monkeypatch) + with pytest.raises(UntrustedWorkflowError, match="not a class"): + load_type_from_full_name("torch.bfloat16", key) + + @pytest.mark.parametrize( + "name", + ["torch.hub.load", "torch.load", "diffusers.utils.load_image", "torch.hub"], + ) + def test_an_in_ecosystem_non_class_is_refused_untrusted(self, monkeypatch, name): + _untrust(monkeypatch) + with pytest.raises(UntrustedWorkflowError, match="not a class"): + load_type_from_full_name(name, "config_type") + + def test_a_bare_non_class_is_refused_untrusted(self, monkeypatch): + from dw.type_helpers import load_type_from_name + + _untrust(monkeypatch) + with pytest.raises(UntrustedWorkflowError, match="not a class"): + load_type_from_name("utils", "component_type") def test_out_of_ecosystem_dotted_type_refused_untrusted(self, monkeypatch): _untrust(monkeypatch) @@ -199,6 +243,41 @@ def test_out_of_ecosystem_constant_allowed_when_trusted(self, monkeypatch): monkeypatch.setenv("DW_TRUST_WORKFLOWS", "1") assert load_constant_from_name("os.sep") == "/" + assert load_constant_from_name("torch.os.sep") == "/" + + @pytest.mark.parametrize( + "name", + ["torch.os.environ", "torch.os.sep", "diffusers.utils.constants.os.environ"], + ) + def test_a_walk_through_an_outside_module_is_refused(self, monkeypatch, name): + from dw.type_helpers import load_constant_from_name + + _untrust(monkeypatch) + with pytest.raises(UntrustedWorkflowError, match="'os' module"): + load_constant_from_name(name) + + @pytest.mark.parametrize( + "name", ["torch._C", "torch.__dict__", "diffusers._version.__version__"] + ) + def test_a_private_segment_is_refused(self, monkeypatch, name): + from dw.type_helpers import load_constant_from_name + + _untrust(monkeypatch) + with pytest.raises(UntrustedWorkflowError, match="private name"): + load_constant_from_name(name) + + def test_a_walk_into_a_dataclass_still_resolves(self, monkeypatch): + from dw.type_helpers import load_constant_from_name + from diffusers.pipelines.ltx2.utils import GEMMA4_PROMPT_ENHANCEMENT_CONFIG + + _untrust(monkeypatch) + assert ( + load_constant_from_name( + "diffusers.pipelines.ltx2.utils." + "GEMMA4_PROMPT_ENHANCEMENT_CONFIG.max_new_tokens" + ) + == GEMMA4_PROMPT_ENHANCEMENT_CONFIG.max_new_tokens + ) class TestValidationGatesUntrustedConstantDefaults: @@ -393,3 +472,74 @@ def test_a_refused_run_emits_no_loading_phase(self, monkeypatch, tmp_path): phases = [e for e in events if e.get("event") == "phase"] assert not any(p.get("phase") == "loading" for p in phases), phases + + +def _catalog_files(): + """Every workflow JSON the repo ships: the runnable catalog and the + packaged builtins its sub-workflow steps name.""" + import glob + import os + + root = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + return sorted( + os.path.relpath(path, root) + for tree in ("workflows", os.path.join("dw", "workflows")) + for path in glob.glob(os.path.join(root, tree, "**", "*.json"), recursive=True) + ) + + +def _catalog_references(node): + """Every '*_type' / '*_dtype' / 'dtype' value a run would load, keyed by + the key it sits under, and every 'constant:' reference.""" + from dw.arguments import NON_TYPE_KEYS, is_constant_reference, is_escaped + + if isinstance(node, dict): + for key, value in node.items(): + if ( + isinstance(value, str) + and (key.endswith("_type") or key.endswith("_dtype") or key == "dtype") + and key not in NON_TYPE_KEYS + and key != "media_type" + and not is_escaped(value) + and ":" not in value + ): + yield "type", key, value + elif is_constant_reference(value): + yield "constant", key, value + else: + yield from _catalog_references(value) + elif isinstance(node, list): + for value in node: + yield from _catalog_references(value) + + +class TestTheCatalogResolvesUntrusted: + """Tightening the untrusted gate to classes and dtypes, and keeping a + constant's walk inside the allowed packages (#407), must not refuse a + name any shipped workflow uses.""" + + @pytest.mark.parametrize("workflow_file", _catalog_files()) + def test_every_type_and_constant_resolves(self, monkeypatch, workflow_file): + import json + import os + + from dw.arguments import fetch_constant + from dw.type_helpers import load_type_from_name + + _untrust(monkeypatch) + root = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + with open(os.path.join(root, workflow_file), encoding="utf-8") as file: + definition = json.load(file) + + for kind, key, value in _catalog_references(definition): + try: + if kind == "type": + load_type_from_name(value, key) + else: + fetch_constant(value) + except UntrustedWorkflowError as error: + pytest.fail(f"{workflow_file}: {key}={value!r} refused: {error}") + except (ImportError, AttributeError, ValueError): + # Not installed here, or not a type at all - test_examples + # owns whether a name resolves; this owns whether it is refused + pass diff --git a/tests/test_workspace.py b/tests/test_workspace.py index 2e79e9b7..6c45ea87 100644 --- a/tests/test_workspace.py +++ b/tests/test_workspace.py @@ -189,11 +189,6 @@ class TestSharedAssetLibrary: the prompt library's treatment applied to a recurring cast, which belongs to no one workspace.""" - def test_it_hangs_off_the_root(self, tmp_path): - root = Workspace(tmp_path / "studio", FLAG) - - assert root.common_assets == os.path.join(root.root, "common", "assets") - def test_a_named_workspace_points_back_at_the_root_s(self, tmp_path): from dw.workspace import named_workspace diff --git a/ui/CLAUDE.md b/ui/CLAUDE.md index 58aceef6..47005503 100644 --- a/ui/CLAUDE.md +++ b/ui/CLAUDE.md @@ -61,6 +61,15 @@ workflow that has never run gets no frame at all rather than a grey placeholder (a fresh workspace would otherwise be a wall of empty plates). Within a folder, workflows that have produced something sort first. +Stripping the run id is also what makes four runs of one workflow four +identical captions, since a file's name is per step rather than per run. +The gallery entry carries `version` - the run's ordinal, assigned by the +engine and never renumbered - and the grid draws it as a `v4` chip ahead +of the label, with the run id in the detail pane beside it. A job carries +the same number as `run_version`, drawn as `v4` in the jobs list and on the +job page (from the `run_start` event while the job runs). The UI reads the +field only; nothing here computes or orders a version. + Every picture in the app sits in the global `.frame` (app.css): the media fills it edge to edge, with no inner padding and no rounding of its own, which is what makes it read as a proof on a sheet rather than as another diff --git a/ui/e2e/security.spec.ts b/ui/e2e/security.spec.ts new file mode 100644 index 00000000..36f98a62 --- /dev/null +++ b/ui/e2e/security.spec.ts @@ -0,0 +1,248 @@ +import { + expect, + test, + type APIRequestContext, + type Page, +} from '@playwright/test' +import * as fs from 'node:fs' +import * as path from 'node:path' + +/* Hostile content, a real server and a real browser. + * + * Everything the UI renders about a workflow, a prompt, a file or a job is + * text somebody else chose - an MCP agent authoring a workflow, a file name + * on disk, the output of a run. The UI keeps the API token in localStorage, + * so a script that runs on this origin can read it. These specs plant + * payloads in every field an untrusted author controls, walk the pages + * that show them, and fail if any payload executes. + * + * Execution is detected, not inferred: every payload sets + * `document.documentElement.dataset.xss` (and an alert would show up as a + * dialog), and each page check first waits for the payload to be *visible + * as text* - so a page that simply failed to render cannot pass. + * + * Content lives in its own workspace (e2e-xss) and one shared prompt, both + * removed in afterAll, so the other specs never see it. Files on disk go + * into the workspace directory the server reports - the spec runs on the + * same machine as the fixture server. */ + +const WS = 'e2e-xss' +const PROMPT = 'e2e-xss-probe' +const MARK = (id: string) => `document.documentElement.dataset.xss='${id}'` + +// For JSON fields: every shape an HTML sink would execute +const PAYLOAD = (id: string) => + `"'>` + + `javascript:${MARK(id)}` +// For file names: no '/', so no closing tags +const FILE_PAYLOAD = `x"'>` + +// A 1x1 PNG, the same bytes serve_fixture.py writes +const PNG = Buffer.from( + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmM' + + 'IQAAAABJRU5ErkJggg==', + 'base64', +) + +const hostileWorkflow = (id: string) => ({ + id, + description: PAYLOAD('description'), + variables: { note: PAYLOAD('default') }, + steps: [ + { + name: 'compose', + task: { + command: 'compose_text', + arguments: { parts: ['variable:note', PAYLOAD('argument')] }, + }, + result: { content_type: 'text/plain' }, + }, + ], +}) + +let outputsDir = '' +let assetsDir = '' +let htmlOutput = '' +let htmlJobId = '' + +async function waitForJob(request: APIRequestContext, id: string) { + for (let i = 0; i < 240; i++) { + const job = await (await request.get(`/api/jobs/${id}`)).json() + if (['succeeded', 'failed', 'cancelled'].includes(job.status)) return job + await new Promise((resolve) => setTimeout(resolve, 500)) + } + throw new Error(`job ${id} did not finish`) +} + +test.beforeAll(async ({ request }) => { + // the first job spawns the worker, which imports torch + test.setTimeout(180_000) + const created = await request.post('/api/workspaces', { data: { name: WS } }) + expect([201, 409]).toContain(created.status()) + const server = await (await request.get(`/api/server?workspace=${WS}`)).json() + outputsDir = server.directories.outputs + assetsDir = server.directories.assets + expect(outputsDir).toContain(WS) + + // names the server itself will list: gallery files and assets + const outputs = path.join(outputsDir, 'hostile') + fs.mkdirSync(outputs, { recursive: true }) + fs.writeFileSync(path.join(outputs, `${FILE_PAYLOAD}.png`), PNG) + fs.writeFileSync(path.join(outputs, 'note.txt'), PAYLOAD('text-output')) + const assets = assetsDir + fs.mkdirSync(assets, { recursive: true }) + fs.writeFileSync(path.join(assets, `${FILE_PAYLOAD}.png`), PNG) + + // what an MCP agent can author + const saved = await request.put(`/api/workflows/hostile?workspace=${WS}`, { + data: { workflow: hostileWorkflow('hostile') }, + }) + expect(saved.ok()).toBeTruthy() + const prompt = await request.put(`/api/prompts/${PROMPT}`, { + data: { + prompt: { + text: PAYLOAD('prompt-text'), + description: PAYLOAD('prompt-description'), + tags: [PAYLOAD('prompt-tag').slice(0, 60)], + }, + }, + }) + expect(prompt.ok()).toBeTruthy() + + // a text/html result is refused before anything is queued (#407) + const script = `` + const page = (contentType: string) => ({ + id: 'hostile-html', + steps: [ + { + name: 'page', + task: { command: 'compose_text', arguments: { parts: [script] } }, + result: { content_type: contentType }, + }, + ], + }) + const refused = await request.post(`/api/jobs?workspace=${WS}`, { + data: { workflow: page('text/html') }, + }) + expect(refused.status()).toBe(400) + + // the same script as text, and then planted as .html beside it - a file + // that reaches /outputs without passing validation + const job = await request.post(`/api/jobs?workspace=${WS}`, { + data: { workflow: page('text/plain') }, + }) + expect(job.ok()).toBeTruthy() + htmlJobId = (await job.json()).id + const finished = await waitForJob(request, htmlJobId) + expect(finished.status).toBe('succeeded') + const written = fs + .readdirSync(outputsDir, { + recursive: true, + }) + .map(String) + .find((name) => name.includes('hostile-html') && name.endsWith('.txt')) + expect(written).toBeTruthy() + htmlOutput = written!.replace(/\.txt$/, '.html') + fs.copyFileSync( + path.join(outputsDir, written!), + path.join(outputsDir, htmlOutput), + ) +}) + +test.afterAll(async ({ request }) => { + await request.delete(`/api/prompts/${PROMPT}`) + await request.delete(`/api/workspaces/${WS}?acknowledged=true`) +}) + +/** Open a page and fail if anything planted ran. `visible` is text the + * page must show first, so an empty page is a failure, not a pass. */ +async function assertInert(page: Page, hash: string, visible: RegExp) { + const dialogs: string[] = [] + page.on('dialog', async (dialog) => { + dialogs.push(dialog.message()) + await dialog.dismiss() + }) + await page.goto(hash) + await expect(page.getByText(visible).first()).toBeVisible({ + timeout: 20_000, + }) + // give onerror/onload handlers of broken images a chance to fire + await page.waitForLoadState('networkidle') + await page.waitForTimeout(300) + const fired = await page.evaluate( + () => document.documentElement.dataset.xss ?? null, + ) + expect(fired, `payload executed on ${hash}`).toBeNull() + expect(dialogs).toEqual([]) + // nothing the payload spelled became a live element or link + expect(await page.locator('img[src="x"]').count()).toBe(0) + expect(await page.locator('[onerror], [onload]').count()).toBe(0) + expect(await page.locator('a[href^="javascript:" i]').count()).toBe(0) +} + +// Each page, and the literal text it has to show to prove it rendered +const PAGES: [string, string, RegExp][] = [ + ['workflow catalog', `/#/ws/${WS}/workflows`, /onerror=/], + ['workflow page', `/#/ws/${WS}/workflows/hostile`, /onerror=/], + ['gallery', `/#/ws/${WS}/gallery`, /onerror=/], + ['assets', `/#/ws/${WS}/assets`, /onerror=/], + ['prompt library', '/#/shared/prompts', /e2e-xss-probe/], +] + +for (const [label, hash, visible] of PAGES) { + test(`hostile content on the ${label} stays text`, async ({ page }) => { + await assertInert(page, hash, visible) + }) +} + +test('hostile content in the prompt editor stays text', async ({ page }) => { + // the editor shows the text inside form fields, not as page text + await assertInert(page, `/#/shared/prompt-edit/${PROMPT}`, /e2e-xss-probe/) + await expect( + page.getByRole('textbox', { name: 'text', exact: true }), + ).toHaveValue(/onerror=/) +}) + +test('hostile content in the workflow editor stays text', async ({ page }) => { + test.setTimeout(60_000) + await assertInert(page, `/#/ws/${WS}/edit/hostile`, /hostile/) +}) + +test('the job page lists the output as an inert link', async ({ page }) => { + await assertInert(page, `/#/ws/${WS}/jobs/${htmlJobId}`, /hostile-html/) + await expect(page.locator('a.filelink').first()).toBeVisible() +}) + +test('a gallery file with a hostile name opens inert', async ({ page }) => { + await assertInert(page, `/#/ws/${WS}/gallery`, /onerror=/) + await page + .getByText(/onerror=/) + .first() + .click() + await page.waitForTimeout(500) + expect( + await page.evaluate(() => document.documentElement.dataset.xss ?? null), + ).toBeNull() +}) + +test.describe('an html file in outputs', () => { + test('opening the output does not run it on the UI origin', async ({ + page, + }) => { + // entered the way a user does, through the header's token popover - + // the UI is what puts the token where the output's script reads it + await page.goto(`/#/ws/${WS}/overview`) + await page.getByRole('button', { name: 'API token' }).click() + await page.getByPlaceholder('API token').fill('secret-probe') + await page.getByRole('button', { name: 'Save' }).click() + await expect(page.getByRole('button', { name: 'Saved' })).toBeVisible() + await page.goto(`/outputs/${htmlOutput}?workspace=${WS}`) + await page.waitForLoadState('load') + const title = await page.title() + const fired = await page.evaluate( + () => document.documentElement.dataset.xss ?? null, + ) + expect(title).not.toContain('secret-probe') + expect(fired).toBeNull() + }) +}) diff --git a/ui/src/lib/api.ts b/ui/src/lib/api.ts index f3261548..7a567808 100644 --- a/ui/src/lib/api.ts +++ b/ui/src/lib/api.ts @@ -344,6 +344,7 @@ export const api = { commands: string[] image_processors: string[] video_processors: string[] + assessment: string[] }>('/api/tasks'), describeTask: (command: string) => request(`/api/tasks/${encodeURIComponent(command)}`), diff --git a/ui/src/lib/format.test.ts b/ui/src/lib/format.test.ts index 4c05b5e9..de14db25 100644 --- a/ui/src/lib/format.test.ts +++ b/ui/src/lib/format.test.ts @@ -17,7 +17,9 @@ it('formats gigabyte sizes to two decimals at and above 1 GB', () => { expect(formatBytes(1024 ** 3 * 2.5)).toBe('2.50 GB') }) -it('renders a unix timestamp as a locale date/time string', () => { - const mtime = 1700000000 - expect(formatMtime(mtime)).toBe(new Date(mtime * 1000).toLocaleString()) +it('reads mtime as unix seconds, not milliseconds', () => { + // 1700000000 s is 2023-11-14T22:13:20Z; read as ms it would land in January 1970 + const rendered = formatMtime(1700000000) + expect(rendered).toBe(new Date('2023-11-14T22:13:20Z').toLocaleString()) + expect(rendered).not.toBe(new Date(1700000000).toLocaleString()) }) diff --git a/ui/src/lib/pages/GalleryPage.svelte b/ui/src/lib/pages/GalleryPage.svelte index e9f0b1ac..29895125 100644 --- a/ui/src/lib/pages/GalleryPage.svelte +++ b/ui/src/lib/pages/GalleryPage.svelte @@ -319,7 +319,20 @@ {:else} ♪ {file.label} {/if} - {file.label} + + {#if file.version} + v{file.version} + + + {' '} + {/if}{file.label} {/snippet} @@ -352,6 +365,15 @@ class="muted" title="open the file itself in a new tab">open file + {#if selected.version} + + version {selected.version} · {selected.run_id} + {/if} {formatBytes(selected.size)} · {formatMtime(selected.mtime)} @@ -524,6 +546,19 @@ white-space: normal; word-break: break-all; } + /* The one part of the caption that must not be broken or clamped away: + with four runs writing the same name it is the only thing on the card + that differs. Inline-block so word-break: break-all cannot split 'v10' + across lines */ + .version { + display: inline-block; + padding: 0 0.3rem; + border-radius: 0.2rem; + background: var(--line); + color: var(--ink); + font-weight: 600; + word-break: keep-all; + } .detail { position: sticky; bottom: 1rem; diff --git a/ui/src/lib/pages/GalleryPage.test.ts b/ui/src/lib/pages/GalleryPage.test.ts index 16c2f495..3d9d6339 100644 --- a/ui/src/lib/pages/GalleryPage.test.ts +++ b/ui/src/lib/pages/GalleryPage.test.ts @@ -1,5 +1,6 @@ import { cleanup, + fireEvent, render, screen, waitFor, @@ -18,6 +19,8 @@ const file = (name: string, subfolder = ''): GalleryFile => ({ name, folder: name.includes('/') ? name.split('/')[0] : '', subfolder, + run_id: '', + version: null, url: `/outputs/${name}`, kind: 'image', size: 1024, @@ -383,3 +386,49 @@ it('leaves the detail open when Escape answers a confirm dialog', async () => { screen.getByLabelText('delete this file from the output directory'), ).toBeTruthy() }) + +it('marks each file with the version of the run that wrote it', async () => { + // Two runs of one workflow write the same basename - the case where the + // label alone tells a person nothing about which is which + listing.files = [ + { ...file('acorn/r1/film.mp4'), label: 'film.mp4', version: 1 }, + { ...file('acorn/r2/film.mp4'), label: 'film.mp4', version: 4 }, + ] + render(GalleryPage) + await waitFor(() => expect(screen.getAllByText('film.mp4')).toHaveLength(2)) + expect(screen.getByText('v1')).toBeTruthy() + expect(screen.getByText('v4')).toBeTruthy() + // One space between chip and label, so a screen reader does not run + // 'v4' into the file name + const caption = screen.getByText('v4').closest('.caption') as HTMLElement + expect(caption.textContent?.replace(/\s+/g, ' ').trim()).toBe('v4 film.mp4') +}) + +it('shows no version for a flat-layout file, which belongs to no run', async () => { + // The flat layout has no runs to number, and a card must not read + // 'vnull' or 'vundefined' because of it + listing.files = [{ ...file('ltx/flat.png'), label: 'flat.png' }] + render(GalleryPage) + await waitFor(() => expect(screen.getByText('flat.png')).toBeTruthy()) + expect(screen.queryByText(/^v\S+$/)).toBeNull() +}) + +it('names the run and its version in the details of the selected file', async () => { + // "Look at version 4" ends here: the pane says which run it reached, so + // the number in the grid can be checked against the one quoted + listing.files = [ + { + ...file('acorn/r2/film.mp4'), + label: 'film.mp4', + run_id: 'r2', + version: 4, + }, + ] + render(GalleryPage) + await waitFor(() => expect(screen.getByText('film.mp4')).toBeTruthy()) + await fireEvent.click(screen.getByText('film.mp4')) + const detail = document.querySelector('.detail') as HTMLElement + // One span holding both forms, so the query is over its whole text + expect(detail.textContent).toContain('version 4') + expect(detail.querySelector('code')?.textContent).toBe('r2') +}) diff --git a/ui/src/lib/pages/JobPage.svelte b/ui/src/lib/pages/JobPage.svelte index f8a7c07d..2cf92e1f 100644 --- a/ui/src/lib/pages/JobPage.svelte +++ b/ui/src/lib/pages/JobPage.svelte @@ -215,6 +215,13 @@ events.find((e) => e.event === 'workflow_start')?.seed as number | undefined, ) + // The record's number once the job has one; while it runs, the run_start + // event says it first - so the page names the run the moment it opens + const runVersion = $derived( + job?.run_version ?? + (events.find((e) => e.event === 'run_start')?.version as + number | undefined), + ) const etaSeconds = $derived.by(() => { if (!denoise?.total_steps || stepTimes.length < 3) return null const window = stepTimes.slice(-6) @@ -345,6 +352,15 @@ >seed {seed} {/if} + {#if runVersion} + + v{runVersion} + {/if} {#if job.acknowledged === 'bound'} diff --git a/ui/src/lib/pages/JobPage.test.ts b/ui/src/lib/pages/JobPage.test.ts index 6e0f90c7..94fb8e4d 100644 --- a/ui/src/lib/pages/JobPage.test.ts +++ b/ui/src/lib/pages/JobPage.test.ts @@ -407,3 +407,21 @@ it("corrects the URL to the job's own workspace", async () => { render(JobPage, { jobId: 'j1' }) await waitFor(() => expect(location.hash).toBe('#/ws/studio/jobs/j1')) }) + +it('names the run by the version the gallery labels its files with', async () => { + detail.job = { + ...job([]), + run_id: '20260922-120000-aaaaaaaa', + run_version: 4, + } + render(JobPage, { jobId: 'j1' }) + const chip = await waitFor(() => screen.getByText('v4')) + expect(chip.getAttribute('title')).toContain('20260922-120000-aaaaaaaa') +}) + +it('shows no version for a job that never opened a run', async () => { + detail.job = { ...job([]), run_version: null } + render(JobPage, { jobId: 'j1' }) + await waitFor(() => expect(screen.getByText('j1')).toBeTruthy()) + expect(screen.queryByText(/^v\d+$/)).toBeNull() +}) diff --git a/ui/src/lib/pages/JobsPage.svelte b/ui/src/lib/pages/JobsPage.svelte index 3f494da2..9c94b050 100644 --- a/ui/src/lib/pages/JobsPage.svelte +++ b/ui/src/lib/pages/JobsPage.svelte @@ -131,6 +131,13 @@ {job.status} {job.workflow} + {#if job.run_version} + v{job.run_version} + {/if} {#if scope === 'all' && (workspace.names?.length ?? 0) > 1} {job.workspace} {/if} diff --git a/ui/src/lib/pages/PromptEditorPage.svelte b/ui/src/lib/pages/PromptEditorPage.svelte index 70523454..8c98a5bc 100644 --- a/ui/src/lib/pages/PromptEditorPage.svelte +++ b/ui/src/lib/pages/PromptEditorPage.svelte @@ -546,7 +546,7 @@ / {/if} - .json in {promptDir} + .json in {promptDir} {#if savePath()}