chore: harden code-standards skill from PR review feedback

[cyaiox]

Pull Request Description

What does this PR change?

Turns recurring code-review feedback into durable, documented project standards, and fixes why the code-standards skill was going unread. Two parts:

1. Capture the rules. Distills the repeated review comments from PRs #9043, #8393, and #7347 into the code-standards skill as anti-patterns #8–20:

• Game Object Creation #8 Comments that narrate authoring/testing/a-good-name
• Transform Parenting #9 Leaking a class's responsibility into its consumer
• Transform Parameters Application #10 Partially-initialized instances (subscribe-once-in-ctor)
• Game Object Destruction #11 Names that hide narrowed/implicit behavior
• Create a mechanism to initialize and dispose components #12 Booleans that conflate "absent" with a state
• Scene Visualisation #13 Side-effects vs return values; deps hidden behind default params
• Budgeting #14 Caches / shared GPU-texture ownership contracts
• Streamable Loading #15 Scattered string-typing/if-else instead of a domain type (REnum)
• Streamable Optimizations #16 Bypassing an existing abstraction (IMessagePipe not raw IDataPipe)
• Prioritisation #17 Thread-safety left implicit on shared mutable state
• Realms Loading #18 Linear scans (O(n) .Exists) & redundant buffers in hot paths
• Scene Runtime - Introduce the scenes concept #19 Dead code, redundant flags, dead stores, duplicated logic
• Create Editor View For Entities #20 Hiding failures (band-aid recovery, stacked catches, silent lookups)

2. Fix why the skill was never read. CLAUDE.md's Startup section implied skills auto-load ("do not manually read them"), but a skill's body is only in context once the Skill tool is invoked — so the standards never reached context. This rewrites the wording to mandate invoking Skill(code-standards) before any .cs edit, and tightens the skill description to drop the "non-trivial" escape hatch.

Docs/agent-tooling only — no runtime code changes.

Test Instructions

Not an in-world change; nothing to run in the explorer. Verify directly:

1. Open .claude/skills/code-standards/SKILL.md — confirm anti-patterns Game Object Creation #8–20 read clearly and each traces to real review feedback.
2. Open CLAUDE.md Startup — confirm it no longer claims skills auto-load and names Skill(code-standards) as a pre-edit step, and the skill description no longer contains the "non-trivial" escape hatch.

Prerequisites

• None — documentation-only change.

Test Steps

1. Review the two changed files listed above.

Additional Testing Notes

• No .cs changed, so the ReSharper Stop lint hook inspects nothing.

Quality Checklist

• Changes have been tested locally
• Documentation has been updated (if required)
• Performance impact has been considered
• For SDK features: Test scene is included

Code Review Reference

Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.

[github-actions]

Build skipped — no changes detected under Explorer/.

[github-actions]

Slack notification sent to #explorer-ext-contributions for external review.
To re-send, delete this comment and re-add the ext-contribution label.

[decentraland-bot]

PR Review: chore: harden code-standards skill from PR review feedback

STEP 2 — Root-cause check: ✅ PASS

The problem is real and well-diagnosed: the code-standards skill was effectively dead because CLAUDE.md claimed skills auto-load ("do not manually read them"), when in fact a skill's full body only enters context after explicit Skill() invocation. The fix addresses the root cause with a three-layer approach (corrected documentation + tightened skill description + deterministic hook), each targeting a distinct failure mode. This is defense-in-depth, not over-engineering.

STEP 3 — Design & integration: ✅ PASS

This PR is docs/agent-tooling only — no runtime C# code, no ECS systems, no long-lived units, no persistent state. No owner search required.

The 13 new anti-patterns (#8–20) are well-scoped:

• #8 vs existing CLAUDE.md "comments narrating caller behavior" — overlap is acknowledged with an explicit cross-reference; #8 broadens scope to authoring/testing narration and name-restating, which are distinct failure modes. Well-handled.
• #9 (leaking responsibility) vs #16 (bypassing abstraction) — related but distinct: #9 is about data-decision ownership, #16 is about using the wrong API layer. Clear separation.
• #13 (side-effects + hidden default params) — bundles two concerns under "hidden wiring" theme. Acceptable.
• #19 (dead code) — broad catch-all for cleanup patterns. Reads more like a checklist, but reasonable for this category.

No true duplicates among the 13 new entries.

STEP 4 — Member audit: N/A

No C# members added or changed.

STEP 5 — Line-level review

See inline comments below (2 × P2).

Security review

No critical or high security issues. The hook script:

• ✅ Degrades safely when jq is unavailable (command -v jq || exit 0)
• ✅ Never blocks a tool call (always exits 0)
• ✅ No command injection — file_path is only used in a case glob, not interpolated into shell commands
• ✅ No secrets or credentials
• ⚠️ session_id used unsanitized in sentinel path (see inline comment — P2)

STEP 6 — Complexity: SIMPLE

4 files, +174/−3 lines, all documentation and agent tooling. No runtime code, no ECS, no async, no assembly changes.

STEP 7 — QA: NO

No user-facing behavior affected. No runtime code changes. Nothing runs in the Unity player.

STEP 8 — Non-blocking warnings: None

Main.unity not modified.

---

REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Docs and agent-tooling changes only (skill markdown, CLAUDE.md wording, bash hook, settings.json config) — no runtime C# code.
QA_REQUIRED: NO

---

Reviewed by Jarvis 🤖 · Requested by unknown via Slack

[decentraland-bot]

[P2] Skill description says "reading" but enforcement only covers writing/modifying. The description says "Use BEFORE reading, writing, or modifying ANY .cs file" but both the CLAUDE.md update and the PreToolUse hook only fire on Edit|Write|MultiEdit — not Read. Loading 123 lines of anti-patterns before every .cs read wastes context window for zero benefit.

Drop "reading" to align with the enforcement mechanisms.

Suggested change

	user-invocable: false
	description: "C# naming conventions, member ordering, formatting rules, nullable reference types, memory/GC rules, test patterns, and PR standards. Use BEFORE writing or modifying ANY .cs file in this Unity project — invoke it before the first Edit/Write on production C#, not only for large changes. Applies to ECS systems, controllers, tests, utilities, and plugins."

[decentraland-bot]

[P2] Unsanitized session_id in sentinel file path. The session_id value from stdin JSON is interpolated directly into a file path. While session_id is set by the Claude Code runtime (not user input), defense-in-depth says to sanitize it — a crafted value with ../ could create/truncate files outside /tmp. Strip non-path-safe characters after extraction:

Suggested change

	esac
	session_id="$(printf '%s' "$input" | jq -r '.session_id // "unknown"' 2>/dev/null || echo unknown)"
	session_id="${session_id//[^a-zA-Z0-9_-]/}"
	sentinel="${TMPDIR:-/tmp}/claude-code-standards-reminder-${session_id}"

[cyaiox]

@claude review

[claude]

Claude finished @cyaiox's task in 4m 4s —— View job

---

PR Review: chore: harden code-standards skill from PR review feedback

• Load context (CLAUDE.md, docs/README.md, diff)
• Root-cause check (STEP 2)
• Design & integration review (STEP 3)
• Member audit (STEP 4)
• Line-level review (STEP 5)
• Complexity & QA assessment (STEPS 6-8)
• Post final verdict (STEP 9)

---

STEP 2 — Root-cause check: ✅ PASS

The root cause is correctly identified: CLAUDE.md falsely claimed skills auto-load ("do not manually read them"), when a skill's full instructions only enter context after an explicit Skill() invocation. Every .cs edit in this session ran without the anti-patterns in context.

The fix addresses the cause, not a symptom, via three independent layers:

1. Correct the CLAUDE.md claim about auto-loading → explicit Skill(code-standards) mandate
2. Tighten the skill description so the model sees the intent from the index alone
3. Add a deterministic PreToolUse hook that fires on the first .cs Edit/Write/MultiEdit per session as a backstop

No null-check workaround, no symptom masking. Genuine root-cause repair.

---

STEP 3 — Design & integration: ✅ PASS

All 4 changed files are docs/agent-tooling — no runtime C# code, no ECS systems, no long-lived units. No owner search required.

The bot review P2 issues (from the prior decentraland-bot pass) were both addressed in commit 841dd13f:

• P2-1 (skill description said "reading"): Fixed — the committed SKILL.md description now reads "Use BEFORE writing, modifying, or reviewing ANY .cs file" (no "reading").
• P2-2 (unsanitized session_id in sentinel path): Fixed — the committed hook script includes session_id="${session_id//[^a-zA-Z0-9_-]/}" after extraction.

The three-layer design (description + CLAUDE.md wording + hook) is correct. Each layer targets a distinct failure mode:

• Description: visible in the skill index without invocation; primes the model to invoke early
• CLAUDE.md: explicit duty at session startup
• Hook: deterministic backstop; fires once per session on first .cs edit regardless of what the model remembered

No true design issues remain.

---

STEP 4 — Member audit: N/A

No C# members added or changed.

---

STEP 5 — Line-level review

No blocking issues. Two minor observations:

[Non-blocking] scripts/hooks/remind-code-standards.sh:39 — final jq output lacks a guaranteed exit-0 escape

jq -cn --arg ctx "$reminder" \
    '{hookSpecificOutput: {hookEventName: "PreToolUse", additionalContext: $ctx}}'

The PR description explicitly promises "it never blocks the tool". The script already exits 0 on all early-exit paths and the jq binary was confirmed present at line 12. However, a jq failure here (however improbable: binary fault, OOM) would exit non-zero. To make the never-block guarantee unconditional, add || exit 0:

jq -cn --arg ctx "$reminder" \
    '{hookSpecificOutput: {hookEventName: "PreToolUse", additionalContext: $ctx}}' || exit 0

This is a one-character fix against an effectively-impossible failure mode, but the PR makes an explicit safety promise so the code should match it exactly.

[Non-blocking] CLAUDE.md:6 — "before any Read/Edit/Write" vs hook fires on Edit/Write only

The line reads: — before any Read/Edit/Write on production code —. The hook fires only on Edit|Write|MultiEdit, and the skill description says "before writing, modifying, or reviewing". Adding "Read" to the CLAUDE.md instruction is intentional (invoke the skill before you even start reading files, not just before you write), but readers may expect the hook to enforce it on Reads too. A parenthetical would make the design intent clear:

— before any Edit/Write on production code (invoke early, ideally before even reading files) —

Neither of these is a blocker — the committed code is functionally correct.

Review of new anti-patterns #8–20: All 13 patterns are well-scoped, trace to real review feedback, and are consistent with the project's standards. The internal API references (IMessagePipe, LivekitAddress, REnum, etc.) are appropriate for an in-repo AI tooling document. No anti-pattern entry itself violates the standards it's teaching.

---

STEP 6 — Complexity: SIMPLE

4 files, docs and agent-tooling only. No runtime code, no ECS, no async, no assembly changes.

STEP 7 — QA: NO

No user-facing behavior affected. Nothing runs in the Unity player.

STEP 8 — Non-blocking warnings: None

Main.unity not modified.

---

REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Docs and agent-tooling changes only (skill markdown, CLAUDE.md wording, bash hook, settings.json config) — no runtime C# code.
QA_REQUIRED: NO

---

---

💰 Review cost: $0.51 · claude-sonnet-4-6 · 15 turns