diff --git a/.claude/skills/code-standards/SKILL.md b/.claude/skills/code-standards/SKILL.md index fec438670fd..a85d30f453f 100644 --- a/.claude/skills/code-standards/SKILL.md +++ b/.claude/skills/code-standards/SKILL.md @@ -29,6 +29,8 @@ user-invocable: false **Exception:** serialized JSON DTO fields keep their wire-format casing — see *Serialized JSON DTOs* below. +**Namespaces are domain names, not folder paths** — never suppress a folder-namespace mismatch with `// ReSharper disable once CheckNamespace`; fix the namespace or leave the warning visible. Full rule: `docs/code-style-guidelines.md` § Namespaces. + ## Member Ordering Within a class, group members in this order: diff --git a/.github/workflows/pr-comment-warnings.yml b/.github/workflows/pr-comment-warnings.yml index c0923f532b7..a22db438e3d 100644 --- a/.github/workflows/pr-comment-warnings.yml +++ b/.github/workflows/pr-comment-warnings.yml @@ -73,6 +73,7 @@ jobs: { echo "body<" + blocked=false if [ -z "$BASELINE" ]; then echo "**Warnings counted: $COUNT** (no baseline established yet)" elif [ "$COUNT" -lt "$BASELINE" ]; then @@ -81,6 +82,27 @@ jobs: echo "**Warnings unchanged: $BASELINE => $COUNT** — allowed on release/hotfix branches." else echo "**Warnings not reduced: $BASELINE => $COUNT** — remove at least one warning to merge." + blocked=true + fi + + # Always list the warnings/errors in files this PR changed; expand only when blocking. + total=$(jq -r '.pr_findings_total // 0' warning-result.json) + if [ "$total" -gt 0 ]; then + open_attr="" + [ "$blocked" = "true" ] && open_attr=" open" + echo "" + echo "Warnings/errors in files changed by this PR ($total)" + echo "" + echo "| File | Line | Rule | Message |" + echo "| --- | --- | --- | --- |" + jq -r '.pr_findings[] | "| \(.file) | \(.line) | \(.rule) | \(.message | gsub("[|\r\n]"; " ")) |"' warning-result.json + shown=$(jq -r '.pr_findings | length' warning-result.json) + if [ "$total" -gt "$shown" ]; then + echo "" + echo "_…and $((total - shown)) more (see the \`csharp-lint-reports\` artifact)._" + fi + echo "" + echo "" fi echo "EOF" } >> "$GITHUB_OUTPUT" diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f6429cc54b6..e13544dbe50 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -398,6 +398,44 @@ jobs: aws s3 cp baseline.json "s3://$EXPLORER_TEAM_S3_BUCKET/${{ env.WARNINGS_BASELINE_S3_KEY }}" \ --content-type application/json --cache-control no-cache + # Warnings/errors located in files this PR changed — surfaced in the PR comment. + - name: Collect PR-file warnings + if: ${{ !cancelled() && github.event_name == 'pull_request' }} + id: pr-findings + env: + BASE_REF: ${{ github.event.pull_request.base.ref }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + MAX_FINDINGS: 50 # max rows listed in the PR comment; change here + run: | + set -euo pipefail + echo '[]' > pr-findings.json + echo "total=0" >> "$GITHUB_OUTPUT" + [ -f filtered.json ] || exit 0 + + base=$(git merge-base "origin/$BASE_REF" "$HEAD_SHA" 2>/dev/null || true) + [ -n "$base" ] || { echo "No merge-base - skipping PR-file findings."; exit 0; } + + # repo-relative paths -> project-relative (strip leading 'Explorer/') + git diff --name-only "$base" "$HEAD_SHA" -- '*.cs' \ + | sed 's#^Explorer/##' | sort -u > changed-files.txt + + jq --rawfile changed changed-files.txt ' + ($changed | split("\n") | map(select(length > 0))) as $files + | map({ + file: (.locations[0].physicalLocation.artifactLocation.uri // ""), + line: (.locations[0].physicalLocation.region.startLine // 0), + rule: (.ruleId // ""), + level: (.level // "warning"), + message: (.message.text // "") + }) + | map(select(.file as $f | $files | index($f) != null)) + ' filtered.json > pr-findings-all.json + + total=$(jq length pr-findings-all.json) + jq --argjson max "$MAX_FINDINGS" '.[:$max]' pr-findings-all.json > pr-findings.json # cap comment size + echo "total=$total" >> "$GITHUB_OUTPUT" + echo "PR-file findings: $total (listing up to $MAX_FINDINGS)" + # Hand the numbers to the trusted workflow_run companion that posts the PR comment. # Runs even when the gate above failed (!cancelled), so a failing run still refreshes the comment # instead of leaving a stale message. @@ -416,7 +454,9 @@ jobs: --argjson count "$COUNT" \ --arg baseline "$BASELINE" \ --argjson allow_equal "$IS_RELEASE_OR_HOTFIX" \ - '{pr: $pr, count: $count, baseline: (if $baseline == "" then null else ($baseline | tonumber) end), allow_equal: $allow_equal}' \ + --slurpfile findings pr-findings.json \ + --argjson findings_total "${{ steps.pr-findings.outputs.total || 0 }}" \ + '{pr: $pr, count: $count, baseline: (if $baseline == "" then null else ($baseline | tonumber) end), allow_equal: $allow_equal, pr_findings: ($findings[0] // []), pr_findings_total: $findings_total}' \ > warning-result.json cat warning-result.json diff --git a/CLAUDE.md b/CLAUDE.md index 0e010149e0a..7606452e03d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -131,6 +131,7 @@ Reviewers have repeatedly identified AI-generated code by these smells. Check yo * **Wiring pooled/virtualized list items per rebind.** For item pools, wire callbacks once when the item is created, not every time `SetItemData` runs. Prefer an `Action` field (single subscriber, direct assignment) over C# `event` (`+=`/`-=` churn) when there is exactly one subscriber. * **Reimplementing primitives that already exist.** Before writing manual atlas UV math, check `TMP_Sprite Asset`. Before hand-batching profile lookups, check the batched `GetProfilesAsync(IReadOnlyList, ct)` overload. Before adding a bespoke event pathway, check `ViewEventBus` / `ChatEvents`. * **Comments that narrate caller/external behavior.** A comment must state only what the annotated code itself does or guarantees ("remove the corrupt file so the next read doesn't hit it"), never what callers or upper layers will do with the result ("so callers treat it as a miss and re-download"). External behavior can change without this code changing, silently turning the comment into a lie. +* **Suppressing `CheckNamespace` with a ReSharper comment.** Never add `// ReSharper disable once CheckNamespace` (or the file-wide variant) — fix the namespace or leave the warning visible. Rationale and full rule: [`docs/code-style-guidelines.md` § Namespaces](docs/code-style-guidelines.md#namespaces). ### Other project-specific rules diff --git a/Explorer/Assets/DCL/Infrastructure/SceneLifeCycle/Systems/UpdateCurrentSceneSystem.cs b/Explorer/Assets/DCL/Infrastructure/SceneLifeCycle/Systems/UpdateCurrentSceneSystem.cs index 4ca2f3a2805..fd3a8433b23 100644 --- a/Explorer/Assets/DCL/Infrastructure/SceneLifeCycle/Systems/UpdateCurrentSceneSystem.cs +++ b/Explorer/Assets/DCL/Infrastructure/SceneLifeCycle/Systems/UpdateCurrentSceneSystem.cs @@ -21,6 +21,7 @@ namespace ECS.SceneLifeCycle.Systems [UpdateInGroup(typeof(RealmGroup))] public partial class UpdateCurrentSceneSystem : BaseUnityLoopSystem { + private const string NO_DATA_STRING = ""; private static readonly int SRC_BLEND = Shader.PropertyToID("_SrcBlend"); diff --git a/docs/code-style-guidelines.md b/docs/code-style-guidelines.md index 443113a4c72..8ccffed80d8 100644 --- a/docs/code-style-guidelines.md +++ b/docs/code-style-guidelines.md @@ -248,6 +248,10 @@ List filteredWords = new FilterLogic(listWords). - Folder structure should be aligned with the namespaces. - Not every folder should be namespace provider, especially folders like `Scripts`, `MainScripts`, `Assets`. - Folders that are deep in the folders hierarchy should be without namespace. +- Never suppress the folder-namespace inspection with `// ReSharper disable once CheckNamespace` (or the file-wide `// ReSharper disable CheckNamespace`). + - Namespaces name domains and deliberately survive folder and assembly reshuffles (e.g. `DCL.Ipfs` lives in the `DCL.Network` assembly), so a folder-namespace mismatch is often intentional. + - The lint filter shared by CI and the local hook (`scripts/lint/filter-warnings.sh`) already excludes the `CheckNamespace` inspection, so the comment changes nothing in the warning count — it is pure noise, and it hides genuine cases where a type joined the wrong namespace. + - If the IDE flags a mismatch, either move the type into the domain namespace its closest collaborators live in, or leave the warning visible. ### Whitespaces diff --git a/scripts/lint/filter-warnings.sh b/scripts/lint/filter-warnings.sh index d8f61296eb6..564414fa6ee 100755 --- a/scripts/lint/filter-warnings.sh +++ b/scripts/lint/filter-warnings.sh @@ -1,11 +1,7 @@ #!/usr/bin/env bash # Filter false positives out of an InspectCode report and print the remaining count. -# Excludes: -# - '.CSharpErrors' / '.CppCompilerErrors': false positives from '--no-build' (unresolved refs). -# - vendored / third-party code we don't own: everything under 'Packages/' and the -# 'DOTween' / 'SocketIO' plugins. -# - everything below warning severity (SARIF level 'note' = ReSharper suggestions/hints): -# only warning-or-higher results participate in the ratchet. +# Only warning-or-higher results (SARIF level 'warning'/'error') in code we own +# participate in the ratchet; the exclusions and their reasons are declared inline below. # # Usage: filter-warnings.sh # Writes the filtered results array to ; prints the integer count to stdout. @@ -14,18 +10,28 @@ set -euo pipefail report="${1:?usage: filter-warnings.sh }" out="${2:?usage: filter-warnings.sh }" +excluded_rules=( + '.CSharpErrors' # false positive from '--no-build' (unresolved refs) + '.CppCompilerErrors' # false positive from '--no-build' (unresolved refs) + 'CheckNamespace' # namespaces are domain names, not folder paths (docs/code-style-guidelines.md § Namespaces) +) + +# vendored / third-party code we don't own +excluded_paths='^(Packages/|Assets/Plugins/(DOTween|SocketIO)/)' + if [ ! -f "$report" ]; then echo "filter-warnings: report not found at '$report'" >&2 exit 1 fi -jq ' +jq --argjson excludedRules "$(printf '%s\n' "${excluded_rules[@]}" | jq -R . | jq -s .)" \ + --arg excludedPaths "$excluded_paths" ' .runs[0].results | map(select( ((.level // "warning") | IN("warning", "error")) - and (.ruleId != ".CSharpErrors" and .ruleId != ".CppCompilerErrors") + and ((.ruleId // "") | IN($excludedRules[]) | not) and ((.locations[0].physicalLocation.artifactLocation.uri // "") - | test("^(Packages/|Assets/Plugins/(DOTween|SocketIO)/)"; "i") | not) + | test($excludedPaths; "i") | not) )) ' "$report" > "$out"