Skip to content

chore(release): prepare ByteFolk mem-mcp 0.1.2 - #193

Closed
PeterGuy326 wants to merge 1 commit into
mainfrom
codex/mem-153-bytefolk-release
Closed

PeterGuy326 wants to merge 1 commit into
mainfrom
codex/mem-153-bytefolk-release

Conversation

@PeterGuy326

@PeterGuy326 PeterGuy326 commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Refs #153.

Rebased onto current main after #216. Windows installer EPERM lock retry is already on main; this PR only migrates identity to @bytefolk/mem-mcp@0.1.2 and the ByteFolk cache namespace.

Head: 1064d70. Does not close release acceptance issues.

Comment thread npm/migration.test.js Fixed
Comment thread npm/migration.test.js Fixed
Comment thread scripts/npm-release.mjs Fixed
Comment thread scripts/npm-release.mjs Fixed
@PeterGuy326

Copy link
Copy Markdown
Collaborator Author

Fresh-head security followup: 79ec49579b444b5c02ef3f2cff772780c2d95c26 (tree 2294aaea11990eaa0f07a9cfac549f9f5e68bb9e). Addresses all four CodeQL findings from head 51ec with descriptor-based asset/snapshot reads and a local-facts-only receipt. Independent review caught an intermediate checksum/build-metadata object substitution; both current-asset and next-asset replacements reproduced red, then passed after binding Go metadata to the same inherited file descriptor. Bounded independent re-review PASS; release regressions 85/85, full npm 78 passed / 1 Windows-only skip, migration 33/33, release guards and Bash compatibility PASS. No alert suppression or dismissal. Remote CI on this exact head is now required; old-head green checks do not transfer. Draft remains: separate Web audit fix #192 needs review/integration, and human approval plus release-owner/RC/npm ownership gates are not satisfied. No main merge, registry publish, tag or release has been performed.

@PeterGuy326

Copy link
Copy Markdown
Collaborator Author

Final CI readback for 79ec49579b444b5c02ef3f2cff772780c2d95c26: Go/race/build, PostgreSQL/race integration, Worker, npm/Node18/Node20/Windows24, browser/lifecycle acceptance, deployment profiles, release guards and all CodeQL checks passed. The CodeQL aggregate reports zero new alerts (check 102753511526, annotations_count=0); the four prior-head findings were fixed, not dismissed. Only Web failed: the current job log confirms js-yaml GHSA-2883-xcg3-v3hh high severity. Separate #192 is green but still REVIEW_REQUIRED; review/integrate that candidate, then update and revalidate/re-review this PR on its resulting head. Human review remains required here; draft retained. No merge to main or npm/GitHub release occurred. Owner authentication, aligned RC bootstrap and the documented release gates remain outstanding.

@sun-970

sun-970 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Code Review

Overall this is a thorough and well-structured migration PR. The naming migration is consistent across all surfaces, the release pipeline has strong defense-in-depth (OIDC with no token fallback, fd-based TOCTOU prevention, isolated npmrc), and test coverage is extensive. Below are the findings worth addressing.

Medium

  1. docs/maintainers/releasing.md#122 cross-reference inconsistent with proof validator
    The releasing guide mentions #153/#122 as the governing issues, but checkProof's evidence regex only accepts #153 or #22. If evidence is recorded on #122, the automated proof check will reject it. Likely #122 should be #22.

  2. npm/install.js:782-797 — TOCTOU race between assertSeparateLegacyEntry and lock acquisition
    assertSeparateLegacyEntry calls statSync on both binPath and the legacy source before ensureCacheDirectory / acquireAssetLock. A concurrent process could create a hardlink between the inode check and lock acquisition. The fast-fail guards are good, but the security-critical invariant should be re-verified under the lock.

  3. npm/migration.test.js:1231-1258 — Symlink test only covers dangling symlinks
    For kind === "symlink", the target path is never created, so only the dangling case is tested. A symlink pointing to a valid file with correct content would pass verifyFile and be copied — contradicting the README statement that "symlinked legacy entries fall back to the normal verified download." Adding a writeFileSync(preserved, f.bytes) before symlinkSync would cover this case.

  4. 0.1.2-rc.0 bootstrap dependency is a hard blocker, not prominently surfaced
    checkRegistryBefore requires data.versions['0.1.2-rc.0'] to exist on npm before stable publication. This is documented as "NOT VERIFIED" in the PR description, but the workflow will fail at runtime if this prerequisite is not met. This ordering constraint should be called out more prominently in the releasing docs, not just in a parenthetical.

Low

  1. npm/install.js:810-823 — Abort signal handling can misclassify error types
    The condition (signal && signal.aborted) || err.name === "AbortError" can misclassify: if signal.aborted is true but the error came from a non-abort cause (e.g. EACCES), the non-abort error is re-thrown as if it were an abort.

  2. npm/install.js:855-861 — Explicit cacheDir skips parent-directory overlap check
    When options.cacheDir is set explicitly, only [cacheDir] is passed as destination paths. The parent directory is not included, unlike the default-root path. Mitigated by pathContains resolution, but a defense-in-depth gap.

  3. scripts/npm-release.mjs:1659GITHUB_EVENT_PATH read without existence check
    readJSON(env.GITHUB_EVENT_PATH) will throw a generic error if the file is missing or malformed. An explicit validation would give a clearer failure message in CI.

  4. Docs: "npm 12" references vs npm@11.15.0 pin
    The workflow pins npm@11.15.0 and checkContext requires >=11.15.0, but the releasing guide references "npm 12 clean-tarball test." A clarifying note that these describe compatibility testing (not the pinned CI version) would avoid confusion.

Positive observations

  • The naming migration is fully consistent — no stale @fullstack-ai-infra references in production code, and all version surfaces align to 0.1.2.
  • The fd-based TOCTOU prevention in npm-release.mjs (single descriptor for path replacement, metadata/checksum binding) is a solid pattern.
  • Shell script portability fixes (-exec basename {} \;, :- for empty arrays, Bash 3.2 compat) are correct and well-tested.
  • Test coverage for partial publication failures, alias detection at multiple path levels, and foreign-platform selection is comprehensive.

@waterbro-8

Copy link
Copy Markdown
Collaborator

Not a review, not an approval, carries no vote. This comment is about one red check on the current head and nothing else. It does not ask anyone to merge, unblock, re-run, or close anything. It is also not a reply to the code review already on this thread (2026-09-11T02:41:59Z): that comment contains zero occurrences of EPERM, mkdir, node24, or the failing test's name, so the two cover different ground.

Non-independence, stated up front: the code this concerns (isLockContention / reclaimStaleLock in npm/install.js) reached main through PR #137, and the merge commit on that PR was authored by this account. So nothing below is an independent review of that code — I am reporting a CI observation I was able to pin down, not certifying a design.

All readings below are live against bytefolk/mem as of the moment this comment was posted, taken from the API and from job logs downloaded by job id.

1. What is red

Current head: bdd1da35ff4ac9d76f333a1f80322f9d5258e617. Enumerating commits/<head>/check-runs and taking the highest check_suite.id per name gives 20 check names, 19 success. The single non-success is:

  • name npm wrapper compatibility (node24-windows), check-run id and job id 102937149929, in run 34496784723, run_attempt = 1, conclusion = failure.

From that job's own log (runner Image: windows-2025-vs2026, Microsoft Windows Server 2025, node: v24.19.0, npm: 11.17.0, test command node --test install.test.js mem-mcp.test.js migration.test.js registry-identity.test.js windows-shim.test.js):

ℹ tests 79
ℹ pass 72
ℹ fail 1
ℹ skipped 6
✖ failing tests:
test at install.test.js:394:1
✖ independent processes contend on one lock and download one binary (533.009ms)
  Error: worker 3192 failed: code=1 signal=null
  Error: EPERM: operation not permitted, mkdir 'C:\Users\RUNNER~1\AppData\Local\Temp\mem-mcp-install-test-0VjlPh\cache\.mem-mcp-windows-amd64.exe.lock'
      at mkdirSync (node:fs:1411:26)
      at acquireAssetLock (D:\a\mem\mem\npm\install.js:386:7)
      at async install (D:\a\mem\mem\npm\install.js:762:10)
      at ChildProcess.<anonymous> (D:\a\mem\mem\npm\install.test.js:76:16)

Other lock tests from the same file pass in that same log, including the two that exercise the same branch of the code:

✔ a contended Windows lock reported as EPERM waits for a proven lock (129.205ms)
✔ a persistent Windows EPERM without a lock fails promptly instead of retrying (89.4024ms)

2. This is not a regression introduced by this head

The failing test and the code it drives come from two files whose contents are byte-identical on four different heads of this branch, three of which produced a green Windows leg:

head npm/install.js blob npm/install.test.js blob node24-windows job that test
f7732714781ca83dccac07e0b002711691ae9a48 36cce894d12af3567544d57069d55520f1e5afe5 a89b6484bd3d0ffe6745bf6fceccc0f95b5d0418 102749088813 success ✔ … (662.0254ms), pass 73 / fail 0
51ec892928659ff3385f48bda38fe23027423f00 same same 102749947600 success ✔ … (586.0301ms), pass 73 / fail 0
79ec49579b444b5c02ef3f2cff772780c2d95c26 same same 102753349188 success ✔ … (761.3076ms), pass 73 / fail 0
bdd1da35ff4ac9d76f333a1f80322f9d5258e617 same same 102937149929 failure ✖ … (533.009ms), pass 72 / fail 1

Every one of those five jobs is run_attempt = 1; none was retried. (I am comparing the job conclusion for this leg only — the overall conclusion of all five runs is failure, so run-level green is not the unit here.)

And the head commit itself adds no candidate cause:

bdd1da35ff4ac9d76f333a1f80322f9d5258e617  "Merge branch 'main' into codex/mem-153-bytefolk-release"
  author.login = PeterGuy326   committer.login = web-flow
  parents: 79ec49579b444b5c02ef3f2cff772780c2d95c26, 87db0dfe0507be2190fe2fdcce0e267be8224f4d
  files: web/package-lock.json +50 -50

So the delta between the last green Windows leg on this branch and the red one is a merge of main that touches one lockfile in web/. Same unit of comparison as §1: the Windows job was green on 79ec49579b44, which is the first parent of this merge commit.

3. Do not conflate it with the branch's earlier red

This branch has exactly two non-green Windows jobs, and they are different failures:

  • job 102747589414 (run 34438202854, head bc0f73345774187f17c8e797bc2b3d4430f6d4a2, attempt=1) failed 11 tests with Error: home directory must be an absolute path: C:\Users\runneradmin, and its independent processes contend on one lock and download one binary passed. That one was addressed by f7732714781ca83dccac07e0b002711691ae9a48fix(npm): resolve protected cache paths on the running host.
  • job 102937149929 (§1) fails 1 test, the contention test, with no home-directory error anywhere in its log.

Treating the two as the same red would make the second look like an unfixed remnant of the first.

4. What the failing path is, in the shipped code

The contract the code declares for itself, verbatim from npm/install.js:318-320 on this head:

// Windows reports EPERM/EACCES rather than EEXIST when another process already
// owns the lock directory or is mid-create on it, so either code is ordinary
// contention there and must not be mistaken for a hard permission failure.

and the two functions that have to honour it — npm/install.js:321-323:

function isLockContention(err, currentPlatform = platform()) {
  if (err.code === "EEXIST") return true;
  return currentPlatform === "win32" && (err.code === "EPERM" || err.code === "EACCES");
}

and npm/install.js:326-337:

function reclaimStaleLock(lockPath, cacheDir, asset, staleMs, orphanGraceMs, mkdirError) {
  let info;
  try {
    info = lstatSync(lockPath);
  } catch (err) {
    // An EEXIST result followed by ENOENT means a competing owner released
    // the lock before inspection. It is safe to retry, but it must take the
    // normal deadline/delay path rather than spin synchronously. A Windows
    // EPERM/EACCES without a lock to inspect remains a real permission failure.
    if (err.code === "ENOENT" && mkdirError.code === "EEXIST") return;
    if (err.code === "ENOENT") throw mkdirError;
    throw err;
  }

On the acquisition loop (npm/install.js:371, mkdirSync at :386), a win32 EPERM from mkdir is accepted as ordinary contention at :399:

      if (!isLockContention(err, osPlatform)) throw err;
      mkdirError = err;

then handed to reclaimStaleLock(...) at :407. The two "lock vanished" arms are not symmetric: only mkdirError.code === "EEXIST" is routed onto the retry path at :335. A contender whose mkdir failed with EPERM, which :318-320 explicitly says is normal contention on Windows, reaches :336 and is re-thrown as a hard failure.

That is the shape the log shows. install() resolves osPlatform from os.platform() (:701, passed through at :762), so on that runner :399 cannot be the throw site for EPERM; and :337 would have surfaced an lstat error text rather than ... mkdir '...'. :336 is the remaining exit. The frame at acquireAssetLock (…install.js:386:7) is consistent with it: re-throwing a preserved Error object does not add a frame, so the reported line is the error's construction site, not its throw site.

The concrete race: twelve workers runWorker at once (npm/install.test.js:394, fan-out at :433). My reading of the artifact is that a process calling mkdir in the window where the holder has removed the lock directory but the OS has not finished the delete sees EPERM, not EEXIST. Note that this is the case :318-320 does not name — the comment only claims "already owns" and "mid-create" — so it is my hypothesis about which window was hit, not something the code or the log states. Either way isLockContention at :321-323 classifies it as ordinary contention, lstatSync then either succeeds against the pending-deletion directory or throws ENOENT while mkdirError.code is EPERM, and the second arm at :336 re-throws it. The loser dies with a raw EPERM instead of taking the :408-411 deadline/poll path.

These lines are not from this PR. The same three arms are on main at npm/install.js:261-263 (matching :335-337 here), blob 4215cc747f37e449fbddceddcbace3b6999330f4 @ 3e8acaeb1b8c5a46a00a63beacc3efc9299474d5, with isLockContention at :247 and mkdirSync at :312. The same twelve-worker test is on main at npm/install.test.js:372 / :411, blob 49aa4e54de266b22e4848152e8c512103a48d4ab. #193 is where this surfaced in CI, not where it was introduced, and a fix probably belongs on main rather than inside a release-preparation PR.

5. Why I would not call this a one-line fix

There is a passing test that pins the opposite behaviour, and it is pinned on two properties at once — npm/install.test.js:970, test("a persistent Windows EPERM without a lock fails promptly instead of retrying"). Its worker makes mkdirSync throw { code: "EPERM", syscall: "mkdir" } for every .lock target, and asserts:

        if (error.code !== "EPERM") throw error;
        if (Date.now() - startedAt >= 1000) throw new Error("permission error entered the retry loop");

with waitTimeoutMs: 10000. So from the loser's side, a genuine ACL problem and a just-released lock are both EPERM at mkdir followed by ENOENT at lstat, and one of them must fail fast with error.code === "EPERM" while the other must retry. Any change that routes EPERM + ENOENT onto the retry loop makes that test wait 10 s and exit through npm/install.js:409, whose error is Timed out waiting for mem-mcp cache lock: ${lockPath} — so code is undefined, not EPERM, and the timing assertion breaks too. That is why "distinguish delete-pending from no-permission" is the actual question here, and I have not answered it.

6. What I am not claiming, and how rare this is

Not claiming:

  • I did not reproduce this locally or on any Windows machine. The mechanism in §4 is a reading of the shipped source against the artifact in the log, not an observation of the interleaving.
  • I have no opinion on which fix is right, or on whether it should land in chore(release): prepare ByteFolk mem-mcp 0.1.2 #193 at all — per §4 the defect is on main, so chore(release): prepare ByteFolk mem-mcp 0.1.2 #193 may be the wrong place for it.
  • I did not test whether the window is specific to windows-2025-vs2026 runner temp directories.
  • I am not asserting this affects end users. The exposed path is a first-run concurrent install into a cold cache directory.

Measured rarity, from actions/runs?created=>=2026-09-01 and each run's jobs (latest attempt of each run; CI run_attempt histogram {1: 131, 2: 19, 3: 1}), window 2026-09-01T02:08:43Z .. 2026-09-11T03:36:50Z:

  • 151 CI runs, 127 node24-windows jobs, 2 non-success — both on this branch, and only one of the two is this EPERM case.

The operational consequence is worth stating plainly: because it is 1-in-127, a re-run will very likely go green and this evidence will disappear from the branch history, leaving the raw-EPERM exit on main unfixed and unreported. On bdd1da35ff4a… there is no CI signal left to look at after that.

The author's own standard on this thread, verbatim (PeterGuy326, 2026-09-10T05:13:07Z):

Remote CI on this exact head is now required; old-head green checks do not transfer

I am holding to that in the direction that cuts against me: nothing here substitutes for a fresh CI run on bdd1da35ff4a…, and I am not asking anyone to accept my analysis in place of one.

7. Writes performed by this comment

None beyond posting it. No review submitted, no vote, no merge or close, no label / assignee / requested-reviewer change, no auto-merge toggle, no Update branch, no ref moved, no tag, no npm publish.

Live PR state as read when this was posted: state=open, draft=true, merged_at=null, closed_at=null, head bdd1da35ff4ac9d76f333a1f80322f9d5258e617, base 87db0dfe0507be2190fe2fdcce0e267be8224f4d, mergeable=false, mergeable_state=dirty, 5 commits / 30 files / +1561 −108, requested_reviewers=[], and the only review on the PR is github-advanced-security[bot] COMMENTED @ bc0f73345774.

@waterbro-8

Copy link
Copy Markdown
Collaborator

Draft + conflicting + Windows npm failure on the last recorded run. Release PR also needs npm org credentials (issue #153). Not merging.

@waterbro-8

Copy link
Copy Markdown
Collaborator

#193 is still DIRTY vs main. Successor with the rebase + G5 OIDC workflow: #222

No tag and no registry write from that PR. Please close this draft as superseded if 222 is the vehicle.

Rebase the identity migration onto current main. Windows installer EPERM
lock retry is already on main from #216; this commit only moves package
name, version, cache namespace, and matching docs/tests to ByteFolk 0.1.2.
@waterbro-8
waterbro-8 force-pushed the codex/mem-153-bytefolk-release branch from bdd1da3 to 1064d70 Compare September 18, 2026 09:41
@waterbro-8
waterbro-8 marked this pull request as ready for review September 18, 2026 09:42
@waterbro-8

Copy link
Copy Markdown
Collaborator

已换基到当前 main(含 #216 的 Windows EPERM 锁重试)。旧 head 上 node24-windows 的失败是 `acquireAssetLock` mkdir EPERM,main 已修。

本 PR 现在只做 ByteFolk 0.1.2 身份迁移。请审。

@waterbro-8

Copy link
Copy Markdown
Collaborator

关闭以免和 #222 双开同一份 ByteFolk 0.1.2 身份迁移。

#222#193 换基到当前 main 的后续,并多了 MCP Registry OIDC(G5)。Windows EPERM 已在 main/#216。请审 #222 而不是本 PR。

本 head `1064d70` 只做了身份字符串,可作对照,不再推进。

@waterbro-8 waterbro-8 closed this Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants