-
Notifications
You must be signed in to change notification settings - Fork 877
Feat: sandbox and sub-agents stability updates #253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
9f9cc7a
bf20035
fff7f0d
b0db701
9095c73
c806f9d
d94733a
e9041f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,7 @@ import { ScreenshotAnalysisOperation } from '../operations/ScreenshotAnalysis'; | |
| // Database schema imports removed - using zero-storage OAuth flow | ||
| import { BaseSandboxService } from '../../services/sandbox/BaseSandboxService'; | ||
| import { WebSocketMessageData, WebSocketMessageType } from '../../api/websocketTypes'; | ||
| import { InferenceContext, ModelConfig } from '../inferutils/config.types'; | ||
| import { InferenceContext } from '../inferutils/config.types'; | ||
| import { ModelConfigService } from '../../database/services/ModelConfigService'; | ||
| import { fixProjectIssues } from '../../services/code-fixer'; | ||
| import { GitVersionControl } from '../git'; | ||
|
|
@@ -355,22 +355,14 @@ export class SimpleCodeGeneratorAgent extends Agent<Env, CodeGenState> { | |
| // Load the latest user configs | ||
| const modelConfigService = new ModelConfigService(this.env); | ||
| const userConfigsRecord = await modelConfigService.getUserModelConfigs(this.state.inferenceContext.userId); | ||
|
|
||
| const userModelConfigs: Record<string, ModelConfig> = {}; | ||
| for (const [actionKey, mergedConfig] of Object.entries(userConfigsRecord)) { | ||
| if (mergedConfig.isUserOverride) { | ||
| const { isUserOverride, userConfigId, ...modelConfig } = mergedConfig; | ||
| userModelConfigs[actionKey] = modelConfig; | ||
| } | ||
| } | ||
| this.setState({ | ||
| ...this.state, | ||
| inferenceContext: { | ||
| ...this.state.inferenceContext, | ||
| userModelConfigs, | ||
| userModelConfigs: userConfigsRecord, | ||
| }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Medium Priority - Type Safety: The removed filtering logic appears to have served a purpose in extracting clean ModelConfig objects by removing metadata fields ( The new code directly assigns
Recommendation: Verify that the type definition for |
||
| }); | ||
| this.logger().info(`Agent ${this.getAgentId()} session: ${this.state.sessionId} onStart: User configs loaded successfully`, {userModelConfigs}); | ||
| this.logger().info(`Agent ${this.getAgentId()} session: ${this.state.sessionId} onStart: User configs loaded successfully`, {userConfigsRecord}); | ||
| } | ||
|
|
||
| private async gitInit() { | ||
|
|
@@ -1702,6 +1694,8 @@ export class SimpleCodeGeneratorAgent extends Agent<Env, CodeGenState> { | |
| } | ||
|
|
||
| const regenerated = await this.regenerateFile({ filePath: path, fileContents, filePurpose }, issues, 0); | ||
| // Invalidate cache | ||
| this.staticAnalysisCache = null; | ||
| // Persist to sandbox instance | ||
| await this.getSandboxServiceClient().writeFiles(sandboxInstanceId, [{ filePath: regenerated.filePath, fileContents: regenerated.fileContents }], `Deep debugger fix: ${path}`); | ||
| return { path, diff: regenerated.lastDiff }; | ||
|
|
@@ -1766,6 +1760,8 @@ export class SimpleCodeGeneratorAgent extends Agent<Env, CodeGenState> { | |
| fileCount: result.files.length | ||
| }); | ||
|
|
||
| await this.deployToSandbox(savedFiles, false); | ||
|
|
||
| return { files: savedFiles.map(f => { | ||
| return { | ||
| path: f.filePath, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -92,6 +92,7 @@ export abstract class BaseSandboxService { | |||||||||||||||||||
| language: t.language, | ||||||||||||||||||||
| frameworks: t.frameworks || [], | ||||||||||||||||||||
| description: t.description, | ||||||||||||||||||||
| disabled: t.disabled ?? false, | ||||||||||||||||||||
| projectType: t.projectType || 'app', | ||||||||||||||||||||
| renderMode: t.renderMode, | ||||||||||||||||||||
| slideDirectory: t.slideDirectory, | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical Bug: Missing The disabled: z.boolean(),However, this mapping on lines 90-98 does not include the Fix:
Suggested change
Note: The PR correctly adds |
||||||||||||||||||||
|
|
@@ -177,6 +178,7 @@ export abstract class BaseSandboxService { | |||||||||||||||||||
| selection: catalogInfo?.description.selection || '', | ||||||||||||||||||||
| usage: catalogInfo?.description.usage || '' | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| disabled: catalogInfo?.disabled ?? false, | ||||||||||||||||||||
| fileTree, | ||||||||||||||||||||
| allFiles: filesMap, | ||||||||||||||||||||
| language: catalogInfo?.language, | ||||||||||||||||||||
|
|
||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type Mismatch: UserModelConfigWithMetadata vs ModelConfig
The
getUserModelConfigs()method returnsRecord<AgentActionKey, UserModelConfigWithMetadata>, which includes extra metadata fields (isUserOverride,userConfigId).However,
InferenceContext.userModelConfigsis typed asRecord<AgentActionKey, ModelConfig>(seeworker/agents/inferutils/config.types.ts:359).The previous code (commit 7b90c82) explicitly filtered out these metadata fields:
Issue: While TypeScript will allow this assignment (since
UserModelConfigWithMetadata extends ModelConfig), downstream code may not expect these extra fields. The old code also only included configs whereisUserOverride === true, but the new code includes all configs.Recommendation: Either:
isUserOverridecheck was intentional, orInferenceContexttype to acceptUserModelConfigWithMetadataif the extra fields are acceptableClarification needed: Was the
isUserOverridefilter intentional, or was it meant to filter all configs regardless of override status?