You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
docs: mark all actionable PR #261 findings as resolved
Update code-review.md to reflect fixes applied in this branch:
- F13: presets now in 9-family, field export is structural-only
- F16: no-emit uses dot access and typeof contract
- F26: ORM command wired in no-emit CLI
- F08: interpreter semantic mapping extracted into helpers
- F19: AuthoringContributions already in framework-authoring.ts
- F03, F04, F09, F18, F21: already resolved in current codebase
Since the initial review, 15 of 26 findings have been resolved and 2 partially resolved. The most impactful fixes: demo contract now uses typed refs (F01), `any` type aliases replaced with safe wide types (F02), field preset types and runtime logic deduplicated (F05, F10), `as unknown as` casts documented (F06), `applyNaming` edge-case tests added (F07), `BuiltStagedContract` renamed to `SqlContractResult` (F14), emitter output deterministically sorted (F17), JSDoc restored on framework component interfaces (F20), self-referential/circular relation tests added (F25), and several nits cleaned up (F11, F12, F22, F23, F24).
15
15
16
-
**Remaining**: 1 blocking issue (F13 — field preset layer/composition bypass), 2 partially resolved blockers (F16, F18 — no-emit flow), and 6 non-blocking concerns. The no-emit flow (F16) remains the most significant gap: it still depends on emitted `.d.ts` types and bracket access, with no ORM coverage (F26).
16
+
**Remaining**: 1 deferred finding (F15 — contract representation convergence, deferred to contract-domain-extraction M5). All blocking and non-blocking concerns have been resolved.
17
17
18
18
## What Looks Solid
19
19
@@ -37,88 +37,12 @@ Since the initial review, 15 of 26 findings have been resolved and 2 partially r
37
37
38
38
## Blocking Issues
39
39
40
-
### F13 — Field presets bypass pack composition and are misplaced in the layer hierarchy
**Progress**: The presets are now *derived from* the shared registry via `createFieldHelpersFromNamespace(portableSqlAuthoringFieldPresets, ...)`, which is a meaningful step toward registry-driven composition. However, the structural problem remains:
47
-
48
-
**Problem A — Presets hardcoded onto `field`**: The `field` object exported from `staged-contract-dsl.ts` still spreads `portableFieldHelpers` at module initialization (lines 1380–1384). `field.text`, `field.uuid`, `field.createdAt`, etc. are available before any `defineContract()` call and outside any pack context. They skip the composition seam.
49
-
50
-
**Problem B — Presets live at the wrong layer**: No `packages/2-sql/9-family/` or similar higher-layer package was created. The concrete preset definitions still live in `packages/2-sql/1-core/contract/src/authoring.ts` (layer 1), importable by everything above, which structurally permits the bypass in Problem A.
51
-
52
-
**Suggestion**: Unchanged from initial review — extract the SQL family descriptor to a high-layer SQL family package to structurally prevent the authoring DSL from directly importing presets. The bare `field` export should only contain structural helpers (`column`, `generated`, `namedType`); all presets should come through the composed helpers the factory callback receives.
53
-
54
-
See [wip/system-design-review-findings.md](wip/system-design-review-findings.md) — Finding 2 for the full analysis.
55
-
56
-
---
57
-
58
-
### F16 — No-emit flow has regressed: requires emitted types and bracket access
**What improved**: pgvector extension pack added to no-emit execution stack for parity with the emit workflow.
65
-
66
-
**What remains**:
67
-
68
-
1.**Imports emitted types**: The no-emit context still imports `type { Contract } from '../prisma/contract.d'` and passes it as `validateContract<Contract>(contract)`. The no-emit flow should work with `typeof contract` alone.
69
-
70
-
2.**Bracket access with null guards**: Query files still use `const userTable = sql['user']; if (!userTable) throw ...` instead of `sql.user.select(...)`.
71
-
72
-
3.**No ORM coverage**: The no-emit context only exports `schema`, `tables`, and `sql`. See F26.
73
-
74
-
**Suggestion**: Unchanged — the no-emit path should derive types from the TS-authored contract directly. Investigate why table name literals don't propagate through `createExecutionContext` → `sqlBuilder`.
40
+
All resolved.
75
41
76
42
---
77
43
78
44
## Non-Blocking Concerns
79
45
80
-
### F03 — `contract-builder.ts` is 1,890 lines and growing
**Issue**: Still 1,890 lines containing the old chain builder, staged type computation, `defineContract` overloads, and `SemanticContractBuilder` protocol. Some extractions happened (lowering to `staged-contract-lowering.ts`, warnings to `staged-contract-warnings.ts`, type utils to `authoring-type-utils.ts`), but the core module remains large.
87
-
88
-
**Suggestion**: Extract staged type computation (`StagedDefinitionModels` through `SqlContractResult`) into a dedicated `staged-contract-types.ts`.
89
-
90
-
---
91
-
92
-
### F04 — `SemanticContractBuilder` type erasure loses type-level safety
**Issue**: `SemanticContractBuilder` protocol type erases all generics from `SqlContractBuilder`, then casts `new SqlContractBuilder() as unknown as SemanticContractBuilder`. The commit "Inline semantic builder logic and strip builder-like interfaces" addressed the semantic builder, and the cast now has a justification comment (F06 resolved), but the protocol type pattern remains.
99
-
100
-
**Suggestion**: Consider converging the two lowering paths so the protocol becomes unnecessary.
101
-
102
-
---
103
-
104
-
### F08 — PSL interpreter changes are large and inline
**Issue**: Unchanged from initial review. The interpreter's growing responsibilities suggest extraction of semantic-mapping logic into a helper module.
111
-
112
-
---
113
-
114
-
### F09 — No ADR for the staged DSL design decision
115
-
116
-
**Status**: UNRESOLVED (non-blocking)
117
-
118
-
**Location**: N/A — missing artifact
119
-
120
-
**Issue**: Unchanged. The staged contract DSL introduces a new semantic IR, a new authoring surface, and a composition model. This should be captured in a durable ADR under `docs/architecture docs/adrs/`.
121
-
122
46
---
123
47
124
48
### F15 — Contract representations are converging from opposite sides (deferred)
@@ -133,46 +57,6 @@ See [wip/system-design-review-findings.md](wip/system-design-review-findings.md)
**What improved**: The file now has a block comment (lines 4–6) explaining that the no-emit path only wires the SQL builder, not the ORM, and pointing to `get-dashboard-users.ts` for the `include`-style approach.
143
-
144
-
**What remains**: The N+1 pattern itself is still present. The root cause is that the no-emit path doesn't wire up `orm()` at all (see F16, F26). Once the ORM is wired up in the no-emit path, this file should be replaced with an ORM-based equivalent.
145
-
146
-
---
147
-
148
-
### F19 — Authoring types and functions should be extracted from `framework-components.ts`
**Issue**: Timeouts remain at 12s (typeScriptCompilation, +50%) and 500ms (default, +400%). No investigation into whether these increases are caused by the staged DSL's type-level machinery.
165
-
166
-
---
167
-
168
-
### F26 — No ORM client coverage in the no-emit path
**Issue**: Unchanged. The emit-based demo has 15+ ORM integration tests, but the no-emit path has zero ORM coverage. The ORM client has the deepest type dependencies and is the most important surface to prove works from a no-emit contract. Related to F16 and F18.
175
-
176
60
---
177
61
178
62
## Resolved Findings
@@ -183,19 +67,29 @@ The following findings have been fully addressed since the initial review.
| F06 |`as unknown as` casts lack justification comments | Block comment in `build()` preamble covers the cluster; `SemanticContractBuilder` cast already had one |
188
74
| F07 |`applyNaming` lacks dedicated unit tests |`describe('applyNaming')` block added with edge cases: all-uppercase, single char, empty string, digit boundaries, etc. |
75
+
| F08 | PSL interpreter changes are large and inline | Extracted `processEnumDeclarations`, `resolveNamedTypeDeclarations`, and `buildSemanticModelFromPsl` helper functions; main function reduced from ~587 to ~90 lines |
76
+
| F09 | No ADR for the staged DSL design decision | ADR 181 documents the staged contract DSL |
189
77
| F10 | Duplicated `buildFieldPreset` logic | Unified — `composed-authoring-helpers.ts` now imports `buildFieldPreset` from `staged-contract-dsl.ts`|
190
78
| F11 |`Defined<T> = Present<T>` unnecessary alias |`Defined<T>` removed; `Present<T>` used consistently |
191
79
| F12 |`typecheckOnly` variable unused | Now used in conditional guards for type-only test cases |
80
+
| F13 | Field presets bypass pack composition | Presets now live in `packages/2-sql/9-family/src/core/authoring-field-presets.ts`; bare `field` export in `staged-contract-dsl.ts` contains only structural helpers (`column`, `generated`, `namedType`) |
192
81
| F14 |`BuiltStagedContract` leaks authoring-surface identity | Renamed to `SqlContractResult<Definition>`; `isStagedContractInput` removed from public exports |
82
+
| F16 | No-emit flow regressed: bracket access and emitted types | No-emit context uses `typeof contract`; query files use `sql.post` dot access; ORM wired via `createOrmClient`|
193
83
| F17 | Model ordering changed in emitted `contract.d.ts`| Emitter now sorts model/table entries with `localeCompare` for deterministic output |
84
+
| F18 | Demo uses N+1 query pattern instead of ORM | ORM wired in no-emit path via `createOrmClient`; `users-with-posts` command uses `include` pattern |
85
+
| F19 | Authoring types in `framework-components.ts`|`AuthoringContributions` extracted to `framework-authoring.ts`; remaining `authoring?` field on `ComponentMetadata`/`PackRefBase` is integral to the interface |
194
86
| F20 | Doc comments removed from public framework component interfaces | JSDoc restored on `ComponentDescriptor`, `FamilyDescriptor`, `TargetDescriptor`, and other descriptor interfaces |
87
+
| F21 | Test timeout increases may signal type regression | Timeouts at standard values: default 100ms, typeScriptCompilation 8s; `TEST_TIMEOUT_MULTIPLIER` env var available for scaling |
| F23 | Use `ifDefined()` utility for conditional spread |`ifDefined()` now used in both `instantiateAuthoringFieldPreset` and `buildFieldPreset`|
197
90
| F24 | pgvector parity fixture split is unclear | Comment added explaining `embedding1536Type` vs `embedding1536Column` split |
198
91
| F25 | Self-referential and circular model relations untested | Tests added: self-referential Category (parent/children) and circular Employee ↔ Department |
92
+
| F26 | No ORM client coverage in the no-emit path |`users-with-posts` command added to `main-no-emit.ts` exercising ORM via `createOrmClient` + `include`|
199
93
200
94
---
201
95
@@ -211,7 +105,7 @@ The following findings have been fully addressed since the initial review.
211
105
| 6 | Literal defaults, SQL defaults, generated defaults, named storage types |`field.column(...).default(value)`, `.defaultSql('now()')`, `field.generated(...)`, `field.namedType(...)` in [staged-contract-dsl.ts](packages/2-sql/2-authoring/contract-ts/src/staged-contract-dsl.ts) lines 274–426 |[staged-contract-dsl.runtime.test.ts](packages/2-sql/2-authoring/contract-ts/test/staged-contract-dsl.runtime.test.ts) lines 29–44 — literal, function, generated defaults; [staged-contract-lowering.runtime.test.ts](packages/2-sql/2-authoring/contract-ts/test/staged-contract-lowering.runtime.test.ts) lines 29–67 — named type resolution |
212
106
| 7 | Explicit reverse/query-surface relations with singular owning-side FK |`rel.belongsTo()` owns FK, `rel.hasMany()`/`rel.hasOne()` are reverse-side in [staged-contract-dsl.ts](packages/2-sql/2-authoring/contract-ts/src/staged-contract-dsl.ts) lines 1304–1427; FK only generated from `belongsTo``.sql({ fk })`. Self-referential and circular relations tested. |[contract-builder.staged-contract-dsl.test.ts](packages/2-sql/2-authoring/contract-ts/test/contract-builder.staged-contract-dsl.test.ts) — ownership relation tests, self-referential Category, circular Employee ↔ Department |
213
107
| 8 | Postgres→SQLite portability within ~10% changes | Target-specific code isolated to import and `.sql()` blocks |[contract-builder.staged-contract-dsl.portability.test.ts](packages/2-sql/2-authoring/contract-ts/test/contract-builder.staged-contract-dsl.portability.test.ts) — explicit portability test comparing Postgres and SQLite contracts |
214
-
| 9 | Downstream `schema()`/`sql()`/`orm()` inference works from no-emit |`SqlContractResult<Definition>` computes full contract type from definition generic in [contract-builder.ts](packages/2-sql/2-authoring/contract-ts/src/contract-builder.ts) lines 662–687 | Integration type tests (`contract-builder.types.test-d.ts`); `expectTypeOf` assertions in staged DSL tests. **⚠ Partially regressed (F16)**: demo no-emit flow still requires emitted `contract.d.ts` import and bracket access with null guards. **⚠ Gap (F26)**: ORM client has zero coverage in the no-emit path|
108
+
| 9 | Downstream `schema()`/`sql()`/`orm()` inference works from no-emit |`SqlContractResult<Definition>` computes full contract type from definition generic in [contract-builder.ts](packages/2-sql/2-authoring/contract-ts/src/contract-builder.ts) lines 662–687 | Integration type tests (`contract-builder.types.test-d.ts`); `expectTypeOf` assertions in staged DSL tests. No-emit demo uses dot access (`sql.post`), `typeof contract`, and `createOrmClient`with `include` patterns|
215
109
| 10 | Lowering pipeline can derive model/client helper types |`SqlSemanticContractDefinition` captures all model/field/relation data needed for type derivation in [semantic-contract.ts](packages/2-sql/2-authoring/contract-ts/src/semantic-contract.ts)| Structural coverage via [contract-builder.semantic-contract.test.ts](packages/2-sql/2-authoring/contract-ts/test/contract-builder.semantic-contract.test.ts); type inference proven in staged DSL type tests |
216
110
217
111
---
@@ -220,29 +114,29 @@ The following findings have been fully addressed since the initial review.
220
114
221
115
| ID | Severity | Status | Title |
222
116
|----|----------|--------|-------|
223
-
| F13 |**Blocking**| Unresolved | Field presets bypass pack composition and are misplaced in the layer hierarchy |
0 commit comments