fix(microsoft-foundry): handle stdout/stderr stream errors in az login#101598
fix(microsoft-foundry): handle stdout/stderr stream errors in az login#101598Alix-007 wants to merge 21 commits into
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 12, 2026, 11:40 PM ET / July 13, 2026, 03:40 UTC. Summary PR surface: Source +97, Tests +474, Generated 0, Other +2. Total +573 across 7 files. Reproducibility: yes. at the source and real-child-process level: current main has piped streams without error listeners, and a forced error on a real child pipe exercises the failure contract. A kernel-originated Azure CLI failure and the native Windows descendant case have not been reproduced. Review metrics: 2 noteworthy metrics.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Expose the smallest maintainer-approved generic process-tree lifecycle seam, document its POSIX and Windows contract, use it from Microsoft Foundry, and prove both normal login completion and stream-error teardown on native Linux and Windows before merge. Do we have a high-confidence way to reproduce the issue? Yes at the source and real-child-process level: current main has piped streams without error listeners, and a forced error on a real child pipe exercises the failure contract. A kernel-originated Azure CLI failure and the native Windows descendant case have not been reproduced. Is this the best way to solve the issue? Unclear. Process-tree teardown is the right behavioral boundary, but maintainers must choose whether three low-level public exports or one higher-level lifecycle helper is the narrowest maintainable SDK contract. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 156f4c89ed03. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +97, Tests +474, Generated 0, Other +2. Total +573 across 7 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (9 earlier review cycles; latest 8 shown)
|
|
@Alix-007 Same pattern, parallel coverage: I just opened two more stream-error PRs in the same campaign wave:
Fix shape mirrors your matrix deps #101597. Verified no open PR for those two files. Happy to coordinate if you're tracking the campaign. |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
…ndry-az-stream-errors # Conflicts: # docs/.generated/plugin-sdk-api-baseline.sha256 # scripts/plugin-sdk-surface-report.mjs
…eam-errors # Conflicts: # docs/.generated/plugin-sdk-api-baseline.sha256
|
Closing this PR because the current branch conflicts with main and the remaining work is no longer a bounded repair of the original stream-error bug. The current shape also makes low-level process-tree helpers part of the public Plugin SDK contract without an approved long-term API, and the Windows descendant lifecycle has not been proven on a native Windows host. Red CI alone is not the reason for closure. A focused replacement would be welcome after maintainers choose the lifecycle owner and API shape. It should keep the public seam as narrow as possible, include native Windows descendant-shutdown proof, and validate the resulting exact head against current main. |
What Problem This Solves
Interactive Microsoft Foundry onboarding uses
az login --use-device-code. Before this PR, stdout/stderr stream errors on that child process could surface as unhandled stream failures or settle the login promise before the child process lifecycle was actually closed. On Windows, killing only the shell wrapper can also leave the shell-launched Azure CLI descendant alive.Why This Change Was Made
errorhandlers for interactiveaz login --use-device-codeopenclaw/plugin-sdk/process-runtimeprocess-tree helpers instead of direct child-onlykill()callsaz loginwith the SDK process-tree detach policy, so POSIX runs can signal the child process group and Windows shell launches can use the shared taskkill-backed process-tree patherrorevents idempotent: the first stream error wins and teardown runs onceazsubstitute process over real stdio pipes, and an SDK contract test for Windows taskkill routingUser Impact
Users who hit a broken pipe or wrapper stream error during Microsoft Foundry
az loginshould get one bounded login failure instead of a hung/stale login process, duplicate teardown, or a left-behind Windows Azure CLI descendant.Evidence
git diff --check— passed after commit9f51c9e2bfnode --import tsx scripts/check-no-extension-test-core-imports.ts— passed after commit9f51c9e2bf; extension tests avoid direct core test/internal importsnode scripts/report-test-temp-creations.mjs --staged --json --fail-on-findings— passed before commit9f51c9e2bf; output[]node scripts/run-vitest.mjs extensions/microsoft-foundry/cli.process.test.ts test/extension-test-boundary.test.ts— passed after commit9f51c9e2bf; boundary shard 11 passed, microsoft-foundry shard 2 passed / 1 Windows-only skippednode scripts/run-vitest.mjs src/plugin-sdk/process-runtime.test.ts extensions/microsoft-foundry/cli.test.ts extensions/microsoft-foundry/cli.process.test.ts test/scripts/plugin-sdk-surface-report.test.ts— passed after commitd4d62c6fa6; tooling shard 10 passed, plugin-sdk shard 1 passed, microsoft-foundry shard 6 passed / 1 Windows-only skippedd4d62c6fa6updated plugin SDK surface budgets for the three public process-tree helper exports;9f51c9e2bfkeeps the extension process proof on publicopenclaw/plugin-sdk/test-envtemp-dir helpers instead of repo-only test helper bridges or bare temp-dir exceptionsReal behavior proof
Behavior addressed: Microsoft Foundry
az login --use-device-codestdout/stderr stream errors are contained without an unhandled stream error, duplicate teardown, or settlement before the login process closes. The latest patch additionally prevents the stream-error cleanup from stopping only the Windows shell wrapper while leaving the shell-launchedazdescendant alive.Real environment tested: local Linux Node source checkout. The proof uses a PATH-injected local
azsubstitute executable — a real spawned process with real stdio pipes, not the Azure CLI and not an in-process mocked child.Exact focused commands run after the latest patches:
Observed result after
9f51c9e2bf: extension boundary script passed for 2404 extension files and 1 plugin helper; temp-dir report output[]; boundary shard 11 passed; microsoft-foundry shard 2 passed / 1 Windows-only skipped. Observed result afterd4d62c6fa6: tooling shard 10 passed, plugin-sdk shard 1 passed, microsoft-foundry shard 6 passed / 1 Windows-only skipped.The Linux real-process tests spy on
process.killand verifySIGTERM/SIGKILLare sent to the spawned process group (-childPid) through the SDK process-tree helper before the promise rejects. The SDK contract test mocksprocess.platform === "win32"and verifiesopenclaw/plugin-sdk/process-runtimeroutessignalChildProcessTree/forceKillChildProcessTreetotaskkill /T /PID <pid>andtaskkill /F /T /PID <pid>instead of callingchild.kill().Windows-specific committed proof:
extensions/microsoft-foundry/cli.process.test.tsnow contains a Windows-only real process test that launches a.cmdshell wrapper, records the descendant fake-az PID, forces a stdout stream error, and asserts the descendant exits before rejection. That test is intentionallydescribe.skipIf(process.platform !== "win32"); it was not run in this local Linux proof.What was not tested: live Azure CLI/device-code authentication, a kernel-originated stdout read error, and the Windows-only
.cmddescendant test on a native Windows host. Node offers no reliable local Linux way to force a kernel read error onchild.stdout, so the runnable proof drives the real exported function with a real child process and forces the Node stream error on that real pipe.Collision check: no open PR found for the Microsoft Foundry az login stream-error issue or
extensions/microsoft-foundry/cli.ts.