refactor(shared): decompose shared type contracts - #945
Merged
Conversation
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
📝 WalkthroughWalkthroughThe shared package separates its monolithic type barrel into domain-specific schema and type modules, preserves compatibility re-exports, updates internal imports, and adds public API, module-boundary, type-contract, and dedicated test-compilation checks. ChangesShared types decomposition
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/shared/src/types/statuses.ts (1)
13-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive duplicated types from their Zod schemas via
z.infer.
SessionStatuscorrectly usesz.infer<typeof sessionStatusSchema>, butSandboxStatus,GitSyncStatus,ArtifactType, andSpawnSourceare manually written unions that duplicate the values in their corresponding Zod enums (sandboxStatusSchema,gitSyncStatusSchema,artifactTypeSchema,spawnSourceSchema). If a value is added to one but not the other, they will silently drift. Deriving the types from the schemas eliminates this risk.♻️ Proposed refactor
-export type SandboxStatus = - | "pending" - | "spawning" - | "connecting" - | "warming" - | "syncing" - | "ready" - | "running" - | "stale" - | "snapshotting" - | "stopped" - | "failed"; export type GitSyncStatus = "pending" | "in_progress" | "completed" | "failed"; export type MessageStatus = "pending" | "processing" | "completed" | "failed"; export type MessageSource = "web" | "slack" | "linear" | "extension" | "github" | "automation"; -export type ArtifactType = "pr" | "screenshot" | "video" | "preview" | "branch"; export type EventType = | "heartbeat" | "ready" | "token" | "tool_call" | "step_start" | "step_finish" | "tool_result" | "git_sync" | "error" | "execution_complete" | "artifact" | "push_complete" | "push_error" | "warning" | "user_message"; export type ParticipantRole = "owner" | "member"; -export type SpawnSource = - | "user" - | "agent" - | "automation" - | "github-bot" - | "linear-bot" - | "slack-bot"; export type ConfidenceLevel = "high" | "medium" | "low"; export const sandboxStatusSchema = z.enum([ "pending", "spawning", "connecting", "warming", "syncing", "ready", "running", "stale", "snapshotting", "stopped", "failed", ]); +export type SandboxStatus = z.infer<typeof sandboxStatusSchema>; + export const gitSyncStatusSchema = z.enum(["pending", "in_progress", "completed", "failed"]); +export type GitSyncStatus = z.infer<typeof gitSyncStatusSchema>; + export const artifactTypeSchema = z.enum(["pr", "screenshot", "video", "preview", "branch"]); +export type ArtifactType = z.infer<typeof artifactTypeSchema>; + export const spawnSourceSchema = z.enum([ "user", "agent", "automation", "github-bot", "linear-bot", "slack-bot", ]); +export type SpawnSource = z.infer<typeof spawnSourceSchema>;Also applies to: 55-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/types/statuses.ts` around lines 13 - 24, Replace the manually defined SandboxStatus, GitSyncStatus, ArtifactType, and SpawnSource unions with z.infer types referencing sandboxStatusSchema, gitSyncStatusSchema, artifactTypeSchema, and spawnSourceSchema respectively, matching the existing SessionStatus pattern; ensure the schemas are declared or imported before their inferred types.packages/shared/src/types/session-api.ts (1)
129-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared refinements to avoid duplication.
The three
.refine()calls are duplicated identically betweencreateSessionRequestSchema(lines 129-141) andcreateSessionInputSchema(lines 145-178). If a validation rule changes, both must be updated in sync. Extracting the refinements into a shared array or helper would eliminate this risk.♻️ Proposed refactor
+const sessionTargetRefinements = [ + { fn: hasMatchingRepositoryIdentifiers, opts: { message: "repoOwner and repoName must be provided together", path: ["repoName"] } }, + { fn: hasRepositoryForBranch, opts: { message: "branch requires repoOwner and repoName", path: ["branch"] } }, + { fn: hasExclusiveSessionTarget, opts: { message: "environmentId, repositories, and repoOwner/repoName/branch are mutually exclusive", path: ["repositories"] } }, +] as const; + +function withSessionTargetRefinements<S extends z.ZodObject>(schema: S) { + return sessionTargetRefinements.reduce( + (s, { fn, opts }) => s.refine(fn, opts), + schema as z.ZodTypeAny + ); +} -export const createSessionRequestSchema = createSessionRequestBaseSchema - .refine(hasMatchingRepositoryIdentifiers, { - message: "repoOwner and repoName must be provided together", - path: ["repoName"], - }) - .refine(hasRepositoryForBranch, { - message: "branch requires repoOwner and repoName", - path: ["branch"], - }) - .refine(hasExclusiveSessionTarget, { - message: "environmentId, repositories, and repoOwner/repoName/branch are mutually exclusive", - path: ["repositories"], - }); +export const createSessionRequestSchema = withSessionTargetRefinements(createSessionRequestBaseSchema);Then apply the same to
createSessionInputSchema:-export const createSessionInputSchema = createSessionRequestBaseSchema - .extend({ - userId: z.string().optional(), - spawnSource: spawnSourceSchema.optional(), - authProvider: z.enum(["github", "google"]).optional(), - authUserId: z.string().optional(), - authEmail: z.string().optional(), - authName: z.string().optional(), - authAvatarUrl: z.string().optional(), - scmUserId: z.string().optional(), - scmLogin: z.string().optional(), - scmName: z.string().optional(), - scmEmail: z.string().optional(), - scmAvatarUrl: z.string().optional(), - actorUserId: z.string().optional(), - actorDisplayName: z.string().optional(), - actorEmail: z.string().optional(), - actorAvatarUrl: z.string().optional(), - scmToken: z.string().optional(), - scmRefreshToken: z.string().optional(), - scmTokenExpiresAt: z.number().optional(), - }) - .refine(hasMatchingRepositoryIdentifiers, { - message: "repoOwner and repoName must be provided together", - path: ["repoName"], - }) - .refine(hasRepositoryForBranch, { - message: "branch requires repoOwner and repoName", - path: ["branch"], - }) - .refine(hasExclusiveSessionTarget, { - message: "environmentId, repositories, and repoOwner/repoName/branch are mutually exclusive", - path: ["repositories"], - }); +export const createSessionInputSchema = withSessionTargetRefinements( + createSessionRequestBaseSchema.extend({ + userId: z.string().optional(), + spawnSource: spawnSourceSchema.optional(), + authProvider: z.enum(["github", "google"]).optional(), + authUserId: z.string().optional(), + authEmail: z.string().optional(), + authName: z.string().optional(), + authAvatarUrl: z.string().optional(), + scmUserId: z.string().optional(), + scmLogin: z.string().optional(), + scmName: z.string().optional(), + scmEmail: z.string().optional(), + scmAvatarUrl: z.string().optional(), + actorUserId: z.string().optional(), + actorDisplayName: z.string().optional(), + actorEmail: z.string().optional(), + actorAvatarUrl: z.string().optional(), + scmToken: z.string().optional(), + scmRefreshToken: z.string().optional(), + scmTokenExpiresAt: z.number().optional(), + }) +);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/types/session-api.ts` around lines 129 - 178, Extract the duplicated refinements used by createSessionRequestSchema and createSessionInputSchema into a shared helper or reusable refinement array containing hasMatchingRepositoryIdentifiers, hasRepositoryForBranch, and hasExclusiveSessionTarget with their existing messages and paths. Apply that shared validation to both schemas while preserving their current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/shared/src/types/session-api.ts`:
- Around line 129-178: Extract the duplicated refinements used by
createSessionRequestSchema and createSessionInputSchema into a shared helper or
reusable refinement array containing hasMatchingRepositoryIdentifiers,
hasRepositoryForBranch, and hasExclusiveSessionTarget with their existing
messages and paths. Apply that shared validation to both schemas while
preserving their current behavior.
In `@packages/shared/src/types/statuses.ts`:
- Around line 13-24: Replace the manually defined SandboxStatus, GitSyncStatus,
ArtifactType, and SpawnSource unions with z.infer types referencing
sandboxStatusSchema, gitSyncStatusSchema, artifactTypeSchema, and
spawnSourceSchema respectively, matching the existing SessionStatus pattern;
ensure the schemas are declared or imported before their inferred types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16aac12c-f04f-44e9-b663-02368154282c
📥 Commits
Reviewing files that changed from the base of the PR and between b4f43a9 and 4cd8fc2d3a0b413ddaf42ea01dcf1d4b86fe0eec.
⛔ Files ignored due to path filters (1)
packages/shared/src/__snapshots__/public-api.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (23)
docs/shared-types-decomposition-plan.mdpackages/shared/package.jsonpackages/shared/src/completion/extractor.tspackages/shared/src/logger.tspackages/shared/src/module-boundaries.test.tspackages/shared/src/public-api.test.tspackages/shared/src/triggers/testing.tspackages/shared/src/triggers/types.tspackages/shared/src/types/analytics.tspackages/shared/src/types/artifacts.tspackages/shared/src/types/automation-primitives.tspackages/shared/src/types/automations.tspackages/shared/src/types/environments.tspackages/shared/src/types/index.tspackages/shared/src/types/repositories.tspackages/shared/src/types/repository-catalog.tspackages/shared/src/types/sandbox-events.tspackages/shared/src/types/server-messages.tspackages/shared/src/types/session-api.tspackages/shared/src/types/sessions.tspackages/shared/src/types/statuses.tspackages/shared/src/types/type-contracts.test.tspackages/shared/tsconfig.test.json
ColeMurray
force-pushed
the
refactor/shared-types-decomposition
branch
from
July 10, 2026 00:16
4cd8fc2 to
9cea116
Compare
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
This was referenced Jul 11, 2026
This was referenced Jul 30, 2026
This was referenced Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
types/index.tsas a 165-line compatibility-only barrel with the existing package-root API unchangedWhy
The multi-repository sessions rollout expanded
packages/shared/src/types/index.tsinto a runtime-heavy protocol module whose Zod schemas, DTOs, normalizers, and domain contracts had several unrelated owners. The environment contracts could not be cleanly extracted without first establishing a dependency leaf for canonical repository-list schemas; importing those values back through the barrel would risk ESM initialization cycles.This refactor makes the ownership and dependency direction explicit without changing consumer imports, wire behavior, schema optionality, normalization, or legacy compatibility.
Compatibility and quality guardrails
RepositoryPairValidationErrorconstructor used byinstanceoftypes/index.ts, the package root, or@open-inspect/sharedThe thermo-nuclear code-quality review found two guardrail defects during iteration: the module-boundary test was initially omitted from strict test typechecking, and bare
@open-inspect/sharedself-imports were not rejected. Both were fixed and re-reviewed. The final thermo verdict approved the full diff with no remaining findings.After review feedback, the local implementation-plan document and generated Vitest
.snapartifact were removed from the PR. The compatibility suite keeps direct type-contract and runtime identity assertions without introducing a new snapshot-file convention.AutomationTriggerTypealso moved intoautomations.tsinstead of living in a standalone primitive module.Validation
npm run build -w @open-inspect/sharednpm run typechecknpm test -w @open-inspect/shared— 372 testsnpm test -w @open-inspect/control-plane— 1,717 testsnpm run test:integration -w @open-inspect/control-plane— 525 testsnpm test -w @open-inspect/web— 620 testsnpm test -w @open-inspect/github-bot— 123 testsnpm test -w @open-inspect/slack-bot— 227 testsnpm test -w @open-inspect/linear-bot— 136 testsnpm run lint -w @open-inspect/sharedgit diff --checkSummary by CodeRabbit