إنتقل إلى المحتوى الرئيسي

REVIEW.md — RaR-IT SaaS Kernel & Verticals

This file guides Claude Code Review. Unlike CLAUDE.md (general project context, violations surface only as nits), everything here is read directly by every finding and verification agent — treat these as the project's actual merge standards, sourced from the SaaS Architecture Policy and Annex A.

Reference: /docs/policies/saas-architecture/policy and /annex-a on the governance site. Cite the section number when flagging a violation so the author can look it up.

Check against the PR's story and acceptance scenarios​

The PR description's "Story" and "Acceptance Scenarios (Given-When-Then)" sections (see .github/PULL_REQUEST_TEMPLATE.md) are the source of truth for what this change is supposed to do — read them before reviewing the diff, not just after.

  • If a listed acceptance scenario has no code path in the diff that appears to satisfy it, flag this as a Missing scenario coverage finding (Important) — name the specific scenario, not a generic "add more tests" comment.
  • If the diff implements behavior that contradicts a listed scenario (e.g. a Given-When-Then implies a validation error should be returned, but the code silently accepts the case), flag as Important and quote the scenario it conflicts with.
  • If the Story/Acceptance Scenarios section is empty or missing entirely, report this once, as a nit, rather than skipping the intent-check — don't guess at intent from the diff alone.
  • Do not flag scenarios that are explicitly out of scope per the PR's "Notes for the reviewer" section (e.g. deliberately deferred to a follow-up PR) — check there before flagging a scenario as missing.

Escalate to Important​

  • Rejected DB-access patterns (Policy §6, A.10). Any fastify.decorate('db', ...), or a raw DB client / transaction object passed as an explicit function parameter, instead of getDb() / withTenantScope() from @rarit-kernel/platform-core. This is a formally rejected pattern, not a style preference.

  • Direct vertical-to-vertical imports (Policy §4). Any import from one vertical into another (e.g. verticals/tourism importing from verticals/amaar), in either direction — even with no cycle. import/no-cycle in CI cannot catch this one-way case; only review can. Correct fix: promote the shared logic to a @rarit-kernel/* package (see Annex A.8).

  • New engine-to-engine import that isn't already one-directional (Policy §4, A.7). If a change adds an import from Engine A into Engine B, check whether B already imports A anywhere in the workspace. If so, this is a reintroduction of the exact IAM↔Notifications shape A.7 exists to prevent — flag it even if CI's no-cycle job hasn't run yet or the cycle is indirect (routed through a third package).

  • RLS enabled without FORCE (Annex A.11.3). Any new ENABLE ROW LEVEL SECURITY on a tenant-scoped table not paired with FORCE ROW LEVEL SECURITY. ENABLE alone does not protect against a table owner or superuser role bypassing the policy — this is the exact anti-pattern documented in A.11.3 from the old system.

  • Cross-tenant reads outside withPlatformAdminScope() (A.11.2 / ACT-001). Any query that appears to read across tenants (missing an implicit tenant filter, a raw SELECT with no WHERE on a multi-tenant table) that isn't going through the approved withPlatformAdminScope(actor, reason, work) function with a logged reason.

  • Route handlers doing DB access directly (engine_structure.md). A DB query inside routes/, or a services/ function that accepts request/reply. Routes → services → repositories is the mandatory three-layer split; this is a layering violation, not a style nit.

  • Payment, Audit, or IAM changes with no accompanying audit write (A.1, A.10). Any mutation to Payment or IAM state (subscriptions, role grants, transactions) that doesn't write an Audit record in the same transaction as the triggering change.

Report as-is (default severity)​

  • New API routes without an integration test.
  • Manifest or event-definition changes placed under v1/ instead of core/ — a version-axis mixup per CLAUDE.md's file-structure notes, not a REST contract change.
  • New cross-engine references implemented as enforced DB-level foreign keys rather than logical FKs (Entitlements → IAM's tenants.id should stay a logical reference, per the ownership note in project memory).

Nit only (CLAUDE.md-level, don't escalate)​

  • General CLAUDE.md footguns already covered there (brace-expansion mkdir bug, declare module "fastify" shadow-module trap, etc.) — these already surface as nits automatically; no need to duplicate here.
  • Naming/formatting inconsistencies with no functional effect.

Skip​

  • *.md, docs/**, signed-records/** — documentation and controlled copies, not code.
  • Generated files: openapi.json, anything under dist/ or build/.

Nit volume​

Report at most five nits per review; mention any remainder as a count in the summary rather than listing them all.

Notes for reviewers​

Findings here are informational — Code Review does not approve or block PRs. This is intentional and matches the team's current trust-based merge process (see Actions ACT-005). If that changes later (e.g. GitHub plan upgraded to enable required status checks), this file doesn't need to change — only the branch-protection configuration does.