feat(cli): expose agent-native color grading#2798
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the hyperframes media-treatment CLI to support agent-friendly discovery of the new advanced color-grading surface (wheels/curves/secondaries), and adds a non-mutating --analyze path that measures selected local media and returns bounded primary-correction suggestions plus source metadata/warnings. It also updates the media-use skill guidance to route agents toward these new capabilities and analysis flow.
Changes:
- Add “Color Grading” capability discovery and detailed capability surfaces (wheels, curves, hue curves, secondaries, scopes) to
media-treatment. - Introduce
media-treatment --analyzeto measure a selected media element and return metadata, warnings, diagnosis, and a suggested primaryadjustpatch. - Update
media-useskill docs/examples and extend the shared signalstats analyzer + tests to support the richer analysis output.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/media-use/SKILL.md | Updates media-use’s “when to offer” table to point to media-treatment --analyze. |
| skills/media-use/scripts/lib/grade-analyzer.test.mjs | Adds deterministic parsing/summary tests and adjusts existing exposure tests. |
| skills/media-use/scripts/lib/grade-analyzer.mjs | Refactors analyzer to parse frames, summarize clipping risk, probe metadata, and return richer analysis. |
| skills/media-use/scripts/lib/grade-analyzer.d.mts | Adds type declarations for the analyzer’s new exported functions and result shape. |
| skills/media-use/references/meta.md | Clarifies split: grade recipe resolution stays in media-use; direct element analysis/authoring lives in CLI. |
| skills/media-use/references/media-treatments.md | Updates guidance to use media-treatment --analyze for measured evidence. |
| skills/media-use/references/grading.md | Updates grading workflow docs to reflect capability discovery and the new analysis command. |
| skills-manifest.json | Bumps media-use skill hash/file count. |
| packages/cli/src/commands/media-treatment.ts | Adds new capability family/details, --analyze, nested asset path resolution, and refactors command flow. |
| packages/cli/src/commands/media-treatment.test.ts | Extends CLI tests for new capability surfaces and source resolution behavior. |
| packages/cli/src/commands/media-treatment-analysis.ts | New CLI wrapper that locates ffmpeg/ffprobe and returns analysis + suggested patch. |
Comments suppressed due to low confidence (1)
packages/cli/src/commands/media-treatment.ts:507
resolveMediaTreatmentSource()can accept a src that becomes empty after trimming/stripping query+hash (e.g."#", whitespace, or"video.mp4#"), which resolves to the project directory and can pass the laterexistsSync()check. This yields confusing behavior (trying to analyze a directory). Guard against emptycleanSourcebefore rewriting/resolving.
const cleanSource = stripUrlSuffix(source.trim());
if (/^[a-z][a-z\d+.-]*:/i.test(cleanSource) || cleanSource.startsWith("//")) {
throw new Error(
"Media analysis requires a local project asset; freeze remote media with media-use first",
);
}
const projectRelative = cleanSource.startsWith("/")
? cleanSource.slice(1)
: rewriteAssetPath(compositionFile, cleanSource);
const mediaPath = resolve(projectDir, projectRelative);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
miguel-heygen
left a comment
There was a problem hiding this comment.
Findings
-
Blocker — use the canonical asset resolver instead of recreating a narrower one. packages/cli/src/commands/media-treatment.ts:484-511 adds its own URL cleaning, remote detection, project-boundary check, and one-shot resolve. The repository already owns this contract in @hyperframes/parsers/asset-resolution: cleanAssetUrl, isRemoteOrInlineUrl, resolveLocalAssetCandidates, and resolveExistingLocalAsset. The difference is observable: with a real file assets/My Clip.mp4 and source /assets/My%20Clip.mp4?v=1, resolveMediaTreatmentSource returns the literal My%20Clip.mp4 path and existsSync is false, while resolveLocalAssetCandidates returns the decoded My Clip.mp4 candidate and finds it. Lint, Studio, and publishProxyBake already use the canonical helper specifically for percent-encoded and root-absolute media. Rewrite the composition-relative URL once, then delegate candidate decoding, containment, and existence to the shared helper; add this encoded-filename case to the regression test. This is both a correctness bug and exactly the duplicated path logic the PR description says it avoids.
-
Important design follow-up — the package dependency points in the wrong direction. packages/cli/src/commands/media-treatment-analysis.ts:1-4 imports executable implementation from skills/media-use/scripts/lib/grade-analyzer.mjs and trusts a handwritten sibling .d.mts declaration. That lets the JS behavior and TypeScript contract drift while making a product package depend on an installable skill tree. Reuse is the right goal, but the canonical analyzer should live behind a typed package/module boundary, with the skill consuming or generated from that boundary rather than the CLI reaching into skills internals.
The conservative analyzer itself is sensible: argv-safe FFmpeg execution, bounded suggestions, explicit HDR/LOG warnings, and no creative controls inferred from measurements. Focused verification passed: 17 CLI tests, 10 analyzer tests, CLI typecheck, oxlint, oxfmt, and diff check. I also read the three existing unresolved documentation comments and the suppressed empty-source finding; they remain valid and are not repeated here.
Verdict: Request changes.
Reasoning: The analysis behavior is well scoped, but the new command forks an existing shared asset-resolution contract and already fails a common valid percent-encoded media path. Fix that blocker and keep the analyzer reuse behind a durable typed boundary.
— Magi
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed with the lens you asked for: duplication, prior art, and whether the CLI surface thin-wraps core or restates it. Read the full changed files, and grepped for existing helpers before accepting any new code as necessary.
Additive only. @miguel-heygen has the resolveMediaTreatmentSource blocker and the skills/** boundary; Copilot has the docs/placeholder findings and the empty-source edge. I'm not restating any of them. What follows is (a) independent confirmation of the blocker with the root cause it exposes, and (b) evidence on the boundary finding that I don't think has been stated yet.
Audited: media-treatment.ts, media-treatment-analysis.ts, grade-analyzer.mjs, grade-analyzer.d.mts, assetResolution.ts + utils/urlPath.ts (as prior art) — read end-to-end.
Trusting: the four skill docs (read for the routing claims only), and media-treatment.test.ts (read for coverage of the findings below).
Confirming the blocker — and it's one root cause behind two reported bugs
I reproduced @miguel-heygen's finding independently rather than take it on faith, and it holds. But the more useful framing is why it happened, because it also explains Copilot's separate empty-source report.
resolveMediaTreatmentSource (media-treatment.ts:501-520) hand-rolls four things that @hyperframes/parsers/asset-resolution already provides — it reimplements three of them and drops the fourth:
| Local code | Canonical helper | Result |
|---|---|---|
stripUrlSuffix(source.trim()) |
cleanAssetUrl (assetResolution.ts:16) |
equivalent |
/^[a-z][a-z\d+.-]*:/i || startsWith("//") |
isRemoteOrInlineUrl (:12) |
narrower |
isPathInside(mediaPath, projectDir) |
isWithinProjectRoot (:20) |
equivalent |
| (nothing) | decodeUrlPathVariants (via :35) |
missing |
The missing decode is @miguel-heygen's bug. Confirmed empirically — for /assets/My%20Clip.mp4?v=1:
PR resolves to : /proj/assets/My%20Clip.mp4 ← does not exist
canonical also tries: /proj/assets/My Clip.mp4 ← the real file
Worth noting the canonical helper is subtler than it looks, in ways a reimplementation has to rediscover: decodeUrlPathVariants (parsers/src/utils/urlPath.ts) unshifts the decoded form so it's tried first, and swallows malformed percent sequences on purpose so a literal % in a filename still resolves. Both behaviours have a comment explaining them.
And the narrower guard is Copilot's empty-source finding, same root cause — I checked the two predicates directly:
"#" → local rejects: false canonical rejects: true
"#frag" → local rejects: false canonical rejects: true
isRemoteOrInlineUrl includes # in its alternation; the local regex doesn't, which is exactly how a bare # gets through to resolve() and lands on the project directory. So Copilot's edge case isn't a separate hardening nit — fixing the duplication fixes it for free.
One more detail that I think settles the "was this reasonable?" question: packages/cli already imports this helper. packages/cli/src/utils/publishProxyBake.ts:36 imports from @hyperframes/parsers/asset-resolution. So this isn't a discoverability miss across an unfamiliar package boundary — the canonical helper is already a dependency of this very package, one directory over. Lint (lint/src/project.ts, lint/src/hevcPreviewLint.ts) and Studio (studio-server/src/routes/preview.ts, helpers/mediaCodecMap.ts) use it too, which means media-treatment analysis is now the one path in the repo that resolves composition asset URLs differently from every other path. That's the divergence worth fixing, more than either individual bug.
On the skills/** boundary — the convention this inverts is written down
@miguel-heygen flagged the inverted boundary; adding the evidence for why it's inverted rather than merely unusual, since I think that changes how it should be fixed.
The only other packages/ → skills/ imports in the repo are packages/core/src/storyboard/vendoredParity.test.ts:17-19, and that file exists specifically to enforce the opposite arrangement. Its own header states the rule and the reason:
Each standalone skill ships a plain-JS copy of this parser at
scripts/lib/storyboard.mjs— skills install vianpx skills add, where a script can't reach@hyperframes/core(and the core export is.tsthat node can't load). The copies MUST stay in lockstep with core. This test fails the moment they drift.
So the house pattern is: core owns the logic in TS, skills vendor a plain-JS copy, and a parity test fails on drift. media-treatment-analysis.ts:4 runs that backwards — the published package takes the skill's .mjs as its implementation. Which suggests the fix isn't "make the import nicer," it's to move analyzeMediaGrade into a package (parsers or core) and have media-use vendor it with a parity test, matching storyboard.mjs.
Two concrete consequences, both verified:
- It works only because of bundling, and nothing records that.
packages/cli/tsup.config.tshasbundle: true, so the.mjsis inlined intodist/— I'm explicitly not claiming a broken publish. Butpackage.jsonhasfiles: ["bin","dist"], which ships neitherskills/nor any reference to it, so the dependency is invisible to the manifest. It survives exactly as long as nobody adds that path toexternaland nobody moves the skill file — and the break would land at build time in some unrelated PR. - The
.d.mtshas no drift guard at all.grade-analyzer.d.mtsis 66 hand-written lines declaring the shape of a plain-JS module. I checked it against the runtime and it is accurate today (probeMedia's return matchesGradeMediaProbe;summarizeMediaTreatmentAnalysis's matchesMediaTreatmentAnalysis). But greppingpackages/,skills/,scripts/, and.github/turns up nothing that references or tests it. So ifgrade-analyzer.mjschanges its return shape, the CLI type-checks green against a declaration that now lies and fails at runtime instead — with a shipped package behind it.vendoredParity.test.tsis the cheap precedent for the guard: one test that runssummarizeMediaTreatmentAnalysison fixture frames and asserts the declared key set.
What's genuinely well done
- The "reuse the shared analyzer, don't write a second implementation" claim is true. I checked for a second
signalstatspath and there isn't one —grade-analyzer.mjs:217is the only implementation, andmedia-treatment-analysis.tsis a real 20-line wrapper that only resolves ffmpeg/ffprobe and reshapesadjustintosuggestedPatch. Credit where the body earned it. - The capability surface derives from core instead of restating it — every detail block binds
contract: capabilities.*offgetHfColorGradingCapabilities()(media-treatment.ts:191, 202, 217, 231), includinghue-curves, with no re-declared bounds. I want to be precise about this given the blocker above: the capability half of this PR has exactly the right SSOT discipline; it's the source-resolution half that reimplements. Same PR, opposite instincts. skills-manifest.jsonis genuinely in sync. Ran the exact CI gate (bun packages/cli/scripts/gen-skills-manifest.ts --check) →✓ skills-manifest.json is in sync (19 skills).- "Shared analyzer: 10 passing" is exact.
node --test grade-analyzer.test.mjs→ 10 pass, 0 fail. - The analysis stays measurement-only and never invents creative controls —
summarizeMediaTreatmentAnalysisemitsadjustplus diagnosis/warnings, and the HDR/unknown-transfer warnings are honest about what container metadata can't tell you (log: "unknown"rather than a guess). That restraint is the right call for an agent-facing surface.
Verdict: COMMENT
Reasoning: The resolveMediaTreatmentSource duplication is a real blocker — I reproduced it, and it's the shared root cause of both Copilot's empty-source edge and @miguel-heygen's %20 failure, so it's broader than either report alone. I'm leaving the gate to @miguel-heygen's REQUEST_CHANGES rather than stacking a second one on the same defect; this review is the independent confirmation plus the boundary evidence. Not approving, and approval isn't mine to give here regardless.
— hyperframes
3c0cce8 to
adc63c8
Compare
|
Addressed the review findings in
Validation: Core |
adc63c8 to
6595ef3
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at be791e39. Both blockers are genuinely fixed, and the boundary fix took the harder, correct route rather than tidying the import. I verified the fixes by running them, which turned out to matter more than usual here — see the CI note first, because it changes how much the green checkmarks on this PR are worth.
Additive only. @miguel-heygen is re-reviewing in parallel; I'm not restating its confirmations. Mine is (a) the CI gap and the gates I ran in its place, (b) the line-level check that the fix didn't introduce a regression, and (c) a demonstrated hole in the new parity guard.
Audited: media-treatment.ts, media-treatment-analysis.ts, mediaGradeAnalyzer.ts, grade-analyzer.mjs, mediaGradeAnalyzer.vendoredParity.test.ts, media-treatment.test.ts, assetResolution.ts + utils/urlPath.ts, the subpath wiring — read end-to-end.
First: this PR's CI does not run the tests that prove the fix
The CI workflow is gated pull_request: branches: [main] (ci.yml:22-23). This PR's base is feat/professional-color-grading-core, so CI never fires on it. Only three workflows ran at this head — regression, preview-regression, Player perf. That means no unit tests, no typecheck, no oxlint, no check:package-subpaths, no skills-manifest gate, and no skill-mirror check have executed against be791e39. Which is to say: the tests that pin both blocker fixes, the new parity test, and the gate for the new subpath export have all never run in CI.
ci.yml:15-20 already documents this exact hazard for stacked bases ("leaving required checks skipped for that head SHA forever"), so the mechanism is known — it just isn't covered for a PR whose base stays off main for its whole life. Not your bug, but it means the green checks here are much weaker evidence than they look, and it's worth knowing before this merges on them.
So I ran the skipped set locally at this head, in a clean worktree with workspace deps built:
| Gate CI skipped | Result |
|---|---|
packages/cli media-treatment.test.ts |
17/17 pass (includes both new blocker assertions) |
packages/core colorGrading + curves + new parity test |
43/43 pass |
packages/parsers colorGradingContract.test.ts |
6/6 pass |
oxlint . |
0 warnings, 0 errors |
tsc --noEmit core / cli / parsers |
clean |
check:package-subpaths |
✓ 12 public workspaces (new ./media-grade-analyzer valid) |
gen-skills-manifest --check |
✓ in sync (19 skills) |
check-skill-mirror |
✓ 24 files byte-identical |
check:workspace-contracts, check:package-cycles |
✓ — no new cycle from the cli → core subpath |
All green. I'm reporting that as verification, not as a substitute for CI actually running it.
The blocker fix is correct, including the two lines that looked like regressions
resolveMediaTreatmentSource now uses isRemoteOrInlineUrl + cleanAssetUrl + resolveExistingLocalAsset. Two changes in it read like regressions on first pass and are in fact both right — I checked each at source rather than assume:
- Dropping
cleanSource.slice(1)is correct, not a broken absolute path. I expectedresolve(projectDir, "/assets/x")to discard the project prefix, butresolveLocalAssetCandidatesstrips the leading slash itself (assetResolution.ts:38) before resolving. Passing the slash through is what the helper expects. - Dropping the explicit
isPathInsidedoesn't remove the traversal guard: containment is enforced inside the helper viaisWithinProjectRoot(:40), with a clamp fallback at:44-48.
One behavioural change worth naming, since nothing pins it: escapes now clamp into the project root instead of throwing, so "Selected media resolves outside the project" is unreachable and a ../../ source reports Media file not found instead. The security property is preserved and is arguably stronger (it can't escape at all), but it's now an untested property of a helper in another package rather than an assertion in this file. One case in media-treatment.test.ts asserting a ../../ source doesn't resolve outside projectDir would keep that honest.
The new tests are the right shape: they build a real temp dir with real files, which is exactly why they bite. The old test asserted path arithmetic against a nonexistent /tmp/hf-project, so it could never have caught an existence-dependent bug — that's the root reason this shipped. /assets/My%20Clip.mp4?v=1 → real file and "#" → throws are both pinned now (:227-252).
The boundary fix took the right route
analyzeMediaGrade moved into packages/core/src/mediaGradeAnalyzer.ts behind a registered ./media-grade-analyzer subpath, the CLI imports the package instead of reaching four directories up into skills/**, the skill vendors the .mjs with a header pointing at its parity test, and the hand-written .d.mts is deleted. That's the vendoredParity.test.ts convention followed properly rather than worked around, and it removes the two costs I flagged: the dependency is now visible to the manifest, and the declaration can no longer drift because there isn't one.
I also checked the .mjs refactor (extracted suggestedExposure / suggestedTemperature / suggestedTint) is behaviour-preserving expression by expression against the old inline chain — it is, including argument order.
The one real gap: the new parity guard can't see the exposure path — important, not blocking
The parity test is a genuine drift guard and a large improvement, but it is single-fixture output-equivalence, and its fixture cannot discriminate the analyzer's primary job. Demonstrated rather than argued — I changed core only (normalizedAverage < 0.28 → < 0.99) and:
repo's vendoredParity.test.ts → PASSES (green)
same two copies, mid-dark input → core: -0.134 vendored: 0 DIVERGED
(YAVG=110 → 0.431, YHIGH=130 → 0.51)
So a real divergence between the two copies survives the guard. The cause is the fixture, not the threshold: its averaged yHigh is 230 (0.902), which fails the yHigh / 255 < 0.65 sub-guard, so neither exposure branch is reachable for it at any threshold. Under/over-exposure correction is the main thing this analyzer exists to compute, and it's the one path the guard doesn't compare.
To be fair to the test, I checked the other branches with the same method: mutating the tint threshold (>= 10 → >= 0) does fail it, so tint and the summarize/parse paths are genuinely covered. The fix is two more entries in the same toEqual: an under-exposed fixture (low YAVG and low YHIGH) and a blown-highlight one (YAVG > 184, YLOW/255 > 0.3). Same fixture style, no new machinery.
Related and worth stating so the guard's scope is understood: analyzeMediaGrade — the function the CLI actually calls — isn't parity-covered, since it shells out to ffmpeg. Reasonable, but it means the ffmpeg argument construction can drift between the copies silently.
Verdict
Verdict: COMMENT
Reasoning: Both blockers are fixed for the right reasons, and I verified them by running the full gate set this PR's CI skips. The parity-guard hole is a real gap in the fix's own safety net, demonstrated with a reproduction, but it's a test-coverage gap rather than a defect in shipped behaviour, so it shouldn't gate the merge. Lifting the gate is @miguel-heygen's call, and a stamp here is James's, not mine.
— hyperframes
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-review at exact head be791e39f4072a666c319b7e3addfec4caf74725.
Both findings from my prior change request are resolved:
resolveMediaTreatmentSourcenow delegates URL cleaning, remote/inline rejection, percent-decoded candidate resolution, containment, and existence to the canonical@hyperframes/parsers/asset-resolutioncontract. Real-file tests pin both/assets/My%20Clip.mp4?v=1and the bare-#rejection.- The analyzer now lives behind the typed
@hyperframes/core/media-grade-analyzerpackage subpath. The skill vendors the standalone copy under the repository's established direction of ownership, the handwritten declaration is gone, and a parity test guards the copies.
One non-blocking coverage gap remains: the parity fixture's YHIGH prevents either exposure branch from executing, so an exposure-threshold drift can still pass. Please add under-exposed and blown-highlight fixtures so the analyzer's primary correction path is compared as well.
CI note: because this stacked PR targets feat/professional-color-grading-core, the main CI workflow does not run its package tests/typechecks. I therefore ran the relevant skipped gates locally at this exact head: 18/18 CLI/parity tests, CLI/core typechecks, package-subpath validation, skills-manifest validation, and diff check all pass. The exact-head regression, preview, and perf workflows are green.
Verdict: APPROVE
Reasoning: The asset-resolution correctness bug and inverted package dependency are fixed using the repository's canonical contracts. The remaining parity-fixture weakness is test hardening rather than a shipped-behavior defect, and all applicable verification is green.
— Magi
be791e3 to
20f4bde
Compare
|
Addressed the remaining review requests in |
miguel-heygen
left a comment
There was a problem hiding this comment.
Restamp at exact head 20f4bde46a8862584470f895c8a6a4bf69716b2a.
The restack includes approved #2796's shared mask/limits contract plus one PR-local hardening commit. The previously non-blocking parity gap is now closed with underexposed and blown-highlight fixtures that execute both exposure branches and compare the typed product analyzer to the vendored skill copy. The source-resolution test also pins traversal containment against a real same-named file outside the project root.
Fresh verification: Core color-grading/analyzer tests 39/39, CLI media-treatment tests 17/17, parser contract tests 6/6, Core/CLI/parser typechecks pass, diff check is clean, and all 25 exact-head check runs are terminal green.
Verdict: APPROVE
Reasoning: The stale-head delta consists of approved downstack contract consolidation and targeted regressions that close the only remaining coverage gaps; no new blocker was introduced.
— Magi
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-stamp after the base update at exact head 20f4bde46a8862584470f895c8a6a4bf69716b2a.
The PR head/tree is byte-identical to my previously approved exact head; the effective diff is now the 14-file #2798 surface on top of merged #2796. I re-audited the current GitHub diff and confirmed the prior fixes remain intact:
packages/cli/src/commands/media-treatment.tsstill delegates media URL cleaning, remote rejection, percent-decoded candidate resolution, containment, and existence to the canonical parser asset-resolution contract.packages/core/src/mediaGradeAnalyzer.tsremains the typed owner, with the skill copy guarded bymediaGradeAnalyzer.vendoredParity.test.ts.- The parity fixtures still execute both underexposed and blown-highlight correction branches, and the CLI regression still proves a real outside-project same-named file cannot escape containment.
All review threads are resolved. Fresh exact-head verification shows 63 check runs terminal with 62 successes and one intentional skip, including all eight regression shards, Windows render/tests, CLI smoke/global install, skills gates, lint, format, typecheck, build, and unit/contract suites.
Verdict: APPROVE
Reasoning: The base update introduced no content delta at the reviewed head, the current effective diff preserves every previously verified fix and regression, and all exact-head required and full CI checks are green.
— Magi
What
media-treatmentCLI with concise advanced-grading discovery and focused legal-range details.Why
Agents need a short deterministic path from vague feedback to a legal grading patch without reading raw HTML attributes or inventing color math.
How
media-treatment --capabilities --jsonas the small first hop and expose detailed wheels, curves, secondary, scopes, and order contracts through--capability.@hyperframes/core/media-grade-analyzer; the standalone skill keeps a parity-tested plain-JS vendor copy.Test plan
%20, remote, empty-fragment, and missing-media resolver casesStack: 2/3. Base:
feat/professional-color-grading-core. Merge after #2796. Next: #2797.