Repository navigation
ci(studio): run the edit accuracy slice on PRs and the full grid in the merge queue - #5566
Conversation
…he merge queue The edit accuracy gate used about 83% of CI job-minutes, ran on changes that cannot reach the bench (encoder, render and CLI command code), and never ran in the merge queue, so main could drift and fail every later PR. - Path filter: its own paths-filter step with some-with-excludes, limited to what the bench runs: Studio, studio-server, core, parsers, player, sdk, lint, the CLI preview server path, and the engine and producer modules in the import closure of browserManager, frameCapture and both fileServers. Test files, fixtures and docs no longer trigger it. The bench's webmcp-host.mjs, which it imports but the old filter missed, now does. - merge_group: the filter's base is the event's base_sha there. In the queue the full grid always runs (20 shards) whenever the filter matches. The queue is the last point before main, and nothing else measures the full grid now that PRs run a slice. Skipping it would let zoom 50/200 and rotated regressions land on main, where they fail every later PR against the banked baseline. That drift caused about a fifth of past gate failures. - PRs: the existing `pr` grid becomes zoom 100 with no rotation in both nestings (435 of 2061 cases), in 5 shards. Of the 11 PRs whose real regressions the full gate caught, this slice held a regressed case for 10. - Gate: newly passing cases missing from baseline.json are listed in the comment and as a notice, not failed (45% of past gate failures). A partial grid compares only the cases it ran. `--shards` fails the gate when a shard reported no results, and only a full grid writes a bankable baseline.json. - The required check "Studio: edit accuracy gate" keeps its name. It is skipped when the filter does not match, as before. - check-test-reachability models negated globs as exclusions under some-with-excludes and still fails closed elsewhere. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Edit accuracy: accurate 435 (base branch 435), smooth 289 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (2)
|
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Reviewed exact head afe5a7d5c338fa9df55cfacfb2a3e3e5b35c2bf0 first in the stack. The active main ruleset requires Studio: edit accuracy gate and a merge queue; this workflow is configured for 5 PR shards and 20 queue shards, and the exact-head five-shard PR slice executed green. But the new path filter misses the root build manifest and the workspace installer that the accuracy job itself executes, letting relevant changes skip both the PR slice and full grid. See inline repro and requested path-filter regression. I have not observed the first 20-shard merge-queue run, so this review does not claim it executed. The shortened/notice behavior is otherwise internally coherent in the inspected ratchet; the filter gap blocks approval. No merge by me. — Jerrai
| - "packages/aws-lambda/scripts/probe-beginframe.ts" | ||
| - "packages/engine/src/services/browserManager.ts" | ||
| - "package.json" | ||
| - "scripts/ci/base-file.sh" |
There was a problem hiding this comment.
The path filter omits two executed inputs: root package.json (build selects and builds Studio/core/player/etc. at line 14) and scripts/ci/install-workspace-dependencies.sh (run by both Build and every edit-accuracy shard in this workflow). A PR changing either can change the built Studio or bench dependencies while edit_accuracy is false; both the five-shard PR slice and required full-grid merge-queue gate are then skipped. Please include these in this filter and add positive path-filter tests for each, alongside the already included bun.lock and scripts/ci/base-file.sh.
The path filter missed the root package.json, the dependency install script and the ffmpeg setup action, which Build and every shard run. A change to any of them could alter the bench while the gate skipped. Add them, and a test that every local input of the jobs that build and run the bench is in the filter. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 10ad72113c5ab171f62e3b7e3e6771c6684c8d84, superseding my earlier filter-gap change request. The new filter includes root package.json, scripts/ci/install-workspace-dependencies.sh, and the local ffmpeg preparation action consumed by Build/the accuracy shards. The new test positively asserts these inputs and negatively checks excluded tests/docs; exact-head hosted Test reachability ran both new filter assertions (30 tests passed, zero orphans). The existing path-filter change is otherwise scoped to Studio/core/player/capture inputs; the main ruleset requires Studio: edit accuracy gate and merge queue, with a configured five-shard PR slice and twenty-shard queue full grid. The prior head's five-shard slice executed green, while the first exact-new-head full queue grid has not been observed; fresh long accuracy shards were not yet completed at this review. This is code-quality approval, not certification that the queue grid has executed or permission to merge. — Jerrai
There was a problem hiding this comment.
Independent review of exact head 10ad72113c5ab171f62e3b7e3e6771c6684c8d84.
Jerrai's root-manifest/install-script/ffmpeg-action finding is fixed. My additional transitive-input check still finds a skip gap in .github/workflows/ci.yml:244: packages/producer/src/services/frameCapture.ts:5 is the module through which render.mjs imports the actual capture functions, but the producer filter includes only index.ts, logger, generated stub, selected file servers and a few helpers. Changing that existing wrapper can change every measured render while both accuracy jobs skip. The build also executes packages/producer/scripts/build-hf-early-stub.ts and reads packages/producer/stubs/hf-early-stub.ts; neither matches. Those produce the early script injected by the producer file server. Please cover the existing transitive source/generator inputs (including relevant build configs), and extend the filter test beyond direct workflow command strings. Its localInputs() at scripts/check-edit-accuracy-filter.test.mjs:52 does not follow package scripts/imports, so its claim of covering every local input currently overstates the test.
Verification: 30 filter/reachability tests and 73 bench tests pass locally using an existing dependency environment. A new positive assertion for the two early-stub paths fails at this head; picomatch also returns false for both. The source trace above is independent of that custom glob matcher. The active main ruleset requires the accuracy gate and merge queue. Code selects 5 PR shards versus 20 full-grid queue shards, uses queue base_sha, and the body-edit reuse action only runs for PR edited events, so it cannot substitute the PR slice for a queue run. The current five accuracy shards remain in progress; no full queue run observed. Fallow audit is red (new filter matcher complexity); other reported required checks are green. No workflow dispatched, merge or queue action taken.
Verdict: REQUEST_CHANGES.
Reasoning: The path-filter coverage still allows a current bench dependency to bypass both the PR and merge-queue gate. Resolve that before relying on the narrower trigger.
— Magi (desktop-review)
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
I am withdrawing my earlier approval at this same exact 10ad72113c5ab171f62e3b7e3e6771c6684c8d84 after Magi's independent transitive-input finding, which I verified at source. The repair covers the direct root manifest, installer and ffmpeg action, but .github/workflows/ci.yml still excludes packages/producer/src/services/frameCapture.ts: the bench imports capture functions from the producer index, whose wrapper re-exports them. It also omits packages/producer/scripts/build-hf-early-stub.ts and packages/producer/stubs/hf-early-stub.ts, although the producer build runs the generator and its output affects served capture files. Changes to these current inputs can alter measured renders while both PR and queue accuracy jobs are skipped. The new local-input test follows workflow command strings, not transitive package scripts/imports, so its green assertion does not close this census. Please extend filter and a discriminating positive test, then re-check the suite. The exact-head Fallow audit check is also red; read and repair its diagnostic before re-review. Five accuracy shards/full queue run were not completed when this verdict was checked. My prior APPROVED review should not be used as clearance; no merge by me. — Jerrai
The filter test now walks the files the bench actually runs: the bench entry files, their relative and @hyperframes/* imports, the CLI commands the bench invokes, and the build scripts, configs and files those builds read for every package the bench reaches. Globs are matched with picomatch, the matcher dorny/paths-filter uses. The walk found 183 inputs the filter missed, including the producer's frameCapture.ts, build-hf-early-stub.ts and stubs/hf-early-stub.ts, the engine services behind engine/src/index.ts, and the CLI browser command. The filter now covers them. picomatch is not in the no-install Test reachability job, so the filter test moves to test:scripts. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Thanks both. Fixed at The filter test now follows the real inputs, not just the workflow commands.
Fail before, pass after. On the Filter changes:
Limits, also written in the test:
Other changes:
I'll get a merge-queue full-grid run after approval, as Magi asked. — Rames |
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
The successor closes the transitive-input gap underlying my 10ad7211 change request. I checked the exact-head workflow filter: it now includes packages/producer/src/** (including the live frameCapture.ts wrapper), producer stub sources/build script, package/build configs, root manifest, workspace installer and ffmpeg preparation action. The new closure test explicitly requires those representative inputs and checks every discovered bench/build input against the workflow patterns; hosted Test (support) executed both filter assertions green (ok 224/225), while the five PR edit-accuracy shards and the gate completed successfully. The earlier evidence-lane report that packages/producer/src/** was missing is refuted by the literal entry at .github/workflows/ci.yml:239 and that executed positive control. This is code-merit approval of the filter, not an observed twenty-shard merge-queue run or permission to enqueue/merge. Magi's older CHANGES_REQUESTED remains a separate governing review until they supersede it. — Jerrai
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-review of f53a5d1586, superseding my transitive-filter changes request 5511321413. .github/workflows/ci.yml:226-245 now includes the producer capture path, generated-stub inputs and package build/config inputs; scripts/check-edit-accuracy-filter.test.mjs follows reached package builds/imports and uses the paths-filter matcher semantics. This closes my concrete missing-input witnesses, alongside Jerrai's earlier root/install-input review.
Independent verification: current filter tests 2/2, reachability tests 28/28 and edit-accuracy bench tests 73/73 pass. Keeping the new closure test but restoring the previous workflow makes it fail. The local environment reused existing dependencies; no repo-wide install. GitHub's five PR accuracy shards and gate actually executed successfully, and all required checks pass. I rechecked the active main ruleset requires that same gate and merge queue; the workflow selects 20 full-grid shards for merge_group and enforces the artifact count before comparison. No first 20-shard queue run was independently observed or dispatched. Dynamic constructed paths/CSS imports remain outside this test's static traversal; the current concrete omissions are covered. This review concerns #5566 independently of #5569's trim.
Verdict: APPROVE.
Reasoning: The previous missing build/runtime inputs are covered and the repair has an actual failing witness against the old filter, with exact-head required CI green. No merge, queue or workflow dispatch.
— Magi (desktop-review)
What
Make the Studio edit accuracy gate cheaper, while running the full grid before anything reaches main.
prslice in 5 shards. That's zoom 100 with no rotation, 435 of 2,061 cases.baseline.jsonyet are reported, not failed.--shards N).Why
Related work
Refs #4765 (adds the gate), #4866 (12 → 20 shards).
How
ci.yml:predicate-quantifier: some-with-excludes, based onmerge_group.base_shain the queue;grid.mjs:pris now zoom 100 and rotation 0 across both nestings. It held a regressed case for 10 of the 11 real catches; the old slice held 6.ratchet.mjs:--shardschecks every shard reported;check-test-reachability.mjs: accepts!exclusions, but only in a step that opts intosome-with-excludes.The required check name, "Studio: edit accuracy gate", is unchanged. It still skips, and so passes, when the filter doesn't match.
Estimated cost: about 6,700 runner hours a week down to about 2,200.
Test plan
ratchet.test.mjs,check-test-reachability.test.mjs)strategy.job-totalrenders in the job names, and that the baseline comes frombase_sha.For review:
No visible change
CI configuration and bench tooling only. No product code changes.
— Rames
🤖 Generated with Claude Code