From 59955a145c74c62e3d14dbb3b15d9b7cfd140247 Mon Sep 17 00:00:00 2001 From: Gabriel Diaz Date: Fri, 10 Jul 2026 11:31:48 +0200 Subject: [PATCH 1/3] chore: harden code-standards skill from PR review feedback Distill recurring review comments from PRs #9043, #8393, and #7347 into the code-standards skill as anti-patterns #8-20 (comment discipline, responsibility placement, no partial init, names that encode behavior, absent-vs-false booleans, domain-types-over-string-typing, not bypassing abstractions, thread-safety, algorithmic complexity, dead code, and hiding-failures). Also fix the delivery gap that let these rules go unread: CLAUDE.md's Startup section wrongly implied skills auto-load, so the code-standards skill body (only surfaced on Skill-tool invocation) never reached context. Rewrite the wording to mandate invoking it before any .cs edit, tighten the skill description to drop the "non-trivial" escape hatch, and add a PreToolUse hook that reminds once per session on .cs edits. --- .claude/settings.json | 11 +++ .claude/skills/code-standards/SKILL.md | 125 ++++++++++++++++++++++++- CLAUDE.md | 4 +- scripts/hooks/remind-code-standards.sh | 37 ++++++++ 4 files changed, 174 insertions(+), 3 deletions(-) create mode 100755 scripts/hooks/remind-code-standards.sh diff --git a/.claude/settings.json b/.claude/settings.json index 4e6f23fe1e7..9f086f9272e 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -1,5 +1,16 @@ { "hooks": { + "PreToolUse": [ + { + "matcher": "Edit|Write|MultiEdit", + "hooks": [ + { + "type": "command", + "command": "bash scripts/hooks/remind-code-standards.sh" + } + ] + } + ], "Stop": [ { "hooks": [ diff --git a/.claude/skills/code-standards/SKILL.md b/.claude/skills/code-standards/SKILL.md index 8c950a69b93..a8031c29df1 100644 --- a/.claude/skills/code-standards/SKILL.md +++ b/.claude/skills/code-standards/SKILL.md @@ -1,6 +1,6 @@ --- name: code-standards -description: "C# naming conventions, member ordering, formatting rules, nullable reference types, memory/GC rules, test patterns, and PR standards. Use when writing, reviewing, or modifying non-trivial C# changes in this Unity project — applies to ECS systems, controllers, tests, utilities, and plugins." +description: "C# naming conventions, member ordering, formatting rules, nullable reference types, memory/GC rules, test patterns, and PR standards. Use BEFORE reading, 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." user-invocable: false --- @@ -289,6 +289,129 @@ A loop that re-queues unresolved items will spin forever when the upstream sourc If `X` does nothing useful without `Y`, and there is no second consumer of `X`, merge them. Splits must pay for themselves in polymorphism, reuse, or test isolation. +### 8. Comments that narrate authoring, testing, or a good name + +A comment must state only what the annotated code itself does or guarantees. Delete: + +- **Authoring / testing narration** — how the code was written or must be verified. `// NOTE: the visual result must be verified in the Unity editor — it cannot be validated headlessly.` is a classic AI tell; it describes your workflow, not the code. +- **Restatements of a good name** — if the member name already says it, the comment is noise. +- **Caller / external-behavior narration** — see the CLAUDE.md anti-pattern "Comments that narrate caller/external behavior". A comment must not describe what other scopes do with the result. + +When a comment *is* warranted, state the real non-obvious invariant explicitly: + +```csharp +// WRONG — narrates the author's process +// The label size and position were tuned by eye in the editor. + +// RIGHT — states an invariant the reader cannot infer from the code +// Not thread-safe — main-thread only (renders into a shared RenderTexture). +``` + +### 9. Leaking a class's responsibility into its consumer + +The class that owns the data owns the decisions about it. If a player owns the video texture, it also owns *which* texture to surface (including a "camera-off" placeholder) — the consuming ECS system must not reach in, pick the texture, and blit it. Push the decision behind the owner's API and pass real collaborators into the constructor directly, rather than reconstructing or re-deciding them in the consumer. + +```csharp +// WRONG — system reaches into the player and decides which texture to show +Texture tex = player.IsMuted && placeholder != null + ? placeholder.TextureFor(player.StreamerName) + : player.LastTexture(); +Graphics.Blit(tex, target); + +// RIGHT — the player owns the decision; the system just consumes it +Graphics.Blit(player.CurrentTexture, target); +``` + +### 10. Partially-initialized instances + +A constructor returns a fully-valid object or throws. Do not scatter `EnsureX()` / `initialized` / `creationFailed` guards that let a half-built instance exist and degrade later — that shatters one invariant across many call sites. Require mandatory dependencies as non-null constructor arguments and validate invariants at creation time. + +```csharp +// WRONG — instance can exist half-built; every method must re-check +public Texture? TextureFor(string name) +{ + if (!EnsureRig()) return null; // rig may have failed to build earlier + // ... +} + +// RIGHT — if the ctor returned, the rig exists. No guard, no nullable return. +public Texture TextureFor(string name) { /* rig is always valid here */ } +``` + +Same principle for one-time wiring: subscribe to an event/pipe **once in the constructor**, not lazily behind `if (!subscribed)` guards. A lazy re-subscribe path overwrites the stored delegate and leaves a dangling callback; the guards are redundant once the subscription is a construction invariant. + +### 11. Names that hide narrowed or implicit behavior + +If a member silently ignores a case, the name must say so. A "video muted" check that deliberately skips screen-share tracks is `IsCurrentCameraVideoMuted`, not `IsCurrentVideoMuted`. Rename types to their real responsibility (`CameraOffScreenComposer` → `AvatarPlaceHolderTextureSource` when the class's job is "provide the placeholder texture", not "compose a screen"). A field's name should reveal its role, not just its contents — a scratch collection is a `…Buffer`, a point-in-time copy a `…Snapshot`. + +### 12. Booleans that conflate "absent" with a state; re-validating inside + +`bool IsX` returning `false` for "the thing doesn't exist" is a lie — absence is not the negative state (a video that does not exist is neither muted nor unmuted). Prefer a method that takes the already-validated, non-null instance over a property that returns a default for the missing case and re-runs the caller's null/has checks. + +```csharp +// WRONG — property conflates "no video" with "not muted", and re-checks +public bool IsCurrentVideoMuted => cvs.HasValue && cvs.Value.Track.Muted; + +// RIGHT — caller validates once; method can't be called in an invalid state +private static bool IsMuted(in CurrentVideoStreamInfo info) => info.Track.Muted; +``` + +Make invalid states unrepresentable in the signature instead of defending against them inside. + +### 13. Side-effects where a return value works; dependencies hidden behind default params + +- If a function can return its result, return it — don't mutate shared state as a side channel. +- Don't give a real collaborator a `= null` default (`CreateForScene(..., AvatarPlaceHolderTextureSource? placeholder = null)`). Pass it explicitly at every call site — with a named argument (`placeholder: null`) when it is genuinely absent — so the wiring stays visible. + +### 14. Caches / shared GPU resources without cost justification or an ownership contract + +Weigh VRAM/heap against the work actually saved before adding a cache. A sub-millisecond per-frame recompute (one orthographic UI bake) often beats a per-key `RenderTexture` cache — 16 × 1024² BGRA32 = 64 MB of VRAM for a fallback graphic. And never hand out raw pooled/shared `RenderTexture` references without an ownership/lifetime contract: a caller that holds the reference past the frame you `Destroy()` it renders a destroyed texture (magenta/black). Prefer one reusable render target, recomposed only when its inputs change. + +### 15. Scattered conditionals / string-typing instead of a domain type + +Many `if/else` branches over a string prefix or a nullable "identity" field — or a condition like `x != null && x.StartsWith(SOME_PREFIX)` repeated across methods — mean a concept wants to be a **type**. Model it as an immutable domain object or a union (this repo uses [REnum](https://github.com/NickKhalow/REnum) for union types with exhaustive matching). The branching collapses and illegal states become unrepresentable. + +Corollary — **don't handle cases that cannot happen.** If every `GateKeeperMode` maps to a valid URL by design, a `null` URL is not a branch to handle, it's a bug to surface (throw/assert). Defensive handling of impossible states hides the real defect and bloats the code. Where the type offers a sentinel (`Weak.Null`), prefer it over a nullable (`Weak?`). + +```csharp +// WRONG — the "mode" is a string prefix, re-tested in every method +if (currentVideoIdentity != null && currentVideoIdentity.StartsWith(PRESENTATION_BOT_PREFIX)) ... + +// RIGHT — the mode is a case of the address type; matching is exhaustive +[REnumField(typeof(UserStream))] +[REnumField(typeof(PresentationBotStream))] +[REnumFieldEmpty("CurrentStream")] +public readonly partial struct LivekitAddress { } +``` + +### 16. Leaking or bypassing an existing abstraction + +Use the abstraction the codebase already provides instead of reaching past it. Route protobuf messages through `IMessagePipe`, not `IDataPipe` directly — direct use leaks the abstraction and forces obscure `try/catch` hacks downstream to compensate. Before writing new resolution/parsing logic, find the owner and reuse it: URL building lives in `DecentralandUrlsSource` (it already handles ORG/ZONE/TODAY); participant display-name resolution already exists in the chat module. + +### 17. Thread-safety left implicit on shared mutable state + +If you add a mutable collection to a class touched from more than one thread, make the safety model explicit: either confine it (document `// Not thread-safe — main-thread only.` and keep all access on that thread) or use a `ConcurrentDictionary` / explicit lock — and ensure access to any nested list is thread-safe too. A non-thread-safe collection reachable from multiple threads with no stated constraint is a latent race. + +### 18. Linear scans and redundant buffers in hot paths + +Watch algorithmic complexity where a query runs per-frame or per-item: + +- Replace O(n) membership checks (`List.Exists`, linear `foreach` search) with a `HashSet` / dictionary keyed by identity. A linear lookup *inside* a loop over all items is O(n²) — e.g. `HasAudioSourceForKey` scanning `audioSources` for every key. +- Drop intermediate buffers that exist only to be re-iterated immediately (process the source directly instead of filling `streamKeysBuffer` first). +- If you only need the first element, index `[0]`; don't `foreach`-and-break. + +### 19. Dead code, redundant flags, dead stores, and duplicated logic + +- Remove methods/fields that are never called (`IsVideoTrackMuted`) and booleans that merely cache a direct expression (`hasLiveAudio` when `IsAudioOpened` computes it inline). +- Remove assignments overwritten on every subsequent path (resetting `currentVideoIdentity` before both branches set it) and `if/else` arms that do the same thing. +- Extract a duplicated condition into one named helper, and **resolve/derive a value in exactly one place** — ideally one pure function marked `[Pure]`. Scattering URL resolution across `DecentralandUrlsSource` and `MainSceneLoader` means a change to one is silently missed by the other (a latent "two different sub-URLs depending on context" bug). + +### 20. Hiding failures instead of surfacing them + +- **Band-aid recovery:** a `try/catch` or "repair the corrupt X" branch added to make a crash disappear without understanding *what* corrupts the data. Find and fix the cause. +- **Stacked catches:** if a later handler already covers a case ("invalid data from participant"), an earlier duplicate catch is excessive — remove it. +- **Silent lookups:** `Assert.IsNotNull` on reflection / serialized-property-name lookups so a rename fails loudly at startup instead of silently doing nothing. + ## PR Standards - **Branches:** Based on `dev` branch diff --git a/CLAUDE.md b/CLAUDE.md index 8cea71cfe9a..0d2798f45d7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2,9 +2,9 @@ ## Startup -At the start of every conversation, read [`docs/README.md`](docs/README.md) to load the project documentation map. Skills are automatically loaded by Claude Code from `.claude/skills/` — do not manually read them. +At the start of every conversation, read [`docs/README.md`](docs/README.md) to load the project documentation map. Claude Code lists each skill's name and description, but a skill's full instructions are **not** in context until you invoke it with the `Skill` tool — invoke skills, don't assume they're already loaded. -Before writing or modifying any code, follow the code-standards skill for naming conventions, member ordering, formatting rules, and test patterns. For edge cases, [`Explorer/.editorconfig`](Explorer/.editorconfig) is the authoritative formatting reference. +Before writing or modifying **any** `.cs` file, **first** invoke the code-standards skill (`Skill(code-standards)`) — before any Read/Edit/Write on production code — for naming conventions, member ordering, formatting rules, nullable/GC rules, anti-patterns, and test patterns. Invoke the relevant domain skills too when they apply (`ecs-system-and-component-design` for ECS, `async-programming` for UniTask, `sdk-component-implementation` for SDK components, `testing-infrastructure` for tests). For edge cases, [`Explorer/.editorconfig`](Explorer/.editorconfig) is the authoritative formatting reference. ## Linting in the AI flow diff --git a/scripts/hooks/remind-code-standards.sh b/scripts/hooks/remind-code-standards.sh new file mode 100755 index 00000000000..2598f76f618 --- /dev/null +++ b/scripts/hooks/remind-code-standards.sh @@ -0,0 +1,37 @@ +#!/usr/bin/env bash +# PreToolUse hook: when an Edit/Write/MultiEdit targets a .cs file, remind the model +# (once per session) to invoke the code-standards skill. A skill's full instructions are +# NOT in context until the Skill tool is invoked, so this closes the gap deterministically +# for C# edits. Emits `additionalContext`; it never blocks the tool. +# +# Input: the PreToolUse hook JSON on stdin. +# Output: on the first .cs edit of a session, a JSON object with hookSpecificOutput +# .additionalContext; otherwise nothing. +set -uo pipefail + +# Never break a tool call: if jq is unavailable, stay silent and let the edit proceed. +command -v jq >/dev/null 2>&1 || exit 0 + +input="$(cat)" + +file_path="$(printf '%s' "$input" | jq -r '.tool_input.file_path // empty' 2>/dev/null || true)" + +# Only C# files; inert for everything else. +case "$file_path" in + *.cs) ;; + *) exit 0 ;; +esac + +session_id="$(printf '%s' "$input" | jq -r '.session_id // "unknown"' 2>/dev/null || echo unknown)" +sentinel="${TMPDIR:-/tmp}/claude-code-standards-reminder-${session_id}" + +# Fire once per session to avoid per-edit noise. +if [ -e "$sentinel" ]; then + exit 0 +fi +: > "$sentinel" 2>/dev/null || true + +reminder="About to edit a C# file. If you have not yet done so this session, invoke Skill(code-standards) BEFORE writing. Its rules (naming, member ordering, nullable, GC, and the anti-patterns distilled from PR review) are NOT in context until the skill is invoked — do not assume they are already loaded." + +jq -cn --arg ctx "$reminder" \ + '{hookSpecificOutput: {hookEventName: "PreToolUse", additionalContext: $ctx}}' From 841dd13fafaf705271c210b045d932a3c8221f52 Mon Sep 17 00:00:00 2001 From: Gabriel Diaz Date: Fri, 10 Jul 2026 11:43:02 +0200 Subject: [PATCH 2/3] chore: address bot review on code-standards hardening - Drop "reading" from the code-standards skill description: neither the CLAUDE.md guidance nor the PreToolUse hook fires on Read, and loading the anti-patterns on every .cs read wastes context. Keep writing/ modifying/reviewing, which the standards actually serve. - Sanitize session_id before using it in the hook's sentinel path (strip non [A-Za-z0-9_-]) so a crafted value can't escape $TMPDIR. --- .claude/skills/code-standards/SKILL.md | 2 +- scripts/hooks/remind-code-standards.sh | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/.claude/skills/code-standards/SKILL.md b/.claude/skills/code-standards/SKILL.md index a8031c29df1..03233a84840 100644 --- a/.claude/skills/code-standards/SKILL.md +++ b/.claude/skills/code-standards/SKILL.md @@ -1,6 +1,6 @@ --- name: code-standards -description: "C# naming conventions, member ordering, formatting rules, nullable reference types, memory/GC rules, test patterns, and PR standards. Use BEFORE reading, 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." +description: "C# naming conventions, member ordering, formatting rules, nullable reference types, memory/GC rules, test patterns, and PR standards. Use BEFORE writing, modifying, or reviewing 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." user-invocable: false --- diff --git a/scripts/hooks/remind-code-standards.sh b/scripts/hooks/remind-code-standards.sh index 2598f76f618..60043e3d375 100755 --- a/scripts/hooks/remind-code-standards.sh +++ b/scripts/hooks/remind-code-standards.sh @@ -23,6 +23,9 @@ case "$file_path" in esac session_id="$(printf '%s' "$input" | jq -r '.session_id // "unknown"' 2>/dev/null || echo unknown)" +# Strip anything not path-safe: the sentinel name must never escape $TMPDIR. +session_id="${session_id//[^a-zA-Z0-9_-]/}" +[ -n "$session_id" ] || session_id="unknown" sentinel="${TMPDIR:-/tmp}/claude-code-standards-reminder-${session_id}" # Fire once per session to avoid per-edit noise. From 68680ee2402324c3f65f88c92a3b6b613ec7b50f Mon Sep 17 00:00:00 2001 From: Gabriel Diaz Date: Fri, 10 Jul 2026 13:25:03 +0200 Subject: [PATCH 3/3] chore: drop the PreToolUse code-standards reminder hook Remove scripts/hooks/remind-code-standards.sh and revert .claude/settings.json to the Stop-lint hook only. The skill description tightening and the CLAUDE.md Startup rewrite remain and carry the intent on their own; the hook is dropped from this PR. --- .claude/settings.json | 11 ------- scripts/hooks/remind-code-standards.sh | 40 -------------------------- 2 files changed, 51 deletions(-) delete mode 100755 scripts/hooks/remind-code-standards.sh diff --git a/.claude/settings.json b/.claude/settings.json index 9f086f9272e..4e6f23fe1e7 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -1,16 +1,5 @@ { "hooks": { - "PreToolUse": [ - { - "matcher": "Edit|Write|MultiEdit", - "hooks": [ - { - "type": "command", - "command": "bash scripts/hooks/remind-code-standards.sh" - } - ] - } - ], "Stop": [ { "hooks": [ diff --git a/scripts/hooks/remind-code-standards.sh b/scripts/hooks/remind-code-standards.sh deleted file mode 100755 index 60043e3d375..00000000000 --- a/scripts/hooks/remind-code-standards.sh +++ /dev/null @@ -1,40 +0,0 @@ -#!/usr/bin/env bash -# PreToolUse hook: when an Edit/Write/MultiEdit targets a .cs file, remind the model -# (once per session) to invoke the code-standards skill. A skill's full instructions are -# NOT in context until the Skill tool is invoked, so this closes the gap deterministically -# for C# edits. Emits `additionalContext`; it never blocks the tool. -# -# Input: the PreToolUse hook JSON on stdin. -# Output: on the first .cs edit of a session, a JSON object with hookSpecificOutput -# .additionalContext; otherwise nothing. -set -uo pipefail - -# Never break a tool call: if jq is unavailable, stay silent and let the edit proceed. -command -v jq >/dev/null 2>&1 || exit 0 - -input="$(cat)" - -file_path="$(printf '%s' "$input" | jq -r '.tool_input.file_path // empty' 2>/dev/null || true)" - -# Only C# files; inert for everything else. -case "$file_path" in - *.cs) ;; - *) exit 0 ;; -esac - -session_id="$(printf '%s' "$input" | jq -r '.session_id // "unknown"' 2>/dev/null || echo unknown)" -# Strip anything not path-safe: the sentinel name must never escape $TMPDIR. -session_id="${session_id//[^a-zA-Z0-9_-]/}" -[ -n "$session_id" ] || session_id="unknown" -sentinel="${TMPDIR:-/tmp}/claude-code-standards-reminder-${session_id}" - -# Fire once per session to avoid per-edit noise. -if [ -e "$sentinel" ]; then - exit 0 -fi -: > "$sentinel" 2>/dev/null || true - -reminder="About to edit a C# file. If you have not yet done so this session, invoke Skill(code-standards) BEFORE writing. Its rules (naming, member ordering, nullable, GC, and the anti-patterns distilled from PR review) are NOT in context until the skill is invoked — do not assume they are already loaded." - -jq -cn --arg ctx "$reminder" \ - '{hookSpecificOutput: {hookEventName: "PreToolUse", additionalContext: $ctx}}'