fix(windows): make recovery evidence authoritative - #3789
Conversation
77dd902 to
b1f348d
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Update on b1f348d66d:
Spec is PASS — recovery probing now uses live fs.promises.open with correct intercept and bounded ADS walk, correctly retrying outside cleanup.
However Standards remains NO-GO:
- Both commits lack required
Generated-by: <tool>trailer despite AI-authored changes (CONTRIBUTING.md:34). - PR template checklist/behavior Yes/No sections are removed, not filled.
Checks on b1f348d66 are test: failure / windows_recovery: failure (both due to queryTurn removal in execution-host-queue.test.ts, not this diff, but still BLOCKED) — not green.
简体中文
实现通过,标准合规与 CI 门待补。|
Current exact-head failures are upstream bootstrap blockers, not failures in this PR: #3790 prevents Runtime Host build and #3788 prevents message-admission startup. Both are repaired together in green PR #3796 because neither standalone fix can pass required CI before the other lands. After #3796 merges, this branch will rebase, drop the now-upstream schema-constructor overlap, and rerun the automatic windows_recovery check through its real recovery steps. |
b1f348d to
7957ebe
Compare
|
Standards follow-up addressed on new exact head \7957ebe6a: both commits now carry the required \Generated-by: Codex\ trailer, and the PR body restores the Checklist plus behavior Yes/No section. I agree the hosted gates remain blocked by current-main #3788/#3790; their combined repair #3796 is exact-head green and will be merged/rebased before this PR can claim hosted success. |
7957ebe to
9047c2e
Compare
|
Rebased onto merged #3796 at new exact head \9047c2eb3. The overlapping schema-constructor handle patch is now upstream and absent from this diff. Rebase-local planner/harness (76 pass / 0 fail / 1 privilege skip), repository control-plane YAML, lint, format, inventory (64 declarations), Core/Storage builds, and diff check pass. Fresh hosted \ est\ and \windows_recovery\ are running; this head should now reach the actual native recovery steps. |
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found no code blocking issues.
Recovery evidence probing now uses live fs.promises.open with intercept and bounded walk, correctly. No new abstraction.
However machine gate is NO-GO due to branch base drift: execution-host-queue.test.ts:248/249/271 still references removed queryTurn/startTurn/stopTurn — fixed in main by #3796 (c4971c9). Rebase to main ≥ c4971c9 and rerun CI.
Checks on 7957ebe04f are test/windows_recovery: failure due to base drift — not PR-introduced.
简体中文
代码无阻断,基座过期需 rebase。|
Base-drift follow-up is closed on exact head \9047c2eb3: the PR base is merged #3796 commit \c4971c938, the stale queue aliases and migration collision are inherited from main, and the overlapping schema-constructor patch is absent from this diff. Fresh hosted test passed in 17m28s and windows_recovery passed in 3m23s through all native recovery steps. Both commits retain \Generated-by: Codex; the checklist and behavior declaration are complete. |
9047c2e to
d69a179
Compare
|
@Astro-Han The base-drift blocker you identified is resolved. I rebased onto the current The only textual conflict was in the pull-request workflow allowlist in New exact head: Verification after the rebase:
GitHub now reports the PR as mergeable. The remaining |
Astro-Han
left a comment
There was a problem hiding this comment.
Update on d69a179ecb:
Freshness from 7957ebe: rebased with bounded PowerShell+Node walk fallback still fail-closed; marker-open intercept retained. No new P0-P3.
Checks on d69a179ecb5a50976a9652a219d3176ac2f816d7 are test: PASS (32858446128) and windows_recovery: PASS (32858445636).
简体中文
该头仍无新增阻断。
jackwener
left a comment
There was a problem hiding this comment.
Review conclusion on exact head d69a179ecb5a50976a9652a219d3176ac2f816d7 (base 421acbeffc65bf47ed05fdc8d2811e15a97246c9): GO with one non-blocking P2; no P0/P1 findings.
The workflow/config relationship is closed: windows_recovery now has stable PR and main triggers, the ASF-managed context list contains the same job name, and the contract tests pin both sides. The marker-race fixture now intercepts the live fs.promises.open implementation, and the PowerShell query fails closed on start, timeout, output-limit, exit, and parse failures. The remaining P2 is an object-coverage gap in that query, noted inline.
Verification: full build:test; focused Storage/marker/crash tests 27 passed with 2 platform skips; CI planner plus release-control tests 62/62; Windows inventory current at 64 declarations; Biome and diff checks clean. Exact-head hosted test and windows_recovery are completed/success, and the hosted merge-result tree equals the reviewed head tree.
简体中文
精确 head `d69a179ecb` 结论:GO,只有 1 条不阻塞的 P2,无 P0/P1。工作流与 required context 的自触发关系、marker 竞态切点及 PowerShell 失败关闭路径均成立;剩余问题是 ADS 查询遗漏目录对象,见行内评论。ef61719 to
b11da18
Compare
|
P2 follow-up and current gate status on exact head
Hosted |
Astro-Han
left a comment
There was a problem hiding this comment.
Update on b11da18e7c:
GO — no P0-P3 on code. Windows recovery now uses P/Invoke FindFirstStreamW with correct handle/EOF handling and directory scanning; prior base compilation P1 closed via rebase. windows_recovery: PASS on this head. Remaining test: FAILURE is unrelated e2e composer-plus-menu-stability flake tracked in #3814, not attributable.
简体中文
该头无阻断,残留失败与本改动无关。Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
b11da18 to
da660e4
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Update on da660e47:
[P2] Marker authority uses mutable fs.promises.open binding
marker-file.ts:38-42 reads fs.promises.open dynamically per call. Later-loaded code can replace it and forge handles whose stat()/readFile() align with real path — bypasses descriptor/path checks for root identity, weakening flags or causing DoS.
Fix: capture intrinsic at module evaluation or inject explicitly; fixture should patch before dynamic import.
Note: windows_recovery: SUCCESS on this head only ran IPC/SQLite paths, not ADS targeted tests — limited evidence.
简体中文
marker 权威绑定可被替换。|
@jackwener The directory-ADS P2 is closed on exact head Fresh exact-head gates are green:
Local file/root/nested-directory ADS cases remain 3/3 and managed dependency crash recovery remains 5/5 after the rebase. Could you please re-review the current head when available? |
Summary
This PR advances #2624 and the Windows support plan in #2142 by making the native recovery check eligible for branch protection and removing two deterministic failure groups from the Windows baseline.
windows_recoveryon every pull request andmainpush;windows_recoverybesidetestin the ASF-managed required status checks and locks the workflow/config relationship with a CI contract;fs.promises.open, so the Node 24 Windows initialization-race fixture reliably reachesmarker_open_pendingwithout relying on builtin ESM export synchronization;fsutil file queryStreamscall with one bounded Windows stream query fed by Node's no-reparse tree walk;FindFirstStreamW/FindNextStreamW, including Win32 namespaced paths for long-path compatibility;Runtime PTY cleanup is intentionally excluded because another contributor has already claimed that slice on #2624. The remaining Git fixture cleanup and symlink-permission inventory are also not folded into this PR.
Why the ADS change is needed
fsutil file queryStreamsis not a supported subcommand on the current developer Windows build or the GitHub hosted runner. Normal dependency trees therefore failed before their receipt could be verified. The previous ADS test matched the words "alternate data stream" in the query-failure diagnostic, so it passed without proving a named stream had been observed.The replacement keeps traversal in Node, rejects reparse points before the external query, and sends a JSON array over stdin to one bounded Windows PowerShell 5.1 process. The process hosts the documented Win32 stream enumeration APIs because the PowerShell 5.1 provider does not enumerate directory streams. Only
ERROR_HANDLE_EOFis accepted as no streams; other API and process failures reject the environment. Regressions now prove exact named-stream rejection for a regular file, the published dependency root, and a nested directory.Verification
Local Windows, Node 24.19 for platform-sensitive tests:
npm run windows:inventory: current, 64 declarations;git diff --check: passed.Hosted exact-head evidence:
windows_recovery: passed in 3m17s, including Local IPC, SQLite crash, Runtime continuation, Runtime Host owner-death, and managed workspace crash recovery;test: passed in 17m23s, including all 67 Desktop e2e tests, Storybook smoke, and installed CLI validation.The post-merge #3788/#3790 blockers were repaired by merged #3796. The overlapping schema-constructor handle fix is inherited from
mainand no longer appears in this PR diff.Administration boundary
.asf.yamlis the repository's declarative branch-protection authority. After merge, ASF infrastructure must apply the updatedmainprotection sowindows_recoveryis actually required. The workflow now reports that stable context on every PR and main push, so enabling it cannot leave unrelated PRs permanently pending.AI use
Tool and scope: Codex traced recent Windows baseline artifacts, reproduced the system API and handle failures on Windows/Node 24, implemented the storage and workflow changes, and ran the listed local gates.
Refs #2624. Refs #2142.
Checklist
windows_recoverypasses on rebased headda660e47dtestpasses on rebased headda660e47dDoes this PR entail a change in behavior?
windows_recoveryreturns to PR/main and becomes a required context; Windows storage validation now uses supported Win32 ADS enumeration for files and directories plus a portable marker-race interception boundary.