Derive scope types and WHERE helpers from single source of truth#2159
Conversation
🦋 Changeset detectedLatest commit: a85b58a The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ecc1603 to
de35aad
Compare
|
✅ Rebased on latest main |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This PR introduces a well-designed single-source-of-truth pattern for hierarchical scope definitions. The implementation is clean, maintains backwards compatibility, and eliminates the prior three-way drift risk between schema columns, type definitions, and query helpers.
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None.
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
utility.ts:50ExportTenantScopeConfigfor completeness alongside other scope config re-exports
✅ APPROVE
Summary: This is an excellent refactor that consolidates three independent definitions (schema columns, TypeScript types, WHERE helpers) into a single source of truth. The type derivation is correct, the generic scopedWhere function is well-implemented, and all existing consumers continue to work unchanged. The named wrappers preserve grep-ability and ergonomics. The only minor observation is that TenantScopeConfig could be re-exported for consistency, but this is a nitpick since it wasn't exported before. Nice work! 🎉
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
scope-definitions.ts:25-27 |
ScopedTable uses any for column values |
Pragmatic tradeoff acknowledged by reviewer — accepts any Drizzle column type; constraints on function parameters provide sufficient safety |
scope-helpers.ts:21-22 |
Type assertions as Record<string, any> |
Standard TypeScript pattern for dynamic key iteration; assertions are safe because constraints guarantee keys exist |
utility.ts:44-50 |
Re-exported types lose explicit intersection semantics | Structural equivalence maintained; hierarchy now encoded in SCOPE_KEYS which is actually an improvement |
runtime-schema.ts:41-59 |
Parallel scope definitions in runtime schema | Pre-existing pattern; extending unification to runtime schema is outside PR scope and may have valid reasons for separation |
data-access/index.ts |
scope-helpers.ts not exported from barrel | Pre-existing architectural boundary; not introduced by this PR |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-architecture |
4 | 0 | 0 | 0 | 1 | 0 | 3 |
pr-review-types |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 7 | 0 | 0 | 0 | 1 | 0 | 6 |
Note: One inline comment from architecture reviewer was merged with types analysis (TenantScopeConfig re-export).
| AgentScopeConfig, | ||
| ProjectScopeConfig, | ||
| SubAgentScopeConfig, | ||
| } from '../db/manage/scope-definitions'; |
There was a problem hiding this comment.
💭 Consider: Export TenantScopeConfig for completeness
Issue: The new scope-definitions.ts exports TenantScopeConfig, but it's not re-exported here with the other scope config types.
Why: Minor asymmetry — TenantScopeConfig was only defined locally in scope-helpers.ts before, so this isn't a regression. However, consumers who need it must import from the deeper path while other scope configs come from this barrel.
Fix:
| } from '../db/manage/scope-definitions'; | |
| export type { | |
| AgentScopeConfig, | |
| ProjectScopeConfig, | |
| SubAgentScopeConfig, | |
| TenantScopeConfig, | |
| } from '../db/manage/scope-definitions'; |
Refs:
- scope-definitions.ts:35 —
TenantScopeConfigexport
Introduce scope-definitions.ts with SCOPE_KEYS as the canonical definition of which columns belong to each scope level. All downstream consumers now derive from this single source: - TypeScript scope config types (ProjectScopeConfig, AgentScopeConfig, etc.) are now computed from SCOPE_KEYS instead of manually defined - scope-helpers.ts uses a generic scopedWhere() that iterates over SCOPE_KEYS, eliminating per-level column enumeration - types/utility.ts re-exports the derived types to preserve existing import paths Adding or removing a scope column now requires changing only SCOPE_KEYS. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
de35aad to
a85b58a
Compare
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a re-review after rebase. The PR introduces a well-designed single-source-of-truth pattern for hierarchical scope definitions. The implementation consolidates three previously independent definitions (schema columns, TypeScript types, WHERE helpers) into SCOPE_KEYS, with everything else derived from it. No new issues were identified.
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None.
💭 Consider (0) 💭
None.
🕐 Pending Recommendations (1)
- 💭
utility.ts:50Re-exportTenantScopeConfigfor API completeness alongside other scope config types
✅ APPROVE
Summary: This is an excellent refactor that consolidates three independent definitions into a single source of truth. The type derivation is correct (SCOPE_KEYS → ScopeConfig<L> → named aliases), the generic scopedWhere() function is well-implemented with safe type assertions, and all existing consumers continue to work unchanged through re-exports. The prior Consider suggestion (re-exporting TenantScopeConfig) remains open but is a minor API completeness nitpick, not a blocking concern. Ship it! 🚀
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
runtime-schema.ts:41-59 |
Parallel scope definitions in runtime schema not consolidated | Pre-existing pattern outside PR scope; extending unification to runtime schema would significantly expand the PR. The manage schema consolidation is the intentional focus. |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-types |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-architecture |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
| Total | 1 | 0 | 0 | 0 | 0 | 0 | 1 |
Note: Architecture reviewer flagged runtime-schema parallel definitions as a potential future improvement; discarded as pre-existing and outside PR scope.
Summary
scope-definitions.tswithSCOPE_KEYSas the canonical definition of which columns belong to each scope level (tenant,project,agent,subAgent)ProjectScopeConfig,AgentScopeConfig,SubAgentScopeConfig) are now computed fromSCOPE_KEYSvia mapped types instead of manually definedscope-helpers.tsuses a genericscopedWhere(level, table, scopes)that iterates overSCOPE_KEYS, eliminating per-level column enumeration. Named wrappers (projectScopedWhere, etc.) are preserved for ergonomicstypes/utility.tsre-exports the derived types to preserve all existing import paths (zero changes needed in consumers)Before (3 independent definitions that can drift)
After (1 definition, everything else derived)
Applies on top of PR #2151 (scope-aware query helpers + isolation tests).
Test plan
🤖 Generated with Claude Code