[AAASM-5245] 🚨 (dashboard): Add no-restricted-syntax rule banning Record<> lookup tables - #1775
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…tables
Prevents regression on Epic AAASM-5208 (all 18 wire-keyed object-literal
lookups converted to Map/Object.create(null)): a pure AST no-restricted-syntax
rule flags any new `Record<...>` type-annotated variable initialised with a
non-empty object literal. Documents its three deliberate gaps in a comment
above the rule per AAASM-5245: (1) misses write-side `= {}` accumulators, (2)
flags legitimate narrow-union Record tables by design, forcing narrowing or a
Map, and (3) trusts the key type annotation even where nothing enforces it at
a fetch-boundary cast (AAASM-5217 audits those sites separately).
Refs AAASM-5216, AAASM-5245, AAASM-5217.
…ables The new AAASM-5245 rule's gap #2 is deliberate: it cannot distinguish a real narrow-union Record<Role, T> lookup from a wire-keyed Record<string, T> one, so it flags every Record<> literal keyed by a closed app union too. Every one of the 37 sites the rule surfaced across the existing codebase is one of these legitimate cases (or, in a couple of spots, a write-side header/tally accumulator under gap #1) — each gets a one-line eslint-disable-next-line explaining why its key type is closed/validated before this table is ever indexed, so a future violation still fails loudly instead of silently reusing an existing disable.
61b94c0 to
c25bfa5
Compare
|
Chisanan232
left a comment
Chisanan232
left a comment
There was a problem hiding this comment.
✅ LGTM — reviewed and approved by Claude Code
(Recorded as a review comment because GitHub blocks self-approval on an own-authored PR; this is the sign-off of record.)
Requirement correctness
The no-restricted-syntax rule exactly matches the ticket's spec (selector, message, properties.length>0 guard preserved as-is). All three documented gaps are captured in a comment directly above the rule in dashboard/.eslintrc.cjs, correctly referencing AAASM-5216/5245/5217.
Running the rule against the existing codebase surfaced 46 sites, not zero — this PR correctly treated that as a real signal to investigate rather than blanket-suppressing:
- 1 genuine missed site:
RoleCapabilityCards.tsx'sROLE_BADGE_TONEwas still a plain object literal. This was independently found and fixed by two parallel efforts in this session — PR #1771 (AAASM-5211) landed first; this PR's branch was rebased onto that merge and its now-duplicate hunk (fix + test) was dropped during rebase rather than reapplied, confirmed viagit diff origin/main...HEADshowing zero remaining touches to eitherRoleCapabilityCards.tsxfile, and a clean post-rebase local run (272 files / 3022 tests, same count as post-#1771-merge — no orphaned duplicate test). - 45 legitimate suppressions: spot-checked a representative sample (
StatusChip.tsx,trace/decision.ts'sVERDICT_META/STATUS_META, several closed-union display tables) — every one has a specific, individualized, checkable justification (closed app union vs. raw wire string, or naming the exact validator function that guards the wire value beforehand) rather than a blanket disable. None just assert "safe" without a reason.
CI — fully green
17/17 checks pass post-rebase, including Dashboard (type-check + lint), Dashboard tests + coverage, Dashboard e2e (Playwright), aa-cli build compat, SonarCloud ×2, CodeQL, and CI Success.
Security
This closes the prevention half of Epic AAASM-5208 — no new risk, and it's the mechanism that stops the whole defect class from being reintroduced.
Merging with a merge commit as org admin.
🤖 Reviewed with Claude Code
1 participant
Description
Adds the
no-restricted-syntaxESLint rule from AAASM-5245 todashboard/.eslintrc.cjs, scoped to the existinglintscript's targets (src tests .storybook,.ts/.tsx). The rule flags anyVariableDeclaratortypedRecord<...>whose initializer is a non-emptyObjectExpression— i.e. a new object-literal lookup table keyed by aRecord<>type. This is the prevention half of Epic AAASM-5208: all 18 wire-data-keyed object-literal lookups identified by that Epic have already been converted toMap/Object.create(null)across prior merged PRs (AAASM-5211 through 5215); this rule stops a new one from being reintroduced.Pure AST rule — no
@typescript-eslinttype-aware parser project required.The three documented gaps (AAASM-5245's title)
Documented in a comment directly above the rule in
dashboard/.eslintrc.cjs:= {}accumulators — deliberately, via the rule's ownproperties.length>0predicate, so legitimate patterns likeconst headers: Record<string, string> = {}aren't flagged.Record<SomeUnion, T>tables that could be narrowed further or converted toMap— treated as a feature, not a false positive, since the rule can't distinguish a real union key from a raw wire key from AST alone.as Tcast at a fetch boundary. AAASM-5217 (a sibling ticket) audits exactly those cast sites separately; this rule can't see across that boundary.Running the rule against the current codebase
Running the rule flagged 46 pre-existing sites. Of those:
RoleCapabilityCards.tsx'sROLE_BADGE_TONEwas still an object literal indexed bycard.role, which is a widenedstringon the live-grants path (fromGET /api/v1/iam/roles), not the closed memberRoleunion — the exact defect class AAASM-5208 targeted. Fixed toMap, mirroring the identical fix already applied to the siblingROLE_BADGE_TONEinMemberList.tsx(AAASM-5109/5190). Added a regression test coveringconstructor/toString/__proto__/hasOwnProperty/valueOfrole ids, mirroring the existingMemberList.test.tsxcoverage.Record<>display/config tables keyed by closed app unions (already validated/allow-listed before the table is ever indexed), plus a couple of write-side header/tally accumulators. Each gets a one-line// eslint-disable-next-line no-restricted-syntaxwith a short justification of why its key is closed or non-wire, so a genuinely new violation still fails loudly.Type of Change
RoleCapabilityCardsmiss)Breaking Changes
Related Issues
Testing
RoleCapabilityCards.test.tsx— inherited-key regression test)pnpm run lint,pnpm exec tsc --noEmit,pnpm exec vitest runall clean: 272 test files / 3016 tests passing)Checklist
🤖 Generated with Claude Code