feat(finalizer): add Effect domain and schema validation - #23
Conversation
Review SummaryThis PR introduces Effect-based domain modeling and validation for the finalizer, including branded identifiers, tagged errors, manifest/finalization-plan schemas, and pure media helpers. It also preserves the existing public API through compatibility exports and adds focused tests, but duplicated manifest and part contracts create a maintainability risk and can drift from the canonical schemas. Key Changes:
Confidence Score: 5/5Recommendation: Review findings before merge. Rationale: The changed files and supplied review finding clearly show duplicated domain contracts between media.ts and schema.ts, while the overall validation flow and compatibility structure are straightforward to assess. Special Attention- Consolidate FinalizerPart and FinalizerManifest in media.ts with the canonical definitions from schema.ts. - Verify checksum typing remains consistent across media helpers and schema validation. - Confirm compatibility exports continue exposing the intended public types and functions.Important Files Changed
Flowchartflowchart TD
A[Unknown manifest input] --> B[Effect Schema decode]
B -->|Invalid shape or branded fields| C[InvalidManifest]
B -->|Valid schema| D[Validate segment and part ordering]
D -->|Invalid or incomplete| E[IncompleteManifest or InvalidPart]
D --> F[Check total input size]
F -->|Over limit| G[InputTooLarge]
F --> H[Validated FinalizerManifest]
H --> I[Decode finalization plan]
I -->|Mismatch| J[InvalidFinalizePlan]
I --> K[Validated FinalizePlan]
Audit history |
| } | ||
|
|
||
| export function validateManifest(manifest: unknown): number { | ||
| const decoded = Effect.runSync(decodeManifest(manifest)); |
There was a problem hiding this comment.
🟡 Warning
Effect.runSync wraps failures in FiberFailure, so processFinalization will not see the underlying InvalidManifest/InvalidFinalizePlan _tag; malformed manifests are therefore treated as transient, requeued, and retried indefinitely instead of being marked failed. Unwrap the Cause via an Exit-based runner or otherwise rethrow the tagged domain error directly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/finalizer/src/domain/validate.ts
Line: 102
Severity: warning
Comment: `Effect.runSync` wraps failures in `FiberFailure`, so `processFinalization` will not see the underlying `InvalidManifest`/`InvalidFinalizePlan` `_tag`; malformed manifests are therefore treated as transient, requeued, and retried indefinitely instead of being marked failed. Unwrap the `Cause` via an Exit-based runner or otherwise rethrow the tagged domain error directly.
How can I resolve this? If you propose a fix, please make it concise.
Think this is incorrect? Reply false positive: <reason>.
| objectKey: string; | ||
| sequence: number; | ||
| } | ||
| export interface FinalizerManifest { |
There was a problem hiding this comment.
🔵 Suggestion
This redeclares FinalizerManifest/FinalizerPart alongside the new canonical Effect schemas in schema.ts, allowing the runtime decoder and media helpers to drift independently. Reuse the schema-derived types here (and similarly consolidate overlapping output types) so the validated boundary remains the single source of truth.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/finalizer/src/domain/media.ts
Line: 11
Severity: suggestion
Comment: This redeclares `FinalizerManifest`/`FinalizerPart` alongside the new canonical Effect schemas in `schema.ts`, allowing the runtime decoder and media helpers to drift independently. Reuse the schema-derived types here (and similarly consolidate overlapping output types) so the validated boundary remains the single source of truth.
How can I resolve this? If you propose a fix, please make it concise.
Think this is incorrect? Reply false positive: <reason>.
5a98f5b to
91e53cf
Compare
91e53cf to
ffbf61e
Compare
There was a problem hiding this comment.
🔴 Critical
The container health probe is configured for container/health, but server.js only serves /health. Cloudflare will receive a 404 health check, mark the container unhealthy, and finalization requests will fail. Set pingEndpoint to /health (or add the matching server route).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/finalizer/src/worker.ts
Line: 65
Severity: critical
Comment: The container health probe is configured for `container/health`, but `server.js` only serves `/health`. Cloudflare will receive a 404 health check, mark the container unhealthy, and finalization requests will fail. Set `pingEndpoint` to `/health` (or add the matching server route).
How can I resolve this? If you propose a fix, please make it concise.
Think this is incorrect? Reply false positive: <reason>.
pnpm 11 fails frozen installs when @effect/platform-node pulls @parcel/watcher and msgpackr-extract without allowBuilds entries.
Domain Schema types made the existing fixtures incomplete. Narrow segment/part access, return Promise<Response> from the fetch mock, and fill claim with leaseExpiresAt plus the required manifest fields.
| export const MAX_SEGMENTS = 5; | ||
| export const MAX_INPUT_BYTES = 2 * 1024 * 1024 * 1024; | ||
|
|
||
| export interface FinalizerPart { |
There was a problem hiding this comment.
🟡 Warning
FinalizerPart and FinalizerManifest are duplicated here even though domain/schema.ts defines the canonical versions. These contracts can drift silently (the media helpers already use a different checksum type); import the shared type from schema.ts or consolidate the model so all helpers use one definition.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/finalizer/src/domain/media.ts
Line: 4
Severity: warning
Comment: `FinalizerPart` and `FinalizerManifest` are duplicated here even though `domain/schema.ts` defines the canonical versions. These contracts can drift silently (the media helpers already use a different checksum type); import the shared type from `schema.ts` or consolidate the model so all helpers use one definition.
How can I resolve this? If you propose a fix, please make it concise.
Think this is incorrect? Reply false positive: <reason>.
Summary
Validation
pnpm --filter @interview-web/finalizer testpnpm --filter @interview-web/finalizer check-types