-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Identify cloud tasks in the extension bridge #8539
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
Conversation
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.
I found some issues that need attention.
| private readonly instanceId: string | ||
| private readonly appProperties: StaticAppProperties | ||
| private readonly gitProperties?: GitProperties | ||
| private readonly isCloudAgent?: boolean |
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.
[P0] Type mismatch can break build: isCloudAgent is declared as optional on the class (boolean | undefined) but passed to ExtensionChannel/TaskChannel which require a boolean. This allows undefined to flow into a non-optional parameter and will fail type-checking under strictNullChecks. Recommend aligning the class property with the options type: make it non-optional.
| private readonly isCloudAgent?: boolean | |
| private readonly isCloudAgent: boolean |
| this.socketBridgeUrl = options.socketBridgeUrl | ||
| this.token = options.token | ||
| this.provider = options.provider | ||
| this.instanceId = options.sessionId || crypto.randomUUID() |
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.
[P3] Minor cleanup: sessionId is now required in BridgeOrchestratorOptions, but the constructor still falls back to crypto.randomUUID(). Since callers must provide sessionId, the fallback is redundant. Consider simplifying to direct assignment for clarity.
| this.instanceId = options.sessionId || crypto.randomUUID() | |
| this.instanceId = options.sessionId |
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.
I found some issues that need attention.
| modes: z.array(z.object({ slug: z.string(), name: z.string() })).optional(), | ||
| providerProfile: z.string().optional(), | ||
| providerProfiles: z.array(z.object({ name: z.string(), provider: z.string().optional() })).optional(), | ||
| isCloudAgent: z.boolean().optional(), |
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.
[P2] Schema default improves robustness: if older clients omit this field, downstream code can always rely on a concrete boolean. Suggest defaulting to false so the property is present after parsing even when not sent.
| isCloudAgent: z.boolean().optional(), | |
| isCloudAgent: z.boolean().optional().default(false), |
| lastHeartbeat: Date.now(), | ||
| task: { taskId: "", taskStatus: TaskStatus.None }, | ||
| taskHistory: [], | ||
| isCloudAgent: this.isCloudAgent, |
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.
[P3] Durability: you set isCloudAgent on initial registration, but updateInstance relies on spreading the prior object to retain it. To make it future‑proof against refactors that rebuild the object, consider explicitly setting isCloudAgent: this.isCloudAgent inside updateInstance as well.
Important
Add
isCloudAgentflag to identify cloud tasks in the extension bridge system.isCloudAgentflag toCloudServiceto identify cloud tasks.isCloudAgenttotrueifStaticTokenAuthServiceis used inCloudService.isCloudAgenttoBaseChannelOptionsandBridgeOrchestratorOptions.isCloudAgenttoExtensionChannelandTaskChannelconstructors.isCloudAgenttoExtensionInstanceschema incloud.ts.ExtensionChannel.test.tsandTaskChannel.test.tsto includeisCloudAgentflag.This description was created by
for ffa5bb7. You can customize this summary. It will automatically update as commits are pushed.