feat/chores: Serialization Behaviour, CI Improvements#261
Open
Eli Pinkerton (wallstop) wants to merge 31 commits into
Open
feat/chores: Serialization Behaviour, CI Improvements#261Eli Pinkerton (wallstop) wants to merge 31 commits into
Eli Pinkerton (wallstop) wants to merge 31 commits into
Conversation
…idator - Gitignore machine-local .mcp.json + .cursor/mcp.json; drop stale DxMessaging cruft. - Fix a command-injection in probe-unity-mcp-endpoint.sh, align arg/env precedence, and correct a misplaced shellcheck directive in both copied MCP scripts. - Remove the dangling "Claude Desktop helper" README section and add a session-binding note. - Add scripts/validate-mcp-config.ps1 (+ self-test + validate-mcp-config.yml CI) enforcing configs are gitignored, valid, target /mcp, and doc refs resolve. - Add the mcp-configuration skill, MCP Local Setup nav entry, and the "mcp" cspell term. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…atrix
- .github/unity-versions.json (2021.3.45f1 / 2022.3.45f1 / 6000.3.16f1) as the
single version source; .github/actionlint.yaml declares the RAM-64GB runner label.
- unity-tests.yml: matrix-config (secrets gate) -> runner-preflight (soft-pass) ->
[self-hosted, Windows, RAM-64GB] matrix (version x editmode/playmode/standalone),
max-parallel:1, org build-lock acquire/release, perf excluded (!Performance;!Stress).
- unity-benchmarks.yml: dedicated perf job (Performance;Stress) + a full-N Random leg
(UH_RANDOM_SAMPLE_COUNT) + auto-commit of raw results to top-level perf-results/.
- 6 composite actions + scripts/unity/{run-ci-tests.ps1, ensure-editor.ps1,
lib/asmdef-discovery.js}; stuck-job-watchdog / unstick-run / runner-bootstrap +
the unity-runners-after-transfer runbook.
- The matrix cleanly SKIPS until ORG_BUILD_LOCK_TOKEN + UNITY_SERIAL/EMAIL/PASSWORD exist.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s taxonomy + lint
- RandomTestBase: SampleCount 12.75M -> env-overridable 250k fast default
(UH_RANDOM_SAMPLE_COUNT restores the full count for the benchmark job); add a
sqrt(average) deviation floor so reduced-N runs stay reliably green while the
large-N run keeps its original tighter sensitivity. Monte-Carlo validated.
- Add lint rules UNH007 (giant literal loops outside perf fixtures), UNH008 (perf
fixtures must carry [Category("Performance"|"Stress")] so the CI filter excludes
them) and UNH009 (advisory: per-test AssetDatabase churn) + red-green tests.
- Tag the 14 Tests/Runtime/Performance fixtures + 2 wall-clock pool tests Performance;
trim the Proto RoundTripLargeCollections correctness test 100k -> 10k.
- Add the test-performance skill.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ress tests - SerializationFailureException (+ sealed subclasses); Serializer deserialize paths wrapped so every public entry point throws SerializationFailureException or has a Try* sibling, enforced by SerializerApiContractTests. - New contract/fuzz/Try-API/exception tests; rename test methods to PascalCase (UNH004) and replace Assert.IsNull/IsNotNull with Assert.IsTrue(== null / != null) (UNH005). - Update defensive-programming / use-serialization / forbidden-patterns concepts and add the serialization-safety skill (the documented "never throw" carve-out). - Trim the JSON RoundTripLargeCollections correctness test 100k -> 10k. - Drop stray .llm/skills/*.md.meta (skills carry no Unity .meta); regenerate skills index. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eaks Pre-existing branch work, committed to keep the tree clean. Lint + csharpier clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…porter Importer metadata change (DefaultImporter -> ScriptedImporter) for the docs SVGs plus a material tweak; pre-existing branch work. Isolated for easy revert. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…atrix Goal 1 (special defines): add -AdditionalScriptingDefines to run-ci-tests.ps1, injected via a PlayerSettings.SetScriptingDefineSymbols + SaveAssets configure pass BEFORE -runTests so the asmdef test assemblies compile with them; add a Unity-6-only `unity-tests-single-threaded` job (editmode+playmode, core-only, SINGLE_THREADED) with its own org-lock/preflight/license/verify scaffolding and -single-threaded cache/artifact paths. Exercises the 10-file SINGLE_THREADED path never compiled in the normal matrix. Goal 2 (integrations): add .github/integration-packages.json (Reflex/VContainer/ Extenject from OpenUPM) + -IncludeIntegrations; the main matrix now installs the 3 DI packages and runs the Reflex/VContainer/Zenject integration asmdefs across all Unity versions. Also prettier-fix asmdef-discovery.js. Unverified without a self-hosted run: define persistence+recompile and OpenUPM pin resolution. Matrix still skips until secrets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- extract-perf-metrics.js parses the benchmark NUnit results (unity-helpers perf tests emit Stopwatch timings as GFM markdown tables in each test's <output> CDATA) into normalized metrics; render-perf-deltas.js diffs vs a committed rolling baseline and renders Markdown + JSON with regression classification (report-only). - Seed perf-results/baseline.json; the benchmark commit job now extracts, diffs, updates the baseline, writes perf-deltas.md, and commits perf-results/**. - Ignore perf-results/** in prettier + cspell so generated reports never trip CI. - Both JS pass --self-test. Unverified without a real results.xml. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| on: | ||
| push: | ||
| branches: | ||
| - main |
There was a problem hiding this comment.
MCP push skips master branch
Low Severity
The new MCP validation workflow’s push trigger lists only main, while unity-tests.yml in the same change set runs on pushes to both master and main. Direct pushes to master that touch MCP-related paths therefore skip this validator even though Unity CI can still run for that branch.
Reviewed by Cursor Bugbot for commit 21a6c97. Configure here.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens and formalizes runtime serialization error behavior (typed exception hierarchy + non-throwing Try* APIs), expands test coverage around those contracts (including fuzz/contract tests), and adds/extends automation around CI + developer workflows (hooks, license header tooling, MCP config validation).
Changes:
- Introduce and enforce a typed serialization failure contract (
SerializationFailureExceptionhierarchy) across serializer entry points; update existing tests to the new contract. - Add new serialization test suites (API contract,
Try*contract, fuzzing, exception hierarchy tests) and adjust performance/correctness tests to keep the main suite fast. - Expand repo automation: MCP local setup tooling + validation workflow, hook install improvements, doc-link lint path-scoping, and license-year cache/worktree safety.
Reviewed changes
Copilot reviewed 155 out of 158 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/Runtime/Utils/RuntimeSingletonTests.cs | Adds coverage for singleton ClearInstance + registry clearing. |
| Tests/Runtime/Serialization/SerializerTryApiTests.cs.meta | Unity meta for new test file. |
| Tests/Runtime/Serialization/SerializerTryApiTests.cs | Adds tests for Try* serializer APIs and exception propagation rules. |
| Tests/Runtime/Serialization/SerializerFuzzTests.cs.meta | Unity meta for new test file. |
| Tests/Runtime/Serialization/SerializerFuzzTests.cs | Adds fuzz tests + allocation guard for lazy exception message composition. |
| Tests/Runtime/Serialization/SerializerExceptionContractTests.cs.meta | Unity meta for new test file. |
| Tests/Runtime/Serialization/SerializerApiContractTests.cs.meta | Unity meta for new test file. |
| Tests/Runtime/Serialization/SerializerApiContractTests.cs | Adds reflection-based “forever gate” for deserializer/Try* sibling parity + null-input Try* contract test. |
| Tests/Runtime/Serialization/SerializerAdditionalTests.cs | Updates existing tests to new configuration/input exception types. |
| Tests/Runtime/Serialization/SerializationFailureExceptionTests.cs.meta | Unity meta for new test file. |
| Tests/Runtime/Serialization/SerializationFailureExceptionTests.cs | Tests the new serialization exception hierarchy (immutability, lazy Message, serialization). |
| Tests/Runtime/Serialization/ProtoSerializationTests.cs | Aligns protobuf tests to typed serialization exception contract. |
| Tests/Runtime/Serialization/ProtoSerializationCorrectnessTests.cs | Reduces large collection sizes to keep correctness tests fast. |
| Tests/Runtime/Serialization/ProtoInterfaceResolutionEdgeTests.cs | Updates interface/abstract resolution tests to expect typed type exceptions. |
| Tests/Runtime/Serialization/JsonSerializationCorrectnessTests.cs | Updates JSON tests to typed corrupt-data exception + reduces large collection sizes. |
| Tests/Runtime/Random/RandomTestBase.cs | Makes sampling count configurable (env override) + adds statistical deviation floor for stability. |
| Tests/Runtime/Pool/WallstopGenericPoolTests.cs | Tags a perf-sensitive test with Performance. |
| Tests/Runtime/Pool/RollingHighWaterMarkTests.cs | Tags a perf-sensitive test with Performance. |
| Tests/Runtime/Performance/SpatialTree3DPerformanceTests.cs | Adds Performance category marker. |
| Tests/Runtime/Performance/SpatialTree2DPerformanceTests.cs | Adds Performance category marker. |
| Tests/Runtime/Performance/RelationalComponentBenchmarkTests.cs | Adds Performance category marker. |
| Tests/Runtime/Performance/ReflectionPerformanceTests.cs | Adds Performance category marker. |
| Tests/Runtime/Performance/RandomPerformanceTests.cs | Adds Performance category marker. |
| Tests/Runtime/Performance/ProtoSerializationPerformanceTests.cs | Adds Performance category marker. |
| Tests/Runtime/Performance/ProtoEqualsPerformanceTests.cs | Adds Performance category marker. |
| Tests/Runtime/Performance/PoolPurgeTriggerPerformanceTests.cs | Adds Performance category marker. |
| Tests/Runtime/Performance/PerformanceBaselineTests.cs | Adds Performance category marker. |
| Tests/Runtime/Performance/ListExtensionPerformanceTests.cs | Adds Performance category marker. |
| Tests/Runtime/Performance/IListSortingPerformanceTests.cs | Adds Performance category marker. |
| Tests/Runtime/Performance/ConvexHullPerformanceTests.cs | Adds Performance category marker. |
| Tests/Runtime/Performance/ConcaveHullPerformanceTests.cs | Adds Performance category marker. |
| Tests/Runtime/Extensions/UnityExtensionsBasicTests.cs | Updates Rigidbody2D stop test (velocity property usage changed). |
| Tests/Editor/CustomDrawers/SerializableSetPropertyDrawerTests.cs | Adds explicit alias using for TestData type. |
| Shaders/Materials/BackgroundMask-Material.mat | Unity material serialization version update + new locking flag. |
| scripts/validate-mcp-config.ps1.meta | Unity meta for new script. |
| scripts/validate-mcp-config.ps1 | Adds MCP client config validation (gitignore, structural validity, doc references). |
| scripts/update-license-headers.sh | Makes license header updates worktree-safe + path-scoped + tracked-file enumeration. |
| scripts/unity/run-ci-tests.ps1.meta | Unity meta for new/updated script asset. |
| scripts/unity/lib/render-perf-deltas.js.meta | Unity meta for script asset. |
| scripts/unity/lib/extract-perf-metrics.js.meta | Unity meta for script asset. |
| scripts/unity/lib/asmdef-discovery.js.meta | Unity meta for script asset. |
| scripts/unity/lib.meta | Unity meta for new folder. |
| scripts/unity/ensure-editor.ps1.meta | Unity meta for script asset. |
| scripts/tests/test-validate-mcp-config.ps1.meta | Unity meta for new test script asset. |
| scripts/tests/test-sync-script-contracts.ps1 | Adds additional contract checks (pwsh path binding, hook installer, Prettier LF override, markdown fence fixer wiring). |
| scripts/tests/test-pre-push-changed-files.sh | Expands changed-file collection tests (diff-filter deletes/renames, doc-link triggers, doc-link scope). |
| scripts/tests/test-lint-tests.ps1 | Adds tests for UNH007/UNH008/UNH009 lint behavior. |
| scripts/tests/test-lint-doc-links.ps1 | Adds path-scoped invocation support and additional markdown lint scenarios. |
| scripts/tests/test-license-cache.sh | Adds worktree-safe cache checks + validates dynamic year + staged-license wiring. |
| scripts/tests/test-hook-patterns.sh | Adds coverage for rename-aware diff filters in pre-commit. |
| scripts/normalize-eol.ps1 | Adds support for trailing args under pwsh -File + merges path inputs. |
| scripts/mcp/start-unity-mcp-bridge.ps1.meta | Unity meta for new script. |
| scripts/mcp/start-unity-mcp-bridge.ps1 | Adds Windows-side supergateway bridge launcher for Unity MCP. |
| scripts/mcp/README.md.meta | Unity meta for new docs asset. |
| scripts/mcp/README.md | Documents Unity MCP bridge workflow for Windows host + Linux devcontainer. |
| scripts/mcp/probe-unity-mcp-endpoint.sh.meta | Unity meta for new script. |
| scripts/mcp/probe-unity-mcp-endpoint.sh | Adds endpoint probing/diagnostics for MCP bridge reachability. |
| scripts/mcp/configure-unity-mcp-endpoint.sh.meta | Unity meta for new script. |
| scripts/mcp/configure-unity-mcp-endpoint.sh | Generates/updates local MCP client configs (VS Code, Claude Code, Cursor, Codex). |
| scripts/mcp.meta | Unity meta for new folder. |
| scripts/lint-tests.ps1 | Adds UNH007/UNH008 blocking rules + UNH009 advisory reporting for test perf budgets/churn. |
| scripts/lint-staged-markdown.ps1 | Runs markdown fence language fixer before markdownlint. |
| scripts/install-hooks.ps1 | Adds -HooksOnly flow + executable bit restoration on non-Windows. |
| scripts/fix-markdown-fence-languages.ps1.meta | Updates Unity meta GUID for the script asset. |
| scripts/check-eol.ps1 | Adds support for trailing args under pwsh -File + merges path inputs. |
| scripts/audit-license-years.sh | Makes cache worktree-safe, computes year dynamically, enumerates tracked files, adds path normalization. |
| Runtime/WallstopStudios.UnityHelpers.asmdef | Switches to overrideReferences: true for assembly reference control. |
| Runtime/Utils/RuntimeSingletonRegistry.cs | Renames init hook and exposes a manual clear entry point for singleton registry. |
| Runtime/Utils/RuntimeSingleton.cs | Makes ClearInstance public + documents registry-based domain reload clearing. |
| Runtime/Protobuf-Net/System.Collections.Immutable.dll.meta | Adjusts plugin importer settings (reference validation + editor enablement). |
| Runtime/Core/Serialization/SerializationFailureException.cs.meta | Unity meta for new exception hierarchy file. |
| Runtime/Core/Extension/Partials/Physics.cs | Updates Rigidbody2D Stop implementation (velocity property usage changed). |
| perf-results/baseline.json.meta | Unity meta for new perf baseline artifact. |
| perf-results/baseline.json | Seeds rolling perf baseline storage for benchmark workflow. |
| perf-results.meta | Unity meta for new folder. |
| package.json | Adds MCP config validation + expands test/validation scripts; updates hooks installer entrypoint. |
| mkdocs.yml | Adds MCP local setup guide to docs nav. |
| docs/runbooks/unity-runners-after-transfer.md.meta | Unity meta for new docs asset. |
| docs/runbooks.meta | Unity meta for new folder. |
| docs/images/utilities/singletons/singletons-lifecycle.svg.meta | Updates SVG importer metadata. |
| docs/images/utilities/singletons/data-distribution-decision.svg.meta | Updates SVG importer metadata. |
| docs/images/utilities/reflection/reflection-scan.svg.meta | Updates SVG importer metadata. |
| docs/images/utilities/random/random-generators.svg.meta | Updates SVG importer metadata. |
| docs/images/utilities/math/polyline-simplify.svg.meta | Updates SVG importer metadata. |
| docs/images/utilities/math/geometry-edge-cases.svg.meta | Updates SVG importer metadata. |
| docs/images/utilities/data-structures/trie.svg.meta | Updates SVG importer metadata. |
| docs/images/utilities/data-structures/sparse-set.svg.meta | Updates SVG importer metadata. |
| docs/images/utilities/data-structures/heap.svg.meta | Updates SVG importer metadata. |
| docs/images/utilities/data-structures/disjoint-set.svg.meta | Updates SVG importer metadata. |
| docs/images/utilities/data-structures/deque.svg.meta | Updates SVG importer metadata. |
| docs/images/utilities/data-structures/deque-queue.svg.meta | Updates SVG importer metadata. |
| docs/images/utilities/data-structures/cyclic-buffer.svg.meta | Updates SVG importer metadata. |
| docs/images/utilities/data-structures/bitset.svg.meta | Updates SVG importer metadata. |
| docs/images/unity-helpers-banner.svg.meta | Updates SVG importer metadata. |
| docs/images/spatial/r-tree-3d.svg.meta | Updates SVG importer metadata. |
| docs/images/spatial/r-tree-2d.svg.meta | Updates SVG importer metadata. |
| docs/images/spatial/query-boundaries.svg.meta | Updates SVG importer metadata. |
| docs/images/spatial/quadtree-2d.svg.meta | Updates SVG importer metadata. |
| docs/images/spatial/octree-3d.svg.meta | Updates SVG importer metadata. |
| docs/images/spatial/kd-tree-3d.svg.meta | Updates SVG importer metadata. |
| docs/images/spatial/kd-tree-2d.svg.meta | Updates SVG importer metadata. |
| docs/images/spatial/convex-hull.svg.meta | Updates SVG importer metadata. |
| docs/images/spatial/concave-hull.svg.meta | Updates SVG importer metadata. |
| docs/images/serialization/serialization-flow.svg.meta | Updates SVG importer metadata. |
| docs/images/relational-components/relational-wiring.svg.meta | Updates SVG importer metadata. |
| docs/images/effects/effects-pipeline.svg.meta | Updates SVG importer metadata. |
| docs/images/effects/attribute-resolution.svg.meta | Updates SVG importer metadata. |
| docs/guides/mcp-local-setup.md.meta | Unity meta for new guide. |
| docs/guides/mcp-local-setup.md | Adds MCP local setup documentation (devcontainer + Windows host relay). |
| cspell.json | Ignores perf artifacts + adds new technical terms. |
| .prettierrc.json | Adds .github/** to LF override set. |
| .prettierignore | Ignores generated perf results tree from Prettier checks. |
| .llm/skills/use-serialization.md | Documents serializer exception contract + Try* usage. |
| .llm/skills/test-performance.md | Adds repository guidance for test performance budgets (UNH007/8/9). |
| .llm/skills/serialization-safety.md | Adds detailed serializer safety contract and forbidden patterns. |
| .llm/skills/optimize-git-hooks.md | Tightens documented hook performance budgets and guidance. |
| .llm/skills/mcp-configuration.md | Documents Unity MCP configuration conventions and validation guarantees. |
| .llm/skills/markdown-reference.md | Clarifies fenced block language guidance (text vs mermaid). |
| .llm/skills/linter-reference.md | Notes scoped doc-link lint option. |
| .llm/skills/formatting-and-linting.md | Updates hook workflow documentation (doc-link lint, license audit, fence fixer). |
| .llm/skills/defensive-programming.md | Updates exception philosophy to include serialization carve-out (table formatting needs correction). |
| .llm/references/forbidden-patterns.md | Adds explicit serialization forbidden patterns and required contracts. |
| .gitignore | Ignores machine-local MCP client config files. |
| .github/workflows/validate-mcp-config.yml | Adds workflow to run MCP config validator + its tests. |
| .github/unity-versions.json | Adds canonical Unity version source for CI matrices. |
| .github/integration-packages.json | Adds canonical pinned DI integration package list for CI. |
| .github/actions/validate-unity-license/action.yml | Adds Unity serial activation preflight composite action. |
| .github/actions/return-unity-license/action.yml | Adds best-effort license return composite action. |
| .github/actions/dump-unity-log-tail/action.yml | Adds diagnostic log tail dump + catastrophic pattern scan on failure/cancel. |
| .github/actions/compute-unity-assemblies/action.yml | Adds asmdef discovery-based test assembly list computation action. |
| .github/actionlint.yaml | Adds actionlint config for custom self-hosted runner labels. |
| .githooks/post-rewrite | Makes license cache invalidation worktree-safe via git rev-parse --git-path. |
Comments suppressed due to low confidence (1)
Runtime/Utils/RuntimeSingleton.cs:135
ClearInstance()destroys a Unity object; calling it from a background thread can cause Unity thread-affinity errors. Since the method is now public, it should enforce main-thread execution before invokingDestroy().
Move the generated skills index out of the embedded context.md block into a standalone .llm/skills/index.md (linked from context.md). Make generation byte-for-byte deterministic across OS: ordinal sort, no timestamp, UTF-8 no-BOM + LF. Linter now validates the file, enforces ASCII-only triggers (root-causing the em-dash drift that broke CI), the no-BOM/LF contract, generator determinism, and trigger field-count/category. CI lint job runs on ubuntu+windows. index.md excluded from authored-skill size limit and Prettier. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…it lint report-slow-tests.ps1 ranks the slowest fixtures/cases from NUnit results (+GitHub step summary) and can fail CI over a per-fixture wall-clock budget; wired into both Unity CI jobs and the devcontainer runner. UNH010 (advisory) flags literal real-time waits (WaitForSeconds/Task.Delay/Thread.Sleep) in non-perf tests. test-performance.md documents the measure-first loop, UNH010, and the logic/IO-split pattern. This is the durable measure-and-enforce backbone for keeping the suite fast. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extract the alpha-bounded rect + padding + pivot computation from ProcessSprite into an internal static ComputeCrop (behavior-preserving; ProcessSprite calls it). Move the 23 dimension/padding/edge [TestCase]s from full texture import round-trips (~6s each) into pure SpriteCropperMathTests (microseconds); keep 8 integration tests covering the real import/crop/output pipeline. MCP-verified: SpriteCropperAdditionalTests 185s -> 53s, 33/33 green, coverage retained and expanded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extract a pure, internal `FitComputation ComputeFit(...)` from FitTextureSizeWindow's per-asset processing path so the exhaustive fit-mode / clamp / rounding permutations can be exercised without a texture-import round-trip. Move the 168 parameterized math cases out of the slow integration fixture into a new pure fixture (FitTextureSizeMathTests) that calls ComputeFit directly; retain the genuinely-integration tests (label/name filters, platform overrides, selection scoping, last-run summary, default-min clamp) in FitTextureSizeWindowTests. Behavior-preserving: the test-file change is a pure relocation (0 insertions, only deletions) and the window now consumes `fit.TargetSize` exactly where it previously computed the size inline. Coverage adversarially audited as retained -- 168/168 deleted rows have an identical pure case, all four FitMode members + min/max precedence + RoundToNearest tie-breaking + power-of-two boundaries are covered, and all seven ComputeFit branches are hit. Pure fixture verified green via the Unity TestRunnerApi (168/168 pass). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ixture Extract a pure, internal `bool WouldTextureSettingsChange(in TextureSettingsState current, SpriteSettings spriteData)` plus a `TextureSettingsState` snapshot struct from SpriteSettingsApplier's importer path. `WillTextureSettingsChange` now builds the snapshot from the live importer and delegates to the pure decision. Move the per-field change-detection cases (every TextureSettingsState field, both changed->true and matches->false, plus all-match, one-differs, and no-apply-flags) out of the slow integration fixture into a new pure fixture (SpriteSettingsApplierLogicTests) that calls WouldTextureSettingsChange directly; retain the genuinely-integration tests (path matching, profile priority, importer round-trip via TryUpdate / buffer reuse / WillChange-vs-TryUpdate consistency) in SpriteSettingsApplierAdditionalTests. Behavior-preserving: the test-file change is a pure relocation (0 insertions, only deletions). Coverage adversarially audited as retained -- all 16 deleted methods have byte-identical TestCaseSource bodies in the pure fixture, all 14 snapshot fields are covered in both directions, pivot vector and alignment are isolated, apply-flag on/off are both exercised, and all 15 comparison branches are hit (the pure fixture even adds a null-profile case). Verified green via the Unity TestRunnerApi (92/92 pass). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extract an `internal static bool AreAnimationClipsContentEqual(AnimationClip, AnimationClip)` overload from AnimationCopierWindow's path-based comparator; the existing path overload now loads both clips via AssetDatabase and delegates to it (behavior-preserving -- the comparison block moved verbatim, same order / operators / Mathf.Approximately thresholds / null->false semantics). Add a pure fixture (AnimationClipContentEqualityTests, 45 cases) that builds in-memory AnimationClips and exercises every comparison dimension -- frameRate, length, wrapMode, legacy, all 10 AnimationClipSettings fields, the animation- event sub-fields + objectReference, float-curve binding (count/path/property/ type) and keyframe detail (time/value/key-count/tangent/weight/pre+post wrap), and object-reference-curve binding/keyframe (count/time/value). Runs in ~0.7s with no asset I/O. This is a coverage improvement, not a runtime cut: the content-equality logic was previously exercised only indirectly by a few slow asset-round-trip integration tests. The genuine integration behavior (CopyAsset GUID preservation, mirror-delete, nested-folder creation, unchanged-after-copy detection) remains in AnimationCopierWindowTests, which is unchanged. Verified green via the Unity TestRunnerApi (45/45 pass); the extraction was adversarially reviewed as 1:1 behavior-preserving. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…in clip compare Two fixes surfaced while hardening AnimationClip content-equality coverage: 1. Remove vestigial work in AnalyzeAnimations. AnimationFileInfo.Hash was computed via AssetDatabase.GetAssetDependencyHash for every source clip AND every destination orphan on every analyze, but the field was never read anywhere (the classifier switched to content comparison long ago). Removed the field, both assignments, and GetDependencyHashString -- eliminating one AssetDatabase call per file in the analyze path. Classification (New/Changed/Unchanged/Orphan) is unaffected; it uses File.Exists + AreAnimationClipsContentEqual, not the hash. 2. Adopt the project's WallMath.Approximately for the clip float comparisons and fix cycleOffset. cycleOffset was compared with exact `!=` while its sibling settings floats (startTime/stopTime) used tolerant comparison, so sub-tolerance float round-trip noise on cycleOffset was spuriously flagged as a change. Switched all 14 float comparisons (frameRate, length, cycleOffset, start/stop time, event time/float, keyframe time/value/tangents/weights, objref keyframe time) to `x.Approximately(y, ContentEqualityTolerance)`. Tolerance is 0f so WallMath falls back to its built-in relative fudge (~1e-6 * magnitude) -- behavior- equivalent to the prior Mathf.Approximately (verified) and tight enough to catch real edits; the default 0.045f would mask real animation changes and skip a needed re-copy. Red-green: added SubToleranceCycleOffsetDifferenceIsTreatedAsEqual (RED under exact `!=`, GREEN after). Full pure fixture 46/46 via Unity TestRunnerApi. Adversarially reviewed: behavior-equivalence brute-force-checked, dead-code removal confirmed reader-free, AreObjectReferencesEqual filename comparison left as-is (verbatim-copy workflow references identical assets, so it is inert). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| try { | ||
| Start-Transcript -Path $logPath -Force | Out-Null | ||
| . $script | ||
| $unityVersions = @('2021.3.45f1', '2022.3.45f1', '6000.3.16f1') |
There was a problem hiding this comment.
Bootstrap pins stale Unity versions
Medium Severity
The runner-bootstrap workflow hardcodes Unity version strings inline while CI matrix jobs read .github/unity-versions.json, so bumping the canonical JSON alone leaves bootstrap maintaining or auditing the wrong editor versions.
Reviewed by Cursor Bugbot for commit 0115eb6. Configure here.
| # This is the ONLY place UH_RANDOM_SAMPLE_COUNT is consumed. | ||
| - name: Run Random suite at full sample count | ||
| id: run_random_thorough | ||
| if: ${{ matrix.test-mode == 'editmode' && steps.compute.outputs.is-empty != 'true' }} |
There was a problem hiding this comment.
Random suite runs after failure
Medium Severity
On editmode benchmark legs, Run Random suite at full sample count only gates on editmode and a non-empty assembly list, not on run_tests succeeding. After the perf run fails, CI can still launch the long full-sample Random Unity run under the same org lock.
Reviewed by Cursor Bugbot for commit b332709. Configure here.
| with: | ||
| results-dir: .artifacts/unity/benchmarks/${{ matrix.unity-version }}-${{ matrix.test-mode }} | ||
| label: Benchmarks ${{ matrix.unity-version }} ${{ matrix.test-mode }} | ||
| expected-empty: ${{ steps.compute.outputs.is-empty }} |
There was a problem hiding this comment.
Random benchmark leg unverified
Medium Severity
The new edit-mode Run Random suite at full sample count step writes results under .artifacts/unity/benchmarks-random/, but Verify tests actually ran only checks .artifacts/unity/benchmarks/${{ matrix.unity-version }}-${{ matrix.test-mode }}. A broken or empty Random run can pass CI while the perf-category leg still verifies.
Reviewed by Cursor Bugbot for commit 9ca9cf1. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fe4b841. Configure here.
| rm -f "$CACHE_FILE" | ||
| echo "License year cache invalidated (history rewritten)." | ||
| if [ ! -f "$hook_script" ] || ! command -v pwsh >/dev/null 2>&1; then | ||
| exit 0 |
There was a problem hiding this comment.
post-rewrite skips cache clear
Medium Severity
The post-rewrite shell wrapper exits successfully when post-rewrite.ps1 is missing or pwsh is not on PATH, so the license year cache is never removed after a history rewrite. The previous hook cleared that cache directly in shell on every successful run.
Reviewed by Cursor Bugbot for commit fe4b841. Configure here.
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
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
This branch tightens serialization behavior and expands the validation around it.
Related Issue
Fixes #
Type of Change
Checklist
Note
Medium Risk
Hooks no longer run many checks locally on commit/push, so developers depend on preflight/CI unless they run it; Unity/bootstrap workflows reference scripts not yet ported and can hard-fail maintenance until those land.
Overview
Git hooks are restructured: thin
shentrypoints delegate to new*.ps1implementations and require PowerShell 7. Pre-commit drops the large bash pipeline (formatting, spelling, linters, license checks, etc.) in favor of a fast path—version sync, final newlines, LLM/meta checks, staged#regionguard—with heavier work pointed atnpm run agent:preflightand CI. Pre-push is similarly slimmed (focused C##regionchecks on pushed changes with optionalagent:preflight:fixhints). Post-rewrite now clears the license-year cache viagit rev-parse --git-pathin PowerShell. Both hook layers add cleanup/validation for stray gitignored hook artifact files.CI and automation add a substantial Unity self-hosted surface: canonical JSON for Unity versions, test-project modules, and OpenUPM integration packages; composite actions for assembly discovery, license validate/return, result verification, log dumps, and runner diagnostics; plus a runner-bootstrap workflow (with a noted gap until
maintain-windows-runner.ps1is ported). Lint workflows change behavior: doc links split internal (PR-gated) vs scheduled external lychee; asmdef lint includes**.cs; LLM lint runs on ubuntu + windows and enforces an up-to-date.llm/skills/index.md; pwsh-invocations lint covers actions and workflow run-expression length.Smaller updates: actionlint config for custom runner labels, docs issue template placeholder URL, and integration-package pins for DI test legs.
Reviewed by Cursor Bugbot for commit fe4b841. Bugbot is set up for automated code reviews on this repo. Configure here.