Skip to content

fix(fs-local): tolerate number-mode stats from Electron asar fs (restores skill catalog) - #737

Open
sakikoTGW wants to merge 1 commit into
dataelement:mainfrom
sakikoTGW:fix/fs-local-asar-stat-mode
Open

sakikoTGW wants to merge 1 commit into
dataelement:mainfrom
sakikoTGW:fix/fs-local-asar-stat-mode

Conversation

@sakikoTGW

Copy link
Copy Markdown

Problem

Fixes the empty skill catalog on the packaged desktop (#696).

Electron's asar-backed fs answers stat(path, { bigint: true }) with a number-mode Stats for paths inside app.asar. The cordis preset declares its skill root at app.asar/node_modules/@deepseek-ai/dsh-agent-preset/skills, so @deepseek-ai/dsh-fs-local probes it through ctx.fs and computes:

mode: Number(info.mode & 0o777n)   // Cannot mix BigInt and other types

That throws inside listSkillRootEntriesFromFileSystem; the skill registry drops the whole filesystem provider, and every local skill (including $DSH_HOME/skills) disappears.

Change

  • patches/@deepseek-ai+dsh-fs-local+0.2.0-rc.2.patch — normalize the mode before masking in both probe() and probeNoFollow(): Number(BigInt(info.mode) & 0o777n). A real Node stat stays a bigint; an asar number-mode stat no longer throws.
  • test/fs-local-asar-stats-patch.test.ts — regression test: asserts postinstall applied the patch (no info.mode & 511n left, exactly two normalized sites), the patch file still targets the pinned version, and the normalized expression accepts both number and bigint modes.

Patch bookkeeping (per patches/AGENTS.md)

  • Upstream package/version: @deepseek-ai/dsh-fs-local@0.2.0-rc.2 (pinned in package.json)
  • Upstream status: the root cause is upstream, but deepseek-ai/deepseek-harness does not accept external pull requests at the moment, so this stays a desktop compatibility patch
  • Regression test: test/fs-local-asar-stats-patch.test.ts (fails if postinstall drops the patch, or a Harness upgrade changes the probe shape without re-validation)
  • Removal condition: remove together with the version bump once the bundled dsh-fs-local ships a probe()/probeNoFollow() that handles number-mode stats (check the published lib/index.js after a Harness upgrade)

Verification

  • Patch regenerated with patch-package against a pristine @deepseek-ai/dsh-fs-local@0.2.0-rc.2 install; npx patch-package applies cleanly on a fresh install.
  • Patched lib/index.js contains two Number(BigInt(info.mode) & 511n) sites and zero Number(info.mode & 511n) sites.
  • Both shapes verified: permissionBits(0o100644) and permissionBits(0o100644n) → 0o644.
  • Full desktop build was not run in this environment; the patch only touches two expressions in the packaged dependency, and npm ci + postinstall re-applies it.

Known limitation

versionOf() still composes mtimeNs/ctimeNs as undefined for asar paths. The preset skill directory is read-only, so that only weakens the version token, not behavior; out of scope for this fix.

Electron's asar-backed fs answers stat(path, { bigint: true }) with a
number-mode Stats object for paths inside app.asar. The cordis preset
declares its skill root at app.asar/node_modules/.../dsh-agent-preset/skills,
so dsh-fs-local's probe()/probeNoFollow() mixed a bigint mask with a number
and threw "Cannot mix BigInt and other types". The skill registry then
dropped the whole filesystem provider and every local skill disappeared
(dataelement#696).

Patch both probe paths to normalize the mode with BigInt() before masking.
Real Node stats are unchanged; an asar number-mode stat no longer throws.

The official DSH repository does not accept external pull requests right
now, so this stays a desktop compatibility patch. Remove it once the
bundled Harness version fixes probe()/probeNoFollow().
@sakikoTGW

Copy link
Copy Markdown
Author

补充验证:根因已定位到 Electron 版本差异(见 issue #696 最新评论)——本补丁是 Electron 43 下 fs.stat(..., { bigint: true }) 返回 number-mode Stats 的兼容修复;官方 Electron 44.7 的 asar fs 已返回 bigint。补丁本身已在干净安装的 @deepseek-ai/dsh-fs-local@0.2.0-rc.2 上用 patch-package 应用验证通过。

@rahul05ranjan rahul05ranjan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the failure mode: Electron's asar layer returns a number-mode Stats for in-archive paths, and info.mode & 511n throws Cannot mix BigInt and other types, so the probe — and with it the whole filesystem skill provider — silently disappeared. Wrapping in BigInt(...) at both probe sites is the minimal correct fix.

The tests are doing double duty the right way: asserting the patched shape is still present in node_modules (so a lost postinstall or a Harness upgrade that reverts the patch fails loudly) and asserting the shipped patch file matches the pinned 0.2.0-rc.2 version. The extracted-expression test documenting that both stat shapes pass once normalized is the right level of paranoia.

Two thoughts:

  • The patch is pinned by version in its filename; when the Harness moves past rc.2 (or the package lands the fix upstream) patch-apply will fail the install. The second test catches it after the fact, but it's worth a heads-up wherever dependency bumps get reviewed so whoever touches dsh-fs-local first checks whether the patch is still needed.
  • BigInt(mode) on a number is only safe for integral values; stat modes are always integral, so no concern in practice.

@sakikoTGW

Copy link
Copy Markdown
Author

Thanks. Good catch on the pin, and you're right that it's worth writing down.

I'd keep this patch on our side rather than upstream's: { bigint: true } is a Node
contract, and the asar fs didn't honor it until Electron 44.7. So the tolerant side
of that mismatch is the shell, and I don't expect a harness change to unblock us.

Checked it hasn't gone stale either: 0.2.1-rc.1 and 0.2.1-rc.2 ship the same
lib/index.js as 0.2.0-rc.2 (same hash), mode & 511n still unguarded at both
call sites.

On naming: the whole bundled harness is pinned to 0.2.0-rc.2 here (220 packages, no
ranges), and 25 of the 28 existing patches use the same +<version> suffix. So a
bump is renaming the patch file, not rebuilding it. I can add "bump -> re-key the
patch filename" to the test failure message if you want that spelled out.

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.

2 participants