chore: exclude CheckNamespace from the lint warning ratchet#9396
Conversation
Namespaces are domain names, not folder paths, so folder-namespace mismatch is expected in this codebase. Drop CheckNamespace from the remote warning count via an opt-in EXCLUDE_RULES filter, while local inspection (Rider and the AI-flow Stop hook) keeps reporting it. Codify in the conventions docs that suppressing it with a ReSharper comment is forbidden. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
|
Sorry, I encountered an error processing your review request. The team has been notified. |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: chore: exclude CheckNamespace from the CI warning ratchet only
STEP 2 — Root-cause check: PASS
The problem is clear: CheckNamespace accounts for 4,444 of 19,087 CI warnings (~23%), but namespaces in this codebase are domain names that deliberately don't match folder paths (e.g. DCL.Ipfs in the DCL.Network assembly). The diff addresses the cause — excluding the irrelevant inspection from the CI ratchet — rather than disabling it everywhere or suppressing individual occurrences. Local inspection remains untouched.
STEP 3 — Design & integration: PASS
No new long-lived units, systems, or lifecycle management. This is a CI script enhancement with a clean, extensible design:
EXCLUDE_RULESenv var — optional, comma-separated, defaults to empty (no exclusion). Backward compatible: every existing caller offilter-warnings.shis unchanged.- Opt-in is scoped — only
.github/actions/count-warnings/action.ymlsetsEXCLUDE_RULES=CheckNamespaceat the step level, so the local AI-flow lint hook (lint-changed.sh) continues reportingCheckNamespace. - jq parameterization —
--arg excluded "${EXCLUDE_RULES:-}"is safe (no injection risk, no code evaluation). Thesplit(",") | map(select(length > 0))correctly handles empty strings, trailing commas, and the zero-rules default.IN($skip[])returnsfalsefor an empty stream, so all rules pass through when unset. Verified.
STEP 4 — Member audit: N/A
No public properties, accessors, or code members added — CI scripts and documentation only.
STEP 5 — Line-level review: No issues found
Pass A (blocking issues):
- Shell script uses
set -euo pipefail, proper double-quoting of${EXCLUDE_RULES:-}, and safejq --argparameterization. No injection vectors. - The
(.ruleId // "")fallback handles results without aruleIdsafely (falls through to empty string, never matches the skip list sincemap(select(length > 0))removes empties). - Documentation updates across
CLAUDE.md,docs/code-style-guidelines.md, and.claude/skills/code-standards/SKILL.mdare consistent and accurate.
Pass B (design/encapsulation smells): Nothing applicable — no runtime code, no classes, no state.
Security review: Clean
EXCLUDE_RULESis hardcoded in the action YAML, not sourced from untrusted PR input.jq --argtreats the value as a string literal, not executable code.- No secrets, credentials, or internal URLs exposed.
IN($skip[])uses exact string equality — no regex/wildcard bypass possible.
Git conventions (ADR-6): PASS
- PR title:
chore: exclude CheckNamespace from the CI warning ratchet only— correct semantic commit format. - Branch name:
chore/exclude-checknamespace-ci— correct<type>/<summary>pattern.
STEP 8 — Non-blocking warnings: None
Main scene not modified. No warnings.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: CI shell script filter and documentation updates only — no runtime, ECS, or Unity code touched.
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni [Dalkia] (<@U07FBNXHWKN>) via Slack
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
github-actions
Bot
added
claude-approved
no QA needed
dalkia
added
the
force-lint
…inters elsewhere Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dalkia
added
force-lint
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review — PR #9396
chore: exclude CheckNamespace from the CI warning ratchet only
STEP 2 — Root-cause check: PASS
Problem: CheckNamespace accounts for ~4,444 of the ~19,087 warnings in the current dev baseline (~23%). Namespaces in this codebase are domain names, not folder paths (e.g. DCL.Ipfs lives in the DCL.Network assembly), so folder-namespace mismatches are intentional and expected — they should not consume the warning ratchet.
Fix: Excludes CheckNamespace from the CI ratchet's count only, leaving local inspection (Rider, AI-flow Stop hook) intact. This is the right approach — it addresses the root cause (noise in the ratchet) rather than suppressing the inspection everywhere.
STEP 3 — Design & integration: PASS
No new long-lived units are introduced — this is a CI/tooling change. The design is clean:
- Generic mechanism (
EXCLUDE_RULESenv var) added to the existingfilter-warnings.shrather than hardcoding a special case. The env-var approach is appropriate: it keeps the script caller-agnostic and composable. - Backward-compatible default: empty/unset
EXCLUDE_RULESexcludes nothing, so the local AI-flow hook (lint-changed.sh) and any other callers are completely unaffected. - Single opt-in point: only the CI action (
count-warnings/action.yml) sets the env var. - Documentation is updated consistently across three locations (CLAUDE.md anti-patterns,
docs/code-style-guidelines.md§ Namespaces,.claude/skills/code-standards/SKILL.md), each at the appropriate level of detail with cross-references.
No lifecycle owners to search — no runtime units affected.
STEP 4 — Member audit: N/A
No public properties, accessors, or methods introduced.
STEP 5 — Line-level review: No issues found
Shell script (filter-warnings.sh):
--arg excluded "${EXCLUDE_RULES:-}"safely passes the env var as a jq string value (not as jq code). No injection risk.split(",") | map(select(length > 0))correctly handles all edge cases: empty string →[], single value, multiple values, double-commas.IN($skip[]) | notis correct when$skipis empty: jq'sIN(empty)returnsfalse,| notgivestrue, so all items pass through. The filter becomes a no-op as intended..ruleId // ""provides a defensive fallback for results missing aruleIdfield.- The hardcoded
.CSharpErrors/.CppCompilerErrorsexclusions are correctly kept separate fromEXCLUDE_RULES— they are internal false-positive filters that must always apply regardless of caller context.
CI action (count-warnings/action.yml):
env: EXCLUDE_RULES: CheckNamespaceis properly scoped to the step.- The explanatory comment clearly states intent and why local inspection is intentionally left unchanged.
Documentation:
- The three locations serve different audiences (AI reviewers via CLAUDE.md, developers via code-style-guidelines.md, AI code-generation via code-standards SKILL.md) and are consistent in message: never suppress CheckNamespace with a ReSharper comment; fix the namespace or leave the warning visible.
Security review: Clean
EXCLUDE_RULESis interpolated into jq via--arg(safe string binding, not code injection).- The value is hardcoded in the CI action — not user-controlled.
- No secrets, credentials, or sensitive data in the diff.
- No new permissions, network access, or external dependencies.
set -euo pipefailis preserved for safe shell execution.
STEP 6, 7, 8
No non-blocking warnings. Main scene not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: CI lint-filter script and documentation only — no runtime code, no ECS systems, no Unity player changes.
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
|
Warnings count reduced: 19087 => 14693 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
NickKhalow
left a comment
NickKhalow
left a comment
There was a problem hiding this comment.
Good addition! One comment: let's encode less meaning into the comments and more into the actual code to preserve more the single source of truth approach
Per review: keep local and CI consistent by excluding the rule in the shared filter script instead of a CI-only EXCLUDE_RULES opt-in. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
dalkia
changed the title
NickKhalow
added
force-lint
Per review: the excluded ruleIds and vendored-path pattern are now named variables the jq filter consumes, with per-entry reasons inline, so the header no longer duplicates the list. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @dalkia's task in 1m 21s —— View job Security-focused re-review (PR synchronize) — PR #9396
What changed since the last passSince the 14:19 review, the only new commits are two whitespace-only "Dummy change: retrigger lint" commits (
Dependency changes — noneNo Workflow files — re-confirmed, LOW
Non-blocking note — stray blank line in a production file
public partial class UpdateCurrentSceneSystem : BaseUnityLoopSystem
{
+
private const string NO_DATA_STRING = "<No data>";Not a security concern, but it's an unintended edit to runtime code that shouldn't ship with a CI-only PR — worth reverting before merge so the diff stays scoped to lint tooling + docs. Pre-existing note (not introduced here)
Summary
No dependency changes, no dynamic code loading, no untrusted-input execution, no secret exposure, no unpinned action introduced by this PR. The workflow files are unchanged from the prior review; the only substantive new content is a stray blank line that should be reverted (functional, not security). DEPENDENCY_REVIEW: PASS |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — chore: exclude CheckNamespace from the lint warning ratchet
STEP 2 — Root-cause check: PASS
The PR correctly identifies that CheckNamespace accounts for ~23% (4,444 / 19,087) of CI warnings, and that folder-namespace mismatches are intentional in this codebase (namespaces are domain names, e.g. DCL.Ipfs in the DCL.Network assembly). Excluding the rule from the warning ratchet fixes the root cause — the ratchet was counting false positives — rather than a symptom.
STEP 3 — Design & integration: PASS
No new systems, plugins, managers, or state-holding units. All changes are CI scripts, GitHub Actions workflows, and documentation. No lifecycle, ECS, or architectural concerns apply.
STEP 4 — Member audit: N/A
No public members added or changed.
STEP 5 — Line-level review
Shell script (scripts/lint/filter-warnings.sh): The refactor from inline rule names to a declared excluded_rules array is clean and extensible. The printf → jq -R → jq -s pipeline correctly builds a JSON array for --argjson. The excluded_paths regex is passed as --arg and applied with test()/IN() — correct.
Workflow (test.yml — "Collect PR-file warnings"): Properly initializes defaults before any fallible command (echo '[]' > pr-findings.json; echo "total=0"). Uses git merge-base with fallback. The sed 's#^Explorer/##' correctly aligns repo-relative paths with SARIF's project-relative paths. The ${{ steps.pr-findings.outputs.total || 0 }} expression safely defaults when the step is skipped.
Workflow (pr-comment-warnings.yml): The blocked flag and conditional <details open> are well-designed — warnings auto-expand only when they block the merge. The gsub("[|\r\n]"; " ") sanitizes pipe/newline chars to avoid breaking the markdown table.
Documentation (CLAUDE.md, style guide, skill): Consistent three-point documentation — full rule in the style guide, one-line pointers in CLAUDE.md and the skill file.
Security: No issues. Env vars (not inline ${{ }}) are used for branch names, avoiding shell injection. All ${{ }} expansions in run: blocks are integer-only (pr.number, total). No secrets exposed. jq processes InspectCode output internally — no command injection vectors.
One finding — see inline comment.
STEP 6 — Complexity: SIMPLE
CI scripts, GitHub Actions YAML, and documentation only.
STEP 7 — QA: NO
No runtime code affected. All changes are CI/tooling/docs.
STEP 8 — Non-blocking warnings
None.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: CI lint scripts, GitHub Actions workflows, and documentation — no runtime or ECS code.
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni [Dalkia] (<@U07RZTGR6CH>) via Slack
Dismissed: latest Jarvis review no longer auto-approves this PR.
|
🔒 Jarvis reviewed this PR — sensitive paths modified ( |
4 participants
Pull Request Description
What does this PR change?
Replaces #9373 with a narrower approach: instead of disabling the
CheckNamespaceinspection everywhere via.editorconfig, it is excluded inscripts/lint/filter-warnings.sh— the filter shared by the CI warning ratchet and the local AI-flow lint hook — so both count warnings identically. IDE inspection (Rider) is untouched and still highlights mismatches.Rationale: namespaces in this codebase are domain names, not folder paths (e.g.
DCL.Ipfslives in theDCL.Networkassembly), so folder-namespace mismatch is expected and should not consume the warning ratchet —CheckNamespaceaccounts for 4,444 of the 19,087 warnings in the current dev baseline (~23%).Changes:
scripts/lint/filter-warnings.sh:CheckNamespaceadded to the built-in ruleId exclusions, next to.CSharpErrors/.CppCompilerErrors.CLAUDE.md,docs/code-style-guidelines.md,.claude/skills/code-standards/SKILL.md: codify that suppressing the inspection with// ReSharper disable once CheckNamespaceis forbidden — the lint filter already ignores the inspection, so the comment is pure noise, and it hides genuine cases where a type joined the wrong namespace. Full rule lives in the style guide; the other two files carry one-line pointers.Verified against the actual InspectCode report from the latest dev run (29508710993): the filtered count drops from 19,087 to 14,643 — exactly the 4,444
CheckNamespaceresults — and no other rule is affected.Note for the merge to dev: the warning count will drop by ~4,444, which the ratchet reads as a reduction, and the baseline re-seeds on the dev push — no
no-warning-ratchetlabel needed.Test Instructions
CI-only change — nothing to run in the client.
Expected result: the Count warnings step of the
test.ymllint job on this PR reports ~14,643 instead of ~19,087, andjq '[.[] | select(.ruleId == "CheckNamespace")] | length' filtered.jsonon theinspect-code-reportsartifact returns 0.Quality Checklist
🤖 Generated with Claude Code