Skip to content

fix: two findings from the agent-skill audit - #69

Open
ziyiunity wants to merge 1 commit into
mainfrom
fix/two-audit-findings
Open

fix: two findings from the agent-skill audit#69
ziyiunity wants to merge 1 commit into
mainfrom
fix/two-audit-findings

Conversation

@ziyiunity

Copy link
Copy Markdown
Contributor

What

Two findings from Zack Asofsky's agent-skill-auditor, both verified before fixing.

1. localization ships code that contradicts its own guidance

SKILL.md prescribes:

Robust Mapping Strategy: When mapping existing UI text to keys, sort keys by string length (descending) and match longest strings first. This prevents short strings (like "NO") from matching parts of longer sentences.

resources/L10nBatchProcessor.cs iterated the mapping Dictionary in enumeration order with a bare Contains, and no sort.

It is worse than a missing optimization, because of the break:

foreach (var kvp in mapping)          // unspecified order
{
    if (!current.Contains(kvp.Key)) continue;
    ...
    matched = true;
    break;                            // first hit wins
}

A label reading "NO ITEMS FOUND" binds to a "NO" key and stops looking. It is also flagged matched, so it never appears in the unmatched list the method returns for exactly this purpose — a silent mis-binding rather than a visible failure. And since Dictionary enumeration order is unspecified, whether it happens at all varies between runs.

Fixed by sorting longest-first, once, outside the per-label loop.

2. physics-3d-collision's only reference file restated the spine

All 23 rows of references/troubleshooting.md's symptom table are already covered by SKILL.md — its eleven Fast-Path sections and twelve checklist steps — in different words. Spot-checked before removing:

Table row Already in SKILL.md
No callback, no Rigidbody Step 1 — Rigidbody rule
OnCollisionEnter never fires on a player character Fast-Path 5 and CharacterController Exception
Two kinematic objects never interact Fast-Path 3 and Step 2 — Interaction matrix
Fast bullet passes through wall Step 10 — Tunneling
Objects launch apart on the first frame Step 11 — Overlapping colliders at simulation start
All physics frozen Fast-Path 7
MeshCollider produces no callbacks Step 7 — MeshCollider rules

The table is removed. The seven Unity Manual links in the file's Resources section are kept — they appear nowhere else, and linking documentation rather than restating it is the direction we want. SKILL.md's pointer is retitled from "Troubleshooting & Resources" to "Resources" to match.

Scope

One behavioral fix, one redundancy removal. No skill trigger or description changes.

localization: the shipped batch processor contradicted its own guidance.
SKILL.md prescribes matching the longest key first so a short string like
"NO" cannot match inside a longer sentence, and the processor iterated the
mapping Dictionary in enumeration order with a bare Contains. Because the
loop breaks on the first hit and marks the label matched, a label reading
"NO ITEMS FOUND" bound to a "NO" key and never appeared in the unmatched
report -- a silent mis-binding, and an irreproducible one, since Dictionary
enumeration order is unspecified. Keys are now sorted longest-first, once.

physics-3d-collision: the only reference file restated the spine. All 23
rows of its troubleshooting table are already covered by SKILL.md's eleven
Fast-Path sections and twelve checklist steps, in different words. The
table is removed and the seven Unity Manual links, which appear nowhere
else, are kept.

Both reported by Zack Asofsky via the agent-skill-auditor.
@ziyiunity
ziyiunity requested a review from a team as a code owner September 10, 2026 14:05

@UnityZappy UnityZappy left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM, thank you for updating these skills!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants