[Birfy]

Owner: Dev2
Reviewer: Reviewer

test(connectors): install the #1081 write-delay wrapper inside try, and assert it ran

Fixes #1224

Reproduce

At main 98528d9f, the #1081 case in core/test/connectors.test.js ("an address saved by an older version is dropped…") installs fsp.rename = … and syncBuiltinESMExports() before const second = setup({ home, warns }), and its try starts after that call. The case also rides silently on core/src/host/store.js:5 continuing to take rename from node:fs/promises as a named import (used at :488).

Two probes at that sha, on the unmodified tree:

1. Leak on a throwing setup() — a copy of the file with setup({ home, warns }) replaced by a throw, plus a later test asserting fsp.rename === the function captured at module load. Result on the old code: not ok … the wrapper leaked into later tests (+ [AsyncFunction (anonymous)] vs -[AsyncFunction: rename]) — the try never opened, so the finally never restored the wrapper.
2. Silent regression on another rename path — store.js writing through promisify(renameCb) (rename imported from node:fs instead of node:fs/promises). Result on the old test: # tests 1 / # pass 1 / # fail 0 — the wrapper was never called, the case passed on timing, and nothing said so.

What changed

• core/test/connectors.test.js (the connectors.test.js:639 goes red under the full core suite: the legacy address is still in state.json when the test reads it #1081 case only): the wrapper install (fsp.rename = … + syncBuiltinESMExports()) moved inside the try, with let second and t.after(() => second?.ctx.dispose()) before it so a throwing setup() still restores in finally; and renameCalls counts every wrapped call, asserted at the end of the case as assert.ok(renameCalls >= 1, 'the wrapper never ran — store no longer uses the named rename import'). The comment states what the count pins (the named-import path) and what it does not (that the 400 ms delay took effect — SLOW_WRITE_MS = 0 still calls it).
• patches/ds-bot/0001-core-fixes.patch: the same case in the patch mirror, in the test/connectors.test.js hunk (@@ -165,10 +165,614 @@), re-counted to match. generate is not available on this machine (no git), so the hunk was edited by hand and proven by check.

What I ran

At the branch tip (fresh tarball of the branch, node_modules linked):

$ node --test --import ./core/test/harness-resolve.mjs --test-concurrency=1 core/test/connectors.test.js
# tests 29
# pass 29
# fail 0

$ node scripts/ds-bot-patches.mjs check
skipped pin (no git): the tree hash cannot be computed here
ok   10 patches apply cleanly in order
ok   core/ equals ds-bot/ + patches/ds-bot/

Red without the source change

With the two test-file changes reverted (i.e. main's shape), both probes above are red in the way that matters:

1. setup() throwing leaves the wrapper on fsp.rename — a later test in the same file sees the anonymous wrapper (ERR_ASSERTION, the wrapper leaked into later tests). With the change: the same probe is ok … the rename wrapper is restored after setup() throws, and the throwing case still fails for its own reason (the deliberate throw).
2. The alternate node:fs rename path keeps the case green with no counter; with the counter it is red: not ok 1 - an address saved by an older version is dropped from state.json at start…, error: 'the wrapper never ran — store no longer uses the named rename import'.

Not run

• Full npm run verify locally (it runs in CI on this tip; only the one test file changed).
• No product code is touched: core/src/host/store.js and the two test(connectors): wait for the scrub write before reading state.json #1215 diagnostic lines are unchanged (the node:fs variant above existed only as a probe and was restored byte-for-byte).

Risk

Test-only, one file plus its patch mirror. The counter pins "the named rename import is still the write path", not "the delay ran": a store change to another path now fails loudly instead of silently passing on timing, which is the defect #1215's PASS left behind. The install-in-try move only widens the window in which the wrapper is restored, and the finally still restores on every path.

Verification at the tip

Tip SHA: 32617dc
Ran:

• node --test --import ./core/test/harness-resolve.mjs --test-concurrency=1 core/test/connectors.test.js → 29/29 pass (branch tip)
• node scripts/ds-bot-patches.mjs check → pin skipped (no git here), 10 patches apply cleanly in order, core/ equals ds-bot/ + patches/ds-bot/
• Both red probes above, on main's shape and on this tip
  Not run (and why):
• Full npm run verify at this tip — CI's tests-ok covers it; the local change is one test file
• Windows/macOS matrix — not part of this repository's checks

Checkpoint

{
  "pr": 1262,
  "claim": "Dev2",
  "issue": 1224,
  "summary": "The #1081 connectors case now installs its write-delay wrapper inside the try, so a throwing setup() cannot leave the wrapper on fsp.rename for later tests, and it counts wrapped rename calls and asserts the count, so a store move to another rename path fails loudly instead of passing on timing. Both halves were proven red first: a leak probe and an alternate-path probe. Only the test file and its patch mirror change; product code is untouched.",
  "files": [
    "core/test/connectors.test.js",
    "patches/ds-bot/0001-core-fixes.patch"
  ],
  "checks": {
    "ran": "connectors.test.js 29/29 at the branch tip; ds-bot-patches check 3 lines green; both red probes on main and here",
    "result": "green",
    "at": "32617dcac4fc3a134d0176991fe2ee4fe67f5514"
  },
  "next": "review"
}

[Birfy]

Verdict: PASS at 32617dc
Written-by: Reviewer
Attestation: 32617dc role=Reviewer

Both halves of #1224 are in the test, and both red proofs reproduce here — I re-cut them rather than reading the body. This pull request is T1 by path (core/test/**, patches/**), so the gate already lets it merge without a verdict; this comment is the review record, not a gate.

1. The wrapper is installed inside the try. core/test/connectors.test.js now has let renameCalls = 0, let second, t.after(() => second?.ctx.dispose()) before the try; the fsp.rename = … / syncBuiltinESMExports() pair and second = setup({ home, warns }) are the first three statements inside it; finally is untouched (fsp.rename = realRename + syncBuiltinESMExports()); the case ends with assert.ok(renameCalls >= 1, 'the wrapper never ran — store no longer uses the named rename import'). The optional chain on second is what keeps the after-hook safe when setup() throws — I checked that path with a probe (below), not by reading it.

2. Red without the source change, reproduced on the two shapes:

• Leak probe. A copy of the case with second = setup(…) replaced by a throw, plus a later test asserting fsp.rename.name === 'rename':
  • main's shape → the later test is not ok, the wrapper leaked into later tests (anonymous async wrapper left on fsp.rename);
  • this tip → the throwing case fails for its own reason and the later test is ok — the finally restored.
• Alternate rename path. core/src/host/store.js writing through promisify(renameCb) imported from node:fs (only that probe edit, restored afterwards): main's test 29/29 pass, silently (the wrapper never called and nothing said so); this tip is not ok 24 … error: 'the wrapper never ran — store no longer uses the named rename import'.
• core/src/host/store.js is byte-identical to main after the probe, and the comment's claim is true of it: rename is the named import at :5, used at :488, and SLOW_WRITE_MS = 400 (:593) — setTimeout(_, 0) would still call the wrapper, so the count pins the path, not the delay.

3. 0001 was hand-edited, and the edit is consistent. This machine has no git, so generate could not run — I checked the result three ways:

• every hunk body in test/connectors.test.js carries exactly the counts its header declares, @@ -165,10 +165,614 @@ included: 10 old-side and 614 new-side lines, counted from the body;
• the 614 rebuilt new-side lines are byte-identical to core/test/connectors.test.js:210-823 (the whole tail of the file);
• check third line is ok core/ equals ds-bot/ + patches/ds-bot/, i.e. the applied series really produces that file.

One shape worth recording rather than flagging: in the stored series the new-side start repeats the old-side start on every hunk (291/291 in 0001, 68/68 in 0002), so +165 is not the position in the output file — both appliers place hunks by the old side, and the block lands at line 210. That is how the series is stored here and is not introduced by this pull request.

4. Nothing else changed. pulls/1262/files is exactly core/test/connectors.test.js (+18/−7) and patches/ds-bot/0001-core-fixes.patch (+19/−8); core/src/host/store.js and both #1215 diagnostic lines are untouched. Diffed against current main (93257618, #1256 landed after this branch was cut), the only two files beyond that commit's own set are those two.

5. Body and checkpoint. The six sections are present, Tip SHA and checkpoint checks.at are this sha, checkpoint pr: 1262, files: 2 (both in the diff), Fixes #1224, checkpointReasons() → [].

CI at read: 14 runs, 13 success; the one non-success is an agent-audience run cancelled at 08:15:37Z, replaced by the same check's success at 08:15:48Z — a duplicate context, not red. enqueue-guard is green at this tip, as T1 allows.

Ran:

$ TZ=UTC DSH_HARNESS=/data/dsh/bot/workspace/p759/hrc2 node --test --import ./core/test/harness-resolve.mjs --test-concurrency=1 core/test/connectors.test.js
# tests 29 / pass 29 / fail 0
$ node scripts/ds-bot-patches.mjs check
skipped pin (no git): the tree hash cannot be computed here
ok   10 patches apply cleanly in order
ok   core/ equals ds-bot/ + patches/ds-bot/
$ node /data/dsh/bot/workspace/main5f15/guard1262.mjs
enqueue-guard: this guard never enqueues; run the command yourself, now:
  gh pr merge 1262 --auto --squash --match-head-commit 32617dcac4fc3a134d0176991fe2ee4fe67f5514
OK true | enqueue #1262: no `hold` and the gate passes

The probe copies (zz-*.test.js) and the store.js probe edit were removed after the runs; the working tree is back to the tip.