Skip to content

Add IL2CPP-aware resource & accessory handling#44

Merged
ifBars merged 8 commits intoifBars:stablefrom
HazDS:haz-itemfix
Feb 4, 2026
Merged

Add IL2CPP-aware resource & accessory handling#44
ifBars merged 8 commits intoifBars:stablefrom
HazDS:haz-itemfix

Conversation

@HazDS
Copy link
Collaborator

@HazDS HazDS commented Feb 4, 2026

Make accessory handling and resource registration IL2CPP-friendly. Use ReflectionUtils.TryGetFieldOrProperty and treat accessory objects as Components to reliably access fields/properties and gameObject on IL2CPP. Extend AccessoryFactory to also register typed Accessory assets (RegisterAssetForType) so Resources.Load and GetComponent lookups work. Rework RuntimeResourceRegistry: add typed registry, typed keys with NormalizeTypeName to strip Il2Cpp prefixes, improved EnsurePatched logic to find and patch Resources.Load overloads, separate IL2CPP/Mono Resources.Load prefixes (handle Il2CppSystem.Type), better logging and nullability handling. Also add Il2CppInterop import for IL2CPP builds and small refactors for clarity.

  • IL2CPP-Friendly Accessory Handling: use ReflectionUtils.TryGetFieldOrProperty to robustly access fields/properties that appear differently under IL2CPP.
  • Safe GameObject Access: treat accessory objects as Components and use component.gameObject to reliably obtain GameObjects in IL2CPP contexts.
  • AccessoryFactory: register typed Accessory assets (RegisterAssetForType) so Resources.Load and GetComponent lookups resolve correctly.
  • RuntimeResourceRegistry rework:
    • Added typed asset registry and typed keys for per-type lookups.
    • Added NormalizeTypeName to strip IL2CPP/Il2Cpp prefixes.
    • Improved EnsurePatched to locate and patch Resources.Load overloads.
    • Separate IL2CPP and Mono Resources.Load handling and support for Il2CppSystem.Type.
    • Return nullable asset types, improved logging and nullability handling.
  • IL2CPP interop: added Il2CppInterop import for IL2CPP builds.
  • Small refactors and region/organization improvements for clarity.
Author Lines Added Lines Removed
ifBars 71358 0

Make accessory handling and resource registration IL2CPP-friendly. Use ReflectionUtils.TryGetFieldOrProperty and treat accessory objects as Components to reliably access fields/properties and gameObject on IL2CPP. Extend AccessoryFactory to also register typed Accessory assets (RegisterAssetForType) so Resources.Load<T> and GetComponent lookups work. Rework RuntimeResourceRegistry: add typed registry, typed keys with NormalizeTypeName to strip Il2Cpp prefixes, improved EnsurePatched logic to find and patch Resources.Load overloads, separate IL2CPP/Mono Resources.Load prefixes (handle Il2CppSystem.Type), better logging and nullability handling. Also add Il2CppInterop import for IL2CPP builds and small refactors for clarity.
@HazDS HazDS requested a review from ifBars February 4, 2026 07:07
@coderabbitai
Copy link

coderabbitai bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

Adds a typed asset registry and typed registration APIs to RuntimeResourceRegistry, updates AccessoryFactory to register both GameObjects and typed Accessory components, refactors AvatarAccessoryPatches for IL2CPP-safe reflection/property access, and updates CI cache keys and minor docs/data timestamps.

Changes

Cohort / File(s) Summary
IL2CPP compatibility: accessory patching
S1API/Internal/Patches/AvatarAccessoryPatches.cs
Replaces direct field/property access with ReflectionUtils.TryGetFieldOrProperty(...); obtains GameObject via safe Component cast to avoid IL2CPP property lookup issues.
Accessory registration changes
S1API/Rendering/AccessoryFactory.cs
RegisterAccessory now registers the GameObject for general lookups and, when present, registers the Accessory component for typed lookups via RuntimeResourceRegistry.RegisterAssetForType(...); returns the gameObject registration result after both attempts.
Typed asset registry & Resources.Load patching
S1API/Rendering/RuntimeResourceRegistry.cs
Adds _typedAssets dictionary, RegisterAssetForType(...), typed key generation and normalization, GetRegisteredAssetForType(...), nullable-return signatures (Object?, T?), and IL2CPP-guarded typed Resources.Load interception including returning Components from GameObjects when applicable.
CI cache and workflows
.github/workflows/coverage.yml, .github/workflows/docs.yml, .github/workflows/il2cpp-build-check.yml
Bump cache key suffixes (v2→v3, v3→v4) and broaden save-cache branch conditions to include main/master/stable branches; update restore/save key usage accordingly.
Docs & data minor edits
README.md, tools/S1APICoverageAnalyzer/coverage-history.json
Updated coverage chart date label in README and two timestamp values in coverage-history.json (data-only changes).

Sequence Diagram(s)

sequenceDiagram
    participant Client as Caller
    participant AF as AccessoryFactory
    participant RRR as RuntimeResourceRegistry
    participant TA as TypedRegistry
    participant UA as UntypedRegistry
    participant Res as Resources.Load Patch

    Client->>AF: RegisterAccessory(resourcePath, gameObject)
    AF->>RRR: RegisterGameObject(resourcePath, gameObject)
    RRR->>UA: Store untyped asset
    UA-->>RRR: acknowledged
    AF->>RRR: (if Accessory component) RegisterAssetForType(resourcePath, component, Accessory)
    RRR->>TA: Store typed asset (typed key)
    TA-->>RRR: acknowledged
    RRR-->>AF: return registration result

    Note over Client,Res: Later: runtime asset lookup
    Client->>Res: Resources.Load(resourcePath) or GetRegisteredAsset<T>(resourcePath)
    Res->>RRR: Check typed registry (if type requested)
    RRR->>TA: Lookup typed key
    alt Found in typed registry
        TA-->>RRR: return Component or asset
    else Fallback
        RRR->>UA: Lookup untyped asset
        UA-->>RRR: return GameObject or asset
    end
    RRR-->>Res: return resolved asset
    Res-->>Client: return asset/component
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I hopped through keys and typed cascades,

I swapped reflections for safer braids,
GameObjects and parts now both find home,
IL2CPP no longer makes me roam,
Accessories dance — hooray, I thump and braid! 🎉

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly describes the main objective of the PR: adding IL2CPP-aware handling for resources and accessories, which is the core focus across all modified files.
Docstring Coverage ✅ Passed Docstring coverage is 93.33% which is sufficient. The required threshold is 70.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Important

Action Needed: IP Allowlist Update

If your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:

  • 136.113.208.247/32 (new)
  • 34.170.211.100/32
  • 35.222.179.152/32

Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@S1API/Rendering/RuntimeResourceRegistry.cs`:
- Around line 311-319: The IL2CPP fallback loop over _typedAssets currently
returns the first entry whose key starts with path regardless of the asset's
type; change the loop so it only assigns __result and returns false when the
asset value is compatible with the requested type (use the same check as the
Mono path, e.g. requestedType.IsInstanceOfType(kvp.Value) or
Type.IsInstanceOfType(kvp.Value)), otherwise continue searching; also guard
against nulls as appropriate so incompatible matches are skipped.
🧹 Nitpick comments (3)
S1API/Internal/Patches/AvatarAccessoryPatches.cs (1)

100-128: Minor: Material cloning occurs even when no textures are applied.

A new Material is created for every material (line 108), but materialsChanged may remain false if none of the texture property names match. This creates unnecessary allocations.

♻️ Suggested optimization
                     for (int i = 0; i < materials.Length; i++)
                     {
                         if (materials[i] == null) continue;

-                        // Clone material to avoid affecting other instances
-                        Material newMaterial = new Material(materials[i]);
-
+                        Material newMaterial = null;
+                        
                         // Apply texture replacements
                         foreach (var kvp in textureReplacements)
                         {
-                            if (newMaterial.HasProperty(kvp.Key))
+                            if (materials[i].HasProperty(kvp.Key))
                             {
+                                // Clone material only when we actually need to modify it
+                                newMaterial ??= new Material(materials[i]);
                                 newMaterial.SetTexture(kvp.Key, kvp.Value);
                                 materialsChanged = true;
                             }
                         }

-                        if (materialsChanged)
+                        if (newMaterial != null)
                         {
                             materials[i] = newMaterial;
                         }
                     }
S1API/Rendering/RuntimeResourceRegistry.cs (1)

169-173: Potential performance concern with linear scan fallback.

GetRegisteredAssetForType iterates over the entire _typedAssets dictionary when no exact match is found. With many registered assets, this becomes O(n) per lookup.

Consider if this fallback is necessary, or if a secondary index by path prefix could improve performance for high-volume scenarios.

S1API/Rendering/AccessoryFactory.cs (1)

160-163: Consider logging if type registration fails.

The result of RegisterAssetForType is not checked. If type registration fails (returns false), this could silently cause typed lookups to fail at runtime.

📝 Suggested enhancement
             // Also register the Accessory component for typed lookups like Resources.Load<Accessory>
             // This is critical for IL2CPP where GetComponent with runtime types can be unreliable
             if (accessoryComponent != null)
             {
-                RuntimeResourceRegistry.RegisterAssetForType(resourcePath, accessoryComponent, typeof(S1AvatarFramework.Accessory));
+                if (!RuntimeResourceRegistry.RegisterAssetForType(resourcePath, accessoryComponent, typeof(S1AvatarFramework.Accessory)))
+                {
+                    Logger.Warning($"Failed to register typed Accessory for '{resourcePath}'. Typed lookups may not work correctly.");
+                }
             }

Comment on lines +311 to +319
// Check for compatible types by path prefix
foreach (var kvp in _typedAssets)
{
if (kvp.Key.StartsWith(path + "|"))
{
__result = kvp.Value;
return false;
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

IL2CPP prefix returns first path match without type compatibility check.

Unlike the Mono version (which checks IsInstanceOfType), the IL2CPP fallback loop at lines 312-318 returns the first asset whose key starts with the path, regardless of whether the asset is compatible with the requested type.

This could return an incompatible asset if multiple types are registered under the same path prefix.

🔧 Suggested fix to add type compatibility check
                 // Check for compatible types by path prefix
                 foreach (var kvp in _typedAssets)
                 {
                     if (kvp.Key.StartsWith(path + "|"))
                     {
-                        __result = kvp.Value;
-                        return false;
+                        // Convert Il2CppSystem.Type to System.Type for compatibility check
+                        var systemType = Il2CppType.From(systemTypeInstance);
+                        if (systemType != null && systemType.IsInstanceOfType(kvp.Value))
+                        {
+                            __result = kvp.Value;
+                            return false;
+                        }
                     }
                 }
🤖 Prompt for AI Agents
In `@S1API/Rendering/RuntimeResourceRegistry.cs` around lines 311 - 319, The
IL2CPP fallback loop over _typedAssets currently returns the first entry whose
key starts with path regardless of the asset's type; change the loop so it only
assigns __result and returns false when the asset value is compatible with the
requested type (use the same check as the Mono path, e.g.
requestedType.IsInstanceOfType(kvp.Value) or Type.IsInstanceOfType(kvp.Value)),
otherwise continue searching; also guard against nulls as appropriate so
incompatible matches are skipped.

@ifBars ifBars added the bug Something isn't working label Feb 4, 2026
@ifBars ifBars self-assigned this Feb 4, 2026
HazDS and others added 2 commits February 4, 2026 00:35
Make accessory handling and resource registration IL2CPP-friendly. Use ReflectionUtils.TryGetFieldOrProperty and treat accessory objects as Components to reliably access fields/properties and gameObject on IL2CPP. Extend AccessoryFactory to also register typed Accessory assets (RegisterAssetForType) so Resources.Load<T> and GetComponent lookups work. Rework RuntimeResourceRegistry: add typed registry, typed keys with NormalizeTypeName to strip Il2Cpp prefixes, improved EnsurePatched logic to find and patch Resources.Load overloads, separate IL2CPP/Mono Resources.Load prefixes (handle Il2CppSystem.Type), better logging and nullability handling. Also add Il2CppInterop import for IL2CPP builds and small refactors for clarity.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In @.github/workflows/il2cpp-build-check.yml:
- Around line 57-67: The cache key for the "Restore IL2CPP Game Assemblies from
Cache" step (id: cache-il2cpp-assemblies, key: il2cpp-game-assemblies-v4-${{
steps.assembly-branch.outputs.branch }}-${{ hashFiles('S1API/S1API.csproj') }})
only includes branch + project hash so it won't change when the external IL2CPP
assemblies repo updates; add a preliminary step that fetches the assemblies repo
metadata (e.g., run a shallow git ls-remote or actions/checkout for that repo)
and emit its commit SHA as an output (e.g., assemblies_repo_sha), then include
that SHA in the cache key and restore-keys so the key becomes
il2cpp-game-assemblies-v4-${{ steps.assembly-branch.outputs.branch }}-${{
steps.fetch-assemblies.outputs.assemblies_repo_sha }}-${{
hashFiles('S1API/S1API.csproj') }} and update restore-keys accordingly to allow
prefix fallback.
- Around line 400-407: The save step "Save IL2CPP Game Assemblies to Cache"
currently runs on pushes because steps.cache-il2cpp-assemblies.outputs.cache-hit
is unset; add a guard to the step's if expression to only attempt save when the
project file hash exists, e.g. append && hashFiles('S1API/S1API.csproj') != ''
to the existing if so the action skips saves when the assemblies/project file is
missing (prevents "Unable to reserve cache" and fork/GITHUB_TOKEN failures);
update the if on the Save IL2CPP Game Assemblies to include this hashFiles guard
referencing the same 'S1API/S1API.csproj' used in the key.

Comment on lines +57 to +67
# v4 suffix allows cache invalidation by bumping version
# Only use cache for PR events to avoid stale assemblies after merges
- name: Restore IL2CPP Game Assemblies from Cache
id: cache-il2cpp-assemblies
if: github.event_name == 'pull_request'
uses: actions/cache/restore@v4
with:
path: S1API/ScheduleOneAssemblies
key: il2cpp-game-assemblies-v3-${{ steps.assembly-branch.outputs.branch }}-${{ hashFiles('S1API/S1API.csproj') }}
key: il2cpp-game-assemblies-v4-${{ steps.assembly-branch.outputs.branch }}-${{ hashFiles('S1API/S1API.csproj') }}
restore-keys: |
il2cpp-game-assemblies-v3-${{ steps.assembly-branch.outputs.branch }}-
il2cpp-game-assemblies-v4-${{ steps.assembly-branch.outputs.branch }}-
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat .github/workflows/il2cpp-build-check.yml | head -100

Repository: ifBars/S1API

Length of output: 4632


🌐 Web query:

GitHub Actions cache actions/cache/save immutability can overwrite existing key v4

💡 Result:

No—with actions/cache/save@v4 you cannot overwrite an existing cache entry for the same key. GitHub Actions caches are immutable: if a cache with that key already exists, the save step will not create a new cache (so the existing cache remains unchanged). You must use a new key to save updated contents. [1], [2]

Common workaround (“update” behavior): save with a unique key per run (e.g., include github.run_id or a timestamp) and use restore-keys to restore the closest prior cache. This creates new cache entries and can increase cache/quota usage. [3]


🌐 Web query:

GitHub Actions cache key strategy best practices including external repository revision

💡 Result:

Cache key best practices (GitHub Actions)

  • Key should change when inputs that affect the cache change: typically include OS + toolchain/runtime version + dependency lockfile hash (e.g., hashFiles('**/package-lock.json')). GitHub explicitly recommends hashFiles to create new caches when dependencies change. [1]
  • Use restore-keys for “closest match” fallbacks (ordered most-specific → least-specific) to improve hit rate while still allowing a full re-save on a miss. GitHub documents the prefix-match behavior for key and restore-keys. [2]
  • Avoid over-busting: don’t include highly volatile values (timestamps, run IDs) unless you intentionally want a near-zero hit rate.
  • Remember cache compatibility constraints: caches are also stamped by a cache version (derived from paths + compression tool), so cross-OS restores may not work as you expect. [1][2]
  • Stay within limits/eviction realities (10GB default repo limit; caches not accessed for 7 days are removed), and design keys to reduce “cache thrash.” [1][2]

Including an external repository revision in the cache key

Because cache keys can include arbitrary command output, you can embed the external repo’s exact commit SHA in the key. [1]

Pattern A — external repo checked out into a subdirectory

jobs:
  build:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4

      - name: Checkout external repo
        uses: actions/checkout@v4
        with:
          repository: org/external-repo
          path: _external
          fetch-depth: 1

      - name: Get external repo revision
        id: extrev
        run: echo "sha=$(git -C _external rev-parse HEAD)" >> "$GITHUB_OUTPUT"

      - uses: actions/cache@v4
        with:
          path: |
            ~/.npm
            node_modules
          key: >
            npm-${{ runner.os }}-node20-
            ext-${{ steps.extrev.outputs.sha }}-
            lock-${{ hashFiles('**/package-lock.json') }}
          restore-keys: |
            npm-${{ runner.os }}-node20-ext-${{ steps.extrev.outputs.sha }}-
            npm-${{ runner.os }}-node20-

Why this works: when the external repo updates (different SHA), you naturally get a new cache namespace; when it doesn’t, you keep reusing the same cache. Using a fallback restore-keys keeps performance decent if only the lockfile changes. [1][2]

Pattern B — submodules

If you use git submodules, capture the pinned submodule commit(s) (e.g., git submodule status --recursive) and hash it into the key (same idea: derive a deterministic “external revision fingerprint,” then include it).

Source notes: GitHub’s cache docs show (1) using hashFiles for dependency-sensitive keys and (2) using command outputs as part of keys; they also document restore-keys prefix matching and general cache behavior/limits. [1][2]

Sources:
[1] actions/cache documentation (creating keys; command output in keys; cache version notes) (github.com)
[2] GitHub Docs — “Caching dependencies to speed up workflows” (key matching; restore-keys; limits/eviction) (docs.github.com)

Citations:


🏁 Script executed:

sed -n '395,410p' .github/workflows/il2cpp-build-check.yml

Repository: ifBars/S1API

Length of output: 991


🏁 Script executed:

# Also check if there's any cache save step in the file
rg -n "cache/save" .github/workflows/il2cpp-build-check.yml -A 8

Repository: ifBars/S1API

Length of output: 683


Cache key won't refresh when IL2CPP assemblies repo changes.

The cache key (lines 65, 407) depends only on branch + S1API.csproj hash. Because GitHub Actions caches are immutable—cache/save@v4 cannot overwrite an existing key—updates to the IL2CPP assemblies repo won't invalidate the cache. The stale cache will persist even after the save step runs.

To fix this, include the IL2CPP assemblies repo's commit SHA in the cache key. Fetch the repo metadata before the cache restore step, output its SHA, and embed it in the key. This ensures a new cache namespace whenever the external repo updates, while restore-keys prefix matching provides a fallback for better hit rates.

🔧 Example approach (fetch repo SHA, include in key + restore-keys)
+      - name: Get IL2CPP Assemblies Revision
+        id: il2cpp-assemblies-meta
+        run: |
+          git ls-remote --exit-code ${{ secrets.IL2CPP_ASSEMBLIES_REPO }} ${{ steps.assembly-branch.outputs.branch }} | awk '{print $1}' | xargs -I {} echo "sha={}" >> $GITHUB_OUTPUT || echo "sha=unknown" >> $GITHUB_OUTPUT

       - name: Restore IL2CPP Game Assemblies from Cache
         id: cache-il2cpp-assemblies
         if: github.event_name == 'pull_request'
         uses: actions/cache/restore@v4
         with:
           path: S1API/ScheduleOneAssemblies
-          key: il2cpp-game-assemblies-v4-${{ steps.assembly-branch.outputs.branch }}-${{ hashFiles('S1API/S1API.csproj') }}
+          key: il2cpp-game-assemblies-v4-${{ steps.assembly-branch.outputs.branch }}-${{ steps.il2cpp-assemblies-meta.outputs.sha }}-${{ hashFiles('S1API/S1API.csproj') }}
           restore-keys: |
             il2cpp-game-assemblies-v4-${{ steps.assembly-branch.outputs.branch }}-
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# v4 suffix allows cache invalidation by bumping version
# Only use cache for PR events to avoid stale assemblies after merges
- name: Restore IL2CPP Game Assemblies from Cache
id: cache-il2cpp-assemblies
if: github.event_name == 'pull_request'
uses: actions/cache/restore@v4
with:
path: S1API/ScheduleOneAssemblies
key: il2cpp-game-assemblies-v3-${{ steps.assembly-branch.outputs.branch }}-${{ hashFiles('S1API/S1API.csproj') }}
key: il2cpp-game-assemblies-v4-${{ steps.assembly-branch.outputs.branch }}-${{ hashFiles('S1API/S1API.csproj') }}
restore-keys: |
il2cpp-game-assemblies-v3-${{ steps.assembly-branch.outputs.branch }}-
il2cpp-game-assemblies-v4-${{ steps.assembly-branch.outputs.branch }}-
# v4 suffix allows cache invalidation by bumping version
# Only use cache for PR events to avoid stale assemblies after merges
- name: Get IL2CPP Assemblies Revision
id: il2cpp-assemblies-meta
run: |
git ls-remote --exit-code ${{ secrets.IL2CPP_ASSEMBLIES_REPO }} ${{ steps.assembly-branch.outputs.branch }} | awk '{print $1}' | xargs -I {} echo "sha={}" >> $GITHUB_OUTPUT || echo "sha=unknown" >> $GITHUB_OUTPUT
- name: Restore IL2CPP Game Assemblies from Cache
id: cache-il2cpp-assemblies
if: github.event_name == 'pull_request'
uses: actions/cache/restore@v4
with:
path: S1API/ScheduleOneAssemblies
key: il2cpp-game-assemblies-v4-${{ steps.assembly-branch.outputs.branch }}-${{ steps.il2cpp-assemblies-meta.outputs.sha }}-${{ hashFiles('S1API/S1API.csproj') }}
restore-keys: |
il2cpp-game-assemblies-v4-${{ steps.assembly-branch.outputs.branch }}-
🤖 Prompt for AI Agents
In @.github/workflows/il2cpp-build-check.yml around lines 57 - 67, The cache key
for the "Restore IL2CPP Game Assemblies from Cache" step (id:
cache-il2cpp-assemblies, key: il2cpp-game-assemblies-v4-${{
steps.assembly-branch.outputs.branch }}-${{ hashFiles('S1API/S1API.csproj') }})
only includes branch + project hash so it won't change when the external IL2CPP
assemblies repo updates; add a preliminary step that fetches the assemblies repo
metadata (e.g., run a shallow git ls-remote or actions/checkout for that repo)
and emit its commit SHA as an output (e.g., assemblies_repo_sha), then include
that SHA in the cache key and restore-keys so the key becomes
il2cpp-game-assemblies-v4-${{ steps.assembly-branch.outputs.branch }}-${{
steps.fetch-assemblies.outputs.assemblies_repo_sha }}-${{
hashFiles('S1API/S1API.csproj') }} and update restore-keys accordingly to allow
prefix fallback.

Comment on lines 400 to +407
# Save cache even if build fails (to enable beta cache priming)
# Only save cache for PR events
# Save on PR events AND on pushes to main branches (to update cache after assembly updates)
- name: Save IL2CPP Game Assemblies to Cache
uses: actions/cache/save@v4
if: always() && github.event_name == 'pull_request' && steps.cache-il2cpp-assemblies.outputs.cache-hit != 'true'
if: always() && (github.event_name == 'pull_request' || (github.event_name == 'push' && (github.ref == 'refs/heads/master' || github.ref == 'refs/heads/main' || github.ref == 'refs/heads/stable' || github.ref == 'refs/heads/npc-prefabs'))) && steps.cache-il2cpp-assemblies.outputs.cache-hit != 'true'
with:
path: S1API/ScheduleOneAssemblies
key: il2cpp-game-assemblies-v3-${{ steps.assembly-branch.outputs.branch }}-${{ hashFiles('S1API/S1API.csproj') }}
key: il2cpp-game-assemblies-v4-${{ steps.assembly-branch.outputs.branch }}-${{ hashFiles('S1API/S1API.csproj') }}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

GitHub Actions fork pull request cache write permissions actions/cache/save

💡 Result:

  • In workflows triggered by pull_request from a fork, GitHub forces GITHUB_TOKEN to be read-only, even if you set permissions: actions: write (public forks are a special case). That means actions/cache/save can’t write/update caches in the base repo in that context. [1]
  • For private/internal repos, you can opt into allowing fork PRs to receive a write token via Settings → Actions → General → “Fork pull request workflows” → “Send write tokens to workflows from pull requests”; otherwise fork PRs run read-only with no secrets. [2]
  • If you switch to pull_request_target to get a write-capable token, GitHub explicitly warns this runs in the base repo security context (write token + secrets) and caches share base-branch scope—so you should not save caches if untrusted PR code could have modified them (cache poisoning risk). [3]
  • Practical patterns:
    • Don’t call actions/cache/save on fork PRs; only restore (or just rely on actions/cache’s normal behavior where save won’t be possible). [4]
    • Save/update caches on trusted events like push to default branch, or a separate post-merge workflow.
    • If using pull_request_target, avoid checking out/running untrusted PR code in the same job that can write caches. [3]

Sources:
[1] https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/ (github.blog)
[2] https://docs.github.com/github/administering-a-repository/managing-repository-settings/disabling-or-limiting-github-actions-for-a-repository (docs.github.com)
[3] https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/ (securitylab.github.com)
[4] https://github.com/marketplace/actions/cache (github.com)

Citations:


🌐 Web query:

GitHub Actions actions/cache/save behavior existing key duplicate

💡 Result:

actions/cache/save (and actions/cache) will not overwrite an existing cache entry for the same (key + version + branch scope). If the key already exists, no new cache is created (caches are effectively immutable); instead you must save under a new key. [1][2]

If two jobs/runs try to save the same key around the same time (or one already saved it), the save can fail with an error like “Unable to reserve cache… Cache already exists.” [3]

Practical implications / fixes

  • To avoid duplicates: run restore first and only save when cache-hit != 'true'. [1]
  • To “update” a cache: change the key (e.g., include hashFiles(...), a build id, date, etc.), because you can’t modify an existing cache. [2]

Sources:
[1] actions/cache README (GitHub) (github.com)
[2] GitHub Docs: caching dependencies / immutability (“You cannot change the contents of an existing cache…”) (docs.github.com)
[3] actions/toolkit issue showing “Unable to reserve cache… Cache already exists.” (github.com)

Citations:


🏁 Script executed:

find . -name "il2cpp-build-check.yml" -type f

Repository: ifBars/S1API

Length of output: 98


🏁 Script executed:

cat -n .github/workflows/il2cpp-build-check.yml | head -450 | tail -100

Repository: ifBars/S1API

Length of output: 5820


🏁 Script executed:

cat -n .github/workflows/il2cpp-build-check.yml | head -250 | tail -150

Repository: ifBars/S1API

Length of output: 8269


🏁 Script executed:

cat -n .github/workflows/il2cpp-build-check.yml | grep -A 5 -B 5 "cache/restore\|Restore IL2CPP"

Repository: ifBars/S1API

Length of output: 903


🏁 Script executed:

cat -n .github/workflows/il2cpp-build-check.yml | grep -A 20 "Fetch Game Version\|il2cpp-scheduleone-assemblies" | head -60

Repository: ifBars/S1API

Length of output: 3980


🏁 Script executed:

cat -n .github/workflows/il2cpp-build-check.yml | head -70 | tail -30

Repository: ifBars/S1API

Length of output: 1814


🌐 Web query:

GitHub Actions undefined step outputs empty string conditional comparison

💡 Result:

In GitHub Actions expressions, a step output is always treated as a string (steps.<id>.outputs.<name>), and if that output was never set, it effectively behaves like an empty string ('') in later expressions/conditionals. (docs.github.com)

Practical ways to handle “undefined output”

Check for non-empty (recommended):

- if: ${{ steps.build.outputs.image_tag != '' }}
  run: echo "tag is set"

Falsy values in if: include "" and null, so this also works:

- if: ${{ steps.build.outputs.image_tag }}
  run: echo "tag is set"

(docs.github.com)

Provide a default (sentinel) value:

env:
  IMAGE_TAG: ${{ steps.build.outputs.image_tag || 'UNSET' }}

Then test env.IMAGE_TAG == 'UNSET'. (This pattern is commonly used because empty/undefined frequently collapse to "".) (docs.github.com)

Common gotcha: comparing to booleans

Even if you “set” true/false, outputs arrive as strings, so compare to 'true' / 'false':

- if: ${{ steps.prev.outputs.ok == 'true' }}
  run: echo "ok"

(docs.github.com)

Citations:


🏁 Script executed:

cat -n .github/workflows/il2cpp-build-check.yml | sed -n '1,50p'

Repository: ifBars/S1API

Length of output: 2067


Add guard to prevent cache save attempts on push events.

On push events, the Restore IL2CPP Game Assemblies from Cache step doesn't run (it's gated to PRs only), so steps.cache-il2cpp-assemblies.outputs.cache-hit is never set. This means the condition on line 404 evaluates to true and the save action attempts to run on every push. When S1API/S1API.csproj hasn't changed between pushes, the cache key remains identical, and actions/cache/save will fail with "Unable to reserve cache" because cache entries are immutable.

For fork PRs, the read-only GITHUB_TOKEN prevents the save from writing, though the action will still attempt and fail.

Add the hashFiles guard to prevent unnecessary save attempts when assemblies are missing and to avoid collisions:

Recommended guard
-        if: always() && (github.event_name == 'pull_request' || (github.event_name == 'push' && (github.ref == 'refs/heads/master' || github.ref == 'refs/heads/main' || github.ref == 'refs/heads/stable' || github.ref == 'refs/heads/npc-prefabs'))) && steps.cache-il2cpp-assemblies.outputs.cache-hit != 'true'
+        if: always() && (github.event_name == 'pull_request' || (github.event_name == 'push' && (github.ref == 'refs/heads/master' || github.ref == 'refs/heads/main' || github.ref == 'refs/heads/stable' || github.ref == 'refs/heads/npc-prefabs'))) && steps.cache-il2cpp-assemblies.outputs.cache-hit != 'true' && hashFiles('S1API/ScheduleOneAssemblies/**/*.dll') != ''

Alternatively, restore the cache on push events as well (remove the if: github.event_name == 'pull_request' gate on line 61) to ensure cache-hit is always populated and reliable.

🤖 Prompt for AI Agents
In @.github/workflows/il2cpp-build-check.yml around lines 400 - 407, The save
step "Save IL2CPP Game Assemblies to Cache" currently runs on pushes because
steps.cache-il2cpp-assemblies.outputs.cache-hit is unset; add a guard to the
step's if expression to only attempt save when the project file hash exists,
e.g. append && hashFiles('S1API/S1API.csproj') != '' to the existing if so the
action skips saves when the assemblies/project file is missing (prevents "Unable
to reserve cache" and fork/GITHUB_TOKEN failures); update the if on the Save
IL2CPP Game Assemblies to include this hashFiles guard referencing the same
'S1API/S1API.csproj' used in the key.

@ifBars ifBars merged commit 5e1b97d into ifBars:stable Feb 4, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants