-
Notifications
You must be signed in to change notification settings - Fork 595
fix: Update @github/copilot-sdk version and enhance version utility t… #840
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 all commits
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 |
|---|---|---|
|
|
@@ -113,3 +113,4 @@ data/ | |
| .planning/ | ||
| .mcp.json | ||
| .planning | ||
| .bg-shell/ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,33 +28,46 @@ const serverLogger = createLogger('Server'); | |
| export async function startServer(): Promise<void> { | ||
| const isDev = !app.isPackaged; | ||
|
|
||
| // Find Node.js executable (handles desktop launcher scenarios) | ||
| const nodeResult = findNodeExecutable({ | ||
| skipSearch: isDev, | ||
| logger: (msg: string) => logger.info(msg), | ||
| }); | ||
| const command = nodeResult.nodePath; | ||
| let command: string; | ||
| let commandSource: string; | ||
| let args: string[]; | ||
| let serverPath: string; | ||
|
|
||
| // Validate that the found Node executable actually exists | ||
| // systemPathExists is used because node-finder returns system paths | ||
| if (command !== 'node') { | ||
| let exists: boolean; | ||
| try { | ||
| exists = systemPathExists(command); | ||
| } catch (error) { | ||
| const originalError = error instanceof Error ? error.message : String(error); | ||
| throw new Error( | ||
| `Failed to verify Node.js executable at: ${command} (source: ${nodeResult.source}). Reason: ${originalError}` | ||
| ); | ||
| } | ||
| if (!exists) { | ||
| throw new Error(`Node.js executable not found at: ${command} (source: ${nodeResult.source})`); | ||
| if (isDev) { | ||
| // In development, run the TypeScript server via the user's Node.js. | ||
| const nodeResult = findNodeExecutable({ | ||
| skipSearch: true, | ||
| logger: (msg: string) => logger.info(msg), | ||
| }); | ||
| command = nodeResult.nodePath; | ||
| commandSource = nodeResult.source; | ||
|
|
||
| // Validate that the found Node executable actually exists | ||
| // systemPathExists is used because node-finder returns system paths | ||
| if (command !== 'node') { | ||
| let exists: boolean; | ||
| try { | ||
| exists = systemPathExists(command); | ||
| } catch (error) { | ||
| const originalError = error instanceof Error ? error.message : String(error); | ||
| throw new Error( | ||
| `Failed to verify Node.js executable at: ${command} (source: ${nodeResult.source}). Reason: ${originalError}` | ||
| ); | ||
| } | ||
| if (!exists) { | ||
| throw new Error( | ||
| `Node.js executable not found at: ${command} (source: ${nodeResult.source})` | ||
| ); | ||
| } | ||
| } | ||
|
Comment on lines
+36
to
62
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. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify that the call site disables discovery and that the helper returns the fallback literal.
rg -n -C2 'skipSearch:\s*true' apps/ui/src/electron/server/backend-server.ts
sed -n '275,300p' libs/platform/src/node-finder.tsRepository: AutoMaker-Org/automaker Length of output: 952 🏁 Script executed: # Check other call sites of findNodeExecutable to understand the pattern
rg -n 'findNodeExecutable' apps/ui/src/electron/server/backend-server.ts libs/platform/src/node-finder.ts --type tsRepository: AutoMaker-Org/automaker Length of output: 751 🏁 Script executed: # Check if there are comments or documentation explaining the skipSearch choice
rg -B5 -A5 'skipSearch.*true' apps/ui/src/electron/server/ --type tsRepository: AutoMaker-Org/automaker Length of output: 887 🏁 Script executed: # Verify the function signature and default behavior
rg -B3 -A10 'export function findNodeExecutable' libs/platform/src/node-finder.ts --type tsRepository: AutoMaker-Org/automaker Length of output: 477 🏁 Script executed: # Check the packaged mode branch to see the alternative approach
rg -B5 -A15 'if (isDev)' apps/ui/src/electron/server/backend-server.ts --type tsRepository: AutoMaker-Org/automaker Length of output: 49 🏁 Script executed: # Look for any environment setup or PATH configuration in the Electron context
rg -n 'NODE_PATH|process.env.PATH|ELECTRON' apps/ui/src/electron/ --type ts | head -20Repository: AutoMaker-Org/automaker Length of output: 574 🏁 Script executed: # Get the full context of the backend server startup including buildEnhancedPath usage
sed -n '1,170p' apps/ui/src/electron/server/backend-server.ts | tail -40Repository: AutoMaker-Org/automaker Length of output: 1614 🏁 Script executed: # Look for buildEnhancedPath implementation and documentation
rg -B3 -A15 'export.*buildEnhancedPath|function buildEnhancedPath' libs/platform/src/ --type tsRepository: AutoMaker-Org/automaker Length of output: 1418 Don't bypass With ♻️ Proposed fix const nodeResult = findNodeExecutable({
- skipSearch: true,
logger: (msg: string) => logger.info(msg),
});🤖 Prompt for AI Agents |
||
| } else { | ||
| // In packaged builds, use Electron's bundled Node runtime instead of a system Node. | ||
| // This makes the desktop app self-contained and avoids incompatibilities with whatever | ||
| // Node version the user happens to have installed globally. | ||
| command = process.execPath; | ||
| commandSource = 'electron'; | ||
| } | ||
|
|
||
| let args: string[]; | ||
| let serverPath: string; | ||
|
|
||
| // __dirname is apps/ui/dist-electron (Vite bundles all into single file) | ||
| if (isDev) { | ||
| serverPath = path.join(__dirname, '../../server/src/index.ts'); | ||
|
|
@@ -133,6 +146,8 @@ export async function startServer(): Promise<void> { | |
| PORT: state.serverPort.toString(), | ||
| DATA_DIR: dataDir, | ||
| NODE_PATH: serverNodeModules, | ||
| // Run packaged backend with Electron's embedded Node runtime. | ||
| ...(app.isPackaged && { ELECTRON_RUN_AS_NODE: '1' }), | ||
| // Pass API key to server for CSRF protection | ||
| AUTOMAKER_API_KEY: state.apiKey!, | ||
| // Only set ALLOWED_ROOT_DIRECTORY if explicitly provided in environment | ||
|
|
@@ -146,6 +161,7 @@ export async function startServer(): Promise<void> { | |
| logger.info('[DATA_DIR_SPAWN] env.DATA_DIR=', env.DATA_DIR); | ||
|
|
||
| logger.info('Starting backend server...'); | ||
| logger.info('Runtime command:', command, `(source: ${commandSource})`); | ||
| logger.info('Server path:', serverPath); | ||
| logger.info('Server root (cwd):', serverRoot); | ||
| logger.info('NODE_PATH:', serverNodeModules); | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
For better debuggability, it's recommended to preserve the original error's context (including stack trace) when re-throwing. The
causeproperty onErroris perfect for this (available in Node.js v16.9.0+). This change also provides an opportunity to use the newcommandSourcevariable for consistency instead ofnodeResult.source.