Skip to content
Merged
2 changes: 2 additions & 0 deletions .claude/skills/code-standards/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
22 changes: 22 additions & 0 deletions .github/workflows/pr-comment-warnings.yml
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ jobs:
{
echo "body<<EOF"
echo "<!-- warning-ratchet -->"
blocked=false
if [ -z "$BASELINE" ]; then
echo "**Warnings counted: $COUNT** (no baseline established yet)"
elif [ "$COUNT" -lt "$BASELINE" ]; then
Expand All @@ -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 "<details$open_attr><summary>Warnings/errors in files changed by this PR ($total)</summary>"
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 "</details>"
fi
echo "EOF"
} >> "$GITHUB_OUTPUT"
Expand Down
42 changes: 41 additions & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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

Expand Down
1 change: 1 addition & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>, 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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ namespace ECS.SceneLifeCycle.Systems
[UpdateInGroup(typeof(RealmGroup))]
public partial class UpdateCurrentSceneSystem : BaseUnityLoopSystem
{

private const string NO_DATA_STRING = "<No data>";

Comment thread
dalkia marked this conversation as resolved.
private static readonly int SRC_BLEND = Shader.PropertyToID("_SrcBlend");
Expand Down
4 changes: 4 additions & 0 deletions docs/code-style-guidelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,10 @@ List<string> 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

Expand Down
24 changes: 15 additions & 9 deletions scripts/lint/filter-warnings.sh
Original file line number Diff line number Diff line change
@@ -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 <report.json> <filtered_output.json>
# Writes the filtered results array to <filtered_output.json>; prints the integer count to stdout.
Expand All @@ -14,18 +10,28 @@ set -euo pipefail
report="${1:?usage: filter-warnings.sh <report.json> <filtered_output.json>}"
out="${2:?usage: filter-warnings.sh <report.json> <filtered_output.json>}"

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"

Expand Down
Loading