refactor(agents-core): enforce data-access layer boundary for auth queries#2196
refactor(agents-core): enforce data-access layer boundary for auth queries#2196amikofalvy wants to merge 1 commit intomainfrom
Conversation
…eries Extract 3 inline Drizzle queries from auth/auth.ts into data-access/runtime/auth.ts and add a lint-staged script that prevents new drizzle-orm imports outside allowed directories (data-access/, db/, dolt/, __tests__/). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Medium
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
scripts/lint-data-access-boundary.sh:13-17Allowlist missingauth/andvalidation/directories with legitimate drizzle-orm imports
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
packages/agents-core/src/data-access/index.ts:29-31Comment placement inconsistent with established pattern
💭 Consider (3) 💭
💭 1) scripts/lint-data-access-boundary.sh CI enforcement gap
Issue: The lint script only runs via lint-staged on pre-commit hooks, not in CI. Developers can bypass with --no-verify, and PRs merged via GitHub UI skip hooks entirely.
Why: The existing createProtectedRoute enforcement uses Biome's noRestrictedImports rule (biome.jsonc lines 58-71), which runs in CI via pnpm check and cannot be bypassed. This creates an inconsistent enforcement model.
Fix: Consider adding the script to CI (run against all agents-core/src/**/*.ts files in .github/workflows/ci.yml), or migrate to Biome's noRestrictedImports rule for consistency. Not blocking since pre-commit enforcement is a reasonable starting point.
💭 2) scripts/lint-data-access-boundary.sh:25 Shell array check could be quoted
Issue: if [ ${#VIOLATIONS[@]} -gt 0 ] could be if [ "${#VIOLATIONS[@]}" -gt 0 ] for strictness consistency.
Why: Extremely minor — bash handles this correctly, but the script uses set -euo pipefail for strictness.
💭 3) scripts/lint-data-access-boundary.sh:1 Consider adding SPDX license header
Issue: Other scripts in the repository (e.g., setup-optional.sh) include an SPDX license identifier.
Why: Style consistency across scripts directory.
🚫 REQUEST CHANGES
Summary: The data-access extraction is clean and follows established curried patterns well. The lint-staged boundary enforcement is a solid architectural improvement. However, the allowlist needs to include auth/*-schema* and validation/drizzle-* patterns to avoid blocking legitimate schema definition files. Once that's fixed, this is ready to merge.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
runtime/auth.ts |
JSDoc documentation missing | Inconsistent pattern in codebase — apiKeys.ts has no JSDoc, users.ts does. 50/50 convention. |
runtime/auth.ts:5 |
Function naming could be getInitialMembershipForUser |
Intentional naming matching original code intent; naming is clear. |
runtime/auth.ts |
Curried pattern conformance | Positive observation, not an issue. |
package.json:125-127 |
lint-staged entry well-structured | Positive observation, not an issue. |
lint-data-access-boundary.sh |
Allowlist drift concern | Overlaps with the MAJOR finding about missing auth/validation directories. |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-architecture |
3 | 0 | 1 | 0 | 0 | 0 | 2 |
pr-review-devops |
5 | 0 | 2 | 0 | 2 | 0 | 1 |
pr-review-consistency |
4 | 0 | 0 | 0 | 0 | 0 | 4 |
| Total | 12 | 0 | 3 | 0 | 2 | 0 | 7 |
Note: Some findings merged across reviewers (e.g., comment ordering flagged by both devops and consistency).
| case "$file" in | ||
| */data-access/* | */db/* | */dolt/* | */__tests__/* | *.test.ts | *.spec.ts | */test-*) | ||
| continue | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
🟠 MAJOR Allowlist missing auth/ and validation/ directories
Issue: The allowlist doesn't include auth/ or validation/ directories, but existing files in these directories have legitimate drizzle-orm imports for schema definitions and type helpers. When any staged change touches these files, commits will be blocked.
Why:
packages/agents-core/src/auth/auth-schema.tsimportsrelationsfromdrizzle-orm(line 1)packages/agents-core/src/validation/drizzle-schema-helpers.tsimports fromdrizzle-orm/sqlite-core(line 2)
These are schema/type definition files, not query logic that should be encapsulated in data-access.
Fix: (1-click apply)
| case "$file" in | |
| */data-access/* | */db/* | */dolt/* | */__tests__/* | *.test.ts | *.spec.ts | */test-*) | |
| continue | |
| ;; | |
| esac | |
| case "$file" in | |
| */data-access/* | */db/* | */dolt/* | */__tests__/* | *.test.ts | *.spec.ts | */test-* | */auth/*-schema* | */validation/drizzle-*) | |
| continue | |
| ;; | |
| esac |
Refs:
| export * from './runtime/apiKeys'; | ||
| // Runtime data access (Postgres - not versioned) | ||
| export * from './runtime/auth'; |
There was a problem hiding this comment.
🟡 Minor Comment placement inconsistent with established pattern
Issue: The section comment // Runtime data access (Postgres - not versioned) was moved to between apiKeys and auth exports, but the established pattern places section comments before the first export of that section.
Why: Minor readability impact — section comments demarcate where a group starts.
Fix: (1-click apply)
| export * from './runtime/apiKeys'; | |
| // Runtime data access (Postgres - not versioned) | |
| export * from './runtime/auth'; | |
| // Runtime data access (Postgres - not versioned) | |
| export * from './runtime/apiKeys'; | |
| export * from './runtime/auth'; |
Refs:
1 participant
Summary
auth/auth.tsinto newdata-access/runtime/auth.ts(curried pattern matching existing conventions)scripts/lint-data-access-boundary.shlint-staged rule that prevents newdrizzle-ormimports outside allowed directories (data-access/,db/,dolt/,__tests__/, test files)data-access/index.tsTest plan
pnpm typecheckpasses (agents-core)pnpm lintpasses (285 files, no warnings)pnpm testpasses (96 files, 1512 tests)knippasses (no unused exports)from 'drizzle-orm'→ exit 1data-access/,db/,dolt/,__tests__/,*.test.ts,*.spec.ts,test-*filesagents-corefiles (confirmed during commit)🤖 Generated with Claude Code