From 4fc02835bc5b9764fc951df801ee338e6ebb70b3 Mon Sep 17 00:00:00 2001 From: Hoang Tuan Nguyen Date: Sun, 20 Sep 2026 00:51:56 +0700 Subject: [PATCH 1/4] fix(review): repair every provider path, add per-role AI config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Video review was failing on four independent paths at once, which is why none of them looked fixable from the symptoms. All four verified live against the installed binaries, not inferred. ffmpeg without drawtext. Homebrew's ffmpeg 8.x is built without libfreetype, so `drawtext` does not exist, and naming an absent filter aborts the whole chain ("No such filter: 'drawtext'"). Frame extraction died before any provider ran — the repo's own 13 contact-sheet tests fail on main for this reason. Probed once and cached; without it the sheets lose their burned-in timestamps and the prompt hands the model the frame interval instead, so it can still answer in time ranges. agy was auto-denied. Given a bare file path agy shells out to look at the file; headless mode cannot prompt for that permission, so the tool is denied and the run returns an empty response with exit code 0 and status SUCCESS. Steering it at its own file-reading tool gets the read done unprivileged, so --dangerously-skip-permissions — which auto-approves every tool including arbitrary shell commands, for a job that reads three JPEGs — is gone. Output now comes from --output-format json, and a denied tool is an error whether or not agy still answered: the prompt carries the rubric, both scene prompts and the character names, which is enough to write a plausible review without looking at a frame. codex no longer bypasses its sandbox. -i hands it the image bytes directly, so the run needs neither a shell nor a writable filesystem; --sandbox read-only already implies approval:never. An empty output file is an error instead of a JSON decode failure three frames away. stdin is closed for all three. Each appends piped stdin to the prompt when stdin is not a terminal — codex documents it as a `` block. Two silent-wrong-answer paths in the scoring, same class, both pre-existing: - Every dimension defaults to 5.0, so an answer carrying no `dimensions` became a complete, plausible review — 5.0 across the board, verdict "poor", zero errors — of a video nothing had looked at. Now refused; a partial dimensions object still defaults the unscored axes. - The error parser required the exact keys severity/time_range/description and silently dropped anything else. What it dropped was usually CRITICAL, the one severity that caps character_consistency at 3.0 and forces the verdict below acceptable, so `timeRange` turned an unusable video into a clean pass. The three fields are now handled by what they can cost: near-miss names are normalised, a missing time range or description is repaired and logged, and only a severity outside {CRITICAL, HIGH, MINOR} fails the scene. VideoError.severity is a Literal now, so the three code paths that branch on it cannot be handed anything else. Per-role provider, model and effort. agent/services/cli_providers.py holds everything the three CLIs disagree about; providers.json gains a `roles` map and the dashboard gains a Settings page. Efforts are validated against each CLI's real ladder; models only for agy, whose catalog is closed, so a slug newer than claude's or codex's cache still goes through. agy's model and effort are mutually exclusive — its slugs name their own effort and it rejects the pair. The legacy {"active": ...} switch still works, keeps a model when the CLI did not change, and does not undo a role the same request configured by name. Tests 259 passing / 13 failing on main -> 338 passing, 0 failing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Ka5BxVaWiQeWNDJakpCJJP --- CLAUDE.md | 2 +- README.md | 77 +++- agent/api/providers.py | 169 ++++++- agent/main.py | 2 +- agent/models/review.py | 9 +- agent/providers.json | 9 +- agent/services/cli_providers.py | 289 ++++++++++++ agent/services/video_reviewer.py | 400 +++++++++++++--- dashboard/src/App.tsx | 6 +- dashboard/src/i18n/translations.ts | 267 +++++++++++ dashboard/src/pages/SettingsPage.tsx | 359 +++++++++++++++ dashboard/src/types/index.ts | 47 ++ skills/fk-change-provider.md | 127 +++-- skills/fk-doctor.md | 20 + tests/unit/test_cli_providers.py | 666 +++++++++++++++++++++++++-- tests/unit/test_video_reviewer.py | 170 +++++++ 16 files changed, 2448 insertions(+), 171 deletions(-) create mode 100644 agent/services/cli_providers.py create mode 100644 dashboard/src/pages/SettingsPage.tsx diff --git a/CLAUDE.md b/CLAUDE.md index 4c4ca2052..599bb43dc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -67,7 +67,7 @@ that change how you work: | `/fk-doctor` | Diagnose errors + prescribe fixes (Flow/extension/worker/YT) | | `/fk-add-material` | Set image material style | | `/fk-change-model` | Change video/image model | -| `/fk-change-provider` | View & switch AI CLI provider used for video review (claude/agy/codex) | +| `/fk-change-provider` | View & switch the AI CLI, model and effort per role (claude/agy/codex) | | `/fk-insert-scene` | Insert scenes into chain | | `/fk-upload-image` | Upload local image to get media_id | | `/fk-thumbnail` | Generate YouTube thumbnails | diff --git a/README.md b/README.md index ea66235c6..5c3411477 100644 --- a/README.md +++ b/README.md @@ -499,7 +499,7 @@ Ready-to-use workflow recipes in `skills/` (also available as `/slash-commands` |-------|-------------| | `/fk-review-video` | AI vision scoring of generated scene videos (quality, consistency, usability) — see [AI Vision Providers](#ai-vision-providers-video-review) below | | `/fk-review-board` | Visual scene-by-scene review board for feedback before locking a cut | -| `/fk-change-provider` | View/switch which AI CLI (claude/agy/codex) powers `/fk-review-video` | +| `/fk-change-provider` | View/switch the AI CLI, model and effort behind `/fk-review-video` | ### Reference @@ -556,22 +556,62 @@ Skills are `.md` recipes any AI coding-assistant CLI can read and follow — thi Separate from the table above — this is about **which CLI backend does the vision analysis** for `/fk-review-video`. Three providers are supported and swappable at runtime, no restart required: -| Provider | Binary | Setup | -|----------|--------|-------| -| `claude` | Claude Code CLI | Default — works out of the box | -| `agy` | Google Antigravity CLI | Install separately, sign in once | -| `codex` | OpenAI Codex CLI | `npm install -g @openai/codex`, then `codex login` once | +| Provider | Binary | Reasoning efforts | Model catalog | Setup | +|----------|--------|-------------------|---------------|-------| +| `claude` | Claude Code CLI | `low` `medium` `high` `xhigh` `max` | aliases (`sonnet`, `opus`, `haiku`, `fable`) or any full model name | Default — works out of the box | +| `agy` | Google Antigravity CLI | `low` `medium` `high` | closed — `agy models` is the whole list and agy rejects anything else | Install separately, sign in once | +| `codex` | OpenAI Codex CLI | `low` `medium` `high` `xhigh` `max` (varies per model) | codex's own on-disk cache, plus slugs newer than it | `npm install -g @openai/codex`, then `codex login` once | + +Provider, model and effort are set **per role** — a role being a job an AI CLI +does for Flow Kit. There is one today, `video_review`; the config is a map so +the next one is an entry rather than a schema change. Model and effort may both +be `null`, meaning "whatever that CLI defaults to". + +**For `agy`, model and effort are mutually exclusive.** Its slugs name their own +effort — `gemini-3.8-flash-low`, `gemini-3.1-pro-high` — so setting both is +rejected (`--model gpt-oss-120b-medium conflicts with --effort=low`), and a slug +with no effort in its name refuses `--effort` outright +(`--effort is not supported for model "claude-sonnet-4-6"`). Pick a model, or +pick an effort and let agy choose the model. The API answers 400 for the pair. ```bash -# View provider status (installed / version-tested / currently active) +# View provider status + the current per-role config +# live=true additionally runs ` --version` on each (a few seconds) curl -s "http://127.0.0.1:8100/api/providers?live=true" | python3 -m json.tool -# Switch provider — hot-reloaded immediately, no server restart +# List a provider's models (refresh=true bypasses the 5-minute cache) +curl -s "http://127.0.0.1:8100/api/providers/models?provider=agy" | python3 -m json.tool + +# Point a role at a provider/model/effort — hot-reloaded, no server restart +curl -X PATCH http://127.0.0.1:8100/api/providers \ + -H "Content-Type: application/json" \ + -d '{"roles": {"video_review": {"provider": "claude", "model": "sonnet", "effort": "high"}}}' + +# agy takes a model OR an effort, never both — its slugs name their own effort +curl -X PATCH http://127.0.0.1:8100/api/providers \ + -H "Content-Type: application/json" \ + -d '{"roles": {"video_review": {"provider": "agy", "model": "gemini-3.8-flash-low"}}}' + +# The older whole-agent switch still works. It also clears each role's model +# (a slug means nothing to a different CLI) and drops an effort the new +# provider does not have. curl -X PATCH http://127.0.0.1:8100/api/providers \ -H "Content-Type: application/json" -d '{"active": "agy"}' ``` -Or just run `/fk-change-provider` for an interactive picker. Full details in `skills/fk-change-provider.md`. +Or edit it in the dashboard under **Settings**, or run `/fk-change-provider` for +an interactive picker. Full details in `skills/fk-change-provider.md`. + +**`codex` needs credits on its OpenAI workspace.** `installed: true` only means +the binary is on PATH; a workspace with no balance fails every review with +`ERROR: Your workspace is out of credits`. + +**Contact sheets lose their timestamps on an ffmpeg without `libfreetype`.** +Homebrew's ffmpeg 8.x is one such build: `drawtext` is simply absent, and +naming a filter that does not exist aborts the whole chain. Flow Kit probes for +it once and falls back to untimestamped frames, telling the model the frame +interval instead so it can still answer in time ranges. `ffmpeg -filters | grep +drawtext` shows whether yours has it. ## Video Generation Techniques @@ -668,7 +708,7 @@ agent/ ├── main.py # FastAPI app + WebSocket server ├── config.py # Configuration (loads models.json, providers.json) ├── models.json # Video/upscale/image model mappings -├── providers.json # Active AI CLI provider for video review (claude/agy/codex) +├── providers.json # Per-role AI CLI provider/model/effort (claude/agy/codex) ├── db/ │ ├── schema.py # SQLite schema (aiosqlite) │ └── crud.py # Async CRUD with column whitelisting @@ -679,6 +719,7 @@ agent/ │ ├── flow_client.py # WS bridge to extension │ ├── tts.py # OmniVoice TTS (subprocess-based) │ ├── scene_chain.py # Continuation scene logic +│ ├── cli_providers.py # What each AI CLI accepts; role → provider/model/effort │ ├── video_reviewer.py # AI vision review — contact sheet + claude/agy/codex CLI dispatch │ └── post_process.py # ffmpeg trim/merge/music └── worker/ @@ -861,6 +902,22 @@ From `youtube/upload.py` (HTTP errors from YouTube Data API v3): Dates are merge dates. Older releases are tagged; `git log` is the full record. +### v1.3.0 — 2026-09-20 — video review works again + +Video review had been failing on every path at once, which is why nothing about +it looked fixable from the symptoms. + +| Date | Change | +|---|---| +| 2026-09-20 | **ffmpeg without `drawtext`**: Homebrew's ffmpeg 8.x is built without `libfreetype`, so the timestamp filter does not exist and naming it aborted the whole chain — frame extraction died before any provider was reached. Probed once, with a fallback to untimestamped frames and a prompt that hands the model the frame interval instead | +| 2026-09-20 | **`agy` was auto-denied**: handed a bare file path, agy reaches for a shell command to look at the file, headless mode cannot prompt for that permission, and the run returns an empty response on a **zero** exit code. Fixed by steering it at its own file-reading tool and naming the sheet directory with `--add-dir`, which gets the read done unprivileged — so `--dangerously-skip-permissions`, which auto-approves every tool including arbitrary shell commands, is gone. Output is parsed from `--output-format json`, and a denied tool is an error whether or not agy still answered — the prompt carries the rubric and both scene prompts, so a denied run can write a plausible review from the text alone | +| 2026-09-20 | **`codex` no longer bypasses its sandbox**: `-i` hands codex the image bytes directly, so the run needs neither a shell nor a writable filesystem. `--dangerously-bypass-approvals-and-sandbox` bought nothing and cost the sandbox; `--sandbox read-only` already implies `approval: never`. An empty output file is now an error instead of a JSON decode failure three frames away | +| 2026-09-20 | **A malformed error entry no longer vanishes.** The parser required the exact keys `severity`/`time_range`/`description` and silently dropped anything else — and what it dropped was usually CRITICAL, the one severity that caps `character_consistency` at 3.0 and forces the verdict below acceptable, so `timeRange` instead of `time_range` turned an unusable video into a clean pass. The three fields are now handled by what they can cost: near-miss names are normalised, a missing time range or description is repaired and logged, and only a severity outside `{CRITICAL, HIGH, MINOR}` fails the scene — that is the one field with no safe default, because without it we do not know whether the video passed. `VideoError.severity` is a `Literal` now, so the three code paths that branch on it cannot be handed anything else | +| 2026-09-20 | **A review with no scores in it is now a failure, not a score.** Every dimension defaults to 5.0, so a CLI answer carrying no `dimensions` became a complete, plausible review — 5.0 across the board, verdict "poor", zero errors — of a video nothing had actually looked at | +| 2026-09-20 | **stdin closed for all three CLIs.** Each appends piped stdin to the prompt when stdin is not a terminal — codex documents it as a `` block. Under uvicorn that is whatever the launching shell handed down | +| 2026-09-20 | **Per-role provider, model and effort**, editable in the dashboard under Settings or via `PATCH /api/providers`. Efforts are validated against each CLI's real ladder (agy stops at `high`); models are validated only for agy, whose catalog is closed, so a slug newer than claude's or codex's cache still goes through. A whole-agent `{"active": …}` switch clears each role's model and clamps its effort, because neither survives a change of CLI | +| 2026-09-20 | **agy's model and effort are mutually exclusive** and the config now says so. Its slugs name their own effort (`gemini-3.8-flash-low`), so the pair is rejected — a mismatch conflicts, and a slug with no effort in its name refuses `--effort` at all | + ### v1.2.0 — 2026-09-18 — the Flow migration Flow moved to `flow.google.com` in September 2026 and stopped minting the bearer diff --git a/agent/api/providers.py b/agent/api/providers.py index 697c4f6d4..a16f1430d 100644 --- a/agent/api/providers.py +++ b/agent/api/providers.py @@ -1,14 +1,29 @@ -"""CLI provider config API — view/switch which AI CLI backend runs video review.""" +"""CLI provider config API — which AI CLI, model and effort each role runs on. + +`active` is the legacy single-provider switch that `/fk-change-provider` and the +statusline still use; `roles` is the per-role configuration the dashboard edits. +The two are kept consistent by every write here, so neither reader ever sees a +stale answer. +""" import asyncio import json import logging import shutil from pathlib import Path -from fastapi import APIRouter, HTTPException +from fastapi import APIRouter, HTTPException, Query from agent import config -from agent.services.video_reviewer import PROVIDER_BINARIES +from agent.services.cli_providers import ( + PROVIDER_BINARIES, + PROVIDER_CATALOG_IS_AUTHORITATIVE, + PROVIDER_EFFORTS, + PROVIDER_MODEL_ENCODES_EFFORT, + ROLES, + current_roles, + list_models, + validate_role_entry, +) router = APIRouter(prefix="/api/providers", tags=["providers"]) logger = logging.getLogger(__name__) @@ -16,8 +31,20 @@ def _read() -> dict: + """Read providers.json and hot-reload it into `config.CLI_PROVIDERS`. + + The file is the source of truth, and it is editable by hand (the skills + do), so re-reading it here is what makes a hand edit take effect without a + restart — and what stops a GET describing a state the worker is not in. + """ with open(_PROVIDERS_FILE) as f: - return json.load(f) + data = json.load(f) + data.setdefault("active", "claude") + data.setdefault("roles", {}) + # Same no-await-in-the-gap rule as _apply. + config.CLI_PROVIDERS.clear() + config.CLI_PROVIDERS.update(data) + return data def _write(data: dict): @@ -26,10 +53,26 @@ def _write(data: dict): f.write("\n") +def _apply(data: dict): + """Persist and hot-reload. config.CLI_PROVIDERS is mutated, never rebound, + because callers hold a reference to that same dict. + + The clear/update pair leaves a momentarily empty dict. That is safe only + because no `await` separates the two lines: this runs on the event loop, so + a concurrent `resolve_role` cannot be scheduled into the gap. Do not put an + await between them — a review landing in that window would silently fall + back to the default provider. + """ + _write(data) + config.CLI_PROVIDERS.clear() + config.CLI_PROVIDERS.update(data) + + async def _probe_version(binary: str) -> tuple: try: proc = await asyncio.create_subprocess_exec( binary, "--version", + stdin=asyncio.subprocess.DEVNULL, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, ) _, stderr = await asyncio.wait_for(proc.communicate(), timeout=10) @@ -40,27 +83,117 @@ async def _probe_version(binary: str) -> tuple: @router.get("") async def get_providers(live: bool = False): - """Return provider status: installed (on PATH) and, if live=true, version-tested.""" + """Provider status plus the per-role configuration. + + `live=true` runs a real ` --version` per provider, which costs a + few seconds — it is opt-in for that reason, not the default. + """ data = _read() statuses = {} for name, binary in PROVIDER_BINARIES.items(): installed = shutil.which(binary) is not None tested, error = (await _probe_version(binary)) if (live and installed) else (None, None) - statuses[name] = {"binary": binary, "installed": installed, "tested": tested, "error": error} - return {"active": data["active"], "providers": statuses} + statuses[name] = { + "binary": binary, + "installed": installed, + "tested": tested, + "error": error, + "efforts": list(PROVIDER_EFFORTS[name]), + "default_model": None, + "catalog_is_authoritative": PROVIDER_CATALOG_IS_AUTHORITATIVE[name], + "model_encodes_effort": PROVIDER_MODEL_ENCODES_EFFORT[name], + } + return { + "active": data["active"], + "roles": current_roles(), + "role_meta": ROLES, + "providers": statuses, + } + + +@router.get("/models") +async def get_provider_models( + provider: str = Query(..., description="claude | agy | codex"), + refresh: bool = Query(False, description="Bypass the 5-minute catalog cache"), +): + """List a provider's models. + + An empty list is a normal answer, not an error: it means the binary is + absent or the catalog call failed, and the caller should fall back to the + provider's own default. + """ + if provider not in PROVIDER_BINARIES: + raise HTTPException(400, f"Unknown provider '{provider}'. Known: {sorted(PROVIDER_BINARIES)}") + models = await list_models(provider, force=refresh) + return { + "provider": provider, + "models": models, + "authoritative": PROVIDER_CATALOG_IS_AUTHORITATIVE[provider], + } @router.patch("") async def patch_providers(body: dict): - """Switch the active CLI provider. Body: {"active": "claude"|"agy"|"codex"}.""" - provider = body.get("active") - if provider not in PROVIDER_BINARIES: - raise HTTPException(400, f"Unknown provider '{provider}'. Known: {list(PROVIDER_BINARIES)}") - if not shutil.which(PROVIDER_BINARIES[provider]): - raise HTTPException(400, f"'{PROVIDER_BINARIES[provider]}' binary not found on PATH — install it first") + """Update the active provider, the per-role config, or both. + + Body is either the legacy `{"active": "claude"}` or + `{"roles": {"video_review": {"provider": ..., "model": ..., "effort": ...}}}`. + """ + if not isinstance(body, dict) or ("active" not in body and "roles" not in body): + raise HTTPException(400, "Body must contain 'active', 'roles', or both") + data = _read() - data["active"] = provider - _write(data) - config.CLI_PROVIDERS.clear() - config.CLI_PROVIDERS.update(data) - return {"status": "updated", "active": provider} + roles = dict(data.get("roles") or {}) + + explicit = set() + if "roles" in body: + incoming = body["roles"] + if not isinstance(incoming, dict): + raise HTTPException(400, "'roles' must be an object keyed by role name") + for role, entry in incoming.items(): + try: + roles[role] = validate_role_entry(role, entry) + except ValueError as e: + raise HTTPException(400, str(e)) + explicit = set(incoming) + + if "active" in body: + provider = body["active"] + if provider not in PROVIDER_BINARIES: + raise HTTPException(400, f"Unknown provider '{provider}'. Known: {list(PROVIDER_BINARIES)}") + if not shutil.which(PROVIDER_BINARIES[provider]): + raise HTTPException(400, f"'{PROVIDER_BINARIES[provider]}' binary not found on PATH — install it first") + data["active"] = provider + # Switching the whole agent over has to carry the roles with it. + for role in list(roles) + [r for r in ROLES if r not in roles]: + if role in explicit: + # The same request configured this role by name. That is the + # more specific instruction and it already passed validation — + # the sweep must not undo it. + continue + prev = roles.get(role) or {} + # A model slug only survives if the CLI did not change: agy would + # reject "sonnet" outright. Re-asserting the provider a role is + # already on must not cost the user their model. + model = prev.get("model") if prev.get("provider") == provider else None + effort = prev.get("effort") + if effort not in PROVIDER_EFFORTS[provider]: + effort = None # agy has no xhigh or max + if model and effort and PROVIDER_MODEL_ENCODES_EFFORT[provider]: + # Only reachable from a hand-edited file — the API refuses to + # store the pair — but persisting it would fail the next review. + effort = None + roles[role] = {"provider": provider, "model": model, "effort": effort} + + data["roles"] = roles + + if "active" not in body and roles: + # Keep the legacy field meaningful for /fk-change-provider and the + # statusline: it reports whatever the primary role runs on. + primary = next(iter(ROLES)) + if primary in roles: + data["active"] = roles[primary]["provider"] + + _apply(data) + logger.info("Providers updated: active=%s roles=%s", data["active"], data["roles"]) + return {"status": "updated", "active": data["active"], "roles": data["roles"]} diff --git a/agent/main.py b/agent/main.py index 46f3a2ba8..4d3bc351a 100644 --- a/agent/main.py +++ b/agent/main.py @@ -113,7 +113,7 @@ async def lifespan(app: FastAPI): logger.info("Flow Kit stopped") -app = FastAPI(title="Flow Kit", version="1.2.0", lifespan=lifespan) +app = FastAPI(title="Flow Kit", version="1.3.0", lifespan=lifespan) app.add_middleware( CORSMiddleware, diff --git a/agent/models/review.py b/agent/models/review.py index d834f3f2e..562f2bb14 100644 --- a/agent/models/review.py +++ b/agent/models/review.py @@ -1,6 +1,6 @@ """Pydantic models for video review results.""" from pydantic import BaseModel -from typing import Optional +from typing import Literal, Optional class SegmentScore(BaseModel): @@ -9,7 +9,12 @@ class SegmentScore(BaseModel): class VideoError(BaseModel): - severity: str # CRITICAL / HIGH / MINOR + # Closed on purpose. `has_critical_errors`, the character_consistency cap + # and `_fix_guide` all branch on this exact string, so a severity outside + # the set is not a cosmetic oddity — it silently disables all three. + # Reviews are computed and returned, never read back from storage, so + # narrowing the type cannot break a load of old data. + severity: Literal["CRITICAL", "HIGH", "MINOR"] time_range: str description: str diff --git a/agent/providers.json b/agent/providers.json index 80d74b117..705c0bc33 100644 --- a/agent/providers.json +++ b/agent/providers.json @@ -1,3 +1,10 @@ { - "active": "claude" + "active": "claude", + "roles": { + "video_review": { + "provider": "claude", + "model": null, + "effort": null + } + } } diff --git a/agent/services/cli_providers.py b/agent/services/cli_providers.py new file mode 100644 index 000000000..ea73bc61b --- /dev/null +++ b/agent/services/cli_providers.py @@ -0,0 +1,289 @@ +"""What each AI CLI accepts, and which one a given role runs on. + +Three CLIs back the vision work: Claude Code (`claude`), Google Antigravity +(`agy`) and OpenAI Codex (`codex`). They disagree about almost everything — +flag names, effort ladders, whether an unknown model slug is an error — so the +differences live here rather than being re-derived at each call site. + +Config lives in `agent/providers.json`, hot-reloaded into `config.CLI_PROVIDERS`: + + {"active": "claude", + "roles": {"video_review": {"provider": "agy", "model": null, "effort": "low"}}} + +`active` predates `roles` and is still the fallback for any role without an +entry, so an old providers.json keeps working untouched. +""" +from __future__ import annotations + +import asyncio +import json +import logging +import shutil +import time +from pathlib import Path + +from agent import config + +logger = logging.getLogger(__name__) + + +PROVIDER_BINARIES = { + "claude": "claude", + "agy": "agy", + "codex": "codex", +} + +# Reasoning-effort ladders, as each CLI actually accepts them. agy's own help +# spells out "(low|medium|high)" and it rejects anything outside that; claude +# and codex both take the longer ladder. Codex's real ladder is per-model +# (`supported_reasoning_levels` in its cache) — this is the union, and codex +# itself rejects a level its chosen model does not support. +PROVIDER_EFFORTS = { + "claude": ("low", "medium", "high", "xhigh", "max"), + "agy": ("low", "medium", "high"), + "codex": ("low", "medium", "high", "xhigh", "max"), +} + +# agy's model slugs carry the effort in them — gemini-3.8-flash-low, +# gemini-3.1-pro-high — so --model and --effort are not independent there. +# Verified against agy 1.2.7: a mismatched pair is rejected ("--model +# gpt-oss-120b-medium conflicts with --effort=low") and a model with no effort +# in its name rejects --effort outright ("--effort is not supported for model +# claude-sonnet-4-6"). Even a *matching* pair only restates the slug. So for +# agy, effort is what you set when you have NOT picked a model. +PROVIDER_MODEL_ENCODES_EFFORT = { + "claude": False, + "agy": True, + "codex": False, +} + +# Whether the catalog we can list is the whole truth. agy validates --model +# against `agy models` and errors out on anything else, so its list is closed. +# claude takes aliases (sonnet, opus) as well as full names, and codex takes +# slugs newer than whatever its on-disk cache happens to hold — for those two, +# an unlisted value is a legitimate escape hatch, not a typo to reject. +PROVIDER_CATALOG_IS_AUTHORITATIVE = { + "claude": False, + "agy": True, + "codex": False, +} + +# Claude Code resolves these aliases itself; full model names also work, which +# is why claude's catalog is not authoritative. +_CLAUDE_ALIASES = [ + {"id": "fable", "label": "Fable"}, + {"id": "opus", "label": "Opus"}, + {"id": "sonnet", "label": "Sonnet"}, + {"id": "haiku", "label": "Haiku"}, +] + +_CODEX_MODELS_CACHE = Path.home() / ".codex" / "models_cache.json" + +# Roles are the jobs an AI CLI does for Flow Kit. One today; the shape is a map +# so adding the next one is a dict entry rather than a schema change. +ROLES = { + "video_review": { + "label": "Video Review", + "description": "Vision analysis of contact sheets during scene video review", + }, +} + +DEFAULT_PROVIDER = "claude" + +_CATALOG_TTL_S = 300.0 +# Unlocked on purpose. Two concurrent misses for the same provider both run the +# listing and the second overwrites the first with the same answer — a wasted +# subprocess, never a wrong result. A lock would cost more than it buys. +_catalog_cache: dict[str, tuple[float, list[dict]]] = {} + + +# ── role resolution ─────────────────────────────────────────── + + +def resolve_role(role: str) -> dict: + """Return {provider, model, effort} for `role`. + + Never raises and never returns an unusable provider: a role naming a + provider we do not know, or an effort outside that provider's ladder, is + logged and dropped rather than passed to a subprocess that would reject it + several seconds later with a worse error. + """ + cfg = config.CLI_PROVIDERS + roles = cfg.get("roles") or {} + entry = roles.get(role) or {} + + provider = entry.get("provider") or cfg.get("active") or DEFAULT_PROVIDER + if provider not in PROVIDER_BINARIES: + logger.warning( + "Role %r names unknown provider %r — falling back to %s", + role, provider, DEFAULT_PROVIDER, + ) + provider = DEFAULT_PROVIDER + + effort = entry.get("effort") or None + if effort and effort not in PROVIDER_EFFORTS[provider]: + logger.warning( + "Dropping effort %r for role %r — %s accepts %s", + effort, role, provider, list(PROVIDER_EFFORTS[provider]), + ) + effort = None + + return { + "provider": provider, + "model": entry.get("model") or None, + "effort": effort, + } + + +def validate_role_entry(role: str, entry: dict) -> dict: + """Normalise one role entry from an API body. Raises ValueError on bad input. + + The model is deliberately *not* checked against the catalog for claude and + codex — see PROVIDER_CATALOG_IS_AUTHORITATIVE. + """ + if role not in ROLES: + raise ValueError(f"Unknown role '{role}'. Known: {sorted(ROLES)}") + if not isinstance(entry, dict): + raise ValueError(f"Role '{role}' must be an object, got {type(entry).__name__}") + + provider = entry.get("provider") + if provider not in PROVIDER_BINARIES: + raise ValueError( + f"Unknown provider '{provider}' for role '{role}'. Known: {sorted(PROVIDER_BINARIES)}" + ) + if not shutil.which(PROVIDER_BINARIES[provider]): + raise ValueError( + f"'{PROVIDER_BINARIES[provider]}' binary not found on PATH — install it first" + ) + + effort = entry.get("effort") or None + if effort is not None and effort not in PROVIDER_EFFORTS[provider]: + raise ValueError( + f"Effort '{effort}' is not supported by {provider}. " + f"Supported: {list(PROVIDER_EFFORTS[provider])}" + ) + + model = entry.get("model") or None + if model is not None and effort is not None and PROVIDER_MODEL_ENCODES_EFFORT[provider]: + raise ValueError( + f"{provider} encodes the effort in the model name, so '{model}' and " + f"effort '{effort}' cannot both be set — pick one" + ) + if model is not None and not isinstance(model, str): + raise ValueError(f"Model for role '{role}' must be a string or null") + if model is not None and model.startswith("-"): + # The model is otherwise unvalidated on purpose (the escape hatch for a + # slug newer than any catalog), and it lands in argv next to --model. + # No real slug starts with a dash, and refusing them removes any + # argument left to have about what a CLI's parser does with one. + raise ValueError(f"Model for role '{role}' must not start with '-'") + + return {"provider": provider, "model": model, "effort": effort} + + +def current_roles() -> dict: + """Every known role resolved, including ones absent from providers.json.""" + return {role: resolve_role(role) for role in ROLES} + + +# ── model catalogs ──────────────────────────────────────────── + + +async def _list_agy_models() -> list[dict]: + """Parse `agy models`, whose output is a header line then `\\tLabel`.""" + proc = await asyncio.create_subprocess_exec( + "agy", "models", + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + stdin=asyncio.subprocess.DEVNULL, + ) + try: + stdout, stderr = await asyncio.wait_for(proc.communicate(), timeout=30) + except asyncio.TimeoutError: + proc.kill() + await proc.communicate() + raise RuntimeError("`agy models` timed out after 30s") + if proc.returncode != 0: + raise RuntimeError(f"`agy models` failed (rc={proc.returncode}): {stderr.decode()[-300:]}") + + models = [] + for line in stdout.decode().splitlines(): + # The first line is "Fetching available models..." — no tab, so it and + # any other chatter drop out without needing to be named here. + if "\t" not in line: + continue + slug, _, label = line.partition("\t") + slug, label = slug.strip(), label.strip() + if slug: + models.append({"id": slug, "label": label or slug}) + return models + + +def _list_codex_models() -> list[dict]: + """Read codex's own on-disk catalog. + + `codex models` is interactive — run headlessly it dies with "stdin is not + a terminal" — so the cache the CLI maintains is the only listing available + to a server process. + """ + if not _CODEX_MODELS_CACHE.exists(): + return [] + with open(_CODEX_MODELS_CACHE) as f: + data = json.load(f) + models = [] + for entry in data.get("models") or []: + if not isinstance(entry, dict) or entry.get("visibility") != "list": + continue + slug = entry.get("slug") + if not slug: + continue + efforts = [ + lvl.get("effort") + for lvl in entry.get("supported_reasoning_levels") or [] + if isinstance(lvl, dict) and lvl.get("effort") + ] + models.append({ + "id": slug, + "label": entry.get("display_name") or slug, + "efforts": efforts, + }) + return models + + +async def list_models(provider: str, force: bool = False) -> list[dict]: + """Model catalog for `provider`, TTL-cached. + + An empty catalog is never cached: it means the binary was missing or the + listing call failed, and both are transient in a way a real empty list is + not. + """ + if provider not in PROVIDER_BINARIES: + raise ValueError(f"Unknown provider '{provider}'") + + now = time.monotonic() + if not force: + hit = _catalog_cache.get(provider) + if hit and now - hit[0] < _CATALOG_TTL_S: + return hit[1] + + if not shutil.which(PROVIDER_BINARIES[provider]): + return [] + + try: + if provider == "agy": + models = await _list_agy_models() + elif provider == "codex": + models = await asyncio.to_thread(_list_codex_models) + else: + models = list(_CLAUDE_ALIASES) + except Exception as e: + logger.warning("Model catalog for %s unavailable: %s", provider, e) + return [] + + if models: + _catalog_cache[provider] = (now, models) + return models + + +def clear_catalog_cache() -> None: + _catalog_cache.clear() diff --git a/agent/services/video_reviewer.py b/agent/services/video_reviewer.py index 013d4e15b..f47cfa440 100644 --- a/agent/services/video_reviewer.py +++ b/agent/services/video_reviewer.py @@ -6,9 +6,11 @@ """ import asyncio import base64 +import functools import json import logging import os +import re import subprocess import tempfile from pathlib import Path @@ -31,6 +33,10 @@ ) from agent.db.crud import list_scenes, get_project_characters from agent.models.review import DimensionScores, SceneReview, SegmentScore, VideoError, VideoReview +from agent.services.cli_providers import ( # noqa: F401 (PROVIDER_BINARIES re-exported) + PROVIDER_BINARIES, + resolve_role, +) logger = logging.getLogger(__name__) @@ -189,8 +195,49 @@ def _frame_to_base64(path: Path) -> str: return base64.standard_b64encode(path.read_bytes()).decode() +@functools.lru_cache(maxsize=1) +def _has_drawtext() -> bool: + """Whether this ffmpeg build carries the `drawtext` filter. + + Homebrew's ffmpeg 8.x is built without libfreetype, so `drawtext` is simply + absent and any filter chain naming it aborts with "No such filter: + 'drawtext'". That took down the whole review path, not just the timestamps + it was there to draw. Probe once and degrade to untimestamped frames. + """ + try: + out = subprocess.run( + ["ffmpeg", "-hide_banner", "-filters"], + capture_output=True, text=True, timeout=30, + ) + except (OSError, subprocess.SubprocessError) as e: + logger.warning("Could not list ffmpeg filters (%s) — assuming no drawtext", e) + return False + present = re.search(r"^\s*\S+\s+drawtext\s", out.stdout, re.MULTILINE) is not None + if not present: + logger.warning( + "ffmpeg has no drawtext filter (no libfreetype) — contact sheets will " + "carry no burned-in timestamps; the vision prompt compensates by " + "describing frame order and interval instead" + ) + return present + + +def _frame_filter(fps: float) -> str: + """Filter chain for contact-sheet frames, timestamped where ffmpeg allows.""" + chain = f"fps={fps},scale=320:-1" + if _has_drawtext(): + chain += ( + ",drawtext=text='%{pts\\:hms}':x=5:y=5:fontsize=14:" + "fontcolor=white:borderw=1:bordercolor=black" + ) + return chain + + def _create_contact_sheets(video_path: str, fps: float, out_dir: str) -> tuple[list[Path], int]: - """Extract all frames (timestamped) and tile them into REVIEW_SHEET_COLSxREVIEW_SHEET_ROWS sheets. + """Extract all frames and tile them into REVIEW_SHEET_COLSxREVIEW_SHEET_ROWS sheets. + + Frames carry burned-in timestamps only where ffmpeg has `drawtext` — see + `_frame_filter`. `_analyze_cli` words the prompt to match either way. Returns (sheet_paths in chronological order, total_frames after any REVIEW_MAX_FRAMES cap). """ @@ -198,12 +245,7 @@ def _create_contact_sheets(video_path: str, fps: float, out_dir: str) -> tuple[l frames_dir.mkdir(exist_ok=True) extract_cmd = [ "ffmpeg", "-y", "-i", video_path, - "-vf", ( - f"fps={fps}," - f"scale=320:-1," - f"drawtext=text='%{{pts\\:hms}}':x=5:y=5:fontsize=14:" - f"fontcolor=white:borderw=1:bordercolor=black" - ), + "-vf", _frame_filter(fps), "-q:v", "2", f"{frames_dir}/frame_%04d.jpg", ] @@ -330,6 +372,31 @@ def _build_prompt(n_frames: int, fps: float, n_sheets: int, scene: dict) -> str: ) +# Model answers stray on field *names* far more often than on values. These are +# the near-misses worth absorbing; anything outside the map is left alone and +# judged on its merits below. Keys are compared with underscores stripped and +# lowercased, so `timeRange` and `time_range` land on the same entry. +_ERROR_KEY_ALIASES = { + "timerange": "time_range", + "time": "time_range", + "desc": "description", + "level": "severity", +} +_SEGMENT_KEY_ALIASES = { + "timerange": "time_range", + "time": "time_range", +} +_SEVERITIES = ("CRITICAL", "HIGH", "MINOR") + + +def _normalise_keys(entry: dict, aliases: dict) -> dict: + """Rename near-miss keys onto the names the rubric asked for.""" + return { + aliases.get(str(k).replace("_", "").lower(), k): v + for k, v in entry.items() + } + + def _parse_json_response(raw: str) -> dict: """Extract JSON from a response that may contain markdown fences.""" raw = raw.strip() @@ -348,11 +415,8 @@ def _parse_json_response(raw: str) -> dict: # ─── Backend 1: CLI providers (default, no API key needed) ─── -PROVIDER_BINARIES = { - "claude": "claude", - "agy": "agy", - "codex": "codex", -} +# PROVIDER_BINARIES and resolve_role are imported from agent.services.cli_providers, +# which is where everything the three CLIs disagree about now lives. async def _communicate_with_timeout(proc, provider: str) -> tuple: @@ -365,8 +429,16 @@ async def _communicate_with_timeout(proc, provider: str) -> tuple: async def _spawn_and_check(args: tuple, provider: str) -> bytes: + # stdin is closed deliberately. All three CLIs read piped stdin when it is + # not a terminal and append it to the prompt — codex says so in its own + # --help ("stdin is appended as a `` block"). Under uvicorn stdin is + # whatever the launching shell handed down, which is nothing we want in a + # vision prompt. proc = await asyncio.create_subprocess_exec( - *args, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, + *args, + stdin=asyncio.subprocess.DEVNULL, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, ) stdout, stderr = await _communicate_with_timeout(proc, provider) if proc.returncode != 0: @@ -374,38 +446,153 @@ async def _spawn_and_check(args: tuple, provider: str) -> bytes: return stdout -async def _run_claude_cli(prompt: str) -> str: - stdout = await _spawn_and_check( - ("claude", "-p", prompt, "--allowedTools", "Read", "--output-format", "text"), "claude" - ) +def _model_effort_args(model: str | None, effort: str | None) -> list: + """claude and agy happen to spell these the same way; codex does not.""" + args = [] + if model: + args += ["--model", model] + if effort: + args += ["--effort", effort] + return args + + +async def _run_claude_cli( + prompt: str, + *, + model: str | None = None, + effort: str | None = None, + add_dirs: tuple = (), +) -> str: + args = ["claude", "-p", prompt, "--allowedTools", "Read", "--output-format", "text"] + for d in add_dirs: + args += ["--add-dir", str(d)] + args += _model_effort_args(model, effort) + stdout = await _spawn_and_check(tuple(args), "claude") return stdout.decode() -async def _run_agy_cli(prompt: str) -> str: - stdout = await _spawn_and_check( - ("agy", "-p", prompt, "--dangerously-skip-permissions", "--output-format", "text"), "agy" - ) - return stdout.decode() +# agy is agentic: handed a bare file path it reaches for a shell command to look +# at the file, headless mode cannot prompt for that permission, so the tool is +# auto-denied and the run returns an empty response on a *zero* exit code. +# Pointing it at its own file-reading tool is what makes the read happen +# unprivileged — verified against agy 1.2.7. The remedy agy itself suggests in +# that error, --dangerously-skip-permissions, auto-approves every tool including +# arbitrary shell commands, for a job whose whole need is reading three JPEGs. +_AGY_READ_STEER = ( + "Use your file-reading tool to read the image file(s). " + "Do NOT run any shell command." +) -async def _run_codex_cli(prompt: str, contact_sheets: list[Path]) -> str: +def _parse_agy_envelope(raw: str) -> str: + """Pull the answer out of `agy --output-format json`, or explain the silence.""" + raw = raw.strip() + if not raw: + raise RuntimeError("agy CLI returned no output") + try: + env = json.loads(raw) + except json.JSONDecodeError: + # Permission refusals and startup errors arrive as bare prose on a zero + # exit code, so _spawn_and_check never sees them. They land here. + raise RuntimeError(f"agy CLI returned non-JSON output: {raw[:300]}") + if not isinstance(env, dict): + raise RuntimeError(f"agy CLI returned unexpected JSON: {raw[:300]}") + + response = (env.get("response") or "").strip() + denied = env.get("denied_actions") or [] + if denied: + # Any denial at all, answer or no answer. The prompt tells agy to read + # the sheets with its file-reading tool and run no shell command, so a + # denial means the steering did not hold — and the prompt also carries + # the full scoring rubric, the scene's image prompt, its video prompt + # and the character names. That is more than enough for a plausible + # review written entirely from the text, without the images ever being + # looked at. A denial plus a confident answer is the more dangerous of + # the two cases, not the safer one. + # + # status stays "SUCCESS" here, which is why this is caught by name + # rather than by the status field. + names = ", ".join( + str(d.get("display_name") or d.get("action")) + for d in denied if isinstance(d, dict) + ) + raise RuntimeError( + f"agy CLI had tools auto-denied headlessly ({names or denied}) — " + f"its {len(response)}-character answer cannot be trusted to have " + f"come from the images" + ) + status = env.get("status") + if status is not None and status != "SUCCESS": + raise RuntimeError(f"agy CLI reported status={status!r}: {raw[:300]}") + if not response: + raise RuntimeError("agy CLI returned an empty response") + return response + + +async def _run_agy_cli( + prompt: str, + *, + model: str | None = None, + effort: str | None = None, + add_dirs: tuple = (), +) -> str: + # --print-timeout is set just inside our own wait_for so agy gets to finish + # and report on its own terms rather than being SIGKILLed with its answer + # still buffered. Left unset it defaults to 0, meaning "wait forever". + print_timeout = max(10, int(REVIEW_CLI_TIMEOUT_S) - 5) + args = [ + "agy", "-p", prompt, + "--output-format", "json", + "--print-timeout", f"{print_timeout}s", + ] + for d in add_dirs: + args += ["--add-dir", str(d)] + if model and effort: + # agy's slugs carry the effort (gemini-3.8-flash-low), so the pair is + # rejected unless it is redundant. The API refuses to store both; this + # covers a hand-edited providers.json, which is supported. + logger.warning( + "Dropping effort %r — agy model %r already names its effort", effort, model + ) + effort = None + args += _model_effort_args(model, effort) + stdout = await _spawn_and_check(tuple(args), "agy") + return _parse_agy_envelope(stdout.decode()) + + +async def _run_codex_cli( + prompt: str, + contact_sheets: list, + *, + model: str | None = None, + effort: str | None = None, +) -> str: with tempfile.NamedTemporaryFile(suffix=".txt", delete=False) as tmp: out_path = Path(tmp.name) try: - image_args = [] + # read-only, not --dangerously-bypass-approvals-and-sandbox: -i hands + # codex the image bytes directly, so the run needs no filesystem write + # and no shell at all. read-only still implies approval:never, so it + # cannot hang waiting for a prompt. --skip-git-repo-check keeps the run + # working if the server is ever started outside a git checkout. + args = ["codex", "exec", "--skip-git-repo-check", "--sandbox", "read-only"] for sheet in contact_sheets: - image_args.extend(["-i", str(sheet)]) - await _spawn_and_check( - ( - "codex", "exec", - *image_args, - "-o", str(out_path), - "--dangerously-bypass-approvals-and-sandbox", - prompt, - ), - "codex", - ) - return out_path.read_text() + args += ["-i", str(sheet)] + args += ["-o", str(out_path)] + if model: + args += ["-m", model] + if effort: + # codex has no --effort; reasoning level is a config override, and + # -c parses its value as TOML, hence the quotes around the string. + args += ["-c", f'model_reasoning_effort="{effort}"'] + args.append(prompt) + await _spawn_and_check(tuple(args), "codex") + answer = out_path.read_text().strip() + if not answer: + raise RuntimeError( + "codex CLI exited cleanly but wrote no answer to its output file" + ) + return answer finally: out_path.unlink(missing_ok=True) @@ -415,31 +602,61 @@ async def _analyze_cli( n_frames: int, fps: float, scene: dict, + timestamped: bool | None = None, ) -> dict: - """Analyze contact sheets via the active CLI provider (claude/agy/codex).""" + """Analyze contact sheets via the CLI provider configured for video_review.""" + if timestamped is None: + timestamped = _has_drawtext() n_sheets = len(contact_sheets) base_prompt = _build_prompt(n_frames, fps, n_sheets, scene) - provider = config.CLI_PROVIDERS["active"] - logger.info("Calling %s CLI for vision analysis (%d frames, %d sheets)", provider, n_frames, n_sheets) + + role = resolve_role("video_review") + provider = role["provider"] + logger.info( + "Calling %s CLI for vision analysis (%d frames, %d sheets, model=%s, effort=%s, timestamps=%s)", + provider, n_frames, n_sheets, + role["model"] or "default", role["effort"] or "default", timestamped, + ) + + if timestamped: + stamp_note = "with timestamps" + else: + # Without burned-in timestamps the model still has to answer in + # time_range, so hand it the arithmetic instead of the labels. + stamp_note = ( + f"without timestamps — frames run left to right then top to bottom, " + f"one every {1 / fps:.2f}s, so frame N starts at (N-1)*{1 / fps:.2f}s" + ) + if n_sheets == 1: - sheet_intro = f"It is a contact sheet of {n_frames} video frames at {fps}fps with timestamps." + sheet_intro = f"It is a contact sheet of {n_frames} video frames at {fps}fps {stamp_note}." else: sheet_intro = ( f"These are {n_sheets} sequential contact sheets covering {n_frames} video frames " - f"at {fps}fps with timestamps, in chronological order (sheet 1 is earliest)." + f"at {fps}fps {stamp_note}, in chronological order (sheet 1 is earliest)." ) + if provider == "codex": full_prompt = f"{sheet_intro}\n\n{base_prompt}" - raw = await _run_codex_cli(full_prompt, contact_sheets) + raw = await _run_codex_cli( + full_prompt, contact_sheets, model=role["model"], effort=role["effort"] + ) else: if n_sheets == 1: read_instruction = f"Read the image at {contact_sheets[0]}." else: sheet_list = ", ".join(str(s) for s in contact_sheets) read_instruction = f"Read the images at: {sheet_list}, in that order." + if provider == "agy": + read_instruction = f"{read_instruction} {_AGY_READ_STEER}" full_prompt = f"{read_instruction} {sheet_intro}\n\n{base_prompt}" + # The sheets live in a TemporaryDirectory outside the server's cwd; + # naming it keeps the read inside a workspace the CLI was told about. + add_dirs = tuple(sorted({str(Path(s).parent) for s in contact_sheets})) runner = {"claude": _run_claude_cli, "agy": _run_agy_cli}[provider] - raw = await runner(full_prompt) + raw = await runner( + full_prompt, model=role["model"], effort=role["effort"], add_dirs=add_dirs + ) return _parse_json_response(raw) @@ -541,22 +758,64 @@ async def review_scene_video( logger.info("Analyzing %d frames across %d sheets via CLI provider", n_frames, len(contact_sheets)) result = await _analyze_cli(contact_sheets, n_frames, fps, scene) - # Parse structured errors with severity + # Parse structured errors with severity. + # + # The three fields are NOT equal, so they are not treated equally. Only + # `severity` can change the outcome: CRITICAL is what caps + # character_consistency at 3.0 and forces the score below "acceptable". + # This block used to require all three keys exactly and silently drop any + # entry that missed one, so a model writing `timeRange` turned "this video + # is unusable" into a clean pass. Now a missing time_range or description + # is repaired and logged — losing those costs the reader context, not the + # verdict — while an unrecognisable severity fails the scene, because at + # that point we genuinely do not know whether the video passed. errors = [] - for e in result.get("errors", []): - if isinstance(e, dict) and "severity" in e and "time_range" in e and "description" in e: - errors.append(VideoError( - severity=e["severity"].upper(), - time_range=e["time_range"], - description=e["description"], - )) - elif isinstance(e, str): - # Fallback: plain string from older response format + repaired_fields = 0 + for e in result.get("errors") or []: + if isinstance(e, str): + # Legacy shape: a bare sentence, no severity to lose. errors.append(VideoError(severity="HIGH", time_range="?", description=e)) + continue + if not isinstance(e, dict): + raise RuntimeError( + f"review answer had an unreadable error entry: {str(e)[:200]}" + ) + + entry = _normalise_keys(e, _ERROR_KEY_ALIASES) + severity = str(entry.get("severity", "")).strip().upper() + if severity not in _SEVERITIES: + raise RuntimeError( + f"review answer had an error entry with no usable severity " + f"(expected one of {list(_SEVERITIES)}): {str(e)[:200]}" + ) + + time_range = entry.get("time_range") + if not isinstance(time_range, str) or not time_range.strip(): + time_range = "?" # the same placeholder the legacy string path uses + repaired_fields += 1 + description = entry.get("description") + if not isinstance(description, str) or not description.strip(): + description = "(no description given)" + repaired_fields += 1 + + errors.append(VideoError( + severity=severity, time_range=time_range, description=description, + )) has_critical = any(e.severity == "CRITICAL" for e in errors) - dims_raw = result.get("dimensions", {}) + dims_raw = result.get("dimensions") or {} + if not isinstance(dims_raw, dict) or not dims_raw: + # Every field below has a 5.0 default, so an answer with no dimensions + # at all becomes a complete, plausible review: 5.0 across the board, + # verdict "poor", no errors. That is a fabricated score wearing the + # shape of a real one, and it reads as a bad video rather than a failed + # review. Refuse it — review_video logs and skips the scene. + raise RuntimeError( + f"{'CLI' if not ANTHROPIC_API_KEY else 'SDK'} answer had no dimensions: " + f"{str(result)[:300]}" + ) + dims = DimensionScores( character_consistency=float(dims_raw.get("character_consistency", 5.0)), prompt_adherence=float(dims_raw.get("prompt_adherence", 5.0)), @@ -577,11 +836,34 @@ async def review_scene_video( if has_critical and overall > 5.9: overall = 5.9 - usable_segments = [ - SegmentScore(time_range=s["time_range"], score=float(s["score"])) - for s in result.get("usable_segments", []) - if isinstance(s, dict) and "time_range" in s and "score" in s - ] + # A segment that cannot be read is dropped rather than raised on: losing one + # errs toward "less usable footage", which cannot turn a bad video into a + # good score the way a lost CRITICAL can. It is still logged — the absence + # of any signal is what kept the error-entry version of this invisible. + usable_segments = [] + dropped_segments = 0 + for seg in result.get("usable_segments") or []: + if not isinstance(seg, dict): + dropped_segments += 1 + continue + norm = _normalise_keys(seg, _SEGMENT_KEY_ALIASES) + try: + score = float(norm["score"]) + except (KeyError, TypeError, ValueError): + dropped_segments += 1 + continue + time_range = norm.get("time_range") + usable_segments.append(SegmentScore( + time_range=time_range if isinstance(time_range, str) and time_range.strip() else "?", + score=score, + )) + + if repaired_fields or dropped_segments: + logger.warning( + "Scene %s: review answer needed repair — %d error field(s) defaulted, " + "%d usable segment(s) unreadable", + scene["id"], repaired_fields, dropped_segments, + ) return SceneReview( scene_id=scene["id"], diff --git a/dashboard/src/App.tsx b/dashboard/src/App.tsx index b0b0125b8..46e3cf333 100644 --- a/dashboard/src/App.tsx +++ b/dashboard/src/App.tsx @@ -1,6 +1,6 @@ import { useState, useEffect } from 'react' import { BrowserRouter, NavLink, Routes, Route, useLocation, useParams, useSearchParams } from 'react-router-dom' -import { LayoutDashboard, FolderOpen, Film, ScrollText, BookOpen } from 'lucide-react' +import { LayoutDashboard, FolderOpen, Film, ScrollText, BookOpen, SlidersHorizontal } from 'lucide-react' import { TooltipProvider } from '@/components/ui/tooltip' import { WebSocketProvider } from './api/WebSocketContext' import { useWebSocketContext } from './api/useWebSocketContext' @@ -15,6 +15,7 @@ import ProjectsPage from './pages/ProjectsPage' import LogsPage from './pages/LogsPage' import GalleryPage from './pages/GalleryPage' import GuidePage from './pages/GuidePage' +import SettingsPage from './pages/SettingsPage' const NAV: { to: string; icon: typeof LayoutDashboard; labelKey: TranslationKey; exact: boolean }[] = [ { to: '/', icon: LayoutDashboard, labelKey: 'nav.dashboard', exact: true }, @@ -22,6 +23,7 @@ const NAV: { to: string; icon: typeof LayoutDashboard; labelKey: TranslationKey; { to: '/gallery', icon: Film, labelKey: 'nav.gallery', exact: false }, { to: '/logs', icon: ScrollText, labelKey: 'nav.logs', exact: false }, { to: '/guide', icon: BookOpen, labelKey: 'nav.guide', exact: false }, + { to: '/settings', icon: SlidersHorizontal, labelKey: 'nav.settings', exact: false }, ] const BREADCRUMB_TAB_KEY: Record = { @@ -65,6 +67,7 @@ function useBreadcrumbs() { } else if (loc.pathname.startsWith('/gallery')) crumbs.push(t('app.breadcrumb.gallery')) else if (loc.pathname.startsWith('/logs')) crumbs.push(t('app.breadcrumb.logs')) else if (loc.pathname.startsWith('/guide')) crumbs.push(t('app.breadcrumb.guide')) + else if (loc.pathname.startsWith('/settings')) crumbs.push(t('app.breadcrumb.settings')) return crumbs } @@ -187,6 +190,7 @@ function Layout() { } /> } /> } /> + } /> diff --git a/dashboard/src/i18n/translations.ts b/dashboard/src/i18n/translations.ts index 695be91a7..c1d240b39 100644 --- a/dashboard/src/i18n/translations.ts +++ b/dashboard/src/i18n/translations.ts @@ -52,6 +52,7 @@ const en = { 'nav.gallery': 'Gallery', 'nav.logs': 'Logs', 'nav.guide': 'Guide', + 'nav.settings': 'Settings', 'app.brandName': 'FLOW KIT', 'app.brandTag': 'ops console', 'app.breadcrumbRoot': 'flow kit', @@ -60,6 +61,7 @@ const en = { 'app.breadcrumb.gallery': 'gallery', 'app.breadcrumb.logs': 'logs', 'app.breadcrumb.guide': 'guide', + 'app.breadcrumb.settings': 'settings', 'app.workers': 'WORKERS', 'app.extensionConnected': 'extension connected', 'app.extensionDisconnected': 'extension disconnected', @@ -296,6 +298,43 @@ const en = { 'sceneSheet.retrying': 'Retrying…', 'sceneSheet.retryStage': 'Retry stage', + // ---- Settings (AI providers) ---- + 'settings.title': 'AI PROVIDERS', + 'settings.desc': 'Which CLI agent runs each automated role, and with what model and effort level.', + 'settings.loading': 'Loading provider settings...', + 'settings.loadFailed': 'Could not load provider settings.', + 'settings.retry': 'Retry', + 'settings.activeProvider': 'ACTIVE', + 'settings.test': 'Test binaries', + 'settings.testing': 'Testing…', + 'settings.testHint': 'Runs --version for every provider. Takes a few seconds.', + 'settings.providers.title': 'PROVIDER BINARIES', + 'settings.providers.installed': 'installed', + 'settings.providers.missing': 'not installed', + 'settings.providers.untested': 'not tested', + 'settings.providers.ok': 'responded', + 'settings.providers.failed': 'failed', + 'settings.field.agent': 'AGENT', + 'settings.field.model': 'MODEL', + 'settings.field.effort': 'EFFORT', + 'settings.effortLockedHint': '{binary} model names already include the effort.', + 'settings.optionDefault': 'Default', + 'settings.optionCustom': 'Custom…', + 'settings.notInstalled': 'not installed', + 'settings.installHint': 'Install the {binary} CLI before assigning this role to it.', + 'settings.modelsLoading': 'Loading models…', + 'settings.modelsEmpty': 'No catalog available for this provider.', + 'settings.modelsFreeText': 'This provider has no authoritative catalog — a custom model slug is accepted.', + 'settings.modelCustomPlaceholder': 'model slug', + 'settings.modelBackToList': 'Pick from list', + 'settings.refreshModels': 'Refresh model catalog', + 'settings.save': 'Save', + 'settings.saving': 'Saving…', + 'settings.saved': 'Saved', + 'settings.unsaved': 'UNSAVED', + 'settings.inSync': 'IN SYNC', + 'settings.empty': 'No roles are configured.', + // ---- EditableText ---- 'editableText.clickToEdit': 'Click to edit', 'editableText.empty': '(empty)', @@ -341,6 +380,7 @@ const vi: Partial> = { 'nav.gallery': 'Thư viện', 'nav.logs': 'Nhật ký', 'nav.guide': 'Hướng dẫn', + 'nav.settings': 'Cài đặt', 'app.brandName': 'FLOW KIT', 'app.brandTag': 'ops console', 'app.breadcrumbRoot': 'flow kit', @@ -349,6 +389,7 @@ const vi: Partial> = { 'app.breadcrumb.gallery': 'thư viện', 'app.breadcrumb.logs': 'nhật ký', 'app.breadcrumb.guide': 'hướng dẫn', + 'app.breadcrumb.settings': 'cài đặt', 'app.workers': 'WORKERS', 'app.extensionConnected': 'extension đã kết nối', 'app.extensionDisconnected': 'extension chưa kết nối', @@ -575,6 +616,42 @@ const vi: Partial> = { 'sceneSheet.retrying': 'Đang thử lại…', 'sceneSheet.retryStage': 'Thử lại giai đoạn', + 'settings.title': 'NHÀ CUNG CẤP AI', + 'settings.desc': 'Chọn CLI agent chạy từng vai trò tự động, cùng model và mức nỗ lực.', + 'settings.loading': 'Đang tải cài đặt nhà cung cấp...', + 'settings.loadFailed': 'Không tải được cài đặt nhà cung cấp.', + 'settings.retry': 'Thử lại', + 'settings.activeProvider': 'ĐANG DÙNG', + 'settings.test': 'Kiểm tra binary', + 'settings.testing': 'Đang kiểm tra…', + 'settings.testHint': 'Chạy --version cho từng nhà cung cấp. Mất vài giây.', + 'settings.providers.title': 'BINARY NHÀ CUNG CẤP', + 'settings.providers.installed': 'đã cài', + 'settings.providers.missing': 'chưa cài', + 'settings.providers.untested': 'chưa kiểm tra', + 'settings.providers.ok': 'phản hồi tốt', + 'settings.providers.failed': 'lỗi', + 'settings.field.agent': 'AGENT', + 'settings.field.model': 'MODEL', + 'settings.field.effort': 'MỨC NỖ LỰC', + 'settings.effortLockedHint': 'Tên model của {binary} đã bao gồm mức nỗ lực.', + 'settings.optionDefault': 'Mặc định', + 'settings.optionCustom': 'Tuỳ chỉnh…', + 'settings.notInstalled': 'chưa cài', + 'settings.installHint': 'Hãy cài CLI {binary} trước khi giao vai trò này cho nó.', + 'settings.modelsLoading': 'Đang tải danh sách model…', + 'settings.modelsEmpty': 'Không có danh mục model cho nhà cung cấp này.', + 'settings.modelsFreeText': 'Nhà cung cấp này không có danh mục chuẩn — có thể nhập model tuỳ ý.', + 'settings.modelCustomPlaceholder': 'mã model', + 'settings.modelBackToList': 'Chọn từ danh sách', + 'settings.refreshModels': 'Làm mới danh mục model', + 'settings.save': 'Lưu', + 'settings.saving': 'Đang lưu…', + 'settings.saved': 'Đã lưu', + 'settings.unsaved': 'CHƯA LƯU', + 'settings.inSync': 'ĐÃ ĐỒNG BỘ', + 'settings.empty': 'Chưa có vai trò nào được cấu hình.', + 'editableText.clickToEdit': 'Nhấn để sửa', 'editableText.empty': '(trống)', } @@ -615,6 +692,7 @@ const hi: Partial> = { 'nav.gallery': 'गैलरी', 'nav.logs': 'लॉग्स', 'nav.guide': 'गाइड', + 'nav.settings': 'सेटिंग्स', 'app.brandName': 'FLOW KIT', 'app.brandTag': 'ops console', 'app.breadcrumbRoot': 'flow kit', @@ -623,6 +701,7 @@ const hi: Partial> = { 'app.breadcrumb.gallery': 'गैलरी', 'app.breadcrumb.logs': 'लॉग्स', 'app.breadcrumb.guide': 'गाइड', + 'app.breadcrumb.settings': 'सेटिंग्स', 'app.workers': 'वर्कर्स', 'app.extensionConnected': 'एक्सटेंशन जुड़ा है', 'app.extensionDisconnected': 'एक्सटेंशन नहीं जुड़ा है', @@ -849,6 +928,42 @@ const hi: Partial> = { 'sceneSheet.retrying': 'पुनः प्रयास हो रहा है…', 'sceneSheet.retryStage': 'स्टेज पुनः प्रयास करें', + 'settings.title': 'AI प्रदाता', + 'settings.desc': 'कौन सा CLI एजेंट हर स्वचालित भूमिका चलाए, और किस मॉडल व प्रयास स्तर के साथ।', + 'settings.loading': 'प्रदाता सेटिंग्स लोड हो रही हैं...', + 'settings.loadFailed': 'प्रदाता सेटिंग्स लोड नहीं हो सकीं।', + 'settings.retry': 'पुनः प्रयास', + 'settings.activeProvider': 'सक्रिय', + 'settings.test': 'बाइनरी जाँचें', + 'settings.testing': 'जाँच हो रही है…', + 'settings.testHint': 'हर प्रदाता के लिए --version चलाता है। इसमें कुछ सेकंड लगते हैं।', + 'settings.providers.title': 'प्रदाता बाइनरी', + 'settings.providers.installed': 'इंस्टॉल है', + 'settings.providers.missing': 'इंस्टॉल नहीं है', + 'settings.providers.untested': 'जाँचा नहीं गया', + 'settings.providers.ok': 'उत्तर मिला', + 'settings.providers.failed': 'विफल', + 'settings.field.agent': 'एजेंट', + 'settings.field.model': 'मॉडल', + 'settings.field.effort': 'प्रयास', + 'settings.effortLockedHint': '{binary} के मॉडल नामों में प्रयास स्तर पहले से शामिल है।', + 'settings.optionDefault': 'डिफ़ॉल्ट', + 'settings.optionCustom': 'कस्टम…', + 'settings.notInstalled': 'इंस्टॉल नहीं है', + 'settings.installHint': 'यह भूमिका सौंपने से पहले {binary} CLI इंस्टॉल करें।', + 'settings.modelsLoading': 'मॉडल लोड हो रहे हैं…', + 'settings.modelsEmpty': 'इस प्रदाता के लिए कोई कैटलॉग उपलब्ध नहीं है।', + 'settings.modelsFreeText': 'इस प्रदाता की कोई आधिकारिक सूची नहीं है — कस्टम मॉडल स्लग स्वीकार्य है।', + 'settings.modelCustomPlaceholder': 'मॉडल स्लग', + 'settings.modelBackToList': 'सूची से चुनें', + 'settings.refreshModels': 'मॉडल कैटलॉग ताज़ा करें', + 'settings.save': 'सहेजें', + 'settings.saving': 'सहेजा जा रहा है…', + 'settings.saved': 'सहेजा गया', + 'settings.unsaved': 'असहेजा', + 'settings.inSync': 'समन्वयित', + 'settings.empty': 'कोई भूमिका कॉन्फ़िगर नहीं है।', + 'editableText.clickToEdit': 'संपादित करने के लिए क्लिक करें', 'editableText.empty': '(खाली)', } @@ -889,6 +1004,7 @@ const id: Partial> = { 'nav.gallery': 'Galeri', 'nav.logs': 'Log', 'nav.guide': 'Panduan', + 'nav.settings': 'Pengaturan', 'app.brandName': 'FLOW KIT', 'app.brandTag': 'ops console', 'app.breadcrumbRoot': 'flow kit', @@ -897,6 +1013,7 @@ const id: Partial> = { 'app.breadcrumb.gallery': 'galeri', 'app.breadcrumb.logs': 'log', 'app.breadcrumb.guide': 'panduan', + 'app.breadcrumb.settings': 'pengaturan', 'app.workers': 'WORKER', 'app.extensionConnected': 'ekstensi terhubung', 'app.extensionDisconnected': 'ekstensi tidak terhubung', @@ -1123,6 +1240,42 @@ const id: Partial> = { 'sceneSheet.retrying': 'Mencoba lagi…', 'sceneSheet.retryStage': 'Coba lagi tahap ini', + 'settings.title': 'PENYEDIA AI', + 'settings.desc': 'Agen CLI mana yang menjalankan tiap peran otomatis, beserta model dan tingkat usahanya.', + 'settings.loading': 'Memuat pengaturan penyedia...', + 'settings.loadFailed': 'Gagal memuat pengaturan penyedia.', + 'settings.retry': 'Coba lagi', + 'settings.activeProvider': 'AKTIF', + 'settings.test': 'Uji biner', + 'settings.testing': 'Menguji…', + 'settings.testHint': 'Menjalankan --version untuk setiap penyedia. Butuh beberapa detik.', + 'settings.providers.title': 'BINER PENYEDIA', + 'settings.providers.installed': 'terpasang', + 'settings.providers.missing': 'belum terpasang', + 'settings.providers.untested': 'belum diuji', + 'settings.providers.ok': 'merespons', + 'settings.providers.failed': 'gagal', + 'settings.field.agent': 'AGEN', + 'settings.field.model': 'MODEL', + 'settings.field.effort': 'USAHA', + 'settings.effortLockedHint': 'Nama model {binary} sudah memuat tingkat usahanya.', + 'settings.optionDefault': 'Bawaan', + 'settings.optionCustom': 'Kustom…', + 'settings.notInstalled': 'belum terpasang', + 'settings.installHint': 'Pasang CLI {binary} sebelum menugaskan peran ini kepadanya.', + 'settings.modelsLoading': 'Memuat model…', + 'settings.modelsEmpty': 'Tidak ada katalog untuk penyedia ini.', + 'settings.modelsFreeText': 'Penyedia ini tidak punya katalog resmi — slug model kustom tetap diterima.', + 'settings.modelCustomPlaceholder': 'slug model', + 'settings.modelBackToList': 'Pilih dari daftar', + 'settings.refreshModels': 'Muat ulang katalog model', + 'settings.save': 'Simpan', + 'settings.saving': 'Menyimpan…', + 'settings.saved': 'Tersimpan', + 'settings.unsaved': 'BELUM DISIMPAN', + 'settings.inSync': 'SINKRON', + 'settings.empty': 'Belum ada peran yang dikonfigurasi.', + 'editableText.clickToEdit': 'Klik untuk mengedit', 'editableText.empty': '(kosong)', } @@ -1163,6 +1316,7 @@ const zh: Partial> = { 'nav.gallery': '素材库', 'nav.logs': '日志', 'nav.guide': '使用指南', + 'nav.settings': '设置', 'app.brandName': 'FLOW KIT', 'app.brandTag': 'ops console', 'app.breadcrumbRoot': 'flow kit', @@ -1171,6 +1325,7 @@ const zh: Partial> = { 'app.breadcrumb.gallery': '素材库', 'app.breadcrumb.logs': '日志', 'app.breadcrumb.guide': '使用指南', + 'app.breadcrumb.settings': '设置', 'app.workers': '工作进程', 'app.extensionConnected': '扩展已连接', 'app.extensionDisconnected': '扩展未连接', @@ -1397,6 +1552,42 @@ const zh: Partial> = { 'sceneSheet.retrying': '重试中…', 'sceneSheet.retryStage': '重试该阶段', + 'settings.title': 'AI 提供方', + 'settings.desc': '为每个自动化角色选择运行的 CLI 代理，以及模型与思考强度。', + 'settings.loading': '正在加载提供方设置...', + 'settings.loadFailed': '无法加载提供方设置。', + 'settings.retry': '重试', + 'settings.activeProvider': '当前', + 'settings.test': '测试二进制', + 'settings.testing': '测试中…', + 'settings.testHint': '对每个提供方执行 --version，需要几秒钟。', + 'settings.providers.title': '提供方二进制', + 'settings.providers.installed': '已安装', + 'settings.providers.missing': '未安装', + 'settings.providers.untested': '未测试', + 'settings.providers.ok': '已响应', + 'settings.providers.failed': '失败', + 'settings.field.agent': '代理', + 'settings.field.model': '模型', + 'settings.field.effort': '强度', + 'settings.effortLockedHint': '{binary} 的模型名称已经包含思考强度。', + 'settings.optionDefault': '默认', + 'settings.optionCustom': '自定义…', + 'settings.notInstalled': '未安装', + 'settings.installHint': '请先安装 {binary} CLI，再把该角色分配给它。', + 'settings.modelsLoading': '正在加载模型…', + 'settings.modelsEmpty': '该提供方没有可用的模型目录。', + 'settings.modelsFreeText': '该提供方没有权威目录 — 可直接填写自定义模型标识。', + 'settings.modelCustomPlaceholder': '模型标识', + 'settings.modelBackToList': '从列表选择', + 'settings.refreshModels': '刷新模型目录', + 'settings.save': '保存', + 'settings.saving': '保存中…', + 'settings.saved': '已保存', + 'settings.unsaved': '未保存', + 'settings.inSync': '已同步', + 'settings.empty': '尚未配置任何角色。', + 'editableText.clickToEdit': '点击编辑', 'editableText.empty': '（空）', } @@ -1437,6 +1628,7 @@ const ko: Partial> = { 'nav.gallery': '갤러리', 'nav.logs': '로그', 'nav.guide': '가이드', + 'nav.settings': '설정', 'app.brandName': 'FLOW KIT', 'app.brandTag': 'ops console', 'app.breadcrumbRoot': 'flow kit', @@ -1445,6 +1637,7 @@ const ko: Partial> = { 'app.breadcrumb.gallery': '갤러리', 'app.breadcrumb.logs': '로그', 'app.breadcrumb.guide': '가이드', + 'app.breadcrumb.settings': '설정', 'app.workers': '워커', 'app.extensionConnected': '익스텐션 연결됨', 'app.extensionDisconnected': '익스텐션 연결 안 됨', @@ -1671,6 +1864,42 @@ const ko: Partial> = { 'sceneSheet.retrying': '재시도 중…', 'sceneSheet.retryStage': '단계 재시도', + 'settings.title': 'AI 제공자', + 'settings.desc': '각 자동화 역할을 실행할 CLI 에이전트와 모델, 노력 수준을 지정합니다.', + 'settings.loading': '제공자 설정을 불러오는 중...', + 'settings.loadFailed': '제공자 설정을 불러오지 못했습니다.', + 'settings.retry': '다시 시도', + 'settings.activeProvider': '활성', + 'settings.test': '바이너리 테스트', + 'settings.testing': '테스트 중…', + 'settings.testHint': '각 제공자에 대해 --version 을 실행합니다. 몇 초 걸립니다.', + 'settings.providers.title': '제공자 바이너리', + 'settings.providers.installed': '설치됨', + 'settings.providers.missing': '설치 안 됨', + 'settings.providers.untested': '테스트 안 함', + 'settings.providers.ok': '응답함', + 'settings.providers.failed': '실패', + 'settings.field.agent': '에이전트', + 'settings.field.model': '모델', + 'settings.field.effort': '노력', + 'settings.effortLockedHint': '{binary} 의 모델 이름에 이미 노력 수준이 포함되어 있습니다.', + 'settings.optionDefault': '기본값', + 'settings.optionCustom': '직접 입력…', + 'settings.notInstalled': '설치 안 됨', + 'settings.installHint': '이 역할을 맡기기 전에 {binary} CLI 를 설치하세요.', + 'settings.modelsLoading': '모델을 불러오는 중…', + 'settings.modelsEmpty': '이 제공자의 카탈로그가 없습니다.', + 'settings.modelsFreeText': '이 제공자는 공식 카탈로그가 없습니다 — 직접 입력한 모델 슬러그도 허용됩니다.', + 'settings.modelCustomPlaceholder': '모델 슬러그', + 'settings.modelBackToList': '목록에서 선택', + 'settings.refreshModels': '모델 카탈로그 새로고침', + 'settings.save': '저장', + 'settings.saving': '저장 중…', + 'settings.saved': '저장됨', + 'settings.unsaved': '저장 안 됨', + 'settings.inSync': '동기화됨', + 'settings.empty': '구성된 역할이 없습니다.', + 'editableText.clickToEdit': '클릭하여 수정', 'editableText.empty': '(비어 있음)', } @@ -1711,6 +1940,7 @@ const ja: Partial> = { 'nav.gallery': 'ギャラリー', 'nav.logs': 'ログ', 'nav.guide': 'ガイド', + 'nav.settings': '設定', 'app.brandName': 'FLOW KIT', 'app.brandTag': 'ops console', 'app.breadcrumbRoot': 'flow kit', @@ -1719,6 +1949,7 @@ const ja: Partial> = { 'app.breadcrumb.gallery': 'ギャラリー', 'app.breadcrumb.logs': 'ログ', 'app.breadcrumb.guide': 'ガイド', + 'app.breadcrumb.settings': '設定', 'app.workers': 'ワーカー', 'app.extensionConnected': '拡張機能 接続済み', 'app.extensionDisconnected': '拡張機能 未接続', @@ -1945,6 +2176,42 @@ const ja: Partial> = { 'sceneSheet.retrying': '再試行中…', 'sceneSheet.retryStage': 'ステージを再試行', + 'settings.title': 'AI プロバイダ', + 'settings.desc': '各自動ロールを実行する CLI エージェントと、モデル・努力レベルを設定します。', + 'settings.loading': 'プロバイダ設定を読み込み中...', + 'settings.loadFailed': 'プロバイダ設定を読み込めませんでした。', + 'settings.retry': '再試行', + 'settings.activeProvider': '有効', + 'settings.test': 'バイナリをテスト', + 'settings.testing': 'テスト中…', + 'settings.testHint': '各プロバイダで --version を実行します。数秒かかります。', + 'settings.providers.title': 'プロバイダのバイナリ', + 'settings.providers.installed': 'インストール済み', + 'settings.providers.missing': '未インストール', + 'settings.providers.untested': '未テスト', + 'settings.providers.ok': '応答あり', + 'settings.providers.failed': '失敗', + 'settings.field.agent': 'エージェント', + 'settings.field.model': 'モデル', + 'settings.field.effort': '努力レベル', + 'settings.effortLockedHint': '{binary} のモデル名には努力レベルが含まれています。', + 'settings.optionDefault': 'デフォルト', + 'settings.optionCustom': 'カスタム…', + 'settings.notInstalled': '未インストール', + 'settings.installHint': 'このロールを割り当てる前に {binary} CLI をインストールしてください。', + 'settings.modelsLoading': 'モデルを読み込み中…', + 'settings.modelsEmpty': 'このプロバイダのカタログはありません。', + 'settings.modelsFreeText': 'このプロバイダには公式カタログがありません — カスタムのモデル名も使えます。', + 'settings.modelCustomPlaceholder': 'モデル名', + 'settings.modelBackToList': '一覧から選ぶ', + 'settings.refreshModels': 'モデルカタログを更新', + 'settings.save': '保存', + 'settings.saving': '保存中…', + 'settings.saved': '保存しました', + 'settings.unsaved': '未保存', + 'settings.inSync': '同期済み', + 'settings.empty': 'ロールが設定されていません。', + 'editableText.clickToEdit': 'クリックして編集', 'editableText.empty': '（空）', } diff --git a/dashboard/src/pages/SettingsPage.tsx b/dashboard/src/pages/SettingsPage.tsx new file mode 100644 index 000000000..8022b70d5 --- /dev/null +++ b/dashboard/src/pages/SettingsPage.tsx @@ -0,0 +1,359 @@ +import { useState, useEffect, useCallback } from 'react' +import { RefreshCw } from 'lucide-react' +import { fetchAPI, patchAPI } from '../api/client' +import { useTranslation } from '../i18n/useTranslation' +import type { RoleAssignment, ProvidersResponse, ProviderModelsResponse, ProvidersUpdateResponse } from '../types' +import { Card, CardHeader, CardTitle, CardDescription, CardContent, CardAction } from '../components/ui/card' +import { Button } from '../components/ui/button' + +const DEFAULT_OPTION = '__default__' +const CUSTOM_OPTION = '__custom__' + +const CONTROL_CLASS = 'text-xs px-2 py-1.5 rounded outline-none w-full' +const CONTROL_STYLE = { background: 'var(--card)', color: 'var(--text)', border: '1px solid var(--border)' } + +interface SaveState { + ok: boolean + text: string +} + +// fetchAPI throws `API : ` — pull the backend's `detail` back out so a 400 reads verbatim. +function apiDetail(err: unknown): string { + const raw = err instanceof Error ? err.message : String(err) + const body = raw.replace(/^API \d+: /, '') + try { + const parsed = JSON.parse(body) as { detail?: unknown } + if (typeof parsed.detail === 'string') return parsed.detail + } catch { + // not JSON — fall through to the raw message + } + return raw +} + +function cloneRoles(roles: Record): Record { + return Object.fromEntries(Object.entries(roles).map(([id, r]) => [id, { ...r }])) +} + +function sameAssignment(a: RoleAssignment, b: RoleAssignment): boolean { + return a.provider === b.provider && a.model === b.model && a.effort === b.effort +} + +export default function SettingsPage() { + const { t } = useTranslation() + const [data, setData] = useState(null) + const [loading, setLoading] = useState(true) + const [loadError, setLoadError] = useState(null) + const [drafts, setDrafts] = useState>({}) + const [models, setModels] = useState>({}) + const [modelsLoading, setModelsLoading] = useState>({}) + const [customModel, setCustomModel] = useState>({}) + const [saving, setSaving] = useState>({}) + const [saveState, setSaveState] = useState>({}) + const [testing, setTesting] = useState(false) + const [testError, setTestError] = useState(null) + + const loadModels = useCallback(async (provider: string, refresh: boolean) => { + setModelsLoading(prev => ({ ...prev, [provider]: true })) + try { + const res = await fetchAPI( + `/api/providers/models?provider=${encodeURIComponent(provider)}&refresh=${refresh}` + ) + setModels(prev => ({ ...prev, [provider]: res })) + } catch { + // an unreachable catalog is not fatal — render it as "no catalog available" + setModels(prev => ({ ...prev, [provider]: { provider, models: [], authoritative: false } })) + } finally { + setModelsLoading(prev => ({ ...prev, [provider]: false })) + } + }, []) + + const load = useCallback(async () => { + setLoading(true) + setLoadError(null) + try { + const res = await fetchAPI('/api/providers?live=false') + setData(res) + setDrafts(cloneRoles(res.roles)) + setCustomModel({}) + setSaveState({}) + Array.from(new Set(Object.values(res.roles).map(r => r.provider))).forEach(p => { loadModels(p, false) }) + } catch (err) { + setData(null) + setLoadError(apiDetail(err)) + } finally { + setLoading(false) + } + }, [loadModels]) + + useEffect(() => { Promise.resolve().then(load) }, [load]) + + function updateDraft(roleId: string, patch: Partial) { + setDrafts(prev => ({ ...prev, [roleId]: { ...prev[roleId], ...patch } })) + setSaveState(prev => { + if (!(roleId in prev)) return prev + const next = { ...prev } + delete next[roleId] + return next + }) + } + + function changeProvider(roleId: string, provider: string) { + // a model slug from one CLI means nothing to another, and effort ladders differ per provider + const efforts = data?.providers[provider]?.efforts ?? [] + const current = drafts[roleId] + const effort = current.effort !== null && efforts.includes(current.effort) ? current.effort : null + updateDraft(roleId, { provider, model: null, effort }) + setCustomModel(prev => ({ ...prev, [roleId]: false })) + if (!models[provider] && !modelsLoading[provider]) loadModels(provider, false) + } + + function setModel(roleId: string, model: string | null) { + // a provider whose slugs name their own effort rejects --model and --effort together + const encodesEffort = data?.providers[drafts[roleId].provider]?.model_encodes_effort ?? false + updateDraft(roleId, model !== null && encodesEffort ? { model, effort: null } : { model }) + } + + function selectModel(roleId: string, value: string) { + if (value === CUSTOM_OPTION) { + setCustomModel(prev => ({ ...prev, [roleId]: true })) + setModel(roleId, null) + return + } + setModel(roleId, value === DEFAULT_OPTION ? null : value) + } + + async function save(roleId: string) { + const draft = drafts[roleId] + const model = draft.model !== null && draft.model.trim() !== '' ? draft.model.trim() : null + const encodesEffort = data?.providers[draft.provider]?.model_encodes_effort ?? false + const effort = model !== null && encodesEffort ? null : draft.effort + setSaving(prev => ({ ...prev, [roleId]: true })) + try { + const res = await patchAPI('/api/providers', { + roles: { [roleId]: { provider: draft.provider, model, effort } }, + }) + setData(prev => (prev ? { ...prev, active: res.active, roles: res.roles } : prev)) + setDrafts(prev => ({ ...prev, [roleId]: { ...(res.roles[roleId] ?? prev[roleId]) } })) + setSaveState(prev => ({ ...prev, [roleId]: { ok: true, text: t('settings.saved') } })) + } catch (err) { + setSaveState(prev => ({ ...prev, [roleId]: { ok: false, text: apiDetail(err) } })) + } finally { + setSaving(prev => ({ ...prev, [roleId]: false })) + } + } + + async function runTest() { + setTesting(true) + setTestError(null) + try { + const res = await fetchAPI('/api/providers?live=true') + setData(prev => (prev ? { ...prev, providers: res.providers } : res)) + } catch (err) { + setTestError(apiDetail(err)) + } finally { + setTesting(false) + } + } + + if (loading) { + return
{t('settings.loading')}
+ } + + if (!data) { + return ( +
+ {loadError ?? t('settings.loadFailed')} + +
+ ) + } + + const providers = data.providers + const roleIds = Object.keys(data.roles) + + return ( +
+
+

{t('settings.title')}

+ {t('settings.desc')} +
+ + + + {t('settings.providers.title')} + {t('settings.testHint')} + + + + + + {testError &&
{testError}
} +
+ {Object.entries(providers).map(([name, info]) => ( +
+ + {info.binary} + + {info.installed ? t('settings.providers.installed') : t('settings.providers.missing')} + + + {info.tested === null ? t('settings.providers.untested') : info.tested ? t('settings.providers.ok') : t('settings.providers.failed')} + + {info.error && {info.error}} + {name === data.active && ( + {t('settings.activeProvider')} + )} +
+ ))} +
+ + + + {roleIds.length === 0 ? ( +
{t('settings.empty')}
+ ) : roleIds.map(roleId => { + const draft = drafts[roleId] + const meta = data.role_meta[roleId] + const info = providers[draft.provider] + const efforts = info?.efforts ?? [] + const catalog = models[draft.provider] + const modelList = catalog?.models ?? [] + const authoritative = catalog?.authoritative ?? false + const modelBusy = modelsLoading[draft.provider] ?? false + const custom = customModel[roleId] ?? false + const knownModel = draft.model !== null && modelList.some(m => m.id === draft.model) + const effortLocked = (info?.model_encodes_effort ?? false) && draft.model !== null + const dirty = !sameAssignment(draft, data.roles[roleId]) + const busy = saving[roleId] ?? false + const state = saveState[roleId] + + return ( + + + {meta?.label ?? roleId} + {meta?.description ?? roleId} + + + {dirty ? t('settings.unsaved') : t('settings.inSync')} + + + + +
+
+ {t('settings.field.agent')} + + {info && !info.installed && ( + + {t('settings.installHint', { binary: info.binary })} + + )} +
+ +
+ {t('settings.field.model')} +
+ {custom ? ( + setModel(roleId, e.target.value === '' ? null : e.target.value)} + placeholder={t('settings.modelCustomPlaceholder')} + className={CONTROL_CLASS} + style={CONTROL_STYLE} + /> + ) : ( + + )} + +
+ + {modelBusy + ? t('settings.modelsLoading') + : modelList.length === 0 + ? t('settings.modelsEmpty') + : !authoritative + ? t('settings.modelsFreeText') + : ''} + + {custom && ( + + )} +
+ +
+ {t('settings.field.effort')} + + {effortLocked && ( + + {t('settings.effortLockedHint', { binary: info.binary })} + + )} +
+
+ +
+ + {state && ( + {state.text} + )} +
+ + + ) + })} +
+ ) +} diff --git a/dashboard/src/types/index.ts b/dashboard/src/types/index.ts index 2c8cccbd1..5eebdd6d5 100644 --- a/dashboard/src/types/index.ts +++ b/dashboard/src/types/index.ts @@ -147,3 +147,50 @@ export interface SceneReview { fps_used: number has_critical_errors: boolean } + +// AI provider settings — match the /api/providers payload exactly +export interface RoleAssignment { + provider: string + model: string | null + effort: string | null +} + +export interface RoleMeta { + label: string + description: string +} + +export interface ProviderInfo { + binary: string + installed: boolean + tested: boolean | null + error: string | null + efforts: string[] + default_model: string | null + catalog_is_authoritative: boolean + model_encodes_effort: boolean +} + +export interface ProvidersResponse { + active: string + roles: Record + role_meta: Record + providers: Record +} + +export interface ProviderModel { + id: string + label: string +} + +export interface ProviderModelsResponse { + provider: string + models: ProviderModel[] + authoritative: boolean +} + +export interface ProvidersUpdateResponse { + status: string + active: string + roles: Record +} diff --git a/skills/fk-change-provider.md b/skills/fk-change-provider.md index eff478185..42b46cdd7 100644 --- a/skills/fk-change-provider.md +++ b/skills/fk-change-provider.md @@ -1,11 +1,18 @@ -# fk-change-provider — View & Switch AI CLI Provider (Video Review) +# fk-change-provider — View & Switch the AI CLI for a Role -View or switch which AI CLI backend (`claude`, `agy`, or `codex`) is used for vision analysis during video review. +View or change which AI CLI (`claude`, `agy`, `codex`) — and which model and +reasoning effort — runs each AI role. One role exists today: `video_review`, +the vision analysis behind `/fk-review-video`. Usage: -- `/fk-change-provider` — show current provider status -- `/fk-change-provider list` — show current provider status -- `/fk-change-provider set ` — switch active provider +- `/fk-change-provider` — show current status +- `/fk-change-provider list` — show current status +- `/fk-change-provider set ` — switch the provider +- `/fk-change-provider set --model ` — switch provider and model +- `/fk-change-provider set --effort ` — switch provider and effort + (for `agy`, model and effort are mutually exclusive — see Step 2) + +The dashboard has the same controls under **Settings**. --- @@ -15,54 +22,112 @@ Usage: curl -s "http://127.0.0.1:8100/api/providers?live=true" | python3 -m json.tool ``` -Display in a readable table: +The response carries `active` (the legacy whole-agent provider), `roles` (what +each role actually runs on), `role_meta` (labels), and `providers` (per-CLI +status and the effort ladder that CLI accepts). + +Display two tables — first the roles: + +| Role | Provider | Model | Effort | +|------|----------|-------|--------| +| Video Review | agy | gemini-3.8-flash-low | low | + +`null` model or effort means "whatever that CLI defaults to" — render it as +`default`, not as an empty cell. + +Then the providers: -| Provider | Binary | Installed | Tested | Active | -|----------|--------|-----------|--------|--------| -| claude | `claude` | Yes | Yes | ✅ | -| agy | `agy` | Yes | Yes | | -| codex | `codex` | Yes | No | | +| Provider | Binary | Installed | Tested | Efforts | +|----------|--------|-----------|--------|---------| +| claude | `claude` | Yes | Yes | low, medium, high, xhigh, max | +| agy | `agy` | Yes | Yes | low, medium, high | +| codex | `codex` | Yes | No | low, medium, high, xhigh, max | - `Installed` reflects whether the binary is found on PATH. -- `Tested` reflects the live ` --version` probe (populated because of `?live=true`); `null`/missing means not yet probed. -- `Active` gets a checkmark on whichever provider matches the top-level `active` field in the response. +- `Tested` reflects the live ` --version` probe (populated because of + `?live=true`); `null` means not yet probed. ## Step 2: Quick Select (Interactive) -If no provider was given as an argument, use `AskUserQuestion` to let the user pick — only offer providers where `installed: true` as selectable options. +If nothing was given as an argument, use `AskUserQuestion` to let the user pick +— only offer providers where `installed: true`. For any provider with +`installed: false`, list it as unavailable with "install `` first" +instead of offering it. + +To offer models, list them for the chosen provider: + +```bash +curl -s "http://127.0.0.1:8100/api/providers/models?provider=" | python3 -m json.tool +``` + +- An **empty** `models` array is a normal answer, not an error — the binary is + missing or the listing call failed. Fall back to the provider's default. +- `authoritative: true` (agy only) means that list is the whole truth and agy + rejects anything outside it. For `claude` and `codex` an unlisted slug is a + legitimate escape hatch — claude takes aliases like `sonnet` and full model + names, codex takes slugs newer than its on-disk cache. +- Offer only efforts from that provider's `efforts` array. **agy has no `xhigh` + or `max`** and the API rejects them with a 400. +- **If the provider has `model_encodes_effort: true` (agy), do not offer both.** + agy's slugs name their own effort, so `--model` and `--effort` together are + rejected — a mismatch conflicts, and a slug with no effort in its name + (`claude-sonnet-4-6`) refuses `--effort` at all. Ask for a model **or** an + effort, not both; the API answers 400 for the pair. -For any provider with `installed: false`, list it as unavailable with a note like "install `` first" instead of offering it as a selectable option. +## Step 3: Change It -## Step 3: Change the Provider +Per role — this is the one to use: ```bash curl -X PATCH http://127.0.0.1:8100/api/providers \ -H "Content-Type: application/json" \ - -d '{"active": ""}' + -d '{"roles": {"video_review": {"provider": "claude", "model": "sonnet", "effort": "high"}}}' ``` -- Returns `{"status": "updated", "active": ""}` on success. -- Returns `400` if the provider name is unknown, or if its binary isn't found on PATH. +- `model` and `effort` are optional and nullable; omit or send `null` for the + CLI's own default. +- For `agy`, send a model **or** an effort, never both — `{"provider": "agy", + "model": "gemini-3.8-flash-low"}` is right, adding `"effort": "low"` is a 400. +- Returns `{"status": "updated", "active": ..., "roles": {...}}`. +- `400` on an unknown role or provider, a binary missing from PATH, or an + effort the provider does not have. The `detail` string says which. -## Step 4: Verify - -After changing, verify the update took effect: +Whole-agent switch (legacy, still supported): ```bash -curl -s "http://127.0.0.1:8100/api/providers?live=true" | python3 -m json.tool +curl -X PATCH http://127.0.0.1:8100/api/providers \ + -H "Content-Type: application/json" -d '{"active": "agy"}' ``` -Confirm the `active` field now matches the provider you selected. +This moves every role onto that provider, **clears each role's model** and +**drops an effort the new provider lacks** — a model slug means nothing to a +different CLI, and agy would reject `sonnet` outright. + +## Step 4: Verify + +```bash +curl -s "http://127.0.0.1:8100/api/providers" | python3 -m json.tool +``` -Changes are **hot-reloaded** — no server restart needed. The new provider is used immediately for all subsequent video review requests. +Confirm `roles.video_review` matches what you set. Changes are **hot-reloaded** +— no server restart, and the next review uses them. --- ## Notes -- `claude` = Claude Code CLI (default) -- `agy` = Google Antigravity CLI -- `codex` = OpenAI Codex CLI -- Switching is hot-reloaded — no server restart needed. -- **`codex` requires a separate one-time `codex login` (interactive OAuth in a terminal) before it will actually work.** Being listed as `installed: true` only means the binary is present on PATH — it does not mean it's authenticated. If a codex-backed review fails with an auth error, run `codex login` and retry. -- If you set a provider whose binary later goes missing, the next review request will fail clearly (not silently) — switch back via this skill. +- `claude` = Claude Code CLI (default) · `agy` = Google Antigravity CLI · + `codex` = OpenAI Codex CLI +- **`codex` needs two things beyond the binary**: a one-time `codex login` + (interactive OAuth in a terminal), and credits on its OpenAI workspace. With + no balance every review fails with `ERROR: Your workspace is out of credits`. + `installed: true` only means the binary is on PATH. +- `agy` is never run with `--dangerously-skip-permissions`. It reads contact + sheets with its own file-reading tool because the prompt tells it to, which + needs no elevated permission. If a review ever comes back saying tools were + "auto-denied headlessly", that steering was lost — do not add the flag, fix + the prompt. +- `codex` runs `--sandbox read-only`. `-i` hands it the image bytes directly, + so it needs no shell and no writable filesystem. +- `agent/providers.json` is the file behind all of this and is safe to edit by + hand; the next API call re-reads it. diff --git a/skills/fk-doctor.md b/skills/fk-doctor.md index cd505235f..f26c8db38 100644 --- a/skills/fk-doctor.md +++ b/skills/fk-doctor.md @@ -11,6 +11,7 @@ Diagnose any FlowKit error and prescribe a fix. Knows the full error taxonomy ac - An HTTP 4xx/5xx reaches the main agent from any endpoint under `127.0.0.1:8100` - A YouTube upload returns `HttpError` from `googleapiclient` - `cryptography` / architecture / import errors surface during setup +- A `/fk-review-video` run fails or returns nothing, or an error mentions `No such filter`, `Frame extraction failed`, `auto-denied`, `out of credits`, `CLI failed`, `CLI timed out`, `invalid model selection` **DO NOT use when:** - The request is still `PENDING` and hasn't been attempted yet @@ -202,6 +203,25 @@ When the user describes a symptom in plain language, map it here first. | Python `cryptography` arch mismatch | Use `python3.10`, not `python3.13` (x86/arm64 binary mismatch) | | `curl: (7) Failed to connect to 127.0.0.1:8100` | Agent not running — `python -m agent.main` | +### G. Video review errors (`services/video_reviewer.py`) + +Review runs outside the worker — no retry policy applies, the call just raises. +Providers, models and efforts come from `agent/providers.json`; see +`/fk-change-provider`. + +| Error | Cause | Fix | +|---|---|---| +| `Frame extraction failed: ... No such filter: 'drawtext'` | ffmpeg built without libfreetype. Should no longer happen — the filter is probed and skipped — so seeing it means the probe was bypassed | `ffmpeg -filters \| grep drawtext`. Absent is fine; sheets just lose their burned-in timestamps | +| `agy CLI had tools auto-denied headlessly (RunCommand) — its N-character answer cannot be trusted to have come from the images` | agy tried to shell out to read the contact sheet instead of using its file-reading tool; headless mode cannot prompt, so the tool was denied. Raised whether or not agy still produced an answer — the prompt carries the rubric, both scene prompts and the character names, which is enough to write a plausible review without ever looking at a frame | Restore the steering in `_AGY_READ_STEER`. **Do not** add `--dangerously-skip-permissions` — it auto-approves every tool including arbitrary shell commands, for a job that only reads JPEGs | +| `agy CLI returned non-JSON output: ...` | agy answered with bare prose on a zero exit code — usually a permission or startup error | Read the quoted text; it names the real problem | +| `agy CLI failed (rc=1): invalid model selection ... conflicts with --effort=` | agy's slugs name their own effort (`gemini-3.8-flash-low`), so model and effort cannot both be set | Clear one of them. The API rejects the pair with a 400; this only reaches the CLI from a hand-edited `providers.json` | +| `codex CLI failed (rc=1): ... Your workspace is out of credits` | The OpenAI workspace has no balance. `installed: true` only means the binary is on PATH | Refill, or switch the role to `claude`/`agy` | +| `codex CLI exited cleanly but wrote no answer to its output file` | codex returned success but produced nothing | Re-run; if it repeats, switch provider | +| ` CLI timed out after Ns` | The review exceeded `REVIEW_CLI_TIMEOUT_S` (default 120) | Raise the env var, or lower `REVIEW_FPS_*` / `REVIEW_MAX_FRAMES` so there is less to look at | +| `review answer had an error entry with no usable severity (expected one of ['CRITICAL', 'HIGH', 'MINOR']): ...` | The model graded an error with something outside the three severities. `has_critical_errors`, the `character_consistency` cap and the fix guide all branch on that exact string, so anything else silently disables all three — the model flagged a defect and the score would not show it | Read the quoted entry. A model that keeps doing this is not following the rubric; switch the role to another one | +| `Scene : review answer needed repair — N error field(s) defaulted, M usable segment(s) unreadable` (warning, not a failure) | A near-miss field name (`timeRange` for `time_range`) or a missing description. Repaired, because losing those costs context but cannot move a score | Nothing required. A rising count means the model is drifting from the rubric | +| `CLI answer had no dimensions: ...` | The CLI returned parseable JSON with no scores in it. Every dimension defaults to 5.0, so this would otherwise have become a complete, plausible "poor" review of a video nobody actually looked at | Read the quoted answer. Usually the model wrote prose around the JSON, or ran out of context — lower `REVIEW_MAX_FRAMES` or try another model | + ## Worker retry policy (`processor.py:_handle_failure`) Decision order — stop at first match: diff --git a/tests/unit/test_cli_providers.py b/tests/unit/test_cli_providers.py index 68ff56ca3..2178eb448 100644 --- a/tests/unit/test_cli_providers.py +++ b/tests/unit/test_cli_providers.py @@ -19,7 +19,10 @@ _run_codex_cli, _analyze_cli, _build_prompt, + _frame_filter, + _parse_agy_envelope, ) +from agent.services import cli_providers from agent.api import providers as providers_api @@ -59,10 +62,24 @@ async def test_argv_and_return_value(self): # _run_agy_cli # --------------------------------------------------------------------------- +def agy_envelope(response: str, **extra) -> bytes: + """A realistic `agy --output-format json` envelope.""" + env = { + "conversation_id": "c-1", + "status": "SUCCESS", + "response": response, + "duration_seconds": 1.0, + "num_turns": 1, + "usage": {"total_tokens": 10}, + } + env.update(extra) + return json.dumps(env).encode() + + class TestRunAgyCli: @pytest.mark.asyncio - async def test_argv_and_return_value(self): - proc = make_proc(stdout=b'{"ok": true}', stderr=b"", returncode=0) + async def test_argv_asks_for_json_and_unwraps_the_envelope(self): + proc = make_proc(stdout=agy_envelope('{"ok": true}'), returncode=0) with patch( "agent.services.video_reviewer.asyncio.create_subprocess_exec", new=AsyncMock(return_value=proc), @@ -70,44 +87,149 @@ async def test_argv_and_return_value(self): result = await _run_agy_cli("hello") args, kwargs = mock_exec.call_args - assert args == ("agy", "-p", "hello", "--dangerously-skip-permissions", "--output-format", "text") + assert args[:6] == ("agy", "-p", "hello", "--output-format", "json", "--print-timeout") + # The answer is the envelope's `response`, not the raw stdout. assert result == '{"ok": true}' + @pytest.mark.asyncio + async def test_never_passes_the_skip_permissions_flag(self): + """--dangerously-skip-permissions auto-approves *every* tool, shell + commands included, for a job whose only need is reading a few JPEGs. + Steering agy at its file-reading tool achieves the read unprivileged + (verified against agy 1.2.7), so the flag must never come back.""" + proc = make_proc(stdout=agy_envelope("x"), returncode=0) + with patch( + "agent.services.video_reviewer.asyncio.create_subprocess_exec", + new=AsyncMock(return_value=proc), + ) as mock_exec: + await _run_agy_cli("hello") + assert "--dangerously-skip-permissions" not in mock_exec.call_args[0] + + @staticmethod + async def _argv_for(**kwargs): + proc = make_proc(stdout=agy_envelope("x"), returncode=0) + with patch( + "agent.services.video_reviewer.asyncio.create_subprocess_exec", + new=AsyncMock(return_value=proc), + ) as mock_exec: + await _run_agy_cli("hello", **kwargs) + return mock_exec.call_args[0] + + @pytest.mark.asyncio + async def test_model_and_add_dirs_reach_the_argv(self): + argv = await self._argv_for(model="gemini-3.8-flash-low", add_dirs=("/tmp/sheets",)) + assert argv[argv.index("--model") + 1] == "gemini-3.8-flash-low" + assert argv[argv.index("--add-dir") + 1] == "/tmp/sheets" + + @pytest.mark.asyncio + async def test_effort_alone_reaches_the_argv(self): + argv = await self._argv_for(effort="high") + assert argv[argv.index("--effort") + 1] == "high" + assert "--model" not in argv + + @pytest.mark.asyncio + async def test_effort_is_dropped_when_a_model_is_set(self): + """agy slugs carry the effort — gemini-3.8-flash-low. Verified against + agy 1.2.7: a mismatched pair is rejected outright ("--model + gpt-oss-120b-medium conflicts with --effort=low") and a slug with no + effort in its name refuses --effort at all. The API will not store both, + but providers.json is hand-editable, so the runner defends too.""" + argv = await self._argv_for(model="gemini-3.8-flash-low", effort="high") + assert argv[argv.index("--model") + 1] == "gemini-3.8-flash-low" + assert "--effort" not in argv + + +class TestParseAgyEnvelope: + def test_returns_the_response_field(self): + assert _parse_agy_envelope(agy_envelope(" hi ").decode()) == "hi" + + def test_denied_tool_with_empty_response_is_an_error_not_an_empty_answer(self): + """The exact shape of the bug this rewrite fixes: agy auto-denies a + tool it cannot prompt for, returns an empty response, and still reports + status SUCCESS on a zero exit code. Read naively that is a silent + empty review; it has to be an error, and it has to name the tool.""" + raw = agy_envelope( + "", denied_actions=[{"action": "command", "display_name": "RunCommand"}] + ).decode() + with pytest.raises(RuntimeError) as excinfo: + _parse_agy_envelope(raw) + assert "RunCommand" in str(excinfo.value) + + def test_a_denied_tool_with_a_confident_answer_is_also_an_error(self): + """The more dangerous half. The prompt carries the full scoring rubric, + the scene's image prompt, its video prompt and the character names — so + agy can write a complete, plausible review from the text alone without + ever having looked at a frame. A denial means the "use your file-reading + tool, run no shell command" steering did not hold, so the answer cannot + be attributed to the images no matter how well-formed it is.""" + raw = agy_envelope( + '{"dimensions": {"character_consistency": 9.0}, "errors": []}', + denied_actions=[{"action": "command", "display_name": "RunCommand"}], + ).decode() + with pytest.raises(RuntimeError, match="auto-denied"): + _parse_agy_envelope(raw) + + def test_a_clean_success_carries_no_denials_and_passes(self): + """The happy path must stay clean — live agy 1.2.7 runs that succeed + omit denied_actions entirely, so widening the guard costs nothing.""" + assert _parse_agy_envelope(agy_envelope("the answer").decode()) == "the answer" + + def test_bare_prose_on_a_zero_exit_code_is_an_error(self): + """What agy actually prints when the permission is denied and no + --output-format json is honoured. rc is 0, so _spawn_and_check lets it + through and this is the only place left to catch it.""" + with pytest.raises(RuntimeError, match="non-JSON"): + _parse_agy_envelope( + "jetski: no output produced — a tool required the \"command\" permission" + ) + + def test_empty_stdout_is_an_error(self): + with pytest.raises(RuntimeError, match="no output"): + _parse_agy_envelope(" ") + + def test_empty_response_is_an_error(self): + with pytest.raises(RuntimeError, match="empty response"): + _parse_agy_envelope(agy_envelope("").decode()) + + def test_non_success_status_is_an_error(self): + with pytest.raises(RuntimeError, match="status"): + _parse_agy_envelope(agy_envelope("hi", status="ERROR").decode()) + # --------------------------------------------------------------------------- # _run_codex_cli # --------------------------------------------------------------------------- +def fake_codex_exec(captured, answer: str = "codex analysis result", returncode: int = 0): + """Stand in for the codex binary: record argv, write the -o file, exit.""" + async def _run(*args, **kwargs): + captured["args"] = args + captured["kwargs"] = kwargs + out_path = Path(args[args.index("-o") + 1]) + out_path.write_text(answer) + return make_proc(stdout=b"", stderr=b"", returncode=returncode) + return _run + + class TestRunCodexCli: @pytest.mark.asyncio async def test_argv_structure_and_output_file_roundtrip(self): contact_sheets = [Path("/tmp/sheet_00.jpg"), Path("/tmp/sheet_01.jpg")] captured = {} - async def fake_create_subprocess_exec(*args, **kwargs): - captured["args"] = args - # Locate the -o path and write our known content to it, simulating - # what the real codex CLI would do before exiting. - o_index = args.index("-o") - out_path = Path(args[o_index + 1]) - out_path.write_text("codex analysis result") - return make_proc(stdout=b"", stderr=b"", returncode=0) - with patch( "agent.services.video_reviewer.asyncio.create_subprocess_exec", - new=AsyncMock(side_effect=fake_create_subprocess_exec), + new=AsyncMock(side_effect=fake_codex_exec(captured)), ): result = await _run_codex_cli("analyze this", contact_sheets) args = captured["args"] - o_index = args.index("-o") - out_path = Path(args[o_index + 1]) + out_path = Path(args[args.index("-o") + 1]) assert args == ( - "codex", "exec", + "codex", "exec", "--skip-git-repo-check", "--sandbox", "read-only", "-i", str(contact_sheets[0]), "-i", str(contact_sheets[1]), "-o", str(out_path), - "--dangerously-bypass-approvals-and-sandbox", "analyze this", ) @@ -115,6 +237,78 @@ async def fake_create_subprocess_exec(*args, **kwargs): # finally: out_path.unlink(missing_ok=True) must have run assert not out_path.exists() + @pytest.mark.asyncio + async def test_never_bypasses_the_sandbox(self): + """-i hands codex the image bytes directly, so the run needs neither a + shell nor a writable filesystem. read-only already implies + approval:never, so nothing hangs — the bypass flag bought nothing and + cost the sandbox.""" + captured = {} + with patch( + "agent.services.video_reviewer.asyncio.create_subprocess_exec", + new=AsyncMock(side_effect=fake_codex_exec(captured)), + ): + await _run_codex_cli("analyze this", [Path("/tmp/sheet_00.jpg")]) + assert "--dangerously-bypass-approvals-and-sandbox" not in captured["args"] + assert "--sandbox" in captured["args"] + + @pytest.mark.asyncio + async def test_model_and_effort_reach_the_argv(self): + captured = {} + with patch( + "agent.services.video_reviewer.asyncio.create_subprocess_exec", + new=AsyncMock(side_effect=fake_codex_exec(captured)), + ): + await _run_codex_cli( + "analyze this", [Path("/tmp/s.jpg")], model="gpt-5.6-sol", effort="high" + ) + argv = captured["args"] + assert argv[argv.index("-m") + 1] == "gpt-5.6-sol" + # codex has no --effort; the reasoning level is a TOML config override. + assert argv[argv.index("-c") + 1] == 'model_reasoning_effort="high"' + + @pytest.mark.asyncio + async def test_empty_output_file_is_an_error_not_an_empty_answer(self): + """codex can exit 0 having written nothing. Returning "" from here + surfaces as a JSON decode error three frames away from the cause.""" + captured = {} + with patch( + "agent.services.video_reviewer.asyncio.create_subprocess_exec", + new=AsyncMock(side_effect=fake_codex_exec(captured, answer="")), + ): + with pytest.raises(RuntimeError, match="no answer"): + await _run_codex_cli("analyze this", [Path("/tmp/s.jpg")]) + + +class TestStdinIsClosed: + """All three CLIs append piped stdin to the prompt when stdin is not a + terminal — codex documents it as a `` block. Under uvicorn stdin is + inherited from the launching shell, so it has to be closed explicitly.""" + + @pytest.mark.asyncio + @pytest.mark.parametrize("runner,stdout", [ + (_run_claude_cli, b"ok"), + (_run_agy_cli, None), + ]) + async def test_stdin_is_devnull(self, runner, stdout): + proc = make_proc(stdout=stdout if stdout is not None else agy_envelope("ok"), returncode=0) + with patch( + "agent.services.video_reviewer.asyncio.create_subprocess_exec", + new=AsyncMock(return_value=proc), + ) as mock_exec: + await runner("hello") + assert mock_exec.call_args.kwargs["stdin"] == asyncio.subprocess.DEVNULL + + @pytest.mark.asyncio + async def test_codex_stdin_is_devnull(self): + captured = {} + with patch( + "agent.services.video_reviewer.asyncio.create_subprocess_exec", + new=AsyncMock(side_effect=fake_codex_exec(captured)), + ): + await _run_codex_cli("analyze this", [Path("/tmp/s.jpg")]) + assert captured["kwargs"]["stdin"] == asyncio.subprocess.DEVNULL + # --------------------------------------------------------------------------- # Timeout path (shared by all three runners via _communicate_with_timeout) @@ -150,6 +344,18 @@ async def fake_wait_for(coro, timeout): # _analyze_cli prompt branching per provider # --------------------------------------------------------------------------- +EMPTY_REVIEW = '{"dimensions": {}, "errors": [], "usable_segments": []}' + + +def capture_runner(captured): + """Stand in for a claude/agy runner, recording prompt and per-role kwargs.""" + async def _run(full_prompt, **kwargs): + captured["prompt"] = full_prompt + captured.update(kwargs) + return EMPTY_REVIEW + return _run + + class TestAnalyzeCliPromptBranching: @pytest.mark.asyncio @pytest.mark.parametrize( @@ -157,36 +363,56 @@ class TestAnalyzeCliPromptBranching: ) async def test_claude_agy_providers_include_read_the_images_at(self, provider, runner_name): captured = {} - - async def fake_run(full_prompt): - captured["prompt"] = full_prompt - return '{"dimensions": {}, "errors": [], "usable_segments": []}' - contact_sheets = [Path("/tmp/sheet_00.jpg"), Path("/tmp/sheet_01.jpg")] with patch("agent.services.video_reviewer.config.CLI_PROVIDERS", {"active": provider}), \ patch("agent.services.video_reviewer._build_prompt", return_value="BASE_PROMPT"), \ - patch(f"agent.services.video_reviewer.{runner_name}", new=AsyncMock(side_effect=fake_run)): - result = await _analyze_cli(contact_sheets, 10, 4.0, {}) + patch(f"agent.services.video_reviewer.{runner_name}", + new=AsyncMock(side_effect=capture_runner(captured))): + result = await _analyze_cli(contact_sheets, 10, 4.0, {}, timestamped=True) assert "Read the images at:" in captured["prompt"] assert str(contact_sheets[0]) in captured["prompt"] assert str(contact_sheets[1]) in captured["prompt"] assert "BASE_PROMPT" in captured["prompt"] + # The sheets live outside the server cwd, so the directory holding them + # is named to the CLI rather than left to be discovered. + assert captured["add_dirs"] == ("/tmp",) assert result == {"dimensions": {}, "errors": [], "usable_segments": []} + @pytest.mark.asyncio + async def test_only_agy_gets_the_file_read_steering(self): + """The steering line is what makes agy read the sheet with its + file-reading tool instead of shelling out and being auto-denied. claude + needs no such nudge, and an extra instruction is an extra thing for it + to obey oddly.""" + prompts = {} + for provider, runner_name in [("claude", "_run_claude_cli"), ("agy", "_run_agy_cli")]: + captured = {} + with patch("agent.services.video_reviewer.config.CLI_PROVIDERS", {"active": provider}), \ + patch("agent.services.video_reviewer._build_prompt", return_value="BASE_PROMPT"), \ + patch(f"agent.services.video_reviewer.{runner_name}", + new=AsyncMock(side_effect=capture_runner(captured))): + await _analyze_cli([Path("/tmp/s.jpg")], 9, 4.0, {}, timestamped=True) + prompts[provider] = captured["prompt"] + + assert "Do NOT run any shell command" in prompts["agy"] + assert "file-reading tool" in prompts["agy"] + assert "Do NOT run any shell command" not in prompts["claude"] + @pytest.mark.asyncio async def test_codex_provider_excludes_read_the_image_at(self): captured = {} - async def fake_run_codex(full_prompt, contact_sheets): + async def fake_run_codex(full_prompt, contact_sheets, **kwargs): captured["prompt"] = full_prompt - return '{"dimensions": {}, "errors": [], "usable_segments": []}' + captured.update(kwargs) + return EMPTY_REVIEW contact_sheets = [Path("/tmp/sheet_00.jpg"), Path("/tmp/sheet_01.jpg")] with patch("agent.services.video_reviewer.config.CLI_PROVIDERS", {"active": "codex"}), \ patch("agent.services.video_reviewer._build_prompt", return_value="BASE_PROMPT"), \ patch("agent.services.video_reviewer._run_codex_cli", new=AsyncMock(side_effect=fake_run_codex)): - result = await _analyze_cli(contact_sheets, 10, 4.0, {}) + result = await _analyze_cli(contact_sheets, 10, 4.0, {}, timestamped=True) assert "Read the image at" not in captured["prompt"] assert "sequential contact sheets" in captured["prompt"] @@ -201,22 +427,50 @@ async def test_single_sheet_wrapper_wording_matches_original_singular_form(self) contact sheets...' which is both grammatically wrong and misleading. """ captured = {} - - async def fake_run_claude(full_prompt): - captured["prompt"] = full_prompt - return '{"dimensions": {}, "errors": [], "usable_segments": []}' - contact_sheets = [Path("/tmp/sheet_00.jpg")] with patch("agent.services.video_reviewer.config.CLI_PROVIDERS", {"active": "claude"}), \ patch("agent.services.video_reviewer._build_prompt", return_value="BASE_PROMPT"), \ - patch("agent.services.video_reviewer._run_claude_cli", new=AsyncMock(side_effect=fake_run_claude)): - await _analyze_cli(contact_sheets, 9, 4.0, {}) + patch("agent.services.video_reviewer._run_claude_cli", + new=AsyncMock(side_effect=capture_runner(captured))): + await _analyze_cli(contact_sheets, 9, 4.0, {}, timestamped=True) assert captured["prompt"].startswith("Read the image at /tmp/sheet_00.jpg.") assert "Read the images at:" not in captured["prompt"] assert "sequential contact sheets" not in captured["prompt"] assert "It is a contact sheet of 9 video frames at 4.0fps with timestamps." in captured["prompt"] + @pytest.mark.asyncio + async def test_untimestamped_sheets_hand_the_model_the_arithmetic(self): + """Every review answer is expressed in time ranges. When the ffmpeg + build cannot burn timestamps in, the prompt must stop claiming they are + there and give the frame interval instead, or the model invents times + off labels that do not exist.""" + captured = {} + with patch("agent.services.video_reviewer.config.CLI_PROVIDERS", {"active": "claude"}), \ + patch("agent.services.video_reviewer._build_prompt", return_value="BASE_PROMPT"), \ + patch("agent.services.video_reviewer._run_claude_cli", + new=AsyncMock(side_effect=capture_runner(captured))): + await _analyze_cli([Path("/tmp/s.jpg")], 9, 4.0, {}, timestamped=False) + + assert "with timestamps" not in captured["prompt"] + assert "without timestamps" in captured["prompt"] + assert "0.25s" in captured["prompt"] # 1/4fps + + @pytest.mark.asyncio + async def test_role_model_and_effort_reach_the_runner(self): + captured = {} + cfg = {"active": "agy", "roles": {"video_review": { + "provider": "claude", "model": "sonnet", "effort": "high"}}} + with patch("agent.services.video_reviewer.config.CLI_PROVIDERS", cfg), \ + patch("agent.services.video_reviewer._build_prompt", return_value="BASE_PROMPT"), \ + patch("agent.services.video_reviewer._run_claude_cli", + new=AsyncMock(side_effect=capture_runner(captured))): + await _analyze_cli([Path("/tmp/s.jpg")], 9, 4.0, {}, timestamped=True) + + # The role entry wins over `active`, and both knobs reach the runner. + assert captured["model"] == "sonnet" + assert captured["effort"] == "high" + # --------------------------------------------------------------------------- # _build_prompt :: sheet_note behavior (single-sheet vs multi-sheet) @@ -238,6 +492,22 @@ def test_multi_sheet_includes_sheet_note(self): # agent/api/providers.py :: patch_providers # --------------------------------------------------------------------------- +@pytest.fixture +def providers_file(monkeypatch, tmp_path): + """Point the API at a throwaway providers.json and isolate config.""" + tmp_file = tmp_path / "providers.json" + tmp_file.write_text(json.dumps({"active": "claude"})) + monkeypatch.setattr(providers_api, "_PROVIDERS_FILE", tmp_file) + monkeypatch.setattr(providers_api.shutil, "which", lambda binary: "/usr/local/bin/fake") + monkeypatch.setattr(cli_providers.shutil, "which", lambda binary: "/usr/local/bin/fake") + # Ensure config.CLI_PROVIDERS mutation doesn't leak to other tests. + monkeypatch.setattr( + providers_api.config, "CLI_PROVIDERS", + dict(providers_api.config.CLI_PROVIDERS), raising=False, + ) + return tmp_file + + class TestPatchProviders: @pytest.mark.asyncio async def test_unknown_provider_raises_400(self): @@ -253,21 +523,323 @@ async def test_known_provider_missing_binary_raises_400(self, monkeypatch): assert excinfo.value.status_code == 400 @pytest.mark.asyncio - async def test_known_provider_with_binary_updates_file_and_config(self, monkeypatch, tmp_path): - tmp_file = tmp_path / "providers.json" - tmp_file.write_text(json.dumps({"active": "claude"})) + async def test_empty_body_raises_400(self): + with pytest.raises(HTTPException) as excinfo: + await providers_api.patch_providers({}) + assert excinfo.value.status_code == 400 + + @pytest.mark.asyncio + async def test_known_provider_with_binary_updates_file_and_config(self, providers_file): + result = await providers_api.patch_providers({"active": "codex"}) + + assert result["status"] == "updated" + assert result["active"] == "codex" + assert json.loads(providers_file.read_text())["active"] == "codex" + assert providers_api.config.CLI_PROVIDERS["active"] == "codex" + + @pytest.mark.asyncio + async def test_legacy_active_switch_migrates_a_file_with_no_roles(self, providers_file): + """providers.json predates roles. Switching `active` on an old file has + to leave a roles map behind, or the dashboard shows nothing to edit.""" + result = await providers_api.patch_providers({"active": "agy"}) + assert result["roles"]["video_review"]["provider"] == "agy" + assert json.loads(providers_file.read_text())["roles"]["video_review"]["provider"] == "agy" + + @pytest.mark.asyncio + async def test_legacy_active_switch_clears_model_and_clamps_effort(self, providers_file): + """A model slug is meaningless to a different CLI — agy rejects + "sonnet" outright — and agy's effort ladder stops at high. Carrying + either across a provider switch turns the next review into a hard CLI + error several seconds in.""" + providers_file.write_text(json.dumps({"active": "claude", "roles": { + "video_review": {"provider": "claude", "model": "sonnet", "effort": "max"}}})) + + result = await providers_api.patch_providers({"active": "agy"}) + + entry = result["roles"]["video_review"] + assert entry == {"provider": "agy", "model": None, "effort": None} + + @pytest.mark.asyncio + async def test_legacy_active_switch_keeps_an_effort_the_new_provider_supports(self, providers_file): + providers_file.write_text(json.dumps({"active": "claude", "roles": { + "video_review": {"provider": "claude", "model": "sonnet", "effort": "high"}}})) + result = await providers_api.patch_providers({"active": "agy"}) + assert result["roles"]["video_review"]["effort"] == "high" + + @pytest.mark.asyncio + async def test_roles_patch_round_trips(self, providers_file): + result = await providers_api.patch_providers({"roles": {"video_review": { + "provider": "claude", "model": "sonnet", "effort": "high"}}}) + + assert result["roles"]["video_review"] == { + "provider": "claude", "model": "sonnet", "effort": "high"} + assert json.loads(providers_file.read_text())["roles"]["video_review"]["model"] == "sonnet" + + @pytest.mark.asyncio + async def test_roles_patch_rejects_model_plus_effort_for_agy(self, providers_file): + """agy's slugs name their own effort, so the pair is either redundant or + contradictory — and agy rejects it either way. Better a 400 here than a + subprocess failure on the next review.""" + with pytest.raises(HTTPException) as excinfo: + await providers_api.patch_providers({"roles": {"video_review": { + "provider": "agy", "model": "gemini-3.8-flash-low", "effort": "low"}}}) + assert excinfo.value.status_code == 400 + assert "pick one" in excinfo.value.detail + + @pytest.mark.asyncio + async def test_roles_patch_allows_agy_with_a_model_and_no_effort(self, providers_file): + result = await providers_api.patch_providers({"roles": {"video_review": { + "provider": "agy", "model": "gemini-3.8-flash-low"}}}) + assert result["roles"]["video_review"] == { + "provider": "agy", "model": "gemini-3.8-flash-low", "effort": None} + + @pytest.mark.asyncio + async def test_active_and_roles_in_one_body_keeps_the_explicit_role(self, providers_file): + """The handler advertises "active, roles, or both". The whole-agent + sweep must not undo a role the same request named: that entry is the + more specific instruction and it already passed validation.""" + result = await providers_api.patch_providers({ + "active": "codex", + "roles": {"video_review": { + "provider": "codex", "model": "gpt-5.6-sol", "effort": "high"}}, + }) + assert result["active"] == "codex" + assert result["roles"]["video_review"] == { + "provider": "codex", "model": "gpt-5.6-sol", "effort": "high"} + + @pytest.mark.asyncio + async def test_reasserting_the_same_provider_keeps_the_model(self, providers_file): + """`/fk-change-provider set claude` on a config already on claude must + be a no-op, not a quiet reset. The reason a model is dropped on a + switch — a slug means nothing to a different CLI — does not apply when + the CLI did not change.""" + providers_file.write_text(json.dumps({"active": "claude", "roles": { + "video_review": {"provider": "claude", "model": "opus", "effort": "high"}}})) - monkeypatch.setattr(providers_api, "_PROVIDERS_FILE", tmp_file) - monkeypatch.setattr(providers_api.shutil, "which", lambda binary: "/usr/local/bin/fake") + result = await providers_api.patch_providers({"active": "claude"}) - # Ensure config.CLI_PROVIDERS mutation doesn't leak to other tests. - original_cli_providers = dict(providers_api.config.CLI_PROVIDERS) - monkeypatch.setattr( - providers_api.config, "CLI_PROVIDERS", dict(original_cli_providers), raising=False + assert result["roles"]["video_review"] == { + "provider": "claude", "model": "opus", "effort": "high"} + + @pytest.mark.asyncio + async def test_roles_patch_keeps_active_in_step(self, providers_file): + """`active` is what /fk-change-provider and the statusline read. If the + dashboard can move video_review without moving `active`, those two + report a provider that is not running.""" + await providers_api.patch_providers( + {"roles": {"video_review": {"provider": "codex"}}} ) + assert json.loads(providers_file.read_text())["active"] == "codex" - result = await providers_api.patch_providers({"active": "codex"}) + @pytest.mark.asyncio + async def test_roles_patch_rejects_an_effort_the_provider_lacks(self, providers_file): + with pytest.raises(HTTPException) as excinfo: + await providers_api.patch_providers({"roles": {"video_review": { + "provider": "agy", "effort": "xhigh"}}}) + assert excinfo.value.status_code == 400 + assert "xhigh" in excinfo.value.detail - assert result == {"status": "updated", "active": "codex"} - assert json.loads(tmp_file.read_text())["active"] == "codex" - assert providers_api.config.CLI_PROVIDERS["active"] == "codex" + @pytest.mark.asyncio + async def test_roles_patch_rejects_an_unknown_role(self, providers_file): + with pytest.raises(HTTPException) as excinfo: + await providers_api.patch_providers({"roles": {"nope": {"provider": "claude"}}}) + assert excinfo.value.status_code == 400 + assert "nope" in excinfo.value.detail + + @pytest.mark.asyncio + async def test_roles_patch_accepts_an_unlisted_model_for_a_non_authoritative_cli( + self, providers_file + ): + """claude takes aliases and full model names, codex takes slugs newer + than its on-disk cache. Validating those against a catalog would break + the day a new model ships.""" + result = await providers_api.patch_providers({"roles": {"video_review": { + "provider": "claude", "model": "claude-something-not-in-any-catalog"}}}) + assert result["roles"]["video_review"]["model"] == "claude-something-not-in-any-catalog" + + +class TestGetProviders: + @pytest.mark.asyncio + async def test_reports_efforts_and_roles_without_probing(self, providers_file): + """live=false must not spawn anything — the dashboard polls this.""" + with patch.object(providers_api, "_probe_version", new=AsyncMock()) as probe: + body = await providers_api.get_providers() + probe.assert_not_called() + + assert body["providers"]["agy"]["efforts"] == ["low", "medium", "high"] + assert "xhigh" in body["providers"]["claude"]["efforts"] + assert body["providers"]["agy"]["catalog_is_authoritative"] is True + assert body["providers"]["claude"]["catalog_is_authoritative"] is False + assert body["roles"]["video_review"]["provider"] == "claude" + assert "video_review" in body["role_meta"] + + @pytest.mark.asyncio + async def test_unknown_provider_models_raises_400(self): + with pytest.raises(HTTPException) as excinfo: + await providers_api.get_provider_models(provider="nope", refresh=False) + assert excinfo.value.status_code == 400 + + +# --------------------------------------------------------------------------- +# agent/services/cli_providers.py :: role resolution +# --------------------------------------------------------------------------- + +class TestResolveRole: + def test_falls_back_to_legacy_active_when_the_role_is_absent(self, monkeypatch): + """An untouched providers.json has no `roles` key at all.""" + monkeypatch.setattr(cli_providers.config, "CLI_PROVIDERS", {"active": "codex"}) + assert cli_providers.resolve_role("video_review") == { + "provider": "codex", "model": None, "effort": None} + + def test_role_entry_wins_over_active(self, monkeypatch): + monkeypatch.setattr(cli_providers.config, "CLI_PROVIDERS", { + "active": "claude", + "roles": {"video_review": {"provider": "agy", "model": "m", "effort": "high"}}}) + assert cli_providers.resolve_role("video_review") == { + "provider": "agy", "model": "m", "effort": "high"} + + def test_unknown_provider_degrades_to_the_default(self, monkeypatch): + """Hand-edited config should not take the review path down with a + KeyError deep inside the runner dispatch.""" + monkeypatch.setattr(cli_providers.config, "CLI_PROVIDERS", {"active": "gemini"}) + assert cli_providers.resolve_role("video_review")["provider"] == "claude" + + def test_effort_outside_the_ladder_is_dropped_not_forwarded(self, monkeypatch): + """agy rejects xhigh. Forwarding it costs a subprocess round trip to + learn what the ladder already says here.""" + monkeypatch.setattr(cli_providers.config, "CLI_PROVIDERS", { + "roles": {"video_review": {"provider": "agy", "effort": "xhigh"}}}) + assert cli_providers.resolve_role("video_review")["effort"] is None + + def test_empty_config_still_resolves(self, monkeypatch): + monkeypatch.setattr(cli_providers.config, "CLI_PROVIDERS", {}) + assert cli_providers.resolve_role("video_review")["provider"] == "claude" + + +class TestValidateRoleEntry: + def test_missing_binary_is_rejected(self, monkeypatch): + monkeypatch.setattr(cli_providers.shutil, "which", lambda b: None) + with pytest.raises(ValueError, match="not found on PATH"): + cli_providers.validate_role_entry("video_review", {"provider": "claude"}) + + def test_non_string_model_is_rejected(self, monkeypatch): + monkeypatch.setattr(cli_providers.shutil, "which", lambda b: "/bin/fake") + with pytest.raises(ValueError, match="string or null"): + cli_providers.validate_role_entry("video_review", {"provider": "claude", "model": 7}) + + def test_a_model_starting_with_a_dash_is_rejected(self, monkeypatch): + """Models are otherwise unvalidated on purpose, and the value lands in + argv right after --model. Nothing legitimate starts with a dash.""" + monkeypatch.setattr(cli_providers.shutil, "which", lambda b: "/bin/fake") + with pytest.raises(ValueError, match="must not start with"): + cli_providers.validate_role_entry( + "video_review", + {"provider": "claude", "model": "--dangerously-skip-permissions"}, + ) + + def test_blank_model_and_effort_normalise_to_none(self, monkeypatch): + """The dashboard's "Default" option sends an empty value; it must mean + "let the CLI decide", not an empty --model argument.""" + monkeypatch.setattr(cli_providers.shutil, "which", lambda b: "/bin/fake") + assert cli_providers.validate_role_entry( + "video_review", {"provider": "claude", "model": "", "effort": ""} + ) == {"provider": "claude", "model": None, "effort": None} + + +# --------------------------------------------------------------------------- +# agent/services/cli_providers.py :: model catalogs +# --------------------------------------------------------------------------- + +class TestModelCatalogs: + @pytest.mark.asyncio + async def test_agy_catalog_skips_the_header_line(self, monkeypatch): + """`agy models` prints "Fetching available models..." before the real + rows. Only the tab-separated lines are models.""" + stdout = ( + b"Fetching available models...\n" + b"gemini-3.8-flash-low\tGemini 3.8 Flash (Low)\n" + b"claude-sonnet-4-6\tClaude Sonnet 4.6 (Thinking)\n" + ) + proc = make_proc(stdout=stdout, returncode=0) + monkeypatch.setattr(cli_providers.shutil, "which", lambda b: "/bin/fake") + with patch("agent.services.cli_providers.asyncio.create_subprocess_exec", + new=AsyncMock(return_value=proc)): + models = await cli_providers.list_models("agy", force=True) + + assert models == [ + {"id": "gemini-3.8-flash-low", "label": "Gemini 3.8 Flash (Low)"}, + {"id": "claude-sonnet-4-6", "label": "Claude Sonnet 4.6 (Thinking)"}, + ] + + @pytest.mark.asyncio + async def test_a_failed_listing_is_an_empty_list_not_an_exception(self, monkeypatch): + """The dashboard asks for this on every provider change. A missing or + broken CLI must degrade to "no catalog", not 500 the settings page.""" + proc = make_proc(stdout=b"", stderr=b"boom", returncode=1) + monkeypatch.setattr(cli_providers.shutil, "which", lambda b: "/bin/fake") + with patch("agent.services.cli_providers.asyncio.create_subprocess_exec", + new=AsyncMock(return_value=proc)): + assert await cli_providers.list_models("agy", force=True) == [] + + @pytest.mark.asyncio + async def test_a_missing_binary_is_an_empty_list(self, monkeypatch): + monkeypatch.setattr(cli_providers.shutil, "which", lambda b: None) + assert await cli_providers.list_models("agy", force=True) == [] + + @pytest.mark.asyncio + async def test_an_empty_catalog_is_not_cached(self, monkeypatch): + """Emptiness here always means a transient failure, so caching it would + wedge the picker for five minutes after the CLI comes back.""" + monkeypatch.setattr(cli_providers.shutil, "which", lambda b: "/bin/fake") + cli_providers.clear_catalog_cache() + + fail = make_proc(stdout=b"", stderr=b"boom", returncode=1) + with patch("agent.services.cli_providers.asyncio.create_subprocess_exec", + new=AsyncMock(return_value=fail)): + assert await cli_providers.list_models("agy") == [] + + ok = make_proc(stdout=b"x\tX\n", returncode=0) + with patch("agent.services.cli_providers.asyncio.create_subprocess_exec", + new=AsyncMock(return_value=ok)): + assert await cli_providers.list_models("agy") == [{"id": "x", "label": "X"}] + cli_providers.clear_catalog_cache() + + def test_codex_catalog_hides_models_codex_hides(self, monkeypatch, tmp_path): + """`codex models` cannot run headlessly ("stdin is not a terminal"), so + its own cache is the only listing available — and it marks some entries + hidden, which are not offerable.""" + cache = tmp_path / "models_cache.json" + cache.write_text(json.dumps({"models": [ + {"slug": "gpt-5.6-sol", "display_name": "GPT-5.6-Sol", "visibility": "list", + "supported_reasoning_levels": [{"effort": "low"}, {"effort": "high"}]}, + {"slug": "gpt-reserve", "display_name": "GPT-Reserve", "visibility": "hide"}, + ]})) + monkeypatch.setattr(cli_providers, "_CODEX_MODELS_CACHE", cache) + + models = cli_providers._list_codex_models() + assert [m["id"] for m in models] == ["gpt-5.6-sol"] + assert models[0]["efforts"] == ["low", "high"] + + def test_codex_catalog_without_a_cache_file_is_empty(self, monkeypatch, tmp_path): + monkeypatch.setattr(cli_providers, "_CODEX_MODELS_CACHE", tmp_path / "nope.json") + assert cli_providers._list_codex_models() == [] + + +# --------------------------------------------------------------------------- +# ffmpeg drawtext fallback +# --------------------------------------------------------------------------- + +class TestFrameFilter: + def test_includes_drawtext_when_ffmpeg_has_it(self, monkeypatch): + monkeypatch.setattr("agent.services.video_reviewer._has_drawtext", lambda: True) + chain = _frame_filter(4.0) + assert chain.startswith("fps=4.0,scale=320:-1,drawtext=") + assert "%{pts\\:hms}" in chain + + def test_omits_drawtext_when_ffmpeg_lacks_it(self, monkeypatch): + """Homebrew's ffmpeg 8.x ships without libfreetype. Naming a filter + that does not exist aborts the whole chain with "No such filter", which + took down frame extraction and therefore every review — not just the + timestamps.""" + monkeypatch.setattr("agent.services.video_reviewer._has_drawtext", lambda: False) + assert _frame_filter(4.0) == "fps=4.0,scale=320:-1" diff --git a/tests/unit/test_video_reviewer.py b/tests/unit/test_video_reviewer.py index 3219504f3..bc5ff39e6 100644 --- a/tests/unit/test_video_reviewer.py +++ b/tests/unit/test_video_reviewer.py @@ -137,3 +137,173 @@ def test_partial_trailing_chunk_uses_zero_waste_layout( ) finally: shutil.rmtree(out_dir, ignore_errors=True) + + +# --------------------------------------------------------------------------- +# review_scene_video :: what a malformed CLI answer is allowed to become +# --------------------------------------------------------------------------- + +class TestReviewScoringRefusesAFabricatedScore: + """Every field of DimensionScores has a 5.0 default, so an answer carrying + no `dimensions` used to become a complete, plausible review — 5.0 across + the board, verdict "poor", zero errors — indistinguishable from a real + verdict on a mediocre video. That is the same class of bug as a CLI + returning an empty response on a zero exit code, and it has to fail loudly + instead. + """ + + @staticmethod + def _patched(monkeypatch, analysis): + import agent.services.video_reviewer as vr + + async def fake_download(url, dest): + Path(dest).write_bytes(b"not really a video") + + def fake_sheets(video_path, fps, out_dir): + sheet = Path(out_dir) / "sheet_00.jpg" + sheet.write_bytes(b"jpeg") + return [sheet], 9 + + async def fake_analyze(sheets, n_frames, fps, scene, timestamped=None): + return analysis + + monkeypatch.setattr(vr, "ANTHROPIC_API_KEY", "") + monkeypatch.setattr(vr, "_download_video", fake_download) + monkeypatch.setattr(vr, "_create_contact_sheets", fake_sheets) + monkeypatch.setattr(vr, "_analyze_cli", fake_analyze) + return vr + + SCENE = {"id": "s1", "vertical_video_url": "https://example.test/v.mp4", + "prompt": "p", "video_prompt": "vp", "character_names": "[]"} + + @pytest.mark.asyncio + async def test_an_answer_with_no_dimensions_raises(self, monkeypatch): + vr = self._patched(monkeypatch, {"errors": [], "usable_segments": []}) + with pytest.raises(RuntimeError, match="no dimensions"): + await vr.review_scene_video(dict(self.SCENE), []) + + @pytest.mark.asyncio + async def test_an_empty_dimensions_object_raises(self, monkeypatch): + vr = self._patched(monkeypatch, {"dimensions": {}, "errors": []}) + with pytest.raises(RuntimeError, match="no dimensions"): + await vr.review_scene_video(dict(self.SCENE), []) + + @pytest.mark.asyncio + async def test_a_partial_dimensions_object_still_defaults(self, monkeypatch): + """A model that scored some axes and not others is answering, just + incompletely — that is worth keeping, unlike one that answered nothing. + """ + vr = self._patched(monkeypatch, { + "dimensions": {"character_consistency": 9.0}, "errors": [], "usable_segments": []}) + review = await vr.review_scene_video(dict(self.SCENE), []) + assert review.dimensions.character_consistency == 9.0 + assert review.dimensions.motion_quality == 5.0 # the documented default + + GOOD_DIMS = {"character_consistency": 9.0, "prompt_adherence": 9.0, + "motion_quality": 9.0, "visual_fidelity": 9.0, + "temporal_coherence": 9.0, "composition": 9.0} + + @pytest.mark.asyncio + async def test_a_near_miss_key_keeps_the_critical_instead_of_dropping_it(self, monkeypatch): + """`timeRange` for `time_range` used to drop the whole entry, and what + drops with it is usually CRITICAL — the one severity that caps + character_consistency at 3.0 and forces the verdict below acceptable. + An unusable video came back clean over a camelCase key. + + The entry is repaired rather than refused: the model found the defect + and said so, it just spelled one field name oddly. Failing the scene + here would throw away a correct finding. + """ + vr = self._patched(monkeypatch, { + "dimensions": dict(self.GOOD_DIMS), + "errors": [{"severity": "CRITICAL", "timeRange": "3s-5s", + "description": "the dog becomes a cat"}], + "usable_segments": [], + }) + review = await vr.review_scene_video(dict(self.SCENE), []) + + assert [e.severity for e in review.errors] == ["CRITICAL"] + assert review.errors[0].time_range == "3s-5s" + assert review.has_critical_errors is True + assert review.dimensions.character_consistency == 3.0 + assert review.overall_score <= 5.9 + + @pytest.mark.asyncio + async def test_a_missing_time_range_is_repaired_not_refused(self, monkeypatch): + """Losing a timestamp costs the reader context; it cannot move a score. + Same placeholder the legacy plain-string path has always used.""" + vr = self._patched(monkeypatch, { + "dimensions": dict(self.GOOD_DIMS), + "errors": [{"severity": "MINOR", "description": "candle count drifts"}], + "usable_segments": [], + }) + review = await vr.review_scene_video(dict(self.SCENE), []) + assert review.errors[0].time_range == "?" + assert review.errors[0].description == "candle count drifts" + + @pytest.mark.asyncio + @pytest.mark.parametrize("severity", [None, "", "SEVERE", "MAJOR", "Critical character drift"]) + async def test_an_unrecognisable_severity_fails_the_scene(self, monkeypatch, severity): + """The asymmetry that makes the rest safe. `has_critical_errors`, the + character_consistency cap and `_fix_guide` all branch on this exact + string, so a severity outside {CRITICAL, HIGH, MINOR} silently disables + all three — the model flagged something and the score does not show it. + Unlike a missing timestamp, there is no safe default: we do not know + whether the video passed.""" + entry = {"time_range": "3s-5s", "description": "character morphs"} + if severity is not None: + entry["severity"] = severity + vr = self._patched(monkeypatch, { + "dimensions": dict(self.GOOD_DIMS), "errors": [entry], "usable_segments": []}) + with pytest.raises(RuntimeError, match="no usable severity"): + await vr.review_scene_video(dict(self.SCENE), []) + + @pytest.mark.asyncio + async def test_an_unreadable_segment_is_dropped_not_raised(self, monkeypatch): + """A lost segment errs toward "less usable footage", which cannot turn + a bad video into a good score the way a lost CRITICAL can — so it is a + drop, not a failure. It is still logged.""" + vr = self._patched(monkeypatch, { + "dimensions": dict(self.GOOD_DIMS), + "errors": [], + "usable_segments": [ + {"time_range": "0s-4s", "score": 8.0}, + {"time_range": "4s-8s"}, # no score — unreadable + {"timeRange": "0s-2s", "score": 7.0}, # near-miss key — repaired + "not even a dict", + ], + }) + review = await vr.review_scene_video(dict(self.SCENE), []) + assert [(s.time_range, s.score) for s in review.usable_segments] == [ + ("0s-4s", 8.0), ("0s-2s", 7.0)] + + @pytest.mark.asyncio + async def test_a_well_formed_critical_still_caps_the_score(self, monkeypatch): + """The other half of the same property: a CRITICAL that IS readable + must go on capping the score, so the strictness above is not covering + for a parser that stopped working.""" + vr = self._patched(monkeypatch, { + "dimensions": {"character_consistency": 9.0, "prompt_adherence": 9.0, + "motion_quality": 9.0, "visual_fidelity": 9.0, + "temporal_coherence": 9.0, "composition": 9.0}, + "errors": [{"severity": "critical", "time_range": "3s-5s", + "description": "the dog becomes a cat"}], + "usable_segments": [], + }) + review = await vr.review_scene_video(dict(self.SCENE), []) + assert review.has_critical_errors is True + assert review.dimensions.character_consistency == 3.0 + assert review.overall_score <= 5.9 + assert review.verdict in ("poor", "unusable") + + @pytest.mark.asyncio + async def test_a_plain_string_error_is_still_accepted(self, monkeypatch): + """The documented legacy shape. Strictness must not break it.""" + vr = self._patched(monkeypatch, { + "dimensions": {"character_consistency": 8.0}, + "errors": ["camera drifts after 4s"], + "usable_segments": [], + }) + review = await vr.review_scene_video(dict(self.SCENE), []) + assert [e.description for e in review.errors] == ["camera drifts after 4s"] + assert review.errors[0].severity == "HIGH" From 32ea1a5e8f014647a552edbc79c5a1fdab949240 Mon Sep 17 00:00:00 2001 From: Hoang Tuan Nguyen Date: Sun, 20 Sep 2026 00:52:55 +0700 Subject: [PATCH 2/4] ci: drawtext is a warning now, not a build breaker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The step existed because naming an absent filter aborted the whole ffmpeg chain and took 13 contact-sheet tests down with it. video_reviewer.py now probes for drawtext and drops it when missing, and the untimestamped path has its own coverage, so its absence is a supported degradation. The render check stays — the timestamped path is the better one and a silent loss of it belongs in the log — but it no longer fails the build for a condition the code handles. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Ka5BxVaWiQeWNDJakpCJJP --- .github/workflows/tests.yml | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 229aa5278..ed84785d8 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -47,20 +47,27 @@ jobs: sudo apt-get update sudo apt-get install -y --no-install-recommends ffmpeg fonts-dejavu-core - - name: Assert drawtext actually renders - # 13 of these tests burn a timestamp into extracted frames. Checking - # `ffmpeg -filters` only proves the filter is compiled in, not that a - # font exists, so render one frame through the same filter the tests - # use. Both failure modes surface here with a message that says what is - # wrong, instead of as 13 tests failing on "Filter not found". + - name: Report whether drawtext renders + # This used to fail the build, because naming an absent filter aborted + # the whole chain and took 13 tests down with it. video_reviewer.py now + # probes for drawtext and drops it when it is missing, so its absence + # is a supported degradation (sheets lose their burned-in timestamps) + # rather than a build breaker — and the fallback has its own coverage. + # + # The check stays because the timestamped path is the better one and a + # silent loss of it is worth seeing in the log. Checking + # `ffmpeg -filters` would only prove the filter is compiled in, not + # that a font exists, so render a real frame through the filter the + # code uses. run: | ffmpeg -hide_banner -loglevel error \ -f lavfi -i "testsrc=duration=1:size=320x240:rate=5" \ -vf "drawtext=text='%{pts\:hms}':x=5:y=5:fontsize=14:fontcolor=white:borderw=1:bordercolor=black" \ - -frames:v 1 -y /tmp/drawtext-smoke.jpg || { - echo "::error::ffmpeg cannot render drawtext — missing --enable-libfreetype, or no font installed." + -frames:v 1 -y /tmp/drawtext-smoke.jpg \ + && echo "drawtext renders — contact sheets will carry timestamps" \ + || { + echo "::warning::ffmpeg cannot render drawtext (no --enable-libfreetype, or no font). Sheets will be untimestamped; the suite still covers that path." ffmpeg -hide_banner -version | head -3 - exit 1 } - name: Install dependencies From 4391da911abe83db3d4551afe0b9d6a76e4f5b9b Mon Sep 17 00:00:00 2001 From: Hoang Tuan Nguyen Date: Sun, 20 Sep 2026 11:46:48 +0700 Subject: [PATCH 3/4] fix(review): close the findings from the adversarial review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One review, one provider. `resolve_role` ran per scene, and each scene awaits a download, an executor hop and a subprocess, so the loop yields repeatedly — while providers.json is documented as hand-editable and a dashboard GET hot-reloads it. Scenes 4..N could run on a different backend than scenes 1..3, with overall_score averaging both and no record of which produced what. Resolved once in review_video and threaded down. The closed catalog is now enforced. catalog_is_authoritative was published to the client and checked nowhere, so a typo'd agy slug got a 200 and then failed seconds into the next review with a raw CLI error — the exact outcome resolve_role's own docstring says this module exists to prevent. An empty catalog still does not block: emptiness means the listing failed, not that the provider has no models. CLI failures carry stdout too. claude puts the readable sentence there ("There's an issue with the selected model (...)") and buries the machine tag under paragraphs of unrelated context advice on stderr. Dropping stdout left the operator reading about token windows. providers.json is written atomically. Truncating in place was survivable when the file changed on a rare provider switch; it is written on every dashboard settings change now, and a truncated file hard-fails agent/config.py at import, so the server would not boot at all. The drawtext probe is an optimisation, not a correctness check. `ffmpeg -filters` proves the filter is compiled in, not that it can render — a build with libfreetype and no resolvable font lists it and then dies on "Cannot find a valid font for the family Sans", which is the original symptom on a box where the probe says everything is fine. Extraction retries untimestamped, and _create_contact_sheets now returns what actually happened rather than letting the caller re-derive it. Smaller ones from the same pass: resolve_role drops a model the API would have rejected, since the file is hand-editable and the two paths have to agree; an `active` sweep no longer silently adopts a role name this build does not know, which the `roles` path 400s on; list_models hands back a copy rather than the cached list itself; and the comments claiming scripts/statusline.sh reads `active` are gone — it does not, the sole reader is skills/fk-change-provider.md. Test gaps the same pass named, now closed: _has_drawtext had zero coverage and its regex had never run against real `ffmpeg -filters` output; _run_claude_cli had no model/effort/add-dirs argv test while agy had three; _spawn_and_check's message was untested. The API tests also stubbed no catalog, so validating an agy model spawned a real `agy models` and passed in CI only because a missing binary degrades to empty. 338 -> 352 tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Ka5BxVaWiQeWNDJakpCJJP --- README.md | 1 + agent/api/providers.py | 48 +++++-- agent/services/cli_providers.py | 46 +++++-- agent/services/video_reviewer.py | 107 ++++++++++++--- skills/fk-change-provider.md | 10 +- tests/unit/test_cli_providers.py | 207 ++++++++++++++++++++++++++++-- tests/unit/test_video_reviewer.py | 122 +++++++++++++++++- 7 files changed, 471 insertions(+), 70 deletions(-) diff --git a/README.md b/README.md index 5c3411477..e5094ae23 100644 --- a/README.md +++ b/README.md @@ -916,6 +916,7 @@ it looked fixable from the symptoms. | 2026-09-20 | **A review with no scores in it is now a failure, not a score.** Every dimension defaults to 5.0, so a CLI answer carrying no `dimensions` became a complete, plausible review — 5.0 across the board, verdict "poor", zero errors — of a video nothing had actually looked at | | 2026-09-20 | **stdin closed for all three CLIs.** Each appends piped stdin to the prompt when stdin is not a terminal — codex documents it as a `` block. Under uvicorn that is whatever the launching shell handed down | | 2026-09-20 | **Per-role provider, model and effort**, editable in the dashboard under Settings or via `PATCH /api/providers`. Efforts are validated against each CLI's real ladder (agy stops at `high`); models are validated only for agy, whose catalog is closed, so a slug newer than claude's or codex's cache still goes through. A whole-agent `{"active": …}` switch clears each role's model and clamps its effort, because neither survives a change of CLI | +| 2026-09-20 | Review hardening from an adversarial pass: one review is pinned to one provider (the role was resolved per scene, so a hand edit or a dashboard poll mid-run could split a video's score across two backends); an unknown agy model is a 400 naming the known slugs instead of a failed review; CLI failures carry stdout as well as stderr (claude puts its readable sentence there); `providers.json` is written atomically (a truncated file hard-fails `config.py` at import, so the server would not boot); and the drawtext probe is an optimisation now — extraction retries untimestamped if the filter is listed but cannot render, which is what an ffmpeg with libfreetype and no font does | | 2026-09-20 | **agy's model and effort are mutually exclusive** and the config now says so. Its slugs name their own effort (`gemini-3.8-flash-low`), so the pair is rejected — a mismatch conflicts, and a slug with no effort in its name refuses `--effort` at all | ### v1.2.0 — 2026-09-18 — the Flow migration diff --git a/agent/api/providers.py b/agent/api/providers.py index a16f1430d..bc8e5f43c 100644 --- a/agent/api/providers.py +++ b/agent/api/providers.py @@ -1,13 +1,18 @@ """CLI provider config API — which AI CLI, model and effort each role runs on. -`active` is the legacy single-provider switch that `/fk-change-provider` and the -statusline still use; `roles` is the per-role configuration the dashboard edits. -The two are kept consistent by every write here, so neither reader ever sees a -stale answer. +`active` is the legacy single-provider switch; `roles` is the per-role +configuration the dashboard edits. Every write here keeps the two consistent, +so neither reader sees a stale answer. + +The only real consumer of `active` is `skills/fk-change-provider.md` +(`scripts/statusline.sh` does not read it — its `active` matches are the +unrelated `/api/active-project`). One reader is reason enough to keep the +field in step; it is not reason to grow more of them. """ import asyncio import json import logging +import os import shutil from pathlib import Path @@ -48,9 +53,25 @@ def _read() -> dict: def _write(data: dict): - with open(_PROVIDERS_FILE, "w") as f: - json.dump(data, f, indent=2) - f.write("\n") + """Write atomically. + + `open(..., "w")` truncates in place, so a crash or a full disk mid-write + leaves a truncated providers.json — which then 500s every request here and + hard-fails `agent/config.py`'s import, so the server will not boot at all. + That was survivable when this file changed on a rare provider switch; it is + written on every dashboard settings change now. + """ + tmp = _PROVIDERS_FILE.with_suffix(".json.tmp") + try: + with open(tmp, "w") as f: + json.dump(data, f, indent=2) + f.write("\n") + f.flush() + os.fsync(f.fileno()) + os.replace(tmp, _PROVIDERS_FILE) # atomic within the same directory + except Exception: + tmp.unlink(missing_ok=True) + raise def _apply(data: dict): @@ -152,7 +173,7 @@ async def patch_providers(body: dict): raise HTTPException(400, "'roles' must be an object keyed by role name") for role, entry in incoming.items(): try: - roles[role] = validate_role_entry(role, entry) + roles[role] = await validate_role_entry(role, entry) except ValueError as e: raise HTTPException(400, str(e)) explicit = set(incoming) @@ -165,7 +186,12 @@ async def patch_providers(body: dict): raise HTTPException(400, f"'{PROVIDER_BINARIES[provider]}' binary not found on PATH — install it first") data["active"] = provider # Switching the whole agent over has to carry the roles with it. - for role in list(roles) + [r for r in ROLES if r not in roles]: + # Only roles this build knows about. A name left behind by another + # version (or a typo in a hand-edited file) is neither rewritten here + # nor accepted by the `roles` path, which 400s on it — one policy for + # both, rather than silently adopting it on one and rejecting it on the + # other. + for role in ROLES: if role in explicit: # The same request configured this role by name. That is the # more specific instruction and it already passed validation — @@ -188,8 +214,8 @@ async def patch_providers(body: dict): data["roles"] = roles if "active" not in body and roles: - # Keep the legacy field meaningful for /fk-change-provider and the - # statusline: it reports whatever the primary role runs on. + # Keep the legacy field meaningful for /fk-change-provider: it reports + # whatever the primary role runs on. primary = next(iter(ROLES)) if primary in roles: data["active"] = roles[primary]["provider"] diff --git a/agent/services/cli_providers.py b/agent/services/cli_providers.py index ea73bc61b..81977d0cc 100644 --- a/agent/services/cli_providers.py +++ b/agent/services/cli_providers.py @@ -128,18 +128,22 @@ def resolve_role(role: str) -> dict: ) effort = None - return { - "provider": provider, - "model": entry.get("model") or None, - "effort": effort, - } + model = entry.get("model") or None + if model is not None and (not isinstance(model, str) or model.startswith("-")): + # The API refuses these, but providers.json is documented as safe to + # hand-edit, so the two paths have to agree on what is acceptable. + logger.warning("Dropping unusable model %r for role %r", model, role) + model = None + + return {"provider": provider, "model": model, "effort": effort} -def validate_role_entry(role: str, entry: dict) -> dict: +async def validate_role_entry(role: str, entry: dict) -> dict: """Normalise one role entry from an API body. Raises ValueError on bad input. - The model is deliberately *not* checked against the catalog for claude and - codex — see PROVIDER_CATALOG_IS_AUTHORITATIVE. + The model is checked against the catalog only where the catalog is closed + (agy) — see PROVIDER_CATALOG_IS_AUTHORITATIVE. For claude and codex an + unlisted slug is a legitimate escape hatch, not a typo. """ if role not in ROLES: raise ValueError(f"Unknown role '{role}'. Known: {sorted(ROLES)}") @@ -172,12 +176,24 @@ def validate_role_entry(role: str, entry: dict) -> dict: if model is not None and not isinstance(model, str): raise ValueError(f"Model for role '{role}' must be a string or null") if model is not None and model.startswith("-"): - # The model is otherwise unvalidated on purpose (the escape hatch for a - # slug newer than any catalog), and it lands in argv next to --model. - # No real slug starts with a dash, and refusing them removes any - # argument left to have about what a CLI's parser does with one. + # The model is otherwise unvalidated for the open-catalog providers (the + # escape hatch for a slug newer than any catalog), and it lands in argv + # next to --model. No real slug starts with a dash, and refusing them + # removes any argument left to have about what a CLI's parser does. raise ValueError(f"Model for role '{role}' must not start with '-'") + if model is not None and PROVIDER_CATALOG_IS_AUTHORITATIVE[provider]: + # agy validates --model itself and errors out several seconds into the + # run. Catching it here is the difference between a 400 that names the + # options and a failed review. An empty catalog means the listing call + # failed, not that no models exist, so it must not block the write. + known = await list_models(provider) + if known and not any(m["id"] == model for m in known): + raise ValueError( + f"Unknown {provider} model '{model}'. {provider} rejects anything " + f"outside its own catalog; known: {[m['id'] for m in known]}" + ) + return {"provider": provider, "model": model, "effort": effort} @@ -264,7 +280,7 @@ async def list_models(provider: str, force: bool = False) -> list[dict]: if not force: hit = _catalog_cache.get(provider) if hit and now - hit[0] < _CATALOG_TTL_S: - return hit[1] + return list(hit[1]) if not shutil.which(PROVIDER_BINARIES[provider]): return [] @@ -282,7 +298,9 @@ async def list_models(provider: str, force: bool = False) -> list[dict]: if models: _catalog_cache[provider] = (now, models) - return models + # A copy: the cache is shared for five minutes and a caller that mutated + # the list it got back would corrupt it for every later reader. + return list(models) def clear_catalog_cache() -> None: diff --git a/agent/services/video_reviewer.py b/agent/services/video_reviewer.py index f47cfa440..7afaf6fdc 100644 --- a/agent/services/video_reviewer.py +++ b/agent/services/video_reviewer.py @@ -222,10 +222,12 @@ def _has_drawtext() -> bool: return present -def _frame_filter(fps: float) -> str: +def _frame_filter(fps: float, drawtext: bool | None = None) -> str: """Filter chain for contact-sheet frames, timestamped where ffmpeg allows.""" + if drawtext is None: + drawtext = _has_drawtext() chain = f"fps={fps},scale=320:-1" - if _has_drawtext(): + if drawtext: chain += ( ",drawtext=text='%{pts\\:hms}':x=5:y=5:fontsize=14:" "fontcolor=white:borderw=1:bordercolor=black" @@ -233,23 +235,49 @@ def _frame_filter(fps: float) -> str: return chain -def _create_contact_sheets(video_path: str, fps: float, out_dir: str) -> tuple[list[Path], int]: +def _create_contact_sheets( + video_path: str, fps: float, out_dir: str +) -> tuple[list[Path], int, bool]: """Extract all frames and tile them into REVIEW_SHEET_COLSxREVIEW_SHEET_ROWS sheets. - Frames carry burned-in timestamps only where ffmpeg has `drawtext` — see - `_frame_filter`. `_analyze_cli` words the prompt to match either way. + Returns (sheet_paths in chronological order, total_frames after any + REVIEW_MAX_FRAMES cap, whether the frames carry burned-in timestamps). - Returns (sheet_paths in chronological order, total_frames after any REVIEW_MAX_FRAMES cap). + The caller is told what actually happened rather than re-deriving it from + `_has_drawtext()`: extraction can fall back to untimestamped frames at + runtime even on a build that lists the filter, and `_analyze_cli` has to + word the prompt for the sheets it really got. """ frames_dir = Path(out_dir) / "frames" frames_dir.mkdir(exist_ok=True) - extract_cmd = [ - "ffmpeg", "-y", "-i", video_path, - "-vf", _frame_filter(fps), - "-q:v", "2", - f"{frames_dir}/frame_%04d.jpg", - ] - result = subprocess.run(extract_cmd, capture_output=True, text=True) + + def _extract(with_drawtext: bool): + return subprocess.run( + [ + "ffmpeg", "-y", "-i", video_path, + "-vf", _frame_filter(fps, with_drawtext), + "-q:v", "2", + f"{frames_dir}/frame_%04d.jpg", + ], + capture_output=True, text=True, + ) + + timestamped = _has_drawtext() + result = _extract(timestamped) + if result.returncode != 0 and timestamped: + # The probe proves drawtext is compiled in, not that it can render. An + # ffmpeg with libfreetype but no resolvable font lists the filter and + # then dies on "Cannot find a valid font for the family Sans" — the + # original symptom all over again, on a box where the probe says + # everything is fine. One retry makes the probe an optimisation rather + # than a load-bearing correctness check. + tail = (result.stderr or "").strip().splitlines() + logger.warning( + "Frame extraction failed with drawtext (%s) — retrying untimestamped", + tail[-1] if tail else "no stderr", + ) + timestamped = False + result = _extract(False) if result.returncode != 0: raise RuntimeError(f"Frame extraction failed: {result.stderr[-500:]}") @@ -285,7 +313,7 @@ def _create_contact_sheets(video_path: str, fps: float, out_dir: str) -> tuple[l raise RuntimeError(f"Contact sheet tiling failed: {result.stderr[-500:]}") sheets.append(output) - return sheets, len(frames) + return sheets, len(frames), timestamped # ─── Claude Vision analysis ─────────────────────────────────── @@ -442,7 +470,15 @@ async def _spawn_and_check(args: tuple, provider: str) -> bytes: ) stdout, stderr = await _communicate_with_timeout(proc, provider) if proc.returncode != 0: - raise RuntimeError(f"{provider} CLI failed (rc={proc.returncode}): {stderr.decode()[-500:]}") + # Both streams. claude puts the readable sentence on stdout ("There's an + # issue with the selected model (...)") and buries the machine tag under + # paragraphs of unrelated context advice on stderr; codex is the other + # way round. Tail-slice each, because the useful part is always last. + detail = stderr.decode()[-500:] + out = stdout.decode().strip() + if out: + detail = f"{detail} | stdout: {out[-300:]}" + raise RuntimeError(f"{provider} CLI failed (rc={proc.returncode}): {detail}") return stdout @@ -603,14 +639,20 @@ async def _analyze_cli( fps: float, scene: dict, timestamped: bool | None = None, + role: dict | None = None, ) -> dict: - """Analyze contact sheets via the CLI provider configured for video_review.""" + """Analyze contact sheets via the CLI provider configured for video_review. + + `role` is passed in by a multi-scene review so every scene runs on the same + backend — see `review_video`. + """ if timestamped is None: timestamped = _has_drawtext() n_sheets = len(contact_sheets) base_prompt = _build_prompt(n_frames, fps, n_sheets, scene) - role = resolve_role("video_review") + if role is None: + role = resolve_role("video_review") provider = role["provider"] logger.info( "Calling %s CLI for vision analysis (%d frames, %d sheets, model=%s, effort=%s, timestamps=%s)", @@ -707,8 +749,13 @@ async def review_scene_video( mode: str = "light", orientation: str = "VERTICAL", project_id: str = None, + role: dict | None = None, ) -> SceneReview: - """Review a single scene's video via frame extraction + Claude Vision.""" + """Review a single scene's video via frame extraction + Claude Vision. + + `role` pins the provider/model/effort. `review_video` resolves it once and + passes it down; a single-scene call resolves it here. + """ fps = REVIEW_FPS_DEEP if mode == "deep" else REVIEW_FPS_LIGHT orient_prefix = "vertical" if orientation.upper() == "VERTICAL" else "horizontal" @@ -750,13 +797,16 @@ async def review_scene_video( else: # CLI path: contact sheets (no API key needed) logger.info("Creating contact sheets at %sfps (CLI mode)", fps) - contact_sheets, n_frames = await asyncio.get_event_loop().run_in_executor( + contact_sheets, n_frames, timestamped = await asyncio.get_event_loop().run_in_executor( None, _create_contact_sheets, str(video_path), fps, tmp ) if not contact_sheets or not all(s.exists() for s in contact_sheets): raise RuntimeError(f"Contact sheets not created for scene {scene['id']}") logger.info("Analyzing %d frames across %d sheets via CLI provider", n_frames, len(contact_sheets)) - result = await _analyze_cli(contact_sheets, n_frames, fps, scene) + result = await _analyze_cli( + contact_sheets, n_frames, fps, scene, + timestamped=timestamped, role=role, + ) # Parse structured errors with severity. # @@ -893,6 +943,18 @@ async def review_video( scenes = [s for s in scenes if s["id"] in id_set] characters = await get_project_characters(project_id) + # Resolved once, for the whole review. Each scene awaits a download, an + # executor hop and a subprocess, so the loop below yields repeatedly — and + # providers.json is documented as safe to hand-edit, while a dashboard GET + # hot-reloads it. Resolving per scene let scenes 4..N run on a different + # backend than scenes 1..3, and `overall_score` then averages two of them + # with no record of which produced what. + role = resolve_role("video_review") + logger.info( + "Reviewing %s on %s (model=%s, effort=%s)", + video_id, role["provider"], role["model"] or "default", role["effort"] or "default", + ) + orient_prefix = "vertical" if orientation.upper() == "VERTICAL" else "horizontal" scene_reviews = [] @@ -906,7 +968,10 @@ async def review_video( continue try: - review = await review_scene_video(scene, characters, mode=mode, orientation=orientation, project_id=project_id) + review = await review_scene_video( + scene, characters, mode=mode, orientation=orientation, + project_id=project_id, role=role, + ) scene_reviews.append(review) except Exception as e: logger.error("Failed to review scene %s: %s", scene["id"], e) diff --git a/skills/fk-change-provider.md b/skills/fk-change-provider.md index 42b46cdd7..9c48e7580 100644 --- a/skills/fk-change-provider.md +++ b/skills/fk-change-provider.md @@ -62,10 +62,12 @@ curl -s "http://127.0.0.1:8100/api/providers/models?provider=" | pytho - An **empty** `models` array is a normal answer, not an error — the binary is missing or the listing call failed. Fall back to the provider's default. -- `authoritative: true` (agy only) means that list is the whole truth and agy - rejects anything outside it. For `claude` and `codex` an unlisted slug is a - legitimate escape hatch — claude takes aliases like `sonnet` and full model - names, codex takes slugs newer than its on-disk cache. +- `authoritative: true` (agy only) means that list is the whole truth. The API + rejects a model outside it with a 400 naming the known slugs, rather than + letting agy reject it several seconds into the next review. For `claude` and + `codex` an unlisted slug is a legitimate escape hatch — claude takes aliases + like `sonnet` and full model names, codex takes slugs newer than its on-disk + cache — so those are not checked. - Offer only efforts from that provider's `efforts` array. **agy has no `xhigh` or `max`** and the API rejects them with a 400. - **If the provider has `model_encodes_effort: true` (agy), do not offer both.** diff --git a/tests/unit/test_cli_providers.py b/tests/unit/test_cli_providers.py index 2178eb448..0a046e1b0 100644 --- a/tests/unit/test_cli_providers.py +++ b/tests/unit/test_cli_providers.py @@ -44,6 +44,45 @@ def make_proc(stdout=b"", stderr=b"", returncode=0): # --------------------------------------------------------------------------- class TestRunClaudeCli: + @pytest.mark.asyncio + async def test_model_effort_and_add_dirs_reach_the_argv(self): + """The default provider's new arguments. agy had three tests for this + and claude had none, which is backwards — claude is what runs unless + someone changes the setting.""" + proc = make_proc(stdout=b"ok", returncode=0) + with patch( + "agent.services.video_reviewer.asyncio.create_subprocess_exec", + new=AsyncMock(return_value=proc), + ) as mock_exec: + await _run_claude_cli( + "hello", model="sonnet", effort="high", add_dirs=("/tmp/sheets",)) + argv = mock_exec.call_args[0] + assert argv[argv.index("--model") + 1] == "sonnet" + assert argv[argv.index("--effort") + 1] == "high" + assert argv[argv.index("--add-dir") + 1] == "/tmp/sheets" + # Unlike agy, claude's model does not encode the effort, so both stay. + assert "--effort" in argv and "--model" in argv + + @pytest.mark.asyncio + async def test_a_failure_carries_both_streams(self): + """claude puts the readable sentence on stdout and buries the machine + tag under paragraphs of unrelated context advice on stderr. Dropping + stdout left the operator reading about token windows.""" + proc = make_proc( + stdout=b"There's an issue with the selected model (nope-xyz).", + stderr=b"[claude-code:unrecognized_model] {\"model\":\"nope-xyz\"}", + returncode=1, + ) + with patch( + "agent.services.video_reviewer.asyncio.create_subprocess_exec", + new=AsyncMock(return_value=proc), + ): + with pytest.raises(RuntimeError) as excinfo: + await _run_claude_cli("hello", model="nope-xyz") + message = str(excinfo.value) + assert "unrecognized_model" in message # stderr tail + assert "issue with the selected model" in message # stdout, previously dropped + @pytest.mark.asyncio async def test_argv_and_return_value(self): proc = make_proc(stdout=b'{"ok": true}', stderr=b"", returncode=0) @@ -492,6 +531,12 @@ def test_multi_sheet_includes_sheet_note(self): # agent/api/providers.py :: patch_providers # --------------------------------------------------------------------------- +FAKE_AGY_CATALOG = [ + {"id": "gemini-3.8-flash-low", "label": "Gemini 3.8 Flash (Low)"}, + {"id": "gemini-3.1-pro-high", "label": "Gemini 3.1 Pro (High)"}, +] + + @pytest.fixture def providers_file(monkeypatch, tmp_path): """Point the API at a throwaway providers.json and isolate config.""" @@ -500,6 +545,15 @@ def providers_file(monkeypatch, tmp_path): monkeypatch.setattr(providers_api, "_PROVIDERS_FILE", tmp_file) monkeypatch.setattr(providers_api.shutil, "which", lambda binary: "/usr/local/bin/fake") monkeypatch.setattr(cli_providers.shutil, "which", lambda binary: "/usr/local/bin/fake") + # Stub the catalog. Validating an agy model otherwise spawns a real + # `agy models`, which makes these tests depend on a signed-in CLI and pass + # in CI only because the missing binary degrades to an empty catalog. + # Enforcement itself is covered in TestValidateRoleEntry. + monkeypatch.setattr( + cli_providers, "list_models", + AsyncMock(side_effect=lambda provider, force=False: list( + FAKE_AGY_CATALOG if provider == "agy" else [])), + ) # Ensure config.CLI_PROVIDERS mutation doesn't leak to other tests. monkeypatch.setattr( providers_api.config, "CLI_PROVIDERS", @@ -621,6 +675,32 @@ async def test_reasserting_the_same_provider_keeps_the_model(self, providers_fil assert result["roles"]["video_review"] == { "provider": "claude", "model": "opus", "effort": "high"} + @pytest.mark.asyncio + async def test_roles_patch_rejects_an_agy_model_outside_its_catalog(self, providers_file): + """agy's catalog is closed and it rejects anything else itself — several + seconds into the next review, with a raw CLI error. A 400 that names the + known slugs is the whole reason this module exists.""" + with pytest.raises(HTTPException) as excinfo: + await providers_api.patch_providers({"roles": {"video_review": { + "provider": "agy", "model": "gemini-9-does-not-exist"}}}) + assert excinfo.value.status_code == 400 + assert "gemini-3.8-flash-low" in excinfo.value.detail + + @pytest.mark.asyncio + async def test_roles_patch_leaves_an_unknown_role_in_the_file_alone(self, providers_file): + """An `active` sweep used to silently adopt a role name this build does + not know, while the `roles` path 400s on the same name. One policy for + both: names outside ROLES are neither rewritten nor accepted.""" + providers_file.write_text(json.dumps({"active": "claude", "roles": { + "legacy_role": {"provider": "agy", "model": None, "effort": None}, + "video_review": {"provider": "claude", "model": None, "effort": None}}})) + + result = await providers_api.patch_providers({"active": "codex"}) + + assert result["roles"]["video_review"]["provider"] == "codex" + assert result["roles"]["legacy_role"] == { + "provider": "agy", "model": None, "effort": None} + @pytest.mark.asyncio async def test_roles_patch_keeps_active_in_step(self, providers_file): """`active` is what /fk-change-provider and the statusline read. If the @@ -716,35 +796,75 @@ def test_empty_config_still_resolves(self, monkeypatch): assert cli_providers.resolve_role("video_review")["provider"] == "claude" +@pytest.fixture +def fake_binaries(monkeypatch): + monkeypatch.setattr(cli_providers.shutil, "which", lambda b: "/bin/fake") + + class TestValidateRoleEntry: - def test_missing_binary_is_rejected(self, monkeypatch): + @pytest.mark.asyncio + async def test_missing_binary_is_rejected(self, monkeypatch): monkeypatch.setattr(cli_providers.shutil, "which", lambda b: None) with pytest.raises(ValueError, match="not found on PATH"): - cli_providers.validate_role_entry("video_review", {"provider": "claude"}) + await cli_providers.validate_role_entry("video_review", {"provider": "claude"}) - def test_non_string_model_is_rejected(self, monkeypatch): - monkeypatch.setattr(cli_providers.shutil, "which", lambda b: "/bin/fake") + @pytest.mark.asyncio + async def test_non_string_model_is_rejected(self, fake_binaries): with pytest.raises(ValueError, match="string or null"): - cli_providers.validate_role_entry("video_review", {"provider": "claude", "model": 7}) + await cli_providers.validate_role_entry( + "video_review", {"provider": "claude", "model": 7}) - def test_a_model_starting_with_a_dash_is_rejected(self, monkeypatch): - """Models are otherwise unvalidated on purpose, and the value lands in - argv right after --model. Nothing legitimate starts with a dash.""" - monkeypatch.setattr(cli_providers.shutil, "which", lambda b: "/bin/fake") + @pytest.mark.asyncio + async def test_a_model_starting_with_a_dash_is_rejected(self, fake_binaries): + """Models are unvalidated for the open-catalog providers on purpose, + and the value lands in argv right after --model. Nothing legitimate + starts with a dash.""" with pytest.raises(ValueError, match="must not start with"): - cli_providers.validate_role_entry( + await cli_providers.validate_role_entry( "video_review", {"provider": "claude", "model": "--dangerously-skip-permissions"}, ) - def test_blank_model_and_effort_normalise_to_none(self, monkeypatch): + @pytest.mark.asyncio + async def test_blank_model_and_effort_normalise_to_none(self, fake_binaries): """The dashboard's "Default" option sends an empty value; it must mean "let the CLI decide", not an empty --model argument.""" - monkeypatch.setattr(cli_providers.shutil, "which", lambda b: "/bin/fake") - assert cli_providers.validate_role_entry( + assert await cli_providers.validate_role_entry( "video_review", {"provider": "claude", "model": "", "effort": ""} ) == {"provider": "claude", "model": None, "effort": None} + @pytest.mark.asyncio + async def test_an_unknown_model_is_rejected_where_the_catalog_is_closed( + self, fake_binaries + ): + """agy validates --model itself and errors out several seconds into the + run. Catching it here is the difference between a 400 that names the + options and a failed review.""" + with patch.object(cli_providers, "list_models", new=AsyncMock( + return_value=[{"id": "gemini-3.8-flash-low", "label": "x"}])): + with pytest.raises(ValueError, match="Unknown agy model"): + await cli_providers.validate_role_entry( + "video_review", {"provider": "agy", "model": "gemini-9-nope"}) + + @pytest.mark.asyncio + async def test_an_unknown_model_passes_where_the_catalog_is_open(self, fake_binaries): + """claude takes aliases and full names, codex takes slugs newer than + its cache. Validating those would break the day a new model ships.""" + with patch.object(cli_providers, "list_models", new=AsyncMock(return_value=[])): + entry = await cli_providers.validate_role_entry( + "video_review", {"provider": "claude", "model": "claude-brand-new"}) + assert entry["model"] == "claude-brand-new" + + @pytest.mark.asyncio + async def test_an_empty_catalog_does_not_block_the_write(self, fake_binaries): + """Emptiness means the listing call failed, not that agy has no models. + Blocking on it would make a transient `agy models` failure look like a + rejected setting.""" + with patch.object(cli_providers, "list_models", new=AsyncMock(return_value=[])): + entry = await cli_providers.validate_role_entry( + "video_review", {"provider": "agy", "model": "gemini-3.8-flash-low"}) + assert entry["model"] == "gemini-3.8-flash-low" + # --------------------------------------------------------------------------- # agent/services/cli_providers.py :: model catalogs @@ -829,6 +949,67 @@ def test_codex_catalog_without_a_cache_file_is_empty(self, monkeypatch, tmp_path # ffmpeg drawtext fallback # --------------------------------------------------------------------------- +# Real rows from `ffmpeg -hide_banner -filters`, kept verbatim: the probe is a +# regex over this exact shape and has never been run against it in a test. +_FILTERS_WITH_DRAWTEXT = """\ +Filters: + T.. = Timeline support + ... drawbox V->V Draw a colored box on the input video. + T.C drawtext V->V Draw text on top of video frames using libfreetype library. + ... scale V->V Scale the input video size and/or convert the image format. +""" + +_FILTERS_WITHOUT_DRAWTEXT = """\ +Filters: + T.. = Timeline support + ... drawbox V->V Draw a colored box on the input video. + ... drawgraph V->V Draw a graph using input video metadata. + ... scale V->V Scale the input video size and/or convert the image format. +""" + + +class TestHasDrawtext: + """The one function whose misbehaviour re-breaks every review. Both + _frame_filter tests stub it out, so without this its regex never runs + against real `ffmpeg -filters` output.""" + + @staticmethod + def _probe(stdout, returncode=0): + import agent.services.video_reviewer as vr + vr._has_drawtext.cache_clear() + completed = MagicMock(stdout=stdout, stderr="", returncode=returncode) + try: + with patch("agent.services.video_reviewer.subprocess.run", return_value=completed): + return vr._has_drawtext() + finally: + vr._has_drawtext.cache_clear() + + def test_finds_drawtext_in_a_real_filter_listing(self): + assert self._probe(_FILTERS_WITH_DRAWTEXT) is True + + def test_absent_drawtext_is_detected(self): + assert self._probe(_FILTERS_WITHOUT_DRAWTEXT) is False + + def test_a_description_mentioning_drawtext_is_not_a_match(self): + """The filter name is the second whitespace-delimited token. Matching + anywhere on the line would make any filter whose description mentions + drawtext a false positive — and a false positive here puts an absent + filter back into the chain, which aborts extraction entirely.""" + listing = " ... overlay V->V Like drawtext but for images.\n" + assert self._probe(listing) is False + + def test_an_ffmpeg_that_cannot_be_run_reports_no_drawtext(self): + """Fail safe: an unusable ffmpeg must not claim the filter is there.""" + import agent.services.video_reviewer as vr + vr._has_drawtext.cache_clear() + try: + with patch("agent.services.video_reviewer.subprocess.run", + side_effect=OSError("no ffmpeg")): + assert vr._has_drawtext() is False + finally: + vr._has_drawtext.cache_clear() + + class TestFrameFilter: def test_includes_drawtext_when_ffmpeg_has_it(self, monkeypatch): monkeypatch.setattr("agent.services.video_reviewer._has_drawtext", lambda: True) diff --git a/tests/unit/test_video_reviewer.py b/tests/unit/test_video_reviewer.py index bc5ff39e6..5287d11d5 100644 --- a/tests/unit/test_video_reviewer.py +++ b/tests/unit/test_video_reviewer.py @@ -33,7 +33,7 @@ class TestCreateContactSheetsChunking: def test_2s_video_at_4fps_produces_correct_sheet_count(self, synthetic_video): # 2s * 4fps = 8 frames -> ceil(8/9) = 1 sheet with tempfile.TemporaryDirectory() as out_dir: - sheets, total_frames = _create_contact_sheets(str(synthetic_video), 4, out_dir) + sheets, total_frames, _timestamped = _create_contact_sheets(str(synthetic_video), 4, out_dir) assert total_frames == 8 assert len(sheets) == 1 assert all(s.exists() and s.stat().st_size > 0 for s in sheets) @@ -41,7 +41,7 @@ def test_2s_video_at_4fps_produces_correct_sheet_count(self, synthetic_video): def test_2s_video_at_8fps_produces_correct_sheet_count(self, synthetic_video): # 2s * 8fps = 16 frames -> ceil(16/9) = 2 sheets with tempfile.TemporaryDirectory() as out_dir: - sheets, total_frames = _create_contact_sheets(str(synthetic_video), 8, out_dir) + sheets, total_frames, _timestamped = _create_contact_sheets(str(synthetic_video), 8, out_dir) assert total_frames == 16 assert len(sheets) == 2 assert all(s.exists() and s.stat().st_size > 0 for s in sheets) @@ -50,7 +50,7 @@ def test_review_max_frames_caps_total_and_sheet_count(self, synthetic_video, mon import agent.services.video_reviewer as vr monkeypatch.setattr(vr, "REVIEW_MAX_FRAMES", 5) with tempfile.TemporaryDirectory() as out_dir: - sheets, total_frames = _create_contact_sheets(str(synthetic_video), 8, out_dir) + sheets, total_frames, _timestamped = _create_contact_sheets(str(synthetic_video), 8, out_dir) # 16 natural frames capped to 5 -> ceil(5/9) = 1 sheet assert total_frames == 5 assert len(sheets) == 1 @@ -71,7 +71,7 @@ def test_downsampling_selects_correct_nonconsecutive_frames_across_chunks( monkeypatch.setattr(vr, "REVIEW_MAX_FRAMES", 20) out_dir = tempfile.mkdtemp() try: - sheets, total_frames = _create_contact_sheets(str(synthetic_video), 30, out_dir) + sheets, total_frames, _timestamped = _create_contact_sheets(str(synthetic_video), 30, out_dir) assert total_frames == 20 assert len(sheets) == 3 @@ -120,7 +120,7 @@ def test_partial_trailing_chunk_uses_zero_waste_layout( monkeypatch.setattr(vr, "REVIEW_MAX_FRAMES", chunk_size) out_dir = tempfile.mkdtemp() try: - sheets, total_frames = _create_contact_sheets(str(synthetic_video), 30, out_dir) + sheets, total_frames, _timestamped = _create_contact_sheets(str(synthetic_video), 30, out_dir) assert total_frames == chunk_size assert len(sheets) == 1 probe = subprocess.run( @@ -162,9 +162,9 @@ async def fake_download(url, dest): def fake_sheets(video_path, fps, out_dir): sheet = Path(out_dir) / "sheet_00.jpg" sheet.write_bytes(b"jpeg") - return [sheet], 9 + return [sheet], 9, True - async def fake_analyze(sheets, n_frames, fps, scene, timestamped=None): + async def fake_analyze(sheets, n_frames, fps, scene, timestamped=None, role=None): return analysis monkeypatch.setattr(vr, "ANTHROPIC_API_KEY", "") @@ -307,3 +307,111 @@ async def test_a_plain_string_error_is_still_accepted(self, monkeypatch): review = await vr.review_scene_video(dict(self.SCENE), []) assert [e.description for e in review.errors] == ["camera drifts after 4s"] assert review.errors[0].severity == "HIGH" + + +# --------------------------------------------------------------------------- +# drawtext: the probe is an optimisation, not a correctness check +# --------------------------------------------------------------------------- + +class TestDrawtextRuntimeFallback: + def test_extraction_retries_without_drawtext_when_the_filter_fails( + self, synthetic_video, monkeypatch + ): + """`ffmpeg -filters` proves drawtext is compiled in, not that it can + render. A build with libfreetype but no resolvable font lists the + filter and then dies on "Cannot find a valid font for the family Sans" + — the original symptom again, on a box where the probe says everything + is fine. One retry makes the probe an optimisation. + + The failure is forced through `_frame_filter` rather than by trusting + this machine's ffmpeg, so the retry is exercised on a runner that has a + working drawtext as well as on one that has none. + """ + import agent.services.video_reviewer as vr + + seen = [] + + def fake_filter(fps, drawtext=None): + seen.append(drawtext) + chain = f"fps={fps},scale=320:-1" + return chain + ",definitely_not_a_real_filter" if drawtext else chain + + monkeypatch.setattr(vr, "_has_drawtext", lambda: True) + monkeypatch.setattr(vr, "_frame_filter", fake_filter) + + with tempfile.TemporaryDirectory() as out_dir: + sheets, total_frames, timestamped = vr._create_contact_sheets( + str(synthetic_video), 4, out_dir) + + assert seen == [True, False] # tried timestamped, then fell back + assert timestamped is False # and says so, rather than guessing + assert total_frames == 8 + assert len(sheets) == 1 and sheets[0].stat().st_size > 0 + + def test_a_failure_unrelated_to_drawtext_is_not_retried_away( + self, synthetic_video, monkeypatch + ): + """The retry exists for one cause. A genuinely broken input must still + surface as an error instead of being masked by a second attempt.""" + import agent.services.video_reviewer as vr + + monkeypatch.setattr(vr, "_has_drawtext", lambda: False) + with tempfile.TemporaryDirectory() as out_dir: + with pytest.raises(RuntimeError, match="Frame extraction failed"): + vr._create_contact_sheets(str(Path(out_dir) / "nope.mp4"), 4, out_dir) + + +# --------------------------------------------------------------------------- +# one review, one provider +# --------------------------------------------------------------------------- + +class TestRoleIsPinnedForTheWholeReview: + @pytest.mark.asyncio + async def test_the_role_is_resolved_once_and_reused_for_every_scene(self, monkeypatch): + """Each scene awaits a download, an executor hop and a subprocess, so + the loop yields repeatedly — and providers.json is documented as safe + to hand-edit while a dashboard GET hot-reloads it. Resolving per scene + let scenes 4..N run on a different backend than scenes 1..3, and + `overall_score` then averages two of them with no record of which + produced what. + """ + import agent.services.video_reviewer as vr + + scenes = [{"id": f"s{i}", "vertical_video_url": f"https://example.test/{i}.mp4"} + for i in range(3)] + + async def fake_list_scenes(vid): + return scenes + + async def fake_characters(pid): + return [] + + resolved = [] + + def fake_resolve(role_name): + resolved.append(role_name) + return {"provider": "claude", "model": "sonnet", "effort": "high"} + + seen_roles = [] + + async def fake_scene_review(scene, characters, **kwargs): + seen_roles.append(kwargs.get("role")) + return vr.SceneReview( + scene_id=scene["id"], overall_score=8.0, verdict="good", + dimensions=vr.DimensionScores( + character_consistency=8.0, prompt_adherence=8.0, motion_quality=8.0, + visual_fidelity=8.0, temporal_coherence=8.0, composition=8.0), + errors=[], usable_segments=[], fix_guide="", frames_analyzed=9, fps_used=4.0) + + monkeypatch.setattr(vr, "list_scenes", fake_list_scenes) + monkeypatch.setattr(vr, "get_project_characters", fake_characters) + monkeypatch.setattr(vr, "resolve_role", fake_resolve) + monkeypatch.setattr(vr, "review_scene_video", fake_scene_review) + + review = await vr.review_video("v1", "p1") + + assert resolved == ["video_review"] # once, not once per scene + assert len(seen_roles) == 3 + assert all(r == {"provider": "claude", "model": "sonnet", "effort": "high"} + for r in seen_roles) + assert review.scenes_reviewed == 3 From 3e0e94aea610fac3a0b73fe668572f17870031ab Mon Sep 17 00:00:00 2001 From: Hoang Tuan Nguyen Date: Sun, 20 Sep 2026 16:39:01 +0700 Subject: [PATCH 4/4] docs(readme): record what the v1.3.0 verification actually covered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things the changelog was missing. First, why this went unnoticed for so long: the suite was red on any dev machine for the whole period, while CI stayed green because the workflow installs ffmpeg and a font and then asserts drawtext renders — the one environment that ran the tests was the one environment where they passed. Second, codex's happy path is no longer unproven. All three providers now have an end-to-end run against the real CLI: a synthetic clip with a planted defect, through extraction, contact sheets and a live vision call, on the default model and on an explicitly selected model + effort. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Ka5BxVaWiQeWNDJakpCJJP --- README.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index e5094ae23..a03106bb0 100644 --- a/README.md +++ b/README.md @@ -905,7 +905,17 @@ Dates are merge dates. Older releases are tagged; `git log` is the full record. ### v1.3.0 — 2026-09-20 — video review works again Video review had been failing on every path at once, which is why nothing about -it looked fixable from the symptoms. +it looked fixable from the symptoms. The unit suite was red on a normal dev +machine for the whole period — 13 of these tests fail on v1.2.0 — while CI +stayed green, because the workflow installs ffmpeg *and* a font and asserts +`drawtext` renders. The one environment that ran the suite was the one +environment where it worked. + +All three providers are verified end-to-end against the real CLIs: a synthetic +clip with a planted mid-clip defect, through frame extraction, contact sheets +and a live vision call, with no mocks. claude, agy and codex each find the +defect and score it, on their default model and on an explicitly selected +model + effort. | Date | Change | |---|---|