Skip to content

fix(microsoft-foundry): handle stdout/stderr stream errors in az login#101598

Closed
Alix-007 wants to merge 21 commits into
openclaw:mainfrom
Alix-007:alix/foundry-az-stream-errors
Closed

fix(microsoft-foundry): handle stdout/stderr stream errors in az login#101598
Alix-007 wants to merge 21 commits into
openclaw:mainfrom
Alix-007:alix/foundry-az-stream-errors

Conversation

@Alix-007

@Alix-007 Alix-007 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

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

  • add stdout/stderr error handlers for interactive az login --use-device-code
  • move the stream-error shutdown path onto the public openclaw/plugin-sdk/process-runtime process-tree helpers instead of direct child-only kill() calls
  • spawn az login with 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 path
  • keep duplicate stream errors and late child error events idempotent: the first stream error wins and teardown runs once
  • cover the lifecycle with mocked-timer regressions, a real spawned local az substitute process over real stdio pipes, and an SDK contract test for Windows taskkill routing

User Impact

Users who hit a broken pipe or wrapper stream error during Microsoft Foundry az login should 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 commit 9f51c9e2bf
  • node --import tsx scripts/check-no-extension-test-core-imports.ts — passed after commit 9f51c9e2bf; extension tests avoid direct core test/internal imports
  • node scripts/report-test-temp-creations.mjs --staged --json --fail-on-findings — passed before commit 9f51c9e2bf; output []
  • node scripts/run-vitest.mjs extensions/microsoft-foundry/cli.process.test.ts test/extension-test-boundary.test.ts — passed after commit 9f51c9e2bf; boundary shard 11 passed, microsoft-foundry shard 2 passed / 1 Windows-only skipped
  • node 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 commit d4d62c6fa6; tooling shard 10 passed, plugin-sdk shard 1 passed, microsoft-foundry shard 6 passed / 1 Windows-only skipped
  • Earlier mutation check: forcing the stream-error path to reject before child close made both real-process tests fail on the close-before-rejection assertions; reverting the mutation made the focused tests pass
  • CI fixes addressed: d4d62c6fa6 updated plugin SDK surface budgets for the three public process-tree helper exports; 9f51c9e2bf keeps the extension process proof on public openclaw/plugin-sdk/test-env temp-dir helpers instead of repo-only test helper bridges or bare temp-dir exceptions

Real behavior proof

Behavior addressed: Microsoft Foundry az login --use-device-code stdout/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-launched az descendant alive.

Real environment tested: local Linux Node source checkout. The proof uses a PATH-injected local az substitute 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:

node --import tsx scripts/check-no-extension-test-core-imports.ts
node scripts/report-test-temp-creations.mjs --staged --json --fail-on-findings
node scripts/run-vitest.mjs extensions/microsoft-foundry/cli.process.test.ts test/extension-test-boundary.test.ts
node 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

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 after d4d62c6fa6: 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.kill and verify SIGTERM / SIGKILL are sent to the spawned process group (-childPid) through the SDK process-tree helper before the promise rejects. The SDK contract test mocks process.platform === "win32" and verifies openclaw/plugin-sdk/process-runtime routes signalChildProcessTree / forceKillChildProcessTree to taskkill /T /PID <pid> and taskkill /F /T /PID <pid> instead of calling child.kill().

Windows-specific committed proof: extensions/microsoft-foundry/cli.process.test.ts now contains a Windows-only real process test that launches a .cmd shell wrapper, records the descendant fake-az PID, forces a stdout stream error, and asserts the descendant exits before rejection. That test is intentionally describe.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 .cmd descendant test on a native Windows host. Node offers no reliable local Linux way to force a kernel read error on child.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.

@clawsweeper

clawsweeper Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 12, 2026, 11:40 PM ET / July 13, 2026, 03:40 UTC.

Summary
The PR contains Microsoft Foundry Azure CLI stream failures, terminates the spawned process tree before settling, exports three process-tree helpers through the Plugin SDK, and adds mocked and real-process regressions.

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.

  • Public SDK surface: 3 function exports added. These functions become compatibility-sensitive third-party API rather than remaining an internal implementation detail.
  • Native platform proof: 1 platform run, 1 platform unrun. Linux real-process behavior is demonstrated, while the Windows descendant lifecycle central to the latest patch remains unexecuted.

Stored data model
Persistent data-model change detected: serialized state: extensions/microsoft-foundry/cli.process.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦐 gold shrimp
Patch quality: 🦐 gold shrimp
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Obtain maintainer approval and document the permanent Plugin SDK process-tree contract.
  • Run the committed Windows descendant test on a native Windows host and attach redacted output.
  • Rebase onto current main, resolve the generated baseline conflict, and rerun the failed guard checks.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The Linux real-child proof is useful, but the new Windows shell-descendant behavior has not run on native Windows; add a redacted Windows terminal transcript or recording, then update the PR body to trigger review or ask a maintainer for @clawsweeper re-review.

Risk before merge

  • [P1] Merging would make three internal process-tree primitives part of the supported third-party Plugin SDK contract without documenting their semantics, platform behavior, or compatibility expectations.
  • [P1] The PR changes normal POSIX launch behavior by spawning device-code login detached and relies on Windows taskkill for shell descendants, but the native Windows lifecycle path has not been executed.
  • [P1] The current head conflicts with main and has two failing guard checks, so the reviewed patch is not yet the exact merge result.

Maintainer options:

  1. Finish the public contract before merge (recommended)
    Obtain maintainer approval for the SDK shape, document the three exports, rebase the branch, and attach native Windows descendant-shutdown proof.
  2. Narrow the exported seam
    Replace the primitive exports with one documented lifecycle helper so plugins cannot assemble inconsistent detach and termination policies.
  3. Pause the SDK expansion
    Close or split the PR if maintainers do not want to support process-tree termination as a public Plugin SDK contract.

Next step before merge

  • [P1] A human must approve the permanent public SDK shape and require native Windows lifecycle evidence; this is not a safe automated repair or merge decision.

Maintainer decision needed

  • Question: Should the three low-level process-tree helpers become a permanent public openclaw/plugin-sdk/process-runtime contract, or should Microsoft Foundry use a narrower higher-level lifecycle seam?
  • Rationale: The runtime bug is concrete, but selecting the long-term third-party Plugin SDK shape is a compatibility decision that code review cannot safely infer from one bundled-plugin consumer.
  • Likely owner: haxudev — The original Microsoft Foundry implementation provides the strongest feature-history context for deciding what lifecycle seam its onboarding path needs.
  • Options:
    • Approve and document the helpers (recommended): Keep the three generic exports, document their platform and lifecycle guarantees, add contract coverage, and require native Windows proof before merge.
    • Expose one lifecycle helper: Replace the low-level triad with a higher-level SDK helper that owns detach, graceful signaling, escalation, and close settlement as one contract.
    • Keep the behavior plugin-local: Avoid expanding the public SDK and retain a Microsoft Foundry-local implementation, accepting duplicated process-tree policy.

Security
Cleared: The diff adds no dependency, secret, permission, download, or publishing changes, and process-tree termination remains scoped to the spawned Azure CLI child.

Review findings

  • [P2] Document and approve the process-tree SDK exports — src/plugin-sdk/process-runtime.ts:4-8
Review details

Best 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:

  • [P2] Document and approve the process-tree SDK exports — src/plugin-sdk/process-runtime.ts:4-8
    This prior finding remains unresolved at the current head. These three exports turn internal termination behavior into supported third-party API, but the PR only updates generated baselines and budgets; add the corresponding Plugin SDK contract documentation after maintainers approve the permanent seam, including detach, signaling, escalation, and Windows taskkill semantics.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 156f4c89ed03.

Label changes

Label changes:

  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The Linux real-child proof is useful, but the new Windows shell-descendant behavior has not run on native Windows; add a redacted Windows terminal transcript or recording, then update the PR body to trigger review or ask a maintainer for @clawsweeper re-review.
  • remove proof: sufficient: Current real behavior proof status is insufficient, not sufficient.
  • remove status: ⏳ waiting on author: Current PR status label is status: 📣 needs proof.

Label justifications:

  • P2: This is a bounded onboarding reliability bug with meaningful but limited blast radius and no evidence of a widespread current outage.
  • merge-risk: 🚨 compatibility: The PR establishes three new public Plugin SDK functions whose permanent semantics and documentation have not been approved.
  • merge-risk: 🚨 availability: Incorrect detach or process-tree termination behavior could hang login, reject before teardown, or leave Azure CLI descendants running.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The Linux real-child proof is useful, but the new Windows shell-descendant behavior has not run on native Windows; add a redacted Windows terminal transcript or recording, then update the PR body to trigger review or ask a maintainer for @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +97, Tests +474, Generated 0, Other +2. Total +573 across 7 files.

View PR surface stats
Area Files Added Removed Net
Source 2 98 1 +97
Tests 3 474 0 +474
Docs 0 0 0 0
Config 0 0 0 0
Generated 1 2 2 0
Other 1 5 3 +2
Total 7 579 6 +573

What I checked:

  • Current-main bug remains: Current main pipes Azure CLI stdout and stderr through data listeners but has no stream error listener; the promise settles only from child close or child error. (extensions/microsoft-foundry/cli.ts:173, 156f4c89ed03)
  • Patch addresses the lifecycle gap: The PR adds first-error-wins stream handling, process-tree signaling, force-kill escalation, timer cleanup, and settlement guards around device-code login. (extensions/microsoft-foundry/cli.ts:173, 60388f0a3789)
  • Public SDK contract expands: The branch adds three public exports—signalChildProcessTree, forceKillChildProcessTree, and shouldDetachChildForProcessTree—to the existing process-runtime SDK entrypoint. (src/plugin-sdk/process-runtime.ts:4, 60388f0a3789)
  • Scoped extension policy requires documentation: The extensions boundary requires intentional contract expansions to update documentation, exported surfaces, package exports, and contract checks together; this branch updates baselines and checks but no Plugin SDK documentation. (extensions/AGENTS.md:284, 156f4c89ed03)
  • Scoped SDK policy treats the exports as compatibility-sensitive: The Plugin SDK guide identifies this directory as a third-party public contract, prefers small purpose-built seams, and requires docs and API checks to remain aligned when the boundary expands. (src/plugin-sdk/AGENTS.md:245, 156f4c89ed03)
  • Prior finding remains unresolved: The previous review required documentation of the new process-tree SDK contract; the current head still changes no source page under docs/plugins. (src/plugin-sdk/process-runtime.ts:4, 60388f0a3789)

Likely related people:

  • haxudev: Introduced the Microsoft Foundry provider, including its Azure CLI device-code onboarding path, through the original merged feature work. (role: introduced behavior; confidence: high; commits: a16dd967da51; files: extensions/microsoft-foundry/cli.ts)
  • Alix-007: Beyond this proposal, previously contributed the merged Google Meet and Matrix stream-error lifecycle fixes that establish sibling behavior for first-failure teardown and child-close settlement. (role: recent process-lifecycle contributor; confidence: high; commits: 5fd5bf2aee1b, 35d5ea069ae9; files: extensions/google-meet/src/realtime.ts, extensions/matrix/src/matrix/deps.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (9 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-08T01:03:23.844Z sha ecf6545 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-08T01:10:42.007Z sha ecf6545 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-09T06:45:49.567Z sha 607845a :: needs maintainer review before merge. :: none
  • reviewed 2026-07-09T08:50:22.284Z sha 7e1ed02 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-09T09:17:44.729Z sha f3574f5 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-09T09:43:58.934Z sha 96b04e7 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-12T10:13:05.619Z sha 6fb3b7e :: needs changes before merge. :: [P2] Terminate the Windows Azure CLI process tree
  • reviewed 2026-07-12T17:39:23.868Z sha 9f51c9e :: found issues before merge. :: [P2] Document the new process-tree SDK contract

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jul 7, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 8, 2026
@wangmiao0668000666

Copy link
Copy Markdown
Contributor

@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.

@Alix-007

Alix-007 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 12, 2026
@Alix-007

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 12, 2026
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation and removed triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 12, 2026
@openclaw-barnacle openclaw-barnacle Bot added the scripts Repository scripts label Jul 12, 2026
@Alix-007

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. label Jul 12, 2026
Alix-007 added 2 commits July 13, 2026 10:53
…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
@clawsweeper clawsweeper Bot added status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 13, 2026
@Alix-007

Copy link
Copy Markdown
Contributor Author

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.

@Alix-007 Alix-007 closed this Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Improvements or additions to documentation merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. scripts Repository scripts size: L status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants