# AGENTS.md — Code Review Guidelines NexusCRM is a Salesforce-compatible CRM platform. Read this file in full before reviewing. In every review comment, answer the three guideline questions below one by one (PASS / violated + file:line). --- ## Review guidelines ### 1. Business hardcoding — P0 In platform code (`backend/internal/**` except `bootstrap/` and `controlplane/packs/`, plus `frontend/src/**`), flag any of: - Specific `__c` field name as a string literal - Specific ProfileId, Profile name, RecordType DeveloperName, or ApprovalProcess DeveloperName - Specific picklist / status / region value matched by `switch` or `if` - Tenant-specific table name (`FROM xxx__c`) - Tenant product / legal-entity / channel name string (PingCAP, TiDB, AliCloud, HubSpot, etc.) Tests and fixtures (`*_test.go`, `tests/**`) are exempt. ### 2. Salesforce metadata-driven compliance — P0 Beyond hardcoding, flag subtler collapses of the three-layer model: - `if objectName == "Lead"`-style SObject-name branches inside generic engines (records / automation / permissions). A `map[string]…` keyed on SObject API names counts the same. - Hardcoded allowlist maps like "fields writable in state X" — belongs in FieldPermissions + ValidationRule - Behavior branching on ProfileId — belongs in PermissionSet + `CheckSystemPermission` - A platform-internal substitute for a fact SF already models (`SharingModel`, `IsSharingParent`, `RecordType`, `FieldPermissions`) — read SF's representation, don't invent one. PR #1966 (issue #1970). - Two engine layers (object-level CRUD / record-level access / RLS / seeder) reading the same fact through different mechanisms — they must share one metadata helper. - Derived SObject-shape facts (cascade parent, FLS opacity, hierarchy parent) authored inside a feature module rather than metadata service. - Any tenant-specific identifier in `bootstrap/**` or anywhere under `controlplane/**` **except** the tenant-quarantine directory `backend/internal/controlplane/tenants//`. The platform binary applies by default; tenant overlays apply only when the deployment opts in via `NEXUSCRM_APPLY_TENANT_OVERLAY=1` and `cmd/setup apply-tenant`. Greenfield orgs never run that step, so tenant data in the quarantine directory never reaches their `_System_*` tables. Tenant identifiers outside the quarantine are always P0. The decisive test: **can an admin change this business rule without a code change?** If no → violation. **Platform boundary:** backend platform code is the Salesforce-compatible engine. Business workflows and tenant rules must be authored through APIs, metadata, and data (objects, fields, ValidationRules, Flows, permissions, ApprovalProcesses), not embedded in generic runtime or control-plane code. **Legacy remediation boundary:** one-time cleanup for historical bad data or obsolete physical storage belongs in `data-specs/`, not in long-lived platform code. Do not add runtime, generic-engine, or control-plane special cases just to clean up a repository-specific historical mistake. The platform must keep the generic metadata-driven model; the data spec handles the legacy debt. ### 3. Refactor discipline — P1 (narrow triggers only) Apply **only** when the PR matches one of these specific triggers (not a generic "big PR" check): - **SSoT cutover** — a read path is being flipped to a different storage - **Staged refactor** — one PR in a multi-PR refactor with deferred tails - **Destructive migration** — `DROP COLUMN` / `DELETE FROM` / `TRUNCATE` combined with a backfill step - **Shared-state bug fix** — a bug fix in permissions, metadata, records, or any subsystem whose state is read by multiple paths If any trigger matches, read `docs/lessons/2026-04-23-permission-ssot-cutover.md` before approving. Use R1–R10 as a checklist. When you file a blocking comment, cite the rule number and quote the specific evidence the rule requires — not the rule itself. The framing is "show me the evidence / name the mechanism", not "the rule says no", except for R9 (backfill + destructive in one spec) where the failure mode is irreversible. ### 4. Do a comprehensive code review to find valuable issues — severity per Codex judgment Read as much as you can to find issues introduced by PR, you have unlimited read quotas and available contexts. When you are not sure about something, you must study the code until you figure out. ### 5. Design doc review — mandatory for design-only PRs For design docs / ADRs / PRDs, do not approve unless all six are clear: 1. How does Salesforce solve this, and what candidate paths exist? 2. What repo evidence was checked (`file:line`)? 3. What deltas from Salesforce are declared? 4. Why is each delta justified? 5. What changes for the next tenant? 6. How are the claimed properties verified? If any answer is missing, send the doc back. Treat unsupported claims as **Unverified**. Review parity at the **product boundary**, not engine identity: - Start from the Salesforce baseline, then judge external contract, authoring model, install topology, and operational visibility - Keep the implementation metadata-driven - Reuse existing primitives first; extensions must be generic metadata-driven platform capabilities - Do not invent a new primitive until Salesforce paths, current platform paths, and reusable extension paths have been exhausted - Engine differences are not themselves findings; undeclared product-boundary drift is - Any product-boundary delta must be declared, justified, and verifiable - Apex/script parity rule: Salesforce uses Apex to carry installable business code. NexusCRM does not clone the Apex language, sandbox, or managed-package executable-code model. NexusCRM "script" has a current implementation subset (Flow metadata, Formula/Expression evaluation, and Go-backed Invocable Actions) and a broader design target: Apex product-role capability equivalence through AI-agent-friendly metadata/script orchestration plus extensible Go-backed built-in functions/actions. Classify each Apex surface as implemented, designed extension path, unsupported gap, or out of target. Do not treat "not implemented today" as "not in the design"; do not treat "architecturally implementable" as "already implemented." REST endpoint authoring, callout orchestration, and complex algorithms can be valid extension paths when designed as generic metadata-driven route/action/credential primitives with permissions, lifecycle, observability, and tests. For Apex/script/Flow/Formula/Invocable Action or package-executable comparisons, read `docs/design/script-layer-apex-positioning.md` in full before judging parity. --- ## Out of scope — do not flag - Style, naming, formatting (`gofmt`, ESLint, Prettier, `tsc` cover these) - Refactors of code outside this PR's scope - Micro-optimizations without benchmark evidence - Missing comments on straightforward code --- ## Metadata-Driven Architecture — Formal Definition This section is the formal definition of metadata-driven architecture for NexusCRM, kept in AGENTS.md so review agents can decide without context-switching to CLAUDE.md. The four commitments are necessary conditions; the three decision tests are decidability procedures (each test checks specific commitments); R1–R11 below are consequences; the *Coding Red Lines* tables further down are the enforcement layer with grep / Q&A checklists. > **The platform is four layers. L0 platform code is an interpreter. > L1 standard metadata is the platform's factory defaults that ship in > the binary. L2 org metadata is admin-editable per-org configuration. > L3 record data is user-produced rows. Each layer has its own author, > lifecycle, and change cost; every fact has a unique representation > in its proper layer, read through a single path.** ### Four layers - **L0 — Platform code.** The engine. Generic over any SObject, any field, any permission. Ships via PR + deploy. Author: engineer. - **L1 — Standard metadata.** Standard SObjects, standard fields, standard permission keys (`ConvertLeads`, `ModifyAllData`, `ViewSetup`). Ships in the binary, identical for every org. Author: engineer. May carry standard names that are part of the Salesforce contract; must never carry tenant artifacts. - **L2 — Org metadata.** `__c` fields, picklist values, RecordTypes, FieldPermissions, ValidationRules, Flows, ApprovalProcesses, PermissionSets, SharingRules — rows in `_System_*` tables. Edit a row, it's live, no deploy. Author: admin. Per-org. - **L3 — Record data.** User business rows in SObject tables. Author: user. A violation = L0 code has learned a fact that belongs in L2 or L3, or L1 has learned a tenant artifact. The moment a specific `__c` name, ProfileId, picklist value, RecordType DeveloperName, tenant table, or tenant product string appears in L0 code or in L1 (`controlplane/packs/**`, `bootstrap/**`), the layers have collapsed: an admin's hot-config task now requires an engineering release. ### Four Commitments — violating any one is a P0 finding 1. **Author decides layer** — engineer-authored facts live in L0 (code) or L1 (standard metadata that ships in the binary); admin-authored facts live in L2 (`_System_*` rows); user-authored facts live in L3 (SObject rows). Misplacing a fact by one layer multiplies change cost by an order of magnitude. 2. **The L0 interpreter is blind to L2 instances** — L0 engines know schema shape only (`ObjectMetadata.Fields`, `Field.Type`, `IsCustom`), never specific instances (SObject names, `__c` field names, ProfileIds, picklist values, tenant tables, RecordType DeveloperNames). Object-specific behavior is an L2 metadata row (Trigger / Flow / ValidationRule / ApprovalProcess) dispatched by a generic L0 engine. L1 may carry standard names (`Account`, `OwnerId`, `ConvertLeads`); it must never carry tenant artifacts (`PingCAP`, `TiDB`, `AliCloud_Channel__c`). 3. **One fact, one SSoT, one read path, one owner** — for any fact SF API v63.0 already models, read SF's representation; never invent a substitute. For any `(user, object, record)` decision, every layer — object CRUD, record access, query RLS, seeder — goes through the same metadata helper; agreement across paths today does not save you, multiple paths *is* the defect. Facts derived from SObject shape live in the metadata service; consuming features call helpers and never own a shape registry of their own. 4. **Exhaust reuse before inventing** — strict order: SF primitive → existing platform primitive → extend an existing primitive into a generic metadata-driven capability → new primitive. Skipping a tier without exhausting it is a defect. Any new primitive must declare its delta from SF, the justification, the impact on the next tenant, and how the claimed properties will be verified. ### Three Decision Tests — decidability procedures, apply to every PR - **Greenfield Test — checks C1 + C2.** Pretend the diff is deployed to a brand-new org with no `__c` fields, no `system_admin` profile, no tenant tables. Walk every changed code path. Anything that crashes or behaves wrong → violates Commitment 1 or 2. - **Admin Self-Service Test — checks C4.** Pretend an admin wants to change the rule this PR introduces (add a region, change a default RecordType, open a field for write). Can they do it through metadata or UI alone? If they need an engineering PR → violates Commitment 4. - **Two-Path Consistency Test — checks C3.** Count the read paths for this fact. Do they all go through the same helper? If a metadata row changes, do all paths see it instantly? If no → violates Commitment 3. **Concrete grep at PR time** (operationalizes Greenfield + Admin Self-Service): `rg '__c|ProfileId\s*[=!]==|"NA"|"APAC"|"Open Opportunity"' ` — every non-test hit needs written justification or removal. Full command set in the Grep Test below. ### What it is NOT (boundary clarifications) - **Not "externalized config."** Moving hardcoded values into a YAML while the author is still an engineer and the lifecycle is still deploy-and-restart is not metadata-driven — it is relocation. - **Not "everything goes into a database."** The interpreter itself must live in code; standard metadata must ship in the binary. The layer must match the fact, no more, no less. - **Not "no `if` statements."** Branching on schema shape (`Field.Type == Picklist`, `ObjectMetadata.IsCustom`) is correct. Branching on a specific instance (`objectName == "Lead"`, `Region == "APAC"`, `ProfileId === 'system_admin'`) is the violation. - **Not "blindly mirror Salesforce."** For facts SF has modeled, reuse SF's representation (Commitment 3). For facts SF has not modeled, extension is allowed (Commitment 4) — but the delta is declared, never silently introduced. ### Mantra (compressed canonical form) > **Author decides layer. Interpreter is blind to instances. One fact, > one SSoT, one read path. Exhaust reuse before inventing.** --- ## Rules R1–R11 — consequences of the four commitments Formal patterns whose presence in L0 platform code (`backend/internal/**` except `bootstrap/`/`controlplane/packs/`, or `frontend/src/**`) constitutes a violation. Each rule traces back to one or two commitments. Operationalized in the *Coding Red Lines* tables below. ### R1–R8 — consequences of C1 (Author decides layer) + C2 (Interpreter is blind to instances) A PR violates these if **any** appears in L0 platform code: 1. **A `__c` custom field name as a string literal** — e.g. `record["Approval_Status__c"]`, `"AliCloud_Channel__c"`, `copyField("Product__c", "Product__c")`. Fields are discovered via `ObjectMetadata.Fields`, not spelled out. 2. **A ProfileId / Profile name hardcoded for branching** — e.g. `user.ProfileId === 'system_admin'`, `if profile == "Sales"`. Use `useSystemPermission(...)` / `CheckSystemPermission(...)` against the permission key instead. 3. **A default value, RecordType DeveloperName, or approval-process DeveloperName as a Go `const` or TS literal driving business behavior** — e.g. `const defaultStatus = "Open Opportunity"`, `"New_Subscription"`, `"Lead_Convert_Approval_APAC"`. These belong in `LeadConvertSettings` / `ApprovalProcess` metadata rows. 4. **A business enum literal the code pattern-matches on** — e.g. `switch region { case "NA": … case "APAC": … }`, `if status == "Activated"`. Regions/stages/statuses are picklist values resolved from metadata; approval-process selection is driven by `entryCriteria`. 5. **A tenant-specific table name in SQL or code** — e.g. `FROM country_dictionary__c`, or any `__c` table reference in platform code. Platform code queries only schema-generic or `_System_*` tables. 6. **Field-level write/read policy hardcoded in code** — e.g. a Go map of "fields writable after X". Use FieldPermissions + ValidationRule + RecordType metadata. 7. **SObject-name switches in generic engines** — `if objectName == "Lead" { … }` inside records / automation / permissions is a code smell. A `map[string]…` keyed on SObject API names counts the same. 8. **Status string literals driving branches** — `"Approved"`, `"Pending Conversion"`, `"In Progress"` in platform code. Status transitions are picklist + ApprovalProcess + StateModel metadata. ### R9–R11 — consequences of C3 (One fact, one SSoT, one read path, one owner) These elaborate C3 with formal patterns and concrete counter-examples. Always suspicious in L0 code; require explicit justification: 9. **Parallel SSoT for an SF-modeled fact** — when SF API v63.0 already models a fact (`SharingModel`, `IsSharingParent`, `RecordType`, `FieldPermissions`), the platform reads SF's representation; it does not invent a substitute (no `implicitCRUDParent` column, no Go cascade-children registry). PR #1966 (issue #1970) is the canonical counter-example. 10. **Cross-layer asymmetry** — for a given `(user, object, record)` tuple, object-level CRUD, record-level access, query-level RLS, and seeder must reach the same decision via the same metadata helper. Two layers reading the same fact through different mechanisms is a defect *even when they currently agree*. 11. **Module ownership of shape facts** — derived facts about SObject shape (cascade parent, FLS opacity, formula reference graph, hierarchy parent) live in metadata service; consuming features (permissions, records, automation) call metadata-service helpers and own only their own decisions. A registry of SObject-shape facts inside a feature module is a module-boundary violation. ### #1574 contrast table (the canonical counter-example) PR #1574 (reverted in `a2ef61dc`) is the canonical violation pattern. Same business outcome, different layer placement: | Need | Metadata-driven (correct) | #1574 anti-pattern (rejected) | |---|---|---| | Whitelist fields writable after Lead is converted | `FieldPermissions` rows + `ValidationRule` with `ISCHANGED(nonWhitelistField) && IsConverted` | Go map `convertedLeadWritableFields = {"Status","Approval_Status__c","Approval_Reason__c"}` in `persistence_service.go` | | Country → Region → which approval process | `Region__c` picklist + Formula or Flow mapping; `ApprovalProcess.entryCriteria` selects the process | Go SQL `FROM country_dictionary__c` + map literal `{"NA":"...APAC":"...","Japan":"..."}` baked into seeds | | System admins skip Opportunity creation on convert | New permission `SkipOpportunityOnConvert`, granted via `PermissionSet`; frontend calls `useSystemPermission(...)` | `runtime?.user?.ProfileId === 'system_admin'` string match in the React modal | Common shape of every #1574 violation: **platform code learned a fact about one specific org**. The fix is always the same — push the fact down into metadata rows and let the generic engine read it. --- ## Coding Red Lines — Mandatory Review Standard **ANY violation = instant rejection (BLOCK).** ### Six Verdict Standards (R1–R6) — CHECK EVERY PR Operationalizes R1–R6 (consequences of C1 + C2) as a grep-friendly review checklist. | # | Question | SF-correct answer | Violation = BLOCK | |---|----------|-------------------|-------------------| | 1 | Can a `__c` field name appear in platform Go/TS code? | ❌ No | `AliCloud_Channel__c` hardcoded | | 2 | Can ProfileId be used for behavior branching? | ❌ No | `ProfileId === 'system_admin'` | | 3 | Where do default values / RecordTypes belong? | Config/metadata | Go const hardcodes "Open" | | 4 | Can business enums be switch-case'd in code? | ❌ No | `{"NA":"...","EMEA":"..."}` | | 5 | Can tenant tables enter platform SQL? | ❌ No | `FROM country_dictionary__c` | | 6 | Who decides field writability? | FieldPermissions | Go map hardcodes field list | ### Five Judgment Standards (R7–R11) — CHECK EVERY PR Operationalizes R7–R11 (consequences of C2 and C3) as a Q&A review checklist. These are not pure-grep detectable; they require judgment + targeted code reading. Apply on every non-trivial PR, especially anything touching permissions, records, automation, metadata service, or cross-replica state. | # | Question | SF-correct answer | Violation = BLOCK | |---|----------|-------------------|-------------------| | 7 | Does generic engine code branch on a specific SObject API name? | ❌ No | `if objectName == "Lead"` or `map[string]X` keyed on SObject name | | 8 | Does platform code branch on a status string literal? | ❌ No | `if status == "Approved"`, `strings.Contains(rt, "master agreement")` | | 9 | Does the diff invent a parallel SSoT for an SF-modeled fact? | ❌ No | New `implicitCRUDParent` column, Go cascade-children registry, or any substitute for `SharingModel` / `IsSharingParent` / `RecordType` / `FieldPermissions` | | 10 | Does the same `(user, object, record)` decision have multiple read paths through different mechanisms? | ❌ No | Object-CRUD, record access, query RLS, seeder reading the same fact via different helpers. Multiple paths is the defect *even when answers currently agree* | | 11 | Does a feature module own SObject-shape derived facts? | ❌ No | A registry of SObject-shape facts inside `permissions/`, `records/`, `automation/`, or any feature module | ### Grep Test — RUN ON EVERY PR ```bash grep -r "__c" backend/ frontend/ --include="*.go" --include="*.ts" | grep -v "_test.go" grep -r "ProfileId ===" backend/ frontend/ | grep -v "_test.go" grep -r '"NA"|"EMEA"|"APAC"|"Japan"' backend/ frontend/ | grep -v "_test.go" grep -r '"Open"|"Prospecting"' backend/ frontend/ --include="*.go" | grep -v "_test.go" ``` Non-test-code hits = BLOCK. ### Greenfield Test (checks C1 + C2) If this PR deploys unchanged to another org with no `__c` fields, no `system_admin` profile, no tenant tables: - Would it run? If NO → BLOCK - Fields don't exist → breaks? BLOCK - Profile isn't called `system_admin` → breaks? BLOCK ### Admin Self-Service Test (checks C4) If an admin wants to change the business rule this PR introduces, can they do it via UI / metadata / config alone? If NO → BLOCK. ### Two-Path Consistency Test (checks C3) For any fact this PR introduces or touches: count the read paths. Do they all go through the same metadata helper? If a metadata row changes, do all paths see it instantly? If NO → BLOCK. *Multiple paths is the defect even when answers currently agree.* --- ## References - `CLAUDE.md` — full architecture source. The Core Definition above mirrors CLAUDE.md's spine; teaching elaboration (PingCAP Concrete scenario, Salesforce three-primitive explanation, four-axiom system, Refactor Discipline, Key Files index, tenant-quarantine deep dive) lives in CLAUDE.md only. - `docs/design/script-layer-apex-positioning.md` — separates current implementation from target design, defines NexusCRM script as Flow + Formula/Expression + Go-backed built-in functions/actions, and explains Apex product-role capability equivalence without Apex runtime compatibility. - `docs/lessons/2026-04-23-permission-ssot-cutover.md` — the incident behind the SSoT discipline R10 encodes. Read before objecting to any R9–R11 finding on a permission/metadata/records change.