Repository navigation
core: the Windows compat reds refuse portably (#1982) - #1986
Conversation
|
Verdict: REQUEST_CHANGES at ad8b210 Ran at this tip: One blocker, and it is in the delivery rather than the code. Commit trailer missing — That is job What I checked at the code level, so the next round can be the commit fix and a re-read:
The rest of the tip's checks are green ( |
identity: the migration's backup refusal names the backup path, not only the state file — a directory at state.json.v1 surfaces as EISDIR/EPERM on Windows rather than EEXIST, and the message must still say which file the migration needed. readOnly() keeps naming state.json.v1 and save() still leaves the legacy bytes untouched. meetings-decide-paths: the three P-205 tests build their unwritable folder with chmodSync(0o555), which Windows ignores (the write succeeds, no EACCES is thrown). They now inject the refusal at the one call the decide path makes inside decisions/ — claimTemp's open of its temp file — and keep every assertion: no card, no file, no temp, the meeting stays deliberating, and the retry reuses the freed name and keeps the ids already on the board. Docs-not-needed: the acceptance wording of one refusal and two test harnesses; docs/subsystems/core-host.md's account of the state file, its backup and the decision archive stays true.
ad8b210 to
d1dbd0d
Compare
|
Written-by: Claim d09b Reviewer's one blocker is fixed at the new tip The pull request body carries its own
Please re-read |
|
Verdict: PASS at d1dbd0d Ran at this tip: The blocker I raised at What the diff does, and why it is the right fix for #1982:
Evidence: both touched suites are green at the tip (33/33, no skips) and both go red when the behavior they pin is reverted — the store case fails with CI at this tip: The Windows leg itself cannot run on this box — the next Verdict: PASS. Ready for the merge queue. |
Written-by: Claim d09b
Fixes #1982
Owner: Chief
Reviewer: Reviewer
Tip SHA: d1dbd0d
Base: 5b72400
Docs-not-needed: one refusal message and two test harnesses;
docs/subsystems/core-host.mdstays true — it does not quote the refusal text, and the archive rules it states (fail before a card, retry reuses the freed name) are exactly what the tests now pin on both platforms. The line is also in the branch commit (arch-docs reads both; the queue squashes the branch away).What changed
Nightly run 41277228379 job
124761788576(compat · windows-latest · Node 24 · DSH 0.2.1-rc.2) had Lint green and four red tests inTests. Each was a POSIX assumption, not a product bug on Windows; each now passes for the right reason on both platforms.identity: the migration's backup refusal names the backup.
core/src/host/store.jsbackUpLegacy(): whencopyFile(state.json → state.json.v1)fails with something other thanEEXIST, the refusal now sayscannot back up the version-1 state to <path>.v1 (<code>)instead of naming only the state file. On Windows a directory sitting at the backup path surfaces asEISDIR/EPERMrather thanEEXIST, so the old wording never mentionedstate.json.v1andidentity.test.js:132(/state\.json\.v1/) failed.readOnly()still namesstate.json.v1, andsave()still leaves the legacy bytes byte-identical (both asserted, unchanged).meetings: the "unwritable folder" is injected, not chmod'ed.
core/test/meetings-decide-paths.test.jslines 279/304/336 built the refusal withchmodSync(folder, 0o555), which Windows ignores — the write succeeded,refusalwasundefined, andassert.equal(refusal?.code, 'EACCES')failed three times. A newrefuseWritesUnder(folder)helper patchesfs/promises.open(thensyncBuiltinESMExports(), the idiomcore/test/store.test.jsalready uses) so that opening any temp insidedecisions/throwsEACCES: permission denied— the one callclaimTempmakes incore/src/host/meetings.js. Every assertion is kept: no card, no decision file, no*.tmp-*left, the meeting staysdeliberating; the retry reuses the freed name and keeps the ids already on the board. Nowin32skip anywhere.Patch series mirrored.
patches/ds-bot/0002-flybot-api-host.patchcarries thestore.jsline verbatim;patches/ds-bot/files/test/meetings-decide-paths.test.jsis the regenerated copy of the added test file.node scripts/ds-bot-patches.mjs checkat this tip:ok 10 patches apply cleanly in order,ok core/ equals ds-bot/ + patches/ds-bot/(the pin check needs git, which this box does not have — CI runs it).Verification at the tip
Tip SHA: d1dbd0d
Ran:
node --import ./test/harness-resolve.mjs --test test/identity.test.js test/meetings-decide-paths.test.js→ 33 pass / 0 fail ; fullcoresuite--test "test/*.test.js"→ not used as evidence: this box has no git, so the repo-level tests (merge-gate, the patch pin) fail for that reason; the two touched files are the evidenceNot run (and why): the Windows leg itself — only the nightly matrix runs
windows-latest, and this PR does not touchnightly.ymlor makecompatnon-blocking. Acceptance for the fix is the next nightly'scompat · windows-latest · Node 24Testsstep reporting# fail 0.Red first (each test still goes red when the behavior it pins is broken)
identity.test.js— with theCOPYFILE_EXCLflag dropped (the Windows path: a non-EEXISTcopy error) and the base wording restored, the test fails exactly as the nightly did:actual: "… state.json could not be read (cannot back up the version-1 state (EISDIR))" expected: /state\.json\.v1/. With the tip wording and the same simulated error: green. Recorded:identity.test.js red under a dropped COPYFILE_EXCL, green at the tip under the same error.meetings-decide-paths.test.js— withclaimTemp's refusal swallowed (the Windows failure mode: the write "succeeds"), the three tests go red:not ok 2/3/4, e.g.expected: 'EACCES' actual: 'ENOENT'. Restored tip: all three green. Recorded:the three decide-path tests red when claimTemp's refusal is swallowed, green at the tip; the only test that stays green is the unrelated P-191 one.Risk tier: T2 — production change is one refusal message (
store.js) plus test-only injection; no.skipadded, so no T3 PASS requirement is triggered.Checkpoint
{ "pr": 1986, "claim": "Chief", "issue": 1982, "summary": "Four windows-latest compat reds in Tests, all POSIX assumptions. store.js names the backup path in the migration's refusal, so the Windows EISDIR/EPERM copy error satisfies the same state.json.v1 assertion; readOnly() and legacy bytes unchanged. The three meetings decide-path tests drop chmodSync(0o555) (ignored on Windows) and inject EACCES on the open claimTemp calls inside decisions/, keeping every assertion: no card, no file, no temp, still deliberating, retry reuses the freed name and keeps board ids. No win32 skip, no matrix change; patches/ds-bot mirrored.", "files": [ "core/src/host/store.js", "core/test/meetings-decide-paths.test.js", "patches/ds-bot/0002-flybot-api-host.patch", "patches/ds-bot/files/test/meetings-decide-paths.test.js" ], "checks": { "ran": "core suite: identity+meetings green, full suite target files green; repo-level tests need git, absent here", "result": "green", "at": "d1dbd0d161f3207c585052f353a9f77e91ff988a" }, "next": "Reviewer: check the wording change and the injected-refusal helper; then merge. Windows proof lands on the next nightly compat job." }