fix(review): repair every provider path, add per-role AI config

[crisng95]

Video review was failing on four independent paths at once, which is why none of them looked fixable from the symptoms. Every finding below was verified live against the installed binaries, not inferred from the code.

The clearest evidence of how long this has been broken: on main, 13 of this repo's own tests fail on a normal dev machine. CI stayed green because .github/workflows/tests.yml installs ffmpeg and fonts-dejavu-core and hard-asserts that drawtext renders — so the one environment that ran the suite was the one environment where it worked.

main:   13 failed, 259 passed
this:    0 failed, 338 passed

The four failures

1. ffmpeg without drawtext. Homebrew's ffmpeg 8.x is built without libfreetype, so the filter does not exist, and naming an absent filter aborts the whole chain (No such filter: 'drawtext'). Frame extraction died before any provider was reached — this broke review for all three CLIs, not just the two named in the report. Now 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. Verified live: both claude and agy computed correct time ranges from untimestamped sheets.

2. agy was auto-denied. Given a bare file path, agy reaches for a shell command 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" — which the old code read straight into the JSON parser.

Steering agy at its own file-reading tool gets the read done unprivileged, so --dangerously-skip-permissions is gone. That flag auto-approves every tool including arbitrary shell commands, for a job whose entire need is reading three JPEGs.

A denied tool is now an error whether or not agy still answered. The denied-and-answered case is the more dangerous one: the prompt carries the full scoring rubric, the scene's image prompt, its video prompt and the character names — enough to write a complete, plausible review from the text without ever looking at a frame.

3. 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, so nothing hangs. An empty output file is an error instead of a JSON decode failure three frames from the cause.

4. stdin closed for all three. Each appends piped stdin to the prompt when stdin is not a terminal — codex documents it as a <stdin> block. Under uvicorn that is whatever the launching shell handed down.

Two silent-wrong-answer paths in the scoring

Both pre-existing, both the same class as #2 — a failure wearing the shape of a result.

• A review with no scores became a score. Every dimension defaults to 5.0, so an answer carrying no dimensions produced 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 — a model that scored some axes is answering, just incompletely.

• A malformed error entry vanished. The 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 instead of time_range was enough to turn 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.

New: per-role provider, model and effort

agent/services/cli_providers.py holds everything the three CLIs disagree about. providers.json gains a roles map; the dashboard gains a Settings page (agent / model / effort per role, in all seven languages).

• Efforts are validated against each CLI's real ladder — agy stops at high, claude and codex go to max.
• Models are validated only for agy, whose catalog is closed (agy models is the whole truth and it rejects anything else). claude takes aliases and full names, codex takes slugs newer than its on-disk cache, so for those an unlisted value is a documented escape hatch. A model starting with - is refused regardless.
• agy's model and effort are mutually exclusive. Its slugs name their own effort, verified live: a mismatch gives --model gpt-oss-120b-medium conflicts with --effort=low, and a slug with no effort in its name gives --effort is not supported for model "claude-sonnet-4-6". The API answers 400 for the pair and the UI cannot construct it.
• The legacy {"active": ...} switch still works, keeps a role's model when the CLI did not change, and does not undo a role the same request configured by name.

/fk-doctor gains a video-review section — the skill CLAUDE.md points at for pipeline errors had nothing about review at all.

Verification

Live end-to-end through the full review_scene_video path (synthetic 8s clip with a deliberate mid-clip anomaly, real CLI calls, no mocks):

claude: verdict=poor overall=5.55 critical=True  -> caught the anomaly at 2.0s-3.0s
agy:    verdict=poor overall=5.75 critical=True  -> caught the anomaly at 2s-3s

codex could not be verified live: its OpenAI workspace is out of credits. The invocation is correct — it parses, attaches the images and reaches the model before failing — and the error path surfaces the real reason, but no live answer was obtained. Reviewer should treat codex's happy path as unproven.

Every new guard was checked for being load-bearing by removing it and confirming exactly the intended tests fail: the dimensions guard, the severity check, the key normalisation, the active+roles clobber and the same-provider model wipe.

Both documented PATCH examples were wrong (they paired an agy model with an effort, which is a 400). Every -d '{...}' in the README and the skill now goes through the real handler in a scripted check; all provider examples return 200.

Frontend: tsc --noEmit clean, eslint clean on changed files, npm run build clean. Three pre-existing react-refresh/only-export-components errors remain in untouched vendored shadcn files.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Ka5BxVaWiQeWNDJakpCJJP

[crisng95]

Adversarial review round applied — 13 findings

A code-review pass found 13 issues. All actionable ones are now closed; the PR body above describes the original change, this comment covers what the review changed.

Landed before the review reported (same conclusion, independently): the agy denied-tool guard now raises on any denial, not only a denial with an empty response. The reviewer's argument for it is worth recording: a hallucinated review produced after a denied file read carries a populated dimensions object, so the "no dimensions" guard never fires. The denial flag in the envelope is the only evidence the answer was fabricated.

HIGH — fixed, each with a regression test verified to fail without the fix

PATCH {"active": …, "roles": {…}} discarded the roles it had just validated	The active sweep overwrote every role including one the same request named. Advertised as "or both" in the handler docstring, returned 200, model silently gone
Re-asserting the same provider wiped the role's model	/fk-change-provider set claude on a config already on claude destroyed the user's model. The justification ("a slug means nothing to a different CLI") does not apply when the CLI did not change
Both documented PATCH examples returned 400	They paired an agy model with an effort. Every -d '{...}' in the README and the skill is now run through the real handler in a scripted check
A malformed error entry vanished	Reviewer's design beat mine and was taken as specified — see below

On the malformed-error fix: I had planned a blanket raise on any entry that did not match the three keys. The reviewer pointed out this fails reviews that correctly identified a CRITICAL defect over a stray timeRange, which is backwards. Final design handles the three fields by what they can cost: near-miss names normalised, a missing time range or description repaired and logged, and only a severity outside {CRITICAL, HIGH, MINOR} fails the scene — the one field with no safe default, because without it we do not know whether the video passed.

MEDIUM — fixed

• One review, one provider. resolve_role ran per scene, and every scene yields at a download, an executor hop and a subprocess. A hand edit (documented as supported) or a dashboard GET could retarget scenes 4..N, and overall_score then averaged two backends with no record of which produced what.
• catalog_is_authoritative was published and enforced nowhere. A typo'd agy slug got a 200 and failed seconds into the next review with a raw CLI error. Now a 400 naming the known slugs. An empty catalog still does not block — emptiness means the listing failed.
• CLI failures carry stdout too. claude puts the readable sentence there and buries the machine tag under context advice on stderr.
• providers.json written atomically. Truncating in place was survivable on a rare provider switch; it is written on every settings change now, and a truncated file hard-fails config.py at import — the server will not boot.

PLAUSIBLE, fixed anyway — the drawtext probe tested presence, not usability. An ffmpeg with libfreetype but no resolvable font lists the filter and then dies at runtime, which is the original symptom on a box where the probe says everything is fine. Extraction retries untimestamped, and _create_contact_sheets returns what actually happened instead of letting the caller re-derive it.

LOW — fixed: resolve_role now applies the same model guard as the API (the file is documented as hand-editable, so the paths must agree); an active sweep no longer adopts a role name this build does not know while the roles path 400s on it; list_models returns a copy; the comments claiming scripts/statusline.sh reads active are gone — it does not, the sole reader is the skill.

Verdicts on the areas I asked about — argument injection clean (effort is a closed tuple at both entry points, so the codex TOML value can never carry a quote or a second key; the --prefix model guard is belt-and-braces); add_dirs clean; no torn read possible (no await between clear() and update(), all readers on the loop — the invariant is now a comment so a future refactor does not break it); legacy {"active": "claude"} clean across every path; _read()'s hot-reload side effect defensible and kept.

Test gaps 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 an empty catalog.

338 → 352 tests. CI green on 3.10 and 3.13. Re-verified live end-to-end after every change — both providers still catch the planted defect and cap the score.

Still unproven: codex's happy path. Its OpenAI workspace is out of credits, so no live answer was ever obtained from it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Ka5BxVaWiQeWNDJakpCJJP

[crisng95]

codex verified — all three providers now proven end-to-end

The workspace has credits again, so the one gap in this PR is closed. Same harness as claude and agy: a synthetic 8s clip with a planted green square that flashes at 2–3s, through real frame extraction, real contact sheets and a live CLI vision call, no mocks anywhere.

provider	model	verdict	found the planted defect
claude	default	poor · 5.12	CRITICAL 2.0s–3.0s
agy	default	poor · 5.75	CRITICAL 2s–3s
agy	gemini-3.1-pro-high	poor · 5.9	CRITICAL 2s–3s
codex	default (gpt-5.6-sol)	excellent · 9.05	MINOR 2.0s–3.0s
codex	gpt-5.6-terra + --effort high	good · 8.7	HIGH 2s–3s

Every path works, including the per-role model and effort actually reaching the CLI — the two codex rows used different models and produced different grades, which is what proves the setting is wired through rather than ignored.

One thing worth flagging rather than burying: codex graded the same planted defect far more leniently than the other two — MINOR on its default model against CRITICAL from both claude and agy, which is the difference between "excellent" and "poor" on the same clip. That is a model judgment difference, not a defect in this PR: all three saw it and reported it, and the severity caps behaved correctly for the severity each assigned. But anyone switching video_review to codex should expect a softer grader and re-baseline their score thresholds, not assume the numbers are comparable across providers.

Changelog updated with what the verification covered, plus the reason this went unnoticed 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 asserts drawtext renders.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Ka5BxVaWiQeWNDJakpCJJP