Feat: sandbox and sub-agents stability updates #253
Conversation
github-actions
bot
added
dependencies
|
Claude encountered an error —— View job Code Review in Progress
|
Add null checks after inference calls in blueprint, template selection, phase generation, user conversation processing, project setup, and code fixer operations. Switch from REACT_RENDER_LOOP_PREVENTION to REACT_RENDER_LOOP_PREVENTION_LITE in code debugger and phase generation. Add explicit warning against installing cloudflare:workers/durable-objects dependencies in multiple prompts. Update phase generation to prioritize runtime
AshishKumar4
force-pushed
the
feat/stability
branch
from
744b687 to
bf20035
Compare
|
Claude encountered an error —— View job Code Quality & Security ReviewRecommendation: APPROVE with one minor suggestion Code QualityMedium Priority Issues:
Positive Changes:
SecurityNo vulnerabilities found This PR focuses on stability improvements in the agent/sandbox layer. All changes are internal to the worker runtime:
SummaryThis is a solid stability improvement PR that addresses real production issues. The null-check guards are essential for surfacing LLM inference failures clearly rather than failing silently. The cache invalidation and deployment consistency fixes directly address reported issues #216 and #246. The only concern is the simplified userModelConfigs assignment which may need type verification, but this is a minor issue that can be validated in testing. Approval recommendation: The changes improve error handling, fix known bugs, and maintain code quality standards. The type safety concern is worth checking but doesn't block approval. Tasks:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| disabled: t.disabled ?? false, | ||
| projectType: t.projectType || 'app', | ||
| renderMode: t.renderMode, | ||
| slideDirectory: t.slideDirectory, |
There was a problem hiding this comment.
Critical Bug: Missing disabled field in template mapping
The TemplateInfoSchema in sandboxTypes.ts (line 116) requires a disabled field:
disabled: z.boolean(),However, this mapping on lines 90-98 does not include the disabled field. This will cause Zod validation failures when the response is validated against TemplateInfoSchema.
Fix:
| slideDirectory: t.slideDirectory, | |
| name: t.name, | |
| language: t.language, | |
| frameworks: t.frameworks || [], | |
| description: t.description, | |
| disabled: t.disabled ?? false, | |
| projectType: t.projectType || 'app', | |
| renderMode: t.renderMode, | |
| slideDirectory: t.slideDirectory, |
Note: The PR correctly adds disabled to line 95 in the diff (in the template details section), but this section at line 90-98 was missed.
| ...this.state.inferenceContext, | ||
| userModelConfigs, | ||
| userModelConfigs: userConfigsRecord, | ||
| }, |
There was a problem hiding this comment.
Medium Priority - Type Safety: The removed filtering logic appears to have served a purpose in extracting clean ModelConfig objects by removing metadata fields (isUserOverride, userConfigId).
The new code directly assigns userConfigsRecord (which contains these extra fields) to userModelConfigs. This could cause issues if:
- The
InferenceContext.userModelConfigstype expectsRecord<string, ModelConfig>without metadata fields - Downstream code doesn't expect these additional fields
Recommendation: Verify that the type definition for InferenceContext.userModelConfigs accepts the full record structure returned by getUserModelConfigs(), or restore the filtering logic to maintain type safety.
| inferenceContext: { | ||
| ...this.state.inferenceContext, | ||
| userModelConfigs, | ||
| userModelConfigs: userConfigsRecord, |
There was a problem hiding this comment.
Type Mismatch: UserModelConfigWithMetadata vs ModelConfig
The getUserModelConfigs() method returns Record<AgentActionKey, UserModelConfigWithMetadata>, which includes extra metadata fields (isUserOverride, userConfigId).
However, InferenceContext.userModelConfigs is typed as Record<AgentActionKey, ModelConfig> (see worker/agents/inferutils/config.types.ts:359).
The previous code (commit 7b90c82) explicitly filtered out these metadata fields:
const userModelConfigs: Record<string, ModelConfig> = {};
for (const [actionKey, mergedConfig] of Object.entries(userConfigsRecord)) {
if (mergedConfig.isUserOverride) {
const { isUserOverride, userConfigId, ...modelConfig } = mergedConfig;
userModelConfigs[actionKey] = modelConfig;
}
}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 where isUserOverride === true, but the new code includes all configs.
Recommendation: Either:
- Keep the old filtering logic if the
isUserOverridecheck was intentional, or - Update the
InferenceContexttype to acceptUserModelConfigWithMetadataif the extra fields are acceptable
Clarification needed: Was the isUserOverride filter intentional, or was it meant to filter all configs regardless of override status?
1 participant
Summary
This PR improves stability of the sandbox integration and sub-agent operations by upgrading the sandbox SDK version, adding null-check guards for LLM inference results, and refining agent prompts for better code generation quality.
Changes
Sandbox Updates
disabledfield to TemplateInfo and TemplateDetails schemas to support template filteringdisabledfield population in template listing and loadingAgent Stability Improvements
Code Generation Quality
Motivation
This PR addresses several stability issues encountered in production:
Null inference results: Multiple operations could fail silently when LLM inference returned null (due to rate limits, network issues, or provider errors). Added explicit null checks with error messages to surface these failures clearly.
Sandbox compatibility: Upgraded to sandbox SDK 0.5.2 to leverage latest stability fixes and features.
Template management: Added
disabledfield to prevent broken or experimental templates from being selected by users.Cache consistency: Deep debugger fixes weren't invalidating static analysis cache, causing stale error reports.
Deployment consistency: Files generated by debugger weren't being deployed to sandbox immediately, requiring manual refresh.
Testing
To verify these changes:
Related Issues
This PR addresses stability concerns raised in:
This PR description was automatically generated by Claude Code