From 591526a2068916e0d5e73e8b6c746237061094f9 Mon Sep 17 00:00:00 2001 From: ali Date: Wed, 8 Apr 2026 15:48:30 +0530 Subject: [PATCH 1/7] UN-3396 [MISC] Introduce design-rules system with prototype per-component files Introduce a file-based, version-controlled design-rules system at design-rules/ following the AGENTS.md nested-discovery convention, plus 5 prototype per-component DESIGN_RULES.md files in the canonical shape defined by design-rules/per-component-contract.md, plus the design-rules Claude skill that exposes list/get/add/update/review/validate operations over the ruleset. The 5 prototype files each exercise a different dimension of the contract: account_v2 (tenant-graph root), workflow_manager (anchor entity with audit-durable on_delete rule), connectors (parent shared library), connectors/databases (deeply-nested component specialising SQL Safety Standard S1), and workers/shared (worker no-Django context with the UNS-205 HTTP session lifecycle rule). Also fixes a bug in the skill validator where the Compatibility-blockquote grep pattern was looking for "Compatibility: All" which never matched the contract's bold form "**Compatibility:** All". Co-Authored-By: Claude Opus 4.6 (1M context) --- .claude/skills/design-rules/SKILL.md | 204 ++++++++++++++++++ .../skills/design-rules/scripts/validate.sh | 65 ++++++ .gitignore | 2 + backend/account_v2/DESIGN_RULES.md | 75 +++++++ backend/workflow_manager/DESIGN_RULES.md | 84 ++++++++ design-rules/README.md | 65 ++++++ .../adr/ADR-001-org-scoping-query-layer.md | 20 ++ .../adr/ADR-002-no-org-fk-on-execution.md | 16 ++ design-rules/adr/ADR-003-usage-string-refs.md | 15 ++ ...005-prompt-studio-registry-publish-gate.md | 17 ++ ...-006-organization-rate-limit-separation.md | 15 ++ ...R-007-adapter-access-runtime-validation.md | 15 ++ .../adr/ADR-012-connectorauth-user-owned.md | 15 ++ ...DR-014-internal-external-api-separation.md | 20 ++ design-rules/adr/README.md | 66 ++++++ design-rules/ai-review-checklist.md | 76 +++++++ design-rules/definition-of-done.md | 48 +++++ design-rules/lifecycle.md | 51 +++++ design-rules/per-component-contract.md | 195 +++++++++++++++++ design-rules/principles.md | 97 +++++++++ design-rules/security/standards.md | 37 ++++ design-rules/security/tenant-isolation.md | 66 ++++++ unstract/connectors/DESIGN_RULES.md | 83 +++++++ .../connectors/databases/DESIGN_RULES.md | 93 ++++++++ workers/shared/DESIGN_RULES.md | 93 ++++++++ 25 files changed, 1533 insertions(+) create mode 100644 .claude/skills/design-rules/SKILL.md create mode 100755 .claude/skills/design-rules/scripts/validate.sh create mode 100644 backend/account_v2/DESIGN_RULES.md create mode 100644 backend/workflow_manager/DESIGN_RULES.md create mode 100644 design-rules/README.md create mode 100644 design-rules/adr/ADR-001-org-scoping-query-layer.md create mode 100644 design-rules/adr/ADR-002-no-org-fk-on-execution.md create mode 100644 design-rules/adr/ADR-003-usage-string-refs.md create mode 100644 design-rules/adr/ADR-005-prompt-studio-registry-publish-gate.md create mode 100644 design-rules/adr/ADR-006-organization-rate-limit-separation.md create mode 100644 design-rules/adr/ADR-007-adapter-access-runtime-validation.md create mode 100644 design-rules/adr/ADR-012-connectorauth-user-owned.md create mode 100644 design-rules/adr/ADR-014-internal-external-api-separation.md create mode 100644 design-rules/adr/README.md create mode 100644 design-rules/ai-review-checklist.md create mode 100644 design-rules/definition-of-done.md create mode 100644 design-rules/lifecycle.md create mode 100644 design-rules/per-component-contract.md create mode 100644 design-rules/principles.md create mode 100644 design-rules/security/standards.md create mode 100644 design-rules/security/tenant-isolation.md create mode 100644 unstract/connectors/DESIGN_RULES.md create mode 100644 unstract/connectors/src/unstract/connectors/databases/DESIGN_RULES.md create mode 100644 workers/shared/DESIGN_RULES.md diff --git a/.claude/skills/design-rules/SKILL.md b/.claude/skills/design-rules/SKILL.md new file mode 100644 index 0000000000..8a343830db --- /dev/null +++ b/.claude/skills/design-rules/SKILL.md @@ -0,0 +1,204 @@ +--- +name: design-rules +description: Manage and apply the file-based design rule system in this repo (design-rules/ and per-component DESIGN_RULES.md). Use when the user says "show design rules", "design rules for [component/path/file/PR]", "which rules apply to X", "add design rule", "add ADR", "update design rule", "review [code/PR] against design rules", "list ADRs", "list principles", or "validate design rules". Operations: list, get, add, update, review, validate. +compatibility: Designed for Claude Code. Requires git, gh, find, grep, ripgrep. +allowed-tools: Read Glob Grep Edit Write Bash(git:*) Bash(gh:*) Bash(find:*) Bash(grep:*) Bash(ls:*) +--- + +# Design Rules Skill + +A skill for managing and applying the version-controlled design rule system in this repo. The system has two layers: global rules in `design-rules/` and per-component `DESIGN_RULES.md` files inside each active Django app, each `unstract/` shared lib component, and each worker. The system is OSS-only — it must never reference cloud / enterprise / pluggable_apps / HITL / subscription / agentic / or any specific PR review tool. + +--- + +## Quick Start + +### Basic Commands + +| What you want | What to say | +|---------------|-------------| +| List principles | "list principles", "list design principles" | +| List ADRs | "list ADRs", "show all ADRs" | +| List components with rules | "list components with rules", "which components have design rules" | +| Get rules for a file/dir/component | "design rules for backend/connector_v2", "which rules apply to api_v2/views.py" | +| Get rules for a PR or local diff | "design rules for PR #1902", "design rules for my changes" | +| Get all global rules | "show all design rules" | +| Add a new ADR | "add ADR for X" | +| Add a per-component rules file | "add DESIGN_RULES.md for backend/foo" | +| Update an existing rule | "update design rule for X", "rule about Y changed" | +| Review code against the rules | "review my changes against design rules", "check PR #1902 against design rules" | +| Validate the rule system itself | "validate design rules" | + +### What you'll get + +| Mode | Output | +|------|--------| +| **list** | Compact text list (titles + numbers, no bodies) | +| **get** | Deduped bundle: compatibility statement, relevant principles, AI Review Checklist, referenced ADRs, security standards, component Dos/Don'ts | +| **add** | New file written + cross-links updated; refused if content describes an aspiration rather than implemented behaviour | +| **update** | Minimal in-place edit; ADR superseded with a new ADR if the change is a reversal | +| **review** | Per-file findings table: file, line, principle/ADR violated, why, minimal fix | +| **validate** | Pass/fail report from forbidden-word grep, compatibility-statement check, ADR-link resolution | + +--- + +## Related Skills + +| Skill | Relationship | +|-------|--------------| +| **code-reviewer** | code-reviewer can call `design-rules review` to add design-rule checks on top of its SOLID/security review | +| **pr-creator** | pr-creator can call `design-rules get` to surface applicable rules in the PR description | +| **jira-manager** | jira-manager can call `design-rules get` when planning implementation of a ticket | +| **architecture-analyst** | architecture-analyst consults `design-rules/principles.md` and ADRs when documenting how a component works | + +--- + +## Command: `list` + +Show what rules exist, titles only — fast and cheap. + +| Sub-command | Action | +|---|---| +| "list principles" | Read `design-rules/principles.md`, return P1–P9 titles only. | +| "list ADRs" | Read `design-rules/adr/README.md`. If missing, `ls design-rules/adr/ADR-*.md` and return numbers + titles. | +| "list components with rules" | `find backend unstract workers -name DESIGN_RULES.md`. Return grouped by directory. | +| "list global rules" | `ls design-rules/ design-rules/security/ design-rules/adr/`. | + +Never dump full file bodies in `list` mode — that's `get`. + +--- + +## Command: `get` + +Fetch the rules that actually apply to a target. + +**Steps:** +1. **Resolve the target paths.** Accept any of: a single file path, a directory, a component name (e.g. `connector_v2`), a glob, or a list of files (from `gh pr diff --name-only` or `git diff --name-only`). +2. **For each target file**, walk from the file's directory upward to the repo root and collect every `DESIGN_RULES.md` along the way. Always include `design-rules/README.md`, `design-rules/principles.md`, and `design-rules/ai-review-checklist.md`. +3. **Read the collected files** and present them in this order: + - Compatibility statement (once) + - Principles referenced by any collected component file's "Read first" + - AI Review Checklist (always) + - ADRs referenced by any collected component file + - Security standards referenced (e.g. `security/tenant-isolation.md`, `security/standards.md`, the inline SQL Safety Standard under `unstract/connectors/.../databases/`) + - The component-specific Dos / Don'ts / Acceptance criteria from each collected `DESIGN_RULES.md` +4. **Deduplicate.** Never show the same principle or ADR twice. +5. If asked for "all" design rules, dump the global files (`design-rules/**`) without per-component noise. + +--- + +## Command: `add` + +Create a new rule, ADR, or per-component file. + +**Gate before adding anything:** verify the content describes current implemented behaviour, not aspirations. The system explicitly excludes unimplemented designs (no AuditLog table, no SoftDeleteMixin, no proposed key/role architecture, etc.). If the user wants to record an aspiration, push back and suggest the issue tracker or Confluence instead. + +### Add a global principle or refinement +- New principle → edit `design-rules/principles.md`, take the next free `P` number, **and** update `design-rules/ai-review-checklist.md` in the same change to add a question for it. +- Refinement of an existing principle → edit it in place; do not duplicate. + +### Add an ADR +1. `ls design-rules/adr/ADR-*.md` to find the next free number. +2. Create `design-rules/adr/ADR-NNN-.md`: + ``` + # ADR-NNN: + + ## Status + Accepted + + ## Context + <why this decision is needed; what's currently true> + + ## Decision + <what was decided> + + ## Consequences + <what becomes easier / harder / required> + ``` +3. Add a one-line entry to `design-rules/adr/README.md`. +4. Cross-link from the relevant per-component `DESIGN_RULES.md` files in their "Read first" section. + +### Add a per-component `DESIGN_RULES.md` +1. Verify the directory exists on disk and contains real source (not just `__pycache__`). +2. Use the standard template from `design-rules/README.md`. Always include the verbatim compatibility statement. +3. Include "Read first" links to `principles.md`, `ai-review-checklist.md`, and any relevant ADRs / security standards. +4. If the component has no specific Dos / Don'ts in source material, write the boilerplate body with `No component-specific rules beyond the global principles.` + +--- + +## Command: `update` + +Apply new changes to existing rules. Triggered by "update design rule for X", "the rule about Y changed", or when implementation drift is detected during review. + +1. Identify the affected files: principle file, ADR(s), per-component file(s). +2. Make minimal edits in place. Never duplicate content between global and component files. +3. If a previously-recorded rule is now wrong because the implementation changed, **update the rule, do not add a contradicting one**. If the change is significant enough to be a decision reversal, supersede the relevant ADR by adding a new ADR and marking the old one's Status as `Superseded by ADR-NNN` (the only allowed Status besides `Accepted`). +4. If a per-component file is updated, also check whether the global ADR / principle / security standard it references still matches. Fix upstream first if both are wrong. + +--- + +## Command: `review` + +Check code or a diff against the design rules. Triggered by "review against design rules", "check this PR against design rules", "does X violate any rule". + +1. **Determine the target.** Accept: a list of changed files, a PR number (use `gh pr diff <n> --name-only`), `git diff --name-only` for local changes, or a single file. +2. **Run `get`** on those targets to collect all applicable rules. +3. **For each changed file**, walk through the AI Review Checklist questions (`design-rules/ai-review-checklist.md`) and the component's Dos / Don'ts. For every violation, output: + - File and line (when known) + - Principle / ADR / security standard violated + - Why it's a violation + - Minimal fix +4. **Pay special attention to known traps:** + + | Trap | What to look for | Reference | + |---|---|---| + | Tenant isolation bypass | New model or queryset bypassing org scoping | `security/tenant-isolation.md` (Three-Layer Defense) | + | SQL injection | String interpolation into SQL inside `unstract/connectors/.../databases/**` | inline SQL Safety Standard S1 in that directory's `DESIGN_RULES.md` | + | Missing auth | New endpoint without authentication | P8 fail-closed | + | Direct org FK on derived model | New model that doesn't inherit org through a parent | P6, ADR-002 | + | Audit/billing CASCADE | New audit/billing record using FK CASCADE to parent | P4, ADR-003 | + | Credential duplication | Storing the same credential twice instead of referencing | P2 | + +5. **Do not invent rules.** If a pattern looks suspicious but no rule covers it, say so explicitly and recommend opening a tracker entry instead of pretending a rule exists. + +--- + +## Command: `validate` + +Sanity-check the rule system itself. Triggered by "validate design rules", "check design rules consistency". + +Run the bundled script from the repo root: + +```bash +.claude/skills/design-rules/scripts/validate.sh +``` + +The script checks: forbidden-word scan, compatibility-statement presence in every per-component file, and that each per-component file's directory contains real source. Exit code 0 = clean; 1 = problems found. Report any hits as fix-required and offer to fix them in place. + +--- + +## What this skill must NOT do + +- Never add debt items, vulnerability reports, or open design questions to the repo. Those belong in the issue tracker and the private security channel. +- Never document a proposed / unimplemented design as if it were a rule. +- Never name a specific PR review tool inside any rule file. +- Never duplicate global rule content into per-component files. +- Never create a `DESIGN_RULES.md` for `pluggable_apps/*`, `workers/pluggable_worker`, or `frontend/`. + +--- + +## Path reference + +| Path | Purpose | +|---|---| +| `design-rules/README.md` | Entry point: navigation, how-to recipes, and the AI-loading instruction (load `DESIGN_RULES.md` from the dir of any changed file). | +| `design-rules/principles.md` | P1–P9. | +| `design-rules/ai-review-checklist.md` | 9 questions every change must answer "yes" to. | +| `design-rules/lifecycle.md` | Design → assembly → deploy → runtime → monitoring. | +| `design-rules/security/tenant-isolation.md` | Three-Layer Defense. | +| `design-rules/security/standards.md` | SQL Safety S1 + current protection patterns as rules. | +| `design-rules/adr/ADR-*.md` | 8 accepted ADRs. | +| `unstract/connectors/src/unstract/connectors/databases/DESIGN_RULES.md` | SQL Safety Standard S1 inlined for the 8 DB connectors. | +| `backend/<app>/DESIGN_RULES.md` × 22 | Per-Django-app component files. | +| `unstract/<component>/DESIGN_RULES.md` × 9 | Per-shared-lib component files. | +| `workers/<component>/DESIGN_RULES.md` × 10 | Per-worker component files. | diff --git a/.claude/skills/design-rules/scripts/validate.sh b/.claude/skills/design-rules/scripts/validate.sh new file mode 100755 index 0000000000..dc54d16aa9 --- /dev/null +++ b/.claude/skills/design-rules/scripts/validate.sh @@ -0,0 +1,65 @@ +#!/usr/bin/env bash +# Validate the design rules system. Run from the repo root. +# +# Checks: +# 1. No forbidden words appear in any design rule file (cloud, enterprise, +# HITL, agentic, vendor names, unimplemented design names, etc.). +# 2. Every per-component DESIGN_RULES.md contains the verbatim compatibility +# statement. +# 3. Every per-component DESIGN_RULES.md lives in a directory that exists +# and contains real source (not just __pycache__). +# +# Exit codes: 0 = clean, 1 = problems found. + +set -u +fail=0 + +ROOT="$(git rev-parse --show-toplevel 2>/dev/null || pwd)" +cd "$ROOT" || exit 2 + +echo "==> 1. Forbidden-word scan" +FORBIDDEN='cloud|enterprise|unstract-cloud|hitl|manual.review|subscription|agentic|simple.prompt.studio|public.shares|greptile|AuditLog|SoftDeleteMixin|departure workflow|GDPR|OWNER role|ManagementKey|ExecutionKey|uxm_|uxe_|deletion guard|informed deletion' + +mapfile -t COMPONENT_FILES < <(find backend unstract workers -name DESIGN_RULES.md 2>/dev/null) + +if grep -rEl -i "$FORBIDDEN" \ + design-rules/ "${COMPONENT_FILES[@]}" 2>/dev/null; then + echo " FAIL: forbidden words found in the files listed above" + fail=1 +else + echo " OK" +fi + +echo +echo "==> 2. Compatibility statement presence" +COMPAT='All changes to this component must remain compatible' +for f in "${COMPONENT_FILES[@]}"; do + if ! grep -q "$COMPAT" "$f"; then + echo " MISSING: $f" + fail=1 + fi +done +[ $fail -eq 0 ] && echo " OK" + +echo +echo "==> 3. Component directory sanity" +for f in "${COMPONENT_FILES[@]}"; do + dir="$(dirname "$f")" + # Real source = at least one file other than DESIGN_RULES.md and __pycache__ + real=$(find "$dir" -maxdepth 1 -mindepth 1 \ + ! -name DESIGN_RULES.md ! -name __pycache__ 2>/dev/null | head -1) + if [ -z "$real" ]; then + echo " EMPTY DIR: $f (no real source alongside)" + fail=1 + fi +done +[ $fail -eq 0 ] && echo " OK" + +echo +if [ $fail -eq 0 ]; then + echo "All design rule checks passed." + exit 0 +else + echo "Design rule checks FAILED. Fix the items listed above." + exit 1 +fi diff --git a/.gitignore b/.gitignore index f6837ce079..6b35a1bef7 100644 --- a/.gitignore +++ b/.gitignore @@ -693,6 +693,8 @@ CLAUDE.md !.claude/skills/ .claude/skills/* !.claude/skills/worktree/ +!.claude/skills/design-rules/ +!.claude/skills/design-rules/** CONTRIBUTION_GUIDE.md .mcp.json diff --git a/backend/account_v2/DESIGN_RULES.md b/backend/account_v2/DESIGN_RULES.md new file mode 100644 index 0000000000..a88064a7dc --- /dev/null +++ b/backend/account_v2/DESIGN_RULES.md @@ -0,0 +1,75 @@ +# account_v2 — Design Rules + +`account_v2` owns the tenant-graph root — `Organization`, `User`, and `OrganizationMember`. Every other tenant-scoped model in this repo ultimately resolves to an `Organization` defined here. + +> **Compatibility:** All changes to this component must remain compatible with extension by an external layer that runs this codebase alongside additional Django applications sharing the same database. + +This file follows the [per-component contract](../../design-rules/per-component-contract.md) (rule format, severity vocabulary, Known Exceptions, M1/M2/M3 meta-rules). + +--- + +## Scope + +| | | +|---|---| +| **Covers** | `backend/account_v2/**` — `Organization`, `User`, `OrganizationMember`, their managers, serializers, views, signals, and the auth middleware glue | +| **Excludes** | Usage rollups → [`account_usage`](../account_usage/DESIGN_RULES.md). Tenant-scoped data in other apps → each app's own `DESIGN_RULES.md`. | + +## Read first + +| File | Why it binds here | +|---|---| +| [`principles.md`](../../design-rules/principles.md) | P1 (org scoping), P2 (credentials), P8 (fail-closed) | +| [`ai-review-checklist.md`](../../design-rules/ai-review-checklist.md) | 9 questions every change must answer | +| [`security/tenant-isolation.md`](../../design-rules/security/tenant-isolation.md) | Three-Layer Defense — `account_v2` owns Layer 1 (middleware) and is the root of Layer 3 (filter backend) | +| [`adr/ADR-001`](../../design-rules/adr/ADR-001-org-scoping-query-layer.md) | Org scoping is enforced at the query layer, not via RLS | + +--- + +## Rules + +### R1 — `Organization` is the single root of the tenant graph + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Every tenant-scoped model must resolve to exactly one `Organization`, by direct FK or documented FK chain. A second root would split tenant isolation and break `OrganizationFilterBackend`'s BFS walk. | +| **Refs** | `principles.md#P1` · `security/tenant-isolation.md` · `adr/ADR-001` | +| **Enforced by** | `OrganizationFilterBackend` (Three-Layer Defense, Layer 3) + code review | + +### R2 — Active-org resolution always goes through `CustomAuthMiddleware` + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | The middleware is Layer 1 of the Three-Layer Defense — it validates the user belongs to the org and stores `org_id` in the thread-local `StateStore`. Reading the org from a header, request param, or cookie directly bypasses validation and is a tenant boundary violation. | +| **Refs** | `principles.md#P8` · `security/tenant-isolation.md` | +| **Enforced by** | `CustomAuthMiddleware` + code review | + +### R3 — Org membership has exactly one source of truth: `OrganizationMember` + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | A second membership table would let an org/user pair exist in one place and not the other, producing inconsistent authorization decisions and orphaned credentials. Membership is the anchor used by every other app to check whether a user can act inside an org. | +| **Refs** | `principles.md#P1` · `principles.md#P2` | +| **Enforced by** | code review only | + +### R4 — Credentials on `User` follow P2 (stored once, referenced by ID) + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Duplicating a credential makes rotation impossible and leaves stale copies behind on member departure. `User`-owned credentials must be referenced by ID from anywhere that consumes them, never copied. | +| **Refs** | `principles.md#P2` | +| **Enforced by** | not yet enforced — see project issue tracker (credential lifecycle) | + +--- + +## Known Exceptions + +None. + +## Checklist + +See [Definition of Done](../../design-rules/definition-of-done.md). diff --git a/backend/workflow_manager/DESIGN_RULES.md b/backend/workflow_manager/DESIGN_RULES.md new file mode 100644 index 0000000000..25e8dd0763 --- /dev/null +++ b/backend/workflow_manager/DESIGN_RULES.md @@ -0,0 +1,84 @@ +# workflow_manager — Design Rules + +`workflow_manager` owns `Workflow`, `WorkflowExecution`, `WorkflowFileExecution`, `ExecutionLog`, and the orchestration that runs workflows. `Workflow` is the anchor entity (P9) for the execution-side data graph. + +> **Compatibility:** All changes to this component must remain compatible with extension by an external layer that runs this codebase alongside additional Django applications sharing the same database. + +This file follows the [per-component contract](../../design-rules/per-component-contract.md) (rule format, severity vocabulary, Known Exceptions, M1/M2/M3 meta-rules). + +--- + +## Scope + +| | | +|---|---| +| **Covers** | `backend/workflow_manager/**` — `Workflow`, `WorkflowExecution`, `WorkflowFileExecution`, `ExecutionLog`, their managers, serializers, viewsets, signals, and the orchestration that runs workflows | +| **Excludes** | Tenant-graph root (`Organization`, `User`, `OrganizationMember`) → [`account_v2`](../account_v2/DESIGN_RULES.md). Pipeline/API deployment triggers → [`pipeline_v2`](../pipeline_v2/DESIGN_RULES.md), [`api_v2`](../api_v2/DESIGN_RULES.md). Tool instance config → [`tool_instance_v2`](../tool_instance_v2/DESIGN_RULES.md). | + +## Read first + +| File | Why it binds here | +|---|---| +| [`principles.md`](../../design-rules/principles.md) | P1 (org scoping), P4 (audit durability), P6 (execution org inheritance), P9 (anchor entity) | +| [`ai-review-checklist.md`](../../design-rules/ai-review-checklist.md) | 9 questions every change must answer | +| [`security/tenant-isolation.md`](../../design-rules/security/tenant-isolation.md) | Three-Layer Defense — execution-side viewsets live on Layer 3 | +| [`adr/ADR-002`](../../design-rules/adr/ADR-002-no-org-fk-on-execution.md) | Execution-side models must not carry a direct `organization` FK | + +--- + +## Rules + +### R1 — `Workflow` is the anchor entity for the execution-side data graph + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Every execution-side model (`WorkflowExecution`, `WorkflowFileExecution`, `ExecutionLog`) must derive org through `Workflow` rather than duplicating `organization_id`. A parallel direct FK would split tenant isolation and let the two paths disagree. | +| **Refs** | `principles.md#P6` · `principles.md#P9` · `adr/ADR-002` | +| **Enforced by** | `OrganizationFilterBackend` (BFS walk) + code review | + +### R2 — Execution-side viewsets scope reads through `OrganizationFilterBackend` + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Bypassing the filter backend with raw querysets (`.objects.all()`, `.objects.filter(workflow__id=...)` without org scoping) opens a tenant-isolation hole. The filter backend is Layer 3 of the Three-Layer Defense and is the single place where org scoping is enforced at the query layer. | +| **Refs** | `principles.md#P1` · `security/tenant-isolation.md` · `adr/ADR-001` | +| **Enforced by** | `OrganizationFilterBackend` + code review | + +### R3 — `on_delete` on Workflow-rooted FKs is audit-durable + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | `WorkflowExecution`, `WorkflowFileExecution`, `ExecutionLog`, and usage rollups are audit-style records and must survive deletion of their source `Workflow` until the retention window expires. A `CASCADE` that wipes execution history on workflow deletion violates P4. Models are the source of truth for cascade behaviour — verify via `grep on_delete` on the PR diff rather than a separate list. | +| **Refs** | `principles.md#P4` · `ai-review-checklist.md` (question 4) | +| **Enforced by** | code review only | + +### R4 — Deployments and schedules reference the workflow, never copy it + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | `Pipeline`, `APIDeployment`, and scheduled triggers are trigger records — they must hold a FK to `Workflow` and re-read its definition at execution time, not snapshot its fields. Copying would let a running deployment diverge from its source workflow silently. | +| **Refs** | `principles.md#P7` | +| **Enforced by** | code review only | + +### R5 — Celery tasks in this component use JSON serialization only + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Workers accept JSON only; `pickle` and equivalents are disabled. Task signatures and arguments must be JSON-serializable. This is also what lets workers communicate with the orchestration across the new workers architecture without Django imports. | +| **Refs** | `security/standards.md` (Celery serialization) · `principles.md#P8` | +| **Enforced by** | Celery broker config (`task_serializer = "json"`) + code review | + +--- + +## Known Exceptions + +None. + +## Checklist + +See [Definition of Done](../../design-rules/definition-of-done.md). diff --git a/design-rules/README.md b/design-rules/README.md new file mode 100644 index 0000000000..1e26054b98 --- /dev/null +++ b/design-rules/README.md @@ -0,0 +1,65 @@ +# Design Rules + +This directory holds the version-controlled architectural rules for this repository. The goal is for any AI agent or reviewer to be able to load the rules that apply to a change without leaving the repo. + +> **Instructions for AI agents and PR review bots.** AI agents and PR review bots working on this repository must load `DESIGN_RULES.md` from the directory of any file being changed (and from each parent directory up to the repo root). Together with `principles.md` and `ai-review-checklist.md`, those files define the rules every change must respect. If a directory does not contain a `DESIGN_RULES.md`, only the global rules in this directory apply. + +This nested-discovery pattern follows the [AGENTS.md](https://agents.md/) convention; `DESIGN_RULES.md` is a structured dialect of it, with the schema defined in [`per-component-contract.md`](per-component-contract.md). + +--- + +## Layout + +| File | Purpose | +|---|---| +| [`principles.md`](principles.md) | Universal principles P1–P9 | +| [`ai-review-checklist.md`](ai-review-checklist.md) | 9 questions to answer on every change | +| [`lifecycle.md`](lifecycle.md) | Change lifecycle from design to monitoring | +| [`security/tenant-isolation.md`](security/tenant-isolation.md) | Three-Layer Defense | +| [`security/standards.md`](security/standards.md) | SQL safety and current protection patterns | +| [`adr/`](adr/) | Accepted Architecture Decision Records | + +--- + +## How to add a global rule + +| Step | Action | +|---|---| +| 1 | Decide whether the rule is universal (goes in `principles.md`) or a security standard (goes in `security/standards.md`). | +| 2 | Open a PR. The rule must describe behaviour that is implemented in current code, not aspirational. | +| 3 | Update `ai-review-checklist.md` if the rule needs a checklist question. | + +--- + +## Per-component `DESIGN_RULES.md` template + +Every active component (each Django app under `backend/`, each `unstract/` shared library, each worker under `workers/`) holds a `DESIGN_RULES.md` at its directory root. The canonical shape is defined in one place and copied from one place — do not embed a duplicate template here. + +| What you need | Where to find it | +|---|---| +| The authoritative spec (section structure, rule format, severity vocabulary, Known Exceptions, M1/M2/M3) | [`per-component-contract.md`](per-component-contract.md) | +| A concrete file to copy when creating a new component's rules | [`backend/account_v2/DESIGN_RULES.md`](../backend/account_v2/DESIGN_RULES.md) | +| The canonical Definition of Done | [`definition-of-done.md`](definition-of-done.md) | + +If `per-component-contract.md` and an example file ever disagree, the contract wins. + +--- + +## How to add an ADR + +| Step | Action | +|---|---| +| 1 | Pick the next free `ADR-NNN` number under `adr/`. | +| 2 | Use the format: Status / Context / Decision / Consequences. | +| 3 | Only ADRs with status "Accepted" land here. Proposals live elsewhere. | +| 4 | Link the ADR from any per-component `DESIGN_RULES.md` it constrains. | + +--- + +## Where debt is tracked + +Technical debt is tracked in the project issue tracker, not in this repository. + +## Where vulnerabilities are tracked + +Security vulnerabilities are tracked in the project's private security channel, not in this repository. diff --git a/design-rules/adr/ADR-001-org-scoping-query-layer.md b/design-rules/adr/ADR-001-org-scoping-query-layer.md new file mode 100644 index 0000000000..0efad9b2ec --- /dev/null +++ b/design-rules/adr/ADR-001-org-scoping-query-layer.md @@ -0,0 +1,20 @@ +# ADR-001: Org scoping is enforced at the query layer + +## Context + +Multi-tenant data isolation can be enforced at the database (row-level security), in the application's query layer, or in the view layer alone. Postgres RLS would require per-request `SET` calls and would couple the application tightly to one database engine. View-layer-only enforcement relies on every developer remembering to filter — a single missed view leaks cross-tenant data. + +## Decision + +Org scoping is enforced in the application's query layer through two complementary mechanisms: + +1. `DefaultOrganizationManagerMixin` for models with a direct organization FK. +2. `OrgAwareManager` and `OrganizationFilterBackend` for models that reach `Organization` only through a chain of FKs (BFS-discovered). + +The thread-local `org_id` is set by `CustomAuthMiddleware` on every request. See `design-rules/security/tenant-isolation.md` for the full layered defense. + +## Consequences + +- Adding a new tenant model is a one-line manager declaration. +- Code outside a request context (workers, tasks) must explicitly set the org context before issuing queries on tenant models. +- The application is portable across database engines because no RLS feature is used. diff --git a/design-rules/adr/ADR-002-no-org-fk-on-execution.md b/design-rules/adr/ADR-002-no-org-fk-on-execution.md new file mode 100644 index 0000000000..1686b0ce06 --- /dev/null +++ b/design-rules/adr/ADR-002-no-org-fk-on-execution.md @@ -0,0 +1,16 @@ +# ADR-002: No org FK on Execution + +## Context + +`WorkflowExecution` is created every time a workflow runs. A direct `organization` FK on `WorkflowExecution` would duplicate information already encoded in `Workflow.organization` and create two sources of truth that could drift. + +## Decision + +`WorkflowExecution` does not carry an `organization` FK. Its organization is derived through `Workflow`. The `OrganizationFilterBackend` BFS-discovers the chain `WorkflowExecution → Workflow → Organization` and applies the org filter at the view layer. + +## Consequences + +- One source of truth for an execution's organization. +- Cross-org listing of executions requires no special case — the filter backend handles it. +- Any worker that creates an execution must do so via a workflow it has already resolved in the user's org context. +- A model that hangs off `WorkflowExecution` (e.g. `WorkflowFileExecution`, `ExecutionLog`) inherits org through the same chain. diff --git a/design-rules/adr/ADR-003-usage-string-refs.md b/design-rules/adr/ADR-003-usage-string-refs.md new file mode 100644 index 0000000000..61f9f75044 --- /dev/null +++ b/design-rules/adr/ADR-003-usage-string-refs.md @@ -0,0 +1,15 @@ +# ADR-003: Usage uses string references + +## Context + +Usage records are written for billing and accounting. If those records held a hard FK to `Workflow`, deleting a workflow would either cascade-delete its billing history (data loss) or be blocked entirely (poor UX). Neither is acceptable for records that must outlive the entity they describe (P4). + +## Decision + +`Usage` stores `workflow_id` as a string reference rather than a FK. The reference points at the workflow but does not enforce referential integrity. Reads that need the workflow object resolve it explicitly. + +## Consequences + +- Billing rows survive workflow deletion. +- String-reference fields cannot be traced via the BFS-based `OrgAwareManager`. `Usage` therefore relies on a direct `organization` FK for tenant scoping. +- Joins from `Usage` to `Workflow` must be performed explicitly in the application; there is no DB-enforced join path. diff --git a/design-rules/adr/ADR-005-prompt-studio-registry-publish-gate.md b/design-rules/adr/ADR-005-prompt-studio-registry-publish-gate.md new file mode 100644 index 0000000000..a2080a0038 --- /dev/null +++ b/design-rules/adr/ADR-005-prompt-studio-registry-publish-gate.md @@ -0,0 +1,17 @@ +# ADR-005: Prompt Studio Registry as the publish gate + +## Context + +Prompt Studio is the authoring surface where prompts are drafted, iterated, and tested. Letting workflow runtime read directly from authoring tables would couple every running execution to in-progress edits, and would let an unsaved draft change the behaviour of a deployed workflow. + +## Decision + +Prompt Studio publishes a runnable artifact to `PromptStudioRegistry`. The registry is the only surface that runtime consumers may read. Drafts in Prompt Studio are not visible to executors until they are published. + +This is the embodiment of P3 (publishing is an explicit gate) for the prompt authoring path. + +## Consequences + +- Drafts are safe to iterate without affecting running deployments. +- A publish step exists and must succeed before a workflow can use the new prompt configuration. +- Runtime code paths must consult the registry, never the authoring tables directly. diff --git a/design-rules/adr/ADR-006-organization-rate-limit-separation.md b/design-rules/adr/ADR-006-organization-rate-limit-separation.md new file mode 100644 index 0000000000..6880fab070 --- /dev/null +++ b/design-rules/adr/ADR-006-organization-rate-limit-separation.md @@ -0,0 +1,15 @@ +# ADR-006: OrganizationRateLimit separated from Configuration + +## Context + +Per-organization configuration values are stored in `Configuration`. Rate limits could have been added as just another configuration row, but rate limits are not passive settings — they actively gate runtime behaviour and need their own update path, validation rules, and audit characteristics. + +## Decision + +`OrganizationRateLimit` is a separate model from `Configuration`. It lives in the `configuration` app but is not stored as a `Configuration` row. + +## Consequences + +- Rate limit changes have a dedicated change surface. +- `Configuration` remains a passive key-value store for settings that do not affect runtime gating. +- Future per-org gates that actively change behaviour should follow the same pattern: a dedicated model, not a `Configuration` row. diff --git a/design-rules/adr/ADR-007-adapter-access-runtime-validation.md b/design-rules/adr/ADR-007-adapter-access-runtime-validation.md new file mode 100644 index 0000000000..eb303f80fd --- /dev/null +++ b/design-rules/adr/ADR-007-adapter-access-runtime-validation.md @@ -0,0 +1,15 @@ +# ADR-007: Adapter access validated at runtime + +## Context + +Adapter access (which user/org may use which LLM, embedding, or vector store adapter) could be validated only at the moment a workflow is created. But access can be revoked between creation and execution. Validating only at creation time would leave revoked workflows still able to run. + +## Decision + +Adapter access is validated at the moment of execution. The executor checks, for each adapter the workflow needs, whether the requesting org currently has access. If not, the run is rejected. + +## Consequences + +- Revoking adapter access immediately stops new runs from using that adapter. +- The executor pays a small per-run validation cost. +- This is the runtime arm of P5 (fail closed): a missing or revoked grant means the run is denied. diff --git a/design-rules/adr/ADR-012-connectorauth-user-owned.md b/design-rules/adr/ADR-012-connectorauth-user-owned.md new file mode 100644 index 0000000000..a537410bbe --- /dev/null +++ b/design-rules/adr/ADR-012-connectorauth-user-owned.md @@ -0,0 +1,15 @@ +# ADR-012: ConnectorAuth is owned by User + +## Context + +A `ConnectorAuth` row stores the OAuth tokens or credentials a person used to log into a third-party service (Google Drive, Box, etc.). The token represents the *individual's* identity at that third party, not the organization's. If `ConnectorAuth` were owned by an organization-membership row, transferring or removing the membership would orphan the token in unintuitive ways. + +## Decision + +`ConnectorAuth` has a FK to `User`, with `on_delete=CASCADE`. Deleting the user removes their connector auth records. The token's lifecycle is tied to the person, not their org membership. + +## Consequences + +- A user who is removed from an org but still exists keeps their connector auth — it is *their* token. +- Deleting the user account cleanly removes all of their third-party tokens. +- Org-level scoping of which connectors are *usable* is enforced separately through the connector instance, which is org-owned. diff --git a/design-rules/adr/ADR-014-internal-external-api-separation.md b/design-rules/adr/ADR-014-internal-external-api-separation.md new file mode 100644 index 0000000000..c3cc6bdcf9 --- /dev/null +++ b/design-rules/adr/ADR-014-internal-external-api-separation.md @@ -0,0 +1,20 @@ +# ADR-014: Internal and external APIs separated per app + +## Context + +Workers and other backend services need to call the Django backend, but they must not authenticate the way an end user does, and the surface they need is narrower and shaped differently from the public REST API. Mixing the two surfaces in the same view module is the path to accidental privilege escalation: a public-facing decorator change can silently affect a service-to-service endpoint. + +## Decision + +Each Django app that exposes a service-to-service surface keeps it in separate files: + +- `urls.py` and `views.py` for the external (end-user) surface. +- `internal_urls.py` and `internal_views.py` for the service-to-service surface. + +The internal surface is mounted under a distinct URL prefix and is protected by `InternalAPIAuthMiddleware`. End-user auth middleware does not run on internal endpoints, and vice versa. + +## Consequences + +- Changing one surface cannot accidentally change the other. +- Workers call backend functionality via the internal client (see `workers/shared/api_client.py`) using the internal surface only. +- New service-to-service needs land in `internal_views.py`, not in the public REST viewset. diff --git a/design-rules/adr/README.md b/design-rules/adr/README.md new file mode 100644 index 0000000000..b5fb13ee52 --- /dev/null +++ b/design-rules/adr/README.md @@ -0,0 +1,66 @@ +# Architecture Decision Records + +This directory contains the **active** Architecture Decision Records for the repository. Proposals, rejected options, and superseded ADRs are not stored here — `git log` is the historical record. + +--- + +## Index + +| ADR | Title | +|---|---| +| [ADR-001](ADR-001-org-scoping-query-layer.md) | Org scoping is enforced at the query layer | +| [ADR-002](ADR-002-no-org-fk-on-execution.md) | No org FK on Execution | +| [ADR-003](ADR-003-usage-string-refs.md) | Usage uses string references | +| [ADR-005](ADR-005-prompt-studio-registry-publish-gate.md) | Prompt Studio Registry as the publish gate | +| [ADR-006](ADR-006-organization-rate-limit-separation.md) | OrganizationRateLimit separated from Configuration | +| [ADR-007](ADR-007-adapter-access-runtime-validation.md) | Adapter access validated at runtime | +| [ADR-012](ADR-012-connectorauth-user-owned.md) | ConnectorAuth is owned by User | +| [ADR-014](ADR-014-internal-external-api-separation.md) | Internal and external APIs separated per app | + +Numbers are assigned in creation order and **never reused** — gaps in the index signal a removed or superseded ADR. + +--- + +## ADR format + +Every active ADR has exactly three sections under the title: + +```markdown +# ADR-NNN: <title> + +## Context +<the situation and constraints that prompted the decision> + +## Decision +<what was decided> + +## Consequences +<what becomes easier, harder, or required as a result> +``` + +There is no `Status` field. The fact that the file exists at this path is what makes it active — the merge IS the acceptance. + +--- + +## Supersession + +When an ADR is overturned, the file is **deleted in the same PR** that introduces the replacement. There is no stub, no tombstone, no Status flip. + +| Step | Action | +|---|---| +| 1 | Write the new ADR. Its `## Context` section cites the old ADR by ID and explains why the prior decision is being overturned. | +| 2 | Delete the old ADR file in the same PR. | +| 3 | Update every reference to the old ADR ID in the repo — per-component `DESIGN_RULES.md` `Refs` rows, `principles.md`, `security/`, code comments, anywhere — to point at the new ADR. M2 enforces this. | +| 4 | The old ADR's number is never reused. The gap in the index is the only marker that an ADR existed at that number. | + +Forensic recovery: `git log --all --diff-filter=D -- design-rules/adr/ADR-NNN-*.md` finds the deletion commit; `git show <commit>^:design-rules/adr/ADR-NNN-*.md` retrieves the original body. + +--- + +## How to add an ADR + +| Step | Action | +|---|---| +| 1 | Pick the next free `ADR-NNN` number (the highest existing number plus one — never fill a gap). | +| 2 | Create `ADR-NNN-short-slug.md` using the format above. | +| 3 | Link the ADR from the per-component `DESIGN_RULES.md` of any directory it constrains. | diff --git a/design-rules/ai-review-checklist.md b/design-rules/ai-review-checklist.md new file mode 100644 index 0000000000..e008c39f9b --- /dev/null +++ b/design-rules/ai-review-checklist.md @@ -0,0 +1,76 @@ +# AI Review Checklist + +Every change must answer "yes" to all applicable questions. A "no" or "unsure" is a blocking finding. + +--- + +## 1. Org scoping (P1) + +| | | +|---|---| +| **Question** | Does every new or modified read/write of tenant data pass through a manager or filter backend that scopes by organization? | + +--- + +## 2. Credentials (P2) + +| | | +|---|---| +| **Question** | Are any new credential, token, or secret fields stored in `EncryptedBinaryField` (or equivalent)? | + +--- + +## 3. Publishing gate (P3) + +| | | +|---|---| +| **Question** | If the change introduces an authoring artifact, is there an explicit publish step that produces a separate runtime artifact? | + +--- + +## 4. Audit/billing durability (P4) + +| | | +|---|---| +| **Question** | If the change touches usage, billing, or audit-style records, do those records survive the deletion of their source? | +| **Question** | Does this PR add or change an `on_delete` behavior on an FK? If yes, is the intent documented inline on the field, and is the cascade safe for audit/retention (P4)? Models are the source of truth for cascades — verify via `grep on_delete` rather than a separate list. | + +--- + +## 5. Fail closed (P5) + +| | | +|---|---| +| **Question** | Do permission and feature checks default to deny on missing or ambiguous inputs? | + +--- + +## 6. Execution org inheritance (P6) + +| | | +|---|---| +| **Question** | If the change adds an execution-side model, does it inherit org through its parent rather than duplicating `organization_id`? | + +--- + +## 7. Deployments as triggers (P7) + +| | | +|---|---| +| **Question** | If the change adds a deployment or schedule, does it reference the workflow rather than copying its data? | + +--- + +## 8. Internal vs external (P8) + +| | | +|---|---| +| **Question** | Is service-to-service traffic on `/internal/...` with internal auth, and end-user traffic on the external surface? | + +--- + +## 9. Anchor integrity (P9) + +| | | +|---|---| +| **Question** | If the change adds a model that belongs to an anchor entity (e.g. `Workflow`), does it derive org from that anchor rather than introducing a parallel direct FK? | diff --git a/design-rules/definition-of-done.md b/design-rules/definition-of-done.md new file mode 100644 index 0000000000..cbb063cbf0 --- /dev/null +++ b/design-rules/definition-of-done.md @@ -0,0 +1,48 @@ +# Definition of Done (per-component changes) + +This file is the single source of truth for the per-component Definition of +Done. Every per-component `DESIGN_RULES.md` ends with a `## Checklist` section +that links here. The body of the checklist lives only in this file. + +A change governed by a per-component `DESIGN_RULES.md` is ready to merge when +**all** of the following are true: + +- [ ] Every rule R1..Rn in the affected file passes for the changed code +- [ ] Every rule's `Enforced by:` mechanism actually runs on this PR + (test, middleware, CI check, or `code review only`) +- [ ] The global AI Review Checklist + (`design-rules/ai-review-checklist.md`) answers "yes" to every + applicable question +- [ ] **M1 (coverage):** if a new sub-component, model, or task was added, + it is covered by a rule in the affected file +- [ ] **M2 (co-change):** if behaviour governed by R1..Rn changed, the + relevant rule(s) were updated in this same PR +- [ ] **M3 (consistency):** no rule in the affected file contradicts a rule + in any other `DESIGN_RULES.md` or in `design-rules/` +- [ ] If a previously-listed rule is being removed or weakened, a new ADR + justifies it; the prior ADR — if any — is **deleted** in the same PR + and every reference to its ID across the repo is updated (see + `adr/README.md`) +- [ ] Any intentional violation is recorded as a `## Known Exceptions` entry + in the affected file, with a tracker reference + +## Why this is one file + +Inlining the same eight-line block in 41 per-component files is duplicated +boilerplate that drifts the moment any line of it changes. Reviewers learn +this checklist once. The per-component file's `## Checklist` section is a +single line that links here. + +## Severity vocabulary used by rules + +Rules in per-component files carry one of three RFC 2119 severities. The +checklist above applies to MUST and SHOULD rules; MAY rules are advisory. + +| Severity | Meaning | Reviewer behaviour | +|---|---|---| +| **MUST** | The rule is non-negotiable. A violation is a merge blocker unless an ADR supersedes the rule or a `Known Exceptions` entry justifies the deviation. | Block the PR. | +| **SHOULD** | The rule reflects strong consensus. A violation is allowed only with a documented reason in the PR description and, if recurring, a `Known Exceptions` entry. | Request changes; accept with explicit justification. | +| **MAY** | The rule is a recommended default. A violation is fine without justification but should still be a conscious choice. | Note in review; do not block. | + +If a rule has no severity tag, the reviewer treats it as MUST. Default to the +strictest reading. diff --git a/design-rules/lifecycle.md b/design-rules/lifecycle.md new file mode 100644 index 0000000000..ce79e9518b --- /dev/null +++ b/design-rules/lifecycle.md @@ -0,0 +1,51 @@ +# Change Lifecycle + +A change moves through five phases. Each phase has its own checks. + +--- + +## 1. Design + +| | | +|---|---| +| **Check** | State the principle(s) the change relies on (link to [`principles.md`](principles.md)). | +| **Check** | If the change introduces a new model relationship, identify the org path: direct FK, BFS-discoverable parent chain, or anchor entity (P9). | +| **Check** | If the change introduces a new ADR-worthy decision, draft the ADR before coding. | + +--- + +## 2. Assembly + +| | | +|---|---| +| **Check** | Apply the per-component `DESIGN_RULES.md` of every directory the change touches. | +| **Check** | Run the AI Review Checklist mentally as you write each commit. | +| **Check** | Migrations: never widen tenant visibility. Never break the org scoping path. | + +--- + +## 3. Deploy + +| | | +|---|---| +| **Check** | Internal endpoints stay behind `InternalAPIAuthMiddleware` (P8). | +| **Check** | External endpoints stay behind the configured auth middleware (P5). | +| **Check** | Celery tasks use JSON serialization only. | + +--- + +## 4. Runtime + +| | | +|---|---| +| **Check** | Adapter access is validated at the moment of execution (ADR-007). | +| **Check** | Org context is set by `CustomAuthMiddleware` and read from thread-local state by managers and filter backends. | + +--- + +## 5. Monitoring + +| | | +|---|---| +| **Check** | Usage and audit-style writes are durable across source-object deletion (P4). | +| **Check** | Logs from execution carry workflow and execution identifiers; org is derived through the workflow chain (P6). | diff --git a/design-rules/per-component-contract.md b/design-rules/per-component-contract.md new file mode 100644 index 0000000000..8bab51bc94 --- /dev/null +++ b/design-rules/per-component-contract.md @@ -0,0 +1,195 @@ +# Per-component contract + +Every per-component `DESIGN_RULES.md` file in this repo (one per active Django app under `backend/`, per shared library under `unstract/`, and per worker under `workers/`) follows this contract. + +This file holds the rationale and the rules-about-rules that would otherwise be duplicated across every per-component file. The reviewer-facing parts — the Compatibility blockquote and the link to the Definition of Done — are still inlined in every per-component file because PR reviewers and diff-scoped review bots need to see them in the file under review without following links. + +The canonical Definition of Done lives in [`definition-of-done.md`](definition-of-done.md), not in this file. + +**Worked example:** [`backend/account_v2/DESIGN_RULES.md`](../backend/account_v2/DESIGN_RULES.md) — copy this file's structure when creating a new per-component file. + +--- + +## Section structure (the only allowed shape) + +Every per-component `DESIGN_RULES.md` contains, in this order: + +| # | Section | +|---|---| +| 1 | Title — `# <component> — Design Rules` | +| 2 | One-line component intro paragraph | +| 3 | Compatibility blockquote (verbatim — see below) | +| 4 | Contract pointer paragraph (one line linking to this file) | +| 5 | `## Scope` — two-row table with `**Covers**` and `**Excludes**` | +| 6 | `## Read first` — table of files and why each binds here | +| 7 | `## Rules` — `R1`..`Rn`, each rendered as an `### R<N> — <title>` heading followed by a 4-row table (Severity / Why / Refs / Enforced by). If the component has no rules yet, the section contains the single line `No component-specific rules yet.` | +| 8 | `## Known Exceptions` — `### <descriptive title>` headings with a 3-row table (Rule / Why / Tracked in), OR the single line `None.` | +| 9 | `## Checklist` — single line linking to `definition-of-done.md` | + +Place a `---` horizontal rule between major content sections (after the Contract pointer, after `Read first`, after `Rules`). They create the visual breaks that make the file scannable in a github diff or markdown preview but are not themselves "sections." + +There is no `## What this is`, no `## Examples`, no inline DoD body, no `Status` field anywhere. + +| | | +|---|---| +| **Target size** | ~70 lines per file. A file longer than 100 lines is a code smell — split the component or move detail into an ADR. (The table layout adds vertical space; substance budget is still small.) | +| **Rule count** | 5–7 rules per component. Hard ceiling 10. If a component needs more, the component is too coarse and should be split. | + +--- + +## The Compatibility blockquote (verbatim) + +Every per-component file places this blockquote immediately after the intro paragraph, exactly once, with the bold `**Compatibility:**` label: + +``` +> **Compatibility:** All changes to this component must remain compatible with extension by an external layer that runs this codebase alongside additional Django applications sharing the same database. +``` + +`validate.sh` greps for the literal substring `All changes to this component must remain compatible` (the grep tolerates the `**Compatibility:**` markdown bold form as well as a plain `Compatibility:` label, since the substring sits after both). Do not paraphrase the rest of the sentence. + +--- + +## The Contract pointer paragraph (one line) + +Immediately under the Compatibility blockquote, on its own paragraph: + +``` +This file follows the [per-component contract](../../design-rules/per-component-contract.md) (rule format, severity vocabulary, Known Exceptions, M1/M2/M3 meta-rules). +``` + +The exact relative path depends on the depth of the per-component file — verify it resolves to `design-rules/per-component-contract.md` from the file's directory before merging. + +--- + +## Rule format + +Every rule in the `## Rules` section uses this exact format — an `H3` heading followed by a 4-row, 2-column markdown table: + +```markdown +### R1 — <one-sentence rule> + +| | | +|---|---| +| **Severity** | MUST · SHOULD · MAY | +| **Why** | <reason — principle, ADR, or past incident> | +| **Refs** | <principles.md#PN · adr/ADR-NNN · security/...> | +| **Enforced by** | <test path | middleware | CI check | code review only | not yet enforced — <ref>> | +``` + +Every rule must have all four rows (Severity, Why, Refs, Enforced by). `validate.sh` checks the field presence, not the field content. + +The middle dot (`·`) is the canonical separator for multiple `Refs` entries — easier to scan than a comma in a table cell. + +**Severity** uses RFC 2119 vocabulary. Definitions are in [`definition-of-done.md`](definition-of-done.md#severity-vocabulary-used-by-rules). If a rule has no severity tag, the reviewer treats it as MUST. + +**Enforced by** must always be a non-empty value. Legitimate values include: + +| Value | Use when | +|---|---| +| A specific test path | e.g. `tests/test_tenant_isolation.py::test_org_filter_backend` | +| A middleware or class name | e.g. `CustomAuthMiddleware` | +| A CI check | e.g. `ci: pre-commit hook ruff S608` | +| `code review only` | For rules that have no automated check today but are routinely caught at PR review | +| `not yet enforced — <tracker reference>` | For rules that describe current expected behaviour but have no enforcement at all yet; the tracker reference is required so the gap is visible | + +A rule with `Enforced by: not yet enforced` is still a real rule, but it is honest about being unverified. + +--- + +## Rule numbering convention + +- Rule IDs are stable: `R1`, `R2`, … assigned in creation order. +- A rule ID is **never reused**. Removing a rule leaves a gap. The next new rule takes the next free number after the highest ever used in this file. +- Rules are scoped to the file they live in. Inside a per-component file, refer to a rule as `R<N>` (e.g. "R2 covers this case"). +- Cite a rule from outside its file as `<component>.R<N>` — for example `account_v2.R2` or `connector_v2.R5`. Use the path-style component name (`unstract/sdk1.R1`) when two components share a base name across `backend/`, `unstract/`, and `workers/`. + +--- + +## Known Exceptions format + +`## Known Exceptions` records intentional deviations from a rule that have been accepted as legitimate. Each entry uses a descriptive `H3` heading followed by a 3-row table: + +```markdown +### <descriptive title — e.g. "Legacy import path"> + +| | | +|---|---| +| **Rule** | R<N> | +| **Why** | <reason the deviation is justified> | +| **Tracked in** | <issue tracker ID, ADR reference, or "permanent — see Why"> | +``` + +Exceptions are temporary by definition — they exist to record an accepted current violation, and they are removed when the violation is fixed. They are not numbered: stable IDs are for things you cite from outside, and exceptions are referred to by their topic, not by an ID. When an exception is removed, the heading simply disappears. + +If there are no known exceptions, the section contains the single line `None.`. The section is mandatory — its absence means the contract is not being followed. + +A `## Known Exceptions` entry is the *only* legitimate way for code to violate a rule without changing the rule. If the deviation is permanent, say so explicitly under `Tracked in:`. If the deviation is temporary, the tracker entry must hold the plan to remove it. + +--- + +## Meta-rules — M1, M2, M3 + +The Definition of Done in [`definition-of-done.md`](definition-of-done.md) references three meta-rules. Their full rationale lives here so the per-component file does not have to repeat it. + +### M1 — Coverage + +Every active component must have a `DESIGN_RULES.md`. Every new active component (Django app, shared library, worker) added in a PR must include a `DESIGN_RULES.md` created in the same PR. If new behaviour added to an existing component falls outside the scope of any existing rule in that component's file, a new rule must be added in the same PR. + +Why: a component without rules is a component that has been decided about in private. The point of this system is to make those decisions visible in the directory of the code they govern. + +### M2 — Co-change + +Any PR that changes behaviour governed by an existing `R1..Rn` rule must update the relevant rule(s) in the same PR. Both directions are blockers: + +- Behaviour change without rule update → blocker (the rule is now stale). +- Rule update without behaviour change → blocker (the rule is now an unenforced wish). + +M2 also covers ADR ID references: any PR that supersedes an ADR must update **every** reference to the old ADR ID anywhere in the repo (per-component `Refs:` rows, `principles.md`, `security/`, code comments, anywhere) in the same PR. The supersession itself is a delete — see `adr/README.md`. + +Why: rules and code that drift apart silently are worse than no rules at all. They give reviewers a false sense of safety. + +### M3 — Consistency + +No rule in any `DESIGN_RULES.md` may contradict a rule in any other `DESIGN_RULES.md` or in `design-rules/`. If a PR creates a conflict, it must either edit one of the rules to reconcile, or add a new ADR that overturns the conflicting decision, **delete** the prior ADR in the same PR, and update every reference to the prior ADR ID across the repo to point at the new one (M2 above). + +Why: a rule system that contradicts itself is no rule system at all. Reviewers cannot be expected to compute the consistent subset on every PR. + +--- + +## When to update a per-component file + +- Add a rule when a new model, manager, task, or sub-module is introduced inside that component. +- Update an existing rule when its `Refs:` line changes (a new ADR supersedes the old one, a principle is renumbered, a security standard shifts). +- Update or remove a rule when it stops describing reality. Removing a rule always leaves a gap in the numbering — never reuse the ID. +- Update `Enforced by:` whenever a new test or check starts covering the rule, or whenever an existing check is removed. +- Add a `Known Exceptions` entry when an intentional deviation lands in the code. Update or remove the entry when the deviation is resolved. +- Co-change with the code: M2 above. This is a hard rule, not a suggestion. + +--- + +## When to add a per-component file + +- Whenever a new active Django app is added under `backend/`, a new shared library directory is added under `unstract/`, or a new worker directory is added under `workers/`. M1 above. +- Copy the structure from a real, current file — `backend/account_v2/DESIGN_RULES.md` is the canonical reference. +- If the new component has no specific rules yet, the `## Rules` section contains a single line: `No component-specific rules yet.` The file is still mandatory, and the `## Known Exceptions` and `## Checklist` sections are still required. + +--- + +## What this contract intentionally does not say + +- It does not list any specific rules. Those live in the per-component files. +- It does not list principles, ADRs, or security standards. Those live in `design-rules/principles.md`, `design-rules/adr/`, and `design-rules/security/`. +- It does not list debt or vulnerabilities. Those live in the project issue tracker and the project's private security channel. +- It does not name any specific PR review tool. The contract is generic: Claude Code, any future AI agent, and any human reviewer all read the same files. + +--- + +## Self-update reminder + +If you change this contract, expect to: + +1. Update [`definition-of-done.md`](definition-of-done.md) if the change affects the checklist body, severity vocabulary, or M1/M2/M3 wording. +2. Update `.claude/skills/design-rules/scripts/validate.sh` if any literal string the script greps for changes (the Compatibility blockquote, the `## Checklist` section header, or the rule field names). +3. Update `.claude/skills/design-rules/SKILL.md` if the contract introduces a new section that the `get` or `review` commands must surface. +4. Re-run `validate.sh` against every per-component file in the repo. +5. If the section structure itself changes, the change touches every per-component file. That is intentional: a structural change is a real event and a 41-file diff is the right blast radius. diff --git a/design-rules/principles.md b/design-rules/principles.md new file mode 100644 index 0000000000..15251b6d87 --- /dev/null +++ b/design-rules/principles.md @@ -0,0 +1,97 @@ +# Principles P1–P9 + +These nine principles govern every change in this repository. They describe rules new code must follow. Existing code that violates a principle is wrong and should be flagged in review. + +> **Severity.** Every principle is **MUST** unless a per-component `DESIGN_RULES.md` explicitly weakens it via a rule with `Severity: SHOULD` or `MAY` and a documented `Why`. + +Principles are referenced from per-component files via `principles.md#PN` (e.g. `principles.md#P1`). Update those references whenever a principle is renumbered. + +--- + +## P1 — Organization scoping is mandatory + +| | | +|---|---| +| **Rule** | Every model that holds tenant data must be reachable from `Organization` and must be filtered by the requesting user's organization on every read and write. | +| **Implementation** | Use `DefaultOrganizationMixin` for direct FK ownership, and `OrgAwareManager` or `OrganizationFilterBackend` for indirect ownership through a parent FK chain. See [`security/tenant-isolation.md`](security/tenant-isolation.md) for the Three-Layer Defense. | +| **Example** | A new model that stores documents must add a direct `organization` FK or a path that BFS-resolves to one. A new DRF view on a tenant model must inherit the project's filter backend so cross-org reads are impossible. | + +--- + +## P2 — Credentials are encrypted at rest + +| | | +|---|---| +| **Rule** | Any field that stores third-party credentials, tokens, or secrets must be encrypted at rest. | +| **Implementation** | `EncryptedBinaryField` is used for `ConnectorInstance.connector_metadata`. New credential fields must use the same field type. | +| **Example** | A new connector that stores an API key must place that key inside the encrypted metadata field, never a plain `CharField`. | + +--- + +## P3 — Publishing is an explicit gate + +| | | +|---|---| +| **Rule** | Authoring artifacts (e.g. Prompt Studio projects) and the runtime artifacts derived from them are separate. Publishing is the explicit gate that turns a draft into a runnable artifact. | +| **Implementation** | See [`adr/ADR-005`](adr/ADR-005-prompt-studio-registry-publish-gate.md): Prompt Studio publishes to `PromptStudioRegistry`; runtime consumers read the registry, never the draft. | +| **Example** | A new authoring surface must not be referenced directly by an executor — it must publish to a registry first. | + +--- + +## P4 — Audit and billing data is append-only in spirit + +| | | +|---|---| +| **Rule** | Records used for usage accounting, billing, or audit must be written in a way that survives the deletion of the source object they describe. | +| **Implementation** | See [`adr/ADR-003`](adr/ADR-003-usage-string-refs.md): `Usage` uses string references to workflow identifiers so that billing rows survive workflow deletion. | +| **Example** | A new metric model must not put a hard FK to an entity that may be deleted if the metric still needs to exist after deletion. | + +--- + +## P5 — Fail closed on permission decisions + +| | | +|---|---| +| **Rule** | Any permission, access, or feature gate must default to deny when its inputs are missing or ambiguous. | +| **Implementation** | Middleware that derives org context returns 401/403 when the org cannot be resolved. Auth backends raise rather than silently authorising. | +| **Example** | A new view must never assume "if no org filter, return all rows". | + +--- + +## P6 — Execution inherits org from its parent entity + +| | | +|---|---| +| **Rule** | Execution-time entities derive their organization from the workflow they run, not from their own FK to organization. | +| **Implementation** | See [`adr/ADR-002`](adr/ADR-002-no-org-fk-on-execution.md): `WorkflowExecution` has no direct `organization` FK; org is derived through `Workflow`. `OrganizationFilterBackend` handles the BFS chain. | +| **Example** | A new execution-side model should not duplicate `organization_id` — it should rely on the parent workflow. | + +--- + +## P7 — Deployments are triggers, not data + +| | | +|---|---| +| **Rule** | A deployment record (pipeline, schedule, API binding) describes how to invoke a workflow. It does not duplicate the workflow's data. | +| **Implementation** | `pipeline_v2` and `scheduler` hold trigger metadata only. | +| **Example** | A new pipeline type must not snapshot workflow steps — it must reference the workflow. | + +--- + +## P8 — Internal and external surfaces are separated + +| | | +|---|---| +| **Rule** | Service-to-service APIs and end-user APIs live on separate URL groups, use separate auth, and never share the same view function. | +| **Implementation** | See [`adr/ADR-014`](adr/ADR-014-internal-external-api-separation.md): each app exposes `urls.py` for external and `internal_urls.py` + `internal_views.py` for service-to-service. The internal surface is protected by `InternalAPIAuthMiddleware`. | +| **Example** | A worker calling the backend must hit `/internal/...`, never the user-facing REST API. | + +--- + +## P9 — Anchor entities define the org boundary + +| | | +|---|---| +| **Rule** | Some entities are *anchors* — canonical owners of an organization's data graph. The most important anchor in this codebase is `Workflow`. Models that hang off an anchor inherit org via that anchor. | +| **Implementation** | `Workflow` is the anchor for execution, file execution, execution logs, tool instances, and prompt registry consumers. | +| **Example** | A new model that belongs to a workflow should derive org through the workflow, never via a parallel direct FK. | diff --git a/design-rules/security/standards.md b/design-rules/security/standards.md new file mode 100644 index 0000000000..824dd8aa7f --- /dev/null +++ b/design-rules/security/standards.md @@ -0,0 +1,37 @@ +# Security Standards + +These standards describe currently implemented protection patterns. New code must follow them. + +--- + +## S1 — SQL Safety Standard + +| | | +|---|---| +| **Applies to** | All database connectors in `unstract/connectors/.../databases/`. | +| **Supported set** | PostgreSQL · BigQuery · Snowflake · MSSQL · MySQL · MariaDB · Oracle · Redshift | + +Rules: + +1. **Identifier validation.** Any identifier (table name, column name, schema) that comes from outside the function must be validated by `validate_identifier` before being used in SQL. +2. **Identifier quoting.** Validated identifiers must be quoted with `quote_identifier` using the DB-specific `QuoteStyle`. Different engines require different quote characters; never hard-code `"` or `` ` ``. +3. **Parameterized values.** All values must be passed as bound parameters through the DB driver. Never interpolate values into the SQL string. +4. **No f-string SQL.** SQL strings must not be assembled with f-strings or `%`-formatting using user input. Identifier substitution is only allowed after validation+quoting (rule 1+2). +5. **Error message hygiene.** Error messages returned to callers must not include raw SQL or raw identifier values from user input. + +These rules apply to every supported database connector. + +--- + +## Other current protection patterns + +| Pattern | Description | +|---|---| +| **`EncryptedBinaryField`** | Used for `ConnectorInstance.connector_metadata` (P2). | +| **CSP, CORS, CSRF, X-Frame-Options** | Middleware are enabled in the backend Django settings. | +| **File upload validation** | Uploaded documents are validated for MIME type (PDF) and bounded to a 200 MB size limit. | +| **Internal API network isolation** | Internal endpoints live under a separate URL group and require `InternalAPIAuthMiddleware`. They must not be exposed to the public network. | +| **`InternalAPIAuthMiddleware`** | Authenticates service-to-service traffic with a shared secret distinct from end-user auth. | +| **Celery serialization** | Workers accept JSON only. `pickle` and equivalents are disabled. | +| **Django admin disabled by default** | In production settings. | +| **Dependabot** | Configured for dependency updates. | diff --git a/design-rules/security/tenant-isolation.md b/design-rules/security/tenant-isolation.md new file mode 100644 index 0000000000..509c18e15a --- /dev/null +++ b/design-rules/security/tenant-isolation.md @@ -0,0 +1,66 @@ +# Tenant Isolation — Three-Layer Defense + +Tenant isolation in this codebase is enforced by three independent layers in the Django backend. Every layer must remain in place; removing any one of them is a regression. + +--- + +## Layer 1 — Middleware + +| | | +|---|---| +| **Component** | `CustomAuthMiddleware` | +| **Behaviour** | Validates that the authenticated user belongs to the organization addressed by the request and stores the resolved `org_id` in a thread-local `StateStore`. Downstream model managers and filter backends read the org id from this store. | +| **Fail mode** | If the org cannot be resolved, the middleware fails closed (P5) and the request is rejected. | + +--- + +## Layer 2 — Model Managers + +Two complementary mechanisms cover the two FK shapes: + +| | | +|---|---| +| **`DefaultOrganizationManagerMixin`** | For models that have a direct `organization` FK. The mixin filters every queryset by the org id from thread-local state. | +| **`OrgAwareManager`** | For models without a direct FK. It performs a breadth-first search over the model's relations to find a path to `Organization` and filters via that path. Any model whose manager inherits from `OrgAwareManager` is automatically scoped. | + +String-reference fields (e.g. `Usage.workflow_id`, `ToolInstance.tool_id`) cannot be traced via BFS — those models rely on a direct org FK or a separate FK path. + +Example use: + +```python +class MyModel(models.Model): + workflow = models.ForeignKey(Workflow, on_delete=models.CASCADE) + + objects = OrgAwareManager() # BFS finds Workflow → Organization +``` + +--- + +## Layer 3 — Filter Backend + +| | | +|---|---| +| **Component** | `OrganizationFilterBackend` | +| **Configured in** | `DEFAULT_FILTER_BACKENDS` | +| **Behaviour** | For DRF views over models without a direct org FK, the backend BFS-discovers the chain (e.g. `ExecutionLog → WorkflowExecution → Workflow → Organization`) and applies the filter at the view layer. | + +Models currently covered by the filter backend include: + +| Model | +|---| +| `WorkflowExecution` | +| `WorkflowFileExecution` | +| `ExecutionLog` | +| `APIKey` | +| `ToolStudioPrompt` | +| `ProfileManager` | +| `IndexManager` | +| `DocumentManager` | + +--- + +## Rules for new code + +1. New tenant models must opt into one of the two manager mechanisms. +2. New DRF viewsets over tenant models must rely on the configured filter backend; do not bypass it with raw querysets. +3. Never derive org from request body input — always from the thread-local set by middleware. diff --git a/unstract/connectors/DESIGN_RULES.md b/unstract/connectors/DESIGN_RULES.md new file mode 100644 index 0000000000..f0541492b9 --- /dev/null +++ b/unstract/connectors/DESIGN_RULES.md @@ -0,0 +1,83 @@ +# connectors — Design Rules + +`unstract/connectors` is the connector framework shared by the backend and workers. It defines the base classes (`UnstractConnector`, the connectorkit registry, exceptions, connection types) and ships three connector families as subtrees: `filesystems/`, `databases/`, and `queues/`. Every concrete connector plugs into this framework and inherits its credential handling, registration, and error contract. + +> **Compatibility:** All changes to this component must remain compatible with extension by an external layer that runs this codebase alongside additional Django applications sharing the same database. + +This file follows the [per-component contract](../../design-rules/per-component-contract.md) (rule format, severity vocabulary, Known Exceptions, M1/M2/M3 meta-rules). + +--- + +## Scope + +| | | +|---|---| +| **Covers** | `unstract/connectors/**` — base classes, `connectorkit` registry, exceptions, connection-type enums, and the shared credential/metadata handling applied to every connector family | +| **Excludes** | SQL-specific rules for database connectors → [`databases/DESIGN_RULES.md`](src/unstract/connectors/databases/DESIGN_RULES.md). Filesystem-specific rules live (or will live) in the filesystems subtree. Backend-side `ConnectorInstance` model and its viewsets → [`backend/connector_v2`](../../backend/connector_v2/DESIGN_RULES.md). | + +## Read first + +| File | Why it binds here | +|---|---| +| [`principles.md`](../../design-rules/principles.md) | P2 (credentials), P8 (fail-closed) | +| [`ai-review-checklist.md`](../../design-rules/ai-review-checklist.md) | 9 questions every change must answer | +| [`security/standards.md`](../../design-rules/security/standards.md) | `EncryptedBinaryField` on `ConnectorInstance.connector_metadata`, SQL Safety Standard (S1) for the databases subtree | + +--- + +## Rules + +### R1 — Persisted connector credentials go through `EncryptedBinaryField` + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Connector credentials (DSN, OAuth tokens, service-account JSON, bucket keys) are persisted on `ConnectorInstance.connector_metadata` and must be stored via `EncryptedBinaryField`. Writing a plaintext credential to the database breaks P2 and leaves a stale copy even after rotation. | +| **Refs** | `principles.md#P2` · `security/standards.md` (`EncryptedBinaryField`) | +| **Enforced by** | `EncryptedBinaryField` (backend-side) + code review | + +### R2 — Credentials never appear in logs or error messages + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Raw credentials, DSN strings, and token values must not be logged, included in exception messages, or echoed to callers. A traceback that leaks a credential is an information-disclosure bug, and every connector is on this boundary. Redact at the point of raise, not at the logging sink. | +| **Refs** | `principles.md#P2` · `security/standards.md` | +| **Enforced by** | code review only | + +### R3 — Every connector registers through the `connectorkit` registry + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Concrete connectors are discovered and instantiated through `connectorkit`. Side-channel instantiation (direct `from unstract.connectors.databases.foo import Foo`) bypasses the registry, loses the shared credential/metadata handling, and makes the connector invisible to the admin/listing endpoints. | +| **Refs** | `principles.md#P5` | +| **Enforced by** | code review only | + +### R4 — Connector errors surface as the framework's exception hierarchy + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Driver-level exceptions (`psycopg2.*`, `google.api_core.*`, `boto3.ClientError`, …) must be wrapped in the `unstract.connectors.exceptions` hierarchy before leaving the connector. Callers depend on a stable error contract to distinguish retryable, auth, and misconfiguration failures — leaking the raw driver exception couples callers to the driver and defeats retry/backoff policy. | +| **Refs** | `principles.md#P8` | +| **Enforced by** | code review only | + +### R5 — Database connectors inherit the SQL Safety Standard from `databases/` + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | The databases subtree is governed by [`databases/DESIGN_RULES.md`](src/unstract/connectors/databases/DESIGN_RULES.md), which specialises S1 for this framework (identifier validation, quoting, bound parameters, no f-string SQL, error hygiene). Any change that touches SQL assembly in this framework — including base classes or helpers that a database connector will call — must be reviewed against those rules in addition to the ones in this file. | +| **Refs** | `security/standards.md#S1` · `databases/DESIGN_RULES.md` R1–R6 | +| **Enforced by** | code review only | + +--- + +## Known Exceptions + +None. + +## Checklist + +See [Definition of Done](../../design-rules/definition-of-done.md). diff --git a/unstract/connectors/src/unstract/connectors/databases/DESIGN_RULES.md b/unstract/connectors/src/unstract/connectors/databases/DESIGN_RULES.md new file mode 100644 index 0000000000..f46f505dac --- /dev/null +++ b/unstract/connectors/src/unstract/connectors/databases/DESIGN_RULES.md @@ -0,0 +1,93 @@ +# databases — Design Rules + +This subtree contains every database connector shipped with Unstract: PostgreSQL, BigQuery, Snowflake, MSSQL, MySQL, MariaDB, Oracle, and Redshift. Each connector builds SQL against tenant data, so every change here is a potential SQL-injection surface and must follow the SQL Safety Standard (S1). + +> **Compatibility:** All changes to this component must remain compatible with extension by an external layer that runs this codebase alongside additional Django applications sharing the same database. + +This file follows the [per-component contract](../../../../../../design-rules/per-component-contract.md) (rule format, severity vocabulary, Known Exceptions, M1/M2/M3 meta-rules). + +--- + +## Scope + +| | | +|---|---| +| **Covers** | `unstract/connectors/src/unstract/connectors/databases/**` — every supported database connector, its query builders, its identifier/quoting helpers, and its error surfaces | +| **Excludes** | Filesystem connectors → `unstract/connectors/src/unstract/connectors/filesystems/`. Queue connectors → `unstract/connectors/src/unstract/connectors/queues/`. Parent connector framework rules → [`unstract/connectors/DESIGN_RULES.md`](../../../../DESIGN_RULES.md). | + +## Read first + +| File | Why it binds here | +|---|---| +| [`principles.md`](../../../../../../design-rules/principles.md) | P2 (credentials) — DSN/auth handling | +| [`ai-review-checklist.md`](../../../../../../design-rules/ai-review-checklist.md) | 9 questions every change must answer | +| [`security/standards.md`](../../../../../../design-rules/security/standards.md) | **S1 — SQL Safety Standard**, which this file specialises for the databases subtree | +| [`unstract/connectors/DESIGN_RULES.md`](../../../../DESIGN_RULES.md) | Parent connector framework rules also apply — load both | + +--- + +## Rules + +### R1 — External identifiers are validated before use in SQL + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Any identifier (table name, column name, schema) that originates outside the function must pass `validate_identifier` before appearing in a SQL string. Without validation, a caller can smuggle arbitrary SQL through what the connector assumes is a name. | +| **Refs** | `security/standards.md#S1` (rule 1) | +| **Enforced by** | code review only | + +### R2 — Validated identifiers are quoted via the engine's `QuoteStyle` + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Different engines require different quote characters (`"`, `` ` ``, `[...]`). Validated identifiers must be quoted with `quote_identifier` using the DB-specific `QuoteStyle`. Hard-coding a quote character in a per-connector helper breaks on the next engine that doesn't use it and tempts copy-paste SQL assembly. | +| **Refs** | `security/standards.md#S1` (rule 2) | +| **Enforced by** | code review only | + +### R3 — Values are always passed as bound parameters + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | All values must be passed through the DB driver as bound parameters. Never interpolate a value into the SQL string, even if it "looks safe" (numeric, enum, internal). The driver's parameter handling is the only boundary we trust. | +| **Refs** | `security/standards.md#S1` (rule 3) | +| **Enforced by** | code review only | + +### R4 — SQL is never assembled with f-strings or `%`-formatting on user input + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | f-strings and `%`-formatting with user input are the canonical path to SQL injection. Identifier substitution is only legitimate after R1 (validation) and R2 (quoting); value substitution always goes through R3. There is no third path. | +| **Refs** | `security/standards.md#S1` (rule 4) · ci: `ruff S608` | +| **Enforced by** | `ruff S608` (hardcoded-sql-expression) + code review | + +### R5 — Errors returned to callers strip raw SQL and raw user identifiers + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Error messages that echo the failing SQL or the raw user-supplied identifier turn a connector error into an information-disclosure channel (schema leakage, injection feedback). Errors surfaced to callers must redact both. | +| **Refs** | `security/standards.md#S1` (rule 5) | +| **Enforced by** | code review only | + +### R6 — New database connectors implement S1 from day one + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Adding a new engine without wiring it through `validate_identifier` + `quote_identifier` + bound parameters regresses S1 by omission. A new connector PR that doesn't exercise the shared helpers is a blocker, even if the engine "happens to" be safe for the specific query shape being added. | +| **Refs** | `security/standards.md#S1` | +| **Enforced by** | code review only | + +--- + +## Known Exceptions + +None. + +## Checklist + +See [Definition of Done](../../../../../../design-rules/definition-of-done.md). diff --git a/workers/shared/DESIGN_RULES.md b/workers/shared/DESIGN_RULES.md new file mode 100644 index 0000000000..ad0d47804d --- /dev/null +++ b/workers/shared/DESIGN_RULES.md @@ -0,0 +1,93 @@ +# workers/shared — Design Rules + +`workers/shared` is the common library imported by every worker under `workers/`. It owns the internal API client used to talk to the backend, HTTP session lifecycle, Celery task wrappers, and the shared config/env surface. Nothing in this directory may import Django — workers run without a Django process. + +> **Compatibility:** All changes to this component must remain compatible with extension by an external layer that runs this codebase alongside additional Django applications sharing the same database. + +This file follows the [per-component contract](../../design-rules/per-component-contract.md) (rule format, severity vocabulary, Known Exceptions, M1/M2/M3 meta-rules). + +--- + +## Scope + +| | | +|---|---| +| **Covers** | `workers/shared/**` — `InternalAPIClient` and its sub-clients, HTTP session management, Celery task base classes, worker-side config/env loading, logging helpers | +| **Excludes** | Individual worker entrypoints → each `workers/<worker>/DESIGN_RULES.md`. Backend-side internal API views → [`backend/api_v2`](../../backend/api_v2/DESIGN_RULES.md) and the `/internal/` url group. Orchestration models → [`backend/workflow_manager`](../../backend/workflow_manager/DESIGN_RULES.md). | + +## Read first + +| File | Why it binds here | +|---|---| +| [`principles.md`](../../design-rules/principles.md) | P6 (execution org inheritance), P8 (internal vs external) | +| [`ai-review-checklist.md`](../../design-rules/ai-review-checklist.md) | 9 questions every change must answer | +| [`adr/ADR-014`](../../design-rules/adr/ADR-014-internal-external-api-separation.md) | Workers reach the backend through `/internal/...` only | +| [`security/standards.md`](../../design-rules/security/standards.md) | Internal API network isolation, `InternalAPIAuthMiddleware`, Celery serialization | + +--- + +## Rules + +### R1 — All backend traffic goes through `InternalAPIClient` + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Every worker-to-backend call must use `InternalAPIClient` (or one of its sub-clients under `workers/shared/clients/`). Ad-hoc `requests.get(...)` against public REST endpoints bypasses the internal auth boundary, the shared session pool, and retry/backoff policy, and makes worker behaviour depend on end-user auth middleware. | +| **Refs** | `principles.md#P8` · `adr/ADR-014` · `security/standards.md` (Internal API network isolation) | +| **Enforced by** | code review only | + +### R2 — Workers authenticate with the internal service key, never user credentials + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Worker-to-backend traffic is authenticated by `InternalAPIAuthMiddleware` using the shared internal service key, which is distinct from end-user auth. Embedding a user's session token, API key, or credentials in a worker request conflates the identities and turns a worker bug into an auth-escalation path. | +| **Refs** | `principles.md#P8` · `security/standards.md` (`InternalAPIAuthMiddleware`) | +| **Enforced by** | `InternalAPIAuthMiddleware` (backend side) + code review | + +### R3 — `workers/shared` must not import Django + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Workers run in Docker images without a Django process. Importing `django.*`, Django models, or Django settings from `workers/shared` turns every worker into a Django dependency and defeats the whole purpose of the new workers architecture (the system must work with AND without the new workers). Data coming from the backend must be consumed as JSON payloads and mapped into local dataclasses. | +| **Refs** | `principles.md#P6` · `CLAUDE.md` (architecture principles) | +| **Enforced by** | not yet enforced — code review only (no import-linter wired in CI today) | + +### R4 — Celery task signatures are JSON-serializable only + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Workers accept JSON only; `pickle` is disabled. Every task argument and return value must be JSON-serializable — pass IDs, dataclass `asdict()` output, or plain primitives. Passing Django model instances or arbitrary Python objects will fail at the broker boundary and regress the pickle lockdown. | +| **Refs** | `security/standards.md` (Celery serialization) | +| **Enforced by** | Celery broker config (`task_serializer = "json"`) + code review | + +### R5 — HTTP sessions are lifecycle-managed and closed on worker shutdown + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | `InternalAPIClient` uses a `requests.Session` with a connection pool sized by `API_CLIENT_POOL_SIZE`. Sessions must be closed idempotently (`_closed` flag), honoured in `try/finally` around task bodies that own a client, and the singleton must be reset in `on_worker_process_shutdown`. Leaking sessions leaks socket FDs in long-running workers (UNS-205). | +| **Refs** | `principles.md#P4` (durability/reliability) | +| **Enforced by** | `workers/shared/tests/test_session_lifecycle.py` + code review | + +### R6 — Org context is read from the task payload, not from thread-local state + +| | | +|---|---| +| **Severity** | MUST | +| **Why** | Workers have no `CustomAuthMiddleware` and no `StateStore` thread-local populated by request processing. Task functions must receive `organization_id` (or a parent entity ID that the backend will resolve to an org) as an explicit argument. Inferring org from a global or "current" variable inside worker code is unsafe and breaks P6 execution org inheritance. | +| **Refs** | `principles.md#P6` · `principles.md#P1` | +| **Enforced by** | code review only | + +--- + +## Known Exceptions + +None. + +## Checklist + +See [Definition of Done](../../design-rules/definition-of-done.md). From ef544fc499b9a72cac8697f5df2db55c11b5f1f6 Mon Sep 17 00:00:00 2001 From: ali <muhammad.ali@zipstack.com> Date: Wed, 8 Apr 2026 15:55:55 +0530 Subject: [PATCH 2/7] UN-3396 [MISC] Use [[ ]] in validate.sh per SonarQube SonarQube flagged 4 single-bracket conditional tests on lines 42, 51, 56, and 59 of .claude/skills/design-rules/scripts/validate.sh. Switched to the [[ ]] form, which is the bash convention SonarQube enforces. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --- .claude/skills/design-rules/scripts/validate.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.claude/skills/design-rules/scripts/validate.sh b/.claude/skills/design-rules/scripts/validate.sh index dc54d16aa9..4f4bbca34b 100755 --- a/.claude/skills/design-rules/scripts/validate.sh +++ b/.claude/skills/design-rules/scripts/validate.sh @@ -39,7 +39,7 @@ for f in "${COMPONENT_FILES[@]}"; do fail=1 fi done -[ $fail -eq 0 ] && echo " OK" +[[ $fail -eq 0 ]] && echo " OK" echo echo "==> 3. Component directory sanity" @@ -48,15 +48,15 @@ for f in "${COMPONENT_FILES[@]}"; do # Real source = at least one file other than DESIGN_RULES.md and __pycache__ real=$(find "$dir" -maxdepth 1 -mindepth 1 \ ! -name DESIGN_RULES.md ! -name __pycache__ 2>/dev/null | head -1) - if [ -z "$real" ]; then + if [[ -z "$real" ]]; then echo " EMPTY DIR: $f (no real source alongside)" fail=1 fi done -[ $fail -eq 0 ] && echo " OK" +[[ $fail -eq 0 ]] && echo " OK" echo -if [ $fail -eq 0 ]; then +if [[ $fail -eq 0 ]]; then echo "All design rule checks passed." exit 0 else From 1ced2cac7673b10b05c534a3acc01a5b4eb4be8e Mon Sep 17 00:00:00 2001 From: ali <muhammad.ali@zipstack.com> Date: Wed, 8 Apr 2026 16:09:41 +0530 Subject: [PATCH 3/7] UN-3396 [MISC] Sync design-rules/README.md with dropped-Status ADR format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "How to add an ADR" table in design-rules/README.md still mentioned the old "format: Status / Context / Decision / Consequences" and "status Accepted" guidance, which contradicts the same PR's own adr/README.md (Status field dropped; merge is the acceptance) and the 8 ADR files themselves. Collapse the duplicate guidance into a one-line pointer to adr/README.md so the ADR mechanics have a single source of truth and cannot drift again. Also add per-component-contract.md and definition-of-done.md to the Layout table — both are core files in the corpus and were missing from the navigation. Rename the adr/ Layout row from "Accepted" to "Active" to match the new vocabulary. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --- design-rules/README.md | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/design-rules/README.md b/design-rules/README.md index 1e26054b98..4f5aefd6f6 100644 --- a/design-rules/README.md +++ b/design-rules/README.md @@ -15,9 +15,11 @@ This nested-discovery pattern follows the [AGENTS.md](https://agents.md/) conven | [`principles.md`](principles.md) | Universal principles P1–P9 | | [`ai-review-checklist.md`](ai-review-checklist.md) | 9 questions to answer on every change | | [`lifecycle.md`](lifecycle.md) | Change lifecycle from design to monitoring | +| [`per-component-contract.md`](per-component-contract.md) | Canonical shape for every per-component `DESIGN_RULES.md` file | +| [`definition-of-done.md`](definition-of-done.md) | Per-component Definition of Done and severity vocabulary | | [`security/tenant-isolation.md`](security/tenant-isolation.md) | Three-Layer Defense | | [`security/standards.md`](security/standards.md) | SQL safety and current protection patterns | -| [`adr/`](adr/) | Accepted Architecture Decision Records | +| [`adr/`](adr/) | Active Architecture Decision Records | --- @@ -47,12 +49,7 @@ If `per-component-contract.md` and an example file ever disagree, the contract w ## How to add an ADR -| Step | Action | -|---|---| -| 1 | Pick the next free `ADR-NNN` number under `adr/`. | -| 2 | Use the format: Status / Context / Decision / Consequences. | -| 3 | Only ADRs with status "Accepted" land here. Proposals live elsewhere. | -| 4 | Link the ADR from any per-component `DESIGN_RULES.md` it constrains. | +See [`adr/README.md`](adr/README.md) for the ADR format, the numbering convention, and the supersession flow. Do not duplicate that guidance here. --- From 3eda6e7b55cf22c5cc2a67c4e31329e9bd0f9768 Mon Sep 17 00:00:00 2001 From: ali <muhammad.ali@zipstack.com> Date: Wed, 8 Apr 2026 16:36:20 +0530 Subject: [PATCH 4/7] UN-3396 [MISC] Address coderabbit and greptile review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In-scope docs/skill fixes flagged by the bots on PR #1908. Out-of-scope code findings (ExecutionViewSet bypassing OrganizationFilterBackend) are deliberately not addressed here — the rules stay strict and the drift is left visible for a follow-up code PR to fix; Known Exceptions are reserved for deliberate, evaluated deviations, not "we haven't decided yet." Changes: * .claude/skills/design-rules/SKILL.md - `review` trap table: "Missing auth" now references P5 (fail closed on permission decisions), not P8 (internal/external API separation). P8 is about URL/auth middleware separation. - `add` subcommand: ADR template in the fenced block dropped the stale `Status: Accepted` row and gained a `markdown` language tag (markdownlint MD040). Now points at design-rules/adr/README.md for the authoritative supersession flow. - `update` subcommand: replaced the stale "Status: Superseded by ADR-NNN" guidance with the pure-delete supersession flow. - Path reference table: dropped hard-coded counts (22/9/10) and softened scope language to "where present" — matches the prototype-first rollout. * .claude/skills/design-rules/scripts/validate.sh - bash 3.2 portability: replaced `mapfile` (bash 4+, missing on macOS default) with a portable `while read -r -d ''` loop over `find -print0`. Fixes script breakage on macOS reviewers. - Per-check pass/fail tracking: each of the three checks now prints its own OK/FAIL regardless of whether an earlier check failed. Previously the global `fail` flag was being used as a per-check gate, silently swallowing the "OK" message on downstream checks. - Empty-array expansion under `set -u`: used `"${COMPONENT_FILES[@]+"${COMPONENT_FILES[@]}"}"` in loop bodies to avoid the bash 4.0-4.3 unbound-variable trip on an empty COMPONENT_FILES array. * design-rules/adr/ADR-006-organization-rate-limit-separation.md - Corrected the model location: OrganizationRateLimit lives in `backend/api_v2/models.py` (the `api_v2` app), not the `configuration` app. Verified by grep on the model definition. * design-rules/security/tenant-isolation.md - Layer 1 fail-mode description corrected to match the actual `CustomAuthMiddleware` implementation: 403 is only raised when `request.organization_id` is set on the URL and cannot be resolved; requests without an org target are allowed through Layer 1 and fail-closed at Layer 2/3. The middleware is identity-and-binding, not the primary fail-closed boundary — Layer 3 (`OrganizationFilterBackend`) is. * workers/shared/DESIGN_RULES.md - R3 `Enforced by` mixed two legitimate patterns ("not yet enforced" and "code review only"). Picked "code review only" — that is the actual current enforcement state; "not yet enforced" requires a tracker reference per the contract. Not addressed in this PR (deliberately): * ExecutionViewSet bypassing OrganizationFilterBackend (workflow_manager.R2): the rule stays strict; documenting the current bypass as a Known Exception would bless a pattern that hasn't been evaluated. Tracker entry to be opened separately. * validate.sh "OK" wording and "smell if >80 lines" targets, SKILL.md trigger-phrase refinements, and similar nice-to-haves: out of scope for the review-feedback batch. Will land with UN-3397 or a later cleanup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --- .claude/skills/design-rules/SKILL.md | 24 +++++------ .../skills/design-rules/scripts/validate.sh | 42 ++++++++++++++----- ...-006-organization-rate-limit-separation.md | 2 +- design-rules/security/tenant-isolation.md | 4 +- workers/shared/DESIGN_RULES.md | 2 +- 5 files changed, 46 insertions(+), 28 deletions(-) diff --git a/.claude/skills/design-rules/SKILL.md b/.claude/skills/design-rules/SKILL.md index 8a343830db..4d33231f27 100644 --- a/.claude/skills/design-rules/SKILL.md +++ b/.claude/skills/design-rules/SKILL.md @@ -7,7 +7,7 @@ allowed-tools: Read Glob Grep Edit Write Bash(git:*) Bash(gh:*) Bash(find:*) Bas # Design Rules Skill -A skill for managing and applying the version-controlled design rule system in this repo. The system has two layers: global rules in `design-rules/` and per-component `DESIGN_RULES.md` files inside each active Django app, each `unstract/` shared lib component, and each worker. The system is OSS-only — it must never reference cloud / enterprise / pluggable_apps / HITL / subscription / agentic / or any specific PR review tool. +A skill for managing and applying the version-controlled design rule system in this repo. The system has two layers: global rules in `design-rules/` and per-component `DESIGN_RULES.md` files placed next to the code they govern, under `backend/`, `unstract/`, and `workers/` (where present). Coverage is rolled out incrementally — not every component has a file yet. The system is OSS-only — it must never reference cloud / enterprise / pluggable_apps / HITL / subscription / agentic / or any specific PR review tool. --- @@ -98,14 +98,11 @@ Create a new rule, ADR, or per-component file. - Refinement of an existing principle → edit it in place; do not duplicate. ### Add an ADR -1. `ls design-rules/adr/ADR-*.md` to find the next free number. +1. `ls design-rules/adr/ADR-*.md` to find the next free number (the highest existing number plus one — never fill a gap). 2. Create `design-rules/adr/ADR-NNN-<kebab-title>.md`: - ``` + ```markdown # ADR-NNN: <title> - ## Status - Accepted - ## Context <why this decision is needed; what's currently true> @@ -115,8 +112,9 @@ Create a new rule, ADR, or per-component file. ## Consequences <what becomes easier / harder / required> ``` -3. Add a one-line entry to `design-rules/adr/README.md`. -4. Cross-link from the relevant per-component `DESIGN_RULES.md` files in their "Read first" section. + There is no `Status` field — the merge IS the acceptance. See `design-rules/adr/README.md` for the full ADR format and the delete-on-supersession flow. +3. Add a one-line entry to the Index table in `design-rules/adr/README.md`. +4. Cross-link from the relevant per-component `DESIGN_RULES.md` files in their `Read first` section. ### Add a per-component `DESIGN_RULES.md` 1. Verify the directory exists on disk and contains real source (not just `__pycache__`). @@ -132,7 +130,7 @@ Apply new changes to existing rules. Triggered by "update design rule for X", "t 1. Identify the affected files: principle file, ADR(s), per-component file(s). 2. Make minimal edits in place. Never duplicate content between global and component files. -3. If a previously-recorded rule is now wrong because the implementation changed, **update the rule, do not add a contradicting one**. If the change is significant enough to be a decision reversal, supersede the relevant ADR by adding a new ADR and marking the old one's Status as `Superseded by ADR-NNN` (the only allowed Status besides `Accepted`). +3. If a previously-recorded rule is now wrong because the implementation changed, **update the rule, do not add a contradicting one**. If the change is significant enough to be a decision reversal, add a new ADR that overturns the prior decision and **delete** the old ADR in the same PR — update every reference to the old ADR ID repo-wide (M2). See `design-rules/adr/README.md` for the full supersession flow. 4. If a per-component file is updated, also check whether the global ADR / principle / security standard it references still matches. Fix upstream first if both are wrong. --- @@ -154,7 +152,7 @@ Check code or a diff against the design rules. Triggered by "review against desi |---|---|---| | Tenant isolation bypass | New model or queryset bypassing org scoping | `security/tenant-isolation.md` (Three-Layer Defense) | | SQL injection | String interpolation into SQL inside `unstract/connectors/.../databases/**` | inline SQL Safety Standard S1 in that directory's `DESIGN_RULES.md` | - | Missing auth | New endpoint without authentication | P8 fail-closed | + | Missing auth | New endpoint without authentication | P5 fail-closed | | Direct org FK on derived model | New model that doesn't inherit org through a parent | P6, ADR-002 | | Audit/billing CASCADE | New audit/billing record using FK CASCADE to parent | P4, ADR-003 | | Credential duplication | Storing the same credential twice instead of referencing | P2 | @@ -199,6 +197,6 @@ The script checks: forbidden-word scan, compatibility-statement presence in ever | `design-rules/security/standards.md` | SQL Safety S1 + current protection patterns as rules. | | `design-rules/adr/ADR-*.md` | 8 accepted ADRs. | | `unstract/connectors/src/unstract/connectors/databases/DESIGN_RULES.md` | SQL Safety Standard S1 inlined for the 8 DB connectors. | -| `backend/<app>/DESIGN_RULES.md` × 22 | Per-Django-app component files. | -| `unstract/<component>/DESIGN_RULES.md` × 9 | Per-shared-lib component files. | -| `workers/<component>/DESIGN_RULES.md` × 10 | Per-worker component files. | +| `backend/**/DESIGN_RULES.md` | Per-Django-app component files (where present). | +| `unstract/**/DESIGN_RULES.md` | Per-shared-lib component files (where present). | +| `workers/**/DESIGN_RULES.md` | Per-worker component files (where present). | diff --git a/.claude/skills/design-rules/scripts/validate.sh b/.claude/skills/design-rules/scripts/validate.sh index 4f4bbca34b..1ee67faec4 100755 --- a/.claude/skills/design-rules/scripts/validate.sh +++ b/.claude/skills/design-rules/scripts/validate.sh @@ -10,6 +10,9 @@ # and contains real source (not just __pycache__). # # Exit codes: 0 = clean, 1 = problems found. +# +# Portability: bash 3.2+ (macOS default). Avoids `mapfile`/`readarray` and +# avoids expanding potentially-empty arrays under `set -u`. set -u fail=0 @@ -17,43 +20,60 @@ fail=0 ROOT="$(git rev-parse --show-toplevel 2>/dev/null || pwd)" cd "$ROOT" || exit 2 +# Populate COMPONENT_FILES portably (no mapfile — bash 3.2 compatible). +COMPONENT_FILES=() +while IFS= read -r -d '' f; do + COMPONENT_FILES+=("$f") +done < <(find backend unstract workers -name DESIGN_RULES.md -print0 2>/dev/null) + echo "==> 1. Forbidden-word scan" FORBIDDEN='cloud|enterprise|unstract-cloud|hitl|manual.review|subscription|agentic|simple.prompt.studio|public.shares|greptile|AuditLog|SoftDeleteMixin|departure workflow|GDPR|OWNER role|ManagementKey|ExecutionKey|uxm_|uxe_|deletion guard|informed deletion' -mapfile -t COMPONENT_FILES < <(find backend unstract workers -name DESIGN_RULES.md 2>/dev/null) - -if grep -rEl -i "$FORBIDDEN" \ - design-rules/ "${COMPONENT_FILES[@]}" 2>/dev/null; then - echo " FAIL: forbidden words found in the files listed above" - fail=1 +check1_fail=0 +if [[ ${#COMPONENT_FILES[@]} -gt 0 ]]; then + if grep -rEl -i "$FORBIDDEN" \ + design-rules/ "${COMPONENT_FILES[@]}" 2>/dev/null; then + echo " FAIL: forbidden words found in the files listed above" + check1_fail=1 + fail=1 + fi else - echo " OK" + if grep -rEl -i "$FORBIDDEN" design-rules/ 2>/dev/null; then + echo " FAIL: forbidden words found in the files listed above" + check1_fail=1 + fail=1 + fi fi +[[ $check1_fail -eq 0 ]] && echo " OK" echo echo "==> 2. Compatibility statement presence" COMPAT='All changes to this component must remain compatible' -for f in "${COMPONENT_FILES[@]}"; do +check2_fail=0 +for f in "${COMPONENT_FILES[@]+"${COMPONENT_FILES[@]}"}"; do if ! grep -q "$COMPAT" "$f"; then echo " MISSING: $f" + check2_fail=1 fail=1 fi done -[[ $fail -eq 0 ]] && echo " OK" +[[ $check2_fail -eq 0 ]] && echo " OK" echo echo "==> 3. Component directory sanity" -for f in "${COMPONENT_FILES[@]}"; do +check3_fail=0 +for f in "${COMPONENT_FILES[@]+"${COMPONENT_FILES[@]}"}"; do dir="$(dirname "$f")" # Real source = at least one file other than DESIGN_RULES.md and __pycache__ real=$(find "$dir" -maxdepth 1 -mindepth 1 \ ! -name DESIGN_RULES.md ! -name __pycache__ 2>/dev/null | head -1) if [[ -z "$real" ]]; then echo " EMPTY DIR: $f (no real source alongside)" + check3_fail=1 fail=1 fi done -[[ $fail -eq 0 ]] && echo " OK" +[[ $check3_fail -eq 0 ]] && echo " OK" echo if [[ $fail -eq 0 ]]; then diff --git a/design-rules/adr/ADR-006-organization-rate-limit-separation.md b/design-rules/adr/ADR-006-organization-rate-limit-separation.md index 6880fab070..5d3a72394e 100644 --- a/design-rules/adr/ADR-006-organization-rate-limit-separation.md +++ b/design-rules/adr/ADR-006-organization-rate-limit-separation.md @@ -6,7 +6,7 @@ Per-organization configuration values are stored in `Configuration`. Rate limits ## Decision -`OrganizationRateLimit` is a separate model from `Configuration`. It lives in the `configuration` app but is not stored as a `Configuration` row. +`OrganizationRateLimit` is a separate model from `Configuration`. It is defined in `backend/api_v2/models.py` (the `api_v2` app) and is not stored as a `Configuration` row. ## Consequences diff --git a/design-rules/security/tenant-isolation.md b/design-rules/security/tenant-isolation.md index 509c18e15a..521f44a584 100644 --- a/design-rules/security/tenant-isolation.md +++ b/design-rules/security/tenant-isolation.md @@ -9,8 +9,8 @@ Tenant isolation in this codebase is enforced by three independent layers in the | | | |---|---| | **Component** | `CustomAuthMiddleware` | -| **Behaviour** | Validates that the authenticated user belongs to the organization addressed by the request and stores the resolved `org_id` in a thread-local `StateStore`. Downstream model managers and filter backends read the org id from this store. | -| **Fail mode** | If the org cannot be resolved, the middleware fails closed (P5) and the request is rejected. | +| **Behaviour** | Validates the request's identity, resolves the active organization from the session/platform key, and stores the resolved `org_id` in a thread-local `StateStore`. Downstream model managers and filter backends read the org id from this store. | +| **Fail mode** | Rejects the request with 403 when the request targets a specific organization (`request.organization_id` is set on the URL) and the session cannot be resolved to that org. Requests that do not target a specific organization are allowed through with no org set in `StateStore`, and are then fail-closed at Layer 2/3 when they touch tenant data. Layer 1 is *identity-and-binding*, not the primary fail-closed boundary — that role belongs to Layer 3 below (P5). | --- diff --git a/workers/shared/DESIGN_RULES.md b/workers/shared/DESIGN_RULES.md index ad0d47804d..50cb98159e 100644 --- a/workers/shared/DESIGN_RULES.md +++ b/workers/shared/DESIGN_RULES.md @@ -53,7 +53,7 @@ This file follows the [per-component contract](../../design-rules/per-component- | **Severity** | MUST | | **Why** | Workers run in Docker images without a Django process. Importing `django.*`, Django models, or Django settings from `workers/shared` turns every worker into a Django dependency and defeats the whole purpose of the new workers architecture (the system must work with AND without the new workers). Data coming from the backend must be consumed as JSON payloads and mapped into local dataclasses. | | **Refs** | `principles.md#P6` · `CLAUDE.md` (architecture principles) | -| **Enforced by** | not yet enforced — code review only (no import-linter wired in CI today) | +| **Enforced by** | code review only | ### R4 — Celery task signatures are JSON-serializable only From 9c05c93d89852ac998bbdb1d27b822ce8492820f Mon Sep 17 00:00:00 2001 From: ali <muhammad.ali@zipstack.com> Date: Wed, 8 Apr 2026 16:49:51 +0530 Subject: [PATCH 5/7] UN-3396 [MISC] Make Known Exceptions section optional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "## Known Exceptions" section was previously mandatory — the contract said absence meant "the contract is not being followed" and the single line `None.` was required when the file had no exceptions. In practice this is cargo culting. The baseline for Known Exceptions is zero: most files will never have one because exceptions are rare by definition. Mandating a `None.` placeholder on every file: - Adds ~4 lines of noise per per-component file (trained readers to skip the section) - Gives the same failure mode as optional (`None.` written reflexively without the author evaluating whether exceptions exist is indistinguishable from a missing section) - Defeats the purpose of the section when a real exception is added, because readers have learned to scroll past it Change the contract to require the section only when at least one intentional, accepted exception exists. Absence of the section now means "no known exceptions today" — equivalent to `None.` but without the placeholder noise. Also tightened the Known Exceptions definition to be explicit that an entry documents an *evaluated, accepted* deviation, not "drift we haven't decided about yet." Unevaluated drift belongs in the issue tracker, not in a Known Exceptions entry. This is the lesson from the ExecutionViewSet finding on PR #1908 — a Known Exception added for unexamined drift would be fraud-by-documentation. Files changed: * design-rules/per-component-contract.md - Section structure row #8 now says the section is optional and describes the presence-only semantics. - Known Exceptions format prose updated: explicit "optional", no `None.` placeholder, entry semantics hardened to "evaluated, accepted" only. * backend/account_v2/DESIGN_RULES.md * backend/workflow_manager/DESIGN_RULES.md * unstract/connectors/DESIGN_RULES.md * unstract/connectors/src/unstract/connectors/databases/DESIGN_RULES.md * workers/shared/DESIGN_RULES.md - Dropped the empty `## Known Exceptions` / `None.` block from each prototype file. All 5 files now go straight from the last rule (or horizontal rule) to `## Checklist`. validate.sh still passes — nothing in the script depends on the Known Exceptions section being present. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --- backend/account_v2/DESIGN_RULES.md | 4 ---- backend/workflow_manager/DESIGN_RULES.md | 4 ---- design-rules/per-component-contract.md | 8 ++++---- unstract/connectors/DESIGN_RULES.md | 4 ---- .../src/unstract/connectors/databases/DESIGN_RULES.md | 4 ---- workers/shared/DESIGN_RULES.md | 4 ---- 6 files changed, 4 insertions(+), 24 deletions(-) diff --git a/backend/account_v2/DESIGN_RULES.md b/backend/account_v2/DESIGN_RULES.md index a88064a7dc..a893e1d0e9 100644 --- a/backend/account_v2/DESIGN_RULES.md +++ b/backend/account_v2/DESIGN_RULES.md @@ -66,10 +66,6 @@ This file follows the [per-component contract](../../design-rules/per-component- --- -## Known Exceptions - -None. - ## Checklist See [Definition of Done](../../design-rules/definition-of-done.md). diff --git a/backend/workflow_manager/DESIGN_RULES.md b/backend/workflow_manager/DESIGN_RULES.md index 25e8dd0763..1d80cbd737 100644 --- a/backend/workflow_manager/DESIGN_RULES.md +++ b/backend/workflow_manager/DESIGN_RULES.md @@ -75,10 +75,6 @@ This file follows the [per-component contract](../../design-rules/per-component- --- -## Known Exceptions - -None. - ## Checklist See [Definition of Done](../../design-rules/definition-of-done.md). diff --git a/design-rules/per-component-contract.md b/design-rules/per-component-contract.md index 8bab51bc94..add986e98e 100644 --- a/design-rules/per-component-contract.md +++ b/design-rules/per-component-contract.md @@ -23,7 +23,7 @@ Every per-component `DESIGN_RULES.md` contains, in this order: | 5 | `## Scope` — two-row table with `**Covers**` and `**Excludes**` | | 6 | `## Read first` — table of files and why each binds here | | 7 | `## Rules` — `R1`..`Rn`, each rendered as an `### R<N> — <title>` heading followed by a 4-row table (Severity / Why / Refs / Enforced by). If the component has no rules yet, the section contains the single line `No component-specific rules yet.` | -| 8 | `## Known Exceptions` — `### <descriptive title>` headings with a 3-row table (Rule / Why / Tracked in), OR the single line `None.` | +| 8 | `## Known Exceptions` (optional) — present *only* when at least one intentional, accepted exception exists. Each entry is a `### <descriptive title>` heading with a 3-row table (Rule / Why / Tracked in). Omit the section entirely when there are none. | | 9 | `## Checklist` — single line linking to `definition-of-done.md` | Place a `---` horizontal rule between major content sections (after the Contract pointer, after `Read first`, after `Rules`). They create the visual breaks that make the file scannable in a github diff or markdown preview but are not themselves "sections." @@ -119,11 +119,11 @@ A rule with `Enforced by: not yet enforced` is still a real rule, but it is hone | **Tracked in** | <issue tracker ID, ADR reference, or "permanent — see Why"> | ``` -Exceptions are temporary by definition — they exist to record an accepted current violation, and they are removed when the violation is fixed. They are not numbered: stable IDs are for things you cite from outside, and exceptions are referred to by their topic, not by an ID. When an exception is removed, the heading simply disappears. +Exceptions are temporary by definition — they exist to record an accepted current violation, and they are removed when the violation is fixed. They are not numbered: stable IDs are for things you cite from outside, and exceptions are referred to by their topic, not by an ID. When an exception is removed, the heading simply disappears; when the last exception goes away, drop the whole `## Known Exceptions` section with it. -If there are no known exceptions, the section contains the single line `None.`. The section is mandatory — its absence means the contract is not being followed. +The section is **optional**. Include it only when at least one intentional, accepted exception exists. Absence of the section means "no known exceptions today" — do not write `None.` or a placeholder. -A `## Known Exceptions` entry is the *only* legitimate way for code to violate a rule without changing the rule. If the deviation is permanent, say so explicitly under `Tracked in:`. If the deviation is temporary, the tracker entry must hold the plan to remove it. +A `## Known Exceptions` entry is the *only* legitimate way for code to violate a rule without changing the rule. An entry documents an **evaluated, accepted** deviation, not "we discovered some drift we haven't decided about yet." Unevaluated drift belongs in the issue tracker, not here. If the deviation is permanent, say so explicitly under `Tracked in:`. If the deviation is temporary, the tracker entry must hold the plan to remove it. --- diff --git a/unstract/connectors/DESIGN_RULES.md b/unstract/connectors/DESIGN_RULES.md index f0541492b9..deae24f8ac 100644 --- a/unstract/connectors/DESIGN_RULES.md +++ b/unstract/connectors/DESIGN_RULES.md @@ -74,10 +74,6 @@ This file follows the [per-component contract](../../design-rules/per-component- --- -## Known Exceptions - -None. - ## Checklist See [Definition of Done](../../design-rules/definition-of-done.md). diff --git a/unstract/connectors/src/unstract/connectors/databases/DESIGN_RULES.md b/unstract/connectors/src/unstract/connectors/databases/DESIGN_RULES.md index f46f505dac..a099b9c9df 100644 --- a/unstract/connectors/src/unstract/connectors/databases/DESIGN_RULES.md +++ b/unstract/connectors/src/unstract/connectors/databases/DESIGN_RULES.md @@ -84,10 +84,6 @@ This file follows the [per-component contract](../../../../../../design-rules/pe --- -## Known Exceptions - -None. - ## Checklist See [Definition of Done](../../../../../../design-rules/definition-of-done.md). diff --git a/workers/shared/DESIGN_RULES.md b/workers/shared/DESIGN_RULES.md index 50cb98159e..e3899b45b0 100644 --- a/workers/shared/DESIGN_RULES.md +++ b/workers/shared/DESIGN_RULES.md @@ -84,10 +84,6 @@ This file follows the [per-component contract](../../design-rules/per-component- --- -## Known Exceptions - -None. - ## Checklist See [Definition of Done](../../design-rules/definition-of-done.md). From 6924017630e9e4042a17c46b8b005e60bb36d7c8 Mon Sep 17 00:00:00 2001 From: ali <muhammad.ali@zipstack.com> Date: Wed, 8 Apr 2026 16:52:00 +0530 Subject: [PATCH 6/7] UN-3396 [MISC] Align SKILL.md terminology with canonical contract format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit (2026-04-08T11:10Z) flagged two remaining terminology drifts in the design-rules skill instructions: 1. The "What you'll get" row for the `validate` mode claimed the script checks "ADR-link resolution" — the script does not, and has never done so. Corrected to list the three actual checks: forbidden-word scan, compatibility-statement presence, and component-directory sanity. 2. Five places inside the skill still used the old "Dos / Don'ts / Acceptance criteria" vocabulary to describe per-component content, which contradicts the canonical per-component contract requiring `## Rules` with `R1..Rn` entries and the four-row (Severity / Why / Refs / Enforced by) table. Replaced each with the canonical terminology: - Quick Start "get" row: "component Dos/Don'ts" → "the component's `## Rules` entries (`R1..Rn` with Severity / Why / Refs / Enforced by)" - Command `get` step 3 bundle item: same replacement, plus an explicit mention of `## Known Exceptions` entries when present - Command `add` per-component step 4: dropped "Dos / Don'ts in source material" wording, replaced with the canonical `No component-specific rules yet.` placeholder under `## Rules` and an explicit warning that inventing Dos/Don'ts is forbidden - Command `review` step 3: walk the component's `## Rules` (`R1..Rn`) rather than "the component's Dos / Don'ts" The only remaining "Dos/Don'ts" mention in SKILL.md is the explicit prohibition in the `add` step — it tells contributors the old shape is not allowed, not that they should use it. No behavioural change. Purely terminology alignment with the contract. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --- .claude/skills/design-rules/SKILL.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.claude/skills/design-rules/SKILL.md b/.claude/skills/design-rules/SKILL.md index 4d33231f27..a35f37773c 100644 --- a/.claude/skills/design-rules/SKILL.md +++ b/.claude/skills/design-rules/SKILL.md @@ -34,11 +34,11 @@ A skill for managing and applying the version-controlled design rule system in t | Mode | Output | |------|--------| | **list** | Compact text list (titles + numbers, no bodies) | -| **get** | Deduped bundle: compatibility statement, relevant principles, AI Review Checklist, referenced ADRs, security standards, component Dos/Don'ts | +| **get** | Deduped bundle: compatibility statement, relevant principles, AI Review Checklist, referenced ADRs, security standards, and the component's `## Rules` entries (`R1..Rn` with Severity / Why / Refs / Enforced by) | | **add** | New file written + cross-links updated; refused if content describes an aspiration rather than implemented behaviour | | **update** | Minimal in-place edit; ADR superseded with a new ADR if the change is a reversal | | **review** | Per-file findings table: file, line, principle/ADR violated, why, minimal fix | -| **validate** | Pass/fail report from forbidden-word grep, compatibility-statement check, ADR-link resolution | +| **validate** | Pass/fail report from forbidden-word scan, compatibility-statement presence check, and component-directory sanity check | --- @@ -81,7 +81,7 @@ Fetch the rules that actually apply to a target. - AI Review Checklist (always) - ADRs referenced by any collected component file - Security standards referenced (e.g. `security/tenant-isolation.md`, `security/standards.md`, the inline SQL Safety Standard under `unstract/connectors/.../databases/`) - - The component-specific Dos / Don'ts / Acceptance criteria from each collected `DESIGN_RULES.md` + - The component-specific `## Rules` entries (`R1..Rn` with Severity / Why / Refs / Enforced by) from each collected `DESIGN_RULES.md`, plus any `## Known Exceptions` entries when present 4. **Deduplicate.** Never show the same principle or ADR twice. 5. If asked for "all" design rules, dump the global files (`design-rules/**`) without per-component noise. @@ -120,7 +120,7 @@ Create a new rule, ADR, or per-component file. 1. Verify the directory exists on disk and contains real source (not just `__pycache__`). 2. Use the standard template from `design-rules/README.md`. Always include the verbatim compatibility statement. 3. Include "Read first" links to `principles.md`, `ai-review-checklist.md`, and any relevant ADRs / security standards. -4. If the component has no specific Dos / Don'ts in source material, write the boilerplate body with `No component-specific rules beyond the global principles.` +4. If the component has no specific rules yet, use the canonical placeholder under `## Rules`: `No component-specific rules yet.` Do not invent Dos/Don'ts — the canonical format is `R1..Rn` entries with the full four-row table. --- @@ -141,7 +141,7 @@ Check code or a diff against the design rules. Triggered by "review against desi 1. **Determine the target.** Accept: a list of changed files, a PR number (use `gh pr diff <n> --name-only`), `git diff --name-only` for local changes, or a single file. 2. **Run `get`** on those targets to collect all applicable rules. -3. **For each changed file**, walk through the AI Review Checklist questions (`design-rules/ai-review-checklist.md`) and the component's Dos / Don'ts. For every violation, output: +3. **For each changed file**, walk through the AI Review Checklist questions (`design-rules/ai-review-checklist.md`) and the component's `## Rules` entries (`R1..Rn`). For every violation, output: - File and line (when known) - Principle / ADR / security standard violated - Why it's a violation From 66ab7648e6d4c70b5350c53d6db1afac5dc979f7 Mon Sep 17 00:00:00 2001 From: ali <muhammad.ali@zipstack.com> Date: Wed, 8 Apr 2026 17:08:20 +0530 Subject: [PATCH 7/7] UN-3396 [MISC] Address coderabbit round 3 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight new CodeRabbit findings landed on PR #1908 after the terminology-alignment push. Seven are real; one is a stale re-scan of an already-fixed item. Fixing the seven in place; the eighth (SKILL.md fence without language identifier at line 103) is already resolved by commit 3eda6e7b5 and needs no action. Changes: * backend/account_v2/DESIGN_RULES.md - Read first table: principle reference for fail-closed was P8, now P5. P8 is "internal vs external surfaces," P5 is "fail closed on permission decisions" — R2 is about the second one. - R2 Refs: same fix (`principles.md#P8` → `principles.md#P5`). - R4 Enforced by: was `not yet enforced — see project issue tracker (credential lifecycle)`. The contract requires a concrete tracker reference for `not yet enforced`; no real tracker ID exists yet. Downgraded to `code review only` (the actual current enforcement state) — same move previously applied to workers/shared R3. If a real tracker ID ever gets assigned, flip back to the `not yet enforced` form with the ID. * unstract/connectors/DESIGN_RULES.md - Read first table: P8 → P5 (same fail-closed fix as account_v2). - R4 (Connector errors surface as the framework's exception hierarchy): Refs previously pointed at `principles.md#P8`, which doesn't fit — R4 is about type-boundary hygiene between driver exceptions and the framework exception hierarchy, not about URL group / auth middleware separation (which is what P8 actually says). No existing principle fits cleanly. Removed the principle reference entirely; Refs now points only at `security/standards.md`, which is honest about the rule's provenance. * design-rules/principles.md - P1 Implementation row previously named only `DefaultOrganizationMixin`, which is only the model mixin (adds the FK and auto-populates it on save). The query-time filter is provided by `DefaultOrganizationManagerMixin` (a separate manager mixin living in the same file — verified in backend/utils/models/organization_mixin.py). Updated the row to name both classes and explain the split: the model mixin adds the FK, the manager mixin filters queries, and together they provide auto-scoping for direct-FK models. CodeRabbit's suggestion to "reconcile to one name" was based on the incorrect assumption that the two names refer to the same class — they don't. * design-rules/per-component-contract.md - Rule format section: softened the overclaim that `validate.sh` "checks the field presence" of rule rows. The script does not parse rule tables today; it only greps for the Compatibility substring, the forbidden-word list, and directory sanity. Reviewers are the current enforcement of rule-field presence, with a note that a future hardening pass may extend the script. - "When to add a per-component file" section: removed the stale "`## Known Exceptions` and `## Checklist` sections are still required" wording, which contradicted the earlier commit making `## Known Exceptions` optional. Now says only `## Checklist` is still required, and Known Exceptions is included only when an accepted exception exists. - Self-update reminder: softened item 2 to describe what `validate.sh` actually greps for (Compatibility blockquote wording, forbidden-word list) instead of the `## Checklist` section header and rule field names (which it does not currently grep for). Not addressed (deliberately): * SKILL.md line 103 fence language identifier: already fixed in commit 3eda6e7b5 (the fence is `` ```markdown ``, not bare). The CodeRabbit finding at 11:22:54Z is a stale re-scan of the pre-alignment state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --- backend/account_v2/DESIGN_RULES.md | 6 +++--- design-rules/per-component-contract.md | 6 +++--- design-rules/principles.md | 2 +- unstract/connectors/DESIGN_RULES.md | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/backend/account_v2/DESIGN_RULES.md b/backend/account_v2/DESIGN_RULES.md index a893e1d0e9..f8e7d6f3e5 100644 --- a/backend/account_v2/DESIGN_RULES.md +++ b/backend/account_v2/DESIGN_RULES.md @@ -19,7 +19,7 @@ This file follows the [per-component contract](../../design-rules/per-component- | File | Why it binds here | |---|---| -| [`principles.md`](../../design-rules/principles.md) | P1 (org scoping), P2 (credentials), P8 (fail-closed) | +| [`principles.md`](../../design-rules/principles.md) | P1 (org scoping), P2 (credentials), P5 (fail-closed) | | [`ai-review-checklist.md`](../../design-rules/ai-review-checklist.md) | 9 questions every change must answer | | [`security/tenant-isolation.md`](../../design-rules/security/tenant-isolation.md) | Three-Layer Defense — `account_v2` owns Layer 1 (middleware) and is the root of Layer 3 (filter backend) | | [`adr/ADR-001`](../../design-rules/adr/ADR-001-org-scoping-query-layer.md) | Org scoping is enforced at the query layer, not via RLS | @@ -43,7 +43,7 @@ This file follows the [per-component contract](../../design-rules/per-component- |---|---| | **Severity** | MUST | | **Why** | The middleware is Layer 1 of the Three-Layer Defense — it validates the user belongs to the org and stores `org_id` in the thread-local `StateStore`. Reading the org from a header, request param, or cookie directly bypasses validation and is a tenant boundary violation. | -| **Refs** | `principles.md#P8` · `security/tenant-isolation.md` | +| **Refs** | `principles.md#P5` · `security/tenant-isolation.md` | | **Enforced by** | `CustomAuthMiddleware` + code review | ### R3 — Org membership has exactly one source of truth: `OrganizationMember` @@ -62,7 +62,7 @@ This file follows the [per-component contract](../../design-rules/per-component- | **Severity** | MUST | | **Why** | Duplicating a credential makes rotation impossible and leaves stale copies behind on member departure. `User`-owned credentials must be referenced by ID from anywhere that consumes them, never copied. | | **Refs** | `principles.md#P2` | -| **Enforced by** | not yet enforced — see project issue tracker (credential lifecycle) | +| **Enforced by** | code review only | --- diff --git a/design-rules/per-component-contract.md b/design-rules/per-component-contract.md index add986e98e..1bb69d1936 100644 --- a/design-rules/per-component-contract.md +++ b/design-rules/per-component-contract.md @@ -76,7 +76,7 @@ Every rule in the `## Rules` section uses this exact format — an `H3` heading | **Enforced by** | <test path | middleware | CI check | code review only | not yet enforced — <ref>> | ``` -Every rule must have all four rows (Severity, Why, Refs, Enforced by). `validate.sh` checks the field presence, not the field content. +Every rule must have all four rows (Severity, Why, Refs, Enforced by). Rule-field presence and checklist-section presence are checked at code review today — `validate.sh` does not yet parse rule tables; it currently only verifies the Compatibility blockquote substring, the forbidden-word list, and component-directory sanity. A future hardening pass may extend it to enforce the full rule format; until then, reviewers are the enforcement. The middle dot (`·`) is the canonical separator for multiple `Refs` entries — easier to scan than a comma in a table cell. @@ -171,7 +171,7 @@ Why: a rule system that contradicts itself is no rule system at all. Reviewers c - Whenever a new active Django app is added under `backend/`, a new shared library directory is added under `unstract/`, or a new worker directory is added under `workers/`. M1 above. - Copy the structure from a real, current file — `backend/account_v2/DESIGN_RULES.md` is the canonical reference. -- If the new component has no specific rules yet, the `## Rules` section contains a single line: `No component-specific rules yet.` The file is still mandatory, and the `## Known Exceptions` and `## Checklist` sections are still required. +- If the new component has no specific rules yet, the `## Rules` section contains a single line: `No component-specific rules yet.` The file is still mandatory; keep `## Checklist`, and include `## Known Exceptions` only when at least one accepted exception exists. --- @@ -189,7 +189,7 @@ Why: a rule system that contradicts itself is no rule system at all. Reviewers c If you change this contract, expect to: 1. Update [`definition-of-done.md`](definition-of-done.md) if the change affects the checklist body, severity vocabulary, or M1/M2/M3 wording. -2. Update `.claude/skills/design-rules/scripts/validate.sh` if any literal string the script greps for changes (the Compatibility blockquote, the `## Checklist` section header, or the rule field names). +2. Update `.claude/skills/design-rules/scripts/validate.sh` if the Compatibility blockquote wording changes or the forbidden-word list needs adjusting — those are the literal strings the script currently greps for. (The script does not today enforce rule-format or section-header literals; if a future hardening pass adds those checks, list them here.) 3. Update `.claude/skills/design-rules/SKILL.md` if the contract introduces a new section that the `get` or `review` commands must surface. 4. Re-run `validate.sh` against every per-component file in the repo. 5. If the section structure itself changes, the change touches every per-component file. That is intentional: a structural change is a real event and a 41-file diff is the right blast radius. diff --git a/design-rules/principles.md b/design-rules/principles.md index 15251b6d87..f65338b31a 100644 --- a/design-rules/principles.md +++ b/design-rules/principles.md @@ -13,7 +13,7 @@ Principles are referenced from per-component files via `principles.md#PN` (e.g. | | | |---|---| | **Rule** | Every model that holds tenant data must be reachable from `Organization` and must be filtered by the requesting user's organization on every read and write. | -| **Implementation** | Use `DefaultOrganizationMixin` for direct FK ownership, and `OrgAwareManager` or `OrganizationFilterBackend` for indirect ownership through a parent FK chain. See [`security/tenant-isolation.md`](security/tenant-isolation.md) for the Three-Layer Defense. | +| **Implementation** | Tenant models with a direct `organization` FK use `DefaultOrganizationMixin` (model mixin — adds the FK and auto-populates it on save) together with `DefaultOrganizationManagerMixin` (manager mixin — filters every queryset by the current org). Models that reach `Organization` only through a parent FK chain use `OrgAwareManager` or rely on `OrganizationFilterBackend` (the view-layer backend that BFS-walks the FK chain). See [`security/tenant-isolation.md`](security/tenant-isolation.md) for the full Three-Layer Defense. | | **Example** | A new model that stores documents must add a direct `organization` FK or a path that BFS-resolves to one. A new DRF view on a tenant model must inherit the project's filter backend so cross-org reads are impossible. | --- diff --git a/unstract/connectors/DESIGN_RULES.md b/unstract/connectors/DESIGN_RULES.md index deae24f8ac..b9ae5f8ec9 100644 --- a/unstract/connectors/DESIGN_RULES.md +++ b/unstract/connectors/DESIGN_RULES.md @@ -19,7 +19,7 @@ This file follows the [per-component contract](../../design-rules/per-component- | File | Why it binds here | |---|---| -| [`principles.md`](../../design-rules/principles.md) | P2 (credentials), P8 (fail-closed) | +| [`principles.md`](../../design-rules/principles.md) | P2 (credentials), P5 (fail-closed) | | [`ai-review-checklist.md`](../../design-rules/ai-review-checklist.md) | 9 questions every change must answer | | [`security/standards.md`](../../design-rules/security/standards.md) | `EncryptedBinaryField` on `ConnectorInstance.connector_metadata`, SQL Safety Standard (S1) for the databases subtree | @@ -60,7 +60,7 @@ This file follows the [per-component contract](../../design-rules/per-component- |---|---| | **Severity** | MUST | | **Why** | Driver-level exceptions (`psycopg2.*`, `google.api_core.*`, `boto3.ClientError`, …) must be wrapped in the `unstract.connectors.exceptions` hierarchy before leaving the connector. Callers depend on a stable error contract to distinguish retryable, auth, and misconfiguration failures — leaking the raw driver exception couples callers to the driver and defeats retry/backoff policy. | -| **Refs** | `principles.md#P8` | +| **Refs** | `security/standards.md` | | **Enforced by** | code review only | ### R5 — Database connectors inherit the SQL Safety Standard from `databases/`