Repository navigation
Preserve Windows KiCad paths when saving health JSON - #951
Conversation
|
Canonical proof status: NO RESULT — NOT PASSED. Boat job The green Merge still requires the previously authorized independent review/scoped-check exception for the known full-hub cap limit. Corrected packaged-build native qualification remains outstanding, and #927 stays open until the verified change ships. |
i2cjak
left a comment
There was a problem hiding this comment.
Independent review of 79a18057e23e6b235f3b57905d52aee5c08b606a: mergeable as-is as a source fix; no must-fix defect found in the health-cache change. The head has not moved. GitHub's current base is e07633b1203b27051d65ef9071816eea5acfea89; the actual merge-base/head parent is caf71af247d87f9d89c0af7ced8a2721d152d541. I reviewed the three-dot PR diff, relevant callers/readers, PR description, comments, and tracked receipts. References below are at the reviewed head.
Findings, ordered by severity (none blocks this scoped fix):
- P2, pre-existing / separate follow-up — JSON still crosses the same shell boundary elsewhere.
src/server/server.bend:8741(Tk.append) passeslineas ash -cargument toprintf. Its callers atsrc/server/server.bend:17479and:17586supplyJson.show(...)containing free-text notes/summaries;.draft.jsonluses it at:8748too. Thus JSON containing Windows backslashes still has the original transport exposure in browser-task logs. This PR removes it from the health-cache writer, not repository-wide. These lines are unchanged by this PR; address them separately rather than claiming all JSON-through-Cygwin paths are gone. - P3 — the new Linux checks do not distinguish the original Windows bug from the old implementation.
.github/workflows/ci.yml:312runs both new tests in the Ubuntujsjob (:229). The compiledRt.saveregression exercises the real encoder, effect dispatch and reader, but restoring just the old shell-basedRt.savewould still pass it on Linux: POSIX argv does not perform the offending native-Windows-to-Cygwin conversion. The effect controls test the new helper directly, so they also cannot detect that caller-only reversion. Add a native Windows/pinned-Bun negative control when doing corrected-package qualification. The value matrices (test/tools/kicad_cache_test.py:22,test/tools/atomic_text_test.cjs:19) cover literal backslashes, b/n/t, spaces, Unicode, UNC and trailing separators, but omit a literal backslash-u path and extended-length\\?\payloads, and do not exercise native UNC/long destination paths or held-open readers. - P3 — make the advertised Windows test explicitly UTF-8.
test/tools/kicad_cache_test.py:35uses locale-defaultread_text()on a UTF-8 file. On Windows Python configured with a legacy code page, the Japanese case can fail despite a correct writer. Specifyencoding='utf-8'; likewise make the captured subprocess text encoding explicit at:29if using this harness on such a runner. This does not invalidate the Linux receipt.
Scope assessment:
- Atomicity/cleanup:
src/server/effects/host.js:1884-1897stages under the destination's parent, so the rename stays on the same volume. It opens exclusively, writes the complete UTF-8 string, closes before rename, and never unlinks/truncates the destination first. Normal partial-write exceptions and rename failures close the descriptor and remove the private staging directory infinally, preserving the old cache. The injected tests attest/tools/atomic_text_test.cjs:32-40meaningfully check both preservation and cleanup; the write injection really writes a partial file before throwing. Cleanup is best effort ifrmSyncitself fails, and process termination can leave staging directories; neither is tested or a reason to promise unconditional cleanup. - Windows replacement/readers:
fs.renameSyncis the replace-existing operation, not a copy/delete fallback implemented by this patch. Windows replacement can fail when a reader denies delete-sharing; that should retain the old complete cache and take the cleanup path. A compatible reader can keep reading the old file. The Windows sharing contract and replace-existing operation support this distinction. The generic injected rename failure is useful, but is not experimental verification of either case in the packaged Bun runtime.Rt.savestill intentionally discards save errors (src/server/kicad.bend:481), as it discarded the old subprocess result. - Fsync policy: there is no file or directory fsync (
src/server/effects/host.js:1887-1891). This provides replacement atomicity during ordinary operation, not power-loss durability. Omitting fsync is reasonable for this disposable, recomputed health cache: boot parses and validates it and rediscovers on a miss (src/server/kicad.bend:426-433). Do not treat the generic helper as durable storage for authoritative data without a separate flush policy. - Exact bytes/compatibility:
Rt.savestill supplies exactlyJson.show(Rt.json(rt)); native UTF-8 writing adds no BOM, CRLF conversion, newline or trailing newline, matching the old intendedprintf '%s'output. Schema and readers are unchanged (src/server/kicad.bend:401-433,src/server/server.bend:3152). Literal b/n/t/u sequences, UNC and extended-prefix strings are opaque JSON payload data at this boundary; no path normalization or second escape interpretation is applied to them. That establishes the serialization logic, not native discovery/execution support for every long-path spelling. - Source/effect integration:
src/server/host.bend:229,src/server/effects/host.js:2365, andsrc/server/kicad.bend:481agree on the two String arguments and Result-to-Unit handling. Both production health-save callers still useRt.save; I found no second health writer or dependency on the removed.tmpname/shell side effects.
The seven discovery tests and installer fixtures are useful adjacent regression checks, but neither invokes the production health-cache save boundary, so they would also pass without this fix. Three actual PowerShell cases were skipped on Boat; the older native package receipts reproduce the bug and establish installer results, not success of this corrected source. The compiled save test and effect controls provide complementary coverage, with the Windows limitations above.
No tests/builds/proof were run locally or remotely for this review; the recorded scoped results were inspected, not independently rerun. The canonical proof remains incomplete, not passed. Corrected packaged-build Windows acceptance remains outstanding and #927 should stay open through qualification/shipping. This COMMENT is not release signoff or authorization to lift the 0.14.2 publication hold; merge remains owned by the #927 thread.
i2cjak
left a comment
There was a problem hiding this comment.
Second independent review of 79a18057e23e6b235f3b57905d52aee5c08b606a. It adds to the earlier COMMENT and does not repeat it. No blockers: I found no defect in the Rt.save change. I agree with that review on Tk.append, the caller-only revert gap, the read_text() encoding, fsync policy and Windows sharing semantics.
This was a static review. I ran no tests, builds or proof, on Boat or locally, so nothing here has been run on Windows.
Should fix before merge (small, not blockers)
1. Neither new test is in the merge gate. docs/reviews/kicad-health-cache-927.md:74 says "Both are included in CI", but ci.yml only has workflow_dispatch and workflow_call triggers, so it runs nightly or on a release. The merge gate is scripts/local-ci.py, and neither test is reachable from it: scripts/local-small.py lists its tests by name, and neither scripts/fast-tests.txt nor scripts/extended-fast-tests.txt includes them. A break would reach main and show up only in the next nightly. atomic_text_test.cjs needs no Bend and takes seconds, so it can go in local-small.py. The doc line should also say nightly/release, on Linux.
2. Two cheap ways to close the Linux-only gap, without waiting for package qualification:
- Caller-only revert: add a source assertion that
Rt.savecallsH.FS.write_atomicand contains noProc.run.test/tools/kicad_open_script_test.py:24already readskicad.bendthis way, and this one does fail on Linux. - Native replacement: add
bun test/tools/atomic_text_test.cjstowindows-agent-tree(ci.yml:65). That job is already onwindows-2025with Bun 1.4.3 and needs no Bend. The test's loop writes the same target five times, so this would be the first run of rename-over-existing on Windows under the shipping runtime.
3. The effect test runs on the wrong runtime. ci.yml:312 and extra/kicad-927-native-fix/check.py use node, but the hub ships on Bun. local-small.py already runs similar effect tests with bun, so switching costs nothing.
Worth doing, can follow
4. Log the failure in Rt.save. src/server/kicad.bend:481 discards the errno that the new effect now returns. The discard itself isn't a regression, but it matters for qualification: a failed mkdtemp or rename on Windows leaves a stale cache and no trace, which looks just like a fix that did nothing. One log line would make the packaged acceptance run diagnosable. Staging also creates a directory beside the target, and I could not confirm how Bun on Windows treats that for very long home paths. That failure would be equally silent.
5. Say in the host.bend comment that FS.write_atomic is not durable. I agree fsync is unnecessary for this cache. But fs_pairing_token in the same file does fsync, so the name invites a later caller to assume more than it provides.
Checked, nothing to change
- Other argv writers: besides
Tk.append,PiRun.write(server.bend:5134),OmpRun.write(:5173) andAgx.write(agyio.bend) also send content throughprintf '%s' "$2". None is exposed today, because their payloads never emit two adjacent backslashes. Pi has no backslash sequences, Omp has one'\\n', and Agy has only single ones. This is fragile, since a\\added later would be halved on Windows without warning, so include them in theTk.appendfollow-up. My search was line-based, so it is not exhaustive. - Upgrading from an already corrupted cache: this heals without a cache version bump. Invalid JSON parses to
Nulland fails the stamp check. A decodedsettingfails thesettingequality inRt.boot. Both go back through discovery, andHub.kfound.*then saves a good file. One residual case: a file that stays valid JSON, with matchingstampandsettingbut some other field decoded, would be trusted byRt.cached. I found no concrete field that does this;Rt.normalkeeps nativeclipaths on forward slashes. - Receipts and base: the sha256 values for
host.bend,host.js,kicad.bend,LAWS.bendandPROOF.bendin the receipts match this head. Main is now703505cae. No commit since the merge basecaf71aftouches those files,src/core/json.bend,src/core/kicad_install.bend,test/check.bendorci.yml, so the scoped results should still hold on the merged tree.scripts/pre-merge.shis still required. - Pins and evidence: no hash pin or effect list needs updating for the new effect. A keyword scan of
extra/kicad-927-native-fixfor tokens, keys and user paths found nothing sensitive.
The missing proof run
Low risk from this diff, but it is still not a pass. The OOM tells us nothing about this change either way, because the run hit the 3.7 GiB cap at 13.5 s and gave no verdict.
LAWS.bendandPROOF.bendare unchanged.- No law or proof names
Rt.save,Rt.jsonorFS.write_atomic, so no proved statement depends on the changed body. bendok.shdoes not pin the count of foreign defs, so one more foreign def inhost.bendshould not change the verdict.Rt.savekeeps its signature, so its callers inserver.bendare unaffected.
The real gap is narrower. LAWS.bend imports server.bend, so kicad.bend and host.bend are inside the proof's closure, and neither the proof nor scripts/check.sh check has completed on this tree. The evidence that those files compile is the kicad_cache.bend compile and run in the Boat receipt. Since that receipt has no separate --check-only result, treat the next nightly check and proof jobs as the required backstop, and confirm they are green after merge.
Verdict
No blockers from this review. Items 1-3 are cheap and worth including before merge. As the PR says, #927 stays open until a corrected package passes native acceptance. This is not release signoff.
i2cjak
left a comment
There was a problem hiding this comment.
Delta review of 319141ec379957d05563fa6c5a2ec0c161c9ee48 against the reviewed 79a18057. The requested changes are good, and I found no blockers. I read the code and hashed git blobs; I ran no tests.
- Production bytes:
git diff 79a18057 319141ec -- src LAWS.bend PROOF.bendis empty, so the earlier source review and proof assessment carry over unchanged. - Merge gate:
atomic_text_test.cjssits above the--focused-onlyand--guards-onlybranches inscripts/local-small.py, so "every lane" is accurate. The lane can't skip it on a stale pass either: with no roots,inputs-key.shhashes every tracked.bendand.jsfile, so a change tokicad.bendorhost.jsforces a rerun. - Source assertion: it closes the caller-only revert gap on Linux. It requires
H.FS.write_atomic(and forbidsProc.run(within theRt.savedef, so moving the shell call into a helper fails too. - Runtime and wording:
ci.ymlandcheck.pynow use Bun, and the doc correctly says nightly/release, Linux, not the per-PR gate. - Sidebar guard failure: this is independent of this PR. I hashed all 177 pins in
scripts/sidebar-draft-inputs.jsonat this head, atcaf71afand at main123676a2. The same three files mismatch at all three commits (src/core/kexport.bend,src/core/refview.bend,src/mobile/hubs.bend), and this PR touches none of them or the guard.
Limits to keep in mind (none blocks this):
backplane/boat-small-gateis red, so the small gate has not passed for this PR. The lane stopped at the sidebar test, so the steps after it never ran in job 1195105, includingkicad_library_header_test.pyandkistack_test.py. The new test ran before that point and passed. The rerun after the #948 remediation is what qualifies the gate.- Receipt
1195105ran on a dirty tree at79a18057, and itssourceshashes cover only the three production files, which do match this head. Its output shows the new assertion ran, but nothing ties the test bytes to319141ec. The rerun on the committed head settles that as well. - Rename-over-existing on Windows under Bun is still unrun, because the optional
windows-agent-treestep was not added. Corrected-package qualification remains the first native check.
The proof status (no result) and the qualification hold are unchanged. This is not release signoff, and merge stays with the #927 thread.
319141e to
f6f0cc0
Compare
i2cjak
left a comment
There was a problem hiding this comment.
Rebase confirmation for f6f0cc08137edc1dac2b96e7013e34e4a978a209 (tree 63f9811113bc00889009823f5526334408f9d677) on main 64eca09231b21cf009a9de81b8e8158dcdc91644. The rebase carried over exactly, and I found no blockers. This was a static review of git objects; I ran no tests.
- Same two commits:
git range-diffshows=for both, and the stable patch IDs match (79a18057→75da4ed03a,319141ec→f6f0cc0813).64eca092is the merge base, and main has not moved since. - No PR-owned file changed: the file set is the same 20 files. I compared blob IDs at
319141ecand this head for all of them except the olderevidence/run-*receipts, which are covered by the patch IDs. That includes the three production files,scripts/local-small.py,test/tools/atomic_text_test.cjs,ci.ymlandtest/tools/kicad_cache_test.py. - Sidebar pins now match: all 177 pins in
scripts/sidebar-draft-inputs.jsonmatch at this head, so the three stale pins that stopped the last small-gate run are gone. That is a hash comparison only, not a run of the guard or its tests. - Earlier assessments still apply: main's commits since
caf71afchanged none ofsrc/core/json.bend,src/core/kicad_install.bend,test/check.bend,scripts/inputs-key.sh,scripts/local-ci.py,scripts/bendok.sh,LAWS.bendorPROOF.bend. No law or proof namesRt.save,Rt.jsonorFS.write_atomic.
Not established by this review:
- No commit status exists on this head yet, and
ompwas still running when I checked. The completelocal-smalland scoped Boat results for this committed tree are what clear the two limits from my delta review (the small gate has not passed, and receipt1195105was not tied to committed test bytes). Please confirm that job ran atf6f0cc08with a clean tree. - Proof remains NO RESULT, not passed, so the next nightly
checkandproofjobs are still the backstop. - Rename-over-existing on Windows under Bun is still unrun. The combined Windows PASS is its first native check.
This is not release signoff. Merge stays with the #927 thread.
|
Rebased #951 onto actual #948 merge Current reviewed head:
The canonical proof is still OOM-killed, NO RESULT, NOT PASSED. Neither scoped tests nor structural checks replace that verdict. Corrected-package native qualification remains outstanding; the new OMP consumer preparation has not run. #927 remains open. Root alone owns the user-authorized re-cut and publication after the corrected combined Windows acceptance passes. |
f6f0cc0 to
40b56de
Compare
i2cjak
left a comment
There was a problem hiding this comment.
Final rebase confirmation for 40b56de2ee29ad3b3fbed4b46c66ad45a97f8777 (tree 9f07ccb5e274187de26148b63f21d733ea9f71ab) on main af56a8b2d0aad0c84a7cf90ff9a4ae22b0fa4100 (tree 928f878df42375ccf8ec3752342ca8ff2ba7d483). The rebase carried over exactly, and I found no blockers. This was a static review of git objects; I ran no tests.
- Same two commits:
git range-diffshows=for both, and the stable patch IDs matchf6f0cc08,319141ecand79a18057.af56a8b2is the merge base and still the tip of main. - No PR-owned file changed: the same 20 files, with zero blob differences from
f6f0cc08. - Main's delta is as described: #955 and #956 change
.github/workflows/release-bootstrap.yml(the producer pin) and fourdesktop/testfixtures. None of those five files is in this PR, and none is asrcfile or LAWS/PROOF, so my earlier source and proof assessments still apply. - Sidebar pins: all 177 match at this head. That is a hash comparison, not a run of the guard.
Not established by this review: this head has no commit status yet, and omp was still running when I checked. The complete small gate needs to pass on 40b56de2 with a clean tree, and it should be the receipt the merge relies on, since earlier ones were for other heads. Proof remains NO RESULT, not passed. Rename-over-existing on Windows under Bun remains unrun until the combined Windows run.
This is not release signoff. Merge stays with the #927 thread.
|
Final stock small-gate step: kernel-confirmed OOM, NO RESULT, NOT PASSED. On reviewed head The diagnostic retry
The diagnostic wrapper exited 0 only because evidence collection completed. It is not a test pass. The diagnostic transport added only an untracked wrapper to the otherwise unchanged source; the original complete-gate attempt was clean. The first retry never launched because Boat excluded its diagnostic file under The unchanged canonical Compiler lane B alone owns the explicitly authorized one-time mailtriage test using composite compiler |
|
Merge-owner concurrence on final head I audited Lane B's composite execution receipt for The actual command compiled and ran the test IO main with composite compiler Root and Bub concur; I accept this as a scoped substitute for the unavailable stock mailtriage execution evidence for this PR's merge only. No repository guard changes or general compiler-equivalence approval follow. The existing The receipt is retained at Next: fresh exact-head capped Boat premerge guard, then normal merge without admin or branch-protection bypass. #927 stays open pending corrected-package native acceptance and shipment; no build or publication is authorized by this receipt alone. |
|
#951 merged normally as Fresh premerge Boat job The remaining Windows corruption was reproduced in health-cache saving: native Windows→Cygwin argv halved JSON backslashes. The native UTF-8 atomic writer removes that shell/argv round trip. Scoped cache/discovery/installer regressions passed. The actual mailtriage IO test passed 138/138 on the exact tree with the assigned composite compiler under the unchanged 3.7 GB / swap 0 cap, accepted by root/Bub/merge owner only for this merge's missing regression evidence. The complete stock small gate did not pass: stock mailtriage was OOM-killed with no result. Canonical PROOF was also OOM-killed with NO RESULT / NOT PASSED. Neither status was rewritten; no proof or guard was weakened. #927 remains OPEN. Corrected-package real Windows standard-user acceptance and shipment are still required; no new-package result or release signoff is claimed. Follow-ups remain |
Native standard-user Windows testing of packaged 0.14.2 reproduced #927's remaining corruption:
Rt.savesends health JSON through native Windows→Cygwin argv, which halves paired backslashes. A savedC:\backplane_kicad\can then decode a backspace;\n/\tpaths are affected too. The UI, RPC, SQLite setting and reload preserve the typed spelling.Write the cache as native UTF-8 through a private same-directory temporary file and atomic rename. Failed writes/renames preserve the previous cache. Add actual Bend
Rt.saveIO coverage and effect controls for drive letters, UNC paths, trailing backslashes, escape sequences, spaces, Unicode, quotes, controls and partial-write failure. Everylocal-small.pylane runs the Bun effect test and asserts thatRt.saveuses the native writer withoutProc.run; Linux nightly/release CI also runs the cache regressions.Refs #927. Keep #927 open until a corrected packaged build passes native acceptance and the fix ships. Native installation itself succeeded on the previous package (real UI download/extraction and pinned CLI execution), but the health-cache check failed. The unchanged canonical #946 native check passed. Original package identities and the end-to-end trace are in
docs/reviews/kicad-health-cache-927.mdand the tracked receipts.Validation on final head
40b56de2ee29ad3b3fbed4b46c66ad45a97f8777, tree9f07ccb5e274187de26148b63f21d733ea9f71ab:j20261104205744-1758427, clean source, cap 3,699,998,720 bytes / swap 0: scoped KiCad controls and actual compiledRt.saveIO passed; discovery 7/7 and installer 21 tests passed (three native PowerShell cases skipped on Linux). The complete small-gate invocation passed its Python/Bun/Node prefix, including 389/389 Node tests, then the final stock Bend mailtriage step was killed. The stock complete gate did not pass.j20261104210111-1791885: unchanged stockbend test/mailtriage_test.bendwas kernel OOM-killed after 19.594 seconds under the same cap, with no output: NO RESULT.j20261104210315-1812403: actual compile, IO main and all 138 original assertions passed on the exact final source with composite compiler41262875, Bun 1.3.10 and frozen flags. Exit 0, 138ok, noFAIL/Error; 150.636 seconds, cgroup peak 2,662,490,112 bytes, swap 0, zero OOM. Root, Bub and the merge owner accept this only as the missing regression execution evidence for this Preserve Windows KiCad paths when saving health JSON #951 merge. It is not a stock-gate pass, establishes no stock/composite output equivalence, and changes no guard.bend PROOF.bendwas attempted unchanged inj20261104193708-1087045: kernel OOM kill after 13.529 seconds under the 3.7 GB cap, no output: NO RESULT / NOT PASSED. LAWS/PROOF were not weakened. Structural validation is a separate check.Corrected-package native qualification, including Windows rename-over-existing under Bun, remains outstanding.
Rt.saveerror reporting and the pre-existingTk.appendJSON-through-shell writer are recorded as nonblocking follow-ups on #927. No local tests/builds, app packaging, deployment or restart was performed here. Root owns the authorized release re-cut and publication after the new combined packaged Windows gate passes. This thread owns checking the next scheduled nightlycheckandproofresults.