Improve automated Skia update analysis and validation - #4573
Merged
Conversation
Provision and pin Mesa lavapipe, verify it before the agent starts, and require passing named Ganesh and Graphite Vulkan results before sync branches can be pushed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f66474bf-0d92-48db-a924-6ca2b009d6b2
Contributor
📦 Try the packages from this PRWarning Do not run these scripts without first reviewing the code in this PR. Step 1 — Download the packages bash / macOS / Linux: curl -fsSL https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.sh | bash -s -- 4573PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4573"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-4573/packages --name skiasharp-pr-4573More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-4573 |
Keep deterministic lavapipe installation and verification while relying on the test suite itself to enforce Vulkan coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f66474bf-0d92-48db-a924-6ca2b009d6b2
mattleibow
changed the title
Contributor
📊 SkiaSharp benchmarks — PR #4573⭐ this PR (full source build) vs 🌙 nightly · Linux · Windows · macOS
Highlights⏱️ Time — 🔴 34 slower · 🟢 31 faster
📦 Allocations — 🔴 1 more alloc · 🟢 0 less alloc
Full per-OS benchmark deltasLinux⏱️ Time (vs 🌙 nightly
📦 Allocations (vs 🌙 nightly
Windows⏱️ Time (vs 🌙 nightly
macOS⏱️ Time (vs 🌙 nightly
|
Contributor
|
📖 Documentation Preview The documentation for this PR has been deployed and is available at: 🔗 View Staging Site This preview will be updated automatically when you push new commits to this PR. This comment is automatically updated by the documentation staging workflow. |
Every GPU code path in the test suite was written as "try to bring up the backend; if anything throws, Assert.Skip". That made a genuine regression -- a broken P/Invoke, a GRContext factory that starts returning null, a CI provisioning step that silently stopped working -- indistinguishable from "this agent has no GPU", so CI stayed green while coverage evaporated. The committed goldens are the receipt: ganesh-gl had a golden for macOS only (Windows and Linux had been skipping), and graphite-dawn had none at all. Replace the catch-and-skip with a declared four-state model in GpuPolicy. Only one state can fail, and only one is configured: Required built and expected here -> any failure is a test failure Disabled opted out for this agent -> SKIASHARP_TEST_SKIP_GPU Unsupported API does not exist here -> compiled in, never configured NotBuilt API exists, we don't ship -> compiled in, never configured The matrix describes platforms; the environment variable describes agents. Nothing about a platform's inherent capabilities is expressed as configuration: Metal on Windows and Vulkan on macOS need no setup to skip. Each backend declares ExistsOn and BuiltOn, with BuiltOn mirroring the gn args in native/*/build.cake, so enabling macOS Vulkan later is a one-token change. IRenderer.IsAvailable/UnavailableReason become a single Backend property and RendererUnavailableException is deleted -- it existed only to launder failures into skips. SKTest.CreateGlContext, VKTest, Direct3DTest and SKGraphiteReleaseTestsBase now gate on the policy and let exceptions escape. Failure messages carry GpuPolicy.OptOutHint so a red test names the exact directive that would legitimise a skip. The hidden TF_BUILD + x64 Metal sniff in the renderers is replaced by a visible uname -m check in the macOS, iOS and Mac Catalyst CI legs. It is the only pre-set opt-out, and it exists to avoid a two-hour shutdown hang on the virtualized x64 Metal driver, not to hide a failure. Device and browser hosts cannot read the agent environment, so the same value reaches them as -p:SkiaSharpTestSkipGpu -> runtimeconfig -> AppContext. GpuPolicyTests emits a ##SKIA-GPU-POLICY## line per backend into every TRX, so each leg records which backends it required and why the rest were skipped. That report is what makes a skip auditable. Expect several legs to go red on the first run -- that is the point. Each failure is coverage that was already missing and is now visible, and each comes with the opt-out directive if the gap turns out to be legitimate. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 03c0625a-14f0-4a6c-a1f6-b02ac5f8701a
Correct Phase 10 to run the full test solution instead of only the base console project. Run the Vulkan satellite explicitly with TRX evidence and document the m152 Ganesh allocator requirement. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f66474bf-0d92-48db-a924-6ca2b009d6b2
mattleibow
changed the title
iOSExtensionsTests.GRContextDisposeDoesNotCrash still skipped when MTLDevice.SystemDefault was null -- the exact pattern this change removes, just in the iOS device host rather than the shared harness. Metal is Required on every Apple platform, so a missing device there is a red test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 03c0625a-14f0-4a6c-a1f6-b02ac5f8701a
Keep the Vulkan execution and evidence requirements without documenting the known m152 allocator diagnosis or fix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f66474bf-0d92-48db-a924-6ca2b009d6b2
Remove individual test project commands and the duplicate Vulkan invocation. Collapse solution dependencies to net10.0 and require the solution output to show executed Vulkan tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f66474bf-0d92-48db-a924-6ca2b009d6b2
Nano Server is now its own platform in the matrix. It runs a different native build -- native/nanoserver/build.cake passes supportVulkan=false and supportDirect3D=false -- so Vulkan and Direct3D are absent from libSkiaSharp.dll there even though the OS is Windows. The matrix previously inherited the plain Windows row and would have marked both Required. Not reachable today (the Nano leg runs only the Console suite, which has no Vulkan/D3D tests), but the matrix is the load-bearing artifact of this change and has to be factually right. OpenGL stays Required there: it IS built for Nano, so a container with no driver is an agent problem, which is the tier-3 case. Normalise the opt-out list to commas before forwarding it. GpuPolicy documents comma, semicolon or whitespace separators, but neither cake forwarding path survived the last two: a space splits the /p: into two process arguments, and a semicolon is MSBuild's own property-list separator. Only commas reached the test host, so anyone following the documented contract by hand would have got a broken build. Drop the Trim metadata from the RuntimeHostConfigurationOption. ILLink collects every option marked Trim=true as a feature switch and parses its value as a bool; this one holds a backend-id list. Dormant today (no test project enables trimming) but a latent landmine, and Trim metadata does not affect runtimeconfig.json generation, so it bought nothing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 03c0625a-14f0-4a6c-a1f6-b02ac5f8701a
The macOS test leg failed before running a single test:
MSBUILD : error MSB1006: Property is not valid.
Switch: graphite-metal
MSBuild's -p: switch splits on commas as well as semicolons, so the
/p:SkiaSharpTestSkipGpu=ganesh-metal,graphite-metal that RunDotNetTest built by
hand parsed as two properties. Confirmed directly: `-p:Foo=a,b` gives MSB1006
while `-p:Foo=a` evaluates fine.
RunDotNetTest never needed the property. Its test process is a child of the cake
process, so it already inherits SKIASHARP_TEST_SKIP_GPU and GpuPolicy reads it
straight from the environment; and with NoBuild=true no runtimeconfig.json is
regenerated, so the property was a no-op even when it parsed. Only the device and
browser hosts need it, and that path goes through Cake's DotNetMSBuildSettings,
which quotes the value properly -- proven by the iOS and Mac Catalyst legs, which
both applied the two-backend opt-out correctly.
Also drop the generic Vulkan sentence from the SharpVk not-built reason. On Linux
it read "Vulkan is not built for this host: the Apple builds have no MoltenVK..."
when Vulkan is in fact built there and only the SharpVk vehicle is Windows-only.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 03c0625a-14f0-4a6c-a1f6-b02ac5f8701a
Contributor
📦 Artifact size reportPackages from this PR (build Total Packages
+38 package(s) unchanged (< 50.0 KB). Per-file changes
|
| File | Size |
|---|---|
runtimes/win-x86/native/libHarfBuzzSharp.pdb |
21.9 MB → 22.2 MB (🔴 +272.0 KB) |
runtimes/win-arm64/native/libHarfBuzzSharp.pdb |
22.3 MB → 22.5 MB (🔴 +184.0 KB) |
SkiaSharp.NativeAssets.WinUI
| File | Size |
|---|---|
runtimes/win-x64/native/libGLESv2.pdb |
66.6 MB → 66.6 MB (🟢 −64.0 KB) |
runtimes/win-arm64/native/libGLESv2.pdb |
64.5 MB → 64.4 MB (🟢 −32.0 KB) |
runtimes/win-x86/native/libGLESv2.pdb |
67.2 MB → 67.2 MB (🟢 −32.0 KB) |
Informational only — this never blocks the PR. Native binaries are labelled by os/arch.
Replaces the uname -m sniff added earlier with a declarative mechanism. The
script was the same mistake as the TF_BUILD sniff it replaced -- inferring a
policy at runtime, and silently adapting if the pool changed instead of failing
loudly. What a host can do is now stated up front in the pipeline definition.
Adds an `env:` parameter to azure-templates-jobs-bootstrapper.yml that flows to
all four bootstrapper invocations: the Windows and non-Windows direct runs (via
the step env) and both Docker runs. Containers inherit nothing from the agent,
so the env is materialised into an env-file and passed to `docker run
--env-file`; the file is always written, empty when nothing is declared, so the
run command stays uniform. This is generic infrastructure -- nothing about it is
GPU-specific.
Corrects the Metal opt-out scope. macOS and Mac Catalyst drive real Metal on the
x64 pool and passed CI, so they are no longer opted out. Only the iOS leg is,
because it runs the *simulator*, whose virtualized Metal leaves dispatch-queue
state that hangs the test host on shutdown.
Declares ganesh-gl opt-outs for the five container legs. Those images have no X
server, no Mesa and no ICD, so OpenGL genuinely cannot come up there.
Fixes two P/Invoke library names that were the actual cause of the Linux and
container GL failures. DllImport("libGL") and DllImport("libX11") probe the
unversioned .so, which is a linker symlink shipped only in the -dev packages; a
runtime-only host has libGL.so.1 and libX11.so.6 and nothing else. So the Linux
leg was failing to load Mesa even though CI provisions it correctly, and had
been silently skipping that cell for as long as the harness existed.
Provisions the software Vulkan ICD for x86 as well as x64. The .NET Framework
leg runs the suite once per bitness and a 32-bit process cannot load a 64-bit
vulkan-1.dll, so x86 Vulkan had no loader at all: CI showed 25/25 passing on x64
and 21/25 failing on x86. Bitness resolution is delegated to Windows the way the
official Vulkan Runtime installer does it -- loader into System32 and SysWOW64,
each ICD manifest registered in the matching registry view -- because PATH
cannot disambiguate two files with one name.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 03c0625a-14f0-4a6c-a1f6-b02ac5f8701a
The Windows agents have no GPU driver, so Windows falls back to the GDI generic OpenGL 1.1 implementation, which advertises no WGL_ARB_* extensions and cannot satisfy WglContext. Declared as an opt-out rather than left red, since there is no fix available to land today. This is a provisioning gap, not a platform limit, and the gap is documented so it does not quietly become permanent: Mesa's llvmpipe would fix it with no test-code change (its WGL extension string statically advertises both WGL_ARB_pixel_format and WGL_ARB_pbuffer, and it reports WGL_FULL_ACCELERATION_ARB), but there is no trustworthy feed for Mesa's Windows binaries yet. Silk.NET ships no desktop-GL package, and its ANGLE package is unusable -- every published version has 32-bit binaries in runtimes/win-x64. Vulkan and Direct3D stay required on Windows: CI showed both working, 25/25 and 5/5 respectively. Also documents the complete set of declared CI opt-outs in gpu-test-policy.md, so what each leg can and cannot do is discoverable without reading the pipeline. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 03c0625a-14f0-4a6c-a1f6-b02ac5f8701a
The policy is a lookup table and one Assert.Skip; it did not need 611 lines across four files. Now 280 across two, with no behaviour change. Collapses GpuBackend.cs, TestPlatforms.cs and GpuPolicy.cs into one file. Drops the GpuAvailability enum in favour of the state token the report already needed, which removes the enum, its doc block and the token mapper. Replaces the Entry class and its constructor with a tuple array, so a backend row is one line. Generates the skip reasons from the id and platform name rather than storing a hand-written paragraph per backend. The detail those paragraphs carried -- which gn arg to flip, which file to edit -- belongs in gpu-test-policy.md, where it is already written, not repeated in every skip message. Trims the call-site comments. "No catch: the policy already decided" appeared at seven sites; the rule is stated once on IRenderer and once in the doc, so the repetitions were noise next to a self-evident RequireOrSkip call. Also drops a stale <see cref="GpuAvailability.Required"/> and the duplicated policy paragraph from the two Vulkan renderers. Verified identical behaviour: same policy report, same typo-guard error, and the same 6092 tests with the same 2 pre-existing failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 03c0625a-14f0-4a6c-a1f6-b02ac5f8701a
Make Phase 10 unconditional: rebuild native, build managed, run the full solution, and fix every failure before staging outputs or creating PRs. Remove platform and human-attention escape hatches. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f66474bf-0d92-48db-a924-6ca2b009d6b2
The policy still carried more machinery than the decision it makes. A GPU test either skips with a reason, fails, or passes; everything else was ceremony. Replaces the GpuBackend enum with GpuBackends string consts. The enum and the id string were the same fact twice, with Id() and Find() only to map between them -- and every renderer declared both, since IRenderer.Name was documented as equal to the id. So IRenderer.Backend is gone entirely and the matrix gates on Name. Replaces the ExistsOn/BuiltOn pair with a single Dictionary<string, TestPlatforms> of where each backend must work. ExistsOn was read in exactly one place, to choose between "does not exist on" and "is not built for" -- a whole platform set per row to pick a verb. The surviving reason, "'ganesh-metal' is not required on windows", is complete; why it is not required belongs in the doc's table, is not actionable at a skip, and was the same per-row prose the previous pass deleted. Moves TestPlatforms and platform detection to TestConfig, which already owns IsWindows, IsApple and IsNanoServer. Neither is a GPU concept, and this removes a third copy of the same #if ladder: VisualPlatform hand-wrote the identical probe order to build golden directory tags, and its strings map exactly onto the enum names, so it now derives from TestConfig. Golden tags verified byte-identical before and after -- that path decides which reference PNG a cell compares to. Drops the messaging helpers. OptOutHint appended "here is how to opt out" to eleven failure messages; the exception already says what broke. Describe() and the ##SKIA-GPU-POLICY## marker duplicated what xUnit already publishes, since a skipped test carries its reason into the same TRX. OptOutSource, the state token and the Cpu special case go with them. Drops Lazy for plain static readonly fields. The runtime already gives the guarantee the Lazy was hand-rolling: a throwing static initializer caches a TypeInitializationException that every later access rethrows, so a malformed opt-out list still fails every GPU test identically. Verified the message is still legible through the wrapper. Startup cost is irrelevant here. Public surface is now two consts, RequireOrSkip and Validate. GpuPolicy.cs goes from 241 lines to 131, and a new test asserts every renderer name is a known id, catching drift the enum could not express. Behaviour verified unchanged: 5927 non-GPU tests still pass with 16 declared skips, the visual matrix still reports 25/5/5/15, golden tags are identical, =all still spares raster, and the typo guard still names the valid ids. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 03c0625a-14f0-4a6c-a1f6-b02ac5f8701a
Deletes documentation/dev/gpu-test-policy.md. The policy is a table and one skip; it does not need a page of its own, and the visual matrix is the main consumer, so the table, the opt-out syntax, the CI opt-out list and the Windows software-GL gap now live in golden-image-tests.md. Merges SkipReason into RequireOrSkip -- SkipReason had no other caller once the report was gone. Replaces Validate() with a public Disabled(). Validate read disabled.Count to force a static initializer, which reads as counting a set that obviously cannot throw; the throw came from class init. Disabled() parses on each call and throws directly, so the typo guard is now a plain InvalidOperationException instead of one wrapped in TypeInitializationException, and the static-init ordering constraint is gone. Tests do not need the cache. Uses the GpuBackends consts for every renderer Name, which were still literals. Rewrites comments that told a story rather than describing the code as it is: the simulator Snap() note said what it "used to be", the Vulkan ICD header narrated the x86 gap as a discovery, test-shared.cake explained what it was deliberately not doing, and Directory.Build.targets did the same for Trim metadata. Also drops comments that only restated the call below them, and fixes two that referenced the removed Unsupported state. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 03c0625a-14f0-4a6c-a1f6-b02ac5f8701a
660 lines across one file becomes 281 across two, and the content now matches
the code.
Splits the GPU policy into its own 77-line page. It gates five test families --
the visual matrix, the GL, Vulkan and Direct3D tests, and the Graphite release
tests -- so scoping it under "golden image tests" was wrong. The previous policy
page was deleted for length, not for existing; this one is a table, an opt-out
syntax and a pointer to the pipeline.
Presents raster, Ganesh and Graphite as three equal families. The old text
described Graphite as in-flight, including a whole "Graphite seam" section that
was a rebase plan for a PR that has since landed, and per-host tables that listed
Graphite renderers as follow-ups.
Corrects claims that no longer held:
* "every cell emits its PNG on pass and fail" was wrong in the case that
matters -- RenderAsync runs before the marker is emitted, so a render failure
emits nothing. The lifecycle section now says which outcomes carry an image.
* "compared strictly" -- comparison is tolerance-based, always.
* golden lookup is three-deep on Nano Server (nanoserver, windows, shared),
not two.
* GpuRenderGate does not exist; serialization is an xUnit collection.
* there is no Direct3D visual renderer, so the coverage table invented one.
* VisualPlatform.Tag is Tags, the tag list omitted nanoserver, and Linux GL is
GLX rather than "GLX/EGL".
* the Desktop/ folder rule claimed to cover Metal, which is shared.
Replaces the host coverage matrix, which conflated compiled, required and seeded
and drifted immediately, with the renderer list plus a pointer to the policy
table and the goldens directory. Merges three overlapping seeding sections into
one, and drops the storage, tolerance and CI rationale essays.
Also removes a dead "direct3d" key from GoldenTolerance -- no renderer has that
name -- and the Ganesh entries that only restated the fallback, leaving raster as
the one special case.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 03c0625a-14f0-4a6c-a1f6-b02ac5f8701a
The TRX markers used three inconsistent names and one was actively wrong:
##SKIA-GOLDEN-IMAGE## carried the *actual* rendered image, not the golden,
and ##SKIA-VISUAL-IMAGE## conflated the golden and the diff, distinguishing
them by a suffix baked into the path.
Rename to one role per marker, all sharing the golden key as their path:
##SKIA-VISUAL-ACTUAL## path=... outcome={pass|mismatch|unseeded} size=... base64=...
##SKIA-VISUAL-GOLDEN## path=... size=... base64=...
##SKIA-VISUAL-DIFF## path=... size=... base64=...
This drops ##SKIA-VISUAL-CELL## by folding the verdict onto the actual image.
The verdict cannot be derived from the images alone -- a pass and an unseeded
test both emit only an actual -- so it still has to be stated, but it no longer
needs a marker of its own. Emitting it means the actual image now goes out
after the comparison rather than before.
The harvest script loses a regex, a dict and the key-mangling that rebuilt
'{scene}.golden.png' to look images up; the three roles are now just three
dicts keyed by the same path.
Vocabulary: 'cell' -> 'test', 'seam' -> 'interfaces', and 'lifecycle' /
'discipline' headings replaced with plain descriptions.
Verified: visual suite 25 total / 5 failed / 5 succeeded / 15 skipped
(unchanged); a forced mismatch emits all three markers and the script writes
actual+golden+diff into mismatch/ with correct bytes; seeding dry-run yields
the same 10 goldens; full non-GPU suite 5927 / 0 failed / 16 skipped.
The outcome= field on the actual-image marker duplicated what the markers
already imply. No golden marker means no golden was committed; a diff only
exists when there was something to diff against. The test result and its
message say whether a comparison passed -- the harvest script does not need
to re-derive that.
So drop the field, and emit the golden and diff whenever a golden was found
rather than only on a mismatch. A pass at 99.5% now ships its diff too, which
is the case actually worth eyeballing.
This deletes the outcome grouping in extract-visual-goldens.py: it no longer
sorts into unseeded/ and mismatch/, it just writes what it found, mirroring
the goldens tree as {renderer}.{platform}/{scene}.{actual,golden,diff}.png.
A scene with only an .actual.png had no golden. Renamed --failures-out to
--images-out to match, since it was never only failures.
Verified: with a forced mismatch, ganesh-gl.windows extracts 5 actual-only
images (unseeded) and raster.windows extracts actual+golden+diff for all 5,
the mismatch showing golden 4323 B against actual 4711 B. Seeding dry-run
unchanged at 10 goldens. Visual suite 25/5/5/15, full non-GPU suite
5927 / 0 failed / 16 skipped. YAML parses.
Two dictionaries, a composite string key and a three-branch lookup existed to express 'raster is strict, everything else is not'. ByRendererScene was empty and ByRenderer held a single entry, since the ganesh-* rows only restated the fallback and the direct3d row named a renderer that does not exist. Now it is a ternary, and sceneName leaves the signature because nothing read it. If a scene ever needs its own tolerance, special-case it then. Visual suite unchanged at 25/5/5/15.
Both said a missing display or ICD makes the GPU tests skip and that the provisioning 'never turns the build red'. Under this policy ganesh-gl and ganesh-vulkan are required on Linux, so missing provisioning fails.
A review against the code turned up five statements this change falsifies:
* AGENTS.md said tests never skip 'except missing hardware' -- missing
hardware is now precisely what fails.
* native-dependency-update and update-skia (twice) told the reader that
backends self-skip when unavailable.
* golden-image-tests.md suggested special-casing a single scene in
GoldenTolerance.For, which takes only the renderer name.
Graphite landed in #3968 and skia_enable_graphite=true on every platform, with Vulkan on Windows/Linux/Android, Metal on Apple and Dawn on WASM. Six skills still told the reader to filter those changes out: * security-audit classed Graphite, Vulkan and Dawn as NOT_REACHABLE compile flags 'we don't use', which would dismiss a live CVE. It now points at the gn args in native/*/build.cake instead of naming a fixed list. * update-skia scored Graphite changes as SKIP, marked skgpu::graphite:: and Dawn/wgpu as irrelevant, and framed shared GpuTypes.h changes as a Graphite-only trap rather than a change reaching both families. * skia-analyst excluded Graphite and Dawn changes outright. * sample-scout classified any file naming Graphite or Dawn as low and called Graphite 'not SkiaSharp'. Backend-internal GMs are still low; the test is now whether it exercises internals, not which backend it names. * api-docs used 'Vulkan (not yet supported)' as a formatting example. Also documents the container legs' ganesh-gl opt-out, which this branch added to five legs without saying so in containerized-testing.md.
The previous pass flipped 'exclude Graphite, we use Ganesh' into 'include
Graphite', which is the same mistake with the sign changed: naming one backend
as included implies the others are not, and the guidance still had to be
rewritten every time a backend lands. It also introduced three false claims --
skia_enable_graphite is not set on Tizen and is conditional on WASM, Nano Server
builds with Vulkan and Direct3D off, and Vulkan is not deprecated.
Rewritten around the real criteria instead:
* update-skia's risk table categorises kinds of change, so the backend row is
simply gone -- a GPU change is already Removed/Renamed/New/Behavior.
* the relevance table now says relevance is whether our C API or a shipped
build touches the code, and folds the four GPU prefixes into one row that
points at native/*/build.cake rather than asserting a platform map.
* skia-analyst just drops the exclusions: what is not excluded is included.
* security-audit's NOT_REACHABLE names no backend at all. It asks for the
platform and the flag, and notes a flag off in one build is often on in
another -- which is the actual trap.
* sample-scout keeps 'is it backend internals?' and drops the platform list.
Also fixes a pre-existing scope error: the 'full test suite' command ran
SkiaSharp.Tests.Console.csproj, which references neither the Vulkan nor the
Direct3D project, while claiming to run all three. It now runs the solution.
Mark command lines, paths, globs, and source symbols as inline code so the Phase 3 prompt is unambiguous and readable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Use the deterministic binding generator directly, removing the region-order compatibility pass. Provision Xvfb with Mesa softpipe alongside lavapipe, remove the ganesh-gl opt-out, and require agents to fix every GpuPolicy backend instead of introducing skips. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Keep Phase 3 read-only, remove sanctioned build and feature bypasses, distinguish initial diagnostics from final validation, pin manifests to the exact merged upstream SHA, and make the post-step verify workflow-resolved state plus the final test exit code. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Export detector results from the job that actually resolves them, require final dependency decisions to match the built DEPS file, and make generated summaries distinguish required coverage from platform-policy skips. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Require Component Governance versions to match authoritative source metadata whenever a tracked dependency revision or enabled state changes, while allowing verified same-version revision rolls. Use Claude Opus 5 for the primary sync agent. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Keep Claude Opus 4.8 because the pinned AWF v0.83.4 runtime rejects claude-opus-5 before agent execution. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Keep long builds and test suites in a single foreground shell invocation so the agent does not spend turns polling progress. Add modest per-run credit headroom for Opus variance. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Keep the long-command no-poll guidance, which returned the workflow to 1,305 credits, but retain 2,000 as the absolute per-run failure ceiling. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Run the same deterministic update workflow with Claude Sonnet 5 to compare correctness and cost against the validated Opus 4.8 baseline. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Prevent whole-update delegation after the Sonnet trial handed mutating work to a fallback model, and require automation handoff values to remain the exact resolved inputs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Require every behavior in a removed patch to be verified upstream and require final DEPS revisions, decisions, and Component Governance metadata to agree exactly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Sonnet 5 reduced cost on a successful run but failed to establish three consecutive clean updates within the retry limit. Keep the general guardrail improvements and restore the proven Opus 4.8 primary model. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Reject fork-head substitutions for the target upstream SHA in version generation, publication, and formal review. Keep the tested mono/skia head and exact upstream merge commit as distinct manifest values. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Keep merge/build/test/fix work with the agent while deriving branch, provenance, version, and publication facts from workflow state and the completed git graph. Finalize metadata before exposing the push-only PAT step. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
The pre-agent tool bootstrap may modify externals/depot_tools; exclude that environmental path while still rejecting uncommitted product and workflow changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Apply the existing depot-tools bootstrap exclusion to the final worktree assertion after deterministic metadata finalization. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Compile the final depot-tools clean-check exclusion into the executable agentic workflow. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Make work detection the detector default with explicit resolve-only and output modes. Consolidate checkout preparation, and keep PR rendering and templates inside the trusted publisher using jq. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Keep each PR template beside the values that render it, and reduce AGENTS.md to durable automation policy without publisher implementation details. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Track exact Skia DEPS identities in cgmanifest and make the metadata helper emit a deterministic change signal. Require source-backed agent review for every changed tracked dependency, reject version-only drift, and retain initial Vulkan test evidence before publishing sync PRs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Populate authoritative version sources for every tracked Skia dependency, including explicit canonical-version and placeholder exceptions. Require source evidence for unchanged baselines as well as dependency rolls. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Make the agent refresh fork-patch fingerprints immediately before completion so late DEPS and C-shim adaptations cannot leave stale evidence for the trusted publisher to reject. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
Run metadata and fork-patch validation as an explicit final Phase 10 section against the tested mono/skia tree. Prevent delivery from starting until stale fingerprints and dependency evidence are fully reconciled. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f137c11-1837-4184-af2b-c25767fcb781
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Make Skia upstream synchronization a clear four-part pipeline:
Chrome m152 is the blind validation case, not an encoded answer. The agent independently discovered the required VMA compatibility roll and Ganesh allocator adaptation through normal compiler/test failures.
Related issues
N/A.
Required skia PR
None.
Areas affected
binding/)externals/skia/src/c,include/c)Changes
None — workflow/skill only; no public API or product runtime behavior changes.
Key behavior:
SKIA_SYNC_*.upstream_refandupstream_merge_commit, so milestone and upstream-main reviews use the correct provenance.skia-sync/*branches.Testing
Final simplified architecture
Workflow run 30776352709 completed successfully from exact PR head
0fa6952f251:d840f796fd, upstream refchrome/m152, and exact upstream SHA2a9b593babThe workflow PR itself is also fully green.
Additional validation
gh aw compile auto-skia-sync --validate: 0 errors, 0 warningsA deep manual review also caught a prior run that conflated the fork head with
upstream_merge_commit. That pair was closed, formal blocked report #328 was persisted, and the failure led to the deterministic finalizer/provenance design validated above.Checklist
Changesabove lists all public API and behavioral changes or states that none changed