feat(factory-sdk): W1 scaffold + ports/config/constants#229
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughThis PR introduces a new ChangesFactory SDK Package
🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the @pear/factory-sdk package, establishing core configuration schemas, ports, and testing fakes for the Factory service. The feedback focuses on improving the robustness and isolation of the SDK: updating the Zod schema for stateIds to support partial overrides, stripping comments in the import-checking test to prevent false positives, deep-cloning files in FakeMountClient to avoid test pollution, and explicitly declaring external dependencies in package.json.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| stateIds: z.object({ | ||
| readyForAgent: z.string(), | ||
| agentImplementing: z.string(), | ||
| done: z.string(), | ||
| inPlanning: z.string(), | ||
| }).default(LINEAR_STATE_IDS), |
There was a problem hiding this comment.
Using .default(LINEAR_STATE_IDS) on the outer stateIds object means that if a user provides a partial stateIds object (e.g., only overriding readyForAgent), Zod validation will fail because the other fields are required and do not have individual defaults.
To allow partial overrides seamlessly, we should define defaults on the individual fields of the nested object and default the outer object to {}.
| stateIds: z.object({ | |
| readyForAgent: z.string(), | |
| agentImplementing: z.string(), | |
| done: z.string(), | |
| inPlanning: z.string(), | |
| }).default(LINEAR_STATE_IDS), | |
| stateIds: z.object({ | |
| readyForAgent: z.string().default(LINEAR_STATE_IDS.readyForAgent), | |
| agentImplementing: z.string().default(LINEAR_STATE_IDS.agentImplementing), | |
| done: z.string().default(LINEAR_STATE_IDS.done), | |
| inPlanning: z.string().default(LINEAR_STATE_IDS.inPlanning), | |
| }).default({}), |
| async readFile(path: string): Promise<{ content: unknown; revision?: string }> { | ||
| const entry = this.files.get(path) | ||
| if (!entry) { | ||
| throw new Error(`File not found: ${path}`) | ||
| } | ||
|
|
||
| return { ...entry } | ||
| } | ||
|
|
||
| async writeFile(path: string, content: unknown): Promise<void> { | ||
| const revision = String((Number(this.files.get(path)?.revision ?? 0) || 0) + 1) | ||
| this.files.set(path, { content, revision }) | ||
| this.writes.push({ path, content }) | ||
| } |
There was a problem hiding this comment.
In FakeMountClient, returning or storing object references directly can lead to accidental state mutation and test pollution. If a test or the system under test mutates the object returned by readFile or passed to writeFile, it will unexpectedly modify the internal state of the fake.
Using structuredClone to deep-clone the content on both read and write operations ensures that the fake's state remains isolated and consistent.
async readFile(path: string): Promise<{ content: unknown; revision?: string }> {
const entry = this.files.get(path)
if (!entry) {
throw new Error(`File not found: ${path}`)
}
const content = entry.content && typeof entry.content === 'object'
? structuredClone(entry.content)
: entry.content
return { content, revision: entry.revision }
}
async writeFile(path: string, content: unknown): Promise<void> {
const revision = String((Number(this.files.get(path)?.revision ?? 0) || 0) + 1)
const clonedContent = content && typeof content === 'object'
? structuredClone(content)
: content
this.files.set(path, { content: clonedContent, revision })
this.writes.push({ path, content: clonedContent })
}| for (const file of await walk(sourceRoot)) { | ||
| const source = await readFile(file, 'utf8') | ||
| for (const match of source.matchAll(importSpecifierPattern)) { |
There was a problem hiding this comment.
The current import specifier regex matches any import/export statements in the file, including those inside single-line or multi-line comments. This can lead to false positives if a developer comments out an import or writes a comment containing an import-like string.
Stripping comments from the source code before running the regex match will make the test much more robust.
| for (const file of await walk(sourceRoot)) { | |
| const source = await readFile(file, 'utf8') | |
| for (const match of source.matchAll(importSpecifierPattern)) { | |
| for (const file of await walk(sourceRoot)) { | |
| const rawSource = await readFile(file, 'utf8') | |
| const source = rawSource.replace(/\/\*[\s\S]*?\*\//g, '').replace(/\/\/.*/g, '') | |
| for (const match of source.matchAll(importSpecifierPattern)) { |
| "private": true, | ||
| "type": "commonjs", | ||
| "exports": { |
There was a problem hiding this comment.
The package @pear/factory-sdk imports external packages like zod, @relayfile/sdk, and @agent-relay/harness-driver but does not declare them in its package.json dependencies or peerDependencies.
Even in a monorepo, explicitly declaring dependencies is a best practice to ensure correct dependency resolution, prevent phantom dependency issues, and support strict package managers (like pnpm or Yarn PnP).
"private": true,
"type": "commonjs",
"dependencies": {
"@relayfile/sdk": "workspace:*",
"zod": "^3.22.0"
},
"peerDependencies": {
"@agent-relay/harness-driver": "workspace:*"
},
"exports": {
|
Implemented the PR-scoped fixes for the confirmed review feedback. Addressed comments
VerificationPassed locally:
I could not reproduce the macOS-only |
1 participant
Summary
@pear/factory-sdkas a source-only package underpackages/factory-sdk.FleetClientsurface.FakeMountClient,FakeFleetClient, and the emptyRelayFleetClientseam stub for the future../relaySDK implementation.Verification
npx vitest run packages/factory-sdknpx tsc --noEmit -p tsconfig.node.jsonGuardrails
src/mainedits.npm install; worktree uses the provisionednode_modulessymlink.