-
Notifications
You must be signed in to change notification settings - Fork 55
Surface launch diagnostics to SDK callers (e.g. copilot.exe) #353
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
96773fd
83308e5
b64f28c
e8ba1aa
9eaf9e1
aad99d4
aeef514
ecd5acb
ff774ea
01b6e13
7922f6f
4f1ec10
194dd95
f64a9e4
1cf59e0
1e618a7
4b93b63
fe48bd3
8f8ee7b
2aedab8
11ee69b
b46faef
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 |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import { parse as semverParse } from 'semver'; | |
| import { SandboxPolicy, ContainerConfig, ContainmentType, ContainmentBackend } from './types.js'; | ||
| import { prepareSpawn, diagLogVersion } from './helper.js'; | ||
| import { diagLog } from './diagnostic.js'; | ||
| import { MxcError, mxcErrorFromCode } from './errors.js'; | ||
|
|
||
| const SUPPORTED_VERSION = '0.6.0-alpha'; | ||
| const MIN_VERSION = '0.4.0-alpha'; | ||
|
|
@@ -697,6 +698,15 @@ export function spawnSandboxAsync( | |
| ptyProcess.onExit((event: { exitCode: number; signal?: number }) => { | ||
| // Note: wxc-exec doesn't separate stdout/stderr when using PTY | ||
| // All output is combined | ||
| // | ||
| // Check for structured error envelopes from wxc-exec on failure. | ||
| if (event.exitCode !== 0) { | ||
| const mxcError = tryParseErrorEnvelopeFromLines(output); | ||
| if (mxcError) { | ||
| reject(mxcError); | ||
| return; | ||
| } | ||
| } | ||
| resolve({ | ||
| stdout: output, | ||
| stderr: '', | ||
|
|
@@ -708,3 +718,27 @@ export function spawnSandboxAsync( | |
| } | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Scans a multi-line string for a JSON error envelope emitted by wxc-exec | ||
| * on stderr. Returns the first matching envelope, or null if none found. | ||
| * The envelope format is: `{"error": {"code": "...", "message": "...", ...}}` | ||
| */ | ||
| function tryParseErrorEnvelopeFromLines(output: string): MxcError | null { | ||
|
Collaborator
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. suggestion: can this be moved out of
Member
Author
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. wontfix. because no time. |
||
| for (const line of output.split('\n')) { | ||
| const trimmed = line.trim(); | ||
| if (!trimmed.startsWith('{')) continue; | ||
| try { | ||
| const parsed = JSON.parse(trimmed); | ||
| if (parsed && typeof parsed === 'object' && 'error' in parsed) { | ||
| const env = parsed.error; | ||
| if (env && typeof env.code === 'string' && typeof env.message === 'string') { | ||
| return mxcErrorFromCode(env.code, env.message, env.details); | ||
| } | ||
| } | ||
| } catch { | ||
| // Not valid JSON on this line, continue scanning. | ||
| } | ||
| } | ||
|
Comment on lines
+723
to
+742
Comment on lines
+722
to
+742
|
||
| return null; | ||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -834,6 +834,7 @@ impl WSLContainerRunner { | |
| } else { | ||
| String::new() | ||
| }, | ||
| ..Default::default() | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
issue: don't we also need this in these places?
mxc/sdk/src/sandbox.ts
Line 416 in f1b455b
mxc/sdk/src/sandbox.ts
Line 526 in f1b455b
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 dont think you can because PTY
Uh oh!
There was an error while loading. Please reload this page.
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.
Hmm but there is a
ptyProcessreturn inspawnWithConfigthough. Users Can usespawnSandboxby itself rather thanspawnSandboxAsync. Same thing withspawnSandboxFromConfigthere is aptyProcessoption in there too.EDIT: Oh wait, I might have misinterpreted what you said. I thought you meant, that we can't because they use PTY