fix(types): validate persisted session rows - #1712
Conversation
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
There was a problem hiding this comment.
The schemas remove unsafe assertions, but the new fallback policy is not behavior-preserving: malformed persisted state is repeatedly converted into ordinary absence. In these paths, absence controls alarm delivery, participant creation, and duplicate-PR protection, so corruption now triggers unsafe control flow rather than a visible integrity failure. Please make invalid-row handling explicit per boundary: preserve fail-closed authentication, distinguish invalid identity rows from missing rows, and do not silently erase authoritative alarm or artifact state.
I found no file-size regression; no changed file approaches 1,000 lines. The blockers are the storage-boundary semantics, not the use of Zod itself.
The authenticated GitHub App is also the PR author, so this is submitted as COMMENT; the findings should nevertheless be treated as request-changes blockers.
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
ColeMurray
left a comment
There was a problem hiding this comment.
Reviewed exact head 376f684, all changed files, migrated SQLite contracts, consumers, and all discussion. All three original review concerns are genuinely addressed: corrupt alarm and artifact rows raise integrity errors; participant identity reads do likewise while token authentication fails closed. Nullable valid data remains supported. Main synchronization preserves current scheduler behavior. No remaining blockers; exact-head CI is green, with intentional Terraform deployment skips. Shared build and changed-file lint/format checks also passed.
This is an automated nightly unsafe-cast remediation sweep. It replaces three selected unsafe assertions of persisted session data with package-local Zod row schemas and
safeParseat the session SQLite read boundaries, following the TypeScript Coding Standards guidance for unsafe-cast / parse-don't-assert and the Zod boundary-validation pattern established in PR #807.packages/control-plane/src/session/artifact-repository.tsresult.toArray() as ArtifactRow[]artifactRowSchemavalidates artifact reads. Missing single rows returnnull; malformed existing rows throwSessionStorageIntegrityError, preserving duplicate-PR protection and authoritative artifact discovery. Nullableurlandmetadataremain valid.packages/control-plane/src/session/participant-repository.tsresult.toArray() as ParticipantRow[]in identity, ID, list, and WebSocket token readsparticipantRowSchemavalidates participant rows. Identity/ID/list reads throwSessionStorageIntegrityErrorfor malformed rows instead of treating corruption as permission to create a duplicate participant. The token-auth lookup intentionally returnsnullfor missing or invalid rows to fail closed. Nullable identity/token columns remain valid.packages/control-plane/src/session/alarm/scheduler.ts.toArray() as AlarmStateRow[]cancelledconstrained to `0The follow-up integrity-error change addresses all three original review findings. The branch has also been updated with current main, preserving its alarm scheduling and Node SQLite adapter changes.
Original implementation verification:
npm run build -w @open-inspect/sharednpm run build -w @open-inspect/control-planenpm run typechecknpm run lintnpm run formatnpm test -w @open-inspect/control-planegit diff --checkCurrent-head verification is recorded in this PR's checks. Regression coverage includes malformed artifact/participant rows, nullable persisted fields, malformed alarm state, and invalid cancellation values.
References: TypeScript Coding Standards unsafe-cast / parse-don't-assert guidance, and the Zod boundary-validation pattern established in PR #807.
Created with Open-Inspect