UN-3396 [MISC] Introduce design-rules system with prototype per-component files#1908
Open
muhammad-ali-e wants to merge 9 commits intomainfrom
Open
UN-3396 [MISC] Introduce design-rules system with prototype per-component files#1908muhammad-ali-e wants to merge 9 commits intomainfrom
muhammad-ali-e wants to merge 9 commits intomainfrom
Conversation
…nent 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) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughAdds a repository-wide design-rules system: global rule documents under Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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>
Contributor
|
| Filename | Overview |
|---|---|
| .claude/skills/design-rules/scripts/validate.sh | All three issues flagged in previous review rounds have been addressed: per-check failure flags prevent ambiguous output, mapfile replaced with portable while-loop, and Compatibility grep pattern updated. Script is clean and passes its own checks. |
| .claude/skills/design-rules/SKILL.md | Previously incorrect P8 trap reference in the review command table has been corrected to P5 (fail-closed). The skill's command set, path reference table, and review traps are all consistent with the global design-rules files. |
| workers/shared/DESIGN_RULES.md | R3's previously non-conforming Enforced by value is now code review only, which is correct. Minor remaining issue: R3 Refs cites CLAUDE.md which falls outside the canonical design-rules reference vocabulary. |
| backend/account_v2/DESIGN_RULES.md | Canonical prototype file; correctly follows per-component-contract structure (title, intro, Compatibility blockquote, contract pointer, Scope, Read first, Rules, Checklist). All four rule fields populated and relative paths resolve correctly. |
| backend/workflow_manager/DESIGN_RULES.md | Anchor-entity prototype file. R3 (audit-durable on_delete) correctly encodes the cascade regression motivating this PR. R5 (JSON-only Celery) is M3-consistent with workers/shared R4. All section structure and rule tables conform to contract. |
| unstract/connectors/src/unstract/connectors/databases/DESIGN_RULES.md | Deep-nested component (6 ../ hops verified). Correctly specialises S1 into R1–R6 for all 8 DB engines. Parent connector framework and global security/standards.md cross-linked appropriately. All rule tables complete. |
| unstract/connectors/DESIGN_RULES.md | Parent library prototype; R5 correctly delegates SQL rules to the databases/ subtree DESIGN_RULES.md. Contract pointer path resolves correctly. Relative link to databases/ DESIGN_RULES.md verified accurate. |
| design-rules/per-component-contract.md | Updated to reflect the new Compatibility blockquote grep pattern (All changes to this component must remain compatible). Self-update reminder section is present and complete. Contract is internally consistent with all five prototype files. |
| design-rules/principles.md | New file; defines P1–P9 with Rule / Implementation / Example rows. P5 (fail-closed) and P8 (internal/external separation) are clearly distinguished, resolving the previous P8-vs-P5 confusion in the skill. |
| .gitignore | Correctly un-ignores .claude/skills/design-rules/ and its contents while keeping other .claude/ directories ignored. The .sh global ignore is overridden by the subsequent !.claude/skills/design-rules/* negation, so validate.sh will be tracked. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["AI Agent / PR Reviewer\ntouches a file"] --> B["Walk up to repo root\ncollecting DESIGN_RULES.md"]
B --> C{"DESIGN_RULES.md\nfound?"}
C -- "Yes" --> D["Load per-component rules\n(Scope · Read first · R1..Rn)"]
C -- "No" --> E["Only global rules apply"]
D --> F["Load referenced global files"]
E --> F
F --> G["design-rules/principles.md\nP1–P9"]
F --> H["design-rules/ai-review-checklist.md\n9 questions"]
F --> I["design-rules/adr/ADR-NNN.md\n8 accepted ADRs"]
F --> J["design-rules/security/*.md\nTenant isolation · SQL S1"]
G & H & I & J --> K["Deduplicated rule bundle"]
K --> L["Review code changes\nagainst rules"]
L --> M{"Violations\nfound?"}
M -- "Yes" --> N["Flag per: file · line\nprinciple/ADR · fix"]
M -- "No" --> O["Proceed to merge"]
P["validate.sh"] --> Q["1. Forbidden-word scan\n(cloud, enterprise, HITL, …)"]
P --> R["2. Compatibility blockquote\npresent in every file?"]
P --> S["3. Component dir\ncontains real source?"]
Q & R & S --> T{"All pass?"}
T -- "Yes" --> U["exit 0"]
T -- "No" --> V["exit 1 — list failures"]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: workers/shared/DESIGN_RULES.md
Line: 55
Comment:
**Non-standard `Refs` target: `CLAUDE.md` is outside the design-rules system**
R3's `Refs` row cites `` `CLAUDE.md` (architecture principles) ``, but the per-component contract establishes a canonical Refs vocabulary of `principles.md#PN`, `adr/ADR-NNN`, and `security/...`. `CLAUDE.md` is an AI-assistant instructions file that sits outside the `design-rules/` hierarchy and is not surfaced by the skill's `get` command when collecting applicable rules.
The "no-Django in workers" constraint is arguably already covered by `principles.md#P6` (execution inherits org from parent entity, not from Django thread-locals), but since that principle doesn't explicitly name the import boundary, a cleaner option is to add a dedicated ADR (e.g. `ADR-015-no-django-in-workers.md`) and reference it here:
```suggestion
| **Refs** | `principles.md#P6` · `principles.md#P8` |
```
Or, once the ADR exists:
```
| **Refs** | `principles.md#P6` · `adr/ADR-015` |
```
Using `CLAUDE.md` as a Ref value means a reviewer/AI agent following the `get` path will find a broken chain — the skill's `get` command only assembles rule bundles from files inside `design-rules/` and the nested `DESIGN_RULES.md` hierarchy.
How can I resolve this? If you propose a fix, please make it concise.Reviews (5): Last reviewed commit: "Merge branch 'main' into misc/UN-3396-MI..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…rmat 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>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
workers/shared/DESIGN_RULES.md (1)
31-38: Consider clarifying R1 scope for external service calls.R1's title "All backend traffic goes through
InternalAPIClient" could be more precise. The "Why" field correctly specifies "worker-to-backend call," but the title might be misread as covering all HTTP traffic from workers (including external services like x2text-service/runner endpoints).Consider rephrasing the title to: "All worker-to-backend calls go through
InternalAPIClient" or adding a parenthetical note excluding external service calls.📝 Optional clarification
-### R1 — All backend traffic goes through `InternalAPIClient` +### R1 — All worker-to-backend calls go through `InternalAPIClient`Or alternatively, add a clarifying note in the "Why" field:
| **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. | +| | (External service calls—e.g., to x2text-service or other non-backend runners—may use plain `requests` as they are not subject to internal auth requirements.) |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/shared/DESIGN_RULES.md` around lines 31 - 38, Update R1's heading and/or Why to clarify scope: change the title "All backend traffic goes through `InternalAPIClient`" to "All worker-to-backend calls go through `InternalAPIClient`" (or add a parenthetical like "(excludes external services such as x2text-service/runner endpoints)"), and/or append a short sentence in the "Why" field explicitly excluding external service calls; reference R1 and InternalAPIClient so reviewers immediately see the intended scope..claude/skills/design-rules/scripts/validate.sh (1)
42-42: Per-check "OK" message uses cumulative failure flag.The
OKmessage at line 42 (and similarly at line 56) uses the globalfailvariable, which accumulates failures across all checks. If check 1 fails, check 2 won't print "OK" even if it passes. This is minor but could confuse users about which specific check passed/failed.♻️ Suggested fix: track per-check status
echo echo "==> 2. Compatibility statement presence" COMPAT='All changes to this component must remain compatible' +check2_ok=1 for f in "${COMPONENT_FILES[@]}"; do if ! grep -q "$COMPAT" "$f"; then echo " MISSING: $f" fail=1 + check2_ok=0 fi done -[[ $fail -eq 0 ]] && echo " OK" +[[ $check2_ok -eq 1 ]] && echo " OK"Apply similar pattern for check 3.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/design-rules/scripts/validate.sh at line 42, The current "OK" echo uses the global fail variable so earlier failures suppress later per-check "OK" messages; update each check to use a per-check status flag (e.g., check_fail or fail_check) that you initialize to 0 before the check, set to 1 on that check's errors, and then replace occurrences of [[ $fail -eq 0 ]] && echo " OK" with [[ $check_fail -eq 0 ]] && echo " OK"; ensure the global fail still accumulates by OR-ing it with the per-check flag (fail=$(( fail | check_fail ))) so overall failure behavior is unchanged but per-check success messages are accurate..claude/skills/design-rules/SKILL.md (1)
182-187: Optional: vary repeated “Never ...” bullets for readability.This is stylistic only, but rewording one or two bullets would address the repeated sentence-beginning warning and improve scanability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/design-rules/SKILL.md around lines 182 - 187, The list in SKILL.md contains several consecutive bullets that all start with "Never ...", reducing scanability; reword one or two of those bullets to vary sentence openings while preserving meaning—e.g., change a bullet like "Never document a proposed / unimplemented design as if it were a rule." to "Do not document proposed or unimplemented designs as rules." and change "Never name a specific PR review tool inside any rule file." to "Avoid naming specific PR review tools in rule files." Update the corresponding bullets in the block shown so the intent remains identical but the phrasing varies for readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/design-rules/SKILL.md:
- Line 10: Update the SKILL.md wording to avoid overstating rollout scope and
remove hard-coded component counts: replace the phrase that claims full-repo
coverage like "each active Django app, each `unstract/` shared lib component,
and each worker" with softer language such as "per-component DESIGN_RULES.md
files (where applicable) in Django apps, shared libs, and workers" and remove
any explicit counts like "× 22", "× 9", "× 10"; instead use nondeterministic
language like "various components" or "examples include..." and note that
rollout is prototype-first and may expand, ensuring the same changes are applied
to the repeated instances around lines referenced (the sentence containing "each
active ..." and the nearby repeats at 202-204).
- Around line 103-117: The fenced ADR template block in SKILL.md uses an untyped
triple-backtick (```), triggering markdownlint MD040; update the fence to
include the language tag by changing the opening fence from ``` to ```md for the
ADR template block (the block starting with the ADR header "# ADR-NNN: <title>"
and ending with the closing ```), so the fenced code is explicitly marked as
Markdown.
In `@backend/workflow_manager/DESIGN_RULES.md`:
- Around line 40-47: ExecutionViewSet overrides filter_backends with
[DjangoFilterBackend, OrderingFilter], which bypasses the global org scoping
enforced by OrganizationFilterBackend and violates R2; fix by adding
OrganizationFilterBackend to ExecutionViewSet.filter_backends (or revert to
using DEFAULT_FILTER_BACKENDS) so all queryset reads go through org scoping, or
if this is intentional, document the exception and justification in
DESIGN_RULES.md under Known Exceptions referencing ExecutionViewSet and the
omitted OrganizationFilterBackend.
In `@design-rules/adr/ADR-006-organization-rate-limit-separation.md`:
- Line 9: The ADR incorrectly states that OrganizationRateLimit lives in the
configuration app; in reality the OrganizationRateLimit model is implemented in
the api_v2 app (models.py) rather than as a Configuration row, so either move
the OrganizationRateLimit model into the configuration app and make it a
Configuration row, or update ADR-006-organization-rate-limit-separation.md to
state that OrganizationRateLimit is a separate model implemented in the api_v2
app (not persisted as a Configuration row) and describe the current
storage/ownership decision; reference the OrganizationRateLimit and
Configuration symbols when editing so the doc matches the code.
In `@design-rules/README.md`:
- Around line 52-54: Remove the conflicting "Status" requirement from the
top-level ADR guidance by changing the item that lists "Status / Context /
Decision / Consequences" to match the ADR contract (i.e., "Context / Decision /
Consequences"), or alternatively update the ADR contract to explicitly include a
Status field—ensure the wording for the ADR structure in the README that
currently mentions "Status" is reconciled with the ADR contract in adr/README.md
so both documents present the same required fields (search for the literal
"Status" string in the ADR guidance and the ADR contract to update the one that
should be authoritative).
In `@design-rules/security/tenant-isolation.md`:
- Around line 11-13: Update the documentation to reflect that
CustomAuthMiddleware does not universally "fail closed": state that it only
rejects when request.organization_id is present and resolution fails (per
CustomAuthMiddleware behavior) and otherwise stores None in StateStore and
allows the request; also clarify that OrgAwareManager and
OrganizationFilterBackend provide additional defense-in-depth (with
OrganizationFilterBackend as the primary fail-closed boundary) so the doc
accurately describes the conditional middleware enforcement and the layered
relationship between CustomAuthMiddleware, OrgAwareManager, and
OrganizationFilterBackend.
---
Nitpick comments:
In @.claude/skills/design-rules/scripts/validate.sh:
- Line 42: The current "OK" echo uses the global fail variable so earlier
failures suppress later per-check "OK" messages; update each check to use a
per-check status flag (e.g., check_fail or fail_check) that you initialize to 0
before the check, set to 1 on that check's errors, and then replace occurrences
of [[ $fail -eq 0 ]] && echo " OK" with [[ $check_fail -eq 0 ]] && echo "
OK"; ensure the global fail still accumulates by OR-ing it with the per-check
flag (fail=$(( fail | check_fail ))) so overall failure behavior is unchanged
but per-check success messages are accurate.
In @.claude/skills/design-rules/SKILL.md:
- Around line 182-187: The list in SKILL.md contains several consecutive bullets
that all start with "Never ...", reducing scanability; reword one or two of
those bullets to vary sentence openings while preserving meaning—e.g., change a
bullet like "Never document a proposed / unimplemented design as if it were a
rule." to "Do not document proposed or unimplemented designs as rules." and
change "Never name a specific PR review tool inside any rule file." to "Avoid
naming specific PR review tools in rule files." Update the corresponding bullets
in the block shown so the intent remains identical but the phrasing varies for
readability.
In `@workers/shared/DESIGN_RULES.md`:
- Around line 31-38: Update R1's heading and/or Why to clarify scope: change the
title "All backend traffic goes through `InternalAPIClient`" to "All
worker-to-backend calls go through `InternalAPIClient`" (or add a parenthetical
like "(excludes external services such as x2text-service/runner endpoints)"),
and/or append a short sentence in the "Why" field explicitly excluding external
service calls; reference R1 and InternalAPIClient so reviewers immediately see
the intended scope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b24e34e1-9d1f-47c2-bb88-166029af3735
📒 Files selected for processing (25)
.claude/skills/design-rules/SKILL.md.claude/skills/design-rules/scripts/validate.sh.gitignorebackend/account_v2/DESIGN_RULES.mdbackend/workflow_manager/DESIGN_RULES.mddesign-rules/README.mddesign-rules/adr/ADR-001-org-scoping-query-layer.mddesign-rules/adr/ADR-002-no-org-fk-on-execution.mddesign-rules/adr/ADR-003-usage-string-refs.mddesign-rules/adr/ADR-005-prompt-studio-registry-publish-gate.mddesign-rules/adr/ADR-006-organization-rate-limit-separation.mddesign-rules/adr/ADR-007-adapter-access-runtime-validation.mddesign-rules/adr/ADR-012-connectorauth-user-owned.mddesign-rules/adr/ADR-014-internal-external-api-separation.mddesign-rules/adr/README.mddesign-rules/ai-review-checklist.mddesign-rules/definition-of-done.mddesign-rules/lifecycle.mddesign-rules/per-component-contract.mddesign-rules/principles.mddesign-rules/security/standards.mddesign-rules/security/tenant-isolation.mdunstract/connectors/DESIGN_RULES.mdunstract/connectors/src/unstract/connectors/databases/DESIGN_RULES.mdworkers/shared/DESIGN_RULES.md
Contributor
|
Tip: Greploops — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
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>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/design-rules/SKILL.md:
- Line 37: The phrase "component Dos/Don'ts" (and any other occurrences of
"Dos/Don'ts/Acceptance criteria") in SKILL.md should be changed to the canonical
per-component contract format: replace those references with "## Rules" and
require enumerated entries R1..Rn, each accompanied by the mandated table fields
(Severity / Why / Refs / Enforced by); update the line containing "| **get** |
... component Dos/Don'ts |" to describe "## Rules (R1..Rn) with table: Severity
/ Why / Refs / Enforced by" and make the same wording change for the other
instances that mention Dos/Don'ts/Acceptance criteria so the file uses the
canonical terminology everywhere.
- Around line 41-42: Update the "validate" output description in SKILL.md (the
row labeled validate) so it matches the actual validator behavior: list the
pass/fail report for forbidden-word grep, compatibility-statement presence
check, and component-directory sanity checks, and remove the mention of ADR-link
resolution (which the script does not perform); ensure the wording references
the same "validate" label so readers can map it to the validator script results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd9eee08-4520-4b57-94ee-3e2e27ef7fa3
📒 Files selected for processing (5)
.claude/skills/design-rules/SKILL.md.claude/skills/design-rules/scripts/validate.shdesign-rules/adr/ADR-006-organization-rate-limit-separation.mddesign-rules/security/tenant-isolation.mdworkers/shared/DESIGN_RULES.md
✅ Files skipped from review due to trivial changes (4)
- design-rules/adr/ADR-006-organization-rate-limit-separation.md
- workers/shared/DESIGN_RULES.md
- design-rules/security/tenant-isolation.md
- .claude/skills/design-rules/scripts/validate.sh
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>
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>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
design-rules/README.md (1)
52-53:⚠️ Potential issue | 🟡 MinorUpdate ADR format reference to match convention.
Step 2 references "Status / Context / Decision / Consequences" but according to
adr/README.mdline 41, ADRs have no Status field. The format should be Context / Decision / Consequences only.📝 Proposed fix
-| 2 | Use the format: Status / Context / Decision / Consequences. | +| 2 | Use the format: Context / Decision / Consequences (three sections exactly). |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@design-rules/README.md` around lines 52 - 53, Update the ADR format reference in the Step 2 list entry that currently reads "Status / Context / Decision / Consequences" to match the convention used in adr/README (remove Status) so it reads "Context / Decision / Consequences"; locate the exact text string "Status / Context / Decision / Consequences" in the design-rules README and replace it with "Context / Decision / Consequences" ensuring any surrounding explanatory text remains consistent.design-rules/adr/README.md (1)
52-53:⚠️ Potential issue | 🟡 MinorUpdate ADR format reference to match "no Status field" rule.
Step 2 instructs to "Use the format: Status / Context / Decision / Consequences" but this contradicts the stated rule that ADRs have no Status field (line 41).
📝 Proposed fix
-| 2 | Use the format: Status / Context / Decision / Consequences. | +| 2 | Use the format: Context / Decision / Consequences (three sections exactly). |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@design-rules/adr/README.md` around lines 52 - 53, The README currently instructs "Use the format: Status / Context / Decision / Consequences", which contradicts the repo rule that ADRs have no Status field; edit the sentence to remove "Status" so it reads "Use the format: Context / Decision / Consequences" and ensure any nearby example or line that references "Status" is updated to match the no-Status rule (search for the exact phrase "Use the format: Status / Context / Decision / Consequences" and replace it with "Use the format: Context / Decision / Consequences" and update any adjacent examples or references accordingly).
🧹 Nitpick comments (3)
design-rules/adr/ADR-007-adapter-access-runtime-validation.md (1)
5-5: Optional: Consider more concise phrasing.For brevity, "at the moment" could be replaced with "when": "could be validated only when a workflow is created."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@design-rules/adr/ADR-007-adapter-access-runtime-validation.md` at line 5, Change the phrase "at the moment a workflow is created" to "when a workflow is created" in the sentence that currently reads "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."—update that exact sentence in ADR-007-adapter-access-runtime-validation.md to use "when" for more concise phrasing.design-rules/ai-review-checklist.md (1)
34-36: Consider splitting compound question into two separate checks.The second P4 question asks both "is the intent documented inline" AND "is the cascade safe for audit/retention". This could be clearer as two distinct checklist items.
♻️ Optional: Split into two questions
| **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. | +| **Question** | Does this PR add or change an `on_delete` behavior on an FK? If yes, is the intent documented inline on the field? | +| **Question** | For any new/changed `on_delete` behavior, 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. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@design-rules/ai-review-checklist.md` around lines 34 - 36, Split the compound checklist line that currently combines two checks into two separate checklist items: keep one item that asks "Does this PR add or change an `on_delete` behavior on an FK? If yes, is the intent documented inline on the field?" and a second item that asks "If yes, is the cascade safe for audit/retention (P4)?" Update the markdown table rows so each question has its own row and ensure the wording matches the original phrasing (refer to the existing lines containing `on_delete` and "cascade safe for audit/retention (P4)") so reviewers can answer each independently..claude/skills/design-rules/SKILL.md (1)
133-137: Consider rewording for variety.Three consecutive sentences start with "If", which slightly reduces readability. Consider using varied sentence structures while maintaining the logical flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/design-rules/SKILL.md around lines 133 - 137, The three consecutive list entries (items "2.", "3.", and "4.") in SKILL.md start with "If", hurting readability; revise the wording to vary sentence openings while preserving meaning — e.g., change one "If" to "When" or start a sentence with an active verb like "Update" or "Ensure that", or merge clauses to avoid repetition; apply these edits directly to the numbered entries (2, 3, 4) in the principle list and keep the original intent and emphasis (do not add or remove rules), ensuring punctuation and emphasis (**bold**/`Status`) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/design-rules/SKILL.md:
- Around line 103-117: The fenced code block that begins with ``` containing the
ADR template (starting with "# ADR-NNN: <title>") lacks a language identifier;
update the opening fence from ``` to ```markdown so the ADR template is treated
as Markdown (look for the triple-backtick fence that wraps the ADR content / the
"# ADR-NNN: <title>" heading) and replace it accordingly.
In `@backend/account_v2/DESIGN_RULES.md`:
- Line 65: Replace the generic phrase in the table cell that reads "| **Enforced
by** | not yet enforced — see project issue tracker (credential lifecycle) |"
with a concrete tracker reference (e.g., an issue ID or ADR identifier and
optional short URL) so the enforcement debt is auditable; update the cell to
reference the specific identifier (for example "not yet enforced — see
ISSUE-1234" or "not yet enforced — see ADR-2026-02") and ensure the identifier
matches the actual issue/ADR in the repository or issue tracker.
- Line 22: Update the incorrect principle reference that maps "fail-closed" to
P8: find the table row containing "`principles.md` | P1 (org scoping), P2
(credentials), P8 (fail-closed)" and change P8 to P5, and also update the second
occurrence that maps fail-closed to P8 (the other table entry around the same
section) to P5 so both references point to the actual P5 definition in
principles.md.
In `@design-rules/per-component-contract.md`:
- Line 79: The document claims validate.sh checks rule-field/checklist literal
contents but the script currently only verifies field presence; either update
validate.sh to perform the deeper checks or change the wording in
design-rules/per-component-contract.md to reflect actual behavior. To fix:
either (A) extend the validate.sh script to parse each rule and verify that the
four rows "Severity", "Why", "Refs", and "Enforced by" contain valid non-empty
literals (add parsing logic and explicit error messages for missing/empty
values), or (B) modify the sentences that reference validate.sh (the lines
mentioning it validates rule-field/checklist literals) to state it only checks
presence of those fields; reference validate.sh and the rule text "Every rule
must have all four rows (Severity, Why, Refs, Enforced by)" when making the
change.
In `@design-rules/principles.md`:
- Line 16: The document uses inconsistent mixin names:
`DefaultOrganizationMixin` here vs `DefaultOrganizationManagerMixin` in the
tenant-isolation doc; pick one canonical symbol and update both locations to
match (e.g., rename `DefaultOrganizationMixin` to
`DefaultOrganizationManagerMixin` or vice versa), then search for and replace
all occurrences of the non-canonical symbol across the docs and examples
(including references inside `design-rules/principles.md` and
`design-rules/security/tenant-isolation.md`) and ensure the README, code
snippets, and any class/mixin declarations (e.g., class or function names that
implement the mixin) use the chosen name consistently.
In `@design-rules/security/tenant-isolation.md`:
- Around line 23-24: The docs use two inconsistent
names—DefaultOrganizationManagerMixin and DefaultOrganizationMixin—so pick one
canonical symbol (e.g., DefaultOrganizationManagerMixin) and update all docs to
match; specifically change the reference in design-rules/principles.md
(currently DefaultOrganizationMixin) to the chosen canonical name and make the
same substitution wherever the alternate occurs (table entry in
tenant-isolation.md and any other mentions) so all design-rules docs
consistently refer to the same class/mixin name.
In `@unstract/connectors/DESIGN_RULES.md`:
- Line 22: Update the incorrect principle ID for "fail-closed" in the
DESIGN_RULES.md table entries that currently list "P8"; change those occurrences
to "P5" so the row reading "`[principles.md](../../design-rules/principles.md) |
P2 (credentials), P8 (fail-closed) |`" becomes
"`[principles.md](../../design-rules/principles.md) | P2 (credentials), P5
(fail-closed) |`" (and likewise for the other identical occurrence).
---
Duplicate comments:
In `@design-rules/adr/README.md`:
- Around line 52-53: The README currently instructs "Use the format: Status /
Context / Decision / Consequences", which contradicts the repo rule that ADRs
have no Status field; edit the sentence to remove "Status" so it reads "Use the
format: Context / Decision / Consequences" and ensure any nearby example or line
that references "Status" is updated to match the no-Status rule (search for the
exact phrase "Use the format: Status / Context / Decision / Consequences" and
replace it with "Use the format: Context / Decision / Consequences" and update
any adjacent examples or references accordingly).
In `@design-rules/README.md`:
- Around line 52-53: Update the ADR format reference in the Step 2 list entry
that currently reads "Status / Context / Decision / Consequences" to match the
convention used in adr/README (remove Status) so it reads "Context / Decision /
Consequences"; locate the exact text string "Status / Context / Decision /
Consequences" in the design-rules README and replace it with "Context / Decision
/ Consequences" ensuring any surrounding explanatory text remains consistent.
---
Nitpick comments:
In @.claude/skills/design-rules/SKILL.md:
- Around line 133-137: The three consecutive list entries (items "2.", "3.", and
"4.") in SKILL.md start with "If", hurting readability; revise the wording to
vary sentence openings while preserving meaning — e.g., change one "If" to
"When" or start a sentence with an active verb like "Update" or "Ensure that",
or merge clauses to avoid repetition; apply these edits directly to the numbered
entries (2, 3, 4) in the principle list and keep the original intent and
emphasis (do not add or remove rules), ensuring punctuation and emphasis
(**bold**/`Status`) remain unchanged.
In `@design-rules/adr/ADR-007-adapter-access-runtime-validation.md`:
- Line 5: Change the phrase "at the moment a workflow is created" to "when a
workflow is created" in the sentence that currently reads "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."—update that exact sentence
in ADR-007-adapter-access-runtime-validation.md to use "when" for more concise
phrasing.
In `@design-rules/ai-review-checklist.md`:
- Around line 34-36: Split the compound checklist line that currently combines
two checks into two separate checklist items: keep one item that asks "Does this
PR add or change an `on_delete` behavior on an FK? If yes, is the intent
documented inline on the field?" and a second item that asks "If yes, is the
cascade safe for audit/retention (P4)?" Update the markdown table rows so each
question has its own row and ensure the wording matches the original phrasing
(refer to the existing lines containing `on_delete` and "cascade safe for
audit/retention (P4)") so reviewers can answer each independently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e0d312d-e6e8-4fb3-b412-3f3c26fdc02c
📒 Files selected for processing (25)
.claude/skills/design-rules/SKILL.md.claude/skills/design-rules/scripts/validate.sh.gitignorebackend/account_v2/DESIGN_RULES.mdbackend/workflow_manager/DESIGN_RULES.mddesign-rules/README.mddesign-rules/adr/ADR-001-org-scoping-query-layer.mddesign-rules/adr/ADR-002-no-org-fk-on-execution.mddesign-rules/adr/ADR-003-usage-string-refs.mddesign-rules/adr/ADR-005-prompt-studio-registry-publish-gate.mddesign-rules/adr/ADR-006-organization-rate-limit-separation.mddesign-rules/adr/ADR-007-adapter-access-runtime-validation.mddesign-rules/adr/ADR-012-connectorauth-user-owned.mddesign-rules/adr/ADR-014-internal-external-api-separation.mddesign-rules/adr/README.mddesign-rules/ai-review-checklist.mddesign-rules/definition-of-done.mddesign-rules/lifecycle.mddesign-rules/per-component-contract.mddesign-rules/principles.mddesign-rules/security/standards.mddesign-rules/security/tenant-isolation.mdunstract/connectors/DESIGN_RULES.mdunstract/connectors/src/unstract/connectors/databases/DESIGN_RULES.mdworkers/shared/DESIGN_RULES.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.claude/skills/design-rules/SKILL.md (1)
198-198: Prefer non-hardcoded ADR count in path reference.Line [198] hardcodes
8 accepted ADRs, which will go stale as soon as a new ADR lands. Consider using count-neutral wording.Suggested wording tweak
-| `design-rules/adr/ADR-*.md` | 8 accepted ADRs. | +| `design-rules/adr/ADR-*.md` | Accepted ADRs. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/design-rules/SKILL.md at line 198, Replace the hardcoded phrase "8 accepted ADRs" in the SKILL.md entry that references `design-rules/adr/ADR-*.md` with count-neutral wording (e.g., "accepted ADRs" or "multiple accepted ADRs") so the statement does not become stale; update the line containing the literal `8 accepted ADRs` to use the chosen neutral phrasing while keeping the path `design-rules/adr/ADR-*.md` intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@design-rules/per-component-contract.md`:
- Around line 172-175: The paragraph claiming that "## Known Exceptions" is
required conflicts with earlier statements that the section is optional; update
the text in design-rules/per-component-contract.md so the guidance is consistent
by stating that "## Known Exceptions" is optional and should only be included
when there are actual exceptions (leave "## Rules" and "## Checklist"
requirements as-is), and ensure the sentence references the section header "##
Known Exceptions" exactly to avoid ambiguity with lines that declare it
optional.
---
Nitpick comments:
In @.claude/skills/design-rules/SKILL.md:
- Line 198: Replace the hardcoded phrase "8 accepted ADRs" in the SKILL.md entry
that references `design-rules/adr/ADR-*.md` with count-neutral wording (e.g.,
"accepted ADRs" or "multiple accepted ADRs") so the statement does not become
stale; update the line containing the literal `8 accepted ADRs` to use the
chosen neutral phrasing while keeping the path `design-rules/adr/ADR-*.md`
intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8a4ebd7-dbc5-40e7-9f1b-82797da5f48f
📒 Files selected for processing (7)
.claude/skills/design-rules/SKILL.mdbackend/account_v2/DESIGN_RULES.mdbackend/workflow_manager/DESIGN_RULES.mddesign-rules/per-component-contract.mdunstract/connectors/DESIGN_RULES.mdunstract/connectors/src/unstract/connectors/databases/DESIGN_RULES.mdworkers/shared/DESIGN_RULES.md
✅ Files skipped from review due to trivial changes (4)
- backend/workflow_manager/DESIGN_RULES.md
- workers/shared/DESIGN_RULES.md
- unstract/connectors/DESIGN_RULES.md
- backend/account_v2/DESIGN_RULES.md
🚧 Files skipped from review as they are similar to previous changes (1)
- unstract/connectors/src/unstract/connectors/databases/DESIGN_RULES.md
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 3eda6e7 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 3eda6e7 (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>
|
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
design-rules/per-component-contract.md (1)
44-44: Optional: Add language specifiers to fenced code blocks.Fenced code blocks on lines 44 and 56 lack language identifiers. Adding
markdownwould enable syntax highlighting in some viewers.✨ Proposed style improvement
-``` +```markdown > **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.Apply the same pattern to line 56. </details> Also applies to: 56-56 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@design-rules/per-component-contract.mdat line 44, Update the two fenced
code blocks that currently start with triple backticks and contain the
"Compatibility:" note to include a language specifier by replacing the
opening fences withmarkdown; specifically modify the fenced block that wraps the "**Compatibility:** All changes..." paragraph (the block at the first occurrence) and the second identical fenced block (the one mentioned at line 56) so they begin withmarkdown to enable syntax highlighting.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@design-rules/per-component-contract.md:
- Line 44: Update the two fenced code blocks that currently start with triple
backticks and contain the "Compatibility:" note to include a language
specifier by replacing the opening fences withmarkdown; specifically modify the fenced block that wraps the "**Compatibility:** All changes..." paragraph (the block at the first occurrence) and the second identical fenced block (the one mentioned at line 56) so they begin withmarkdown to enable syntax
highlighting.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `a24af5ff-3537-43e5-8e9d-7aa12d9f4540` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 6924017630e9e4042a17c46b8b005e60bb36d7c8 and 2459e87051138eebafd4d70d92ebf003bd97f012. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `backend/account_v2/DESIGN_RULES.md` * `design-rules/per-component-contract.md` * `design-rules/principles.md` * `unstract/connectors/DESIGN_RULES.md` </details> <details> <summary>✅ Files skipped from review due to trivial changes (2)</summary> * backend/account_v2/DESIGN_RULES.md * unstract/connectors/DESIGN_RULES.md </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
design-rules/containing universal principles (P1–P9), an AI review checklist, a per-component contract that defines the canonical shape of per-componentDESIGN_RULES.mdfiles, a definition of done, a change lifecycle, security standards (Three-Layer Defense, SQL Safety Standard S1), and 8 accepted ADRs.design-rulesClaude skill at.claude/skills/design-rules/(un-ignored from.gitignorein this PR) exposinglist,get,add,update,review, andvalidateoperations over the ruleset, with avalidate.shscript that enforces the canonical shape.DESIGN_RULES.mdfiles in the canonical shape, each chosen to exercise a different dimension of the contract:backend/account_v2/— tenant-graph root.backend/workflow_manager/— anchor entity (P9) with execution-side children. R3 encodes audit-durability ofon_deletefor Workflow-rooted FKs (motivated by the recent dashboard regression where workflow deletion cascaded throughWorkflowExecution→ExecutionLogand broke recent-activity).unstract/connectors/— parent shared library; R5 delegates SQL rules to its child databases file.unstract/connectors/src/unstract/connectors/databases/— deeply-nested component (6../hops to the contract); specialises Security Standard S1 into R1–R6 for every supported DB engine.workers/shared/— worker context (no Django imports, no thread-local org); R5 encodes the UNS-205 HTTP session lifecycle with a real test path.validate.shwhere the Compatibility-blockquote grep pattern was looking for the literal substringCompatibility: Allwhich never matched the contract's bold form**Compatibility:** All. The pattern is nowAll changes to this component must remain compatible, which matches both forms.per-component-contract.mdis updated to reflect the new pattern.Why
DESIGN_RULES.mdfrom the file's directory plus every parent up to the repo root, and gets a deterministic list of applicable rules.How
design-rules/is added as a new top-level directory following the AGENTS.md convention.design-rules/per-component-contract.md) is the canonical schema: every per-component file follows a fixedScope/Read first/Rules/Known Exceptions/Checkliststructure, with a 4-rowSeverity/Why/Refs/Enforced bytable per rule, ~70-line target, 5–7 rules per component (hard ceiling 10).workflow_manager.R5andworkers/shared.R4both require JSON-only Celery serialization;connectors.R5delegates todatabases.R1–R6.Enforced byfield of every rule is honest — rules that have no automated check today saycode review onlyornot yet enforced — <reason>. No aspirational enforcement claims.Can this PR break any existing features. If yes, please list possible items. If no, please explain why.
No. This PR adds documentation files (and one Claude skill) only. No code paths, models, migrations, settings, runtime behaviour, or CI configuration are touched. The files are read by AI agents and human reviewers; nothing in production loads them. The only behavioural change is that
validate.shnow correctly validates the canonical Compatibility blockquote — it was previously a no-op for any file using the bold form, which is every file in this PR.Database Migrations
None.
Env Config
None.
Relevant Docs
Related Issues or PRs
Dependencies Versions
None.
Notes on Testing
This PR is documentation + a shell-script validator. Validation is done via the bundled
design-rulesClaude skill'svalidate.shscript:The script checks three things: a forbidden-word scan (
cloud,enterprise,hitl,subscription,agentic, vendor names, etc.), the verbatim Compatibility blockquote on every per-component file, and a directory-sanity check that each component dir contains real source. All three checks pass against this PR's files.Reviewers can spot-check by:
design-rules/per-component-contract.md(the deeply-nesteddatabases/file uses 6../hops; the others use 2).workflow_manager.R5↔workers/shared.R4both require JSON-only Celery serialization;connectors.R5delegates todatabases.R1–R6.Checklist
🤖 Generated with Claude Code